From dac64066a4f4b53cb2c5de65eac892f1dbbd44e0 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Fri, 6 Nov 2026 16:16:56 +0530 Subject: [PATCH] worker: combine task patches with a three-way apply, so a plan with dependent tasks can open its PR Every stored task patch is `git diff HEAD` of a checkout at the plan's base, so a dependent task's patch also carries the changes of the tasks it depends on (they were applied to its sandbox, staged, not committed). The pull-request gate applied the patches in index order with a plain `git apply`, which refused the repeat. On plan 5bbf7d86 (four tasks, three depending on the first) every task passed and the pull request failed with "the change of task 1 does not apply". `git apply --3way` sees the same change on both sides and takes it once; two different changes to the same lines still conflict and fail. Replaying that plan's real stored patches: a plain apply fails on task 1, the three-way apply gives the expected two files, +72 lines, and the repository's tests pass. The worker applies dependency patches with the same method, so a task depending on two tasks that share a dependency no longer fails either. --- control-plane/mergit_worker/workspace.py | 9 +++-- control-plane/tests/test_github.py | 44 ++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/control-plane/mergit_worker/workspace.py b/control-plane/mergit_worker/workspace.py index ada5781d..091350b9 100644 --- a/control-plane/mergit_worker/workspace.py +++ b/control-plane/mergit_worker/workspace.py @@ -242,9 +242,14 @@ def _refuse_nested_git(self) -> None: def apply_patch(self, patch: str) -> None: """Apply a stored task patch onto the checkout and stage it. `git apply` refuses paths inside - `.git`, so a patch cannot plant hooks or config.""" + `.git`, so a patch cannot plant hooks or config. + + `--3way`: every stored patch is taken against the plan's base commit, so a dependent task's patch + carries its dependencies' changes again. A plain apply refuses that repeat once those changes are + in; a three-way merge sees the same change on both sides and takes it once. Two different changes + to the same lines still conflict and fail here.""" _git( - [*self._base_args(), "apply", "--index", "--whitespace=nowarn", "-"], + [*self._base_args(), "apply", "--index", "--3way", "--whitespace=nowarn", "-"], _git_env(()), self._git_timeout_s, patch.encode(), diff --git a/control-plane/tests/test_github.py b/control-plane/tests/test_github.py index a0546cf4..68c5d408 100644 --- a/control-plane/tests/test_github.py +++ b/control-plane/tests/test_github.py @@ -252,6 +252,50 @@ def test_a_patch_that_does_not_apply_names_the_task_and_pushes_nothing(remotes: assert out.strip() == "" +def _patch_after(tmp_path: Path, name: str, files: dict[str, str]) -> str: + """What the worker stores for a task: `git diff --binary HEAD` of a checkout of the fixture repository + after `files` were written, the way `Workspace.diff_from_head` takes it.""" + work = tmp_path / name + git(tmp_path, "clone", "-q", make_repo(tmp_path / f"{name}-origin"), str(work)) + for path, text in files.items(): + (work / path).write_text(text) + git(work, "add", "-N", "--", ".") + return git(work, "diff", "--binary", "HEAD") + + +FIXED = "def refund(amount, fee):\n return amount - fee\n" + + +@respx.mock +def test_a_dependent_tasks_patch_that_repeats_its_dependencys_change_still_combines( + remotes: Path, tmp_path: Path +): + """A dependent task runs on top of its dependency's change, and its stored patch, taken against the base + commit, carries that change again. Applying both in order must not fail on the repeat.""" + respx.post(f"{API}/repos/acme/widgets/pulls").mock(return_value=created()) + fix = _patch_after(tmp_path, "t0", {"payments/refund.py": FIXED}) + both = {"payments/refund.py": FIXED, "docs/notes.md": "notes\nmore\n"} + fix_and_docs = _patch_after(tmp_path, "t1", both) + result = opener(remotes, tmp_path).open( + spec(tasks=[PrTask(0, "0x" + "a" * 64, "fix", fix), PrTask(1, "0x" + "b" * 64, "docs", fix_and_docs)]) + ) + bare = remotes / "acme" / "widgets.git" + assert git(bare, "show", f"{result.commit_sha}:payments/refund.py") == FIXED + assert git(bare, "show", f"{result.commit_sha}:docs/notes.md") == "notes\nmore\n" + + +@respx.mock +def test_two_tasks_that_change_the_same_line_differently_still_refuse(remotes: Path, tmp_path: Path): + route = respx.post(f"{API}/repos/acme/widgets/pulls").mock(return_value=created()) + one = _patch_after(tmp_path, "t0", {"payments/refund.py": FIXED}) + other = _patch_after(tmp_path, "t1", {"payments/refund.py": "def refund(amount, fee):\n return 0\n"}) + with pytest.raises(GitHubError, match="task 1 does not apply"): + opener(remotes, tmp_path).open( + spec(tasks=[PrTask(0, "0x" + "a" * 64, "fix", one), PrTask(1, "0x" + "b" * 64, "zero", other)]) + ) + assert not route.called + + @respx.mock def test_patch_cannot_write_into_dot_git(remotes: Path, tmp_path: Path): respx.post(f"{API}/repos/acme/widgets/pulls").mock(return_value=created())