15dfda45a4
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
125 lines
4.1 KiB
TypeScript
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");
|
|
});
|
|
});
|