frankenbit/release-toolkit

Watch

Fork 0

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

New issue

Closed

opened 2026-10-02 11:14:05 +02:00 by claude · 3 comments

claude commented

2026-10-02 11:14:05 +02:00

Member

The issue-comments endpoint on this forge does not paginate. It ignores both page and limit, and every request returns the complete list. Three readers in the tree walk it as if it paginated, and each one fails once the history is non-empty:

reader	ends the walk on	fails when	effect
`scripts/record-landing-review.sh`	an empty page; refuses a repeated page	the PR has 1+ comments	`go-ci / record reviewed vs landed commit (push)` is red on main and no record is posted
`forgejo.ListIssueComments` (`paginateStrict`)	an empty page; ErrAPI at `MaxPages=40`	the PR has 1+ comments	`rt prep`'s rolling-deadlock path is fatal (latent)
`forknotice.findExisting`	a page shorter than 50 rows	the PR has 50+ comments	the fork-PR approval notice refuses (latent)

Measured

Source. Forgejo tag v15.0.7 on Codeberg, routers/api/v1/repo/issue_comment.go, function ListIssueComments:

Its swagger block declares only since and before.
The handler builds issues_model.FindCommentsOptions{IssueID, Since, Before, Type} with no ListOptions.
models/issues/comment.go FindComments paginates only if opts.Page > 0, so it never does here.
The handler does set X-Total-Count.

Live. GET /api/v1/version returns 15.0.7+gitea-1.22.0. Probed 2026-10-02:

/issues/1488/comments                       2 rows  ids [113675, 113681]
/issues/1488/comments?limit=50&page=1       2 rows  ids [113675, 113681]
/issues/1488/comments?limit=50&page=2       2 rows  ids [113675, 113681]
/issues/1488/comments?limit=50&page=3       2 rows  ids [113675, 113681]
/issues/1488/comments?limit=1&page=1        2 rows  ids [113675, 113681]
/issues/1488/comments?limit=1&page=2        2 rows  ids [113675, 113681]
/issues/1488/comments?page=200              2 rows  ids [113675, 113681]
/issues/607/comments   bare | limit=50&page=2 | limit=1   42 rows each, 42 unique ids   X-Total-Count: 42
/issues/1348/comments  bare | limit=50&page=2             35 rows each                  X-Total-Count: 35

/pulls/1488/reviews?limit=50&page=1         3 rows  ids [7143, 7147, 7148]
/pulls/1488/reviews?limit=50&page=2         0 rows
/pulls/1488/reviews?limit=1&page=2          1 row   id 7147

limit is ignored too: limit=1 still returns 2 rows.
The 35- and 42-row bare reads exceed default_paging_num=30 (GET /settings/api).
/reviews paginates normally.

Not measured live: a history above max_response_items=50. No issue or PR in this repo has more than 42 comments (a scan of all 1494), and I did not post 50 comments to make one. The source reading above is the evidence that a single read is not capped at 50.

1. Visible now: the landing recorder is red on main

go-ci / record reviewed vs landed commit (push), read from each job's log in the web UI:

main commit	PR	other comments	run	step log
`2e8f46f`	#1486	3	18545	`comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade`, then `exitcode '2'`
`29b541b`	#1488	2	18602	same line, `exitcode '2'`
`55a12b9`	#1489	2	18632	same line

Every PR merged since the bounding change (#1472, c5142ab) follows the same split:

Zero comments, record posted: #1468 (112396) and #1472 (112387).
Comments, no record: #1486, #1488 and #1489.

The green control is f639dcb. It was a direct push: /commits/{sha}/pull returns 404, so the script exits before it reads any comments.

Not this bug: 31677cc also shows this job as failure, but on the (pull_request) context, and the job in run 18451 reads Canceled.

#1469 turned the endless walk into this refusal. Its AC accepted rc=2 at the repeated comment page. It kept paginating, though, so the recorder now refuses on every landed PR that has a comment.

2. Latent: `rt prep`'s rolling-deadlock path is fatal on any comment

How the failure reaches rt prep:

internal/forgejo/reads.go ListIssueComments calls paginateStrict, which calls paginateMode (internal/forgejo/client.go).
paginateMode ends only on an empty page. Otherwise it returns ErrAPI pagination hit MaxPages=40 cap on a full page.
internal/prep/pr.go AnnounceRollingPRDeadlock reads comments to de-duplicate its marker and returns any read error.
cmd/rt/prep.go returns that error, so rt prep fails.

Measured on unfixed origin/main 29b541b, in golang:1.25, against a fake server that ignores page the way Forgejo does:

ListIssueComments, 51-row history          -> ErrAPI "pagination hit MaxPages=40 cap on a full page" after 40 requests
AnnounceRollingPRDeadlock, 1 comment        -> "read comments for rolling PR #42 before announcing deadlock: ... MaxPages=40 cap" after 40 reads
runPrep --rolling, live REQUEST_CHANGES,
  1 unrelated comment                       -> same error; rt prep fails

So:

A deadlocked rolling PR with any comment cannot be announced.
One with no comments fails from its second deadlocked refresh, because the first notice is itself a comment.

It hasn't surfaced because the path runs only while a rolling PR carries a live REQUEST_CHANGES. The existing fixtures serve [] for comments, or answer page 2 with [], which is not how this endpoint behaves.

3. Latent: the fork-PR approval notice at 50+ comments

internal/forknotice findExisting walks ?limit=50&page=N and stops on a page shorter than 50 rows. Below 50 comments, page 1 is "short" and the walk ends correctly. At 50 or more, measured against the same page-ignoring fake on 29b541b:

Notice present: page 2 re-delivers it, and the run refuses with found more than one approval notice after 2 GETs.
Notice absent: it refuses with comment walk exceeded 100 pages after 100 GETs.

Why it matters

Red main: main gets a red go-ci context on almost every PR landing. The reviewed-vs-landed identity record (#1299) has not been posted for any commented PR since #1472. A context that is always red also teaches people to ignore it.
Fatal rt prep: the deadlock notice exists so that a blocked rolling PR is visibly blocked (#1183). As written, it makes rt prep fatal on the cut path instead.
Fails on ordinary input: all three readers fail closed, so nothing was ever written wrongly, but they fail on ordinary input.

Acceptance criteria

record-landing-review.sh reads the comments collection in ONE request, with no page or limit. It validates the response as a JSON array of {id: number, body: string} rows and searches the full list for the marker. The reviews walk stays paginated and bounded. — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). One bare GET in scripts/record-landing-review.sh; the reviews walk keeps check_page_progress and max_pages.
forgejo.ListIssueComments makes ONE request and strict-decodes a JSON array, so {}, null and a row without id all refuse. The error classes stay the same. — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). callRetry + decodeJSONArray, as in ListDirectory; the {}, null and missing-id arms refuse.
forknotice.findExisting makes ONE request, and its duplicate-notice refusal stays. — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). Duplicate-notice refusal kept; JSON null now refused too.
The PR lists every GET reader of /issues/{n}/comments and what happened to each. — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). See the PR description's "Every GET reader of the endpoint" table: 3 readers, plus the POST-only and prose hits.
Each reader's fixtures ignore page the way Forgejo does, returning the same non-empty array on every request. Each fixture asserts one request and the full list. Each arm is mutation-verified: it goes red when the paginating read is restored. — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). Mutants M1–M7 are red (PR description), plus the reviewer's two (review 7179).
A changelog.d/<issue>-<slug>.fixed.md fragment is added. — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). changelog.d/1495-comments-single-read.fixed.md; the density check passes.
After landing, the landing commit's go-ci / record reviewed vs landed commit (push) is green and its PR carries the record (verified by read-back). — DONE @ #1496 (e4c0f35, reviewed as e4c0f35). Run 18971 success; its log reads recorded review-to-landing identity for PR #1496 at e4c0f35…. Record comment 114071 names review 7181 by @alex: "no identity change".

Not in scope

X-Total-Count cross-check: comparing the single read against X-Total-Count (live, it equals the row count on all three issues probed). It would catch a future Forgejo that starts paginating this endpoint. The Go client's call does not expose response headers today, so it needs its own tracker.
Back-fill: adding records for PRs that landed while the recorder refused.

The issue-comments endpoint on this forge does not paginate. It ignores both `page` and `limit`, and every request returns the complete list. Three readers in the tree walk it as if it paginated, and each one fails once the history is non-empty: | reader | ends the walk on | fails when | effect | |---|---|---|---| | `scripts/record-landing-review.sh` | an empty page; refuses a repeated page | the PR has 1+ comments | `go-ci / record reviewed vs landed commit (push)` is red on main and no record is posted | | `forgejo.ListIssueComments` (`paginateStrict`) | an empty page; ErrAPI at `MaxPages=40` | the PR has 1+ comments | `rt prep`'s rolling-deadlock path is fatal (latent) | | `forknotice.findExisting` | a page shorter than 50 rows | the PR has 50+ comments | the fork-PR approval notice refuses (latent) | ## Measured **Source.** Forgejo tag `v15.0.7` on Codeberg, `routers/api/v1/repo/issue_comment.go`, function `ListIssueComments`: - Its swagger block declares only `since` and `before`. - The handler builds `issues_model.FindCommentsOptions{IssueID, Since, Before, Type}` with no `ListOptions`. - `models/issues/comment.go` `FindComments` paginates only `if opts.Page > 0`, so it never does here. - The handler does set `X-Total-Count`. **Live.** `GET /api/v1/version` returns `15.0.7+gitea-1.22.0`. Probed 2026-10-02: ``` /issues/1488/comments 2 rows ids [113675, 113681] /issues/1488/comments?limit=50&page=1 2 rows ids [113675, 113681] /issues/1488/comments?limit=50&page=2 2 rows ids [113675, 113681] /issues/1488/comments?limit=50&page=3 2 rows ids [113675, 113681] /issues/1488/comments?limit=1&page=1 2 rows ids [113675, 113681] /issues/1488/comments?limit=1&page=2 2 rows ids [113675, 113681] /issues/1488/comments?page=200 2 rows ids [113675, 113681] /issues/607/comments bare | limit=50&page=2 | limit=1 42 rows each, 42 unique ids X-Total-Count: 42 /issues/1348/comments bare | limit=50&page=2 35 rows each X-Total-Count: 35 /pulls/1488/reviews?limit=50&page=1 3 rows ids [7143, 7147, 7148] /pulls/1488/reviews?limit=50&page=2 0 rows /pulls/1488/reviews?limit=1&page=2 1 row id 7147 ``` - `limit` is ignored too: `limit=1` still returns 2 rows. - The 35- and 42-row bare reads exceed `default_paging_num=30` (`GET /settings/api`). - `/reviews` paginates normally. **Not measured live:** a history above `max_response_items=50`. No issue or PR in this repo has more than 42 comments (a scan of all 1494), and I did not post 50 comments to make one. The source reading above is the evidence that a single read is not capped at 50. ### 1. Visible now: the landing recorder is red on main `go-ci / record reviewed vs landed commit (push)`, read from each job's log in the web UI: | main commit | PR | other comments | run | step log | |---|---|---|---|---| | `2e8f46f` | #1486 | 3 | 18545 | `comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade`, then `exitcode '2'` | | `29b541b` | #1488 | 2 | 18602 | same line, `exitcode '2'` | | `55a12b9` | #1489 | 2 | 18632 | same line | Every PR merged since the bounding change (#1472, `c5142ab`) follows the same split: - **Zero comments, record posted:** #1468 (`112396`) and #1472 (`112387`). - **Comments, no record:** #1486, #1488 and #1489. The green control is `f639dcb`. It was a direct push: `/commits/{sha}/pull` returns 404, so the script exits before it reads any comments. Not this bug: `31677cc` also shows this job as `failure`, but on the `(pull_request)` context, and the job in run 18451 reads **Canceled**. #1469 turned the endless walk into this refusal. Its AC accepted `rc=2` at the repeated comment page. It kept paginating, though, so the recorder now refuses on every landed PR that has a comment. ### 2. Latent: `rt prep`'s rolling-deadlock path is fatal on any comment How the failure reaches `rt prep`: 1. `internal/forgejo/reads.go` `ListIssueComments` calls `paginateStrict`, which calls `paginateMode` (`internal/forgejo/client.go`). 2. `paginateMode` ends only on an empty page. Otherwise it returns ErrAPI `pagination hit MaxPages=40 cap on a full page`. 3. `internal/prep/pr.go` `AnnounceRollingPRDeadlock` reads comments to de-duplicate its marker and returns any read error. 4. `cmd/rt/prep.go` returns that error, so `rt prep` fails. Measured on unfixed `origin/main` `29b541b`, in `golang:1.25`, against a fake server that ignores `page` the way Forgejo does: ``` ListIssueComments, 51-row history -> ErrAPI "pagination hit MaxPages=40 cap on a full page" after 40 requests AnnounceRollingPRDeadlock, 1 comment -> "read comments for rolling PR #42 before announcing deadlock: ... MaxPages=40 cap" after 40 reads runPrep --rolling, live REQUEST_CHANGES, 1 unrelated comment -> same error; rt prep fails ``` So: - A deadlocked rolling PR with any comment cannot be announced. - One with no comments fails from its second deadlocked refresh, because the first notice is itself a comment. It hasn't surfaced because the path runs only while a rolling PR carries a live `REQUEST_CHANGES`. The existing fixtures serve `[]` for comments, or answer page 2 with `[]`, which is not how this endpoint behaves. ### 3. Latent: the fork-PR approval notice at 50+ comments `internal/forknotice` `findExisting` walks `?limit=50&page=N` and stops on a page shorter than 50 rows. Below 50 comments, page 1 is "short" and the walk ends correctly. At 50 or more, measured against the same page-ignoring fake on `29b541b`: - **Notice present:** page 2 re-delivers it, and the run refuses with `found more than one approval notice` after 2 GETs. - **Notice absent:** it refuses with `comment walk exceeded 100 pages` after 100 GETs. ## Why it matters - **Red main:** `main` gets a red `go-ci` context on almost every PR landing. The reviewed-vs-landed identity record (#1299) has not been posted for any commented PR since #1472. A context that is always red also teaches people to ignore it. - **Fatal `rt prep`:** the deadlock notice exists so that a blocked rolling PR is visibly blocked (#1183). As written, it makes `rt prep` fatal on the cut path instead. - **Fails on ordinary input:** all three readers fail closed, so nothing was ever written wrongly, but they fail on ordinary input. ## Acceptance criteria - [x] `record-landing-review.sh` reads the comments collection in ONE request, with no `page` or `limit`. It validates the response as a JSON array of `{id: number, body: string}` rows and searches the full list for the marker. The reviews walk stays paginated and bounded. — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). One bare GET in `scripts/record-landing-review.sh`; the reviews walk keeps `check_page_progress` and `max_pages`. - [x] `forgejo.ListIssueComments` makes ONE request and strict-decodes a JSON array, so `{}`, `null` and a row without `id` all refuse. The error classes stay the same. — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). `callRetry` + `decodeJSONArray`, as in `ListDirectory`; the `{}`, `null` and missing-`id` arms refuse. - [x] `forknotice.findExisting` makes ONE request, and its duplicate-notice refusal stays. — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). Duplicate-notice refusal kept; JSON `null` now refused too. - [x] The PR lists every GET reader of `/issues/{n}/comments` and what happened to each. — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). See the PR description's "Every GET reader of the endpoint" table: 3 readers, plus the POST-only and prose hits. - [x] Each reader's fixtures ignore `page` the way Forgejo does, returning the same non-empty array on every request. Each fixture asserts one request and the full list. Each arm is mutation-verified: it goes red when the paginating read is restored. — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). Mutants M1–M7 are red (PR description), plus the reviewer's two (review 7179). - [x] A `changelog.d/<issue>-<slug>.fixed.md` fragment is added. — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). `changelog.d/1495-comments-single-read.fixed.md`; the density check passes. - [x] After landing, the landing commit's `go-ci / record reviewed vs landed commit (push)` is green and its PR carries the record (verified by read-back). — DONE @ #1496 (`e4c0f35`, reviewed as `e4c0f35`). Run 18971 `success`; its log reads `recorded review-to-landing identity for PR #1496 at e4c0f35…`. Record comment 114071 names review 7181 by @alex: "no identity change". ## Not in scope - **`X-Total-Count` cross-check:** comparing the single read against `X-Total-Count` (live, it equals the row count on all three issues probed). It would catch a future Forgejo that starts paginating this endpoint. The Go client's `call` does not expose response headers today, so it needs its own tracker. - **Back-fill:** adding records for PRs that landed while the recorder refused.

claude added the

labels

2026-10-02 11:14:05 +02:00

claude referenced this issue from a commit

2026-10-02 11:21:38 +02:00

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

claude referenced this issue

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

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

claude commented

2026-10-02 12:28:57 +02:00

Author

Member

Another instance, read from the job log in the web UI.

2060ed1 (#1493, landed 2026-10-02 12:20): go-ci / record reviewed vs landed commit (push) failed in run 18782, job 2, with comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade, then exitcode '2'.

The other 15 push contexts on that commit are green, so the red go-ci on main there is this recorder alone. The same context is also red on 55a12b9 (#1485) and 29b541b (#1484), at "Failing after 3s" per the commit status, which is consistent with the table above. I did not read those two logs.

Another instance, read from the job log in the web UI. `2060ed1` (#1493, landed 2026-10-02 12:20): `go-ci / record reviewed vs landed commit (push)` failed in run 18782, job 2, with `comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade`, then `exitcode '2'`. The other 15 push contexts on that commit are green, so the red `go-ci` on `main` there is this recorder alone. The same context is also red on `55a12b9` (#1485) and `29b541b` (#1484), at "Failing after 3s" per the commit status, which is consistent with the table above. I did not read those two logs.

claude referenced this issue

2026-10-02 12:29:16 +02:00

recover-pending-cut fold-back silently drops [Unreleased] text outside an accepted ### heading #1491

claude referenced this issue

2026-10-02 13:27:34 +02:00

bug(prep): the [Unreleased] drop refusal prints a CRLF file's \r, so a terminal overwrites the line it names #1507

claude commented

2026-10-02 13:27:35 +02:00

Author

Member

Another instance, read from the job log in the web UI.

de49186 (#1508, landed 2026-10-02 13:23): go-ci / record reviewed vs landed commit (push) failed in run 18842, job 2, with comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade, then exitcode '2'.

When I read the commit status, 14 of the other 15 push contexts on that commit were green, and release / fire-cut (push) was still waiting to run. So the red go-ci on main there is this recorder alone. The PR had two comments and three reviews when it landed.

Another instance, read from the job log in the web UI. `de49186` (#1508, landed 2026-10-02 13:23): `go-ci / record reviewed vs landed commit (push)` failed in run 18842, job 2, with `comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade`, then `exitcode '2'`. When I read the commit status, 14 of the other 15 push contexts on that commit were green, and `release / fire-cut (push)` was still waiting to run. So the red `go-ci` on `main` there is this recorder alone. The PR had two comments and three reviews when it landed.

claude referenced this issue

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 referenced this issue from a commit

2026-10-02 14:48:13 +02:00

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

release-bot referenced this issue

2026-10-02 15:08:26 +02:00

chore(release): v0.64.0 #1494

claude commented

2026-10-02 15:10:36 +02:00

Author

Member

All seven acceptance criteria are ticked above. The PR used Refs, so this issue is being shut by hand.

Landed: #1496 was fast-forwarded to main at 15:07 as e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd.
- That is the reviewed head: the server's merge_commit_sha equals it, and a local git range-diff shows = against the original commit.
- The head branch is deleted.
Approval: review 7181 by alex, APPROVED at e4c0f35, official and not stale. The reviewer's verdicts are reviews 7179 and 7180, posted as COMMENT with "Verdict: APPROVE at e4c0f35".
Checks on e4c0f35:
- 15 of 16 push contexts are green. They include go-ci / record reviewed vs landed commit (push), run 18971. Its log reads "recorded review-to-landing identity for PR #1496", and record comment 114071 on #1496 reports "no identity change".
- release / fire-cut (push) is queued for a go runner. In the same run, rt decide chose mode=update, so fire-cut will skip, as it did on the earlier commits. No release is being cut.
- Every pull_request context was green at merge.
Re-pin: none is needed. See the "Self-bootstrap" section of #1496's description.

The X-Total-Count cross-check and back-filling the missed records remain out of scope, as listed above.

All seven acceptance criteria are ticked above. The PR used `Refs`, so this issue is being shut by hand. - **Landed:** #1496 was fast-forwarded to `main` at 15:07 as `e4c0f35f0abfa39486089f0be4ec3a00ee53a9fd`. - That is the reviewed head: the server's `merge_commit_sha` equals it, and a local `git range-diff` shows `=` against the original commit. - The head branch is deleted. - **Approval:** review 7181 by alex, APPROVED at `e4c0f35`, official and not stale. The reviewer's verdicts are reviews 7179 and 7180, posted as COMMENT with "Verdict: APPROVE at `e4c0f35`". - **Checks on `e4c0f35`:** - 15 of 16 push contexts are green. They include `go-ci / record reviewed vs landed commit (push)`, run 18971. Its log reads "recorded review-to-landing identity for PR #1496", and record comment 114071 on #1496 reports "no identity change". - `release / fire-cut (push)` is queued for a `go` runner. In the same run, `rt decide` chose `mode=update`, so `fire-cut` will skip, as it did on the earlier commits. No release is being cut. - Every `pull_request` context was green at merge. - **Re-pin:** none is needed. See the "Self-bootstrap" section of #1496's description. The `X-Total-Count` cross-check and back-filling the missed records remain out of scope, as listed above.

claude closed this issue

2026-10-02 15:10:36 +02:00

claude referenced this issue from a commit

2026-10-06 01:31:13 +02:00

fix(ac-closure): grade the forge's own closing references from comments, review comments and the title (#1506)

claude referenced this issue

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