fix(forgejo): retire c.paginate, and page /releases to completion #1256

Closed
opened 2026-09-06 10:11:25 +02:00 by bosun · 3 comments
Member

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 whether c.paginate should 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

/releases            caps at 50   AND  unpublished_draft.go makes an ABSENCE claim off it
ListTags             c.paginate   lenient — #1225's exact shape
ListPRs              c.paginate   lenient
ListLabels           c.paginate   lenient

c.paginate treats 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.paginate should exist now that paginateStrict does. Four separate fixes leave the lenient helper in the tree and the next reader reaches for it, which is the same argument that made #1246 an allowlist rather than a fourth skip.

The shapes available

  • RETIRE c.paginate — every reader takes strict, and each 404-sensitive one takes paginateStrictNotFound. Costs a pass over four readers; removes the hazard by construction.
  • KEEP IT, RENAMED — e.g. paginateBestEffort, with a doc comment saying it must not back an absence claim. Cheaper, and it relies on the next author reading the name.
  • KEEP IT AS IS — and accept that the next absence-claiming reader will use it.

⚠️ The /releases cap is a separate fact and does not go away under any of the three: a reader that pages correctly still gets 50 per page, and unpublished_draft.go must page to completion regardless of which helper it calls.

AC

  • A decision on 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 above paginateStrict where the next author choosing between helpers will read it.
  • unpublished_draft.go pages /releases to completion — its absence claim is unsound today — DONE FIRST, as the only defect reachable today. ListDraftReleases was ONE request with limit=50; a draft beyond the cap was invisible and the gate reported clean. Now paginateStrict. ⚠️ The 404 stays ErrAPI deliberately — every repo has a releases list, so mapping it to ErrNotFound would launder a failed read into a clean pass inside the reader (rt#1001).
  • Whichever readers keep a lenient path, none of them backs an absence claim — VACUOUS BY CONSTRUCTION: no reader keeps a lenient path, because the helper no longer exists. 🔴 The enumeration said three readers; there are FOUR — FindPRByHead was not named, and had only the three been converted the helper would have stayed with a live caller and could not have been retired.
  • An arm per converted reader covering the malformed-second-page case — DONE, five readers, all keyed on the SECOND page: a malformed FIRST page fails under either helper, so an arm built on one cannot tell them apart. ⚠️ Plus a separate PagesToCompletion arm, 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, the strictNotFound swap on FindPRByHead reddens its own 404 arm, and each lenient revert reddens only its own reader.

#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 #1223 AC4 — three readers ruled out with measurements, four live findings, deliberately unfixed.

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 whether `c.paginate` should 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 ``` /releases caps at 50 AND unpublished_draft.go makes an ABSENCE claim off it ListTags c.paginate lenient — #1225's exact shape ListPRs c.paginate lenient ListLabels c.paginate lenient ``` **`c.paginate` treats 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.paginate` should exist now that `paginateStrict` does.** Four separate fixes leave the lenient helper in the tree and the next reader reaches for it, which is the same argument that made `#1246` an allowlist rather than a fourth skip. ## The shapes available - **RETIRE `c.paginate`** — every reader takes strict, and each 404-sensitive one takes `paginateStrictNotFound`. Costs a pass over four readers; **removes the hazard by construction.** - **KEEP IT, RENAMED** — e.g. `paginateBestEffort`, with a doc comment saying it must not back an absence claim. **Cheaper, and it relies on the next author reading the name.** - **KEEP IT AS IS** — and accept that the next absence-claiming reader will use it. ⚠️ **The `/releases` cap is a separate fact and does not go away under any of the three:** a reader that pages correctly still gets 50 per page, and `unpublished_draft.go` must page to completion regardless of which helper it calls. ## AC - [x] A decision on `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 above `paginateStrict` where the next author choosing between helpers will read it. - [x] `unpublished_draft.go` pages `/releases` to completion — its absence claim is unsound today — **DONE FIRST**, as the only defect reachable today. `ListDraftReleases` was ONE request with `limit=50`; a draft beyond the cap was invisible and the gate reported clean. Now `paginateStrict`. ⚠️ The 404 stays `ErrAPI` deliberately — every repo has a releases list, so mapping it to `ErrNotFound` would launder a failed read into a clean pass inside the reader (rt#1001). - [x] Whichever readers keep a lenient path, none of them backs an absence claim — **VACUOUS BY CONSTRUCTION**: no reader keeps a lenient path, because the helper no longer exists. 🔴 **The enumeration said three readers; there are FOUR** — `FindPRByHead` was not named, and had only the three been converted the helper would have stayed with a live caller and could not have been retired. - [x] An arm per converted reader covering the malformed-second-page case — **DONE**, five readers, all keyed on the SECOND page: a malformed FIRST page fails under either helper, so an arm built on one cannot tell them apart. ⚠️ Plus a separate `PagesToCompletion` arm, 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, the `strictNotFound` swap on `FindPRByHead` reddens 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 `#1223` AC4 — three readers ruled out with measurements, four live findings, deliberately unfixed.
Author
Member

🔴 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.

RETIRE            removes the hazard BY CONSTRUCTION      cost: one pass over four readers
KEEP, RENAMED     relies on the next author reading it    cost: ~0    hazard: intact
KEEP AS IS        accepts the next wrong absence claim    cost: 0     hazard: intact

🔑 #1246 is 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 renamed paginateBestEffort is 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 /releases via unpublished_draft.go. paginateStrict and paginateStrictNotFound both already exist and are both already exercised (#1240 landed the second one at d63f9918 with 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.go makes 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

plain list, no 404 semantics        -> paginateStrict
reader that documents ErrNotFound   -> paginateStrictNotFound

⚠️ paginateStrictNotFound preserves a FIRST-PAGE 404 only. A later-page 404 must be ErrAPI — @lookout caught the unscoped form on #1240 and @surveyor's arms pin both halves. Do not convert a 404-sensitive reader with a naive strict swap: collapsing 404 into ErrAPI inverts #1126 in 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.

🔴 **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.** ``` RETIRE removes the hazard BY CONSTRUCTION cost: one pass over four readers KEEP, RENAMED relies on the next author reading it cost: ~0 hazard: intact KEEP AS IS accepts the next wrong absence claim cost: 0 hazard: intact ``` 🔑 **`#1246` is 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 renamed `paginateBestEffort` is 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 `/releases` via `unpublished_draft.go`. **`paginateStrict` and `paginateStrictNotFound` both already exist and are both already exercised** (`#1240` landed the second one at `d63f9918` with 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.go` makes 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 ``` plain list, no 404 semantics -> paginateStrict reader that documents ErrNotFound -> paginateStrictNotFound ``` ⚠️ **`paginateStrictNotFound` preserves a FIRST-PAGE 404 only.** A later-page 404 must be `ErrAPI` — @lookout caught the unscoped form on `#1240` and @surveyor's arms pin both halves. **Do not convert a 404-sensitive reader with a naive strict swap: collapsing 404 into `ErrAPI` inverts `#1126` in 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.**
bosun changed title from decide: retire c.paginate, or accept that the next absence claim rides a lenient walk to fix(forgejo): retire c.paginate, and page /releases to completion 2026-09-06 11:20:15 +02:00
Author
Member

🔴 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:

plain list, no 404 semantics        -> paginateStrict
reader that documents ErrNotFound   -> paginateStrictNotFound

⚠️ FindPRByHead documents ErrNotFound and must NOT take paginateStrictNotFound. 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 ErrNotFound to 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:

the endpoint's own 404 IS the answer                  -> paginateStrictNotFound
                                                         (page 1 only; later pages are ErrAPI)
the not-found is a CONCLUSION from the full list      -> paginateStrict
                                                         a 404 here is a different fact and must stay ErrAPI

📌 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.

FindPRByHead was not on the list, and it is the 404-sensitive one. ⚠️ Had only the three named readers been converted, c.paginate would 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 PagesToCompletion arm 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: ListDraftReleases was ONE request with limit=50 backing "no unpublished drafts exist". A draft beyond the cap was invisible and the gate reported clean. Its 404 stays ErrAPI deliberately, per #1001.

📌 TestPaginate was REPOINTED, not deleted — what it pins is the WALK, which lives in paginateMode and 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.

🔴 **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:** ``` plain list, no 404 semantics -> paginateStrict reader that documents ErrNotFound -> paginateStrictNotFound ``` ⚠️ **`FindPRByHead` documents `ErrNotFound` and must NOT take `paginateStrictNotFound`.** 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 `ErrNotFound` to 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:** ``` the endpoint's own 404 IS the answer -> paginateStrictNotFound (page 1 only; later pages are ErrAPI) the not-found is a CONCLUSION from the full list -> paginateStrict a 404 here is a different fact and must stay ErrAPI ``` 📌 **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.** **`FindPRByHead` was not on the list, and it is the 404-sensitive one.** ⚠️ **Had only the three named readers been converted, `c.paginate` would 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 `PagesToCompletion` arm 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: `ListDraftReleases` was ONE request with `limit=50` backing "no unpublished drafts exist". A draft beyond the cap was invisible and the gate reported clean.** Its 404 stays `ErrAPI` deliberately, per `#1001`. 📌 **`TestPaginate` was REPOINTED, not deleted — what it pins is the WALK, which lives in `paginateMode` and 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.**
bosun closed this issue 2026-09-06 11:47:39 +02:00
Author
Member

✅ CLOSED — #1281 merged at bc44686e. All four ACs verified against origin/main, and the absence claim was run with a positive control.

internal/forgejo/client.go:330   paginateStrict
internal/forgejo/client.go:345   paginateStrictNotFound
internal/forgejo/client.go:349   paginateMode          the shared walk
internal/forgejo/reads.go:765    paginateActionScope

grep for a bare `paginate` call, excluding the four above:  ZERO
positive control — "paginate" appears 14x in client.go, so the needle works

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.

FindPRByHead was not named. ⚠️ Had only the three been converted, c.paginate would 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:

FindPRByHead   documents ErrNotFound, takes paginateStrict
               its not-found is a CONCLUSION from the full list
               a 404 there means THE REPO IS MISSING -> must stay ErrAPI

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 ErrNotFound to mean the second.


AC2's defect was the reachable one and it was done first, as directed

ListDraftReleases   ONE request, limit=50, backing "no unpublished drafts exist"
                    a draft beyond the cap was INVISIBLE and the gate reported CLEAN

⚠️ Its 404 stays ErrAPI deliberately (#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 PagesToCompletion arm, 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 strictNotFound swap on FindPRByHead reddens its own 404 arm, and each lenient revert reddens only its own reader's subtest.

📌 TestPaginate was REPOINTED, not deleted — what it pins is the WALK, which lives in paginateMode and 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.

✅ **CLOSED — `#1281` merged at `bc44686e`. All four ACs verified against `origin/main`, and the absence claim was run with a positive control.** ``` internal/forgejo/client.go:330 paginateStrict internal/forgejo/client.go:345 paginateStrictNotFound internal/forgejo/client.go:349 paginateMode the shared walk internal/forgejo/reads.go:765 paginateActionScope grep for a bare `paginate` call, excluding the four above: ZERO positive control — "paginate" appears 14x in client.go, so the needle works ``` **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.** **`FindPRByHead` was not named.** ⚠️ **Had only the three been converted, `c.paginate` would 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**: ``` FindPRByHead documents ErrNotFound, takes paginateStrict its not-found is a CONCLUSION from the full list a 404 there means THE REPO IS MISSING -> must stay ErrAPI ``` **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 `ErrNotFound` to mean the second.** --- ## AC2's defect was the reachable one and it was done first, as directed ``` ListDraftReleases ONE request, limit=50, backing "no unpublished drafts exist" a draft beyond the cap was INVISIBLE and the gate reported CLEAN ``` ⚠️ **Its 404 stays `ErrAPI` deliberately (`#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 `PagesToCompletion` arm, 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 `strictNotFound` swap on `FindPRByHead` reddens its own 404 arm, and each lenient revert reddens only its own reader's subtest.** 📌 **`TestPaginate` was REPOINTED, not deleted — what it pins is the WALK, which lives in `paginateMode` and 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.**
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
frankenbit/release-toolkit#1256
No description provided.