From 15dfda45a4d429a846444f74513ee292e088a801 Mon Sep 17 00:00:00 2001 From: roboomp Date: Wed, 1 Jul 2026 07:05:25 +0000 Subject: [PATCH 1/2] fix(edit): guarded apply_patch against clobber and swallowed multi-file failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/coding-agent/CHANGELOG.md | 5 + packages/coding-agent/src/edit/index.ts | 28 ++++ packages/coding-agent/src/edit/modes/patch.ts | 19 +++ .../test/core/apply-patch-multi-file.test.ts | 124 ++++++++++++++++++ .../test/core/apply-patch.test.ts | 64 +++++++++ .../expected/renamed/dir/name.txt | 1 - .../expected}/old/name.txt | 0 .../expected/old/other.txt | 0 .../expected}/renamed/dir/name.txt | 0 .../input/old/name.txt | 1 + .../input/old/other.txt | 0 .../input/renamed/dir/name.txt | 1 + .../patch.txt | 0 .../expected/duplicate.txt | 1 - .../expected}/duplicate.txt | 0 .../input/duplicate.txt | 1 + .../patch.txt | 0 17 files changed, 243 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/core/apply-patch-multi-file.test.ts delete mode 100644 packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/renamed/dir/name.txt rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{010_move_overwrites_existing_destination/input => 010_move_rejects_existing_destination/expected}/old/name.txt (100%) rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{010_move_overwrites_existing_destination => 010_move_rejects_existing_destination}/expected/old/other.txt (100%) rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{010_move_overwrites_existing_destination/input => 010_move_rejects_existing_destination/expected}/renamed/dir/name.txt (100%) create mode 100644 packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/name.txt rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{010_move_overwrites_existing_destination => 010_move_rejects_existing_destination}/input/old/other.txt (100%) create mode 100644 packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/renamed/dir/name.txt rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{010_move_overwrites_existing_destination => 010_move_rejects_existing_destination}/patch.txt (100%) delete mode 100644 packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/expected/duplicate.txt rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{011_add_overwrites_existing_file/input => 011_add_rejects_existing_file/expected}/duplicate.txt (100%) create mode 100644 packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/input/duplicate.txt rename packages/coding-agent/test/fixtures/apply-patch/scenarios/{011_add_overwrites_existing_file => 011_add_rejects_existing_file}/patch.txt (100%) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d1722f2ec..c3ab2cc8e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### 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 ### Breaking Changes 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 6ad984a7a..019442e1e 100644 --- a/packages/coding-agent/src/edit/modes/patch.ts +++ b/packages/coding-agent/src/edit/modes/patch.ts @@ -1490,6 +1490,16 @@ 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 @@ -1497,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 From 74e54de2bce0bbda57554b776f7bc0667f89fca1 Mon Sep 17 00:00:00 2001 From: roboomp Date: Wed, 1 Jul 2026 07:05:38 +0000 Subject: [PATCH 2/2] style: bun run fix --- packages/coding-agent/src/edit/modes/patch.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/coding-agent/src/edit/modes/patch.ts b/packages/coding-agent/src/edit/modes/patch.ts index 019442e1e..058398c55 100644 --- a/packages/coding-agent/src/edit/modes/patch.ts +++ b/packages/coding-agent/src/edit/modes/patch.ts @@ -1496,9 +1496,7 @@ async function applyNormalizedPatch(input: PatchInput, options: ApplyPatchOption // 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.`, - ); + throw new ApplyPatchError(`Cannot rename ${input.path} to ${input.rename}: destination already exists.`); } }