Refactor cycle detection traversal#163
Conversation
Reviewer's GuideThis PR refactors the cycle detection logic by encapsulating traversal state in a dedicated CycleDetector struct with new detect and visit methods, streamlines the canonicalize_cycle helper, enriches targeted cycle-detection tests, and updates design documentation accordingly. Class diagram for updated cycle canonicalization helperclassDiagram
class canonicalize_cycle {
+cycle: Vec<PathBuf>
+returns: Vec<PathBuf>
}
canonicalize_cycle --> PathBuf
Class diagram for new cycle-detection test helpersclassDiagram
class edge_with_inputs {
+inputs: &[&str]
+output: &str
+returns: BuildEdge
}
class cyclic_targets {
+returns: HashMap<PathBuf, BuildEdge>
}
edge_with_inputs --> BuildEdge
cyclic_targets --> BuildEdge
cyclic_targets --> PathBuf
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
More reviews will be available in 24 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughExtract the cycle-finding traversal into ChangesCycle detection refactoring and canonicalisation
Sequence DiagramsequenceDiagram
participant analyse
participant CycleDetector
participant visit
analyse->>CycleDetector: detect()
CycleDetector->>CycleDetector: collect & sort target nodes
CycleDetector->>visit: visit(unvisited node)
visit->>visit: traverse dependencies recursively
visit->>CycleDetector: return cycle or None
CycleDetector->>analyse: return first cycle or None
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
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 | |
|---|---|---|---|
| ir.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.
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/netsuke-design.md (1)
1-1715: Use en‑GB Oxford “-ize” spelling consistently in prose.The style guide specifies en‑GB‑oxendict with “-ize/-our”. Replace instances such as “serialise/deserialise/serialisation” with “serialize/deserialize/serialization” across the docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(8 hunks)src/ir.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/**/*.{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/netsuke-design.md
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
🔍 Remote MCP Deepwiki, Ref
Summary of additional context found (concise, review‑focused)
-
The repository’s developer documentation includes a “Developer Guide” with pages for Testing Framework, Code Quality Standards, and Documentation Testing — these directly relate to the PR’s test/format/lint checklist (make fmt, make check-fmt, make lint, make test, make markdownlint, make nixie).
-
Architecture/implementation docs (Architecture → Data Structures / Implementation Details) are present and likely document the IR / BuildEdge / error types (e.g., IrGenError::CircularDependency) referenced by the PR; consult them to validate behavioral claims.
-
Attempts to fetch the PR page and the modified source files (PR #163, files / src/ir.rs) directly failed (HTTP 402), so I could not retrieve the diffs or file contents via the ref tool — reviewers should inspect the PR diffs on GitHub and run the test suite locally to verify behavior.
Actionable review checklist (minimal)
- Confirm unit tests added for cycles/self-edges run and cover expected cases (self-edge rejection, simple cycles, acyclic).
- Run make check-fmt, lint, test, and markdownlint per repo standards.
- Verify canonicalize_cycle rotation is deterministic and messages match expectations (lexicographic rotation to smallest node).
- Confirm no public API/signature changes were introduced.
Citations
- Repo documentation structure (pages found):
- Failed direct fetches of PR/files (HTTP 402):,
⏰ 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 (7)
src/ir.rs (4)
481-486: Encapsulate traversal state; good separation of concerns.Ownership of
stackandstatesinCycleDetectorclarifies the DFS responsibilities.
546-549: Delegate via thin wrapper; OK.
find_cyclecorrectly wraps the detector without exposing internals.
551-568: Canonical cycle normalisation looks correct.Dropping the duplicate terminator, rotating to the minimal node, then re‑closing the cycle yields stable, readable diagnostics.
Ensure ordering invariants are documented in the error message format used by callers so snapshot tests remain stable across platforms.
481-486: Replace std::path::PathBuf with camino::Utf8PathBuf in IR types; verify MSRV and dependencyReplace PathBuf with Utf8PathBuf in IR structs and tests in a follow‑up to avoid widening this PR. Confirm the project’s MSRV supports the camino version already used, or add camino in a separate change to avoid MSRV churn.
Location: src/ir.rs — lines 481–486, 574–584
docs/netsuke-design.md (3)
1078-1088: Document algorithm precisely; matches implementation.State that order‑only deps are ignored and that cycles are reported from the first re‑visited node with the cycle rotated to the lexicographically smallest element. This aligns with
CycleDetector.
1189-1189: Tighten CLI description around-Cand stdio piping.Explicitly call out that Netsuke resolves
-Citself and forwards targets and-jto Ninja; stdio is piped and mirrored. This is clear and actionable.Also applies to: 1194-1194, 1196-1198, 1200-1204
1538-1538: Keep roadmap tasks in imperative mood and consistent style.Entries read cleanly and map to deliverables; good.
Also applies to: 1541-1541, 1544-1544, 1546-1546
| fn detect(&mut self) -> Option<Vec<PathBuf>> { | ||
| for node in self.targets.keys() { | ||
| if self.states.contains_key(node.as_path()) { | ||
| continue; | ||
| } | ||
|
|
||
| if let Some(cycle) = visit(targets, dep, stack, states) { | ||
| if let Some(cycle) = self.visit(node.clone()) { | ||
| return Some(cycle); | ||
| } | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Make traversal order deterministic.
HashMap iteration order is non‑deterministic; the first reported cycle may vary run‑to‑run. Sort keys before traversal to stabilise error reporting.
Apply this diff:
- fn detect(&mut self) -> Option<Vec<PathBuf>> {
- for node in self.targets.keys() {
+ fn detect(&mut self) -> Option<Vec<PathBuf>> {
+ let mut nodes: Vec<_> = self.targets.keys().cloned().collect();
+ nodes.sort();
+ for node in nodes {
if self.states.contains_key(node.as_path()) {
continue;
}
- if let Some(cycle) = self.visit(node.clone()) {
+ if let Some(cycle) = self.visit(node.clone()) {
return Some(cycle);
}
}
None
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn detect(&mut self) -> Option<Vec<PathBuf>> { | |
| for node in self.targets.keys() { | |
| if self.states.contains_key(node.as_path()) { | |
| continue; | |
| } | |
| if let Some(cycle) = visit(targets, dep, stack, states) { | |
| if let Some(cycle) = self.visit(node.clone()) { | |
| return Some(cycle); | |
| } | |
| } | |
| None | |
| } | |
| fn detect(&mut self) -> Option<Vec<PathBuf>> { | |
| let mut nodes: Vec<_> = self.targets.keys().cloned().collect(); | |
| nodes.sort(); | |
| for node in nodes { | |
| if self.states.contains_key(node.as_path()) { | |
| continue; | |
| } | |
| if let Some(cycle) = self.visit(node.clone()) { | |
| return Some(cycle); | |
| } | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
In src/ir.rs around lines 497 to 507, the detect() traversal uses
HashMap::keys() which yields a non-deterministic order; collect the keys into a
Vec<PathBuf>, sort that Vec (e.g. let mut keys: Vec<_> =
self.targets.keys().cloned().collect(); keys.sort();), then iterate over the
sorted keys so cycle detection is deterministic and error reports are stable
across runs.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the relevant file(s)
fd -e rs cycle
fd -e rs ir
# Check current implementation of detect()
rg -n "fn detect" --type=rs -A 15Length of output: 502
🏁 Script executed:
#!/bin/bash
# Cat the cycle detection file to see current state
cat src/ir/cycle.rsLength of output: 7368
@leynos — the latest commit has not resolved this. The detect() method in src/ir/cycle.rs still iterates self.targets.keys() directly without sorting:
fn detect(&mut self) -> Option<Vec<Utf8PathBuf>> {
for node in self.targets.keys() { // ← still non-deterministic
if self.is_visited(node) {Note that the type has migrated from PathBuf to camino::Utf8PathBuf, so the fix must reference that type. Here is the prompt for an AI coding agent:
File: src/ir/cycle.rs
Symbol: CycleDetector::detect
The `detect` method iterates `self.targets.keys()` which yields a non-deterministic
order from the underlying `HashMap`. This makes the first reported cycle unstable
across runs.
Make the following change inside `impl CycleDetector<'_>`:
1. In `detect()`, collect the keys into a sorted `Vec` before iterating:
```diff
fn detect(&mut self) -> Option<Vec<Utf8PathBuf>> {
- for node in self.targets.keys() {
+ let mut nodes: Vec<Utf8PathBuf> = self.targets.keys().cloned().collect();
+ nodes.sort();
+ for node in nodes {
if self.is_visited(node) {
continue;
}
- if let Some(cycle) = self.visit(node.clone()) {
+ if let Some(cycle) = self.visit(node) {
return Some(cycle);
}
}
None
}
```
2. Verify that `cargo test` passes after the change and commit only that change.
Make no other modifications to the file.
| fn visit(&mut self, node: PathBuf) -> Option<Vec<PathBuf>> { | ||
| match self.states.get(node.as_path()) { | ||
| Some(VisitState::Visited) => return None, | ||
| Some(VisitState::Visiting) => { | ||
| let idx = self | ||
| .stack | ||
| .iter() | ||
| .position(|n| n == &node) | ||
| .expect("node should be present in the traversal stack"); | ||
| let mut cycle: Vec<PathBuf> = self.stack.iter().skip(idx).cloned().collect(); | ||
| cycle.push(node.clone()); | ||
| return Some(canonicalize_cycle(cycle)); | ||
| } | ||
| None => { | ||
| self.states.insert(node.clone(), VisitState::Visiting); | ||
| } | ||
| } | ||
| if let Some(cycle) = visit(targets, node, &mut stack, &mut states) { | ||
| return Some(cycle); | ||
|
|
||
| self.stack.push(node.clone()); | ||
|
|
||
| if let Some(edge) = self.targets.get(&node) { | ||
| for dep in &edge.inputs { | ||
| if !self.targets.contains_key(dep) { | ||
| continue; | ||
| } | ||
| if let Some(cycle) = self.visit(dep.clone()) { | ||
| return Some(cycle); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| self.stack.pop(); | ||
| self.states.insert(node, VisitState::Visited); | ||
| None | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid needless cloning by visiting by reference.
Passing PathBuf by value forces clones on every edge. Visit by &Path to cut allocations on deep graphs.
Apply this diff (and adjust call sites accordingly):
- fn visit(&mut self, node: PathBuf) -> Option<Vec<PathBuf>> {
- match self.states.get(node.as_path()) {
+ fn visit(&mut self, node: &std::path::Path) -> Option<Vec<PathBuf>> {
+ match self.states.get(node) {
Some(&VisitState::Visited) => return None,
Some(&VisitState::Visiting) => {
- let idx = self
- .stack
- .iter()
- .position(|n| n == &node)
+ let idx = self.stack.iter().position(|n| n.as_path() == node)
.expect("node should be present in the traversal stack");
let mut cycle: Vec<PathBuf> = self.stack.iter().skip(idx).cloned().collect();
- cycle.push(node.clone());
+ cycle.push(node.to_path_buf());
return Some(canonicalize_cycle(cycle));
}
None => {
- self.states.insert(node.clone(), VisitState::Visiting);
+ self.states.insert(node.to_path_buf(), VisitState::Visiting);
}
}
- self.stack.push(node.clone());
+ self.stack.push(node.to_path_buf());
- if let Some(edge) = self.targets.get(&node) {
+ if let Some(edge) = self.targets.get(node) {
for dep in &edge.inputs {
if !self.targets.contains_key(dep) {
continue;
}
- if let Some(cycle) = self.visit(dep.clone()) {
+ if let Some(cycle) = self.visit(dep) {
return Some(cycle);
}
}
}
self.stack.pop();
- self.states.insert(node, VisitState::Visited);
+ self.states.insert(node.to_path_buf(), VisitState::Visited);
None
}And in detect:
- if let Some(cycle) = self.visit(node.clone()) {
+ if let Some(cycle) = self.visit(node.as_path()) {
return Some(cycle);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn visit(&mut self, node: PathBuf) -> Option<Vec<PathBuf>> { | |
| match self.states.get(node.as_path()) { | |
| Some(VisitState::Visited) => return None, | |
| Some(VisitState::Visiting) => { | |
| let idx = self | |
| .stack | |
| .iter() | |
| .position(|n| n == &node) | |
| .expect("node should be present in the traversal stack"); | |
| let mut cycle: Vec<PathBuf> = self.stack.iter().skip(idx).cloned().collect(); | |
| cycle.push(node.clone()); | |
| return Some(canonicalize_cycle(cycle)); | |
| } | |
| None => { | |
| self.states.insert(node.clone(), VisitState::Visiting); | |
| } | |
| } | |
| if let Some(cycle) = visit(targets, node, &mut stack, &mut states) { | |
| return Some(cycle); | |
| self.stack.push(node.clone()); | |
| if let Some(edge) = self.targets.get(&node) { | |
| for dep in &edge.inputs { | |
| if !self.targets.contains_key(dep) { | |
| continue; | |
| } | |
| if let Some(cycle) = self.visit(dep.clone()) { | |
| return Some(cycle); | |
| } | |
| } | |
| } | |
| self.stack.pop(); | |
| self.states.insert(node, VisitState::Visited); | |
| None | |
| } | |
| fn visit(&mut self, node: &std::path::Path) -> Option<Vec<PathBuf>> { | |
| match self.states.get(node) { | |
| Some(&VisitState::Visited) => return None, | |
| Some(&VisitState::Visiting) => { | |
| let idx = self | |
| .stack | |
| .iter() | |
| .position(|n| n.as_path() == node) | |
| .expect("node should be present in the traversal stack"); | |
| let mut cycle: Vec<PathBuf> = self.stack.iter().skip(idx).cloned().collect(); | |
| cycle.push(node.to_path_buf()); | |
| return Some(canonicalize_cycle(cycle)); | |
| } | |
| None => { | |
| self.states.insert(node.to_path_buf(), VisitState::Visiting); | |
| } | |
| } | |
| self.stack.push(node.to_path_buf()); | |
| if let Some(edge) = self.targets.get(node) { | |
| for dep in &edge.inputs { | |
| if !self.targets.contains_key(dep) { | |
| continue; | |
| } | |
| if let Some(cycle) = self.visit(dep) { | |
| return Some(cycle); | |
| } | |
| } | |
| } | |
| self.stack.pop(); | |
| self.states.insert(node.to_path_buf(), VisitState::Visited); | |
| None | |
| } |
🤖 Prompt for AI Agents
In src/ir.rs around lines 509 to 543, change visit to take a reference (fn
visit(&mut self, node: &Path)) and update all uses inside: lookup states with
self.states.get(node), insert with self.states.insert(node.to_path_buf(),
VisitState::Visiting), push onto stack with self.stack.push(node.to_path_buf()),
build cycle by comparing stack entries to node (use node.as_ref() or &** as
needed), and when recursing call self.visit(dep.as_path()) instead of cloning;
after finishing, pop the stack and mark visited with
self.states.insert(self.stack_last_or_node.to_path_buf(), VisitState::Visited)
(or insert node.to_path_buf() if easier). Also update detect and any other call
sites to pass &Path (e.g., node.as_path()) so you avoid unnecessary PathBuf
clones on every edge traversal.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what files exist in src/ir/
fd . src/ir --type f
# Check the current state of the visit function
rg -n "fn visit" src/ir.rs src/ir/ 2>/dev/null || true
# Show the full visit function implementation
ast-grep --pattern 'fn visit(&mut self, $_) -> Option<Vec<PathBuf>> {
$$$
}'
# Also check detect function for how visit is called
rg -n -A 5 -B 2 "self\.visit" src/ir.rs src/ir/ 2>/dev/null || true
# Get the PR comment ID for the comment to resolve
gh api repos/leynos/netsuke/pulls/163/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:100]}'Length of output: 2438
🏁 Script executed:
#!/bin/bash
# Show the full visit function and detect function
sed -n '40,140p' src/ir/cycle.rsLength of output: 3063
@leynos, this comment is not yet resolved. The code was refactored into src/ir/cycle.rs and the type changed from PathBuf to Utf8PathBuf (camino), but visit still takes ownership by value — clones remain at every call site:
self.visit(node.clone())indetect()self.visit(dep.clone())invisit_dependency()
Use the following prompt to instruct an AI coding agent to apply the fix.
File: src/ir/cycle.rs
Change `fn visit` to accept `&Utf8Path` instead of `Utf8PathBuf` and update all affected call sites and helpers. The `Utf8Path` type comes from the `camino` crate.
Steps:
1. Change the signature:
```
fn visit(&mut self, node: &Utf8Path) -> Option<Vec<Utf8PathBuf>>
```
2. Inside `visit`:
- `self.states.get(&node)` → `self.states.get(node)` (HashMap<Utf8PathBuf,_> already implements Borrow<Utf8Path> via camino).
- `self.states.insert(node.clone(), VisitState::Visiting)` → `self.states.insert(node.to_path_buf(), VisitState::Visiting)`.
- `.position(|n| n == &node)` → `.position(|n| n.as_path() == node)`.
- `cycle.push(node)` → `cycle.push(node.to_path_buf())`.
- `self.stack.push(node.clone())` → `self.stack.push(node.to_path_buf())`.
- `self.targets.get(&node)` → `self.targets.get(node)`.
- `self.states.insert(node, VisitState::Visited)` → `self.states.insert(node.to_path_buf(), VisitState::Visited)`.
3. Update `detect`:
- `self.visit(node.clone())` → `self.visit(node.as_path())`.
4. Update `visit_dependency`:
- Change both parameters to `&Utf8Path`:
```
fn visit_dependency(&mut self, node: &Utf8Path, dep: &Utf8Path) -> Option<Vec<Utf8PathBuf>>
```
- `self.record_missing_dependency(node, dep)` — update `record_missing_dependency` to accept `&Utf8Path` as well, converting to `Utf8PathBuf` only when inserting into `self.missing_dependencies`.
- `self.visit(dep.clone())` → `self.visit(dep)`.
5. Update `is_visited`:
- Change parameter to `&Utf8Path`: `fn is_visited(&self, node: &Utf8Path) -> bool`.
- `self.states.get(node)` works directly.
6. Update the call to `is_visited` in `detect`:
- `self.is_visited(node)` → `self.is_visited(node.as_path())`.
7. Run `cargo test` to confirm all tests pass, then run `cargo clippy` to confirm no new lints.
Make no other changes.
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
| fn edge_with_inputs(inputs: &[&str], output: &str) -> BuildEdge { | ||
| BuildEdge { | ||
| action_id: "id".into(), | ||
| inputs: vec![PathBuf::from("b")], | ||
| explicit_outputs: vec![PathBuf::from("a")], | ||
| inputs: inputs.iter().map(PathBuf::from).collect(), | ||
| explicit_outputs: vec![PathBuf::from(output)], | ||
| implicit_outputs: Vec::new(), | ||
| order_only_deps: Vec::new(), | ||
| phony: false, | ||
| always: false, | ||
| }; | ||
| let edge_b = BuildEdge { | ||
| action_id: "id".into(), | ||
| inputs: vec![PathBuf::from("a")], | ||
| explicit_outputs: vec![PathBuf::from("b")], | ||
| implicit_outputs: Vec::new(), | ||
| order_only_deps: Vec::new(), | ||
| phony: false, | ||
| always: false, | ||
| }; | ||
| targets.insert(PathBuf::from("a"), edge_a); | ||
| targets.insert(PathBuf::from("b"), edge_b); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tests cover core scenarios; expand for determinism and multi‑cycle graphs.
Add a test that asserts determinism of the first reported cycle across runs, and a case where multiple disjoint cycles exist.
I can draft #[rstest] parametrised cases for multi‑cycle graphs and a seed‑stability check once traversal order is sorted.
Also applies to: 586-591, 593-602, 604-613, 615-623, 624-633, 634-651, 652-668
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the cycle detection file
fd -e rs cycle src/
# Show the full test module contents
rg -n '#\[test\]|#\[rstest\]|fn.*cycle|fn.*determinism|fn.*disjoint|fn.*multi' src/ --type rs -A2Length of output: 485
🏁 Script executed:
#!/bin/bash
cat src/ir/cycle.rsLength of output: 7330
@leynos — Not resolved. The test suite still lacks both items flagged in the original comment:
- Determinism —
detect()iterates overtargets.keys(), aHashMapwith non-deterministic key order. No test asserts that the same cycle is returned on repeated invocations over the same graph. - Multi-disjoint-cycle — No test exercises a graph containing two or more independent cycles and asserts that at least one is detected.
Below is an AI coding-agent prompt to address the remaining work:
## Task: Add determinism and multi-cycle tests to `src/ir/cycle.rs`
### Context
File: `src/ir/cycle.rs`
Test module: the `#[cfg(test)] mod tests` block at the bottom of that file.
The `CycleDetector::find_cycle` helper is already available inside the test module.
### Requirements
#### 1. Determinism test
Add a test named `find_cycle_is_deterministic` that:
- Builds a `HashMap<Utf8PathBuf, BuildEdge>` containing **two disjoint 2-node cycles**:
- cycle α: `p` → `q` → `p`
- cycle β: `x` → `y` → `x`
- Calls `CycleDetector::find_cycle(&targets)` **100 times** in a loop.
- Asserts that every call returns `Some(cycle)` and that all returned cycles are identical (i.e., the first cycle found is stable across invocations even though `HashMap` iteration order is non-deterministic in general).
- If after 100 runs every call returns the same value, emit a `tracing::info!` or a comment explaining this is a probabilistic check; the test should `panic!` if any two results differ.
Note: because `HashMap` key order is not guaranteed, this test documents a known limitation. If the test is inherently flaky, extract a `TODO` comment explaining that a deterministic traversal order (e.g. `BTreeMap`) is needed and skip the assertion—but still leave the test scaffolding in place.
#### 2. Multi-disjoint-cycle test
Add a test named `find_cycle_detects_one_of_multiple_disjoint_cycles` that:
- Builds a `HashMap` containing the same two disjoint 2-node cycles as above (α and β).
- Calls `CycleDetector::find_cycle(&targets)`.
- Asserts `Some(_)` is returned (i.e., at least one cycle is detected).
- Does **not** assert which specific cycle is returned (because order is non-deterministic).
#### Constraints
- Use the existing `path(name: &str)` and `build_edge(inputs, output)` helpers already present in the test module.
- Keep `#[test]` attributes. Do not use `#[rstest]` for these two new tests.
- Make no other changes to production code.
- Verify `cargo test` passes before committing.
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
lodyai
Bot
force-pushed
the
codex/refactor-find_cycle-to-reduce-nesting-complexity-xa3wfu
branch
from
24d8cd0 to
983dbe1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +653 to +667 fn canonicalize_cycle_handles_reverse_direction() {
let cycle = vec![
PathBuf::from("c"),
PathBuf::from("b"),
PathBuf::from("a"),
PathBuf::from("c"),
];
let canonical = canonicalize_cycle(cycle);
let expected = vec![
PathBuf::from("a"),
PathBuf::from("c"),
PathBuf::from("b"),
PathBuf::from("a"),
];
assert_eq!(canonical, expected);❌ New issue: Code Duplication |
|
@coderabbitai Have the following now been resolved? Annul any requirements that violate the en-GB-oxendict spelling (-ize / -yse / -our) conventions (for example a request to replace "normalize" with "normalise" or "artefact" with "artifact"), or where the requirement unnecessarily increases cyclomatic complexity. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Deduplicate the two table-driven canonicalize_cycle tests by introducing `check_canonicalize_cycle` in the tests module. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (3 errors, 3 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
Pair every `stack.push` with `stack.pop` even when a cycle is found, and only mark the node `Visited` when no cycle was detected. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai Have the following now been resolved? docs/netsuke-design.md (1)
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Add a regression test that `CycleDetector::detect` leaves the traversal stack empty once a cycle has been found. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ir/cycle.rs (1)
86-98:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnwind traversal state before returning a found cycle.
Fix the early return at Line 93. Pop
self.stackunconditionally, then markVisitedonly when no cycle was found. Preserve stack invariants across all return paths.Patch
- if let Some(cycle) = self + let cycle = self .targets .get(&node) .into_iter() .flat_map(|edge| edge.inputs.iter()) - .find_map(|dep| self.visit_dependency(&node, dep)) - { - return Some(cycle); - } + .find_map(|dep| self.visit_dependency(&node, dep)); self.stack.pop(); - self.states.insert(node, VisitState::Visited); - None + if cycle.is_none() { + self.states.insert(node, VisitState::Visited); + } + cycle🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ir/cycle.rs` around lines 86 - 98, The current early return when a cycle is found leaves self.stack unpopped and breaks invariants; change the logic in the block that checks self.targets/get(&node)...find_map(...) so that you always call self.stack.pop() before any return, and only call self.states.insert(node, VisitState::Visited) when no cycle was found; adjust usage around visit_dependency(&node, dep) and the conditional return so stack popping happens unconditionally and VisitState::Visited is set only in the no-cycle path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/ir/cycle.rs`:
- Around line 86-98: The current early return when a cycle is found leaves
self.stack unpopped and breaks invariants; change the logic in the block that
checks self.targets/get(&node)...find_map(...) so that you always call
self.stack.pop() before any return, and only call self.states.insert(node,
VisitState::Visited) when no cycle was found; adjust usage around
visit_dependency(&node, dep) and the conditional return so stack popping happens
unconditionally and VisitState::Visited is set only in the no-cycle path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 020f1c05-4b11-4d74-a791-8496f2f0aed0
📒 Files selected for processing (1)
src/ir/cycle.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document `analyse`, `CycleDetectionReport`, and the `CycleDetector` traversal flow in the module-level rustdoc. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add rustdoc to `CycleDetectionReport`, `analyse`, `CycleDetector`, and related helpers in `src/ir/cycle.rs`. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Verify idempotence, rotation invariance, smallest-first ordering, and cycle closure with property-based tests. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Iterate nodes in deterministic order so the first reported cycle is stable across runs. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add `find_cycle_is_deterministic` (100-run stability check) and `find_cycle_detects_one_of_multiple_disjoint_cycles` for two disjoint 2-node cycles. Accept `&Utf8Path` in visit helpers to avoid redundant clones. Co-authored-by: Cursor <cursoragent@cursor.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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ir/cycle.rs`:
- Around line 345-360: The formatting of the proptest strategy builder in the
node_names function (the proptest::collection::vec(...).prop_filter(...) chain)
does not satisfy rustfmt; run cargo fmt --all or reformat the node_names
function so the chained calls are laid out per rustfmt expectations (e.g., align
method chaining or break arguments onto separate lines) to satisfy cargo fmt
--check—update the node_names function in src/ir/cycle.rs that references
proptest::collection::vec and prop_filter (keeping the uniqueness filter logic
intact) and then re-run the formatter.
- Around line 171-185: The function signature for visit_dependency (and its
parameter list and return type) is not formatted to satisfy rustfmt; run cargo
fmt --all to reformat the file and/or adjust the visit_dependency
signature/parameters to match rustfmt style (ensure the fn visit_dependency(&mut
self, node: &Utf8Path, dep: &Utf8Path) -> Option<Vec<Utf8PathBuf>> declaration
is split/wrapped according to rustfmt rules) and keep the internal calls to
self.record_missing_dependency and self.visit unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2cce7ea8-5853-4237-b894-e4a074a3cfa3
📒 Files selected for processing (1)
src/ir/cycle.rs
Adjust `visit_dependency` signature and `node_names` `prop_filter` chain
to satisfy `rustfmt`.
Replace `if ... { panic!() }` with `assert!` in the determinism test to
satisfy `clippy::manual_assert`, and use `.get()` / `.first()` with
`.expect()` in proptest helpers to satisfy `clippy::indexing_slicing`.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove an extra blank line before a heading and delete a duplicate Section 2.7 table block that was repeated later in the document.
1 participant
Summary
CycleDetectorto own traversal state and expose adetectAPI, avoiding borrowed keyscanonicalize_cycleby rotating the prefix after removing the duplicate terminatorcloses #74
Testing
make fmtmake check-fmtmake lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_68c615de9a808322b7eca4a379fe4f9b
Summary by Sourcery
Refactor the cycle detection logic into a dedicated CycleDetector struct with a clear detect API, simplify cycle canonicalization, and reinforce correctness with targeted tests and documentation updates.
Enhancements:
Documentation:
Tests: