diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 773ebd3..1cec82a 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -31,7 +31,11 @@ set -euo pipefail HUMAN="${HUMAN_REVIEWER:-danmt}" BOTS=(claude-bot-andresmgsl codex-bot-andresmgsl grok-bot-andresmgsl) -STATES=(state:building state:needs-rebase state:bots-reviewing state:addressing state:needs-human) +STATES=(state:building state:bots-reviewing state:addressing state:needs-human) +BLOCKERS=(blocker:conflict blocker:ci-red blocker:unrequested) +# Labels this machine used to own and no longer does. Cleared on sight so a +# retirement heals the board instead of stranding a label nothing recomputes. +RETIRED=(state:needs-rebase) STALE_AFTER=$((48 * 3600)) log() { printf 'labels: %s\n' "$*"; } @@ -52,7 +56,14 @@ 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 +checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | UNREADABLE + # UNREADABLE is the absence of the key itself, which is what a failed fetch + # leaves behind — distinct from a present-but-empty rollup, which honestly + # means this PR has no checks. Collapsing the two let an API hiccup present + # as "nothing is failing", i.e. as mergeable-by-a-human: the same + # unknown-certified-as-green shape as the bug this machine exists to stop. + # The caller skips the PR entirely rather than labelling on facts it did not + # read; blocking on it instead would flap the whole board on one bad call. # 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 @@ -66,6 +77,8 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE # 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 ' + if (has("statusCheckRollup") | not) then "UNREADABLE" else + # 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 @@ -117,7 +130,9 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE | if ($latest | length) == 0 then "NONE" elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE" elif (($latest - $passing) | length) > 0 then "PENDING" - else "SUCCESS" end' + else "SUCCESS" end + + end' } bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK @@ -153,25 +168,66 @@ human_request_needed() { # 0 when needs-human requires a FRESH human request return 0 } +blockers() { # → the blocker:* labels this PR should carry, one per line + # The second axis. These are FACTS ABOUT THE BRANCH, and they are mutually + # independent — a PR can be conflicted and red and unasked at once — so they + # are a set, not an ordering. That is the whole point of splitting them out + # of state:*: every precedence bug this machine has had (needs-human + # surviving a conflict, MISSING swallowing STALE) came from projecting + # independent facts onto one totally-ordered label. A set has no precedence + # to get wrong. + # + # UNKNOWN mergeability is deliberately NOT a conflict: GitHub reports it for + # about a minute after every merge while it recomputes, and flapping every + # open PR on each merge would be worse than the bug. Same for a failed read + # of either fact — both default to the "do not know" value, which blocks + # nothing. An unset global (an older fixture, a failed fetch) must never + # invent a verdict it did not read. + case "${MERGEABLE:-UNKNOWN}" in CONFLICTING) echo blocker:conflict ;; esac + case "${CHECKS:-NONE}" in FAILURE) echo blocker:ci-red ;; esac + + # Nobody is on the hook for a verdict somebody still owes. Distinct from + # bots-reviewing, which says a request is live and an answer is coming: + # here the round is stalled because no one was ever asked, and the board + # said "waiting on the bots" for the 48h it took `stale` to notice. + # A draft is exempt (the bots ignore drafts by design), and so is an + # explicit human request — a maintainer claiming a PR early is deliberate, + # not a dropped ball. + if [ "$DRAFT" != true ] && ! requested "$HUMAN"; then + local b v owed=false any_requested=false + for b in "${BOTS[@]}"; do + requested "$b" && any_requested=true + # MISSING and STALE are both verdicts this head does not have: nobody + # reviewed it, or everybody reviewed something else. The agent owes an + # ask either way — the stale round is if anything the worse of the two, + # since it has approvals on the page that no longer describe the tree. + v="$(bot_verdict "$b")" + case "$v" in MISSING | STALE) owed=true ;; esac + done + if [ "$owed" = true ] && [ "$any_requested" = false ]; then + echo blocker:unrequested + fi + fi +} + decide_state() { # → the one state:* label this PR should carry if [ "$DRAFT" = true ]; then echo state:building; return; fi - # state:needs-human means ONE thing: a human could merge this right now. - # Anything that makes that false outranks the request that put it there — - # otherwise the board invites a merge that cannot or must not happen, and - # nothing else on the page contradicts it (#136). - # - # A conflicted or red branch is the agent's to fix, not the human's to - # merge. UNKNOWN is deliberately NOT treated as unmergeable: GitHub reports - # it for a minute after every merge while it recomputes, and flapping every - # open PR through needs-rebase on each merge would be worse than the bug. - # An unknown mergeability simply does not trigger this arm; the next sweep - # sees the settled value. - # Both default to the "do not know" value: an unset global (older fixture, - # a failed fetch) must never invent a verdict it did not read. - case "${MERGEABLE:-UNKNOWN}" in CONFLICTING) echo state:needs-rebase; return ;; esac - case "${CHECKS:-NONE}" in FAILURE) echo state:needs-rebase; return ;; esac + local s + s="$(round_state)" + # The one rule joining the two axes: state:needs-human means a human could + # merge this RIGHT NOW, so it requires a clear branch. Any blocker at all + # means the work is the agent's — whatever the review round says — and the + # blocker label says which work it is. Nothing else in this function reads + # the branch, which is what keeps the ordering below purely about reviews. + if [ "$s" = state:needs-human ] && [ -n "$(blockers)" ]; then + echo state:addressing; return + fi + echo "$s" +} + +round_state() { # → the state the REVIEW ROUND alone implies; knows no branch facts local b verdicts="" for b in "${BOTS[@]}"; do if requested "$b"; then echo state:bots-reviewing; return; fi @@ -199,9 +255,16 @@ decide_state() { # → the one state:* label this PR should carry # 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. + # + # Otherwise it is the AGENT's ball, not the bots'. The loop above already + # returned for every live bot request, so reaching here with a MISSING + # means somebody owes a verdict and nobody was asked for one — the round + # is not running. Calling that bots-reviewing was the lie that let a + # forgotten PR read "waiting on the reviewers" for the 48h it took the + # stale sweep to notice. blocker:unrequested says why. *MISSING*) if requested "$HUMAN"; then echo state:needs-human; return; fi - echo state:bots-reviewing; return ;; + echo state:addressing; 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 @@ -233,8 +296,10 @@ bootstrap_labels() { # dispatch-only: ~20 upserts is too chatty for every cron t 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|Mergeable, green, all bots approve — waiting on the human reviewer +state:needs-human|8250DF|No blockers, all bots approve — waiting on the human reviewer +blocker:conflict|B60205|Does not merge — the branch conflicts and the agent owes a rebase +blocker:ci-red|B60205|A check is failing — the agent owes a fix (not a rebase) +blocker:unrequested|E99695|Somebody still owes a verdict and nobody was asked for one 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 @@ -267,17 +332,66 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch log "#$n: requested $HUMAN (round passed)" fi - # ---- converge the state:* labels ---- + # ---- converge both axes ---- + # state:* is exclusive (everything but $desired comes off); blocker:* is a + # set (each one on or off on its own); RETIRED always comes off. One edit + # call for all of it, so a PR never flickers through a half-applied board. + local want_blockers add="" + want_blockers="$(blockers)" + remove="" for s in "${STATES[@]}"; do if [ "$s" != "$desired" ] && has_label "$s"; then remove="$remove,$s"; fi done + for s in "${RETIRED[@]}"; do + if has_label "$s"; then remove="$remove,$s"; fi + done + for s in "${BLOCKERS[@]}"; do + if grep -qxF "$s" <<<"$want_blockers"; then + has_label "$s" || add="$add,$s" + else + has_label "$s" && remove="$remove,$s" + fi + done + add="${add#,}" remove="${remove#,}" - if ! has_label "$desired" || [ -n "$remove" ]; then - args=(--add-label "$desired") + + # Never NAME a label the repo does not have. `gh issue edit --add-label` + # rejects the WHOLE call on one unknown name — nothing is applied — so a + # single missing blocker would take the state convergence down with it, on + # exactly the PRs this change exists to fix, surfacing only as a log line. + # Batching state and blockers into one edit for anti-flicker is what widened + # that blast radius; filtering the add side is what closes it again. + # Removals need no filter: they are built from has_label, so the label + # provably exists. REPO_LABELS unreadable means no filtering rather than + # filtering everything out — a failed read must not silently strip the board. + local skip_edit=false + if [ -n "${REPO_LABELS:-}" ]; then + local kept="" missing="" want + for want in ${add//,/ }; do + if grep -qxF "$want" <<<"$REPO_LABELS"; then kept="$kept,$want" + else missing="$missing $want"; fi + done + add="${kept#,}" + # A missing STATE label skips only the EDIT — never the rest of this + # function. Everything below is independent of the state:* taxonomy, and + # returning here stranded it: `merge-next` kept claiming "merge this one + # next" on a PR the board had moved to the agent, and the stale sweep + # stopped running. That is the original false-invitation bug, reintroduced + # in the very fix meant to survive a cold-start repo — and a regression + # against the old behaviour, which failed the edit and fell through. + if ! grep -qxF "$desired" <<<"$REPO_LABELS"; then + log "#$n: WARNING: state label '$desired' does not exist — skipping the label edit; dispatch the workflow to bootstrap" + skip_edit=true + elif [ -n "$missing" ]; then + log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap" + fi + fi + if [ "$skip_edit" = false ] && { ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; }; then + args=(--add-label "$desired${add:+,$add}") [ -n "$remove" ] && args+=(--remove-label "$remove") if run gh issue edit "$n" -R "$REPO" "${args[@]}" >/dev/null; then - log "#$n: state -> $desired${remove:+ (cleared $remove)}" + log "#$n: state -> $desired${add:+ +$add}${remove:+ (cleared $remove)}" else # a deleted label must not wedge the sweep — dispatch heals the taxonomy log "#$n: WARNING: label edit failed (missing label? run the workflow manually to bootstrap)" @@ -327,6 +441,11 @@ main() { bootstrap_labels fi + # The repo's label set, read ONCE per sweep — reconcile_pr filters every + # add against it, because one unknown name fails the whole edit call. + REPO_LABELS="$(gh label list -R "$REPO" --limit 200 --json name --jq '.[].name' 2>/dev/null || echo "")" + [ -z "$REPO_LABELS" ] && log "WARNING: could not read the label set — applying labels unfiltered" + local n for n in $(gh pr list -R "$REPO" --state open --limit 100 --json number --jq '.[].number'); do ( @@ -348,6 +467,13 @@ main() { GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')" MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")" CHECKS="$(checks_state <<<"$GH_VIEW")" + # Read failed: leave this PR exactly as it is. Recomputing on facts we + # did not read is how an API hiccup turns into a false "merge me" — + # and the next tick is 15 minutes away, not 15 hours. + if [ "$CHECKS" = UNREADABLE ]; then + log "#$n: could not read mergeability/checks — left alone this pass" + exit 0 + fi reconcile_pr "$n" ) || log "#$n: reconcile failed — continuing with the remaining PRs" done diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3ecbcee..0465a53 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,12 @@ jobs: shellcheck -x "${files[@]}" - name: cli tests run: bash test/cli.sh + # test/labels-reconcile.sh existed here since #87 but ran nowhere: the + # label state machine gates every PR on this repo and its fixtures were + # green only when someone remembered to run them by hand. Same step, same + # place as heavy-duty/box. + - name: labels state-machine tests + run: bash test/labels-reconcile.sh - name: release-flow tests run: bash test/release.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 2075c5d..9f0a755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,31 @@ on the way to cutting its first release, and this file starts there. ### Fixed +- **An unreadable check rollup no longer reads as "nothing is failing"** (#90) + — when `gh pr view` failed, the fallback left the `statusCheckRollup` key + absent entirely, and `(.statusCheckRollup // [])` collapsed that into the + same `NONE` as a PR that genuinely has no checks. `NONE` blocks nothing, so + a transient API failure presented the PR as mergeable by a human: an unknown + certified as green, which is the exact shape of the bug #87 was opened to + stop, surviving in the one place that fix never looked. + + `checks_state` now separates the two — `UNREADABLE` for an absent key (a + read that failed), `NONE` for a present-but-empty array (a PR that really + has no checks) — and the sweep leaves an `UNREADABLE` PR exactly as it + found it rather than recomputing labels from facts it did not read. + Deliberately *not* a blocker: blocking would flap the whole board on one bad + API call, and the next tick is fifteen minutes away. Caught by the author + after opening the PR, not by review. + +- **CI runs `test/labels-reconcile.sh`, which it had never run** (#90) — the + file arrived with #87 and `ci.yml` was not extended to call it, so the label + state machine that gates every PR in this repo went covered only by whoever + remembered to run its fixtures by hand. #88 merged reporting 51 passing + fixtures: true on the author's machine, never once verified here. Box and + cast both ran the suite already; only rig did not, so this closes a rig-local + gap rather than a family-wide one. It was found by asking, while adding + fixtures to the suite, where the suite actually ran. + - **`state:needs-human` no longer appears on PRs a human cannot merge** (#87, heavy-duty/box#136) — `decide_state()` derived state from three inputs (draft flag, requested reviewers, submitted reviews) and read *nothing* about @@ -163,6 +188,55 @@ on the way to cutting its first release, and this file starts there. ### Changed +- **PR labels split into two axes: `state:*` (whose ball) and `blocker:*` + (what is in the way)** (heavy-duty/box#137) — `state:needs-rebase`, added + here only days ago by #87, is retired in the same breath. In its place: + `blocker:conflict`, `blocker:ci-red` and `blocker:unrequested`, applied + additively. A single rule joins the axes — `state:needs-human` requires zero + blockers — which is the invariant #87 was reaching for, stated once instead + of defended at every branch of a precedence chain. + + The single-label design projected independent facts onto one totally-ordered + value. Mergeability, check status and the review round move on their own + clocks; a PR can be conflicted *and* red *and* stalled at the same instant. + A total order has to pick one of those to say, so the rest vanish. Every + precedence bug this machine has had lived on that ordering, #87's included: + the fix there was to reorder the chain and collect the round before deciding, + which bought correctness for one more configuration without removing the + reason the next one would break. `state:needs-rebase` was the design's + clearest tell — a single label fired by both a conflict and a failing check, + two problems needing opposite work, telling an agent to rebase when what it + owed was a bug fix. Box's board has the case in the open: #120 was conflicted + **and** red, and could only ever say one of them. + + Blockers are a set. There is no precedence between them to get wrong, and + adding a fourth one later cannot reshuffle the meaning of the other three. + What stays on the ordered axis is purely the review round, which is the one + place here where an ordering is genuinely meaningful — a round really does + have a sequence. + + `state:bots-reviewing` tightens with it, to mean strictly *a request is live + and an answer is coming*. A ready PR nobody was asked to review used to read + as "waiting on the reviewers" until the stale sweep caught up; it now reads + `state:addressing` + `blocker:unrequested`, because the agent owes the ask + and the board should say so. `blocker:unrequested` covers both shapes of + "this head has no verdict from somebody": nobody reviewed it, or everybody + reviewed an older tree and the approvals staled behind a push. The second is + the worse of the two, since it leaves approvals on the page that no longer + describe the code. Drafts stay exempt — the bots ignore drafts by design — + as does an explicit human request, since a maintainer claiming a PR early is + deliberate. + + The reconciler strips `state:needs-rebase` on sight via a `RETIRED` list, so + the retirement heals the existing board instead of stranding a label that + nothing recomputes. It also filters every label it is about to *add* against + the repo's actual label set, read once per sweep: `gh issue edit` rejects the + whole call on one unknown name, so on a repo that has not yet bootstrapped + the new `blocker:*` labels a single missing one would have taken the state + convergence down with it — on exactly the PRs this change exists to heal. + Now the state still converges and the missing labels are named in the log. + Fixtures 51 → 72. + - **BREAKING: `--class human|server` is now `--root-door closed|open`** (#77) — the trait was named for who *lives on* a box; what it decides is one thing, and it is not occupancy: whether root SSH stays open as the control plane's diff --git a/LABELS.md b/LABELS.md index 9527f6d..f352850 100644 --- a/LABELS.md +++ b/LABELS.md @@ -16,33 +16,61 @@ single reply, and a human takes the final review. |---|---|---|---|---| | `state:building` | `#FBCA04` | the coding agent, still building | PR opened as draft | marked ready + bot reviews requested | | `state:bots-reviewing` | `#1D76DB` | the reviewer bots to finish the round | ready with reviews requested, or fixes pushed and reviews re-requested | all three bots have reviewed the round | -| `state:addressing` | `#D93F0B` | the coding agent to reply and push fixes | all bots reviewed the round, not all approved | the single round-reply is posted and fixes pushed | -| `state:needs-rebase` | `#B60205` | the coding agent to rebase or fix | the branch does not merge — GitHub says `CONFLICTING`, or a check has failed | it merges cleanly and checks are green again | -| `state:needs-human` | `#8250DF` | the human reviewer | the PR **could be merged right now**: mergeable, checks not failing, three formal head-current approvals — and the human review is requested | merged — or changes requested, which cycles back to `state:addressing` | +| `state:addressing` | `#D93F0B` | the coding agent to reply, fix, or ask | all bots reviewed and not all approved; or nobody was asked; or a blocker is up | the round-reply is posted and fixes pushed — and any blocker named alongside is cleared | +| `state:needs-human` | `#8250DF` | the human reviewer | the PR **could be merged right now**: no blockers, three formal head-current approvals — and the human review is requested | merged — or changes requested, which cycles back to `state:addressing` | `bots-reviewing` and `addressing` are deliberately distinct: staleness in the first means *poke the bots*, staleness in the second means *the agent dropped the ball*. Collapsing them loses exactly the information a sweep needs. +`bots-reviewing` therefore means strictly *a request is live and an answer is +coming* — a PR nobody was asked to review is the agent's ball, not the bots'. + +## The second axis: `blocker:*` + +State answers *whose ball is it*. Blockers answer *what is in the way*, and +unlike states they are *facts about the branch* — mutually independent, so a +PR carries as many as apply. + +| Label | Color | Means | Clears when | +|---|---|---|---| +| `blocker:conflict` | `#B60205` | GitHub says `CONFLICTING` — the agent owes a **rebase** | it merges cleanly | +| `blocker:ci-red` | `#B60205` | a check failed — the agent owes a **fix**, which a rebase will not provide | checks are green | +| `blocker:unrequested` | `#E99695` | this head has no verdict from somebody — never reviewed, or staled by a push — and **nobody was asked** for one | reviews are requested | + +One rule joins the axes: **`state:needs-human` requires zero blockers.** Any +blocker means the work is the agent's, whatever the review round says. + +This split exists because the single-label version kept lying. Independent +facts were projected onto one totally-ordered label, so one always had to win +and the losers vanished off the board: a PR that was *both* conflicted and red +could only say one of them, and `needs-rebase` told an agent to rebase when +what it actually owed was a bug fix. Precedence between two blockers is not a +question a set has to answer, which is why every ordering bug this machine has +had — `needs-human` surviving a conflict, `MISSING` swallowing `STALE` — lived +on the axis that had to be totally ordered. + +`state:needs-rebase` was the first attempt at this and is **retired**; the +reconciler strips it on sight so no PR is left carrying a label nothing +recomputes. **`state:needs-human` means one thing: a human could merge this right now.** -Anything that makes that false outranks the review request that put it there, -because the label is the only signal a maintainer scanning the board (or a -phone) actually reads — and a label that says "your turn" on an unmergeable PR -is worse than no label at all. Two things therefore take precedence over an +The label is the only signal a maintainer scanning the board (or a phone) +actually reads, and one that says "your turn" on an unmergeable PR is worse +than no label at all. So beyond the blockers, one review fact also outranks an explicit human request: -- **it does not merge** — `CONFLICTING`, or a failing check → `state:needs-rebase` - **nobody reviewed *this* head** — every approval staled by a push → `state:addressing`, because the agent owes a re-request -The second is the more dangerous of the two: with a conflict, GitHub at least -disables the merge button, while a staled-approval PR reads green, mergeable +That case is more dangerous than any blocker: a blocked PR at least shows an X +or a disabled merge button, while a staled-approval PR reads green, mergeable and "waiting on the human" over code no reviewer has seen. -`UNKNOWN` mergeability is deliberately **not** treated as unmergeable. GitHub +`UNKNOWN` mergeability is deliberately **not** treated as a conflict. GitHub 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 than -the bug this precedence fixes. +flapping every open PR through `blocker:conflict` on each merge would be worse +than the bug this fixes. A failed read of either branch fact degrades to the +same "do not know" value, for the same reason. An *unfinished* round still yields to an explicit human request — a maintainer pulling a PR to themselves early is a deliberate act. `MISSING` (nobody has @@ -96,8 +124,12 @@ missing label idempotently. To create them by hand (needs push access): gh label create "state:building" --color FBCA04 --description "PR is a draft — the coding agent is still building" --force 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 "Mergeable, green, all bots approve — waiting on the human reviewer" --force +gh label create "blocker:conflict" --color B60205 --description "Does not merge — the branch conflicts and the agent owes a rebase" --force +gh label create "blocker:ci-red" --color B60205 --description "A check is failing — the agent owes a fix (not a rebase)" --force +gh label create "blocker:unrequested" --color E99695 --description "Somebody still owes a verdict and nobody was asked for one" --force +# retired — the reconciler strips it; delete it once no PR carries it +# gh label delete "state:needs-rebase" +gh label create "state:needs-human" --color 8250DF --description "No blockers, 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 b68de99..cce4691 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -44,10 +44,15 @@ $BOT3" REVIEWS_JSON='[]' expect "requested bots mean bots-reviewing" state:bots-reviewing "$(decide_state)" # -- a bot that never reviewed keeps the round open --------------------------- -REQUESTED="" REVIEWS_JSON="$(reviews \ +# With a live request that is the bots' ball; with NO request outstanding it +# is the agent's, because nothing is coming until somebody asks. +REQUESTED="$BOT3" REVIEWS_JSON="$(reviews \ "$(rev "$BOT1" APPROVED head1 "" t1)" \ "$(rev "$BOT2" APPROVED head1 "" t2)")" -expect "missing bot review means bots-reviewing" state:bots-reviewing "$(decide_state)" +expect "a missing bot WITH a live request is bots-reviewing" state:bots-reviewing "$(decide_state)" +REQUESTED="" +expect "...but with nobody asked it is the agent's ball" state:addressing "$(decide_state)" +expect "...and the blocker names the stall" blocker:unrequested "$(blockers)" # -- a comment is a non-verdict, agreement body or not: the author escalates -- REVIEWS_JSON="$(reviews \ @@ -157,23 +162,65 @@ ALL_APPROVE="$(reviews \ "$(rev "$BOT3" APPROVED head1 "" t3)")" # -- flavour 1: not mergeable. The merge button is disabled, yet the board -# said "your turn" on #119/#120/#127 for hours. +# said "your turn" on #119/#120/#127 for hours. The branch fact now rides +# the blocker axis; the state says whose ball it is, which is the agent's. DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=CONFLICTING CHECKS=SUCCESS -expect "a CONFLICTING PR is needs-rebase, not needs-human" state:needs-rebase "$(decide_state)" +expect "a CONFLICTING PR is the agent's, not the human's" state:addressing "$(decide_state)" +expect "...and says WHY on the blocker axis" blocker:conflict "$(blockers)" REQUESTED="$HUMAN" -expect "...even with the human explicitly requested" state:needs-rebase "$(decide_state)" +expect "...even with the human explicitly requested" state:addressing "$(decide_state)" -# -- red CI is the same claim: not something a human should merge. +# -- red CI is the same claim, but NOT the same work: a rebase does not fix a +# failing test. Collapsing both into one needs-rebase label told the agent +# to do the wrong thing, which is why the axis split exists. REQUESTED="" MERGEABLE=MERGEABLE CHECKS=FAILURE -expect "a red PR is needs-rebase" state:needs-rebase "$(decide_state)" +expect "a red PR is the agent's" state:addressing "$(decide_state)" +expect "...and is distinguishable from a conflict" blocker:ci-red "$(blockers)" REQUESTED="$HUMAN" -expect "...and a human request does not override red CI" state:needs-rebase "$(decide_state)" +expect "...and a human request does not override red CI" state:addressing "$(decide_state)" + +# -- both at once. The single-axis design could not say this at all: one label +# had to win, and the loser silently vanished off the board. +REQUESTED="" MERGEABLE=CONFLICTING CHECKS=FAILURE +expect "a conflicted AND red PR reports both blockers" "blocker:conflict +blocker:ci-red" "$(blockers)" +expect "...and is still just the agent's ball" state:addressing "$(decide_state)" # -- UNKNOWN is NOT unmergeable. GitHub reports it for ~a minute after every # merge while it recomputes; treating it as broken would flap every open PR -# into needs-rebase on each merge — worse than the bug being fixed. +# on each merge — worse than the bug being fixed. REQUESTED="" MERGEABLE=UNKNOWN CHECKS=PENDING -expect "UNKNOWN mergeability does not trigger needs-rebase" state:needs-human "$(decide_state)" +expect "UNKNOWN mergeability blocks nothing" state:needs-human "$(decide_state)" +expect "...and raises no blocker" "" "$(blockers)" + +# -- blocker:unrequested — the stalled round. Nobody owes an answer because +# nobody was ever asked, yet the board read "waiting on the bots" until +# `stale` noticed 48h later. +MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]' +expect "ready, nobody asked, nothing reviewed raises unrequested" blocker:unrequested "$(blockers)" +# ...the partial case is equally stalled: one verdict in, nobody asked for the rest +REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" +expect "one bot in, none requested is still unrequested" blocker:unrequested "$(blockers)" +# ...a STALE round with nobody asked is the same debt, and arguably worse: the +# page carries approvals that no longer describe the tree. Guarding on +# MISSING alone let this one through with no blocker at all. +REVIEWS_JSON="$(reviews \ + "$(rev "$BOT1" APPROVED oldhead "" t1)" \ + "$(rev "$BOT2" APPROVED oldhead "" t2)" \ + "$(rev "$BOT3" APPROVED oldhead "" t3)")" +expect "a stale round with nobody asked is unrequested too" blocker:unrequested "$(blockers)" +expect "...and is still the agent's ball" state:addressing "$(decide_state)" +# ...but a live request means an answer IS coming +REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" +REQUESTED="$BOT2" +expect "a live bot request is not a stalled round" "" "$(blockers)" +# ...and a draft is exempt: the bots ignore drafts by design +DRAFT=true REQUESTED="" REVIEWS_JSON='[]' +expect "a draft with nobody asked is not stalled" "" "$(blockers)" +# ...as is an explicit human request — claiming a PR early is deliberate +DRAFT=false REQUESTED="$HUMAN" +expect "an early human claim is not a stalled round" "" "$(blockers)" +REQUESTED="" REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS # -- flavour 2 (the dangerous one): mergeable, green, human requested, and # NOBODY has reviewed this head. Observed on #119 after a rebase: every @@ -207,7 +254,7 @@ expect "...and the same when the stale verdict is the LAST bot in BOTS" \ REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")" expect "an unfinished round still yields to an explicit human request" state:needs-human "$(decide_state)" REQUESTED="" -expect "...and without that request it is still bots-reviewing" state:bots-reviewing "$(decide_state)" +expect "...and without that request the agent owes the ask" state:addressing "$(decide_state)" # --------------------------------------------------------------------------- # checks_state: the rollup classifier. It lived inline in main() for the first @@ -221,6 +268,13 @@ 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)" +# A failed fetch leaves no rollup KEY; a PR with no checks leaves an empty +# ARRAY. Collapsing the two let an API hiccup read as "nothing is failing" — +# the same unknown-certified-as-green shape as #136, in the one place that +# fix did not look. The caller skips an UNREADABLE PR rather than relabelling. +expect "a failed read is UNREADABLE, not NONE" UNREADABLE "$(echo '{}' | checks_state)" +expect "...and a real empty rollup is still NONE" NONE \ + "$(echo '{"mergeable":"MERGEABLE","statusCheckRollup":[]}' | checks_state)" expect "all green is SUCCESS" SUCCESS \ "$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)" expect "a queued run is PENDING" PENDING \ @@ -314,7 +368,8 @@ expect "...and the same when it finished green — mid-flight is not mergeable" # 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)" +expect "a cancelled check reaches decide_state as the agent's ball" state:addressing "$(decide_state)" +expect "...via blocker:ci-red, not a conflict" blocker:ci-red "$(blockers)" # -- the happy path survives all of the above. REVIEWS_JSON="$ALL_APPROVE" MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" @@ -324,5 +379,38 @@ DRAFT=true MERGEABLE=CONFLICTING expect "a draft is building even when conflicted" state:building "$(decide_state)" DRAFT=false MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]' +# --------------------------------------------------------------------------- +# reconcile_pr's cold-start path. Everything above tests pure functions, which +# is exactly why a per-PR `return` in the label pre-flight got through review: +# the fixtures could not reach it. A missing state:* label must skip the label +# EDIT only — merge-next clearing and the stale sweep are independent of the +# taxonomy, and stranding them reintroduced the false-invitation bug (a +# `merge-next` claim surviving on a PR the board had moved to the agent). +# --------------------------------------------------------------------------- +reconcile_probe() { # $1 = REPO_LABELS content → the log lines reconcile_pr emits + ( + REPO_LABELS="$1" REPO=owner/repo NOW="$(date +%s)" + LABELS="merge-next" # the PR carries a queue claim + DRAFT=false HEAD_SHA=head1 REQUESTED="" REVIEWS_JSON='[]' + MERGEABLE=MERGEABLE CHECKS=SUCCESS + PR_JSON='{"created_at":"2020-01-01T00:00:00Z"}' + run() { :; } # swallow mutations + gh() { :; } # no network + reconcile_pr 777 2>&1 + ) +} + +cold="$(reconcile_probe "merge-next")" # state:* labels absent entirely +expect "a cold-start repo still clears merge-next" \ + yes "$(grep -q 'cleared merge-next' <<<"$cold" && echo yes || echo no)" +expect "...and still runs the stale sweep" \ + yes "$(grep -q 'stale (' <<<"$cold" && echo yes || echo no)" +expect "...while warning that the state label is missing" \ + yes "$(grep -q "state label 'state:addressing' does not exist" <<<"$cold" && echo yes || echo no)" + +warm="$(reconcile_probe "$(printf 'state:addressing\nmerge-next\nstale\nblocker:unrequested')")" +expect "a bootstrapped repo converges the state as well" \ + yes "$(grep -q 'state -> state:addressing' <<<"$warm" && echo yes || echo no)" + printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail" [ "$fail" -eq 0 ]