Introduce NinjaEnvGuard and NINJA_ENV-based test isolation#243
Conversation
Replace test fixtures that put a fake ninja executable on PATH with fixtures that set NINJA_ENV environment variable instead. This avoids mutating PATH, enabling tests to run in parallel without interference. Add documentation explaining benefits of using NINJA_ENV and provide fixture patterns for controlled ninja execution during tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Reviewer's GuideRefactors test infrastructure to resolve the Ninja binary via NINJA_ENV with scoped guards instead of mutating PATH, enabling safe parallel execution, and documents the new test-isolation pattern. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdd new documentation describing isolation of tests from the Ninja binary via NINJA_ENV and refactor test fixtures and test-support code to use a NinjaEnvGuard that holds an EnvLock instead of mutating PATH; update tests to the new guard-based lifecycle and add tests exercising success/failure behaviours. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Now that
NINJA_ENVis guarded, consider removing the remaining#[serial]onrun_respects_env_override_for_ninjaifNinjaEnvGuardalready handles synchronization, to keep this test consistent with the others that now run in parallel. - In
run_respects_env_override_for_ninja, the explicitdrop(guard)at the end is redundant since the guard will be dropped when it goes out of scope; you can simplify by relying on RAII unless you need the environment restored earlier in the test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that `NINJA_ENV` is guarded, consider removing the remaining `#[serial]` on `run_respects_env_override_for_ninja` if `NinjaEnvGuard` already handles synchronization, to keep this test consistent with the others that now run in parallel.
- In `run_respects_env_override_for_ninja`, the explicit `drop(guard)` at the end is redundant since the guard will be dropped when it goes out of scope; you can simplify by relying on RAII unless you need the environment restored earlier in the test.
## Individual Comments
### Comment 1
<location> `tests/runner_tests.rs:228-232` </location>
<code_context>
#[test]
#[serial]
fn run_respects_env_override_for_ninja() -> Result<()> {
- let (temp_dir, ninja_path) = fake_ninja(0u8)?;
+ let (_temp_dir, ninja_path) = fake_ninja(0u8)?;
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test that proves `NINJA_ENV` takes precedence over a conflicting Ninja on `PATH`.
This currently verifies that `NINJA_ENV` is used, but not that it takes precedence over a conflicting `PATH` entry. To cover the documented resolution order, consider adding a variant that:
1. Puts a different (or invalid) fake Ninja on `PATH`.
2. Sets `NINJA_ENV` via `override_ninja_env` to the expected fake Ninja.
3. Asserts that `run` uses the `NINJA_ENV` Ninja (via exit code, output, or another observable effect).
That would more directly exercise the precedence behavior and protect against regressions in resolution order.
Suggested implementation:
```rust
#[test]
#[serial]
fn run_respects_env_override_for_ninja() -> Result<()> {
// Successful Ninja that we will reference via NINJA_ENV.
let (_temp_dir_env, ninja_env_path) = fake_ninja(0u8)?;
// Failing Ninja that we will place on PATH to ensure NINJA_ENV takes precedence.
let (_temp_dir_path, ninja_path_on_path) = fake_ninja(1u8)?;
let env = SystemEnv::new();
// Put the failing Ninja on PATH. If PATH took precedence over NINJA_ENV,
// `run` would fail due to this Ninja's non‑zero exit code.
let ninja_dir_on_path = ninja_path_on_path
.parent()
.expect("fake_ninja path should have a parent directory");
std::env::set_var("PATH", ninja_dir_on_path);
// Override NINJA_ENV to point at the successful fake Ninja.
let guard = override_ninja_env(&env, &ninja_env_path);
let (_temp, manifest_path) = create_test_manifest()?;
run(&cli)
.context("expected run to prefer NINJA_ENV over conflicting PATH entry")?;
drop(guard);
Ok(())
}
```
1. This change assumes that:
- `fake_ninja(1u8)` produces a Ninja that reliably fails (e.g., exits with status 1) when invoked.
- The `run(&cli)` invocation and `cli` construction remain valid and unchanged elsewhere in the file.
2. If `std::env::set_var("PATH", ninja_dir_on_path)` conflicts with any existing test helpers (e.g., if there is a dedicated environment abstraction or helper for overriding `PATH`), you may want to replace that line with the appropriate helper call that the rest of the tests use.
3. If your test harness or environment requires restoring `PATH` after the test, consider capturing the previous value before `set_var` and restoring it in a guard similar to `override_ninja_env`. If such a helper already exists (e.g., `override_path_env`), prefer using it for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/runner_tests.rs (1)
226-241: Re-evaluate need for #[serial] on run_respects_env_override_for_ninjaDrop
#[serial]here unless another unsynchronised environment mutation justifies it.override_ninja_envalready acquiresEnvLockand restoresNINJA_ENVon drop, which aligns with the new parallel-safe pattern; keeping#[serial]undermines the PR’s goal of parallel test execution.Run this script to locate any remaining serialised tests that might still rely on unsafe environment patterns:
#!/bin/bash # Find remaining serial_test usage across Rust tests rg -n '#\\[serial\\]' --type=rust
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/test-isolation-with-ninja-env.md(1 hunks)tests/runner_tests.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/test-isolation-with-ninja-env.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/test-isolation-with-ninja-env.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/test-isolation-with-ninja-env.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/test-isolation-with-ninja-env.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/test-isolation-with-ninja-env.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/runner_tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/runner_tests.rs
🧬 Code graph analysis (1)
tests/runner_tests.rs (4)
test_support/src/env.rs (1)
override_ninja_env(238-246)test_support/src/lib.rs (1)
fake_ninja(95-127)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)test_support/src/path_guard.rs (1)
new(27-31)
🔍 Remote MCP Deepwiki
Summary of Relevant Context for PR Review
Based on the repository documentation, here are the key facts and context for reviewing this pull request:
NinjaEnvGuard Implementation Details
NinjaEnvGuard is a Rust struct that leverages RAII to ensure NINJA_ENV is restored to its original state when dropped, preventing global environment pollution. The mechanism uses EnvLock to serialize environment mutations across threads, which is crucial because std::env::set_var is unsafe and can cause race conditions in tests.
How NINJA_ENV Works
The netsuke application resolves the path to the Ninja executable by first attempting to read NINJA_ENV from the environment; if set and containing valid UTF-8, that path is used; otherwise it falls back to the default "ninja" program.
Why PATH Mutation is Problematic
PATH mutation is problematic for parallel tests because it modifies global process state, leading to flaky tests, parallel execution conflicts, and potential state corruption if a test panics. The override_ninja_env function with NinjaEnvGuard solves this by providing an RAII guard that restores NINJA_ENV to its original value when it goes out of scope, ensuring changes are localized to a single test.
Testing Framework Context
The project uses rstest for fixture-based unit testing, cucumber-rs for behavior-driven development, and insta for snapshot testing. For rstest, fixtures are called fresh for each test to provide proper test isolation, with regular (per-test) fixtures preferred over #[once] fixtures to ensure proper cleanup.
Design Pattern Validation
The refactoring aligns with the project's established testing patterns: The project emphasizes Dependency Injection to handle external dependencies like environment variables, with the mockable crate providing traits like Env and Clock to prevent global state access in tests.,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
🔇 Additional comments (3)
tests/runner_tests.rs (3)
11-27: Use NinjaEnvGuard-based fixture for NINJA_ENV correctlyKeep this import and fixture; they correctly wrap
fake_ninja_check_build_filewithSystemEnvandoverride_ninja_env, and they keep both the temporary directory andNinjaEnvGuardalive for the test duration via RAII. No issues here.
29-40: Keep ninja_with_exit_code aligned with the NINJA_ENV guard patternRetain this fixture shape; it mirrors
ninja_in_env, exposes only(TempDir, NinjaEnvGuard), and avoids leaking the fake Ninja path or mutatingPATH. This fits the parallel-friendly test strategy.
97-115: Use ninja_in_env / ninja_with_exit_code fixtures to isolate Ninja correctlyUse of
ninja_in_env()andninja_with_exit_code(0)in these tests correctly scopes NINJA_ENV, keeps the fake Ninja directory alive, and removes any reliance onPATHor shell configuration. Binding the guard to_guardis sufficient to keep it in scope without triggering clippy.Also applies to: 119-151, 155-178
|
@coderabbitai Please suggest a fix for this issue that will expand the lifetime of the environment guard to cover the execution of the CLI and supply a prompt for an AI coding agent to enable it to apply the fix: Guard NINJA_ENV-based tests against parallel races The new |
Add a test that ensures the NINJA_ENV environment variable takes precedence over any ninja executable on PATH. This involves placing a failing fake ninja on PATH and a working fake ninja via NINJA_ENV, verifying that the latter is respected during builds. Also update documentation to clarify NINJA_ENV precedence behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/test-isolation-with-ninja-env.md (1)
37-43: Expand the CLI acronym on first use in the exampleReplace the inline comment so that it spells out “command-line interface (CLI)” on first use, in line with the acronym guidance.
Apply a change along these lines:
- // run the CLI here; the guard restores NINJA_ENV on drop + // run the command-line interface (CLI) here; the guard restores NINJA_ENV on drop
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/test-isolation-with-ninja-env.md(1 hunks)tests/runner_tests.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/runner_tests.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/runner_tests.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/test-isolation-with-ninja-env.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/test-isolation-with-ninja-env.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/test-isolation-with-ninja-env.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/test-isolation-with-ninja-env.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/test-isolation-with-ninja-env.md
🧬 Code graph analysis (1)
tests/runner_tests.rs (4)
test_support/src/env.rs (2)
override_ninja_env(238-246)prepend_dir_to_path(194-208)test_support/src/lib.rs (1)
fake_ninja(95-127)test_support/src/check_ninja.rs (1)
fake_ninja_check_build_file(13-59)test_support/src/path_guard.rs (1)
new(27-31)
🔍 Remote MCP Deepwiki
Summary of additional findings relevant to the PR review (concise, actionable):
-
Tests and runner resolve Ninja from NINJA_ENV before PATH; the codebase already prefers NINJA_ENV for resolution and uses RAII guards to restore env on Drop (so moving from PATH mutation → NINJA_ENV is appropriate).,
-
Current Env-guard pattern (EnvGuard / NinjaEnvGuard) restores NINJA_ENV on Drop but does not hold a lock across CLI invocation — so returning a guard from a fixture without the test holding it for the entire CLI lifetime allows other tests to overwrite NINJA_ENV while the CLI is running (race). The PR comments already note this exact race.
Recommendation to fix (minimal, test-focused):
-
Ensure the environment guard's lifetime spans the actual CLI/Ninja execution. Two practical options:
- Change test fixtures so they return the guard and the test invokes the CLI while the guard is still in scope (i.e., hold the guard variable until after run completes). Example: let (_tmpdir, ninja_env_guard) = ninja_with_exit_code(...); run_cli(...); drop(ninja_env_guard);
- Or enhance NinjaEnvGuard to acquire a process-wide EnvLock (existing EnvLock mechanism) for the duration of CLI execution: provide a helper API run_with_ninja_env(ninja_path, closure) that sets NINJA_ENV and holds EnvLock until the closure (which runs the CLI) returns. This guarantees mutual exclusion across threads/tests and prevents interleaving NINJA_ENV mutations.
-
Prefer option 2 for robustness (tests need not remember to hold/drop); option 1 is smaller and quick to land if tests are updated accordingly.
Caveats and verification steps:
- If adopting option 2, use EnvLock to serialize environment mutations only while the CLI subprocess is executing (avoid holding the lock for longer than necessary).
- Add a test that runs two fixtures in parallel asserting their fake-ninja behaviors do not interfere (to confirm the race is fixed).
Tools/sources used:
- Repository documentation and structure via Deepwiki:,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (1)
tests/runner_tests.rs (1)
225-241: Retain this precedence test once the NINJA_ENV guard holds the env lockThis test correctly asserts that
NINJA_ENVtakes precedence over a conflictingPATHentry by:
- Putting a failing fake Ninja on
PATHviaprepend_dir_to_path.- Pointing
NINJA_ENVat a successful fake Ninja viaoverride_ninja_env.- Expecting
run(&cli)to succeed.After updating
override_ninja_envso thatNinjaEnvGuardholds the environment lock for its lifetime, keep this test as-is; it will continue to validate the documented resolution order while also benefiting from the stronger isolation guarantees.Run
cargo test --tests -- --test-threads=4after adjustingoverride_ninja_envto ensure this test remains stable and still passes under parallel execution.
|
@coderabbitai have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 163 🏁 Script executed: Length of output: 803 🏁 Script executed: Length of output: 614 @leynos Looking at the current code state: Status of the two issues:
However, there's a critical issue: ❌ The underlying fix has NOT been applied yet. The This means: Removing Recommendation: Apply the proposed fix to |
The NinjaEnvGuard now retains the EnvLock for its entire lifetime, ensuring that parallel mutations of the NINJA_ENV environment variable are serialized. This prevents race conditions when tests override NINJA_ENV concurrently. The guard restores the prior value on drop and holds the lock to prevent interleaving writes, confining unsafety to the guard's scope. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Added integration tests verifying behavior when using the NINJA_ENV environment variable. Tests cover the successful execution of the CLI with a fake ninja binary and failure when the ninja binary exits with a non-zero status. Also improved safety comments and refined NinjaEnvGuard implementation in env.rs. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file #[serial]
fn run_executes_ninja_without_persisting_file() -> Result<()> {
let (_ninja_dir, ninja_path, _guard) = ninja_in_path()?;
let (_ninja_dir, _guard) = ninja_in_env()?;❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Refactored repeated setup code in runner_tests.rs into a shared helper function `setup_ninja_env_test`. This new function consolidates the creation of a fake ninja directory, temporary project directory, CLI construction, and environment guard setup for tests that rely on the `NINJA_ENV` environment variable. The change improves code reuse and readability in the test suite. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/test-isolation-with-ninja-env.md(1 hunks)test_support/src/env.rs(3 hunks)tests/runner_tests.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/test-isolation-with-ninja-env.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/test-isolation-with-ninja-env.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/test-isolation-with-ninja-env.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/test-isolation-with-ninja-env.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/test-isolation-with-ninja-env.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/runner_tests.rstest_support/src/env.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/runner_tests.rstest_support/src/env.rs
🧬 Code graph analysis (2)
tests/runner_tests.rs (1)
test_support/src/env.rs (2)
override_ninja_env(244-255)prepend_dir_to_path(198-212)
test_support/src/env.rs (3)
test_support/src/env_lock.rs (1)
acquire(17-22)test_support/src/env_var_guard.rs (1)
original(68-70)test_support/src/env_guard.rs (1)
with_env_and_lock(76-88)
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to this PR review (sources cited):
-
Netsuke resolves which ninja executable to run from an environment override before falling back to PATH; tests and runner rely on an environment-based override mechanism (NINJA_ENV) for ninja selection and for CLI subprocess resolution.
-
The repo has a dedicated test_support crate and fixtures pattern; tests use RAII-style guards, tempfile-based resources, and aim for parallel execution where possible (serial_test used only for resource-exclusive tests). Test fixtures and runner expect guards to span the lifetime of any subprocess spawned by the CLI.
-
The test infrastructure and docs emphasize avoiding global PATH mutations in tests and recommend overriding NINJA_ENV for isolated, parallel-friendly tests; manifest/runner code and tests rely on environment guards and temp-file RAII to keep resources alive during subprocess execution.
-
The codebase already enforces strict error/context propagation (anyhow/thiserror/miette) and deterministic test output; changing guard semantics (holding an EnvLock for guard lifetime) is required to make the NINJA_ENV-based isolation actually safe for parallel tests (otherwise "last setter wins" races remain). This aligns with the PR's stated root-cause and proposed fix.
Tools used:
- Deepwiki_read_wiki_contents (leynos/netsuke)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-14, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-13, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (1)
tests/runner_tests.rs (1)
14-54: KeepNINJA_ENVguards in scope to maintain parallel safetyMaintain the current pattern where
ninja_in_env,ninja_with_exit_code, andsetup_ninja_env_testall return aNinjaEnvGuardthat stays in scope for the full duration of each test. This bracketsrun(&cli)with the guard’s lifetime and matches theoverride_ninja_envlocking semantics, so tests remain parallel-safe without#[serial]or manualdropcalls.Also applies to: 111-186
…NV tests - Updated ninja_in_env and ninja_with_exit_code fixtures to return PathBuf for ninja_path alongside NinjaEnvGuard. - Modified setup_ninja_env_test and tests to use the ninja_path for existence checks and improved clarity. - Enhanced NinjaEnvGuard docs to clarify lock acquisition and restore order. - Added documentation on EnvLock preventing parallel test NINJA_ENV mutation races. This refactor clarifies test fixtures and ensures robust handling of environment variables in tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
1 participant
Summary
Changes
Docs
Tests
Rationale
Migration notes
Testing plan
Additional notes
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/e5fb4018-e860-42f6-ac83-94aa85fff64f