From aa5a6baed6c98006e3fea74bdb5a95c708b4b658 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 15:18:49 +0000 Subject: [PATCH 1/5] fix(labels): state:needs-human means a human could merge it right now 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, 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 saying "merge me" over a tree no reviewer had seen. That second shape is the dangerous one, because unlike a conflict nothing on the page contradicts it. The rule the label now keeps: state:needs-human means a human could merge this RIGHT NOW, so anything making that false outranks the request that put it there. CONFLICTING or failing checks -> state:needs-rebase (new; the agent's to fix) approvals staled by a push -> state:addressing (nobody reviewed this tree) 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). That distinction is why the two are handled in different arms rather than collapsed. 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 being fixed. 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 -- precisely the staleness that made needs-human untrustworthy. Both live shapes are pinned in test/labels-reconcile.sh (19 -> 29 fixtures), including that UNKNOWN does not trigger needs-rebase and that a draft outranks a conflict. Proven non-vacuous: dropping the mergeability arm fails 4 assertions, dropping the STALE precedence fails 2, restoring returns 29/0. Closes #136 Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 75 ++++++++++++++++++++++++++--- CHANGELOG.md | 39 +++++++++++++++ LABELS.md | 31 +++++++++++- test/labels-reconcile.sh | 56 +++++++++++++++++++++ 4 files changed, 193 insertions(+), 8 deletions(-) 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 ] From 724f10390842e1559be5738afa45ed90bb6b0328 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:04:16 +0000 Subject: [PATCH 2/5] fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2. Two blockers, both real, both closing the same hole this PR exists to close — a `state:needs-human` that invites a human to merge a tree that will not merge. The check-rollup classifier enumerated the outcomes that block and let the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all read as green. Inverted: it now lists the outcomes that DON'T block — SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything else as blocking. The direction is the point. The rollup mixes two closed enums (CheckRun.conclusion, StatusContext.state) and an outcome the list forgets is one we cannot certify as mergeable; the costs are not symmetric, since a false FAILURE parks the PR on the agent who looks, while a false SUCCESS is #136 exactly. Once CANCELLED blocks, superseded runs must be dropped first: a re-run does not evict the run it replaced, and this PR's own tip carries a CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each context now collapses to its newest entry before anything is judged, keyed on workflow + job name because a bare job name is only unique within its workflow. That preserves the re-run case the panel split over while still blocking a cancelled run that is the newest word. The classifier also moved out of main() into checks_state(). That is why no fixture caught this: it was inline in the fetch loop, so the fixtures could only inject CHECKS= as an already-decided string. Second, decide_state() returned from inside the bot loop on the first MISSING, so a STALE belonging to a later bot in BOTS was never read — a round that was both unfinished and staled came out needs-human over a head nobody had reviewed. The whole round is now collected before precedence is applied to it as a unit, STALE before MISSING. test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome enum, the supersede rule (both orders, plus same name in another workflow), and the mixed round at both ends of BOTS. All verified non-vacuous against the round-1 code. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 84 +++++++++++++++++++++-------- CHANGELOG.md | 23 +++++++- LABELS.md | 2 +- test/labels-reconcile.sh | 70 ++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 24 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 65328f8..152eb7b 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -52,6 +52,45 @@ 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 + # 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; ordering falls back through the timestamps a pending run has. + | [ (.statusCheckRollup // [])[] + | { ctx: [.workflowName // "", .name // .context // ""], + at: (.completedAt // .startedAt // .createdAt // ""), + outcome: ((.conclusion // .state // "") | ascii_upcase) } ] + | group_by(.ctx) | map(sort_by(.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" \ @@ -104,29 +143,37 @@ decide_state() { # → the one state:* label this PR should carry 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="" + 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 - # 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" + 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 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. + # 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 @@ -158,7 +205,7 @@ 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 +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 @@ -271,12 +318,7 @@ main() { # 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")" + 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 c6dc467..76ce04a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,7 +85,25 @@ which records not just what changed but what each drill run proved. 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). + 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 this PR'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`. `UNKNOWN` mergeability is deliberately not treated as unmergeable: GitHub reports it for about a minute after every merge while it recomputes, and @@ -98,7 +116,8 @@ which records not just what changed but what each drill run proved. `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). + shapes, the mixed round, and the whole check-outcome enum are pinned in + `test/labels-reconcile.sh` (19 fixtures → 44). - **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) — `globstar` makes `**` descend into subdirectories, but a glob still does diff --git a/LABELS.md b/LABELS.md index 90ae453..13a39cd 100644 --- a/LABELS.md +++ b/LABELS.md @@ -97,7 +97,7 @@ gh label create "state:building" --color FBCA04 --description "PR is a dra 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 "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 diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index fb17d4a..32b0bf3 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -185,6 +185,21 @@ REVIEWS_JSON="$(reviews \ "$(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 @@ -194,6 +209,61 @@ expect "an unfinished round still yields to an explicit human request" state:nee 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)" + +# -- 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)" From e40d2df5f1f0bc32806ca6ac7d4fdcc6cdd1c7e1 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:18:22 +0000 Subject: [PATCH 3/5] fix(labels): date a check run by the newest stamp it carries, not by its completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3. @claude-bot-andresmgsl and @codex-bot-andresmgsl independently caught that the supersede rule added in round 2 inverted itself on the one shape its fixtures could not express: a re-run still in flight. A running check does not omit `completedAt`. `gh` marshals the Go zero time as the STRING "0001-01-01T00:00:00Z", and jq's `//` only falls through null/false, so the sentinel won the sort key and sorted before every real timestamp. The live re-run became the OLDEST entry in its context, `last` discarded it, and the run it superseded was judged instead — exactly backwards, and wrong in both directions: green + re-run in flight -> SUCCESS (should be PENDING) CANCELLED + re-run in flight -> FAILURE (should be PENDING) The first is #136 restored by the very rule meant to close it: all bots approve, mergeable, state:needs-human — over a tree whose merge button branch protection has disabled. It was also a regression from round 1, which caught it via `any(. == "")`. The second is the re-run flap the supersede rule exists to prevent, narrowed rather than removed. Fixed by taking the newest timestamp a run actually carries and discarding BOTH spellings of absent — null and the zero sentinel — rather than by reordering the fallbacks. An entry with no usable timestamp now sorts LAST rather than first, so an undateable in-flight run is never dropped in favour of a stale success. Every ambiguity resolves toward "not settled". The fixtures could not have caught this: the `run_()` helper sets only `completedAt`, so every supersede fixture was a race between two FINISHED runs. The helper now expresses an in-flight entry, and the four new fixtures assert PENDING over both a green and a cancelled predecessor. 44 -> 48; reverting just the dating expression fails 3 of the 4. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 25 ++++++++++++++++++++++--- CHANGELOG.md | 16 +++++++++++++--- test/labels-reconcile.sh | 25 +++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 6 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 152eb7b..048f62a 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -78,12 +78,31 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE # 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; ordering falls back through the timestamps a pending run has. + # 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: take the newest timestamp a run actually carries, discarding both + # spellings of absent (null, and the zero sentinel). 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 here resolves toward "not settled". | [ (.statusCheckRollup // [])[] | { ctx: [.workflowName // "", .name // .context // ""], - at: (.completedAt // .startedAt // .createdAt // ""), + at: ([.startedAt, .createdAt, .completedAt] + | map(select(type == "string" and . != "" + and (startswith("0001-01-01") | not))) + | max // ""), outcome: ((.conclusion // .state // "") | ascii_upcase) } ] - | group_by(.ctx) | map(sort_by(.at) | last | .outcome) as $latest + | 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" diff --git a/CHANGELOG.md b/CHANGELOG.md index 76ce04a..19980be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,7 +103,16 @@ which records not just what changed but what each drill run proved. context collapsing to its newest entry: a re-run does not evict the run it replaced, so this PR'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`. + stranded every re-run PR in `needs-rebase`. Which entry is newest is decided + on the newest timestamp a run actually carries, because a run still in flight + does not omit its completion — `gh` marshals the Go zero time as the *string* + `"0001-01-01T00:00:00Z"`, which `//` will not fall through. Dating on + completion therefore sorted the live re-run to the bottom and picked the run + it superseded, reporting the old `SUCCESS` while a replacement was still + running: #136 restored, by the very rule meant to close it. An entry carrying + no usable timestamp sorts last rather than first, so an undateable in-flight + run 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 @@ -116,8 +125,9 @@ which records not just what changed but what each drill run proved. `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, the mixed round, and the whole check-outcome enum are pinned in - `test/labels-reconcile.sh` (19 fixtures → 44). + shapes, the mixed round, the whole check-outcome enum, and the in-flight + re-run superseding both a green and a cancelled predecessor are pinned in + `test/labels-reconcile.sh` (19 fixtures → 48). - **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) — `globstar` makes `**` descend into subdirectories, but a glob still does diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 32b0bf3..86be743 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -258,6 +258,31 @@ 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 +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)" + # -- 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 From cbc8f4f7a880bf5741069eaf2e505b19c51ec80a Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:29:59 +0000 Subject: [PATCH 4/5] =?UTF-8?q?test(labels):=20pin=20the=20reverse=20direc?= =?UTF-8?q?tion=20=E2=80=94=20a=20run=20that=20finished=20after=20an=20in-?= =?UTF-8?q?flight=20entry=20settles=20the=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The supersede fixtures pinned "an in-flight re-run is the newer word". Nothing pinned the converse: a run that FINISHED after an earlier in-flight entry is also the newer word, so the context is settled and reads SUCCESS rather than staying PENDING. That is the assertion that catches an over-correction — widening "an undateable entry sorts last" into "an in-flight entry always wins". Verified it fails under exactly that mutation and that nothing else in the suite does. Also corrects the comment above the undateable fixture: that case is guarded by the sort tiebreak, not by the dating expression, so the two changes are now separately pinned. Reverting only the dating expression fails 2 fixtures (the two zero-sentinel ones), not 3. 48 -> 49. Reconciler unchanged. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 6 ++++-- test/labels-reconcile.sh | 10 +++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19980be..f2654c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,8 +126,10 @@ which records not just what changed but what each drill run proved. 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, the mixed round, the whole check-outcome enum, and the in-flight - re-run superseding both a green and a cancelled predecessor are pinned in - `test/labels-reconcile.sh` (19 fixtures → 48). + re-run superseding both a green and a cancelled predecessor — in both + directions, since a run that *finished* after an earlier in-flight entry + settles the context — are pinned in `test/labels-reconcile.sh` + (19 fixtures → 49). - **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) — `globstar` makes `**` descend into subdirectories, but a glob still does diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 86be743..7a532ed 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -277,11 +277,19 @@ expect "a replacement in flight for a CANCELLED run is pending, not failed" PEND "$(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 +# 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 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. From b9527d3cc131774f1b1b9bb9f8ab54e8639d6744 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:41:49 +0000 Subject: [PATCH 5/5] fix(labels): date a check run by when it BEGAN, not by the newest stamp it carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third and final correction to the supersede rule, and the second one that silently restored #136. Round 3 replaced "date by completion" with "date by the newest stamp the run carries". That is still not an ordering on runs: `max` over [startedAt, createdAt, completedAt] resolves to completedAt for a FINISHED run and startedAt for a LIVE one — different quantities, so the comparison was never between like and like. The consequence is the ordinary concurrency-group path, not an edge case. A run cancelled by a concurrency group does not stop instantly; it drains AFTER its replacement has already started, so predecessor.completedAt > successor.startedAt is the normal shape. On this PR's own aa5a6ba the window was 13 seconds. Inside it the dead predecessor out-dated the live run replacing it, and a green predecessor reported SUCCESS while a re-run was still in flight: SUCCESS completing 15:30:13, replacement started 15:30:00 max -> SUCCESS (#136: needs-human over a disabled merge button) first -> PENDING Fixed with `max` -> `first`. The list is already in preference order, so `first` IS "date it by when it began" — and a replacement always begins after the run it replaces, whatever order they finish in. The sentinel filtering and the undateable-sorts-last tiebreak are unchanged; this narrows the rule to a quantity that actually orders. Prescribed independently by claude-bot-andresmgsl and codex-bot-andresmgsl. Two fixtures pin the drain window in both colours. 49 -> 51. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 24 ++++++++++++++------ CHANGELOG.md | 34 +++++++++++++++++++---------- test/labels-reconcile.sh | 19 ++++++++++++++++ 3 files changed, 58 insertions(+), 19 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 048f62a..db00ea3 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -88,18 +88,28 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE # pick the very run it superseded — reporting the old SUCCESS while a # replacement was still running, which is #136 again. # - # So: take the newest timestamp a run actually carries, discarding both - # spellings of absent (null, and the zero sentinel). 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 here resolves toward "not settled". + # 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))) - | max // ""), + | first // ""), outcome: ((.conclusion // .state // "") | ascii_upcase) } ] | group_by(.ctx) | map(sort_by([(.at == ""), .at]) | last | .outcome) as $latest diff --git a/CHANGELOG.md b/CHANGELOG.md index f2654c5..0679af6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,16 +103,25 @@ which records not just what changed but what each drill run proved. context collapsing to its newest entry: a re-run does not evict the run it replaced, so this PR'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 decided - on the newest timestamp a run actually carries, because a run still in flight - does not omit its completion — `gh` marshals the Go zero time as the *string* - `"0001-01-01T00:00:00Z"`, which `//` will not fall through. Dating on - completion therefore sorted the live re-run to the bottom and picked the run - it superseded, reporting the old `SUCCESS` while a replacement was still - running: #136 restored, by the very rule meant to close it. An entry carrying - no usable timestamp sorts last rather than first, so an undateable in-flight - run is never discarded in favour of a stale success — every ambiguity here - resolves toward "not settled". + stranded every re-run PR in `needs-rebase`. + + A run is dated by **when it began**, which took two corrections to get right + and both restored #136 in the meantime. Dating on completion fails because a + run still in flight does not omit its completion — `gh` marshals the Go zero + time as the *string* `"0001-01-01T00:00:00Z"`, which `//` will not fall + through — so the live re-run sorted to the bottom and the run it superseded + was judged instead. Taking the *newest* stamp a run carries fails for a + subtler reason: it resolves to `completedAt` for a finished run and + `startedAt` for a live one, which are different quantities, so it never + ordered runs at all. A run cancelled by the concurrency group drains *after* + its replacement starts — 13 seconds on this PR's own `aa5a6ba` — so the dead + predecessor routinely out-dated the live run replacing it, and a green + predecessor in that window read `SUCCESS` with a re-run still in flight. + Start time has neither failure: a replacement always begins after the run it + replaces, whatever order they finish in. An entry carrying no usable stamp + sorts last rather than first, so an undateable in-flight run is never + discarded in favour of a stale success — every ambiguity 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 @@ -128,8 +137,9 @@ which records not just what changed but what each drill run proved. shapes, the mixed round, the whole check-outcome enum, and the in-flight re-run superseding both a green and a cancelled predecessor — in both directions, since a run that *finished* after an earlier in-flight entry - settles the context — are pinned in `test/labels-reconcile.sh` - (19 fixtures → 49). + settles the context, and across the drain window where the predecessor + completes last — are pinned in `test/labels-reconcile.sh` + (19 fixtures → 51). - **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) — `globstar` makes `**` descend into subdirectories, but a glob still does diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 7a532ed..b68de99 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -291,6 +291,25 @@ 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