fix(review): stop stubbing files whose worktree content the size probe cannot find

.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.

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<AllFilesCodeViewProps> = ({
         {!collapsed && isOversizedReviewStubPatch(file.patch) && (
           <OversizedFileNotice onHeightChange={() => 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) && (
+          <BinaryFileNotice onHeightChange={() => 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

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 (
+    <div
+      data-binary-file-notice=""
+      className="px-4 py-2 text-xs leading-relaxed text-muted-foreground border-b border-border bg-muted/30"
+    >
+      Binary or oversized file, content not shown.
+    </div>
+  );
+};

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 (
+    <DiffViewer
+      patch={patch}
+      filePath={filePath}
+      diffStyle="unified"
+      annotations={[]}
+      selectedAnnotationId={null}
+      scrollTargetAnnotation={null}
+      pendingSelection={null}
+      onLineSelection={() => {}}
+      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);
+  });
+});

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<DiffViewerProps> = ({
   // 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<DiffViewerProps> = ({
         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 && <OversizedFileNotice />}
+        {isContentlessBinary && <BinaryFileNotice />}
         <FileCommentBanner
           comments={fileComments}
           selectedAnnotationId={selectedAnnotationId}

packages/shared/diff-paths.test.ts

@@ -1,5 +1,80 @@
 import { describe, expect, test } from "bun:test";
-import { parseDiffFilePathLines, parsePatchPathToken, unquoteGitPath } from "./diff-paths";
+import {
+  isContentlessBinaryPatch,
+  parseDiffFilePathLines,
+  parsePatchPathToken,
+  unquoteGitPath,
+} from "./diff-paths";
+
+describe("isContentlessBinaryPatch", () => {
+  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", () => {

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;