Bound aggregate Git diff bytes and retained file records

Author: mchwang

README.md

@@ -43,7 +43,7 @@ Inputs such as `planText` and the ledger must come from the caller. The future `
 
 ## Current limits and safety
 
-- History must be linear and descend from the requested base. Merge histories are rejected with a rebase instruction; repositories using object alternates or symlinked object storage are rejected (storage inspection is limited to 100,000 entries). Reads are bounded to 500 commits, 32 MiB per Git response, 64 MiB of unique blob bytes across the history (callers may lower `maxBlobBytes`), and a 2-second budget per line diff; oversized work fails explicitly.
+- History must be linear and descend from the requested base. Merge histories are rejected with a rebase instruction; repositories using object alternates or symlinked object storage are rejected (storage inspection is limited to 100,000 entries). Reads are bounded to 500 commits, 32 MiB per Git response, 64 MiB of unique blob bytes across the history (callers may lower `maxBlobBytes`), 8 MiB of cumulative diff output, 20,000 cumulative file records (including the final diff), and a 2-second budget per line diff; oversized work fails explicitly.
 - The caller selects and trusts the repository and its Git administrative directory. Normal Git discovery, linked-worktree gitfiles, and symlinked gitdirs are supported; object-storage links and alternates inside that selected gitdir are rejected. This adapter is not a filesystem-containment boundary for untrusted repository roots.
 - Object storage must remain unchanged during a read; these library checks do not isolate a concurrently hostile filesystem.
 - Ownership uses line diffs, not semantic inference. Within one replacement block, new lines inherit all affected owners conservatively. Function context comes from Git hunk headers, not an AST.

docs/implementation/build-step-1.md

@@ -63,6 +63,10 @@ Declined the YAML finding: the pinned yaml 2.9.1 implementation and types explic
 
 Clarified the gitdir policy rather than rejecting normal linked worktrees. The caller selects and trusts the repository and its administrative directory; gitfiles and symlinked gitdirs are supported, while object-storage symlinks and alternates within that gitdir are rejected. Real-Git fixtures exercise both administrative layouts. Filesystem containment of an untrusted repository root belongs to the runner, not this read-only library.
 
+## Review round 8
+
+Fixed aggregate diff retention outside the blob budget. The adapter limits total raw-diff and context-patch output to 8 MiB and total file records across commit and final diffs to 20,000. Both budgets can be lowered by callers. Small-budget cases first failed and now reject explicitly; the exact file-record boundary succeeds.
+
 ## Remaining gates
 
 This is a working foundation, not a completed application or a claim that all implementation tasks are done. T18's pure validation/edit core is present; its agent adapters, import UI, and persistence are pending. Ledger storage, rebase mappings, and the read-only review screen remain next. The already-fixed GitHub check belongs to the later GitHub/runner integration.

git/history.ts

@@ -4,12 +4,21 @@ import { resolve as resolvePath, join } from 'node:path';
 import type { FileDelta, FileVersion, History } from '../core/linking.ts';
 
 /** Read-only Git adapter. Never follows working-tree symlinks or runs diff helpers. */
-export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', limits: { maxBlobBytes?: number; maxObjectEntries?: number } = {}): History {
+export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', limits: { maxBlobBytes?: number; maxObjectEntries?: number; maxDiffBytes?: number; maxFileEntries?: number } = {}): History {
   const maxBlobBytes = limits.maxBlobBytes ?? 64 * 1024 * 1024;
   if (!Number.isSafeInteger(maxBlobBytes) || maxBlobBytes < 1 || maxBlobBytes > 64 * 1024 * 1024) throw new Error('Blob byte budget must be a positive integer no larger than 64 MiB.');
   const maxObjectEntries = limits.maxObjectEntries ?? 100_000;
   if (!Number.isSafeInteger(maxObjectEntries) || maxObjectEntries < 1 || maxObjectEntries > 100_000) throw new Error('Object entry budget must be a positive integer no larger than 100000.');
-  let blobBytes = 0;
+  const maxDiffBytes = limits.maxDiffBytes ?? 8 * 1024 * 1024;
+  if (!Number.isSafeInteger(maxDiffBytes) || maxDiffBytes < 1 || maxDiffBytes > 8 * 1024 * 1024) throw new Error('Diff byte budget must be a positive integer no larger than 8 MiB.');
+  const maxFileEntries = limits.maxFileEntries ?? 20_000;
+  if (!Number.isSafeInteger(maxFileEntries) || maxFileEntries < 1 || maxFileEntries > 20_000) throw new Error('File entry budget must be a positive integer no larger than 20000.');
+  let blobBytes = 0, diffBytes = 0, fileEntries = 0;
+  const accountDiff = (data: Buffer): Buffer => {
+    diffBytes += data.length;
+    if (diffBytes > maxDiffBytes) throw new Error('Review history exceeds the cumulative diff byte budget; choose a narrower base.');
+    return data;
+  };
   // Inherited Git variables can redirect repository, index, config, and object lookup.
   const environment = Object.fromEntries(Object.entries(process.env).filter(([key]) => !/^GIT_/i.test(key)));
   const run = (...args: string[]) => execFileSync('git', ['--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'protocol.allow=never', ...args], {
@@ -67,10 +76,11 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
     return { oid, mode, text: blobs.get(oid)! };
   };
   const diff = (from: string, to: string, contexts: boolean): FileDelta[] => {
-    const raw = run('diff', '--ignore-submodules=none', '--no-relative', '--raw', '-z', '--no-abbrev', '--no-ext-diff', '--no-textconv', '-M', from, to, '--');
+    const raw = accountDiff(run('diff', '--ignore-submodules=none', '--no-relative', '--raw', '-z', '--no-abbrev', '--no-ext-diff', '--no-textconv', '-M', from, to, '--'));
     const fields = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(raw).split('\0');
     const result: FileDelta[] = [];
     for (let i = 0; i < fields.length && fields[i];) {
+      if (++fileEntries > maxFileEntries) throw new Error('Review history exceeds the cumulative file entry budget; choose a narrower base.');
       const match = /^:(\d+) (\d+) ([0-9a-f]+) ([0-9a-f]+) ([A-Z])\d*$/.exec(fields[i++]!);
       if (!match) throw new Error('Unexpected Git raw diff record.');
       const [, oldMode, newMode, oldOid, newOid, status] = match;
@@ -82,7 +92,7 @@ export function readHistory(repo: string, baseRef: string, headRef = 'HEAD', lim
       if (contexts && (before?.text !== null || after?.text !== null)) {
         const paths = [...new Set([oldPath, newPath].filter((path): path is string => path !== null))];
         // Literal pathspecs preserve filenames containing Git pathspec metacharacters.
-        const patch = run('diff', '--ignore-submodules=none', '--no-relative', '--no-ext-diff', '--no-textconv', '--no-color', '--unified=0', '-M', from, to, '--', ...paths.map(path => `:(literal)${path}`)).toString();
+        const patch = accountDiff(run('diff', '--ignore-submodules=none', '--no-relative', '--no-ext-diff', '--no-textconv', '--no-color', '--unified=0', '-M', from, to, '--', ...paths.map(path => `:(literal)${path}`))).toString();
         for (const line of patch.split('\n')) {
           const hunk = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)$/.exec(line);
           if (hunk) ranges.push({ oldStart: +hunk[1]!, oldCount: +(hunk[2] ?? 1), newStart: +hunk[3]!, newCount: +(hunk[4] ?? 1), name: hunk[5]!.trim() });

test/history.test.ts

@@ -225,3 +225,12 @@ it('supports a caller-selected symlinked gitdir with regular object storage', ()
   rmSync(metadata, { recursive: true }); renameSync(join(f.dir, '.git'), metadata); symlinkSync(metadata, join(f.dir, '.git'));
   expect(readHistory(f.dir, f.base).final[0]!.after!.text).toBe('changed\n');
 });
+
+it.each([
+  [{ maxDiffBytes: 1 }, /diff byte budget/i],
+  [{ maxFileEntries: 1 }, /file entry budget/i],
+] as const)('bounds accumulated diff records with %j', (limits, error) => {
+  const f = fixture(); f.write('a.txt', 'changed\n'); f.commit('P1');
+  expect(() => readHistory(f.dir, f.base, 'HEAD', limits)).toThrow(error);
+  expect(readHistory(f.dir, f.base, 'HEAD', { maxFileEntries: 2 }).final).toHaveLength(1);
+});