diff --git a/.github/scripts/pr-review/prepare.mjs b/.github/scripts/pr-review/prepare.mjs index 69f9378..c283f1d 100644 --- a/.github/scripts/pr-review/prepare.mjs +++ b/.github/scripts/pr-review/prepare.mjs @@ -86,7 +86,9 @@ const diffArgs = (from, to, tripleDot = false) => [ ]; const mergeBase = String(git(repo, ["merge-base", baseTipSha, headSha])).trim(); -const fullDiff = Buffer.from(git(repo, diffArgs(mergeBase, headSha), { +// Include binary payloads in admission and identity without putting them in +// the text-oriented per-file patches sent to the reviewer. +const fullDiff = Buffer.from(git(repo, [...diffArgs(mergeBase, headSha), "--binary"], { encoding: null, maxBuffer: diffReadBufferBytes, })); diff --git a/.github/scripts/pr-review/test.mjs b/.github/scripts/pr-review/test.mjs index 0f21544..a69337c 100644 --- a/.github/scripts/pr-review/test.mjs +++ b/.github/scripts/pr-review/test.mjs @@ -4,6 +4,7 @@ import assert from "node:assert/strict"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { randomBytes } from "node:crypto"; import { spawnSync } from "node:child_process"; import { createRequire } from "node:module"; import { @@ -503,6 +504,52 @@ try { fs.writeFileSync(path.join(temporary, "nested", "b"), "three"); assert.notEqual(treeHash(temporary), first); + const binaryRepo = path.join(temporary, "binary-repo"); + fs.mkdirSync(binaryRepo); + const runBinaryGit = (...args) => { + const result = spawnSync("git", args, { cwd: binaryRepo, encoding: "utf8" }); + assert.equal(result.status, 0, result.stderr); + return result.stdout.trim(); + }; + runBinaryGit("init", "-q"); + runBinaryGit("config", "user.name", "Review Test"); + runBinaryGit("config", "user.email", "review@example.com"); + runBinaryGit("commit", "--allow-empty", "-qm", "base"); + const binaryBase = runBinaryGit("rev-parse", "HEAD"); + fs.writeFileSync(path.join(binaryRepo, "model.glb"), + Buffer.concat([Buffer.from([0]), randomBytes(8192)])); + runBinaryGit("add", "model.glb"); + runBinaryGit("commit", "-qm", "binary model"); + const binaryHead = runBinaryGit("rev-parse", "HEAD"); + const plainPatch = spawnSync("git", ["diff", `${binaryBase}..${binaryHead}`], { + cwd: binaryRepo, encoding: null, + }); + assert.equal(plainPatch.status, 0); + assert.ok(plainPatch.stdout.length < 1000); + const prepareBinary = (approved) => spawnSync(process.execPath, [ + path.join(path.dirname(new URL(import.meta.url).pathname), "prepare.mjs"), + ], { + cwd: binaryRepo, + encoding: "utf8", + env: { + ...process.env, + REPOSITORY_DIR: binaryRepo, + PR_REVIEW_STATE_DIR: path.join(temporary, "binary-state"), + PR_BASE_SHA: binaryBase, + PR_HEAD_SHA: binaryHead, + SESSION_KEY: "repo:1:pr:binary:v2", + MAX_DIFF_BYTES: "20000", + AUTOMATIC_MAX_DIFF_BYTES: "4000", + LARGE_DIFF_APPROVED: String(approved), + CHUNK_TARGET_BYTES: "600", + READINESS_CONTEXT_SHA256: "binary-context", + }, + }); + const blockedBinary = prepareBinary(false); + assert.notEqual(blockedBinary.status, 0); + assert.match(blockedBinary.stderr, /automatic reviews are limited to 4000 bytes/); + assert.equal(prepareBinary(true).status, 0); + const repo = path.join(temporary, "repo"); fs.mkdirSync(repo); const run = (...args) => { diff --git a/README.md b/README.md index 26ecc3a..20212d9 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,9 @@ and `actions: write` so the reviewer can restore the latest per-PR Codex session artifact and delete superseded snapshots only after a replacement upload succeeds. -Complete diffs up to 5,000,000 bytes are reviewed automatically. For a larger +Complete binary-aware Git patches up to 5,000,000 bytes are reviewed +automatically. Binary models, images, and other changed binary payloads count +toward this limit even though their contents are not sent to the model. For a larger diff, an admin of the **calling repository** must post this line as a PR comment with the current full head commit SHA: