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",