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