Skip to content

fix: keep graph-backend plans acyclic - #62

Merged
vitali87 merged 3 commits into
mainfrom
fix/graph-partition-acyclic
Aug 31, 2026
Merged

fix: keep graph-backend plans acyclic#62
vitali87 merged 3 commits into
mainfrom
fix/graph-partition-acyclic

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Problem

The graph partition backend could emit a plan with a dependency cycle. The greedy grouper (_group_units_graph) may put file A's earlier hunk and file B's later hunk in one group and the reverse in another; _derive_merge_order_dependencies then produces e.g.

pr-1 depends_on [pr-5]  lib/f1.py [1,2]
pr-2 depends_on [pr-1]  lib/f1.py [3], lib/f2.py [0,1,2]
pr-5 depends_on [pr-2]  lib/f1.py [0], lib/f2.py [3]

Nothing in partition_diff checks for this, and score_plan (called by plan_split before validate_plan ever runs) then crashes with a raw graphlib.CycleError traceback. Found by fuzzing: 1 in 400 random diffs with --priority logical.

Fix

  • New _merge_order_is_acyclic(grouped_units) mirrors the dependency derivation (per-file order by earliest unit) and checks it with graphlib.
  • _group_units_graph rejects a candidate that would create a cycle. The check only runs when the candidate's file already appears in a finished group, since that is the only way a new edge is introduced.
  • _best_graph_merge_target rejects a min_loc merge that would create a cycle.
  • partition_diff validates the final DAG as a backstop, so a backend that still yields a cycle raises PlanValidationError(CYCLE_DETECTED) instead of a traceback.

Verification

  • Regression test with the exact units from the fuzz counter-example, plus unit tests for the helper (2- and 3-group cycles), the min_loc merge path, and the backstop.
  • Re-ran the fuzzer: 0 cyclic plans in 800 runs (both priorities, with and without min_loc).
  • 450 tests pass, ruff check/format clean.

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 23 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2e875523-99e5-4566-a4f8-66bca920390f

📥 Commits

Reviewing files that changed from the base of the PR and between 2ddb9e2 and 14831cf.

📒 Files selected for processing (2)
  • pr_split/planner/partitioning.py
  • tests/test_partitioning.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Aug 30, 2026

Copy link
Copy Markdown

Greptile Summary

This change prevents graph-based partitioning and minimum-size repair from creating circular merge dependencies, then validates the completed plan before returning it. The previously cycle-prone multi-file/multi-hunk behavior was exercised through the public partitioning interface, along with a real minimum-size repair merge and 80 deterministic inputs; every emitted plan had a valid merge order.

Confidence Score: 5/5

Safe to merge: exercised graph-partition plans remained acyclic across the historical reproduction, repair behavior, and deterministic multi-file inputs.

No actionable defects remain after executing the focused partitioning checks. The test source and captured results show that the exercised failure path now produces valid plans.

Files Needing Attention: No files need follow-up changes.

T-Rex T-Rex Logs

What T-Rex did

  • Ran focused pytest validation for GRAPH merge-order cycle using partition_diff and independently validated every returned plan with PlanDAG.
  • The checks covered the historical cycle-prone input, a minimum-size repair merge, and 80 deterministic inputs, and pytest reported 3 passed with no cyclic plan emitted.
  • The after-PR run completed all exercised paths without a cycle, while the parent revision reproduced cyclic plans.
  • The after-PR validation also produced external evidence artifacts, including an external Python source and a validation log accessible via URL.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix: keep graph-backend plans acyclic" | Re-trigger Greptile

The greedy grouper could put file A's earlier hunk and file B's later hunk
in one group and the reverse in another; the file-order dependency
derivation then produced a cycle, which surfaced later as a raw
graphlib.CycleError traceback from score_plan.

- _merge_order_is_acyclic mirrors _derive_merge_order_dependencies and
  is consulted before accepting a candidate in _group_units_graph (only
  when the candidate's file already appears in a finished group) and
  before accepting a min_loc merge in _best_graph_merge_target.
- partition_diff validates the final DAG so any backend that still
  produces a cycle raises PlanValidationError(CYCLE_DETECTED) instead of
  a traceback.

Fuzzing 800 random diffs across both priorities and min_loc settings
produced 0 cyclic plans (1/400 before).
@vitali87
vitali87 force-pushed the fix/graph-partition-acyclic branch from d64fdb8 to 00d9c05 Compare August 30, 2026 13:05
@vitali87
vitali87 merged commit 93cf0fb into main Aug 31, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant