From d423979c2d906222ac2ead78457678cea31f0906 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:03:33 -0400 Subject: [PATCH 1/5] perf(claude-lanes)!: review only what changed, fold the status job, cap timeouts Both review lanes now scope each review. opened, reopened and ready_for_review review the whole pull request; a later push reviews only the pull request's files changed since the lane's last completed review, whose head is kept in the PR's Actions cache. A rewritten history, 300 or more changed files, or a base merge that touched a file of the PR forces a whole review; a push that changes no file of the PR is not reviewed again. The security lane skips a scope that is all documentation (docs-only-paths). Both behaviors sit behind inputs whose defaults turn them on (incremental-review: true; docs-only-paths empty for code review). Each lane is now one job named after its status check, so ` / claude-review-status` and ` / claude-security-review-status` keep their names while the `review / review` and `security-review / security-review` contexts go away. Timeouts follow measured durations: code review 15 -> 13 minutes (step 11 -> 12), security review 25 -> 16 (step 18 -> 15). BREAKING CHANGE: the ` / review` and ` / security-review` check contexts no longer report; the status-check contexts are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/claude-lane-scope.test.cjs | 328 ++++++++++++++++++ .../scripts/claude-lane-status-check.test.cjs | 81 +++-- .github/workflows/claude-review.yml | 260 ++++++++++++-- .github/workflows/claude-security-review.yml | 266 ++++++++++++-- README.md | 37 +- 5 files changed, 867 insertions(+), 105 deletions(-) create mode 100644 .github/scripts/claude-lane-scope.test.cjs diff --git a/.github/scripts/claude-lane-scope.test.cjs b/.github/scripts/claude-lane-scope.test.cjs new file mode 100644 index 0000000..c264984 --- /dev/null +++ b/.github/scripts/claude-lane-scope.test.cjs @@ -0,0 +1,328 @@ +"use strict"; + +// Both Claude lanes scope each review: the whole pull request on open, reopen +// and ready, only the files changed since the lane's last completed review on +// a later push, and no review when nothing in scope changed or every file in +// scope is documentation. The last reviewed head travels in the PR's Actions +// cache. These tests pin the wiring and run the inline github-script against +// a mocked API for every branch of that decision. + +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const test = require("node:test"); + +const { parseWorkflow } = require("./workflow-yaml.cjs"); + +const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; +const workflowsDir = path.join(__dirname, "..", "workflows"); + +const lanes = [ + { file: "claude-review.yml", job: "review", prefix: "claude-review-reviewed-" }, + { + file: "claude-security-review.yml", + job: "security-review", + prefix: "claude-security-review-reviewed-", + }, +]; + +const load = (file) => + parseWorkflow(fs.readFileSync(path.join(workflowsDir, file), "utf8")); +const stepNamed = (job, name) => job.steps.find((step) => step.name === name); + +const HEAD = "a".repeat(40); +const LAST = "b".repeat(40); +const MB_OLD = "c".repeat(40); +const MB_NEW = "d".repeat(40); + +const file = (filename, extra = {}) => ({ filename, status: "modified", ...extra }); + +async function runScope(script, { env, prFiles, compares = {}, fail = false, state }) { + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "scope-")); + const stateFile = path.join(directory, "state", "reviewed-sha"); + const diffFile = path.join(directory, "state", "incremental.diff"); + if (state !== undefined) { + fs.mkdirSync(path.dirname(stateFile), { recursive: true }); + fs.writeFileSync(stateFile, `${state}\n`); + } + const outputs = {}; + const messages = []; + const core = { + setOutput: (key, value) => { + outputs[key] = value; + }, + info: (message) => messages.push(message), + warning: (message) => messages.push(`warning: ${message}`), + }; + const github = { + paginate: async (method, params) => (await method(params)).data, + rest: { + pulls: { + listFiles: async () => { + if (fail) throw new Error("listFiles failed"); + return { data: prFiles }; + }, + }, + repos: { + compareCommitsWithBasehead: async ({ basehead }) => { + assert.ok(basehead in compares, `unexpected compare ${basehead}`); + return { data: compares[basehead] }; + }, + }, + }, + }; + const values = { + INCREMENTAL: "true", + DOCS_ONLY_PATHS: "", + EVENT_ACTION: "synchronize", + PR_NUMBER: "7", + HEAD_SHA: HEAD, + BASE_REF: "main", + STATE_FILE: stateFile, + DIFF_FILE: diffFile, + ...env, + }; + const saved = Object.fromEntries(Object.keys(values).map((key) => [key, process.env[key]])); + Object.assign(process.env, values); + try { + await new AsyncFunction("require", "github", "context", "core", script)( + require, + github, + { repo: { owner: "o", repo: "r" } }, + core, + ); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } + const read = (target) => (fs.existsSync(target) ? fs.readFileSync(target, "utf8") : undefined); + return { outputs, messages, state: read(stateFile), diff: read(diffFile) }; +} + +// The base did not move between the last review and the head. +const unmovedBase = (since) => ({ + [`${LAST}...${HEAD}`]: { status: "ahead", files: since }, + [`main...${LAST}`]: { merge_base_commit: { sha: MB_OLD } }, + [`main...${HEAD}`]: { merge_base_commit: { sha: MB_OLD } }, +}); + +const [codeLane, securityLane] = lanes.map((lane) => { + const workflow = load(lane.file); + const job = workflow.jobs[lane.job]; + return { ...lane, workflow, job, script: stepNamed(job, "Scope the review").with.script }; +}); + +test("both lanes run the same scope script", () => { + assert.equal(codeLane.script, securityLane.script); + assert.doesNotMatch(codeLane.script, /\$\{\{/u); +}); + +for (const lane of [codeLane, securityLane]) { + const { job, workflow } = lane; + + test(`${lane.file}: the job timeout is 16 minutes or less and outlasts the review step`, () => { + const claudeStep = job.steps.find((step) => + String(step.uses ?? "").startsWith("anthropics/claude-code-action@"), + ); + assert.ok(job["timeout-minutes"] <= 16); + assert.ok(claudeStep["timeout-minutes"] < job["timeout-minutes"]); + }); + + test(`${lane.file}: the scope step reads every value through env and fails open`, () => { + const scope = stepNamed(job, "Scope the review"); + assert.equal(scope.id, "scope"); + assert.equal(scope["continue-on-error"], true); + assert.match(scope.uses, /^actions\/github-script@[0-9a-f]{40}$/u); + assert.equal(scope.env.INCREMENTAL, "${{ inputs.incremental-review }}"); + assert.equal(scope.env.DOCS_ONLY_PATHS, "${{ inputs.docs-only-paths }}"); + assert.equal(scope.env.EVENT_ACTION, "${{ github.event.action }}"); + assert.equal(scope.env.HEAD_SHA, "${{ github.event.pull_request.head.sha }}"); + assert.equal(workflow.on.workflow_call.inputs["incremental-review"].default, true); + }); + + test(`${lane.file}: the reviewed head is restored and saved under one per-PR key`, () => { + const key = `${lane.prefix}\${{ github.event.pull_request.number }}-\${{ github.event.pull_request.head.sha }}`; + const restore = stepNamed(job, "Restore the last reviewed head"); + const save = stepNamed(job, "Record the reviewed head"); + const scope = stepNamed(job, "Scope the review"); + assert.match(restore.uses, /^actions\/cache\/restore@[0-9a-f]{40}$/u); + assert.match(save.uses, /^actions\/cache\/save@[0-9a-f]{40}$/u); + assert.equal(restore.with.key, key); + assert.equal(restore.with["restore-keys"], `${lane.prefix}\${{ github.event.pull_request.number }}-`); + assert.equal(save.with.key, key); + assert.equal(restore.with.path, scope.env.STATE_FILE); + assert.equal(save.with.path, scope.env.STATE_FILE); + assert.match(save.if, /steps\.scope\.outputs\.record == 'true'/u); + assert.match(save.if, /steps\.review-outcome\.outputs\.review-ran == 'true'/u); + }); + + test(`${lane.file}: a not-needed review skips every review step and the prompt carries the scope note`, () => { + for (const name of ["Compose Claude CLI arguments", "Report review outcome"]) { + assert.equal(stepNamed(job, name).if, "steps.scope.outputs.review != 'false'"); + } + const claudeStep = job.steps.find((step) => step.id === "claude-review"); + assert.equal(claudeStep.if, "steps.scope.outputs.review != 'false'"); + assert.match(claudeStep.with.prompt, /^\$\{\{ steps\.scope\.outputs\.note \}\}$/mu); + }); +} + +test("opened, reopened and ready_for_review review the whole pull request and record the head", async () => { + for (const action of ["opened", "reopened", "ready_for_review"]) { + const result = await runScope(codeLane.script, { + env: { EVENT_ACTION: action }, + prFiles: [file("src/a.js")], + state: LAST, + }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.note, undefined); + assert.equal(result.outputs.record, "true"); + assert.equal(result.state, `${HEAD}\n`); + } +}); + +test("a push with no recorded review reviews the whole pull request", async () => { + const result = await runScope(codeLane.script, { prFiles: [file("src/a.js")] }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.note, undefined); + assert.ok(result.messages.some((message) => /no earlier review is recorded/u.test(message))); +}); + +test("a push reviews only the pull request's files changed since the last review", async () => { + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js"), file("src/b.js"), file("src/new.js", { previous_filename: "src/old.js" })], + compares: unmovedBase([ + file("src/a.js", { patch: "@@ -1 +1 @@\n-x\n+y" }), + file("src/old.js"), + file("unrelated.txt"), + ]), + state: LAST, + }); + assert.equal(result.outputs.review, "true"); + assert.match(result.outputs.note, /^REVIEW SCOPE: incremental\. This lane last reviewed b{40}\./mu); + assert.match(result.outputs.note, /^- src\/a\.js$/mu); + assert.match(result.outputs.note, /^- src\/new\.js$/mu); + assert.doesNotMatch(result.outputs.note, /src\/b\.js|unrelated/u); + assert.match(result.diff, /^diff --git a\/src\/a\.js b\/src\/a\.js\nstatus: modified\n@@ -1 \+1 @@/mu); + assert.match(result.diff, /no patch from the API/u); + assert.equal(result.state, `${HEAD}\n`); +}); + +test("a push that changes no file of the pull request is not reviewed again", async () => { + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js")], + compares: unmovedBase([file("elsewhere.js")]), + state: LAST, + }); + assert.equal(result.outputs.review, "false"); + assert.match(result.outputs["skip-reason"], /no file of this pull request changed since the review of b{40}/u); +}); + +test("a re-run of a head already reviewed is not reviewed again", async () => { + const result = await runScope(codeLane.script, { prFiles: [file("src/a.js")], state: HEAD }); + assert.equal(result.outputs.review, "false"); +}); + +test("a base merge that changed a file of the pull request forces a whole review", async () => { + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js"), file("src/b.js")], + compares: { + [`${LAST}...${HEAD}`]: { status: "ahead", files: [file("src/a.js"), file("src/b.js")] }, + [`main...${LAST}`]: { merge_base_commit: { sha: MB_OLD } }, + [`main...${HEAD}`]: { merge_base_commit: { sha: MB_NEW } }, + [`${MB_OLD}...${MB_NEW}`]: { files: [file("src/b.js"), file("other.js")] }, + }, + state: LAST, + }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.note, undefined); + assert.ok(result.messages.some((message) => /base-branch merge/u.test(message))); +}); + +test("a base merge that changed only other files keeps the review incremental", async () => { + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js"), file("src/b.js")], + compares: { + [`${LAST}...${HEAD}`]: { status: "ahead", files: [file("src/a.js"), file("other.js")] }, + [`main...${LAST}`]: { merge_base_commit: { sha: MB_OLD } }, + [`main...${HEAD}`]: { merge_base_commit: { sha: MB_NEW } }, + [`${MB_OLD}...${MB_NEW}`]: { files: [file("other.js")] }, + }, + state: LAST, + }); + assert.match(result.outputs.note, /^- src\/a\.js$/mu); + assert.doesNotMatch(result.outputs.note, /src\/b\.js|other\.js/u); +}); + +test("a rewritten history or an oversized change forces a whole review", async () => { + for (const since of [ + { status: "diverged", files: [] }, + { status: "ahead", files: Array.from({ length: 300 }, (_, i) => file(`f${i}`)) }, + ]) { + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js")], + compares: { [`${LAST}...${HEAD}`]: since }, + state: LAST, + }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.note, undefined); + } +}); + +test("an API failure reviews the whole pull request with a warning", async () => { + const result = await runScope(codeLane.script, { prFiles: [], fail: true, state: LAST }); + assert.equal(result.outputs.review, "true"); + assert.ok(result.messages.some((message) => /^warning: Scoping failed/u.test(message))); + assert.equal(result.outputs.record, "true"); +}); + +test("incremental-review false neither narrows the review nor records a head", async () => { + const result = await runScope(codeLane.script, { + env: { INCREMENTAL: "false" }, + prFiles: [file("src/a.js")], + state: LAST, + }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.record, undefined); + assert.equal(result.state, `${LAST}\n`); +}); + +test("the security lane's default docs-only paths skip documentation, not code or agent instructions", async () => { + const docs = securityLane.workflow.on.workflow_call.inputs["docs-only-paths"].default; + assert.equal(codeLane.workflow.on.workflow_call.inputs["docs-only-paths"].default, ""); + const decide = async (filenames) => + ( + await runScope(securityLane.script, { + env: { DOCS_ONLY_PATHS: docs, EVENT_ACTION: "opened" }, + prFiles: filenames.map((name) => file(name)), + }) + ).outputs; + const skipped = await decide([ + "README.md", + "plugins/x/README.md", + "plugins/x/CHANGELOG.md", + "docs/adr/0001-a.md", + "docs/guide.md", + ]); + assert.equal(skipped.review, "false"); + assert.equal(skipped["skip-reason"], "every file in scope matches docs-only-paths"); + for (const filenames of [ + ["docs/guide.md", "src/a.js"], + ["plugins/x/skills/y/SKILL.md"], + ["CLAUDE.md"], + ["docs/records.json"], + ["plugins/docs/notes.md"], + ]) { + assert.equal((await decide(filenames)).review, "true", filenames.join(", ")); + } +}); + +test("a rename into the documentation paths still reviews the source it came from", async () => { + const result = await runScope(securityLane.script, { + env: { DOCS_ONLY_PATHS: "docs/**/*.md", EVENT_ACTION: "opened" }, + prFiles: [file("docs/moved.md", { previous_filename: "src/run.sh" })], + }); + assert.equal(result.outputs.review, "true"); +}); diff --git a/.github/scripts/claude-lane-status-check.test.cjs b/.github/scripts/claude-lane-status-check.test.cjs index 3eb2cb2..762d4ee 100644 --- a/.github/scripts/claude-lane-status-check.test.cjs +++ b/.github/scripts/claude-lane-status-check.test.cjs @@ -1,10 +1,11 @@ "use strict"; -// Both Claude lanes conclude green on an infrastructure failure by design, so -// each carries a status job that runs whenever the review job was not skipped, -// goes red whenever no review happened, and names the cause. These tests pin -// the wiring (reads the job result and outputs through env) and run the job's -// own script for every cause, so a green check always means a review ran. +// Each Claude lane is one job that reviews and reports. The job carries the +// status-check name consumers read (` / claude-review-status`), +// and its last step goes red whenever no review happened and names the cause, +// so a green check always means a review ran or was not needed. These tests +// pin the wiring (values reach the script through env) and run the step's own +// script for every cause. const assert = require("node:assert/strict"); const { spawnSync } = require("node:child_process"); @@ -18,11 +19,11 @@ const { parseWorkflow } = require("./workflow-yaml.cjs"); const workflowsRoot = path.join(__dirname, "..", "workflows"); const lanes = [ - { file: "claude-review.yml", job: "claude-review-status", needs: "review" }, + { file: "claude-review.yml", job: "review", name: "claude-review-status" }, { file: "claude-security-review.yml", - job: "claude-security-review-status", - needs: "security-review", + job: "security-review", + name: "claude-security-review-status", }, ]; @@ -37,9 +38,12 @@ function runStep(step, env) { ); fs.writeFileSync(summary, ""); const result = spawnSync("bash", ["-e", "-c", step.run], { + // step.env holds unexpanded expressions; only the literal LANE is real. env: { PATH: process.env.PATH, - ...step.env, + LANE: step.env.LANE, + JOB_STATUS: "failure", + SKIP_REASON: "", ...env, GITHUB_STEP_SUMMARY: summary, }, @@ -55,41 +59,32 @@ function runStep(step, env) { for (const lane of lanes) { const workflow = load(lane.file); const job = workflow.jobs[lane.job]; - const [step] = job.steps; + const step = job.steps.at(-1); - test(`${lane.file}: ${lane.job} runs after the review unless the review was skipped`, () => { - assert.equal(job.needs, lane.needs); - assert.equal(job.if, `always() && needs.${lane.needs}.result != 'skipped'`); + test(`${lane.file}: one job reviews and reports under the status-check name`, () => { + assert.deepEqual(Object.keys(workflow.jobs), [lane.job]); + assert.equal(job.name, lane.name); + assert.equal(step.name, "Report the review status"); + assert.equal(step.if, "always()"); assert.equal(workflow.on.workflow_call.inputs["status-check"], undefined); - assert.deepEqual(job.permissions, {}); - assert.equal(job.steps.length, 1); }); - test(`${lane.file}: the review job survives a failed attempt and forwards the verdict`, () => { - const reviewJob = workflow.jobs[lane.needs]; - const claudeStep = reviewJob.steps.find((candidate) => + test(`${lane.file}: the review step survives a failed attempt and the job forwards the verdict`, () => { + const claudeStep = job.steps.find((candidate) => String(candidate.uses ?? "").startsWith("anthropics/claude-code-action@"), ); assert.equal(claudeStep["continue-on-error"], true); assert.ok(claudeStep["timeout-minutes"] > 0); for (const name of ["review-failed", "failure-class"]) { - assert.equal( - reviewJob.outputs[name], - `\${{ steps.review-outcome.outputs.${name} }}`, - ); + assert.equal(job.outputs[name], `\${{ steps.review-outcome.outputs.${name} }}`); } }); test(`${lane.file}: the verdict reaches the script through env, never inline`, () => { - assert.equal(step.env.REVIEW_RESULT, `\${{ needs.${lane.needs}.result }}`); - assert.equal( - step.env.REVIEW_FAILED, - `\${{ needs.${lane.needs}.outputs.review-failed }}`, - ); - assert.equal( - step.env.FAILURE_CLASS, - `\${{ needs.${lane.needs}.outputs.failure-class }}`, - ); + assert.equal(step.env.JOB_STATUS, "${{ job.status }}"); + assert.equal(step.env.SKIP_REASON, "${{ steps.scope.outputs.skip-reason }}"); + assert.equal(step.env.REVIEW_FAILED, "${{ steps.review-outcome.outputs.review-failed }}"); + assert.equal(step.env.FAILURE_CLASS, "${{ steps.review-outcome.outputs.failure-class }}"); assert.doesNotMatch(step.run, /\$\{\{/u); }); @@ -99,6 +94,16 @@ for (const lane of lanes) { assert.doesNotMatch(result.summary, /failed:/u); }); + test(`${lane.file}: a review that was not needed stays green and says why`, () => { + const result = runStep(step, { + SKIP_REASON: "every file in scope matches docs-only-paths", + REVIEW_FAILED: "", + FAILURE_CLASS: "", + }); + assert.equal(result.status, 0); + assert.match(result.summary, /no review needed: every file in scope/u); + }); + test(`${lane.file}: every way no review happened goes red and is named`, () => { for (const [failed, klass, named] of [ ["true", "auth", "auth"], @@ -108,24 +113,16 @@ for (const lane of lanes) { ["true", "", "unknown"], // The action skipped itself: green step, nothing reviewed. ["false", "skipped-validation", "skipped-validation"], - // The review job ended before the outcome step wrote any output. + // The job ended before the outcome step wrote any output. ["", "", "no-outcome"], ]) { const result = runStep(step, { - REVIEW_RESULT: "failure", REVIEW_FAILED: failed, FAILURE_CLASS: klass, }); - assert.equal( - result.status, - 1, - `'${failed}'/'${klass}' must fail the check`, - ); + assert.equal(result.status, 1, `'${failed}'/'${klass}' must fail the check`); assert.match(result.summary, new RegExp(`failed: \`${named}\``, "u")); - assert.match( - result.stdout, - new RegExp(`^::error .*failure-class=${named}:`, "mu"), - ); + assert.match(result.stdout, new RegExp(`^::error .*failure-class=${named}:`, "mu")); } }); } diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 7ea0033..942801c 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -35,11 +35,26 @@ name: claude-review # (exclude-comments-by-actor), as prompt-injection hygiene. # - The checkout must persist credentials until claude-code-action#1236 # ships; the last step strips them. +# - The last reviewed head is kept in the pull request's Actions cache, which +# any workflow run on the PR's branch can write. A writer to that branch can +# therefore narrow a later incremental review; same-repository writers +# already receive this lane's secret, so the lane trusts them. # -# The review job stays green on an infrastructure failure. The -# `claude-review-status` job runs after it whenever it was not skipped, and -# goes red, naming the cause, when no review happened. Never make that check -# required. +# REVIEW CADENCE: drafts are skipped. `opened`, `reopened` and +# `ready_for_review` review the whole pull request. A later push +# (`synchronize`) reviews only the files that changed since this lane's last +# completed review (`incremental-review`). It reviews the whole pull request +# instead when no earlier review is recorded, the recorded head is not an +# ancestor of the new head (force push or rebase), more than 300 files changed +# since, or a base-branch merge since then changed a file this pull request +# also changes. A push that changes no file of the pull request is not +# reviewed again. +# +# One job reviews and reports. It is named `claude-review-status`, so its check +# is ` / claude-review-status`, and it goes red, naming the cause, +# when no review happened. A review that is not needed (nothing changed, or +# every file in scope matches `docs-only-paths`) stays green. Never make that +# check required. on: workflow_call: @@ -84,11 +99,25 @@ on: kinds (upstream #1514). type: string default: dependabot,dependabot[bot] + incremental-review: + description: >- + On a push to a reviewed pull request, review only the files changed + since the last completed review. false reviews the whole pull + request on every push. + type: boolean + default: true + docs-only-paths: + description: >- + Newline-separated path globs (`*` within one path segment, `**` + across segments). When every file in the review's scope matches one, + no review runs and the check stays green. Empty never skips. + type: string + default: "" outputs: review-failed: description: >- 'true' on an infrastructure failure, else 'false'. Empty when the - review job did not run. + review did not run. value: ${{ jobs.review.outputs.review-failed }} failure-class: description: >- @@ -105,8 +134,12 @@ permissions: jobs: review: + name: claude-review-status runs-on: ${{ inputs.runner }} - timeout-minutes: 15 + # Measured on claude-code-plugins, 407 successful jobs: p95 434 s, max 688 s + # (7 jobs reached the old 11-minute step limit), so + # ceil(max(1.5 x p95, 1.1 x max) / 60) = 13. + timeout-minutes: 13 # The draft and fork tests are scoped to pull_request so a privileged # trigger still reaches the tripwire. if: >- @@ -139,11 +172,175 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} filter: blob:none + # The newest cache under the PR's prefix holds the last head this lane + # finished reviewing. A miss means a whole-PR review. + - name: Restore the last reviewed head + if: inputs.incremental-review && github.event.action == 'synchronize' + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: .claude-lane/reviewed-sha + key: claude-review-reviewed-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }} + restore-keys: claude-review-reviewed-${{ github.event.pull_request.number }}- + + # Decides whole-PR, incremental, or no review (see REVIEW CADENCE). Any + # failure here leaves `review` unset, which reviews the whole PR. + - name: Scope the review + id: scope + continue-on-error: true + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + INCREMENTAL: ${{ inputs.incremental-review }} + DOCS_ONLY_PATHS: ${{ inputs.docs-only-paths }} + EVENT_ACTION: ${{ github.event.action }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + STATE_FILE: .claude-lane/reviewed-sha + DIFF_FILE: .claude-lane/incremental.diff + with: + script: | + const fs = require("node:fs"); + const path = require("node:path"); + const env = process.env; + const { owner, repo } = context.repo; + const head = env.HEAD_SHA; + const FILE_CAP = 300; + + const globToRegExp = (glob) => { + let out = ""; + for (let i = 0; i < glob.length; i += 1) { + if (glob.startsWith("**/", i)) { + out += "(?:.*/)?"; + i += 2; + } else if (glob.startsWith("**", i)) { + out += ".*"; + i += 1; + } else if (glob[i] === "*") { + out += "[^/]*"; + } else if (glob[i] === "?") { + out += "[^/]"; + } else { + out += glob[i].replace(/[.+^$(){}|[\]\\]/u, "\\$&"); + } + } + return new RegExp(`^${out}$`, "u"); + }; + const docsOnly = (env.DOCS_ONLY_PATHS ?? "") + .split("\n") + .map((line) => line.trim()) + .filter(Boolean) + .map(globToRegExp); + const names = (file) => [file.filename, file.previous_filename].filter(Boolean); + const isDocs = (file) => names(file).every((name) => docsOnly.some((re) => re.test(name))); + const compare = async (basehead) => + (await github.rest.repos.compareCommitsWithBasehead({ owner, repo, basehead })).data; + + // Returns { files, patches } for an incremental review, or { full: reason }. + const sinceLastReview = async (prFiles, last) => { + if (!/^[0-9a-f]{40}$/u.test(last)) return { full: "no earlier review is recorded" }; + if (last === head) return { files: [], patches: new Map() }; + const since = await compare(`${last}...${head}`); + if (since.status !== "ahead") { + return { full: `the last reviewed head ${last} is not an ancestor of ${head}` }; + } + const changed = since.files ?? []; + if (changed.length >= FILE_CAP) return { full: `${FILE_CAP} or more files changed since ${last}` }; + const prNames = new Set(prFiles.flatMap(names)); + const [before, now] = await Promise.all( + [last, head].map(async (ref) => (await compare(`${env.BASE_REF}...${ref}`)).merge_base_commit.sha), + ); + if (before !== now) { + const moved = (await compare(`${before}...${now}`)).files ?? []; + if (moved.length >= FILE_CAP || moved.some((file) => names(file).some((name) => prNames.has(name)))) { + return { full: "a base-branch merge since the last review changed a file this pull request changes" }; + } + } + const patches = new Map(); + for (const file of changed) for (const name of names(file)) patches.set(name, file); + return { files: prFiles.filter((file) => names(file).some((name) => patches.has(name))), patches }; + }; + + const writeDiff = (files, patches) => { + const text = files.map((file) => { + const change = patches.get(file.filename) ?? patches.get(file.previous_filename); + return [ + `diff --git a/${change.previous_filename ?? change.filename} b/${change.filename}`, + `status: ${change.status}`, + change.patch ?? "(no patch from the API; read the file at the head commit)", + "", + ].join("\n"); + }); + fs.mkdirSync(path.dirname(env.DIFF_FILE), { recursive: true }); + fs.writeFileSync(env.DIFF_FILE, text.join("\n")); + }; + + const incremental = env.INCREMENTAL === "true"; + try { + const prFiles = await github.paginate(github.rest.pulls.listFiles, { + owner, + repo, + pull_number: Number(env.PR_NUMBER), + per_page: 100, + }); + let scope = prFiles; + let last = ""; + if (incremental && env.EVENT_ACTION === "synchronize") { + let recorded = ""; + try { + recorded = fs.readFileSync(env.STATE_FILE, "utf8").trim(); + } catch { + recorded = ""; + } + const result = await sinceLastReview(prFiles, recorded); + if (result.full) { + core.info(`Reviewing the whole pull request: ${result.full}.`); + } else { + last = recorded; + scope = result.files; + if (scope.length > 0) writeDiff(scope, result.patches); + } + } + + if (last && scope.length === 0) { + core.setOutput("review", "false"); + core.setOutput("skip-reason", `no file of this pull request changed since the review of ${last}`); + } else if (docsOnly.length > 0 && scope.length > 0 && scope.every(isDocs)) { + core.setOutput("review", "false"); + core.setOutput("skip-reason", "every file in scope matches docs-only-paths"); + } else { + core.setOutput("review", "true"); + if (last) { + const shown = scope.slice(0, 100).map((file) => `- ${file.filename}`); + if (scope.length > shown.length) shown.push(`- and ${scope.length - shown.length} more (see the diff file)`); + core.setOutput( + "note", + [ + `REVIEW SCOPE: incremental. This lane last reviewed ${last}.`, + "Review only what changed since then, in these files:", + ...shown, + `That change is in ${env.DIFF_FILE}; read it, and use \`gh pr diff\` only for context.`, + `Every other file of this pull request is unchanged since ${last} and was reviewed then.`, + ].join("\n"), + ); + } + core.info(last ? `Incremental review of ${scope.length} file(s) since ${last}.` : "Whole pull request review."); + } + } catch (error) { + core.warning(`Scoping failed, reviewing the whole pull request: ${error.message}`); + core.setOutput("review", "true"); + } + if (incremental) { + fs.mkdirSync(path.dirname(env.STATE_FILE), { recursive: true }); + fs.writeFileSync(env.STATE_FILE, `${head}\n`); + core.setOutput("record", "true"); + } + # The inline-comment and plugin-command Skill grants go after the caller's # args so replacing claude-args cannot drop them; heredoc output as # claude-args may be multiline. - name: Compose Claude CLI arguments id: compose-args + if: steps.scope.outputs.review != 'false' env: BASE_ARGS: ${{ inputs.claude-args }} PLUGIN_COMMAND: ${{ inputs.plugin-command }} @@ -158,12 +355,13 @@ jobs: echo "$delimiter" } >> "$GITHUB_OUTPUT" - # continue-on-error keeps an infrastructure failure off this job so the - # outcome step can classify it; the status job carries the red. + # continue-on-error keeps an infrastructure failure from ending the job + # before the outcome step classifies it; the last step carries the red. - name: Claude review id: claude-review + if: steps.scope.outputs.review != 'false' continue-on-error: true - timeout-minutes: 11 + timeout-minutes: 12 uses: anthropics/claude-code-action@756cc22e19660d20e8cc9496b4f242475a7f7790 # v1.0.235 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} @@ -177,6 +375,7 @@ jobs: REPO: ${{ github.repository }} PR NUMBER: ${{ github.event.pull_request.number }} HEAD SHA: ${{ github.event.pull_request.head.sha }} + ${{ steps.scope.outputs.note }} Invoke ${{ inputs.plugin-command }} now and follow its instructions exactly for this pull request. The REPO / PR NUMBER / HEAD SHA header above is authoritative context for the command. @@ -195,34 +394,45 @@ jobs: - name: Report review outcome id: review-outcome + if: steps.scope.outputs.review != 'false' uses: melodic-software/ci-workflows/.github/actions/claude-lane-outcome@ac062650c46005edb4787aff378347746bf63804 # v0.27.0 with: outcome: ${{ steps.claude-review.outcome }} execution-file: ${{ steps.claude-review.outputs.execution_file }} lane: Claude review + # Only a completed review, or a review that was not needed, moves the + # recorded head forward. + - name: Record the reviewed head + if: >- + steps.scope.outputs.record == 'true' + && (steps.scope.outputs.review == 'false' + || steps.review-outcome.outputs.review-ran == 'true') + uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: .claude-lane/reviewed-sha + key: claude-review-reviewed-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }} + # zizmor `artipacked` mitigation: scrub the token checkout persisted. - name: Strip persisted git credentials if: always() run: git config --unset-all http.https://github.com/.extraheader || true - # Reads the review job's outputs, never its log, and goes red whenever no - # review happened. The outputs reach the script through env, never inline - # expressions. - claude-review-status: - needs: review - if: always() && needs.review.result != 'skipped' - runs-on: ${{ inputs.runner }} - timeout-minutes: 5 - permissions: {} - steps: - - name: Report the review outcome + # Goes red whenever no review happened and none was skipped on purpose. + # Every value reaches the script through env, never inline expressions. + - name: Report the review status + if: always() env: LANE: claude-review - REVIEW_RESULT: ${{ needs.review.result }} - REVIEW_FAILED: ${{ needs.review.outputs.review-failed }} - FAILURE_CLASS: ${{ needs.review.outputs.failure-class }} + JOB_STATUS: ${{ job.status }} + SKIP_REASON: ${{ steps.scope.outputs.skip-reason }} + REVIEW_FAILED: ${{ steps.review-outcome.outputs.review-failed }} + FAILURE_CLASS: ${{ steps.review-outcome.outputs.failure-class }} run: | + if [ -n "$SKIP_REASON" ]; then + echo "$LANE: no review needed: $SKIP_REASON." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi if [ -z "$REVIEW_FAILED" ]; then FAILURE_CLASS=no-outcome elif [ "$REVIEW_FAILED" != "true" ] && [ "$FAILURE_CLASS" != "skipped-validation" ]; then @@ -234,8 +444,8 @@ jobs: rate-limit) why="the usage limit was hit (429); re-run once it resets" ;; overloaded) why="the Claude service was overloaded (5xx); re-run later" ;; skipped-validation) why="claude-code-action skipped itself because the caller workflow differs from the default branch's copy; nothing was reviewed" ;; - no-outcome) why="the review job ended ($REVIEW_RESULT) before reporting an outcome; read the review job log" ;; - *) why="an unclassified failure; read the review job log" ;; + no-outcome) why="the job ended ($JOB_STATUS) before the review reported an outcome; read the job log" ;; + *) why="an unclassified failure; read the job log" ;; esac { echo "### $LANE failed: \`${FAILURE_CLASS:-unknown}\`" diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index 0610580..b4a5327 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -2,9 +2,9 @@ name: claude-security-review # Reusable workflow: dedicated LLM security review with # anthropics/claude-code-action, running the org review plugin's security -# command. A sibling of claude-review.yml with the same secrets interface and -# safe-handling model. Findings are advisory: they never fail the job. It runs -# on every non-draft pull request. +# command. A sibling of claude-review.yml with the same secrets interface, +# safe-handling model and review cadence. Findings are advisory: they never +# fail the job. # # Canonical caller (the caller owns triggers, concurrency and the permission # grant; a called workflow can only downgrade the caller's grant): @@ -39,11 +39,22 @@ name: claude-security-review # (exclude-comments-by-actor), as prompt-injection hygiene. # - The checkout must persist credentials until claude-code-action#1236 # ships; the last step strips them. +# - The last reviewed head is kept in the pull request's Actions cache, which +# any workflow run on the PR's branch can write. A writer to that branch can +# therefore narrow a later incremental review; same-repository writers +# already receive this lane's secret, so the lane trusts them. # -# The review job stays green on an infrastructure failure. The -# `claude-security-review-status` job runs after it whenever it was not -# skipped, and goes red, naming the cause, when no review happened. Never make -# that check required. +# REVIEW CADENCE: the same as claude-review.yml (see its header): drafts are +# skipped, `opened`, `reopened` and `ready_for_review` review the whole pull +# request, and a later push reviews only the files changed since this lane's +# last completed review. When every file in scope matches `docs-only-paths` +# (by default top-level `docs/` markdown, READMEs and changelogs), no security +# review runs. +# +# One job reviews and reports. It is named `claude-security-review-status`, so +# its check is ` / claude-security-review-status`, and it goes red, +# naming the cause, when no review happened. A review that is not needed stays +# green. Never make that check required. on: workflow_call: @@ -88,6 +99,25 @@ on: kinds (upstream #1514). type: string default: dependabot,dependabot[bot] + incremental-review: + description: >- + On a push to a reviewed pull request, review only the files changed + since the last completed review. false reviews the whole pull + request on every push. + type: boolean + default: true + docs-only-paths: + description: >- + Newline-separated path globs (`*` within one path segment, `**` + across segments). When every file in the review's scope matches one, + no review runs and the check stays green. Empty never skips. Agent + instruction files (CLAUDE.md, AGENTS.md, skills, rules) are not + documentation here; keep them out of this list. + type: string + default: | + docs/**/*.md + **/README.md + **/CHANGELOG.md secrets: CLAUDE_CODE_OAUTH_TOKEN: description: Claude Code OAuth token (from `claude setup-token`). @@ -98,8 +128,13 @@ permissions: jobs: security-review: + name: claude-security-review-status runs-on: ${{ inputs.runner }} - timeout-minutes: 25 + # Measured on claude-code-plugins, 428 successful jobs: p95 307 s, max + # 1,040 s, which puts ceil(max(1.5 x p95, 1.1 x max) / 60) at 20. Capped at + # the 16-minute ceiling set for every CI job; incremental scoping shortens + # the tail. + timeout-minutes: 16 # The draft and fork tests are scoped to pull_request so a privileged # trigger still reaches the tripwire. if: >- @@ -132,11 +167,175 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} filter: blob:none + # The newest cache under the PR's prefix holds the last head this lane + # finished reviewing. A miss means a whole-PR review. + - name: Restore the last reviewed head + if: inputs.incremental-review && github.event.action == 'synchronize' + uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: .claude-lane/reviewed-sha + key: claude-security-review-reviewed-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }} + restore-keys: claude-security-review-reviewed-${{ github.event.pull_request.number }}- + + # Decides whole-PR, incremental, or no review (see REVIEW CADENCE). Any + # failure here leaves `review` unset, which reviews the whole PR. + - name: Scope the review + id: scope + continue-on-error: true + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + INCREMENTAL: ${{ inputs.incremental-review }} + DOCS_ONLY_PATHS: ${{ inputs.docs-only-paths }} + EVENT_ACTION: ${{ github.event.action }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + STATE_FILE: .claude-lane/reviewed-sha + DIFF_FILE: .claude-lane/incremental.diff + with: + script: | + const fs = require("node:fs"); + const path = require("node:path"); + const env = process.env; + const { owner, repo } = context.repo; + const head = env.HEAD_SHA; + const FILE_CAP = 300; + + const globToRegExp = (glob) => { + let out = ""; + for (let i = 0; i < glob.length; i += 1) { + if (glob.startsWith("**/", i)) { + out += "(?:.*/)?"; + i += 2; + } else if (glob.startsWith("**", i)) { + out += ".*"; + i += 1; + } else if (glob[i] === "*") { + out += "[^/]*"; + } else if (glob[i] === "?") { + out += "[^/]"; + } else { + out += glob[i].replace(/[.+^$(){}|[\]\\]/u, "\\$&"); + } + } + return new RegExp(`^${out}$`, "u"); + }; + const docsOnly = (env.DOCS_ONLY_PATHS ?? "") + .split("\n") + .map((line) => line.trim()) + .filter(Boolean) + .map(globToRegExp); + const names = (file) => [file.filename, file.previous_filename].filter(Boolean); + const isDocs = (file) => names(file).every((name) => docsOnly.some((re) => re.test(name))); + const compare = async (basehead) => + (await github.rest.repos.compareCommitsWithBasehead({ owner, repo, basehead })).data; + + // Returns { files, patches } for an incremental review, or { full: reason }. + const sinceLastReview = async (prFiles, last) => { + if (!/^[0-9a-f]{40}$/u.test(last)) return { full: "no earlier review is recorded" }; + if (last === head) return { files: [], patches: new Map() }; + const since = await compare(`${last}...${head}`); + if (since.status !== "ahead") { + return { full: `the last reviewed head ${last} is not an ancestor of ${head}` }; + } + const changed = since.files ?? []; + if (changed.length >= FILE_CAP) return { full: `${FILE_CAP} or more files changed since ${last}` }; + const prNames = new Set(prFiles.flatMap(names)); + const [before, now] = await Promise.all( + [last, head].map(async (ref) => (await compare(`${env.BASE_REF}...${ref}`)).merge_base_commit.sha), + ); + if (before !== now) { + const moved = (await compare(`${before}...${now}`)).files ?? []; + if (moved.length >= FILE_CAP || moved.some((file) => names(file).some((name) => prNames.has(name)))) { + return { full: "a base-branch merge since the last review changed a file this pull request changes" }; + } + } + const patches = new Map(); + for (const file of changed) for (const name of names(file)) patches.set(name, file); + return { files: prFiles.filter((file) => names(file).some((name) => patches.has(name))), patches }; + }; + + const writeDiff = (files, patches) => { + const text = files.map((file) => { + const change = patches.get(file.filename) ?? patches.get(file.previous_filename); + return [ + `diff --git a/${change.previous_filename ?? change.filename} b/${change.filename}`, + `status: ${change.status}`, + change.patch ?? "(no patch from the API; read the file at the head commit)", + "", + ].join("\n"); + }); + fs.mkdirSync(path.dirname(env.DIFF_FILE), { recursive: true }); + fs.writeFileSync(env.DIFF_FILE, text.join("\n")); + }; + + const incremental = env.INCREMENTAL === "true"; + try { + const prFiles = await github.paginate(github.rest.pulls.listFiles, { + owner, + repo, + pull_number: Number(env.PR_NUMBER), + per_page: 100, + }); + let scope = prFiles; + let last = ""; + if (incremental && env.EVENT_ACTION === "synchronize") { + let recorded = ""; + try { + recorded = fs.readFileSync(env.STATE_FILE, "utf8").trim(); + } catch { + recorded = ""; + } + const result = await sinceLastReview(prFiles, recorded); + if (result.full) { + core.info(`Reviewing the whole pull request: ${result.full}.`); + } else { + last = recorded; + scope = result.files; + if (scope.length > 0) writeDiff(scope, result.patches); + } + } + + if (last && scope.length === 0) { + core.setOutput("review", "false"); + core.setOutput("skip-reason", `no file of this pull request changed since the review of ${last}`); + } else if (docsOnly.length > 0 && scope.length > 0 && scope.every(isDocs)) { + core.setOutput("review", "false"); + core.setOutput("skip-reason", "every file in scope matches docs-only-paths"); + } else { + core.setOutput("review", "true"); + if (last) { + const shown = scope.slice(0, 100).map((file) => `- ${file.filename}`); + if (scope.length > shown.length) shown.push(`- and ${scope.length - shown.length} more (see the diff file)`); + core.setOutput( + "note", + [ + `REVIEW SCOPE: incremental. This lane last reviewed ${last}.`, + "Review only what changed since then, in these files:", + ...shown, + `That change is in ${env.DIFF_FILE}; read it, and use \`gh pr diff\` only for context.`, + `Every other file of this pull request is unchanged since ${last} and was reviewed then.`, + ].join("\n"), + ); + } + core.info(last ? `Incremental review of ${scope.length} file(s) since ${last}.` : "Whole pull request review."); + } + } catch (error) { + core.warning(`Scoping failed, reviewing the whole pull request: ${error.message}`); + core.setOutput("review", "true"); + } + if (incremental) { + fs.mkdirSync(path.dirname(env.STATE_FILE), { recursive: true }); + fs.writeFileSync(env.STATE_FILE, `${head}\n`); + core.setOutput("record", "true"); + } + # The inline-comment and plugin-command Skill grants go after the caller's # args so replacing claude-args cannot drop them; heredoc output as # claude-args may be multiline. - name: Compose Claude CLI arguments id: compose-args + if: steps.scope.outputs.review != 'false' env: BASE_ARGS: ${{ inputs.claude-args }} PLUGIN_COMMAND: ${{ inputs.plugin-command }} @@ -151,12 +350,13 @@ jobs: echo "$delimiter" } >> "$GITHUB_OUTPUT" - # continue-on-error keeps an infrastructure failure off this job so the - # outcome step can classify it; the status job carries the red. + # continue-on-error keeps an infrastructure failure from ending the job + # before the outcome step classifies it; the last step carries the red. - name: Claude security review id: claude-review + if: steps.scope.outputs.review != 'false' continue-on-error: true - timeout-minutes: 18 + timeout-minutes: 15 uses: anthropics/claude-code-action@756cc22e19660d20e8cc9496b4f242475a7f7790 # v1.0.235 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} @@ -170,6 +370,7 @@ jobs: REPO: ${{ github.repository }} PR NUMBER: ${{ github.event.pull_request.number }} HEAD SHA: ${{ github.event.pull_request.head.sha }} + ${{ steps.scope.outputs.note }} Invoke ${{ inputs.plugin-command }} now and follow its instructions exactly for this pull request. The REPO / PR NUMBER / HEAD SHA header above is authoritative context for the command. @@ -181,34 +382,45 @@ jobs: - name: Report review outcome id: review-outcome + if: steps.scope.outputs.review != 'false' uses: melodic-software/ci-workflows/.github/actions/claude-lane-outcome@ac062650c46005edb4787aff378347746bf63804 # v0.27.0 with: outcome: ${{ steps.claude-review.outcome }} execution-file: ${{ steps.claude-review.outputs.execution_file }} lane: Claude security review + # Only a completed review, or a review that was not needed, moves the + # recorded head forward. + - name: Record the reviewed head + if: >- + steps.scope.outputs.record == 'true' + && (steps.scope.outputs.review == 'false' + || steps.review-outcome.outputs.review-ran == 'true') + uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: .claude-lane/reviewed-sha + key: claude-security-review-reviewed-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }} + # zizmor `artipacked` mitigation: scrub the token checkout persisted. - name: Strip persisted git credentials if: always() run: git config --unset-all http.https://github.com/.extraheader || true - # Reads the review job's outputs, never its log, and goes red whenever no - # review happened. The outputs reach the script through env, never inline - # expressions. - claude-security-review-status: - needs: security-review - if: always() && needs.security-review.result != 'skipped' - runs-on: ${{ inputs.runner }} - timeout-minutes: 5 - permissions: {} - steps: - - name: Report the review outcome + # Goes red whenever no review happened and none was skipped on purpose. + # Every value reaches the script through env, never inline expressions. + - name: Report the review status + if: always() env: LANE: claude-security-review - REVIEW_RESULT: ${{ needs.security-review.result }} - REVIEW_FAILED: ${{ needs.security-review.outputs.review-failed }} - FAILURE_CLASS: ${{ needs.security-review.outputs.failure-class }} + JOB_STATUS: ${{ job.status }} + SKIP_REASON: ${{ steps.scope.outputs.skip-reason }} + REVIEW_FAILED: ${{ steps.review-outcome.outputs.review-failed }} + FAILURE_CLASS: ${{ steps.review-outcome.outputs.failure-class }} run: | + if [ -n "$SKIP_REASON" ]; then + echo "$LANE: no review needed: $SKIP_REASON." >> "$GITHUB_STEP_SUMMARY" + exit 0 + fi if [ -z "$REVIEW_FAILED" ]; then FAILURE_CLASS=no-outcome elif [ "$REVIEW_FAILED" != "true" ] && [ "$FAILURE_CLASS" != "skipped-validation" ]; then @@ -220,8 +432,8 @@ jobs: rate-limit) why="the usage limit was hit (429); re-run once it resets" ;; overloaded) why="the Claude service was overloaded (5xx); re-run later" ;; skipped-validation) why="claude-code-action skipped itself because the caller workflow differs from the default branch's copy; nothing was reviewed" ;; - no-outcome) why="the review job ended ($REVIEW_RESULT) before reporting an outcome; read the review job log" ;; - *) why="an unclassified failure; read the review job log" ;; + no-outcome) why="the job ended ($JOB_STATUS) before the review reported an outcome; read the job log" ;; + *) why="an unclassified failure; read the job log" ;; esac { echo "### $LANE failed: \`${FAILURE_CLASS:-unknown}\`" diff --git a/README.md b/README.md index 67aa81a..802e073 100644 --- a/README.md +++ b/README.md @@ -729,7 +729,8 @@ GitHub continues the normal weekly patching of each hosted image generation. secrets interface and safe-handling model, running `/review:security-review`. It reviews the PR's changed files for the vulnerabilities static analysis misses and reports findings as a PR review. Findings are advisory: they never - fail the job. It runs on every non-draft pull request. + fail the job. It skips a pull request whose files in scope are all + documentation (`docs-only-paths`). ## Claude lanes — shared consumption contract @@ -775,20 +776,34 @@ parent secret. | `plugin-command` | `/review:code-review` or `/review:security-review` | Command the review runs | | `claude-args` | `--model claude-sonnet-5 --max-turns 75 --allowedTools "Bash(gh pr diff:*)"` | Claude CLI args; the inline-comment grant and a `Skill()` grant are always appended | | `exclude-comments-by-actor` | `dependabot,dependabot[bot]` | Actors whose comments are withheld from the model (prompt-injection hygiene) | +| `incremental-review` | `true` | On a later push, review only the files changed since the last completed review | +| `docs-only-paths` | empty (code review); `docs/**/*.md`, `**/README.md`, `**/CHANGELOG.md` (security review) | Globs; when every file in scope matches, no review runs | -**Skips.** The review job skips draft PRs, fork PRs (no secrets reach them; -review fork changes by hand) and every bot actor (the action rejects bots it -was not told to allow). Calling either lane from `pull_request_target` or +**Skips.** The job skips draft PRs, fork PRs (no secrets reach them; review +fork changes by hand) and every bot actor (the action rejects bots it was not +told to allow). Calling either lane from `pull_request_target` or `workflow_run` fails the job. -**Status check, red means no review.** The review job stays green on an infrastructure -failure. Each lane's status job (`claude-review-status`, -`claude-security-review-status`) runs after it and goes red, naming the cause, -whenever no review happened: a failed attempt (`auth`, `rate-limit`, +**Cadence.** `opened`, `reopened` and `ready_for_review` review the whole pull +request. A later push reviews only the pull request's files that changed since +the lane's last completed review; the prompt names them and the job writes +their diff to `.claude-lane/incremental.diff`. The last reviewed head is kept +in the pull request's Actions cache. A push reviews the whole pull request +instead when no earlier review is recorded, the recorded head is not an +ancestor of the new head (force push or rebase), 300 or more files changed +since, or a base-branch merge since then changed a file the pull request also +changes. A push that changes no file of the pull request is not reviewed +again. + +**Status check, red means no review.** Each lane is one job, named +`claude-review-status` or `claude-security-review-status`, so its check is +` / claude-review-status`. Its last step goes red, naming the +cause, whenever no review happened: a failed attempt (`auth`, `rate-limit`, `overloaded`, `other`), the action skipping itself because the PR edits the -caller workflow (`skipped-validation`), or a review job that ended before -reporting (`no-outcome`). A skipped review job skips the status job too. Never -make the status check required. +caller workflow (`skipped-validation`), or a job that ended before the review +reported (`no-outcome`). A review that was not needed (nothing changed, or +docs only) stays green and says why in the job summary. Re-run a failed review +with `gh run rerun --failed`. Never make the status check required. `claude-review.yml` also exposes `review-failed` and `failure-class` as workflow outputs. From 906113874c5ea7aac269c3ba902fffa5049a8d15 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 2 Oct 2026 15:28:52 -0400 Subject: [PATCH 2/5] style(claude-lanes): format the lane tests to the Biome fixture config Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/claude-lane-scope.test.cjs | 162 ++++++++++++++---- .../scripts/claude-lane-status-check.test.cjs | 33 +++- 2 files changed, 154 insertions(+), 41 deletions(-) diff --git a/.github/scripts/claude-lane-scope.test.cjs b/.github/scripts/claude-lane-scope.test.cjs index c264984..5de16dc 100644 --- a/.github/scripts/claude-lane-scope.test.cjs +++ b/.github/scripts/claude-lane-scope.test.cjs @@ -19,7 +19,11 @@ const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; const workflowsDir = path.join(__dirname, "..", "workflows"); const lanes = [ - { file: "claude-review.yml", job: "review", prefix: "claude-review-reviewed-" }, + { + file: "claude-review.yml", + job: "review", + prefix: "claude-review-reviewed-", + }, { file: "claude-security-review.yml", job: "security-review", @@ -36,9 +40,16 @@ const LAST = "b".repeat(40); const MB_OLD = "c".repeat(40); const MB_NEW = "d".repeat(40); -const file = (filename, extra = {}) => ({ filename, status: "modified", ...extra }); +const file = (filename, extra = {}) => ({ + filename, + status: "modified", + ...extra, +}); -async function runScope(script, { env, prFiles, compares = {}, fail = false, state }) { +async function runScope( + script, + { env, prFiles, compares = {}, fail = false, state }, +) { const directory = fs.mkdtempSync(path.join(os.tmpdir(), "scope-")); const stateFile = path.join(directory, "state", "reviewed-sha"); const diffFile = path.join(directory, "state", "incremental.diff"); @@ -83,7 +94,9 @@ async function runScope(script, { env, prFiles, compares = {}, fail = false, sta DIFF_FILE: diffFile, ...env, }; - const saved = Object.fromEntries(Object.keys(values).map((key) => [key, process.env[key]])); + const saved = Object.fromEntries( + Object.keys(values).map((key) => [key, process.env[key]]), + ); Object.assign(process.env, values); try { await new AsyncFunction("require", "github", "context", "core", script)( @@ -98,7 +111,8 @@ async function runScope(script, { env, prFiles, compares = {}, fail = false, sta else process.env[key] = value; } } - const read = (target) => (fs.existsSync(target) ? fs.readFileSync(target, "utf8") : undefined); + const read = (target) => + fs.existsSync(target) ? fs.readFileSync(target, "utf8") : undefined; return { outputs, messages, state: read(stateFile), diff: read(diffFile) }; } @@ -112,7 +126,12 @@ const unmovedBase = (since) => ({ const [codeLane, securityLane] = lanes.map((lane) => { const workflow = load(lane.file); const job = workflow.jobs[lane.job]; - return { ...lane, workflow, job, script: stepNamed(job, "Scope the review").with.script }; + return { + ...lane, + workflow, + job, + script: stepNamed(job, "Scope the review").with.script, + }; }); test("both lanes run the same scope script", () => { @@ -136,11 +155,17 @@ for (const lane of [codeLane, securityLane]) { assert.equal(scope.id, "scope"); assert.equal(scope["continue-on-error"], true); assert.match(scope.uses, /^actions\/github-script@[0-9a-f]{40}$/u); - assert.equal(scope.env.INCREMENTAL, "${{ inputs.incremental-review }}"); - assert.equal(scope.env.DOCS_ONLY_PATHS, "${{ inputs.docs-only-paths }}"); - assert.equal(scope.env.EVENT_ACTION, "${{ github.event.action }}"); - assert.equal(scope.env.HEAD_SHA, "${{ github.event.pull_request.head.sha }}"); - assert.equal(workflow.on.workflow_call.inputs["incremental-review"].default, true); + assert.equal(scope.env.INCREMENTAL, `\${{ inputs.incremental-review }}`); + assert.equal(scope.env.DOCS_ONLY_PATHS, `\${{ inputs.docs-only-paths }}`); + assert.equal(scope.env.EVENT_ACTION, `\${{ github.event.action }}`); + assert.equal( + scope.env.HEAD_SHA, + `\${{ github.event.pull_request.head.sha }}`, + ); + assert.equal( + workflow.on.workflow_call.inputs["incremental-review"].default, + true, + ); }); test(`${lane.file}: the reviewed head is restored and saved under one per-PR key`, () => { @@ -151,21 +176,36 @@ for (const lane of [codeLane, securityLane]) { assert.match(restore.uses, /^actions\/cache\/restore@[0-9a-f]{40}$/u); assert.match(save.uses, /^actions\/cache\/save@[0-9a-f]{40}$/u); assert.equal(restore.with.key, key); - assert.equal(restore.with["restore-keys"], `${lane.prefix}\${{ github.event.pull_request.number }}-`); + assert.equal( + restore.with["restore-keys"], + `${lane.prefix}\${{ github.event.pull_request.number }}-`, + ); assert.equal(save.with.key, key); assert.equal(restore.with.path, scope.env.STATE_FILE); assert.equal(save.with.path, scope.env.STATE_FILE); assert.match(save.if, /steps\.scope\.outputs\.record == 'true'/u); - assert.match(save.if, /steps\.review-outcome\.outputs\.review-ran == 'true'/u); + assert.match( + save.if, + /steps\.review-outcome\.outputs\.review-ran == 'true'/u, + ); }); test(`${lane.file}: a not-needed review skips every review step and the prompt carries the scope note`, () => { - for (const name of ["Compose Claude CLI arguments", "Report review outcome"]) { - assert.equal(stepNamed(job, name).if, "steps.scope.outputs.review != 'false'"); + for (const name of [ + "Compose Claude CLI arguments", + "Report review outcome", + ]) { + assert.equal( + stepNamed(job, name).if, + "steps.scope.outputs.review != 'false'", + ); } const claudeStep = job.steps.find((step) => step.id === "claude-review"); assert.equal(claudeStep.if, "steps.scope.outputs.review != 'false'"); - assert.match(claudeStep.with.prompt, /^\$\{\{ steps\.scope\.outputs\.note \}\}$/mu); + assert.match( + claudeStep.with.prompt, + /^\$\{\{ steps\.scope\.outputs\.note \}\}$/mu, + ); }); } @@ -184,15 +224,25 @@ test("opened, reopened and ready_for_review review the whole pull request and re }); test("a push with no recorded review reviews the whole pull request", async () => { - const result = await runScope(codeLane.script, { prFiles: [file("src/a.js")] }); + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js")], + }); assert.equal(result.outputs.review, "true"); assert.equal(result.outputs.note, undefined); - assert.ok(result.messages.some((message) => /no earlier review is recorded/u.test(message))); + assert.ok( + result.messages.some((message) => + /no earlier review is recorded/u.test(message), + ), + ); }); test("a push reviews only the pull request's files changed since the last review", async () => { const result = await runScope(codeLane.script, { - prFiles: [file("src/a.js"), file("src/b.js"), file("src/new.js", { previous_filename: "src/old.js" })], + prFiles: [ + file("src/a.js"), + file("src/b.js"), + file("src/new.js", { previous_filename: "src/old.js" }), + ], compares: unmovedBase([ file("src/a.js", { patch: "@@ -1 +1 @@\n-x\n+y" }), file("src/old.js"), @@ -201,11 +251,17 @@ test("a push reviews only the pull request's files changed since the last review state: LAST, }); assert.equal(result.outputs.review, "true"); - assert.match(result.outputs.note, /^REVIEW SCOPE: incremental\. This lane last reviewed b{40}\./mu); + assert.match( + result.outputs.note, + /^REVIEW SCOPE: incremental\. This lane last reviewed b{40}\./mu, + ); assert.match(result.outputs.note, /^- src\/a\.js$/mu); assert.match(result.outputs.note, /^- src\/new\.js$/mu); assert.doesNotMatch(result.outputs.note, /src\/b\.js|unrelated/u); - assert.match(result.diff, /^diff --git a\/src\/a\.js b\/src\/a\.js\nstatus: modified\n@@ -1 \+1 @@/mu); + assert.match( + result.diff, + /^diff --git a\/src\/a\.js b\/src\/a\.js\nstatus: modified\n@@ -1 \+1 @@/mu, + ); assert.match(result.diff, /no patch from the API/u); assert.equal(result.state, `${HEAD}\n`); }); @@ -217,11 +273,17 @@ test("a push that changes no file of the pull request is not reviewed again", as state: LAST, }); assert.equal(result.outputs.review, "false"); - assert.match(result.outputs["skip-reason"], /no file of this pull request changed since the review of b{40}/u); + assert.match( + result.outputs["skip-reason"], + /no file of this pull request changed since the review of b{40}/u, + ); }); test("a re-run of a head already reviewed is not reviewed again", async () => { - const result = await runScope(codeLane.script, { prFiles: [file("src/a.js")], state: HEAD }); + const result = await runScope(codeLane.script, { + prFiles: [file("src/a.js")], + state: HEAD, + }); assert.equal(result.outputs.review, "false"); }); @@ -229,23 +291,33 @@ test("a base merge that changed a file of the pull request forces a whole review const result = await runScope(codeLane.script, { prFiles: [file("src/a.js"), file("src/b.js")], compares: { - [`${LAST}...${HEAD}`]: { status: "ahead", files: [file("src/a.js"), file("src/b.js")] }, + [`${LAST}...${HEAD}`]: { + status: "ahead", + files: [file("src/a.js"), file("src/b.js")], + }, [`main...${LAST}`]: { merge_base_commit: { sha: MB_OLD } }, [`main...${HEAD}`]: { merge_base_commit: { sha: MB_NEW } }, - [`${MB_OLD}...${MB_NEW}`]: { files: [file("src/b.js"), file("other.js")] }, + [`${MB_OLD}...${MB_NEW}`]: { + files: [file("src/b.js"), file("other.js")], + }, }, state: LAST, }); assert.equal(result.outputs.review, "true"); assert.equal(result.outputs.note, undefined); - assert.ok(result.messages.some((message) => /base-branch merge/u.test(message))); + assert.ok( + result.messages.some((message) => /base-branch merge/u.test(message)), + ); }); test("a base merge that changed only other files keeps the review incremental", async () => { const result = await runScope(codeLane.script, { prFiles: [file("src/a.js"), file("src/b.js")], compares: { - [`${LAST}...${HEAD}`]: { status: "ahead", files: [file("src/a.js"), file("other.js")] }, + [`${LAST}...${HEAD}`]: { + status: "ahead", + files: [file("src/a.js"), file("other.js")], + }, [`main...${LAST}`]: { merge_base_commit: { sha: MB_OLD } }, [`main...${HEAD}`]: { merge_base_commit: { sha: MB_NEW } }, [`${MB_OLD}...${MB_NEW}`]: { files: [file("other.js")] }, @@ -259,7 +331,10 @@ test("a base merge that changed only other files keeps the review incremental", test("a rewritten history or an oversized change forces a whole review", async () => { for (const since of [ { status: "diverged", files: [] }, - { status: "ahead", files: Array.from({ length: 300 }, (_, i) => file(`f${i}`)) }, + { + status: "ahead", + files: Array.from({ length: 300 }, (_, i) => file(`f${i}`)), + }, ]) { const result = await runScope(codeLane.script, { prFiles: [file("src/a.js")], @@ -272,9 +347,17 @@ test("a rewritten history or an oversized change forces a whole review", async ( }); test("an API failure reviews the whole pull request with a warning", async () => { - const result = await runScope(codeLane.script, { prFiles: [], fail: true, state: LAST }); + const result = await runScope(codeLane.script, { + prFiles: [], + fail: true, + state: LAST, + }); assert.equal(result.outputs.review, "true"); - assert.ok(result.messages.some((message) => /^warning: Scoping failed/u.test(message))); + assert.ok( + result.messages.some((message) => + /^warning: Scoping failed/u.test(message), + ), + ); assert.equal(result.outputs.record, "true"); }); @@ -290,8 +373,12 @@ test("incremental-review false neither narrows the review nor records a head", a }); test("the security lane's default docs-only paths skip documentation, not code or agent instructions", async () => { - const docs = securityLane.workflow.on.workflow_call.inputs["docs-only-paths"].default; - assert.equal(codeLane.workflow.on.workflow_call.inputs["docs-only-paths"].default, ""); + const docs = + securityLane.workflow.on.workflow_call.inputs["docs-only-paths"].default; + assert.equal( + codeLane.workflow.on.workflow_call.inputs["docs-only-paths"].default, + "", + ); const decide = async (filenames) => ( await runScope(securityLane.script, { @@ -307,7 +394,10 @@ test("the security lane's default docs-only paths skip documentation, not code o "docs/guide.md", ]); assert.equal(skipped.review, "false"); - assert.equal(skipped["skip-reason"], "every file in scope matches docs-only-paths"); + assert.equal( + skipped["skip-reason"], + "every file in scope matches docs-only-paths", + ); for (const filenames of [ ["docs/guide.md", "src/a.js"], ["plugins/x/skills/y/SKILL.md"], @@ -315,7 +405,11 @@ test("the security lane's default docs-only paths skip documentation, not code o ["docs/records.json"], ["plugins/docs/notes.md"], ]) { - assert.equal((await decide(filenames)).review, "true", filenames.join(", ")); + assert.equal( + (await decide(filenames)).review, + "true", + filenames.join(", "), + ); } }); diff --git a/.github/scripts/claude-lane-status-check.test.cjs b/.github/scripts/claude-lane-status-check.test.cjs index 762d4ee..46b5421 100644 --- a/.github/scripts/claude-lane-status-check.test.cjs +++ b/.github/scripts/claude-lane-status-check.test.cjs @@ -76,15 +76,27 @@ for (const lane of lanes) { assert.equal(claudeStep["continue-on-error"], true); assert.ok(claudeStep["timeout-minutes"] > 0); for (const name of ["review-failed", "failure-class"]) { - assert.equal(job.outputs[name], `\${{ steps.review-outcome.outputs.${name} }}`); + assert.equal( + job.outputs[name], + `\${{ steps.review-outcome.outputs.${name} }}`, + ); } }); test(`${lane.file}: the verdict reaches the script through env, never inline`, () => { - assert.equal(step.env.JOB_STATUS, "${{ job.status }}"); - assert.equal(step.env.SKIP_REASON, "${{ steps.scope.outputs.skip-reason }}"); - assert.equal(step.env.REVIEW_FAILED, "${{ steps.review-outcome.outputs.review-failed }}"); - assert.equal(step.env.FAILURE_CLASS, "${{ steps.review-outcome.outputs.failure-class }}"); + assert.equal(step.env.JOB_STATUS, `\${{ job.status }}`); + assert.equal( + step.env.SKIP_REASON, + `\${{ steps.scope.outputs.skip-reason }}`, + ); + assert.equal( + step.env.REVIEW_FAILED, + `\${{ steps.review-outcome.outputs.review-failed }}`, + ); + assert.equal( + step.env.FAILURE_CLASS, + `\${{ steps.review-outcome.outputs.failure-class }}`, + ); assert.doesNotMatch(step.run, /\$\{\{/u); }); @@ -120,9 +132,16 @@ for (const lane of lanes) { REVIEW_FAILED: failed, FAILURE_CLASS: klass, }); - assert.equal(result.status, 1, `'${failed}'/'${klass}' must fail the check`); + assert.equal( + result.status, + 1, + `'${failed}'/'${klass}' must fail the check`, + ); assert.match(result.summary, new RegExp(`failed: \`${named}\``, "u")); - assert.match(result.stdout, new RegExp(`^::error .*failure-class=${named}:`, "mu")); + assert.match( + result.stdout, + new RegExp(`^::error .*failure-class=${named}:`, "mu"), + ); } }); } From c277c9e17add1ec73d92f112e07b026ba3683981 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 2 Oct 2026 23:54:34 -0400 Subject: [PATCH 3/5] fix(claude-lanes): keep the gate context, record the head in a PR comment The security-review job is named `security-review` again, so with the canonical caller its check is `security-review / security-review`, the context github-iac's security-review-gate org ruleset names (OrgRulesets.cs, integration 15368). The code-review job keeps `claude-review-status`. The last reviewed head moves from the Actions cache, which evicts, to one marker comment per lane that the job writes as github-actions[bot] and edits after each completed review; the scope step reads only that author's marker. This needs no permission beyond the caller's `pull-requests: write`. The code-review step limit is 11 minutes again. The job limit stays 13: outside the step, 1,169 successful jobs on claude-code-plugins (2026-09-29 to 2026-10-01) took p95 17 s, max 82 s. A base merge of 300 or more files now names that reason, and every statement of the threshold reads "300 or more files". Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/claude-lane-scope.test.cjs | 347 +++++++++++++----- .../scripts/claude-lane-status-check.test.cjs | 13 +- .github/workflows/claude-review.yml | 104 ++++-- .github/workflows/claude-security-review.yml | 106 +++--- README.md | 40 +- 5 files changed, 408 insertions(+), 202 deletions(-) diff --git a/.github/scripts/claude-lane-scope.test.cjs b/.github/scripts/claude-lane-scope.test.cjs index 5de16dc..c0bbdad 100644 --- a/.github/scripts/claude-lane-scope.test.cjs +++ b/.github/scripts/claude-lane-scope.test.cjs @@ -3,9 +3,10 @@ // Both Claude lanes scope each review: the whole pull request on open, reopen // and ready, only the files changed since the lane's last completed review on // a later push, and no review when nothing in scope changed or every file in -// scope is documentation. The last reviewed head travels in the PR's Actions -// cache. These tests pin the wiring and run the inline github-script against -// a mocked API for every branch of that decision. +// scope is documentation. The last reviewed head travels in a marker comment +// the lane's job token writes on the PR. These tests pin the wiring and run +// the inline github-script steps against a mocked API for every branch of +// that decision. const assert = require("node:assert/strict"); const fs = require("node:fs"); @@ -19,15 +20,11 @@ const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; const workflowsDir = path.join(__dirname, "..", "workflows"); const lanes = [ - { - file: "claude-review.yml", - job: "review", - prefix: "claude-review-reviewed-", - }, + { file: "claude-review.yml", job: "review", lane: "claude-review" }, { file: "claude-security-review.yml", job: "security-review", - prefix: "claude-security-review-reviewed-", + lane: "claude-security-review", }, ]; @@ -39,6 +36,7 @@ const HEAD = "a".repeat(40); const LAST = "b".repeat(40); const MB_OLD = "c".repeat(40); const MB_NEW = "d".repeat(40); +const BOT = "github-actions[bot]"; const file = (filename, extra = {}) => ({ filename, @@ -46,17 +44,7 @@ const file = (filename, extra = {}) => ({ ...extra, }); -async function runScope( - script, - { env, prFiles, compares = {}, fail = false, state }, -) { - const directory = fs.mkdtempSync(path.join(os.tmpdir(), "scope-")); - const stateFile = path.join(directory, "state", "reviewed-sha"); - const diffFile = path.join(directory, "state", "incremental.diff"); - if (state !== undefined) { - fs.mkdirSync(path.dirname(stateFile), { recursive: true }); - fs.writeFileSync(stateFile, `${state}\n`); - } +async function runScript(script, values, github) { const outputs = {}; const messages = []; const core = { @@ -66,9 +54,74 @@ async function runScope( info: (message) => messages.push(message), warning: (message) => messages.push(`warning: ${message}`), }; + const saved = Object.fromEntries( + Object.keys(values).map((key) => [key, process.env[key]]), + ); + Object.assign(process.env, values); + try { + await new AsyncFunction("require", "github", "context", "core", script)( + require, + { + paginate: async (method, params) => (await method(params)).data, + ...github, + }, + { repo: { owner: "o", repo: "r" } }, + core, + ); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } + return { outputs, messages }; +} + +// Runs a lane's record step and returns the comment call it made. +async function record(lane, { sha = HEAD, markerId = "" } = {}) { + const calls = []; + const capture = (kind) => async (params) => { + calls.push({ kind, ...params }); + return { data: {} }; + }; + await runScript( + lane.recordScript, + { LANE: lane.lane, PR_NUMBER: "7", HEAD_SHA: sha, MARKER_ID: markerId }, + { + rest: { + issues: { + createComment: capture("create"), + updateComment: capture("update"), + }, + }, + }, + ); + assert.equal(calls.length, 1); + return calls[0]; +} + +// The marker comment the lane's own record step writes for `sha`. +async function markerFor(lane, sha, { id = 41, login = BOT } = {}) { + const { body } = await record(lane, { sha }); + return { id, user: { login }, body }; +} + +async function runScope( + lane, + { env, prFiles, compares = {}, fail = false, comments = [] }, +) { + const directory = fs.mkdtempSync(path.join(os.tmpdir(), "scope-")); + const diffFile = path.join(directory, "state", "incremental.diff"); + let listedComments = 0; const github = { - paginate: async (method, params) => (await method(params)).data, rest: { + issues: { + listComments: async ({ issue_number }) => { + assert.equal(issue_number, 7); + listedComments += 1; + return { data: comments }; + }, + }, pulls: { listFiles: async () => { if (fail) throw new Error("listFiles failed"); @@ -83,37 +136,25 @@ async function runScope( }, }, }; - const values = { - INCREMENTAL: "true", - DOCS_ONLY_PATHS: "", - EVENT_ACTION: "synchronize", - PR_NUMBER: "7", - HEAD_SHA: HEAD, - BASE_REF: "main", - STATE_FILE: stateFile, - DIFF_FILE: diffFile, - ...env, - }; - const saved = Object.fromEntries( - Object.keys(values).map((key) => [key, process.env[key]]), + const { outputs, messages } = await runScript( + lane.script, + { + LANE: lane.lane, + INCREMENTAL: "true", + DOCS_ONLY_PATHS: "", + EVENT_ACTION: "synchronize", + PR_NUMBER: "7", + HEAD_SHA: HEAD, + BASE_REF: "main", + DIFF_FILE: diffFile, + ...env, + }, + github, ); - Object.assign(process.env, values); - try { - await new AsyncFunction("require", "github", "context", "core", script)( - require, - github, - { repo: { owner: "o", repo: "r" } }, - core, - ); - } finally { - for (const [key, value] of Object.entries(saved)) { - if (value === undefined) delete process.env[key]; - else process.env[key] = value; - } - } - const read = (target) => - fs.existsSync(target) ? fs.readFileSync(target, "utf8") : undefined; - return { outputs, messages, state: read(stateFile), diff: read(diffFile) }; + const diff = fs.existsSync(diffFile) + ? fs.readFileSync(diffFile, "utf8") + : undefined; + return { outputs, messages, diff, listedComments }; } // The base did not move between the last review and the head. @@ -131,12 +172,24 @@ const [codeLane, securityLane] = lanes.map((lane) => { workflow, job, script: stepNamed(job, "Scope the review").with.script, + recordScript: stepNamed(job, "Record the reviewed head").with.script, }; }); -test("both lanes run the same scope script", () => { +test("both lanes run the same scope and record scripts", () => { assert.equal(codeLane.script, securityLane.script); - assert.doesNotMatch(codeLane.script, /\$\{\{/u); + assert.equal(codeLane.recordScript, securityLane.recordScript); + for (const script of [codeLane.script, codeLane.recordScript]) { + assert.doesNotMatch(script, /\$\{\{/u); + } +}); + +test("the code-review step keeps its 11-minute limit", () => { + const claudeStep = codeLane.job.steps.find( + (step) => step.id === "claude-review", + ); + assert.equal(claudeStep["timeout-minutes"], 11); + assert.equal(codeLane.job["timeout-minutes"], 13); }); for (const lane of [codeLane, securityLane]) { @@ -155,6 +208,7 @@ for (const lane of [codeLane, securityLane]) { assert.equal(scope.id, "scope"); assert.equal(scope["continue-on-error"], true); assert.match(scope.uses, /^actions\/github-script@[0-9a-f]{40}$/u); + assert.equal(scope.env.LANE, lane.lane); assert.equal(scope.env.INCREMENTAL, `\${{ inputs.incremental-review }}`); assert.equal(scope.env.DOCS_ONLY_PATHS, `\${{ inputs.docs-only-paths }}`); assert.equal(scope.env.EVENT_ACTION, `\${{ github.event.action }}`); @@ -168,26 +222,31 @@ for (const lane of [codeLane, securityLane]) { ); }); - test(`${lane.file}: the reviewed head is restored and saved under one per-PR key`, () => { - const key = `${lane.prefix}\${{ github.event.pull_request.number }}-\${{ github.event.pull_request.head.sha }}`; - const restore = stepNamed(job, "Restore the last reviewed head"); + test(`${lane.file}: the reviewed head lives in the lane's PR comment, never the Actions cache`, () => { const save = stepNamed(job, "Record the reviewed head"); - const scope = stepNamed(job, "Scope the review"); - assert.match(restore.uses, /^actions\/cache\/restore@[0-9a-f]{40}$/u); - assert.match(save.uses, /^actions\/cache\/save@[0-9a-f]{40}$/u); - assert.equal(restore.with.key, key); + assert.match(save.uses, /^actions\/github-script@[0-9a-f]{40}$/u); + assert.equal(save["continue-on-error"], true); + assert.equal(save.env.LANE, lane.lane); + assert.equal(save.env.MARKER_ID, `\${{ steps.scope.outputs.marker-id }}`); assert.equal( - restore.with["restore-keys"], - `${lane.prefix}\${{ github.event.pull_request.number }}-`, + save.env.HEAD_SHA, + `\${{ github.event.pull_request.head.sha }}`, ); - assert.equal(save.with.key, key); - assert.equal(restore.with.path, scope.env.STATE_FILE); - assert.equal(save.with.path, scope.env.STATE_FILE); assert.match(save.if, /steps\.scope\.outputs\.record == 'true'/u); assert.match( save.if, /steps\.review-outcome\.outputs\.review-ran == 'true'/u, ); + assert.ok( + job.steps.every( + (step) => !String(step.uses ?? "").startsWith("actions/cache"), + ), + ); + assert.deepEqual(job.permissions, { + contents: "read", + "pull-requests": "write", + "id-token": "write", + }); }); test(`${lane.file}: a not-needed review skips every review step and the prompt carries the scope note`, () => { @@ -207,24 +266,77 @@ for (const lane of [codeLane, securityLane]) { /^\$\{\{ steps\.scope\.outputs\.note \}\}$/mu, ); }); + + test(`${lane.file}: the record step creates the marker once, then updates it`, async () => { + const created = await record(lane); + assert.equal(created.kind, "create"); + assert.equal(created.issue_number, 7); + assert.match( + created.body, + new RegExp( + `^\n`, + "u", + ), + ); + const updated = await record(lane, { markerId: "41" }); + assert.equal(updated.kind, "update"); + assert.equal(updated.comment_id, 41); + }); + + test(`${lane.file}: a push reads back the head its own record step wrote`, async () => { + const result = await runScope(lane, { + prFiles: [file("src/a.js")], + compares: unmovedBase([file("src/a.js", { patch: "@@ -1 +1 @@" })]), + comments: [await markerFor(lane, LAST)], + }); + assert.equal(result.outputs["marker-id"], "41"); + assert.match(result.outputs.note, /last reviewed b{40}/u); + }); } +test("a marker from another author or another lane is ignored", async () => { + for (const comments of [ + [await markerFor(codeLane, LAST, { login: "someone" })], + [await markerFor(securityLane, LAST)], + ]) { + const result = await runScope(codeLane, { + prFiles: [file("src/a.js")], + comments, + }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.note, undefined); + assert.equal(result.outputs["marker-id"], undefined); + } +}); + +test("the newest of several markers wins", async () => { + const older = await markerFor(codeLane, "e".repeat(40), { id: 40 }); + const newer = await markerFor(codeLane, LAST, { id: 42 }); + const result = await runScope(codeLane, { + prFiles: [file("src/a.js")], + compares: unmovedBase([file("src/a.js")]), + comments: [older, newer], + }); + assert.equal(result.outputs["marker-id"], "42"); + assert.match(result.outputs.note, /last reviewed b{40}/u); +}); + test("opened, reopened and ready_for_review review the whole pull request and record the head", async () => { for (const action of ["opened", "reopened", "ready_for_review"]) { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { env: { EVENT_ACTION: action }, prFiles: [file("src/a.js")], - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "true"); assert.equal(result.outputs.note, undefined); assert.equal(result.outputs.record, "true"); - assert.equal(result.state, `${HEAD}\n`); + assert.equal(result.outputs["marker-id"], "41"); } }); test("a push with no recorded review reviews the whole pull request", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [file("src/a.js")], }); assert.equal(result.outputs.review, "true"); @@ -237,7 +349,7 @@ test("a push with no recorded review reviews the whole pull request", async () = }); test("a push reviews only the pull request's files changed since the last review", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [ file("src/a.js"), file("src/b.js"), @@ -248,7 +360,7 @@ test("a push reviews only the pull request's files changed since the last review file("src/old.js"), file("unrelated.txt"), ]), - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "true"); assert.match( @@ -263,14 +375,14 @@ test("a push reviews only the pull request's files changed since the last review /^diff --git a\/src\/a\.js b\/src\/a\.js\nstatus: modified\n@@ -1 \+1 @@/mu, ); assert.match(result.diff, /no patch from the API/u); - assert.equal(result.state, `${HEAD}\n`); + assert.equal(result.outputs.record, "true"); }); test("a push that changes no file of the pull request is not reviewed again", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [file("src/a.js")], compares: unmovedBase([file("elsewhere.js")]), - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "false"); assert.match( @@ -280,15 +392,15 @@ test("a push that changes no file of the pull request is not reviewed again", as }); test("a re-run of a head already reviewed is not reviewed again", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [file("src/a.js")], - state: HEAD, + comments: [await markerFor(codeLane, HEAD)], }); assert.equal(result.outputs.review, "false"); }); test("a base merge that changed a file of the pull request forces a whole review", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [file("src/a.js"), file("src/b.js")], compares: { [`${LAST}...${HEAD}`]: { @@ -301,17 +413,42 @@ test("a base merge that changed a file of the pull request forces a whole review files: [file("src/b.js"), file("other.js")], }, }, - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "true"); assert.equal(result.outputs.note, undefined); assert.ok( - result.messages.some((message) => /base-branch merge/u.test(message)), + result.messages.some((message) => + /base-branch merge .* changed a file this pull request changes/u.test( + message, + ), + ), + ); +}); + +test("a base merge of 300 or more files forces a whole review and says so", async () => { + const result = await runScope(codeLane, { + prFiles: [file("src/a.js")], + compares: { + [`${LAST}...${HEAD}`]: { status: "ahead", files: [file("src/a.js")] }, + [`main...${LAST}`]: { merge_base_commit: { sha: MB_OLD } }, + [`main...${HEAD}`]: { merge_base_commit: { sha: MB_NEW } }, + [`${MB_OLD}...${MB_NEW}`]: { + files: Array.from({ length: 300 }, (_, i) => file(`base${i}`)), + }, + }, + comments: [await markerFor(codeLane, LAST)], + }); + assert.equal(result.outputs.note, undefined); + assert.ok( + result.messages.some((message) => + /changed 300 or more files/u.test(message), + ), ); }); test("a base merge that changed only other files keeps the review incremental", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [file("src/a.js"), file("src/b.js")], compares: { [`${LAST}...${HEAD}`]: { @@ -322,35 +459,49 @@ test("a base merge that changed only other files keeps the review incremental", [`main...${HEAD}`]: { merge_base_commit: { sha: MB_NEW } }, [`${MB_OLD}...${MB_NEW}`]: { files: [file("other.js")] }, }, - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.match(result.outputs.note, /^- src\/a\.js$/mu); assert.doesNotMatch(result.outputs.note, /src\/b\.js|other\.js/u); }); -test("a rewritten history or an oversized change forces a whole review", async () => { - for (const since of [ - { status: "diverged", files: [] }, - { - status: "ahead", - files: Array.from({ length: 300 }, (_, i) => file(`f${i}`)), - }, +test("a rewritten history or 300 or more changed files forces a whole review", async () => { + for (const [since, reason] of [ + [{ status: "diverged", files: [] }, /is not an ancestor/u], + [ + { + status: "ahead", + files: Array.from({ length: 300 }, (_, i) => file(`f${i}`)), + }, + /300 or more files changed since/u, + ], ]) { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [file("src/a.js")], compares: { [`${LAST}...${HEAD}`]: since }, - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "true"); assert.equal(result.outputs.note, undefined); + assert.ok(result.messages.some((message) => reason.test(message))); } }); +test("299 changed files still review incrementally", async () => { + const since = Array.from({ length: 299 }, (_, i) => file(`f${i}`)); + const result = await runScope(codeLane, { + prFiles: [file("f0")], + compares: unmovedBase(since), + comments: [await markerFor(codeLane, LAST)], + }); + assert.match(result.outputs.note, /^- f0$/mu); +}); + test("an API failure reviews the whole pull request with a warning", async () => { - const result = await runScope(codeLane.script, { + const result = await runScope(codeLane, { prFiles: [], fail: true, - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "true"); assert.ok( @@ -361,15 +512,15 @@ test("an API failure reviews the whole pull request with a warning", async () => assert.equal(result.outputs.record, "true"); }); -test("incremental-review false neither narrows the review nor records a head", async () => { - const result = await runScope(codeLane.script, { +test("incremental-review false neither reads, narrows nor records", async () => { + const result = await runScope(codeLane, { env: { INCREMENTAL: "false" }, prFiles: [file("src/a.js")], - state: LAST, + comments: [await markerFor(codeLane, LAST)], }); assert.equal(result.outputs.review, "true"); assert.equal(result.outputs.record, undefined); - assert.equal(result.state, `${LAST}\n`); + assert.equal(result.listedComments, 0); }); test("the security lane's default docs-only paths skip documentation, not code or agent instructions", async () => { @@ -381,7 +532,7 @@ test("the security lane's default docs-only paths skip documentation, not code o ); const decide = async (filenames) => ( - await runScope(securityLane.script, { + await runScope(securityLane, { env: { DOCS_ONLY_PATHS: docs, EVENT_ACTION: "opened" }, prFiles: filenames.map((name) => file(name)), }) @@ -414,7 +565,7 @@ test("the security lane's default docs-only paths skip documentation, not code o }); test("a rename into the documentation paths still reviews the source it came from", async () => { - const result = await runScope(securityLane.script, { + const result = await runScope(securityLane, { env: { DOCS_ONLY_PATHS: "docs/**/*.md", EVENT_ACTION: "opened" }, prFiles: [file("docs/moved.md", { previous_filename: "src/run.sh" })], }); diff --git a/.github/scripts/claude-lane-status-check.test.cjs b/.github/scripts/claude-lane-status-check.test.cjs index 46b5421..5f3ddd0 100644 --- a/.github/scripts/claude-lane-status-check.test.cjs +++ b/.github/scripts/claude-lane-status-check.test.cjs @@ -1,11 +1,12 @@ "use strict"; // Each Claude lane is one job that reviews and reports. The job carries the -// status-check name consumers read (` / claude-review-status`), -// and its last step goes red whenever no review happened and names the cause, -// so a green check always means a review ran or was not needed. These tests -// pin the wiring (values reach the script through env) and run the step's own -// script for every cause. +// check name consumers read (` / claude-review-status`, and +// `security-review / security-review`, the context the github-iac +// `security-review-gate` ruleset names), and its last step goes red whenever +// no review happened and names the cause, so a green check always means a +// review ran or was not needed. These tests pin the wiring (values reach the +// script through env) and run the step's own script for every cause. const assert = require("node:assert/strict"); const { spawnSync } = require("node:child_process"); @@ -23,7 +24,7 @@ const lanes = [ { file: "claude-security-review.yml", job: "security-review", - name: "claude-security-review-status", + name: "security-review", }, ]; diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 942801c..794bfe9 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -35,20 +35,22 @@ name: claude-review # (exclude-comments-by-actor), as prompt-injection hygiene. # - The checkout must persist credentials until claude-code-action#1236 # ships; the last step strips them. -# - The last reviewed head is kept in the pull request's Actions cache, which -# any workflow run on the PR's branch can write. A writer to that branch can -# therefore narrow a later incremental review; same-repository writers -# already receive this lane's secret, so the lane trusts them. +# - The last reviewed head is kept in a pull request comment this job writes +# with its own token, so its author is github-actions[bot]; the lane reads +# only that author's marker. Any workflow in the repository holding +# `pull-requests: write` could write one and narrow a later incremental +# review; same-repository writers already receive this lane's secret, so +# the lane trusts them. # # REVIEW CADENCE: drafts are skipped. `opened`, `reopened` and # `ready_for_review` review the whole pull request. A later push # (`synchronize`) reviews only the files that changed since this lane's last # completed review (`incremental-review`). It reviews the whole pull request # instead when no earlier review is recorded, the recorded head is not an -# ancestor of the new head (force push or rebase), more than 300 files changed +# ancestor of the new head (force push or rebase), 300 or more files changed # since, or a base-branch merge since then changed a file this pull request -# also changes. A push that changes no file of the pull request is not -# reviewed again. +# also changes (or 300 or more files, too many to check). A push that changes +# no file of the pull request is not reviewed again. # # One job reviews and reports. It is named `claude-review-status`, so its check # is ` / claude-review-status`, and it goes red, naming the cause, @@ -136,9 +138,11 @@ jobs: review: name: claude-review-status runs-on: ${{ inputs.runner }} - # Measured on claude-code-plugins, 407 successful jobs: p95 434 s, max 688 s - # (7 jobs reached the old 11-minute step limit), so - # ceil(max(1.5 x p95, 1.1 x max) / 60) = 13. + # The review step stops at 11 minutes (660 s); incremental scoping, not a + # longer step, shortens the reviews that reached it. Outside that step, + # 1,169 successful jobs on claude-code-plugins (2026-09-29 to 2026-10-01) + # took p95 17 s, max 82 s, which leaves 38 s of 13 minutes (780 s) for the + # scope and record steps. timeout-minutes: 13 # The draft and fork tests are scoped to pull_request so a privileged # trigger still reaches the tripwire. @@ -172,30 +176,22 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} filter: blob:none - # The newest cache under the PR's prefix holds the last head this lane - # finished reviewing. A miss means a whole-PR review. - - name: Restore the last reviewed head - if: inputs.incremental-review && github.event.action == 'synchronize' - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: .claude-lane/reviewed-sha - key: claude-review-reviewed-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }} - restore-keys: claude-review-reviewed-${{ github.event.pull_request.number }}- - - # Decides whole-PR, incremental, or no review (see REVIEW CADENCE). Any - # failure here leaves `review` unset, which reviews the whole PR. + # Decides whole-PR, incremental, or no review (see REVIEW CADENCE). The + # last head this lane finished reviewing is read from the lane's marker + # comment on the PR. Any failure here leaves `review` unset, which + # reviews the whole PR. - name: Scope the review id: scope continue-on-error: true uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: + LANE: claude-review INCREMENTAL: ${{ inputs.incremental-review }} DOCS_ONLY_PATHS: ${{ inputs.docs-only-paths }} EVENT_ACTION: ${{ github.event.action }} PR_NUMBER: ${{ github.event.pull_request.number }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} BASE_REF: ${{ github.event.pull_request.base.ref }} - STATE_FILE: .claude-lane/reviewed-sha DIFF_FILE: .claude-lane/incremental.diff with: script: | @@ -205,6 +201,7 @@ jobs: const { owner, repo } = context.repo; const head = env.HEAD_SHA; const FILE_CAP = 300; + const MARKER = ``, + `${env.LANE} has reviewed this pull request through ${env.HEAD_SHA}; a later push is reviewed from there.`, + ].join("\n"); + if (env.MARKER_ID) { + await github.rest.issues.updateComment({ owner, repo, comment_id: Number(env.MARKER_ID), body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: Number(env.PR_NUMBER), body }); + } # zizmor `artipacked` mitigation: scrub the token checkout persisted. - name: Strip persisted git credentials diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index b4a5327..3854061 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -39,22 +39,26 @@ name: claude-security-review # (exclude-comments-by-actor), as prompt-injection hygiene. # - The checkout must persist credentials until claude-code-action#1236 # ships; the last step strips them. -# - The last reviewed head is kept in the pull request's Actions cache, which -# any workflow run on the PR's branch can write. A writer to that branch can -# therefore narrow a later incremental review; same-repository writers -# already receive this lane's secret, so the lane trusts them. +# - The last reviewed head is kept in a pull request comment this job writes +# with its own token, so its author is github-actions[bot]; the lane reads +# only that author's marker. Any workflow in the repository holding +# `pull-requests: write` could write one and narrow a later incremental +# review; same-repository writers already receive this lane's secret, so +# the lane trusts them. # # REVIEW CADENCE: the same as claude-review.yml (see its header): drafts are # skipped, `opened`, `reopened` and `ready_for_review` review the whole pull # request, and a later push reviews only the files changed since this lane's -# last completed review. When every file in scope matches `docs-only-paths` -# (by default top-level `docs/` markdown, READMEs and changelogs), no security -# review runs. +# last completed review; 300 or more files changed since forces a whole review. +# When every file in scope matches `docs-only-paths` (by default top-level +# `docs/` markdown, READMEs and changelogs), no security review runs. # -# One job reviews and reports. It is named `claude-security-review-status`, so -# its check is ` / claude-security-review-status`, and it goes red, -# naming the cause, when no review happened. A review that is not needed stays -# green. Never make that check required. +# One job reviews and reports. It is named `security-review`, so with the +# canonical caller its check is `security-review / security-review`, the +# context the github-iac `security-review-gate` org ruleset names. It goes +# red, naming the cause, when no review happened; a review that is not needed +# stays green. That ruleset is disabled (github-iac ADR 0011 keeps agentic +# review advisory); do not make the check required elsewhere. on: workflow_call: @@ -128,7 +132,7 @@ permissions: jobs: security-review: - name: claude-security-review-status + name: security-review runs-on: ${{ inputs.runner }} # Measured on claude-code-plugins, 428 successful jobs: p95 307 s, max # 1,040 s, which puts ceil(max(1.5 x p95, 1.1 x max) / 60) at 20. Capped at @@ -167,30 +171,22 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} filter: blob:none - # The newest cache under the PR's prefix holds the last head this lane - # finished reviewing. A miss means a whole-PR review. - - name: Restore the last reviewed head - if: inputs.incremental-review && github.event.action == 'synchronize' - uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 - with: - path: .claude-lane/reviewed-sha - key: claude-security-review-reviewed-${{ github.event.pull_request.number }}-${{ github.event.pull_request.head.sha }} - restore-keys: claude-security-review-reviewed-${{ github.event.pull_request.number }}- - - # Decides whole-PR, incremental, or no review (see REVIEW CADENCE). Any - # failure here leaves `review` unset, which reviews the whole PR. + # Decides whole-PR, incremental, or no review (see REVIEW CADENCE). The + # last head this lane finished reviewing is read from the lane's marker + # comment on the PR. Any failure here leaves `review` unset, which + # reviews the whole PR. - name: Scope the review id: scope continue-on-error: true uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: + LANE: claude-security-review INCREMENTAL: ${{ inputs.incremental-review }} DOCS_ONLY_PATHS: ${{ inputs.docs-only-paths }} EVENT_ACTION: ${{ github.event.action }} PR_NUMBER: ${{ github.event.pull_request.number }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} BASE_REF: ${{ github.event.pull_request.base.ref }} - STATE_FILE: .claude-lane/reviewed-sha DIFF_FILE: .claude-lane/incremental.diff with: script: | @@ -200,6 +196,7 @@ jobs: const { owner, repo } = context.repo; const head = env.HEAD_SHA; const FILE_CAP = 300; + const MARKER = ``, + `${env.LANE} has reviewed this pull request through ${env.HEAD_SHA}; a later push is reviewed from there.`, + ].join("\n"); + if (env.MARKER_ID) { + await github.rest.issues.updateComment({ owner, repo, comment_id: Number(env.MARKER_ID), body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: Number(env.PR_NUMBER), body }); + } # zizmor `artipacked` mitigation: scrub the token checkout persisted. - name: Strip persisted git credentials diff --git a/README.md b/README.md index 802e073..8c2b115 100644 --- a/README.md +++ b/README.md @@ -787,23 +787,29 @@ told to allow). Calling either lane from `pull_request_target` or **Cadence.** `opened`, `reopened` and `ready_for_review` review the whole pull request. A later push reviews only the pull request's files that changed since the lane's last completed review; the prompt names them and the job writes -their diff to `.claude-lane/incremental.diff`. The last reviewed head is kept -in the pull request's Actions cache. A push reviews the whole pull request -instead when no earlier review is recorded, the recorded head is not an -ancestor of the new head (force push or rebase), 300 or more files changed -since, or a base-branch merge since then changed a file the pull request also -changes. A push that changes no file of the pull request is not reviewed -again. - -**Status check, red means no review.** Each lane is one job, named -`claude-review-status` or `claude-security-review-status`, so its check is -` / claude-review-status`. Its last step goes red, naming the -cause, whenever no review happened: a failed attempt (`auth`, `rate-limit`, -`overloaded`, `other`), the action skipping itself because the PR edits the -caller workflow (`skipped-validation`), or a job that ended before the review -reported (`no-outcome`). A review that was not needed (nothing changed, or -docs only) stays green and says why in the job summary. Re-run a failed review -with `gh run rerun --failed`. Never make the status check required. +their diff to `.claude-lane/incremental.diff`. Each lane keeps the last head +it reviewed in one pull request comment that the job writes with its own token +(author `github-actions[bot]`) and edits after each completed review. A push +reviews the whole pull request instead when no earlier review is recorded, the +recorded head is not an ancestor of the new head (force push or rebase), 300 +or more files changed since, or a base-branch merge since then changed a file +the pull request also changes (or 300 or more files, too many to check). A +push that changes no file of the pull request is not reviewed again. + +**Status check, red means no review.** Each lane is one job. The code-review +job is named `claude-review-status`, so its check is +` / claude-review-status`. The security-review job is named +`security-review`, so with the canonical caller its check is +`security-review / security-review`, the context the github-iac +`security-review-gate` org ruleset names. The job's last step goes red, naming +the cause, whenever no review happened: a failed attempt (`auth`, +`rate-limit`, `overloaded`, `other`), the action skipping itself because the +PR edits the caller workflow (`skipped-validation`), or a job that ended +before the review reported (`no-outcome`). A review that was not needed +(nothing changed, or docs only) stays green and says why in the job summary. +Re-run a failed review with `gh run rerun --failed`. github-iac keeps +`security-review-gate` disabled (its ADR 0011 keeps agentic review advisory); +do not make either check required anywhere else. `claude-review.yml` also exposes `review-failed` and `failure-class` as workflow outputs. From 4d4b9a5a2247a8aa2ca34f88b8df0f9785b39305 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:09:51 -0400 Subject: [PATCH 4/5] fix(claude-lanes): never skip agent instructions, review unshown patches whole Two review findings on the first live run of these lanes: - docs-only-paths is a caller input, so a wider list such as `**/*.md` could have skipped a security review of CLAUDE.md, AGENTS.md, a SKILL.md or a rules file. Those names, and anything under .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/ or prompts/, now never count as documentation. - The compare API drops the patch of a large text change. An incremental scope holding such a file now falls back to a whole review instead of asking the model to read the file; a binary file or a pure rename, which has no patch and no changed lines, stays incremental. The security-review step drops from 15 to 14 minutes so the 16-minute job holds the measured 82 s maximum outside the step plus the scope and record steps, which took 1 s each on this pull request's first run. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/claude-lane-scope.test.cjs | 69 ++++++++++++++++++-- .github/workflows/claude-review.yml | 29 ++++++-- .github/workflows/claude-security-review.yml | 31 ++++++--- README.md | 9 +-- 4 files changed, 112 insertions(+), 26 deletions(-) diff --git a/.github/scripts/claude-lane-scope.test.cjs b/.github/scripts/claude-lane-scope.test.cjs index c0bbdad..cd2bd04 100644 --- a/.github/scripts/claude-lane-scope.test.cjs +++ b/.github/scripts/claude-lane-scope.test.cjs @@ -184,12 +184,17 @@ test("both lanes run the same scope and record scripts", () => { } }); -test("the code-review step keeps its 11-minute limit", () => { - const claudeStep = codeLane.job.steps.find( - (step) => step.id === "claude-review", - ); - assert.equal(claudeStep["timeout-minutes"], 11); - assert.equal(codeLane.job["timeout-minutes"], 13); +test("each review step leaves its job the measured overhead", () => { + for (const [lane, step, job] of [ + [codeLane, 11, 13], + [securityLane, 14, 16], + ]) { + const claudeStep = lane.job.steps.find( + (candidate) => candidate.id === "claude-review", + ); + assert.equal(claudeStep["timeout-minutes"], step, lane.file); + assert.equal(lane.job["timeout-minutes"], job, lane.file); + } }); for (const lane of [codeLane, securityLane]) { @@ -487,6 +492,35 @@ test("a rewritten history or 300 or more changed files forces a whole review", a } }); +test("a changed file the API returns no patch for forces a whole review", async () => { + const result = await runScope(codeLane, { + prFiles: [file("src/a.js"), file("img.png"), file("src/c.js")], + compares: unmovedBase([ + file("img.png", { changes: 0 }), + file("src/c.js", { changes: 0, previous_filename: "src/b.js" }), + file("src/a.js", { changes: 4000 }), + ]), + comments: [await markerFor(codeLane, LAST)], + }); + assert.equal(result.outputs.note, undefined); + assert.ok( + result.messages.some((message) => /no patch for src\/a\.js/u.test(message)), + ); +}); + +test("a binary or pure-rename change without a patch stays incremental", async () => { + const result = await runScope(codeLane, { + prFiles: [file("img.png"), file("src/c.js")], + compares: unmovedBase([ + file("img.png", { changes: 0 }), + file("src/c.js", { changes: 0, previous_filename: "src/b.js" }), + ]), + comments: [await markerFor(codeLane, LAST)], + }); + assert.match(result.outputs.note, /^- img\.png$/mu); + assert.match(result.diff, /no patch from the API/u); +}); + test("299 changed files still review incrementally", async () => { const since = Array.from({ length: 299 }, (_, i) => file(`f${i}`)); const result = await runScope(codeLane, { @@ -564,6 +598,29 @@ test("the security lane's default docs-only paths skip documentation, not code o } }); +test("a caller's wider docs-only-paths never skips agent instructions", async () => { + const decide = async (filenames) => + ( + await runScope(securityLane, { + env: { DOCS_ONLY_PATHS: "**/*.md", EVENT_ACTION: "opened" }, + prFiles: filenames.map((name) => file(name)), + }) + ).outputs.review; + assert.equal(await decide(["README.md", "docs/a.md"]), "false"); + for (const name of [ + "CLAUDE.md", + "sub/AGENTS.md", + "plugins/x/skills/y/SKILL.md", + "plugins/x/skills/y/reference/notes.md", + ".claude/rules/a.md", + "plugins/x/agents/a.md", + "plugins/x/commands/c.md", + ".github/copilot-instructions.md", + ]) { + assert.equal(await decide(["README.md", name]), "true", name); + } +}); + test("a rename into the documentation paths still reviews the source it came from", async () => { const result = await runScope(securityLane, { env: { DOCS_ONLY_PATHS: "docs/**/*.md", EVENT_ACTION: "opened" }, diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 794bfe9..0bd513e 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -48,9 +48,10 @@ name: claude-review # completed review (`incremental-review`). It reviews the whole pull request # instead when no earlier review is recorded, the recorded head is not an # ancestor of the new head (force push or rebase), 300 or more files changed -# since, or a base-branch merge since then changed a file this pull request -# also changes (or 300 or more files, too many to check). A push that changes -# no file of the pull request is not reviewed again. +# since, a base-branch merge since then changed a file this pull request also +# changes (or 300 or more files, too many to check), or the API returns no +# patch for a changed text file (too large to show). A push that changes no +# file of the pull request is not reviewed again. # # One job reviews and reports. It is named `claude-review-status`, so its check # is ` / claude-review-status`, and it goes red, naming the cause, @@ -112,7 +113,10 @@ on: description: >- Newline-separated path globs (`*` within one path segment, `**` across segments). When every file in the review's scope matches one, - no review runs and the check stays green. Empty never skips. + no review runs and the check stays green. Empty never skips. Agent + instruction files (CLAUDE.md, AGENTS.md, SKILL.md, and anything under + .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/ + or prompts/) never count as documentation, whatever this list says. type: string default: "" outputs: @@ -142,7 +146,7 @@ jobs: # longer step, shortens the reviews that reached it. Outside that step, # 1,169 successful jobs on claude-code-plugins (2026-09-29 to 2026-10-01) # took p95 17 s, max 82 s, which leaves 38 s of 13 minutes (780 s) for the - # scope and record steps. + # scope and record steps; they took 1 s each on ci-workflows#653. timeout-minutes: 13 # The draft and fork tests are scoped to pull_request so a privileged # trigger still reaches the tripwire. @@ -227,8 +231,12 @@ jobs: .map((line) => line.trim()) .filter(Boolean) .map(globToRegExp); + // Agent instructions are never documentation, whatever docs-only-paths says. + const INSTRUCTIONS = + /(?:^|\/)(?:(?:CLAUDE|CLAUDE\.local|AGENTS|GEMINI|SKILL|copilot-instructions)\.md$|(?:\.claude|skills|agents|commands|rules|hooks|instructions|prompts)\/)/u; const names = (file) => [file.filename, file.previous_filename].filter(Boolean); - const isDocs = (file) => names(file).every((name) => docsOnly.some((re) => re.test(name))); + const isDocs = (file) => + names(file).every((name) => !INSTRUCTIONS.test(name) && docsOnly.some((re) => re.test(name))); const compare = async (basehead) => (await github.rest.repos.compareCommitsWithBasehead({ owner, repo, basehead })).data; @@ -257,7 +265,14 @@ jobs: } const patches = new Map(); for (const file of changed) for (const name of names(file)) patches.set(name, file); - return { files: prFiles.filter((file) => names(file).some((name) => patches.has(name))), patches }; + const files = prFiles.filter((file) => names(file).some((name) => patches.has(name))); + // The API drops the patch of a large text change; only a whole review covers it. + const unshown = files.find((file) => { + const change = patches.get(file.filename) ?? patches.get(file.previous_filename); + return change.patch === undefined && change.changes > 0; + }); + if (unshown) return { full: `the API returned no patch for ${unshown.filename}` }; + return { files, patches }; }; const writeDiff = (files, patches) => { diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index 3854061..df61072 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -115,8 +115,9 @@ on: Newline-separated path globs (`*` within one path segment, `**` across segments). When every file in the review's scope matches one, no review runs and the check stays green. Empty never skips. Agent - instruction files (CLAUDE.md, AGENTS.md, skills, rules) are not - documentation here; keep them out of this list. + instruction files (CLAUDE.md, AGENTS.md, SKILL.md, and anything under + .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/ + or prompts/) never count as documentation, whatever this list says. type: string default: | docs/**/*.md @@ -134,10 +135,11 @@ jobs: security-review: name: security-review runs-on: ${{ inputs.runner }} - # Measured on claude-code-plugins, 428 successful jobs: p95 307 s, max - # 1,040 s, which puts ceil(max(1.5 x p95, 1.1 x max) / 60) at 20. Capped at - # the 16-minute ceiling set for every CI job; incremental scoping shortens - # the tail. + # Capped at the 16-minute ceiling set for every CI job (960 s). Outside the + # review step, 1,366 successful jobs on claude-code-plugins (2026-09-29 to + # 2026-10-01) took p95 17 s, max 82 s, and the scope and record steps took + # 1 s each on ci-workflows#653, so the step gets 14 minutes (840 s); + # incremental scoping, not a longer step, shortens the reviews that reach it. timeout-minutes: 16 # The draft and fork tests are scoped to pull_request so a privileged # trigger still reaches the tripwire. @@ -222,8 +224,12 @@ jobs: .map((line) => line.trim()) .filter(Boolean) .map(globToRegExp); + // Agent instructions are never documentation, whatever docs-only-paths says. + const INSTRUCTIONS = + /(?:^|\/)(?:(?:CLAUDE|CLAUDE\.local|AGENTS|GEMINI|SKILL|copilot-instructions)\.md$|(?:\.claude|skills|agents|commands|rules|hooks|instructions|prompts)\/)/u; const names = (file) => [file.filename, file.previous_filename].filter(Boolean); - const isDocs = (file) => names(file).every((name) => docsOnly.some((re) => re.test(name))); + const isDocs = (file) => + names(file).every((name) => !INSTRUCTIONS.test(name) && docsOnly.some((re) => re.test(name))); const compare = async (basehead) => (await github.rest.repos.compareCommitsWithBasehead({ owner, repo, basehead })).data; @@ -252,7 +258,14 @@ jobs: } const patches = new Map(); for (const file of changed) for (const name of names(file)) patches.set(name, file); - return { files: prFiles.filter((file) => names(file).some((name) => patches.has(name))), patches }; + const files = prFiles.filter((file) => names(file).some((name) => patches.has(name))); + // The API drops the patch of a large text change; only a whole review covers it. + const unshown = files.find((file) => { + const change = patches.get(file.filename) ?? patches.get(file.previous_filename); + return change.patch === undefined && change.changes > 0; + }); + if (unshown) return { full: `the API returned no patch for ${unshown.filename}` }; + return { files, patches }; }; const writeDiff = (files, patches) => { @@ -363,7 +376,7 @@ jobs: id: claude-review if: steps.scope.outputs.review != 'false' continue-on-error: true - timeout-minutes: 15 + timeout-minutes: 14 uses: anthropics/claude-code-action@756cc22e19660d20e8cc9496b4f242475a7f7790 # v1.0.235 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/README.md b/README.md index 3441b68..537e4da 100644 --- a/README.md +++ b/README.md @@ -854,7 +854,7 @@ parent secret. | `claude-args` | `--model claude-sonnet-5 --max-turns 75 --allowedTools "Bash(gh pr diff:*)"` | Claude CLI args; the inline-comment grant and a `Skill()` grant are always appended | | `exclude-comments-by-actor` | `dependabot,dependabot[bot]` | Actors whose comments are withheld from the model (prompt-injection hygiene) | | `incremental-review` | `true` | On a later push, review only the files changed since the last completed review | -| `docs-only-paths` | empty (code review); `docs/**/*.md`, `**/README.md`, `**/CHANGELOG.md` (security review) | Globs; when every file in scope matches, no review runs | +| `docs-only-paths` | empty (code review); `docs/**/*.md`, `**/README.md`, `**/CHANGELOG.md` (security review) | Globs; when every file in scope matches, no review runs. Agent-instruction files (`CLAUDE.md`, `AGENTS.md`, `SKILL.md`, anything under `.claude/`, `skills/`, `agents/`, `commands/`, `rules/`, `hooks/`, `instructions/` or `prompts/`) never match | **Skips.** The job skips draft PRs, fork PRs (no secrets reach them; review fork changes by hand) and every bot actor (the action rejects bots it was not @@ -869,9 +869,10 @@ it reviewed in one pull request comment that the job writes with its own token (author `github-actions[bot]`) and edits after each completed review. A push reviews the whole pull request instead when no earlier review is recorded, the recorded head is not an ancestor of the new head (force push or rebase), 300 -or more files changed since, or a base-branch merge since then changed a file -the pull request also changes (or 300 or more files, too many to check). A -push that changes no file of the pull request is not reviewed again. +or more files changed since, a base-branch merge since then changed a file the +pull request also changes (or 300 or more files, too many to check), or the +API returns no patch for a changed text file (too large to show). A push that +changes no file of the pull request is not reviewed again. **Status check, red means no review.** Each lane is one job. The code-review job is named `claude-review-status`, so its check is From b994bca56d6ea7d4f6468a81f30992e1c5c17994 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:15:41 -0400 Subject: [PATCH 5/5] docs(claude-lanes): list every agent-instruction name the docs skip ignores The instruction floor also covers CLAUDE.local.md, GEMINI.md and copilot-instructions.md; the docs-only-paths descriptions and the README now name them, and the test covers the two it missed. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/claude-lane-scope.test.cjs | 2 ++ .github/workflows/claude-review.yml | 7 ++++--- .github/workflows/claude-security-review.yml | 7 ++++--- README.md | 2 +- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/scripts/claude-lane-scope.test.cjs b/.github/scripts/claude-lane-scope.test.cjs index cd2bd04..70b9a01 100644 --- a/.github/scripts/claude-lane-scope.test.cjs +++ b/.github/scripts/claude-lane-scope.test.cjs @@ -609,6 +609,8 @@ test("a caller's wider docs-only-paths never skips agent instructions", async () assert.equal(await decide(["README.md", "docs/a.md"]), "false"); for (const name of [ "CLAUDE.md", + "CLAUDE.local.md", + "GEMINI.md", "sub/AGENTS.md", "plugins/x/skills/y/SKILL.md", "plugins/x/skills/y/reference/notes.md", diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 0bd513e..d99b3cf 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -114,9 +114,10 @@ on: Newline-separated path globs (`*` within one path segment, `**` across segments). When every file in the review's scope matches one, no review runs and the check stays green. Empty never skips. Agent - instruction files (CLAUDE.md, AGENTS.md, SKILL.md, and anything under - .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/ - or prompts/) never count as documentation, whatever this list says. + instruction files (CLAUDE.md, CLAUDE.local.md, AGENTS.md, GEMINI.md, + SKILL.md, copilot-instructions.md, and anything under .claude/, + skills/, agents/, commands/, rules/, hooks/, instructions/ or + prompts/) never count as documentation, whatever this list says. type: string default: "" outputs: diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index df61072..913f3e7 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -115,9 +115,10 @@ on: Newline-separated path globs (`*` within one path segment, `**` across segments). When every file in the review's scope matches one, no review runs and the check stays green. Empty never skips. Agent - instruction files (CLAUDE.md, AGENTS.md, SKILL.md, and anything under - .claude/, skills/, agents/, commands/, rules/, hooks/, instructions/ - or prompts/) never count as documentation, whatever this list says. + instruction files (CLAUDE.md, CLAUDE.local.md, AGENTS.md, GEMINI.md, + SKILL.md, copilot-instructions.md, and anything under .claude/, + skills/, agents/, commands/, rules/, hooks/, instructions/ or + prompts/) never count as documentation, whatever this list says. type: string default: | docs/**/*.md diff --git a/README.md b/README.md index 537e4da..4f8c0e0 100644 --- a/README.md +++ b/README.md @@ -854,7 +854,7 @@ parent secret. | `claude-args` | `--model claude-sonnet-5 --max-turns 75 --allowedTools "Bash(gh pr diff:*)"` | Claude CLI args; the inline-comment grant and a `Skill()` grant are always appended | | `exclude-comments-by-actor` | `dependabot,dependabot[bot]` | Actors whose comments are withheld from the model (prompt-injection hygiene) | | `incremental-review` | `true` | On a later push, review only the files changed since the last completed review | -| `docs-only-paths` | empty (code review); `docs/**/*.md`, `**/README.md`, `**/CHANGELOG.md` (security review) | Globs; when every file in scope matches, no review runs. Agent-instruction files (`CLAUDE.md`, `AGENTS.md`, `SKILL.md`, anything under `.claude/`, `skills/`, `agents/`, `commands/`, `rules/`, `hooks/`, `instructions/` or `prompts/`) never match | +| `docs-only-paths` | empty (code review); `docs/**/*.md`, `**/README.md`, `**/CHANGELOG.md` (security review) | Globs; when every file in scope matches, no review runs. Agent-instruction files (`CLAUDE.md`, `CLAUDE.local.md`, `AGENTS.md`, `GEMINI.md`, `SKILL.md`, `copilot-instructions.md`, anything under `.claude/`, `skills/`, `agents/`, `commands/`, `rules/`, `hooks/`, `instructions/` or `prompts/`) never match | **Skips.** The job skips draft PRs, fork PRs (no secrets reach them; review fork changes by hand) and every bot actor (the action rejects bots it was not