From 6b68ea2837883edcdc6ca4fb6a24902c0c1cd285 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 22 Feb 2026 18:02:24 +0000 Subject: [PATCH 01/14] feat(runner/process): parse Ninja status lines to drive Stage 6 task progress Add a new parser module for Ninja status lines to extract task-level progress updates and integrate it with subprocess output forwarding. Extend status reporters to reflect task progress during Stage 6 execution. Implement fallback textual updates for non-TTY stdout and accessible mode, controlled by the existing progress configuration with localized CLI help. Add comprehensive unit and behavioural tests to cover happy, unhappy, and edge cases. Update user and design documentation to reflect new progress reporting. This implements roadmap item 3.9.2, enabling Netsuke to show deterministic task progress from Ninja build output. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 371 ++++++++++++++++++ 1 file changed, 371 insertions(+) create mode 100644 docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md new file mode 100644 index 00000000..231c4c6d --- /dev/null +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -0,0 +1,371 @@ +# Parse Ninja status lines to drive task progress (roadmap 3.9.2) + +This ExecPlan is a living document. The sections `Constraints`, `Tolerances`, +`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and +`Outcomes & Retrospective` must be kept up to date as work proceeds. + +Status: DRAFT + +No `PLANS.md` file exists in this repository. + +## Purpose / big picture + +Netsuke currently reports six pipeline stages, but Stage 6 does not yet derive +task-level progress from Ninja execution lines. Roadmap item 3.9.2 requires +Ninja status-line parsing so Netsuke can present task progress during build +execution. The same milestone also requires deterministic textual fallback +updates when stdout is not a TTY and whenever accessible mode is active. + +After this change: + +- Stage 6 task progress is driven by parsed Ninja status lines. +- Standard interactive mode keeps the existing `indicatif`-based UX, now with + task counters. +- Non-interactive stdout and accessible mode emit textual task updates. +- Progress control continues through OrthoConfig layering with localized help + copy (`--progress`, `NETSUKE_PROGRESS`, config file key). + +Observable success: + +- Running `netsuke --accessible false --progress true build` against a manifest + that executes multiple Ninja edges shows Stage 6 task advancement. +- Running the same command with stdout redirected still emits textual task + updates suitable for logs and CI. +- Running with `--accessible true` emits text-only task updates (no animated + redraw dependency). +- `make check-fmt`, `make lint`, and `make test` pass. + +## Constraints + +- Implement roadmap item `3.9.2` only: parse Ninja status lines for Stage 6 + task progress plus fallback textual updates. +- Preserve the six-stage model already implemented in `src/status_pipeline.rs`. +- Keep accessible mode text-first and non-animated. +- Use OrthoConfig layering for user control; do not introduce ad-hoc config + reads. Existing `progress: Option` remains the controlling switch. +- Keep CLI help localization wired through Fluent and `src/cli_l10n.rs`. +- Maintain public behaviour compatibility for existing commands. +- Add unit tests using `rstest`. +- Add behavioural tests using `rstest-bdd` v0.5.0. +- Cover happy path, unhappy path, and edge conditions. +- Record final design decisions in `docs/netsuke-design.md`. +- Update `docs/users-guide.md` with user-visible changes. +- Mark roadmap item `3.9.2` done only after implementation and validation. +- Run required quality gates with logged output: + `make check-fmt`, `make lint`, `make test`. + +## Tolerances (exception triggers) + +- Scope: if implementation needs more than 16 files or 900 net new lines, + stop and escalate. +- API compatibility: if a public function signature must break (for example + `runner::run_ninja`), stop and escalate with alternatives. +- Configuration: if this milestone requires a new top-level CLI/config field + instead of extending `progress`, stop and confirm. +- Output contracts: if preserving subprocess stream behaviour conflicts with + progress parsing, stop and escalate before altering semantics. +- Tests: if deterministic coverage cannot be achieved after three iterations, + stop and escalate with specific flake evidence. +- File-size guardrail: if any edited source file would exceed 400 lines, split + into focused submodules before proceeding. + +## Risks + +- Risk: Ninja status lines vary by output settings and may be absent or + malformed. Severity: high Likelihood: medium Mitigation: parse + conservatively, ignore malformed lines, and keep reporter state monotonic. + +- Risk: status parsing and raw output forwarding can interfere with stream + ordering. Severity: high Likelihood: medium Mitigation: parse in a tee-style + forwarding path that preserves original bytes and writes unchanged child + output. + +- Risk: fallback mode criteria (`stdout` TTY, accessible mode, hidden draw + target) can diverge across environments. Severity: medium Likelihood: high + Mitigation: centralize fallback predicate and cover it with parameterized + tests using dependency-injected terminal capability inputs. + +- Risk: BDD tests that rely on host Ninja binaries can be flaky in CI. + Severity: medium Likelihood: high Mitigation: run behavioural scenarios with + deterministic fake Ninja fixtures that emit controlled status lines. + +## Progress + +- [x] (2026-02-22 17:55Z) Reviewed roadmap, current status/reporter pipeline, + process streaming code, OrthoConfig integration, and existing test + surfaces. +- [x] (2026-02-22 17:55Z) Drafted this ExecPlan at + `docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md`. +- [ ] Stage A: Add Ninja status parser and forwarding hooks. +- [ ] Stage B: Wire parsed task progress into status reporters. +- [ ] Stage C: Add textual fallback behaviour for non-TTY stdout and + accessible mode. +- [ ] Stage D: Extend unit and behavioural tests (happy/unhappy/edge cases). +- [ ] Stage E: Update docs/design/roadmap and run quality gates. + +## Surprises & Discoveries + +- `src/runner/process/mod.rs` is already 421 lines, so adding parser logic in + that file directly would violate project file-size guidance. This work should + extract logic into new submodules rather than grow `mod.rs`. +- Current BDD progress scenarios (`tests/features/progress_output.feature`) use + `manifest -`, which does not invoke Ninja. New scenarios must exercise build + execution with controlled Ninja output. +- MCP project-memory tools (`qdrant-find` / `qdrant-store`) were not available + in this environment; repository docs were used as the only source of truth. + +## Decision Log + +- Decision: parse bracketed Ninja status tokens of the form `[current/total]` + at line start for Stage 6 progress, treating malformed lines as non-events. + Rationale: this is the documented, stable surface from Ninja output and keeps + parser complexity low. Date/Author: 2026-02-22 / Codex + +- Decision: use the existing OrthoConfig-backed `progress` setting as the sole + control for stage and task progress output, and update localized help text to + reflect the expanded behaviour. Rationale: avoids unnecessary configuration + sprawl while satisfying layered config and localization requirements. + Date/Author: 2026-02-22 / Codex + +- Decision: emit explicit textual task updates whenever stdout is non-TTY or + accessible mode is active, regardless of `indicatif` draw target details. + Rationale: roadmap requirement is about deterministic accessibility/log + readability and must not depend solely on terminal redraw support. + Date/Author: 2026-02-22 / Codex + +## Outcomes & Retrospective + +Pending implementation. + +Target outcomes: + +- Stage 6 task progress advances from parsed Ninja status lines. +- Non-TTY and accessible flows receive deterministic textual updates. +- Unit + BDD coverage proves happy, unhappy, and edge behaviour. +- Documentation and roadmap remain synchronized with implemented behaviour. + +## Context and orientation + +Primary implementation surfaces: + +- `src/runner/process/mod.rs`: child process spawning and output forwarding. +- `src/runner/mod.rs`: reporter construction and pipeline orchestration. +- `src/status.rs`: reporter trait and implementations. +- `src/status_pipeline.rs`: six-stage canonical ordering and labels. +- `src/cli/mod.rs`: OrthoConfig-derived CLI configuration. +- `src/cli_l10n.rs`: localized clap help mapping. +- `src/localization/keys.rs`: Fluent message key constants. +- `locales/en-US/messages.ftl` and `locales/es-ES/messages.ftl`: localized + user-facing strings. +- `tests/features/progress_output.feature` and `tests/bdd/steps/*`: behavioural + test coverage. +- `src/status_tests.rs` and `src/runner/process/*tests*`: unit coverage. +- `docs/users-guide.md`: user-visible behaviour documentation. +- `docs/netsuke-design.md`: design decisions and rationale. +- `docs/roadmap.md`: implementation status tracking. + +Terminology used in this plan: + +- Ninja status line: the leading progress token Ninja emits per edge, typically + `[current/total]`. +- Stage 6: `PipelineStage::NinjaSynthesisAndExecution`. +- Textual fallback: non-animated, line-oriented progress output suitable for + screen readers, CI logs, and redirected output. + +## Plan of work + +### Stage A: parser and process plumbing + +Create a dedicated Ninja status parser module and integrate it with subprocess +output forwarding in a non-disruptive way. + +Planned edits: + +- Add `src/runner/process/ninja_status.rs` with: + - a parsed update type (for example `NinjaTaskProgress`), + - `parse_ninja_status_line(&str) -> Option`, + - monotonic update guard logic to avoid regressions. +- Extract/reshape forwarding code from `src/runner/process/mod.rs` so file size + does not grow beyond guidance. +- Add an internal observer hook for parsed status updates while preserving the + current public `run_ninja` API contract. + +Validation gate: + +- Unit tests for parser happy/unhappy cases pass before wiring reporter logic. + +### Stage B: reporter integration for Stage 6 task progress + +Connect parsed status updates to status reporting so Stage 6 reflects +task-level advancement. + +Planned edits: + +- Extend `StatusReporter` in `src/status.rs` with a task-progress method + (default no-op for compatibility). +- Implement task-progress handling in: + - `IndicatifReporter` (interactive summaries), + - `AccessibleReporter` (text-first updates), + - `SilentReporter` (no output). +- Ensure Stage 6 state remains coherent (`running` -> `done`/`failed`) when + task updates are present or absent. + +Validation gate: + +- Unit tests verify no reporter panics on malformed/duplicate/regressive + updates and that state transitions remain correct. + +### Stage C: fallback textual updates and OrthoConfig-aligned behaviour + +Implement deterministic fallback rules and ensure layered configuration still +controls progress ergonomically. + +Planned edits: + +- Centralize fallback predicate: + - fallback when stdout is not a TTY, or + - accessible mode is active. +- Update localized help copy for `progress` to clarify that it controls stage + and task progress summaries. +- Keep precedence through OrthoConfig (`config < env < CLI`) with existing + `progress: Option` semantics. + +Validation gate: + +- CLI merge/parsing tests still pass; explicit `--progress false` suppresses + task progress in all modes. + +### Stage D: tests with `rstest` and `rstest-bdd` v0.5.0 + +Add focused unit and behavioural coverage for happy, unhappy, and edge paths. + +Planned unit tests (`rstest`): + +- Parser coverage in `src/runner/process/ninja_status.rs` tests: + - valid bracketed lines, + - malformed lines, + - zero/overflow/regression handling, + - duplicate progress updates. +- Reporter coverage in `src/status_tests.rs`: + - Stage 6 task update formatting, + - fallback predicate combinations, + - failure-path finalization with partial task progress. + +Planned behavioural tests (`rstest-bdd` v0.5.0): + +- Extend `tests/features/progress_output.feature` with build-execution + scenarios that use fake Ninja output. +- Add `tests/bdd/steps/progress_output.rs` (new module) to keep + `tests/bdd/steps/manifest_command.rs` under 400 lines and to isolate progress + fixtures/steps. +- Include scenarios for: + - happy path: parsed updates advance task progress, + - happy path accessible: textual updates appear in accessible mode, + - unhappy path: malformed status lines are ignored safely, + - edge case: `--progress false` suppresses progress updates. + +Validation gate: + +- New BDD scenarios are deterministic and do not depend on host Ninja. + +### Stage E: documentation, design record, roadmap, and gates + +Synchronize docs and complete quality validation. + +Planned edits: + +- Update `docs/users-guide.md`: + - Stage 6 task progress behaviour, + - fallback textual updates in non-TTY and accessible flows, + - `progress` configuration semantics. +- Update `docs/netsuke-design.md` (Section 8.4 decisions) with parser approach, + fallback rules, and configuration rationale. +- Mark roadmap item `3.9.2` and its fallback sub-bullet done in + `docs/roadmap.md` once all tests and gates pass. + +Validation gate: + +- `make check-fmt`, `make lint`, and `make test` all succeed. + +## Interfaces and dependencies + +The implementation should remain dependency-neutral (no new external crates). +Use existing crates already in `Cargo.toml`: + +- `indicatif` for standard mode rendering. +- `ortho_config` for layered configuration and localized help integration. +- `rstest` and `rstest-bdd` v0.5.0 for unit and behavioural tests. + +Expected new internal interfaces (names may vary, behaviour is required): + +- A parser function in `src/runner/process/ninja_status.rs` that converts a + single output line into an optional task-progress event. +- A process-layer hook that receives parsed task-progress events without + changing emitted child output bytes. +- A status-reporter method for Stage 6 task updates with safe default no-op + semantics for reporters that do not display progress. + +## Concrete steps + +Run all commands from `/home/user/project`. + +1. Implement parser module and parser unit tests. +2. Wire parser into process forwarding path with observer callbacks. +3. Extend reporter trait/implementations for task-progress updates. +4. Implement fallback predicate and update localized `progress` help strings. +5. Add/extend BDD scenarios and steps using fake Ninja emitters. +6. Update `docs/users-guide.md`, `docs/netsuke-design.md`, and + `docs/roadmap.md`. +7. Run quality gates with `tee` logs: + +```sh +set -o pipefail +make check-fmt 2>&1 | tee /tmp/3-9-2-check-fmt.log +make lint 2>&1 | tee /tmp/3-9-2-lint.log +make test 2>&1 | tee /tmp/3-9-2-test.log +``` + +1. Run documentation gates after doc edits: + +```sh +set -o pipefail +make fmt 2>&1 | tee /tmp/3-9-2-fmt.log +make markdownlint 2>&1 | tee /tmp/3-9-2-markdownlint.log +make nixie 2>&1 | tee /tmp/3-9-2-nixie.log +``` + +## Validation and acceptance + +Acceptance requires all of the following: + +- Parsed Ninja status lines produce Stage 6 task progress updates. +- When stdout is non-TTY, fallback textual task updates are emitted. +- When accessible mode is active, fallback textual task updates are emitted. +- Malformed or missing Ninja status lines do not panic and do not corrupt + overall stage completion/failure reporting. +- `progress = false` disables stage/task progress output consistently across + config file, env (`NETSUKE_PROGRESS`), and CLI (`--progress false`). +- Unit tests (`rstest`) and behavioural tests (`rstest-bdd` v0.5.0) cover + happy, unhappy, and edge conditions. +- `make check-fmt`, `make lint`, and `make test` succeed. +- `docs/users-guide.md`, `docs/netsuke-design.md`, and `docs/roadmap.md` are + updated and consistent. + +## Idempotence and recovery + +- Parser and reporter changes are additive and re-runnable; repeated test runs + should produce the same output assertions. +- If status parsing causes regressions, retain raw output forwarding and gate + parser callbacks behind `progress` while investigating. +- If fallback behaviour causes noisy logs, disable task updates via + `progress = false` temporarily and continue with non-progress execution while + preserving deterministic command output. + +## Artifacts and notes + +Capture concise evidence for review: + +- Parser unit test names and pass/fail status. +- BDD scenario names proving fallback behaviour. +- A short stderr excerpt showing Stage 6 textual updates in non-TTY mode. +- Command exit codes for `make check-fmt`, `make lint`, and `make test`. From c4d94697d24414db37e5d1ba4165d2f84011c802 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 00:48:56 +0000 Subject: [PATCH 02/14] feat(runner): parse Ninja status lines to emit task progress updates - Added parsing of Ninja status lines in `src/runner/process/ninja_status.rs`. - Enhanced process streaming to handle and observe parsed task progress. - Integrated task progress reporting in `StatusReporter` trait and implementations. - Updated `IndicatifReporter` to display task progress and fallback to textual output when needed. - Enable textual task updates on non-TTY stdout or accessible mode. - Added BDD and unit tests covering happy/unhappy parsing cases and progress emission. - Updated CLI flags documentation and localization keys/messages for task progress. - Added a fake ninja executable test fixture emitting task status lines. - Synchronized docs, roadmap, and user guides with the new progress feature. - Ensured monotonic acceptance of task progress to avoid regressive updates. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 62 +++- docs/netsuke-design.md | 16 +- docs/roadmap.md | 4 +- docs/users-guide.md | 34 ++- locales/en-US/messages.ftl | 5 +- locales/es-ES/messages.ftl | 5 +- src/localization/keys.rs | 3 + src/runner/mod.rs | 41 ++- src/runner/process/mod.rs | 277 +++++++----------- src/runner/process/ninja_status.rs | 135 +++++++++ src/runner/process/streaming.rs | 272 +++++++++++++++++ src/runner/tests.rs | 16 + src/status.rs | 83 +++++- src/status_tests.rs | 38 +++ tests/bdd/steps/mod.rs | 1 + tests/bdd/steps/progress_output.rs | 85 ++++++ tests/features/progress_output.feature | 27 +- tests/localization_tests.rs | 32 ++ 18 files changed, 917 insertions(+), 219 deletions(-) create mode 100644 src/runner/process/ninja_status.rs create mode 100644 src/runner/process/streaming.rs create mode 100644 tests/bdd/steps/progress_output.rs diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 231c4c6d..05f67b31 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -4,7 +4,7 @@ This ExecPlan is a living document. The sections `Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. -Status: DRAFT +Status: DONE (2026-02-24) No `PLANS.md` file exists in this repository. @@ -96,12 +96,26 @@ Observable success: surfaces. - [x] (2026-02-22 17:55Z) Drafted this ExecPlan at `docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md`. -- [ ] Stage A: Add Ninja status parser and forwarding hooks. -- [ ] Stage B: Wire parsed task progress into status reporters. -- [ ] Stage C: Add textual fallback behaviour for non-TTY stdout and - accessible mode. -- [ ] Stage D: Extend unit and behavioural tests (happy/unhappy/edge cases). -- [ ] Stage E: Update docs/design/roadmap and run quality gates. +- [x] (2026-02-24) Stage A: Added Ninja status parser and forwarding hooks via + `src/runner/process/ninja_status.rs` and + `src/runner/process/streaming.rs`; process orchestration in + `src/runner/process/mod.rs` now accepts an internal status observer. +- [x] (2026-02-24) Stage B: Wired parsed task progress into reporters by + extending `StatusReporter` with `report_task_progress` and implementing + task-progress rendering in `IndicatifReporter` and + `AccessibleReporter`. +- [x] (2026-02-24) Stage C: Added centralized fallback behaviour in + `src/runner/mod.rs` (`should_force_text_task_updates`) to emit textual + task updates when stdout is non-TTY or accessible mode is enabled. +- [x] (2026-02-24) Stage D: Added/updated unit and behavioural coverage using + `rstest` and `rstest-bdd`: + `src/runner/process/ninja_status.rs`, + `src/runner/process/streaming.rs`, `src/status_tests.rs`, + `src/runner/tests.rs`, `tests/features/progress_output.feature`, and + `tests/bdd/steps/progress_output.rs`. +- [x] (2026-02-24) Stage E: Updated docs and roadmap: + `docs/users-guide.md`, `docs/netsuke-design.md`, `docs/roadmap.md`; + validated with `make check-fmt`, `make lint`, and `make test`. ## Surprises & Discoveries @@ -113,6 +127,12 @@ Observable success: execution with controlled Ninja output. - MCP project-memory tools (`qdrant-find` / `qdrant-store`) were not available in this environment; repository docs were used as the only source of truth. +- Fluent rendering includes bidi isolation markers in localized strings, so + brittle literal assertions were replaced with content assertions that strip + isolation code points in unit tests. +- The initial fake-Ninja fixture shell script used `cat`, which failed under + test PATH constraints. Replaced with shell built-ins (`read` + `printf`) to + keep fixtures deterministic. ## Decision Log @@ -135,14 +155,26 @@ Observable success: ## Outcomes & Retrospective -Pending implementation. - -Target outcomes: - -- Stage 6 task progress advances from parsed Ninja status lines. -- Non-TTY and accessible flows receive deterministic textual updates. -- Unit + BDD coverage proves happy, unhappy, and edge behaviour. -- Documentation and roadmap remain synchronized with implemented behaviour. +Implemented outcomes: + +- Stage 6 task progress now advances from parsed Ninja status lines in + `[current/total] description` form, with monotonic filtering to ignore + malformed and regressive updates. +- Non-TTY stdout and accessible mode now force deterministic textual task + updates while preserving standard `indicatif` stage rendering elsewhere. +- Unit + BDD coverage now includes parser happy/unhappy cases, monotonic guard + behaviour, fallback predicate combinations, and behavioural scenarios using + deterministic fake Ninja output. +- Documentation and roadmap were synchronized: + `docs/users-guide.md`, `docs/netsuke-design.md`, `docs/roadmap.md`. + +Validation summary: + +- `make check-fmt`: pass. +- `make lint`: pass. +- `make test`: pass. +- Additional doc gates run after doc updates: + `make fmt`, `make markdownlint`, `make nixie`. ## Context and orientation diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index ae37d449..a4835624 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2043,12 +2043,16 @@ Real-time stage reporting now uses a six-stage model in `src/status.rs` backed by `indicatif::MultiProgress` for standard terminals. The reporter keeps one persistent summary line per stage and updates each line through localized state labels (`pending`, `in progress`, `done`, `failed`) plus localized stage text. -When stderr is not a TTY, the same reporter falls back to emitting localized -summary lines so non-interactive runs still surface deterministic stage state. -Accessible output remains text-first and static; it does not animate. The -standard reporter is configurable through OrthoConfig layering via -`progress: Option` (`--progress`, `NETSUKE_PROGRESS`, or config file), -with accessible mode taking precedence when enabled. +During Stage 6, Netsuke parses Ninja status lines of the form +`[current/total] ...` and emits localized task progress updates. Parsed updates +are monotonic: malformed lines, regressive counts, and total-mismatch lines are +ignored to avoid noisy or inconsistent progress state. Task updates fall back +to textual output when stdout is not a TTY, ensuring deterministic CI/log +output; accessible mode always uses textual output. Accessible output remains +text-first and static; it does not animate. The standard reporter is +configurable through OrthoConfig layering via `progress: Option` +(`--progress`, `NETSUKE_PROGRESS`, or config file), with accessible mode taking +precedence when enabled. For screen readers: The following flowchart shows how the build script audits localization keys against English and Spanish Fluent bundles. diff --git a/docs/roadmap.md b/docs/roadmap.md index 5b18a8b3..02b49b67 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -260,8 +260,8 @@ library, and CLI ergonomics. - [x] 3.9.1. Integrate `indicatif::MultiProgress`. - [x] Surface the six pipeline stages with persistent summaries. - [x] Apply localization-aware labelling. -- [ ] 3.9.2. Parse Ninja status lines to drive task progress. - - [ ] Emit fallback textual updates when stdout is not a TTY or accessible +- [x] 3.9.2. Parse Ninja status lines to drive task progress. + - [x] Emit fallback textual updates when stdout is not a TTY or accessible mode is active. - [ ] 3.9.3. Capture per-stage timing metrics in verbose mode. - [ ] Include metrics in completion summary. diff --git a/docs/users-guide.md b/docs/users-guide.md index aaa39a06..bc647a75 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -569,9 +569,8 @@ For information on contributing translations, see the ### Accessible output mode -Netsuke supports an accessible output mode that replaces animated progress -indicators with static, labelled status lines suitable for screen readers and -dumb terminals. +Netsuke supports an accessible output mode that uses static, labelled status +lines suitable for screen readers and dumb terminals. Accessible mode is auto-enabled when: @@ -592,16 +591,31 @@ When accessible mode is active, each pipeline stage produces a labelled status line on stderr: ```text -Stage 1/5: Configuring network policy -Stage 2/5: Loading manifest -Stage 3/5: Building dependency graph -Stage 4/5: Generating Ninja file -Stage 5/5: Executing Build +Stage 1/6: Reading manifest file +Stage 2/6: Parsing YAML document +Stage 3/6: Expanding template directives +Stage 4/6: Deserializing and rendering manifest values +Stage 5/6: Building and validating dependency graph +Stage 6/6: Synthesizing Ninja plan and executing Build +Task 1/2: cc -c src/a.c +Task 2/2: cc -c src/b.c Build complete. ``` -In standard mode, no status lines are emitted. Future versions may add animated -progress indicators for standard mode terminals. +In standard mode, Netsuke uses `indicatif` stage summaries when progress is +enabled. During Stage 6, Netsuke parses Ninja status lines (`[current/total]`) +and emits task progress updates. When stdout is not a TTY, task progress +automatically falls back to textual updates so CI and redirected logs remain +readable. + +Progress output can be controlled via OrthoConfig layering: + +- CLI flag: `--progress true` or `--progress false` +- Environment variable: `NETSUKE_PROGRESS=true|false` +- Configuration file: `progress = true|false` + +When progress is disabled, Netsuke suppresses stage and task progress output in +both standard and accessible modes. ### Emoji and accessibility preferences diff --git a/locales/en-US/messages.ftl b/locales/en-US/messages.ftl index 24861245..cd8cb846 100644 --- a/locales/en-US/messages.ftl +++ b/locales/en-US/messages.ftl @@ -15,7 +15,7 @@ cli.flag.fetch_allow_host.help = Hostnames that are permitted when default deny cli.flag.fetch_block_host.help = Hostnames that are always blocked, even when allowed elsewhere. cli.flag.fetch_default_deny.help = Deny all hosts by default; only allow the declared allowlist. cli.flag.accessible.help = Force accessible output mode on or off. -cli.flag.progress.help = Force standard progress summaries on or off. +cli.flag.progress.help = Force standard stage and task progress summaries on or off. # Subcommand descriptions. cli.subcommand.build.about = Build targets defined in the manifest (default). @@ -325,6 +325,9 @@ status.state.done = done status.state.failed = failed status.stage.label = Stage { $current }/{ $total }: { $description } status.stage.summary = [{ $state }] { $label } +status.stage.summary_with_task = [{ $state }] { $label } ({ $task_progress }) +status.task.progress_label = Task { $current }/{ $total } +status.task.progress_update = { $task }: { $description } status.stage.manifest_ingestion = Reading manifest file status.stage.initial_yaml_parsing = Parsing YAML document status.stage.template_expansion = Expanding template directives diff --git a/locales/es-ES/messages.ftl b/locales/es-ES/messages.ftl index 8cd2c832..022c8da2 100644 --- a/locales/es-ES/messages.ftl +++ b/locales/es-ES/messages.ftl @@ -15,7 +15,7 @@ cli.flag.fetch_allow_host.help = Nombres de host permitidos cuando la denegació cli.flag.fetch_block_host.help = Nombres de host siempre bloqueados, incluso cuando están permitidos. cli.flag.fetch_default_deny.help = Denegar todos los hosts por defecto; solo permitir la lista de permitidos. cli.flag.accessible.help = Forzar el modo de salida accesible (activado o desactivado). -cli.flag.progress.help = Forzar los resúmenes de progreso estándar (activados o desactivados). +cli.flag.progress.help = Forzar los resúmenes de progreso estándar de etapas y tareas (activados o desactivados). # Descripciones de subcomandos. cli.subcommand.build.about = Compila objetivos definidos en el manifiesto (predeterminado). @@ -325,6 +325,9 @@ status.state.done = completada status.state.failed = fallida status.stage.label = Etapa { $current }/{ $total }: { $description } status.stage.summary = [{ $state }] { $label } +status.stage.summary_with_task = [{ $state }] { $label } ({ $task_progress }) +status.task.progress_label = Tarea { $current }/{ $total } +status.task.progress_update = { $task }: { $description } status.stage.manifest_ingestion = Leyendo el archivo del manifiesto status.stage.initial_yaml_parsing = Analizando el documento YAML status.stage.template_expansion = Expandiendo directivas de plantilla diff --git a/src/localization/keys.rs b/src/localization/keys.rs index ef81b2ba..3aaf593b 100644 --- a/src/localization/keys.rs +++ b/src/localization/keys.rs @@ -284,6 +284,9 @@ define_keys! { STATUS_STATE_FAILED => "status.state.failed", STATUS_STAGE_LABEL => "status.stage.label", STATUS_STAGE_SUMMARY => "status.stage.summary", + STATUS_STAGE_SUMMARY_WITH_TASK => "status.stage.summary_with_task", + STATUS_TASK_PROGRESS_LABEL => "status.task.progress_label", + STATUS_TASK_PROGRESS_UPDATE => "status.task.progress_update", STATUS_STAGE_MANIFEST_INGESTION => "status.stage.manifest_ingestion", STATUS_STAGE_INITIAL_YAML_PARSING => "status.stage.initial_yaml_parsing", STATUS_STAGE_TEMPLATE_EXPANSION => "status.stage.template_expansion", diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 6980de77..2a581271 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -20,6 +20,7 @@ use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result}; use camino::Utf8PathBuf; use std::borrow::Cow; +use std::io::IsTerminal; use std::path::Path; use tempfile::NamedTempFile; use tracing::{debug, info}; @@ -97,14 +98,20 @@ fn make_reporter( mode: OutputMode, progress_enabled: bool, prefs: OutputPrefs, + stdout_is_tty: bool, ) -> Box { + let force_text_task_updates = should_force_text_task_updates(mode, stdout_is_tty); match (mode, progress_enabled) { (OutputMode::Accessible, _) => Box::new(AccessibleReporter::new(prefs)), - (OutputMode::Standard, true) => Box::new(IndicatifReporter::new()), + (OutputMode::Standard, true) => Box::new(IndicatifReporter::new(force_text_task_updates)), (OutputMode::Standard, false) => Box::new(SilentReporter), } } +const fn should_force_text_task_updates(mode: OutputMode, stdout_is_tty: bool) -> bool { + mode.is_accessible() || !stdout_is_tty +} + /// Execute the parsed [`Cli`] commands with the given output preferences. /// /// # Errors @@ -112,7 +119,9 @@ fn make_reporter( /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { let mode = output_mode::resolve(cli.accessible); - let reporter = make_reporter(mode, cli.progress.unwrap_or(true), prefs); + let progress_enabled = cli.progress.unwrap_or(true); + let stdout_is_tty = std::io::stdout().is_terminal(); + let reporter = make_reporter(mode, progress_enabled, prefs, stdout_is_tty); let command = cli.command.clone().unwrap_or(Commands::Build(BuildArgs { emit: None, @@ -172,7 +181,19 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R } let program = process::resolve_ninja_program(); - run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).with_context(|| { + let mut on_task_progress = |current: u32, total: u32, description: &str| { + reporter.report_task_progress(current, total, description); + }; + process::run_ninja_invocation( + &process::NinjaInvocation::Build { + program: program.as_path(), + cli, + build_file: build_path.as_ref(), + targets: &targets, + }, + Some(&mut on_task_progress), + ) + .with_context(|| { format!( "running {} with build file {}", program.display(), @@ -209,7 +230,19 @@ fn handle_ninja_tool( let build_path = tmp.path(); let program = process::resolve_ninja_program(); - run_ninja_tool(program.as_path(), cli, build_path, tool).with_context(|| { + let mut on_task_progress = |current: u32, total: u32, description: &str| { + reporter.report_task_progress(current, total, description); + }; + process::run_ninja_invocation( + &process::NinjaInvocation::Tool { + program: program.as_path(), + cli, + build_file: build_path, + tool, + }, + Some(&mut on_task_progress), + ) + .with_context(|| { format!( "running {} -t {} with build file {}", program.display(), diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 932c525f..68d15d84 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -8,7 +8,7 @@ use ninja_env::NINJA_ENV; use std::{ env, ffi::OsString, - io::{self, BufReader, ErrorKind, Read, Write}, + io::{self, BufReader, ErrorKind}, path::{Path, PathBuf}, process::{Child, Command, ExitStatus, Stdio}, thread, @@ -16,12 +16,35 @@ use std::{ use tracing::info; mod file_io; +mod ninja_status; mod paths; mod redaction; +mod streaming; pub use file_io::*; pub use paths::*; use redaction::{CommandArg, redact_sensitive_args}; +use streaming::{ForwardStats, forward_child_output, forward_child_output_with_ninja_status}; + +type StatusObserver<'a> = &'a mut dyn FnMut(u32, u32, &str); + +/// Prepared Ninja invocation variant. +pub(crate) enum NinjaInvocation<'a> { + /// Standard build invocation. + Build { + program: &'a Path, + cli: &'a Cli, + build_file: &'a Path, + targets: &'a BuildTargets<'a>, + }, + /// Ninja tool invocation (`ninja -t `). + Tool { + program: &'a Path, + cli: &'a Cli, + build_file: &'a Path, + tool: &'a str, + }, +} // Public helpers for doctests only. This exposes internal helpers as a stable // testing surface without exporting them in release builds. @@ -61,8 +84,8 @@ pub fn resolve_ninja_program() -> PathBuf { /// Configure the base Ninja command with working directory, job count, and build file. /// -/// Sets up stdout/stderr pipes for streaming. Callers append targets or tool flags -/// after this function returns. +/// Sets up stdout/stderr pipes for streaming. Callers append targets or tool +/// flags after this function returns. fn configure_ninja_base(cmd: &mut Command, cli: &Cli, build_file: &Path) -> io::Result<()> { if let Some(dir) = &cli.directory { let canonical = canonicalize_utf8_path(dir.as_path())?; @@ -130,10 +153,13 @@ fn log_command_execution(cmd: &Command) { } /// Log, spawn, stream output, and check exit status for a prepared command. -fn run_command_and_stream(mut cmd: Command) -> io::Result<()> { +fn run_command_and_stream( + mut cmd: Command, + status_observer: Option>, +) -> io::Result<()> { log_command_execution(&cmd); let child = cmd.spawn()?; - let status = spawn_and_stream_output(child)?; + let status = spawn_and_stream_output(child, status_observer)?; check_exit_status(status) } @@ -147,16 +173,19 @@ fn run_command_and_stream(mut cmd: Command) -> io::Result<()> { /// /// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard /// streams are unavailable, or when Ninja reports a non-zero exit status. -/// pub fn run_ninja( program: &Path, cli: &Cli, build_file: &Path, targets: &BuildTargets<'_>, ) -> io::Result<()> { - let mut cmd = Command::new(program); - configure_ninja_build_command(&mut cmd, cli, build_file, targets)?; - run_command_and_stream(cmd) + let invocation = NinjaInvocation::Build { + program, + cli, + build_file, + targets, + }; + run_ninja_invocation(&invocation, None) } /// Invoke a Ninja tool (e.g., `ninja -t clean`) with the provided CLI settings. @@ -170,27 +199,63 @@ pub fn run_ninja( /// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard /// streams are unavailable, or when Ninja reports a non-zero exit status. pub fn run_ninja_tool(program: &Path, cli: &Cli, build_file: &Path, tool: &str) -> io::Result<()> { - let mut cmd = Command::new(program); - configure_ninja_tool_command(&mut cmd, cli, build_file, tool)?; - run_command_and_stream(cmd) + let invocation = NinjaInvocation::Tool { + program, + cli, + build_file, + tool, + }; + run_ninja_invocation(&invocation, None) +} + +/// Invoke Ninja and stream parsed task updates from status lines. +pub(crate) fn run_ninja_invocation( + invocation: &NinjaInvocation<'_>, + status_observer: Option>, +) -> io::Result<()> { + match invocation { + NinjaInvocation::Build { + program, + cli, + build_file, + targets, + } => { + let mut cmd = Command::new(program); + configure_ninja_build_command(&mut cmd, cli, build_file, targets)?; + run_command_and_stream(cmd, status_observer) + } + NinjaInvocation::Tool { + program, + cli, + build_file, + tool, + } => { + let mut cmd = Command::new(program); + configure_ninja_tool_command(&mut cmd, cli, build_file, tool)?; + run_command_and_stream(cmd, status_observer) + } + } +} + +fn handle_forwarding_stats(stats: ForwardStats, stream_name: &str) { + if stats.write_failed { + tracing::debug!("{stream_name} forwarding encountered closed pipe; output truncated"); + } } fn handle_forwarding_thread_result(result: thread::Result, stream_name: &str) { match result { - Ok(stats) => { - if stats.write_failed { - tracing::debug!( - "{stream_name} forwarding encountered closed pipe; output truncated" - ); - } - } + Ok(stats) => handle_forwarding_stats(stats, stream_name), Err(err) => { tracing::warn!("{stream_name} forwarding thread panicked: {err:?}"); } } } -fn spawn_and_stream_output(mut child: Child) -> io::Result { +fn spawn_and_stream_output( + mut child: Child, + status_observer: Option>, +) -> io::Result { let Some(stdout) = child.stdout.take() else { terminate_child(&mut child, "stdout pipe unavailable"); return Err(io::Error::other("child process missing stdout pipe")); @@ -200,17 +265,26 @@ fn spawn_and_stream_output(mut child: Child) -> io::Result { return Err(io::Error::other("child process missing stderr pipe")); }; - let out_handle = thread::spawn(move || { - let mut lock = io::stdout().lock(); - forward_child_output(BufReader::new(stdout), &mut lock, "stdout") - }); let err_handle = thread::spawn(move || { let mut lock = io::stderr().lock(); forward_child_output(BufReader::new(stderr), &mut lock, "stderr") }); + let stdout_stats = { + let mut lock = io::stdout().lock(); + match status_observer { + Some(observer) => forward_child_output_with_ninja_status( + BufReader::new(stdout), + &mut lock, + observer, + "stdout", + ), + None => forward_child_output(BufReader::new(stdout), &mut lock, "stdout"), + } + }; + let status = child.wait()?; - handle_forwarding_thread_result(out_handle.join(), "stdout"); + handle_forwarding_stats(stdout_stats, "stdout"); handle_forwarding_thread_result(err_handle.join(), "stderr"); Ok(status) } @@ -224,98 +298,6 @@ fn terminate_child(child: &mut Child, context: &str) { } } -#[derive(Debug, Default, PartialEq, Eq)] -struct ForwardStats { - bytes_read: usize, - bytes_written: usize, - write_failed: bool, -} - -struct CountingReader<'a, R> { - inner: &'a mut R, - read: u64, -} - -impl Read for CountingReader<'_, R> { - fn read(&mut self, buf: &mut [u8]) -> io::Result { - let count = self.inner.read(buf)?; - self.read += count as u64; - Ok(count) - } -} - -struct CountingWriter<'a, W> { - inner: &'a mut W, - written: u64, -} - -impl Write for CountingWriter<'_, W> { - fn write(&mut self, buf: &[u8]) -> io::Result { - match self.inner.write(buf) { - Ok(count) => { - self.written += count as u64; - Ok(count) - } - Err(err) => Err(err), - } - } - - fn flush(&mut self) -> io::Result<()> { - self.inner.flush() - } -} - -fn clamp_u64_to_usize(value: u64) -> usize { - usize::try_from(value).unwrap_or(usize::MAX) -} - -fn forward_child_output( - mut reader: R, - mut writer: W, - stream_name: &'static str, -) -> ForwardStats -where - R: Read, - W: Write, -{ - let mut stats = ForwardStats::default(); - let mut counting_reader = CountingReader { - inner: &mut reader, - read: 0, - }; - let mut counting_writer = CountingWriter { - inner: &mut writer, - written: 0, - }; - - match io::copy(&mut counting_reader, &mut counting_writer) { - Ok(_) => { - stats.bytes_written = clamp_u64_to_usize(counting_writer.written); - stats.bytes_read = clamp_u64_to_usize(counting_reader.read); - } - Err(err) => { - stats.write_failed = true; - stats.bytes_written = clamp_u64_to_usize(counting_writer.written); - stats.bytes_read = clamp_u64_to_usize(counting_reader.read); - tracing::debug!( - "Failed to write child {stream_name} output to parent: {err}; discarding remaining bytes" - ); - match io::copy(&mut counting_reader, &mut io::sink()) { - Ok(_) => { - stats.bytes_read = clamp_u64_to_usize(counting_reader.read); - } - Err(drain_err) => { - tracing::debug!( - "Failed to drain child {stream_name} output after writer closed: {drain_err}" - ); - } - } - } - } - - stats -} - fn check_exit_status(status: ExitStatus) -> io::Result<()> { if status.success() { Ok(()) @@ -335,41 +317,7 @@ fn check_exit_status(status: ExitStatus) -> io::Result<()> { mod tests { use super::*; use camino::Utf8PathBuf; - use std::{ - ffi::OsString, - io::Cursor, - sync::{ - Arc, - atomic::{AtomicUsize, Ordering}, - }, - }; - - #[derive(Clone)] - struct FailingWriter { - writes: Arc, - } - - impl FailingWriter { - fn new(writes: Arc) -> Self { - Self { writes } - } - } - - impl Write for FailingWriter { - fn write(&mut self, _buf: &[u8]) -> io::Result { - let previous = self.writes.fetch_add(1, Ordering::SeqCst); - let error_kind = if previous == 0 { - io::ErrorKind::BrokenPipe - } else { - io::ErrorKind::Other - }; - Err(io::Error::new(error_kind, "sink closed")) - } - - fn flush(&mut self) -> io::Result<()> { - Ok(()) - } - } + use std::ffi::OsString; #[test] fn resolve_ninja_program_utf8_prefers_env_override() { @@ -393,29 +341,4 @@ mod tests { }); assert_eq!(resolved, Utf8PathBuf::from(NINJA_PROGRAM)); } - - #[test] - fn forward_output_writes_all_bytes_when_parent_alive() { - let input = b"alpha\nbravo\ncharlie\n".to_vec(); - let reader = BufReader::new(Cursor::new(input.clone())); - let stats = forward_child_output(reader, Vec::new(), "stdout"); - - assert_eq!(stats.bytes_read, input.len()); - assert_eq!(stats.bytes_written, input.len()); - assert!(!stats.write_failed); - } - - #[test] - fn forward_output_continues_draining_after_write_failure() { - let input = b"echo-one\necho-two\necho-three\n".to_vec(); - let reader = BufReader::new(Cursor::new(input.clone())); - let write_attempts = Arc::new(AtomicUsize::new(0)); - let failing_writer = FailingWriter::new(write_attempts.clone()); - let stats = forward_child_output(reader, failing_writer, "stdout"); - - assert_eq!(stats.bytes_read, input.len()); - assert_eq!(write_attempts.load(Ordering::SeqCst), 1); - assert!(stats.write_failed); - assert_eq!(stats.bytes_written, 0); - } } diff --git a/src/runner/process/ninja_status.rs b/src/runner/process/ninja_status.rs new file mode 100644 index 00000000..2ee25086 --- /dev/null +++ b/src/runner/process/ninja_status.rs @@ -0,0 +1,135 @@ +//! Ninja status-line parsing for task progress updates. + +/// Parsed task progress from a Ninja status line. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) struct NinjaTaskProgress { + current: u32, + total: u32, + description: String, +} + +impl NinjaTaskProgress { + /// Build a parsed status update. + pub(super) const fn new(current: u32, total: u32, description: String) -> Self { + Self { + current, + total, + description, + } + } + + /// Return the completed task count. + pub(super) const fn current(&self) -> u32 { + self.current + } + + /// Return the total task count. + pub(super) const fn total(&self) -> u32 { + self.total + } + + /// Return the trailing human-readable status text. + pub(super) fn description(&self) -> &str { + &self.description + } +} + +/// Tracks task updates and filters regressive or inconsistent lines. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(super) struct NinjaTaskProgressTracker { + total: Option, + last_current: u32, +} + +impl NinjaTaskProgressTracker { + /// Accept a new update when it is consistent and monotonic. + pub(super) const fn accept(&mut self, update: &NinjaTaskProgress) -> bool { + if update.total() == 0 || update.current() == 0 || update.current() > update.total() { + return false; + } + match self.total { + Some(total) if total != update.total() || update.current() < self.last_current => false, + _ => { + self.total = Some(update.total()); + self.last_current = update.current(); + true + } + } + } +} + +/// Parse a Ninja status line in the form `[current/total] description`. +pub(super) fn parse_ninja_status_line(line: &str) -> Option { + let trimmed = line.trim_start(); + let rest = trimmed.strip_prefix('[')?; + let (current_raw, remaining) = rest.split_once('/')?; + let (total_raw, description_raw) = remaining.split_once(']')?; + if current_raw.is_empty() + || total_raw.is_empty() + || !current_raw.bytes().all(|byte| byte.is_ascii_digit()) + || !total_raw.bytes().all(|byte| byte.is_ascii_digit()) + { + return None; + } + let current = current_raw.parse::().ok()?; + let total = total_raw.parse::().ok()?; + let description = description_raw + .trim_start() + .trim_end_matches('\r') + .to_owned(); + Some(NinjaTaskProgress::new(current, total, description)) +} + +#[cfg(test)] +mod tests { + use super::{NinjaTaskProgressTracker, parse_ninja_status_line}; + use rstest::rstest; + + #[rstest] + #[case("[1/3] cc -c src/a.c", Some((1, 3, "cc -c src/a.c")))] + #[case(" [2/3] cc -c src/b.c", Some((2, 3, "cc -c src/b.c")))] + #[case("[3/3]\r", Some((3, 3, "")))] + #[case("no prefix", None)] + #[case("[/3] invalid", None)] + #[case("[2/] invalid", None)] + #[case("[two/3] invalid", None)] + #[case("[2/three] invalid", None)] + #[case("[4/3] invalid", Some((4, 3, "invalid")))] + fn parse_ninja_status_line_parses_expected( + #[case] line: &str, + #[case] expected: Option<(u32, u32, &str)>, + ) { + let parsed = parse_ninja_status_line(line); + let actual = parsed.map(|progress| { + ( + progress.current(), + progress.total(), + progress.description().to_owned(), + ) + }); + let expected_owned = + expected.map(|(current, total, description)| (current, total, description.to_owned())); + assert_eq!(actual, expected_owned); + } + + #[rstest] + #[case(vec!["[1/3] a", "[2/3] b", "[3/3] c"], vec![true, true, true])] + #[case(vec!["[2/3] b", "[1/3] a"], vec![true, false])] + #[case(vec!["[1/3] a", "[2/4] b"], vec![true, false])] + #[case(vec!["[1/3] a", "[1/3] a"], vec![true, true])] + #[case(vec!["[0/3] a"], vec![false])] + #[case(vec!["[1/0] a"], vec![false])] + fn tracker_accepts_only_monotonic_updates( + #[case] lines: Vec<&str>, + #[case] expected: Vec, + ) { + let mut tracker = NinjaTaskProgressTracker::default(); + let actual: Vec = lines + .into_iter() + .map(|line| { + parse_ninja_status_line(line).is_some_and(|progress| tracker.accept(&progress)) + }) + .collect(); + assert_eq!(actual, expected); + } +} diff --git a/src/runner/process/streaming.rs b/src/runner/process/streaming.rs new file mode 100644 index 00000000..e8584d63 --- /dev/null +++ b/src/runner/process/streaming.rs @@ -0,0 +1,272 @@ +//! Streaming helpers for subprocess output forwarding. + +use super::ninja_status::{NinjaTaskProgressTracker, parse_ninja_status_line}; +use std::io::{self, Read, Write}; + +/// Forwarding statistics for a child output stream. +#[derive(Debug, Default, PartialEq, Eq, Clone, Copy)] +pub(super) struct ForwardStats { + pub(super) bytes_read: usize, + pub(super) bytes_written: usize, + pub(super) write_failed: bool, +} + +struct CountingReader<'a, R> { + inner: &'a mut R, + read: u64, +} + +impl Read for CountingReader<'_, R> { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let count = self.inner.read(buf)?; + self.read = self.read.saturating_add(count as u64); + Ok(count) + } +} + +struct CountingWriter<'a, W> { + inner: &'a mut W, + written: u64, +} + +impl Write for CountingWriter<'_, W> { + fn write(&mut self, buf: &[u8]) -> io::Result { + let count = self.inner.write(buf)?; + self.written = self.written.saturating_add(count as u64); + Ok(count) + } + + fn flush(&mut self) -> io::Result<()> { + self.inner.flush() + } +} + +struct NinjaStatusParsingReader<'a, R, F> { + inner: &'a mut R, + tracker: NinjaTaskProgressTracker, + pending_line: Vec, + observer: &'a mut F, +} + +impl NinjaStatusParsingReader<'_, R, F> { + fn consume_bytes(&mut self, bytes: &[u8]) + where + F: FnMut(u32, u32, &str), + { + for byte in bytes { + if *byte == b'\n' { + self.finish_line(); + } else { + self.pending_line.push(*byte); + } + } + } + + fn finish_line(&mut self) + where + F: FnMut(u32, u32, &str), + { + if self.pending_line.is_empty() { + return; + } + let text = String::from_utf8_lossy(&self.pending_line); + if let Some(progress) = parse_ninja_status_line(text.as_ref()) + && self.tracker.accept(&progress) + { + (self.observer)(progress.current(), progress.total(), progress.description()); + } + self.pending_line.clear(); + } +} + +impl Read for NinjaStatusParsingReader<'_, R, F> +where + R: Read, + F: FnMut(u32, u32, &str), +{ + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let count = self.inner.read(buf)?; + if count == 0 { + self.finish_line(); + return Ok(0); + } + let Some(slice) = buf.get(..count) else { + return Ok(0); + }; + self.consume_bytes(slice); + Ok(count) + } +} + +fn clamp_u64_to_usize(value: u64) -> usize { + usize::try_from(value).unwrap_or(usize::MAX) +} + +fn copy_with_stats(reader: &mut R, writer: &mut W, stream_name: &'static str) -> ForwardStats +where + R: Read, + W: Write, +{ + let mut stats = ForwardStats::default(); + let mut counting_reader = CountingReader { + inner: reader, + read: 0, + }; + let mut counting_writer = CountingWriter { + inner: writer, + written: 0, + }; + + match io::copy(&mut counting_reader, &mut counting_writer) { + Ok(_) => { + stats.bytes_read = clamp_u64_to_usize(counting_reader.read); + stats.bytes_written = clamp_u64_to_usize(counting_writer.written); + } + Err(err) => { + stats.write_failed = true; + stats.bytes_read = clamp_u64_to_usize(counting_reader.read); + stats.bytes_written = clamp_u64_to_usize(counting_writer.written); + tracing::debug!( + "Failed to write child {stream_name} output to parent: {err}; discarding remaining bytes" + ); + if let Err(drain_err) = io::copy(&mut counting_reader, &mut io::sink()) { + tracing::debug!( + "Failed to drain child {stream_name} output after writer closed: {drain_err}" + ); + } else { + stats.bytes_read = clamp_u64_to_usize(counting_reader.read); + } + } + } + stats +} + +/// Forward child output to a writer while tracking read/write statistics. +pub(super) fn forward_child_output( + mut reader: R, + mut writer: W, + stream_name: &'static str, +) -> ForwardStats +where + R: Read, + W: Write, +{ + copy_with_stats(&mut reader, &mut writer, stream_name) +} + +/// Forward child output and parse Ninja status updates from complete lines. +pub(super) fn forward_child_output_with_ninja_status( + mut reader: R, + mut writer: W, + mut observer: F, + stream_name: &'static str, +) -> ForwardStats +where + R: Read, + W: Write, + F: FnMut(u32, u32, &str), +{ + let mut parsing_reader = NinjaStatusParsingReader { + inner: &mut reader, + tracker: NinjaTaskProgressTracker::default(), + pending_line: Vec::new(), + observer: &mut observer, + }; + copy_with_stats(&mut parsing_reader, &mut writer, stream_name) +} + +#[cfg(test)] +mod tests { + use super::{forward_child_output, forward_child_output_with_ninja_status}; + use std::{ + io::{BufReader, Cursor, Write}, + sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }, + }; + + #[derive(Clone)] + struct FailingWriter { + writes: Arc, + } + + impl FailingWriter { + fn new(writes: Arc) -> Self { + Self { writes } + } + } + + impl Write for FailingWriter { + fn write(&mut self, _buf: &[u8]) -> std::io::Result { + let previous = self.writes.fetch_add(1, Ordering::SeqCst); + let error_kind = if previous == 0 { + std::io::ErrorKind::BrokenPipe + } else { + std::io::ErrorKind::Other + }; + Err(std::io::Error::new(error_kind, "sink closed")) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + #[test] + fn forward_output_writes_all_bytes_when_parent_alive() { + let input = b"alpha\nbravo\ncharlie\n".to_vec(); + let reader = BufReader::new(Cursor::new(input.clone())); + let stats = forward_child_output(reader, Vec::new(), "stdout"); + + assert_eq!(stats.bytes_read, input.len()); + assert_eq!(stats.bytes_written, input.len()); + assert!(!stats.write_failed); + } + + #[test] + fn forward_output_continues_draining_after_write_failure() { + let input = b"echo-one\necho-two\necho-three\n".to_vec(); + let reader = BufReader::new(Cursor::new(input.clone())); + let write_attempts = Arc::new(AtomicUsize::new(0)); + let failing_writer = FailingWriter::new(write_attempts.clone()); + let stats = forward_child_output(reader, failing_writer, "stdout"); + + assert_eq!(stats.bytes_read, input.len()); + assert_eq!(write_attempts.load(Ordering::SeqCst), 1); + assert!(stats.write_failed); + assert_eq!(stats.bytes_written, 0); + } + + #[test] + fn forward_output_with_ninja_status_parses_monotonic_updates() { + let input = concat!( + "[1/3] cc -c a.c\n", + "warning: not a status line\n", + "[2/3] cc -c b.c\n", + "[1/3] stale line\n", + "[3/3] cc -c c.c\n", + ); + let reader = BufReader::new(Cursor::new(input.as_bytes().to_vec())); + let mut updates = Vec::new(); + let stats = forward_child_output_with_ninja_status( + reader, + Vec::new(), + |current, total, description| { + updates.push((current, total, description.to_owned())); + }, + "stdout", + ); + + assert_eq!(stats.bytes_read, input.len()); + assert_eq!(stats.bytes_written, input.len()); + assert_eq!( + updates, + vec![ + (1, 3, "cc -c a.c".to_owned()), + (2, 3, "cc -c b.c".to_owned()), + (3, 3, "cc -c c.c".to_owned()), + ] + ); + } +} diff --git a/src/runner/tests.rs b/src/runner/tests.rs index 4e44375f..2cea902e 100644 --- a/src/runner/tests.rs +++ b/src/runner/tests.rs @@ -20,3 +20,19 @@ fn resolve_output_path_respects_directory( let resolved = resolve_output_path(&cli, Path::new(input)); assert_eq!(resolved.as_ref(), Path::new(expected)); } + +#[rstest] +#[case(OutputMode::Standard, true, false)] +#[case(OutputMode::Standard, false, true)] +#[case(OutputMode::Accessible, true, true)] +#[case(OutputMode::Accessible, false, true)] +fn force_text_task_updates_when_required( + #[case] mode: OutputMode, + #[case] stdout_is_tty: bool, + #[case] expected: bool, +) { + assert_eq!( + should_force_text_task_updates(mode, stdout_is_tty), + expected + ); +} diff --git a/src/status.rs b/src/status.rs index 854cab00..2642da52 100644 --- a/src/status.rs +++ b/src/status.rs @@ -79,10 +79,30 @@ fn stage_summary( .to_string() } +fn task_progress_label(current: u32, total: u32) -> String { + localization::message(keys::STATUS_TASK_PROGRESS_LABEL) + .with_arg("current", current.to_string()) + .with_arg("total", total.to_string()) + .to_string() +} + +fn task_progress_update(current: u32, total: u32, description: &str) -> String { + let task = task_progress_label(current, total); + if description.is_empty() { + return task; + } + localization::message(keys::STATUS_TASK_PROGRESS_UPDATE) + .with_arg("task", task) + .with_arg("description", description) + .to_string() +} + /// Reports pipeline stage transitions and completion. pub trait StatusReporter { /// Emit a stage update. fn report_stage(&self, current: StageNumber, total: StageNumber, description: &str); + /// Emit task progress for Stage 6 execution. + fn report_task_progress(&self, _current: u32, _total: u32, _description: &str) {} /// Emit a final completion message. fn report_complete(&self, tool_key: LocalizationKey); } @@ -117,6 +137,11 @@ impl StatusReporter for AccessibleReporter { let message = localization::message(keys::STATUS_COMPLETE).with_arg("tool", tool); drop(writeln!(io::stderr(), "{prefix} {message}")); } + + fn report_task_progress(&self, current: u32, total: u32, description: &str) { + let message = task_progress_update(current, total, description); + drop(writeln!(io::stderr(), "{message}")); + } } /// Reporter that suppresses status output. @@ -135,6 +160,8 @@ struct IndicatifState { running_index: Option, completed: bool, is_hidden: bool, + force_text_task_updates: bool, + last_task_progress: Option<(u32, u32)>, } /// Standard reporter backed by `indicatif::MultiProgress`. @@ -145,7 +172,7 @@ pub struct IndicatifReporter { impl IndicatifReporter { /// Build a multi-progress reporter with one line per pipeline stage. #[must_use] - pub fn new() -> Self { + pub fn new(force_text_task_updates: bool) -> Self { let progress = MultiProgress::with_draw_target(ProgressDrawTarget::stderr_with_hz(12)); progress.set_move_cursor(false); let style = ProgressStyle::with_template("{msg}") @@ -176,6 +203,8 @@ impl IndicatifReporter { descriptions, running_index: None, completed: false, + force_text_task_updates, + last_task_progress: None, }), } } @@ -208,7 +237,7 @@ impl IndicatifReporter { impl Default for IndicatifReporter { fn default() -> Self { - Self::new() + Self::new(false) } } @@ -268,9 +297,55 @@ impl StatusReporter for IndicatifReporter { LocalizationKey::new(keys::STATUS_STATE_RUNNING), false, ); + if index != stage6_index() { + state.last_task_progress = None; + } state.running_index = Some(index); } + fn report_task_progress(&self, current: u32, total: u32, description: &str) { + if total == 0 || current == 0 || current > total { + return; + } + let mut state = self + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let stage_index = stage6_index(); + if stage_index >= state.bars.len() || state.running_index != Some(stage_index) { + return; + } + if let Some((last_current, last_total)) = state.last_task_progress + && (total != last_total || current < last_current) + { + return; + } + state.last_task_progress = Some((current, total)); + let task = task_progress_update(current, total, description); + if state.is_hidden || state.force_text_task_updates { + drop(writeln!(io::stderr(), "{task}")); + return; + } + let Some(stage_current_raw) = u32::try_from(stage_index + 1).ok() else { + return; + }; + let stage_current = StageNumber::new_unchecked(stage_current_raw); + let stage_description = state + .descriptions + .get(stage_index) + .map_or("", String::as_str); + let state_label = localization::message(keys::STATUS_STATE_RUNNING).to_string(); + let stage_line = stage_label(stage_current, PIPELINE_STAGE_TOTAL, stage_description); + let message = localization::message(keys::STATUS_STAGE_SUMMARY_WITH_TASK) + .with_arg("state", state_label) + .with_arg("label", stage_line) + .with_arg("task_progress", &task) + .to_string(); + if let Some(bar) = state.bars.get(stage_index) { + bar.set_message(message); + } + } + fn report_complete(&self, tool_key: LocalizationKey) { let mut state = self .state @@ -294,6 +369,10 @@ impl StatusReporter for IndicatifReporter { } } +const fn stage6_index() -> usize { + 5 +} + #[cfg(test)] #[path = "status_tests.rs"] mod tests; diff --git a/src/status_tests.rs b/src/status_tests.rs index 3dfc6b71..f961690b 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -3,6 +3,13 @@ use super::*; use rstest::rstest; +fn strip_isolates(value: &str) -> String { + value + .chars() + .filter(|ch| !matches!(ch, '\u{2068}' | '\u{2069}')) + .collect() +} + #[rstest] #[case(PipelineStage::ManifestIngestion, 1)] #[case(PipelineStage::InitialYamlParsing, 2)] @@ -31,3 +38,34 @@ fn localization_key_from_static_str() { let key: LocalizationKey = keys::STATUS_STATE_PENDING.into(); assert_eq!(key.as_str(), keys::STATUS_STATE_PENDING); } + +#[rstest] +#[case(1, 2, "cc -c src/main.c", "Task 1/2: cc -c src/main.c")] +#[case(2, 2, "", "Task 2/2")] +fn task_progress_update_formats_expected_text( + #[case] current: u32, + #[case] total: u32, + #[case] description: &str, + #[case] expected: &str, +) { + let rendered = task_progress_update(current, total, description); + assert_eq!(strip_isolates(&rendered), expected); +} + +#[test] +fn indicatif_reporter_rejects_regressive_task_updates() { + let reporter = IndicatifReporter::new(true); + reporter.report_stage( + PipelineStage::NinjaSynthesisAndExecution.index(), + PIPELINE_STAGE_TOTAL, + "Executing Build", + ); + reporter.report_task_progress(2, 3, "cc -c src/b.c"); + reporter.report_task_progress(1, 3, "stale"); + + let state = reporter + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + assert_eq!(state.last_task_progress, Some((2, 3))); +} diff --git a/tests/bdd/steps/mod.rs b/tests/bdd/steps/mod.rs index 84145278..cca6bb77 100644 --- a/tests/bdd/steps/mod.rs +++ b/tests/bdd/steps/mod.rs @@ -24,6 +24,7 @@ mod manifest; mod manifest_command; mod ninja; mod process; +mod progress_output; mod stdlib; // Step functions are registered via macros, so we don't need to re-export diff --git a/tests/bdd/steps/progress_output.rs b/tests/bdd/steps/progress_output.rs new file mode 100644 index 00000000..026dab21 --- /dev/null +++ b/tests/bdd/steps/progress_output.rs @@ -0,0 +1,85 @@ +//! Step definitions for progress-output scenarios that require fake Ninja output. + +use crate::bdd::fixtures::TestWorld; +use anyhow::{Context, Result}; +use std::fs; +use std::path::{Path, PathBuf}; +use test_support::env::{SystemEnv, override_ninja_env}; + +fn workspace_root(world: &TestWorld) -> Result { + let temp = world.temp_dir.borrow(); + let dir = temp.as_ref().context("temp dir has not been initialised")?; + Ok(dir.path().to_path_buf()) +} + +#[cfg(unix)] +fn make_script_executable(path: &Path) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + + let mut permissions = fs::metadata(path) + .with_context(|| format!("read metadata for {}", path.display()))? + .permissions(); + permissions.set_mode(0o755); + fs::set_permissions(path, permissions) + .with_context(|| format!("set executable bit for {}", path.display()))?; + Ok(()) +} + +#[cfg(not(unix))] +fn make_script_executable(_path: &Path) -> Result<()> { + Ok(()) +} + +fn build_fake_ninja_script(lines: &[&str]) -> String { + if cfg!(windows) { + let mut script = String::from("@echo off\r\n"); + for line in lines { + script.push_str("echo "); + script.push_str(line); + script.push_str("\r\n"); + } + script.push_str("exit /B 0\r\n"); + script + } else { + let mut script = String::from( + "#!/bin/sh\nwhile IFS= read -r line; do\n printf '%s\\n' \"$line\"\ndone <<'NETSUKE_STATUS'\n", + ); + for line in lines { + script.push_str(line); + script.push('\n'); + } + script.push_str("NETSUKE_STATUS\nexit 0\n"); + script + } +} + +fn fake_ninja_path(root: &Path) -> PathBuf { + if cfg!(windows) { + return root.join("fake-ninja-progress.cmd"); + } + root.join("fake-ninja-progress") +} + +fn install_fake_ninja(world: &TestWorld, lines: &[&str]) -> Result<()> { + let root = workspace_root(world)?; + let script_path = fake_ninja_path(&root); + let script = build_fake_ninja_script(lines); + fs::write(&script_path, script) + .with_context(|| format!("write fake ninja script {}", script_path.display()))?; + make_script_executable(&script_path)?; + + let env = SystemEnv::new(); + world.ninja_env_guard.borrow_mut().take(); + *world.ninja_env_guard.borrow_mut() = Some(override_ninja_env(&env, &script_path)); + Ok(()) +} + +#[rstest_bdd_macros::given("a fake ninja executable that emits task status lines")] +fn fake_ninja_emits_task_status_lines(world: &TestWorld) -> Result<()> { + install_fake_ninja(world, &["[1/2] cc -c src/a.c", "[2/2] cc -c src/b.c"]) +} + +#[rstest_bdd_macros::given("a fake ninja executable that emits malformed task status lines")] +fn fake_ninja_emits_malformed_task_status_lines(world: &TestWorld) -> Result<()> { + install_fake_ninja(world, &["[x/2] broken", "[2/] broken", "plain output only"]) +} diff --git a/tests/features/progress_output.feature b/tests/features/progress_output.feature index 6c01b2ea..34cdfb56 100644 --- a/tests/features/progress_output.feature +++ b/tests/features/progress_output.feature @@ -1,5 +1,28 @@ Feature: Progress output + Scenario: Standard mode reports task updates from Ninja status lines + Given a minimal Netsuke workspace + And a fake ninja executable that emits task status lines + When netsuke is run with arguments "--accessible false --progress true build" + Then the command should succeed + And stderr should contain "Task 1/2" + And stderr should contain "Task 2/2" + + Scenario: Accessible mode emits textual task updates + Given a minimal Netsuke workspace + And a fake ninja executable that emits task status lines + When netsuke is run with arguments "--accessible true --progress true build" + Then the command should succeed + And stderr should contain "Task 1/2" + And stderr should contain "Task 2/2" + + Scenario: Malformed Ninja status lines are ignored safely + Given a minimal Netsuke workspace + And a fake ninja executable that emits malformed task status lines + When netsuke is run with arguments "--accessible false --progress true build" + Then the command should succeed + And stderr should not contain "Task 1/" + Scenario: Standard mode shows six stage summaries Given a minimal Netsuke workspace When netsuke is run with arguments "--accessible false --progress true manifest -" @@ -25,9 +48,11 @@ Feature: Progress output Scenario: Progress output can be disabled in standard mode Given a minimal Netsuke workspace - When netsuke is run with arguments "--accessible false --progress false manifest -" + And a fake ninja executable that emits task status lines + When netsuke is run with arguments "--accessible false --progress false build" Then the command should succeed And stderr should not contain "Stage 1/6" + And stderr should not contain "Task 1/2" Scenario: Failed runs mark the active stage as failed Given an empty workspace diff --git a/tests/localization_tests.rs b/tests/localization_tests.rs index bc57fec7..d926d99e 100644 --- a/tests/localization_tests.rs +++ b/tests/localization_tests.rs @@ -178,3 +178,35 @@ fn progress_stage_messages_resolve( ); Ok(()) } + +#[rstest] +#[case("en-US", "Task 2/6", "cc -c src/main.c")] +#[case("es-ES", "Tarea 2/6", "cc -c src/main.c")] +fn progress_task_messages_resolve( + #[case] locale: &str, + #[case] expected_label: &str, + #[case] expected_description: &str, +) -> Result<()> { + let _guards = localizer_guards(locale)?; + + let task_label = localization::message(keys::STATUS_TASK_PROGRESS_LABEL) + .with_arg("current", 2) + .with_arg("total", 6) + .to_string(); + let task_update = localization::message(keys::STATUS_TASK_PROGRESS_UPDATE) + .with_arg("task", &task_label) + .with_arg("description", "cc -c src/main.c") + .to_string(); + let normalized_label = normalize_fluent_isolates(&task_label); + let normalized_update = normalize_fluent_isolates(&task_update); + + ensure!( + normalized_label.contains(expected_label), + "expected task label for locale {locale} to contain {expected_label:?}, got: {task_label}" + ); + ensure!( + normalized_update.contains(expected_description), + "expected task update for locale {locale} to contain {expected_description:?}, got: {task_update}" + ); + Ok(()) +} From 18cb9bcc6dba57878a2a4fcfca21456fc93b3497 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 07:49:39 +0000 Subject: [PATCH 03/14] refactor(status,ninja_status,runner/process): consolidate and improve validation of task progress updates - Introduced helper functions for validating numeric strings and task progress consistency. - Encapsulated invalid update checks inside NinjaTaskProgressTracker. - Added monotonicity and active stage checks in IndicatifReporter for safer progress reporting. - Simplified and clarified conditions to reject invalid or inconsistent progress updates. This refactoring improves code clarity, reusability, and correctness by centralizing validation logic and enforcing strict progress monotonocity and state consistency. Co-authored-by: devboxerhub[bot] --- src/runner/process/ninja_status.rs | 17 +++++++++++------ src/status.rs | 23 ++++++++++++++++++----- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/runner/process/ninja_status.rs b/src/runner/process/ninja_status.rs index 2ee25086..f8643174 100644 --- a/src/runner/process/ninja_status.rs +++ b/src/runner/process/ninja_status.rs @@ -42,9 +42,14 @@ pub(super) struct NinjaTaskProgressTracker { } impl NinjaTaskProgressTracker { + /// Check whether an update is invalid (zero counts or current exceeds total). + const fn is_invalid_update(update: &NinjaTaskProgress) -> bool { + update.total() == 0 || update.current() == 0 || update.current() > update.total() + } + /// Accept a new update when it is consistent and monotonic. pub(super) const fn accept(&mut self, update: &NinjaTaskProgress) -> bool { - if update.total() == 0 || update.current() == 0 || update.current() > update.total() { + if Self::is_invalid_update(update) { return false; } match self.total { @@ -58,17 +63,17 @@ impl NinjaTaskProgressTracker { } } +fn is_valid_numeric_string(value: &str) -> bool { + !value.is_empty() && value.bytes().all(|byte| byte.is_ascii_digit()) +} + /// Parse a Ninja status line in the form `[current/total] description`. pub(super) fn parse_ninja_status_line(line: &str) -> Option { let trimmed = line.trim_start(); let rest = trimmed.strip_prefix('[')?; let (current_raw, remaining) = rest.split_once('/')?; let (total_raw, description_raw) = remaining.split_once(']')?; - if current_raw.is_empty() - || total_raw.is_empty() - || !current_raw.bytes().all(|byte| byte.is_ascii_digit()) - || !total_raw.bytes().all(|byte| byte.is_ascii_digit()) - { + if !is_valid_numeric_string(current_raw) || !is_valid_numeric_string(total_raw) { return None; } let current = current_raw.parse::().ok()?; diff --git a/src/status.rs b/src/status.rs index 2642da52..0e034693 100644 --- a/src/status.rs +++ b/src/status.rs @@ -209,6 +209,21 @@ impl IndicatifReporter { } } + const fn is_valid_task_progress(current: u32, total: u32) -> bool { + total != 0 && current != 0 && current <= total + } + + fn is_stage6_active(state: &IndicatifState, stage_index: usize) -> bool { + stage_index < state.bars.len() && state.running_index == Some(stage_index) + } + + const fn is_task_progress_monotonic(state: &IndicatifState, current: u32, total: u32) -> bool { + match state.last_task_progress { + Some((last_current, last_total)) => total == last_total && current >= last_current, + None => true, + } + } + fn set_stage_state( state: &mut IndicatifState, index: usize, @@ -304,7 +319,7 @@ impl StatusReporter for IndicatifReporter { } fn report_task_progress(&self, current: u32, total: u32, description: &str) { - if total == 0 || current == 0 || current > total { + if !Self::is_valid_task_progress(current, total) { return; } let mut state = self @@ -312,12 +327,10 @@ impl StatusReporter for IndicatifReporter { .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); let stage_index = stage6_index(); - if stage_index >= state.bars.len() || state.running_index != Some(stage_index) { + if !Self::is_stage6_active(&state, stage_index) { return; } - if let Some((last_current, last_total)) = state.last_task_progress - && (total != last_total || current < last_current) - { + if !Self::is_task_progress_monotonic(&state, current, total) { return; } state.last_task_progress = Some((current, total)); From aea9c2c92b5662b9e04dad2f08c6294f809e7655 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 08:22:48 +0000 Subject: [PATCH 04/14] feat(runner): add progress_enabled flag to Ninja invocations and refactor process calls - Introduce a `progress_enabled` boolean controlling whether task progress is reported during Ninja execution. - Refactor Ninja invocation APIs to accept typed requests and optional task progress callbacks. - Update build, clean, and graph commands to honor `progress_enabled` for conditional progress reporting. - Simplify IndicatifReporter task progress handling by removing state and improving stage 6 task progress display logic. - Update tests and BDD scenarios to exercise progress enabling/disabling. This change enables disabling progress output while maintaining standard invocation flow, improving user control over task progress reporting. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 9 +- docs/netsuke-design.md | 12 +- docs/users-guide.md | 4 +- src/runner/mod.rs | 158 ++++++++++++------ src/runner/process/mod.rs | 102 +++++------ src/status.rs | 49 ++---- src/status_tests.rs | 50 +++++- tests/features/progress_output.feature | 8 + 8 files changed, 234 insertions(+), 158 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 05f67b31..134df272 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -30,7 +30,7 @@ Observable success: - Running `netsuke --accessible false --progress true build` against a manifest that executes multiple Ninja edges shows Stage 6 task advancement. - Running the same command with stdout redirected still emits textual task - updates suitable for logs and CI. + updates suitable for logs and continuous integration (CI). - Running with `--accessible true` emits text-only task updates (no animated redraw dependency). - `make check-fmt`, `make lint`, and `make test` pass. @@ -46,7 +46,7 @@ Observable success: - Keep CLI help localization wired through Fluent and `src/cli_l10n.rs`. - Maintain public behaviour compatibility for existing commands. - Add unit tests using `rstest`. -- Add behavioural tests using `rstest-bdd` v0.5.0. +- Add behaviour-driven development (BDD) tests using `rstest-bdd` v0.5.0. - Cover happy path, unhappy path, and edge conditions. - Record final design decisions in `docs/netsuke-design.md`. - Update `docs/users-guide.md` with user-visible changes. @@ -125,8 +125,9 @@ Observable success: - Current BDD progress scenarios (`tests/features/progress_output.feature`) use `manifest -`, which does not invoke Ninja. New scenarios must exercise build execution with controlled Ninja output. -- MCP project-memory tools (`qdrant-find` / `qdrant-store`) were not available - in this environment; repository docs were used as the only source of truth. +- Model Context Protocol (MCP) project-memory tools (`qdrant-find` / + `qdrant-store`) were not available in this environment; repository docs were + used as the only source of truth. - Fluent rendering includes bidi isolation markers in localized strings, so brittle literal assertions were replaced with content assertions that strip isolation code points in unit tests. diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index a4835624..b26c842c 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2047,12 +2047,12 @@ During Stage 6, Netsuke parses Ninja status lines of the form `[current/total] ...` and emits localized task progress updates. Parsed updates are monotonic: malformed lines, regressive counts, and total-mismatch lines are ignored to avoid noisy or inconsistent progress state. Task updates fall back -to textual output when stdout is not a TTY, ensuring deterministic CI/log -output; accessible mode always uses textual output. Accessible output remains -text-first and static; it does not animate. The standard reporter is -configurable through OrthoConfig layering via `progress: Option` -(`--progress`, `NETSUKE_PROGRESS`, or config file), with accessible mode taking -precedence when enabled. +to textual output when stdout is not a teletype terminal (TTY), ensuring +deterministic continuous integration (CI) logs; accessible mode always uses +textual output. Accessible output remains text-first and static; it does not +animate. The standard reporter is configurable through OrthoConfig layering via +`progress: Option` (`--progress`, `NETSUKE_PROGRESS`, or config file), +with accessible mode taking precedence when enabled. For screen readers: The following flowchart shows how the build script audits localization keys against English and Spanish Fluent bundles. diff --git a/docs/users-guide.md b/docs/users-guide.md index bc647a75..303cbf9a 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -605,8 +605,8 @@ Build complete. In standard mode, Netsuke uses `indicatif` stage summaries when progress is enabled. During Stage 6, Netsuke parses Ninja status lines (`[current/total]`) and emits task progress updates. When stdout is not a TTY, task progress -automatically falls back to textual updates so CI and redirected logs remain -readable. +automatically falls back to textual updates so continuous integration (CI) and +redirected logs remain readable. Progress output can be controlled via OrthoConfig layering: diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 2a581271..1f77fee8 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -100,11 +100,13 @@ fn make_reporter( prefs: OutputPrefs, stdout_is_tty: bool, ) -> Box { + if !progress_enabled { + return Box::new(SilentReporter); + } let force_text_task_updates = should_force_text_task_updates(mode, stdout_is_tty); - match (mode, progress_enabled) { - (OutputMode::Accessible, _) => Box::new(AccessibleReporter::new(prefs)), - (OutputMode::Standard, true) => Box::new(IndicatifReporter::new(force_text_task_updates)), - (OutputMode::Standard, false) => Box::new(SilentReporter), + match mode { + OutputMode::Accessible => Box::new(AccessibleReporter::new(prefs)), + OutputMode::Standard => Box::new(IndicatifReporter::new(force_text_task_updates)), } } @@ -128,7 +130,7 @@ pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { targets: Vec::new(), })); match command { - Commands::Build(args) => handle_build(cli, &args, reporter.as_ref()), + Commands::Build(args) => handle_build(cli, &args, reporter.as_ref(), progress_enabled), Commands::Manifest { file } => { let ninja = generate_ninja(cli, reporter.as_ref(), None)?; if process::is_stdout_path(file.as_path()) { @@ -140,8 +142,8 @@ pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { reporter.report_complete(keys::STATUS_TOOL_MANIFEST.into()); Ok(()) } - Commands::Clean => handle_clean(cli, reporter.as_ref()), - Commands::Graph => handle_graph(cli, reporter.as_ref()), + Commands::Clean => handle_clean(cli, reporter.as_ref(), progress_enabled), + Commands::Graph => handle_graph(cli, reporter.as_ref(), progress_enabled), } } @@ -160,7 +162,12 @@ pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { /// let args = BuildArgs { emit: None, targets: vec![] }; /// handle_build(&cli, &args, &SilentReporter).unwrap(); /// ``` -fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> Result<()> { +fn handle_build( + cli: &Cli, + args: &BuildArgs, + reporter: &dyn StatusReporter, + progress_enabled: bool, +) -> Result<()> { let ninja = generate_ninja(cli, reporter, Some(keys::STATUS_TOOL_BUILD.into()))?; let targets = BuildTargets::new(&args.targets); @@ -181,25 +188,35 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R } let program = process::resolve_ninja_program(); - let mut on_task_progress = |current: u32, total: u32, description: &str| { - reporter.report_task_progress(current, total, description); - }; - process::run_ninja_invocation( - &process::NinjaInvocation::Build { - program: program.as_path(), - cli, - build_file: build_path.as_ref(), - targets: &targets, - }, - Some(&mut on_task_progress), - ) - .with_context(|| { - format!( - "running {} with build file {}", - program.display(), - build_path.display() + if progress_enabled { + let mut on_task_progress = |current: u32, total: u32, description: &str| { + reporter.report_task_progress(current, total, description); + }; + process::run_ninja_with_status( + process::NinjaBuildRequest { + program: program.as_path(), + cli, + build_file: build_path.as_ref(), + targets: &targets, + }, + &mut on_task_progress, ) - })?; + .with_context(|| { + format!( + "running {} with build file {}", + program.display(), + build_path.display() + ) + })?; + } else { + run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).with_context(|| { + format!( + "running {} with build file {}", + program.display(), + build_path.display() + ) + })?; + } reporter.report_complete(keys::STATUS_TOOL_BUILD.into()); Ok(()) } @@ -213,55 +230,88 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R /// # Errors /// /// Returns an error if manifest generation or Ninja execution fails. +#[derive(Clone, Copy)] +struct NinjaToolSpec<'a> { + name: &'a str, + key: LocalizationKey, +} + fn handle_ninja_tool( cli: &Cli, - tool: &str, - tool_key: LocalizationKey, + tool: NinjaToolSpec<'_>, reporter: &dyn StatusReporter, + progress_enabled: bool, ) -> Result<()> { info!( target: "netsuke::subcommand", - subcommand = tool, + subcommand = tool.name, "Preparing Ninja tool invocation" ); - let ninja = generate_ninja(cli, reporter, Some(tool_key))?; + let ninja = generate_ninja(cli, reporter, Some(tool.key))?; let tmp = process::create_temp_ninja_file(&ninja)?; let build_path = tmp.path(); let program = process::resolve_ninja_program(); - let mut on_task_progress = |current: u32, total: u32, description: &str| { - reporter.report_task_progress(current, total, description); - }; - process::run_ninja_invocation( - &process::NinjaInvocation::Tool { - program: program.as_path(), - cli, - build_file: build_path, - tool, - }, - Some(&mut on_task_progress), - ) - .with_context(|| { - format!( - "running {} -t {} with build file {}", - program.display(), - tool, - build_path.display() + if progress_enabled { + let mut on_task_progress = |current: u32, total: u32, description: &str| { + reporter.report_task_progress(current, total, description); + }; + process::run_ninja_tool_with_status( + process::NinjaToolRequest { + program: program.as_path(), + cli, + build_file: build_path, + tool: tool.name, + }, + &mut on_task_progress, ) - })?; - reporter.report_complete(tool_key); + .with_context(|| { + format!( + "running {} -t {} with build file {}", + program.display(), + tool.name, + build_path.display() + ) + })?; + } else { + run_ninja_tool(program.as_path(), cli, build_path, tool.name).with_context(|| { + format!( + "running {} -t {} with build file {}", + program.display(), + tool.name, + build_path.display() + ) + })?; + } + reporter.report_complete(tool.key); Ok(()) } /// Remove build artefacts by invoking `ninja -t clean`. -fn handle_clean(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { - handle_ninja_tool(cli, "clean", keys::STATUS_TOOL_CLEAN.into(), reporter) +fn handle_clean(cli: &Cli, reporter: &dyn StatusReporter, progress_enabled: bool) -> Result<()> { + handle_ninja_tool( + cli, + NinjaToolSpec { + name: "clean", + key: keys::STATUS_TOOL_CLEAN.into(), + }, + reporter, + progress_enabled, + ) } /// Display build dependency graph by invoking `ninja -t graph`. -fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { - handle_ninja_tool(cli, "graph", keys::STATUS_TOOL_GRAPH.into(), reporter) +fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter, progress_enabled: bool) -> Result<()> { + handle_ninja_tool( + cli, + NinjaToolSpec { + name: "graph", + key: keys::STATUS_TOOL_GRAPH.into(), + }, + reporter, + progress_enabled, + ) } /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 68d15d84..7aca4e31 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -28,24 +28,6 @@ use streaming::{ForwardStats, forward_child_output, forward_child_output_with_ni type StatusObserver<'a> = &'a mut dyn FnMut(u32, u32, &str); -/// Prepared Ninja invocation variant. -pub(crate) enum NinjaInvocation<'a> { - /// Standard build invocation. - Build { - program: &'a Path, - cli: &'a Cli, - build_file: &'a Path, - targets: &'a BuildTargets<'a>, - }, - /// Ninja tool invocation (`ninja -t `). - Tool { - program: &'a Path, - cli: &'a Cli, - build_file: &'a Path, - tool: &'a str, - }, -} - // Public helpers for doctests only. This exposes internal helpers as a stable // testing surface without exporting them in release builds. #[cfg(doctest)] @@ -163,6 +145,22 @@ fn run_command_and_stream( check_exit_status(status) } +#[derive(Clone, Copy)] +pub(crate) struct NinjaBuildRequest<'a> { + pub(crate) program: &'a Path, + pub(crate) cli: &'a Cli, + pub(crate) build_file: &'a Path, + pub(crate) targets: &'a BuildTargets<'a>, +} + +#[derive(Clone, Copy)] +pub(crate) struct NinjaToolRequest<'a> { + pub(crate) program: &'a Path, + pub(crate) cli: &'a Cli, + pub(crate) build_file: &'a Path, + pub(crate) tool: &'a str, +} + /// Invoke the Ninja executable with the provided CLI settings. /// /// The function forwards the job count and working directory to Ninja, @@ -179,13 +177,13 @@ pub fn run_ninja( build_file: &Path, targets: &BuildTargets<'_>, ) -> io::Result<()> { - let invocation = NinjaInvocation::Build { + let request = NinjaBuildRequest { program, cli, build_file, targets, }; - run_ninja_invocation(&invocation, None) + run_ninja_build_internal(request, None) } /// Invoke a Ninja tool (e.g., `ninja -t clean`) with the provided CLI settings. @@ -199,42 +197,47 @@ pub fn run_ninja( /// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard /// streams are unavailable, or when Ninja reports a non-zero exit status. pub fn run_ninja_tool(program: &Path, cli: &Cli, build_file: &Path, tool: &str) -> io::Result<()> { - let invocation = NinjaInvocation::Tool { + let request = NinjaToolRequest { program, cli, build_file, tool, }; - run_ninja_invocation(&invocation, None) + run_ninja_tool_internal(request, None) } -/// Invoke Ninja and stream parsed task updates from status lines. -pub(crate) fn run_ninja_invocation( - invocation: &NinjaInvocation<'_>, +fn run_ninja_build_internal( + request: NinjaBuildRequest<'_>, status_observer: Option>, ) -> io::Result<()> { - match invocation { - NinjaInvocation::Build { - program, - cli, - build_file, - targets, - } => { - let mut cmd = Command::new(program); - configure_ninja_build_command(&mut cmd, cli, build_file, targets)?; - run_command_and_stream(cmd, status_observer) - } - NinjaInvocation::Tool { - program, - cli, - build_file, - tool, - } => { - let mut cmd = Command::new(program); - configure_ninja_tool_command(&mut cmd, cli, build_file, tool)?; - run_command_and_stream(cmd, status_observer) - } - } + let mut cmd = Command::new(request.program); + configure_ninja_build_command(&mut cmd, request.cli, request.build_file, request.targets)?; + run_command_and_stream(cmd, status_observer) +} + +fn run_ninja_tool_internal( + request: NinjaToolRequest<'_>, + status_observer: Option>, +) -> io::Result<()> { + let mut cmd = Command::new(request.program); + configure_ninja_tool_command(&mut cmd, request.cli, request.build_file, request.tool)?; + run_command_and_stream(cmd, status_observer) +} + +/// Invoke `ninja` build and stream parsed task updates from status lines. +pub(crate) fn run_ninja_with_status( + request: NinjaBuildRequest<'_>, + status_observer: StatusObserver<'_>, +) -> io::Result<()> { + run_ninja_build_internal(request, Some(status_observer)) +} + +/// Invoke `ninja -t` and stream parsed task updates from status lines. +pub(crate) fn run_ninja_tool_with_status( + request: NinjaToolRequest<'_>, + status_observer: StatusObserver<'_>, +) -> io::Result<()> { + run_ninja_tool_internal(request, Some(status_observer)) } fn handle_forwarding_stats(stats: ForwardStats, stream_name: &str) { @@ -266,8 +269,9 @@ fn spawn_and_stream_output( }; let err_handle = thread::spawn(move || { - let mut lock = io::stderr().lock(); - forward_child_output(BufReader::new(stderr), &mut lock, "stderr") + // Avoid holding a long-lived stderr lock while stdout parsing may emit + // task updates to stderr from the observer callback. + forward_child_output(BufReader::new(stderr), io::stderr(), "stderr") }); let stdout_stats = { diff --git a/src/status.rs b/src/status.rs index 0e034693..b2d3dcdd 100644 --- a/src/status.rs +++ b/src/status.rs @@ -79,15 +79,11 @@ fn stage_summary( .to_string() } -fn task_progress_label(current: u32, total: u32) -> String { - localization::message(keys::STATUS_TASK_PROGRESS_LABEL) +fn task_progress_update(current: u32, total: u32, description: &str) -> String { + let task = localization::message(keys::STATUS_TASK_PROGRESS_LABEL) .with_arg("current", current.to_string()) .with_arg("total", total.to_string()) - .to_string() -} - -fn task_progress_update(current: u32, total: u32, description: &str) -> String { - let task = task_progress_label(current, total); + .to_string(); if description.is_empty() { return task; } @@ -161,9 +157,10 @@ struct IndicatifState { completed: bool, is_hidden: bool, force_text_task_updates: bool, - last_task_progress: Option<(u32, u32)>, } +const STAGE6_INDEX: usize = (PipelineStage::NinjaSynthesisAndExecution as usize) - 1; + /// Standard reporter backed by `indicatif::MultiProgress`. pub struct IndicatifReporter { state: Mutex, @@ -204,24 +201,12 @@ impl IndicatifReporter { running_index: None, completed: false, force_text_task_updates, - last_task_progress: None, }), } } - const fn is_valid_task_progress(current: u32, total: u32) -> bool { - total != 0 && current != 0 && current <= total - } - - fn is_stage6_active(state: &IndicatifState, stage_index: usize) -> bool { - stage_index < state.bars.len() && state.running_index == Some(stage_index) - } - - const fn is_task_progress_monotonic(state: &IndicatifState, current: u32, total: u32) -> bool { - match state.last_task_progress { - Some((last_current, last_total)) => total == last_total && current >= last_current, - None => true, - } + fn is_stage6_active(state: &IndicatifState) -> bool { + state.running_index == Some(STAGE6_INDEX) && STAGE6_INDEX < state.bars.len() } fn set_stage_state( @@ -312,28 +297,18 @@ impl StatusReporter for IndicatifReporter { LocalizationKey::new(keys::STATUS_STATE_RUNNING), false, ); - if index != stage6_index() { - state.last_task_progress = None; - } state.running_index = Some(index); } fn report_task_progress(&self, current: u32, total: u32, description: &str) { - if !Self::is_valid_task_progress(current, total) { - return; - } - let mut state = self + let state = self .state .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - let stage_index = stage6_index(); - if !Self::is_stage6_active(&state, stage_index) { + if !Self::is_stage6_active(&state) { return; } - if !Self::is_task_progress_monotonic(&state, current, total) { - return; - } - state.last_task_progress = Some((current, total)); + let stage_index = STAGE6_INDEX; let task = task_progress_update(current, total, description); if state.is_hidden || state.force_text_task_updates { drop(writeln!(io::stderr(), "{task}")); @@ -382,10 +357,6 @@ impl StatusReporter for IndicatifReporter { } } -const fn stage6_index() -> usize { - 5 -} - #[cfg(test)] #[path = "status_tests.rs"] mod tests; diff --git a/src/status_tests.rs b/src/status_tests.rs index f961690b..0214ee4f 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -53,19 +53,61 @@ fn task_progress_update_formats_expected_text( } #[test] -fn indicatif_reporter_rejects_regressive_task_updates() { +fn indicatif_reporter_ignores_task_updates_when_stage6_is_not_running() { let reporter = IndicatifReporter::new(true); + reporter.report_task_progress(1, 2, "cc -c src/a.c"); + + let state = reporter + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let stage6_message = state + .bars + .get(STAGE6_INDEX) + .expect("stage 6 progress bar should exist") + .message() + .to_string(); + assert!(!strip_isolates(&stage6_message).contains("Task 1/2")); +} + +#[test] +fn indicatif_reporter_sets_stage6_bar_message_for_non_text_updates() { + let reporter = IndicatifReporter::new(false); + { + let mut state = reporter + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + state.is_hidden = false; + } reporter.report_stage( PipelineStage::NinjaSynthesisAndExecution.index(), PIPELINE_STAGE_TOTAL, "Executing Build", ); - reporter.report_task_progress(2, 3, "cc -c src/b.c"); - reporter.report_task_progress(1, 3, "stale"); + reporter.report_task_progress(1, 2, "cc -c src/a.c"); let state = reporter .state .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); - assert_eq!(state.last_task_progress, Some((2, 3))); + let stage6_message = state + .bars + .get(STAGE6_INDEX) + .expect("stage 6 progress bar should exist") + .message() + .to_string(); + let task = task_progress_update(1, 2, "cc -c src/a.c"); + let state_label = localization::message(keys::STATUS_STATE_RUNNING).to_string(); + let stage_line = stage_label( + PipelineStage::NinjaSynthesisAndExecution.index(), + PIPELINE_STAGE_TOTAL, + "Executing Build", + ); + let expected = localization::message(keys::STATUS_STAGE_SUMMARY_WITH_TASK) + .with_arg("state", state_label) + .with_arg("label", stage_line) + .with_arg("task_progress", &task) + .to_string(); + assert_eq!(strip_isolates(&stage6_message), strip_isolates(&expected)); } diff --git a/tests/features/progress_output.feature b/tests/features/progress_output.feature index 34cdfb56..9b619214 100644 --- a/tests/features/progress_output.feature +++ b/tests/features/progress_output.feature @@ -54,6 +54,14 @@ Feature: Progress output And stderr should not contain "Stage 1/6" And stderr should not contain "Task 1/2" + Scenario: Progress output can be disabled in accessible mode + Given a minimal Netsuke workspace + And a fake ninja executable that emits task status lines + When netsuke is run with arguments "--accessible true --progress false build" + Then the command should succeed + And stderr should not contain "Stage 1/6" + And stderr should not contain "Task 1/2" + Scenario: Failed runs mark the active stage as failed Given an empty workspace When netsuke is run with arguments "--accessible false --progress true" From e7425703d6677c94f84f3474c3c3ab0c25689a28 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 08:51:42 +0000 Subject: [PATCH 05/14] feat(runner/status): parse Ninja status lines for Stage 6 task progress - Implement parsing of Ninja status lines like `[current/total]` to drive task-level progress updates during build execution (Stage 6). - Add option to force textual task progress output fallback when stdout is non-TTY or accessible mode is active. - Update IndicatifReporter with ability to control forced text fallback for task progress. - Enhance tests for task progress reporting with new fixtures and verify behavior in different modes. - Document the parsing and progress reporting approach in execplans and user guide. - Improve concurrency in process streaming to avoid blocking on stderr while reporting task progress. This feature fulfills roadmap item 3.9.2 by enabling deterministic task progress reporting from Ninja output and ensures accessibility and CI-friendly textual fallbacks. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 8 +- docs/users-guide.md | 2 +- src/runner/mod.rs | 4 +- src/runner/process/mod.rs | 7 +- src/runner/process/streaming.rs | 6 +- src/runner/tests.rs | 2 +- src/status.rs | 14 ++- src/status_tests.rs | 92 ++++++++++--------- tests/bdd/steps/progress_output.rs | 2 + 9 files changed, 81 insertions(+), 56 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 134df272..15d36eb1 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -12,7 +12,7 @@ No `PLANS.md` file exists in this repository. Netsuke currently reports six pipeline stages, but Stage 6 does not yet derive task-level progress from Ninja execution lines. Roadmap item 3.9.2 requires -Ninja status-line parsing so Netsuke can present task progress during build +Ninja status-line parsing, so Netsuke can present task progress during build execution. The same milestone also requires deterministic textual fallback updates when stdout is not a TTY and whenever accessible mode is active. @@ -140,19 +140,19 @@ Observable success: - Decision: parse bracketed Ninja status tokens of the form `[current/total]` at line start for Stage 6 progress, treating malformed lines as non-events. Rationale: this is the documented, stable surface from Ninja output and keeps - parser complexity low. Date/Author: 2026-02-22 / Codex + parser complexity low. Date/Author: 2026-02-22 / Codex. - Decision: use the existing OrthoConfig-backed `progress` setting as the sole control for stage and task progress output, and update localized help text to reflect the expanded behaviour. Rationale: avoids unnecessary configuration sprawl while satisfying layered config and localization requirements. - Date/Author: 2026-02-22 / Codex + Date/Author: 2026-02-22 / Codex. - Decision: emit explicit textual task updates whenever stdout is non-TTY or accessible mode is active, regardless of `indicatif` draw target details. Rationale: roadmap requirement is about deterministic accessibility/log readability and must not depend solely on terminal redraw support. - Date/Author: 2026-02-22 / Codex + Date/Author: 2026-02-22 / Codex. ## Outcomes & Retrospective diff --git a/docs/users-guide.md b/docs/users-guide.md index 303cbf9a..4b84b592 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -605,7 +605,7 @@ Build complete. In standard mode, Netsuke uses `indicatif` stage summaries when progress is enabled. During Stage 6, Netsuke parses Ninja status lines (`[current/total]`) and emits task progress updates. When stdout is not a TTY, task progress -automatically falls back to textual updates so continuous integration (CI) and +automatically falls back to textual updates, so continuous integration (CI) and redirected logs remain readable. Progress output can be controlled via OrthoConfig layering: diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 1f77fee8..e872d15a 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -106,7 +106,9 @@ fn make_reporter( let force_text_task_updates = should_force_text_task_updates(mode, stdout_is_tty); match mode { OutputMode::Accessible => Box::new(AccessibleReporter::new(prefs)), - OutputMode::Standard => Box::new(IndicatifReporter::new(force_text_task_updates)), + OutputMode::Standard => Box::new(IndicatifReporter::with_force_text_task_updates( + force_text_task_updates, + )), } } diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 7aca4e31..01bad8b6 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -145,6 +145,7 @@ fn run_command_and_stream( check_exit_status(status) } +/// Borrowed parameter bundle for `ninja` build execution helpers. #[derive(Clone, Copy)] pub(crate) struct NinjaBuildRequest<'a> { pub(crate) program: &'a Path, @@ -153,6 +154,7 @@ pub(crate) struct NinjaBuildRequest<'a> { pub(crate) targets: &'a BuildTargets<'a>, } +/// Borrowed parameter bundle for `ninja -t` tool execution helpers. #[derive(Clone, Copy)] pub(crate) struct NinjaToolRequest<'a> { pub(crate) program: &'a Path, @@ -269,8 +271,9 @@ fn spawn_and_stream_output( }; let err_handle = thread::spawn(move || { - // Avoid holding a long-lived stderr lock while stdout parsing may emit - // task updates to stderr from the observer callback. + // Avoid a long-lived stderr lock: status observers invoked while + // draining stdout may emit task updates to stderr, and that path must + // not block behind stderr forwarding. forward_child_output(BufReader::new(stderr), io::stderr(), "stderr") }); diff --git a/src/runner/process/streaming.rs b/src/runner/process/streaming.rs index e8584d63..fb9362a5 100644 --- a/src/runner/process/streaming.rs +++ b/src/runner/process/streaming.rs @@ -90,9 +90,9 @@ where self.finish_line(); return Ok(0); } - let Some(slice) = buf.get(..count) else { - return Ok(0); - }; + let slice = buf + .get(..count) + .ok_or_else(|| io::Error::other("reader returned out-of-range byte count"))?; self.consume_bytes(slice); Ok(count) } diff --git a/src/runner/tests.rs b/src/runner/tests.rs index 2cea902e..4e2c3382 100644 --- a/src/runner/tests.rs +++ b/src/runner/tests.rs @@ -1,4 +1,4 @@ -//! Unit tests for the runner module's path resolution helpers. +//! Unit tests for runner path resolution, predicate helpers, and core helpers. use super::*; use rstest::rstest; diff --git a/src/status.rs b/src/status.rs index b2d3dcdd..d9165c41 100644 --- a/src/status.rs +++ b/src/status.rs @@ -98,6 +98,9 @@ pub trait StatusReporter { /// Emit a stage update. fn report_stage(&self, current: StageNumber, total: StageNumber, description: &str); /// Emit task progress for Stage 6 execution. + /// + /// Callers must pass validated, monotonic task updates for a single Ninja + /// execution stream (`total > 0`, `current > 0`, `current <= total`). fn report_task_progress(&self, _current: u32, _total: u32, _description: &str) {} /// Emit a final completion message. fn report_complete(&self, tool_key: LocalizationKey); @@ -205,6 +208,15 @@ impl IndicatifReporter { } } + /// Build a reporter while explicitly controlling task-update text fallback. + /// + /// This forwards to [`Self::new`] and sets + /// `IndicatifState::force_text_task_updates` with the same value. + #[must_use] + pub fn with_force_text_task_updates(force_text_task_updates: bool) -> Self { + Self::new(force_text_task_updates) + } + fn is_stage6_active(state: &IndicatifState) -> bool { state.running_index == Some(STAGE6_INDEX) && STAGE6_INDEX < state.bars.len() } @@ -237,7 +249,7 @@ impl IndicatifReporter { impl Default for IndicatifReporter { fn default() -> Self { - Self::new(false) + Self::with_force_text_task_updates(false) } } diff --git a/src/status_tests.rs b/src/status_tests.rs index 0214ee4f..56c7662c 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -1,7 +1,7 @@ //! Tests for status stage modelling and index conversions. use super::*; -use rstest::rstest; +use rstest::{fixture, rstest}; fn strip_isolates(value: &str) -> String { value @@ -10,6 +10,42 @@ fn strip_isolates(value: &str) -> String { .collect() } +fn stage6_message(reporter: &IndicatifReporter) -> String { + let state = reporter + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + state + .bars + .get(STAGE6_INDEX) + .expect("stage 6 progress bar should exist") + .message() + .to_string() +} + +#[fixture] +fn force_text_reporter() -> IndicatifReporter { + IndicatifReporter::with_force_text_task_updates(true) +} + +#[fixture] +fn running_stage6_reporter() -> IndicatifReporter { + let reporter = IndicatifReporter::with_force_text_task_updates(false); + { + let mut state = reporter + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + state.is_hidden = false; + } + reporter.report_stage( + PipelineStage::NinjaSynthesisAndExecution.index(), + PIPELINE_STAGE_TOTAL, + "Executing Build", + ); + reporter +} + #[rstest] #[case(PipelineStage::ManifestIngestion, 1)] #[case(PipelineStage::InitialYamlParsing, 2)] @@ -52,51 +88,21 @@ fn task_progress_update_formats_expected_text( assert_eq!(strip_isolates(&rendered), expected); } -#[test] -fn indicatif_reporter_ignores_task_updates_when_stage6_is_not_running() { - let reporter = IndicatifReporter::new(true); - reporter.report_task_progress(1, 2, "cc -c src/a.c"); - - let state = reporter - .state - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let stage6_message = state - .bars - .get(STAGE6_INDEX) - .expect("stage 6 progress bar should exist") - .message() - .to_string(); +#[rstest] +fn indicatif_reporter_ignores_task_updates_when_stage6_is_not_running( + force_text_reporter: IndicatifReporter, +) { + force_text_reporter.report_task_progress(1, 2, "cc -c src/a.c"); + let stage6_message = stage6_message(&force_text_reporter); assert!(!strip_isolates(&stage6_message).contains("Task 1/2")); } -#[test] -fn indicatif_reporter_sets_stage6_bar_message_for_non_text_updates() { - let reporter = IndicatifReporter::new(false); - { - let mut state = reporter - .state - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - state.is_hidden = false; - } - reporter.report_stage( - PipelineStage::NinjaSynthesisAndExecution.index(), - PIPELINE_STAGE_TOTAL, - "Executing Build", - ); - reporter.report_task_progress(1, 2, "cc -c src/a.c"); - - let state = reporter - .state - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let stage6_message = state - .bars - .get(STAGE6_INDEX) - .expect("stage 6 progress bar should exist") - .message() - .to_string(); +#[rstest] +fn indicatif_reporter_sets_stage6_bar_message_for_non_text_updates( + running_stage6_reporter: IndicatifReporter, +) { + running_stage6_reporter.report_task_progress(1, 2, "cc -c src/a.c"); + let stage6_message = stage6_message(&running_stage6_reporter); let task = task_progress_update(1, 2, "cc -c src/a.c"); let state_label = localization::message(keys::STATUS_STATE_RUNNING).to_string(); let stage_line = stage_label( diff --git a/tests/bdd/steps/progress_output.rs b/tests/bdd/steps/progress_output.rs index 026dab21..565563c5 100644 --- a/tests/bdd/steps/progress_output.rs +++ b/tests/bdd/steps/progress_output.rs @@ -69,6 +69,8 @@ fn install_fake_ninja(world: &TestWorld, lines: &[&str]) -> Result<()> { make_script_executable(&script_path)?; let env = SystemEnv::new(); + // Drop any existing guard first so its environment override is restored + // before installing a replacement for this scenario. world.ninja_env_guard.borrow_mut().take(); *world.ninja_env_guard.borrow_mut() = Some(override_ninja_env(&env, &script_path)); Ok(()) From dd8fbd53da56dc6764a7cdedc1ece700ce38905f Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 09:02:21 +0000 Subject: [PATCH 06/14] refactor(runner): replace handle_clean and handle_graph with handle_ninja_tool calls Refactored the Commands::Clean and Commands::Graph branches to use a unified handle_ninja_tool function with NinjaToolSpec parameters. Removed the now redundant handle_clean and handle_graph functions to simplify code and reduce duplication. Co-authored-by: devboxerhub[bot] --- src/runner/mod.rs | 46 ++++++++++++++++++--------------------------- src/status_tests.rs | 2 +- 2 files changed, 19 insertions(+), 29 deletions(-) diff --git a/src/runner/mod.rs b/src/runner/mod.rs index e872d15a..458b0e09 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -144,8 +144,24 @@ pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { reporter.report_complete(keys::STATUS_TOOL_MANIFEST.into()); Ok(()) } - Commands::Clean => handle_clean(cli, reporter.as_ref(), progress_enabled), - Commands::Graph => handle_graph(cli, reporter.as_ref(), progress_enabled), + Commands::Clean => handle_ninja_tool( + cli, + NinjaToolSpec { + name: "clean", + key: keys::STATUS_TOOL_CLEAN.into(), + }, + reporter.as_ref(), + progress_enabled, + ), + Commands::Graph => handle_ninja_tool( + cli, + NinjaToolSpec { + name: "graph", + key: keys::STATUS_TOOL_GRAPH.into(), + }, + reporter.as_ref(), + progress_enabled, + ), } } @@ -290,32 +306,6 @@ fn handle_ninja_tool( Ok(()) } -/// Remove build artefacts by invoking `ninja -t clean`. -fn handle_clean(cli: &Cli, reporter: &dyn StatusReporter, progress_enabled: bool) -> Result<()> { - handle_ninja_tool( - cli, - NinjaToolSpec { - name: "clean", - key: keys::STATUS_TOOL_CLEAN.into(), - }, - reporter, - progress_enabled, - ) -} - -/// Display build dependency graph by invoking `ninja -t graph`. -fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter, progress_enabled: bool) -> Result<()> { - handle_ninja_tool( - cli, - NinjaToolSpec { - name: "graph", - key: keys::STATUS_TOOL_GRAPH.into(), - }, - reporter, - progress_enabled, - ) -} - /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. /// /// Reports manifest and graph/synthesis pipeline stages via the provided diff --git a/src/status_tests.rs b/src/status_tests.rs index 56c7662c..6b9ac363 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -20,7 +20,7 @@ fn stage6_message(reporter: &IndicatifReporter) -> String { .get(STAGE6_INDEX) .expect("stage 6 progress bar should exist") .message() - .to_string() + .clone() } #[fixture] From bc4175c670341bc500b39046d048947074c4d2b8 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 10:53:37 +0000 Subject: [PATCH 07/14] feat(runner): drain stdout on main thread to stabilize status output ordering Ensure that when a status observer is present, stdout is drained on the main thread. This change keeps forwarding and callback-driven status updates in a stable order, preventing regression of output timing and interleaving issues. Additional minor docs updates clarify the term "teletype terminal (TTY)" replacing previous shorthand and showcase progress control via OrthoConfig layering. This improves task progress reporting and output consistency. Co-authored-by: devboxerhub[bot] --- ...3-9-2-parse-ninja-status-lines-to-drive-task-progress.md | 3 ++- docs/users-guide.md | 6 +++--- src/runner/mod.rs | 3 ++- src/runner/process/mod.rs | 3 +++ src/status_tests.rs | 1 - 5 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 15d36eb1..2d62dee3 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -14,7 +14,8 @@ Netsuke currently reports six pipeline stages, but Stage 6 does not yet derive task-level progress from Ninja execution lines. Roadmap item 3.9.2 requires Ninja status-line parsing, so Netsuke can present task progress during build execution. The same milestone also requires deterministic textual fallback -updates when stdout is not a TTY and whenever accessible mode is active. +updates when stdout is not a teletype terminal (TTY) and whenever accessible +mode is active. After this change: diff --git a/docs/users-guide.md b/docs/users-guide.md index 4b84b592..2a2ba5c8 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -604,9 +604,9 @@ Build complete. In standard mode, Netsuke uses `indicatif` stage summaries when progress is enabled. During Stage 6, Netsuke parses Ninja status lines (`[current/total]`) -and emits task progress updates. When stdout is not a TTY, task progress -automatically falls back to textual updates, so continuous integration (CI) and -redirected logs remain readable. +and emits task progress updates. When stdout is not a teletype terminal (TTY), +task progress automatically falls back to textual updates, so continuous +integration (CI) and redirected logs remain readable. Progress output can be controlled via OrthoConfig layering: diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 458b0e09..51de5098 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -178,7 +178,8 @@ pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { /// use netsuke::status::SilentReporter; /// let cli = Cli::default(); /// let args = BuildArgs { emit: None, targets: vec![] }; -/// handle_build(&cli, &args, &SilentReporter).unwrap(); +/// let progress_enabled = true; +/// handle_build(&cli, &args, &SilentReporter, progress_enabled).unwrap(); /// ``` fn handle_build( cli: &Cli, diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 01bad8b6..5247f85d 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -277,6 +277,9 @@ fn spawn_and_stream_output( forward_child_output(BufReader::new(stderr), io::stderr(), "stderr") }); + // Intentionally drain stdout on the main thread when `status_observer` is + // present so forwarding and callback-driven status updates keep a stable + // ordering; moving this elsewhere can regress output timing/interleaving. let stdout_stats = { let mut lock = io::stdout().lock(); match status_observer { diff --git a/src/status_tests.rs b/src/status_tests.rs index 6b9ac363..2db80c5f 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -20,7 +20,6 @@ fn stage6_message(reporter: &IndicatifReporter) -> String { .get(STAGE6_INDEX) .expect("stage 6 progress bar should exist") .message() - .clone() } #[fixture] From 6ec23a5cdedbff548703a0fbd57f032a21eb6a8f Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 11:23:04 +0000 Subject: [PATCH 08/14] refactor(runner): simplify Ninja invocation error context with shared closures Switched to using reusable closure functions for building error contexts when calling Ninja and Ninja tool commands in `handle_build` and `handle_ninja_tool`. This improves clarity and removes duplicated closure code for context formatting. Additionally, relocated the `NinjaToolSpec` struct for clarity and added focused comments for the task-progress callback type alias to document its contract. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 7 ++- src/runner/mod.rs | 62 ++++++++----------- src/runner/process/mod.rs | 6 ++ 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 2d62dee3..c7b1951e 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -44,7 +44,8 @@ Observable success: - Keep accessible mode text-first and non-animated. - Use OrthoConfig layering for user control; do not introduce ad-hoc config reads. Existing `progress: Option` remains the controlling switch. -- Keep CLI help localization wired through Fluent and `src/cli_l10n.rs`. +- Keep command-line interface (CLI) help localization wired through Fluent and + `src/cli_l10n.rs`. - Maintain public behaviour compatibility for existing commands. - Add unit tests using `rstest`. - Add behaviour-driven development (BDD) tests using `rstest-bdd` v0.5.0. @@ -359,7 +360,7 @@ make lint 2>&1 | tee /tmp/3-9-2-lint.log make test 2>&1 | tee /tmp/3-9-2-test.log ``` -1. Run documentation gates after doc edits: +8. Run documentation gates after doc edits: ```sh set -o pipefail @@ -395,7 +396,7 @@ Acceptance requires all of the following: `progress = false` temporarily and continue with non-progress execution while preserving deterministic command output. -## Artifacts and notes +## Artefacts and notes Capture concise evidence for review: diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 51de5098..f3623ed5 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -207,6 +207,13 @@ fn handle_build( } let program = process::resolve_ninja_program(); + let ctx = || { + format!( + "running {} with build file {}", + program.display(), + build_path.display() + ) + }; if progress_enabled { let mut on_task_progress = |current: u32, total: u32, description: &str| { reporter.report_task_progress(current, total, description); @@ -220,26 +227,21 @@ fn handle_build( }, &mut on_task_progress, ) - .with_context(|| { - format!( - "running {} with build file {}", - program.display(), - build_path.display() - ) - })?; + .with_context(ctx)?; } else { - run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).with_context(|| { - format!( - "running {} with build file {}", - program.display(), - build_path.display() - ) - })?; + run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).with_context(ctx)?; } reporter.report_complete(keys::STATUS_TOOL_BUILD.into()); Ok(()) } +/// Specification for a Ninja tool invocation: name and localization key. +#[derive(Clone, Copy)] +struct NinjaToolSpec<'a> { + name: &'a str, + key: LocalizationKey, +} + /// Execute a Ninja tool (e.g., `ninja -t clean`) using a temporary build file. /// /// Generates the Ninja manifest to a temporary file, then invokes Ninja with @@ -249,12 +251,6 @@ fn handle_build( /// # Errors /// /// Returns an error if manifest generation or Ninja execution fails. -#[derive(Clone, Copy)] -struct NinjaToolSpec<'a> { - name: &'a str, - key: LocalizationKey, -} - fn handle_ninja_tool( cli: &Cli, tool: NinjaToolSpec<'_>, @@ -272,6 +268,14 @@ fn handle_ninja_tool( let build_path = tmp.path(); let program = process::resolve_ninja_program(); + let ctx = || { + format!( + "running {} -t {} with build file {}", + program.display(), + tool.name, + build_path.display() + ) + }; if progress_enabled { let mut on_task_progress = |current: u32, total: u32, description: &str| { reporter.report_task_progress(current, total, description); @@ -285,23 +289,9 @@ fn handle_ninja_tool( }, &mut on_task_progress, ) - .with_context(|| { - format!( - "running {} -t {} with build file {}", - program.display(), - tool.name, - build_path.display() - ) - })?; + .with_context(ctx)?; } else { - run_ninja_tool(program.as_path(), cli, build_path, tool.name).with_context(|| { - format!( - "running {} -t {} with build file {}", - program.display(), - tool.name, - build_path.display() - ) - })?; + run_ninja_tool(program.as_path(), cli, build_path, tool.name).with_context(ctx)?; } reporter.report_complete(tool.key); Ok(()) diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 5247f85d..723a967a 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -26,6 +26,12 @@ pub use paths::*; use redaction::{CommandArg, redact_sensitive_args}; use streaming::{ForwardStats, forward_child_output, forward_child_output_with_ninja_status}; +/// Callback contract for task-progress updates from parsed Ninja status lines. +/// +/// Accepts `(current, total, description)` where `current` and `total` are +/// progress counters and `description` is a human-readable status string. +/// This alias appears in `pub(crate)` function signatures and borrows a mutable +/// callback for the call duration, so callers can retain state across updates. type StatusObserver<'a> = &'a mut dyn FnMut(u32, u32, &str); // Public helpers for doctests only. This exposes internal helpers as a stable From 8d33dedba9960210a1197d0540c2f67cad80984a Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 12:23:46 +0000 Subject: [PATCH 09/14] refactor(runner): refactor task progress reporting to use callback helper - Extracted task progress closure in runner to a reusable function - Used the new callback helper in both build and tool handlers - Added detailed error doc comments to ninja process streaming functions - Improved markdown formatting and clarity in related docs Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 26 +++++++++---------- src/runner/mod.rs | 14 +++++----- src/runner/process/mod.rs | 10 +++++++ 3 files changed, 31 insertions(+), 19 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index c7b1951e..ce6f48c9 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -342,7 +342,7 @@ Expected new internal interfaces (names may vary, behaviour is required): ## Concrete steps -Run all commands from `/home/user/project`. +Run all commands from the repository root. 1. Implement parser module and parser unit tests. 2. Wire parser into process forwarding path with observer callbacks. @@ -353,21 +353,21 @@ Run all commands from `/home/user/project`. `docs/roadmap.md`. 7. Run quality gates with `tee` logs: -```sh -set -o pipefail -make check-fmt 2>&1 | tee /tmp/3-9-2-check-fmt.log -make lint 2>&1 | tee /tmp/3-9-2-lint.log -make test 2>&1 | tee /tmp/3-9-2-test.log -``` + ```sh + set -o pipefail + make check-fmt 2>&1 | tee /tmp/3-9-2-check-fmt.log + make lint 2>&1 | tee /tmp/3-9-2-lint.log + make test 2>&1 | tee /tmp/3-9-2-test.log + ``` 8. Run documentation gates after doc edits: -```sh -set -o pipefail -make fmt 2>&1 | tee /tmp/3-9-2-fmt.log -make markdownlint 2>&1 | tee /tmp/3-9-2-markdownlint.log -make nixie 2>&1 | tee /tmp/3-9-2-nixie.log -``` + ```sh + set -o pipefail + make fmt 2>&1 | tee /tmp/3-9-2-fmt.log + make markdownlint 2>&1 | tee /tmp/3-9-2-markdownlint.log + make nixie 2>&1 | tee /tmp/3-9-2-nixie.log + ``` ## Validation and acceptance diff --git a/src/runner/mod.rs b/src/runner/mod.rs index f3623ed5..e15e3c90 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -165,6 +165,12 @@ pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { } } +fn on_task_progress_callback(reporter: &dyn StatusReporter) -> impl FnMut(u32, u32, &str) + '_ { + move |current: u32, total: u32, description: &str| { + reporter.report_task_progress(current, total, description); + } +} + /// Resolve the manifest, generate the Ninja file and invoke the build. /// /// # Errors @@ -215,9 +221,7 @@ fn handle_build( ) }; if progress_enabled { - let mut on_task_progress = |current: u32, total: u32, description: &str| { - reporter.report_task_progress(current, total, description); - }; + let mut on_task_progress = on_task_progress_callback(reporter); process::run_ninja_with_status( process::NinjaBuildRequest { program: program.as_path(), @@ -277,9 +281,7 @@ fn handle_ninja_tool( ) }; if progress_enabled { - let mut on_task_progress = |current: u32, total: u32, description: &str| { - reporter.report_task_progress(current, total, description); - }; + let mut on_task_progress = on_task_progress_callback(reporter); process::run_ninja_tool_with_status( process::NinjaToolRequest { program: program.as_path(), diff --git a/src/runner/process/mod.rs b/src/runner/process/mod.rs index 723a967a..c5dcb23c 100644 --- a/src/runner/process/mod.rs +++ b/src/runner/process/mod.rs @@ -233,6 +233,11 @@ fn run_ninja_tool_internal( } /// Invoke `ninja` build and stream parsed task updates from status lines. +/// +/// # Errors +/// +/// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard +/// streams are unavailable, or when Ninja reports a non-zero exit status. pub(crate) fn run_ninja_with_status( request: NinjaBuildRequest<'_>, status_observer: StatusObserver<'_>, @@ -241,6 +246,11 @@ pub(crate) fn run_ninja_with_status( } /// Invoke `ninja -t` and stream parsed task updates from status lines. +/// +/// # Errors +/// +/// Returns an [`io::Error`] if the Ninja process fails to spawn, the standard +/// streams are unavailable, or when Ninja reports a non-zero exit status. pub(crate) fn run_ninja_tool_with_status( request: NinjaToolRequest<'_>, status_observer: StatusObserver<'_>, From 8df28d80e1ab7601c413ed604314ba19d55418bd Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 13:48:23 +0000 Subject: [PATCH 10/14] refactor(runner/process): simplify byte slice handling in streaming process Replaced manual buffer slicing with split_at to safely obtain a byte slice in `streaming.rs`, reducing bounds checking complexity and potential errors. Also removed an unnecessary example in `runner/mod.rs` and fixed minor doc markdown formatting. Co-authored-by: devboxerhub[bot] --- ...parse-ninja-status-lines-to-drive-task-progress.md | 6 +++--- src/runner/mod.rs | 11 ----------- src/runner/process/streaming.rs | 4 +--- 3 files changed, 4 insertions(+), 17 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index ce6f48c9..29ebf52a 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -119,7 +119,7 @@ Observable success: `docs/users-guide.md`, `docs/netsuke-design.md`, `docs/roadmap.md`; validated with `make check-fmt`, `make lint`, and `make test`. -## Surprises & Discoveries +## Surprises & discoveries - `src/runner/process/mod.rs` is already 421 lines, so adding parser logic in that file directly would violate project file-size guidance. This work should @@ -137,7 +137,7 @@ Observable success: test PATH constraints. Replaced with shell built-ins (`read` + `printf`) to keep fixtures deterministic. -## Decision Log +## Decision log - Decision: parse bracketed Ninja status tokens of the form `[current/total]` at line start for Stage 6 progress, treating malformed lines as non-events. @@ -156,7 +156,7 @@ Observable success: readability and must not depend solely on terminal redraw support. Date/Author: 2026-02-22 / Codex. -## Outcomes & Retrospective +## Outcomes & retrospective Implemented outcomes: diff --git a/src/runner/mod.rs b/src/runner/mod.rs index e15e3c90..089d17b5 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -176,17 +176,6 @@ fn on_task_progress_callback(reporter: &dyn StatusReporter) -> impl FnMut(u32, u /// # Errors /// /// Returns an error if manifest generation or Ninja execution fails. -/// -/// # Examples -/// ```ignore -/// use netsuke::cli::{BuildArgs, Cli}; -/// use netsuke::runner::handle_build; -/// use netsuke::status::SilentReporter; -/// let cli = Cli::default(); -/// let args = BuildArgs { emit: None, targets: vec![] }; -/// let progress_enabled = true; -/// handle_build(&cli, &args, &SilentReporter, progress_enabled).unwrap(); -/// ``` fn handle_build( cli: &Cli, args: &BuildArgs, diff --git a/src/runner/process/streaming.rs b/src/runner/process/streaming.rs index fb9362a5..6bdc8700 100644 --- a/src/runner/process/streaming.rs +++ b/src/runner/process/streaming.rs @@ -90,9 +90,7 @@ where self.finish_line(); return Ok(0); } - let slice = buf - .get(..count) - .ok_or_else(|| io::Error::other("reader returned out-of-range byte count"))?; + let (slice, _) = buf.split_at(count); self.consume_bytes(slice); Ok(count) } From ae938d3319a67472348f22662c034b29e2ff6838 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 15:30:00 +0000 Subject: [PATCH 11/14] feat(status): add with_text_task_updates() for text-only task updates Introduce a new constructor `with_text_task_updates` on `IndicatifReporter` that forces text-only task updates. This is useful for non-TTY environments or accessibility fallbacks where graphical task updates are not desired. Co-authored-by: devboxerhub[bot] --- src/status.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/status.rs b/src/status.rs index d9165c41..194f13ac 100644 --- a/src/status.rs +++ b/src/status.rs @@ -208,6 +208,13 @@ impl IndicatifReporter { } } + /// Build a reporter that forces text-only task updates (for non-TTY / + /// accessible fallback). + #[must_use] + pub fn with_text_task_updates() -> Self { + Self::new(true) + } + /// Build a reporter while explicitly controlling task-update text fallback. /// /// This forwards to [`Self::new`] and sets From 33a68974c374f84d956ea07581434d2f9414c52d Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 18:09:59 +0000 Subject: [PATCH 12/14] docs(execplans): improve wording in progress UX documentation Refined description of the standard interactive mode from "indicatif-based UX" to "indicatif-based user experience (UX)" and clarified the progress control copy listing by adding "and" before the config file key. Co-authored-by: devboxerhub[bot] --- ...3-9-2-parse-ninja-status-lines-to-drive-task-progress.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 29ebf52a..13c6a3c5 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -20,11 +20,11 @@ mode is active. After this change: - Stage 6 task progress is driven by parsed Ninja status lines. -- Standard interactive mode keeps the existing `indicatif`-based UX, now with - task counters. +- Standard interactive mode keeps the existing `indicatif`-based user + experience (UX), now with task counters. - Non-interactive stdout and accessible mode emit textual task updates. - Progress control continues through OrthoConfig layering with localized help - copy (`--progress`, `NETSUKE_PROGRESS`, config file key). + copy (`--progress`, `NETSUKE_PROGRESS`, and config file key). Observable success: From 905eea4533cab0c0f063a678b107a6d7a4153e2f Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 19:54:17 +0000 Subject: [PATCH 13/14] docs(execplans): enhance ninja status lines documentation with detailed test artefacts - Expanded artefacts and notes section with explicit unit test and BDD scenario names and statuses. - Included a representative stderr excerpt from non-TTY mode illustrating progress output. - Added command exit code results for formatting, linting, and testing commands. - Minor text correction from 'bidi' to 'bidirectional' for clarity. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 13c6a3c5..01346a98 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -130,9 +130,9 @@ Observable success: - Model Context Protocol (MCP) project-memory tools (`qdrant-find` / `qdrant-store`) were not available in this environment; repository docs were used as the only source of truth. -- Fluent rendering includes bidi isolation markers in localized strings, so - brittle literal assertions were replaced with content assertions that strip - isolation code points in unit tests. +- Fluent rendering includes bidirectional (bidi) isolation markers in localized + strings, so brittle literal assertions were replaced with content assertions + that strip isolation code points in unit tests. - The initial fake-Ninja fixture shell script used `cat`, which failed under test PATH constraints. Replaced with shell built-ins (`read` + `printf`) to keep fixtures deterministic. @@ -398,9 +398,28 @@ Acceptance requires all of the following: ## Artefacts and notes -Capture concise evidence for review: - -- Parser unit test names and pass/fail status. -- BDD scenario names proving fallback behaviour. -- A short stderr excerpt showing Stage 6 textual updates in non-TTY mode. -- Command exit codes for `make check-fmt`, `make lint`, and `make test`. +- Parser unit test + `runner::process::ninja_status::tests::parse_ninja_status_line_parses_expected::case_1` + — PASSED. +- Parser unit test + `runner::process::ninja_status::tests::tracker_accepts_only_monotonic_updates::case_1` + — PASSED. +- Parser unit test + `runner::process::streaming::tests::forward_output_with_ninja_status_parses_monotonic_updates` + — PASSED. +- BDD scenario + `features_scenarios::progress_output_standard_mode_reports_task_updates_from_ninja_status_lines` + — PASSED. +- BDD scenario + `features_scenarios::progress_output_accessible_mode_emits_textual_task_updates` + — PASSED. +- Non-TTY stderr excerpt (stdout redirected): + ```text + [⁨in progress⁩] ⁨Stage ⁨6⁩/⁨6⁩: ⁨Synthesizing Ninja plan and executing ⁨Build⁩⁩⁩ + ⁨Task ⁨1⁩/⁨2⁩⁩: ⁨cc -c src/a.c⁩ + ⁨Task ⁨2⁩/⁨2⁩⁩: ⁨cc -c src/b.c⁩ + [⁨done⁩] ⁨Stage ⁨6⁩/⁨6⁩: ⁨Synthesizing Ninja plan and executing ⁨Build⁩⁩⁩ + ``` +- `make check-fmt`: `0`. +- `make lint`: `0`. +- `make test`: `0`. From 049770435cf093d89379f9b1c51f0761b891d889 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 24 Feb 2026 22:57:18 +0000 Subject: [PATCH 14/14] docs(execplans): correct 'localized' to 'localised' spelling in documentation Consistently update spelling of 'localized' to 'localised' across the execplans documentation and related localized help text, reflecting preferred UK English spelling. No functional code changes; purely documentation and comment spelling corrections. Co-authored-by: devboxerhub[bot] --- ...nja-status-lines-to-drive-task-progress.md | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md index 01346a98..2d2b5895 100644 --- a/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md +++ b/docs/execplans/3-9-2-parse-ninja-status-lines-to-drive-task-progress.md @@ -23,7 +23,7 @@ After this change: - Standard interactive mode keeps the existing `indicatif`-based user experience (UX), now with task counters. - Non-interactive stdout and accessible mode emit textual task updates. -- Progress control continues through OrthoConfig layering with localized help +- Progress control continues through OrthoConfig layering with localised help copy (`--progress`, `NETSUKE_PROGRESS`, and config file key). Observable success: @@ -44,7 +44,7 @@ Observable success: - Keep accessible mode text-first and non-animated. - Use OrthoConfig layering for user control; do not introduce ad-hoc config reads. Existing `progress: Option` remains the controlling switch. -- Keep command-line interface (CLI) help localization wired through Fluent and +- Keep command-line interface (CLI) help localisation wired through Fluent and `src/cli_l10n.rs`. - Maintain public behaviour compatibility for existing commands. - Add unit tests using `rstest`. @@ -54,7 +54,7 @@ Observable success: - Update `docs/users-guide.md` with user-visible changes. - Mark roadmap item `3.9.2` done only after implementation and validation. - Run required quality gates with logged output: - `make check-fmt`, `make lint`, `make test`. + `make check-fmt`, `make lint`, and `make test`. ## Tolerances (exception triggers) @@ -130,7 +130,7 @@ Observable success: - Model Context Protocol (MCP) project-memory tools (`qdrant-find` / `qdrant-store`) were not available in this environment; repository docs were used as the only source of truth. -- Fluent rendering includes bidirectional (bidi) isolation markers in localized +- Fluent rendering includes bidirectional (bidi) isolation markers in localised strings, so brittle literal assertions were replaced with content assertions that strip isolation code points in unit tests. - The initial fake-Ninja fixture shell script used `cat`, which failed under @@ -145,9 +145,9 @@ Observable success: parser complexity low. Date/Author: 2026-02-22 / Codex. - Decision: use the existing OrthoConfig-backed `progress` setting as the sole - control for stage and task progress output, and update localized help text to + control for stage and task progress output, and update localised help text to reflect the expanded behaviour. Rationale: avoids unnecessary configuration - sprawl while satisfying layered config and localization requirements. + sprawl while satisfying layered config and localisation requirements. Date/Author: 2026-02-22 / Codex. - Decision: emit explicit textual task updates whenever stdout is non-TTY or @@ -188,9 +188,9 @@ Primary implementation surfaces: - `src/status.rs`: reporter trait and implementations. - `src/status_pipeline.rs`: six-stage canonical ordering and labels. - `src/cli/mod.rs`: OrthoConfig-derived CLI configuration. -- `src/cli_l10n.rs`: localized clap help mapping. +- `src/cli_l10n.rs`: localised clap help mapping. - `src/localization/keys.rs`: Fluent message key constants. -- `locales/en-US/messages.ftl` and `locales/es-ES/messages.ftl`: localized +- `locales/en-US/messages.ftl` and `locales/es-ES/messages.ftl`: localised user-facing strings. - `tests/features/progress_output.feature` and `tests/bdd/steps/*`: behavioural test coverage. @@ -260,7 +260,7 @@ Planned edits: - Centralize fallback predicate: - fallback when stdout is not a TTY, or - accessible mode is active. -- Update localized help copy for `progress` to clarify that it controls stage +- Update localised help copy for `progress` to clarify that it controls stage and task progress summaries. - Keep precedence through OrthoConfig (`config < env < CLI`) with existing `progress: Option` semantics. @@ -328,7 +328,7 @@ The implementation should remain dependency-neutral (no new external crates). Use existing crates already in `Cargo.toml`: - `indicatif` for standard mode rendering. -- `ortho_config` for layered configuration and localized help integration. +- `ortho_config` for layered configuration and localised help integration. - `rstest` and `rstest-bdd` v0.5.0 for unit and behavioural tests. Expected new internal interfaces (names may vary, behaviour is required): @@ -347,7 +347,7 @@ Run all commands from the repository root. 1. Implement parser module and parser unit tests. 2. Wire parser into process forwarding path with observer callbacks. 3. Extend reporter trait/implementations for task-progress updates. -4. Implement fallback predicate and update localized `progress` help strings. +4. Implement fallback predicate and update localised `progress` help strings. 5. Add/extend BDD scenarios and steps using fake Ninja emitters. 6. Update `docs/users-guide.md`, `docs/netsuke-design.md`, and `docs/roadmap.md`. @@ -414,12 +414,14 @@ Acceptance requires all of the following: `features_scenarios::progress_output_accessible_mode_emits_textual_task_updates` — PASSED. - Non-TTY stderr excerpt (stdout redirected): + ```text [⁨in progress⁩] ⁨Stage ⁨6⁩/⁨6⁩: ⁨Synthesizing Ninja plan and executing ⁨Build⁩⁩⁩ ⁨Task ⁨1⁩/⁨2⁩⁩: ⁨cc -c src/a.c⁩ ⁨Task ⁨2⁩/⁨2⁩⁩: ⁨cc -c src/b.c⁩ [⁨done⁩] ⁨Stage ⁨6⁩/⁨6⁩: ⁨Synthesizing Ninja plan and executing ⁨Build⁩⁩⁩ ``` + - `make check-fmt`: `0`. - `make lint`: `0`. - `make test`: `0`.