ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs

@@ -1,3 +1,5 @@
+using System.Collections;
+using System.Reflection;
 using System.Threading.Channels;
 using ErsatzTV.Application;
 using ErsatzTV.Application.ProgramSchedules;
@@ -65,7 +67,8 @@ public class ScheduleItemResponseRoundTripTests
             await replaceHandler.Handle(new ReplaceProgramScheduleItems(scheduleId, reconstructed), CancellationToken.None);
         replaced.IsRight.ShouldBeTrue(replaced.LeftToSeq().HeadOrNone().Match(e => e.Value, () => "unknown"));
 
-        // GET again → envelope B; A and B must be semantically identical (ignoring regenerated row ids).
+        // GET again → envelope B; A and B must be semantically identical INCLUDING row ids —
+        // the handler reconciles by id and updates in place, it does not regenerate rows.
         ScheduleItemsResponseModel envelopeB = await GetItemsEnvelope(scheduleId);
 
         envelopeB.Items.Count.ShouldBe(envelopeA.Items.Count);
@@ -340,62 +343,97 @@ public class ScheduleItemResponseRoundTripTests
             r.PreferredSubtitleLanguageCode,
             r.SubtitleMode);
 
+    // ersatztv#779 (detector G): the compared field list is DERIVED from the DTO by reflection,
+    // never hand-copied. The previous version was a hand-written run of `b.X.ShouldBe(a.X)` lines.
+    // It was COMPLETE on the day it was written — every property but Id — and had no way
+    // to report the day it stopped being: a field added to ScheduleItemResponseModel simply went
+    // uncompared, and this "lossless round-trip" test kept passing while the round trip silently
+    // dropped it. That is #754's mechanism exactly (a hand-maintained mirror drifting from a
+    // 28-property DTO by one field, HTTP 200, no error), one altitude up — in the very test whose
+    // job is to catch losses.
+    //
+    // Properties deliberately NOT compared. The set is EMPTY, and that is a finding rather than an
+    // oversight. The first version exempted Id on the reasoning that "the PUT replaces the item
+    // set, so B's rows are new rows with new ids". ReplaceProgramScheduleItemsHandler does not do
+    // that for this fixture's payload: it forwards every Id, takes the id-based reconcile, and
+    // updates rows in place. So Id compares equal, and the exemption was unnecessary.
+    //
+    // Two mutations of this fixture, both EXECUTED — recorded as results, with no account of why,
+    // because three earlier drafts of this comment each supplied a confident mechanism for a
+    // correct observation and two of them were contradicted by the code:
+    //
+    //   ToReplaceCommand passes `null` for EVERY id      -> test stays GREEN
+    //   ToReplaceCommand passes `null` for index 0 only  -> test goes RED, "Id differs"
+    //
+    // So the Id comparison does discriminate; it is not decorative. What it is NOT is a substitute
+    // for ReplaceProgramScheduleItemsReconcileTests, whose
+    // Reorder_ById_Should_Move_State_With_The_Logical_Item_Not_The_Slot and
+    // Insert_ById_In_Middle_Should_Keep_Existing_Ids_And_State pass real ids and pin that state
+    // moves with the logical item rather than the slot. Those are the #252 tests; this is a
+    // round-trip check that happens to also notice a lost row.
+    //
+    // Any name added here must still exist on ScheduleItemResponseModel (asserted below), so
+    // renaming a field cannot leave a stale exemption silently exempting nothing.
+    private static readonly System.Collections.Generic.HashSet<string> RoundTripExemptProperties =
+        new(StringComparer.Ordinal);
+
     private static void AssertSemanticallyEqual(ScheduleItemResponseModel a, ScheduleItemResponseModel b)
     {
-        b.Index.ShouldBe(a.Index);
-        b.StartType.ShouldBe(a.StartType);
-        b.StartTime.ShouldBe(a.StartTime);
-        b.FixedStartTimeBehavior.ShouldBe(a.FixedStartTimeBehavior);
-        b.PlayoutMode.ShouldBe(a.PlayoutMode);
-        b.CollectionType.ShouldBe(a.CollectionType);
-        b.CollectionId.ShouldBe(a.CollectionId);
-        b.MultiCollectionId.ShouldBe(a.MultiCollectionId);
-        b.SmartCollectionId.ShouldBe(a.SmartCollectionId);
-        b.RerunCollectionId.ShouldBe(a.RerunCollectionId);
-        b.MediaItemId.ShouldBe(a.MediaItemId);
-        b.PlaylistId.ShouldBe(a.PlaylistId);
-        b.SearchTitle.ShouldBe(a.SearchTitle);
-        b.SearchQuery.ShouldBe(a.SearchQuery);
-        b.PlaybackOrder.ShouldBe(a.PlaybackOrder);
-        b.MarathonGroupBy.ShouldBe(a.MarathonGroupBy);
-        b.MarathonShuffleGroups.ShouldBe(a.MarathonShuffleGroups);
-        b.MarathonShuffleItems.ShouldBe(a.MarathonShuffleItems);
-        b.MarathonBatchSize.ShouldBe(a.MarathonBatchSize);
-        b.FillWithGroupMode.ShouldBe(a.FillWithGroupMode);
-        b.MultipleMode.ShouldBe(a.MultipleMode);
-        b.MultipleCount.ShouldBe(a.MultipleCount);
-        b.PlayoutDuration.ShouldBe(a.PlayoutDuration);
-        b.TailMode.ShouldBe(a.TailMode);
-        b.DiscardToFillAttempts.ShouldBe(a.DiscardToFillAttempts);
-        b.CustomTitle.ShouldBe(a.CustomTitle);
-        b.GuideMode.ShouldBe(a.GuideMode);
-        b.PreRollFillerId.ShouldBe(a.PreRollFillerId);
-        b.MidRollFillerId.ShouldBe(a.MidRollFillerId);
-        b.PostRollFillerId.ShouldBe(a.PostRollFillerId);
-        b.TailFillerId.ShouldBe(a.TailFillerId);
-        b.FallbackFillerId.ShouldBe(a.FallbackFillerId);
-        b.WatermarkIds.ShouldBe(a.WatermarkIds);
-        b.GraphicsElementIds.ShouldBe(a.GraphicsElementIds);
-        b.PreferredAudioLanguageCode.ShouldBe(a.PreferredAudioLanguageCode);
-        b.PreferredAudioTitle.ShouldBe(a.PreferredAudioTitle);
-        b.PreferredSubtitleLanguageCode.ShouldBe(a.PreferredSubtitleLanguageCode);
-        b.SubtitleMode.ShouldBe(a.SubtitleMode);
-        b.CollectionName.ShouldBe(a.CollectionName);
-        b.MultiCollectionName.ShouldBe(a.MultiCollectionName);
-        b.SmartCollectionName.ShouldBe(a.SmartCollectionName);
-        b.RerunCollectionName.ShouldBe(a.RerunCollectionName);
-        b.PlaylistName.ShouldBe(a.PlaylistName);
-        b.PlaylistGroupId.ShouldBe(a.PlaylistGroupId);
-        b.MediaItemName.ShouldBe(a.MediaItemName);
-        b.PreRollFillerName.ShouldBe(a.PreRollFillerName);
-        b.MidRollFillerName.ShouldBe(a.MidRollFillerName);
-        b.PostRollFillerName.ShouldBe(a.PostRollFillerName);
-        b.TailFillerName.ShouldBe(a.TailFillerName);
-        b.FallbackFillerName.ShouldBe(a.FallbackFillerName);
-        b.Watermarks.Select(w => (w.Id, w.Name)).ShouldBe(a.Watermarks.Select(w => (w.Id, w.Name)));
-        b.GraphicsElements.Select(g => (g.Id, g.Name)).ShouldBe(a.GraphicsElements.Select(g => (g.Id, g.Name)));
-        b.Name.ShouldBe(a.Name);
-        b.DurationEstimate.ShouldBe(a.DurationEstimate);
+        PropertyInfo[] properties = typeof(ScheduleItemResponseModel)
+            .GetProperties(BindingFlags.Public | BindingFlags.Instance);
+
+        // A stale exemption is a silent hole: it would exempt nothing while reading as a reviewed
+        // decision, and the property it once named would be compared or not by accident.
+        foreach (string exempt in RoundTripExemptProperties)
+        {
+            properties.Any(p => p.Name == exempt).ShouldBeTrue(
+                $"'{exempt}' is exempted from the round-trip comparison but is not a property of "
+                + $"{nameof(ScheduleItemResponseModel)}; remove the stale exemption or fix the name.");
+        }
+
+        var compared = 0;
+        foreach (PropertyInfo property in properties)
+        {
+            if (RoundTripExemptProperties.Contains(property.Name))
+            {
+                continue;
+            }
+
+            object? expected = property.GetValue(a);
+            object? actual = property.GetValue(b);
+
+            if (expected is IEnumerable expectedSequence and not string)
+            {
+                // Collection-valued members (WatermarkIds, Watermarks, GraphicsElementIds,
+                // GraphicsElements). The elementwise walk still delegates to each element's Equals,
+                // so it is value equality only because those elements are records
+                // (NamedIdResponseModel) or value types (the int id lists); a future element type that
+                // is neither would silently be compared by REFERENCE inside this loop. It is also order-sensitive, which is
+                // correct for these ordered lists but would be wrong for an unordered type such as
+                // a dictionary-valued property.
+                actual.ShouldNotBeNull($"{property.Name} was null on the round-tripped item");
+                var actualSequence = (IEnumerable)actual;
+                actualSequence.Cast<object?>().ToList()
+                    .ShouldBe(expectedSequence.Cast<object?>().ToList(), $"{property.Name} differs");
+            }
+            else
+            {
+                actual.ShouldBe(expected, $"{property.Name} differs");
+            }
+
+            compared++;
+        }
+
+        // Anti-vacuity, as a PIN rather than a floor. A `>=` floor lets properties vanish silently,
+        // which is the one-sided version of the both-directions rule this test is meant to embody.
+        // Comparing against the reflected count minus exemptions would be tautological — both sides
+        // come from the same reflection — so the expected number is written down and must be
+        // bumped deliberately in the same change that adds or removes a DTO field.
+        const int expectedComparedProperties = 55;
+        compared.ShouldBe(
+            expectedComparedProperties,
+            $"{compared} properties were compared, expected {expectedComparedProperties}; update "
+            + "this pin in the same change that alters ScheduleItemResponseModel's field list");
     }
 
     private async Task<int> SeedScheduleAndReferences(bool shuffleScheduleItems)

ErsatzTV.Tests/Services/ApiKeyProviderTests.cs

@@ -126,6 +126,70 @@ public class ApiKeyProviderTests
         key.ShouldMatch("^[0-9a-f]{64}$");
     }
 
+    // ---- Api:RequireKeyForReads, read through the REAL provider (ersatztv#779, detector F) ----
+    //
+    // Every other assertion about the read-gating posture goes through a hand-written
+    // FakeApiKeyProvider that is HANDED the bool (ApiAuthorizationFilterTests,
+    // ApiKeyEndpointRequiresKeyTests). Those fakes prove the FILTER reacts to the flag; they cannot
+    // see the line that DERIVES it, because they never run it. Until these tests, nothing in the
+    // suite constructed ApiKeyProvider at all, so a mistyped configuration key or a flipped default
+    // would have left the whole suite green while shipping anonymous reads (#280/#282).
+    //
+    // Api:WriteKey is set in every case purely so ResolveKey returns before touching the real
+    // FileSystemLayout.ApiKeyPath — the constructor would otherwise generate and persist a key into
+    // the live config volume. It is deliberately NOT the subject of these tests.
+    private static ApiKeyProvider ProviderWith(params (string Key, string Value)[] settings)
+    {
+        // The WriteKey entry is appended LAST so a caller cannot override it to empty. That is not
+        // hypothetical tidiness: an empty Api:WriteKey sends ResolveKey down the real path, which
+        // reads, generates and PERSISTS a key into the live config volume (FileSystemLayout
+        // .ApiKeyPath) from a unit test.
+        var withKey = new List<(string, string)>(settings) { (ApiKeyProvider.WriteKeyConfigurationKey, "test-key") };
+        return new ApiKeyProvider(Config(withKey.ToArray()), NullLogger<ApiKeyProvider>.Instance);
+    }
+
+    [Test]
+    public void Read_Gating_Is_Required_When_The_Setting_Is_Absent()
+    {
+        // The shipped default, and the case a fixture that simply OMITS the field would test by
+        // accident. Asserted explicitly so it is a pinned decision rather than a coincidence.
+        ProviderWith().RequireKeyForReads.ShouldBeTrue();
+    }
+
+    [TestCase("true")]
+    [TestCase("True")]
+    [TestCase("TRUE")]
+    public void Read_Gating_Is_Required_At_The_Explicit_Production_Value(string configured)
+    {
+        // The DENY path at the production value, which is the half #756 showed can stay invisible:
+        // the absent case behaving correctly says nothing about the configured one.
+        ProviderWith((ApiKeyProvider.RequireKeyForReadsConfigurationKey, configured))
+            .RequireKeyForReads.ShouldBeTrue();
+    }
+
+    [TestCase("")]
+    [TestCase("1")]
+    [TestCase("yes")]
+    public void A_Non_Boolean_Read_Gating_Value_Fails_Startup_Rather_Than_Reads(string configured)
+    {
+        // The fourth cell of the matrix, and the one an operator actually hits: `Api__RequireKeyForReads=`
+        // with nothing after it in a compose file, or a habitual `1`/`yes`. ConfigurationBinder returns
+        // the default ONLY for a null section value, so any present-but-unparseable string goes through
+        // BooleanConverter and throws. That is fail-CLOSED — the app refuses to start rather than
+        // quietly choosing a posture — and it is pinned here so a future switch to a lenient parse
+        // (TryParse with a fallback) cannot silently turn a typo into anonymous reads.
+        Should.Throw<InvalidOperationException>(() =>
+            ProviderWith((ApiKeyProvider.RequireKeyForReadsConfigurationKey, configured)));
+    }
+
+    [TestCase("false")]
+    [TestCase("False")]
+    public void Read_Gating_Is_Waived_Only_By_An_Explicit_Opt_Out(string configured)
+    {
+        ProviderWith((ApiKeyProvider.RequireKeyForReadsConfigurationKey, configured))
+            .RequireKeyForReads.ShouldBeFalse();
+    }
+
     [Test]
     public void Returns_A_Usable_Key_Even_When_Persist_Fails()
     {

docs/README.md

@@ -25,6 +25,8 @@ doc below, or that changes which sections a task signal points to.**
 | Auth / security-surface work | `docs/decisions/api-auth-security.md` |
 | CI / release pipeline work | `docs/ci-cd.md` + `docs/decisions/release-ci-governance.md` |
 | Proposing a new guard / CI check / regression test convention | `docs/defect-shapes-773.md` §4 (detector menu + the classes where no detector is plausible), then the two rules every guard must satisfy: `docs/decisions/records/testing/guard-derives-population-from-source.md` and `…/guard-ships-with-mutation-proof.md` |
+| Testing a surface gated by config / an env var / a credential | `docs/decisions/records/testing/deny-path-at-production-config-value.md` — cover the setting absent, at its production value, and each opt-out, and assert the DENY branch |
+| Touching a full-replace write path or a hand-built request object | `docs/decisions/records/testing/full-replace-asserts-field-list.md` — derive the field list from the DTO and assert set equality; reconcile by id where child state exists |
 | Adding / changing / deleting a guard file | `docs/guard-inventory.md` — every guard's row is machine-checked by `scripts/tests/test_guard_inventory.py`, so a new guard must acquire a row before the suite goes green |
 | Writing code that reads live Gitea/remote state and then acts on it | `docs/decisions/records/process/check-and-use-pins-a-version.md`, then `docs/remote-state-inventory.md` — a new executable under `scripts/` (**excluding `scripts/tests/`**), `.claude/hooks/`, `.husky/` or `.gitea/workflows/` must acquire a row there before `scripts/tests/test_remote_state_inventory.py` goes green |
 | Finding every site that references a symbol (multi-site fix/sweep) | `docs/local-lsp-tooling.md` — which surface answers, and why a delegated agent must be pointed at the `csharp-lsp` MCP tools rather than the `LSP` tool |

docs/decisions/README.md

@@ -192,10 +192,12 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
 | `spa.topbar-primary-action` | The TopBar's primary-action "+" button renders only when the active route declares a non-empty `primaryAction`, is wired (via a shared `usePrimaryAction` hook) only on single-unambiguous-create-flow list screens, and is dropped everywhere else rather than left as a dead/no-op button. | 2026-07-12 | [link](records/spa/topbar-primary-action.md) |
 | `spa.yaml-validator-textarea` | The YAML playout validator takes pasted YAML via a `<textarea>`, not a server-side file path, since the SPA has no filesystem access. | 2026-07-09 | [link](records/spa/yaml-validator-textarea.md) |
 | `startup.parallel-orientation` | A fresh session runs two concurrent tracks at startup — Orientation (`AGENTS.md`/`CLAUDE.md` → `docs/README.md` task-signal map → the active decisions catalog `docs/decisions/README.md`) and, only when no issue is named, Selection (`scripts/select-queue.sh N`, deterministic live-Gitea ranking). A named issue skips Selection entirely. ersatztv#237, the closed pickup tracker this replaces, is reduced to a single archival breadcrumb and MUST NOT be read for live state. | 2026-07-21 | [link](records/startup/parallel-orientation.md) |
+| `testing.deny-path-at-production-config-value` | Where behaviour is gated by a configuration value, an environment variable or a credential, the test matrix covers every value the surface will actually meet — the setting ABSENT, the setting at its PRODUCTION value, and each explicit opt-out — and it asserts the DENY branch, not only the allow branch. A fixture that OMITS the field tests the default and nothing else, so a fail-open reachable only through the configured value stays invisible however many tests are green (#756: thirty of them were). Two corollaries carry most of the weight. FIRST, a hand-written test double that is HANDED the resolved flag proves the CONSUMER reacts to it and says nothing about the line that DERIVES it; if no test constructs the real provider, a mistyped configuration key or a flipped default is unobservable to the whole suite. SECOND, the dangerous cell is whichever one production occupies, which is not always the explicit one: when the shipped default IS the permissive branch the absent case is the production case (#280's null `Api:WriteKey`), and when the default is fail-closed the configured value is the one nothing has exercised. Enumerate the cells before deciding which to test; do not infer the risky one from which is easier to write. This rule is NOT mechanically enforced and deliberately so — deciding whether a given test used the production value is a string predicate over test source, the class this repo has withdrawn twice. | 2026-08-21 | [link](records/testing/deny-path-at-production-config-value.md) |
 | `testing.e2e-cleanup-scope-by-pid` | An E2E harness or agent may only kill processes whose PIDs it captured at launch — capture the PID; whoever owns the lifecycle releases it from a `trap ... EXIT INT TERM`. Never `pkill -f "dotnet ErsatzTV.dll"` (or any pattern that can match a process this run did not start). A foreign listener is reported, not reaped. | 2026-07-25 | [link](records/testing/e2e-cleanup-scope-by-pid.md) |
 | `testing.e2e-local-fresh-config-dir` | Always point `scripts/e2e-local.sh` at a fresh config dir — leftover channels/schedules/DB rows bleed state between runs and corrupt assertions. (The *readiness-probe hang* this record was originally written about was fixed in #533; the fresh-dir rule stands on state-bleed grounds alone.) | 2026-07-21 | [link](records/testing/e2e-local-fresh-config-dir.md) |
 | `testing.enumerating-guard-identity-not-position` | A guard that cross-checks a hand-reviewed registry against call sites discovered across the whole repo must key each entry on properties INTRINSIC to the site — file, kind, and the value source text — and never on its absolute line or column. A registry keyed on position is a function of every other file in the repo, so a branch that never touches the guard can invalidate it; and because each PR is green against its own base, that failure is structurally invisible pre-merge and lands on `main` after review and after the merge gate. Dropping the position keeps every mutation the guard exists for — a NEW site, a REMOVED site and a CHANGED value each still fail, since each changes the identity multiset — and costs exactly ONE case, which must be stated rather than implied: a SAME-IDENTITY SUBSTITUTION within one file (delete a registered site, add a different unreviewed one with the same kind and value token, net-zero count) now passes. A REPORTED failure still prints the discovered line:column, because identity and diagnostics need not share a format. Comparison stays a MULTISET count rather than set membership, so two sites in one file sharing an identity must be discovered exactly that many times and a third occurrence still fails. A SCANNER test that asserts real AST positions against FIXED inline fixtures is the opposite case and keeps its line/column identity — it has no churn, because its input does not move. | 2026-07-27 | [link](records/testing/enumerating-guard-identity-not-position.md) |
 | `testing.fix-ships-a-witnessed-red-test` | A commit claiming to fix something may carry a `Proves: <pytest selector>` trailer; when it does, `scripts/prove-fix.sh` must show that selector GREEN with the fix and RED with the code side reverted, and CI enforces it per-PR. The trailer is opt-in — an unproven commit is allowed — but a claimed proof that does not hold fails the build. | 2026-08-16 | [link](records/testing/fix-ships-a-witnessed-red-test.md) |
+| `testing.full-replace-asserts-field-list` | Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field's value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today's schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions. | 2026-08-21 | [link](records/testing/full-replace-asserts-field-list.md) |
 | `testing.guard-derives-population-from-source` | A guard that asserts a COMPLETENESS property enumerates its population from a machine-readable authoritative source — the enum, the generated OpenAPI document, the parsed workflow YAML, the provider list — and asserts SET EQUALITY in BOTH directions against it. It may not narrow that population with a filter, a `Where`, a `grep` or an early `continue` before the assertion, because a filter cannot see the member that is MISSING: the member whose absence is the defect is precisely the one the predicate excludes. A hand-written literal list of members is the same defect in slower motion — a filter frozen at authoring time, correct on the day it was written and unable to report the day it stopped being. Two boundaries bound the rule rather than weaken it. FIRST, filtering to select the SUBJECT of a PER-MEMBER property is legitimate and is not this defect: the excluded members satisfy the property vacuously, so the filtered walk and the whole walk assert the same thing (`ToolCatalogTests.Every_Query_Parameter_Should_Be_A_Declared_Property` filters to tools that declare query parameters, and a tool declaring none has nothing to check). The defect is filtering the population before a COMPLETENESS claim, which is what makes an absent member unrepresentable (#757 filtered on `QueryParameters is {Count: > 0}` and so could not see a tool that should have declared one and did not). SECOND, a population of VALUES always has an external authoritative source and this rule applies directly; a population of SITES IN CODE has no such list, needs find-all-references tooling, and is tracked separately in #777 — do not stretch a set-equality assertion over it. Distinguish the guard SCOPE (which subsystems it covers — a reviewed policy choice, legitimately hand-written) from the guard POPULATION (the members inside that scope — always derived). When the scope itself MIRRORS an authoritative source, the mirror needs its own equality check or a dated staleness marker, or the guard is complete within a scope that has silently gone stale. The canonical worked example in this repo is `ToolCatalogTests.Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`; the canonical residual gap is `MARKED_JOBS` in `scripts/tests/test_ci_dropped_step_guard.py`. | 2026-08-13 | [link](records/testing/guard-derives-population-from-source.md) |
 | `testing.guard-ships-with-mutation-proof` | A guard is not considered tested because a test involving it passes. It ships with a MUTATION PROOF: remove or disarm THAT GUARD'S CLAUSE ALONE, and a NAMED test must go red. ONE NAMED EXCEPTION, with its limits, because the rule degenerates without it: where the guard IS a test (a checker enforcing a repo invariant, with no separate script behind it), disarming it makes it ABSENT rather than red, so the proof is the contrapositive — INTRODUCE THE DEFECT THE GUARD EXISTS TO CATCH into an isolated copy of the guarded artifact, and the named test must go red. That is a mutation of the guarded SYSTEM rather than of the assertion, and it is admissible ONLY for checker-guards and ONLY when the mutation was executed and witnessed. It is NOT a licence to grade an ordinary script-guard MUTATION for having a bad-input test: feeding a script an input its clause rejects is BEHAVIOUR-ONLY, which is what three rows were regraded for. A file-level grade under this exception covers the clause its cited case actually mutates, not every assertion that later lands in the same file; clause-level grading is tracked in #790. Three things this excludes, each of which has already shipped here as a green suite over a dead check. FIRST, a behavioural test — one that feeds the guard a good input and a bad input and checks it passes and fails — proves the guard REACTS, never that it is LOAD-BEARING; #685 had two guards on one condition where deleting either left the whole suite green while every behavioural test passed. SECOND, mutating the WHOLE FILE does not count (#510): a whole-file revert cannot show that a test reaches a particular clause, so the mutation must target the clause. THIRD, the guard being WIRED is not the guard RUNNING — #631's suite was invoked by no CI job, #751's step was dropped by the runner and the job reported success in 6s against a normal 14-17s, and #719's new logic was never connected to stdin. Every guard also carries an ANTI-VACUITY assertion, because the characteristic failure of a completeness check is reporting that it proved everything while its population was empty. Mechanical enforcement is possible for the BOOKKEEPING and not for the JUDGEMENT, and the split is the decision: `docs/guard-inventory.md` lists every guard file with its Kind, its Proof class (`MUTATION`/`BEHAVIOUR-ONLY`/`NONE`) and a `file::function` ref, and `scripts/tests/test_guard_inventory.py` derives the guard population from the filesystem and the call sites, asserts SET EQUALITY against the rows, and resolves every claimed ref to a real `def`. So a new guard cannot ship unclassified and a renamed test cannot leave a row silently claiming coverage. What stays with review, and is stated rather than papered over: nothing checks that a row claiming `MUTATION` is telling the truth. | 2026-08-13 | [link](records/testing/guard-ships-with-mutation-proof.md) |
 | `testing.hook-reports-its-own-execution` | Every script in `.claude/hooks/` sources `scripts/hook-fire-log.sh` and calls `etv_hook_fire_begin <its-own-name> <label> <capture\|stream>` as its FIRST act, before anything reads stdin. Two records are appended per invocation — a `fire` record on entry and an `exit` record carrying the exit status and the decision — to a session-scoped JSONL log. THE DECISION IS READ FROM WHAT THE HOOK ACTUALLY EMITTED, never declared by the hook author: Claude Code hooks (`capture` mode) always exit 0 and communicate by PRINTING JSON, so their stdout is diverted and replayed, and the recorded decision is parsed from those bytes; git hooks (`stream` mode) decide by EXIT CODE and their stdout is live progress text a human is watching, so it is not diverted and the decision is the status. That split is not a tuning knob — capturing a slow pre-push hook's output would hold it back until the end and read as a hang, and inferring a git hook's decision from absent JSON would put the report back into the guessing business this record exists to end. The population is DERIVED from `.claude/hooks/*.sh` by `scripts/tests/test_hook_fire_log.py`, so a new hook is uninstrumented-and-red rather than silently unobserved, and the report lists every hook that EXISTS rather than every hook that appears in the log — a report built from the log alone can only show hooks that fired, which makes the never-fired hook, the one finding worth having, invisible. THE INSTRUMENTATION MUST BE INVISIBLE TO THE HARNESS, and this is the load-bearing half: it sits in the stdin and stdout path of the most authoritative guards in the repo, so a differential test drives EVERY hook with and without it over a payload matrix and demands byte-equal stdout and equal exit status. It fails OPEN in exactly one direction — if the log cannot be written the hook behaves exactly as before — because observability that breaks a guard is worse than the blindness it replaces. Two mechanical traps are pinned by tests rather than left to care: stdout must be replayed from the FILE, since `out=$(cat f)` strips trailing newlines and delivers a guard's JSON one byte short with no parser anywhere to complain; and stdin must never be slurped when it is a TTY, because an interactive `git commit` hands its hooks a terminal and `cat` would block forever, hanging the commit the instrumentation was added to observe. | 2026-08-14 | [link](records/testing/hook-reports-its-own-execution.md) |

docs/decisions/records/testing/deny-path-at-production-config-value.md

@@ -0,0 +1,87 @@
+---
+key: testing.deny-path-at-production-config-value
+title: '2026-08-21 — A config-gated guard is tested on its DENY branch at the value production actually runs (#779)'
+status: active
+since: '2026-08-21'
+supersedes: none
+superseded-by: none
+rule: 'Where behaviour is gated by a configuration value, an environment variable or a credential, the test matrix covers every value the surface will actually meet — the setting ABSENT, the setting at its PRODUCTION value, and each explicit opt-out — and it asserts the DENY branch, not only the allow branch. A fixture that OMITS the field tests the default and nothing else, so a fail-open reachable only through the configured value stays invisible however many tests are green (#756: thirty of them were). Two corollaries carry most of the weight. FIRST, a hand-written test double that is HANDED the resolved flag proves the CONSUMER reacts to it and says nothing about the line that DERIVES it; if no test constructs the real provider, a mistyped configuration key or a flipped default is unobservable to the whole suite. SECOND, the dangerous cell is whichever one production occupies, which is not always the explicit one: when the shipped default IS the permissive branch the absent case is the production case (#280''s null `Api:WriteKey`), and when the default is fail-closed the configured value is the one nothing has exercised. Enumerate the cells before deciding which to test; do not infer the risky one from which is easier to write. This rule is NOT mechanically enforced and deliberately so — deciding whether a given test used the production value is a string predicate over test source, the class this repo has withdrawn twice.'
+signals: 'deny path at the production value · fixture omits the field tests only the default · fail-open by default · parametrise the whole config matrix · absent versus configured versus opt-out · a fake that is handed the flag never runs the line that derives it · nobody constructs the real provider · `Api:WriteKey` · `Api:RequireKeyForReads` · `ERSATZTV_ALLOW_WRITES` · `memory_pressure` absent · `xxd` absent on the runner · paths: `ErsatzTV/Services/ApiKeyProvider.cs`, `ErsatzTV.Tests/Services/ApiKeyProviderTests.cs`, `ErsatzTV.Tests/Filters/ApiAuthorizationFilterTests.cs`, `docs/guard-inventory.md` · issues: #779, #773, #756, #768, #751, #647, #282, #280'
+mechanics: 'Detector F of `docs/defect-shapes-773.md` §4. No CI check implements it; the population of config-gated guard FILES is already derived and machine-checked by `scripts/tests/test_guard_inventory.py`, and this rule is the judgement layered on top of that population.'
+---
+
+**The rule exists because the green suite is the symptom, not the reassurance.** #756's fixture
+omitted a field. The unset behaviour was correct, thirty tests said so, and a fail-open reachable
+only through the *production* value sat behind them untouched. Nobody skipped a test; they tested
+the cell that was easy to construct and read the result as coverage of the setting.
+
+**The three cells, because naming them is most of the fix.** A gated surface has a value that is
+absent, a value production sets, and one or more explicit opt-outs. Which cell is dangerous is a
+property of the surface, not a constant, and this is where the reasoning usually goes wrong:
+
+- **Default permissive.** #280 — API writes fail *open* when `Api:WriteKey` is null or empty, and
+  null/empty was the shipped default. Here the absent cell *is* production, and a test that
+  configures a key to exercise the guard has stepped off the dangerous cell to do it.
+- **Default fail-closed.** `Api:RequireKeyForReads` defaults to `true`. Now the absent cell is safe
+  and the configured cells are the unexercised ones.
+
+**The variant this repo was actually carrying, found by #779's audit and worth more than the
+principle.** Every assertion about the read-gating posture ran through a hand-written
+`FakeApiKeyProvider` that is *handed* the boolean — `ApiAuthorizationFilterTests`,
+`ApiKeyEndpointRequiresKeyTests`. Those tests are good ones: they cover the deny branch, at
+`requireKeyForReads: true`, which is the production value. And they could not see a defect in
+`ApiKeyProvider` at all, because **no test in the repository constructed it.** The single line
+deriving the posture from configuration — `configuration.GetValue(RequireKeyForReadsConfigurationKey,
+true)` — had never been executed by a test. Mistype the key, flip the default, and every one of
+those green deny-path assertions stays green while the shipped build serves anonymous reads.
+
+That is the shape to internalise: *the fake was accurate, the fixture was at the production value,
+and the coverage was still absent* — because the double stood exactly where the untested code was.
+Ask not only "which value did the test use" but "which code did the value flow through".
+
+**Fixed here**, by constructing the real provider across the matrix in `ApiKeyProviderTests`:
+absent, `true`/`True`/`TRUE`, `false`/`False`, and — the cell the first draft of this record forgot
+while claiming "the whole matrix" — a present-but-non-boolean value. `ConfigurationBinder` returns
+the default ONLY for a null section value, so `""`, `1` or `yes` reach `BooleanConverter` and throw
+`InvalidOperationException` at construction (measured). That is fail-CLOSED: an operator who writes
+`Api__RequireKeyForReads=` with nothing after it in a compose file gets a refusing app, not silent
+anonymous reads. It is pinned so a later switch to a lenient `TryParse` cannot turn a typo into a
+posture change. `Api:WriteKey` is set in those cases only so
+`ResolveKey` returns before touching the live config volume; that is a test-isolation detail and
+deliberately not their subject.
+
+**What the mutation proof here does and does not show, because the honest version is weaker than
+the headline.** Flipping the shipped default `true` -> `false` reddens
+`Read_Gating_Is_Required_When_The_Setting_Is_Absent` and only that test. That establishes the
+*absent* cell is load-bearing. It also shows the three explicitly-configured `true`/`True`/`TRUE`
+cases — the cells this rule is named for — are NOT independently load-bearing against that mutation:
+a mistyped configuration key is caught, but by the `false` cases, not the `true` ones. A mutation class that would
+redden `"True"`/`"TRUE"` alone is a hand-rolled case-sensitive parse replacing `GetValue`. They are kept as behaviour coverage of the real binder rather than deleted, and the
+distinction is written down instead of being smoothed over: a matrix that is *complete* is not
+thereby a matrix in which every cell *discriminates*.
+
+**Why no check enforces this.** Deciding whether a test exercised a production value means reading
+test source and judging intent — a string-matching predicate over code, which
+`docs/defect-shapes-773.md` §4 argues against and which this repo has withdrawn twice after
+round-churn (#629, #774). What *is* mechanical already exists: `test_guard_inventory.py` derives the
+guard-file population and asserts set equality, so this rule has a maintained list to be applied to.
+The judgement stays with review, and saying so is the honest position rather than shipping a
+keyword matcher that would manufacture the confident-but-empty coverage the rule is about.
+
+**Environment counts as configuration.** The permissive branch is often reached by a tool being
+absent rather than a setting being wrong, and it looks identical from inside. `pretooluse-bom-guard.sh`
+detected BOMs with `xxd`, which ships with vim and was **absent on the Linux CI runner**, so the
+comparison never matched and every BOM was allowed in silence while the hook fired on every commit
+(`docs/guard-inventory.md`). `pretooluse-agent-ram.sh` has the same construction today —
+`memory_pressure` is macOS-only and its absence yields `exit 0`, allowing unbounded fan-out — and its
+deny branch is exercised by no test. It is left to #785, which owns mutation proofs for the unproven
+guards; splitting one guard's proof across two issues is how a row ends up claiming coverage twice
+and holding none.
+
+**Audit residue, so the gaps are tracked rather than implied closed.** Of the config-gated guards
+enumerated from `git ls-files` over `.claude/hooks/`, `.husky/`, `scripts/` and `.gitea/workflows/`:
+the C# auth surface is now covered end to end; `docker-build.yml`'s dropped-step fail-open is covered
+by `test_ci_dropped_step_guard.py`; and the untested permissive branches that remain are all hook
+`exit 0` escape hatches (`ETV_ALLOW_DIRTY_PUSH`, `ETV_SKIP_REBASE_CHECK`, absent credentials in
+`prepush-donewhen.sh`, absent `python3` in `decisions-guard.sh`) — each already a row in
+`docs/guard-inventory.md` with proof `NONE`, and so already inside #785's scope.

docs/decisions/records/testing/full-replace-asserts-field-list.md

@@ -0,0 +1,138 @@
+---
+key: testing.full-replace-asserts-field-list
+title: '2026-08-21 — A full-replace path asserts its COMPLETE field list against the DTO, and reconciles by id where child state exists (#779)'
+status: active
+since: '2026-08-21'
+supersedes: none
+superseded-by: none
+rule: 'Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field''s value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today''s schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions.'
+signals: 'full replace asserts its field list · hand-maintained mirror drifts by one field · reconcile by id not delete and reinsert · 200 and the field is gone · add filter and remove filter must share a key · fill-group progression reset by a PUT · derive the comparer from the DTO · a lossless round-trip test that is itself a hand-copied list · anti-vacuity PIN not a floor on a reflective walk · stale exemption must still name a real property · a complete comparer over a hand-written fixture · universal negative in a record is a trap · paths: `ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs` · issues: #779, #773, #757, #754, #500, #252'
+mechanics: 'Detector G of `docs/defect-shapes-773.md` §4. Enforced per-site by a reflective comparison over the DTO plus a written-down count pin, not by a repo-wide check — see the record body for why a global one is not proposed. The exemption set is empty today; the machinery guarding exemptions remains for the first one that earns its place.'
+---
+
+**This is `testing.guard-derives-population-from-source` applied to a write path, and it is worth its
+own record because the population here is not obviously a population.** A guard asserting
+completeness over an enum or a tool catalog visibly has members to enumerate. A PUT handler and its
+test look like ordinary code with a lot of fields in them — and the fields *are* the population.
+#754 is what that looks like when nobody notices: a hand-maintained wrapper one field short of a
+28-property DTO, returning 200, clearing the value, and surfacing hours later as missing pixels.
+
+**The repo already had the right answer in one place.** `ToolCatalogTests`'s write-tool test reads
+the accepted request-body fields out of the generated OpenAPI document, walks the tool catalog
+unfiltered, and compares both directions — an accepted field a tool never declares and a declared
+field the API does not accept are opposite defects and are reported as such. That test is the fix
+for #754/#757 and it is the shape to copy, not the prose above it.
+
+**Where #779's audit found the mechanism still live: in the test whose job was to catch it.**
+`ScheduleItemResponseRoundTripTests` is the release gate for the flat schedule-item DTO — a
+GET → map → PUT → GET fixed point asserting the round trip is lossless. Its comparison was
+`AssertSemanticallyEqual`, a hand-written run of `b.X.ShouldBe(a.X)` lines. On audit it covered every
+property but `Id` — so it was *complete*, and had no way to say so: add one more field to
+`ScheduleItemResponseModel` and it is compared by nobody, the round trip drops it, and the test named
+for losslessness stays green. The gate against the drift was itself the drift, one altitude up.
+
+It now derives the property set by reflection over the DTO, and the exemption set is **empty** —
+which is the most useful thing this exercise produced. The first version exempted `Id`, reasoning
+that the PUT replaces the item set so B's rows are new rows with new ids. That is the opposite of
+what the endpoint does: the request forwards each `Id`, so the handler takes its id-based reconcile
+path and updates rows in place. The exemption was discarding an assertion on a rationale that
+contradicted the code — the failure this record is about, committed inside the fix for it.
+
+Two further details. Every exempted name is asserted to still *exist* on the record, because a stale
+exemption exempts nothing while reading in review as a considered decision. And the anti-vacuity
+check is a **pin** (`ShouldBe(55)`), not a `>=` floor: a floor lets properties vanish silently, which
+is the one-sided version of the both-directions rule this record invokes.
+(`testing.guard-derives-population-from-source` endorses a floor on its canonical example; this is
+strictly stronger, not a contradiction of it.) Comparing against the
+reflected count minus exemptions would be tautological — both sides come from the same reflection —
+so the number is written down and must be bumped deliberately. It was obtained by raising the pin and
+reading the failure, not counted off the source.
+
+**The `Id` exemption, and the paragraph that replaced it three times.** Two mutations of the
+round-trip fixture, both executed:
+
+| Mutation of `ToReplaceCommand` | Result |
+|---|---|
+| `null` for **every** id | GREEN — `requestIds.Count == 0` takes the positional fallback, which also reuses each row |
+| `null` for **index 0 only** | RED, `Id differs` — the id-based branch deletes the unreferenced row and inserts the id-less item as new |
+
+So comparing `Id` discriminates, and the exemption was discarding a real assertion. It is still not a
+substitute for `ReplaceProgramScheduleItemsReconcileTests`
+(`Reorder_ById_Should_Move_State_With_The_Logical_Item_Not_The_Slot`,
+`Insert_ById_In_Middle_Should_Keep_Existing_Ids_And_State`), which pass real ids and pin that state
+moves with the logical item rather than the slot.
+
+**The transferable part is not the `Id` detail.** This paragraph was written four times. Every draft
+paired a correct observation with a confident causal story, and three of those stories were
+contradicted by the code — including one that generalised a single measured case into a universal
+("no id-less mutation can redden it") that a *partial* payload falsifies. The rule that survives:
+**state the measurement and the code path you actually read; do not generalise from one executed case
+to a class of cases, and do not explain a mechanism you did not measure.** A decision record is
+exactly where such a story does the most damage, because it reads as checked.
+
+
+**A completeness guarantee on the COMPARER is not one on the FIXTURE.** The seed builder feeding this
+round trip is still a hand-written list. A new nullable field is now reflected and compared — as
+`null` against `null` — until someone also seeds it, so the round trip can still drop it while this
+test stays green. Detector G is satisfied for the comparison, not for the fixture; that residue is
+real and is named here rather than left implied.
+
+**On the second clause, the audit's finding was a negative one — and the first version of it was too
+strong, which is exactly why negatives are dangerous to record.** Four handlers — playlist items,
+block items, deco-template items, template items — `RemoveRange` their children and rebuild, the
+shape #252 was fixed for. The claim written here first was that no persisted state is keyed to those
+item ids. That is false, and the counterexamples live in the same subsystem the record cites
+(verified 2026-08-21):
+
+- `PlayoutItem.SchedulingContext` is a persisted column holding a serialized
+  `BlockSchedulingContext(BlockId, **BlockItemId**, Enumerator, Seed, Index)`.
+  `ReplaceBlockItemsHandler` deletes and reinserts, so the stored `BlockItemId` no longer refers to
+  the row it was written for. `ProcessSchedulingContextHandler` looks it up: it either resolves
+  nothing and falls back to `new BlockContextBlockItem(id, null, null)`, or — if the id was reused by
+  a different item — resolves to the WRONG one. Which of the two happens was not measured, so the
+  claim here is only that the reference is orphaned; the second case is the worse one and is the
+  reason not to write this off.
+- `PlayoutItem.GuideGroup` is set to `effectiveBlock.TemplateItemId` and persisted, while
+  `ReplaceTemplateItemsHandler` deletes and reinserts. `GuideGroup` is the XMLTV grouping key, so a
+  reused id colliding with one already persisted on older items is a HAZARD rather than a
+  demonstrated consequence — unmeasured. Note the block path groups by
+  `{GuideStart, GuideFinish, GuideGroup}` together (`ChannelGuideProjector.ProjectBlock`), a plain
+  `GroupBy` and NOT contiguity-sensitive, so two non-adjacent runs sharing the triple would merge;
+  the contiguity logic is the flood projector, which never sees a `TemplateItemId`-derived
+  `GuideGroup`.
+
+The accurate statement is narrower: **no progression or enumerator state is keyed to those ids**, so
+the replace does not reset scheduling position — but two persisted *diagnostic/grouping* fields do
+embed them and are orphaned by it. Degradation, not data loss. The evidence originally offered
+(`BlockKey` keys off parent id plus `DateUpdated` ticks, never `BlockItem.Id`) is true and supports
+only that narrower claim. The other full-replace handlers reconcile by id rather than delete-and-reinsert:
+`ReplaceProgramScheduleItemsHandler` (#252's scoped positional/no-op reconcile),
+`ReplacePlayoutAlternateScheduleItemsHandler` (over `ProgramScheduleAlternate`) and
+`ReplacePlayoutTemplateItemsHandler` (over `PlayoutTemplate`) — neither of the last two touches the
+`TemplateItem` rows listed among the four above, despite the names.
+
+Only the FIRST of those three is demonstrably protecting identity-keyed state: `PlayoutScheduleItemFillGroupIndex`
+holds an FK to `ProgramScheduleItemId`, whereas `PlayoutTemplateId` and `ProgramScheduleAlternateId`
+appear nowhere in the solution, model snapshot included (verified 2026-08-21). The other two reconcile
+by id for their own reasons, and attributing that to progression state would be a causal story the
+schema does not support.
+
+The general lesson is the one this record nearly failed to learn: a universal negative in a durable
+document reads as a checked fact and talks the next reader out of checking. Bound it to what was
+actually enumerated, and date it.
+
+
+**Why this is not a repo-wide mechanical check.** Finding "every full-replace path" means deciding
+which PUT/POST handlers replace a whole collection versus patch fields — a judgement about intent,
+and the population is *sites in code*, which `testing.guard-derives-population-from-source` explicitly
+scopes out as needing find-all-references tooling this repo does not have (#777). A global lint would
+be a name matcher over handler classes ending `Replace…Handler`, which misses the ones that do not
+and flags the ones that do it correctly. Per-site derivation, applied when a full-replace path is
+touched, is the enforceable version.
+
+**Residue, tracked rather than implied closed.** The SPA builds request objects field-by-field in six
+screens (`normalizeForSave` and its equivalents). A *required* field omitted there fails `tsc`; an
+*optional* field added to the DTO and not carried through compiles clean and drops silently — #754's
+mechanism narrowed to optional fields, with no equivalent of the MCP write-tool test on the SPA side.
+That is the highest-probability recurrence in the repo today and is filed separately rather than
+folded in here.