ci(lint): enforce ready_for_review on every draft-gated workflow, fleet-wide#120
Merged
Merged
Conversation
…et-wide A workflow gated on `draft == false` whose `types:` omits `ready_for_review` FAILS OPEN: the PR opens as a draft, `opened` skips the job on the gate (correct), then `gh pr ready` fires an event the caller is not listening for, so nothing runs and the check stays `skipped` — which GitHub counts as a SATISFIED required context. The PR merges green and UNREVIEWED. Fleet policy makes DRAFT the standard auto-merge opt-out, so every manual-merge PR is draft->ready. This omission has now been made twice (`reopened` dropped first, then `ready_for_review` dropped directly below the comment warning about `reopened`), and BOTH central fixes were non-retroactive: ci-workflows#115 and dotclaude#156 landed 2026-07-15, yet a sweep on 2026-07-16 still found 21 installed repos vulnerable. A comment did not prevent the recurrence, and fixing the template does not fix consumers. So the rule is pinned to the runtime, in the one place that reaches consumers without a re-install. Adds: - selftest/check_draft_gate_triggers.py — the checker. YAML-parses rather than greps, because the transitive case (a caller with NO `draft` expression, whose gate lives in the reusable it calls) is invisible to grep. Treats an absent `types:` as a violation too: GitHub's defaults exclude ready_for_review, so the default is itself a denylist. - lint.yml `draft-gate-triggers` job — runs the checker against the CALLER's workflows on every PR, so installed-repo drift is caught continuously rather than by a manual audit. Defaults ON (unlike run_shellcheck etc., which default off to avoid style noise) because what it catches is silent and fails open. Opt-out: `run_draft_gate_check: false`. - selftest/ + .github/workflows/selftest.yml — this repo had no selftest dir. Covers the checker's own failure mode, which is passing VACUOUSLY: mutation- tested both ways — dropping a reusable from the registry fails test_registry_matches_repo, and regressing the transitive detection to a text search fails 5 tests. test_registry_matches_repo also derives the draft-gated reusable set from the repo itself, so the registry cannot rot when a new draft-gating reusable is added. While registering the reusables, found the known set was incomplete: alongside claude-review.yml, claude-author-automerge.yml and safe-paths-automerge.yml, codex-review.yml and claude-adversarial-review.yml also gate on `draft == false`. Callers of those two were not covered by any prior audit. Verified: 26 selftests pass; the checker exits 1 on the real pre-fix caller that was live in 20 repos, and 0 on this repo's own workflows; actionlint clean. Simulated against all 30 lint.yml consumers on current main — flags exactly the 21 repos with open fix PRs and no one else, so this goes green fleet-wide once those merge. MERGE ORDER: this must land AFTER the 21 repo fix PRs, or every unfixed repo reddens on its next PR. Co-Authored-By: Claude Opus 4.8 <[email protected]>
topcoder1
marked this pull request as ready for review
July 17, 2026 04:08
|
Risk class: This PR touches one of the blocked path categories from Auto-merge is refused by (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.") |
|
Coverage Floor — mode:
|
|
No issues found. Checker logic is sound (handles PyYAML |
…trap The first push was written against a stale picture of this repo and broke three of its checks. All three were real; fixing them here. 1. `draft-gate triggers` died with "No such file or directory". The job fetched the checker from ci-workflows@main, but on the PR that ADDS the checker, main has no such file — and on any ci-workflows PR it would test main's checker rather than the PR's. In a reusable, `github.repository` is the CALLER's repo, so it now distinguishes: use the local checkout when the caller IS ci-workflows, fetch @main for every consumer. 2. `Tests (Python)` died at COLLECTION with ModuleNotFoundError: no module named 'yaml' — which takes the entire selftest suite with it, not just the new file. pyyaml is now declared in the dev dependency group that `uv run pytest` resolves (uv.lock updated to match). 3. Coverage came in at 92% against this repo's 99.0 floor. Now 100%: the added cases cover the `on:` shapes a real workflow can take — `on: pull_request` (string) and `on: [pull_request, push]` (list) are both valid and used in the wild, and a checker that mis-reads them silently stops checking those workflows. The __main__ guard is `# pragma: no cover` rather than fake-covered. Also dropped two things that fought the existing layout: selftest/conftest.py (this repo's selftest/ is a package with __init__.py, so imports are package-qualified and the sys.path hack was wrong), and .github/workflows/selftest.yml (tests-runner.yml already runs `uv run pytest -q` on this repo's PRs — a second runner was redundant). Verified with the repo's own toolchain: `uv run pytest -q` 33 passed, coverage 100% (floor 99.0), actionlint clean across all workflows, checker exits 0 on this repo and 1 on the real pre-fix caller, and the registry-rot mutation is still caught. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
No issues found. Checker logic, registry sync, and self-reference/bootstrap handling all verified against the repo; test coverage is thorough. |
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.
Why a check, and why here
A workflow gated on
draft == falsewhosetypes:omitsready_for_reviewfails open:openedgh pr readyready_for_reviewThe check stays
skipped, GitHub counts a skipped REQUIRED context as SATISFIED, andthe 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:
reopenedfirst, then
ready_for_reviewdirectly below the comment warning aboutreopened.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 reachesconsumers without a re-install. That closes the gap that let this bug survive twice.
What lands
selftest/check_draft_gate_triggers.pyselftest/)lint.yml→draft-gate-triggersjobpyproject.toml/uv.lockpyyaml(the checker parses YAML)It parses; it does not grep. The transitive case is the one that bites:
pr-review.ymlcontains no
draftexpression at all — the gate lives in the reusable it calls, sogrepping the caller cannot see it. An absent
types:is a violation too: GitHub'sdefaults (
opened/synchronize/reopened) excludeready_for_review, so the default isitself a denylist.
Defaults ON, unlike
run_shellcheckand friends. Those default off to avoid stylenoise 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.repositoryis the caller's repo, so the jobuses the local checkout when the caller is ci-workflows itself and fetches
@maineverywhere else. Without that, this very PR failed with
No such file or directory—@mainhas 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:Callers of the last two were never checked — which is how nanoclaw's
pr-codex-review.ymlwas missed by the original sweep (fixed in topcoder1/nanoclaw#98).
test_registry_matches_repoderives 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:
codex-review.ymlfrom the registrytest_registry_matches_repofailsRun with this repo's own toolchain:
uv run pytest -q→ 33 passed; coverage 100% (floor 99.0)actionlintclean across all workflows; no${{ }}in anydescription:pr-review.ymlthat was live in 20 repos, 0 on this repolint.ymlconsumers on currentmain: flags exactlythe 21 repos with open fix PRs, 9 clean, zero surprises
The first push broke three of this repo's checks (bootstrap, a collection-time
ModuleNotFoundErrorthat would have taken the whole selftest suite down, and 92% vs the99% floor). All three are fixed in the follow-up commit rather than papered over; the
coverage work added the
on: pull_requeststring- and list-form cases, which are realshapes a caller can use.
Auto-merge rationale: MANUAL MERGE — touches
.github/workflows/**(prod infra), andPRs to
ci-workflowsitself are always manual merge per fleet policy. The classifier appliesrisk:blockedand refuses auto-merge automatically. Merge last.🤖 Generated with Claude Code