The vetter sends producer drafts back to needs-work - #233
Conversation
A draft leaked permanently: the vetter structurally could not label one (`vet_action` returned a terminal `SkipDraft`), the leak population never asks whether a PR is a draft, and `leak_reason` fires on any newest producer note. An unlabelled draft plus one required merge-update note sat in `Lane::Leak` with no defect for any ruling to fix. `SkipDraft` becomes `DraftNeedsWork`: the state-load verdicts the draft `ai:needs-work` itself, with the work order that the producer confirm the PR is not a draft if it intends to merge something. The verdict is mechanical because the draft flag decides it, so it is written by the tool rather than offered to the vetter as work no reading could change. The draft arm sits under the `vetted_at_head` check, so the send-back costs one verdict per head rather than one per run, and under `human_sacred`, which still wins. The label edit precedes the comment: the label is what leaves the leak population and the comment is the currency stamp, so writing the stamp first would strand an unlabelled PR that reads as vetted. `leak_reason`, `is_leak_candidate` and the leak population are untouched — the hole closes because the PR now carries an `ai:*` label. `skipDraft` is now `draftNeedsWork` in both the counts and the summary line, and the drafts sent back are named unconditionally in the document: they are the one class of row this call writes to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughDraft pull requests are now classified as ChangesDraft send-back workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Vetter
participant PRClassifier
participant GitHub
participant Report
Vetter->>PRClassifier: load open pull requests including drafts
PRClassifier->>GitHub: apply ai:needs-work label
GitHub-->>PRClassifier: label write result
PRClassifier->>GitHub: create producer work-order comment
GitHub-->>PRClassifier: comment write result
PRClassifier->>Report: record draftNeedsWork and sentBack
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The gate asserted that no committed rollup line carries the post-rename `needsWork` key. That was the same fact as "the fold is load-bearing" only until the first rollup after #231 was committed; sixteen such lines are on main today, so the gate fires on the date rather than on a defect. It now asserts the property that is actually invariant over an append-only history: each folded spelling was measured on committed lines where `needsWork` is absent, which is exactly the span a series folding nothing would lose. Those lines exist and no later rollup can un-write them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pr-review-report-rs/src/main.rs`:
- Around line 54287-54295: Refactor record_draft_send_back to execute its
generated plan through an injectable runner, preserving Iterator::all
short-circuit behavior. Add a test alongside
the_send_back_writes_the_label_before_the_currency_stamp that uses a runner
returning false for the label-edit command and verifies the comment command is
never invoked.
- Around line 21304-21310: Track the number of gh API calls made by the live
unvetted state-load, including the per-open-PR gh pr view calls for drafts, and
expose that count for orgs with many long-lived drafts. Add a rate-limit alert
when frequent state-load runs drive the call volume toward the available limit,
while preserving draft inclusion and vetted_at_head behavior.
- Around line 54231-54256: Update labels_after so the --add-label and
--remove-label branches consume their following label value directly from the
main iterator instead of using the cloning arg closure; preserve the existing
missing-value expectation and label update behavior.
- Around line 20134-20158: Update send_back_draft and its surrounding
draft-send-back flow to handle an empty headRefOid deterministically: either add
a transition after clearing draft flags and pushing to populate the head SHA, or
emit a distinct terminal report instead of repeatedly recording sentBack: false.
Normalize labels into the label_names shape before extraction, supporting label
objects via their name field so removal requests are preserved.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5fdfcb2-451f-458f-8bac-f951e4fc66e4
📒 Files selected for processing (2)
pr-review-report-rs/src/main.rsreview-prompt.txt
`record_draft_send_back` relied on `Iterator::all` short-circuiting inside a function that shells out, so the one property the plan's order buys — a failed label write abandons the currency stamp — rested on nothing any test could reach. The run loop is now `run_draft_send_back`, which takes the runner as an argument, and a test drives both outcomes. The `labels_after` test helper peeked at each flag's value instead of consuming it, so the label was re-read as a flag on the next pass and fell through the catch-all arm. The replay was right only because no `ai:*` label is spelled like a flag; it now consumes, as an argv parser does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The send-back's label edit leaves exactly one `ai:*` verdict, so on a draft already carrying a modeled state it does not fill a hole — it DELETES that state. `ai:close-candidate` and `ai:design` are enumerated BY LABEL, so stripping one retracts a producer's request to destroy work, or a design question, out of the human's queue with nothing on the record but a needs-work comment; `ai:blocked-infra` is the third reachable case. Before this branch a draft was skipped entirely — no write, no strip — so the collapse was new here. The ruling stands: a draft must never sit in nobody's queue. A draft in a modeled state is already in somebody's, and it was never in the leak population the ruling closes — `is_leak_candidate` requires `ai_state_label(..).is_none()`, so a draft carrying an `ai:*` label is not in the leak bucket at all. The leak is the UNLABELLED draft, and that case is unchanged. The guard is DERIVED from `classify_lane`, never a list of labels, for `is_leak_candidate`'s own reason: a hand list drifts the first time a state is added, and this one would drift silently, into destroying the new state's rows. It asks about the WRITE rather than about the PR — every label the plan would REMOVE must name no state, i.e. the classifier asked about that label ALONE still answers `un-vetted`. `Some(false)` for the ready currency is a fact at this arm, not a supposition: the draft arm sits below the currency check. That is why a stale `ai:ready` is still written over and `ai:design` is not — the line is whether the label still names a state, not who wrote it. Per label rather than over the set, because precedence would otherwise SHADOW the state being destroyed (`ai:ready` beside `ai:design` classifies as `un-vetted` by the set). `ai:needs-work` is written over too: it is the send-back's own target, so nothing is stripped and the write is only the currency stamp — which is what heals a send-back whose comment failed and left the PR `Parked` on a human. The withheld draft is `VetAction::SkipDraftInState`, counted under `counts.skipDraftInState` and named nowhere as a write: every PR in it carries a modeled `ai:*` label, so it is already inventoried by the state that label names, which is the entire reason the write was withheld. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The send-back no longer destroys a state a human owes a decision onFolded into this branch. The rule stands as ruled — the vetter sends producer The defect
The consequence is a draft carrying an
Latent, not active: zero live instances of any of the three across Why this is not a narrowing of the rulingThe ruling's intent is that a draft must never sit in nobody's queue. A draft The structural fact that makes it clean: The predicate, and how it is derivedNot a list of three label strings — that drifts the first time a state is added, The question is asked about the write rather than about the PR:
fn draft_send_back_strips_no_state(labels: &[String]) -> bool {
labels_to_remove(labels, STATE_NEEDS_WORK.key)
.iter()
.all(|l| classify_lane(std::slice::from_ref(l), Some(false), false).1 == STATE_UN_VETTED.key)
}Three choices in there, each with a reason:
Two labels are therefore still written over, each for a reason about the write:
The new outcome is TestsInline in
Adversarial pass over the fixCan a label still be destroyed?
Can the guard wrongly suppress a send-back a leaked draft needs? No, and it is Still broken, unchanged by this fix (both pre-date it and neither is
The two other things, assessed1. Protocol-bump write storm — I do not think it needs bounding. A 2. Per-run fetch cost — inherent, and bounded. Confirmed: the bound is one One residual this guard CREATES, found on the adversarial pass
It is not a leak and it is not silent: it carries a label, it is inventoried in The other two protected states do NOT have this shape. Live populations across |
Both this branch and #234 replaced `the_needs_work_series_draws_every_spelling_the_state_has_had` after its premise expired, so main arrived carrying a different fix for the same test. Git's automatic resolution combined them into something neither wrote, and it would not have failed: this branch's `if counts.contains_key("needsWork") { continue; }` line filter survived alongside main's `first_seen.get("needsWork")`. With that filter in place `first_seen` never observes a `needsWork` line, so `native_first` is always `None` and main's ordering assertion takes its `continue` arm for every fold key. The test compiles, passes, and checks nothing about ordering. No conflict marker covered that interaction — the markers were on the doc comment and the accumulator line, and the contradiction sat in the auto-merged region between them. Main's version is taken whole. It is the stronger of the two: it keeps the "every folded spelling has samples" check, adds the ordering property that each folded spelling's earliest measurement strictly precedes the new name's first, and compares stamps by instant so the `Z` and `+00:00` spellings in the committed history cannot order two samples by their zone suffix. This branch's "measured on a line without `needsWork`" is a weaker proxy for the same claim and is now redundant. The draft send-back this branch exists for is untouched; the conflict was confined to that one test. Full suite green on the merge: 1147 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed 92492de: approve Read every commit against the tree, plus the merge I resolved myself. The rule. The write order. Label before comment, and the argument holds: the label is what leaves the leak population, the comment is the currency stamp. Stamp-first plus a failed label write leaves a PR reading as vetted at its head with no state label — un-labelled, never re-derived, permanently leaked, the exact outcome the rule removes. Label-first inverts it into The guard ( The merge ( Known and not fixed, both pre-dating this branch and neither draft-specific: a PR verdicted then hand-stripped of its label still leaks permanently, and a PR verdicted Rulings-conformance: graded against the repo's CLAUDE.md north star and every ruling the human stated for this work, each named with how the artifact obeys it.
|
1 participant
Closes #232
What changed
VetAction::SkipDraftis deleted as a terminal skip and replaced byVetAction::DraftNeedsWork. A draft producer PR is verdictedai:needs-workbythe state-load itself, carrying the work order that the producer confirm the PR
is not a draft if it intends to merge something — clear the flag and hand it
off, or keep shaping it, but not sit where nobody owns it.
Why the tool writes it rather than offering it to the vetter. The verdict is
mechanical: the draft flag decides it, and no reading of the diff can change the
answer. Handing the draft to the vetter as work would spend a scarce vetting slot
(the run budget is 3 items) on a foregone conclusion, and
record_verdict's lensgate would first demand a
pr_checkoutplus anauditinvocation scoped to a PRnobody is asking to have reviewed. The state-load already performs one transition
of exactly this shape — the
ai:blocked-onclearance writes a comment and editslabels in-line — so the precedent and the placement are the existing ones.
Precedence (
vet_action):human_sacred→vetted_at_head→is_draft→Vet. Two things follow from that order, and nothing had to be added for either:head is vetted-at-head like any other PR, so it skips. A draft that sits for
weeks costs one verdict, not one per run; a push that moves its head buys it a
fresh send-back at the new head, exactly as it buys any other PR a fresh
verdict. No second currency notion was introduced — one that existed would be
free to disagree with
vetted_at_head.human_sacredstill wins. The draft arm is below it, so a PR a person hasdecided is never dragged back to needs-work. Pinned in both currency states.
The write, and its order.
draft_send_back_planis a pure function returningthe
ghargv to run, so the order, the label plan and the no-head-sha refusal arevalues a test reads rather than a sequence buried in a network call. The label
edit comes before the comment — the mirror image of the blocked-on clearance's
order, for that order's own kind of reason. The label is what takes the PR out of
the leak population; the comment is the currency stamp that stops the next run
repeating the write. Posting the stamp first and then failing the label write
would leave a PR reading as vetted at its head while wearing no state label at
all — un-labelled, never re-derived, permanently leaked, which is the outcome this
whole rule exists to remove. Label first inverts that: a failed comment leaves
ai:needs-workwith nothing trusted behind it, which isNeedsWorkState::Parked— modeled, visible, safe — and the next state-load re-derives the send-back. Add
and strip ride in ONE
gh pr edit, so the PR is never momentarily wearing two AIverdicts; the edit is omitted entirely when the labels are already right, so the
retry after a half-landed write costs one call, not two.
The leak machinery is untouched.
is_leak_candidate,leak_reasonandproducer_pr_inventoryare unchanged. The hole closes as a consequence: a draftverdicted
ai:needs-workcarries anai:*label, soai_state_label(..).is_some()and the leak arm is unreachable for it — with the producer's notes unchanged,
which is the point. No note text is classified anywhere.
Counters.
skipDraftis gone.counts.draftNeedsWorkis the number of draftsthis call SENT BACK, the summary line reads
N draft->needs-work, and a newdraftNeedsWorkarray names them unconditionally (sentBack: falsewhen thewrite did not land). Unconditional for a stronger reason than
openThreadshas:everything else in that document reports, and this one acts.
Drafts are no longer pre-filtered out of the per-PR fetch — the send-back's
currency check reads the comment thread, so the detail is needed. That is one
gh pr viewper draft per run; the live population is one draft across 148 openauthored PRs.
The vetter prompt and the
unvettedtool description both said drafts were"already excluded"; both now say what happens to them instead.
next_design's own draft withholding (ND_WHY_DRAFT) — unchanged, deliberatelynext_designwithholds a draft from the presented head and names it in thewithheldlist with its reason. I checked it against this ruling and left italone, because it is not the same defect and the ruling does not reach it:
queue. An
ai:designdraft is in a modeled state, is enumerated by name everyrun, and its
countsare a partition — it is owned and visible, which is theproperty
next_designalready argues for in its own doc comment.ground that a ruling would be spent against a tree its author is still shaping.
That reason is untouched by whose queue the PR sits in.
ai:designdraft reaches the vetter's state-load un-vetted at head and is sentback, so it stops being a question waiting on a human and becomes producer work
with a stated move.
Which brings me to the one consequence the issue does not mention, stated plainly
rather than buried: the send-back strips
ai:designfrom a draft that carriesit (
labels_to_removeleaves exactly one AI verdict, as every verdict writedoes). The raised question stays on the PR as a comment and the work order is to
un-draft — after which the question can be re-raised against settled code — but
the human's design queue loses that row in the meantime. No such PR exists today
(one draft in the orgs, carrying
ai:needs-work), so this is a shape to be awareof, not a live regression. Narrowing the rule to spare labelled drafts would
contradict "a draft from the producer is verdicted
ai:needs-work, always", so Iimplemented the ruling as written and am reporting the edge rather than quietly
carving it out.
Adversarial pass over this diff
Can a draft still reach
Lane::Leak?sentBack: falseon the row and named indraftNeedsWork. Self-healing, andloud. A repo that permanently refuses the write (without being in the archived
set) would leak permanently — and be reported as a leak, which is the honest
outcome.
unvetted_fetchandproducer_pr_inventoryuse the sameorg_owner_args()+--author+--state openscope and both withhold archived repos, so the two populationscannot diverge by repo.
change: with the vetter comment still current at head,
vetted_at_headis true,so
unvettedskips the PR and never re-applies a label. It is not draft-specific— a non-draft in the same state leaks identically. Not fixed here; worth its own
issue.
SkipHumanwins, so it stays unlabelled andcan leak. That is constraint 3 of the issue working as specified, not a defect
in the implementation.
None), the row sayssentBack: false.Can the idempotence wrongly suppress a needed verdict? It reuses
vetted_at_headverbatim: a moved head, or a protocol bump, makes the stored verdict non-current
and the send-back is re-derived. A force-push back to an already-verdicted sha
correctly reuses that verdict — same input, same answer.
One surviving starvation, one lane over, that this PR does not fix: a PR
verdicted
ai:readyand then converted to draft with no push keeps a currentverdict at its head, so
vet_actionreturnsSkipVettedbefore the draft arm isreached, while
presentable_queuedrops drafts from the human's queue. It is nota leak (it carries a label, and
human-queuestill lists it underai:ready), butno actor moves it. Closing that would mean putting the draft arm above the currency
check, which re-posts a verdict per run on every draft — the thing constraint 2
forbids. The right shape is probably for
presentable_queueto name its excludeddrafts rather than only count them; that is a separate issue.
QA
vetter_state_load_tests::{a_draft_is_sent_back_rather_than_left_in_nobody_s_queue, a_draft_verdicted_at_its_head_is_not_verdicted_again, a_draft_row_carries_the_send_back_action_and_the_draft_flag, the_send_back_writes_the_label_before_the_currency_stamp, the_send_back_comment_is_the_work_order_and_the_currency_stamp, the_send_back_is_written_once_per_head_and_read_back_as_current, a_sent_back_draft_is_no_longer_a_leak_candidate, the_send_back_leaves_exactly_one_ai_verdict_and_touches_nothing_else, a_retry_after_a_failed_comment_re_posts_only_the_comment, a_draft_with_no_head_sha_is_not_written_to_at_all, only_a_draft_row_is_ever_written_to, the_write_is_driven_by_the_rows_own_head_and_labels, a_failed_send_back_is_stated_on_the_row_and_named_in_the_doc, a_human_decided_draft_is_never_written_to, a_human_decision_survives_a_moved_head}— they cannot be run against base (they nameDraftNeedsWork/draft_send_back_plan/send_back_draft, which base does not have), so base-discrimination is established by mutation instead: M2 and M1 below restore base's exact behaviour ("a draft is never verdicted", and the draft arm above the currency check) and both are killed.main.rs, whole suite re-run, source restored; the pre-existingstate_descriptor_tests::the_needs_work_series_draws_every_spelling_the_state_has_hadfailure was excluded when attributing killers):vet_action— draft arm moved above thevetted_at_headcheck (base's order) -> killed bya_draft_verdicted_at_its_head_is_not_verdicted_again,the_send_back_is_written_once_per_head_and_read_back_as_currentvet_action—is_draftno longer routes to the send-back (base's skip) -> killed bya_draft_is_sent_back_rather_than_left_in_nobody_s_queue+ 6 othersvet_action—human_sacreddemoted below the draft arm -> killed bya_human_decision_survives_a_moved_head,a_human_decided_draft_is_never_written_todraft_send_back_plan— plan reversed, so the comment is posted before the label edit -> killed bythe_send_back_writes_the_label_before_the_currency_stamp(+2)draft_send_back_plan— the--remove-labelloop dropped -> killed bythe_send_back_leaves_exactly_one_ai_verdict_and_touches_nothing_elsedraft_send_back_plan— empty-head guard disabled -> killed bya_draft_with_no_head_sha_is_not_written_to_at_alldraft_send_back_plan— label edit always issued -> killed bya_retry_after_a_failed_comment_re_posts_only_the_commentdraft_send_back_plan— target label switched toSTATE_READY.key-> killed bya_sent_back_draft_is_no_longer_a_leak_candidate(+3)verdict_commentcall —DRAFT_SEND_BACK_NOTEreplaced by""-> killed bythe_send_back_comment_is_the_work_order_and_the_currency_stampsend_back_draft— action guard disabled (writes to every row) -> killed byonly_a_draft_row_is_ever_written_to(+2)send_back_draft—sentBackhardcoded totrue-> killed bya_failed_send_back_is_stated_on_the_row_and_named_in_the_docunvetted_doc—DraftNeedsWorkfolded inton_vetted(the_-arm regression) -> killed bydoc_lists_only_vet_rows_in_vet_first_order_and_counts_the_rest(+2)run_draft_send_back— runner always reports success, so the plan runs on past a failed label write -> killed bythe_write_stops_at_the_first_failure_so_the_stamp_never_outruns_the_label(added in the third commit, after CodeRabbit rightly pointed out the short-circuit was untestable inside a function that shells out).is_leak_candidate/classify_lanedecide whether the composition worked,needs_work_statedecides whether the producer receives a work order,vetted_at_head/verdict_protocol/pr_verdict_linedecide whether the comment is current, and the label outcome is computed by replaying the plannedgh pr editargv rather than by asserting a literal list. The issue's own evidence (flow#475: unlabelled draft + merge-update notes) is the leak fixture.vetted_at_head, (c)human_sacredprecedence intact, (d) counters renamed, (e) noleak_reasonnote-matching, (f) anext_designconsistency verdict. Covered: (a) tests 1-2 + the plan tests, (b)a_draft_verdicted_at_its_head_is_not_verdicted_againand the round-trip test, (c)a_human_decision_survives_a_moved_head+a_human_decided_draft_is_never_written_to, (d)a_failed_send_back_is_stated_on_the_row_and_named_in_the_doc+doc_lists_only_vet_rows_..., (e)a_sent_back_draft_is_no_longer_a_leak_candidateassertsleak_reasonis unchanged across the transition and the leak functions are untouched in the diff, (f) prose above, no code change.A second commit: a main-red the branch fixes
state_descriptor_tests::the_needs_work_series_draws_every_spelling_the_state_has_hadwas failing on
mainbefore this branch existed, and would have failed here forthe same reason. It asserted that no committed
human-queue-history.jsonllinecarries the post-rename
needsWorkkey — true only until the first rollup after#231 was committed, and there are sixteen such lines on
maintoday. The gate wasfiring on the calendar rather than on a defect.
The second commit replaces that assertion with the property that is actually
invariant over an append-only history: each folded spelling was measured on
committed lines where
needsWorkis ABSENT, which is exactly the span a seriesfolding nothing would lose. Those lines exist and no later rollup can un-write
them. (
relinkis still emitted alongsideneedsWorktoday at count 0, so astrict non-overlap rule would not have held either — that is worth knowing, and it
is why the gate is stated as "the folded span is non-empty".)
cargo teston the branch: 1060 passing, 0 failing.One local-only note that does NOT fail CI: nixpkgs' current clippy reports a
collapsible_matchwarning atmain.rs:4096, in code this PR does not touch.rs-static(which pins its own toolchain) is green.Review threads
All four CodeRabbit threads are answered and resolved. Two were real and are fixed
in
3e52132: the run loop moved behind an injectable runner so "a failed labelwrite abandons the currency stamp" is a tested property rather than a reliance on
Iterator::allinside a function that shells out; and thelabels_aftertesthelper peeked at each flag's value instead of consuming it, which made the argv
replay correct only because no
ai:*label is spelled like a flag. Two wereanswered without a change: the missing-head-sha state is
VerdictPlan::NoSha'shandling of the identical condition (loud, never silent, self-healing on the next
readable fetch), and the
label_namesnote reads the detail JSON where the codereads the already-flattened row.