forked from heavy-duty/rig
fix(labels): unrecognised check outcomes block, and STALE outranks MISSING
Round 2 review found two ways the "a human could merge this right now" invariant still leaked, both of which let state:needs-human land on a PR the button would refuse. The check-rollup classifier enumerated the outcomes that block and defaulted the rest to SUCCESS, so ERROR, CANCELLED and STALE fell through into green. Inverted to an allow-list of the outcomes that DON'T block (SUCCESS, NEUTRAL, SKIPPED, plus the pending set); everything else, including an outcome neither enum has today, blocks. The rollup mixes CheckRun.conclusion with StatusContext.state and an outcome the list forgets is one we cannot certify as mergeable — a false FAILURE parks the PR on the agent, a false SUCCESS invites a bad merge. The classifier also moved out of main() into checks_state(), which is why no fixture caught this: it was inline in the fetch loop and the jq itself was untestable. Once CANCELLED blocks, superseded runs must be dropped first — a re-run does not evict the run it replaced, and judging every entry would strand every re-run PR in needs-rebase. Each context now collapses to its newest entry, keyed on workflow + job name because a bare job name is only unique within its workflow. 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 with nothing bound to the head. The whole round is now collected before any precedence is applied, STALE ahead of MISSING. The MISSING-yields-to-an-explicit-human-request rule is untouched. Fixtures 29 -> 44, pinning the whole check-outcome enum, the supersede rule in both orders, and the mixed round at both ends of BOTS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dan-claude-bot
parent
ef798f302c
commit
a0951eacf8
4 changed files with 157 additions and 25 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
|
||||
|
|
|
|||
26
CHANGELOG.md
26
CHANGELOG.md
|
|
@ -31,7 +31,25 @@ on the way to cutting its first release, and this file starts there.
|
|||
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 box#137'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
|
||||
|
|
@ -43,8 +61,10 @@ 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. Ported from heavy-duty/box#137 so the three
|
||||
repos' reconcilers stay byte-identical; fixtures 19 → 29.
|
||||
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.
|
||||
|
||||
### Added
|
||||
|
||||
|
|
|
|||
|
|
@ -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