fix(ci): listen for ready_for_review so draft PRs get reviewed by topcoder1 · Pull Request #98 · topcoder1/nanoclaw

topcoder1 · 2026-07-17T02:46:24Z

What

This repo was the fleet's only bespoke case — two vulnerable callers, both also
missing the draft-flag concurrency partition:

file	draft gate	fix
`pr-review.yml`	transitive — via `ci-workflows/claude-review.yml`	`+ready_for_review`, `+`partition
`pr-codex-review.yml`	direct — on its `codex-classify` job	`+ready_for_review`, `+`partition

Plus a paired regression test per this repo's tests/regression/ convention.

Why — this fails OPEN

Both callers gate on draft == false. With types limited to [opened, synchronize, reopened]:

step	event	result
PR opened as draft	`opened`	job skips — correct
`gh pr ready`	`ready_for_review`	neither caller was listening — NOTHING runs

review / Claude Review then sits at skipped — and GitHub counts a skipped REQUIRED
context as SATISFIED. The PR merges green and unreviewed.

This hits 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.

pr-review.yml is the case that hides from grep: it contains no draft expression at
all — the gate lives in the reusable it calls. pr-codex-review.yml gates directly, and
was not in the original fleet-audit list precisely because that audit looked at
pr-review.yml only. An explicit types: list is a denylist by omission.

Why the concurrency partition is in scope

Not a drive-by — it is load-bearing for this bug. A synchronize run whose payload was
snapshotted while the PR was still draft is a guaranteed no-op on the draft gate, yet in a
shared group it can cancel the ready_for_review run. Without the partition, a PR taken
draft → ready could still merge with zero review even with the trigger fixed. The rest
of the fleet already has this on pr-review.yml; this repo did not. Mirrors
ci-workflows/pr-review.yml and pr-codex-review.yml (attaxion_dev#303, 2026-07-06).

pr-codex-review.yml is edited surgically, not synced from the template: this repo's
copy is deliberately customized (it reviews risk:blocked as well as risk:sensitive, and
uses a distinct codex-classify job id so it cannot pollute the required
classify / Classify PR Risk context). Overwriting it from the template would have silently
reverted both.

The regression test

tests/regression/test_workflow_ready_for_review.test.ts encodes the general rule, not
the two filenames that happened to be wrong:

any workflow gating on draft == false — in its own jobs or in a reusable it calls —
must list ready_for_review.

It YAML-parses rather than greps, because in the transitive case there is no draft text
to grep for. A dedicated case asserts that detector property against a fixture containing no
draft expression, so the rule cannot regress into a text search and start passing vacuously
on the exact file it exists to protect.

Verified RED → GREEN (both workflows reverted → both caught):

× pr-codex-review.yml
× pr-review.yml
  AssertionError: ... gates on `draft == false` but does not list `ready_for_review`
  in its pull_request types [opened, synchronize, reopened]
Tests  2 failed | 12 passed (14)

# with the fixes applied
Tests  14 passed (14)

No new dependency: js-yaml and vitest are already declared, and tests/**/*.test.ts is
already in vitest's include. Prettier- and eslint-clean.

Scope

Fixed centrally in ci-workflows#115 and in the caller template in dotclaude#156
(both 2026-07-15) — but neither retroactively fixes already-installed repos. Found by a
fleet-wide YAML-parse sweep on 2026-07-16 (21 repos affected, one PR each). Reference fix:
topcoder1/domain-rank#35.

Verification

End-to-end on this PR itself — a pull_request run uses the workflow from the merge ref, so
this PR tests its own fix: opened as a draft, then marked ready. review / Claude Review
must flip from skipped to a real run with no close→reopen. Verified this way on the pilot,
topcoder1/crawl-infra#43; result recorded in a comment below.

Auto-merge rationale: MANUAL MERGE — touches .github/workflows/**, a blocked: /
prod-infra surface. The classifier applies risk:blocked and refuses auto-merge
automatically. Opened as draft so automerge cannot race the ready click; do not enable
auto-merge.

🤖 Generated with Claude Code

pr-review.yml and pr-codex-review.yml both gate on `draft == false` — pr-review.yml transitively, via ci-workflows/claude-review.yml; pr-codex-review.yml directly, on its codex-classify job. Neither listed `ready_for_review`. With types limited to [opened, synchronize, reopened], a PR opened as a draft skips at `opened`, and `gh pr ready` fires `ready_for_review` — an event neither caller was listening for. Nothing ran. `review / Claude Review` then sits at `skipped`, and GitHub counts a skipped REQUIRED context as SATISFIED, so the PR merges green and UNREVIEWED. This fails open, and it hits 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. Also adds the draft-flag concurrency partition to both callers, which this repo was missing (the rest of the fleet already has it on pr-review.yml). It is load-bearing for the same bug, not a drive-by: a `synchronize` run whose payload was snapshotted while the PR was still draft is a guaranteed no-op on the draft gate, yet in a shared group it can cancel the `ready_for_review` run — so a PR taken draft -> ready would still merge with zero review even with the trigger fixed. Mirrors ci-workflows/pr-review.yml and pr-codex-review.yml (attaxion_dev#303, 2026-07-06). pr-codex-review.yml is edited surgically rather than synced from the template: this repo's copy is deliberately customized (reviews risk:blocked as well as risk:sensitive, and uses a distinct `codex-classify` job id to avoid polluting the required `classify / Classify PR Risk` context). Pairs a regression test per this repo's tests/regression/ convention. It encodes the GENERAL rule rather than the two filenames that happened to be wrong: any workflow gating on `draft == false` — in its own jobs OR in a reusable it calls — must list `ready_for_review`. It YAML-parses rather than greps, because in the transitive case the caller contains no `draft` expression at all. Verified RED before / GREEN after: with both workflows reverted the test fails on both files ("2 failed | 12 passed"); with the fixes applied, 14 passed. Co-Authored-By: Claude Opus 4.8 <[email protected]>

github-actions · 2026-07-17T02:47:10Z

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-codex-review.yml (matched: ^\.github/workflows/.*)
.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`.

github-actions · 2026-07-17T02:47:44Z

Risk class: blocked — manual merge required.

This PR touches one of the blocked path categories from .github/risk-paths.yml (Dockerfiles, docker-compose, .github/workflows/**, **/.env*, **/secrets*, infra/, terraform/, k8s/, or the classifier config itself).

Auto-merge is refused by claude-author-automerge.yml. A maintainer should review the diff and click "Squash and merge" themselves.

(This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.")

github-actions · 2026-07-17T02:48:21Z

Coverage Floor — mode: enforce

metric	value
measured	`71.8%`
floor (current)	`70.8%`
target	`80.0%`
last bumped	`2026-05-22`

github-actions · 2026-07-17T02:48:31Z

Codex review

regression: .github/workflows/pr-review.yml:37 - no test asserts that draft and live events use distinct concurrency groups, so this load-bearing race fix can regress unnoticed
regression: .github/workflows/pr-review.yml:37 - no test asserts that draft and live events use distinct concurrency groups, so this load-bearing race fix can regress unnoticed

…aller's pushed_at (#99) Fixes the intermittent `invariants-runtime-proof.test.ts` failure that has been blocking nanoclaw PRs on the required `ci` check (seen on #98, run 29550755556; passed on a bare re-run). ## Root cause — the fixture, not the predicate JS evaluates argument expressions **before** the call, so: ```js seedItem({ state: 'pushed', pushed_at: Date.now() }); // T0 sampled HERE (caller) // ...then inside seedItem: const now = Date.now(); // T1 sampled here, T1 >= T0 // detected_at: now, ...overrides ``` `pushed_at` (T0) was sampled *before* `detected_at` (T1). When the millisecond ticked between the two reads, `pushed_at` landed **1ms before** `detected_at` and tripped `timestamps-monotonic`. Measured the inversion directly: **477 in 20,000,000 iterations (~1 in 42k)** on an idle machine. A loaded runner's scheduler gap between the two reads widens that window, which is why bare re-runs went green and it read as "just flaky". Six tests shared the pattern (lines 136/142/150/163/169/254) — `handleDismiss` just drew the short straw. **The originally suspected fix would have been wrong.** The predicate is already `>=`-tolerant (it trips on strict `<` only), so equal timestamps never failed it — 99.998% of runs produce exactly equal stamps. Loosening it would have masked a real invariant while leaving the bug in place. Production code is unaffected: every mutation path stamps `pushed_at`/`resolved_at` at mutation time, always after insert. Same root-cause family as #92 (2026-07-01 trust-graduation): **two clock reads assumed to be one instant.** ## Fix The fixture derives its default `detected_at` from the earliest stamp on the row, so a caller-sampled timestamp can't be post-dated. An explicit `detected_at` still wins, keeping the time-reversal counter-examples constructible. Fixture moved to a shared helper so the regression test pins the real thing rather than a copy. ## Verification - Regression test **fails on the pre-fix fixture with the byte-identical CI assertion** (`predicate returned 1 (want 0)`), passes after. - All **30 tests in the suite pass under a clock that ticks on every read** — the condition that made `handleDismiss` fail deterministically pre-fix. Covers all six susceptible tests, not just the one that failed. - Full suite vs pristine `origin/main` baseline: **2113 passed / 0 failed** (baseline 2111 / 0). The 45 module-resolution failures appear identically on untouched `main` — a local `node_modules` artifact, not this change. - `prettier --check` and `eslint` clean on all touched files; no tsc errors in them. Codex rounds: 2 (zero findings both rounds; round 2 adversarially probed whether `Math.min` masks real violations, whether counter-examples still construct, and whether the regression test is tautological — all clear). Auto-merge rationale: safe — test-only + docs. Every changed file matches `safe_test` (`**/__tests__/**`, `tests/**`) or `trivial` (`**/*.md`) in `.github/risk-paths.yml`; none match `blocked:` or `sensitive:`. No production code changes, so runtime behavior is unchanged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Jonathan Zhang <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]>

…et-wide (#120) > ### ⚠️ MERGE ORDER — land this **LAST** > This must merge **after** the 21 repo fix PRs. It defaults ON, so merging it first would > redden every unfixed repo on its next PR. Simulated below: on today's `main` it flags > exactly those 21 and no one else. ## Why a check, and why here A workflow gated on `draft == false` whose `types:` omits `ready_for_review` **fails open**: | step | event | result | | --- | --- | --- | | PR opened as draft | `opened` | job skips on the gate — correct | | `gh pr ready` | `ready_for_review` | **caller not listening — NOTHING runs** | The check stays `skipped`, **GitHub counts a skipped REQUIRED context as SATISFIED**, and the PR merges green and unreviewed. Fleet policy makes DRAFT the standard auto-merge opt-out, so every manual-merge PR is draft→ready. Two facts drove the design: 1. **A comment did not prevent the recurrence.** This has been dropped twice — `reopened` first, then `ready_for_review` *directly below the comment warning about `reopened`*. 2. **Central fixes are not retroactive.** ci-workflows#115 and dotclaude#156 both landed 2026-07-15; a sweep on 2026-07-16 still found **21 installed repos vulnerable**. Fixing the reusable and the template fixes neither existing consumers nor anything installed. So the rule is pinned to the runtime inside `lint.yml` — the one place that reaches consumers **without a re-install**. That closes the gap that let this bug survive twice. ## What lands | file | role | | --- | --- | | `selftest/check_draft_gate_triggers.py` | the checker (+ its selftest, in the existing `selftest/`) | | `lint.yml` → `draft-gate-triggers` job | runs it against the **caller's** workflows, every PR | | `pyproject.toml` / `uv.lock` | declares `pyyaml` (the checker parses YAML) | **It parses; it does not grep.** The transitive case is the one that bites: `pr-review.yml` contains **no `draft` expression at all** — the gate lives in the reusable it calls, so grepping the caller cannot see it. An **absent** `types:` is a violation too: GitHub's defaults (`opened/synchronize/reopened`) exclude `ready_for_review`, so the default is itself a denylist. **Defaults ON**, unlike `run_shellcheck` and friends. Those default off to avoid style noise on first install; this one catches a silent failure that merges unreviewed code. Opt-out is `run_draft_gate_check: false`. **Self-reference:** in a reusable `github.repository` is the *caller's* repo, so the job uses the local checkout when the caller is ci-workflows itself and fetches `@main` everywhere else. Without that, this very PR failed with `No such file or directory` — `@main` has no checker until this merges. ## The registry was incomplete Registering the draft-gating reusables surfaced that the known set of three was wrong. **Five** reusables here gate on `draft == false`: ``` claude-review.yml claude-author-automerge.yml safe-paths-automerge.yml codex-review.yml claude-adversarial-review.yml <-- not in any prior audit ``` Callers of the last two were never checked — which is how nanoclaw's `pr-codex-review.yml` was missed by the original sweep (fixed in topcoder1/nanoclaw#98). `test_registry_matches_repo` derives this set **from the repo itself** and fails on drift, so a new draft-gating reusable can't silently create a class of violation the checker can't see. ## Verification The checker's real failure mode is **passing vacuously** — so the selftest is mutation-tested: | mutation | result | | --- | --- | | drop `codex-review.yml` from the registry | ✅ `test_registry_matches_repo` fails | | regress transitive detection to a text search | ✅ 5 tests fail | | (restored) | ✅ 33 passed | Run with this repo's own toolchain: - `uv run pytest -q` → **33 passed**; coverage **100%** (floor 99.0) - `actionlint` clean across all workflows; no `${{ }}` in any `description:` - checker exits **1** on the real pre-fix `pr-review.yml` that was live in 20 repos, **0** on this repo - **fleet simulation** against all 30 `lint.yml` consumers on current `main`: flags exactly the 21 repos with open fix PRs, 9 clean, zero surprises The first push broke three of this repo's checks (bootstrap, a collection-time `ModuleNotFoundError` that would have taken the whole selftest suite down, and 92% vs the 99% floor). All three are fixed in the follow-up commit rather than papered over; the coverage work added the `on: pull_request` string- and list-form cases, which are real shapes a caller can use. Auto-merge rationale: **MANUAL MERGE** — touches `.github/workflows/**` (prod infra), and PRs to `ci-workflows` itself are always manual merge per fleet policy. The classifier applies `risk:blocked` and refuses auto-merge automatically. **Merge last.** 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <[email protected]>

topcoder1 marked this pull request as ready for review July 17, 2026 02:46

github-actions Bot added the risk:blocked Risk class: blocked label Jul 17, 2026

topcoder1 mentioned this pull request Jul 17, 2026

test(invariants): de-flake — seed fixture sampled detected_at after caller's pushed_at #99

Merged

topcoder1 mentioned this pull request Jul 17, 2026

ci(lint): enforce ready_for_review on every draft-gated workflow, fleet-wide topcoder1/ci-workflows#120

Merged

topcoder1 merged commit 1b40d59 into main Jul 17, 2026
21 of 22 checks passed

topcoder1 deleted the claude/fix-ready-for-review-trigger branch July 17, 2026 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ci): listen for ready_for_review so draft PRs get reviewed#98

fix(ci): listen for ready_for_review so draft PRs get reviewed#98
topcoder1 merged 1 commit into
mainfrom
claude/fix-ready-for-review-trigger

topcoder1 commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

topcoder1 commented Jul 17, 2026

What

Why — this fails OPEN

Why the concurrency partition is in scope

The regression test

Scope

Verification

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Codex review

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant