Skip to content

Commit 28e097d

Browse files
Implement 3.10.3 JSON diagnostics mode (#274)
* docs(execplans): add execplan for JSON diagnostics mode roadmap item 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> * feat(diagnostics): add --diag-json machine-readable JSON diagnostics 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> * refactor(diagnostic_json): make related field type recursive with Self 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> * feat(diag-json): add --diag-json flag for machine-readable diagnostics - 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> * docs(execplans): fix typo in JSON diagnostics mode documentation Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * test(logging_stderr): refactor diag-json tests to remove duplication 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> * docs(execplans): update approval status in JSON diagnostics mode doc 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> * test(logging_stderr): convert tests to rstest and add fixture for temp 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> * docs(execplans): fix typo in JSON diagnostics docs 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> --------- Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
1 parent b7321a8 commit 28e097d

28 files changed

+2079
-103
lines changed

build.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
9494
// tests.
9595
const _: usize = std::mem::size_of::<HostPattern>();
9696
const _: fn(&[OsString]) -> Option<String> = cli::locale_hint_from_args;
97+
const _: fn(&[OsString]) -> Option<bool> = cli::diag_json_hint_from_args;
98+
const _: fn(&str) -> Option<bool> = cli_l10n::parse_bool_hint;
99+
const _: fn(&cli::Cli, &ArgMatches) -> bool = cli::resolve_merged_diag_json;
97100
const _: fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult<cli::Cli> =
98101
cli::merge_with_config;
99102
const _: LocalizedParseFn = cli::parse_with_localizer_from;

docs/execplans/3-10-1-guarantee-status-message-ordering.md

Lines changed: 40 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -65,25 +65,21 @@ Thresholds that trigger escalation when breached:
6565
Known uncertainties that might affect the plan:
6666

6767
- Risk: Indicatif progress bars may have implicit stdout writes.
68-
Severity: medium
69-
Likelihood: low
70-
Mitigation: Verify `ProgressDrawTarget::stderr_with_hz()` is consistently used
71-
throughout `IndicatifReporter`. The codebase already uses stderr targets.
68+
Severity: medium Likelihood: low Mitigation: Verify
69+
`ProgressDrawTarget::stderr_with_hz()` is consistently used throughout
70+
`IndicatifReporter`. The codebase already uses stderr targets.
7271

7372
- Risk: Timing-sensitive interleaving between status updates and subprocess
74-
output may be difficult to test deterministically.
75-
Severity: medium
76-
Likelihood: medium
77-
Mitigation: Use BDD scenarios with controlled fake Ninja executables that emit
78-
predictable output patterns. Avoid timing-based assertions.
73+
output may be difficult to test deterministically. Severity: medium
74+
Likelihood: medium Mitigation: Use BDD scenarios with controlled fake Ninja
75+
executables that emit predictable output patterns. Avoid timing-based
76+
assertions.
7977

8078
- Risk: The existing subprocess streaming architecture splits stdout/stderr
8179
handling across threads, which could introduce race conditions if status
82-
callbacks write to stdout.
83-
Severity: medium
84-
Likelihood: low
85-
Mitigation: Audit all paths where `StatusReporter` methods write output;
86-
confirm they exclusively use stderr.
80+
callbacks write to stdout. Severity: medium Likelihood: low Mitigation: Audit
81+
all paths where `StatusReporter` methods write output; confirm they
82+
exclusively use stderr.
8783

8884
## Progress
8985

@@ -132,9 +128,9 @@ Known uncertainties that might affect the plan:
132128

133129
**Outcome**: SUCCESS
134130

135-
The existing implementation already correctly separates status messages (stderr)
136-
from subprocess output (stdout). This plan was primarily a verification and
137-
documentation exercise rather than an implementation task.
131+
The existing implementation already correctly separates status messages
132+
(stderr) from subprocess output (stdout). This plan was primarily a
133+
verification and documentation exercise rather than an implementation task.
138134

139135
**What went well**:
140136

@@ -146,8 +142,8 @@ documentation exercise rather than an implementation task.
146142
**Lessons learned**:
147143

148144
- Always audit the existing implementation before assuming changes are needed.
149-
The roadmap item implied implementation work, but the architecture was already
150-
correct.
145+
The roadmap item implied implementation work, but the architecture was
146+
already correct.
151147
- End-to-end tests that verify stream separation provide valuable regression
152148
protection even when no code changes are required.
153149

@@ -209,17 +205,17 @@ The output architecture spans several modules:
209205
Based on code analysis, the following table summarizes stream routing for
210206
reporters and subprocesses:
211207

212-
| Component | Output destination | Notes |
213-
| --------- | ------------------ | ----- |
214-
| `AccessibleReporter::report_stage` | stderr | `writeln!(io::stderr(), ...)` |
215-
| `AccessibleReporter::report_complete` | stderr | `writeln!(io::stderr(), ...)` |
216-
| `AccessibleReporter::report_task_progress` | stderr | `writeln!(io::stderr(), ...)` |
217-
| `IndicatifReporter` (progress bars) | stderr | `ProgressDrawTarget::stderr_with_hz(12)` |
218-
| `IndicatifReporter` (hidden mode fallback) | stderr | `writeln!(io::stderr(), ...)` |
219-
| `IndicatifReporter::report_complete` | stderr | `writeln!(io::stderr(), ...)` |
220-
| `VerboseTimingReporter` timing summary | stderr | Via wrapped reporter |
221-
| Ninja subprocess stdout | stdout | Via `spawn_and_stream_output()` |
222-
| Ninja subprocess stderr | stderr | Via background thread forwarding |
208+
| Component | Output destination | Notes |
209+
| ------------------------------------------ | ------------------ | ---------------------------------------- |
210+
| `AccessibleReporter::report_stage` | stderr | `writeln!(io::stderr(), ...)` |
211+
| `AccessibleReporter::report_complete` | stderr | `writeln!(io::stderr(), ...)` |
212+
| `AccessibleReporter::report_task_progress` | stderr | `writeln!(io::stderr(), ...)` |
213+
| `IndicatifReporter` (progress bars) | stderr | `ProgressDrawTarget::stderr_with_hz(12)` |
214+
| `IndicatifReporter` (hidden mode fallback) | stderr | `writeln!(io::stderr(), ...)` |
215+
| `IndicatifReporter::report_complete` | stderr | `writeln!(io::stderr(), ...)` |
216+
| `VerboseTimingReporter` timing summary | stderr | Via wrapped reporter |
217+
| Ninja subprocess stdout | stdout | Via `spawn_and_stream_output()` |
218+
| Ninja subprocess stderr | stderr | Via background thread forwarding |
223219

224220
### Existing test coverage
225221

@@ -271,34 +267,38 @@ model provides the necessary ordering guarantees.
271267

272268
### Stage D: End-to-end tests (completed)
273269

274-
**Outcome**: Three BDD scenarios were added to `tests/features/progress_output.feature`
275-
to verify stream separation:
270+
**Outcome**: Three BDD scenarios were added to
271+
`tests/features/progress_output.feature` to verify stream separation:
276272

277273
1. **Subprocess stdout is separate from status messages**: Verifies that stdout
278-
markers from the fake Ninja appear only in stdout, while stderr markers appear
279-
only in stderr. Uses stable machine markers (`NINJA_STDOUT_MARKER`,
274+
markers from the fake Ninja appear only in stdout, while stderr markers
275+
appear only in stderr. Uses stable machine markers (`NINJA_STDOUT_MARKER`,
280276
`NINJA_STDERR_MARKER`) to avoid coupling to localized UI strings.
281277

282-
2. **Status messages do not contaminate stdout in standard mode**: Verifies stream
278+
2. **Status messages do not contaminate stdout in standard mode**: Verifies
279+
stream
283280
routing in non-accessible mode using the same stable markers.
284281

285282
3. **Build artifacts can be captured via stdout redirection**: Verifies that
286283
`netsuke manifest -` output goes to stdout without status contamination.
287284

288285
Supporting infrastructure added:
289286

290-
- `FakeNinjaConfig` struct in `tests/bdd/steps/progress_output.rs` for configurable
287+
- `FakeNinjaConfig` struct in `tests/bdd/steps/progress_output.rs` for
288+
configurable
291289
fixture generation with optional stderr markers.
292290
- `install_fake_ninja_with_config()` function for flexible fixture setup.
293-
- Updated `fake_ninja_emits_stdout_output` fixture to emit both stdout and stderr
291+
- Updated `fake_ninja_emits_stdout_output` fixture to emit both stdout and
292+
stderr
294293
markers for comprehensive stream routing verification.
295294

296295
### Stage E: Documentation (completed)
297296

298-
**Outcome**: The `docs/users-guide.md` file was updated with an "Output streams"
299-
section documenting the stream separation behaviour:
297+
**Outcome**: The `docs/users-guide.md` file was updated with an "Output
298+
streams" section documenting the stream separation behaviour:
300299

301-
- Explains that status messages go to stderr and subprocess output goes to stdout.
300+
- Explains that status messages go to stderr and subprocess output goes to
301+
stdout.
302302
- Provides example redirection commands for common use cases.
303303
- Documents the `--progress false` flag for suppressing status output entirely.
304304

docs/execplans/3-10-2-consistent-log-prefixes.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,8 @@ Hard invariants that must hold throughout implementation:
7777
## Surprises & discoveries
7878

7979
- All existing BDD assertions used substring `contains` matching, so adding
80-
prefixes to output lines did not break any existing scenarios. Only
81-
scenario titles and a few new assertions needed updating.
80+
prefixes to output lines did not break any existing scenarios. Only scenario
81+
titles and a few new assertions needed updating.
8282
- The `IndicatifReporter` needed `OutputPrefs` added to its constructor and
8383
all downstream call sites (runner, tests) to support the success prefix.
8484

@@ -100,8 +100,8 @@ Hard invariants that must hold throughout implementation:
100100
**Outcome**: SUCCESS
101101

102102
All quality gates pass on first attempt. The implementation introduces
103-
consistent, localizable, emoji-aware prefixes across all three output
104-
channels without breaking any existing functionality.
103+
consistent, localizable, emoji-aware prefixes across all three output channels
104+
without breaking any existing functionality.
105105

106106
**What went well**:
107107

0 commit comments

Comments
 (0)