diff --git a/apps/pi-extension/server/vcs.ts b/apps/pi-extension/server/vcs.ts index bde221212..693514725 100644 --- a/apps/pi-extension/server/vcs.ts +++ b/apps/pi-extension/server/vcs.ts @@ -1,5 +1,6 @@ import { spawn, spawnSync } from "node:child_process"; -import { readFileSync } from "node:fs"; +import { lstatSync, readFileSync, readlinkSync } from "node:fs"; +import { resolve as resolvePath } from "node:path"; import { type DiffResult, type DiffType, @@ -42,7 +43,7 @@ function runCommand( cwd: options?.cwd, detached: isolateProcessGroup, env: preparedGitCommand?.env ?? commandEnvironment, - stdio: ["ignore", "pipe", "pipe"], + stdio: [options?.stdin === undefined ? "ignore" : "pipe", "pipe", "pipe"], windowsHide: true, }); @@ -73,6 +74,7 @@ function runCommand( const stderrChunks: Buffer[] = []; proc.stdout!.on("data", (chunk: Buffer) => stdoutChunks.push(chunk)); proc.stderr!.on("data", (chunk: Buffer) => stderrChunks.push(chunk)); + if (options?.stdin !== undefined) proc.stdin!.end(options.stdin); proc.on("close", (code) => { if (timer) clearTimeout(timer); @@ -106,6 +108,31 @@ export const reviewRuntime: ReviewGitRuntime = { return null; } }, + + async getFileInfo(basePath, path) { + const fullPath = resolvePath(basePath ?? "", path); + try { + const fileStat = lstatSync(fullPath); + return { + path: fullPath, + size: fileStat.size, + mtimeMs: fileStat.mtimeMs, + isFile: fileStat.isFile(), + isSymbolicLink: fileStat.isSymbolicLink(), + isExecutable: (fileStat.mode & 0o111) !== 0, + }; + } catch { + return null; + } + }, + + async readLink(path: string): Promise { + try { + return readlinkSync(path); + } catch { + return null; + } + }, }; export const jjRuntime: ReviewJjRuntime = { diff --git a/packages/server/git-background.test.ts b/packages/server/git-background.test.ts index f04461b5f..05f1a061d 100644 --- a/packages/server/git-background.test.ts +++ b/packages/server/git-background.test.ts @@ -1,6 +1,14 @@ import { afterEach, describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { pathToFileURL } from "node:url"; @@ -98,6 +106,36 @@ process.exit(1); }; } +describe.skipIf(process.platform === "win32")("review runtime filesystem seam", () => { + for (const fixture of fixtures) { + test(`${fixture.name} resolves file metadata and symlink payloads`, async () => { + const root = mkdtempSync(join(tmpdir(), "plannotator-runtime-file-")); + tempDirs.push(root); + const file = join(root, "file.txt"); + const link = join(root, "file-link"); + writeFileSync(file, "content\n", "utf-8"); + symlinkSync("file.txt", link); + + const runtimeModule = await import(pathToFileURL(fixture.modulePath).href); + const runtime = runtimeModule[fixture.exportName] as { + getFileInfo(basePath: string, path: string): Promise<{ + path: string; + size: number; + isFile: boolean; + isSymbolicLink: boolean; + } | null>; + readLink(path: string): Promise; + }; + const fileInfo = await runtime.getFileInfo(root, "file.txt"); + const linkInfo = await runtime.getFileInfo(root, "file-link"); + + expect(fileInfo).toMatchObject({ path: file, size: 8, isFile: true, isSymbolicLink: false }); + expect(linkInfo).toMatchObject({ path: link, isFile: false, isSymbolicLink: true }); + await expect(runtime.readLink(link)).resolves.toBe("file.txt"); + }); + } +}); + function createHttpCredentialFixture(remoteUrl: string): { repo: string; askpassMarker: string; diff --git a/packages/server/git.ts b/packages/server/git.ts index dcc5de0ea..93ef44f49 100644 --- a/packages/server/git.ts +++ b/packages/server/git.ts @@ -5,6 +5,9 @@ * Used by both Claude Code hook and OpenCode plugin. */ +import { lstat, readlink } from "node:fs/promises"; +import { resolve as resolvePath } from "node:path"; + import { type DiffOption, type DiffResult, @@ -47,7 +50,9 @@ async function runGit( cwd: options?.cwd, detached: command.isolateProcessGroup, env: command.env, - stdin: "ignore", + stdin: options?.stdin === undefined + ? "ignore" + : new TextEncoder().encode(options.stdin), stdout: "pipe", stderr: "pipe", windowsHide: true, @@ -96,6 +101,29 @@ export const runtime: ReviewGitRuntime = { return null; } }, + async getFileInfo(basePath, path) { + const fullPath = resolvePath(basePath ?? "", path); + try { + const fileStat = await lstat(fullPath); + return { + path: fullPath, + size: fileStat.size, + mtimeMs: fileStat.mtimeMs, + isFile: fileStat.isFile(), + isSymbolicLink: fileStat.isSymbolicLink(), + isExecutable: (fileStat.mode & 0o111) !== 0, + }; + } catch { + return null; + } + }, + async readLink(path: string): Promise { + try { + return await readlink(path); + } catch { + return null; + } + }, }; export function getCurrentBranch(): Promise { diff --git a/packages/shared/commit-history.test.ts b/packages/shared/commit-history.test.ts index b8a07b0f0..1c441cfc2 100644 --- a/packages/shared/commit-history.test.ts +++ b/packages/shared/commit-history.test.ts @@ -26,6 +26,12 @@ function git(cwd: string, args: string[]): string { function makeRuntime(baseCwd: string): ReviewGitRuntime { return { + async getFileInfo() { + return null; + }, + async readLink() { + return null; + }, async runGit(args: string[], options?: { cwd?: string }) { const result = spawnSync("git", args, { cwd: options?.cwd ?? baseCwd, diff --git a/packages/shared/diff-fingerprint.test.ts b/packages/shared/diff-fingerprint.test.ts index ddd6da227..b0a2057ae 100644 --- a/packages/shared/diff-fingerprint.test.ts +++ b/packages/shared/diff-fingerprint.test.ts @@ -1,7 +1,7 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { lstatSync, mkdtempSync, readlinkSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, resolve as resolvePath } from "node:path"; import { getGitDiffFingerprint, MAX_REVIEW_FILE_CONTENT_BYTES, @@ -16,6 +16,9 @@ const runtime: ReviewGitRuntime = { cwd: options?.cwd, stdout: "pipe", stderr: "pipe", + stdin: options?.stdin === undefined + ? "ignore" + : new TextEncoder().encode(options.stdin), }); const [stdout, stderr] = await Promise.all([ new Response(proc.stdout).text(), @@ -31,6 +34,29 @@ const runtime: ReviewGitRuntime = { return null; } }, + async getFileInfo(basePath, path) { + const fullPath = resolvePath(basePath ?? "", path); + try { + const fileStat = lstatSync(fullPath); + return { + path: fullPath, + size: fileStat.size, + mtimeMs: fileStat.mtimeMs, + isFile: fileStat.isFile(), + isSymbolicLink: fileStat.isSymbolicLink(), + isExecutable: (fileStat.mode & 0o111) !== 0, + }; + } catch { + return null; + } + }, + async readLink(path) { + try { + return readlinkSync(path); + } catch { + return null; + } + }, }; let repo: string; diff --git a/packages/shared/gitbutler-core.test.ts b/packages/shared/gitbutler-core.test.ts index c0ea84d9c..6965c2255 100644 --- a/packages/shared/gitbutler-core.test.ts +++ b/packages/shared/gitbutler-core.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import type { GitCommandOptions, GitCommandResult } from "./review-core"; +import { MAX_REVIEW_FILE_CONTENT_BYTES } from "./review-core"; import { GITBUTLER_WORKSPACE_DIFF, GitButlerContractError, @@ -25,6 +26,8 @@ const ROOT = "/repo"; const MERGE_BASE = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; const LOWER_TIP = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; const TOP_TIP = "cccccccccccccccccccccccccccccccccccccccc"; +const PATCH_OLD_OBJECT = "d".repeat(40); +const PATCH_NEW_OBJECT = "e".repeat(40); function commandResult( stdout = "", @@ -87,7 +90,13 @@ function createRuntime(options: { let patch = "diff --git a/file.txt b/file.txt\n-old\n+new\n"; const runtime: ReviewGitButlerRuntime = { - async runGit(args: string[]): Promise { + async getFileInfo() { + return null; + }, + async readLink() { + return null; + }, + async runGit(args: string[], commandOptions?: GitCommandOptions): Promise { gitCalls.push(args); const commandArgs = args[0] === "--no-optional-locks" ? args.slice(1) : args; if (commandArgs[0] === "symbolic-ref") { @@ -116,6 +125,21 @@ function createRuntime(options: { if (commandArgs[0] === "merge-base") { return commandResult(`${MERGE_BASE}\n`); } + if (commandArgs[0] === "cat-file" && commandArgs[1] === "-s") { + return commandResult("10\n"); + } + if (commandArgs[0] === "cat-file" && commandArgs.some((arg) => arg.startsWith("--batch-check"))) { + return commandResult( + (commandOptions?.stdin ?? "").trim().split("\n").filter(Boolean).map((objectId) => + `${objectId} blob 10`, + ).join("\n"), + ); + } + if (commandArgs[0] === "diff" && commandArgs.includes("--raw")) { + return commandResult( + `:100644 100644 ${PATCH_OLD_OBJECT} ${PATCH_NEW_OBJECT} M\0file.txt\0`, + ); + } if (commandArgs[0] === "diff") return commandResult(patch); if (commandArgs[0] === "status") return commandResult(); if (commandArgs[0] === "ls-files") return commandResult(); @@ -542,6 +566,54 @@ describe("GitButler diffs and expansion", () => { )).resolves.toMatchObject({ patch }); }); + test("omits oversized committed object diffs with a content-sensitive binary stub", async () => { + const fixture = createRuntime(); + const originalRunGit = fixture.runtime.runGit.bind(fixture.runtime); + const oldObjectId = "d".repeat(40); + let newObjectId = "e".repeat(40); + let sawRawDiff = false; + fixture.setPatch("x".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1)); + fixture.runtime.runGit = async (args, options) => { + const commandArgs = args[0] === "-c" ? args.slice(2) : args; + if (commandArgs[0] === "diff" && commandArgs.includes("--raw")) { + sawRawDiff = true; + return commandResult( + `:100644 100644 ${oldObjectId} ${newObjectId} M\0large [*]?.txt\0`, + ); + } + if (commandArgs[0] === "cat-file") { + return commandResult(`${MAX_REVIEW_FILE_CONTENT_BYTES + 1}\n`); + } + if (commandArgs[0] === "diff" && commandArgs.some((arg) => arg.startsWith(":(top,exclude,literal)"))) { + return commandResult(); + } + return originalRunGit(args, options); + }; + + const result = await runGitButlerDiff( + fixture.runtime, + "gitbutler:branch:feature%2Ftop%20lane", + ROOT, + ); + expect(result.patch.length).toBeLessThan(2_000); + expect(result.patch).toContain("Binary files"); + expect(result.patch).not.toContain("xxxxxxxxxx"); + expect(sawRawDiff).toBe(true); + + const first = await getGitButlerDiffFingerprint( + fixture.runtime, + "gitbutler:branch:feature%2Ftop%20lane", + ROOT, + ); + newObjectId = "f".repeat(40); + const second = await getGitButlerDiffFingerprint( + fixture.runtime, + "gitbutler:branch:feature%2Ftop%20lane", + ROOT, + ); + expect(second).not.toBe(first); + }); + test("returns explicit errors when status or a selected target disappears", async () => { const failedStatus = createRuntime({ status: commandResult("", "database locked", 1) }); await expect(runGitButlerDiff(failedStatus.runtime, GITBUTLER_WORKSPACE_DIFF, ROOT)).resolves.toMatchObject({ @@ -574,6 +646,28 @@ describe("GitButler diffs and expansion", () => { }); }); + test("does not expand oversized committed GitButler blobs", async () => { + const fixture = createRuntime(); + const originalRunGit = fixture.runtime.runGit.bind(fixture.runtime); + let showCalls = 0; + fixture.runtime.runGit = async (args, options) => { + if (args[0] === "cat-file" && args[1] === "-s") { + return commandResult(`${MAX_REVIEW_FILE_CONTENT_BYTES + 1}\n`); + } + if (args[0] === "show") showCalls++; + return originalRunGit(args, options); + }; + + await expect(getGitButlerFileContentsForDiff( + fixture.runtime, + "gitbutler:branch:feature%2Ftop%20lane", + "src/new.ts", + "src/old.ts", + ROOT, + )).resolves.toEqual({ oldContent: null, newContent: null }); + expect(showCalls).toBe(0); + }); + test("fingerprints the exact visible patch content", async () => { const fixture = createRuntime(); const first = await getGitButlerDiffFingerprint( diff --git a/packages/shared/gitbutler-core.ts b/packages/shared/gitbutler-core.ts index e0a8d5915..cc47e8bc9 100644 --- a/packages/shared/gitbutler-core.ts +++ b/packages/shared/gitbutler-core.ts @@ -6,6 +6,7 @@ * and authoritative Git object-to-object diffs. */ +import { lstat, readlink } from "node:fs/promises"; import { basename, resolve } from "node:path"; import { @@ -19,6 +20,8 @@ import { getEmptyTreeSha, getWorkingTreeDiffFromBase, hashFingerprintPart, + MAX_REVIEW_FILE_CONTENT_BYTES, + runBoundedTrackedDiff, validateFilePath, } from "./review-core"; @@ -595,13 +598,14 @@ async function diffObjects( "--end-of-options", `${base}..${tip}`, ]; - const result = await runtime.runGit(args, { cwd }); - if (result.exitCode !== 0) { + try { + return await runBoundedTrackedDiff(runtime, args, cwd); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); throw new GitButlerContractError( - `Git failed while building the GitButler diff${result.stderr.trim() ? `: ${result.stderr.trim()}` : "."}`, + `Git failed while building the GitButler diff: ${message}`, ); } - return result.stdout; } function errorResult(diffType: DiffType, error: unknown): DiffResult { @@ -691,10 +695,29 @@ async function gitShow( path: string, cwd: string, ): Promise { - const result = await runtime.runGit(["show", "--end-of-options", `${ref}:${path}`], { cwd }); + const object = `${ref}:${path}`; + const size = await runtime.runGit(["cat-file", "-s", "--", object], { cwd }); + if (size.exitCode !== 0 || Number(size.stdout.trim()) > MAX_REVIEW_FILE_CONTENT_BYTES) return null; + const result = await runtime.runGit(["show", "--end-of-options", object], { cwd }); return result.exitCode === 0 ? result.stdout : null; } +async function readWorkingTreeFile( + runtime: ReviewGitButlerRuntime, + root: string, + path: string, +): Promise { + const fullPath = resolve(root, path); + try { + const fileStat = await lstat(fullPath); + 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); +} + /** Resolve full old/new file content for expandable GitButler diffs. */ export async function getGitButlerFileContentsForDiff( runtime: ReviewGitButlerRuntime, @@ -717,7 +740,7 @@ export async function getGitButlerFileContentsForDiff( await validateWorkspaceMergeBase(runtime, status.mergeBase.commitId, root); return { oldContent: await gitShow(runtime, status.mergeBase.commitId, oldFilePath, root), - newContent: await runtime.readTextFile(resolve(root, filePath)), + newContent: await readWorkingTreeFile(runtime, root, filePath), }; } diff --git a/packages/shared/pr-stack.test.ts b/packages/shared/pr-stack.test.ts index c5d389c2a..e5f8580d3 100644 --- a/packages/shared/pr-stack.test.ts +++ b/packages/shared/pr-stack.test.ts @@ -1,12 +1,21 @@ import { describe, expect, test } from "bun:test"; import type { PRMetadata } from "./pr-types"; -import type { GitCommandResult, ReviewGitRuntime } from "./review-core"; +import { + MAX_REVIEW_FILE_CONTENT_BYTES, + type GitCommandResult, + type ReviewGitRuntime, +} from "./review-core"; import { runPRFullStackDiff, runPRLayerLocalDiff } from "./pr-stack"; function result(stdout = "", stderr = "", exitCode = 0): GitCommandResult { return { stdout, stderr, exitCode }; } +const unavailableFileMethods = { + async getFileInfo() { return null; }, + async readLink() { return null; }, +}; + const metadata: PRMetadata = { platform: "github", host: "github.com", @@ -27,11 +36,15 @@ describe("runPRFullStackDiff", () => { test("uses origin default branch when it is available", async () => { const calls: string[][] = []; const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, async runGit(args) { calls.push(args); if (args[0] === "show-ref" && args[3] === "refs/remotes/origin/main") { return result(); } + if (args[0] === "diff" && args.includes("--raw")) { + return result(); + } if (args[0] === "diff") { return result("diff --git a/src/auth.ts b/src/auth.ts\n"); } @@ -60,6 +73,7 @@ describe("runPRFullStackDiff", () => { test("falls back to a local default branch", async () => { const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, async runGit(args) { if (args[0] === "show-ref" && args[3] === "refs/remotes/origin/main") { return result("", "", 1); @@ -67,6 +81,9 @@ describe("runPRFullStackDiff", () => { if (args[0] === "show-ref" && args[3] === "refs/heads/main") { return result(); } + if (args[0] === "diff" && args.includes("--raw")) { + return result(); + } if (args[0] === "diff") { return result("local branch patch"); } @@ -87,6 +104,7 @@ describe("runPRFullStackDiff", () => { test("returns an error when no default branch ref exists locally", async () => { const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, async runGit() { return result("", "", 1); }, @@ -101,6 +119,41 @@ describe("runPRFullStackDiff", () => { expect(diff.label).toBe("Full stack diff unavailable"); expect(diff.error).toContain("Could not find origin/main or local main"); }); + + test("omits oversized tracked object content with literal pathspec exclusions", async () => { + const calls: string[][] = []; + const oldObjectId = "a".repeat(40); + const newObjectId = "b".repeat(40); + const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, + async runGit(args) { + calls.push(args); + if (args[0] === "show-ref") return result(); + if (args[0] === "cat-file") return result(`${MAX_REVIEW_FILE_CONTENT_BYTES + 1}\n`); + if (args[0] === "diff" && args.includes("--raw")) { + return result( + `:100644 100644 ${oldObjectId} ${newObjectId} M\0large [*]?.txt\0`, + ); + } + if (args[0] === "diff" && args.some((arg) => arg.startsWith(":(top,exclude,literal)"))) { + return result(); + } + if (args[0] === "diff") return result("x".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1)); + return result("", "unexpected", 1); + }, + async readTextFile() { + return null; + }, + }; + + const diff = await runPRFullStackDiff(runtime, metadata, "/repo"); + + expect(diff.patch.length).toBeLessThan(2_000); + expect(diff.patch).toContain("Binary files"); + expect(diff.patch).not.toContain("xxxxxxxxxx"); + expect(calls.some((args) => args[0] === "diff" && args.includes("--raw"))).toBe(true); + expect(calls.some((args) => args.some((arg) => arg === ":(top,exclude,literal)large [*]?.txt"))).toBe(true); + }); }); describe("runPRLayerLocalDiff", () => { @@ -127,6 +180,7 @@ describe("runPRLayerLocalDiff", () => { return { calls, runtime: { + ...unavailableFileMethods, async runGit(args) { calls.push(args); if (args[0] === "cat-file") { @@ -138,6 +192,9 @@ describe("runPRLayerLocalDiff", () => { if (opts.fetchable?.has(sha)) missing.delete(sha); return result(); } + if (args[0] === "diff" && args.includes("--raw")) { + return result(); + } if (args[0] === "diff") { return result( opts.diffStdout ?? "diff --git a/x.ts b/x.ts\n", @@ -242,4 +299,38 @@ describe("runPRLayerLocalDiff", () => { expect(diff.error).toContain("Invalid PR head SHA"); expect(calls.length).toBe(0); }); + + test("omits oversized layer objects before rendering their patch", async () => { + const calls: string[][] = []; + const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, + async runGit(args) { + calls.push(args); + if (args[0] === "cat-file" && args[1] === "-t") return result(); + if (args[0] === "cat-file" && args[1] === "-s") { + return result(`${MAX_REVIEW_FILE_CONTENT_BYTES + 1}\n`); + } + if (args[0] === "diff" && args.includes("--raw")) { + return result( + `:100644 100644 ${"a".repeat(40)} ${"b".repeat(40)} A\0large [*]?.txt\0`, + ); + } + if (args[0] === "diff" && args.some((arg) => arg.startsWith(":(top,exclude,literal)"))) { + return result(); + } + if (args[0] === "diff") return result("x".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1)); + return result("", "unexpected", 1); + }, + async readTextFile() { + return null; + }, + }; + + const diff = await runPRLayerLocalDiff(runtime, layerMetadata, "/repo"); + + expect(diff.patch.length).toBeLessThan(2_000); + expect(diff.patch).toContain("Binary files"); + expect(diff.patch).not.toContain("xxxxxxxxxx"); + expect(calls.some((args) => args[0] === "diff" && args.includes("--raw"))).toBe(true); + }); }); diff --git a/packages/shared/pr-stack.ts b/packages/shared/pr-stack.ts index 17dca3719..95eb4601b 100644 --- a/packages/shared/pr-stack.ts +++ b/packages/shared/pr-stack.ts @@ -1,4 +1,8 @@ -import type { DiffResult, ReviewGitRuntime } from "./review-core"; +import { + runBoundedTrackedDiff, + type DiffResult, + type ReviewGitRuntime, +} from "./review-core"; import { ensureObjectAvailable } from "./worktree"; import type { PRDiffScopeOption, @@ -13,6 +17,12 @@ function branchNameIsSafe(branch: string): boolean { return branch.trim().length > 0 && !branch.startsWith("-") && !branch.includes("\0"); } +function diffFailureMessage(error: unknown): string { + const message = error instanceof Error ? error.message : String(error); + const failedAt = message.indexOf(" failed: "); + return failedAt === -1 ? message : message.slice(failedAt + " failed: ".length); +} + export function getPRStackInfo(metadata: PRMetadata | undefined): PRStackInfo | null { if (!metadata?.defaultBranch) return null; if (metadata.baseBranch === metadata.defaultBranch) return null; @@ -120,9 +130,11 @@ export async function runPRFullStackDiff( "--end-of-options", `${baseRef}...HEAD`, ]; - const diff = await runtime.runGit(diffArgs, { cwd }); - if (diff.exitCode !== 0) { - const message = diff.stderr.trim() || `git ${diffArgs.join(" ")} failed`; + let patch: string; + try { + patch = await runBoundedTrackedDiff(runtime, diffArgs, cwd); + } catch (error) { + const message = diffFailureMessage(error) || `git diff failed`; return { patch: "", label: "Full stack diff unavailable", @@ -131,7 +143,7 @@ export async function runPRFullStackDiff( } return { - patch: diff.stdout, + patch, label: `Full stack diff vs ${baseRef}`, }; } @@ -198,16 +210,18 @@ export async function runPRLayerLocalDiff( return unavailable("Could not resolve the PR base commit in the local checkout."); } - const diff = await runtime.runGit(diffArgsFor(range), { cwd }); - if (diff.exitCode !== 0) { - const message = diff.stderr.trim() || "git diff failed"; + let patch: string; + try { + patch = await runBoundedTrackedDiff(runtime, diffArgsFor(range), cwd); + } catch (error) { + const message = diffFailureMessage(error) || "git diff failed"; return unavailable(message.split("\n").find((line) => line.trim().length > 0) ?? message); } - if (!diff.stdout.trim()) { + if (!patch.trim()) { return unavailable("Local recompute produced an empty diff."); } - return { patch: diff.stdout, label: "PR diff (recomputed locally)" }; + return { patch, label: "PR diff (recomputed locally)" }; } /** diff --git a/packages/shared/review-core.test-d.ts b/packages/shared/review-core.test-d.ts new file mode 100644 index 000000000..593aed0bf --- /dev/null +++ b/packages/shared/review-core.test-d.ts @@ -0,0 +1,11 @@ +import type { ReviewGitRuntime } from "./review-core"; + +type Assert = T; +type IsRequired = {} extends Pick ? false : true; + +type RuntimeRequiresFileInfo = Assert< + IsRequired +>; +type RuntimeRequiresReadLink = Assert< + IsRequired +>; diff --git a/packages/shared/review-core.test.ts b/packages/shared/review-core.test.ts index dcf90e944..c860cf057 100644 --- a/packages/shared/review-core.test.ts +++ b/packages/shared/review-core.test.ts @@ -1,9 +1,12 @@ import { afterEach, describe, expect, test } from "bun:test"; import { spawnSync } from "node:child_process"; import { + chmodSync, + lstatSync, mkdirSync, mkdtempSync, readFileSync, + readlinkSync, rmSync, symlinkSync, writeFileSync, @@ -33,6 +36,11 @@ import { type ReviewGitRuntime, } from "./review-core"; +const unavailableFileMethods = { + async getFileInfo() { return null; }, + async readLink() { return null; }, +}; + describe("splitPorcelainRename", () => { test("splits a plain rename on the top-level separator", () => { expect(splitPorcelainRename("old.txt -> new.txt")).toEqual(["old.txt", "new.txt"]); @@ -74,10 +82,12 @@ function git(cwd: string, args: string[]): string { function makeRuntime(baseCwd: string): ReviewGitRuntime { return { - async runGit(args: string[], options?: { cwd?: string }) { + async runGit(args: string[], options?: { cwd?: string; stdin?: string }) { const result = spawnSync("git", args, { cwd: options?.cwd ?? baseCwd, encoding: "utf-8", + maxBuffer: MAX_REVIEW_FILE_CONTENT_BYTES * 4, + input: options?.stdin, }); return { @@ -95,6 +105,31 @@ function makeRuntime(baseCwd: string): ReviewGitRuntime { return null; } }, + + async getFileInfo(basePath, path) { + const fullPath = resolvePath(basePath ?? baseCwd, path); + try { + const fileStat = lstatSync(fullPath); + return { + path: fullPath, + size: fileStat.size, + mtimeMs: fileStat.mtimeMs, + isFile: fileStat.isFile(), + isSymbolicLink: fileStat.isSymbolicLink(), + isExecutable: (fileStat.mode & 0o111) !== 0, + }; + } catch { + return null; + } + }, + + async readLink(path: string) { + try { + return readlinkSync(path); + } catch { + return null; + } + }, }; } @@ -190,6 +225,7 @@ describe("review-core", () => { test("remote-default discovery requests bounded noninteractive execution", async () => { const calls: Array<{ args: string[]; options: unknown }> = []; const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, async runGit(args, options) { calls.push({ args, options }); return { stdout: "", stderr: "origin is absent", exitCode: 2 }; @@ -265,6 +301,436 @@ describe("review-core", () => { expect(isBinaryPatchFile(result.patch, "large build.bin")).toBe(true); }); + test("large tracked text files render as binary in staged and working-tree diffs (#1120)", async () => { + const repoDir = initRepo(); + const runtime = makeRuntime(repoDir); + // Pure text (no NUL bytes), so WITHOUT the size guard git would emit the + // whole multi-megabyte text patch — the memory blowup #1120 reports once a + // large file is staged into (or modified in) the tracked diff. + const bigText = "a".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 100); + writeFileSync(join(repoDir, "artifact.js"), bigText, "utf-8"); + git(repoDir, ["add", "artifact.js"]); + + const staged = await runGitDiff(runtime, "staged", "main"); + expect(staged.patch).toContain("diff --git a/artifact.js b/artifact.js"); + expect(staged.patch).toContain("Binary files /dev/null and b/artifact.js differ"); + expect(isBinaryPatchFile(staged.patch, "artifact.js")).toBe(true); + // The oversized contents never entered the buffered patch. + expect(staged.patch).not.toContain("aaaaaaaaaa"); + expect(staged.patch.length).toBeLessThan(1024); + + // The same file, seen through the working-tree views (git diff HEAD / + // merge-base), is bounded the same way. + const uncommitted = await runGitDiff(runtime, "uncommitted", "main"); + expect(isBinaryPatchFile(uncommitted.patch, "artifact.js")).toBe(true); + expect(uncommitted.patch).not.toContain("aaaaaaaaaa"); + const sinceBase = await runGitDiff(runtime, "since-base", "main"); + expect(isBinaryPatchFile(sinceBase.patch, "artifact.js")).toBe(true); + expect(sinceBase.patch).not.toContain("aaaaaaaaaa"); + }); + + test("equal-sized tracked worktree edits stay bounded with textconv and change the fingerprint", async () => { + const repoDir = initRepo(); + writeFileSync(join(repoDir, ".gitattributes"), "tracked.txt diff=force-text\n", "utf-8"); + git(repoDir, ["add", ".gitattributes"]); + git(repoDir, ["commit", "-m", "configure textconv"]); + git(repoDir, ["config", "diff.force-text.textconv", "cat"]); + + const largeSize = MAX_REVIEW_FILE_CONTENT_BYTES + 1; + writeFileSync( + join(repoDir, "tracked.txt"), + "a".repeat(largeSize), + "utf-8", + ); + git(repoDir, ["add", "tracked.txt"]); + git(repoDir, ["commit", "-m", "add large tracked text"]); + + writeFileSync(join(repoDir, "tracked.txt"), "b".repeat(largeSize), "utf-8"); + const direct = await runGitDiff(makeRuntime(repoDir), "uncommitted", "main"); + expect(direct.patch.length).toBeLessThan(2_000); + expect(direct.patch).not.toContain("bbbbbbbbbb"); + expect(direct.patch).toContain("Binary files"); + + const baseRuntime = makeRuntime(repoDir); + const renderedPatches: string[] = []; + const runtime: ReviewGitRuntime = { + ...baseRuntime, + async runGit(args, options) { + const result = await baseRuntime.runGit(args, options); + const commandArgs = args[0] === "--no-optional-locks" ? args.slice(1) : args; + if (commandArgs[0] === "diff" && !commandArgs.includes("--raw")) { + renderedPatches.push(result.stdout); + } + return result; + }, + }; + const first = await getGitDiffFingerprint(runtime, "uncommitted", "main"); + expect(first).not.toBeNull(); + + writeFileSync(join(repoDir, "tracked.txt"), "c".repeat(largeSize), "utf-8"); + const second = await getGitDiffFingerprint(runtime, "uncommitted", "main"); + expect(second).not.toBeNull(); + expect(second).not.toBe(first); + expect(renderedPatches).toHaveLength(2); + for (const patch of renderedPatches) { + expect(patch.length).toBeLessThan(2_000); + expect(patch).not.toContain("bbbbbbbbbb"); + expect(patch).not.toContain("cccccccccc"); + } + }, 20_000); + + test("keeps exactly the tracked-file content limit as text and omits one byte over", async () => { + const repoDir = initRepo(); + const runtime = makeRuntime(repoDir); + const atLimitText = "x\n".repeat(MAX_REVIEW_FILE_CONTENT_BYTES / 2); + + writeFileSync(join(repoDir, "tracked.txt"), atLimitText, "utf-8"); + const atLimit = await runGitDiff(runtime, "uncommitted", "main"); + expect(atLimit.patch.length).toBeGreaterThan(MAX_REVIEW_FILE_CONTENT_BYTES); + expect(atLimit.patch).toContain("+x\n+x\n"); + + writeFileSync(join(repoDir, "tracked.txt"), `${atLimitText}y`, "utf-8"); + const overLimit = await runGitDiff(runtime, "uncommitted", "main"); + expect(overLimit.patch.length).toBeLessThan(2_000); + expect(overLimit.patch).not.toContain("yyyyyyyyyy"); + expect(isBinaryPatchFile(overLimit.patch, "tracked.txt")).toBe(true); + }, 20_000); + + test("omits oversized staged adds, deletes, edits, and renames with literal pathspecs", async () => { + const repoDir = initRepo(); + const baseRuntime = makeRuntime(repoDir); + const gitCalls: string[][] = []; + const runtime: ReviewGitRuntime = { + ...baseRuntime, + async runGit(args, options) { + gitCalls.push(args); + return baseRuntime.runGit(args, options); + }, + }; + const modified = "modify [*]?.txt"; + const deleted = "delete space [*]?.txt"; + const renamedFrom = "rename from [*]?.txt"; + const renamedTo = "rename to [*]?.txt"; + const added = "add [*]?.txt"; + + writeFileSync(join(repoDir, modified), "m".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), "utf-8"); + writeFileSync(join(repoDir, deleted), "d".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), "utf-8"); + writeFileSync(join(repoDir, renamedFrom), "r".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), "utf-8"); + git(repoDir, ["add", "."]); + git(repoDir, ["commit", "-m", "add oversized tracked files"]); + git(repoDir, ["config", "diff.renames", "false"]); + + writeFileSync(join(repoDir, modified), "n".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), "utf-8"); + git(repoDir, ["rm", deleted]); + writeFileSync(join(repoDir, added), "z".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), "utf-8"); + git(repoDir, ["add", "."]); + + const changed = await runGitDiff(runtime, "staged", "main"); + + expect(changed.patch.length).toBeLessThan(8_000); + expect(changed.patch).toContain("Binary files"); + expect(changed.patch).not.toContain("mmmmmmmmmm"); + expect(changed.patch).not.toContain("nnnnnnnnnn"); + expect(listPatchFiles(changed.patch).map((file) => file.path)).toEqual( + expect.arrayContaining([modified, deleted, added]), + ); + for (const path of [modified, deleted, added]) { + expect(gitCalls.some((args) => args.includes(`:(top,exclude,literal)${path}`))).toBe(true); + } + + git(repoDir, ["reset", "--hard", "HEAD"]); + git(repoDir, ["config", "diff.renames", "true"]); + git(repoDir, ["mv", renamedFrom, renamedTo]); + git(repoDir, ["add", "."]); + const renamed = await runGitDiff(runtime, "staged", "main"); + expect(renamed.patch.length).toBeLessThan(2_000); + expect(renamed.patch).not.toContain("Binary files"); + expect(listPatchFiles(renamed.patch).map((file) => file.path)).toContain(renamedTo); + }, 20_000); + + test("keeps gitlink pointers as normal subproject diffs and fingerprints them", async () => { + const superproject = initRepo(); + const submoduleSource = makeTempDir("plannotator-review-core-submodule-"); + git(submoduleSource, ["init"]); + git(submoduleSource, ["config", "user.email", "submodule@example.com"]); + git(submoduleSource, ["config", "user.name", "Submodule"]); + writeFileSync(join(submoduleSource, "module.txt"), "first\n", "utf-8"); + git(submoduleSource, ["add", "module.txt"]); + git(submoduleSource, ["commit", "-m", "first"]); + const first = git(submoduleSource, ["rev-parse", "HEAD"]); + + git(superproject, [ + "-c", + "protocol.file.allow=always", + "-c", + "core.hooksPath=/dev/null", + "submodule", + "add", + submoduleSource, + "deps/module", + ]); + git(superproject, ["commit", "-m", "add submodule"]); + + writeFileSync(join(submoduleSource, "module.txt"), "second\n", "utf-8"); + git(submoduleSource, ["add", "module.txt"]); + git(submoduleSource, ["commit", "-m", "second"]); + const second = git(submoduleSource, ["rev-parse", "HEAD"]); + git(superproject, [ + "-C", + "deps/module", + "-c", + "protocol.file.allow=always", + "-c", + "core.hooksPath=/dev/null", + "fetch", + "origin", + ]); + git(superproject, ["-C", "deps/module", "-c", "core.hooksPath=/dev/null", "checkout", second]); + git(superproject, ["add", "deps/module"]); + + const runtime = makeRuntime(superproject); + const staged = await runGitDiff(runtime, "staged", "main"); + expect(staged.patch).toContain(`-Subproject commit ${first}`); + expect(staged.patch).toContain(`+Subproject commit ${second}`); + expect(staged.patch).not.toContain("Binary files"); + const firstFingerprint = await getGitDiffFingerprint(runtime, "staged", "main"); + + writeFileSync(join(submoduleSource, "module.txt"), "third\n", "utf-8"); + git(submoduleSource, ["add", "module.txt"]); + git(submoduleSource, ["commit", "-m", "third"]); + const third = git(submoduleSource, ["rev-parse", "HEAD"]); + git(superproject, [ + "-C", + "deps/module", + "-c", + "protocol.file.allow=always", + "-c", + "core.hooksPath=/dev/null", + "fetch", + "origin", + ]); + git(superproject, ["-C", "deps/module", "-c", "core.hooksPath=/dev/null", "checkout", third]); + git(superproject, ["add", "deps/module"]); + + const secondFingerprint = await getGitDiffFingerprint(runtime, "staged", "main"); + expect(secondFingerprint).not.toBe(firstFingerprint); + }, 20_000); + + test("preserves small textconv output while excluding oversized textconv paths", async () => { + const repoDir = initRepo(); + const textconv = join(repoDir, "textconv.sh"); + writeFileSync( + textconv, + ["#!/bin/sh", "printf 'rendered:'", 'cat "$1"', ""].join("\n"), + "utf-8", + ); + chmodSync(textconv, 0o755); + writeFileSync(join(repoDir, ".gitattributes"), "*.txt diff=rendered\n", "utf-8"); + writeFileSync(join(repoDir, "small.txt"), "small before\n", "utf-8"); + writeFileSync( + join(repoDir, "large.txt"), + "a".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), + "utf-8", + ); + git(repoDir, ["add", ".gitattributes", "small.txt", "large.txt"]); + git(repoDir, ["commit", "-m", "configure textconv"]); + git(repoDir, ["config", "diff.rendered.textconv", textconv]); + + writeFileSync(join(repoDir, "small.txt"), "small after\n", "utf-8"); + writeFileSync( + join(repoDir, "large.txt"), + "b".repeat(MAX_REVIEW_FILE_CONTENT_BYTES + 1), + "utf-8", + ); + + const result = await runGitDiff(makeRuntime(repoDir), "uncommitted", "main"); + + expect(result.patch).toContain("+rendered:small after"); + expect(result.patch).toContain("Binary files"); + expect(result.patch).not.toContain("bbbbbbbbbb"); + expect(result.patch.length).toBeLessThan(4_000); + }, 20_000); + + test("does not mark unchanged oversized rename or mode-only stubs as binary", async () => { + const objectId = "a".repeat(40); + const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, + async runGit(args, options) { + if (args[0] === "diff" && args.includes("--raw")) { + return { + stdout: [ + `:100644 100644 ${objectId} ${objectId} R100`, + "old-large.txt", + "new-large.txt", + `:100644 100755 ${objectId} ${objectId} M`, + "mode-large.txt", + "", + ].join("\0"), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "cat-file" && args.some((arg) => arg.startsWith("--batch-check"))) { + const input = (options as { stdin?: string } | undefined)?.stdin ?? ""; + return { + stdout: input.trim().split("\n").map((id) => + `${id} blob ${MAX_REVIEW_FILE_CONTENT_BYTES + 1}`, + ).join("\n"), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "rev-parse") return { stdout: "/repo\n", stderr: "", exitCode: 0 }; + if (args[0] === "diff") return { stdout: "", stderr: "", exitCode: 0 }; + throw new Error(`Unexpected git command: ${args.join(" ")}`); + }, + async readTextFile() { + return null; + }, + }; + + const result = await runGitDiff(runtime, "staged", "main", "/repo"); + + expect(result.patch).toContain("rename from old-large.txt"); + expect(result.patch).toContain("old mode 100644\nnew mode 100755"); + expect(result.patch).not.toContain("Binary files"); + }); + + test("fingerprinting oversized tracked worktree files uses metadata without hashing them", async () => { + const repoDir = initRepo(); + const largeSize = MAX_REVIEW_FILE_CONTENT_BYTES + 1; + writeFileSync(join(repoDir, "tracked.txt"), "a".repeat(largeSize), "utf-8"); + git(repoDir, ["add", "tracked.txt"]); + git(repoDir, ["commit", "-m", "add large file"]); + + const baseRuntime = makeRuntime(repoDir); + let hashObjectCalls = 0; + const runtime: ReviewGitRuntime = { + ...baseRuntime, + async runGit(args, options) { + if (args[0] === "--no-optional-locks" && args[1] === "hash-object") { + hashObjectCalls++; + } + return baseRuntime.runGit(args, options); + }, + }; + + writeFileSync(join(repoDir, "tracked.txt"), "b".repeat(largeSize), "utf-8"); + const first = await getGitDiffFingerprint(runtime, "uncommitted", "main"); + writeFileSync(join(repoDir, "tracked.txt"), `b${"b".repeat(largeSize)}`, "utf-8"); + const second = await getGitDiffFingerprint(runtime, "uncommitted", "main"); + + expect(first).not.toBeNull(); + expect(second).not.toBe(first); + expect(hashObjectCalls).toBe(0); + }, 20_000); + + test("preflights many tracked objects with one cat-file batch query", async () => { + const objectIds = Array.from({ length: 12 }, (_, index) => index.toString(16).padStart(40, "0")); + let individualSizeCalls = 0; + let batchCalls = 0; + const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, + async runGit(args, options) { + if (args[0] === "diff" && args.includes("--raw")) { + return { + stdout: objectIds.map((objectId, index) => + `:100644 100644 ${objectId} ${objectId} M\0file-${index}.txt\0`, + ).join(""), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "cat-file" && args[1] === "-s") { + individualSizeCalls++; + return { stdout: `${MAX_REVIEW_FILE_CONTENT_BYTES + 1}\n`, stderr: "", exitCode: 0 }; + } + if (args[0] === "cat-file" && args.some((arg) => arg.startsWith("--batch-check"))) { + batchCalls++; + const input = (options as { stdin?: string } | undefined)?.stdin ?? ""; + return { + stdout: input.trim().split("\n").map((objectId) => + `${objectId} blob ${MAX_REVIEW_FILE_CONTENT_BYTES + 1}`, + ).join("\n"), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "rev-parse") return { stdout: "/repo\n", stderr: "", exitCode: 0 }; + if (args[0] === "diff") return { stdout: "", stderr: "", exitCode: 0 }; + throw new Error(`Unexpected git command: ${args.join(" ")}`); + }, + async readTextFile() { + return null; + }, + }; + + const result = await runGitDiff(runtime, "staged", "main", "/repo"); + + expect(result.patch.length).toBeLessThan(8_000); + expect(batchCalls).toBe(1); + expect(individualSizeCalls).toBe(0); + }); + + test("synthesizes quoted rename and copy metadata from raw status details", async () => { + const renamedFrom = 'old "rename" path'; + const renamedTo = "new \\ rename path"; + const copiedFrom = 'old "copy" path'; + const copiedTo = "new \\ copy path"; + const oldObjectId = "a".repeat(40); + const newObjectId = "b".repeat(40); + const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, + async runGit(args, options) { + if (args[0] === "diff" && args.includes("--raw")) { + return { + stdout: [ + `:100644 100755 ${oldObjectId} ${newObjectId} R087`, + renamedFrom, + renamedTo, + `:100644 100644 ${oldObjectId} ${newObjectId} C065`, + copiedFrom, + copiedTo, + "", + ].join("\0"), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "cat-file" && args[1] === "-s") { + return { stdout: `${MAX_REVIEW_FILE_CONTENT_BYTES + 1}\n`, stderr: "", exitCode: 0 }; + } + if (args[0] === "cat-file" && args.some((arg) => arg.startsWith("--batch-check"))) { + const input = (options as { stdin?: string } | undefined)?.stdin ?? ""; + return { + stdout: input.trim().split("\n").map((objectId) => + `${objectId} blob ${MAX_REVIEW_FILE_CONTENT_BYTES + 1}`, + ).join("\n"), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "rev-parse") return { stdout: "/repo\n", stderr: "", exitCode: 0 }; + if (args[0] === "diff") return { stdout: "", stderr: "", exitCode: 0 }; + throw new Error(`Unexpected git command: ${args.join(" ")}`); + }, + async readTextFile() { + return null; + }, + }; + + const result = await runGitDiff(runtime, "staged", "main", "/repo"); + + expect(result.patch).toContain("similarity index 87%"); + expect(result.patch).toContain(`rename from ${JSON.stringify(renamedFrom)}`); + expect(result.patch).toContain(`rename to ${JSON.stringify(renamedTo)}`); + expect(result.patch).toContain("old mode 100644\nnew mode 100755"); + expect(result.patch).toContain("similarity index 65%"); + expect(result.patch).toContain(`copy from ${JSON.stringify(copiedFrom)}`); + expect(result.patch).toContain(`copy to ${JSON.stringify(copiedTo)}`); + expect(result.patch).not.toContain("similarity index 100%"); + }); + test("binary patch detection follows rename metadata", () => { const patch = [ 'diff --git "a/old name.bin" "b/new name.bin"', @@ -310,10 +776,14 @@ describe("review-core", () => { test("ordinary working-tree diffs keep tracked changes when an untracked file cannot be read", async () => { const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, async runGit(args) { if (args[0] === "rev-parse") { return { stdout: "/repo\n", stderr: "", exitCode: 0 }; } + if (args[0] === "diff" && args.includes("--raw")) { + return { stdout: "", stderr: "", exitCode: 0 }; + } if (args[0] === "ls-files") { return { stdout: "blocked.txt\n", stderr: "", exitCode: 0 }; } @@ -340,10 +810,14 @@ describe("review-core", () => { test("ordinary working-tree diffs keep tracked changes when untracked discovery fails", async () => { const runtime: ReviewGitRuntime = { + ...unavailableFileMethods, async runGit(args) { if (args[0] === "rev-parse") { return { stdout: "/repo\n", stderr: "", exitCode: 0 }; } + if (args[0] === "diff" && args.includes("--raw")) { + return { stdout: "", stderr: "", exitCode: 0 }; + } if (args[0] === "ls-files") { return { stdout: "", stderr: "fatal: cannot read index", exitCode: 128 }; } @@ -548,6 +1022,7 @@ describe("review-core", () => { // don't break it. expect(result.error).toContain("git diff"); expect(result.error).toContain("master..HEAD"); + expect(result.error).not.toContain("core.bigFileThreshold"); }); test("git context lists worktrees and file content lookup returns old/new content", async () => { @@ -588,6 +1063,50 @@ describe("review-core", () => { expect(newFileContents.newContent).toBe("brand new\n"); }); + test("file-content expansion uses runtime filesystem capabilities", async () => { + const inspectedPaths: Array<[string, string]> = []; + const readPaths: string[] = []; + const runtime: ReviewGitRuntime = { + async runGit(args: string[]) { + if (args[0] === "rev-parse") { + return { stdout: "/virtual/repo\n", stderr: "", exitCode: 0 }; + } + if (args[0] === "cat-file") { + return { stdout: "", stderr: "missing", exitCode: 1 }; + } + throw new Error(`Unexpected git command: ${args.join(" ")}`); + }, + async readTextFile(path: string) { + readPaths.push(path); + return path === "/virtual/repo/generated.ts" ? "runtime content\n" : null; + }, + async getFileInfo(basePath: string | undefined, path: string) { + if (!basePath) return null; + inspectedPaths.push([basePath, path]); + return { + path: "/virtual/repo/generated.ts", + size: 16, + mtimeMs: 1, + isFile: true, + isSymbolicLink: false, + isExecutable: false, + }; + }, + async readLink() { + return null; + }, + }; + + await expect(getFileContentsForDiff( + runtime, + "uncommitted", + "main", + "generated.ts", + )).resolves.toEqual({ oldContent: null, newContent: "runtime content\n" }); + expect(inspectedPaths).toEqual([["/virtual/repo", "generated.ts"]]); + expect(readPaths).toEqual(["/virtual/repo/generated.ts"]); + }); + test("file content lookup refuses oversized working-tree files", async () => { const repoDir = initRepo(); const runtime = makeRuntime(repoDir); diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index c15422bee..d77cfd082 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -5,9 +5,8 @@ * 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 { + formatDiffMetadataPathToken, formatPatchPathToken, unquoteGitPath, parsePatchPathToken, @@ -145,6 +144,8 @@ export interface GitCommandResult { export interface GitCommandOptions { cwd?: string; timeoutMs?: number; + /** UTF-8 data written to stdin, then closed before waiting for output. */ + stdin?: string; /** Whether the command may ask the user for credentials. Defaults to `"allow"`. */ interaction?: "allow" | "forbid"; } @@ -159,12 +160,29 @@ export interface PreparedGitCommand { isolateProcessGroup: boolean; } +/** Filesystem metadata resolved by the host runtime, never by browser-safe core code. */ +export interface ReviewFileInfo { + path: string; + size: number; + mtimeMs: number; + isFile: boolean; + isSymbolicLink: boolean; + isExecutable: boolean; +} + export interface ReviewGitRuntime { runGit: ( args: string[], options?: GitCommandOptions, ) => Promise; readTextFile: (path: string) => Promise; + /** Resolve and stat one file relative to a repository root or other base path. */ + getFileInfo: ( + basePath: string | undefined, + path: string, + ) => Promise; + /** Read a symlink payload without following its target. */ + readLink: (path: string) => Promise; } function quoteGitSshPath(path: string): string { @@ -701,6 +719,289 @@ async function resolveRepoToplevel( return trimmed || cwd; } +interface RawDiffEntry { + oldMode: string; + newMode: string; + oldObjectId: string; + newObjectId: string; + status: string; + oldPath: string | null; + newPath: string | null; +} + +interface OversizedTrackedDiffEntry extends RawDiffEntry { + oldObjectId: string; + newObjectId: string; +} + +const NULL_OBJECT_ID = /^0+$/; + +function parseRawDiffEntries(output: string): RawDiffEntry[] { + const fields = output.split("\0"); + const entries: RawDiffEntry[] = []; + let index = 0; + + while (index < fields.length) { + const header = fields[index++]; + if (!header) continue; + if (!header.startsWith(":")) { + throw new Error("git diff --raw returned an invalid record"); + } + + const metadata = header.slice(1).split(" "); + if (metadata.length !== 5 || !metadata[4]) { + throw new Error("git diff --raw returned malformed metadata"); + } + const [oldMode, newMode, oldObjectId, newObjectId, status] = metadata; + const renamedOrCopied = status[0] === "R" || status[0] === "C"; + const oldPath = fields[index++] ?? null; + const newPath = renamedOrCopied ? fields[index++] ?? null : oldPath; + if (!oldPath || !newPath) { + throw new Error("git diff --raw returned a record without a path"); + } + + entries.push({ + oldMode, + newMode, + oldObjectId, + newObjectId, + status, + oldPath: status[0] === "A" ? null : oldPath, + newPath: status[0] === "D" ? null : newPath, + }); + } + + return entries; +} + +function isNullObjectId(objectId: string): boolean { + return NULL_OBJECT_ID.test(objectId); +} + +function isGitlink(entry: RawDiffEntry): boolean { + return entry.oldMode === "160000" || entry.newMode === "160000"; +} + +async function getGitObjectSizes( + runtime: ReviewGitRuntime, + objectIds: string[], + cwd?: string, +): Promise> { + const uniqueObjectIds = [...new Set(objectIds.filter((objectId) => !isNullObjectId(objectId)))]; + const sizes = new Map(); + if (uniqueObjectIds.length === 0) return sizes; + + const result = await runtime.runGit( + ["cat-file", "--batch-check=%(objectname) %(objecttype) %(objectsize)"], + { cwd, stdin: `${uniqueObjectIds.join("\n")}\n` }, + ); + if (result.exitCode !== 0) { + for (const objectId of uniqueObjectIds) sizes.set(objectId, Number.POSITIVE_INFINITY); + return sizes; + } + + for (const line of result.stdout.split("\n")) { + const [objectId, objectType, objectSize] = line.split(" "); + if (!objectId || objectType === "missing") continue; + const size = Number(objectSize); + sizes.set( + objectId, + Number.isFinite(size) && size >= 0 ? size : Number.POSITIVE_INFINITY, + ); + } + for (const objectId of uniqueObjectIds) { + if (!sizes.has(objectId)) sizes.set(objectId, Number.POSITIVE_INFINITY); + } + return sizes; +} + +async function getWorkingTreeFileInfo( + runtime: ReviewGitRuntime, + root: string | undefined, + path: string | null, +): Promise { + if (!path) return null; + try { + const fileInfo = await runtime.getFileInfo(root, path); + return fileInfo?.isFile || fileInfo?.isSymbolicLink ? fileInfo : null; + } catch { + return null; + } +} + +async function hashOversizedWorkingTreeFile( + runtime: ReviewGitRuntime, + path: string, + file: ReviewFileInfo, + cwd?: string, +): Promise { + const result = await runtime.runGit( + ["hash-object", "--no-filters", "--", file.path], + { cwd }, + ); + if (result.exitCode === 0 && /^[0-9a-f]{40,64}$/i.test(result.stdout.trim())) { + return result.stdout.trim(); + } + // The patch remains safely omitted even if a concurrently deleted file + // cannot be hashed. Retain deterministic stat metadata for freshness. + return hashFingerprintPart(`${path}:${file.size}:${file.mtimeMs}`); +} + +function buildOversizedTrackedStub(entry: OversizedTrackedDiffEntry): string { + const headerOldToken = formatPatchPathToken("a", entry.oldPath ?? entry.newPath!); + const headerNewToken = formatPatchPathToken("b", entry.newPath ?? entry.oldPath!); + const oldToken = entry.oldPath ? formatPatchPathToken("a", entry.oldPath) : "/dev/null"; + const newToken = entry.newPath ? formatPatchPathToken("b", entry.newPath) : "/dev/null"; + const oldId = isNullObjectId(entry.oldObjectId) ? "000000000000" : entry.oldObjectId.slice(0, 12); + const newId = isNullObjectId(entry.newObjectId) ? "000000000000" : entry.newObjectId.slice(0, 12); + const lines = [ + `diff --git ${headerOldToken} ${headerNewToken}`, + ]; + + if (!entry.oldPath) lines.push(`new file mode ${entry.newMode}`); + if (!entry.newPath) lines.push(`deleted file mode ${entry.oldMode}`); + if (entry.status[0] === "R") { + const similarity = Number(entry.status.slice(1)); + if (Number.isFinite(similarity)) lines.push(`similarity index ${similarity}%`); + lines.push(`rename from ${formatDiffMetadataPathToken(entry.oldPath!)}`); + lines.push(`rename to ${formatDiffMetadataPathToken(entry.newPath!)}`); + } else if (entry.status[0] === "C") { + const similarity = Number(entry.status.slice(1)); + if (Number.isFinite(similarity)) lines.push(`similarity index ${similarity}%`); + lines.push(`copy from ${formatDiffMetadataPathToken(entry.oldPath!)}`); + lines.push(`copy to ${formatDiffMetadataPathToken(entry.newPath!)}`); + } + if (entry.oldPath && entry.newPath && entry.oldMode !== entry.newMode) { + lines.push(`old mode ${entry.oldMode}`); + lines.push(`new mode ${entry.newMode}`); + } + lines.push( + `index ${oldId}..${newId}${entry.oldMode === entry.newMode ? ` ${entry.newMode}` : ""}`, + ); + if (entry.oldObjectId !== entry.newObjectId) { + lines.push(`Binary files ${oldToken} and ${newToken} differ`); + } + lines.push(""); + return lines.join("\n"); +} + +interface BoundedTrackedDiff { + patch: string; + fingerprintMetadata: string[]; +} + +/** + * Render a tracked diff without ever asking Git to format an oversized file. + * + * A raw, no-textconv preflight identifies changed paths and object sizes first. + * Every over-limit path is then excluded with a top-level literal pathspec and + * represented by a small, parseable binary stub. The stub's object ids retain + * a content-sensitive fingerprint without putting file bytes in patch output. + */ +async function buildBoundedTrackedDiff( + runtime: ReviewGitRuntime, + args: string[], + cwd?: string, + fingerprintMode = false, +): Promise { + const diffIndex = args.indexOf("diff"); + if (diffIndex === -1) throw new Error("Expected a git diff command"); + // Textconv is disabled only for the machine-readable preflight. The rendered + // diff retains Git's normal textconv behavior for sub-threshold paths, while + // every oversized path is excluded before that rendered invocation begins. + const rawFlags = args.includes("--no-textconv") ? [] : ["--no-textconv"]; + const rawArgs = [ + ...args.slice(0, diffIndex + 1), + ...rawFlags, + "--raw", + "-z", + "--no-abbrev", + ...args.slice(diffIndex + 1), + ]; + const rawResult = assertGitSuccess(await runtime.runGit(rawArgs, { cwd }), rawArgs); + const entries = parseRawDiffEntries(rawResult.stdout); + if (entries.length === 0) { + return { + patch: assertGitSuccess(await runtime.runGit(args, { cwd }), args).stdout, + fingerprintMetadata: [], + }; + } + + const root = await resolveRepoToplevel(runtime, cwd); + const oversized: OversizedTrackedDiffEntry[] = []; + const fingerprintMetadata: string[] = []; + const nonGitlinks = entries.filter((entry) => !isGitlink(entry)); + const objectSizes = await getGitObjectSizes( + runtime, + nonGitlinks.flatMap((entry) => [entry.oldObjectId, entry.newObjectId]), + cwd, + ); + for (const entry of entries) { + if (isGitlink(entry)) continue; + const oldSize = isNullObjectId(entry.oldObjectId) + ? null + : objectSizes.get(entry.oldObjectId) ?? Number.POSITIVE_INFINITY; + const newObjectSize = isNullObjectId(entry.newObjectId) + ? null + : objectSizes.get(entry.newObjectId) ?? Number.POSITIVE_INFINITY; + const workingTreeInfo = isNullObjectId(entry.newObjectId) + ? await getWorkingTreeFileInfo(runtime, root, entry.newPath) + : null; + const newSize = newObjectSize ?? workingTreeInfo?.size ?? null; + if (oldSize === null && newSize === null) continue; + if ((oldSize ?? 0) <= MAX_REVIEW_FILE_CONTENT_BYTES + && (newSize ?? 0) <= MAX_REVIEW_FILE_CONTENT_BYTES) { + continue; + } + // Fingerprint polling follows the large-untracked policy: path, byte + // size, and mtime avoid re-reading a large file every few seconds. As with + // untracked files, same-size edits within a filesystem timestamp tick can + // collide; one-shot patch generation still hashes exact content. + if (fingerprintMode && workingTreeInfo && entry.newPath) { + fingerprintMetadata.push( + `large:${entry.newPath}:${workingTreeInfo.size}:${workingTreeInfo.mtimeMs}`, + ); + } + const workingObjectId = !fingerprintMode && workingTreeInfo && entry.newPath + ? await hashOversizedWorkingTreeFile(runtime, entry.newPath, workingTreeInfo, cwd) + : null; + oversized.push({ + ...entry, + newObjectId: newObjectSize === null && workingObjectId + ? workingObjectId + : entry.newObjectId, + }); + } + + if (oversized.length === 0) { + return { + patch: assertGitSuccess(await runtime.runGit(args, { cwd }), args).stdout, + fingerprintMetadata, + }; + } + + const exclusions = oversized.flatMap((entry) => [ + ...(entry.oldPath ? [`:(top,exclude,literal)${entry.oldPath}`] : []), + ...(entry.newPath && entry.newPath !== entry.oldPath + ? [`:(top,exclude,literal)${entry.newPath}`] + : []), + ]); + const patchArgs = [...args, "--", ...exclusions]; + const boundedPatch = assertGitSuccess(await runtime.runGit(patchArgs, { cwd }), patchArgs).stdout; + return { + patch: boundedPatch + oversized.map(buildOversizedTrackedStub).join(""), + fingerprintMetadata, + }; +} + +export async function runBoundedTrackedDiff( + runtime: ReviewGitRuntime, + args: string[], + cwd?: string, +): Promise { + return (await buildBoundedTrackedDiff(runtime, args, cwd)).patch; +} + async function getUntrackedFileDiffs( runtime: ReviewGitRuntime, srcPrefix = "a/", @@ -766,23 +1067,24 @@ async function getUntrackedFileDiffs( // 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. + let fileInfo: ReviewFileInfo | null = null; 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"); - } + fileInfo = await runtime.getFileInfo(rootCwd, file); } catch { // Preserve the existing best-effort/strict behavior below: Git reports // the authoritative read error for files that disappear mid-snapshot. } + if (fileInfo?.isFile && fileInfo.size > MAX_REVIEW_FILE_CONTENT_BYTES) { + const mode = fileInfo.isExecutable ? "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"); + } const diffResult = await runtime.runGit( [ @@ -842,7 +1144,7 @@ export async function getWorkingTreeDiffFromBase( "--end-of-options", base, ]; - const trackedPatch = assertGitSuccess(await runtime.runGit(args, { cwd }), args).stdout; + const trackedPatch = await runBoundedTrackedDiff(runtime, args, cwd); const untracked = await getUntrackedFileDiffs( runtime, "a/", @@ -890,8 +1192,7 @@ function assertGitSuccess( } // LOCKSTEP: packages/review-editor/App.tsx's activeWorktreePath memo -// hand-parses worktree: diffTypes with a COPY of this list (this module -// can't enter the browser bundle — node:path import above). Adding a +// hand-parses worktree: diffTypes with a COPY of this list. Adding a // subtype here without updating that copy makes the client derive a // different worktreePath than the server stamped on guide/tour jobs, // silently breaking their context matching. Real fix (cleanup PR): @@ -1023,7 +1324,7 @@ export async function runGitDiff( "--end-of-options", `${baseRef}..${sha}`, ]; - patch = assertGitSuccess(await runtime.runGit(commitDiffArgs, { cwd }), commitDiffArgs).stdout; + patch = await runBoundedTrackedDiff(runtime, commitDiffArgs, cwd); label = subject ? `Commit ${shortSha} — ${subject}` : `Commit ${shortSha}`; } else if (effectiveDiffType.startsWith("commit:")) { return { patch: "", label: `Error: ${diffType}`, error: "Invalid commit ref" }; @@ -1066,18 +1367,15 @@ export async function runGitDiff( "diff", "--no-ext-diff", ...wFlag, - "HEAD", "--src-prefix=a/", "--dst-prefix=b/", + "HEAD", ]; const hasHead = (await runtime.runGit(["rev-parse", "--verify", "HEAD"], { cwd })) .exitCode === 0; const trackedPatch = hasHead - ? assertGitSuccess( - await runtime.runGit(trackedDiffArgs, { cwd }), - trackedDiffArgs, - ).stdout + ? await runBoundedTrackedDiff(runtime, trackedDiffArgs, cwd) : ""; const untracked = await getUntrackedFileDiffs(runtime, "a/", "b/", cwd, options); patch = removeTrackedDeletions(trackedPatch, new Set(untracked.paths)) + untracked.diff; @@ -1094,11 +1392,7 @@ export async function runGitDiff( "--src-prefix=a/", "--dst-prefix=b/", ]; - const stagedDiff = assertGitSuccess( - await runtime.runGit(stagedDiffArgs, { cwd }), - stagedDiffArgs, - ); - patch = stagedDiff.stdout; + patch = await runBoundedTrackedDiff(runtime, stagedDiffArgs, cwd); label = "Staged changes"; break; } @@ -1111,12 +1405,11 @@ export async function runGitDiff( "--src-prefix=a/", "--dst-prefix=b/", ]; - const trackedDiff = assertGitSuccess( - await runtime.runGit(trackedDiffArgs, { cwd }), - trackedDiffArgs, - ); const untracked = await getUntrackedFileDiffs(runtime, "a/", "b/", cwd, options); - patch = removeTrackedDeletions(trackedDiff.stdout, new Set(untracked.paths)) + untracked.diff; + patch = removeTrackedDeletions( + await runBoundedTrackedDiff(runtime, trackedDiffArgs, cwd), + new Set(untracked.paths), + ) + untracked.diff; label = "Unstaged changes"; break; } @@ -1128,13 +1421,9 @@ export async function runGitDiff( ); const args = hasParent.exitCode === 0 - ? ["diff", "--no-ext-diff", ...wFlag, "HEAD~1..HEAD", "--src-prefix=a/", "--dst-prefix=b/"] - : ["diff", "--no-ext-diff", ...wFlag, "--root", "HEAD", "--src-prefix=a/", "--dst-prefix=b/"]; - const lastCommitDiff = assertGitSuccess( - await runtime.runGit(args, { cwd }), - args, - ); - patch = lastCommitDiff.stdout; + ? ["diff", "--no-ext-diff", ...wFlag, "--src-prefix=a/", "--dst-prefix=b/", "HEAD~1..HEAD"] + : ["diff", "--no-ext-diff", ...wFlag, "--src-prefix=a/", "--dst-prefix=b/", "--root", "HEAD"]; + patch = await runBoundedTrackedDiff(runtime, args, cwd); label = "Last commit"; break; } @@ -1153,11 +1442,7 @@ export async function runGitDiff( "--end-of-options", `${defaultBranch}..HEAD`, ]; - const branchDiff = assertGitSuccess( - await runtime.runGit(branchDiffArgs, { cwd }), - branchDiffArgs, - ); - patch = branchDiff.stdout; + patch = await runBoundedTrackedDiff(runtime, branchDiffArgs, cwd); label = `Changes vs ${displayRef(defaultBranch)}`; break; } @@ -1178,11 +1463,7 @@ export async function runGitDiff( "--end-of-options", `${mergeBase}..HEAD`, ]; - const mergeBaseDiff = assertGitSuccess( - await runtime.runGit(mergeBaseDiffArgs, { cwd }), - mergeBaseDiffArgs, - ); - patch = mergeBaseDiff.stdout; + patch = await runBoundedTrackedDiff(runtime, mergeBaseDiffArgs, cwd); label = `PR diff vs ${displayRef(defaultBranch)}`; break; } @@ -1199,11 +1480,7 @@ export async function runGitDiff( "--end-of-options", `${emptyTree}..HEAD`, ]; - const allDiff = assertGitSuccess( - await runtime.runGit(allDiffArgs, { cwd }), - allDiffArgs, - ); - patch = allDiff.stdout; + patch = await runBoundedTrackedDiff(runtime, allDiffArgs, cwd); label = "All files"; break; } @@ -1287,18 +1564,35 @@ const MAX_UNTRACKED_FINGERPRINT_FILES = 20; const UNTRACKED_STATUS_OUTPUT_CAP = 2 * 1024 * 1024; const collapsedUntrackedCwds = new Set(); -type ReadOnlyGitRunner = (args: string[]) => Promise; +type ReadOnlyGitRunner = ( + args: string[], + options?: GitCommandOptions, +) => Promise; async function appendDiffFingerprint( runReadOnlyGit: ReadOnlyGitRunner, + runtime: ReviewGitRuntime, parts: string[], whitespaceArgs: string[], args: string[], ): Promise { - const result = await runReadOnlyGit(["diff", "--no-ext-diff", ...whitespaceArgs, ...args]); - if (result.exitCode !== 0) return false; - parts.push(hashFingerprintPart(result.stdout)); - return true; + try { + const diff = await buildBoundedTrackedDiff( + { + runGit: (diffArgs, options) => runReadOnlyGit(diffArgs, options), + readTextFile: async () => null, + getFileInfo: runtime.getFileInfo, + readLink: runtime.readLink, + }, + ["diff", "--no-ext-diff", ...whitespaceArgs, ...args], + undefined, + true, + ); + parts.push(hashFingerprintPart(diff.patch), ...diff.fingerprintMetadata); + return true; + } catch { + return false; + } } async function appendUntrackedFingerprint( @@ -1332,32 +1626,35 @@ async function appendUntrackedFingerprint( if (untracked.length > 0) { const baseDir = await resolveRepoToplevel(runtime, cwd); for (const path of untracked) { - const fullPath = baseDir ? resolvePath(baseDir, path) : path; try { - const fileStat = await lstat(fullPath); - if (fileStat.isSymbolicLink()) { + const fileInfo = await runtime.getFileInfo(baseDir, path); + if (!fileInfo) { + parts.push("unreadable"); + continue; + } + if (fileInfo.isSymbolicLink) { // Hash the link payload Git records without following it into a // potentially huge target file. - parts.push(hashFingerprintPart(`symlink:${await readlink(fullPath)}`)); + const link = await runtime.readLink(fileInfo.path); + parts.push(link != null ? hashFingerprintPart(`symlink:${link}`) : "unreadable"); continue; } - if (!fileStat.isFile()) { - parts.push(`non-file:${fileStat.size}:${fileStat.mtimeMs}`); + if (!fileInfo.isFile) { + parts.push(`non-file:${fileInfo.size}:${fileInfo.mtimeMs}`); continue; } - if (fileStat.size > MAX_UNTRACKED_FINGERPRINT_CONTENT_BYTES) { + if (fileInfo.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}`); + parts.push(`large:${fileInfo.size}:${fileInfo.mtimeMs}`); continue; } + const content = await runtime.readTextFile(fileInfo.path); + parts.push(content != null ? hashFingerprintPart(content) : "unreadable"); } catch { parts.push("unreadable"); - continue; } - const content = await runtime.readTextFile(fullPath); - parts.push(content != null ? hashFingerprintPart(content) : "unreadable"); } } return true; @@ -1386,8 +1683,8 @@ export async function getGitDiffFingerprint( // every few seconds) and must NEVER take git's index lock — `status`/`diff` // opportunistically refresh the index by default, which races concurrent // `git add`/commit operations (the agent working while the user reviews). - const runReadOnlyGit = (args: string[]) => - runtime.runGit(["--no-optional-locks", ...args], { cwd }); + const runReadOnlyGit = (args: string[], options?: GitCommandOptions) => + runtime.runGit(["--no-optional-locks", ...args], { ...options, cwd }); // commit: — the diff is anchored to an immutable object, so the // fingerprint is the sha plus whether it still resolves. Deliberately NOT @@ -1409,7 +1706,7 @@ export async function getGitDiffFingerprint( const parts = ["git", effectiveDiffType, headSha]; const hashDiffOutput = (args: string[]): Promise => - appendDiffFingerprint(runReadOnlyGit, parts, wFlag, args); + appendDiffFingerprint(runReadOnlyGit, runtime, parts, wFlag, args); // Untracked files: porcelain `??` lines capture existence; hash their // contents too so editing a freshly-created (untracked) file is detected. @@ -1508,18 +1805,18 @@ export async function getFileContentsForDiff( // path and hunk expansion silently returns null. (The `git show ref:path` // 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); + const fileInfo = await runtime.getFileInfo(baseDir, path); + if (!fileInfo) return null; // 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; + if (fileInfo.isSymbolicLink) return await runtime.readLink(fileInfo.path); + if (!fileInfo.isFile || fileInfo.size > MAX_REVIEW_FILE_CONTENT_BYTES) return null; + return runtime.readTextFile(fileInfo.path); } catch { return null; } - return runtime.readTextFile(fullPath); } // commit: — old side is the first parent (null on a root commit, which diff --git a/packages/shared/vcs-core.test.ts b/packages/shared/vcs-core.test.ts index fdee9281d..558be03b9 100644 --- a/packages/shared/vcs-core.test.ts +++ b/packages/shared/vcs-core.test.ts @@ -59,6 +59,12 @@ function provider( } const gitRuntime: ReviewGitRuntime = { + async getFileInfo() { + return null; + }, + async readLink() { + return null; + }, async runGit() { return { stdout: "", stderr: "", exitCode: 0 }; }, diff --git a/packages/shared/worktree-pool.test.ts b/packages/shared/worktree-pool.test.ts index 01856caf4..4535b14c3 100644 --- a/packages/shared/worktree-pool.test.ts +++ b/packages/shared/worktree-pool.test.ts @@ -6,6 +6,8 @@ import { createWorktreePool } from "./worktree-pool"; function fakeRuntime(): { runtime: ReviewGitRuntime; commands: string[][] } { const commands: string[][] = []; const runtime: ReviewGitRuntime = { + async getFileInfo() { return null; }, + async readLink() { return null; }, async runGit(args) { commands.push(args); return { stdout: "", stderr: "", exitCode: 0 }; @@ -293,6 +295,8 @@ describe("worktree-pool seeded warmup", () => { // if creations ran concurrently instead of through the serialization chain. const commands: string[][] = []; const runtime: ReviewGitRuntime = { + async getFileInfo() { return null; }, + async readLink() { return null; }, async runGit(args) { await Bun.sleep(1); commands.push(args);