fix(labels): a missing state label skips the edit, not the whole PR
The round-1 label pre-flight returned out of reconcile_pr when the desired state:* label did not exist. That stranded the two things the function still owed and which depend on no part of the state:* taxonomy: clearing a stale merge-next, and the staleness sweep. A `merge-next` claim reading "merge this one next" then survived on a PR the board had moved to the agent, and the stale detector went quiet entirely. This was a regression against main, not a missed improvement: main fails the edit, logs, and falls THROUGH to both blocks. The pre-flight turned a per-edit failure into a per-PR abort — and it was reachable without anyone deleting anything, since a repo adopting this script before its first bootstrap has no state:* labels at all. Now a flag skips only the edit and control reaches the rest of the function. Also taken, both from review: the dead "$desired" term in the filter loop (it was appended and then unconditionally skipped, being checked separately), and `[ -n "$missing" ] && log` becomes a proper elif rather than an &&-as-statement under set -e. Four new fixtures drive reconcile_pr itself with `run` and `gh` stubbed — the first in this suite to reach past the pure functions, which is precisely why a per-PR return was invisible to it. Restoring the return fails exactly those two cold-start assertions and none of the other 70. Fixtures 68 -> 72. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
f51ef29b79
commit
1f270c7578
3 changed files with 54 additions and 8 deletions
20
.github/scripts/labels-reconcile.sh
vendored
20
.github/scripts/labels-reconcile.sh
vendored
|
|
@ -365,21 +365,29 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch
|
||||||
# Removals need no filter: they are built from has_label, so the label
|
# Removals need no filter: they are built from has_label, so the label
|
||||||
# provably exists. REPO_LABELS unreadable means no filtering rather than
|
# provably exists. REPO_LABELS unreadable means no filtering rather than
|
||||||
# filtering everything out — a failed read must not silently strip the board.
|
# filtering everything out — a failed read must not silently strip the board.
|
||||||
|
local skip_edit=false
|
||||||
if [ -n "${REPO_LABELS:-}" ]; then
|
if [ -n "${REPO_LABELS:-}" ]; then
|
||||||
local kept="" missing="" want
|
local kept="" missing="" want
|
||||||
for want in ${add//,/ } "$desired"; do
|
for want in ${add//,/ }; do
|
||||||
[ "$want" = "$desired" ] && continue
|
|
||||||
if grep -qxF "$want" <<<"$REPO_LABELS"; then kept="$kept,$want"
|
if grep -qxF "$want" <<<"$REPO_LABELS"; then kept="$kept,$want"
|
||||||
else missing="$missing $want"; fi
|
else missing="$missing $want"; fi
|
||||||
done
|
done
|
||||||
add="${kept#,}"
|
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
|
if ! grep -qxF "$desired" <<<"$REPO_LABELS"; then
|
||||||
log "#$n: WARNING: state label '$desired' does not exist — run the workflow manually to bootstrap"
|
log "#$n: WARNING: state label '$desired' does not exist — skipping the label edit; dispatch the workflow to bootstrap"
|
||||||
return
|
skip_edit=true
|
||||||
|
elif [ -n "$missing" ]; then
|
||||||
|
log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap"
|
||||||
fi
|
fi
|
||||||
[ -n "$missing" ] && log "#$n: WARNING: missing label(s)$missing — state still converged; dispatch the workflow to bootstrap"
|
|
||||||
fi
|
fi
|
||||||
if ! has_label "$desired" || [ -n "$remove" ] || [ -n "$add" ]; then
|
if [ "$skip_edit" = false ] && { ! 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")
|
||||||
if run gh issue edit "$n" -R "$REPO" "${args[@]}" >/dev/null; then
|
if run gh issue edit "$n" -R "$REPO" "${args[@]}" >/dev/null; then
|
||||||
|
|
|
||||||
|
|
@ -45,7 +45,7 @@ actually cutting it, and this file starts there.
|
||||||
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. A verdict is owed in two shapes and both raise
|
nothing recomputes. A verdict is owed in two shapes and both raise
|
||||||
`blocker:unrequested`: `MISSING` (nobody reviewed) and `STALE` (everybody
|
`blocker:unrequested`: `MISSING` (nobody reviewed) and `STALE` (everybody
|
||||||
reviewed an older head). Fixtures 51 → 68.
|
reviewed an older head). Fixtures 51 → 72.
|
||||||
|
|
||||||
### Fixed
|
### Fixed
|
||||||
|
|
||||||
|
|
@ -59,7 +59,12 @@ actually cutting it, and this file starts there.
|
||||||
line. The add side is now filtered against the repo's real label set, read
|
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
|
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
|
they provably exist), and an unreadable label set filters *nothing* rather
|
||||||
than everything — a failed read must not silently strip the board.
|
than everything — a failed read must not silently strip the board. A missing
|
||||||
|
*state* label skips only the label edit, not the rest of the PR: clearing a
|
||||||
|
stale `merge-next` and the staleness sweep depend on no part of the `state:*`
|
||||||
|
taxonomy, and a cold-start repo that skipped them would leave "merge this one
|
||||||
|
next" sitting on a PR the board had moved to the agent — the same false
|
||||||
|
invitation, one scope smaller.
|
||||||
|
|
||||||
- **An unreadable check rollup is no longer read as "nothing is failing"** —
|
- **An unreadable check rollup is no longer read as "nothing is failing"** —
|
||||||
when `gh pr view` failed, the fallback left the `statusCheckRollup` key
|
when `gh pr view` failed, the fallback left the `statusCheckRollup` key
|
||||||
|
|
|
||||||
|
|
@ -379,5 +379,38 @@ DRAFT=true MERGEABLE=CONFLICTING
|
||||||
expect "a draft is building even when conflicted" state:building "$(decide_state)"
|
expect "a draft is building even when conflicted" state:building "$(decide_state)"
|
||||||
DRAFT=false MERGEABLE=MERGEABLE CHECKS=SUCCESS REQUESTED="" REVIEWS_JSON='[]'
|
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"
|
printf 'labels-reconcile tests: %d passed, %d failed\n' "$pass" "$fail"
|
||||||
[ "$fail" -eq 0 ]
|
[ "$fail" -eq 0 ]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue