fix(cargo-ensure-no-cyclic-deps): trace directed cycles in forward dependency order

[Austin (AustinAviv)]

Summary of Changes

Fixes #232

• Trace actual forward graph edges within multi-node SCCs instead of using raw tarjan_scc stack-pop order.
• Canonicalize cycle formatting so the lexicographically lowest package name leads the cycle.
• Strengthen integration test assertions to verify the exact formatted cycle path instead of only testing word presence.

Validation

• Ran cargo test -p cargo-ensure-no-cyclic-deps (all unit and integration tests pass).
• Ran cargo clippy -p cargo-ensure-no-cyclic-deps --all-targets -- -D warnings (clean).
• Verified tests/fixtures/with_cycle now correctly outputs crate_a -> crate_b -> crate_c -> crate_a instead of crate_c -> crate_b -> crate_a -> crate_c.

[Copilot]

Copilot review overview

🟡 Changes recommended

A self-loop on the selected SCC start can suppress the multi-node cycle and produce malformed duplicate output.

Review effort: Balanced
Findings: 1

Open (1)

• Unvisited SCC start lets self-loop mask multi-node cycle · New

What changed in this PR

Fixes cycle output to follow actual dependency direction and use deterministic formatting.

Changes:

• Reconstructs directed cycle paths within SCCs.
• Canonicalizes output and strengthens integration assertions.

File	Description
src/​lib.rs	Adds cycle traversal and canonical formatting.
tests/​integration_tests.rs	Verifies the exact forward cycle path.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Cycle reconstruction has quadratic worst-case work, and branched SCC behavior remains untested.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Unvisited SCC start lets self-loop mask multi-node cycle

Previously missed (2)

In code that hasn't changed since last review

Avoid O(N²) path cloning during cycle traversal

crates/​cargo_ensure_no_cyclic_deps/​src/​lib.rs:218

Each newly discovered node clones its entire path. A long SCC such as an N-package ring therefore turns the cycle reconstruction from linear work into O(N²) copied indices and allocations. Keep one predecessor per visited node and reconstruct the path only after finding the edge back to start.

Add branched SCC fixture asserting valid dependency transitions

crates/​cargo_ensure_no_cyclic_deps/​tests/​integration_tests.rs:40

This assertion covers reversed ordering in a simple ring, but not the branched/multi-cycle SCC case called out in the issue, where raw SCC order can imply edges that do not exist. Add a fixture with a branched SCC and assert an exact path whose every transition is a declared dependency; that specifically protects the new traversal rather than only its rotation behavior.

[Austin (AustinAviv)]

@microsoft-github-policy-service agree

[Copilot]

Copilot review overview

🟡 Changes recommended

The branched-SCC behavior central to the fix lacks targeted regression coverage.

Review effort: Balanced
Findings: 1

Open (1)

• Add branched-SCC test for intersecting cycles · New

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused implementation correctly addresses the reported ordering defect with appropriate regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Add branched-SCC test for intersecting cycles

[Copilot]

Copilot review overview

🟡 Changes recommended

The regression test still uses substring matching and can accept malformed cycle output.

Review effort: Balanced
Findings: 1

Open (1)

• Assert exact cycle-path formatting in stderr regression test · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

The regression test still uses substring matching rather than enforcing the exact output contract.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Assert exact cycle-path formatting in stderr regression test

[Copilot]

Copilot review overview

🔵 Needs a closer look

The integration test still uses substring matching rather than validating the complete formatted output.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Assert the deterministic expected cycle

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.9%. Comparing base (3cd0306) to head (5886090).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/cargo_ensure_no_cyclic_deps/src/lib.rs	92.5%	2 Missing ⚠️

❌ Your project status has failed because the head coverage (97.9%) is below the target coverage (98.7%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #233      +/-   ##
========================================
- Coverage   98.9%   97.9%    -1.1%     
========================================
  Files        304       2     -302     
  Lines      44436      97   -44339     
========================================
- Hits       43975      95   -43880     
+ Misses       461       2     -459     

Flag	Coverage Δ
linux	97.9% <92.5%> (-1.1%)	⬇️
linux-arm	97.9% <92.5%> (-1.1%)	⬇️
scheduled	?
windows	97.9% <92.5%> (-1.0%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Austin (AustinAviv)]

Hi Pato Sandaña (@psandana) martin-kolinek,

Thanks for reviewing and approving the changes!

All unit and integration tests pass locally, and the MSRV and mutation test suites in CI are green. However, Fast Checks and Tests and Coverage are currently failing in the Anvil runner. As an outside contributor, I cannot see the detailed runner logs for those jobs.

Could you please check what Anvil is reporting for those steps, or let me know if any formatting or test coverage adjustments are needed?

Thanks!

[Pato Sandaña (psandana)]

Hi Pato Sandaña (Pato Sandaña (@psandana)) martin-kolinek,

Thanks for reviewing and approving the changes!

All unit and integration tests pass locally, and the MSRV and mutation test suites in CI are green. However, Fast Checks and Tests and Coverage are currently failing in the Anvil runner. As an outside contributor, I cannot see the detailed runner logs for those jobs.

Could you please check what Anvil is reporting for those steps, or let me know if any formatting or test coverage adjustments are needed?

Thanks!

  error: spellcheck(Hunspell)
      --> /home/runner/work/ox-tools/ox-tools/crates/cargo_ensure_no_cyclic_deps/src/lib.rs:196
       |
   196 |  Finds a directed cycle path within an SCC by traversing graph edges.
       |                                        ^^^
       | - CC, SEC, SCI, SIC, SAC, SOC, SCH, or one of 4 others
       |
       |   Possible spelling mistake found.
  
  25herror: recipe `anvil-spellcheck` failed with exit code 1

Spell checker. Just add SCC to our list of words in https://github.com/microsoft/ox-tools/blob/main/.spelling. Be sure to add it in order (lexicographical)

[Copilot]

🟢 Approval recommended

The implementation preserves directed edges, handles branching SCCs, and covers the reported regression.

0 open findings

🧠 Review effort: Balanced

[Austin (AustinAviv)]

Hi Pato Sandaña (Pato Sandaña (Pato Sandaña (@psandana))) martin-kolinek,
Thanks for reviewing and approving the changes!
All unit and integration tests pass locally, and the MSRV and mutation test suites in CI are green. However, Fast Checks and Tests and Coverage are currently failing in the Anvil runner. As an outside contributor, I cannot see the detailed runner logs for those jobs.
Could you please check what Anvil is reporting for those steps, or let me know if any formatting or test coverage adjustments are needed?
Thanks!

  error: spellcheck(Hunspell)
      --> /home/runner/work/ox-tools/ox-tools/crates/cargo_ensure_no_cyclic_deps/src/lib.rs:196
       |
   196 |  Finds a directed cycle path within an SCC by traversing graph edges.
       |                                        ^^^
       | - CC, SEC, SCI, SIC, SAC, SOC, SCH, or one of 4 others
       |
       |   Possible spelling mistake found.
  
  25herror: recipe `anvil-spellcheck` failed with exit code 1

Spell checker. Just add SCC to our list of words in https://github.com/microsoft/ox-tools/blob/main/.spelling. Be sure to add it in order (lexicographical)

Added SCC to .spelling in lexicographical order right before SCCACHE. Thank you for the quick pointer!

[Austin (AustinAviv)]

Hi Pato Sandaña (Pato Sandaña (Pato Sandaña (Pato Sandaña (@psandana)))) martin-kolinek,
Thanks for reviewing and approving the changes!
All unit and integration tests pass locally, and the MSRV and mutation test suites in CI are green. However, Fast Checks and Tests and Coverage are currently failing in the Anvil runner. As an outside contributor, I cannot see the detailed runner logs for those jobs.
Could you please check what Anvil is reporting for those steps, or let me know if any formatting or test coverage adjustments are needed?
Thanks!

  error: spellcheck(Hunspell)
      --> /home/runner/work/ox-tools/ox-tools/crates/cargo_ensure_no_cyclic_deps/src/lib.rs:196
       |
   196 |  Finds a directed cycle path within an SCC by traversing graph edges.
       |                                        ^^^
       | - CC, SEC, SCI, SIC, SAC, SOC, SCH, or one of 4 others
       |
       |   Possible spelling mistake found.
  
  25herror: recipe `anvil-spellcheck` failed with exit code 1

Spell checker. Just add SCC to our list of words in https://github.com/microsoft/ox-tools/blob/main/.spelling. Be sure to add it in order (lexicographical)

Added SCC to .spelling in lexicographical order right before SCCACHE. Thank you for the quick pointer!

Hi Pato Sandaña (@psandana),

Thanks! Adding SCC to .spelling fixed the spellchecker, and all Fast Checks are now green (24 of 28 checks passing).

The only remaining failure is in Tests and Coverage (anvil-llvm-cov coverage gate). Could you let me know what you'd like done for the coverage gate here, or if there is a specific test case you would prefer added? Also happy to update the branch with main whenever you're ready.

Thanks!

[Copilot]

🔵 Needs a closer look

The new regression assertions should verify deterministic, exact outcomes as required by the repository test policy.

0 open findings

🧠 Review effort: Balanced

[Sergey Galkin (sgalkin)]

The forward-edge traversal and the branching SCC regression look right. A couple of nonblocking suggestions for 9ab544e:

• In the combined self-loop/branching test, could we insert graph.add_edge(a, a, ()) after the branch edges? Petgraph 0.8.3 visits the most recently added outgoing edge first. With the current insertion order, the FIFO traversal without the start seed reaches a valid closing branch before the duplicated-start path, so this construction does not distinguish omission of that guard. Adding the self-loop last strengthens that regression without changing the graph or requiring a particular branch: both [a, b] and [a, c] remain valid.
• Optionally, could the crate documentation briefly clarify that reports show one representative directed cycle per multi-node SCC, with self-loops separate? Fixing the displayed cycle can leave another cycle in the same component. This is a small reporting clarification, not a request for exhaustive enumeration or stable ordering.

The existing exact-stderr feedback also remains applicable: the ring regression still uses contains at integration_tests.rs:42. Complete stderr equality would check the whole diagnostic; referencing that thread here rather than adding a duplicate finding.

🤖 Co-authored with Copilot.

[Sergey Galkin (sgalkin)]

The directed-cycle fix looks correct. The remaining test and documentation suggestions are nonblocking.

🤖 Co-authored with Copilot.

[Copilot]

🟢 Approval recommended

The implementation produces valid forward-edge cycles and the updated tests cover both the reported regression and branching SCC behavior.

0 open findings

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Copilot]

🟡 Changes recommended

The generated crate README must be updated to include the new cycle-reporting documentation.

1 open finding

• Generated crate README omits cycle-reporting contract · New

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

[Austin (AustinAviv)]

Thanks martinhavelka (@wukchung) and Sergey Galkin (@sgalkin)!

Pushed commit 4569f45 addressing all feedback:

• Replaced the unreachable fallback in find_cycle_in_scc and min_by_key with invariant .expect(...) calls documenting the preconditions.
• Added an external non-SCC neighbor edge in the unit test to exercise the !scc_set.contains branch.
• Inserted graph.add_edge(a, a, ()) after the branch edges to strengthen the self-loop guard test under Petgraph edge visitation order.
• Clarified representative cycle reporting in the crate documentation.
• Switched integration test stderr assertions to predicate::str::diff for complete diagnostic validation.

[Copilot]

🔵 Needs a closer look

The generated crate README must be regenerated to include the new reporting contract.

0 open findings

1 resolved since last review

• Generated crate README omits cycle-reporting contract

🧠 Review effort: Balanced

---

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.