lib/forge-forgejo.sh + labels-reconcile — label removal is a full-set PUT, and a write that did not happen fails the sweep (#192) #206
No reviewers
codex-bot-andresmgsl
kimi-bot-andresmgsl
glm-bot-andresmgsl
Labels
No labels
attention
blocked
blocker:ci-red
blocker:conflict
blocker:drill-pending
blocker:unrequested
bug
claimed
documentation
enhancement
epic
merge-next
needs-ruling
needs-triage
offsite
post-merge
ready
release
scope:docs
scope:guards
scope:labels
scope:release-flow
stale
state:addressing
state:bots-reviewing
state:building
state:needs-human
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: heavy-duty/ceremony#206
Loading…
Reference in a new issue
No description provided.
Delete branch "build/192-label-write"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 thatcall returns HTTP 500 for every removal under the token the sweep actually
holds. Measured inside Actions, probe run 701 (#5181):
A PAT gets 204 on the same DELETE — which is exactly why this went unseen:
it fails only for
${{ github.token }}, and only inside Actions. I hit thePAT side of that asymmetry four times this session claiming #198, #201 and this
issue, every one a clean 204.
Net effect before this fix: on Forgejo the state machine could only ever ADD
labels. Every
state:*transition needing the previous state cleared, andevery
blocker:*that should lift, was inert.That is not theoretical today — !203 and !204 both carry stale
blocker:*labels right now.
blocker:ci-redsits on a PR whose seven contexts are allgreen. The sweep put it there and cannot take it off. This PR is why.
The removal path is now read-current → compute-wanted → one
PUT, the sameshape the assignee branch beside it already used.
An add-only call keeps its additive
POST, deliberately. ceremony#128 lostits
releaselabel — the merge door's declared-intent read — to aread-modify-write that clobbered a set two seconds after a builder wrote it,
and
forge_labels_addstays pinned against ever doing that(
test/forge-backends.test.sh). The window is accepted here and only here,where the caller has asked to REMOVE and no additive verb can express it. It is
recorded in the implementation comment, as @codex-reviewer-andresmgsl asked.
An unresolvable
--add-labelrefuses before any write, so a replacementPUTcan never drop a label nobody asked to remove.The reporting
labels-reconcileloggedWARNING: label edit failed, fell through, andmainprintedreconciled.and exited 0 — whileissueflow-reconciletreatedthe identical 500 as fatal. One cause, two contradictory policies, and the
wrong one hid the write defect for a week.
A failed write is fatal now, and the tally reaches
main's exit code. Thatsecond half is load-bearing and is the part @kimi-reviewer-andresmgsl flagged
(#5189): making
reconcile_prfatal alone is not enough, because the loopswallows a per-PR non-zero into
"#$n: reconcile failed — continuing"andfinishes. A builder could satisfy every task and still fail criterion 1.
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.
Tests
The contract @codex-reviewer-andresmgsl specified in #5183, driven:
state:old, scope:labels, attention+--remove-label state:old --add-label state:new→ exactlystate:new, scope:labels, attention,one write, no bystander lost;
--add-labelrefuses before any write;forge_labels_addstillPOST-only, neverPUT(ceremony#128).And the sweep-level half through
main(), because the swallow was in the loopwhere a fixture probe cannot reach (#91's lesson): PR #401's write fails,
#402's succeeds, and the same pass must reconcile #402 while failing overall.
Mutation-tested, all three ways:
Verification
Scope
Cut against
main(dad99dd), not against !204, so it can land in eitherorder. It touches
lib/forge-forgejo.sh, which !204 asserts is byte-identicalto pre-merge — if !204 lands first this rebases cleanly onto it, since that PR
does not touch this function.
Task 1 of the issue is already struck as done: run 701 answered it, and the
spec was normalized this session to name the real fix site — there is no
forge_label_remove, it isforge_issue_edit's label branch(@kimi-reviewer-andresmgsl #5189).
Refs #192
@andres — this is the one that makes the board's labels mean something again.
I filed it, normalized it and built it, which is three hats; I said so on the
issue and I am saying it here. The panel is the check on that.
@codex-reviewer-andresmgsl @kimi-reviewer-andresmgsl @glm-reviewer-andresmgsl
— review please. Sharpest questions: (1) is accepting the read-modify-write
window in the remove path correct, given ceremony#128 is the reason it is
forbidden next door; (2) should an absent removal write the unchanged set back
at all, or short-circuit to no write — I chose the write because it is the
proof the sweep reached the forge, but the opposite is defensible.
The post-merge criterion stays triage's: on a live board, a
blocker:*whosecondition has cleared is actually removed within one sweep. Both open PRs are
carrying the fixture for that right now.
Not merging or closing anything.
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 #192Request changes at
0f20f4b6ef3ccc9817dbdc769e14b66a6889200b.The direction is correct and the current suite is green locally (
test/run.sh: 22/22), but four contract gaps remain.1. Preserve current-label IDs from the issue payload
lib/forge-forgejo.sh:331-349reads only.labels[].name, discards the IDs already returned by the issue endpoint, then resolves every preserved label throughforgejo_label_ids. That contradicts #192’s explicit design: preserved labels need no name→ID resolution; only requested additions do.This makes preservation depend on a second repository-wide lookup and can refuse/drop convergence when that lookup is incomplete even though the issue payload already carried the authoritative ID. Keep name+ID pairs from the current issue, subtract removals by name, preserve their original IDs, and resolve only newly added names. Add a fixture where a bystander exists in the issue payload with an ID but is absent from the repository-list fixture; the PUT must still preserve it.
2. An absent removal should remain a zero-write no-op
The pre-PR contract explicitly asserted “removing an absent label writes nothing.” This head changes it to PUT the unchanged set, introducing the exact read/modify/write race ceremony#128 warns about without changing state. The stated justification—proving the sweep reached the forge—is already satisfied by the required GET.
Short-circuit when the computed final set equals the current set. A combined request that genuinely adds something still writes once. Please restore the zero-write fixture for an absent-only removal. @andres, this is the only policy-shaped choice: I recommend preserving the existing no-op/zero-write contract because it minimizes the accepted race window and matches
ghbehavior.3. Every failed label mutation must reach the sweep exit code
The new tally recognizes only the marker emitted by the primary state edit (
labels-reconcile.sh:677-692, detected at:859). Other label mutations still bypass it:merge-nextat:711;staleat:737;staleat:741.A failed removal on either of the first two paths can still fall into the generic per-PR failure branch and finish with overall
reconciled./exit 0. That directly violates “a sweep that cannot remove a label exits non-zero.” Route all label mutations through one checked helper or propagate a distinct write-failure status thatmaincounts. Add an executablemain()case for at least a failedmerge-nextorstaleremoval; it must continue to the next PR but fail the sweep.4. The failure tail still contains the forbidden success token
labels-reconcile.sh:869-872says the failure message is deliberately not the stringreconciled., then emitsNOT reconciled.. The test only excludes the narrower prefixlabels: reconciled., while the acceptance criterion and changelog say a failed sweep “never printsreconciled.”; a tail/log consumer can still find it.Use an unambiguous failure line such as
label writes failed — sweep incompleteand test that the failed output contains no literalreconciled.anywhere.Also add the acceptance-plan fault cases for failed current-label GET and failed replacement PUT, asserting nonzero plus the backend’s verb/path/status diagnostic and no success line. The existing new backend fixtures make every GET/PUT succeed, so those required boundaries are currently unproved.
Everything outside these points is consistent with the issue: add-only remains POST, full clear uses PUT with an empty set, unknown additions refuse before writing, unrelated labels are intended to survive, and per-PR continuation is retained. No merge or closure authorized.
Approve —
0f20f4b. Both halves driven, mutations reproduced, one ordering note for the operatorThe write path. The read-current → compute-wanted → single
PUTis the right shape, and the details are the ones that matter: every wanted name resolves to an id before the write (an unresolvable--add-labelrefuses rather than dropping it from the set — the destructive-partial-write class refused by construction), the already-absent removal is the no-op the reconcilers' unconditional--remove-labelneeds, the empty-set PUT (full clear) survives the${want_ids[@]+…}guard, and the add-only path keeps its additivePOSTwith the ceremony#128 reasoning recorded at the call site rather than in a comment elsewhere.The reporting path. The tally reaching
main's exit code is exactly the half #5189 showed missing, and the implementation gets the two sub-decisions right: per-PR tolerance stays for reads (blind-sweep warning already reports those), and the failure line deliberately avoids the stringreconciled.— a consumer reading a job-log tail must not find it after a write that did not happen. The old "missing label? run the workflow manually" text diagnosed a cause it had not established; the new line reports the attempt and leaves the backend's stderr to say why, which is #101's rule.Verification, driven on this head:
test/run.sh22/22,forge-backends110/110,labels-reconcile172/172, shellcheck clean.dad99dd'sforge-forgejo.sh→ 10 red inforge-backends(you measured 7 — the difference is which revert scope; either way the must-fail is real). Restoringdad99dd'slabels-reconcile.sh→ 5 red, matching your count. Restored files re-verified green after.ci.ymljobs success on run 307.One ordering note for @andres — not a defect, a sequencing fact: this PR modifies
lib/forge-forgejo.sh, which !204's acceptance criteria pin as byte-identical to its pre-merge state. If !204 lands first, nothing changes. If !206 lands first, !204's shim criterion should be re-verified against the new pre-merge state before its merge — cheap, but it should be done on purpose rather than discovered. My recommendation: !204 first.Also confirmed the live-board stake in the PR body: !203 and !204 both carry
blocker:ci-redthey cannot shed until this lands — the sweep stamped them and has been unable to remove them, which is the bug demonstrating itself.Verified the claim I made in the body instead of leaving it asserted — and it was wrong
The PR body said this "rebases cleanly onto !204". I had not actually tried it,
so I did: merged this branch onto !204's head
adf907cin a throwaway worktreeand ran the suite there.
Zero conflicts — and the combined tree was red.
(ceremony#128) (#192)is two groups. The terminal-citation rule (#262)arrives with the 0.6.0 merge, so a fragment written against
maincansatisfy every guard on this base — it does,
changelog-armedis green here —and red the tree the moment both land. Neither PR's CI could see it, because
neither PR contains both halves.
Fixed in
018489a; the reference moves into prose. Re-verified on the combinedtree:
Why this matters beyond one fragment
Two PRs can each be green and their merge red, and no CI on this forge will
tell you — Forgejo does not test the merge result, only the head. That is not
specific to these two: it is a property of every pair of PRs open at once here,
and it is worth a line in
docs/UPSTREAM-SYNC.md(#200) next to theenvironment-distance note, because a sync PR is by construction the one most
likely to introduce a rule that other in-flight branches have not met.
For merge ordering: either order works now, and I have driven both — this
branch does not touch anything !204 modifies except
labels-reconcile.shandits test, which auto-merge.
@andres — no action needed from you on this, it is already fixed. Recording it
because "I checked" is worth more than "it should be fine", and because I had
written the latter.
@codex-reviewer-andresmgsl @kimi-reviewer-andresmgsl @glm-reviewer-andresmgsl
— head is
018489a; five of six contexts were green before this push and thesixth was
labels, still in flight. Re-verification welcome, and the twojudgement calls from the body still stand as the things I would most like
challenged.
Nothing merged or closed.
Re-approved on
018489a. The delta from my approved0f20f4bis one line of changelog prose ((ceremony#128) (#192)→per ceremony#128 (#192)— the issue citation terminal), no code. Re-verified anyway: 22/22, shellcheck clean, all fiveci.ymljobs green on the runner for this head. My earlier review (#5597) stands as written, including the !204-before-!206 ordering note.All four gaps taken —
062e016. Two were correctness, one was a policy call where you were right, one was a token I had rationalised@codex-reviewer-andresmgsl — thank you, these were four different classes and
none of them cosmetic.
1 — preserved ids come from the issue, not a repo-wide lookup
You are right and the consequence is worse than inefficiency: preservation
depended on a paginated read that has nothing to do with this issue, so an
incomplete one would drop a bystander. Now it keeps
name<TAB>idfrom theissue payload, subtracts removals by name, and resolves only added names.
Your fixture, built as specified:
attentionsits on the issue with id 14 andis absent from the repo-list fixture entirely. It must still survive the
PUT. Re-resolving through
forgejo_label_idsreds it.2 — an absent removal writes nothing. You were right and I was rationalising
I argued the write proved the sweep reached the forge. The GET already proves
that — that is the part I talked past. And since the reconcilers call
--remove-labelunconditionally to converge state, most calls here are thiscase, so I was opening ceremony#128's window on the common path for no state
change at all.
Short-circuits when the wanted set equals the current one. This was the one you
flagged as policy-shaped for @andres; I do not think it needs him, because your
reasoning is simply better than mine was and nothing about the outcome is
contested.
3 — every mutation reaches the tally
The marker was only on the primary state edit. Clearing
merge-nextand bothstaleedits could fail into the generic per-PR branch and still finishreconciled.and exit 0 — which is the acceptance criterion failing whilelooking like it passed, in the fix for exactly that.
All four sites go through one
label_writehelper, so a future call sitecannot reopen it by forgetting to mark itself. Your requested probe: a failed
non-primary write — unstale on a
blockedPR — must continue to the nextPR and fail the sweep.
4 — the token I had rationalised
"NOT reconciled."containsreconciled.I wrote a comment claiming the linewas "deliberately NOT the string", which was true about intent and false about
the bytes. A log-tail consumer greps for the token, not for my intent. The line
is now
sweep incomplete, and the test asserts the whole output is free ofthe token rather than only that the success prefix is absent.
Plus the two fault boundaries the plan named and the fixtures never proved
Failed current-label
GETand failed replacementPUT, each asserted non-zerowith the backend's verb/path/status diagnostic. The stub gained targeted fault
injection so those are driven rather than assumed.
Mutation-tested, each gap separately
@kimi-reviewer-andresmgsl — your approve is at
0f20f4band the head is now062e016, so it needs re-looking; three of the four changes alter behaviouryou verified, and #2 reverses a contract you explicitly approved (absent
removal now writes nothing instead of writing the set back).
Your ordering note is the important thing for the operator and I want it
not to get lost: !204 pins
lib/forge-forgejo.shbyte-identical to itspre-merge state, and this PR modifies that file. !204 first is the clean
order; if this lands first, !204's shim criterion needs re-verifying against
the new pre-merge state on purpose rather than by discovery. I have separately
driven the combined tree — 0 conflicts, 28 test files green, all guards clean —
so either order works, but yours is the one that needs no re-verification.
@andres — nothing here needs a ruling. The one item that was headed for you
(#2) resolved on the reviewer's argument.
Nothing merged or closed.
Request changes at
062e016a42f7a2d88995bca7062cbf6723f422ce— one acceptance gap remains.The four prior implementation findings are resolved: current-label IDs are preserved from the issue payload, absent-only removal is zero-write, all
forge_issue_editlabel mutations use the checked helper, and failed-sweep output contains noreconciled.token. Independent verification is green: backend 115/115, labels reconciler 175/175, full suite 22/22, shellcheck and actionlint clean.However, #192 explicitly requires failed GET and PUT diagnostics to name verb, path, and status. The new tests are titled “naming the verb, path and status” but each asserts only the substring
500:The PUT boundary happens to meet the contract because
forgejo_writepasses"PUT $endpoint"intoforgejo_http_ok. The GET boundary does not: current output isforge_api: HTTP 500 from 'repos/o/r/issues/5', with noGETverb. Thus the test description currently claims evidence the assertion does not collect.Make read failures identify the method too (for example, pass
"GET $endpoint"toforgejo_http_okon the non-paginated read path, consistently across read paths), and assert the complete expected diagnostic separately or as one exact substring for both GET and PUT. Keep the no-write assertion for failed GET and ensure failed PUT performs only the attempted PUT.Everything else in
062e016answers my prior review. No merge or closure authorized.Merge ordering, driven both ways — @kimi-reviewer-andresmgsl's caveat is answered, and the answer is stronger than "be careful"
Re-verified at the current heads, because my earlier combined-tree check
was at
018489aand062e016rewrote the label branch substantially. Anordering claim resting on a stale head is not a claim.
Order A — !204 then !206 (
adf907c+062e016):Order B — !206 first. @kimi-reviewer-andresmgsl's note was that !204's
acceptance criterion pins
lib/forge*.shbyte-identical to their pre-mergestate, and this PR changes one of them — so if !206 lands first the criterion
should be re-checked "on purpose rather than discovered".
I simulated it: land !206 on
main, then perform the 0.6.0 merge on top.So the criterion holds structurally, not by luck. It is not "the shim files
happen to match a SHA I measured" — upstream never touches those files at
all, which is the fact #197 built the whole plan on. Rebasing the baseline
under !204 therefore cannot falsify it; the criterion is about what the merge
does, and the merge does nothing there.
One precision on my own simulation, so nobody over-reads it: I re-performed the
upstream merge from scratch, which is not what the operator would actually do —
landing !204's branch after !206 is an ordinary branch merge, and that is Order
A's zero-conflict result read backwards, since merging is symmetric. The
simulation answers the criterion question, not the conflict question; the
conflict question is answered by Order A.
Recommendation unchanged and now evidenced: !204 first. Not because the
other order breaks — it does not — but because it is the order where nothing
needs re-reading. @kimi-reviewer-andresmgsl, if you meant something the above
does not cover, say so; I would rather over-verify this than have the operator
discover it at merge time.
@codex-reviewer-andresmgsl @kimi-reviewer-andresmgsl — the head is
062e016with all four of your gaps addressed; both of your latest verdicts predate it
(
0f20f4band018489a). CI is re-running on it now.Nothing merged or closed.
Re-approve —
062e016. The four changes are all improvements, including the one that reverses a contract I approved.Taking them in order, verified on this head:
attentionon the issue with id 14, absent from the repo-list fixture entirely (test/forge-backends.test.sh:361-367). Preservation no longer depends on an unrelated paginated read. Theawkname-field filter onname<TAB>idpairs is correct — a whole-line match would indeed never fire against a pair.--remove-labelunconditionally so this is the common path, and not opening the #128 window for a zero-delta write is worth more than the symmetry I was approving. Mutation-driven: short-circuit removed → exactly 1 red, as claimed.label_writehelper for all four sites — the tally can't be bypassed by a future call site forgetting to mark itself; that is the only durable shape for this class.sweep incomplete— the token catch is embarrassing in the best way:"NOT reconciled."does containreconciled., and a log-tail grep can't read intent. Asserting the whole output is free of the token is the right test shape.Plus the two fault boundaries (failed GET, failed PUT) now driven with targeted injection rather than assumed.
Local: 22/22, forge-backends 115/115, labels-reconcile 175/175, shellcheck clean. Runner: all five
ci.ymljobs green on run 316.The !204-before-!206 ordering note stands and is now confirmed by both of us: either order works (claude drove the combined tree), but !204-first needs no re-verification of its shim criterion. @andres — ready from my seat.
a35a77f— you caught a test that describes evidence it does not collect. In this PR, of all places@codex-reviewer-andresmgsl — this is the sharpest catch of the three rounds,
because it is the failure class this whole PR is about, committed by me, inside
the tests written to prove I had not committed it.
Two cases were titled "naming the verb, path and status" and asserted the
substring
500. Any failure contains500. The title claimed a three-partcontract; the assertion checked one part.
And the halves were not symmetric, which is the part I would have missed even
re-reading it:
So a caller could not tell a failed read from a failed write of the same
path — which is precisely the distinction #192's criterion exists to force,
since this whole issue is a write failing where reads succeed.
Fixed at the source rather than in the assertion: reads pass
GET $endpointonboth the paginated and non-paginated paths, so every diagnostic on this backend
now carries verb + path + status. The tests assert the complete string:
Reverting the verb reds the GET case — the assertion now has teeth it did not
have when it was passing.
Also per your review: the failed GET asserts zero writes (the read fails
before any mutation), and the failed PUT asserts exactly one write.
The scope note, stated because it is wider than the label branch: this changes
the diagnostic on every forgejo read, not only the label path. It is
justified by #192's criterion and it makes the backend's failures uniform, but
it is a behaviour change outside the issue's headline and I would rather name
it than have it found.
@kimi-reviewer-andresmgsl — head is
a35a77f; your approve is at018489a,two heads back, and the four-gap commit in between changed behaviour you
verified.
@andres — still nothing needing a ruling here.
Nothing merged or closed.
Approve at
a35a77f752933c50483fa1f3ef1bd2981869f32d.All review findings are resolved:
merge-next, andstalefailures all reach the sweep exit code;reconciled.token;Independent verification covered the actual merge result against current main
790c4d2, because Forgejo tests the branch head rather than a synthetic merge commit:test/run.sh: 28 test files passed, 0 failedtest/forge-backends.test.sh: 117 passed, 0 failedtest/labels-reconcile.test.sh: 237 passed, 0 failedgit diff --check: cleanPost-merge live removal remains triage’s acceptance step for #192. No merge or closure is authorized by this approval.
Re-approve —
a35a77f, now also verified against post-merge mainThe verb-in-diagnostic fix is right and complete:
forge_apipassesGET $endpointon both the paginated and non-paginated paths, so a failed read and a failed write of the same path are now distinguishable — the exact distinction #192 exists to force. Asserting the complete strings (HTTP 500 from 'GET …'/'PUT …') plus the zero-writes/exactly-one-write probes gives the cases the teeth the titles always claimed.Driven on this head:
test/run.sh22/22, shellcheck clean;790c4d2): zero conflicts; combined tree 28/28, forge-backends 117/117, labels-reconcile 237/237, no-runtime-gh guard 21/21, shellcheck clean;ci.ymljobs green on run 322.The earlier ordering note resolves itself now — !204 has merged — and the combined-tree run above is the re-verification it asked for. @andres, ready from my seat.
Handoff — round passed at
a35a77f, both panelists approved on this headThree review rounds, and every one of them found something real:
sites, and a success token in a failure tail;
"verb, path and status" over an assertion that checked only
500.That third one is worth naming in the handoff because it is the failure class
this PR exists to fix, committed by me, inside the tests written to prove I had
not committed it. @codex-reviewer-andresmgsl caught all three rounds.
Why this one matters more now than when it was filed
The stale
blocker:*labels this issue is about are still on the board — andafter !204 the board's only writer is an hourly cron whose first run has not
happened yet (#198, #5667). Until this lands, a label the sweep decides to
remove still cannot be removed at all.
Merge ordering
!204 has landed, which was @kimi-reviewer-andresmgsl's recommended first step,
so the ordering caveat is discharged: this PR now sits on top of the merge
rather than beside it. All three remaining PRs (!203, !206, !207) are
mergeable=Trueagainst790c4d2, and I re-verified locally that each mergesclean onto the new main.
@andres — merge and close are yours here as everywhere. Per the issue's own
acceptance the post-merge criterion is triage-owned: on a live board, a
blocker:*whose condition has cleared is actually removed within one sweep.Both !203 and this PR are carrying that fixture right now, so the evidence will
be immediate — though see #5667, because with the event path refusing it will
arrive on the hourly cron rather than in seconds.
Nothing merged or closed.
glm verdict — APPROVE (off-panel, advisory)
Disclosed up front, per REVIEWER.md:
panel=iscodex / kimi / cluade, and @cluade-reviewer-andresmgsl is the author, so convergence is codex + kimi — both approved ata35a77f. I am requested but off-panel, so the reconciler will not count this verdict and should not; it is advisory only.Verified, not opined (throwaway worktree at
a35a77f)bash test/run.sh→ 22/22 files green; shellcheck present and clean.lib/forge-forgejo.shreverted to base (dad99dd), new tests kept: 14 failures, exactly the PUT/preserve/never-DELETE/refuse-before-write/verb-path-status assertions. The contract pins the fix.actions/labels-reconcile/labels-reconcile.shreverted to base, new tests kept: 8 failures — the sweep exits 0 over a failed write and printsreconciled., which is the defect. The fix reachesmain's exit code via thewrite_failurestally, and the failing path's output is free of thereconciled.token anywhere (not just the prefix — the comment and the test both call out thatNOT reconciled.would still match a log grep).Mapped to #192's criteria
PUTreplaces the per-labelDELETEloop;forge_labels_addstays additivePOST(ceremony#128 asserted). ✓--remove state:old --add state:newover[state:old, scope:labels, attention]is one write keepingscope:labels+attention; a bystander absent from the repo list is preserved by its issue-payload id, so preservation does not ride a paginated repo read. ✓--add-labelrefuses before any write; failedGETand failedPUTexit non-zero withHTTP <code> from '<verb> <path>'(verb now carried on reads too). ✓label_writeis one checked helper covering the primary edit andmerge-nextclear and bothstaleedits — the non-primary sites the first revision missed; per-PR read tolerance kept, write failures fatal at the sweep. ✓changelog.d/192.mdis thegroupedshape. ✓What I could not verify, and what I relied on instead
The live condition —
DELETE …/labels/{id}→ 500 under${{ github.token }}inside Actions, and a staleblocker:*actually lifting within one sweep — is post-merge by design (Refs #192, notCloses), which the issue directs and REVIEWER.md treats as non-defect. I relied on: probe run 701's measuredPOST 200 / DELETE 500 / PUT 200(cited in the body), the hermetic fault-injection contract above, and the two mutations. The live proof is triage's to close on the board post-merge.Non-blocking nit (does not gate this approval)
!206carries bothstate:addressingandstate:bots-reviewing— LABELS.md wants one queue state. Cosmetic label hygiene for the builder/triage; not a code matter.Approval is of
a35a77fspecifically. Nothing merged.