From ef798f302c00081a7630bdad0b3995bae115ed66 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 15:26:54 +0000 Subject: [PATCH 1/5] fix(labels): state:needs-human means a human could merge it right now Ported from heavy-duty/box#137 (heavy-duty/box#136) so the three repos' reconcilers stay byte-identical. The state machine here was byte-identical to box's before this change, and remains so after -- only the scope:* taxonomy differs, correctly. decide_state() derived state from three inputs -- draft flag, requested reviewers, submitted reviews -- and read NOTHING about mergeability or checks. With the `if requested "$HUMAN"` short-circuit at the top of its precedence, the label was sticky: once the maintainer was requested, a PR read state:needs-human through conflicts, through red CI, through a force-push that staled every approval. This repo paid for it directly. During the ten-PR batch merged today, every merge re-conflicted the PRs below it through CHANGELOG.md, and each kept its state:needs-human label throughout -- inviting merges that could not happen. It was caught only by opening them one at a time, which is the work the label exists to save. 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 -- MISSING (nobody has reviewed yet) is a different fact from STALE (everyone reviewed something else). UNKNOWN mergeability is NOT treated as unmergeable: GitHub reports it for about a minute after every merge, and flapping every open PR through needs-rebase on each merge would be worse than the bug. A failed read degrades to the same "do not know" value. Also adds merge-next: queue order is intent, so the reconciler never sets it, only CLEARS it once the PR stops being mergeable-by-a-human. Fixtures 19 -> 29, including that UNKNOWN does not trigger needs-rebase and a draft outranks a conflict. No live dry-run evidence here -- this repo has no open PRs right now -- so the fixtures and box's live dry-run are the proof. Closes #87 Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 75 ++++++++++++++++++++++++++--- CHANGELOG.md | 40 +++++++++++++++ LABELS.md | 31 +++++++++++- test/labels-reconcile.sh | 56 +++++++++++++++++++++ 4 files changed, 194 insertions(+), 8 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 0a6c4b1..0912e4b 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 359ee07..c617a8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,46 @@ 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). + + `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. Ported from heavy-duty/box#137 so the three + repos' reconcilers stay byte-identical; fixtures 19 → 29. + ### Added - **`rig platform` — what is this machine, calculated at run time, stored diff --git a/LABELS.md b/LABELS.md index 913a9ef..4871f8e 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 a0951eacf8ecb603c73ee7f25c9ec630a999c7d7 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:05:39 +0000 Subject: [PATCH 2/5] fix(labels): unrecognised check outcomes block, and STALE outranks MISSING MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 review found two ways the "a human could merge this right now" invariant still leaked, both of which let state:needs-human land on a PR the button would refuse. The check-rollup classifier enumerated the outcomes that block and defaulted the rest to SUCCESS, so ERROR, CANCELLED and STALE fell through into green. Inverted to an allow-list of the outcomes that DON'T block (SUCCESS, NEUTRAL, SKIPPED, plus the pending set); everything else, including an outcome neither enum has today, blocks. The rollup mixes CheckRun.conclusion with StatusContext.state and an outcome the list forgets is one we cannot certify as mergeable — a false FAILURE parks the PR on the agent, a false SUCCESS invites a bad merge. The classifier also moved out of main() into checks_state(), which is why no fixture caught this: it was inline in the fetch loop and the jq itself was untestable. Once CANCELLED blocks, superseded runs must be dropped first — a re-run does not evict the run it replaced, and judging every entry would strand every re-run PR in needs-rebase. Each context now collapses to its newest entry, keyed on workflow + job name because a bare job name is only unique within its workflow. 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 with nothing bound to the head. The whole round is now collected before any precedence is applied, STALE ahead of MISSING. The MISSING-yields-to-an-explicit-human-request rule is untouched. Fixtures 29 -> 44, pinning the whole check-outcome enum, the supersede rule in both orders, and the mixed round at both ends of BOTS. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 84 +++++++++++++++++++++-------- CHANGELOG.md | 26 +++++++-- LABELS.md | 2 +- test/labels-reconcile.sh | 70 ++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 25 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 0912e4b..013a5f5 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 c617a8b..5bb8565 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,25 @@ on the way to cutting its first release, and this file starts there. 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 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`. `UNKNOWN` mergeability is deliberately not treated as unmergeable: GitHub reports it for about a minute after every merge while it recomputes, and @@ -43,8 +61,10 @@ on the way to cutting its first release, and this file starts there. *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. Ported from heavy-duty/box#137 so the three - repos' reconcilers stay byte-identical; fixtures 19 → 29. + made `needs-human` untrustworthy. Both live shapes, the mixed round, and the + whole check-outcome enum are pinned in `test/labels-reconcile.sh`. Ported from + heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; + fixtures 19 → 44. ### Added diff --git a/LABELS.md b/LABELS.md index 4871f8e..9527f6d 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 800571fdfbf72c57c0271581683d2b4df2a6fb37 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:18:50 +0000 Subject: [PATCH 3/5] fix(labels): date a check run by when it started, not by a zero completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The supersede collapse added in the previous commit dated each run by `.completedAt // .startedAt // .createdAt`. 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 jq's `//` falls through null and false only. The sentinel was therefore taken as the sort key, and it sorts before every real timestamp — so the LIVE re-run became the oldest entry in its context, `last` discarded it, and the run it superseded was judged instead. That restored #136 through the fix for it: a green context with a replacement mid-flight reported SUCCESS, so a PR read mergeable, green, all bots approve — state:needs-human — while branch protection had the merge button disabled. It also narrowed rather than removed the flap the supersede rule exists to prevent: between "run A cancelled by the concurrency group" and "run B finishes", the PR reported FAILURE and the agent was sent to fix something that was not broken. Runs are now dated by the newest timestamp they actually carry, with both spellings of absent discarded (null, and the zero sentinel). Entries that carry no usable timestamp sort 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". Found independently by claude-bot-andresmgsl and codex-bot-andresmgsl. The fixtures could not have caught it: `run_()` always emits a real completedAt, so every supersede fixture was a race between two finished runs, and the bug lived in the one shape the helper could not express. New `inflight_()` helper covers it; fixtures 44 -> 48. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 25 ++++++++++++++++++++++--- CHANGELOG.md | 21 +++++++++++++++++---- test/labels-reconcile.sh | 25 +++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 7 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 013a5f5..0c23879 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 5bb8565..e290b1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,18 @@ on the way to cutting its first release, and this file starts there. `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 the newest timestamp a run actually + carries, 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 — #136 restored by the fix for it. An entry + carrying no usable timestamp 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 @@ -61,10 +73,11 @@ on the way to cutting its first release, and this file starts there. *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, and the - whole check-outcome enum are pinned in `test/labels-reconcile.sh`. Ported from - heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; - fixtures 19 → 44. + made `needs-human` untrustworthy. Both live shapes, the mixed round, the + whole check-outcome enum, and the superseded and still-in-flight re-run — + in both spellings of an absent completion — are pinned in + `test/labels-reconcile.sh`. Ported from heavy-duty/box#137 so the three + repos' reconcilers stay byte-identical; fixtures 19 → 48. ### Added 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 57e37363f404b40a5096293e5b640ca8dfb53958 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:32:53 +0000 Subject: [PATCH 4/5] =?UTF-8?q?fix(labels):=20order=20check=20runs=20by=20?= =?UTF-8?q?one=20consistent=20quantity=20=E2=80=94=20when=20they=20began?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dating expression took the newest stamp each run carries. That reads `completedAt` for a finished run and `startedAt` for a live one, so the comparison comes down to "when this one ended" against "when that one 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 `predecessor.completedAt > successor.startedAt` is the ordinary case, not a corner. On the box#137 tip that motivated the supersede rule the window was 13s wide — the superseding run started 15:19:38, the run it cancelled did not finish until 15:19:51 — and for that whole window the dying predecessor out-dated its own live replacement, so `last` discarded the replacement and judged the corpse. Both round-3 failure modes came back inside that window, narrowed rather than closed: a draining CANCELLED predecessor reported FAILURE and sent the agent to fix nothing, and a draining SUCCESS predecessor reported SUCCESS — mergeable, all bots approve, state:needs-human — over a tree whose merge button branch protection had already disabled. #136 again, one field over. Dated by `first` of the preference-ordered stamps rather than `max` of them: start time if the run recorded one, falling back only if it did not. The sentinel filtering is unchanged, and finished runs still date by completion when that is all they carry, so the supersede rule keeps the case it exists for. Found independently by claude-bot-andresmgsl and codex-bot-andresmgsl. No existing fixture could express it — `run_()` carries no startedAt, so every supersede fixture spaced the predecessor's completion safely before the successor's start, the same blind spot as round 3 one field over. New `drained_()` helper pins both directions; fixtures 48 -> 51 (with the reverse-direction in-flight fixture ported from cast#128). Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 24 ++++++++++++----- CHANGELOG.md | 42 ++++++++++++++++++----------- test/labels-reconcile.sh | 32 +++++++++++++++++++++- 3 files changed, 75 insertions(+), 23 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 0c23879..f2e4dbe 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 fall back only if it never recorded a + # beginning. Deliberately `first` over the preference-ordered list and not + # `max` of it: max mixes "when it started" with "when it ended", which are + # different quantities, so it 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 predecessor.completedAt > successor.startedAt is the ordinary case + # (13s on the box#137 tip), and under max the dying predecessor + # out-dated its live replacement for the whole drain window. + # + # 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: ([.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 e290b1d..2075c5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,17 +51,28 @@ on the way to cutting its first release, and this file starts there. `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 the newest timestamp a run actually - carries, 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 — #136 restored by the fix for it. An entry - carrying no usable timestamp 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". + 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 @@ -74,10 +85,11 @@ on the way to cutting its first release, and this file starts there. 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 superseded and still-in-flight re-run — - in both spellings of an absent completion — are pinned in - `test/labels-reconcile.sh`. Ported from heavy-duty/box#137 so the three - repos' reconcilers stay byte-identical; fixtures 19 → 48. + 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 diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 86be743..eea4561 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -277,11 +277,41 @@ 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 DRAIN WINDOW. 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's completion lands AFTER the successor's +# start. On box#137's own tip that window was 13s (superseding run started +# 15:19:38, the run it cancelled finished 15:19:51). `run_()` cannot express +# it either — it carries no startedAt — so every fixture above spaces the +# predecessor's completion safely before the successor's start, and the whole +# window is invisible to them. This is why the run is dated by `first` of the +# preference-ordered stamps and not by `max` of them: max compares "when it +# ended" against "when it began", which is not an ordering on runs, and the +# dying predecessor out-dated its live replacement for the entire window. +drained_() { jq -n --arg n "$1" --arg c "$2" --arg s "$3" --arg e "$4" \ + '{__typename:"CheckRun", workflowName:"ci", name:$n, conclusion:$c, + startedAt:$s, completedAt:$e}'; } + +expect "a predecessor still draining does not out-date its live replacement" PENDING \ + "$(rollup "[$(drained_ build CANCELLED 2026-07-20T15:19:29Z 2026-07-20T15:19:51Z),\ + $(inflight_ build 2026-07-20T15:19:38Z)]" | checks_state)" +expect "...and the same when the draining predecessor is green (the #136 shape)" PENDING \ + "$(rollup "[$(drained_ build SUCCESS 2026-07-20T15:19:29Z 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. From 108a944e1c8016890c9ebc798f693d32873c0b61 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:44:19 +0000 Subject: [PATCH 5/5] chore(labels): converge reconciler and fixtures on the canonical text box, rig and cast carry the same reconciler, and the three had drifted to functionally identical but textually different versions of checks_state after the same fix was reached independently in each repo. Comment prose and one fixture helper name differed, so the files were no longer byte-identical and the convergence check had nothing to compare. Standardised on the cast wording for checks_state and on the canonical test/labels-reconcile.sh, which carries the same 51 fixtures under overlap_() where this repo had named the helper drained_(). No behaviour change: the diff to the reconciler is comments only, and the suite is unchanged at 51 passed, 0 failed. Only this repo's own scope:* rows are preserved as local content. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 22 ++++++++--------- test/labels-reconcile.sh | 37 +++++++++++++---------------- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index f2e4dbe..773ebd3 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -89,21 +89,21 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE # 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 fall back only if it never recorded a - # beginning. Deliberately `first` over the preference-ordered list and not - # `max` of it: max mixes "when it started" with "when it ended", which are - # different quantities, so it 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 predecessor.completedAt > successor.startedAt is the ordinary case - # (13s on the box#137 tip), and under max the dying predecessor - # out-dated its live replacement for the whole drain window. + # (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 here resolves - # toward "not settled". + # discarded in favour of a stale success. Every ambiguity resolves toward + # "not settled". | [ (.statusCheckRollup // [])[] | { ctx: [.workflowName // "", .name // .context // ""], at: ([.startedAt, .createdAt, .completedAt] diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index eea4561..b68de99 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -291,26 +291,23 @@ 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 DRAIN WINDOW. 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's completion lands AFTER the successor's -# start. On box#137's own tip that window was 13s (superseding run started -# 15:19:38, the run it cancelled finished 15:19:51). `run_()` cannot express -# it either — it carries no startedAt — so every fixture above spaces the -# predecessor's completion safely before the successor's start, and the whole -# window is invisible to them. This is why the run is dated by `first` of the -# preference-ordered stamps and not by `max` of them: max compares "when it -# ended" against "when it began", which is not an ordering on runs, and the -# dying predecessor out-dated its live replacement for the entire window. -drained_() { jq -n --arg n "$1" --arg c "$2" --arg s "$3" --arg e "$4" \ - '{__typename:"CheckRun", workflowName:"ci", name:$n, conclusion:$c, - startedAt:$s, completedAt:$e}'; } - -expect "a predecessor still draining does not out-date its live replacement" PENDING \ - "$(rollup "[$(drained_ build CANCELLED 2026-07-20T15:19:29Z 2026-07-20T15:19:51Z),\ - $(inflight_ build 2026-07-20T15:19:38Z)]" | checks_state)" -expect "...and the same when the draining predecessor is green (the #136 shape)" PENDING \ - "$(rollup "[$(drained_ build SUCCESS 2026-07-20T15:19:29Z 2026-07-20T15:19:51Z),\ +# -- 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