Conversation
Reviewer's GuideIntroduce Ninja subprocess invocation by refactoring the runner to return and propagate io::Result, implementing a run_ninja helper to spawn Ninja with proper flags and stream its output, updating main to handle errors via ExitCode, and augmenting tests and documentation to cover the new logic. Sequence diagram for invoking Ninja as a subprocess from CLIsequenceDiagram
actor User
participant CLI as Cli::parse_with_default
participant Runner as runner::run
participant Ninja as Ninja subprocess
User->>CLI: Run application
CLI->>Runner: runner::run(&cli)
Runner->>Ninja: Spawn Ninja process (with args)
Ninja-->>Runner: Stream stdout/stderr
Runner-->>CLI: Return Ok or io::Error
CLI-->>User: Output Ninja's messages or error
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (16)
Summary by CodeRabbit
WalkthroughUpdate the codebase to invoke the Ninja build system as a subprocess, forwarding relevant CLI arguments and streaming output. Refactor logic to use stable Rust syntax, adjust error handling, and propagate errors appropriately. Expand test coverage with new feature and unit tests, and implement Cucumber steps for process execution scenarios. Update documentation and roadmap accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Runner
participant Ninja (Subprocess)
User->>CLI: Invoke CLI command
CLI->>Runner: Parse arguments, call run(&Cli)
Runner->>Ninja (Subprocess): Spawn Ninja with args (-j, -C, targets)
Ninja (Subprocess)-->>Runner: Stream stdout/stderr
Runner-->>CLI: Return Ok or error
CLI-->>User: Output Ninja messages or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
- run_ninja currently calls Command::output() which buffers the entire output; use Command::spawn with piped stdout/stderr and forwarding lines as they arrive to provide real-time streaming.
- The tests modify PATH globally via std::env::set_var, which can lead to interference in parallel runs; use Command::env or a scoped environment helper to isolate PATH changes per test.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- run_ninja currently calls Command::output() which buffers the entire output; if you want real-time streaming, consider using Command::spawn with piped stdout/stderr and forwarding lines as they arrive.
- The tests modify PATH globally via std::env::set_var, which can lead to interference in parallel runs; consider using Command::env or a scoped environment helper to isolate PATH changes per test.
## Individual Comments
### Comment 1
<location> `src/runner.rs:54` </location>
<code_context>
+ }
+ cmd.args(targets);
+
+ let output = cmd.output()?;
+ io::stdout().write_all(&output.stdout)?;
+ io::stderr().write_all(&output.stderr)?;
+ if output.status.success() {
</code_context>
<issue_to_address>
Using Command::output buffers all output before writing, which may delay feedback for long-running builds.
Consider using Command::spawn to stream output directly, providing real-time feedback during long-running builds.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let output = cmd.output()?;
io::stdout().write_all(&output.stdout)?;
io::stderr().write_all(&output.stderr)?;
if output.status.success() {
Ok(())
} else {
Err(io::Error::other(format!(
"ninja exited with {}",
output.status
)))
}
=======
use std::io::{BufRead, BufReader};
use std::thread;
cmd.stdout(std::process::Stdio::piped());
cmd.stderr(std::process::Stdio::piped());
let mut child = cmd.spawn()?;
let stdout = child.stdout.take().expect("Failed to capture stdout");
let stderr = child.stderr.take().expect("Failed to capture stderr");
let stdout_handle = thread::spawn(move || {
let reader = BufReader::new(stdout);
let mut handle = io::stdout();
for line in reader.lines() {
if let Ok(line) = line {
let _ = writeln!(handle, "{}", line);
}
}
});
let stderr_handle = thread::spawn(move || {
let reader = BufReader::new(stderr);
let mut handle = io::stderr();
for line in reader.lines() {
if let Ok(line) = line {
let _ = writeln!(handle, "{}", line);
}
}
});
let status = child.wait()?;
let _ = stdout_handle.join();
let _ = stderr_handle.join();
if status.success() {
Ok(())
} else {
Err(io::Error::other(format!(
"ninja exited with {}",
status
)))
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/runner.rs:60` </location>
<code_context>
+ if output.status.success() {
+ Ok(())
+ } else {
+ Err(io::Error::other(format!(
+ "ninja exited with {}",
+ output.status
+ )))
+ }
</code_context>
<issue_to_address>
io::Error::other is unstable and not available on stable Rust.
Use io::Error::new(io::ErrorKind::Other, ...) for stable Rust compatibility.
</issue_to_address>
### Comment 3
<location> `tests/steps/process_steps.rs:69` </location>
<code_context>
+ assert_eq!(world.run_status, Some(true));
+}
+
+#[then("the command should fail")]
+fn command_should_fail(world: &mut CliWorld) {
+ assert_eq!(world.run_status, Some(false));
+}
</code_context>
<issue_to_address>
Consider asserting on the error message for failed commands.
Asserting on the error message will help verify that failures are handled as expected and provide clearer feedback.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
#[then("the command should fail")]
fn command_should_fail(world: &mut CliWorld) {
assert_eq!(world.run_status, Some(false));
}
=======
#[then("the command should fail")]
fn command_should_fail(world: &mut CliWorld) {
assert_eq!(world.run_status, Some(false));
assert!(
world.run_error.is_some(),
"Expected an error message, but none was found"
);
}
/// Asserts that the command failed and the error message matches the expected value.
#[then(expr = "the command should fail with error {string}")]
fn command_should_fail_with_error(world: &mut CliWorld, expected_error: String) {
assert_eq!(world.run_status, Some(false));
let actual_error = world
.run_error
.as_ref()
.expect("Expected an error message, but none was found");
assert!(
actual_error.contains(&expected_error),
"Expected error message to contain '{}', but got '{}'",
expected_error,
actual_error
);
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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
♻️ Duplicate comments (2)
src/runner.rs (2)
60-64: Replace unstable API with stable alternative.
io::Error::otheris unstable and not available on stable Rust. Use the stable alternative.Apply this diff:
- Err(io::Error::other(format!( - "ninja exited with {}", - output.status - ))) + Err(io::Error::new( + io::ErrorKind::Other, + format!("ninja exited with {}", output.status) + ))
54-64: Implement streaming output for better user experience.Using
Command::output()buffers all output before writing, which delays feedback during long-running builds. Stream output directly for real-time feedback.This impacts user experience significantly during long builds where immediate feedback is crucial.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/ir.rs(3 hunks)src/main.rs(1 hunks)src/runner.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/ninja_process.feature(1 hunks)tests/runner_tests.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/roadmap.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/roadmap.mddocs/netsuke-design.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/steps/mod.rssrc/ir.rssrc/main.rstests/runner_tests.rssrc/runner.rstests/cucumber.rstests/steps/process_steps.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.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/mod.rssrc/ir.rssrc/main.rstests/runner_tests.rssrc/runner.rstests/cucumber.rstests/steps/process_steps.rs
🧬 Code Graph Analysis (1)
tests/runner_tests.rs (1)
src/runner.rs (1)
run_ninja(44-65)
🪛 GitHub Check: build-test
src/ir.rs
[failure] 325-325:
this if statement can be collapsed
[failure] 343-343:
this if statement can be collapsed
[failure] 356-356:
this if statement can be collapsed
🪛 GitHub Actions: CI
src/ir.rs
[error] 325-325: Clippy lint error: this if statement can be collapsed.
🔇 Additional comments (18)
tests/steps/mod.rs (1)
5-5: LGTM!The addition of the
process_stepsmodule correctly integrates the new Cucumber step definitions for testing ninja subprocess execution scenarios.docs/roadmap.md (1)
64-65: LGTM!Correctly marks the process management logic implementation as complete, reflecting the successful integration of ninja subprocess invocation functionality.
src/main.rs (1)
1-17: LGTM!Excellent implementation of the application entry point with proper module documentation, error handling, and exit code management. The change from unit return to
ExitCodewith appropriate error handling follows Rust best practices and meets the coding guidelines requirements.docs/netsuke-design.md (1)
1158-1162: LGTM!Excellent documentation addition that clearly describes the ninja subprocess invocation implementation approach. The technical details about flag forwarding, output handling, and error reporting provide valuable context and align perfectly with the implemented functionality.
tests/features/ninja_process.feature (1)
1-20: Excellent BDD test coverage for Ninja subprocess scenarios.The feature file provides comprehensive coverage of the key scenarios: success, failure, and missing executable cases. The Given/When/Then structure is clear and follows BDD best practices.
tests/runner_tests.rs (5)
1-1: Perfect module documentation compliance.The module-level doc comment clearly explains the module's purpose, adhering to the coding guidelines requirement.
11-20: Clean test helper function.The
test_cli()function provides a good default configuration for testing. The structure is clear and appropriate.
22-46: Well-designed test fixture with proper Unix handling.The
FakeNinjastruct provides an excellent abstraction for creating temporary executables. The Unix-specific permission handling withcfg(unix)is appropriate and follows best practices.
48-56: Excellent use of parameterised testing.The rstest parameterisation cleanly tests both success and failure scenarios in a single test function. This follows the coding guidelines recommendation to use rstest for avoiding test duplication.
58-64: Proper error kind verification.The test correctly verifies that attempting to run a non-existent executable returns
NotFounderror kind. Good use ofexpect_err()for clarity.tests/cucumber.rs (1)
11-14: Good extension of test world state.The new fields appropriately support the Ninja process testing scenarios. The field names are clear and purposeful.
src/runner.rs (4)
1-6: Excellent module documentation.The module documentation clearly explains the purpose and mentions the key functionality of delegating to Ninja subprocess with output streaming.
18-33: Good refactoring to return Result type.The function signature change to borrow
Cliand returnio::Result<()>improves error propagation. The command dispatch logic is clean and appropriate.
35-43: Clear function documentation.The documentation clearly explains the function's purpose, parameters, and error conditions following Rustdoc conventions.
44-53: Correct command construction logic.The argument building logic properly handles optional directory and jobs parameters, forwarding them correctly to Ninja.
tests/steps/process_steps.rs (3)
1-1: Proper module documentation.The module-level doc comment clearly explains the module's purpose for step definitions.
49-62: Solid process execution step implementation.The step correctly captures both success and error states from the runner, providing good integration between the Cucumber tests and the main logic.
64-72: Basic assertion steps work correctly.The success and failure assertion steps provide the necessary verification for the test scenarios.
| #[given("no ninja executable is available")] | ||
| fn no_ninja(world: &mut CliWorld) { | ||
| let dir = tempfile::tempdir().expect("temp dir"); | ||
| if world.original_path.is_none() { | ||
| world.original_path = Some(std::env::var_os("PATH").unwrap_or_default()); | ||
| } | ||
| unsafe { | ||
| std::env::set_var("PATH", dir.path()); | ||
| } | ||
| world.temp = Some(dir); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary unsafe block.
The unsafe block is incorrect as std::env::set_var is not an unsafe function.
Apply this diff:
- unsafe {
- std::env::set_var("PATH", dir.path());
- }
+ std::env::set_var("PATH", dir.path());📝 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.
| #[given("no ninja executable is available")] | |
| fn no_ninja(world: &mut CliWorld) { | |
| let dir = tempfile::tempdir().expect("temp dir"); | |
| if world.original_path.is_none() { | |
| world.original_path = Some(std::env::var_os("PATH").unwrap_or_default()); | |
| } | |
| unsafe { | |
| std::env::set_var("PATH", dir.path()); | |
| } | |
| world.temp = Some(dir); | |
| } | |
| #[given("no ninja executable is available")] | |
| fn no_ninja(world: &mut CliWorld) { | |
| let dir = tempfile::tempdir().expect("temp dir"); | |
| if world.original_path.is_none() { | |
| world.original_path = Some(std::env::var_os("PATH").unwrap_or_default()); | |
| } | |
| std::env::set_var("PATH", dir.path()); | |
| world.temp = Some(dir); | |
| } |
🤖 Prompt for AI Agents
In tests/steps/process_steps.rs around lines 37 to 47, remove the unnecessary
unsafe block wrapping the call to std::env::set_var, since set_var is a safe
function and does not require unsafe. Simply call std::env::set_var("PATH",
dir.path()) directly without unsafe.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
src/runner.rs (2)
54-64: Use stable Rust API for error creation.
io::Error::otheris unstable. Use the stable alternative.- Err(io::Error::other(format!( + Err(io::Error::new(io::ErrorKind::Other, format!( "ninja exited with {}", output.status )))
54-64: Stream output for real-time feedback.The current implementation buffers all output before displaying it, which delays feedback during long builds. Stream output directly instead.
tests/steps/process_steps.rs (1)
69-72: Consider more specific error assertions.Assert on error message content for better test coverage and clearer failure feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)src/ir.rs(3 hunks)src/main.rs(1 hunks)src/runner.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/ninja_process.feature(1 hunks)tests/runner_tests.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/steps/mod.rssrc/main.rssrc/ir.rstests/runner_tests.rssrc/runner.rstests/cucumber.rstests/steps/process_steps.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.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/mod.rssrc/main.rssrc/ir.rstests/runner_tests.rssrc/runner.rstests/cucumber.rstests/steps/process_steps.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in thedocs/directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.
Files:
docs/roadmap.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/netsuke-design.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/roadmap.mddocs/netsuke-design.md
🧬 Code Graph Analysis (1)
tests/runner_tests.rs (1)
src/runner.rs (1)
run_ninja(44-65)
🪛 GitHub Check: build-test
src/ir.rs
[failure] 325-325:
this if statement can be collapsed
[failure] 343-343:
this if statement can be collapsed
[failure] 356-356:
this if statement can be collapsed
🪛 GitHub Actions: CI
src/ir.rs
[error] 325-325: Clippy lint error: this if statement can be collapsed.
🔇 Additional comments (7)
tests/steps/mod.rs (1)
5-5: LGTM - proper module organization.The addition of the
process_stepsmodule follows the established pattern and properly integrates the new Cucumber step definitions for ninja process testing.docs/roadmap.md (1)
64-65: LGTM - roadmap item correctly marked as complete.The subprocess implementation task is properly marked as done, reflecting the ninja invocation functionality implemented in this PR.
docs/netsuke-design.md (1)
1158-1163: LGTM - comprehensive subprocess implementation documentation.This documentation clearly describes the ninja subprocess invocation approach, including flag forwarding, stream handling, and error reporting. The technical details align well with the implementation objectives.
tests/features/ninja_process.feature (1)
1-20: LGTM - comprehensive ninja process execution scenarios.This Cucumber feature file provides excellent test coverage for ninja subprocess execution, including success, failure, and missing executable scenarios. The Given/When/Then structure is properly formatted and the test cases comprehensively cover the key execution paths.
src/main.rs (1)
1-17: Excellent error handling and documentation.The changes properly implement exit code handling, error propagation, and include comprehensive module documentation. The separation of concerns between CLI parsing and execution delegation is well-structured.
tests/runner_tests.rs (2)
58-64: Make error assertion more specific.Assert on the specific error kind rather than just checking it exists.
#[rstest] fn run_ninja_not_found() { let cli = test_cli(); let err = runner::run_ninja(Path::new("does-not-exist"), &cli, &[]).expect_err("process should fail"); assert_eq!(err.kind(), std::io::ErrorKind::NotFound); }The current implementation is actually correct. The test properly verifies the error kind.
22-46: Document the FakeNinja struct and its methods.Add proper documentation for the test utility struct and its methods.
+/// Test utility that creates a fake ninja executable for testing. +/// +/// Creates a temporary shell script that exits with the specified code, +/// enabling controlled testing of subprocess behaviour. struct FakeNinja { _dir: TempDir, path: PathBuf, } impl FakeNinja { + /// Creates a new fake ninja executable that exits with the given code. fn new(exit_code: i32) -> Self {+ /// Returns the path to the fake ninja executable. fn path(&self) -> &Path {Likely an incorrect or invalid review comment.
| impl Drop for CliWorld { | ||
| fn drop(&mut self) { | ||
| if let Some(path) = self.original_path.take() { | ||
| unsafe { | ||
| std::env::set_var("PATH", path); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove unnecessary unsafe block.
std::env::set_var is safe and doesn't require an unsafe block.
impl Drop for CliWorld {
fn drop(&mut self) {
if let Some(path) = self.original_path.take() {
- unsafe {
- std::env::set_var("PATH", path);
- }
+ std::env::set_var("PATH", path);
}
}
}📝 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.
| impl Drop for CliWorld { | |
| fn drop(&mut self) { | |
| if let Some(path) = self.original_path.take() { | |
| unsafe { | |
| std::env::set_var("PATH", path); | |
| } | |
| } | |
| } | |
| } | |
| impl Drop for CliWorld { | |
| fn drop(&mut self) { | |
| if let Some(path) = self.original_path.take() { | |
| std::env::set_var("PATH", path); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/cucumber.rs around lines 19 to 27, remove the unnecessary unsafe block
wrapping the call to std::env::set_var inside the Drop implementation for
CliWorld, since std::env::set_var is a safe function and does not require
unsafe.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
tests/cucumber.rs (1)
11-13: Add documentation for the new test state fields.Document the purpose of each new field to improve code maintainability:
pub ninja: Option<String>, + /// Status of the last process execution (true for success, false for failure). pub run_status: Option<bool>, + /// Error message from the last failed process execution. pub run_error: Option<String>, + /// Temporary directory handle for test isolation. pub temp: Option<tempfile::TempDir>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/ir.rs(3 hunks)src/runner.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/ninja_process.feature(1 hunks)tests/runner_tests.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/cucumber.rstests/runner_tests.rssrc/ir.rssrc/runner.rstests/steps/process_steps.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.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/cucumber.rstests/runner_tests.rssrc/ir.rssrc/runner.rstests/steps/process_steps.rs
🧬 Code Graph Analysis (1)
tests/runner_tests.rs (1)
src/runner.rs (1)
run_ninja(49-96)
🔇 Additional comments (10)
src/ir.rs (2)
325-328: Good refactoring to avoid unstable features.The change from nested if-let statements to
.get(node).and_then(...)maintains the same logic while using stable Rust syntax. This correctly addresses the previous Clippy warnings without requiring the unstablelet_chainsfeature.
359-363: Excellent use of early return pattern.The refactoring to use
continuefor already-processed nodes improves readability by separating concerns. This makes the control flow clearer than the previous combined condition.tests/features/ninja_process.feature (1)
1-19: Well-structured feature tests for Ninja process execution.The scenarios comprehensively cover the main execution paths: success, failure with exit code, and missing executable. The error messages are specific enough to verify correct error handling.
tests/runner_tests.rs (1)
1-65: Excellent test coverage for Ninja subprocess invocation.The test module is well-structured with:
- Clear module documentation
- Parameterised tests using
rstestfor success/failure cases- Clever
FakeNinjahelper that creates executable scripts with proper permissions- Platform-specific handling for Unix permissions
- Proper RAII pattern with
_dirfield ensuring cleanupsrc/runner.rs (2)
19-34: Well-implemented command dispatch with proper error propagation.The refactored
runfunction correctly:
- Returns
io::Result<()>for error handling- Defaults to Build command when none specified
- Delegates to
run_ninjafor build execution- Returns
Ok(())for stub commands
49-96: Excellent implementation of Ninja subprocess management.The
run_ninjafunction implements all requirements effectively:
- Forwards CLI options (-C, -j) to Ninja
- Streams output in real-time using separate threads
- Handles both stdout and stderr concurrently
- Provides clear error messages on failure
- Documents the expect() calls appropriately
- Uses appropriate lint suppression with clear reasoning
The implementation aligns perfectly with the documented design goals.
tests/steps/process_steps.rs (4)
1-1: LGTM: Proper module documentation.The module-level doc comment clearly explains the module's purpose.
67-70: LGTM: Proper lint suppression with clear reasoning.The #[allow] directive is correctly scoped and includes a clear reason explaining why the parameter must be owned for Cucumber step definitions.
71-82: LGTM: Well-documented function with proper error handling.The function is properly documented and implements robust error checking with descriptive assertion messages.
52-55: Add function documentation for public step definition.Document this assertion step to explain what it verifies.
Apply this diff to add documentation:
+/// Asserts that the last ninja command executed successfully. #[then("the command should succeed")] fn command_should_succeed(world: &mut CliWorld) {Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tests/cucumber.rs(2 hunks)tests/runner_tests.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)tests/support/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/support/mod.rstests/runner_tests.rstests/cucumber.rstests/steps/process_steps.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.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/support/mod.rstests/runner_tests.rstests/cucumber.rstests/steps/process_steps.rs
🧠 Learnings (2)
📚 Learning: applies to docs/tests/cucumber.rs : the `world` struct should be defined per test suite to encapsula...
Learnt from: CR
PR: leynos/netsuke#0
File: docs/behavioural-testing-in-rust-with-cucumber.md:0-0
Timestamp: 2025-07-27T17:58:24.453Z
Learning: Applies to docs/tests/cucumber.rs : The `World` struct should be defined per test suite to encapsulate all scenario state, and should be instantiated per scenario to avoid state leakage between tests.
Applied to files:
tests/cucumber.rs
📚 Learning: applies to docs/tests/cucumber.rs : the `world` struct should be defined to encapsulate all scenario...
Learnt from: CR
PR: leynos/comenq#0
File: docs/behavioural-testing-in-rust-with-cucumber.md:0-0
Timestamp: 2025-07-27T00:57:23.556Z
Learning: Applies to docs/tests/cucumber.rs : The `World` struct should be defined to encapsulate all scenario state, and each scenario must use its own instance to avoid state leakage and concurrency issues.
Applied to files:
tests/cucumber.rs
🧬 Code Graph Analysis (1)
tests/support/mod.rs (1)
tests/steps/process_steps.rs (1)
fake_ninja(9-13)
🔇 Additional comments (14)
tests/support/mod.rs (2)
1-24: LGTM! The fake executable creation logic is sound.The implementation correctly creates a temporary fake Ninja executable with controlled exit behaviour. The conditional Unix permission setting and proper resource management are well implemented.
15-15: Useconcat!()for multi-line string literals.Replace the escaped newline with the
concat!()macro as per coding guidelines.- writeln!(file, "#!/bin/sh\nexit {exit_code}").expect("write script"); + writeln!(file, concat!("#!/bin/sh", "\nexit {exit_code}")).expect("write script");Likely an incorrect or invalid review comment.
tests/cucumber.rs (2)
14-21: LGTM! Proper documentation and state management.The new fields are well-documented and appropriately typed for process execution testing. The additions support proper test isolation as outlined in the retrieved learnings about World struct design.
24-24: LGTM! Module organisation follows project structure.Adding the support module alongside steps maintains consistent test organisation.
tests/runner_tests.rs (4)
1-18: LGTM! Well-structured unit tests with proper documentation.The test module follows coding guidelines with module documentation, documented helper functions, and proper use of rstest for parameterisation.
22-30: LGTM! Effective parameterised testing approach.The rstest parameterisation clearly tests both success and failure scenarios with controlled exit codes.
32-38: LGTM! Proper error handling verification.The test correctly verifies the specific error kind when the executable is not found, using
expect_err()appropriately.
20-20: Confirm correct support module import
Importmod support;intests/runner_tests.rscorrectly referencestests/support/mod.rs. Nosupport.rsexists—no changes required.tests/steps/process_steps.rs (6)
1-6: LGTM! Well-structured step definitions module.The module documentation and imports are appropriate for Cucumber step definitions supporting Ninja process testing.
7-13: LGTM! Clean fake ninja setup step.The step correctly uses the support utility and manages temporary resources in the test world state.
15-25: LGTM! Proper missing executable simulation.The step appropriately creates a scenario where ninja is not found by using a non-existent path in a temporary directory.
27-49: LGTM! Comprehensive process execution and state capture.The step correctly handles both success and failure cases, properly updating the world state for subsequent assertions.
51-55: LGTM! Clear success assertion.Simple and effective verification of command success.
57-65: LGTM! Thorough failure assertion.The step correctly verifies both failure status and presence of error message.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| process_steps.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/cucumber.rs (1)
26-35: Remove unnecessary unsafe block.The
unsafeblock violates coding guidelines which state "Avoid unsafe unless absolutely necessary". Thestd::env::set_varfunction is not unsafe, so this block is incorrect.Apply this diff to remove the unnecessary unsafe:
impl Drop for CliWorld { fn drop(&mut self) { - if let Some(path) = self.original_path.as_ref() { - // SAFETY: restoring the environment ensures isolation between tests. - unsafe { - std::env::set_var("PATH", path); - } - } + if let Some(path) = self.original_path.as_ref() { + std::env::set_var("PATH", path); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/cucumber.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/steps/process_steps.rstests/cucumber.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.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/process_steps.rstests/cucumber.rs
🧠 Learnings (1)
📚 Learning: applies to docs/tests/cucumber.rs : the main cucumber test runner must be implemented as a rust file...
Learnt from: CR
PR: leynos/ortho-config#0
File: docs/behavioural-testing-in-rust-with-cucumber.md:0-0
Timestamp: 2025-08-03T16:45:14.708Z
Learning: Applies to docs/tests/cucumber.rs : The main Cucumber test runner must be implemented as a Rust file in the `tests/` directory (e.g., `tests/cucumber.rs`) and must provide a custom `main` function with `#[tokio::main]` and call `World::run("tests/features")`.
Applied to files:
tests/cucumber.rs
🔇 Additional comments (8)
tests/cucumber.rs (2)
13-24: Well-documented field additions.The new fields for process testing are properly documented and serve clear purposes in the testing infrastructure.
38-38: Module structure follows conventions.The addition of the support module aligns with the test infrastructure requirements.
tests/steps/process_steps.rs (6)
1-6: Proper module structure and imports.The module documentation and import structure follows conventions appropriately.
18-31: Well-implemented fake executable creation.The step properly creates a fake ninja executable and manages the test environment appropriately.
33-50: Clean implementation for missing executable scenario.The step correctly sets up an environment where ninja is not available for testing error conditions.
52-74: Robust process execution step.The step properly executes the ninja process and captures both success and error states for testing.
76-108: Comprehensive assertion steps.The assertion steps provide thorough verification of command outcomes, including specific error message matching.
93-96: Correct use of expect attribute.The use of
#[expect]instead of#[allow]follows coding guidelines properly with clear reasoning.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/ast.rs(1 hunks)tests/steps/process_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
src/ast.rstests/steps/process_steps.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.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/ast.rstests/steps/process_steps.rs
🔇 Additional comments (1)
tests/steps/process_steps.rs (1)
1-108: Excellent test infrastructure implementation.The step definitions are well-structured with proper documentation, appropriate lint suppressions, and comprehensive error handling. The helper function effectively eliminates code duplication between the setup steps, and the test isolation through PATH manipulation is correctly implemented.
leynos
force-pushed
the
codex/implement-process-management-in-main.rs
branch
from
0a082b9 to
119db99
Compare
1 participant
Summary
Testing
make lintmake testmake markdownlintmake nixie(fails: too many arguments. Expected 0 arguments but got 1)https://chatgpt.com/codex/tasks/task_e_688dddde83308322b0c26df2217a0278
Summary by Sourcery
Invoke the Ninja build tool as a subprocess in the CLI, stream its output, and handle errors gracefully while updating the application exit code and tests accordingly
Enhancements:
Documentation:
Tests: