fix(pr-pipeline): verify a diff of up to 12 chunks instead of stopping at 4

.changes/pr-pipeline/fix-pr-verify-large-diffs-5c1e8a37.md

@@ -0,0 +1,11 @@
+---
+bump: patch
+---
+
+### Fixed
+
+- `pr-verify` judges a diff of any realistic size. It splits a diff into as many chunks of at most
+  300000 bytes as it takes, up to 12 (`MAX_CHUNKS` in `chunk-diff.sh`, which holds a 3 MB diff), each
+  with its own judge step, and the head still passes only when every chunk and the final judge do. A
+  file over 300000 bytes, or a diff that needs more than 12 chunks, still ends the run with no
+  verdict and an error naming the file, or the diff's size and the chunk count.

.github/scripts/pr-verify/chunk-diff.sh

@@ -17,6 +17,13 @@
 # shellcheck disable=SC2154 # the step's env sets DATA and the limits
 set -euo pipefail
 
+# The most chunks a head is judged in, and the run's cost guard: each judging
+# job in pr-verify.yml has a judge step for every chunk up to this, and a
+# timeout that holds them all and the final judge (workflow.test.mjs holds both
+# to it). 12 chunks of MAX_DIFF_BYTES hold a 3 MB diff of whole files. Only a
+# test sets its own.
+MAX_CHUNKS=${MAX_CHUNKS:-12}
+
 here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 mkdir "$DATA/diff"
 status=0

.github/scripts/pr-verify/chunk-diff.test.mjs

@@ -97,6 +97,39 @@ test("a diff that needs more chunks than the job has slots stops and says so", (
   assert.match(stdout, /^::error::.*needs 3 chunks, more than 2/m);
 });
 
+// The limits the "Write the diff" step runs under: its env in pr-verify.yml,
+// and no MAX_CHUNKS, so the script's own ceiling holds.
+const WORKFLOW = readFileSync(new URL("../../workflows/pr-verify.yml", import.meta.url), "utf8");
+const LIMITS = {
+  ...Object.fromEntries(["MAX_DIFF_BYTES", "PART_BYTES", "PART_LINES"].map((key) => [key, new RegExp(`^ +${key}: "(\\d+)"$`, "m").exec(WORKFLOW)[1]])),
+  MAX_CHUNKS: undefined,
+};
+const CEILING = Number(/^MAX_CHUNKS=\$\{MAX_CHUNKS:-(\d+)\}$/m.exec(readFileSync(SCRIPT, "utf8"))[1]);
+// A file of about `bytes` bytes: each content line is `+x\n`, 3 bytes.
+const sized = (name, bytes) => file(name, Math.floor(bytes / 3));
+
+test("under the workflow's limits a 3 MB diff is judged in as many chunks as it needs, with no stop", () => {
+  // Each file over half a chunk, so each takes a chunk of its own: 12 chunks.
+  const patch = Array.from({ length: 12 }, (_, i) => sized(`src/file-${i + 1}.mjs`, 250000)).join("");
+  assert.ok(Buffer.byteLength(patch) > 3000000);
+  const { status, data, outputs, stdout } = run(patch, LIMITS);
+  assert.equal(status, 0, stdout);
+  assert.equal(outputs, "chunks=12\n");
+  assert.doesNotMatch(stdout, /::error::/);
+  for (const k of [1, 10, 12]) assert.ok(partsOf(data, k).join("").startsWith(`diff --git a/src/file-${k}.mjs `), `chunk ${k}`);
+  assert.equal(Array.from({ length: 12 }, (_, i) => partsOf(data, i + 1).join("")).join(""), patch);
+  assert.match(manifestOf(data), /^chunk 12 diff\/chunk-12\/part-001\.patch modified src\/file-12\.mjs$/m);
+});
+
+test("a diff past the ceiling stops, naming its size and the ceiling", () => {
+  const ceiling = CEILING;
+  const patch = Array.from({ length: ceiling + 1 }, (_, i) => sized(`f${i}`, 250000)).join("");
+  const { status, outputs, stdout } = run(patch, LIMITS);
+  assert.equal(status, 1);
+  assert.equal(outputs, "");
+  assert.match(stdout, new RegExp(`^::error::no verdict for this head: the diff is ${Buffer.byteLength(patch)} bytes; the diff needs ${ceiling + 1} chunks, more than ${ceiling}$`, "m"));
+});
+
 test("the manifest names every file's part, in files of one Read each", () => {
   const names = Array.from({ length: 200 }, (_, i) => `src/module-${i}.mjs`);
   const { status, data, stdout } = run(names.map((name) => file(name, 1)).join(""), { MAX_DIFF_BYTES: "8000", PART_BYTES: "1000" });

.github/scripts/pr-verify/workflow.test.mjs

@@ -42,6 +42,11 @@ const list = (block, key) => {
   const body = new RegExp(`^ {10}${key}: \\|\\n((?: {12}.*\\n?)+)`, "m").exec(block)?.[1] ?? "";
   return body.split("\n").map((line) => line.trim()).filter(Boolean);
 };
+// The most chunks a head is judged in: chunk-diff.sh's one MAX_CHUNKS, and
+// each job holds a judge step for every one (the slot tests below).
+const chunkScript = readFileSync(new URL("./chunk-diff.sh", import.meta.url), "utf8");
+const CEILING = Number(/^MAX_CHUNKS=\$\{MAX_CHUNKS:-([1-9][0-9]*)\}$/m.exec(chunkScript)?.[1]);
+const SLOTS = Array.from({ length: CEILING }, (_, i) => i + 1);
 const PIN = /^melodic-software\/ci-workflows\/\.github\/actions\/([a-z-]+)@[0-9a-f]{40} # \S/;
 // The subscription job's one lane record names why the key did not judge,
 // in report-lane-outcome's fallback-reason (ci-workflows intake-triage.yml).
@@ -99,7 +104,9 @@ const glob = (pattern) => {
 };
 const REPO = "melodic-software/claude-code-plugins";
 // A job output read from each chunk's step, chunk 1's id being `first`.
-const chainOf = (build, first) => [first, `${first}-2`, `${first}-3`, `${first}-4`, `${first}-spec`].map(build).join(" || ");
+const chainOf = (build, first) => [first, ...SLOTS.slice(1).map((k) => `${first}-${k}`), `${first}-spec`].map(build).join(" || ");
+// Every leg check of a job left no clean handoff: "&& steps.<leg>-k.outcome != 'success'" for each slot and the spec.
+const legsClear = (first) => chainOf((id) => `steps\\.${id}\\.outcome != 'success'`, first).replaceAll(" || ", " && ");
 const upstreamRun = (overrides = {}) => ({
   github: {
     ref: "refs/heads/main",
@@ -285,7 +292,7 @@ test("the subscription job runs only on a clean handoff or an empty key, on a fi
   const steps = job.slice(job.indexOf("\n    steps:\n"));
   assert.deepEqual(
     steps.split("\n").filter((line) => line.includes("needs.")),
-    [CARRIED, ...Array(5).fill(`          fallback-reason: ${FALLBACK_REASON}`)],
+    [CARRIED, ...Array(SLOTS.length + 1).fill(`          fallback-reason: ${FALLBACK_REASON}`)],
   );
   const key = jobText(KEY);
   const outputs = /^ {4}outputs:\n((?: {6}.*\n)+)/m.exec(key)?.[1];
@@ -339,7 +346,7 @@ test("the fallback job runs only on the subscription job's clean handoff, on a f
   const steps = job.slice(job.indexOf("\n    steps:\n"));
   assert.deepEqual(
     steps.split("\n").filter((line) => line.includes("needs.")),
-    Array(5).fill(`          fallback-reason: ${FB_REASON}`),
+    Array(SLOTS.length + 1).fill(`          fallback-reason: ${FB_REASON}`),
   );
 });
 
@@ -416,11 +423,11 @@ test("the subscription job repeats the key job's steps but for the credential",
           .replace("\n          fallback-ready: ${{ steps.credential.outputs.fallback-ready }}", "")
           .replace(/PR_VERIFY_CLAUDE_CODE_OAUTH_TOKEN/g, "PR_VERIFY_CLAUDE_API_KEY")
           .replace(/^( {10})subscription-token: /m, "$1api-key: ")
-          .replace(/if: \(steps\.(judge(?:-\d|-spec)?)\.outcome == 'success' \|\| steps\.\1\.outcome == 'failure'\) && \(steps\.\1\.outputs\.handoff != 'true' \|\| steps\.(?:key|sub)-leg(?:-\d|-spec)?\.outcome == 'failure'\)/, "if: steps.$1.outcome == 'success' || steps.$1.outcome == 'failure'")
-          .replace(/known-failure-class: \$\{\{ steps\.(judge(?:-\d|-spec)?)\.outputs\.lane-class == 'credit-stop' && 'rate-limit' \|\| steps\.\1\.outputs\.lane-class \}\}/, "known-failure-class: ${{ steps.$1.outputs.lane-class }}")
+          .replace(/if: \(steps\.(judge(?:-\d+|-spec)?)\.outcome == 'success' \|\| steps\.\1\.outcome == 'failure'\) && \(steps\.\1\.outputs\.handoff != 'true' \|\| steps\.(?:key|sub)-leg(?:-\d+|-spec)?\.outcome == 'failure'\)/, "if: steps.$1.outcome == 'success' || steps.$1.outcome == 'failure'")
+          .replace(/known-failure-class: \$\{\{ steps\.(judge(?:-\d+|-spec)?)\.outputs\.lane-class == 'credit-stop' && 'rate-limit' \|\| steps\.\1\.outputs\.lane-class \}\}/, "known-failure-class: ${{ steps.$1.outputs.lane-class }}")
           .replace(`\n          fallback-reason: ${FALLBACK_REASON}`, "")
           .replace(/ && steps\.credential\.outputs\.present != 'false'/, "")
-          .replace(/(?: && steps\.(?:key|sub)-leg(?:-\d|-spec)?\.outcome != 'success')+$/m, ""),
+          .replace(/(?: && steps\.(?:key|sub)-leg(?:-\d+|-spec)?\.outcome != 'success')+$/m, ""),
       );
   assert.deepEqual(normalize(STEPS[SUB]), normalize(STEPS[KEY]));
   // The fallback job reads everything again, as the key job does; its
@@ -550,11 +557,11 @@ test("the outcome is reported whenever a judge ran to a final class, and a missi
   // Only a hand-off spares the key job its red; an empty token fails the subscription job.
   assert.match(
     STEPS[KEY].at(-1).block,
-    /^ {8}if: always\(\) && steps\.agree\.outcome == 'success' && steps\.verdict\.outcome != 'success' && steps\.credential\.outputs\.present != 'false' && steps\.key-leg\.outcome != 'success' && steps\.key-leg-2\.outcome != 'success' && steps\.key-leg-3\.outcome != 'success' && steps\.key-leg-4\.outcome != 'success' && steps\.key-leg-spec\.outcome != 'success'$/m,
+    new RegExp(`^ {8}if: always\\(\\) && steps\\.agree\\.outcome == 'success' && steps\\.verdict\\.outcome != 'success' && steps\\.credential\\.outputs\\.present != 'false' && ${legsClear("key-leg")}$`, "m"),
   );
   assert.match(
     STEPS[SUB].at(-1).block,
-    /^ {8}if: always\(\) && steps\.agree\.outcome == 'success' && steps\.verdict\.outcome != 'success' && steps\.sub-leg\.outcome != 'success' && steps\.sub-leg-2\.outcome != 'success' && steps\.sub-leg-3\.outcome != 'success' && steps\.sub-leg-4\.outcome != 'success' && steps\.sub-leg-spec\.outcome != 'success'$/m,
+    new RegExp(`^ {8}if: always\\(\\) && steps\\.agree\\.outcome == 'success' && steps\\.verdict\\.outcome != 'success' && ${legsClear("sub-leg")}$`, "m"),
   );
   assert.match(STEPS[FB].at(-1).block, /^ {8}if: always\(\) && steps\.agree\.outcome == 'success' && steps\.verdict\.outcome != 'success'$/m);
 });
@@ -606,7 +613,6 @@ test("the resolver reads the trusted-actor list from main in both jobs", () => {
   }
 });
 
-const SLOTS = [1, 2, 3, 4];
 const judgeSpec = (job) => step("Judge the whole spec", job);
 const SPEC_IF = "fromJSON(steps.diff.outputs.chunks || '0') > 1 && steps.chunk-results.outcome == 'success'";
 const judgeId = (k) => (k === 1 ? "judge" : `judge-${k}`);
@@ -616,11 +622,20 @@ const legName = { [KEY]: "Check the key leg", [SUB]: "Check the subscription leg
 const legId = { [KEY]: "key-leg", [SUB]: "sub-leg" };
 const chain = (build) => [...SLOTS.map(build), build("spec")].join(" || ");
 
+test("the chunk ceiling is set once, in chunk-diff.sh, and a 3 MB diff fits under it", () => {
+  assert.ok(Number.isInteger(CEILING), "chunk-diff.sh sets MAX_CHUNKS");
+  assert.doesNotMatch(text, /MAX_CHUNKS: /, "no step sets its own");
+  // 3 MB of whole files in chunks of MAX_DIFF_BYTES (chunk-diff.test.mjs splits one).
+  const chunkBytes = Number(input(step("Write the diff"), "MAX_DIFF_BYTES").replaceAll('"', ""));
+  assert.ok(CEILING * chunkBytes >= 3000000, `${CEILING} chunks of ${chunkBytes} bytes`);
+});
+
 test("every job has one judge step per chunk, each repeating the first but for its chunk", () => {
   for (const job of JOBS) {
     const first = step("Judge the pull request", job);
     assert.equal(STEPS[job].filter((s) => /^ {8}uses: melodic-software\/ci-workflows\/\.github\/actions\/run-claude-cli@/m.test(s.block)).length, SLOTS.length + 1, job);
-    assert.equal(input(step("Write the diff", job), "MAX_CHUNKS"), `"${SLOTS.length}"`, job);
+    // The ceiling is chunk-diff.sh's; no step sets its own.
+    assert.equal(input(step("Write the diff", job), "MAX_CHUNKS"), undefined, job);
     assert.equal(input(first, "prompt-file"), "${{ runner.temp }}/pr-verify-prompt-1.md", job);
     const bare = (block) => block.split("\n").filter((line) => !line.trim().startsWith("#")).filter((line) => !/^ +(- )?(name|id|if): /.test(line)).join("\n").trim();
     for (const k of SLOTS.slice(1)) {
@@ -718,6 +733,12 @@ test("the prompt step gives each chunk its own parts, and a lone chunk no note",
   assert.match(prompt(1), /chunk 1 of 2/);
   assert.match(prompt(2), /chunk 2 of 2/);
   assert.ok(prompt(2).includes(`chunk-2/part-001.patch`));
+  // Past nine chunks, each chunk still gets its own prompt and parts.
+  const many = write(12, Array.from({ length: 12 }, () => 1));
+  assert.match(many.prompt(12), /chunk 12 of 12/);
+  assert.ok(many.prompt(12).includes(`  - \`${many.cwd}/data/diff/chunk-12/part-001.patch\`\n`));
+  assert.ok(!many.prompt(12).includes("chunk-1/part"));
+  assert.ok(many.prompt(10).includes(`chunk-10/part-001.patch`));
   const one = write(1, [1]);
   assert.ok(!one.prompt(1).includes("data/manifest"));
   assert.ok(prompt(1).includes(`${cwd}/data/diff/chunk-N`) || prompt(1).includes(`relative to \`${cwd}/data\``));
@@ -750,7 +771,7 @@ test("the whole-spec step runs only on a chunked head, after every chunk, and ca
     const denied = list(judge, "disallowed-tools");
     assert.ok(denied.includes("Read(./pr-verify-data/diff/**)"), job);
     assert.deepEqual(denied.filter((rule) => rule !== "Read(./pr-verify-data/diff/**)"), list(step("Judge the pull request", job), "disallowed-tools"), job);
-    assert.ok(index("Write the chunk results", job) > index("Upload chunk 4's sealed transcript", job), job);
+    assert.ok(index("Write the chunk results", job) > index(`Upload chunk ${SLOTS.length}'s sealed transcript`, job), job);
     assert.ok(index("Upload the spec judgment's sealed transcript", job) < index("Write the verdict", job), job);
     assert.equal(input(step("Report the spec judgment's outcome", job), "step-started-at"), "${{ steps.time-spec.outputs.started-at }}", job);
     assert.equal(input(step("Upload the spec judgment's sealed transcript", job), "job-id"), "${{ github.job }}-spec", job);
@@ -784,7 +805,7 @@ test("after each judge a counted fail stops the slots and reaches the verdict st
       assert.match(check, /write-verdict\.mjs chunk-fail$/m, job);
       const name = k === 1 ? "Judge the pull request" : `Judge chunk ${k}`;
       assert.ok(index(`Read chunk ${k}'s verdict`, job) > index(name, job), job);
-      if (k < 4) assert.ok(index(`Read chunk ${k}'s verdict`, job) < index(`Time chunk ${k + 1}`, job), job);
+      if (k < SLOTS.length) assert.ok(index(`Read chunk ${k}'s verdict`, job) < index(`Time chunk ${k + 1}`, job), job);
     }
     assert.match(step("Summarize the verdict", job), /^ {8}if: \$\{\{ !cancelled\(\) && steps\.verdict\.outcome == 'success' \}\}$/m, job);
   }

.github/scripts/pr-verify/write-verdict.mjs

@@ -524,9 +524,9 @@ export function checkChunk(env = process.env) {
 
 // Each chunk by position: `output`, or `error` for a chunk that did not count.
 function judgedChunks(env) {
-  const chunkDirs = readdirSync(env.DIFF_DIR).filter((name) => /^chunk-[1-9]$/.test(name));
+  const chunkDirs = readdirSync(env.DIFF_DIR).filter((name) => /^chunk-[1-9][0-9]?$/.test(name));
   const count = Number(env.CHUNKS);
-  if (!/^[1-9]$/.test(env.CHUNKS) || chunkDirs.length !== count) {
+  if (!/^[1-9][0-9]?$/.test(env.CHUNKS) || chunkDirs.length !== count) {
     throw new Error(`the chunk count does not match ${chunkDirs.length} chunk(s) of the diff`);
   }
   const executions = positions(env.EXECUTION_FILES);

.github/scripts/pr-verify/write-verdict.test.mjs

@@ -647,6 +647,87 @@ test("main writes one verdict from every chunk, and none when a chunk leaves non
   }
 });
 
+test("a head of twelve chunks gets a verdict from every chunk, and none when one leaves none", () => {
+  const COUNT = 12;
+  const dir = tempDir("pr-verify-many-chunks-");
+  const spec = path.join(dir, "spec.json");
+  writeFileSync(spec, JSON.stringify([ISSUE]));
+  const diff = path.join(dir, "diff");
+  const parts = Array.from({ length: COUNT }, (_, i) => {
+    mkdirSync(path.join(diff, `chunk-${i + 1}`), { recursive: true });
+    const part = path.join(diff, `chunk-${i + 1}`, "part-001.patch");
+    writeFileSync(part, `diff --git a/${i + 1} b/${i + 1}\n`);
+    return part;
+  });
+  const manifestDir = path.join(dir, "manifest");
+  mkdirSync(manifestDir);
+  const manifest = path.join(manifestDir, "part-001.txt");
+  writeFileSync(manifest, "chunk 1 diff/chunk-1/part-001.patch modified 1\n");
+  const resultsDir = path.join(dir, "results");
+  mkdirSync(resultsDir);
+  const results = parts.map((_, i) => {
+    const file = path.join(resultsDir, `chunk-${i + 1}.json`);
+    writeFileSync(file, "{}");
+    return file;
+  });
+  let id = 0;
+  const write = (name, value) => {
+    const file = path.join(dir, `${name}-${(id += 1)}`);
+    writeFileSync(file, value);
+    return file;
+  };
+  const execution = (read, output) =>
+    write(
+      "execution.json",
+      JSON.stringify([
+        { type: "assistant", message: { content: read.map((file, i) => ({ type: "tool_use", id: `t${i}`, name: "Read", input: { file_path: file } })) } },
+        { type: "result", subtype: "success", structured_output: output },
+      ]),
+    );
+  const log = () => write("log.jsonl", JSON.stringify({ leg: 1, denials: { count: 0 } }));
+  const run = (label, outputs) => ({
+    PR_NUMBER: "41",
+    HEAD_SHA: HEAD,
+    BASE_SHA: BASE,
+    HEAD_REF: "feat/x",
+    SPEC_FILE: spec,
+    GITHUB_RUN_ID: "9001",
+    CHUNKS: String(COUNT),
+    DIFF_DIR: diff,
+    VERDICT_DIR: path.join(dir, `verdict-${label}`),
+    GITHUB_OUTPUT: path.join(dir, `out-${label}`),
+    EXECUTION_FILES: `${outputs.map((output, i) => (output === null ? "" : execution([parts[i], manifest], output))).join("\n")}\n`,
+    LOG_FILES: `${outputs.map((output) => (output === null ? "" : log())).join("\n")}\n`,
+    FINAL_EXECUTION_FILE: execution([...results, spec, manifest], { ...FINAL_PASS, items: [{ item: "item 1", chunks: [12] }] }),
+    FINAL_LOG_FILE: log(),
+    RESULTS_DIR: resultsDir,
+    MANIFEST_DIR: manifestDir,
+  });
+  const verdictOf = (env) => JSON.parse(readFileSync(path.join(env.VERDICT_DIR, "verdict.json"), "utf8"));
+  const all = (output) => Array.from({ length: COUNT }, () => output);
+
+  const passed = run("pass", all(CHUNK_PASS));
+  main(passed);
+  assert.equal(verdictOf(passed).verdict, "pass");
+  assert.match(readFileSync(passed.GITHUB_OUTPUT, "utf8"), new RegExp(`^name=pr-verify-41-${HEAD}-pass\n`));
+  assert.match(verdictOf(passed).summary, /^Chunk 12 of 12, pass: chunk fine$/m);
+
+  const failing = all(CHUNK_PASS);
+  failing[10] = CHUNK_FAIL;
+  const failed = run("fail", failing);
+  main(failed);
+  assert.equal(verdictOf(failed).verdict, "fail");
+  assert.deepEqual(verdictOf(failed).notes, CHUNK_FAIL.notes);
+  assert.match(verdictOf(failed).summary, /^Chunk 11 of 12, fail: chunk broken$/m);
+
+  const missing = all(CHUNK_PASS);
+  missing[10] = null;
+  const none = run("none", missing);
+  assert.throws(() => main(none), /chunk 11 of 12/);
+  assert.equal(existsSync(path.join(none.VERDICT_DIR, "verdict.json")), false);
+  assert.equal(existsSync(none.GITHUB_OUTPUT), false);
+});
+
 test("checkKeyLeg reads every chunk's log, so a denial in an earlier chunk stops the handoff", () => {
   const dir = tempDir("pr-verify-key-legs-");
   const file = (name, count) => {