fix(labels): date a check run by when it started, not when it finished

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 <noreply@anthropic.com>
This commit is contained in:
dan-claude-bot 2026-07-20 16:19:57 +00:00
parent d8f73f446d
commit ef230e4a99
3 changed files with 66 additions and 5 deletions

View file

@ -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 # Once CANCELLED blocks, judging every entry would strand this very PR in
# needs-rebase forever, so collapse each context to its newest entry first. # 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 # 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 // [])[] | [ (.statusCheckRollup // [])[]
| { ctx: [.workflowName // "", .name // .context // ""], | { 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) } ] 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" | if ($latest | length) == 0 then "NONE"
elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE" elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE"

View file

@ -55,6 +55,22 @@ actually cutting it, and this file starts there.
`CANCELLED` blocks, judging every entry rather than the newest would strand `CANCELLED` blocks, judging every entry rather than the newest would strand
every re-run PR in `needs-rebase`. 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 `UNKNOWN` mergeability is deliberately not treated as unmergeable: GitHub
reports it for about a minute after every merge while it recomputes, and 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 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 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 **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 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 live shapes, the mixed round, the in-flight run superseding a finished one —
`test/labels-reconcile.sh` (fixtures 19 → 44). 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 ## 0.1.1 — 2026-07-19

View file

@ -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"}'),\ "$(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)" $(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 # -- 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. # 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 DRAFT=false HEAD_SHA=head1 REQUESTED="$HUMAN" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE