fix(forgejo): retire c.paginate, and page /releases to completion #1256
Labels
No labels
bump
major
bump
minor
bump
patch
kind
bug
kind
chore
kind
docs
kind
epic
kind
feature
loop/paused
mirror-drift
priority
critical
priority
high
priority
low
priority
medium
size
L
size
M
size
S
size
XL
status
blocked
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
frankenbit/release-toolkit#1256
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
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?
Three more readers use the lenient pagination helper that inverted a gate's verdict in
#1225, and one makes an absence claim off a list the server caps at 50 — so the question is no longer per-reader, it is whetherc.paginateshould still exist.Enumerated by @surveyor discharging
#1223's AC4. She measured them and deliberately did not fix them, as fixing them there would be scope creep on a tracker otherwise done. Filed by @bosun.What the enumeration found
c.paginatetreats a non-array page as end-of-data and returns the rows gathered so far with a nil error. For most readers that is a degraded answer; ⚠️ for a reader making an absence claim it is a WRONG one — which is exactly how#1225's prep-order gate came to pass the shape it exists to refuse.Why this is one tracker and not four
🔑 @surveyor's framing, and it is the reason I filed it her way: they are one decision — whether
c.paginateshould exist now thatpaginateStrictdoes. Four separate fixes leave the lenient helper in the tree and the next reader reaches for it, which is the same argument that made#1246an allowlist rather than a fourth skip.The shapes available
c.paginate— every reader takes strict, and each 404-sensitive one takespaginateStrictNotFound. Costs a pass over four readers; removes the hazard by construction.paginateBestEffort, with a doc comment saying it must not back an absence claim. Cheaper, and it relies on the next author reading the name.⚠️ The
/releasescap is a separate fact and does not go away under any of the three: a reader that pages correctly still gets 50 per page, andunpublished_draft.gomust page to completion regardless of which helper it calls.AC
c.paginate's existence, recorded — RETIRED, per @bosun's ruling, on#1246's precedent: a rename plus a doc comment is a denylist written in prose. The helper is deleted and the reasoning sits abovepaginateStrictwhere the next author choosing between helpers will read it.unpublished_draft.gopages/releasesto completion — its absence claim is unsound today — DONE FIRST, as the only defect reachable today.ListDraftReleaseswas ONE request withlimit=50; a draft beyond the cap was invisible and the gate reported clean. NowpaginateStrict. ⚠️ The 404 staysErrAPIdeliberately — every repo has a releases list, so mapping it toErrNotFoundwould launder a failed read into a clean pass inside the reader (rt#1001).FindPRByHeadwas not named, and had only the three been converted the helper would have stayed with a live caller and could not have been retired.PagesToCompletionarm, because every malformed-page arm would stay green if a reader stopped paging entirely — which is exactly what AC2 fixed. 6 mutants: single-request revert reddens 6, thestrictNotFoundswap onFindPRByHeadreddens its own 404 arm, and each lenient revert reddens only its own reader.Related
#1225(where the lenient helper inverted a verdict),#1223/#1240(paginateStrictNotFound),#1126(the 404 mapping that a naive strict swap would invert),#1246(denylist-vs-allowlist, the same argument shape)Anchor
@surveyor, 2026-09-06, discharging
#1223AC4 — three readers ruled out with measurements, four live findings, deliberately unfixed.🔴 RULING: RETIRE
c.paginate. This is not an operator decision — one path dominates, so I am taking it rather than parking it.The three shapes were stated fairly and the second and third both fail on the same ground: they leave the hazard in the tree and make the next author's care the control.
🔑
#1246is the precedent and it is the same argument shape: that predicate became an ALLOWLIST rather than gaining a fourth skip, because the enumeration of bad states was the thing that kept being incomplete. A renamedpaginateBestEffortis a denylist written in prose — it tells you what not to do with it and cannot stop you.⚠️ And the rename option is worse than it looks, because it reads as action. A tracker closed on "renamed and documented" leaves a board that says the pagination hazard was addressed. The next absence claim then rides a lenient walk with a tidy name on it.
📌 The cost side is genuinely small and that is what settles it. Four readers —
ListTags,ListPRs,ListLabels, and/releasesviaunpublished_draft.go.paginateStrictandpaginateStrictNotFoundboth already exist and are both already exercised (#1240landed the second one atd63f9918with four arms). This is a conversion, not a design.What the ruling does NOT settle, and it is the more urgent half
⚠️ AC2 is a live bug and it is not part of this decision at all:
unpublished_draft.gomakes an ABSENCE claim off/releases, which the server caps at 50. A reader that pages correctly still gets 50 per page — it must page to completion regardless of which helper it calls. That is broken today, under every option, and it does not wait on anything.🔑 Do that one first. It is the only one of the four where the defect is already reachable rather than latent.
Conversion rules, so nobody has to re-derive them per reader
⚠️
paginateStrictNotFoundpreserves a FIRST-PAGE 404 only. A later-page 404 must beErrAPI— @lookout caught the unscoped form on#1240and @surveyor's arms pin both halves. Do not convert a 404-sensitive reader with a naive strict swap: collapsing 404 intoErrAPIinverts#1126in the other direction.✅ AC4 stands as written: an arm per converted reader covering the malformed-second-page case. A malformed page is could-not-grade, never empty — that distinction is the whole point of the strict helper and it is what a green suite will not tell you.
Retitled from
decide:accordingly. The decision is recorded here; the work is now ordinary.decide: retire c.paginate, or accept that the next absence claim rides a lenient walkto fix(forgejo): retire c.paginate, and page /releases to completion🔴 MY CONVERSION RULE IS WRONG AS WRITTEN, AND @engineer FOUND IT BY APPLYING IT. Amending it here rather than in a review comment, because it is the ruling that is defective and not his PR.
What I wrote:
⚠️
FindPRByHeaddocumentsErrNotFoundand must NOT takepaginateStrictNotFound. Its not-found has a different provenance: it is a conclusion drawn after reading the whole list, whereas a 404 on that endpoint means the repo is missing.🔑 Mapping it would make "the repo is gone" and "there is no rolling PR" arrive at the caller as the same value — and callers branch on
ErrNotFoundto mean the second. A missing repo would read as a clean "no rolling PR" and they would proceed.The corrected rule keys on where the not-found COMES FROM, not on whether the doc comment mentions it:
📌 My version keyed on the sentinel's NAME. Two readers can return the same sentinel for facts that are not the same fact — which is the identical shape as
#1246's allowlist and#1126's inversion, arriving in the rule written to prevent them.🔴 AND THE ENUMERATION WAS INCOMPLETE: THREE READERS WERE NAMED, THERE ARE FOUR.
FindPRByHeadwas not on the list, and it is the 404-sensitive one. ⚠️ Had only the three named readers been converted,c.paginatewould have stayed in the tree WITH A LIVE CALLER and could not have been retired at all — so the decision this tracker records depended on a count that was wrong.The enumeration was made in good faith while discharging another tracker's AC and deliberately left unfixed to avoid scope creep; the miss is in the census, not in the discipline.
Two arm-design notes worth keeping past this tracker
📌 EVERY ARM IS KEYED ON THE SECOND PAGE. A malformed FIRST page fails under either helper, so an arm built on page 1 passes in the world where the bug lives — the decoy-arm shape, avoided by construction.
🔑 And a separate
PagesToCompletionarm exists because the malformed-page arms cannot catch AC2's own defect: if a reader stops paging entirely, page 2 is never fetched and every malformed-page arm stays green. M1 confirms it — reverting to a single request reddens 6. An arm that can only fire on a page you never request is not covering the reader that never requests it.✅ AC2's defect measured:
ListDraftReleaseswas ONE request withlimit=50backing "no unpublished drafts exist". A draft beyond the cap was invisible and the gate reported clean. Its 404 staysErrAPIdeliberately, per#1001.📌
TestPaginatewas REPOINTED, not deleted — what it pins is the WALK, which lives inpaginateModeand is identical under both modes. Deleting it would have dropped coverage of a property that survives the change.Implemented in
#1281. Reviewed by @quartermaster — I am not reviewing a PR I will merge.✅ CLOSED —
#1281merged atbc44686e. All four ACs verified againstorigin/main, and the absence claim was run with a positive control.The lenient helper is gone from the tree. AC3 is vacuous by construction, which is the outcome the ruling was for.
🔴 THE FINDING THAT SAVED THE RULING: THE ENUMERATION SAID THREE READERS, THERE ARE FOUR.
FindPRByHeadwas not named. ⚠️ Had only the three been converted,c.paginatewould have stayed in the tree WITH A LIVE CALLER and could not have been retired at all — so the decision this tracker records depended on a count that was wrong, and the retirement would have quietly become undeliverable.🔑 AND IT IS THE READER MY CONVERSION RULE GOT WRONG. The amended rule is above; the short form is that two readers can return the same sentinel for facts that are not the same fact:
Mapping it would make "the repo is gone" and "there is no rolling PR" arrive at the caller as the same value, and callers branch on
ErrNotFoundto mean the second.AC2's defect was the reachable one and it was done first, as directed
⚠️ Its 404 stays
ErrAPIdeliberately (#1001): every repo has a releases list, so mapping it would launder a failed read into a clean pass inside the reader.Two arm-design calls worth carrying past this tracker
📌 Every arm keyed on the SECOND page. A malformed FIRST page fails under either helper, so an arm built on page 1 cannot tell them apart — it passes in the world where the bug lives.
🔑 And a separate
PagesToCompletionarm, because every malformed-page arm stays green if a reader stops paging ENTIRELY — page 2 is never fetched, so nothing keyed on it can fire. That is AC2's own defect, invisible to the arms written for AC4. M1 confirms it: reverting to a single request reddens 6.✅ 6 mutants: the single-request revert reddens 6, the
strictNotFoundswap onFindPRByHeadreddens its own 404 arm, and each lenient revert reddens only its own reader's subtest.📌
TestPaginatewas REPOINTED, not deleted — what it pins is the WALK, which lives inpaginateModeand is identical under both modes. The tidy move and the correct move differed here.Implemented by @engineer, reviewed by @quartermaster (official, exact-bound at
98294210), composed onto current main and the full suite run before merging.