From 2d49a5ad426794a48b1fd81134603040a33ca1c6 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 17:27:06 +0000 Subject: [PATCH 1/5] refactor(labels): split PR labels into state (whose ball) and blocker (what is in the way) Retires state:needs-rebase. Mergeability, check status and the review round are independent facts; forcing them through one totally-ordered label meant one always won and the rest vanished, which is where every precedence bug in this machine has lived. needs-rebase was the clearest casualty -- it fired on both a conflict and a failing check, which need opposite work. Blockers are a set, so there is no ordering between them to get wrong. The ordered axis is now purely about reviews. bots-reviewing also tightens to mean a request is actually live; a PR nobody was asked to review is the agent's. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 112 ++++++++++++++++++++++------ CHANGELOG.md | 28 +++++++ LABELS.md | 62 +++++++++++---- test/labels-reconcile.sh | 62 ++++++++++++--- 4 files changed, 214 insertions(+), 50 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index db00ea3..216353e 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -31,7 +31,11 @@ set -euo pipefail HUMAN="${HUMAN_REVIEWER:-danmt}" BOTS=(claude-bot-andresmgsl codex-bot-andresmgsl grok-bot-andresmgsl) -STATES=(state:building state:needs-rebase state:bots-reviewing state:addressing state:needs-human) +STATES=(state:building state:bots-reviewing state:addressing state:needs-human) +BLOCKERS=(blocker:conflict blocker:ci-red blocker:unrequested) +# Labels this machine used to own and no longer does. Cleared on sight so a +# retirement heals the board instead of stranding a label nothing recomputes. +RETIRED=(state:needs-rebase) STALE_AFTER=$((48 * 3600)) log() { printf 'labels: %s\n' "$*"; } @@ -153,25 +157,61 @@ human_request_needed() { # 0 when needs-human requires a FRESH human request return 0 } +blockers() { # → the blocker:* labels this PR should carry, one per line + # The second axis. These are FACTS ABOUT THE BRANCH, and they are mutually + # independent — a PR can be conflicted and red and unasked at once — so they + # are a set, not an ordering. That is the whole point of splitting them out + # of state:*: every precedence bug this machine has had (needs-human + # surviving a conflict, MISSING swallowing STALE) came from projecting + # independent facts onto one totally-ordered label. A set has no precedence + # to get wrong. + # + # UNKNOWN mergeability is deliberately NOT a conflict: GitHub reports it for + # about a minute after every merge while it recomputes, and flapping every + # open PR on each merge would be worse than the bug. Same for a failed read + # of either fact — both default to the "do not know" value, which blocks + # nothing. An unset global (an older fixture, a failed fetch) must never + # invent a verdict it did not read. + case "${MERGEABLE:-UNKNOWN}" in CONFLICTING) echo blocker:conflict ;; esac + case "${CHECKS:-NONE}" in FAILURE) echo blocker:ci-red ;; esac + + # Nobody is on the hook for a verdict somebody still owes. Distinct from + # bots-reviewing, which says a request is live and an answer is coming: + # here the round is stalled because no one was ever asked, and the board + # said "waiting on the bots" for the 48h it took `stale` to notice. + # A draft is exempt (the bots ignore drafts by design), and so is an + # explicit human request — a maintainer claiming a PR early is deliberate, + # not a dropped ball. + if [ "$DRAFT" != true ] && ! requested "$HUMAN"; then + local b any_missing=false any_requested=false + for b in "${BOTS[@]}"; do + requested "$b" && any_requested=true + [ "$(bot_verdict "$b")" = MISSING ] && any_missing=true + done + if [ "$any_missing" = true ] && [ "$any_requested" = false ]; then + echo blocker:unrequested + fi + fi +} + decide_state() { # → the one state:* label this PR should carry if [ "$DRAFT" = true ]; then echo state:building; return; fi - # state:needs-human means ONE thing: a human could merge this right now. - # Anything that makes that false outranks the request that put it there — - # otherwise the board invites a merge that cannot or must not happen, and - # nothing else on the page contradicts it (#136). - # - # A conflicted or red branch is the agent's to fix, not the human's to - # merge. UNKNOWN is deliberately NOT treated as unmergeable: GitHub reports - # it for a minute after every merge while it recomputes, and flapping every - # open PR through needs-rebase on each merge would be worse than the bug. - # An unknown mergeability simply does not trigger this arm; the next sweep - # sees the settled value. - # Both default to the "do not know" value: an unset global (older fixture, - # a failed fetch) must never invent a verdict it did not read. - case "${MERGEABLE:-UNKNOWN}" in CONFLICTING) echo state:needs-rebase; return ;; esac - case "${CHECKS:-NONE}" in FAILURE) echo state:needs-rebase; return ;; esac + local s + s="$(round_state)" + # The one rule joining the two axes: state:needs-human means a human could + # merge this RIGHT NOW, so it requires a clear branch. Any blocker at all + # means the work is the agent's — whatever the review round says — and the + # blocker label says which work it is. Nothing else in this function reads + # the branch, which is what keeps the ordering below purely about reviews. + if [ "$s" = state:needs-human ] && [ -n "$(blockers)" ]; then + echo state:addressing; return + fi + echo "$s" +} + +round_state() { # → the state the REVIEW ROUND alone implies; knows no branch facts local b verdicts="" for b in "${BOTS[@]}"; do if requested "$b"; then echo state:bots-reviewing; return; fi @@ -199,9 +239,16 @@ decide_state() { # → the one state:* label this PR should carry # No verdict at all from some bot, and nothing staled. An explicit human # request still outranks an unfinished round — a maintainer pulling a PR # to themselves early is a deliberate act, and the original precedence. + # + # Otherwise it is the AGENT's ball, not the bots'. The loop above already + # returned for every live bot request, so reaching here with a MISSING + # means somebody owes a verdict and nobody was asked for one — the round + # is not running. Calling that bots-reviewing was the lie that let a + # forgotten PR read "waiting on the reviewers" for the 48h it took the + # stale sweep to notice. blocker:unrequested says why. *MISSING*) if requested "$HUMAN"; then echo state:needs-human; return; fi - echo state:bots-reviewing; return ;; + echo state:addressing; return ;; esac # an explicit human request outranks the remaining bot outcomes — it is the # final gate, and a maintainer pulling a PR to themselves early counts too @@ -233,8 +280,10 @@ bootstrap_labels() { # dispatch-only: ~20 upserts is too chatty for every cron t state:building|FBCA04|PR is a draft — the coding agent is still building state:bots-reviewing|1D76DB|Waiting on the bot reviewers to finish the round state:addressing|D93F0B|All bots reviewed — coding agent owes the single reply + fixes -state:needs-rebase|B60205|Does not merge — conflicts or failing checks; the agent owes a fix -state:needs-human|8250DF|Mergeable, green, all bots approve — waiting on the human reviewer +state:needs-human|8250DF|No blockers, all bots approve — waiting on the human reviewer +blocker:conflict|B60205|Does not merge — the branch conflicts and the agent owes a rebase +blocker:ci-red|B60205|A check is failing — the agent owes a fix (not a rebase) +blocker:unrequested|E99695|Somebody still owes a verdict and nobody was asked for one merge-next|0E8A16|Head of the merge queue — merge this one next (set by hand/agent, cleared here) stale|B60205|No activity for 48h — needs a poke (sweep-managed) blocked|6A737D|Waiting on another PR or issue to land first @@ -267,17 +316,34 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch log "#$n: requested $HUMAN (round passed)" fi - # ---- converge the state:* labels ---- + # ---- converge both axes ---- + # state:* is exclusive (everything but $desired comes off); blocker:* is a + # set (each one on or off on its own); RETIRED always comes off. One edit + # call for all of it, so a PR never flickers through a half-applied board. + local want_blockers add="" + want_blockers="$(blockers)" + remove="" for s in "${STATES[@]}"; do if [ "$s" != "$desired" ] && has_label "$s"; then remove="$remove,$s"; fi done + for s in "${RETIRED[@]}"; do + if has_label "$s"; then remove="$remove,$s"; fi + done + for s in "${BLOCKERS[@]}"; do + if grep -qxF "$s" <<<"$want_blockers"; then + has_label "$s" || add="$add,$s" + else + has_label "$s" && remove="$remove,$s" + fi + done + add="${add#,}" remove="${remove#,}" - if ! has_label "$desired" || [ -n "$remove" ]; then - args=(--add-label "$desired") + if ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; then + args=(--add-label "$desired${add:+,$add}") [ -n "$remove" ] && args+=(--remove-label "$remove") if run gh issue edit "$n" -R "$REPO" "${args[@]}" >/dev/null; then - log "#$n: state -> $desired${remove:+ (cleared $remove)}" + log "#$n: state -> $desired${add:+ +$add}${remove:+ (cleared $remove)}" else # a deleted label must not wedge the sweep — dispatch heals the taxonomy log "#$n: WARNING: label edit failed (missing label? run the workflow manually to bootstrap)" diff --git a/CHANGELOG.md b/CHANGELOG.md index 0679af6..434052b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,34 @@ which records not just what changed but what each drill run proved. ### Changed +- **PR labels split into two axes: `state:*` (whose ball) and `blocker:*` + (what is in the way)** — `state:needs-rebase` is retired, replaced by + `blocker:conflict`, `blocker:ci-red` and `blocker:unrequested`. One rule + joins them: `state:needs-human` requires zero blockers. + + The single-label design forced independent facts through one totally-ordered + value, and the ordering was where every bug lived. Mergeability, check + status and the review round move independently — a PR can be conflicted + *and* red *and* stalled at once — so a total order has to pick a winner and + silently drop the rest. `state:needs-rebase` was the clearest casualty: it + fired on both a conflict and a failing check, which need opposite work, and + told an agent to rebase when what it owed was a bug fix. On this repo's own + board, #120 was conflicted **and** red and could only say one of them. + + Blockers are a set, so there is no precedence between them to get wrong. + What is left on the ordered axis is purely about reviews, which is the one + place an ordering is genuinely meaningful. + + `state:bots-reviewing` also tightens to mean strictly *a request is live and + an answer is coming*. A ready PR nobody was asked to review read "waiting on + the reviewers" for the 48 hours it took the stale sweep to notice; it is now + `state:addressing` + `blocker:unrequested`, because the agent owes the ask. + Drafts are exempt (the bots ignore drafts by design), as is an explicit + human request — a maintainer claiming a PR early is deliberate. + + The reconciler strips `state:needs-rebase` on sight, so the retirement heals + the board rather than stranding a label nothing recomputes. Fixtures 51 → 64. + - **The tenant templates carry rig's family suffix: `claude` → `claude-box`, `codex` → `codex-box`, `grok` → `grok-box`, `staging` → `staging-box`** (#123, following heavy-duty/rig#76) — rig is growing a second family of diff --git a/LABELS.md b/LABELS.md index 13a39cd..1ce4cd4 100644 --- a/LABELS.md +++ b/LABELS.md @@ -16,33 +16,61 @@ single reply, and a human takes the final review. |---|---|---|---|---| | `state:building` | `#FBCA04` | the coding agent, still building | PR opened as draft | marked ready + bot reviews requested | | `state:bots-reviewing` | `#1D76DB` | the reviewer bots to finish the round | ready with reviews requested, or fixes pushed and reviews re-requested | all three bots have reviewed the round | -| `state:addressing` | `#D93F0B` | the coding agent to reply and push fixes | all bots reviewed the round, not all approved | the single round-reply is posted and fixes pushed | -| `state:needs-rebase` | `#B60205` | the coding agent to rebase or fix | the branch does not merge — GitHub says `CONFLICTING`, or a check has failed | it merges cleanly and checks are green again | -| `state:needs-human` | `#8250DF` | the human reviewer | the PR **could be merged right now**: mergeable, checks not failing, three formal head-current approvals — and the human review is requested | merged — or changes requested, which cycles back to `state:addressing` | +| `state:addressing` | `#D93F0B` | the coding agent to reply, fix, or ask | all bots reviewed and not all approved; or nobody was asked; or a blocker is up | the thing the blocker names is done | +| `state:needs-human` | `#8250DF` | the human reviewer | the PR **could be merged right now**: no blockers, three formal head-current approvals — and the human review is requested | merged — or changes requested, which cycles back to `state:addressing` | `bots-reviewing` and `addressing` are deliberately distinct: staleness in the first means *poke the bots*, staleness in the second means *the agent dropped the ball*. Collapsing them loses exactly the information a sweep needs. +`bots-reviewing` therefore means strictly *a request is live and an answer is +coming* — a PR nobody was asked to review is the agent's ball, not the bots'. + +## The second axis: `blocker:*` + +State answers *whose ball is it*. Blockers answer *what is in the way*, and +unlike states they are *facts about the branch* — mutually independent, so a +PR carries as many as apply. + +| Label | Color | Means | Clears when | +|---|---|---|---| +| `blocker:conflict` | `#B60205` | GitHub says `CONFLICTING` — the agent owes a **rebase** | it merges cleanly | +| `blocker:ci-red` | `#B60205` | a check failed — the agent owes a **fix**, which a rebase will not provide | checks are green | +| `blocker:unrequested` | `#E99695` | somebody still owes a verdict and **nobody was asked** for one | reviews are requested | + +One rule joins the axes: **`state:needs-human` requires zero blockers.** Any +blocker means the work is the agent's, whatever the review round says. + +This split exists because the single-label version kept lying. Independent +facts were projected onto one totally-ordered label, so one always had to win +and the losers vanished off the board: a PR that was *both* conflicted and red +could only say one of them, and `needs-rebase` told an agent to rebase when +what it actually owed was a bug fix. Precedence between two blockers is not a +question a set has to answer, which is why every ordering bug this machine has +had — `needs-human` surviving a conflict, `MISSING` swallowing `STALE` — lived +on the axis that had to be totally ordered. + +`state:needs-rebase` was the first attempt at this and is **retired**; the +reconciler strips it on sight so no PR is left carrying a label nothing +recomputes. **`state:needs-human` means one thing: a human could merge this right now.** -Anything that makes that false outranks the review request that put it there, -because the label is the only signal a maintainer scanning the board (or a -phone) actually reads — and a label that says "your turn" on an unmergeable PR -is worse than no label at all. Two things therefore take precedence over an +The label is the only signal a maintainer scanning the board (or a phone) +actually reads, and one that says "your turn" on an unmergeable PR is worse +than no label at all. So beyond the blockers, one review fact also outranks an explicit human request: -- **it does not merge** — `CONFLICTING`, or a failing check → `state:needs-rebase` - **nobody reviewed *this* head** — every approval staled by a push → `state:addressing`, because the agent owes a re-request -The second is the more dangerous of the two: with a conflict, GitHub at least -disables the merge button, while a staled-approval PR reads green, mergeable +That case is more dangerous than any blocker: a blocked PR at least shows an X +or a disabled merge button, while a staled-approval PR reads green, mergeable and "waiting on the human" over code no reviewer has seen. -`UNKNOWN` mergeability is deliberately **not** treated as unmergeable. GitHub +`UNKNOWN` mergeability is deliberately **not** treated as a conflict. GitHub reports it for about a minute after every merge while it recomputes, and -flapping every open PR through `needs-rebase` on each merge would be worse than -the bug this precedence fixes. +flapping every open PR through `blocker:conflict` on each merge would be worse +than the bug this fixes. A failed read of either branch fact degrades to the +same "do not know" value, for the same reason. An *unfinished* round still yields to an explicit human request — a maintainer pulling a PR to themselves early is a deliberate act. `MISSING` (nobody has @@ -96,8 +124,12 @@ missing label idempotently. To create them by hand (needs push access): gh label create "state:building" --color FBCA04 --description "PR is a draft — the coding agent is still building" --force gh label create "state:bots-reviewing" --color 1D76DB --description "Waiting on the bot reviewers to finish the round" --force gh label create "state:addressing" --color D93F0B --description "All bots reviewed — coding agent owes the single reply + fixes" --force -gh label create "state:needs-rebase" --color B60205 --description "Does not merge — conflicts or failing checks; the agent owes a fix" --force -gh label create "state:needs-human" --color 8250DF --description "Mergeable, green, all bots approve — waiting on the human reviewer" --force +gh label create "blocker:conflict" --color B60205 --description "Does not merge — the branch conflicts and the agent owes a rebase" --force +gh label create "blocker:ci-red" --color B60205 --description "A check is failing — the agent owes a fix (not a rebase)" --force +gh label create "blocker:unrequested" --color E99695 --description "Somebody still owes a verdict and nobody was asked for one" --force +# retired — the reconciler strips it; delete it once no PR carries it +# gh label delete "state:needs-rebase" +gh label create "state:needs-human" --color 8250DF --description "No blockers, all bots approve — waiting on the human reviewer" --force gh label create "merge-next" --color 0E8A16 --description "Head of the merge queue — merge this one next (set by hand/agent, cleared here)" --force gh label create "stale" --color B60205 --description "No activity for 48h — needs a poke (sweep-managed)" --force gh label create "blocked" --color 6A737D --description "Waiting on another PR or issue to land first" --force diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index b68de99..863e216 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -44,10 +44,15 @@ $BOT3" REVIEWS_JSON='[]' expect "requested bots mean bots-reviewing" state:bots-reviewing "$(decide_state)" # -- a bot that never reviewed keeps the round open --------------------------- -REQUESTED="" REVIEWS_JSON="$(reviews \ +# With a live request that is the bots' ball; with NO request outstanding it +# is the agent's, because nothing is coming until somebody asks. +REQUESTED="$BOT3" REVIEWS_JSON="$(reviews \ "$(rev "$BOT1" APPROVED head1 "" t1)" \ "$(rev "$BOT2" APPROVED head1 "" t2)")" -expect "missing bot review means bots-reviewing" state:bots-reviewing "$(decide_state)" +expect "a missing bot WITH a live request is bots-reviewing" state:bots-reviewing "$(decide_state)" +REQUESTED="" +expect "...but with nobody asked it is the agent's ball" state:addressing "$(decide_state)" +expect "...and the blocker names the stall" blocker:unrequested "$(blockers)" # -- a comment is a non-verdict, agreement body or not: the author escalates -- REVIEWS_JSON="$(reviews \ @@ -157,23 +162,55 @@ ALL_APPROVE="$(reviews \ "$(rev "$BOT3" APPROVED head1 "" t3)")" # -- flavour 1: not mergeable. The merge button is disabled, yet the board -# said "your turn" on #119/#120/#127 for hours. +# said "your turn" on #119/#120/#127 for hours. The branch fact now rides +# the blocker axis; the state says whose ball it is, which is the agent's. DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=CONFLICTING CHECKS=SUCCESS -expect "a CONFLICTING PR is needs-rebase, not needs-human" state:needs-rebase "$(decide_state)" +expect "a CONFLICTING PR is the agent's, not the human's" state:addressing "$(decide_state)" +expect "...and says WHY on the blocker axis" blocker:conflict "$(blockers)" REQUESTED="$HUMAN" -expect "...even with the human explicitly requested" state:needs-rebase "$(decide_state)" +expect "...even with the human explicitly requested" state:addressing "$(decide_state)" -# -- red CI is the same claim: not something a human should merge. +# -- red CI is the same claim, but NOT the same work: a rebase does not fix a +# failing test. Collapsing both into one needs-rebase label told the agent +# to do the wrong thing, which is why the axis split exists. REQUESTED="" MERGEABLE=MERGEABLE CHECKS=FAILURE -expect "a red PR is needs-rebase" state:needs-rebase "$(decide_state)" +expect "a red PR is the agent's" state:addressing "$(decide_state)" +expect "...and is distinguishable from a conflict" blocker:ci-red "$(blockers)" REQUESTED="$HUMAN" -expect "...and a human request does not override red CI" state:needs-rebase "$(decide_state)" +expect "...and a human request does not override red CI" state:addressing "$(decide_state)" + +# -- both at once. The single-axis design could not say this at all: one label +# had to win, and the loser silently vanished off the board. +REQUESTED="" MERGEABLE=CONFLICTING CHECKS=FAILURE +expect "a conflicted AND red PR reports both blockers" "blocker:conflict +blocker:ci-red" "$(blockers)" +expect "...and is still just the agent's ball" state:addressing "$(decide_state)" # -- UNKNOWN is NOT unmergeable. GitHub reports it for ~a minute after every # merge while it recomputes; treating it as broken would flap every open PR -# into needs-rebase on each merge — worse than the bug being fixed. +# on each merge — worse than the bug being fixed. REQUESTED="" MERGEABLE=UNKNOWN CHECKS=PENDING -expect "UNKNOWN mergeability does not trigger needs-rebase" state:needs-human "$(decide_state)" +expect "UNKNOWN mergeability blocks nothing" state:needs-human "$(decide_state)" +expect "...and raises no blocker" "" "$(blockers)" + +# -- blocker:unrequested — the stalled round. Nobody owes an answer because +# nobody was ever asked, yet the board read "waiting on the bots" until +# `stale` noticed 48h later. +MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]' +expect "ready, nobody asked, nothing reviewed raises unrequested" blocker:unrequested "$(blockers)" +# ...the partial case is equally stalled: one verdict in, nobody asked for the rest +REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" +expect "one bot in, none requested is still unrequested" blocker:unrequested "$(blockers)" +# ...but a live request means an answer IS coming +REQUESTED="$BOT2" +expect "a live bot request is not a stalled round" "" "$(blockers)" +# ...and a draft is exempt: the bots ignore drafts by design +DRAFT=true REQUESTED="" REVIEWS_JSON='[]' +expect "a draft with nobody asked is not stalled" "" "$(blockers)" +# ...as is an explicit human request — claiming a PR early is deliberate +DRAFT=false REQUESTED="$HUMAN" +expect "an early human claim is not a stalled round" "" "$(blockers)" +REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS # -- flavour 2 (the dangerous one): mergeable, green, human requested, and # NOBODY has reviewed this head. Observed on #119 after a rebase: every @@ -207,7 +244,7 @@ expect "...and the same when the stale verdict is the LAST bot in BOTS" \ REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" expect "an unfinished round still yields to an explicit human request" state:needs-human "$(decide_state)" REQUESTED="" -expect "...and without that request it is still bots-reviewing" state:bots-reviewing "$(decide_state)" +expect "...and without that request the agent owes the ask" state:addressing "$(decide_state)" # --------------------------------------------------------------------------- # checks_state: the rollup classifier. It lived inline in main() for the first @@ -314,7 +351,8 @@ expect "...and the same when it finished green — mid-flight is not mergeable" # take the PR off the human's plate, which is the whole point of #136. DRAFT=false HEAD_SHA=head1 REQUESTED="$HUMAN" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS="$(rollup "[$(run_ a SUCCESS),$(run_ b CANCELLED)]" | checks_state)" -expect "a cancelled check reaches decide_state as needs-rebase" state:needs-rebase "$(decide_state)" +expect "a cancelled check reaches decide_state as the agent's ball" state:addressing "$(decide_state)" +expect "...via blocker:ci-red, not a conflict" blocker:ci-red "$(blockers)" # -- the happy path survives all of the above. REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" From 9bf669dabc7bcbbcbc4ca8661afdd123d5568456 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 17:36:46 +0000 Subject: [PATCH 2/5] fix(labels): a failed rollup read must not read as "nothing is failing" gh returning nothing left the rollup KEY absent, which collapsed into the same NONE as a PR that genuinely has no checks -- so an API hiccup presented as mergeable-by-a-human. That is the unknown-certified-as-green shape this machine exists to stop, surviving in the one place the #136 fix did not look. checks_state now reports UNREADABLE for an absent key, and the sweep leaves that PR exactly as it is rather than recomputing on facts it did not read. Blocking on it instead would flap the whole board on one bad call. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 22 ++++++++++++++++++++-- test/labels-reconcile.sh | 7 +++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 216353e..89ff1cb 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -56,7 +56,14 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing requested() { grep -qxF "$1" <<<"$REQUESTED"; } -checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE +checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | UNREADABLE + # UNREADABLE is the absence of the key itself, which is what a failed fetch + # leaves behind — distinct from a present-but-empty rollup, which honestly + # means this PR has no checks. Collapsing the two let an API hiccup present + # as "nothing is failing", i.e. as mergeable-by-a-human: the same + # unknown-certified-as-green shape as the bug this machine exists to stop. + # The caller skips the PR entirely rather than labelling on facts it did not + # read; blocking on it instead would flap the whole board on one bad call. # The rollup mixes two node types with two different closed enums: CheckRun # carries `conclusion` (CheckConclusionState), StatusContext carries `state` # (StatusState). Rather than list the outcomes that block — the version that @@ -70,6 +77,8 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE # not in consequence: a false FAILURE parks the PR on the agent, who looks; # a false SUCCESS invites a human to merge a tree that will not merge. jq -r ' + if (has("statusCheckRollup") | not) then "UNREADABLE" else + # NEUTRAL and SKIPPED satisfy branch protection — a skipped required check # is not a failed one, and path-filtered jobs skip constantly here. ["SUCCESS", "NEUTRAL", "SKIPPED"] as $passing @@ -121,7 +130,9 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | if ($latest | length) == 0 then "NONE" elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE" elif (($latest - $passing) | length) > 0 then "PENDING" - else "SUCCESS" end' + else "SUCCESS" end + + end' } bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK @@ -414,6 +425,13 @@ main() { GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')" MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")" CHECKS="$(checks_state <<<"$GH_VIEW")" + # Read failed: leave this PR exactly as it is. Recomputing on facts we + # did not read is how an API hiccup turns into a false "merge me" — + # and the next tick is 15 minutes away, not 15 hours. + if [ "$CHECKS" = UNREADABLE ]; then + log "#$n: could not read mergeability/checks — left alone this pass" + exit 0 + fi reconcile_pr "$n" ) || log "#$n: reconcile failed — continuing with the remaining PRs" done diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 863e216..c756fb1 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -258,6 +258,13 @@ ctx_() { jq -n --arg n "$1" --arg s "$2" --arg t "${3:-2026-07-20T15:00:00Z}" \ '{__typename:"StatusContext", context:$n, state:$s, createdAt:$t}'; } expect "no checks at all is NONE" NONE "$(rollup '[]' | checks_state)" +# A failed fetch leaves no rollup KEY; a PR with no checks leaves an empty +# ARRAY. Collapsing the two let an API hiccup read as "nothing is failing" — +# the same unknown-certified-as-green shape as #136, in the one place that +# fix did not look. The caller skips an UNREADABLE PR rather than relabelling. +expect "a failed read is UNREADABLE, not NONE" UNREADABLE "$(echo '{}' | checks_state)" +expect "...and a real empty rollup is still NONE" NONE \ + "$(echo '{"mergeable":"MERGEABLE","statusCheckRollup":[]}' | checks_state)" expect "all green is SUCCESS" SUCCESS \ "$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)" expect "a queued run is PENDING" PENDING \ From 074aea31d88701a93bee41100e7bd9223bae8069 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 17:42:56 +0000 Subject: [PATCH 3/5] docs(changelog): record the UNREADABLE rollup fix Its own Fixed entry rather than a line in the refactor above it: the two-axis split is a design change, this is a correctness bug in a different function, and a reader bisecting a false "merge me" wants the second one named. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 434052b..0cc1f89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,21 @@ which records not just what changed but what each drill run proved. ### Fixed +- **A failed rollup read no longer reads as "nothing is failing"** — when + `gh pr view` returned nothing, the fallback left the `statusCheckRollup` + *key* absent, and `checks_state` collapsed that into the same `NONE` as a PR + that genuinely has no checks. `NONE` blocks nothing, so a transient API + failure presented as mergeable-by-a-human: the same + unknown-certified-as-green shape as #136, surviving in the one place that + fix never looked. + + `checks_state` now distinguishes the two — `UNREADABLE` for an absent key, + `NONE` for a present-but-empty array — and the sweep leaves an `UNREADABLE` + PR exactly as it is rather than relabelling on facts it did not read. It is + deliberately **not** a blocker: blocking on it would flap the entire board + on one bad API call, and the next tick is fifteen minutes away. Fixtures + 64 → 66. + - **`state:needs-human` no longer appears on PRs a human cannot merge** (#136) — `decide_state()` derived state from three inputs (draft flag, requested reviewers, submitted reviews) and read *nothing* about mergeability or From 27cfc5b8c1c8c6cc1799b8868965998273e319ac Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 17:50:00 +0000 Subject: [PATCH 4/5] fix(labels): never name a label the repo lacks; a stale round owes an ask too Round 1 review (claude-bot, codex-bot on #138 and the sibling PRs). gh issue edit rejects the WHOLE call on one unknown label name, and this change made state and blockers share a single edit for anti-flicker. On a repo whose taxonomy predates the blocker:* labels -- every repo, at merge -- the first sweep would therefore converge NOTHING on exactly the PRs the change exists to fix, surfacing only as a warning in a cron log. Adds are now filtered against the repo's real label set, read once per sweep. Removals need no filter: they are built from has_label, so they provably exist. An unreadable label set does not filter, because a failed read must not silently strip the board. blocker:unrequested also missed the stale round. MISSING and STALE are both verdicts this head does not have -- nobody reviewed it, or everybody reviewed something else -- and the agent owes an ask either way. The stale one is the worse of the two: it has approvals on the page that no longer describe the tree, which is the case LABELS.md already called the most dangerous. Fixtures 66 -> 68. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 40 ++++++++++++++++++++++++++--- CHANGELOG.md | 8 +++++- LABELS.md | 4 +-- test/labels-reconcile.sh | 10 ++++++++ 4 files changed, 56 insertions(+), 6 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 89ff1cb..f25b0e2 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -194,12 +194,17 @@ blockers() { # → the blocker:* labels this PR should carry, one per line # explicit human request — a maintainer claiming a PR early is deliberate, # not a dropped ball. if [ "$DRAFT" != true ] && ! requested "$HUMAN"; then - local b any_missing=false any_requested=false + local b v owed=false any_requested=false for b in "${BOTS[@]}"; do requested "$b" && any_requested=true - [ "$(bot_verdict "$b")" = MISSING ] && any_missing=true + # MISSING and STALE are both verdicts this head does not have: nobody + # reviewed it, or everybody reviewed something else. The agent owes an + # ask either way — the stale round is if anything the worse of the two, + # since it has approvals on the page that no longer describe the tree. + v="$(bot_verdict "$b")" + case "$v" in MISSING | STALE) owed=true ;; esac done - if [ "$any_missing" = true ] && [ "$any_requested" = false ]; then + if [ "$owed" = true ] && [ "$any_requested" = false ]; then echo blocker:unrequested fi fi @@ -350,6 +355,30 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch done add="${add#,}" remove="${remove#,}" + + # Never NAME a label the repo does not have. `gh issue edit --add-label` + # rejects the WHOLE call on one unknown name — nothing is applied — so a + # single missing blocker would take the state convergence down with it, on + # exactly the PRs this change exists to fix, surfacing only as a log line. + # Batching state and blockers into one edit for anti-flicker is what widened + # that blast radius; filtering the add side is what closes it again. + # Removals need no filter: they are built from has_label, so the label + # provably exists. REPO_LABELS unreadable means no filtering rather than + # filtering everything out — a failed read must not silently strip the board. + if [ -n "${REPO_LABELS:-}" ]; then + local kept="" missing="" want + for want in ${add//,/ } "$desired"; do + [ "$want" = "$desired" ] && continue + if grep -qxF "$want" <<<"$REPO_LABELS"; then kept="$kept,$want" + else missing="$missing $want"; fi + done + add="${kept#,}" + if ! grep -qxF "$desired" <<<"$REPO_LABELS"; then + log "#$n: WARNING: state label '$desired' does not exist — run the workflow manually to bootstrap" + return + fi + [ -n "$missing" ] && log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap" + fi if ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; then args=(--add-label "$desired${add:+,$add}") [ -n "$remove" ] && args+=(--remove-label "$remove") @@ -404,6 +433,11 @@ main() { bootstrap_labels fi + # The repo's label set, read ONCE per sweep — reconcile_pr filters every + # add against it, because one unknown name fails the whole edit call. + REPO_LABELS="$(gh label list -R "$REPO" --limit 200 --json name --jq '.[].name' 2>/dev/null || echo "")" + [ -z "$REPO_LABELS" ] && log "WARNING: could not read the label set — applying labels unfiltered" + local n for n in $(gh pr list -R "$REPO" --state open --limit 100 --json number --jq '.[].number'); do ( diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cc1f89..b40dc69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,13 @@ which records not just what changed but what each drill run proved. human request — a maintainer claiming a PR early is deliberate. The reconciler strips `state:needs-rebase` on sight, so the retirement heals - the board rather than stranding a label nothing recomputes. Fixtures 51 → 64. + the board rather than stranding a label nothing recomputes. It also never + *names* a label the repo does not have: `gh issue edit` rejects the whole + call on one unknown name, so on a repo whose taxonomy predates this change + an unbootstrapped `blocker:*` would otherwise take the state convergence + down with it, on exactly the PRs the change exists to fix. Adds are filtered + against the repo's real label set and the shortfall is logged. + Fixtures 51 → 66. - **The tenant templates carry rig's family suffix: `claude` → `claude-box`, `codex` → `codex-box`, `grok` → `grok-box`, `staging` → `staging-box`** diff --git a/LABELS.md b/LABELS.md index 1ce4cd4..b70c98e 100644 --- a/LABELS.md +++ b/LABELS.md @@ -16,7 +16,7 @@ single reply, and a human takes the final review. |---|---|---|---|---| | `state:building` | `#FBCA04` | the coding agent, still building | PR opened as draft | marked ready + bot reviews requested | | `state:bots-reviewing` | `#1D76DB` | the reviewer bots to finish the round | ready with reviews requested, or fixes pushed and reviews re-requested | all three bots have reviewed the round | -| `state:addressing` | `#D93F0B` | the coding agent to reply, fix, or ask | all bots reviewed and not all approved; or nobody was asked; or a blocker is up | the thing the blocker names is done | +| `state:addressing` | `#D93F0B` | the coding agent to reply, fix, or ask | all bots reviewed and not all approved; or nobody was asked; or a blocker is up | the round-reply is posted and fixes pushed — and any blocker named alongside is cleared | | `state:needs-human` | `#8250DF` | the human reviewer | the PR **could be merged right now**: no blockers, three formal head-current approvals — and the human review is requested | merged — or changes requested, which cycles back to `state:addressing` | `bots-reviewing` and `addressing` are deliberately distinct: staleness in the @@ -35,7 +35,7 @@ PR carries as many as apply. |---|---|---|---| | `blocker:conflict` | `#B60205` | GitHub says `CONFLICTING` — the agent owes a **rebase** | it merges cleanly | | `blocker:ci-red` | `#B60205` | a check failed — the agent owes a **fix**, which a rebase will not provide | checks are green | -| `blocker:unrequested` | `#E99695` | somebody still owes a verdict and **nobody was asked** for one | reviews are requested | +| `blocker:unrequested` | `#E99695` | this head has no verdict from somebody — never reviewed, or staled by a push — and **nobody was asked** for one | reviews are requested | One rule joins the axes: **`state:needs-human` requires zero blockers.** Any blocker means the work is the agent's, whatever the review round says. diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index c756fb1..5970c08 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -201,7 +201,17 @@ expect "ready, nobody asked, nothing reviewed raises unrequested" blocker:unrequ # ...the partial case is equally stalled: one verdict in, nobody asked for the rest REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" expect "one bot in, none requested is still unrequested" blocker:unrequested "$(blockers)" +# ...a STALE round with nobody asked is the same debt, and arguably worse: the +# page carries approvals that no longer describe the tree. Guarding on +# MISSING alone let this one through with no blocker at all. +REVIEWS_JSON="$(reviews \ + "$(rev "$BOT1" APPROVED oldhead "" t1)" \ + "$(rev "$BOT2" APPROVED oldhead "" t2)" \ + "$(rev "$BOT3" APPROVED oldhead "" t3)")" +expect "a stale round with nobody asked is unrequested too" blocker:unrequested "$(blockers)" +expect "...and is still the agent's ball" state:addressing "$(decide_state)" # ...but a live request means an answer IS coming +REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" REQUESTED="$BOT2" expect "a live bot request is not a stalled round" "" "$(blockers)" # ...and a draft is exempt: the bots ignore drafts by design From f956a85a1c5eec5410a832c0b0fbc6f648ef42ba Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 18:09:07 +0000 Subject: [PATCH 5/5] fix(labels): a missing state label must skip the EDIT, not the whole PR Round 2 review (claude-bot, codex-bot, grok-bot -- all three, independently). The label pre-flight added in round 1 returned out of reconcile_pr entirely. Everything below that point is independent of the state:* taxonomy: clearing a stale merge-next, and the stale sweep. Stranding them meant a merge-next claim reading "merge this one next" survived on a PR the board had moved to the agent -- the same false invitation as #136, one scope smaller -- and the staleness detector went silent. On a cold-start repo, where no state:* label exists yet, that was EVERY PR. It was also a regression against main rather than a missed improvement: the old code failed the edit, logged, and fell through to both blocks. The round-1 fix turned a per-edit failure into a per-PR abort. Now skip_edit=true, and control reaches the rest of the function. Also taken, both from claude-bot and grok-bot: the dead "$desired" term in the filter loop, and `[ -n "$missing" ] && log` becoming a proper elif rather than an &&-as-statement under set -e. Four fixtures now drive reconcile_pr itself, stubbing run/gh -- the first in this suite to reach past the pure functions, which is exactly why a per-PR return was invisible to the fixtures that existed. Fixtures 68 -> 72. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 20 +++++++++++------ CHANGELOG.md | 6 ++++-- test/labels-reconcile.sh | 33 +++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index f25b0e2..917892a 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -365,21 +365,29 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch # Removals need no filter: they are built from has_label, so the label # provably exists. REPO_LABELS unreadable means no filtering rather than # filtering everything out — a failed read must not silently strip the board. + local skip_edit=false if [ -n "${REPO_LABELS:-}" ]; then local kept="" missing="" want - for want in ${add//,/ } "$desired"; do - [ "$want" = "$desired" ] && continue + for want in ${add//,/ }; do if grep -qxF "$want" <<<"$REPO_LABELS"; then kept="$kept,$want" else missing="$missing $want"; fi done add="${kept#,}" + # A missing STATE label skips only the EDIT — never the rest of this + # function. Everything below is independent of the state:* taxonomy, and + # returning here stranded it: `merge-next` kept claiming "merge this one + # next" on a PR the board had moved to the agent, and the stale sweep + # stopped running. That is the original false-invitation bug, reintroduced + # in the very fix meant to survive a cold-start repo — and a regression + # against the old behaviour, which failed the edit and fell through. if ! grep -qxF "$desired" <<<"$REPO_LABELS"; then - log "#$n: WARNING: state label '$desired' does not exist — run the workflow manually to bootstrap" - return + log "#$n: WARNING: state label '$desired' does not exist — skipping the label edit; dispatch the workflow to bootstrap" + skip_edit=true + elif [ -n "$missing" ]; then + log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap" fi - [ -n "$missing" ] && log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap" fi - if ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; then + if [ "$skip_edit" = false ] && { ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; }; then args=(--add-label "$desired${add:+,$add}") [ -n "$remove" ] && args+=(--remove-label "$remove") if run gh issue edit "$n" -R "$REPO" "${args[@]}" >/dev/null; then diff --git a/CHANGELOG.md b/CHANGELOG.md index b40dc69..c7cb3e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,8 +38,10 @@ which records not just what changed but what each drill run proved. call on one unknown name, so on a repo whose taxonomy predates this change an unbootstrapped `blocker:*` would otherwise take the state convergence down with it, on exactly the PRs the change exists to fix. Adds are filtered - against the repo's real label set and the shortfall is logged. - Fixtures 51 → 66. + against the repo's real label set and the shortfall is logged — and a + taxonomy gap skips only the label edit, never the `merge-next` clearing or + the stale sweep, which do not depend on the `state:*` set. + Fixtures 51 → 72. - **The tenant templates carry rig's family suffix: `claude` → `claude-box`, `codex` → `codex-box`, `grok` → `grok-box`, `staging` → `staging-box`** diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 5970c08..cce4691 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -379,5 +379,38 @@ DRAFT=true MERGEABLE=CONFLICTING expect "a draft is building even when conflicted" state:building "$(decide_state)" DRAFT=false MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]' +# --------------------------------------------------------------------------- +# reconcile_pr's cold-start path. Everything above tests pure functions, which +# is exactly why a per-PR `return` in the label pre-flight got through review: +# the fixtures could not reach it. A missing state:* label must skip the label +# EDIT only — merge-next clearing and the stale sweep are independent of the +# taxonomy, and stranding them reintroduced the false-invitation bug (a +# `merge-next` claim surviving on a PR the board had moved to the agent). +# --------------------------------------------------------------------------- +reconcile_probe() { # $1 = REPO_LABELS content → the log lines reconcile_pr emits + ( + REPO_LABELS="$1" REPO=owner/repo NOW="$(date +%s)" + LABELS="merge-next" # the PR carries a queue claim + DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON='[]' + MERGEABLE=MERGEABLE CHECKS=SUCCESS + PR_JSON='{"created_at":"2020-01-01T00:00:00Z"}' + run() { :; } # swallow mutations + gh() { :; } # no network + reconcile_pr 777 2>&1 + ) +} + +cold="$(reconcile_probe "merge-next")" # state:* labels absent entirely +expect "a cold-start repo still clears merge-next" \ + yes "$(grep -q 'cleared merge-next' <<<"$cold" && echo yes || echo no)" +expect "...and still runs the stale sweep" \ + yes "$(grep -q 'stale (' <<<"$cold" && echo yes || echo no)" +expect "...while warning that the state label is missing" \ + yes "$(grep -q "state label 'state:addressing' does not exist" <<<"$cold" && echo yes || echo no)" + +warm="$(reconcile_probe "$(printf 'state:addressing\nmerge-next\nstale\nblocker:unrequested')")" +expect "a bootstrapped repo converges the state as well" \ + yes "$(grep -q 'state -> state:addressing' <<<"$warm" && echo yes || echo no)" + printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ]