From 607c3a4cf88745c204045bd0a186851cf57eca28 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 21:21:53 +0200 Subject: [PATCH 1/4] fix(754): declare graphicsElementIds on ersatztv_update_channel, and pin every MCP write tool to its OpenAPI request body `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 --- ErsatzTV.Mcp.Tests/ErsatzTV.Mcp.Tests.csproj | 12 ++ ErsatzTV.Mcp.Tests/ToolCatalogTests.cs | 153 +++++++++++++++++- ErsatzTV.Mcp/ToolCatalog.cs | 24 ++- docs/decisions/README.md | 1 + .../records/mcp/tool-schema-openapi-parity.md | 55 +++++++ docs/mcp.md | 23 ++- 6 files changed, 259 insertions(+), 9 deletions(-) create mode 100644 docs/decisions/records/mcp/tool-schema-openapi-parity.md diff --git a/ErsatzTV.Mcp.Tests/ErsatzTV.Mcp.Tests.csproj b/ErsatzTV.Mcp.Tests/ErsatzTV.Mcp.Tests.csproj index 7bd2aef80..c7cb34f26 100644 --- a/ErsatzTV.Mcp.Tests/ErsatzTV.Mcp.Tests.csproj +++ b/ErsatzTV.Mcp.Tests/ErsatzTV.Mcp.Tests.csproj @@ -21,4 +21,16 @@ + + + + + diff --git a/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs b/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs index d220511f3..edb590bef 100644 --- a/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs +++ b/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs @@ -77,9 +77,22 @@ public class ToolCatalogTests // Enums must NOT be forced required (they have server-side defaults). createRequired.ShouldNotContain("streamingMode"); - // Update carries the same body fields plus the route id. - update.InputSchema.RootElement.GetProperty("properties").TryGetProperty("id", out _).ShouldBeTrue(); - update.InputSchema.RootElement.GetProperty("properties").TryGetProperty("showInEpg", out _).ShouldBeTrue(); + // Update carries the create body fields plus the route id... + JsonElement updateProps = update.InputSchema.RootElement.GetProperty("properties"); + updateProps.TryGetProperty("id", out _).ShouldBeTrue(); + updateProps.TryGetProperty("showInEpg", out _).ShouldBeTrue(); + + // ...plus graphicsElementIds, which is on UpdateChannelRequest only. PUT is a full replace, so + // while the tool could not express this field an agent following the tool's own "send the full + // desired state" instruction silently detached every graphics element (issue #754). + updateProps.TryGetProperty("graphicsElementIds", out JsonElement graphicsElementIds).ShouldBeTrue(); + graphicsElementIds.GetProperty("type").GetString().ShouldBe("array"); + graphicsElementIds.GetProperty("items").GetProperty("type").GetString().ShouldBe("integer"); + + // Create must NOT send it: CreateChannelRequest has no such property, and the tool schema is + // additionalProperties:false. This is why it is declared on the update tool rather than in the + // shared ChannelFields(). + createProps.TryGetProperty("graphicsElementIds", out _).ShouldBeFalse(); } [Test] @@ -256,4 +269,138 @@ public class ToolCatalogTests tool.QueryParameters.ShouldNotBeNull(); tool.QueryParameters!.ShouldContain("deep"); } + + // #754: ToolCatalog declared 27 of UpdateChannelRequest's 28 properties. The missing one was + // graphicsElementIds, and because PUT /api/v1/channels/{id} is a FULL REPLACE the omission was not + // merely "one field you cannot set" — an agent that GET-edit-PUT the channel, exactly as the tool's + // description tells it to, detached every graphics element (including the On Now/Next overlay) with + // a 200 and no error. The same shape was live on ersatztv_update_schedule, which omitted + // padToNearestMinute and silently cleared a configured pad. + // + // Neither is fixable by counting fields once: the defect is that nothing tied the tool schema to the + // contract it wraps. So this test asserts the tie for EVERY write tool against the generated OpenAPI + // document (the actual wire contract, linked into the test output by the csproj). A new property on + // any request DTO now fails here until the catalog declares it. + [Test] + public void Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields() + { + using JsonDocument spec = LoadOpenApiDocument(); + JsonElement paths = spec.RootElement.GetProperty("paths"); + + ToolDefinition[] writeTools = ToolCatalog.All + .Where(t => t.HttpMethod == HttpMethod.Post + || t.HttpMethod == HttpMethod.Put + || t.HttpMethod == HttpMethod.Patch) + .ToArray(); + + // Pin the covered set rather than trusting the filter. A tool that stopped being a write verb, + // or a new write tool, must show up as a change here — a bare loop over a filtered set passes + // just as happily when the set silently shrinks to nothing. + string[] expectedWriteTools = + [ + "ersatztv_add_collection_items", + "ersatztv_create_channel", + "ersatztv_create_collection", + "ersatztv_create_playout", + "ersatztv_create_schedule", + "ersatztv_create_smart_collection", + "ersatztv_enable_jellyfin_library_sync", + "ersatztv_refresh_jellyfin_libraries", + "ersatztv_reset_channel_playout", + "ersatztv_scan_jellyfin_collections", + "ersatztv_scan_library", + "ersatztv_update_channel", + "ersatztv_update_collection", + "ersatztv_update_collection_custom_order", + "ersatztv_update_playout", + "ersatztv_update_schedule", + "ersatztv_update_smart_collection" + ]; + + writeTools.Select(t => t.Name).OrderBy(n => n, StringComparer.Ordinal) + .ShouldBe(expectedWriteTools.OrderBy(n => n, StringComparer.Ordinal)); + + foreach (ToolDefinition tool in writeTools) + { + paths.TryGetProperty(tool.PathTemplate, out JsonElement pathItem) + .ShouldBeTrue($"{tool.Name}: {tool.PathTemplate} is not in the OpenAPI document"); + + string verb = tool.HttpMethod.Method.ToLowerInvariant(); + pathItem.TryGetProperty(verb, out JsonElement operation) + .ShouldBeTrue($"{tool.Name}: {verb.ToUpperInvariant()} {tool.PathTemplate} is not in the OpenAPI document"); + + HashSet declared = DeclaredBodyArguments(tool); + HashSet accepted = RequestBodyProperties(spec, operation, tool.Name); + + declared.OrderBy(n => n, StringComparer.Ordinal) + .ShouldBe( + accepted.OrderBy(n => n, StringComparer.Ordinal), + customMessage: + $"{tool.Name} declares body fields that do not match {verb.ToUpperInvariant()} {tool.PathTemplate}. " + + "A field the endpoint accepts but the tool omits is silently dropped on a full-replace " + + "write (#754); a field the tool sends but the endpoint does not accept is rejected."); + } + } + + // The body is every declared argument that is not routed elsewhere — mirroring exactly how + // ErsatzTvApiClient builds the request, so this test cannot disagree with the code it guards. + private static HashSet DeclaredBodyArguments(ToolDefinition tool) + { + var pathParameters = Regex.Matches(tool.PathTemplate, @"\{([^}]+)\}") + .Select(m => m.Groups[1].Value) + .ToHashSet(StringComparer.Ordinal); + + IReadOnlySet queryParameters = tool.QueryParameters ?? new HashSet(StringComparer.Ordinal); + + return tool.InputSchema.RootElement.TryGetProperty("properties", out JsonElement properties) + ? properties.EnumerateObject() + .Select(p => p.Name) + .Where(name => !pathParameters.Contains(name) + && !queryParameters.Contains(name) + && !string.Equals(name, "ifMatch", StringComparison.Ordinal)) + .ToHashSet(StringComparer.Ordinal) + : []; + } + + private static HashSet RequestBodyProperties(JsonDocument spec, JsonElement operation, string toolName) + { + // No request body at all (queue/scan POSTs) — the tool must send none either. + if (!operation.TryGetProperty("requestBody", out JsonElement requestBody)) + { + return []; + } + + JsonElement schema = requestBody + .GetProperty("content") + .GetProperty("application/json") + .GetProperty("schema"); + + // Every request body in this document is a plain $ref to a component schema. Anything else + // (allOf/inline/oneOf) is a contract shape this guard has not been taught to read, so fail + // loudly rather than comparing against an empty set and reporting a false pass. + schema.TryGetProperty("$ref", out JsonElement reference) + .ShouldBeTrue($"{toolName}: request body schema is not a $ref; teach this test the new shape"); + + string componentName = reference.GetString().ShouldNotBeNull().Split('/')[^1]; + + return spec.RootElement + .GetProperty("components") + .GetProperty("schemas") + .GetProperty(componentName) + .GetProperty("properties") + .EnumerateObject() + .Select(p => p.Name) + .ToHashSet(StringComparer.Ordinal); + } + + private static JsonDocument LoadOpenApiDocument() + { + string path = Path.Combine(AppContext.BaseDirectory, "openapi", "v1.json"); + + // A missing spec would make every assertion above vacuous, so it is an explicit failure. + File.Exists(path).ShouldBeTrue( + $"OpenAPI document not found at {path}; the test project links it from ErsatzTV/wwwroot/openapi/v1.json"); + + return JsonDocument.Parse(File.ReadAllText(path)); + } } diff --git a/ErsatzTV.Mcp/ToolCatalog.cs b/ErsatzTV.Mcp/ToolCatalog.cs index f18c93032..44053393b 100644 --- a/ErsatzTV.Mcp/ToolCatalog.cs +++ b/ErsatzTV.Mcp/ToolCatalog.cs @@ -159,9 +159,22 @@ public static class ToolCatalog ChannelFields()), Put( "ersatztv_update_channel", - "Update a channel. Send the full desired state; enum fields take the enum name (GET the channel first to copy current values).", + "Update a channel. Send the full desired state; enum fields take the enum name (GET the channel first to copy current values). " + + "graphicsElementIds is part of that state: omitting it DETACHES every graphics element (e.g. the On Now/Next overlay), " + + "so copy it from ersatztv_get_channel unless you mean to clear it.", "/api/v1/channels/{id}", - [IdPath("Channel id."), .. ChannelFields()]), + [ + IdPath("Channel id."), + .. ChannelFields(), + + // Update-only: UpdateChannelRequest carries GraphicsElementIds, CreateChannelRequest does + // not, so this cannot move into the shared ChannelFields() without making create send an + // unknown property. PUT is a full replace, so omitting it detaches every attached element + // with no error — issue #754. + IntArray( + "graphicsElementIds", + "Ids of the graphics elements attached to the channel. Full replace: omit or send [] to detach all.") + ]), Post( "ersatztv_reset_channel_playout", "Queue a rebuild of a channel's playout (202 Accepted; 409 if a build is already running).", @@ -297,7 +310,12 @@ public static class ToolCatalog Bool("treatCollectionsAsShows", "Treat collections as shows."), Bool("shuffleScheduleItems", "Shuffle schedule items."), Bool("randomStartPoint", "Use a random start point."), - Str("fixedStartTimeBehavior", "Fixed start-time behavior (enum name; GET a schedule to see valid values).") + Str("fixedStartTimeBehavior", "Fixed start-time behavior (enum name; GET a schedule to see valid values)."), + + // Both Create- and UpdateScheduleRequest carry this, so it belongs in the shared helper. The + // update PUT is a full replace that writes the value unconditionally, so omitting it used to + // clear a configured pad silently — the same #754 shape as channel graphicsElementIds. + Int("padToNearestMinute", "Pad each item to the nearest N minutes; omit or send null for no padding.") ]; // ---- Tool factories ---- diff --git a/docs/decisions/README.md b/docs/decisions/README.md index b6e1fa15d..094b2bd8b 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -96,6 +96,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `iptv.logo-drives-bug-preset` | One uploaded channel logo drives both the listing logo and the on-screen bug via a shared, seeded `ChannelLogo`-sourced watermark preset (`Channel Bug`), not new per-channel schema. | 2026-07-20 | [link](records/iptv/logo-drives-bug-preset.md) | | `locking.entitylocker-atomic-flags` | `EntityLocker` uses `Interlocked.CompareExchange`-guarded atomic flags plus a documented single-owner-release discipline (no owner tokens/leases); `Unlock*` on an already-unlocked slot returns `false` and logs a Warning rather than throwing. | 2026-07-11 | [link](records/locking/entitylocker-atomic-flags.md) | | `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) | +| `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral, because the write paths are full replaces: the omission is silently applied as a clear. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | | `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) | | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | diff --git a/docs/decisions/records/mcp/tool-schema-openapi-parity.md b/docs/decisions/records/mcp/tool-schema-openapi-parity.md new file mode 100644 index 000000000..afbfb30ee --- /dev/null +++ b/docs/decisions/records/mcp/tool-schema-openapi-parity.md @@ -0,0 +1,55 @@ +--- +key: mcp.tool-schema-openapi-parity +title: '2026-08-06 — every MCP write tool declares exactly its endpoint''s OpenAPI request-body fields, asserted in CI (#754)' +status: active +since: '2026-08-06' +supersedes: none +superseded-by: none +rule: 'Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral, because the write paths are full replaces: the omission is silently applied as a clear.' +signals: 'MCP tool schema drift, full-replace write, silently dropped field, graphicsElementIds, padToNearestMinute, additionalProperties false · paths: `ErsatzTV.Mcp/ToolCatalog.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `docs/mcp.md` · issues: #754, #58, #616' +mechanics: '`ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`; `ErsatzTV.Mcp.Tests.csproj` links `openapi/v1.json`' +--- + +`ToolCatalog.ChannelFields()` declared 27 of `UpdateChannelRequest`'s 28 properties. The missing one +was `graphicsElementIds`, which attaches channel-level graphics elements including the built-in On +Now/Next overlay (`graphics.channel-level-attachment`). + +The cost was not "one field you cannot set". `PUT /api/v1/channels/{id}` is a **full replace**, and +the tool's own description instructs the caller to *"send the full desired state"* — which the schema +could not express. An agent that faithfully GET-edit-PUT a channel detached every attached graphics +element, with a `200` and no error. Nothing surfaced until the overlay stopped rendering at the next +transition, hours later. That is the `optional-parameter-on-shared-primitive-is-opt-out` shape: the +omission is invisible at the call site and only observable as missing pixels. + +Fixing the one field would have left the mechanism intact, and the mechanism had already produced a +second instance: `ScheduleFlags()` omitted `padToNearestMinute`, which both `CreateScheduleRequest` +and `UpdateScheduleRequest` carry and `UpdateProgramScheduleHandler` writes unconditionally — so +`ersatztv_update_schedule` silently cleared a configured pad the same way. Nothing tied a tool's +declared arguments to the contract it wraps, so the next added DTO property would have drifted too. + +So the guard is the decision, and it is asserted against the **generated OpenAPI document** rather +than the DTO types: `v1.json` is the actual wire contract, it is already regenerated by +`scripts/update-openapi.sh` as part of the API checklist, and asserting against it keeps +`ErsatzTV.Mcp.Tests` free of a project reference to the whole ASP.NET host. The test derives each +tool's body set exactly as `ErsatzTvApiClient` does — declared arguments minus path parameters, minus +query parameters, minus the reserved `ifMatch` header — so the guard cannot disagree with the routing +it guards. + +Two anti-vacuity properties are deliberate, per the repo's standing "a test that filters on the +property it asserts cannot see what is missing" rule: + +- The **covered write-tool set is pinned by name**, not merely filtered. A tool that stops being a + write verb, or a new one that is added, changes this list rather than silently leaving the loop. +- A **missing or unrecognised spec is a failure**, never an empty comparison: an absent `v1.json` + fails with the path it looked in, and a request body that is not a plain `$ref` (an `allOf`, + `oneOf`, or inline schema) fails asking to be taught the shape instead of comparing against `{}`. + +Both were verified by mutation rather than assumed: dropping `graphicsElementIds`, dropping +`padToNearestMinute`, and removing the copied spec each turn the suite red, and each failure names +the field or path at fault. + +`graphicsElementIds` is declared on the **update tool only**, not in the shared `ChannelFields()`: +`CreateChannelRequest` has no such property, and the tool schemas are `additionalProperties:false`, +so sharing it would make every create call send an unknown property. `padToNearestMinute` is on both +schedule requests, so it does belong in the shared `ScheduleFlags()`. The parity test is what makes +that per-field placement checkable rather than a matter of care. diff --git a/docs/mcp.md b/docs/mcp.md index 251570a7f..f60bc3487 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -186,12 +186,29 @@ Re-adding an already-present item is an **idempotent no-op** (no duplicate rows, referenced id does not exist the whole batch is rejected (`422`). So the flow is: search → add ids → re-run to confirm idempotence. +### Full-replace writes drop what you omit (`mcp.tool-schema-openapi-parity`) + +Every write here is a **full replace**, not a patch: a field you leave out is not "left unchanged", it +is written as empty. So the GET → edit one field → PUT flow is only safe if the tool can express the +whole state, and `ersatztv_update_channel` could not — it omitted `graphicsElementIds`, so that flow +silently detached every graphics element (including the On Now/Next overlay) with a `200` and no +error, visible only as missing pixels at the next transition. `ersatztv_update_schedule` cleared +`padToNearestMinute` the same way (ersatztv#754). + +Both are fixed, and the class is now guarded: `ToolCatalogTests` asserts that every POST/PUT/PATCH +tool declares **exactly** the request-body fields its endpoint accepts, comparing against the +generated `ErsatzTV/wwwroot/openapi/v1.json`. A new property on a request DTO fails that test until +the catalog declares it. When adding a write tool, regenerate the spec +(`./scripts/update-openapi.sh`) and add the tool to the pinned list in that test. + ## Deferred -Channel create/update (`ersatztv_create_channel` / `ersatztv_update_channel`) wrap a 28-field DTO with -nine enum fields. Only `name`/`number`/`ffmpegProfileId` are required; the rest have server-side +Channel create/update (`ersatztv_create_channel` / `ersatztv_update_channel`) wrap a large DTO with +nine enum fields — 27 body fields on create, and 28 on update, which additionally carries +`graphicsElementIds`. Only `name`/`number`/`ffmpegProfileId` are required; the rest have server-side defaults, and the enum fields take the enum **name** (the API validates them). Discover an existing -channel's shape and current enum values with `ersatztv_get_channel` before creating/updating. +channel's shape and current enum values with `ersatztv_get_channel` before creating/updating — and +copy its `graphicsElementIds` through unless you mean to detach them. Deliberately **not** exposed in this cautious first write pass: -- 2.47.3 From b9e63980ac37940ff2a4bea30c1f63b251c53108 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 21:36:03 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(754):=20review=20round=201=20=E2=80=94?= =?UTF-8?q?=20warn=20on=20the=20schedule=20twin,=20scope=20the=20full-repl?= =?UTF-8?q?ace=20claim,=20make=20the=20guard=20type-aware?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- ErsatzTV.Mcp.Tests/ToolCatalogTests.cs | 86 ++++++++++++++----- ErsatzTV.Mcp/ToolCatalog.cs | 3 +- docs/decisions/README.md | 2 +- .../records/mcp/tool-schema-openapi-parity.md | 31 +++++-- docs/mcp.md | 13 ++- 5 files changed, 104 insertions(+), 31 deletions(-) diff --git a/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs b/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs index edb590bef..7748569d5 100644 --- a/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs +++ b/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs @@ -329,22 +329,28 @@ public class ToolCatalogTests pathItem.TryGetProperty(verb, out JsonElement operation) .ShouldBeTrue($"{tool.Name}: {verb.ToUpperInvariant()} {tool.PathTemplate} is not in the OpenAPI document"); - HashSet declared = DeclaredBodyArguments(tool); - HashSet accepted = RequestBodyProperties(spec, operation, tool.Name); + Dictionary declared = DeclaredBodyArguments(tool); + Dictionary accepted = RequestBodyProperties(spec, operation, tool.Name); - declared.OrderBy(n => n, StringComparer.Ordinal) + // Compare name AND type. Names alone would let a field drift to the wrong JSON type: the + // tool would advertise "string" for an int?, the agent would send "30", and the API would + // 400 — green test, broken tool. + declared.Select(p => $"{p.Key}: {p.Value}").OrderBy(s => s, StringComparer.Ordinal) .ShouldBe( - accepted.OrderBy(n => n, StringComparer.Ordinal), + accepted.Select(p => $"{p.Key}: {p.Value}").OrderBy(s => s, StringComparer.Ordinal), customMessage: $"{tool.Name} declares body fields that do not match {verb.ToUpperInvariant()} {tool.PathTemplate}. " + "A field the endpoint accepts but the tool omits is silently dropped on a full-replace " - + "write (#754); a field the tool sends but the endpoint does not accept is rejected."); + + "write (#754); a field the tool sends but the endpoint does not accept is rejected; " + + "a field declared with the wrong type is rejected at the API."); } } // The body is every declared argument that is not routed elsewhere — mirroring exactly how // ErsatzTvApiClient builds the request, so this test cannot disagree with the code it guards. - private static HashSet DeclaredBodyArguments(ToolDefinition tool) + // DELETE is not compared: ErsatzTvApiClient sets hasBody for POST/PUT/PATCH only, so a body + // argument on a DELETE tool would be silently dropped. No DELETE tool has one today. + private static Dictionary DeclaredBodyArguments(ToolDefinition tool) { var pathParameters = Regex.Matches(tool.PathTemplate, @"\{([^}]+)\}") .Select(m => m.Groups[1].Value) @@ -352,17 +358,29 @@ public class ToolCatalogTests IReadOnlySet queryParameters = tool.QueryParameters ?? new HashSet(StringComparer.Ordinal); - return tool.InputSchema.RootElement.TryGetProperty("properties", out JsonElement properties) - ? properties.EnumerateObject() - .Select(p => p.Name) - .Where(name => !pathParameters.Contains(name) - && !queryParameters.Contains(name) - && !string.Equals(name, "ifMatch", StringComparison.Ordinal)) - .ToHashSet(StringComparer.Ordinal) - : []; + if (!tool.InputSchema.RootElement.TryGetProperty("properties", out JsonElement properties)) + { + return []; + } + + return properties.EnumerateObject() + .Where(p => !pathParameters.Contains(p.Name) + && !queryParameters.Contains(p.Name) + && !string.Equals(p.Name, "ifMatch", StringComparison.Ordinal)) + .ToDictionary(p => p.Name, p => DeclaredType(p.Value), StringComparer.Ordinal); } - private static HashSet RequestBodyProperties(JsonDocument spec, JsonElement operation, string toolName) + // The tool schema's own shape: a plain "type", plus the array element type where there is one. + private static string DeclaredType(JsonElement property) + { + string type = property.GetProperty("type").GetString().ShouldNotBeNull(); + + return type == "array" && property.TryGetProperty("items", out JsonElement items) + ? $"array<{items.GetProperty("type").GetString()}>" + : type; + } + + private static Dictionary RequestBodyProperties(JsonDocument spec, JsonElement operation, string toolName) { // No request body at all (queue/scan POSTs) — the tool must send none either. if (!operation.TryGetProperty("requestBody", out JsonElement requestBody)) @@ -381,16 +399,44 @@ public class ToolCatalogTests schema.TryGetProperty("$ref", out JsonElement reference) .ShouldBeTrue($"{toolName}: request body schema is not a $ref; teach this test the new shape"); + JsonElement schemas = spec.RootElement.GetProperty("components").GetProperty("schemas"); string componentName = reference.GetString().ShouldNotBeNull().Split('/')[^1]; - return spec.RootElement - .GetProperty("components") - .GetProperty("schemas") + return schemas .GetProperty(componentName) .GetProperty("properties") .EnumerateObject() - .Select(p => p.Name) - .ToHashSet(StringComparer.Ordinal); + .ToDictionary(p => p.Name, p => SpecType(schemas, p.Value, toolName, p.Name), StringComparer.Ordinal); + } + + // Normalize the generator's shapes onto the catalog's vocabulary. Two forms appear in this + // document: a nullable type as ["null", T] (the catalog has no nullable notion — optionality is + // carried by `required`), and a $ref to a component, which for the enum fields is a string enum + // and for `logo` is an object. + private static string SpecType(JsonElement schemas, JsonElement property, string toolName, string fieldName) + { + if (property.TryGetProperty("$ref", out JsonElement reference)) + { + string componentName = reference.GetString().ShouldNotBeNull().Split('/')[^1]; + return SpecType(schemas, schemas.GetProperty(componentName), toolName, fieldName); + } + + JsonElement type = property.GetProperty("type"); + + string[] types = type.ValueKind == JsonValueKind.Array + ? type.EnumerateArray().Select(t => t.GetString()).OfType().Where(t => t != "null").ToArray() + : [type.GetString().ShouldNotBeNull()]; + + // More than one non-null type is a shape this guard has not been taught to read; fail rather + // than picking one and reporting a comparison that means nothing. + types.Length.ShouldBe(1, $"{toolName}.{fieldName}: unexpected OpenAPI type union [{string.Join(", ", types)}]"); + + // The element schema is resolved through the same normalization: an array's items can itself be + // a $ref to a component (ReplaceRemoteLibraryPreferencesRequest.libraries), which the catalog + // declares as an object array. + return types[0] == "array" && property.TryGetProperty("items", out JsonElement items) + ? $"array<{SpecType(schemas, items, toolName, fieldName)}>" + : types[0]; } private static JsonDocument LoadOpenApiDocument() diff --git a/ErsatzTV.Mcp/ToolCatalog.cs b/ErsatzTV.Mcp/ToolCatalog.cs index 44053393b..0a36837b4 100644 --- a/ErsatzTV.Mcp/ToolCatalog.cs +++ b/ErsatzTV.Mcp/ToolCatalog.cs @@ -132,7 +132,8 @@ public static class ToolCatalog [Str("name", "Schedule name.", required: true), .. ScheduleFlags()]), Put( "ersatztv_update_schedule", - "Update a program schedule's settings.", + "Update a program schedule. Send the full desired state: every field is applied, so omitting " + + "padToNearestMinute CLEARS a configured pad (GET the schedule first to copy current values).", "/api/v1/schedules/{id}", [IdPath("Schedule id."), Str("name", "Schedule name.", required: true), .. ScheduleFlags()]), Delete("ersatztv_delete_schedule", "Delete a program schedule.", "/api/v1/schedules/{id}", IdPath("Schedule id.")), diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 094b2bd8b..0429888d5 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -96,7 +96,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `iptv.logo-drives-bug-preset` | One uploaded channel logo drives both the listing logo and the on-screen bug via a shared, seeded `ChannelLogo`-sourced watermark preset (`Channel Bug`), not new per-channel schema. | 2026-07-20 | [link](records/iptv/logo-drives-bug-preset.md) | | `locking.entitylocker-atomic-flags` | `EntityLocker` uses `Interlocked.CompareExchange`-guarded atomic flags plus a documented single-owner-release discipline (no owner tokens/leases); `Unlock*` on an already-unlocked slot returns `false` and logs a Warning rather than throwing. | 2026-07-11 | [link](records/locking/entitylocker-atomic-flags.md) | | `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) | -| `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral, because the write paths are full replaces: the omission is silently applied as a clear. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | +| `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. READ tools are outside this guard. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | | `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) | | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | diff --git a/docs/decisions/records/mcp/tool-schema-openapi-parity.md b/docs/decisions/records/mcp/tool-schema-openapi-parity.md index afbfb30ee..f9830e264 100644 --- a/docs/decisions/records/mcp/tool-schema-openapi-parity.md +++ b/docs/decisions/records/mcp/tool-schema-openapi-parity.md @@ -5,7 +5,7 @@ status: active since: '2026-08-06' supersedes: none superseded-by: none -rule: 'Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral, because the write paths are full replaces: the omission is silently applied as a clear.' +rule: 'Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. READ tools are outside this guard.' signals: 'MCP tool schema drift, full-replace write, silently dropped field, graphicsElementIds, padToNearestMinute, additionalProperties false · paths: `ErsatzTV.Mcp/ToolCatalog.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `docs/mcp.md` · issues: #754, #58, #616' mechanics: '`ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`; `ErsatzTV.Mcp.Tests.csproj` links `openapi/v1.json`' --- @@ -35,18 +35,37 @@ tool's body set exactly as `ErsatzTvApiClient` does — declared arguments minus query parameters, minus the reserved `ifMatch` header — so the guard cannot disagree with the routing it guards. -Two anti-vacuity properties are deliberate, per the repo's standing "a test that filters on the +Three anti-vacuity properties are deliberate, per the repo's standing "a test that filters on the property it asserts cannot see what is missing" rule: - The **covered write-tool set is pinned by name**, not merely filtered. A tool that stops being a write verb, or a new one that is added, changes this list rather than silently leaving the loop. - A **missing or unrecognised spec is a failure**, never an empty comparison: an absent `v1.json` fails with the path it looked in, and a request body that is not a plain `$ref` (an `allOf`, - `oneOf`, or inline schema) fails asking to be taught the shape instead of comparing against `{}`. + `oneOf`, or inline schema), or a property whose type is a union this guard has not been taught, + fails asking to be taught the shape instead of comparing against `{}`. +- **Names are compared with types**, not alone. A name-only guard is the same defect one level down: + the tool would advertise `string` for an `int?`, the agent would send `"30"`, and the API would + reject it — green test, broken tool. The generator's `["null", T]` nullable form and its `$ref` + (enum → `string`, model → `object`) are normalized onto the catalog's vocabulary, arrays down to + their element type. -Both were verified by mutation rather than assumed: dropping `graphicsElementIds`, dropping -`padToNearestMinute`, and removing the copied spec each turn the suite red, and each failure names -the field or path at fault. +All were verified by mutation rather than assumed: dropping `graphicsElementIds`, dropping +`padToNearestMinute`, retyping either field, drifting an array's element type, and removing the +copied spec each turn the suite red, and each failure names the field or path at fault. + +**Scope, stated so it is not mistaken for more.** The guard covers POST/PUT/PATCH **request bodies** +only. It does not cover DELETE (`ErsatzTvApiClient` builds a body for POST/PUT/PATCH only, so a body +argument there would be silently dropped — no tool has one today), and it does not cover **query +parameters on any verb**, so the read tools are unguarded and two of them already drift +(`ersatztv_list_playouts` omits `query`, `ersatztv_get_playout_items` omits `showFiller` — both lost +capability, not data loss; tracked in #757). Extending the same comparison to +`parameters[in=query]` is the obvious next step and the spec data is already loaded. + +The guard is also a **two-job conjunction**, not self-contained: it compares against a checked-in +`v1.json`, so it is only as fresh as the regeneration. What keeps it honest is the `api-docs` CI job, +whose `^ErsatzTV/Controllers/Api/` path filter covers the directory every request DTO lives in — a +new DTO property cannot leave `v1.json` stale without that job going red. `graphicsElementIds` is declared on the **update tool only**, not in the shared `ChannelFields()`: `CreateChannelRequest` has no such property, and the tool schemas are `additionalProperties:false`, diff --git a/docs/mcp.md b/docs/mcp.md index f60bc3487..50eb94930 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -188,9 +188,16 @@ re-run to confirm idempotence. ### Full-replace writes drop what you omit (`mcp.tool-schema-openapi-parity`) -Every write here is a **full replace**, not a patch: a field you leave out is not "left unchanged", it -is written as empty. So the GET → edit one field → PUT flow is only safe if the tool can express the -whole state, and `ersatztv_update_channel` could not — it omitted `graphicsElementIds`, so that flow +**Check each tool's own description — the write tools are not uniform.** Some are full replaces, where +a field you leave out is not "left unchanged" but written as empty: `ersatztv_update_channel`, +`ersatztv_update_schedule`, `ersatztv_update_collection_custom_order`. Others are additive or +leave-unchanged and say so: `ersatztv_add_collection_items` is an idempotent add (it does **not** +replace membership), `ersatztv_update_collection` leaves an omitted `useCustomPlaybackOrder` alone, +`ersatztv_update_playout` leaves an omitted `scheduleFile` alone, and +`ersatztv_enable_jellyfin_library_sync` leaves an absent row untouched. + +For the full-replace ones, the GET → edit one field → PUT flow is only safe if the tool can express +the whole state, and `ersatztv_update_channel` could not — it omitted `graphicsElementIds`, so that flow silently detached every graphics element (including the On Now/Next overlay) with a `200` and no error, visible only as missing pixels at the next transition. `ersatztv_update_schedule` cleared `padToNearestMinute` the same way (ersatztv#754). -- 2.47.3 From b1c2b9e18cceb4b52953b5d90e692cb4efa58363 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 21:43:34 +0200 Subject: [PATCH 3/4] fix(757): guard query parameters on every tool, and reclassify update_playout as mixed 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 --- ErsatzTV.Mcp.Tests/ToolCatalogTests.cs | 64 +++++++++++++++++++ ErsatzTV.Mcp/ToolCatalog.cs | 21 +++++- docs/decisions/README.md | 2 +- .../records/mcp/tool-schema-openapi-parity.md | 44 +++++++++---- docs/mcp.md | 38 +++++++---- 5 files changed, 143 insertions(+), 26 deletions(-) diff --git a/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs b/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs index 7748569d5..3cbcbd298 100644 --- a/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs +++ b/ErsatzTV.Mcp.Tests/ToolCatalogTests.cs @@ -346,6 +346,70 @@ public class ToolCatalogTests } } + // #757, the sibling of the body guard above. Query parameters drift the same way and are WORSE for + // reads: ToolArgumentValidator rejects undeclared arguments, so a parameter the tool omits is not + // merely undocumented, it is unreachable — the caller cannot pass it at all. That is how #616's + // paging omission hard-capped two tools at the first page. This covers EVERY tool, not just the + // write verbs, because the drift that existed when this was written was entirely on reads. + [Test] + public void Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters() + { + using JsonDocument spec = LoadOpenApiDocument(); + JsonElement paths = spec.RootElement.GetProperty("paths"); + + // Every tool is covered, so an emptiness guard is enough here — there is no filter to escape. + ToolCatalog.All.Count.ShouldBeGreaterThan(30); + + // Accumulate rather than throwing on the first mismatch, so one run reports the WHOLE drift set. + // Failing fast here would hand back one tool at a time and invite fixing them one at a time, + // which is how the #754 twin stayed hidden in the first place. + List drift = []; + + foreach (ToolDefinition tool in ToolCatalog.All) + { + paths.TryGetProperty(tool.PathTemplate, out JsonElement pathItem) + .ShouldBeTrue($"{tool.Name}: {tool.PathTemplate} is not in the OpenAPI document"); + + string verb = tool.HttpMethod.Method.ToLowerInvariant(); + pathItem.TryGetProperty(verb, out JsonElement operation) + .ShouldBeTrue($"{tool.Name}: {verb.ToUpperInvariant()} {tool.PathTemplate} is not in the OpenAPI document"); + + IReadOnlySet declared = tool.QueryParameters ?? new HashSet(StringComparer.Ordinal); + HashSet accepted = QueryParameterNames(operation); + + string[] missing = accepted.Except(declared, StringComparer.Ordinal).OrderBy(n => n, StringComparer.Ordinal).ToArray(); + string[] phantom = declared.Except(accepted, StringComparer.Ordinal).OrderBy(n => n, StringComparer.Ordinal).ToArray(); + + if (missing.Length > 0 || phantom.Length > 0) + { + drift.Add( + $"{tool.Name} ({verb.ToUpperInvariant()} {tool.PathTemplate}): " + + $"unreachable={string.Join(",", missing)} phantom={string.Join(",", phantom)}"); + } + } + + // A parameter the endpoint accepts but the tool omits is UNREACHABLE, not merely undocumented: + // ToolArgumentValidator rejects undeclared arguments, so the caller cannot pass it at all + // (#616 hard-capped two paged tools exactly this way). A phantom is the reverse — the tool + // advertises something the endpoint ignores. + drift.ShouldBeEmpty(); + } + + private static HashSet QueryParameterNames(JsonElement operation) + { + if (!operation.TryGetProperty("parameters", out JsonElement parameters)) + { + return []; + } + + return parameters.EnumerateArray() + .Where(p => p.TryGetProperty("in", out JsonElement location) + && string.Equals(location.GetString(), "query", StringComparison.Ordinal)) + .Select(p => p.GetProperty("name").GetString()) + .OfType() + .ToHashSet(StringComparer.Ordinal); + } + // The body is every declared argument that is not routed elsewhere — mirroring exactly how // ErsatzTvApiClient builds the request, so this test cannot disagree with the code it guards. // DELETE is not compared: ErsatzTvApiClient sets hasBody for POST/PUT/PATCH only, so a body diff --git a/ErsatzTV.Mcp/ToolCatalog.cs b/ErsatzTV.Mcp/ToolCatalog.cs index 0a36837b4..33918184c 100644 --- a/ErsatzTV.Mcp/ToolCatalog.cs +++ b/ErsatzTV.Mcp/ToolCatalog.cs @@ -29,14 +29,31 @@ 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 (paged).", "/api/v1/playouts", [], Page()), + Get( + "ersatztv_list_playouts", + "List playouts (paged), optionally filtered by channel name.", + "/api/v1/playouts", + [], + [ + Str( + "query", + "Optional case-insensitive substring match on the CHANNEL name (not the playout or schedule name); omit for all playouts.", + arg: In.Query), + .. 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 (paged).", "/api/v1/playouts/{id}/items", [IdPath("Playout id.")], - Page()), + [ + Bool( + "showFiller", + "Include filler items (pre/mid/post-roll, tail) in the result; default false returns only non-filler items.", + arg: In.Query), + .. 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( diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 0429888d5..8ebe9f9d2 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -96,7 +96,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `iptv.logo-drives-bug-preset` | One uploaded channel logo drives both the listing logo and the on-screen bug via a shared, seeded `ChannelLogo`-sourced watermark preset (`Channel Bug`), not new per-channel schema. | 2026-07-20 | [link](records/iptv/logo-drives-bug-preset.md) | | `locking.entitylocker-atomic-flags` | `EntityLocker` uses `Interlocked.CompareExchange`-guarded atomic flags plus a documented single-owner-release discipline (no owner tokens/leases); `Unlock*` on an already-unlocked slot returns `false` and logs a Warning rather than throwing. | 2026-07-11 | [link](records/locking/entitylocker-atomic-flags.md) | | `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) | -| `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. READ tools are outside this guard. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | +| `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, and EVERY tool (read and write) declares exactly its endpoint's query parameters, both asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields` and `Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. An omitted query parameter is UNREACHABLE, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | | `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) | | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | diff --git a/docs/decisions/records/mcp/tool-schema-openapi-parity.md b/docs/decisions/records/mcp/tool-schema-openapi-parity.md index f9830e264..9ec8234f8 100644 --- a/docs/decisions/records/mcp/tool-schema-openapi-parity.md +++ b/docs/decisions/records/mcp/tool-schema-openapi-parity.md @@ -1,12 +1,12 @@ --- key: mcp.tool-schema-openapi-parity -title: '2026-08-06 — every MCP write tool declares exactly its endpoint''s OpenAPI request-body fields, asserted in CI (#754)' +title: '2026-08-06 — every MCP tool declares exactly its endpoint''s OpenAPI request-body fields and query parameters, asserted in CI (#754, #757)' status: active since: '2026-08-06' supersedes: none superseded-by: none -rule: 'Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. READ tools are outside this guard.' -signals: 'MCP tool schema drift, full-replace write, silently dropped field, graphicsElementIds, padToNearestMinute, additionalProperties false · paths: `ErsatzTV.Mcp/ToolCatalog.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `docs/mcp.md` · issues: #754, #58, #616' +rule: 'Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, and EVERY tool (read and write) declares exactly its endpoint''s query parameters, both asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields` and `Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. An omitted query parameter is UNREACHABLE, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments.' +signals: 'MCP tool schema drift, full-replace write, silently dropped field, graphicsElementIds, padToNearestMinute, additionalProperties false · paths: `ErsatzTV.Mcp/ToolCatalog.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `docs/mcp.md` · issues: #754, #757, #58, #616' mechanics: '`ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`; `ErsatzTV.Mcp.Tests.csproj` links `openapi/v1.json`' --- @@ -54,18 +54,40 @@ All were verified by mutation rather than assumed: dropping `graphicsElementIds` `padToNearestMinute`, retyping either field, drifting an array's element type, and removing the copied spec each turn the suite red, and each failure names the field or path at fault. -**Scope, stated so it is not mistaken for more.** The guard covers POST/PUT/PATCH **request bodies** -only. It does not cover DELETE (`ErsatzTvApiClient` builds a body for POST/PUT/PATCH only, so a body -argument there would be silently dropped — no tool has one today), and it does not cover **query -parameters on any verb**, so the read tools are unguarded and two of them already drift -(`ersatztv_list_playouts` omits `query`, `ersatztv_get_playout_items` omits `showFiller` — both lost -capability, not data loss; tracked in #757). Extending the same comparison to -`parameters[in=query]` is the obvious next step and the spec data is already loaded. +**Query parameters are guarded the same way, across every tool (#757).** A second test compares each +tool's routed `QueryParameters` against the spec's `parameters[in=query]` for its path and verb, reads +included — the drift that existed when this was written was entirely on reads. An omitted parameter +there is worse than an undeclared body field: `additionalProperties:false` means the caller cannot +pass it *at all*, so the capability is unreachable rather than merely undocumented (`ersatztv_list_playouts` +had lost its channel-name `query` filter and `ersatztv_get_playout_items` its `showFiller`; #616 was +the same shape with paging). That test **accumulates** its mismatches and asserts once, so a run +reports the whole drift set — failing on the first would invite fixing one tool at a time, which is +how the twin in this very issue stayed hidden. + +**Scope, stated so it is not mistaken for more.** Request bodies are compared for POST/PUT/PATCH only. +DELETE is uncovered because `ErsatzTvApiClient` builds a body for POST/PUT/PATCH only, so a body +argument on a DELETE tool would be silently dropped; no tool has one today. Header arguments (`ifMatch`) +and per-parameter *descriptions* are not compared either — `api.paging-zero-based` is pinned by its own +test. + +The type comparison is **lossy by design, at the catalog's ceiling**: the catalog's vocabulary is +`{string, integer, number, boolean, object, array}`, so every object component collapses to `object` +and every enum to `string`. Swapping one model or enum for another is therefore invisible here +(verified by repointing `logo` at a structurally unrelated model — the suite stays green), as is +`format` (`int32` vs `int64`). That is the right ceiling rather than a gap to close: comparing deeper +than the catalog can express would assert a distinction no tool schema carries, and an opaque object +like `logo` is copied through from a GET verbatim, so nested drift cannot cause the silent-clear this +record exists to prevent. `integer` vs `number` IS distinguished. The `>1` non-null type-union +assertion is a fail-loud guard for a shape this generator does not currently emit, so it is deliberate +but **unexercised**. The guard is also a **two-job conjunction**, not self-contained: it compares against a checked-in `v1.json`, so it is only as fresh as the regeneration. What keeps it honest is the `api-docs` CI job, whose `^ErsatzTV/Controllers/Api/` path filter covers the directory every request DTO lives in — a -new DTO property cannot leave `v1.json` stale without that job going red. +new DTO property cannot leave `v1.json` stale without that job going red. That holds for a DTO's OWN +properties and no further: a NESTED model such as `ArtworkContentTypeModel` lives in +`ErsatzTV.Application/Artworks/`, outside that filter, so changing it can leave `v1.json` stale without +the job firing. Pre-existing, and harmless to this guard only because nested shape is not compared. `graphicsElementIds` is declared on the **update tool only**, not in the shared `ChannelFields()`: `CreateChannelRequest` has no such property, and the tool schemas are `additionalProperties:false`, diff --git a/docs/mcp.md b/docs/mcp.md index 50eb94930..154037674 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -188,13 +188,19 @@ re-run to confirm idempotence. ### Full-replace writes drop what you omit (`mcp.tool-schema-openapi-parity`) -**Check each tool's own description — the write tools are not uniform.** Some are full replaces, where -a field you leave out is not "left unchanged" but written as empty: `ersatztv_update_channel`, -`ersatztv_update_schedule`, `ersatztv_update_collection_custom_order`. Others are additive or -leave-unchanged and say so: `ersatztv_add_collection_items` is an idempotent add (it does **not** -replace membership), `ersatztv_update_collection` leaves an omitted `useCustomPlaybackOrder` alone, -`ersatztv_update_playout` leaves an omitted `scheduleFile` alone, and -`ersatztv_enable_jellyfin_library_sync` leaves an absent row untouched. +**Check each tool's own description — the write tools are not uniform, and one is not uniform with +itself.** Three are full replaces, where a field you leave out is not "left unchanged" but written as +empty: `ersatztv_update_channel`, `ersatztv_update_schedule`, `ersatztv_update_collection_custom_order`. + +`ersatztv_update_playout` is **mixed, and this is the easy one to get wrong**: `scheduleFile` is +leave-unchanged, but `dailyRebuildTime` is always applied — `UpdatePlayoutHandler` sets it to `null` +unconditionally before re-applying a supplied value, so calling this tool to set `scheduleFile` while +omitting `dailyRebuildTime` **silently clears the daily reset**. Send both, or neither. + +The rest are additive or leave-unchanged and say so: `ersatztv_add_collection_items` is an idempotent +add (it does **not** replace membership), `ersatztv_update_collection` leaves an omitted +`useCustomPlaybackOrder` alone, and `ersatztv_enable_jellyfin_library_sync` leaves an absent row +untouched. For the full-replace ones, the GET → edit one field → PUT flow is only safe if the tool can express the whole state, and `ersatztv_update_channel` could not — it omitted `graphicsElementIds`, so that flow @@ -202,11 +208,19 @@ silently detached every graphics element (including the On Now/Next overlay) wit error, visible only as missing pixels at the next transition. `ersatztv_update_schedule` cleared `padToNearestMinute` the same way (ersatztv#754). -Both are fixed, and the class is now guarded: `ToolCatalogTests` asserts that every POST/PUT/PATCH -tool declares **exactly** the request-body fields its endpoint accepts, comparing against the -generated `ErsatzTV/wwwroot/openapi/v1.json`. A new property on a request DTO fails that test until -the catalog declares it. When adding a write tool, regenerate the spec -(`./scripts/update-openapi.sh`) and add the tool to the pinned list in that test. +Both are fixed, and the class is now guarded by two tests in `ToolCatalogTests`, comparing against the +generated `ErsatzTV/wwwroot/openapi/v1.json`: + +- Every POST/PUT/PATCH tool declares **exactly** the request-body fields its endpoint accepts, each + with a matching type. A new property on a request DTO fails until the catalog declares it. +- Every tool — read **and** write — declares **exactly** its endpoint's query parameters. An omitted + one is not merely undocumented but *unreachable*, since `ToolArgumentValidator` rejects undeclared + arguments; that is how #616 hard-capped two paged tools at the first page, and how + `ersatztv_list_playouts` (`query`) and `ersatztv_get_playout_items` (`showFiller`) lost their + filters until ersatztv#757. + +When adding a write tool, regenerate the spec (`./scripts/update-openapi.sh`) and add the tool to the +pinned list in the body test. ## Deferred -- 2.47.3 From 29abd470e479e5d7d677fb5ed258d386f66addc7 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 21:49:23 +0200 Subject: [PATCH 4/4] docs(757): enumerate all seven filler kinds, and record why the two query tests compose 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 --- ErsatzTV.Mcp/ToolCatalog.cs | 3 ++- docs/decisions/records/mcp/tool-schema-openapi-parity.md | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ErsatzTV.Mcp/ToolCatalog.cs b/ErsatzTV.Mcp/ToolCatalog.cs index 33918184c..6e5ad0ad1 100644 --- a/ErsatzTV.Mcp/ToolCatalog.cs +++ b/ErsatzTV.Mcp/ToolCatalog.cs @@ -50,7 +50,8 @@ public static class ToolCatalog [ Bool( "showFiller", - "Include filler items (pre/mid/post-roll, tail) in the result; default false returns only non-filler items.", + "Include items whose filler kind is not None (pre/mid/post-roll, tail, fallback, guide-mode, deco); " + + "default false returns only non-filler items.", arg: In.Query), .. Page() ]), diff --git a/docs/decisions/records/mcp/tool-schema-openapi-parity.md b/docs/decisions/records/mcp/tool-schema-openapi-parity.md index 9ec8234f8..215c8b4bb 100644 --- a/docs/decisions/records/mcp/tool-schema-openapi-parity.md +++ b/docs/decisions/records/mcp/tool-schema-openapi-parity.md @@ -64,6 +64,14 @@ the same shape with paging). That test **accumulates** its mismatches and assert reports the whole drift set — failing on the first would invite fixing one tool at a time, which is how the twin in this very issue stayed hidden. +It also **composes with** the older `Every_Query_Parameter_Should_Be_A_Declared_Property`, and the pair +is the clearest illustration in this repo of why "a test that filters on the property it asserts cannot +see what is missing" is a rule. That older test filters `Where(t => t.QueryParameters is { Count: > 0 })` +— so a tool that lost its query parameters entirely escaped it, which is exactly how `list_playouts` and +`get_playout_items` hid. The new test has no filter and reports them as *unreachable*; the old one then +checks that a routed parameter is also a declared argument. Neither subsumes the other, and the inner +duplicate of the old check was deliberately removed from the new test rather than kept as a second copy. + **Scope, stated so it is not mistaken for more.** Request bodies are compared for POST/PUT/PATCH only. DELETE is uncovered because `ErsatzTvApiClient` builds a body for POST/PUT/PATCH only, so a body argument on a DELETE tool would be silently dropped; no tool has one today. Header arguments (`ifMatch`) -- 2.47.3