Refactor runner process helpers into dedicated module#169
Conversation
Reviewer's GuideThis PR refactors Ninja process helpers by extracting temporary file creation, file writing, and program resolution logic into a dedicated module (src/runner/process.rs), updating the runner to delegate to these new utilities and removing redundant local implementations. Class diagram for refactored Ninja process helpersclassDiagram
class Runner {
+run(cli: Cli): Result
+handle_build(cli: Cli, args: BuildArgs): Result
-process::write_ninja_file(path: &Path, content: &NinjaContent): AnyResult
-process::create_temp_ninja_file(content: &NinjaContent): AnyResult
-process::resolve_ninja_program(): PathBuf
}
class Process {
+write_ninja_file(path: &Path, content: &NinjaContent): AnyResult
+create_temp_ninja_file(content: &NinjaContent): AnyResult
+resolve_ninja_program(): PathBuf
}
Runner --> Process : delegates
class NinjaContent {
+new(content: String)
+as_str(): &str
}
Process o-- NinjaContent : uses
Process o-- Path : uses
Process o-- NamedTempFile : returns
Flow diagram for delegation of Ninja helpers to process moduleflowchart TD
A[Runner module] -->|delegates| B[Process module]
B --> C[create_temp_ninja_file]
B --> D[write_ninja_file]
B --> E[resolve_ninja_program]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
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
WalkthroughMove Ninja file I/O, temporary-file helpers and Ninja-resolution logic from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant Runner as runner::run
participant Proc as runner::process
participant FS as Filesystem
participant Env as Environment
participant Ninja as Ninja
CLI->>Runner: invoke run(...)
Runner->>Proc: resolve_ninja_program()
Proc->>Env: read NINJA_ENV / NINJA_PROGRAM
Env-->>Proc: return program path (Utf8PathBuf)
Proc-->>Runner: return resolved program
Runner->>Proc: write_ninja_file(path, content) or create_temp_ninja_file(content)
Proc->>FS: create dirs / write file (UTF‑8 aware)
FS-->>Proc: ok / error
Proc-->>Runner: file handle / result
Runner->>Ninja: spawn with resolved program and ninja file
Ninja-->>Runner: exit status
Runner-->>CLI: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/runner/process.rs:48` </location>
<code_context>
pub fn redact_sensitive_args(args: &[CommandArg]) -> Vec<CommandArg> {
super::redact_sensitive_args(args)
}
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing trivial forwarding methods with direct re-exports to eliminate unnecessary boilerplate.
```suggestion
runner/doc.rs
// ─── 1. Replace trivial forwarding methods with `pub use` re-exports ────────────────────────────────
// At the top of the file (after your `use` block), add:
pub use super::{
is_sensitive_arg,
redact_argument,
redact_sensitive_args,
create_temp_ninja_file,
write_ninja_file,
resolve_ninja_program,
};
// ─── 2. Remove the boilerplate from `impl CommandArg` ───────────────────────────────────────────────
// Delete these methods entirely:
impl CommandArg {
// pub fn is_sensitive_arg(arg: &CommandArg) -> bool { super::is_sensitive_arg(arg) }
// pub fn redact_argument(arg: &CommandArg) -> CommandArg { super::redact_argument(arg) }
// pub fn redact_sensitive_args(args: &[CommandArg]) -> Vec<CommandArg> { super::redact_sensitive_args(args) }
// pub fn create_temp_ninja_file(content: &super::NinjaContent) -> super::AnyResult<super::NamedTempFile> {
// super::create_temp_ninja_file(content)
// }
// pub fn write_ninja_file(path: &super::Path, content: &super::NinjaContent) -> super::AnyResult<()> {
// super::write_ninja_file(path, content)
// }
}
```
This removes the pass-through methods on `CommandArg` (all of which just call the parent) and instead re-exports the real implementations at the module root. You keep the same public API and examples, but cut out the boilerplate wrappers.
</issue_to_address>
### Comment 2
<location> `src/runner/process.rs:132` </location>
<code_context>
args.iter().map(redact_argument).collect()
}
+/// Create a temporary Ninja file on disk containing `content`.
+///
+/// # Errors
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for create_temp_ninja_file and write_ninja_file functions.
The newly added functions create_temp_ninja_file and write_ninja_file lack corresponding behavioural and unit tests. Ensure that these functions are covered by appropriate tests to verify their correctness and error handling.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/runner/process.rs (2)
29-64: Stop exporting thedocmodule in release builds (public API leak).
pub mod docis compiled and exported in all builds;#[doc(hidden)]only hides it from docs. Gate it for doctests to avoid expanding the public API.Apply these diffs:
-#[doc(hidden)] -pub mod doc { +#[cfg(doctest)] +pub mod doc {And in
src/runner.rs, gate the re-export (see comment there). This keeps doctests compiling while avoiding a semver commitment to a testing-only surface. As per coding guidelines.
79-81: Remove duplicated doc line.The summary line is repeated twice.
Apply this diff:
-/// Determine whether the argument should be redacted. /// Determine whether the argument should be redacted.src/runner.rs (2)
21-24: Gate the re-export of the testingdocmodule.Do not export testing helpers in release builds. Align this with the change in
process.rs.Apply this diff:
-mod process; -#[doc(hidden)] -pub use process::doc; +mod process; +#[cfg(doctest)] +pub use process::doc; pub use process::run_ninja;This avoids unintended public API surface. As per coding guidelines.
115-148: Drop the explicitdrop(tmp_file);(nit).
tmp_filegoes out of scope immediately afterOk(())); RAII handles cleanup. Removing the explicitdropreduces noise.Apply this diff:
- drop(tmp_file); Ok(())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/runner.rs(4 hunks)src/runner/process.rs(3 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 / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/runner/process.rssrc/runner.rs
🪛 GitHub Check: build-test (ubuntu-latest, 1.89.0)
src/runner/process.rs
[warning] 5-5:
Diff in /home/runner/work/netsuke/netsuke/src/runner/process.rs
[warning] 1-1:
Diff in /home/runner/work/netsuke/netsuke/src/runner/process.rs
🪛 GitHub Check: build-test (ubuntu-latest, stable)
src/runner/process.rs
[warning] 5-5:
Diff in /home/runner/work/netsuke/netsuke/src/runner/process.rs
[warning] 1-1:
Diff in /home/runner/work/netsuke/netsuke/src/runner/process.rs
🪛 GitHub Actions: CI
src/runner/process.rs
[warning] 1-1: cargo fmt --check detected formatting diffs in netsuke/src/runner/process.rs (import order and formatting changes required).
🔍 Remote MCP
Additional Context for PR Review
1. Existing Module Size and Line-Count Guideline
- The
Developer Guideenforces a hard limit of 400 lines per source file. Files that exceed this should be split by extracting helper functions or moving large data/execution logic into separate modules Deepwiki_ask_question. - Prior to this PR,
src/runner.rscontained 434 lines (exceeding the 400-line rule) as noted in linked issue #155.
2. New process Module’s Responsibilities
runner/process.rsnow encapsulates:create_temp_ninja_file(&NinjaContent) -> AnyResult<NamedTempFile>write_ninja_file(&Path, &NinjaContent) -> AnyResult<()>resolve_ninja_program() -> PathBuf
- This aligns with the “Extract Helper Functions” strategy by moving process-related plumbing out of
runner.rs.
3. Ninja Environment Variables
- The resolver function uses:
NINJA_ENV(from theninja_envcrate) first, then falls back toNINJA_PROGRAMdefault.
- Confirm these constants exist and behave as expected:
ninja_env::NINJA_ENVis a&'static strpointing at the user‐override var name.NINJA_PROGRAMdefaults to e.g."ninja"for fallback.
4. Tests and Documentation
- Doctest wrappers in
src/runner/process.rs::docexpose the new helpers without making them public in release builds. - Ensure existing tests cover:
- Temporary file creation and cleanup paths.
- Writing content matches
NinjaContentformatting expectations. - Program resolution respects environment override.
5. Linked Issue Compliance
- Issue #155 explicitly called for splitting by process concerns and reducing
runner.rsto ≤400 lines. This PR satisfies that requirement by off-loading file and process helpers Deepwiki_ask_question.
⏰ 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: Sourcery review
🔇 Additional comments (6)
src/runner/process.rs (4)
169-178: LGTM: create parent dirs and log write path.Directory creation is guarded; error contexts are precise; logging is informative.
180-184: LGTM: environment fallback for Ninja programme.Resolve via
NINJA_ENVwith sane default; mark as#[must_use].
200-266: Stream child stdout/stderr safely; keep redaction in logs.Threaded line streaming and argument redaction look solid;
#[expect(..)]is narrowly scoped with reason.Run the child-process path once with
NINJA_ENVunset and once set to a custom stub in CI to validate redaction and argument formation end‑to‑end.
1-13: Run rustfmt locally
Run rustfmt in your local environment and commit the formatted imports to unblock CI.cargo fmt --all cargo fmt --all -- --checksrc/runner.rs (2)
86-89: LGTM: delegate manifest emission toprocess::write_ninja_file.Responsibility split is clear; error contexts remain intact.
138-146: LGTM: resolve Ninja programme via helper and attach context.Programme resolution, invocation, and contextualised error message are tidy.
Replace the doc module's forwarding helpers with direct re-exports and make the underlying functions visible for doctests. Add unit and behavioural tests that exercise the Ninja file helpers directly to cover the new process utilities.
Detail that process orchestration now lives in src/runner/process.rs so the design guide matches the current module layout.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runner/process.rs (1)
181-189: Null stdin for spawned process.Prevent accidental reads from the parent’s stdin by the child.
- let mut cmd = Command::new(program); + let mut cmd = Command::new(program); + cmd.stdin(Stdio::null());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/netsuke-design.md(1 hunks)src/runner/process.rs(7 hunks)tests/runner_tests.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the docs/ markdown files as the reference knowledge base and update them proactively when decisions or requirements change
docs/**/*.md: In Markdown docs that contain Rust examples, mark fenced code blocks with the rust language specifier (```rust)
Use assert!, assert_eq!, or assert_ne! in Markdown doctests to verify outcomes
Avoid using .unwrap() or .expect() in Markdown doctest examples; handle errors idiomatically
For fallible Markdown doctests using ?, use an explicit fn main() -> Result<...> and hide boilerplate with #
In Markdown doctests, you may end code with exact (()) (no spaces) to enable implicit Result-returning main
Use hidden lines (prefix #) in Markdown doctests to hide setup and boilerplate
In Markdown doctests with side effects, use no_run to avoid executing while still compiling
Use should_panic in Markdown doctests for panic-intended examples
Use compile_fail in Markdown doctests to illustrate invalid code; beware brittleness
Only use ignore in Markdown doctests for pseudocode/non-Rust/temporary skips; avoid otherwise
Use edition20xx in Markdown doctests when examples require a specific Rust edition
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar (LICENSE name excluded)
Validate Markdown with make markdownlint and run make fmt to format Markdown (including fixing table markup)
Validate Mermaid diagrams in Markdown by running make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Do not wrap tables and headings in Markdown
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
src/runner/process.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 / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/runner/process.rstests/runner_tests.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Large blocks of test data should be moved to external data files
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Mock non-deterministic dependencies (Env, Clock) via dependency injection using mockable crate
Test documentation should omit examples that merely reiterate test logic
Files:
tests/runner_tests.rs
🧬 Code graph analysis (2)
src/runner/process.rs (1)
src/runner.rs (3)
new(30-32)new(50-52)as_str(34-36)
tests/runner_tests.rs (2)
src/runner.rs (4)
run(78-99)new(30-32)new(50-52)as_str(34-36)src/runner/process.rs (4)
new(19-21)create_temp_ninja_file(120-128)as_str(23-25)write_ninja_file(144-153)
🔍 Remote MCP Deepwiki
Summary of additional relevant facts for reviewing PR #169
-
Developer Guide enforces a hard 400-line-per-file rule and explicitly recommends extracting helpers into modules (e.g., move long functions/helpers or doctest helpers into separate modules or cfg(doctest) helpers). This directly motivates extracting process helpers from src/runner.rs.
-
The new process module implements three concrete helpers used by the runner:
- create_temp_ninja_file(&NinjaContent) -> AnyResult
- write_ninja_file(&Path, &NinjaContent) -> AnyResult<()>
- resolve_ninja_program() -> PathBuf
These meet the stated objectives of moving Ninja file/program plumbing out of runner.rs (matches PR description and ai_summary). [PR context]
-
resolve_ninja_program behavior: it prefers an environment override (NINJA_ENV) and falls back to a default NINJA_PROGRAM value. Ensure tests or review verify precedence and fallback behavior (env var name and default). [PR context]
-
Tests and doctest exposure:
- The process module exposes doctest wrappers to surface the new helpers in documentation tests without changing public API surface.
- New tests were added to verify create_temp_ninja_file and write_ninja_file behaviors (persistence, directory creation). Confirm test coverage addresses cleanup and error paths. [PR context]
Cited sources
- Developer Guide enforcement and recommendations:
⏰ 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: Sourcery review
🔇 Additional comments (3)
docs/netsuke-design.md (1)
1601-1607: Docs updated to reflect process module delegation.The wording accurately reflects the new separation of concerns between
runnerandrunner::process. LGTM.src/runner/process.rs (2)
1-1: Add a module-level//!doc comment.Begin the module with a brief overview per repository guidelines.
+//! Process helpers for Ninja file lifecycle, argument redaction, and subprocess I/O. +//! Internal to `runner`; public API is defined in `runner.rs`. use super::{BuildTargets, NINJA_PROGRAM, NinjaContent};
121-127: Do not rewrite a still-openNamedTempFileby path (Windows sharing violation).Write via the existing handle instead of reopening by path.
-pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult<NamedTempFile> { - let tmp = Builder::new() +pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult<NamedTempFile> { + let mut tmp = Builder::new() .prefix("netsuke.") .suffix(".ninja") .tempfile() .context("create temp file")?; - write_ninja_file(tmp.path(), content)?; + tmp.as_file_mut() + .write_all(content.as_str().as_bytes()) + .context("write temp Ninja file")?; + info!("Generated temporary Ninja file at {}", tmp.path().display()); Ok(tmp) }
Tighten the redaction utilities to only match key=value secrets, add capability-based Ninja file writers, and cover the behaviour with focused tests. Update the runner design doc wrapping changed by fmt.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/runner/process.rs (1)
247-312: Keep FS operations capability‑based inrun_ninja.
fs::canonicalizeuses ambient FS. Canonicalize viacap_stdwhere feasible to align with the project’s FS model, and prefer camino types for paths passed to logs.
- Replace
fs::canonicalize(dir)?withcap_fs::Dir::open_ambient_dir(dir, ambient_authority())?.canonicalize(".")?or equivalent.- Replace
build_file.canonicalize()analogously or pre‑canonicalize when emitting the file.Confirm cap‑std API availability in your MSRV before changing.
src/runner.rs (1)
165-175: Unify path types withcaminofor cohesion.Adopt
Utf8PathBuffor manifest resolution and propagate through call sites to align with the crate‑wide path policy.
- Change
resolve_manifest_pathto returnUtf8PathBuf.- Convert at CLI boundary and only fall back to
PathBufat OS interfaces.Also applies to: 187-191
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/netsuke-design.md(1 hunks)src/runner.rs(4 hunks)src/runner/process.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
src/runner/process.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 / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/runner/process.rssrc/runner.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the docs/ markdown files as the reference knowledge base and update them proactively when decisions or requirements change
docs/**/*.md: In Markdown docs that contain Rust examples, mark fenced code blocks with the rust language specifier (```rust)
Use assert!, assert_eq!, or assert_ne! in Markdown doctests to verify outcomes
Avoid using .unwrap() or .expect() in Markdown doctest examples; handle errors idiomatically
For fallible Markdown doctests using ?, use an explicit fn main() -> Result<...> and hide boilerplate with #
In Markdown doctests, you may end code with exact (()) (no spaces) to enable implicit Result-returning main
Use hidden lines (prefix #) in Markdown doctests to hide setup and boilerplate
In Markdown doctests with side effects, use no_run to avoid executing while still compiling
Use should_panic in Markdown doctests for panic-intended examples
Use compile_fail in Markdown doctests to illustrate invalid code; beware brittleness
Only use ignore in Markdown doctests for pseudocode/non-Rust/temporary skips; avoid otherwise
Use edition20xx in Markdown doctests when examples require a specific Rust edition
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar (LICENSE name excluded)
Validate Markdown with make markdownlint and run make fmt to format Markdown (including fixing table markup)
Validate Mermaid diagrams in Markdown by running make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Do not wrap tables and headings in Markdown
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
🧬 Code graph analysis (2)
src/runner/process.rs (1)
src/runner.rs (3)
new(29-31)new(49-51)as_str(33-35)
src/runner.rs (1)
src/runner/process.rs (3)
write_ninja_file(207-214)create_temp_ninja_file(140-148)resolve_ninja_program(228-230)
🔍 Remote MCP
Summary of additional facts relevant to reviewing PR #169 (Refactor runner process helpers)
-
What changed (concise)
- Ninja file/program handling moved from src/runner.rs into a new src/runner/process.rs. The runner now calls:
- process::create_temp_ninja_file(&NinjaContent) -> AnyResult
- process::write_ninja_file(&Path, &NinjaContent) -> AnyResult<()>
- process::resolve_ninja_program() -> PathBuf
- src/runner.rs removed local implementations and reduced imports; it re-exports process::doc under #[cfg(doctest)].
- docs/netsuke-design.md updated to state process-related helpers live in src/runner/process.rs.
- Ninja file/program handling moved from src/runner.rs into a new src/runner/process.rs. The runner now calls:
-
Behavior details to check in review
- resolve_ninja_program reads environment overrides (NINJA_ENV) and falls back to a default NINJA_PROGRAM; verify correct precedence, variable names, and fallback path semantics.
- create_temp_ninja_file and write_ninja_file now create/persist files and create parent directories as needed — verify error contexts, logging, and cleanup semantics (temp file lifetime).
- Doctest and visibility: helpers are exposed via a doctest-facing pub use; confirm this does not unintentionally widen public API surface beyond intended cfg(doctest) usage.
- Tests: new tests cover persistence and directory creation for the two file helpers — confirm they also exercise error paths and resolve_ninja_program behavior.
-
PR objectives alignment
- Addresses issue #155 (reduce runner.rs length and extract process plumbing).
- Keeps high-level run/CLI logic in runner.rs; moves low-level process plumbing to the new module as intended.
Sources
- PR context and file summaries provided in the PR payload (src/runner.rs, src/runner/process.rs, docs/netsuke-design.md).
⏰ 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: Sourcery review
🔇 Additional comments (6)
src/runner/process.rs (4)
43-56: Good: sensitive‑key set is explicit and case‑insensitive.The dedicated predicate tightens redaction and avoids path false‑positives. LGTM.
66-71: Good: only flagkey=valuepairs with sensitive keys.Using
split_once('=')plusis_some_andis clear and efficient. LGTM.
207-214: Public wrapper aligns with UTF‑8 + cap‑std inner API.The wrapper preserves legacy callers while funnelling writes through the capability‑based implementation and logging once. LGTM.
140-148: Fix Windows share violation: write temp Ninja via the open handle, not by path.Rewriting a still-open
NamedTempFileby path can fail on Windows (share violation). Write throughtmp.as_file_mut()instead.Apply:
-pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult<NamedTempFile> { - let tmp = Builder::new() +pub fn create_temp_ninja_file(content: &NinjaContent) -> AnyResult<NamedTempFile> { + let mut tmp = Builder::new() .prefix("netsuke.") .suffix(".ninja") .tempfile() .context("create temp file")?; - write_ninja_file(tmp.path(), content)?; + tmp.as_file_mut() + .write_all(content.as_str().as_bytes()) + .context("write temp Ninja file")?; + // Optional but prudent for immediate consumer processes: + tmp.as_file_mut().flush().ok(); Ok(tmp) }As per coding guidelines (cross‑platform correctness).
src/runner.rs (2)
21-24: Doctest surface export is correctly scoped.Re‑export under
#[cfg(doctest)]prevents accidental API exposure while enabling doctest imports. LGTM.
86-87: Delegation toprocessreduces coupling and meets the ≤400‑line goal.
write_ninja_file, temp‑file lifecycle, and program resolution are cleanly delegated. The_tmp_file_guardensures temp persistence during the run. LGTM.Run
cargo clippyto ensure_tmp_file_guardis not flagged; if it is, gate with#[allow(unused_variables)]locally or rewrite to_guard.Also applies to: 122-131, 134-134
Flush Ninja file writes, probe ancestors via capabilities, and cover the runner documentation surface with targeted tests. Document the NINJA_ENV override to resolve review feedback.
Resolve build and working directory paths using cap-std handles while preserving the prior fallback for missing build files. Rework run_ninja logging to favour UTF-8 paths and add a helper that returns canonicalised UTF-8 paths. Update the manifest resolution to return Utf8PathBuf so generation uses UTF-8-safe paths.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Methodsrc/runner/process.rs: canonicalize_utf8_path What lead to degradation?canonicalize_utf8_path has a cyclomatic complexity of 9, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Rust language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. ing complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE # complex_method.js
function postItem(item) {
- if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+} |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Large Methodsrc/runner/process.rs: run_ninja What lead to degradation?run_ninja has 78 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
|
Tip For best results, initiate chat on the files or code changes. A summary of the changes CodeRabbit can apply:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Here are the copyable edits based on the planned changes: Copyable Editssrc/runner/process.rs@@ -243,6 +243,20 @@
resolve_ninja_program_utf8().into()
}
+/// Invoke the Ninja executable with the provided CLI settings.
+///
+/// The function forwards the job count and working directory to Ninja,
+/// specifies the temporary build file, and streams its standard output and
+/// error back to the user.
+///
+/// # Errors
+///
+/// Returns an [`io::Error`] if the Ninja process fails to spawn or reports a
+/// non-zero exit status.
+///
+/// # Panics
+///
+/// Panics if the child's output streams cannot be captured.
/// Invoke the Ninja executable with the provided CLI settings.
///
/// The function forwards the job count and working directory to Ninja,
@@ -263,6 +277,18 @@
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
+ let mut cmd = configure_ninja_command(program, cli, build_file, targets)?;
+ log_command_execution(&cmd);
+ let status = spawn_and_stream_output(&mut cmd)?;
+ check_exit_status(status)
+}
+
+fn configure_ninja_command(
+ program: &Path,
+ cli: &Cli,
+ build_file: &Path,
+ targets: &BuildTargets<'_>,
+) -> io::Result<Command> {
let mut cmd = Command::new(program);
if let Some(dir) = &cli.directory {
let canonical = canonicalize_utf8_path(dir.as_path())?;
@@ -287,6 +313,10 @@
cmd.stdout(Stdio::piped());
cmd.stderr(Stdio::piped());
+ Ok(cmd)
+}
+
+fn log_command_execution(cmd: &Command) {
let program_path = PathBuf::from(cmd.get_program());
let program_display = Utf8PathBuf::from_path_buf(program_path.clone()).map_or_else(
|_| program_path.to_string_lossy().into_owned(),
@@ -303,7 +333,9 @@
program_display,
arg_strings.join(" ")
);
+}
+fn spawn_and_stream_output(cmd: &mut Command) -> io::Result<std::process::ExitStatus> {
let mut child = cmd.spawn()?;
let stdout = child.stdout.take().expect("child stdout");
let stderr = child.stderr.take().expect("child stderr");
@@ -327,6 +359,10 @@
let _ = out_handle.join();
let _ = err_handle.join();
+ Ok(status)
+}
+
+fn check_exit_status(status: std::process::ExitStatus) -> io::Result<()> {
if status.success() {
Ok(())
} else {
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runner/process.rs (1)
260-342: Decomposerun_ninjainto focused helper functions.The function spans 78 lines (threshold: 70) and handles multiple responsibilities: command configuration, logging, process spawning/streaming, and exit status checking. Extract four private helpers to satisfy the line limit and single-responsibility principle:
configure_ninja_command(cmd: &mut Command, cli: &Cli, build_file: &Path, targets: &BuildTargets) -> io::Result<()>log_command_execution(cmd: &Command)spawn_and_stream_output(mut child: Child) -> io::Result<()>check_exit_status(status: ExitStatus) -> io::Result<()>Then reduce
run_ninjato a 6-line delegating implementation. Preserve all behaviour, logging, error messages, and Clippy annotations in the extracted helpers.🤖 Prompt for AI Agents
In src/runner/process.rs at lines 260-342, the run_ninja function exceeds the 70-line threshold and handles multiple responsibilities; refactor it by extracting four private helper functions: configure_ninja_command (lines 266-286: directory, jobs, build file args), log_command_execution (lines 290-305: program display, arg redaction, info! logging), spawn_and_stream_output (lines 307-328: spawn, stream stdout/stderr in threads, wait), and check_exit_status (lines 330-341: success check and error construction with clippy expect); place these helpers immediately after run_ninja in the file, ensure run_ninja calls them in sequence and returns their results, preserve all error contexts and Clippy annotations in the helpers, and verify with make fmt and make test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runner/process.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
src/runner/process.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 / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/runner/process.rs
🧬 Code graph analysis (1)
src/runner/process.rs (1)
src/runner.rs (4)
new(30-32)new(50-52)into_string(38-40)as_str(34-36)
🪛 GitHub Actions: CI
src/runner/process.rs
[error] 373-373: Clippy error: this function's return value is unnecessarily wrapped by Result. Command failed: cargo clippy --all-targets --all-features -- -D warnings
🪛 GitHub Check: build-test (ubuntu-latest, 1.89.0)
src/runner/process.rs
[failure] 373-373:
this function's return value is unnecessarily wrapped by Result
🪛 GitHub Check: build-test (ubuntu-latest, stable)
src/runner/process.rs
[failure] 373-373:
this function's return value is unnecessarily wrapped by Result
🔍 Remote MCP
Additional Context for Review
-
tempfile::NamedTempFile semantics
TheNamedTempFiletype creates a file in the OS temporary directory and relies on its Rust destructor to delete the file when dropped. If the destructor does not run (e.g., process exit via signal or panic, or static instances), the temporary file may persist on disk. On Unix-like systems, external “temporary file cleaners” (e.g., systemd’s tmpfiles) can unlink named temp files while still open, leading to potential path reuse by other processes. Applications can disable cleanup or choose in-memory temporary files for greater safety, but must handle errors if cleanup fails. (docs.rs) -
camino::Utf8PathBuf and canonicalize_utf8
TheUtf8PathBuftype guarantees that paths contain valid UTF-8, simplifying code that converts paths to strings. Itscanonicalize_utf8()method performs an absolute, symlink-resolving canonicalization and returns aUtf8PathBuf, erroring if any component is not valid UTF-8. This differs from the standardstd::fs::canonicalize, which returns aPathBufand may yield non-UTF-8 data. (docs.rs) -
cap-std::fs::Dir canonicalize behavior
TheDirtype in thecap-std::fscrate provides filesystem operations relative to a capability-based directory handle. Itscanonicalize()method resolves intermediate components and symlinks but returns a path relative to the directory, not an absolute filesystem path—unlikestd::fs::canonicalize. This ensures compliance with the capability model by avoiding global namespace assumptions. (docs.rs)
⏰ 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: Sourcery review
🔇 Additional comments (12)
src/runner/process.rs (12)
1-2: Module documentation is clear and follows guidelines.The
//!comment properly explains the module's purpose and scope.
3-18: Imports properly align with project guidelines.Cap-std for capability-based FS, camino for UTF-8 paths, and anyhow for error handling are correctly employed.
20-31: CommandArg newtype wrapper is correctly implemented.The struct provides clear type safety for command arguments with appropriate accessor methods.
35-42: Doctest exposure module correctly re-exports helpers.The
pub useapproach cleanly exposes internal functions for doctests without boilerplate forwarding methods.
44-72: Sensitive keyword detection correctly scoped to key=value pairs.The implementation properly restricts redaction to recognised sensitive keys on the left of '=', avoiding false positives on paths or positional arguments.
74-126: Redaction functions correctly preserve keys whilst masking sensitive values.The implementation properly handles key=value pairs, edge cases, and provides clear documentation with examples.
156-175: UTF-8 file writing function properly implements durability and capability-based FS.The function correctly uses cap-std, flushes and syncs data before returning, and provides clear error context at each step.
177-198: Ancestor discovery correctly uses capability-based filesystem probes.The function properly avoids ambient
Path::exists()and instead attempts to open ancestors via cap-std, maintaining the capability model throughout.
214-221: Legacy wrapper correctly delegates to UTF-8 and cap-std implementation.The function provides backward compatibility whilst migrating to the preferred UTF-8 path handling.
223-244: Ninja program resolution correctly implements dependency injection for testability.The internal helper accepts an env reader closure, enabling safe testing without mutating the environment (per coding guidelines). The public API provides both UTF-8 and legacy PathBuf variants.
344-405: Path canonicalization correctly decomposed into focused helper functions.The dispatcher delegates to four specialised helpers (current dir, root, relative, absolute paths), each handling a single scenario. The conversion helper provides clear error messages. This design satisfies the complexity guideline and improves maintainability.
407-519: Test coverage is comprehensive and uses proper techniques for cross-platform compatibility.Tests correctly use dependency injection for env resolution (avoiding unsafe mutations per guidelines) and employ
.reopen()to read temp files (avoiding Windows share violations). The test suite will continue to work oncecreate_temp_ninja_fileis fixed to write via the open handle.
Update canonicalize_root_path to return a bare Utf8PathBuf and wrap its caller so clippy no longer flags an unnecessary Result.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runner/process.rs (1)
1-540: Decompose this file to comply with the 400-line limit.The file contains 540 lines, exceeding the 400-line maximum by 140 lines. Per coding guidelines, large modules must be decomposed.
Split this module:
runner/process/redaction.rs— argument redaction helpers (lines 44-126, ~83 lines)runner/process/file_io.rs— Ninja file creation/writing (lines 128-228, ~101 lines)runner/process/paths.rs— path resolution/canonicalization (lines 230-251, 365-426, ~93 lines)runner/process.rs— command building/execution, re-export sub-modules (lines 253-363, ~111 lines + re-exports)Update
runner/process.rsto re-export public APIs:pub use redaction::*; pub use file_io::*; pub use paths::*;Move tests into respective sub-module files.
In src/runner/process.rs, the file exceeds 400 lines (currently 540); decompose it into four sub-modules under runner/process/: redaction.rs for argument redaction helpers (lines 44-126), file_io.rs for Ninja file creation and writing (lines 128-228), paths.rs for path resolution and canonicalization (lines 230-251 and 365-426), and keep command building/execution in process.rs (lines 253-363); move each segment's tests into the corresponding sub-module file; update process.rs to re-export public APIs from the sub-modules using pub use; ensure each sub-module begins with a module-level //! doc comment; run make fmt and make test to verify no regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runner/process.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Keep functions small with single responsibility; apply command-query separation
Function documentation must include clear examples; test documentation should omit redundant examples
No single Rust code file may exceed 400 lines; split long switches/dispatch tables; move large test data to external files
Disallow Clippy warnings; code must be clean under clippy
Fix warnings emitted during tests in code, do not silence them
Extract helper functions when a function is too long; maintain separation of concerns and CQRS
Group many related parameters into well-named structs
Consider using Arc for large error values to reduce data returned
Every module must begin with a module-level //! comment describing purpose and utility
Document public APIs with Rustdoc /// comments so cargo doc can generate docs
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary; document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditionals with more than two branches
Do not silence lints except as a last resort
When suppressing lints, scope narrowly and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint handling
When functions are unused for certain features, gate with #[cfg]/#[cfg_attr]
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use cap-std for filesystem operations; avoid std::fs directly
Use camino for path handling; avoid std::path::PathBuf directly
Prefer semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Use opaque eyre::Report only at the app boundary; do not expose in public APIs
Never export opaque error types from libraries; convert to domain...
Files:
src/runner/process.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 / -yse / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in...
Files:
src/runner/process.rs
🧬 Code graph analysis (1)
src/runner/process.rs (1)
src/runner.rs (4)
new(30-32)new(50-52)into_string(38-40)as_str(34-36)
⏰ 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). (2)
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: Sourcery review
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1 participant
Summary
src/runner/process.rsTesting
https://chatgpt.com/codex/tasks/task_e_68d96f618bf083229c0679f8403b391f
Summary by Sourcery
Extract and consolidate Ninja file and program management into a dedicated process module and update the runner to delegate to these new helpers.
Enhancements:
Documentation: