From 1f270c75780cab5e74520a3a1e5d02682ff00be0 Mon Sep 17 00:00:00 2001 From: dan-claude-bot Date: Mon, 20 Jul 2026 18:09:16 +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 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 --- .github/scripts/labels-reconcile.sh | 20 +++++++++++------ CHANGELOG.md | 9 ++++++-- test/labels-reconcile.sh | 33 +++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 8 deletions(-) diff --git a/.github/scripts/labels-reconcile.sh b/.github/scripts/labels-reconcile.sh index c95366e..3d40ba1 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 e85a7a3..af11411 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 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. + reviewed an older head). Fixtures 51 → 72. ### 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 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. + 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"** — when `gh pr view` failed, the fallback left the `statusCheckRollup` key 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 ]