Merged
Conversation
Add a detailed execplan document outlining the implementation of machine-readable diagnostics mode (`--diag-json`) for netsuke. This roadmap item `3.10.3` describes the JSON schema, constraints, risks, progress, and plans for integration and testing, aiming to enable a stable JSON diagnostic output alongside traditional human-readable diagnostics. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements a Netsuke-owned, versioned JSON diagnostics mode triggered via a new diag_json configuration flag, wiring it through startup, CLI parsing, runtime error handling, and runner output suppression, along with documentation, localization, and test coverage (unit, integration, and BDD) plus an ExecPlan doc and roadmap updates. Sequence diagram for JSON diagnostics error handling with diag_jsonsequenceDiagram
actor User
participant Cli as cli::Cli
participant Locale as locale_resolution
participant Main as main_rs
participant Runner as runner::run
participant DiagJson as diagnostic_json
participant Stderr as stderr
User->>Main: main()
Main->>Main: args = collect_args()
Main->>Locale: resolve_startup_diag_json(args, env)
Locale-->>Main: diag_json_hint (bool)
Main->>Main: localizer = startup_localizer(args, env, system_locale)
Main->>Cli: parse_cli_or_exit(args, localizer, diag_json_hint)
alt clap parse error
Cli-->>Main: Err(ExitCode) via emit_json_result
Main->>Stderr: JSON diagnostics (render_error_json)
Main-->>User: ExitCode::FAILURE
else parse ok
Cli-->>Main: (parsed_cli, matches)
Main->>Main: requested_diag_json = diag_json_hint || parsed_cli.diag_json
Main->>Cli: merge_cli_or_exit(&parsed_cli, &matches, requested_diag_json)
alt config merge error
Cli-->>Main: Err(ExitCode) via emit_json_result
Main->>Stderr: JSON diagnostics (render_error_json)
Main-->>User: ExitCode::FAILURE
else merge ok
Cli-->>Main: merged_cli
Main->>Main: configure_runtime(&merged_cli, system_locale)
Main->>Runner: run(&merged_cli, output_prefs)
alt Runner returns Ok
Runner-->>Main: Ok(())
Main-->>User: ExitCode::SUCCESS
else Runner returns Err
Runner-->>Main: Err(anyhow::Error)
Main->>Main: handle_runner_error(err, prefs, merged_cli.diag_json)
alt merged_cli.diag_json == true
Main->>DiagJson: render_runtime_error_json(&err)
DiagJson-->>Main: serde_json::Result<String>
Main->>Main: emit_json_result(result)
Main->>Stderr: single JSON document
Main-->>User: ExitCode::FAILURE
else merged_cli.diag_json == false
Main->>Stderr: human-readable diagnostics with prefixes
Main-->>User: ExitCode::FAILURE
end
end
end
end
Class diagram for diagnostic_json JSON diagnostics schema and serializerclassDiagram
class DiagnosticDocument {
+u32 schema_version
+GeneratorInfo generator
+Vec~DiagnosticEntry~ diagnostics
+from_diagnostic(diagnostic: Diagnostic) DiagnosticDocument
+from_error(error: StdError) DiagnosticDocument
}
class GeneratorInfo {
+&'static str name
+&'static str version
+current() GeneratorInfo
}
class DiagnosticEntry {
+String message
+Option~String~ code
+&'static str severity
+Option~String~ help
+Option~String~ url
+Vec~String~ causes
+Option~DiagnosticSource~ source
+Option~DiagnosticSpan~ primary_span
+Vec~DiagnosticSpan~ labels
+Vec~DiagnosticEntry~ related
+from_diagnostic(diagnostic: Diagnostic) DiagnosticEntry
+from_error(error: StdError) DiagnosticEntry
}
class DiagnosticSource {
+String name
}
class DiagnosticSpan {
+Option~String~ label
+usize offset
+usize length
+u32 line
+u32 column
+u32 end_line
+u32 end_column
+Option~String~ snippet
}
class diagnostic_json_module {
+render_report_json(report: Report) Result~String~
+render_diagnostic_json(diagnostic: Diagnostic) Result~String~
+render_error_json(error: StdError) Result~String~
+render_runtime_error_json(err: anyhow::Error) Result~String~
+emit_json_result(result: Result~String~) ExitCode
}
DiagnosticDocument --> GeneratorInfo : has
DiagnosticDocument --> "1..*" DiagnosticEntry : diagnostics
DiagnosticEntry --> DiagnosticSource : source
DiagnosticEntry --> "0..*" DiagnosticSpan : labels
DiagnosticEntry --> DiagnosticSpan : primary_span
DiagnosticEntry --> "0..*" DiagnosticEntry : related
diagnostic_json_module ..> DiagnosticDocument : builds
diagnostic_json_module ..> DiagnosticEntry : converts from diagnostics
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdd a gated machine‑readable JSON diagnostics mode (cli/config/env) and a new public Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI Arg Parser"
participant Startup as "Startup Resolver"
participant Config as "Config Merge"
participant Runner as "Runner / Reporter"
participant Diag as "diagnostic_json"
participant IO as "stderr / stdout"
User->>CLI: invoke with --diag-json
CLI->>Startup: raw‑arg hints (diag_json_hint_from_args)
Startup->>Startup: resolve_startup_diag_json (CLI -> ENV -> default)
Startup->>Config: propagate diag_json into merge
Config->>Runner: configure runner (diag_json flag)
Runner->>Runner: run tasks (suppress progress if diag_json)
alt Error occurs (startup or runtime)
Runner->>Diag: build DiagnosticDocument (render_*_json)
Diag->>IO: emit single JSON document to stderr
IO->>User: JSON diagnostics on stderr
else Success
Runner->>IO: stream stdout artefacts (manifest/graph)
IO->>User: stdout contains artefacts, stderr empty
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
…mode This introduces a new CLI flag and configuration option --diag-json for enabling a stable, versioned JSON diagnostics output on stderr suitable for automation and editor integration. The implementation includes: - A dedicated `diagnostic_json` module owning the Netsuke JSON diagnostic schema. - Integration of JSON diagnostic rendering for startup failures, configuration errors, and runtime errors. - Suppression of human-oriented stderr output during JSON diagnostics mode to keep stderr machine-readable. - Preserving stdout behaviour for normal command output even when JSON diagnostics is enabled. - Support for layering JSON diagnostic flag through CLI, environment variables (`NETSUKE_DIAG_JSON`), and configuration files. - Comprehensive unit, integration, and BDD test coverage ensuring schema stability and feature correctness. - Documentation updates including user guide, design notes, roadmap, localization, and CLI help texts. This feature enhances automation scripting and editor integration by providing a consistent machine-readable error reporting mechanism. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Changed the type of the `related` field in `DiagnosticEntry` from `Vec<DiagnosticEntry>` to `Vec<Self>` to improve code clarity and maintainability by using a recursive type reference. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 8 issues, and left some high level feedback:
- There are now two separate
parse_bool_hintimplementations (incli_l10n.rsandlocale_resolution.rs); consider centralizing this helper in a shared module to avoid divergence in how boolean hints are interpreted across flags. - In
fallback_json_payload, the generator version is hard-coded to"0.1.0"instead of usingenv!("CARGO_PKG_VERSION")like the primary path, which could lead to misleading version info if the crate version changes; consider reusing the same constant there.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are now two separate `parse_bool_hint` implementations (in `cli_l10n.rs` and `locale_resolution.rs`); consider centralizing this helper in a shared module to avoid divergence in how boolean hints are interpreted across flags.
- In `fallback_json_payload`, the generator version is hard-coded to `"0.1.0"` instead of using `env!("CARGO_PKG_VERSION")` like the primary path, which could lead to misleading version info if the crate version changes; consider reusing the same constant there.
## Individual Comments
### Comment 1
<location path="src/main.rs" line_range="144-153" />
<code_context>
+ ExitCode::FAILURE
+}
+
+fn render_runtime_error_json(err: &anyhow::Error) -> serde_json::Result<String> {
+ if let Some(runner_err) = err.downcast_ref::<runner::RunnerError>() {
+ return diagnostic_json::render_diagnostic_json(runner_err);
+ }
+ if let Some(manifest_err) = err
+ .chain()
+ .find_map(|cause| cause.downcast_ref::<manifest::ManifestError>())
+ {
+ return diagnostic_json::render_diagnostic_json(manifest_err);
+ }
+ diagnostic_json::render_error_json(err.as_ref())
+}
+
</code_context>
<issue_to_address>
**suggestion:** Consider preferring `miette` diagnostics when present instead of always falling back to `render_error_json`
This always falls back to `render_error_json` for non-`RunnerError` / `ManifestError` failures, dropping any span/label data from errors that implement `miette::Diagnostic`. Consider detecting `Diagnostic` errors (e.g. via `err.downcast_ref::<dyn Diagnostic>()` or a concrete diagnostic type) and using `render_diagnostic_json` when available to keep JSON output richer and consistent with other error paths.
Suggested implementation:
```rust
fn render_runtime_error_json(err: &anyhow::Error) -> serde_json::Result<String> {
// Prefer rich `miette` diagnostics when available.
// 1. Known structured diagnostics from our own types.
if let Some(runner_err) = err.downcast_ref::<runner::RunnerError>() {
return diagnostic_json::render_diagnostic_json(runner_err);
}
if let Some(manifest_err) = err
.chain()
.find_map(|cause| cause.downcast_ref::<manifest::ManifestError>())
{
return diagnostic_json::render_diagnostic_json(manifest_err);
}
// 2. Any `miette::Report` in the error chain (implements `miette::Diagnostic`).
if let Some(report) = err
.chain()
.find_map(|cause| cause.downcast_ref::<Report>())
{
return diagnostic_json::render_diagnostic_json(report);
}
// 3. Fallback: plain error JSON without diagnostic spans/labels.
diagnostic_json::render_error_json(err.as_ref())
}
```
This implementation assumes `miette::Report` is already imported (which it is in the provided snippet).
If you have other common concrete diagnostic wrapper types beyond `Report`, you can extend the second `chain().find_map(...)` block with additional downcasts in the same style as for `ManifestError`.
</issue_to_address>
### Comment 2
<location path="src/diagnostic_json.rs" line_range="190-218" />
<code_context>
+ causes
+}
+
+fn extract_source_and_labels(
+ diagnostic: &dyn Diagnostic,
+) -> (Option<DiagnosticSource>, Vec<DiagnosticSpan>) {
+ let Some(labelled_diagnostic) = diagnostic_with_labels(diagnostic) else {
+ return (None, Vec::new());
+ };
+ let Some(source_code) = labelled_diagnostic.source_code() else {
+ return (None, Vec::new());
+ };
+ let Some(labels) = labelled_diagnostic.labels() else {
+ return (None, Vec::new());
+ };
+
+ let mut source = None;
+ let spans = labels
+ .filter_map(|label| {
+ let span = build_span(&label, source_code)?;
+ if source.is_none() {
+ source = span
+ .snippet
+ .as_ref()
+ .and_then(|_| source_name_for(&label, source_code))
+ .map(|name| DiagnosticSource { name });
+ }
+ Some(span)
+ })
+ .collect();
+ (source, spans)
+}
+
</code_context>
<issue_to_address>
**suggestion:** Source name extraction is currently coupled to snippet availability
Because `source` is only set when `span.snippet` is `Some`, any non-UTF-8 span or empty snippet will cause the source name to be dropped even when `source_name_for` could return one. It would be more robust to derive `DiagnosticSource` solely from `source_name_for` (with its own `Option`) and decouple it from snippet extraction, so the file name is still reported when the snippet cannot be rendered.
```suggestion
fn extract_source_and_labels(
diagnostic: &dyn Diagnostic,
) -> (Option<DiagnosticSource>, Vec<DiagnosticSpan>) {
let Some(labelled_diagnostic) = diagnostic_with_labels(diagnostic) else {
return (None, Vec::new());
};
let Some(source_code) = labelled_diagnostic.source_code() else {
return (None, Vec::new());
};
let Some(labels) = labelled_diagnostic.labels() else {
return (None, Vec::new());
};
let mut source: Option<DiagnosticSource> = None;
let spans = labels
.filter_map(|label| {
// Derive the source name independently of snippet extraction, so we
// still report a file name even when the snippet cannot be rendered.
if source.is_none() {
if let Some(name) = source_name_for(&label, source_code) {
source = Some(DiagnosticSource { name });
}
}
build_span(&label, source_code)
})
.collect();
(source, spans)
}
```
</issue_to_address>
### Comment 3
<location path="docs/execplans/3-10-3-json-diagnostics-mode.md" line_range="135-138" />
<code_context>
+- Schema: if a required field cannot be populated consistently across the
+ current diagnostic types without inventing misleading placeholder values,
+ stop and escalate with a schema revision proposal.
+- Early-startup behaviour: if configuration-file-sourced `diag_json`
+ cannot be honoured for configuration-load failures without a large bootstrap
+ refactor, do not improvize. Document the limitation and escalate for
+ confirmation.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider changing "improvize" to "improvise" for standard spelling.
In the sentence `do not improvize. Document the limitation and escalate for`, "improvize" appears to be a misspelling of "improvise". Please update it for consistent, standard spelling.
```suggestion
- Early-startup behaviour: if configuration-file-sourced `diag_json`
cannot be honoured for configuration-load failures without a large bootstrap
refactor, do not improvise. Document the limitation and escalate for
confirmation.
```
</issue_to_address>
### Comment 4
<location path="src/main.rs" line_range="26" />
<code_context>
+}
- let (parsed_cli, matches) = match cli::parse_with_localizer_from(args, &localizer) {
+fn run_with_args(
+ args: Vec<OsString>,
+ env: &impl locale_resolution::EnvProvider,
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing a single `DiagMode` enum and centralizing JSON-versus-human error handling to replace the multiple diagnostic booleans and scattered helpers.
You can keep all the new functionality and still simplify the control flow by:
1. Computing a single diagnostic mode up-front (instead of threading booleans).
2. Centralizing “error → JSON or human” handling into one helper, instead of duplicating it in `parse_cli_or_exit`, `merge_cli_or_exit`, `handle_runner_error`.
3. Hiding the JSON fallback logic behind the `diagnostic_json` API.
Concretely:
### 1. Replace the boolean flags with a `DiagMode`
Right now `diag_json_hint`, `requested_diag_json`, and `merged_cli.diag_json` are interpreted in multiple places. You can unify this:
```rust
enum DiagMode {
Human,
Json,
}
fn run_with_args(
args: Vec<OsString>,
env: &impl locale_resolution::EnvProvider,
system_locale: &impl locale_resolution::SystemLocale,
) -> ExitCode {
let diag_json_hint = locale_resolution::resolve_startup_diag_json(&args, env);
let localizer = startup_localizer(&args, env, system_locale);
let (parsed_cli, matches) = match parse_cli_or_exit(args, &localizer, DiagMode::from_hint(diag_json_hint)) {
Ok(parsed) => parsed,
Err(code) => return code,
};
let mode = if diag_json_hint || parsed_cli.diag_json {
DiagMode::Json
} else {
DiagMode::Human
};
let merged_cli = match merge_cli_or_exit(&parsed_cli, &matches, mode) {
Ok(merged) => merged,
Err(code) => return code,
};
configure_runtime(&merged_cli, system_locale, &mode);
let prefs = output_prefs::resolve(merged_cli.no_emoji);
match runner::run(&merged_cli, prefs) {
Ok(()) => ExitCode::SUCCESS,
Err(err) => handle_runner_error(err, prefs, &mode),
}
}
```
With a trivial helper:
```rust
impl DiagMode {
fn from_hint(hint: bool) -> Self {
if hint { DiagMode::Json } else { DiagMode::Human }
}
fn is_json(&self) -> bool {
matches!(self, DiagMode::Json)
}
}
```
Then you can simplify signatures and internal branching:
```rust
fn parse_cli_or_exit(
args: Vec<OsString>,
localizer: &Arc<dyn Localizer>,
mode: DiagMode,
) -> Result<(cli::Cli, ArgMatches), ExitCode> {
match cli::parse_with_localizer_from(args, localizer) {
Ok(parsed) => Ok(parsed),
Err(err) => {
if mode.is_json() {
Err(emit_json_result(diagnostic_json::render_error_json(&err)))
} else {
err.exit();
}
}
}
}
fn merge_cli_or_exit(
parsed_cli: &cli::Cli,
matches: &ArgMatches,
mode: DiagMode,
) -> Result<cli::Cli, ExitCode> {
match cli::merge_with_config(parsed_cli, matches) {
Ok(merged) => Ok(merged.with_default_command()),
Err(err) => {
if mode.is_json() {
Err(emit_json_result(diagnostic_json::render_error_json(err.as_ref())))
} else {
init_tracing(Level::ERROR);
tracing::error!(error = %err, "configuration load failed");
Err(ExitCode::FAILURE)
}
}
}
}
```
`configure_runtime` and `handle_runner_error` can take `&DiagMode` instead of `bool`:
```rust
fn configure_runtime(
merged_cli: &cli::Cli,
system_locale: &impl locale_resolution::SystemLocale,
mode: &DiagMode,
) {
let runtime_locale = locale_resolution::resolve_runtime_locale(merged_cli, system_locale);
let runtime_localizer = Arc::from(cli_localization::build_localizer(runtime_locale.as_deref()));
localization::set_localizer(Arc::clone(&runtime_localizer));
if !mode.is_json() {
let max_level = if merged_cli.verbose { Level::DEBUG } else { Level::ERROR };
init_tracing(max_level);
}
}
fn handle_runner_error(
err: anyhow::Error,
prefs: output_prefs::OutputPrefs,
mode: &DiagMode,
) -> ExitCode {
if mode.is_json() {
return emit_json_result(render_runtime_error_json(&err));
}
// existing human path unchanged
let prefix = prefs.error_prefix();
match err.downcast::<runner::RunnerError>() {
Ok(runner_err) => {
let report = Report::new(runner_err);
drop(writeln!(io::stderr(), "{prefix} {report:?}"));
}
Err(other_err) => {
tracing::error!(error = %other_err, "runner failed");
drop(writeln!(io::stderr(), "{prefix} {other_err}"));
}
}
ExitCode::FAILURE
}
```
This removes the cross-cutting `diag_json_hint` / `requested_diag_json` booleans and makes the “mode” explicit and consistent everywhere.
### 2. Hide JSON fallback behind the `diagnostic_json` API
`emit_json_result` + `fallback_json_payload` are very specific to diagnostics JSON and could live in the `diagnostic_json` module, so `main.rs` only deals with a single “render and emit” API.
You can keep behavior identical but centralize it:
```rust
// in diagnostic_json:
pub fn emit_or_fallback(result: serde_json::Result<String>) -> ExitCode {
let payload = result.unwrap_or_else(|err| fallback_payload(&err));
drop(writeln!(io::stderr(), "{payload}"));
ExitCode::FAILURE
}
```
Then `main.rs` doesn’t need to know about the JSON shape or fallback:
```rust
fn parse_cli_or_exit(...) -> Result<_, ExitCode> {
// ...
if mode.is_json() {
Err(diagnostic_json::emit_or_fallback(
diagnostic_json::render_error_json(&err),
))
} else {
err.exit();
}
}
```
This keeps your new JSON-diagnostics feature, but reduces the number of small cross-cutting helpers and centralizes the key decisions (mode selection and JSON emission) into single places.
</issue_to_address>
### Comment 5
<location path="src/locale_resolution.rs" line_range="133" />
<code_context>
env: &impl EnvProvider,
system: &impl SystemLocale,
) -> Option<String> {
+ // Keep early-startup hint scanning aligned across locale and diagnostics.
+ let _diag_json_hint = cli::diag_json_hint_from_args(args);
let cli_hint = cli::locale_hint_from_args(args);
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the unused diagnostics scan from locale resolution and reusing a shared boolean-hint parser to keep startup behaviour simple and centralized.
You can trim a bit of complexity here by:
1. **Removing the unused diagnostics scan from locale resolution**
`_diag_json_hint` is effectively dead code and makes the locale resolver look coupled to diagnostics:
```rust
pub fn resolve_startup_locale(
args: &[OsString],
env: &impl EnvProvider,
system: &impl SystemLocale,
) -> Option<String> {
let cli_hint = cli::locale_hint_from_args(args);
let env_locale = env.var(NETSUKE_LOCALE_ENV);
let system_locale = system.system_locale();
select_locale([
cli_hint.as_deref(),
env_locale.as_deref(),
system_locale.as_deref(),
])
}
```
If you need a shared “early-startup scanning” behaviour, consider a dedicated helper that is clearly about pre-scan side effects, instead of a throwaway local bound to `_`.
2. **De-duplicating boolean-hint parsing**
If `cli_l10n::parse_bool_hint` already exists with the same semantics, reuse it instead of redefining `fn parse_bool_hint` here. For example:
In `cli_l10n.rs` (or wherever it currently lives):
```rust
pub fn parse_bool_hint(value: &str) -> Option<bool> {
match value.to_ascii_lowercase().as_str() {
"1" | "true" | "yes" | "on" => Some(true),
"0" | "false" | "no" | "off" => Some(false),
_ => None,
}
}
```
In this module:
```rust
use crate::cli_l10n::parse_bool_hint;
pub fn resolve_startup_diag_json(args: &[OsString], env: &impl EnvProvider) -> bool {
cli::diag_json_hint_from_args(args).unwrap_or_else(|| {
env.var(NETSUKE_DIAG_JSON_ENV)
.as_deref()
.and_then(parse_bool_hint)
.unwrap_or(false)
})
}
```
This keeps the startup `diag-json` behaviour intact while avoiding duplicated parsing rules and making it clear there is a single source of truth for boolean hint semantics.
</issue_to_address>
### Comment 6
<location path="src/cli_l10n.rs" line_range="204" />
<code_context>
hint
}
+
+fn parse_bool_hint(value: &str) -> Option<bool> {
+ match value.to_ascii_lowercase().as_str() {
+ "1" | "true" | "yes" | "on" => Some(true),
</code_context>
<issue_to_address>
**issue (review_instructions):** Eliminate the duplicated `parse_bool_hint` implementation and centralize it in a single shared helper to keep the code DRY.
Consolidate the boolean-hint parsing logic into a single function instead of maintaining identical implementations in both `cli_l10n.rs` and `locale_resolution.rs`.
For example, expose a shared helper (e.g. in a small internal util module or via `cli_l10n`) and have both `diag_json_hint_from_args` and `resolve_startup_diag_json` call it. This avoids divergence if the accepted string forms ever need to change and keeps behaviour consistent across early-startup and CLI parsing.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Keep code DRY, but readable. Use refactoring approaches best suited for the language in question.
</details>
</issue_to_address>
### Comment 7
<location path="docs/execplans/3-10-3-json-diagnostics-mode.md" line_range="316" />
<code_context>
+ [src/manifest/diagnostics/yaml.rs](/home/user/project/src/manifest/diagnostics/yaml.rs)
+ create `miette` diagnostics for manifest and YAML failures.
+
+The current output-channel contract is already strong enough to build on:
+
+- `stdout` carries command artefacts such as `manifest -` output and
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym "DOT" is used for graph output without being defined on first use.
In the bullet list just below, "graph DOT output" appears without expanding DOT. Consider updating that bullet to something like "Graphviz DOT (DOT) output" or similar, so the acronym is defined the first time it is mentioned.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 8
<location path="docs/execplans/3-10-3-json-diagnostics-mode.md" line_range="141" />
<code_context>
+ confirmation.
+- File size: if any edited Rust file would exceed 400 lines, split the
+ work into focused helper modules before continuing.
+- Test determinism: if JSON snapshots or BDD assertions remain flaky
+ after two attempts, stop and escalate with the specific unstable field or
+ ordering behaviour.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym "BDD" is introduced without an explicit definition on first use.
"BDD" (behaviour-driven development) is referenced here without expansion. Please expand it on first use, for example "BDD (behaviour-driven development) assertions", to comply with the requirement to define uncommon acronyms.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27f01b0e66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli_tests/locale.rs (1)
1-1: 🧹 Nitpick | 🔵 TrivialUpdate the module doc comment to reflect expanded scope.
The module now tests both locale hints and diagnostic JSON hints. Update the comment to accurately describe the module's purpose.
📝 Proposed fix
-//! Locale-specific CLI tests. +//! CLI hint extraction tests. +//! +//! Validates early argument scanning for `--locale` and `--diag-json` flags +//! before full clap parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_tests/locale.rs` at line 1, Update the module-level doc comment at the top of locale.rs to reflect the expanded scope (it now tests both locale hints and diagnostic JSON hints); replace the existing "Locale-specific CLI tests." comment with a brief sentence mentioning both "locale hints" and "diagnostic JSON hints" (e.g., "Locale and diagnostic JSON hint CLI tests.") so the module documentation accurately describes the tests in this file.
♻️ Duplicate comments (1)
src/locale_resolution.rs (1)
191-197: 🛠️ Refactor suggestion | 🟠 MajorDuplicate code: see comment in
src/cli_l10n.rs.This
parse_bool_hintfunction is identical to the one insrc/cli_l10n.rs. Extract to a shared module to eliminate the duplication.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/locale_resolution.rs` around lines 191 - 197, The function parse_bool_hint is duplicated in src/locale_resolution.rs and src/cli_l10n.rs; extract it into a single shared helper (e.g., a new pub fn parse_bool_hint in a small module like src/utils.rs or src/common/l10n.rs), keep the exact signature Option<bool> parse_bool_hint(value: &str), make it pub so both modules can call it, remove the duplicate implementations from locale_resolution.rs and cli_l10n.rs, and update both files to import and call the shared parse_bool_hint (add the appropriate use path).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-10-3-json-diagnostics-mode.md`:
- Around line 25-28: Replace all repository-local absolute links that start with
"/home/user/project/" in this ExecPlan with repository-relative paths so they
resolve on GitHub (e.g., change "/home/user/project/docs/users-guide.md" and
"/home/user/project/docs/netsuke-design.md" to "docs/users-guide.md" or
"./docs/users-guide.md" as appropriate). Search for the literal prefix
"/home/user/project/" in this file, update each occurrence to a repo-relative
path, and keep link targets identical after the prefix swap so link text and
anchors remain unchanged.
In `@src/cli_l10n.rs`:
- Around line 204-210: The function parse_bool_hint is duplicated; extract it
into a single shared helper (e.g., create pub(crate) fn parse_bool_hint in a new
util or parsing_utils module) and remove the duplicate definitions, then import
and call that shared parse_bool_hint from both places that currently define it;
ensure the helper uses to_ascii_lowercase().as_str() and the same match arms so
behavior remains identical.
In `@src/cli/mod.rs`:
- Around line 141-145: The diag_json flag is currently always present with its
default false and thus overwrites config values; in cli_overrides_from_matches()
add "diag_json" to the list of stripped fields unless
matches.value_source("diag_json") == Some(ValueSource::CommandLine) so the field
is only included when explicitly passed on the CLI; also fix the parsing/hint
logic by changing the diag_json field to Option<bool> (or otherwise treat it as
tri-state) and update diag_json_hint_from_args() / parse_bool_hint usage so the
hint/parser aligns with clap's boolean flag semantics (SetTrue) and preserves an
unset state when the flag isn't provided.
In `@src/diagnostic_json.rs`:
- Around line 98-104: The code currently sets primary_span by taking
labels.first(), which misidentifies the primary span when a non-primary label is
listed first; update extract_source_and_labels to return both the labels and an
explicit primary index or id by inspecting each miette::LabeledSpan::primary()
flag, then change from_diagnostic to use that returned primary marker to set
primary_span (instead of labels.first()). Locate and update
extract_source_and_labels and from_diagnostic to propagate and consume the
primary flag so primary_span is derived from the LabeledSpan::primary() value
rather than list position.
In `@src/locale_resolution.rs`:
- Around line 133-134: The variable _diag_json_hint is computed via
cli::diag_json_hint_from_args(args) and immediately discarded; since that
function is pure there are no side effects, so remove the unused computation
(delete the let _diag_json_hint = ... line) or, if it was meant to perform
initialization, replace the discard with an explicit usage or add a comment
documenting the side-effectful behavior; locate the call to
cli::diag_json_hint_from_args and either remove it or ensure its return value is
stored/used (or document why the call must remain) in locale_resolution.rs.
In `@src/main.rs`:
- Around line 86-105: The branch deciding whether to emit JSON currently only
uses requested_diag_json, but cli::merge_with_config can fail after a config
file has already enabled diag_json; fix by resolving the effective diag_json
flag from the config files (not just the CLI request) before emitting errors:
add or call a helper (e.g. cli::diag_json_enabled_from_files or
cli::read_config_to_check_diag_json) that inspects the same config sources used
by cli::merge_with_config (using matches and parsed_cli) and returns a bool,
then in merge_cli_or_exit's Err(err) branch use that resolved flag (instead of
requested_diag_json) to decide whether to call
emit_json_result(diagnostic_json::render_error_json(...)) or fall back to text
tracing and ExitCode::FAILURE.
- Around line 74-81: The code currently sends all clap errors through JSON when
diag_json_hint is set, causing DisplayHelp and DisplayVersion to fail; update
the match branch handling Err(err) so it first checks err.kind() and if it
equals clap::ErrorKind::DisplayHelp or clap::ErrorKind::DisplayVersion calls
err.exit() immediately, otherwise if diag_json_hint is true call
emit_json_result(diagnostic_json::render_error_json(&err)) and return Err(...),
and if diag_json_hint is false fall back to err.exit(); reference
cli::parse_with_localizer_from, err.kind(),
clap::ErrorKind::{DisplayHelp,DisplayVersion},
diagnostic_json::render_error_json and emit_json_result to locate the change.
---
Outside diff comments:
In `@tests/cli_tests/locale.rs`:
- Line 1: Update the module-level doc comment at the top of locale.rs to reflect
the expanded scope (it now tests both locale hints and diagnostic JSON hints);
replace the existing "Locale-specific CLI tests." comment with a brief sentence
mentioning both "locale hints" and "diagnostic JSON hints" (e.g., "Locale and
diagnostic JSON hint CLI tests.") so the module documentation accurately
describes the tests in this file.
---
Duplicate comments:
In `@src/locale_resolution.rs`:
- Around line 191-197: The function parse_bool_hint is duplicated in
src/locale_resolution.rs and src/cli_l10n.rs; extract it into a single shared
helper (e.g., a new pub fn parse_bool_hint in a small module like src/utils.rs
or src/common/l10n.rs), keep the exact signature Option<bool>
parse_bool_hint(value: &str), make it pub so both modules can call it, remove
the duplicate implementations from locale_resolution.rs and cli_l10n.rs, and
update both files to import and call the shared parse_bool_hint (add the
appropriate use path).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34f6727f-1221-4c3e-bc0c-0918df8bdb29
⛔ Files ignored due to path filters (1)
src/snapshots/diagnostic_json/netsuke__diagnostic_json__tests__manifest_parse_error.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
build.rsdocs/execplans/3-10-3-json-diagnostics-mode.mddocs/netsuke-design.mddocs/roadmap.mddocs/users-guide.mdlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/cli/mod.rssrc/cli_l10n.rssrc/diagnostic_json.rssrc/diagnostic_json_tests.rssrc/lib.rssrc/locale_resolution.rssrc/localization/keys.rssrc/main.rssrc/runner/mod.rstests/bdd/steps/json_diagnostics.rstests/bdd/steps/mod.rstests/cli_tests/locale.rstests/cli_tests/merge.rstests/cli_tests/parsing.rstests/features/json_diagnostics.featuretests/logging_stderr_tests.rs
- Introduce --diag-json CLI flag with localized help text. - Implement early detection of --diag-json to enable JSON diagnostics mode. - Serialize diagnostics and runtime errors as structured JSON to stderr. - Suppress usual stderr output and status messages when JSON mode is active. - Extend config and environment variable support for diag_json option. - Update streaming subprocess handling to discard child's stderr in JSON mode. - Add comprehensive tests verifying JSON diagnostics, stderr suppression, and fallback. - Document --diag-json behavior and usage in CLI and user guide. This feature facilitates integration with external tooling by providing a stable, structured diagnostic output format while preserving normal human-readable output when the flag is not used. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/cli_l10n.rs (1)
204-210: 🛠️ Refactor suggestion | 🟠 MajorExtract
parse_bool_hintto a shared location to eliminate duplication.This function is duplicated in
src/locale_resolution.rs. Move it to a shared utility module and import from both call sites to maintain DRY principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli_l10n.rs` around lines 204 - 210, The parse_bool_hint function is duplicated; extract it into a shared utility module (e.g., create a new util/mod or l10n/util module) and export it so both callers can use the single implementation. Move the body of parse_bool_hint into that shared module, change its visibility to pub(crate) or pub as needed, then replace the local definitions in src/cli_l10n.rs and src/locale_resolution.rs with use/import statements that reference the shared parse_bool_hint; update any references to the function name (parse_bool_hint) to point to the new module path and remove the duplicated code from both files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-10-3-json-diagnostics-mode.md`:
- Line 211: Replace the British spelling "centralised" with the Oxford spelling
"centralized" in the text (the token "centralised" on the line currently reading
"The current hook point for runtime error rendering is centralised in"); update
that single word to "centralized" so the document adheres to the en-GB-oxendict
"-ize" guideline.
In `@src/diagnostic_json.rs`:
- Around line 347-365: The end_position function has CRLF-handling logic inside
its loop increasing cognitive complexity; extract that conditional into a small
helper (e.g., is_crlf_pair or should_skip_crlf) and call it from end_position so
the while-let loop only updates line/column and defers CRLF detection to the
helper; update references inside end_position to use the new helper (keep the
function name end_position and the peekable chars iteration) and ensure the
helper accepts the current char and the Peekable<char> reference (or equivalent)
to preserve behavior.
---
Duplicate comments:
In `@src/cli_l10n.rs`:
- Around line 204-210: The parse_bool_hint function is duplicated; extract it
into a shared utility module (e.g., create a new util/mod or l10n/util module)
and export it so both callers can use the single implementation. Move the body
of parse_bool_hint into that shared module, change its visibility to pub(crate)
or pub as needed, then replace the local definitions in src/cli_l10n.rs and
src/locale_resolution.rs with use/import statements that reference the shared
parse_bool_hint; update any references to the function name (parse_bool_hint) to
point to the new module path and remove the duplicated code from both files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9970581a-ed18-4f5d-9326-10028574f395
⛔ Files ignored due to path filters (1)
src/snapshots/diagnostic_json/netsuke__diagnostic_json__tests__manifest_parse_error.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
build.rsdocs/execplans/3-10-1-guarantee-status-message-ordering.mddocs/execplans/3-10-2-consistent-log-prefixes.mddocs/execplans/3-10-3-json-diagnostics-mode.mddocs/execplans/3-9-1-integrate-indicatif-multi-progress.mdsrc/cli/mod.rssrc/cli_l10n.rssrc/diagnostic_json.rssrc/locale_resolution.rssrc/main.rssrc/runner/process/mod.rstests/cli_tests/locale.rstests/cli_tests/merge.rstests/logging_stderr_tests.rs
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file fn diag_json_help_uses_normal_clap_output() -> Result<()> {
let output = assert_cmd::cargo::cargo_bin_cmd!("netsuke")
.arg("--diag-json")
.arg("--help")
.output()
.context("run netsuke --diag-json --help")?;
ensure!(output.status.success(), "help should exit successfully");
let stdout = String::from_utf8(output.stdout).context("stdout should be valid UTF-8")?;
ensure!(
stdout.contains("Usage:"),
"help output should remain human-readable"
);
ensure!(
output.stderr.is_empty(),
"help should not emit diagnostics JSON"
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-10-3-json-diagnostics-mode.md`:
- Around line 624-627: Remove or update the "Approval gate" section so it no
longer states the plan is draft-only and prevents implementation; edit the
"Approval gate" heading (or delete the two sentences "This document is the draft
phase only. Do not begin implementation until the user explicitly approves this
ExecPlan or asks for revisions.") and replace with a post-approval status that
matches the completed "Progress" and "Outcomes & Retrospective" sections,
ensuring the document consistently permits implementation going forward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a2d6e5c-e315-488e-b8f9-0b6d550cd5b0
📒 Files selected for processing (1)
docs/execplans/3-10-3-json-diagnostics-mode.md
Owner
Author
|
@coderabbitai Have the following now been resolved? In |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 298
//! Locale and diagnostic JSON hint CLI tests.
That matches the requested wording precisely — no further action is needed. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4214
[resolve] |
Refactored the diag-json help and version tests by introducing a helper function `assert_diag_json_passthrough` that checks for human-readable stdout and empty stderr output when using the `--diag-json` flag. This reduces code duplication and improves test clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/logging_stderr_tests.rs (1)
189-197: 🧹 Nitpick | 🔵 TrivialCollapse these passthrough cases into one
#[rstest]table.Line 190 and Line 195 still repeat the same test shape with only the inputs changed. Replace both wrappers with one parameterized case table and keep
assert_diag_json_passthroughas the shared body.Refactor sketch
+use rstest::rstest; ... -#[test] -fn diag_json_help_uses_normal_clap_output() -> Result<()> { - assert_diag_json_passthrough("--help", "help", "Usage:") -} - -#[test] -fn diag_json_version_uses_normal_clap_output() -> Result<()> { - assert_diag_json_passthrough("--version", "version", "netsuke") +#[rstest] +#[case("--help", "help", "Usage:")] +#[case("--version", "version", "netsuke")] +fn diag_json_passthrough_uses_normal_clap_output( + #[case] flag: &str, + #[case] ctx: &str, + #[case] stdout_marker: &str, +) -> Result<()> { + assert_diag_json_passthrough(flag, ctx, stdout_marker) }As per coding guidelines, "In Rust tests, replace duplicated tests with
#[rstest(...)]parameterised cases."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/logging_stderr_tests.rs` around lines 189 - 197, Replace the two nearly identical test functions diag_json_help_uses_normal_clap_output and diag_json_version_uses_normal_clap_output with a single parameterized rstest that calls the shared helper assert_diag_json_passthrough; add use rstest::rstest at the top, create a new function (e.g. diag_json_passthrough_uses_normal_clap_output) annotated with #[rstest] and two #[case] entries ("--help","help","Usage:") and ("--version","version","netsuke"), accept parameters (flag: &str, ctx: &str, stdout_marker: &str) returning Result<()> and invoke assert_diag_json_passthrough(flag, ctx, stdout_marker) as the test body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/logging_stderr_tests.rs`:
- Around line 189-197: Replace the two nearly identical test functions
diag_json_help_uses_normal_clap_output and
diag_json_version_uses_normal_clap_output with a single parameterized rstest
that calls the shared helper assert_diag_json_passthrough; add use
rstest::rstest at the top, create a new function (e.g.
diag_json_passthrough_uses_normal_clap_output) annotated with #[rstest] and two
#[case] entries ("--help","help","Usage:") and
("--version","version","netsuke"), accept parameters (flag: &str, ctx: &str,
stdout_marker: &str) returning Result<()> and invoke
assert_diag_json_passthrough(flag, ctx, stdout_marker) as the test body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d67ddb3-0707-4262-a07b-9d28147499f5
📒 Files selected for processing (1)
tests/logging_stderr_tests.rs
Change the 'Approval gate' section to 'Approval status' to reflect that the ExecPlan has been approved and implemented, serving as the execution record for roadmap item 3.10.3. Future follow-up work should use the completed sections as the source of truth. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/execplans/3-10-3-json-diagnostics-mode.md (1)
211-211:⚠️ Potential issue | 🟡 MinorReplace the Oxford spelling at Line 211.
Change
centralisedtocentralizedto match repository spelling rules.✏️ Proposed fix
-- The current hook point for runtime error rendering is centralised in +- The current hook point for runtime error rendering is centralized inAs per coding guidelines,
docs/**/*.mdmust use en-GB-oxendict spelling,
including the-izesuffix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/3-10-3-json-diagnostics-mode.md` at line 211, Replace the British spelling token "centralised" with the American variant "centralized" in the sentence that currently reads "The current hook point for runtime error rendering is centralised in" so the line becomes "The current hook point for runtime error rendering is centralized in"; ensure this edit is applied in the docs markdown file containing that sentence (docs/**/*.md) and keep surrounding punctuation/spacing unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/logging_stderr_tests.rs`:
- Around line 140-145: Add an rstest fixture named temp_with_minimal_manifest
that returns a tempfile::TempDir and performs the duplicated setup (create
tempdir, build manifest_path, copy "tests/data/minimal.yml" into it) and mark it
with #[fixture]; import rstest::{fixture, rstest}. Then update the two tests
diag_json_success_keeps_stdout_artifact_and_stderr_empty and
diag_json_success_discards_child_stderr to accept temp_with_minimal_manifest:
tempfile::TempDir as a parameter, replace their local temp setup with let temp =
temp_with_minimal_manifest; and remove the duplicated copy logic from those
tests.
---
Duplicate comments:
In `@docs/execplans/3-10-3-json-diagnostics-mode.md`:
- Line 211: Replace the British spelling token "centralised" with the American
variant "centralized" in the sentence that currently reads "The current hook
point for runtime error rendering is centralised in" so the line becomes "The
current hook point for runtime error rendering is centralized in"; ensure this
edit is applied in the docs markdown file containing that sentence
(docs/**/*.md) and keep surrounding punctuation/spacing unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60c30f1a-ece2-4333-aa8e-7fd7f517f2cb
📒 Files selected for processing (2)
docs/execplans/3-10-3-json-diagnostics-mode.mdtests/logging_stderr_tests.rs
…p manifest - Converted several #[test] functions to use #[rstest] with shared fixture - Added `temp_with_minimal_manifest` fixture to reuse temp dir setup with manifest - Improved test code reuse and clarity in logging_stderr_tests.rs Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-10-3-json-diagnostics-mode.md`:
- Around line 208-209: The sentence "That makes it useful as implementation
reference, but not as the supported wire contract." is missing the indefinite
article before "implementation reference"; update the text to read "That makes
it useful as an implementation reference, but not as the supported wire
contract." to correct the grammar.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4b9cb35-150b-4d4c-a139-fcd821975d87
📒 Files selected for processing (3)
docs/execplans/3-10-3-json-diagnostics-mode.mdsrc/diagnostic_json.rstests/logging_stderr_tests.rs
Corrected 'as implementation reference' to 'as an implementation reference' in the JSON diagnostics mode documentation to improve clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
New module: src/diagnostic_json.rs
New tests: src/diagnostic_json_tests.rs
Wiring and integration across the codebase:
diag_json: boolflag and default gate in CLI.diag_jsonflag help.Documentation and design updates:
--diag-json,NETSUKE_DIAG_JSON, and config knobs.Localization and build: updated builds/localization to support diag_json hints and help text.
Public surface: src/lib.rs exports
diagnostic_jsonto consumers.Tests and gate coverage:
Misc build/localization:
Rationale
Validation and acceptance
Context and references
📎 Task: https://www.devboxer.com/task/c36edb20-4cdb-49e9-bf10-0be9c6b63511