Adopt structured errors for Ninja generation#152
Conversation
Reviewer's GuideThis PR standardizes error handling using Sequence diagram for error propagation in ninja_gen::generatesequenceDiagram
participant Caller
participant NinjaGen
participant BuildGraph
participant NinjaGenError
Caller->>NinjaGen: generate(graph)
NinjaGen->>BuildGraph: iterate build edges
BuildGraph-->>NinjaGen: edge with action_id
NinjaGen->>NinjaGenError: if action_id missing, return MissingAction
NinjaGen-->>Caller: Result::Err(NinjaGenError::MissingAction)
Note over NinjaGen,Caller: If all actions found, returns Ok(String)
Sequence diagram for error context propagation using anyhowsequenceDiagram
participant Main
participant Manifest
participant NinjaGen
participant NinjaGenError
Main->>Manifest: from_path(path)
Manifest->>Manifest: from_str_named(yaml, name)
Manifest->>NinjaGen: generate(graph)
NinjaGen-->>Manifest: Result<String, NinjaGenError>
Manifest-->>Main: Result with context (anyhow)
Main-->>User: Display error chain with context
Class diagram for runner process module refactorclassDiagram
class CommandArg {
+new(arg: String)
+as_str(): &str
}
class Process {
+run_ninja(program: Path, cli: Cli, build_file: Path, targets: BuildTargets): io::Result<()>
+contains_sensitive_keyword(arg: CommandArg): bool
+is_sensitive_arg(arg: CommandArg): bool
+redact_argument(arg: CommandArg): CommandArg
+redact_sensitive_args(args: [CommandArg]): [CommandArg]
}
Process ..> CommandArg : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughShift error handling from miette to anyhow across core and tests, add Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as CLI
participant Runner as runner::generate_ninja
participant Manifest as manifest::from_path / from_str
participant Graph as BuildGraph::from_manifest
participant NinjaGen as ninja_gen::generate
participant Content as NinjaContent
CLI->>Runner: generate_ninja()
Runner->>Manifest: parse manifest
Manifest-->>Runner: Result<NetsukeManifest>
Runner->>Graph: build graph
Graph-->>Runner: Result<BuildGraph>
Runner->>NinjaGen: generate(&graph)
alt Success
NinjaGen-->>Runner: Ok(ninja_string)
Runner->>Content: NinjaContent::new(ninja_string)
Runner-->>CLI: Ok(NinjaContent)
else MissingAction
NinjaGen-->>Runner: Err(NinjaGenError::MissingAction{id})
Runner-->>CLI: Err(context "generating Ninja file")
else FormatError
NinjaGen-->>Runner: Err(NinjaGenError::Format)
Runner-->>CLI: Err(context "generating Ninja file")
end
sequenceDiagram
autonumber
participant NG as ninja_gen::generate
participant Actions as actions map
participant Edges as edges (sorted, deduped)
NG->>Edges: iterate edges
loop for each edge
NG->>Actions: lookup action by id
alt Found
Actions-->>NG: &Action
NG->>NG: write rule/build/default (propagate fmt errors)
else Missing
Actions-->>NG: None
NG-->>Caller: Err(NinjaGenError::MissingAction{id})
end
end
NG-->>Caller: Ok(ninja_string)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Poem
Poem
Poem
Poem
Poem
Poem
Poem
Pre-merge checks✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/ninja_gen_tests.rs:338` </location>
<code_context>
+ always: false,
+ };
+ graph.targets.insert(PathBuf::from("out"), edge);
+ let err = generate(&graph).expect_err("missing action");
+ assert!(matches!(err, NinjaGenError::MissingAction { id } if id == "missing"));
+}
</code_context>
<issue_to_address>
Consider adding a test for the Format error variant of NinjaGenError.
Only the MissingAction error is tested. Please add a test that triggers and asserts the Format error variant, such as by simulating a formatting failure.
Suggested implementation:
```rust
let err = generate(&graph).expect_err("missing action");
assert!(matches!(err, NinjaGenError::MissingAction { id } if id == "missing"));
}
#[test]
fn generate_format_error() {
use std::fmt;
struct BadDisplay;
impl fmt::Display for BadDisplay {
fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result {
Err(fmt::Error)
}
}
// Simulate a graph/action that will cause a formatting error.
// This assumes your generate function or related code will attempt to format something
// and propagate the error as NinjaGenError::Format.
// You may need to adjust this to fit your actual implementation.
let mut graph = BuildGraph::default();
let edge = BuildEdge {
action_id: "bad_display".into(),
inputs: Vec::new(),
explicit_outputs: vec![PathBuf::from("out")],
implicit_outputs: Vec::new(),
order_only_deps: Vec::new(),
phony: false,
always: false,
};
graph.targets.insert(PathBuf::from("out"), edge);
// Here, you would need to ensure that generate will try to format BadDisplay and fail.
// For demonstration, let's assume generate will fail with Format error for this action_id.
let err = generate(&graph).expect_err("formatting should fail");
assert!(matches!(err, NinjaGenError::Format { .. }));
}
```
If your `generate` function does not currently attempt to format a type that can fail, you may need to:
1. Add logic to your implementation so that an action with id "bad_display" or similar will attempt to format a value of type `BadDisplay`.
2. Ensure that formatting errors are propagated as `NinjaGenError::Format`.
Adjust the test as needed to fit your actual error propagation and formatting logic.
</issue_to_address>
### Comment 2
<location> `src/ninja_gen.rs:45` </location>
<code_context>
-pub fn generate(graph: &BuildGraph) -> String {
+/// Returns [`NinjaGenError`] if a build edge references an unknown action or
+/// writing to the output fails.
+pub fn generate(graph: &BuildGraph) -> Result<String, NinjaGenError> {
let mut out = String::new();
</code_context>
<issue_to_address>
Add unit tests for error cases in the new NinjaGenError enum, especially MissingAction and Format variants.
The new error handling in generate (returning Result and NinjaGenError) is not covered by unit tests for error cases. Add tests that verify MissingAction is returned when an unknown action is referenced, and that Format errors are handled.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/diagnostics.rs (1)
16-18: Gate miette shims behind a feature flag
Verified no.diag(_with)orResultExtusage outside src/diagnostics.rs. Add at the top of that file:#![cfg(any(test, feature = "miette-compat"))]Alternatively delete the entire module if compatibility isn’t required.
tests/yaml_error_tests.rs (1)
70-75: Prefer Display over Debug for stable assertions.
{err:?}output shape can change with backtraces and sources;{err}is typically more stable for text matching.- let msg = normalise_report(&format!("{err:?}")); + let msg = normalise_report(&format!("{err}"));Run the suite to ensure the needles still match:
#!/bin/bash set -euo pipefail cargo test -p netsuke --tests yaml_diagnostics_are_actionabledocs/netsuke-design.md (1)
1292-1341: Clarify miette’s role vs anyhow in the current code.The repo is migrating to
anyhowfor context; tests stopped usingGraphicalReportHandler. Adjust Section 7.2 to state thatmietterendering is optional/transitional and identify which code paths still emit miette-formatted diagnostics.#!/bin/bash # Grep for explicit miette usage to reflect reality in docs. rg -n 'miette' -g '!**/target/**' -C2src/runner.rs (1)
1-434: Split module to satisfy ≤400‑line rule and improve cohesion.Extract the Ninja process plumbing and redaction helpers into
runner/process.rs(or similar). Keeprun(), CLI dispatch, and high-level generation orchestration here.I can draft a minimal split (public API preserved) if desired.
src/manifest.rs (3)
39-41: Replace #[allow] with #[expect] per lint policyDo not silence lints with
#[allow]. Use a narrowly scoped#[expect(..., reason = "...")].Apply:
- #[allow(clippy::useless_conversion, reason = "future-proof span length type")] + #[expect(clippy::useless_conversion, reason = "future-proof span length type")]
1-2: Split this module; file length exceeds 400 linesDecompose into focused submodules (e.g., diagnostics.rs, glob.rs, expand.rs, render.rs) to meet the 400‑line limit and improve cohesion.
145-150: Standardise en‑GB spelling in docs (“normalise” vs “normalize”)Keep docs/comments aligned to en‑GB. Update occurrences in these doc comments.
Apply:
-/// Process a single glob entry and normalize its output. +/// Process a single glob entry and normalise its output. @@ -/// Requires matched paths to be valid UTF-8; output is normalized to use +/// Requires matched paths to be valid UTF-8; output is normalised to use @@ - // Normalize separators so `/` and `\\` behave the same on all platforms. + // Normalise separators so `/` and `\\` behave the same on all platforms.Also applies to: 498-506
tests/ast_tests.rs (1)
294-303: Rename test to en‑GB spellingUse en‑GB in identifiers for consistency with guidelines.
Apply:
-fn actions_behavior( +fn actions_behaviour(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
docs/netsuke-design.md(7 hunks)docs/roadmap.md(1 hunks)docs/snapshot-testing-in-netsuke-using-insta.md(2 hunks)src/diagnostics.rs(1 hunks)src/manifest.rs(8 hunks)src/ninja_gen.rs(4 hunks)src/runner.rs(5 hunks)tests/ast_tests.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/ninja.feature(1 hunks)tests/ninja_gen_tests.rs(5 hunks)tests/ninja_snapshot_tests.rs(1 hunks)tests/steps/ir_steps.rs(3 hunks)tests/steps/ninja_steps.rs(2 hunks)tests/yaml_error_tests.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Comment why, not what; document assumptions, edge cases, trade-offs, and complexity without restating obvious code
Extract reusable logic into functions; prefer composition and declarative constructs when readable
Keep functions small, single-responsibility, and observe command/query separation
Use precise, descriptive names; boolean names should start with is/has/should
Use en-GB-oxendict spelling/grammar in comments (except external API references)
Function docs must include clear usage examples; avoid redundant examples in test docs
No Rust source file may exceed 400 lines; split long switches/tables; move large test data to external files
Fix test warnings in code; do not silence them
Extract helpers when functions are too long; group many parameters into well-named structs
Consider using Arc when returning large errors to reduce data movement
Document public APIs using Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Prefer Result-based error handling over panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate helper functions when conditionals have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect(...)] over #[allow(...)] for lint management
Prefer .expect() over .unwrap()
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Use concat!() to join long string literals rather than using backslash-newline escapes
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums deriving std::error::Error via thiserror for caller-inspectable conditions
Files:
tests/ast_tests.rstests/cucumber.rstests/ninja_snapshot_tests.rssrc/diagnostics.rstests/steps/ir_steps.rstests/steps/ninja_steps.rssrc/runner.rstests/ninja_gen_tests.rssrc/manifest.rstests/yaml_error_tests.rssrc/ninja_gen.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
tests/ast_tests.rstests/cucumber.rstests/ninja_snapshot_tests.rssrc/diagnostics.rstests/steps/ir_steps.rstests/steps/ninja_steps.rssrc/runner.rstests/ninja_gen_tests.rssrc/manifest.rstests/yaml_error_tests.rssrc/ninja_gen.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection (e.g., Env/Clock traits) using mockable crate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/ast_tests.rstests/cucumber.rstests/ninja_snapshot_tests.rstests/steps/ir_steps.rstests/steps/ninja_steps.rstests/ninja_gen_tests.rstests/yaml_error_tests.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Treat docs/ markdown as the reference source of truth for requirements and decisions; keep updated with changes
Use en-GB-oxendict spelling/grammar in documentation (LICENSE name unchanged)
Files:
docs/roadmap.mddocs/snapshot-testing-in-netsuke-using-insta.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Wrap Markdown paragraphs and bullet points at 80 columns; do not wrap tables or headings
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Validate Mermaid diagrams in Markdown using make nixie
Files:
docs/roadmap.mddocs/snapshot-testing-in-netsuke-using-insta.mddocs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- 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/snapshot-testing-in-netsuke-using-insta.mddocs/netsuke-design.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
In fenced code blocks for docs, explicitly mark code fences with rust (```rust) for clarity
Files:
docs/roadmap.mddocs/snapshot-testing-in-netsuke-using-insta.mddocs/netsuke-design.md
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.rs: Every module must begin with a //! module-level comment explaining its purpose and utility
Never export eyre::Report from library code; convert to domain error enums at API boundaries
Files:
src/diagnostics.rssrc/runner.rssrc/manifest.rssrc/ninja_gen.rs
🧬 Code graph analysis (5)
tests/ninja_snapshot_tests.rs (1)
src/ninja_gen.rs (1)
generate(45-86)
tests/steps/ir_steps.rs (1)
src/ir.rs (1)
from_manifest(122-133)
tests/steps/ninja_steps.rs (1)
src/ninja_gen.rs (1)
generate(45-86)
src/runner.rs (2)
src/ir.rs (1)
from_manifest(122-133)src/ninja_gen.rs (1)
generate(45-86)
tests/ninja_gen_tests.rs (1)
src/ninja_gen.rs (1)
generate(45-86)
🪛 GitHub Actions: CI
tests/steps/ir_steps.rs
[warning] 1-1: Rust fmt diff detected in netsuke/tests/steps/ir_steps.rs:1. Run 'cargo fmt' to fix formatting.
[warning] 51-51: Rust fmt diff detected in netsuke/tests/steps/ir_steps.rs:51. Run 'cargo fmt' to fix formatting.
src/manifest.rs
[warning] 549-556: Rust fmt diff detected in netsuke/src/manifest.rs. Run 'cargo fmt' to fix formatting.
🪛 GitHub Check: build-test (1.89.0)
src/manifest.rs
[warning] 549-549:
Diff in /home/runner/work/netsuke/netsuke/src/manifest.rs
🪛 GitHub Check: build-test (stable)
src/manifest.rs
[warning] 549-549:
Diff in /home/runner/work/netsuke/netsuke/src/manifest.rs
🔇 Additional comments (24)
docs/netsuke-design.md (1)
1349-1352: Good: DocumentNinjaGenError::MissingActionas an actionable generation error.The doc matches the new generator API and makes the failure mode explicit.
tests/steps/ir_steps.rs (3)
5-5: LGTM: switch to anyhow::Context.The step now composes error context consistently with the rest of the PR.
53-58: LGTM: propagate crisp context for IR build failures.
with_context(|| format!(...))provides actionable path info.
86-91: Fix rustfmt warnings.The CI warns about formatting at Line 1 and Line 51. Run
cargo fmt.#!/bin/bash set -euo pipefail cargo fmt --all git diff --name-only --exit-code || (echo "Unformatted files remain"; exit 1)src/runner.rs (4)
9-9: LGTM: unify on anyhow::{Context, Result}.Imports align with the migration and simplify error flow.
193-202: LGTM: surface temp-file creation errors via context.This keeps failure points obvious.
215-226: LGTM: add precise write paths to error messages.The
with_contextclosures make file I/O failures actionable.
255-257: LGTM: handleninja_gen::generatefallibly and returnNinjaContent.This matches the new
NinjaGenErrorpath.src/manifest.rs (1)
549-556: Fix rustfmt drift flagged by CIFormat the file to clear the pipeline warning.
Run:
#!/bin/bash set -euo pipefail cargo fmt --all -- --checktests/ast_tests.rs (1)
3-3: LGTM: switch tests to anyhow::ResultThe import aligns tests with the new anyhow-based API.
tests/features/ninja.feature (1)
25-29: LGTM: behaviour scenario covers actionable error pathThe new scenario exercises the MissingAction path without panicking.
tests/ninja_snapshot_tests.rs (1)
43-43: LGTM!The test correctly handles the new
Resultreturn type fromninja_gen::generate.docs/roadmap.md (1)
141-154: Excellent completion of Phase 3 error reporting!The actionable error reporting phase has been successfully completed with comprehensive implementation of structured error types and contextual error propagation. The adoption of
anyhowandthiserrorprovides a robust foundation for clear, actionable error messages throughout the codebase.tests/steps/ninja_steps.rs (2)
13-22: Well-structured error handlingThe match expression properly handles both success and error cases from the new
Result-based API, maintaining clear separation between the ninja content and error state.
71-82: Comprehensive error assertionThe new test step provides good coverage for verifying error messages when Ninja generation fails, completing the error path testing.
tests/ninja_gen_tests.rs (3)
11-11: Good public API exposureExplicitly importing
NinjaGenErroralongsidegeneratemakes the error type part of the public API, enabling proper error handling by consumers.
134-135: Consistent error handling across testsAll test calls to
generateconsistently use.expect("generate ninja")to handle theResulttype, maintaining uniform error handling throughout the test suite.Also applies to: 141-142, 175-176, 302-303
325-340: Excellent error path test coverageThe test thoroughly validates the
MissingActionerror case, ensuring the error type and message are correct when an edge references a non-existent action.src/ninja_gen.rs (6)
13-21: Well-designed error typeThe
NinjaGenErrorenum provides clear, actionable error variants with descriptive messages. The use ofthiserrorfor derivingErrorand automatic conversion fromfmt::Errorfollows best practices.
39-45: Clear API documentationThe function documentation properly describes the error conditions and return type, making the API contract explicit for consumers.
54-61: Effective deduplication logicThe edge sorting and deduplication using
path_keyensures deterministic output whilst preventing duplicate build statements. The implementation correctly maintains insertion order for the first occurrence.
62-68: Robust error handling for missing actionsThe explicit check for missing actions with a descriptive error message provides clear feedback when the build graph is inconsistent. The error includes the action ID for easy debugging.
51-51: Consistent error propagationAll write operations properly use the
?operator for error propagation, ensuring formatting errors are captured and returned asNinjaGenError::Format.Also applies to: 76-76, 82-82
211-211: Test properly updated for new APIThe test correctly handles the
Resultreturn type with appropriate error expectation.
|
@coderabbitai Re:
Can we make it clear that miette must be used for presentation of YAML / Jinja diagnostics? Please suggest an AI prompt to achieve this |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Based on my analysis of the codebase, I can see that while the PR migrates most error handling from miette to anyhow, there are still specific areas where miette diagnostics are essential for presenting structured error information, particularly for YAML parsing errors. Here's an AI prompt you can use to clarify miette's continued role for YAML/Jinja diagnostics: This prompt will help ensure the documentation clearly establishes miette's ongoing role in diagnostic presentation while acknowledging anyhow's role in error context management. 🏁 Script executed: Length of output: 4541 🏁 Script executed: Length of output: 472 🏁 Script executed: Length of output: 3083 Based on my analysis of the codebase, I can now provide you with a more targeted AI prompt. I can see that the code already has a good foundation with Here's an AI prompt tailored to your actual codebase structure: This prompt will help ensure your documentation clearly establishes that miette is essential for user-facing error presentation, especially following the 🐰 ears perked up with curiosity, having learned something valuable about error handling patterns ✏️ Learnings added
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/manifest.rs (2)
1-1: Split this module; file exceeds 400-line limit.Decompose into focused submodules (e.g., yaml_diag.rs, jinja_helpers.rs, glob.rs) to meet the 400‑line cap and lower complexity.
Say the word and a patch splitting out:
- YAML diagnostics (YamlDiagnostic, map_yaml_error, to_span, hints)
- Jinja helpers (env_var, render_*, eval_expression)
- Glob normalisation/validation
will be generated with updatedmodwiring and visibility.
39-41: Replace forbidden #[allow] with #[expect].Replace the attribute in src/manifest.rs; replace remaining #[allow(...)] occurrences in tests/steps/ninja_steps.rs (lines 25, 38, 62, 71) and src/diagnostics.rs (line 32) with narrowly scoped #[expect(...)] and include a reason.
File: src/manifest.rs (lines 39-41)
- #[allow(clippy::useless_conversion, reason = "future-proof span length type")] + #[expect(clippy::useless_conversion, reason = "future-proof span length type")] SourceSpan::new(start.into(), len.into())
♻️ Duplicate comments (4)
tests/steps/ir_steps.rs (1)
91-97: Fix nondeterministic action removal in step.HashMap iteration order is nondeterministic; delete the action referenced by the first target to produce stable failures. This was flagged previously.
Apply:
fn remove_action(world: &mut CliWorld) { let graph = world.build_graph.as_mut().expect("graph"); - if let Some(id) = graph.actions.keys().next().cloned() { - graph.actions.remove(&id); - } + let first_action = graph + .targets + .values() + .next() + .map(|t| t.action_id.clone()); + if let Some(id) = first_action { + graph.actions.remove(&id); + } }src/manifest.rs (1)
531-533: Unify YAML parse errors under ManifestError::Parse.Return the same domain error for both YAML decoding phases to keep caller-inspectable errors stable. This was requested earlier and remains unaddressed.
- let mut doc: YamlValue = - serde_yml::from_str(yaml).map_err(|e| map_yaml_error(e, yaml, name))?; + let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| ManifestError::Parse { + source: map_yaml_error(e, yaml, name), + })?;Also applies to: 552-556
tests/ninja_gen_tests.rs (2)
324-340: Cover MissingAction variant with a focused unit test.Exercise
NinjaGenError::MissingActionwith precise pattern matching. This addresses prior feedback to test error cases.
342-379: Exercise Format variant by forcing a writer failure.Inject a failing
fmt::Writeto assertNinjaGenError::Format. This directly satisfies the earlier request to add a formatting error test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/srgn.md(3 hunks)src/manifest.rs(8 hunks)src/ninja_gen.rs(4 hunks)tests/ninja_gen_tests.rs(5 hunks)tests/steps/ir_steps.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use en-GB-oxendict spelling and grammar in code comments (exceptions allowed for external API names)
Function documentation must include clear examples; test documentation should omit redundant examples
Keep file size manageable: no Rust source file longer than 400 lines; split large switches/dispatch tables and move big test data to external files
Disallow Clippy warnings; fix warnings in code rather than silencing them
Extract helper functions for long functions; maintain separation of concerns and CQRS
Group many related parameters into meaningful structs
If returning a large error type, consider using Arc to reduce data copied/returned
Each Rust module must begin with a module-level //! comment stating purpose and utility
Document public APIs with Rustdoc (///) so cargo doc can generate documentation
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions when conditional criteria have more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer expect over allow for lint management
Use conditional compilation (#[cfg]/#[cfg_attr]) for functions unused under specific feature sets
Prefer .expect() over .unwrap()
Use concat!() for long string literals instead of escaping newlines with backslashes
Prefer single-line function bodies when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer semantic error enums (derive std::error::Error via thiserror) for inspectable conditions
Use an opaque eyre::Report only at the application boundary for human-readable logs; do not expose in public APIs
Never export eyre::Report from libraries; convert to domain error enums at API boundaries and to eyre only in main/top-level async tas...
Files:
src/ninja_gen.rssrc/manifest.rstests/ninja_gen_tests.rstests/steps/ir_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and the
mockablecrate are the preferred option.- If mockable cannot be used, env mutations in tests ...
Files:
src/ninja_gen.rssrc/manifest.rstests/ninja_gen_tests.rstests/steps/ir_steps.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Write unit and behavioural tests for new functionality; run before and after changes
Use rstest fixtures for shared setup and replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs
Mock non-deterministic dependencies via dependency injection using mockable (traits like Env and Clock) where appropriate; follow docs/reliable-testing-in-rust-via-dependency-injection.md
Files:
tests/ninja_gen_tests.rstests/steps/ir_steps.rs
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
In fenced code blocks for docs, explicitly mark code fences with rust (```rust) for clarity
Files:
docs/srgn.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architecture
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation current
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)
Files:
docs/srgn.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown files using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns; code blocks at 120; do not wrap tables or headings
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references
Files:
docs/srgn.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -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/srgn.md
🧬 Code graph analysis (2)
tests/ninja_gen_tests.rs (1)
src/ninja_gen.rs (4)
generate(45-49)generate_into(56-95)fmt(133-160)fmt(170-185)
tests/steps/ir_steps.rs (1)
src/ir.rs (1)
from_manifest(122-133)
🔍 Remote MCP Deepwiki
Based on my search of the netsuke repository, I can see this is a pull request that introduces structured error handling with anyhow and a NinjaGenError type. Let me provide a comprehensive summary of the relevant context I've found:
Technical Context for PR Review
Error Handling Migration
This PR implements a migration from miette-heavy error propagation to anyhow for contextual error handling throughout the codebase. The key changes include:
- Structured Error Types: Introduction of
NinjaGenErrorwith variantsMissingActionandFormatfor actionable Ninja generation failures - API Changes: The
ninja_gen::generate()function now returnsResult<String, NinjaGenError>instead of panicking, with a newgenerate_into()function for writing to external writers - Error Context Migration: Replacement of custom
ResultExttrait and.diag()methods with standardanyhow::Contextand.with_context()throughout runner, manifest, and testing modules
Security and Command Handling
The codebase implements significant security measures:
- Automatic Shell Escaping: All command interpolation uses the
shell-quotecrate to prevent command injection attacks - Template Sandboxing: Jinja templates are processed in a controlled environment with whitelisted functions (
env(),glob()) - Command Validation: Generated commands are validated with
shlex::split()before output
Architecture and Testing
The system follows a five-stage compilation pipeline:
- Manifest ingestion
- Jinja template evaluation
- YAML parsing into AST
- IR generation and validation
- Ninja synthesis and execution
The testing strategy uses three frameworks:
- rstest: Fixture-based unit testing
- cucumber-rs: BDD integration testing
- insta: Snapshot testing for IR and Ninja output validation
Critical Review Points
Error Handling Strategy: The migration maintains a two-tier architecture where miette remains essential for user-facing diagnostic presentation (especially for YAML and Jinja errors) while anyhow handles internal context propagation. This separation ensures rich diagnostics aren't lost.
API Breaking Changes: The ninja_gen::generate function signature change from returning String to Result<String, NinjaGenError> is a breaking change that requires updates to all calling code, which the PR addresses comprehensively.
Test Coverage: The PR includes extensive test updates, including new Cucumber scenarios for missing action error paths and proper handling of the new Result-returning APIs in snapshot tests.
Documentation Updates: The roadmap is updated to reflect completion of the "Actionable Error Reporting" phase, and design documentation is updated to reflect the new error handling approach.
This appears to be a well-structured refactoring that improves error handling ergonomics while maintaining the system's security guarantees and deterministic build properties.
🔇 Additional comments (14)
tests/steps/ir_steps.rs (3)
4-4: LGTM: adopt anyhow::Context in steps.Import the correct trait for
.context(...)and.with_context(...).
54-56: LGTM: context layering is correct.Chain
from_path(...).and_then(...).with_context(...)to keep a single error type and preserve context.
87-88: LGTM: assertion message formatting.The trailing comma is fine and keeps rustfmt happy.
src/manifest.rs (1)
766-771: LGTM: file IO context message.Surface a precise path in the error;
.with_contextusage is correct.tests/ninja_gen_tests.rs (5)
11-11: Adopt the new public API in tests.Import
NinjaGenError,generate, andgenerate_intoto align tests with the Result-returning API. LGTM.
134-136: Handle Result explicitly in unit tests.Use
.expect("generate ninja")to fail loudly on generation errors. LGTM.
141-143: Assert empty output for empty graph via Result-aware call.The change to
.expect("generate ninja")is correct and consistent.
175-188: Snapshot the multiline script output via Result-aware call.Use of
.expect("generate ninja")and snapshot assertions is correct. LGTM.
302-303: Write generated Ninja to file after explicit Result handling.Call
.expect("generate ninja")before persisting to disk. LGTM.src/ninja_gen.rs (5)
15-21: Introduce a domain error type for Ninja generation.Define
NinjaGenErrorwithMissingActionandFormat(#[from] fmt::Error). Error messages are actionable and integrate cleanly with?. LGTM.
60-61: Propagate formatting failures viaFormatvariant.Use
?onwrite!forNamedActionto surfacefmt::ErrorasNinjaGenError::Format. LGTM.
71-77: ReturnMissingActioninstead of panicking.Replace implicit unwrap with an explicit, inspectable error. This aligns with the structured error strategy. LGTM.
91-91: Propagate write failure when emitting defaults.Use
writeln!with?to keep error flow consistent. LGTM.
220-228: Update internal unit test to the Result-returning API.Use
.expect("generate ninja")and compare against the expected string. LGTM.
|
@coderabbitai please create new GitHub issues for the following: |
|
Tip For best results, initiate chat on the files or code changes. @leynos I'd be happy to create GitHub issues for you! However, your comment appears to be incomplete - it ends with "for the following:" but doesn't specify what items you'd like me to create issues for. Could you please provide the specific topics, problems, or tasks you'd like me to create GitHub issues for? Once you share those details, I'll create the issues using the GitHub CLI with appropriate titles and descriptions. |
|
@coderabbitai please create a new GitHub issue for the following: src/runner.rs (1) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a GitHub issue for the runner.rs refactoring task. The GitHub issue has been created to track the refactoring of 🏁 Script executed: Length of output: 44 Perfect! I've successfully created GitHub issue #155 for refactoring
The issue is now available at: #155 🐰 satisfied tail wiggle after successfully organizing the refactoring task |
|
@coderabbitai "normalize" is correct for en-GB-oxendict as used in this project: """ """ |
|
Tip For best results, initiate chat on the files or code changes. @leynos Thank you for the correction! I appreciate you clarifying that "normalize" is indeed the correct spelling for this project's en-GB-oxendict standard. I'll make sure to follow the project's established spelling conventions going forward. 🐰 ears drooping slightly in acknowledgment of the correction, then perking up with newfound understanding ✏️ Learnings added
|
|
@coderabbitai please create a new GitHub issue for the following: src/manifest.rs: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a GitHub issue for refactoring 🐰 focused concentration while organizing the module splitting task 🏁 Script executed: Length of output: 44 Perfect! I've successfully created GitHub issue #156 for refactoring
The issue is now available at: #156 🐰 pleased ear twitch after successfully organizing another module refactoring task |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai new GitHub issue, please: src/manifest.rs (2) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create a new GitHub issue for refactoring 🏁 Script executed: Length of output: 44 Perfect! I've successfully created GitHub issue #157 for refactoring
The issue is now available at: #157 🐰 satisfied ear twitch after precisely organizing the module refactoring with your exact specifications |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/manifest.rs:533` </location>
<code_context>
///
/// Returns an error if YAML parsing or Jinja evaluation fails.
fn from_str_named(yaml: &str, name: &str) -> Result<NetsukeManifest> {
- let mut doc: YamlValue =
- serde_yml::from_str(yaml).map_err(|e| Report::new(map_yaml_error(e, yaml, name)))?;
+ let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| ManifestError::Parse {
+ source: map_yaml_error(e, yaml, name),
+ })?;
</code_context>
<issue_to_address>
Error mapping now uses ManifestError::Parse, but map_yaml_error may return a type not compatible with ManifestError.
Ensure that map_yaml_error returns a type compatible with ManifestError::Parse's source field to avoid type mismatches and preserve error details.
Suggested implementation:
```rust
fn from_str_named(yaml: &str, name: &str) -> Result<NetsukeManifest> {
let mut doc: YamlValue = serde_yml::from_str(yaml).map_err(|e| ManifestError::Parse {
source: map_yaml_error(e, yaml, name),
})?;
```
```rust
source: map_yaml_error(e, yaml, name),
```
```rust
use std::error::Error;
use std::sync::Arc;
// Update map_yaml_error to return a boxed error compatible with ManifestError::Parse
fn map_yaml_error(e: serde_yml::Error, yaml: &str, name: &str) -> Box<dyn Error + Send + Sync> {
// You can wrap the error with context using anyhow or custom error type
// For example, using anyhow:
use anyhow::{anyhow, Context};
let msg = format!("YAML parse error in '{}': {}", name, e);
anyhow!(msg).context(format!("YAML content:\n{}", yaml)).into()
}
```
- You may need to update the definition of `ManifestError::Parse` to ensure its `source` field is of type `Box<dyn Error + Send + Sync>`, or another compatible type.
- If other usages of `map_yaml_error` exist, update them to handle the new return type.
- If you use a custom error type instead of `anyhow`, adjust the implementation accordingly.
</issue_to_address>
### Comment 2
<location> `src/runner/process.rs:130` </location>
<code_context>
-/// # Panics
-///
-/// Panics if the child's output streams cannot be captured.
-pub fn run_ninja(
- program: &Path,
- cli: &Cli,
</code_context>
<issue_to_address>
Consider refactoring run_ninja into three focused helper functions to simplify its structure and improve readability.
```markdown
You can dramatically reduce the cognitive load in `run_ninja` by pulling out three focused helpers:
1. **Command‐builder** – assemble args, cwd, jobs, build file
2. **Command‐logger** – redact & log the final invocation
3. **Output‐streamer** – spawn threads to pump `stdout`/`stderr`, and return when both finish
Then `run_ninja` becomes a simple orchestrator:
```rust
pub fn run_ninja(
program: &Path,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<()> {
let mut cmd = build_ninja_command(program, cli, build_file, targets)?;
log_command(&cmd);
let mut child = cmd.spawn()?;
let (out_handle, err_handle) = stream_output(child.stdout.take().unwrap(), child.stderr.take().unwrap());
let status = child.wait()?;
out_handle.join().ok();
err_handle.join().ok();
if status.success() {
Ok(())
} else {
Err(io::Error::new(
io::ErrorKind::Other,
format!("ninja exited with {status}"),
))
}
}
```
And the helpers might look like:
```rust
fn build_ninja_command(
program: &Path,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets<'_>,
) -> io::Result<Command> {
let mut cmd = Command::new(program);
if let Some(d) = &cli.directory {
cmd.current_dir(fs::canonicalize(d)?);
}
if let Some(j) = cli.jobs {
cmd.args(&["-j", &j.to_string()]);
}
let bf = build_file.canonicalize().unwrap_or_else(|_| build_file.to_path_buf());
cmd.args(&["-f", bf.to_str().unwrap()]);
cmd.args(targets.as_slice());
cmd.stdout(Stdio::piped()).stderr(Stdio::piped());
Ok(cmd)
}
fn log_command(cmd: &Command) {
let program = cmd.get_program().to_string_lossy();
let args = cmd
.get_args()
.map(|a| CommandArg::new(a.to_string_lossy().into_owned()))
.collect::<Vec<_>>();
let redacted = redact_sensitive_args(&args);
let joined = redacted.iter().map(|a| a.as_str()).join(" ");
info!("Running command: {program} {joined}");
}
fn stream_output(
stdout: impl Read + Send + 'static,
stderr: impl Read + Send + 'static,
) -> (thread::JoinHandle<()>, thread::JoinHandle<()>) {
let out = thread::spawn(move || {
BufReader::new(stdout).lines().for_each(|l| {
if let Ok(line) = l {
println!("{line}");
}
})
});
let err = thread::spawn(move || {
BufReader::new(stderr).lines().for_each(|l| {
if let Ok(line) = l {
eprintln!("{line}");
}
})
});
(out, err)
}
```
This keeps all functionality, but splits the large `run_ninja` into three self‐contained pieces, each trivially testable and understandable in isolation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
force-pushed
the
codex/implement-actionable-error-reporting
branch
from
e60ad53 to
8c4c456
Compare
1 participant
Summary
anyhowfor contextual error propagation in runner and manifestNinjaGenErrorfor actionable Ninja generation failuresTesting
make fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68c0a537c8648322b451b857125d22df
Summary by Sourcery
Adopt structured error handling for Ninja generation and manifest parsing by introducing NinjaGenError, propagating errors with anyhow’s context methods, standardizing diagnostics with thiserror and miette, restructuring runner logic into a process module, and updating tests and documentation to cover and reflect these changes
New Features:
Enhancements:
Documentation:
Tests:
Chores: