From 29abd470e479e5d7d677fb5ed258d386f66addc7 Mon Sep 17 00:00:00 2001 From: Timothy Date: Thu, 6 Aug 2026 21:49:23 +0200 Subject: [PATCH] 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`)