diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index d322f0e38..28075b1d9 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -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 diff --git a/packages/coding-agent/src/edit/renderer.ts b/packages/coding-agent/src/edit/renderer.ts index 1a4c4863e..2115255f8 100644 --- a/packages/coding-agent/src/edit/renderer.ts +++ b/packages/coding-agent/src/edit/renderer.ts @@ -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; } diff --git a/packages/coding-agent/src/edit/snapshot-details.ts b/packages/coding-agent/src/edit/snapshot-details.ts index 8926b6eb6..97da75a4a 100644 --- a/packages/coding-agent/src/edit/snapshot-details.ts +++ b/packages/coding-agent/src/edit/snapshot-details.ts @@ -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(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; } /** diff --git a/packages/coding-agent/test/edit-snapshot-details.test.ts b/packages/coding-agent/test/edit-snapshot-details.test.ts index 607980785..856357c77 100644 --- a/packages/coding-agent/test/edit-snapshot-details.test.ts +++ b/packages/coding-agent/test/edit-snapshot-details.test.ts @@ -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); + }); +});