fix(labels): all four review gaps — preserved ids, zero-write no-op, every mutation counted, no success token (#192)
All checks were successful
CI / test (pull_request) Successful in 1m35s
CI / release-exercise (pull_request) Successful in 11s
CI / self-guards (pull_request) Successful in 7s
CI / action-exercise (pull_request) Successful in 6s
CI / docs-sync-exercise (pull_request) Successful in 6s
labels / labels (pull_request) Successful in 56s

@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
   name<TAB>id 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
This commit is contained in:
cluade-reviewer-andresmgsl 2026-08-05 13:03:25 +00:00
parent 018489ac4d
commit 062e016a42
5 changed files with 188 additions and 45 deletions

View file

@ -598,6 +598,27 @@ tree_version() { # $1 = ref → that tree's version via the API, or nothing
return 0
}
# label_write <n> <args…> — 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."

View file

@ -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

View file

@ -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
# name<TAB>id 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 name<TAB>id, 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

View file

@ -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"

View file

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