diff --git a/.github/scripts/classify-infra-failure-render.test.cjs b/.github/scripts/classify-infra-failure-render.test.cjs index a44b42c..7f3d9cc 100644 --- a/.github/scripts/classify-infra-failure-render.test.cjs +++ b/.github/scripts/classify-infra-failure-render.test.cjs @@ -70,55 +70,61 @@ function stepSource(workflow, stepName) { return next === -1 ? rest : rest.slice(0, next); } -// Passing the check on an infrastructure failure is an operator-ratified -// non-goal, and until now it was guarded only by reading the file. It is the -// single invariant this whole design rests on: the review lane reports a -// verdict, never an outage. A future edit that adds a nonzero exit to the -// outcome step — or drops continue-on-error from the action step — would flip -// every Anthropic-side blip into a merge blocker across every consumer repo, -// and nothing in CI would have caught it. -test("an infrastructure failure never fails the review job", () => { - for (const name of consumers) { - const source = workflowSource(name); +// The two lanes diverge here, and the difference is the whole point (#266, +// adjudicated; narrowing the #228 non-goal). Infra pass-through is ratified for +// the ADVISORY lane: `review / review` gates nothing whether it is green or red, +// so reddening it on an Anthropic-side blip buys no safety and costs noise — +// an infra failure is not a code-quality signal. +// +// That rationale reasons from merge-irrelevance, so it does not transfer to the +// SECURITY lane, whose check is required precisely to prove a pass ran. There, +// passing on an infra failure certifies an execution that did not happen; it is +// asserted to fail closed by claude-security-review-fail-closed.test.cjs, which +// replays a rate-limited payload through the real outcome step. +// +// Both directions are pinned, because both are one edit away from silently +// inverting across every consumer repo. +test("an infrastructure failure never fails the ADVISORY review job", () => { + const name = "claude-review.yml"; + const source = workflowSource(name); - // The action exits nonzero on infra errors; continue-on-error is what keeps - // that off the check's conclusion. - assert.match( - stepSource( - source, - name === "claude-review.yml" - ? "Claude review" - : "Claude security review", - ), - /^ {8}continue-on-error: true$/mu, - `${name} must keep continue-on-error on the action step`, - ); + // The action exits nonzero on infra errors; continue-on-error is what keeps + // that off the check's conclusion. + assert.match( + stepSource(source, "Claude review"), + /^ {8}continue-on-error: true$/mu, + `${name} must keep continue-on-error on the action step`, + ); - // The outcome step reports the failure; it must not become the failure. - const outcome = stepSource(source, "Report review outcome"); - const exits = [...outcome.matchAll(/^\s*exit\s+(\d+)\s*$/gmu)].map( - (match) => match[1], - ); - assert.ok( - exits.length > 0, - `${name}: expected the outcome step to exit explicitly`, + // The outcome step reports the failure; it must not become the failure. + const outcome = stepSource(source, "Report review outcome"); + const exits = [...outcome.matchAll(/^\s*exit\s+(\d+)\s*$/gmu)].map( + (match) => match[1], + ); + assert.ok( + exits.length > 0, + `${name}: expected the outcome step to exit explicitly`, + ); + for (const code of exits) { + assert.equal( + code, + "0", + `${name}: the advisory lane's outcome step must not exit nonzero on an infra failure (found exit ${code}). If this lane is being promoted to a required check, that is a posture change needing its own ratification — see #266.`, ); - for (const code of exits) { - assert.equal( - code, - "0", - `${name}: the outcome step must not exit nonzero on an infra failure (found exit ${code})`, - ); - } + } +}); - // Nor may the steps that surface the failure turn it into one. These are - // actions/github-script steps, so a shell `exit` check would never fire on - // them. Two mechanisms can fail a JS step: an explicit failure call, and an - // unhandled rejection — every one of these steps awaits GitHub API calls - // that can reject for reasons unrelated to the code under review, and - // github-script turns a rejection into a failed step on its own. Only - // continue-on-error covers that second path, so it is asserted rather than - // assumed. +// Applies to BOTH lanes: whatever a lane's conclusion policy is, the steps that +// merely annotate the PR must never be what decides it. These are +// actions/github-script steps, so a shell `exit` check would never fire on them. +// Two mechanisms can fail a JS step: an explicit failure call, and an unhandled +// rejection — every one of these steps awaits GitHub API calls that can reject +// for reasons unrelated to the code under review, and github-script turns a +// rejection into a failed step on its own. Only continue-on-error covers that +// second path, so it is asserted rather than assumed. +test("the comment steps never decide either lane's conclusion", () => { + for (const name of consumers) { + const source = workflowSource(name); for (const step of [ "Comment on genuine review failure", "Clear stale failure comment after successful review", diff --git a/.github/scripts/claude-security-review-fail-closed.test.cjs b/.github/scripts/claude-security-review-fail-closed.test.cjs new file mode 100644 index 0000000..814035d --- /dev/null +++ b/.github/scripts/claude-security-review-fail-closed.test.cjs @@ -0,0 +1,399 @@ +"use strict"; + +// The security lane's required check certifies EXECUTION, so "in scope and could +// not run" must conclude failure — `neutral` and `skipped` both satisfy a +// required check and cannot express it (#266). These tests run the outcome step's +// real script rather than grepping for `exit 1`, because the thing worth pinning +// is the exit code a replayed 429 payload actually produces. + +const assert = require("node:assert/strict"); +const { spawnSync } = require("node:child_process"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); +const test = require("node:test"); + +const repositoryRoot = path.join(__dirname, "..", ".."); +const workflowPath = path.join( + repositoryRoot, + ".github", + "workflows", + "claude-security-review.yml", +); +const workflow = fs.readFileSync(workflowPath, "utf8"); + +function stepSource(stepName) { + const start = workflow.indexOf(` - name: ${stepName}\n`); + assert.notEqual(start, -1, `step not found: ${stepName}`); + const rest = workflow.slice(start + 1); + const next = rest.indexOf("\n - name: "); + return next === -1 ? rest : rest.slice(0, next); +} + +// The `run:` body is plain shell with no ${{ }} interpolation, which is what +// makes executing it here faithful rather than an approximation. +function runScript(stepName) { + const step = stepSource(stepName); + const marker = " run: |\n"; + const start = step.indexOf(marker); + assert.notEqual(start, -1, `${stepName} has no literal run block`); + const body = step.slice(start + marker.length); + assert.doesNotMatch( + body, + /\$\{\{/u, + `${stepName}'s run block interpolates a github expression, so executing it here would not match CI`, + ); + return body + .split("\n") + .map((line) => (line.startsWith(" ") ? line.slice(10) : line)) + .join("\n"); +} + +// Run 30217744377's payload: the shape a rate-limited call leaves behind — the +// SDK's success variant with is_error true and no turns taken. This is the run +// #266 was filed from, and it concluded green. +function rateLimitedExecutionFile() { + return JSON.stringify([ + { + type: "result", + subtype: "success", + is_error: true, + num_turns: 1, + duration_ms: 480, + total_cost_usd: 0, + result: "model-authored free text that must never be published", + api_error_status: 429, + }, + ]); +} + +function reportOutcome({ + outcome, + executionFileContents, + isPullRequest = "true", +}) { + const directory = fs.mkdtempSync( + path.join(os.tmpdir(), "security-review-outcome-"), + ); + try { + const githubOutput = path.join(directory, "github-output"); + fs.writeFileSync(githubOutput, ""); + const environment = { + ...process.env, + REVIEW_OUTCOME: outcome, + IS_PULL_REQUEST: isPullRequest, + GITHUB_OUTPUT: githubOutput, + }; + if (executionFileContents !== undefined) { + const executionFile = path.join(directory, "execution.json"); + fs.writeFileSync(executionFile, executionFileContents); + environment.EXECUTION_FILE = executionFile; + } + const result = spawnSync( + "bash", + ["-c", runScript("Report review outcome")], + { + encoding: "utf8", + env: environment, + }, + ); + const outputs = Object.fromEntries( + fs + .readFileSync(githubOutput, "utf8") + .split("\n") + .filter((line) => line.includes("=")) + .map((line) => { + const separator = line.indexOf("="); + return [line.slice(0, separator), line.slice(separator + 1)]; + }), + ); + return { ...result, outputs }; + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } +} + +test("a replayed rate-limited review fails the job and still reports its class", () => { + const result = reportOutcome({ + outcome: "failure", + executionFileContents: rateLimitedExecutionFile(), + }); + + assert.equal( + result.status, + 1, + `the outcome step must exit nonzero so the required check concludes failure\n${result.stdout}\n${result.stderr}`, + ); + assert.equal(result.outputs.review_failed, "true"); + // Written before the exit, so the PR still gets its infra-status comment. + assert.equal(result.outputs.failure_class, "rate-limit"); + assert.match(result.stdout, /::error::Claude security review exited with:/u); +}); + +// The free-text SDK fields stay off this public repo's logs even on the path that +// now fails the job. +test("failing closed does not start publishing the model-authored result", () => { + const result = reportOutcome({ + outcome: "failure", + executionFileContents: rateLimitedExecutionFile(), + }); + assert.doesNotMatch(result.stdout, /must never be published/u); + for (const value of Object.values(result.outputs)) { + assert.doesNotMatch(value, /must never be published/u); + } +}); + +test("a completed review still passes", () => { + const result = reportOutcome({ outcome: "success" }); + assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); + assert.equal(result.outputs.review_failed, "false"); +}); + +// A missing execution file must not become a second failure mode: the class +// degrades to `other` and the job still fails closed on the same signal. +test("an unreadable execution file still fails closed", () => { + const result = reportOutcome({ outcome: "failure" }); + assert.equal(result.status, 1, `${result.stdout}\n${result.stderr}`); + assert.equal(result.outputs.review_failed, "true"); + assert.equal(result.outputs.failure_class, "other"); +}); + +// The action rejects merge_group outright and rejects every non-PR event while +// track_progress is on, so those runs fail for a cause no head change can fix. +// Failing them closed would wedge a consumer's merge queue on a red check that +// the PR-gated comment steps cannot even explain. +test("a non-pull_request run reports the failure without failing the job", () => { + const result = reportOutcome({ + outcome: "failure", + executionFileContents: rateLimitedExecutionFile(), + isPullRequest: "false", + }); + assert.equal( + result.status, + 0, + `merge_group / workflow_dispatch / schedule runs must keep the historical pass-through\n${result.stdout}\n${result.stderr}`, + ); + // Still classified and still annotated — pass-through, not silence. + assert.equal(result.outputs.review_failed, "true"); + assert.equal(result.outputs.failure_class, "rate-limit"); +}); + +// The resolve step decides what the outcome step sees, so a bug here reddens +// every CLEAN review — the widest blast radius in this workflow. +function resolveAttempt({ first, firstFile, retry, retryFile }) { + const directory = fs.mkdtempSync( + path.join(os.tmpdir(), "security-review-attempt-"), + ); + try { + const githubOutput = path.join(directory, "github-output"); + fs.writeFileSync(githubOutput, ""); + const result = spawnSync( + "bash", + ["-c", runScript("Resolve the effective review attempt")], + { + encoding: "utf8", + env: { + ...process.env, + FIRST_OUTCOME: first, + FIRST_FILE: firstFile, + RETRY_OUTCOME: retry, + RETRY_FILE: retryFile, + GITHUB_OUTPUT: githubOutput, + }, + }, + ); + assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); + return Object.fromEntries( + fs + .readFileSync(githubOutput, "utf8") + .split("\n") + .filter((line) => line.includes("=")) + .map((line) => { + const separator = line.indexOf("="); + return [line.slice(0, separator), line.slice(separator + 1)]; + }), + ); + } finally { + fs.rmSync(directory, { recursive: true, force: true }); + } +} + +test("a clean first attempt resolves to itself when the retry is skipped", () => { + // Actions sets a skipped step's outcome to the literal "skipped"; resolving + // that as the effective outcome would fail every clean review closed. + for (const skipped of ["skipped", ""]) { + const resolved = resolveAttempt({ + first: "success", + firstFile: "/tmp/first.json", + retry: skipped, + retryFile: "", + }); + assert.equal( + resolved.outcome, + "success", + `retry outcome ${skipped || "(empty)"}`, + ); + assert.equal(resolved.execution_file, "/tmp/first.json"); + } +}); + +test("a successful retry supersedes the failed first attempt", () => { + const resolved = resolveAttempt({ + first: "failure", + firstFile: "/tmp/first.json", + retry: "success", + retryFile: "/tmp/retry.json", + }); + assert.equal(resolved.outcome, "success"); + // The first attempt's payload would classify a failure that no longer applies. + assert.equal(resolved.execution_file, "/tmp/retry.json"); +}); + +test("both attempts failing resolves to the retry's payload", () => { + const resolved = resolveAttempt({ + first: "failure", + firstFile: "/tmp/first.json", + retry: "failure", + retryFile: "/tmp/retry.json", + }); + assert.equal(resolved.outcome, "failure"); + assert.equal(resolved.execution_file, "/tmp/retry.json"); +}); + +// Everything below pins the three no-verdict paths that must NOT reach the +// failing step. Each is the reason the mapping keys on review_failed rather than +// on "no verdict was produced". +test("fork PRs skip the job instead of failing closed forever", () => { + const jobCondition = workflow.slice( + workflow.indexOf(" security-review:"), + workflow.indexOf(" - name: Reject privileged triggers"), + ); + + // A fork run gets no secrets, so no number of retries can make it pass. The + // two halves are asserted as ONE clause on purpose: the fork test scoped to + // pull_request is a security control, not a style choice. Unscoped, it also + // matches a fork PR arriving via pull_request_target or workflow_run and + // would skip the job before `Reject privileged triggers` could hard-fail it, + // turning a consumer's dangerous misconfiguration into a green check. + assert.match( + jobCondition, + /\(github\.event_name != 'pull_request'\s+\|\| github\.event\.pull_request\.head\.repo\.full_name == github\.repository\)/u, + "the job must skip fork PRs, and the fork test must stay scoped to pull_request so privileged triggers still reach the tripwire", + ); + assert.match( + jobCondition, + /needs\.changes\.outputs\.relevant != 'false'/u, + "out-of-scope PRs must still skip at job level", + ); + assert.match( + jobCondition, + /contains\(format\(',\{0\},', inputs\.skip-actors\)/u, + "the skip-actors exception is operator-ratified (ADR 0002) and must stay job-level", + ); +}); + +test("a superseded head reports nothing, so it cannot fail closed", () => { + const step = stepSource("Report review outcome"); + assert.match( + step, + /^ {8}if: always\(\) && steps\.freshness\.outputs\.superseded != 'true'$/mu, + "a retired run must skip the outcome step, leaving review_failed unset", + ); +}); + +// Actions cannot loop a `uses:` step, so the retry is a verbatim copy of the +// first attempt. Divergence between the two would mean the retry reviews under +// different rules than the attempt it replaces — asserted, not trusted. +test("the review retry is configured identically to the first attempt", () => { + // A step's source runs up to the next `- name:`, which drags in that step's + // leading comment block — trailing blanks and comments are trimmed so the + // comparison is over inputs only. + const withBlock = (stepName) => { + const step = stepSource(stepName); + const start = step.indexOf(" with:\n"); + assert.notEqual(start, -1, `${stepName} has no with: block`); + const lines = step.slice(start).split("\n"); + while ( + lines.length > 0 && + /^\s*(#.*)?$/u.test(lines[lines.length - 1] ?? "") + ) { + lines.pop(); + } + return lines.join("\n"); + }; + + assert.equal( + withBlock("Claude security review (retry)"), + withBlock("Claude security review"), + "the retry's inputs have drifted from the first attempt's", + ); + + const retry = stepSource("Claude security review (retry)"); + assert.match( + retry, + /^ {8}continue-on-error: true$/mu, + "the retry must not fail the job itself; the outcome step owns the red", + ); + assert.match( + retry, + /steps\.claude-review\.outcome == 'failure'/u, + "the retry must run only after a failed first attempt", + ); + assert.match( + retry, + /steps\.freshness\.outputs\.superseded != 'true'/u, + "a superseded run must not spend a retry", + ); + + // Same pin, or the retry is a different action than the one that was reviewed. + const pin = /uses: (anthropics\/claude-code-action@[0-9a-f]{40})/u; + assert.equal( + retry.match(pin)?.[1], + stepSource("Claude security review").match(pin)?.[1], + "the retry must pin the same action SHA as the first attempt", + ); + + assert.match( + stepSource("Back off before the review retry"), + /^ {8}run: sleep \d+$/mu, + "the retry must back off rather than immediately re-hammering a rate-limited API", + ); +}); + +test("the outcome step reads the resolved attempt, not just the first one", () => { + const step = stepSource("Report review outcome"); + assert.match( + step, + /^ {10}REVIEW_OUTCOME: \$\{\{ steps\.attempt\.outputs\.outcome \}\}$/mu, + "reading steps.claude-review directly would ignore a successful retry", + ); + assert.match( + step, + /^ {10}EXECUTION_FILE: \$\{\{ steps\.attempt\.outputs\.execution_file \}\}$/mu, + "the classified payload must come from the attempt that actually ran last", + ); + // The harness supplies this variable itself, so without pinning the YAML a + // deleted or incorrect env line would leave it unset in CI and every event + // would take the pass-through branch — reverting #266 silently, and in the + // fail-OPEN direction this suite exists to prevent. + assert.match( + step, + /^ {10}IS_PULL_REQUEST: \$\{\{ github\.event\.pull_request\.number != '' \}\}$/mu, + "a missing or incorrect IS_PULL_REQUEST disables fail-closed on every event", + ); +}); + +// Belt and braces with the assertion above: even if the wiring is lost, the +// guard's sense must keep an unset value on the fail-closed path. +test("an unset IS_PULL_REQUEST fails closed rather than passing through", () => { + const result = reportOutcome({ + outcome: "failure", + executionFileContents: rateLimitedExecutionFile(), + isPullRequest: "", + }); + assert.equal( + result.status, + 1, + `an absent IS_PULL_REQUEST must not be read as "not a pull request"\n${result.stdout}\n${result.stderr}`, + ); +}); diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index d0079dd..407ef57 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -6,17 +6,42 @@ name: claude-security-review # ADVISORY VERDICT. A whole-job concern (own job permissions + secrets # interface), so a reusable workflow, not a composite action. # -# POSTURE — advisory VERDICT, name-stable EXECUTION check: -# This lane POSTS a security review as PR comments; the Claude step is -# continue-on-error so an OIDC/usage-limit/SDK blip does not show as a red -# check, and findings never fail ci-status. The VERDICT (what the review says) -# stays advisory. What a consumer CAN gate is EXECUTION EVIDENCE: a ruleset -# may make the `security-review` check REQUIRED, so every protected-branch PR -# carries proof a security pass ran — or was judged not-applicable. The -# intended promotion path for the verdict itself: flip to blocking on CRITICAL +# POSTURE — advisory VERDICT, FAIL-CLOSED EXECUTION check: +# This lane POSTS a security review as PR comments. The VERDICT (what the +# review says) stays ADVISORY: findings never fail the job, and the intended +# promotion path for the verdict itself is to flip to blocking on CRITICAL # findings once the lane's precision is proven over a sustained window — an # earned promotion (trust-before-scale), mirroring the guardrail-matrix -# verification-promotion discipline. Until then the verdict stays advisory. +# verification-promotion discipline. +# +# EXECUTION is different, and it FAILS CLOSED. A consumer's ruleset may make +# the `security-review` check REQUIRED, so the check's whole claim is that a +# security pass RAN — or was judged not-applicable. When the PR IS in scope +# and the review could not run at all (usage limit, dead credential, SDK +# crash), the check therefore reports FAILURE: it cannot certify an execution +# that did not happen. `success`, `neutral` and `skipped` all satisfy a +# required check, so failure is the only conclusion that does not silently +# authorize a merge on absent evidence. +# +# Scope of that claim, stated precisely: it holds for `pull_request` runs. +# Fork PRs and non-PR events (`merge_group`, `workflow_dispatch`, `schedule`) +# cannot run the review at all — no secrets, or an event the action rejects — +# so they keep the pass-through and their green is NOT execution evidence. +# See the fork bullet below and the guard in `Report review outcome`. +# +# NARROWING AMENDMENT (#266, adjudicated; amends the #228 non-goal): infra +# pass-through is ratified for ADVISORY lanes — claude-review.yml still passes +# through, because a red check there gates nothing and an infra failure is not +# a code-quality signal. That rationale reasons from merge-irrelevance and so +# does not transfer here, where the check is the sole required security +# context. Required execution-evidence contexts fail closed. Measured before +# the change: 55 of 129 in-scope merges (42.6%) carried a green required check +# with no security pass at the merge head. +# +# Availability is bought back two ways, not by weakening the claim: a bounded +# in-job retry absorbs the sporadic-429 class, and a sustained outage is meant +# to be overridden by an explicit, logged, attributable break-glass on the +# consumer's ruleset — never by a check that lies. # # ALWAYS-REPORT SHAPE — the caller does NOT path-filter at workflow level: # A security pass on every PR is noise in a doc-heavy repo where most PRs are @@ -43,7 +68,14 @@ name: claude-security-review # pull_request: # types: [opened, synchronize, ready_for_review, reopened] # merge_group: # ONLY if the consumer runs a merge queue — without it -# # the required check never reports for queued PRs +# # the required check never reports for queued PRs. +# # CAVEAT: the pinned action cannot serve merge_group (it +# # is not a supported event type), so a queued run reports +# # GREEN WITHOUT REVIEWING. That unblocks the queue, which +# # is why the trigger is still listed — but on this event +# # the check is NOT execution evidence, so a merge queue +# # is not covered the way a pull_request gate is. Review +# # of the PR itself is what carries the evidence. # jobs: # security-review: # permissions: @@ -75,7 +107,13 @@ name: claude-security-review # The `reject-privileged-triggers` step is a tripwire that hard-fails those # two events; every other event is allowed (the consumer keeps flexibility). # - Fork PRs receive no secrets and a read-only token by design, so they are -# simply not reviewed — that is correct, not a gap to "fix". +# simply not reviewed — that is correct, not a gap to "fix". The job SKIPS +# them at job level rather than letting a doomed, secretless action run: with +# the fail-closed mapping above, running would turn every fork PR red for a +# cause no push can fix. A skipped job is name-stable and a ruleset reads it +# as success, exactly as for an out-of-scope PR — so a required check does +# NOT prove a fork PR was reviewed, and a human must review fork changes to +# security-sensitive surfaces before merge. # - The action gates triggering on the actor's write access; bots are blocked # unless named in `allowed_bots`. # - `display_report` / `show_full_output` stay off (public-repo log-leak risk). @@ -145,7 +183,17 @@ on: skip-actors: description: >- Comma-separated actors (no spaces) for whom the job skips entirely, to - save runner minutes on PRs that never need review. Empty reviews all. + save runner minutes on PRs that never need review. + + Empty reviews all — but removing the default `dependabot[bot]` carries + a SECRET CONTRACT. A Dependabot-triggered run reads from the separate + Dependabot secrets store, never from Actions or organization secrets + (GitHub: "When a Dependabot event triggers a workflow, the only secrets + available to the workflow are Dependabot secrets"). Unless the consumer + mirrors CLAUDE_CODE_OAUTH_TOKEN into that store, the review cannot + authenticate, and because this lane FAILS CLOSED the required check + then blocks every Dependabot PR until an operator provisions it. + Mirror the secret first, or keep dependabot[bot] in this list. type: string default: dependabot[bot] secrets: @@ -280,7 +328,9 @@ jobs: security-review: needs: changes runs-on: ${{ inputs.runner }} - timeout-minutes: 30 + # Two review attempts plus the backoff now share this budget; sized so a slow + # first attempt cannot starve the retry into a timeout-shaped red check. + timeout-minutes: 45 # Skip configured bot/automation actors (saves runner minutes). The action's # own write-access gate is the security control; this is just an optimization. # Comma-wrap both sides so a substring of one actor can't match another. @@ -293,10 +343,26 @@ jobs: # review still runs. A silent skip would let unreviewed security-relevant # changes merge, so paying for a run beats a silent evidence gap. always() # is deliberately avoided — a cancelled workflow should still stop. + # + # The fork clause is load-bearing for the FAIL-CLOSED mapping (see POSTURE): + # a fork-triggered pull_request run gets no secrets, so the SDK call cannot + # succeed no matter how often it is retried. Without this skip the mapping + # would pin every fork PR red permanently. + # + # It is scoped to `pull_request` for a security reason, not a stylistic one. + # An unscoped fork test also matches a fork PR arriving via + # pull_request_target / workflow_run — the two privileged triggers this + # workflow exists to reject — and would skip the job before `Reject + # privileged triggers` could hard-fail it, turning a consumer's dangerous + # misconfiguration into a green skipped check. Every non-pull_request event + # must reach that tripwire; the ones it allows through then take the + # pass-through path in `Report review outcome`. if: >- ${{ !cancelled() && needs.changes.outputs.relevant != 'false' - && !contains(format(',{0},', inputs.skip-actors), format(',{0},', github.actor)) }} + && !contains(format(',{0},', inputs.skip-actors), format(',{0},', github.actor)) + && (github.event_name != 'pull_request' + || github.event.pull_request.head.repo.full_name == github.repository) }} permissions: contents: read pull-requests: write @@ -391,8 +457,15 @@ jobs: id: claude-review if: steps.freshness.outputs.superseded != 'true' # claude-code-action exits non-zero only on infrastructure errors; review - # findings go to PR comments, not the exit code. continue-on-error keeps - # an OIDC/usage-limit/SDK blip from showing as a red check. + # findings go to PR comments, not the exit code. + # + # continue-on-error stays, but its job here is NOT to hide the failure + # (this lane fails closed — see POSTURE). It keeps the ACTION step from + # ending the job so the retry below gets its turn and `Report review + # outcome` can classify the failure into a machine-readable class. The + # red conclusion is raised deliberately by that outcome step, on the one + # signal that means "in scope and did not run", instead of incidentally + # by whichever step happened to exit first. continue-on-error: true uses: anthropics/claude-code-action@12531344451323133b0493233c759991ac61da12 # v1.0.174 with: @@ -420,6 +493,87 @@ jobs: commit blob (not the PR) using a permalink with line numbers: https://github.com/${{ github.repository }}/blob/${{ github.event.pull_request.head.sha }}/#L + # ONE bounded retry, because this lane now fails closed: a sporadic 429 + # that a re-run would clear must not block a merge. Measured motivation — + # run 30217744377 failed in 25s with api_error_status 429 and returned a + # real verdict in 3m17s on a manual re-run. + # + # Deliberately ONE retry, not a loop: Actions cannot loop a `uses:` step, + # so every attempt is a verbatim copy of the step above, and each copy is a + # place where the two can silently diverge (the render test asserts they do + # not). The measured failure population does not justify more — sustained + # multi-hour blackouts dominate, and no in-job backoff survives those. They + # are meant to be handled by the consumer's break-glass, not by retrying. + # Gated on being a pull_request too: on the events the action rejects + # outright the first attempt always fails, so retrying only spends a + # runner and a minute of wall clock to throw again. + - name: Back off before the review retry + if: >- + steps.freshness.outputs.superseded != 'true' && + steps.claude-review.outcome == 'failure' && + github.event.pull_request.number != '' + run: sleep 60 + + - name: Claude security review (retry) + id: claude-review-retry + if: >- + steps.freshness.outputs.superseded != 'true' && + steps.claude-review.outcome == 'failure' && + github.event.pull_request.number != '' + # Same rationale as the first attempt: the outcome step owns the red. + continue-on-error: true + uses: anthropics/claude-code-action@12531344451323133b0493233c759991ac61da12 # v1.0.174 + with: + claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} + # A clean security pass otherwise produces no visible output (upstream + # #1071); the tracking comment guarantees a "no findings" signal. + track_progress: true + # Off on this public lane (log-leak risk); hardcoded, not a flippable + # input, so a consumer cannot turn it on. + display_report: false + allowed_bots: dependabot[bot] + exclude_comments_by_actor: dependabot[bot] + claude_args: ${{ inputs.claude-args }} + # github-context expressions must appear directly in the step (a value + # carried in via inputs.prompt is NOT re-evaluated), so the structural + # header + permalink guidance live here and wrap the consumer's body. + prompt: | + REPO: ${{ github.repository }} + PR NUMBER: ${{ github.event.pull_request.number }} + HEAD SHA: ${{ github.event.pull_request.head.sha }} + + ${{ inputs.prompt }} + + When referencing specific code locations in findings, link to the + commit blob (not the PR) using a permalink with line numbers: + https://github.com/${{ github.repository }}/blob/${{ github.event.pull_request.head.sha }}/#L + + # Collapse the two attempts into the one pair of facts the outcome step + # needs. Reading the retry's fields directly would be wrong: when it is + # skipped its outcome is the literal `skipped` and its execution_file is + # empty, so the first attempt's result has to be the fallback. + - name: Resolve the effective review attempt + id: attempt + if: always() && steps.freshness.outputs.superseded != 'true' + env: + FIRST_OUTCOME: ${{ steps.claude-review.outcome }} + FIRST_FILE: ${{ steps.claude-review.outputs.execution_file }} + RETRY_OUTCOME: ${{ steps.claude-review-retry.outcome }} + RETRY_FILE: ${{ steps.claude-review-retry.outputs.execution_file }} + run: | + if [ "$RETRY_OUTCOME" = "skipped" ] || [ -z "$RETRY_OUTCOME" ]; then + outcome="$FIRST_OUTCOME" + execution_file="$FIRST_FILE" + else + echo "First attempt failed; reporting the retry's result instead." + outcome="$RETRY_OUTCOME" + execution_file="$RETRY_FILE" + fi + { + echo "outcome=$outcome" + echo "execution_file=$execution_file" + } >>"$GITHUB_OUTPUT" + # Also gated on the guard: a superseded run has no outcome to report, and # skipping this step leaves review_failed empty so both marker-comment # steps below skip too — a retired run must not touch the newer run's PR @@ -428,8 +582,9 @@ jobs: id: review-outcome if: always() && steps.freshness.outputs.superseded != 'true' env: - REVIEW_OUTCOME: ${{ steps.claude-review.outcome }} - EXECUTION_FILE: ${{ steps.claude-review.outputs.execution_file }} + REVIEW_OUTCOME: ${{ steps.attempt.outputs.outcome }} + EXECUTION_FILE: ${{ steps.attempt.outputs.execution_file }} + IS_PULL_REQUEST: ${{ github.event.pull_request.number != '' }} run: | if [ "$REVIEW_OUTCOME" = "success" ]; then echo "Claude security review completed. Findings (if any) are in the PR comments." @@ -557,6 +712,38 @@ jobs: echo "review_failed=true" >> "$GITHUB_OUTPUT" + # Only a pull_request run gates a merge, so only a pull_request run + # fails closed. The action rejects other events outright — `merge_group` + # is not a supported event type at all, and `track_progress` (hardcoded + # on above) rejects every non-PR event — so those runs fail for a reason + # the head cannot fix and no retry can clear. Reddening them would wedge + # a consumer's merge queue permanently, and the comment steps below are + # PR-gated too, so the red would carry no explanation. They keep the + # historical pass-through: still annotated, still classified, not fatal. + # Tested for the PASS-THROUGH value, not the fail-closed one, so the + # default is closed: if this variable is ever unset or garbled — a + # deleted env line, a renamed step — the comparison is false and the + # run still fails closed. Written as `!= "true"` it would do the + # opposite, silently reverting #266 on every event. + if [ "$IS_PULL_REQUEST" = "false" ]; then + echo "Not a pull_request run; reporting the failure without failing the job." + exit 0 + fi + + # FAIL CLOSED (#266). Ordering is load-bearing: review_failed and the + # class are written to GITHUB_OUTPUT above, before this exit, so the + # two `always()`-gated comment steps below still fire and the PR still + # gets its infra-status comment. Exiting here is what makes the check + # report FAILURE, and failure is the only conclusion a required check + # does not accept — `neutral` and `skipped` both read as success, so + # neither can express "in scope and did not run". + # + # Reached only when the review was in scope, the head is still current, + # and both attempts failed. The three legitimate no-verdict paths never + # arrive here: out-of-scope and fork/skip-actor PRs skip the whole job, + # and a superseded head skips this step (leaving review_failed unset). + exit 1 + # Runs only on a genuine infra failure (never on a clean review) and only # when the token can write: fork-triggered pull_request runs get a # read-only GITHUB_TOKEN and no secrets by design (CLAUDE.md), so @@ -598,7 +785,13 @@ jobs: `> - Failure class: \`${process.env.REVIEW_CLASS}\``, `> - Last SDK result: \`${process.env.REVIEW_DETAIL}\``, ">", - "> Re-running the job, or pushing a new commit, will retry the review.", + "> **The check is red on purpose.** It certifies that a security pass ran, and this one did not complete, so it cannot report success. Where this check is required, merging is blocked until a review actually finishes.", + ">", + "> Re-running the job, or pushing a new commit, will retry the review — one automatic retry already ran.", + ">", + "> Re-running does NOT help for every class:", + "> - `rate-limit` that persists across re-runs, or `auth` — the credential or usage budget needs an **operator**; retrying will not clear it.", + "> - a run that exhausted its turn budget (`\"subtype\":\"error_max_turns\"` above) will exhaust it again. **As the PR author**, split the change into smaller PRs; raising `--max-turns` is a change to the *caller workflow*, not something you can set on this PR.", ].join("\n"); const comments = await github.paginate(github.rest.issues.listComments, {