From 91f3030a3e623d5e9086ccdcca9ceee14eea53b7 Mon Sep 17 00:00:00 2001
From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com>
Date: Sun, 26 Jul 2026 18:19:40 -0400
Subject: [PATCH 1/5] fix(claude-security-review): fail the required check when
 an in-scope review could not run

The security lane's check is promoted to a required context to prove a
security pass RAN (ADR 0002's #509 addendum: "proves the pass ran; does
not gate on the verdict"). It reported success when the SDK call failed,
so it certified an execution that never happened.

Measured on claude-code-plugins before this change: 55 of 129 in-scope
merges (42.6%) since 2026-07-25 landed on main with the check green and
no security pass at the merge head.

`success`, `neutral` and `skipped` all satisfy a required check, so
failure is the only conclusion that can express "in scope and did not
run". The mapping keys on `review_failed == 'true'`, which is precise
against all three legitimate no-verdict paths: out-of-scope and
skip-actor PRs skip at job level (ADR 0002's ratified exception is
untouched), and a superseded head leaves the output unset.

Fork PRs now skip at job level. They get no secrets, so the review can
never succeed and the mapping would otherwise pin every fork PR red
permanently. A skipped job is name-stable and reads as success, so the
required check does not prove a fork PR was reviewed.

One bounded retry with a 60s backoff absorbs the sporadic-429 class.
It is deliberately not a substitute for the fail-closed mapping:
sustained multi-hour blackouts dominate the measured failures and no
in-job backoff survives those.

Narrows #228's pass-through non-goal rather than contradicting it:
pass-through stays ratified for advisory lanes, whose rationale reasons
from merge-irrelevance; required execution-evidence contexts fail closed.
The render test's shared invariant is split along that line.
---
 .../classify-infra-failure-render.test.cjs    |  96 ++++---
 ...laude-security-review-fail-closed.test.cjs | 270 ++++++++++++++++++
 .github/workflows/claude-security-review.yml  | 166 +++++++++--
 3 files changed, 471 insertions(+), 61 deletions(-)
 create mode 100644 .github/scripts/claude-security-review-fail-closed.test.cjs

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..7fe11d3
--- /dev/null
+++ b/.github/scripts/claude-security-review-fail-closed.test.cjs
@@ -0,0 +1,270 @@
+"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 }) {
+  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,
+      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");
+});
+
+// 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.
+  assert.match(
+    jobCondition,
+    /github\.event\.pull_request\.head\.repo\.full_name == github\.repository/u,
+    "the job must skip fork PRs; without this the fail-closed mapping pins every fork PR red",
+  );
+  // Without this disjunct the empty head.repo on a non-PR event reads as a fork
+  // and would skip every workflow_dispatch / schedule invocation.
+  assert.match(
+    jobCondition,
+    /github\.event\.pull_request\.number == ''/u,
+    "the fork guard must not skip non-pull_request events",
+  );
+  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",
+  );
+});
diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml
index d0079dd..4283af3 100644
--- a/.github/workflows/claude-security-review.yml
+++ b/.github/workflows/claude-security-review.yml
@@ -6,17 +6,36 @@ 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.
+#
+#   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
@@ -75,7 +94,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).
@@ -293,10 +318,20 @@ 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. The `number == ''` disjunct keeps
+    # non-PR events (workflow_dispatch / schedule) unaffected — there
+    # `head.repo.full_name` is empty, which would otherwise read as "a fork" and
+    # skip every dispatch invocation.
     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.pull_request.number == ''
+              || github.event.pull_request.head.repo.full_name == github.repository) }}
     permissions:
       contents: read
       pull-requests: write
@@ -391,8 +426,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 +462,82 @@ jobs:
             commit blob (not the PR) using a permalink with line numbers:
             https://github.com/${{ github.repository }}/blob/${{ github.event.pull_request.head.sha }}/<path>#L<line>
 
+      # 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.
+      - name: Back off before the review retry
+        if: >-
+          steps.freshness.outputs.superseded != 'true' &&
+          steps.claude-review.outcome == 'failure'
+        run: sleep 60
+
+      - name: Claude security review (retry)
+        id: claude-review-retry
+        if: >-
+          steps.freshness.outputs.superseded != 'true' &&
+          steps.claude-review.outcome == 'failure'
+        # 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 }}/<path>#L<line>
+
+      # 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 +546,8 @@ 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 }}
         run: |
           if [ "$REVIEW_OUTCOME" = "success" ]; then
             echo "Claude security review completed. Findings (if any) are in the PR comments."
@@ -557,6 +675,20 @@ jobs:
 
           echo "review_failed=true" >> "$GITHUB_OUTPUT"
 
+          # 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 +730,9 @@ 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 none did, so it cannot report success. Where this check is required, merging is blocked until a review actually completes.",
+              ">",
+              "> Re-running the job, or pushing a new commit, will retry the review — one automatic retry already ran. If the failure class is `auth`, or `rate-limit` persists across re-runs, the credential or usage budget needs an operator, and re-running will not help.",
             ].join("\n");
 
             const comments = await github.paginate(github.rest.issues.listComments, {

From 4df8002c35a8d7fb8b7f71f533ae9874e895b323 Mon Sep 17 00:00:00 2001
From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com>
Date: Sun, 26 Jul 2026 18:41:16 -0400
Subject: [PATCH 2/5] fix(claude-security-review): keep non-pull_request runs
 on the pass-through path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Independent review caught that the fail-closed mapping reddened events the
action cannot serve at all. `merge_group` is in neither ENTITY_EVENT_NAMES
nor AUTOMATION_EVENT_NAMES at the pinned SHA, so parseGitHubContext throws
`Unsupported event type`; `track_progress` is hardcoded on and its validator
rejects every non-PR event. Both fail for a cause no head change can fix and
no retry can clear.

Left alone, a consumer following this workflow's own CONSUMER CONTRACT — which
tells them to add `merge_group:` — would have wedged its merge queue on a
permanently red required check. The comment steps are PR-gated, so that red
would have carried no explanation either.

Only a pull_request run gates a merge, so only a pull_request run fails
closed. Everything else keeps the historical pass-through: still annotated,
still classified, not fatal.

Also from review:

- Cover the resolve step with executable tests. It was the widest blast
  radius in the workflow and had none: a mutation making it never select the
  first attempt reddens every CLEAN review, and the suite stayed green.
- Tell the truth about remedies in the PR comment. Re-running does not clear
  an exhausted turn budget; that needs a smaller diff or a higher --max-turns.
- Raise the job budget to 45m so a slow first attempt cannot starve the retry
  into a timeout-shaped red check.
---
 ...laude-security-review-fail-closed.test.cjs | 109 +++++++++++++++++-
 .github/workflows/claude-security-review.yml  |  22 +++-
 2 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/.github/scripts/claude-security-review-fail-closed.test.cjs b/.github/scripts/claude-security-review-fail-closed.test.cjs
index 7fe11d3..661b9ee 100644
--- a/.github/scripts/claude-security-review-fail-closed.test.cjs
+++ b/.github/scripts/claude-security-review-fail-closed.test.cjs
@@ -67,7 +67,11 @@ function rateLimitedExecutionFile() {
   ]);
 }
 
-function reportOutcome({ outcome, executionFileContents }) {
+function reportOutcome({
+  outcome,
+  executionFileContents,
+  isPullRequest = "true",
+}) {
   const directory = fs.mkdtempSync(
     path.join(os.tmpdir(), "security-review-outcome-"),
   );
@@ -77,6 +81,7 @@ function reportOutcome({ outcome, executionFileContents }) {
     const environment = {
       ...process.env,
       REVIEW_OUTCOME: outcome,
+      IS_PULL_REQUEST: isPullRequest,
       GITHUB_OUTPUT: githubOutput,
     };
     if (executionFileContents !== undefined) {
@@ -153,6 +158,108 @@ test("an unreadable execution file still fails closed", () => {
   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".
diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml
index 4283af3..f6de6cc 100644
--- a/.github/workflows/claude-security-review.yml
+++ b/.github/workflows/claude-security-review.yml
@@ -305,7 +305,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.
@@ -548,6 +550,7 @@ jobs:
         env:
           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."
@@ -675,6 +678,19 @@ 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.
+          if [ "$IS_PULL_REQUEST" != "true" ]; 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
@@ -732,7 +748,9 @@ jobs:
               ">",
               "> **The check is red on purpose.** It certifies that a security pass ran, and none did, so it cannot report success. Where this check is required, merging is blocked until a review actually completes.",
               ">",
-              "> Re-running the job, or pushing a new commit, will retry the review — one automatic retry already ran. If the failure class is `auth`, or `rate-limit` persists across re-runs, the credential or usage budget needs an operator, and re-running will not help.",
+              "> 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. `auth`, or `rate-limit` that persists across re-runs, needs an operator (credential or usage budget). A run that exhausted its turn budget on an oversized diff will exhaust it again — split the PR or raise `claude-args: --max-turns`.",
             ].join("\n");
 
             const comments = await github.paginate(github.rest.issues.listComments, {

From e7ef20b710417bf78fd76c2a750611b78d415997 Mon Sep 17 00:00:00 2001
From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com>
Date: Sun, 26 Jul 2026 18:44:42 -0400
Subject: [PATCH 3/5] fix(claude-security-review): scope the fork guard so
 privileged triggers still hit the tripwire
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Codex review, P1. The fork exemption was written as a bare origin test, which
also matches a fork PR arriving via pull_request_target or workflow_run — the
two privileged triggers this workflow exists to reject. Those runs were skipped
at job level before `Reject privileged triggers` could hard-fail them, so a
consumer's dangerous misconfiguration would have surfaced as a green skipped
required check: the tripwire silently disarmed by the very guard added to keep
fork PRs from being pinned red.

Scoping the exemption to `github.event_name == 'pull_request'` sends every other
event to the tripwire first, and the events it allows through then take the
non-PR pass-through path in the outcome step.

The test now asserts both halves as one clause, so the fork test cannot be
reintroduced without its scope.
---
 ...claude-security-review-fail-closed.test.cjs | 18 ++++++++----------
 .github/workflows/claude-security-review.yml   | 16 +++++++++++-----
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/.github/scripts/claude-security-review-fail-closed.test.cjs b/.github/scripts/claude-security-review-fail-closed.test.cjs
index 661b9ee..28011bc 100644
--- a/.github/scripts/claude-security-review-fail-closed.test.cjs
+++ b/.github/scripts/claude-security-review-fail-closed.test.cjs
@@ -269,18 +269,16 @@ test("fork PRs skip the job instead of failing closed forever", () => {
     workflow.indexOf("      - name: Reject privileged triggers"),
   );
 
-  // A fork run gets no secrets, so no number of retries can make it pass.
+  // 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\.pull_request\.head\.repo\.full_name == github\.repository/u,
-    "the job must skip fork PRs; without this the fail-closed mapping pins every fork PR red",
-  );
-  // Without this disjunct the empty head.repo on a non-PR event reads as a fork
-  // and would skip every workflow_dispatch / schedule invocation.
-  assert.match(
-    jobCondition,
-    /github\.event\.pull_request\.number == ''/u,
-    "the fork guard must not skip non-pull_request events",
+    /\(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,
diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml
index f6de6cc..e219fc6 100644
--- a/.github/workflows/claude-security-review.yml
+++ b/.github/workflows/claude-security-review.yml
@@ -324,15 +324,21 @@ jobs:
     # 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. The `number == ''` disjunct keeps
-    # non-PR events (workflow_dispatch / schedule) unaffected — there
-    # `head.repo.full_name` is empty, which would otherwise read as "a fork" and
-    # skip every dispatch invocation.
+    # 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))
-          && (github.event.pull_request.number == ''
+          && (github.event_name != 'pull_request'
               || github.event.pull_request.head.repo.full_name == github.repository) }}
     permissions:
       contents: read

From 0c086dd0c3d914a177883bb2ce16db978f1fa521 Mon Sep 17 00:00:00 2001
From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com>
Date: Sun, 26 Jul 2026 18:56:18 -0400
Subject: [PATCH 4/5] fix(claude-security-review): make an absent
 IS_PULL_REQUEST fail closed, not open
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Round-2 independent review found that the non-PR pass-through added in 4df8002
introduced a fail-OPEN blind spot — the one direction this suite exists to
prevent. Two mutations survived the whole suite: deleting the
`IS_PULL_REQUEST:` env line, and rewiring it to a wrong expression. Either
leaves the variable unset in CI, where `[ "" != "true" ]` is true, so every
event takes the pass-through branch and #266 silently reverts.

The test harness supplied the variable itself, so nothing pinned the YAML
wiring even though the file already used that pattern for its siblings.

Fixed in both directions:

- The guard now tests for the PASS-THROUGH value (`= "false"`) rather than the
  fail-closed one, so an unset or garbled value keeps the run failing closed.
- The wiring is asserted against the workflow source, and a test covers the
  unset case end to end.

Also from round 2:

- Gate the backoff and retry on being a pull_request. On events the action
  rejects outright the first attempt always fails, so retrying only spent a
  runner and a minute of wall clock to throw again.
- Correct two contract claims that this change had made untrue. POSTURE said
  required execution-evidence contexts fail closed without qualifying that fork
  and non-PR runs cannot, and the CONSUMER CONTRACT told consumers to add
  `merge_group:` without noting the action cannot serve that event — so a queued
  run reports green without reviewing, and gating a merge queue on this check is
  not equivalent to gating a PR on it.
- The failure comment said "none did" where a max-turns run may have posted a
  partial review, and offered `--max-turns` to PR authors who cannot set it.
---
 ...laude-security-review-fail-closed.test.cjs | 24 ++++++++++++
 .github/workflows/claude-security-review.yml  | 37 ++++++++++++++++---
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/.github/scripts/claude-security-review-fail-closed.test.cjs b/.github/scripts/claude-security-review-fail-closed.test.cjs
index 28011bc..814035d 100644
--- a/.github/scripts/claude-security-review-fail-closed.test.cjs
+++ b/.github/scripts/claude-security-review-fail-closed.test.cjs
@@ -372,4 +372,28 @@ test("the outcome step reads the resolved attempt, not just the first one", () =
     /^ {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 e219fc6..4d31b31 100644
--- a/.github/workflows/claude-security-review.yml
+++ b/.github/workflows/claude-security-review.yml
@@ -23,6 +23,12 @@ name: claude-security-review
 #   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
@@ -62,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:
@@ -481,17 +494,22 @@ jobs:
       # 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'
+          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'
+          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
@@ -692,7 +710,12 @@ jobs:
           # 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.
-          if [ "$IS_PULL_REQUEST" != "true" ]; then
+          # 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
@@ -752,11 +775,13 @@ jobs:
               `> - Failure class: \`${process.env.REVIEW_CLASS}\``,
               `> - Last SDK result: \`${process.env.REVIEW_DETAIL}\``,
               ">",
-              "> **The check is red on purpose.** It certifies that a security pass ran, and none did, so it cannot report success. Where this check is required, merging is blocked until a review actually completes.",
+              "> **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. `auth`, or `rate-limit` that persists across re-runs, needs an operator (credential or usage budget). A run that exhausted its turn budget on an oversized diff will exhaust it again — split the PR or raise `claude-args: --max-turns`.",
+              "> 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, {

From 887a77f3862a3b9ddefc27bfd0b3a671b37b29ed Mon Sep 17 00:00:00 2001
From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com>
Date: Sun, 26 Jul 2026 19:04:05 -0400
Subject: [PATCH 5/5] docs(claude-security-review): record the Dependabot
 secret contract on skip-actors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Codex review, P2. "Empty reviews all" was true before this lane failed closed
and is now a trap: a Dependabot-triggered run reads only from the Dependabot
secrets store, never from Actions or organization secrets, so a consumer who
clears the default to review Dependabot PRs gets a review that cannot
authenticate — and a required check that blocks every one of them until an
operator provisions the mirrored secret.

Documenting rather than auto-skipping: the default already excludes
dependabot[bot], and a consumer who deliberately opts in may well have
mirrored the secret. Silently overriding that choice would deny them the
review; passing through on a missing secret would reopen the evidence gap
this PR closes.
---
 .github/workflows/claude-security-review.yml | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml
index 4d31b31..407ef57 100644
--- a/.github/workflows/claude-security-review.yml
+++ b/.github/workflows/claude-security-review.yml
@@ -183,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: