diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3c98bdf1a..3bb8a9ea5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -24,6 +24,10 @@ ### Fixed - Added retry-path diagnostics for assistant-tail removal and scheduled continuations after transient provider errors ([#4070](https://github.com/can1357/oh-my-pi/issues/4070)). +### Fixed + +- Fixed the `apply_patch` tool silently overwriting pre-existing destinations for `*** Add File:` (create) and `*** Move to:` (rename), which could clobber unrelated content and, in the rename case, also delete the source. Both operations now reject upfront with an `ApplyPatchError` and leave the source and destination byte-identical ([#4074](https://github.com/can1357/oh-my-pi/issues/4074)). +- Fixed multi-file `apply_patch` swallowing per-file failures: the aggregate result now stops at the first failing file, surfaces the applied vs. skipped file paths, and sets top-level `isError` so the agent loop and renderer take the error branch instead of treating a partial application as success ([#4074](https://github.com/can1357/oh-my-pi/issues/4074)). ## [16.2.12] - 2026-07-01 diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index 28075b1d9..5a312c89e 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -138,6 +138,7 @@ async function executeApplyPatchPerFile( const perFileResults: EditToolPerFileResult[] = []; const contentTexts: string[] = []; + let errorCount = 0; for (let i = 0; i < fileEntries.length; i++) { const { path, run } = fileEntries[i]; @@ -169,6 +170,29 @@ async function executeApplyPatchPerFile( const displayErrorText = err instanceof HashlineMismatchError ? err.displayMessage : undefined; perFileResults.push({ path, diff: "", isError: true, errorText, displayErrorText }); contentTexts.push(`Error editing ${path}: ${errorText}`); + errorCount++; + // Later entries were authored assuming this file's post-state; a + // partial cascade after failure typically compounds damage. Stop + // here, report applied vs. skipped, and let the caller re-issue + // only the failed and unapplied files. Matches + // `executeSinglePathEntries` semantics. + if (i > 0) { + const appliedPaths = fileEntries + .slice(0, i) + .map(e => e.path) + .join(", "); + contentTexts.push(`Files already applied: ${appliedPaths}.`); + } + if (i + 1 < fileEntries.length) { + const skippedPaths = fileEntries + .slice(i + 1) + .map(e => e.path) + .join(", "); + contentTexts.push( + `Files NOT applied: ${skippedPaths}; re-read the affected files and re-issue only the failed and unapplied files.`, + ); + } + break; } // Emit partial result after each file so UI shows progressive completion @@ -197,6 +221,10 @@ async function executeApplyPatchPerFile( firstChangedLine: perFileResults.find(r => r.firstChangedLine)?.firstChangedLine, perFileResults, }), + // Any per-file failure marks the aggregate result as an error so the + // agent loop and renderer take the error branch instead of treating + // a mixed partial application as a successful edit. + ...(errorCount > 0 ? { isError: true } : {}), }; } diff --git a/packages/coding-agent/src/edit/modes/patch.ts b/packages/coding-agent/src/edit/modes/patch.ts index 0a313389e..6af16052e 100644 --- a/packages/coding-agent/src/edit/modes/patch.ts +++ b/packages/coding-agent/src/edit/modes/patch.ts @@ -1492,6 +1492,14 @@ async function applyNormalizedPatch(input: PatchInput, options: ApplyPatchOption if (destPath === absolutePath) { throw new ApplyPatchError("rename path is the same as source path"); } + // The `*** Move to` / rename contract is strictly non-overwriting: + // reject before the update path reads or writes anything, so both + // source and pre-existing destination remain untouched. Callers who + // really need to replace the destination must delete it in an + // earlier hunk. + if (await fs.exists(destPath)) { + throw new ApplyPatchError(`Cannot rename ${input.path} to ${input.rename}: destination already exists.`); + } } // Handle CREATE operation @@ -1499,6 +1507,15 @@ async function applyNormalizedPatch(input: PatchInput, options: ApplyPatchOption if (!input.diff) { throw new ApplyPatchError("Create operation requires diff (file content)"); } + // The `*** Add File` / create contract is strictly non-overwriting: + // reject before mkdir/write so pre-existing content stays intact and + // the caller can re-issue as an explicit `*** Update File` (or a + // delete+add pair) if overwrite is genuinely intended. + if (await fs.exists(absolutePath)) { + throw new ApplyPatchError( + `Cannot create ${input.path}: file already exists. Use *** Update File to modify it in place.`, + ); + } // Strip + prefixes if present (handles diffs formatted as additions) const normalizedContent = normalizeCreateContent(input.diff); const content = normalizedContent.endsWith("\n") ? normalizedContent : `${normalizedContent}\n`; diff --git a/packages/coding-agent/test/core/apply-patch-multi-file.test.ts b/packages/coding-agent/test/core/apply-patch-multi-file.test.ts new file mode 100644 index 000000000..001e87f21 --- /dev/null +++ b/packages/coding-agent/test/core/apply-patch-multi-file.test.ts @@ -0,0 +1,124 @@ +/** + * 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"); + }); +}); diff --git a/packages/coding-agent/test/core/apply-patch.test.ts b/packages/coding-agent/test/core/apply-patch.test.ts index a2b6f4313..647bf5b36 100644 --- a/packages/coding-agent/test/core/apply-patch.test.ts +++ b/packages/coding-agent/test/core/apply-patch.test.ts @@ -717,4 +717,68 @@ describe("applyCodexPatch (production)", () => { // First op should have landed before the failure. expect(await Bun.file(path.join(tempDir, "first.txt")).text()).toBe("A\n"); }); + + // #4074-A: Add File / Move to must not silently overwrite pre-existing + // destinations. The apply_patch grammar documents these as create/rename + // with no overwrite affordance (see prompts/tools/apply-patch.md), and + // the fs-level `applyPatch`, the envelope-level `applyCodexPatch`, and + // their shared `applyNormalizedPatch` implementation share the guard. + test("applyPatch create refuses to overwrite an existing file", async () => { + const target = path.join(tempDir, "exists.txt"); + await Bun.write(target, "original\n"); + + await expect( + applyPatch({ path: "exists.txt", op: "create", diff: "replacement\n" }, { cwd: tempDir }), + ).rejects.toBeInstanceOf(ApplyPatchError); + // Pre-existing content must remain byte-identical. + expect(await Bun.file(target).text()).toBe("original\n"); + }); + + test("applyPatch rename refuses to overwrite an existing destination", async () => { + const src = path.join(tempDir, "src.txt"); + const dst = path.join(tempDir, "dst.txt"); + await Bun.write(src, "source\n"); + await Bun.write(dst, "destination\n"); + + await expect( + applyPatch( + { path: "src.txt", op: "update", rename: "dst.txt", diff: "@@\n-source\n+source2" }, + { cwd: tempDir }, + ), + ).rejects.toBeInstanceOf(ApplyPatchError); + // Both source and destination must remain byte-identical. + expect(await Bun.file(src).text()).toBe("source\n"); + expect(await Bun.file(dst).text()).toBe("destination\n"); + }); + + test("applyCodexPatch *** Add File over existing file rejects and preserves content", async () => { + const target = path.join(tempDir, "hello.txt"); + await Bun.write(target, "kept\n"); + + const patch = ["*** Begin Patch", "*** Add File: hello.txt", "+overwritten", "*** End Patch"].join("\n"); + + await expect(applyCodexPatch(patch, { cwd: tempDir })).rejects.toBeInstanceOf(ApplyPatchError); + expect(await Bun.file(target).text()).toBe("kept\n"); + }); + + test("applyCodexPatch *** Move to over existing destination rejects and preserves both files", async () => { + const src = path.join(tempDir, "src.txt"); + const dst = path.join(tempDir, "dst.txt"); + await Bun.write(src, "hello\n"); + await Bun.write(dst, "will-be-preserved\n"); + + const patch = [ + "*** Begin Patch", + "*** Update File: src.txt", + "*** Move to: dst.txt", + "@@", + "-hello", + "+HELLO", + "*** End Patch", + ].join("\n"); + + await expect(applyCodexPatch(patch, { cwd: tempDir })).rejects.toBeInstanceOf(ApplyPatchError); + expect(await Bun.file(src).text()).toBe("hello\n"); + expect(await Bun.file(dst).text()).toBe("will-be-preserved\n"); + }); }); diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/renamed/dir/name.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/renamed/dir/name.txt deleted file mode 100644 index 3e757656c..000000000 --- a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/renamed/dir/name.txt +++ /dev/null @@ -1 +0,0 @@ -new diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/input/old/name.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/expected/old/name.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/input/old/name.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/expected/old/name.txt diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/old/other.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/expected/old/other.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/old/other.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/expected/old/other.txt diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/input/renamed/dir/name.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/expected/renamed/dir/name.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/input/renamed/dir/name.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/expected/renamed/dir/name.txt diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/name.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/name.txt new file mode 100644 index 000000000..3940df7cd --- /dev/null +++ b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/name.txt @@ -0,0 +1 @@ +from diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/input/old/other.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/other.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/input/old/other.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/other.txt diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/renamed/dir/name.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/renamed/dir/name.txt new file mode 100644 index 000000000..cbaf024e5 --- /dev/null +++ b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/renamed/dir/name.txt @@ -0,0 +1 @@ +existing diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/patch.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/patch.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/patch.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/patch.txt diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/expected/duplicate.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/expected/duplicate.txt deleted file mode 100644 index b66ba06d3..000000000 --- a/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/expected/duplicate.txt +++ /dev/null @@ -1 +0,0 @@ -new content diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/input/duplicate.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/expected/duplicate.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/input/duplicate.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/expected/duplicate.txt diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/input/duplicate.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/input/duplicate.txt new file mode 100644 index 000000000..33194a0a6 --- /dev/null +++ b/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/input/duplicate.txt @@ -0,0 +1 @@ +old content diff --git a/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/patch.txt b/packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/patch.txt similarity index 100% rename from packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/patch.txt rename to packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/patch.txt