fix(coding-agent): preserve pruned-snapshot marker through single-path aggregation

When a multi-entry single-path edit prunes the first entry's snapshots (large pre-image) and keeps a later entry's snapshots (file shrunk between entries), the aggregator at `executeSinglePathEntries` recorded the later entry's small `oldText`/`newText` as the whole-file transition. ACP clients would then render a misleading partial diff instead of degrading to text-only for the over-budget edit. Add an explicit `snapshotsPruned` marker on `EditToolDetails` / `EditToolPerFileResult`, set by `pruneSnapshot` whenever it strips a payload. `executeSinglePathEntries` tracks the flag across child results and suppresses aggregate `oldText`/`newText` (re-stamping the marker on the aggregate) the moment any child was pruned; `executeApplyPatchPerFile` propagates the flag onto each per-file entry. Regression test exercises the exact scenario the reviewer raised on #3787: replace mode where entry 1 collapses a >1 MB file to a single line (pruned) and entry 2 trivially renames the now-tiny result; the aggregate result now carries `snapshotsPruned: true` with both snapshot fields omitted.
2026-06-29 05:22:18 +00:00
parent 9e4e0f6691
commit 3ced3a923e
4 changed files with 68 additions and 6 deletions
@@ -160,6 +160,7 @@ async function executeApplyPatchPerFile(
 				meta: details?.meta,
 				oldText: details?.oldText,
 				newText: details?.newText,
+				snapshotsPruned: details?.snapshotsPruned,
 			});
 			const text = result.content?.find(c => c.type === "text")?.text ?? "";
 			if (text) contentTexts.push(text);
@@ -218,6 +219,11 @@ async function executeSinglePathEntries(
 	let firstOldText: string | undefined;
 	let hasLastNewText = false;
 	let lastNewText: string | undefined;
+	// Any pruned child invalidates the aggregate snapshot: combining a kept
+	// first-entry oldText with a pruned next entry's newText (or vice-versa)
+	// would describe a transition the file never made. Suppress aggregate
+	// snapshots and stamp the marker so ACP/downstream can degrade cleanly.
+	let snapshotsPruned = false;

 	for (let i = 0; i < runs.length; i++) {
 		const isLast = i === runs.length - 1;
@@ -241,6 +247,7 @@ async function executeSinglePathEntries(
 				lastNewText = details.newText;
 				hasLastNewText = true;
 			}
+			if (details?.snapshotsPruned) snapshotsPruned = true;
 			const text = result.content?.find(c => c.type === "text")?.text ?? "";
 			if (text) contentTexts.push(text);
 		} catch (err) {
@@ -282,8 +289,12 @@ async function executeSinglePathEntries(
 			diff: diffTexts.join("\n"),
 			firstChangedLine,
 			path: metadataPath ?? path,
-			...(hasFirstOldText ? { oldText: firstOldText } : {}),
-			...(hasLastNewText ? { newText: lastNewText } : {}),
+			...(snapshotsPruned
+				? { snapshotsPruned: true as const }
+				: {
+						...(hasFirstOldText ? { oldText: firstOldText } : {}),
+						...(hasLastNewText ? { newText: lastNewText } : {}),
+					}),
 		}),
 		// Any per-entry failure marks the aggregate result as an error so the
 		// renderer takes the error branch instead of falling through to the
@@ -70,6 +70,8 @@ export interface EditToolPerFileResult {
 	oldText?: string;
 	/** Source-of-truth content after the edit; `undefined` for delete operations. */
 	newText?: string;
+	/** True when {@link pruneOversizedEditSnapshots} dropped `oldText`/`newText` from this entry. Aggregators check this to suppress misleading combined snapshots when at least one entry of a multi-entry single-path edit was pruned. */
+	snapshotsPruned?: boolean;
 	/** Pre-move source path; set only when the edit moved/renamed the file. The header renders `sourcePath → path`. */
 	sourcePath?: string;
 }
@@ -95,6 +97,8 @@ export interface EditToolDetails {
 	oldText?: string;
 	/** Source-of-truth content after the edit; `undefined` for delete operations. */
 	newText?: string;
+	/** True when {@link pruneOversizedEditSnapshots} dropped `oldText`/`newText` from this entry. Aggregators check this to suppress misleading combined snapshots when at least one entry of a multi-entry single-path edit was pruned. */
+	snapshotsPruned?: boolean;
 	/** Pre-move source path; set only when the edit moved/renamed the file. The header renders `sourcePath → path`. */
 	sourcePath?: string;
 }
@@ -22,14 +22,14 @@ import type { EditToolDetails, EditToolPerFileResult } from "./renderer";
 */
 export const MAX_EDIT_SNAPSHOT_TEXT_CHARS = 32_768;

-type WithSnapshot = { oldText?: string; newText?: string };
+type WithSnapshot = { oldText?: string; newText?: string; snapshotsPruned?: boolean };

 function pruneSnapshot<T extends WithSnapshot>(details: T): T {
 	if ((details.oldText?.length ?? 0) + (details.newText?.length ?? 0) <= MAX_EDIT_SNAPSHOT_TEXT_CHARS) {
 		return details;
 	}
 	const { oldText: _old, newText: _new, ...rest } = details;
-	return rest as T;
+	return { ...rest, snapshotsPruned: true } as T;
 }

 /**
@@ -5,6 +5,8 @@ import * as path from "node:path";
 import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
 import {
 	DEFAULT_FUZZY_THRESHOLD,
+	EditTool,
+	type EditToolDetails,
 	executePatchSingle,
 	executeReplaceSingle,
 	MAX_EDIT_SNAPSHOT_TEXT_CHARS,
@@ -69,7 +71,7 @@ describe("pruneOversizedEditSnapshots", () => {
 			oldText: oversized,
 			newText: oversized,
 		});
-		expect(result).toEqual({ diff: "@@", path: "/p", firstChangedLine: 5 });
+		expect(result).toEqual({ diff: "@@", path: "/p", firstChangedLine: 5, snapshotsPruned: true });
 		expect("oldText" in result).toBe(false);
 		expect("newText" in result).toBe(false);
 	});
@@ -84,7 +86,7 @@ describe("pruneOversizedEditSnapshots", () => {
 				{ path: "/small", diff: "d2", oldText: small, newText: small },
 			],
 		});
-		expect(result.perFileResults?.[0]).toEqual({ path: "/big", diff: "d1" });
+		expect(result.perFileResults?.[0]).toEqual({ path: "/big", diff: "d1", snapshotsPruned: true });
 		expect(result.perFileResults?.[1]).toEqual({
 			path: "/small",
 			diff: "d2",
@@ -141,3 +143,48 @@ describe("executeReplaceSingle on oversized files", () => {
 		expect(details.newText).toBeUndefined();
 	});
 });
+
+describe("EditTool single-path aggregation across mixed-size entries", () => {
+	test("pruned first-entry snapshots suppress aggregate snapshots from a later kept entry", async () => {
+		// Reviewer scenario from #3787: a multi-entry single-path edit where the
+		// first entry shrinks a large file (oldText pruned, file becomes tiny)
+		// and a later entry trivially edits the now-tiny file (snapshots kept).
+		// Without the marker, the aggregator would record the second entry's
+		// small oldText as the whole-file pre-image and ACP clients would
+		// render a misleading partial diff.
+		await Bun.write(path.join(tempDir, "shrink.txt"), `${FILLER}TAIL\n`);
+
+		// Replace mode lets us shrink the file in one edit, then tweak the result.
+		const replaceSession = {
+			cwd: tempDir,
+			hasUI: false,
+			getSessionFile: () => null,
+			getSessionSpawns: () => "*",
+			enableLsp: false,
+			settings: Settings.isolated({ "edit.mode": "replace" }),
+			getArtifactsDir: () => null,
+			getSessionId: () => null,
+			getPlanModeState: () => undefined,
+		} as unknown as ToolSession;
+		const tool = new EditTool(replaceSession);
+
+		const result = await tool.execute("call-shrink", {
+			path: "shrink.txt",
+			edits: [
+				// Entry 1: collapse the entire large prefix into one tiny token —
+				// oldText is ~1.28 MB, newText is tiny → combined > 32 KB → pruned.
+				{ old_text: FILLER, new_text: "tiny\n" },
+				// Entry 2: trivial rename on the now-tiny file —
+				// oldText/newText combined well under 32 KB → kept by the inner.
+				{ old_text: "TAIL", new_text: "DONE" },
+			],
+		});
+
+		const details = result.details as EditToolDetails;
+		expect(details.snapshotsPruned).toBe(true);
+		expect(details.oldText).toBeUndefined();
+		expect(details.newText).toBeUndefined();
+		// Aggregate diff still reflects both transitions.
+		expect(details.diff.length).toBeGreaterThan(0);
+	});
+});