diff --git a/tests/auto_agents/controller/test_conflict_rebase.py b/tests/auto_agents/controller/test_conflict_rebase.py index eb8b3af01..35bbb4013 100644 --- a/tests/auto_agents/controller/test_conflict_rebase.py +++ b/tests/auto_agents/controller/test_conflict_rebase.py @@ -36,7 +36,7 @@ def _git(cwd, *args: str) -> str: ).stdout -def _build(tmp_path, *, conflicting: bool, extra_master_file=None): +def _build(tmp_path, *, conflicting: bool, extra_master_file=None, extra_pr_file=None): """Bare remote + a conflict-resolver worktree checked out at a PR branch that, rebased onto master, either conflicts or doesn't. @@ -76,6 +76,13 @@ def _build(tmp_path, *, conflicting: bool, extra_master_file=None): (seed / "shared.txt").write_text("PR version of the line\n") else: (seed / "prfile.txt").write_text("a brand-new PR-only file\n") + # An extra file the PR branch itself carries — it lands IN the PR's + # diff vs master (unlike extra_master_file, which is in the base). + if extra_pr_file is not None: + name, content = extra_pr_file + path = seed / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) _git(seed, "add", ".") _git(seed, "commit", "-m", "pr change") _git(seed, "push", "origin", "pr-branch") @@ -218,6 +225,7 @@ class TestFinalizeConflictResolution: head_ref="pr-branch", expected_remote_sha=pr_head, base_branch="master", + resolved_files=["shared.txt"], ) def test_unrelated_marker_file_does_not_false_positive(self, tmp_path): @@ -239,11 +247,41 @@ class TestFinalizeConflictResolution: head_ref="pr-branch", expected_remote_sha=pr_head, base_branch="master", + resolved_files=["shared.txt"], ) # The clean resolution landed — the unrelated marker file was # outside the PR's diff and correctly ignored. assert _git(bare, "rev-parse", "pr-branch").strip() == new_sha + def test_marker_file_in_pr_diff_but_unresolved_does_not_false_positive( + self, tmp_path + ): + """Production regression (run-9: 125 false rejections): a file + the PR itself carries that legitimately contains conflict-marker + text (e.g. an agent def documenting markers) is IN the PR's diff + but is NOT a resolved file. The marker check must grep only what + the resolution actually touched and ignore it.""" + bare, worktree, pr_head = _build( + tmp_path, + conflicting=True, + extra_pr_file=( + "agents/git-rebase-util.md", + "Conflict markers look like:\n<<<<<<< HEAD\nx\n>>>>>>> branch\n", + ), + ) + prepare_conflict_rebase(worktree, base_branch="master") + _resolve_and_continue(worktree) # cleanly resolves shared.txt + new_sha = finalize_conflict_resolution( + worktree, + head_ref="pr-branch", + expected_remote_sha=pr_head, + base_branch="master", + resolved_files=["shared.txt"], + ) + # The marker-doc file is in the PR diff but not in resolved_files + # → ignored → the clean resolution landed. + assert _git(bare, "rev-parse", "pr-branch").strip() == new_sha + # ─── synth_resolved_output ──────────────────────────────────────────── @@ -532,6 +570,7 @@ class TestFinalizeMergeTrack: expected_remote_sha=pr_head, base_branch="master", track="merge", + resolved_files=["shared.txt"], ) diff --git a/tools/controller/worker/conflict_rebase.py b/tools/controller/worker/conflict_rebase.py index edb1d0b47..dca738107 100644 --- a/tools/controller/worker/conflict_rebase.py +++ b/tools/controller/worker/conflict_rebase.py @@ -352,6 +352,7 @@ def finalize_conflict_resolution( expected_remote_sha: str, base_branch: str, track: str = "rebase", + resolved_files: list[str] | None = None, ) -> str: """Verify the agent's resolution and land it on the PR head branch. @@ -370,6 +371,12 @@ def finalize_conflict_resolution( forwardable superset of the old tip); a force-push would be *wrong*, so it uses a **plain** ``git push``. + ``resolved_files`` is the set of files this resolution actually + touched. The committed-conflict-marker check greps ONLY those — a + leftover marker can only be in a file the agent resolved. + ``None``/empty skips that check (a clean rebase/merge resolved + nothing). + Raises ``RuntimeError`` if the resolution is incomplete (rebase/ merge not finished, unmerged files, conflict markers committed, dirty tree) OR the push is rejected / cannot be confirmed on the @@ -393,27 +400,19 @@ def finalize_conflict_resolution( f"worktree not clean after resolution: {status[:200]}" ) # git status is clean even if the agent *committed* conflict-marker - # garbage — catch that. Scope the grep to the PR's own changed - # files: a whole-tree grep false-positives on any repo file that - # legitimately documents conflict markers (e.g. a git-rebase agent - # def), which would reject *every* resolution. - # - # ``origin/..HEAD`` is the PR's changed-file set for BOTH - # tracks: a rebased HEAD sits directly on origin/, and a merged - # HEAD has origin/ as a parent — so in either case - # origin/ is an ancestor of HEAD and this endpoint diff is - # exactly the PR's contribution. - changed = [ - ln.strip() - for ln in _git( - worktree, "diff", "--name-only", f"origin/{base_branch}..HEAD" - ).splitlines() - if ln.strip() - ] + # garbage — catch that. Grep ONLY the files this resolution actually + # touched (``resolved_files``): a leftover marker can only be in a + # file the agent resolved. Grepping the whole PR diff false-positived + # on any unrelated file that legitimately contains marker-like lines + # — e.g. an agent def that *documents* conflict markers — which on a + # large sentinel PR's 400+-file diff rejected every resolution. + # ``None``/empty (a clean rebase/merge — nothing was conflicted) + # means there is nothing to check. if ( - changed + resolved_files and _git_rc( - worktree, "grep", "-lE", r"^(<<<<<<<|>>>>>>>) ", "HEAD", "--", *changed + worktree, "grep", "-lE", r"^(<<<<<<<|>>>>>>>) ", "HEAD", + "--", *resolved_files, ) == 0 ): @@ -567,12 +566,15 @@ def run_conflict_resolver_attempt( "-m", f"Merge origin/{base_branch} into {head_ref}", ) + # Clean prep — no conflicts were resolved, so there is no + # committed-marker check to do. new_sha = finalize_conflict_resolution( worktree, head_ref=head_ref, expected_remote_sha=head_sha, base_branch=base_branch, track=track, + resolved_files=[], ) except RuntimeError as exc: _abort(track) @@ -606,6 +608,12 @@ def run_conflict_resolver_attempt( # 3. The agent only decides the merge — the worker lands it. if result.get("outcome") == "resolved": _check_lock() + # The committed-marker check greps only what this resolution + # actually touched: the prep-time conflicted set (controller- + # trusted) plus whatever files the agent reports it modified. + resolved = sorted( + set(conflicted) | set(result.get("files_modified") or []) + ) try: new_sha = finalize_conflict_resolution( worktree, @@ -613,6 +621,7 @@ def run_conflict_resolver_attempt( expected_remote_sha=head_sha, base_branch=base_branch, track=track, + resolved_files=resolved, ) except RuntimeError as exc: _abort(track)