Skip to content

fix(ci): Claude Review never ran on draft→ready PRs#12

Merged
topcoder1 merged 1 commit into
mainfrom
fix/pr-review-ready-for-review
Jul 16, 2026
Merged

fix(ci): Claude Review never ran on draft→ready PRs#12
topcoder1 merged 1 commit into
mainfrom
fix/pr-review-ready-for-review

Conversation

@topcoder1

Copy link
Copy Markdown
Owner

What

One line: add ready_for_review to pr-review.yml's trigger types.

Why — a required check was failing open

ci-workflows/claude-review.yml (the reusable) gates its job on draft == false. So:

  1. PR opened as draft → opened fires → review skips (correct)
  2. gh pr ready → fires ready_for_reviewthis workflow isn't listening → nothing runs
  3. review / Claude Review sits at skipped forever — and GitHub counts a skipped required check as SATISFIED

The PR merges green, unreviewed.

This hit the PRs that matter most. Fleet policy makes DRAFT the standard auto-merge opt-out, so every manual-merge PR is draft→ready — the recommended workflow was the one guaranteed to skip the review.

Root cause is upstream, not this repo

The shared caller template shipped without ready_for_review; every repo installed from it inherited the bug. Template fixed in dotclaude#156 — that stops new installs, but can't retroactively fix already-installed repos. Hence this PR.

A fleet audit on 2026-07-15 (YAML-parsed, not grepped) found 43 of the 91 repos with workflows live-vulnerable, 38 byte-identical.

Real merged evidence:

PR what happened
wxa-mcp-server#339 prod proxyproto change — ready_for_reviewmerged 7 seconds later, review / Claude Review: skipping
attaxion_dev#344 merged 3 seconds after ready, same skip

wxa_vpn#983/#984 fixed this independently on 2026-07-04 and nobody swept the fleet — the fix existed for 11 days while the rest stayed exposed.

The rule

If a workflow gates on draft == false — in its own jobs or in a reusable it calls — its types: MUST include ready_for_review. An explicit types: list is a denylist by omission. (ready_for_review is not a GitHub default; the defaults are opened, synchronize, reopened.)

Verification

pr-review.yml YAML-parses to types: ['opened', 'synchronize', 'reopened', 'ready_for_review']. No other change.

Auto-merge rationale: MANUAL MERGE — .github/workflows/** is a blocked: privilege-escalation path by design. Opened as a draft deliberately. Pushed with unset GH_TOKEN (harness token lacks workflow scope).

HUMAN_READABLE_SUMMARY: Our required AI code review was silently never running on any pull request that started as a draft — which, given how our merge policy works, is most PRs a human approves by hand. The workflow wasn't listening for the "draft is now ready" signal, so the check reported "skipped," and GitHub treats a skipped required check as a passing one. This makes it listen.

🤖 Generated with Claude Code

pr-review.yml listed `types:` without `ready_for_review`. The reusable it
calls (ci-workflows/claude-review.yml) gates its job on `draft == false`, so a
PR opened as a draft skips the review at `opened` — and with no
`ready_for_review` trigger, `gh pr ready` fires nothing this workflow listens
for. The review never runs.

`review / Claude Review` then sits at `skipped` forever, and GitHub counts a
skipped required status check as SATISFIED. The PR merges green, unreviewed.
That is a fail-open on a required check.

It hit the PRs that matter most: fleet policy makes DRAFT the standard
auto-merge opt-out, so every manual-merge PR is draft→ready — the recommended
workflow was the one guaranteed to skip the review.

Root cause was the shared caller template (fixed in dotclaude#156); every repo
installed from it inherited this. Fleet audit 2026-07-15 (YAML-parsed, not
grepped): 43 of the 91 repos with workflows were live-vulnerable. Evidence:
wxa-mcp-server#339 went ready_for_review → merged 7 seconds later with
`review / Claude Review: skipping`; attaxion_dev#344 merged 3s after ready,
same skip.

Precedent: wxa_vpn#983/#984 fixed this independently on 2026-07-04;
wxa-jake-ai#879 fixed it there.

RULE: if a workflow gates on `draft == false` — in its own jobs OR in a
reusable it calls — its `types:` MUST include `ready_for_review`.

Auto-Merge-Risk: high

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@topcoder1
topcoder1 marked this pull request as ready for review July 16, 2026 05:44
@github-actions

Copy link
Copy Markdown

Auto-merge blocked — risk-tier paths touched.

This Claude-authored PR modifies files matching the risk-tier patterns
defined in the global CLAUDE.md policy (auth / secrets / migrations /
billing / production infra). Manual click-merge required, OR use one of
the bypass paths below.

Matched files:

.github/workflows/pr-review.yml (matched: ^\.github/workflows/.*)```

**Bypass options (no PR detail navigation needed):**

- Apply the `auto-merge-approved` label to this PR. The workflow will re-run on
  the label event and enable auto-merge. One click from the PR list page.
- Wait for the `review / Codex Review` status check to pass. If Codex Review is
  installed on this repo and it returns SUCCESS, the workflow auto-bypasses
  the risk gate on its next run (e.g. on push of a fixup commit).

If a path is misclassified, fix the regex in
`topcoder1/ci-workflows/.github/workflows/claude-author-automerge.yml`.

@topcoder1
topcoder1 merged commit fbc95a0 into main Jul 16, 2026
11 checks passed
@topcoder1
topcoder1 deleted the fix/pr-review-ready-for-review branch July 16, 2026 05:47
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