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..3cbcbd298 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,248 @@ 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"); + + Dictionary declared = DeclaredBodyArguments(tool); + Dictionary accepted = RequestBodyProperties(spec, operation, tool.Name); + + // 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.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; " + + "a field declared with the wrong type is rejected at the API."); + } + } + + // #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 + // 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) + .ToHashSet(StringComparer.Ordinal); + + IReadOnlySet queryParameters = tool.QueryParameters ?? new HashSet(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); + } + + // 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)) + { + 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"); + + JsonElement schemas = spec.RootElement.GetProperty("components").GetProperty("schemas"); + string componentName = reference.GetString().ShouldNotBeNull().Split('/')[^1]; + + return schemas + .GetProperty(componentName) + .GetProperty("properties") + .EnumerateObject() + .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() + { + 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..6e5ad0ad1 100644 --- a/ErsatzTV.Mcp/ToolCatalog.cs +++ b/ErsatzTV.Mcp/ToolCatalog.cs @@ -29,14 +29,32 @@ 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 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() + ]), 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( @@ -132,7 +150,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.")), @@ -159,9 +178,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 +329,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..8ebe9f9d2 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, 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 new file mode 100644 index 000000000..215c8b4bb --- /dev/null +++ b/docs/decisions/records/mcp/tool-schema-openapi-parity.md @@ -0,0 +1,104 @@ +--- +key: mcp.tool-schema-openapi-parity +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, 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`' +--- + +`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. + +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), 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. + +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. + +**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. + +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`) +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. 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`, +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..154037674 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -186,12 +186,50 @@ 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`) + +**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 +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 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 -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: