Conversation
Reviewer's GuideThis PR refactors the runner module to inject an Env abstraction, modifying runner::run, handle_build, and resolve_ninja_program to accept an Env implementation; updates main.rs to supply DefaultEnv; adapts tests to use DefaultEnv and MockEnv; and adds the mockable crate for environment mocking. Sequence diagram for runner::run with injected EnvsequenceDiagram
participant main_rs
participant DefaultEnv
participant runner
participant Env
main_rs->>DefaultEnv: new()
main_rs->>runner: run(&cli, &env)
runner->>runner: handle_build(&cli, &args, &env)
runner->>runner: resolve_ninja_program(&env)
runner->>Env: raw(NINJA_ENV)
Env-->>runner: returns value or error
runner->>runner: run_ninja(...)
runner-->>main_rs: returns Result
Class diagram for runner function signatures after Env injectionclassDiagram
class runner {
+run(cli: &Cli, env: &impl Env) -> Result<()>
+handle_build(cli: &Cli, args: &BuildArgs, env: &impl Env) -> Result<()>
+resolve_ninja_program(env: &impl Env) -> PathBuf
}
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. Summary by CodeRabbit
WalkthroughInject an environment abstraction into the runtime. Promote mockable from dev to runtime dependency. Update main to construct DefaultEnv and pass it to runner::run. Change runner API and internal resolution to use Env for Ninja selection via NINJA_ENV. Update tests to use DefaultEnv/MockEnv and validate overrides and fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Args
participant Main
participant Runner
participant Env
participant Ninja
User->>CLI: Provide args
Main->>Env: Construct DefaultEnv
Main->>Runner: run(&cli, &env)
Runner->>Env: raw("NINJA_ENV")
alt Env provides path
Runner->>Ninja: Execute at env path
else Not set
Runner->>Ninja: Execute default "ninja"
end
Ninja-->>Runner: Exit status
Runner-->>Main: Result
Main-->>User: ExitCode/Errors
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}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Add unit and behavioural tests for resolve_ninja_program's Env-based override logic. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/runner_tests.rs:247` </location>
<code_context>
+ std::env::set_var(NINJA_ENV, "does-not-exist");
}
+ let mut env = MockEnv::new();
+ let path_string = ninja_path
+ .to_str()
+ .expect("ninja path is valid UTF-8")
+ .to_string();
+ env.expect_raw().returning(move |key| {
+ assert_eq!(key, NINJA_ENV);
+ Ok(path_string.clone())
</code_context>
<issue_to_address>
Consider adding a test for when NINJA_ENV is not set in the environment abstraction.
Please add a test case where the environment abstraction does not provide NINJA_ENV, verifying that the default ninja path is used.
Suggested implementation:
```rust
fn run_respects_env_override_for_ninja() {
let (temp_dir, ninja_path) = support::fake_ninja(0);
let original = std::env::var_os(NINJA_ENV);
// SAFETY: Rust 2024 marks `set_var` as unsafe. This test injects a bogus
// value and restores `NINJA_ENV` afterwards to avoid leaking state.
unsafe {
std::env::set_var(NINJA_ENV, "does-not-exist");
}
let mut env = MockEnv::new();
let path_string = ninja_path
.to_str()
.expect("ninja path is valid UTF-8")
.to_string();
env.expect_raw().returning(move |key| {
assert_eq!(key, NINJA_ENV);
Ok(path_string.clone())
});
let temp = tempfile::tempdir().expect("temp dir");
let manifest_path = temp.path().join("Netsukefile");
std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest");
})),
};
let result = run(&cli, &env);
}
// Test that when NINJA_ENV is not set in the environment abstraction, the default ninja path is used.
#[test]
fn run_uses_default_ninja_path_when_env_not_set() {
let (temp_dir, ninja_path) = support::fake_ninja(0);
// Do not set NINJA_ENV in the real environment.
std::env::remove_var(NINJA_ENV);
// MockEnv returns None for NINJA_ENV, simulating it not being set.
let mut env = MockEnv::new();
env.expect_raw().returning(|key| {
if key == NINJA_ENV {
Ok(None)
} else {
Ok(None)
}
});
let temp = tempfile::tempdir().expect("temp dir");
let manifest_path = temp.path().join("Netsukefile");
std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest");
// Prepare CLI and run
let cli = Cli {
manifest_path: manifest_path.clone(),
// ... other CLI setup as needed
};
let result = run(&cli, &env);
// Assert that the default ninja path is used
// (You may need to adjust this assertion based on how your code exposes the used ninja path)
assert!(result.used_ninja_path == support::default_ninja_path());
}
```
- You may need to adjust the assertion `result.used_ninja_path == support::default_ninja_path()` to match how your code exposes the ninja path that was actually used.
- Ensure that the `run` function and its result type provide a way to inspect which ninja path was used, or adapt the test to check the correct behavior.
- If your test framework requires a specific setup for `Cli`, fill in the necessary fields.
</issue_to_address>
### Comment 2
<location> `src/runner.rs:281` </location>
<code_context>
#[must_use]
-fn resolve_ninja_program() -> PathBuf {
- std::env::var_os(NINJA_ENV).map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from)
+fn resolve_ninja_program(env: &impl Env) -> PathBuf {
+ env.raw(NINJA_ENV)
+ .ok()
</code_context>
<issue_to_address>
Add unit and behavioural tests for resolve_ninja_program's Env-based override logic.
The resolve_ninja_program function now uses an Env parameter to determine the Ninja executable path. You must add tests to verify that environment overrides are respected and that the fallback logic works as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let mut env = MockEnv::new(); | ||
| let path_string = ninja_path | ||
| .to_str() | ||
| .expect("ninja path is valid UTF-8") | ||
| .to_string(); | ||
| env.expect_raw().returning(move |key| { |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for when NINJA_ENV is not set in the environment abstraction.
Please add a test case where the environment abstraction does not provide NINJA_ENV, verifying that the default ninja path is used.
Suggested implementation:
fn run_respects_env_override_for_ninja() {
let (temp_dir, ninja_path) = support::fake_ninja(0);
let original = std::env::var_os(NINJA_ENV);
// SAFETY: Rust 2024 marks `set_var` as unsafe. This test injects a bogus
// value and restores `NINJA_ENV` afterwards to avoid leaking state.
unsafe {
std::env::set_var(NINJA_ENV, "does-not-exist");
}
let mut env = MockEnv::new();
let path_string = ninja_path
.to_str()
.expect("ninja path is valid UTF-8")
.to_string();
env.expect_raw().returning(move |key| {
assert_eq!(key, NINJA_ENV);
Ok(path_string.clone())
});
let temp = tempfile::tempdir().expect("temp dir");
let manifest_path = temp.path().join("Netsukefile");
std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest");
})),
};
let result = run(&cli, &env);
}
// Test that when NINJA_ENV is not set in the environment abstraction, the default ninja path is used.
#[test]
fn run_uses_default_ninja_path_when_env_not_set() {
let (temp_dir, ninja_path) = support::fake_ninja(0);
// Do not set NINJA_ENV in the real environment.
std::env::remove_var(NINJA_ENV);
// MockEnv returns None for NINJA_ENV, simulating it not being set.
let mut env = MockEnv::new();
env.expect_raw().returning(|key| {
if key == NINJA_ENV {
Ok(None)
} else {
Ok(None)
}
});
let temp = tempfile::tempdir().expect("temp dir");
let manifest_path = temp.path().join("Netsukefile");
std::fs::copy("tests/data/minimal.yml", &manifest_path).expect("copy manifest");
// Prepare CLI and run
let cli = Cli {
manifest_path: manifest_path.clone(),
// ... other CLI setup as needed
};
let result = run(&cli, &env);
// Assert that the default ninja path is used
// (You may need to adjust this assertion based on how your code exposes the used ninja path)
assert!(result.used_ninja_path == support::default_ninja_path());
}- You may need to adjust the assertion
result.used_ninja_path == support::default_ninja_path()to match how your code exposes the ninja path that was actually used. - Ensure that the
runfunction and its result type provide a way to inspect which ninja path was used, or adapt the test to check the correct behavior. - If your test framework requires a specific setup for
Cli, fill in the necessary fields.
| #[must_use] | ||
| fn resolve_ninja_program() -> PathBuf { | ||
| std::env::var_os(NINJA_ENV).map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from) | ||
| fn resolve_ninja_program(env: &impl Env) -> PathBuf { |
There was a problem hiding this comment.
issue (review_instructions): Add unit and behavioural tests for resolve_ninja_program's Env-based override logic.
The resolve_ninja_program function now uses an Env parameter to determine the Ninja executable path. You must add tests to verify that environment overrides are respected and that the fallback logic works as intended.
Review instructions:
Path patterns: **/*
Instructions:
For any new feature or change to an existing feature, both behavioural and unit tests are required.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/runner.rs (1)
1-469: Split runner module; file exceeds 400-line limitThe module is 469 lines, breaching the file size guideline. Extract cohesive parts into submodules: e.g., redact.rs (logging redaction), ninja.rs (process invocation and program resolution), manifest_gen.rs (manifest to Ninja).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
Cargo.toml(1 hunks)src/main.rs(2 hunks)src/runner.rs(6 hunks)tests/runner_tests.rs(9 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:
src/main.rstests/runner_tests.rssrc/runner.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:
src/main.rstests/runner_tests.rssrc/runner.rs
🔍 MCP Research (1 server)
Deepwiki:
- The project uses a custom Jinja function env(var_name: &str) implemented in Rust to read environment variables for manifest rendering; this function is part of the templating integration used during manifest parsing and is registered when the Jinja environment is set up. Source: https://deepwiki.com/search/inject-env-abstraction-into-ru_dfb1bf9d-b0c1-4a17-ab26-73528f7e3526
- The runner module orchestrates manifest loading/parsing and registers custom Jinja functions (including env()), and the repository wiki includes Architecture and Command Line Interface pages relevant to how env() and Jinja integration are used. Source: https://deepwiki.com/search/inject-env-abstraction-into-ru_dfb1bf9d-b0c1-4a17-ab26-73528f7e3526
🔇 Additional comments (5)
src/runner.rs (2)
171-175: LGTM on Env-threaded program resolution in handle_buildPlumb env through handle_build and resolve the ninja program once per invocation.
440-468: Add unit tests for Env overrides and fallbacksThe added tests exercise both override and NotPresent paths for resolve_ninja_program.
src/main.rs (1)
5-5: Wire DefaultEnv through main to runner::runInject the real environment at the app boundary and keep runner testable.
Also applies to: 16-18
tests/runner_tests.rs (2)
286-308: LGTM on default-path behaviour when NINJA_ENV is NotPresentMock Env to return NotPresent and validate PATH-based ninja execution. Serialise because PATH is mutated.
36-49: Constrain unsafe PATH mutation to a minimal, well-scoped guardPathGuard cleanly restores PATH and documents the safety. Keep all PATH mutations behind this guard.
Also applies to: 57-70
| serde_json = "1" | ||
| serde_json_canonicalizer = "0.3" | ||
| tempfile = "3.8.0" | ||
| mockable = { version = "0.3", features = ["mock"] } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not enable mocking features in production builds
Ship the Env abstraction without the "mock" feature to avoid pulling test-only code into release artefacts. Enable "mock" only for tests.
Apply this diff within [dependencies]:
-mockable = { version = "0.3", features = ["mock"] }
+mockable = { version = "0.3" }And add this to [dev-dependencies] to enable mocks in tests only:
mockable = { version = "0.3", features = ["mock"] }🤖 Prompt for AI Agents
In Cargo.toml around line 22, the mockable crate is being pulled into
[dependencies] with the "mock" feature enabled which should only be used for
tests; move or remove the mocked feature from the production dependency and
instead add mockable with features = ["mock"] under [dev-dependencies] so
production builds do not include test-only code. Update [dependencies] to use
mockable without the "mock" feature (or remove it if unused), and add a new
[dev-dependencies] entry mockable = { version = "0.3", features = ["mock"] } so
mocks are only enabled for test builds.
| /// The environment determines which Ninja program to run. Injecting an | ||
| /// implementation of [`Env`] allows tests to control `NINJA_ENV` without | ||
| /// touching global state. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if manifest generation or the Ninja process fails. | ||
| pub fn run(cli: &Cli) -> Result<()> { | ||
| pub fn run(cli: &Cli, env: &impl Env) -> Result<()> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Adopt Env injection on public API and update all call sites
The revised signature for run(cli, env) is sound and improves testability. Verify no remaining call sites still invoke run(&Cli) without env.
As a follow-up, align with guidelines by returning a domain error (thiserror) from run rather than anyhow::Result in library code. Keep opaque errors at the app boundary.
Run this script to find outdated calls and direct std::env usage that should be replaced by Env:
🏁 Script executed:
#!/bin/bash
# 1) Find any calls to runner::run that don't pass two args
rg -n --no-heading $'runner::run\\(' -A 1
# 2) Find any lingering direct env access in src (should be via Env)
rg -n --no-heading 'std::env::(var|vars|set_var|remove_var|var_os)' src -g '!**/tests/**'Length of output: 316
MCP Context Analysis:
📊 Deepwiki:
- The project uses a custom Jinja function env(var_name: &str) implemented in Rust to read environment variables for manifest rendering; this function is part of the templating integration used during manifest parsing and is registered when the Jinja environment is set up. Source: https://deepwiki.com/search/inject-env-abstraction-into-ru_dfb1bf9d-b0c1-4a17-ab26-73528f7e3526
- The runner module orchestrates manifest loading/parsing and registers custom Jinja functions (including env()), and the repository wiki includes Architecture and Command Line Interface pages relevant to how env() and Jinja integration are used. Source: https://deepwiki.com/search/inject-env-abstraction-into-ru_dfb1bf9d-b0c1-4a17-ab26-73528f7e3526
🏁 Script executed:
#!/bin/bash
# Show top of runner.rs to inspect imports and Result alias
sed -n '1,200p' src/runner.rs
# Search for any anyhow::Result usage in runner.rs
rg -n --no-heading 'anyhow::Result' src/runner.rs
# Search for a type alias named Result in the file or modules
rg -n --no-heading 'type Result' src/runner.rsLength of output: 5997
Confirm Env injection complete; switch to domain errors
No remaining runner::run calls without the env argument. Remove or replace the commented direct std::env::set_var in runner.rs. Update your public API to return a domain‐specific error rather than anyhow::Result.
• In src/runner.rs at the top, drop use anyhow::{Context, Result}; and import your new error type (e.g. use crate::error::RunnerError;).
• Change the signature
- pub fn run(cli: &Cli, env: &impl Env) -> Result<()> {
+ pub fn run(cli: &Cli, env: &impl Env) -> std::result::Result<(), RunnerError> {• Define a RunnerError enum deriving thiserror::Error for all failure cases inside runner.rs.
• Inside main.rs, continue mapping your domain error to an exit code or logging via .map_err(...)—keep anyhow only at the CLI boundary.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// The environment determines which Ninja program to run. Injecting an | |
| /// implementation of [`Env`] allows tests to control `NINJA_ENV` without | |
| /// touching global state. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns an error if manifest generation or the Ninja process fails. | |
| pub fn run(cli: &Cli) -> Result<()> { | |
| pub fn run(cli: &Cli, env: &impl Env) -> Result<()> { | |
| /// The environment determines which Ninja program to run. Injecting an | |
| /// implementation of [`Env`] allows tests to control `NINJA_ENV` without | |
| /// touching global state. | |
| /// | |
| /// # Errors | |
| /// | |
| /// Returns an error if manifest generation or the Ninja process fails. | |
| -pub fn run(cli: &Cli, env: &impl Env) -> Result<()> { | |
| +pub fn run(cli: &Cli, env: &impl Env) -> std::result::Result<(), RunnerError> { |
🤖 Prompt for AI Agents
In src/runner.rs around lines 103 to 110, remove the commented direct
std::env::set_var and stop using anyhow::Result: drop the top-level import use
anyhow::{Context, Result}; and instead import your domain error type (e.g. use
crate::error::RunnerError;). Change the run signature from pub fn run(cli: &Cli,
env: &impl Env) -> Result<()> to return your domain error (e.g. -> Result<(),
RunnerError>), and implement a RunnerError enum in this file (or in crate::error
and import it) deriving thiserror::Error that covers each failure case currently
mapped to anyhow (manifest generation, process spawn/exit, IO, etc.), updating
internal error returns to produce the appropriate RunnerError variants
(preserving context messages). Ensure there are no remaining calls to run
without the env parameter and keep mapping RunnerError to exit codes or logging
in main.rs (use anyhow only at the CLI boundary when converting RunnerError to
an exit code/log).
| /// # Examples | ||
| /// ```ignore | ||
| /// use mockable::DefaultEnv; | ||
| /// use netsuke::cli::{BuildArgs, Cli}; | ||
| /// use netsuke::runner::handle_build; | ||
| /// let cli = Cli { file: "Netsukefile".into(), directory: None, jobs: None, verbose: false, command: None }; | ||
| /// let args = BuildArgs { emit: None, targets: vec![] }; | ||
| /// handle_build(&cli, &args).unwrap(); | ||
| /// handle_build(&cli, &args, &DefaultEnv::new()).unwrap(); | ||
| /// ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Thread Env into manifest/Jinja env registration to keep environment access consistent
The runner orchestrates manifest loading and Jinja setup. Ensure the custom Jinja env() function reads via the injected Env rather than std::env so tests can fully mock environment-driven template behaviour and to comply with the project's environment-abstraction strategy.
Run this to locate any direct std::env usage or env() registration that isn’t fed from Env:
🏁 Script executed:
#!/bin/bash
# Direct OS env usage in source (should be replaced or wrapped)
rg -n --no-heading 'std::env::(var|var_os|vars|set_var|remove_var)' src -g '!**/tests/**'
# Places where the Jinja env() helper is registered/implemented
rg -n --no-heading 'env\\s*\\(' src | rg -v 'std::env'Length of output: 297
MCP Context Analysis:
📊 Deepwiki:
- The project uses a custom Jinja function env(var_name: &str) implemented in Rust to read environment variables for manifest rendering; this function is part of the templating integration used during manifest parsing and is registered when the Jinja environment is set up. Source: https://deepwiki.com/search/inject-env-abstraction-into-ru_dfb1bf9d-b0c1-4a17-ab26-73528f7e3526
- The runner module orchestrates manifest loading/parsing and registers custom Jinja functions (including env()), and the repository wiki includes Architecture and Command Line Interface pages relevant to how env() and Jinja integration are used. Source: https://deepwiki.com/search/inject-env-abstraction-into-ru_dfb1bf9d-b0c1-4a17-ab26-73528f7e3526
Abstract environment access in runner.rs
Refactor direct OS environment calls to use the injected Env trait so Jinja’s env() helper and the NINJA_ENV override (src/runner.rs:276) use the mockable Env. This ensures consistent environment-abstraction and enables full mocking of template-driven behaviour.
- Replace the direct call at src/runner.rs:276
- unsafe { std::env::set_var(NINJA_ENV, "/custom/ninja"); } + env.set_var(NINJA_ENV, "/custom/ninja");
- In the Jinja setup (around lines 139–147 of src/runner.rs), register the
env()helper by capturing the injectedEnvinstance instead of callingstd::envdirectly. - Update tests to supply a
MockEnvintohandle_buildand verify that template rendering reads from the mockableEnv.
| /// Determine which Ninja executable to invoke, allowing an environment | ||
| /// override. | ||
| /// | ||
| /// # Examples | ||
| /// ```ignore | ||
| /// use mockable::DefaultEnv; | ||
| /// use netsuke::runner::{resolve_ninja_program, NINJA_ENV}; | ||
| /// unsafe { std::env::set_var(NINJA_ENV, "/custom/ninja"); } | ||
| /// let env = DefaultEnv::new(); | ||
| /// assert!(resolve_ninja_program(&env).ends_with("/custom/ninja")); | ||
| /// ``` | ||
| #[must_use] | ||
| fn resolve_ninja_program() -> PathBuf { | ||
| std::env::var_os(NINJA_ENV).map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from) | ||
| fn resolve_ninja_program(env: &impl Env) -> PathBuf { | ||
| env.raw(NINJA_ENV) | ||
| .ok() | ||
| .map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden NINJA_ENV handling: treat empty values as unset
Guard against NINJA_ENV="" which currently resolves to the empty path and likely fails at runtime. Treat empty/whitespace-only values as not present.
Apply this diff:
-fn resolve_ninja_program(env: &impl Env) -> PathBuf {
- env.raw(NINJA_ENV)
- .ok()
- .map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from)
-}
+fn resolve_ninja_program(env: &impl Env) -> PathBuf {
+ env.raw(NINJA_ENV)
+ .ok()
+ .and_then(|s| {
+ let t = s.trim();
+ if t.is_empty() { None } else { Some(t.to_string()) }
+ })
+ .map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Determine which Ninja executable to invoke, allowing an environment | |
| /// override. | |
| /// | |
| /// # Examples | |
| /// ```ignore | |
| /// use mockable::DefaultEnv; | |
| /// use netsuke::runner::{resolve_ninja_program, NINJA_ENV}; | |
| /// unsafe { std::env::set_var(NINJA_ENV, "/custom/ninja"); } | |
| /// let env = DefaultEnv::new(); | |
| /// assert!(resolve_ninja_program(&env).ends_with("/custom/ninja")); | |
| /// ``` | |
| #[must_use] | |
| fn resolve_ninja_program() -> PathBuf { | |
| std::env::var_os(NINJA_ENV).map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from) | |
| fn resolve_ninja_program(env: &impl Env) -> PathBuf { | |
| env.raw(NINJA_ENV) | |
| .ok() | |
| .map_or_else(|| PathBuf::from(NINJA_PROGRAM), PathBuf::from) | |
| } | |
| /// Determine which Ninja executable to invoke, allowing an environment | |
| /// override. | |
| /// | |
| /// # Examples | |
| /// |
🤖 Prompt for AI Agents
In src/runner.rs around lines 269 to 285, resolve_ninja_program currently treats
an empty NINJA_ENV value as a valid path; change the logic to treat empty or
whitespace-only env values as unset. Specifically, retrieve
env.raw(NINJA_ENV).ok(), then filter out strings where s.trim().is_empty()
before mapping to PathBuf, and finally fallback to PathBuf::from(NINJA_PROGRAM)
when no valid env value exists.
| /// use mockable::DefaultEnv; | ||
| /// use netsuke::runner::{resolve_ninja_program, NINJA_ENV}; | ||
| /// unsafe { std::env::set_var(NINJA_ENV, "/custom/ninja"); } | ||
| /// let env = DefaultEnv::new(); | ||
| /// assert!(resolve_ninja_program(&env).ends_with("/custom/ninja")); | ||
| /// ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid unsafe environment mutation in docs; demonstrate with MockEnv
Replace the ignored example that uses unsafe std::env::set_var with a pure MockEnv example.
-/// use mockable::DefaultEnv;
-/// use netsuke::runner::{resolve_ninja_program, NINJA_ENV};
-/// unsafe { std::env::set_var(NINJA_ENV, "/custom/ninja"); }
-/// let env = DefaultEnv::new();
-/// assert!(resolve_ninja_program(&env).ends_with("/custom/ninja"));
+/// use mockable::MockEnv;
+/// use netsuke::runner::resolve_ninja_program;
+/// let mut env = MockEnv::new();
+/// env.expect_raw().returning(|key| {
+/// assert_eq!(key, super::NINJA_ENV);
+/// Ok("/custom/ninja".to_string())
+/// });
+/// assert!(resolve_ninja_program(&env).ends_with("/custom/ninja"));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/runner.rs around lines 274 to 279, replace the doctest that uses unsafe
std::env::set_var with a pure MockEnv-based example: import and construct a
MockEnv (e.g. use mockable::MockEnv), set the NINJA_ENV variable on that mock
environment, pass the mock to resolve_ninja_program and assert the returned path
ends_with the mock value; also update the example imports to show MockEnv
instead of mutating the global environment.
| // SAFETY: Rust 2024 marks `set_var` as unsafe. This test injects a bogus | ||
| // value and restores `NINJA_ENV` afterwards to avoid leaking state. | ||
| unsafe { | ||
| std::env::set_var(NINJA_ENV, &ninja_path); | ||
| std::env::set_var(NINJA_ENV, "does-not-exist"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove unnecessary unsafe mutation of NINJA_ENV in test
The test injects MockEnv, so touching the real NINJA_ENV is unnecessary and adds flakiness. Delete the OS env mutation and restoration.
Apply this diff:
- let (temp_dir, ninja_path) = support::fake_ninja(0);
- let original = std::env::var_os(NINJA_ENV);
- // SAFETY: Rust 2024 marks `set_var` as unsafe. This test injects a bogus
- // value and restores `NINJA_ENV` afterwards to avoid leaking state.
- unsafe {
- std::env::set_var(NINJA_ENV, "does-not-exist");
- }
+ let (temp_dir, ninja_path) = support::fake_ninja(0);
@@
- let result = run(&cli, &env);
- assert!(result.is_ok());
-
- // SAFETY: restore original `NINJA_ENV` for other tests.
- unsafe {
- if let Some(val) = original {
- std::env::set_var(NINJA_ENV, val);
- } else {
- std::env::remove_var(NINJA_ENV);
- }
- }
+ let result = run(&cli, &env);
+ assert!(result.is_ok());Also applies to: 274-281
🤖 Prompt for AI Agents
In tests/runner_tests.rs around lines 241-246 and 274-281, remove the
unnecessary unsafe mutation and restoration of the real NINJA_ENV: delete the
unsafe std::env::set_var(NINJA_ENV, "does-not-exist"); and the corresponding
restore code and any temporary variable used only for saving/restoring the real
env value, since the test injects a MockEnv and touching the OS env is
unnecessary; also remove any now-empty unsafe block or trailing code that
referenced the removed variables so the test compiles cleanly.
|
Superseded. |
1 participant
Summary
Envimplementation inrunner::runandresolve_ninja_programNINJA_ENVvia the injected environmentMockEnvTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_689a356401788322ac8861510fbf227c
Summary by Sourcery
Inject an Env abstraction into the runner to manage environment variables, refactor runner and handle_build to accept Env, update main to supply DefaultEnv, add mockable dependency, and adjust tests to use DefaultEnv and MockEnv for controlling NINJA_ENV.
New Features:
Enhancements:
Build:
Tests: