diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 0a6c4b1..773ebd3 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,10 +46,80 @@ 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"; } +checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE + # 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 + # shipped in this PR's first round listed four, and ERROR, CANCELLED and + # STALE fell through its `else` into SUCCESS — this lists the outcomes that + # DON'T, and treats everything else as blocking. + # + # That direction is the point. An outcome we do not recognise is one we + # cannot certify as mergeable, and certifying the unrecognised as green is + # the exact shape of #136. The cost of being wrong is symmetric in form and + # 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 ' + # 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 + # "" covers a StatusContext still reported with no state at all. + | ["", "PENDING", "IN_PROGRESS", "QUEUED", "WAITING", "REQUESTED", "EXPECTED"] as $waiting + + # A re-run does not evict the run it superseded — the rollup keeps both. + # This PR proved it: its own tip carried a CANCELLED `scope` (15:19:39) + # beside the SUCCESS `scope` (15:19:45) that replaced it, same workflow. + # Once CANCELLED blocks, judging every entry would strand this very PR in + # needs-rebase forever, so collapse each context to its newest entry first. + # Key on workflow + name because a bare job name is only unique within its + # workflow. + # + # Dating a run is the subtle part, and getting it wrong restores the bug. + # A run still in flight has no completion, but `gh` does not omit the + # field: its Go struct marshals the zero time as "0001-01-01T00:00:00Z", + # which is a string, so `//` will not fall through it. Ordering on + # completion therefore sorted the LIVE re-run to the bottom and let `last` + # pick the very run it superseded — reporting the old SUCCESS while a + # replacement was still running, which is #136 again. + # + # So: date a run by when it BEGAN, discarding both spellings of absent + # (null, and the zero sentinel) and falling back only if it never recorded + # a beginning. NOT by the newest stamp of any kind: `max` compares the + # completion of a finished run against the start of a live one, which are + # different quantities and not an ordering on runs. A run cancelled by the + # concurrency group does not stop the instant its replacement starts — the + # runner has to wind down — so predecessor.completedAt > successor.startedAt + # is the ordinary case, and `max` dated the dead predecessor newer than the + # live run that replaced it, narrowing both failures above without closing + # them. The list is already in preference order, so `first` IS that rule. + # + # An entry that carries no usable timestamp at all sorts LAST rather than + # first — something we cannot date is most likely the thing just created, + # and treating it as newest keeps an undateable in-flight run from being + # discarded in favour of a stale success. Every ambiguity resolves toward + # "not settled". + | [ (.statusCheckRollup // [])[] + | { ctx: [.workflowName // "", .name // .context // ""], + at: ([.startedAt, .createdAt, .completedAt] + | map(select(type == "string" and . != "" + and (startswith("0001-01-01") | not))) + | first // ""), + outcome: ((.conclusion // .state // "") | ascii_upcase) } ] + | group_by(.ctx) + | map(sort_by([(.at == ""), .at]) | last | .outcome) as $latest + + | if ($latest | length) == 0 then "NONE" + elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE" + elif (($latest - $passing) | length) > 0 then "PENDING" + else "SUCCESS" end' +} + bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK local review state commit review="$(jq -c --arg u "$1" \ @@ -85,22 +155,60 @@ 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 - local b v verdicts="" + + # 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 verdicts="" for b in "${BOTS[@]}"; do if requested "$b"; then echo state:bots-reviewing; return; fi done + # Collect the WHOLE round before applying any precedence. Deciding inside + # the loop let BOTS order pick the winner: a MISSING returned immediately, + # so a STALE belonging to a later bot was never even read, and the mixed + # round (one approval staled by a push, another bot yet to review) came out + # needs-human — the #136 headline shape, with zero reviews bound to the head. for b in "${BOTS[@]}"; do - v="$(bot_verdict "$b")" - if [ "$v" = MISSING ]; then echo state:bots-reviewing; return; fi - verdicts="$verdicts $v" + verdicts="$verdicts $(bot_verdict "$b")" done + case "$verdicts" in + # STALE = a verdict for an older head. Unlike MISSING, this outranks the + # human request: every approval it covers 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. + # Checked before MISSING because "unfinished" must not swallow "and also + # stale": a round that is both is a push that outran the re-requests, not + # a maintainer deliberately claiming the PR early. + *STALE*) echo state:addressing; return ;; + esac + case "$verdicts" in + # 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. + *MISSING*) + if requested "$HUMAN"; then echo state:needs-human; return; fi + echo state:bots-reviewing; 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 +233,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-human|8250DF|All bots approve — waiting on the human reviewer +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 +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 +284,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 +338,16 @@ 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="$(checks_state <<<"$GH_VIEW")" reconcile_pr "$n" ) || log "#$n: reconcile failed — continuing with the remaining PRs" done diff --git a/CHANGELOG.md b/CHANGELOG.md index 359ee07..2075c5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,91 @@ on the way to cutting its first release, and this file starts there. ## Unreleased +### Fixed + +- **`state:needs-human` no longer appears on PRs a human cannot merge** + (#87, heavy-duty/box#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. + + This repo paid for it directly. During the ten-PR batch merged on 2026-07-20, + every merge re-conflicted the PRs below it through `CHANGELOG.md` — and each + one kept its `state:needs-human` label the whole time, inviting a merge that + could not happen. It was noticed only by opening them one at a time, which is + the exact work the label exists to save. + + 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). Precedence is applied to the round as a whole, after every + verdict is collected: deciding inside the loop let the order of `BOTS` pick + the answer, so a round that was *both* unfinished and staled returned on the + `MISSING` before any later bot's `STALE` was read — and came out + `needs-human` over a head nobody had reviewed, the original bug wearing a + different hat. + + Whether a check blocks is judged by listing the outcomes that *don't* — + `SUCCESS`, `NEUTRAL`, `SKIPPED`, and the pending set — rather than the + outcomes that do. The rollup mixes two closed enums (`CheckRun.conclusion` + and `StatusContext.state`), and an outcome the list forgets is one the label + cannot certify as mergeable: `ERROR`, `CANCELLED` and `STALE` all read as + green under an allow-list of failures. The costs are not symmetric — 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. Superseded runs are dropped first, each + context collapsing to its newest entry: a re-run does not evict the run it + replaced, so box#137's own tip carried a `CANCELLED` `scope` beside the + `SUCCESS` `scope` that superseded it, and judging every entry would have + stranded every re-run PR in `needs-rebase`. + + Which entry is newest is dated by **when the run began** — `startedAt`, + falling back only if it never recorded one — discarding *both* spellings of + absent. A run still in flight has no completion, but `gh` does not omit the + field: its Go struct marshals the zero time as the string + `0001-01-01T00:00:00Z`, and `//` falls through `null` and `false` only. + Dating by completion therefore sorted the *live* re-run to the bottom and + let `last` pick the very run it superseded — a green context with a + replacement mid-flight read `SUCCESS`, inviting a merge the button had + already disabled, which is #136 restored by the fix for it. + + Taking the *newest* stamp each run carries is not a fix either, and this is + the subtle part: it compares a finished predecessor by when it **ended** + against a live successor by when it **began**, which is not an ordering on + runs at all. A run cancelled by the concurrency group does not stop the + instant its replacement starts — the runner has to receive the signal and + wind down — so the predecessor completing *after* the successor started is + the ordinary case, 13s wide on the box#137 tip that motivated the supersede + rule. For that whole drain window the dying predecessor out-dated its own + replacement. One consistent quantity, start time, is the only ordering that + holds. An entry carrying no usable timestamp at all sorts last rather than + first, so something that cannot be dated is never discarded in favour of a + stale success. Every ambiguity here resolves toward "not settled". + + `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. + + Also adds `merge-next`, because a correct `needs-human` still does not say + *which* PR to merge first, and order matters when they conflict. Queue order + is intent, so the reconciler never sets it — it only **clears** it once the + PR stops being mergeable-by-a-human, which is precisely the staleness that + made `needs-human` untrustworthy. Both live shapes, the mixed round, the + whole check-outcome enum, and the re-run in every temporal arrangement it + occurs in — superseded, still in flight in both spellings of an absent + completion, undateable, and still draining past its replacement's start — + are pinned in `test/labels-reconcile.sh`. Ported from heavy-duty/box#137 so + the three repos' reconcilers stay byte-identical; fixtures 19 → 51. + ### Added - **`rig platform` — what is this machine, calculated at run time, stored diff --git a/LABELS.md b/LABELS.md index 913a9ef..9527f6d 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-human" --color 8250DF --description "All bots approve — waiting on the human reviewer" --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 "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..b68de99 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -146,5 +146,183 @@ 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)" + +# -- ...and a round that is BOTH unfinished and staled is still the agent's. +# Deciding inside the bot loop made this depend on BOTS order: the MISSING +# returned before any later bot's STALE was read, so the mixed round came +# out needs-human with nothing bound to the head. Pinned at both ends of +# the array, because the whole failure was one of ordering. +MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="$HUMAN" +REVIEWS_JSON="$(reviews \ + "$(rev "$BOT1" APPROVED oldhead "" t1)" \ + "$(rev "$BOT2" APPROVED oldhead "" t2)")" +expect "stale approvals + a bot yet to review is addressing, not needs-human" \ + state:addressing "$(decide_state)" +REVIEWS_JSON="$(reviews "$(rev "$BOT3" APPROVED oldhead "" t3)")" +expect "...and the same when the stale verdict is the LAST bot in BOTS" \ + 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)" + +# --------------------------------------------------------------------------- +# checks_state: the rollup classifier. It lived inline in main() for the first +# round of this PR, which is why nothing here caught it calling ERROR, +# CANCELLED and STALE green. Extracted so the enum can be pinned down. +# --------------------------------------------------------------------------- +rollup() { jq -n --argjson c "$1" '{statusCheckRollup: $c}'; } +run_() { jq -n --arg n "$1" --arg o "$2" --arg t "${3:-2026-07-20T15:00:00Z}" \ + '{__typename:"CheckRun", workflowName:"ci", name:$n, conclusion:$o, completedAt:$t}'; } +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)" +expect "all green is SUCCESS" SUCCESS \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)" +expect "a queued run is PENDING" PENDING \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b QUEUED)]" | checks_state)" +expect "a plain failure is FAILURE" FAILURE \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b FAILURE)]" | checks_state)" + +# -- the round-1 gap: outcomes that are neither success nor pending, and that +# leave a required check unsatisfied. All three reached the old `else`. +expect "a commit status ERROR blocks" FAILURE \ + "$(rollup "[$(run_ a SUCCESS),$(ctx_ lint ERROR)]" | checks_state)" +expect "a CANCELLED run blocks" FAILURE \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b CANCELLED)]" | checks_state)" +expect "a STALE run blocks" FAILURE \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b STALE)]" | checks_state)" +expect "an outcome the enum does not know blocks, it does not pass" FAILURE \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b SOME_FUTURE_STATE)]" | checks_state)" + +# -- NEUTRAL and SKIPPED satisfy branch protection; path-filtered jobs skip +# constantly, and calling that red would park every PR on the agent. +expect "NEUTRAL and SKIPPED are not failures" SUCCESS \ + "$(rollup "[$(run_ a SUCCESS),$(run_ b NEUTRAL),$(run_ c SKIPPED)]" | checks_state)" + +# -- latest-wins. The rollup keeps superseded runs, so this PR's own tip +# carried a CANCELLED `scope` beside the SUCCESS `scope` that replaced it. +# Without collapsing, making CANCELLED block would strand it forever. +expect "a re-run supersedes the cancelled original" SUCCESS \ + "$(rollup "[$(run_ scope CANCELLED 2026-07-20T15:19:39Z),\ + $(run_ scope SUCCESS 2026-07-20T15:19:45Z)]" | checks_state)" +expect "...and the reverse order is not a re-run passing, it is one failing" FAILURE \ + "$(rollup "[$(run_ scope SUCCESS 2026-07-20T15:19:39Z),\ + $(run_ scope CANCELLED 2026-07-20T15:19:45Z)]" | checks_state)" +# same job name in a different workflow is a different context, not a re-run +expect "same name in another workflow does not supersede" FAILURE \ + "$(rollup "[$(jq -n '{__typename:"CheckRun",workflowName:"labels",name:"scope",conclusion:"FAILURE",completedAt:"2026-07-20T15:00:00Z"}'),\ + $(run_ scope SUCCESS 2026-07-20T15:19:45Z)]" | checks_state)" + +# -- a run still IN FLIGHT. `run_()` cannot express this: it always carries a +# real completedAt, which is exactly why the supersede rule shipped dating +# runs by completion and nothing caught it. Both spellings of "no +# completion" are pinned, because `gh` emits the zero sentinel (a string, +# which `//` does not fall through) while the API emits null. +inflight_() { jq -n --arg n "$1" --arg t "$2" --arg c "${3:-0001-01-01T00:00:00Z}" \ + '{__typename:"CheckRun", workflowName:"ci", name:$n, status:"IN_PROGRESS", + conclusion:"", startedAt:$t, completedAt:(if $c == "null" then null else $c end)}'; } + +expect "a re-run in flight beats the success it superseded (zero sentinel)" PENDING \ + "$(rollup "[$(run_ build SUCCESS 2026-07-20T15:00:00Z),\ + $(inflight_ build 2026-07-20T15:10:00Z)]" | checks_state)" +expect "...and the same when the absent completion is null" PENDING \ + "$(rollup "[$(run_ build SUCCESS 2026-07-20T15:00:00Z),\ + $(inflight_ build 2026-07-20T15:10:00Z null)]" | checks_state)" +expect "a replacement in flight for a CANCELLED run is pending, not failed" PENDING \ + "$(rollup "[$(run_ build CANCELLED 2026-07-20T15:00:00Z),\ + $(inflight_ build 2026-07-20T15:10:00Z)]" | checks_state)" +# an entry carrying no usable timestamp is treated as newest, not oldest — +# ambiguity resolves toward "not settled" rather than toward a stale success. +# Guarded by the sort tiebreak rather than the dating expression: reverting +# only `at:` leaves this passing, so the two changes are separately pinned. +expect "an undateable in-flight run is not discarded for a stale success" PENDING \ + "$(rollup "[$(run_ build SUCCESS 2026-07-20T15:00:00Z),\ + $(jq -n '{__typename:"CheckRun",workflowName:"ci",name:"build",conclusion:"",startedAt:null,completedAt:null}')]" \ + | checks_state)" +# ...and the reverse direction, which stops "in flight sorts last" being +# widened into "in flight always wins": a run that FINISHED after an earlier +# in-flight entry is the newer word, and the context is settled. +expect "a finished re-run supersedes an earlier in-flight run" SUCCESS \ + "$(rollup "[$(inflight_ build 2026-07-20T15:19:00Z),\ + $(run_ build SUCCESS 2026-07-20T15:19:45Z)]" | checks_state)" + +# -- the wind-down window. A predecessor cancelled by the concurrency group +# does not stop the instant its replacement starts, so its completion +# routinely lands AFTER the successor's start — on box's aa5a6ba the +# replacement started 15:19:38 and the run it cancelled finished 15:19:51. +# Dating by "newest stamp of any kind" compares the dead run's completion +# against the live run's start, which is not an ordering on runs, and the +# predecessor wins. Every fixture above spaces completion before start, so +# none of them can see it. run_() cannot express the overlap either — it +# carries no startedAt — hence the explicit payloads. +overlap_() { jq -n --arg n "$1" --arg o "$2" --arg s "$3" --arg c "$4" \ + '{__typename:"CheckRun", workflowName:"ci", name:$n, conclusion:$o, + startedAt:$s, completedAt:$c}'; } +expect "a predecessor finishing after its replacement started is still older (CANCELLED)" PENDING \ + "$(rollup "[$(overlap_ scope CANCELLED 2026-07-20T15:19:00Z 2026-07-20T15:19:51Z),\ + $(inflight_ scope 2026-07-20T15:19:38Z)]" | checks_state)" +expect "...and the same when it finished green — mid-flight is not mergeable" PENDING \ + "$(rollup "[$(overlap_ build SUCCESS 2026-07-20T15:19:00Z 2026-07-20T15:19:51Z),\ + $(inflight_ build 2026-07-20T15:19:38Z)]" | checks_state)" + +# -- the classifier feeds the state machine: a cancelled required check must +# 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)" + +# -- 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 ]