From 57e37363f404b40a5096293e5b640ca8dfb53958 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:32:53 +0000 Subject: [PATCH] =?UTF-8?q?fix(labels):=20order=20check=20runs=20by=20one?= =?UTF-8?q?=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.