fix(633): document the 0-based paging contract on the OpenAPI parameters #646

Merged

timothy merged 3 commits from fix/633-openapi-paging-descriptions into main 2026-07-26 13:06:32 +02:00

Conversation 1 Commits 3 Files Changed 13

+312 -31

timothy commented 2026-07-26 11:35:21 +02:00

Owner

Copy Link

Copy Source

fixes #633

What

api.paging-zero-based says pageNum is 0-based across /api/v1 and every wrapper of it. That was true of the MCP tool catalog and the docs, and not true of the generated OpenAPI document: all 24 paging parameters across the 12 paged operations were emitted with no description at all. A consumer reading only v1.json — the intended contract, and what generated clients surface to their users — had to infer the base from default: 0. That is the same inference that cost #487 a verification pass on the MCP side, where the description was present but wrong.

Each [FromQuery] paging parameter now carries [Description] (System.ComponentModel) — the mechanism parentId already used in ImagesController — and v1.json is regenerated.

The caps are per-endpoint, and the descriptions say so

The record explicitly forbids documenting the cap as one number, because it genuinely differs:

Endpoint	default	cap
/channels/auto-tune/members	required	200 (and pageSize <= 0 falls back to 100, not clamped up to 1)
/search	50	100
/search/all-items	500	1000 (and pageNum is clamped above at 2,000,000)
the other nine	100	100

Every pageSize description states its own endpoint's cap and that the offset derives from the effective (capped) size, so an over-large pageSize narrows the page instead of widening the offset.

The test pins the set, not just the parameters

OpenApiPagingContractTests runs against the in-process generated document (via the existing GeneratedOpenApiDocument helper), so it goes red without needing v1.json regenerated first.

It names the expected set of 12 operations rather than only filtering for parameters called pageNum. A filter cannot see an endpoint that should page and doesn't — which is exactly how two MCP tools escaped the equivalent check until #616. Set equality is asserted in both directions: a new paged endpoint fails until it is added with descriptions, and an endpoint that quietly drops paging fails too. expectedCaps carries the same guard-the-guard, so adding an operation without its cap can't skip that assertion.

Residual gap, stated rather than papered over: a brand-new endpoint that returns a page while declaring no paging parameters at all under any name is invisible to any signal in the document. That stays a review concern.

Verification

• ErsatzTV.Tests full suite: 1905 passed, 0 failed. Format gate clean (--folder --include on the changed set), no BOMs.
• Mutation-verified three ways: dropping one [Description] reddens the description test; making one endpoint stop exposing pageNum/pageSize under those names reddens the set-equality test; and making /logs claim "capped at 1000" reddens the cap test.
• The TypeScript client (web/src/api/generated/v1.d.ts) and docs/endpoint-index.md are unchanged by design — the generator covers DTOs, not query parameters, and the index carries summaries. Confirmed by regeneration, not assumed; the blocking api-docs job would catch it either way.

Independent review

Codex (different model family), cold, review-only. First pass BLOCKED with three findings, two of them real defects:

1. search/all-items documented only its lower pageNum clamp, so the contract looked unbounded — a client sending pageNum=int.MaxValue is silently served page 2,000,000. Now stated, and flagged as the exception it is.
2. The cap assertion could not fail in the direction that mattered. ShouldContain("capped at 100") is satisfied by "capped at 1000", so a cap-100 endpoint claiming 1000 passed — precisely the defect the test was added to catch. A test that cannot fail on its own subject is worse than none. Now matched as a whole token, and mutation-verified.
3. Description used First, throwing "Sequence contains no matching element" — naming neither endpoint nor parameter. Fails informatively now.

The same pass independently confirmed 12 is the complete paged set, that every other cap matches its controller's clamp, that the attributes are runtime-inert (including no System.ComponentModel / DataAnnotations ambiguity), and that the unchanged generated artifacts are correct.

It took three rounds on that one assertion, and each round found the previous fix's blind spot rather than a new mistake — the failure mode was consistently "the new check tests presence of the right thing, not absence of the wrong thing." Round 2's whole-token match still passed a description naming a wrong cap elsewhere in the sentence alongside the right one; round 3 enumerates every capped at <n> and requires the set to be exactly the right number. Each round is mutation-verified against the specific case the reviewer constructed. Round 2's commit message had called its guard "exact" — it wasn't, and that overstatement is corrected in round 3's message rather than left in the history unremarked.

Known bound, judged non-blocking by both sides: a description expressing a cap in different words ("maximum of 1000" rather than "capped at 1000") escapes the check. None of the twelve does, and widening the grammar to chase it is scope creep.

Verification caveat, stated rather than buried: the reviewer could not execute the test suite in either round — dotnet test exited 82 (MSBuild denied temp-directory creation) and a direct VSTest invocation could not bind its IPC socket — so it flagged the 1905/0 figure as unverified rather than trusting it. That number is from my own runs in this worktree; its static verification (the v1.json diff is exactly one changed leaf, git diff-tree --check clean) did pass, and CI is the independent confirmation.

Docs

• docs/api-conventions.md — the paged-GET exemplar now states the [Description] mechanism, that the cap is per-endpoint, and that OpenApiPagingContractTests will fail a new paged endpoint that skips it.
• docs/decisions/records/api/paging-zero-based.md — drops the "OpenAPI still doesn't" caveat from the title and rewrites the "where it still isn't" paragraph, since that gap is what this PR closes. Rationale-prose edit, so the commits carry a Decisions-Edit: yes trailer. The rule: field is unchanged, so the generated catalog is unchanged.

fixes #633 ## What `api.paging-zero-based` says `pageNum` is 0-based across `/api/v1` **and every wrapper of it**. That was true of the MCP tool catalog and the docs, and not true of the generated OpenAPI document: all **24** paging parameters across the **12** paged operations were emitted with no `description` at all. A consumer reading only `v1.json` — the intended contract, and what generated clients surface to their users — had to infer the base from `default: 0`. That is the same inference that cost #487 a verification pass on the MCP side, where the description was present but *wrong*. Each `[FromQuery]` paging parameter now carries `[Description]` (`System.ComponentModel`) — the mechanism `parentId` already used in `ImagesController` — and `v1.json` is regenerated. ## The caps are per-endpoint, and the descriptions say so The record explicitly forbids documenting the cap as one number, because it genuinely differs: | Endpoint | default | cap | |---|---|---| | `/channels/auto-tune/members` | *required* | **200** (and `pageSize <= 0` falls back to 100, not clamped up to 1) | | `/search` | 50 | 100 | | `/search/all-items` | 500 | **1000** (and `pageNum` is clamped **above** at 2,000,000) | | the other nine | 100 | 100 | Every `pageSize` description states its own endpoint's cap and that the offset derives from the **effective** (capped) size, so an over-large `pageSize` narrows the page instead of widening the offset. ## The test pins the set, not just the parameters `OpenApiPagingContractTests` runs against the in-process generated document (via the existing `GeneratedOpenApiDocument` helper), so it goes red without needing `v1.json` regenerated first. It **names the expected set of 12 operations** rather than only filtering for parameters called `pageNum`. A filter cannot see an endpoint that *should* page and doesn't — which is exactly how two MCP tools escaped the equivalent check until #616. Set equality is asserted in both directions: a new paged endpoint fails until it is added *with* descriptions, and an endpoint that quietly drops paging fails too. `expectedCaps` carries the same guard-the-guard, so adding an operation without its cap can't skip that assertion. Residual gap, stated rather than papered over: a brand-new endpoint that returns a page while declaring no paging parameters at all under any name is invisible to any signal in the document. That stays a review concern. ## Verification - `ErsatzTV.Tests` full suite: **1905 passed**, 0 failed. Format gate clean (`--folder --include` on the changed set), no BOMs. - **Mutation-verified three ways**: dropping one `[Description]` reddens the description test; making one endpoint stop exposing `pageNum`/`pageSize` under those names reddens the set-equality test; and making `/logs` claim "capped at 1000" reddens the cap test. - The TypeScript client (`web/src/api/generated/v1.d.ts`) and `docs/endpoint-index.md` are **unchanged by design** — the generator covers DTOs, not query parameters, and the index carries summaries. Confirmed by regeneration, not assumed; the blocking `api-docs` job would catch it either way. ## Independent review Codex (different model family), cold, review-only. First pass **BLOCKED** with three findings, two of them real defects: 1. `search/all-items` documented only its lower `pageNum` clamp, so the contract looked unbounded — a client sending `pageNum=int.MaxValue` is silently served page 2,000,000. Now stated, and flagged as the exception it is. 2. **The cap assertion could not fail in the direction that mattered.** `ShouldContain("capped at 100")` is satisfied by `"capped at 1000"`, so a cap-100 endpoint claiming 1000 passed — precisely the defect the test was added to catch. A test that cannot fail on its own subject is worse than none. Now matched as a whole token, and mutation-verified. 3. `Description` used `First`, throwing "Sequence contains no matching element" — naming neither endpoint nor parameter. Fails informatively now. The same pass independently confirmed 12 is the complete paged set, that every other cap matches its controller's clamp, that the attributes are runtime-inert (including no `System.ComponentModel` / `DataAnnotations` ambiguity), and that the unchanged generated artifacts are correct. **It took three rounds on that one assertion**, and each round found the previous fix's blind spot rather than a new mistake — the failure mode was consistently *"the new check tests presence of the right thing, not absence of the wrong thing."* Round 2's whole-token match still passed a description naming a wrong cap elsewhere in the sentence alongside the right one; round 3 enumerates every `capped at <n>` and requires the set to be exactly the right number. Each round is mutation-verified against the specific case the reviewer constructed. Round 2's commit message had called its guard "exact" — it wasn't, and that overstatement is corrected in round 3's message rather than left in the history unremarked. Known bound, judged non-blocking by both sides: a description expressing a cap in different words ("maximum of 1000" rather than "capped at 1000") escapes the check. None of the twelve does, and widening the grammar to chase it is scope creep. **Verification caveat, stated rather than buried:** the reviewer could not execute the test suite in either round — `dotnet test` exited 82 (MSBuild denied temp-directory creation) and a direct VSTest invocation could not bind its IPC socket — so it flagged the 1905/0 figure as unverified rather than trusting it. That number is from my own runs in this worktree; its static verification (the `v1.json` diff is exactly one changed leaf, `git diff-tree --check` clean) did pass, and CI is the independent confirmation. ## Docs - `docs/api-conventions.md` — the paged-GET exemplar now states the `[Description]` mechanism, that the cap is per-endpoint, and that `OpenApiPagingContractTests` will fail a new paged endpoint that skips it. - `docs/decisions/records/api/paging-zero-based.md` — drops the "OpenAPI still doesn't" caveat from the title and rewrites the "where it still isn't" paragraph, since that gap is what this PR closes. Rationale-prose edit, so the commits carry a `Decisions-Edit: yes` trailer. The `rule:` field is unchanged, so the generated catalog is unchanged.

timothy added 3 commits 2026-07-26 11:35:21 +02:00

fix(633): document the 0-based paging contract on the OpenAPI parameters ... 214fad2dcd

`api.paging-zero-based` says `pageNum` is 0-based across `/api/v1` and every wrapper
of it. That was true of the MCP tool catalog and the docs, and not true of the
generated OpenAPI document: all 24 paging parameters across the 12 paged operations
were emitted with no `description` at all, so a consumer reading only `v1.json` — the
intended contract, and what generated clients surface to their users — had to infer
the base from `default: 0`. That is the same inference that cost #487 a verification
pass on the MCP side, where the description was present but wrong.

Annotates each `[FromQuery]` paging parameter with `[Description]`
(`System.ComponentModel`), the mechanism `parentId` already used in ImagesController,
and regenerates `v1.json`. `pageSize` states the endpoint's OWN cap, because the caps
genuinely differ — 100 typical, 200 auto-tune members, 1000 search/all-items — and the
record forbids documenting one global number; it also states that the offset derives
from the effective (capped) size, so an over-large `pageSize` narrows the page instead
of widening the offset.

The generated TypeScript client covers DTOs only, not query parameters, so it is
unchanged; `endpoint-index.md` carries summaries, not parameter descriptions, so it is
unchanged too.

Pinned by OpenApiPagingContractTests against the in-process generated document. The
test NAMES the expected set of 12 paged operations rather than only filtering for
parameters called `pageNum`: a filter cannot see an endpoint that should page and
doesn't, which is exactly how two MCP tools escaped the equivalent check in #616. Set
equality is asserted in both directions, and the caps are pinned per endpoint so a
description naming the wrong cap fails — a wrong justification outlives a wrong line.

Mutation-verified both ways: dropping one `[Description]` reddens the description test,
and making one endpoint stop exposing `pageNum`/`pageSize` under those names reddens
the set-equality test.

Refs #633

Decisions-Edit: yes

fix(633): review fixes — the all-items upper clamp, and a cap test that couldn't fail ... 23791c1bbb

Independent review (Codex) found three, two of them real defects rather than polish.

1. `search/all-items` is the one paged endpoint that clamps `pageNum` ABOVE as well as
   below — `Math.Clamp(pageNum, 0, MaxAllItemsPageNum)`, 2,000,000, so pageNum*pageSize
   cannot overflow int into a 500. The description documented only the lower clamp, so
   the published contract looked unbounded: a client sending pageNum=int.MaxValue is
   silently served page 2,000,000. Now stated, and called out as the exception it is.

2. The cap assertion could not fail in the direction that matters. `ShouldContain(
   "capped at 100")` is satisfied by the string "capped at 1000", so a cap-100 endpoint
   whose description claimed 1000 passed — precisely the wrong-cap defect the test was
   added to catch, and a test that cannot fail on its own subject is worse than none.
   Matched as a whole token instead, and mutation-verified: making /logs claim 1000 now
   reddens it, where before it stayed green.

3. `Description` used `First`, so a missing parameter threw "Sequence contains no
   matching element" — naming neither endpoint nor parameter, and reading as a broken
   test rather than the contract violation it is. Fails informatively now.

The review confirmed independently that 12 is the complete paged set, that every other
cap matches its controller, that the attributes are runtime-inert, and that the
unchanged TypeScript client and endpoint index are correct rather than a missed regen.

Refs #633

Decisions-Edit: yes

fix(633): assert the cap set, not the presence of one true cap claim ... 33e9abdd20

Round-3 review finding, and a correction to what the previous commit claimed.

That commit said the regex matched the cap "as a whole token" and called the result
"exact". The whole-token part was true and did fix the 100-within-1000 substring hole.
"Exact" was not: `capped at 100(?!\d)` asks only whether a correct claim is PRESENT,
which is not the same as asking whether an incorrect one is ABSENT. A description
reading "not capped at 1000 for this endpoint; capped at 100 …" satisfied it while
publishing a wrong number to every consumer.

Enumerate every `capped at <n>` in the description instead and require the set to be
exactly one number, the right one. Mutation-verified on the constructed case: /logs
naming both 1000 and 100 now reddens the test, where it passed under the previous form.

This is the third round on this one assertion, and each round found the previous fix's
blind spot rather than a fresh mistake — the failure mode was consistently "the new
check tests presence of the right thing, not absence of the wrong thing."

Note on verification: the reviewer could not run the suite (its sandbox could not
create a temp dir, and a direct VSTest invocation could not bind its IPC socket), so it
explicitly flagged the 1905/0 result as unverified rather than trusting it. That figure
comes from my own run in this worktree, re-run after this change, and CI is the
independent confirmation.

Refs #633

Decisions-Edit: yes

timothy commented 2026-07-26 11:35:37 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 33e9abd

Independent Codex review over three rounds; final pass on this head found nothing above Nit. Two real defects were caught and fixed: the search/all-items upper pageNum clamp was undocumented, and the cap assertion could not fail on its own subject. Reviewer could not execute the suite (sandbox); the 1905/0 result is from my run, CI is the independent check.

Review-verdict: MERGEABLE @ 33e9abd Independent Codex review over three rounds; final pass on this head found nothing above Nit. Two real defects were caught and fixed: the search/all-items upper pageNum clamp was undocumented, and the cap assertion could not fail on its own subject. Reviewer could not execute the suite (sandbox); the 1905/0 result is from my run, CI is the independent check.

timothy merged commit 4fd806bebf into main 2026-07-26 13:06:32 +02:00

timothy deleted branch fix/633-openapi-paging-descriptions 2026-07-26 13:06:33 +02:00

timothy referenced this issue from a commit 2026-07-26 13:06:33 +02:00

Merge pull request 'fix(633): document the 0-based paging contract on the OpenAPI parameters' (#646) from fix/633-openapi-paging-descriptions into main

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

OpenAPI `pageNum` parameters carry no description, so the 0-based contract is invisible to REST consumers #633

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#646