fix(review): a PR too big for GitHub to diff is reviewed anyway

SPEC.md

@@ -935,7 +935,20 @@ what keeps a wedged agent from holding a claim forever.
    happen has no runner to blame — and only from here on do writes belong to
    the runner.
 2. Fetch the diff; resolve trust (§14.2); prepare the checkout (§10),
-   catching failure into diff-only mode; evict old checkouts.
+   catching failure into diff-only mode; evict old checkouts. GitHub refuses
+   to render a diff past 300 changed files (HTTP 406); such a diff MUST be
+   re-assembled from the `pulls/N/files` API — the per-file hunks with their
+   `diff --git`/`---`/`+++` headers restored, so it parses identically to
+   `gh pr diff` output. A file whose patch GitHub withholds (binary, or too
+   large) keeps its headers and carries a line saying so, so the gap cannot
+   read as "nothing changed here", and a change past that API's own 3000-file
+   cap is likewise flagged — as the doubt it is, since hitting the cap exactly
+   does not prove anything was lost. A pure rename carries
+   `rename from`/`rename to` and no hunks, as git emits it — it MUST NOT be
+   reported as binary merely for changing no lines. GitHub's `too_large`
+   refusal is the only diff failure that falls back; every other one fails the
+   run. The match is on that signature rather than on the 406 status, which
+   GitHub also returns for unrelated reasons.
 3. Persist the `running` artifact **before** the agent starts: fresh empty
    draft, a full `run` block (`startedAt`, `withSource`, `trusted`,
    `trigger`, `reviewedSha: null`), and the previous conversation carried

src/core/gh.test.ts

@@ -1,14 +1,17 @@
 import { execFile } from "node:child_process";
 import { Mock, beforeEach, describe, expect, it, vi } from "vitest";
 import {
+  assembleDiff,
   classifyReply,
   currentLogin,
   lastMentionOfYou,
+  fetchPrDiff,
   lastRequestOf,
   latestOwnReview,
   mentionsYou,
   resetLoginCache,
 } from "./gh.js";
+import { newSideLineText, splitDiffByFile } from "./diff.js";
 
 // gh.ts calls `promisify(execFile)`, which honours this symbol — so the mock
 // resolves to the `{ stdout }` shape the real one does, while still recording
@@ -352,3 +355,142 @@ describe("ghErrorDetail", () => {
     expect(ghErrorDetail(undefined, undefined)).toBe("");
   });
 });
+
+describe("assembleDiff", () => {
+  const file = (over: Partial<Parameters<typeof assembleDiff>[0][number]>) => ({
+    filename: "src/a.ts",
+    status: "modified",
+    previous_filename: null,
+    additions: 1,
+    deletions: 1,
+    patch: "@@ -1,2 +1,2 @@\n-old\n+new",
+    ...over,
+  });
+
+  it("puts the headers back so the result parses like `gh pr diff` output", () => {
+    const diff = assembleDiff([file({})]);
+    expect(diff).toBe(
+      "diff --git a/src/a.ts b/src/a.ts\n" +
+        "--- a/src/a.ts\n" +
+        "+++ b/src/a.ts\n" +
+        "@@ -1,2 +1,2 @@\n-old\n+new\n",
+    );
+    expect(splitDiffByFile(diff).map((p) => p.path)).toEqual(["src/a.ts"]);
+    expect(newSideLineText(diff).get("src/a.ts")?.get(1)).toBe("new");
+  });
+
+  it("uses /dev/null on the side an added or removed file does not have", () => {
+    const diff = assembleDiff([
+      file({ filename: "new.ts", status: "added", patch: "@@ -0,0 +1 @@\n+hello" }),
+      file({ filename: "gone.ts", status: "removed", patch: "@@ -1 +0,0 @@\n-bye" }),
+    ]);
+    expect(diff).toContain("--- /dev/null\n+++ b/new.ts");
+    expect(diff).toContain("--- a/gone.ts\n+++ /dev/null");
+    // A deletion is attributed to the path it removed, as `gh pr diff` is.
+    expect(splitDiffByFile(diff).map((p) => p.path)).toEqual(["new.ts", "gone.ts"]);
+  });
+
+  it("names both sides of a rename", () => {
+    const diff = assembleDiff([
+      file({ filename: "b.ts", status: "renamed", previous_filename: "a.ts" }),
+    ]);
+    expect(diff).toContain("diff --git a/a.ts b/b.ts");
+    expect(diff).toContain("rename from a.ts\nrename to b.ts");
+    expect(splitDiffByFile(diff).map((p) => p.path)).toEqual(["b.ts"]);
+  });
+
+  it("leaves a pure rename at its rename lines instead of calling it binary", () => {
+    // A file that only moved changes no lines and carries no patch — the same
+    // shape a binary file arrives in. Counting lines alone would report the
+    // move as a binary change, which is wrong and the more alarming way to be
+    // wrong. git emits the rename lines and stops; so does this.
+    const diff = assembleDiff([
+      file({
+        filename: "b.ts",
+        status: "renamed",
+        previous_filename: "a.ts",
+        additions: 0,
+        deletions: 0,
+        patch: null,
+      }),
+    ]);
+    expect(diff).toBe("diff --git a/a.ts b/b.ts\nrename from a.ts\nrename to b.ts\n");
+    expect(diff).not.toContain("Binary");
+  });
+
+  it("still reports a renamed file's patch when it moved and changed", () => {
+    const diff = assembleDiff([
+      file({ filename: "b.ts", status: "renamed", previous_filename: "a.ts", patch: "@@ -1 +1 @@\n-old\n+new" }),
+    ]);
+    expect(diff).toContain("--- a/a.ts\n+++ b/b.ts");
+    expect(newSideLineText(diff).get("b.ts")?.get(1)).toBe("new");
+  });
+
+  it("says so when GitHub withheld a patch, rather than showing an empty file", () => {
+    const diff = assembleDiff([
+      file({ filename: "logo.png", status: "added", additions: 0, deletions: 0, patch: null }),
+      file({ filename: "huge.md", additions: 523, deletions: 48, patch: null }),
+    ]);
+    // git names the side an added file does not have /dev/null, here too.
+    expect(diff).toContain("Binary files /dev/null and b/logo.png differ");
+    expect(diff).toContain("GitHub withheld this file's patch — 523 addition(s), 48 deletion(s)");
+    // Neither note may be mistaken for a changed line.
+    expect(newSideLineText(diff).get("huge.md")?.size).toBe(0);
+  });
+});
+
+describe("fetchPrDiff", () => {
+  beforeEach(() => vi.clearAllMocks());
+
+  const tooLarge = Object.assign(new Error("failed"), {
+    stderr:
+      "could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300).\nPullRequest.diff too_large",
+  });
+
+  it("re-assembles the diff from the files API when GitHub refuses to render it", async () => {
+    exec.mockRejectedValueOnce(tooLarge).mockResolvedValueOnce({
+      stdout:
+        JSON.stringify({
+          filename: "src/a.ts",
+          status: "modified",
+          previous_filename: null,
+          additions: 1,
+          deletions: 0,
+          patch: "@@ -1 +1,2 @@\n line\n+added",
+        }) + "\n",
+    });
+    const diff = await fetchPrDiff({ owner: "o", repo: "r", number: 1 });
+    expect(diff).toContain("diff --git a/src/a.ts b/src/a.ts");
+    expect(diff).toContain("+added");
+    expect(exec.mock.calls[1]![1]).toContain("repos/o/r/pulls/1/files?per_page=100");
+  });
+
+  it("flags the files API's own 3000-file cap as doubt, not as a proven loss", async () => {
+    const rows = Array.from(
+      { length: 3000 },
+      (_, i) =>
+        JSON.stringify({
+          filename: `f${i}.ts`,
+          status: "added",
+          previous_filename: null,
+          additions: 1,
+          deletions: 0,
+          patch: "@@ -0,0 +1 @@\n+x",
+        }) + "\n",
+    ).join("");
+    exec.mockRejectedValueOnce(tooLarge).mockResolvedValueOnce({ stdout: rows });
+    const diff = await fetchPrDiff({ owner: "o", repo: "r", number: 1 });
+    // Exactly 3000 files is a PR that may or may not have been truncated, and
+    // nothing here can tell the two apart — so the note must not assert one.
+    expect(diff).toContain("returns at most 3000 files and returned exactly that many");
+    expect(diff).toContain("if this PR changes more");
+  });
+
+  it("does not paper over any other gh failure", async () => {
+    exec.mockRejectedValue(Object.assign(new Error("failed"), { stderr: "gh: not authenticated" }));
+    await expect(fetchPrDiff({ owner: "o", repo: "r", number: 1 })).rejects.toThrow(
+      "not authenticated",
+    );
+    expect(exec).toHaveBeenCalledTimes(1);
+  });
+});

src/core/gh.ts

@@ -104,8 +104,102 @@ async function membershipCheck(args: string[]): Promise<boolean> {
   }
 }
 
+/**
+ * GitHub refuses to render a diff past 300 changed files: `gh pr diff` comes
+ * back 406 and the review dies before it starts. The files API answers for ten
+ * times as many, so a too-large diff is re-assembled from it rather than
+ * reported as a broken review — see {@link assembleDiff}.
+ */
 export async function fetchPrDiff(ref: PrRef): Promise<string> {
-  return gh(["pr", "diff", String(ref.number), "--repo", `${ref.owner}/${ref.repo}`]);
+  try {
+    return await gh(["pr", "diff", String(ref.number), "--repo", `${ref.owner}/${ref.repo}`]);
+  } catch (err: unknown) {
+    // Matched on GitHub's own `too_large` signature rather than the bare 406:
+    // 406 is "Not Acceptable" generally, while `too_large` is emitted for this
+    // refusal and nothing else. Matching the meaning survives a status change;
+    // matching the status would fall back on some unrelated 406 one day.
+    const message = err instanceof Error ? err.message : String(err);
+    if (!/too_large|exceeded the maximum number of files/i.test(message)) throw err;
+    const files = await fetchPrFiles(ref);
+    const diff = assembleDiff(files);
+    // The files API stops at 3000 too, and says nothing when it does. Hitting
+    // the cap exactly is not proof of truncation — a PR can change exactly
+    // 3000 files — and there is nothing here to tell the two apart, so the
+    // note reports the doubt rather than asserting a loss that may not exist.
+    return files.length < FILES_API_CAP
+      ? diff
+      : `${diff}[GitHub's files API returns at most ${FILES_API_CAP} files and returned exactly that many — if this PR changes more, the rest is missing from this diff. Read the checkout.]\n`;
+  }
+}
+
+const FILES_API_CAP = 3000;
+
+/** One changed file as the pulls/N/files API reports it. */
+export interface PrFile {
+  filename: string;
+  status: string;
+  previous_filename?: string | null;
+  additions: number;
+  deletions: number;
+  /** Absent for binary files, and for text files GitHub decided are too big. */
+  patch?: string | null;
+}
+
+async function fetchPrFiles(ref: PrRef): Promise<PrFile[]> {
+  const out = await gh([
+    "api",
+    "--paginate",
+    `repos/${ref.owner}/${ref.repo}/pulls/${ref.number}/files?per_page=100`,
+    "--jq",
+    ".[] | {filename, status, previous_filename, additions, deletions, patch}",
+  ]);
+  // --jq streams one object per line rather than a JSON array.
+  return out
+    .split("\n")
+    .filter((line) => line.trim() !== "")
+    .map((line) => JSON.parse(line) as PrFile);
+}
+
+/**
+ * Put the `diff --git` / `---` / `+++` headers back on the per-file hunks the
+ * files API returns, so the result parses exactly like `gh pr diff` output —
+ * `splitDiffByFile` and the line anchoring read both the same way.
+ *
+ * A file whose patch GitHub withheld still gets its headers and a line saying
+ * so: an unexplained gap would read as "nothing changed here".
+ */
+export function assembleDiff(files: PrFile[]): string {
+  const out: string[] = [];
+  for (const file of files) {
+    const renamed = file.status === "renamed" && file.previous_filename;
+    const oldPath = file.status === "added" ? null : renamed ? file.previous_filename! : file.filename;
+    const newPath = file.status === "removed" ? null : file.filename;
+    // The side a file does not have is /dev/null, in the headers and in the
+    // binary line alike — that is how git names it.
+    const a = oldPath ? `a/${oldPath}` : "/dev/null";
+    const b = newPath ? `b/${newPath}` : "/dev/null";
+    out.push(`diff --git a/${oldPath ?? file.filename} b/${newPath ?? file.filename}`);
+    if (renamed) out.push(`rename from ${oldPath}`, `rename to ${newPath}`);
+
+    if (file.patch) {
+      out.push(`--- ${a}`, `+++ ${b}`, file.patch.replace(/\n$/, ""));
+    } else if (file.additions > 0 || file.deletions > 0) {
+      // GitHub counted the lines and then withheld the patch: a text file it
+      // decided was too big. Say so — an empty file block reads as "unchanged".
+      out.push(
+        `--- ${a}`,
+        `+++ ${b}`,
+        `[GitHub withheld this file's patch — ${file.additions} addition(s), ${file.deletions} deletion(s). Read the file in the checkout.]`,
+      );
+    } else if (!renamed) {
+      out.push(`Binary files ${a} and ${b} differ`);
+    }
+    // A pure rename ends here, with no hunks and no ---/+++ pair: nothing about
+    // the content changed, and `rename from`/`rename to` have already said
+    // everything. Calling it binary — which counting lines alone would — is
+    // both wrong and the more alarming of the two ways to be wrong.
+  }
+  return out.length === 0 ? "" : out.join("\n") + "\n";
 }
 
 export interface DiscoveredPr extends PrRef {