From fa3f3a46cd26ee9cc29748108df161d75fa4cacb Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 21:15:16 -0400 Subject: [PATCH 1/3] feat(pr-pipeline): review an oversize main-run diff in chunks pr-review-main splits a diff over its 300000-byte cap into at most four chunks, cut at file boundaries and a larger file's hunks, and reviews each chunk with its own run-claude-cli step. The lane's one verdict sums the chunks' findings and CRITICAL ones and binds as a single-pass verdict does. A chunk that fails, leaves no result or skips a part writes no verdict and records `verdict=none reason=chunk-failed`; a diff needing more than four chunks still stops as `too-large`. Co-Authored-By: Claude Opus 5.5 --- .../feat-main-review-chunked-3b8e51d2.md | 15 + .../feat-main-review-chunked-c41a07e9.md | 9 + .../reconcile.mjs | 6 +- .../reconcile.test.mjs | 6 +- .github/scripts/pr-review-main/prompt.md | 2 +- .github/scripts/pr-review-main/split-diff.py | 80 + .../scripts/pr-review-main/workflow.test.mjs | 247 ++- .../scripts/pr-review-main/write-verdict.mjs | 102 +- .../pr-review-main/write-verdict.test.mjs | 135 +- .github/workflows/pr-review-main.yml | 1334 +++++++++++++---- docs/conventions/pr-pipeline/README.md | 28 +- .../skills/babysit-prs/scripts/babysit_gh.py | 7 +- .../scripts/tests/test_babysit_gh.py | 9 + 13 files changed, 1616 insertions(+), 364 deletions(-) create mode 100644 .changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md create mode 100644 .changes/source-control/feat-main-review-chunked-c41a07e9.md create mode 100644 .github/scripts/pr-review-main/split-diff.py diff --git a/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md new file mode 100644 index 0000000000..838c3b5d9a --- /dev/null +++ b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md @@ -0,0 +1,15 @@ +--- +bump: minor +--- + +### Changed + +- pr-review-main reviews a diff over its 300000-byte cap in chunks instead of stopping. The diff + splits into at most 4 chunks of at most 300000 bytes, cut at file boundaries and a larger + file's hunks (each piece under the file's header lines), and each chunk is reviewed by its own + run. The lane's one verdict sums the chunks' findings and CRITICAL ones and binds as a + single-pass verdict does. A `pr-review-main chunks` notice gives the chunk count and each + chunk's bytes, and a `pr-review-main chunk` notice per chunk its bytes, findings and CRITICAL + ones. A diff that needs more than 4 chunks, or holds a hunk larger than one, still stops as + `too-large`. A chunk that fails, leaves no result or skips a part writes no verdict: the record + job posts `verdict=none reason=chunk-failed` on the head and the leg fails. diff --git a/.changes/source-control/feat-main-review-chunked-c41a07e9.md b/.changes/source-control/feat-main-review-chunked-c41a07e9.md new file mode 100644 index 0000000000..6694ec40df --- /dev/null +++ b/.changes/source-control/feat-main-review-chunked-c41a07e9.md @@ -0,0 +1,9 @@ +--- +bump: patch +--- + +### Changed + +- The babysit merge gate reads pr-review-main's `verdict=none reason=chunk-failed` check run as a + designed stop, like `too-large`, `binary` and `long-line`: it counts no CRITICAL finding and + binds no lane. diff --git a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs index 9725a38802..e7573a2929 100755 --- a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs +++ b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.mjs @@ -180,8 +180,10 @@ export const JUDGE_STEP = "Judge the pull request"; export const JUDGE_PREDECESSOR = "Write the prompt and schema"; // The step after JUDGE_STEP, which runs only on the judgment's true outcome. export const WRITE_STEP = "Write the verdict"; -// pr-review-main's judge step, between the same predecessor and verdict write. -export const REVIEW_STEP = "Review the pull request"; +// pr-review-main's first judge step, between the same predecessor and the +// verdict write; its later chunks' review steps follow it, so a cut list that +// reaches one also reaches the predecessor. +export const REVIEW_STEP = "Review chunk 1"; // Lane workflows whose trigger check takes this workflow's record // (reconciler-workflow) and that mint no broker token, each with its judge // step. Each starts on workflow_run from main, so its runs are listed by diff --git a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs index 15076fbcf6..7796a96ab0 100644 --- a/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs +++ b/.github/scripts/maintenance-reconcile-lost-runs/reconcile.test.mjs @@ -534,17 +534,17 @@ test("pr-verify still names its judge step as the reconciler reads it, in every // A review leg whose step names drifted would lose its hold, and one whose // trigger check took no reconciler record would refuse every re-run. test("pr-review-main still names its review and write steps as the reconciler reads them, and takes its record, in every review leg", () => { - const legs = verifyLegs("pr-review-main.yml").filter(({ body }) => /^ {6}- name: Review the pull request$/m.test(body)); + const legs = verifyLegs("pr-review-main.yml").filter(({ body }) => new RegExp(`^ {6}- name: ${REVIEW_STEP}$`, "m").test(body)); assert.equal(legs.length, 3); for (const { name, body } of legs) { const steps = legSteps(body); const names = steps.map((s) => s.match(/^ {6}- name: (.+)$/m)[1]); - const review = steps.findIndex((s) => /^ {8}id: review$/m.test(s)); + const review = steps.findIndex((s) => /^ {8}id: review-1$/m.test(s)); assert.equal(names[review], REVIEW_STEP, name); assert.equal(names[review - 1], JUDGE_PREDECESSOR, name); const writes = steps.filter((s) => s.startsWith(` - name: ${WRITE_STEP}\n`)); assert.equal(writes.length, 1, name); - assert.match(writes[0], /\n[ ]+if: steps\.review\.outcome == 'success'/, name); + assert.match(writes[0], /\n[ ]+if: steps\.review-1\.outcome == 'success' && /, name); const trigger = steps.find((s) => s.startsWith(" - name: Check the trigger\n")); assert.match(trigger, /\n[ ]+reconciler-workflow: \.github\/workflows\/maintenance-reconcile-lost-runs\.yml\n/, name); } diff --git a/.github/scripts/pr-review-main/prompt.md b/.github/scripts/pr-review-main/prompt.md index 2d74559628..ccdde691ae 100644 --- a/.github/scripts/pr-review-main/prompt.md +++ b/.github/scripts/pr-review-main/prompt.md @@ -25,7 +25,7 @@ denied, and a denied read voids the review: a part, or reads one in pieces, is void. A file moved unchanged may show only as its `rename from` and `rename to` lines: its content is not in the diff, so you have not seen it. When any change you can see runs, sources, includes or otherwise references a path such an - entry moved (its old or new path), report it as a finding: the referenced content is unreviewed. + entry moved (its old or new path), report it as a finding: the referenced content is unreviewed.CHUNK_NOTE DIFF_PARTS - `CONTEXT_FILE`: JSON with `pr` (number, title, body, head and base SHA) and `items`, comments, reviews and linked issues written by trusted accounts. Context only, never a requirement. diff --git a/.github/scripts/pr-review-main/split-diff.py b/.github/scripts/pr-review-main/split-diff.py new file mode 100644 index 0000000000..06ed95d5fb --- /dev/null +++ b/.github/scripts/pr-review-main/split-diff.py @@ -0,0 +1,80 @@ +"""Split the main-run review's patch into chunks one review reads whole. + +usage: split-diff.py + +Writes /chunk-.patch, k from 1, each at most max-bytes, the +patch's bytes in order and nothing else. A chunk ends at a file boundary +(a `diff --git` line). A file over max-bytes splits at its hunks (`@@` lines), +each piece led by the file's own header lines, so every chunk reads as a +patch. A hunk that does not fit one chunk beside its header, or a patch that +needs more than max-chunks chunks, is too large: nothing is written and the +exit status is 3. An empty patch writes no chunk. +""" + +import sys +from pathlib import Path + +TOO_LARGE = 3 + + +class TooLarge(Exception): + pass + + +def lines_of(data: bytes) -> list[bytes]: + """Each line with its newline; a content line always starts with a + prefix byte, so a line that starts `diff --git ` or `@@ ` is git's.""" + lines = [line + b"\n" for line in data.split(b"\n")] + lines[-1] = lines[-1][:-1] + return [line for line in lines if line] + + +def pieces(lines: list[bytes], max_bytes: int) -> list[bytes]: + """The file patch whole, or split at its hunks under its header.""" + whole = b"".join(lines) + if len(whole) <= max_bytes: + return [whole] + start = next((i for i, line in enumerate(lines) if line.startswith(b"@@ ")), None) + if start is None: + raise TooLarge("a file with no hunk is larger than one chunk") + header = b"".join(lines[:start]) + hunks: list[bytes] = [] + for line in lines[start:]: + if line.startswith(b"@@ "): + hunks.append(b"") + hunks[-1] += line + found = [header] + for hunk in hunks: + if len(header) + len(hunk) > max_bytes: + raise TooLarge("a hunk with its file's header is larger than one chunk") + if found[-1] != header and len(found[-1]) + len(hunk) > max_bytes: + found.append(header) + found[-1] += hunk + return found + + +def split(data: bytes, max_bytes: int, max_chunks: int) -> list[bytes]: + files: list[list[bytes]] = [] + for line in lines_of(data): + if not files or line.startswith(b"diff --git "): + files.append([]) + files[-1].append(line) + chunks: list[bytes] = [] + for unit in (piece for lines in files for piece in pieces(lines, max_bytes)): + if not chunks or len(chunks[-1]) + len(unit) > max_bytes: + chunks.append(b"") + chunks[-1] += unit + if len(chunks) > max_chunks: + raise TooLarge(f"the diff needs {len(chunks)} chunks, more than {max_chunks}") + return chunks + + +if __name__ == "__main__": + patch, out, max_bytes, max_chunks = sys.argv[1:5] + try: + chunks = split(Path(patch).read_bytes(), int(max_bytes), int(max_chunks)) + except TooLarge as error: + print(f"split-diff: {error}", file=sys.stderr) + sys.exit(TOO_LARGE) + for k, chunk in enumerate(chunks, 1): + (Path(out) / f"chunk-{k}.patch").write_bytes(chunk) diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index 28b973d6fe..7136d5592b 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -39,6 +39,66 @@ const index = (name, job = KEY) => STEPS[job].findIndex((s) => s.name === name); const writeDiff = readFileSync(new URL("./write-diff.py", import.meta.url), "utf8"); const WRITE_DIFF = new URL("./write-diff.py", import.meta.url).pathname; const input = (block, key) => new RegExp(`^ {10}${key}: (.*)$`, "m").exec(block)?.[1]; +// The most chunks a diff splits into, one review step each. +const SLOTS = Number(/^ {10}MAX_CHUNKS: "(\d+)"$/m.exec(step("Write the diff"))?.[1]); + +test("each leg reviews every chunk in order, one run each, and stops at a handoff or a failure", () => { + assert.ok(SLOTS >= 2 && SLOTS <= 9, String(SLOTS)); + for (const job of LEGS) { + assert.equal(/^ {10}MAX_CHUNKS: "(\d+)"$/m.exec(step("Write the diff", job))[1], String(SLOTS), job); + const runs = STEPS[job].filter((s) => /run-claude-cli@/.test(s.block)).map((s) => s.name); + assert.deepEqual(runs, Array.from({ length: SLOTS }, (_, i) => `Review chunk ${i + 1}`), job); + const first = step("Review chunk 1", job); + assert.equal(/^ {8}if: (.*)$/m.exec(first)[1], "steps.prompt.outcome == 'success'", job); + for (let k = 1; k <= SLOTS; k++) { + const review = step(`Review chunk ${k}`, job); + assert.match(review, new RegExp(`^ {8}id: review-${k}$`, "m"), job); + assert.equal(input(review, "prompt-file"), `\${{ runner.temp }}/pr-review-main-prompt-${k}.md`, job); + // Every input but the prompt is chunk 1's, credentials included. + const rest = (block) => block.replace(/^ {6}- name: .*\n|^ {8}(?:id|if): .*\n/gm, "").replace(/^ {10}prompt-file: .*$/m, ""); + assert.equal(rest(review), rest(first), `${job} ${k}`); + if (k > 1) { + assert.equal(/^ {8}if: (.*)$/m.exec(review)[1], `steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= ${k} && steps.review-${k - 1}.outcome == 'success' && steps.review-${k - 1}.outputs.handoff != 'true'`, `${job} ${k}`); + } + // Each run's transcript is its own artifact. + const transcript = step(`Upload chunk ${k}'s sealed transcript`, job); + assert.equal(input(transcript, "execution-file"), `\${{ steps.review-${k}.outputs.execution-file }}`); + assert.equal(input(transcript, "job-id"), k === 1 ? "${{ github.job }}-${{ matrix.lane }}" : `\${{ github.job }}-\${{ matrix.lane }}-${k}`); + } + // The job's time holds every chunk's review. + assert.ok(Number(/^ {4}timeout-minutes: (\d+)$/m.exec(jobText(job))[1]) >= SLOTS * 22 + 5, job); + // The prompt step writes one prompt per chunk. + const prompts = step("Write the prompt and schema", job); + assert.match(prompts, /printf '%s\\n' "\$prompt" >"\$PROMPT_STEM-\$k\.md"/); + assert.equal(input(prompts, "PROMPT_STEM"), "${{ runner.temp }}/pr-review-main-prompt"); + } +}); + +test("a chunked review that wrote no verdict records chunk-failed for the record job", () => { + for (const job of LEGS) { + const stop = step("Record a failed chunk", job); + assert.equal(index("Record a failed chunk", job), index("Write the verdict", job) + 1, job); + assert.match(stop, /^ {8}id: chunk-stop$/m); + // A chunked leg that reviewed, handed nothing on and wrote no verdict: + // a failed chunk skips the write, a void chunk fails it. + const noHandoff = Array.from({ length: SLOTS }, (_, i) => `steps.review-${i + 1}.outputs.handoff != 'true'`).join(" && "); + assert.equal(/^ {8}if: (.*)$/m.exec(stop)[1], `\${{ !cancelled() && fromJSON(steps.diff.outputs.chunks || '0') > 1 && steps.review-1.outcome != 'skipped' && steps.verdict.outcome != 'success' && ${noHandoff} }}`); + const run = /^ {8}run: \|\n((?: {10}.*\n?)+)/m.exec(`${stop}\n`)[1].replace(/^ {10}/gm, ""); + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-chunk-stop-")); + try { + const out = path.join(dir, "out"); + writeFileSync(out, ""); + const result = spawnSync("bash", ["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", run], { encoding: "utf8", env: { ...process.env, LANE: "claude-review", HEAD_SHA: SHA_A, PR_NUMBER: "7", GITHUB_OUTPUT: out } }); + assert.equal(result.status, 0); + assert.equal(readFileSync(out, "utf8"), `claude-review-stop=chunk-failed\nclaude-review-head=${SHA_A}\n`); + assert.match(result.stdout, /^::notice title=pr-review-main stop::reason=chunk-failed lane=claude-review pr=7; /m); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + // The job still fails: no verdict binds. + assert.match(step("Fail without a verdict", job), /steps\.verdict\.outcome != 'success'/); + } +}); test("it runs only from main on pr-require-checks' completion", () => { const on = /\non:\n([\s\S]*?)\n\S/.exec(text)[1]; @@ -83,20 +143,20 @@ test("the diff shows every file as text and refuses one a review cannot read who assert.match(diff, /python3 -I \.base\/\.github\/scripts\/pr-review-main\/write-diff\.py "\$SRC" "\$BASE_SHA" "\$HEAD_SHA" "\$DATA\/diff\.patch" "\$MAX_DIFF_BYTES"$/m); assert.match(writeDiff, /^DIFF = \["diff", "--text", "--no-ext-diff", "--no-textconv", "--no-color"\]$/m); assert.match(diff, /\^\(Binary files \.\* differ\|GIT binary patch\)\$/); - assert.match(diff, /-gt "\$MAX_DIFF_BYTES"/); + assert.match(diff, /^ {10}python3 -I \.base\/\.github\/scripts\/pr-review-main\/split-diff\.py "\$DATA\/diff\.patch" "\$DATA\/diff" "\$MAX_DIFF_BYTES" "\$MAX_CHUNKS" \|\| split=\$\?$/m); assert.match(diff, /MAX_DIFF_BYTES: "\d+"/); }); test("the diff is split into parts one read returns whole, and a longer line gets no verdict", () => { const diff = step("Write the diff"); - const awk = diff.match(/LC_ALL=C (awk -v dir=.*?' "\$DATA\/diff\.patch")/s)[1]; + const awk = diff.match(/LC_ALL=C (awk -v dir=.*?' "\$dir\.patch")/s)[1]; const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-split-")); try { const run = (text) => { rmSync(path.join(dir, "diff"), { recursive: true, force: true }); mkdirSync(path.join(dir, "diff")); writeFileSync(path.join(dir, "diff.patch"), text); - return spawnSync("bash", ["-c", `LC_ALL=C ${awk}`], { env: { ...process.env, DATA: dir, PART_BYTES: "10", PART_LINES: "2" } }); + return spawnSync("bash", ["-c", `LC_ALL=C ${awk}`], { env: { ...process.env, dir: path.join(dir, "diff"), PART_BYTES: "10", PART_LINES: "2" } }); }; // 5 lines of 4 bytes: 2 lines per part at most, so 3 parts, in order. assert.equal(run("aaa\nbbb\nccc\nddd\neee\n").status, 0); @@ -129,9 +189,81 @@ test("a diff with a line past the review's cap gets no verdict", async () => { } }); +// split-diff.py on a fixture patch: the chunk files it writes, by name. +const SPLIT_DIFF = new URL("./split-diff.py", import.meta.url).pathname; +function split(patch, maxBytes, maxChunks) { + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-chunks-")); + try { + writeFileSync(path.join(dir, "diff.patch"), patch); + mkdirSync(path.join(dir, "out")); + const run = spawnSync("python3", ["-I", SPLIT_DIFF, path.join(dir, "diff.patch"), path.join(dir, "out"), String(maxBytes), String(maxChunks)], { encoding: "utf8" }); + const chunks = Object.fromEntries(readdirSync(path.join(dir, "out")).sort().map((name) => [name, readFileSync(path.join(dir, "out", name), "utf8")])); + return { status: run.status, chunks, stderr: run.stderr }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} +// A file's patch of exactly `bytes` bytes: its header and one hunk padded +// with one added line. +const filePatch = (name, bytes) => { + const head = `diff --git a/${name} b/${name}\n@@ -1 +1 @@\n+`; + const patch = `${head}${"x".repeat(bytes - head.length - 1)}\n`; + assert.equal(Buffer.byteLength(patch), bytes); + return patch; +}; +// The 43-byte header of the file big, and a hunk of exactly `bytes` bytes. +const BIG = "diff --git a/big b/big\n--- a/big\n+++ b/big\n"; +const hunkPatch = (n, bytes) => { + const head = `@@ -${n} +${n} @@\n+`; + const hunk = `${head}${"y".repeat(bytes - head.length - 1)}\n`; + assert.equal(Buffer.byteLength(hunk), bytes); + return hunk; +}; + +test("an oversize diff splits into chunks under the cap at file boundaries, in order", () => { + const [a, b, c] = [filePatch("a", 60), filePatch("b", 70), filePatch("c", 50)]; + // 60 + 70 fits 150; 50 more does not, so c starts the second chunk. + assert.deepEqual(split(a + b + c, 150, 4), { status: 0, chunks: { "chunk-1.patch": a + b, "chunk-2.patch": c }, stderr: "" }); + // At 180 bytes the whole diff is one chunk. + assert.deepEqual(split(a + b + c, 180, 4).chunks, { "chunk-1.patch": a + b + c }); +}); + +test("a single file over the cap splits at its hunks, each piece under its own header", () => { + const [h1, h2, h3] = [hunkPatch(1, 30), hunkPatch(5, 30), hunkPatch(9, 30)]; + assert.equal(BIG.length, 43); + // 43 + 30 + 30 = 103 fits 110; the third hunk goes with the header again. + const result = split(BIG + h1 + h2 + h3, 110, 4); + assert.equal(result.status, 0, result.stderr); + assert.deepEqual(result.chunks, { "chunk-1.patch": BIG + h1 + h2, "chunk-2.patch": BIG + h3 }); + // A file after it packs beside the last piece when it fits: at 120 the + // pieces are the same (133 > 120), and 43 + 30 + 40 = 113 fits. + const small = filePatch("s", 40); + assert.deepEqual(split(BIG + h1 + h2 + h3 + small, 120, 4).chunks, { "chunk-1.patch": BIG + h1 + h2, "chunk-2.patch": BIG + h3 + small }); +}); + +test("a chunk of exactly the cap, and exactly the most chunks, still split", () => { + const [a, b] = [filePatch("a", 100), filePatch("b", 100)]; + assert.deepEqual(split(a, 100, 1), { status: 0, chunks: { "chunk-1.patch": a }, stderr: "" }); + assert.deepEqual(split(a + b, 100, 2).chunks, { "chunk-1.patch": a, "chunk-2.patch": b }); +}); + +test("more chunks than the most, or a hunk larger than one chunk, is too large and writes nothing", () => { + const [a, b, c] = [filePatch("a", 100), filePatch("b", 100), filePatch("c", 100)]; + const over = split(a + b + c, 100, 2); + assert.equal(over.status, 3); + assert.deepEqual(over.chunks, {}); + assert.match(over.stderr, /3 chunks/); + const hunk = split(BIG + hunkPatch(1, 30) + hunkPatch(5, 80), 110, 4); + assert.equal(hunk.status, 3); + assert.deepEqual(hunk.chunks, {}); + // An empty diff is no chunk at all. + assert.deepEqual(split("", 100, 2), { status: 0, chunks: {}, stderr: "" }); +}); + // The "Write the diff" script run under bash as Actions runs it, with git and // python3 stubbed: python3 (write-diff.py) writes the given patch, or fails. -function runDiff(patch, { pythonFails = false, job = KEY, maxBytes = "300" } = {}) { +const PYTHON = spawnSync("bash", ["-c", "command -v python3"], { encoding: "utf8" }).stdout.trim(); +function runDiff(patch, { pythonFails = false, job = KEY, maxBytes = "300", maxChunks = "4" } = {}) { const run = /^ {8}run: \|\n((?:(?: {10}.*)?\n)+)/m.exec(`${step("Write the diff", job)}\n`)[1].replace(/^ {10}/gm, ""); const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-stop-")); try { @@ -139,17 +271,19 @@ function runDiff(patch, { pythonFails = false, job = KEY, maxBytes = "300" } = { mkdirSync(bin); writeFileSync(path.join(bin, "git"), `#!/bin/sh\ncase "$*" in *rev-parse*) echo ${"e".repeat(40)} ;; esac\n`); writeFileSync(path.join(dir, "fixture.patch"), patch); - writeFileSync(path.join(bin, "python3"), pythonFails ? "#!/bin/sh\nexit 1\n" : `#!/bin/sh\ncp "${path.join(dir, "fixture.patch")}" "$6"\n`); + // write-diff.py writes the fixture, or fails; split-diff.py runs as is. + const writes = pythonFails ? "exit 1" : `cp "${path.join(dir, "fixture.patch")}" "$6"`; + writeFileSync(path.join(bin, "python3"), `#!/bin/sh\ncase "$2" in\n *write-diff.py) ${writes} ;;\n *split-diff.py) shift 2; exec "${PYTHON}" -I "${SPLIT_DIFF}" "$@" ;;\n *) exit 9 ;;\nesac\n`); chmodSync(path.join(bin, "git"), 0o755); chmodSync(path.join(bin, "python3"), 0o755); const out = path.join(dir, "out"); writeFileSync(out, ""); const data = path.join(dir, "data"); - const env = { ...process.env, PATH: `${bin}:${process.env.PATH}`, GITHUB_OUTPUT: out, LANE: "claude-security-review", HEAD_SHA: SHA_A, BASE_SHA: SHA_B, PR_NUMBER: "7", DATA: data, SRC: path.join(dir, "src"), MAX_DIFF_BYTES: maxBytes, PART_BYTES: "15000", PART_LINES: "1000", LONG_LINE_BYTES: "1800" }; + const env = { ...process.env, PATH: `${bin}:${process.env.PATH}`, GITHUB_OUTPUT: out, LANE: "claude-security-review", HEAD_SHA: SHA_A, BASE_SHA: SHA_B, PR_NUMBER: "7", DATA: data, SRC: path.join(dir, "src"), MAX_DIFF_BYTES: maxBytes, MAX_CHUNKS: maxChunks, PART_BYTES: "15000", PART_LINES: "1000", LONG_LINE_BYTES: "1800" }; const result = spawnSync("bash", ["--noprofile", "--norc", "-e", "-o", "pipefail", "-c", run], { encoding: "utf8", env }); const parts = (() => { try { - return readdirSync(path.join(data, "diff")); + return readdirSync(path.join(data, "diff"), { recursive: true }).sort(); } catch { return []; } @@ -163,11 +297,26 @@ function runDiff(patch, { pythonFails = false, job = KEY, maxBytes = "300" } = { test("a diff the review cannot read whole stops green and names its reason for the record job", () => { const small = "diff --git a/x b/x\n@@ -1 +1 @@\n+short\n"; for (const job of LEGS) { + // One hunk of 612 bytes fits no 300-byte chunk. const tooLarge = runDiff(`diff --git a/x b/x\n@@ -1 +1 @@\n${"+line\n".repeat(100)}`, { job }); assert.equal(tooLarge.status, 0, job); assert.equal(tooLarge.output, `stop=too-large\nclaude-security-review-stop=too-large\nclaude-security-review-head=${SHA_A}\n`, job); assert.deepEqual(tooLarge.parts, [], job); assert.match(tooLarge.stdout, /^::notice title=pr-review-main stop::reason=too-large lane=claude-security-review pr=7; /m, job); + // Five 200-byte files need five 300-byte chunks, one more than the four. + const files = ["a", "b", "c", "d", "e"].map((name) => filePatch(name, 200)); + const tooMany = runDiff(files.join(""), { job }); + assert.equal(tooMany.status, 0, job); + assert.match(tooMany.output, /^stop=too-large$/m, job); + assert.deepEqual(tooMany.parts, [], job); + // Four fit: one chunk each, each split into its parts, and the counts + // and bytes per chunk in a notice. + const four = runDiff(files.slice(0, 4).join(""), { job }); + assert.equal(four.status, 0, job); + assert.doesNotMatch(four.output, /stop=/, job); + assert.match(four.output, /^chunks=4$/m, job); + assert.deepEqual(four.parts, ["chunk-1", "chunk-1/part-001.patch", "chunk-2", "chunk-2/part-001.patch", "chunk-3", "chunk-3/part-001.patch", "chunk-4", "chunk-4/part-001.patch"], job); + assert.match(four.stdout, /^::notice title=pr-review-main chunks::lane=claude-security-review pr=7 chunks=4 bytes=200,200,200,200$/m, job); const longLine = runDiff(`diff --git a/x b/x\n@@ -1 +1 @@\n+${"x".repeat(1801)}\n`, { job, maxBytes: "300000" }); assert.equal(longLine.status, 0, job); assert.match(longLine.output, /^stop=long-line$/m, job); @@ -179,7 +328,8 @@ test("a diff the review cannot read whole stops green and names its reason for t const whole = runDiff(small, { job }); assert.equal(whole.status, 0, job); assert.doesNotMatch(whole.output, /stop=/, job); - assert.deepEqual(whole.parts, ["part-001.patch"], job); + assert.deepEqual(whole.parts, ["chunk-1", "chunk-1/part-001.patch"], job); + assert.match(whole.output, /^chunks=1$/m, job); } }); @@ -190,7 +340,7 @@ test("after a stop no later step of the leg runs, and each lane's stop leaves th for (const s of gated) assert.match(s.block, /steps\.diff\.outcome == 'success' && steps\.diff\.outputs\.stop == ''/, `${job}: ${s.name}`); for (const lane of ["claude-review", "claude-security-review"]) { for (const kind of ["stop", "head"]) { - assert.match(jobText(job), new RegExp(`^ {6}${lane}-${kind}: \\$\\{\\{ steps\\.diff\\.outputs\\.${lane}-${kind} \\}\\}$`, "m"), `${job} ${lane}-${kind}`); + assert.match(jobText(job), new RegExp(`^ {6}${lane}-${kind}: \\$\\{\\{ steps\\.diff\\.outputs\\.${lane}-${kind} \\|\\| steps\\.chunk-stop\\.outputs\\.${lane}-${kind} \\}\\}$`, "m"), `${job} ${lane}-${kind}`); } } } @@ -218,7 +368,7 @@ test("the record job posts each stop as a neutral verdict=none record, from a cl const block = stepsOf("record").find((s) => s.name === "Record the stops").block; // A stop run uploads no artifact, so the step never waits on the download. assert.doesNotMatch(block, /steps\.download/); - assert.match(block, /\^\(too-large\|binary\|long-line\)\$/); + assert.match(block, /\^\(too-large\|binary\|long-line\|chunk-failed\)\$/); for (const [lane, prefix] of [["claude-review", "CODE"], ["claude-security-review", "SECURITY"]]) { for (const kind of ["stop", "head"]) { assert.equal(input(block, `${prefix}_${kind.toUpperCase()}`), `\${{ needs.review.outputs.${lane}-${kind} || needs.review-subscription.outputs.${lane}-${kind} || needs.review-fallback.outputs.${lane}-${kind} }}`); @@ -231,6 +381,7 @@ test("the record job posts each stop as a neutral verdict=none record, from a cl assert.ok(args.includes(arg), arg); } assert.doesNotMatch(posted.calls, /claude-review\n/); + assert.ok(recordStops({ CODE_STOP: "chunk-failed", CODE_HEAD: SHA_A }).calls.split("\n").includes("output[title]=verdict=none reason=chunk-failed")); // No stop, nothing posted; a reason or head outside the closed set fails. assert.deepEqual(recordStops({}), { status: 0, calls: "" }); for (const bad of [{ CODE_STOP: "other", CODE_HEAD: SHA_A }, { CODE_STOP: "too-large", CODE_HEAD: "main" }, { CODE_STOP: "too-large\nx", CODE_HEAD: SHA_A }]) { @@ -445,14 +596,23 @@ test("an edited rename prints whole, as a delete and an add", () => { assert.match(patch, /^\+run the payload$/m); }); -test("each leg leaves a lane record", () => { +test("each chunk's review leaves a lane record", () => { + assert.ok(SLOTS >= 2, String(SLOTS)); for (const job of LEGS) { - const report = step("Report the review's outcome", job); - assert.match(report, /report-lane-outcome@[0-9a-f]{40}/); - assert.match(report, /^ {10}lane: pr-review-main$/m); - assert.match(report, /^ {10}lane-id: pr-review-main-\$\{\{ matrix\.lane \}\}$/m); - assert.ok(index("Review the pull request", job) < index("Report the review's outcome", job)); - assert.match(step("Review the pull request", job), /^ {8}continue-on-error: true$/m); + for (let k = 1; k <= SLOTS; k++) { + const report = step(`Report chunk ${k}'s outcome`, job); + assert.match(report, /report-lane-outcome@[0-9a-f]{40}/); + assert.match(report, /^ {10}lane: pr-review-main$/m); + assert.match(report, /^ {10}lane-id: pr-review-main-\$\{\{ matrix\.lane \}\}$/m); + assert.equal(input(report, "outcome"), `\${{ steps.review-${k}.outcome }}`); + assert.equal(input(report, "step-started-at"), k === 1 ? "${{ steps.prompt.outputs.started-at }}" : `\${{ steps.time-${k}.outputs.started-at }}`); + assert.equal(index(`Report chunk ${k}'s outcome`, job), index(`Review chunk ${k}`, job) + 1); + assert.match(step(`Review chunk ${k}`, job), /^ {8}continue-on-error: true$/m); + if (k > 1) { + assert.equal(index(`Time chunk ${k}`, job) + 1, index(`Review chunk ${k}`, job)); + assert.equal(/^ {8}if: (.*)$/m.exec(step(`Time chunk ${k}`, job))[1], /^ {8}if: (.*)$/m.exec(step(`Review chunk ${k}`, job))[1]); + } + } } }); @@ -490,8 +650,8 @@ test("it checks out main only, and runs local actions from that checkout", () => }); test("the model reads its data directory alone", () => { - for (const job of LEGS) { - const review = step("Review the pull request", job); + for (const job of LEGS) for (let k = 1; k <= SLOTS; k++) { + const review = step(`Review chunk ${k}`, job); assert.match(review, /^ {10}tools: Read$/m); assert.match(review, /^ {10}allowed-tools: Read\(\.\/pr-review-main-data\/\*\*\)$/m); assert.match(review, /^ {10}permission-mode: dontAsk$/m); @@ -499,7 +659,7 @@ test("the model reads its data directory alone", () => { assert.ok(review.includes(` ${denied}\n`), denied); } } - for (const placeholder of ["LANE_NAME", "CRITERIA_DIR", "STANDARDS_DIR", "DIFF_PARTS", "CONTEXT_FILE", "PR_FILE"]) { + for (const placeholder of ["LANE_NAME", "CRITERIA_DIR", "STANDARDS_DIR", "DIFF_PARTS", "CHUNK_NOTE", "CONTEXT_FILE", "PR_FILE"]) { assert.ok(prompt.includes(placeholder), placeholder); assert.ok(step("Write the prompt and schema").includes(`prompt=\${prompt//${placeholder}/`), placeholder); } @@ -518,8 +678,15 @@ test("the verdict uploads once, after it is written, under the writer's name", ( const write = step("Write the verdict", job); assert.match(write, /TREE_SHA: \$\{\{ steps\.diff\.outputs\.tree \}\}/); assert.match(write, /HEAD_SHA: \$\{\{ steps\.gate\.outputs\.head-sha \}\}/); - // A handed-off leg writes no verdict. - assert.match(write, /^ {8}if: steps\.review\.outcome == 'success' && steps\.review\.outputs\.handoff != 'true'$/m); + // The verdict is written only when every chunk's review truly + // succeeded (the reconciler holds a lost run once it starts), and a + // handed-off leg writes none. + const slots = Array.from({ length: SLOTS }, (_, i) => i + 1); + const each = slots.slice(1).map((k) => `(fromJSON(steps.diff.outputs.chunks || '0') < ${k} || steps.review-${k}.outcome == 'success')`); + const noHandoff = slots.map((k) => `steps.review-${k}.outputs.handoff != 'true'`); + assert.equal(/^ {8}if: (.*)$/m.exec(write)[1], ["steps.review-1.outcome == 'success'", ...each, ...noHandoff].join(" && "), job); + assert.ok(write.includes(` EXECUTION_FILES: |\n${slots.map((k) => ` \${{ steps.review-${k}.outputs.execution-file }}\n`).join("")}`), job); + assert.equal(input(write, "CHUNKS"), "${{ steps.diff.outputs.chunks }}"); } assert.match(step("Write the diff"), /rev-parse --verify "\$HEAD_SHA\^\{tree\}"/); }); @@ -565,7 +732,7 @@ test("the legs run the review lanes' credentials in the review lanes' order", () } assert.deepEqual(order, LEGS); const used = order.map((job) => { - const review = step("Review the pull request", job); + const review = step("Review chunk 1", job); const set = CREDENTIAL_INPUTS.filter((name) => input(review, name) !== undefined); assert.equal(set.length, 1, `${job} passes one credential`); return [set[0], /^\$\{\{ secrets\.([A-Z_]+) \}\}$/.exec(input(review, set[0]))[1]]; @@ -589,7 +756,7 @@ test("the legs run the review lanes' credentials in the review lanes' order", () for (const job of LEGS) { for (const s of STEPS[job]) { const values = [...s.block.matchAll(/secrets\.([A-Z_]+)(?![A-Z_]| != '')/g)].map(([, name]) => name); - if (s.name === "Review the pull request") assert.equal(values.length, 1, job); + if (/^Review chunk \d$/.test(s.name)) assert.equal(values.length, 1, job); else assert.deepEqual(values, [], `${job}: ${s.name}`); } } @@ -689,9 +856,11 @@ test("a later leg runs only for the lanes the previous leg handed on", () => { [...fbOutputs.matchAll(/^ {6}([a-z-]+):/gm)].map(([, name]) => name), ["claude-review-stop", "claude-security-review-stop", "claude-review-head", "claude-security-review-head"], ); - assert.doesNotMatch(step("Review the pull request", KEY), /fallback-ready/); - assert.equal(input(step("Review the pull request", SUB), "fallback-ready"), "${{ steps.credential.outputs.fallback-ready }}"); - assert.doesNotMatch(step("Review the pull request", FB), /fallback-ready/); + for (let k = 1; k <= SLOTS; k++) { + assert.doesNotMatch(step(`Review chunk ${k}`, KEY), /fallback-ready/); + assert.equal(input(step(`Review chunk ${k}`, SUB), "fallback-ready"), "${{ steps.credential.outputs.fallback-ready }}"); + assert.doesNotMatch(step(`Review chunk ${k}`, FB), /fallback-ready/); + } // The record job waits for every leg and runs when a later one skipped. assert.match(jobText("record"), /^ {4}needs: \[review, review-subscription, review-fallback\]$/m); assert.match(jobText("record"), /^ {4}if: \$\{\{ !cancelled\(\) && github\.ref == 'refs\/heads\/main' \}\}$/m); @@ -700,10 +869,12 @@ test("a later leg runs only for the lanes the previous leg handed on", () => { test("the handoff step names its lane and a closed reason, and a handed-off leg stays green", () => { for (const job of [KEY, SUB]) { const handoff = step("Hand the lane on", job); - assert.match(handoff, /^ {8}if: steps\.review\.outputs\.handoff == 'true' \|\| steps\.credential\.outputs\.present == 'false'$/m); - assert.match(handoff, /^ {10}REASON: \$\{\{ steps\.credential\.outputs\.present == 'false' && 'no-key' \|\| steps\.review\.outputs\.handoff-reason \}\}$/m); - assert.ok(index("Review the pull request", job) + 1 === index("Hand the lane on", job)); - assert.match(step("Report the review's outcome", job), /&& steps\.review\.outputs\.handoff != 'true'$/m); + // Any chunk's handoff hands the whole lane on. + const slots = Array.from({ length: SLOTS }, (_, i) => i + 1); + assert.equal(/^ {8}if: (.*)$/m.exec(handoff)[1], `${slots.map((k) => `steps.review-${k}.outputs.handoff == 'true'`).join(" || ")} || steps.credential.outputs.present == 'false'`); + assert.equal(input(handoff, "REASON"), `\${{ steps.credential.outputs.present == 'false' && 'no-key' || ${slots.map((k) => `steps.review-${k}.outputs.handoff-reason`).join(" || ")} }}`); + assert.ok(index(`Upload chunk ${SLOTS}'s sealed transcript`, job) + 1 === index("Hand the lane on", job)); + for (const k of slots) assert.match(step(`Report chunk ${k}'s outcome`, job), new RegExp(`&& steps\\.review-${k}\\.outputs\\.handoff != 'true'$`, "m")); assert.match(step("Fail without a verdict", job), /&& steps\.handoff\.outcome != 'success'$/m); } const run = /^ {8}run: \|\n((?: {10}.*\n?)+)/m.exec(step("Hand the lane on"))[1].replace(/^ {10}/gm, ""); @@ -740,17 +911,19 @@ test("every later leg repeats the key leg's steps but for its credential and why assert.deepEqual(STEPS[FB].map((s) => s.name), keyNames.filter((name) => name !== "Hand the lane on")); for (const job of [SUB, FB]) { for (const s of STEPS[job]) assert.equal(normal(s.block), normal(step(s.name, KEY)), `${job}: ${s.name}`); - // Its record names why it ran, from the route's closed reason. - assert.equal( - input(step("Report the review's outcome", job), "fallback-reason"), - "${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }}", - ); + // Each record names why it ran, from the route's closed reason. + for (let k = 1; k <= SLOTS; k++) { + assert.equal( + input(step(`Report chunk ${k}'s outcome`, job), "fallback-reason"), + "${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }}", + ); + } // Its gates are the key leg's. // Past the route checks, its gates are the key leg's. const gates = (id) => /^ {4}if: >-\n((?: {6}.*\n)+)/m.exec(jobText(id))[1].replace(/^ {6}!cancelled\(\)\n {6}&& needs\.[a-z-]+\.result == 'success'\n {6}&& /, " "); assert.equal(gates(job), gates(KEY)); } - assert.equal(input(step("Report the review's outcome", KEY), "fallback-reason"), undefined); + assert.equal(input(step("Report chunk 1's outcome", KEY), "fallback-reason"), undefined); }); test("one lane handed on beside one that reviewed routes only the handed-on lane", () => { diff --git a/.github/scripts/pr-review-main/write-verdict.mjs b/.github/scripts/pr-review-main/write-verdict.mjs index 0be9c50614..3e1e604c18 100644 --- a/.github/scripts/pr-review-main/write-verdict.mjs +++ b/.github/scripts/pr-review-main/write-verdict.mjs @@ -1,14 +1,16 @@ -// Checks one main-run review's structured output, the last result message of -// run-claude-cli's execution file, and writes the verdict file that +// Checks each chunk's main-run review structured output, the last result +// message of that chunk's run-claude-cli execution file, and writes the one +// verdict file that // pr-review-main.yml uploads as pr-review-main--- // (babysit_review_evidence.main_review_name). The gate counts only its // numbers: `findings`, every finding the model returned, and `critical`, the -// CRITICAL ones. The pull request, head, base and tree come from the job's +// CRITICAL ones, each summed over the chunks. The pull request, head, base and tree come from the job's // own reads, never from the model. A run with any permission denial, output // that is not {findings, summary}, a finding that is not // {severity, path, line, title, body}, or model text a credential pattern // matches (lib/publish-gate.mjs findSecret) writes nothing and throws, so the -// job fails and uploads no verdict. Model text is cut to fixed lengths and +// job fails and uploads no verdict; so does any one chunk that ran no review, +// left no result or did not read its own parts whole. Model text is cut to fixed lengths and // never reaches a shell. import { appendFileSync, mkdirSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; @@ -161,42 +163,100 @@ export function buildVerdict({ output, lane, pr, headSha, baseSha, tree, runId, }; } +// One verdict over the chunks a diff over one review's size cap was split +// into, in chunk order: each chunk's output is checked as a whole review's, +// and the counts are the sums. One chunk is the single-pass verdict. +export function buildChunkedVerdict({ outputs, ...rest }) { + if (!Array.isArray(outputs) || outputs.length === 0) { + throw new Error("there is no chunk output"); + } + const chunks = outputs.map((output, i) => { + try { + return buildVerdict({ ...rest, output }); + } catch (error) { + throw new Error(`chunk ${i + 1} of ${outputs.length}: ${error.message}`); + } + }); + if (chunks.length === 1) { + return chunks[0]; + } + return { + ...chunks[0], + findings: chunks.reduce((sum, c) => sum + c.findings, 0), + critical: chunks.reduce((sum, c) => sum + c.critical, 0), + summary: chunks.map((c, i) => `Chunk ${i + 1} of ${chunks.length}: ${c.summary}`).join("\n\n"), + items: chunks.flatMap((c) => c.items), + }; +} + +// The diff is DIFF_DIR/chunk-/part-.patch; EXECUTION_FILES names each +// chunk's execution file, one per line in chunk order. export function main(env = process.env) { - for (const key of ["EXECUTION_FILE", "LANE", "PR_NUMBER", "HEAD_SHA", "BASE_SHA", "TREE_SHA", "GITHUB_RUN_ID", "VERDICT_DIR", "DIFF_DIR", "GITHUB_OUTPUT"]) { + for (const key of ["EXECUTION_FILES", "CHUNKS", "LANE", "PR_NUMBER", "HEAD_SHA", "BASE_SHA", "TREE_SHA", "GITHUB_RUN_ID", "VERDICT_DIR", "DIFF_DIR", "GITHUB_OUTPUT"]) { if (!env[key]) { throw new Error(`${key} is not set`); } } - let messages; - try { - messages = JSON.parse(readFileSync(env.EXECUTION_FILE, "utf8")); - } catch { - throw new Error("the execution file is unreadable"); + if (!/^[1-9][0-9]?$/.test(env.CHUNKS)) { + throw new Error("the chunk count is malformed"); } - let parts; + const count = Number(env.CHUNKS); + let dirs; try { - parts = readdirSync(env.DIFF_DIR) - .filter((name) => /^part-\d{3}\.patch$/.test(name)) - .map((name) => path.join(env.DIFF_DIR, name)); + dirs = readdirSync(env.DIFF_DIR).filter((name) => /^chunk-\d+$/.test(name)); } catch { - throw new Error("the diff parts are unreadable"); + throw new Error("the diff chunks are unreadable"); + } + const want = Array.from({ length: count }, (_, i) => `chunk-${i + 1}`); + if (dirs.length !== count || !want.every((name) => dirs.includes(name))) { + throw new Error(`the diff holds ${dirs.length} chunk directories, not the ${count} the diff step wrote`); } - readCoverage(messages, parts); - const maxLineBytes = longestLine(parts.map((part) => readFileSync(part, "utf8"))); - const verdict = buildVerdict({ - maxLineBytes, - output: structuredOutput(messages), + const files = env.EXECUTION_FILES.split("\n"); + const chunks = want.map((name, i) => { + const label = `chunk ${i + 1} of ${count}`; + let parts; + try { + parts = readdirSync(path.join(env.DIFF_DIR, name)) + .filter((part) => /^part-\d{3}\.patch$/.test(part)) + .map((part) => path.join(env.DIFF_DIR, name, part)); + } catch { + throw new Error(`${label}: the diff parts are unreadable`); + } + try { + if (!files[i]) { + throw new Error("no review ran"); + } + let messages; + try { + messages = JSON.parse(readFileSync(files[i], "utf8")); + } catch { + throw new Error("the execution file is unreadable"); + } + readCoverage(messages, parts); + const texts = parts.map((part) => readFileSync(part, "utf8")); + return { texts, output: structuredOutput(messages), bytes: texts.reduce((sum, t) => sum + Buffer.byteLength(t, "utf8"), 0) }; + } catch (error) { + throw new Error(`${label}: ${error.message}`); + } + }); + const rest = { + maxLineBytes: longestLine(chunks.flatMap((c) => c.texts)), lane: env.LANE, pr: env.PR_NUMBER, headSha: env.HEAD_SHA, baseSha: env.BASE_SHA, tree: env.TREE_SHA, runId: env.GITHUB_RUN_ID, - }); + }; + const verdict = buildChunkedVerdict({ ...rest, outputs: chunks.map((c) => c.output) }); mkdirSync(env.VERDICT_DIR, { recursive: true }); writeFileSync(path.join(env.VERDICT_DIR, "verdict.json"), `${JSON.stringify(verdict, null, 2)}\n`); const name = artifactName(verdict.lane, verdict.pr, verdict.tree); appendFileSync(env.GITHUB_OUTPUT, `name=${name}\nfindings=${verdict.findings}\ncritical=${verdict.critical}\n`); + chunks.forEach((c, i) => { + const one = buildVerdict({ ...rest, output: c.output }); + console.log(`::notice title=pr-review-main chunk::lane=${verdict.lane} chunk=${i + 1}/${count} bytes=${c.bytes} findings=${one.findings} critical=${one.critical}`); + }); console.log(`write-verdict: ${name} findings=${verdict.findings} critical=${verdict.critical}`); } diff --git a/.github/scripts/pr-review-main/write-verdict.test.mjs b/.github/scripts/pr-review-main/write-verdict.test.mjs index 86e7071f14..c15f397dd8 100644 --- a/.github/scripts/pr-review-main/write-verdict.test.mjs +++ b/.github/scripts/pr-review-main/write-verdict.test.mjs @@ -9,7 +9,7 @@ import path from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; -import { artifactName, buildVerdict, LONG_LINE_BYTES, longestLine, main, readCoverage, structuredOutput } from "./write-verdict.mjs"; +import { artifactName, buildChunkedVerdict, buildVerdict, LONG_LINE_BYTES, longestLine, main, readCoverage, structuredOutput } from "./write-verdict.mjs"; const HEAD = "a".repeat(40); const BASE = "b".repeat(40); @@ -110,33 +110,124 @@ test("the cap is the merge gate's", () => { assert.equal(Number(python.stdout.trim()), LONG_LINE_BYTES); }); +test("a chunked review's verdict sums every chunk's findings and CRITICAL ones", () => { + const { output: _, ...rest } = args(undefined); + const verdict = buildChunkedVerdict({ + ...rest, + outputs: [ + { findings: [finding("CRITICAL"), finding("IMPORTANT")], summary: "first" }, + { findings: [finding("CRITICAL"), finding("CRITICAL"), finding("SUGGESTION")], summary: "second" }, + ], + }); + assert.deepEqual({ findings: verdict.findings, critical: verdict.critical, items: verdict.items.length }, { findings: 5, critical: 3, items: 5 }); + assert.equal(verdict.summary, "Chunk 1 of 2: first\n\nChunk 2 of 2: second"); + assert.deepEqual(Object.keys(verdict).sort(), Object.keys(buildVerdict(args({ findings: [], summary: "s" }))).sort()); + // One chunk is the single-pass verdict, unchanged. + const one = { findings: [finding("CRITICAL")], summary: "s" }; + assert.deepEqual(buildChunkedVerdict({ ...rest, outputs: [one] }), buildVerdict(args(one))); + // Any chunk off the schema voids the whole verdict. + assert.throws(() => buildChunkedVerdict({ ...rest, outputs: [one, { findings: [finding("HIGH")], summary: "s" }] }), /chunk 2 of 2/); + assert.throws(() => buildChunkedVerdict({ ...rest, outputs: [] })); +}); + +// A diff directory of chunk-/part-.patch files, and each chunk's +// execution file: a Read of each of its own parts and a result. +function chunkedRun(dir, chunks, { skip = [], result = () => ({ findings: [finding("CRITICAL")], summary: "s" }) } = {}) { + const diff = path.join(dir, "diff"); + const files = chunks.map((texts, i) => { + const chunk = path.join(diff, `chunk-${i + 1}`); + mkdirSync(chunk, { recursive: true }); + const parts = texts.map((body, j) => { + const part = path.join(chunk, `part-00${j + 1}.patch`); + writeFileSync(part, body); + return part; + }); + if (skip.includes(i + 1)) return ""; + const execution = path.join(dir, `execution-${i + 1}.json`); + const output = result(i + 1); + const messages = parts.map((part, j) => read(`r${i}-${j}`, part)); + if (output !== undefined) messages.push({ type: "result", structured_output: output, permission_denials: [] }); + writeFileSync(execution, JSON.stringify(messages)); + return execution; + }); + const output = path.join(dir, "output"); + writeFileSync(output, ""); + return { + EXECUTION_FILES: `${files.join("\n")}\n`, + CHUNKS: String(chunks.length), + LANE: "claude-review", + PR_NUMBER: "42", + HEAD_SHA: HEAD, + BASE_SHA: BASE, + TREE_SHA: TREE, + GITHUB_RUN_ID: "9", + VERDICT_DIR: path.join(dir, "verdict"), + DIFF_DIR: diff, + GITHUB_OUTPUT: output, + }; +} + test("main writes verdict.json and the name output", () => { const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-")); try { - const execution = path.join(dir, "execution.json"); - const parts = path.join(dir, "diff"); - mkdirSync(parts); - writeFileSync(path.join(parts, "part-001.patch"), "+x\n"); - const result = { type: "result", structured_output: { findings: [finding("CRITICAL")], summary: "s" }, permission_denials: [] }; - writeFileSync(execution, JSON.stringify([read("r1", path.join(parts, "part-001.patch")), result])); - const output = path.join(dir, "output"); - writeFileSync(output, ""); - main({ - EXECUTION_FILE: execution, - LANE: "claude-review", - PR_NUMBER: "42", - HEAD_SHA: HEAD, - BASE_SHA: BASE, - TREE_SHA: TREE, - GITHUB_RUN_ID: "9", - VERDICT_DIR: path.join(dir, "verdict"), - DIFF_DIR: parts, - GITHUB_OUTPUT: output, - }); + const env = chunkedRun(dir, [["+x\n"]]); + main(env); const verdict = JSON.parse(readFileSync(path.join(dir, "verdict", "verdict.json"), "utf8")); assert.equal(verdict.findings, 1); assert.equal(verdict.critical, 1); - assert.match(readFileSync(output, "utf8"), new RegExp(`^name=pr-review-main-claude-review-42-${TREE}$`, "m")); + assert.match(readFileSync(env.GITHUB_OUTPUT, "utf8"), new RegExp(`^name=pr-review-main-claude-review-42-${TREE}$`, "m")); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("a chunked run writes one verdict summed over its chunks and a notice per chunk", () => { + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-")); + try { + // Chunk 1 is 3 + 4 bytes with one CRITICAL; chunk 2 is 5 bytes with two + // CRITICAL and one IMPORTANT. + const outputs = { 1: { findings: [finding("CRITICAL")], summary: "a" }, 2: { findings: [finding("CRITICAL"), finding("IMPORTANT"), finding("CRITICAL")], summary: "b" } }; + const env = chunkedRun(dir, [["+x\n", "+yz\n"], ["+abc\n"]], { result: (k) => outputs[k] }); + const script = fileURLToPath(new URL("./write-verdict.mjs", import.meta.url)); + const run = spawnSync(process.execPath, [script], { encoding: "utf8", env: { ...process.env, ...env } }); + assert.equal(run.status, 0, run.stdout + run.stderr); + const verdict = JSON.parse(readFileSync(path.join(dir, "verdict", "verdict.json"), "utf8")); + assert.deepEqual({ findings: verdict.findings, critical: verdict.critical }, { findings: 4, critical: 3 }); + assert.match(run.stdout, /^::notice title=pr-review-main chunk::lane=claude-review chunk=1\/2 bytes=7 findings=1 critical=1$/m); + assert.match(run.stdout, /^::notice title=pr-review-main chunk::lane=claude-review chunk=2\/2 bytes=5 findings=3 critical=2$/m); + assert.match(readFileSync(env.GITHUB_OUTPUT, "utf8"), /^findings=4\ncritical=3$/m); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("a chunk that failed, left no verdict, or skipped a part writes no verdict at all", () => { + const cases = { + "a chunk never ran": { skip: [2] }, + "a chunk left no result": { result: (k) => (k === 2 ? undefined : { findings: [], summary: "s" }) }, + "a chunk's output is off the schema": { result: (k) => (k === 2 ? { findings: [], verdict: "pass" } : { findings: [], summary: "s" }) }, + }; + for (const [name, options] of Object.entries(cases)) { + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-")); + try { + const env = chunkedRun(dir, [["+x\n"], ["+y\n"]], options); + assert.throws(() => main(env), /chunk 2 of 2/, name); + assert.throws(() => readFileSync(path.join(dir, "verdict", "verdict.json")), /ENOENT/, name); + assert.equal(readFileSync(env.GITHUB_OUTPUT, "utf8"), "", name); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + } + // A run that read chunk 1's parts for chunk 2, or a chunk count that is + // not the directories on disk, voids the verdict too. + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-")); + try { + const env = chunkedRun(dir, [["+x\n"], ["+y\n"]]); + const [first] = env.EXECUTION_FILES.split("\n"); + assert.throws(() => main({ ...env, EXECUTION_FILES: `${first}\n${first}\n` }), /chunk 2 of 2: the run did not read 1 of 1/); + assert.throws(() => main({ ...env, CHUNKS: "1" }), /2 chunk directories/); + assert.throws(() => main({ ...env, CHUNKS: "3" }), /2 chunk directories/); + assert.throws(() => readFileSync(path.join(dir, "verdict", "verdict.json")), /ENOENT/); } finally { rmSync(dir, { recursive: true, force: true }); } diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 0ef00f387f..6f0c11d7da 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -11,7 +11,9 @@ name: pr-review-main # workflow_run runs this file from main, never a head's copy. No pull request # code runs here: the diff, the trusted pull request text and the lane's # criteria (copied from main) are data files in pr-review-main-data, and the -# model reads them with the Read tool alone. Each lane uploads its verdict as +# model reads them with the Read tool alone. A diff over one review's size +# cap is reviewed in chunks, one run each, and the verdict sums them. Each +# lane uploads its verdict as # the artifact pr-review-main--- (write-verdict.mjs); only a # run's own steps can upload its artifacts, so the gate authenticates the # name through this run's record (path, workflow_run event, main, completed @@ -85,7 +87,8 @@ jobs: route: review-security switch: CLAUDE_SECURITY_REVIEW_DISABLED runs-on: ubuntu-24.04 - timeout-minutes: 30 + # Each chunk's review has 22 minutes. + timeout-minutes: 100 permissions: contents: read # check out main; read the PR's commits for the diff pull-requests: read # the PR, its files, comments and reviews @@ -97,10 +100,10 @@ jobs: claude-security-review: ${{ steps.handoff.outputs.claude-security-review }} claude-review-base-sha: ${{ steps.base.outputs.claude-review-base-sha }} claude-security-review-base-sha: ${{ steps.base.outputs.claude-security-review-base-sha }} - claude-review-stop: ${{ steps.diff.outputs.claude-review-stop }} - claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop }} - claude-review-head: ${{ steps.diff.outputs.claude-review-head }} - claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head }} + claude-review-stop: ${{ steps.diff.outputs.claude-review-stop || steps.chunk-stop.outputs.claude-review-stop }} + claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop || steps.chunk-stop.outputs.claude-security-review-stop }} + claude-review-head: ${{ steps.diff.outputs.claude-review-head || steps.chunk-stop.outputs.claude-review-head }} + claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head || steps.chunk-stop.outputs.claude-security-review-head }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -193,13 +196,16 @@ jobs: # into or out of a lane source, a mode change, a symlink or submodule, # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an - # added line names still print whole. A patch that still - # shows a binary file, is larger than one review can read whole, or - # holds a line longer than Read returns whole gets no verdict: the step - # stops green with the reason as an output, the record job posts it as - # `verdict=none reason=` on the head, and the lanes stay unbound - # under that reason. Any other failure fails the step. The tree the - # verdict is keyed on comes from git, never from the model. + # added line names still print whole. A patch over the size cap is + # split into chunks of at most the cap, at file boundaries and a larger + # file's hunks (split-diff.py), and each chunk is reviewed by its own + # run; the verdict sums them. A patch that still shows a binary file, + # needs more than MAX_CHUNKS chunks, or holds a line longer than Read + # returns whole gets no verdict: the step stops green with the reason + # as an output, the record job posts it as `verdict=none + # reason=` on the head, and the lanes stay unbound under that + # reason. Any other failure fails the step. The tree the verdict is + # keyed on comes from git, never from the model. - name: Write the diff id: diff if: steps.select.outputs.review == 'true' @@ -213,6 +219,9 @@ jobs: # About 75k tokens of patch: room in the lane's turn budget to read # it whole beside the criteria (judgment). MAX_DIFF_BYTES: "300000" + # The most chunks, each one review: at most the leg's Review chunk + # steps (workflow.test.mjs holds them equal). + MAX_CHUNKS: "4" # One Read returns at most 25,000 tokens # (https://code.claude.com/docs/en/tools-reference#large-files), and # a token is at least a byte, so a part this size with its line @@ -240,36 +249,52 @@ jobs: if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then stop binary "the diff holds a binary file the review cannot read" fi - if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then - stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, over the $MAX_DIFF_BYTES one review reads whole" - fi # A diff line's text, past its +, - or space, longer than the cap. if ! LC_ALL=C awk -v max="$LONG_LINE_BYTES" 'length($0) - 1 > max { exit 3 }' "$DATA/diff.patch"; then stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi - # Parts the reviewer reads whole, each named in the prompt; the - # verdict counts only when every part was read whole - # (write-verdict.mjs `readCoverage`). A line longer than a part gets - # no verdict. + # Chunks of at most MAX_DIFF_BYTES, one review each; exit 3 is a + # diff more chunks than MAX_CHUNKS hold, or a hunk over one chunk. mkdir "$DATA/diff" - if ! LC_ALL=C awk -v dir="$DATA/diff" -v maxb="$PART_BYTES" -v maxl="$PART_LINES" ' - { len = length($0) + 1 - if (len > maxb) exit 3 - if (n == 0 || b + len > maxb || l >= maxl) { if (f) close(f); f = sprintf("%s/part-%03d.patch", dir, ++n); b = 0; l = 0 } - print > f; b += len; l++ }' "$DATA/diff.patch"; then - echo "::error::a diff line is over $PART_BYTES bytes, more than one read returns whole; no verdict, so the lanes stay unbound" + split=0 + python3 -I .base/.github/scripts/pr-review-main/split-diff.py "$DATA/diff.patch" "$DATA/diff" "$MAX_DIFF_BYTES" "$MAX_CHUNKS" || split=$? + if [ "$split" -eq 3 ]; then + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes each read whole" + elif [ "$split" -ne 0 ]; then + echo "::error::the diff did not split into chunks; no verdict, so the lanes stay unbound" exit 1 fi rm "$DATA/diff.patch" - if [ -z "$(find "$DATA/diff" -type f)" ]; then + chunks=$(find "$DATA/diff" -name 'chunk-*.patch' | wc -l) + if [ "$chunks" -eq 0 ]; then echo "::error::the diff is empty; no verdict, so the lanes stay unbound" exit 1 fi + # Each chunk's parts, which its reviewer reads whole, each named in + # its prompt; the verdict counts only when every part was read + # whole (write-verdict.mjs `readCoverage`). A line longer than a + # part gets no verdict. + bytes= + for k in $(seq "$chunks"); do + dir="$DATA/diff/chunk-$k" + mkdir "$dir" + if ! LC_ALL=C awk -v dir="$dir" -v maxb="$PART_BYTES" -v maxl="$PART_LINES" ' + { len = length($0) + 1 + if (len > maxb) exit 3 + if (n == 0 || b + len > maxb || l >= maxl) { if (f) close(f); f = sprintf("%s/part-%03d.patch", dir, ++n); b = 0; l = 0 } + print > f; b += len; l++ }' "$dir.patch"; then + echo "::error::a diff line is over $PART_BYTES bytes, more than one read returns whole; no verdict, so the lanes stay unbound" + exit 1 + fi + bytes="$bytes${bytes:+,}$(wc -c <"$dir.patch")" + rm "$dir.patch" + done tree=$(git -C "$SRC" rev-parse --verify "$HEAD_SHA^{tree}") jq -n --argjson pr "$PR_NUMBER" --arg head "$HEAD_SHA" --arg base "$BASE_SHA" --arg tree "$tree" \ '{pr: $pr, head_sha: $head, base_sha: $base, tree: $tree}' >"$DATA/pr.json" - echo "tree=$tree" >>"$GITHUB_OUTPUT" - echo "diff: $(cat "$DATA"/diff/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" + { echo "tree=$tree"; echo "chunks=$chunks"; } >>"$GITHUB_OUTPUT" + echo "::notice title=pr-review-main chunks::lane=$LANE pr=$PR_NUMBER chunks=$chunks bytes=$bytes" + echo "diff: $(cat "$DATA"/diff/chunk-*/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" - name: Select the trusted PR text if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' @@ -344,7 +369,8 @@ jobs: with: install-subprocess-isolation: true - # Files, not step outputs: run-claude-cli reads both. + # Files, not step outputs: run-claude-cli reads both. One prompt per + # chunk, naming that chunk's parts. - name: Write the prompt and schema id: prompt if: steps.credential.outputs.present == 'true' @@ -353,23 +379,36 @@ jobs: LANE: ${{ matrix.lane }} SKILL: ${{ matrix.skill }} MODEL: ${{ steps.models.outputs.model }} - PROMPT_FILE: ${{ runner.temp }}/pr-review-main-prompt.md + CHUNKS: ${{ steps.diff.outputs.chunks }} + PROMPT_STEM: ${{ runner.temp }}/pr-review-main-prompt SCHEMA_FILE: ${{ runner.temp }}/pr-review-main-schema.json run: | if [ -z "$MODEL" ]; then echo "::error::the review profile's model is default, which run-claude-cli cannot pass; name an alias" exit 1 fi - prompt=$(<.base/.github/scripts/pr-review-main/prompt.md) - prompt=${prompt//LANE_NAME/$LANE} - prompt=${prompt//CRITERIA_DIR/$DATA/review/skills/$SKILL} - prompt=${prompt//STANDARDS_DIR/$DATA/standards} - # shellcheck disable=SC2016 # the backticks are Markdown, not a command - parts=$(printf ' - `%s`\n' "$DATA"/diff/part-*.patch) - prompt=${prompt//DIFF_PARTS/$parts} - prompt=${prompt//CONTEXT_FILE/$DATA/context.json} - prompt=${prompt//PR_FILE/$DATA/pr.json} - printf '%s\n' "$prompt" >"$PROMPT_FILE" + if ! [[ $CHUNKS =~ ^[1-9]$ ]]; then + echo "::error::the chunk count is malformed" + exit 1 + fi + template=$(<.base/.github/scripts/pr-review-main/prompt.md) + for k in $(seq "$CHUNKS"); do + prompt=$template + prompt=${prompt//LANE_NAME/$LANE} + prompt=${prompt//CRITERIA_DIR/$DATA/review/skills/$SKILL} + prompt=${prompt//STANDARDS_DIR/$DATA/standards} + # shellcheck disable=SC2016 # the backticks are Markdown, not a command + parts=$(printf ' - `%s`\n' "$DATA/diff/chunk-$k"/part-*.patch) + prompt=${prompt//DIFF_PARTS/$parts} + note= + if [ "$CHUNKS" -gt 1 ]; then + note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries (a larger file at its hunks, each piece under the file's header lines), and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." + fi + prompt=${prompt//CHUNK_NOTE/$note} + prompt=${prompt//CONTEXT_FILE/$DATA/context.json} + prompt=${prompt//PR_FILE/$DATA/pr.json} + printf '%s\n' "$prompt" >"$PROMPT_STEM-$k.md" + done echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" printf '%s\n' '{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"severity":{"enum":["CRITICAL","IMPORTANT","SUGGESTION"]},"path":{"type":"string"},"line":{"type":"integer","minimum":0},"title":{"type":"string"},"body":{"type":"string"}},"required":["severity","path","line","title","body"],"additionalProperties":false}},"summary":{"type":"string"}},"required":["findings","summary"],"additionalProperties":false}' >"$SCHEMA_FILE" @@ -381,8 +420,14 @@ jobs: # the outcome report; "Fail without a verdict" carries the red. A # handoff skips the verdict and the outcome report: the next leg # reviews the lane and records it. - - name: Review the pull request - id: review + # One run per chunk, in order, each on its own prompt: chunk k runs + # only when the diff has k chunks and chunk k-1 reviewed and handed + # nothing on, so a handoff hands the whole lane on and the next leg + # reviews every chunk again. Each run leaves its own lane record and + # its sealed transcript (failed runs included, kept 7 days for the + # puller; a failed upload never fails the job). + - name: Review chunk 1 + id: review-1 if: steps.prompt.outcome == 'success' continue-on-error: true timeout-minutes: 22 @@ -393,7 +438,7 @@ jobs: with: lane: pr-review-main role: reviews - prompt-file: ${{ runner.temp }}/pr-review-main-prompt.md + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-1.md preamble: >- You run unattended; return only the findings the schema asks for; nothing you write is posted. @@ -419,58 +464,261 @@ jobs: json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json leg-timeout-minutes: "15" - # The lane's handoff reason, from run-claude-cli's closed handoff-reason - # (or an empty credential), under the lane's own output name. - - name: Hand the lane on - id: handoff - if: steps.review.outputs.handoff == 'true' || steps.credential.outputs.present == 'false' - env: - LANE: ${{ matrix.lane }} - REASON: ${{ steps.credential.outputs.present == 'false' && 'no-key' || steps.review.outputs.handoff-reason }} - run: | - case "$REASON" in - credit-stop | auth | limit-stop | no-key) ;; - *) echo "::error::the handoff reason is not one run-claude-cli sets"; exit 1 ;; - esac - echo "$LANE=$REASON" >>"$GITHUB_OUTPUT" - echo "::notice::$LANE handed on ($REASON); the next leg reviews it" - - # One lane record per lane and run, as pr-verify.yml keeps one, so this - # lane's model legs show in lane telemetry and spend: the leg that - # handed nothing on records it. - - name: Report the review's outcome - if: (steps.review.outcome == 'success' || steps.review.outcome == 'failure') && steps.review.outputs.handoff != 'true' + - name: Report chunk 1's outcome + if: (steps.review-1.outcome == 'success' || steps.review-1.outcome == 'failure') && steps.review-1.outputs.handoff != 'true' uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 with: - outcome: ${{ steps.review.outcome }} - execution-file: ${{ steps.review.outputs.execution-file }} - log-file: ${{ steps.review.outputs.log-file }} + outcome: ${{ steps.review-1.outcome }} + execution-file: ${{ steps.review-1.outputs.execution-file }} + log-file: ${{ steps.review-1.outputs.log-file }} lane: pr-review-main lane-id: pr-review-main-${{ matrix.lane }} step-started-at: ${{ steps.prompt.outputs.started-at }} step-timeout-minutes: 22 - credential: ${{ steps.review.outputs.credential }} + credential: ${{ steps.review-1.outputs.credential }} max-turns: ${{ steps.models.outputs.max-turns }} - known-failure-class: ${{ steps.review.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review.outputs.lane-class }} + known-failure-class: ${{ steps.review-1.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-1.outputs.lane-class }} - # The leg's transcript, failed legs included, sealed to the transcript - # key and kept 7 days for the puller; a failed upload never fails the job. - - name: Upload the sealed transcript - if: always() && steps.review.outputs.execution-file != '' + - name: Upload chunk 1's sealed transcript + if: always() && steps.review-1.outputs.execution-file != '' continue-on-error: true uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] with: - execution-file: ${{ steps.review.outputs.execution-file }} + execution-file: ${{ steps.review-1.outputs.execution-file }} public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} job-id: ${{ github.job }}-${{ matrix.lane }} + - name: Time chunk 2 + id: time-2 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 2 && steps.review-1.outcome == 'success' && steps.review-1.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 2 + id: review-2 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 2 && steps.review-1.outcome == 'success' && steps.review-1.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-2.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + api-key: ${{ secrets.REVIEWS_CLAUDE_API_KEY }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + - name: Report chunk 2's outcome + if: (steps.review-2.outcome == 'success' || steps.review-2.outcome == 'failure') && steps.review-2.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-2.outcome }} + execution-file: ${{ steps.review-2.outputs.execution-file }} + log-file: ${{ steps.review-2.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-2.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-2.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-2.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-2.outputs.lane-class }} + + - name: Upload chunk 2's sealed transcript + if: always() && steps.review-2.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-2.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-2 + + - name: Time chunk 3 + id: time-3 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 3 && steps.review-2.outcome == 'success' && steps.review-2.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 3 + id: review-3 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 3 && steps.review-2.outcome == 'success' && steps.review-2.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-3.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + api-key: ${{ secrets.REVIEWS_CLAUDE_API_KEY }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + - name: Report chunk 3's outcome + if: (steps.review-3.outcome == 'success' || steps.review-3.outcome == 'failure') && steps.review-3.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-3.outcome }} + execution-file: ${{ steps.review-3.outputs.execution-file }} + log-file: ${{ steps.review-3.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-3.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-3.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-3.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-3.outputs.lane-class }} + + - name: Upload chunk 3's sealed transcript + if: always() && steps.review-3.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-3.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-3 + + - name: Time chunk 4 + id: time-4 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 4 && steps.review-3.outcome == 'success' && steps.review-3.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 4 + id: review-4 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 4 && steps.review-3.outcome == 'success' && steps.review-3.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-4.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + api-key: ${{ secrets.REVIEWS_CLAUDE_API_KEY }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + - name: Report chunk 4's outcome + if: (steps.review-4.outcome == 'success' || steps.review-4.outcome == 'failure') && steps.review-4.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-4.outcome }} + execution-file: ${{ steps.review-4.outputs.execution-file }} + log-file: ${{ steps.review-4.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-4.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-4.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-4.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-4.outputs.lane-class }} + + - name: Upload chunk 4's sealed transcript + if: always() && steps.review-4.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-4.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-4 + + # The lane's handoff reason, from run-claude-cli's closed handoff-reason + # (or an empty credential), under the lane's own output name. + - name: Hand the lane on + id: handoff + if: steps.review-1.outputs.handoff == 'true' || steps.review-2.outputs.handoff == 'true' || steps.review-3.outputs.handoff == 'true' || steps.review-4.outputs.handoff == 'true' || steps.credential.outputs.present == 'false' + env: + LANE: ${{ matrix.lane }} + REASON: ${{ steps.credential.outputs.present == 'false' && 'no-key' || steps.review-1.outputs.handoff-reason || steps.review-2.outputs.handoff-reason || steps.review-3.outputs.handoff-reason || steps.review-4.outputs.handoff-reason }} + run: | + case "$REASON" in + credit-stop | auth | limit-stop | no-key) ;; + *) echo "::error::the handoff reason is not one run-claude-cli sets"; exit 1 ;; + esac + echo "$LANE=$REASON" >>"$GITHUB_OUTPUT" + echo "::notice::$LANE handed on ($REASON); the next leg reviews it" + # The structured output is untrusted model text: read from the file and # checked by the script, never interpolated into a command. - name: Write the verdict id: verdict - if: steps.review.outcome == 'success' && steps.review.outputs.handoff != 'true' + if: steps.review-1.outcome == 'success' && (fromJSON(steps.diff.outputs.chunks || '0') < 2 || steps.review-2.outcome == 'success') && (fromJSON(steps.diff.outputs.chunks || '0') < 3 || steps.review-3.outcome == 'success') && (fromJSON(steps.diff.outputs.chunks || '0') < 4 || steps.review-4.outcome == 'success') && steps.review-1.outputs.handoff != 'true' && steps.review-2.outputs.handoff != 'true' && steps.review-3.outputs.handoff != 'true' && steps.review-4.outputs.handoff != 'true' env: - EXECUTION_FILE: ${{ steps.review.outputs.execution-file }} + # Each chunk's execution file in chunk order; it runs only when + # every chunk's review succeeded and handed nothing on. + EXECUTION_FILES: | + ${{ steps.review-1.outputs.execution-file }} + ${{ steps.review-2.outputs.execution-file }} + ${{ steps.review-3.outputs.execution-file }} + ${{ steps.review-4.outputs.execution-file }} + CHUNKS: ${{ steps.diff.outputs.chunks }} LANE: ${{ matrix.lane }} PR_NUMBER: ${{ steps.gate.outputs.pr-number }} HEAD_SHA: ${{ steps.gate.outputs.head-sha }} @@ -480,6 +728,21 @@ jobs: DIFF_DIR: ${{ github.workspace }}/pr-review-main-data/diff run: node .base/.github/scripts/pr-review-main/write-verdict.mjs + # A chunked review that wrote no verdict and handed nothing on (a + # chunk's review failed, or one left no result or skipped a part) is a + # designed stop too: no chunk's counts bind, and the record job names + # it on the head. The job still fails. + - name: Record a failed chunk + id: chunk-stop + if: ${{ !cancelled() && fromJSON(steps.diff.outputs.chunks || '0') > 1 && steps.review-1.outcome != 'skipped' && steps.verdict.outcome != 'success' && steps.review-1.outputs.handoff != 'true' && steps.review-2.outputs.handoff != 'true' && steps.review-3.outputs.handoff != 'true' && steps.review-4.outputs.handoff != 'true' }} + env: + LANE: ${{ matrix.lane }} + HEAD_SHA: ${{ steps.gate.outputs.head-sha }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + run: | + echo "::notice title=pr-review-main stop::reason=chunk-failed lane=$LANE pr=$PR_NUMBER; a chunk's review left no verdict, so no verdict binds and the lanes stay unbound" + { echo "$LANE-stop=chunk-failed"; echo "$LANE-head=$HEAD_SHA"; } >>"$GITHUB_OUTPUT" + - name: Upload the verdict if: steps.verdict.outcome == 'success' uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 @@ -575,7 +838,8 @@ jobs: fail-fast: false matrix: ${{ fromJSON(needs.review-handoffs.outputs.matrix) }} runs-on: ubuntu-24.04 - timeout-minutes: 30 + # Each chunk's review has 22 minutes. + timeout-minutes: 100 permissions: contents: read # check out main; read the PR's commits for the diff pull-requests: read # the PR, its files, comments and reviews @@ -587,10 +851,10 @@ jobs: claude-security-review: ${{ steps.handoff.outputs.claude-security-review }} claude-review-base-sha: ${{ steps.base.outputs.claude-review-base-sha }} claude-security-review-base-sha: ${{ steps.base.outputs.claude-security-review-base-sha }} - claude-review-stop: ${{ steps.diff.outputs.claude-review-stop }} - claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop }} - claude-review-head: ${{ steps.diff.outputs.claude-review-head }} - claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head }} + claude-review-stop: ${{ steps.diff.outputs.claude-review-stop || steps.chunk-stop.outputs.claude-review-stop }} + claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop || steps.chunk-stop.outputs.claude-security-review-stop }} + claude-review-head: ${{ steps.diff.outputs.claude-review-head || steps.chunk-stop.outputs.claude-review-head }} + claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head || steps.chunk-stop.outputs.claude-security-review-head }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -682,13 +946,16 @@ jobs: # into or out of a lane source, a mode change, a symlink or submodule, # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an - # added line names still print whole. A patch that still - # shows a binary file, is larger than one review can read whole, or - # holds a line longer than Read returns whole gets no verdict: the step - # stops green with the reason as an output, the record job posts it as - # `verdict=none reason=` on the head, and the lanes stay unbound - # under that reason. Any other failure fails the step. The tree the - # verdict is keyed on comes from git, never from the model. + # added line names still print whole. A patch over the size cap is + # split into chunks of at most the cap, at file boundaries and a larger + # file's hunks (split-diff.py), and each chunk is reviewed by its own + # run; the verdict sums them. A patch that still shows a binary file, + # needs more than MAX_CHUNKS chunks, or holds a line longer than Read + # returns whole gets no verdict: the step stops green with the reason + # as an output, the record job posts it as `verdict=none + # reason=` on the head, and the lanes stay unbound under that + # reason. Any other failure fails the step. The tree the verdict is + # keyed on comes from git, never from the model. - name: Write the diff id: diff if: steps.select.outputs.review == 'true' @@ -702,6 +969,9 @@ jobs: # About 75k tokens of patch: room in the lane's turn budget to read # it whole beside the criteria (judgment). MAX_DIFF_BYTES: "300000" + # The most chunks, each one review: at most the leg's Review chunk + # steps (workflow.test.mjs holds them equal). + MAX_CHUNKS: "4" # One Read returns at most 25,000 tokens # (https://code.claude.com/docs/en/tools-reference#large-files), and # a token is at least a byte, so a part this size with its line @@ -729,36 +999,52 @@ jobs: if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then stop binary "the diff holds a binary file the review cannot read" fi - if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then - stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, over the $MAX_DIFF_BYTES one review reads whole" - fi # A diff line's text, past its +, - or space, longer than the cap. if ! LC_ALL=C awk -v max="$LONG_LINE_BYTES" 'length($0) - 1 > max { exit 3 }' "$DATA/diff.patch"; then stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi - # Parts the reviewer reads whole, each named in the prompt; the - # verdict counts only when every part was read whole - # (write-verdict.mjs `readCoverage`). A line longer than a part gets - # no verdict. + # Chunks of at most MAX_DIFF_BYTES, one review each; exit 3 is a + # diff more chunks than MAX_CHUNKS hold, or a hunk over one chunk. mkdir "$DATA/diff" - if ! LC_ALL=C awk -v dir="$DATA/diff" -v maxb="$PART_BYTES" -v maxl="$PART_LINES" ' - { len = length($0) + 1 - if (len > maxb) exit 3 - if (n == 0 || b + len > maxb || l >= maxl) { if (f) close(f); f = sprintf("%s/part-%03d.patch", dir, ++n); b = 0; l = 0 } - print > f; b += len; l++ }' "$DATA/diff.patch"; then - echo "::error::a diff line is over $PART_BYTES bytes, more than one read returns whole; no verdict, so the lanes stay unbound" + split=0 + python3 -I .base/.github/scripts/pr-review-main/split-diff.py "$DATA/diff.patch" "$DATA/diff" "$MAX_DIFF_BYTES" "$MAX_CHUNKS" || split=$? + if [ "$split" -eq 3 ]; then + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes each read whole" + elif [ "$split" -ne 0 ]; then + echo "::error::the diff did not split into chunks; no verdict, so the lanes stay unbound" exit 1 fi rm "$DATA/diff.patch" - if [ -z "$(find "$DATA/diff" -type f)" ]; then + chunks=$(find "$DATA/diff" -name 'chunk-*.patch' | wc -l) + if [ "$chunks" -eq 0 ]; then echo "::error::the diff is empty; no verdict, so the lanes stay unbound" exit 1 fi + # Each chunk's parts, which its reviewer reads whole, each named in + # its prompt; the verdict counts only when every part was read + # whole (write-verdict.mjs `readCoverage`). A line longer than a + # part gets no verdict. + bytes= + for k in $(seq "$chunks"); do + dir="$DATA/diff/chunk-$k" + mkdir "$dir" + if ! LC_ALL=C awk -v dir="$dir" -v maxb="$PART_BYTES" -v maxl="$PART_LINES" ' + { len = length($0) + 1 + if (len > maxb) exit 3 + if (n == 0 || b + len > maxb || l >= maxl) { if (f) close(f); f = sprintf("%s/part-%03d.patch", dir, ++n); b = 0; l = 0 } + print > f; b += len; l++ }' "$dir.patch"; then + echo "::error::a diff line is over $PART_BYTES bytes, more than one read returns whole; no verdict, so the lanes stay unbound" + exit 1 + fi + bytes="$bytes${bytes:+,}$(wc -c <"$dir.patch")" + rm "$dir.patch" + done tree=$(git -C "$SRC" rev-parse --verify "$HEAD_SHA^{tree}") jq -n --argjson pr "$PR_NUMBER" --arg head "$HEAD_SHA" --arg base "$BASE_SHA" --arg tree "$tree" \ '{pr: $pr, head_sha: $head, base_sha: $base, tree: $tree}' >"$DATA/pr.json" - echo "tree=$tree" >>"$GITHUB_OUTPUT" - echo "diff: $(cat "$DATA"/diff/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" + { echo "tree=$tree"; echo "chunks=$chunks"; } >>"$GITHUB_OUTPUT" + echo "::notice title=pr-review-main chunks::lane=$LANE pr=$PR_NUMBER chunks=$chunks bytes=$bytes" + echo "diff: $(cat "$DATA"/diff/chunk-*/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" - name: Select the trusted PR text if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' @@ -833,7 +1119,8 @@ jobs: with: install-subprocess-isolation: true - # Files, not step outputs: run-claude-cli reads both. + # Files, not step outputs: run-claude-cli reads both. One prompt per + # chunk, naming that chunk's parts. - name: Write the prompt and schema id: prompt if: steps.credential.outputs.present == 'true' @@ -842,23 +1129,36 @@ jobs: LANE: ${{ matrix.lane }} SKILL: ${{ matrix.skill }} MODEL: ${{ steps.models.outputs.model }} - PROMPT_FILE: ${{ runner.temp }}/pr-review-main-prompt.md + CHUNKS: ${{ steps.diff.outputs.chunks }} + PROMPT_STEM: ${{ runner.temp }}/pr-review-main-prompt SCHEMA_FILE: ${{ runner.temp }}/pr-review-main-schema.json run: | if [ -z "$MODEL" ]; then echo "::error::the review profile's model is default, which run-claude-cli cannot pass; name an alias" exit 1 fi - prompt=$(<.base/.github/scripts/pr-review-main/prompt.md) - prompt=${prompt//LANE_NAME/$LANE} - prompt=${prompt//CRITERIA_DIR/$DATA/review/skills/$SKILL} - prompt=${prompt//STANDARDS_DIR/$DATA/standards} - # shellcheck disable=SC2016 # the backticks are Markdown, not a command - parts=$(printf ' - `%s`\n' "$DATA"/diff/part-*.patch) - prompt=${prompt//DIFF_PARTS/$parts} - prompt=${prompt//CONTEXT_FILE/$DATA/context.json} - prompt=${prompt//PR_FILE/$DATA/pr.json} - printf '%s\n' "$prompt" >"$PROMPT_FILE" + if ! [[ $CHUNKS =~ ^[1-9]$ ]]; then + echo "::error::the chunk count is malformed" + exit 1 + fi + template=$(<.base/.github/scripts/pr-review-main/prompt.md) + for k in $(seq "$CHUNKS"); do + prompt=$template + prompt=${prompt//LANE_NAME/$LANE} + prompt=${prompt//CRITERIA_DIR/$DATA/review/skills/$SKILL} + prompt=${prompt//STANDARDS_DIR/$DATA/standards} + # shellcheck disable=SC2016 # the backticks are Markdown, not a command + parts=$(printf ' - `%s`\n' "$DATA/diff/chunk-$k"/part-*.patch) + prompt=${prompt//DIFF_PARTS/$parts} + note= + if [ "$CHUNKS" -gt 1 ]; then + note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries (a larger file at its hunks, each piece under the file's header lines), and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." + fi + prompt=${prompt//CHUNK_NOTE/$note} + prompt=${prompt//CONTEXT_FILE/$DATA/context.json} + prompt=${prompt//PR_FILE/$DATA/pr.json} + printf '%s\n' "$prompt" >"$PROMPT_STEM-$k.md" + done echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" printf '%s\n' '{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"severity":{"enum":["CRITICAL","IMPORTANT","SUGGESTION"]},"path":{"type":"string"},"line":{"type":"integer","minimum":0},"title":{"type":"string"},"body":{"type":"string"}},"required":["severity","path","line","title","body"],"additionalProperties":false}},"summary":{"type":"string"}},"required":["findings","summary"],"additionalProperties":false}' >"$SCHEMA_FILE" @@ -870,8 +1170,14 @@ jobs: # the outcome report; "Fail without a verdict" carries the red. A # handoff skips the verdict and the outcome report: the next leg # reviews the lane and records it. - - name: Review the pull request - id: review + # One run per chunk, in order, each on its own prompt: chunk k runs + # only when the diff has k chunks and chunk k-1 reviewed and handed + # nothing on, so a handoff hands the whole lane on and the next leg + # reviews every chunk again. Each run leaves its own lane record and + # its sealed transcript (failed runs included, kept 7 days for the + # puller; a failed upload never fails the job). + - name: Review chunk 1 + id: review-1 if: steps.prompt.outcome == 'success' continue-on-error: true timeout-minutes: 22 @@ -882,7 +1188,7 @@ jobs: with: lane: pr-review-main role: reviews - prompt-file: ${{ runner.temp }}/pr-review-main-prompt.md + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-1.md preamble: >- You run unattended; return only the findings the schema asks for; nothing you write is posted. @@ -909,110 +1215,337 @@ jobs: fallback-ready: ${{ steps.credential.outputs.fallback-ready }} leg-timeout-minutes: "15" - # The lane's handoff reason, from run-claude-cli's closed handoff-reason - # (or an empty credential), under the lane's own output name. - - name: Hand the lane on - id: handoff - if: steps.review.outputs.handoff == 'true' || steps.credential.outputs.present == 'false' - env: - LANE: ${{ matrix.lane }} - REASON: ${{ steps.credential.outputs.present == 'false' && 'no-key' || steps.review.outputs.handoff-reason }} - run: | - case "$REASON" in - credit-stop | auth | limit-stop | no-key) ;; - *) echo "::error::the handoff reason is not one run-claude-cli sets"; exit 1 ;; - esac - echo "$LANE=$REASON" >>"$GITHUB_OUTPUT" - echo "::notice::$LANE handed on ($REASON); the next leg reviews it" - - # One lane record per lane and run, as pr-verify.yml keeps one, so this - # lane's model legs show in lane telemetry and spend: the leg that - # handed nothing on records it. - - name: Report the review's outcome - if: (steps.review.outcome == 'success' || steps.review.outcome == 'failure') && steps.review.outputs.handoff != 'true' + - name: Report chunk 1's outcome + if: (steps.review-1.outcome == 'success' || steps.review-1.outcome == 'failure') && steps.review-1.outputs.handoff != 'true' uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 with: - outcome: ${{ steps.review.outcome }} - execution-file: ${{ steps.review.outputs.execution-file }} - log-file: ${{ steps.review.outputs.log-file }} + outcome: ${{ steps.review-1.outcome }} + execution-file: ${{ steps.review-1.outputs.execution-file }} + log-file: ${{ steps.review-1.outputs.log-file }} lane: pr-review-main lane-id: pr-review-main-${{ matrix.lane }} step-started-at: ${{ steps.prompt.outputs.started-at }} step-timeout-minutes: 22 - credential: ${{ steps.review.outputs.credential }} + credential: ${{ steps.review-1.outputs.credential }} max-turns: ${{ steps.models.outputs.max-turns }} - known-failure-class: ${{ steps.review.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review.outputs.lane-class }} + known-failure-class: ${{ steps.review-1.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-1.outputs.lane-class }} # Why this leg ran, for the lane's one record. fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} - # The leg's transcript, failed legs included, sealed to the transcript - # key and kept 7 days for the puller; a failed upload never fails the job. - - name: Upload the sealed transcript - if: always() && steps.review.outputs.execution-file != '' + - name: Upload chunk 1's sealed transcript + if: always() && steps.review-1.outputs.execution-file != '' continue-on-error: true uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] with: - execution-file: ${{ steps.review.outputs.execution-file }} + execution-file: ${{ steps.review-1.outputs.execution-file }} public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} job-id: ${{ github.job }}-${{ matrix.lane }} - # The structured output is untrusted model text: read from the file and - # checked by the script, never interpolated into a command. - - name: Write the verdict - id: verdict - if: steps.review.outcome == 'success' && steps.review.outputs.handoff != 'true' - env: - EXECUTION_FILE: ${{ steps.review.outputs.execution-file }} - LANE: ${{ matrix.lane }} - PR_NUMBER: ${{ steps.gate.outputs.pr-number }} - HEAD_SHA: ${{ steps.gate.outputs.head-sha }} - BASE_SHA: ${{ steps.gate.outputs.base-sha }} - TREE_SHA: ${{ steps.diff.outputs.tree }} - VERDICT_DIR: ${{ runner.temp }}/pr-review-main-verdict - DIFF_DIR: ${{ github.workspace }}/pr-review-main-data/diff - run: node .base/.github/scripts/pr-review-main/write-verdict.mjs - - - name: Upload the verdict - if: steps.verdict.outcome == 'success' - uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 - with: - name: ${{ steps.verdict.outputs.name }} - path: ${{ runner.temp }}/pr-review-main-verdict/verdict.json - # The longest: an expired verdict with findings would lower the count. - retention-days: 90 - if-no-files-found: error - - - name: Summarize the verdict - if: steps.verdict.outcome == 'success' - env: - VERDICT_FILE: ${{ runner.temp }}/pr-review-main-verdict/verdict.json - LANE: ${{ matrix.lane }} - run: | - { - echo "### $LANE, reviewed from main" - echo - echo '```json' - jq '{findings, critical, summary: (.summary | gsub("`"; "'"'"'")), items: (.items | map(.title |= gsub("`"; "'"'"'") | .body |= gsub("`"; "'"'"'")))}' "$VERDICT_FILE" - echo '```' - } >>"$GITHUB_STEP_SUMMARY" + - name: Time chunk 2 + id: time-2 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 2 && steps.review-1.outcome == 'success' && steps.review-1.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" - - name: Log the API budget - if: always() + - name: Review chunk 2 + id: review-2 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 2 && steps.review-1.outcome == 'success' && steps.review-1.outputs.handoff != 'true' continue-on-error: true - timeout-minutes: 1 - uses: ./.base/.github/actions/log-api-budget # zizmor: ignore[self-repository] + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" with: - tokens: github-token - - # A reviewed pull request that left no verdict and handed nothing on - # fails here, so the run shows it; the lane stays unbound either way. - - name: Fail without a verdict - if: always() && steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' && steps.handoff.outcome != 'success' - run: | - echo "::error::the review left no verdict for this head" - exit 1 - - # The lanes the subscription leg handed on, as review-fallback's matrix. + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-2.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + subscription-token: ${{ secrets.REVIEWS_CLAUDE_CODE_OAUTH_TOKEN }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + fallback-ready: ${{ steps.credential.outputs.fallback-ready }} + leg-timeout-minutes: "15" + + - name: Report chunk 2's outcome + if: (steps.review-2.outcome == 'success' || steps.review-2.outcome == 'failure') && steps.review-2.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-2.outcome }} + execution-file: ${{ steps.review-2.outputs.execution-file }} + log-file: ${{ steps.review-2.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-2.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-2.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-2.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-2.outputs.lane-class }} + # Why this leg ran, for the lane's one record. + fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} + + - name: Upload chunk 2's sealed transcript + if: always() && steps.review-2.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-2.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-2 + + - name: Time chunk 3 + id: time-3 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 3 && steps.review-2.outcome == 'success' && steps.review-2.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 3 + id: review-3 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 3 && steps.review-2.outcome == 'success' && steps.review-2.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-3.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + subscription-token: ${{ secrets.REVIEWS_CLAUDE_CODE_OAUTH_TOKEN }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + fallback-ready: ${{ steps.credential.outputs.fallback-ready }} + leg-timeout-minutes: "15" + + - name: Report chunk 3's outcome + if: (steps.review-3.outcome == 'success' || steps.review-3.outcome == 'failure') && steps.review-3.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-3.outcome }} + execution-file: ${{ steps.review-3.outputs.execution-file }} + log-file: ${{ steps.review-3.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-3.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-3.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-3.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-3.outputs.lane-class }} + # Why this leg ran, for the lane's one record. + fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} + + - name: Upload chunk 3's sealed transcript + if: always() && steps.review-3.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-3.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-3 + + - name: Time chunk 4 + id: time-4 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 4 && steps.review-3.outcome == 'success' && steps.review-3.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 4 + id: review-4 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 4 && steps.review-3.outcome == 'success' && steps.review-3.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-4.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + subscription-token: ${{ secrets.REVIEWS_CLAUDE_CODE_OAUTH_TOKEN }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + fallback-ready: ${{ steps.credential.outputs.fallback-ready }} + leg-timeout-minutes: "15" + + - name: Report chunk 4's outcome + if: (steps.review-4.outcome == 'success' || steps.review-4.outcome == 'failure') && steps.review-4.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-4.outcome }} + execution-file: ${{ steps.review-4.outputs.execution-file }} + log-file: ${{ steps.review-4.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-4.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-4.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-4.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-4.outputs.lane-class }} + # Why this leg ran, for the lane's one record. + fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} + + - name: Upload chunk 4's sealed transcript + if: always() && steps.review-4.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-4.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-4 + + # The lane's handoff reason, from run-claude-cli's closed handoff-reason + # (or an empty credential), under the lane's own output name. + - name: Hand the lane on + id: handoff + if: steps.review-1.outputs.handoff == 'true' || steps.review-2.outputs.handoff == 'true' || steps.review-3.outputs.handoff == 'true' || steps.review-4.outputs.handoff == 'true' || steps.credential.outputs.present == 'false' + env: + LANE: ${{ matrix.lane }} + REASON: ${{ steps.credential.outputs.present == 'false' && 'no-key' || steps.review-1.outputs.handoff-reason || steps.review-2.outputs.handoff-reason || steps.review-3.outputs.handoff-reason || steps.review-4.outputs.handoff-reason }} + run: | + case "$REASON" in + credit-stop | auth | limit-stop | no-key) ;; + *) echo "::error::the handoff reason is not one run-claude-cli sets"; exit 1 ;; + esac + echo "$LANE=$REASON" >>"$GITHUB_OUTPUT" + echo "::notice::$LANE handed on ($REASON); the next leg reviews it" + + # The structured output is untrusted model text: read from the file and + # checked by the script, never interpolated into a command. + - name: Write the verdict + id: verdict + if: steps.review-1.outcome == 'success' && (fromJSON(steps.diff.outputs.chunks || '0') < 2 || steps.review-2.outcome == 'success') && (fromJSON(steps.diff.outputs.chunks || '0') < 3 || steps.review-3.outcome == 'success') && (fromJSON(steps.diff.outputs.chunks || '0') < 4 || steps.review-4.outcome == 'success') && steps.review-1.outputs.handoff != 'true' && steps.review-2.outputs.handoff != 'true' && steps.review-3.outputs.handoff != 'true' && steps.review-4.outputs.handoff != 'true' + env: + # Each chunk's execution file in chunk order; it runs only when + # every chunk's review succeeded and handed nothing on. + EXECUTION_FILES: | + ${{ steps.review-1.outputs.execution-file }} + ${{ steps.review-2.outputs.execution-file }} + ${{ steps.review-3.outputs.execution-file }} + ${{ steps.review-4.outputs.execution-file }} + CHUNKS: ${{ steps.diff.outputs.chunks }} + LANE: ${{ matrix.lane }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + HEAD_SHA: ${{ steps.gate.outputs.head-sha }} + BASE_SHA: ${{ steps.gate.outputs.base-sha }} + TREE_SHA: ${{ steps.diff.outputs.tree }} + VERDICT_DIR: ${{ runner.temp }}/pr-review-main-verdict + DIFF_DIR: ${{ github.workspace }}/pr-review-main-data/diff + run: node .base/.github/scripts/pr-review-main/write-verdict.mjs + + # A chunked review that wrote no verdict and handed nothing on (a + # chunk's review failed, or one left no result or skipped a part) is a + # designed stop too: no chunk's counts bind, and the record job names + # it on the head. The job still fails. + - name: Record a failed chunk + id: chunk-stop + if: ${{ !cancelled() && fromJSON(steps.diff.outputs.chunks || '0') > 1 && steps.review-1.outcome != 'skipped' && steps.verdict.outcome != 'success' && steps.review-1.outputs.handoff != 'true' && steps.review-2.outputs.handoff != 'true' && steps.review-3.outputs.handoff != 'true' && steps.review-4.outputs.handoff != 'true' }} + env: + LANE: ${{ matrix.lane }} + HEAD_SHA: ${{ steps.gate.outputs.head-sha }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + run: | + echo "::notice title=pr-review-main stop::reason=chunk-failed lane=$LANE pr=$PR_NUMBER; a chunk's review left no verdict, so no verdict binds and the lanes stay unbound" + { echo "$LANE-stop=chunk-failed"; echo "$LANE-head=$HEAD_SHA"; } >>"$GITHUB_OUTPUT" + + - name: Upload the verdict + if: steps.verdict.outcome == 'success' + uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 + with: + name: ${{ steps.verdict.outputs.name }} + path: ${{ runner.temp }}/pr-review-main-verdict/verdict.json + # The longest: an expired verdict with findings would lower the count. + retention-days: 90 + if-no-files-found: error + + - name: Summarize the verdict + if: steps.verdict.outcome == 'success' + env: + VERDICT_FILE: ${{ runner.temp }}/pr-review-main-verdict/verdict.json + LANE: ${{ matrix.lane }} + run: | + { + echo "### $LANE, reviewed from main" + echo + echo '```json' + jq '{findings, critical, summary: (.summary | gsub("`"; "'"'"'")), items: (.items | map(.title |= gsub("`"; "'"'"'") | .body |= gsub("`"; "'"'"'")))}' "$VERDICT_FILE" + echo '```' + } >>"$GITHUB_STEP_SUMMARY" + + - name: Log the API budget + if: always() + continue-on-error: true + timeout-minutes: 1 + uses: ./.base/.github/actions/log-api-budget # zizmor: ignore[self-repository] + with: + tokens: github-token + + # A reviewed pull request that left no verdict and handed nothing on + # fails here, so the run shows it; the lane stays unbound either way. + - name: Fail without a verdict + if: always() && steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' && steps.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' && steps.handoff.outcome != 'success' + run: | + echo "::error::the review left no verdict for this head" + exit 1 + + # The lanes the subscription leg handed on, as review-fallback's matrix. subscription-handoffs: needs: review-subscription if: >- @@ -1063,7 +1596,8 @@ jobs: fail-fast: false matrix: ${{ fromJSON(needs.subscription-handoffs.outputs.matrix) }} runs-on: ubuntu-24.04 - timeout-minutes: 30 + # Each chunk's review has 22 minutes. + timeout-minutes: 100 permissions: contents: read # check out main; read the PR's commits for the diff pull-requests: read # the PR, its files, comments and reviews @@ -1071,10 +1605,10 @@ jobs: # One output per lane, since a matrix job's outputs merge: the designed # stop that left the lane without a verdict, and the head it names. outputs: - claude-review-stop: ${{ steps.diff.outputs.claude-review-stop }} - claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop }} - claude-review-head: ${{ steps.diff.outputs.claude-review-head }} - claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head }} + claude-review-stop: ${{ steps.diff.outputs.claude-review-stop || steps.chunk-stop.outputs.claude-review-stop }} + claude-security-review-stop: ${{ steps.diff.outputs.claude-security-review-stop || steps.chunk-stop.outputs.claude-security-review-stop }} + claude-review-head: ${{ steps.diff.outputs.claude-review-head || steps.chunk-stop.outputs.claude-review-head }} + claude-security-review-head: ${{ steps.diff.outputs.claude-security-review-head || steps.chunk-stop.outputs.claude-security-review-head }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -1166,13 +1700,16 @@ jobs: # into or out of a lane source, a mode change, a symlink or submodule, # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an - # added line names still print whole. A patch that still - # shows a binary file, is larger than one review can read whole, or - # holds a line longer than Read returns whole gets no verdict: the step - # stops green with the reason as an output, the record job posts it as - # `verdict=none reason=` on the head, and the lanes stay unbound - # under that reason. Any other failure fails the step. The tree the - # verdict is keyed on comes from git, never from the model. + # added line names still print whole. A patch over the size cap is + # split into chunks of at most the cap, at file boundaries and a larger + # file's hunks (split-diff.py), and each chunk is reviewed by its own + # run; the verdict sums them. A patch that still shows a binary file, + # needs more than MAX_CHUNKS chunks, or holds a line longer than Read + # returns whole gets no verdict: the step stops green with the reason + # as an output, the record job posts it as `verdict=none + # reason=` on the head, and the lanes stay unbound under that + # reason. Any other failure fails the step. The tree the verdict is + # keyed on comes from git, never from the model. - name: Write the diff id: diff if: steps.select.outputs.review == 'true' @@ -1186,6 +1723,9 @@ jobs: # About 75k tokens of patch: room in the lane's turn budget to read # it whole beside the criteria (judgment). MAX_DIFF_BYTES: "300000" + # The most chunks, each one review: at most the leg's Review chunk + # steps (workflow.test.mjs holds them equal). + MAX_CHUNKS: "4" # One Read returns at most 25,000 tokens # (https://code.claude.com/docs/en/tools-reference#large-files), and # a token is at least a byte, so a part this size with its line @@ -1213,36 +1753,52 @@ jobs: if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then stop binary "the diff holds a binary file the review cannot read" fi - if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then - stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, over the $MAX_DIFF_BYTES one review reads whole" - fi # A diff line's text, past its +, - or space, longer than the cap. if ! LC_ALL=C awk -v max="$LONG_LINE_BYTES" 'length($0) - 1 > max { exit 3 }' "$DATA/diff.patch"; then stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi - # Parts the reviewer reads whole, each named in the prompt; the - # verdict counts only when every part was read whole - # (write-verdict.mjs `readCoverage`). A line longer than a part gets - # no verdict. + # Chunks of at most MAX_DIFF_BYTES, one review each; exit 3 is a + # diff more chunks than MAX_CHUNKS hold, or a hunk over one chunk. mkdir "$DATA/diff" - if ! LC_ALL=C awk -v dir="$DATA/diff" -v maxb="$PART_BYTES" -v maxl="$PART_LINES" ' - { len = length($0) + 1 - if (len > maxb) exit 3 - if (n == 0 || b + len > maxb || l >= maxl) { if (f) close(f); f = sprintf("%s/part-%03d.patch", dir, ++n); b = 0; l = 0 } - print > f; b += len; l++ }' "$DATA/diff.patch"; then - echo "::error::a diff line is over $PART_BYTES bytes, more than one read returns whole; no verdict, so the lanes stay unbound" + split=0 + python3 -I .base/.github/scripts/pr-review-main/split-diff.py "$DATA/diff.patch" "$DATA/diff" "$MAX_DIFF_BYTES" "$MAX_CHUNKS" || split=$? + if [ "$split" -eq 3 ]; then + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes each read whole" + elif [ "$split" -ne 0 ]; then + echo "::error::the diff did not split into chunks; no verdict, so the lanes stay unbound" exit 1 fi rm "$DATA/diff.patch" - if [ -z "$(find "$DATA/diff" -type f)" ]; then + chunks=$(find "$DATA/diff" -name 'chunk-*.patch' | wc -l) + if [ "$chunks" -eq 0 ]; then echo "::error::the diff is empty; no verdict, so the lanes stay unbound" exit 1 fi + # Each chunk's parts, which its reviewer reads whole, each named in + # its prompt; the verdict counts only when every part was read + # whole (write-verdict.mjs `readCoverage`). A line longer than a + # part gets no verdict. + bytes= + for k in $(seq "$chunks"); do + dir="$DATA/diff/chunk-$k" + mkdir "$dir" + if ! LC_ALL=C awk -v dir="$dir" -v maxb="$PART_BYTES" -v maxl="$PART_LINES" ' + { len = length($0) + 1 + if (len > maxb) exit 3 + if (n == 0 || b + len > maxb || l >= maxl) { if (f) close(f); f = sprintf("%s/part-%03d.patch", dir, ++n); b = 0; l = 0 } + print > f; b += len; l++ }' "$dir.patch"; then + echo "::error::a diff line is over $PART_BYTES bytes, more than one read returns whole; no verdict, so the lanes stay unbound" + exit 1 + fi + bytes="$bytes${bytes:+,}$(wc -c <"$dir.patch")" + rm "$dir.patch" + done tree=$(git -C "$SRC" rev-parse --verify "$HEAD_SHA^{tree}") jq -n --argjson pr "$PR_NUMBER" --arg head "$HEAD_SHA" --arg base "$BASE_SHA" --arg tree "$tree" \ '{pr: $pr, head_sha: $head, base_sha: $base, tree: $tree}' >"$DATA/pr.json" - echo "tree=$tree" >>"$GITHUB_OUTPUT" - echo "diff: $(cat "$DATA"/diff/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" + { echo "tree=$tree"; echo "chunks=$chunks"; } >>"$GITHUB_OUTPUT" + echo "::notice title=pr-review-main chunks::lane=$LANE pr=$PR_NUMBER chunks=$chunks bytes=$bytes" + echo "diff: $(cat "$DATA"/diff/chunk-*/part-*.patch | wc -l) lines in $(find "$DATA/diff" -type f | wc -l) parts" - name: Select the trusted PR text if: steps.diff.outcome == 'success' && steps.diff.outputs.stop == '' @@ -1317,7 +1873,8 @@ jobs: with: install-subprocess-isolation: true - # Files, not step outputs: run-claude-cli reads both. + # Files, not step outputs: run-claude-cli reads both. One prompt per + # chunk, naming that chunk's parts. - name: Write the prompt and schema id: prompt if: steps.credential.outputs.present == 'true' @@ -1326,23 +1883,36 @@ jobs: LANE: ${{ matrix.lane }} SKILL: ${{ matrix.skill }} MODEL: ${{ steps.models.outputs.model }} - PROMPT_FILE: ${{ runner.temp }}/pr-review-main-prompt.md + CHUNKS: ${{ steps.diff.outputs.chunks }} + PROMPT_STEM: ${{ runner.temp }}/pr-review-main-prompt SCHEMA_FILE: ${{ runner.temp }}/pr-review-main-schema.json run: | if [ -z "$MODEL" ]; then echo "::error::the review profile's model is default, which run-claude-cli cannot pass; name an alias" exit 1 fi - prompt=$(<.base/.github/scripts/pr-review-main/prompt.md) - prompt=${prompt//LANE_NAME/$LANE} - prompt=${prompt//CRITERIA_DIR/$DATA/review/skills/$SKILL} - prompt=${prompt//STANDARDS_DIR/$DATA/standards} - # shellcheck disable=SC2016 # the backticks are Markdown, not a command - parts=$(printf ' - `%s`\n' "$DATA"/diff/part-*.patch) - prompt=${prompt//DIFF_PARTS/$parts} - prompt=${prompt//CONTEXT_FILE/$DATA/context.json} - prompt=${prompt//PR_FILE/$DATA/pr.json} - printf '%s\n' "$prompt" >"$PROMPT_FILE" + if ! [[ $CHUNKS =~ ^[1-9]$ ]]; then + echo "::error::the chunk count is malformed" + exit 1 + fi + template=$(<.base/.github/scripts/pr-review-main/prompt.md) + for k in $(seq "$CHUNKS"); do + prompt=$template + prompt=${prompt//LANE_NAME/$LANE} + prompt=${prompt//CRITERIA_DIR/$DATA/review/skills/$SKILL} + prompt=${prompt//STANDARDS_DIR/$DATA/standards} + # shellcheck disable=SC2016 # the backticks are Markdown, not a command + parts=$(printf ' - `%s`\n' "$DATA/diff/chunk-$k"/part-*.patch) + prompt=${prompt//DIFF_PARTS/$parts} + note= + if [ "$CHUNKS" -gt 1 ]; then + note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries (a larger file at its hunks, each piece under the file's header lines), and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." + fi + prompt=${prompt//CHUNK_NOTE/$note} + prompt=${prompt//CONTEXT_FILE/$DATA/context.json} + prompt=${prompt//PR_FILE/$DATA/pr.json} + printf '%s\n' "$prompt" >"$PROMPT_STEM-$k.md" + done echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" printf '%s\n' '{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"severity":{"enum":["CRITICAL","IMPORTANT","SUGGESTION"]},"path":{"type":"string"},"line":{"type":"integer","minimum":0},"title":{"type":"string"},"body":{"type":"string"}},"required":["severity","path","line","title","body"],"additionalProperties":false}},"summary":{"type":"string"}},"required":["findings","summary"],"additionalProperties":false}' >"$SCHEMA_FILE" @@ -1354,8 +1924,14 @@ jobs: # the outcome report; "Fail without a verdict" carries the red. A # handoff skips the verdict and the outcome report: the next leg # reviews the lane and records it. - - name: Review the pull request - id: review + # One run per chunk, in order, each on its own prompt: chunk k runs + # only when the diff has k chunks and chunk k-1 reviewed and handed + # nothing on, so a handoff hands the whole lane on and the next leg + # reviews every chunk again. Each run leaves its own lane record and + # its sealed transcript (failed runs included, kept 7 days for the + # puller; a failed upload never fails the job). + - name: Review chunk 1 + id: review-1 if: steps.prompt.outcome == 'success' continue-on-error: true timeout-minutes: 22 @@ -1366,7 +1942,7 @@ jobs: with: lane: pr-review-main role: reviews - prompt-file: ${{ runner.temp }}/pr-review-main-prompt.md + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-1.md preamble: >- You run unattended; return only the findings the schema asks for; nothing you write is posted. @@ -1392,44 +1968,253 @@ jobs: json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json leg-timeout-minutes: "15" - # One lane record per lane and run, as pr-verify.yml keeps one, so this - # lane's model legs show in lane telemetry and spend: the leg that - # handed nothing on records it. - - name: Report the review's outcome - if: (steps.review.outcome == 'success' || steps.review.outcome == 'failure') && steps.review.outputs.handoff != 'true' + - name: Report chunk 1's outcome + if: (steps.review-1.outcome == 'success' || steps.review-1.outcome == 'failure') && steps.review-1.outputs.handoff != 'true' uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 with: - outcome: ${{ steps.review.outcome }} - execution-file: ${{ steps.review.outputs.execution-file }} - log-file: ${{ steps.review.outputs.log-file }} + outcome: ${{ steps.review-1.outcome }} + execution-file: ${{ steps.review-1.outputs.execution-file }} + log-file: ${{ steps.review-1.outputs.log-file }} lane: pr-review-main lane-id: pr-review-main-${{ matrix.lane }} step-started-at: ${{ steps.prompt.outputs.started-at }} step-timeout-minutes: 22 - credential: ${{ steps.review.outputs.credential }} + credential: ${{ steps.review-1.outputs.credential }} max-turns: ${{ steps.models.outputs.max-turns }} - known-failure-class: ${{ steps.review.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review.outputs.lane-class }} + known-failure-class: ${{ steps.review-1.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-1.outputs.lane-class }} # Why this leg ran, for the lane's one record. fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} - # The leg's transcript, failed legs included, sealed to the transcript - # key and kept 7 days for the puller; a failed upload never fails the job. - - name: Upload the sealed transcript - if: always() && steps.review.outputs.execution-file != '' + - name: Upload chunk 1's sealed transcript + if: always() && steps.review-1.outputs.execution-file != '' continue-on-error: true uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] with: - execution-file: ${{ steps.review.outputs.execution-file }} + execution-file: ${{ steps.review-1.outputs.execution-file }} public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} job-id: ${{ github.job }}-${{ matrix.lane }} + - name: Time chunk 2 + id: time-2 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 2 && steps.review-1.outcome == 'success' && steps.review-1.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 2 + id: review-2 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 2 && steps.review-1.outcome == 'success' && steps.review-1.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-2.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + fallback-token: ${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + - name: Report chunk 2's outcome + if: (steps.review-2.outcome == 'success' || steps.review-2.outcome == 'failure') && steps.review-2.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-2.outcome }} + execution-file: ${{ steps.review-2.outputs.execution-file }} + log-file: ${{ steps.review-2.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-2.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-2.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-2.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-2.outputs.lane-class }} + # Why this leg ran, for the lane's one record. + fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} + + - name: Upload chunk 2's sealed transcript + if: always() && steps.review-2.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-2.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-2 + + - name: Time chunk 3 + id: time-3 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 3 && steps.review-2.outcome == 'success' && steps.review-2.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 3 + id: review-3 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 3 && steps.review-2.outcome == 'success' && steps.review-2.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-3.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + fallback-token: ${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + - name: Report chunk 3's outcome + if: (steps.review-3.outcome == 'success' || steps.review-3.outcome == 'failure') && steps.review-3.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-3.outcome }} + execution-file: ${{ steps.review-3.outputs.execution-file }} + log-file: ${{ steps.review-3.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-3.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-3.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-3.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-3.outputs.lane-class }} + # Why this leg ran, for the lane's one record. + fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} + + - name: Upload chunk 3's sealed transcript + if: always() && steps.review-3.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-3.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-3 + + - name: Time chunk 4 + id: time-4 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 4 && steps.review-3.outcome == 'success' && steps.review-3.outputs.handoff != 'true' + run: echo "started-at=$(date +%s)" >>"$GITHUB_OUTPUT" + + - name: Review chunk 4 + id: review-4 + if: steps.prompt.outcome == 'success' && fromJSON(steps.diff.outputs.chunks || '0') >= 4 && steps.review-3.outcome == 'success' && steps.review-3.outputs.handoff != 'true' + continue-on-error: true + timeout-minutes: 22 + uses: melodic-software/ci-workflows/.github/actions/run-claude-cli@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + env: + # No work may outlive the result message the launcher reads. + CLAUDE_CODE_DISABLE_BACKGROUND_TASKS: "1" + with: + lane: pr-review-main + role: reviews + prompt-file: ${{ runner.temp }}/pr-review-main-prompt-4.md + preamble: >- + You run unattended; return only the findings the schema asks for; + nothing you write is posted. + fallback-token: ${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + - name: Report chunk 4's outcome + if: (steps.review-4.outcome == 'success' || steps.review-4.outcome == 'failure') && steps.review-4.outputs.handoff != 'true' + uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + with: + outcome: ${{ steps.review-4.outcome }} + execution-file: ${{ steps.review-4.outputs.execution-file }} + log-file: ${{ steps.review-4.outputs.log-file }} + lane: pr-review-main + lane-id: pr-review-main-${{ matrix.lane }} + step-started-at: ${{ steps.time-4.outputs.started-at }} + step-timeout-minutes: 22 + credential: ${{ steps.review-4.outputs.credential }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review-4.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review-4.outputs.lane-class }} + # Why this leg ran, for the lane's one record. + fallback-reason: ${{ matrix.reason == 'credit-stop' && 'credit-stop' || matrix.reason == 'auth' && 'auth' || matrix.reason == 'limit-stop' && 'limit-stop' || matrix.reason == 'no-key' && 'no-key' || '' }} + + - name: Upload chunk 4's sealed transcript + if: always() && steps.review-4.outputs.execution-file != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review-4.outputs.execution-file }} + public-key: ${{ vars.LANE_TRANSCRIPT_PUBLIC_KEY }} + job-id: ${{ github.job }}-${{ matrix.lane }}-4 + # The structured output is untrusted model text: read from the file and # checked by the script, never interpolated into a command. - name: Write the verdict id: verdict - if: steps.review.outcome == 'success' && steps.review.outputs.handoff != 'true' + if: steps.review-1.outcome == 'success' && (fromJSON(steps.diff.outputs.chunks || '0') < 2 || steps.review-2.outcome == 'success') && (fromJSON(steps.diff.outputs.chunks || '0') < 3 || steps.review-3.outcome == 'success') && (fromJSON(steps.diff.outputs.chunks || '0') < 4 || steps.review-4.outcome == 'success') && steps.review-1.outputs.handoff != 'true' && steps.review-2.outputs.handoff != 'true' && steps.review-3.outputs.handoff != 'true' && steps.review-4.outputs.handoff != 'true' env: - EXECUTION_FILE: ${{ steps.review.outputs.execution-file }} + # Each chunk's execution file in chunk order; it runs only when + # every chunk's review succeeded and handed nothing on. + EXECUTION_FILES: | + ${{ steps.review-1.outputs.execution-file }} + ${{ steps.review-2.outputs.execution-file }} + ${{ steps.review-3.outputs.execution-file }} + ${{ steps.review-4.outputs.execution-file }} + CHUNKS: ${{ steps.diff.outputs.chunks }} LANE: ${{ matrix.lane }} PR_NUMBER: ${{ steps.gate.outputs.pr-number }} HEAD_SHA: ${{ steps.gate.outputs.head-sha }} @@ -1439,6 +2224,21 @@ jobs: DIFF_DIR: ${{ github.workspace }}/pr-review-main-data/diff run: node .base/.github/scripts/pr-review-main/write-verdict.mjs + # A chunked review that wrote no verdict and handed nothing on (a + # chunk's review failed, or one left no result or skipped a part) is a + # designed stop too: no chunk's counts bind, and the record job names + # it on the head. The job still fails. + - name: Record a failed chunk + id: chunk-stop + if: ${{ !cancelled() && fromJSON(steps.diff.outputs.chunks || '0') > 1 && steps.review-1.outcome != 'skipped' && steps.verdict.outcome != 'success' && steps.review-1.outputs.handoff != 'true' && steps.review-2.outputs.handoff != 'true' && steps.review-3.outputs.handoff != 'true' && steps.review-4.outputs.handoff != 'true' }} + env: + LANE: ${{ matrix.lane }} + HEAD_SHA: ${{ steps.gate.outputs.head-sha }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + run: | + echo "::notice title=pr-review-main stop::reason=chunk-failed lane=$LANE pr=$PR_NUMBER; a chunk's review left no verdict, so no verdict binds and the lanes stay unbound" + { echo "$LANE-stop=chunk-failed"; echo "$LANE-head=$HEAD_SHA"; } >>"$GITHUB_OUTPUT" + - name: Upload the verdict if: steps.verdict.outcome == 'success' uses: actions/upload-artifact@cf430e030ddbb5b0abf93d22962f4752f3646cd9 # v7.0.2 @@ -1507,13 +2307,13 @@ jobs: for lane in claude-review claude-security-review; do if [ "$lane" = claude-review ]; then reason=$CODE_STOP head=$CODE_HEAD; else reason=$SECURITY_STOP head=$SECURITY_HEAD; fi [ -z "$reason" ] && continue - [[ $reason =~ ^(too-large|binary|long-line)$ && $head =~ ^[0-9a-f]{40}$ ]] || { + [[ $reason =~ ^(too-large|binary|long-line|chunk-failed)$ && $head =~ ^[0-9a-f]{40}$ ]] || { echo "::error::a stop output is malformed for $lane"; exit 1; } gh api -X POST "repos/$GITHUB_REPOSITORY/check-runs" \ -f name="pr-review-main: $lane" -f head_sha="$head" -f status=completed \ -f conclusion=neutral \ -f "output[title]=verdict=none reason=$reason" \ - -f "output[summary]=pr-review-main did not review $head from main: the diff stopped the review ($reason), so it left no verdict." >/dev/null + -f "output[summary]=pr-review-main did not review $head from main: the review stopped ($reason), so it left no verdict." >/dev/null echo "recorded $lane $head stop=$reason" done diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index c300e5fdbb..5d6744891c 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1090,10 +1090,19 @@ the rules that only tighten in does, and for a pull request whose changed files include a lane source runs both lanes on the head's diff as data, each lane's criteria and the standards copied from main, with a read-only token and a model that may only Read its data directory. The diff is `git diff --text`, so a file - git would call binary is shown as text, and a patch that still holds a binary file or is over - 300000 bytes gets no verdict. The diff is split into parts one Read returns whole (15000 bytes - and 1000 lines at most; a longer line gets no verdict), and a verdict is written only when the - run read every part whole, with no offset or limit (`write-verdict.mjs` `readCoverage`). A diff + git would call binary is shown as text, and a patch that still holds a binary file gets no + verdict. A patch over 300000 bytes is split into chunks of at most 300000 bytes, cut at file + boundaries and a larger file's hunks, each piece under the file's own header lines + (`split-diff.py`); each chunk is reviewed by its own run, in order, and the lane's one verdict + sums the chunks' findings and CRITICAL ones. A patch that needs more than 4 chunks (the diff + step's `MAX_CHUNKS`, at most the leg's review steps), or holds a hunk larger than one chunk, + gets no verdict. The diff step logs a `pr-review-main chunks` notice with the chunk count and + each chunk's bytes, and the verdict step one `pr-review-main chunk` notice per chunk with its + bytes, findings and CRITICAL ones. Each chunk is split into parts one Read returns whole (15000 + bytes and 1000 lines at most; a longer line gets no verdict), and a verdict is written only when + every chunk's run succeeded and read each of its parts whole, with no offset or limit + (`write-verdict.mjs` `readCoverage`). A chunk that hands off hands the whole lane to the next + leg, which reviews every chunk again. A diff line over 1800 bytes (`babysit_risk.LONG_LINE_BYTES`), which Read may cut short, gets no verdict: the merge gate holds a pull request only the main-run review binds (`needs_main_review`, as `bind()` decides; a read-only workflow binds through the pull_request lanes, which read the @@ -1114,12 +1123,13 @@ the rules that only tighten in `pr-review-main---`, the tree read by git, never the model, and a separate job with no model posts its counts as the check run `pr-review-main: ` (titled `findings= critical=`, conclusion neutral) on the reviewed head. A designed stop (a - binary file, a patch over the size cap or a line over 1800 bytes) leaves the leg green with no + binary file, a patch over the chunk cap or a line over 1800 bytes) leaves the leg green with no verdict, and the record job posts it on the head as the same check run titled - `verdict=none reason=`, so each stop is a countable record; it - posts nothing else. A stop counts no CRITICAL finding and never binds a lane: with no verdict - the lanes stay unbound as `main-review-stop:`, a lane's reason. A forged stop only - renames that reason. + `verdict=none reason=`, so each stop is a countable + record; it posts nothing else. `chunk-failed` is a chunked review one of whose chunks failed, + left no result or skipped a part: no chunk's counts bind, and that leg still fails. A stop + counts no CRITICAL finding and never binds a lane: with no verdict the lanes stay unbound as + `main-review-stop:`, a lane's reason. A forged stop only renames that reason. The gate (`babysit_review_evidence._main_bound`) counts a verdict only when its run is that file, event `workflow_run`, on the default branch of this repository with a head the branch holds, and the verdict names that lane, pull request and tree. As with pr-verify's FAIL, a diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 08d1a433b1..eef4bc88bd 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -2476,9 +2476,12 @@ def read(path: str, binary: bool = False) -> Any: # The check run pr-review-main.yml's record job posts per lane on each head it # reviewed, titled with its counts, or with the designed stop that left that -# head without a verdict (a diff the review cannot read whole). +# head without a verdict (a diff the review cannot read whole, or a chunked +# review one of whose chunks left no verdict). MAIN_REVIEW_CHECK_TITLE = re.compile(r"findings=(\d{1,6}) critical=(\d{1,6})") -MAIN_REVIEW_STOP_TITLE = re.compile(r"verdict=none reason=(too-large|binary|long-line)") +MAIN_REVIEW_STOP_TITLE = re.compile( + r"verdict=none reason=(too-large|binary|long-line|chunk-failed)" +) # One record: (findings, CRITICAL), ("stop", reason), or None when unread. MainReviewRecord = tuple[int, int] | tuple[str, str] | None diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py index ae68b3ece4..16e7bde933 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh.py @@ -1369,6 +1369,15 @@ def test_a_stop_title_reads_as_the_stop_and_its_reason(self) -> None: }, ) + def test_a_failed_chunk_reads_as_a_stop_that_counts_no_critical(self) -> None: + # A chunked main-run review whose chunk left no verdict records + # `verdict=none reason=chunk-failed`, a designed stop like the others. + title = "verdict=none reason=chunk-failed" + counts = self.counts({"a" * 40: [self.check("claude-review", title)]}) + self.assertEqual(counts["a" * 40]["claude-review"], [("stop", "chunk-failed")]) + found = self.collect({"a" * 40: [self.check("claude-review", title)]}) + self.assertEqual(found["claude-review"], 0) + def test_a_stop_counts_no_critical_and_an_unknown_reason_counts_one(self) -> None: found = self.collect( { From c6af84f3cfe434dd770a9ad5fdf075ce3ff3de24 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 30 Oct 2026 02:52:43 -0400 Subject: [PATCH 2/3] fix(pr-pipeline): keep each file whole in a main-review chunk and take the max CRITICAL Issue #11000 says to split an oversize diff by file, keep each file whole, and stop a single file over the cap as too-large; the max CRITICAL count across chunks wins. split-diff.py no longer cuts a file at its hunks: a file over max-bytes raises TooLarge (exit 3). buildChunkedVerdict reports critical as the most any one chunk found and keeps findings as the sum. The chunk note, workflow comments, docs and changelog fragment say so. Co-Authored-By: Claude Opus 5.5 --- .../feat-main-review-chunked-3b8e51d2.md | 16 +++---- .github/scripts/pr-review-main/split-diff.py | 40 ++++------------ .../scripts/pr-review-main/workflow.test.mjs | 28 +++++++---- .../scripts/pr-review-main/write-verdict.mjs | 14 +++--- .../pr-review-main/write-verdict.test.mjs | 16 ++++--- .github/workflows/pr-review-main.yml | 48 ++++++++++--------- docs/conventions/pr-pipeline/README.md | 14 +++--- 7 files changed, 85 insertions(+), 91 deletions(-) diff --git a/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md index 838c3b5d9a..02d6a404f3 100644 --- a/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md +++ b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md @@ -5,11 +5,11 @@ bump: minor ### Changed - pr-review-main reviews a diff over its 300000-byte cap in chunks instead of stopping. The diff - splits into at most 4 chunks of at most 300000 bytes, cut at file boundaries and a larger - file's hunks (each piece under the file's header lines), and each chunk is reviewed by its own - run. The lane's one verdict sums the chunks' findings and CRITICAL ones and binds as a - single-pass verdict does. A `pr-review-main chunks` notice gives the chunk count and each - chunk's bytes, and a `pr-review-main chunk` notice per chunk its bytes, findings and CRITICAL - ones. A diff that needs more than 4 chunks, or holds a hunk larger than one, still stops as - `too-large`. A chunk that fails, leaves no result or skips a part writes no verdict: the record - job posts `verdict=none reason=chunk-failed` on the head and the leg fails. + splits into at most 4 chunks of at most 300000 bytes, cut at file boundaries with each file + whole, and each chunk is reviewed by its own run. The lane's one verdict sums the chunks' + findings, takes the most CRITICAL ones any one chunk found, and binds as a single-pass verdict + does. A `pr-review-main chunks` notice gives the chunk count and each chunk's bytes, and a + `pr-review-main chunk` notice per chunk its bytes, findings and CRITICAL ones. A diff that holds + a single file over 300000 bytes, or needs more than 4 chunks, still stops as `too-large`. A + chunk that fails, leaves no result or skips a part writes no verdict: the record job posts + `verdict=none reason=chunk-failed` on the head and the leg fails. diff --git a/.github/scripts/pr-review-main/split-diff.py b/.github/scripts/pr-review-main/split-diff.py index 06ed95d5fb..d037965913 100644 --- a/.github/scripts/pr-review-main/split-diff.py +++ b/.github/scripts/pr-review-main/split-diff.py @@ -3,12 +3,10 @@ usage: split-diff.py Writes /chunk-.patch, k from 1, each at most max-bytes, the -patch's bytes in order and nothing else. A chunk ends at a file boundary -(a `diff --git` line). A file over max-bytes splits at its hunks (`@@` lines), -each piece led by the file's own header lines, so every chunk reads as a -patch. A hunk that does not fit one chunk beside its header, or a patch that -needs more than max-chunks chunks, is too large: nothing is written and the -exit status is 3. An empty patch writes no chunk. +patch's bytes in order and nothing else. A chunk ends at a file boundary (a +`diff --git` line), and a file is never split: a file over max-bytes, or a +patch that needs more than max-chunks chunks, is too large: nothing is +written and the exit status is 3. An empty patch writes no chunk. """ import sys @@ -23,36 +21,12 @@ class TooLarge(Exception): def lines_of(data: bytes) -> list[bytes]: """Each line with its newline; a content line always starts with a - prefix byte, so a line that starts `diff --git ` or `@@ ` is git's.""" + prefix byte, so a line that starts `diff --git ` is git's.""" lines = [line + b"\n" for line in data.split(b"\n")] lines[-1] = lines[-1][:-1] return [line for line in lines if line] -def pieces(lines: list[bytes], max_bytes: int) -> list[bytes]: - """The file patch whole, or split at its hunks under its header.""" - whole = b"".join(lines) - if len(whole) <= max_bytes: - return [whole] - start = next((i for i, line in enumerate(lines) if line.startswith(b"@@ ")), None) - if start is None: - raise TooLarge("a file with no hunk is larger than one chunk") - header = b"".join(lines[:start]) - hunks: list[bytes] = [] - for line in lines[start:]: - if line.startswith(b"@@ "): - hunks.append(b"") - hunks[-1] += line - found = [header] - for hunk in hunks: - if len(header) + len(hunk) > max_bytes: - raise TooLarge("a hunk with its file's header is larger than one chunk") - if found[-1] != header and len(found[-1]) + len(hunk) > max_bytes: - found.append(header) - found[-1] += hunk - return found - - def split(data: bytes, max_bytes: int, max_chunks: int) -> list[bytes]: files: list[list[bytes]] = [] for line in lines_of(data): @@ -60,7 +34,9 @@ def split(data: bytes, max_bytes: int, max_chunks: int) -> list[bytes]: files.append([]) files[-1].append(line) chunks: list[bytes] = [] - for unit in (piece for lines in files for piece in pieces(lines, max_bytes)): + for unit in (b"".join(lines) for lines in files): + if len(unit) > max_bytes: + raise TooLarge("a file is larger than one chunk") if not chunks or len(chunks[-1]) + len(unit) > max_bytes: chunks.append(b"") chunks[-1] += unit diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index 7136d5592b..1daead7896 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -228,17 +228,18 @@ test("an oversize diff splits into chunks under the cap at file boundaries, in o assert.deepEqual(split(a + b + c, 180, 4).chunks, { "chunk-1.patch": a + b + c }); }); -test("a single file over the cap splits at its hunks, each piece under its own header", () => { +test("a single file over the cap is too large and writes nothing, however small its hunks", () => { const [h1, h2, h3] = [hunkPatch(1, 30), hunkPatch(5, 30), hunkPatch(9, 30)]; assert.equal(BIG.length, 43); - // 43 + 30 + 30 = 103 fits 110; the third hunk goes with the header again. + // 43 + 3 * 30 = 133 is over 110: each file stays whole, so it fits no chunk. const result = split(BIG + h1 + h2 + h3, 110, 4); - assert.equal(result.status, 0, result.stderr); - assert.deepEqual(result.chunks, { "chunk-1.patch": BIG + h1 + h2, "chunk-2.patch": BIG + h3 }); - // A file after it packs beside the last piece when it fits: at 120 the - // pieces are the same (133 > 120), and 43 + 30 + 40 = 113 fits. + assert.equal(result.status, 3); + assert.deepEqual(result.chunks, {}); + assert.match(result.stderr, /a file is larger than one chunk/); + // Beside a small file it is still too large; at 133 it is one whole chunk. const small = filePatch("s", 40); - assert.deepEqual(split(BIG + h1 + h2 + h3 + small, 120, 4).chunks, { "chunk-1.patch": BIG + h1 + h2, "chunk-2.patch": BIG + h3 + small }); + assert.equal(split(BIG + h1 + h2 + h3 + small, 120, 4).status, 3); + assert.deepEqual(split(BIG + h1 + h2 + h3 + small, 133, 4).chunks, { "chunk-1.patch": BIG + h1 + h2 + h3, "chunk-2.patch": small }); }); test("a chunk of exactly the cap, and exactly the most chunks, still split", () => { @@ -247,7 +248,7 @@ test("a chunk of exactly the cap, and exactly the most chunks, still split", () assert.deepEqual(split(a + b, 100, 2).chunks, { "chunk-1.patch": a, "chunk-2.patch": b }); }); -test("more chunks than the most, or a hunk larger than one chunk, is too large and writes nothing", () => { +test("more chunks than the most, or a file larger than one chunk, is too large and writes nothing", () => { const [a, b, c] = [filePatch("a", 100), filePatch("b", 100), filePatch("c", 100)]; const over = split(a + b + c, 100, 2); assert.equal(over.status, 3); @@ -297,12 +298,21 @@ function runDiff(patch, { pythonFails = false, job = KEY, maxBytes = "300", maxC test("a diff the review cannot read whole stops green and names its reason for the record job", () => { const small = "diff --git a/x b/x\n@@ -1 +1 @@\n+short\n"; for (const job of LEGS) { - // One hunk of 612 bytes fits no 300-byte chunk. + // One file of 612 bytes fits no 300-byte chunk. const tooLarge = runDiff(`diff --git a/x b/x\n@@ -1 +1 @@\n${"+line\n".repeat(100)}`, { job }); assert.equal(tooLarge.status, 0, job); assert.equal(tooLarge.output, `stop=too-large\nclaude-security-review-stop=too-large\nclaude-security-review-head=${SHA_A}\n`, job); assert.deepEqual(tooLarge.parts, [], job); assert.match(tooLarge.stdout, /^::notice title=pr-review-main stop::reason=too-large lane=claude-security-review pr=7; /m, job); + // One file over the cap in many small hunks is still too large: a file + // is never split at its hunks. + const hunks = Array.from({ length: 12 }, (_, i) => `@@ -${i * 10 + 1} +${i * 10 + 1} @@\n+line ${i}\n`).join(""); + const oneFile = `diff --git a/x b/x\n--- a/x\n+++ b/x\n${hunks}`; + assert.ok(Buffer.byteLength(oneFile) > 300 && hunks.length / 12 < 30, job); + const manyHunks = runDiff(oneFile, { job }); + assert.equal(manyHunks.status, 0, job); + assert.equal(manyHunks.output, `stop=too-large\nclaude-security-review-stop=too-large\nclaude-security-review-head=${SHA_A}\n`, job); + assert.deepEqual(manyHunks.parts, [], job); // Five 200-byte files need five 300-byte chunks, one more than the four. const files = ["a", "b", "c", "d", "e"].map((name) => filePatch(name, 200)); const tooMany = runDiff(files.join(""), { job }); diff --git a/.github/scripts/pr-review-main/write-verdict.mjs b/.github/scripts/pr-review-main/write-verdict.mjs index 3e1e604c18..f6b42557b2 100644 --- a/.github/scripts/pr-review-main/write-verdict.mjs +++ b/.github/scripts/pr-review-main/write-verdict.mjs @@ -3,9 +3,10 @@ // verdict file that // pr-review-main.yml uploads as pr-review-main--- // (babysit_review_evidence.main_review_name). The gate counts only its -// numbers: `findings`, every finding the model returned, and `critical`, the -// CRITICAL ones, each summed over the chunks. The pull request, head, base and tree come from the job's -// own reads, never from the model. A run with any permission denial, output +// numbers: `findings`, every finding the model returned, summed over the +// chunks, and `critical`, the CRITICAL ones, the most in any one chunk. The +// pull request, head, base and tree come from the job's own reads, never from +// the model. A run with any permission denial, output // that is not {findings, summary}, a finding that is not // {severity, path, line, title, body}, or model text a credential pattern // matches (lib/publish-gate.mjs findSecret) writes nothing and throws, so the @@ -164,8 +165,9 @@ export function buildVerdict({ output, lane, pr, headSha, baseSha, tree, runId, } // One verdict over the chunks a diff over one review's size cap was split -// into, in chunk order: each chunk's output is checked as a whole review's, -// and the counts are the sums. One chunk is the single-pass verdict. +// into, in chunk order: each chunk's output is checked as a whole review's; +// `findings` is the sum and `critical` the most any one chunk found. +// One chunk is the single-pass verdict. export function buildChunkedVerdict({ outputs, ...rest }) { if (!Array.isArray(outputs) || outputs.length === 0) { throw new Error("there is no chunk output"); @@ -183,7 +185,7 @@ export function buildChunkedVerdict({ outputs, ...rest }) { return { ...chunks[0], findings: chunks.reduce((sum, c) => sum + c.findings, 0), - critical: chunks.reduce((sum, c) => sum + c.critical, 0), + critical: Math.max(...chunks.map((c) => c.critical)), summary: chunks.map((c, i) => `Chunk ${i + 1} of ${chunks.length}: ${c.summary}`).join("\n\n"), items: chunks.flatMap((c) => c.items), }; diff --git a/.github/scripts/pr-review-main/write-verdict.test.mjs b/.github/scripts/pr-review-main/write-verdict.test.mjs index c15f397dd8..f456087ab2 100644 --- a/.github/scripts/pr-review-main/write-verdict.test.mjs +++ b/.github/scripts/pr-review-main/write-verdict.test.mjs @@ -110,16 +110,18 @@ test("the cap is the merge gate's", () => { assert.equal(Number(python.stdout.trim()), LONG_LINE_BYTES); }); -test("a chunked review's verdict sums every chunk's findings and CRITICAL ones", () => { +test("a chunked review's verdict sums every chunk's findings and takes the most CRITICAL ones in a chunk", () => { const { output: _, ...rest } = args(undefined); + // The chunk with the most CRITICAL findings comes first: the max wins, not + // the last chunk or the sum. const verdict = buildChunkedVerdict({ ...rest, outputs: [ - { findings: [finding("CRITICAL"), finding("IMPORTANT")], summary: "first" }, - { findings: [finding("CRITICAL"), finding("CRITICAL"), finding("SUGGESTION")], summary: "second" }, + { findings: [finding("CRITICAL"), finding("CRITICAL"), finding("SUGGESTION")], summary: "first" }, + { findings: [finding("CRITICAL"), finding("IMPORTANT")], summary: "second" }, ], }); - assert.deepEqual({ findings: verdict.findings, critical: verdict.critical, items: verdict.items.length }, { findings: 5, critical: 3, items: 5 }); + assert.deepEqual({ findings: verdict.findings, critical: verdict.critical, items: verdict.items.length }, { findings: 5, critical: 2, items: 5 }); assert.equal(verdict.summary, "Chunk 1 of 2: first\n\nChunk 2 of 2: second"); assert.deepEqual(Object.keys(verdict).sort(), Object.keys(buildVerdict(args({ findings: [], summary: "s" }))).sort()); // One chunk is the single-pass verdict, unchanged. @@ -181,7 +183,7 @@ test("main writes verdict.json and the name output", () => { } }); -test("a chunked run writes one verdict summed over its chunks and a notice per chunk", () => { +test("a chunked run writes one verdict over its chunks, the most CRITICAL in a chunk, and a notice per chunk", () => { const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-")); try { // Chunk 1 is 3 + 4 bytes with one CRITICAL; chunk 2 is 5 bytes with two @@ -192,10 +194,10 @@ test("a chunked run writes one verdict summed over its chunks and a notice per c const run = spawnSync(process.execPath, [script], { encoding: "utf8", env: { ...process.env, ...env } }); assert.equal(run.status, 0, run.stdout + run.stderr); const verdict = JSON.parse(readFileSync(path.join(dir, "verdict", "verdict.json"), "utf8")); - assert.deepEqual({ findings: verdict.findings, critical: verdict.critical }, { findings: 4, critical: 3 }); + assert.deepEqual({ findings: verdict.findings, critical: verdict.critical }, { findings: 4, critical: 2 }); assert.match(run.stdout, /^::notice title=pr-review-main chunk::lane=claude-review chunk=1\/2 bytes=7 findings=1 critical=1$/m); assert.match(run.stdout, /^::notice title=pr-review-main chunk::lane=claude-review chunk=2\/2 bytes=5 findings=3 critical=2$/m); - assert.match(readFileSync(env.GITHUB_OUTPUT, "utf8"), /^findings=4\ncritical=3$/m); + assert.match(readFileSync(env.GITHUB_OUTPUT, "utf8"), /^findings=4\ncritical=2$/m); } finally { rmSync(dir, { recursive: true, force: true }); } diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 39e1ffbe6a..7a4ce8463d 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -12,7 +12,8 @@ name: pr-review-main # code runs here: the diff, the trusted pull request text and the lane's # criteria (copied from main) are data files in pr-review-main-data, and the # model reads them with the Read tool alone. A diff over one review's size -# cap is reviewed in chunks, one run each, and the verdict sums them. Each +# cap is reviewed in chunks of whole files, one run each, and one verdict +# sums their findings and takes the most CRITICAL ones in a chunk. Each # lane uploads its verdict as # the artifact pr-review-main--- (write-verdict.mjs); only a # run's own steps can upload its artifacts, so the gate authenticates the @@ -197,10 +198,11 @@ jobs: # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an # added line names still print whole. A patch over the size cap is - # split into chunks of at most the cap, at file boundaries and a larger - # file's hunks (split-diff.py), and each chunk is reviewed by its own - # run; the verdict sums them. A patch that still shows a binary file, - # needs more than MAX_CHUNKS chunks, or holds a line longer than Read + # split into chunks of at most the cap at file boundaries, each file + # whole (split-diff.py), and each chunk is reviewed by its own run; the + # verdict sums their findings and takes the most CRITICAL ones in a + # chunk. A patch that still shows a binary file, holds a file over the + # cap, needs more than MAX_CHUNKS chunks, or holds a line longer than Read # returns whole gets no verdict: the step stops green with the reason # as an output, the record job posts it as `verdict=none # reason=` on the head, and the lanes stay unbound under that @@ -254,12 +256,12 @@ jobs: stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi # Chunks of at most MAX_DIFF_BYTES, one review each; exit 3 is a - # diff more chunks than MAX_CHUNKS hold, or a hunk over one chunk. + # file over one chunk, or a diff more chunks than MAX_CHUNKS hold. mkdir "$DATA/diff" split=0 python3 -I .base/.github/scripts/pr-review-main/split-diff.py "$DATA/diff.patch" "$DATA/diff" "$MAX_DIFF_BYTES" "$MAX_CHUNKS" || split=$? if [ "$split" -eq 3 ]; then - stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes each read whole" + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes: a file over $MAX_DIFF_BYTES bytes, or more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes of whole files" elif [ "$split" -ne 0 ]; then echo "::error::the diff did not split into chunks; no verdict, so the lanes stay unbound" exit 1 @@ -402,7 +404,7 @@ jobs: prompt=${prompt//DIFF_PARTS/$parts} note= if [ "$CHUNKS" -gt 1 ]; then - note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries (a larger file at its hunks, each piece under the file's header lines), and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." + note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries, each file whole, and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." fi prompt=${prompt//CHUNK_NOTE/$note} prompt=${prompt//CONTEXT_FILE/$DATA/context.json} @@ -947,10 +949,11 @@ jobs: # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an # added line names still print whole. A patch over the size cap is - # split into chunks of at most the cap, at file boundaries and a larger - # file's hunks (split-diff.py), and each chunk is reviewed by its own - # run; the verdict sums them. A patch that still shows a binary file, - # needs more than MAX_CHUNKS chunks, or holds a line longer than Read + # split into chunks of at most the cap at file boundaries, each file + # whole (split-diff.py), and each chunk is reviewed by its own run; the + # verdict sums their findings and takes the most CRITICAL ones in a + # chunk. A patch that still shows a binary file, holds a file over the + # cap, needs more than MAX_CHUNKS chunks, or holds a line longer than Read # returns whole gets no verdict: the step stops green with the reason # as an output, the record job posts it as `verdict=none # reason=` on the head, and the lanes stay unbound under that @@ -1004,12 +1007,12 @@ jobs: stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi # Chunks of at most MAX_DIFF_BYTES, one review each; exit 3 is a - # diff more chunks than MAX_CHUNKS hold, or a hunk over one chunk. + # file over one chunk, or a diff more chunks than MAX_CHUNKS hold. mkdir "$DATA/diff" split=0 python3 -I .base/.github/scripts/pr-review-main/split-diff.py "$DATA/diff.patch" "$DATA/diff" "$MAX_DIFF_BYTES" "$MAX_CHUNKS" || split=$? if [ "$split" -eq 3 ]; then - stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes each read whole" + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes: a file over $MAX_DIFF_BYTES bytes, or more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes of whole files" elif [ "$split" -ne 0 ]; then echo "::error::the diff did not split into chunks; no verdict, so the lanes stay unbound" exit 1 @@ -1152,7 +1155,7 @@ jobs: prompt=${prompt//DIFF_PARTS/$parts} note= if [ "$CHUNKS" -gt 1 ]; then - note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries (a larger file at its hunks, each piece under the file's header lines), and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." + note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries, each file whole, and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." fi prompt=${prompt//CHUNK_NOTE/$note} prompt=${prompt//CONTEXT_FILE/$DATA/context.json} @@ -1701,10 +1704,11 @@ jobs: # a move that may run or load (an executable, a skills, agents, # commands or hooks path, a file a tool reads by name), and a move an # added line names still print whole. A patch over the size cap is - # split into chunks of at most the cap, at file boundaries and a larger - # file's hunks (split-diff.py), and each chunk is reviewed by its own - # run; the verdict sums them. A patch that still shows a binary file, - # needs more than MAX_CHUNKS chunks, or holds a line longer than Read + # split into chunks of at most the cap at file boundaries, each file + # whole (split-diff.py), and each chunk is reviewed by its own run; the + # verdict sums their findings and takes the most CRITICAL ones in a + # chunk. A patch that still shows a binary file, holds a file over the + # cap, needs more than MAX_CHUNKS chunks, or holds a line longer than Read # returns whole gets no verdict: the step stops green with the reason # as an output, the record job posts it as `verdict=none # reason=` on the head, and the lanes stay unbound under that @@ -1758,12 +1762,12 @@ jobs: stop long-line "a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole" fi # Chunks of at most MAX_DIFF_BYTES, one review each; exit 3 is a - # diff more chunks than MAX_CHUNKS hold, or a hunk over one chunk. + # file over one chunk, or a diff more chunks than MAX_CHUNKS hold. mkdir "$DATA/diff" split=0 python3 -I .base/.github/scripts/pr-review-main/split-diff.py "$DATA/diff.patch" "$DATA/diff" "$MAX_DIFF_BYTES" "$MAX_CHUNKS" || split=$? if [ "$split" -eq 3 ]; then - stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes, more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes each read whole" + stop too-large "the diff is $(wc -c <"$DATA/diff.patch") bytes: a file over $MAX_DIFF_BYTES bytes, or more than $MAX_CHUNKS reviews of $MAX_DIFF_BYTES bytes of whole files" elif [ "$split" -ne 0 ]; then echo "::error::the diff did not split into chunks; no verdict, so the lanes stay unbound" exit 1 @@ -1906,7 +1910,7 @@ jobs: prompt=${prompt//DIFF_PARTS/$parts} note= if [ "$CHUNKS" -gt 1 ]; then - note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries (a larger file at its hunks, each piece under the file's header lines), and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." + note=" The diff is larger than one review reads whole, so it is split into $CHUNKS chunks at file boundaries, each file whole, and each chunk gets its own review. These parts are chunk $k of $CHUNKS: here the whole diff means these parts, and the other chunks are not yours to judge." fi prompt=${prompt//CHUNK_NOTE/$note} prompt=${prompt//CONTEXT_FILE/$DATA/context.json} diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 79bef64af0..123f6947a1 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1096,13 +1096,13 @@ the rules that only tighten in token and a model that may only Read its data directory. The diff is `git diff --text`, so a file git would call binary is shown as text, and a patch that still holds a binary file gets no verdict. A patch over 300000 bytes is split into chunks of at most 300000 bytes, cut at file - boundaries and a larger file's hunks, each piece under the file's own header lines - (`split-diff.py`); each chunk is reviewed by its own run, in order, and the lane's one verdict - sums the chunks' findings and CRITICAL ones. A patch that needs more than 4 chunks (the diff - step's `MAX_CHUNKS`, at most the leg's review steps), or holds a hunk larger than one chunk, - gets no verdict. The diff step logs a `pr-review-main chunks` notice with the chunk count and - each chunk's bytes, and the verdict step one `pr-review-main chunk` notice per chunk with its - bytes, findings and CRITICAL ones. Each chunk is split into parts one Read returns whole (15000 + boundaries with each file whole (`split-diff.py`); each chunk is reviewed by its own run, in + order, and the lane's one verdict sums the chunks' findings and takes the most CRITICAL ones + any one chunk found. A patch that holds a single file over 300000 bytes, or needs more than 4 + chunks (the diff step's `MAX_CHUNKS`, at most the leg's review steps), gets no verdict. The + diff step logs a `pr-review-main chunks` notice with the chunk count and each chunk's bytes, + and the verdict step one `pr-review-main chunk` notice per chunk with its bytes, findings and + CRITICAL ones. Each chunk is split into parts one Read returns whole (15000 bytes and 1000 lines at most; a longer line gets no verdict), and a verdict is written only when every chunk's run succeeded and read each of its parts whole, with no offset or limit (`write-verdict.mjs` `readCoverage`). A chunk that hands off hands the whole lane to the next From d40517b8b162aaa40076a6b713ee5f712036e694 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 30 Oct 2026 03:52:43 -0400 Subject: [PATCH 3/3] fix(pr-pipeline): name each chunked finding's chunk and state the multi-file over-cap stop A chunked main-run verdict's `items` listed every chunk's findings while `critical` took the most CRITICAL in any one chunk, so the listed CRITICAL items could outnumber the count with nothing to tell them apart. Each chunked item now carries its 1-based `chunk`; the gate still reads only the counts. The convention and changelog fragment now state that a multi-file diff over MAX_CHUNKS x 300000 bytes (about 1.2 MB) still stops as too-large, and point at #11000 for the dynamic chunk count that would lift it. Refs: #11000 Co-Authored-By: Claude Opus 5.5 --- .../feat-main-review-chunked-3b8e51d2.md | 5 ++++- .github/scripts/pr-review-main/write-verdict.mjs | 9 ++++++--- .../scripts/pr-review-main/write-verdict.test.mjs | 15 +++++++++++++++ docs/conventions/pr-pipeline/README.md | 8 ++++++-- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md index 02d6a404f3..0ccbf95425 100644 --- a/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md +++ b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md @@ -10,6 +10,9 @@ bump: minor findings, takes the most CRITICAL ones any one chunk found, and binds as a single-pass verdict does. A `pr-review-main chunks` notice gives the chunk count and each chunk's bytes, and a `pr-review-main chunk` notice per chunk its bytes, findings and CRITICAL ones. A diff that holds - a single file over 300000 bytes, or needs more than 4 chunks, still stops as `too-large`. A + a single file over 300000 bytes, or needs more than 4 chunks (a multi-file diff over about + 1.2 MB), still stops as `too-large`; lifting that needs a dynamic chunk count + ([#11000](https://github.com/melodic-software/claude-code-plugins/issues/11000)). Each listed + finding names its chunk, so the listed CRITICAL findings can outnumber the `critical` count. A chunk that fails, leaves no result or skips a part writes no verdict: the record job posts `verdict=none reason=chunk-failed` on the head and the leg fails. diff --git a/.github/scripts/pr-review-main/write-verdict.mjs b/.github/scripts/pr-review-main/write-verdict.mjs index f6b42557b2..17b2250396 100644 --- a/.github/scripts/pr-review-main/write-verdict.mjs +++ b/.github/scripts/pr-review-main/write-verdict.mjs @@ -4,7 +4,8 @@ // pr-review-main.yml uploads as pr-review-main--- // (babysit_review_evidence.main_review_name). The gate counts only its // numbers: `findings`, every finding the model returned, summed over the -// chunks, and `critical`, the CRITICAL ones, the most in any one chunk. The +// chunks, and `critical`, the CRITICAL ones, the most in any one chunk +// (`items` lists them all, each chunked one with its `chunk`). The // pull request, head, base and tree come from the job's own reads, never from // the model. A run with any permission denial, output // that is not {findings, summary}, a finding that is not @@ -166,7 +167,9 @@ export function buildVerdict({ output, lane, pr, headSha, baseSha, tree, runId, // One verdict over the chunks a diff over one review's size cap was split // into, in chunk order: each chunk's output is checked as a whole review's; -// `findings` is the sum and `critical` the most any one chunk found. +// `findings` is the sum and `critical` the most any one chunk found. `items` +// lists every chunk's findings, each with its 1-based `chunk`, so it can hold +// more CRITICAL items than `critical`; the gate reads only the counts. // One chunk is the single-pass verdict. export function buildChunkedVerdict({ outputs, ...rest }) { if (!Array.isArray(outputs) || outputs.length === 0) { @@ -187,7 +190,7 @@ export function buildChunkedVerdict({ outputs, ...rest }) { findings: chunks.reduce((sum, c) => sum + c.findings, 0), critical: Math.max(...chunks.map((c) => c.critical)), summary: chunks.map((c, i) => `Chunk ${i + 1} of ${chunks.length}: ${c.summary}`).join("\n\n"), - items: chunks.flatMap((c) => c.items), + items: chunks.flatMap((c, i) => c.items.map((item) => ({ ...item, chunk: i + 1 }))), }; } diff --git a/.github/scripts/pr-review-main/write-verdict.test.mjs b/.github/scripts/pr-review-main/write-verdict.test.mjs index f456087ab2..dd540f0b74 100644 --- a/.github/scripts/pr-review-main/write-verdict.test.mjs +++ b/.github/scripts/pr-review-main/write-verdict.test.mjs @@ -123,6 +123,21 @@ test("a chunked review's verdict sums every chunk's findings and takes the most }); assert.deepEqual({ findings: verdict.findings, critical: verdict.critical, items: verdict.items.length }, { findings: 5, critical: 2, items: 5 }); assert.equal(verdict.summary, "Chunk 1 of 2: first\n\nChunk 2 of 2: second"); + // `items` lists 3 CRITICAL findings while `critical` is 2: each item names + // its chunk, so the CRITICAL ones `critical` counts are the ones in one chunk. + assert.deepEqual( + verdict.items.map((item) => [item.chunk, item.severity]), + [ + [1, "CRITICAL"], + [1, "CRITICAL"], + [1, "SUGGESTION"], + [2, "CRITICAL"], + [2, "IMPORTANT"], + ], + ); + for (let chunk = 1; chunk <= 2; chunk++) { + assert.ok(verdict.items.filter((item) => item.chunk === chunk && item.severity === "CRITICAL").length <= verdict.critical); + } assert.deepEqual(Object.keys(verdict).sort(), Object.keys(buildVerdict(args({ findings: [], summary: "s" }))).sort()); // One chunk is the single-pass verdict, unchanged. const one = { findings: [finding("CRITICAL")], summary: "s" }; diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 84f465c9ee..6e0c65ad72 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1106,8 +1106,12 @@ the rules that only tighten in verdict. A patch over 300000 bytes is split into chunks of at most 300000 bytes, cut at file boundaries with each file whole (`split-diff.py`); each chunk is reviewed by its own run, in order, and the lane's one verdict sums the chunks' findings and takes the most CRITICAL ones - any one chunk found. A patch that holds a single file over 300000 bytes, or needs more than 4 - chunks (the diff step's `MAX_CHUNKS`, at most the leg's review steps), gets no verdict. The + any one chunk found; the verdict's `items` give each finding its `chunk`, so they can hold more + CRITICAL items than its `critical` count, which alone binds. A patch that holds a single file + over 300000 bytes, or needs more than 4 chunks (the diff step's `MAX_CHUNKS`, at most the leg's + review steps), gets no verdict: a multi-file diff over about 1.2 MB still stops as `too-large` + until a dynamic chunk count lifts it + ([#11000](https://github.com/melodic-software/claude-code-plugins/issues/11000)). The diff step logs a `pr-review-main chunks` notice with the chunk count and each chunk's bytes, and the verdict step one `pr-review-main chunk` notice per chunk with its bytes, findings and CRITICAL ones. Each chunk is split into parts one Read returns whole (15000