perf(edit): bound the post-edit diff and unified patch

packages/agent-tools/src/desktop/edit-diff-bounds.test.ts

@@ -0,0 +1,185 @@
+import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+
+import { afterEach, beforeEach, describe, expect, it } from 'vitest';
+
+import { readPluginHookCompatibleToolResponse } from '../plugin-hooks/vendor-tool-response.js';
+import { LocalEditTool } from './local-pi-tools.js';
+
+interface CompatibleHunk {
+  readonly oldStart: number;
+  readonly oldLines: number;
+  readonly newStart: number;
+  readonly newLines: number;
+  readonly lines: readonly string[];
+}
+
+const context = { sessionId: 'edit-diff-bounds-session', turnId: 'edit-diff-bounds-turn' };
+
+// Myers costs O((N+M)·D) in the length D of the edit script, so a whole-file
+// rewrite is quadratic in the number of changed lines. Rewriting every line of
+// this file took 120-139 s unbounded (the tool diffed the same input twice) and
+// produced a ~1.3 MB diff no renderer displays; bounded it settles in ~220 ms.
+// Vitest's default 5 s timeout therefore also guards the bound: if it is ever
+// removed, the whole-file case stops finishing in time.
+const wholeFileRewriteLines = 20_000;
+
+// Replacing a line costs 2 edits, so DIFF_MAX_EDIT_LENGTH (2000) admits a full
+// rewrite of a file this long and omits anything longer.
+const boundedRewriteLines = 1_000;
+
+function body(lines: number, render: (index: number) => string): string {
+  return `${Array.from({ length: lines }, (_, index) => render(index)).join('\n')}\n`;
+}
+
+describe('edit diff bounds', () => {
+  let directory: string;
+  let file: string;
+
+  beforeEach(async () => {
+    directory = await mkdtemp(join(tmpdir(), 'edit-diff-bounds-'));
+    file = join(directory, 'subject.ts');
+  });
+
+  afterEach(async () => {
+    await rm(directory, { recursive: true, force: true });
+  });
+
+  it('keeps the diff and patch for an ordinary edit', async () => {
+    await writeFile(file, 'const value = 1;\nconst other = 2;\n');
+
+    const result = await new LocalEditTool(directory).execute(context, {
+      file_path: 'subject.ts',
+      old_string: 'const value = 1;',
+      new_string: 'const value = 42;',
+    });
+
+    expect(await readFile(file, 'utf8')).toBe('const value = 42;\nconst other = 2;\n');
+    expect(result.details?.diffOmitted).toBeUndefined();
+    expect(result.details?.patchOmitted).toBeUndefined();
+    expect(result.details?.diff).toContain('const value = 42;');
+    expect(result.details?.patch).toContain('@@');
+    expect(result.details?.patch).toContain('+const value = 42;');
+  });
+
+  it('keeps the diff for a large but bounded block replacement', async () => {
+    const original = body(wholeFileRewriteLines, (index) => `const value${index} = ${index};`);
+    await writeFile(file, original);
+    // 400 replaced lines cost 800 edits, which stays under the bound even
+    // though the surrounding file is large: the bound tracks changed lines,
+    // not file size.
+    const oldBlock = body(400, (index) => `const value${index} = ${index};`);
+    const newBlock = body(400, (index) => `const value${index} = ${index + 1};`);
+
+    const result = await new LocalEditTool(directory).execute(context, {
+      file_path: 'subject.ts',
+      old_string: oldBlock,
+      new_string: newBlock,
+    });
+
+    expect(await readFile(file, 'utf8')).toBe(`${newBlock}${original.slice(oldBlock.length)}`);
+    expect(result.details?.diffOmitted).toBeUndefined();
+    expect(result.details?.patch).toContain('@@');
+  });
+
+  // Rewriting a few hundred lines is an ordinary edit, not the pathological
+  // case this bound targets: a reported 501-line whole-file replacement lost
+  // its diff under the first bound this test pins.
+  it('keeps the diff for a whole-file rewrite that fits the bound', async () => {
+    const original = body(boundedRewriteLines, (index) => `const value${index} = ${index};`);
+    const rewritten = body(boundedRewriteLines, (index) => `let renamed${index} = ${index * 2};`);
+    await writeFile(file, original);
+
+    const result = await new LocalEditTool(directory).execute(context, {
+      file_path: 'subject.ts',
+      old_string: original,
+      new_string: rewritten,
+    });
+
+    expect(await readFile(file, 'utf8')).toBe(rewritten);
+    expect(result.details?.diffOmitted).toBeUndefined();
+    expect(result.details?.patchOmitted).toBeUndefined();
+    expect(result.details?.patch).toContain('+let renamed0 = 0;');
+  });
+
+  it('omits the diff for a whole-file rewrite but still writes the file', async () => {
+    const original = body(wholeFileRewriteLines, (index) => `const value${index} = ${index};`);
+    const rewritten = body(wholeFileRewriteLines, (index) => `let renamed${index} = ${index * 2};`);
+    await writeFile(file, original);
+
+    const result = await new LocalEditTool(directory).execute(context, {
+      file_path: 'subject.ts',
+      old_string: original,
+      new_string: rewritten,
+    });
+
+    // The edit itself must be unaffected: the diff is a receipt computed after
+    // the write, so giving it up never changes what lands on disk.
+    expect(await readFile(file, 'utf8')).toBe(rewritten);
+    expect(result.isError).toBeFalsy();
+
+    expect(result.details?.diffOmitted).toBe('too_many_changes');
+    expect(result.details?.patchOmitted).toBe('too_many_changes');
+    expect(result.details?.patch).toBeUndefined();
+    expect(result.details?.diff).toContain('diff omitted');
+  });
+
+  // A Compatible PostToolUse handler is skipped with HOOK_INVALID_INPUT when
+  // `structuredPatch` is missing, so dropping `details.patch` silently disabled
+  // every such hook on exactly the edits this bound targets.
+  it('still hands the Compatible hook a structured patch when the patch is omitted', async () => {
+    const original = body(wholeFileRewriteLines, (index) => `const value${index} = ${index};`);
+    const rewritten = body(wholeFileRewriteLines, (index) => `let renamed${index} = ${index * 2};`);
+    await writeFile(file, original);
+
+    const result = await new LocalEditTool(directory).execute(context, {
+      file_path: 'subject.ts',
+      old_string: original,
+      new_string: rewritten,
+    });
+
+    expect(result.details?.patch).toBeUndefined();
+    const response = readPluginHookCompatibleToolResponse(result);
+    expect(response).toBeDefined();
+    expect(response?.originalFile).toBe(original);
+    const hunks = response?.structuredPatch as readonly CompatibleHunk[] | undefined;
+    expect(hunks).toHaveLength(1);
+    const hunk = hunks?.[0];
+    expect(hunk?.oldStart).toBe(1);
+    expect(hunk?.newStart).toBe(1);
+    expect(hunk?.oldLines).toBe(wholeFileRewriteLines);
+    expect(hunk?.newLines).toBe(wholeFileRewriteLines);
+    // Every old line is removed and every new line added, in that order.
+    expect(hunk?.lines).toHaveLength(wholeFileRewriteLines * 2);
+    expect(hunk?.lines[0]).toBe('-const value0 = 0;');
+    expect(hunk?.lines[wholeFileRewriteLines - 1]).toBe(
+      `-const value${wholeFileRewriteLines - 1} = ${wholeFileRewriteLines - 1};`,
+    );
+    expect(hunk?.lines[wholeFileRewriteLines]).toBe('+let renamed0 = 0;');
+  });
+
+  it('marks a missing trailing newline in the synthesized hunk', async () => {
+    const original = body(wholeFileRewriteLines, (index) => `const value${index} = ${index};`);
+    const rewritten = body(
+      wholeFileRewriteLines,
+      (index) => `let renamed${index} = ${index * 2};`,
+    ).slice(0, -1);
+    await writeFile(file, original);
+
+    const result = await new LocalEditTool(directory).execute(context, {
+      file_path: 'subject.ts',
+      old_string: original,
+      new_string: rewritten,
+    });
+
+    expect(await readFile(file, 'utf8')).toBe(rewritten);
+    const hunks = readPluginHookCompatibleToolResponse(result)?.structuredPatch as
+      | readonly CompatibleHunk[]
+      | undefined;
+    // jsdiff emits the marker after the side that lacks the newline; only the
+    // new side does here, so it lands last.
+    expect(hunks?.[0]?.lines.at(-1)).toBe('\\ No newline at end of file');
+    expect(hunks?.[0]?.lines.filter((line) => line.startsWith('\\'))).toHaveLength(1);
+  });
+});

packages/agent-tools/src/desktop/local-pi-tools.ts

@@ -299,6 +299,7 @@ export class LocalEditTool implements ToolImpl<
       oldString: normalized?.oldText ?? input.old_string,
       newString: normalized?.newText ?? input.new_string,
       originalFile: editCapture.originalFile,
+      updatedFile: editCapture.updatedFile,
       replaceAll: input.replace_all === true,
       userModified: false,
     });

packages/agent-tools/src/plugin-hooks/vendor-tool-response.ts

@@ -195,41 +195,106 @@ export function withCompatibleGrepToolResponse(
   });
 }
 
+interface CompatibleStructuredPatchHunk {
+  readonly oldStart: number;
+  readonly oldLines: number;
+  readonly newStart: number;
+  readonly newLines: number;
+  readonly lines: readonly string[];
+}
+
+const NO_NEWLINE_MARKER = '\\ No newline at end of file';
+
 export function withCompatibleEditToolResponse(
   result: ToolResult,
   input: {
     readonly filePath: string;
     readonly oldString: string;
     readonly newString: string;
     readonly originalFile: string;
+    readonly updatedFile: string;
     readonly replaceAll: boolean;
     readonly userModified: boolean;
   },
 ): ToolResult {
-  const patch = typeof result.details?.patch === 'string' ? result.details.patch : undefined;
-  if (!patch) return result;
-  let parsed: ReturnType<typeof parsePatch>[number] | undefined;
-  try {
-    parsed = parsePatch(patch)[0];
-  } catch {
-    // Hook compatibility is an enhancement; malformed vendor metadata must
-    // never turn a successful edit into a failed tool call.
-    return result;
-  }
-  if (!parsed) return result;
+  const structuredPatch =
+    parseStructuredPatch(result.details?.patch) ??
+    wholeFileStructuredPatch(input.originalFile, input.updatedFile);
+  if (!structuredPatch) return result;
   return withPluginHookCompatibleToolResponse(result, {
     filePath: input.filePath,
     oldString: input.oldString,
     newString: input.newString,
     originalFile: input.originalFile,
-    structuredPatch: parsed.hunks.map((hunk) => ({
-      oldStart: hunk.oldStart,
-      oldLines: hunk.oldLines,
-      newStart: hunk.newStart,
-      newLines: hunk.newLines,
-      lines: hunk.lines,
-    })),
+    structuredPatch,
     userModified: input.userModified,
     replaceAll: input.replaceAll,
   });
 }
+
+function parseStructuredPatch(patch: unknown): CompatibleStructuredPatchHunk[] | undefined {
+  if (typeof patch !== 'string') return undefined;
+  let parsed: ReturnType<typeof parsePatch>[number] | undefined;
+  try {
+    parsed = parsePatch(patch)[0];
+  } catch {
+    // Hook compatibility is an enhancement; malformed vendor metadata must
+    // never turn a successful edit into a failed tool call.
+    return undefined;
+  }
+  if (!parsed) return undefined;
+  return parsed.hunks.map((hunk) => ({
+    oldStart: hunk.oldStart,
+    oldLines: hunk.oldLines,
+    newStart: hunk.newStart,
+    newLines: hunk.newLines,
+    lines: hunk.lines,
+  }));
+}
+
+/**
+ * Rebuild the single hunk jsdiff emits for a whole-file replacement, without
+ * running Myers.
+ *
+ * `details.patch` is dropped whenever the bounded diff gives up, and a
+ * Compatible PostToolUse handler is skipped outright when `structuredPatch` is
+ * missing, so the hook contract cannot be allowed to depend on the unified
+ * patch surviving that bound. Replacing every old line with every new line is
+ * a correct but deliberately imprecise description of the edit: it is the
+ * coarsest hunk that still round-trips, and it costs one pass over the two
+ * strings instead of O((N+M)·D). Payload stays O(N), the same order as
+ * `originalFile`, which this response already carries in full.
+ */
+function wholeFileStructuredPatch(
+  originalFile: string,
+  updatedFile: string,
+): CompatibleStructuredPatchHunk[] | undefined {
+  const oldLines = splitPatchLines(originalFile);
+  const newLines = splitPatchLines(updatedFile);
+  if (oldLines.length === 0 && newLines.length === 0) return undefined;
+  return [
+    {
+      oldStart: 1,
+      oldLines: oldLines.length,
+      newStart: 1,
+      newLines: newLines.length,
+      lines: [
+        ...oldLines.map((line) => `-${line}`),
+        ...(endsWithoutNewline(originalFile) ? [NO_NEWLINE_MARKER] : []),
+        ...newLines.map((line) => `+${line}`),
+        ...(endsWithoutNewline(updatedFile) ? [NO_NEWLINE_MARKER] : []),
+      ],
+    },
+  ];
+}
+
+function splitPatchLines(text: string): string[] {
+  if (text === '') return [];
+  const lines = text.split('\n');
+  if (lines[lines.length - 1] === '') lines.pop();
+  return lines;
+}
+
+function endsWithoutNewline(text: string): boolean {
+  return text !== '' && !text.endsWith('\n');
+}

packages/agent-tools/src/shared/edit-capture.ts

@@ -5,6 +5,11 @@ import type { EditOperations } from '@earendil-works/pi-coding-agent';
 export interface EditCapture {
   readonly path: string;
   readonly originalFile: string;
+  readonly updatedFile: string;
+}
+
+function stripBom(text: string): string {
+  return text.charCodeAt(0) === 0xfeff ? text.slice(1) : text;
 }
 
 /**
@@ -29,10 +34,10 @@ export function createCapturingEditOperations(): {
       await writeFile(path, content, 'utf8');
       const original = pendingReads.get(path);
       if (original) {
-        const decoded = original.toString('utf8');
         captures.set(path, {
           path,
-          originalFile: decoded.charCodeAt(0) === 0xfeff ? decoded.slice(1) : decoded,
+          originalFile: stripBom(original.toString('utf8')),
+          updatedFile: stripBom(content),
         });
       }
     },