Stop the mock environment mutating the real process environment (#490)

[leynos]

Summary

test_support::env::EnvMut was implemented for both DefaultEnv and mockable::MockEnv, and both impls called std::env::set_var. Setting a variable through the mock mutated the real process environment, while the double's own expect_raw expectations carried on returning their programmed values. Any test believing it operated on an isolated environment was writing global state, and from that point the double and the process disagreed about the value.

This deletes the trait rather than wrapping it, per the AGENTS.md testing mandate landed in 3f84545.

The bug, concretely

tests/ninja_env_tests.rs programmed a mock to report NINJA_ENV as absent:

env.expect_raw()
    .withf(|k| k == NINJA_ENV)
    .returning(|_| Err(std::env::VarError::NotPresent));
let _guard = override_ninja_env(&env, ninja_tmp.as_path());
let after = std::env::var_os(NINJA_ENV)  // ← reads the *real* environment

The guard captured its "original" value from the mock but wrote to and restored the process. Each assertion happened to consult whichever half of the pair agreed with it, so the tests passed regardless of whether the behaviour was correct. On a host with NETSUKE_NINJA genuinely set, the second test would have silently deleted it.

Changes

• Delete the EnvMut trait and both impls. Mutation now goes through a private set_process_var, reachable only by the real-environment helpers in the module, so no mock can mutate anything.
• Replace mocked_path_env() with mock_env_with_path(value). The old constructor seeded a double from the ambient PATH, making its behaviour depend on the environment of whoever ran the suite.
• prepend_dir_to_path and override_ninja_env now take &SystemEnv rather than a generic parameter. This is honest about what they do: they mutate the live environment because runner::run_ninja resolves its program from it and has no injected seam yet (Inject an Env seam into Ninja program resolution #488).
• install_test_ninja drops its env parameter, since every call site passed system_env().
• Rewrite the two affected test binaries to establish known state with a VarGuard rather than a mock, and add a test pinning that mock_env_with_path reports the supplied value rather than the ambient one.

Also: the injected command environment

The first revision deferred reworking PathGuard. That is now done, because the deferral rested on a mistaken assumption — that the fake Ninja had to be discoverable through the parent process.

prepend_dir_to_path mutated the parent PATH under EnvLock and restored it on drop. Serialising the writers never isolated the readers: test_support/src/netsuke.rs forwards ambient PATH into child processes without taking the lock, so one scenario could inherit another's temporary executable.

Ninja now receives its environment as data:

• CommandEnv carries overrides applied with Command::env. NinjaBuildRequest and NinjaToolRequest carry one, and configure_ninja_base applies it.
• run_ninja and run_ninja_tool keep their signatures and pass CommandEnv::inherit(), so production behaviour is unchanged. run_ninja_with / run_ninja_tool_with take a request for callers supplying an environment.
• path_with_dir_prepended(existing, dir) replaces prepend_dir_to_path. It takes the starting value explicitly, so its result depends only on its inputs, and distinguishes absent from empty: absent yields just the directory, empty keeps the empty entry the caller attached meaning to.
• PathGuard, tests/path_guard_tests.rs, and with_isolated_path are deleted. With nothing mutating PATH, there is nothing to restore.

The BDD world carries a composed CommandEnv in place of the guard. tests/env_path_tests.rs is rewritten around composition — present, empty and absent PATH, later overrides replacing earlier ones, a composed value reaching the command environment verbatim, and that composing leaves the parent PATH unchanged. None of them is #[serial].

Two judgement calls

NETSUKE_NINJA still goes through the process in run_respects_env_override_for_ninja. That test exercises resolve_ninja_program reading the live environment, which is the production behaviour under test; only the competing PATH entry moved to the child.

process::mod passed the 400-line limit, so command construction moved to process::configure. Handled rather than suppressed.

Verification

All six commit gates pass: check-fmt, lint, typecheck, test (1241 nextest), markdownlint, nixie. CodeScene delta: no issues.

Closes #490.
Refs #496, #488, #494.

🤖 Generated with Claude Code

Summary by Sourcery

Prevent mocked environments from mutating the real process environment and align PATH/NINJA_ENV helpers and tests with explicit, process-scoped mutation.

Bug Fixes:

• Stop MockEnv-based tests from unintentionally writing to and restoring the real process environment while still reading expectations from the mock.

Enhancements:

• Replace the generic EnvMut trait with a private process-scoped setter and restrict PATH/NINJA_ENV helpers to SystemEnv to make their behaviour explicit.
• Introduce mock_env_with_path to build deterministic PATH-mocking doubles independent of the ambient environment.

Tests:

• Rework ninja environment and PATH tests to use VarGuard and the real environment under serial execution, and add coverage ensuring mock_env_with_path returns its supplied PATH value.

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[sourcery-ai]

Reviewer's Guide

This PR removes a leaky environment-mutation abstraction so that only real-environment helpers can mutate process globals, makes PATH/NINJA helpers explicitly depend on SystemEnv, and rewrites tests to use VarGuard and a deterministic mock environment constructor instead of mocks that silently mutate the process state.

File-Level Changes

Change	Details	Files
Restrict environment mutation to a private helper and make PATH/NINJA mutators depend on SystemEnv instead of a generic EnvMut trait.	Delete the EnvMut trait and its implementations for DefaultEnv and MockEnv. Introduce unsafe set_process_var as a private helper that writes to std::env and is only used by scoped guards. Change prepend_dir_to_path and override_ninja_env to take &SystemEnv and call set_process_var under EnvLock instead of env.set_var.	test_support/src/env.rs
Make the test ninja installation steps explicitly use the real process environment and stop passing mock environments around.	Remove the EnvMut import from process step tests. Simplify install_test_ninja to construct SystemEnv internally and apply prepend_dir_to_path and override_ninja_env to it. Update all BDD steps that installed fake ninja to stop using mocked_path_env and call install_test_ninja without an env parameter.	tests/bdd/steps/process.rs
Rewrite environment-related tests to set up known process state with VarGuard and add coverage for the new mock_env_with_path helper.	Change ninja_env_tests to use VarGuard::set/unset plus system_env instead of MockEnv, and assert against the real process env before/after override_ninja_env. Remove manual restoration of NINJA_ENV since VarGuard now owns cleanup. Adjust env_path_tests to treat prepend_dir_to_path as mutating the real PATH, using VarGuard and serial_test, and to assert restored PATH equals the sentinel value. Replace mocked_path_env with mock_env_with_path in tests and add a test that verifies mock_env_with_path returns the injected PATH instead of the ambient one.	tests/ninja_env_tests.rs tests/env_path_tests.rs
Replace mocked_path_env with a deterministic mock_env_with_path constructor that no longer depends on the ambient PATH.	Remove mocked_path_env, which previously seeded a MockEnv from the current process PATH. Add mock_env_with_path(path) that programs MockEnv::raw("PATH") to return the supplied string value. Document that mock_env_with_path does not read the ambient PATH and is safe to use concurrently.	test_support/src/env.rs tests/env_path_tests.rs tests/bdd/steps/process.rs

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#490	Delete the EnvMut trait and its implementations so that no test_support type can mutate the real process environment via a mock.	✅
#490	Replace mocked_path_env() with a constructor that takes an explicit PATH value and update the related tests (e.g., env_path_tests.rs, path_guard-related tests) to use it so mocks no longer depend on ambient PATH.	✅
#490	Rework PathGuard and prepend_dir_to_path to compose an Env value instead of mutating the process environment, returning the modified environment for injection, and require explicit PATH value for PathGuard construction.	❌	prepend_dir_to_path continues to mutate the real process environment using SystemEnv and PathGuard; the PR explicitly states that reworking PathGuard to a compositional, injected Env is deferred until runner::run_ninja gains an injected seam and PathGuard is later removed.

---

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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Summary

• Remove EnvMut and PathGuard to prevent parent-environment mutation.
• Add CommandEnv for explicit child-process environment composition.
• Add run_ninja_with and run_ninja_tool_with request APIs.
• Preserve inherited environments in existing Ninja wrappers.
• Handle present, empty, and absent PATH values without changing the parent environment.
• Update BDD fixtures and tests, including property-based, child-process, and compile-time API coverage.
• Document the design in ADR-002, the developer guide, and the Ninja environment testing guide.
• Address issue #490.

Verification

• Pass formatting, lint, behavioural, property, concurrency, performance, and maintainability checks.

Walkthrough

Changes

Explicit child command environments

Layer / File(s)	Summary
Define environment-aware Ninja requests src/runner/process/command_env.rs, src/runner/process/request.rs, src/runner/process/configure.rs, src/runner/process/mod.rs	Add CommandEnv, public Ninja request types, and shared command configuration with additive environment application.
Expose environment-aware Ninja execution src/runner/mod.rs, src/runner/process/mod.rs	Add request-based Ninja runners. Keep existing wrappers on the inherited environment.
Migrate BDD environment handling and documentation tests/bdd/fixtures/mod.rs, tests/bdd/steps/process.rs, docs/adr-002-replace-cucumber-with-rstest-bdd.md, docs/developers-guide.md, docs/test-isolation-with-ninja-env.md, docs/users-guide.md, test_support/src/env.rs	Carry composed child environments through BDD state. Document PATH composition, executable selection, request APIs, and empty PATH handling.
Validate composition and the public API tests/env_path_tests.rs, tests/env_path_property_tests.rs, tests/env_path_tests.proptest-regressions, tests/command_env_ui_tests.rs, tests/ui/command_env_embedder_pass.rs	Test PATH ordering, override precedence, child-process propagation, parent preservation, tool overrides, and external compilation against the public API.

Sequence Diagram(s)

sequenceDiagram
  participant BDDSteps
  participant CommandEnv
  participant run_ninja_with
  participant NinjaProcess
  BDDSteps->>CommandEnv: compose child PATH
  BDDSteps->>run_ninja_with: submit NinjaBuildRequest
  run_ninja_with->>NinjaProcess: spawn with explicit environment
  NinjaProcess-->>BDDSteps: return build result

Loading

Possibly related issues

• leynos/netsuke#496: Replace process-wide environment mutation and locking with injected child environments.
• leynos/netsuke#493: Replace process-wide PATH mutation in environment-path tests.
• leynos/netsuke#111: Replace unsafe global PATH mutation with explicit child-process environment injection.

Possibly related PRs

• leynos/netsuke#515: Addresses the same environment-isolation and Ninja PATH propagation code.
• leynos/netsuke#530: Relates to Ninja executable selection and controlled child-process environments.

Suggested labels: Issue

Suggested reviewers: codescene-access

Poem

Compose the PATH without changing the host,
Pass each Ninja setting as a request.
Keep inherited values in their place,
Test every byte and lookup trace.
Child commands now run with clear state.

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 warning, 7 inconclusive)

Check name	Status	Explanation	Resolution
Observability	⚠️ Warning	The new public CommandEnv changes Ninja child state, but the existing spawn/exit logs and span omit whether environment overrides were applied, hindering diagnosis of environment-caused failures.	Add bounded, non-sensitive fields such as env_override_count and path_overridden to the Ninja span and spawn/exit events; do not log environment names or values.
Testing (Overall)	❓ Inconclusive	Investigation is still in progress.	Inspect the added tests and implementation before deciding.
User-Facing Documentation	❓ Inconclusive	Investigation is still in progress.	Inspect the guide, API semantics, and migration documentation before deciding.
Developer Documentation	❓ Inconclusive	Evidence gathering is still in progress; no assessment submitted yet.	Inspect the developer guide and design records against the new command-environment APIs and architecture.
Testing (Unit And Behavioural)	❓ Inconclusive	Investigation in progress; no verdict yet.	Gather source and test evidence before deciding.
Unit Architecture	❓ Inconclusive	Assessment pending source and test inspection.	Inspect the changed environment and process boundaries, then verify query purity, injected dependencies, and side-effect tests.
Security And Privacy	❓ Inconclusive	Investigation is in progress; no verdict has been recorded yet.	Gather implementation and diff evidence before deciding.
Architectural Complexity And Maintainability	❓ Inconclusive	Evidence collection is still in progress; the architecture requires inspection of the new environment and request boundaries.	Inspect the new modules, call sites, dependency graph, and existing equivalent utilities before deciding.

✅ Passed checks (12 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix and includes the linked issue number (#490).
Description check	✅ Passed	The description clearly explains the environment-mutation defect, the implementation changes, and the verification results.
Linked Issues check	✅ Passed	The changes address #490 by removing EnvMut, eliminating process PATH mutation, requiring explicit PATH values, updating tests, and reporting required checks as passing.
Out of Scope Changes check	✅ Passed	The CommandEnv APIs, documentation, BDD updates, and tests directly support explicit child-environment handling and the linked issue objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Module-Level Documentation	✅ Passed	Verify that every changed Rust module has leading //! documentation; the docs state each module's purpose, use, and relevant relationship to Ninja, runner, tests, or fixtures.
Testing (Property / Proof)	✅ Passed	Proptest covers arbitrary PATH entries, absent/empty values, and repeated override sequences; fixed tests verify child propagation, inheritance, and parent-PATH preservation.
Testing (Compile-Time / Ui)	✅ Passed	The added command_env_ui_tests integration target directly invokes rustc to type-check an external fixture covering CommandEnv, both request types, and both public runner APIs; this is a valid tryb...
Domain Architecture	✅ Passed	PASS: Environment and process concerns stay in runner/process; PATH is composed as data, and core IR/domain modules do not depend on CommandEnv or runner adapters.
Performance And Resource Use	✅ Passed	Keep the check passing: production uses empty CommandEnv::inherit(); overrides are unique-key bounded, PATH composition is linear, and no new polling or unbounded runtime loop exists.
Concurrency And State	✅ Passed	CommandEnv owns immutable per-invocation overrides; no new global state or environment mutation was added. EnvLock tests cover re-entry, contention and poisoning, while child propagation tests veri...
Rust Compiler Lint Integrity	✅ Passed	Keep the lint surface intact: the PR adds no broad unused/dead-code allowances; its function-scoped expectations target only disallowed environment reads, and new APIs have real callers. The two te...

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-490-remove-envmut-and-rework-pathguard

---

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e4aa7661a

ℹ️ 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".

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[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.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope).

#490 Rework PathGuard and prepend_dir_to_path to compose an Env value instead of mutating the process environment, returning the modified environment for injection, and require explicit PATH value for PathGuard construction. ❌ prepend_dir_to_path continues to mutate the real process environment using SystemEnv and PathGuard; the PR explicitly states that reworking PathGuard to a compositional, injected Env is deferred until runner::run_ninja gains an injected seam and PathGuard is later removed.

[leynos]

On the failed pre-merge rows:

• Rust Compiler Lint Integrity (error): fixed in c5a32d5. The _run/_run_tool signature anchors and the tuple discard are gone; the fixture now calls run_ninja_with(&build)? and run_ninja_tool_with(&tool)? for real and main returns io::Result<()>. The earlier anchor design rested on a mistaken premise — the harness stops at --emit=metadata, so the fixture is never executed and genuine calls spawn nothing.
• Testing (Unit And Behavioural) (warning): the observation is accurate but the proposed remedy contradicts the intended contract. prepend_path_value deliberately collapses an empty prior PATH exactly like an absent one: preserving the empty entry would reintroduce the implicit current-directory lookup an empty PATH element denotes, which is never wanted in the isolated environments these tests compose. The parameterized case (case::empty / case::missing) verifies precisely that contract. What was missing was the contract being stated, so c5a32d5 documents the rule on prepend_path_value itself.
• Testing (Property / Proof) (inconclusive): validated — prepend_dir_to_path_preserves_every_generated_entry covers ordering over arbitrary entry lists, and the properties module covers entry lists including empty entries plus arbitrary override sequences with repeated keys, checked against an independent last-write-wins model (get and is_empty both pinned).
• Concurrency And State (inconclusive): validated — nothing in the diff mutates the parent process environment; CommandEnv is data applied via Command::env at spawn, the test file's module doc records that no test needs #[serial] or EnvLock, and the inheritance test now derives its baseline from a directly spawned child rather than reading the parent's environment.
• Performance And Resource Use (inconclusive): validated — the overrides are a small Vec of key/value pairs traversed once per spawn; no allocation-heavy or repeated work is introduced on any hot path (Ninja spawns dominate by orders of magnitude).
• Architectural Complexity And Maintainability (inconclusive): validated — the seam is split across command_env.rs/request.rs/configure.rs, all modules stay under the 400-line ceiling, and the CodeScene delta on this branch is clean after the earlier parameterization and Parts-struct refactors.

[codescene-access]

No quality gates enabled for this code.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 2636-2643: The executable-resolution rule is inconsistent across
the documentation and test description. In docs/developers-guide.md lines
2636-2643, replace the unconditional “Callers therefore pass resolved paths”
statement with a conditional requirement that callers use an absolute or
otherwise resolved program path when executable-selection isolation from
injected PATH is required; in tests/env_path_tests.rs lines 168-173, update the
Unix behavior description to state that relative programs are resolved through
the child PATH rather than the parent PATH.

In `@src/runner/process/command_env.rs`:
- Around line 64-72: Update the Rustdoc example for
CommandEnv::inherit().with_path to construct the PATH value with
std::env::join_paths using platform-native path components instead of a
colon-delimited literal. Preserve the assertion against the resulting
platform-appropriate value and the existing parent-environment behavior.

In `@tests/command_env_ui_tests.rs`:
- Around line 142-155: Add a tracked-work reference to the item-level
#[expect(clippy::disallowed_methods)] reasons on both cargo() and rustc(),
preserving their narrow explanations; alternatively, replace the ambient
environment lookups with an approved injected source and remove the
suppressions.

In `@tests/env_path_tests.rs`:
- Around line 323-408: Extract the properties module containing entry,
composition_prepends_and_preserves_order, and
overrides_resolve_to_their_last_declaration into a sibling integration-test
file, preserving its imports, helpers, and tests. Remove the original module
from tests/env_path_tests.rs so the file remains within the 400-line limit, and
ensure the extracted module still references the same public APIs and test
support utilities.

🪄 Autofix

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 Plus

Run ID: 17f53f1c-73a9-4508-bec4-db61994c59b0

📥 Commits

Reviewing files that changed from the base of the PR and between 525a0b6 and 53d9107.

📒 Files selected for processing (16)

• docs/adr-002-replace-cucumber-with-rstest-bdd.md
• docs/developers-guide.md
• docs/test-isolation-with-ninja-env.md
• docs/users-guide.md
• src/runner/mod.rs
• src/runner/process/command_env.rs
• src/runner/process/configure.rs
• src/runner/process/mod.rs
• src/runner/process/request.rs
• test_support/src/env.rs
• tests/bdd/fixtures/mod.rs
• tests/bdd/steps/process.rs
• tests/command_env_ui_tests.rs
• tests/env_path_tests.proptest-regressions
• tests/env_path_tests.rs
• tests/ui/command_env_embedder_pass.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/rstest-bdd (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/whitaker (auto-detected)
• leynos/shared-actions (auto-detected)

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/developers-guide.md`:
- Around line 2659-2666: Update the documentation for
test_support::env::prepend_path_value to state that it errors when a PATH entry
contains an unrepresentable character on the target platform, specifically
reflecting Windows where quotes are unrepresentable and semicolons are supported
through quoting. Match the terminology used in tests/env_path_tests.rs and leave
the remaining composition behavior unchanged.

In `@src/runner/process/mod.rs`:
- Around line 147-149: Update the documentation example for
CommandEnv::with_path to state that it replaces the child PATH rather than
prepending a directory. Clarify that callers must compose the full PATH value
before invoking with_path when prepend behavior is needed.

🪄 Autofix

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 Plus

Run ID: ef51c5e3-a4f1-462c-9ae4-430eaef4f9c4

📥 Commits

Reviewing files that changed from the base of the PR and between 15f0ba5 and 222bf2d.

📒 Files selected for processing (17)

• docs/adr-002-replace-cucumber-with-rstest-bdd.md
• docs/developers-guide.md
• docs/test-isolation-with-ninja-env.md
• docs/users-guide.md
• src/runner/mod.rs
• src/runner/process/command_env.rs
• src/runner/process/configure.rs
• src/runner/process/mod.rs
• src/runner/process/request.rs
• test_support/src/env.rs
• tests/bdd/fixtures/mod.rs
• tests/bdd/steps/process.rs
• tests/command_env_ui_tests.rs
• tests/env_path_property_tests.rs
• tests/env_path_tests.proptest-regressions
• tests/env_path_tests.rs
• tests/ui/command_env_embedder_pass.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/rstest-bdd (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/whitaker (auto-detected)
• leynos/shared-actions (auto-detected)

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the Windows join_paths error condition.

Replace “entry contains the platform path separator”. On Windows, ; is
representable through quoting. " is the unrepresentable character. Describe
the error as an unrepresentable PATH entry, as tests/env_path_tests.rs does.

Triage: [type:docstyle]

Proposed documentation update

- It returns an error when an entry contains the platform path separator, which
- `std::env::join_paths` itself reports.
+ It returns an error when an entry is not representable in a PATH value. For
+ example, Unix rejects `:`, while Windows rejects `"`;
+ `std::env::join_paths` reports both cases.

As per coding guidelines, keep documentation strategy synchronized with the implementation.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	`PATH` values are composed with `test_support::env::prepend_path_value`, a
	pure function that places a directory ahead of an explicitly supplied prior
	value. It takes the starting value rather than reading the process, so the
	result depends only on its inputs. An absent prior value yields just the new
	directory, and — by the helper's contract, which its tests pin — a wholly
	empty prior value is treated the same way; empty entries inside a non-empty
	value survive composition. It returns an error when an entry contains the
	platform path separator, which `std::env::join_paths` itself reports.
	`PATH` values are composed with `test_support::env::prepend_path_value`, a
	pure function that places a directory ahead of an explicitly supplied prior
	value. It takes the starting value rather than reading the process, so the
	result depends only on its inputs. An absent prior value yields just the new
	directory, and — by the helper's contract, which its tests pin — a wholly
	empty prior value is treated the same way; empty entries inside a non-empty
	value survive composition. It returns an error when an entry is not
	representable in a PATH value. For example, Unix rejects `:`, while Windows
	rejects `"`;
	`std::env::join_paths` reports both cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/developers-guide.md` around lines 2659 - 2666, Update the documentation
for test_support::env::prepend_path_value to state that it errors when a PATH
entry contains an unrepresentable character on the target platform, specifically
reflecting Windows where quotes are unrepresentable and semicolons are supported
through quoting. Match the terminology used in tests/env_path_tests.rs and leave
the remaining composition behavior unchanged.

Source: Coding guidelines

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct the with_path description.

State that CommandEnv::with_path replaces the child PATH. It calls
with_var("PATH", path) and does not prepend a directory. Compose the full
PATH value before calling with_path when prepend behaviour is required.

Proposed correction

-/// // `inherit()` reproduces `run_ninja`; `with_path` puts a directory ahead of
-/// // the child's `PATH` without touching the parent process.
+/// // `inherit()` reproduces `run_ninja`; `with_path` replaces the child's
+/// // `PATH` without touching the parent process.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	/// // `inherit()` reproduces `run_ninja`; `with_path` puts a directory ahead of
	/// // the child's `PATH` without touching the parent process.
	/// let env = CommandEnv::inherit().with_path("/opt/toolchain/bin");
	/// // `inherit()` reproduces `run_ninja`; `with_path` replaces the child's
	/// // `PATH` without touching the parent process.
	/// let env = CommandEnv::inherit().with_path("/opt/toolchain/bin");

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/runner/process/mod.rs` around lines 147 - 149, Update the documentation
example for CommandEnv::with_path to state that it replaces the child PATH
rather than prepending a directory. Clarify that callers must compose the full
PATH value before invoking with_path when prepend behavior is needed.