fix(labels): never name a label the repo lacks, and do not read an unreadable rollup as green
Round-1 review fixes, canonical across box/rig/cast. `gh issue edit --add-label` rejects the WHOLE call on one unknown label name, applying nothing. Batching state and blockers into a single edit for anti-flicker meant one missing `blocker:*` would take the `state:*` convergence down with it — and since the taxonomy was only ever created by a manual workflow_dispatch, the first sweep after the two-axis change would have healed nothing on exactly the PRs it exists to fix, surfacing only as a log line. The add side is now filtered against the repo's real label set, read once per sweep. Removals need no filter (built from has_label, so they provably exist); an unreadable label set filters nothing rather than everything, because a failed read must not silently strip the board. `checks_state` returns UNREADABLE when the `statusCheckRollup` key is absent — what a failed `gh pr view` leaves behind — distinct from NONE for a present-but-empty array. Collapsing the two let an API hiccup present as "nothing is failing", i.e. as mergeable-by-a-human: the unknown-certified-as- green shape this machine exists to stop, surviving where the #128 fix never looked. The sweep now leaves that PR exactly as it is. Deliberately not a blocker: blocking would flap the whole board on one bad call. `blocker:unrequested` also fires on a STALE round, not just a MISSING one. Both mean this head has no verdict from that reviewer and both owe an ask; the stale round is the worse of the two, since it carries approvals on the page that no longer describe the tree. Fixtures 64 -> 68. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
f281b8c5ae
commit
f51ef29b79
4 changed files with 103 additions and 8 deletions
62
.github/scripts/labels-reconcile.sh
vendored
62
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -56,7 +56,14 @@ run() { # every mutation goes through here — DRY_RUN=1 logs instead of doing
|
||||||
|
|
||||||
requested() { grep -qxF "$1" <<<"$REQUESTED"; }
|
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
|
# The rollup mixes two node types with two different closed enums: CheckRun
|
||||||
# carries `conclusion` (CheckConclusionState), StatusContext carries `state`
|
# carries `conclusion` (CheckConclusionState), StatusContext carries `state`
|
||||||
# (StatusState). Rather than list the outcomes that block — the version that
|
# (StatusState). Rather than list the outcomes that block — the version that
|
||||||
|
|
@ -70,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;
|
# 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.
|
# a false SUCCESS invites a human to merge a tree that will not merge.
|
||||||
jq -r '
|
jq -r '
|
||||||
|
if (has("statusCheckRollup") | not) then "UNREADABLE" else
|
||||||
|
|
||||||
# NEUTRAL and SKIPPED satisfy branch protection — a skipped required check
|
# NEUTRAL and SKIPPED satisfy branch protection — a skipped required check
|
||||||
# is not a failed one, and path-filtered jobs skip constantly here.
|
# is not a failed one, and path-filtered jobs skip constantly here.
|
||||||
["SUCCESS", "NEUTRAL", "SKIPPED"] as $passing
|
["SUCCESS", "NEUTRAL", "SKIPPED"] as $passing
|
||||||
|
|
@ -121,7 +130,9 @@ checks_state() { # rollup JSON on stdin → SUCCESS | FAILURE | PENDING | NONE
|
||||||
| if ($latest | length) == 0 then "NONE"
|
| if ($latest | length) == 0 then "NONE"
|
||||||
elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE"
|
elif (($latest - $passing - $waiting) | length) > 0 then "FAILURE"
|
||||||
elif (($latest - $passing) | length) > 0 then "PENDING"
|
elif (($latest - $passing) | length) > 0 then "PENDING"
|
||||||
else "SUCCESS" end'
|
else "SUCCESS" end
|
||||||
|
|
||||||
|
end'
|
||||||
}
|
}
|
||||||
|
|
||||||
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
bot_verdict() { # $1 = login → MISSING | BLOCK | APPROVE | STALE | FEEDBACK
|
||||||
|
|
@ -183,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,
|
# explicit human request — a maintainer claiming a PR early is deliberate,
|
||||||
# not a dropped ball.
|
# not a dropped ball.
|
||||||
if [ "$DRAFT" != true ] && ! requested "$HUMAN"; then
|
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
|
for b in "${BOTS[@]}"; do
|
||||||
requested "$b" && any_requested=true
|
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
|
done
|
||||||
if [ "$any_missing" = true ] && [ "$any_requested" = false ]; then
|
if [ "$owed" = true ] && [ "$any_requested" = false ]; then
|
||||||
echo blocker:unrequested
|
echo blocker:unrequested
|
||||||
fi
|
fi
|
||||||
fi
|
fi
|
||||||
|
|
@ -339,6 +355,30 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch
|
||||||
done
|
done
|
||||||
add="${add#,}"
|
add="${add#,}"
|
||||||
remove="${remove#,}"
|
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
|
if ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; then
|
||||||
args=(--add-label "$desired${add:+,$add}")
|
args=(--add-label "$desired${add:+,$add}")
|
||||||
[ -n "$remove" ] && args+=(--remove-label "$remove")
|
[ -n "$remove" ] && args+=(--remove-label "$remove")
|
||||||
|
|
@ -393,6 +433,11 @@ main() {
|
||||||
bootstrap_labels
|
bootstrap_labels
|
||||||
fi
|
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
|
local n
|
||||||
for n in $(gh pr list -R "$REPO" --state open --limit 100 --json number --jq '.[].number'); do
|
for n in $(gh pr list -R "$REPO" --state open --limit 100 --json number --jq '.[].number'); do
|
||||||
(
|
(
|
||||||
|
|
@ -414,6 +459,13 @@ main() {
|
||||||
GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')"
|
GH_VIEW="$(gh pr view "$n" -R "$REPO" --json mergeable,statusCheckRollup 2>/dev/null || echo '{}')"
|
||||||
MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")"
|
MERGEABLE="$(jq -r '.mergeable // "UNKNOWN"' <<<"$GH_VIEW")"
|
||||||
CHECKS="$(checks_state <<<"$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"
|
reconcile_pr "$n"
|
||||||
) || log "#$n: reconcile failed — continuing with the remaining PRs"
|
) || log "#$n: reconcile failed — continuing with the remaining PRs"
|
||||||
done
|
done
|
||||||
|
|
|
||||||
28
CHANGELOG.md
28
CHANGELOG.md
|
|
@ -43,10 +43,36 @@ actually cutting it, and this file starts there.
|
||||||
|
|
||||||
The reconciler carries a `RETIRED` array and strips `state:needs-rebase` on
|
The reconciler carries a `RETIRED` array and strips `state:needs-rebase` on
|
||||||
sight, so retiring a label heals the board instead of stranding one that
|
sight, so retiring a label heals the board instead of stranding one that
|
||||||
nothing recomputes. Fixtures 51 → 64.
|
nothing recomputes. A verdict is owed in two shapes and both raise
|
||||||
|
`blocker:unrequested`: `MISSING` (nobody reviewed) and `STALE` (everybody
|
||||||
|
reviewed an older head). Fixtures 51 → 68.
|
||||||
|
|
||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
|
- **A label the repo does not have no longer takes the whole edit down with
|
||||||
|
it** — `gh issue edit --add-label` rejects the *entire* call on one unknown
|
||||||
|
name, applying nothing. Batching state and blockers into a single edit (for
|
||||||
|
anti-flicker) meant one missing `blocker:*` would also drop the `state:*`
|
||||||
|
convergence, and the taxonomy was only created by a manual
|
||||||
|
`workflow_dispatch` — so the first sweep after this change would have healed
|
||||||
|
*nothing* on precisely the PRs it exists to fix, surfacing only as a log
|
||||||
|
line. The add side is now filtered against the repo's real label set, read
|
||||||
|
once per sweep. Removals need no filter (they are built from `has_label`, so
|
||||||
|
they provably exist), and an unreadable label set filters *nothing* rather
|
||||||
|
than everything — a failed read must not silently strip the board.
|
||||||
|
|
||||||
|
- **An unreadable check rollup is no longer read as "nothing is failing"** —
|
||||||
|
when `gh pr view` failed, the fallback left the `statusCheckRollup` key
|
||||||
|
absent, and `(.statusCheckRollup // [])` collapsed that into the same `NONE`
|
||||||
|
as a PR that genuinely has no checks. `NONE` blocks nothing, so an API
|
||||||
|
hiccup presented as mergeable-by-a-human — the unknown-certified-as-green
|
||||||
|
shape this machine exists to stop, surviving in the one place the #128 fix
|
||||||
|
never looked. `checks_state` now returns `UNREADABLE` for the absent key,
|
||||||
|
distinct from `NONE` for a present-but-empty array, and the sweep leaves
|
||||||
|
that PR exactly as it is rather than recomputing on facts it did not read.
|
||||||
|
Deliberately *not* a blocker: blocking would flap the whole board on one bad
|
||||||
|
call, and the next tick is 15 minutes away.
|
||||||
|
|
||||||
- **`state:needs-human` no longer appears on PRs a human cannot merge**
|
- **`state:needs-human` no longer appears on PRs a human cannot merge**
|
||||||
(#127, heavy-duty/box#136) — `decide_state()` derived state from three inputs
|
(#127, heavy-duty/box#136) — `decide_state()` derived state from three inputs
|
||||||
(draft flag, requested reviewers, submitted reviews) and read *nothing* about
|
(draft flag, requested reviewers, submitted reviews) and read *nothing* about
|
||||||
|
|
|
||||||
|
|
@ -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: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: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` |
|
| `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
|
`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: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: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
|
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.
|
blocker means the work is the agent's, whatever the review round says.
|
||||||
|
|
|
||||||
|
|
@ -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
|
# ...the partial case is equally stalled: one verdict in, nobody asked for the rest
|
||||||
REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")"
|
REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")"
|
||||||
expect "one bot in, none requested is still unrequested" blocker:unrequested "$(blockers)"
|
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
|
# ...but a live request means an answer IS coming
|
||||||
|
REVIEWS_JSON="$(reviews "$(rev "$BOT1" APPROVED head1 "" t1)")"
|
||||||
REQUESTED="$BOT2"
|
REQUESTED="$BOT2"
|
||||||
expect "a live bot request is not a stalled round" "" "$(blockers)"
|
expect "a live bot request is not a stalled round" "" "$(blockers)"
|
||||||
# ...and a draft is exempt: the bots ignore drafts by design
|
# ...and a draft is exempt: the bots ignore drafts by design
|
||||||
|
|
@ -258,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}'; }
|
'{__typename:"StatusContext", context:$n, state:$s, createdAt:$t}'; }
|
||||||
|
|
||||||
expect "no checks at all is NONE" NONE "$(rollup '[]' | checks_state)"
|
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 \
|
expect "all green is SUCCESS" SUCCESS \
|
||||||
"$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)"
|
"$(rollup "[$(run_ a SUCCESS),$(run_ b SUCCESS)]" | checks_state)"
|
||||||
expect "a queued run is PENDING" PENDING \
|
expect "a queued run is PENDING" PENDING \
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue