forked from heavy-duty/box
fix(labels): date a check run by the newest stamp it carries, not by its completion
Round 3. @claude-bot-andresmgsl and @codex-bot-andresmgsl independently caught that the supersede rule added in round 2 inverted itself on the one shape its fixtures could not express: a re-run still in flight. A running check does not omit `completedAt`. `gh` marshals the Go zero time as the STRING "0001-01-01T00:00:00Z", and jq's `//` only falls through null/false, so the sentinel won the sort key and sorted before every real timestamp. The live re-run became the OLDEST entry in its context, `last` discarded it, and the run it superseded was judged instead — exactly backwards, and wrong in both directions: green + re-run in flight -> SUCCESS (should be PENDING) CANCELLED + re-run in flight -> FAILURE (should be PENDING) The first is #136 restored by the very rule meant to close it: all bots approve, mergeable, state:needs-human — over a tree whose merge button branch protection has disabled. It was also a regression from round 1, which caught it via `any(. == "")`. The second is the re-run flap the supersede rule exists to prevent, narrowed rather than removed. Fixed by taking the newest timestamp a run actually carries and discarding BOTH spellings of absent — null and the zero sentinel — rather than by reordering the fallbacks. An entry with no usable timestamp now sorts LAST rather than first, so an undateable in-flight run is never dropped in favour of a stale success. Every ambiguity resolves toward "not settled". The fixtures could not have caught this: the `run_()` helper sets only `completedAt`, so every supersede fixture was a race between two FINISHED runs. The helper now expresses an in-flight entry, and the four new fixtures assert PENDING over both a green and a cancelled predecessor. 44 -> 48; reverting just the dating expression fails 3 of the 4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
724f103908
commit
e40d2df5f1
3 changed files with 60 additions and 6 deletions
25
.github/scripts/labels-reconcile.sh
vendored
25
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -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"
|
||||||
|
|
|
||||||
16
CHANGELOG.md
16
CHANGELOG.md
|
|
@ -103,7 +103,16 @@ which records not just what changed but what each drill run proved.
|
||||||
context collapsing to its newest entry: a re-run does not evict the run it
|
context collapsing to its newest entry: a re-run does not evict the run it
|
||||||
replaced, so this PR's own tip carried a `CANCELLED` `scope` beside the
|
replaced, so this PR's own tip carried a `CANCELLED` `scope` beside the
|
||||||
`SUCCESS` `scope` that superseded it, and judging every entry would have
|
`SUCCESS` `scope` that superseded it, and judging every entry would have
|
||||||
stranded every re-run PR in `needs-rebase`.
|
stranded every re-run PR in `needs-rebase`. Which entry is newest is decided
|
||||||
|
on the newest timestamp a run actually carries, because a run still in flight
|
||||||
|
does not omit its completion — `gh` marshals the Go zero time as the *string*
|
||||||
|
`"0001-01-01T00:00:00Z"`, which `//` will not fall through. Dating on
|
||||||
|
completion therefore sorted the live re-run to the bottom and picked the run
|
||||||
|
it superseded, reporting the old `SUCCESS` while a replacement was still
|
||||||
|
running: #136 restored, by the very rule meant to close it. An entry carrying
|
||||||
|
no usable timestamp sorts last rather than first, so an undateable in-flight
|
||||||
|
run 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
|
`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
|
||||||
|
|
@ -116,8 +125,9 @@ which records not just what changed but what each drill run proved.
|
||||||
`CHANGELOG.md`. Queue order is intent, so the reconciler never sets it — it
|
`CHANGELOG.md`. Queue order is intent, so the reconciler never sets it — it
|
||||||
only **clears** it the moment the PR stops being mergeable-by-a-human, which
|
only **clears** it the moment the PR stops being mergeable-by-a-human, which
|
||||||
is precisely the staleness that made `needs-human` untrustworthy. Both live
|
is precisely the staleness that made `needs-human` untrustworthy. Both live
|
||||||
shapes, the mixed round, and the whole check-outcome enum are pinned in
|
shapes, the mixed round, the whole check-outcome enum, and the in-flight
|
||||||
`test/labels-reconcile.sh` (19 fixtures → 44).
|
re-run superseding both a green and a cancelled predecessor are pinned in
|
||||||
|
`test/labels-reconcile.sh` (19 fixtures → 48).
|
||||||
|
|
||||||
- **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) —
|
- **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) —
|
||||||
`globstar` makes `**` descend into subdirectories, but a glob still does
|
`globstar` makes `**` descend into subdirectories, but a glob still does
|
||||||
|
|
|
||||||
|
|
@ -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
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue