fix(754,757): declare graphicsElementIds + padToNearestMinute, and pin every MCP tool to its OpenAPI contract #760

Merged

timothy merged 4 commits from fix/754-mcp-graphics-element-ids into main 2026-08-06 22:42:37 +02:00

Conversation 1 Commits 4 Files Changed 6

+461 -12

timothy commented 2026-08-06 21:52:17 +02:00

Owner

Copy Link

Copy Source

What

Two instances of one defect, plus the guard that makes the class non-recurring.

ToolCatalog.ChannelFields() declared 27 of UpdateChannelRequest's 28 properties. The missing
one was graphicsElementIds. PUT /api/v1/channels/{id} is a full replace and the tool's own
description says "send the full desired state" — which the schema could not express. An agent that
faithfully GET-edit-PUT a channel detached every graphics element, including the built-in On
Now/Next overlay, with a 200 and no error. Nothing surfaced until the overlay stopped rendering.

Fixing that one field would have left the mechanism intact — and the mechanism had already produced
a second live instance that #754 did not know about: ScheduleFlags() omitted padToNearestMinute,
which UpdateProgramScheduleHandler writes unconditionally, so ersatztv_update_schedule silently
cleared a configured pad the same way.

Live proof (ersatztv-test), with the bug as its own positive control

step	graphicsElementIds
before	[1]
update omitting the field — 200, isError=false	[] ← the bug
update with the field	[1] restored

The guard (this is the actual fix)

Two tests in ToolCatalogTests, asserting against the generated v1.json (linked into the test
project — no project reference to the ASP.NET host):

• every POST/PUT/PATCH tool declares exactly its endpoint's request-body fields, each with a
  matching type;
• every tool, read and write, declares exactly its endpoint's query parameters (#757, folded
  in at the user's request). An omitted one is unreachable, not merely undocumented, because
  ToolArgumentValidator rejects undeclared arguments. ersatztv_list_playouts had lost its
  channel-name query filter and ersatztv_get_playout_items its showFiller; the guard found
  exactly those two and nothing else.

Body derivation mirrors ErsatzTvApiClient exactly, so the guard cannot disagree with the routing it
guards. The query test accumulates mismatches and asserts once — failing on the first would invite
fixing one tool at a time, which is how the twin in this very issue stayed hidden.

Verified by mutation, not assumed. Dropping either field, retyping either, drifting an array's
element type, dropping either query parameter, adding a phantom query parameter, and deleting the
copied spec each turn the suite red and name the fault.

Review

Three adversarial rounds (cold, worktree-isolated, review-only). Round 1 BLOCKED: the schedule
twin got the schema fix but not its warning; my docs/mcp.md claim that "every write is a full
replace" was false for four tools; and the guard compared names only — the reviewer proved by
mutation that retyping padToNearestMinute to a string stayed green. Round 2 BLOCKED: I had filed
ersatztv_update_playout as leave-unchanged when it is mixed (DailyRebuildTime is nulled
unconditionally), so the paragraph written to prevent the #754 shape would have caused it. Round 3
MERGEABLE, re-confirmed on the merging sha 29abd470e.

Recorded rather than fixed, in the decision record's scope section: the type comparison is lossy at
the catalog's ceiling (objects collapse to object, enums to string), the >1 type-union assertion
is deliberate but unexercised, and the api-docs freshness conjunction covers a DTO's own properties
only, not nested models.

Follow-ups filed

• #758 — GetPagedPlayouts counts before both the query and Channel != null filters, so a
  filtered page reports the unfiltered total. Pre-existing; #757 is what exposes it. Same family as
  #690.
• #759 — the query guard silently ignores a $ref'd parameter where the body guard fails loudly.
  Dormant (zero such parameters today), filed with that evidence.

Verification

61/61 MCP tests · 441 script tests · dotnet format exit 0 · no BOMs · decisions-validate: OK ·
live E2E on ersatztv-test.

Caveat, stated rather than buried: the padToNearestMinute half has no live round-trip. The
deployed test image predates that field entirely (its /openapi/v1.json lacks it), so the API ignored
it. Environment limitation, not a defect — the field is real on main, which is what the guard
asserts against.

fixes #754
fixes #757

## What Two instances of one defect, plus the guard that makes the class non-recurring. `ToolCatalog.ChannelFields()` declared **27 of `UpdateChannelRequest`'s 28** properties. The missing one was `graphicsElementIds`. `PUT /api/v1/channels/{id}` is a **full replace** and the tool's own description says *"send the full desired state"* — which the schema could not express. An agent that faithfully GET-edit-PUT a channel **detached every graphics element**, including the built-in On Now/Next overlay, with a `200` and no error. Nothing surfaced until the overlay stopped rendering. Fixing that one field would have left the mechanism intact — and the mechanism had **already produced a second live instance** that #754 did not know about: `ScheduleFlags()` omitted `padToNearestMinute`, which `UpdateProgramScheduleHandler` writes unconditionally, so `ersatztv_update_schedule` silently cleared a configured pad the same way. ## Live proof (ersatztv-test), with the bug as its own positive control | step | `graphicsElementIds` | |---|---| | before | `[1]` | | update **omitting** the field — `200`, `isError=false` | `[]` ← the bug | | update **with** the field | `[1]` restored | ## The guard (this is the actual fix) Two tests in `ToolCatalogTests`, asserting against the generated `v1.json` (linked into the test project — no project reference to the ASP.NET host): - every **POST/PUT/PATCH** tool declares exactly its endpoint's request-body fields, **each with a matching type**; - every tool, **read and write**, declares exactly its endpoint's **query parameters** (#757, folded in at the user's request). An omitted one is *unreachable*, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments. `ersatztv_list_playouts` had lost its channel-name `query` filter and `ersatztv_get_playout_items` its `showFiller`; the guard found exactly those two and nothing else. Body derivation mirrors `ErsatzTvApiClient` exactly, so the guard cannot disagree with the routing it guards. The query test **accumulates** mismatches and asserts once — failing on the first would invite fixing one tool at a time, which is how the twin in this very issue stayed hidden. **Verified by mutation, not assumed.** Dropping either field, retyping either, drifting an array's element type, dropping either query parameter, adding a phantom query parameter, and deleting the copied spec each turn the suite red and name the fault. ## Review Three adversarial rounds (cold, worktree-isolated, review-only). Round 1 **BLOCKED**: the schedule twin got the schema fix but not its warning; my `docs/mcp.md` claim that "every write is a full replace" was false for four tools; and the guard compared **names only** — the reviewer proved by mutation that retyping `padToNearestMinute` to a string stayed green. Round 2 **BLOCKED**: I had filed `ersatztv_update_playout` as leave-unchanged when it is **mixed** (`DailyRebuildTime` is nulled unconditionally), so the paragraph written to prevent the #754 shape would have caused it. Round 3 **MERGEABLE**, re-confirmed on the merging sha `29abd470e`. Recorded rather than fixed, in the decision record's scope section: the type comparison is lossy at the catalog's ceiling (objects collapse to `object`, enums to `string`), the `>1` type-union assertion is deliberate but unexercised, and the `api-docs` freshness conjunction covers a DTO's own properties only, not nested models. ## Follow-ups filed - **#758** — `GetPagedPlayouts` counts before both the `query` and `Channel != null` filters, so a filtered page reports the unfiltered total. Pre-existing; #757 is what *exposes* it. Same family as #690. - **#759** — the query guard silently ignores a `$ref`'d parameter where the body guard fails loudly. Dormant (zero such parameters today), filed with that evidence. ## Verification 61/61 MCP tests · 441 script tests · `dotnet format` exit 0 · no BOMs · `decisions-validate: OK` · live E2E on ersatztv-test. **Caveat, stated rather than buried:** the `padToNearestMinute` half has **no** live round-trip. The deployed test image predates that field entirely (its `/openapi/v1.json` lacks it), so the API ignored it. Environment limitation, not a defect — the field is real on `main`, which is what the guard asserts against. fixes #754 fixes #757

timothy added 4 commits 2026-08-06 21:52:17 +02:00

fix(754): declare graphicsElementIds on ersatztv_update_channel, and pin every MCP write tool to its OpenAPI request body ... 607c3a4cf8

`ToolCatalog.ChannelFields()` declared 27 of `UpdateChannelRequest`'s 28
properties. The missing one was `graphicsElementIds`. Because
`PUT /api/v1/channels/{id}` is a full replace and the tool's own description
says "send the full desired state", an agent that faithfully GET-edit-PUT a
channel detached every attached graphics element -- including the built-in On
Now/Next overlay -- with a 200 and no error, observable only as missing pixels
at the next transition.

It is declared on the update tool only: `CreateChannelRequest` has no such
property and the tool schemas are `additionalProperties:false`, so putting it in
the shared helper would make every create call send an unknown property.

Fixing that one field would have left the mechanism intact, and the mechanism
had already produced a second live instance: `ScheduleFlags()` omitted
`padToNearestMinute`, which both schedule requests carry and
`UpdateProgramScheduleHandler` writes unconditionally, so
`ersatztv_update_schedule` silently cleared a configured pad the same way. That
one belongs in the shared helper, and is added there.

So the guard is the fix: `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`
asserts every POST/PUT/PATCH tool's body set against the generated `v1.json`
(linked into the test project), deriving the body exactly as `ErsatzTvApiClient`
does so the guard cannot disagree with the routing it guards. The covered tool
set is pinned by name rather than only filtered, and a missing or non-`$ref`
spec fails loudly instead of comparing against an empty set. Verified by
mutation: dropping either field, or the copied spec, turns the suite red and
names the fault.

fixes #754

Decisions-Edit: yes

fix(754): review round 1 — warn on the schedule twin, scope the full-replace claim, make the guard type-aware ... b9e63980ac

Independent review found three defects in the first pass.

1. HIGH. `ersatztv_update_schedule` got the schema fix but not the guidance.
   Declaring `padToNearestMinute` makes it EXPRESSIBLE, not SENT: it is
   optional, and the handler writes it unconditionally, so a rename that omits
   it still clears a configured pad. The channel tool carries exactly that
   warning and the schedule tool -- this PR's own second instance -- did not.
   It now does.

2. MEDIUM-HIGH. docs/mcp.md asserted "every write here is a full replace" one
   paragraph after describing `add_collection_items` as an idempotent ADD. It
   is false for four tools (add-collection-items, update_collection,
   update_playout.scheduleFile, enable_jellyfin_library_sync), and an agent
   believing it would assume add-items REPLACES membership -- a new bug in the
   opposite direction. The claim is now scoped, in the doc and in the decision
   record's `rule:` frontmatter, which is the string the catalog and MemPalace
   mirror.

3. MEDIUM. The guard compared NAMES only, so type drift was invisible: retyping
   `padToNearestMinute` to a string left the suite green, and that is the one
   field this PR adds. It now compares name AND type, normalizing the
   generator's ["null", T] and $ref forms (enum -> string, model -> object) and
   resolving arrays to their element type. Verified by mutation: the reviewer's
   exact Int->Str mutation, an array element-type drift, and an unrelated
   Int->Str all go red; the array-of-$ref shape
   (ReplaceRemoteLibraryPreferencesRequest.libraries) is resolved recursively
   rather than assumed scalar.

The record now also states the guard's SCOPE rather than implying more: query
parameters are uncovered on every verb, DELETE is uncovered, and two read tools
already drift (#757). It also notes the guard is a two-job conjunction with the
`api-docs` freshness job, not self-contained.

Decisions-Edit: yes

fix(757): guard query parameters on every tool, and reclassify update_playout as mixed ... b1c2b9e18c

Folds ersatztv#757 into this PR at the user's request, and fixes review round 2.

Query-parameter parity (#757). A second test compares every tool's routed
QueryParameters against the spec's parameters[in=query] -- reads included,
because the drift was entirely on reads. `ersatztv_list_playouts` now declares
`query` (case-insensitive substring on the CHANNEL name, per
GetPagedPlayoutsHandler) and `ersatztv_get_playout_items` declares `showFiller`
(per GetFuturePlayoutItemsByIdHandler's FillerKind.None filter). Those two were
the complete drift set -- the guard reported them, not a hand audit.

The test ACCUMULATES mismatches and asserts once, so one run reports the whole
drift set. Failing on the first would hand back one tool at a time and invite
fixing them one at a time, which is precisely how the #754 twin stayed hidden.

Verified by mutation: dropping either newly declared parameter goes red, and a
phantom query arg on a write tool goes red (the reviewer's round-1 finding 5,
which was green before this).

Review round 2 (CONFIRMED at UpdatePlayoutHandler.cs:31). My round-1 docs fix
sorted the write tools into full-replace vs leave-unchanged and filed
`ersatztv_update_playout` under leave-unchanged. It is MIXED: `scheduleFile` is
leave-unchanged, but `DailyRebuildTime` is set to null unconditionally before a
supplied value is re-applied, so setting scheduleFile while omitting
dailyRebuildTime silently clears the daily reset. The paragraph written to
prevent the #754 shape would have caused it on the one tool with mixed
semantics. Now called out explicitly as the easy one to get wrong.

Also from round 2, recorded rather than fixed: the type comparison is lossy at
the catalog's ceiling (every object component collapses to `object`, every enum
to `string`), so a swapped model or enum is invisible -- confirmed green when
`logo` is repointed at an unrelated model. Deeper comparison would assert a
distinction no tool schema carries, and an opaque object is copied through from
a GET verbatim, so it cannot cause a silent clear. The >1 type-union assertion
is deliberate but unexercised. The api-docs freshness conjunction covers a DTO's
own properties only, not nested models outside ^ErsatzTV/Controllers/Api/.

fixes #757

Decisions-Edit: yes

docs(757): enumerate all seven filler kinds, and record why the two query tests compose ... 29abd470e4

Round-3 review found one statement in the PR that was not true of the code:
showFiller's description named four of FillerKind's seven non-None values
(pre/mid/post-roll, tail), omitting Fallback, GuideMode and DecoDefault. The
handler filters `ShowFiller || FillerKind == None`, so it excludes ALL of them,
and an agent debugging a missing Fallback or Deco item would not have found it
in that list. Verified against Core/Domain/Filler/FillerKind.cs.

Also records the anti-vacuity argument the reviewer surfaced, which was the
strongest one in this PR and was going unstated: the pre-existing
Every_Query_Parameter_Should_Be_A_Declared_Property filters on
`QueryParameters is { Count: > 0 }` -- a filter on the very property it asserts
-- so a tool that lost its query parameters escaped it entirely. That is
precisely how list_playouts and get_playout_items hid. The new unfiltered test
reports them as unreachable and the old one checks the routed-vs-declared half;
neither subsumes the other, which is why the inner duplicate was removed rather
than kept.

Decisions-Edit: yes

timothy commented 2026-08-06 21:53:05 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 29abd47

3 adversarial rounds (2 BLOCKED then fixed); re-confirmed on merging sha. Mutation-verified guard; live E2E on ersatztv-test. Follow-ups #758, #759.

Review-verdict: MERGEABLE @ 29abd47 3 adversarial rounds (2 BLOCKED then fixed); re-confirmed on merging sha. Mutation-verified guard; live E2E on ersatztv-test. Follow-ups #758, #759.

timothy merged commit 9881d1ff81 into main 2026-08-06 22:42:37 +02:00

timothy deleted branch fix/754-mcp-graphics-element-ids 2026-08-06 22:42:38 +02:00

timothy referenced this pull request 2026-08-06 22:43:25 +02:00

MCP `ersatztv_update_channel` omits `graphicsElementIds` — cannot set the On Now/Next overlay, and silently strips it #754

timothy referenced this pull request 2026-08-06 22:43:49 +02:00

MCP tool catalog: query parameters are unguarded — list_playouts omits `query`, get_playout_items omits `showFiller` #757

timothy referenced this issue from a commit 2026-08-10 19:43:18 +02:00

fix(751): rebase onto #760, close the fail-OPEN twin, and retire four claims that had rotted

timothy referenced this pull request 2026-08-10 21:09:35 +02:00

fix(751): a stray expression delimiter in a COMMENT killed the verdict gate — plus the two nil-slice twins and a dead-code guard it hid #764

timothy referenced this pull request 2026-08-10 21:22:10 +02:00

fix(751): a stray expression delimiter in a COMMENT killed the verdict gate — plus the two nil-slice twins and a dead-code guard it hid #764

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