From 44a8000c99ce0266e0b7062a9c21e93bb1e949fe Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Feb 2026 13:34:12 +0000 Subject: [PATCH 1/9] feat(progress): add ExecPlan for integrating indicatif::MultiProgress Add a detailed ExecPlan document for roadmap 3.9.1 specifying design, constraints, risks, and staged work for integrating indicatif::MultiProgress reporting with six stages and localized progress feedback. This plan lays out progress configuration, stage model expansion, reporter implementation, manifest refactoring, testing, and documentation updates. Co-authored-by: devboxerhub[bot] --- .../3-8-1-add-accessible-output-mode.md | 3 + ...-9-1-integrate-indicatif-multi-progress.md | 341 ++++++++++++++++++ 2 files changed, 344 insertions(+) create mode 100644 docs/execplans/3-9-1-integrate-indicatif-multi-progress.md diff --git a/docs/execplans/3-8-1-add-accessible-output-mode.md b/docs/execplans/3-8-1-add-accessible-output-mode.md index 7ff47e2a..5a7489cc 100644 --- a/docs/execplans/3-8-1-add-accessible-output-mode.md +++ b/docs/execplans/3-8-1-add-accessible-output-mode.md @@ -171,6 +171,7 @@ Implementation complete. All quality gates pass (`make check-fmt`, `make lint`, `make test`). **What went well:** + - The `resolve_with` dependency-injection pattern for environment variable lookup made both unit tests and BDD tests clean and deterministic. - The `OutputMode` enum and `StatusReporter` trait provide a clean @@ -179,6 +180,7 @@ Implementation complete. All quality gates pass (`make check-fmt`, worked well with checkpoint validation between stages. **What was surprising:** + - `clippy::print_stderr` is denied globally, requiring `writeln!(io::stderr())` instead of `eprintln!`. The `drop()` wrapper follows the `main.rs` pattern. - The BDD `Given the environment variable` step already existed in @@ -186,6 +188,7 @@ Implementation complete. All quality gates pass (`make check-fmt`, env vars in the TestWorld avoided test interference. **Metrics:** + - 4 new files created, 10 files modified. - 12 unit tests (output_mode), 7 BDD scenarios (accessible_output). - ~300 net new lines of code (well within the 800-line tolerance). diff --git a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md b/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md new file mode 100644 index 00000000..10f58f35 --- /dev/null +++ b/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md @@ -0,0 +1,341 @@ +# Integrate `indicatif::MultiProgress` for six-stage feedback (roadmap 3.9.1) + +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 static stage lines only in accessible mode and emits +no progress in standard mode. Roadmap item 3.9.1 requires standard-mode +real-time feedback using `indicatif::MultiProgress`, with six pipeline stages, +persistent stage summaries, and localized labels. + +After this change: + +- Standard mode can display six persistent stage summaries managed by + `indicatif::MultiProgress`. +- Accessible mode remains static, textual, and screen-reader friendly. +- Stage labels and status words are localized via Fluent. +- Progress behaviour is configurable through OrthoConfig layering (config + file, environment variable, CLI), with localized help text. + +Observable success: + +- Running `netsuke --progress true manifest -` in a valid workspace emits + six stage summary lines on stderr and the Ninja manifest on stdout. +- Running with `--locale es-ES` localizes stage labels. +- Running with `--accessible true` keeps static textual stage reporting. +- `make check-fmt`, `make lint`, and `make test` pass. + +## Constraints + +- Keep the pipeline model aligned with the six-stage build flow documented in + `docs/netsuke-design.md`: Manifest Ingestion, Initial YAML Parsing, Template + Expansion, Deserialisation & Final Rendering, IR Generation & Validation, and + Ninja Synthesis & Execution. +- Preserve accessible mode guarantees from roadmap 3.8.1. +- Integrate progress configuration with OrthoConfig conventions and localized + CLI help. +- Localize all newly introduced user-facing strings in both: + `locales/en-US/messages.ftl` and `locales/es-ES/messages.ftl`. +- Add/adjust unit tests using `rstest`. +- Add/adjust behavioural tests using `rstest-bdd` v0.5.0. +- Cover happy paths, unhappy paths, and edge cases. +- Update `docs/users-guide.md` for user-visible behaviour changes. +- Record implementation decisions in the design document. +- Mark roadmap entry 3.9.1 as done only when implementation and validation are + complete. +- Run quality gates before finalizing: + `make check-fmt`, `make lint`, and `make test`. + +## Tolerances (exception triggers) + +- Scope: if the implementation requires more than ~18 touched files or + ~1000 net new lines, stop and confirm scope before continuing. +- API compatibility: if any existing public API must break, stop and escalate. +- Output ordering: if `MultiProgress` causes irreconcilable stream corruption + with subprocess output, stop and escalate with alternatives. +- Test stability: if progress rendering tests remain flaky after two + stabilization attempts, stop and escalate with deterministic fallback options. +- Localization drift: if Fluent key expansion requires broad unrelated + catalogue refactors, stop and isolate the minimal path. + +## Risks + +- Risk: Netsuke currently reports five stages in `src/status.rs`, not six. + Mitigation: introduce a six-stage canonical enum and route all reporters + through it; add tests asserting stage count and order. + +- Risk: Manifest pipeline internals are currently collapsed in + `manifest::from_str_named`, making fine-grained stage reporting difficult. + Mitigation: refactor manifest loading into explicit pipeline steps and report + boundaries from runner-controlled orchestration. + +- Risk: `indicatif` redraw and subprocess stderr output can interleave. + Mitigation: keep stage progress updates coarse-grained (stage transitions), + avoid high-frequency spinner ticks, and test stderr behaviour with stripped + ANSI assertions. + +- Risk: Non-interactive test environments can hide or alter progress drawing. + Mitigation: add explicit progress config (`--progress`) so behavioural tests + can force deterministic progress mode without relying on TTY auto-detection. + +- Risk: Fluent key additions can miss one locale and fail build-time audits. + Mitigation: add keys in both locales together and include localization smoke + tests for new progress messages. + +## Progress + +- [x] 2026-02-12: Gathered context from roadmap, design docs, status/runner + modules, localization files, and existing BDD/unit test surfaces. +- [x] 2026-02-12: Drafted this ExecPlan in + `docs/execplans/3-9-1-integrate-indicatif-multi-progress.md`. +- [ ] Implement six-stage progress model and `indicatif::MultiProgress` + standard reporter. +- [ ] Add OrthoConfig-backed progress configuration and localized help. +- [ ] Add unit tests (`rstest`) for reporter logic, stage mapping, and config + precedence. +- [ ] Add behavioural tests (`rstest-bdd` v0.5.0) for standard/accessible, + localized, and failure paths. +- [ ] Update `docs/users-guide.md` and design document decisions. +- [ ] Mark roadmap item 3.9.1 done. +- [ ] Run and pass required quality gates: + `make check-fmt`, `make lint`, `make test`. + +## Surprises & Discoveries + +- The current status pipeline is explicitly five stages in `src/status.rs`, + while the design and roadmap target six-stage user-facing reporting. +- `manifest::from_path_with_policy` currently performs multiple pipeline steps + inside one function, so stage-level reporting needs extraction. +- Standard mode currently uses `SilentReporter`; no `indicatif` dependency is + present. +- No project-memory MCP resources were available in this environment during + planning, so repository docs were used as the authoritative source. + +## Decision Log + +- Decision: adopt the six-stage user-facing model from `docs/netsuke-design.md` + for progress reporting, even if internal implementation details differ. + Rationale: roadmap 3.9.1 explicitly asks to surface six stages, and the + design document already establishes those stages as the user mental model. + Date/Author: 2026-02-12 / Codex. + +- Decision: keep accessible mode as the non-animated fallback path and do not + regress existing static output semantics. Rationale: roadmap 3.8.1 and + accessibility requirements remain hard invariants. Date/Author: 2026-02-12 / + Codex. + +- Decision: introduce an OrthoConfig-managed `progress` setting to control + progress rendering ergonomically via config/env/CLI, with localized help. + Rationale: this satisfies the explicit requirement to use OrthoConfig and + provides deterministic behavioural testing in non-TTY environments. + Date/Author: 2026-02-12 / Codex. + +## Outcomes & Retrospective + +Pending implementation. + +## Context and orientation + +Primary implementation surfaces: + +- `src/status.rs`: progress abstractions, stage enum, localized descriptions. +- `src/runner/mod.rs`: reporter selection and stage transition calls. +- `src/manifest/mod.rs`: manifest pipeline decomposition for stage boundaries. +- `src/runner/process/mod.rs`: subprocess output streaming interaction surface. +- `src/cli/mod.rs`: OrthoConfig-backed CLI fields and merge filtering. +- `src/cli_l10n.rs`: localized help key mapping. +- `src/localization/keys.rs`: Fluent key constants. +- `locales/en-US/messages.ftl`, `locales/es-ES/messages.ftl`: localized copy. +- `tests/`: unit, integration, and BDD coverage. +- `docs/users-guide.md`: user-visible output/configuration documentation. +- `docs/netsuke-design.md`: design decision updates. +- `docs/roadmap.md`: task completion checkbox updates. + +Useful existing patterns: + +- `OutputMode::resolve_with` in `src/output_mode.rs` for dependency-injected + detection. +- Existing `StatusReporter` abstraction in `src/status.rs`. +- BDD command execution and stderr assertions in + `tests/bdd/steps/manifest_command.rs`. +- Localized CLI help integration in `src/cli_l10n.rs`. + +## Plan of work + +## Stage A: Add progress configuration through OrthoConfig + +Extend CLI configuration to include a progress toggle that follows layered +precedence and localized help text. + +Planned changes: + +- Add `progress: Option` to `Cli` in `src/cli/mod.rs`. +- Ensure `Cli::default()` initializes `progress` to `None`. +- Include `progress` in `cli_overrides_from_matches` command-line source + filtering. +- Add localized flag help key (for example `cli.flag.progress.help`) in: + `src/localization/keys.rs`, `locales/en-US/messages.ftl`, + `locales/es-ES/messages.ftl`, and `src/cli_l10n.rs`. +- Update CLI parsing/merge tests to verify OrthoConfig precedence and explicit + override behaviour. + +Acceptance for Stage A: + +- CLI accepts `--progress true|false`. +- `NETSUKE_PROGRESS` and config file `progress = true/false` merge correctly. +- Help text is localized for the new option. + +## Stage B: Refactor reporting model to six pipeline stages + +Replace five-stage reporting with six canonical stages and explicit stage +status transitions. + +Planned changes: + +- Update `PipelineStage` in `src/status.rs` to six variants aligned with + `docs/netsuke-design.md`. +- Introduce stage status semantics (`pending`, `running`, `done`, `failed`) + for persistent summaries. +- Add localized keys for new stage labels/status words as needed. +- Ensure a single source of truth for stage count and ordering. + +Acceptance for Stage B: + +- Stage count constant is six and verified by unit tests. +- Stage descriptions are localized and deterministic. + +## Stage C: Integrate `indicatif::MultiProgress` standard reporter + +Implement a standard reporter backed by `indicatif::MultiProgress`, keeping +lines persistent as stages complete. + +Planned changes: + +- Add `indicatif` dependency to `Cargo.toml` (caret requirement). +- Replace `SilentReporter` usage in standard mode with a new reporter (for + example `IndicatifReporter`) that: creates six stage lines up front, updates + stage state transitions, keeps summaries visible after completion, marks + failure when a stage errors. +- Keep `AccessibleReporter` intact for accessible mode. +- Select reporter in `runner::run` using resolved output mode plus progress + config. + +Acceptance for Stage C: + +- Standard mode shows six persistent summary lines. +- Accessible mode remains static text and does not animate. +- Failure paths mark the failing stage clearly. + +## Stage D: Expose true six-stage boundaries in runner + manifest + +Refactor manifest loading/orchestration so stage transitions match real +pipeline boundaries. + +Planned changes: + +- Split manifest loading flow in `src/manifest/mod.rs` into explicit steps that + can be reported individually. +- Update `src/runner/mod.rs` to report stage transitions at the boundary of + each of the six stages. +- Ensure stage 6 covers synthesis + execution semantics for the active command + (`build`, `clean`, `graph`, `manifest`) with localized wording. + +Acceptance for Stage D: + +- Stage transitions occur in the documented order. +- Early failures stop at the correct stage and preserve prior summaries. + +## Stage E: Unit tests with `rstest` + +Add focused unit coverage for stage modelling, reporter transitions, and +configuration precedence. + +Planned test additions: + +- `src/status.rs` or `src/runner/tests.rs`: + stage count/order assertions, localized stage-label resolution checks, + reporter transition happy and failure-path tests. +- `tests/cli_tests/*`: + parse + merge cases for `progress` from defaults, config, env, and CLI. +- `tests/localization_tests.rs`: + resolve newly introduced progress/stage messages in `en-US` and `es-ES`. + +Key edge cases: + +- `accessible=true` with `progress=true` (accessible must win). +- `progress=false` in standard mode (no multi-progress output). +- failure before stage 6 still yields persistent summaries for completed + stages. + +## Stage F: Behavioural tests with `rstest-bdd` v0.5.0 + +Add BDD scenarios that exercise real command execution and user-visible output +contracts. + +Planned changes: + +- Add `tests/features/progress_output.feature`. +- Add `tests/bdd/steps/progress_output.rs` and register it in + `tests/bdd/steps/mod.rs`. +- Reuse existing command-run steps/helpers for capturing stdout/stderr. + +Planned scenarios: + +- Happy path: standard mode with progress enabled emits six stage summaries. +- Happy path localized: `--locale es-ES --progress true` emits Spanish labels. +- Unhappy path: invalid manifest reports failure and marks the current stage. +- Edge case: accessible mode overrides standard progress rendering. +- Edge case: progress explicitly disabled suppresses standard progress UI. + +## Stage G: Documentation and roadmap updates + +Synchronize user and design documentation with implemented behaviour. + +Planned changes: + +- Update `docs/users-guide.md`: + standard-mode progress behaviour, six stages, persistent summaries, + `--progress` / `NETSUKE_PROGRESS` / config key usage and precedence. +- Update design decisions in `docs/netsuke-design.md` to record chosen + architecture/trade-offs for six-stage reporting and reporter selection. +- Mark roadmap item 3.9.1 and its sub-bullets done in `docs/roadmap.md` once + implementation/tests are complete. + +## Validation and evidence capture + +Run all required gates with `tee` and `pipefail`: + + set -o pipefail + make check-fmt 2>&1 | tee /tmp/3-9-1-check-fmt.log + make lint 2>&1 | tee /tmp/3-9-1-lint.log + make test 2>&1 | tee /tmp/3-9-1-test.log + +Documentation gates after doc updates: + + set -o pipefail + make fmt 2>&1 | tee /tmp/3-9-1-fmt.log + make markdownlint 2>&1 | tee /tmp/3-9-1-markdownlint.log + make nixie 2>&1 | tee /tmp/3-9-1-nixie.log + +Record concise evidence in commit/PR notes: + +- command exit codes, +- relevant new/updated test names, +- stderr snippets demonstrating six-stage persistent summaries, +- locale-specific output proof points. + +## Rollback / recovery strategy + +- If `indicatif` integration causes severe output regressions, retain + `AccessibleReporter` and a guarded standard fallback path (`progress=false`) + while keeping stage model and localization changes. +- If pipeline-stage refactor introduces functional regressions, land progress + reporter scaffolding first behind standard-mode gating, then complete stage + boundary refactor in a follow-up atomic change. From a4890707959621d563c9e1df25f9c114c189871b Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Feb 2026 17:14:44 +0000 Subject: [PATCH 2/9] feat(progress): add six-stage real-time progress reporting with indicatif - Integrate indicatif::MultiProgress to display persistent progress summaries for six pipeline stages. - Provide localized, stateful labels for pipeline stages: pending, in progress, done, failed. - Support CLI flag, env var, and config file options to enable or disable standard progress summaries. - Accessible mode produces static, labelled status lines and takes precedence over standard mode. - Add manifest loading stages with explicit callbacks for fine-grained progress reporting. - Update documentation and localization files for progress reporting and new CLI flag `--progress`. - Add behavioral tests for progress output, including localization and failure scenarios. - Replace silent reporter with indicatif reporter when progress summaries are enabled in standard mode. - Ensure accurate stage reporting and graceful fallback on failures. This enhancement improves user feedback during builds with real-time, accessible, and localized progress summaries. Co-authored-by: devboxerhub[bot] --- Cargo.lock | 98 +++++ Cargo.toml | 1 + ...-9-1-integrate-indicatif-multi-progress.md | 9 +- docs/netsuke-design.md | 9 + docs/roadmap.md | 6 +- docs/users-guide.md | 48 ++- locales/en-US/messages.ftl | 19 +- locales/es-ES/messages.ftl | 19 +- src/cli/mod.rs | 8 + src/cli_l10n.rs | 1 + src/localization/keys.rs | 19 +- src/manifest/mod.rs | 51 ++- src/manifest/tests/mod.rs | 1 + src/manifest/tests/stages.rs | 73 ++++ src/runner/mod.rs | 159 ++++---- src/status.rs | 345 +++++++++++++----- tests/bdd/steps/manifest_command.rs | 25 ++ tests/cli_tests/merge.rs | 17 +- tests/cli_tests/parsing.rs | 11 + tests/features/progress_output.feature | 37 ++ tests/localization_tests.rs | 34 ++ 21 files changed, 805 insertions(+), 185 deletions(-) create mode 100644 src/manifest/tests/stages.rs create mode 100644 tests/features/progress_output.feature diff --git a/Cargo.lock b/Cargo.lock index ddd2ac90..748373aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -201,6 +201,12 @@ dependencies = [ "serde", ] +[[package]] +name = "bumpalo" +version = "3.19.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5dd9dc738b7a8311c7ade152424974d8115f2cdad61e8dab8dac9f2362298510" + [[package]] name = "bytemuck" version = "1.24.0" @@ -337,6 +343,7 @@ dependencies = [ "encode_unicode", "libc", "once_cell", + "unicode-width 0.2.1", "windows-sys 0.59.0", ] @@ -1037,6 +1044,19 @@ dependencies = [ "serde_core", ] +[[package]] +name = "indicatif" +version = "0.17.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "183b3088984b400f4cfac3620d5e076c84da5364016b4f49473de574b2586235" +dependencies = [ + "console", + "number_prefix", + "portable-atomic", + "unicode-width 0.2.1", + "web-time", +] + [[package]] name = "inlinable_string" version = "0.1.15" @@ -1152,6 +1172,16 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4a5f13b858c8d314ee3e8f639011f7ccefe71f97f96e50151fb991f267928e2c" +[[package]] +name = "js-sys" +version = "0.3.85" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8c942ebf8e95485ca0d52d97da7c5a2c387d0e7f0ba4c35e93bfcaee045955b3" +dependencies = [ + "once_cell", + "wasm-bindgen", +] + [[package]] name = "lazy_static" version = "1.5.0" @@ -1351,6 +1381,7 @@ dependencies = [ "digest", "glob", "indexmap", + "indicatif", "insta", "itertools 0.12.1", "itoa", @@ -1439,6 +1470,12 @@ dependencies = [ "autocfg", ] +[[package]] +name = "number_prefix" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "830b246a0e5f20af87141b25c173cd1b609bd7779a4617d6ec582abaf90870f3" + [[package]] name = "object" version = "0.36.7" @@ -1607,6 +1644,12 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" +[[package]] +name = "portable-atomic" +version = "1.13.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c33a9471896f1c69cecef8d20cbe2f7accd12527ce60845ff44c153bb2a21b49" + [[package]] name = "potential_utf" version = "0.1.3" @@ -2940,6 +2983,61 @@ dependencies = [ "wit-bindgen-rt", ] +[[package]] +name = "wasm-bindgen" +version = "0.2.108" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "64024a30ec1e37399cf85a7ffefebdb72205ca1c972291c51512360d90bd8566" +dependencies = [ + "cfg-if", + "once_cell", + "rustversion", + "wasm-bindgen-macro", + "wasm-bindgen-shared", +] + +[[package]] +name = "wasm-bindgen-macro" +version = "0.2.108" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "008b239d9c740232e71bd39e8ef6429d27097518b6b30bdf9086833bd5b6d608" +dependencies = [ + "quote", + "wasm-bindgen-macro-support", +] + +[[package]] +name = "wasm-bindgen-macro-support" +version = "0.2.108" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5256bae2d58f54820e6490f9839c49780dff84c65aeab9e772f15d5f0e913a55" +dependencies = [ + "bumpalo", + "proc-macro2", + "quote", + "syn 2.0.104", + "wasm-bindgen-shared", +] + +[[package]] +name = "wasm-bindgen-shared" +version = "0.2.108" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1f01b580c9ac74c8d8f0c0e4afb04eeef2acf145458e52c03845ee9cd23e3d12" +dependencies = [ + "unicode-ident", +] + +[[package]] +name = "web-time" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a6580f308b1fad9207618087a65c04e7a10bc77e02c8e84e9b00dd4b12fa0bb" +dependencies = [ + "js-sys", + "wasm-bindgen", +] + [[package]] name = "webpki-roots" version = "0.26.11" diff --git a/Cargo.toml b/Cargo.toml index 50a8766b..7e8e56cb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,6 +27,7 @@ cap-std = { version = "3.4.4", features = ["fs_utf8"] } camino = "1.2.0" semver = { version = "1", features = ["serde"] } anyhow = "1" +indicatif = "0.17.11" thiserror = "1" miette = { version = "7.6.0", features = ["fancy"] } digest = "0.10" diff --git a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md b/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md index 10f58f35..8275862f 100644 --- a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md +++ b/docs/execplans/3-9-1-integrate-indicatif-multi-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: IN PROGRESS No `PLANS.md` file exists in this repository. @@ -95,9 +95,9 @@ Observable success: modules, localization files, and existing BDD/unit test surfaces. - [x] 2026-02-12: Drafted this ExecPlan in `docs/execplans/3-9-1-integrate-indicatif-multi-progress.md`. -- [ ] Implement six-stage progress model and `indicatif::MultiProgress` +- [x] Implement six-stage progress model and `indicatif::MultiProgress` standard reporter. -- [ ] Add OrthoConfig-backed progress configuration and localized help. +- [x] Add OrthoConfig-backed progress configuration and localized help. - [ ] Add unit tests (`rstest`) for reporter logic, stage mapping, and config precedence. - [ ] Add behavioural tests (`rstest-bdd` v0.5.0) for standard/accessible, @@ -117,6 +117,9 @@ Observable success: present. - No project-memory MCP resources were available in this environment during planning, so repository docs were used as the authoritative source. +- The runtime `manifest` command path needed an explicit completion call after + synthesis; otherwise an in-progress stage was finalized as failed in the new + reporter drop path. ## Decision Log diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index aff2d6ab..26306f91 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2039,6 +2039,15 @@ a configured working directory and resolves relative output paths (for example real directory change. Error scenarios are validated using clap's `ErrorKind` enumeration in unit tests and via Cucumber steps for behavioural coverage. +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. +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 1f3f5f10..186c6913 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -257,9 +257,9 @@ library, and CLI ergonomics. ### 3.9. Real-time feedback and progress -- [ ] 3.9.1. Integrate `indicatif::MultiProgress`. - - [ ] Surface the six pipeline stages with persistent summaries. - - [ ] Apply localization-aware labelling. +- [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 mode is active. diff --git a/docs/users-guide.md b/docs/users-guide.md index c28f87a3..6bf26e19 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -570,8 +570,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. +indicators with static, labelled status lines suitable for screen readers and +dumb terminals. Accessible mode is auto-enabled when: @@ -588,20 +588,46 @@ Explicit configuration always takes precedence over auto-detection, in either direction (`--accessible false` disables accessible mode even when `NO_COLOR` is set). -When accessible mode is active, each pipeline stage produces a labelled -status line on stderr: +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 Build complete. ``` -In standard mode, no status lines are emitted. Future versions may add -animated progress indicators for standard mode terminals. +### Real-time progress summaries + +In standard mode, Netsuke uses persistent progress summaries for the six-stage +pipeline. The summaries are localized and remain visible as stages complete. + +Progress summaries are enabled by default in standard mode. You can force them +on or off through layered configuration: + +- CLI flag: `--progress true` or `--progress false` +- Environment variable: `NETSUKE_PROGRESS=true` +- Configuration file: `progress = true` + +Accessible mode takes precedence. When `--accessible true` (or auto-detected +accessible mode) is active, Netsuke uses static accessible lines even when +`progress = true`. + +Typical standard-mode output includes localized state prefixes: + +```text +[done] Stage 1/6: Reading manifest file +[done] Stage 2/6: Parsing YAML document +[done] Stage 3/6: Expanding template directives +[done] Stage 4/6: Deserializing and rendering manifest values +[done] Stage 5/6: Building and validating dependency graph +[in progress] Stage 6/6: Synthesizing Ninja plan and executing Build +Build complete. +``` ### Exit Codes diff --git a/locales/en-US/messages.ftl b/locales/en-US/messages.ftl index 0e520b5b..9ccfd0b0 100644 --- a/locales/en-US/messages.ftl +++ b/locales/en-US/messages.ftl @@ -15,6 +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. # Subcommand descriptions. cli.subcommand.build.about = Build targets defined in the manifest (default). @@ -318,16 +319,24 @@ stdlib.register.resolve_dir = Failed to resolve current directory for stdlib reg stdlib.register.dir_non_utf8 = Current directory contains non-UTF-8 components: { $path }. # Status reporting for accessible output mode. +status.state.pending = pending +status.state.running = in progress +status.state.done = done +status.state.failed = failed status.stage.label = Stage { $current }/{ $total }: { $description } -status.stage.manifest_load = Loading manifest -status.stage.network_policy = Configuring network policy -status.stage.build_graph = Building dependency graph -status.stage.generate_ninja = Generating Ninja file -status.stage.execute = Executing { $tool } +status.stage.summary = [{ $state }] { $label } +status.stage.manifest_ingestion = Reading manifest file +status.stage.initial_yaml_parsing = Parsing YAML document +status.stage.template_expansion = Expanding template directives +status.stage.final_rendering = Deserializing and rendering manifest values +status.stage.ir_generation_validation = Building and validating dependency graph +status.stage.ninja_synthesis = Synthesizing Ninja build plan +status.stage.ninja_synthesis_execute = Synthesizing Ninja plan and executing { $tool } status.complete = { $tool } complete. status.tool.build = Build status.tool.clean = Clean status.tool.graph = Graph +status.tool.manifest = Manifest # Plural form examples for translators. # These messages demonstrate Fluent's select expression syntax using CLDR diff --git a/locales/es-ES/messages.ftl b/locales/es-ES/messages.ftl index 8a6c3723..d86e53a9 100644 --- a/locales/es-ES/messages.ftl +++ b/locales/es-ES/messages.ftl @@ -15,6 +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). # Descripciones de subcomandos. cli.subcommand.build.about = Compila objetivos definidos en el manifiesto (predeterminado). @@ -318,16 +319,24 @@ stdlib.register.resolve_dir = No se pudo resolver el directorio actual para regi stdlib.register.dir_non_utf8 = El directorio actual contiene componentes no UTF-8: { $path }. # Informes de estado para el modo de salida accesible. +status.state.pending = pendiente +status.state.running = en progreso +status.state.done = completada +status.state.failed = fallida status.stage.label = Etapa { $current }/{ $total }: { $description } -status.stage.manifest_load = Cargando manifiesto -status.stage.network_policy = Configurando política de red -status.stage.build_graph = Construyendo grafo de dependencias -status.stage.generate_ninja = Generando archivo Ninja -status.stage.execute = Ejecutando { $tool } +status.stage.summary = [{ $state }] { $label } +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 +status.stage.final_rendering = Deserializando y renderizando valores del manifiesto +status.stage.ir_generation_validation = Construyendo y validando el grafo de dependencias +status.stage.ninja_synthesis = Sintetizando el plan de compilación Ninja +status.stage.ninja_synthesis_execute = Sintetizando el plan Ninja y ejecutando { $tool } status.complete = { $tool } completo. status.tool.build = Compilación status.tool.clean = Limpieza status.tool.graph = Grafo +status.tool.manifest = Manifiesto # Ejemplos de formas plurales para traductores. # Estos mensajes demuestran la sintaxis de expresiones select de Fluent diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 6c921f83..a95ae07c 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -138,6 +138,12 @@ pub struct Cli { #[arg(long)] pub accessible: Option, + /// Force standard progress summaries on or off. + /// + /// When omitted, Netsuke enables progress summaries in standard mode. + #[arg(long)] + pub progress: Option, + /// Optional subcommand to execute; defaults to `build` when omitted. /// /// `OrthoConfig` merging ignores this field; CLI parsing supplies it. @@ -174,6 +180,7 @@ impl Default for Cli { fetch_block_host: Vec::new(), fetch_default_deny: false, accessible: None, + progress: None, command: None, } .with_default_command() @@ -301,6 +308,7 @@ fn cli_overrides_from_matches(cli: &Cli, matches: &ArgMatches) -> OrthoResult) -> Option<&'static "fetch_block_host" => Some(keys::CLI_FLAG_FETCH_BLOCK_HOST_HELP), "fetch_default_deny" => Some(keys::CLI_FLAG_FETCH_DEFAULT_DENY_HELP), "accessible" => Some(keys::CLI_FLAG_ACCESSIBLE_HELP), + "progress" => Some(keys::CLI_FLAG_PROGRESS_HELP), _ => None, }, Some("build") => match arg_id { diff --git a/src/localization/keys.rs b/src/localization/keys.rs index fc3ca347..8ad1aada 100644 --- a/src/localization/keys.rs +++ b/src/localization/keys.rs @@ -22,6 +22,7 @@ define_keys! { CLI_FLAG_FETCH_BLOCK_HOST_HELP => "cli.flag.fetch_block_host.help", CLI_FLAG_FETCH_DEFAULT_DENY_HELP => "cli.flag.fetch_default_deny.help", CLI_FLAG_ACCESSIBLE_HELP => "cli.flag.accessible.help", + CLI_FLAG_PROGRESS_HELP => "cli.flag.progress.help", CLI_SUBCOMMAND_BUILD_ABOUT => "cli.subcommand.build.about", CLI_SUBCOMMAND_BUILD_LONG_ABOUT => "cli.subcommand.build.long_about", CLI_SUBCOMMAND_CLEAN_ABOUT => "cli.subcommand.clean.about", @@ -277,16 +278,24 @@ define_keys! { STDLIB_REGISTER_OPEN_DIR => "stdlib.register.open_dir", STDLIB_REGISTER_RESOLVE_DIR => "stdlib.register.resolve_dir", STDLIB_REGISTER_DIR_NON_UTF8 => "stdlib.register.dir_non_utf8", + STATUS_STATE_PENDING => "status.state.pending", + STATUS_STATE_RUNNING => "status.state.running", + STATUS_STATE_DONE => "status.state.done", + STATUS_STATE_FAILED => "status.state.failed", STATUS_STAGE_LABEL => "status.stage.label", - STATUS_STAGE_MANIFEST_LOAD => "status.stage.manifest_load", - STATUS_STAGE_NETWORK_POLICY => "status.stage.network_policy", - STATUS_STAGE_BUILD_GRAPH => "status.stage.build_graph", - STATUS_STAGE_GENERATE_NINJA => "status.stage.generate_ninja", - STATUS_STAGE_EXECUTE => "status.stage.execute", + STATUS_STAGE_SUMMARY => "status.stage.summary", + 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", + STATUS_STAGE_FINAL_RENDERING => "status.stage.final_rendering", + STATUS_STAGE_IR_GENERATION_VALIDATION => "status.stage.ir_generation_validation", + STATUS_STAGE_NINJA_SYNTHESIS => "status.stage.ninja_synthesis", + STATUS_STAGE_NINJA_SYNTHESIS_EXECUTE => "status.stage.ninja_synthesis_execute", STATUS_COMPLETE => "status.complete", STATUS_TOOL_BUILD => "status.tool.build", STATUS_TOOL_CLEAN => "status.tool.clean", STATUS_TOOL_GRAPH => "status.tool.graph", + STATUS_TOOL_MANIFEST => "status.tool.manifest", EXAMPLE_FILES_PROCESSED => "example.files_processed", EXAMPLE_ERRORS_FOUND => "example.errors_found", } diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index 77eb3131..091b91b6 100644 --- a/src/manifest/mod.rs +++ b/src/manifest/mod.rs @@ -52,6 +52,19 @@ pub use render::render_manifest; use self::jinja_macros::register_manifest_macros; +/// Stages in the manifest-loading sub-pipeline. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum ManifestLoadStage { + /// Read raw manifest content from the filesystem. + ManifestIngestion, + /// Parse raw YAML into a `serde_json::Value` tree. + InitialYamlParsing, + /// Expand `foreach` and `when` template directives. + TemplateExpansion, + /// Deserialize and render string fields into typed manifest data. + FinalRendering, +} + /// Resolve the value of an environment variable for the `env()` Jinja helper. /// /// Returns the variable's value or a structured error that mirrors Jinja's @@ -101,6 +114,20 @@ fn from_str_named( name: &ManifestName, stdlib_config: Option, ) -> Result { + let mut no_op = |_stage| {}; + from_str_named_with_stage_callback(yaml, name, stdlib_config, &mut no_op) +} + +fn from_str_named_with_stage_callback( + yaml: &str, + name: &ManifestName, + stdlib_config: Option, + on_stage: &mut F, +) -> Result +where + F: FnMut(ManifestLoadStage), +{ + on_stage(ManifestLoadStage::InitialYamlParsing); let mut doc: ManifestValue = serde_saphyr::from_str(yaml).map_err(|e| ManifestError::Parse { source: map_yaml_error(e, &ManifestSource::from(yaml), name), @@ -135,10 +162,12 @@ fn from_str_named( } } + on_stage(ManifestLoadStage::TemplateExpansion); register_manifest_macros(&doc, &mut jinja)?; expand_foreach(&mut doc, &jinja)?; + on_stage(ManifestLoadStage::FinalRendering); let manifest: NetsukeManifest = serde_json::from_value(doc).map_err(|e| ManifestError::Parse { source: map_data_error(e, name), @@ -190,6 +219,26 @@ pub fn from_path_with_policy( path: impl AsRef, policy: NetworkPolicy, ) -> Result { + from_path_with_policy_and_stage_callback(path, policy, |_stage| {}) +} + +/// Load a [`NetsukeManifest`] from the given file path using an explicit +/// network policy and a stage callback. +/// +/// The callback is invoked in order for each manifest stage. +/// +/// # Errors +/// +/// Returns an error if the file cannot be read or the YAML fails to parse. +pub fn from_path_with_policy_and_stage_callback( + path: impl AsRef, + policy: NetworkPolicy, + mut on_stage: F, +) -> Result +where + F: FnMut(ManifestLoadStage), +{ + on_stage(ManifestLoadStage::ManifestIngestion); let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref).with_context(|| { localization::message(keys::MANIFEST_READ_FAILED) @@ -197,7 +246,7 @@ pub fn from_path_with_policy( })?; let name = ManifestName::new(path_ref.display().to_string()); let config = stdlib_config_for_manifest(path_ref, policy)?; - from_str_named(&data, &name, Some(config)) + from_str_named_with_stage_callback(&data, &name, Some(config), &mut on_stage) } #[cfg(test)] diff --git a/src/manifest/tests/mod.rs b/src/manifest/tests/mod.rs index 1bc80b98..c7bd4ad8 100644 --- a/src/manifest/tests/mod.rs +++ b/src/manifest/tests/mod.rs @@ -1,4 +1,5 @@ //! Tests for manifest parsing and macro helpers. mod macros; +mod stages; mod workspace; diff --git a/src/manifest/tests/stages.rs b/src/manifest/tests/stages.rs new file mode 100644 index 00000000..edaa0f3c --- /dev/null +++ b/src/manifest/tests/stages.rs @@ -0,0 +1,73 @@ +//! Tests for manifest stage callback ordering. + +use crate::manifest::{self, ManifestLoadStage}; +use crate::stdlib::NetworkPolicy; +use anyhow::{Context, Result, ensure}; + +fn minimal_manifest() -> &'static str { + concat!( + "netsuke_version: \"1.0.0\"\n", + "rules:\n", + " - name: touch\n", + " command: \"touch $out\"\n", + "targets:\n", + " - name: out.txt\n", + " rule: touch\n", + ) +} + +#[test] +fn stage_callback_reports_expected_order_for_valid_manifest() -> Result<()> { + let dir = tempfile::tempdir().context("create temp workspace for stage test")?; + let manifest_path = dir.path().join("Netsukefile"); + std::fs::write(&manifest_path, minimal_manifest()) + .with_context(|| format!("write {}", manifest_path.display()))?; + + let mut stages = Vec::new(); + let manifest = manifest::from_path_with_policy_and_stage_callback( + &manifest_path, + NetworkPolicy::default(), + |stage| stages.push(stage), + )?; + + ensure!( + !manifest.targets.is_empty(), + "manifest should contain targets" + ); + ensure!( + stages + == vec![ + ManifestLoadStage::ManifestIngestion, + ManifestLoadStage::InitialYamlParsing, + ManifestLoadStage::TemplateExpansion, + ManifestLoadStage::FinalRendering, + ], + "unexpected stage ordering: {stages:?}" + ); + Ok(()) +} + +#[test] +fn stage_callback_stops_after_parse_failure() -> Result<()> { + let dir = tempfile::tempdir().context("create temp workspace for stage failure test")?; + let manifest_path = dir.path().join("Netsukefile"); + std::fs::write(&manifest_path, "targets:\n\t- name: broken\n") + .with_context(|| format!("write {}", manifest_path.display()))?; + + let mut stages = Vec::new(); + let result = manifest::from_path_with_policy_and_stage_callback( + &manifest_path, + NetworkPolicy::default(), + |stage| stages.push(stage), + ); + ensure!(result.is_err(), "invalid manifest should fail"); + ensure!( + stages + == vec![ + ManifestLoadStage::ManifestIngestion, + ManifestLoadStage::InitialYamlParsing, + ], + "unexpected stages for parse failure: {stages:?}" + ); + Ok(()) +} diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 213f103b..2dafc5d9 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -12,7 +12,8 @@ use crate::cli::{BuildArgs, Cli, Commands}; use crate::localization::{self, keys}; use crate::output_mode::{self, OutputMode}; use crate::status::{ - AccessibleReporter, PipelineStage, SilentReporter, StatusReporter, report_pipeline_stage, + AccessibleReporter, IndicatifReporter, PipelineStage, SilentReporter, StatusReporter, + report_pipeline_stage, }; use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result, anyhow}; @@ -93,9 +94,10 @@ impl Default for BuildTargets<'_> { /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli) -> Result<()> { let mode = output_mode::resolve(cli.accessible); - let reporter: Box = match mode { - OutputMode::Accessible => Box::new(AccessibleReporter), - OutputMode::Standard => Box::new(SilentReporter), + let reporter: Box = match (mode, cli.progress.unwrap_or(true)) { + (OutputMode::Accessible, _) => Box::new(AccessibleReporter), + (OutputMode::Standard, true) => Box::new(IndicatifReporter::new()), + (OutputMode::Standard, false) => Box::new(SilentReporter), }; let command = cli.command.clone().unwrap_or(Commands::Build(BuildArgs { @@ -105,13 +107,14 @@ pub fn run(cli: &Cli) -> Result<()> { match command { Commands::Build(args) => handle_build(cli, &args, reporter.as_ref()), Commands::Manifest { file } => { - let ninja = generate_ninja(cli, reporter.as_ref())?; + let ninja = generate_ninja(cli, reporter.as_ref(), None)?; if process::is_stdout_path(file.as_path()) { process::write_ninja_stdout(&ninja)?; } else { let output_path = resolve_output_path(cli, file.as_path()); process::write_ninja_file(output_path.as_ref(), &ninja)?; } + reporter.report_complete(keys::STATUS_TOOL_MANIFEST); Ok(()) } Commands::Clean => handle_clean(cli, reporter.as_ref()), @@ -135,7 +138,7 @@ pub fn run(cli: &Cli) -> Result<()> { /// handle_build(&cli, &args, &SilentReporter).unwrap(); /// ``` fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> Result<()> { - let ninja = generate_ninja(cli, reporter)?; + let ninja = generate_ninja(cli, reporter, Some(keys::STATUS_TOOL_BUILD))?; let targets = BuildTargets::new(&args.targets); // Normalize the build file path and keep the temporary file alive for the @@ -154,11 +157,6 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R _tmp_file_guard = Some(tmp); } - report_pipeline_stage( - reporter, - PipelineStage::Execute, - Some(keys::STATUS_TOOL_BUILD), - ); let program = process::resolve_ninja_program(); run_ninja(program.as_path(), cli, build_path.as_ref(), &targets).with_context(|| { format!( @@ -191,12 +189,11 @@ fn handle_ninja_tool( subcommand = tool, "Preparing Ninja tool invocation" ); - let ninja = generate_ninja(cli, reporter)?; + let ninja = generate_ninja(cli, reporter, Some(tool_key))?; let tmp = process::create_temp_ninja_file(&ninja)?; let build_path = tmp.path(); - report_pipeline_stage(reporter, PipelineStage::Execute, Some(tool_key)); let program = process::resolve_ninja_program(); run_ninja_tool(program.as_path(), cli, build_path, tool).with_context(|| { format!( @@ -222,7 +219,8 @@ fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. /// -/// Reports pipeline stages 1 through 4 via the provided [`StatusReporter`]. +/// Reports manifest and graph/synthesis pipeline stages via the provided +/// [`StatusReporter`]. /// /// # Errors /// @@ -237,56 +235,18 @@ fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { /// let ninja = generate_ninja(&cli, &SilentReporter).expect("generate"); /// assert!(ninja.as_str().contains("rule")); /// ``` -fn generate_ninja(cli: &Cli, reporter: &dyn StatusReporter) -> Result { +fn generate_ninja( + cli: &Cli, + reporter: &dyn StatusReporter, + tool_key: Option<&'static str>, +) -> Result { let manifest_path = resolve_manifest_path(cli)?; + ensure_manifest_exists_or_error(cli, reporter, &manifest_path)?; - // Check for missing manifest and provide a helpful error with hint. - if !manifest_path.as_std_path().exists() { - // `resolve_manifest_path()` validates that `file_name()` is Some. - let manifest_name = manifest_path - .file_name() - .ok_or_else(|| { - anyhow!( - "{}", - localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME) - .with_arg("path", manifest_path.as_str()) - ) - })? - .to_owned(); - let directory = if cli.directory.is_some() { - let parent = manifest_path - .parent() - .map_or_else(|| manifest_path.as_str(), camino::Utf8Path::as_str); - localization::message(keys::RUNNER_MANIFEST_DIRECTORY) - .with_arg("directory", parent) - .to_string() - } else { - localization::message(keys::RUNNER_MANIFEST_CURRENT_DIRECTORY).to_string() - }; - let message = localization::message(keys::RUNNER_MANIFEST_NOT_FOUND) - .with_arg("manifest_name", manifest_name.as_str()) - .with_arg("directory", &directory); - return Err(RunnerError::ManifestNotFound { - manifest_name, - directory, - path: manifest_path.into_std_path_buf(), - message, - help: localization::message(keys::RUNNER_MANIFEST_NOT_FOUND_HELP), - } - .into()); - } - - report_pipeline_stage(reporter, PipelineStage::NetworkPolicy, None); let policy = cli .network_policy() .context(localization::message(keys::RUNNER_CONTEXT_NETWORK_POLICY))?; - - report_pipeline_stage(reporter, PipelineStage::ManifestLoad, None); - let manifest = manifest::from_path_with_policy(manifest_path.as_std_path(), policy) - .with_context(|| { - localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) - .with_arg("path", manifest_path.as_str()) - })?; + let manifest = load_manifest_with_stage_reporting(&manifest_path, policy, reporter)?; if tracing::enabled!(tracing::Level::DEBUG) { let ast_json = serde_json::to_string_pretty(&manifest).context(localization::message( keys::RUNNER_CONTEXT_SERIALISE_MANIFEST, @@ -294,16 +254,93 @@ fn generate_ninja(cli: &Cli, reporter: &dyn StatusReporter) -> Result Result<()> { + if manifest_path.as_std_path().exists() { + return Ok(()); + } + + report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); + // `resolve_manifest_path()` validates that `file_name()` is Some. + let manifest_name = manifest_path + .file_name() + .ok_or_else(|| { + anyhow!( + "{}", + localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME) + .with_arg("path", manifest_path.as_str()) + ) + })? + .to_owned(); + let directory = if cli.directory.is_some() { + let parent = manifest_path + .parent() + .map_or_else(|| manifest_path.as_str(), camino::Utf8Path::as_str); + localization::message(keys::RUNNER_MANIFEST_DIRECTORY) + .with_arg("directory", parent) + .to_string() + } else { + localization::message(keys::RUNNER_MANIFEST_CURRENT_DIRECTORY).to_string() + }; + let message = localization::message(keys::RUNNER_MANIFEST_NOT_FOUND) + .with_arg("manifest_name", manifest_name.as_str()) + .with_arg("directory", &directory); + Err(RunnerError::ManifestNotFound { + manifest_name, + directory, + path: manifest_path.to_path_buf().into_std_path_buf(), + message, + help: localization::message(keys::RUNNER_MANIFEST_NOT_FOUND_HELP), + } + .into()) +} + +fn load_manifest_with_stage_reporting( + manifest_path: &Utf8PathBuf, + policy: crate::stdlib::NetworkPolicy, + reporter: &dyn StatusReporter, +) -> Result { + manifest::from_path_with_policy_and_stage_callback( + manifest_path.as_std_path(), + policy, + |stage| match stage { + manifest::ManifestLoadStage::ManifestIngestion => { + report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); + } + manifest::ManifestLoadStage::InitialYamlParsing => { + report_pipeline_stage(reporter, PipelineStage::InitialYamlParsing, None); + } + manifest::ManifestLoadStage::TemplateExpansion => { + report_pipeline_stage(reporter, PipelineStage::TemplateExpansion, None); + } + manifest::ManifestLoadStage::FinalRendering => { + report_pipeline_stage(reporter, PipelineStage::FinalRendering, None); + } + }, + ) + .with_context(|| { + localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) + .with_arg("path", manifest_path.as_str()) + }) +} + /// Determine the manifest path respecting the CLI's directory option. /// /// # Errors diff --git a/src/status.rs b/src/status.rs index ae588e8e..541d44d3 100644 --- a/src/status.rs +++ b/src/status.rs @@ -1,60 +1,60 @@ //! Pipeline status reporting for accessible and standard output modes. //! -//! This module provides a [`StatusReporter`] trait and concrete -//! implementations that emit progress feedback during Netsuke's build -//! pipeline. [`AccessibleReporter`] writes static, labelled status lines -//! to stderr suitable for screen readers and dumb terminals. -//! [`SilentReporter`] emits nothing, serving as the default until future -//! animated progress indicators are added. +//! This module provides a [`StatusReporter`] trait plus concrete reporters for +//! both accessibility-first textual output and standard terminal progress +//! output. Standard mode uses `indicatif::MultiProgress` to keep stage summaries +//! persistent while the pipeline advances. use crate::localization::{self, keys}; +use indicatif::{MultiProgress, ProgressBar, ProgressDrawTarget, ProgressStyle}; use std::io::{self, Write}; +use std::sync::Mutex; + +fn stage_label(current: u32, total: u32, description: &str) -> String { + localization::message(keys::STATUS_STAGE_LABEL) + .with_arg("current", current.to_string()) + .with_arg("total", total.to_string()) + .with_arg("description", description) + .to_string() +} + +fn stage_summary(state_key: &'static str, current: u32, total: u32, description: &str) -> String { + let state = localization::message(state_key).to_string(); + let label = stage_label(current, total, description); + localization::message(keys::STATUS_STAGE_SUMMARY) + .with_arg("state", state) + .with_arg("label", label) + .to_string() +} /// Report pipeline progress to the user. -/// -/// Implementations decide how (or whether) to present stage transitions -/// and completion to the user. The trait is object-safe so callers can -/// dispatch dynamically based on the resolved output mode. pub trait StatusReporter { - /// Emit a status line for the given pipeline stage. + /// Emit a status update for the given pipeline stage. fn report_stage(&self, current: u32, total: u32, description: &str); /// Emit a completion message after a successful pipeline run. - /// - /// The `tool_key` is a Fluent message key identifying the tool name - /// (e.g., [`keys::STATUS_TOOL_BUILD`]). fn report_complete(&self, tool_key: &'static str); } /// Accessible reporter: writes static, labelled lines to stderr. -/// -/// Each line follows the pattern `Stage N/M: Description`, using -/// localized messages from the Fluent resource bundle. pub struct AccessibleReporter; impl StatusReporter for AccessibleReporter { fn report_stage(&self, current: u32, total: u32, description: &str) { - let message = localization::message(keys::STATUS_STAGE_LABEL) - .with_arg("current", current.to_string()) - .with_arg("total", total.to_string()) - .with_arg("description", description); - // Intentionally discard the write result: a failed status line - // should not abort the build pipeline. + let message = stage_label(current, total, description); + // Intentionally discard the write result: status output failures should + // not abort the build pipeline. drop(writeln!(io::stderr(), "{message}")); } fn report_complete(&self, tool_key: &'static str) { let tool = localization::message(tool_key); let message = localization::message(keys::STATUS_COMPLETE).with_arg("tool", tool); - // Intentionally discard the write result (see above). drop(writeln!(io::stderr(), "{message}")); } } /// Silent reporter: emits nothing. -/// -/// Used in standard output mode until future work (roadmap 3.9) adds -/// animated progress indicators via `indicatif`. pub struct SilentReporter; impl StatusReporter for SilentReporter { @@ -62,37 +62,175 @@ impl StatusReporter for SilentReporter { fn report_complete(&self, _tool_key: &'static str) {} } -/// Enumerates the known pipeline stages in the order they are reported. -/// -/// Keeping stage indices and descriptions centralized here avoids -/// hard-coded literals at call sites and ensures [`PIPELINE_STAGE_COUNT`] -/// stays consistent with the stages that are reported. -#[derive(Copy, Clone, Debug)] +#[derive(Debug)] +struct IndicatifState { + progress: MultiProgress, + bars: Vec, + descriptions: Vec, + running_index: Option, + completed: bool, +} + +/// Standard reporter backed by `indicatif::MultiProgress`. +pub struct IndicatifReporter { + state: Mutex, +} + +impl IndicatifReporter { + /// Construct an `indicatif` reporter with one persistent line per stage. + #[must_use] + pub fn new() -> Self { + let progress = MultiProgress::with_draw_target(ProgressDrawTarget::stderr_with_hz(12)); + progress.set_move_cursor(false); + let style = ProgressStyle::with_template("{msg}") + .unwrap_or_else(|_| ProgressStyle::default_spinner()); + + let mut bars = Vec::with_capacity(PipelineStage::ALL.len()); + let mut descriptions = Vec::with_capacity(PipelineStage::ALL.len()); + for stage in PipelineStage::ALL { + let description = stage.description(None); + let current = stage.index(); + let bar = progress.add(ProgressBar::new(1)); + bar.set_style(style.clone()); + bar.set_message(stage_summary( + keys::STATUS_STATE_PENDING, + current, + PIPELINE_STAGE_COUNT, + &description, + )); + bars.push(bar); + descriptions.push(description); + } + + Self { + state: Mutex::new(IndicatifState { + progress, + bars, + descriptions, + running_index: None, + completed: false, + }), + } + } + + fn set_stage_state( + state: &mut IndicatifState, + index: usize, + status_key: &'static str, + finish_line: bool, + ) { + let Ok(current) = u32::try_from(index + 1) else { + return; + }; + let description = state + .descriptions + .get(index) + .cloned() + .unwrap_or_else(String::new); + let message = stage_summary(status_key, current, PIPELINE_STAGE_COUNT, &description); + if let Some(bar) = state.bars.get(index) { + if finish_line { + bar.finish_with_message(message); + } else { + bar.set_message(message); + } + } + } +} + +impl Default for IndicatifReporter { + fn default() -> Self { + Self::new() + } +} + +impl Drop for IndicatifReporter { + fn drop(&mut self) { + let mut state = self + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if state.completed { + return; + } + if let Some(index) = state.running_index.take() { + Self::set_stage_state(&mut state, index, keys::STATUS_STATE_FAILED, true); + } + let _ = &state.progress; + } +} + +impl StatusReporter for IndicatifReporter { + fn report_stage(&self, current: u32, _total: u32, description: &str) { + let Ok(index) = usize::try_from(current.saturating_sub(1)) else { + return; + }; + + let mut state = self + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if index >= state.bars.len() { + return; + } + + let Some(existing_description) = state.descriptions.get_mut(index) else { + return; + }; + description.clone_into(existing_description); + if let Some(previous) = state.running_index + && previous != index + { + Self::set_stage_state(&mut state, previous, keys::STATUS_STATE_DONE, true); + } + + Self::set_stage_state(&mut state, index, keys::STATUS_STATE_RUNNING, false); + state.running_index = Some(index); + } + + fn report_complete(&self, tool_key: &'static str) { + let mut state = self + .state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + if let Some(index) = state.running_index.take() { + Self::set_stage_state(&mut state, index, keys::STATUS_STATE_DONE, true); + } + state.completed = true; + let _ = &state.progress; + + let tool = localization::message(tool_key); + let message = localization::message(keys::STATUS_COMPLETE).with_arg("tool", tool); + drop(writeln!(io::stderr(), "{message}")); + } +} + +/// Enumerates the known pipeline stages in reporting order. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum PipelineStage { - /// Stage 1: configuring the network policy. - NetworkPolicy = 1, - /// Stage 2: loading the manifest. - ManifestLoad = 2, - /// Stage 3: building the dependency graph. - BuildGraph = 3, - /// Stage 4: generating the Ninja file. - GenerateNinja = 4, - /// Stage 5: executing the build. - Execute = 5, + /// Stage 1: read the manifest from disk. + ManifestIngestion = 1, + /// Stage 2: parse raw YAML into an intermediate tree. + InitialYamlParsing = 2, + /// Stage 3: expand `foreach` and `when` template directives. + TemplateExpansion = 3, + /// Stage 4: deserialize and render manifest values. + FinalRendering = 4, + /// Stage 5: build and validate the dependency graph. + IrGenerationValidation = 5, + /// Stage 6: synthesize Ninja and execute the selected tool. + NinjaSynthesisAndExecution = 6, } impl PipelineStage { /// All pipeline stages in reporting order. - /// - /// Adding a new variant to [`PipelineStage`] without updating this - /// array will cause [`PIPELINE_STAGE_COUNT`] to drift, so keep them - /// in sync. - const ALL: [Self; 5] = [ - Self::NetworkPolicy, - Self::ManifestLoad, - Self::BuildGraph, - Self::GenerateNinja, - Self::Execute, + pub const ALL: [Self; 6] = [ + Self::ManifestIngestion, + Self::InitialYamlParsing, + Self::TemplateExpansion, + Self::FinalRendering, + Self::IrGenerationValidation, + Self::NinjaSynthesisAndExecution, ]; /// 1-based index of this stage within the pipeline. @@ -101,57 +239,56 @@ impl PipelineStage { self as u32 } + /// Convert a 1-based stage index into a [`PipelineStage`]. + #[must_use] + pub const fn from_index(index: u32) -> Option { + match index { + 1 => Some(Self::ManifestIngestion), + 2 => Some(Self::InitialYamlParsing), + 3 => Some(Self::TemplateExpansion), + 4 => Some(Self::FinalRendering), + 5 => Some(Self::IrGenerationValidation), + 6 => Some(Self::NinjaSynthesisAndExecution), + _ => None, + } + } + /// Localized description of this stage. - /// - /// For [`PipelineStage::Execute`], the description includes a - /// `{$tool}` placeholder resolved from the provided `tool_key`. Pass - /// [`None`] for non-Execute stages. #[must_use] pub fn description(self, tool_key: Option<&'static str>) -> String { match self { - Self::NetworkPolicy => { - localization::message(keys::STATUS_STAGE_NETWORK_POLICY).to_string() + Self::ManifestIngestion => { + localization::message(keys::STATUS_STAGE_MANIFEST_INGESTION).to_string() } - Self::ManifestLoad => { - localization::message(keys::STATUS_STAGE_MANIFEST_LOAD).to_string() + Self::InitialYamlParsing => { + localization::message(keys::STATUS_STAGE_INITIAL_YAML_PARSING).to_string() } - Self::BuildGraph => localization::message(keys::STATUS_STAGE_BUILD_GRAPH).to_string(), - Self::GenerateNinja => { - localization::message(keys::STATUS_STAGE_GENERATE_NINJA).to_string() + Self::TemplateExpansion => { + localization::message(keys::STATUS_STAGE_TEMPLATE_EXPANSION).to_string() } - Self::Execute => { - let tool = - tool_key.map_or_else(String::new, |k| localization::message(k).to_string()); - localization::message(keys::STATUS_STAGE_EXECUTE) - .with_arg("tool", tool) - .to_string() + Self::FinalRendering => { + localization::message(keys::STATUS_STAGE_FINAL_RENDERING).to_string() } + Self::IrGenerationValidation => { + localization::message(keys::STATUS_STAGE_IR_GENERATION_VALIDATION).to_string() + } + Self::NinjaSynthesisAndExecution => tool_key.map_or_else( + || localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS).to_string(), + |tool_message_key| { + let tool = localization::message(tool_message_key).to_string(); + localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS_EXECUTE) + .with_arg("tool", tool) + .to_string() + }, + ), } } } /// The total number of pipeline stages reported during a build. -/// -/// Derived from `PipelineStage::ALL` so that changes to the enum -/// cannot desynchronize the reported total. -pub const PIPELINE_STAGE_COUNT: u32 = { - // SAFETY: the array length is a small compile-time constant. - #[expect( - clippy::cast_possible_truncation, - reason = "PipelineStage::ALL.len() is a small compile-time constant" - )] - { - PipelineStage::ALL.len() as u32 - } -}; +pub const PIPELINE_STAGE_COUNT: u32 = 6; /// Report a pipeline stage via a [`StatusReporter`]. -/// -/// Centralizes the use of [`PIPELINE_STAGE_COUNT`] so call sites do not -/// need to know the numeric indices or total stage count. Pass a -/// `tool_key` for the [`PipelineStage::Execute`] stage to produce a -/// tool-specific description (e.g., "Executing clean"); other stages -/// ignore it. pub fn report_pipeline_stage( reporter: &dyn StatusReporter, stage: PipelineStage, @@ -163,3 +300,33 @@ pub fn report_pipeline_stage( &stage.description(tool_key), ); } + +#[cfg(test)] +mod tests { + use super::*; + use rstest::rstest; + + #[test] + fn pipeline_stage_count_matches_stage_array() { + let stage_count = u32::try_from(PipelineStage::ALL.len()).unwrap_or(0); + assert_eq!(PIPELINE_STAGE_COUNT, stage_count); + } + + #[rstest] + #[case(PipelineStage::ManifestIngestion, 1)] + #[case(PipelineStage::InitialYamlParsing, 2)] + #[case(PipelineStage::TemplateExpansion, 3)] + #[case(PipelineStage::FinalRendering, 4)] + #[case(PipelineStage::IrGenerationValidation, 5)] + #[case(PipelineStage::NinjaSynthesisAndExecution, 6)] + fn stage_index_round_trips(#[case] stage: PipelineStage, #[case] expected: u32) { + assert_eq!(stage.index(), expected); + assert_eq!(PipelineStage::from_index(expected), Some(stage)); + } + + #[test] + fn invalid_stage_index_returns_none() { + assert_eq!(PipelineStage::from_index(0), None); + assert_eq!(PipelineStage::from_index(7), None); + } +} diff --git a/tests/bdd/steps/manifest_command.rs b/tests/bdd/steps/manifest_command.rs index 3b015ae1..5f0f8ddd 100644 --- a/tests/bdd/steps/manifest_command.rs +++ b/tests/bdd/steps/manifest_command.rs @@ -47,6 +47,21 @@ fn assert_output_contains( assert_slot_contains(output, fragment.as_str(), &output_type.to_string()) } +fn assert_output_not_contains( + output: &Slot, + output_type: OutputType, + fragment: &OutputFragment, +) -> Result<()> { + let value = output + .get() + .with_context(|| format!("{output_type} output should be captured"))?; + ensure!( + !value.contains(fragment.as_str()), + "expected {output_type} to omit '{fragment}', but it was present in:\n{value}", + ); + Ok(()) +} + fn assert_file_existence(world: &TestWorld, name: &FileName, should_exist: bool) -> Result<()> { let temp_path = get_temp_path(world)?; let path = temp_path.join(name.as_str()); @@ -162,6 +177,16 @@ fn stderr_should_contain(world: &TestWorld, fragment: OutputFragment) -> Result< assert_output_contains(&world.command_stderr, OutputType::Stderr, &fragment) } +#[then("stdout should not contain {fragment:string}")] +fn stdout_should_not_contain(world: &TestWorld, fragment: OutputFragment) -> Result<()> { + assert_output_not_contains(&world.command_stdout, OutputType::Stdout, &fragment) +} + +#[then("stderr should not contain {fragment:string}")] +fn stderr_should_not_contain(world: &TestWorld, fragment: OutputFragment) -> Result<()> { + assert_output_not_contains(&world.command_stderr, OutputType::Stderr, &fragment) +} + #[then("the file {name:string} should exist")] fn file_should_exist(world: &TestWorld, name: FileName) -> Result<()> { assert_file_existence(world, &name, true) diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index db0389a2..dc1a33b0 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -32,23 +32,27 @@ fn cli_merge_layers_respects_precedence_and_appends_lists( .context("defaults should be an object")?; defaults_object.insert("jobs".to_owned(), json!(1)); defaults_object.insert("fetch_allow_scheme".to_owned(), json!(["https"])); + defaults_object.insert("progress".to_owned(), json!(true)); composer.push_defaults(defaults); composer.push_file( json!({ "file": "Configfile", "jobs": 2, "fetch_allow_scheme": ["http"], - "locale": "en-US" + "locale": "en-US", + "progress": false }), None, ); composer.push_environment(json!({ "jobs": 3, - "fetch_allow_scheme": ["ftp"] + "fetch_allow_scheme": ["ftp"], + "progress": true })); composer.push_cli(json!({ "jobs": 4, "fetch_allow_scheme": ["git"], + "progress": false, "verbose": true })); let merged = Cli::merge_from_layers(composer.layers())?; @@ -61,6 +65,10 @@ fn cli_merge_layers_respects_precedence_and_appends_lists( merged.fetch_allow_scheme == vec!["https", "http", "ftp", "git"], "list values should append in layer order", ); + ensure!( + merged.progress == Some(false), + "CLI layer should override progress setting", + ); ensure!( merged.locale.as_deref() == Some("en-US"), "file layer should populate locale when CLI does not override", @@ -81,6 +89,7 @@ fetch_allow_scheme = ["https"] verbose = true fetch_default_deny = true locale = "es-ES" +progress = false "#; fs::write(&config_path, config).context("write netsuke.toml")?; @@ -119,6 +128,10 @@ locale = "es-ES" merged.locale.as_deref() == Some("es-ES"), "config locale should be retained when CLI does not override", ); + ensure!( + merged.progress == Some(false), + "config progress should apply when CLI and env do not override", + ); Ok(()) } diff --git a/tests/cli_tests/parsing.rs b/tests/cli_tests/parsing.rs index a58c0205..44d36df5 100644 --- a/tests/cli_tests/parsing.rs +++ b/tests/cli_tests/parsing.rs @@ -19,6 +19,7 @@ struct CliCase { allow_host: Vec<&'static str>, block_host: Vec<&'static str>, default_deny: bool, + progress: Option, expected_cmd: Commands, } @@ -35,6 +36,7 @@ impl Default for CliCase { allow_host: Vec::new(), block_host: Vec::new(), default_deny: false, + progress: None, expected_cmd: Commands::Build(BuildArgs { emit: None, targets: Vec::new(), @@ -61,6 +63,11 @@ impl Default for CliCase { verbose: true, ..CliCase::default() })] +#[case(CliCase { + argv: vec!["netsuke", "--progress", "false"], + progress: Some(false), + ..CliCase::default() +})] #[case(CliCase { argv: vec!["netsuke", "--locale", "es-ES"], locale: Some("es-ES"), @@ -155,6 +162,10 @@ fn parse_cli(#[case] case: CliCase) -> Result<()> { cli.fetch_default_deny == case.default_deny, "default-deny flag should match input", ); + ensure!( + cli.progress == case.progress, + "progress flag should match input", + ); let command = cli.command.context("command should be set")?; ensure!( command == case.expected_cmd, diff --git a/tests/features/progress_output.feature b/tests/features/progress_output.feature new file mode 100644 index 00000000..15e01e1e --- /dev/null +++ b/tests/features/progress_output.feature @@ -0,0 +1,37 @@ +Feature: Progress output + + Scenario: Standard mode shows six stage summaries + Given a minimal Netsuke workspace + When netsuke is run with arguments "--progress true manifest -" + Then the command should succeed + And stderr should contain "Stage 1/6" + And stderr should contain "Stage 6/6" + And stderr should contain "Manifest complete." + + Scenario: Stage summaries localize to Spanish + Given a minimal Netsuke workspace + When netsuke is run with arguments "--locale es-ES --progress true manifest -" + Then the command should succeed + And stderr should contain "Etapa 1/6" + And stderr should contain "Etapa 6/6" + And stderr should contain "Manifiesto completo." + + Scenario: Accessible mode still uses static stage labels + Given a minimal Netsuke workspace + When netsuke is run with arguments "--accessible true --progress true manifest -" + Then the command should succeed + And stderr should contain "Stage 1/6" + And stderr should contain "Stage 6/6" + + Scenario: Progress output can be disabled in standard mode + Given a minimal Netsuke workspace + When netsuke is run with arguments "--progress false manifest -" + Then the command should succeed + And stderr should not contain "Stage 1/6" + + Scenario: Failed runs mark the active stage as failed + Given an empty workspace + When netsuke is run with arguments "--progress true" + Then the command should fail + And stderr should contain "Stage 1/6" + And stderr should contain "failed" diff --git a/tests/localization_tests.rs b/tests/localization_tests.rs index 7d2949bf..8922eda1 100644 --- a/tests/localization_tests.rs +++ b/tests/localization_tests.rs @@ -141,3 +141,37 @@ fn variable_interpolation_works_correctly() -> Result<()> { ); Ok(()) } + +#[rstest] +#[case("en-US", "Stage 2/6", "pending")] +#[case("es-ES", "Etapa 2/6", "pendiente")] +fn progress_stage_messages_resolve( + #[case] locale: &str, + #[case] expected_label: &str, + #[case] expected_state: &str, +) -> Result<()> { + let _guards = localizer_guards(locale)?; + + let label = localization::message(keys::STATUS_STAGE_LABEL) + .with_arg("current", 2) + .with_arg("total", 6) + .with_arg( + "description", + localization::message(keys::STATUS_STAGE_TEMPLATE_EXPANSION), + ) + .to_string(); + let summary = localization::message(keys::STATUS_STAGE_SUMMARY) + .with_arg("state", localization::message(keys::STATUS_STATE_PENDING)) + .with_arg("label", &label) + .to_string(); + + ensure!( + label.contains(expected_label), + "expected stage label for locale {locale} to contain {expected_label:?}, got: {label}" + ); + ensure!( + summary.contains(expected_state), + "expected summary state for locale {locale} to contain {expected_state:?}, got: {summary}" + ); + Ok(()) +} From 59f7ace7eb035ef5144f4b6b1ad5ef659740ff2d Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Feb 2026 18:32:00 +0000 Subject: [PATCH 3/9] feat(progress): add six-stage progress reporting with indicatif multi-progress - Integrated indicatif::MultiProgress for standard terminal progress reporting. - Implemented six-stage progress model with localized stage/state labels. - Added fallback for non-TTY stderr: emit localized summary lines deterministically. - Configured via OrthoConfig layering with support for CLI args & env variables. - Expanded rstest and rstest-bdd coverage for reporter logic, stages, and failure paths. - Updated documentation and marked roadmap item 3.9.1 as done. - Improved testing helpers to normalize Fluent bidi isolate characters. This enables persistent interactive progress summaries on TTYs while providing stable, testable summary lines in CI and non-interactive environments. Co-authored-by: devboxerhub[bot] --- ...-9-1-integrate-indicatif-multi-progress.md | 32 +++++++++++++++---- docs/netsuke-design.md | 2 ++ docs/users-guide.md | 2 ++ src/status.rs | 6 ++++ tests/bdd/helpers/assertions.rs | 20 ++++++++++-- tests/bdd/steps/manifest_command.rs | 11 ++++++- tests/features/progress_output.feature | 8 ++--- tests/localization_tests.rs | 13 ++++++-- 8 files changed, 78 insertions(+), 16 deletions(-) diff --git a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md b/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md index 8275862f..e9a37b77 100644 --- a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md +++ b/docs/execplans/3-9-1-integrate-indicatif-multi-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: IN PROGRESS +Status: DONE No `PLANS.md` file exists in this repository. @@ -98,13 +98,13 @@ Observable success: - [x] Implement six-stage progress model and `indicatif::MultiProgress` standard reporter. - [x] Add OrthoConfig-backed progress configuration and localized help. -- [ ] Add unit tests (`rstest`) for reporter logic, stage mapping, and config +- [x] Add unit tests (`rstest`) for reporter logic, stage mapping, and config precedence. -- [ ] Add behavioural tests (`rstest-bdd` v0.5.0) for standard/accessible, +- [x] Add behavioural tests (`rstest-bdd` v0.5.0) for standard/accessible, localized, and failure paths. -- [ ] Update `docs/users-guide.md` and design document decisions. -- [ ] Mark roadmap item 3.9.1 done. -- [ ] Run and pass required quality gates: +- [x] Update `docs/users-guide.md` and design document decisions. +- [x] Mark roadmap item 3.9.1 done. +- [x] Run and pass required quality gates: `make check-fmt`, `make lint`, `make test`. ## Surprises & Discoveries @@ -120,6 +120,9 @@ Observable success: - The runtime `manifest` command path needed an explicit completion call after synthesis; otherwise an in-progress stage was finalized as failed in the new reporter drop path. +- `indicatif` automatically hides progress drawing when stderr is not a TTY, + so non-interactive tests and CI required a deterministic summary-line + fallback in the standard reporter. ## Decision Log @@ -140,9 +143,24 @@ Observable success: provides deterministic behavioural testing in non-TTY environments. Date/Author: 2026-02-12 / Codex. +- Decision: keep `indicatif::MultiProgress` as the canonical standard-mode + reporter, but emit localized summary lines when its draw target is hidden. + Rationale: preserves interactive persistent summaries while keeping CI/log + runs observable and testable in non-TTY environments. Date/Author: 2026-02-12 + / Codex. + ## Outcomes & Retrospective -Pending implementation. +Implemented and validated. + +- Added six-stage progress reporting with localized stage/state labels and + standard-mode `indicatif::MultiProgress`. +- Added OrthoConfig layering for `progress` (`--progress`, + `NETSUKE_PROGRESS`, and config file support) and localized help text. +- Added deterministic non-TTY fallback summaries for standard mode. +- Added/updated `rstest` unit coverage and `rstest-bdd` behavioural coverage + for happy and unhappy paths. +- Updated user and design documentation and marked roadmap 3.9.1 as done. ## Context and orientation diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index 26306f91..ae37d449 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2043,6 +2043,8 @@ 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), diff --git a/docs/users-guide.md b/docs/users-guide.md index 6bf26e19..f21179bf 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -605,6 +605,8 @@ Build complete. In standard mode, Netsuke uses persistent progress summaries for the six-stage pipeline. The summaries are localized and remain visible as stages complete. +When stderr is not a TTY (for example, in CI logs), Netsuke emits deterministic +summary lines instead of animated redraws. Progress summaries are enabled by default in standard mode. You can force them on or off through layered configuration: diff --git a/src/status.rs b/src/status.rs index 541d44d3..33a31392 100644 --- a/src/status.rs +++ b/src/status.rs @@ -69,6 +69,7 @@ struct IndicatifState { descriptions: Vec, running_index: Option, completed: bool, + is_hidden: bool, } /// Standard reporter backed by `indicatif::MultiProgress`. @@ -104,6 +105,7 @@ impl IndicatifReporter { Self { state: Mutex::new(IndicatifState { + is_hidden: progress.is_hidden(), progress, bars, descriptions, @@ -128,6 +130,10 @@ impl IndicatifReporter { .cloned() .unwrap_or_else(String::new); let message = stage_summary(status_key, current, PIPELINE_STAGE_COUNT, &description); + if state.is_hidden { + drop(writeln!(io::stderr(), "{message}")); + return; + } if let Some(bar) = state.bars.get(index) { if finish_line { bar.finish_with_message(message); diff --git a/tests/bdd/helpers/assertions.rs b/tests/bdd/helpers/assertions.rs index c94203c2..15b7b07a 100644 --- a/tests/bdd/helpers/assertions.rs +++ b/tests/bdd/helpers/assertions.rs @@ -7,6 +7,18 @@ use anyhow::{Context, Result, ensure}; use rstest_bdd::Slot; +/// Remove Fluent bidi isolate characters used around placeables. +/// +/// Fluent inserts these markers (`\u{2068}` and `\u{2069}`) to preserve +/// directionality when interpolating values. They are invisible to users but +/// can make plain substring assertions fail. +#[must_use] +fn normalize_fluent_isolates(text: &str) -> String { + text.chars() + .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') + .collect() +} + /// Assert that optional content contains an expected fragment. /// /// This unifies the pattern from `ninja.rs::assert_content_contains`. @@ -32,8 +44,10 @@ pub fn assert_optional_contains( content_label: &str, ) -> Result<()> { let text = content.context(format!("{content_label} should be available"))?; + let normalized_text = normalize_fluent_isolates(&text); + let normalized_fragment = normalize_fluent_isolates(fragment); ensure!( - text.contains(fragment), + normalized_text.contains(&normalized_fragment), "{content_label} should contain '{fragment}'" ); Ok(()) @@ -66,8 +80,10 @@ pub fn assert_slot_contains( let content = slot .get() .with_context(|| format!("no {content_label} captured"))?; + let normalized_content = normalize_fluent_isolates(&content); + let normalized_fragment = normalize_fluent_isolates(fragment); ensure!( - content.contains(fragment), + normalized_content.contains(&normalized_fragment), "expected {content_label} to contain '{fragment}', got '{content}'" ); Ok(()) diff --git a/tests/bdd/steps/manifest_command.rs b/tests/bdd/steps/manifest_command.rs index 5f0f8ddd..dfc3da36 100644 --- a/tests/bdd/steps/manifest_command.rs +++ b/tests/bdd/steps/manifest_command.rs @@ -29,6 +29,13 @@ impl fmt::Display for OutputType { } } +#[must_use] +fn normalize_fluent_isolates(text: &str) -> String { + text.chars() + .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') + .collect() +} + // --------------------------------------------------------------------------- // Helper functions // --------------------------------------------------------------------------- @@ -55,8 +62,10 @@ fn assert_output_not_contains( let value = output .get() .with_context(|| format!("{output_type} output should be captured"))?; + let normalized_value = normalize_fluent_isolates(&value); + let normalized_fragment = normalize_fluent_isolates(fragment.as_str()); ensure!( - !value.contains(fragment.as_str()), + !normalized_value.contains(&normalized_fragment), "expected {output_type} to omit '{fragment}', but it was present in:\n{value}", ); Ok(()) diff --git a/tests/features/progress_output.feature b/tests/features/progress_output.feature index 15e01e1e..6c01b2ea 100644 --- a/tests/features/progress_output.feature +++ b/tests/features/progress_output.feature @@ -2,7 +2,7 @@ Feature: Progress output Scenario: Standard mode shows six stage summaries Given a minimal Netsuke workspace - When netsuke is run with arguments "--progress true manifest -" + When netsuke is run with arguments "--accessible false --progress true manifest -" Then the command should succeed And stderr should contain "Stage 1/6" And stderr should contain "Stage 6/6" @@ -10,7 +10,7 @@ Feature: Progress output Scenario: Stage summaries localize to Spanish Given a minimal Netsuke workspace - When netsuke is run with arguments "--locale es-ES --progress true manifest -" + When netsuke is run with arguments "--accessible false --locale es-ES --progress true manifest -" Then the command should succeed And stderr should contain "Etapa 1/6" And stderr should contain "Etapa 6/6" @@ -25,13 +25,13 @@ Feature: Progress output Scenario: Progress output can be disabled in standard mode Given a minimal Netsuke workspace - When netsuke is run with arguments "--progress false manifest -" + When netsuke is run with arguments "--accessible false --progress false manifest -" Then the command should succeed And stderr should not contain "Stage 1/6" Scenario: Failed runs mark the active stage as failed Given an empty workspace - When netsuke is run with arguments "--progress true" + When netsuke is run with arguments "--accessible false --progress true" Then the command should fail And stderr should contain "Stage 1/6" And stderr should contain "failed" diff --git a/tests/localization_tests.rs b/tests/localization_tests.rs index 8922eda1..c6fa35f2 100644 --- a/tests/localization_tests.rs +++ b/tests/localization_tests.rs @@ -44,6 +44,13 @@ fn which_message(command: &str) -> String { .to_string() } +#[must_use] +fn normalize_fluent_isolates(text: &str) -> String { + text.chars() + .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') + .collect() +} + #[rstest] #[case("es-ES", "no encontrado")] #[case("fr-FR", "not found")] @@ -164,13 +171,15 @@ fn progress_stage_messages_resolve( .with_arg("state", localization::message(keys::STATUS_STATE_PENDING)) .with_arg("label", &label) .to_string(); + let normalized_label = normalize_fluent_isolates(&label); + let normalized_summary = normalize_fluent_isolates(&summary); ensure!( - label.contains(expected_label), + normalized_label.contains(expected_label), "expected stage label for locale {locale} to contain {expected_label:?}, got: {label}" ); ensure!( - summary.contains(expected_state), + normalized_summary.contains(expected_state), "expected summary state for locale {locale} to contain {expected_state:?}, got: {summary}" ); Ok(()) From 98bbe2942eb15c47696c35ea8accd82fc5f4e91a Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 13 Feb 2026 01:55:59 +0000 Subject: [PATCH 4/9] refactor(status): introduce typed wrappers for pipeline stage reporting - Replaced raw integers and string keys related to pipeline stages with strongly typed structs: `StageNumber`, `StageDescription`, and `LocalizationKey`. - Moved `PipelineStage` enum and related reporting functions to a new module `status_pipeline.rs`. - Updated all `StatusReporter` trait methods and implementations to use the new types instead of raw primitives. - Improved error handling and validation around stage numbers. - Added tests for the new stage modeling types in `status_tests.rs`. - Enhanced localization usage consistency by wrapping keys in `LocalizationKey` type. - Changed calls in `runner` to pass `LocalizationKey` instances instead of plain static string keys. This refactor enhances type safety, readability, and maintainability of the pipeline status reporting subsystem. Co-authored-by: devboxerhub[bot] --- src/runner/mod.rs | 32 ++-- src/status.rs | 336 ++++++++++++++++++++--------------------- src/status_pipeline.rs | 103 +++++++++++++ src/status_tests.rs | 60 ++++++++ 4 files changed, 354 insertions(+), 177 deletions(-) create mode 100644 src/status_pipeline.rs create mode 100644 src/status_tests.rs diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 2dafc5d9..56ffe621 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -12,8 +12,8 @@ use crate::cli::{BuildArgs, Cli, Commands}; use crate::localization::{self, keys}; use crate::output_mode::{self, OutputMode}; use crate::status::{ - AccessibleReporter, IndicatifReporter, PipelineStage, SilentReporter, StatusReporter, - report_pipeline_stage, + AccessibleReporter, IndicatifReporter, LocalizationKey, PipelineStage, SilentReporter, + StatusReporter, report_pipeline_stage, }; use crate::{ir::BuildGraph, manifest, ninja_gen}; use anyhow::{Context, Result, anyhow}; @@ -114,7 +114,7 @@ pub fn run(cli: &Cli) -> Result<()> { let output_path = resolve_output_path(cli, file.as_path()); process::write_ninja_file(output_path.as_ref(), &ninja)?; } - reporter.report_complete(keys::STATUS_TOOL_MANIFEST); + reporter.report_complete(LocalizationKey::new(keys::STATUS_TOOL_MANIFEST)); Ok(()) } Commands::Clean => handle_clean(cli, reporter.as_ref()), @@ -138,7 +138,11 @@ pub fn run(cli: &Cli) -> Result<()> { /// handle_build(&cli, &args, &SilentReporter).unwrap(); /// ``` fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> Result<()> { - let ninja = generate_ninja(cli, reporter, Some(keys::STATUS_TOOL_BUILD))?; + let ninja = generate_ninja( + cli, + reporter, + Some(LocalizationKey::new(keys::STATUS_TOOL_BUILD)), + )?; let targets = BuildTargets::new(&args.targets); // Normalize the build file path and keep the temporary file alive for the @@ -165,7 +169,7 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R build_path.display() ) })?; - reporter.report_complete(keys::STATUS_TOOL_BUILD); + reporter.report_complete(LocalizationKey::new(keys::STATUS_TOOL_BUILD)); Ok(()) } @@ -181,7 +185,7 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R fn handle_ninja_tool( cli: &Cli, tool: &str, - tool_key: &'static str, + tool_key: LocalizationKey, reporter: &dyn StatusReporter, ) -> Result<()> { info!( @@ -209,12 +213,22 @@ fn handle_ninja_tool( /// 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, reporter) + handle_ninja_tool( + cli, + "clean", + LocalizationKey::new(keys::STATUS_TOOL_CLEAN), + reporter, + ) } /// 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, reporter) + handle_ninja_tool( + cli, + "graph", + LocalizationKey::new(keys::STATUS_TOOL_GRAPH), + reporter, + ) } /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. @@ -238,7 +252,7 @@ fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { fn generate_ninja( cli: &Cli, reporter: &dyn StatusReporter, - tool_key: Option<&'static str>, + tool_key: Option, ) -> Result { let manifest_path = resolve_manifest_path(cli)?; ensure_manifest_exists_or_error(cli, reporter, &manifest_path)?; diff --git a/src/status.rs b/src/status.rs index 33a31392..e6c2e7dc 100644 --- a/src/status.rs +++ b/src/status.rs @@ -1,25 +1,111 @@ //! Pipeline status reporting for accessible and standard output modes. -//! -//! This module provides a [`StatusReporter`] trait plus concrete reporters for -//! both accessibility-first textual output and standard terminal progress -//! output. Standard mode uses `indicatif::MultiProgress` to keep stage summaries -//! persistent while the pipeline advances. use crate::localization::{self, keys}; use indicatif::{MultiProgress, ProgressBar, ProgressDrawTarget, ProgressStyle}; use std::io::{self, Write}; use std::sync::Mutex; +use thiserror::Error; -fn stage_label(current: u32, total: u32, description: &str) -> String { +/// Total count of user-visible pipeline stages. +pub const PIPELINE_STAGE_COUNT: u32 = 6; + +/// Validation error when constructing a [`StageNumber`]. +#[derive(Debug, Error, Copy, Clone, PartialEq, Eq)] +pub enum StageNumberError { + /// Provided value is not within the inclusive stage range. + #[error("stage number {0} is out of range (expected 1..={PIPELINE_STAGE_COUNT})")] + OutOfRange(u32), +} + +/// Validated stage index in the range `1..=PIPELINE_STAGE_COUNT`. +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct StageNumber(u32); + +impl StageNumber { + /// Build a validated stage number. + /// + /// # Errors + /// + /// Returns [`StageNumberError::OutOfRange`] when `value` is not between 1 + /// and [`PIPELINE_STAGE_COUNT`] inclusive. + #[must_use = "validate and use the constructed stage number"] + pub const fn new(value: u32) -> Result { + if value >= 1 && value <= PIPELINE_STAGE_COUNT { + Ok(Self(value)) + } else { + Err(StageNumberError::OutOfRange(value)) + } + } + + /// Return the raw numeric stage index. + #[must_use] + pub const fn get(self) -> u32 { + self.0 + } +} + +/// Localized description text for a pipeline stage. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct StageDescription(String); + +impl StageDescription { + /// Wrap a localized stage description. + #[must_use] + pub const fn new(value: String) -> Self { + Self(value) + } + + /// Borrow the wrapped description. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl From for StageDescription { + fn from(value: String) -> Self { + Self::new(value) + } +} + +/// Fluent localization key used for status output messages. +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub struct LocalizationKey(&'static str); + +impl LocalizationKey { + /// Wrap a static Fluent key string. + #[must_use] + pub const fn new(value: &'static str) -> Self { + Self(value) + } + + /// Return the wrapped Fluent key. + #[must_use] + pub const fn as_str(self) -> &'static str { + self.0 + } +} + +const PIPELINE_STAGE_TOTAL: StageNumber = StageNumber(PIPELINE_STAGE_COUNT); +#[path = "status_pipeline.rs"] +mod pipeline; +pub use pipeline::{PipelineStage, report_pipeline_stage}; + +fn stage_label(current: StageNumber, total: StageNumber, description: &StageDescription) -> String { localization::message(keys::STATUS_STAGE_LABEL) - .with_arg("current", current.to_string()) - .with_arg("total", total.to_string()) - .with_arg("description", description) + .with_arg("current", current.get().to_string()) + .with_arg("total", total.get().to_string()) + .with_arg("description", description.as_str()) .to_string() } -fn stage_summary(state_key: &'static str, current: u32, total: u32, description: &str) -> String { - let state = localization::message(state_key).to_string(); +fn stage_summary( + state_key: LocalizationKey, + current: StageNumber, + total: StageNumber, + description: &StageDescription, +) -> String { + let state = localization::message(state_key.as_str()).to_string(); let label = stage_label(current, total, description); localization::message(keys::STATUS_STAGE_SUMMARY) .with_arg("state", state) @@ -27,46 +113,54 @@ fn stage_summary(state_key: &'static str, current: u32, total: u32, description: .to_string() } -/// Report pipeline progress to the user. +/// Reports pipeline stage transitions and completion. pub trait StatusReporter { - /// Emit a status update for the given pipeline stage. - fn report_stage(&self, current: u32, total: u32, description: &str); - - /// Emit a completion message after a successful pipeline run. - fn report_complete(&self, tool_key: &'static str); + /// Emit a stage update. + fn report_stage(&self, current: StageNumber, total: StageNumber, description: StageDescription); + /// Emit a final completion message. + fn report_complete(&self, tool_key: LocalizationKey); } -/// Accessible reporter: writes static, labelled lines to stderr. +/// Accessible reporter that emits static lines. pub struct AccessibleReporter; impl StatusReporter for AccessibleReporter { - fn report_stage(&self, current: u32, total: u32, description: &str) { - let message = stage_label(current, total, description); - // Intentionally discard the write result: status output failures should - // not abort the build pipeline. + fn report_stage( + &self, + current: StageNumber, + total: StageNumber, + description: StageDescription, + ) { + let message = stage_label(current, total, &description); drop(writeln!(io::stderr(), "{message}")); } - fn report_complete(&self, tool_key: &'static str) { - let tool = localization::message(tool_key); + fn report_complete(&self, tool_key: LocalizationKey) { + let tool = localization::message(tool_key.as_str()); let message = localization::message(keys::STATUS_COMPLETE).with_arg("tool", tool); drop(writeln!(io::stderr(), "{message}")); } } -/// Silent reporter: emits nothing. +/// Reporter that suppresses status output. pub struct SilentReporter; impl StatusReporter for SilentReporter { - fn report_stage(&self, _current: u32, _total: u32, _description: &str) {} - fn report_complete(&self, _tool_key: &'static str) {} + fn report_stage( + &self, + _current: StageNumber, + _total: StageNumber, + _description: StageDescription, + ) { + } + fn report_complete(&self, _tool_key: LocalizationKey) {} } #[derive(Debug)] struct IndicatifState { progress: MultiProgress, bars: Vec, - descriptions: Vec, + descriptions: Vec, running_index: Option, completed: bool, is_hidden: bool, @@ -78,7 +172,7 @@ pub struct IndicatifReporter { } impl IndicatifReporter { - /// Construct an `indicatif` reporter with one persistent line per stage. + /// Build a multi-progress reporter with one line per pipeline stage. #[must_use] pub fn new() -> Self { let progress = MultiProgress::with_draw_target(ProgressDrawTarget::stderr_with_hz(12)); @@ -94,9 +188,9 @@ impl IndicatifReporter { let bar = progress.add(ProgressBar::new(1)); bar.set_style(style.clone()); bar.set_message(stage_summary( - keys::STATUS_STATE_PENDING, + LocalizationKey::new(keys::STATUS_STATE_PENDING), current, - PIPELINE_STAGE_COUNT, + PIPELINE_STAGE_TOTAL, &description, )); bars.push(bar); @@ -118,18 +212,21 @@ impl IndicatifReporter { fn set_stage_state( state: &mut IndicatifState, index: usize, - status_key: &'static str, + status_key: LocalizationKey, finish_line: bool, ) { - let Ok(current) = u32::try_from(index + 1) else { + let Ok(current_raw) = u32::try_from(index + 1) else { + return; + }; + let Ok(current) = StageNumber::new(current_raw) else { return; }; let description = state .descriptions .get(index) .cloned() - .unwrap_or_else(String::new); - let message = stage_summary(status_key, current, PIPELINE_STAGE_COUNT, &description); + .unwrap_or_else(|| StageDescription::new(String::new())); + let message = stage_summary(status_key, current, PIPELINE_STAGE_TOTAL, &description); if state.is_hidden { drop(writeln!(io::stderr(), "{message}")); return; @@ -160,15 +257,25 @@ impl Drop for IndicatifReporter { return; } if let Some(index) = state.running_index.take() { - Self::set_stage_state(&mut state, index, keys::STATUS_STATE_FAILED, true); + Self::set_stage_state( + &mut state, + index, + LocalizationKey::new(keys::STATUS_STATE_FAILED), + true, + ); } let _ = &state.progress; } } impl StatusReporter for IndicatifReporter { - fn report_stage(&self, current: u32, _total: u32, description: &str) { - let Ok(index) = usize::try_from(current.saturating_sub(1)) else { + fn report_stage( + &self, + current: StageNumber, + _total: StageNumber, + description: StageDescription, + ) { + let Ok(index) = usize::try_from(current.get().saturating_sub(1)) else { return; }; @@ -183,156 +290,49 @@ impl StatusReporter for IndicatifReporter { let Some(existing_description) = state.descriptions.get_mut(index) else { return; }; - description.clone_into(existing_description); + *existing_description = description; if let Some(previous) = state.running_index && previous != index { - Self::set_stage_state(&mut state, previous, keys::STATUS_STATE_DONE, true); + Self::set_stage_state( + &mut state, + previous, + LocalizationKey::new(keys::STATUS_STATE_DONE), + true, + ); } - Self::set_stage_state(&mut state, index, keys::STATUS_STATE_RUNNING, false); + Self::set_stage_state( + &mut state, + index, + LocalizationKey::new(keys::STATUS_STATE_RUNNING), + false, + ); state.running_index = Some(index); } - fn report_complete(&self, tool_key: &'static str) { + fn report_complete(&self, tool_key: LocalizationKey) { let mut state = self .state .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); if let Some(index) = state.running_index.take() { - Self::set_stage_state(&mut state, index, keys::STATUS_STATE_DONE, true); + Self::set_stage_state( + &mut state, + index, + LocalizationKey::new(keys::STATUS_STATE_DONE), + true, + ); } state.completed = true; let _ = &state.progress; - let tool = localization::message(tool_key); + let tool = localization::message(tool_key.as_str()); let message = localization::message(keys::STATUS_COMPLETE).with_arg("tool", tool); drop(writeln!(io::stderr(), "{message}")); } } -/// Enumerates the known pipeline stages in reporting order. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub enum PipelineStage { - /// Stage 1: read the manifest from disk. - ManifestIngestion = 1, - /// Stage 2: parse raw YAML into an intermediate tree. - InitialYamlParsing = 2, - /// Stage 3: expand `foreach` and `when` template directives. - TemplateExpansion = 3, - /// Stage 4: deserialize and render manifest values. - FinalRendering = 4, - /// Stage 5: build and validate the dependency graph. - IrGenerationValidation = 5, - /// Stage 6: synthesize Ninja and execute the selected tool. - NinjaSynthesisAndExecution = 6, -} - -impl PipelineStage { - /// All pipeline stages in reporting order. - pub const ALL: [Self; 6] = [ - Self::ManifestIngestion, - Self::InitialYamlParsing, - Self::TemplateExpansion, - Self::FinalRendering, - Self::IrGenerationValidation, - Self::NinjaSynthesisAndExecution, - ]; - - /// 1-based index of this stage within the pipeline. - #[must_use] - pub const fn index(self) -> u32 { - self as u32 - } - - /// Convert a 1-based stage index into a [`PipelineStage`]. - #[must_use] - pub const fn from_index(index: u32) -> Option { - match index { - 1 => Some(Self::ManifestIngestion), - 2 => Some(Self::InitialYamlParsing), - 3 => Some(Self::TemplateExpansion), - 4 => Some(Self::FinalRendering), - 5 => Some(Self::IrGenerationValidation), - 6 => Some(Self::NinjaSynthesisAndExecution), - _ => None, - } - } - - /// Localized description of this stage. - #[must_use] - pub fn description(self, tool_key: Option<&'static str>) -> String { - match self { - Self::ManifestIngestion => { - localization::message(keys::STATUS_STAGE_MANIFEST_INGESTION).to_string() - } - Self::InitialYamlParsing => { - localization::message(keys::STATUS_STAGE_INITIAL_YAML_PARSING).to_string() - } - Self::TemplateExpansion => { - localization::message(keys::STATUS_STAGE_TEMPLATE_EXPANSION).to_string() - } - Self::FinalRendering => { - localization::message(keys::STATUS_STAGE_FINAL_RENDERING).to_string() - } - Self::IrGenerationValidation => { - localization::message(keys::STATUS_STAGE_IR_GENERATION_VALIDATION).to_string() - } - Self::NinjaSynthesisAndExecution => tool_key.map_or_else( - || localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS).to_string(), - |tool_message_key| { - let tool = localization::message(tool_message_key).to_string(); - localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS_EXECUTE) - .with_arg("tool", tool) - .to_string() - }, - ), - } - } -} - -/// The total number of pipeline stages reported during a build. -pub const PIPELINE_STAGE_COUNT: u32 = 6; - -/// Report a pipeline stage via a [`StatusReporter`]. -pub fn report_pipeline_stage( - reporter: &dyn StatusReporter, - stage: PipelineStage, - tool_key: Option<&'static str>, -) { - reporter.report_stage( - stage.index(), - PIPELINE_STAGE_COUNT, - &stage.description(tool_key), - ); -} - #[cfg(test)] -mod tests { - use super::*; - use rstest::rstest; - - #[test] - fn pipeline_stage_count_matches_stage_array() { - let stage_count = u32::try_from(PipelineStage::ALL.len()).unwrap_or(0); - assert_eq!(PIPELINE_STAGE_COUNT, stage_count); - } - - #[rstest] - #[case(PipelineStage::ManifestIngestion, 1)] - #[case(PipelineStage::InitialYamlParsing, 2)] - #[case(PipelineStage::TemplateExpansion, 3)] - #[case(PipelineStage::FinalRendering, 4)] - #[case(PipelineStage::IrGenerationValidation, 5)] - #[case(PipelineStage::NinjaSynthesisAndExecution, 6)] - fn stage_index_round_trips(#[case] stage: PipelineStage, #[case] expected: u32) { - assert_eq!(stage.index(), expected); - assert_eq!(PipelineStage::from_index(expected), Some(stage)); - } - - #[test] - fn invalid_stage_index_returns_none() { - assert_eq!(PipelineStage::from_index(0), None); - assert_eq!(PipelineStage::from_index(7), None); - } -} +#[path = "status_tests.rs"] +mod tests; diff --git a/src/status_pipeline.rs b/src/status_pipeline.rs new file mode 100644 index 00000000..74860f8d --- /dev/null +++ b/src/status_pipeline.rs @@ -0,0 +1,103 @@ +//! Pipeline stage model and stage reporting helper. + +use super::{LocalizationKey, PIPELINE_STAGE_TOTAL, StageDescription, StageNumber, StatusReporter}; +use crate::localization::{self, keys}; + +/// Enumerates pipeline stages in user-visible execution order. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub enum PipelineStage { + /// Stage 1: read manifest content from disk. + ManifestIngestion = 1, + /// Stage 2: parse the YAML document. + InitialYamlParsing = 2, + /// Stage 3: expand templated target directives. + TemplateExpansion = 3, + /// Stage 4: finalize manifest rendering. + FinalRendering = 4, + /// Stage 5: build and validate dependency IR. + IrGenerationValidation = 5, + /// Stage 6: synthesize Ninja and execute command intent. + NinjaSynthesisAndExecution = 6, +} + +impl PipelineStage { + /// All stages in pipeline order. + pub const ALL: [Self; 6] = [ + Self::ManifestIngestion, + Self::InitialYamlParsing, + Self::TemplateExpansion, + Self::FinalRendering, + Self::IrGenerationValidation, + Self::NinjaSynthesisAndExecution, + ]; + + /// Return the validated stage index for this variant. + #[must_use] + pub const fn index(self) -> StageNumber { + StageNumber(self as u32) + } + + /// Map a raw stage index to a stage enum variant. + #[must_use] + pub const fn from_index(index: u32) -> Option { + match index { + 1 => Some(Self::ManifestIngestion), + 2 => Some(Self::InitialYamlParsing), + 3 => Some(Self::TemplateExpansion), + 4 => Some(Self::FinalRendering), + 5 => Some(Self::IrGenerationValidation), + 6 => Some(Self::NinjaSynthesisAndExecution), + _ => None, + } + } + + /// Return the localized description for this stage. + #[must_use] + pub fn description(self, tool_key: Option) -> StageDescription { + match self { + Self::ManifestIngestion => StageDescription::new( + localization::message(keys::STATUS_STAGE_MANIFEST_INGESTION).to_string(), + ), + Self::InitialYamlParsing => StageDescription::new( + localization::message(keys::STATUS_STAGE_INITIAL_YAML_PARSING).to_string(), + ), + Self::TemplateExpansion => StageDescription::new( + localization::message(keys::STATUS_STAGE_TEMPLATE_EXPANSION).to_string(), + ), + Self::FinalRendering => StageDescription::new( + localization::message(keys::STATUS_STAGE_FINAL_RENDERING).to_string(), + ), + Self::IrGenerationValidation => StageDescription::new( + localization::message(keys::STATUS_STAGE_IR_GENERATION_VALIDATION).to_string(), + ), + Self::NinjaSynthesisAndExecution => tool_key.map_or_else( + || { + StageDescription::new( + localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS).to_string(), + ) + }, + |tool_message_key| { + let tool = localization::message(tool_message_key.as_str()).to_string(); + StageDescription::new( + localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS_EXECUTE) + .with_arg("tool", tool) + .to_string(), + ) + }, + ), + } + } +} + +/// Emit a localized status update for a concrete pipeline stage. +pub fn report_pipeline_stage( + reporter: &dyn StatusReporter, + stage: PipelineStage, + tool_key: Option, +) { + reporter.report_stage( + stage.index(), + PIPELINE_STAGE_TOTAL, + stage.description(tool_key), + ); +} diff --git a/src/status_tests.rs b/src/status_tests.rs new file mode 100644 index 00000000..a73731d3 --- /dev/null +++ b/src/status_tests.rs @@ -0,0 +1,60 @@ +//! Tests for status stage modelling and index conversions. + +use super::*; +use rstest::rstest; + +#[rstest] +#[case(1)] +#[case(PIPELINE_STAGE_COUNT)] +fn stage_number_accepts_in_range_values(#[case] value: u32) { + let stage = StageNumber::new(value).expect("in-range stage number should be valid"); + assert_eq!(stage.get(), value); +} + +#[rstest] +#[case(0)] +#[case(PIPELINE_STAGE_COUNT + 1)] +#[case(u32::MAX)] +fn stage_number_rejects_out_of_range_values(#[case] value: u32) { + let error = StageNumber::new(value).expect_err("out-of-range stage number should fail"); + assert_eq!(error, StageNumberError::OutOfRange(value)); +} + +#[test] +fn stage_description_round_trips_string_content() { + let description = StageDescription::new(String::from("rendering")); + assert_eq!(description.as_str(), "rendering"); + + let from_impl: StageDescription = String::from("building").into(); + assert_eq!(from_impl.as_str(), "building"); +} + +#[test] +fn localization_key_round_trips_static_key() { + let key = LocalizationKey::new(keys::STATUS_STATE_PENDING); + assert_eq!(key.as_str(), keys::STATUS_STATE_PENDING); +} + +#[test] +fn pipeline_stage_count_matches_stage_array() { + let stage_count = u32::try_from(PipelineStage::ALL.len()).unwrap_or(0); + assert_eq!(PIPELINE_STAGE_COUNT, stage_count); +} + +#[rstest] +#[case(PipelineStage::ManifestIngestion, 1)] +#[case(PipelineStage::InitialYamlParsing, 2)] +#[case(PipelineStage::TemplateExpansion, 3)] +#[case(PipelineStage::FinalRendering, 4)] +#[case(PipelineStage::IrGenerationValidation, 5)] +#[case(PipelineStage::NinjaSynthesisAndExecution, 6)] +fn stage_index_round_trips(#[case] stage: PipelineStage, #[case] expected: u32) { + assert_eq!(stage.index().get(), expected); + assert_eq!(PipelineStage::from_index(expected), Some(stage)); +} + +#[test] +fn invalid_stage_index_returns_none() { + assert_eq!(PipelineStage::from_index(0), None); + assert_eq!(PipelineStage::from_index(7), None); +} From d94e8afe55a11de77824d35f30e220c1d56b9c25 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 14 Feb 2026 23:28:18 +0000 Subject: [PATCH 5/9] refactor(status): simplify status reporting stage description handling Refactor status reporting interfaces and implementations to replace the StageDescription wrapper with plain &str or String parameters. This simplifies the pipeline stage reporting code, removes unnecessary wrapper types, and improves clarity. Impacts include: - Remove StageDescription struct and associated error type. - Change StatusReporter trait methods to accept &str for stage description. - Update implementations (AccessibleReporter, SilentReporter, IndicatifReporter) accordingly. - Adjust PipelineStage::description to return String directly. - Simplify related test code. This refactor retains previous behavior but cleans up types and improves interface consistency for pipeline stage status reporting. Co-authored-by: devboxerhub[bot] --- Cargo.lock | 51 ++++++-- Cargo.toml | 2 +- ...-9-1-integrate-indicatif-multi-progress.md | 30 +++-- docs/users-guide.md | 6 +- src/manifest/mod.rs | 29 ++--- src/manifest/tests/stages.rs | 109 ++++++++++++++-- src/runner/mod.rs | 26 +--- src/status.rs | 119 +++++------------- src/status_pipeline.rs | 78 +++++------- src/status_tests.rs | 55 +++----- test_support/src/fluent.rs | 24 ++++ test_support/src/lib.rs | 1 + tests/bdd/helpers/assertions.rs | 13 +- tests/bdd/steps/manifest_command.rs | 9 +- tests/localization_tests.rs | 8 +- 15 files changed, 276 insertions(+), 284 deletions(-) create mode 100644 test_support/src/fluent.rs diff --git a/Cargo.lock b/Cargo.lock index 748373aa..727c9106 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -343,10 +343,22 @@ dependencies = [ "encode_unicode", "libc", "once_cell", - "unicode-width 0.2.1", "windows-sys 0.59.0", ] +[[package]] +name = "console" +version = "0.16.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "03e45a4a8926227e4197636ba97a9fc9b00477e9f4bd711395687c5f0734bec4" +dependencies = [ + "encode_unicode", + "libc", + "once_cell", + "unicode-width 0.2.1", + "windows-sys 0.61.2", +] + [[package]] name = "convert_case" version = "0.4.0" @@ -1046,14 +1058,14 @@ dependencies = [ [[package]] name = "indicatif" -version = "0.17.11" +version = "0.18.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "183b3088984b400f4cfac3620d5e076c84da5364016b4f49473de574b2586235" +checksum = "25470f23803092da7d239834776d653104d551bc4d7eacaf31e6837854b8e9eb" dependencies = [ - "console", - "number_prefix", + "console 0.16.2", "portable-atomic", "unicode-width 0.2.1", + "unit-prefix", "web-time", ] @@ -1069,7 +1081,7 @@ version = "1.43.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "154934ea70c58054b556dd430b99a98c2a7ff5309ac9891597e339b5c28f4371" dependencies = [ - "console", + "console 0.15.11", "once_cell", "serde", "similar", @@ -1470,12 +1482,6 @@ dependencies = [ "autocfg", ] -[[package]] -name = "number_prefix" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "830b246a0e5f20af87141b25c173cd1b609bd7779a4617d6ec582abaf90870f3" - [[package]] name = "object" version = "0.36.7" @@ -2882,6 +2888,12 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4a1a07cc7db3810833284e8d372ccdc6da29741639ecc70c9ec107df0fa6154c" +[[package]] +name = "unit-prefix" +version = "0.5.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "81e544489bf3d8ef66c953931f56617f423cd4b5494be343d9b9d3dda037b9a3" + [[package]] name = "untrusted" version = "0.9.0" @@ -3087,6 +3099,12 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" +[[package]] +name = "windows-link" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" + [[package]] name = "windows-sys" version = "0.48.0" @@ -3114,6 +3132,15 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "windows-sys" +version = "0.61.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae137229bcbd6cdf0f7b80a31df61766145077ddf49416a728b02cb3921ff3fc" +dependencies = [ + "windows-link", +] + [[package]] name = "windows-targets" version = "0.48.5" diff --git a/Cargo.toml b/Cargo.toml index 7e8e56cb..080d7015 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,7 @@ cap-std = { version = "3.4.4", features = ["fs_utf8"] } camino = "1.2.0" semver = { version = "1", features = ["serde"] } anyhow = "1" -indicatif = "0.17.11" +indicatif = "0.18.4" thiserror = "1" miette = { version = "7.6.0", features = ["fancy"] } digest = "0.10" diff --git a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md b/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md index e9a37b77..ff366eb2 100644 --- a/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md +++ b/docs/execplans/3-9-1-integrate-indicatif-multi-progress.md @@ -36,7 +36,7 @@ Observable success: - Keep the pipeline model aligned with the six-stage build flow documented in `docs/netsuke-design.md`: Manifest Ingestion, Initial YAML Parsing, Template - Expansion, Deserialisation & Final Rendering, IR Generation & Validation, and + Expansion, Deserialization & Final Rendering, IR Generation & Validation, and Ninja Synthesis & Execution. - Preserve accessible mode guarantees from roadmap 3.8.1. - Integrate progress configuration with OrthoConfig conventions and localized @@ -92,7 +92,8 @@ Observable success: ## Progress - [x] 2026-02-12: Gathered context from roadmap, design docs, status/runner - modules, localization files, and existing BDD/unit test surfaces. + modules, localization files, and existing behaviour-driven + development (BDD)/unit test surfaces. - [x] 2026-02-12: Drafted this ExecPlan in `docs/execplans/3-9-1-integrate-indicatif-multi-progress.md`. - [x] Implement six-stage progress model and `indicatif::MultiProgress` @@ -115,8 +116,9 @@ Observable success: inside one function, so stage-level reporting needs extraction. - Standard mode currently uses `SilentReporter`; no `indicatif` dependency is present. -- No project-memory MCP resources were available in this environment during - planning, so repository docs were used as the authoritative source. +- No project-memory Model Context Protocol (MCP) resources were + available in this environment during planning, so repository docs were + used as the authoritative source. - The runtime `manifest` command path needed an explicit completion call after synthesis; otherwise an in-progress stage was finalized as failed in the new reporter drop path. @@ -333,17 +335,21 @@ Planned changes: Run all required gates with `tee` and `pipefail`: - set -o pipefail - make check-fmt 2>&1 | tee /tmp/3-9-1-check-fmt.log - make lint 2>&1 | tee /tmp/3-9-1-lint.log - make test 2>&1 | tee /tmp/3-9-1-test.log +```sh +set -o pipefail +make check-fmt 2>&1 | tee /tmp/3-9-1-check-fmt.log +make lint 2>&1 | tee /tmp/3-9-1-lint.log +make test 2>&1 | tee /tmp/3-9-1-test.log +``` Documentation gates after doc updates: - set -o pipefail - make fmt 2>&1 | tee /tmp/3-9-1-fmt.log - make markdownlint 2>&1 | tee /tmp/3-9-1-markdownlint.log - make nixie 2>&1 | tee /tmp/3-9-1-nixie.log +```sh +set -o pipefail +make fmt 2>&1 | tee /tmp/3-9-1-fmt.log +make markdownlint 2>&1 | tee /tmp/3-9-1-markdownlint.log +make nixie 2>&1 | tee /tmp/3-9-1-nixie.log +``` Record concise evidence in commit/PR notes: diff --git a/docs/users-guide.md b/docs/users-guide.md index f21179bf..d607fde7 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -605,10 +605,10 @@ Build complete. In standard mode, Netsuke uses persistent progress summaries for the six-stage pipeline. The summaries are localized and remain visible as stages complete. -When stderr is not a TTY (for example, in CI logs), Netsuke emits deterministic -summary lines instead of animated redraws. +When stderr is not a TTY (for example, in continuous integration (CI) logs), +Netsuke emits deterministic summary lines instead of animated redraws. -Progress summaries are enabled by default in standard mode. You can force them +Progress summaries are enabled by default in standard mode. They can be forced on or off through layered configuration: - CLI flag: `--progress true` or `--progress false` diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index 091b91b6..1ed5d375 100644 --- a/src/manifest/mod.rs +++ b/src/manifest/mod.rs @@ -113,20 +113,8 @@ fn from_str_named( yaml: &str, name: &ManifestName, stdlib_config: Option, + on_stage: &mut dyn FnMut(ManifestLoadStage), ) -> Result { - let mut no_op = |_stage| {}; - from_str_named_with_stage_callback(yaml, name, stdlib_config, &mut no_op) -} - -fn from_str_named_with_stage_callback( - yaml: &str, - name: &ManifestName, - stdlib_config: Option, - on_stage: &mut F, -) -> Result -where - F: FnMut(ManifestLoadStage), -{ on_stage(ManifestLoadStage::InitialYamlParsing); let mut doc: ManifestValue = serde_saphyr::from_str(yaml).map_err(|e| ManifestError::Parse { @@ -186,7 +174,7 @@ where /// /// Returns an error if YAML parsing or Jinja evaluation fails. pub fn from_str(yaml: &str) -> Result { - from_str_named(yaml, &ManifestName::new("Netsukefile"), None) + from_str_named(yaml, &ManifestName::new("Netsukefile"), None, &mut |_| {}) } /// Load a [`NetsukeManifest`] from the given file path. @@ -219,7 +207,7 @@ pub fn from_path_with_policy( path: impl AsRef, policy: NetworkPolicy, ) -> Result { - from_path_with_policy_and_stage_callback(path, policy, |_stage| {}) + from_path_with_policy_and_stage_callback(path, policy, |_| {}) } /// Load a [`NetsukeManifest`] from the given file path using an explicit @@ -230,14 +218,11 @@ pub fn from_path_with_policy( /// # Errors /// /// Returns an error if the file cannot be read or the YAML fails to parse. -pub fn from_path_with_policy_and_stage_callback( +pub fn from_path_with_policy_and_stage_callback( path: impl AsRef, policy: NetworkPolicy, - mut on_stage: F, -) -> Result -where - F: FnMut(ManifestLoadStage), -{ + mut on_stage: impl FnMut(ManifestLoadStage), +) -> Result { on_stage(ManifestLoadStage::ManifestIngestion); let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref).with_context(|| { @@ -246,7 +231,7 @@ where })?; let name = ManifestName::new(path_ref.display().to_string()); let config = stdlib_config_for_manifest(path_ref, policy)?; - from_str_named_with_stage_callback(&data, &name, Some(config), &mut on_stage) + from_str_named(&data, &name, Some(config), &mut on_stage) } #[cfg(test)] diff --git a/src/manifest/tests/stages.rs b/src/manifest/tests/stages.rs index edaa0f3c..10391760 100644 --- a/src/manifest/tests/stages.rs +++ b/src/manifest/tests/stages.rs @@ -4,24 +4,32 @@ use crate::manifest::{self, ManifestLoadStage}; use crate::stdlib::NetworkPolicy; use anyhow::{Context, Result, ensure}; -fn minimal_manifest() -> &'static str { - concat!( - "netsuke_version: \"1.0.0\"\n", - "rules:\n", - " - name: touch\n", - " command: \"touch $out\"\n", - "targets:\n", - " - name: out.txt\n", - " rule: touch\n", +/// Create a temporary workspace with a manifest containing the given command. +fn temp_manifest(command: &str) -> Result<(tempfile::TempDir, std::path::PathBuf)> { + let dir = tempfile::tempdir().context("create temp workspace for stage test")?; + let manifest_path = dir.path().join("Netsukefile"); + std::fs::write( + &manifest_path, + format!( + concat!( + "netsuke_version: \"1.0.0\"\n", + "rules:\n", + " - name: touch\n", + " command: \"{command}\"\n", + "targets:\n", + " - name: out.txt\n", + " rule: touch\n", + ), + command = command, + ), ) + .with_context(|| format!("write {}", manifest_path.display()))?; + Ok((dir, manifest_path)) } #[test] fn stage_callback_reports_expected_order_for_valid_manifest() -> Result<()> { - let dir = tempfile::tempdir().context("create temp workspace for stage test")?; - let manifest_path = dir.path().join("Netsukefile"); - std::fs::write(&manifest_path, minimal_manifest()) - .with_context(|| format!("write {}", manifest_path.display()))?; + let (_dir, manifest_path) = temp_manifest("touch $out")?; let mut stages = Vec::new(); let manifest = manifest::from_path_with_policy_and_stage_callback( @@ -71,3 +79,78 @@ fn stage_callback_stops_after_parse_failure() -> Result<()> { ); Ok(()) } + +/// Template expansion failures should report stages up to and including +/// `TemplateExpansion`. +#[test] +fn stage_callback_stops_after_template_expansion_failure() -> Result<()> { + let dir = tempfile::tempdir().context("create temp workspace for template test")?; + let manifest_path = dir.path().join("Netsukefile"); + // Reference a non-existent Jinja variable to trigger expansion failure. + std::fs::write( + &manifest_path, + concat!( + "netsuke_version: \"1.0.0\"\n", + "rules:\n", + " - name: echo\n", + " command: \"echo hello\"\n", + "targets:\n", + " - name: out.txt\n", + " rule: echo\n", + " when: \"{{ nonexistent_variable }}\"\n", + ), + ) + .with_context(|| format!("write {}", manifest_path.display()))?; + + let mut stages = Vec::new(); + let result = manifest::from_path_with_policy_and_stage_callback( + &manifest_path, + NetworkPolicy::default(), + |stage| stages.push(stage), + ); + ensure!(result.is_err(), "template expansion should fail"); + ensure!( + stages.contains(&ManifestLoadStage::TemplateExpansion), + "stages should include TemplateExpansion: {stages:?}" + ); + ensure!( + !stages.contains(&ManifestLoadStage::FinalRendering), + "stages should not include FinalRendering: {stages:?}" + ); + Ok(()) +} + +/// Final rendering failures should report stages up to and including +/// `FinalRendering`. +#[test] +fn stage_callback_stops_after_final_rendering_failure() -> Result<()> { + let dir = tempfile::tempdir().context("create temp workspace for rendering test")?; + let manifest_path = dir.path().join("Netsukefile"); + // Missing required `netsuke_version` causes a deserialization error during + // FinalRendering. + std::fs::write( + &manifest_path, + concat!( + "rules:\n", + " - name: echo\n", + " command: \"echo hello\"\n", + "targets:\n", + " - name: out.txt\n", + " rule: echo\n", + ), + ) + .with_context(|| format!("write {}", manifest_path.display()))?; + + let mut stages = Vec::new(); + let result = manifest::from_path_with_policy_and_stage_callback( + &manifest_path, + NetworkPolicy::default(), + |stage| stages.push(stage), + ); + ensure!(result.is_err(), "final rendering should fail"); + ensure!( + stages.contains(&ManifestLoadStage::FinalRendering), + "stages should include FinalRendering: {stages:?}" + ); + Ok(()) +} diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 56ffe621..ed9a8aa7 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -114,7 +114,7 @@ pub fn run(cli: &Cli) -> Result<()> { let output_path = resolve_output_path(cli, file.as_path()); process::write_ninja_file(output_path.as_ref(), &ninja)?; } - reporter.report_complete(LocalizationKey::new(keys::STATUS_TOOL_MANIFEST)); + reporter.report_complete(keys::STATUS_TOOL_MANIFEST.into()); Ok(()) } Commands::Clean => handle_clean(cli, reporter.as_ref()), @@ -138,11 +138,7 @@ pub fn run(cli: &Cli) -> Result<()> { /// handle_build(&cli, &args, &SilentReporter).unwrap(); /// ``` fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> Result<()> { - let ninja = generate_ninja( - cli, - reporter, - Some(LocalizationKey::new(keys::STATUS_TOOL_BUILD)), - )?; + let ninja = generate_ninja(cli, reporter, Some(keys::STATUS_TOOL_BUILD.into()))?; let targets = BuildTargets::new(&args.targets); // Normalize the build file path and keep the temporary file alive for the @@ -169,7 +165,7 @@ fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> R build_path.display() ) })?; - reporter.report_complete(LocalizationKey::new(keys::STATUS_TOOL_BUILD)); + reporter.report_complete(keys::STATUS_TOOL_BUILD.into()); Ok(()) } @@ -213,22 +209,12 @@ fn handle_ninja_tool( /// Remove build artefacts by invoking `ninja -t clean`. fn handle_clean(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { - handle_ninja_tool( - cli, - "clean", - LocalizationKey::new(keys::STATUS_TOOL_CLEAN), - reporter, - ) + handle_ninja_tool(cli, "clean", keys::STATUS_TOOL_CLEAN.into(), reporter) } /// Display build dependency graph by invoking `ninja -t graph`. fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { - handle_ninja_tool( - cli, - "graph", - LocalizationKey::new(keys::STATUS_TOOL_GRAPH), - reporter, - ) + handle_ninja_tool(cli, "graph", keys::STATUS_TOOL_GRAPH.into(), reporter) } /// Generate the Ninja manifest string from the Netsuke manifest referenced by `cli`. @@ -246,7 +232,7 @@ fn handle_graph(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> { /// use netsuke::runner::generate_ninja; /// use netsuke::status::SilentReporter; /// let cli = Cli::default(); -/// let ninja = generate_ninja(&cli, &SilentReporter).expect("generate"); +/// let ninja = generate_ninja(&cli, &SilentReporter, None).expect("generate"); /// assert!(ninja.as_str().contains("rule")); /// ``` fn generate_ninja( diff --git a/src/status.rs b/src/status.rs index e6c2e7dc..c06f331d 100644 --- a/src/status.rs +++ b/src/status.rs @@ -4,37 +4,23 @@ use crate::localization::{self, keys}; use indicatif::{MultiProgress, ProgressBar, ProgressDrawTarget, ProgressStyle}; use std::io::{self, Write}; use std::sync::Mutex; -use thiserror::Error; /// Total count of user-visible pipeline stages. pub const PIPELINE_STAGE_COUNT: u32 = 6; -/// Validation error when constructing a [`StageNumber`]. -#[derive(Debug, Error, Copy, Clone, PartialEq, Eq)] -pub enum StageNumberError { - /// Provided value is not within the inclusive stage range. - #[error("stage number {0} is out of range (expected 1..={PIPELINE_STAGE_COUNT})")] - OutOfRange(u32), -} - -/// Validated stage index in the range `1..=PIPELINE_STAGE_COUNT`. +/// Thin wrapper for a 1-based stage index. +/// +/// All current call sites derive stage numbers from the `PipelineStage` enum +/// whose discriminants are statically known to be in range, so validation at +/// construction time is unnecessary. #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct StageNumber(u32); impl StageNumber { - /// Build a validated stage number. - /// - /// # Errors - /// - /// Returns [`StageNumberError::OutOfRange`] when `value` is not between 1 - /// and [`PIPELINE_STAGE_COUNT`] inclusive. - #[must_use = "validate and use the constructed stage number"] - pub const fn new(value: u32) -> Result { - if value >= 1 && value <= PIPELINE_STAGE_COUNT { - Ok(Self(value)) - } else { - Err(StageNumberError::OutOfRange(value)) - } + /// Build a stage number without runtime validation. + #[must_use] + pub const fn new_unchecked(value: u32) -> Self { + Self(value) } /// Return the raw numeric stage index. @@ -44,30 +30,6 @@ impl StageNumber { } } -/// Localized description text for a pipeline stage. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct StageDescription(String); - -impl StageDescription { - /// Wrap a localized stage description. - #[must_use] - pub const fn new(value: String) -> Self { - Self(value) - } - - /// Borrow the wrapped description. - #[must_use] - pub fn as_str(&self) -> &str { - &self.0 - } -} - -impl From for StageDescription { - fn from(value: String) -> Self { - Self::new(value) - } -} - /// Fluent localization key used for status output messages. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct LocalizationKey(&'static str); @@ -86,16 +48,22 @@ impl LocalizationKey { } } +impl From<&'static str> for LocalizationKey { + fn from(s: &'static str) -> Self { + Self::new(s) + } +} + const PIPELINE_STAGE_TOTAL: StageNumber = StageNumber(PIPELINE_STAGE_COUNT); #[path = "status_pipeline.rs"] mod pipeline; pub use pipeline::{PipelineStage, report_pipeline_stage}; -fn stage_label(current: StageNumber, total: StageNumber, description: &StageDescription) -> String { +fn stage_label(current: StageNumber, total: StageNumber, description: &str) -> String { localization::message(keys::STATUS_STAGE_LABEL) .with_arg("current", current.get().to_string()) .with_arg("total", total.get().to_string()) - .with_arg("description", description.as_str()) + .with_arg("description", description) .to_string() } @@ -103,7 +71,7 @@ fn stage_summary( state_key: LocalizationKey, current: StageNumber, total: StageNumber, - description: &StageDescription, + description: &str, ) -> String { let state = localization::message(state_key.as_str()).to_string(); let label = stage_label(current, total, description); @@ -116,7 +84,7 @@ fn stage_summary( /// Reports pipeline stage transitions and completion. pub trait StatusReporter { /// Emit a stage update. - fn report_stage(&self, current: StageNumber, total: StageNumber, description: StageDescription); + fn report_stage(&self, current: StageNumber, total: StageNumber, description: &str); /// Emit a final completion message. fn report_complete(&self, tool_key: LocalizationKey); } @@ -125,13 +93,8 @@ pub trait StatusReporter { pub struct AccessibleReporter; impl StatusReporter for AccessibleReporter { - fn report_stage( - &self, - current: StageNumber, - total: StageNumber, - description: StageDescription, - ) { - let message = stage_label(current, total, &description); + fn report_stage(&self, current: StageNumber, total: StageNumber, description: &str) { + let message = stage_label(current, total, description); drop(writeln!(io::stderr(), "{message}")); } @@ -146,13 +109,7 @@ impl StatusReporter for AccessibleReporter { pub struct SilentReporter; impl StatusReporter for SilentReporter { - fn report_stage( - &self, - _current: StageNumber, - _total: StageNumber, - _description: StageDescription, - ) { - } + fn report_stage(&self, _current: StageNumber, _total: StageNumber, _description: &str) {} fn report_complete(&self, _tool_key: LocalizationKey) {} } @@ -160,7 +117,7 @@ impl StatusReporter for SilentReporter { struct IndicatifState { progress: MultiProgress, bars: Vec, - descriptions: Vec, + descriptions: Vec, running_index: Option, completed: bool, is_hidden: bool, @@ -215,18 +172,12 @@ impl IndicatifReporter { status_key: LocalizationKey, finish_line: bool, ) { - let Ok(current_raw) = u32::try_from(index + 1) else { - return; - }; - let Ok(current) = StageNumber::new(current_raw) else { + let Some(current_raw) = u32::try_from(index + 1).ok() else { return; }; - let description = state - .descriptions - .get(index) - .cloned() - .unwrap_or_else(|| StageDescription::new(String::new())); - let message = stage_summary(status_key, current, PIPELINE_STAGE_TOTAL, &description); + let current = StageNumber::new_unchecked(current_raw); + let description = state.descriptions.get(index).map_or("", String::as_str); + let message = stage_summary(status_key, current, PIPELINE_STAGE_TOTAL, description); if state.is_hidden { drop(writeln!(io::stderr(), "{message}")); return; @@ -264,17 +215,13 @@ impl Drop for IndicatifReporter { true, ); } + // Keep `state` alive so the MultiProgress flush completes before drop. let _ = &state.progress; } } impl StatusReporter for IndicatifReporter { - fn report_stage( - &self, - current: StageNumber, - _total: StageNumber, - description: StageDescription, - ) { + fn report_stage(&self, current: StageNumber, _total: StageNumber, description: &str) { let Ok(index) = usize::try_from(current.get().saturating_sub(1)) else { return; }; @@ -287,10 +234,9 @@ impl StatusReporter for IndicatifReporter { return; } - let Some(existing_description) = state.descriptions.get_mut(index) else { - return; - }; - *existing_description = description; + if let Some(existing) = state.descriptions.get_mut(index) { + description.clone_into(existing); + } if let Some(previous) = state.running_index && previous != index { @@ -325,6 +271,7 @@ impl StatusReporter for IndicatifReporter { ); } state.completed = true; + // Keep `state` alive so the MultiProgress flush completes before drop. let _ = &state.progress; let tool = localization::message(tool_key.as_str()); diff --git a/src/status_pipeline.rs b/src/status_pipeline.rs index 74860f8d..b82c946a 100644 --- a/src/status_pipeline.rs +++ b/src/status_pipeline.rs @@ -1,6 +1,6 @@ //! Pipeline stage model and stage reporting helper. -use super::{LocalizationKey, PIPELINE_STAGE_TOTAL, StageDescription, StageNumber, StatusReporter}; +use super::{LocalizationKey, PIPELINE_STAGE_TOTAL, StageNumber, StatusReporter}; use crate::localization::{self, keys}; /// Enumerates pipeline stages in user-visible execution order. @@ -34,70 +34,54 @@ impl PipelineStage { /// Return the validated stage index for this variant. #[must_use] pub const fn index(self) -> StageNumber { - StageNumber(self as u32) - } - - /// Map a raw stage index to a stage enum variant. - #[must_use] - pub const fn from_index(index: u32) -> Option { - match index { - 1 => Some(Self::ManifestIngestion), - 2 => Some(Self::InitialYamlParsing), - 3 => Some(Self::TemplateExpansion), - 4 => Some(Self::FinalRendering), - 5 => Some(Self::IrGenerationValidation), - 6 => Some(Self::NinjaSynthesisAndExecution), - _ => None, - } + StageNumber::new_unchecked(self as u32) } /// Return the localized description for this stage. #[must_use] - pub fn description(self, tool_key: Option) -> StageDescription { + pub fn description(self, tool_key: Option) -> String { match self { - Self::ManifestIngestion => StageDescription::new( - localization::message(keys::STATUS_STAGE_MANIFEST_INGESTION).to_string(), - ), - Self::InitialYamlParsing => StageDescription::new( - localization::message(keys::STATUS_STAGE_INITIAL_YAML_PARSING).to_string(), - ), - Self::TemplateExpansion => StageDescription::new( - localization::message(keys::STATUS_STAGE_TEMPLATE_EXPANSION).to_string(), - ), - Self::FinalRendering => StageDescription::new( - localization::message(keys::STATUS_STAGE_FINAL_RENDERING).to_string(), - ), - Self::IrGenerationValidation => StageDescription::new( - localization::message(keys::STATUS_STAGE_IR_GENERATION_VALIDATION).to_string(), - ), + Self::ManifestIngestion => { + localization::message(keys::STATUS_STAGE_MANIFEST_INGESTION).to_string() + } + Self::InitialYamlParsing => { + localization::message(keys::STATUS_STAGE_INITIAL_YAML_PARSING).to_string() + } + Self::TemplateExpansion => { + localization::message(keys::STATUS_STAGE_TEMPLATE_EXPANSION).to_string() + } + Self::FinalRendering => { + localization::message(keys::STATUS_STAGE_FINAL_RENDERING).to_string() + } + Self::IrGenerationValidation => { + localization::message(keys::STATUS_STAGE_IR_GENERATION_VALIDATION).to_string() + } Self::NinjaSynthesisAndExecution => tool_key.map_or_else( - || { - StageDescription::new( - localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS).to_string(), - ) - }, + || localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS).to_string(), |tool_message_key| { let tool = localization::message(tool_message_key.as_str()).to_string(); - StageDescription::new( - localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS_EXECUTE) - .with_arg("tool", tool) - .to_string(), - ) + localization::message(keys::STATUS_STAGE_NINJA_SYNTHESIS_EXECUTE) + .with_arg("tool", tool) + .to_string() }, ), } } } +/// Compile-time guard ensuring `PipelineStage::ALL` stays in sync with the +/// declared `PIPELINE_STAGE_COUNT`. +const _: () = assert!( + PipelineStage::ALL.len() == super::PIPELINE_STAGE_COUNT as usize, + "PipelineStage::ALL length must equal PIPELINE_STAGE_COUNT" +); + /// Emit a localized status update for a concrete pipeline stage. pub fn report_pipeline_stage( reporter: &dyn StatusReporter, stage: PipelineStage, tool_key: Option, ) { - reporter.report_stage( - stage.index(), - PIPELINE_STAGE_TOTAL, - stage.description(tool_key), - ); + let description = stage.description(tool_key); + reporter.report_stage(stage.index(), PIPELINE_STAGE_TOTAL, &description); } diff --git a/src/status_tests.rs b/src/status_tests.rs index a73731d3..514e20bf 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -4,29 +4,20 @@ use super::*; use rstest::rstest; #[rstest] -#[case(1)] -#[case(PIPELINE_STAGE_COUNT)] -fn stage_number_accepts_in_range_values(#[case] value: u32) { - let stage = StageNumber::new(value).expect("in-range stage number should be valid"); - assert_eq!(stage.get(), value); -} - -#[rstest] -#[case(0)] -#[case(PIPELINE_STAGE_COUNT + 1)] -#[case(u32::MAX)] -fn stage_number_rejects_out_of_range_values(#[case] value: u32) { - let error = StageNumber::new(value).expect_err("out-of-range stage number should fail"); - assert_eq!(error, StageNumberError::OutOfRange(value)); +#[case(PipelineStage::ManifestIngestion, 1)] +#[case(PipelineStage::InitialYamlParsing, 2)] +#[case(PipelineStage::TemplateExpansion, 3)] +#[case(PipelineStage::FinalRendering, 4)] +#[case(PipelineStage::IrGenerationValidation, 5)] +#[case(PipelineStage::NinjaSynthesisAndExecution, 6)] +fn stage_index_matches_discriminant(#[case] stage: PipelineStage, #[case] expected: u32) { + assert_eq!(stage.index().get(), expected); } #[test] -fn stage_description_round_trips_string_content() { - let description = StageDescription::new(String::from("rendering")); - assert_eq!(description.as_str(), "rendering"); - - let from_impl: StageDescription = String::from("building").into(); - assert_eq!(from_impl.as_str(), "building"); +fn pipeline_stage_count_matches_stage_array() { + let stage_count = u32::try_from(PipelineStage::ALL.len()).expect("stage array length fits u32"); + assert_eq!(PIPELINE_STAGE_COUNT, stage_count); } #[test] @@ -36,25 +27,7 @@ fn localization_key_round_trips_static_key() { } #[test] -fn pipeline_stage_count_matches_stage_array() { - let stage_count = u32::try_from(PipelineStage::ALL.len()).unwrap_or(0); - assert_eq!(PIPELINE_STAGE_COUNT, stage_count); -} - -#[rstest] -#[case(PipelineStage::ManifestIngestion, 1)] -#[case(PipelineStage::InitialYamlParsing, 2)] -#[case(PipelineStage::TemplateExpansion, 3)] -#[case(PipelineStage::FinalRendering, 4)] -#[case(PipelineStage::IrGenerationValidation, 5)] -#[case(PipelineStage::NinjaSynthesisAndExecution, 6)] -fn stage_index_round_trips(#[case] stage: PipelineStage, #[case] expected: u32) { - assert_eq!(stage.index().get(), expected); - assert_eq!(PipelineStage::from_index(expected), Some(stage)); -} - -#[test] -fn invalid_stage_index_returns_none() { - assert_eq!(PipelineStage::from_index(0), None); - assert_eq!(PipelineStage::from_index(7), None); +fn localization_key_from_static_str() { + let key: LocalizationKey = keys::STATUS_STATE_PENDING.into(); + assert_eq!(key.as_str(), keys::STATUS_STATE_PENDING); } diff --git a/test_support/src/fluent.rs b/test_support/src/fluent.rs new file mode 100644 index 00000000..fe8ff47a --- /dev/null +++ b/test_support/src/fluent.rs @@ -0,0 +1,24 @@ +//! Fluent localization test utilities. +//! +//! Provides helpers for normalizing Fluent output in test assertions. + +/// Remove Fluent bidi isolate characters used around placeables. +/// +/// Fluent inserts these markers (`\u{2068}` and `\u{2069}`) to preserve +/// directionality when interpolating values. They are invisible to users but +/// can make plain substring assertions fail. +/// +/// # Examples +/// +/// ``` +/// use test_support::fluent::normalize_fluent_isolates; +/// +/// let raw = "Stage \u{2068}2\u{2069}/\u{2068}6\u{2069}"; +/// assert_eq!(normalize_fluent_isolates(raw), "Stage 2/6"); +/// ``` +#[must_use] +pub fn normalize_fluent_isolates(text: &str) -> String { + text.chars() + .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') + .collect() +} diff --git a/test_support/src/lib.rs b/test_support/src/lib.rs index 97f29bf6..68de81a4 100644 --- a/test_support/src/lib.rs +++ b/test_support/src/lib.rs @@ -19,6 +19,7 @@ pub mod env_guard; pub mod env_lock; pub mod env_var_guard; pub mod exec; +pub mod fluent; pub mod hash; pub mod http; pub mod locale_stubs; diff --git a/tests/bdd/helpers/assertions.rs b/tests/bdd/helpers/assertions.rs index 15b7b07a..294abd03 100644 --- a/tests/bdd/helpers/assertions.rs +++ b/tests/bdd/helpers/assertions.rs @@ -6,18 +6,7 @@ use anyhow::{Context, Result, ensure}; use rstest_bdd::Slot; - -/// Remove Fluent bidi isolate characters used around placeables. -/// -/// Fluent inserts these markers (`\u{2068}` and `\u{2069}`) to preserve -/// directionality when interpolating values. They are invisible to users but -/// can make plain substring assertions fail. -#[must_use] -fn normalize_fluent_isolates(text: &str) -> String { - text.chars() - .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') - .collect() -} +pub use test_support::fluent::normalize_fluent_isolates; /// Assert that optional content contains an expected fragment. /// diff --git a/tests/bdd/steps/manifest_command.rs b/tests/bdd/steps/manifest_command.rs index dfc3da36..4adab00b 100644 --- a/tests/bdd/steps/manifest_command.rs +++ b/tests/bdd/steps/manifest_command.rs @@ -1,7 +1,7 @@ //! Step definitions for `netsuke manifest` behavioural tests. use crate::bdd::fixtures::TestWorld; -use crate::bdd::helpers::assertions::assert_slot_contains; +use crate::bdd::helpers::assertions::{assert_slot_contains, normalize_fluent_isolates}; use crate::bdd::types::{ CliArgs, DirectoryName, FileName, ManifestOutputPath, OutputFragment, PathString, }; @@ -29,13 +29,6 @@ impl fmt::Display for OutputType { } } -#[must_use] -fn normalize_fluent_isolates(text: &str) -> String { - text.chars() - .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') - .collect() -} - // --------------------------------------------------------------------------- // Helper functions // --------------------------------------------------------------------------- diff --git a/tests/localization_tests.rs b/tests/localization_tests.rs index c6fa35f2..bc57fec7 100644 --- a/tests/localization_tests.rs +++ b/tests/localization_tests.rs @@ -8,6 +8,7 @@ use test_support::localizer_test_lock; use netsuke::cli_localization; use netsuke::localization::{self, LocalizerGuard, keys}; +use test_support::fluent::normalize_fluent_isolates; /// Guard pair holding both the test lock and the localizer override. /// @@ -44,13 +45,6 @@ fn which_message(command: &str) -> String { .to_string() } -#[must_use] -fn normalize_fluent_isolates(text: &str) -> String { - text.chars() - .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}') - .collect() -} - #[rstest] #[case("es-ES", "no encontrado")] #[case("fr-FR", "not found")] From 936f98d8ed1c32b1f125d14f22b67080954e9690 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 15 Feb 2026 18:48:24 +0000 Subject: [PATCH 6/9] refactor(runner): extract path helpers and manifest checks Moved manifest path resolution, output path resolution, and manifest existence validation into a new module `runner::path_helpers` to improve code organization and separation of concerns. Additionally, streamlined runner module by delegating these responsibilities to the helper functions. Co-authored-by: devboxerhub[bot] --- src/cli_l10n.rs | 23 ++++- src/manifest/mod.rs | 49 +++++----- src/manifest/tests/stages.rs | 82 ++++++++-------- src/manifest/tests/workspace.rs | 1 + src/runner/mod.rs | 164 +++++++------------------------- src/runner/path_helpers.rs | 118 +++++++++++++++++++++++ src/status.rs | 5 +- src/status_pipeline.rs | 27 ++++-- src/status_tests.rs | 6 +- 9 files changed, 259 insertions(+), 216 deletions(-) create mode 100644 src/runner/path_helpers.rs diff --git a/src/cli_l10n.rs b/src/cli_l10n.rs index e9e33211..8404321c 100644 --- a/src/cli_l10n.rs +++ b/src/cli_l10n.rs @@ -22,14 +22,20 @@ pub(crate) fn localize_command(mut command: Command, localizer: &dyn Localizer) let usage = localizer.message(keys::CLI_USAGE, Some(&args), &fallback_usage); command = command.override_usage(usage); - if let Some(about) = command.get_about().map(ToString::to_string) { + if let Some(about) = command + .get_about() + .map(|s: &clap::builder::StyledStr| s.to_string()) + { let localized_text = localizer.message(keys::CLI_ABOUT, None, &about); command = command.about(localized_text); } else if let Some(message) = localizer.lookup(keys::CLI_ABOUT, None) { command = command.about(message); } - if let Some(long_about) = command.get_long_about().map(ToString::to_string) { + if let Some(long_about) = command + .get_long_about() + .map(|s: &clap::builder::StyledStr| s.to_string()) + { let localized_text = localizer.message(keys::CLI_LONG_ABOUT, None, &long_about); command = command.long_about(localized_text); } else if let Some(message) = localizer.lookup(keys::CLI_LONG_ABOUT, None) { @@ -56,7 +62,10 @@ fn localize_arguments( let Some(key) = flag_help_key(arg_id, subcommand_name) else { return arg; }; - if let Some(help) = arg.get_help().map(ToString::to_string) { + if let Some(help) = arg + .get_help() + .map(|s: &clap::builder::StyledStr| s.to_string()) + { let message = localizer.message(key, None, &help); return arg.help(message); } @@ -86,7 +95,9 @@ fn localize_subcommands(command: &mut Command, localizer: &dyn Localizer) { if let Some(localized) = localize_field( localizer, subcommand_about_key(&name), - updated.get_about().map(ToString::to_string), + updated + .get_about() + .map(|s: &clap::builder::StyledStr| s.to_string()), ) { updated = updated.about(localized); } @@ -94,7 +105,9 @@ fn localize_subcommands(command: &mut Command, localizer: &dyn Localizer) { if let Some(localized) = localize_field( localizer, subcommand_long_about_key(&name), - updated.get_long_about().map(ToString::to_string), + updated + .get_long_about() + .map(|s: &clap::builder::StyledStr| s.to_string()), ) { updated = updated.long_about(localized); } diff --git a/src/manifest/mod.rs b/src/manifest/mod.rs index 1ed5d375..74b0d66f 100644 --- a/src/manifest/mod.rs +++ b/src/manifest/mod.rs @@ -101,6 +101,16 @@ fn env_var(name: &str) -> std::result::Result { } } +/// Invoke the stage callback when present. +fn notify_stage( + on_stage: &mut Option<&mut dyn FnMut(ManifestLoadStage)>, + stage: ManifestLoadStage, +) { + if let Some(cb) = on_stage.as_mut() { + cb(stage); + } +} + /// Parse a manifest string using Jinja for value templating. /// /// The input YAML must be valid on its own. Jinja expressions are evaluated @@ -113,9 +123,9 @@ fn from_str_named( yaml: &str, name: &ManifestName, stdlib_config: Option, - on_stage: &mut dyn FnMut(ManifestLoadStage), + on_stage: &mut Option<&mut dyn FnMut(ManifestLoadStage)>, ) -> Result { - on_stage(ManifestLoadStage::InitialYamlParsing); + notify_stage(on_stage, ManifestLoadStage::InitialYamlParsing); let mut doc: ManifestValue = serde_saphyr::from_str(yaml).map_err(|e| ManifestError::Parse { source: map_yaml_error(e, &ManifestSource::from(yaml), name), @@ -150,12 +160,12 @@ fn from_str_named( } } - on_stage(ManifestLoadStage::TemplateExpansion); + notify_stage(on_stage, ManifestLoadStage::TemplateExpansion); register_manifest_macros(&doc, &mut jinja)?; expand_foreach(&mut doc, &jinja)?; - on_stage(ManifestLoadStage::FinalRendering); + notify_stage(on_stage, ManifestLoadStage::FinalRendering); let manifest: NetsukeManifest = serde_json::from_value(doc).map_err(|e| ManifestError::Parse { source: map_data_error(e, name), @@ -174,7 +184,7 @@ fn from_str_named( /// /// Returns an error if YAML parsing or Jinja evaluation fails. pub fn from_str(yaml: &str) -> Result { - from_str_named(yaml, &ManifestName::new("Netsukefile"), None, &mut |_| {}) + from_str_named(yaml, &ManifestName::new("Netsukefile"), None, &mut None) } /// Load a [`NetsukeManifest`] from the given file path. @@ -183,11 +193,13 @@ pub fn from_str(yaml: &str) -> Result { /// /// Returns an error if the file cannot be read or the YAML fails to parse. pub fn from_path(path: impl AsRef) -> Result { - from_path_with_policy(path, NetworkPolicy::default()) + from_path_with_policy(path, NetworkPolicy::default(), None) } -/// Load a [`NetsukeManifest`] from the given file path using an explicit network -/// policy. +/// Load a [`NetsukeManifest`] from the given file path using an explicit +/// network policy and an optional stage callback. +/// +/// The callback, when provided, is invoked in order for each manifest stage. /// /// # Errors /// @@ -200,30 +212,15 @@ pub fn from_path(path: impl AsRef) -> Result { /// use netsuke::stdlib::NetworkPolicy; /// /// let policy = NetworkPolicy::default(); -/// let manifest = manifest::from_path_with_policy("Netsukefile", policy); +/// let manifest = manifest::from_path_with_policy("Netsukefile", policy, None); /// assert!(manifest.is_ok()); /// ``` pub fn from_path_with_policy( path: impl AsRef, policy: NetworkPolicy, + mut on_stage: Option<&mut dyn FnMut(ManifestLoadStage)>, ) -> Result { - from_path_with_policy_and_stage_callback(path, policy, |_| {}) -} - -/// Load a [`NetsukeManifest`] from the given file path using an explicit -/// network policy and a stage callback. -/// -/// The callback is invoked in order for each manifest stage. -/// -/// # Errors -/// -/// Returns an error if the file cannot be read or the YAML fails to parse. -pub fn from_path_with_policy_and_stage_callback( - path: impl AsRef, - policy: NetworkPolicy, - mut on_stage: impl FnMut(ManifestLoadStage), -) -> Result { - on_stage(ManifestLoadStage::ManifestIngestion); + notify_stage(&mut on_stage, ManifestLoadStage::ManifestIngestion); let path_ref = path.as_ref(); let data = fs::read_to_string(path_ref).with_context(|| { localization::message(keys::MANIFEST_READ_FAILED) diff --git a/src/manifest/tests/stages.rs b/src/manifest/tests/stages.rs index 10391760..7b0f8855 100644 --- a/src/manifest/tests/stages.rs +++ b/src/manifest/tests/stages.rs @@ -3,39 +3,40 @@ use crate::manifest::{self, ManifestLoadStage}; use crate::stdlib::NetworkPolicy; use anyhow::{Context, Result, ensure}; +use rstest::{fixture, rstest}; -/// Create a temporary workspace with a manifest containing the given command. -fn temp_manifest(command: &str) -> Result<(tempfile::TempDir, std::path::PathBuf)> { +/// Create a temporary workspace with a manifest path but no file content. +#[fixture] +fn temp_workspace() -> Result<(tempfile::TempDir, std::path::PathBuf)> { let dir = tempfile::tempdir().context("create temp workspace for stage test")?; let manifest_path = dir.path().join("Netsukefile"); + Ok((dir, manifest_path)) +} + +#[rstest] +fn stage_callback_reports_expected_order_for_valid_manifest( + temp_workspace: Result<(tempfile::TempDir, std::path::PathBuf)>, +) -> Result<()> { + let (_dir, manifest_path) = temp_workspace?; std::fs::write( &manifest_path, - format!( - concat!( - "netsuke_version: \"1.0.0\"\n", - "rules:\n", - " - name: touch\n", - " command: \"{command}\"\n", - "targets:\n", - " - name: out.txt\n", - " rule: touch\n", - ), - command = command, + concat!( + "netsuke_version: \"1.0.0\"\n", + "rules:\n", + " - name: touch\n", + " command: \"touch $out\"\n", + "targets:\n", + " - name: out.txt\n", + " rule: touch\n", ), ) .with_context(|| format!("write {}", manifest_path.display()))?; - Ok((dir, manifest_path)) -} - -#[test] -fn stage_callback_reports_expected_order_for_valid_manifest() -> Result<()> { - let (_dir, manifest_path) = temp_manifest("touch $out")?; let mut stages = Vec::new(); - let manifest = manifest::from_path_with_policy_and_stage_callback( + let manifest = manifest::from_path_with_policy( &manifest_path, NetworkPolicy::default(), - |stage| stages.push(stage), + Some(&mut |stage| stages.push(stage)), )?; ensure!( @@ -55,18 +56,19 @@ fn stage_callback_reports_expected_order_for_valid_manifest() -> Result<()> { Ok(()) } -#[test] -fn stage_callback_stops_after_parse_failure() -> Result<()> { - let dir = tempfile::tempdir().context("create temp workspace for stage failure test")?; - let manifest_path = dir.path().join("Netsukefile"); +#[rstest] +fn stage_callback_stops_after_parse_failure( + temp_workspace: Result<(tempfile::TempDir, std::path::PathBuf)>, +) -> Result<()> { + let (_dir, manifest_path) = temp_workspace?; std::fs::write(&manifest_path, "targets:\n\t- name: broken\n") .with_context(|| format!("write {}", manifest_path.display()))?; let mut stages = Vec::new(); - let result = manifest::from_path_with_policy_and_stage_callback( + let result = manifest::from_path_with_policy( &manifest_path, NetworkPolicy::default(), - |stage| stages.push(stage), + Some(&mut |stage| stages.push(stage)), ); ensure!(result.is_err(), "invalid manifest should fail"); ensure!( @@ -82,10 +84,11 @@ fn stage_callback_stops_after_parse_failure() -> Result<()> { /// Template expansion failures should report stages up to and including /// `TemplateExpansion`. -#[test] -fn stage_callback_stops_after_template_expansion_failure() -> Result<()> { - let dir = tempfile::tempdir().context("create temp workspace for template test")?; - let manifest_path = dir.path().join("Netsukefile"); +#[rstest] +fn stage_callback_stops_after_template_expansion_failure( + temp_workspace: Result<(tempfile::TempDir, std::path::PathBuf)>, +) -> Result<()> { + let (_dir, manifest_path) = temp_workspace?; // Reference a non-existent Jinja variable to trigger expansion failure. std::fs::write( &manifest_path, @@ -103,10 +106,10 @@ fn stage_callback_stops_after_template_expansion_failure() -> Result<()> { .with_context(|| format!("write {}", manifest_path.display()))?; let mut stages = Vec::new(); - let result = manifest::from_path_with_policy_and_stage_callback( + let result = manifest::from_path_with_policy( &manifest_path, NetworkPolicy::default(), - |stage| stages.push(stage), + Some(&mut |stage| stages.push(stage)), ); ensure!(result.is_err(), "template expansion should fail"); ensure!( @@ -122,10 +125,11 @@ fn stage_callback_stops_after_template_expansion_failure() -> Result<()> { /// Final rendering failures should report stages up to and including /// `FinalRendering`. -#[test] -fn stage_callback_stops_after_final_rendering_failure() -> Result<()> { - let dir = tempfile::tempdir().context("create temp workspace for rendering test")?; - let manifest_path = dir.path().join("Netsukefile"); +#[rstest] +fn stage_callback_stops_after_final_rendering_failure( + temp_workspace: Result<(tempfile::TempDir, std::path::PathBuf)>, +) -> Result<()> { + let (_dir, manifest_path) = temp_workspace?; // Missing required `netsuke_version` causes a deserialization error during // FinalRendering. std::fs::write( @@ -142,10 +146,10 @@ fn stage_callback_stops_after_final_rendering_failure() -> Result<()> { .with_context(|| format!("write {}", manifest_path.display()))?; let mut stages = Vec::new(); - let result = manifest::from_path_with_policy_and_stage_callback( + let result = manifest::from_path_with_policy( &manifest_path, NetworkPolicy::default(), - |stage| stages.push(stage), + Some(&mut |stage| stages.push(stage)), ); ensure!(result.is_err(), "final rendering should fail"); ensure!( diff --git a/src/manifest/tests/workspace.rs b/src/manifest/tests/workspace.rs index 8df94558..65a95802 100644 --- a/src/manifest/tests/workspace.rs +++ b/src/manifest/tests/workspace.rs @@ -127,6 +127,7 @@ fn from_path_uses_manifest_directory_for_caches() -> AnyResult<()> { .deny_all_hosts() .allow_hosts(["127.0.0.1", "localhost"])? .allow_scheme("http")?, + None, )?; if let Err(err) = server.join() { return Err(anyhow!("join server thread panicked: {err:?}")); diff --git a/src/runner/mod.rs b/src/runner/mod.rs index ed9a8aa7..edc98cc5 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -16,7 +16,7 @@ use crate::status::{ StatusReporter, report_pipeline_stage, }; use crate::{ir::BuildGraph, manifest, ninja_gen}; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result}; use camino::Utf8PathBuf; use std::borrow::Cow; use std::path::Path; @@ -28,11 +28,14 @@ pub const NINJA_PROGRAM: &str = "ninja"; /// Environment variable override for the Ninja executable. pub use ninja_env::NINJA_ENV; +mod path_helpers; mod process; #[cfg(doctest)] pub use process::doc; pub use process::{run_ninja, run_ninja_tool}; +use path_helpers::{ensure_manifest_exists_or_error, resolve_manifest_path, resolve_output_path}; + /// Wrapper around generated Ninja manifest text. #[derive(Debug, Clone)] pub struct NinjaContent(String); @@ -87,6 +90,16 @@ impl Default for BuildTargets<'_> { } } +/// Build the appropriate [`StatusReporter`] for the resolved output mode and +/// progress preference. +fn make_reporter(mode: OutputMode, progress_enabled: bool) -> Box { + match (mode, progress_enabled) { + (OutputMode::Accessible, _) => Box::new(AccessibleReporter), + (OutputMode::Standard, true) => Box::new(IndicatifReporter::new()), + (OutputMode::Standard, false) => Box::new(SilentReporter), + } +} + /// Execute the parsed [`Cli`] commands. /// /// # Errors @@ -94,11 +107,7 @@ impl Default for BuildTargets<'_> { /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli) -> Result<()> { let mode = output_mode::resolve(cli.accessible); - let reporter: Box = match (mode, cli.progress.unwrap_or(true)) { - (OutputMode::Accessible, _) => Box::new(AccessibleReporter), - (OutputMode::Standard, true) => Box::new(IndicatifReporter::new()), - (OutputMode::Standard, false) => Box::new(SilentReporter), - }; + let reporter = make_reporter(mode, cli.progress.unwrap_or(true)); let command = cli.command.clone().unwrap_or(Commands::Build(BuildArgs { emit: None, @@ -268,137 +277,30 @@ fn generate_ninja( Ok(NinjaContent::new(ninja)) } -fn ensure_manifest_exists_or_error( - cli: &Cli, - reporter: &dyn StatusReporter, - manifest_path: &Utf8PathBuf, -) -> Result<()> { - if manifest_path.as_std_path().exists() { - return Ok(()); - } - - report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); - // `resolve_manifest_path()` validates that `file_name()` is Some. - let manifest_name = manifest_path - .file_name() - .ok_or_else(|| { - anyhow!( - "{}", - localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME) - .with_arg("path", manifest_path.as_str()) - ) - })? - .to_owned(); - let directory = if cli.directory.is_some() { - let parent = manifest_path - .parent() - .map_or_else(|| manifest_path.as_str(), camino::Utf8Path::as_str); - localization::message(keys::RUNNER_MANIFEST_DIRECTORY) - .with_arg("directory", parent) - .to_string() - } else { - localization::message(keys::RUNNER_MANIFEST_CURRENT_DIRECTORY).to_string() - }; - let message = localization::message(keys::RUNNER_MANIFEST_NOT_FOUND) - .with_arg("manifest_name", manifest_name.as_str()) - .with_arg("directory", &directory); - Err(RunnerError::ManifestNotFound { - manifest_name, - directory, - path: manifest_path.to_path_buf().into_std_path_buf(), - message, - help: localization::message(keys::RUNNER_MANIFEST_NOT_FOUND_HELP), - } - .into()) -} - fn load_manifest_with_stage_reporting( manifest_path: &Utf8PathBuf, policy: crate::stdlib::NetworkPolicy, reporter: &dyn StatusReporter, ) -> Result { - manifest::from_path_with_policy_and_stage_callback( - manifest_path.as_std_path(), - policy, - |stage| match stage { - manifest::ManifestLoadStage::ManifestIngestion => { - report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); - } - manifest::ManifestLoadStage::InitialYamlParsing => { - report_pipeline_stage(reporter, PipelineStage::InitialYamlParsing, None); - } - manifest::ManifestLoadStage::TemplateExpansion => { - report_pipeline_stage(reporter, PipelineStage::TemplateExpansion, None); - } - manifest::ManifestLoadStage::FinalRendering => { - report_pipeline_stage(reporter, PipelineStage::FinalRendering, None); - } - }, - ) - .with_context(|| { - localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) - .with_arg("path", manifest_path.as_str()) - }) -} - -/// Determine the manifest path respecting the CLI's directory option. -/// -/// # Errors -/// Returns an error when the CLI `file` or `directory` paths are not valid UTF-8. -/// -/// # Examples -/// ```ignore -/// use crate::cli::Cli; -/// use crate::runner::resolve_manifest_path; -/// let cli = Cli::default(); -/// let path = resolve_manifest_path(&cli).expect("valid manifest path"); -/// assert!(path.as_str().ends_with("Netsukefile")); -/// ``` -fn resolve_manifest_path(cli: &Cli) -> Result { - let file = Utf8PathBuf::from_path_buf(cli.file.clone()).map_err(|path| { - anyhow!( - "{}", - localization::message(keys::RUNNER_MANIFEST_PATH_UTF8) - .with_arg("path", path.display().to_string()) - ) - })?; - let resolved = if let Some(dir) = &cli.directory { - let base = Utf8PathBuf::from_path_buf(dir.clone()).map_err(|path| { - anyhow!( - "{}", - localization::message(keys::RUNNER_MANIFEST_DIR_UTF8) - .with_arg("path", path.display().to_string()) - ) - })?; - base.join(&file) - } else { - file + let mut on_stage = |stage: manifest::ManifestLoadStage| match stage { + manifest::ManifestLoadStage::ManifestIngestion => { + report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); + } + manifest::ManifestLoadStage::InitialYamlParsing => { + report_pipeline_stage(reporter, PipelineStage::InitialYamlParsing, None); + } + manifest::ManifestLoadStage::TemplateExpansion => { + report_pipeline_stage(reporter, PipelineStage::TemplateExpansion, None); + } + manifest::ManifestLoadStage::FinalRendering => { + report_pipeline_stage(reporter, PipelineStage::FinalRendering, None); + } }; - if resolved.file_name().is_none() { - return Err(anyhow!( - "{}", - localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME) - .with_arg("path", resolved.as_str()) - )); - } - Ok(resolved) -} - -/// Resolve an output path relative to the CLI working directory. -/// -/// The Netsuke `-C/--directory` option behaves like a working directory change -/// for any filesystem paths supplied on the command line. When `path` is -/// relative and a directory has been configured, the returned path is -/// `directory/path`. -#[must_use] -fn resolve_output_path<'a>(cli: &Cli, path: &'a Path) -> Cow<'a, Path> { - if path.is_relative() { - cli.directory - .as_ref() - .map_or_else(|| Cow::Borrowed(path), |dir| Cow::Owned(dir.join(path))) - } else { - Cow::Borrowed(path) - } + manifest::from_path_with_policy(manifest_path.as_std_path(), policy, Some(&mut on_stage)) + .with_context(|| { + localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) + .with_arg("path", manifest_path.as_str()) + }) } #[cfg(test)] diff --git a/src/runner/path_helpers.rs b/src/runner/path_helpers.rs new file mode 100644 index 00000000..8c74b4b9 --- /dev/null +++ b/src/runner/path_helpers.rs @@ -0,0 +1,118 @@ +//! Path resolution helpers for the runner module. +//! +//! Centralises manifest and output path logic so the main runner module stays +//! focused on command dispatch. + +use crate::cli::Cli; +use crate::localization::{self, keys}; +use crate::status::{PipelineStage, StatusReporter, report_pipeline_stage}; +use anyhow::{Result, anyhow}; +use camino::Utf8PathBuf; +use std::borrow::Cow; +use std::path::Path; + +use super::RunnerError; + +/// Determine the manifest path respecting the CLI's directory option. +/// +/// # Errors +/// Returns an error when the CLI `file` or `directory` paths are not valid UTF-8. +/// +/// # Examples +/// ```ignore +/// use crate::cli::Cli; +/// use crate::runner::resolve_manifest_path; +/// let cli = Cli::default(); +/// let path = resolve_manifest_path(&cli).expect("valid manifest path"); +/// assert!(path.as_str().ends_with("Netsukefile")); +/// ``` +pub(super) fn resolve_manifest_path(cli: &Cli) -> Result { + let file = Utf8PathBuf::from_path_buf(cli.file.clone()).map_err(|path| { + anyhow!( + "{}", + localization::message(keys::RUNNER_MANIFEST_PATH_UTF8) + .with_arg("path", path.display().to_string()) + ) + })?; + let resolved = if let Some(dir) = &cli.directory { + let base = Utf8PathBuf::from_path_buf(dir.clone()).map_err(|path| { + anyhow!( + "{}", + localization::message(keys::RUNNER_MANIFEST_DIR_UTF8) + .with_arg("path", path.display().to_string()) + ) + })?; + base.join(&file) + } else { + file + }; + if resolved.file_name().is_none() { + return Err(anyhow!( + "{}", + localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME) + .with_arg("path", resolved.as_str()) + )); + } + Ok(resolved) +} + +/// Resolve an output path relative to the CLI working directory. +/// +/// The Netsuke `-C/--directory` option behaves like a working directory change +/// for any filesystem paths supplied on the command line. When `path` is +/// relative and a directory has been configured, the returned path is +/// `directory/path`. +#[must_use] +pub(super) fn resolve_output_path<'a>(cli: &Cli, path: &'a Path) -> Cow<'a, Path> { + if path.is_relative() { + cli.directory + .as_ref() + .map_or_else(|| Cow::Borrowed(path), |dir| Cow::Owned(dir.join(path))) + } else { + Cow::Borrowed(path) + } +} + +pub(super) fn ensure_manifest_exists_or_error( + cli: &Cli, + reporter: &dyn StatusReporter, + manifest_path: &Utf8PathBuf, +) -> Result<()> { + if manifest_path.as_std_path().exists() { + return Ok(()); + } + + report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None); + // `resolve_manifest_path()` validates that `file_name()` is Some. + let manifest_name = manifest_path + .file_name() + .ok_or_else(|| { + anyhow!( + "{}", + localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME) + .with_arg("path", manifest_path.as_str()) + ) + })? + .to_owned(); + let directory = if cli.directory.is_some() { + let parent = manifest_path + .parent() + .map_or_else(|| manifest_path.as_str(), camino::Utf8Path::as_str); + localization::message(keys::RUNNER_MANIFEST_DIRECTORY) + .with_arg("directory", parent) + .to_string() + } else { + localization::message(keys::RUNNER_MANIFEST_CURRENT_DIRECTORY).to_string() + }; + let message = localization::message(keys::RUNNER_MANIFEST_NOT_FOUND) + .with_arg("manifest_name", manifest_name.as_str()) + .with_arg("directory", &directory); + Err(RunnerError::ManifestNotFound { + manifest_name, + directory, + path: manifest_path.to_path_buf().into_std_path_buf(), + message, + help: localization::message(keys::RUNNER_MANIFEST_NOT_FOUND_HELP), + } + .into()) +} diff --git a/src/status.rs b/src/status.rs index c06f331d..ab838e3f 100644 --- a/src/status.rs +++ b/src/status.rs @@ -5,9 +5,6 @@ use indicatif::{MultiProgress, ProgressBar, ProgressDrawTarget, ProgressStyle}; use std::io::{self, Write}; use std::sync::Mutex; -/// Total count of user-visible pipeline stages. -pub const PIPELINE_STAGE_COUNT: u32 = 6; - /// Thin wrapper for a 1-based stage index. /// /// All current call sites derive stage numbers from the `PipelineStage` enum @@ -54,9 +51,9 @@ impl From<&'static str> for LocalizationKey { } } -const PIPELINE_STAGE_TOTAL: StageNumber = StageNumber(PIPELINE_STAGE_COUNT); #[path = "status_pipeline.rs"] mod pipeline; +use pipeline::PIPELINE_STAGE_TOTAL; pub use pipeline::{PipelineStage, report_pipeline_stage}; fn stage_label(current: StageNumber, total: StageNumber, description: &str) -> String { diff --git a/src/status_pipeline.rs b/src/status_pipeline.rs index b82c946a..ed607c17 100644 --- a/src/status_pipeline.rs +++ b/src/status_pipeline.rs @@ -1,8 +1,26 @@ //! Pipeline stage model and stage reporting helper. -use super::{LocalizationKey, PIPELINE_STAGE_TOTAL, StageNumber, StatusReporter}; +use super::{LocalizationKey, StageNumber, StatusReporter}; use crate::localization::{self, keys}; +/// Total pipeline stage count, derived from the canonical stage list. +/// +/// The array length is a small compile-time constant (currently 6) so the +/// truncating cast is safe. A static assertion below guards against the +/// theoretical case where the array exceeds `u32::MAX` entries. +#[expect( + clippy::cast_possible_truncation, + reason = "PipelineStage::ALL length is a small compile-time constant" +)] +pub(crate) const PIPELINE_STAGE_TOTAL: StageNumber = + StageNumber::new_unchecked(PipelineStage::ALL.len() as u32); + +/// Guard that the stage array never exceeds `u32::MAX` entries. +const _: () = assert!( + PipelineStage::ALL.len() <= u32::MAX as usize, + "PipelineStage::ALL length must fit in u32" +); + /// Enumerates pipeline stages in user-visible execution order. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum PipelineStage { @@ -69,13 +87,6 @@ impl PipelineStage { } } -/// Compile-time guard ensuring `PipelineStage::ALL` stays in sync with the -/// declared `PIPELINE_STAGE_COUNT`. -const _: () = assert!( - PipelineStage::ALL.len() == super::PIPELINE_STAGE_COUNT as usize, - "PipelineStage::ALL length must equal PIPELINE_STAGE_COUNT" -); - /// Emit a localized status update for a concrete pipeline stage. pub fn report_pipeline_stage( reporter: &dyn StatusReporter, diff --git a/src/status_tests.rs b/src/status_tests.rs index 514e20bf..3dfc6b71 100644 --- a/src/status_tests.rs +++ b/src/status_tests.rs @@ -15,9 +15,9 @@ fn stage_index_matches_discriminant(#[case] stage: PipelineStage, #[case] expect } #[test] -fn pipeline_stage_count_matches_stage_array() { - let stage_count = u32::try_from(PipelineStage::ALL.len()).expect("stage array length fits u32"); - assert_eq!(PIPELINE_STAGE_COUNT, stage_count); +fn pipeline_stage_total_derived_from_all() { + let expected = u32::try_from(PipelineStage::ALL.len()).expect("stage array length fits u32"); + assert_eq!(PIPELINE_STAGE_TOTAL.get(), expected); } #[test] From 348895686fc62ceb13ec7d78f49d4e8b64aefe1c Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 15 Feb 2026 22:49:45 +0000 Subject: [PATCH 7/9] refactor(ir): replace str::to_string with str::to_owned and clarify path string conversion - Changed map_string_or_list from using str::to_string to str::to_owned to avoid unnecessary allocations. - Updated get_target_display_name to use explicit closure and Utf8PathBuf to_string call for clarity. Co-authored-by: devboxerhub[bot] --- src/ir/from_manifest.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ir/from_manifest.rs b/src/ir/from_manifest.rs index 0db69995..820ff1ec 100644 --- a/src/ir/from_manifest.rs +++ b/src/ir/from_manifest.rs @@ -187,7 +187,7 @@ fn to_paths(sol: &StringOrList) -> Vec { } fn to_string_vec(sol: &StringOrList) -> Vec { - map_string_or_list(sol, str::to_string) + map_string_or_list(sol, str::to_owned) } fn extract_single(sol: &StringOrList) -> Option<&str> { @@ -257,5 +257,8 @@ fn find_duplicates( } fn get_target_display_name(paths: &[Utf8PathBuf]) -> String { - paths.first().map(ToString::to_string).unwrap_or_default() + paths + .first() + .map(|p: &Utf8PathBuf| p.to_string()) + .unwrap_or_default() } From db6a75ce18c1b0dec2c631bdbf7e74c9ff14c6f8 Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 16 Feb 2026 23:31:02 +0000 Subject: [PATCH 8/9] refactor(tests): replace str::to_string with str::to_owned in test code Replaced multiple instances of `str::to_string` method calls with `str::to_owned` in test modules to improve semantic clarity and consistency in string conversions without changing functionality. Co-authored-by: devboxerhub[bot] --- tests/bdd/types.rs | 2 +- tests/command_escaping_tests.rs | 12 ++++++------ tests/manifest_jinja_tests.rs | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/bdd/types.rs b/tests/bdd/types.rs index aa044275..fa35125d 100644 --- a/tests/bdd/types.rs +++ b/tests/bdd/types.rs @@ -230,7 +230,7 @@ impl NamesList { /// Collect the names into a set. pub fn to_set(&self) -> std::collections::BTreeSet { - self.iter().map(str::to_string).collect() + self.iter().map(str::to_owned).collect() } } diff --git a/tests/command_escaping_tests.rs b/tests/command_escaping_tests.rs index 5721d9fb..71e6c8d4 100644 --- a/tests/command_escaping_tests.rs +++ b/tests/command_escaping_tests.rs @@ -41,7 +41,7 @@ fn inputs_and_outputs_are_quoted() -> Result<()> { let words = command_words( "targets:\n - name: 'out file'\n sources: 'in file'\n command: \"cat $in > $out\"\n", )?; - let expected = ["cat", "in file", ">", "out file"].map(str::to_string); + let expected = ["cat", "in file", ">", "out file"].map(str::to_owned); ensure!( words == expected, "expected shell words to match quoted inputs/outputs" @@ -57,7 +57,7 @@ fn multiple_inputs_outputs_with_special_chars_are_quoted() -> Result<()> { let expected = [ "echo", "in file", "input$1", "&&", "echo", "out file", "out&2", ] - .map(str::to_string); + .map(str::to_owned); ensure!( words == expected, "expected words to preserve quoting for lists" @@ -70,7 +70,7 @@ fn variable_name_overlap_not_rewritten() -> Result<()> { let words = command_words( "targets:\n - name: 'out file'\n sources: in\n command: \"echo $input > $out\"\n", )?; - let expected = ["echo", "$input", ">", "out file"].map(str::to_string); + let expected = ["echo", "$input", ">", "out file"].map(str::to_owned); ensure!(words == expected, "unexpected placeholder rewriting"); Ok(()) } @@ -80,7 +80,7 @@ fn output_variable_overlap_not_rewritten() -> Result<()> { let words = command_words( "targets:\n - name: out\n sources: in\n command: \"echo $output_dir > $out\"\n", )?; - let expected = ["echo", "$output_dir", ">", "out"].map(str::to_string); + let expected = ["echo", "$output_dir", ">", "out"].map(str::to_owned); ensure!(words == expected, "unexpected output placeholder rewriting"); Ok(()) } @@ -90,7 +90,7 @@ fn newline_in_paths_is_quoted() -> Result<()> { let words = command_words( "targets:\n - name: \"o'ut\\nfile\"\n sources: \"-in file\"\n command: \"printf %s $in > $out\"\n", )?; - let expected = ["printf", "%s", "-in file", ">", "o'ut\nfile"].map(str::to_string); + let expected = ["printf", "%s", "-in file", ">", "o'ut\nfile"].map(str::to_owned); ensure!(words == expected, "expected newline to be preserved"); Ok(()) } @@ -99,7 +99,7 @@ fn newline_in_paths_is_quoted() -> Result<()> { fn command_without_placeholders_remains_valid() -> Result<()> { let words = command_words("targets:\n - name: out\n sources: in\n command: \"echo hi\"\n")?; - let expected = ["echo", "hi"].map(str::to_string); + let expected = ["echo", "hi"].map(str::to_owned); ensure!( words == expected, "command without placeholders should split literally" diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs index d407e1b9..be2ac07b 100644 --- a/tests/manifest_jinja_tests.rs +++ b/tests/manifest_jinja_tests.rs @@ -413,7 +413,7 @@ fn expands_foreach_with_item_and_index( names == expected_names .iter() - .map(ToString::to_string) + .map(ToOwned::to_owned) .collect::>(), "unexpected names: {names:?}" ); @@ -423,7 +423,7 @@ fn expands_foreach_with_item_and_index( commands == expected_commands .iter() - .map(ToString::to_string) + .map(ToOwned::to_owned) .collect::>(), "unexpected commands: {commands:?}" ); From 75b3a5b583ab0824ac3f003f675830e0aa7362ee Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 16 Feb 2026 23:41:52 +0000 Subject: [PATCH 9/9] refactor: use typed closures to satisfy str_to_string lint on nightly Replace `ToString::to_string` function references with explicitly typed closures (e.g. `|e: &(dyn std::error::Error + '_)| e.to_string()`) when converting trait objects and non-str Display implementors. Nightly clippy's `str_to_string` lint false-positively flags `ToString::to_string` on `&dyn Error` and `&LocalizedMessage` as `&str`-to-`String` conversions. A plain closure triggers the `redundant_closure_for_method_calls` lint instead. Adding an explicit type annotation to the closure parameter disambiguates the call and satisfies both lints simultaneously. Co-Authored-By: Claude Opus 4.6 --- src/manifest/diagnostics/yaml.rs | 2 +- tests/ast_tests.rs | 2 +- tests/manifest_jinja_tests.rs | 2 +- tests/runner_tests.rs | 10 ++++++++-- tests/runner_tool_subcommands_tests.rs | 10 ++++++++-- tests/yaml_error_tests.rs | 2 +- 6 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/manifest/diagnostics/yaml.rs b/src/manifest/diagnostics/yaml.rs index 83b8e36b..299adb2f 100644 --- a/src/manifest/diagnostics/yaml.rs +++ b/src/manifest/diagnostics/yaml.rs @@ -186,7 +186,7 @@ mod tests { let help = yaml_diag .help .as_ref() - .map(ToString::to_string) + .map(|m: &LocalizedMessage| m.to_string()) .unwrap_or_default(); ensure!(help == expected, "message missing tab hint: {help}"); Ok(()) diff --git a/tests/ast_tests.rs b/tests/ast_tests.rs index ba6808fe..da47a0a3 100644 --- a/tests/ast_tests.rs +++ b/tests/ast_tests.rs @@ -128,7 +128,7 @@ fn vars_section_must_be_object() -> Result<()> { .context("vars should be an object")?; let chain = err .chain() - .map(ToString::to_string) + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) .collect::>() .join("\n"); let expected = localization::message(keys::MANIFEST_VARS_NOT_OBJECT).to_string(); diff --git a/tests/manifest_jinja_tests.rs b/tests/manifest_jinja_tests.rs index be2ac07b..6ee32cd7 100644 --- a/tests/manifest_jinja_tests.rs +++ b/tests/manifest_jinja_tests.rs @@ -530,7 +530,7 @@ fn foreach_vars_must_be_mapping() -> Result<()> { .context("vars must be a mapping")?; ensure!( err.chain() - .map(ToString::to_string) + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) .any(|msg| msg.contains("Target `vars` must be an object")), "unexpected error: {err}" ); diff --git a/tests/runner_tests.rs b/tests/runner_tests.rs index 001a63fa..5e534548 100644 --- a/tests/runner_tests.rs +++ b/tests/runner_tests.rs @@ -84,7 +84,10 @@ fn run_exits_with_manifest_error_on_invalid_version() -> Result<()> { err.to_string().contains(&expected), "error should mention manifest loading, got: {err}" ); - let chain: Vec = err.chain().map(ToString::to_string).collect(); + let chain: Vec = err + .chain() + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) + .collect(); let parse_message = localization::message(keys::MANIFEST_PARSE).to_string(); ensure!( chain.iter().any(|s| s.contains(&parse_message)), @@ -107,7 +110,10 @@ fn assert_ninja_failure_propagates(command: Option) -> Result<()> { let Err(err) = run(&cli) else { bail!("expected run to fail when ninja exits non-zero"); }; - let messages: Vec = err.chain().map(ToString::to_string).collect(); + let messages: Vec = err + .chain() + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) + .collect(); ensure!( messages.iter().any(|m| m.contains("ninja exited")), "error should report ninja exit status, got: {messages:?}" diff --git a/tests/runner_tool_subcommands_tests.rs b/tests/runner_tool_subcommands_tests.rs index fcb11fe1..cf3a8a14 100644 --- a/tests/runner_tool_subcommands_tests.rs +++ b/tests/runner_tool_subcommands_tests.rs @@ -54,7 +54,10 @@ fn assert_ninja_failure_propagates(command: Commands) -> Result<()> { let Err(err) = run(&cli) else { bail!("expected run to fail when ninja exits non-zero"); }; - let messages: Vec = err.chain().map(ToString::to_string).collect(); + let messages: Vec = err + .chain() + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) + .collect(); ensure!( messages.iter().any(|m| m.contains("ninja exited")), "error should report ninja exit status, got: {messages:?}" @@ -118,7 +121,10 @@ fn assert_subcommand_fails_with_invalid_manifest( let Err(err) = run(&cli) else { bail!("expected {name} to fail for invalid manifest"); }; - let messages: Vec = err.chain().map(ToString::to_string).collect(); + let messages: Vec = err + .chain() + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) + .collect(); let expected = localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST) .with_arg("path", manifest_path.display().to_string()) .to_string(); diff --git a/tests/yaml_error_tests.rs b/tests/yaml_error_tests.rs index 97c386ca..b4996fde 100644 --- a/tests/yaml_error_tests.rs +++ b/tests/yaml_error_tests.rs @@ -91,7 +91,7 @@ fn yaml_diagnostics_are_actionable(#[case] yaml: &str, #[case] needles: &[&str]) }; let msg = normalise_report( &err.chain() - .map(ToString::to_string) + .map(|e: &(dyn std::error::Error + '_)| e.to_string()) .collect::>() .join("\n"), )?;