Advanced Usage: 3.13.2 docs, tests, and roadmap completed

[leynos]

Summary

• This PR implements the Advanced Usage work stream for 3.13.2. It adds a cohesive Advanced Usage Chapter in the user guide, Behavioural Driven Development (BDD) scenarios, integration test scaffolding, and test-harness improvements. The ExecPlan Stage E–F remain descoped per the updated plan; changes are additive and include concrete assets for documentation and test coverage, not just planning.

Changes

• Adds docs/execplans/3-13-2-user-documentation-advanced-usage-chapter.md detailing the complete plan for an Advanced Usage chapter, BDD scenarios, integration tests, design decisions, and roadmap updates.
• Updates docs/users-guide.md to introduce Section 12 "Advanced Usage" with subsections 12.1–12.5, detailing usage of clean, graph, manifest, configuration layering, and JSON diagnostics, including worked examples and cross-references.
• Adds Behavioural Driven Development (BDD) component:
  • tests/features/advanced_usage.feature with eight scenarios covering manifest streaming, manifest to stdout/file, clean, graph, configuration layering with environment/CLI overrides, and JSON diagnostics on error.
  • tests/bdd/steps/advanced_usage.rs containing new step definitions for the advanced usage workflows (config file setup, environment variables, and graph output checks).
  • Updates to tests/bdd/steps/mod.rs to register the new advanced_usage module.
• Adds integration test scaffold:
  • tests/advanced_usage_tests.rs (rstest-based) to cover edge cases and unhappy paths not reached by BDD.
• Test harness adjustments:
  • tests/bdd/steps/manifest_command.rs updated to locate the netsuke executable and to support environment variable handling and robust command execution, including explicit PATH handling.
  • tests/bdd/steps/manifest_command.rs now preserves NINJA_ENV and applies environment variables from the test world to netsuke invocations.
• Tests and fixtures enhancements:
  • tests/bdd/fixtures/mod.rs extended with an env_vars_forward map and related wiring to forward scenario-controlled environment variables to child processes.
  • test_support updates: test_support/src/env.rs and test_support/src/env_guard.rs gain original_ref() helpers to peek at captured original values without consuming the guard.
  • tests/bdd/helpers/env_mutation.rs updated to propagate forward-env values and preserve isolation guarantees.
• Design/docs updates:
  • docs/netsuke-design.md updated to reflect the design decision that advanced usage documentation is backed by behavioural tests.
  • docs/roadmap.md updated to reflect the 3.13.2 milestone as completed.
  • docs/developers-guide.md adjusted to clarify BDD command helpers and environment handling changes (in-line with the updated test harness).
• Documentation coherence:
  • Minor alignment and wording adjustments to maintain en-GB-oxendict spelling and project style.
• Additional test coverage and tooling:
  • tests/features/advanced_usage.feature added.
  • tests/advanced_usage_tests.rs added with rstest-based integration tests for edge/unhappy paths.
  • tests/bdd/steps/manifest_command_tests.rs added to provide unit coverage for environment handling helpers.

Note: This work is additive and focused on improving the reliability and coverage of advanced usage scenarios through documentation, BDD, integration tests, and test harness improvements.

Plan of work (Stage A–F) – status update

• Stage A: Audit and structure – completed.
• Stage B: Write Advanced Usage chapter – completed and integrated into docs/users-guide.md as Section 12.
• Stage C: Add BDD behavioural scenarios – completed with tests/features/advanced_usage.feature and tests/bdd/steps/advanced_usage.rs.
• Stage D: Add focused rstest integration tests – completed in tests/advanced_usage_tests.rs.
• Stage E: Update design docs and roadmap – updated; Stage E described as descoped per the updated ExecPlan.
• Stage F: Validation and evidence – additive changes; gates to be run locally to confirm formatting, linting, tests, and docs checks pass in the target environment.

Acceptance criteria

• The Advanced Usage chapter exists in the user guide with a coherent flow and cross-references.
• BDD feature file tests/features/advanced_usage.feature validates documented workflows.
• Focused rstest integration tests cover edge cases and unhappy paths.
• Design documentation updated and roadmap item 3.13.2 marked done.
• All repository gates pass: formatting, linting, tests, and documentation checks.

Artefacts and notes

Key artefacts introduced by this plan:

• docs/execplans/3-13-2-user-documentation-advanced-usage-chapter.md (new planning doc)
• References to planned updates to docs/users-guide.md, docs/netsuke-design.md, and docs/roadmap.md (plan documented in ExecPlan)
• Guidance for future steps (submission order, test wiring, gating requirements)
• Expanded test harness and environment isolation tooling to support deterministic, test-scoped env propagation

Validation and acceptance

To verify locally:

• Review the new ExecPlan for completeness and traceability.
• If proceeding, implement Stage A–F in separate commits and run the gates:
  • make check-fmt, make lint, make test, make markdownlint, make nixie.
  • Run BDD and integration tests as described in Stage B–D.

Idempotence and rollback

All stages are additive and self-contained. If a stage fails, revert only the affected changes using git.

---

Generated by the updated plan and work items for Advanced Usage documentation and test coverage.

📎 Task: https://www.devboxer.com/task/05a40bb2-4c56-466d-b742-6e351fbaf2ce

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e9e237ba-b360-4960-b5d1-7243d96aae6a

📥 Commits

Reviewing files that changed from the base of the PR and between c90f68d and 23b0991.

📒 Files selected for processing (1)

• docs/developers-guide.md

---

Overview

Implements roadmap item 3.13.2 by adding an "Advanced usage" chapter to the user guide, delivering BDD and rstest integration tests for the chapter, and hardening the BDD test harness and developer documentation around environment‑handling semantics. Changes are additive and include an ExecPlan, docs, tests, and test‑harness improvements.

ExecPlan and Design

• New living execplan: docs/execplans/3-13-2-user-documentation-advanced-usage-chapter.md — defines requirements, constraints (en‑GB spelling, no new public APIs, no new crate deps), staged progress (Stages A–D complete; E–F descoped), risks/mitigations, acceptance evidence and local validation commands. The ExecPlan is referenced from delivered docs and developer notes.
• Design/roadmap/docs updated: docs/netsuke-design.md, docs/roadmap.md, docs/developers-guide.md to reflect the BDD‑backed documentation and test approach.

User Documentation

• Added Section 12 "Advanced usage" to docs/users-guide.md covering:
  • Subcommands: clean, graph, manifest — behaviour and stdout/stderr contracts (e.g. graph fails before DOT output on manifest errors).
  • Configuration layering/precedence: defaults → merged config files by scope → NETSUKE_ environment variables → CLI flags; NETSUKE_CONFIG_PATH bypass; mapping rules (kebab-case → SCREAMING_SNAKE_CASE; __ for nested/indexed values).
  • JSON diagnostics mode (--diag-json, NETSUKE_OUTPUT_FORMAT=json): machine‑clean stdout, structured JSON diagnostics on stderr for failures, and suppression of verbose/tracing output when JSON mode is active.
• Minor editorial fixes (en‑GB style, removed 2nd‑person phrasing).

Behavioural Tests (BDD)

• New feature: tests/features/advanced_usage.feature — Gherkin scenarios for manifest streaming/output, clean/graph failure modes when manifest is missing, JSON diagnostics behaviour, invalid‑config reporting, and configuration/verbosity precedence.
• New step: tests/bdd/steps/advanced_usage.rs — given_config_file_with_setting(world, key, value) writes a .netsuke.toml in the scenario workspace (parses "true"/"false" into TOML booleans).
• Step module registered in tests/bdd/steps/mod.rs.

Test Harness, Fixtures & Unit Tests

• Centralised command‑building helpers in tests/bdd/steps/manifest_command.rs:
  • netsuke_executable() — resolves the test binary via assert_cmd::cargo::cargo_bin!.
  • build_netsuke_command(world, args) — constructs assert_cmd::Command with env_clear(); explicitly forwards host PATH and scenario‑forwarded variables from world.env_vars_forward; sets current_dir to the scenario temp workspace.
  • run_netsuke_and_store(...) and run_manifest_command(&TestWorld, ...) — run the built command and capture stdout/stderr (UTF‑8 lossy) and success status.
• Unit tests (tests/bdd/steps/manifest_command_tests.rs) assert: scenario‑tracked vars are forwarded, host vars are not inherited, PATH is explicitly forwarded, and the resolved netsuke executable is used.
• TestWorld extended with env_vars_forward: RefCell<HashMap<String, OsString>> and track_env_var signature changed to accept new_value so forwardable env values are recorded.
• BDD steps/helpers that mutate environment updated to supply new_value to track_env_var (fs, process, progress_output, stdlib/workspace, and env_mutation helper).
• Added non‑consuming original_ref() accessors to NinjaEnvGuard / EnvGuard to inspect captured originals without consuming guards.

Integration Tests

• New rstest-based integration suite: tests/advanced_usage_tests.rs covering edge and unhappy paths around clean, graph, manifest, configuration precedence, and --diag-json.
• Helpers standardise workspace setup, fake‑ninja installation and explicit env forwarding; tests assert file creation, error messages, JSON diagnostics on stderr, and manifest streaming semantics.
• Review identified and addressed missing test assertions/cases; outstanding coverage/refactor items tracked.

Test-support API & Dev Dependencies

• Added test_support::run_netsuke_in_with_env(current_dir, args, extra_env) to run netsuke under env_clear() while restoring PATH, conditionally forwarding NETSUKE_NINJA when parent variable exists, and applying explicit extra_env entries.
• Added dev‑dependency toml = "0.8" to Cargo.toml for test TOML handling.

Environment Handling: Races, Locking & Fixes

• Problems found during review:
  • build_netsuke_command previously read scenario‑tracked keys from the live process env and attempted to acquire EnvLock while NinjaEnvGuard held it, causing races/deadlocks.
  • Clearing PATH for child processes prevented subprocess discovery (e.g. ninja); some tests mutated process‑wide PATH without restoring it, poisoning concurrent tests.
• Fixes implemented:
  • build_netsuke_command now reads forwarded values from TestWorld.env_vars_forward (populated when steps mutate env) instead of re‑reading the live process env.
  • Host PATH is explicitly forwarded into child commands while still using env_clear() to ensure subprocess discovery.
  • Removed extra EnvLock acquisition around PATH reads to avoid re‑entrant locking deadlocks.
  • Tests that modify PATH now use VarGuard/EnvGuard to save/restore it and avoid cross‑test interference.
• Larger refactor to remove remaining live process‑env reads and improve TestWorld ergonomics tracked in issue #284.

Developer Documentation

• docs/developers-guide.md updated with BDD command helper docs (netsuke_executable, build_netsuke_command, run_netsuke_and_store), the post‑env_clear() contract (host PATH + world.env_vars_forward presented to spawned processes), a Mermaid e2e BDD flow diagram, and notes contrasting BDD helpers with integration helpers.
• Documentation edits and inline style fixes applied; remaining sync items tracked against the refactor (issue #284).

Acceptance Evidence & Follow‑ups

• Acceptance evidence: Advanced Usage chapter, ExecPlan, BDD feature/steps, rstest integration tests, developer docs updates, unit tests asserting env forwarding/PATH behaviour, and documented local validation (formatting, linting, tests).
• Follow‑ups:
  • Issue #284 tracks the env_vars_forward refactor and removal of remaining live process‑env reads from command construction and TestWorld ergonomics.
  • Outstanding suggested follow‑ups: add Stage D tests noted in review (manifest‑to‑nonexistent‑directory, stderr emptiness checks) and document new test APIs in the developers guide alongside the refactor.

Walkthrough

Add an "Advanced usage" docs chapter and tests for clean, graph and manifest; refactor BDD environment handling to record forwarded variables; add command‑building helpers that spawn isolated processes forwarding only PATH plus scenario‑forwarded entries; introduce non‑consuming env‑guard accessors.

Changes

Cohort / File(s)	Summary
Documentation docs/execplans/3-13-2-user-documentation-advanced-usage-chapter.md, docs/users-guide.md, docs/developers-guide.md, docs/execplans/3-11-1-cli-config-struct-derived-with-ortho-config.md	Add Advanced Usage plan and user docs for clean/graph/manifest; define config‑layering precedence and JSON diagnostics; update BDD env guidance to prefer test_support::env wrappers and document new helpers.
BDD Feature + Steps tests/features/advanced_usage.feature, tests/bdd/steps/advanced_usage.rs, tests/bdd/steps/mod.rs	Add Gherkin scenarios for advanced usage; add step to write .netsuke.toml; register new advanced_usage step module.
Environment Tracking tests/bdd/fixtures/mod.rs, tests/bdd/helpers/env_mutation.rs, tests/bdd/steps/fs.rs, tests/bdd/steps/process.rs, tests/bdd/steps/progress_output.rs, tests/bdd/steps/stdlib/workspace.rs	Introduce TestWorld.env_vars_forward; change track_env_var to accept new_value and maintain forward map; update callers to pass owned OsString new values for forwarding/unsetting.
Command Building & Manifest Runner tests/bdd/steps/manifest_command.rs, tests/bdd/steps/manifest_command_tests.rs	Refactor manifest runner to accept &TestWorld; add build_netsuke_command and netsuke_executable(); use env_clear() and forward only host PATH plus keys from env_vars_forward; add tests validating env isolation and command composition.
Integration Tests & Helpers tests/advanced_usage_tests.rs, test_support/src/netsuke.rs, Cargo.toml	Add integration tests exercising subcommands, config precedence and --diag-json; add run_netsuke_in_with_env helper that sets isolated env and explicit extra env forwarding; add toml = "0.8" as a dev‑dependency.
Env Guard Accessors test_support/src/env.rs, test_support/src/env_guard.rs	Add non‑consuming original_ref() accessor to NinjaEnvGuard and generic EnvGuard<E> to inspect captured original values without consuming guards.
BDD Safety Helpers tests/bdd/helpers/env_mutation.rs, tests/bdd/steps/*	Update mutate_env_var and step callers to convert new values into owned OsString and pass them into track_env_var so RAII restoration and forwarding semantics are preserved.
CI Workflow Pins .github/workflows/*.yml	Update pinned commit SHAs for reusable actions and bump WiX extension default; preserve existing workflow logic and inputs.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant TW as TestWorld
  participant Builder as build_netsuke_command
  participant Host as HostEnv
  participant Cmd as assert_cmd::Command
  participant Netsuke as netsuke process

  TW->>Builder: request command with args + env_vars_forward
  Builder->>Host: read host PATH
  Host-->>Builder: return PATH
  Builder->>Cmd: env_clear()
  Builder->>Cmd: set PATH (from Host)
  Builder->>Cmd: inject env_vars_forward entries
  Builder->>Cmd: set program to netsuke_executable()
  Cmd->>Netsuke: spawn process
  Netsuke-->>Cmd: return stdout, stderr, exit status
  Cmd-->>Builder: return execution result
  Builder-->>TW: return stdout/stderr/status

Loading

Poem

🌱 Docs sprout, tests take their place,
Env guards hold truth with gentle grace,
PATH and chosen keys alone pass through,
Commands spawn tidy, outputs come true,
Scoped restores finish—next scenario, pursue.

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Testing	⚠️ Warning	New public test APIs in test_support lack dedicated unit tests; text-based output validation uses substring assertions instead of snapshot tests despite insta infrastructure availability.	Add unit tests for original_ref() and run_netsuke_in_with_env() behaviour; adopt insta snapshot testing for Ninja manifest rules and JSON diagnostics format validation.

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the principal change: delivering the Advanced Usage work stream for milestone 3.13.2 including docs, tests, and roadmap completion.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, detailing all documentation, BDD, integration test, and test-harness changes introduced in this PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
User-Facing Documentation	✅ Passed	The pull request comprehensively documents all new user-facing functionality in Section 12 of users-guide.md, including three utility subcommands, configuration layering precedence model, and JSON diagnostics mode with clear examples.
Developer Documentation	✅ Passed	The developer's guide adequately documents all new and changed internal APIs, architectural boundaries, and abstractions, including TestWorld field additions, BDD command helpers, non-consuming accessor methods, test helpers, and configuration steps with comprehensive architectural rationale.
Module-Level Documentation	✅ Passed	All three newly added Rust modules carry appropriate module-level documentation using the //! syntax, aligning with the codebase's missing_docs = "deny" enforcement.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch docs-extend-advanced-usage-fqstu8

---

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

[sourcery-ai]

Reviewer's Guide

Implements the first part of ExecPlan 3.13.2 by adding an Advanced Usage chapter to the user guide, introducing new BDD feature and step definitions plus a small feature file for advanced workflows, tightening the BDD manifest command harness to use assert_cmd with explicit PATH and env propagation, and documenting the plan and design linkages in a new execplan and minor doc edits.

Sequence diagram for BDD advanced usage manifest streaming workflow

sequenceDiagram
    actor Developer
    participant BddRunner
    participant TestWorld
    participant AdvancedUsageSteps
    participant ManifestCommandSteps
    participant AssertCmdCommand
    participant NetsukeBinary
    participant NinjaTool

    Developer->>BddRunner: run bdd_tests advanced_usage
    BddRunner->>TestWorld: create TestWorld fixture

    BddRunner->>AdvancedUsageSteps: execute Given a minimal Netsuke workspace
    AdvancedUsageSteps->>ManifestCommandSteps: reuse workspace_setup_steps
    ManifestCommandSteps->>TestWorld: create_workspace_with_manifest()

    BddRunner->>AdvancedUsageSteps: execute When netsuke is run with args "manifest -"
    AdvancedUsageSteps->>TestWorld: set_env_from_world()
    TestWorld->>AssertCmdCommand: build_command_with_explicit_path()
    AssertCmdCommand->>AssertCmdCommand: inherit_NINJA_ENV()
    AssertCmdCommand->>AssertCmdCommand: apply_world_environment_overrides()
    AssertCmdCommand->>NetsukeBinary: spawn_with_env_and_path()
    NetsukeBinary->>NinjaTool: optional_ninja_invocation()
    NinjaTool-->>NetsukeBinary: build_status
    NetsukeBinary-->>AssertCmdCommand: exit_code_stdout_stderr
    AssertCmdCommand-->>TestWorld: store_process_output()

    BddRunner->>AdvancedUsageSteps: execute Then stdout should contain Ninja_manifest
    AdvancedUsageSteps->>TestWorld: assert_stdout_contains_manifest_markers()

    BddRunner->>AdvancedUsageSteps: execute And stderr should be empty
    AdvancedUsageSteps->>TestWorld: assert_stderr_empty()

    BddRunner-->>Developer: scenario_passes

Loading

Class diagram for updated BDD steps, env handling, and test world

classDiagram
    class TestWorld {
      +Path workspace_dir
      +HashMap~String,String~ env_overrides
      +Output stdout
      +Output stderr
      +i32 exit_code
      +set_env_var(name: String, value: String) void
      +apply_env_to_command(command: AssertCmdCommand) void
      +store_output(output: CommandOutput) void
    }

    class ManifestCommandSteps {
      +given_minimal_workspace(world: TestWorld) void
      +given_empty_workspace(world: TestWorld) void
      +when_netsuke_run_with_args(world: TestWorld, args: Vec~String~) void
      +when_netsuke_run_without_args(world: TestWorld) void
      +then_command_should_succeed(world: TestWorld) void
      +then_command_should_fail(world: TestWorld) void
      +stdout_should_contain(world: TestWorld, fragment: String) void
      +stderr_should_contain(world: TestWorld, fragment: String) void
      +stdout_should_be_empty(world: TestWorld) void
      +stderr_should_be_empty(world: TestWorld) void
      -build_command(world: TestWorld, args: Vec~String~) AssertCmdCommand
      -attach_env_and_path(world: TestWorld, command: AssertCmdCommand) AssertCmdCommand
    }

    class AdvancedUsageSteps {
      +given_workspace_with_config_setting(world: TestWorld, key: String, value: String) void
      +given_env_var_set(world: TestWorld, name: String, value: String) void
      +stdout_should_contain_dot_graph(world: TestWorld) void
      +stderr_should_be_valid_diagnostics_json(world: TestWorld) void
      +stderr_should_not_contain(world: TestWorld, fragment: String) void
    }

    class EnvSupport {
      +set_var(name: String, value: String) VarGuard
      +remove_var(name: String) VarGuard
      +restore_many(vars: HashMap~String,Option~String~~) void
      +acquire_env_lock() EnvLock
    }

    class EnvLock {
      +lock() void
      +unlock() void
    }

    class VarGuard {
      -String name
      -Option~String~ original
      +new(name: String, original: Option~String~) VarGuard
      +drop() void
    }

    class AssertCmdCommand {
      +env(name: String, value: String) AssertCmdCommand
      +env_remove(name: String) AssertCmdCommand
      +env_clear() AssertCmdCommand
      +current_dir(path: Path) AssertCmdCommand
      +assert() CommandOutput
    }

    class JsonDiagnosticsSteps {
      +stderr_should_be_valid_diagnostics_json(world: TestWorld) void
      +stderr_diagnostics_code_should_be(world: TestWorld, code: String) void
    }

    ManifestCommandSteps --> TestWorld : uses
    AdvancedUsageSteps --> TestWorld : uses
    AdvancedUsageSteps --> ManifestCommandSteps : reuses
    ManifestCommandSteps --> AssertCmdCommand : builds
    TestWorld --> AssertCmdCommand : applies_env
    EnvSupport --> EnvLock : manages
    EnvSupport --> VarGuard : returns
    ManifestCommandSteps --> EnvSupport : uses_for_env_management
    AdvancedUsageSteps --> EnvSupport : uses_for_env_management
    JsonDiagnosticsSteps --> TestWorld : reads_stderr

Loading

Flow diagram for validation and testing pipeline introduced by ExecPlan 3_13_2

flowchart TD
    A["Edit docs and tests for Advanced Usage"] --> B["Run make fmt"]
    B --> C["Run make markdownlint"]
    C --> D["Run make nixie"]
    D --> E["Run cargo test --test bdd_tests advanced_usage"]
    E --> F["Run cargo test --test advanced_usage_tests"]
    F --> G["Run make check-fmt"]
    G --> H["Run make lint"]
    H --> I["Run make test"]
    I --> J["All gates pass, roadmap 3_13_2 can be marked done"]

Loading

File-Level Changes

Change	Details	Files
Add Advanced Usage chapter to the user guide describing clean/graph/manifest, configuration layering, and JSON diagnostics with worked examples.	Append new Section 12 'Advanced Usage' to the user guide with subsections 12.1–12.5. Document behaviour, requirements, and example workflows for the clean, graph, and manifest subcommands. Describe configuration layering precedence (defaults, config files, environment variables, CLI flags) including discovery rules and TOML/environment formats. Explain JSON diagnostics mode, its interaction with stdout/stderr and verbose logging, and give CI integration examples.	docs/users-guide.md
Refine BDD manifest-command harness to invoke the real netsuke binary via assert_cmd and support controlled environment propagation.	Replace usage of run_netsuke_in with direct assert_cmd::Command invocation that calls a helper to locate the netsuke binary. Force an empty PATH for manifest-command runs to ensure the correct binary is exercised. Preserve NINJA_ENV when present so fake-ninja scenarios continue to work. Read environment overrides from TestWorld.env_vars and apply them (or remove variables) to each command invocation. Convert raw Command output into RunResult by decoding stdout/stderr and checking the status.	tests/bdd/steps/manifest_command.rs
Introduce specialised BDD steps and feature scenarios to cover advanced usage workflows, and register the new steps with the BDD harness.	Create advanced_usage.rs step module with steps for writing a .netsuke.toml config file, setting environment variables into TestWorld, and asserting that stderr omits specific fragments. Use shared Fluent-normalisation helper when checking stderr content to keep locale output stable. Register the new advanced_usage module in the BDD step registry. Add advanced_usage.feature with three scenarios covering manifest streaming to stdout, JSON diagnostics on error, and JSON diagnostics for manifest, using existing generic steps where possible.	tests/bdd/steps/advanced_usage.rs tests/features/advanced_usage.feature tests/bdd/steps/mod.rs
Add a living ExecPlan document for roadmap item 3.13.2, describing constraints, risks, and staged work for the Advanced Usage chapter and tests.	Create a new execplan documenting motivation, scope, constraints, tolerances, risks, and progress for implementing the Advanced Usage chapter and associated tests. Detail repository context (docs, design, implementation, BDD and integration tests) and key patterns for contributors. Lay out Stages A–F with concrete deliverables, validation steps, and acceptance criteria for documentation, BDD, integration tests, design updates, and roadmap updates. Describe idempotence, rollback, and artefacts table for the planned work.	docs/execplans/3-13-2-user-documentation-advanced-usage-chapter.md
Tidy existing documentation to maintain wrapping, style, and risk-mitigation guidance coherence.	Reflow a long risk-mitigation bullet in the 3-11-1 CLI config execplan to avoid awkward line breaks while preserving content. Rewrap a paragraph in the netsuke-design document so that sentences are grouped more naturally without changing meaning.	docs/execplans/3-11-1-cli-config-struct-derived-with-ortho-config.md docs/netsuke-design.md

---

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.

[leynos]

@coderabbitai Have the following now been resolved?

Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity.

Please address the comments from this code review:

## Overall Comments
- In `run_manifest_command` and `run_netsuke_and_store` you now set `PATH` to an empty string; this may break scenarios that rely on resolving `ninja` (or a fake ninja) via `PATH`, so consider either preserving the existing `PATH` or scoping this behavior behind a test helper flag so ninja discovery still works as intended.
- The new command-building logic in `manifest_command.rs` duplicates parts of the existing `run_netsuke_in` helper; factoring the common process setup (binary lookup, env propagation, current_dir, etc.) into a single helper would reduce drift and make future environment-handling changes easier.
- The `given_config_file_with_setting` step treats all non-boolean values as strings, which will make it awkward to use for numeric or more structured config keys later; you might want to extend it to recognize simple numeric literals (and possibly arrays) so the step remains generally useful as config evolves.

## Individual Comments

### Comment 1
<location path="tests/bdd/steps/manifest_command.rs" line_range="137-146" />
<code_context>
     let temp_path = get_temp_path(world)?;
-    let run = run_netsuke_in(&temp_path, args)?;
+
+    // Build command with environment variables from TestWorld
+    let mut cmd = assert_cmd::Command::new(netsuke_executable()?);
+    cmd.current_dir(&temp_path).env("PATH", "").args(args);
+
+    // Preserve NINJA_ENV if it's set (used by fake ninja scenarios)
+    if let Ok(ninja_env) = std::env::var("NINJA_ENV") {
+        cmd.env("NINJA_ENV", ninja_env);
+    }
+
+    // Apply environment variables from TestWorld
+    let env_vars = world.env_vars.borrow();
+    for (key, value) in env_vars.iter() {
+        if let Some(val) = value {
+            cmd.env(key, val);
+        } else {
+            cmd.env_remove(key);
+        }
+    }
+
+    let output = cmd.output().context("run netsuke command")?;
+
     store_run_result(
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to exercise the new environment-handling behaviour in the manifest command harness

The harness now preserves `NINJA_ENV`, clears `PATH`, and applies `world.env_vars` when invoking netsuke, but there are no tests asserting this. Please add BDD or integration coverage for at least:
- An env var in `world.env_vars` that changes netsuke behaviour (proving the map is applied).
- An env var cleared via `env_remove`, confirming netsuke no longer sees it.
- A case where a different `netsuke` earlier in `PATH` is not used, proving `netsuke_executable()` is respected.
These will lock in the new environment-handling semantics and prevent regressions.

Suggested implementation:

```rust
fn build_netsuke_command(
    world: &TestWorld,
    args: &[&str],
) -> Result<assert_cmd::Command> {
    let temp_path = get_temp_path(world)?;

    // Build command with environment variables from TestWorld
    let mut cmd = assert_cmd::Command::new(netsuke_executable()?);
    cmd.current_dir(&temp_path).env("PATH", "").args(args);

    // Preserve NINJA_ENV if it's set (used by fake ninja scenarios)
    if let Ok(ninja_env) = std::env::var("NINJA_ENV") {
        cmd.env("NINJA_ENV", ninja_env);
    }

    // Apply environment variables from TestWorld
    let env_vars = world.env_vars.borrow();
    for (key, value) in env_vars.iter() {
        if let Some(val) = value {
            cmd.env(key, val);
        } else {
            cmd.env_remove(key);
        }
    }

    Ok(cmd)
}

/// Run netsuke with the given arguments and store the result.
fn run_netsuke_and_store(world: &TestWorld, args: &[&str]) -> Result<()> {
    let mut cmd = build_netsuke_command(world, args)?;

    let output = cmd.output().context("run netsuke command")?;

    store_run_result(
        world,
        RunResult {
            stdout: String::from_utf8_lossy(&output.stdout).into_owned(),
            stderr: String::from_utf8_lossy(&output.stderr).into_owned(),
            success: output.status.success(),
        },
    );
    Ok(())
}

#[cfg(test)]
mod tests {
    use super::*;
    use std::ffi::OsStr;

    fn env_value<'a>(
        cmd: &'a assert_cmd::Command,
        key: &str,
    ) -> Option<Option<&'a OsStr>> {
        cmd.get_envs()
            .find(|(k, _)| k == OsStr::new(key))
            .map(|(_, v)| v.as_deref())
    }

    #[test]
    fn world_env_vars_with_value_are_applied() {
        let mut world = TestWorld::default();
        {
            let mut env_vars = world.env_vars.borrow_mut();
            env_vars.insert("NETSUKE_TEST_FLAG".to_string(), Some("enabled".to_string()));
        }

        let cmd = build_netsuke_command(&world, &["--help"]).expect("build command");

        let val = env_value(&cmd, "NETSUKE_TEST_FLAG")
            .expect("NETSUKE_TEST_FLAG should be present");
        assert_eq!(val, Some(OsStr::new("enabled")));
    }

    #[test]
    fn world_env_vars_none_are_removed() {
        let mut world = TestWorld::default();
        // Ensure there is an existing value in the process env so we can observe removal
        std::env::set_var("NETSUKE_REMOVE_ME", "to-be-removed");
        {
            let mut env_vars = world.env_vars.borrow_mut();
            env_vars.insert("NETSUKE_REMOVE_ME".to_string(), None);
        }

        let cmd = build_netsuke_command(&world, &["--help"]).expect("build command");

        // Command should contain an explicit removal entry
        let val = env_value(&cmd, "NETSUKE_REMOVE_ME")
            .expect("NETSUKE_REMOVE_ME removal entry should be present");
        assert!(val.is_none(), "NETSUKE_REMOVE_ME should be removed in child env");
    }

    #[test]
    fn path_is_cleared_and_netsuke_executable_is_used() {
        // Simulate a different netsuke early in PATH that must not be used
        std::env::set_var("PATH", "/fake/bin");

        let world = TestWorld::default();
        let cmd = build_netsuke_command(&world, &["--version"]).expect("build command");

        // PATH should be explicitly cleared in the command environment
        let path_val = env_value(&cmd, "PATH")
            .expect("PATH should be explicitly set on the command");
        assert_eq!(path_val, Some(OsStr::new("")));

        // Command should be configured to run the resolved netsuke_executable(), not a PATH lookup
        let exe = netsuke_executable().expect("netsuke_executable");
        assert_eq!(cmd.get_program(), exe.as_os_str());
    }
}

```

These edits assume:

1. `TestWorld` implements `Default`. If it does not, replace `TestWorld::default()` with whatever constructor or helper you already use in your BDD harness to create a minimal `TestWorld` instance (for tests, it only needs a valid `env_vars` field and whatever `get_temp_path` expects).
2. `TestWorld.env_vars` is initialized in `Default`/constructor; if not, make sure the tests initialize it appropriately before use.
3. The helper `build_netsuke_command` is placed in a location that keeps the module’s existing structure; if there is code following `run_netsuke_and_store` in this file, ensure it remains after the new function and tests when you integrate this patch.

If your Rust version or `assert_cmd::Command` does not expose `get_envs()`/`get_program()`, you may need to:
- Update `assert_cmd` to a version that provides these accessors, or
- Wrap the underlying `std::process::Command` in your own struct that exposes these for testing.

These tests validate:
- `world.env_vars` with `Some` values are applied (changing netsuke behaviour via env).
- `world.env_vars` with `None` are removed from the child env.
- `PATH` is cleared and the resolved `netsuke_executable()` path is used, so a different `netsuke` earlier in `PATH` cannot be selected.
</issue_to_address>

### Comment 2
<location path="docs/users-guide.md" line_range="1045" />
<code_context>
+netsuke build --emit build.ninja
+```
+
+Use `manifest` when you want the Ninja file *without* running the build, and
+`--emit` when you want both the file and the build execution.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This sentence uses second-person pronouns, which the documentation style instructions forbid.

Consider rephrasing to avoid "you", for example: "Use `manifest` to obtain the Ninja file *without* running the build, and `--emit` to obtain both the file and the build execution."

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

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

**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").

</details>
</issue_to_address>

### Comment 3
<location path="docs/users-guide.md" line_range="1115" />
<code_context>
+colour_policy = "auto"
+```
+
+You can override `colour_policy` for a single invocation:
+
+```sh
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This line uses "You", violating the no first-/second-person pronoun rule.

A possible alternative: "`colour_policy` can be overridden for a single invocation:" or "To override `colour_policy` for a single invocation:".

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

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

**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").

</details>
</issue_to_address>

[leynos]

@coderabbitai Please suggest a fix for the following test failures and provide an AI coding agent prompt for the fix:

failures:

---- features_scenarios::novice_flows_first_run_succeeds_in_a_minimal_workspace stdout ----

thread 'features_scenarios::novice_flows_first_run_succeeds_in_a_minimal_workspace' (11106) panicked at tests/bdd_tests.rs:19:1:
Step failed at index ⁨3⁩: ⁨Then⁩ ⁨the command should succeed⁩ - ⁨Error executing step '⁨the command should succeed⁩' via function '⁨command_should_succeed⁩': ⁨command run status should be success⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features/novice_flows.feature⁩, scenario: ⁨First run succeeds in a minimal workspace⁩)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_scenarios::novice_flows_first_run_succeeds_in_a_minimal_workspace::novice_flows_first_run_succeeds_in_a_minimal_workspace
   4: bdd_tests::features_scenarios::novice_flows_first_run_succeeds_in_a_minimal_workspace
   5: bdd_tests::features_scenarios::novice_flows_first_run_succeeds_in_a_minimal_workspace::{{closure}}
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- features_scenarios::progress_output_accessible_mode_emits_textual_task_updates stdout ----

thread 'features_scenarios::progress_output_accessible_mode_emits_textual_task_updates' (11122) panicked at tests/bdd_tests.rs:19:1:
Step failed at index ⁨3⁩: ⁨Then⁩ ⁨the command should succeed⁩ - ⁨Error executing step '⁨the command should succeed⁩' via function '⁨command_should_succeed⁩': ⁨command run status should be success⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features/progress_output.feature⁩, scenario: ⁨Accessible mode emits textual task updates⁩)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_scenarios::progress_output_accessible_mode_emits_textual_task_updates::progress_output_accessible_mode_emits_textual_task_updates
   4: bdd_tests::features_scenarios::progress_output_accessible_mode_emits_textual_task_updates
   5: bdd_tests::features_scenarios::progress_output_accessible_mode_emits_textual_task_updates::{{closure}}
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- features_scenarios::progress_output_malformed_ninja_status_lines_are_ignored_safely stdout ----

thread 'features_scenarios::progress_output_malformed_ninja_status_lines_are_ignored_safely' (11149) panicked at tests/bdd_tests.rs:19:1:
Step failed at index ⁨3⁩: ⁨Then⁩ ⁨the command should succeed⁩ - ⁨Error executing step '⁨the command should succeed⁩' via function '⁨command_should_succeed⁩': ⁨command run status should be success⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features/progress_output.feature⁩, scenario: ⁨Malformed Ninja status lines are ignored safely⁩)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_scenarios::progress_output_malformed_ninja_status_lines_are_ignored_safely::progress_output_malformed_ninja_status_lines_are_ignored_safely
   4: bdd_tests::features_scenarios::progress_output_malformed_ninja_status_lines_are_ignored_safely
   5: bdd_tests::features_scenarios::progress_output_malformed_ninja_status_lines_are_ignored_safely::{{closure}}
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- features_scenarios::progress_output_progress_output_can_be_disabled_in_accessible_mode stdout ----

thread 'features_scenarios::progress_output_progress_output_can_be_disabled_in_accessible_mode' (11160) panicked at tests/bdd_tests.rs:19:1:
Step failed at index ⁨3⁩: ⁨Then⁩ ⁨the command should succeed⁩ - ⁨Error executing step '⁨the command should succeed⁩' via function '⁨command_should_succeed⁩': ⁨command run status should be success⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features/progress_output.feature⁩, scenario: ⁨Progress output can be disabled in accessible mode⁩)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_scenarios::progress_output_progress_output_can_be_disabled_in_accessible_mode::progress_output_progress_output_can_be_disabled_in_accessible_mode
   4: bdd_tests::features_scenarios::progress_output_progress_output_can_be_disabled_in_accessible_mode
   5: bdd_tests::features_scenarios::progress_output_progress_output_can_be_disabled_in_accessible_mode::{{closure}}
   6: core::ops::function::FnOnce::call_once
error: test failed, to rerun pass `--test bdd_tests`
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- features_scenarios::progress_output_progress_output_can_be_disabled_in_standard_mode stdout ----

stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_scenarios::stdlib_with_suffix_rewrites_extensions::stdlib_with_suffix_rewrites_extensions
   4: bdd_tests::features_scenarios::stdlib_with_suffix_rewrites_extensions
   5: bdd_tests::features_scenarios::stdlib_with_suffix_rewrites_extensions::{{closure}}
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- features_scenarios::stdlib_which_filter_resolves_the_first_path_entry stdout ----

thread 'features_scenarios::stdlib_which_filter_resolves_the_first_path_entry' (11266) panicked at tests/bdd_tests.rs:19:1:
Step failed at index ⁨4⁩: ⁨When⁩ ⁨I render the stdlib template "{{ 'tool' | which }}" without context⁩ - ⁨Error executing step '⁨I render the stdlib template {template:string} without context⁩' via function '⁨render_stdlib_template_without_path⁩': ⁨acquire localizer test lock⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features/stdlib.feature⁩, scenario: ⁨which filter resolves the first PATH entry⁩)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_scenarios::stdlib_which_filter_resolves_the_first_path_entry::stdlib_which_filter_resolves_the_first_path_entry
   4: bdd_tests::features_scenarios::stdlib_which_filter_resolves_the_first_path_entry
   5: bdd_tests::features_scenarios::stdlib_which_filter_resolves_the_first_path_entry::{{closure}}
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

---- features_unix_scenarios::stdlib_realpath_resolves_symlinks stdout ----

thread 'features_unix_scenarios::stdlib_realpath_resolves_symlinks' (11305) panicked at tests/bdd_tests.rs:23:1:
Step failed at index ⁨1⁩: ⁨When⁩ ⁨I render template "{{ path | realpath }}" at stdlib path "link"⁩ - ⁨Error executing step '⁨I render template {template:string} at stdlib path {path:string}⁩' via function '⁨render_stdlib_template⁩': ⁨acquire localizer test lock⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features_unix/stdlib.feature⁩, scenario: ⁨realpath resolves symlinks⁩)
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic_display
   3: bdd_tests::features_unix_scenarios::stdlib_realpath_resolves_symlinks::stdlib_realpath_resolves_symlinks
   4: bdd_tests::features_unix_scenarios::stdlib_realpath_resolves_symlinks
   5: bdd_tests::features_unix_scenarios::stdlib_realpath_resolves_symlinks::{{closure}}
   6: core::ops::function::FnOnce::call_once
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    features_scenarios::novice_flows_first_run_succeeds_in_a_minimal_workspace
    features_scenarios::progress_output_accessible_mode_emits_textual_task_updates
    features_scenarios::progress_output_malformed_ninja_status_lines_are_ignored_safely
    features_scenarios::progress_output_progress_output_can_be_disabled_in_accessible_mode
    features_scenarios::progress_output_progress_output_can_be_disabled_in_standard_mode
    features_scenarios::progress_output_standard_mode_reports_task_updates_from_ninja_status_lines
    features_scenarios::progress_output_status_messages_do_not_contaminate_stdout_in_standard_mode
    features_scenarios::progress_output_subprocess_stdout_is_separate_from_status_messages
    features_scenarios::stdlib_grep_filter_extracts_matching_lines
    features_scenarios::stdlib_grep_filter_streams_large_output_to_a_temporary_file
    features_scenarios::stdlib_group_by_clusters_items_by_attribute
    features_scenarios::stdlib_group_by_clusters_items_with_non_string_keys
    features_scenarios::stdlib_group_by_reports_errors_for_missing_attributes
    features_scenarios::stdlib_hash_computes_the_sha256_digest
    features_scenarios::stdlib_linecount_counts_newline_delimited_lines
    features_scenarios::stdlib_relative_to_returns_the_child_component
    features_scenarios::stdlib_rendering_basename_for_a_file_path
    features_scenarios::stdlib_shell_filter_enforces_command_output_limits
    features_scenarios::stdlib_shell_filter_enforces_command_stream_limits
    features_scenarios::stdlib_shell_filter_reports_command_failures
    features_scenarios::stdlib_shell_filter_streams_large_output_to_a_temporary_file
    features_scenarios::stdlib_shell_filter_transforms_text_and_marks_templates_impure
    features_scenarios::stdlib_size_filter_reports_errors_for_missing_files
    features_scenarios::stdlib_size_returns_the_byte_length
    features_scenarios::stdlib_time_now_rejects_invalid_offsets
    features_scenarios::stdlib_time_rendering_now_with_an_offset_preserves_the_offset
    features_scenarios::stdlib_time_rendering_now_yields_a_utc_timestamp
    features_scenarios::stdlib_time_timedelta_captures_nanosecond_precision
    features_scenarios::stdlib_time_timedelta_composes_multiple_components
    features_scenarios::stdlib_time_timedelta_overflow_surfaces_an_error
    features_scenarios::stdlib_time_timedelta_supports_negative_values
    features_scenarios::stdlib_uniq_removes_duplicates
    features_scenarios::stdlib_which_errors_are_localised_in_spanish
    features_scenarios::stdlib_which_filter_lists_every_path_match
    features_scenarios::stdlib_which_filter_reports_missing_executables
    features_scenarios::stdlib_which_filter_resolves_the_first_path_entry
    features_scenarios::stdlib_which_function_honours_cwd_mode
    features_scenarios::stdlib_with_suffix_rewrites_extensions
    features_unix_scenarios::stdlib_realpath_resolves_symlinks

test result: FAILED. 168 passed; 39 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.63s

make: *** [Makefile:20: test] Error 101

[leynos]

@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix:

Overall comment — PATH and ninja discovery

Still worth noting: build_netsuke_command now uses env_clear() and no longer specially preserves NINJA_ENV (that preservation exists only in the old run_manifest_command inline path). Ensure any BDD scenarios that rely on a fake ninja via NINJA_ENV invoke through build_netsuke_command (i.e. via run_netsuke_and_store) rather than through run_manifest_command directly, or add NINJA_ENV to world.env_vars tracking in those scenarios.

[leynos]

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Testing	⚠️ Warning	Tests for six Stage D edge cases (clean without prior build, graph with invalid manifest, configuration layering, JSON diagnostics with verbose suppression) are missing from tests/advanced_usage_tests.rs.	Create tests/advanced_usage_tests.rs with rstest integration tests covering all six specified edge cases from ExecPlan Stage D and verify with cargo test --test advanced_usage_tests.
Developer Documentation	⚠️ Warning	PR introduces three internal test infrastructure APIs (netsuke_executable(), build_netsuke_command(), run_netsuke_and_store()) without updating the developers guide as mandated for test strategy changes.	Add a subsection to docs/developers-guide.md under 'Behavioural testing strategy' documenting build_netsuke_command(world, args), netsuke_executable(), and the environment-handling contract with variable forwarding patterns in the same commit.

[leynos]

@coderabbitai I'm concerned that we are using imperative environment manipulation rather than using env_lock (https://docs.rs/env-lock/latest/env_lock/) and RAII for this.

Every netsuke invocation — whether through `run_netsuke_and_store` or `run_manifest_command` — now routes through the single `build_netsuke_command` helper, which:

1. Calls `env_clear()` for isolation.
2. Forwards the host `PATH` so netsuke can exec ninja.
3. Explicitly forwards `NINJA_ENV` if a scenario has set one via `NinjaEnvGuard`.
4. Re-applies any scenario-tracked vars from `world.env_vars`.

No feature files reference `NINJA_ENV` directly — scenarios set it via the `ninja_env_guard` field on `TestWorld`, which writes it into the process environment where `std::env::var(ninja_env::NINJA_ENV)` in step 3 above picks it up correctly.

The original concern has been fully addressed by the current implementation.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Line 456: Insert a comma after the word "steps" in the sentence mentioning the
TestWorld fixture (the phrase "The `TestWorld` fixture coordinates state across
steps while") so it reads "across steps, while" to improve readability and
conform to en-GB punctuation; update the sentence in the
docs/developers-guide.md where `TestWorld` is described.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 99c08222-0a21-4eab-af58-7bb325fa9dcd

📥 Commits

Reviewing files that changed from the base of the PR and between 804aec9 and 02874ce.

📒 Files selected for processing (2)

• docs/developers-guide.md
• tests/advanced_usage_tests.rs

[leynos]

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Developer Documentation	⚠️ Warning	Documentation lacks coverage of track_env_var() signature change, original_ref() accessors, given_config_file_with_setting() BDD step, and toml dev-dependency rationale.	Document the track_env_var() signature change, original_ref() accessors, given_config_file_with_setting() step definition, and explain the toml 0.8 dev-dependency purpose.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 362-477: Update the earlier docs to match the new BDD env
contract: add env_vars_forward to the TestWorld field groups table and adjust
its Purpose to mention "forwarding map for child commands"; update the TestWorld
method docs for track_env_var(key, previous) to the new signature
track_env_var(key, previous, new_value) and explain it records the restoration
value in env_vars and the forwarding value in env_vars_forward; and document the
new non-consuming accessors EnvGuard::original_ref() and
NinjaEnvGuard::original_ref() (in the EnvVarGuard or TestWorld methods section)
so readers know they can inspect captured original values without consuming the
guard.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e1a9fb60-546f-4b20-9ee8-34137c42c3f0

📥 Commits

Reviewing files that changed from the base of the PR and between 02874ce and b50a2a9.

📒 Files selected for processing (5)

• .github/workflows/build-and-package.yml
• .github/workflows/ci.yml
• .github/workflows/netsukefile-test.yml
• .github/workflows/release.yml
• docs/developers-guide.md

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 446-448: Replace the British '-ise' spelling in the phrase
containing "toml = \"0.8\"" and the word "serialisation" with the Oxford '-ize'
form: change "serialisation" to "serialization" in the sentence that mentions
the `toml = "0.8"` dev-dependency and the `toml` crate so the documentation
follows the repo's en-GB-oxendict convention.
- Around line 152-156: The sentence in the docs paragraph explaining how to
inspect the environment before the guard is activated contains an incorrect
comma before the essential "because" clause; edit the paragraph (the one
referencing track_env_var and consuming original(self)) to remove the comma
immediately before "because" so the restrictive clause reads correctly, matching
the same fix applied at the other location (commit 02874ce).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3f98781f-8979-4806-9370-f139c40767da

📥 Commits

Reviewing files that changed from the base of the PR and between b50a2a9 and a8da1cf.

📒 Files selected for processing (2)

• docs/developers-guide.md
• tests/advanced_usage_tests.rs

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/build-and-package.yml:
- Line 59: The workflow sets WiX Toolset default to '7' but WiX v7 changes path
semantics and removes Heat; update the build to ensure .wxs Files element paths
are correct (paths are now relative to the .wxs source directory) and
remove/replace any Heat.exe or Heat MSBuild usage—migrate harvesting steps to
WiX Files or Payloads elements instead; search for references to "default: '7'",
"Heat.exe", "Heat", "Files element", and any .wxs harvesting scripts and adjust
path references and harvesting logic accordingly.

In `@docs/developers-guide.md`:
- Around line 160-164: The example call to world.track_env_var passes
ninja_path.as_os_str().to_owned() directly but the method expects new_value:
Option<OsString>; update the call to wrap the third argument in Some(...), e.g.
pass Some(ninja_path.as_os_str().to_owned()), leaving the first two args
(ninja_env::NINJA_ENV.to_owned(), previous) unchanged so the call to
world.track_env_var matches the documented signature.
- Around line 155-169: The docs incorrectly reference a consuming guard method
named original(self); update the API text and examples to document and call
into_original(self) -> Option<OsString> instead. Locate the example using
override_ninja_env, let guard, and
guard.original_ref()/world.track_env_var/world.ninja_env_guard and replace
mentions of original(self) with into_original(self), and update any signature
text at the earlier API reference to show into_original(self) ->
Option<OsString> so readers know the consuming method name and return type.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 758de11a-80a6-4dfe-be5c-dccc8b196e34

📥 Commits

Reviewing files that changed from the base of the PR and between a8da1cf and c90f68d.

📒 Files selected for processing (3)

• .github/workflows/build-and-package.yml
• .github/workflows/release.yml
• docs/developers-guide.md

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== docs/developers-guide.md lines 148-165 ==="
sed -n '148,165p' docs/developers-guide.md

echo ""
echo "=== docs/developers-guide.md lines 440-455 ==="
sed -n '440,455p' docs/developers-guide.md

echo ""
echo "=== Search for 'serialisation' in developers-guide.md ==="
grep -n "serialis" docs/developers-guide.md

echo ""
echo "=== Search for comma before 'because' near track_env_var ==="
grep -n ", because" docs/developers-guide.md

echo ""
echo "=== Start of tests/bdd/steps/advanced_usage.rs ==="
head -10 tests/bdd/steps/advanced_usage.rs

echo ""
echo "=== Start of tests/advanced_usage_tests.rs ==="
head -10 tests/advanced_usage_tests.rs

echo ""
echo "=== Start of tests/bdd/steps/manifest_command_tests.rs ==="
head -10 tests/bdd/steps/manifest_command_tests.rs

Length of output: 2954

---

@leynos: Both warnings are now fully resolved.

---

Developer Documentation ✅ Resolved

Both prose corrections are confirmed in the live file:

• Comma before 'because' — removed; line 153 now reads: …when calling \track_env_var` because the consuming `into_original(self)`…`
• 'serialisation' → 'serialization' — corrected; grep -n "serialis" returns no matches.

---

Module-Level Documentation ✅ Resolved

All three files open with //! docstrings before any imports:

File	Docstring
tests/bdd/steps/advanced_usage.rs	//! Step definitions for advanced usage workflow scenarios.
tests/advanced_usage_tests.rs	Multi-line //! block covering purpose, coverage scope, and subcommands
tests/bdd/steps/manifest_command_tests.rs	Multi-line //! block describing environment-isolation contract

No further work is required.