Merge PR #4111: fix(edit): guard apply_patch against clobber and swallowed multi-file failures (@roboomp)

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
 

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 } : {}),
 	};
 }
 

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`;

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");
+	});
+});

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");
+	});
 });

packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_overwrites_existing_destination/expected/renamed/dir/name.txt

@@ -1 +0,0 @@
-new

packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/old/name.txt

@@ -0,0 +1 @@
+from

packages/coding-agent/test/fixtures/apply-patch/scenarios/010_move_rejects_existing_destination/input/renamed/dir/name.txt

@@ -0,0 +1 @@
+existing

packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_overwrites_existing_file/expected/duplicate.txt

@@ -1 +0,0 @@
-new content

packages/coding-agent/test/fixtures/apply-patch/scenarios/011_add_rejects_existing_file/input/duplicate.txt

@@ -0,0 +1 @@
+old content