Skip to content

chore(ci): bump the review reusables to pick up infra-failure classes - #1552

Merged
kyle-sexton merged 2 commits into
mainfrom
chore/bump-review-reusable-pins
Jul 26, 2026
Merged

chore(ci): bump the review reusables to pick up infra-failure classes#1552
kyle-sexton merged 2 commits into
mainfrom
chore/bump-review-reusable-pins

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

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:

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.

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 of which predate the infra-failure classifier.
When the lane fails here it reports green and says nothing about why.

Bumps both to ci-workflows e295107, which carries the classifier from
ci-workflows#248 plus the status/substring mirror fix from #249. Infra
failures now emit 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 in the safe projection.

The classifier is already proven against a live production failure — the
lane infra-failed on ci-workflows#248's own run and correctly emitted
class=rate-limit with api_error_status 429.

Interface compatibility checked rather than assumed: the workflow_call
block of claude-review.yml is byte-identical across 90f1c54..e295107, and
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, trigger, permission, and template-injection findings). No
input or secret was added, removed, or renamed.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d45804142

ℹ️ 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".

Comment thread .github/workflows/claude-security-review.yml
@kyle-sexton
kyle-sexton marked this pull request as draft July 26, 2026 15:23
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Blocked on a security decision that is not mine to make — converting to draft

The Runner policy gate declined this bump, and it is correct to. Recording the analysis rather than working around it.

What the gate found

runner-target-contract: the reusable workflow path@SHA has no reviewed runner-input contract
(auto-approval declined: inputs changed since the previously reviewed
melodic-software/ci-workflows/.github/workflows/claude-review.yml@1d3762c2)

Auto-approval of a SHA bump requires the reusable's security surface to be structurally identical to a reviewed basis. Diffing the workflow_call block between the reviewed basis 1d3762c2 and the target e295107, the surface gained:

  • a runner input;
  • a standards-ref input — mounts private melodic-software/standards content into the review agent;
  • STANDARDS_REVIEW_APP_ID and STANDARDS_REVIEW_APP_PRIVATE_KEY secrets.

The upstream input's own description states the constraint plainly:

Empty (default) skips the mount entirely, which is the required value for any PUBLIC calling repo — this reusable workflow does not enforce that restriction itself […] mounting private standards content into a review agent whose output lands in public Actions logs is a prompt-injection exfiltration path.

This repository is public. So the bump would approve, for a public caller, a contract whose surface now includes an opt-in private-content mount that the reusable does not itself police — safe only because this caller never sets the input. That is exactly the judgement the gate exists to force, and it declined on purpose.

Why I am not clearing it

  1. The allowlist lives in .github/standards/runner-policy/policy.json, which distribution/sync-manifest.yml lists as a managed materialization of melodic-software/standards. Editing it here would be editing a managed materialization as the source of a change. The real change belongs in standards at components/runner-policy/policy.json.
  2. Adding an approvedReusableWorkflowContracts entry is a security-boundary approval. Self-approving one to unblock my own PR would defeat the control.

There is no clean way around it: the classifier and the standards-ref surface both landed upstream before e295107, so no intermediate SHA carries the fix without the new surface.

What this costs while it sits

Nothing regresses — this repo stays exactly as it is today. But it stays blind: when the review lane infra-fails here it reports green with no machine-readable cause. That is the repo where both blackouts were measured (2h52m, 158/159 runs failed, 36/38 PRs merged unreviewed), and a second blackout was live while this PR was opened.

Draft until an operator decides whether to add the reviewed contract entry upstream in standards.

Upstream: melodic-software/ci-workflows#228, #237, #248, #249.

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

From a concurrent PR-drain sweep — diagnosis only, no pushes to this branch.

This PR cannot go green from inside this repository. Runner policy (and therefore ci-status)
fails for a reason that is upstream-owned:

.github/workflows/claude-review.yml#review: runner-target-contract: the reusable workflow path@SHA
  has no reviewed runner-input contract (auto-approval declined: inputs changed since the previously
  reviewed melodic-software/ci-workflows/.github/workflows/claude-review.yml@1d3762c2)
.github/workflows/claude-security-review.yml#security-review: runner-target-contract: ... (auto-approval
  declined: inputs changed since the previously reviewed .../claude-security-review.yml@99cb082e)

runner-policy.mjs auto-approves a reusable-workflow SHA bump only when the new SHA's security
surface — workflow_call inputs/secrets, effective per-job permissions, and job routing — is
structurally identical to a previously reviewed SHA. e2951077 changes the workflow_call input
contract, so auto-approval is correctly declined and the bump needs an explicit reviewed entry.

That entry lives in .github/standards/runner-policy/policy.json under
approvedReusableWorkflowContracts, keyed …/claude-review.yml@e2951077… and
…/claude-security-review.yml@e2951077…. That file is upstream-owned — every commit that has
ever touched it is a chore: sync standards components PR:

$ git log --oneline -5 -- .github/standards/runner-policy/policy.json
76730807 chore: sync standards components (#1103)
167a9377 chore: sync standards components (#1080)
9533a144 chore: sync standards components (#873)
a08b4ddd chore: sync standards components (#855)
1fd84dad chore: sync standards components (#827)

Editing the materialized copy here would be overwritten on the next sync (AGENTS.md: "fix the
cause upstream in melodic-software/standards and let the sync carry the correction back — never
patch the materialized copy here").

The repo already has a worked precedent for exactly this, recorded in
docs/topics/ladder-climb-roadmap/PLAN.md:161:

ci-workflows#189 (reusable always-report shape, pin 99cb082), standards#229 + sync #827
(reviewed runner-input contract for the new pin)
, #825 (this caller adopts the new input)

So the ordered path to green is:

  1. melodic-software/standards — add the two @e2951077… entries to
    approvedReusableWorkflowContracts (routing, allowedInputs, allowedSecrets, fixedRunsOn),
    reviewing the actual input/secret/permission delta introduced by e2951077.
  2. A chore: sync standards components PR into this repo carrying that policy.json forward.
  3. Then this PR's two SHA bumps pass Runner policy unchanged — no edit needed here.

Until step 2 lands, keeping this PR in draft is the right state; there is nothing to fix on this
branch. Do not add a runner-policy.json exception to route around it — the declined auto-approval
is the gate working as designed on a genuine input-contract change.

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
kyle-sexton marked this pull request as ready for review July 26, 2026 16:11
@kyle-sexton
kyle-sexton merged commit 84f1804 into main Jul 26, 2026
34 checks passed
@kyle-sexton
kyle-sexton deleted the chore/bump-review-reusable-pins branch July 26, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant