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.
This commit is contained in:
roboomp
2026-07-01 12:06:06 +00:00
parent 4c9f18972c
commit f474fa0e11
3 changed files with 62 additions and 11 deletions
@@ -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;
}
}
}
}
+2
View File
@@ -103,6 +103,7 @@ export interface PatchOptions {
readonly cached?: boolean;
readonly check?: boolean;
readonly env?: Record<string, string | undefined>;
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;
@@ -36,7 +36,7 @@ async function git(repoRoot: string, ...args: string[]): Promise<string> {
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",