Conversation
Reviewer's GuideReplace ad-hoc PATH fixture code with a reusable Env-based prepend_dir_to_path helper that scopes unsafe std::env::set_var under EnvLock and PathGuard, document the mutability hazards in Rust 2024, and add dedicated tests to verify guard behavior. File-Level Changes
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 5 minutes and 54 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 ignored due to path filters (1)
📒 Files selected for processing (7)
Summary by CodeRabbit
WalkthroughAdd a test helper to prepend a directory to PATH under EnvLock with RAII restoration, refactor tests to use that helper and SystemEnv, add serial tests covering normal/empty/missing PATH cases, and adjust PathGuard to represent and restore an unset PATH. Suppress dead-code lint on an existing test helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Env as Env (SystemEnv / Mock)
participant Lock as EnvLock
participant Guard as PathGuard
Note over Test,Env: Request to prepend dir to PATH
Test->>Env: read PATH (Option<String>)
Test->>Lock: acquire
Lock->>Env: unsafe set_var(PATH = dir + (if PATH Some then ":" + PATH else "" ))
Lock-->>Test: release
Env-->>Guard: return PathGuard(original = Some(Set(path)) | Some(Unset))
Note over Guard: On drop, match original -> restore or remove PATH under EnvLock
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 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}
tests/env_path_tests.rs
Outdated
| #[serial] | ||
| fn prepend_dir_to_path_sets_and_restores() { | ||
| let env = mocked_path_env(); | ||
| let original = env.raw("PATH").expect("PATH missing in mock"); |
There was a problem hiding this comment.
style: Consider using .expect("PATH should be set in mock") for more descriptive error message
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider using
env.set_var(...)inprepend_dir_to_pathinstead ofstd::env::set_varso the helper fully respects theEnvtrait and works correctly with mocked environments. - Rename
DefaultEnvto something likeSystemEnvorRealEnvto more clearly indicate it interacts with the real process environment.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using `env.set_var(...)` in `prepend_dir_to_path` instead of `std::env::set_var` so the helper fully respects the `Env` trait and works correctly with mocked environments.
- Rename `DefaultEnv` to something like `SystemEnv` or `RealEnv` to more clearly indicate it interacts with the real process environment.
## Individual Comments
### Comment 1
<location> `tests/support/env.rs:54` </location>
<code_context>
+/// globals. `EnvLock` serialises access and `PathGuard` rolls back the change,
+/// keeping the unsafety scoped to a single test.
+#[allow(dead_code, reason = "used in runner tests")]
+pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard {
+ let original = env.raw("PATH").unwrap_or_default();
+ let original_os: OsString = original.clone().into();
+ let mut paths: Vec<_> = std::env::split_paths(&original_os).collect();
+ paths.insert(0, dir.to_path_buf());
+ let new_path = std::env::join_paths(paths).expect("join paths");
+ let _lock = EnvLock::acquire();
+ // SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`.
+ unsafe { std::env::set_var("PATH", &new_path) };
+ PathGuard::new(original_os)
+}
</code_context>
<issue_to_address>
Consider adding a test for prepend_dir_to_path with an empty PATH variable.
Testing with an empty or unset PATH will ensure prepend_dir_to_path handles these cases correctly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[allow(dead_code, reason = "used in runner tests")]
pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard {
let original = env.raw("PATH").unwrap_or_default();
let original_os: OsString = original.clone().into();
let mut paths: Vec<_> = std::env::split_paths(&original_os).collect();
paths.insert(0, dir.to_path_buf());
let new_path = std::env::join_paths(paths).expect("join paths");
let _lock = EnvLock::acquire();
// SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`.
unsafe { std::env::set_var("PATH", &new_path) };
PathGuard::new(original_os)
}
=======
#[allow(dead_code, reason = "used in runner tests")]
pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard {
let original = env.raw("PATH").unwrap_or_default();
let original_os: OsString = original.clone().into();
let mut paths: Vec<_> = std::env::split_paths(&original_os).collect();
paths.insert(0, dir.to_path_buf());
let new_path = std::env::join_paths(paths).expect("join paths");
let _lock = EnvLock::acquire();
// SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`.
unsafe { std::env::set_var("PATH", &new_path) };
PathGuard::new(original_os)
}
#[cfg(test)]
mod tests {
use super::*;
use std::env;
use std::ffi::OsString;
use std::path::PathBuf;
struct TestEnv;
impl Env for TestEnv {
fn raw(&self, key: &str) -> Option<String> {
env::var(key).ok()
}
}
#[test]
fn test_prepend_dir_to_path_with_empty_path() {
let _lock = EnvLock::acquire();
// Save original PATH
let original_path = env::var_os("PATH");
// Set PATH to empty
env::remove_var("PATH");
let test_dir = PathBuf::from("/test/dir");
let env = TestEnv;
let guard = prepend_dir_to_path(&env, &test_dir);
let new_path = env::var_os("PATH").unwrap();
let mut paths: Vec<_> = env::split_paths(&new_path).collect();
assert_eq!(paths.len(), 1);
assert_eq!(paths[0], test_dir);
// Drop guard to restore original PATH
drop(guard);
let restored_path = env::var_os("PATH");
assert_eq!(restored_path, original_path);
}
#[test]
fn test_prepend_dir_to_path_with_unset_path() {
let _lock = EnvLock::acquire();
// Save original PATH
let original_path = env::var_os("PATH");
// Unset PATH
env::remove_var("PATH");
let test_dir = PathBuf::from("/another/test/dir");
let env = TestEnv;
let guard = prepend_dir_to_path(&env, &test_dir);
let new_path = env::var_os("PATH").unwrap();
let mut paths: Vec<_> = env::split_paths(&new_path).collect();
assert_eq!(paths.len(), 1);
assert_eq!(paths[0], test_dir);
// Drop guard to restore original PATH
drop(guard);
let restored_path = env::var_os("PATH");
assert_eq!(restored_path, original_path);
}
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #[allow(dead_code, reason = "used in runner tests")] | ||
| pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard { | ||
| let original = env.raw("PATH").unwrap_or_default(); | ||
| let original_os: OsString = original.clone().into(); | ||
| let mut paths: Vec<_> = std::env::split_paths(&original_os).collect(); | ||
| paths.insert(0, dir.to_path_buf()); | ||
| let new_path = std::env::join_paths(paths).expect("join paths"); | ||
| let _lock = EnvLock::acquire(); | ||
| // SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`. | ||
| unsafe { std::env::set_var("PATH", &new_path) }; | ||
| PathGuard::new(original_os) | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for prepend_dir_to_path with an empty PATH variable.
Testing with an empty or unset PATH will ensure prepend_dir_to_path handles these cases correctly.
| #[allow(dead_code, reason = "used in runner tests")] | |
| pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard { | |
| let original = env.raw("PATH").unwrap_or_default(); | |
| let original_os: OsString = original.clone().into(); | |
| let mut paths: Vec<_> = std::env::split_paths(&original_os).collect(); | |
| paths.insert(0, dir.to_path_buf()); | |
| let new_path = std::env::join_paths(paths).expect("join paths"); | |
| let _lock = EnvLock::acquire(); | |
| // SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`. | |
| unsafe { std::env::set_var("PATH", &new_path) }; | |
| PathGuard::new(original_os) | |
| } | |
| #[allow(dead_code, reason = "used in runner tests")] | |
| pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard { | |
| let original = env.raw("PATH").unwrap_or_default(); | |
| let original_os: OsString = original.clone().into(); | |
| let mut paths: Vec<_> = std::env::split_paths(&original_os).collect(); | |
| paths.insert(0, dir.to_path_buf()); | |
| let new_path = std::env::join_paths(paths).expect("join paths"); | |
| let _lock = EnvLock::acquire(); | |
| // SAFETY: protected by `EnvLock` and reverted by the returned `PathGuard`. | |
| unsafe { std::env::set_var("PATH", &new_path) }; | |
| PathGuard::new(original_os) | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| use std::env; | |
| use std::ffi::OsString; | |
| use std::path::PathBuf; | |
| struct TestEnv; | |
| impl Env for TestEnv { | |
| fn raw(&self, key: &str) -> Option<String> { | |
| env::var(key).ok() | |
| } | |
| } | |
| #[test] | |
| fn test_prepend_dir_to_path_with_empty_path() { | |
| let _lock = EnvLock::acquire(); | |
| // Save original PATH | |
| let original_path = env::var_os("PATH"); | |
| // Set PATH to empty | |
| env::remove_var("PATH"); | |
| let test_dir = PathBuf::from("/test/dir"); | |
| let env = TestEnv; | |
| let guard = prepend_dir_to_path(&env, &test_dir); | |
| let new_path = env::var_os("PATH").unwrap(); | |
| let mut paths: Vec<_> = env::split_paths(&new_path).collect(); | |
| assert_eq!(paths.len(), 1); | |
| assert_eq!(paths[0], test_dir); | |
| // Drop guard to restore original PATH | |
| drop(guard); | |
| let restored_path = env::var_os("PATH"); | |
| assert_eq!(restored_path, original_path); | |
| } | |
| #[test] | |
| fn test_prepend_dir_to_path_with_unset_path() { | |
| let _lock = EnvLock::acquire(); | |
| // Save original PATH | |
| let original_path = env::var_os("PATH"); | |
| // Unset PATH | |
| env::remove_var("PATH"); | |
| let test_dir = PathBuf::from("/another/test/dir"); | |
| let env = TestEnv; | |
| let guard = prepend_dir_to_path(&env, &test_dir); | |
| let new_path = env::var_os("PATH").unwrap(); | |
| let mut paths: Vec<_> = env::split_paths(&new_path).collect(); | |
| assert_eq!(paths.len(), 1); | |
| assert_eq!(paths[0], test_dir); | |
| // Drop guard to restore original PATH | |
| drop(guard); | |
| let restored_path = env::var_os("PATH"); | |
| assert_eq!(restored_path, original_path); | |
| } | |
| } |
tests/support/env.rs
Outdated
| let original_os: OsString = original.clone().into(); | ||
| let mut paths: Vec<_> = std::env::split_paths(&original_os).collect(); | ||
| paths.insert(0, dir.to_path_buf()); | ||
| let new_path = std::env::join_paths(paths).expect("join paths"); |
There was a problem hiding this comment.
style: Consider using .expect("Failed to join PATH entries") for more descriptive error message
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
tests/runner_tests.rs (1)
212-245: Inconsistent safety approach in environment override test.This test still uses raw
unsafeblocks forset_varandremove_varwithout theEnvLockprotection that other tests use. Refactor to use the same safe pattern or document why this test requires direct manipulation.#[test] #[serial] fn run_respects_env_override_for_ninja() { let (temp_dir, ninja_path) = support::fake_ninja(0); - let original = std::env::var_os(NINJA_ENV); - unsafe { - std::env::set_var(NINJA_ENV, &ninja_path); - } + let original = std::env::var_os(NINJA_ENV); + { + let _lock = support::env_lock::EnvLock::acquire(); + unsafe { + std::env::set_var(NINJA_ENV, &ninja_path); + } + } // ... test logic ... - unsafe { - if let Some(val) = original { - std::env::set_var(NINJA_ENV, val); - } else { - std::env::remove_var(NINJA_ENV); + { + let _lock = support::env_lock::EnvLock::acquire(); + unsafe { + if let Some(val) = original { + std::env::set_var(NINJA_ENV, val); + } else { + std::env::remove_var(NINJA_ENV); + } } }
♻️ Duplicate comments (1)
tests/env_path_tests.rs (1)
15-15: Descriptive error message already implemented.The test already uses
.expect("PATH should be set in mock")as suggested in the past review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/env_path_tests.rs(1 hunks)tests/runner_tests.rs(2 hunks)tests/support/env.rs(3 hunks)tests/support/mod.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:
tests/env_path_tests.rstests/runner_tests.rstests/support/mod.rstests/support/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:
tests/env_path_tests.rstests/runner_tests.rstests/support/mod.rstests/support/env.rs
🧬 Code Graph Analysis (3)
tests/env_path_tests.rs (3)
tests/support/env.rs (2)
mocked_path_env(21-28)prepend_dir_to_path(54-65)tests/support/path_guard.rs (1)
drop(30-36)tests/support/env_lock.rs (1)
acquire(19-21)
tests/runner_tests.rs (5)
tests/support/env.rs (1)
prepend_dir_to_path(54-65)tests/support/check_ninja.rs (1)
fake_ninja_check_build_file(12-36)tests/support/path_guard.rs (1)
new(22-26)tests/support/mod.rs (1)
fake_ninja(21-34)tests/steps/process_steps.rs (1)
fake_ninja(36-40)
tests/support/env.rs (3)
tests/env_path_tests.rs (1)
std(40-42)tests/support/env_lock.rs (1)
acquire(19-21)tests/support/path_guard.rs (1)
new(22-26)
🔍 MCP Research (1 server)
Context7:
Selected library ID: none
Reason: No relevant library matches found for the query. Please provide a repository or package identifier (for example "leynos/netsuke" or "/org/project") or a file/path from the repo so I can resolve and fetch documentation specific to those code changes.
⏰ 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/support/mod.rs (1)
20-20: LGTM! Correct lint suppression for cross-crate test usage.The
#[allow(dead_code)]attribute with a clear reason correctly addresses the lint warning for this function that's used by other test crates.tests/support/env.rs (2)
6-13: LGTM! Clean imports and proper module structure.The imports align with the new PATH manipulation functionality and the module organisation follows established patterns.
48-65: Excellent documentation of unsafe PATH mutation hazards.The function correctly documents the Rust 2024 unsafe nature of
std::env::set_varand provides a well-architected solution usingEnvLockfor serialisation andPathGuardfor RAII restoration. The implementation properly handles the original PATH retrieval, prepending logic, and guard creation.tests/runner_tests.rs (3)
1-1: LGTM! Clean integration of environment abstraction.The
SystemEnvalias and module imports properly integrate the new environment abstraction pattern for PATH manipulation.Also applies to: 10-13
16-29: Excellent documentation of unsafe PATH mutation mitigation.The fixture documentation clearly explains the Rust 2024 safety concerns and how
EnvLockandPathGuardaddress them. The implementation correctly uses the new helper function.
31-44: LGTM! Consistent pattern for PATH fixture with exit code.The fixture follows the same safe pattern as
ninja_in_pathand properly documents the approach. The parameterised exit code functionality is maintained whilst adopting the safer PATH manipulation.tests/env_path_tests.rs (3)
1-9: LGTM! Proper test module setup and imports.The imports correctly bring in the environment abstraction, test utilities, and serialisation support needed for PATH manipulation testing.
11-26: LGTM! Comprehensive test of PATH prepend and restore behaviour.The test correctly verifies that
prepend_dir_to_pathplaces the directory first in PATH and that thePathGuardrestores the original value when dropped. The use ofmocked_path_envproperly isolates the test.
28-53: Excellent edge case testing for empty PATH.This test properly validates the behaviour when PATH is empty, correctly using
EnvLockfor safe manipulation and ensuring proper restoration. The filtering of empty path entries on lines 40-42 correctly handles the edge case where an empty PATH might introduce empty path components.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
tests/support/env.rs (2)
33-46: Replace #[allow] with #[expect] on write_manifest.Keep lint policy consistent.
Apply this diff:
-#[allow(dead_code, reason = "used in Cucumber tests")] +#[expect(dead_code, reason = "used in Cucumber tests")] pub fn write_manifest(file: &mut impl Write) -> io::Result<()> {
53-68: Replace #[allow] with #[expect] on prepend_dir_to_path.Apply the repo lint policy and keep the suppression tightly scoped.
Apply this diff:
-#[allow(dead_code, reason = "used in runner tests")] +#[expect(dead_code, reason = "used in runner tests")] pub fn prepend_dir_to_path(env: &impl Env, dir: &Path) -> PathGuard {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/env_path_tests.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)tests/support/env.rs(3 hunks)tests/support/path_guard.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:
tests/steps/process_steps.rstests/env_path_tests.rstests/support/env.rstests/support/path_guard.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/steps/process_steps.rstests/env_path_tests.rstests/support/env.rstests/support/path_guard.rs
🧬 Code Graph Analysis (4)
tests/steps/process_steps.rs (1)
tests/support/path_guard.rs (1)
new(29-34)
tests/env_path_tests.rs (3)
tests/support/env.rs (2)
mocked_path_env(21-28)prepend_dir_to_path(54-68)tests/support/path_guard.rs (2)
drop(38-47)new(29-34)tests/support/env_lock.rs (1)
acquire(19-21)
tests/support/env.rs (3)
tests/env_path_tests.rs (1)
std(40-42)tests/support/env_lock.rs (1)
acquire(19-21)tests/support/path_guard.rs (1)
new(29-34)
tests/support/path_guard.rs (2)
tests/support/env_lock.rs (1)
acquire(19-21)tests/env_path_tests.rs (1)
std(40-42)
⏰ 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 (5)
tests/support/path_guard.rs (1)
41-46: LGTM: Scoped restoration logic is correct and threadsafe under EnvLock.Match on the captured state, serialise with EnvLock, and revert via set/remove is the right approach given Rust 2024’s
unsafeon env mutation.tests/support/env.rs (2)
21-28: Preserve “unset PATH” semantics in the mock.Do not collapse an unset PATH to an empty string. Align the mock with real behaviour so callers can distinguish None (unset) from Some("") (set but empty). If
mockable::MockEnvcannot express “unset”, return an error forraw("PATH")when it was unset at capture time and let call sites use.ok()to recoverNone.I can draft a patch once you confirm the error type to return from
MockEnv::rawfor a missing key.
48-69: Good: Scoped, cross‑platform PATH mutation with EnvLock + RAII.Read via Env, build with split/join_paths, serialise with EnvLock, and restore via PathGuard. This centralises the hazard and avoids platform separator bugs.
tests/env_path_tests.rs (2)
28-54: LGTM: Validate empty PATH handling and restoration.Serialise with EnvLock, assert that only the new dir is in PATH, and verify restoration to an empty PATH after guard drop.
56-80: LGTM: Validate missing PATH handling and restoration.Unset PATH, prepend safely, assert the single entry, and ensure PATH is removed again on drop.
Explain restoration behaviour in PathGuard::new and streamline path splitting in tests.
leynos
force-pushed
the
codex/expand-comments-in-ninja-fixtures-and-refactor
branch
from
9032e68 to
fde9d5f
Compare
leynos
deleted the
codex/expand-comments-in-ninja-fixtures-and-refactor
branch
1 participant
Summary
set_varunsafety in PATH fixturesEnvinto PATH helper and fixturesTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_689a8cdfc7d08322b0fc8982a7d7474f
Summary by Sourcery
Refactor test fixtures to use a new Env-based helper for scoped PATH mutations, document the unsafety of std::env::set_var under Rust 2024, and add tests to validate PathGuard behavior.
Enhancements:
Documentation:
Tests: