From 5eddf2e2edf03bcddf9bee7e3e46820d4e8d477a Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 18:02:03 +0000 Subject: [PATCH] fix(labels): a missing state label skips the edit, not the whole PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/scripts/labels-reconcile.sh | 20 +++++++++++------ CHANGELOG.md | 2 +- test/labels-reconcile.sh | 33 +++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index 9986dad..1cec82a 100644 --- a/.github/scripts/labels-reconcile.sh +++ b/.github/scripts/labels-reconcile.sh @@ -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 # 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//,/ } "$desired"; do - [ "$want" = "$desired" ] && continue + 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 — run the workflow manually to bootstrap" - return + 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 - [ -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 [ "$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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 8aaabd1..9f0a755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 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 → 68. + 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, diff --git a/test/labels-reconcile.sh b/test/labels-reconcile.sh index 5970c08..cce4691 100644 --- a/test/labels-reconcile.sh +++ b/test/labels-reconcile.sh @@ -379,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 ]