diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs index 0d34bd88d..fc2754f83 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs +++ b/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 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().ToList() + .ShouldBe(expectedSequence.Cast().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 SeedScheduleAndReferences(bool shuffleScheduleItems) diff --git a/ErsatzTV.Tests/Services/ApiKeyProviderTests.cs b/ErsatzTV.Tests/Services/ApiKeyProviderTests.cs index a78a906f7..12353011d 100644 --- a/ErsatzTV.Tests/Services/ApiKeyProviderTests.cs +++ b/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.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(() => + 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() { diff --git a/docs/README.md b/docs/README.md index 726a52a29..f9a933a97 100644 --- a/docs/README.md +++ b/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 | diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 939574815..4af092e6e 100644 --- a/docs/decisions/README.md +++ b/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 `