timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

page_statuses still terminates on its first empty page — is /statuses/{sha} subject to #870's post-pagination filtering? #893

New Issue
Closed
opened 2026-08-30 10:57:49 +02:00 by timothy · 2 comments
timothy commented 2026-08-30 10:57:49 +02:00
Owner
Copy Link
Copy Source

Split out of #870, which fixed the SAME defect shape one endpoint over and deliberately did not
touch this walk.

The asymmetry #870 created

count_pr_mutations no longer treats an empty page as the end of the list: it reads every page to
its 20-page cap and trusts the counts only when the LAST page came back empty. page_statuses
still returns on its FIRST empty page:

case "$kind" in
  null)  ph_ok=yes; ph_rows=$acc; return 0 ;;
  array) ;;
  *)     ph_rows=$acc; return 0 ;;
esac
n=$(printf '%s' "$raw" | jq -r 'length' 2>/dev/null) || n=""
case "$n" in ''|*[!0-9]*) ph_rows=$acc; return 0 ;; esac
if [ "$n" -eq 0 ]; then ph_ok=yes; ph_rows=$acc; return 0; fi

The two walks now share only the CAP rule, not a termination rule. #870 originally claimed in prose
that "the two now agree"; cold review caught that and it was corrected rather than shipped.

Why this is a real question and not tidiness

#870's defect was not about empty pages in general — it was about ONE endpoint's handler.
ListIssueCommentsAndTimeline applies LIMIT/OFFSET at the database level and filters
CommentTypeCode rows out AFTERWARDS into a nil slice, so a page of 50 inline comments is
byte-identical to a page past the end. If /statuses/{sha} filters after paging too, then
page_statuses truncating early can drop a raced human Review-verdict: row from ph_rows —
which by that file's own account is the worst outcome this gate can produce, since the post-write
check would then conclude raced=0 and leave a machine success standing over a human rejection.

What is measured, and what is not

Measured on this instance at Gitea 1.27.1 on 2026-08-30:

endpoint ?limit=1 ?limit=50 true total?
/issues/{n}/timeline (14-row PR) X-Total-Count: 1 X-Total-Count: 14 no — post-filter PAGE length
/statuses/{sha} (105-row head) X-Total-Count: 105 X-Total-Count: 105 yes

A true total is consistent with counting before filtering, and so with a terminator that means what
it says. That is evidence, not proof — it shows the count is not computed from the serialized
page, which is what makes the timeline endpoint's terminator a lie; it does not exhibit the absence
of a filtering predicate in ListStatuses. Two independent reviews of #870 read the Gitea source as
not filtering there, and neither treated that as settling it.

State it as the open question it is: nobody has exhibited a filtering predicate on this endpoint
either way.

Options

  1. Establish it from the v1.27.1 source — read ListStatuses / GetLatestCommitStatus and say
    whether any row is dropped after the LIMIT/OFFSET. Cheapest, and it either closes this outright
    or turns it into a live defect.
  2. Adopt #870's rule here anyway, on the grounds that a terminator nobody can prove is a
    terminator nobody should trust. Costs 20 requests per walk on a walk that already retries each
    page, and page_statuses runs on the post-write path where latency is least welcome.
  3. Record why the asymmetry is correct — if option 1 shows no filtering, the right outcome is a
    dated sentence in ci.verdict-write-retarget-fence saying the two walks differ because the two
    endpoints differ    , so the next reader does not "tidy" them together.

There is a real argument for the asymmetry: an empty page 1 is ANOMALOUS on a PR timeline (a PR is
created by a push, which is an event) and ORDINARY on /statuses/{sha} (a head nothing has posted
to yet). The walks already diverge there deliberately, and that divergence is documented.

Note the X-Total-Count measurement is itself worth keeping whatever the outcome: it is a
derivable page count this walk does not use, and it would give page_statuses a terminator that
does not depend on interpreting an empty page at all.

Done-when

  • Whether /statuses/{sha} drops rows after pagination is ESTABLISHED from the 1.27.1 source,
    not inferred from the header measurement above — it does NOT: getCommitStatuses appends
    unconditionally and its only filter is a SQL WHERE in the same query as the LIMIT/OFFSET
  • If it does: page_statuses gets #870's treatment... N/A — branch not taken. Ticked to
    record that it was resolved, not done: option 1 showed no post-pagination filtering, so there
    is no live defect and no fixture to write. page_statuses is untouched.
  • If it does not: the asymmetry is recorded with its reason and a date, so it is not tidied away
  • ci.verdict-write-retarget-fence states which of the two it is, replacing #870's pointer here
  • Adversarial review passed — 5 cold rounds plus a cross-family Codex pass; three reviewers each
    attacked this conclusion specifically and none found a short/empty-page path. Final: CLEAN
Split out of #870, which fixed the SAME defect shape one endpoint over and deliberately did not touch this walk. ## The asymmetry #870 created `count_pr_mutations` no longer treats an empty page as the end of the list: it reads every page to its 20-page cap and trusts the counts only when the LAST page came back empty. `page_statuses` still returns on its FIRST empty page: ```sh case "$kind" in null) ph_ok=yes; ph_rows=$acc; return 0 ;; array) ;; *) ph_rows=$acc; return 0 ;; esac n=$(printf '%s' "$raw" | jq -r 'length' 2>/dev/null) || n="" case "$n" in ''|*[!0-9]*) ph_rows=$acc; return 0 ;; esac if [ "$n" -eq 0 ]; then ph_ok=yes; ph_rows=$acc; return 0; fi ``` The two walks now share only the CAP rule, not a termination rule. #870 originally claimed in prose that "the two now agree"; cold review caught that and it was corrected rather than shipped. ## Why this is a real question and not tidiness #870's defect was not about empty pages in general — it was about ONE endpoint's handler. `ListIssueCommentsAndTimeline` applies LIMIT/OFFSET at the database level and filters `CommentTypeCode` rows out AFTERWARDS into a nil slice, so a page of 50 inline comments is byte-identical to a page past the end. **If `/statuses/{sha}` filters after paging too, then `page_statuses` truncating early can drop a raced human `Review-verdict:` row from `ph_rows`** — which by that file's own account is the worst outcome this gate can produce, since the post-write check would then conclude `raced=0` and leave a machine `success` standing over a human rejection. ## What is measured, and what is not Measured on this instance at Gitea 1.27.1 on 2026-08-30: | endpoint | `?limit=1` | `?limit=50` | true total? | |---|---|---|---| | `/issues/{n}/timeline` (14-row PR) | `X-Total-Count: 1` | `X-Total-Count: 14` | **no** — post-filter PAGE length | | `/statuses/{sha}` (105-row head) | `X-Total-Count: 105` | `X-Total-Count: 105` | **yes** | A true total is consistent with counting before filtering, and so with a terminator that means what it says. **That is evidence, not proof** — it shows the count is not computed from the serialized page, which is what makes the timeline endpoint's terminator a lie; it does not exhibit the absence of a filtering predicate in `ListStatuses`. Two independent reviews of #870 read the Gitea source as not filtering there, and neither treated that as settling it. State it as the open question it is: **nobody has exhibited a filtering predicate on this endpoint either way.** ## Options 1. **Establish it from the v1.27.1 source** — read `ListStatuses` / `GetLatestCommitStatus` and say whether any row is dropped after the LIMIT/OFFSET. Cheapest, and it either closes this outright or turns it into a live defect. 2. **Adopt #870's rule here anyway**, on the grounds that a terminator nobody can prove is a terminator nobody should trust. Costs 20 requests per walk on a walk that already retries each page, and `page_statuses` runs on the post-write path where latency is least welcome. 3. **Record why the asymmetry is correct** — if option 1 shows no filtering, the right outcome is a dated sentence in `ci.verdict-write-retarget-fence` saying the two walks differ *because the two endpoints differ*, so the next reader does not "tidy" them together. There is a real argument for the asymmetry: an empty page 1 is ANOMALOUS on a PR timeline (a PR is created by a push, which is an event) and ORDINARY on `/statuses/{sha}` (a head nothing has posted to yet). The walks already diverge there deliberately, and that divergence is documented. Note the `X-Total-Count` measurement is itself worth keeping whatever the outcome: it is a derivable page count this walk does not use, and it would give `page_statuses` a terminator that does not depend on interpreting an empty page at all. ## Done-when - [x] Whether `/statuses/{sha}` drops rows after pagination is ESTABLISHED from the 1.27.1 source, not inferred from the header measurement above — it does NOT: `getCommitStatuses` appends unconditionally and its only filter is a SQL `WHERE` in the same query as the LIMIT/OFFSET - [x] ~~If it does: `page_statuses` gets #870's treatment...~~ **N/A — branch not taken.** Ticked to record that it was resolved, not done: option 1 showed no post-pagination filtering, so there is no live defect and no fixture to write. `page_statuses` is untouched. - [x] If it does not: the asymmetry is recorded with its reason and a date, so it is not tidied away - [x] `ci.verdict-write-retarget-fence` states which of the two it is, replacing #870's pointer here - [x] Adversarial review passed — 5 cold rounds plus a cross-family Codex pass; three reviewers each attacked this conclusion specifically and none found a short/empty-page path. Final: CLEAN
timothy added the ci-cdpriority: mediumsecurity labels 2026-08-30 10:58:03 +02:00
timothy added the in-progress label 2026-09-02 00:07:33 +02:00
timothy commented 2026-09-02 00:07:44 +02:00
Author
Owner
Copy Link
Copy Source

Claiming — Claude Code session (Opus 5), bundled with #869.

Same mechanism as #869: establish a claim from the v1.27.1 Gitea source rather than inferring it. Here that is option 1 — read ListStatuses / GetLatestCommitStatus and say whether any row is dropped after the LIMIT/OFFSET — which per this issue either closes it outright or turns it into a live defect in page_statuses.

Claiming — Claude Code session (Opus 5), bundled with #869. Same mechanism as #869: establish a claim from the v1.27.1 Gitea source rather than inferring it. Here that is option 1 — read `ListStatuses` / `GetLatestCommitStatus` and say whether any row is dropped after the LIMIT/OFFSET — which per this issue either closes it outright or turns it into a live defect in `page_statuses`.
timothy commented 2026-09-02 21:56:05 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Option 1 taken and it closed the question outright — no live defect. PR #905. GET /repos/{o}/{r}/statuses/{sha} does NOT drop rows after pagination at v1.27.1, so page_statuses terminating on its FIRST empty page is safe and the asymmetry with count_pr_mutations is correct. Recorded with its reason and a date (option 3) so it is not tidied away.

Established from source, not inferred from the header:

  • repo.GetCommitStatuses → getCommitStatuses calls db.FindAndCount[git_model.CommitStatus], then builds the response with an unconditional append loop. No continue, no predicate, no nil-drop — convert.ToCommitStatus returns a pointer for every row it is handed.
  • CommitStatusOptions.ToConds() is the only filter — repo_id, sha, optional state — and it is a SQL WHERE the database evaluates in the same query as the LIMIT/OFFSET, never a pass over rows after they return.
  • The contrast is exact: ListIssueCommentsAndTimeline pages at the DB level and then appends conditionally on two predicates — comment.Type != CommentTypeCode and isXRefCommentAccessible(...), a per-viewer check — which is what makes a fully filtered page byte-identical to the end of the list.

Root cause: #870 fixed a defect in one endpoint's handler and correctly declined to generalise to another. The open question was whether the two handlers shared the defect; nobody had read the second one. They do not.

Decisions/conventions changed: no new keys. ci.verdict-write-retarget-fence now states which of the two it is, replacing this issue's pointer.

Reusable knowledge:

  1. The asymmetry is load-bearing — do not "tidy" the two walks together. They differ because their endpoints differ. Adopting #870's treatment here would cost 20 requests per walk on the post-write path and buy nothing.
  2. The per-endpoint terminator shapes are a source-level property, not a coincidence. getCommitStatuses builds make([]*api.CommitStatus, 0, len(statuses)) — a non-nil empty slice serializing as []; the timeline declares var apiComments []*api.TimelineComment — a nil slice serializing as bare null. The repo had this as an empirical table; it is now checkable in the source instead of by constructing a past-the-end request.
  3. X-Total-Count is now explained on both sides. /statuses/{sha} sets it from FindAndCount's SQL COUNT (a true total); the timeline sets it to int64(len(apiComments)) — literally the filtered page length, which is why that header cannot derive a page count there. Re-confirmed live where the two must differ: page 1 of 50 on a head reporting X-Total-Count: 63.
  4. Worth keeping in mind for a future change: a true X-Total-Count would give page_statuses a terminator that does not depend on interpreting an empty page at all. Not adopted here — the current terminator is now known-sound — but it is the cheaper option if this ever needs hardening.

Verification: source read at tag v1.27.1 (handlers, model options, db.FindAndCount, both serializers); live re-confirmation of the header behaviour; scripts/tests 1565 passed / 3 skipped. Three independent cold reviewers each attacked the conclusion specifically — looking for a filter, permission check, nil-drop, alternate handler, state/sort param or ListAll path that could return a short/empty page with rows beyond — and none found one. No executable change: page_statuses is untouched.

Deferred: none.

Docs updated: docs/decisions/records/ci/verdict-write-retarget-fence.md, docs/ci-cd.md.

## Closing record **Outcome:** Option 1 taken and it closed the question outright — no live defect. PR #905. `GET /repos/{o}/{r}/statuses/{sha}` does **NOT** drop rows after pagination at v1.27.1, so `page_statuses` terminating on its FIRST empty page is safe and the asymmetry with `count_pr_mutations` is **correct**. Recorded with its reason and a date (option 3) so it is not tidied away. Established from source, not inferred from the header: - `repo.GetCommitStatuses` → `getCommitStatuses` calls `db.FindAndCount[git_model.CommitStatus]`, then builds the response with an **unconditional** `append` loop. No `continue`, no predicate, no nil-drop — `convert.ToCommitStatus` returns a pointer for every row it is handed. - `CommitStatusOptions.ToConds()` is the only filter — `repo_id`, `sha`, optional `state` — and it is a SQL `WHERE` the database evaluates in the **same query** as the `LIMIT`/`OFFSET`, never a pass over rows after they return. - The contrast is exact: `ListIssueCommentsAndTimeline` pages at the DB level and then appends conditionally on **two** predicates — `comment.Type != CommentTypeCode` **and** `isXRefCommentAccessible(...)`, a per-viewer check — which is what makes a fully filtered page byte-identical to the end of the list. **Root cause:** #870 fixed a defect in one endpoint's handler and correctly declined to generalise to another. The open question was whether the two handlers shared the defect; nobody had read the second one. They do not. **Decisions/conventions changed:** no new keys. `ci.verdict-write-retarget-fence` now states which of the two it is, replacing this issue's pointer. **Reusable knowledge:** 1. **The asymmetry is load-bearing — do not "tidy" the two walks together.** They differ because their endpoints differ. Adopting #870's treatment here would cost 20 requests per walk on the post-write path and buy nothing. 2. **The per-endpoint terminator shapes are a source-level property, not a coincidence.** `getCommitStatuses` builds `make([]*api.CommitStatus, 0, len(statuses))` — a non-nil empty slice serializing as `[]`; the timeline declares `var apiComments []*api.TimelineComment` — a nil slice serializing as bare `null`. The repo had this as an empirical table; it is now checkable in the source instead of by constructing a past-the-end request. 3. **`X-Total-Count` is now explained on both sides.** `/statuses/{sha}` sets it from `FindAndCount`'s SQL COUNT (a true total); the timeline sets it to `int64(len(apiComments))` — literally the filtered page length, which is why that header cannot derive a page count there. Re-confirmed live where the two must differ: page 1 of 50 on a head reporting `X-Total-Count: 63`. 4. Worth keeping in mind for a future change: a true `X-Total-Count` would give `page_statuses` a terminator that does not depend on interpreting an empty page at all. Not adopted here — the current terminator is now known-sound — but it is the cheaper option if this ever needs hardening. **Verification:** source read at tag `v1.27.1` (handlers, model options, `db.FindAndCount`, both serializers); live re-confirmation of the header behaviour; `scripts/tests` 1565 passed / 3 skipped. Three independent cold reviewers each attacked the conclusion specifically — looking for a filter, permission check, nil-drop, alternate handler, `state`/`sort` param or ListAll path that could return a short/empty page with rows beyond — and none found one. No executable change: `page_statuses` is untouched. **Deferred:** none. **Docs updated:** `docs/decisions/records/ci/verdict-write-retarget-fence.md`, `docs/ci-cd.md`.
timothy removed the in-progress label 2026-09-02 22:21:12 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels ci-cd priority: medium security
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#893