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

MCP/API paging traps: pageNum documented 1-based but is 0-based, pageSize cap doesn't clamp the offset, and playout rows carry no channelId #616

New Issue
Closed
opened 2026-07-25 17:14:57 +02:00 by timothy · 3 comments
timothy commented 2026-07-25 17:14:57 +02:00
Owner
Copy Link
Copy Source

Found driving 11 collections / 1,184 item-adds through the MCP write path in #487. Full write-up in #58 (closed, so filing the actionable parts here).

All three fail silently and plausibly, which is what makes them worth fixing.

1. pageNum is 0-based but documented as 1-based (highest value)

ErsatzTV.Mcp/ToolCatalog.cs:235:

Int("pageNum", "1-based page number (optional).", arg: In.Query),

It is actually 0-based. A caller that trusts the description and starts at pageNum=1:

collection totalCount fetched starting at pageNum=1
15 Music Video Mix (15 items) 15 0
20 Vaporwave (204 items) 204 104 (page 1 = items 101-200, page 2 = 201-204)

No error, no warning — just a short set. It reads exactly like data loss, and I burned a verification pass chasing it as one.

Fix: either correct the description to "0-based", or make the parameter 1-based to match the description. Prefer the latter (1-based is the least-surprise convention and /api/v1 is additive-only, but this is a fix to an under-specified param rather than a contract break — worth a judgment call).

2. pageSize is capped for the returned page, but the offset honors the requested value

GET /api/v1/collections/20/items?pageSize=500              -> totalCount 204, returned 100
GET /api/v1/collections/20/items?pageSize=500&pageNum=2    -> totalCount 204, returned 4

The returned page is capped at 100, but the offset arithmetic still uses ~500, so page 2 lands past item 200 instead of at item 101. An over-large pageSize silently changes which items you get, not just how many.

Fix: clamp pageSize first, then derive the offset from the clamped value.

3. reset_channel_playout takes a channel id; list_playouts exposes no channel id

ersatztv_list_playouts rows are {id, channelNumber, channelName, scheduleKind, buildStatus, …} — the id is the playout id. ersatztv_reset_channel_playout takes a channel id. Nothing in the playout row lets you get from one to the other, and the two id spaces overlap numerically, so passing the playout id succeeds against the wrong entity.

I hit this live: resetting "playout 23" reset channel 23 (Reality TV) instead of ch400 Music Video Mix. Harmless here (a rebuild), but the same confusion on a destructive tool would not be.

Fix: add channelId to the playout list/detail rows (additive, allowed post-freeze), and/or rename the tool arg to channelId so the schema names what it wants.

Not included here

Two additive-surface items from the same run are deliberately left out as larger design questions — see #58: no exact-tag match operator (tag: is analyzed, so tag:"Electronic" also matches X-mix 5 - Mr. C - The Electronic Storm), and the paged list_* MCP tools still exposing no pageNum/pageSize.

Done-when

  • pageNum semantics and its description agree; a test pins which — API is 0-based; ToolCatalogTests.Paged_Tools_Should_Document_PageNum_As_Zero_Based pins it for every paged tool (mutation-verified)
  • pageSize over-cap clamps the offset coherently; a test pins page-2 contents at an over-large pageSize — it already did on all 12 endpoints; GetCollectionItemsHandlerTests.Handle_Should_Derive_Offset_From_The_Clamped_PageSize pins it (mutation-verified: the raw-offset mutation returns an empty page)
  • Playout rows expose channelId (or the reset arg is renamed), OpenAPI artifacts regenerated — list rows since #297; detail DTO added here; the reset arg description now names the id-space trap; v1.json + v1.d.ts regenerated
  • Adversarial review passed — cold cross-family review, 3 rounds; Review-verdict: MERGEABLE @ 78ec997e
Found driving 11 collections / 1,184 item-adds through the MCP write path in #487. Full write-up in #58 (closed, so filing the actionable parts here). All three fail **silently and plausibly**, which is what makes them worth fixing. ## 1. `pageNum` is 0-based but documented as 1-based (highest value) `ErsatzTV.Mcp/ToolCatalog.cs:235`: ```csharp Int("pageNum", "1-based page number (optional).", arg: In.Query), ``` It is actually **0-based**. A caller that trusts the description and starts at `pageNum=1`: | collection | totalCount | fetched starting at pageNum=1 | |---|---|---| | 15 Music Video Mix (15 items) | 15 | **0** | | 20 Vaporwave (204 items) | 204 | **104** (page 1 = items 101-200, page 2 = 201-204) | No error, no warning — just a short set. It reads exactly like data loss, and I burned a verification pass chasing it as one. **Fix:** either correct the description to "0-based", or make the parameter 1-based to match the description. Prefer the latter (1-based is the least-surprise convention and `/api/v1` is additive-only, but this is a fix to an under-specified param rather than a contract break — worth a judgment call). ## 2. `pageSize` is capped for the returned page, but the offset honors the requested value ``` GET /api/v1/collections/20/items?pageSize=500 -> totalCount 204, returned 100 GET /api/v1/collections/20/items?pageSize=500&pageNum=2 -> totalCount 204, returned 4 ``` The returned page is capped at 100, but the offset arithmetic still uses ~500, so page 2 lands past item 200 instead of at item 101. An over-large `pageSize` silently changes *which* items you get, not just how many. **Fix:** clamp `pageSize` first, then derive the offset from the clamped value. ## 3. `reset_channel_playout` takes a channel id; `list_playouts` exposes no channel id `ersatztv_list_playouts` rows are `{id, channelNumber, channelName, scheduleKind, buildStatus, …}` — the `id` is the **playout** id. `ersatztv_reset_channel_playout` takes a **channel** id. Nothing in the playout row lets you get from one to the other, and the two id spaces overlap numerically, so passing the playout id succeeds against the wrong entity. I hit this live: resetting "playout 23" reset channel 23 (`Reality TV`) instead of ch400 `Music Video Mix`. Harmless here (a rebuild), but the same confusion on a destructive tool would not be. **Fix:** add `channelId` to the playout list/detail rows (additive, allowed post-freeze), and/or rename the tool arg to `channelId` so the schema names what it wants. ## Not included here Two additive-surface items from the same run are deliberately left out as larger design questions — see #58: no exact-tag match operator (`tag:` is analyzed, so `tag:"Electronic"` also matches `X-mix 5 - Mr. C - The Electronic Storm`), and the paged `list_*` MCP tools still exposing no `pageNum`/`pageSize`. ## Done-when - [x] `pageNum` semantics and its description agree; a test pins which — API is 0-based; `ToolCatalogTests.Paged_Tools_Should_Document_PageNum_As_Zero_Based` pins it for every paged tool (mutation-verified) - [x] `pageSize` over-cap clamps the offset coherently; a test pins page-2 contents at an over-large `pageSize` — it already did on all 12 endpoints; `GetCollectionItemsHandlerTests.Handle_Should_Derive_Offset_From_The_Clamped_PageSize` pins it (mutation-verified: the raw-offset mutation returns an empty page) - [x] Playout rows expose `channelId` (or the reset arg is renamed), OpenAPI artifacts regenerated — list rows since #297; detail DTO added here; the reset arg description now names the id-space trap; `v1.json` + `v1.d.ts` regenerated - [x] Adversarial review passed — cold cross-family review, 3 rounds; `Review-verdict: MERGEABLE @ 78ec997e`
timothy added the apibugpriority: medium labels 2026-07-25 17:15:02 +02:00
timothy added the in-progress label 2026-07-25 23:04:40 +02:00
timothy commented 2026-07-25 23:04:41 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this one (Claude Code, Opus 5 orchestrator session 2026-07-25).

Selector's top pick #629 is blocked behind PR #630, so this is the top eligible backlog item. No api-labelled siblings are open, so it runs solo.

Scope as filed: all three traps — (1) pageNum base mismatch, (2) pageSize clamp-before-offset, (3) channelId on playout rows.

Claiming this one (Claude Code, Opus 5 orchestrator session 2026-07-25). Selector's top pick #629 is blocked behind PR #630, so this is the top eligible backlog item. No `api`-labelled siblings are open, so it runs solo. Scope as filed: all three traps — (1) `pageNum` base mismatch, (2) `pageSize` clamp-before-offset, (3) `channelId` on playout rows.
timothy commented 2026-07-25 23:43:17 +02:00
Author
Owner
Copy Link
Copy Source

Scope correction after verifying each claim against the code

Working this on fix/616-paging-traps. Only one of the three filed traps was real as described — recording the evidence here since two of them will otherwise be re-derived by the next reader.

# Filed as Verified
1 pageNum documented 1-based, actually 0-based REAL — fixed
2 pageSize capped but offset honors the raw value NOT REPRODUCIBLE — pinned by test instead
3 playout rows carry no channelId STALE — list rows got it in #297; only the detail DTO lacked it

On (2). All 12 paged endpoints clamp before passing, and every handler skips by the clamped size; GetCollectionItemsHandler re-clamps defensively. The reported observation — pageSize=500&pageNum=2 on a 204-item collection returning 4 items — is exactly correct 0-based behaviour at the clamped width of 100: page 2 is items 201–204. This issue's own trap-1 table states the same arithmetic. A cold cross-family review independently enumerated all 12 endpoints and reached the same conclusion. So it is pinned by a mutation-verified test rather than "fixed".

On (3). PlayoutListItemResponseModel gained ChannelId in 1a3c8e27 (#297, 2026-07-22) — three days before this issue was filed. The report was measured against prod, which tracks the older :prod image, so it was true when observed and stale when filed. The detail DTO PlayoutResponseModel genuinely still lacked it; that is what the PR adds.

On (1) — the judgment call. The issue leaned toward making the parameter 1-based to match the description. I did the opposite and corrected the description, because /api/v1 is additive-only post-freeze (api.versioning-v1), 0-based is load-bearing in a dozen controllers plus the SPA, and a 1-based wrapper over a 0-based API would make the same parameter name mean two different things on two surfaces a reader routinely reads together. Rationale is recorded in the new api.paging-zero-based.

What the review then found that the issue didn't

  • A live instance of this bug class in the SPA. SchedulesScreen.tsx requested the rerun-collection picker with pageNum: 1, so it skipped the first page: with ≤100 rerun collections the picker was served an empty page and silently offered nothing. Fixed, with a mutation-verified vitest.
  • Two MCP tools wrapped paged endpoints while declaring no paging args — ersatztv_list_playouts and ersatztv_get_playout_items. Since ToolArgumentValidator rejects undeclared arguments, an agent was hard-capped at the first 100 rows with no way to ask for more. Both now take Page().

Deferred, tracked

  • #633 — the 12 OpenAPI pageNum parameters carry no description, so the 0-based contract is invisible to REST consumers. The decision record names this as a known gap rather than claiming coverage it doesn't have.
  • #634 — the same picker still asks for pageSize: 1000 against a 100 cap, so it truncates above 100 rerun collections.
## Scope correction after verifying each claim against the code Working this on `fix/616-paging-traps`. Only one of the three filed traps was real as described — recording the evidence here since two of them will otherwise be re-derived by the next reader. | # | Filed as | Verified | |---|---|---| | 1 | `pageNum` documented 1-based, actually 0-based | **REAL** — fixed | | 2 | `pageSize` capped but offset honors the raw value | **NOT REPRODUCIBLE** — pinned by test instead | | 3 | playout rows carry no `channelId` | **STALE** — list rows got it in #297; only the *detail* DTO lacked it | **On (2).** All 12 paged endpoints clamp before passing, and every handler skips by the clamped size; `GetCollectionItemsHandler` re-clamps defensively. The reported observation — `pageSize=500&pageNum=2` on a 204-item collection returning 4 items — is exactly correct 0-based behaviour at the clamped width of 100: page 2 *is* items 201–204. This issue's own trap-1 table states the same arithmetic. A cold cross-family review independently enumerated all 12 endpoints and reached the same conclusion. So it is pinned by a mutation-verified test rather than "fixed". **On (3).** `PlayoutListItemResponseModel` gained `ChannelId` in `1a3c8e27` (#297, 2026-07-22) — three days *before* this issue was filed. The report was measured against prod, which tracks the older `:prod` image, so it was true when observed and stale when filed. The **detail** DTO `PlayoutResponseModel` genuinely still lacked it; that is what the PR adds. **On (1) — the judgment call.** The issue leaned toward making the parameter 1-based to match the description. I did the opposite and corrected the description, because `/api/v1` is additive-only post-freeze (`api.versioning-v1`), 0-based is load-bearing in a dozen controllers plus the SPA, and a 1-based wrapper over a 0-based API would make the same parameter name mean two different things on two surfaces a reader routinely reads together. Rationale is recorded in the new `api.paging-zero-based`. ## What the review then found that the issue didn't - **A live instance of this bug class in the SPA.** `SchedulesScreen.tsx` requested the rerun-collection picker with `pageNum: 1`, so it skipped the first page: with ≤100 rerun collections the picker was served an **empty page** and silently offered nothing. Fixed, with a mutation-verified vitest. - **Two MCP tools wrapped paged endpoints while declaring no paging args** — `ersatztv_list_playouts` and `ersatztv_get_playout_items`. Since `ToolArgumentValidator` rejects undeclared arguments, an agent was hard-capped at the first 100 rows with no way to ask for more. Both now take `Page()`. ## Deferred, tracked - #633 — the 12 OpenAPI `pageNum` parameters carry no description, so the 0-based contract is invisible to REST consumers. The decision record names this as a known gap rather than claiming coverage it doesn't have. - #634 — the same picker still asks for `pageSize: 1000` against a 100 cap, so it truncates above 100 rerun collections.
timothy commented 2026-07-26 00:12:02 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Merged as PR #635 (f394d6ce). Of the three filed traps, one was real, one was not reproducible, and one was already fixed on main before this issue was written. The fix that shipped is therefore mostly documentation of an existing convention plus two defects the issue did not contain: a live 1-based caller in the SPA and two MCP tools that silently hard-capped paging.

Root cause: pageNum is 0-based across all 12 paged /api/v1 endpoints, and that convention was correct in code but written down nowhere. The MCP tool catalog therefore described it as "1-based", and a caller trusting the description skipped the first page — returning a short set with no error, which reads as data loss rather than an off-by-one (it cost #487 a verification pass). The same unwritten convention had already produced a second, unnoticed casualty in SchedulesScreen.tsx.

Decisions/conventions changed: added api.paging-zero-based (0-based everywhere; the offset derives from the EFFECTIVE page size, never the requested one; the cap is per-endpoint — 100 typical, 200 auto-tune members, 1000 search/all-items — and must not be documented as one number).

Reusable knowledge:

  • Two of three filed claims did not survive contact with the code. Trap 2's own evidence disproved it: pageSize=500&pageNum=2 returning 4 items of 204 is exactly correct 0-based behaviour at the clamped width of 100. Trap 3 was measured against prod, which tracks the older :prod image — it was true when observed and stale when filed. Re-verify a bug report against main before fixing it, and treat a prod-measured report as dated by the image, not by the report.
  • Documenting a convention is not the same as enforcing it. Writing down "pageNum is 0-based" immediately raised "so who violates it?" — which is what found the SPA bug and the two capped MCP tools. Neither was in the issue.
  • A filter-based test can be vacuous in the direction that matters. The first ToolCatalogTests version filtered on tools that already declared pageNum, so a tool wrapping a paged endpoint with no paging args escaped it entirely — exactly the defect present. Non-emptiness was not enough; it now pins the expected set by name.
  • Inserting a field mid-record in a positional C# record is a live hazard — an int landing in a different int slot compiles clean. Enumerate every construction site (here: 2).

Verification: .NET 1900 + MCP 59; web 996 across 105 files; tsc, eslint, format gate, BOM check all clean. All three behavioural tests mutation-verified — each fails when its fix is reverted. Cold cross-family adversarial review over three rounds (Review-verdict: MERGEABLE @ 78ec997e). CI green on every required check.

Deferred: #633 (the 12 OpenAPI pageNum parameters carry no description, so the 0-based contract is invisible to REST consumers — the record names this as a known gap rather than claiming coverage it lacks) and #634 (the schedules picker still asks pageSize: 1000 against a 100 cap, so it truncates above 100 rerun collections). Also noted but not filed: ToolCatalogTests compares tool names rather than PathTemplate against the paged-endpoint set, so a future paged tool without Page() needs the expected-set edit to catch it.

Docs updated: docs/decisions/records/api/paging-zero-based.md (new), docs/decisions/README.md (regenerated), docs/api-conventions.md, docs/mcp.md, plus regenerated ErsatzTV/wwwroot/openapi/v1.json and web/src/api/generated/v1.d.ts.

## Closing record **Outcome:** Merged as PR #635 (`f394d6ce`). Of the three filed traps, one was real, one was not reproducible, and one was already fixed on `main` before this issue was written. The fix that shipped is therefore mostly *documentation of an existing convention* plus two defects the issue did not contain: a live 1-based caller in the SPA and two MCP tools that silently hard-capped paging. **Root cause:** `pageNum` is 0-based across all 12 paged `/api/v1` endpoints, and that convention was correct in code but written down nowhere. The MCP tool catalog therefore described it as "1-based", and a caller trusting the description skipped the first page — returning a short set with no error, which reads as data loss rather than an off-by-one (it cost #487 a verification pass). The same unwritten convention had already produced a second, unnoticed casualty in `SchedulesScreen.tsx`. **Decisions/conventions changed:** added `api.paging-zero-based` (0-based everywhere; the offset derives from the EFFECTIVE page size, never the requested one; the cap is per-endpoint — 100 typical, 200 auto-tune members, 1000 search/all-items — and must not be documented as one number). **Reusable knowledge:** - **Two of three filed claims did not survive contact with the code.** Trap 2's own evidence disproved it: `pageSize=500&pageNum=2` returning 4 items of 204 is exactly correct 0-based behaviour at the clamped width of 100. Trap 3 was measured against **prod**, which tracks the older `:prod` image — it was true when observed and stale when filed. Re-verify a bug report against `main` before fixing it, and treat a prod-measured report as dated by the image, not by the report. - **Documenting a convention is not the same as enforcing it.** Writing down "pageNum is 0-based" immediately raised "so who violates it?" — which is what found the SPA bug and the two capped MCP tools. Neither was in the issue. - **A filter-based test can be vacuous in the direction that matters.** The first `ToolCatalogTests` version filtered on tools that *already* declared `pageNum`, so a tool wrapping a paged endpoint with **no** paging args escaped it entirely — exactly the defect present. Non-emptiness was not enough; it now pins the expected set by name. - **Inserting a field mid-record in a positional C# record is a live hazard** — an `int` landing in a different `int` slot compiles clean. Enumerate every construction site (here: 2). **Verification:** .NET 1900 + MCP 59; web 996 across 105 files; `tsc`, eslint, format gate, BOM check all clean. All three behavioural tests mutation-verified — each fails when its fix is reverted. Cold cross-family adversarial review over three rounds (`Review-verdict: MERGEABLE @ 78ec997e`). CI green on every required check. **Deferred:** #633 (the 12 OpenAPI `pageNum` parameters carry no description, so the 0-based contract is invisible to REST consumers — the record names this as a known gap rather than claiming coverage it lacks) and #634 (the schedules picker still asks `pageSize: 1000` against a 100 cap, so it truncates above 100 rerun collections). Also noted but not filed: `ToolCatalogTests` compares tool names rather than `PathTemplate` against the paged-endpoint set, so a future paged tool without `Page()` needs the expected-set edit to catch it. **Docs updated:** `docs/decisions/records/api/paging-zero-based.md` (new), `docs/decisions/README.md` (regenerated), `docs/api-conventions.md`, `docs/mcp.md`, plus regenerated `ErsatzTV/wwwroot/openapi/v1.json` and `web/src/api/generated/v1.d.ts`.
timothy removed the in-progress label 2026-07-26 00:12: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 api bug priority: medium
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#616