fix(616): paging is 0-based — correct the MCP contract, fix a live 1-based SPA caller, un-cap two MCP tools #635

Merged

timothy merged 4 commits from fix/616-paging-traps into main 2026-07-26 00:11:22 +02:00

Conversation 1 Commits 4 Files Changed 14

+300 -7

timothy commented 2026-07-25 23:51:57 +02:00

Owner

Copy Link

Copy Source

Fixes #616.

What was actually wrong

The issue filed three paging traps. Verifying each against the code before changing it, only one was real as described:

#	Filed as	Verified
1	pageNum documented 1-based, actually 0-based	REAL — fixed
2	pageSize capped but the 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

(2) All 12 paged endpoints clamp before passing, and every handler skips by the clamped size. 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. A cold cross-family review independently enumerated all 12 endpoints and agreed. Pinned by a mutation-verified test rather than "fixed".

(3) ChannelId landed on the list DTO in 1a3c8e27 (#297) three days before this issue was filed — the report was measured against prod, which tracks the older :prod image. The detail DTO genuinely lacked it; that is what this adds.

(1) — the judgment call. The issue leaned toward making the parameter 1-based to match its description. This does the opposite and corrects the description: /api/v1 is additive-only post-freeze (api.versioning-v1), 0-based is load-bearing across a dozen controllers and 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 reads together. Recorded in the new api.paging-zero-based.

What the review 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. Since pageSize clamps to 100, that skipped the entire first page — with ≤100 rerun collections (the ordinary case) the picker was served an empty page and silently offered no rerun collections at all. Fixed, mutation-verified.
• Two MCP tools wrapped paged endpoints while declaring no paging args (ersatztv_list_playouts, ersatztv_get_playout_items). ToolArgumentValidator rejects undeclared arguments, so an agent was hard-capped at the first 100 rows with no way to ask for more. Both now take Page().
• The new MCP test was vacuous in the direction that mattered — it filtered on tools that already declared pageNum, so a tool with no paging escaped it entirely. Now pins the expected set by name.
• Four over-broad claims in the new decision record / docs/mcp.md corrected (one caught by self-review, three by the reviewer). These read as normative, so a false universal in them is a defect.

Commits

1. 8d35a279 — the verified fixes: pageNum description, ChannelId on the detail DTO, both mutation-verified tests, record + docs + OpenAPI/SPA regen.
2. 5648f8e9 — self-review: the pageSize cap is per-endpoint (100 / 200 / 1000), not one number; a docs/mcp.md example was outright false for search_all_items.
3. af39123e — review findings: the SPA bug, the two MCP tools, the test count-guard, record corrections.
4. 78ec997e — docs: stop the record title and mcp.md contradicting their own bodies.

Deferred, tracked

• #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.
• #634 — the same picker still asks pageSize: 1000 against a 100 cap, so it truncates above 100 rerun collections.

Verification

.NET 1900 + MCP 59 pass · web 996 across 105 files · tsc clean · eslint clean · format gate exit 0 · no BOMs. Both new behavioural tests and the SPA test are mutation-verified — each fails when its fix is reverted.

🤖 Generated with Claude Code

Fixes #616. ## What was actually wrong The issue filed three paging traps. Verifying each against the code before changing it, **only one was real as described**: | # | Filed as | Verified | |---|---|---| | 1 | `pageNum` documented 1-based, actually 0-based | **REAL** — fixed | | 2 | `pageSize` capped but the 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 | **(2)** All 12 paged endpoints clamp before passing, and every handler skips by the clamped size. 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. A cold cross-family review independently enumerated all 12 endpoints and agreed. Pinned by a mutation-verified test rather than "fixed". **(3)** `ChannelId` landed on the list DTO in `1a3c8e27` (#297) three days *before* this issue was filed — the report was measured against prod, which tracks the older `:prod` image. The detail DTO genuinely lacked it; that is what this adds. **(1) — the judgment call.** The issue leaned toward making the parameter 1-based to match its description. This does the opposite and corrects the description: `/api/v1` is additive-only post-freeze (`api.versioning-v1`), 0-based is load-bearing across a dozen controllers and 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 reads together. Recorded in the new `api.paging-zero-based`. ## What the review 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`. Since `pageSize` clamps to 100, that skipped the entire first page — with ≤100 rerun collections (the ordinary case) the picker was served an **empty page** and silently offered no rerun collections at all. Fixed, mutation-verified. - **Two MCP tools wrapped paged endpoints while declaring no paging args** (`ersatztv_list_playouts`, `ersatztv_get_playout_items`). `ToolArgumentValidator` rejects undeclared arguments, so an agent was hard-capped at the first 100 rows with no way to ask for more. Both now take `Page()`. - **The new MCP test was vacuous in the direction that mattered** — it filtered on tools that *already* declared `pageNum`, so a tool with no paging escaped it entirely. Now pins the expected set by name. - **Four over-broad claims** in the new decision record / `docs/mcp.md` corrected (one caught by self-review, three by the reviewer). These read as normative, so a false universal in them is a defect. ## Commits 1. `8d35a279` — the verified fixes: `pageNum` description, `ChannelId` on the detail DTO, both mutation-verified tests, record + docs + OpenAPI/SPA regen. 2. `5648f8e9` — self-review: the `pageSize` cap is **per-endpoint** (100 / 200 / 1000), not one number; a `docs/mcp.md` example was outright false for `search_all_items`. 3. `af39123e` — review findings: the SPA bug, the two MCP tools, the test count-guard, record corrections. 4. `78ec997e` — docs: stop the record title and `mcp.md` contradicting their own bodies. ## Deferred, tracked - #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. - #634 — the same picker still asks `pageSize: 1000` against a 100 cap, so it truncates above 100 rerun collections. ## Verification .NET 1900 + MCP 59 pass · web 996 across 105 files · `tsc` clean · eslint clean · format gate exit 0 · no BOMs. Both new behavioural tests and the SPA test are mutation-verified — each fails when its fix is reverted. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 4 commits 2026-07-25 23:51:58 +02:00

fix(616): document paging as 0-based, expose channelId on playout detail ... 8d35a2792f

#616 filed three MCP/API paging traps. Two were real; one was not, and one was
already half-fixed on main. Verified each against the code before changing it.

REAL — pageNum documented as 1-based. `ToolCatalog.Page()` described pageNum as
"1-based page number" while every paged controller defaults it to 0, floors it
with `Math.Max(0, pageNum)`, and skips `PageNum * PageSize`. A caller that
trusted the description started at page 1 and silently lost the first page: no
error, just a short set that reads as data loss rather than an off-by-one (it
cost #487 a verification pass). Fixed in the description rather than by making
the MCP layer 1-based: /api/v1 is additive-only post-freeze, 0-based is
load-bearing in a dozen controllers and 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 reads together.

NOT REAL — "pageSize caps the page but the offset honors the requested value".
Not reproducible on any endpoint. Every controller clamps before passing, every
handler skips by the clamped size, and 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. The issue's own trap-1 table
states this. Pinned by test rather than "fixed".

ALREADY FIXED — playout LIST rows gained channelId in #297 (2026-07-22), three
days before #616 was filed; the report was measured against prod, which runs an
older :prod image. The DETAIL response (PlayoutResponseModel) genuinely still
lacked it, so channelId is added there (additive) and the reset_channel_playout
argument now names the trap: the id spaces overlap numerically, so passing a
playout id silently resets a different channel and returns a plausible 202.

Tests, both mutation-verified (each fails when its fix is reverted):
- ToolCatalogTests pins "0-based" on EVERY paged tool's pageNum description,
  with a non-empty guard so it can't pass vacuously over an empty tool set.
- GetCollectionItemsHandlerTests pins 0-based page boundaries and proves the
  offset derives from the clamped pageSize (page 1 at pageSize=500 returns
  items 101-150; the mutation that honors 500 returns an empty page).

Docs: new decision record api.paging-zero-based (catalog regenerated), the
api-conventions paging bullet, and a Paging section in docs/mcp.md. OpenAPI
v1.json + web/src/api/generated/v1.d.ts regenerated for the added field.

fixes #616

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs(616): scope the pageSize cap per endpoint — it is not one number ... 5648f8e92e

Self-review of the previous commit caught an overclaim I introduced. I wrote the
paging docs as if the pageSize cap were uniformly 100. It is not:

  most reads (collections, library browse, logs, playouts)  Math.Clamp(.., 1, 100)
  GetAutoTuneChannelMembers                                 pageSize <= 0 ? 100 : Min(.., 200)
  GET /api/v1/search/all-items                              default 500, cap 1000

That made a concrete example in docs/mcp.md simply false. It claimed
`pageSize=500&pageNum=1` returns items 101-200 "not 501-1000" — but
ersatztv_search_all_items also takes Page(), and 500 is UNDER its cap, so there
page 1 really is items 501-1000. A caller following that example on the one tool
most likely to be paged hard would have mis-derived its offsets, which is the
same class of silent-short-set error this issue is about.

The invariant that actually holds everywhere is the derivation, not any single
cap: the offset comes from the EFFECTIVE (bounded) page size, never the
requested one. Reworded to say that, in docs/mcp.md, the api.paging-zero-based
record (rule + mechanics + body), and the ToolCatalog Page() comment. Catalog
regenerated. The record's mechanics line no longer claims every controller uses
Math.Clamp — ChannelController does not.

No behaviour change; MCP suite still 59/59.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(616): fix a live 1-based SPA caller and un-cap two MCP playout tools ... af39123e8e

Findings from the cold cross-family review of 8d35a279/5648f8e9. The review
confirmed the three conclusions in 8d35a279 (the offset math is sound on all 12
paged endpoints; the list DTO really did already carry ChannelId; the new tests
are non-vacuous) but found the previous commits had documented a convention
without checking who actually violates it.

HIGH — a live instance of this issue's own bug class, in the SPA.
web/src/screens/SchedulesScreen.tsx asked for the rerun-collection picker with
`pageNum: 1, pageSize: 1000`. pageNum is 0-based and pageSize clamps to 100, so
the request skipped the first 100 rows: with <=100 rerun collections (the normal
case) the picker was served an EMPTY page and silently offered no rerun
collections at all; above 100 it dropped rows 1-100. Exactly the silent-short-set
failure #616 is about, shipped in the UI. Now `pageNum: 0`, with a vitest that
asserts the offset and is mutation-verified (restoring `pageNum: 1` fails it).

Checked the rest of the SPA rather than assuming: every other pageNum caller is
0-based. CollectionsScreen's paging loop starts at 1 but only after fetching
page 0 explicitly, so it is correct — verified before touching it.

MEDIUM — two MCP tools wrapped paged endpoints while declaring no paging args.
ersatztv_list_playouts and ersatztv_get_playout_items had no pageNum/pageSize,
and ToolArgumentValidator rejects undeclared arguments, so an agent was hard
capped at the first 100 rows with no way to ask for more and no error saying so.
Both now take Page(). A catalog-wide sweep confirmed these were the only two:
the other paged endpoints are not exposed as MCP tools at all.

That same gap made the new ToolCatalog test vacuous in the direction that
mattered — it filtered on tools that ALREADY declare pageNum, so a tool missing
paging entirely escaped it. It now pins the expected set by name, so a new tool
over a paged endpoint has to be added deliberately.

Record corrections (these are read as normative, so over-broad claims are
defects): "every wrapper says 0-based" was false for the OpenAPI surface, whose
12 pageNum parameters carry no description — named as a remaining gap instead of
claimed as done. "Every controller floors with Math.Max" ignored
SearchController's Math.Clamp. The ids-in-rows corollary was stated as an audit
result when PlayoutListItemResponseModel.ScheduleName has no schedule id;
restated as a rule about actionable ids.

Verification: .NET 1900 + MCP 59 pass; web 996 pass across 105 files; tsc clean;
eslint clean; format gate exit 0; no BOMs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs(616): stop the record title and mcp.md from contradicting their own bodies ... 78ec997eae

Two LOWs from the re-review of af39123e. Both are the same defect I have already
corrected twice on this branch: a universal claim that the corrected body right
below it disproves. These read as normative, so a reader who stops at the title
gets the false version.

- The record was titled "...and every wrapper says so" while its own body admits
  the 12 OpenAPI pageNum parameters carry no description at all (tracked as
  #633). Retitled to state the target and name the exception.
- docs/mcp.md led with "Rows that reference another entity carry that entity's
  id" — but PlayoutListItemResponseModel.ScheduleName ships with no schedule id.
  Restated as a rule about ACTIONABLE references, matching the wording the record
  already uses, with the known exception named.

The two remaining LOWs are accepted deferrals, not fixed here: the >100 rerun
collection truncation needs paging-to-completeness plus a multi-page fixture
(#634), and the catalog test's forward-looking gap (it compares tool names, not
PathTemplate against the paged-endpoint set) would need the MCP test project to
reference the controllers to close properly.

Docs only; catalog regenerated. No behaviour change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

timothy commented 2026-07-25 23:55:16 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 78ec997

Cold cross-family adversarial review, three rounds against this branch.

Round 1 (initial diff) confirmed the three scope conclusions — the offset math is sound on all 12 paged endpoints, the list DTO already carried ChannelId, and the new tests are non-vacuous — and found one HIGH (a live 1-based caller in SchedulesScreen.tsx serving an empty picker), two MEDIUMs (two MCP tools wrapping paged endpoints with no paging args; the catalog test vacuous for exactly that case) and two LOWs.

Round 2 (fix commit af39123e) verified every fix and independently re-derived the MCP sweep: only 5 paged REST routes are MCP-exposed and all 5 now declare Page(). No BLOCKER/HIGH/MEDIUM. Verdict MERGEABLE @ af39123e with 4 LOWs.

Round 3 (78ec997e) covers the two doc LOWs taken from round 2 — the record title and the mcp.md ids-in-rows lead-in no longer contradict their own corrected bodies. No findings at any severity; build_decisions_catalog.py --check and decisions_validate.py both pass.

Remaining LOWs are accepted deferrals, tracked: #634 (picker truncates above 100 rerun collections; needs paging-to-completeness plus a multi-page fixture) and #633 (OpenAPI pageNum parameters carry no description). The catalog test's forward-looking gap — it compares tool names, not PathTemplate against the paged-endpoint set — would need the MCP test project to reference the controllers to close properly.

Review-verdict: MERGEABLE @ 78ec997 Cold cross-family adversarial review, three rounds against this branch. Round 1 (initial diff) confirmed the three scope conclusions — the offset math is sound on all 12 paged endpoints, the list DTO already carried ChannelId, and the new tests are non-vacuous — and found one HIGH (a live 1-based caller in SchedulesScreen.tsx serving an empty picker), two MEDIUMs (two MCP tools wrapping paged endpoints with no paging args; the catalog test vacuous for exactly that case) and two LOWs. Round 2 (fix commit af39123e) verified every fix and independently re-derived the MCP sweep: only 5 paged REST routes are MCP-exposed and all 5 now declare Page(). No BLOCKER/HIGH/MEDIUM. Verdict MERGEABLE @ af39123e with 4 LOWs. Round 3 (78ec997e) covers the two doc LOWs taken from round 2 — the record title and the mcp.md ids-in-rows lead-in no longer contradict their own corrected bodies. No findings at any severity; build_decisions_catalog.py --check and decisions_validate.py both pass. Remaining LOWs are accepted deferrals, tracked: #634 (picker truncates above 100 rerun collections; needs paging-to-completeness plus a multi-page fixture) and #633 (OpenAPI pageNum parameters carry no description). The catalog test's forward-looking gap — it compares tool names, not PathTemplate against the paged-endpoint set — would need the MCP test project to reference the controllers to close properly.

timothy merged commit f394d6ce83 into main 2026-07-26 00:11:22 +02:00

timothy deleted branch fix/616-paging-traps 2026-07-26 00:11:23 +02:00

timothy referenced this issue from a commit 2026-07-26 00:11:23 +02:00

Merge pull request 'fix(616): paging is 0-based — correct the MCP contract, fix a live 1-based SPA caller, un-cap two MCP tools' (#635) from fix/616-paging-traps into main

timothy referenced this pull request 2026-07-26 00:12:02 +02:00

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

timothy referenced this pull request 2026-07-26 00:20:18 +02:00

`.claude/skills/jellyfin/` is a stale divergent copy, not the symlink CLAUDE.md describes — it documents auth that v12 only accepts by legacy opt-in #617

Sign in to join this conversation.

Reviewers

No Reviewers

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

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#635