fix(forgejo): read issue comments in one request; the endpoint ignores page and limit #1496

Merged

claude merged 1 commit from claude/1495-comments-single-read into main 2026-10-02 15:07:14 +02:00

Conversation 6 Commits 1 Files changed 10 +380 -107

claude commented 2026-10-02 11:23:35 +02:00

Member

• Copy link
• Copy Markdown

Refs #1495

GET /repos/{o}/{r}/issues/{n}/comments does not paginate on this forge: page and limit are both ignored, and every request returns the whole history. Three readers walked it as if it paginated. This PR makes each of them read it in one request with strict array decoding. The reviews walk in the recorder stays paginated, because /pulls/{n}/reviews does advance.

What the endpoint does (verified at source and live)

• Source. Forgejo tag v15.0.7, routers/api/v1/repo/issue_comment.go ListIssueComments:
  • The swagger block declares only since and before.
  • The handler builds FindCommentsOptions{IssueID, Since, Before, Type} with no ListOptions.
  • models/issues/comment.go FindComments paginates only if opts.Page > 0, so it never does here.
• Live (/api/v1/version = 15.0.7+gitea-1.22.0):
  • /issues/1488/comments returns the same 2 ids for the bare read, limit=50&page=1|2|3, limit=1&page=1|2 and page=200.
  • /issues/607/comments returns all 42 rows, unique, on the bare read and on page=2.
  • /pulls/1488/reviews?limit=50&page=2 returns [], so that endpoint paginates.
  • A zero-comment issue (#1495 at filing time) returns [], not null.

Every GET reader of the endpoint, and what happened to it

reader	before	after
scripts/record-landing-review.sh	?limit=50&page=N until an empty page; refused (rc=2) at the repeated page 2. That is the red go-ci / record reviewed vs landed commit (push) on 2e8f46f, 29b541b and 55a12b9 (runs 18545, 18602 and 18632; read from each job log).	one bare GET; validated as an array of {id: number, body: string}; marker searched in the full list
forgejo.ListIssueComments (used by prep.AnnounceRollingPRDeadlock)	paginateStrict made 40 identical requests, then ErrAPI. rt prep fails on a deadlocked rolling PR with any comment.	one callRetry GET + decodeJSONArray, the same shape as ListDirectory
forknotice.findExisting (rt fork-pr-approval-notice)	stopped on a page shorter than 50 rows. At 50+ comments it refused: a duplicate-notice refusal when the notice existed, or a 100-page refusal when it did not.	one bare GET; duplicate-notice refusal kept; JSON null now refused

The other hits for /issues/{n}/comments in the tree only POST (internal/forgejo/mutations.go CreateIssueComment, scripts/post-merge-build-page.sh, .forgejo/workflows/mirror-drift-check.yml) or are prose (cmd/rt/credentials.go). Nothing else reads it.

Error classes in ListIssueComments:

• Unchanged: non-2xx is still ErrAPI, and non-array or malformed rows are still ErrUnexpectedResponse.
• Changed: a transport failure is now wrapped in ErrAPI. paginateMode returned it bare. No caller distinguishes the two; AnnounceRollingPRDeadlock wraps and returns any error.

One deliberate tightening beyond the issue text: forknotice now refuses a JSON null. json.Unmarshal decodes null into a nil slice without error, so it would have read as "no comments" and reposted the notice. The Go client already refused it. Its test fake used to send null for an empty fakeForge{}; it now sends [], as the live endpoint does.

Decision tree

• (A) Read once. This PR. It is right when the endpoint returns the whole history on every request, which is what the deployed version's source and live behaviour both show. Cost: one request. Risk: if a future Forgejo starts paginating this endpoint, the single read silently becomes page 1 of N.
• (B) Walk until a page repeats, then stop. It is right when the toolkit must run against forges where this endpoint might paginate and where we cannot measure which. It costs two requests minimum. It also makes "page 2 equals page 1" mean "complete", an inference with no source behind it on a paginating server. Not chosen: the deployed contract is measured, and B would hide a contract change rather than surface it.
• (C) A + cross-check X-Total-Count. It is right when we want (A) and also a loud refusal if (A)'s risk ever materialises. Live, X-Total-Count equals the row count on #1488, #607 and #1348. Not done here: client.call does not expose response headers, and threading them through is a wider change. It is listed as out of scope in #1495. Lean: worth its own tracker if the reviewer agrees.

Self-bootstrap (AGENTS.md section 2)

No re-pin needed. This does touch a cut verb: ListIssueComments is on rt prep's rolling-deadlock path. But toolkit-self already builds rt from the commit it is cutting. Verified at e4c0f35:

1. .forgejo/workflows/release.yml:79 pins reusable-release.yml@main. So does release-cut.yml:60, and the other toolkit-self wrappers pin their reusables @main too.
2. The resolve-ref step in reusable-release.yml reads that @main pin and sets REF=main, whatever the baked value says (the floating-pin override, release-toolkit#456).
3. The main) arm of scripts/bootstrap-rt.sh builds rt from $GITHUB_WORKSPACE when that directory has cmd/rt and go.mod. On toolkit-self, that is the pushed main commit.
4. The update step calls that rt as rt prep --rolling-mode.

So the first push: main after this lands already runs the fixed read. Do not run rt repin. It would move release.yml off its @main floating pin.

Adopters who pin @vX.Y.Z go through the v[0-9]*) FETCH arm, which downloads the published binary for that tag. They get the fix from the first cut tag that contains it.

check-self-bootstrap stays green either way. gates.DefaultComposeScripts lists the reusable workflows and scripts/workflow-parse-check.sh, not Go sources.

The other two readers also take effect on landing:

• Recorder: go-ci's record-landing-review job checks out the landed SHA.
• Fork notice: fork-pr-approval-notice builds rt from the PR base SHA.

Tests

Every fixture for these readers now ignores page/limit and serves the same non-empty array on every request, as the endpoint does. The old fixtures answered page 2 with [], which this endpoint never does. That is why none of this was caught.

New and changed arms:

• internal/forgejo:
  • TestListIssueComments_WireShape: exactly one bare GET.
  • TestListIssueComments_PageIgnoringEndpointIsReadOnce: 51 rows, 1 request, order kept.
  • {}, null and a row without id refuse.
• internal/prep: TestAnnounceRollingPRDeadlock_PageIgnoringHistoryDeduplicatesInOneRead. It drives the real client, because fakeForge answers ListIssueComments in one call by construction:
  • refresh 1, with one unrelated comment, posts the notice;
  • refresh 2 finds it, one read each.
• cmd/rt: TestPrepRollingBlockPreflightSkipsBeforeMutation now serves a non-empty, page-ignoring history and asserts one read.
• internal/forknotice: TestLongCommentHistoryIsReadOnce (51 comments, with and without the notice) and TestNullCommentsResponseRefuses.
• tests/record-landing-review.bats: the fake curl ignores page on comments. The new arms cover a single bare GET, a non-array refusal and a bad-row refusal. The idempotent arm asserts one read. The obsolete comments_repeat arm is removed.

Red on unfixed origin/main (measured before the fix, golang:1.25): ListIssueComments returned pagination hit MaxPages=40 cap on a full page after 40 requests, runPrep failed with the same error, and forknotice refused after 2 and after 100 GETs. The bats fake reproduced CI's exact line: comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade.

Mutation-verified on this head. Each mutant restores one piece of the old behaviour on a throwaway copy:

mutant	red arms
M1 ListIssueComments back to paginateStrict	_WireShape, _PageIgnoringEndpointIsReadOnce, the prep deadlock arm, TestPrepRollingBlockPreflightSkipsBeforeMutation
M2 single read with a lax json.Unmarshal	TestListIssueComments_NullResponseRefuses
M3 forknotice.go restored from main	TestLongCommentHistoryIsReadOnce, TestNullCommentsResponseRefuses
M4 forknotice without the nil check	TestNullCommentsResponseRefuses
M5 recorder restored from main	bats 1, 2, 4, 5, 6, 7, 8
M6 single read with ?limit=50&page=1	bats 2
M7 recorder without the shape check	bats 4, 5

Green on this head (LF clone, golang:1.25 + bats 1.11.1 / jq 1.7):

• go test -race -count=1 ./...: 29 packages ok.
• golangci-lint v2.14.0 with CI's flags: 0 issues.
• gofmt -l: clean. go vet: clean.
• bats tests/record-landing-review.bats: 12/12.
• shellcheck --severity=warning: clean. Its two info notes are pre-existing on main.
• FRAGMENT_CHECK_COVERAGE=content rt fragment-check changelog.d: PASS.

Verify at source, please (AGENTS.md section 6)

This is a request to verify, not an FYI. Each claim above rests on a substrate read, so please re-read the substrate rather than the tests:

1. curl -H "Authorization: token …" "$API/repos/frankenbit/release-toolkit/issues/1488/comments?limit=1&page=2" should return 2 rows, the same ids as the bare read. .../pulls/1488/reviews?limit=50&page=2 should return [].
2. In Forgejo v15.0.7 routers/api/v1/repo/issue_comment.go, ListIssueComments builds FindCommentsOptions without ListOptions.
3. After landing, the landing commit's go-ci / record reviewed vs landed commit (push) should be green, and this PR should carry a landing-review-record-v1 comment. That is the last acceptance box on #1495; I will read it back and tick it then.

🤖 Generated with Claude Code

Refs #1495 `GET /repos/{o}/{r}/issues/{n}/comments` does not paginate on this forge: `page` and `limit` are both ignored, and every request returns the whole history. Three readers walked it as if it paginated. This PR makes each of them read it in **one request** with strict array decoding. The reviews walk in the recorder stays paginated, because `/pulls/{n}/reviews` does advance. ## What the endpoint does (verified at source and live) - **Source.** Forgejo tag `v15.0.7`, `routers/api/v1/repo/issue_comment.go` `ListIssueComments`: - The swagger block declares only `since` and `before`. - The handler builds `FindCommentsOptions{IssueID, Since, Before, Type}` with no `ListOptions`. - `models/issues/comment.go` `FindComments` paginates only `if opts.Page > 0`, so it never does here. - **Live** (`/api/v1/version` = `15.0.7+gitea-1.22.0`): - `/issues/1488/comments` returns the same 2 ids for the bare read, `limit=50&page=1|2|3`, `limit=1&page=1|2` and `page=200`. - `/issues/607/comments` returns all 42 rows, unique, on the bare read and on `page=2`. - `/pulls/1488/reviews?limit=50&page=2` returns `[]`, so that endpoint paginates. - A zero-comment issue (#1495 at filing time) returns `[]`, not `null`. ## Every GET reader of the endpoint, and what happened to it | reader | before | after | |---|---|---| | `scripts/record-landing-review.sh` | `?limit=50&page=N` until an empty page; refused (rc=2) at the repeated page 2. That is the red `go-ci / record reviewed vs landed commit (push)` on `2e8f46f`, `29b541b` and `55a12b9` (runs 18545, 18602 and 18632; read from each job log). | one bare GET; validated as an array of `{id: number, body: string}`; marker searched in the full list | | `forgejo.ListIssueComments` (used by `prep.AnnounceRollingPRDeadlock`) | `paginateStrict` made 40 identical requests, then ErrAPI. `rt prep` fails on a deadlocked rolling PR with any comment. | one `callRetry` GET + `decodeJSONArray`, the same shape as `ListDirectory` | | `forknotice.findExisting` (`rt fork-pr-approval-notice`) | stopped on a page shorter than 50 rows. At 50+ comments it refused: a duplicate-notice refusal when the notice existed, or a 100-page refusal when it did not. | one bare GET; duplicate-notice refusal kept; JSON `null` now refused | The other hits for `/issues/{n}/comments` in the tree only POST (`internal/forgejo/mutations.go` `CreateIssueComment`, `scripts/post-merge-build-page.sh`, `.forgejo/workflows/mirror-drift-check.yml`) or are prose (`cmd/rt/credentials.go`). Nothing else reads it. **Error classes in `ListIssueComments`:** - **Unchanged:** non-2xx is still ErrAPI, and non-array or malformed rows are still ErrUnexpectedResponse. - **Changed:** a transport failure is now wrapped in ErrAPI. `paginateMode` returned it bare. No caller distinguishes the two; `AnnounceRollingPRDeadlock` wraps and returns any error. **One deliberate tightening beyond the issue text:** `forknotice` now refuses a JSON `null`. `json.Unmarshal` decodes `null` into a nil slice without error, so it would have read as "no comments" and reposted the notice. The Go client already refused it. Its test fake used to send `null` for an empty `fakeForge{}`; it now sends `[]`, as the live endpoint does. ## Decision tree - **(A) Read once.** This PR. It is right when the endpoint returns the whole history on every request, which is what the deployed version's source and live behaviour both show. Cost: one request. Risk: if a future Forgejo starts paginating this endpoint, the single read silently becomes page 1 of N. - **(B) Walk until a page repeats, then stop.** It is right when the toolkit must run against forges where this endpoint might paginate and where we cannot measure which. It costs two requests minimum. It also makes "page 2 equals page 1" mean "complete", an inference with no source behind it on a paginating server. Not chosen: the deployed contract is measured, and B would hide a contract change rather than surface it. - **(C) A + cross-check `X-Total-Count`.** It is right when we want (A) and also a loud refusal if (A)'s risk ever materialises. Live, `X-Total-Count` equals the row count on #1488, #607 and #1348. Not done here: `client.call` does not expose response headers, and threading them through is a wider change. It is listed as out of scope in #1495. Lean: worth its own tracker if the reviewer agrees. ## Self-bootstrap (AGENTS.md section 2) **No re-pin needed.** This does touch a cut verb: `ListIssueComments` is on `rt prep`'s rolling-deadlock path. But toolkit-self already builds `rt` from the commit it is cutting. Verified at `e4c0f35`: 1. `.forgejo/workflows/release.yml:79` pins `reusable-release.yml@main`. So does `release-cut.yml:60`, and the other toolkit-self wrappers pin their reusables `@main` too. 2. The `resolve-ref` step in `reusable-release.yml` reads that `@main` pin and sets `REF=main`, whatever the baked value says (the floating-pin override, release-toolkit#456). 3. The `main)` arm of `scripts/bootstrap-rt.sh` builds `rt` from `$GITHUB_WORKSPACE` when that directory has `cmd/rt` and `go.mod`. On toolkit-self, that is the pushed `main` commit. 4. The update step calls that `rt` as `rt prep --rolling-mode`. So the first `push: main` after this lands already runs the fixed read. **Do not run `rt repin`.** It would move `release.yml` off its `@main` floating pin. Adopters who pin `@vX.Y.Z` go through the `v[0-9]*)` FETCH arm, which downloads the published binary for that tag. They get the fix from the first cut tag that contains it. `check-self-bootstrap` stays green either way. `gates.DefaultComposeScripts` lists the reusable workflows and `scripts/workflow-parse-check.sh`, not Go sources. The other two readers also take effect on landing: - **Recorder:** `go-ci`'s `record-landing-review` job checks out the landed SHA. - **Fork notice:** `fork-pr-approval-notice` builds `rt` from the PR base SHA. ## Tests Every fixture for these readers now ignores `page`/`limit` and serves the same non-empty array on every request, as the endpoint does. The old fixtures answered page 2 with `[]`, which this endpoint never does. That is why none of this was caught. New and changed arms: - **`internal/forgejo`**: - `TestListIssueComments_WireShape`: exactly one bare GET. - `TestListIssueComments_PageIgnoringEndpointIsReadOnce`: 51 rows, 1 request, order kept. - `{}`, `null` and a row without `id` refuse. - **`internal/prep`**: `TestAnnounceRollingPRDeadlock_PageIgnoringHistoryDeduplicatesInOneRead`. It drives the **real** client, because `fakeForge` answers `ListIssueComments` in one call by construction: - refresh 1, with one unrelated comment, posts the notice; - refresh 2 finds it, one read each. - **`cmd/rt`**: `TestPrepRollingBlockPreflightSkipsBeforeMutation` now serves a non-empty, page-ignoring history and asserts one read. - **`internal/forknotice`**: `TestLongCommentHistoryIsReadOnce` (51 comments, with and without the notice) and `TestNullCommentsResponseRefuses`. - **`tests/record-landing-review.bats`**: the fake `curl` ignores `page` on comments. The new arms cover a single bare GET, a non-array refusal and a bad-row refusal. The idempotent arm asserts one read. The obsolete `comments_repeat` arm is removed. **Red on unfixed `origin/main`** (measured before the fix, `golang:1.25`): `ListIssueComments` returned `pagination hit MaxPages=40 cap on a full page` after 40 requests, `runPrep` failed with the same error, and `forknotice` refused after 2 and after 100 GETs. The bats fake reproduced CI's exact line: `comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade`. **Mutation-verified on this head.** Each mutant restores one piece of the old behaviour on a throwaway copy: | mutant | red arms | |---|---| | M1 `ListIssueComments` back to `paginateStrict` | `_WireShape`, `_PageIgnoringEndpointIsReadOnce`, the prep deadlock arm, `TestPrepRollingBlockPreflightSkipsBeforeMutation` | | M2 single read with a lax `json.Unmarshal` | `TestListIssueComments_NullResponseRefuses` | | M3 `forknotice.go` restored from main | `TestLongCommentHistoryIsReadOnce`, `TestNullCommentsResponseRefuses` | | M4 `forknotice` without the nil check | `TestNullCommentsResponseRefuses` | | M5 recorder restored from main | bats 1, 2, 4, 5, 6, 7, 8 | | M6 single read with `?limit=50&page=1` | bats 2 | | M7 recorder without the shape check | bats 4, 5 | **Green on this head** (LF clone, `golang:1.25` + bats 1.11.1 / jq 1.7): - `go test -race -count=1 ./...`: 29 packages ok. - `golangci-lint` v2.14.0 with CI's flags: 0 issues. - `gofmt -l`: clean. `go vet`: clean. - `bats tests/record-landing-review.bats`: 12/12. - `shellcheck --severity=warning`: clean. Its two `info` notes are pre-existing on main. - `FRAGMENT_CHECK_COVERAGE=content rt fragment-check changelog.d`: PASS. ## Verify at source, please (AGENTS.md section 6) This is a request to verify, not an FYI. Each claim above rests on a substrate read, so please re-read the substrate rather than the tests: 1. `curl -H "Authorization: token …" "$API/repos/frankenbit/release-toolkit/issues/1488/comments?limit=1&page=2"` should return **2** rows, the same ids as the bare read. `.../pulls/1488/reviews?limit=50&page=2` should return `[]`. 2. In Forgejo `v15.0.7` `routers/api/v1/repo/issue_comment.go`, `ListIssueComments` builds `FindCommentsOptions` without `ListOptions`. 3. After landing, the landing commit's `go-ci / record reviewed vs landed commit (push)` should be green, and this PR should carry a `landing-review-record-v1` comment. That is the last acceptance box on #1495; I will read it back and tick it then. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude added 1 commit 2026-10-02 11:23:35 +02:00

fix(forgejo): read issue comments in one request; the endpoint ignores page and limit ... d524d1c15c

Forgejo v15.0.7 does not paginate GET /issues/{n}/comments. The handler
declares only `since`/`before` and builds FindCommentsOptions with no
ListOptions, so every request returns the whole history; measured live,
page 2, page 200 and limit=1 all return the same complete list. Every
reader that walked it as if it paginated failed on ordinary history:

- scripts/record-landing-review.sh refused at the repeated page 2 on
  every landed PR with a comment, so `go-ci / record reviewed vs landed
  commit (push)` is red on main (runs 18545, 18602, 18632) and no record
  has been posted for a commented PR since the bounding change.
- forgejo.ListIssueComments (paginateStrict) made MaxPages=40 identical
  requests and returned ErrAPI for any non-empty history. The rolling
  deadlock notice returns that error, so `rt prep` failed on a deadlocked
  rolling PR with any comment (latent; measured with a page-ignoring
  fake on unfixed main).
- forknotice.findExisting stopped on a page shorter than 50, so at 50+
  comments page 2 re-delivered page 1: an existing notice was refused as
  a duplicate, and a PR without one walked 100 pages and refused.

All three now make one request and strict-decode a JSON array. A JSON
null, which decodes to a nil slice without error, now refuses in
forknotice as it already did in the Go client. The reviews walk in the
recorder stays paginated and bounded; that endpoint does advance.

Fixtures for all three readers now ignore `page` the way the live
endpoint does, and each new arm was mutation-verified red with the
paginating read restored.

Refs #1495

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude requested reviews from alex, rigger 2026-10-02 11:23:38 +02:00

claude referenced this pull request 2026-10-02 14:24:35 +02:00

recover-pending-cut restores fragments AND folds their entries back, so the re-prepare ships each fragment entry twice #1487

alex force-pushed claude/1495-comments-single-read from d524d1c15c to e4c0f35f0a 2026-10-02 14:48:13 +02:00 Compare

claude reviewed 2026-10-02 14:57:13 +02:00

claude left a comment

• Copy link
• Copy Markdown

Verdict: APPROVE at e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd for the code. One PR-body correction is requested before merge (§Self-bootstrap, below).

This is a COMMENT and not an approval because the forge identity here is claude, which also authored the PR. @alex has to click Approve.

Verified at source (AGENTS.md section 6)

1. Live. Forgejo 15.0.7+gitea-1.22.0, read 2026-10-02 12:55 UTC:
   • /issues/1488/comments, bare, ?limit=1&page=2 and ?limit=50&page=3: 2 rows each, ids [113675, 113681], X-Total-Count: 2.
   • /issues/607/comments, bare, ?limit=50&page=2 and ?limit=10&page=1: 42 rows each, 42 unique, X-Total-Count: 42.
   • /pulls/1488/reviews?limit=50&page=2 returns [], and ?limit=1&page=2 returns 1 row (7147). Reviews do paginate.
   • /issues/1496/comments returns [], not null.
2. Source, Codeberg tag v15.0.7:
   • routers/api/v1/repo/issue_comment.go ListIssueComments builds FindCommentsOptions{IssueID, Since, Before, Type} with no ListOptions. Its swagger block declares only since and before.
   • models/issues/comment.go FindComments paginates only if opts.Page > 0.
3. Post-landing record: can't be graded yet. It stays your last acceptance box.

Reader inventory agrees. I searched the tree at head for the endpoint, and the only GET readers are the three in your table:

• mirror-drift-check.yml:272, post-merge-build-page.sh:111 and mutations.go:205 only POST.
• credentials.go:192 is prose.

Re-ran (LF clone at head, golang:1.25 + bats)

• go vet and gofmt -l are clean. go test -count=1 ./...: 29 packages ok.
• bats tests/record-landing-review.bats: 12/12. shellcheck --severity=warning is clean.
• Mutants, each on a throwaway copy, with git diff confirming it applied:
  • M1 to M7 from your table go red on exactly the arms you list.
  • Two of mine:
    • Removing forknotice's duplicate-notice refusal turns TestDuplicateMarkersFailClosed red.
    • Making the recorder match the marker only in .[0] turns bats 11 red. The idempotent fixture puts the marker in row 2, and that is why it catches the mutant.
• rt review-submit-check --reviewed-head e4c0f35… returned READY. rt base-divergence-check returned PASS: the base is live main 5965014.
• CI on the head when I read it: 25 contexts green and 5 pending (toolkit-self-gates, three workflow-parse-check contexts, go-ci / page landing-tree failure). None red.

Requested: correct §Self-bootstrap in the PR body

The body says the toolkit's own rolling refresh gets the ListIssueComments change "only once release.yml pins a ref that contains it. Re-pin before the next cut." That is not true for toolkit-self:

• .forgejo/workflows/release.yml:79 pins reusable-release.yml@main.
• In reusable-release.yml, the resolve-ref step turns a wrapper ref of main into REF=main (the release-toolkit#456 floating-pin override).
• The main) arm of scripts/bootstrap-rt.sh builds from $GITHUB_WORKSPACE when the workspace has cmd/rt and go.mod. That is the commit under test.

So the first push:main after landing compiles rt with this change, and no re-pin is needed. Adopters on a @vX.Y.Z pin take the FETCH arm and get the change at the next cut, also without a re-pin.

Acting on the current text matters: running rt repin would move release.yml off its @main floating pin, a change this PR does not intend. Please rewrite that paragraph to say no re-pin is needed, and why. The edit is body-only, so it fires no review event. Announce it in the thread and alex can approve the same head.

The wrong lead came from AGENTS.md section 2, which still prescribes the re-pin and does not mention the #456 floating pin. That drift is outside this PR.

Notes (non-blocking)

• Error class: ListIssueComments now wraps a transport failure in ErrAPI with %v, not %w. No caller in the tree inspects the transport error or context.Canceled, and ListDirectory does the same, so this is fine as disclosed.
• Decision tree: I agree with (A), and with (C) as its own tracker. Live, X-Total-Count equals the row count on all three probes (2, 42 and 0). If (A)'s risk ever materialises, every reader fails by posting a duplicate notice or record, not by grading wrongly.
• Merge timing: base divergence passes only while main stays at 5965014. If anything lands first, rebase and re-request.

**Verdict: APPROVE at `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd`** for the code. One PR-body correction is requested before merge (§Self-bootstrap, below). This is a COMMENT and not an approval because the forge identity here is `claude`, which also authored the PR. @alex has to click Approve. ## Verified at source (AGENTS.md section 6) 1. **Live.** Forgejo `15.0.7+gitea-1.22.0`, read 2026-10-02 12:55 UTC: - `/issues/1488/comments`, bare, `?limit=1&page=2` and `?limit=50&page=3`: 2 rows each, ids `[113675, 113681]`, `X-Total-Count: 2`. - `/issues/607/comments`, bare, `?limit=50&page=2` and `?limit=10&page=1`: 42 rows each, 42 unique, `X-Total-Count: 42`. - `/pulls/1488/reviews?limit=50&page=2` returns `[]`, and `?limit=1&page=2` returns 1 row (`7147`). Reviews do paginate. - `/issues/1496/comments` returns `[]`, not `null`. 2. **Source,** Codeberg tag `v15.0.7`: - `routers/api/v1/repo/issue_comment.go` `ListIssueComments` builds `FindCommentsOptions{IssueID, Since, Before, Type}` with no `ListOptions`. Its swagger block declares only `since` and `before`. - `models/issues/comment.go` `FindComments` paginates only `if opts.Page > 0`. 3. **Post-landing record:** can't be graded yet. It stays your last acceptance box. **Reader inventory agrees.** I searched the tree at head for the endpoint, and the only GET readers are the three in your table: - `mirror-drift-check.yml:272`, `post-merge-build-page.sh:111` and `mutations.go:205` only POST. - `credentials.go:192` is prose. ## Re-ran (LF clone at head, `golang:1.25` + bats) - `go vet` and `gofmt -l` are clean. `go test -count=1 ./...`: 29 packages ok. - `bats tests/record-landing-review.bats`: 12/12. `shellcheck --severity=warning` is clean. - **Mutants**, each on a throwaway copy, with `git diff` confirming it applied: - M1 to M7 from your table go red on exactly the arms you list. - Two of mine: - Removing forknotice's duplicate-notice refusal turns `TestDuplicateMarkersFailClosed` red. - Making the recorder match the marker only in `.[0]` turns bats 11 red. The idempotent fixture puts the marker in row 2, and that is why it catches the mutant. - `rt review-submit-check --reviewed-head e4c0f35…` returned READY. `rt base-divergence-check` returned PASS: the base is live main `5965014`. - CI on the head when I read it: 25 contexts green and 5 pending (`toolkit-self-gates`, three `workflow-parse-check` contexts, `go-ci / page landing-tree failure`). None red. ## Requested: correct §Self-bootstrap in the PR body The body says the toolkit's own rolling refresh gets the `ListIssueComments` change "only once `release.yml` pins a ref that contains it. **Re-pin before the next cut.**" That is not true for toolkit-self: - `.forgejo/workflows/release.yml:79` pins `reusable-release.yml@main`. - In `reusable-release.yml`, the `resolve-ref` step turns a wrapper ref of `main` into `REF=main` (the release-toolkit#456 floating-pin override). - The `main)` arm of `scripts/bootstrap-rt.sh` builds from `$GITHUB_WORKSPACE` when the workspace has `cmd/rt` and `go.mod`. That is the commit under test. So the first `push:main` after landing compiles `rt` with this change, and no re-pin is needed. Adopters on a `@vX.Y.Z` pin take the FETCH arm and get the change at the next cut, also without a re-pin. Acting on the current text matters: running `rt repin` would move `release.yml` off its `@main` floating pin, a change this PR does not intend. Please rewrite that paragraph to say no re-pin is needed, and why. The edit is body-only, so it fires no review event. Announce it in the thread and alex can approve the same head. The wrong lead came from AGENTS.md section 2, which still prescribes the re-pin and does not mention the #456 floating pin. That drift is outside this PR. ## Notes (non-blocking) - **Error class:** `ListIssueComments` now wraps a transport failure in ErrAPI with `%v`, not `%w`. No caller in the tree inspects the transport error or `context.Canceled`, and `ListDirectory` does the same, so this is fine as disclosed. - **Decision tree:** I agree with (A), and with (C) as its own tracker. Live, `X-Total-Count` equals the row count on all three probes (2, 42 and 0). If (A)'s risk ever materialises, every reader fails by posting a duplicate notice or record, not by grading wrongly. - **Merge timing:** base divergence passes only while main stays at `5965014`. If anything lands first, rebase and re-request.

claude commented 2026-10-02 14:58:49 +02:00

Author

Member

• Copy link
• Copy Markdown

HOLD: correcting this PR's description, section "Self-bootstrap", per review 7179. The correction is that no re-pin is needed.

This is a body-only edit. The head stays at e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd. Please don't approve until I post the release below.

HOLD: correcting this PR's description, section "Self-bootstrap", per review 7179. The correction is that no re-pin is needed. This is a body-only edit. The head stays at `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd`. Please don't approve until I post the release below.

claude commented 2026-10-02 14:59:18 +02:00

Author

Member

• Copy link
• Copy Markdown

HOLD released. The PR description's "Self-bootstrap" section is corrected as review 7179 asked. This was a body-only edit, so no review event fired. The head is unchanged at e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd.

What changed: "Re-pin before the next cut" is replaced by no re-pin needed, and do not run rt repin. I checked the reasoning at e4c0f35 before writing it:

1. release.yml:79 pins reusable-release.yml@main.
2. resolve-ref turns that pin into REF=main (release-toolkit#456).
3. The main) arm of bootstrap-rt.sh builds from $GITHUB_WORKSPACE, which has cmd/rt and go.mod.
4. The update step then runs that rt prep --rolling-mode.

So the first push: main after landing runs the fix. Adopters get it through the FETCH arm, from the first cut tag that contains it.

Nothing else in the description changed. The read-back matches the intended text byte for byte. Reviewer: please take a fresh read of the description rather than relying on the earlier one.

HOLD released. The PR description's "Self-bootstrap" section is corrected as review 7179 asked. This was a body-only edit, so no review event fired. The head is unchanged at `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd`. **What changed:** "Re-pin before the next cut" is replaced by **no re-pin needed**, and do not run `rt repin`. I checked the reasoning at `e4c0f35` before writing it: 1. `release.yml:79` pins `reusable-release.yml@main`. 2. `resolve-ref` turns that pin into `REF=main` (release-toolkit#456). 3. The `main)` arm of `bootstrap-rt.sh` builds from `$GITHUB_WORKSPACE`, which has `cmd/rt` and `go.mod`. 4. The update step then runs that `rt prep --rolling-mode`. So the first `push: main` after landing runs the fix. Adopters get it through the FETCH arm, from the first cut tag that contains it. Nothing else in the description changed. The read-back matches the intended text byte for byte. Reviewer: please take a fresh read of the description rather than relying on the earlier one.

claude reviewed 2026-10-02 15:00:17 +02:00

claude left a comment

• Copy link
• Copy Markdown

Verdict: APPROVE at e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd. The PR-body correction review 7179 asked for is in, so nothing is outstanding. @alex has to click Approve; this is a COMMENT for the same reason as before.

Fresh read of the description, after HOLD (114064) and its release (114065):

• I diffed the body against my first read. Only the §Self-bootstrap section changed, and nothing else in the description moved.
• No close keyword appears before #N in the title, body or thread comments.

New claims, checked at e4c0f35:

• release-cut.yml:60 pins reusable-release.yml@main.
• Every other toolkit-self wrapper that names a toolkit reusable pins @main: changelog-body-check, fragment-check, manifest-check, recover-pending-cut and register-check.
• The update step invokes rt prep --rolling-mode at reusable-release.yml:831.
• DefaultComposeScripts (internal/gates/selfboot_check.go:234) lists reusables and scripts/workflow-parse-check.sh, with no Go sources. So "check-self-bootstrap stays green either way" holds.

Gates, just now:

• rt review-submit-check --reviewed-head e4c0f35… returned READY.
• rt base-divergence-check returned PASS: main is still 5965014.
• CI on the head: 30 of 30 contexts green.

If anything lands on main before this merges, it needs a rebase and a fresh approval.

**Verdict: APPROVE at `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd`.** The PR-body correction review 7179 asked for is in, so nothing is outstanding. @alex has to click Approve; this is a COMMENT for the same reason as before. **Fresh read of the description,** after HOLD (114064) and its release (114065): - I diffed the body against my first read. Only the §Self-bootstrap section changed, and nothing else in the description moved. - No close keyword appears before `#N` in the title, body or thread comments. **New claims, checked at `e4c0f35`:** - `release-cut.yml:60` pins `reusable-release.yml@main`. - Every other toolkit-self wrapper that names a toolkit reusable pins `@main`: changelog-body-check, fragment-check, manifest-check, recover-pending-cut and register-check. - The update step invokes `rt prep --rolling-mode` at `reusable-release.yml:831`. - `DefaultComposeScripts` (`internal/gates/selfboot_check.go:234`) lists reusables and `scripts/workflow-parse-check.sh`, with no Go sources. So "`check-self-bootstrap` stays green either way" holds. **Gates, just now:** - `rt review-submit-check --reviewed-head e4c0f35…` returned READY. - `rt base-divergence-check` returned PASS: main is still `5965014`. - CI on the head: 30 of 30 contexts green. If anything lands on main before this merges, it needs a rebase and a fresh approval.

alex approved these changes 2026-10-02 15:06:20 +02:00

claude merged commit e4c0f35f0a into main 2026-10-02 15:07:14 +02:00

claude deleted branch claude/1495-comments-single-read 2026-10-02 15:07:14 +02:00

forgejo-actions commented 2026-10-02 15:08:00 +02:00

• Copy link
• Copy Markdown

Landing identity record

• PR: #1496
• landed commit (server merge_commit_sha): e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd
• effective official approval(s):
  • @alex, review #7181, stamped commit: e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd
• replay comparison: no identity change (stamped SHA equals landed SHA)

This is a post-merge identity record. It does not retroactively review the landed object; it records whether the server landed the object that an official approval named.

<!-- release-toolkit:landing-review-record-v1 landed=e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd --> ## Landing identity record - PR: #1496 - landed commit (server merge_commit_sha): `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd` - effective official approval(s): - @alex, review #7181, stamped commit: `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd` - replay comparison: no identity change (stamped SHA equals landed SHA) This is a post-merge identity record. It does not retroactively review the landed object; it records whether the server landed the object that an official approval named.

claude referenced this pull request 2026-10-02 15:09:15 +02:00

GET /issues/{n}/comments ignores page and limit, so all three paginated readers of it fail on ordinary history #1495

claude referenced this pull request 2026-10-02 15:10:36 +02:00

GET /issues/{n}/comments ignores page and limit, so all three paginated readers of it fail on ordinary history #1495

claude referenced this pull request 2026-10-02 15:19:41 +02:00

docs(agents): section 2 still prescribes an rc re-pin for toolkit-self, but #456 floats every wrapper at @main #1510

claude referenced this pull request from a commit 2026-10-02 15:24:38 +02:00

docs(agents): section 2 leads with the #456 floating pin; the rc re-pin is dormant (#1510)

claude referenced this pull request 2026-10-02 15:25:42 +02:00

docs(agents): section 2 leads with the #456 floating pin; the rc re-pin is dormant (#1510) #1511

claude referenced this pull request 2026-10-02 15:45:02 +02:00

docs(self-bootstrap): check-self-bootstrap, rt repin and arc42 still prescribe the rc re-pin that #456 made dormant #1512

claude referenced this pull request 2026-10-06 01:31:21 +02:00

fix(ac-closure): grade closing references in PR comments, inline review comments and the title (#1506) #1560

claude referenced this pull request 2026-10-06 11:08:00 +02:00

bug(gates): ac-closure-check does not see PR comments, inline review comments or the PR title, and Forgejo closes from all three #1506

Sign in to join this conversation.

Reviewers

No reviewers

rigger

alex

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

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

No description provided.