Conversation
Reviewer's GuideThis PR introduces a safe, scoped mechanism to override the NINJA_ENV process variable by wrapping mutations in EnvLock and restoring the original value on guard drop. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 46 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 (2)
Summary by CodeRabbit
WalkthroughAdd a scoped, RAII-style API to temporarily override the process-global NINJA_ENV variable and restore it on drop; add tests for set/restore behaviour; move NINJA_ENV into a new ninja_env crate and update test-support and runner tests to use the guard. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant API as override_ninja_env
participant Lock as EnvLock
participant OS as ProcessEnv
Test->>API: call override_ninja_env(env, path)
API->>Lock: acquire lock
API->>OS: set NINJA_ENV = path (unsafe, confined)
API-->>Test: return NinjaEnvGuard
Test-->>API: drop NinjaEnvGuard
API->>Lock: reacquire lock
alt original existed
API->>OS: set NINJA_ENV = original
else no original
API->>OS: remove NINJA_ENV
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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
♻️ Duplicate comments (5)
tests/ninja_env_tests.rs (4)
4-4: Import NINJA_ENV from test_support instead.The constant is now duplicated across multiple files. Import it from
test_support::envto maintain a single source of truth.-use netsuke::runner::NINJA_ENV; +use test_support::env::NINJA_ENV;Also update
test_support/src/env.rsto export the constant:pub const NINJA_ENV: &str = "NETSUKE_NINJA";
20-20: Improve the expect message.Provide a more descriptive message as per the coding guidelines.
- let after = std::env::var(NINJA_ENV).expect("env var"); + let after = std::env::var(NINJA_ENV).expect("NINJA_ENV should be set after override");
24-25: Fix test logic inconsistency.The test expects the mock to return "restored" but then asserts that the real environment variable contains "restored". This creates confusion about what's being tested. The guard's Drop implementation uses
std::env::set_var/remove_vardirectly, not the mock's methods.The guard should either:
- Use the mock for restoration (which it doesn't), or
- The test should verify restoration to the original value captured before the override
- env.expect_raw() - .withf(|k| k == NINJA_ENV) - .returning(|_| Ok("restored".to_string())); + env.expect_raw() + .withf(|k| k == NINJA_ENV) + .returning(move |_| before.as_ref().map(|s| s.to_string_lossy().into_owned()).ok_or("not set".to_string()));And update the assertion:
- let restored = std::env::var(NINJA_ENV).expect("env var"); - assert_eq!(restored, "restored"); + let restored = std::env::var_os(NINJA_ENV); + assert_eq!(restored, before);
26-34: Remove redundant manual cleanup.The manual cleanup duplicates the logic already implemented in
NinjaEnvGuard::drop(). This code is unnecessary since the test is already wrapped with#[serial]for isolation.- let _lock = EnvLock::acquire(); - // SAFETY: `EnvLock` serialises access while the variable is reset. - unsafe { - if let Some(val) = before { - std::env::set_var(NINJA_ENV, val); - } else { - std::env::remove_var(NINJA_ENV); - } - }test_support/src/env.rs (1)
90-90: Export the constant to avoid duplication.The NINJA_ENV constant is duplicated from runner.rs. Export it publicly to allow other modules to import it, maintaining a single source of truth.
-const NINJA_ENV: &str = "NETSUKE_NINJA"; +pub const NINJA_ENV: &str = "NETSUKE_NINJA";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
test_support/src/env.rs(1 hunks)tests/ninja_env_tests.rs(1 hunks)tests/runner_tests.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
tests/ninja_env_tests.rstest_support/src/env.rstests/runner_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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()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.
Files:
tests/ninja_env_tests.rstest_support/src/env.rstests/runner_tests.rs
🧬 Code Graph Analysis (3)
tests/ninja_env_tests.rs (3)
tests/env_path_tests.rs (1)
std(39-41)test_support/src/env.rs (5)
override_ninja_env(116-122)drop(125-135)set_var(24-24)set_var(28-30)set_var(34-36)test_support/src/env_lock.rs (1)
acquire(17-21)
test_support/src/env.rs (2)
test_support/src/env_lock.rs (1)
acquire(17-21)test_support/src/path_guard.rs (3)
drop(75-84)set_var(18-18)set_var(25-27)
tests/runner_tests.rs (1)
test_support/src/env.rs (2)
override_ninja_env(116-122)drop(125-135)
⏰ 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). (1)
- GitHub Check: build-test
🔇 Additional comments (9)
tests/ninja_env_tests.rs (1)
1-1: Add module-level documentation.Add a module-level comment (
//!) explaining the module's purpose and utility, as required by the coding guidelines.+//! Tests for the NINJA_ENV override guard functionality. +//! +//! Validates that the `override_ninja_env` function correctly sets and restores +//! the NINJA_ENV environment variable using RAII semantics. //! Tests for overriding the `NINJA_ENV` variable via a mock environment.Likely an incorrect or invalid review comment.
tests/runner_tests.rs (4)
2-2: LGTM! Clean refactor to use the new guard API.The removal of
NINJA_ENVfrom imports aligns with the API change where it's no longer exported fromnetsuke::runner.
8-8: LGTM! Good adoption of the new guard-based API.The import of
override_ninja_envfrom test_support replaces the previous manual environment manipulation approach.
211-212: LGTM! Clean RAII pattern implementation.The use of
SystemEnvandoverride_ninja_envproperly encapsulates the environment override with automatic restoration via the guard's Drop implementation.
229-229: LGTM! Explicit drop for clarity.The explicit
drop(guard)makes the restoration point clear, though it would happen automatically at scope end.test_support/src/env.rs (4)
92-96: LGTM! Well-documented guard struct.The struct is properly documented and follows the RAII pattern for environment variable restoration.
98-115: LGTM! Excellent documentation with example.The function is well-documented with clear safety notes and a practical example demonstrating usage.
116-122: LGTM! Proper synchronisation and safety handling.The function correctly acquires the lock before mutation and documents the safety invariant.
124-136: LGTM! Correct Drop implementation with proper synchronisation.The Drop implementation properly acquires the lock and restores the original value or removes the variable as appropriate.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
test_support/Cargo.toml(1 hunks)test_support/src/env.rs(2 hunks)tests/ninja_env_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
test_support/src/env.rstests/ninja_env_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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()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.
Files:
test_support/src/env.rstests/ninja_env_tests.rs
🧬 Code Graph Analysis (2)
test_support/src/env.rs (2)
test_support/src/env_lock.rs (1)
acquire(17-21)test_support/src/path_guard.rs (3)
drop(75-84)set_var(18-18)set_var(25-27)
tests/ninja_env_tests.rs (2)
test_support/src/env.rs (2)
override_ninja_env(116-122)drop(125-135)test_support/src/env_lock.rs (1)
acquire(17-21)
⏰ 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). (1)
- GitHub Check: build-test
🔇 Additional comments (1)
test_support/src/env.rs (1)
7-7: Import the source-of-truth constant to avoid drift.Import
NINJA_ENVfromnetsuke::runnerrather than duplicating a local constant to prevent divergence if the value changes upstream.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainBased on my analysis of the code: @leynos Yes, both concerns have been resolved: ✅ NINJA_ENV constant centralization: The constant is now centralized in the use netsuke::runner::NINJA_ENV;This means both modules reference the same constant definition rather than having duplicate definitions. ✅ Test for unset NINJA_ENV: The test
Both issues from your previous feedback have been properly addressed in this PR. 🏁 Script executed: Length of output: 12052 @leynos Yes, both concerns have been fully resolved: ✅ NINJA_ENV constant centralization: The constant is properly centralized in pub const NINJA_ENV: &str = "NETSUKE_NINJA";And imported in both test modules:
There's no duplication - both modules reference the same constant definition. ✅ Test for unset NINJA_ENV: The test
This test ensures that when Both issues from your previous feedback have been properly addressed. |
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
Cargo.toml (1)
1-78: Unify MSRV and lint policy at the workspace levelAdd these sections to your root Cargo.toml:
[workspace.package] rust-version = "1.85.0" [workspace.lints.clippy] pedantic = { level = "warn", priority = -1 } allow_attributes_without_reason = "deny" blanket_clippy_restriction_lints = "deny" unwrap_used = "deny" indexing_slicing = "deny" string_slice = "deny" integer_division = "deny" integer_division_remainder_used = "deny" host_endian_bytes = "deny" little_endian_bytes = "deny" big_endian_bytes = "deny" or_fun_call = "deny" option_if_let_else = "deny" use_self = "deny" string_lit_as_bytes = "deny" float_arithmetic = "deny"Cargo supports
[workspace.package] rust-versionfrom 1.84.0, and the 2024 edition requires Rust 1.85.0. Ensure CI uses at least Rust 1.85.0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)ninja_env/Cargo.toml(1 hunks)ninja_env/src/lib.rs(1 hunks)src/runner.rs(1 hunks)test_support/Cargo.toml(1 hunks)test_support/src/env.rs(2 hunks)tests/ninja_env_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Mandate caret requirements for all dependencies. All crate versions specified in Cargo.toml must use SemVer-compatible caret requirements (e.g., some-crate = "1.2.3").
Prohibit unstable version specifiers. The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
ninja_env/src/lib.rstests/ninja_env_tests.rssrc/runner.rstest_support/src/env.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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()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.
Files:
ninja_env/src/lib.rstests/ninja_env_tests.rssrc/runner.rstest_support/src/env.rs
🧬 Code Graph Analysis (2)
tests/ninja_env_tests.rs (2)
test_support/src/env.rs (4)
override_ninja_env(116-122)set_var(25-25)set_var(29-31)set_var(35-37)test_support/src/env_lock.rs (1)
acquire(17-21)
test_support/src/env.rs (2)
test_support/src/env_lock.rs (1)
acquire(17-21)test_support/src/path_guard.rs (3)
drop(75-84)set_var(18-18)set_var(25-27)
⏰ 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). (1)
- GitHub Check: build-test
🔇 Additional comments (10)
test_support/Cargo.toml (1)
11-11: LGTM: Depend on the shared ninja_env crate.Depend on
ninja_envhere to reference the canonical constant and avoid redefinition. This keeps tests in lock-step with the runtime semantics without reintroducing cycles.Cargo.toml (1)
22-22: LGTM: Introduce a path dependency on ninja_env.Centralise the
NINJA_ENVsymbol in a dedicated crate and re-export from the runtime. This preserves the public API while improving cohesion.src/runner.rs (2)
23-23: LGTM: Re-export NINJA_ENV to preserve the public API while centralising the source.Re-exporting from
ninja_envkeeps external users onnetsuke::runner::NINJA_ENVwithout duplication. This avoids drift and clarifies ownership.
23-23: Approve centralised NINJA_ENV definitionNo other
const NINJA_ENVfound outsideninja_env/src/lib.rs. All imports reference the single central constant. Keep thepub use ninja_env::NINJA_ENV;insrc/runner.rsas is.test_support/src/env.rs (3)
7-7: Centralise constant import to avoid drift — LGTMImport
NINJA_ENVfrom the single source of truth. This removes duplication and eliminates divergence risk.
116-122: Fix TOCTOU by acquiring the lock before read and set — LGTMAcquire
EnvLockbefore reading and writing to eliminate races with other well-behaved callers.
124-135: Restore under lock with narrowly scoped unsafe — LGTMReacquire
EnvLockfor restoration and keepunsafetightly scoped to the actual mutations. This aids auditing and correctness.tests/ninja_env_tests.rs (3)
1-9: Test structure and serialisation — LGTMScope the overrides and serialise execution with
serial_test. Imports target the public API surface.
30-36: Serialise setup that mutates global state — LGTMAcquire
EnvLockand confine the unsaferemove_varto setup. This keeps tests deterministic and isolated.
49-59: Restore original environment state at the end — LGTMReacquire
EnvLockand restore/remove as appropriate to avoid leaking process-global state across tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
ninja_env/Cargo.toml (1)
10-49: Promote pedantic to deny to uphold “no warnings” policyEliminate warning chaff from this crate by denying pedantic. This keeps the lint posture consistent with “Clippy warnings MUST be disallowed”.
Apply:
[lints.clippy] -pedantic = { level = "warn", priority = -1 } +pedantic = { level = "deny", priority = -1 }
♻️ Duplicate comments (2)
test_support/src/env.rs (2)
91-96: Enforce RAII correctness by marking the guard #[must_use]Prevent accidental immediate drop that silently reverts the override.
Apply:
/// Guard that restores `NINJA_ENV` to its previous value on drop. -#[derive(Debug)] +#[derive(Debug)] +#[must_use = "Dropping this guard immediately defeats the override; bind it for the intended scope"] pub struct NinjaEnvGuard { original: Option<OsString>, }
97-116: Clarify lock semantics in docs; keep the example OS-portableDocument that the guard serialises writes during construction/restoration only and does not hold the lock for its lifetime. The example is already portable; keep it as-is.
Apply:
/// Override the `NINJA_ENV` variable with `path`, returning a guard that resets it. /// /// In Rust 2024 `std::env::set_var` is `unsafe` because it mutates process-global /// state. `EnvLock` serialises the mutation and the guard restores the prior /// value, confining the unsafety to the scope of the guard. /// +/// Note: The guard does not hold `EnvLock` for its entire lifetime; it only +/// serialises writes during construction and on drop. Acquire `EnvLock` around +/// any other environment mutations while the guard is active to avoid lost updates. /// /// # Examples
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ninja_env/Cargo.toml(1 hunks)test_support/src/env.rs(2 hunks)tests/ninja_env_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Use functions and composition. Avoid repetition by extracting reusable logic. Prefer generators or comprehensions, and declarative code to imperative repetition when readable.
Small, meaningful functions. Functions must be small, clear in purpose, single responsibility, and obey command/query segregation.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer single line versions of functions where appropriate.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Keep file size manageable. No single code file may be longer than 400 lines. Long switch statements or dispatch tables should be broken up by feature and constituents colocated with targets. Large blocks of test data should be moved to external data files.
Illustrate with clear examples. Function documentation must include clear examples demonstrating the usage and outcome of the function. Test documentation should omit examples where the example serves only to reiterate the test logic.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
...
Files:
test_support/src/env.rstests/ninja_env_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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()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.
Files:
test_support/src/env.rstests/ninja_env_tests.rs
🧬 Code Graph Analysis (2)
test_support/src/env.rs (3)
test_support/src/env_lock.rs (1)
acquire(17-21)test_support/src/path_guard.rs (3)
drop(75-84)set_var(18-18)set_var(25-27)tests/env_path_tests.rs (1)
std(39-41)
tests/ninja_env_tests.rs (2)
test_support/src/env.rs (4)
override_ninja_env(116-122)set_var(25-25)set_var(29-31)set_var(35-37)test_support/src/env_lock.rs (1)
acquire(17-21)
⏰ 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). (1)
- GitHub Check: build-test
🔇 Additional comments (8)
ninja_env/Cargo.toml (1)
1-6: Mark crate internal and declare MSRV: LGTMKeep
publish = falseandrust-version = "1.85.0"here. This aligns with an internal utility crate and the 2024 edition.test_support/src/env.rs (3)
7-7: Centralise NINJA_ENV import: LGTMUse the shared constant from the dedicated crate to avoid drift across modules and tests.
116-122: Acquire lock before read and write: LGTMRead the original value and perform the override while holding
EnvLockto avoid TOCTOU races.
124-135: Minimise unsafe surface during restoration: LGTMRestrict unsafe to the exact mutation calls and serialise under
EnvLock. This aids auditing and future refactors.tests/ninja_env_tests.rs (4)
1-7: Serialise tests that mutate process env: LGTMGate with
serial_testand keep the tests isolated behind the same key constant. This avoids flaky interleavings.
12-19: Stub original value correctly and avoid lossy conversions: LGTMCapture
var_osin test 1 and convert fallibly for the mock. This handles non‑UTF‑8 values safely.
21-25: Use OS-portable temp path and descriptive expects: LGTMBuild the target from
temp_dir()and assert with a clear failure message.
52-62: Restore the exact original OS value for test isolationRestore
OsString(if present) under the lock; do not degrade to UTF‑8 only.Apply:
// Restore original global state for isolation { let _lock = EnvLock::acquire(); - if let Some(val) = before { + if let Some(val) = before { // SAFETY: `EnvLock` serialises mutations while restoring. unsafe { std::env::set_var(NINJA_ENV, val) }; } else { // SAFETY: `EnvLock` serialises mutations while restoring. unsafe { std::env::remove_var(NINJA_ENV) }; } }Likely an incorrect or invalid review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ninja_env/src/lib.rs(1 hunks)tests/ninja_env_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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()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.
Files:
ninja_env/src/lib.rstests/ninja_env_tests.rs
🧬 Code Graph Analysis (1)
tests/ninja_env_tests.rs (2)
test_support/src/env.rs (4)
override_ninja_env(116-122)set_var(25-25)set_var(29-31)set_var(35-37)test_support/src/env_lock.rs (1)
acquire(17-21)
🔇 Additional comments (1)
ninja_env/src/lib.rs (1)
1-1: Good: Forbid unsafe code at the crate level.Keep this. It enforces the intended safety guarantees for this crate.
1 participant
Summary
Testing
make fmtmake lint(fails: interrupted; command did not complete)make testhttps://chatgpt.com/codex/tasks/task_e_689d10a97afc83229768883ca3244fb8
Summary by Sourcery
Add a scoped guard for safely overriding the NINJA_ENV environment variable using EnvLock and ensure its original value is restored on drop, with corresponding tests to validate the behavior.
New Features:
Enhancements:
Tests: