feat(work): keep AI pull requests in draft until review is confident - #215
Conversation
The other two pull-request workflows already decline to run while a PR is a draft; CodeQL did not, so a draft still paid for a full analysis. Add the same gate plus the ready_for_review trigger type the un-draft needs, written with an event-name guard because this workflow also runs on push, on a schedule and on dispatch — none of which is gated, so nothing on main or dev goes unanalysed. Refs #211
The implement loop opened its PR ready for review, so every round triggered a full CI run — including the rounds the review was about to hand straight back. The review forms its opinion before CI ever reports, so the expensive signal was being spent ahead of the cheap one. Let the draft state carry the loop's confidence instead: work-implement opens every PR as a draft and never un-drafts it, and work-review marks one ready only where its own review would accept, then waits for CI — green accepts, red hands back the failure as feedback. Once ready a PR stays ready, so the round answering a CI failure sees CI rather than going dark again. The wait needs a bound, and CI duration is per-repo, so work.review.timeout (default 600s) sizes it; still pending when it elapses escalates to needs human, because no verdict may rest on a gate that has not reported. A head that triggers no workflow at all is the existing unknown case and is already answered by the verify run the review performs itself. Refs #211
The CI-gotchas bullet read as an incidental fact about draft PRs. The work loop now depends on it deliberately, and each pull-request workflow needs both halves — the draft gate and ready_for_review in its trigger types — or the un-draft fires nothing. Refs #211
AI review — changes requested (round 1 of 3, head
|
The poll folded every non-fail, non-pending bucket into green, and `skipping` is exactly what a job-level draft gate reports before the un-draft. `gh pr ready` fires a fresh run for the same head SHA but that run takes seconds to register, so a poll inside the window saw three stale `skipping` rows — not empty, so the "none yet" guard never engaged — and wrote `done` on a head CI had never analysed. Filter the buckets by `startedAt` against the moment the head became reviewable: the stale rows drop out, the list reads empty, and the existing "none yet is not none at all" rule handles it. A `skipping` that survives the filter is a job declining on a condition of its own and is a real pass. The deadline needed the same base, and it could not come from the `ready_for_review` event alone — that event is written once, so the round answering a CI failure recomputed it from a flip hours old and escalated before CI reported, and a PR that never carried the event had no base at all. Take the later of the un-draft and this head's own commit, and treat an underivable base as needs-human rather than as an elapsed one, the way an unreadable round count already is. Also stop branching on `gh pr checks`' exit status, which is non-zero for pending and failing runs alike and was blanking the payload it had just read, and fold a cancelled run into pending so the recipe no longer emits a fifth reading neither verdict table consumes. Refs #211
The draft gate reads as "a draft costs nothing to run", and CLAUDE.md went further and called a draft's check list empty. Neither is what a gate on the job produces: it still registers a check run and reports it as `skipping`, one row per gated job, so all three of this repo's workflows leave a full, non-failing list on every draft. That is the shape anything reading those checks has to allow for, so state it where the gate is specified rather than leaving the review side to discover it. Refs #211
Re-work round 2 — review feedback addressed (head
|
AI review — changes requested (round 2 of 3, head
|
ready_for_review visible in the timeline |
new check runs registered | $since |
reading | |
|---|---|---|---|---|
| yes | no | flip | none → poll on |
safe |
| yes | yes | flip | pending → poll on |
safe |
| no | yes | head commit | pending (new rows) → poll on |
safe by luck |
| no | no | head commit | green |
false done |
So the fix's correctness rests on an unstated race: the flip must be visible in the timeline before the new check runs register. The prose already says that registration "takes seconds" — which is precisely the window the first poll lands in, and $since is recomputed on every poll. A transient timeline read (404 / rate limit / 5xx) puts the run in the bottom row deterministically, and the same paragraph that insists an unreadable round count must not read as zero lets this one read as a permissive base without a word.
Same pattern, second instance, two lines above: draft=$(gh pr view "$pr" --json isDraft --jq '.isDraft') || draft=. A failed read leaves draft empty, [ "" = 'true' ] is false, gh pr ready is silently skipped, the PR stays a draft — and the poll then takes the same bottom row and writes green. Nothing asserts the flip actually happened.
Two ways to close it, either sufficient:
- Assert the base instead of degrading it. Where this run un-drafted the PR (or the PR is still a draft), the flip moment is required; an unreadable one is the underivable case →
needs human, exactly as an empty$sincealready is. The head commit is a legitimate base only for a PR that was never a draft. - Stop folding
skippingintogreenat all — round 1's other option, and race-free. Dropskippingrows from the list rather than counting them: a head that genuinely needs no check then lands innone, which the "none yet is not none at all" paragraph already routes correctly, and where the accept rests on step 5's ownverifyas the fourth table row already says. That removes the timestamp filter as a single point of failure rather than tuning it.
Non-blocking
work.review.timeout's documented meaning and the re-work round's base disagree. The schema says the key bounds "the review unit's wait for CI after it marks a draft pull request ready", but on a round answering a CI failure the base is the head commit — so the budget is already being spent during the reviewer's own worktree create,pnpm installandverifybefore the first poll. The behaviour is defensible (it measures CI's own elapsed time, and it fails towardneeds human), but say so in the key's description, because as written a repo sizingtimeoutto its pipeline duration will size it short.select(.startedAt >= $since)drops a row whosestartedAtisnull(jq ordersnullbefore any string). A queued-but-not-started check therefore reads as "not yet" rather than as pending. Safe direction, worth one clause.*fail*sits before*pending*, so a red is returned while sibling jobs are still running. Givenci.yml's!cancelled()fan-out that is probably intended — but the feedback then carries only the jobs that had reported, which is worth stating.- The adjacent CI-gotchas bullet is factually wrong, and this PR is what makes it load-bearing.
CLAUDE.md/AGENTS.mdstill say "CodeQL only fires on**/*.{js,ts,mjs,cjs}or workflow changes", and the PR description repeats it ("it is path-scoped, so it rarely fires on a prose-only PR").codeql.ymlcarries nopaths:filter — verified on feat(work): settle the owner decisions behind the needs-human backlog #207, a docs-and-skills-only PR, where bothAnalyzematrix legs ran and passed. It fires on every PR tomain/dev. The gate this PR adds is therefore worth more than the description claims, but the new poll explicitly depends on "read which workflows the base runs before reading their result", and that bullet is the repo's own authority for it. Pre-existing, so not a blocker — but this is the change that turns it into a correctness input.
Checked and fine
- All three PR workflows confirmed at the head to carry both halves:
ready_for_reviewintypesand a job-levelifdraft gate.codeql.yml'sgithub.event_name != 'pull_request' || …guard is correct —push,scheduleandworkflow_dispatchkeep analysing. gh pr checks --json bucket,startedAtverified to accept both fields and to return ISO-8601 UTC that sorts lexicographically, as the recipe assumes.- The
--paginate-prints-per-page trap is handled:max // emptyper page plussort | tail -1across them picks the true maximum. - Not branching on
gh pr checks' exit status is right — it exits 8 while pending and 1 on failure or with no checks; the old form did blank the payload. casebucket names matchgh's actual vocabulary (pass/fail/pending/skipping/cancel); no substring collisions.- Folding
cancelintopendingis sound givencancel-in-progress: trueon all three workflows. allowed-tools: Bashcoversgh pr ready; the guardrail's argument that an un-draft is not a read-only violation holds.pull-requestopens ready by default and declines to change an existing PR's draft state, so "ask it for a draft" and "leave a re-work's draft state as found" are both reachable as written.CLAUDE.mdandAGENTS.mdare byte-identical (the mirror test passes), and no generated artifact drifted (pnpm skills:checkgreen).- No secrets and no attribution lines in the diff.
The CI poll folded every non-fail, non-pending bucket into green, so the `skipping` rows a job-level draft gate reports could be read as CI having passed. Timestamping them against the deadline base only narrowed that window: the base legitimately falls back to the head commit, which predates the rows the push registers, so one transient timeline read reopened it. Drop `skipping` outright instead. A job that declined said nothing about this head — whether it declined on the draft gate or on a `paths` filter of its own — so the list reads empty and lands in the "none yet is not none at all" branch that already handles it correctly. The poll no longer needs `startedAt`, and `$since` now bounds the wait only, never correctness. An unreadable draft state gets the same treatment as every other unreadable value here: escalate rather than take the permissive reading, instead of letting an empty `isDraft` skip the flip while the loop proceeds as though the pull request had gone ready. Refs #211
The key's description said it bounds the wait "after it marks a draft pull request ready", but the base is the later of the un-draft and the head's own commit — so on a round answering a CI failure, where nothing un-drafts, the clock starts at the push and is partly spent by the review's own worktree, install and verify before the first poll. A repo sizing the timeout to its pipeline duration would size it short. Refs #211
The CI-gotchas bullet claimed CodeQL only fires on JS/TS or workflow changes. `codeql.yml` carries no `paths:` filter at all: both matrix legs run on every pull request to main/dev, verified on a docs-and-skills-only PR. Pre-existing, but the work loop's CI read now depends on knowing which workflows a head actually triggers, which makes this bullet an input rather than trivia. Refs #211
Re-work round 3 — review feedback addressed (head
|
AI review — accepted (round 3 of 3, head
|
# Conflicts: # skills/work/work-implement/SKILL.md # skills/work/work-review/SKILL.md # test/schema.test.ts
The gate landed with #215 and binds every workflow added later: a PR workflow needs both the draft gate and ready_for_review in its trigger types, or the un-draft fires nothing and the check silently never runs. Until now that reasoning lived only in CLAUDE.md and work-implement's REFERENCE, so the decision log read as complete without it. Scoped deliberately: #217 and #218 stay unrecorded here, their rationale travelling with their own skill as ADR-0014 decided.
Summary
CI here is gated on the pull request not being a draft, so a draft costs no CI minutes — but
work-implementopened its PRs ready for review, so every implement round triggered a full run, the rounds a review was about to hand straight back included. The review loop forms its opinion about whether the work is finished before CI ever reports, so the expensive signal was being spent ahead of the cheap one.The draft state now carries the loop's confidence:
work-implementopens every PR as a draft and leaves it there — first round and re-work rounds alike. It never marks a PR ready for review; an already-ready PR is left ready, never pushed back into draft.work-reviewmarks it ready only where its own review would accept, then waits for CI: green →ai: done, red →ai: changes requestedwith the failure as the feedback. Every other verdict leaves the draft alone, so a round the review is rejecting costs no CI at all.Net effect: CI runs once per "the review thinks this is finished" moment rather than once per push.
Reading CI after the un-draft
Two properties of that wait are easy to get subtly wrong, and both are specified rather than left to the recipe's shape.
A
skippingcheck is never a pass, so those rows are dropped rather than counted. A draft gate on the job — which is what all three of this repo's workflows use — does not report nothing while the PR is a draft; it reportsskipping, one row per gated job.gh pr readyfires a fresh run for the same head SHA, but that run takes seconds to register, so a poll inside that window sees a full, non-failing list of stale rows. The poll therefore discards everyskippingbucket outright and reads only the jobs that actually answered: the list falls empty and the existing "none yet is not none at all" rule handles it. A job declining on a condition of its own (apathsfilter, a matrix exclusion) is dropped by the same rule and for the same reason — it said nothing about this head — and lands in the fourth table row, where the accept rests on theverifythis review ran itself.The wait's base exists on every round, and bounds the wait only. It is the later of the
ready_for_reviewevent and this head's own commit — not the event alone, which is written once: the round answering a CI failure finds the PR already ready, writes no new event, and would otherwise recompute its deadline from a flip hours old and escalate before CI reported. A PR that never carried the event at all (one a human opened ready) has the head as its base. An underivable base escalates toai: needs humanrather than being read as an elapsed one — the same treatment an unreadable round count and an unreadable draft state get. Because that base falls back to the head commit it is deliberately not used to decide which check results count; getting it wrong can only make the wait too long or too short.The two open questions in the issue
How long to wait, and what a timeout means. A new
work.review.timeout(seconds, default 600) bounds the wait, mirroringrelease.timeout. It is per-repo because CI duration is: a fixed number would escalate every issue in a repo whose pipeline outlasts it. Still pending when it elapses →ai: needs human, never an accept and never back into draft — no verdict may rest on a gate that has not reported. Its description says what it is measured from, since on a re-work round the budget starts at the push and the review's own worktree, install andverifyare spent inside it."No checks at all" needed no new rule: it is the existing
unknowncase, and step 5's second source already answers it — the review runs the repo's ownverifyagainst the pushed head, so the accept rests on that, not on CI's silence. What did need stating is that none yet and none triggerable are different readings, told apart by reading which workflows the base runs before reading their result.codeql.yml's missing draft gate is in scope, because without it the issue's premise does not hold: CodeQL ran on every draft push and — lackingready_for_reviewin its trigger types — would never have re-run at the moment the review un-drafts. It now carries the same gate, written with an event-name guard because it also runs onpush, on a schedule and on dispatch; none of those is gated, so nothing onmain/devgoes unanalysed.Type of change
Checklist
pnpm verifypasses locallySKILL.mdupdated (description, examples)Test discipline
Mostly a prose change (
SKILL.md/REFERENCE.mdacross both loops), which drivestddnot at all. One slice does touch something a test reaches — thework.review.timeoutconfig key — and that slice was driven red-green: the seam is the schema contract the issue's "how long shouldwork-reviewwait" implies, the failing assertions went intotest/schema.test.tsfirst (/work/review must NOT have additional properties), and the schema key followed.Rounds 2 and 3 are prose-only — every finding was about what the REFERENCE's recipe specifies, not about the schema — so
tdddrove nothing in them, which is the defined outcome for a prose change rather than a skipped step. Round 3's claims were measured against this pull request rather than assumed:codeql.ymlwas read at the head and confirmed to carry nopaths:filter, which is what the corrected CI-gotchas bullet inCLAUDE.md/AGENTS.mdnow says.atomic-commitwas driven for the commit split; its confirmation step could not run unattended and was skipped, which is the documented deviation. The PR was opened with the forge CLI rather than thepull-requestskill, same base and head — the stated fallback.Related issues
Refs #211