From 0fd10f613025cb142a015d5484067f9889001525 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:23:40 -0400 Subject: [PATCH 1/4] fix(pr-pipeline): give the main-run review the lanes' credential chain and collapse exact renames pr-review-main ran on REVIEWS_CLAUDE_API_KEY alone, so an exhausted key ended every main-run review in a credit stop with no verdict and left every lane-source pull request unbound. Each lane now runs the review lanes' chain: the key leg, then review-subscription on the role's subscription token for a lane the key leg handed off (credit stop, 401, 402) or with no key, then review-fallback on the central fallback token for a lane that leg handed off. Each leg is its own job with one credential and repeats the key job's steps; route jobs with no secret pass on only lanes whose handoff reason is in a closed set. Every leg writes the same verdict, and a handed-off leg stays green. The review diff now shows a file moved unchanged as its rename lines alone (-M100%), so pure moves fit under the size cap; an edited rename, a move into or out of a lane source, and a mode change still show whole. Co-Authored-By: Claude Opus 5.5 --- ...in-run-review-credential-chain-7c4e19a2.md | 8 + .github/artifact-retention.json | 4 +- .github/lane-funding.json | 4 +- .../scripts/pr-review-main/workflow.test.mjs | 411 ++++++-- .github/scripts/pr-review-main/write-diff.py | 87 ++ .github/workflows/pr-review-main.yml | 978 +++++++++++++++++- docs/conventions/pr-pipeline/README.md | 20 +- 7 files changed, 1431 insertions(+), 81 deletions(-) create mode 100644 .changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md create mode 100644 .github/scripts/pr-review-main/write-diff.py diff --git a/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md b/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md new file mode 100644 index 0000000000..952baa52ed --- /dev/null +++ b/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md @@ -0,0 +1,8 @@ +--- +bump: patch +--- + +### Fixed + +- The main-run review (`pr-review-main.yml`) runs on the review lanes' credential chain instead of the API key alone: a lane whose key leg credit-stops, is refused with a 401 or 402, or has no key is reviewed on the reviews role's subscription token, and a lane that leg hands off (a limit stop, a credit stop, a 401 or 402, or no token) on `FALLBACK_CLAUDE_CODE_OAUTH_TOKEN`. Each leg is its own job holding one credential, writes the same verdict, and stays green when it hands off, so an exhausted key no longer leaves every lane-source pull request unbound. +- The main-run review's diff shows a file moved unchanged as its rename lines alone, so a pull request of pure moves stays under the review's size cap. An edited rename, a move into or out of a lane source and a move that changes the file mode still show whole. diff --git a/.github/artifact-retention.json b/.github/artifact-retention.json index f1c283ba62..cd2ac89caa 100644 --- a/.github/artifact-retention.json +++ b/.github/artifact-retention.json @@ -29,7 +29,9 @@ "max-reader": "plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py", "reason": "A verdict that reported findings must outlive a later clean review of the same tree, or its expiry would lower the count the gate reads; the gate counts the most findings any unexpired verdict of the tree reported.", "uploads": [ - ".github/workflows/pr-review-main.yml#review" + ".github/workflows/pr-review-main.yml#review", + ".github/workflows/pr-review-main.yml#review-subscription", + ".github/workflows/pr-review-main.yml#review-fallback" ] }, "pr-merge-dispatched-*": { diff --git a/.github/lane-funding.json b/.github/lane-funding.json index 72577b2817..80152adaea 100644 --- a/.github/lane-funding.json +++ b/.github/lane-funding.json @@ -12,7 +12,9 @@ "lanes": [ { "id": "pr-review", "workflow": "pr-review-hosted.yml", "launcher": "run-claude-cli", "triggers": ["pull_request"], "currentSecrets": ["REVIEWS_CLAUDE_API_KEY", "REVIEWS_CLAUDE_CODE_OAUTH_TOKEN", "FALLBACK_CLAUDE_CODE_OAUTH_TOKEN"] }, { "id": "pr-review-security", "workflow": "pr-review-security-hosted.yml", "launcher": "run-claude-cli", "triggers": ["pull_request"], "currentSecrets": ["REVIEWS_CLAUDE_API_KEY", "REVIEWS_CLAUDE_CODE_OAUTH_TOKEN", "FALLBACK_CLAUDE_CODE_OAUTH_TOKEN"] }, - { "id": "review", "workflow": "pr-review-main.yml", "launcher": "run-claude-cli", "triggers": ["workflow_run"], "gate": "same-repo", "currentSecrets": ["REVIEWS_CLAUDE_API_KEY"] } + { "id": "review", "workflow": "pr-review-main.yml", "launcher": "run-claude-cli", "triggers": ["workflow_run"], "gate": "same-repo", "currentSecrets": ["REVIEWS_CLAUDE_API_KEY"] }, + { "id": "review-subscription", "workflow": "pr-review-main.yml", "launcher": "run-claude-cli", "triggers": ["workflow_run"], "gate": "same-repo", "currentSecrets": ["REVIEWS_CLAUDE_CODE_OAUTH_TOKEN", "FALLBACK_CLAUDE_CODE_OAUTH_TOKEN"] }, + { "id": "review-fallback", "workflow": "pr-review-main.yml", "launcher": "run-claude-cli", "triggers": ["workflow_run"], "gate": "same-repo", "currentSecrets": ["FALLBACK_CLAUDE_CODE_OAUTH_TOKEN"] } ] }, { diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index ab8f234e27..22333e1a87 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -1,26 +1,44 @@ // pr-review-main.yml's hardening, from its header and ADR 0049: it runs only // from main on workflow_run, its token can only read, it checks out main and -// never the head, the model reads only its data directory, and the verdict is -// uploaded once, after it is written, under the name the writer chose. +// never the head, the model reads only its data directory, and each leg +// uploads the verdict once, after it is written, under the name the writer +// chose. Each lane runs one leg on the review lanes' credentials in their +// order, a later leg only for a lane the previous one handed on. import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { test } from "node:test"; const text = readFileSync(new URL("../../workflows/pr-review-main.yml", import.meta.url), "utf8"); const prompt = readFileSync(new URL("./prompt.md", import.meta.url), "utf8"); -const steps = text - .split(/\n(?= - name: )/) - .slice(1) - .map((block) => ({ name: /^ - name: (.*)$/m.exec(block)[1], block })); -const step = (name) => { - const found = steps.find((s) => s.name === name); - assert.ok(found, `a step named ${name}`); +const KEY = "review"; +const SUB = "review-subscription"; +const FB = "review-fallback"; +const LEGS = [KEY, SUB, FB]; +const jobText = (id) => { + const found = new RegExp(`\\n ${id}:\\n([\\s\\S]*?)(?=\\n [a-z-]+:\\n|$)`).exec(text); + assert.ok(found, `a job ${id}`); + return found[1]; +}; +// Each step starts at a ` - name:` line of its job. +const stepsOf = (id) => + jobText(id) + .split(/\n(?= - name: )/) + .slice(1) + // A step ends before the next step's or job's comment. + .map((block) => ({ name: /^ - name: (.*)$/m.exec(block)[1], block: block.replace(/\n\n {2}(?: {4})?#[\s\S]*$/, "") })); +const STEPS = Object.fromEntries(LEGS.map((id) => [id, stepsOf(id)])); +const step = (name, job = KEY) => { + const found = STEPS[job].find((s) => s.name === name); + assert.ok(found, `a step named ${name} in ${job}`); return found.block; }; -const index = (name) => steps.findIndex((s) => s.name === name); +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]; test("it runs only from main on pr-require-checks' completion", () => { const on = /\non:\n([\s\S]*?)\n\S/.exec(text)[1]; @@ -33,21 +51,24 @@ test("it runs only from main on pr-require-checks' completion", () => { assert.match(text, /github\.event\.workflow_run\.head_repository\.full_name == github\.repository/); }); -const jobText = (id) => { - const found = new RegExp(`\\n ${id}:\\n([\\s\\S]*?)(?=\\n [a-z-]+:\\n|$)`).exec(text); - assert.ok(found, `a job ${id}`); - return found[1]; -}; const grantsOf = (block) => [...block.matchAll(/^ {6}([a-z-]+): (\w+)(?: #.*)?$/gm)].filter(([, scope]) => ["contents", "pull-requests", "actions", "checks", "issues", "statuses", "id-token"].includes(scope), ); -test("the model's job can only read, and the one writer runs no model", () => { +test("the model's jobs can only read, and the one writer runs no model", () => { assert.match(text, /^permissions: \{\}$/m); - const review = grantsOf(jobText("review")); - assert.ok(review.length > 0); - for (const [line, , level] of review) assert.equal(level, "read", line); + for (const job of LEGS) { + const review = grantsOf(jobText(job)); + assert.ok(review.length > 0, job); + for (const [line, , level] of review) assert.equal(level, "read", `${job}: ${line}`); + } + // The routes between legs hold no grant, no secret and no model. + for (const job of ["review-handoffs", "subscription-handoffs"]) { + const route = jobText(job); + assert.match(route, /^ {4}permissions: \{\}$/m, job); + assert.doesNotMatch(route, /run-claude-cli|secrets\.|actions\/checkout@|uses:/, job); + } const record = jobText("record"); assert.deepEqual(grantsOf(record).map(([, scope, level]) => `${scope}:${level}`), ["checks:write"]); assert.doesNotMatch(record, /run-claude-cli|secrets\.|actions\/checkout@/); @@ -59,7 +80,8 @@ test("the model's job can only read, and the one writer runs no model", () => { test("the diff shows every file as text and refuses one a review cannot read whole", () => { const diff = step("Write the diff"); - assert.match(diff, /git -C "\$SRC" diff --text --no-renames --no-ext-diff --no-textconv/); + assert.match(diff, /python3 -I \.base\/\.github\/scripts\/pr-review-main\/write-diff\.py "\$SRC" "\$BASE_SHA" "\$HEAD_SHA" "\$DATA\/diff\.patch"/); + 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, /MAX_DIFF_BYTES: "\d+"/); @@ -107,48 +129,115 @@ test("a diff with a line past the review's cap gets no verdict", async () => { } }); -test("a partial rename is reviewed as the whole new file", () => { - const diff = step("Write the diff"); - const command = diff.match(/^ *(git -C "\$SRC" diff .*?) >"\$DATA\/diff\.patch"$/m)[1]; - assert.match(command, / --no-renames /); +// write-diff.py run on a fixture repository: each case is a commit on top of +// a base, and the patch it prints for base...head. +function diffCase(build) { const src = mkdtempSync(path.join(tmpdir(), "pr-review-main-rename-")); try { - const git = (...args) => spawnSync("git", ["-C", src, ...args], { encoding: "utf8", env: { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1" } }); + const env = { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1" }; + const git = (...args) => { + if (args[0] === "mv") mkdirSync(path.dirname(path.join(src, args[2])), { recursive: true }); + const run = spawnSync("git", ["-C", src, ...args], { encoding: "utf8", env }); + assert.equal(run.status, 0, run.stderr); + return run.stdout.trim(); + }; + const write = (file, body) => { + mkdirSync(path.dirname(path.join(src, file)), { recursive: true }); + writeFileSync(path.join(src, file), body); + }; git("init", "-q"); - const lines = Array.from({ length: 40 }, (_, i) => `line ${i}`); - writeFileSync(path.join(src, "notes.md"), `${lines.join("\n")}\n`); + build.base(write); git("add", "-A"); git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "base"); - const base = git("rev-parse", "HEAD").stdout.trim(); - mkdirSync(path.join(src, ".claude")); - git("mv", "notes.md", ".claude/hook.md"); - writeFileSync(path.join(src, ".claude/hook.md"), `${lines.join("\n")}\nrun the payload\n`); + const base = git("rev-parse", "HEAD"); + build.head(git, write, (file) => chmodSync(path.join(src, file), 0o755)); git("add", "-A"); git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "head"); - const head = git("rev-parse", "HEAD").stdout.trim(); - const out = spawnSync("bash", ["-c", command], { encoding: "utf8", env: { ...process.env, SRC: src, BASE_SHA: base, HEAD_SHA: head } }).stdout; - // Every line of the new file is shown as added, not only the change. - assert.doesNotMatch(out, /^rename from/m); - assert.match(out, /^\+line 0$/m); - assert.match(out, /^\+run the payload$/m); + const head = git("rev-parse", "HEAD"); + const out = path.join(src, "..", `${path.basename(src)}.patch`); + const run = spawnSync("python3", ["-I", WRITE_DIFF, src, base, head, out], { encoding: "utf8", env }); + assert.equal(run.status, 0, run.stderr); + const patch = readFileSync(out, "utf8"); + rmSync(out); + return patch; } finally { rmSync(src, { recursive: true, force: true }); } +} +const lines = (prefix, n) => `${Array.from({ length: n }, (_, i) => `${prefix} ${i}`).join("\n")}\n`; + +test("a file moved unchanged outside the lane sources prints as its rename lines alone", () => { + const patch = diffCase({ + base: (write) => { + for (let i = 0; i < 5; i++) write(`docs/a${i}.md`, lines(`file ${i} line`, 400)); + }, + head: (git) => { + for (let i = 0; i < 5; i++) git("mv", `docs/a${i}.md`, `guide/a${i}.md`); + }, + }); + // Five moves of 400 lines each: five headers, no content line. + assert.equal(patch.match(/^rename from docs\/a\d\.md$/gm).length, 5); + assert.equal(patch.match(/^rename to guide\/a\d\.md$/gm).length, 5); + assert.doesNotMatch(patch, /^[+-]file /m); + assert.ok(patch.length < 1000, `${patch.length} bytes`); +}); + +test("a move into or out of a lane source, or one that changes the mode, prints whole", () => { + const patch = diffCase({ + base: (write) => { + write("notes/hook.md", lines("into", 30)); + write(".github/workflows/old.yml", lines("out of", 30)); + write("tools/run.sh", lines("mode", 30)); + }, + head: (git, _write, chmod) => { + git("mv", "notes/hook.md", ".claude/hook.md"); + git("mv", ".github/workflows/old.yml", "archive/old.yml"); + git("mv", "tools/run.sh", "tools/go.sh"); + chmod("tools/go.sh"); + }, + }); + assert.doesNotMatch(patch, /^rename from/m); + // Each new file's every line is shown as added, each old one's as deleted. + for (const word of ["into", "out of", "mode"]) { + assert.equal(patch.match(new RegExp(`^\\+${word} \\d+$`, "gm")).length, 30, word); + assert.equal(patch.match(new RegExp(`^-${word} \\d+$`, "gm")).length, 30, word); + } + assert.match(patch, /^new file mode 100755$/m); +}); + +test("an edited rename prints whole, as a delete and an add", () => { + const patch = diffCase({ + base: (write) => write("notes.md", lines("line", 40)), + head: (git, write) => { + git("mv", "notes.md", "docs/notes.md"); + write("docs/notes.md", `${lines("line", 40)}run the payload\n`); + }, + }); + assert.doesNotMatch(patch, /^rename from/m); + assert.match(patch, /^\+line 0$/m); + assert.match(patch, /^-line 0$/m); + assert.match(patch, /^\+run the payload$/m); }); test("each leg leaves a lane record", () => { - const report = step("Report the review's outcome"); - assert.match(report, /report-lane-outcome@[0-9a-f]{40}/); - assert.match(report, /^ {10}lane: pr-review-main$/m); - assert.ok(index("Review the pull request") < index("Report the review's outcome")); - assert.match(step("Review the pull request"), /^ {8}continue-on-error: true$/m); + 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); + } }); test("it checks out main only, and runs local actions from that checkout", () => { - const checkouts = steps.filter((s) => /uses: actions\/checkout@/.test(s.block)); - assert.equal(checkouts.length, 1); - assert.match(checkouts[0].block, /^ {10}ref: refs\/heads\/main$/m); - assert.match(checkouts[0].block, /^ {10}persist-credentials: false$/m); + assert.equal(text.match(/uses: actions\/checkout@/g).length, LEGS.length); + for (const job of LEGS) { + const checkouts = STEPS[job].filter((s) => /uses: actions\/checkout@/.test(s.block)); + assert.equal(checkouts.length, 1, job); + assert.match(checkouts[0].block, /^ {10}ref: refs\/heads\/main$/m); + assert.match(checkouts[0].block, /^ {10}persist-credentials: false$/m); + } for (const [, local] of text.matchAll(/^\s+uses: (\.\/\S+)/gm)) { assert.ok(local.startsWith("./.base/.github/actions/"), local); } @@ -158,13 +247,15 @@ test("it checks out main only, and runs local actions from that checkout", () => }); test("the model reads its data directory alone", () => { - const review = step("Review the pull request"); + for (const job of LEGS) { + const review = step("Review the pull request", 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); for (const denied of ["Read(./.base/**)", "Bash", "Write", "Edit", "WebFetch", "Agent", "mcp__*"]) { assert.ok(review.includes(` ${denied}\n`), denied); } + } for (const placeholder of ["LANE_NAME", "CRITERIA_DIR", "STANDARDS_DIR", "DIFF_PARTS", "CONTEXT_FILE", "PR_FILE"]) { assert.ok(prompt.includes(placeholder), placeholder); assert.ok(step("Write the prompt and schema").includes(`prompt=\${prompt//${placeholder}/`), placeholder); @@ -172,16 +263,21 @@ test("the model reads its data directory alone", () => { }); test("the verdict uploads once, after it is written, under the writer's name", () => { - const uploads = steps.filter((s) => /uses: actions\/upload-artifact@/.test(s.block)); - assert.equal(uploads.length, 1); - assert.equal(uploads[0].name, "Upload the verdict"); - assert.ok(index("Write the verdict") < index("Upload the verdict")); - assert.match(uploads[0].block, /^ {8}if: steps\.verdict\.outcome == 'success'$/m); - assert.match(uploads[0].block, /^ {10}name: \$\{\{ steps\.verdict\.outputs\.name \}\}$/m); - // The tree is git's, the head and base the trigger gate's: never the model's. - const write = step("Write the verdict"); - assert.match(write, /TREE_SHA: \$\{\{ steps\.diff\.outputs\.tree \}\}/); - assert.match(write, /HEAD_SHA: \$\{\{ steps\.gate\.outputs\.head-sha \}\}/); + assert.equal(text.match(/uses: actions\/upload-artifact@/g).length, LEGS.length); + for (const job of LEGS) { + const uploads = STEPS[job].filter((s) => /uses: actions\/upload-artifact@/.test(s.block)); + assert.equal(uploads.length, 1, job); + assert.equal(uploads[0].name, "Upload the verdict"); + assert.ok(index("Write the verdict", job) < index("Upload the verdict", job)); + assert.match(uploads[0].block, /^ {8}if: steps\.verdict\.outcome == 'success'$/m); + assert.match(uploads[0].block, /^ {10}name: \$\{\{ steps\.verdict\.outputs\.name \}\}$/m); + // The tree is git's, the head and base the trigger gate's: never the model's. + 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); + } assert.match(step("Write the diff"), /rev-parse --verify "\$HEAD_SHA\^\{tree\}"/); }); @@ -191,3 +287,202 @@ test("only a lane-source pull request is reviewed, by the gate's own predicate", assert.match(select, /ev\.lane_source\(p\)/); assert.match(step("Write the diff"), /^ {8}if: steps\.select\.outputs\.review == 'true'$/m); }); + +// The review lanes' credential order, read from their callers' `secrets:` +// blocks, never from this workflow: each names the reusable's secret and the +// repository secret it passes. +const hostedChain = (file) => { + const caller = readFileSync(new URL(`../../workflows/${file}`, import.meta.url), "utf8"); + const block = /^ {4}secrets:\n((?: {6}.*\n)+)/m.exec(caller)[1]; + return [...block.matchAll(/^ {6}([A-Z_]+): \$\{\{ secrets\.([A-Z_]+) \}\}$/gm)].map(([, as, secret]) => [as, secret]); +}; +// The run-claude-cli input each reusable secret reaches in ci-workflows +// pr-review-cli.yml: review-key passes CLAUDE_API_KEY as api-key, +// review-subscription CLAUDE_CODE_OAUTH_TOKEN as subscription-token, +// review-fallback FALLBACK_CLAUDE_CODE_OAUTH_TOKEN as fallback-token. +const LAUNCHER_INPUT = { + CLAUDE_API_KEY: "api-key", + CLAUDE_CODE_OAUTH_TOKEN: "subscription-token", + FALLBACK_CLAUDE_CODE_OAUTH_TOKEN: "fallback-token", +}; +const CREDENTIAL_INPUTS = Object.values(LAUNCHER_INPUT); +const needsOf = (job) => /^ {4}needs: (.*)$/m.exec(jobText(job))?.[1]; + +test("the legs run the review lanes' credentials in the review lanes' order", () => { + const hosted = hostedChain("pr-review-hosted.yml"); + assert.deepEqual(hostedChain("pr-review-security-hosted.yml"), hosted); + assert.equal(hosted.length, 3); + // Walk this workflow's chain from the key leg through each route. + const order = [KEY]; + for (let job = KEY; ; ) { + const route = ["review-handoffs", "subscription-handoffs"].find((id) => needsOf(id) === job); + if (!route) break; + job = LEGS.find((id) => needsOf(id) === route); + order.push(job); + } + assert.deepEqual(order, LEGS); + const used = order.map((job) => { + const review = step("Review the pull request", 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]]; + }); + assert.deepEqual(used, hosted.map(([as, secret]) => [LAUNCHER_INPUT[as], secret])); + // Each leg checks its own credential; the key leg hands an empty key on, + // and the subscription leg checks whether the fallback token is set. + const later = { + [KEY]: '"true"', + [SUB]: "${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN != '' }}", + [FB]: '"false"', + }; + order.forEach((job, i) => { + const check = step("Check the credential", job); + assert.equal(input(check, "PRESENT"), `\${{ secrets.${used[i][1]} != '' }}`, job); + assert.equal(input(check, "SECRET_NAME"), used[i][1], job); + assert.equal(input(check, "LATER_PRESENT"), later[job], job); + }); + // Each secret's value reaches only its leg's launcher step; elsewhere a + // job may ask only whether it is set. + 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); + else assert.deepEqual(values, [], `${job}: ${s.name}`); + } + } + assert.doesNotMatch(text, /secrets: inherit|toJSON\(secrets\)/); +}); + +// run-claude-cli's handoff-reason (its action.yml at the pinned SHA): a key +// leg hands off on credit-stop or auth, a subscription leg with fallback-ready +// on limit-stop, credit-stop or auth; an empty credential with a later leg's +// set hands off as no-key (report-lane-outcome's fallback-reason). +const ROUTES = { + "review-handoffs": ["credit-stop", "auth", "no-key"], + "subscription-handoffs": ["limit-stop", "credit-stop", "auth", "no-key"], +}; +const routeRun = (job) => { + const block = jobText(job); + const run = /^ {8}run: \|\n((?: {10}.*\n?)+)/m.exec(block)[1].replace(/^ {10}/gm, ""); + return (env) => { + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-route-")); + try { + const out = path.join(dir, "out"); + writeFileSync(out, ""); + const reasons = /^ {10}REASONS: (.*)$/m.exec(block)[1]; + const result = spawnSync("bash", ["-e", "-c", run], { encoding: "utf8", env: { ...process.env, REASONS: reasons, GITHUB_OUTPUT: out, ...env } }); + const line = readFileSync(out, "utf8").trim(); + return { status: result.status, matrix: line ? JSON.parse(line.replace(/^matrix=/, "")) : undefined }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; +}; + +test("a route hands the next leg fixed lane rows, only for a lane handed on with a closed reason", () => { + const rows = { + "claude-review": { lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED" }, + "claude-security-review": { lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED" }, + }; + // The rows are the key leg's own matrix. + for (const row of Object.values(rows)) { + assert.ok(jobText(KEY).includes(` - lane: ${row.lane}\n skill: ${row.skill}\n route: ${row.route}\n switch: ${row.switch}\n`), row.lane); + } + for (const [job, reasons] of Object.entries(ROUTES)) { + assert.deepEqual(/^ {10}REASONS: (.*)$/m.exec(jobText(job))[1].split(" "), reasons, job); + const route = routeRun(job); + assert.deepEqual(route({ CODE: "credit-stop", SECURITY: "" }), { status: 0, matrix: { include: [{ ...rows["claude-review"], reason: "credit-stop" }] } }); + assert.deepEqual(route({ CODE: "auth", SECURITY: "no-key" }), { + status: 0, + matrix: { include: [{ ...rows["claude-review"], reason: "auth" }, { ...rows["claude-security-review"], reason: "no-key" }] }, + }); + for (const bad of ["other", "credit-stop ", "true"]) { + const result = route({ CODE: "", SECURITY: bad }); + assert.notEqual(result.status, 0, `${job} ${JSON.stringify(bad)}`); + assert.equal(result.matrix, undefined); + } + } + // limit-stop comes only from a subscription leg. + assert.notEqual(routeRun("review-handoffs")({ CODE: "limit-stop", SECURITY: "" }).status, 0); + assert.equal(routeRun("subscription-handoffs")({ CODE: "limit-stop", SECURITY: "" }).status, 0); +}); + +test("a later leg runs only for the lanes the previous leg handed on", () => { + for (const [route, from, to] of [["review-handoffs", KEY, SUB], ["subscription-handoffs", SUB, FB]]) { + const routeJob = jobText(route); + assert.equal(needsOf(route), from); + assert.match(routeJob, new RegExp(`&& \\(needs\\.${from}\\.outputs\\.claude-review != '' \\|\\| needs\\.${from}\\.outputs\\.claude-security-review != ''\\)`)); + assert.doesNotMatch(routeJob, /always\(\)|!cancelled\(\)/); + const leg = jobText(to); + assert.equal(needsOf(to), route); + assert.match(leg, new RegExp(`^ {6}matrix: \\$\\{\\{ fromJSON\\(needs\\.${route}\\.outputs\\.matrix\\) \\}\\}$`, "m")); + assert.doesNotMatch(leg, /always\(\) \|\||!cancelled\(\)/); + // Each lane's reason is its own output, set only by the handoff step. + for (const lane of ["claude-review", "claude-security-review"]) { + assert.match(jobText(from), new RegExp(`^ {6}${lane}: \\$\\{\\{ steps\\.handoff\\.outputs\\.${lane} \\}\\}$`, "m")); + } + } + // The fallback leg is the last: it hands nothing on. + assert.equal(index("Hand the lane on", FB), -1); + assert.doesNotMatch(jobText(FB), /^ {4}outputs:$/m); + 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/); + // 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); +}); + +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); + 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, ""); + const dir = mkdtempSync(path.join(tmpdir(), "pr-review-main-handoff-")); + try { + const out = path.join(dir, "out"); + const hand = (reason) => { + writeFileSync(out, ""); + const status = spawnSync("bash", ["-e", "-c", run], { env: { ...process.env, LANE: "claude-security-review", REASON: reason, GITHUB_OUTPUT: out } }).status; + return [status, readFileSync(out, "utf8")]; + }; + assert.deepEqual(hand("credit-stop"), [0, "claude-security-review=credit-stop\n"]); + assert.deepEqual(hand("no-key"), [0, "claude-security-review=no-key\n"]); + assert.notEqual(hand("")[0], 0); + assert.equal(hand("x\nclaude-review=auth")[1], ""); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("every later leg repeats the key leg's steps but for its credential and why it ran", () => { + const normal = (block) => + block + .replace(/secrets\.[A-Z_]+/g, "secrets.X") + .replace(/^( {10}SECRET_NAME: ).*$/m, "$1X") + .replace(new RegExp(`^( {10})(?:${CREDENTIAL_INPUTS.join("|")}): `, "m"), "$1credential: ") + .replace(/^ {10}LATER_PRESENT: .*\n/m, "") + .replace(/^ {10}fallback-ready: \$\{\{ steps\.credential\.outputs\.fallback-ready \}\}\n/m, "") + .replace(/\n {10}# Why this leg ran, for the lane's one record\.\n {10}fallback-reason: [^\n]*/, "") + .replace(/ && steps\.handoff\.outcome != 'success'|and handed nothing on\n {6}# fails here,|fails here,/g, ""); + const keyNames = STEPS[KEY].map((s) => s.name); + assert.deepEqual(STEPS[SUB].map((s) => s.name), keyNames); + 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' || '' }}", + ); + // Its gates are the key leg's. + const gates = (id) => /^ {4}if: >-\n((?: {6}.*\n)+)/m.exec(jobText(id))[1]; + assert.equal(gates(job), gates(KEY)); + } + assert.equal(input(step("Report the review's outcome", KEY), "fallback-reason"), undefined); +}); diff --git a/.github/scripts/pr-review-main/write-diff.py b/.github/scripts/pr-review-main/write-diff.py new file mode 100644 index 0000000000..3b3b500acf --- /dev/null +++ b/.github/scripts/pr-review-main/write-diff.py @@ -0,0 +1,87 @@ +"""Write the main-run review's patch for BASE...HEAD of a repository. + +usage: write-diff.py + +A file moved unchanged (git's 100% similarity) prints as its `rename from` and +`rename to` lines alone, so a pull request of pure moves stays inside the +review's size cap. Every other change prints whole, as `--no-renames` shows +it: an edited rename as a delete and an add, and a move into or out of a lane +source (babysit_review_evidence.lane_source, the gate's own predicate) or one +that changes the file mode as its old file deleted and its new file added, +so the reviewer reads what the lane source holds. `--text` shows a file git +would call binary as text, and no attribute, diff driver or textconv in the +repository runs. The caller applies its binary, size and line caps to the +result. +""" + +import os +import subprocess +import sys +from pathlib import Path + +sys.path.insert( + 0, + str( + Path(__file__).resolve().parents[3] + / "plugins/source-control/skills/babysit-prs/scripts" + ), +) +import babysit_review_evidence as ev # noqa: E402 + +DIFF = ["diff", "--text", "--no-ext-diff", "--no-textconv", "--no-color"] + + +def git(repo: str, *args: str) -> bytes: + return subprocess.run( + ["git", "-C", repo, *args], check=True, stdout=subprocess.PIPE + ).stdout + + +def whole_paths(raw: bytes) -> list[str]: + """The paths of each exact rename in `git diff --raw -z -M100%` output that + must print whole: a lane source on either side, or a mode change.""" + fields = raw.split(b"\0") + paths: list[str] = [] + i = 0 + while i < len(fields) and fields[i]: + old_mode, new_mode, _, _, status = ( + fields[i].decode("ascii").lstrip(":").split(" ") + ) + if status[0] in "RC": + src, dst = (os.fsdecode(f) for f in fields[i + 1 : i + 3]) + i += 3 + if ( + status != "R100" + or old_mode != new_mode + or ev.lane_source(src) + or ev.lane_source(dst) + ): + paths += [src, dst] + else: + i += 2 + return paths + + +def write_diff(repo: str, base: str, head: str) -> bytes: + commits = f"{base}...{head}" + whole = whole_paths( + git(repo, "diff", "--raw", "-z", "-M100%", "--no-ext-diff", commits) + ) + patch = git( + repo, *DIFF, "-M100%", commits, "--", *(f":(exclude,literal){p}" for p in whole) + ) + if whole: + patch += git( + repo, + *DIFF, + "--no-renames", + commits, + "--", + *(f":(literal){p}" for p in whole), + ) + return patch + + +if __name__ == "__main__": + repo, base, head, out = sys.argv[1:5] + Path(out).write_bytes(write_diff(repo, base, head)) diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 766b97415f..87645cb328 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -18,9 +18,24 @@ name: pr-review-main # success). The review job's token can only read; the record job, which runs # no model, posts only each verdict's counts as a check run. # -# One leg per lane, on the reviews role's API key. A run that leaves no -# verdict (a handoff, a kill switch, a failure) leaves the lane unbound, as -# it was before this workflow, and the next push reviews again. +# Each lane runs one leg on the reviews role's credentials, in the review +# lanes' order (pr-review-hosted.yml's secrets): the review job on the API +# key; review-subscription on the role's subscription token for a lane whose +# key leg handed off (run-claude-cli's handoff: a credit stop, a 401 or 402) +# or whose key is empty; review-fallback on FALLBACK_CLAUDE_CODE_OAUTH_TOKEN +# for a lane the subscription leg handed off (a limit stop, a credit stop, a +# 401 or 402, or an empty token, with the fallback token set). Each leg is +# its own job on a new VM holding one credential, and repeats every step from +# a fresh main checkout (workflow.test.mjs holds them equal). A matrix job's +# outputs merge, so each lane's handoff reason is its own output, set after +# run-claude-cli restored the step files; review-handoffs and +# subscription-handoffs, which hold no secret and run no model, check each +# reason against a closed set and give the next leg fixed lane rows. Every +# leg writes the same verdict, so the record job and the gate never see +# which leg ran; the lane record names the credential and why it ran. A +# lane no leg leaves a verdict for (a kill switch, a failure, the last leg +# stopping) stays unbound, as it was before this workflow, and the next push +# reviews again. # # Every local action runs from the main checkout at ./.base. zizmor's # self-repository advice (`$/...`) is ignored on those lines, as in @@ -72,6 +87,11 @@ jobs: contents: read # check out main; read the PR's commits for the diff pull-requests: read # the PR, its files, comments and reviews actions: read # the upstream run the trigger gate reads + # One output per lane, since a matrix job's outputs merge: why the lane + # was handed to the next leg, empty when it was not. + outputs: + claude-review: ${{ steps.handoff.outputs.claude-review }} + claude-security-review: ${{ steps.handoff.outputs.claude-security-review }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -145,10 +165,13 @@ jobs: # A blobless fetch of the two commits into its own repository outside # the workspace. git diff reads no attributes or diff drivers from it, - # so nothing in the PR runs; the patch is a data file. `--text` shows a - # file git would call binary (a NUL byte early in a script) as text, so - # the reviewer sees what node or bash would run, and `--no-renames` - # shows a renamed file whole, so a partial rename into a lane source is + # so nothing in the PR runs; the patch is a data file (write-diff.py). + # `--text` shows a file git would call binary (a NUL byte early in a + # script) as text, so the reviewer sees what node or bash would run. A + # file moved unchanged prints as its rename lines alone, so pure moves + # stay under the size cap; an edited rename, a move into or out of a + # lane source and a move that changes the mode print whole, as + # `--no-renames` shows them, so a partial rename into a lane source is # reviewed as the new file. A patch that still # shows a binary file, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree @@ -181,7 +204,7 @@ jobs: git init -q "$SRC" git -C "$SRC" remote add origin "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY" git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" - git -C "$SRC" diff --text --no-renames --no-ext-diff --no-textconv --no-color "$BASE_SHA...$HEAD_SHA" >"$DATA/diff.patch" + python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" exit 1 @@ -256,18 +279,29 @@ jobs: model-route: ${{ matrix.route }} output-path: ${{ runner.temp }}/pr-review-main-models.json - # Only whether the secret is set leaves this step, never its value. + # Only whether the secrets are set leaves this step, never a value. An + # empty credential with a later leg's set hands the lane on; with none + # set, nothing reviews and the lane stays unbound. - name: Check the credential id: credential if: steps.diff.outcome == 'success' env: PRESENT: ${{ secrets.REVIEWS_CLAUDE_API_KEY != '' }} + SECRET_NAME: REVIEWS_CLAUDE_API_KEY + # Whether a later leg can review, never its token. The key leg + # always hands an empty key on: a step reads no key beside a token, + # and the subscription leg checks its own and the fallback's. + LATER_PRESENT: "true" run: | if [ "$PRESENT" = true ]; then echo "present=true" >>"$GITHUB_OUTPUT" + elif [ "$LATER_PRESENT" = true ]; then + echo "::warning::$SECRET_NAME is empty; the next leg reviews this lane." + echo "present=false" >>"$GITHUB_OUTPUT" else - echo "::warning::REVIEWS_CLAUDE_API_KEY is empty; no main-run review, so the lane stays unbound." + echo "::warning::$SECRET_NAME is empty and no later leg's credential is set; no main-run review, so the lane stays unbound." fi + echo "fallback-ready=$LATER_PRESENT" >>"$GITHUB_OUTPUT" # run-claude-cli refuses to run without these root-owned settings, which # need sudo, so they are written before the runner is isolated. @@ -315,7 +349,9 @@ jobs: # may share its job with the upload: it has Read alone and the job # token can only read. # continue-on-error keeps a launcher failure from ending the job before - # the outcome report; "Fail without a verdict" carries the red. + # 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 if: steps.prompt.outcome == 'success' @@ -354,10 +390,917 @@ jobs: json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json leg-timeout-minutes: "15" - # One lane record per leg, as pr-verify.yml keeps one, so this lane's - # model legs show in lane telemetry and spend. + # 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' + 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 }} + 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 }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review.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 != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review.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: 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.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 key leg handed on, as review-subscription's matrix: fixed + # lane rows, each reason checked against the closed set the key leg hands + # on with. It holds no secret, runs no model and checks nothing out. + review-handoffs: + needs: review + if: >- + github.ref == 'refs/heads/main' + && (needs.review.outputs.claude-review != '' || needs.review.outputs.claude-security-review != '') + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + matrix: ${{ steps.route.outputs.matrix }} + steps: + - name: Route the handed-off lanes + id: route + env: + CODE: ${{ needs.review.outputs.claude-review }} + SECURITY: ${{ needs.review.outputs.claude-security-review }} + REASONS: credit-stop auth no-key + run: | + matrix=$(jq -cn --arg code "$CODE" --arg security "$SECURITY" --arg reasons "$REASONS" ' + ($reasons | split(" ")) as $ok + | [{lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED", reason: $code}, + {lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED", reason: $security}] + | map(select(.reason != "")) + | if all(.[]; .reason as $r | any($ok[]; . == $r)) then {include: .} else error("a handoff reason is not one this leg hands on with") end') + echo "matrix=$matrix" >>"$GITHUB_OUTPUT" + echo "handed on: $(jq -r '[.include[].lane] | join(", ")' <<<"$matrix")" + + # The subscription leg, one per lane the key leg handed on, on a new VM: + # the key leg's processes, files and environment do not reach it. Its steps + # repeat the review job's from a fresh main checkout but for the + # credential, fallback-ready and the fallback reason (workflow.test.mjs + # holds them equal), so the gates, the kill switch and the hardening run + # again. Only this job's launcher step gets the subscription token. + review-subscription: + needs: review-handoffs + if: >- + github.ref == 'refs/heads/main' + && github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.conclusion == 'success' + && github.event.workflow_run.head_repository.full_name == github.repository + strategy: + fail-fast: false + matrix: ${{ fromJSON(needs.review-handoffs.outputs.matrix) }} + runs-on: ubuntu-24.04 + timeout-minutes: 30 + permissions: + contents: read # check out main; read the PR's commits for the diff + pull-requests: read # the PR, its files, comments and reviews + actions: read # the upstream run the trigger gate reads + # One output per lane, since a matrix job's outputs merge: why the lane + # was handed to the next leg, empty when it was not. + outputs: + claude-review: ${{ steps.handoff.outputs.claude-review }} + claude-security-review: ${{ steps.handoff.outputs.claude-security-review }} + steps: + - name: Check out the default branch tip + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: refs/heads/main + path: .base + persist-credentials: false + + - name: Set up Node + uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version-file: .base/.node-version + package-manager-cache: false + + - name: Check the kill switch + id: kill + uses: ./.base/.github/actions/check-kill-switch # zizmor: ignore[self-repository] + with: + value: ${{ vars.CLAUDE_LANES_DISABLED }} + repo-value: ${{ vars.CLAUDE_LANES_REPO_DISABLED }} + + - name: Report the kill switch + id: switch + env: + PROCEED: ${{ steps.kill.outputs.proceed }} + REASON: ${{ steps.kill.outputs.reason }} + LANE_SWITCH: ${{ vars[matrix.switch] }} + run: | + if [ "$PROCEED" = true ] && [ "$LANE_SWITCH" != true ]; then + echo "proceed=true" >>"$GITHUB_OUTPUT" + else + echo "::notice::a kill switch stops this review (lanes: ${REASON:-unreadable}, lane: ${LANE_SWITCH:-unset}); no verdict, so the lane stays unbound" + fi + + # The upstream run's actors, the re-runner and the PR author must all be + # on the trusted-actor list. + - name: Check the trigger + id: gate + if: steps.switch.outputs.proceed == 'true' + env: + API_BUDGET_TOKEN: github-token + uses: ./.base/.github/actions/check-trusted-trigger # zizmor: ignore[self-repository] + with: + trusted-actors-path: .base/.github/standards/trusted-actors/trusted-actors.json + triggering-actor: ${{ github.triggering_actor }} + reconciler-workflow: .github/workflows/maintenance-reconcile-lost-runs.yml + ci-status-rerun-workflow: .github/workflows/pr-feedback-dispatch.yml + + # Only a pull request that changes a lane source is reviewed here; the + # predicate is the merge gate's own, read from main. + - name: Select a lane-source pull request + id: select + if: steps.gate.outputs.proceed == 'true' + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + FILES: ${{ runner.temp }}/pr-review-main-files.txt + run: | + gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files?per_page=100" \ + --jq '.[] | .filename, (.previous_filename // empty)' >"$FILES" + review=$(python3 -I -c ' + import sys + sys.path.insert(0, ".base/plugins/source-control/skills/babysit-prs/scripts") + import babysit_review_evidence as ev + paths = [line.rstrip("\n") for line in open(sys.argv[1], encoding="utf-8")] + # The files API lists at most 3000; a longer list may hide one. + print("true" if len(paths) >= 3000 or any(ev.lane_source(p) for p in paths) else "false") + ' "$FILES") + echo "review=$review" >>"$GITHUB_OUTPUT" + echo "lane source changed: $review" + + # A blobless fetch of the two commits into its own repository outside + # the workspace. git diff reads no attributes or diff drivers from it, + # so nothing in the PR runs; the patch is a data file (write-diff.py). + # `--text` shows a file git would call binary (a NUL byte early in a + # script) as text, so the reviewer sees what node or bash would run. A + # file moved unchanged prints as its rename lines alone, so pure moves + # stay under the size cap; an edited rename, a move into or out of a + # lane source and a move that changes the mode print whole, as + # `--no-renames` shows them, so a partial rename into a lane source is + # reviewed as the new file. A patch that still + # shows a binary file, or is larger than one review can read whole, + # gets no verdict: the step fails and the lanes stay unbound. 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' + env: + HEAD_SHA: ${{ steps.gate.outputs.head-sha }} + BASE_SHA: ${{ steps.gate.outputs.base-sha }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + DATA: ${{ github.workspace }}/pr-review-main-data + SRC: ${{ runner.temp }}/pr-review-main-src + # About 75k tokens of patch: room in the lane's turn budget to read + # it whole beside the criteria (judgment). + MAX_DIFF_BYTES: "300000" + # 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 + # numbers comes back whole. + PART_BYTES: "15000" + PART_LINES: "1000" + # Read may cut a long line short, so a diff with a longer line gets + # no verdict, and the merge gate holds it for a person + # (babysit_risk LONG_LINE_BYTES, `long-line-review:`). + LONG_LINE_BYTES: "1800" + run: | + export GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null + mkdir -p "$DATA" + git init -q "$SRC" + git -C "$SRC" remote add origin "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY" + git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" + python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" + if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then + echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" + exit 1 + fi + if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then + echo "::error::the diff is over $MAX_DIFF_BYTES bytes, more than one review reads whole; no verdict, so the lanes stay unbound" + exit 1 + 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 + echo "::error::a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole; no verdict, so the lanes stay unbound and the merge gate holds it for a person" + exit 1 + 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. + 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" + exit 1 + fi + rm "$DATA/diff.patch" + if [ -z "$(find "$DATA/diff" -type f)" ]; then + echo "::error::the diff is empty; no verdict, so the lanes stay unbound" + exit 1 + fi + 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" + + - name: Select the trusted PR text + if: steps.diff.outcome == 'success' + env: + API_BUDGET_TOKEN: github-token + uses: ./.base/.github/actions/select-trusted-text # zizmor: ignore[self-repository] + with: + pr-number: ${{ steps.gate.outputs.pr-number }} + trusted-actors-path: .base/.github/standards/trusted-actors/trusted-actors.json + output-path: ${{ github.workspace }}/pr-review-main-data/context.json + github-token: ${{ github.token }} + + # The lane's criteria and the repository's standards, from main. The + # skill keeps its place under the plugin's layout, so a relative pointer + # in it (`../../reference/...`) resolves inside the data directory; the + # rules the code lane names go beside the standards. + - name: Copy the review criteria + if: steps.diff.outcome == 'success' + env: + SKILL: ${{ matrix.skill }} + DATA: ${{ github.workspace }}/pr-review-main-data + run: | + mkdir -p "$DATA/review/skills" "$DATA/standards" + cp -R .base/plugins/review/skills/"$SKILL" "$DATA/review/skills/" + rm -rf "$DATA/review/skills/$SKILL/evals" + cp -R .base/plugins/review/reference "$DATA/review/reference" + cp .base/REVIEW.md "$DATA/standards/" + cp -R .base/docs/standards "$DATA/standards/standards" + if [ -d .base/.claude/rules ]; then cp -R .base/.claude/rules "$DATA/standards/rules"; fi + + - name: Resolve the review's model + id: models + if: steps.diff.outcome == 'success' + uses: ./.base/.github/actions/resolve-config # zizmor: ignore[self-repository] + with: + model-route: ${{ matrix.route }} + output-path: ${{ runner.temp }}/pr-review-main-models.json + + # Only whether the secrets are set leaves this step, never a value. An + # empty credential with a later leg's set hands the lane on; with none + # set, nothing reviews and the lane stays unbound. + - name: Check the credential + id: credential + if: steps.diff.outcome == 'success' + env: + PRESENT: ${{ secrets.REVIEWS_CLAUDE_CODE_OAUTH_TOKEN != '' }} + SECRET_NAME: REVIEWS_CLAUDE_CODE_OAUTH_TOKEN + # Whether a later leg can review, never its token. The key leg + # always hands an empty key on: a step reads no key beside a token, + # and the subscription leg checks its own and the fallback's. + LATER_PRESENT: ${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN != '' }} + run: | + if [ "$PRESENT" = true ]; then + echo "present=true" >>"$GITHUB_OUTPUT" + elif [ "$LATER_PRESENT" = true ]; then + echo "::warning::$SECRET_NAME is empty; the next leg reviews this lane." + echo "present=false" >>"$GITHUB_OUTPUT" + else + echo "::warning::$SECRET_NAME is empty and no later leg's credential is set; no main-run review, so the lane stays unbound." + fi + echo "fallback-ready=$LATER_PRESENT" >>"$GITHUB_OUTPUT" + + # run-claude-cli refuses to run without these root-owned settings, which + # need sudo, so they are written before the runner is isolated. + - name: Apply the Claude managed settings + if: steps.credential.outputs.present == 'true' + uses: melodic-software/ci-workflows/.github/actions/apply-claude-managed-settings@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + + - name: Isolate the runner + if: steps.credential.outputs.present == 'true' + uses: ./.base/.github/actions/setup-runner-isolation # zizmor: ignore[self-repository] + with: + install-subprocess-isolation: true + + # Files, not step outputs: run-claude-cli reads both. + - name: Write the prompt and schema + id: prompt + if: steps.credential.outputs.present == 'true' + env: + DATA: ${{ github.workspace }}/pr-review-main-data + LANE: ${{ matrix.lane }} + SKILL: ${{ matrix.skill }} + MODEL: ${{ steps.models.outputs.model }} + PROMPT_FILE: ${{ runner.temp }}/pr-review-main-prompt.md + 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" + 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" + + # Under dontAsk only the allow rule's directory is readable; the deny + # rules hold if the tools or the working directory change. The model + # may share its job with the upload: it has Read alone and the job + # token can only read. + # continue-on-error keeps a launcher failure from ending the job before + # 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 + if: steps.prompt.outcome == 'success' + 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.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" + + # 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' + 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 }} + 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 }} + max-turns: ${{ steps.models.outputs.max-turns }} + known-failure-class: ${{ steps.review.outputs.lane-class == 'credit-stop' && 'rate-limit' || steps.review.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 != '' + continue-on-error: true + uses: ./.base/.github/actions/upload-lane-transcript # zizmor: ignore[self-repository] + with: + execution-file: ${{ steps.review.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: 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.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: >- + github.ref == 'refs/heads/main' + && (needs.review-subscription.outputs.claude-review != '' || needs.review-subscription.outputs.claude-security-review != '') + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: {} + outputs: + matrix: ${{ steps.route.outputs.matrix }} + steps: + - name: Route the handed-off lanes + id: route + env: + CODE: ${{ needs.review-subscription.outputs.claude-review }} + SECURITY: ${{ needs.review-subscription.outputs.claude-security-review }} + REASONS: limit-stop credit-stop auth no-key + run: | + matrix=$(jq -cn --arg code "$CODE" --arg security "$SECURITY" --arg reasons "$REASONS" ' + ($reasons | split(" ")) as $ok + | [{lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED", reason: $code}, + {lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED", reason: $security}] + | map(select(.reason != "")) + | if all(.[]; .reason as $r | any($ok[]; . == $r)) then {include: .} else error("a handoff reason is not one this leg hands on with") end') + echo "matrix=$matrix" >>"$GITHUB_OUTPUT" + echo "handed on: $(jq -r '[.include[].lane] | join(", ")' <<<"$matrix")" + + # The fallback leg, on the central fallback token, one per lane the + # subscription leg handed on. Its steps repeat the review job's but for the + # credential and the fallback reason, and it hands nothing on: it is the + # last leg, so a stop here leaves the lane unbound and fails the job. + review-fallback: + needs: subscription-handoffs + if: >- + github.ref == 'refs/heads/main' + && github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.conclusion == 'success' + && github.event.workflow_run.head_repository.full_name == github.repository + strategy: + fail-fast: false + matrix: ${{ fromJSON(needs.subscription-handoffs.outputs.matrix) }} + runs-on: ubuntu-24.04 + timeout-minutes: 30 + permissions: + contents: read # check out main; read the PR's commits for the diff + pull-requests: read # the PR, its files, comments and reviews + actions: read # the upstream run the trigger gate reads + steps: + - name: Check out the default branch tip + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: refs/heads/main + path: .base + persist-credentials: false + + - name: Set up Node + uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 + with: + node-version-file: .base/.node-version + package-manager-cache: false + + - name: Check the kill switch + id: kill + uses: ./.base/.github/actions/check-kill-switch # zizmor: ignore[self-repository] + with: + value: ${{ vars.CLAUDE_LANES_DISABLED }} + repo-value: ${{ vars.CLAUDE_LANES_REPO_DISABLED }} + + - name: Report the kill switch + id: switch + env: + PROCEED: ${{ steps.kill.outputs.proceed }} + REASON: ${{ steps.kill.outputs.reason }} + LANE_SWITCH: ${{ vars[matrix.switch] }} + run: | + if [ "$PROCEED" = true ] && [ "$LANE_SWITCH" != true ]; then + echo "proceed=true" >>"$GITHUB_OUTPUT" + else + echo "::notice::a kill switch stops this review (lanes: ${REASON:-unreadable}, lane: ${LANE_SWITCH:-unset}); no verdict, so the lane stays unbound" + fi + + # The upstream run's actors, the re-runner and the PR author must all be + # on the trusted-actor list. + - name: Check the trigger + id: gate + if: steps.switch.outputs.proceed == 'true' + env: + API_BUDGET_TOKEN: github-token + uses: ./.base/.github/actions/check-trusted-trigger # zizmor: ignore[self-repository] + with: + trusted-actors-path: .base/.github/standards/trusted-actors/trusted-actors.json + triggering-actor: ${{ github.triggering_actor }} + reconciler-workflow: .github/workflows/maintenance-reconcile-lost-runs.yml + ci-status-rerun-workflow: .github/workflows/pr-feedback-dispatch.yml + + # Only a pull request that changes a lane source is reviewed here; the + # predicate is the merge gate's own, read from main. + - name: Select a lane-source pull request + id: select + if: steps.gate.outputs.proceed == 'true' + env: + GH_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + FILES: ${{ runner.temp }}/pr-review-main-files.txt + run: | + gh api --paginate "repos/$GITHUB_REPOSITORY/pulls/$PR_NUMBER/files?per_page=100" \ + --jq '.[] | .filename, (.previous_filename // empty)' >"$FILES" + review=$(python3 -I -c ' + import sys + sys.path.insert(0, ".base/plugins/source-control/skills/babysit-prs/scripts") + import babysit_review_evidence as ev + paths = [line.rstrip("\n") for line in open(sys.argv[1], encoding="utf-8")] + # The files API lists at most 3000; a longer list may hide one. + print("true" if len(paths) >= 3000 or any(ev.lane_source(p) for p in paths) else "false") + ' "$FILES") + echo "review=$review" >>"$GITHUB_OUTPUT" + echo "lane source changed: $review" + + # A blobless fetch of the two commits into its own repository outside + # the workspace. git diff reads no attributes or diff drivers from it, + # so nothing in the PR runs; the patch is a data file (write-diff.py). + # `--text` shows a file git would call binary (a NUL byte early in a + # script) as text, so the reviewer sees what node or bash would run. A + # file moved unchanged prints as its rename lines alone, so pure moves + # stay under the size cap; an edited rename, a move into or out of a + # lane source and a move that changes the mode print whole, as + # `--no-renames` shows them, so a partial rename into a lane source is + # reviewed as the new file. A patch that still + # shows a binary file, or is larger than one review can read whole, + # gets no verdict: the step fails and the lanes stay unbound. 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' + env: + HEAD_SHA: ${{ steps.gate.outputs.head-sha }} + BASE_SHA: ${{ steps.gate.outputs.base-sha }} + PR_NUMBER: ${{ steps.gate.outputs.pr-number }} + DATA: ${{ github.workspace }}/pr-review-main-data + SRC: ${{ runner.temp }}/pr-review-main-src + # About 75k tokens of patch: room in the lane's turn budget to read + # it whole beside the criteria (judgment). + MAX_DIFF_BYTES: "300000" + # 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 + # numbers comes back whole. + PART_BYTES: "15000" + PART_LINES: "1000" + # Read may cut a long line short, so a diff with a longer line gets + # no verdict, and the merge gate holds it for a person + # (babysit_risk LONG_LINE_BYTES, `long-line-review:`). + LONG_LINE_BYTES: "1800" + run: | + export GIT_CONFIG_NOSYSTEM=1 GIT_CONFIG_GLOBAL=/dev/null + mkdir -p "$DATA" + git init -q "$SRC" + git -C "$SRC" remote add origin "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY" + git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" + python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" + if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then + echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" + exit 1 + fi + if [ "$(wc -c <"$DATA/diff.patch")" -gt "$MAX_DIFF_BYTES" ]; then + echo "::error::the diff is over $MAX_DIFF_BYTES bytes, more than one review reads whole; no verdict, so the lanes stay unbound" + exit 1 + 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 + echo "::error::a diff line is over $LONG_LINE_BYTES bytes, more than the review reads whole; no verdict, so the lanes stay unbound and the merge gate holds it for a person" + exit 1 + 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. + 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" + exit 1 + fi + rm "$DATA/diff.patch" + if [ -z "$(find "$DATA/diff" -type f)" ]; then + echo "::error::the diff is empty; no verdict, so the lanes stay unbound" + exit 1 + fi + 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" + + - name: Select the trusted PR text + if: steps.diff.outcome == 'success' + env: + API_BUDGET_TOKEN: github-token + uses: ./.base/.github/actions/select-trusted-text # zizmor: ignore[self-repository] + with: + pr-number: ${{ steps.gate.outputs.pr-number }} + trusted-actors-path: .base/.github/standards/trusted-actors/trusted-actors.json + output-path: ${{ github.workspace }}/pr-review-main-data/context.json + github-token: ${{ github.token }} + + # The lane's criteria and the repository's standards, from main. The + # skill keeps its place under the plugin's layout, so a relative pointer + # in it (`../../reference/...`) resolves inside the data directory; the + # rules the code lane names go beside the standards. + - name: Copy the review criteria + if: steps.diff.outcome == 'success' + env: + SKILL: ${{ matrix.skill }} + DATA: ${{ github.workspace }}/pr-review-main-data + run: | + mkdir -p "$DATA/review/skills" "$DATA/standards" + cp -R .base/plugins/review/skills/"$SKILL" "$DATA/review/skills/" + rm -rf "$DATA/review/skills/$SKILL/evals" + cp -R .base/plugins/review/reference "$DATA/review/reference" + cp .base/REVIEW.md "$DATA/standards/" + cp -R .base/docs/standards "$DATA/standards/standards" + if [ -d .base/.claude/rules ]; then cp -R .base/.claude/rules "$DATA/standards/rules"; fi + + - name: Resolve the review's model + id: models + if: steps.diff.outcome == 'success' + uses: ./.base/.github/actions/resolve-config # zizmor: ignore[self-repository] + with: + model-route: ${{ matrix.route }} + output-path: ${{ runner.temp }}/pr-review-main-models.json + + # Only whether the secrets are set leaves this step, never a value. An + # empty credential with a later leg's set hands the lane on; with none + # set, nothing reviews and the lane stays unbound. + - name: Check the credential + id: credential + if: steps.diff.outcome == 'success' + env: + PRESENT: ${{ secrets.FALLBACK_CLAUDE_CODE_OAUTH_TOKEN != '' }} + SECRET_NAME: FALLBACK_CLAUDE_CODE_OAUTH_TOKEN + # Whether a later leg can review, never its token. The key leg + # always hands an empty key on: a step reads no key beside a token, + # and the subscription leg checks its own and the fallback's. + LATER_PRESENT: "false" + run: | + if [ "$PRESENT" = true ]; then + echo "present=true" >>"$GITHUB_OUTPUT" + elif [ "$LATER_PRESENT" = true ]; then + echo "::warning::$SECRET_NAME is empty; the next leg reviews this lane." + echo "present=false" >>"$GITHUB_OUTPUT" + else + echo "::warning::$SECRET_NAME is empty and no later leg's credential is set; no main-run review, so the lane stays unbound." + fi + echo "fallback-ready=$LATER_PRESENT" >>"$GITHUB_OUTPUT" + + # run-claude-cli refuses to run without these root-owned settings, which + # need sudo, so they are written before the runner is isolated. + - name: Apply the Claude managed settings + if: steps.credential.outputs.present == 'true' + uses: melodic-software/ci-workflows/.github/actions/apply-claude-managed-settings@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 + + - name: Isolate the runner + if: steps.credential.outputs.present == 'true' + uses: ./.base/.github/actions/setup-runner-isolation # zizmor: ignore[self-repository] + with: + install-subprocess-isolation: true + + # Files, not step outputs: run-claude-cli reads both. + - name: Write the prompt and schema + id: prompt + if: steps.credential.outputs.present == 'true' + env: + DATA: ${{ github.workspace }}/pr-review-main-data + LANE: ${{ matrix.lane }} + SKILL: ${{ matrix.skill }} + MODEL: ${{ steps.models.outputs.model }} + PROMPT_FILE: ${{ runner.temp }}/pr-review-main-prompt.md + 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" + 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" + + # Under dontAsk only the allow rule's directory is readable; the deny + # rules hold if the tools or the working directory change. The model + # may share its job with the upload: it has Read alone and the job + # token can only read. + # continue-on-error keeps a launcher failure from ending the job before + # 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 + if: steps.prompt.outcome == 'success' + 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.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" + + # 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' + if: (steps.review.outcome == 'success' || steps.review.outcome == 'failure') && steps.review.outputs.handoff != 'true' uses: melodic-software/ci-workflows/.github/actions/report-lane-outcome@81a603a6e2b0319428c981bf28f3fe923032afb2 # v0.65.17 with: outcome: ${{ steps.review.outcome }} @@ -370,6 +1313,8 @@ jobs: credential: ${{ steps.review.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 }} + # 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. @@ -430,8 +1375,7 @@ jobs: with: tokens: github-token - # A reviewed pull request that left no verdict fails here, so the run - # shows it; the lane stays unbound either way. + # A reviewed pull request that left no verdict 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.credential.outputs.present == 'true' && steps.verdict.outcome != 'success' run: | @@ -446,7 +1390,7 @@ jobs: # a hold. No model runs in this job; it reads only the counts that # write-verdict.mjs checked, from this run's own artifacts. record: - needs: review + needs: [review, review-subscription, review-fallback] if: ${{ !cancelled() && github.ref == 'refs/heads/main' }} runs-on: ubuntu-24.04 timeout-minutes: 5 diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 1542cc65fa..77caecf517 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1078,8 +1078,11 @@ the rules that only tighten in 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 whole diff) for a person as `long-line-review:`, - and the binder counts no verdict whose recorded `max_line_bytes` is past the cap. The diff - uses `--no-renames`, so a renamed file is reviewed whole. A verdict binds only when the head + and the binder counts no verdict whose recorded `max_line_bytes` is past the cap. A file moved + unchanged (git's 100% similarity) shows as its rename lines alone, so pure moves stay under the + size cap; an edited rename, a move into or out of a lane source (`lane_source`) and a move that + changes the file mode show whole, as `--no-renames` would (`write-diff.py`), and the caps apply + after. A verdict binds only when the head also carries the record job's `pr-review-main: ` check run with the same counts, so a run cancelled before it recorded binds nothing. Each lane uploads `verdict.json` as @@ -1100,8 +1103,17 @@ the rules that only tighten in an artifact or a comment, not a check run, and a forged one can only add a hold. A count that cannot be read leaves the lane unbound. With no such verdict the lanes stay unbound (`lane-source-changed`), a lane's reason, and a floor `enforcement` path beside it waits for that review. Each lane runs one leg on the reviews - role's API key and leaves a lane record (`report-lane-outcome`); a run that leaves no verdict - leaves the lanes unbound until the next push. + role's credentials in the review lanes' order: the API key (`REVIEWS_CLAUDE_API_KEY`); then, + for a lane whose key leg handed off (a credit stop, a 401 or 402) or whose key is empty, the + subscription token (`REVIEWS_CLAUDE_CODE_OAUTH_TOKEN`) in the `review-subscription` job; then, + for a lane that leg handed off (a limit stop, a credit stop, a 401 or 402, or an empty token) + with `FALLBACK_CLAUDE_CODE_OAUTH_TOKEN` set, the fallback token in `review-fallback`. Each leg + is a job on a new VM holding one credential, and a route job with no secret passes on only + lanes whose handoff reason is in a closed set. Every leg writes the same verdict, so neither the + record job nor the gate depends on the leg; the lane record (`report-lane-outcome`) names the + credential and why the leg ran. A handed-off leg stays green, so a later leg's verdict still + comes from a successful run; a run that leaves no verdict leaves the lanes unbound until the + next push. `merge.diff-check.denied-paths` (empty here) joins the person classes. `test_merge_holds_sync.py` derives the JS and Python import closures and requires them on the From 267e01e325791388c0a1e7ed6fa95b974746a5db Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:45:06 -0400 Subject: [PATCH 2/4] fix(pr-pipeline): collapse renames only over the size cap and show referenced moves Security review of the main-run review diff and leg chain: - write-diff.py prints today's whole-file --no-renames patch unless it is over MAX_DIFF_BYTES. Only then do exact renames collapse, and a move whose new path (or its last directory and name) an added line names prints whole, repeated until no shown line names a collapsed move. - A move of a symlink (120000) or submodule (160000) prints whole. - The prompt says a rename-only entry's content is unseen and a change referencing a moved path is a finding. - The route jobs run on !cancelled(), so one lane's failed leg does not drop the other lane's handoff, and every leg checks out github.sha, the main commit whose copy of the workflow runs. Co-Authored-By: Claude Opus 5.5 --- ...in-run-review-credential-chain-7c4e19a2.md | 2 +- .github/scripts/pr-review-main/prompt.md | 5 +- .../scripts/pr-review-main/workflow.test.mjs | 110 +++++++++++++++--- .github/scripts/pr-review-main/write-diff.py | 84 ++++++++----- .github/workflows/pr-review-main.yml | 60 +++++----- docs/conventions/pr-pipeline/README.md | 12 +- 6 files changed, 198 insertions(+), 75 deletions(-) diff --git a/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md b/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md index 952baa52ed..2fb2b7b3ab 100644 --- a/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md +++ b/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md @@ -5,4 +5,4 @@ bump: patch ### Fixed - The main-run review (`pr-review-main.yml`) runs on the review lanes' credential chain instead of the API key alone: a lane whose key leg credit-stops, is refused with a 401 or 402, or has no key is reviewed on the reviews role's subscription token, and a lane that leg hands off (a limit stop, a credit stop, a 401 or 402, or no token) on `FALLBACK_CLAUDE_CODE_OAUTH_TOKEN`. Each leg is its own job holding one credential, writes the same verdict, and stays green when it hands off, so an exhausted key no longer leaves every lane-source pull request unbound. -- The main-run review's diff shows a file moved unchanged as its rename lines alone, so a pull request of pure moves stays under the review's size cap. An edited rename, a move into or out of a lane source and a move that changes the file mode still show whole. +- When the main-run review's whole-file diff is over its size cap, a file moved unchanged shows as its rename lines alone, so a pull request of pure moves fits. A move into or out of a lane source, a mode change, a moved symlink or submodule, and a move whose new path an added line names still show whole, and the reviewer is told a rename-only entry is unseen. diff --git a/.github/scripts/pr-review-main/prompt.md b/.github/scripts/pr-review-main/prompt.md index c8ddbeface..2d74559628 100644 --- a/.github/scripts/pr-review-main/prompt.md +++ b/.github/scripts/pr-review-main/prompt.md @@ -22,7 +22,10 @@ denied, and a denied read voids the review: `CRITERIA_DIR`, where the lane's relative links point. - The pull request's diff, from its base to the reviewed head, split into these parts in order. Read every part whole, each with one Read call and no `offset` or `limit`: a review that skips - a part, or reads one in pieces, is void. + 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. 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/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index 22333e1a87..6bc2fc7116 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -6,7 +6,7 @@ // order, a later leg only for a lane the previous one handed on. import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, symlinkSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { test } from "node:test"; @@ -80,7 +80,7 @@ test("the model's jobs can only read, and the one writer runs no model", () => { test("the diff shows every file as text and refuses one a review cannot read whole", () => { const diff = step("Write the diff"); - assert.match(diff, /python3 -I \.base\/\.github\/scripts\/pr-review-main\/write-diff\.py "\$SRC" "\$BASE_SHA" "\$HEAD_SHA" "\$DATA\/diff\.patch"/); + 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"/); @@ -131,7 +131,8 @@ test("a diff with a line past the review's cap gets no verdict", async () => { // write-diff.py run on a fixture repository: each case is a commit on top of // a base, and the patch it prints for base...head. -function diffCase(build) { +// A cap of 0 makes every patch over it, so moves may collapse. +function diffCase(build, maxBytes = 0) { const src = mkdtempSync(path.join(tmpdir(), "pr-review-main-rename-")); try { const env = { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1" }; @@ -146,7 +147,11 @@ function diffCase(build) { writeFileSync(path.join(src, file), body); }; git("init", "-q"); - build.base(write); + const symlink = (target, file) => { + mkdirSync(path.dirname(path.join(src, file)), { recursive: true }); + symlinkSync(target, path.join(src, file)); + }; + build.base(write, symlink); git("add", "-A"); git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "base"); const base = git("rev-parse", "HEAD"); @@ -155,7 +160,7 @@ function diffCase(build) { git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "head"); const head = git("rev-parse", "HEAD"); const out = path.join(src, "..", `${path.basename(src)}.patch`); - const run = spawnSync("python3", ["-I", WRITE_DIFF, src, base, head, out], { encoding: "utf8", env }); + const run = spawnSync("python3", ["-I", WRITE_DIFF, src, base, head, out, String(maxBytes)], { encoding: "utf8", env }); assert.equal(run.status, 0, run.stderr); const patch = readFileSync(out, "utf8"); rmSync(out); @@ -166,15 +171,27 @@ function diffCase(build) { } const lines = (prefix, n) => `${Array.from({ length: n }, (_, i) => `${prefix} ${i}`).join("\n")}\n`; -test("a file moved unchanged outside the lane sources prints as its rename lines alone", () => { - const patch = diffCase({ - base: (write) => { - for (let i = 0; i < 5; i++) write(`docs/a${i}.md`, lines(`file ${i} line`, 400)); - }, - head: (git) => { - for (let i = 0; i < 5; i++) git("mv", `docs/a${i}.md`, `guide/a${i}.md`); - }, - }); +const fiveMoves = { + base: (write) => { + for (let i = 0; i < 5; i++) write(`docs/a${i}.md`, lines(`file ${i} line`, 400)); + }, + head: (git) => { + for (let i = 0; i < 5; i++) git("mv", `docs/a${i}.md`, `guide/a${i}.md`); + }, +}; + +test("under the size cap every move prints whole, as --no-renames shows it", () => { + // Five 400-line files, each about 7 KB as added and again as deleted. + const patch = diffCase(fiveMoves, 300000); + assert.doesNotMatch(patch, /^rename from/m); + for (let i = 0; i < 5; i++) { + assert.equal(patch.match(new RegExp(`^\\+file ${i} line \\d+$`, "gm")).length, 400); + assert.equal(patch.match(new RegExp(`^-file ${i} line \\d+$`, "gm")).length, 400); + } +}); + +test("over the size cap a file moved unchanged outside the lane sources prints as its rename lines alone", () => { + const patch = diffCase(fiveMoves, 1000); // Five moves of 400 lines each: five headers, no content line. assert.equal(patch.match(/^rename from docs\/a\d\.md$/gm).length, 5); assert.equal(patch.match(/^rename to guide\/a\d\.md$/gm).length, 5); @@ -182,6 +199,45 @@ test("a file moved unchanged outside the lane sources prints as its rename lines assert.ok(patch.length < 1000, `${patch.length} bytes`); }); +test("over the size cap a move an added line names prints whole, and so does one that move names", () => { + const patch = diffCase({ + base: (write) => { + write("tools/setup.sh", `source x/helper.sh\n${lines("hostile", 20)}`); + write("lib/x/helper.sh", lines("helper", 20)); + write("lib/quiet.md", lines("quiet", 20)); + write("ci.yml", "steps: []\n"); + }, + head: (git, write) => { + git("mv", "tools/setup.sh", "build/setup.sh"); + git("mv", "lib/x/helper.sh", "build/x/helper.sh"); + git("mv", "lib/quiet.md", "docs/quiet.md"); + // The edit names the first move by its whole path; that move, printed + // whole, names the second by its last directory and name. + write("ci.yml", "steps: []\nrun: bash build/setup.sh\n"); + }, + }); + for (const word of ["hostile", "helper"]) { + assert.equal(patch.match(new RegExp(`^\\+${word} \\d+$`, "gm")).length, 20, word); + } + assert.match(patch, /^\+source x\/helper\.sh$/m); + // The move nothing names still collapses. + assert.match(patch, /^rename to docs\/quiet\.md$/m); + assert.doesNotMatch(patch, /^\+quiet /m); + assert.doesNotMatch(patch, /^rename to build\//m); +}); + +test("over the size cap a moved symlink prints whole", () => { + const patch = diffCase({ + base: (_write, symlink) => symlink("../../etc/hostile", "links/run"), + head: (git) => git("mv", "links/run", "bin/run"), + }); + // The link's target shows on both sides, as --no-renames prints it. + assert.doesNotMatch(patch, /^rename from/m); + assert.match(patch, /^new file mode 120000$/m); + assert.match(patch, /^\+\.\.\/\.\.\/etc\/hostile$/m); + assert.match(patch, /^-\.\.\/\.\.\/etc\/hostile$/m); +}); + test("a move into or out of a lane source, or one that changes the mode, prints whole", () => { const patch = diffCase({ base: (write) => { @@ -235,7 +291,9 @@ test("it checks out main only, and runs local actions from that checkout", () => for (const job of LEGS) { const checkouts = STEPS[job].filter((s) => /uses: actions\/checkout@/.test(s.block)); assert.equal(checkouts.length, 1, job); - assert.match(checkouts[0].block, /^ {10}ref: refs\/heads\/main$/m); + // workflow_run's github.sha is the default branch commit whose copy of + // this file runs, so every leg checks out that same main. + assert.match(checkouts[0].block, /^ {10}ref: \$\{\{ github\.sha \}\}$/m); assert.match(checkouts[0].block, /^ {10}persist-credentials: false$/m); } for (const [, local] of text.matchAll(/^\s+uses: (\.\/\S+)/gm)) { @@ -412,7 +470,10 @@ test("a later leg runs only for the lanes the previous leg handed on", () => { const routeJob = jobText(route); assert.equal(needsOf(route), from); assert.match(routeJob, new RegExp(`&& \\(needs\\.${from}\\.outputs\\.claude-review != '' \\|\\| needs\\.${from}\\.outputs\\.claude-security-review != ''\\)`)); - assert.doesNotMatch(routeJob, /always\(\)|!cancelled\(\)/); + // !cancelled(): one lane's failed leg does not drop the other lane's + // handoff; the route still passes on only closed-set reasons. + assert.match(routeJob, /^ {4}if: >-\n {6}!cancelled\(\)\n {6}&& github\.ref == 'refs\/heads\/main'\n/m); + assert.doesNotMatch(routeJob, /always\(\)/); const leg = jobText(to); assert.equal(needsOf(to), route); assert.match(leg, new RegExp(`^ {6}matrix: \\$\\{\\{ fromJSON\\(needs\\.${route}\\.outputs\\.matrix\\) \\}\\}$`, "m")); @@ -486,3 +547,20 @@ test("every later leg repeats the key leg's steps but for its credential and why } assert.equal(input(step("Report the review's outcome", KEY), "fallback-reason"), undefined); }); + +test("one lane handed on beside one that reviewed routes only the handed-on lane", () => { + for (const job of Object.keys(ROUTES)) { + // The security lane reviewed (no output); the code lane handed on. + const { status, matrix } = routeRun(job)({ CODE: "credit-stop", SECURITY: "" }); + assert.equal(status, 0, job); + assert.deepEqual(matrix.include.map((row) => [row.lane, row.reason]), [["claude-review", "credit-stop"]], job); + } +}); + +test("the prompt says a rename-only entry is unseen and a change that references a moved path is a finding", () => { + const flat = prompt.replace(/\s+/g, " "); + assert.ok(flat.includes("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.")); + assert.ok(flat.includes("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")); + // It sits with the diff parts, before the placeholder they replace. + assert.ok(prompt.indexOf("rename from") < prompt.indexOf("DIFF_PARTS")); +}); diff --git a/.github/scripts/pr-review-main/write-diff.py b/.github/scripts/pr-review-main/write-diff.py index 3b3b500acf..ef35ac2a33 100644 --- a/.github/scripts/pr-review-main/write-diff.py +++ b/.github/scripts/pr-review-main/write-diff.py @@ -1,14 +1,17 @@ """Write the main-run review's patch for BASE...HEAD of a repository. -usage: write-diff.py +usage: write-diff.py -A file moved unchanged (git's 100% similarity) prints as its `rename from` and -`rename to` lines alone, so a pull request of pure moves stays inside the -review's size cap. Every other change prints whole, as `--no-renames` shows -it: an edited rename as a delete and an add, and a move into or out of a lane -source (babysit_review_evidence.lane_source, the gate's own predicate) or one -that changes the file mode as its old file deleted and its new file added, -so the reviewer reads what the lane source holds. `--text` shows a file git +The patch is `--no-renames`, so every file prints whole, unless that patch is +over max-bytes, the review's size cap. Then a file moved unchanged (git's 100% +similarity) prints as its `rename from` and `rename to` lines alone, so a pull +request of pure moves fits, and every other change still prints whole. A move +prints whole even then when the reviewer could not judge it from the other +changes: a move into or out of a lane source +(babysit_review_evidence.lane_source, the gate's own predicate), one that +changes the file mode or moves a symlink or a submodule, and one whose new +path (or its last directory and name) an added line of the patch names, so a +change that runs a moved file shows what it runs. `--text` shows a file git would call binary as text, and no attribute, diff driver or textconv in the repository runs. The caller applies its binary, size and line caps to the result. @@ -29,6 +32,8 @@ import babysit_review_evidence as ev # noqa: E402 DIFF = ["diff", "--text", "--no-ext-diff", "--no-textconv", "--no-color"] +# A symlink's or a submodule's mode: the rename lines show neither target. +LINK_MODES = {"120000", "160000"} def git(repo: str, *args: str) -> bytes: @@ -37,11 +42,12 @@ def git(repo: str, *args: str) -> bytes: ).stdout -def whole_paths(raw: bytes) -> list[str]: - """The paths of each exact rename in `git diff --raw -z -M100%` output that - must print whole: a lane source on either side, or a mode change.""" +def renames(raw: bytes) -> tuple[list[str], list[tuple[str, str]]]: + """From `git diff --raw -z -M100%` output, the paths of each rename that + must print whole, and the pure moves that may collapse.""" fields = raw.split(b"\0") - paths: list[str] = [] + whole: list[str] = [] + moves: list[tuple[str, str]] = [] i = 0 while i < len(fields) and fields[i]: old_mode, new_mode, _, _, status = ( @@ -53,35 +59,61 @@ def whole_paths(raw: bytes) -> list[str]: if ( status != "R100" or old_mode != new_mode + or {old_mode, new_mode} & LINK_MODES or ev.lane_source(src) or ev.lane_source(dst) ): - paths += [src, dst] + whole += [src, dst] + else: + moves.append((src, dst)) else: i += 2 - return paths + return whole, moves -def write_diff(repo: str, base: str, head: str) -> bytes: +def named(path: str, added: bytes) -> bool: + """Whether an added line names `path`, or its last directory and name.""" + tail = "/".join(path.split("/")[-2:]) + return os.fsencode(path) in added or os.fsencode(tail) in added + + +def write_diff(repo: str, base: str, head: str, max_bytes: int) -> bytes: commits = f"{base}...{head}" - whole = whole_paths( + plain = git(repo, *DIFF, "--no-renames", commits) + if len(plain) <= max_bytes: + return plain + whole, moves = renames( git(repo, "diff", "--raw", "-z", "-M100%", "--no-ext-diff", commits) ) - patch = git( - repo, *DIFF, "-M100%", commits, "--", *(f":(exclude,literal){p}" for p in whole) - ) - if whole: - patch += git( + while True: + patch = git( repo, *DIFF, - "--no-renames", + "-M100%", commits, "--", - *(f":(literal){p}" for p in whole), + *(f":(exclude,literal){p}" for p in whole), ) - return patch + if whole: + patch += git( + repo, + *DIFF, + "--no-renames", + commits, + "--", + *(f":(literal){p}" for p in whole), + ) + # Every `+` line, file headers too: a content line can look like one. + added = b"\n".join(line for line in patch.split(b"\n") if line.startswith(b"+")) + # A move printed whole adds lines, which may name another move. + shown = [move for move in moves if named(move[1], added)] + if not shown: + return patch + for move in shown: + moves.remove(move) + whole += list(move) if __name__ == "__main__": - repo, base, head, out = sys.argv[1:5] - Path(out).write_bytes(write_diff(repo, base, head)) + repo, base, head, out, max_bytes = sys.argv[1:6] + Path(out).write_bytes(write_diff(repo, base, head, int(max_bytes))) diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 87645cb328..0326816107 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -96,7 +96,8 @@ jobs: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: refs/heads/main + # The main commit this run started from, so every leg runs the same main. + ref: ${{ github.sha }} path: .base persist-credentials: false @@ -167,12 +168,13 @@ jobs: # the workspace. git diff reads no attributes or diff drivers from it, # so nothing in the PR runs; the patch is a data file (write-diff.py). # `--text` shows a file git would call binary (a NUL byte early in a - # script) as text, so the reviewer sees what node or bash would run. A - # file moved unchanged prints as its rename lines alone, so pure moves - # stay under the size cap; an edited rename, a move into or out of a - # lane source and a move that changes the mode print whole, as - # `--no-renames` shows them, so a partial rename into a lane source is - # reviewed as the new file. A patch that still + # script) as text, so the reviewer sees what node or bash would run. + # Every file prints whole (`--no-renames`), so a partial rename into a + # lane source is reviewed as the new file. Only a patch over the size + # cap shows a file moved unchanged as its rename lines alone; a move + # into or out of a lane source, a mode change, a symlink or submodule, + # and a move whose new path an added line names still print whole. A + # patch that still # shows a binary file, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree # the verdict is keyed on comes from git, never from the model. @@ -204,7 +206,7 @@ jobs: git init -q "$SRC" git -C "$SRC" remote add origin "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY" git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" - python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" + python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" "$MAX_DIFF_BYTES" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" exit 1 @@ -497,7 +499,8 @@ jobs: review-handoffs: needs: review if: >- - github.ref == 'refs/heads/main' + !cancelled() + && github.ref == 'refs/heads/main' && (needs.review.outputs.claude-review != '' || needs.review.outputs.claude-security-review != '') runs-on: ubuntu-24.04 timeout-minutes: 5 @@ -552,7 +555,8 @@ jobs: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: refs/heads/main + # The main commit this run started from, so every leg runs the same main. + ref: ${{ github.sha }} path: .base persist-credentials: false @@ -623,12 +627,13 @@ jobs: # the workspace. git diff reads no attributes or diff drivers from it, # so nothing in the PR runs; the patch is a data file (write-diff.py). # `--text` shows a file git would call binary (a NUL byte early in a - # script) as text, so the reviewer sees what node or bash would run. A - # file moved unchanged prints as its rename lines alone, so pure moves - # stay under the size cap; an edited rename, a move into or out of a - # lane source and a move that changes the mode print whole, as - # `--no-renames` shows them, so a partial rename into a lane source is - # reviewed as the new file. A patch that still + # script) as text, so the reviewer sees what node or bash would run. + # Every file prints whole (`--no-renames`), so a partial rename into a + # lane source is reviewed as the new file. Only a patch over the size + # cap shows a file moved unchanged as its rename lines alone; a move + # into or out of a lane source, a mode change, a symlink or submodule, + # and a move whose new path an added line names still print whole. A + # patch that still # shows a binary file, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree # the verdict is keyed on comes from git, never from the model. @@ -660,7 +665,7 @@ jobs: git init -q "$SRC" git -C "$SRC" remote add origin "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY" git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" - python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" + python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" "$MAX_DIFF_BYTES" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" exit 1 @@ -954,7 +959,8 @@ jobs: subscription-handoffs: needs: review-subscription if: >- - github.ref == 'refs/heads/main' + !cancelled() + && github.ref == 'refs/heads/main' && (needs.review-subscription.outputs.claude-review != '' || needs.review-subscription.outputs.claude-security-review != '') runs-on: ubuntu-24.04 timeout-minutes: 5 @@ -1002,7 +1008,8 @@ jobs: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: refs/heads/main + # The main commit this run started from, so every leg runs the same main. + ref: ${{ github.sha }} path: .base persist-credentials: false @@ -1073,12 +1080,13 @@ jobs: # the workspace. git diff reads no attributes or diff drivers from it, # so nothing in the PR runs; the patch is a data file (write-diff.py). # `--text` shows a file git would call binary (a NUL byte early in a - # script) as text, so the reviewer sees what node or bash would run. A - # file moved unchanged prints as its rename lines alone, so pure moves - # stay under the size cap; an edited rename, a move into or out of a - # lane source and a move that changes the mode print whole, as - # `--no-renames` shows them, so a partial rename into a lane source is - # reviewed as the new file. A patch that still + # script) as text, so the reviewer sees what node or bash would run. + # Every file prints whole (`--no-renames`), so a partial rename into a + # lane source is reviewed as the new file. Only a patch over the size + # cap shows a file moved unchanged as its rename lines alone; a move + # into or out of a lane source, a mode change, a symlink or submodule, + # and a move whose new path an added line names still print whole. A + # patch that still # shows a binary file, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree # the verdict is keyed on comes from git, never from the model. @@ -1110,7 +1118,7 @@ jobs: git init -q "$SRC" git -C "$SRC" remote add origin "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY" git -C "$SRC" fetch -q --no-tags --filter=blob:none origin "$BASE_SHA" "$HEAD_SHA" - python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" + python3 -I .base/.github/scripts/pr-review-main/write-diff.py "$SRC" "$BASE_SHA" "$HEAD_SHA" "$DATA/diff.patch" "$MAX_DIFF_BYTES" if grep -aqE '^(Binary files .* differ|GIT binary patch)$' "$DATA/diff.patch"; then echo "::error::the diff holds a binary file the review cannot read; no verdict, so the lanes stay unbound" exit 1 diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 77caecf517..e3620947eb 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1078,11 +1078,13 @@ the rules that only tighten in 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 whole diff) for a person as `long-line-review:`, - and the binder counts no verdict whose recorded `max_line_bytes` is past the cap. A file moved - unchanged (git's 100% similarity) shows as its rename lines alone, so pure moves stay under the - size cap; an edited rename, a move into or out of a lane source (`lane_source`) and a move that - changes the file mode show whole, as `--no-renames` would (`write-diff.py`), and the caps apply - after. A verdict binds only when the head + and the binder counts no verdict whose recorded `max_line_bytes` is past the cap. The diff is + `--no-renames`, so a renamed file is reviewed whole. Only when that patch is over the size cap + does a file moved unchanged (git's 100% similarity) show as its rename lines alone, and even then + a move into or out of a lane source (`lane_source`), one that changes the file mode or moves a + symlink or submodule, and one whose new path (or its last directory and name) an added line names + show whole (`write-diff.py`); the prompt tells the reviewer a rename-only entry is unseen and that + a change referencing a moved path is a finding. The caps apply after. A verdict binds only when the head also carries the record job's `pr-review-main: ` check run with the same counts, so a run cancelled before it recorded binds nothing. Each lane uploads `verdict.json` as From 56a9066560ea9ab2098f57f88dd0bd8554353725 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:19:58 -0400 Subject: [PATCH 3/4] fix(pr-pipeline): show moves that may run or load, and keep each lane's later legs - write-diff.py prints whole, even over the size cap, a move that may run or load: an executable (100755), a path under a skills, agents, commands or hooks directory, or a file a tool reads by name (SKILL.md, hooks.json, plugin.json, marketplace.json, conftest.py, __init__.py, settings.json). A move is also shown when an added line names its bare name or its name without the extension. - review-subscription and review-fallback run on !cancelled() and their route job's success, so one lane's failed leg no longer skips the other lane's next leg through the implicit success() on every ancestor. Co-Authored-By: Claude Opus 5.5 --- ...in-run-review-credential-chain-7c4e19a2.md | 2 +- .../scripts/pr-review-main/workflow.test.mjs | 71 +++++++++++++++++-- .github/scripts/pr-review-main/write-diff.py | 33 ++++++++- .github/workflows/pr-review-main.yml | 27 ++++--- docs/conventions/pr-pipeline/README.md | 5 +- 5 files changed, 121 insertions(+), 17 deletions(-) diff --git a/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md b/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md index 2fb2b7b3ab..14120ccf1f 100644 --- a/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md +++ b/.changes/pr-pipeline/fix-main-run-review-credential-chain-7c4e19a2.md @@ -5,4 +5,4 @@ bump: patch ### Fixed - The main-run review (`pr-review-main.yml`) runs on the review lanes' credential chain instead of the API key alone: a lane whose key leg credit-stops, is refused with a 401 or 402, or has no key is reviewed on the reviews role's subscription token, and a lane that leg hands off (a limit stop, a credit stop, a 401 or 402, or no token) on `FALLBACK_CLAUDE_CODE_OAUTH_TOKEN`. Each leg is its own job holding one credential, writes the same verdict, and stays green when it hands off, so an exhausted key no longer leaves every lane-source pull request unbound. -- When the main-run review's whole-file diff is over its size cap, a file moved unchanged shows as its rename lines alone, so a pull request of pure moves fits. A move into or out of a lane source, a mode change, a moved symlink or submodule, and a move whose new path an added line names still show whole, and the reviewer is told a rename-only entry is unseen. +- When the main-run review's whole-file diff is over its size cap, a file moved unchanged shows as its rename lines alone, so a pull request of pure moves fits. A move into or out of a lane source, a mode change, a moved symlink or submodule, a move that may run or load (an executable, a skills, agents, commands or hooks path, or a file a tool reads by name), and a move an added line names still show whole, and the reviewer is told a rename-only entry is unseen. diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index 6bc2fc7116..7467587269 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -151,11 +151,12 @@ function diffCase(build, maxBytes = 0) { mkdirSync(path.dirname(path.join(src, file)), { recursive: true }); symlinkSync(target, path.join(src, file)); }; - build.base(write, symlink); + const chmod = (file) => chmodSync(path.join(src, file), 0o755); + build.base(write, symlink, chmod); git("add", "-A"); git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "base"); const base = git("rev-parse", "HEAD"); - build.head(git, write, (file) => chmodSync(path.join(src, file), 0o755)); + build.head(git, write, chmod); git("add", "-A"); git("-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "head"); const head = git("rev-parse", "HEAD"); @@ -226,6 +227,64 @@ test("over the size cap a move an added line names prints whole, and so does one assert.doesNotMatch(patch, /^rename to build\//m); }); +test("over the size cap a move a shown line names by its bare name, or its name without the extension, prints whole", () => { + const patch = diffCase({ + base: (write) => { + write("lib/helper.sh", lines("helper", 10)); + write("lib/payload.py", lines("payload", 10)); + write("lib/quiet.md", lines("quiet", 10)); + write("ci.sh", "true\n"); + }, + head: (git, write) => { + git("mv", "lib/helper.sh", "tools/helper.sh"); + git("mv", "lib/payload.py", "tools/payload.py"); + git("mv", "lib/quiet.md", "docs/quiet.md"); + write("ci.sh", 'true\nsource "$(dirname "$0")/helper.sh"\nimport payload\n'); + }, + }); + for (const word of ["helper", "payload"]) { + assert.equal(patch.match(new RegExp(`^\\+${word} \\d+$`, "gm")).length, 10, word); + } + // An unrelated pure move still collapses. + assert.match(patch, /^rename to docs\/quiet\.md$/m); + assert.doesNotMatch(patch, /^\+quiet /m); +}); + +test("over the size cap a move that may run or load prints whole", () => { + const patch = diffCase({ + base: (write, _symlink) => { + write("fixtures/evil/SKILL.md", lines("skill", 10)); + write("notes/run.md", lines("hook", 10)); + write("lib/x/conftest.py", lines("conftest", 10)); + write("lib/quiet.md", lines("quiet", 10)); + }, + head: (git) => { + git("mv", "fixtures/evil/SKILL.md", "plugins/p/skills/evil/SKILL.md"); + git("mv", "notes/run.md", "plugins/p/hooks/run.md"); + git("mv", "lib/x/conftest.py", "tests/conftest.py"); + git("mv", "lib/quiet.md", "docs/quiet.md"); + }, + }); + for (const word of ["skill", "hook", "conftest"]) { + assert.equal(patch.match(new RegExp(`^\\+${word} \\d+$`, "gm")).length, 10, word); + } + assert.match(patch, /^rename to docs\/quiet\.md$/m); + assert.doesNotMatch(patch, /^\+quiet /m); +}); + +test("over the size cap a moved executable prints whole", () => { + const patch = diffCase({ + // Executable on both sides, so the mode does not change. + base: (write, _symlink, chmod) => { + write("tools/run", lines("exec", 10)); + chmod("tools/run"); + }, + head: (git) => git("mv", "tools/run", "bin/run"), + }); + assert.doesNotMatch(patch, /^old mode/m); + assert.equal(patch.match(/^\+exec \d+$/gm).length, 10); +}); + test("over the size cap a moved symlink prints whole", () => { const patch = diffCase({ base: (_write, symlink) => symlink("../../etc/hostile", "links/run"), @@ -477,7 +536,10 @@ test("a later leg runs only for the lanes the previous leg handed on", () => { const leg = jobText(to); assert.equal(needsOf(to), route); assert.match(leg, new RegExp(`^ {6}matrix: \\$\\{\\{ fromJSON\\(needs\\.${route}\\.outputs\\.matrix\\) \\}\\}$`, "m")); - assert.doesNotMatch(leg, /always\(\) \|\||!cancelled\(\)/); + // !cancelled() with the route's own success: the implicit success() needs + // every ancestor, so one lane's failed leg would skip the other's. + assert.match(leg, new RegExp(`^ {4}if: >-\\n {6}!cancelled\\(\\)\\n {6}&& needs\\.${route}\\.result == 'success'\\n {6}&& github\\.ref == 'refs\\/heads\\/main'\\n`, "m")); + assert.doesNotMatch(leg, /always\(\) \|\|/); // Each lane's reason is its own output, set only by the handoff step. for (const lane of ["claude-review", "claude-security-review"]) { assert.match(jobText(from), new RegExp(`^ {6}${lane}: \\$\\{\\{ steps\\.handoff\\.outputs\\.${lane} \\}\\}$`, "m")); @@ -542,7 +604,8 @@ test("every later leg repeats the key leg's steps but for its credential and why "${{ 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. - const gates = (id) => /^ {4}if: >-\n((?: {6}.*\n)+)/m.exec(jobText(id))[1]; + // 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); diff --git a/.github/scripts/pr-review-main/write-diff.py b/.github/scripts/pr-review-main/write-diff.py index ef35ac2a33..5e13ee0b91 100644 --- a/.github/scripts/pr-review-main/write-diff.py +++ b/.github/scripts/pr-review-main/write-diff.py @@ -34,6 +34,28 @@ DIFF = ["diff", "--text", "--no-ext-diff", "--no-textconv", "--no-color"] # A symlink's or a submodule's mode: the rename lines show neither target. LINK_MODES = {"120000", "160000"} +# A move that may run or load: an executable, a path under a directory a +# plugin or Claude Code loads from, or a file such a tool reads by its name. +EXEC_MODE = "100755" +LOADED_SEGMENTS = {"skills", "agents", "commands", "hooks"} +LOADED_NAMES = { + "SKILL.md", + "hooks.json", + "plugin.json", + "marketplace.json", + "conftest.py", + "__init__.py", + "settings.json", +} + + +def loaded(mode: str, path: str) -> bool: + parts = path.split("/") + return ( + mode == EXEC_MODE + or bool(LOADED_SEGMENTS.intersection(parts[:-1])) + or parts[-1] in LOADED_NAMES + ) def git(repo: str, *args: str) -> bytes: @@ -60,6 +82,8 @@ def renames(raw: bytes) -> tuple[list[str], list[tuple[str, str]]]: status != "R100" or old_mode != new_mode or {old_mode, new_mode} & LINK_MODES + or loaded(old_mode, src) + or loaded(new_mode, dst) or ev.lane_source(src) or ev.lane_source(dst) ): @@ -72,9 +96,12 @@ def renames(raw: bytes) -> tuple[list[str], list[tuple[str, str]]]: def named(path: str, added: bytes) -> bool: - """Whether an added line names `path`, or its last directory and name.""" - tail = "/".join(path.split("/")[-2:]) - return os.fsencode(path) in added or os.fsencode(tail) in added + """Whether an added line names `path`, its last directory and name, its + name, or its name without the extension. Matching too much only prints + more.""" + name = path.split("/")[-1] + forms = {path, "/".join(path.split("/")[-2:]), name, name.rsplit(".", 1)[0]} + return any(form and os.fsencode(form) in added for form in forms) def write_diff(repo: str, base: str, head: str, max_bytes: int) -> bytes: diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 0326816107..45706585a7 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -173,8 +173,9 @@ jobs: # lane source is reviewed as the new file. Only a patch over the size # cap shows a file moved unchanged as its rename lines alone; a move # into or out of a lane source, a mode change, a symlink or submodule, - # and a move whose new path an added line names still print whole. A - # patch that still + # 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, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree # the verdict is keyed on comes from git, never from the model. @@ -532,8 +533,12 @@ jobs: # again. Only this job's launcher step gets the subscription token. review-subscription: needs: review-handoffs + # !cancelled(): the implicit success() also needs every ancestor, so one + # lane's failed leg would skip the other lane's next leg. if: >- - github.ref == 'refs/heads/main' + !cancelled() + && needs.review-handoffs.result == 'success' + && github.ref == 'refs/heads/main' && github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.head_repository.full_name == github.repository @@ -632,8 +637,9 @@ jobs: # lane source is reviewed as the new file. Only a patch over the size # cap shows a file moved unchanged as its rename lines alone; a move # into or out of a lane source, a mode change, a symlink or submodule, - # and a move whose new path an added line names still print whole. A - # patch that still + # 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, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree # the verdict is keyed on comes from git, never from the model. @@ -990,8 +996,12 @@ jobs: # last leg, so a stop here leaves the lane unbound and fails the job. review-fallback: needs: subscription-handoffs + # !cancelled(): the implicit success() also needs every ancestor, so one + # lane's failed leg would skip the other lane's next leg. if: >- - github.ref == 'refs/heads/main' + !cancelled() + && needs.subscription-handoffs.result == 'success' + && github.ref == 'refs/heads/main' && github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success' && github.event.workflow_run.head_repository.full_name == github.repository @@ -1085,8 +1095,9 @@ jobs: # lane source is reviewed as the new file. Only a patch over the size # cap shows a file moved unchanged as its rename lines alone; a move # into or out of a lane source, a mode change, a symlink or submodule, - # and a move whose new path an added line names still print whole. A - # patch that still + # 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, or is larger than one review can read whole, # gets no verdict: the step fails and the lanes stay unbound. The tree # the verdict is keyed on comes from git, never from the model. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index a3193f19fe..94013fe1d0 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1082,7 +1082,10 @@ the rules that only tighten in `--no-renames`, so a renamed file is reviewed whole. Only when that patch is over the size cap does a file moved unchanged (git's 100% similarity) show as its rename lines alone, and even then a move into or out of a lane source (`lane_source`), one that changes the file mode or moves a - symlink or submodule, and one whose new path (or its last directory and name) an added line names + symlink or submodule, one that may run or load (an executable, a path under a `skills`, `agents`, + `commands` or `hooks` directory, or a file a tool reads by name such as `SKILL.md`, `hooks.json`, + `plugin.json`, `marketplace.json`, `conftest.py`, `__init__.py` or `settings.json`), and one whose + new path, last directory and name, name, or name without its extension an added line names show whole (`write-diff.py`); the prompt tells the reviewer a rename-only entry is unseen and that a change referencing a moved path is a finding. The caps apply after. A verdict binds only when the head also carries the record job's `pr-review-main: ` check run with the same counts, so a run From 66871e7fab47e3b07a1d49921a58b9aba4bbc40f Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:31:50 -0400 Subject: [PATCH 4/4] fix(pr-pipeline): run later review legs on the key leg's recorded main commit Checking every leg out at github.sha let a re-run of an old run execute that commit's review code (trusted actors, write-diff.py, the prompt, lane_source), bypassing later tightenings. The key leg checks out the main tip again and records the commit under the lane's own output; the route job passes it on only as exactly 40 hex characters and fails otherwise, and the later legs check out that commit. Co-Authored-By: Claude Opus 5.5 --- .../scripts/pr-review-main/workflow.test.mjs | 38 ++++++++-- .github/workflows/pr-review-main.yml | 69 +++++++++++++++---- docs/conventions/pr-pipeline/README.md | 5 +- 3 files changed, 90 insertions(+), 22 deletions(-) diff --git a/.github/scripts/pr-review-main/workflow.test.mjs b/.github/scripts/pr-review-main/workflow.test.mjs index 7467587269..fdffccc0db 100644 --- a/.github/scripts/pr-review-main/workflow.test.mjs +++ b/.github/scripts/pr-review-main/workflow.test.mjs @@ -350,11 +350,26 @@ test("it checks out main only, and runs local actions from that checkout", () => for (const job of LEGS) { const checkouts = STEPS[job].filter((s) => /uses: actions\/checkout@/.test(s.block)); assert.equal(checkouts.length, 1, job); - // workflow_run's github.sha is the default branch commit whose copy of - // this file runs, so every leg checks out that same main. - assert.match(checkouts[0].block, /^ {10}ref: \$\{\{ github\.sha \}\}$/m); + // The key leg checks out the tip when it starts, never github.sha, which + // a re-run of an old run would bring back; a later leg checks out the + // commit its lane's key leg recorded, which the route passed on. + const ref = /^ {10}ref: (.*)$/m.exec(checkouts[0].block)[1]; + assert.equal(ref, job === KEY ? "refs/heads/main" : "${{ matrix.base }}", job); + // Right after it, the leg records the commit under the lane's name. + assert.equal(index("Record the main commit", job), index(checkouts[0].name, job) + 1, job); + assert.match(step("Record the main commit", job), /^ {8}run: echo "\$LANE-base-sha=\$\(git -C \.base rev-parse --verify HEAD\)" >>"\$GITHUB_OUTPUT"$/m); assert.match(checkouts[0].block, /^ {10}persist-credentials: false$/m); } + assert.doesNotMatch(text, /github\.sha/); + // The key and subscription legs pass each lane's commit on, and each route + // reads the previous leg's. + for (const [route, from] of [["review-handoffs", KEY], ["subscription-handoffs", SUB]]) { + for (const lane of ["claude-review", "claude-security-review"]) { + assert.match(jobText(from), new RegExp(`^ {6}${lane}-base-sha: \\$\\{\\{ steps\\.base\\.outputs\\.${lane}-base-sha \\}\\}$`, "m")); + } + assert.match(jobText(route), new RegExp(`^ {10}CODE_BASE: \\$\\{\\{ needs\\.${from}\\.outputs\\.claude-review-base-sha \\}\\}$`, "m")); + assert.match(jobText(route), new RegExp(`^ {10}SECURITY_BASE: \\$\\{\\{ needs\\.${from}\\.outputs\\.claude-security-review-base-sha \\}\\}$`, "m")); + } for (const [, local] of text.matchAll(/^\s+uses: (\.\/\S+)/gm)) { assert.ok(local.startsWith("./.base/.github/actions/"), local); } @@ -478,6 +493,8 @@ const ROUTES = { "review-handoffs": ["credit-stop", "auth", "no-key"], "subscription-handoffs": ["limit-stop", "credit-stop", "auth", "no-key"], }; +const SHA_A = "a".repeat(40); +const SHA_B = "0123456789abcdef0123456789abcdef01234567"; const routeRun = (job) => { const block = jobText(job); const run = /^ {8}run: \|\n((?: {10}.*\n?)+)/m.exec(block)[1].replace(/^ {10}/gm, ""); @@ -487,7 +504,7 @@ const routeRun = (job) => { const out = path.join(dir, "out"); writeFileSync(out, ""); const reasons = /^ {10}REASONS: (.*)$/m.exec(block)[1]; - const result = spawnSync("bash", ["-e", "-c", run], { encoding: "utf8", env: { ...process.env, REASONS: reasons, GITHUB_OUTPUT: out, ...env } }); + const result = spawnSync("bash", ["-e", "-c", run], { encoding: "utf8", env: { ...process.env, REASONS: reasons, GITHUB_OUTPUT: out, CODE_BASE: SHA_A, SECURITY_BASE: SHA_B, ...env } }); const line = readFileSync(out, "utf8").trim(); return { status: result.status, matrix: line ? JSON.parse(line.replace(/^matrix=/, "")) : undefined }; } finally { @@ -508,11 +525,19 @@ test("a route hands the next leg fixed lane rows, only for a lane handed on with for (const [job, reasons] of Object.entries(ROUTES)) { assert.deepEqual(/^ {10}REASONS: (.*)$/m.exec(jobText(job))[1].split(" "), reasons, job); const route = routeRun(job); - assert.deepEqual(route({ CODE: "credit-stop", SECURITY: "" }), { status: 0, matrix: { include: [{ ...rows["claude-review"], reason: "credit-stop" }] } }); + assert.deepEqual(route({ CODE: "credit-stop", SECURITY: "" }), { status: 0, matrix: { include: [{ ...rows["claude-review"], reason: "credit-stop", base: SHA_A }] } }); assert.deepEqual(route({ CODE: "auth", SECURITY: "no-key" }), { status: 0, - matrix: { include: [{ ...rows["claude-review"], reason: "auth" }, { ...rows["claude-security-review"], reason: "no-key" }] }, + matrix: { include: [{ ...rows["claude-review"], reason: "auth", base: SHA_A }, { ...rows["claude-security-review"], reason: "no-key", base: SHA_B }] }, }); + // A handed-on lane with no main commit, or one that is not 40 hex, fails + // the route, so no later leg runs for it; a lane not handed on needs none. + for (const base of ["", "main", "A".repeat(40), `${SHA_A}\n`, "a".repeat(39)]) { + const result = route({ CODE: "credit-stop", SECURITY: "", CODE_BASE: base }); + assert.notEqual(result.status, 0, `${job} ${JSON.stringify(base)}`); + assert.equal(result.matrix, undefined); + } + assert.equal(route({ CODE: "credit-stop", SECURITY: "", SECURITY_BASE: "" }).status, 0); for (const bad of ["other", "credit-stop ", "true"]) { const result = route({ CODE: "", SECURITY: bad }); assert.notEqual(result.status, 0, `${job} ${JSON.stringify(bad)}`); @@ -587,6 +612,7 @@ test("every later leg repeats the key leg's steps but for its credential and why const normal = (block) => block .replace(/secrets\.[A-Z_]+/g, "secrets.X") + .replace(/^( {8}with:\n)(?: {10}#.*\n)+ {10}ref: .*\n/m, "$1 ref: REF\n") .replace(/^( {10}SECRET_NAME: ).*$/m, "$1X") .replace(new RegExp(`^( {10})(?:${CREDENTIAL_INPUTS.join("|")}): `, "m"), "$1credential: ") .replace(/^ {10}LATER_PRESENT: .*\n/m, "") diff --git a/.github/workflows/pr-review-main.yml b/.github/workflows/pr-review-main.yml index 45706585a7..a19b4f9048 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -26,7 +26,8 @@ name: pr-review-main # for a lane the subscription leg handed off (a limit stop, a credit stop, a # 401 or 402, or an empty token, with the fallback token set). Each leg is # its own job on a new VM holding one credential, and repeats every step from -# a fresh main checkout (workflow.test.mjs holds them equal). A matrix job's +# a fresh checkout of the main commit the lane's key leg recorded, the tip +# when that leg started (workflow.test.mjs holds them equal). A matrix job's # outputs merge, so each lane's handoff reason is its own output, set after # run-claude-cli restored the step files; review-handoffs and # subscription-handoffs, which hold no secret and run no model, check each @@ -92,15 +93,27 @@ jobs: outputs: claude-review: ${{ steps.handoff.outputs.claude-review }} 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 }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # The main commit this run started from, so every leg runs the same main. - ref: ${{ github.sha }} + # The tip when the run starts, never an older commit a re-run of an + # old run would bring back; later legs check out the commit this + # leg recorded. + ref: refs/heads/main path: .base persist-credentials: false + # The main commit this leg checked out, under the lane's own output + # name: a later leg for the lane checks out the same commit. + - name: Record the main commit + id: base + env: + LANE: ${{ matrix.lane }} + run: echo "$LANE-base-sha=$(git -C .base rev-parse --verify HEAD)" >>"$GITHUB_OUTPUT" + - name: Set up Node uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: @@ -514,14 +527,17 @@ jobs: env: CODE: ${{ needs.review.outputs.claude-review }} SECURITY: ${{ needs.review.outputs.claude-security-review }} + CODE_BASE: ${{ needs.review.outputs.claude-review-base-sha }} + SECURITY_BASE: ${{ needs.review.outputs.claude-security-review-base-sha }} REASONS: credit-stop auth no-key run: | - matrix=$(jq -cn --arg code "$CODE" --arg security "$SECURITY" --arg reasons "$REASONS" ' + matrix=$(jq -cn --arg code "$CODE" --arg security "$SECURITY" --arg code_base "$CODE_BASE" --arg security_base "$SECURITY_BASE" --arg reasons "$REASONS" ' ($reasons | split(" ")) as $ok - | [{lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED", reason: $code}, - {lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED", reason: $security}] + | [{lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED", reason: $code, base: $code_base}, + {lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED", reason: $security, base: $security_base}] | map(select(.reason != "")) - | if all(.[]; .reason as $r | any($ok[]; . == $r)) then {include: .} else error("a handoff reason is not one this leg hands on with") end') + | if all(.[]; .reason as $r | any($ok[]; . == $r)) then . else error("a handoff reason is not one this leg hands on with") end + | if all(.[]; .base | test("\\A[0-9a-f]{40}\\z")) then {include: .} else error("a handed-on lane has no main commit to check out") end') echo "matrix=$matrix" >>"$GITHUB_OUTPUT" echo "handed on: $(jq -r '[.include[].lane] | join(", ")' <<<"$matrix")" @@ -556,15 +572,26 @@ jobs: outputs: claude-review: ${{ steps.handoff.outputs.claude-review }} 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 }} steps: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # The main commit this run started from, so every leg runs the same main. - ref: ${{ github.sha }} + # The main commit this lane's key leg checked out, passed on by the + # route job only as 40 hex characters. + ref: ${{ matrix.base }} path: .base persist-credentials: false + # The main commit this leg checked out, under the lane's own output + # name: a later leg for the lane checks out the same commit. + - name: Record the main commit + id: base + env: + LANE: ${{ matrix.lane }} + run: echo "$LANE-base-sha=$(git -C .base rev-parse --verify HEAD)" >>"$GITHUB_OUTPUT" + - name: Set up Node uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: @@ -979,14 +1006,17 @@ jobs: env: CODE: ${{ needs.review-subscription.outputs.claude-review }} SECURITY: ${{ needs.review-subscription.outputs.claude-security-review }} + CODE_BASE: ${{ needs.review-subscription.outputs.claude-review-base-sha }} + SECURITY_BASE: ${{ needs.review-subscription.outputs.claude-security-review-base-sha }} REASONS: limit-stop credit-stop auth no-key run: | - matrix=$(jq -cn --arg code "$CODE" --arg security "$SECURITY" --arg reasons "$REASONS" ' + matrix=$(jq -cn --arg code "$CODE" --arg security "$SECURITY" --arg code_base "$CODE_BASE" --arg security_base "$SECURITY_BASE" --arg reasons "$REASONS" ' ($reasons | split(" ")) as $ok - | [{lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED", reason: $code}, - {lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED", reason: $security}] + | [{lane: "claude-review", skill: "code-review", route: "review-code", switch: "CLAUDE_REVIEW_DISABLED", reason: $code, base: $code_base}, + {lane: "claude-security-review", skill: "security-review", route: "review-security", switch: "CLAUDE_SECURITY_REVIEW_DISABLED", reason: $security, base: $security_base}] | map(select(.reason != "")) - | if all(.[]; .reason as $r | any($ok[]; . == $r)) then {include: .} else error("a handoff reason is not one this leg hands on with") end') + | if all(.[]; .reason as $r | any($ok[]; . == $r)) then . else error("a handoff reason is not one this leg hands on with") end + | if all(.[]; .base | test("\\A[0-9a-f]{40}\\z")) then {include: .} else error("a handed-on lane has no main commit to check out") end') echo "matrix=$matrix" >>"$GITHUB_OUTPUT" echo "handed on: $(jq -r '[.include[].lane] | join(", ")' <<<"$matrix")" @@ -1018,11 +1048,20 @@ jobs: - name: Check out the default branch tip uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # The main commit this run started from, so every leg runs the same main. - ref: ${{ github.sha }} + # The main commit this lane's key leg checked out, passed on by the + # route job only as 40 hex characters. + ref: ${{ matrix.base }} path: .base persist-credentials: false + # The main commit this leg checked out, under the lane's own output + # name: a later leg for the lane checks out the same commit. + - name: Record the main commit + id: base + env: + LANE: ${{ matrix.lane }} + run: echo "$LANE-base-sha=$(git -C .base rev-parse --verify HEAD)" >>"$GITHUB_OUTPUT" + - name: Set up Node uses: actions/setup-node@949feb2413d6458794dcd2491c4babbbce0c15c1 # v7.1.0 with: diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index ff4352dafc..bd19c0009d 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1120,7 +1120,10 @@ the rules that only tighten in for a lane that leg handed off (a limit stop, a credit stop, a 401 or 402, or an empty token) with `FALLBACK_CLAUDE_CODE_OAUTH_TOKEN` set, the fallback token in `review-fallback`. Each leg is a job on a new VM holding one credential, and a route job with no secret passes on only - lanes whose handoff reason is in a closed set. Every leg writes the same verdict, so neither the + lanes whose handoff reason is in a closed set. The key leg checks out the main tip when it + starts and records that commit; a later leg checks out the recorded commit, which the route + passes on only as 40 hex characters, so a re-run of an old run cannot bring back older review + code. Every leg writes the same verdict, so neither the record job nor the gate depends on the leg; the lane record (`report-lane-outcome`) names the credential and why the leg ran. A handed-off leg stays green, so a later leg's verdict still comes from a successful run; a run that leaves no verdict leaves the lanes unbound until the