fix(labels): an unrecognised check outcome blocks, and a staled round outranks an unfinished one
Round 2. Two blockers, both real, both closing the same hole this PR exists to close — a `state:needs-human` that invites a human to merge a tree that will not merge. The check-rollup classifier enumerated the outcomes that block and let the rest fall into `else "SUCCESS"`, so ERROR, CANCELLED and STALE all read as green. Inverted: it now lists the outcomes that DON'T block — SUCCESS, NEUTRAL, SKIPPED, plus the pending set — and treats everything else as blocking. The direction is the point. The rollup mixes two closed enums (CheckRun.conclusion, StatusContext.state) and an outcome the list forgets is one we cannot certify as mergeable; the costs are not symmetric, since a false FAILURE parks the PR on the agent who looks, while a false SUCCESS is #136 exactly. Once CANCELLED blocks, superseded runs must be dropped first: a re-run does not evict the run it replaced, and this PR's own tip carries a CANCELLED `scope` beside the SUCCESS `scope` that superseded it. Each context now collapses to its newest entry before anything is judged, keyed on workflow + job name because a bare job name is only unique within its workflow. That preserves the re-run case the panel split over while still blocking a cancelled run that is the newest word. The classifier also moved out of main() into checks_state(). That is why no fixture caught this: it was inline in the fetch loop, so the fixtures could only inject CHECKS= as an already-decided string. Second, 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 — a round that was both unfinished and staled came out needs-human over a head nobody had reviewed. The whole round is now collected before precedence is applied to it as a unit, STALE before MISSING. test/labels-reconcile.sh: 29 -> 44 fixtures, pinning the check-outcome enum, the supersede rule (both orders, plus same name in another workflow), and the mixed round at both ends of BOTS. All verified non-vacuous against the round-1 code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dan-claude-bot
parent
aa5a6baed6
commit
724f103908
4 changed files with 155 additions and 24 deletions
84
.github/scripts/labels-reconcile.sh
vendored
84
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -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
|
||||
|
|
|
|||
23
CHANGELOG.md
23
CHANGELOG.md
|
|
@ -85,7 +85,25 @@ which records not just what changed but what each drill run proved.
|
|||
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, so this PR's own tip carried a `CANCELLED` `scope` beside the
|
||||
`SUCCESS` `scope` that superseded it, and judging every entry would have
|
||||
stranded 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
|
||||
|
|
@ -98,7 +116,8 @@ 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
|
||||
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
|
||||
shapes are pinned in `test/labels-reconcile.sh` (19 fixtures → 29).
|
||||
shapes, the mixed round, and the whole check-outcome enum are pinned in
|
||||
`test/labels-reconcile.sh` (19 fixtures → 44).
|
||||
|
||||
- **CI's shellcheck sweep never lints `.github/scripts/*.sh`** (#116) —
|
||||
`globstar` makes `**` descend into subdirectories, but a glob still does
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)"
|
||||
|
|
|
|||
Loading…
Reference in a new issue