Refactor cycle detector to own traversal paths#164
Conversation
|
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
WalkthroughReplace all public PathBuf path types with camino::Utf8PathBuf across IR and Ninja generation, and refactor cycle detection into an owned CycleDetector that returns canonicalised Utf8PathBuf cycles and records missing dependency pairs alongside cycle data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant CycleDetector
participant Targets as Targets (HashMap<Utf8PathBuf, BuildEdge>)
participant Canon as canonicalize_cycle
Caller->>CycleDetector: analyse(&Targets)
CycleDetector->>Targets: iterate cloned Utf8PathBuf keys
loop for each unvisited target
CycleDetector->>CycleDetector: visit(node: Utf8PathBuf)
CycleDetector->>Targets: lookup edge.inputs (Utf8PathBuf)
alt dep present in Targets
CycleDetector->>CycleDetector: recurse visit(dep: Utf8PathBuf)
else dep missing
CycleDetector->>CycleDetector: record missing_dependencies (node, dep)
CycleDetector->>Caller: emit tracing::warn! (missing dep)
end
alt back-edge detected
CycleDetector->>CycleDetector: slice stack to assemble cycle (Utf8PathBuf...)
CycleDetector->>Canon: canonicalize_cycle(Vec<Utf8PathBuf>)
Canon-->>CycleDetector: canonical cycle
CycleDetector-->>Caller: return CycleDetectionReport { cycle, missing_dependencies }
else continue traversal
CycleDetector-->>CycleDetector: mark Visited
end
end
CycleDetector-->>Caller: return CycleDetectionReport (no cycle)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
Reviewer's GuideRefactors the cycle detection logic to use owned PathBuf traversal state with a unified visit method, simplifies the cycle canonicalization, augments targeted tests for cycle detection behavior, and updates documentation to reflect the new design. Class diagram for refactored CycleDetector and related typesclassDiagram
class CycleDetector {
- targets: &HashMap<PathBuf, BuildEdge>
- stack: Vec<PathBuf>
- states: HashMap<PathBuf, VisitState>
+ new(targets: &HashMap<PathBuf, BuildEdge>)
+ is_visited(node: &PathBuf): bool
+ visit(node: PathBuf): Option<Vec<PathBuf>>
+ find_cycle(targets: &HashMap<PathBuf, BuildEdge>): Option<Vec<PathBuf>>
}
class VisitState {
<<enum>>
Visiting
Visited
}
class BuildEdge {
action_id: String
inputs: Vec<PathBuf>
explicit_outputs: Vec<PathBuf>
implicit_outputs: Vec<PathBuf>
order_only_deps: Vec<PathBuf>
phony: bool
always: bool
}
CycleDetector --> "1" BuildEdge
CycleDetector --> "*" VisitState
BuildEdge --> "*" PathBuf
CycleDetector "stack" --> "*" PathBuf
CycleDetector "states" --> "*" PathBuf
Flow diagram for unified cycle detection traversalflowchart TD
A["Start traversal for each node in targets"] --> B["Check if node is visited"]
B -- Yes --> C["Skip node"]
B -- No --> D["Call visit(node)"]
D --> E["Mark node as Visiting"]
E --> F["Push node onto stack"]
F --> G["For each dependency in edge.inputs"]
G --> H["If dependency not in targets, continue"]
G --> I["Call visit(dep)"]
I -- Cycle found --> J["Return cycle"]
I -- No cycle --> K["Continue"]
G --> L["Pop node from stack"]
L --> M["Mark node as Visited"]
M --> N["End"]
File-Level Changes
Possibly linked issues
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.
![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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/ir.rs:536` </location>
<code_context>
- return Some(cycle);
+ if let Some(edge) = self.targets.get(&node) {
+ for dep in &edge.inputs {
+ if !self.targets.contains_key(dep) {
+ continue;
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Skipping missing dependencies may hide graph inconsistencies.
Logging or tracking missing dependencies would help identify and resolve potential graph integrity issues.
Suggested implementation:
```rust
for dep in &edge.inputs {
if !self.targets.contains_key(dep) {
// Log or track missing dependency for graph integrity diagnostics
eprintln!(
"Warning: Missing dependency '{}' for node '{}'.",
dep, node
);
continue;
}
```
If you want to track missing dependencies instead of just logging, you could add a `missing_dependencies: Vec<(Node, Node)>` field to your struct and push `(node, dep)` to it here. You may also want to replace `eprintln!` with your project's preferred logging mechanism.
</issue_to_address>
### Comment 2
<location> `src/ir.rs:475` </location>
<code_context>
}
-#[derive(Clone, Copy)]
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum VisitState {
Visiting,
</code_context>
<issue_to_address>
**issue (review_instructions):** The attribute #[derive(...)] is placed before any doc comment, but there is no doc comment present for the VisitState enum.
Please add a doc comment above the VisitState enum and ensure that the attribute is placed after the doc comment, as per the instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Place function attributes after doc comments.
</details>
</issue_to_address>
### Comment 3
<location> `src/ir.rs:481` </location>
<code_context>
- }
-}
-
/// Detects cycles in a dependency graph by tracking traversal state.
struct CycleDetector<'a> {
targets: &'a HashMap<PathBuf, BuildEdge>,
</code_context>
<issue_to_address>
**issue (review_instructions):** The module does not begin with a //! comment as required.
Please add a //! module-level comment at the top of src/ir.rs to describe the module's purpose.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Every module must begin with a //! comment.
</details>
</issue_to_address>
### Comment 4
<location> `docs/netsuke-design.md:1103` </location>
<code_context>
- references from the `targets` map, so `targets` must remain unchanged during
- detection.
+ type owns the recursion stack and visitation map, allowing the traversal
+ functions to remain focused and easily testable. Nodes are stored as owned
+ `PathBuf`s, so the detector can clone keys from `targets` without mutating
+ the map during detection.
</code_context>
<issue_to_address>
**issue (review_instructions):** This paragraph line exceeds the 80 column wrapping limit for prose.
Please wrap this paragraph line to 80 columns to comply with documentation formatting standards.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>
### Comment 5
<location> `docs/netsuke-design.md:1104` </location>
<code_context>
- detection.
+ type owns the recursion stack and visitation map, allowing the traversal
+ functions to remain focused and easily testable. Nodes are stored as owned
+ `PathBuf`s, so the detector can clone keys from `targets` without mutating
+ the map during detection.
</code_context>
<issue_to_address>
**issue (review_instructions):** This paragraph line exceeds the 80 column wrapping limit for prose.
Please wrap this paragraph line to 80 columns for consistency with documentation formatting guidelines.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Paragraphs and bullets must be wrapped to 80 columns
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(1 hunks)src/ir.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the docs/ markdown files as the reference knowledge base and update them proactively when decisions or requirements change
Files:
docs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar (LICENSE name excluded)
Validate Markdown with make markdownlint and run make fmt to format Markdown (including fixing table markup)
Validate Mermaid diagrams in Markdown by running make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Do not wrap tables and headings in Markdown
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/netsuke-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/netsuke-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
src/ir.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/ir.rs
🔍 Remote MCP Deepwiki
Additional repository facts to help review PR #164 (CycleDetector ownership refactor)
- IR generation performs cycle detection as part of IR validation (BuildGraph::from_manifest → cycle detection). Review impact of the CycleDetector change on that validation path.
- BuildGraph.targets uses PathBuf keys (HashMap<PathBuf, BuildEdge>); owning PathBufs in the detector aligns with the IR key types and avoids lifetime coupling.
- Public IR types exported by the crate include BuildGraph, Action, BuildEdge and IrGenError — the repo docs show these as the public IR API (PR notes no signature changes). Confirm no accidental API break.
- Tests and BDD features covering IR behavior live under tests/features/ir.feature and tests/steps/ir_steps.rs; the project uses rstest + cucumber + insta snapshot tooling — ensure PR updates corresponding unit/BDD tests and snapshots.
- Repo CI / developer workflow enforces formatting, linting and tests (make fmt, make check-fmt, make lint, make test, make markdownlint). Run these as part of review.
- Design docs (docs/netsuke-design.md) include the IR description and cycle-detection rationale; the PR updates that doc — verify doc wording matches the implementation semantics.
Files to pay special attention to in the PR: src/ir.rs, docs/netsuke-design.md, tests/* (IR tests & snapshots).
⏰ 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). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (10)
docs/netsuke-design.md (1)
1102-1105: LGTM! The ownership refactor improves design clarity.The updated documentation correctly describes the
CycleDetectorowning its traversal state withPathBufs. This ownership model eliminates lifetime coupling and aligns well with theBuildGraph.targetskey type.src/ir.rs (9)
28-28: LGTM! Clean import simplification.Removing
Pathfrom the imports while retainingPathBufreduces the API surface and enforces ownership semantics consistently.
475-475: LGTM! Appropriate trait derivations for state tracking.The addition of
Clone, Copy, Eqtraits toVisitStateis well-justified for the HashMap-based cycle detection implementation.
484-485: LGTM! Ownership model aligns with IR design.Storing
Vec<PathBuf>for the stack andHashMap<PathBuf, VisitState>for states eliminates lifetime constraints and matches theBuildGraph.targetskey type.
514-530: Solid cycle detection with clear state transitions.The
visitmethod correctly handles the three visitation states:
Visited: Skip (no cycle)Visiting: Cycle detected (extract and canonicalise)None: Mark as visiting and recurseThe use of
expectat line 522 is appropriate as the invariant is guaranteed by the state machine.
534-544: LGTM! Clean traversal logic with proper filtering.The traversal correctly:
- Skips non-target dependencies (external files)
- Recursively visits each input dependency
- Clones the
PathBuffor ownership transfer
567-571: Elegant canonicalisation using split_at_mut.The refactored rotation logic using
split_at_mutis more idiomatic than the previous implementation. The careful rebinding of the first element to close the cycle maintains correctness.
580-594: Well-structured test helpers.The
pathandbuild_edgehelpers improve test readability by abstracting thePathBufconstruction and providing sensible defaults forBuildEdgefields.
596-603: Good edge case coverage for self-cycles.Testing self-referential cycles ensures the detector handles this degenerate case correctly.
619-627: Clear test for basic cycle detection.The test properly validates a simple two-node cycle with the expected canonicalised output.
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ir.rs (2)
100-114: Keep IR error types aligned with UTF‑8 paths.If migrating to camino, ensure
IrGenError::CircularDependencycarriesVec<camino::Utf8PathBuf>to preserve UTF‑8 guarantees end‑to‑end.
206-211: Surface missing dependencies diagnostics when failing cycles (optional).Cycle detection now logs missing inputs. Optionally enrich
CircularDependencyerrors with any recordedmissing_dependenciesto aid debugging.#!/bin/bash # Grep callsites where IrGenError::CircularDependency is rendered to users. rg -nP 'CircularDependency\s*\{' -C3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
docs/netsuke-design.md(1 hunks)docs/reliable-testing-in-rust-via-dependency-injection.md(3 hunks)docs/rust-doctest-dry-guide.md(1 hunks)src/ir.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the docs/ markdown files as the reference knowledge base and update them proactively when decisions or requirements change
Files:
docs/rust-doctest-dry-guide.mddocs/netsuke-design.mddocs/reliable-testing-in-rust-via-dependency-injection.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar (LICENSE name excluded)
Validate Markdown with make markdownlint and run make fmt to format Markdown (including fixing table markup)
Validate Mermaid diagrams in Markdown by running make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Do not wrap tables and headings in Markdown
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/rust-doctest-dry-guide.mddocs/netsuke-design.mddocs/reliable-testing-in-rust-via-dependency-injection.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/rust-doctest-dry-guide.mddocs/netsuke-design.mddocs/reliable-testing-in-rust-via-dependency-injection.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
src/ir.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/ir.rs
docs/reliable-testing-in-rust-via-dependency-injection.md
📄 CodeRabbit inference engine (AGENTS.md)
Follow the project’s guide for reliable testing in Rust via dependency injection
Files:
docs/reliable-testing-in-rust-via-dependency-injection.md
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
src/ir.rs (10)
476-482: LGTM: VisitState derives are appropriate.Deriving
Debug, Eq, PartialEqenables straightforward assertions in tests and simplifies state comparisons.
483-490: Scope the diagnostics buffer to tests only.
missing_dependenciesis retained in the struct; exposure is test‑only which is correct. Keep it private to avoid accidental API surface growth.
518-559: LGTM: DFS with back‑edge detection and canonicalisation is correct.The stack/visited handling, back‑edge cycle extraction, and canonicalisation produce deterministic cycles. The self‑edge path is handled.
567-584: LGTM: Canonical rotation viasplit_at_mutis tight and safe.The rotation excludes the closing node and then mirrors the first to the terminal slot, preserving cycle closure.
607-614: LGTM: Self‑edge test asserts exact 2‑node cycle.Test covers the corner case and validates canonicalisation.
616-633: LGTM: Visitation and stack cleanup assertions are solid.The test ensures both nodes end in the
Visitedstate and the stack is empty post‑traversal.
634-644: LGTM: Missing dependency recording verified.The unit test validates diagnostics capture without failing the traversal.
645-653: LGTM: Two‑node cycle detection path.Deterministic cycle ordering is asserted.
655-661: LGTM: Canonicalisation rotates smallest node to front.Expected canonical form is asserted.
663-669: LGTM: Reverse‑direction cycle canonicalises deterministically.Test ensures consistent canonical output independent of traversal direction.
|
@coderabbitai pause |
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
tests/ninja_gen_tests.rs (1)
315-317: Fix type mismatch when joining paths (Utf8PathBuf vs std::Path).
Path::joinexpectsAsRef<Path>. ConvertUtf8PathBufviaas_std_path()to compile.- assert!(dir.path().join(target_name).exists()); + assert!(dir.path().join(target_name.as_std_path()).exists()); @@ - let content = - fs::read_to_string(dir.path().join(target_name)).expect("read target file"); + let content = fs::read_to_string( + dir.path().join(target_name.as_std_path()) + ) + .expect("read target file");Also applies to: 319-322
docs/netsuke-design.md (1)
1244-1254: Use en‑GB spelling (“flavours”).Replace “flavors” with “flavours” to meet the docs guidelines.
Apply this diff:
- While other crates like `shlex` exist, `shell-quote` offers a more robust and - flexible API specifically designed for this purpose.[^22] It supports quoting - for multiple shell flavors (e.g., Bash, sh, Fish), which is vital for a cross- + While other crates like `shlex` exist, `shell-quote` offers a more robust and + flexible API specifically designed for this purpose.[^22] It supports quoting + for multiple shell flavours (e.g., Bash, sh, Fish), which is vital for a cross-src/ir.rs (5)
277-285: Fix compile error: Utf8PathBuf has no as_os_str(); quote via as_str() (UTF‑8).
Utf8PathBufdoes not exposeas_os_str(). Quote the UTF‑8 string directly or go viaas_std_path().as_os_str()if OS‑native quoting is required.- fn quote_paths(paths: &[Utf8PathBuf]) -> Vec<String> { - paths - .iter() - .map(|p| { - let quoted_bytes: Vec<u8> = p.as_os_str().quoted(Sh); - String::from_utf8_lossy("ed_bytes).into_owned() - }) - .collect() - } + fn quote_paths(paths: &[Utf8PathBuf]) -> Vec<String> { + paths + .iter() + .map(|p| { + // Utf8PathBuf guarantees UTF‑8; avoid lossy conversion. + let bytes: Vec<u8> = p.as_str().quoted(Sh); + String::from_utf8(bytes).expect("utf-8 shell quoting") + }) + .collect() + }
119-137: Document error variants with Rustdoc examples.Add usage examples to public API docs per guidelines so
cargo docis useful.
462-477: Stable output for duplicate detection: use sorted UTF‑8 paths explicitly.
ToString::to_stringis fine here; ensure sort is on&strfor clarity.- let mut dups: Vec<_> = outputs + let mut dups: Vec<String> = outputs .iter() .filter(|o| targets.contains_key(*o)) - .map(ToString::to_string) + .map(|p| p.as_str().to_owned()) .collect();
382-401: Token substitution: guard against$in/$outinside backticks.Skip substitutions within back‑quoted spans to avoid accidental rewrites in shell snippets.
Offer to patch if desired.
1-6: Split src/ir.rs into submodules to respect ≤400‑line limit.
- File size: 692 lines.
- Extract cycle detection (types + tests) → src/ir/cycle.rs.
- Extract command interpolation → src/ir/cmd_interpolate.rs.
- Re-export both from src/ir.rs.
+mod cycle; +mod cmd_interpolate; +pub use cycle::*; +pub use cmd_interpolate::*;Apply to end of file: lines 607–692.
♻️ Duplicate comments (1)
docs/rust-doctest-dry-guide.md (1)
631-635: Replace http with https and remove mdformat guards in footnote [^5].Fix the lingering formatter guards and insecure link per prior review.
-<!-- mdformat off --> -[^5]: Documentation tests — Massachusetts Institute of Technology, accessed on - July 15, 2025, - <http://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/rustdoc/documentation-tests.html> - <!-- mdformat on --> +[^5]: Documentation tests — Massachusetts Institute of Technology, accessed on + July 15, 2025, + <https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/rustdoc/documentation-tests.html>web Confirm that the HTTPS URL for the MIT rustdoc “Documentation tests” page resolves and mirrors the HTTP content referenced in footnote [^5].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/netsuke-design.md(10 hunks)docs/rust-doctest-dry-guide.md(1 hunks)src/ir.rs(12 hunks)src/ninja_gen.rs(5 hunks)tests/ir_from_manifest_tests.rs(2 hunks)tests/ir_tests.rs(3 hunks)tests/ninja_gen_tests.rs(12 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the docs/ markdown files as the reference knowledge base and update them proactively when decisions or requirements change
Files:
docs/rust-doctest-dry-guide.mddocs/netsuke-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar (LICENSE name excluded)
Validate Markdown with make markdownlint and run make fmt to format Markdown (including fixing table markup)
Validate Mermaid diagrams in Markdown by running make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Do not wrap tables and headings in Markdown
Wrap code blocks in Markdown at 120 columns
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1])
Files:
docs/rust-doctest-dry-guide.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/rust-doctest-dry-guide.mddocs/netsuke-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Each Rust module file must begin with a module-level //! comment explaining its purpose
Document public APIs using Rustdoc comments (///) so cargo doc can generate docs
Place function attributes after doc comments
Prefer immutable data; avoid unnecessary mut bindings
Handle errors with Result instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and clearly document any usage
Do not use return in single-line functions
Prefer single-line function bodies where appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Use predicate functions for conditional criteria with more than two branches
Where a function is too long, extract meaningfully named helper functions (separation of concerns, CQRS)
Where a function has too many parameters, group related parameters into meaningfully named structs
If a function is unused with specific features selected, use conditional compilation with #[cfg] or #[cfg_attr]
Where a function returns a large error, consider using Arc to reduce returned data size
Use semantic error enums deriving std::error::Error via thiserror for inspectable conditions
Clippy warnings must be disallowed; fix issues in code rather than silencing
Do not silence lints except as a last resort; any suppression must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] when suppressing lints
Use cap-std for all filesystem operations (capability-based, sandboxed)
Use camino for path handling; avoid std::path::PathBuf directly
Avoid std::fs directly; wrap with cap-std
Comments must use en-GB-oxendict spelling and grammar (except external API names)
No single Rust source file may exceed 400 lines; break up large switches/dispatch tables by feature
Name booleans with is/has/should prefixes; use clear, descriptive names for variables and functions
Function documentation must include...
Files:
tests/ir_tests.rstests/ir_from_manifest_tests.rstests/ninja_gen_tests.rssrc/ninja_gen.rssrc/ir.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/ir_tests.rstests/ir_from_manifest_tests.rstests/ninja_gen_tests.rssrc/ninja_gen.rssrc/ir.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Large blocks of test data should be moved to external data files
Use rstest fixtures for shared setup
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks and stubs
Mock non-deterministic dependencies (Env, Clock) via dependency injection using mockable crate
Test documentation should omit examples that merely reiterate test logic
Files:
tests/ir_tests.rstests/ir_from_manifest_tests.rstests/ninja_gen_tests.rs
🧬 Code graph analysis (4)
tests/ir_tests.rs (1)
src/ir.rs (1)
new(504-511)
tests/ir_from_manifest_tests.rs (1)
src/ir.rs (1)
missing_dependencies(584-586)
tests/ninja_gen_tests.rs (2)
src/ninja_gen.rs (2)
generate(66-70)generate_into(99-138)src/ir.rs (1)
new(504-511)
src/ninja_gen.rs (1)
src/ir.rs (1)
new(504-511)
⏰ 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 (4)
tests/ir_from_manifest_tests.rs (1)
3-3: Adopt Utf8PathBuf for test expectations.Import aligns tests with the public API’s path type migration. No further action.
tests/ir_tests.rs (1)
30-40: LGTM: path type migration in tests is correct.The updates to inputs/outputs and map keys using Utf8PathBuf are consistent with the IR changes and assertions remain valid.
Also applies to: 84-111
docs/netsuke-design.md (2)
965-979: Path policy alignment: good migration to Utf8PathBuf.The IR types and diagram now reflect camino::Utf8PathBuf across targets and edges. No further action here.
Also applies to: 1020-1041
756-764: Remove stray numeric artefacts and correct footnote markers.Replace the dangling “19” and “4” with GitHub‑flavoured footnotes and fix US spelling "flavors" → "flavours" in docs/netsuke-design.md (numeric artefacts: ~lines 756–764; spelling: ~lines 1246–1248).
- `std::process::Command` 19, parse the output using - the - `semver` crate 4, and compare it against + `std::process::Command`[^24], parse the output using the + `semver` crate[^4], and compare it against
✅ Actions performedReviews paused. |
1 similar comment
✅ 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.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
- migrate the IR graph and cycle detector to camino::Utf8PathBuf, expose missing dependencies alongside detected cycles, and derive a reusable analysis report
Extract the cycle detection logic and command interpolation helpers into dedicated ir::cycle and ir::cmd_interpolate modules so src/ir.rs stays below the 400 line limit. Update BuildGraph to call the new analysis API, add tests covering command interpolation behaviour in the new module, and refresh the design docs and doctest footnotes to reflect the module layout.
Factor the build graph structures into src/ir/graph.rs and move the manifest conversion helpers into src/ir/from_manifest.rs so each Rust file stays under the 400 line limit while preserving the existing API.
Document the minimum cycle length assumption with a debug assertion and simplify the suffix update so the trailing node is rewritten from the rotated prefix without panicking.
Reintroduce the mdformat off/on markers around footnote [^5] after the rebase conflict resolution dropped them. This keeps the guide formatting stable and matches the revised main branch layout.
leynos
force-pushed
the
codex/address-code-review-comments-gscfc1
branch
from
7086876 to
894ec88
Compare
1 participant
Summary
CycleDetectorto manage ownedPathBuftraversal state and fold self-cycle handling intovisitcanonicalize_cyclerotation logic and add targeted tests covering cycle detection behaviourTesting
https://chatgpt.com/codex/tasks/task_e_68c8776424708322a80f0f7f1ea8ffae
Summary by Sourcery
Refactor cycle detection logic to use owned PathBufs for traversal state, simplify cycle normalization, document the new design, and add targeted tests.
Enhancements:
visitmethod and exposingfind_cycleDocumentation:
Tests: