diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 0912e4b..013a5f5 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -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 diff --git a/CHANGELOG.md b/CHANGELOG.md index c617a8b..5bb8565 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/LABELS.md b/LABELS.md index 4871f8e..9527f6d 100644 --- a/LABELS.md +++ b/LABELS.md @@ -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 diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index fb17d4a..32b0bf3 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -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)"