From 0f20f4b6ef3ccc9817dbdc769e14b66a6889200b Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 12:48:09 +0000 Subject: [PATCH] fix(labels): a label removal that cannot happen fails the sweep, and removal itself now works (#192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects, one cause, and the second is why the first survived a week. THE WRITE. Removal was a per-label `DELETE .../labels/{id}` loop. On this instance that call returns HTTP 500 for every removal under the token the sweep actually holds — measured inside Actions, probe run 701, where the same `PUT .../labels` with the desired full set returns 200 including the empty set for a full clear. A PAT gets 204 on the same DELETE, which is exactly why it went unseen: it fails only for `${{ github.token }}`. Net effect before this: on Forgejo the state machine could only ever ADD labels. Every `state:*` transition needing the previous state cleared and every `blocker:*` that should lift was inert. Both PRs open right now carry stale `blocker:*` labels that are false and that nothing can remove. So the removal path is read-current, compute-wanted, one PUT — the same shape the assignee branch beside it already used. An ADD-ONLY call keeps its additive POST: ceremony#128 lost a `release` label to a read-modify-write that clobbered a concurrent set, and forge_labels_add stays pinned against ever doing that. The window is accepted here and only here, where the caller asked to REMOVE and no additive verb can say that. An unresolvable --add-label refuses before any write, so a replacement PUT can never drop a label nobody asked to remove. THE REPORTING. `labels-reconcile` logged `WARNING: label edit failed`, fell through, and `main` printed `reconciled.` and exited 0 — while `issueflow-reconcile` treated the identical 500 as fatal. One cause, two contradictory policies, and the wrong one hid the write defect. A failed write is fatal now, and the tally reaches main's exit code. That second half is load-bearing: making reconcile_pr fatal alone is not enough, because the loop swallows a per-PR non-zero into a log line and finishes. The per-PR tolerance is right and stays — one bad PR must not blind the board — but it now applies to READS. A sweep that could not write exits non-zero and never prints `reconciled.` The diagnostic says what was attempted and that it did not happen. The old text blamed a missing label and told the operator to bootstrap, when the label was present and the call returned 500 — #101's rule is report, do not diagnose. Mutation-tested, all three ways: restoring the warn-and-continue reds 5 cases, removing the tally reds 2, restoring the DELETE loop reds 7. test/run.sh 22 files 0 failed under jq 1.7 and jq 1.6; shellcheck 0.10.0 and actionlint clean. Refs #192 --- actions/labels-reconcile/labels-reconcile.sh | 40 +++++++++- changelog.d/192.md | 32 ++++++++ lib/forge-forgejo.sh | 75 +++++++++++++++---- test/forge-backends.test.sh | 79 +++++++++++++++++--- test/labels-reconcile.test.sh | 79 ++++++++++++++++++++ 5 files changed, 275 insertions(+), 30 deletions(-) create mode 100644 changelog.d/192.md diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index 04c84ef..f61a76d 100755 --- a/actions/labels-reconcile/labels-reconcile.sh +++ b/actions/labels-reconcile/labels-reconcile.sh @@ -677,8 +677,19 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch if run forge_issue_edit "$n" "${args[@]}" >/dev/null; then log "#$n: state -> $desired${add:+ +$add}${remove:+ (cleared $remove)}" else - # a deleted label must not wedge the sweep — dispatch heals the taxonomy - log "#$n: WARNING: label edit failed (missing label? run the workflow manually to bootstrap)" + # A WRITE THAT DID NOT HAPPEN IS FATAL, not a warning (#192). This was + # `log WARNING` and fell through, so the sweep printed `reconciled.` and + # exited green over an edit the forge had refused — the + # degraded-write-reports-success class #188 exists to eliminate, + # surviving inside the reconciler that reports it. + # + # The old text also diagnosed a cause it had not established: it named a + # missing label and told the operator to bootstrap, when the label was + # present and the call had returned 500. #101's rule is report, do not + # diagnose — so this says what was attempted and that it did not happen, + # and leaves the backend's own stderr to say why. + log "#$n: label edit FAILED — attempted: forge_issue_edit $n ${args[*]}; the write did not happen (reason on stderr above)" + return 1 fi fi @@ -773,7 +784,7 @@ main() { [ -z "$REPO_LABELS" ] && log "WARNING: could not read the label set — applying labels unfiltered" missing_core_labels_warning "$(core_label_rows)" "$REPO_LABELS" - local n output status total=0 unreadable=0 sampled_reason="" + local n output status total=0 unreadable=0 write_failures=0 sampled_reason="" while IFS= read -r n; do [ -n "$n" ] || continue total=$((total + 1)) @@ -836,10 +847,31 @@ main() { sampled_reason="$(sed -n "s/^labels: #$n: read failed: //p" <<<"$output" | head -n1)" fi elif [ "$status" -ne 0 ]; then - log "#$n: reconcile failed — continuing with the remaining PRs" + # The per-PR tolerance is right and stays: one bad PR must not blind the + # sweep over the rest of the board. What was missing is the sweep-level + # accounting — a failed WRITE has to reach main's exit code, or a builder + # satisfies every task and the sweep still prints `reconciled.` over an + # edit that never happened (#192, @kimi-reviewer-andresmgsl #5189). + # + # Reads stay tolerated: an unreadable fact is already reported by the + # blind-sweep warning and leaves the board untouched. A write is + # different — the board and the tree now disagree. + if grep -q "^labels: #$n: label edit FAILED" <<<"$output"; then + write_failures=$((write_failures + 1)) + log "#$n: reconcile failed on a WRITE — continuing the sweep, but it will not report success" + else + log "#$n: reconcile failed — continuing with the remaining PRs" + fi fi done < <(forge_pr_list) blind_sweep_warning "$unreadable" "$total" "$sampled_reason" + if [ "$write_failures" -gt 0 ]; then + # Deliberately NOT the string "reconciled." — tests pin that exact word and + # a consumer reading the tail of a job log must not find it after a write + # that did not happen. + log "$write_failures label write(s) this sweep attempted did not happen — NOT reconciled." + return 1 + fi log "reconciled." } diff --git a/changelog.d/192.md b/changelog.d/192.md new file mode 100644 index 0000000..fed8c4f --- /dev/null +++ b/changelog.d/192.md @@ -0,0 +1,32 @@ +### Fixed + +- Label removal on Forgejo is a full-set `PUT`, not a per-label `DELETE`. The + workflow token gets HTTP 500 on every `DELETE .../labels/{id}` on this + instance, so the state machine could only ever ADD labels (#192). + +- Every `state:*` transition that needs the previous state cleared, and every + `blocker:*` that should lift, can now actually clear. They were inert (#192). + +- A label edit that fails is fatal to `labels-reconcile`, matching + `issueflow-reconcile`. One cause had two contradictory policies (#192). + +- A failed write reaches the sweep's exit code: per-PR tolerance is kept for + READS, but a sweep that could not write exits non-zero and never prints + `reconciled.` (#192). + +- The diagnostic names what was attempted and that it did not happen, instead + of blaming a missing label and telling the operator to bootstrap — a cause it + had not established (#192, #101). + +- An add-label the repo does not carry refuses before any write, so a + replacement `PUT` can never drop a label nobody asked to remove (#192). + +### Added + +- `test/forge-backends.test.sh` pins the replacement contract: preserve + unrelated labels across a combined add+remove, an absent removal as a + successful no-op, the empty set as a full clear, and `forge_labels_add` + still `POST`-only (ceremony#128) (#192). + +- `test/labels-reconcile.test.sh` drives a failing write through `main()` — the + swallow was in the loop, where a fixture-level probe cannot reach (#192). diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index 426776a..26bf96c 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -298,25 +298,72 @@ forge_issue_edit() { shift done - if [ "${#add_labels[@]}" -gt 0 ]; then + # THE LABEL DELTA (#192). Removal used to be a per-label + # `DELETE .../labels/{id}` loop. On this instance that call returns HTTP 500 + # for EVERY removal under the token the sweep actually holds — measured + # under a real Actions token inside a workflow, probe run 701: + # + # POST /issues/{n}/labels ["probe-a","probe-b"] -> 200 + # DELETE /issues/{n}/labels/{id} -> 500 labels unchanged + # PUT /issues/{n}/labels {"labels":[]} -> 200 + # PUT /issues/{n}/labels {"labels":[]} -> 200 (full clear) + # + # A PAT gets 204 on the same DELETE, which is why this survived a week + # unseen: it fails only for `${{ github.token }}`, and only inside Actions. + # Net effect before this fix: on Forgejo the state machine could only ever + # ADD labels — every `state:*` transition needing the previous state cleared, + # and every `blocker:*` that should lift, was inert. + # + # So a removal is expressed as a full-set PUT, exactly as the assignee branch + # below expresses its own delta as one PATCH — read current, compute wanted, + # write once. + # + # AN ADD-ONLY CALL KEEPS ITS ADDITIVE POST, deliberately. ceremony#128 lost + # its `release` label — the merge door's declared-intent read — to a + # read-modify-write that clobbered a label set two seconds after a builder + # wrote it, and `forge_labels_add` is pinned against ever doing that + # (test/forge-backends.test.sh). The read-modify-write window is real and is + # accepted HERE and only here, where the caller has asked to REMOVE something + # and no additive verb can express that. + if [ "${#rm_labels[@]}" -gt 0 ]; then + local current want ids id name payload + local want_ids=() missing=() + current="$(forge_api "repos/$REPO/issues/$n" --jq '[.labels[]?.name] | join("\n")')" || return 1 + want="$( + { + printf '%s\n' "$current" + [ "${#add_labels[@]}" -gt 0 ] && printf '%s\n' "${add_labels[@]}" + } | grep -v '^$' | sort -u + )" + # A label the issue does not carry is not an error: the reconcilers call + # --remove-label unconditionally to converge state, and gh's own behaviour + # there is a no-op. Subtracting a name that is not in `want` is exactly + # that no-op, and the PUT below then writes the set back unchanged. + want="$(grep -vxF -f <(printf '%s\n' "${rm_labels[@]}") <<<"$want" || true)" + ids="$(forgejo_label_ids)" || return 1 + while IFS= read -r name; do + [ -n "$name" ] || continue + id="$(awk -F '\t' -v want="$name" '$1 == want { print $2; exit }' <<<"$ids")" + if [ -z "$id" ]; then missing+=("$name"); continue; fi + want_ids+=("$id") + done <<<"$want" + # Every wanted name must resolve BEFORE the write. A PUT that silently + # dropped an unresolvable one would remove a label nobody asked to remove — + # a destructive write dressed as a partial success, which is the class this + # whole issue is about. + if [ "${#missing[@]}" -gt 0 ]; then + echo "forge_issue_edit: #$n: no label id on $REPO for: ${missing[*]} — refusing to PUT a set that would drop it" >&2 + return 1 + fi + payload="$(printf '%s\n' ${want_ids[@]+"${want_ids[@]}"} \ + | jq -R 'select(. != "") | tonumber' | jq -sc '{labels: .}')" + forgejo_write PUT "repos/$REPO/issues/$n/labels" "$payload" >/dev/null || return 1 + elif [ "${#add_labels[@]}" -gt 0 ]; then local payload payload="$(printf '%s\n' "${add_labels[@]}" | jq -R . | jq -sc '{labels: .}')" forgejo_write POST "repos/$REPO/issues/$n/labels" "$payload" >/dev/null || return 1 fi - if [ "${#rm_labels[@]}" -gt 0 ]; then - local ids id name - ids="$(forgejo_label_ids)" || return 1 - for name in "${rm_labels[@]}"; do - id="$(awk -F '\t' -v want="$name" '$1 == want { print $2; exit }' <<<"$ids")" - # A label the repo does not have is not an error: the reconcilers call - # --remove-label unconditionally to converge state, and gh's own - # behaviour there is a no-op. - [ -n "$id" ] || continue - forgejo_write DELETE "repos/$REPO/issues/$n/labels/$id" '' >/dev/null || return 1 - done - fi - if [ "${#add_assignees[@]}" -gt 0 ] || [ "${#rm_assignees[@]}" -gt 0 ]; then local current want payload current="$(forge_api "repos/$REPO/issues/$n" --jq '[.assignees[]?.login] | join("\n")')" || return 1 diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index d59f39e..bf4692a 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -260,6 +260,9 @@ stub_writes() { printf 'HTTP/1.1 200 OK\r\nX-Total-Count: %s\r\n\r\n' "${FAKE_LABEL_N:-1}" >"$hdr" case "$url" in *"/labels?"* | */labels) printf '%s' "${FAKE_LABELS:-[]}" >"$out" ;; + # The issue itself: the removal path reads its CURRENT label set before + # computing the set to PUT (#192). + */issues/[0-9]*) printf '{"labels": %s}' "${FAKE_ISSUE_LABELS:-[]}" >"$out" ;; *) printf '{}' >"$out" ;; esac [ "$method" = GET ] || printf '%s %s %s\n' "$method" "${url##*/api/v1/}" "$payload" >>"$WRITES" @@ -285,19 +288,71 @@ check "...carrying the updated description" 0 "" grep -q 'new text' "$WRITES" # #4751 item 2). Live scratch-repo evidence proved these work; these prove # they keep working, and pin the SHAPE of the requests. -# Removal resolves name -> id, because Forgejo takes names on add and only a -# numeric id on remove. Measured: DELETE .../labels/probe:one -> 422, -# DELETE .../labels/149 -> 204. -FAKE_LABELS='[{"name":"stale","id":11},{"name":"ready","id":12}]' FAKE_LABEL_N=2 stub_writes -FAKE_LABELS='[{"name":"stale","id":11},{"name":"ready","id":12}]' FAKE_LABEL_N=2 REPO=o/r forge_issue_edit 5 --remove-label stale -check "removing a label resolves its numeric id" 0 "" grep -q '^DELETE repos/o/r/issues/5/labels/11 ' "$WRITES" -check "...and never sends the name as the path segment" 1 "" grep -q 'labels/stale' "$WRITES" +# Removal is a FULL-SET PUT, not a per-label DELETE (#192). Measured under a +# real Actions token, probe run 701: DELETE .../labels/{id} -> 500 for every +# removal, PUT .../labels -> 200 including the empty set. A PAT gets 204 on the +# same DELETE, which is why it went unseen — it fails only for the identity the +# sweep holds. +ROSTER='[{"name":"state:old","id":11},{"name":"state:new","id":12},{"name":"scope:labels","id":13},{"name":"attention","id":14}]' +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_ISSUE_LABELS='[{"name":"state:old"},{"name":"scope:labels"}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +check "removing a label PUTs the whole wanted set" 0 "" \ + grep -q '^PUT repos/o/r/issues/5/labels ' "$WRITES" +check "...and never DELETEs, which this instance answers 500" 1 "" \ + grep -q '^DELETE ' "$WRITES" +check "...carrying the surviving label's id and not the removed one" 0 '{"labels":[13]}' \ + cat "$WRITES" -# A label the repo does not have is a no-op, matching gh: the reconcilers -# call --remove-label unconditionally to converge state. -FAKE_LABELS='[{"name":"ready","id":12}]' FAKE_LABEL_N=1 stub_writes -FAKE_LABELS='[{"name":"ready","id":12}]' FAKE_LABEL_N=1 REPO=o/r forge_issue_edit 5 --remove-label nonexistent -check "removing an absent label writes nothing" 0 "" test ! -s "$WRITES" +# The contract @codex-reviewer-andresmgsl asked for (#5183): a full-set PUT +# replaces everything, so removal alone proves nothing about PRESERVATION. One +# call, a combined delta, and two bystanders that must survive it. +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 \ + FAKE_ISSUE_LABELS='[{"name":"state:old"},{"name":"scope:labels"},{"name":"attention"}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old --add-label state:new +check "a combined add+remove is ONE write" 0 "" test "$(wc -l <"$WRITES")" -eq 1 +preserves_bystanders() { # the PUT keeps state:new(12), scope:labels(13), attention(14) + grep -q 12 "$WRITES" && grep -q 13 "$WRITES" && grep -q 14 "$WRITES" +} +check "...and preserves every unrelated label" 0 "" preserves_bystanders +check "...while dropping only what was asked for" 1 "" grep -qE '(^|[^0-9])11([^0-9]|$)' "$WRITES" + +# A label the issue does not carry is a successful no-op, matching gh: the +# reconcilers call --remove-label unconditionally to converge state. The set +# goes back unchanged rather than nothing being written — the write is what +# proves the sweep reached the forge. +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_ISSUE_LABELS='[{"name":"scope:labels"}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +check "removing an absent label succeeds" 0 "" test "$?" -eq 0 +check "...writing the unchanged set back" 0 '{"labels":[13]}' cat "$WRITES" + +# A full clear is the empty set, which this instance answers 200 (run 701). +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_ISSUE_LABELS='[{"name":"state:old"}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +check "clearing the last label PUTs the empty set" 0 '{"labels":[]}' cat "$WRITES" + +# An add-label the repo does not have must refuse BEFORE any write: a PUT that +# silently dropped an unresolvable name would remove a label nobody asked to +# remove — a destructive write dressed as a partial success. +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +edit_unknown_add() { + FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_ISSUE_LABELS='[{"name":"state:old"}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old --add-label no-such-label +} +check "an unknown add-label refuses" 1 "no label id" edit_unknown_add +check "...before writing anything" 0 "" test ! -s "$WRITES" + +# An ADD-ONLY call keeps the additive POST (ceremony#128): a read-modify-write +# there clobbered a label set two seconds after a builder wrote it. +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_ISSUE_LABELS='[{"name":"scope:labels"}]' \ + REPO=o/r forge_issue_edit 5 --add-label state:new +check "an add-only edit still POSTs additively" 0 "" \ + grep -q '^POST repos/o/r/issues/5/labels ' "$WRITES" +check "...and never PUTs the whole set (ceremony#128)" 1 "" grep -q '^PUT ' "$WRITES" # Adding takes names directly — no lookup, one request. FAKE_LABELS='[]' FAKE_LABEL_N=0 stub_writes diff --git a/test/labels-reconcile.test.sh b/test/labels-reconcile.test.sh index 87f4107..25dbc7c 100755 --- a/test/labels-reconcile.test.sh +++ b/test/labels-reconcile.test.sh @@ -969,6 +969,85 @@ for ev in schedule pull_request_target; do no "$(grep -q '^delete ' "$EXEC/record" && echo yes || echo no)" done +# --------------------------------------------------------------------------- +# A WRITE THAT DID NOT HAPPEN FAILS THE SWEEP (#192) +# +# The defect this replaces: forge_issue_edit returned non-zero, reconcile_pr +# logged `WARNING: label edit failed`, fell through, and main printed +# `reconciled.` and exited 0. On this forge that was every removal — the +# workflow token gets HTTP 500 on DELETE .../labels/{id} — so the board could +# only ever gain labels, and the sweep said it had reconciled. +# +# Driven through main() rather than the pure functions, because the swallow +# was in the LOOP: reconcile_pr's non-zero became a log line and the loop +# finished. A fixture-level probe cannot reach that (#91's lesson). +# --------------------------------------------------------------------------- +write_fail_probe() { # $1 = ok | fail — whether the label edit write succeeds + ( + GITHUB_EVENT_NAME=schedule + REPO=owner/repo + LABELS_CONF=.github/labels.conf + CEREMONY_FORGE=github + WMODE="$1" + # shellcheck disable=SC2317 # reached through the forge backend, not called directly (#188) + gh() { + if [ "$1" = label ] && [ "$2" = list ]; then core_label_rows | cut -d'|' -f1; return 0; fi + if [ "$1" = pr ] && [ "$2" = list ]; then printf '%s\n' 401 402; return 0; fi + if [ "$1" = pr ] && [ "$2" = view ]; then + jq -n '{mergeable:"MERGEABLE", + statusCheckRollup:[{__typename:"CheckRun",workflowName:"ci", + name:"check",conclusion:"SUCCESS", + startedAt:"2026-07-01T00:00:00Z"}]}' + return 0 + fi + if [ "$1" = issue ] && [ "$2" = edit ]; then + # #401's write is the one that fails; #402's succeeds, so the probe + # also proves the sweep KEPT GOING rather than aborting on the first. + if [ "$WMODE" = fail ] && [ "$3" = 401 ]; then + printf 'forge_api: HTTP 500 from DELETE repos/owner/repo/issues/401/labels/93\n' >&2 + return 1 + fi + printf '%s\n' "$*" >>"$RTMP/wedits" + return 0 + fi + case "$*" in + */pulls/401) jq -n '{draft:false,user:{login:"author"},head:{sha:"h"},base:{sha:"b"}, + labels:[{name:"blocker:ci-red"}],requested_reviewers:[], + created_at:"2026-07-01T00:00:00Z"}' ;; + */pulls/402) jq -n '{draft:false,user:{login:"author"},head:{sha:"h"},base:{sha:"b"}, + labels:[],requested_reviewers:[], + created_at:"2026-07-01T00:00:00Z"}' ;; + *) printf '[]\n' ;; + esac + } + main + ) +} + +: >"$RTMP/wedits" +wf_rc=0 +wf_out="$(write_fail_probe fail 2>&1)" || wf_rc=$? +expect "a sweep whose label write failed exits non-zero" 1 "$wf_rc" +expect "...and never prints reconciled." \ + no "$(grep -q 'labels: reconciled\.' <<<"$wf_out" && echo yes || echo no)" +expect "...saying instead that the write did not happen" \ + yes "$(grep -q 'did not happen' <<<"$wf_out" && echo yes || echo no)" +expect "...naming what it attempted, not a cause it has not established" \ + yes "$(grep -q 'attempted: forge_issue_edit 401' <<<"$wf_out" && echo yes || echo no)" +expect "...and asserting no bootstrap diagnosis it cannot support" \ + no "$(grep -qi 'bootstrap' <<<"$wf_out" && echo yes || echo no)" +# The per-PR tolerance is deliberately KEPT: one bad PR must not blind the +# board. #402 is reconciled in the same pass that #401 failed in. +expect "...while still reconciling the rest of the board" \ + yes "$(grep -q '^issue edit 402' "$RTMP/wedits" && echo yes || echo no)" + +: >"$RTMP/wedits" +ok_rc=0 +ok_out="$(write_fail_probe ok 2>&1)" || ok_rc=$? +expect "the control: the same sweep with writes working exits 0" 0 "$ok_rc" +expect "...and does print reconciled." \ + yes "$(grep -q 'labels: reconciled\.' <<<"$ok_out" && echo yes || echo no)" + # --------------------------------------------------------------------------- # outstanding_requests — the portable "who still owes a verdict" (#188 term 4) #