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) #