test(claude-review): pin the gate-passthrough invariant executably - #251
Conversation
Passing the check on an infrastructure failure is an operator-ratified non-goal, and it was the one invariant in this design with no executable guard — it held only by code reading and actionlint. A later edit adding a nonzero exit to the outcome step, or dropping continue-on-error from the action step, would turn every Anthropic-side blip into a merge blocker across every consumer repo, and CI would not have caught it. Asserts, for both reusables: continue-on-error is present on the action step; every explicit exit in the outcome step is `exit 0`; and neither comment step fails the job. The comment steps are actions/github-script, so a shell `exit` check on them would never fire — the assertion targets `core.setFailed` and `process.exit` instead, and additionally pins that they are still github-script steps, so the check fails loudly rather than silently going inert if their mechanism changes. Mutation-verified, all four caught: an `exit 1` in the outcome step, a removed continue-on-error, a `core.setFailed` in the comment step, and a `process.exit` in the comment step. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 23s —— View job Review
The test is sound. Each assertion pins the correct primitive, the mutation-testing list in the PR description accounts for all four failure paths, and the design rationale in the inline comments is accurate. One minor nit and a few notes follow. Minor:
|
![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: 5fe8f37c60
ℹ️ 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".
…he job Review of the invariant test found the invariant itself was untrue. Both comment steps are actions/github-script and both await GitHub API calls — listComments, then createComment or updateComment. github-script turns an unhandled rejection into a failed step on its own, with no explicit core.setFailed or process.exit involved, and neither step carried continue-on-error. So a rejected API call while *reporting* an outage turned the infra-failure path red, which is exactly what pass-through exists to prevent. The first version of the test asserted only the explicit failure calls, so it certified an invariant that the code did not hold. Adds continue-on-error to both steps in both reusables, and asserts it, so the rejection path is covered rather than assumed. Reporting an outage must never become one; tidying a stale comment on a successful review must not turn it red either. Mutation-verified, all four caught: dropping continue-on-error from the failure-comment step and from the clear-stale step, in each of the two reusables. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error after 1m 36s —— View job Review
|
|
Both findings addressed. @chatgpt-codex-connector's P2 was right, and it was more than a test gap — the invariant itself was untrue. Both comment steps are My first version of the test asserted only the explicit failure primitives, so it certified an invariant the code did not hold. That is the same vacuity class this PR was written to eliminate, reintroduced one layer up. Fixed at the source rather than by narrowing the claim: Mutation-verified, all four caught — dropping Also fixed the State: 273 node tests and 23 shell cases passing, generated blocks in sync ( |
|
Claude encountered an error after 1m 32s —— View job Security Review
No security issues found. Scope of changes: Workflow changes — security analysis: The four affected steps are purely comment-management operations (list/create/update/delete on The two env vars threaded into the failure-notification step's
Test file — security analysis:
Deferred to zizmor: supply-chain / action-pin hygiene, dangerous trigger patterns, template injection in |
|
To use Codex here, create an environment for this repo. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
… review could not run (#269) 🤖 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 `success` when the SDK call failed, certifying an execution that never happened. `success`, `neutral` and `skipped` all satisfy a required check, so `failure` is the only conclusion that can express "in scope and did not run". **Scale, 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. Classified by the workflow's own `Comment on genuine review failure` step conclusion, not by a duration heuristic. ### What lands 1. **Conclusion mapping** keyed on `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: - out-of-scope PRs skip at job level; - **skip-actors** (`dependabot[bot]`, `melodic-standards-sync[bot]`) also skip at job level — ADR 0002's operator-ratified exception is untouched; - a superseded head skips the outcome step, leaving the output unset. 2. **One bounded retry** with a 60s backoff, absorbing the sporadic-429 class (run `30217744377`: 25s failure, 3m17s clean verdict on manual re-run). Ships alongside the mapping, explicitly *not* as a substitute for it — sustained multi-hour blackouts dominate the measured failures and no in-job backoff survives those. 3. **Fork guard**, and **non-`pull_request` runs stay on the pass-through path** — see the flagged consequences below. 4. **Narrowing amendment** to #228's non-goal, in the workflow's POSTURE header and in the render test: pass-through stays ratified for advisory lanes, whose rationale reasons from merge-irrelevance (`review / review` "gates nothing whether it reports green or red"); required execution-evidence contexts fail closed. The shared invariant test is split along exactly that line, so both directions are pinned. ###⚠️ Flagged consequence — fork PRs now skip the job 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 **security control, not a style choice** — see the third flagged item below. ###⚠️ Second flagged consequence — only `pull_request` runs fail closed Caught by independent review of the first commit, which had reddened these: The pinned action **cannot serve `merge_group` at all** — it is in neither `ENTITY_EVENT_NAMES` nor `AUTOMATION_EVENT_NAMES`, so `parseGitHubContext` throws `Unsupported event type` — and `track_progress` (hardcoded on here) rejects every non-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 red required check — carrying no explanation, because the comment steps are PR-gated too. 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. The tradeoff is explicit: a genuine infra failure on a `workflow_dispatch` invocation stays green, exactly as it does today. ###⚠️ Third flagged consequence — the fork guard must stay scoped to `pull_request` Caught 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_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 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_request` event 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-turns` counts as "no verdict" and now fails closed, and unlike 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 real way 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 for `exit 1`, so what is pinned is the exit code 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 style` on both new/changed `run:` 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 fed through the real outcome step. Asserted: exit status **1**, `review_failed=true`, `failure_class=rate-limit`, and the `::error::` annotation still emitted. Two companion tests assert a completed review still exits 0, and that an unreadable execution file degrades to class `other` while still failing closed. Log hygiene is re-pinned on the new failing path: the replay payload carries a canary string in the model-authored `result` field, and the test asserts it reaches neither stdout 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):** | Mutation | Result | | --- | --- | | `exit 1` removed from the outcome step (the #266 defect itself) | caught | | fork guard removed from the job condition | caught | | skip-actors exception removed | caught | | retry inputs diverged from the first attempt | caught | | outcome step points back at attempt 1, ignoring a successful retry | caught | | `continue-on-error` removed from the retry | caught | | **advisory lane** (`claude-review.yml`) made to fail closed | caught | | resolve step never selects the first attempt (would redden every CLEAN review) | caught | | resolve step discards a successful retry | caught | | non-`pull_request` pass-through removed (merge-queue wedge) | caught | | fork guard unscoped from `pull_request` (disarms the privileged-trigger tripwire) | caught | | `IS_PULL_REQUEST` env line deleted (fail-OPEN: reverts #266 on every event) | caught | | `IS_PULL_REQUEST` rewired to a wrong expression | caught | | guard reverted to the fail-open sense (`!= "true"`) | caught | 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-review` is not a required context here and is not in `ci-status`'s `needs:`, 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_group` wedge) 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_REQUEST` would 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 semantics were verified against the runner source and official docs rather than assumed. ### Deployment note — merging this does not yet close the gap on `claude-code-plugins` Consumers pin this reusable by SHA. `claude-code-plugins` currently pins `e2951077a7b43c09fc5a8dee4da52ba6f0fb39ed`, which predates this change, so the measured 42.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. - Decision comment (the spec this implements): #266 (comment) - #228 — the pass-through non-goal this narrows rather than contradicts. - #251 — the executable gate-passthrough invariant whose shared assertion is split here. - ADR 0002 (`melodic-software/claude-code-plugins`, `docs/adr/0002-*.md`) — the #509 addendum 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_actors` grant on `security-review-gate` that makes a sustained blackout an explicit, logged, attributable override instead of a hard stop — and the app-pinning question for required contexts.
1 participant
Summary
Closes the last finding from independent review of #248: the gate-passthrough invariant had no executable test.
Passing the check on an infrastructure failure is an operator-ratified non-goal — it is the single invariant this whole design rests on. The review lane reports a verdict, never an outage. Until now it was guarded only by reading the file and by
actionlint. A later edit that added a nonzero exit to the outcome step, or droppedcontinue-on-errorfrom 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.Asserts, for both reusables:
continue-on-error: trueis present on the action stepexitinReport review outcomeisexit 0One thing worth calling out
My first draft of this test was itself vacuous. The two comment steps are
actions/github-script, not shell — so theexit [1-9]check I originally wrote against them could never fire, no matter how broken they got. It passed, and it proved nothing.The assertion now targets the primitives that actually fail a JS step (
core.setFailed,process.exit), and additionally pins that those steps are stillgithub-script— so if their mechanism ever changes, the test fails loudly with a message saying it must be rewritten, instead of silently going inert again.Test plan
node --test .github/scripts/*.test.cjs— 273 passing, 0 failing.bash .github/scripts/classify-infra-failure.test.sh— 23 cases passing.Mutation-verified, all four caught:
exit 1added to the outcome stepcontinue-on-errorremoved from the action stepcore.setFailed(...)added to the failure-comment stepprocess.exit(1)added to the failure-comment stepEach mutation was applied to the real workflow, the suite run, and the file restored from a byte snapshot — confirmed byte-identical afterwards, with the baseline suite green.
Related
Follow-up to #248 and #249. Refs #237, #228.
No linked issue: this closes a SUGGESTION-tier finding from review of #248, small enough that filing an issue to immediately close it would be noise. The finding and its provenance are recorded above.