fix: keep graph-backend plans acyclic

pr_split/planner/partitioning.py

@@ -2,16 +2,20 @@
 
 from collections import defaultdict
 from dataclasses import dataclass
+from graphlib import CycleError, TopologicalSorter
 from itertools import pairwise
 from pathlib import PurePosixPath
 from typing import TYPE_CHECKING
 
 from ..constants import AssignmentType, PartitionStrategy, Priority
 from ..exceptions import PRSplitError
+from ..graph import PlanDAG
 from ..schemas import Group, GroupAssignment
 from .chunker import recompute_estimated_loc
 
 if TYPE_CHECKING:
+    from collections.abc import Iterable, Sequence
+
     from ortools.sat.python.cp_model import IntVar
 
     from ..config import Settings
@@ -126,11 +130,40 @@ def _affinity_score(unit_a: PartitionUnit, unit_b: PartitionUnit, priority: Prio
     return score
 
 
+def _merge_order_is_acyclic(grouped_units: Iterable[Sequence[PartitionUnit]]) -> bool:
+    """Check that the merge-order dependencies implied by ``grouped_units`` form a DAG.
+
+    Mirrors ``_derive_merge_order_dependencies``: within each file, groups are ordered by
+    their earliest unit and each group depends on the one before it.
+    """
+    file_occurrences: dict[str, list[tuple[int, int]]] = defaultdict(list)
+    for group_idx, group_units in enumerate(grouped_units):
+        first_positions: dict[str, int] = {}
+        for unit in group_units:
+            previous = first_positions.get(unit.file_path)
+            if previous is None or unit.position < previous:
+                first_positions[unit.file_path] = unit.position
+        for file_path, position in first_positions.items():
+            file_occurrences[file_path].append((position, group_idx))
+
+    sorter: TopologicalSorter[int] = TopologicalSorter()
+    for occurrences in file_occurrences.values():
+        ordered_group_indices = [group_idx for _, group_idx in sorted(occurrences)]
+        for parent_idx, child_idx in pairwise(ordered_group_indices):
+            sorter.add(child_idx, parent_idx)
+    try:
+        sorter.prepare()
+    except CycleError:
+        return False
+    return True
+
+
 def _group_units_graph(
     units: list[PartitionUnit], *, settings: Settings
 ) -> list[list[PartitionUnit]]:
     remaining = set(range(len(units)))
     grouped_units: list[list[PartitionUnit]] = []
+    grouped_files: set[str] = set()
 
     while remaining:
         seed = max(remaining, key=lambda idx: (units[idx].loc, -units[idx].position))
@@ -146,6 +179,10 @@ def _group_units_graph(
                 candidate_unit = units[candidate]
                 if current_load and current_load + candidate_unit.loc > settings.max_loc:
                     continue
+                if candidate_unit.file_path in grouped_files and not _merge_order_is_acyclic(
+                    [*grouped_units, [*(units[idx] for idx in current_group), candidate_unit]]
+                ):
+                    continue
 
                 affinity = sum(
                     _affinity_score(candidate_unit, units[group_idx], settings.priority)
@@ -173,6 +210,7 @@ def _group_units_graph(
         grouped_units.append(
             sorted((units[idx] for idx in current_group), key=lambda unit: unit.position)
         )
+        grouped_files.update(units[idx].file_path for idx in current_group)
 
     return grouped_units
 
@@ -244,6 +282,8 @@ def _best_graph_merge_target(
             continue
         if not _shared_file_merge_is_contiguous(grouped_units, source_idx, target_idx):
             continue
+        if not _merge_order_is_acyclic(_merge_group_units(grouped_units, source_idx, target_idx)):
+            continue
 
         current_underflow = source_underflow + max(0, settings.min_loc - _group_load(target_group))
         merged_underflow = max(0, settings.min_loc - merged_load)
@@ -554,8 +594,10 @@ def partition_diff(parsed_diff: ParsedDiff, settings: Settings) -> list[Group]:
         case _:
             raise PRSplitError(f"Unsupported partition strategy '{settings.partition_strategy}'")
 
-    return _build_groups_from_units(
+    groups = _build_groups_from_units(
         grouped_units,
         parsed_diff,
         backend=settings.partition_strategy,
     )
+    PlanDAG(groups).validate_acyclic()
+    return groups

tests/test_partitioning.py

@@ -5,7 +5,15 @@
 from pr_split.config import Settings
 from pr_split.constants import PartitionStrategy, Priority
 from pr_split.diff_ops.parser import parse_diff
-from pr_split.planner.partitioning import build_partition_units, partition_diff
+from pr_split.exceptions import PlanValidationError
+from pr_split.planner.partitioning import (
+    PartitionUnit,
+    _group_units_graph,
+    _merge_order_is_acyclic,
+    _repair_graph_min_loc,
+    build_partition_units,
+    partition_diff,
+)
 
 SAMPLE_DIFF = """\
 diff --git a/a.py b/a.py
@@ -109,3 +117,107 @@ def test_cp_sat_backend_returns_groups(self, monkeypatch: pytest.MonkeyPatch) ->
         groups = partition_diff(parsed, settings)
         assert len(groups) == 2
         assert all(group.depends_on == [] for group in groups)
+
+
+def _unit(file_path: str, hunks: tuple[int, ...], loc: int, position: int) -> PartitionUnit:
+    return PartitionUnit(
+        id=f"{file_path}:{hunks[0]}-{hunks[-1]}",
+        file_path=file_path,
+        hunk_indices=hunks,
+        loc=loc,
+        position=position,
+    )
+
+
+# Greedy grouping used to produce pr-1 -> pr-5 -> pr-2 -> pr-1 for these units:
+# {f1[1,2]}, {f1[3], f2[0..2]}, {f1[0], f2[3]} invert the f1/f2 order between groups.
+CYCLE_PRONE_UNITS = [
+    _unit("lib/f0.py", (0,), 73, 0),
+    _unit("lib/f1.py", (0,), 84, 1),
+    _unit("lib/f1.py", (1, 2), 126, 2),
+    _unit("lib/f1.py", (3,), 28, 3),
+    _unit("lib/f2.py", (0, 1, 2), 121, 4),
+    _unit("lib/f2.py", (3,), 60, 5),
+    _unit("src/f3.py", (0, 1), 113, 6),
+    _unit("src/f3.py", (2, 3), 104, 7),
+]
+
+
+class TestMergeOrderIsAcyclic:
+    def test_consistent_file_order_is_acyclic(self) -> None:
+        grouped = [
+            [_unit("a.py", (0,), 10, 0), _unit("b.py", (0,), 10, 2)],
+            [_unit("a.py", (1,), 10, 1), _unit("b.py", (1,), 10, 3)],
+        ]
+        assert _merge_order_is_acyclic(grouped) is True
+
+    def test_inverted_file_order_is_cyclic(self) -> None:
+        grouped = [
+            [_unit("a.py", (0,), 10, 0), _unit("b.py", (1,), 10, 3)],
+            [_unit("a.py", (1,), 10, 1), _unit("b.py", (0,), 10, 2)],
+        ]
+        assert _merge_order_is_acyclic(grouped) is False
+
+    def test_three_group_cycle_is_detected(self) -> None:
+        grouped = [
+            [_unit("a.py", (0,), 10, 0), _unit("c.py", (1,), 10, 5)],
+            [_unit("a.py", (1,), 10, 1), _unit("b.py", (0,), 10, 2)],
+            [_unit("b.py", (1,), 10, 3), _unit("c.py", (0,), 10, 4)],
+        ]
+        assert _merge_order_is_acyclic(grouped) is False
+
+
+class TestGraphGroupingStaysAcyclic:
+    def test_greedy_grouping_never_inverts_file_order(self) -> None:
+        settings = Settings(
+            max_loc=150,
+            priority=Priority.LOGICAL,
+            partition_strategy=PartitionStrategy.GRAPH,
+            max_refinement_iterations=0,
+        )
+        grouped = _group_units_graph(CYCLE_PRONE_UNITS, settings=settings)
+        assert _merge_order_is_acyclic(grouped)
+        assert sorted(unit.id for group in grouped for unit in group) == sorted(
+            unit.id for unit in CYCLE_PRONE_UNITS
+        )
+
+    def test_min_loc_repair_refuses_cycle_creating_merge(self) -> None:
+        # g0 is undersized. Merging it into g1 exceeds max_loc; merging it into g2 would
+        # give {a[0], b[1]}, which a.py orders before g1 and b.py orders after g1 -> cycle.
+        grouped = [
+            [_unit("a.py", (0,), 10, 0)],
+            [_unit("a.py", (1,), 95, 1), _unit("b.py", (0,), 0, 2)],
+            [_unit("b.py", (1,), 80, 3)],
+        ]
+        settings = Settings(
+            min_loc=30,
+            max_loc=100,
+            priority=Priority.LOGICAL,
+            partition_strategy=PartitionStrategy.GRAPH,
+            max_refinement_iterations=0,
+        )
+        repaired = _repair_graph_min_loc(grouped, settings=settings)
+        assert repaired == grouped
+        assert _merge_order_is_acyclic(repaired)
+
+    def test_partition_diff_rejects_cyclic_backend_output(
+        self, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        cyclic = [
+            [_unit("a.py", (0,), 10, 0), _unit("b.py", (1,), 10, 3)],
+            [_unit("a.py", (1,), 10, 1), _unit("b.py", (0,), 10, 2)],
+        ]
+        monkeypatch.setattr(
+            "pr_split.planner.partitioning._group_units_graph", lambda units, settings: cyclic
+        )
+        parsed_diff = parse_diff(
+            "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n"
+            "@@ -1,1 +1,2 @@\n x\n+y\n@@ -10,1 +11,2 @@\n x\n+z\n"
+            "diff --git a/b.py b/b.py\n--- a/b.py\n+++ b/b.py\n"
+            "@@ -1,1 +1,2 @@\n x\n+y\n@@ -10,1 +11,2 @@\n x\n+z\n"
+        )
+        settings = Settings(
+            max_loc=400, partition_strategy=PartitionStrategy.GRAPH, max_refinement_iterations=0
+        )
+        with pytest.raises(PlanValidationError, match="cycle"):
+            partition_diff(parsed_diff, settings)