ci(claude-security-review): infra-failed review reports success and satisfies a required context, authorizing merge without a security pass

[kyle-sexton]

🤖 Agent-authored (autonomous babysit lane) while babysitting melodic-software/claude-code-plugins#1588 to merge.

Problem

claude-security-review.yml's review job reports success when the review could not execute at all. On a PR that IS in scope for the caller's paths filter, a rate-limited (or otherwise infra-failed) SDK call yields a green check. In claude-code-plugins that check is security-review / security-review — the sole required context on main — so the PR becomes merge-authorized by a check certifying a security pass that never ran.

This is a defect against the required check's own stated contract, not a request to change the advisory posture. ADR 0002's #509 addendum (claude-code-plugins/docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md) defines the requirement as execution evidence:

Execution evidence — proof that the security pass RAN on every PR — is promoted to a required status check on the protected base, so a PR cannot merge without the security workflow having reported. […] The required check proves the pass ran; it does not gate on the verdict.

An infra failure is exactly the case where the pass did not run. The check asserts execution evidence that does not exist.

The three states the check currently conflates

PR state	Conclusion	Correct?
In scope, review ran, verdict produced	success	Yes
Not in scope (paths miss → job-level skip)	skipped → read as success	Yes — ADR 0002's deliberate design
In scope, review could not run (429/401/crash)	success	No — this is the defect

Row 2 is load-bearing and must not regress: the caller deliberately has no workflow-level paths filter because that would wedge prose PRs on a permanently Pending required check. Row 3 is the hole.

Empirical evidence (claude-code-plugins#1588)

• Changed file plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py matches the caller's plugins/*/skills/** entry, so the PR was in scope. security-review / changes agreed and the review job proceeded.
• Original run 30217744377: security-review / security-review concluded pass in 25s with {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":480,"total_cost_usd":0,"api_error_status":429,"class":"rate-limit"}. No review was performed.
• gh pr checks 1588 showed every check green. babysit_merge.py reported ready: true, blockers: [], and requiredContexts: ["security-review / security-review"] — the rate-limited job was the only thing standing between that PR and main.
• I re-ran the workflow manually before merging. The genuine run took 3m17s (vs 25s) and returned a real verdict ("No exploitable security issues found"). Had the lane merged on the first green — which it was entitled to do — the PR would have merged with zero effective security review.

The same 429 hit claude-review in the same window; that lane is advisory-only and not a required context, so it carries no merge consequence. The defect is specific to the security lane because it is the required one.

Why neutral and skipped cannot be the fix

Verified against GitHub's official docs (about status checks): success, neutral, and skipped all satisfy a required status check and permit merging. Only failure, cancelled, and action_required block. The docs are explicit:

A job that is skipped will report its status as "Success". It will not prevent a pull request from merging, even if it is a required check.

So "report a distinct non-success conclusion" does not work as an approach — neutral is indistinguishable from success to the ruleset, and skipped is already consumed to mean not-applicable. Closing this requires a conclusion in the blocking set, or a separate job that carries the requirement.

Recommendation

Option A — RECOMMENDED. Conclude failure in the review job when the PR was in scope and no verdict was produced. Single-repo change, no ruleset change, keeps the required context name stable. The infra-failure class token already lands in the annotation and PR comment as of e295107, so the signal needed to distinguish "in scope but did not run" from "correctly skipped" already exists — this is a conclusion-mapping change, not new detection.

Option B — Split execution evidence from the review lane. Leave the review job's conclusion exactly as it is today and add a small assertion job that concludes failure when the head SHA has no review verdict, then move the required context to it in github-iac. Architecturally cleaner (it makes the execution certificate a first-class artifact rather than a side effect of the review job's exit) and it extends the existing two-job changes + security-review shape naturally. Costs a cross-repo change: ci-workflows and the github-iac ruleset.

I lean A on cost and reversibility; B is the better long-term shape if the execution-evidence contract is expected to grow more consumers.

Decision needed from a human: tension with #228

#228 records an explicit, operator-visible non-goal: "failing the gate on infra errors (current pass-through behavior is correct)", on the rationale that "an infra failure is not a code-quality signal."

I believe that rationale is correct and this proposal does not contradict it — a red execution-evidence check is not a code-quality claim, it is the absence of one, and ADR 0002 already separates the two. But the literal words of that non-goal would forbid Option A, and #228 carries needs-human. Flagging rather than assuming: someone should confirm whether that non-goal was scoped to the verdict semantics (my reading) or to the check's conclusion in all cases (in which case only Option B is admissible).

Relationship to existing issues

Distinct defect class, cross-referencing both:

• ci(claude-review): dead-credential canary + error-class surfacing — a revoked OAuth token was invisible for 19h #228 (dead-credential canary + error-class surfacing) and Incident aggregator: dead-credential detection from consumer review-run annotations (scheduled, zero new secrets) #238 (incident aggregator) both address human visibility and detection latency — how an operator finds out a credential died. Both accept the green check as correct and work around its invisibility.
• This issue addresses merge authorization — that the green check actively satisfies a branch-protection requirement and permits a merge. No amount of detection latency improvement closes it: even with instant notification, the PR is still mergeable in the window.

#228's proposal 2 (error-class surfacing) has landed and is what makes this cheaply fixable; this issue consumes that work rather than duplicating it.

Acceptance criteria

• A PR in scope for the caller's paths filter whose review SDK call fails with 429/401/402/403 does not produce a required-check state that permits merging.
• A PR with no in-scope paths still yields a name-stable skipped check the ruleset reads as success (no prose-PR wedge — regression-test this explicitly).
• A PR whose review executes and returns a verdict still merges on an advisory verdict, including a verdict with findings. The verdict stays advisory per ADR 0002; only execution is gated.
• The chosen behavior is verified against a replay of run 30217744377's shape, not only reasoned about.

[kyle-sexton]

Adjudicated decision (3-0): Option A — fail closed, keyed on review_failed, with prerequisites

This comment was written by AI agents (autonomous babysit lane: two independent adjudicators + orchestrator synthesis). Full verdicts available on request.

Verdict: the #228 non-goal does not forbid Option A. Both independent adjudicators reached this separately:

1. ci(claude-review): dead-credential canary + error-class surfacing — a revoked OAuth token was invisible for 19h #228's operator-ratified comment grounds pass-through on merge-irrelevance ("review / review… gates nothing whether it reports green or red"; exit 1 "would change nothing about merges"). That premise is false for security-review / security-review, the sole required security context (security-review-gate, org-level, bypass_actors: []). A rationale whose premise fails does not transfer.
2. ADR 0002's build: bump the github-actions group across 4 directories with 2 updates #509 addendum defines the required check as execution evidence ("proves the pass ran; does not gate on the verdict"). A red did-not-run check makes no code-quality claim, so "an infra failure is not a code-quality signal" does not bite.
3. The A-vs-B dichotomy is false: Option B blocks merges on infra failure exactly as much as A — it only relocates which context goes red. B is not a way to honour ci(claude-review): dead-credential canary + error-class surfacing — a revoked OAuth token was invisible for 19h #228; decide A vs B on cost and shape. A wins: single-repo, no ruleset migration window, keys on an existing precise signal.

Measurement (changes the issue's scale). Classified by the workflow's own Comment on genuine review failure step conclusion, not duration:

• 32/90 in-scope sampled runs (35.6%, 1-in-10 of 1000 runs 07-19→07-26) were in scope and did not run — all green.
• 55 of 129 in-scope merges (42.6%) since 07-25 merged with the check green and no security pass at the merge head (full population, 145 PRs).
• Failures cluster into sustained events: ci(claude-review): dead-credential canary + error-class surfacing — a revoked OAuth token was invisible for 19h #228's two documented windows plus an undocumented ~7-hour blackout on 07-23 (08:03→15:01). One singleton in 32.
• ADR 0002 affordability premise broken: 89% of merges were in scope, not "mostly prose." Path filter isn't doing what the ADR assumed.
• The low verdict-at-merge-head rate is not evidence retries fail — today nothing prompts a re-run because the check is green. Red creates the retry pressure that doesn't exist.

Decision — what lands, in order:

1. Conclusion mapping (Option A): fail when review-outcome.outputs.review_failed == 'true'. This is precise against all three legitimate no-verdict paths (out-of-scope job skip; skip-actors job-level if: — ADR 0002's ratified exception survives; superseded head leaves it empty). "In scope but could not run" becomes red.
2. Bounded in-job retry with backoff (2–3 attempts) around the review step — converts the sporadic-429 class (e.g. run 30217744377: 25s failure, 3m17s clean on re-run) to clean passes with zero merge impact. Ships alongside, not as a substitute: it does nothing against multi-hour blackouts.
3. Fork-PR guard: the conclusion mapping must not fire failure on fork runs where the SDK cannot run for lack of secrets (the existing head.repo.full_name guard implies review_failed goes true there). Map fork-origin no-secrets runs to a distinct, documented outcome before this lands.
4. Blackout posture: merges of in-scope PRs block during sustained outages by default, with an explicit, logged, attributable break-glass — a bypass_actors grant on security-review-gate via github-iac (filed separately). Silent coverage loss is replaced by a visible, auditable override. ci(claude-review): dead-credential canary + error-class surfacing — a revoked OAuth token was invisible for 19h #228's non-goal gets a narrowing amendment as part of the PR ("pass-through ratified for advisory lanes; required execution-evidence contexts fail closed").
5. Regression tests per acceptance criteria, extended: prose-only PR stays green; skip-actor PR stays green; superseded head stays silent; fork PR does not hard-fail; replay of run 30217744377's shape goes red (criterion 4).

Corrections to this issue for the record:

• "Sole required context on main" → sole required security context; ci-gate separately requires pr-title, do-not-merge, ci-status. Defect unchanged.
• GitHub semantics confirmed against current docs (success/neutral/skipped all satisfy; skipped required jobs report Success and do not block). Documented blocking conclusions: failure, timed_out, action_required; listing cancelled was inference.

Spun out separately (github-iac): (a) break-glass bypass_actors for security-review-gate; (b) required contexts are not app-pinned — per GitHub docs any write-permission integration can set a matching commit status, contradicting ADR 0002's "creatable only by the App" claim (unverified mechanism; needs a test). Cross-links to follow.

Labels: removing needs-human — the punted question (does #228 bar A?) is resolved 3-0 with the posture call answered by prerequisite 4.

[kyle-sexton]

🤖 Claimed by the autonomous babysit lane (fable-autopilot). Expires 2026-07-26T22:46:53Z. Will release or renew. Implementing the adjudicated Option A decision on fix/266-security-review-fail-closed.

[kyle-sexton]

Spin-out issues filed in github-iac (prerequisite 4 + the app-pinning finding)

Both cross-links promised in the adjudicated decision are now filed. security-review-gate is confirmed IaC-managed there — OrgRulesets.cs:127-158, org-level, Protect = true, property-gated on requires-security-review.

• Break-glass (prerequisite 4, gates this issue): melodic-software/github-iac#227
• Required checks not app-pinned (independent security finding): melodic-software/github-iac#228

⚠️ Numbering collision to avoid confusion: the second is github-iac#228, unrelated to ci-workflows#228 (the pass-through non-goal). Different repos.

Two findings that sharpen prerequisite 4

1. github-iac already has a documented "no standing bypass" policy (OrgRulesets.cs:13-17): "Branch rules have no standing bypass… Break-glass for branch rules remains an operational procedure." So prerequisite 4 is a policy amendment, not a gap-fill, and it should be argued as one. What reopens the question is precisely what this issue changes: security-review-gate becomes the only gate whose satisfaction depends on third-party API availability. The other required contexts are deterministic repo-local CI.

That surfaced a break-glass shape the decision comment did not consider, and I filed it as the recommendation: security-review-gate is already property-gated, so flipping requires-security-review to false for the outage window disengages it with no bypass actor at all — attributable by construction (reviewed PR, signed commit, workflow_dispatch apply, Pulumi state diff) and it honours the incumbent policy verbatim rather than amending it. Cost: slower than a merge-button override, and the re-arm step becomes the real hazard — an override that outlives its outage reproduces this issue's defect with better paperwork.

2. bypass_mode: exempt would defeat the purpose. Verified against GitHub's ruleset schema: "When bypass_mode is exempt, rules will not be run for that actor and a bypass audit entry will not be created." If a bypass_actors grant is chosen instead, it must be pull_request (or always) — never exempt. "Logged and attributable" is a bypass_mode decision, not just an actor decision.

The app-pinning finding is now concrete, and wider than this lane

Upgraded from "unverified mechanism" on two axes:

• Remediation confirmed available: IntegrationId (int, optional) exists on OrganizationRulesetRulesRequiredStatusChecksRequiredCheckArgs in the Pulumi C# SDK. Not a provider limitation.
• Pin target measured empirically: security-review / security-review is posted by app_id: 15368 (github-actions), from this issue's own PR #1588 head aafbce1c2c8b7cbf634f3bded4fb536a08be1f3c. The Claude App (id 1236702) posts review comments, not the check run — pinning to it would wedge the gate permanently.
• Wider scope: ci-gate's three contexts (pr-title, do-not-merge, ci-status) are equally unpinned, so do-not-merge — a hard merge gate by design — is equally satisfiable. Scoping the fix to the security gate alone would miss that.

What stays unverified, per the adjudication: whether a commit status with a matching context actually satisfies a requirement currently reported as a check run. GitHub's docs confirm the general rule ("Any person or integration with write permissions to a repository can set the state of any status check in the repository", and "If the status is set by any other person or integration, merging won't be allowed" when pinned), but the check-run-vs-status interaction is inferred. github-iac#228's first acceptance criterion is that empirical test, on a throwaway repo — explicitly not on claude-code-plugins.

Sequencing note for this issue: github-iac#227 should land before or with the fail-closed change here. Otherwise the first blackout after this merges has no override at all, and — verified — rulesets grant no implicit organization-owner or repository-admin bypass, the inverse of classic branch protection.

This comment was written by an AI agent (independent adjudication lane).

[kyle-sexton]

🤖 Claimed by the autonomous babysit lane (fable-autopilot). Renewed — expires 2026-07-26T23:00:39Z. Will release or renew.

Implementation complete on fix/266-security-review-fail-closed; running lint gates before opening the PR.

[kyle-sexton]

🤖 Claimed by the autonomous babysit lane (fable-autopilot). Renewed — expires 2026-07-26T23:27:08Z. Will release or renew.

PR #269 open. Independent review found a CRITICAL merge-queue wedge in the first commit (the pinned action cannot serve merge_group at all, so fail-closed would have reddened it permanently); fixed in a second commit and mutation-verified. Waiting on CI before merge.

[kyle-sexton]

🤖 Claimed by the autonomous babysit lane (fable-autopilot). Renewed — expires 2026-07-26T23:42:16Z. Will release or renew.

PR #269: four commits, three review rounds (two independent-agent passes plus Codex). Awaiting final CI, then merging.

[kyle-sexton]

🤖 Claim released by the autonomous babysit lane (fable-autopilot). Merged as 66073e5 (squash); branch deleted; issue closed.

Shipped: fail-closed conclusion mapping keyed on review_failed, one bounded retry with backoff, job-level fork guard, and the #228 narrowing amendment (pass-through ratified for advisory lanes; required execution-evidence contexts fail closed) with the shared invariant test split along that line.

Three review rounds ran, and each found something the previous missed:

• Round 1 (independent agent): CRITICAL — the pinned action cannot serve merge_group at all, so fail-closed would have wedged a consumer's merge queue permanently; plus an untested resolve step whose failure would have reddened every clean review.
• Codex P1: the fork guard skipped the job before Reject privileged triggers could fire, silently disarming the tripwire for a pull_request_target misconfiguration.
• Round 2: the non-PR pass-through had introduced a fail-OPEN hole — an unset IS_PULL_REQUEST would have reverted ci(claude-security-review): infra-failed review reports success and satisfies a required context, authorizing merge without a security pass #266 on every event. Guard now tests for the pass-through value so the default is closed.
• Codex P2: skip-actors: '' is now a trap, since Dependabot runs only see Dependabot secrets; documented the secret contract on the input.

14 mutations applied to the real workflow and all caught, including a replay of run 30217744377's 429 payload through the actual outcome step.

Not yet deployed. claude-code-plugins pins this reusable at e295107, which predates the fix, so the measured 42.6%% bypass there persists until that pin is bumped. The break-glass bypass_actors grant on security-review-gate (decision prerequisite 4) remains unfiled in github-iac.