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

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

Browse Source 
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>
This commit is contained in:
timothy
2026-07-25 23:41:54 +02:00
co-authored by Claude Opus 5
parent 5648f8e92e
commit af39123e8e
6 changed files with 66 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ErsatzTV.Mcp.Tests/ToolCatalogTests.cs
+19 -3
View File
@@ -213,9 +213,25 @@ public class ToolCatalogTests
.Where(t => t.InputSchema.RootElement.GetProperty("properties").TryGetProperty("pageNum", out _))
.ToArray();
// Guard the guard: if the catalog stops exposing paged tools this test must fail loudly
// rather than pass vacuously over an empty set.
paged.ShouldNotBeEmpty();
// Guard the guard twice over. An emptiness check alone is not enough: this test filters on
// tools that ALREADY declare pageNum, so a tool wrapping a paged endpoint while declaring no
// paging args escapes the filter entirely and the test still passes. That is not
// hypothetical — ersatztv_list_playouts and ersatztv_get_playout_items did exactly that, and
// because ToolArgumentValidator rejects undeclared arguments, an MCP caller was hard-capped
// at the first 100 rows with no way to ask for more. So the expected set is named here: a
// new tool over a paged endpoint must be added to it, and dropping paging from any of these
// fails the test rather than silently shrinking its scope.
string[] mustDeclarePaging =
[
"ersatztv_get_collection_items",
"ersatztv_get_playout_items",
"ersatztv_list_playouts",
"ersatztv_search",
"ersatztv_search_all_items"
];
paged.Select(t => t.Name).OrderBy(n => n, StringComparer.Ordinal)
.ShouldBe(mustDeclarePaging.OrderBy(n => n, StringComparer.Ordinal));
foreach (ToolDefinition tool in paged)
{
ErsatzTV.Mcp/ToolCatalog.cs
+7 -2
View File
@@ -29,9 +29,14 @@ public static class ToolCatalog
Get("ersatztv_list_schedules", "List schedules.", "/api/v1/schedules"),
Get("ersatztv_get_schedule", "Get a schedule by id.", "/api/v1/schedules/{id}", IdPath("Schedule id.")),
Get("ersatztv_get_schedule_items", "Get a schedule's items. Emits the schedule ETag.", "/api/v1/schedules/{id}/items", IdPath("Schedule id.")),
Get("ersatztv_list_playouts", "List playouts.", "/api/v1/playouts"),
Get("ersatztv_list_playouts", "List playouts (paged).", "/api/v1/playouts", [], Page()),
Get("ersatztv_get_playout", "Get a playout by id.", "/api/v1/playouts/{id}", IdPath("Playout id.")),
Get("ersatztv_get_playout_items", "Get upcoming items (and unscheduled gaps) for a playout.", "/api/v1/playouts/{id}/items", IdPath("Playout id.")),
Get(
"ersatztv_get_playout_items",
"Get upcoming items (and unscheduled gaps) for a playout (paged).",
"/api/v1/playouts/{id}/items",
[IdPath("Playout id.")],
Page()),
Get("ersatztv_list_ffmpeg_profiles", "List FFmpeg profiles.", "/api/v1/ffmpeg/profiles"),
Get("ersatztv_get_ffmpeg_profile", "Get an FFmpeg profile by id.", "/api/v1/ffmpeg/profiles/{id}", IdPath("FFmpeg profile id.")),
Get(
docs/decisions/records/api/paging-zero-based.md
+14 -5
View File
@@ -10,9 +10,10 @@ signals: '`pageNum`, `pageSize`, `Math.Clamp(pageSize`, `Skip(pageNum * pageSize
mechanics: '`Math.Max(0, pageNum)` + a per-endpoint upper bound on `pageSize` (`Math.Clamp(pageSize, 1, MaxPageSize)` in most controllers) then `Skip(PageNum * PageSize)` in the handler'
---
Every paged controller on `/api/v1` defaults `pageNum` to `0`, floors it with `Math.Max(0, pageNum)`,
bounds `pageSize` above by a per-endpoint maximum, and passes **both bounded values** to a handler
that skips `PageNum * PageSize`. That makes paging uniformly 0-based, and makes the offset a function
Every paged controller on `/api/v1` defaults `pageNum` to `0`, floors it at 0 (`Math.Max(0, pageNum)`
— `search/all-items` uses `Math.Clamp(pageNum, 0, 2_000_000)` because it additionally needs an upper
bound to keep `pageNum * pageSize` inside `int`), bounds `pageSize` above by a per-endpoint maximum,
and passes **both bounded values** to a handler that skips `PageNum * PageSize`. That makes paging uniformly 0-based, and makes the offset a function
of the effective size rather than the requested one.
The *maximum* is deliberately not uniform and must not be documented as if it were: most reads clamp
@@ -41,8 +42,16 @@ dozen controllers and the SPA, and a 1-based wrapper over a 0-based API would ma
parameter name* mean different things on two surfaces a reader routinely reads together — trading a
documented off-by-one for an undocumented one. Accuracy in the description is the cheaper contract.
**Corollary — ids in paged rows.** A row that names a related entity must expose that entity's id, not
only its display fields. `reset_channel_playout` takes a *channel* id while playout rows exposed only
**Where "0-based" is stated, and where it still isn't.** The MCP tool catalog and these docs say it
explicitly. The generated OpenAPI `pageNum` parameters carry **no description at all** (12 of them),
so a REST consumer reading only `v1.json` still has to infer the base from the default — a real
remaining gap, tracked separately rather than fixed here. Treat "every wrapper says 0-based" as the
target this record sets, not a property already true of the OpenAPI surface.
**Corollary — ids in paged rows.** A row that names a related entity should expose that entity's id,
not only its display fields, wherever a caller is expected to act on that entity. This is a rule about
actionable ids, not an audit result: `PlayoutListItemResponseModel.ScheduleName` still ships without
a schedule id, which is fine while nothing asks a caller to address a schedule from that row. `reset_channel_playout` takes a *channel* id while playout rows exposed only
the playout `id` plus channel name/number; the id spaces overlap numerically, so passing the row's id
silently reset a different channel and returned a plausible 202. List rows gained `channelId` in #297;
#616 added it to `PlayoutResponseModel` (the detail response) and named the trap in the MCP argument
docs/mcp.md
+6 -2
View File
@@ -86,8 +86,12 @@ replace PUT), the tool result appends a `\n[etag: "N"]` marker so an agent can r
### Paging (`api.paging-zero-based`)
The paged tools take `pageNum`/`pageSize` and pass them through unchanged — the MCP layer does **not**
translate them, so they mean exactly what they mean on `/api/v1`:
Tools whose endpoint is paged declare `pageNum`/`pageSize` and pass them through unchanged — the MCP
layer does **not** translate them, so they mean exactly what they mean on `/api/v1`. (A tool that
wraps a paged endpoint *without* declaring these is a bug, not a "no paging needed" signal:
`ToolArgumentValidator` rejects undeclared arguments, so the caller is hard-capped at the first page
with no way to ask for more. `ersatztv_list_playouts` and `ersatztv_get_playout_items` were capped
that way until #616. `ToolCatalogTests` pins the expected set.)
- **`pageNum` is 0-based.** The first page is `0`. Starting at `1` silently skips a page and returns a
short set with no error, which is easy to misread as missing data (ersatztv#616 — the catalog used
web/src/screens/SchedulesScreen.test.tsx
+16
View File
@@ -170,6 +170,22 @@ describe('SchedulesScreen — load', () => {
expect(screen.getByLabelText('Schedule lineup')).toBeTruthy();
});
it('requests the rerun-collection picker from page 0, not page 1 (#616)', async () => {
const handle = await renderReady();
const rerunRequests = handle.requests.filter((r) => r.url.startsWith('/api/v1/rerun-collections'));
expect(rerunRequests.length).toBeGreaterThan(0);
// pageNum is 0-based (api.paging-zero-based). This asked for page 1, and because pageSize is
// clamped to 100 server-side that skipped the first 100 rows — so the picker showed nothing at
// all for the ordinary case of <=100 rerun collections. Assert the offset, not just the URL
// shape: `pageNum=1` here is a silent empty picker, never an error.
for (const request of rerunRequests) {
const pageNum = new URL(request.url, 'http://localhost').searchParams.get('pageNum');
expect(pageNum).toBe('0');
}
});
it('renders the "Active schedule" selector non-full-width so it cannot overflow the header (#463)', async () => {
await renderReady();
const select = screen.getByLabelText('Active schedule');
web/src/screens/SchedulesScreen.tsx
+4 -1
View File
@@ -49,7 +49,10 @@ type BootState =
async function loadPickerData(): Promise<SchedulePickerData> {
const [rerun, playlistGroups, watermarks, graphicsElements, languages, pre, mid, post, tail, fallback] =
await Promise.all([
getRerunCollections({ pageNum: 1, pageSize: 1000 }),
// pageNum is 0-BASED (api.paging-zero-based). This asked for page 1, which skipped the first
// page entirely: pageSize is clamped to 100 server-side, so the picker was served rows 101+
// and showed nothing at all for the normal case of <=100 rerun collections (ersatztv#616).
getRerunCollections({ pageNum: 0, pageSize: 1000 }),
getPlaylistGroups(),
getWatermarks(),
getGraphicsElements(),