fix(claude-review): mirror the status mapping in the substring allowlist - #249
Merged
Conversation
The status path buckets every 5xx into `overloaded` via a range check, but the substring allowlist listed only `overloaded_error` and `api_error`. A payload that recovered no numeric status and carried a serialized 504 `timeout_error` therefore landed in `other` — the two paths disagreed for the same failure. Adds `timeout_error`, which closes the last gap: the allowlist now mirrors the status mapping type-for-type against the published error table, so a payload with no recoverable status lands in the class the status would have chosen. The four remaining published types — `invalid_request_error` (400), `not_found_error` (404), `conflict_error` (409), `request_too_large` (413) — are client errors rather than infrastructure and stay `other` on both paths deliberately. Two of them are now pinned by assertions so that stays a decision rather than an omission, and the header records the mirror rule so adding a type to one path without the other is visibly wrong. Verified by mutation: removing `timeout_error` fails the new case. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
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. |
|
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. |
This was referenced Jul 26, 2026
kyle-sexton
added a commit
that referenced
this pull request
Jul 26, 2026
) ## 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 dropped `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. Asserts, for both reusables: - `continue-on-error: true` is present on the action step - every explicit `exit` in `Report review outcome` is `exit 0` - neither comment step fails the job ### One thing worth calling out My first draft of this test was itself vacuous. The two comment steps are `actions/github-script`, not shell — so the `exit [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 still `github-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:** | Mutation | Result | | --- | --- | | `exit 1` added to the outcome step | caught | | `continue-on-error` removed from the action step | caught | | `core.setFailed(...)` added to the failure-comment step | caught | | `process.exit(1)` added to the failure-comment step | caught | Each 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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
to melodic-software/standards
that referenced
this pull request
Jul 26, 2026
…in e295107 (#275) ## What Add the reviewed runner-input contracts for `melodic-software/ci-workflows/.github/workflows/claude-review.yml@e2951077a7b43c09fc5a8dee4da52ba6f0fb39ed` and `melodic-software/ci-workflows/.github/workflows/claude-security-review.yml@e2951077a7b43c09fc5a8dee4da52ba6f0fb39ed` (the ci-workflows#248/#249 squash-merge SHA) to `approvedReusableWorkflowContracts`. Both entries are shape-identical to the previously reviewed pins (`claude-review@90f1c54`, `claude-security-review@99cb082e`): same routing (`runner-input` via `runner`), same `allowedInputs` (`runner`, `skip-actors`; plus `paths` for security-review), same single `CLAUDE_CODE_OAUTH_TOKEN` secret mapping, and same `allowedCallerPermissions` (`contents:read`, `pull-requests:write`, `id-token:write`). The superseded entries stay until the last caller re-pins and can then be retired, per the #229 precedent. ## Why The runner-policy gate correctly declined auto-approval on melodic-software/claude-code-plugins#1552: both contracts carry `allowedCallerPermissions`, so a bumped SHA's step content must be human-reviewed and recorded here — auto-approval is categorically declined for that contract class regardless of surface identity. This PR is that review. Contract review of `e2951077` against the reviewed pins: - **No** `workflow_call` input, secret, permission, `runs-on`, or routing change in either workflow. `claude-security-review`'s `prompt` input only changed its **default prose** (defer to zizmor's static lane for what it already covers) — no input added or removed, no type/required change. - Step content changed (the reason human review is required): - `gh` CLI run steps (PR head freshness check, infra-failure PR comment lifecycle, PR file listing) replaced with `actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3`, verified against upstream as the official `v9.0.0` annotated-tag commit. Same API operations under the same implicit `github.token` and unchanged job permission grants. - The generated infra-failure classifier (`.github/scripts/classify-infra-failure.sh`) reads the SDK result file and publishes only structured metadata fields plus a constrained `class` token (`auth|rate-limit|overloaded|other`) — the model-authored free text and raw error stacks never leave the block. - No new secret, no widened permission, no runner-label/routing change — no privilege widening. Once merged, the standards-sync PR carries `.github/standards/runner-policy/policy.json` into consumers, and claude-code-plugins#1552 passes unchanged. ## Verification - `npm run lint:runner-policy` — Runner policy passed. - `npm run test:runner-policy` — 238 pass / 0 fail. - `check-jsonschema --schemafile components/runner-policy/policy.schema.json components/runner-policy/policy.json` — ok. No linked issue (reviewed-contract update; the blocked PR lives in claude-code-plugins). ## Related - melodic-software/ci-workflows#248, melodic-software/ci-workflows#249 (merged, `e295107`) - melodic-software/claude-code-plugins#1552 (blocked on this review + sync) - Precedent: #229 → sync → caller bump 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton
added a commit
to melodic-software/claude-code-plugins
that referenced
this pull request
Jul 26, 2026
…#1552) ## Summary This repository is where both silent-review blackouts were measured, and it is the one repository still blind to them. It pinned the review reusable at `90f1c54` (2026-07-18) and the security-review reusable at `99cb082` (2026-07-21) — both predate the infra-failure classifier. When the review lane fails here it reports green and says nothing about why. Bumps both to `ci-workflows@e295107`, which carries: - **melodic-software/ci-workflows#248** — infra failures classify as `auth` | `rate-limit` | `overloaded` | `other`, emitted as a bare `class=<token>` term in the job's `::error` annotation and a `Failure class:` line in the marker-managed infra-status PR comment, with the numeric `api_error_status` alongside in the safe structured projection. - **melodic-software/ci-workflows#249** — the substring allowlist mirrors the status mapping type-for-type, so a payload with no recoverable numeric status lands in the class the status would have chosen. **Gate behavior is unchanged.** Passing the check on an infrastructure failure remains an explicit non-goal upstream; this bump adds diagnosis, not blocking. Nothing about which checks gate a merge here changes. ### Why it matters here specifically The measured impact on this repo, from the investigation on ci-workflows#228: - 2026-07-25T22:48:55Z → 2026-07-26T01:40:48Z, ~2h52m - **158 of the 159** review runs launched in that window failed — a complete census, every run's step conclusions inspected — and every one reported job conclusion `success` - **36 of the 38** PRs merged in that window merged with a silently failed review - A second blackout was still live at the time of writing, with every run since 09:02Z failing Throughout all of it, the only signal was a warning comment. There was no machine-readable cause, and no way to tell a revoked credential from a rate limit without guessing from `duration_ms`. ### Interface compatibility — checked, not assumed - `claude-review.yml`: the `workflow_call` block is **byte-identical** across `90f1c54..e295107`. - `claude-security-review.yml`: differs only in the prose of an input *default* — the security prompt now defers to zizmor's static lane for supply-chain, dangerous-trigger, excessive-permission, and template-injection findings, so this lane focuses on logic, architecture, data-flow, and trust-boundary reasoning. No input or secret was added, removed, or renamed in either. ## Test plan - Interface diff of both reusables' `workflow_call` blocks across the pin jump — verified above, no breaking change. - Confirmed the target SHA exists and that both reusables at `e295107` contain the class emission (`gh api .../contents/...?ref=e295107`). - Upstream verification at the source: 273 node tests and 23 shell cases passing on `ci-workflows@e295107`, with the classification branches and the no-leak canary mutation-verified. - Already proven end-to-end in production: the lane infra-failed on ci-workflows#248's own run and emitted `class=rate-limit` with `"api_error_status":429`, on the exact `subtype: success` / `is_error: true` shape both blackouts produced. - **This PR's own review runs are the live check.** If the lane is still rate-limited when CI runs, the infra-status comment on this PR should now name the class instead of leaving it unexplained — which is the whole point of the bump. ## Related No linked issue. Upstream work is melodic-software/ci-workflows#228, #237, #248, #249. The prior diagnosis in this repo, #1122, is closed; this is the consumer-side adoption of the fix that came out of it, not new work needing its own tracking item. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #248, from an independent post-merge review of it.
The status path buckets every 5xx into
overloadedwith a range check, but the substring allowlist listed onlyoverloaded_error(529) andapi_error(500). A payload that recovered no numeric status and carried a serialized 504timeout_errorlanded inother— the two classification paths disagreed about the same failure.Adding
timeout_errorcloses the last gap. The allowlist now mirrors the status mapping type-for-type against Anthropic's published error table, so a payload with no recoverable status lands in the class the status would have chosen:authauthentication_error401,billing_error402,permission_error403rate-limitrate_limit_error429overloadedapi_error500,timeout_error504,overloaded_error529otherinvalid_request_error400,not_found_error404,conflict_error409,request_too_large413The four
othertypes are client errors rather than infrastructure and stayotheron both paths deliberately. Two are now pinned by assertions so that remains a recorded decision rather than an omission, and the classifier header states the mirror rule so adding a type to one path without the other reads as visibly wrong.I audited the full published table rather than only the one type the review named —
timeout_errorwas the only remaining asymmetry.Test plan
bash .github/scripts/classify-infra-failure.test.sh— now 23 cases, passing. Run under both conditions, withGITHUB_OUTPUTexported (the CI shape) and without (the workstation shape).node --test .github/scripts/classify-infra-failure-render.test.cjs— 4/4 passing.timeout_errorfrom the allowlist fails the newtimeout_errorcase (expected class 'overloaded', got 'other'), then restored.Related
Follow-up to #248. Refs #237, #228.
No linked issue: this is a defect found in review of #248 after it merged, small enough that filing an issue to immediately close it would be noise. The finding and its provenance are recorded above.