From 9e4e0f66913e38ecdefb6e01e05e706487b87dda Mon Sep 17 00:00:00 2001 From: roboomp Date: Mon, 29 Jun 2026 05:09:58 +0000 Subject: [PATCH] fix(coding-agent): bounded edit-tool oldText/newText snapshots in tool-result details MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Edit-tool results carried the full pre/post file content in `details.oldText` / `details.newText`. For large files this bloated each per-turn JSONL line by hundreds of KB even though the snapshots are never sent to the LLM (provider serializers send only `content`) and only consumed by the ACP event mapper for diff visualization. Add `pruneOversizedEditSnapshots` and apply it at every site that constructs an `EditToolDetails` / `EditToolPerFileResult`: `executePatchSingle`, `executeReplaceSingle`, hashline `renderSection` (delete + update branches), and both aggregators in `edit/index.ts`. When combined `oldText` + `newText` exceeds 32 KB the helper returns a shallow copy with both fields omitted; smaller edits pass through unchanged. The diff, path, firstChangedLine, op, move, and diagnostics fields are preserved, and ACP returns no diff content for over-budget files (the text content still flows — graceful degradation). Fixes #3786 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/edit/hashline/execute.ts | 20 ++- packages/coding-agent/src/edit/index.ts | 10 +- packages/coding-agent/src/edit/modes/patch.ts | 5 +- .../coding-agent/src/edit/modes/replace.ts | 5 +- .../coding-agent/src/edit/snapshot-details.ts | 51 +++++++ .../test/edit-snapshot-details.test.ts | 143 ++++++++++++++++++ 7 files changed, 220 insertions(+), 15 deletions(-) create mode 100644 packages/coding-agent/src/edit/snapshot-details.ts create mode 100644 packages/coding-agent/test/edit-snapshot-details.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d231182bd..63495ba4a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed the bash interceptor blocking `echo` / `printf` redirects to `/dev/null`, `/dev/tty`, `/dev/stdout`, and `/dev/stderr` device sinks while still directing real file writes to the write tool. ([#3763](https://github.com/can1357/oh-my-pi/issues/3763)) +- Fixed the `edit` tool persisting unbounded full-file `oldText` / `newText` snapshots in tool-result `details`, inflating per-turn session JSONL lines (hundreds of KB per edit on large files). `details.oldText`/`details.newText` are now pruned when their combined length exceeds 32 KB; the visible diff, path, line, and diagnostic metadata are preserved, and ACP `diff` content still flows for smaller edits. ([#3786](https://github.com/can1357/oh-my-pi/issues/3786)) ## [16.2.5] - 2026-06-28 diff --git a/packages/coding-agent/src/edit/hashline/execute.ts b/packages/coding-agent/src/edit/hashline/execute.ts index cb5d61d5d..f51bac2db 100644 --- a/packages/coding-agent/src/edit/hashline/execute.ts +++ b/packages/coding-agent/src/edit/hashline/execute.ts @@ -27,6 +27,7 @@ import { ToolError } from "../../tools/tool-errors"; import { generateDiffString } from "../diff"; import { getFileSnapshotStore } from "../file-snapshot-store"; import type { EditToolDetails, EditToolPerFileResult, LspBatchRequest } from "../renderer"; +import { pruneOversizedEditSnapshots } from "../snapshot-details"; import { nativeBlockResolver } from "./block-resolver"; import { HashlineFilesystem } from "./filesystem"; import { hashPatchInput, NOOP_HARD_LIMIT, recordNoopEdit, resetNoopEdit } from "./noop-loop-guard"; @@ -114,17 +115,22 @@ function renderSection( if (result.op === "delete") { const toolResult: AgentToolResult = { content: [{ type: "text", text: `Deleted ${result.path}` }], - details: { + details: pruneOversizedEditSnapshots({ diff: "", op: "delete", path: result.path, oldText: result.before, meta: outputMeta().get(), - }, + }), }; return { toolResult, - perFileResult: { path: result.path, diff: "", op: "delete", oldText: result.before }, + perFileResult: pruneOversizedEditSnapshots({ + path: result.path, + diff: "", + op: "delete", + oldText: result.before, + }), }; } @@ -161,7 +167,7 @@ function renderSection( text: `${result.header}${blockBlock}${moveBlock}${previewBlock}${warningsBlock}`, }, ], - details: { + details: pruneOversizedEditSnapshots({ diff: diff.diff, firstChangedLine, diagnostics, @@ -172,9 +178,9 @@ function renderSection( oldText: result.before, newText: result.after, meta, - }, + }), }, - perFileResult: { + perFileResult: pruneOversizedEditSnapshots({ path: result.moveDest ?? result.path, diff: diff.diff, firstChangedLine, @@ -184,7 +190,7 @@ function renderSection( sourcePath: result.moveDest ? sourcePath : undefined, oldText: result.before, newText: result.after, - }, + }), }; } diff --git a/packages/coding-agent/src/edit/index.ts b/packages/coding-agent/src/edit/index.ts index 420206e57..d322f0e38 100644 --- a/packages/coding-agent/src/edit/index.ts +++ b/packages/coding-agent/src/edit/index.ts @@ -25,6 +25,7 @@ import applyPatchGrammar from "./modes/apply-patch.lark" with { type: "text" }; import { executePatchSingle, type PatchEditEntry, type PatchParams, patchEditSchema } from "./modes/patch"; import { executeReplaceSingle, type ReplaceEditEntry, type ReplaceParams, replaceEditSchema } from "./modes/replace"; import { type EditToolDetails, type EditToolPerFileResult, getLspBatchRequest, type LspBatchRequest } from "./renderer"; +import { pruneOversizedEditSnapshots } from "./snapshot-details"; import { EDIT_MODE_STRATEGIES } from "./streaming"; export * from "@oh-my-pi/hashline"; @@ -38,6 +39,7 @@ export * from "./modes/patch"; export * from "./modes/replace"; export * from "./normalize"; export * from "./renderer"; +export * from "./snapshot-details"; export * from "./streaming"; type TInput = @@ -186,14 +188,14 @@ async function executeApplyPatchPerFile( return { content: [{ type: "text", text: contentTexts.join("\n") }], - details: { + details: pruneOversizedEditSnapshots({ diff: perFileResults .map(r => r.diff) .filter(Boolean) .join("\n"), firstChangedLine: perFileResults.find(r => r.firstChangedLine)?.firstChangedLine, perFileResults, - }, + }), }; } @@ -276,13 +278,13 @@ async function executeSinglePathEntries( return { content: [{ type: "text", text: contentTexts.join("\n") }], - details: { + details: pruneOversizedEditSnapshots({ diff: diffTexts.join("\n"), firstChangedLine, path: metadataPath ?? path, ...(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 // streaming-edit preview (which displays the *proposed* diff and looks diff --git a/packages/coding-agent/src/edit/modes/patch.ts b/packages/coding-agent/src/edit/modes/patch.ts index 762d8a237..6ad984a7a 100644 --- a/packages/coding-agent/src/edit/modes/patch.ts +++ b/packages/coding-agent/src/edit/modes/patch.ts @@ -48,6 +48,7 @@ import { } from "../normalize"; import { readEditFileText, serializeEditFileText } from "../read-file"; import type { EditToolDetails, LspBatchRequest } from "../renderer"; +import { pruneOversizedEditSnapshots } from "../snapshot-details"; import { type ContextLineResult, DEFAULT_FUZZY_THRESHOLD, @@ -1894,7 +1895,7 @@ export async function executePatchSingle( return { content: [{ type: "text", text: resultText }], - details: { + details: pruneOversizedEditSnapshots({ diff: diffResult.diff, // When the patch moves the file, anchor the diff to the destination // path. ACP `ToolCallContent.diff.path` comes from this field, and @@ -1909,6 +1910,6 @@ export async function executePatchSingle( meta, oldText, newText, - }, + }), }; } diff --git a/packages/coding-agent/src/edit/modes/replace.ts b/packages/coding-agent/src/edit/modes/replace.ts index 96c02dafa..9c910e34b 100644 --- a/packages/coding-agent/src/edit/modes/replace.ts +++ b/packages/coding-agent/src/edit/modes/replace.ts @@ -24,6 +24,7 @@ import { } from "../normalize"; import { readEditFileText, serializeEditFileText } from "../read-file"; import type { EditToolDetails, LspBatchRequest } from "../renderer"; +import { pruneOversizedEditSnapshots } from "../snapshot-details"; export interface FuzzyMatch { actualText: string; @@ -1123,7 +1124,7 @@ export async function executeReplaceSingle( return { content: [{ type: "text", text: resultText }], - details: { + details: pruneOversizedEditSnapshots({ diff: diffResult.diff, path: absolutePath, firstChangedLine: diffResult.firstChangedLine, @@ -1131,6 +1132,6 @@ export async function executeReplaceSingle( meta, oldText: rawContent, newText: finalContent, - }, + }), }; } diff --git a/packages/coding-agent/src/edit/snapshot-details.ts b/packages/coding-agent/src/edit/snapshot-details.ts new file mode 100644 index 000000000..8926b6eb6 --- /dev/null +++ b/packages/coding-agent/src/edit/snapshot-details.ts @@ -0,0 +1,51 @@ +/** + * Bound the size of the `oldText` / `newText` snapshots that edit-tool results + * carry in `details`. These fields hold the full pre/post file content; for + * large files they balloon the per-turn JSONL line and the session file + * (300 KB+ each on the cases reported in #3786) without paying for any LLM + * context (provider serializers send only `content`, never `details`). + * + * Only consumer of the raw snapshots is the ACP event mapper, which builds a + * `diff` ToolCallContent for ACP clients (Zed). When the snapshots are pruned + * the mapper returns `undefined` for that file and the text content still + * flows — diff visualization degrades gracefully for over-threshold edits. + */ + +import type { EditToolDetails, EditToolPerFileResult } from "./renderer"; + +/** + * Combined `oldText` + `newText` character budget per edit-tool result. + * + * Picked so typical code-file edits keep ACP diff visualization while + * pathological cases (large generated files, full-file rewrites) drop the raw + * snapshots before they hit the session JSONL. + */ +export const MAX_EDIT_SNAPSHOT_TEXT_CHARS = 32_768; + +type WithSnapshot = { oldText?: string; newText?: string }; + +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; +} + +/** + * Prune oversized `oldText` / `newText` from an edit-tool details payload, + * recursing into `perFileResults` when present. Per-file overload comes first + * so the more specific shape (required `path`) wins overload resolution at + * the per-file call sites. + */ +export function pruneOversizedEditSnapshots(details: EditToolPerFileResult): EditToolPerFileResult; +export function pruneOversizedEditSnapshots(details: EditToolDetails): EditToolDetails; +export function pruneOversizedEditSnapshots( + details: EditToolDetails | EditToolPerFileResult, +): EditToolDetails | EditToolPerFileResult { + const pruned = pruneSnapshot(details); + if ("perFileResults" in pruned && pruned.perFileResults) { + return { ...pruned, perFileResults: pruned.perFileResults.map(pruneSnapshot) }; + } + return pruned; +} diff --git a/packages/coding-agent/test/edit-snapshot-details.test.ts b/packages/coding-agent/test/edit-snapshot-details.test.ts new file mode 100644 index 000000000..607980785 --- /dev/null +++ b/packages/coding-agent/test/edit-snapshot-details.test.ts @@ -0,0 +1,143 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import * as fs from "node:fs/promises"; +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 { + DEFAULT_FUZZY_THRESHOLD, + executePatchSingle, + executeReplaceSingle, + MAX_EDIT_SNAPSHOT_TEXT_CHARS, + pruneOversizedEditSnapshots, +} from "@oh-my-pi/pi-coding-agent/edit"; +import { writethroughNoop } from "@oh-my-pi/pi-coding-agent/lsp"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { removeWithRetries } from "@oh-my-pi/pi-utils"; + +function makeSession(cwd: string): ToolSession { + return { + cwd, + hasUI: false, + getSessionFile: () => null, + getSessionSpawns: () => "*", + enableLsp: false, + settings: Settings.isolated({ "edit.mode": "patch" }), + getArtifactsDir: () => null, + getSessionId: () => null, + getPlanModeState: () => undefined, + } as unknown as ToolSession; +} + +const noopBeginDeferred = (_p: string) => ({ + onDeferredDiagnostics: () => {}, + signal: new AbortController().signal, + finalize: () => {}, +}); + +// 100 KB of line-broken content. Real code has line breaks, so the generated +// unified diff stays bounded — the bug under test is the unbounded +// `oldText`/`newText` snapshots that survived in `details`, not the diff. +const FILLER = `${"a line of content xxxx yyyy zzzz".repeat(20)}\n`.repeat(2_000); + +let tempDir: string; + +beforeEach(async () => { + resetSettingsForTest(); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-edit-snapshot-")); + await Settings.init({ inMemory: true, cwd: tempDir }); +}); + +afterEach(async () => { + resetSettingsForTest(); + await removeWithRetries(tempDir); +}); + +describe("pruneOversizedEditSnapshots", () => { + test("returns input unchanged when combined snapshot is under the budget", () => { + const oldText = "x".repeat(MAX_EDIT_SNAPSHOT_TEXT_CHARS / 2); + const newText = "y".repeat(MAX_EDIT_SNAPSHOT_TEXT_CHARS / 2); + const details = { diff: "d", path: "/p", oldText, newText }; + expect(pruneOversizedEditSnapshots(details)).toBe(details); + }); + + test("drops oldText and newText when combined size exceeds the budget", () => { + const oversized = "x".repeat(MAX_EDIT_SNAPSHOT_TEXT_CHARS); + const result = pruneOversizedEditSnapshots({ + diff: "@@", + path: "/p", + firstChangedLine: 5, + oldText: oversized, + newText: oversized, + }); + expect(result).toEqual({ diff: "@@", path: "/p", firstChangedLine: 5 }); + expect("oldText" in result).toBe(false); + expect("newText" in result).toBe(false); + }); + + test("prunes snapshots inside perFileResults independently of the aggregate", () => { + const oversized = "x".repeat(MAX_EDIT_SNAPSHOT_TEXT_CHARS); + const small = "tiny"; + const result = pruneOversizedEditSnapshots({ + diff: "d", + perFileResults: [ + { path: "/big", diff: "d1", oldText: oversized, newText: oversized }, + { path: "/small", diff: "d2", oldText: small, newText: small }, + ], + }); + expect(result.perFileResults?.[0]).toEqual({ path: "/big", diff: "d1" }); + expect(result.perFileResults?.[1]).toEqual({ + path: "/small", + diff: "d2", + oldText: small, + newText: small, + }); + }); +}); + +describe("executePatchSingle on oversized files", () => { + test("prunes oldText / newText while keeping diff and path", async () => { + await Bun.write(path.join(tempDir, "big.txt"), `${FILLER}anchor\n${FILLER}`); + + const result = await executePatchSingle({ + session: makeSession(tempDir), + path: "big.txt", + params: { op: "update", diff: "@@\n-anchor\n+ANCHOR" }, + allowFuzzy: true, + fuzzyThreshold: DEFAULT_FUZZY_THRESHOLD, + writethrough: writethroughNoop, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + + const details = result.details!; + expect(details.path).toBe(path.join(tempDir, "big.txt")); + expect(details.diff).toMatch(/-\d+\|anchor/); + expect(details.diff).toMatch(/\+\d+\|ANCHOR/); + expect(details.oldText).toBeUndefined(); + expect(details.newText).toBeUndefined(); + + // The serialized result stays well under the source file. Before the fix + // it was ~2x the file size (full oldText + full newText in details). + expect(JSON.stringify(result).length).toBeLessThan(FILLER.length / 10); + }); +}); + +describe("executeReplaceSingle on oversized files", () => { + test("prunes oldText / newText while keeping diff", async () => { + await Bun.write(path.join(tempDir, "big.txt"), `${FILLER}LINE A\n${FILLER}`); + + const result = await executeReplaceSingle({ + session: makeSession(tempDir), + path: "big.txt", + params: { old_text: "LINE A", new_text: "LINE B" }, + allowFuzzy: false, + fuzzyThreshold: DEFAULT_FUZZY_THRESHOLD, + writethrough: writethroughNoop, + beginDeferredDiagnosticsForPath: noopBeginDeferred, + }); + + const details = result.details!; + expect(details.path).toBe(path.join(tempDir, "big.txt")); + expect(details.oldText).toBeUndefined(); + expect(details.newText).toBeUndefined(); + }); +});