fix(labels): a missing state label skips the edit, not the whole PR

Round 2 review (claude-bot, codex-bot — both raised this, independently).

The `return` added in round 1 aborted all of reconcile_pr, not just the
label edit. Everything below it is independent of the state:* taxonomy:
`merge-next` clearing and the stale sweep both stopped running. So a
cold-start repo left `merge-next` claiming "merge this one next" on a PR
the board had moved to the agent — the original false-invitation bug,
reintroduced inside the very fix meant to survive a cold start.

It was also a regression against main, not just a missed improvement:
the old code failed the `gh issue edit`, logged, and fell through to both
blocks. Round 1 turned a per-edit failure into a per-PR abort.

Now a `skip_edit` flag skips only the edit and control reaches the rest.

Also from review: drop the dead `"$desired"` term from the filter loop
(it was appended and then unconditionally continued past), and turn
`[ -n "$missing" ] && log` into a proper `elif` rather than an
&&-as-statement under `set -e`.

Adds the first four fixtures that exercise reconcile_pr itself, stubbing
run/gh to probe a cold-start repo against a bootstrapped one. Everything
before this tested pure functions, which is exactly why a per-PR return
got through: nothing could see it.

Fixtures 68 -> 72.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dan-claude-bot 2026-07-20 18:02:03 +00:00
parent b07e734fc0
commit 5eddf2e2ed
3 changed files with 48 additions and 7 deletions

View file

@ -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

View file

@ -235,7 +235,7 @@ on the way to cutting its first release, and this file starts there.
the new `blocker:*` labels a single missing one would have taken the state 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. 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. Now the state still converges and the missing labels are named in the log.
Fixtures 51 → 68. Fixtures 51 → 72.
- **BREAKING: `--class human|server` is now `--root-door closed|open`** (#77) — - **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, the trait was named for who *lives on* a box; what it decides is one thing,

View file

@ -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 ]