From b063e6bc428da1715a6e33fd336c3b012ccb30ad Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 15:28:50 +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. In this repo the SECOND half is the live one: three PRs sit at state:needs-human simultaneously with nothing saying which to merge first, and they will conflict through CHANGELOG.md the moment one lands. The stickiness has not bitten here yet only because nothing has conflicted -- the code carried it identically, so the first merge would have reproduced box's situation. 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 -- the label this repo needs most today, since a correct needs-human still does not say which of three ready PRs to merge first. Queue order is intent, so the reconciler never sets it, only CLEARS it. Fixtures 19 -> 29. DRY_RUN against this repo changes NOTHING, which is the correct result: every open PR here is currently mergeable, so the new precedence is a no-op on a healthy board and fires only when something is actually wrong. npm test 623 passed. Closes #127 Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 75 ++++++++++++++++++++++++++--- CHANGELOG.md | 41 ++++++++++++++++ LABELS.md | 31 +++++++++++- test/labels-reconcile.sh | 56 +++++++++++++++++++++ 4 files changed, 195 insertions(+), 8 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index d301e74..fb2fd36 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 09de761..302cb1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,47 @@ actually cutting it, and this file starts there. ## Unreleased +### Fixed + +- **`state:needs-human` no longer appears on PRs a human cannot merge** + (#127, 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. + + In this repo the *second* half is the live one: three PRs currently sit at + `state:needs-human` simultaneously, with nothing saying which to merge first + — and they will conflict through `CHANGELOG.md` the moment one lands. The + stickiness has not bitten here yet only because nothing has conflicted; the + code carried it identically, so the first merge would have reproduced box's + situation exactly. + + 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` — the label this repo needs most today, since a + correct `needs-human` still does not say *which* of three ready PRs to merge + first. Queue order is intent, so the reconciler never sets it; it only + **clears** it once the PR stops being mergeable-by-a-human. Ported from + heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; + fixtures 19 → 29. + ## 0.1.1 — 2026-07-19 ### Fixed diff --git a/LABELS.md b/LABELS.md index a6401b9..3eb4f39 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 ] -- 2.45.2 From d8f73f446d96a3cb105933e16607589ba977339e Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:05:17 +0000 Subject: [PATCH 2/5] fix(labels): unknown check outcomes and mixed rounds must not read green MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of #128. Two blockers from the bot panel, both real holes in the invariant this PR exists to establish. The check-rollup classifier enumerated the outcomes that block and defaulted everything else to SUCCESS, so ERROR, CANCELLED and STALE fell through to green. Inverted to an allow-list of the outcomes that do NOT block — SUCCESS, NEUTRAL, SKIPPED and the pending set — with everything else blocking. The rollup mixes two closed enums (CheckRun.conclusion and StatusContext.state) and the costs are asymmetric: 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 keyed on workflow + job name, so a re-run does not strand its own PR in needs-rebase. 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 could only ever be injected pre-decided. 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, and a round that was both unfinished and staled came out needs-human over a head nobody had reviewed — the original bug wearing a different hat. The whole round is now collected before any precedence is applied, STALE checked before MISSING. The MISSING-yields-to-an-explicit-human-request rule is untouched. Fixtures 29 -> 44, pinning the check-outcome enum, the supersede rule at both orderings, and the mixed round at both ends of BOTS. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 84 +++++++++++++++++++++-------- CHANGELOG.md | 27 ++++++++-- LABELS.md | 2 +- test/labels-reconcile.sh | 70 ++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 25 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index fb2fd36..c22ec2e 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 302cb1d..0f84aa9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,27 @@ actually cutting it, 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, and the rollup keeps both. That shape is live on this board — this + PR's own tip carried two `scope` and two `reconcile` entries — and on + heavy-duty/box#137's tip the superseded half was `CANCELLED`, so once + `CANCELLED` blocks, judging every entry rather than the newest would strand + 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 @@ -45,8 +65,9 @@ actually cutting it, and this file starts there. correct `needs-human` still does not say *which* of three ready PRs to merge first. Queue order is intent, so the reconciler never sets it; it only **clears** it once the PR stops being mergeable-by-a-human. Ported from - heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; - fixtures 19 → 29. + heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; both + live shapes, the mixed round, and the whole check-outcome enum are pinned in + `test/labels-reconcile.sh` (fixtures 19 → 44). ## 0.1.1 — 2026-07-19 diff --git a/LABELS.md b/LABELS.md index 3eb4f39..567ec8d 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)" -- 2.45.2 From ef230e4a991af668f93996d674f1d883ddb73414 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:19:57 +0000 Subject: [PATCH 3/5] fix(labels): date a check run by when it started, not when it finished MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of #128. @claude-bot-andresmgsl and @codex-bot-andresmgsl independently found the same regression in the round-2 supersede rule. The collapse-to-newest step 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 `//` only falls through on null/false. So the sentinel was taken as the sort key and sorted below every real timestamp: the live re-run went to the bottom of its context and `last` discarded it, judging the very run it superseded. That inverted the rule in both directions. A green context with a replacement mid-flight read SUCCESS — #136 restored, needs-human pointing a human at a disabled merge button — and a CANCELLED original whose replacement was still running read FAILURE, the flap the collapse was added to prevent. A run is now dated by the newest timestamp it actually carries, with both spellings of absent discarded (null, and the zero sentinel), rather than by assuming which field is populated. An entry carrying no usable timestamp sorts last rather than first: something undateable is most likely the thing just created, so ambiguity resolves toward "not settled" instead of toward a stale success. Fixtures 44 -> 48. The gap was structural — the existing run_() helper always sets a real completedAt, so every supersede fixture raced two finished runs and none could express an in-flight one. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 25 ++++++++++++++++++++++--- CHANGELOG.md | 21 +++++++++++++++++++-- test/labels-reconcile.sh | 25 +++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index c22ec2e..03a9164 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 0f84aa9..2eadd0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,22 @@ actually cutting it, and this file starts there. `CANCELLED` blocks, judging every entry rather than the newest would strand every re-run PR in `needs-rebase`. + Dating a run turned out to be the subtle half, and getting it wrong restored + 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"`, a + string, which jq's `//` will not fall through. Ordering on completion + therefore sorted the *live* re-run below every finished one and let the + collapse discard it, judging the very run it superseded: a green context with + a replacement mid-flight read `SUCCESS` — the original bug restored, pointing + a human at a disabled merge button — and a `CANCELLED` original whose + replacement was still running read `FAILURE`, the flap the collapse exists to + prevent. So a run is dated by the newest timestamp it actually carries, with + both spellings of absent discarded (`null`, and the zero sentinel), and an + entry that carries no usable timestamp at all sorts **last** rather than + first: something undateable is most likely the thing just created, and every + ambiguity here resolves toward "not settled" rather than toward a stale + success. + `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 @@ -66,8 +82,9 @@ actually cutting it, and this file starts there. first. Queue order is intent, so the reconciler never sets it; it only **clears** it once the PR stops being mergeable-by-a-human. Ported from heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; both - live shapes, the mixed round, and the whole check-outcome enum are pinned in - `test/labels-reconcile.sh` (fixtures 19 → 44). + live shapes, the mixed round, the in-flight run superseding a finished one — + in both spellings of an absent completion — and the whole check-outcome enum + are pinned in `test/labels-reconcile.sh` (fixtures 19 → 48). ## 0.1.1 — 2026-07-19 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 -- 2.45.2 From 5d39783b7797107a266a7fd1f73bf15149f8092a Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:23:09 +0000 Subject: [PATCH 4/5] test(labels): pin the reverse direction of the supersede rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A finished re-run that completed after an earlier in-flight entry is the newer word on its context, and the context is settled. Nothing asserted that, so "an undateable/in-flight run sorts last" could be widened into "in flight always wins" — sort_by([(.outcome == ""), .at]) — and the suite stayed green. It now fails exactly this fixture and nothing else. Also records why the undateable fixture is non-vacuous: it is guarded by the sort tiebreak, not the dating expression. Reverting only `at:` leaves it passing, so the two code changes are separately pinned rather than jointly credited to the dating fix. Fixtures 48 -> 49. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 5 +++-- test/labels-reconcile.sh | 10 +++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eadd0b..da6488c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,8 +83,9 @@ actually cutting it, and this file starts there. **clears** it once the PR stops being mergeable-by-a-human. Ported from heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; both live shapes, the mixed round, the in-flight run superseding a finished one — - in both spellings of an absent completion — and the whole check-outcome enum - are pinned in `test/labels-reconcile.sh` (fixtures 19 → 48). + in both spellings of an absent completion, and in both directions — and the + whole check-outcome enum are pinned in `test/labels-reconcile.sh` + (fixtures 19 → 49). ## 0.1.1 — 2026-07-19 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. -- 2.45.2 From 3766a150227b661270aed36d72ad149d3e3460c3 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:32:22 +0000 Subject: [PATCH 5/5] fix(labels): date a run by when it began, not by its newest stamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of #128. @claude-bot-andresmgsl and @codex-bot-andresmgsl again converged on the same defect, in the round-3 dating expression itself. `max` over [startedAt, createdAt, completedAt] resolves to completedAt for a finished run and startedAt for a live one. Those are different quantities, so the comparison was never an ordering on runs — it was "newest stamp of any kind". 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 rather than a corner. On box's aa5a6ba the superseding run started 15:19:38 and the run it cancelled did not finish until 15:19:51 — thirteen seconds in which the dead predecessor out-dated the live run that replaced it, and the collapse discarded the wrong one. That narrowed round 3's two failures without closing them: a CANCELLED predecessor read FAILURE and a SUCCESS predecessor read SUCCESS, where both should be PENDING. The second is #136 restored — needs-human over a tree whose merge button branch protection has disabled. The list is already in preference order and the select leaves only stamps the run actually carries, so `first` is exactly "date it by when it began, falling back only if it never recorded a beginning". Fixtures 49 -> 51. None of the existing 49 could see this: every one spaces the predecessor's completion before the successor's start, and run_() carries no startedAt at all, so the overlap needed explicit payloads. Both new fixtures fail under `max` and the other 49 do not. Co-Authored-By: Claude Opus 4.8 --- .github/scripts/labels-reconcile.sh | 24 +++++++++++++++++------- CHANGELOG.md | 24 +++++++++++++++--------- test/labels-reconcile.sh | 19 +++++++++++++++++++ 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 03a9164..514479a 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 da6488c..d6b9e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,12 +64,18 @@ actually cutting it, and this file starts there. a replacement mid-flight read `SUCCESS` — the original bug restored, pointing a human at a disabled merge button — and a `CANCELLED` original whose replacement was still running read `FAILURE`, the flap the collapse exists to - prevent. So a run is dated by the newest timestamp it actually carries, with - both spellings of absent discarded (`null`, and the zero sentinel), and an - entry that carries no usable timestamp at all sorts **last** rather than - first: something undateable is most likely the thing just created, and every - ambiguity here resolves toward "not settled" rather than toward a stale - success. + prevent. So a run is dated by when it **began**, with both spellings of + absent discarded (`null`, and the zero sentinel) and a fallback only for a + run that never recorded a beginning — not by the newest stamp of any kind, + which compares the completion of a finished run against the start of a live + one. Those are different quantities, and a run cancelled by the concurrency + group does not stop the instant its replacement starts: the runner winds + down, so a predecessor routinely finishes *after* its successor began, and + dating by "newest stamp" let the dead run out-rank the live one that + replaced it. An entry carrying no usable timestamp at all sorts **last** + rather than first: something undateable is most likely the thing just + created, and every ambiguity here resolves toward "not settled" rather than + toward a stale success. `UNKNOWN` mergeability is deliberately not treated as unmergeable: GitHub reports it for about a minute after every merge while it recomputes, and @@ -83,9 +89,9 @@ actually cutting it, and this file starts there. **clears** it once the PR stops being mergeable-by-a-human. Ported from heavy-duty/box#137 so the three repos' reconcilers stay byte-identical; both live shapes, the mixed round, the in-flight run superseding a finished one — - in both spellings of an absent completion, and in both directions — and the - whole check-outcome enum are pinned in `test/labels-reconcile.sh` - (fixtures 19 → 49). + in both spellings of an absent completion, in both directions, and across the + wind-down window where the two overlap — and the whole check-outcome enum are + pinned in `test/labels-reconcile.sh` (fixtures 19 → 51). ## 0.1.1 — 2026-07-19 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 -- 2.45.2