Skip to content

Implement cycle detection in IR#36

Merged
leynos merged 5 commits intomainfrom
codex/implement-cycle-detection-algorithm
Jul 30, 2025
Merged

Implement cycle detection in IR#36
leynos merged 5 commits intomainfrom
codex/implement-cycle-detection-algorithm

Conversation

@leynos
Copy link
Copy Markdown
Owner

@leynos leynos commented Jul 28, 2025

Summary

  • detect cycles during manifest transformation
  • return CircularDependency error from BuildGraph
  • test cycle detection via rstest
  • cover the feature in cucumber scenarios
  • document cycle detection in design docs
  • mark roadmap item as done

Testing

  • make fmt
  • make lint
  • make test
  • make markdownlint
  • make nixie (fails: too many arguments. Expected 0 arguments but got 1.)

https://chatgpt.com/codex/tasks/task_e_6886b9ebfba883228941ef3e1d7eca79

Summary by Sourcery

Implement cycle detection in the IR build graph and fail manifest transformation on circular dependencies

New Features:

  • Add detect_cycles method to BuildGraph to identify circular dependencies
  • Introduce CircularDependency error in IrGenError when a cycle is found
  • Invoke cycle detection during BuildGraph::from_manifest

Documentation:

  • Document the cycle detection algorithm in design documentation
  • Mark the cycle detection item as completed in the project roadmap

Tests:

  • Add rstest case and cucumber scenario for circular dependency detection
  • Include tests/data/circular.yml fixture for cycle detection tests

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented Jul 28, 2025

Reviewer's Guide

Implements cycle detection in the IR transformation by adding a new CircularDependency error, invoking a DFS-based check in BuildGraph, and updating tests and documentation to cover the new behavior.

Flow diagram for cycle detection in BuildGraph

flowchart TD
    A[Start BuildGraph::from_manifest]
    B[process_targets]
    C[process_defaults]
    D[detect_cycles]
    E{Cycle detected?}
    F[Return CircularDependency error]
    G[Return BuildGraph]

    A --> B --> C --> D --> E
    E -- Yes --> F
    E -- No --> G
Loading

File-Level Changes

Change Details Files
Add cycle detection to BuildGraph IR generation
  • Introduce IrGenError::CircularDependency variant
  • Invoke graph.detect_cycles() in from_manifest
  • Implement detect_cycles() using DFS with visitation states to identify back edges
src/ir.rs
Extend tests to cover circular dependency errors
  • Add CircularDependency to ExpectedError enum and rstest cases
  • Assert path matches PathBuf in unit tests
  • Add cucumber scenario for circular manifest
  • Include tests/data/circular.yml as test fixture
tests/ir_from_manifest_tests.rs
tests/features/ir.feature
tests/data/circular.yml
Update documentation for cycle detection
  • Describe DFS-based cycle detection in design docs
  • Mark roadmap cycle detection item as done
docs/netsuke-design.md
docs/roadmap.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jul 28, 2025

Summary by CodeRabbit

  • New Features

    • Added detection and error reporting for circular dependencies in build targets.
    • Error messages for circular dependencies now display the full cycle for easier debugging.
  • Bug Fixes

    • Compilation now fails gracefully when circular dependencies are present in the build graph.
  • Documentation

    • Improved documentation explaining the cycle detection algorithm and updated the roadmap to mark this feature as complete.
  • Tests

    • Added new test cases and data to verify correct detection and handling of circular dependencies.

Walkthrough

Document the addition of cycle detection logic to the build graph IR generation. Update the error handling to include a new error variant for circular dependencies. Extend the documentation and roadmap to reflect this feature. Add targeted tests and test data to verify correct failure behaviour when circular dependencies are present.

Changes

Cohort / File(s) Change Summary
Documentation Updates
docs/netsuke-design.md, docs/roadmap.md
Expand the design doc with a detailed explanation of the cycle detection algorithm and update the roadmap to mark cycle detection as complete.
IR and Error Handling
src/ir.rs
Add CircularDependency variant to IrGenError. Implement detect_cycles method with DFS for cycle detection in BuildGraph. Integrate cycle detection into graph construction.
Test Data
tests/data/circular.yml
Introduce a manifest with two targets referencing each other to create a circular dependency for testing purposes.
Feature Tests
tests/features/ir.feature
Add a scenario to verify that IR generation fails when a circular dependency is present in the manifest.
Unit Tests
tests/ir_from_manifest_tests.rs
Add a test case and error variant to check for circular dependency errors when loading manifests. Update error matching logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant BuildGraph
    participant IrGenError

    User->>BuildGraph: from_manifest(manifest)
    BuildGraph->>BuildGraph: process rules, targets, defaults
    BuildGraph->>BuildGraph: detect_cycles()
    alt No cycles
        BuildGraph-->>User: Return BuildGraph
    else Cycle detected
        BuildGraph-->>IrGenError: CircularDependency(cycle)
        IrGenError-->>User: Error with cycle path
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~18 minutes

Possibly related PRs

Suggested reviewers

  • codescene-delta-analysis

Poem

In the graph where targets play,
A cycle lurked, but not today!
With depth-first eyes, the code now sees
Circular paths and shouts a freeze.
No endless loops shall sneak on through—
The build is safe, the tests are too!
🕵️‍♂️🔄


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0cbaaf8 and 81be382.

📒 Files selected for processing (3)
  • docs/netsuke-design.md (2 hunks)
  • src/ir.rs (4 hunks)
  • tests/ir_from_manifest_tests.rs (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit Inference Engine (AGENTS.md)

**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type 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 for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.

Files:

  • tests/ir_from_manifest_tests.rs
  • 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 return in single-line functions.

  • Move conditionals with >2 branches into a predicate function.

  • Avoid unsafe unless 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.
  • Use rstest fixtures for shared setup and to avoid repetition between tests.

  • Replace duplicated tests with #[rstest(...)] parameterised cases.

  • Prefer mockall for 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.

Files:

  • tests/ir_from_manifest_tests.rs
  • src/ir.rs
docs/**/*.md

📄 CodeRabbit Inference Engine (AGENTS.md)

docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in the docs/ directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.

Files:

  • docs/netsuke-design.md
**/*.md

📄 CodeRabbit Inference Engine (AGENTS.md)

**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.

Files:

  • docs/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
  • Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
  • Code blocks should be wrapped to 120 columns.
  • 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
🔇 Additional comments (11)
src/ir.rs (5)

96-98: LGTM! Well-structured error variant for cycle detection.

The CircularDependency variant properly represents a cycle as a vector of paths, providing clear debugging information.


116-117: LGTM! Cycle detection properly integrated into the build flow.

The placement after graph construction and before returning ensures cycles are caught early.


174-179: LGTM! Clean wrapper for cycle detection.

The method properly delegates to find_cycle() and converts the result to the appropriate error type.


283-381: LGTM! Well-implemented cycle detection with proper DFS and canonicalization.

The implementation correctly:

  • Uses DFS with state tracking to detect cycles
  • Extracts the cycle path from the recursion stack
  • Canonicalizes cycles for deterministic error messages
  • Excludes order-only dependencies from cycle detection as intended

383-416: LGTM! Test properly handles non-deterministic cycle ordering.

The test correctly accounts for HashMap's non-deterministic iteration order by accepting either valid cycle representation.

tests/ir_from_manifest_tests.rs (4)

8-8: LGTM! Required import for cycle path handling.


42-42: LGTM! Consistent enum variant for circular dependency testing.


69-72: LGTM! Well-structured test case for circular dependency detection.


96-105: LGTM! Robust comparison handling non-deterministic ordering.

The sorting approach elegantly handles HashMap's non-deterministic iteration order while maintaining test correctness.

docs/netsuke-design.md (2)

992-999: LGTM! Comprehensive documentation of the cycle detection algorithm.

The documentation clearly explains the DFS approach, recursion stack usage, and cycle canonicalization for deterministic error reporting.


1210-1211: LGTM! Documentation correctly reflects the error variant structure.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/implement-cycle-detection-algorithm

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@codescene-delta-analysis codescene-delta-analysis Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Failed
Enforce critical code health rules (1 file with Bumpy Road Ahead)

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Reason for failure
Enforce critical code health rules Violations Code Health Impact
ir.rs 1 critical rule 10.00 → 9.84 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.

Comment thread src/ir.rs
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.

  • Enhance the CircularDependency error to include the full cycle of targets (not just the repeated node) for improved debugging.
  • Refactor detect_cycles to match on graph.targets.get(dep) directly instead of separate contains_key and get() calls to simplify lookup logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Enhance the CircularDependency error to include the full cycle of targets (not just the repeated node) for improved debugging.
- Refactor detect_cycles to match on graph.targets.get(dep) directly instead of separate contains_key and get() calls to simplify lookup logic.

## Individual Comments

### Comment 1
<location> `src/ir.rs:193` </location>
<code_context>
+            }
+
+            states.insert(node, State::Visiting);
+            if let Some(edge) = graph.targets.get(node) {
+                for dep in edge.inputs.iter().chain(&edge.order_only_deps) {
+                    if graph.targets.contains_key(dep) {
+                        visit(graph, dep, states)?;
</code_context>

<issue_to_address>
Order-only dependencies are treated the same as regular inputs in cycle detection.

If order-only dependencies should not be part of cycle detection, filter them out before this logic.
</issue_to_address>

### Comment 2
<location> `src/ir.rs:174` </location>
<code_context>
         }
     }
+
+    fn detect_cycles(&self) -> Result<(), IrGenError> {
+        #[derive(Clone, Copy)]
+        enum State {
</code_context>

<issue_to_address>
Consider refactoring the cycle detection logic by extracting the DFS into a separate function or switching to Kahn’s algorithm for clarity and testability.

You can simplify `detect_cycles` and remove the ad-hoc DFS/state‐enum by either

1. pulling the nested helper + `State` enum out into its own (testable!) function, or  
2. switching to a Kahn-style topological sort.

–––

### 1) Minimal refactor: extract the DFS into its own function

```rust
// at module‐scope (so you can unit–test it)
enum VisitState { Visiting, Visited }

fn find_cycle(
    targets: &HashMap<PathBuf, Edge>
) -> Option<PathBuf> {
    fn visit(
        node: &PathBuf,
        targets: &HashMap<PathBuf, Edge>,
        states: &mut HashMap<&PathBuf, VisitState>,
    ) -> Option<PathBuf> {
        match states.get(node) {
            Some(VisitState::Visited) => return None,
            Some(VisitState::Visiting) => return Some(node.clone()),
            None => {},
        }
        states.insert(node, VisitState::Visiting);
        if let Some(edge) = targets.get(node) {
            for dep in edge.inputs.iter().chain(&edge.order_only_deps) {
                if targets.contains_key(dep) {
                    if let Some(cycle_node) = visit(dep, targets, states) {
                        return Some(cycle_node);
                    }
                }
            }
        }
        states.insert(node, VisitState::Visited);
        None
    }

    let mut states = HashMap::new();
    for node in targets.keys() {
        if !states.contains_key(node) {
            if let Some(cycle_node) = visit(node, targets, &mut states) {
                return Some(cycle_node);
            }
        }
    }
    None
}

impl BuildGraph {
    fn detect_cycles(&self) -> Result<(), IrGenError> {
        if let Some(path) = find_cycle(&self.targets) {
            Err(IrGenError::CircularDependency { path })
        } else {
            Ok(())
        }
    }
}
```

–––

### 2) Alternative: Kahn’s Algorithm (no recursion or enum)

```rust
use std::collections::VecDeque;

impl BuildGraph {
    fn detect_cycles(&self) -> Result<(), IrGenError> {
        // 1) build indegree map
        let mut indegree = HashMap::new();
        for (node, edge) in &self.targets {
            indegree.entry(node.clone()).or_insert(0);
            for dep in edge.inputs.iter().chain(&edge.order_only_deps) {
                if self.targets.contains_key(dep) {
                    *indegree.entry(dep.clone()).or_insert(0) += 1;
                }
            }
        }

        // 2) seed queue with zero‐indegree nodes
        let mut queue: VecDeque<_> = indegree
            .iter()
            .filter_map(|(n, &d)| if d == 0 { Some(n.clone()) } else { None })
            .collect();

        // 3) process
        let mut seen = 0;
        while let Some(node) = queue.pop_front() {
            seen += 1;
            if let Some(edge) = self.targets.get(&node) {
                for dep in edge.inputs.iter().chain(&edge.order_only_deps) {
                    if let Some(count) = indegree.get_mut(dep) {
                        *count -= 1;
                        if *count == 0 {
                            queue.push_back(dep.clone());
                        }
                    }
                }
            }
        }

        // 4) if not all nodes visited, there is a cycle
        if seen != self.targets.len() {
            // optionally: detect which node(s) remain in indegree>0 as the cycle ‘path’
            Err(IrGenError::CircularDependency { path: PathBuf::new() })
        } else {
            Ok(())
        }
    }
}
```

Either approach removes the nested enum+function noise, flattens the logic, and makes testing much easier.
</issue_to_address>

### Comment 3
<location> `src/ir.rs:174` </location>
<code_context>
         }
     }
+
+    fn detect_cycles(&self) -> Result<(), IrGenError> {
+        #[derive(Clone, Copy)]
+        enum State {
</code_context>

<issue_to_address>
The module is missing a `//!` comment at the top as required by the review instructions.

Please add a `//!` module-level comment at the very top of this file to describe its purpose, as per the project guidelines.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/ir.rs Outdated
Comment thread src/ir.rs
Comment thread src/ir.rs
Comment thread src/ir.rs
Copy link
Copy Markdown

@codescene-delta-analysis codescene-delta-analysis Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gates Failed
Enforce critical code health rules (1 file with Bumpy Road Ahead)
Enforce advisory code health rules (1 file with Complex Method)

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Reason for failure
Enforce critical code health rules Violations Code Health Impact
ir.rs 1 critical rule 10.00 → 9.54 Suppress
Enforce advisory code health rules Violations Code Health Impact
ir.rs 1 advisory rule 10.00 → 9.54 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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/ir.rs (1)

283-338: Refactor to reduce complexity below threshold

The cycle detection has cyclomatic complexity of 9 and nested conditionals, violating project standards. Extract the DFS logic into a separate module-level function or use Kahn's algorithm as suggested in previous reviews.

Example refactor extracting DFS:

-#[derive(Clone, Copy)]
-enum VisitState {
-    Visiting,
-    Visited,
-}
-
-fn find_cycle(targets: &HashMap<PathBuf, BuildEdge>) -> Option<Vec<PathBuf>> {
-    fn visit(
-        targets: &HashMap<PathBuf, BuildEdge>,
-        node: &PathBuf,
-        stack: &mut Vec<PathBuf>,
-        states: &mut HashMap<PathBuf, VisitState>,
-    ) -> Option<Vec<PathBuf>> {
-        match states.get(node) {
-            Some(VisitState::Visited) => return None,
-            Some(VisitState::Visiting) => {
-                if let Some(idx) = stack.iter().position(|n| n == node) {
-                    let mut cycle = stack.get(idx..).expect("slice").to_vec();
-                    cycle.push(node.clone());
-                    return Some(cycle);
-                }
-                return Some(vec![node.clone()]);
-            }
-            None => {}
-        }
-
-        states.insert(node.clone(), VisitState::Visiting);
-        stack.push(node.clone());
-
-        if let Some(edge) = targets.get(node) {
-            for dep in &edge.inputs {
-                if targets.get(dep).is_some()
-                    && let Some(cycle) = visit(targets, dep, stack, states)
-                {
-                    return Some(cycle);
-                }
-            }
-        }
-
-        stack.pop();
-        states.insert(node.clone(), VisitState::Visited);
-        None
-    }
-
-    let mut states = HashMap::new();
-    let mut stack = Vec::new();
-
-    for node in targets.keys() {
-        if !states.contains_key(node)
-            && let Some(cycle) = visit(targets, node, &mut stack, &mut states)
-        {
-            return Some(cycle);
-        }
-    }
-    None
-}
+// At module level for testability
+#[derive(Clone, Copy)]
+enum VisitState {
+    Visiting,
+    Visited,
+}
+
+fn visit_node(
+    targets: &HashMap<PathBuf, BuildEdge>,
+    node: &PathBuf,
+    stack: &mut Vec<PathBuf>,
+    states: &mut HashMap<PathBuf, VisitState>,
+) -> Option<Vec<PathBuf>> {
+    match states.get(node) {
+        Some(VisitState::Visited) => return None,
+        Some(VisitState::Visiting) => {
+            let idx = stack.iter().position(|n| n == node).unwrap_or(0);
+            let mut cycle = stack[idx..].to_vec();
+            cycle.push(node.clone());
+            return Some(cycle);
+        }
+        None => {}
+    }
+
+    states.insert(node.clone(), VisitState::Visiting);
+    stack.push(node.clone());
+
+    if let Some(edge) = targets.get(node) {
+        for dep in &edge.inputs {
+            if targets.contains_key(dep) {
+                if let Some(cycle) = visit_node(targets, dep, stack, states) {
+                    return Some(cycle);
+                }
+            }
+        }
+    }
+
+    stack.pop();
+    states.insert(node.clone(), VisitState::Visited);
+    None
+}
+
+fn find_cycle(targets: &HashMap<PathBuf, BuildEdge>) -> Option<Vec<PathBuf>> {
+    let mut states = HashMap::new();
+    let mut stack = Vec::new();
+
+    for node in targets.keys() {
+        if !states.contains_key(node) {
+            if let Some(cycle) = visit_node(targets, node, &mut stack, &mut states) {
+                return Some(cycle);
+            }
+        }
+    }
+    None
+}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54ca782 and 9aba499.

📒 Files selected for processing (3)
  • docs/netsuke-design.md (2 hunks)
  • src/ir.rs (4 hunks)
  • tests/ir_from_manifest_tests.rs (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

📄 CodeRabbit Inference Engine (AGENTS.md)

**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type 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 for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.

Files:

  • tests/ir_from_manifest_tests.rs
  • 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 return in single-line functions.

  • Move conditionals with >2 branches into a predicate function.

  • Avoid unsafe unless 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.
  • Use rstest fixtures for shared setup and to avoid repetition between tests.

  • Replace duplicated tests with #[rstest(...)] parameterised cases.

  • Prefer mockall for 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.

Files:

  • tests/ir_from_manifest_tests.rs
  • src/ir.rs
docs/**/*.md

📄 CodeRabbit Inference Engine (AGENTS.md)

docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in the docs/ directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.

Files:

  • docs/netsuke-design.md
**/*.md

📄 CodeRabbit Inference Engine (AGENTS.md)

**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.

Files:

  • docs/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
  • Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
  • Code blocks should be wrapped to 120 columns.
  • 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
🪛 GitHub Actions: CI
tests/ir_from_manifest_tests.rs

[error] 101-101: Test failure: assertion left == right failed in manifest_error_cases::case_6. Expected ["a", "b", "a"], got ["b", "a", "b"]

src/ir.rs

[error] 369-369: Test failure: assertion left == right failed in ir::tests::find_cycle_identifies_cycle. Expected ["a", "b", "a"], got ["b", "a", "b"]

🔇 Additional comments (6)
src/ir.rs (2)

97-98: Good improvement to error reporting!

Including the full cycle path in the error variant provides better debugging information than a single node.


174-179: Clean delegation pattern

The method correctly delegates to find_cycle and converts the result appropriately.

tests/ir_from_manifest_tests.rs (2)

42-42: Correct addition of test variant

The new variant properly extends the test enum to handle circular dependency errors.


69-72: Test case properly covers circular dependency scenario

The test correctly exercises the circular dependency detection with the circular.yml fixture.

docs/netsuke-design.md (2)

992-996: Clear and accurate documentation of cycle detection

The documentation properly describes the algorithm, design decisions, and error reporting improvements.


1208-1209: Documentation correctly updated for new error variant

The error variant documentation accurately reflects the change from single path to cycle vector.

Comment thread src/ir.rs
Comment thread tests/ir_from_manifest_tests.rs
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/ir.rs (1)

386-391: Fix test to handle non-deterministic HashMap iteration

The cycle detection order depends on HashMap iteration, which is non-deterministic. Even with canonicalization, verify that the cycle is valid rather than expecting exact order.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9aba499 and 0cbaaf8.

📒 Files selected for processing (2)
  • docs/netsuke-design.md (2 hunks)
  • src/ir.rs (4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md

📄 CodeRabbit Inference Engine (AGENTS.md)

docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Proactively update the relevant file(s) in the docs/ directory to reflect the latest state when new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve.

Files:

  • docs/netsuke-design.md
**/*.md

📄 CodeRabbit Inference Engine (AGENTS.md)

**/*.md: Documentation must use en-GB-oxendict spelling and grammar, except for the naming of the "LICENSE" file.
Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.

Files:

  • docs/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
  • Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
  • Code blocks should be wrapped to 120 columns.
  • 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: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type 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 for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.

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 return in single-line functions.

  • Move conditionals with >2 branches into a predicate function.

  • Avoid unsafe unless 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.
  • Use rstest fixtures for shared setup and to avoid repetition between tests.

  • Replace duplicated tests with #[rstest(...)] parameterised cases.

  • Prefer mockall for 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.

Files:

  • src/ir.rs
🔇 Additional comments (5)
src/ir.rs (4)

96-99: LGTM!

The new CircularDependency error variant follows the established pattern and provides clear information about the detected cycle.


116-117: LGTM!

Cycle detection is correctly placed after graph construction and uses proper error propagation.


173-180: LGTM!

Clean implementation that delegates to find_cycle and properly converts the result to an error.


340-356: LGTM!

The canonicalization logic correctly ensures deterministic cycle representation by rotating to start with the lexicographically smallest node.

docs/netsuke-design.md (1)

1210-1211: LGTM!

The error variant documentation correctly reflects the implementation change from a single path to a vector of paths representing the full cycle.

Comment thread docs/netsuke-design.md
Comment thread src/ir.rs Outdated
@leynos leynos merged commit bd898d5 into main Jul 30, 2025
3 checks passed
@leynos leynos deleted the codex/implement-cycle-detection-algorithm branch July 30, 2025 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant