From f474fa0e116cf736edbbc54863be8910caadf31a Mon Sep 17 00:00:00 2001 From: roboomp Date: Wed, 1 Jul 2026 12:06:06 +0000 Subject: [PATCH] fix(agent): detected patch-mode idempotence via reverse-check git apply --3way --check exits 0 even when the real apply would write conflict markers and unmerged index stages, so the previous fix left the worktree dirty on conflicting patches while only flipping changesApplied to false. Dropped --3way for patch-mode merge and used a --reverse --check probe instead: it succeeds only when the target state is already present (true no-op) and reads without touching the worktree. Conflicts fall through to the normal --check + apply path, which rejects them before writing anything. Added regression coverage for the conflict scenario asserting the worktree stays clean, and for the fresh apply path. --- .../coding-agent/src/task/isolation-runner.ts | 27 ++++++++---- packages/coding-agent/src/utils/git.ts | 2 + .../test/task/isolation-runner.test.ts | 44 +++++++++++++++++-- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/src/task/isolation-runner.ts b/packages/coding-agent/src/task/isolation-runner.ts index 0fba6799c..301093a77 100644 --- a/packages/coding-agent/src/task/isolation-runner.ts +++ b/packages/coding-agent/src/task/isolation-runner.ts @@ -291,14 +291,25 @@ export async function mergeIsolatedChanges(opts: IsolationMergeOptions): Promise hadAnyChanges = false; } else { const normalized = patchText.endsWith("\n") ? patchText : `${patchText}\n`; - changesApplied = await git.patch.canApplyText(repoRoot, normalized, { threeWay: true }); - hadAnyChanges = false; - if (changesApplied) { - try { - await git.patch.applyText(repoRoot, normalized, { threeWay: true }); - hadAnyChanges = true; - } catch { - changesApplied = false; + // Idempotence: if the reverse patch applies cleanly the target state is + // already present. Reverse-check reads without touching the worktree, so + // a conflicting patch cannot be misdetected as a no-op — unlike + // `--3way --check`, which exits 0 even when the real apply would write + // conflict markers and unmerged index entries. + const alreadyApplied = await git.patch.canApplyText(repoRoot, normalized, { reverse: true }); + if (alreadyApplied) { + changesApplied = true; + hadAnyChanges = false; + } else { + changesApplied = await git.patch.canApplyText(repoRoot, normalized); + hadAnyChanges = false; + if (changesApplied) { + try { + await git.patch.applyText(repoRoot, normalized); + hadAnyChanges = true; + } catch { + changesApplied = false; + } } } } diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index c9817b72a..addfb8262 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -103,6 +103,7 @@ export interface PatchOptions { readonly cached?: boolean; readonly check?: boolean; readonly env?: Record; + readonly reverse?: boolean; readonly threeWay?: boolean; readonly signal?: AbortSignal; } @@ -389,6 +390,7 @@ function buildApplyArgs(patchPath: string, options: PatchOptions): string[] { const args = ["apply"]; if (options.check) args.push("--check"); if (options.cached) args.push("--cached"); + if (options.reverse) args.push("--reverse"); if (options.threeWay) args.push("--3way"); args.push("--binary", patchPath); return args; diff --git a/packages/coding-agent/test/task/isolation-runner.test.ts b/packages/coding-agent/test/task/isolation-runner.test.ts index 9f72f9219..622a518ad 100644 --- a/packages/coding-agent/test/task/isolation-runner.test.ts +++ b/packages/coding-agent/test/task/isolation-runner.test.ts @@ -36,7 +36,7 @@ async function git(repoRoot: string, ...args: string[]): Promise { return result.text(); } -async function makeAlreadyAppliedPatchRepo(): Promise<{ repoRoot: string; patchPath: string }> { +async function seedFooRepo(finalContent: string): Promise<{ repoRoot: string; patchPath: string }> { const repoRoot = await fs.mkdtemp(path.join(os.tmpdir(), "omp-isolation-merge-")); tempRoots.push(repoRoot); @@ -47,11 +47,19 @@ async function makeAlreadyAppliedPatchRepo(): Promise<{ repoRoot: string; patchP await git(repoRoot, "add", "foo.txt"); await git(repoRoot, "commit", "-m", "base"); await Bun.write(path.join(repoRoot, "foo.txt"), "new\n"); - await git(repoRoot, "commit", "-am", "change"); + await git(repoRoot, "commit", "-am", "change to new"); const patchPath = path.join(repoRoot, "task.patch"); const patchText = await git(repoRoot, "diff-tree", "--binary", "--full-index", "--no-commit-id", "-p", "HEAD"); await Bun.write(patchPath, patchText); + + if (finalContent !== "new\n") { + await git(repoRoot, "reset", "--hard", "HEAD~1"); + if (finalContent !== "old\n") { + await Bun.write(path.join(repoRoot, "foo.txt"), finalContent); + await git(repoRoot, "commit", "-am", "diverge"); + } + } return { repoRoot, patchPath }; } @@ -98,7 +106,7 @@ describe("mergeIsolatedChanges", () => { }); it("treats already-applied patch-mode diffs as successful no-ops", async () => { - const { repoRoot, patchPath } = await makeAlreadyAppliedPatchRepo(); + const { repoRoot, patchPath } = await seedFooRepo("new\n"); const outcome = await mergeIsolatedChanges({ repoRoot, @@ -111,6 +119,36 @@ describe("mergeIsolatedChanges", () => { expect(await git(repoRoot, "status", "--porcelain", "--", "foo.txt")).toBe(""); }); + it("rejects patch-mode conflicts without dirtying the worktree", async () => { + const { repoRoot, patchPath } = await seedFooRepo("other\n"); + + const outcome = await mergeIsolatedChanges({ + repoRoot, + mergeMode: "patch", + result: result({ patchPath }), + }); + + expect(outcome.changesApplied).toBe(false); + expect(outcome.summary).toContain("Patches were not applied"); + expect(await git(repoRoot, "status", "--porcelain", "--", "foo.txt")).toBe(""); + expect(await Bun.file(path.join(repoRoot, "foo.txt")).text()).toBe("other\n"); + expect(await git(repoRoot, "ls-files", "-u", "--", "foo.txt")).toBe(""); + }); + + it("applies a fresh patch-mode diff when context matches", async () => { + const { repoRoot, patchPath } = await seedFooRepo("old\n"); + + const outcome = await mergeIsolatedChanges({ + repoRoot, + mergeMode: "patch", + result: result({ patchPath }), + }); + + expect(outcome.changesApplied).toBe(true); + expect(outcome.hadAnyChanges).toBe(true); + expect(await Bun.file(path.join(repoRoot, "foo.txt")).text()).toBe("new\n"); + }); + it("does not mark failed branch-mode runs as nested-patch eligible", async () => { const outcome = await mergeIsolatedChanges({ repoRoot: "/repo",