Conversation
Implemented the `graph` command, which generates the Ninja manifest into a temporary file and runs `ninja -t graph` to output the build dependency graph in DOT format. Updated CLI help and documentation to reflect this new command. Added comprehensive tests to verify the behavior of the graph subcommand under various conditions. This provides a useful introspection tool for users to visualize and debug build dependencies using Ninja's graph functionality. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughSummarise the runner to delegate Ninja tool invocations (e.g., Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI
participant Runner as netsuke::runner
participant FS as Filesystem
participant Ninja as ninja (external)
participant GraphViz as Graphviz (dot)
User->>Runner: invoke `netsuke graph` (with -j, manifest path)
Runner->>FS: ensure working dir, generate build.ninja via synth (invoke synth stage)
note right of FS: build.ninja is temporary
Runner->>Ninja: run `ninja -t graph` (preserve -j)
Ninja-->>Runner: emit DOT to stdout / exit code
alt success
Runner->>GraphViz: optional `dot -Tsvg` (mentioned in docs)
Runner-->>User: write DOT/SVG to stdout or file
else failure
Runner-->>User: propagate ninja exit error with context "ninja -t graph"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Reviewer's GuideImplements a concrete Sequence diagram for netsuke graph subcommand invoking ninja -t graphsequenceDiagram
actor User
participant Cli as Cli_args
participant Runner as Runner_run
participant Manifest as NinjaManifestGen
participant Process as Process_utils
participant Ninja as Ninja_executable
User->>Cli: netsuke graph [OPTIONS]
Cli->>Runner: run(cli)
Runner->>Runner: match Commands::Graph
Runner->>Runner: handle_graph(cli)
Runner->>Runner: handle_ninja_tool(cli, tool=graph)
Runner->>Manifest: generate_ninja(cli)
Manifest-->>Runner: ninja_manifest_string
Runner->>Process: create_temp_ninja_file(ninja_manifest_string)
Process-->>Runner: temp_build_file_path
Runner->>Process: resolve_ninja_program()
Process-->>Runner: ninja_program_path
Runner->>Process: run_ninja_tool(ninja_program_path, cli, temp_build_file_path, graph)
Process->>Ninja: ninja -t graph -f temp_build_file [jobs]
Ninja-->>Process: DOT_output_to_stdout
Process-->>User: DOT_output_streamed
Process-->>Runner: Ok
Runner-->>Cli: Ok
Cli-->>User: exit_code 0
Class diagram for refactored runner module with shared Ninja tool handlerclassDiagram
class Cli {
}
class Runner {
+run(cli : Cli) Result
+handle_build(cli : Cli, args : BuildArgs) Result
+handle_ninja_tool(cli : Cli, tool : str) Result
+handle_clean(cli : Cli) Result
+handle_graph(cli : Cli) Result
+generate_ninja(cli : Cli) Result
}
class BuildArgs {
}
class Process_utils {
+create_temp_ninja_file(ninja_manifest : str) Result
+resolve_ninja_program() PathBuf
+run_ninja_tool(program_path : Path, cli : Cli, build_file_path : Path, tool : str) Result
}
Cli <.. Runner : used_by
BuildArgs <.. Runner : used_by
Runner ..> Process_utils : uses
Runner ..> BuildArgs : uses
Runner ..> Cli : reads_options_from
class Commands {
<<enumeration>>
Build
Clean
Graph
}
Commands <.. Runner : dispatches_to_handlers
Commands : Build -> handle_build
Commands : Clean -> handle_clean
Commands : Graph -> handle_graph
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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
|
|
@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 +190 to +206 fn handle_graph(cli: &Cli) -> Result<()> {
info!(target: "netsuke::subcommand", subcommand = "graph", "Subcommand requested");
let ninja = generate_ninja(cli)?;
let tmp = process::create_temp_ninja_file(&ninja)?;
let build_path = tmp.path();
let program = process::resolve_ninja_program();
run_ninja_tool(program.as_path(), cli, build_path, "graph").with_context(|| {
format!(
"running {} -t graph with build file {}",
program.display(),
build_path.display()
)
})?;
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 +423 to +442 fn run_graph_fails_with_invalid_manifest() -> Result<()> {
let temp = tempfile::tempdir().context("create temp dir for invalid manifest test")?;
let manifest_path = temp.path().join("Netsukefile");
std::fs::copy("tests/data/invalid_version.yml", &manifest_path)
.with_context(|| format!("copy invalid manifest to {}", manifest_path.display()))?;
let cli = Cli {
file: manifest_path.clone(),
command: Some(Commands::Graph),
..Cli::default()
};
let Err(err) = run(&cli) else {
bail!("expected graph to fail for invalid manifest");
};
ensure!(
err.to_string().contains("loading manifest at"),
"error should mention manifest loading, got: {err}"
);
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 +248 to +258 fn run_graph(world: &mut CliWorld) -> Result<()> {
prepare_cli_with_absolute_file(world)?;
let cli = world
.cli
.as_ref()
.context("CLI configuration has not been initialised")?;
// Use alternate formatting to capture the full anyhow error chain.
let result = runner::run(cli).map_err(|e| format!("{e:#}"));
record_result(world, result);
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.
Refactored the Ninja tool invocations by introducing handle_ninja_tool to handle generic Ninja tools via temporary build files. Replaced specialized handle_clean and handle_graph implementations to use this unified handler. Updated tests to use common helpers for validating subcommand success and failure cases without leaving temporary build files. Refactored Cucumber test steps to consolidate subcommand runs through a shared function. This improves code reuse, clarity, and maintainability. 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/users-guide.md (2)
496-503: Fix list punctuation and wrap to 80 columns.
Remove the stray punctuation/spacing between thecleanandgraphbullets and keep thegraphdescription wrapped.- `clean`: Removes build artefacts by running `ninja -t clean`. Requires rules/targets to be properly configured for cleaning in Ninja (often via `phony` targets). -- `graph`: Generates the build dependency graph by running `ninja -t graph` on - the generated `build.ninja`, outputting DOT to stdout (suitable for - Graphviz). Future versions may support other formats like `--html`. +- `graph`: Generates the build dependency graph by running `ninja -t graph` on + the generated `build.ninja`, outputting DOT to stdout (suitable for + Graphviz). Future versions may support other formats like `--html`.
64-67: Document YAML 1.2 support and reject duplicate keys (required by guidelines).
Add an explicit statement in the user guide that Netsuke targets YAML 1.2 and forbids duplicate keys in manifests.tests/runner_tests.rs (1)
360-431: Collapse clean/graph subcommand tests into a single#[rstest]table.
Replace the duplicated test functions with one parameterised test over(fixture, command, name)to match the project’s Rust testing guidelines.tests/steps/process_steps.rs (1)
103-114: OverrideNINJA_ENVto a non-existent path in theno_ninjastep to ensure deterministic test execution.The current implementation uses
mocked_path_env()which only affectsPATHlookup. However,resolve_ninja_program()checksNINJA_ENVfirst before falling back to the"ninja"binary. On hosts with system Ninja installed, the lookup succeeds incorrectly, making the scenario non-deterministic. Useoverride_ninja_env()with the sharedEnvLockguard (already established in test_support) to force resolution to fail.This applies to both the
no_ninjastep (lines 103–114) and the subcommand execution paths (lines 223–255) that callrunner::run(cli)directly without passingworld.ninja.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/netsuke-design.md(1 hunks)docs/roadmap.md(1 hunks)docs/users-guide.md(1 hunks)src/runner/mod.rs(2 hunks)tests/features/graph.feature(1 hunks)tests/runner_tests.rs(3 hunks)tests/steps/process_steps.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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.mddocs/netsuke-design.mddocs/users-guide.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.mddocs/netsuke-design.mddocs/users-guide.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.mddocs/netsuke-design.mddocs/users-guide.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.mddocs/netsuke-design.mddocs/users-guide.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.mddocs/netsuke-design.mddocs/users-guide.md
**/*.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:
tests/steps/process_steps.rstests/runner_tests.rssrc/runner/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:
tests/steps/process_steps.rstests/runner_tests.rssrc/runner/mod.rs
🧬 Code graph analysis (3)
tests/steps/process_steps.rs (2)
test_support/src/check_ninja.rs (5)
fake_ninja_expect_tool(131-133)fake_ninja_expect_tool(292-294)new(15-17)fake_ninja_expect_tool_with_jobs(265-288)fake_ninja_expect_tool_with_jobs(298-304)test_support/src/env.rs (1)
mocked_path_env(53-60)
tests/runner_tests.rs (2)
test_support/src/check_ninja.rs (3)
fake_ninja_expect_tool(131-133)fake_ninja_expect_tool(292-294)new(15-17)test_support/src/env.rs (1)
override_ninja_env(249-260)
src/runner/mod.rs (4)
scripts/upload_release_assets.py (1)
cli(347-360)tests/steps/ninja_steps.rs (1)
generate_ninja(77-90)src/runner/process/file_io.rs (1)
create_temp_ninja_file(13-29)src/runner/process/mod.rs (2)
resolve_ninja_program(58-60)run_ninja_tool(172-176)
🪛 LanguageTool
docs/users-guide.md
[uncategorized] ~500-~500: Loose punctuation mark.
Context: ...often via phony targets). - graph: Generates the build dependency graph by...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ 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). (7)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (5)
docs/netsuke-design.md (1)
2000-2006: Hard-wrap to 80 columns and keep commands in code spans.
Enforce the docs wrapping rule and ensureninja -t graph/dot -Tsvgremain in backticks.src/runner/mod.rs (2)
80-100: Route Graph through the shared tool runner exactly as done here.
This dispatch keeps behaviour consistent with Clean and limits duplication.
156-206: Tool dispatch is already appropriately constrained and private.The
handle_ninja_tool()function is private, and tool values ("clean", "graph") flow exclusively through hardcoded enum dispatch inpub fn run(). No arbitrary user input reaches this function. The current pattern already prevents tool injection.Additionally,
ninja_envis a local workspace crate providing only a constant (NETSUKE_NINJA), so external version pinning concerns do not apply.tests/steps/process_steps.rs (1)
84-101: Keep graph fake-ninja fixtures aligned with Clean fixtures.
The tool-name expectations are consistent with the sharedcheck_ninja::fake_ninja_expect_tool*helpers.tests/runner_tests.rs (1)
135-186: Keep the shared helper assertions; they lock in the “no persisted build.ninja” contract.
The helper structure and error-context assertions are solid and readable.
Renamed tests/features/clean.feature, graph.feature, and ninja_process.feature to tests/features_unix/clean.feature, graph.feature, and ninja_process.feature respectively. Added @unix tag to each renamed feature file to specify Unix platform. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
- Document YAML 1.2 manifest constraints in the user guide. - Ensure Cucumber process steps override NINJA_ENV so missing-ninja scenarios are deterministic. - Collapse clean/graph subcommand tests into an rstest table to reduce duplication.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/roadmap.md (1)
184-185: Keep the roadmap item formatted and wrapped as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/roadmap.md(1 hunks)docs/users-guide.md(2 hunks)test_support/src/env.rs(1 hunks)test_support/src/env_lock.rs(2 hunks)tests/cucumber.rs(3 hunks)tests/features_unix/clean.feature(1 hunks)tests/features_unix/graph.feature(1 hunks)tests/features_unix/ninja_process.feature(1 hunks)tests/runner_tests.rs(2 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.rstest_support/src/env_lock.rstests/steps/process_steps.rstests/runner_tests.rstests/cucumber.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.rstest_support/src/env_lock.rstests/steps/process_steps.rstests/runner_tests.rstests/cucumber.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/users-guide.mddocs/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/users-guide.mddocs/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/users-guide.mddocs/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/users-guide.mddocs/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/users-guide.mddocs/roadmap.md
🧬 Code graph analysis (4)
test_support/src/env.rs (1)
test_support/src/env_lock.rs (1)
fmt(17-19)
test_support/src/env_lock.rs (3)
test_support/src/env.rs (1)
fmt(227-229)test_support/src/lib.rs (1)
fmt(64-72)src/ninja_gen.rs (2)
fmt(248-252)fmt(262-277)
tests/steps/process_steps.rs (2)
test_support/src/check_ninja.rs (5)
new(15-17)fake_ninja_expect_tool(131-133)fake_ninja_expect_tool(292-294)fake_ninja_expect_tool_with_jobs(265-288)fake_ninja_expect_tool_with_jobs(298-304)test_support/src/env.rs (2)
override_ninja_env(255-266)mocked_path_env(53-60)
tests/cucumber.rs (1)
test_support/src/env.rs (1)
restore_many(103-117)
🪛 LanguageTool
docs/users-guide.md
[uncategorized] ~504-~504: Loose punctuation mark.
Context: ...often via phony targets). - graph: Generates the build dependency graph by...
(UNLIKELY_OPENING_PUNCTUATION)
🔍 Remote MCP Ref
Let me search for general information about Ninja's graph tool and project structure patterns:## Summary of Relevant Context for PR Review
Ninja Graph Tool Behavior
Ninja can output GraphViz dependency graphs via the command ninja -t graph, which is what this PR leverages. The tool outputs a file in GraphViz syntax and is typically used via ninja -t graph mytarget | dot -Tpng -ograph.png. This confirms the PR's approach of invoking the Ninja tool to generate DOT format output for further processing or visualization.
Rust CLI Testing Patterns & Code Organization
The review comments correctly identify duplicated patterns. For similar situations, Rust CLI best practices recommend:
- Helper functions should group tests by common purpose, testing similar behavior, while avoiding helpers created solely to reduce repetitive code
- Specialized assert functions can be defined and reused repeatedly to verify different test scenarios within file scope
- When there are many test files with shared helper code, Rust's module system is the recommended approach
Subcommand Architecture
The PR correctly follows Rust CLI patterns where for a program with subcommands, a recommended structure delegates command execution through matched enum variants. The shared handle_ninja_tool helper approach aligns with this pattern by centralizing Ninja invocation logic for both clean and graph subcommands.
Test Fixture and Helper Design
Integration tests should cover all observable user behavior types without needing to cover all edge cases, with unit tests handling edge cases. The PR's feature tests for graph (success, Ninja failure, jobs handling, missing Ninja) and runner tests follow this sensible division.
Key Observations for Reviewers
- The refactoring suggestions in the PR comments (extracting
handle_ninja_tool,run_subcommand, and assertion helpers) align with established Rust patterns for reducing duplication while maintaining clarity. - The
ninja -t graphoption outputs dependency graphs in GraphViz format, validating the documentation updates claiming DOT output. - The test structure appropriately separates concerns: feature tests for user-observable behavior, runner tests for command execution, and step implementations for test infrastructure.
⏰ 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 (14)
test_support/src/env.rs (1)
226-230: Keep the non-exhaustive Debug impl; align it with EnvLock.
Match the establishedfinish_non_exhaustive()pattern so debug output never commits to field layout.tests/features_unix/clean.feature (1)
1-1: Keep the @unix tag.
Ensure Unix-only features remain trivially filterable in CI and local runs.test_support/src/env_lock.rs (1)
7-7: Keep the EnvLock Debug impl non-exhaustive.
Expose a stable debug shape without leaking internals.Also applies to: 16-20
docs/users-guide.md (2)
68-70: Keep the YAML 1.2 + duplicate-key prohibition note explicit.
State the behavioural contract unambiguously at the user-guide level. As per coding guidelines, document YAML 1.2 and duplicate-key rejection.
504-507: Keep the graph command description tied toninja -t graphand stdout DOT.
Prevent divergence between docs and the actual execution path.tests/features_unix/ninja_process.feature (1)
1-1: Keep the @unix tag.tests/features_unix/graph.feature (1)
1-29: Keep the scenario set; cover the full observable contract.
Assert success, propagation of Ninja exit failure,-jforwarding, and missing-executable behaviour.tests/cucumber.rs (1)
10-14: Enforce Drop ordering: drop NinjaEnvGuard before restoring env vars.
Keepself.ninja_env_guard.take();ahead ofrestore_environment()to prevent EnvLock contention while restoringenv_vars.Also applies to: 44-46, 187-193
tests/steps/process_steps.rs (3)
34-48: Avoid env-override lifetime leaks; keep the guard reset and SystemEnv override.Keep
world.ninja_env_guard.take();before installing the newNINJA_ENVoverride to prevent scenario-to-scenario bleed and to ensure the old guard drops before the new one is created. This is the right shape for deterministic step execution.
87-105: Keep graph fake-ninja steps Unix-gated and aligned with the tool-validation fixture.Maintain
#[cfg(unix)]on the graph steps becausecheck_ninja::fake_ninja_expect_tool_with_jobsis Unix-only; the ToolName wiring matches the runner’s-t graphcontract.
225-258: Keep subcommand execution centralised; preserve full error-chain formatting.Retain
run_subcommandand the clean/graph wrappers to prevent divergence between subcommand steps, and keepformat!("{e:#}")to preserve the full anyhow chain for assertions.tests/runner_tests.rs (3)
135-158: Keep the shared “no persisted build.ninja” assertion.Keep this helper: it hardens the tool-subcommand contract and prevents regressions where a transient manifest leaks into the project directory.
184-185: Keep theNinjaToolFixturealias to make the rstest table readable.This alias keeps the case table legible and avoids repeating a noisy function type.
388-399: Keep the graph fixture symmetrical with the clean fixture.Maintain this mirror fixture; it keeps the clean/graph subcommand tests structurally equivalent and easier to extend.
Move clean/graph tool-subcommand coverage into its own integration test module to keep test files under the 400-line limit. Also assert invalid-manifest failures via the error chain for robustness.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/runner_tests.rs (1)
85-88: Address the past review comment: check the error chain directly.Lines 85-88 still check
err.to_string()before inspecting the chain (lines 89-93). This pattern was flagged in a previous review. Consolidate into a single chain-based assertion to ensure resilience if the top-level message wording changes whilst the causal chain remains correct.Apply this diff:
- ensure!( - err.to_string().contains("loading manifest at"), - "error should mention manifest loading, got: {err}" - ); let chain: Vec<String> = err.chain().map(ToString::to_string).collect(); ensure!( - chain.iter().any(|s| s.contains("manifest parse error")), - "expected error chain to include 'manifest parse error', got: {chain:?}" + chain.iter().any(|s| s.contains("loading manifest at")), + "expected error chain to include 'loading manifest at', got: {chain:?}" + ); + ensure!( + chain.iter().any(|s| s.contains("manifest parse error")), + "expected error chain to include 'manifest parse error', got: {chain:?}" );In tests/runner_tests.rs at lines 85-93, replace the err.to_string().contains() check with a direct error chain assertion. Remove lines 85-88 that check err.to_string(), and modify the existing chain check at lines 89-93 to assert both "loading manifest at" and "manifest parse error" are present in the chain. Keep both assertions so the test validates the full error context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/runner_tests.rs(1 hunks)tests/runner_tool_subcommands_tests.rs(1 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:
tests/runner_tool_subcommands_tests.rstests/runner_tests.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:
tests/runner_tool_subcommands_tests.rstests/runner_tests.rs
🧬 Code graph analysis (1)
tests/runner_tool_subcommands_tests.rs (2)
test_support/src/env.rs (1)
override_ninja_env(255-266)test_support/src/lib.rs (1)
fake_ninja(95-127)
⏰ 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-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (8)
tests/runner_tests.rs (1)
9-9: LGTM: Import cleaned up after refactoring.The removal of
selfandToolNamefrom thecheck_ninjaimport aligns with the migration of tool-specific tests to the newrunner_tool_subcommands_tests.rsmodule.tests/runner_tool_subcommands_tests.rs (7)
1-5: LGTM: Clear module documentation.The module-level doc comment properly explains the file's scope and purpose.
39-59: LGTM: Proper error chain validation.The helper correctly validates ninja failures by inspecting the error chain (lines 53-56) rather than the top-level string representation, ensuring test resilience.
61-82: LGTM: Clean helper for verifying no file persistence.The helper appropriately validates that tool subcommands execute successfully without leaving
build.ninjaartefacts in the project directory.
84-107: LGTM: Exemplary error chain assertion pattern.Lines 101-104 correctly check the error chain directly rather than
err.to_string(), providing resilient assertions that remain valid even if top-level message wording changes. This is the pattern that should be followed throughout the test suite.
109-131: LGTM: Well-structured fixtures for tool-specific testing.The type alias (line 109) and tool-specific fixtures cleanly support parameterised test cases whilst maintaining type safety and clear intent.
133-143: LGTM: Clean delegation to shared helper.Both tests appropriately delegate to
assert_ninja_failure_propagatesfor ninja failure scenarios.
145-170: LGTM: Excellent use of parameterised testing.The
#[case]parameterisation elegantly covers success and failure paths for bothcleanandgraphsubcommands without duplicating test logic. The dispatch pattern (lines 164-169) cleanly separates fixture-based success tests from invalid-manifest failure tests.
|
@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 file use std::path::{Path, PathBuf};
use test_support::{
check_ninja::{self, ToolName},
check_ninja,❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Introduce a shared tests/common module for integration tests. - Deduplicate create_test_manifest and ninja_with_exit_code. - Keep rstest fixture discovery via local re-exports where needed.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract a helper function `assert_runner_not_found` to consolidate repeated code that asserts runner functions fail with `NotFound` errors when the ninja binary is missing. Updated existing tests `run_ninja_not_found` and `run_ninja_tool_not_found` to use this helper, improving code reuse and clarity. 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.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/runner_tests.rs(3 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:
tests/runner_tests.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:
tests/runner_tests.rs
🧬 Code graph analysis (1)
tests/runner_tests.rs (4)
test_support/src/env.rs (2)
override_ninja_env(255-266)prepend_dir_to_path(196-210)test_support/src/lib.rs (1)
fake_ninja(95-127)tests/common/mod.rs (2)
create_test_manifest(16-22)ninja_with_exit_code(28-35)test_support/src/check_ninja.rs (1)
new(15-17)
🔍 Remote MCP
Summary — additional repository facts relevant to reviewing PR #249
-
Runner/ninja execution:
- Runner generates a temporary build.ninja (or writes an --emit path), resolves Ninja program (env override supported), and invokes ninja with -f, -j and targets; temporary file kept alive by an RAII guard (NamedTempFile) so file exists while ninja runs [::Deepwiki_read_wiki_contents::].
- Errors use anyhow::Context with descriptive messages at each stage (load manifest → build graph → generate ninja → run ninja) — useful when reviewing new error contexts in handle_ninja_tool/handle_graph [::Deepwiki_read_wiki_contents::].
-
Manifest → IR → Ninja pipeline (relevant to graph behavior):
- Six-stage pipeline: parse YAML → template expansion (foreach/when) → deserialize → IR generation (validation, dedupe) → ninja synthesis → ninja execution. Graph generation path must run through IR + ninja synthesis to produce build.ninja before invoking ninja -t graph, matching docs/tests in this PR [::Deepwiki_read_wiki_contents::].
-
Security & command quoting (affects cmd construction passed to ninja -t graph/clean):
- All command placeholders are shell-escaped (shell-quote, POSIX sh mode) and validated with shlex; scripts are wrapped via printf %b into a single-line command — ensures generated build.ninja lines are safe for ninja to execute [::Deepwiki_read_wiki_contents::].
-
Tests and test helpers (what PR touched and must remain consistent):
- Test infra provides fake-ninja fixtures, NinjaEnvGuard, and helpers in tests/common/mod.rs and test_support crate; BDD steps (tests/steps/process_steps.rs) include installing fake ninja expecting -t graph or -t graph + -j — ensure new runner dispatch matches those expectations [::Deepwiki_read_wiki_contents::].
- CliWorld holds optional NinjaEnvGuard to override PATH during scenarios; Drop takes the guard to avoid env leakage [::Deepwiki_read_wiki_contents::].
-
Determinism & output expectations:
- Generated build.ninja and ninja rules are produced deterministically: actions and edges are sorted (hashes, path keys). Tests rely on deterministic ordering for snapshots — refactor must preserve ordering and formatting [::Deepwiki_read_wiki_contents::].
-
Documentation/docs changes:
- Docs updated to state graph now runs via ninja -t graph on a generated build.ninja (Stage 6); roadmap and user guide updated accordingly — runner implementation should match this described flow [::Deepwiki_read_wiki_contents::].
Cited sources:
- [::Deepwiki_read_wiki_contents::] (repository overview, pipeline, runner, manifest, IR, ninja_gen, tests, and docs pages used above)
⏰ 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-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / 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)
🔇 Additional comments (5)
tests/runner_tests.rs (5)
9-9: Import simplification is correct.Removing the unused
ToolNameimport keeps dependencies minimal.
14-15: Good refactoring to share test infrastructure.Extracting
create_test_manifestto the common module follows DRY principles and makes it available for other test files.
125-134: Excellent refactoring that eliminates test duplication.This helper consolidates the repeated pattern from
run_ninja_not_foundandrun_ninja_tool_not_found, directly addressing the CodeScene duplication warning.
136-147: Test correctly refactored to use the shared helper.The closure captures the necessary setup and delegates to
assert_runner_not_found, maintaining the original test behavior whilst eliminating duplication.
306-315: Test correctly refactored, completing the duplication fix.This refactoring, together with
run_ninja_not_found, fully addresses the CodeScene warning about duplicate test structure.
|
@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 file fn run_ninja_tool_not_found() -> Result<()> {
assert_binary_not_found(|| {
let cli = Cli::default();
assert_runner_not_found(|cli| {❌ New issue: Code Duplication |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10108 @leynos: The duplication between Looking at the current state of Current implementation (lines 141-151): #[rstest]
fn run_ninja_not_found() -> Result<()> {
assert_runner_not_found(|cli| {
let targets = BuildTargets::default();
run_ninja(
Path::new("does-not-exist"),
cli,
Path::new("build.ninja"),
&targets,
)
})
}Current implementation (lines 313-321): #[rstest]
fn run_ninja_tool_not_found() -> Result<()> {
assert_runner_not_found(|cli| {
run_ninja_tool(
Path::new("does-not-exist"),
cli,
Path::new("build.ninja"),
"clean",
)
})
}The helper function No further action is required for this duplication issue. ✅ |
…ge and error messages - Enhance test to verify error messages include the specific ninja tool invoked (e.g., clean, graph) when ninja exits with a failure status. - Check presence of build file context in error outputs. - Add doc comments to fixture re-exports for clarity. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| runner_tests.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
1 participant
Summary
ninja -t graphon a temporarybuild.ninja, and outputs DOT to stdout. Supports-jjobs via Ninja.cleanandgraphsubcommands. Thecleansubcommand now delegates to the shared helper, whilegraphuses it for invocation.Changes
handle_graphimplementation.handle_ninja_tool(cli, tool)to share Ninja invocation logic across subcommands, and updatedhandle_cleanto call it with"clean".handle_graphnow delegates tohandle_ninja_tool(cli, "graph").tests/features_unix/graph.featurewith scenarios for invoking ninja with the graph tool, handling failures, respecting jobs, and missing Ninja.tests/runner_tests.rs: Extended with fixtures and tests for graph subcommand, including helpers to ensure behavior with residual files and invalid manifests.tests/steps/process_steps.rs: Added graph-specific fixtures and step implementations, including fixtures for a fake Ninja that expects-t graph(with and without-j), and a run step for the graph subcommand.ninja -t graph).build.ninja.Why
cleanandgraph, and to simplify future command additions.Test plan
cargo test(or your project’s test command).graph.featurescenarios using a fake Ninja that validates-t graphand optional-jflags.Notes
📎 Task: https://www.terragonlabs.com/task/96b3e9d2-cdfb-4186-8612-3fcacac0f015