Add CycleAwareDependencyGraphDumper to visualize cycles (resolves #1561)

[Jimin15]

Following this checklist to help us incorporate your
contribution quickly and easily:

• Your pull request should address just one issue, without pulling in other changes.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.

Summary

This PR adds CycleAwareDependencyGraphDumper, a new dependency visitor that visualizes cycles in dependency graphs while preventing StackOverflow errors. This addresses issue #1561.

What

Added CycleAwareDependencyGraphDumper class that wraps DependencyGraphDumper and adds cycle detection capabilities- When a cycle is detected, it displays the cycle with ^N notation where N is the index of the node in the path that it cycles back to- The visitor stops traversing children of cycle nodes to prevent infinite recursion

Why

Currently, DependencyGraphDumper fails with StackOverflowError when visualizing dependency graphs in FULL verbosity mode that contain cycles. While TreeDependencyVisitor can prevent the error, it doesn't visualize cycles. This new visitor provides both cycle visualization and StackOverflow prevention.

How

The implementation uses a Deque to track the current path during traversal- Cycle detection is performed by comparing versionless artifact IDs using ArtifactIdUtils.equalsVersionlessId()- When a cycle is detected, custom indentation formatting is applied to match the tree structure- The visitor delegates to DependencyGraphDumper for normal nodes and handles cycle nodes separately

Testing

Added comprehensive unit tests in CycleAwareDependencyGraphDumperTest- Tests verify: cycle detection, visualization format, StackOverflow prevention, and compatibility with non-cycle graphs- All tests pass successfully

Related Issue

Resolves #1561

• Each commit in the pull request should have a meaningful subject line and body.
  Note that commits might be squashed by a maintainer on merge.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
  This may not always be possible but is a best-practice.
• Run mvn verify to make sure basic checks pass.
  A more thorough check will be performed on your pull request automatically.
• You have run the integration tests successfully (mvn -Prun-its verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

• I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
• In any other case, please file an Apache Individual Contributor License Agreement.

[cstamas]

PR has formatting issues, did you build it locally?

[kwin]

Where is this supposed to be used?

[cstamas]

Usage can be really anywhere, for example Toolbox dirty-tree mojo...
maveniverse/toolbox#318 (is not using it, nor is displaying cycles yet)

But any reporting plugin that may show dirty tree (like m-dependency-p?)

[gnodet]

AI Review — PR #1758: Add CycleAwareDependencyGraphDumper

Hi @Jimin15, thanks for tackling cycle visualization in dependency graphs — this addresses a real need (issue #1561)!

After review and independent verification, a few issues stood out:

⚠️ CI is red — needs rebase (high)

All 12 Verify jobs fail. The PR branch is significantly behind master. A local build/test run of the PR code itself passes (413 tests, 0 failures), so the failure is likely caused by the stale branch base rather than your code. A rebase onto current master should fix CI.

⚠️ Cycle index semantics are misleading (medium)

findCycleInPath returns an index where 0 is the parent (most recent ancestor), not the root — because ArrayDeque.push adds to the front. The code comment on line 144 says "0-based, root is 0" which is incorrect. The ^N notation in output is internally consistent but could confuse users who expect a root-relative index. The test assertion (cycleIndex >= 0 && cycleIndex < output.size()) is also too weak to validate semantic correctness of the index.

⚠️ isLastChild false positive risk (medium)

The equalsVersionlessId fallback in isLastChild compares groupId:artifactId:extension:classifier but not version. If siblings share the same GA but differ in version (plausible in FULL verbosity mode), isLastChild could return true for a non-last child, producing incorrect tree indentation (\- instead of +-).

💡 Design: composition with tight coupling (medium)

The class uses composition (new DependencyGraphDumper(consumer)) but calls protected methods (dumper.formatNode) and relies on knowledge of internal deque semantics. This works because both classes are in the same package, but it's fragile — if formatNode were changed to reference internal state beyond peek(), this would break. Consider inheritance or a cleaner decorator pattern.

What works well:

• The core cycle detection logic is correct — the verifier confirmed that formatNode(currentPath) correctly peeks the cycle node
• The enter/leave delegation is properly paired (both skipped for cycle nodes, both called for non-cycle nodes)
• Test coverage for basic scenarios is solid

---

🤖 This review was generated by ForgeBot using a maker/checker pattern (reviewer + independent verifier). 2 of 6 original findings were removed as false positives after verification.

[gnodet]

Review Summary

Thanks for working on this, @Jimin15! A cycle-aware graph dumper is a useful addition for debugging dependency resolution. The composition-based approach wrapping DependencyGraphDumper is a reasonable design choice.

After review and independent verification, I found several confirmed issues:

Medium Severity

1. Misleading cycle index semantics — The comment on findCycleInPath says "0-based, root is 0" but ArrayDeque.push() adds to the head, and the for-each iterator starts from the head. So index 0 actually corresponds to the most recently pushed node (direct parent), not the root. The ^N notation in output will show an index relative to the top of the stack, which may confuse users.

2. isLastChild versionless ID fallback — The equalsVersionlessId() fallback ignores version, so if two siblings share the same groupId:artifactId but differ in version (possible in FULL verbosity mode), the wrong sibling could match, producing incorrect tree indentation.

3. Incorrect @since 2.0.0 tag — Version 2.0.0 was released long ago (latest is 2.0.20). Recently added classes use accurate version tags like @since 2.0.19. This should be updated to the next release version.

4. Weak test assertion in cycleReferencePointsToCorrectIndex — The test checks cycleIndex >= 0 && cycleIndex < output.size(), comparing a path-depth index against the number of output lines. These are unrelated quantities, so the test would pass even with an incorrect cycle index as long as it's a small positive number.

Low Severity

5. Test simplification — The element-by-element loop comparison in dumpSimple could be simplified to assertEquals(standardOutput, cycleAwareOutput) for clearer failure messages.

6. Missing test coverage — The two-argument constructor accepting decorators has no test coverage.

Notes

• The reviewer initially flagged a potential "stack desynchronization bug" in visitLeave, but independent verification confirmed this is not a bug — the DependencyVisitor contract guarantees matching visitEnter/visitLeave pairs, so both stacks remain synchronized.
• The composition approach using protected methods from DependencyGraphDumper is a valid design choice since both classes are co-located in the same package.

---

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

[gnodet]

Thanks for working on cycle visualization — this addresses a real need (issue #1561). The implementation has a few issues that should be addressed before merge.

Correctness bug:

• formatCycleIndentation (line 172) uses identity comparison child == cycleNode to determine tree-drawing characters. In cycle graphs, DependencyGraphParser reuses the same DependencyNode Java object for ^N references, so the cycle node IS the same object as an earlier ancestor in the path. This causes end=true prematurely at the wrong depth, producing incorrect indentation (e.g., +- instead of | ). Fix: replace with a positional check like depth == path.size() - 2.

Other issues:

• isLastChild versionless ID fallback via equalsVersionlessId() (line 205) ignores version. In FULL verbosity mode, siblings with the same groupId:artifactId but different versions would be misidentified as the last child, producing incorrect \- instead of +-.
• The comment "0-based, root is 0" in findCycleInPath (line 144) is incorrect — ArrayDeque.push() adds to the head, so index 0 is the direct parent, not the root.
• @since 2.0.0 should be @since 2.0.22 (2.0.0 was released long ago; current dev is 2.0.22-SNAPSHOT).
• Test cycleReferencePointsToCorrectIndex compares a path-depth index against output.size() (line count) — these are unrelated quantities, so the assertion is vacuously true. Tests should validate against expected exact output strings.
• All 12 CI Verify jobs are failing — a rebase onto current master is needed.
• The two-argument constructor accepting decorators has no test coverage.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

[gnodet]

The cycle-aware graph dumper has a correctness bug and several issues that should be addressed:

High severity:

• 🐛 Correctness bug in formatCycleIndentation (line 183): The identity comparison child == cycleNode produces incorrect indentation when the same Java object appears at multiple positions in the path (the normal case for cycle graphs). Fix: Replace boolean end = child == cycleNode; with boolean end = !iter.hasNext();.

Medium severity:

• @since 2.0.0 tag is incorrect — the current development version is 2.0.22-SNAPSHOT. Should be @since 2.0.22.
• Comment at line 154 says "0-based, root is 0" but ArrayDeque's for-each iterator goes from head (most recently pushed = direct parent) to tail, so index 0 is the direct parent, not the root. The comment is factually wrong.
• isLastChild versionless ID fallback via equalsVersionlessId() ignores version. In FULL verbosity mode, siblings with the same groupId:artifactId but different versions would be misidentified as the last child.
• cycleReferencePointsToCorrectIndex test compares cycleIndex (path-depth index) against output.size() (total line count) — these are unrelated quantities, making the assertion vacuously true.

Low severity:

• Element-by-element loop comparison in dumpSimple can be simplified to assertEquals(standardOutput, cycleAwareOutput) for clearer failure messages.
• Two-argument constructor accepting decorators has no test coverage.
• dumpCycles test only checks pattern match, not exact output format. A test with expected output strings would have caught the indentation bug.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet