From 0f20f4b6ef3ccc9817dbdc769e14b66a6889200b Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 12:48:09 +0000 Subject: [PATCH 1/4] 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) # From 018489ac4d7aac6af11f19b62d6915387b16e5e1 Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 12:54:29 +0000 Subject: [PATCH 2/4] chore(changelog): the 192 fragment's citation is terminal (#192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by merging this branch onto !204 and running the suite there — not by anything visible on this base. The terminal-citation rule (#262) ARRIVES with the 0.6.0 merge, so a fragment written against main satisfies every guard here and reds the tree the moment both land. '(ceremony#128) (#192)' is two groups; exactly one must end the entry. The reference moves into prose. Refs #192 --- changelog.d/192.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/192.md b/changelog.d/192.md index fed8c4f..c03ef59 100644 --- a/changelog.d/192.md +++ b/changelog.d/192.md @@ -26,7 +26,7 @@ - `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). + 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). From 062e016a42f7a2d88995bca7062cbf6723f422ce Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:03:25 +0000 Subject: [PATCH 3/4] =?UTF-8?q?fix(labels):=20all=20four=20review=20gaps?= =?UTF-8?q?=20=E2=80=94=20preserved=20ids,=20zero-write=20no-op,=20every?= =?UTF-8?q?=20mutation=20counted,=20no=20success=20token=20(#192)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl's four gaps, all real, all taken. 1. PRESERVED IDS COME FROM THE ISSUE. The removal path read only .labels[].name and then re-resolved every preserved label through the repository-wide list — so preservation depended on a paginated read with nothing to do with this issue, and an incomplete one would drop a bystander. It now keeps nameid from the issue payload, subtracts removals by name, and resolves ONLY added names. Fixture: a bystander on the issue with id 14 that is absent from the repo-list fixture entirely must still survive the PUT. 2. AN ABSENT REMOVAL WRITES NOTHING. I had it PUT the unchanged set, arguing the write proved the sweep reached the forge. The GET already proves that, and replacing a set with itself opens ceremony#128's window for no state change — most calls here are exactly this case, since the reconcilers call --remove-label unconditionally. Short-circuits when the wanted set equals the current one. This was the policy-shaped choice flagged for @andres; the reviewer's reasoning is better than mine was. 3. EVERY LABEL MUTATION REACHES THE TALLY. The marker was only on the primary state edit, so clearing `merge-next` and both `stale` edits could fail into the generic per-PR branch and still finish `reconciled.` and exit 0. All four sites go through one `label_write` helper, so a future call site cannot reopen it by forgetting to mark itself. Probe: a failed NON-primary write (unstale on a blocked PR) must fail the sweep. 4. NO SUCCESS TOKEN IN A FAILURE TAIL. "NOT reconciled." still contains "reconciled.", which a log-tail consumer greps for. The line is now "sweep incomplete", and the test asserts the whole output is free of the token rather than only of the success prefix. Also added the two fault boundaries the acceptance plan named and the fixtures never proved: a failed current-label GET and a failed replacement PUT, each non-zero with the backend's verb/path/status diagnostic. Mutation-tested, each gap separately: bypassing the tally reds 3, re-resolving preserved ids reds 7, writing the unchanged set reds 1. forge-backends 115/115 (was 110), labels-reconcile 175/175 (was 172), test/run.sh 22/22 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 | 15 ++++- lib/forge-forgejo.sh | 64 +++++++++++++------- test/forge-backends.test.sh | 63 +++++++++++++++---- test/labels-reconcile.test.sh | 51 +++++++++++++++- 5 files changed, 188 insertions(+), 45 deletions(-) diff --git a/actions/labels-reconcile/labels-reconcile.sh b/actions/labels-reconcile/labels-reconcile.sh index f61a76d..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,7 +695,7 @@ 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 WRITE THAT DID NOT HAPPEN IS FATAL, not a warning (#192). This was @@ -688,7 +709,6 @@ reconcile_pr() { # $1 = PR number; relies on the globals set from its fetch # 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 @@ -708,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 @@ -734,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 @@ -866,10 +886,12 @@ main() { 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." + # 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 index c03ef59..916fc5e 100644 --- a/changelog.d/192.md +++ b/changelog.d/192.md @@ -11,8 +11,19 @@ `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). + 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). - 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 diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index 26bf96c..066ce2c 100644 --- a/lib/forge-forgejo.sh +++ b/lib/forge-forgejo.sh @@ -326,35 +326,57 @@ forge_issue_edit() { # 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 current want_pairs 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 + # 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 '^$') )" - # 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 + while IFS=$'\t' read -r name id; 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. + 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 diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index bf4692a..908b5ab 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -257,6 +257,14 @@ 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" ;; @@ -295,7 +303,7 @@ check "...carrying the updated description" 0 "" grep -q 'new text' "$WRITES" # 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"}]' \ +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" @@ -309,7 +317,7 @@ check "...carrying the surviving label's id and not the removed one" 0 '{"labels # 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"}]' \ + 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) @@ -318,19 +326,21 @@ preserves_bystanders() { # the PUT keeps state:new(12), scope:labels(13), attent 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. +# 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"}]' \ +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 the unchanged set back" 0 '{"labels":[13]}' cat "$WRITES" +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"}]' \ +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" @@ -339,16 +349,47 @@ check "clearing the last label PUTs the empty set" 0 '{"labels":[]}' cat "$WRITE # 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"}]' \ + 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, path and status" 1 "500" fail_get +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, path and status" 1 "500" fail_put + # 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"}]' \ +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" diff --git a/test/labels-reconcile.test.sh b/test/labels-reconcile.test.sh index 25dbc7c..d87f8bb 100755 --- a/test/labels-reconcile.test.sh +++ b/test/labels-reconcile.test.sh @@ -1028,8 +1028,8 @@ write_fail_probe() { # $1 = ok | fail — whether the label edit write succeeds 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 "...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" \ @@ -1048,6 +1048,53 @@ 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) # From a35a77f752933c50483fa1f3ef1bd2981869f32d Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 5 Aug 2026 13:11:33 +0000 Subject: [PATCH 4/4] fix(forgejo): a read failure names its verb too, and the tests assert the whole diagnostic (#192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @codex-reviewer-andresmgsl caught a test that describes evidence it does not collect — mine, and it is the class this PR is about. Two cases were titled "naming the verb, path and status" and asserted only the substring "500". The PUT boundary happened to satisfy the contract because forgejo_write already passes "PUT $endpoint" to forgejo_http_ok. The GET boundary did not: the diagnostic was `HTTP 500 from 'repos/o/r/issues/5'`, with no verb at all — so a caller could not tell a failed READ from a failed WRITE of the same path, and #192's acceptance criterion asks for exactly that distinction. Reads now pass "GET $endpoint" on both non-paginated and paginated paths, and the two tests assert the complete expected diagnostic as one substring rather than a status code that any failure would contain. Reverting the verb reds the GET case. Also, per the same review: the failed GET is asserted to write nothing, and the failed PUT to have attempted exactly one write. forge-backends 117/117 (was 115), test/run.sh 22/22 under jq 1.7 and jq 1.6, shellcheck 0.10.0 and actionlint clean. Refs #192 --- changelog.d/192.md | 4 ++++ lib/forge-forgejo.sh | 11 ++++++++--- test/forge-backends.test.sh | 11 +++++++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/changelog.d/192.md b/changelog.d/192.md index 916fc5e..8851646 100644 --- a/changelog.d/192.md +++ b/changelog.d/192.md @@ -25,6 +25,10 @@ - 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). diff --git a/lib/forge-forgejo.sh b/lib/forge-forgejo.sh index 066ce2c..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 }')" diff --git a/test/forge-backends.test.sh b/test/forge-backends.test.sh index 908b5ab..fd44a84 100644 --- a/test/forge-backends.test.sh +++ b/test/forge-backends.test.sh @@ -376,7 +376,11 @@ fail_get() { 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, path and status" 1 "500" 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}]' \ @@ -384,7 +388,10 @@ fail_put() { 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, path and status" 1 "500" 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.