diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 89ff1cb..f25b0e2 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -194,12 +194,17 @@ blockers() { # → the blocker:* labels this PR should carry, one per line # explicit human request — a maintainer claiming a PR early is deliberate, # not a dropped ball. if [ "$DRAFT" != true ] && ! requested "$HUMAN"; then - local b any_missing=false any_requested=false + local b v owed=false any_requested=false for b in "${BOTS[@]}"; do requested "$b" && any_requested=true - [ "$(bot_verdict "$b")" = MISSING ] && any_missing=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 [ "$any_missing" = true ] && [ "$any_requested" = false ]; then + if [ "$owed" = true ] && [ "$any_requested" = false ]; then echo blocker:unrequested fi fi @@ -350,6 +355,30 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch done add="${add#,}" remove="${remove#,}" + + # 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. + if [ -n "${REPO_LABELS:-}" ]; then + local kept="" missing="" want + for want in ${add//,/ } "$desired"; do + [ "$want" = "$desired" ] && continue + if grep -qxF "$want" <<<"$REPO_LABELS"; then kept="$kept,$want" + else missing="$missing $want"; fi + done + add="${kept#,}" + if ! grep -qxF "$desired" <<<"$REPO_LABELS"; then + log "#$n: WARNING: state label '$desired' does not exist — run the workflow manually to bootstrap" + return + fi + [ -n "$missing" ] && log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap" + fi if ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; then args=(--add-label "$desired${add:+,$add}") [ -n "$remove" ] && args+=(--remove-label "$remove") @@ -404,6 +433,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 ( diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cc1f89..b40dc69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,13 @@ which records not just what changed but what each drill run proved. human request — a maintainer claiming a PR early is deliberate. The reconciler strips `state:needs-rebase` on sight, so the retirement heals - the board rather than stranding a label nothing recomputes. Fixtures 51 → 64. + the board rather than stranding a label nothing recomputes. It also never + *names* a label the repo does not have: `gh issue edit` rejects the whole + call on one unknown name, so on a repo whose taxonomy predates this change + an unbootstrapped `blocker:*` would otherwise take the state convergence + down with it, on exactly the PRs the change exists to fix. Adds are filtered + against the repo's real label set and the shortfall is logged. + Fixtures 51 → 66. - **The tenant templates carry rig's family suffix: `claude` → `claude-box`, `codex` → `codex-box`, `grok` → `grok-box`, `staging` → `staging-box`** diff --git a/LABELS.md b/LABELS.md index 1ce4cd4..b70c98e 100644 --- a/LABELS.md +++ b/LABELS.md @@ -16,7 +16,7 @@ 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, fix, or ask | all bots reviewed and not all approved; or nobody was asked; or a blocker is up | the thing the blocker names is done | +| `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 @@ -35,7 +35,7 @@ PR carries as many as apply. |---|---|---|---| | `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` | somebody still owes a verdict and **nobody was asked** for one | reviews are requested | +| `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. diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index c756fb1..5970c08 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -201,7 +201,17 @@ expect "ready, nobody asked, nothing reviewed raises unrequested" blocker:unrequ # ...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