Conversation
Implemented the `clean` subcommand by generating the Ninja manifest and invoking Ninja's clean tool (`ninja -t clean`). - Added `handle_clean` function to run the clean tool. - Introduced `run_ninja_tool` helper for running Ninja tools. - Updated CLI commands to handle `clean` properly. - Added feature tests to verify clean subcommand behavior. - Updated roadmap documentation to mark clean subcommand as done. This enables users to remove build artefacts through the clean command. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Reviewer's GuideImplements the Sequence diagram for clean subcommand invoking ninja -t cleansequenceDiagram
actor User
participant Cli as Cli_main
participant Runner as Runner_run
participant Process as Process_module
participant Ninja as Ninja_binary
User->>Cli: netsuke clean [flags]
Cli->>Runner: run(cli)
Runner->>Runner: match cli.command
Runner->>Runner: handle_clean(cli)
Runner->>Runner: generate_ninja(cli)
Runner->>Process: create_temp_ninja_file(ninja_manifest)
Process-->>Runner: temp_build_file_path
Runner->>Process: resolve_ninja_program()
Process-->>Runner: ninja_program_path
Runner->>Process: run_ninja_tool(program, cli, build_file, "clean")
Process->>Process: configure_ninja_command(cmd, cli, build_file, Tool("clean"))
Process->>Process: configure_ninja_base(cmd, cli, build_file)
Process->>Ninja: spawn ninja -t clean -f build_file [-C dir] [-j N]
Ninja-->>Process: exit_status
Process->>Process: check_exit_status(exit_status)
Process-->>Runner: Ok / Err(io::Error)
Runner-->>Cli: Result
Cli-->>User: Success output or error (includes "ninja exited" on failure)
Updated class diagram for Ninja process helpers and clean handlingclassDiagram
class Cli {
+directory
+jobs
}
class BuildTargets {
+as_slice()
}
class Command {
+current_dir(path)
+arg(arg)
+args(args)
+stdout(pipe)
+stderr(pipe)
+spawn()
+get_program()
}
class NinjaCommandType {
<<enumeration>>
Build(targets: BuildTargets)
Tool(tool: str)
}
class ProcessModule {
+resolve_ninja_program(): PathBuf
+configure_ninja_base(cmd: Command, cli: Cli, build_file: Path): Result
+configure_ninja_command(cmd: Command, cli: Cli, build_file: Path, command_type: NinjaCommandType): Result
+run_ninja(program: Path, cli: Cli, build_file: Path, targets: BuildTargets): Result
+run_ninja_tool(program: Path, cli: Cli, build_file: Path, tool: str): Result
+create_temp_ninja_file(manifest: NinjaManifest): Result
}
class RunnerModule {
+run(cli: Cli): Result
+handle_build(cli: Cli, args: BuildArgs): Result
+handle_clean(cli: Cli): Result
+generate_ninja(cli: Cli): Result
}
class NinjaBinary {
<<external>>
+build(targets)
+tool(tool: str)
}
Cli --> RunnerModule : passed_into
RunnerModule --> ProcessModule : uses
ProcessModule --> Command : configures
ProcessModule --> NinjaCommandType : configures_with
ProcessModule --> NinjaBinary : invokes
RunnerModule --> BuildTargets : builds_with
ProcessModule --> BuildTargets : builds_with
RunnerModule --> Cli : reads_flags
ProcessModule --> Cli : reads_flags
RunnerModule --> NinjaBinary : indirect_invocation_via_run_ninja_tool
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. WalkthroughSummarise the change: add an explicit Clean subcommand that generates a temporary Ninja manifest and invokes Ninja with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI handler
participant Runner as runner::run / handle_clean
participant Manifest as manifest generator
participant Temp as temp file writer
participant Process as process::run_ninja_tool
participant Ninja as Ninja binary
CLI->>Runner: invoke `clean` subcommand
activate Runner
Runner->>Manifest: generate build manifest text
Manifest-->>Runner: return manifest text
Runner->>Temp: write manifest to temporary file
Temp-->>Runner: return temp build file path
Runner->>Process: run_ninja_tool(program, cli, build_file, "clean")
activate Process
Process->>Process: configure_ninja_base (cwd, -j, -f)
Process->>Process: configure_ninja_tool_command (append `-t clean`)
Process->>Ninja: exec `ninja -f <build_file> -j <jobs> -t clean`
Ninja-->>Process: stream stdout/stderr and return exit status
Process-->>Runner: propagate result or error with context
Runner-->>CLI: return success or error
deactivate Process
deactivate Runner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Implement the clean subcommand by invoking Ninja tool '-t clean' via
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +91 to +100 fn configure_ninja_command(
cmd: &mut Command,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
configure_ninja_base(cmd, cli, build_file)?;
cmd.args(targets.as_slice());
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +318 to +335 fn run_clean_fails_with_failing_ninja() -> Result<()> {
let (_ninja_dir, _ninja_path, _guard) = ninja_with_exit_code(7)?;
let (temp, manifest_path) = create_test_manifest()?;
let cli = Cli {
file: manifest_path.clone(),
directory: Some(temp.path().to_path_buf()),
command: Some(Commands::Clean),
..Cli::default()
};
let err = run(&cli).expect_err("expected clean to fail when ninja exits non-zero");
let messages: Vec<String> = err.chain().map(ToString::to_string).collect();
ensure!(
messages.iter().any(|m| m.contains("ninja exited")),
"error should report ninja exit status, got: {messages:?}"
);
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +168 to +205 fn run_clean(world: &mut CliWorld) -> Result<()> {
let dir = world
.temp
.as_ref()
.context("CLI temp directory has not been initialised")?;
{
let cli = world
.cli
.as_mut()
.context("CLI configuration has not been initialised")?;
let temp_path = Utf8Path::from_path(dir.path())
.ok_or_else(|| anyhow!("temporary directory path is not valid UTF-8"))?;
let manifest_path = Utf8Path::from_path(&cli.file)
.ok_or_else(|| anyhow!("CLI manifest path is not valid UTF-8"))?;
let manifest = ensure_manifest_exists(temp_path, manifest_path)
.context("ensure manifest exists in temp workspace")?;
cli.file = manifest.into_std_path_buf();
// Clear directory since file is now an absolute path; otherwise
// resolve_manifest_path would incorrectly join them.
cli.directory = None;
}
let cli = world
.cli
.as_ref()
.context("CLI configuration has not been initialised")?;
match runner::run(cli) {
Ok(()) => {
world.run_status = Some(true);
world.run_error = None;
}
Err(e) => {
world.run_status = Some(false);
// Use alternate formatting to capture the full anyhow error chain.
world.run_error = Some(format!("{e:#}"));
}
}
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…est setup - Introduced NinjaCommandType enum to unify configuration of build and tool commands. - Replaced separate configure functions with a single configure_ninja_command using NinjaCommandType. - Centralized CLI preparation and result recording in test steps to reduce duplication. - Added helper to assert Ninja failure propagation in tests. - Improved error reporting consistency on Ninja non-zero exit statuses. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +331 to +340 fn run_ninja_tool_not_found() -> Result<()> {
let cli = Cli::default();
let err = run_ninja_tool(
Path::new("does-not-exist"),
&cli,
Path::new("build.ninja"),
"clean",
)
.err()
.context("expected run_ninja_tool to fail when binary is missing")?;❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
The clippy expect attribute on the test helper function was reformatted from a single line to multiple lines for improved readability and consistency. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Refactored tests for run_ninja and run_ninja_tool when the binary is missing by extracting common assertion logic into a new helper function `assert_binary_not_found`. This reduces code duplication and improves readability of failure checks for NotFound errors. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
fake_ninja_expect_toolhelper always writes a POSIXshscript and is used in step definitions without#[cfg(unix)], which will fail on Windows runners; consider either adding a Windows-specific script variant (e.g.,.bat) or guarding the usage and function with#[cfg(unix)]as you did for some of the tests. - The
prepare_cli(world, clear_directory: bool)helper uses a boolean flag to alter behavior, which makes call sites less self-documenting; consider splitting this into two explicit helpers (e.g.,prepare_cli_with_directoryandprepare_cli_with_absolute_file) or an enum to clarify intent at the call site.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `fake_ninja_expect_tool` helper always writes a POSIX `sh` script and is used in step definitions without `#[cfg(unix)]`, which will fail on Windows runners; consider either adding a Windows-specific script variant (e.g., `.bat`) or guarding the usage and function with `#[cfg(unix)]` as you did for some of the tests.
- The `prepare_cli(world, clear_directory: bool)` helper uses a boolean flag to alter behavior, which makes call sites less self-documenting; consider splitting this into two explicit helpers (e.g., `prepare_cli_with_directory` and `prepare_cli_with_absolute_file`) or an enum to clarify intent at the call site.
## Individual Comments
### Comment 1
<location> `tests/runner_tests.rs:320-322` </location>
<code_context>
use tempfile::TempDir;
+/// Make a script file executable on Unix platforms.
+#[cfg(unix)]
+fn make_script_executable(path: &PathBuf, context: &str) -> Result<()> {
+ use std::os::unix::fs::PermissionsExt;
</code_context>
<issue_to_address>
**suggestion (testing):** Add a more targeted assertion that `run_ninja_tool` passes the expected arguments (e.g. `-f`, `-C`, `-j`) when invoked via the clean subcommand.
The current test only validates that `-t clean` is passed, so it would still succeed if `run_ninja_tool` stopped providing other required flags like `-f <build_file>` or `-C`/`-j`. Please either extend `fake_ninja_expect_tool` to assert these additional arguments, or add a dedicated test that checks the full argument vector passed to `run_ninja_tool`, so regressions in the new configuration logic are caught reliably.
</issue_to_address>
### Comment 2
<location> `src/runner/process/mod.rs:91` </location>
<code_context>
Ok(())
}
+/// Specifies whether Ninja should run a build or invoke a tool subcommand.
+enum NinjaCommandType<'a> {
+ /// Standard build invocation with optional targets.
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the enum-based `NinjaCommandType` with two explicit helper functions for build and tool commands to keep the shared base setup while simplifying the control flow and call sites.
The enum-based `NinjaCommandType<'a>` does add indirection and a lifetime that don’t buy much for the two current call sites. You can keep the useful `configure_ninja_base` extraction while simplifying the rest by splitting into two thin helpers and removing the enum entirely.
For example:
```rust
fn configure_ninja_base(cmd: &mut Command, cli: &Cli, build_file: &Path) -> io::Result<()> {
if let Some(dir) = &cli.directory {
let canonical = canonicalize_utf8_path(dir.as_path())?;
cmd.current_dir(canonical.as_std_path());
}
if let Some(jobs) = cli.jobs {
cmd.arg("-j").arg(jobs.to_string());
}
let build_file_path = canonicalize_utf8_path(build_file).or_else(|_| {
Utf8PathBuf::from_path_buf(build_file.to_path_buf()).map_err(|_| {
io::Error::new(
ErrorKind::InvalidData,
format!(
"build file path {} is not valid UTF-8",
build_file.display()
),
)
})
})?;
cmd.arg("-f").arg(build_file_path.as_std_path());
cmd.stdout(Stdio::piped());
cmd.stderr(Stdio::piped());
Ok(())
}
```
Then replace `NinjaCommandType` + `configure_ninja_command` with two explicit helpers:
```rust
fn configure_ninja_build_command(
cmd: &mut Command,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
configure_ninja_base(cmd, cli, build_file)?;
cmd.args(targets.as_slice());
Ok(())
}
fn configure_ninja_tool_command(
cmd: &mut Command,
cli: &Cli,
build_file: &Path,
tool: &str,
) -> io::Result<()> {
configure_ninja_base(cmd, cli, build_file)?;
cmd.arg("-t").arg(tool);
Ok(())
}
```
Call sites become simpler and more self-documenting:
```rust
pub fn run_ninja(
program: &Path,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
let mut cmd = Command::new(program);
configure_ninja_build_command(&mut cmd, cli, build_file, targets)?;
log_command_execution(&cmd);
let child = cmd.spawn()?;
let status = spawn_and_stream_output(child)?;
check_exit_status(status)
}
pub fn run_ninja_tool(
program: &Path,
cli: &Cli,
build_file: &Path,
tool: &str,
) -> io::Result<()> {
let mut cmd = Command::new(program);
configure_ninja_tool_command(&mut cmd, cli, build_file, tool)?;
log_command_execution(&cmd);
let child = cmd.spawn()?;
let status = spawn_and_stream_output(child)?;
check_exit_status(status)
}
```
This keeps all current behavior (including the new tool invocation feature), while:
- Removing the enum and its lifetime parameter.
- Making each path (`build` vs `tool`) explicit and easier to follow.
- Preserving the shared setup in `configure_ninja_base`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…rate functions Replaced the NinjaCommandType enum with separate functions for configuring standard build commands and tool commands to simplify command setup and improve clarity. Additionally, enhanced test support by improving the fake ninja tool script validation to check for both -t <tool> and -f <build_file> arguments. Updated process step tests to better manage CLI directory and file path preparations. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runner/process/mod.rs (1)
142-174: File exceeds 400-line limit.At 419 lines, this file breaches the 400-line maximum specified in the coding guidelines. The
CountingReader,CountingWriter, andForwardStatstypes (lines 225–315) are candidates for extraction into a dedicatedio_forwardingsubmodule.As per coding guidelines: "Files must not exceed 400 lines in length."
♻️ Duplicate comments (1)
tests/runner_tests.rs (1)
320-334: Reasonable coverage for success path.The test validates that the clean subcommand succeeds with the
ninja_expecting_cleanfixture, which already asserts both-t cleanand-f <build_file>are present.A previous review suggested extending assertions to cover
-C/-jflags. This remains a valid enhancement for comprehensive argument validation but is not blocking for the success case with default CLI options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/roadmap.md(1 hunks)src/runner/mod.rs(3 hunks)src/runner/process/mod.rs(3 hunks)test_support/src/check_ninja.rs(2 hunks)test_support/src/env.rs(1 hunks)tests/features/clean.feature(1 hunks)tests/runner_tests.rs(4 hunks)tests/steps/process_steps.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
test_support/src/env.rstests/steps/process_steps.rssrc/runner/mod.rstest_support/src/check_ninja.rstests/runner_tests.rssrc/runner/process/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
test_support/src/env.rstests/steps/process_steps.rssrc/runner/mod.rstest_support/src/check_ninja.rstests/runner_tests.rssrc/runner/process/mod.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: 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.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/roadmap.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/roadmap.md
🧬 Code graph analysis (3)
tests/steps/process_steps.rs (3)
test_support/src/check_ninja.rs (1)
fake_ninja_expect_tool(79-119)test_support/src/env.rs (1)
mocked_path_env(53-60)test_support/src/lib.rs (1)
ensure_manifest_exists(194-213)
src/runner/mod.rs (2)
src/runner/process/mod.rs (3)
run_ninja(143-155)run_ninja_tool(167-174)resolve_ninja_program(58-60)src/runner/process/file_io.rs (1)
create_temp_ninja_file(13-29)
tests/runner_tests.rs (4)
src/runner/mod.rs (4)
run(85-103)default(75-77)new(32-34)new(55-57)src/runner/process/mod.rs (3)
run_ninja(143-155)run_ninja_tool(167-174)new(351-353)test_support/src/check_ninja.rs (1)
fake_ninja_expect_tool(79-119)test_support/src/env.rs (1)
override_ninja_env(249-260)
🔍 Remote MCP
Based on my exploration of the Netsuke repository documentation and architecture, I can now provide you with a comprehensive summary of additional context relevant to reviewing this PR.
Additional Context Found
Repository Architecture and Related Work
Clean Subcommand Architecture:
The PR implements the clean subcommand as described in the related issue #158, which proposed a command-builder refactoring. The implementation creates:
- A new
handle_clean()function that generates a Ninja manifest and invokes it with the clean tool (-t clean) - A public
run_ninja_tool()function that enables tool-based command execution - Refactored helper functions:
configure_ninja_base(),configure_ninja_build_command(), andconfigure_ninja_tool_command()to support both standard builds and tool invocations
Runner Module Responsibilities:
The runner module acts as the central orchestrator between CLI arguments and the Ninja build system. It handles:
- Command dispatch (Build, Clean, Graph, Manifest subcommands)
- Manifest generation through a four-stage pipeline (path resolution, loading, IR generation, code generation)
- Temporary file management using RAII patterns
- Subprocess execution with proper error context propagation
Test Architecture Pattern:
The codebase employs a comprehensive three-layer testing strategy:
- Unit tests with
rstestfixtures for core algorithms - BDD tests with Cucumber for user-facing behavior specifications
- Snapshot tests with
instafor regression detection of generated IR and Ninja files
Review Comment Context
The review comments identify legitimate code duplication patterns that the PR introduces:
configure_ninja_commandvsconfigure_ninja_tool_commandsimilarity - Both build Ninja command structures with overlapping logic that could be consolidated through an enum-based approach- Test helper duplication - Multiple tests (
run_clean_fails_with_failing_ninja,run_fails_with_failing_ninja_env, etc.) share identical setup and assertion patterns - Path preparation duplication in steps - Both
runandrun_cleanprepare CLI with manifest paths and record results similarly
Design Philosophy Alignment
The PR aligns with Netsuke's core design principles:
- Security by default: Commands are shell-escaped during IR generation, not at Ninja execution time
- Separation of concerns: The runner delegates manifest loading, IR generation, and code generation to specialized modules
- Deterministic execution: The clean command follows the same deterministic pipeline as build commands
- Comprehensive error handling: Uses
anyhow::Contextfor error propagation with actionable messages
Implementation Quality Standards
Per AGENTS.md guidelines, the codebase enforces:
- Strict clippy linting (deny warnings for panic-prone operations, portability, unwrap usage)
- No direct
std::fsorstd::pathusage (mandatecap-stdandcamino) - Error types:
thiserrorfor domain errors,anyhowonly at application boundary - File maximum 400 lines per function for maintainability
- Test isolation through temporary directories and environment variable mocking
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (23)
docs/roadmap.md (1)
182-182: LGTM!The roadmap correctly marks the
cleansubcommand as complete, consistent with the implementation introduced in this PR.test_support/src/env.rs (1)
179-189: LGTM!The simplified manifest structure removes the nested
recipeblock and promotescommandto a direct target field. This aligns with the updated manifest parsing expectations used by the new tool-based invocation workflow.src/runner/process/mod.rs (1)
62-89: LGTM!The
configure_ninja_basehelper cleanly encapsulates common setup logic (working directory, job count, build file path, and I/O pipes). The UTF-8 path validation with fallback is appropriately defensive.test_support/src/check_ninja.rs (1)
9-25: LGTM!The
make_script_executablehelper properly abstracts platform-specific permission handling. The Unix implementation sets 0o755 with appropriate error context, whilst the non-Unix variant is a harmless no-op.tests/features/clean.feature (1)
1-29: LGTM!The feature file provides solid BDD coverage for the clean subcommand:
- Happy path with tool flag validation
- Error propagation when Ninja fails
- Working directory (
-C) flag handling- Jobs (
-j) flag handlingThe scenarios use consistent structure and align with the implementation's CLI options.
src/runner/mod.rs (3)
24-24: LGTM!Public export of
run_ninja_toolaligns with the PR objective to enable tool-based Ninja invocations from tests and subcommands.
97-97: LGTM!The
Cleanarm now correctly delegates tohandle_clean, replacing the previous no-op implementation.
159-183: LGTM!The
handle_cleanimplementation correctly:
- Generates the Ninja manifest via
generate_ninja.- Holds the temporary file in scope until
run_ninja_toolcompletes.- Provides actionable error context on failure.
- Uses en-GB spelling ("artefacts").
tests/steps/process_steps.rs (6)
64-71: LGTM!The Unix-only step correctly sets up a fake Ninja binary that validates the
-t cleantool invocation, following the established pattern for test ninja installation.
101-122: LGTM!The
prepare_cli_with_directoryhelper correctly extracts common CLI preparation logic, with clear error context and proper UTF-8 path validation.
124-137: LGTM!The
prepare_cli_with_absolute_filehelper correctly composesprepare_cli_with_directoryand clears the directory setting to prevent path-joining issues. The documentation clearly explains the rationale.
139-151: LGTM!The
record_resulthelper cleanly encapsulates the common pattern of updatingworld.run_statusandworld.run_errorbased on command execution outcome.
173-190: LGTM!The refactored
runstep correctly uses the extracted helpers, reducing duplication and improving clarity. The error propagation via?and result recording follow best practices.
192-209: LGTM!The Unix-only
run_cleanstep correctly:
- Uses
prepare_cli_with_absolute_filesincerunner::runhandles path resolution internally.- Captures the full
anyhowerror chain with alternate formatting ({e:#}).- Records results via the shared
record_resulthelper.tests/runner_tests.rs (9)
5-5: LGTM!Import of
run_ninja_toolenables the new test coverage for tool-based Ninja invocations.
97-119: LGTM!The
assert_ninja_failure_propagateshelper correctly extracts the common pattern for testing ninja failure propagation, reducing test duplication as noted in the PR comments. The#[expect(clippy::expect_used)]is appropriately scoped with a clear reason.
121-135: LGTM!The generic
assert_binary_not_foundhelper cleanly extracts the commonNotFoundassertion pattern, enabling reuse acrossrun_ninjaandrun_ninja_tooltests as suggested in the PR comments.
137-149: LGTM!The refactored
run_ninja_not_foundtest cleanly uses theassert_binary_not_foundhelper, making the test intent clear and reducing duplication.
302-305: LGTM!The refactored
run_fails_with_failing_ninja_envtest now delegates toassert_ninja_failure_propagates, eliminating duplication while preserving test coverage.
307-318: LGTM!The
ninja_expecting_cleanfixture follows the established pattern for test fixtures, correctly setting up a fake Ninja that validates the-t cleantool invocation.
336-340: LGTM!The
run_clean_fails_with_failing_ninjatest cleanly uses the shared helper to verify failure propagation for the clean subcommand.
342-354: LGTM!The
run_ninja_tool_not_foundtest correctly usesassert_binary_not_foundto verifyNotFounderror handling, following the established pattern.
356-377: LGTM!The
run_clean_fails_with_invalid_manifesttest correctly verifies that the clean subcommand reports manifest loading errors with actionable context.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +91 to +100 fn configure_ninja_build_command(
cmd: &mut Command,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
configure_ninja_base(cmd, cli, build_file)?;
cmd.args(targets.as_slice());
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…sions in YamlDiagnostic Add attribute annotations to suppress 'unused_assignments' lint warnings caused by miette/thiserror derive macro false positives on YamlDiagnostic struct fields. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… lint Remove redundant #[allow(unfulfilled_lint_expectations)] and #[expect] attributes, and replace with a single #[allow(unused_assignments)] on YamlDiagnostic struct. This addresses a false positive from miette/thiserror derive macros causing unused_assignments lint in certain Rust versions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…used assignments - Added `unused_assignments` to allowed Clippy lints in yaml.rs. - Removed redundant `#[allow(unused_assignments)]` attribute before the error struct. This change addresses inconsistent lint warnings caused by derive macros expanding field assignments differently across Rust versions, improving lint stability without affecting functionality. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Large Methodtest_support/src/check_ninja.rs: build_tool_validation_script What lead to degradation?build_tool_validation_script has 101 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
This comment was marked as resolved.
This comment was marked as resolved.
…ular flag checks Refactored the shell script generation for ninja tool validation in test_support/src/check_ninja.rs. Introduced reusable functions and a FlagValidation struct to modularize initialization, loop checking, and validation of optional flags (-j for jobs and -C for directory). This simplifies adding or modifying flag validations and reduces code duplication while preserving existing behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: test_support/src/check_ninja.rs Comment on file //! Helpers for validating build file paths via fake Ninja binaries.
//! Helpers for validating build file paths and tool invocations via fake Ninja binaries.
❌ New issue: String Heavy Function Arguments Use NewTypes to model domain values and eliminate "string soup". Reach for newt-hype when introducing many homogeneous wrappers that share behaviour; add small shims such as From<&str> and AsRef for string-backed wrappers. For path-centric wrappers implement AsRef alongside into_inner() and to_path_buf(), avoid attempting impl From for PathBuf because of the orphan rule. Prefer explicit tuple structs whenever bespoke validation or tailored trait surfaces are required, customizing Deref, AsRef, and TryFrom per type. Use the-newtype when defining traits and needing blanket implementations that apply across wrappers satisfying Newtype + AsRef/AsMut, or when establishing a coherent internal convention that keeps trait forwarding consistent without per-type boilerplate. Combine approaches: lean on newt-hype for the common case, tuple structs for outliers, and the-newtype to unify behaviour when owning the trait definitions. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/manifest/diagnostics/yaml.rs (1)
14-18: Replace module-level#![allow(...)]with scoped#[expect]on the struct.The module-level lint suppression violates the explicit coding guideline: "
#[allow]is forbidden. Only narrowly scoped#[expect(lint, reason = '...')]is allowed." The justification in the comment is incorrect—scope the#[expect]to theYamlDiagnosticstruct at lines 81-94, not at module level. If the lint doesn't fire in some versions, a narrowly-scoped#[expect]will handle that correctly.Apply this diff (remove module-level suppression, add scoped expectation to struct):
-// Module-level suppression for version-dependent lint false positives from -// miette/thiserror derive macros. The unused_assignments lint fires in some -// Rust versions but not others. Since `#[expect]` fails when the lint doesn't -// fire, and `unfulfilled_lint_expectations` cannot be expected, we must use -// `#[allow]` here. FIXME: remove once upstream is fixed. -#![allow( - clippy::allow_attributes, - clippy::allow_attributes_without_reason, - unused_assignments -)] - use super::{ManifestName, ManifestSource};Then at the struct definition:
+// Expect a version-dependent unused_assignments false positive from the +// miette/thiserror derive expansion. FIXME: remove once upstream is fixed or +// link to specific issue. +#[expect( + unused_assignments, + reason = "version-dependent false positive from miette/thiserror derive expansion" +)] #[derive(Debug, Error, Diagnostic)] #[error("{message}")] #[diagnostic(code(netsuke::yaml::parse))] struct YamlDiagnostic {🤖 Prompt for AI Agents
In src/manifest/diagnostics/yaml.rs, remove the module-level #![allow(...)] block at lines 14-18 and its associated comment at lines 9-13. Then, at the YamlDiagnostic struct definition (around line 81), add a narrowly scoped #[expect(unused_assignments, reason = "version-dependent false positive from miette/thiserror derive expansion")] attribute immediately before the #[derive(Debug, Error, Diagnostic)] line, along with a brief comment explaining the version-dependent false positive and a FIXME to remove once upstream is fixed. Do not include any lint groups or clippy::allow_attributes. Run cargo clippy and cargo test to verify the change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/manifest/diagnostics/yaml.rs(1 hunks)test_support/src/check_ninja.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
src/manifest/diagnostics/yaml.rstest_support/src/check_ninja.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/manifest/diagnostics/yaml.rstest_support/src/check_ninja.rs
🔍 Remote MCP
Summary
I've successfully retrieved comprehensive documentation from the Netsuke repository that provides substantial context for reviewing the "Implement clean subcommand" pull request (#247). Here's what I found:
Key Context for PR Review:
Architecture & Design:
- Netsuke is a build system compiler that transforms YAML manifests with Jinja2 templating into Ninja build files
- Uses a six-stage pipeline: ingestion → YAML parse → template expansion → deserialization → IR generation → Ninja synthesis
- The
runnermodule orchestrates the complete pipeline
PR-Relevant Implementation Details:
-
Command Runner Architecture - The PR adds a clean subcommand by extending the command dispatch system in
src/runner.rs. The existinghandle_build()pattern shows how commands are orchestrated. -
Process Module - The PR introduces
run_ninja_tool()as a complement to the existingrun_ninja(). The documentation confirms the process module handles subprocess invocation with proper error handling and context propagation usinganyhow::Context. -
Test Support Infrastructure - The documentation details:
- BDD testing via cucumber with
CliWorldstate management test_supportcrate providing fake ninja executables for testing- Snapshot testing with
instafor output validation - The PR extends this with
fake_ninja_expect_tool()functions
- BDD testing via cucumber with
-
Security & Design Patterns:
- Shell-quote escaping for command safety (
shell-quotecrate) - POSIX sh mode validation with
shlex - Capability-based I/O through
cap-stdfor sandboxed filesystem access - UTF-8 path handling via
camino
- Shell-quote escaping for command safety (
-
Error Handling:
- Three-layer model: domain errors (
thiserror) → context propagation (anyhow) → user-facing diagnostics - The PR's test helpers properly follow this pattern
- Three-layer model: domain errors (
Code Quality Standards:
- Strict clippy configuration with panic-prone operation denial
#[cfg(unix)]guards for platform-specific code (as implemented in the PR)- Atomic, focused functions with clear error contexts
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10772 @leynos: I've analysed the module. The AnalysisString parameters by category:
Suggested FixThe most idiomatic Rust approach is to use
Update the signatures to accept /// Builds the shell script content for validating ninja tool invocation.
#[cfg(unix)]
fn build_tool_validation_script(
expected_tool: ToolName,
expected_jobs: Option<u32>,
expected_directory: Option<&Path>,
) -> String {
let jobs = expected_jobs
.map(|j| FlagValidation::new("-j", "jobs", &j.to_string()))
.unwrap_or_else(FlagValidation::empty);
let dir = expected_directory
.and_then(|p| p.to_str())
.map(|d| FlagValidation::new("-C", "dir", d))
.unwrap_or_else(FlagValidation::empty);
TOOL_VALIDATION_TEMPLATE
.replace("{expected}", expected_tool.as_str())
.replace("{flag_inits}", &format!("{}{}", jobs.init, dir.init))
.replace(
"{flag_loop_checks}",
&format!("{}{}", jobs.loop_check, dir.loop_check),
)
.replace(
"{flag_validations}",
&format!("{}{}", jobs.validation, dir.validation),
)
}
/// Create a fake Ninja that validates `-t <tool>` and optionally `-j <jobs>` and `-C <dir>`.
///
/// The script scans command-line arguments for `-t <tool>`, `-f <file>`, and
/// optionally `-j <jobs>` and `-C <directory>`. It exits with status `0` if all
/// expected arguments are found, otherwise `1`.
///
/// # Arguments
///
/// * `expected_tool` - The tool name that should follow `-t` (e.g., `"clean"`)
/// * `expected_jobs` - Optional job count that should follow `-j`
/// * `expected_directory` - Optional working directory path that should follow `-C`
///
/// # Example
///
/// ```rust,ignore
/// use test_support::check_ninja::{fake_ninja_expect_tool_with_jobs, ToolName};
/// use std::path::Path;
///
/// let (dir, ninja_path) = fake_ninja_expect_tool_with_jobs(
/// ToolName::new("clean"),
/// Some(4),
/// Some(Path::new("/path/to/build")),
/// )?;
/// // ninja_path will succeed only when invoked with `-t clean -j 4 -C /path/to/build`
/// ```
#[cfg(unix)]
pub fn fake_ninja_expect_tool_with_jobs(
expected_tool: ToolName,
expected_jobs: Option<u32>,
expected_directory: Option<&Path>,
) -> Result<(TempDir, PathBuf)> {
let dir = TempDir::new().context("fake_ninja_expect_tool_with_jobs: create temp dir")?;
let path = dir.path().join("ninja");
let mut file = File::create(&path).with_context(|| {
format!(
"fake_ninja_expect_tool_with_jobs: create script {}",
path.display()
)
})?;
let script_content =
build_tool_validation_script(expected_tool, expected_jobs, expected_directory);
write!(file, "{}", script_content).with_context(|| {
format!(
"fake_ninja_expect_tool_with_jobs: write script {}",
path.display()
)
})?;
make_script_executable(&path, "fake_ninja_expect_tool_with_jobs")?;
Ok((dir, path))
}
/// Stub for non-Unix platforms that returns an error.
#[cfg(not(unix))]
pub fn fake_ninja_expect_tool_with_jobs(
_expected_tool: ToolName,
_expected_jobs: Option<u32>,
_expected_directory: Option<&Path>,
) -> Result<(TempDir, PathBuf)> {
anyhow::bail!("fake_ninja_expect_tool_with_jobs is only supported on Unix platforms")
}Update the test in the same file: let (dir, ninja_path) =
fake_ninja_expect_tool_with_jobs(ToolName::new("clean"), None, Some(Path::new("/path/to/build")))?;This reduces string parameters from 15 to 12 out of ~27 total parameters, bringing the ratio to ~44.4% → wait, that's still too high. Let me recalculate... Actually, the issue states the module has gone from having string parameters to 40.7%, so we need minimal reduction. Converting 3 AI Coding Agent Prompt |
…ests Refactor the `fake_ninja_expect_tool_with_jobs` function and related calls to accept `Option<&Path>` instead of `Option<&str>` for the expected directory argument. This provides better type safety and consistency. Additionally, the `fake_ninja_validates_directory_flag` test was updated to use parameterized test cases via `rstest` to cover multiple scenarios of the `-C` flag more succinctly and clearly. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: String Heavy Function Argumentstest_support/src/check_ninja.rs: What lead to degradation?In this module, 40.7% of all arguments to its 16 functions are strings. The threshold for string arguments is 39.0% Why does this problem occur?String is a generic type that fail to capture the constraints of the domain object it represents. In this module, 41 % of all function arguments are string types. How to fix it?Heavy string usage indicates a missing domain language. Introduce data types that encapsulate the semantics. For example, a user_name is better represented as a constrained User type rather than a pure string, which could be anything. |
This comment was marked as resolved.
This comment was marked as resolved.
…t generation - Introduced ShellFlag struct to encapsulate Ninja flags and variable names. - Combined separate functions into a single build_flag_validation returning init, loop_check, validation snippets. - Removed FlagValidation struct in favor of tuple returns, simplifying code. - Updated script building code to use new abstractions for jobs and directory flags. This improves code clarity and reduces duplication in shell script validations. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test_support/src/check_ninja.rs (1)
91-100: Fix broken error output infake_ninja_check_build_filescript.$2with double quotes so the argument interpolates; the current single-quoted string prints a literal$2.- " echo 'missing build file: $2' >&2\n", + " echo \"missing build file: $2\" >&2\n",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test_support/src/check_ninja.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
test_support/src/check_ninja.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
test_support/src/check_ninja.rs
🔍 Remote MCP Deepwiki, Ref
Summary of additional relevant facts for reviewing PR #247 ("Implement clean subcommand"):
-
New public API: run_ninja_tool(program, cli, build_file, tool) added; runner now re-exports run_ninja and run_ninja_tool (affects public surface and tests).,
-
Process refactor: configure_ninja_base + two configurators (configure_ninja_build_command, configure_ninja_tool_command) and centralized run_command_and_stream; run_ninja now uses build configurator and run_ninja_tool uses tool configurator.
-
Clean implementation: runner.handle_clean generates Ninja manifest (temporary or emitted), writes it, and invokes Ninja with -t clean via run_ninja_tool; uses same manifest-generation pipeline as build.
-
Tests and test-support:
- test_support: introduces ToolName newtype and Unix-only fake_ninja_expect_tool* helpers that create shell-script fake Ninja validating -t, -f, optional -j and -C flags; non-Unix stubs present.
- tests: new BDD feature tests (tests/features/clean.feature), step defs (tests/steps/process_steps.rs), and unit/integration tests (tests/runner_tests.rs) cover success, failing ninja exit, missing ninja binary, invalid manifest, and respect for -C and -j. Many tests are Unix-only guarded.
-
Manifest/test fixture change: test_support/src/env.rs changed embedded manifest structure (removed nested recipe block; promoted command field) — may affect tests that assert manifest text.
-
Security / platform notes: fake-Ninja helpers produce POSIX sh scripts and are appropriately #[cfg(unix)]-guarded; Windows stubs exist but do not validate tool invocation. Reviewers previously requested and confirmed these platform guards.
-
Documentation: docs/roadmap.md updated to mark the "clean" subcommand implemented.
Files of interest to review closely: src/runner/mod.rs, src/runner/process/mod.rs, test_support/src/check_ninja.rs, test_support/src/env.rs, tests/features/clean.feature, tests/runner_tests.rs, tests/steps/process_steps.rs, docs/roadmap.md.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (2)
test_support/src/check_ninja.rs (2)
9-29: KeepToolNameas-is; it enforces the domain concept and reduces string soup.
285-318: Keep the#[rstest]parameterisation; it removes duplication and tightens coverage.
Add a helper to safely shell-quote strings for insertion into shell scripts used in fake ninja tests. This prevents issues with unescaped values causing wrong shell script behavior or security risks. Update the tool validation scripts to use this quoting when embedding expected tool names and directory paths. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
1 participant
Summary
-t clean) with end-to-end tests, including success and failure scenarios.run_ninja_tooland refactors Ninja invocation helpers to support tool-based commands.-t clean) and wiring for tool-based tests.Changes
handle_cleanto implement thecleansubcommand (replacing the no-op prior handling)run_ninjaandrun_ninja_toolare publicly accessible for tests and subcommandsconfigure_ninja_base,configure_ninja_command, andconfigure_ninja_tool_commandrun_ninja_tool(program, cli, build_file, tool)to run Ninja with a specified tool (e.g.,clean)run_ninjalogic remains for manifest generation and normal build flowsfake_ninja_expect_tool(expected_tool)to simulate Ninja confirming it was called with-t <tool>run_cleanto execute the subcommand and capture results for assertionsTest plan
ninja -t cleanwith the expected argumentsHow to run tests
📎 Task: https://www.terragonlabs.com/task/ddd65a30-f188-4294-91ee-6785072de592
📎 Task: https://www.terragonlabs.com/task/091807bd-8f6b-4ef7-9d8f-8a2f7707942c