diff --git a/.github/scripts/claude-lane-scope.test.cjs b/.github/scripts/claude-lane-scope.test.cjs new file mode 100644 index 0000000..70b9a01 --- /dev/null +++ b/.github/scripts/claude-lane-scope.test.cjs @@ -0,0 +1,632 @@ +"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 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"); +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", lane: "claude-review" }, + { + file: "claude-security-review.yml", + job: "security-review", + lane: "claude-security-review", + }, +]; + +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 BOT = "github-actions[bot]"; + +const file = (filename, extra = {}) => ({ + filename, + status: "modified", + ...extra, +}); + +async function runScript(script, values, github) { + const outputs = {}; + const messages = []; + const core = { + setOutput: (key, value) => { + outputs[key] = value; + }, + 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 = { + 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"); + return { data: prFiles }; + }, + }, + repos: { + compareCommitsWithBasehead: async ({ basehead }) => { + assert.ok(basehead in compares, `unexpected compare ${basehead}`); + return { data: compares[basehead] }; + }, + }, + }, + }; + 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, + ); + 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. +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, + recordScript: stepNamed(job, "Record the reviewed head").with.script, + }; +}); + +test("both lanes run the same scope and record scripts", () => { + assert.equal(codeLane.script, securityLane.script); + assert.equal(codeLane.recordScript, securityLane.recordScript); + for (const script of [codeLane.script, codeLane.recordScript]) { + assert.doesNotMatch(script, /\$\{\{/u); + } +}); + +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]) { + 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.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 }}`); + 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 lives in the lane's PR comment, never the Actions cache`, () => { + const save = stepNamed(job, "Record the reviewed head"); + 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( + save.env.HEAD_SHA, + `\${{ github.event.pull_request.head.sha }}`, + ); + 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`, () => { + 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(`${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, { + env: { EVENT_ACTION: action }, + prFiles: [file("src/a.js")], + 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.outputs["marker-id"], "41"); + } +}); + +test("a push with no recorded review reviews the whole pull request", async () => { + const result = await runScope(codeLane, { + 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, { + 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"), + ]), + comments: [await markerFor(codeLane, 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.outputs.record, "true"); +}); + +test("a push that changes no file of the pull request is not reviewed again", async () => { + const result = await runScope(codeLane, { + prFiles: [file("src/a.js")], + compares: unmovedBase([file("elsewhere.js")]), + comments: [await markerFor(codeLane, 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, { + prFiles: [file("src/a.js")], + 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, { + 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")], + }, + }, + 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 .* 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, { + 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")] }, + }, + 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 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, { + prFiles: [file("src/a.js")], + compares: { [`${LAST}...${HEAD}`]: since }, + 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("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, { + 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, { + prFiles: [], + fail: true, + comments: [await markerFor(codeLane, 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 reads, narrows nor records", async () => { + const result = await runScope(codeLane, { + env: { INCREMENTAL: "false" }, + prFiles: [file("src/a.js")], + comments: [await markerFor(codeLane, LAST)], + }); + assert.equal(result.outputs.review, "true"); + assert.equal(result.outputs.record, undefined); + assert.equal(result.listedComments, 0); +}); + +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, { + 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 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", + "CLAUDE.local.md", + "GEMINI.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" }, + 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..5f3ddd0 100644 --- a/.github/scripts/claude-lane-status-check.test.cjs +++ b/.github/scripts/claude-lane-status-check.test.cjs @@ -1,10 +1,12 @@ "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 +// 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"); @@ -18,11 +20,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: "security-review", }, ]; @@ -37,9 +39,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,40 +60,43 @@ 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], + 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.JOB_STATUS, `\${{ job.status }}`); + assert.equal( + step.env.SKIP_REASON, + `\${{ steps.scope.outputs.skip-reason }}`, + ); assert.equal( step.env.REVIEW_FAILED, - `\${{ needs.${lane.needs}.outputs.review-failed }}`, + `\${{ steps.review-outcome.outputs.review-failed }}`, ); assert.equal( step.env.FAILURE_CLASS, - `\${{ needs.${lane.needs}.outputs.failure-class }}`, + `\${{ steps.review-outcome.outputs.failure-class }}`, ); assert.doesNotMatch(step.run, /\$\{\{/u); }); @@ -99,6 +107,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,11 +126,10 @@ 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, }); diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 7ea0033..d99b3cf 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -35,11 +35,29 @@ 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 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. # -# 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), 300 or more files changed +# 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, +# 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 +102,29 @@ 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, 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: 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 +141,14 @@ permissions: jobs: review: + name: claude-review-status runs-on: ${{ inputs.runner }} - timeout-minutes: 15 + # 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; 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. if: >- @@ -139,11 +181,189 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} filter: blob:none + # 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 }} + 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 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 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 +484,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..913f3e7 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,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 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. # -# 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; 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 `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: @@ -88,6 +103,27 @@ 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, 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 + **/README.md + **/CHANGELOG.md secrets: CLAUDE_CODE_OAUTH_TOKEN: description: Claude Code OAuth token (from `claude setup-token`). @@ -98,8 +134,14 @@ permissions: jobs: security-review: + name: security-review runs-on: ${{ inputs.runner }} - timeout-minutes: 25 + # 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. if: >- @@ -132,11 +174,189 @@ jobs: ref: ${{ github.event.pull_request.head.sha }} filter: blob:none + # 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 }} + 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 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 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 +470,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 51f539e..4f8c0e0 100644 --- a/README.md +++ b/README.md @@ -806,7 +806,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 @@ -852,20 +853,41 @@ 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. 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 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`, -`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. +**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`. 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, 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 +` / 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.