Files
oh-my-pi/packages/coding-agent/test/core/apply-patch-multi-file.test.ts
T
roboomp 15dfda45a4 fix(edit): guarded apply_patch against clobber and swallowed multi-file failures
The apply_patch language documents `*** Add File` and `*** Move to` as
strictly non-overwriting (create / rename), but the fs-level create and
rename paths in applyNormalizedPatch wrote through to the resolved target
without checking whether it already existed. Existing destinations were
silently replaced, and in the rename case the source was also deleted.

The multi-file executeApplyPatchPerFile aggregator caught each per-file
exception, appended an error entry, and kept iterating. Later files
still ran against an inconsistent post-state, and the aggregate result
had no top-level isError — so a mixed partial application looked like a
successful edit to the agent loop.

Changes:
- Add fs.exists guards before the create write and before the rename
  write/delete in packages/coding-agent/src/edit/modes/patch.ts. Both
  reject with ApplyPatchError before any side effect.
- Make executeApplyPatchPerFile in packages/coding-agent/src/edit/index.ts
  stop at the first per-file failure, list applied vs. skipped files in
  the aggregate text, and propagate isError, matching executeSinglePathEntries.
- Rename the two apply-patch scenario fixtures (010_move_..., 011_add_...)
  that pinned the buggy overwrite behavior to _rejects_ variants, and
  flip their expected/ trees so source and pre-existing destination
  remain byte-identical after the rejected apply.
- Cover both failure modes with new regressions in
  packages/coding-agent/test/core/apply-patch.test.ts and a new
  packages/coding-agent/test/core/apply-patch-multi-file.test.ts.

Fixes #4074
2026-07-01 07:05:25 +00:00

125 lines
4.1 KiB
TypeScript

/**
* Regression coverage for #4074-B: multi-file apply_patch must stop at the
* first per-file failure and surface `isError` on the aggregate result so the
* agent loop and renderers take the error branch instead of treating a
* mixed partial application as a successful edit.
*
* The single-file (`executeSinglePathEntries`) counterpart already stops at
* the first failure and stamps `isError`; this file pins the same semantics
* for the multi-file (`executeApplyPatchPerFile`) apply_patch aggregate.
*/
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
import * as fs from "node:fs";
import * as os from "node:os";
import * as path from "node:path";
import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
import { EditTool, type EditToolDetails } from "@oh-my-pi/pi-coding-agent/edit";
import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools";
import { removeWithRetries } from "@oh-my-pi/pi-utils";
function makeApplyPatchSession(cwd: string): ToolSession {
return {
cwd,
hasUI: false,
getSessionFile: () => null,
getSessionSpawns: () => "*",
enableLsp: false,
settings: Settings.isolated({ "edit.mode": "apply_patch" }),
getArtifactsDir: () => null,
getSessionId: () => null,
getPlanModeState: () => undefined,
} as unknown as ToolSession;
}
let tempDir: string;
beforeEach(async () => {
resetSettingsForTest();
tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), "omp-apply-patch-multi-"));
await Settings.init({ inMemory: true, cwd: tempDir });
});
afterEach(async () => {
resetSettingsForTest();
await removeWithRetries(tempDir);
});
describe("EditTool apply_patch multi-file aggregate (#4074-B)", () => {
test("stops at first per-file failure, marks isError, and skips remaining files", async () => {
await Bun.write(path.join(tempDir, "a.txt"), "a\n");
const tool = new EditTool(makeApplyPatchSession(tempDir));
const patch = [
"*** Begin Patch",
"*** Update File: a.txt",
"@@",
"-a",
"+A",
"*** Update File: missing.txt",
"@@",
"-x",
"+y",
"*** Add File: c.txt",
"+new content",
"*** End Patch",
"",
].join("\n");
const result = await tool.execute("call-#4074-B", { input: patch });
// First file must have landed (matches existing partial-success
// semantics of applyCodexPatch).
expect(await Bun.file(path.join(tempDir, "a.txt")).text()).toBe("A\n");
// Third entry MUST NOT be applied after the second one failed.
expect(fs.existsSync(path.join(tempDir, "c.txt"))).toBe(false);
// Aggregate MUST report failure so the agent loop takes the error
// branch.
expect(result.isError).toBe(true);
// The failed and skipped files must be surfaced so the caller can
// re-issue only the missing work.
const text = result.content?.find(c => c.type === "text")?.text ?? "";
expect(text).toContain("missing.txt");
expect(text).toContain("c.txt");
expect(text).toContain("NOT applied");
// Per-file details must include an error entry for the failing file.
const details = result.details as EditToolDetails | undefined;
const perFile = details?.perFileResults ?? [];
const failed = perFile.find(r => r.path.endsWith("missing.txt"));
expect(failed?.isError).toBe(true);
// The skipped third file must not have a per-file entry (we stop
// before attempting it).
expect(perFile.some(r => r.path.endsWith("c.txt"))).toBe(false);
});
test("all-success multi-file apply_patch does not set isError", async () => {
await Bun.write(path.join(tempDir, "a.txt"), "a\n");
await Bun.write(path.join(tempDir, "b.txt"), "b\n");
const tool = new EditTool(makeApplyPatchSession(tempDir));
const patch = [
"*** Begin Patch",
"*** Update File: a.txt",
"@@",
"-a",
"+A",
"*** Update File: b.txt",
"@@",
"-b",
"+B",
"*** End Patch",
"",
].join("\n");
const result = await tool.execute("call-#4074-B-ok", { input: patch });
expect(result.isError).toBeUndefined();
expect(await Bun.file(path.join(tempDir, "a.txt")).text()).toBe("A\n");
expect(await Bun.file(path.join(tempDir, "b.txt")).text()).toBe("B\n");
});
});