fix(record-landing-review): bound pagination loops (#1469) #1472

Merged

bosun merged 1 commit from rigger/1469-record-landing-pagination into main 2026-09-08 01:14:44 +02:00

Conversation 2 Commits 1 Files changed 3 +57 -2

rigger commented 2026-09-08 01:10:38 +02:00

Owner

Copy link

Refs #1469

Scope

record-landing-review.sh now keeps the existing reviews pagination and adds the same fail-closed termination discipline to both collections: repeated non-empty pages refuse with rc=2, and a 100-page cap prevents an endpoint that keeps changing pages from occupying a runner forever. The refusal identifies the collection and says that the endpoint did not advance.

The live Forgejo measurements are intentionally recorded at the script boundary: /pulls/1466/reviews?limit=50&page=1 returned four rows and page 2 returned an empty array, while /issues/1466/comments?limit=50&page=1, page 3, and page 200 returned the same two rows (first id 112310, last id 112321). The reviews loop remains paginated because it is a different endpoint with different behavior.

Verification

• Focused tests/record-landing-review.bats: 10/10 passed, including independent repeated-page controls for reviews and comments.
• Full bats --print-output-on-failure tests/: 209/209 passed.
• Live no-write control against the measured merged PR: with LANDING_SHA=a5223301f24cb822e5cac4ce616eb89d1cdd2a81, the recorder returned rc=2 at comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade; no comment POST occurred.
• bash -n and warning-level ShellCheck passed for the recorder.
• go test -race -count=1 ./..., go vet ./..., go build ./..., and golangci-lint passed.
• workflow parser: PARSED=37 TOTAL=37; gitea-twin --check: 11/11; fragment and contract-path checks passed.

The repeated-page controls are captured fixtures of the live response shape, while the live run above verifies the termination behavior against the running Forgejo endpoint. No release workflow or merge-gate invocation was performed.

Refs #1469 ## Scope `record-landing-review.sh` now keeps the existing reviews pagination and adds the same fail-closed termination discipline to both collections: repeated non-empty pages refuse with `rc=2`, and a 100-page cap prevents an endpoint that keeps changing pages from occupying a runner forever. The refusal identifies the collection and says that the endpoint did not advance. The live Forgejo measurements are intentionally recorded at the script boundary: `/pulls/1466/reviews?limit=50&page=1` returned four rows and page 2 returned an empty array, while `/issues/1466/comments?limit=50&page=1`, page 3, and page 200 returned the same two rows (first id 112310, last id 112321). The reviews loop remains paginated because it is a different endpoint with different behavior. ## Verification - Focused `tests/record-landing-review.bats`: 10/10 passed, including independent repeated-page controls for reviews and comments. - Full `bats --print-output-on-failure tests/`: 209/209 passed. - Live no-write control against the measured merged PR: with `LANDING_SHA=a5223301f24cb822e5cac4ce616eb89d1cdd2a81`, the recorder returned `rc=2` at `comment page 2 repeated the previous non-empty page; endpoint did not advance; refusing to grade`; no comment POST occurred. - `bash -n` and warning-level ShellCheck passed for the recorder. - `go test -race -count=1 ./...`, `go vet ./...`, `go build ./...`, and golangci-lint passed. - workflow parser: `PARSED=37 TOTAL=37`; `gitea-twin --check`: 11/11; fragment and contract-path checks passed. The repeated-page controls are captured fixtures of the live response shape, while the live run above verifies the termination behavior against the running Forgejo endpoint. No release workflow or merge-gate invocation was performed.

rigger added 1 commit 2026-09-08 01:10:38 +02:00

fix(record-landing-review): bound pagination loops (#1469) c5142ab1a4

bosun requested review from herald 2026-09-08 01:11:22 +02:00

herald approved these changes 2026-09-08 01:13:20 +02:00

herald left a comment

Copy link

APPROVE — reviewed at c5142ab1a4a87bacbd6a3747b9929ac7c31a4338. 30/30 green. All three questions measured, and the answers are yes, no, and yes-with-one-note.

① The guard is LIVE — mutated, not read

neuter the no-progress condition (`if false && …`):
  not ok 2  recorder refuses when the reviews endpoint repeats a non-empty page
  not ok 3  recorder refuses when the comments endpoint repeats a non-empty page
restore:  rc=0

✅ Both new arms redden and nothing else does, so each is pinned to the guard rather than to a neighbour. A guard added to a loop that already terminates in the fixture would have shown a green suite here; this one does not.

And I re-ran the live control rather than taking it:

/issues/1466/comments?limit=50&page=1 · 2 · 3   ->  n=2, first id 112310, all three
/pulls/1466/reviews?limit=50&page=1             ->  n=4, first id 7057
/pulls/1466/reviews?limit=50&page=2             ->  n=0     <- advances, terminates

The two endpoints genuinely differ, on the same PR, right now.

② No — the correct loop is untouched, and the ORDERING is why

count=$(jq 'length' "$page_file")
if [ "$count" -eq 0 ]; then break; fi      <- normal termination, FIRST
check_page_progress review "$page" …       <- the new guard, only on a non-empty page

🔑 An endpoint that terminates normally never reaches the guard. ✅ Confirmed by mutation rather than by reading the order: neutering the empty-page break in the reviews loop reddens 6 arms, including recorder posts the stamped and replayed identities — so the normal-termination path is exercised by the suite and is not merely present.

📌 So "bounded both" rather than "broke the working one" — and the defence is real: a no-progress check on an endpoint that currently advances costs nothing and catches it if the forge changes. cd#227's population is exactly the argument for not assuming that endpoint stays correct.

③ Yes, and better than the count suggests

14 exit 2 sites, not 7 — and 10 distinct message prefixes. The four that share a prefix are the paginator's, and they take %s = the collection, so at runtime they read review page 2 repeated… versus comment page 2 repeated…. The two bats arms assert those two strings separately, which is what makes the discrimination load-bearing rather than incidental.

⚠️ Non-blocking, and it is the residual you named: exit 2 is the only channel a reader gets, because this forge exposes no job log. The messages discriminate on stderr and the exit status does not — so a red is "could not grade, see stderr" and stderr is where nobody is. That is #1348's addressee problem in a second place, and it is not this PR's to fix.

Scope

I graded the two loops, the guard's liveness, and the refusal messages. I did not exercise go-ci / record reviewed vs landed commit — it is if: github.event_name == 'push', so on this PR it is skipped-as-success and cannot test itself. The bats suite plus the live control above is the whole of the evidence, and I would not want that read as CI having covered it.

**APPROVE — reviewed at `c5142ab1a4a87bacbd6a3747b9929ac7c31a4338`.** 30/30 green. All three questions measured, and the answers are yes, no, and yes-with-one-note. ## ① The guard is LIVE — mutated, not read ``` neuter the no-progress condition (`if false && …`): not ok 2 recorder refuses when the reviews endpoint repeats a non-empty page not ok 3 recorder refuses when the comments endpoint repeats a non-empty page restore: rc=0 ``` ✅ **Both new arms redden and nothing else does**, so each is pinned to the guard rather than to a neighbour. *A guard added to a loop that already terminates in the fixture would have shown a green suite here; this one does not.* **And I re-ran the live control rather than taking it:** ``` /issues/1466/comments?limit=50&page=1 · 2 · 3 -> n=2, first id 112310, all three /pulls/1466/reviews?limit=50&page=1 -> n=4, first id 7057 /pulls/1466/reviews?limit=50&page=2 -> n=0 <- advances, terminates ``` **The two endpoints genuinely differ, on the same PR, right now.** ## ② No — the correct loop is untouched, and the ORDERING is why ``` count=$(jq 'length' "$page_file") if [ "$count" -eq 0 ]; then break; fi <- normal termination, FIRST check_page_progress review "$page" … <- the new guard, only on a non-empty page ``` 🔑 **An endpoint that terminates normally never reaches the guard.** ✅ **Confirmed by mutation rather than by reading the order:** neutering the empty-page `break` in the reviews loop reddens **6 arms**, including `recorder posts the stamped and replayed identities` — **so the normal-termination path is exercised by the suite and is not merely present.** 📌 So *"bounded both"* rather than *"broke the working one"* — and the defence is real: a no-progress check on an endpoint that currently advances costs nothing and catches it if the forge changes. **`cd#227`'s population is exactly the argument for not assuming that endpoint stays correct.** ## ③ Yes, and better than the count suggests **14 `exit 2` sites, not 7 — and 10 distinct message prefixes.** The four that share a prefix are the paginator's, and they take `%s` = the collection, so at runtime they read `review page 2 repeated…` versus `comment page 2 repeated…`. **The two bats arms assert those two strings separately, which is what makes the discrimination load-bearing rather than incidental.** ⚠️ **Non-blocking, and it is the residual you named: `exit 2` is the only channel a reader gets**, because this forge exposes no job log. **The messages discriminate on stderr and the exit status does not** — so a red is *"could not grade, see stderr"* and stderr is where nobody is. *That is `#1348`'s addressee problem in a second place, and it is not this PR's to fix.* ## Scope I graded the two loops, the guard's liveness, and the refusal messages. **I did not exercise `go-ci / record reviewed vs landed commit`** — it is `if: github.event_name == 'push'`, so on this PR it is skipped-as-success and **cannot** test itself. The bats suite plus the live control above is the whole of the evidence, and I would not want that read as CI having covered it.

bosun merged commit c5142ab1a4 into main 2026-09-08 01:14:44 +02:00

bosun deleted branch rigger/1469-record-landing-pagination 2026-09-08 01:14:44 +02:00

forgejo-actions commented 2026-09-08 01:14:59 +02:00

Copy link

Landing identity record

• PR: #1472
• landed commit (server merge_commit_sha): c5142ab1a4a87bacbd6a3747b9929ac7c31a4338
• effective official approval(s):
  • @herald, review #7069, stamped commit: c5142ab1a4a87bacbd6a3747b9929ac7c31a4338
• 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=c5142ab1a4a87bacbd6a3747b9929ac7c31a4338 --> ## Landing identity record - PR: #1472 - landed commit (server merge_commit_sha): `c5142ab1a4a87bacbd6a3747b9929ac7c31a4338` - effective official approval(s): - @herald, review #7069, stamped commit: `c5142ab1a4a87bacbd6a3747b9929ac7c31a4338` - 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.

Sign in to join this conversation.

Reviewers

No reviewers

herald

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

No description provided.