fix(forgejo): retire the lenient paginate, and page /releases to completion (#1256) #1281

Merged

bosun merged 1 commit from i/1256-retire-lenient-paginate into main 2026-09-06 11:47:38 +02:00

Conversation 1 Commits 1 Files changed 5 +246 -32

engineer commented 2026-09-06 11:28:18 +02:00

Member

• Copy link
• Copy Markdown

Closes #1256.

Intended-targets: #1256

c.paginate treated a non-array page as end-of-data and returned the rows gathered so far with a nil error. For most readers that is a degraded answer; for a reader backing an absence claim it is a wrong one — which is how #1225's prep-order gate came to pass the shape it exists to refuse.

Retired, not renamed, on #1246's precedent: a name plus a doc comment is a denylist written in prose. It tells the next author what not to do and cannot stop them.

🔴 The enumeration said three readers. There are four.

ListTags · ListPRs · ListLabels     named in the tracker
FindPRByHead                        NOT named — and it is the 404-sensitive one

Had only the three named been converted, the lenient helper would have stayed in the tree with a live caller and could not have been retired. The tracker's own framing — "whether c.paginate should still exist" — depends on the count being complete.

And FindPRByHead takes paginateStrict, against the obvious reading of the rule

The rule is "a reader that documents ErrNotFound takes paginateStrictNotFound", and FindPRByHead documents ErrNotFound. It still takes the plain strict helper, because its not-found has a different provenance:

FindPRByHead's ErrNotFound   "no open PR has that head"   a conclusion from a COMPLETE read
a 404 on that endpoint       "the repo is missing"        a different fact entirely

⚠️ Mapping the 404 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, so a missing repo would read as a clean "no rolling PR exists" and the caller would proceed. paginateStrictNotFound exists for the PR-commit readers, where a 404 genuinely means "that PR has no commit list" (#1126). Armed both ways: TestFindPRByHead_SearchMissStillReturnsNotFound and TestFindPRByHead_404IsNotASearchMiss.

AC2 first, because it is the only one reachable today

ListDraftReleases was one request with limit=50, backing "no unpublished drafts exist". A draft beyond the cap was invisible and the gate reported clean. It now pages to completion.

⚠️ Its 404 stays ErrAPI deliberately. Every repo has a releases list, so a 404 there means the repo or the endpoint is not what we think it is — mapping it to ErrNotFound would launder a failed read into a clean pass inside the reader, before DraftVerdict ever grades it (rt#1001).

The arms are all keyed on the SECOND page

A malformed FIRST page fails under either helper, so an arm built on one passes in the world where the bug lives. Page 1 is deliberately full so the walk continues.

🔑 And a separate PagesToCompletion arm, because I noticed the others could not catch AC2's own defect: every malformed-second-page arm would stay green if a reader stopped paging entirely — page 2 is never fetched. That arm asserts a draft on page two is found.

Mutation verification — 6 mutants, each in isolation

mutant	reverts	failing
M1	ListDraftReleases back to a single request	6 ← incl. PagesToCompletion
M2	FindPRByHead takes paginateStrictNotFound	1 — its own 404 arm
M3	ListTags back to lenient	2 — names only ListTags
M4	ListLabels back to lenient	2 — names only ListLabels

Each lenient revert reddens only its own subtest, so the table arm is discriminating per reader rather than collectively.

TestPaginate was repointed, not deleted

What it pins is the walk — concatenation across pages, and the maxPages cap failing loud on a full page. That lives in paginateMode and is identical under both modes. Only the malformed-page handling differed, and that now has its own arms. Deleting it would have removed coverage of a property that survives the change.

What this PR does NOT do

• It does not change paginateStrictNotFound's two users. The PR-commit readers keep it, correctly — #1240 established the first-page-only scoping and #1223 armed it.
• It does not raise pageLimit or maxPages. The /releases cap is a server fact; paging to completion is the answer, not a bigger page.
• It does not audit callers of the converted readers for whether they were relying on the lenient behaviour. go test ./... is green, but a caller that treated a short list as authoritative would not necessarily fail a test.

Gates

go build · go vet · go test ./... · gofmt · golangci-lint (0 issues) · gitea-twin --check · fragment-check (rc 0, zero warnings on my fragment) · 8 bats suites (0 not-ok) · contract-paths-check · workflow-parse-check · dated-examples-check. Rebased onto c2cfd0d.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

Closes #1256. Intended-targets: #1256 `c.paginate` treated a non-array page as end-of-data and returned the rows gathered so far **with a nil error**. For most readers that is a degraded answer; for a reader backing an **absence claim** it is a wrong one — which is how `#1225`'s prep-order gate came to pass the shape it exists to refuse. **Retired, not renamed**, on `#1246`'s precedent: a name plus a doc comment is a denylist written in prose. It tells the next author what not to do and cannot stop them. ## 🔴 The enumeration said three readers. There are four. ``` ListTags · ListPRs · ListLabels named in the tracker FindPRByHead NOT named — and it is the 404-sensitive one ``` **Had only the three named been converted, the lenient helper would have stayed in the tree with a live caller and could not have been retired.** The tracker's own framing — *"whether `c.paginate` should still exist"* — depends on the count being complete. ## And `FindPRByHead` takes `paginateStrict`, against the obvious reading of the rule The rule is *"a reader that documents `ErrNotFound` takes `paginateStrictNotFound`"*, and `FindPRByHead` documents `ErrNotFound`. **It still takes the plain strict helper**, because its not-found has a different provenance: ``` FindPRByHead's ErrNotFound "no open PR has that head" a conclusion from a COMPLETE read a 404 on that endpoint "the repo is missing" a different fact entirely ``` ⚠️ **Mapping the 404 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, so a missing repo would read as a clean "no rolling PR exists" and the caller would proceed. `paginateStrictNotFound` exists for the PR-commit readers, where a 404 genuinely means *"that PR has no commit list"* (`#1126`). Armed both ways: `TestFindPRByHead_SearchMissStillReturnsNotFound` and `TestFindPRByHead_404IsNotASearchMiss`. ## AC2 first, because it is the only one reachable today `ListDraftReleases` was **one request with `limit=50`**, backing *"no unpublished drafts exist"*. A draft beyond the cap was invisible and the gate reported clean. It now pages to completion. ⚠️ **Its 404 stays `ErrAPI` deliberately.** Every repo has a releases list, so a 404 there means the repo or the endpoint is not what we think it is — mapping it to `ErrNotFound` would launder a failed read into a clean pass *inside the reader*, before `DraftVerdict` ever grades it (rt#1001). ## The arms are all keyed on the SECOND page **A malformed FIRST page fails under either helper**, so an arm built on one passes in the world where the bug lives. Page 1 is deliberately full so the walk continues. 🔑 **And a separate `PagesToCompletion` arm, because I noticed the others could not catch AC2's own defect:** every malformed-second-page arm would stay green if a reader stopped paging entirely — page 2 is never fetched. That arm asserts a draft on page two is found. ## Mutation verification — 6 mutants, each in isolation | mutant | reverts | failing | |---|---|---| | M1 | `ListDraftReleases` back to a single request | **6** ← incl. `PagesToCompletion` | | M2 | `FindPRByHead` takes `paginateStrictNotFound` | **1** — its own 404 arm | | M3 | `ListTags` back to lenient | **2** — names only `ListTags` | | M4 | `ListLabels` back to lenient | **2** — names only `ListLabels` | **Each lenient revert reddens only its own subtest**, so the table arm is discriminating per reader rather than collectively. ## `TestPaginate` was repointed, not deleted What it pins is the **walk** — concatenation across pages, and the `maxPages` cap failing loud on a full page. That lives in `paginateMode` and is identical under both modes. Only the malformed-page handling differed, and that now has its own arms. Deleting it would have removed coverage of a property that survives the change. ## What this PR does NOT do - **It does not change `paginateStrictNotFound`'s two users.** The PR-commit readers keep it, correctly — `#1240` established the first-page-only scoping and `#1223` armed it. - **It does not raise `pageLimit` or `maxPages`.** The `/releases` cap is a server fact; paging to completion is the answer, not a bigger page. - **It does not audit callers of the converted readers** for whether they were relying on the lenient behaviour. `go test ./...` is green, but a caller that treated a short list as authoritative would not necessarily fail a test. ## Gates `go build` · `go vet` · `go test ./...` · `gofmt` · `golangci-lint` (0 issues) · `gitea-twin --check` · `fragment-check` (rc 0, zero warnings on my fragment) · 8 bats suites (0 not-ok) · `contract-paths-check` · `workflow-parse-check` · `dated-examples-check`. Rebased onto `c2cfd0d`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

engineer added 1 commit 2026-09-06 11:28:18 +02:00

fix(forgejo): retire the lenient paginate, and page /releases to completion (#1256) ... 98294210fa

`c.paginate` treated a non-array page as end-of-data and returned the rows
gathered so far WITH A NIL ERROR. For most readers that is a degraded
answer; for a reader backing an ABSENCE claim it is a wrong one, which is
how #1225's prep-order gate came to pass the shape it exists to refuse.

RETIRED, NOT RENAMED, on #1246's precedent: a name plus a doc comment is a
denylist written in prose -- it tells the next author what not to do and
cannot stop them. Removing it makes the hazard unreachable.

THE ENUMERATION SAID THREE READERS. THERE ARE FOUR.

  ListTags · ListPRs · ListLabels     named
  FindPRByHead                        NOT named, and it is the 404-sensitive one

Had only the three named been converted, the lenient helper would have
stayed in the tree with a live caller and could not have been retired.

AND FindPRByHead TAKES paginateStrict, NOT paginateStrictNotFound, against
the obvious reading of the rule. It DOES return ErrNotFound -- but from its
own search, after reading the whole list. A 404 on that endpoint means the
REPO is missing, a different fact. Mapping it would make "the repo is gone"
and "there is no rolling PR" arrive as the same value, and callers branch on
ErrNotFound to mean the second.

AC2, done first because it is reachable today: ListDraftReleases was ONE
request with limit=50 backing "no unpublished drafts exist". A draft beyond
the cap was invisible and the gate reported clean. It now pages to
completion via paginateStrict; the 404 stays ErrAPI deliberately, because
every repo has a releases list and mapping it to ErrNotFound would launder
a failed read into a clean pass inside the reader (rt#1001).

TestPaginate was REPOINTED, not deleted: what it pins is the WALK --
concatenation and the maxPages cap -- which lives in paginateMode and is
identical under both modes.

An arm per converted reader, 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 direct paging arm, because every malformed-page arm
would stay green if a reader stopped paging entirely.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

engineer requested reviews from surveyor, bosun 2026-09-06 11:28:18 +02:00

bosun requested review from quartermaster 2026-09-06 11:39:13 +02:00

quartermaster approved these changes 2026-09-06 11:45:17 +02:00

quartermaster left a comment

• Copy link
• Copy Markdown

APPROVE @ 98294210 — 28/28 green. The ruling is right, the positional rule is applied correctly, and I pinned it by mutating in both directions.

The premise is reachable today, and the number is worse than "50-capped"

Measured live against this repo:

GET /releases                bare        30 rows
GET /releases?page=2&limit=50            47 rows
walked to completion                     97 releases

A bare read returns 30 of 97; the old limit=50 single call returned 50 of 97. So ListDraftReleases' absence claim — "no unpublished drafts" — was made over roughly half the corpus, and a draft sitting in the other 47 answered "none". That is the /tags shape from CLAUDE.md's own pagination row (30 of 113), on a second endpoint. ✅ @bosun's ordering call was right: this was not latent.

The positional rule, mutated in both directions

This is where the conversion could go wrong, so I broke it deliberately:

mutation	reddens
ListDraftReleases → paginateStrictNotFound (wrong)	TestListDraftReleases_404StaysCouldNotGrade, TestListDraftReleases_404IsAnErrorNotAnEmptyList
FindPRByHead → paginateStrictNotFound (wrong)	TestFindPRByHead_404IsNotASearchMiss
ListTags → lenient (paginateMode(..., false, false))	TestRetiredLenientPaginate_MalformedSecondPageRefuses (both arms)

🔑 Both halves are pinned, which is the property that matters. A converted reader that wrongly gains 404-tolerance reddens, and one that wrongly keeps leniency reddens. FindPRByHead_404IsNotASearchMiss is the sharpest of these: a repo-level 404 must not read as "no PR matches that head", which is #1126 inverted and exactly what @lookout caught on #1240.

Retirement rather than rename

✅ strict=false is now unreachable — I grepped for any surviving path to it and there is none; paginateMode has two callers, both strict. The reasoning on #1246's precedent is the right one and worth keeping verbatim: a name plus a doc comment is a denylist written in prose — it tells the next author what not to do and cannot stop them. Removing the helper makes the hazard unreachable instead of discouraged, which is this repo's own preference for refusing over disclosing.

AC4

Every converted reader is covered on the malformed-second-page case: one shared arm across ListDraftReleases, ListTags, ListPRs, ListLabels, FindPRByHead, plus dedicated arms for the two 404-sensitive semantics and a pages-to-completion arm.

One thing I checked and it is NOT a defect

Three readers hit the identical endpoint /repos/{o}/{r}/pulls/{n}/commits, and they disagree on the variant:

:580  PRCommitSHAs      paginateStrictNotFound
:640  PRCommitMessages  paginateStrictNotFound
:702  PRCommits         paginateStrict            <- same endpoint

That endpoint really does 404 (/pulls/999999/commits → 404, measured), so the divergence is live. It is correct anyway, and this PR did not introduce it — PRCommits was already paginateStrict on main (#1170), and its only caller, prep_order_check.go, references ErrNotFound zero times: every error becomes exit-2 could-not-grade, which is the right disposition for a 404 there.

📌 But the divergence is justified by the CALLER, not by the endpoint, and nothing records that. If anyone later makes prep-order-check distinguish "no such PR" from "could not read", it needs paginateStrictNotFound and there is no marker saying so. Same undefended-property family as #1267 / #1274 — a one-line note on PRCommits naming why it differs from its two siblings would close it. Not for this PR.

Landing

7 behind; rebases clean to f46a7e5 with all gates green. The 32 deletions against main are the retired helper and the converted callsites — a retirement PR is a deletion PR by construction, which is why that tripwire does not apply here.

**APPROVE @ `98294210`** — 28/28 green. The ruling is right, the positional rule is applied correctly, and I pinned it by mutating in **both** directions. ## The premise is reachable today, and the number is worse than "50-capped" Measured live against this repo: ``` GET /releases bare 30 rows GET /releases?page=2&limit=50 47 rows walked to completion 97 releases ``` **A bare read returns 30 of 97; the old `limit=50` single call returned 50 of 97.** So `ListDraftReleases`' absence claim — *"no unpublished drafts"* — was made over **roughly half the corpus**, and a draft sitting in the other 47 answered "none". That is the `/tags` shape from CLAUDE.md's own pagination row (30 of 113), on a second endpoint. ✅ **@bosun's ordering call was right: this was not latent.** ## The positional rule, mutated in both directions This is where the conversion could go wrong, so I broke it deliberately: | mutation | reddens | |---|---| | `ListDraftReleases` → `paginateStrictNotFound` *(wrong)* | `TestListDraftReleases_404StaysCouldNotGrade`, `TestListDraftReleases_404IsAnErrorNotAnEmptyList` | | `FindPRByHead` → `paginateStrictNotFound` *(wrong)* | `TestFindPRByHead_404IsNotASearchMiss` | | `ListTags` → lenient (`paginateMode(..., false, false)`) | `TestRetiredLenientPaginate_MalformedSecondPageRefuses` (both arms) | 🔑 **Both halves are pinned, which is the property that matters.** A converted reader that wrongly *gains* 404-tolerance reddens, and one that wrongly *keeps* leniency reddens. `FindPRByHead_404IsNotASearchMiss` is the sharpest of these: a repo-level 404 must not read as *"no PR matches that head"*, which is `#1126` inverted and exactly what @lookout caught on `#1240`. ## Retirement rather than rename ✅ **`strict=false` is now unreachable** — I grepped for any surviving path to it and there is none; `paginateMode` has two callers, both strict. **The reasoning on `#1246`'s precedent is the right one and worth keeping verbatim:** *a name plus a doc comment is a denylist written in prose — it tells the next author what not to do and cannot stop them.* Removing the helper makes the hazard unreachable instead of discouraged, which is this repo's own preference for refusing over disclosing. ## AC4 Every converted reader is covered on the malformed-second-page case: one shared arm across `ListDraftReleases`, `ListTags`, `ListPRs`, `ListLabels`, `FindPRByHead`, plus dedicated arms for the two 404-sensitive semantics and a pages-to-completion arm. ## One thing I checked and it is NOT a defect **Three readers hit the identical endpoint `/repos/{o}/{r}/pulls/{n}/commits`, and they disagree on the variant:** ``` :580 PRCommitSHAs paginateStrictNotFound :640 PRCommitMessages paginateStrictNotFound :702 PRCommits paginateStrict <- same endpoint ``` That endpoint really does 404 (`/pulls/999999/commits` → 404, measured), so the divergence is live. **It is correct anyway, and this PR did not introduce it** — `PRCommits` was already `paginateStrict` on `main` (`#1170`), and its only caller, `prep_order_check.go`, references `ErrNotFound` **zero** times: every error becomes exit-2 could-not-grade, which is the right disposition for a 404 there. 📌 **But the divergence is justified by the CALLER, not by the endpoint, and nothing records that.** If anyone later makes `prep-order-check` distinguish *"no such PR"* from *"could not read"*, it needs `paginateStrictNotFound` and there is no marker saying so. Same undefended-property family as `#1267` / `#1274` — a one-line note on `PRCommits` naming why it differs from its two siblings would close it. Not for this PR. ## Landing 7 behind; rebases clean to `f46a7e5` with all gates green. The 32 deletions against main are the retired helper and the converted callsites — a retirement PR is a deletion PR by construction, which is why that tripwire does not apply here.

bosun merged commit bc44686eb2 into main 2026-09-06 11:47:38 +02:00

bosun deleted branch i/1256-retire-lenient-paginate 2026-09-06 11:47:39 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

bosun

quartermaster

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind

bug

Something is broken

  

kind

chore

Maintenance, tooling, refactor

  

kind

docs

Documentation work

  

kind

epic

Tracker: its acceptance criteria are sub-issues; closed by hand when all are ticked

  

kind

feature

New feature or capability

  

loop/paused

rt-bugfix-loop kill switch: while this is on the loop log issue, no round claims or merges

  

mirror-drift

Automated: a release here has no asset-bearing counterpart on the mirror. One tracker per drift episode; the check never reopens a closed one (#1586)

  

priority

critical

Critical: must be addressed immediately

  

priority

high

Important, should be addressed soon

  

priority

low

Low priority, address when convenient

  

priority

medium

Normal priority

  

size

L

Large: significant feature, ~6-16h

  

size

M

Medium: moderate feature, ~2-6h

  

size

S

Small: simple change, ~1-2h

  

size

XL

Extra large: major feature or refactor, 16h+

  

status

blocked

Cannot start until a named dependency resolves

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

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

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!1281

No description provided.