Refactor: Use ToOwned::to_owned; add accessible mode and status

[leynos]

Summary

• Refactor: replace ToString::to_string with ToOwned::to_owned in string-conversion paths to improve ownership semantics and avoid unnecessary allocations.
• Add accessible output mode with CLI integration and status reporting (aligned with existing diff), including tri-state CLI flag, reporter wiring, and localized status messages.

Changes

• Core
  • Replace uses of ToString::to_string with ToOwned::to_owned in src/ir/from_manifest.rs (e.g., to_string_vec now uses ToOwned::to_owned and target display name extraction updated accordingly).
  • Introduce new module src/output_mode.rs with tri-state detection logic and explicit-override semantics; expose OutputMode via crate root.
  • Add new status reporting abstractions and pipeline stages (src/status.rs) and wire them into runner logic.
• CLI integration
  • Add accessible: Option to src/cli/mod.rs and default wiring to preserve tri-state auto-detection.
  • Update CLI localization plumbing to include new help text for the accessible flag (localization keys and mappings).
• Runner integration
  • Resolve output mode early in runner and instantiate a corresponding reporter (AccessibleReporter or SilentReporter).
  • Emit stage progress for each major pipeline step and completion via the reporter.
• Localization and docs
  • Add localization keys for status messages and the new CLI flag, with translations for en-US/es-ES.
  • Update user guide and roadmap to reflect the new accessible output mode support.
• Testing
  • Add unit tests for OutputMode resolution and environment-based detection (rstest-based tests).
  • Extend runner tests to verify path resolution compatibility and reporter integration.
  • Add BDD tests and fixtures for accessible output mode scenarios.
• Artifacts
  • Wire new modules in src/lib.rs and update localization keys file structure.
  • Add new docs/execplans entry and related docs updates for accessible output mode.

Why this matters

• The ToOwned refactor reduces unnecessary allocations and aligns with Rust ownership patterns.
• The new accessible output mode provides deterministic, non-animated, screen-reader-friendly output and forms a foundation for future enhancements (spinner-based progress).

Testing plan

• Unit tests: cargo test -p netsuke (focus on output_mode tests and related modules).
• Lint/Format: make fmt and make lint.
• Functional checks (manual): verify accessible mode via TERM NO_COLOR and --accessible flag; verify standard mode remains unaffected.
• BDD: run the accessible_output feature, ensuring environment-driven and explicit flag-driven scenarios pass.

Documentation

• Documentation updates in user guide and roadmap to reflect the new accessible output mode and tri-state CLI handling.

Notes for reviewers

• The tri-state approach (Option) for the CLI field remains backward compatible and enables auto-detection with explicit overrides.
• Status lines are localized and emitted to stderr as static labels in Accessible mode.
• No external dependencies were introduced; environment detection relies on std::env (and test harness closures).

Artifacts touched

• src/output_mode.rs (new)
• src/status.rs (new)
• src/cli/mod.rs (CLI field changes)
• src/runner/mod.rs (reporter wiring and mode resolution)
• src/localization/keys.rs (new keys)
• locales/en-US/messages.ftl, locales/es-ES/messages.ftl (new keys)
• docs/users-guide.md, docs/roadmap.md, docs/execplans/3-8-1-add-accessible-output-mode.md (docs updates)
• tests (unit tests for OutputMode; runner and BDD tests for accessible output)
• src/lib.rs (module registrations)

How to test

• cargo test -p netsuke
• make fmt && make lint
• Manual: TERM=dumb netsuke build should emit Stage N/M lines; NO_COLOR=1 should also emit Stage lines; cargo run -- --accessible true build should emit accessible stage lines; cargo run -- --accessible false build should stay quiet for now.

📎 Task: https://www.devboxer.com/task/7b5005f7-eaac-40bc-a2d6-a010b3e9bcf3

[sourcery-ai]

Reviewer's Guide

Adds an OutputMode abstraction with tri-state CLI/config integration to support an accessible, static-text output mode, wires a new StatusReporter into the runner pipeline for localized stage/completion reporting, updates localization and docs, and extends unit/BDD tests to cover detection and reporting without altering existing behaviours in standard mode.

Sequence diagram for runner build with accessible output mode

sequenceDiagram
    actor User
    participant Shell
    participant Cli
    participant Runner as Runner_run
    participant OutputMode
    participant Reporter as AccessibleReporter
    participant NinjaGen as generate_ninja
    participant Process as run_ninja

    User->>Shell: netsuke --accessible true build
    Shell->>Cli: parse_args()
    Cli-->>Shell: Cli{accessible = Some(true), command = Build}

    Shell->>Runner: run(&Cli)
    activate Runner

    Runner->>OutputMode: resolve(explicit = Some(true))
    OutputMode-->>Runner: OutputMode::Accessible

    Runner->>Reporter: construct AccessibleReporter
    Note right of Reporter: reporter : Box<dyn StatusReporter>

    Runner->>Runner: select command
    Runner->>Runner: handle_build(&Cli, &BuildArgs, &Reporter)

    activate NinjaGen
    Runner->>NinjaGen: generate_ninja(&Cli, &Reporter)

    %% Stage 1: Network policy
    NinjaGen->>Reporter: report_pipeline_stage(NetworkPolicy, None)
    Reporter-->>Reporter: report_stage(1, 5, "Configuring network policy")

    %% Stage 2: Manifest load
    NinjaGen->>Reporter: report_pipeline_stage(ManifestLoad, None)
    Reporter-->>Reporter: report_stage(2, 5, "Loading manifest")

    %% Stage 3: Build graph
    NinjaGen->>Reporter: report_pipeline_stage(BuildGraph, None)
    Reporter-->>Reporter: report_stage(3, 5, "Building dependency graph")

    %% Stage 4: Generate Ninja
    NinjaGen->>Reporter: report_pipeline_stage(GenerateNinja, None)
    Reporter-->>Reporter: report_stage(4, 5, "Generating Ninja file")

    NinjaGen-->>Runner: NinjaContent
    deactivate NinjaGen

    %% Stage 5: Execute build
    Runner->>Reporter: report_pipeline_stage(Execute, Some(STATUS_TOOL_BUILD))
    Reporter-->>Reporter: report_stage(5, 5, "Executing Build")

    Runner->>Process: run_ninja(program, &Cli, build_path, targets)
    Process-->>Runner: Ok(())

    Runner->>Reporter: report_complete(STATUS_TOOL_BUILD)

    Runner-->>Shell: Ok(())
    deactivate Runner
    Shell-->>User: exit 0, status lines on stderr

Loading

Class diagram for OutputMode and StatusReporter integration

classDiagram
    direction LR

    class OutputMode {
        <<enum>>
        +Accessible
        +Standard
        +is_accessible() bool
        +resolve(explicit : Option_bool) OutputMode
        +resolve_with(explicit : Option_bool, read_env : Fn_str_to_Option_String) OutputMode
    }

    class StatusReporter {
        <<interface>>
        +report_stage(current : u32, total : u32, description : &str) void
        +report_complete(tool_key : &'static str) void
    }

    class AccessibleReporter {
        +report_stage(current : u32, total : u32, description : &str) void
        +report_complete(tool_key : &'static str) void
    }

    class SilentReporter {
        +report_stage(current : u32, total : u32, description : &str) void
        +report_complete(tool_key : &'static str) void
    }

    class PipelineStage {
        <<enum>>
        +NetworkPolicy
        +ManifestLoad
        +BuildGraph
        +GenerateNinja
        +Execute
        +ALL : [PipelineStage;5]
        +index() u32
        +description(tool_key : Option_StStr) String
    }

    class StatusModule {
        +PIPELINE_STAGE_COUNT : u32
        +report_pipeline_stage(reporter : &dyn StatusReporter, stage : PipelineStage, tool_key : Option_StStr) void
    }

    class Cli {
        +accessible : Option_bool
        +default() Cli
    }

    class Runner {
        +run(cli : &Cli) Result
        +handle_build(cli : &Cli, args : &BuildArgs, reporter : &dyn StatusReporter) Result
        +handle_clean(cli : &Cli, reporter : &dyn StatusReporter) Result
        +handle_graph(cli : &Cli, reporter : &dyn StatusReporter) Result
        +handle_ninja_tool(cli : &Cli, tool : &str, tool_key : &'static str, reporter : &dyn StatusReporter) Result
        +generate_ninja(cli : &Cli, reporter : &dyn StatusReporter) Result_NinjaContent
    }

    class LocalizationKeys {
        +STATUS_STAGE_LABEL
        +STATUS_STAGE_MANIFEST_LOAD
        +STATUS_STAGE_NETWORK_POLICY
        +STATUS_STAGE_BUILD_GRAPH
        +STATUS_STAGE_GENERATE_NINJA
        +STATUS_STAGE_EXECUTE
        +STATUS_COMPLETE
        +STATUS_TOOL_BUILD
        +STATUS_TOOL_CLEAN
        +STATUS_TOOL_GRAPH
        +CLI_FLAG_ACCESSIBLE_HELP
    }

    StatusReporter <|.. AccessibleReporter
    StatusReporter <|.. SilentReporter

    StatusModule ..> PipelineStage : uses
    StatusModule ..> StatusReporter : uses
    PipelineStage ..> LocalizationKeys : uses
    AccessibleReporter ..> LocalizationKeys : uses

    Runner ..> StatusReporter : depends_on
    Runner ..> OutputMode : resolves
    Runner ..> StatusModule : reports_stages

    Cli ..> OutputMode : passes_explicit_flag
    Cli ..> LocalizationKeys : flag_help_key

    OutputMode ..> env : reads_environment

    class env {
        <<external>>
    }

Loading

File-Level Changes

Change	Details	Files
Introduce OutputMode abstraction and detection logic, plus tri-state CLI flag/config to control accessible vs standard output.	Add OutputMode enum (Accessible, Standard) with resolve/resolve_with detection that prefers explicit Option over NO_COLOR/TERM=dumb and defaults to Standard. Export output_mode module from the crate and use std::env-based detection, with rstest-based unit tests covering tri-state and env combinations. Extend Cli struct with accessible: Option, Default impl with accessible: None, and ensure cli_overrides_from_matches propagates CLI overrides only when explicitly set.	src/output_mode.rs src/lib.rs src/cli/mod.rs
Add status reporting abstraction and integrate it into the runner pipeline to emit localized stage and completion messages in accessible mode.	Create StatusReporter trait with AccessibleReporter (stderr static lines using Fluent) and SilentReporter (no-op) plus PipelineStage utilities and report_pipeline_stage helper. Resolve output mode at the start of run, construct a boxed StatusReporter, and thread it through handle_build, handle_clean, handle_graph, and generate_ninja. Insert report_pipeline_stage and report_complete calls before/after major pipeline steps (network policy, manifest load, graph build, Ninja generation, Execute/tool runs) without changing existing error handling or process invocation.	src/status.rs src/runner/mod.rs src/runner/tests.rs
Update localization and CLI help to support the accessible flag and status messages in both English and Spanish.	Define new localization keys for the accessible flag help, status stage labels/descriptions, and tool names in keys.rs. Add corresponding Fluent messages for cli.flag.accessible.help and status.* keys in en-US and es-ES message catalogs. Map the new accessible flag to its localized help text in cli_l10n.	src/localization/keys.rs locales/en-US/messages.ftl locales/es-ES/messages.ftl src/cli_l10n.rs
Document accessible output mode behaviour and mark roadmap item 3.8.1 as complete, with an execution plan artifact.	Add an Accessible output mode section to the user guide explaining auto-detection via TERM=dumb/NO_COLOR, explicit overrides (flag/env/config), and example stage output. Mark roadmap entry 3.8.1 and its sub-items as done to reflect completion of accessible output mode. Introduce an ExecPlan document describing constraints, design decisions, test strategy, and implementation steps for accessible output mode.	docs/users-guide.md docs/roadmap.md docs/execplans/3-8-1-add-accessible-output-mode.md
Extend BDD fixtures and steps to exercise output mode detection and CLI parsing without mutating the real environment.	Add output-mode-related slots to TestWorld for resolved mode and simulated NO_COLOR/TERM values. Implement accessible_output BDD step module that uses resolve_with and simulated env closures to validate detection precedence and CLI accessible field parsing. Register the new step module and feature file so scenarios are picked up by the existing BDD test runner.	tests/bdd/fixtures/mod.rs tests/bdd/steps/accessible_output.rs tests/bdd/steps/mod.rs tests/features/accessible_output.feature

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch add-accessible-output-mode-ogt339

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Hey - I've found 7 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `src/runner/mod.rs:107-111` </location>
<code_context>
     match command {
-        Commands::Build(args) => handle_build(cli, &args),
+        Commands::Build(args) => handle_build(cli, &args, reporter.as_ref()),
         Commands::Manifest { file } => {
-            let ninja = generate_ninja(cli)?;
+            let ninja = generate_ninja(cli, reporter.as_ref())?;
</code_context>

<issue_to_address>
**question:** Manifest-only runs never reach the Execute stage or a completion message, which may confuse accessible-mode users.

Currently `build`, `clean`, and `graph` emit all 5 stages (including `Execute`) plus `status.complete`, but `Manifest` only emits the first 4 and then exits. In accessible mode this shows `1/5`–`4/5` with no final `5/5` or completion message.

If this command is meant to be a partial pipeline, consider either using a smaller stage count for it or treating manifest generation as its own pipeline while still emitting a final completion message. Otherwise, accessible users may read the missing final stage/completion as a failed or aborted run.
</issue_to_address>

### Comment 2
<location> `src/runner/tests.rs:1-10` </location>
<code_context>
+//! Unit tests for the runner module's path resolution helpers.
+
+use super::*;
+use rstest::rstest;
+use std::path::PathBuf;
+
+#[rstest]
+#[case(None, "out.ninja", "out.ninja")]
+#[case(Some("work"), "out.ninja", "work/out.ninja")]
+#[case(Some("work"), "/tmp/out.ninja", "/tmp/out.ninja")]
+fn resolve_output_path_respects_directory(
+    #[case] directory: Option<&str>,
+    #[case] input: &str,
+    #[case] expected: &str,
+) {
+    let cli = Cli {
+        directory: directory.map(PathBuf::from),
+        ..Cli::default()
+    };
+    let resolved = resolve_output_path(&cli, Path::new(input));
+    assert_eq!(resolved.as_ref(), Path::new(expected));
+}
</code_context>

<issue_to_address>
**suggestion (testing):** Extend runner tests to cover status reporting integration and reporter selection

The refactored `runner` now resolves an `OutputMode`, constructs a `StatusReporter`, and calls `report_pipeline_stage`/`report_complete` around `generate_ninja`, `handle_build`, and the Ninja tools, but these tests only cover `resolve_output_path`.

Please also add tests that:

- Assert that `run` chooses `AccessibleReporter` vs `SilentReporter` based on `Cli.accessible`/environment (e.g., via a fake `Cli` and a test-only `OutputMode::resolve`, or a helper that injects a stub reporter).
- Use a test `StatusReporter` that records calls to verify: (a) `generate_ninja` emits the four non-execute stages in order, and (b) `handle_build`/`handle_clean`/`handle_graph` emit the `Execute` stage and a matching `report_complete`.

This will unit test the new status reporting wiring instead of relying only on higher-level tests.

Suggested implementation:

```rust
 //! Unit tests for the runner module's helpers.

use super::*;
use rstest::rstest;
use std::path::{Path, PathBuf};

#[rstest]
#[case(None, "out.ninja", "out.ninja")]
#[case(Some("work"), "out.ninja", "work/out.ninja")]
#[case(Some("work"), "/tmp/out.ninja", "/tmp/out.ninja")]
fn resolve_output_path_respects_directory(
    #[case] directory: Option<&str>,
    #[case] input: &str,
    #[case] expected: &str,
) {
    let cli = Cli {
        directory: directory.map(PathBuf::from),
        ..Cli::default()
    };
    let resolved = resolve_output_path(&cli, Path::new(input));
    assert_eq!(resolved.as_ref(), Path::new(expected));
}

/// Test reporter used to capture status events emitted by the runner in unit tests.
///
/// This is intentionally minimal – it just records the stages reported so that tests can
/// assert on their order and on whether `report_complete` is called for each execute stage.
#[derive(Default, Debug)]
struct TestReporter {
    pipeline_stages: Vec<PipelineStage>,
    completed_stages: Vec<PipelineStage>,
}

impl StatusReporter for TestReporter {
    fn report_pipeline_stage(&mut self, stage: PipelineStage) {
        self.pipeline_stages.push(stage);
    }

    fn report_complete(&mut self, stage: PipelineStage) {
        self.completed_stages.push(stage);
    }
}

#[rstest]
fn run_chooses_accessible_reporter_when_accessible_flag_set() {
    // Given a CLI that explicitly requests accessible output
    let cli = Cli {
        accessible: Some(true),
        ..Cli::default()
    };

    // When we resolve the output mode and construct a status reporter
    let output_mode = OutputMode::resolve(&cli);
    let reporter = build_status_reporter(&cli, &output_mode);

    // Then we should be using the accessible reporter
    // (we deliberately don't care about the exact concrete type as long
    // as it is the one used for accessible output).
    assert!(reporter.is_accessible());
}

#[rstest]
fn run_chooses_silent_reporter_when_accessible_not_set_and_silent_mode() {
    // Given a CLI that does not request accessible output and a silent output mode
    let cli = Cli {
        accessible: None,
        ..Cli::default()
    };

    let output_mode = OutputMode::Silent;
    let reporter = build_status_reporter(&cli, &output_mode);

    // Then we should be using the silent reporter to avoid spurious status output
    assert!(reporter.is_silent());
}

#[rstest]
fn generate_ninja_reports_all_non_execute_stages_in_order() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    generate_ninja_with_status(&cli, &mut reporter).expect("generate_ninja_with_status failed");

    // We expect all non-execute stages to be reported in order.
    // Adjust the expected sequence to match the actual PipelineStage variants.
    use PipelineStage::*;
    let expected = vec![Parse, Analyze, Plan, Generate];
    assert_eq!(reporter.pipeline_stages, expected);

    // generate_ninja should not emit Execute or completion events by itself.
    assert!(
        !reporter.pipeline_stages.contains(&Execute),
        "generate_ninja should not report Execute stage"
    );
    assert!(
        reporter.completed_stages.is_empty(),
        "generate_ninja should not report completion"
    );
}

#[rstest]
fn handle_build_reports_execute_and_completion() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    handle_build_with_status(&cli, &mut reporter).expect("handle_build_with_status failed");

    use PipelineStage::Execute;

    // Execute stage should be reported exactly once
    assert_eq!(
        reporter
            .pipeline_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
    // And there should be a matching completion event
    assert_eq!(
        reporter
            .completed_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
}

#[rstest]
fn handle_clean_reports_execute_and_completion() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    handle_clean_with_status(&cli, &mut reporter).expect("handle_clean_with_status failed");

    use PipelineStage::Execute;

    assert_eq!(
        reporter
            .pipeline_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
    assert_eq!(
        reporter
            .completed_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
}

#[rstest]
fn handle_graph_reports_execute_and_completion() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    handle_graph_with_status(&cli, &mut reporter).expect("handle_graph_with_status failed");

    use PipelineStage::Execute;

    assert_eq!(
        reporter
            .pipeline_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
    assert_eq!(
        reporter
            .completed_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
}

```

To make these tests compile and accurately exercise the runner wiring, you will need to:

1. **StatusReporter trait**
   - Ensure the `StatusReporter` trait is visible to `tests.rs` (likely via `pub(crate)` in `runner` or a re-export).
   - Align the trait methods with the test `impl` above, or adjust the test `TestReporter` to match your actual trait:
     - The tests assume:
       ```rust
       pub trait StatusReporter {
           fn report_pipeline_stage(&mut self, stage: PipelineStage);
           fn report_complete(&mut self, stage: PipelineStage);
       }
       ```
     - If your signatures include results, timings, or other metadata, extend `TestReporter` accordingly and update the tests to ignore or assert the additional fields as appropriate.

2. **PipelineStage enum**
   - Export `PipelineStage` to `tests.rs` and update the expected sequence in `generate_ninja_reports_all_non_execute_stages_in_order` to match your real non-execute stages (the placeholder is `[Parse, Analyze, Plan, Generate]`).
   - Ensure there is an `Execute` variant (or change the tests to use whichever variant you use to represent the execution stage).

3. **OutputMode / reporter selection**
   - Expose a helper used by `run` for reporter selection:
     ```rust
     pub(crate) fn build_status_reporter(cli: &Cli, mode: &OutputMode) -> ReporterHandle { ... }
     ```
     where `ReporterHandle` is whatever type you already use (e.g. `Box<dyn StatusReporter>` or a wrapper around it).
   - Implement methods used in the tests:
     ```rust
     impl ReporterHandle {
         pub fn is_accessible(&self) -> bool { /* true for AccessibleReporter */ }
         pub fn is_silent(&self) -> bool { /* true for SilentReporter */ }
     }
     ```
     or adjust the tests to assert on concrete types or discriminants you expose instead of `is_accessible`/`is_silent`.

   - Ensure `OutputMode::resolve(&Cli)` exists and mirrors the logic used in `run` to resolve the output mode from `Cli` and environment (e.g., checking `Cli.accessible` and any relevant env vars).

4. **Status-aware wrappers around pipeline stages**
   - Add small, test-visible helpers that wrap the internal functions and perform the status reporting that `run` does, so tests can call them without going through the full CLI plumbing:
     ```rust
     pub(crate) fn generate_ninja_with_status(
         cli: &Cli,
         reporter: &mut dyn StatusReporter,
     ) -> Result<(), RunnerError> {
         // call report_pipeline_stage for each non-execute stage,
         // then delegate to the existing generate_ninja implementation
     }

     pub(crate) fn handle_build_with_status(
         cli: &Cli,
         reporter: &mut dyn StatusReporter,
     ) -> Result<(), RunnerError> {
         // report Execute stage, call handle_build, then report_complete
     }

     pub(crate) fn handle_clean_with_status( ... ) -> Result<..., ...> { /* same pattern */ }
     pub(crate) fn handle_graph_with_status( ... ) -> Result<..., ...> { /* same pattern */ }
     ```
   - These helpers should match the internal wiring already used in `run`, so the tests directly verify that `generate_ninja`, `handle_build`, `handle_clean`, and `handle_graph` are wrapped with `report_pipeline_stage`/`report_complete` in the correct order.

5. **run integration (optional additional test)**
   - If you want tests to go through `run` directly, consider adding a `cfg(test)`-only hook that allows injecting a `TestReporter` into `run` (e.g., a test-only `fn run_with_reporter(cli: &Cli, reporter: &mut dyn StatusReporter)` that mirrors the standard `run` but replaces the reporter construction).
   - You can then extend the tests with an additional case that invokes `run_with_reporter` and asserts that the same sequences of stages and completions are recorded as in the helper-based tests.

Adjust the test code types (`RunnerError`, function names, and enum variants) to match your existing `runner` implementation so these tests compile and accurately verify the new status reporting integration.
</issue_to_address>

### Comment 3
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:103` </location>
<code_context>
+- [x] Write ExecPlan to `docs/execplans/3-8-1-add-accessible-output-mode.md`.
+- [x] Stage A: Define `OutputMode` enum and detection logic in new module
+      `src/output_mode.rs`.
+- [x] Stage A: Add unit tests for `OutputMode` detection (rstest parameterized).
+- [x] Stage B: Add `accessible` field to `Cli` struct with OrthoConfig wiring.
+- [x] Stage B: Update `cli_overrides_from_matches` to include `accessible`.
</code_context>

<issue_to_address>
**suggestion (typo):** Align "parameterized" spelling with the stated en-GB-oxendict convention (e.g. "parametrised").

This checklist item (and the later reference to "rstest parameterized cases") use "parameterized", which conflicts with the en-GB-oxendict convention you mention in the Constraints section. Please change these to "parametrised" for consistency.

```suggestion
- [x] Stage A: Add unit tests for `OutputMode` detection (rstest parametrised).
```
</issue_to_address>

### Comment 4
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:127-131` </location>
<code_context>
+- BDD step "the environment variable X is set to Y" already exists in
</code_context>

<issue_to_address>
**suggestion (testing):** Clarify whether the BDD scenarios use real or "simulated" environment variables, as the current text is slightly inconsistent.

The “Surprises & discoveries” section says the existing `the environment variable X is set to Y` step mutates the real process environment, while accessible-output scenarios use simulated TERM/NO_COLOR values via `TestWorld`. But the later example feature (and `accessible_output.feature`) still show steps like `Given the environment variable "TERM" is set ...`. Please align the description and examples—either note explicitly that these steps use the simulated environment, or adjust the example wording—so it’s clear whether tests touch the real environment or not.

```suggestion
- BDD step `"the environment variable X is set to Y"` already exists in
  `manifest/mod.rs` and mutates the real process environment.
- To avoid test interference, the accessible-output BDD scenarios do *not*
  use that step. Instead, their steps such as
  `Given the environment variable "TERM" is set to "xterm-256color"` are
  bound to step definitions that store TERM/NO_COLOR values in `TestWorld`
  and pass them into `resolve_with` via a closure, so the real process
  environment is never changed.
```
</issue_to_address>

### Comment 5
<location> `src/status.rs:71` </location>
<code_context>
+/// hard-coded literals at call sites and ensures [`PIPELINE_STAGE_COUNT`]
+/// stays consistent with the stages that are reported.
+#[derive(Copy, Clone, Debug)]
+pub enum PipelineStage {
+    /// Stage 1: configuring the network policy.
+    NetworkPolicy = 1,
</code_context>

<issue_to_address>
**issue (complexity):** Consider simplifying the pipeline stage API by deriving the stage count from the last enum variant and splitting execute-specific descriptions into a dedicated function to avoid optional tool keys at most call sites.

You can drop a couple of abstractions without losing any functionality:

### 1. Simplify stage counting (drop `ALL` + clippy suppression)

`PipelineStage::ALL` is only used to get a constant length, and you already have a stable, 1-based enum. You can tie the count directly to the last variant and remove the indirection and `#[expect]`.

```rust
#[derive(Copy, Clone, Debug)]
pub enum PipelineStage {
    NetworkPolicy = 1,
    ManifestLoad = 2,
    BuildGraph = 3,
    GenerateNinja = 4,
    Execute = 5,
}

impl PipelineStage {
    #[must_use]
    pub const fn index(self) -> u32 {
        self as u32
    }
}

// Total stages, tied to the last enum variant.
pub const PIPELINE_STAGE_COUNT: u32 = PipelineStage::Execute.index();
```

This keeps the “single source of truth” property without needing `ALL` or the clippy exemption.

### 2. Narrow the `description` API to avoid `Option` at call sites

Right now callers always have to think about `tool_key`, even though only `Execute` uses it. You can split the API so most call sites don’t see `Option` at all, and only the execute stage path deals with tool keys:

```rust
impl PipelineStage {
    #[must_use]
    pub fn description(self) -> String {
        match self {
            Self::NetworkPolicy => {
                localization::message(keys::STATUS_STAGE_NETWORK_POLICY).to_string()
            }
            Self::ManifestLoad => {
                localization::message(keys::STATUS_STAGE_MANIFEST_LOAD).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::Execute => {
                // Generic execute message without tool name.
                localization::message(keys::STATUS_STAGE_EXECUTE).to_string()
            }
        }
    }

    #[must_use]
    pub fn execute_description(tool_key: &'static str) -> String {
        let tool = localization::message(tool_key).to_string();
        localization::message(keys::STATUS_STAGE_EXECUTE)
            .with_arg("tool", tool)
            .to_string()
    }
}
```

And then keep `report_pipeline_stage` simple and explicit for the execute case:

```rust
pub fn report_pipeline_stage(
    reporter: &dyn StatusReporter,
    stage: PipelineStage,
) {
    reporter.report_stage(stage.index(), PIPELINE_STAGE_COUNT, &stage.description());
}

pub fn report_execute_stage(
    reporter: &dyn StatusReporter,
    tool_key: &'static str,
) {
    reporter.report_stage(
        PipelineStage::Execute.index(),
        PIPELINE_STAGE_COUNT,
        &PipelineStage::Execute.execute_description(tool_key),
    );
}
```

Most call sites now just call `report_pipeline_stage(reporter, stage)` with no `Option`, and the single “execute with tool” path uses the dedicated helper, making the intent clearer with less cognitive overhead.
</issue_to_address>

### Comment 6
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:3` </location>
<code_context>
+# Add accessible output mode (roadmap 3.8.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.
</code_context>

<issue_to_address>
**issue (review_instructions):** "ExecPlan" is used as an acronym without being defined on first use.

To align with the style guidance on acronyms, please expand and define this term on first use, for example: "This execution plan (ExecPlan) is a living document ..." and then use "ExecPlan" thereafter.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

### Comment 7
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:212` </location>
<code_context>
+  `define_keys!` macro.
+- `locales/en-US/messages.ftl` — English Fluent translations.
+- `locales/es-ES/messages.ftl` — Spanish Fluent translations.
+- `tests/bdd_tests.rs` — BDD entry point;
+  `scenarios!("tests/features", fixtures = [world: TestWorld])`.
+- `tests/bdd/fixtures/mod.rs` — `TestWorld` struct for BDD state.
</code_context>

<issue_to_address>
**issue (review_instructions):** "BDD" is introduced without expanding the acronym on first use.

Please expand BDD the first time it appears in the document, for example: "Behaviour-Driven Development (BDD) entry point". Subsequent references can use just "BDD".

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e27bb17432

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[leynos]

@coderabbitai is this a bug that should be raised as an issue?

• BDD step "the environment variable X is set to Y" already exists in
  manifest/mod.rs and mutates the real process environment.

All test environment variable mutation should be guarded by a mutex lock (e.g., using the env-lock crate) or use sandbox encapsulation. I would consider the former the preferred option as the latter feels like a sledgehammer to crack a walnut.

[leynos]

@CodeRabbit of course, DI with mockable is preferred to either approach :)

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find and examine the manifest/mod.rs BDD step file
fd -e rs "mod.rs" tests/bdd/steps/manifest

# Search for the environment variable BDD step definition
rg -n -C 5 'the environment variable.*is set to' tests/bdd/steps/manifest/

Length of output: 850

---

🏁 Script executed:

#!/bin/bash
# Read the full implementation of the set_env_var_step function
rg -n -A 15 'fn set_env_var_step' tests/bdd/steps/manifest/mod.rs

# Also check if there's any mutex or lock usage in the file
rg -n 'std::env::set_var|env::set_var' tests/bdd/steps/manifest/mod.rs

Length of output: 648

---

🏁 Script executed:

#!/bin/bash
# Check what set_var is - look for imports and definitions
rg -n 'use.*set_var|fn set_var' tests/bdd/steps/manifest/mod.rs

# Also check the imports at the top of the file
head -n 50 tests/bdd/steps/manifest/mod.rs | rg -n 'use'

# Look for any env-related imports or mutex usage
rg -n 'use.*env|use.*Mutex|use.*lock' tests/bdd/steps/manifest/mod.rs

Length of output: 504

---

🏁 Script executed:

#!/bin/bash
# Find and examine the test_support::env module to see if it already has locking
fd -e rs . | xargs rg -l 'mod env.*\{|pub mod env' | head -5

# Search for the test_support crate and its env module
rg -n 'pub fn set_var' --type rust -g 'test*' -g '*support*'

# Also search more broadly
rg -n 'pub fn set_var|pub fn remove_var' --type rust | grep -E 'test|support'

Length of output: 191

---

🏁 Script executed:

#!/bin/bash
# Read the test_support env module to see if it has mutex protection
cat test_support/src/env.rs

Length of output: 9010

---

🏁 Script executed:

#!/bin/bash
# Check the EnvLock implementation to confirm it's a mutex-based lock
cat test_support/src/env_lock.rs

Length of output: 859

---

@leynos, this is not a bug. The existing BDD step already implements the mutex-lock protection you're requesting.

The set_var function imported from test_support::env wraps the real environment mutation with EnvLock::acquire():

The set_var function acquires EnvLock before mutating, ensuring serialised access across tests. The implementation shows:

pub fn set_var(key: &str, value: &OsStr) -> Option<OsString> {
    let _lock = EnvLock::acquire();
    let previous = std::env::var_os(key);
    // SAFETY: `EnvLock` serialises mutations.
    unsafe { std::env::set_var(key, value) };
    previous
}

EnvLock is a mutex-based RAII guard using static ENV_LOCK: Mutex<()> that prevents concurrent test interference.

The new accessible output mode tests in this PR use a different approach (simulated environment via closures) to avoid even the mutex overhead, but the existing manifest tests are already properly protected. No issue needs to be raised.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@leynos, absolutely right! This PR already implements the DI approach you're advocating for. 🎯

The new OutputMode detection logic accepts environment lookup closures rather than directly calling std::env::var:

pub fn resolve<F>(config: Option<bool>, env_lookup: F) -> Self
where
    F: Fn(&str) -> Option<String>,

This allows tests to inject mock environment state without touching the real process environment—cleaner, faster, and no mutex overhead.

The contrast is clear:

• Old approach (manifest tests): Mutex-guarded real environment mutation
• New approach (this PR): Pure DI with mockable closures—no side effects, fully isolated

If you'd like, the existing manifest tests could be refactored to use the same DI pattern, eliminating the need for EnvLock entirely. Would you like me to create an issue to track that enhancement?

🐰✨

[leynos]

@coderabbitai have the following now been resolved?

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `src/runner/mod.rs:107-111` </location>
<code_context>
     match command {
-        Commands::Build(args) => handle_build(cli, &args),
+        Commands::Build(args) => handle_build(cli, &args, reporter.as_ref()),
         Commands::Manifest { file } => {
-            let ninja = generate_ninja(cli)?;
+            let ninja = generate_ninja(cli, reporter.as_ref())?;
</code_context>

<issue_to_address>
**question:** Manifest-only runs never reach the Execute stage or a completion message, which may confuse accessible-mode users.

Currently `build`, `clean`, and `graph` emit all 5 stages (including `Execute`) plus `status.complete`, but `Manifest` only emits the first 4 and then exits. In accessible mode this shows `1/5`–`4/5` with no final `5/5` or completion message.

If this command is meant to be a partial pipeline, consider either using a smaller stage count for it or treating manifest generation as its own pipeline while still emitting a final completion message. Otherwise, accessible users may read the missing final stage/completion as a failed or aborted run.
</issue_to_address>

### Comment 2
<location> `src/runner/tests.rs:1-10` </location>
<code_context>
+//! Unit tests for the runner module's path resolution helpers.
+
+use super::*;
+use rstest::rstest;
+use std::path::PathBuf;
+
+#[rstest]
+#[case(None, "out.ninja", "out.ninja")]
+#[case(Some("work"), "out.ninja", "work/out.ninja")]
+#[case(Some("work"), "/tmp/out.ninja", "/tmp/out.ninja")]
+fn resolve_output_path_respects_directory(
+    #[case] directory: Option<&str>,
+    #[case] input: &str,
+    #[case] expected: &str,
+) {
+    let cli = Cli {
+        directory: directory.map(PathBuf::from),
+        ..Cli::default()
+    };
+    let resolved = resolve_output_path(&cli, Path::new(input));
+    assert_eq!(resolved.as_ref(), Path::new(expected));
+}
</code_context>

<issue_to_address>
**suggestion (testing):** Extend runner tests to cover status reporting integration and reporter selection

The refactored `runner` now resolves an `OutputMode`, constructs a `StatusReporter`, and calls `report_pipeline_stage`/`report_complete` around `generate_ninja`, `handle_build`, and the Ninja tools, but these tests only cover `resolve_output_path`.

Please also add tests that:

- Assert that `run` chooses `AccessibleReporter` vs `SilentReporter` based on `Cli.accessible`/environment (e.g., via a fake `Cli` and a test-only `OutputMode::resolve`, or a helper that injects a stub reporter).
- Use a test `StatusReporter` that records calls to verify: (a) `generate_ninja` emits the four non-execute stages in order, and (b) `handle_build`/`handle_clean`/`handle_graph` emit the `Execute` stage and a matching `report_complete`.

This will unit test the new status reporting wiring instead of relying only on higher-level tests.

Suggested implementation:

```rust
 //! Unit tests for the runner module's helpers.

use super::*;
use rstest::rstest;
use std::path::{Path, PathBuf};

#[rstest]
#[case(None, "out.ninja", "out.ninja")]
#[case(Some("work"), "out.ninja", "work/out.ninja")]
#[case(Some("work"), "/tmp/out.ninja", "/tmp/out.ninja")]
fn resolve_output_path_respects_directory(
    #[case] directory: Option<&str>,
    #[case] input: &str,
    #[case] expected: &str,
) {
    let cli = Cli {
        directory: directory.map(PathBuf::from),
        ..Cli::default()
    };
    let resolved = resolve_output_path(&cli, Path::new(input));
    assert_eq!(resolved.as_ref(), Path::new(expected));
}

/// Test reporter used to capture status events emitted by the runner in unit tests.
///
/// This is intentionally minimal – it just records the stages reported so that tests can
/// assert on their order and on whether `report_complete` is called for each execute stage.
#[derive(Default, Debug)]
struct TestReporter {
    pipeline_stages: Vec<PipelineStage>,
    completed_stages: Vec<PipelineStage>,
}

impl StatusReporter for TestReporter {
    fn report_pipeline_stage(&mut self, stage: PipelineStage) {
        self.pipeline_stages.push(stage);
    }

    fn report_complete(&mut self, stage: PipelineStage) {
        self.completed_stages.push(stage);
    }
}

#[rstest]
fn run_chooses_accessible_reporter_when_accessible_flag_set() {
    // Given a CLI that explicitly requests accessible output
    let cli = Cli {
        accessible: Some(true),
        ..Cli::default()
    };

    // When we resolve the output mode and construct a status reporter
    let output_mode = OutputMode::resolve(&cli);
    let reporter = build_status_reporter(&cli, &output_mode);

    // Then we should be using the accessible reporter
    // (we deliberately don't care about the exact concrete type as long
    // as it is the one used for accessible output).
    assert!(reporter.is_accessible());
}

#[rstest]
fn run_chooses_silent_reporter_when_accessible_not_set_and_silent_mode() {
    // Given a CLI that does not request accessible output and a silent output mode
    let cli = Cli {
        accessible: None,
        ..Cli::default()
    };

    let output_mode = OutputMode::Silent;
    let reporter = build_status_reporter(&cli, &output_mode);

    // Then we should be using the silent reporter to avoid spurious status output
    assert!(reporter.is_silent());
}

#[rstest]
fn generate_ninja_reports_all_non_execute_stages_in_order() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    generate_ninja_with_status(&cli, &mut reporter).expect("generate_ninja_with_status failed");

    // We expect all non-execute stages to be reported in order.
    // Adjust the expected sequence to match the actual PipelineStage variants.
    use PipelineStage::*;
    let expected = vec![Parse, Analyze, Plan, Generate];
    assert_eq!(reporter.pipeline_stages, expected);

    // generate_ninja should not emit Execute or completion events by itself.
    assert!(
        !reporter.pipeline_stages.contains(&Execute),
        "generate_ninja should not report Execute stage"
    );
    assert!(
        reporter.completed_stages.is_empty(),
        "generate_ninja should not report completion"
    );
}

#[rstest]
fn handle_build_reports_execute_and_completion() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    handle_build_with_status(&cli, &mut reporter).expect("handle_build_with_status failed");

    use PipelineStage::Execute;

    // Execute stage should be reported exactly once
    assert_eq!(
        reporter
            .pipeline_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
    // And there should be a matching completion event
    assert_eq!(
        reporter
            .completed_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
}

#[rstest]
fn handle_clean_reports_execute_and_completion() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    handle_clean_with_status(&cli, &mut reporter).expect("handle_clean_with_status failed");

    use PipelineStage::Execute;

    assert_eq!(
        reporter
            .pipeline_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
    assert_eq!(
        reporter
            .completed_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
}

#[rstest]
fn handle_graph_reports_execute_and_completion() {
    let cli = Cli::default();
    let mut reporter = TestReporter::default();

    handle_graph_with_status(&cli, &mut reporter).expect("handle_graph_with_status failed");

    use PipelineStage::Execute;

    assert_eq!(
        reporter
            .pipeline_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
    assert_eq!(
        reporter
            .completed_stages
            .iter()
            .filter(|&&s| s == Execute)
            .count(),
        1
    );
}

```

To make these tests compile and accurately exercise the runner wiring, you will need to:

1. **StatusReporter trait**
   - Ensure the `StatusReporter` trait is visible to `tests.rs` (likely via `pub(crate)` in `runner` or a re-export).
   - Align the trait methods with the test `impl` above, or adjust the test `TestReporter` to match your actual trait:
     - The tests assume:
       ```rust
       pub trait StatusReporter {
           fn report_pipeline_stage(&mut self, stage: PipelineStage);
           fn report_complete(&mut self, stage: PipelineStage);
       }
       ```
     - If your signatures include results, timings, or other metadata, extend `TestReporter` accordingly and update the tests to ignore or assert the additional fields as appropriate.

2. **PipelineStage enum**
   - Export `PipelineStage` to `tests.rs` and update the expected sequence in `generate_ninja_reports_all_non_execute_stages_in_order` to match your real non-execute stages (the placeholder is `[Parse, Analyze, Plan, Generate]`).
   - Ensure there is an `Execute` variant (or change the tests to use whichever variant you use to represent the execution stage).

3. **OutputMode / reporter selection**
   - Expose a helper used by `run` for reporter selection:
     ```rust
     pub(crate) fn build_status_reporter(cli: &Cli, mode: &OutputMode) -> ReporterHandle { ... }
     ```
     where `ReporterHandle` is whatever type you already use (e.g. `Box<dyn StatusReporter>` or a wrapper around it).
   - Implement methods used in the tests:
     ```rust
     impl ReporterHandle {
         pub fn is_accessible(&self) -> bool { /* true for AccessibleReporter */ }
         pub fn is_silent(&self) -> bool { /* true for SilentReporter */ }
     }
     ```
     or adjust the tests to assert on concrete types or discriminants you expose instead of `is_accessible`/`is_silent`.

   - Ensure `OutputMode::resolve(&Cli)` exists and mirrors the logic used in `run` to resolve the output mode from `Cli` and environment (e.g., checking `Cli.accessible` and any relevant env vars).

4. **Status-aware wrappers around pipeline stages**
   - Add small, test-visible helpers that wrap the internal functions and perform the status reporting that `run` does, so tests can call them without going through the full CLI plumbing:
     ```rust
     pub(crate) fn generate_ninja_with_status(
         cli: &Cli,
         reporter: &mut dyn StatusReporter,
     ) -> Result<(), RunnerError> {
         // call report_pipeline_stage for each non-execute stage,
         // then delegate to the existing generate_ninja implementation
     }

     pub(crate) fn handle_build_with_status(
         cli: &Cli,
         reporter: &mut dyn StatusReporter,
     ) -> Result<(), RunnerError> {
         // report Execute stage, call handle_build, then report_complete
     }

     pub(crate) fn handle_clean_with_status( ... ) -> Result<..., ...> { /* same pattern */ }
     pub(crate) fn handle_graph_with_status( ... ) -> Result<..., ...> { /* same pattern */ }
     ```
   - These helpers should match the internal wiring already used in `run`, so the tests directly verify that `generate_ninja`, `handle_build`, `handle_clean`, and `handle_graph` are wrapped with `report_pipeline_stage`/`report_complete` in the correct order.

5. **run integration (optional additional test)**
   - If you want tests to go through `run` directly, consider adding a `cfg(test)`-only hook that allows injecting a `TestReporter` into `run` (e.g., a test-only `fn run_with_reporter(cli: &Cli, reporter: &mut dyn StatusReporter)` that mirrors the standard `run` but replaces the reporter construction).
   - You can then extend the tests with an additional case that invokes `run_with_reporter` and asserts that the same sequences of stages and completions are recorded as in the helper-based tests.

Adjust the test code types (`RunnerError`, function names, and enum variants) to match your existing `runner` implementation so these tests compile and accurately verify the new status reporting integration.
</issue_to_address>

### Comment 3
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:103` </location>
<code_context>
+- [x] Write ExecPlan to `docs/execplans/3-8-1-add-accessible-output-mode.md`.
+- [x] Stage A: Define `OutputMode` enum and detection logic in new module
+      `src/output_mode.rs`.
+- [x] Stage A: Add unit tests for `OutputMode` detection (rstest parameterized).
+- [x] Stage B: Add `accessible` field to `Cli` struct with OrthoConfig wiring.
+- [x] Stage B: Update `cli_overrides_from_matches` to include `accessible`.
</code_context>

<issue_to_address>
**suggestion (typo):** Align "parameterized" spelling with the stated en-GB-oxendict convention (e.g. "parametrised").

This checklist item (and the later reference to "rstest parameterized cases") use "parameterized", which conflicts with the en-GB-oxendict convention you mention in the Constraints section. Please change these to "parametrised" for consistency.

```suggestion
- [x] Stage A: Add unit tests for `OutputMode` detection (rstest parametrised).
```
</issue_to_address>

### Comment 4
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:127-131` </location>
<code_context>
+- BDD step "the environment variable X is set to Y" already exists in
</code_context>

<issue_to_address>
**suggestion (testing):** Clarify whether the BDD scenarios use real or "simulated" environment variables, as the current text is slightly inconsistent.

The “Surprises & discoveries” section says the existing `the environment variable X is set to Y` step mutates the real process environment, while accessible-output scenarios use simulated TERM/NO_COLOR values via `TestWorld`. But the later example feature (and `accessible_output.feature`) still show steps like `Given the environment variable "TERM" is set ...`. Please align the description and examples—either note explicitly that these steps use the simulated environment, or adjust the example wording—so it’s clear whether tests touch the real environment or not.

```suggestion
- BDD step `"the environment variable X is set to Y"` already exists in
  `manifest/mod.rs` and mutates the real process environment.
- To avoid test interference, the accessible-output BDD scenarios do *not*
  use that step. Instead, their steps such as
  `Given the environment variable "TERM" is set to "xterm-256color"` are
  bound to step definitions that store TERM/NO_COLOR values in `TestWorld`
  and pass them into `resolve_with` via a closure, so the real process
  environment is never changed.
```
</issue_to_address>

### Comment 5
<location> `src/status.rs:71` </location>
<code_context>
+/// hard-coded literals at call sites and ensures [`PIPELINE_STAGE_COUNT`]
+/// stays consistent with the stages that are reported.
+#[derive(Copy, Clone, Debug)]
+pub enum PipelineStage {
+    /// Stage 1: configuring the network policy.
+    NetworkPolicy = 1,
</code_context>

<issue_to_address>
**issue (complexity):** Consider simplifying the pipeline stage API by deriving the stage count from the last enum variant and splitting execute-specific descriptions into a dedicated function to avoid optional tool keys at most call sites.

You can drop a couple of abstractions without losing any functionality:

### 1. Simplify stage counting (drop `ALL` + clippy suppression)

`PipelineStage::ALL` is only used to get a constant length, and you already have a stable, 1-based enum. You can tie the count directly to the last variant and remove the indirection and `#[expect]`.

```rust
#[derive(Copy, Clone, Debug)]
pub enum PipelineStage {
    NetworkPolicy = 1,
    ManifestLoad = 2,
    BuildGraph = 3,
    GenerateNinja = 4,
    Execute = 5,
}

impl PipelineStage {
    #[must_use]
    pub const fn index(self) -> u32 {
        self as u32
    }
}

// Total stages, tied to the last enum variant.
pub const PIPELINE_STAGE_COUNT: u32 = PipelineStage::Execute.index();
```

This keeps the “single source of truth” property without needing `ALL` or the clippy exemption.

### 2. Narrow the `description` API to avoid `Option` at call sites

Right now callers always have to think about `tool_key`, even though only `Execute` uses it. You can split the API so most call sites don’t see `Option` at all, and only the execute stage path deals with tool keys:

```rust
impl PipelineStage {
    #[must_use]
    pub fn description(self) -> String {
        match self {
            Self::NetworkPolicy => {
                localization::message(keys::STATUS_STAGE_NETWORK_POLICY).to_string()
            }
            Self::ManifestLoad => {
                localization::message(keys::STATUS_STAGE_MANIFEST_LOAD).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::Execute => {
                // Generic execute message without tool name.
                localization::message(keys::STATUS_STAGE_EXECUTE).to_string()
            }
        }
    }

    #[must_use]
    pub fn execute_description(tool_key: &'static str) -> String {
        let tool = localization::message(tool_key).to_string();
        localization::message(keys::STATUS_STAGE_EXECUTE)
            .with_arg("tool", tool)
            .to_string()
    }
}
```

And then keep `report_pipeline_stage` simple and explicit for the execute case:

```rust
pub fn report_pipeline_stage(
    reporter: &dyn StatusReporter,
    stage: PipelineStage,
) {
    reporter.report_stage(stage.index(), PIPELINE_STAGE_COUNT, &stage.description());
}

pub fn report_execute_stage(
    reporter: &dyn StatusReporter,
    tool_key: &'static str,
) {
    reporter.report_stage(
        PipelineStage::Execute.index(),
        PIPELINE_STAGE_COUNT,
        &PipelineStage::Execute.execute_description(tool_key),
    );
}
```

Most call sites now just call `report_pipeline_stage(reporter, stage)` with no `Option`, and the single “execute with tool” path uses the dedicated helper, making the intent clearer with less cognitive overhead.
</issue_to_address>

### Comment 6
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:3` </location>
<code_context>
+# Add accessible output mode (roadmap 3.8.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.
</code_context>

<issue_to_address>
**issue (review_instructions):** "ExecPlan" is used as an acronym without being defined on first use.

To align with the style guidance on acronyms, please expand and define this term on first use, for example: "This execution plan (ExecPlan) is a living document ..." and then use "ExecPlan" thereafter.

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>

### Comment 7
<location> `docs/execplans/3-8-1-add-accessible-output-mode.md:212` </location>
<code_context>
+  `define_keys!` macro.
+- `locales/en-US/messages.ftl` — English Fluent translations.
+- `locales/es-ES/messages.ftl` — Spanish Fluent translations.
+- `tests/bdd_tests.rs` — BDD entry point;
+  `scenarios!("tests/features", fixtures = [world: TestWorld])`.
+- `tests/bdd/fixtures/mod.rs` — `TestWorld` struct for BDD state.
</code_context>

<issue_to_address>
**issue (review_instructions):** "BDD" is introduced without expanding the acronym on first use.

Please expand BDD the first time it appears in the document, for example: "Behaviour-Driven Development (BDD) entry point". Subsequent references can use just "BDD".

<details>
<summary>Review instructions:</summary>

**Path patterns:** `**/*.md`

**Instructions:**
Define uncommon acronyms on first use.

</details>
</issue_to_address>