diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index c43e1cf..65328f8 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -31,7 +31,7 @@ set -euo pipefail HUMAN="${HUMAN_REVIEWER:-danmt}" BOTS=(claude-bot-andresmgsl codex-bot-andresmgsl grok-bot-andresmgsl) -STATES=(state:building state:bots-reviewing state:addressing state:needs-human) +STATES=(state:building state:needs-rebase state:bots-reviewing state:addressing state:needs-human) STALE_AFTER=$((48 * 3600)) log() { printf 'labels: %s\n' "$*"; } @@ -46,6 +46,8 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing # HEAD_SHA the PR's current head commit # REQUESTED newline-separated logins with a review currently requested # REVIEWS_JSON JSON array of submitted (non-PENDING) reviews +# MERGEABLE MERGEABLE | CONFLICTING | UNKNOWN (GitHub's own verdict) +# CHECKS SUCCESS | FAILURE | PENDING | NONE (the check rollup) # --------------------------------------------------------------------------- requested() { grep -qxF "$1" <<<"$REQUESTED"; } @@ -85,22 +87,52 @@ human_request_needed() { # 0 when needs-human requires a FRESH human request decide_state() { # → the one state:* label this PR should carry if [ "$DRAFT" = true ]; then echo state:building; return; fi - # an explicit human request outranks the bot rounds — it is the final - # gate, and a maintainer pulling a PR to themselves early counts too - if requested "$HUMAN"; then echo state:needs-human; 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 b v verdicts="" for b in "${BOTS[@]}"; do if requested "$b"; then echo state:bots-reviewing; return; fi done for b in "${BOTS[@]}"; do v="$(bot_verdict "$b")" - if [ "$v" = MISSING ]; then echo state:bots-reviewing; return; fi + if [ "$v" = MISSING ]; then + # No verdict at all from this bot. An explicit human request still + # outranks an unfinished bot round — a maintainer pulling a PR to + # themselves early is a deliberate act, and the original precedence. + if requested "$HUMAN"; then echo state:needs-human; return; fi + echo state:bots-reviewing; return + fi verdicts="$verdicts $v" done + case "$verdicts" in + # STALE = a verdict for an older head. Unlike MISSING, this outranks the + # human request: every approval was invalidated by a push, so NOBODY has + # reviewed this tree. Handing that to the human is the #136 case where + # everything reads green — mergeable, CI passing, "waiting on the human" — + # over code no reviewer has seen. The agent owes a re-request. + *STALE*) 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 + if requested "$HUMAN"; then echo state:needs-human; return; fi case "$verdicts" in # FEEDBACK = a comment with no verdict → the agent owes the round-reply. - # STALE = a verdict for an older head → the agent owes a re-request. - *BLOCK* | *FEEDBACK* | *STALE*) echo state:addressing; return ;; + *BLOCK* | *FEEDBACK*) echo state:addressing; return ;; esac # the bots all approve — but if the human's standing word is # changes-requested (and nobody re-requested them yet), the agent owes @@ -125,7 +157,9 @@ 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|All bots approve — waiting on the human reviewer +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 release|0E8A16|Release flow and version/packaging work @@ -174,6 +208,18 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch fi fi + # ---- merge-next: cleared, never set ---------------------------------- + # Queue order is INTENT — which PR should land first is a judgement about + # conflicts and dependencies that GitHub knows nothing about, so the + # reconciler must not guess it (LABELS.md's rule for `blocked`/`release`). + # What it CAN do is stop the label going stale the way needs-human did: + # the moment the PR is no longer the thing a human should merge next, the + # claim is removed. Setting it stays with whoever owns the queue. + if has_label merge-next && [ "$desired" != state:needs-human ]; then + run gh issue edit "$n" -R "$REPO" --remove-label merge-next >/dev/null + log "#$n: cleared merge-next (state is $desired, not mergeable-by-a-human)" + fi + # ---- stale: real activity only, and blocked is legitimately quiet ---- last_activity="$( { @@ -216,6 +262,21 @@ main() { # PENDING reviews are unsubmitted drafts in someone's browser — not a verdict REVIEWS_JSON="$(gh api --paginate "repos/$REPO/pulls/$n/reviews" --jq '.[]' \ | jq -s '[.[] | select(.state != "PENDING")]')" + # mergeability + the check rollup, the two facts the state machine was + # blind to (#136). `gh pr view` rather than the REST PR object: the API's + # `mergeable` is a tri-state boolean that GitHub computes lazily, while + # this returns the same MERGEABLE/CONFLICTING/UNKNOWN string the UI shows. + # Failure to read them is NOT fatal and NOT treated as broken — an API + # hiccup must never flap every PR into needs-rebase, so both degrade to + # the "do not know" value that triggers nothing. + GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')" + MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")" + CHECKS="$(jq -r ' + (.statusCheckRollup // []) as $c + | if ($c | length) == 0 then "NONE" + elif ($c | map(.conclusion // .state // "") | any(. == "FAILURE" or . == "TIMED_OUT" or . == "STARTUP_FAILURE" or . == "ACTION_REQUIRED")) then "FAILURE" + elif ($c | map(.conclusion // .state // "") | any(. == "" or . == "PENDING" or . == "IN_PROGRESS" or . == "QUEUED")) then "PENDING" + else "SUCCESS" end' <<<"$GH_VIEW")" reconcile_pr "$n" ) || log "#$n: reconcile failed — continuing with the remaining PRs" done diff --git a/CHANGELOG.md b/CHANGELOG.md index e5cc1dc..c6dc467 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,45 @@ which records not just what changed but what each drill run proved. ### Fixed +- **`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 + checks. Combined with the `if requested "$HUMAN"` short-circuit at the top of + its precedence, the label was **sticky**: once the maintainer was requested, + the PR read `state:needs-human` through conflicts, through red CI, through a + force-push that staled every approval. Nothing demoted it. + + Observed twice in one afternoon on this repo, in two different shapes. Three + PRs sat at `state:needs-human` while `CONFLICTING` for hours — the board + inviting a merge GitHub had already disabled. And #119, after a rebase, read + `MERGEABLE`, four green checks, `state:needs-human` — with **zero** reviews + bound to its head. Every visible signal said *merge me* over a tree no + reviewer had seen, and unlike the conflict case, nothing on the page + contradicted it. + + The rule the label now keeps is that **`state:needs-human` means a human + could merge this right now**, so anything making that false outranks the + request that put it there. A `CONFLICTING` branch or a failing check is the + agent's to fix: new `state:needs-rebase`. Approvals staled by a push mean + nobody reviewed this tree: `state:addressing`, because the agent owes a + re-request. An *unfinished* round still yields to an explicit human request — + a maintainer pulling a PR to themselves early is deliberate, and `MISSING` + (nobody has reviewed yet) is a different fact from `STALE` (everyone reviewed + something else). + + `UNKNOWN` mergeability is deliberately not treated as unmergeable: 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. A failed read of either fact degrades to the same "do not know" + value for the same reason — an API hiccup must not relabel the board. + + Also adds `merge-next`, because a correct `needs-human` still does not say + *which* PR to merge first, and order matters when they conflict through + `CHANGELOG.md`. Queue order is intent, so the reconciler never sets it — it + only **clears** it the moment the PR stops being mergeable-by-a-human, which + is precisely the staleness that made `needs-human` untrustworthy. Both live + shapes are pinned in `test/labels-reconcile.sh` (19 fixtures → 29). + - **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) — `globstar` makes `**` descend into subdirectories, but a glob still does not *match* a dot-prefixed name, so `**/` never entered `.github/`. The diff --git a/LABELS.md b/LABELS.md index 612b38c..90ae453 100644 --- a/LABELS.md +++ b/LABELS.md @@ -17,12 +17,38 @@ 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-human` | `#8250DF` | the human reviewer | the human review is requested — by the author when the round passes, or automatically on three formal head-current approvals | merged — or changes requested, which cycles back to `state:addressing` | +| `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` | `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. +**`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 +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 +and "waiting on the human" over code no reviewer has seen. + +`UNKNOWN` mergeability is deliberately **not** treated as unmergeable. 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. + +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 +reviewed yet) and `STALE` (everyone reviewed something else) are different +facts and are treated differently. + ## Cross-cutting (PRs and issues) | Label | Color | Meaning | @@ -30,6 +56,7 @@ the ball*. Collapsing them loses exactly the information a sweep needs. | `stale` | `#B60205` | No activity for 48h. Sweep-managed, never hand-applied. `state:building` + `stale` is precisely a forgotten draft. | | `blocked` | `#6A737D` | Waiting on another PR or issue to land first. Quiet *legitimately* — the staleness sweep skips it. | | `release` | `#0E8A16` | Release flow, versioning, and packaging work. | +| `merge-next` | `#0E8A16` | Head of the merge queue — **merge this one next**. Queue order is *intent* (which PR lands first, given how they conflict), so the reconciler never sets it: you or the agent maintaining the queue do. The reconciler only **clears** it, the moment the PR stops being something a human could merge — so it cannot go stale the way `state:needs-human` did. | ## Scope — which surface? (PRs and issues, any number) @@ -69,7 +96,9 @@ 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 "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 gh label create "release" --color 0E8A16 --description "Release flow and version/packaging work" --force diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 10318a2..fb17d4a 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -146,5 +146,61 @@ REQUESTED="$HUMAN" expect "live human request suppresses re-request" not-needed "$(human_request_needed && echo needed || echo not-needed)" REQUESTED="" +# --------------------------------------------------------------------------- +# #136: state:needs-human must mean "a human could merge this RIGHT NOW". +# Both cases below were observed live in this repo on 2026-07-20, and both +# showed state:needs-human while being unmergeable in different ways. +# --------------------------------------------------------------------------- +ALL_APPROVE="$(reviews \ + "$(rev "$BOT1" APPROVED head1 "" t1)" \ + "$(rev "$BOT2" APPROVED head1 "" t2)" \ + "$(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. +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)" +REQUESTED="$HUMAN" +expect "...even with the human explicitly requested" state:needs-rebase "$(decide_state)" + +# -- red CI is the same claim: not something a human should merge. +REQUESTED="" MERGEABLE=MERGEABLE CHECKS=FAILURE +expect "a red PR is needs-rebase" state:needs-rebase "$(decide_state)" +REQUESTED="$HUMAN" +expect "...and a human request does not override red CI" state:needs-rebase "$(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. +REQUESTED="" MERGEABLE=UNKNOWN CHECKS=PENDING +expect "UNKNOWN mergeability does not trigger needs-rebase" state:needs-human "$(decide_state)" + +# -- flavour 2 (the dangerous one): mergeable, green, human requested, and +# NOBODY has reviewed this head. Observed on #119 after a rebase: every +# signal read "merge me" and nothing on the page contradicted it. +MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="$HUMAN" +REVIEWS_JSON="$(reviews \ + "$(rev "$BOT1" APPROVED oldhead "" t1)" \ + "$(rev "$BOT2" APPROVED oldhead "" t2)" \ + "$(rev "$BOT3" APPROVED oldhead "" t3)")" +expect "stale approvals outrank the human request (nobody reviewed this tree)" state:addressing "$(decide_state)" + +# -- but an UNFINISHED round still yields to an explicit human request: a +# maintainer pulling a PR to themselves early is deliberate, and was the +# original precedence. MISSING differs from STALE — nobody has reviewed +# YET, versus everyone reviewed something else. +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)" + +# -- the happy path survives all of the above. +REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" +expect "mergeable + green + three head-current approvals is needs-human" state:needs-human "$(decide_state)" +# -- and a draft outranks everything, including a conflict. +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='[]' + printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ]