diff --git a/SPEC.md b/SPEC.md index 8ba6997..946d79a 100644 --- a/SPEC.md +++ b/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 diff --git a/src/core/gh.test.ts b/src/core/gh.test.ts index 53478ab..a330d0f 100644 --- a/src/core/gh.test.ts +++ b/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[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); + }); +}); diff --git a/src/core/gh.ts b/src/core/gh.ts index 3f50a36..c1c1f05 100644 --- a/src/core/gh.ts +++ b/src/core/gh.ts @@ -104,8 +104,102 @@ async function membershipCheck(args: string[]): Promise { } } +/** + * 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 { - 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 { + 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 {