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..0ccbf95425 --- /dev/null +++ b/.changes/pr-pipeline/feat-main-review-chunked-3b8e51d2.md @@ -0,0 +1,18 @@ +--- +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 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 (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/.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 eba284de79..c7fd570983 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..d037965913 --- /dev/null +++ b/.github/scripts/pr-review-main/split-diff.py @@ -0,0 +1,56 @@ +"""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), 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 +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 ` 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 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 (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 + 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..1daead7896 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,82 @@ 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 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 + 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, 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.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", () => { + 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 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); + 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 +272,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 +298,35 @@ 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 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 }); + 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 +338,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 +350,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 +378,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 +391,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 +606,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 +660,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 +669,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 +688,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 +742,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 +766,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 +866,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 +879,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 +921,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..17b2250396 100644 --- a/.github/scripts/pr-review-main/write-verdict.mjs +++ b/.github/scripts/pr-review-main/write-verdict.mjs @@ -1,14 +1,18 @@ -// 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 -// 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 +// (`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 // {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 +165,103 @@ 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. `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) { + 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: 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, i) => c.items.map((item) => ({ ...item, chunk: i + 1 }))), + }; +} + +// 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..dd540f0b74 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,141 @@ 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 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("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: 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" }; + 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 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 + // 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: 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=2$/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 8ee8979a85..7a4ce8463d 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -11,7 +11,10 @@ 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 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 # name through this run's record (path, workflow_run event, main, completed @@ -85,7 +88,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 +101,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 +197,17 @@ 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, 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 + # 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 +221,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 +251,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 + # file over one chunk, or a diff more chunks than MAX_CHUNKS hold. 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: 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 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 +371,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 +381,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, 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} + 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 +422,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 +440,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 +466,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 +730,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 +840,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 +853,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 +948,17 @@ 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, 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 + # 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 +972,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 +1002,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 + # file over one chunk, or a diff more chunks than MAX_CHUNKS hold. 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: 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 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 +1122,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 +1132,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, 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} + 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 +1173,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 +1191,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 +1218,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 +1599,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 +1608,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 +1703,17 @@ 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, 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 + # 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 +1727,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 +1757,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 + # file over one chunk, or a diff more chunks than MAX_CHUNKS hold. 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: 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 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 +1877,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 +1887,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, 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} + 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 +1928,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 +1946,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 +1972,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 +2228,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 +2311,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 fd07666295..6e0c65ad72 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1102,10 +1102,23 @@ 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 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; 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 + 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 @@ -1126,12 +1139,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( {