From 2d6211fd6e9f5b50f50d57a7d657a5f49a4e7afc Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 23 Jul 2026 22:26:49 -0500 Subject: [PATCH] fix(review): bound memory for large untracked files --- apps/pi-extension/server/serverReview.ts | 6 + .../components/AllFilesCodeView.tsx | 2 +- .../review-editor/components/DiffViewer.tsx | 2 +- packages/server/review-workspace.test.ts | 74 ++++++++++ packages/server/review.ts | 5 + packages/shared/diff-fingerprint.test.ts | 26 +++- packages/shared/review-core.test.ts | 112 +++++++++++++- packages/shared/review-core.ts | 138 +++++++++++++++++- 8 files changed, 355 insertions(+), 10 deletions(-) diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index c25a06ba2..c5b8901b0 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -43,6 +43,7 @@ import { detectRemoteDefaultInfo, getFileContentsForDiff as getFileContentsForDiffCore, getSinceBaseSections, + isBinaryPatchFile, isSameCwdCommitSwitch, listPatchFiles, parseCommitDiffType, @@ -2328,6 +2329,11 @@ export async function startReviewServer(options: { } } + if (isBinaryPatchFile(currentPatch, filePath)) { + json(res, { oldContent: null, newContent: null }); + return; + } + if (workspace) { try { const result = await workspace.getFileContents(filePath, oldPath); diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 0b662b75b..e40a2d0f6 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -1101,7 +1101,7 @@ export const AllFilesCodeView: React.FC = ({ newFile: data.newContent != null ? { name: file.path, contents: data.newContent } : undefined, }); - if (!result) { + if (!result || result.isPartial) { augmentState.set(itemId, { status: 'done', controller, generation }); return; } diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index 133ac6678..c36fa5871 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -353,7 +353,7 @@ export const DiffViewer: React.FC = ({ oldFile: fileContents.old != null ? { name: oldPath || filePath, contents: fileContents.old } : undefined, newFile: fileContents.new != null ? { name: filePath, contents: fileContents.new } : undefined, }); - return result || fileDiff; + return result && !result.isPartial ? result : fileDiff; } catch { return fileDiff; } diff --git a/packages/server/review-workspace.test.ts b/packages/server/review-workspace.test.ts index bf6acef53..b6a238275 100644 --- a/packages/server/review-workspace.test.ts +++ b/packages/server/review-workspace.test.ts @@ -947,6 +947,80 @@ describe("review-workspace", () => { }); describe("workspace review server integration", () => { + it("short-circuits binary file expansion before provider content retrieval", async () => { + const root = makeTempDir("plannotator-workspace-binary-content-"); + const repo = join(root, "api"); + mkdirSync(join(repo, ".git"), { recursive: true }); + let fileContentCalls = 0; + + const runtime = { + async getVcsContext(cwd?: string): Promise { + return { + vcsType: "git", + currentBranch: "main", + defaultBranch: "main", + cwd: cwd ?? repo, + worktrees: [], + availableBranches: { local: [], remote: [] }, + diffOptions: [{ id: "uncommitted", label: "Uncommitted changes" }], + }; + }, + async runVcsDiff() { + return { + patch: [ + "diff --git a/asset.bin b/asset.bin", + "new file mode 100644", + "Binary files /dev/null and b/asset.bin differ", + "", + ].join("\n"), + label: "Uncommitted changes", + }; + }, + async getVcsFileContentsForDiff() { + fileContentCalls += 1; + return { oldContent: "unexpected", newContent: "unexpected" }; + }, + async getVcsDiffFingerprint() { + return "stable-binary-workspace"; + }, + async canStageFiles() { + return false; + }, + async stageFile() {}, + async unstageFile() {}, + }; + + const workspace = await WorkspaceReviewSession.create(runtime, root); + const aggregate = aggregateWorkspacePatch(workspace.repos); + const server = await startReviewServer({ + rawPatch: aggregate.rawPatch, + gitRef: aggregate.gitRef, + error: aggregate.errors.join("\n") || undefined, + origin: "claude-code", + workspace, + agentCwd: workspace.root, + htmlContent: "review", + }); + + try { + const diffPayload = await fetch(`${server.url}/api/diff`).then((response) => + response.json() + ) as { snapshotId: string }; + const response = await fetch( + `${server.url}/api/file-content?path=api/asset.bin&snapshot=${encodeURIComponent(diffPayload.snapshotId)}`, + ); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + oldContent: null, + newContent: null, + }); + expect(fileContentCalls).toBe(0); + } finally { + server.stop(); + } + }); + it("maps one workspace mode across mixed Git and JJ repos", async () => { const root = makeTempDir("plannotator-workspace-mixed-vcs-"); const gitRepo = join(root, "api"); diff --git a/packages/server/review.ts b/packages/server/review.ts index 9a69dd042..705f1a3c8 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -22,6 +22,7 @@ import { resolveBaseBranch, getSinceBaseSections, detectRemoteDefaultInfo, + isBinaryPatchFile, listPatchFiles, type RemoteDefaultInfo, type SinceBaseSections, @@ -2350,6 +2351,10 @@ export async function startReviewServer( } } + if (isBinaryPatchFile(currentPatch, filePath)) { + return Response.json({ oldContent: null, newContent: null }); + } + if (workspace) { try { const result = await workspace.getFileContents(filePath, oldPath); diff --git a/packages/shared/diff-fingerprint.test.ts b/packages/shared/diff-fingerprint.test.ts index 264fdac30..ddd6da227 100644 --- a/packages/shared/diff-fingerprint.test.ts +++ b/packages/shared/diff-fingerprint.test.ts @@ -2,7 +2,11 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { getGitDiffFingerprint, type ReviewGitRuntime } from "./review-core"; +import { + getGitDiffFingerprint, + MAX_REVIEW_FILE_CONTENT_BYTES, + type ReviewGitRuntime, +} from "./review-core"; // Real-git runtime against a throwaway repo — fingerprints are only meaningful // against actual VCS behavior, so no mocks. @@ -80,6 +84,26 @@ describe("getGitDiffFingerprint", () => { expect(edited).not.toBe(created!); }); + test("large untracked files use metadata without entering the JS heap", async () => { + const path = join(repo, "large-untracked.bin"); + writeFileSync(path, Buffer.alloc(MAX_REVIEW_FILE_CONTENT_BYTES + 1)); + let largeFileReads = 0; + const guardedRuntime: ReviewGitRuntime = { + ...runtime, + async readTextFile(requestedPath) { + if (requestedPath === path) largeFileReads++; + return runtime.readTextFile(requestedPath); + }, + }; + + const before = await getGitDiffFingerprint(guardedRuntime, "uncommitted", "main", repo); + writeFileSync(path, Buffer.alloc(MAX_REVIEW_FILE_CONTENT_BYTES + 2, 1)); + const after = await getGitDiffFingerprint(guardedRuntime, "uncommitted", "main", repo); + + expect(largeFileReads).toBe(0); + expect(after).not.toBe(before); + }); + test("uncommitted: changes when a commit lands (HEAD moves)", async () => { const before = await getGitDiffFingerprint(runtime, "uncommitted", "main", repo); await git("add", "-A"); diff --git a/packages/shared/review-core.test.ts b/packages/shared/review-core.test.ts index 28ec7733a..dcf90e944 100644 --- a/packages/shared/review-core.test.ts +++ b/packages/shared/review-core.test.ts @@ -1,6 +1,13 @@ import { afterEach, describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve as resolvePath } from "node:path"; import { @@ -12,8 +19,11 @@ import { getWorkingTreeDiffFromBase, gitAddFile, gitResetFile, + isBinaryPatchFile, isSameCwdCommitSwitch, + listPatchFiles, listRecentCommits, + MAX_REVIEW_FILE_CONTENT_BYTES, parseCommitDiffType, parseWorktreeDiffType, prepareGitCommand, @@ -235,6 +245,69 @@ describe("review-core", () => { expect(result.patch).toContain("+++ b/untracked.txt"); }); + test("large untracked files stay visible without diffing their contents", async () => { + const repoDir = initRepo(); + const runtime = makeRuntime(repoDir); + writeFileSync( + join(repoDir, "large build.bin"), + Buffer.alloc(MAX_REVIEW_FILE_CONTENT_BYTES + 1), + ); + + const result = await runGitDiff(runtime, "uncommitted", "main"); + + expect(result.patch).toContain('diff --git "a/large build.bin" "b/large build.bin"'); + expect(result.patch).toContain('Binary files /dev/null and "b/large build.bin" differ'); + expect(listPatchFiles(result.patch)).toContainEqual({ + path: "large build.bin", + additions: 0, + deletions: 0, + }); + expect(isBinaryPatchFile(result.patch, "large build.bin")).toBe(true); + }); + + test("binary patch detection follows rename metadata", () => { + const patch = [ + 'diff --git "a/old name.bin" "b/new name.bin"', + "similarity index 100%", + "rename from old name.bin", + "rename to new name.bin", + "GIT binary patch", + "", + ].join("\n"); + + expect(isBinaryPatchFile(patch, "new name.bin")).toBe(true); + expect(isBinaryPatchFile(patch, "old name.bin")).toBe(false); + }); + + test("untracked diff collection caps concurrent git processes", async () => { + const repoDir = initRepo(); + const baseRuntime = makeRuntime(repoDir); + for (let index = 0; index < 12; index++) { + writeFileSync(join(repoDir, `untracked-${index}.txt`), `${index}\n`); + } + let active = 0; + let peak = 0; + const runtime: ReviewGitRuntime = { + ...baseRuntime, + async runGit(args, options) { + if (!args.includes("--no-index")) return baseRuntime.runGit(args, options); + active++; + peak = Math.max(peak, active); + await new Promise((resolve) => setTimeout(resolve, 10)); + try { + return await baseRuntime.runGit(args, options); + } finally { + active--; + } + }, + }; + + await runGitDiff(runtime, "uncommitted", "main"); + + expect(peak).toBeGreaterThan(1); + expect(peak).toBeLessThanOrEqual(4); + }); + test("ordinary working-tree diffs keep tracked changes when an untracked file cannot be read", async () => { const runtime: ReviewGitRuntime = { async runGit(args) { @@ -515,6 +588,43 @@ describe("review-core", () => { expect(newFileContents.newContent).toBe("brand new\n"); }); + test("file content lookup refuses oversized working-tree files", async () => { + const repoDir = initRepo(); + const runtime = makeRuntime(repoDir); + writeFileSync( + join(repoDir, "large-generated.js"), + Buffer.alloc(MAX_REVIEW_FILE_CONTENT_BYTES + 1, 0x20), + ); + + const contents = await getFileContentsForDiff( + runtime, + "uncommitted", + "main", + "large-generated.js", + ); + + expect(contents).toEqual({ oldContent: null, newContent: null }); + }); + + test("file content lookup reads a symlink payload without following its target", async () => { + const repoDir = initRepo(); + const runtime = makeRuntime(repoDir); + writeFileSync( + join(repoDir, "large-target.bin"), + Buffer.alloc(MAX_REVIEW_FILE_CONTENT_BYTES + 1), + ); + symlinkSync("large-target.bin", join(repoDir, "generated-link")); + + const contents = await getFileContentsForDiff( + runtime, + "uncommitted", + "main", + "generated-link", + ); + + expect(contents).toEqual({ oldContent: null, newContent: "large-target.bin" }); + }); + test("getDefaultBranch falls back to local when origin/HEAD points at an unfetched ref", () => { // Simulates a narrow / partial clone where origin/HEAD is configured but // the target ref was never fetched. Before the verify step, the server diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index 871250cdf..c15422bee 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -5,10 +5,25 @@ * self-contained while review diff logic remains sourced from one module. */ +import { lstat, readlink } from "node:fs/promises"; import { resolve as resolvePath } from "node:path"; -import { unquoteGitPath, parsePatchPathToken, parseDiffFilePathLines, parseDiffGitHeader } from "./diff-paths"; +import { + formatPatchPathToken, + unquoteGitPath, + parsePatchPathToken, + parseDiffFilePathLines, + parseDiffGitHeader, + parseDiffMetadataPathLines, +} from "./diff-paths"; export const JJ_TRUNK_REVSET = "trunk()"; +/** Maximum regular-file payload accepted for Git diff expansion. */ +export const MAX_REVIEW_FILE_CONTENT_BYTES = 5 * 1024 * 1024; + +const MAX_UNTRACKED_DIFF_CONCURRENCY = 4; +// Fingerprints run every few seconds, so use a deliberately lower read ceiling +// than one-shot diff generation. Larger files use size + mtime metadata. +const MAX_UNTRACKED_FINGERPRINT_CONTENT_BYTES = 1024 * 1024; export type DiffType = | "since-base" @@ -724,8 +739,51 @@ async function getUntrackedFileDiffs( if (files.length === 0) return { diff: "", paths: [] }; - const diffs = await Promise.all( - files.map(async (file) => { + const mapWithConcurrency = async ( + items: T[], + concurrency: number, + mapper: (item: T) => Promise, + ): Promise => { + const results = new Array(items.length); + let nextIndex = 0; + const workers = Array.from( + { length: Math.min(concurrency, items.length) }, + async () => { + while (nextIndex < items.length) { + const index = nextIndex++; + results[index] = await mapper(items[index]); + } + }, + ); + await Promise.all(workers); + return results; + }; + + const diffs = await mapWithConcurrency( + files, + MAX_UNTRACKED_DIFF_CONCURRENCY, + async (file) => { + // Avoid asking Git to inspect arbitrarily large untracked payloads. They + // remain visible in the review as binary additions, but their bytes never + // enter Git's diff machinery or the server's buffered stdout. + try { + const fileStat = await lstat(resolvePath(rootCwd ?? "", file)); + if (fileStat.isFile() && fileStat.size > MAX_REVIEW_FILE_CONTENT_BYTES) { + const mode = (fileStat.mode & 0o111) !== 0 ? "100755" : "100644"; + const oldToken = formatPatchPathToken("a", file); + const newToken = formatPatchPathToken("b", file); + return [ + `diff --git ${oldToken} ${newToken}`, + `new file mode ${mode}`, + `Binary files /dev/null and ${newToken} differ`, + "", + ].join("\n"); + } + } catch { + // Preserve the existing best-effort/strict behavior below: Git reports + // the authoritative read error for files that disappear mid-snapshot. + } + const diffResult = await runtime.runGit( [ "diff", @@ -752,7 +810,7 @@ async function getUntrackedFileDiffs( ); } return diffResult.stdout; - }), + }, ); return { diff: diffs.join(""), paths: files }; @@ -1274,7 +1332,31 @@ async function appendUntrackedFingerprint( if (untracked.length > 0) { const baseDir = await resolveRepoToplevel(runtime, cwd); for (const path of untracked) { - const content = await runtime.readTextFile(baseDir ? resolvePath(baseDir, path) : path); + const fullPath = baseDir ? resolvePath(baseDir, path) : path; + try { + const fileStat = await lstat(fullPath); + if (fileStat.isSymbolicLink()) { + // Hash the link payload Git records without following it into a + // potentially huge target file. + parts.push(hashFingerprintPart(`symlink:${await readlink(fullPath)}`)); + continue; + } + if (!fileStat.isFile()) { + parts.push(`non-file:${fileStat.size}:${fileStat.mtimeMs}`); + continue; + } + if (fileStat.size > MAX_UNTRACKED_FINGERPRINT_CONTENT_BYTES) { + // A metadata fingerprint avoids decoding a multi-GB binary into a JS + // string every five seconds. Size/mtime changes still invalidate the + // review, while small files retain content-accurate detection. + parts.push(`large:${fileStat.size}:${fileStat.mtimeMs}`); + continue; + } + } catch { + parts.push("unreadable"); + continue; + } + const content = await runtime.readTextFile(fullPath); parts.push(content != null ? hashFingerprintPart(content) : "unreadable"); } } @@ -1407,8 +1489,16 @@ export async function getFileContentsForDiff( } async function gitShow(ref: string, path: string): Promise { + const object = `${ref}:${path}`; + const sizeResult = await runtime.runGit( + ["cat-file", "-s", "--", object], + { cwd }, + ); + if (sizeResult.exitCode !== 0) return null; + const size = Number(sizeResult.stdout.trim()); + if (!Number.isFinite(size) || size > MAX_REVIEW_FILE_CONTENT_BYTES) return null; // `--end-of-options` hardens against user-supplied refs starting with `-`. - const result = await runtime.runGit(["show", "--end-of-options", `${ref}:${path}`], { cwd }); + const result = await runtime.runGit(["show", "--end-of-options", object], { cwd }); return result.exitCode === 0 ? result.stdout : null; } @@ -1419,6 +1509,16 @@ export async function getFileContentsForDiff( // sibling is immune: ref paths are root-relative regardless of cwd.) const baseDir = await resolveRepoToplevel(runtime, cwd); const fullPath = baseDir ? resolvePath(baseDir, path) : path; + try { + const fileStat = await lstat(fullPath); + // Git stores the link destination as the blob contents. Reading the link + // itself preserves expansion without following an arbitrarily large + // target. + if (fileStat.isSymbolicLink()) return await readlink(fullPath); + if (!fileStat.isFile() || fileStat.size > MAX_REVIEW_FILE_CONTENT_BYTES) return null; + } catch { + return null; + } return runtime.readTextFile(fullPath); } @@ -1732,3 +1832,29 @@ export function listPatchFiles( return files; } + +/** Whether the named file's patch chunk contains a Git binary marker. */ +export function isBinaryPatchFile(patch: string, filePath: string): boolean { + const chunkStarts = [...patch.matchAll(/^diff --git /gm)]; + for (let i = 0; i < chunkStarts.length; i++) { + const start = chunkStarts[i].index ?? 0; + const end = chunkStarts[i + 1]?.index ?? patch.length; + const chunk = patch.slice(start, end); + const lines = chunk.split("\n"); + const header = parseDiffGitHeader(lines[0] ?? ""); + const fileLines = parseDiffFilePathLines(lines); + const metadata = parseDiffMetadataPathLines(lines); + const path = + metadata.newPath ?? + fileLines.newPath ?? + header.newPath ?? + metadata.oldPath ?? + fileLines.oldPath ?? + header.oldPath; + if (path !== filePath) continue; + return lines.some( + (line) => line === "GIT binary patch" || line.startsWith("Binary files "), + ); + } + return false; +}