fix(claude-security-review): fail the required check when an in-scope review could not run - #269
Conversation
… 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.
|
Claude finished @kyle-sexton's task in 3m 42s —— View job PR Review — fix(claude-security-review): fail the required check when an in-scope review could not run
SummaryThis PR correctly implements the fail-closed decision from #266. The implementation is sound, the documentation is thorough, and the test suite is unusually rigorous (executing the real shell script rather than grepping for CorrectnessThe
Both The three no-verdict paths are all guarded at job level, so they cannot reach the failing step:
The Retry resolution is correct. The One structural observation worth naming
Fork guard — the flagged consequence is stated correctlyThe PR body's treatment of this is correct and the One precision: the test checks for the presence of each clause as a pattern match against a substring of the workflow source, not against the parsed The distinction from ADR 0002's skip-actors exception is accurately stated. The compensating control (human review of fork changes to security-sensitive surfaces) is the right one. Test qualityThe The log-hygiene test (lines 130–139) checks both stdout and every step output value against the canary string. That covers the two surfaces where the free-text The CLAUDE.md alignment
VerdictApproved. The implementation is correct, the test coverage is comprehensive, and all the design decisions are well-reasoned and documented. The fork guard consequence is accurately characterized and transparently flagged rather than buried. No changes required. |
|
Claude finished @kyle-sexton's task in 3m 58s —— View job Security Review — PR #269
Reviewed: Trust boundary analysisJob condition / fork guard. The new ${{ !cancelled()
&& needs.changes.outputs.relevant != 'false'
&& !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) }}Both Privileged-trigger tripwire. Unchanged; still hard-fails Secret handling. Output sanitization / log hygieneThe jq projection at lines 620–652 explicitly excludes The The Retry mechanismThe One edge case: if the first attempt's outcome is Fail-closed correctnessThe fork-skip property is the acknowledged gap: fork PRs satisfy the required check via a skipped job (name-stable, ruleset reads as success) with no review having run. This is the same trust model as the skip-actors operator exception and the out-of-scope skip. The PR documents it explicitly and calls out that it has not been formally ratified to the same standard as ADR 0002's skip-actors addendum. From a threat-model perspective: a fork PR author cannot affect the job condition value at No security vulnerabilities foundNo injection surfaces, no secret-exfiltration paths, no authorization bypasses, no supply-chain regressions, and no log-hygiene regressions were identified in the changed files. The fail-closed mapping, fork guard, retry, and output sanitization all behave as described. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91f3030a3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hrough path 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.
PR Review — fix(claude-security-review): fail the required check when an in-scope review could not run
Work in progress… |
Security Review — PR #269
|
…ers still hit the tripwire 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 finished @kyle-sexton's task in 1m 47s —— View job PR Review — fix(claude-security-review): fail the required check when an in-scope review could not run
This is the third review pass on this PR (the first two didn't complete due to infrastructure failure). I reviewed the current HEAD Codex P1 Fix (
|
|
Claude finished @kyle-sexton's task in 4m 46s —— View job Security Review — PR #269
Reviewed: Fork guard / tripwire interaction (e7ef20b — the Codex P1 fix)The fix is correct and complete. Working through every relevant event path against the final job condition: && (github.event_name != 'pull_request'
|| github.event.pull_request.head.repo.full_name == github.repository)
The prior form ( IS_PULL_REQUEST determinationIS_PULL_REQUEST: ${{ github.event.pull_request.number != '' }}On Injection surfaces in run: blocksAll
No shell injection surface in the changed code. Secret handling in the retry pathThe retry step is a verbatim copy of the first attempt (confirmed by the test at lines 307–361 which asserts Output sanitization / log hygieneThe jq projection at lines 629–661 projects exactly: The Marker-comment lookupBoth the failure comment and stale-comment-clear steps locate the marker by filtering No security vulnerabilities foundThe fork guard fix (e7ef20b) correctly routes privileged triggers to the tripwire while keeping fork |
…ed, not open 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.
|
Claude finished @kyle-sexton's task in 2m 14s —— View job PR Review — fix(claude-security-review): fail the required check when an in-scope review could not run
This pass covers only
|
| Variable state | != "true" (old) |
= "false" (new) |
|---|---|---|
"true" (PR event) |
false → falls through → exit 1 ✓ |
false → falls through → exit 1 ✓ |
"false" (non-PR event) |
true → exit 0 ✓ |
true → exit 0 ✓ |
"" (env line deleted) |
true → exit 0 (fail-open ✗) |
false → falls through → exit 1 ✓ |
| any other value | true → exit 0 (fail-open) |
false → falls through → exit 1 ✓ |
The new form is fail-closed by default for any unset or unexpected value. A deleted or misconfigured IS_PULL_REQUEST env line would have silently reverted #266 on every event under the old form; under the new form it still fails closed.
Retry gate
if: >-
steps.freshness.outputs.superseded != 'true' &&
steps.claude-review.outcome == 'failure' &&
github.event.pull_request.number != ''Correct. On merge_group / workflow_dispatch / schedule, the pinned action throws on event type — the first attempt always fails for a reason no head change can fix. Adding the PR-number gate prevents burning a runner and 60 wall-clock seconds on a second guaranteed failure. The two retry conditions (backoff and retry step itself) are kept symmetrical, as they must be.
Tests
Both new assertions are load-bearing:
Structural pin — claude-security-review-fail-closed.test.cjs:379–383: the regex requires the exact IS_PULL_REQUEST: ${{ github.event.pull_request.number != '' }} line with correct indentation. If the env line is removed or rewritten, this test fails before the behavioral test even matters. The 10-space indent requirement is semantic for YAML, so the specificity is appropriate.
Behavioral guarantee — lines 388–399: exercises the real shell script with isPullRequest: "" and asserts exit status 1. This is the test that would have caught the original != "true" bug — and now directly pins the = "false" fix's behavioral consequence. Two layers (structural + behavioral) for the same invariant is the right call here given the silent-revert risk.
One minor observation: the retry gate condition (github.event.pull_request.number != '' on both retry steps) is not pinned with a structural test the way IS_PULL_REQUEST is. If those conditions were dropped, the retry would fire unnecessarily on non-PR events — wasted effort but not a security regression, since the outcome step still gates the final conclusion. The asymmetry in test depth matches the asymmetry in consequence, so this is defensible as-is.
Documentation
The POSTURE header addition (lines 25–31) and the CONSUMER CONTRACT merge_group caveat (lines 74–81) are accurate. In particular, calling out that merge_group triggers report green without reviewing is useful consumer guidance; the evidence for the PR-level gate is still valid because the PR's own pull_request run carries it.
The PR comment copy change — splitting the Re-running does NOT help block into bullet points and distinguishing operator-remediation from author-remediation for the error_max_turns case — is clearer for consumers than the prior single paragraph.
Verdict
Approved. 0c086dd is a targeted, well-reasoned correctness fix: the guard direction change is exactly right, both new tests pin it from independent angles, the retry gate is a correct optimization, and the documentation is accurate. No issues.
|
Claude finished @kyle-sexton's task in 4m 35s —— View job Security Review — PR #269
Reviewed: Scope of this pass: logic flaws, authorization gaps, injection surfaces, secret handling, and dangerous Actions patterns that static analysis cannot reach. Deferring to zizmor's lane for template injection, unpinned actions, and dangerous trigger patterns — not re-reported here. Fork guard and privileged trigger interaction — confirmed correctWorking all relevant event paths against the final job-level condition: && (github.event_name != 'pull_request'
|| github.event.pull_request.head.repo.full_name == github.repository)
The Codex P1 (commit Injection surfaces in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c086dd0c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…n skip-actors 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.
|
Claude finished @kyle-sexton's task in 1m 50s —— View job PR Review — fix(claude-security-review): fail the required check when an in-scope review could not run
Commits
|
|
Claude finished @kyle-sexton's task in 2m 58s —— View job Security Review — PR #269
Reviewed: Scope: logic flaws, authorization and access-control gaps, injection surfaces, unsafe secret handling, and dangerous Actions patterns that static analysis misses. Deferring to zizmor for supply-chain / unpinned-action risk, dangerous trigger patterns, excessive permissions, and template injection in Fork guard / privileged-trigger interactionWorking all event paths against the final job-level condition at lines 360–365: && (github.event_name != 'pull_request'
|| github.event.pull_request.head.repo.full_name == github.repository)
The
|
… the carve-out (#273) 🤖 Agent-authored (autonomous babysit lane, fable-autopilot). Operator-ratified follow-up to #269. ## Summary #269 made execution evidence enforceable — but only on `pull_request` runs. This PR makes the reusable stop *recommending* the one trigger where that claim is false, and gives the gap a tracked owner. `merge_group` cannot be enforced: the pinned action throws `Unsupported event type` for it (`src/github/context.ts:251` at `1253134…`), so the run passes through and the check reports **green without reviewing**. Failing closed there would wedge any adopting consumer's merge queue on a permanently red check instead — so the pass-through is correct, and the honest move is to stop pointing consumers at it. Three changes, all documentation: 1. **CONSUMER CONTRACT no longer recommends `merge_group:`.** It previously told consumers to add it (with a caveat added in #269). On a merge queue — which *is* the merge gate — that yields a check that looks like execution evidence and is not: exactly the failure #266 was filed to eliminate. Without the trigger the check simply never reports for queued PRs, which is the safer default. 2. **POSTURE carve-out is explicit and cross-linked** to #272, so the exception has a durable owner instead of living only in a workflow comment. 3. **The dogfood caller** (`claude-security-review-self.yml`) said "add merge_group only when the repo actually runs a merge queue" — now says not to, for the same reason. Behaviour is unchanged; this only corrects guidance that #269 made untrue. ## Test plan No logic touched — comments and one caller comment only. - `node --test .github/scripts/*.test.cjs` — **287 passing, 0 failing** (unchanged). - `actionlint` on both modified workflows — clean. - Line endings verified LF on both files. ## Related **No linked issue** — deliberately. This PR *documents* the carve-out tracked by #272; it does not close it. #272 stays open until the underlying gap is fixed (upstream `merge_group` support, or a merge queue actually being adopted), so a closing keyword here would retire the tracker while the gap is still open — the opposite of the intent. Refs #272 (the tracked carve-out, with its revisit trigger). Follow-up to #269; adjudicated decision in #266. Trigger recorded in #272: any org repo adopting a merge queue on a branch protected by `security-review-gate`, or `claude-code-action` gaining `merge_group` support upstream. Latent today — no org repo runs a merge queue, and `claude-code-plugins` (the only consumer requiring this check) triggers on `pull_request` only. ### Known inconsistency, not fixed here (cross-repo) ADR 0002 in `melodic-software/claude-code-plugins` carries a revisit trigger that says the opposite: *"A merge queue is enabled on the base → the security workflow must add the `merge_group` trigger, or its required check is never reported for queued PRs."* That was correct before the check could fail closed; it is now wrong, because adding the trigger produces false evidence. It needs amending in that repo, which is out of scope for a ci-workflows PR — recorded on #272.
…3e5 (#280) ## Summary Adds the reviewed runner-input contract for `ci-workflows/.github/workflows/claude-security-review.yml@66073e5` — the fail-closed fix from melodic-software/ci-workflows#269 — to `approvedReusableWorkflowContracts`, cloned unchanged from the existing `e295107` entry. Auto-approval declines this bump on its own: between `e295107` and `66073e5` the `prompt` input's default text and the `skip-actors` description changed, which the structural differ treats as an input-surface change. The contract surface consumers are actually held to — input names, secrets, caller permissions, routing — is identical, so the entry is a byte-for-byte clone keyed to the new SHA. Consumers can then pin `66073e5` and pass the runner-policy gate; melodic-software/claude-code-plugins#1684 (deploying the fail-closed pin that closes the measured 42.6% exit-0-on-429 review bypass) is waiting on this. ## Test plan - `python -m json.tool` parses the file; Biome check clean (pre-commit hook run). - Entry is a clone of the already-reviewed `e295107` contract with only the SHA key changed — verified by diff. - Downstream proof: after sync lands in claude-code-plugins, the `Runner policy` check on claude-code-plugins#1684 (currently failing with `runner-target-contract: no reviewed runner-input contract`) goes green. ## Related - melodic-software/ci-workflows#269 — the fail-closed fix the new SHA carries - melodic-software/claude-code-plugins#1684 — the pin-bump PR this unblocks No linked issue. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…osed fix (#1684) Deploys the ci-workflows fail-closed fix to this repo's security-review lane. At the current pin (e295107) the reusable workflow exits 0 when the in-scope review cannot run (Anthropic 429 rate limit, SDK infra failure), so the required `security-review / security-review` check reports pass with no review performed — measured at 42.6% of in-scope merges since 07-25, including an ~7h blackout on 07-23. ci-workflows#269 (66073e5) fails the required check when an in-scope review could not run; this bumps the pin to deploy it. Net diff is the one pin line. (Branch history contains an accidental delete/restore pair; squash merge collapses it.) Deliberately pins 66073e5 rather than current ci-workflows main (a7c7145): the composite-lane refactor (PR-A2) merged hours ago and can ride a separate, independently revertable bump. ## Related - melodic-software/ci-workflows#269 — the fail-closed fix this deploys - #1327 — the repo-wide report of green check rows over failed SDK reviews (this closes the security-review half of the reporting gap; the claude-review lane pin is unchanged) No linked issue. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
🤖 Agent-authored (autonomous babysit lane, fable-autopilot) implementing the operator-adjudicated decision on #266.
Summary
Implements the adjudicated decision on #266 (3-0, Option A): the security lane's
required check now fails closed when a PR was in scope and the review could not
run at all.
The check exists to prove a security pass RAN — ADR 0002's #509 addendum: "proves
the pass ran; it does not gate on the verdict." It was reporting
successwhen theSDK call failed, certifying an execution that never happened.
success,neutraland
skippedall satisfy a required check, sofailureis the only conclusion thatcan express "in scope and did not run".
Scale, measured on
claude-code-pluginsbefore this change: 55 of 129 in-scopemerges (42.6%) since 2026-07-25 landed on
mainwith the check green and no securitypass at the merge head. Classified by the workflow's own
Comment on genuine review failurestep conclusion, not by a duration heuristic.What lands
review-outcome.outputs.review_failed == 'true'.That signal is precise against all three legitimate no-verdict paths, so none of
them can be caught by it:
dependabot[bot],melodic-standards-sync[bot]) also skip atjob level — ADR 0002's operator-ratified exception is untouched;
(run
30217744377: 25s failure, 3m17s clean verdict on manual re-run). Shipsalongside the mapping, explicitly not as a substitute for it — sustained
multi-hour blackouts dominate the measured failures and no in-job backoff survives
those.
pull_requestruns stay on the pass-through path —see the flagged consequences below.
the render test: pass-through stays ratified for advisory lanes, whose rationale
reasons from merge-irrelevance (
review / review"gates nothing whether it reportsgreen or red"); required execution-evidence contexts fail closed. The shared
invariant test is split along exactly that line, so both directions are pinned.
Fork-triggered runs get no secrets, so the review can never succeed; without a guard
the new mapping would pin every fork PR red permanently, for a cause no push can
fix. They now skip at job level.
A skipped job is name-stable and a ruleset reads it as success — so on a consumer
where this check is required, a fork PR satisfies the sole required security context
with no review having run. That is the same accepted property as the skip-actors
exception, but note the difference in provenance: skip-actors was explicitly
operator-ratified in ADR 0002's step-3 addendum, and this one has not been. Calling
it out rather than burying it in a workflow comment. Human review of fork changes to
security-sensitive surfaces is the compensating control.
The exemption is scoped to
github.event_name == 'pull_request', which is a securitycontrol, not a style choice — see the third flagged item below.
pull_requestruns fail closedCaught by independent review of the first commit, which had reddened these:
The pinned action cannot serve
merge_groupat all — it is in neitherENTITY_EVENT_NAMESnorAUTOMATION_EVENT_NAMES, soparseGitHubContextthrowsUnsupported event type— andtrack_progress(hardcoded on here) rejects everynon-PR event. Both throw for a cause no head change can fix and no retry can clear.
Had that shipped, 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 redrequired check — carrying no explanation, because the comment steps are PR-gated too.
Only a
pull_requestrun gates a merge, so only apull_requestrun fails closed.Everything else keeps the historical pass-through: still annotated, still classified,
not fatal. The tradeoff is explicit: a genuine infra failure on a
workflow_dispatchinvocation stays green, exactly as it does today.
pull_requestCaught by Codex review of the first commit (P1), and missed by two prior passes:
Written as a bare origin test, the fork exemption also matched a fork PR arriving via
pull_request_targetorworkflow_run— the two privileged triggers this workflowexists to reject. Those runs were skipped at job level before
Reject privileged triggerscould hard-fail them, so a consumer's dangerous misconfiguration would havesurfaced as a green skipped required check: the tripwire silently disarmed by the
guard added to keep fork PRs from being pinned red, producing exactly the
unreviewed-but-green state this PR exists to eliminate.
Scoping to the event name sends every non-
pull_requestevent to the tripwire first.The regression test asserts both halves as a single clause, so the fork test cannot be
reintroduced without its scope.
Known sharp edge — turn-budget exhaustion
A review that exhausts
--max-turnscounts as "no verdict" and now fails closed, andunlike a rate limit no re-run clears it — the remedy is a smaller diff or a higher
--max-turns. That is semantically right (no verdict was produced) but it is a realway for a large security-relevant PR to be blocked, so the PR comment now names that
remedy instead of telling the author to re-run. Flagging it rather than reclassifying:
turning it into a pass-through class would reopen a silent evidence gap, which is a
posture decision, not an implementation detail.
Test plan
New suite
.github/scripts/claude-security-review-fail-closed.test.cjs(13 tests).It executes the real outcome step — extracted from the workflow and run under
bash— rather than grepping the source forexit 1, so what is pinned is the exitcode a replayed payload actually produces.
node --test .github/scripts/*.test.cjs— 287 passing, 0 failing (was 273).bash .github/scripts/classify-infra-failure.test.sh— all cases passing.actionlint .github/workflows/claude-security-review.yml— clean.shellcheck -S styleon both new/changedrun:blocks — clean.biome ci --config-path=fixtures/typescript/good/biome.json .github/scripts— clean.Acceptance criterion 4 — replay of run
30217744377's shape: its payload(
subtype: success,is_error: true,api_error_status: 429, one turn, $0) is fedthrough the real outcome step. Asserted: exit status 1,
review_failed=true,failure_class=rate-limit, and the::error::annotation still emitted. Twocompanion tests assert a completed review still exits 0, and that an unreadable
execution file degrades to class
otherwhile still failing closed.Log hygiene is re-pinned on the new failing path: the replay payload carries a canary
string in the model-authored
resultfield, and the test asserts it reaches neitherstdout nor any step output.
Mutation-verified — all 14 applied to the real workflow at the FINAL branch state,
suite re-run per mutation, files restored from byte snapshots (tree confirmed clean
afterwards):
exit 1removed from the outcome step (the #266 defect itself)continue-on-errorremoved from the retryclaude-review.yml) made to fail closedpull_requestpass-through removed (merge-queue wedge)pull_request(disarms the privileged-trigger tripwire)IS_PULL_REQUESTenv line deleted (fail-OPEN: reverts #266 on every event)IS_PULL_REQUESTrewired to a wrong expression!= "true")The advisory-lane mutation matters most: it proves the narrowing amendment is enforced
in both directions, not just relaxed for the security lane.
The last four were added as the review findings landed. The resolve-step mutation is
the one independent review used to demonstrate the original test gap — it passed all 8
tests before, and is caught now.
Not covered by automated tests
The end-to-end behaviour of a real rate-limited run against live branch protection is
not reproducible in CI. This repo dogfoods the lane via
claude-security-review-self.yml,so this PR exercises the new code on itself;
security-reviewis not a required contexthere and is not in
ci-status'sneeds:, so a red review cannot self-block this merge.Review
Independently reviewed in a fresh context with the rationale withheld, so the audit
targeted the diff rather than the narrative. It found one CRITICAL (the
merge_groupwedge) and three IMPORTANT issues, all fixed in the second commit. Codex review
independently found a P1 that both of those passes missed — the fork guard disarming
the privileged-trigger tripwire — fixed in the third commit. A round-2 pass over that
fix then found a fail-OPEN blind spot it had introduced (an unset
IS_PULL_REQUESTwould have reverted #266 on every event), fixed in the fourth commit by inverting the
guard's sense so the safe default is closed. The
outcome-vs-conclusion, output-survives-exit 1, and skipped-step-context semanticswere verified against the runner source and official docs rather than assumed.
Deployment note — merging this does not yet close the gap on
claude-code-pluginsConsumers pin this reusable by SHA.
claude-code-pluginscurrently pinse2951077a7b43c09fc5a8dee4da52ba6f0fb39ed, which predates this change, so the measured42.6% bypass on that repo persists until its pin is bumped through the ordinary
Dependabot/SHA-bump path. Merging here fixes the reusable; the consumer bump is what
deploys it.
Related
Closes #266.
melodic-software/claude-code-plugins,docs/adr/0002-*.md) — the #509addendum defining the required check as execution evidence, and the step-3 addendum
recording the skip-actor exception.
Spun out separately (github-iac, not touched here) per the decision's prerequisite 4:
the break-glass
bypass_actorsgrant onsecurity-review-gatethat makes a sustainedblackout an explicit, logged, attributable override instead of a hard stop — and the
app-pinning question for required contexts.