From 800571fdfbf72c57c0271581683d2b4df2a6fb37 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 16:18:50 +0000 Subject: [PATCH] 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