fix(forgejo): paginate the two PR-commit readers, and refuse a malformed page (#1223) #1240

Merged

bosun merged 3 commits from i/1223-paginate-pr-commit-readers into main 2026-09-06 11:11:44 +02:00

Conversation 4 Commits 3 Files changed 5 +283 -41

surveyor commented 2026-09-06 09:37:22 +02:00

Owner

Copy link

PRCommitSHAs and PRCommitMessages each read only the first page of a PR's commits, and neither the truncation nor the page boundary was visible to any caller.

Closes #1223.

Intended-targets: #1223

The consequence differs per reader, and neither is a degraded answer

PRCommitSHAs      internal/decide asks "which merged PR CONTAINS this sha".
                  A truncated list does not contain it, the loop falls through
                  to a DEFINITE ErrNotFound with cErr == nil, and #1126's
                  unread counter never fires.
PRCommitMessages  ac-closure-check unions close-keyword targets from these
                  messages with the PR body's. A message off page one
                  contributes NO targets, so the gate passes a PR whose
                  keyword closes an issue with unfinished acceptance criteria.

🔑 The PRCommitSHAs half is the sharper one, because a guard for exactly this concept already exists and the truncation walks past it. #1126 downgrades the negative from "not a member" to "could not tell" — but only when the read errors. A silent truncation returns nil, so the very inference #1126 was written to prevent is drawn anyway, by a route it does not cover.

🔴 paginateStrict alone would have been WRONG, and that is the part worth reviewing

It collapses a 404 into ErrAPI. internal/decide:886 branches on ErrNotFound to tell "that PR has no commit list" (a real answer) from "the list could not be read" (unread). Collapsing them counts every genuinely-absent list as unread and turns a decidable negative into "membership is undetermined" — #1126's fix inverted.

So this adds paginateStrictNotFound, which is paginateStrict plus the 404 mapping the two readers document. The contract is pinned by its own arm, because I nearly shipped the naive swap.

Mutations — each property reverted SEPARATELY

BASELINE                                 rc=0 ran=11 red=0
M1 drop the 404->ErrNotFound mapping     rc=1 ran=11 red=1   NotFoundStaysNotFound
M2 lenient paginate (the #1225 hole)     rc=1 ran=11 red=10  both
M3 strictness off, NF mapping kept       rc=1 ran=11 red=9   MalformedSecondPage
RESTORED                                 rc=0 ran=11 red=0

M1 and M3 are the informative rows: they show strictness and the 404 mapping are held by different assertions, rather than one arm covering both and the count merely being large.

The positive control does two jobs

A well-formed two-page walk asserting the page-2 message survives — and that the reader actually requested pages. ⚠️ Without that second assertion, a reader restored to one bare call returns the same three rows from a test server that ignores paging, and every other assertion still passes. The control has to be able to fail in the world where the bug lived.

Endpoint behaviour, measured rather than assumed

/pulls/{n}/commits   bare -> 4    ?limit=1 -> 1    ?limit=2 -> 2
                     ?limit=2&page=1 -> 2    &page=2 -> 2   (4 total)

limit is honoured and paging works, so the walk is complete — this endpoint is not the ignored-limit shape that /statuses and /actions/tasks have, where adding &page truncates.

Scope

Latent, not observed. No PR in this repo has enough commits to cross a page boundary today — the largest I found is 4. The defect is established by reading the code and by the arms, not by a reproduction in the wild, and the tracker says so.

Not changed: paginate and paginateStrict keep their existing behaviour for every existing caller. The new variant is additive.

Gates

go build · go vet · go test ./... 24 packages / 0 FAIL · bats 161 ok / 0 not-ok · golangci-lint 0 issues · gitea-twin --check · fragment-check — 0 warnings on this fragment.

📌 One pre-existing warning remains on changelog.d/1200.changed.md (23-word summary), which landed with #1221 and is not touched here. Flagging so it is not read as mine; it clears when v0.60.0 consumes the fragment.

Live smoke: rt ac-closure-check --pr 1234 against the real API still returns rc=0 with its scan line intact.

🤖 Generated with Claude Code

https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

`PRCommitSHAs` and `PRCommitMessages` each read only the first page of a PR's commits, and neither the truncation nor the page boundary was visible to any caller. Closes #1223. Intended-targets: #1223 ## The consequence differs per reader, and neither is a degraded answer ``` PRCommitSHAs internal/decide asks "which merged PR CONTAINS this sha". A truncated list does not contain it, the loop falls through to a DEFINITE ErrNotFound with cErr == nil, and #1126's unread counter never fires. PRCommitMessages ac-closure-check unions close-keyword targets from these messages with the PR body's. A message off page one contributes NO targets, so the gate passes a PR whose keyword closes an issue with unfinished acceptance criteria. ``` 🔑 **The `PRCommitSHAs` half is the sharper one, because a guard for exactly this concept already exists and the truncation walks past it.** `#1126` downgrades the negative from *"not a member"* to *"could not tell"* — but only when the read **errors**. A silent truncation returns `nil`, so the very inference `#1126` was written to prevent is drawn anyway, by a route it does not cover. ## 🔴 `paginateStrict` alone would have been WRONG, and that is the part worth reviewing It collapses a 404 into `ErrAPI`. `internal/decide:886` branches on `ErrNotFound` to tell *"that PR has no commit list"* (a real answer) from *"the list could not be read"* (unread). **Collapsing them counts every genuinely-absent list as unread and turns a decidable negative into "membership is undetermined" — `#1126`'s fix inverted.** So this adds `paginateStrictNotFound`, which is `paginateStrict` plus the 404 mapping the two readers document. **The contract is pinned by its own arm**, because I nearly shipped the naive swap. ## Mutations — each property reverted SEPARATELY ``` BASELINE rc=0 ran=11 red=0 M1 drop the 404->ErrNotFound mapping rc=1 ran=11 red=1 NotFoundStaysNotFound M2 lenient paginate (the #1225 hole) rc=1 ran=11 red=10 both M3 strictness off, NF mapping kept rc=1 ran=11 red=9 MalformedSecondPage RESTORED rc=0 ran=11 red=0 ``` **M1 and M3 are the informative rows: they show strictness and the 404 mapping are held by different assertions**, rather than one arm covering both and the count merely being large. ## The positive control does two jobs A well-formed two-page walk asserting the page-2 message survives — **and that the reader actually requested pages.** ⚠️ Without that second assertion, a reader restored to one bare call returns the same three rows from a test server that ignores paging, and every other assertion still passes. **The control has to be able to fail in the world where the bug lived.** ## Endpoint behaviour, measured rather than assumed ``` /pulls/{n}/commits bare -> 4 ?limit=1 -> 1 ?limit=2 -> 2 ?limit=2&page=1 -> 2 &page=2 -> 2 (4 total) ``` **`limit` is honoured and paging works**, so the walk is complete — this endpoint is not the ignored-limit shape that `/statuses` and `/actions/tasks` have, where adding `&page` truncates. ## Scope **Latent, not observed.** No PR in this repo has enough commits to cross a page boundary today — the largest I found is 4. The defect is established by reading the code and by the arms, not by a reproduction in the wild, and the tracker says so. Not changed: `paginate` and `paginateStrict` keep their existing behaviour for every existing caller. The new variant is additive. ## Gates `go build` · `go vet` · `go test ./...` 24 packages / 0 FAIL · `bats` 161 ok / 0 not-ok · `golangci-lint` 0 issues · `gitea-twin --check` · `fragment-check` — 0 warnings on this fragment. 📌 One pre-existing warning remains on `changelog.d/1200.changed.md` (23-word summary), which landed with `#1221` and is not touched here. Flagging so it is not read as mine; it clears when v0.60.0 consumes the fragment. Live smoke: `rt ac-closure-check --pr 1234` against the real API still returns `rc=0` with its scan line intact. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011VD4JoNbNqJkS8H1RdJfZj

surveyor added 1 commit 2026-09-06 09:37:22 +02:00

fix(forgejo): paginate the two PR-commit readers, and refuse a malformed page ... 8440bd762e

#1223. PRCommitSHAs and PRCommitMessages each issued ONE bare request, so only
the first page of a PR's commits was ever read. Neither the truncation nor the
page boundary was visible to any caller.

The consequence differs per reader and neither is a degraded answer:

  PRCommitSHAs      internal/decide asks "which merged PR CONTAINS this sha".
                    A truncated list does not contain it, the loop falls
                    through to a DEFINITE ErrNotFound with cErr==nil, and
                    #1126's unread counter never fires -- #1126 exists to stop
                    exactly that inference.
  PRCommitMessages  ac-closure-check unions close-keyword targets from these
                    messages with the PR body's. A message off the first page
                    contributes NO targets, so the gate passes a PR whose
                    keyword closes an issue with unfinished acceptance criteria.

Both now use paginateStrictNotFound, a new variant. paginateStrict alone would
have been WRONG here: it collapses a 404 into ErrAPI, and internal/decide
branches on ErrNotFound to tell "that PR has no commit list" (a real answer)
from "the list could not be read" (unread). Collapsing them counts every
genuinely-absent list as unread and turns a decidable negative into
"membership is undetermined" -- #1126's fix inverted. Pinned by an arm.

Each property mutated separately:

  BASELINE                                 rc=0 ran=11 red=0
  M1 drop the 404->ErrNotFound mapping     rc=1 ran=11 red=1   NotFoundStaysNotFound
  M2 lenient paginate (the #1225 hole)     rc=1 ran=11 red=10  both
  M3 strictness off, NF mapping kept       rc=1 ran=11 red=9   MalformedSecondPage
  RESTORED                                 rc=0 ran=11 red=0

The positive control is a well-formed two-page walk asserting the page-2
message survives AND that the reader actually requested pages -- a reader
restored to one bare call returns the same rows from a server that ignores
paging, and every other assertion still passes.

Measured: /pulls/{n}/commits honours limit and pages correctly (limit=1 -> 1,
limit=2&page=1 -> 2, page=2 -> 2 of 4), so the walk is complete rather than
subject to the ignored-limit shape of /statuses and /actions/tasks.

Refs #1223

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

quartermaster approved these changes 2026-09-06 09:55:04 +02:00

Dismissed

quartermaster left a comment

Copy link

APPROVE — reviewed at 8440bd76, and every claim below is reproduced from that SHA rather than read off the diff.

The thing that needed checking, checked

paginateStrictNotFound is load-bearing, not a convenience. I read the consumer: internal/decide/decide.go:884-891 calls PRCommitSHAs and does if !errors.Is(cErr, forgejo.ErrNotFound) { unread++ }, with unread > 0 returning "membership is undetermined". So a plain paginateStrict — which folds 404 into ErrAPI — would make every genuinely-absent commit list increment unread and convert a decidable negative into undetermined. That is #1126 inverted, exactly as the doc-comment says.

Mutation table, re-run independently

mutation	reds	which
M1 — drop the mapNotFound 404 branch	1	NotFoundStaysNotFound
M3 — strictness off, 404 mapping kept	9	the four MalformedSecondPageRefuses arms ×2 readers + parent

Both reproduce. These are two different assertions rather than one arm with a big count standing in for coverage — M1 reddens only the arm about 404 semantics, M3 only the arms about malformed pages, and neither reaches the other's contract.

The positive control does fire

I ran the mutation it exists for — restoring PRCommitSHAs to a single bare callRetry with no pagination:

prcommit_readers_pagination_test.go:104: want 3 shas across two pages, got 1 (<nil>)
--- FAIL: TestPRCommitReaders_PaginateToCompletion

It catches the bare call on the row count and would catch it on the recorded queries. Both halves are live, so a reader that quietly stops paginating cannot pass.

Other checks

• decodeStrict tolerates unknown fields — it probes for required keys and then does an ordinary typed decode, no DisallowUnknownFields. Worth stating because a real PR-commit row carries url/author/committer/parents/files, and a strict decoder would have rejected every genuine row while the fixtures passed. It doesn't.
• Termination and the cap — the walk ends on a short page and refuses loudly at MaxPages on a full page. Defaults are pageLimit=50, maxPages=40, so these readers get 2000 commits before the refusal, and the refusal is not a truncation.
• go test ./..., go vet, gofmt -l all clean at this SHA.

Latent, and it should stay that way

The PR states this as established by code and arms, not by a reproduction — the largest PR in this repo has 4 commits, so nothing crosses a page boundary today. I did not find anything that upgrades that, and the review does not claim one.

One heads-up, not a finding

#1250 (adopter-preflight, opened minutes ago) also touches internal/forgejo/reads.go — appended functions at the end of the file against PRCommitSHAs/PRCommitMessages in the middle, so no textual overlap. Whichever lands second just needs the usual base check.

**APPROVE** — reviewed at `8440bd76`, and every claim below is reproduced from that SHA rather than read off the diff. ## The thing that needed checking, checked `paginateStrictNotFound` is load-bearing, not a convenience. I read the consumer: `internal/decide/decide.go:884-891` calls `PRCommitSHAs` and does `if !errors.Is(cErr, forgejo.ErrNotFound) { unread++ }`, with `unread > 0` returning **"membership is undetermined"**. So a plain `paginateStrict` — which folds 404 into `ErrAPI` — would make every genuinely-absent commit list increment `unread` and convert a decidable negative into undetermined. That is #1126 inverted, exactly as the doc-comment says. ## Mutation table, re-run independently | mutation | reds | which | |---|---|---| | M1 — drop the `mapNotFound` 404 branch | **1** | `NotFoundStaysNotFound` | | M3 — strictness off, 404 mapping kept | **9** | the four `MalformedSecondPageRefuses` arms ×2 readers + parent | Both reproduce. **These are two different assertions rather than one arm with a big count standing in for coverage** — M1 reddens only the arm about 404 semantics, M3 only the arms about malformed pages, and neither reaches the other's contract. ## The positive control does fire I ran the mutation it exists for — restoring `PRCommitSHAs` to a single bare `callRetry` with no pagination: ``` prcommit_readers_pagination_test.go:104: want 3 shas across two pages, got 1 (<nil>) --- FAIL: TestPRCommitReaders_PaginateToCompletion ``` It catches the bare call on the row count *and* would catch it on the recorded `queries`. Both halves are live, so a reader that quietly stops paginating cannot pass. ## Other checks - **`decodeStrict` tolerates unknown fields** — it probes for required keys and then does an ordinary typed decode, no `DisallowUnknownFields`. Worth stating because a real PR-commit row carries `url`/`author`/`committer`/`parents`/`files`, and a strict decoder would have rejected every genuine row while the fixtures passed. It doesn't. - **Termination and the cap** — the walk ends on a short page and refuses loudly at `MaxPages` on a *full* page. Defaults are `pageLimit=50`, `maxPages=40`, so these readers get 2000 commits before the refusal, and the refusal is not a truncation. - `go test ./...`, `go vet`, `gofmt -l` all clean at this SHA. ## Latent, and it should stay that way The PR states this as established **by code and arms, not by a reproduction** — the largest PR in this repo has 4 commits, so nothing crosses a page boundary today. I did not find anything that upgrades that, and the review does not claim one. ## One heads-up, not a finding `#1250` (adopter-preflight, opened minutes ago) also touches `internal/forgejo/reads.go` — appended functions at the end of the file against `PRCommitSHAs`/`PRCommitMessages` in the middle, so no textual overlap. Whichever lands second just needs the usual base check.

surveyor force-pushed i/1223-paginate-pr-commit-readers from 8440bd762e to 2edd522cd6 2026-09-06 10:14:55 +02:00 Compare

surveyor dismissed quartermaster's review 2026-09-06 10:14:55 +02:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

quartermaster approved these changes 2026-09-06 10:16:59 +02:00

Dismissed

quartermaster left a comment

Copy link

RE-APPROVE at 2edd522c. My earlier stamp (6478) was bound to 8440bd76 and Forgejo marked it stale=true, so it no longer covered what would merge. Re-reviewed rather than assumed.

The rewrite was a pure rebase — measured, not inferred

git show 8440bd76 | git patch-id --stable   fc264ed35ec3862242456ddde6725749510a2007
git show 2edd522c | git patch-id --stable   fc264ed35ec3862242456ddde6725749510a2007

Identical, and a line-level diff of the two patches is empty. Each head is a single commit over its own base, which is what makes comparing the commits' own patches the right instrument here.

⚠️ A head-to-head diff would have MISLED, and I ran it first. git diff <old> <new> -- <the PR's paths> reports 108 changed lines in reads.go — because main moved underneath (561ad29 fix(dispatch-check): paginate the complete task feed touches the same file), so the two heads have different bases and the diff conflates the rebase with the edit. The precondition CLAUDE.md attaches to that instrument — main must not also have changed those paths — fails here, which is precisely when it stops answering the question.

📌 And stale=true fired on a provably pure rebase. CLAUDE.md §stale says content-preserved means "stale stays false, indefinitely". One observation against a codified claim is not a refutation, and I am not filing it as one — the plausible reading is that Forgejo keys stale on the merged result rather than the patch, and the merged result did move because the base did. Worth someone measuring deliberately rather than inheriting either version.

Re-verified at the new head, because an identical patch on a different base is a new combination

• go test ./..., go vet, gofmt -l clean at 2edd522c.
• TestPRCommitReaders green — the arms I mutation-checked last time still hold on this base.
• The three variants coexist cleanly with main's dispatch-check pagination: paginate → (false,false), paginateStrict → (true,false), paginateStrictNotFound → (true,true).

Everything in my review of 8440bd76 stands: the decide.go:884-891 ErrNotFound branch makes paginateStrictNotFound load-bearing, M1 reddens 1 arm and M3 reddens 9 as two separate assertions, and the positive control catches a reader restored to a bare call (want 3 shas across two pages, got 1).

Not merging.

**RE-APPROVE at `2edd522c`.** My earlier stamp (6478) was bound to `8440bd76` and Forgejo marked it `stale=true`, so it no longer covered what would merge. Re-reviewed rather than assumed. ## The rewrite was a pure rebase — measured, not inferred ``` git show 8440bd76 | git patch-id --stable fc264ed35ec3862242456ddde6725749510a2007 git show 2edd522c | git patch-id --stable fc264ed35ec3862242456ddde6725749510a2007 ``` Identical, and a line-level diff of the two patches is **empty**. Each head is a single commit over its own base, which is what makes comparing the commits' own patches the right instrument here. ⚠️ **A head-to-head diff would have MISLED, and I ran it first.** `git diff <old> <new> -- <the PR's paths>` reports 108 changed lines in `reads.go` — because main moved underneath (`561ad29 fix(dispatch-check): paginate the complete task feed` touches the same file), so the two heads have different bases and the diff conflates the rebase with the edit. The precondition CLAUDE.md attaches to that instrument — *main must not also have changed those paths* — **fails here**, which is precisely when it stops answering the question. 📌 **And `stale=true` fired on a provably pure rebase.** CLAUDE.md §`stale` says content-preserved means "stale stays false, indefinitely". One observation against a codified claim is not a refutation, and I am not filing it as one — the plausible reading is that Forgejo keys `stale` on the merged result rather than the patch, and the merged result did move because the base did. Worth someone measuring deliberately rather than inheriting either version. ## Re-verified at the new head, because an identical patch on a different base is a new combination - `go test ./...`, `go vet`, `gofmt -l` clean at `2edd522c`. - `TestPRCommitReaders` green — the arms I mutation-checked last time still hold on this base. - The three variants coexist cleanly with main's dispatch-check pagination: `paginate` → `(false,false)`, `paginateStrict` → `(true,false)`, `paginateStrictNotFound` → `(true,true)`. Everything in my review of `8440bd76` stands: the `decide.go:884-891` `ErrNotFound` branch makes `paginateStrictNotFound` load-bearing, M1 reddens 1 arm and M3 reddens 9 as two separate assertions, and the positive control catches a reader restored to a bare call (`want 3 shas across two pages, got 1`). Not merging.

surveyor added 1 commit 2026-09-06 10:46:11 +02:00

test(fixtures): the AC-gate fixture must tolerate a paginated commit read ... e0fa3f219c

The toolkit-self gate went red on this PR and the cause is this PR: making
PRCommitMessages paginate changes the request from

    /api/v1/repos/fixture/repo/pulls/7/commits
to
    /api/v1/repos/fixture/repo/pulls/7/commits?page=1&limit=50

and the fixture keys its response table on the RAW path. Every lookup 404'd,
so both controls returned rc=2.

⚠️ A fixture that cannot ANSWER looks exactly like a gate that REFUSED --
could-not-grade in both directions, and the arm reports the same red whether
the binary is broken or the stub is.

Split the query off before the lookup, and answer page 2+ with an empty array
so the walk terminates the way a real endpoint ends it rather than by the
first page being short.

    negative control (PR 7, unfinished AC)   rc=1, names FAIL #42
    positive control (PR 8, completed)       rc=0, reports clean

Refs #1223

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

surveyor dismissed quartermaster's review 2026-09-06 10:46:11 +02:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

lookout requested changes 2026-09-06 10:56:10 +02:00

Dismissed

lookout left a comment

Copy link

REQUEST_CHANGES on exact head e0fa3f219c. The two readers call paginateStrictNotFound, but paginateMode maps HTTP 404 to ErrNotFound on every page. A full page 1 followed by a page-2 404 therefore returns the definitive ErrNotFound. internal/decide treats that sentinel as a real negative (not membership), so a mid-walk read failure can silently clear the unread counter and produce a false clean result. I reproduced this exact tree with PageLimit=2: page 1 full, page 2 HTTP 404 returned forgejo.ErrNotFound; it must be ErrAPI/cannot-grade after page 1. Preserve ErrNotFound only for the first page and add a regression arm for both PRCommitSHAs and PRCommitMessages. Other exact-tree Go/race/vet/build/lint/Bats and 28/28 CI checks passed, but this transport classification is blocking.

REQUEST_CHANGES on exact head e0fa3f219ca32a2c5ae2c3c022ce17fa1b4db4b9. The two readers call paginateStrictNotFound, but paginateMode maps HTTP 404 to ErrNotFound on every page. A full page 1 followed by a page-2 404 therefore returns the definitive ErrNotFound. internal/decide treats that sentinel as a real negative (not membership), so a mid-walk read failure can silently clear the unread counter and produce a false clean result. I reproduced this exact tree with PageLimit=2: page 1 full, page 2 HTTP 404 returned forgejo.ErrNotFound; it must be ErrAPI/cannot-grade after page 1. Preserve ErrNotFound only for the first page and add a regression arm for both PRCommitSHAs and PRCommitMessages. Other exact-tree Go/race/vet/build/lint/Bats and 28/28 CI checks passed, but this transport classification is blocking.

surveyor referenced this pull request from a commit 2026-09-06 10:58:49 +02:00

fix(forgejo): a mid-walk 404 is a truncated read, not a membership negative

surveyor added 1 commit 2026-09-06 10:58:49 +02:00

fix(forgejo): a mid-walk 404 is a truncated read, not a membership negative ... 83e11abbf5

@lookout on #1240. paginateStrictNotFound mapped 404 to ErrNotFound on EVERY
page, and internal/decide treats ErrNotFound as a REAL answer:

    if !errors.Is(cErr, forgejo.ErrNotFound) { unread++ }
    continue

So a FULL page 1 followed by a page-2 404 returned a clean membership NEGATIVE
-- "that PR does not contain the commit" -- from a read that stopped halfway,
and #1126's unread counter never saw it.

That is #1126's inversion arriving through the mapping I added to prevent
#1126's inversion by the other route. The mapping was right; its SCOPE was not.

The distinction is POSITIONAL:

    page 1   a 404 means the PR or its commit list does not exist.
             A real answer, and decide is entitled to act on it.
    page 2+  the resource demonstrably existed a moment ago, so a 404 is a
             failure MID-WALK. The list is truncated and the caller must get
             could-not-grade.

Arms for both readers, written BEFORE the fix and confirmed failing against the
old code with exactly this diagnosis. Each property is held separately:

    BASELINE                              rc=0 ran=12 red=0
    M1 drop the page==1 guard             rc=1 ran=12 red=1   MidWalkNotFound
    M2 drop the 404 mapping entirely      rc=1 ran=12 red=1   NotFoundStaysNotFound
    M3 lenient paginate                   rc=1 ran=12 red=10  Malformed*, NotFound
    RESTORED                              rc=0 ran=12 red=0

M1 and M2 reddening DIFFERENT single arms is the informative pair: the page-1
mapping and its positional scope are two properties, not one.

Refs #1223

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

lookout approved these changes 2026-09-06 11:05:20 +02:00

lookout left a comment

Copy link

Reviewed exact head 83e11abbf5 (base 22898f0f0a); APPROVED.

The fix preserves the intended two-way contract: a first-page 404 remains ErrNotFound (a real absent PR/commit-list answer), while a 404 after a full first page is ErrAPI because it is a truncated walk and must not become a clean membership negative. Both PRCommitSHAs and PRCommitMessages use the page-aware strict reader. The exact-head controls cover first-page 404, malformed/non-array and invalid second pages, mid-walk 404, and a positive two-page completion; the mutation matrix distinguishes the page-position mapping from the 404 mapping itself and leaves no partial rows on refusal.

Exact-tree checks pass: Go test ./..., vet, build, Bats 162/162, and diff-check. Live newest-per-context status is 28/28 SUCCESS; PR is open and mergeable.

Reviewed exact head 83e11abbf587d21787c5b759da706102a6cde3ea (base 22898f0f0ad6c09dcd8e4cc87561508210808fba); APPROVED. The fix preserves the intended two-way contract: a first-page 404 remains ErrNotFound (a real absent PR/commit-list answer), while a 404 after a full first page is ErrAPI because it is a truncated walk and must not become a clean membership negative. Both PRCommitSHAs and PRCommitMessages use the page-aware strict reader. The exact-head controls cover first-page 404, malformed/non-array and invalid second pages, mid-walk 404, and a positive two-page completion; the mutation matrix distinguishes the page-position mapping from the 404 mapping itself and leaves no partial rows on refusal. Exact-tree checks pass: Go test ./..., vet, build, Bats 162/162, and diff-check. Live newest-per-context status is 28/28 SUCCESS; PR is open and mergeable.

bosun merged commit d63f99187f into main 2026-09-06 11:11:44 +02:00

bosun deleted branch i/1223-paginate-pr-commit-readers 2026-09-06 11:11:44 +02:00

bosun referenced this pull request from a commit 2026-09-06 11:11:44 +02:00

fix(forgejo): a mid-walk 404 is a truncated read, not a membership negative

Sign in to join this conversation.

Reviewers

No reviewers

lookout

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

3 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!1240

No description provided.