diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b73a45c8d..939de2f63 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -55,6 +55,7 @@ jobs: DOM_TESTS=1 bun test packages/review-editor/components/DiffViewer.fullContentSwap.test.tsx packages/review-editor/components/DiffViewer.oversizedStub.test.tsx + packages/review-editor/components/DiffViewer.binaryNotice.test.tsx # Seam contracts + the remaining DOM-gated tests. Scoped to the DOM files # (not the whole ui suite) to keep this process light. diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index ffbedb603..8e0932672 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -30,10 +30,11 @@ import { buildCodeNavRequest } from '../utils/buildCodeNavRequest'; import { getDiffSelection, getLineNumberFromNode, getSideFromNode } from '../utils/diffSelection'; import { isContentConsistentWithPatch } from '../utils/patchConsistency'; import { hashString } from '../utils/hashString'; -import { isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; +import { isContentlessBinaryPatch, isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; import { OversizedFileNotice } from './OversizedFileNotice'; import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost'; import { FileHeader } from './FileHeader'; +import { BinaryFileNotice } from './BinaryFileNotice'; import { EditSessionHud } from './EditSessionHud'; import { FileCommentBanner } from './FileCommentBanner'; import { annotationMatchesPrScope, isFileScopedAnnotation, lineRangeForAnnotation } from '../utils/annotationScope'; @@ -2181,6 +2182,14 @@ export const AllFilesCodeView: React.FC = ({ {!collapsed && isOversizedReviewStubPatch(file.patch) && ( refreshItem(item.id)} /> )} + {/* The general fallback under that specific case: any OTHER hunkless + binary chunk draws nothing either. Gated on the marker so a + marker-carrying stub is explained exactly once, by the line above. */} + {!collapsed + && !isOversizedReviewStubPatch(file.patch) + && isContentlessBinaryPatch(file.patch) && ( + refreshItem(item.id)} /> + )} {/* EXPERIMENTAL edit-session HUD: session controls + state in a slim strip below the header, above the file content. Appears/disappears with session start/end, which both go through a version-bumped diff --git a/packages/review-editor/components/BinaryFileNotice.tsx b/packages/review-editor/components/BinaryFileNotice.tsx new file mode 100644 index 000000000..f92112ed5 --- /dev/null +++ b/packages/review-editor/components/BinaryFileNotice.tsx @@ -0,0 +1,31 @@ +import React, { useLayoutEffect } from 'react'; + +/** + * Says why a file's card has no diff in it. + * + * A patch chunk with a binary marker and no hunks renders as an empty body: + * the card is a bare header with no counts and no reason, which reads as a + * broken diff. That shape covers genuine binary files and files the review + * core declined to read, so the copy commits to neither cause. + */ +export const BinaryFileNotice: React.FC<{ + /** Re-measure hook for the virtualized all-files host, whose custom-header + * slot heights are not auto-observed. */ + onHeightChange?: () => void; +}> = ({ onHeightChange }) => { + // Before paint, so the host re-measures without a one-frame overlap with the + // content below. + useLayoutEffect(() => { + onHeightChange?.(); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + return ( +
+ Binary or oversized file, content not shown. +
+ ); +}; diff --git a/packages/review-editor/components/DiffViewer.binaryNotice.test.tsx b/packages/review-editor/components/DiffViewer.binaryNotice.test.tsx new file mode 100644 index 000000000..d39f8148b --- /dev/null +++ b/packages/review-editor/components/DiffViewer.binaryNotice.test.tsx @@ -0,0 +1,161 @@ +/** + * A file whose card has no diff in it must SAY so. + * + * A patch chunk carrying a binary marker and no hunks renders as an empty + * body, so the card is a bare header with no counts and no reason. That is + * what a file dropped by the review size probe looked like (#1167): reviewers + * saw an empty card and could approve without ever seeing the content. + * + * DOM-gated (DOM_TESTS=1) and registered in .github/workflows/test.yml's + * "Run UI seam-contract + DOM tests" step. + */ +import { afterEach, describe, expect, mock, test } from 'bun:test'; +import React from 'react'; +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { OVERSIZED_REVIEW_STUB_MARKER } from '@plannotator/shared/diff-paths'; + +mock.module('../workerPool', () => ({ + useIsWorkerPoolReadyOrDisabled: () => true, + useWorkerPoolThemeSync: () => {}, +})); + +mock.module('../hooks/usePierreTheme', () => ({ + usePierreTheme: () => ({ type: 'light', css: '' }), +})); + +mock.module('./ToolbarHost', () => ({ + ToolbarHost: React.forwardRef(function MockToolbarHost() { + return null; + }), +})); + +const { DiffViewer } = await import('./DiffViewer'); + +const hasDom = typeof document !== 'undefined'; + +// The shape the review core emits for a file it declined to read: rename +// metadata, no hunks. Before the fix a renamed-and-edited file could land here +// purely because its size probe could not find the worktree blob. +const STUB_PATCH = [ + 'diff --git a/src/Card.tsx b/src/Panel.tsx', + 'similarity index 94%', + 'rename from src/Card.tsx', + 'rename to src/Panel.tsx', + 'index bab081fdb737..99fffbd3cac3 100644', + 'Binary files a/src/Card.tsx and b/src/Panel.tsx differ', + '', +].join('\n'); + +const REAL_BINARY = [ + 'diff --git a/assets/logo.png b/assets/logo.png', + 'index 1111111111aa..2222222222bb 100644', + 'Binary files a/assets/logo.png and b/assets/logo.png differ', + '', +].join('\n'); + +const TEXT_PATCH = [ + 'diff --git a/calc.ts b/calc.ts', + 'index 0000000..1111111 100644', + '--- a/calc.ts', + '+++ b/calc.ts', + '@@ -1,3 +1,3 @@', + ' const a = 1;', + '-const b = 1;', + '+const b = 2;', + ' const c = 3;', + '', +].join('\n'); + +// The same shape PLUS the size-cap marker. The specific notice owns this one, +// so exactly one explanation must appear on the card. +const MARKED_OVERSIZED_STUB = [ + 'diff --git a/assets/blob.pack b/assets/blob.pack', + OVERSIZED_REVIEW_STUB_MARKER, + 'index 1111111111aa..2222222222bb 100644', + 'Binary files a/assets/blob.pack and b/assets/blob.pack differ', + '', +].join('\n'); + +const NOTICE_SELECTOR = '[data-binary-file-notice]'; +const OVERSIZED_NOTICE_SELECTOR = '[data-oversized-file-notice]'; + +function view(patch: string, filePath: string) { + return ( + {}} + onAddAnnotation={() => {}} + onAddFileComment={() => {}} + onEditAnnotation={() => {}} + onSelectAnnotation={() => {}} + onDeleteAnnotation={() => {}} + /> + ); +} + +describe.if(hasDom)('contentless binary card presentation (DOM)', () => { + let root: Root | null = null; + let host: HTMLDivElement | null = null; + const originalFetch = globalThis.fetch; + + async function render(patch: string, filePath: string) { + // There is no expandable content for these shapes; keep the lookup inert. + globalThis.fetch = (async () => + new Response(JSON.stringify({ oldContent: null, newContent: null }), { + headers: { 'content-type': 'application/json' }, + })) as typeof fetch; + + host = document.createElement('div'); + document.body.appendChild(host); + root = createRoot(host); + await act(async () => { + root!.render(view(patch, filePath)); + await new Promise((resolve) => setTimeout(resolve, 25)); + }); + return host; + } + + afterEach(async () => { + if (root) { + await act(async () => root!.unmount()); + root = null; + } + host?.remove(); + host = null; + globalThis.fetch = originalFetch; + }); + + test('a hunkless stub explains its empty body', async () => { + const el = await render(STUB_PATCH, 'src/Panel.tsx'); + const notice = el.querySelector(NOTICE_SELECTOR); + expect(notice).not.toBeNull(); + expect(notice!.textContent).toContain('content not shown'); + }); + + test('a genuine binary file explains its empty body too', async () => { + const el = await render(REAL_BINARY, 'assets/logo.png'); + expect(el.querySelector(NOTICE_SELECTOR)).not.toBeNull(); + }); + + test('an ordinary text diff is left alone', async () => { + const el = await render(TEXT_PATCH, 'calc.ts'); + expect(el.querySelector(NOTICE_SELECTOR)).toBeNull(); + expect(el.querySelector(OVERSIZED_NOTICE_SELECTOR)).toBeNull(); + }); + + test('a marker-carrying stub is explained once, by the specific notice', async () => { + // Specific beats general: the size-cap notice knows WHY the body is empty, + // so the fallback must stand down rather than stack a second line on it. + const el = await render(MARKED_OVERSIZED_STUB, 'assets/blob.pack'); + expect(el.querySelectorAll(OVERSIZED_NOTICE_SELECTOR).length).toBe(1); + expect(el.querySelectorAll(NOTICE_SELECTOR).length).toBe(0); + expect(el.textContent).not.toContain(OVERSIZED_REVIEW_STUB_MARKER); + }); +}); diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index 0c9f31c24..9c5e251ab 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -13,9 +13,10 @@ import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost'; import { OverlayScrollArea } from '@plannotator/ui/components/OverlayScrollArea'; import { useOverlayViewport } from '@plannotator/ui/hooks/useOverlayViewport'; import { FileHeader } from './FileHeader'; +import { BinaryFileNotice } from './BinaryFileNotice'; import { FileCommentBanner } from './FileCommentBanner'; import { OversizedFileNotice } from './OversizedFileNotice'; -import { isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; +import { isContentlessBinaryPatch, isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; import { isFileScopedAnnotation, lineRangeForAnnotation } from '../utils/annotationScope'; import { lineAnnotationMetadata } from '../utils/annotationDisplay'; import type { AnnotationScrollTarget } from '../types'; @@ -686,6 +687,15 @@ export const DiffViewer: React.FC = ({ // renders as an empty body. Say so instead of showing a bare header. const isOversizedStub = useMemo(() => isOversizedReviewStubPatch(patch), [patch]); + // The general fallback under that specific case: any OTHER hunkless binary + // chunk (a genuine binary file, or a stub shape the marker does not cover) + // still renders an empty body and still has to say why. Gated on the marker + // so a marker-carrying stub is explained exactly once, by the message above. + const isContentlessBinary = useMemo( + () => !isOversizedStub && isContentlessBinaryPatch(patch), + [patch, isOversizedStub], + ); + // Replay a selected line/range comment's anchor as the controlled highlight so // clicking it (inline card or sidebar) lights up its lines. A live compose // selection (pendingSelection) wins while the toolbar is open; file-scoped @@ -731,7 +741,10 @@ export const DiffViewer: React.FC = ({ overflowX="scroll" onViewportReady={onViewportReady} > + {/* Specific first, general second, and never both: whichever applies, + a card with no hunks to draw says why instead of reading as empty. */} {isOversizedStub && } + {isContentlessBinary && } { + test("flags a git binary chunk with no hunks", () => { + expect(isContentlessBinaryPatch([ + "diff --git a/logo.png b/logo.png", + "index 1111111111aa..2222222222bb 100644", + "Binary files a/logo.png and b/logo.png differ", + "", + ].join("\n"))).toBe(true); + }); + + test("flags a review stub for a file the server declined to read", () => { + expect(isContentlessBinaryPatch([ + "diff --git a/src/Panel.tsx b/src/Panel.tsx", + "similarity index 94%", + "rename from src/Card.tsx", + "rename to src/Panel.tsx", + "index bab081fdb737..99fffbd3cac3 100644", + "Binary files a/src/Card.tsx and b/src/Panel.tsx differ", + "", + ].join("\n"))).toBe(true); + }); + + test("flags a literal GIT binary patch payload", () => { + expect(isContentlessBinaryPatch([ + "diff --git a/logo.png b/logo.png", + "GIT binary patch", + "literal 12", + "", + ].join("\n"))).toBe(true); + }); + + test("does not flag a text patch", () => { + expect(isContentlessBinaryPatch([ + "diff --git a/calc.ts b/calc.ts", + "--- a/calc.ts", + "+++ b/calc.ts", + "@@ -1,2 +1,2 @@", + "-const b = 1;", + "+const b = 2;", + "", + ].join("\n"))).toBe(false); + }); + + test("does not flag a text patch whose content mentions the binary marker", () => { + // Content lines always carry a +/-/space prefix, and the scan stops at the + // first hunk header, so quoted marker text cannot be mistaken for a header. + expect(isContentlessBinaryPatch([ + "diff --git a/notes.md b/notes.md", + "--- a/notes.md", + "+++ b/notes.md", + "@@ -1,2 +1,2 @@", + "-old note", + "+Binary files a/x and b/x differ", + " GIT binary patch", + "", + ].join("\n"))).toBe(false); + }); + + test("does not flag a metadata-only chunk with no binary marker", () => { + // A pure mode change has no body either, but git says nothing about + // content there, so it keeps its existing rendering. + expect(isContentlessBinaryPatch([ + "diff --git a/run.sh b/run.sh", + "old mode 100644", + "new mode 100755", + "", + ].join("\n"))).toBe(false); + }); +}); describe("diff path parsing", () => { test("unquoteGitPath decodes octal (UTF-8 byte) escapes", () => { diff --git a/packages/shared/diff-paths.ts b/packages/shared/diff-paths.ts index b704229f5..b1455d4c8 100644 --- a/packages/shared/diff-paths.ts +++ b/packages/shared/diff-paths.ts @@ -210,6 +210,35 @@ export function isOversizedReviewStubPatch(patch: string): boolean { return patch.split("\n").some((line) => line === OVERSIZED_REVIEW_STUB_MARKER); } +/** + * True when a single file's patch chunk carries a binary marker and no hunks, + * so a diff renderer has literally nothing to draw for it. + * + * The GENERAL case, of which `isOversizedReviewStubPatch` above is the one + * specific case we can name: git emits this shape for real binary files, and + * the review core emits it for files it declined to read. Either way the card + * renders as a bare header with no counts and no body, which reads as a broken + * or empty diff rather than as content that was deliberately not shown. + * + * Callers that can say something more specific should ask the marker predicate + * FIRST and fall back to this one, so a marker-carrying stub is explained once, + * by the message that knows why. + * + * Scanning stops at the first hunk header: content lines always carry a `+`, + * `-`, or space prefix, so a `Binary files ` line at column zero before any + * `@@ ` can only be the extended header git (or the stub builder) wrote. + */ +export function isContentlessBinaryPatch(patch: string): boolean { + let hasBinaryMarker = false; + for (const line of patch.split("\n")) { + if (line.startsWith("@@ ")) return false; + if (line.startsWith("Binary files ") || line === "GIT binary patch") { + hasBinaryMarker = true; + } + } + return hasBinaryMarker; +} + export function parseDiffMetadataPathLines(lines: string[]): DiffPathPair { let oldPath: string | undefined; let newPath: string | undefined; diff --git a/packages/shared/review-core.test.ts b/packages/shared/review-core.test.ts index bac20cbd0..f4245c375 100644 --- a/packages/shared/review-core.test.ts +++ b/packages/shared/review-core.test.ts @@ -845,11 +845,22 @@ describe("review-core", () => { } }); - test("a single object the probe reports missing still excludes only that path", async () => { + test("a probed-oversized object excludes only its path, and a missing one none", async () => { + // Per-object evidence stays per-object: one oversized blob costs one path, + // never the whole review. An object the probe cannot SIZE is a different + // answer from one it sizes above the cap: `missing` is unknown, so that + // path keeps rendering under git's own core.bigFileThreshold bound. const smallOld = "1".repeat(40); const smallNew = "2".repeat(40); - const brokenOld = "3".repeat(40); - const brokenNew = "4".repeat(40); + const missingOld = "3".repeat(40); + const missingNew = "4".repeat(40); + const hugeOld = "5".repeat(40); + const hugeNew = "6".repeat(40); + const renderedPatch = [ + "diff --git a/small.ts b/small.ts\n-old\n+new\n", + "diff --git a/unfetched.ts b/unfetched.ts\n-gone\n+restored\n", + ].join(""); + const excludedPaths: string[] = []; const runtime: ReviewGitRuntime = { ...unavailableFileMethods, async runGit(args, options) { @@ -857,7 +868,8 @@ describe("review-core", () => { return { stdout: [ `:100644 100644 ${smallOld} ${smallNew} M\0small.ts\0`, - `:100644 100644 ${brokenOld} ${brokenNew} M\0broken.bin\0`, + `:100644 100644 ${missingOld} ${missingNew} M\0unfetched.ts\0`, + `:100644 100644 ${hugeOld} ${hugeNew} M\0huge.bin\0`, ].join(""), stderr: "", exitCode: 0, @@ -866,16 +878,23 @@ describe("review-core", () => { 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").filter(Boolean).map((objectId) => - objectId === brokenNew ? `${objectId} missing` : `${objectId} blob 10`, - ).join("\n"), + stdout: input.trim().split("\n").filter(Boolean).map((objectId) => { + if (objectId === missingNew) return `${objectId} missing`; + if (objectId === hugeNew) { + return `${objectId} blob ${MAX_REVIEW_FILE_CONTENT_BYTES + 1}`; + } + return `${objectId} blob 10`; + }).join("\n"), stderr: "", exitCode: 0, }; } if (args[0] === "rev-parse") return { stdout: "/repo\n", stderr: "", exitCode: 0 }; if (args[0] === "diff") { - return { stdout: "diff --git a/small.ts b/small.ts\n-old\n+new\n", stderr: "", exitCode: 0 }; + excludedPaths.push( + ...args.filter((arg) => arg.startsWith(":(top,exclude,literal)")), + ); + return { stdout: renderedPatch, stderr: "", exitCode: 0 }; } throw new Error(`Unexpected git command: ${args.join(" ")}`); }, @@ -887,7 +906,12 @@ describe("review-core", () => { const result = await runGitDiff(runtime, "staged", "main", "/repo"); expect(result.patch).toContain("+new"); - expect(result.patch).toContain("Binary files a/broken.bin and b/broken.bin differ"); + expect(result.patch).toContain("Binary files a/huge.bin and b/huge.bin differ"); + expect(excludedPaths).toEqual([":(top,exclude,literal)huge.bin"]); + // The unfetchable object's path is not stubbed away: git renders it, and + // a git that truly cannot read it fails loudly instead of blanking it. + expect(result.patch).toContain("+restored"); + expect(result.patch).not.toContain("Binary files a/unfetched.ts"); expect(result.patch).not.toContain("Binary files a/small.ts"); }); @@ -966,6 +990,90 @@ describe("review-core", () => { expect(second).not.toBe(first); }, 20_000); + test("renders a renamed file whose edited worktree blob the probe cannot find", async () => { + // Rename/copy detection makes git hash the WORKING-TREE content and print + // that hash in --raw output, but the blob is never written to the object + // database. The size probe answers `missing` for it. Treating that as + // "oversized" dropped the whole renamed file out of the review (#1167). + const repoDir = initRepo(); + mkdirSync(join(repoDir, "src")); + const original = Array.from({ length: 40 }, (_, index) => `line ${index + 1}`).join("\n"); + writeFileSync(join(repoDir, "src/Card.tsx"), `${original}\n`, "utf-8"); + git(repoDir, ["add", "-A"]); + git(repoDir, ["commit", "-m", "add card"]); + const base = git(repoDir, ["rev-parse", "HEAD"]); + git(repoDir, ["mv", "src/Card.tsx", "src/Panel.tsx"]); + git(repoDir, ["commit", "-m", "rename card to panel"]); + writeFileSync( + join(repoDir, "src/Panel.tsx"), + `${original}\nline 41 unstaged\n`, + "utf-8", + ); + + const runtime = makeConfigForwardingRuntime(repoDir); + const patch = await getWorkingTreeDiffFromBase(runtime, base); + + expect(patch).toContain("+line 41 unstaged"); + expect(patch).not.toContain("Binary files"); + }, 20_000); + + test("renders a tracked add whose worktree blob the probe cannot find", async () => { + // A delete/add pair also feeds rename detection, so the added path carries + // a real-but-unwritten worktree hash in --raw output. + const repoDir = initRepo(); + const base = git(repoDir, ["rev-parse", "HEAD"]); + writeFileSync(join(repoDir, "replacement.txt"), "fresh content\n", "utf-8"); + git(repoDir, ["rm", "-q", "tracked.txt"]); + git(repoDir, ["add", "-A"]); + git(repoDir, ["commit", "-m", "replace tracked file"]); + writeFileSync(join(repoDir, "replacement.txt"), "fresh content\nedited later\n", "utf-8"); + + const runtime = makeConfigForwardingRuntime(repoDir); + const patch = await getWorkingTreeDiffFromBase(runtime, base); + + expect(patch).toContain("+fresh content"); + expect(patch).toContain("+edited later"); + expect(patch).not.toContain("Binary files"); + }, 20_000); + + test("renders a file whose index blob is missing from the object database", async () => { + // Partial clones (and pruned object databases) can report `missing` for an + // index blob git can still diff perfectly well from the working tree. + const repoDir = initRepo(); + writeFileSync(join(repoDir, "added.txt"), "content from the index\n", "utf-8"); + git(repoDir, ["add", "added.txt"]); + const blobId = git(repoDir, ["rev-parse", ":added.txt"]); + rmSync(join(repoDir, ".git", "objects", blobId.slice(0, 2), blobId.slice(2)), { force: true }); + + const runtime = makeConfigForwardingRuntime(repoDir); + const result = await runGitDiff(runtime, "uncommitted", "main"); + + expect(result.patch).toContain("+content from the index"); + expect(result.patch).not.toContain("Binary files"); + }, 20_000); + + test("still stubs an oversized worktree file whose blob the probe cannot find", async () => { + // The size probe cannot bound this file (its hash is missing) and + // core.bigFileThreshold does not bound working-tree sides, so the + // filesystem stat has to be the authoritative bound. + const repoDir = initRepo(); + const largeSize = MAX_REVIEW_FILE_CONTENT_BYTES + 1; + writeFileSync(join(repoDir, "old-big.txt"), "a".repeat(largeSize), "utf-8"); + git(repoDir, ["add", "-A"]); + git(repoDir, ["commit", "-m", "add big file"]); + const base = git(repoDir, ["rev-parse", "HEAD"]); + git(repoDir, ["mv", "old-big.txt", "new-big.txt"]); + git(repoDir, ["commit", "-m", "rename big file"]); + writeFileSync(join(repoDir, "new-big.txt"), "b".repeat(largeSize), "utf-8"); + + const runtime = makeConfigForwardingRuntime(repoDir); + const patch = await getWorkingTreeDiffFromBase(runtime, base); + + expect(patch).toContain("Binary files"); + expect(patch).not.toContain("bbbbbbbbbb"); + expect(patch.length).toBeLessThan(4_000); + }, 40_000); + test("synthesizes quoted rename and copy metadata from raw status details", async () => { const renamedFrom = 'old "rename" path'; const renamedTo = "new \\ rename path"; diff --git a/packages/shared/review-core.ts b/packages/shared/review-core.ts index b1e726619..19584fd3e 100644 --- a/packages/shared/review-core.ts +++ b/packages/shared/review-core.ts @@ -832,19 +832,25 @@ function isGitlink(entry: RawDiffEntry): boolean { * oversized replaced the whole review with binary stubs on one failed probe. * Working-tree sizes come from filesystem stat and never from this probe. * - * The per-object doors stay conservative on a probe that ran: an object the - * batch reports as `missing`, omits from its output, or answers with an - * unparseable/negative size maps to infinity. That is evidence about one - * object only, and excluding just its path keeps the stub's rename/copy/mode - * metadata rendering while the rest of the diff stays intact. + * A probe that ran answers per object with a size or with `null`, which means + * "unknown", never "oversized": an object the batch reports as `missing`, + * omits from its output, or answers with an unparseable/negative size has no + * usable size. `missing` is routine for tree-vs-worktree diffs — every path + * pulled into rename/copy detection gets its WORKING-TREE content hashed by + * git and that hash printed in `--raw` output without ever being written to + * the object database — and it also happens in partial clones. Mapping those + * to infinity excluded files git can plainly diff, so a renamed-and-edited + * file rendered as an empty binary stub (#1167). Callers bound an unknown new + * side by filesystem stat, and every ODB blob stays bounded by + * `core.bigFileThreshold` (`BOUNDED_DIFF_GIT_CONFIG`). */ async function getGitObjectSizes( runtime: ReviewGitRuntime, objectIds: string[], cwd?: string, -): Promise | null> { +): Promise | null> { const uniqueObjectIds = [...new Set(objectIds.filter((objectId) => !isNullObjectId(objectId)))]; - const sizes = new Map(); + const sizes = new Map(); if (uniqueObjectIds.length === 0) return sizes; const result = await runtime.runGit( @@ -862,13 +868,10 @@ async function getGitObjectSizes( 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, - ); + sizes.set(objectId, Number.isFinite(size) && size >= 0 ? size : null); } for (const objectId of uniqueObjectIds) { - if (!sizes.has(objectId)) sizes.set(objectId, Number.POSITIVE_INFINITY); + if (!sizes.has(objectId)) sizes.set(objectId, null); } return sizes; } @@ -1029,19 +1032,28 @@ async function buildBoundedTrackedDiff( // the file for the index line and content-based binary detection wins), // but their sizes come from filesystem stat, not the probe, so that // exclusion door keeps working below regardless of the probe outcome. + // - An id the probe answered for but could not size (`missing`, omitted, + // unparseable) is unknown, not oversized. git prints such an id for every + // worktree path that rename/copy detection hashed, and for blobs a + // partial clone has not fetched; a file git can plainly diff must never + // be replaced by a content-free binary stub (#1167). + const probedSize = (objectId: string): number | null => + isNullObjectId(objectId) || objectSizes === null + ? null + : objectSizes.get(objectId) ?? null; for (const entry of entries) { if (isGitlink(entry)) continue; - const oldSize = isNullObjectId(entry.oldObjectId) - ? null - : objectSizes === null - ? null - : objectSizes.get(entry.oldObjectId) ?? Number.POSITIVE_INFINITY; - const newObjectSize = isNullObjectId(entry.newObjectId) - ? null - : objectSizes === null - ? null - : objectSizes.get(entry.newObjectId) ?? Number.POSITIVE_INFINITY; - const workingTreeInfo = isNullObjectId(entry.newObjectId) + const oldSize = probedSize(entry.oldObjectId); + const newObjectSize = probedSize(entry.newObjectId); + // The working-tree file is what git formats whenever the new side has no + // readable object behind it: an unhashed worktree side (all-zero id), or + // an id the probe that RAN could not find. Either way its stat size is the + // authoritative bound. A probe that FAILED outright says nothing about any + // single object, so those ids keep relying on core.bigFileThreshold rather + // than on a working-tree file that may not be the diff's new side at all. + const newSideUnreadable = isNullObjectId(entry.newObjectId) + || (objectSizes !== null && newObjectSize === null); + const workingTreeInfo = newSideUnreadable ? await getWorkingTreeFileInfo(runtime, root, entry.newPath) : null; const newSize = newObjectSize ?? workingTreeInfo?.size ?? null; @@ -1059,14 +1071,18 @@ async function buildBoundedTrackedDiff( `large:${entry.newPath}:${workingTreeInfo.size}:${workingTreeInfo.mtimeMs}`, ); } - const workingObjectId = !fingerprintMode && workingTreeInfo && entry.newPath + // Only an all-zero new side needs a synthesized content-sensitive id. When + // git printed a real id it already hashed this exact working-tree content, + // so the stub keeps it instead of re-hashing an oversized file. + const workingObjectId = !fingerprintMode + && isNullObjectId(entry.newObjectId) + && workingTreeInfo + && entry.newPath ? await hashOversizedWorkingTreeFile(runtime, entry.newPath, workingTreeInfo, cwd) : null; oversized.push({ ...entry, - newObjectId: newObjectSize === null && workingObjectId - ? workingObjectId - : entry.newObjectId, + newObjectId: workingObjectId ?? entry.newObjectId, }); }