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..14120ccf1f --- /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. +- 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/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/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 ab8f234e27..fdffccc0db 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, symlinkSync, 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" "\$MAX_DIFF_BYTES"$/m); + assert.match(writeDiff, /^DIFF = \["diff", "--text", "--no-ext-diff", "--no-textconv", "--no-color"\]$/m); assert.match(diff, /\^\(Binary files \.\* differ\|GIT binary patch\)\$/); assert.match(diff, /-gt "\$MAX_DIFF_BYTES"/); assert.match(diff, /MAX_DIFF_BYTES: "\d+"/); @@ -107,48 +129,247 @@ 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. +// 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 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`); + const symlink = (target, file) => { + mkdirSync(path.dirname(path.join(src, file)), { recursive: true }); + symlinkSync(target, path.join(src, file)); + }; + 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").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, chmod); 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, String(maxBytes)], { 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`; + +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); + assert.doesNotMatch(patch, /^[+-]file /m); + 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 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"), + 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) => { + 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); + // 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); } @@ -158,13 +379,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 +395,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 +419,237 @@ 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 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, ""); + 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, 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 { + 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", base: SHA_A }] } }); + assert.deepEqual(route({ CODE: "auth", SECURITY: "no-key" }), { + status: 0, + 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)}`); + 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 != ''\\)`)); + // !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")); + // !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")); + } + } + // 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(/^( {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, "") + .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. + // 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); +}); + +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 new file mode 100644 index 0000000000..5e13ee0b91 --- /dev/null +++ b/.github/scripts/pr-review-main/write-diff.py @@ -0,0 +1,146 @@ +"""Write the main-run review's patch for BASE...HEAD of a repository. + +usage: write-diff.py + +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. +""" + +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"] +# 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: + return subprocess.run( + ["git", "-C", repo, *args], check=True, stdout=subprocess.PIPE + ).stdout + + +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") + whole: list[str] = [] + moves: list[tuple[str, 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 {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) + ): + whole += [src, dst] + else: + moves.append((src, dst)) + else: + i += 2 + return whole, moves + + +def named(path: str, added: bytes) -> bool: + """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: + commits = f"{base}...{head}" + 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) + ) + while True: + 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), + ) + # 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, 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 766b97415f..a19b4f9048 100644 --- a/.github/workflows/pr-review-main.yml +++ b/.github/workflows/pr-review-main.yml @@ -18,9 +18,25 @@ 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 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 +# 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,14 +88,980 @@ 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 }} + 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 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: + 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. + # 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, + # 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. + - 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" "$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 + 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_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::$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. + api-key: ${{ secrets.REVIEWS_CLAUDE_API_KEY }} + model: ${{ steps.models.outputs.model }} + effort: ${{ steps.models.outputs.effort }} + max-turns: ${{ steps.models.outputs.max-turns }} + permission-mode: dontAsk + tools: Read + allowed-tools: Read(./pr-review-main-data/**) + disallowed-tools: | + Read(./.base/**) + Glob + Grep + Bash + Edit + Write + NotebookEdit + WebFetch + WebSearch + Agent + mcp__* + json-schema-file: ${{ runner.temp }}/pr-review-main-schema.json + leg-timeout-minutes: "15" + + # 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: >- + !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 + 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 }} + 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 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, 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 . 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")" + + # 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 + # !cancelled(): the implicit success() also needs every ancestor, so one + # lane's failed leg would skip the other lane's next leg. + if: >- + !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 + 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 }} + 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 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: + 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. + # 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, + # 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. + - 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" "$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 + 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: >- + !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 + 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 }} + 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 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, 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 . 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")" + + # 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 + # !cancelled(): the implicit success() also needs every ancestor, so one + # lane's failed leg would skip the other lane's next leg. + if: >- + !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 + 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: + # 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: @@ -145,11 +1127,16 @@ 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 - # reviewed as the new file. A patch that still + # 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. + # 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, + # 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. @@ -181,7 +1168,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" "$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 @@ -256,18 +1243,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 != '' }} + 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::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 +1313,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' @@ -332,7 +1332,7 @@ jobs: preamble: >- You run unattended; return only the findings the schema asks for; nothing you write is posted. - api-key: ${{ secrets.REVIEWS_CLAUDE_API_KEY }} + 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 }} @@ -354,10 +1354,11 @@ 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. + # 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 +1371,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 +1433,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 +1448,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 67a5d50486..bd19c0009d 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1084,8 +1084,16 @@ 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. 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, 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 cancelled before it recorded binds nothing. Each lane uploads `verdict.json` as @@ -1106,8 +1114,20 @@ 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. 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 + 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