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