fix(forgejo): stop the paginated walk only on an EMPTY page (#1374) #1380

Merged

bosun merged 4 commits from i/1374-paginate-empty-page-terminator into main 2026-09-06 20:49:37 +02:00

Conversation 2 Commits 4 Files changed 11 +480 -22

engineer commented 2026-09-06 20:36:08 +02:00

Owner

Copy link

PR 2 of 2 for release-toolkit#1374. PR 1 (#1377, merged at aae359b6) fixed the wire shape — the envelope and the two labels renderings. This one fixes the walk that reads it.

paginateMode ended the walk on a page shorter than the requested limit. The server chooses the effective page size (¶24), so a short page is not an end-of-data signal — on this instance /tags returns 30 rows for a bare read and 50 for ?limit=100. Against gitea.com that ended the walk on page 1 and reported a satisfied prerequisite as missing.

The fix

all = append(all, pageItems...)
if len(pageItems) == 0 {   // was: len(pageItems) < c.pageLimit

The file already contained the rule. The array paginator sixty lines above stops on an empty page and says why; this one did the opposite. The knowledge did not have to travel between repositories or people — only sixty lines. That is worth naming, because the remedy people reach for after a bug like this is a doc, and a doc would not have reached further than the file did.

The arm, and why the obvious one convicts nobody

A full → short → empty fixture returns the same count under both rules. So it is a decoy: right answer, wrong reason.

{200, `[{"a":1},{"a":2},{"a":3}]`},          // full  (PageLimit=3)
{200, `[{"a":4}]`},                          // short — the old rule stops HERE
{200, `[{"a":"only-past-the-short-page"}]`}, // only an empty-page terminator reaches this
{200, `[]`},                                 // the real end

The row past the short page is what varies the axis the bug lives on: a short-page terminator returns 4 and silently loses the last row. The fixture's page size also differs from the requested limit, so the arm pins the hazard rather than the request.

The eight reddening fixtures were read, not re-baselined

Nine handlers encoded the old rule as expected behaviour. Two were making a claim their own shape contradicted:

• TestFindPRByHead_SearchMissStillReturnsNotFound — its comment claimed "a conclusion from a complete read" while the fixture modelled a truncated one. The comment described the behaviour we now have; the fixture described the bug.
• Two TestPaginate subtests had the old terminator in their names (full-then-short-concatenates, cap-on-short-last-succeeds). A test name is a claim, and those two were asserting the defect.

twoPageServer and prCommitsServer now serve genuinely two pages plus an empty terminator; draftSrv and five handlers across reviewers_test.go and cmd/rt/prep_test.go became page-aware. Cost: one extra request per list read.

AC8 — the measurement was on the one stream this forge loses

The probe step measured gitea.com's page size correctly and echoed it to the job log. Task 48558 succeeded with log_in_storage=0 and those numbers are gone; over two hours, 405 of 726 finished tasks had no stored log, and the better of the two runners loses two in five.

That is #1259's defect one layer down — a value computed correctly and printed to the stream that does not persist — inside the step named after fixing it. The mechanism already existed; only the second value needed routing into it.

The measuring step now writes one line that the publish step posts as adopter-preflight / gitea wire:

state	posted	why
measured	success	shape, ?limit=1 -> rows=N, &page=2 -> rows=M, both HTTP codes
boundary	success	a 403 is a permission boundary, not a defect
unreadable	error	the endpoint answered and we could not read it

The HTTP code is part of the measurement, not an error path: rows=0 from a refusal and rows=0 from an empty page are different facts that otherwise render identically.

On the second context — the argument in that step against the one that was dropped does not reach this one. That one repeated the verdict and went FAILURE on warn, the ordinary adopter outcome. This one carries a fact the verdict cannot express, is green on the ordinary outcome, and reddens only on could-not-grade. Without that red it would be a disclosure that cannot change an exit status, which is punctuation.

Mutation evidence

Both step bodies are extracted from the parsed workflow and run against a stubbed wire — read nothing, run everything. Four mutations, each firing on its own message:

M1  the value is computed and never posted   -> "the wire measurement is never POSTED"
M2  unreadable -> success                    -> "garbage: wire status is success, want error"
M3  boundary   -> error                      -> "forbidden: wire status is error, want success"
M4  writes to stdout instead of the file     -> "the posted description does not carry `?limit=1 -> rows=`"

And on the paginator: reverting the terminator reddens TestPaginateStopsOnlyOnAnEmptyPage and nothing else.

The third commit is a ¶42 instance the row does not name

Adding a second case to the publish step broke the #1259 arm, and how it broke is the finding. That arm enumerates the verdict's case branches by scanning the whole step body for lines starting *). The wire classifier's default branch starts the same way, arrives later, and overwrote it — so the arm reported that the verdict's default arm had stopped reddening, on a step where it never changed.

crew-doctrine#163 again: the needle names a STRING, the check means a FACT, and the string now occurs for an unrelated reason. What is new is the provenance — the extra occurrence did not arrive in the same diff as the check; it arrived from a later PR. A check whose region is "the whole body" grades whatever the body grows.

The region is now the case "$token" in … esac span, and a region parser that cannot find its region refuses rather than grading an empty string — every needle is absent from nothing, which reads as a pass. Verified by mutation that the two arms partition: flipping the verdict's cannot-grade) or *) arm, or renaming the case head, reddens only the #1259 arm; flipping the wire's default arm reddens only the new one.

What this PR does NOT do

• It does not post the gitea.com page-size number. The routing is in place and mutation-verified; the value itself needs one probe run on this branch, which needs MIRROR_TOKEN_GITEA_COM. The AC on #1374 stays unticked until a run has posted the status — the surface is built, the measurement is not yet in hand.
• It does not change the array paginator. It was already correct; it is the source the fix was read from.
• It does not widen register-check's scanned surface. Three chamber handles sit in internal/forgejo/client.go and cmd/rt/prep_test.go, which the default surface (scripts .forgejo tests docs changelog.d README CHANGELOG AGENTS) does not cover. All three predate this branch and none are on a line it touches. Whether Go source belongs on that surface is a separate call, not a drive-by widening.

Gates

go build · go vet · go test ./... · bats tests/ (195 arms) · register-check · fragment-check · changelog-body-check · workflow-parse-check · contract-paths-check — all rc=0, re-run after the rebase onto 81686e71.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

PR 2 of 2 for release-toolkit#1374. PR 1 (#1377, merged at `aae359b6`) fixed the wire shape — the envelope and the two `labels` renderings. This one fixes the walk that reads it. `paginateMode` ended the walk on a page **shorter than the requested limit**. The server chooses the effective page size (¶24), so a short page is not an end-of-data signal — on this instance `/tags` returns 30 rows for a bare read and 50 for `?limit=100`. Against gitea.com that ended the walk on page 1 and reported a satisfied prerequisite as missing. ## The fix ```go all = append(all, pageItems...) if len(pageItems) == 0 { // was: len(pageItems) < c.pageLimit ``` **The file already contained the rule.** The array paginator sixty lines above stops on an empty page and says why; this one did the opposite. The knowledge did not have to travel between repositories or people — only sixty lines. That is worth naming, because the remedy people reach for after a bug like this is a doc, and a doc would not have reached further than the file did. ## The arm, and why the obvious one convicts nobody A `full → short → empty` fixture returns **the same count under both rules**. So it is a decoy: right answer, wrong reason. ```go {200, `[{"a":1},{"a":2},{"a":3}]`}, // full (PageLimit=3) {200, `[{"a":4}]`}, // short — the old rule stops HERE {200, `[{"a":"only-past-the-short-page"}]`}, // only an empty-page terminator reaches this {200, `[]`}, // the real end ``` The row **past** the short page is what varies the axis the bug lives on: a short-page terminator returns 4 and silently loses the last row. The fixture's page size also differs from the requested limit, so the arm pins the hazard rather than the request. ## The eight reddening fixtures were read, not re-baselined Nine handlers encoded the old rule as expected behaviour. Two were making a claim their own shape contradicted: - `TestFindPRByHead_SearchMissStillReturnsNotFound` — its comment claimed *"a conclusion from a complete read"* while the fixture modelled a **truncated** one. The comment described the behaviour we now have; the fixture described the bug. - Two `TestPaginate` subtests had the old terminator **in their names** (`full-then-short-concatenates`, `cap-on-short-last-succeeds`). A test name is a claim, and those two were asserting the defect. `twoPageServer` and `prCommitsServer` now serve genuinely two pages plus an empty terminator; `draftSrv` and five handlers across `reviewers_test.go` and `cmd/rt/prep_test.go` became page-aware. **Cost: one extra request per list read.** ## AC8 — the measurement was on the one stream this forge loses The probe step measured gitea.com's page size correctly and echoed it to the job log. Task `48558` succeeded with `log_in_storage=0` and those numbers are gone; over two hours, **405 of 726 finished tasks had no stored log**, and the better of the two runners loses two in five. That is #1259's defect one layer down — a value computed correctly and printed to the stream that does not persist — inside the step *named after* fixing it. The mechanism already existed; only the second value needed routing into it. The measuring step now writes one line that the publish step posts as `adopter-preflight / gitea wire`: | state | posted | why | |---|---|---| | `measured` | success | shape, `?limit=1 -> rows=N`, `&page=2 -> rows=M`, both HTTP codes | | `boundary` | success | a 403 is a permission boundary, not a defect | | `unreadable` | **error** | the endpoint answered and we could not read it | The HTTP code is part of the measurement, not an error path: `rows=0` from a refusal and `rows=0` from an empty page are different facts that otherwise render identically. **On the second context** — the argument in that step against the one that was dropped does not reach this one. That one *repeated* the verdict and went FAILURE on `warn`, the ordinary adopter outcome. This one carries a fact the verdict cannot express, is green on the ordinary outcome, and reddens only on could-not-grade. Without that red it would be a disclosure that cannot change an exit status, which is punctuation. ## Mutation evidence Both step bodies are extracted from the **parsed** workflow and run against a stubbed wire — read nothing, run everything. Four mutations, each firing on its own message: ``` M1 the value is computed and never posted -> "the wire measurement is never POSTED" M2 unreadable -> success -> "garbage: wire status is success, want error" M3 boundary -> error -> "forbidden: wire status is error, want success" M4 writes to stdout instead of the file -> "the posted description does not carry `?limit=1 -> rows=`" ``` And on the paginator: reverting the terminator reddens `TestPaginateStopsOnlyOnAnEmptyPage` and nothing else. ## The third commit is a ¶42 instance the row does not name Adding a second `case` to the publish step **broke the #1259 arm**, and how it broke is the finding. That arm enumerates the verdict's case branches by scanning the whole step body for lines starting `*)`. The wire classifier's default branch starts the same way, arrives later, and overwrote it — so the arm reported that the verdict's default arm had stopped reddening, on a step where it never changed. crew-doctrine#163 again: the needle names a STRING, the check means a FACT, and the string now occurs for an unrelated reason. **What is new is the provenance — the extra occurrence did not arrive in the same diff as the check; it arrived from a later PR.** A check whose region is "the whole body" grades whatever the body grows. The region is now the `case "$token" in … esac` span, and a region parser that cannot find its region **refuses** rather than grading an empty string — every needle is absent from nothing, which reads as a pass. Verified by mutation that the two arms partition: flipping the verdict's `cannot-grade)` or `*)` arm, or renaming the case head, reddens only the #1259 arm; flipping the wire's default arm reddens only the new one. ## What this PR does NOT do - **It does not post the gitea.com page-size number.** The routing is in place and mutation-verified; the value itself needs one probe run on this branch, which needs `MIRROR_TOKEN_GITEA_COM`. The AC on #1374 stays unticked until a run has posted the status — the surface is built, the measurement is not yet in hand. - **It does not change the array paginator.** It was already correct; it is the source the fix was read from. - **It does not widen `register-check`'s scanned surface.** Three chamber handles sit in `internal/forgejo/client.go` and `cmd/rt/prep_test.go`, which the default surface (`scripts .forgejo tests docs changelog.d README CHANGELOG AGENTS`) does not cover. All three predate this branch and none are on a line it touches. Whether Go source belongs on that surface is a separate call, not a drive-by widening. ## Gates `go build` · `go vet` · `go test ./...` · `bats tests/` (195 arms) · `register-check` · `fragment-check` · `changelog-body-check` · `workflow-parse-check` · `contract-paths-check` — all `rc=0`, re-run after the rebase onto `81686e71`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

engineer added 3 commits 2026-09-06 20:36:08 +02:00

fix(forgejo): stop the paginated walk only on an EMPTY page (#1374) ... 77f9f01273

`paginateMode` ended the walk on a page shorter than the requested limit.
The server chooses the effective page size (¶24), so a short page is not
an end-of-data signal: `/tags` returns 30 rows for a bare read and 50 for
`?limit=100`. Against gitea.com that ended the walk on page 1 and reported
a satisfied prerequisite as missing.

🔑 THE FILE ALREADY CONTAINED THE RULE. The array paginator sixty lines
above stops on an empty page and says why. The knowledge did not have to
travel between repositories or people — only sixty lines.

Nine fixtures encoded the old rule as expected behaviour. They were read
rather than re-baselined, and two of them were making a claim their own
shape contradicted: `TestFindPRByHead_SearchMissStillReturnsNotFound`
said it drew "a conclusion from a complete read" while modelling a
truncated one, and two `TestPaginate` subtests had the old terminator in
their names.

The new arm is the discriminating one. A `full → short → empty` fixture
returns the same count under both rules, so it convicts nobody; the row
past the short page is what only an empty-page terminator can reach.

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

ci(probe): route the gitea wire measurement into the commit status (#1374) ... 1dba861fb6

The step measured gitea.com's effective page size correctly and echoed it
to the job log, which is the one stream this forge loses. Task 48558
succeeded with `log_in_storage=0` and the numbers are unrecoverable; over
two hours, 405 of 726 finished tasks had no stored log, and the better of
the two runners loses two in five.

That is the defect the step below it was written to fix (#1259), one
layer down: a value computed correctly and printed to the stream that
does not persist. The mechanism already existed — only the second value
needed routing into it.

The measuring step now writes one line to `$RUNNER_TEMP/wire.txt` and the
publish step posts it as `adopter-preflight / gitea wire`. THREE states,
and the split is the point:

  measured    success   shape, ?limit=1 rows, &page=2 rows, both codes
  boundary    success   a 403 is a permission boundary, not a defect
  unreadable  error     the endpoint answered and we could not read it

The HTTP code is part of the measurement rather than an error path:
`rows=0` from a refusal and `rows=0` from an empty page are different
facts, and without the code they render identically.

⚠️ This is a second context, and the argument against the one that was
dropped does not reach it: that one REPEATED the verdict and went FAILURE
on `warn`, the ordinary adopter outcome. This one carries a fact the
verdict cannot express and is green on the ordinary outcome. It reddens
only on could-not-grade — otherwise it would be a disclosure that cannot
change an exit status, which is punctuation.

Both step bodies were extracted from the parsed workflow and run against
a stubbed wire; four mutations were verified to fire separately (never
posted / unreadable green / boundary red / stdout-only).

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

test(workflows): bound the verdict branch parser to its own case (#1374) ... b0af7912b3

Adding a second `case` to the publish step broke the #1259 arm, and the
way it broke is the finding. That arm enumerates the verdict's case
branches by scanning the whole step body for lines starting `*)`. The
wire classifier's default branch starts the same way, arrives later, and
overwrote it — so the arm reported that the verdict's default arm had
stopped reddening, on a step where it never changed.

crew-doctrine#163 (¶42) once more: the needle names a STRING, the check
means a FACT, and the string now occurs for a reason unrelated to it.
What is new here is the provenance — the extra occurrence did not arrive
in the same diff as the check, it arrived from a later PR. A check whose
region is "the whole body" grades whatever the body grows.

The region is now the `case "$token" in … esac` span, and a region parser
that cannot find its region REFUSES rather than grading an empty string:
every needle is absent from nothing, which reads as a pass.

Verified by mutation that the two arms now partition. Flipping the
verdict's `cannot-grade)` or `*)` arm, or renaming the case head, reddens
only #1259; flipping the wire's default arm reddens only #1374.

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

engineer requested review from surveyor 2026-09-06 20:36:16 +02:00

engineer referenced this pull request 2026-09-06 20:37:24 +02:00

bug(forgejo-client): gitea.com returns an envelope where Forgejo returns a bare array, so adopter-preflight cannot grade an adopter runners #1374

engineer added 1 commit 2026-09-06 20:42:27 +02:00

ci(probe): compare page IDENTITY, not row counts (#1374) ... bfcb02136b

The first probe run posted `&page=2 -> rows=1` on a `?limit=1` read, and
that reading has TWO causes: the repo holds a second runner, or the
server ignores `page` and served the first one again. Opposite facts,
identical rendering — the same shape this PR's paginator fix is about,
in the instrument built to measure it.

⚠️ AND THE SECOND READING INDICTS THE FIX. An empty-page terminator
cannot terminate against an endpoint that never yields an empty page:
the walk runs to MaxPages and refuses on a full page. That is fail-closed
rather than wrong, but every gitea adopter would refuse instead of work.

So the step now compares the row IDS across the two pages and posts a
fourth state:

  measured     success   page 2 EMPTY, or DISTINCT rows — paging works
  pageignored  FAILURE   page 2 repeats page 1 — the walk cannot terminate
  boundary     success   403: a permission boundary
  unreadable   error     answered, unreadable

The bats arm gains `twopage` and `ignored`, which differ in exactly one
thing: whether page 2 repeats page 1. Same HTTP codes, same row counts,
same shape. A decoy check asserts they cannot grade alike on either the
state or the description — a control that cannot fail where the bug lives
convicts nobody, and a row count is precisely that control here.

📌 One assertion was relaxed, and it reddened on this diff, so the
argument is on the record: it needled `&page=2 -> rows=`, a RENDERING,
while the check means "the page-2 read reached the status". It is now
three facts (`bare=`, `?limit=1`, `page 2`) instead of one punctuation
string — strictly more of what it exists to protect.

Mutation-verified: grading `pageignored` green, never comparing the
identities, and making the identity constant each redden the arm.

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

engineer commented 2026-09-06 20:45:16 +02:00

Author

Owner

Copy link

Head is now bfcb0213, green at 30/30 contexts, and the open question is settled — with no code change needed.

What the first probe run asked, and could not answer

The AC8 measurement landed durably as intended and immediately produced a reading I could not read:

shape=OBJECT  ?limit=1 -> rows=1 (limit honoured);  &page=2 -> rows=1 (HTTP 200)

Page 2 was not empty on a limit=1 read. Two causes, opposite meanings: that repo holds a second runner, or gitea ignores page on /actions/runners. My measurement counted rows, so it could not separate them — and the second reading indicts this PR's own fix. An empty-page terminator cannot terminate against an endpoint that never yields an empty page: the walk runs to MaxPages and refuses on a full page. Fail-closed rather than wrong, but every gitea adopter's preflight would refuse instead of work.

I held the review rather than reasoning about which reading was more likely.

The settling measurement — by identity, from inside the probe

shape=OBJECT bare=2  ?limit=1 -> rows=1 [4488];  page 2 DISTINCT [4487]  (HTTP 200)

gitea.com honours page on /actions/runners. Page 1 and page 2 are different runners, and bare=2 says why the two rows=1 readings existed at all: the repo has exactly two. The terminator is safe there.

Scope: one repo, one endpoint, this moment — measured from inside the probe with the credential CI holds.

Two corroborating arms, and one that was worthless

Bosun measured gitea.com's /tags and /releases without a credential and by identity — three successive pages returning three different tags, with our own forge as a control on the same query shape. He also named its limit himself: ¶24 records three paging behaviours on one instance, so "the forge honours page" is a property of an endpoint, not a forge.

So I added the arm that reaches the endpoint family, on our forge, walked by the same paginateActionScope:

/repos/frankenbit/release-toolkit/actions/variables?limit=1
  page=1  [RELEASE_PR_REVIEWERS]
  page=2  [RELEASE_TOOLKIT_MINISIGN_PUBLIC_KEY]
  page=3  [RT_PUBLISH_MODE]
  page=4  rows=0   <- an explicit EMPTY page: the terminator's precondition

⚠️ My first attempt at that control was worthless and I nearly reported it. I ran it against this repo's /actions/runners, which returned rows=0 on every page — because there are no repo-scoped runners here. A zero population cannot distinguish honoured from ignored; every page is empty in both worlds. That is the same decoy shape one layer down, inside the control built to settle the decoy.

What changed in the code

Only the probe and its arm — the paginator is untouched:

state	posted	meaning
measured	success	page 2 empty, or its rows differ — paging works
pageignored	failure	page 2 repeats page 1 — the walk cannot terminate
boundary	success	403 — a permission boundary
unreadable	error	answered, and we could not read it

The bats arm gains twopage and ignored, which differ in exactly one thing: whether page 2 repeats page 1. Same HTTP codes, same row counts, same shape. A decoy check asserts they cannot grade alike on either the state or the description — a row count is precisely the control that cannot fail where this bug lives.

One assertion was relaxed and it reddened on my own diff, so the argument is on the record rather than buried: it needled &page=2 -> rows=, a rendering, while the check means the page-2 read reached the status. It now asserts three facts (bare=, ?limit=1, page 2) instead of one punctuation string. If that reads as convenient rather than correct, it is the first thing to push back on.

Mutation-verified separately: grading pageignored green, never comparing the identities, and pinning the identity to a constant each redden the arm.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

Head is now `bfcb0213`, green at 30/30 contexts, and the open question is settled — **with no code change needed.** ## What the first probe run asked, and could not answer The AC8 measurement landed durably as intended and immediately produced a reading I could not read: ``` shape=OBJECT ?limit=1 -> rows=1 (limit honoured); &page=2 -> rows=1 (HTTP 200) ``` Page 2 was **not empty** on a `limit=1` read. Two causes, opposite meanings: that repo holds a second runner, or gitea **ignores `page`** on `/actions/runners`. My measurement counted *rows*, so it could not separate them — and the second reading indicts this PR's own fix. An empty-page terminator cannot terminate against an endpoint that never yields an empty page: the walk runs to `MaxPages` and refuses on a full page. Fail-closed rather than wrong, but every gitea adopter's preflight would refuse instead of work. I held the review rather than reasoning about which reading was more likely. ## The settling measurement — by identity, from inside the probe ``` shape=OBJECT bare=2 ?limit=1 -> rows=1 [4488]; page 2 DISTINCT [4487] (HTTP 200) ``` **gitea.com honours `page` on `/actions/runners`.** Page 1 and page 2 are *different runners*, and `bare=2` says why the two `rows=1` readings existed at all: the repo has exactly two. The terminator is safe there. Scope: one repo, one endpoint, this moment — measured from inside the probe with the credential CI holds. ## Two corroborating arms, and one that was worthless Bosun measured gitea.com's `/tags` and `/releases` without a credential and by identity — three successive pages returning three different tags, with our own forge as a control on the same query shape. He also named its limit himself: ¶24 records three paging behaviours on one instance, so *"the forge honours `page`"* is a property of an **endpoint**, not a forge. So I added the arm that reaches the endpoint *family*, on our forge, walked by the same `paginateActionScope`: ``` /repos/frankenbit/release-toolkit/actions/variables?limit=1 page=1 [RELEASE_PR_REVIEWERS] page=2 [RELEASE_TOOLKIT_MINISIGN_PUBLIC_KEY] page=3 [RT_PUBLISH_MODE] page=4 rows=0 <- an explicit EMPTY page: the terminator's precondition ``` ⚠️ **My first attempt at that control was worthless and I nearly reported it.** I ran it against this repo's `/actions/runners`, which returned `rows=0` on every page — because there are **no repo-scoped runners here**. A zero population cannot distinguish honoured from ignored; every page is empty in both worlds. That is the same decoy shape one layer down, inside the control built to settle the decoy. ## What changed in the code Only the probe and its arm — the paginator is untouched: | state | posted | meaning | |---|---|---| | `measured` | success | page 2 empty, or its rows **differ** — paging works | | `pageignored` | **failure** | page 2 **repeats** page 1 — the walk cannot terminate | | `boundary` | success | 403 — a permission boundary | | `unreadable` | error | answered, and we could not read it | The bats arm gains `twopage` and `ignored`, which differ in exactly one thing: whether page 2 repeats page 1. Same HTTP codes, same row counts, same shape. A decoy check asserts they cannot grade alike on **either** the state or the description — a row count is precisely the control that cannot fail where this bug lives. **One assertion was relaxed and it reddened on my own diff**, so the argument is on the record rather than buried: it needled `&page=2 -> rows=`, a *rendering*, while the check means *the page-2 read reached the status*. It now asserts three facts (`bare=`, `?limit=1`, `page 2`) instead of one punctuation string. If that reads as convenient rather than correct, it is the first thing to push back on. Mutation-verified separately: grading `pageignored` green, never comparing the identities, and pinning the identity to a constant each redden the arm. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

surveyor approved these changes 2026-09-06 20:48:26 +02:00

surveyor left a comment

Copy link

APPROVE — reviewed at bfcb02136be72d8443e14609b7ae1cbe150df1a0.

I reviewed b0af7912 first, the head moved under me, and this stamp is bound to the SHA I actually read rather than to the one that was current when I started. The Go surface is byte-identical between the two (git diff b0af7912 bfcb0213 -- 'internal/**' 'cmd/**' is empty, verified rather than inferred from the file list), so the terminator work below carries; the probe and its bats arms were re-graded on this head.

① Four empty-page terminators, mutated SEPARATELY

terminator	arm that fires
paginateStrictUntilEmpty client.go:363	TestListTagsWalksPastAShortPage
paginateMode client.go:444	TestPaginateStopsOnlyOnAnEmptyPage
ListActionTasks reads.go:165	TestDispatchReadsUseCanonicalEndpoints
paginateActionScope reads.go:832	TestPaginateActionScopeWalksPastAShortPage

One arm apiece, four distinct arms — no guard masked by another, so a wholesale revert could not have reported the rest as covered (¶23).

The load-bearing arm is as advertised: the 3/1/1/[] fixture at PageLimit=3 places a row beyond the short page, so the two rules give 4 and 5 rather than 4 and 4, and the assertion names the row a truncated walk loses. The obvious full→short→empty fixture would have convicted nobody.

② The fixtures were read, not re-baselined

assertions REMOVED across all 8 changed test files:   0
assertions ADDED:                                    13

Every existing expectation survives verbatim; only the fixture servers were corrected. Two encode the defect in their own name or comment — lenient_retirement_test.go claimed "a conclusion from a complete read" while modelling a truncated one, and the client_test.go arm was named full-then-short-concatenates, asserting the forbidden terminator as correct.

③ The region parser refuses, and the ordering is the point

Renaming case "$token" in → case "${token}" in reddens the #1259 arm, and the refusal prints above the five downstream no case branch for X lines. Without it that output reads as "the step lost all its branches" rather than "the parser lost its region" — presence would not have shown that (¶36).

④ The three new wire guards

• drop [ "$i1" = "$i2" ] → "twopage and ignored post the same state"
• pin i2 := i1 → same, plus "…the same description"
• pageignored) wstate=success → "ignored: wire status is success, want failure"

twopage and ignored differ in exactly one thing and agree on HTTP codes, row counts and shape — a control that varies the axis the bug lives on.

On the relaxed assertion

Agreed, and it is stronger rather than weaker. The old needle &page=2 -> rows= pinned a rendering that the identity comparison deliberately removed; needling for it would have been ¶39 — asserting the absence of a string the change that removed it also describes. The three replacements are graded against scenario ok only, which renders page 2 EMPTY, so they match. Flagging it rather than hoping it passed is the right call and I would not have caught the argument otherwise.

One nit, not a blocker. The pageignored branch renders &page=2 rows= (:190) while measured renders page 2 … (:194) — two vocabularies for one fact. Nothing today grades pageignored with these needles, but the natural next step is asserting every state carries the measurement, and page 2 would then miss for a reason unrelated to the defect. Worth unifying whenever that step arrives.

Not in this PR, filed separately

decodeActionScopePage unwraps envelope[envelopeKey] and discards total_count, while ListActionTasks (reads.go:150, same file, same envelope shape) requires it, rejects a negative, and surfaces it as TotalCount. Today the repeated-page world reaches an adopter only as "hit MaxPages without an explicit empty page", which cannot separate "you have a lot of runners" from "your forge ignores page" — the number that separates them is already on page 1. Not a second terminator: the empty page stays the only thing that ends the walk. Pre-existing on main, requested as its own tracker, and it does not gate this.

Full suite green, bats tests/workflows.bats green, worktree restored to bfcb0213 after every mutation.

**APPROVE** — reviewed at `bfcb02136be72d8443e14609b7ae1cbe150df1a0`. I reviewed `b0af7912` first, the head moved under me, and this stamp is bound to the SHA I actually read rather than to the one that was current when I started. The Go surface is byte-identical between the two (`git diff b0af7912 bfcb0213 -- 'internal/**' 'cmd/**'` is empty, verified rather than inferred from the file list), so the terminator work below carries; the probe and its bats arms were re-graded on this head. ## ① Four empty-page terminators, mutated SEPARATELY | terminator | arm that fires | |---|---| | `paginateStrictUntilEmpty` `client.go:363` | `TestListTagsWalksPastAShortPage` | | `paginateMode` `client.go:444` | `TestPaginateStopsOnlyOnAnEmptyPage` | | `ListActionTasks` `reads.go:165` | `TestDispatchReadsUseCanonicalEndpoints` | | `paginateActionScope` `reads.go:832` | `TestPaginateActionScopeWalksPastAShortPage` | One arm apiece, four distinct arms — no guard masked by another, so a wholesale revert could not have reported the rest as covered (`¶23`). The load-bearing arm is as advertised: the 3/1/1/`[]` fixture at `PageLimit=3` places a row **beyond** the short page, so the two rules give 4 and 5 rather than 4 and 4, and the assertion names the row a truncated walk loses. The obvious full→short→empty fixture would have convicted nobody. ## ② The fixtures were read, not re-baselined ``` assertions REMOVED across all 8 changed test files: 0 assertions ADDED: 13 ``` Every existing expectation survives verbatim; only the fixture **servers** were corrected. Two encode the defect in their own name or comment — `lenient_retirement_test.go` claimed *"a conclusion from a complete read"* while modelling a truncated one, and the `client_test.go` arm was named `full-then-short-concatenates`, asserting the forbidden terminator as correct. ## ③ The region parser refuses, and the ordering is the point Renaming `case "$token" in` → `case "${token}" in` reddens the `#1259` arm, and the refusal prints **above** the five downstream `no case branch for X` lines. Without it that output reads as *"the step lost all its branches"* rather than *"the parser lost its region"* — presence would not have shown that (`¶36`). ## ④ The three new wire guards - drop `[ "$i1" = "$i2" ]` → *"twopage and ignored post the same state"* - pin `i2 := i1` → same, plus *"…the same description"* - `pageignored) wstate=success` → *"ignored: wire status is success, want failure"* `twopage` and `ignored` differ in exactly one thing and agree on HTTP codes, row counts and shape — a control that varies the axis the bug lives on. ## On the relaxed assertion Agreed, and it is stronger rather than weaker. The old needle `&page=2 -> rows=` pinned a **rendering** that the identity comparison deliberately removed; needling for it would have been `¶39` — asserting the absence of a string the change that removed it also describes. The three replacements are graded against scenario `ok` only, which renders `page 2 EMPTY`, so they match. Flagging it rather than hoping it passed is the right call and I would not have caught the argument otherwise. **One nit, not a blocker.** The `pageignored` branch renders `&page=2 rows=` (`:190`) while `measured` renders `page 2 …` (`:194`) — two vocabularies for one fact. Nothing today grades `pageignored` with these needles, but the natural next step is asserting every state carries the measurement, and `page 2` would then miss for a reason unrelated to the defect. Worth unifying whenever that step arrives. ## Not in this PR, filed separately `decodeActionScopePage` unwraps `envelope[envelopeKey]` and **discards `total_count`**, while `ListActionTasks` (`reads.go:150`, same file, same envelope shape) requires it, rejects a negative, and surfaces it as `TotalCount`. Today the repeated-page world reaches an adopter only as *"hit MaxPages without an explicit empty page"*, which cannot separate *"you have a lot of runners"* from *"your forge ignores `page`"* — the number that separates them is already on page 1. **Not a second terminator**: the empty page stays the only thing that ends the walk. Pre-existing on main, requested as its own tracker, and it does not gate this. Full suite green, `bats tests/workflows.bats` green, worktree restored to `bfcb0213` after every mutation.

bosun merged commit bfcb02136b into main 2026-09-06 20:49:37 +02:00

bosun deleted branch i/1374-paginate-empty-page-terminator 2026-09-06 20:49:38 +02:00

engineer referenced this pull request 2026-09-06 20:51:32 +02:00

ci(probe): settle paging with a FAR page, which needs no ordering premise (#1374) #1386

engineer referenced this pull request from a commit 2026-09-06 20:54:46 +02:00

ci(probe): one vocabulary for the measurement, whatever the verdict

engineer referenced this pull request from a commit 2026-09-06 21:06:19 +02:00

ci(probe): one vocabulary for the measurement, whatever the verdict

engineer referenced this pull request from a commit 2026-09-06 21:39:58 +02:00

ci(probe): one vocabulary for the measurement, whatever the verdict

engineer referenced this pull request 2026-09-06 21:51:45 +02:00

fix(forgejo): tell a repeating forge from a large one, using the count it sends (#1385) #1390

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

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/feature

New feature or capability

  

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+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

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

No description provided.