diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index 04c84ef..287463a 100755 --- a/actions/labels-reconcile/labels-reconcile.sh +++ b/actions/labels-reconcile/labels-reconcile.sh @@ -598,6 +598,27 @@ tree_version() { # $1 = ref → that tree's version via the API, or nothing return 0 } +# label_write — every label mutation on this surface goes through +# here (#192). A write that did not happen must reach main's exit code, and the +# first version of this fix marked only the primary state edit: clearing +# `merge-next` and the two `stale` edits could still fail into the generic +# per-PR branch and finish with `reconciled.` and exit 0 +# (@codex-reviewer-andresmgsl). One helper means a future call site cannot +# reopen that by forgetting to mark itself. +# +# The marker is a log line rather than a return code because reconcile_pr runs +# in a subshell whose STDOUT main reads — the same channel the degraded-read +# warning already travels on. +label_write() { + local n="$1" + shift + if run forge_issue_edit "$n" "$@" >/dev/null; then + return 0 + fi + log "#$n: label edit FAILED — attempted: forge_issue_edit $n $*; the write did not happen (reason on stderr above)" + return 1 +} + reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch local n="$1" desired remove s args last_activity last_activity_epoch age @@ -674,11 +695,21 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch 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 forge_issue_edit "$n" "${args[@]}" >/dev/null; then + if label_write "$n" "${args[@]}"; 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. + return 1 fi fi @@ -697,7 +728,7 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch # the moment the PR is no longer the thing a human should merge next, the # claim is removed. Setting it stays with whoever owns the queue. if has_label merge-next && [ "$desired" != state:needs-human ]; then - run forge_issue_edit "$n" --remove-label merge-next >/dev/null + label_write "$n" --remove-label merge-next || return 1 log "#$n: cleared merge-next (state is $desired, not mergeable-by-a-human)" fi @@ -723,11 +754,11 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch # (#50 D10). The 7-day nudge is #52's, once for both surfaces. if has_label blocked || has_label needs-ruling || [ "$age" -le "$STALE_AFTER" ]; then if has_label stale; then - run forge_issue_edit "$n" --remove-label stale >/dev/null + label_write "$n" --remove-label stale || return 1 log "#$n: unstale" fi elif ! has_label stale; then - run forge_issue_edit "$n" --add-label stale >/dev/null + label_write "$n" --add-label stale || return 1 log "#$n: stale ($((age / 3600))h quiet)" fi @@ -773,7 +804,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 +867,33 @@ 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 + # The line must not contain the literal "reconciled." ANYWHERE — "NOT + # reconciled." still does, and a consumer grepping a job-log tail for that + # token would find it after a write that did not happen + # (@codex-reviewer-andresmgsl). The test asserts the whole output is free + # of it, not merely that the success prefix is absent. + log "$write_failures label write(s) attempted did not happen — sweep incomplete" + return 1 + fi log "reconciled." } diff --git a/changelog.d/192.md b/changelog.d/192.md new file mode 100644 index 0000000..8851646 --- /dev/null +++ b/changelog.d/192.md @@ -0,0 +1,47 @@ +### 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 its output carries + no `reconciled.` token at all (#192). + +- Every label mutation goes through one checked helper, so clearing + `merge-next` or either `stale` edit fails the sweep too — not only the + primary state edit (#192). + +- A preserved label keeps the id the issue payload already carried, so + preservation does not depend on a repository-wide list that has nothing to do + with the issue (#192). + +- A removal that changes nothing writes nothing, rather than replacing the set + with itself and opening a race for no state change (#192). + +- Every failure diagnostic on the forgejo backend names the verb as well as the + path and the status. A read used to say `HTTP 500 from 'repos/…'`, which + cannot be told from a failed write of the same path (#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, per 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..0583f86 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -123,7 +123,7 @@ forge_api() { echo "forge_api: request failed: $endpoint" >&2 return 1 fi - forgejo_http_ok "$hdr" "$endpoint" || return 1 + forgejo_http_ok "$hdr" "GET $endpoint" || return 1 if [ "$have_jq" = true ]; then jq -r "$jqexpr" <"$body"; else cat "$body"; fi return 0 fi @@ -140,7 +140,7 @@ forge_api() { echo "forge_api: request failed: $endpoint (page $page)" >&2 return 1 fi - forgejo_http_ok "$hdr" "$endpoint" || return 1 + forgejo_http_ok "$hdr" "GET $endpoint" || return 1 # Re-read on EVERY page, not once (#4712). A board that changes size # under the walk was invisible: page 1 declaring 4 and page 2 declaring @@ -223,9 +223,14 @@ EOF printf '%s\n' "$total" } -# forgejo_http_ok — a non-2xx is named, not +# forgejo_http_ok — a non-2xx is named, not # swallowed. gh exits non-zero on HTTP failure; curl does not without -f, # and -f would throw away the body that says why. +# The second argument carries the VERB as well as the path — "GET repos/…", +# "PUT repos/…". #192's acceptance criterion is that a failure names the verb, +# the path and the status, and reads used to omit the verb: a caller reading +# `HTTP 500 from 'repos/o/r/issues/5'` could not tell a failed read from a +# failed write of the same path (@codex-reviewer-andresmgsl). forgejo_http_ok() { local hdr="$1" endpoint="$2" code code="$(tr -d '\r' <"$hdr" | awk '/^HTTP\// { c = $2 } END { print c }')" @@ -298,25 +303,94 @@ 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_pairs ids id name payload + local want_ids=() missing=() + # nameid straight from the ISSUE payload. Preserved labels carry + # their authoritative id here already, so they need no second lookup — + # re-resolving them through the repository-wide list would make + # preservation depend on a paginated read that has nothing to do with + # this issue, and an incomplete one would drop a bystander + # (@codex-reviewer-andresmgsl). Only ADDED names need forgejo_label_ids. + current="$(forge_api "repos/$REPO/issues/$n" \ + --jq '[.labels[]? | "\(.name)\t\(.id)"] | join("\n")')" || return 1 + # The rows are nameid, so removals filter on the NAME field — a + # whole-line match would never fire against a pair. + want_pairs="$( + awk -F '\t' 'NR==FNR { drop[$0]=1; next } !($1 in drop)' \ + <(printf '%s\n' "${rm_labels[@]}") \ + <(printf '%s\n' "$current" | grep -v '^$') + )" + while IFS=$'\t' read -r name id; do + [ -n "$name" ] || continue + want_ids+=("$id") + done <<<"$want_pairs" + if [ "${#add_labels[@]}" -gt 0 ]; then + ids="$(forgejo_label_ids)" || return 1 + for name in "${add_labels[@]}"; do + # already on the issue? its id is in want_ids already + awk -F '\t' -v want="$name" '$1 == want { found=1 } END { exit !found }' \ + <<<"$want_pairs" && 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 + fi + # An add-label the repo does not carry refuses BEFORE the 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. + 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 + # NOTHING TO CHANGE, NOTHING TO WRITE. The reconcilers call + # --remove-label unconditionally to converge state, so most calls here ask + # to remove a label the issue does not carry. Writing the unchanged set + # back would open the read-modify-write window of ceremony#128 for no + # state change at all; the GET above is already the proof the sweep + # reached the forge (@codex-reviewer-andresmgsl). gh's own behaviour on an + # absent --remove-label is likewise a no-op. + local current_ids + current_ids="$(printf '%s\n' "$current" | grep -v '^$' | cut -f2 | sort -n | tr '\n' ' ')" + if [ "$(printf '%s\n' ${want_ids[@]+"${want_ids[@]}"} | grep -v '^$' | sort -n | tr '\n' ' ')" = "$current_ids" ]; then + return 0 + 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..fd44a84 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -257,9 +257,20 @@ stub_writes() { esac shift done + # FAKE_FAIL_URL + FAKE_HTTP fault-inject one endpoint, so the refusal + # boundaries are driven rather than assumed (#192 review). + if [ -n "${FAKE_FAIL_URL:-}" ] && [ "${url##*"$FAKE_FAIL_URL"}" != "$url" ]; then + printf 'HTTP/1.1 %s Server Error\r\n\r\n' "${FAKE_HTTP:-500}" >"$hdr" + printf '{}' >"$out" + [ "$method" = GET ] || printf '%s %s %s\n' "$method" "${url##*/api/v1/}" "$payload" >>"$WRITES" + return 0 + fi 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 +296,111 @@ 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","id":11},{"name":"scope:labels","id":13}]' \ + 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","id":11},{"name":"scope:labels","id":13},{"name":"attention","id":14}]' \ + 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 that writes NOTHING, +# matching gh: the reconcilers call --remove-label unconditionally to converge +# state, so most calls here ask to remove something absent. Writing the +# unchanged set back would open ceremony#128's read-modify-write window for no +# state change at all, and the GET above is already the proof the sweep reached +# the forge (@codex-reviewer-andresmgsl, #192 review). +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 stub_writes +FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_ISSUE_LABELS='[{"name":"scope:labels","id":13}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +check "removing an absent label succeeds" 0 "" test "$?" -eq 0 +check "...writing nothing at all" 0 "" test ! -s "$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","id":11}]' \ + 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","id":11}]' \ + 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" + +# The preserved-id contract, and the reason it is not merely an optimisation +# (@codex-reviewer-andresmgsl, #192 review): a bystander's id comes from the +# ISSUE payload, so preservation must not depend on a repository-wide list +# that has nothing to do with this issue. Here `attention` is on the issue with +# id 14 and is ABSENT from the repo-list fixture entirely — a resolution that +# went through forgejo_label_ids would refuse or drop it. +PARTIAL_ROSTER='[{"name":"state:old","id":11},{"name":"state:new","id":12},{"name":"scope:labels","id":13}]' +FAKE_LABELS="$PARTIAL_ROSTER" FAKE_LABEL_N=3 stub_writes +FAKE_LABELS="$PARTIAL_ROSTER" FAKE_LABEL_N=3 \ + FAKE_ISSUE_LABELS='[{"name":"state:old","id":11},{"name":"attention","id":14}]' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +check "a bystander absent from the repo list is still preserved by its issue id" 0 \ + '{"labels":[14]}' cat "$WRITES" + +# The two fault boundaries the acceptance plan names. Both must be non-zero +# with the backend's own diagnostic, and neither may report success. +fail_get() { + FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 FAKE_HTTP=500 FAKE_FAIL_URL='/issues/5' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +} +check "a failed current-label GET refuses, non-zero" 1 "" fail_get +check "...naming the verb, the path AND the status, in one diagnostic" 1 \ + "HTTP 500 from 'GET repos/o/r/issues/5'" fail_get +get_write_count() { : >"$WRITES"; fail_get >/dev/null 2>&1; wc -l <"$WRITES"; } +check "...having written nothing: the read failed before any mutation" 0 "0" \ + get_write_count +fail_put() { + FAKE_LABELS="$ROSTER" FAKE_LABEL_N=4 \ + FAKE_ISSUE_LABELS='[{"name":"state:old","id":11},{"name":"scope:labels","id":13}]' \ + FAKE_HTTP=500 FAKE_FAIL_URL='/issues/5/labels' \ + REPO=o/r forge_issue_edit 5 --remove-label state:old +} +check "a failed replacement PUT refuses, non-zero" 1 "" fail_put +check "...naming the verb, the path AND the status, in one diagnostic" 1 \ + "HTTP 500 from 'PUT repos/o/r/issues/5/labels'" fail_put +put_write_count() { : >"$WRITES"; fail_put >/dev/null 2>&1; wc -l <"$WRITES"; } +check "...having attempted only the one PUT" 0 "1" put_write_count + +# 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","id":13}]' \ + 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..d87f8bb 100755 --- a/test/labels-reconcile.test.sh +++ b/test/labels-reconcile.test.sh @@ -969,6 +969,132 @@ 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 the output contains no 'reconciled.' token anywhere" \ + no "$(grep -qF '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)" + +# The tally must catch EVERY label mutation, not only the primary state edit. +# `stale` is written from a different call site; before the one-helper change +# a failure there fell into the generic per-PR branch and the sweep still +# exited 0 (@codex-reviewer-andresmgsl, #192 review). +stale_fail_probe() { + ( + GITHUB_EVENT_NAME=schedule + REPO=owner/repo + LABELS_CONF=.github/labels.conf + CEREMONY_FORGE=github + # 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' 501; 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 + # only the `stale` write fails; the primary state edit succeeds, so + # this probe proves the NON-primary site reaches the tally + case "$*" in + *stale*) printf 'forge_api: HTTP 500 from PUT repos/owner/repo/issues/501/labels\n' >&2; return 1 ;; + esac + return 0 + fi + case "$*" in + */pulls/501) jq -n '{draft:false,user:{login:"author"},head:{sha:"h"},base:{sha:"b"}, + labels:[{name:"stale"},{name:"blocked"}],requested_reviewers:[], + created_at:"2026-07-01T00:00:00Z"}' ;; + *) printf '[]\n' ;; + esac + } + main + ) +} +sf_rc=0 +sf_out="$(stale_fail_probe 2>&1)" || sf_rc=$? +expect "a failed NON-primary label write also fails the sweep" 1 "$sf_rc" +expect "...with no 'reconciled.' token in the output" \ + no "$(grep -qF 'reconciled.' <<<"$sf_out" && echo yes || echo no)" +expect "...naming the attempt that did not happen" \ + yes "$(grep -q 'label edit FAILED' <<<"$sf_out" && echo yes || echo no)" + # --------------------------------------------------------------------------- # outstanding_requests — the portable "who still owes a verdict" (#188 term 4) #