Conversation
Reviewer's GuideThis PR refactors the runner to generate Ninja manifests on the fly—either writing to a temporary file by default or to a user-specified path—by extracting generation logic into a helper, extending the CLI with a new --emit option and Emit subcommand, updating documentation and tests to cover both transient and persistent emission, and adjusting run_ninja to invoke Ninja with the proper build file. Sequence diagram for build command with and without --emitsequenceDiagram
actor User
participant CLI
participant Runner
participant FileSystem
participant Ninja
User->>CLI: netsuke build [--emit FILE] [targets]
CLI->>Runner: run(cli)
Runner->>Runner: generate_ninja(cli)
alt --emit FILE specified
Runner->>FileSystem: write Ninja file to FILE
Runner->>Ninja: run_ninja -f FILE [targets]
else default (no --emit)
Runner->>FileSystem: create temp Ninja file
Runner->>Ninja: run_ninja -f temp_file [targets]
Runner->>FileSystem: delete temp Ninja file
end
Sequence diagram for emit subcommandsequenceDiagram
actor User
participant CLI
participant Runner
participant FileSystem
User->>CLI: netsuke emit FILE
CLI->>Runner: run(cli)
Runner->>Runner: generate_ninja(cli)
Runner->>FileSystem: write Ninja file to FILE
Note over Runner,FileSystem: No build is run
Class diagram for updated CLI command structureclassDiagram
class Cli {
file: PathBuf
directory: Option<PathBuf>
jobs: Option<usize>
verbose: bool
command: Option<Commands>
}
class Commands {
}
class Build {
emit: Option<PathBuf>
targets: Vec<String>
}
class Emit {
file: PathBuf
}
Commands <|-- Build
Commands <|-- Emit
Cli --> Commands
Class diagram for runner logic refactorclassDiagram
class Runner {
+run(cli: &Cli) -> io::Result<()>
+generate_ninja(cli: &Cli) -> io::Result<String>
+run_ninja(program: &Path, cli: &Cli, build_file: &Path, targets: &[String]) -> io::Result<()>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughUpdate the CLI, runner, and related documentation to add new subcommands and options for emitting Ninja manifest files. Refactor the build logic to support writing the manifest to user-specified locations or temporary files. Extend and adapt tests and step definitions to cover the new CLI surface and behaviour. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Runner
participant FileSystem
participant Ninja
User->>CLI: netsuke build --emit out.ninja target
CLI->>Runner: Commands::Build { emit: Some(out.ninja), targets: [...] }
Runner->>Runner: generate_ninja()
Runner->>FileSystem: Write Ninja manifest to out.ninja
Runner->>Ninja: Run Ninja with -f out.ninja
Ninja-->>Runner: Build output
Runner-->>CLI: Exit
User->>CLI: netsuke emit out.ninja
CLI->>Runner: Commands::Emit { file: out.ninja }
Runner->>Runner: generate_ninja()
Runner->>FileSystem: Write Ninja manifest to out.ninja
Runner-->>CLI: Exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
🧰 Additional context used📓 Path-based instructions (4)Cargo.toml📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
docs/**/*.md📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
**/*.md📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🪛 GitHub Check: build-testtests/support/mod.rs[warning] 58-58: [warning] 30-30: [warning] 58-58: [warning] 30-30: 🪛 GitHub Actions: CItests/support/mod.rs[error] 30-30: Prettier formatting check failed. Diff detected in attribute formatting for #[allow(unfulfilled_lint_expectations, reason = ...)]. Run 'cargo fmt --all -- --write' to fix code style issues. [error] 58-58: Prettier formatting check failed. Diff detected in attribute formatting for #[allow(unfulfilled_lint_expectations, reason = ...)]. Run 'cargo fmt --all -- --write' to fix code style issues. [error] 84-84: Prettier formatting check failed. Diff detected in attribute formatting for #[allow(unfulfilled_lint_expectations, reason = ...)]. Run 'cargo fmt --all -- --write' to fix code style issues. 🔇 Additional comments (29)
✨ 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.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/runner.rs:88` </location>
<code_context>
+/// let ninja = generate_ninja(&cli).expect("generate");
+/// assert!(ninja.contains("rule"));
+/// ```
+fn generate_ninja(cli: &Cli) -> io::Result<String> {
+ let manifest_path = cli
+ .directory
</code_context>
<issue_to_address>
The error handling in generate_ninja may obscure the original error source.
Using io::Error::other erases error details. Consider using thiserror, anyhow, or at least include the original error message for better debugging.
Suggested implementation:
```rust
+use anyhow::{Context, Result};
+
+fn generate_ninja(cli: &Cli) -> Result<String> {
+ let manifest_path = cli
+ .directory
```
- You will need to add `anyhow` to your `Cargo.toml` dependencies if it is not already present.
- Update all internal error handling in `generate_ninja` to use `?` for propagation, and optionally `.context("...")` to add custom error messages.
- Update all call sites of `generate_ninja` to handle `anyhow::Result` instead of `io::Result`.
</issue_to_address>
### Comment 2
<location> `src/runner.rs:187` </location>
<code_context>
if let Some(jobs) = cli.jobs {
cmd.arg("-j").arg(jobs.to_string());
}
+ cmd.arg("-f").arg(build_file);
cmd.args(targets);
cmd.stdout(Stdio::piped());
</code_context>
<issue_to_address>
Passing the build file path to Ninja may require normalization.
If build_file is a temporary file, verify its path is valid and accessible from Ninja's working directory. Consider using an absolute or canonical path to avoid issues with relative paths.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
cmd.arg("-f").arg(build_file);
=======
// Canonicalize the build_file path to ensure Ninja receives an absolute path.
let build_file_path = match build_file.canonicalize() {
Ok(abs_path) => abs_path,
Err(_) => build_file.to_path_buf(), // fallback to original if canonicalization fails
};
cmd.arg("-f").arg(&build_file_path);
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `tests/steps/process_steps.rs:65` </location>
<code_context>
} else {
std::path::Path::new("ninja")
};
- match runner::run_ninja(program, cli, &[]) {
+ match runner::run_ninja(program, cli, Path::new("build.ninja"), &[]) {
Ok(()) => {
world.run_status = Some(true);
</code_context>
<issue_to_address>
Process step updated to pass build file path to run_ninja.
Please add tests for scenarios where the build file is missing or invalid.
</issue_to_address>
### Comment 4
<location> `src/runner.rs:32` </location>
<code_context>
- run_ninja(Path::new("ninja"), cli, &targets)
+ Commands::Build { targets, emit } => {
+ let ninja_content = generate_ninja(cli)?;
+ if let Some(path) = emit {
+ fs::write(&path, ninja_content.as_bytes()).map_err(io::Error::other)?;
+ info!("Generated Ninja file at {}", path.display());
</code_context>
<issue_to_address>
Consider extracting the repeated file-writing and logging logic into a helper function to simplify the control flow in the run function.
Here’s one way to collapse all three “write‐file → log → maybe run Ninja” sites into a tiny helper, then use a single `match emit` in your `Build` arm and a trivial call in `Emit`:
```rust
// add at top of runner.rs
fn write_and_log(path: &Path, content: &str) -> io::Result<()> {
fs::write(path, content).map_err(io::Error::other)?;
info!("Generated Ninja file at {}", path.display());
Ok(())
}
```
Then simplify your `run()` to something like:
```rust
pub fn run(cli: &Cli) -> io::Result<()> {
match cli.command.clone().unwrap() {
Commands::Build { targets, emit } => {
let ninja = generate_ninja(cli)?;
match emit {
Some(path) => {
write_and_log(&path, &ninja)?;
run_ninja(Path::new("ninja"), cli, &path, &targets)
}
None => {
let mut tmp = Builder::new()
.prefix("netsuke.")
.suffix(".ninja")
.tempfile()
.map_err(io::Error::other)?;
write_and_log(tmp.path(), &ninja)?;
run_ninja(Path::new("ninja"), cli, tmp.path(), &targets)
}
}
}
Commands::Emit { file } => {
let ninja = generate_ninja(cli)?;
write_and_log(&file, &ninja)
}
// ...
}
}
```
This:
- Extracts the common write + info into `write_and_log`.
- Collapses the two Build‐branches into one `match emit`.
- Makes `Emit` a one‐liner.
</issue_to_address>
### Comment 5
<location> `docs/netsuke-design.md:1351` </location>
<code_context>
-use clap::Subcommand;
-#[rustfmt::skip]
-use std::path::PathBuf;
+use clap::{Parser, Subcommand}; use std::path::PathBuf;
#[derive(Parser)]
</code_context>
<issue_to_address>
Rust code block line exceeds 120 columns; please wrap to comply with code block wrapping rule.
The line combining multiple imports in the Rust code block exceeds the 120 column limit for code blocks. Please wrap the line so that no line in the code block exceeds 120 columns.
</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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
Cargo.toml(1 hunks)docs/netsuke-design.md(4 hunks)src/cli.rs(3 hunks)src/runner.rs(6 hunks)tests/cli_tests.rs(1 hunks)tests/features/cli.feature(1 hunks)tests/runner_tests.rs(4 hunks)tests/steps/cli_steps.rs(5 hunks)tests/steps/process_steps.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: 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.rssrc/cli.rstests/cli_tests.rstests/steps/cli_steps.rstests/runner_tests.rssrc/runner.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.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.rssrc/cli.rstests/cli_tests.rstests/steps/cli_steps.rstests/runner_tests.rssrc/runner.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/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/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/netsuke-design.md
🧬 Code Graph Analysis (2)
tests/steps/process_steps.rs (1)
src/runner.rs (1)
run_ninja(169-235)
src/runner.rs (3)
src/manifest.rs (1)
from_path(49-54)src/ir.rs (1)
from_manifest(108-119)src/ninja_gen.rs (1)
generate(37-73)
🔇 Additional comments (24)
tests/steps/process_steps.rs (2)
6-6: LGTM!Addition of Path import is necessary for the updated run_ninja function call.
65-65: LGTM!Correctly updated the run_ninja call to include the explicit build file path parameter, aligning with the new function signature.
tests/features/cli.feature (1)
26-37: LGTM!Well-structured test scenarios that comprehensively cover the new emit functionality for both the --emit option and standalone emit subcommand.
tests/cli_tests.rs (1)
12-37: LGTM!Comprehensive test coverage that correctly adapts existing cases to include the new emit field and adds proper test cases for the new emit functionality. Good use of rstest parameterisation as per coding guidelines.
src/cli.rs (2)
75-90: LGTM!Well-designed CLI extension that adds the optional emit field to the Build command with proper default initialisation.
102-108: LGTM!Clean implementation of the new Emit subcommand with clear documentation and appropriate field structure.
tests/steps/cli_steps.rs (4)
20-22: LGTM!Correctly adds the new
emitfield to the defaultBuildcommand withNoneas expected.
34-40: LGTM!Function signature correctly updated to return both targets and emit field as a tuple, matching the expanded
Commands::Buildstructure.
90-97: LGTM!New step definition follows the established pattern for command verification and correctly matches the
Commands::Emitvariant.
115-115: LGTM!Correctly destructures the tuple from
extract_buildto access targets while ignoring the emit field.docs/netsuke-design.md (3)
1351-1351: LGTM!Good consolidation of related imports from the same crates, improving readability.
1376-1391: LGTM!Documentation accurately reflects the CLI structure changes, clearly explaining the new
--emitoption and additional subcommands with consistent formatting.
1402-1425: LGTM!Clear and accurate documentation of the new emit functionality, properly explaining both the
--emitoption behavior and theemitsubcommand's purpose.tests/runner_tests.rs (5)
4-4: LGTM!Necessary import for serial test execution to avoid race conditions when modifying environment variables.
20-23: LGTM!Correctly updates the
Commands::Buildusage to include the newemitfield with appropriateNonevalue.
37-95: LGTM!Good addition of error case testing with
run_ninja_not_foundand improved test naming. The assertion ensuring no ninja file persists correctly validates the temporary file behavior.
97-138: LGTM!Comprehensive test for emit functionality with proper environment setup, file validation, and cleanup. Good use of platform-specific testing with
cfg(unix).
140-169: LGTM!Well-designed test for the emit subcommand that cleverly ensures ninja isn't invoked by clearing PATH, while properly validating file creation and cleanup.
src/runner.rs (6)
12-12: LGTM!Necessary import addition for the updated
run_ninjafunction signature.
15-15: LGTM!Required import for temporary file creation functionality.
25-66: LGTM!Well-structured implementation that correctly handles both emit and temporary file scenarios. Good use of
tempfile::Builderfor secure temporary file creation and proper resource management.
68-98: LGTM!Well-designed helper function with clear single responsibility, proper error handling, and useful debug logging. Good separation of concerns from the main run logic.
157-174: LGTM!Good improvement to make the build file path explicit rather than implicit. The signature change enhances flexibility while maintaining clear documentation.
187-187: LGTM!Correctly uses the explicit build file parameter with the
-fflag for Ninja execution.
There was a problem hiding this comment.
Gates Failed
(1 hotspot with String Heavy Function Arguments)
(1 file with String Heavy Function Arguments)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| runner.rs | 1 rule in this hotspot | 10.00 → 9.69 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| runner.rs | 1 advisory rule | 10.00 → 9.69 | 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/netsuke-design.md(4 hunks)src/runner.rs(5 hunks)tests/features/ninja_process.feature(1 hunks)tests/runner_tests.rs(3 hunks)tests/steps/process_steps.rs(4 hunks)tests/support/mod.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/support/mod.rstests/steps/process_steps.rssrc/runner.rstests/runner_tests.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.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/steps/process_steps.rssrc/runner.rstests/runner_tests.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/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/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/netsuke-design.md
🔇 Additional comments (18)
tests/support/mod.rs (1)
40-47: LGTM! Excellent use ofconcat!macro.The multi-line shell script construction follows the coding guidelines perfectly by using
concat!()rather than escaping newlines with backslashes.tests/features/ninja_process.feature (1)
21-34: LGTM! Comprehensive error scenario coverage.The new test scenarios effectively cover critical edge cases for build file validation. The scenarios appropriately test both missing files and invalid file types (directory instead of regular file), with clear step definitions and proper error expectations.
tests/steps/process_steps.rs (4)
6-7: LGTM! Clean import additions.The new imports for
std::fsandstd::path::Pathare properly added and necessary for the new step definitions.
37-42: LGTM! Well-structured step definition.The
fake_ninja_checkstep definition properly integrates with the support module and follows the established pattern of the existingfake_ninjastep.
56-69: LGTM! Comprehensive test setup steps.Both
cli_uses_temp_dirandbuild_dir_existsstep definitions are well-implemented and provide the necessary test setup for the new build file validation scenarios.
88-88: LGTM! Properly addresses past review feedback.The explicit build file path parameter addresses the previous review comment about adding tests for missing/invalid build files. The change aligns with the updated
run_ninjasignature.tests/runner_tests.rs (6)
4-4: LGTM! Proper test isolation with serial execution.Adding
serial_test::serialimport enables proper test isolation for environment variable modifications.
19-23: LGTM! Consistent CLI structure updates.The
Commands::Buildvariant properly includes the newemit: Nonefield, maintaining consistency with the updated CLI structure.
31-51: LGTM! Comprehensive error handling test.The
run_ninja_not_foundtest properly validates theNotFounderror condition when the ninja executable doesn't exist.
54-55: LGTM! Proper test serialisation for environment safety.The
#[serial]attribute ensures safe modification of the PATH environment variable without interfering with concurrent tests.
91-132: LGTM! Thorough emit functionality testing.The
run_build_with_emit_keeps_filetest comprehensively validates that the emit functionality creates the expected file with proper content whilst not leaving a default build.ninja file.
134-163: LGTM! Complete emit subcommand coverage.The
run_emit_subcommand_writes_filetest properly validates the standalone emit subcommand functionality without requiring ninja execution.docs/netsuke-design.md (2)
378-393: LGTM! Comprehensive CLI documentation updates.The updated CLI structure properly documents all new commands and options, including the
--emitflag for the Build command and the new Clean, Graph, and Emit subcommands. The documentation accurately reflects the implementation.
1351-1351: Approve CLI documentation updatesThe consolidated import at line 1351 is 23 characters long, well under the 120-column limit. All CLI commands and the new emit option are documented accurately and match the implementation.
src/runner.rs (4)
9-16: Good adoption of anyhow for error handlingThe switch to
anyhow::ResultwithContextimproves error reporting clarity, addressing the previous review suggestion effectively.
24-50: Well-structured refactoring with helper functionsThe extraction of
generate_ninjaandwrite_and_loghelpers effectively addresses the previous complexity concerns. The control flow is now cleaner and more maintainable.
78-109: Excellent error handling with contextThe function properly implements the suggested anyhow error handling with descriptive context messages. The extraction of this logic improves modularity.
180-204: Robust path handling implementationThe canonicalization of the build file path with fallback successfully addresses the previous review suggestion about path normalization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/runner.rs (1)
121-125: Use anyhow::Result for consistency.Change the return type to
Result<()>(anyhow) and add context to the file write operation for consistency with the rest of the module.-fn write_and_log(path: &Path, content: &NinjaContent) -> io::Result<()> { - fs::write(path, content.as_str())?; +fn write_and_log(path: &Path, content: &NinjaContent) -> Result<()> { + fs::write(path, content.as_str()) + .with_context(|| format!("Failed to write Ninja file to {}", path.display()))?; info!("Generated Ninja file at {}", path.display()); Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/runner.rs(5 hunks)tests/runner_tests.rs(3 hunks)tests/steps/process_steps.rs(4 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/runner_tests.rssrc/runner.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.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/runner_tests.rssrc/runner.rs
🔇 Additional comments (18)
tests/steps/process_steps.rs (4)
37-42: LGTM!The new step definition properly delegates to the support module and follows the established pattern.
56-62: LGTM!The function correctly sets up the CLI to use the temporary directory for testing scenarios.
64-69: LGTM!The function correctly creates a directory to test error handling when the build file is not a regular file.
88-89: LGTM!The changes correctly adapt to the updated
run_ninjaAPI with explicit build file path andBuildTargetswrapper.tests/runner_tests.rs (4)
9-29: LGTM!The test correctly verifies error handling for invalid manifest versions with the updated
Commands::Buildstructure.
31-52: LGTM!The test correctly verifies error handling when the Ninja executable is not found, using the new API with
BuildTargetsand explicit build file path.
54-90: LGTM!The test correctly verifies that the Ninja file is not persisted when
emitisNone, and properly handles PATH manipulation with serial test isolation.
92-133: LGTM!The test thoroughly verifies that the Ninja file is persisted at the specified path when using
--emit, and that no defaultbuild.ninjais created.src/runner.rs (10)
19-34: LGTM!The
NinjaContentwrapper provides semantic meaning for Ninja file content with appropriate#[must_use]attributes.
36-47: LGTM!The
CommandArgwrapper provides semantic meaning for command arguments with appropriate#[must_use]attributes.
49-64: LGTM!The
BuildTargetswrapper provides semantic meaning for build target collections with appropriate methods and#[must_use]attributes.
71-93: LGTM!The
runfunction correctly handles both ephemeral and persistent Ninja file scenarios based on theemitoption.
147-158: LGTM!The
generate_ninjafunction correctly loads the manifest, builds the graph, and generates Ninja content with proper error context.
167-170: LGTM!The function correctly identifies sensitive keywords using the
CommandArgwrapper.
179-181: LGTM!The function correctly delegates to
contains_sensitive_keywordusing theCommandArgwrapper.
194-203: LGTM!The function correctly redacts sensitive information while preserving keys, using the
CommandArgwrapper throughout.
217-219: LGTM!The function correctly applies redaction to all arguments using the
CommandArgwrapper.
235-308: LGTM!The
run_ninjafunction correctly handles the build file path with canonicalization fallback, uses the new wrapper types, and properly redacts sensitive arguments before logging.
|
@coderabbitai Please create a new GitHub issue for the following: tests/steps/process_steps.rs (1) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I've created a GitHub issue for the code improvement you identified in 🏁 Script executed: Length of output: 1869 🏁 Script executed: Length of output: 2470 @leynos I've successfully created GitHub issue #57 for the code improvement you identified in
The issue is available at: #57 The issue has been assigned to you and includes references back to this PR and discussion for full context. |
leynos
force-pushed
the
codex/implement-manifest-to-ninja-pipeline
branch
from
55dac5d to
360972a
Compare
1 participant
Summary
-f--emitoption andemitsubcommandTesting
make fmtmake lintmake testmake markdownlintmake nixie(fails: error: too many arguments. Expected 0 arguments but got 1.)https://chatgpt.com/codex/tasks/task_e_68924b2aa17c8322ad8b42ce4cdff36d
Summary by Sourcery
Allow dynamic generation of Ninja build files with options to emit or use temporary files when running the build
New Features:
Enhancements:
Build:
Documentation:
Tests: