fix(823,824): a scheduling NULL collection reads as UNRESTRICTED and is guarded at both read sites; the Elastic indexer gets its own mutation proof #879
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#879
Reference in New Issue
Block a user
Closes both #701 deferrals together, because both rewrite the same decision record.
#823 — is a runtime null reachable on the six scheduling collection columns?
Yes. Measured against a real
TvContexton both providers (SQLite, and MySQL 8.4 on an ephemeral server), because the available reasoning pointed the wrong way: the two converters differ on their read side, so a NULL row was expected to behave differently per column. Neither converter runs — EF does not invoke a value converter for a NULL column at all, so all six materialize as CLRnullandIntCollectionValueConverter's null-to-empty branch is dead on this path. Unguarded,AlternateScheduleSelector's.Containscalls throwNullReferenceException.A NULL reads as UNRESTRICTED, not empty. The first draft had this backwards. It is decided by the one NULL reachable without any code writing one: SQLite's
20240113140741_Add_PlayoutTemplate_DaysOfMonthis anAddColumnwithnullable: trueand nodefaultValue, so rows inserted before it hold NULL and by construction had no day-of-month restriction. Reading that as empty would invert the row's meaning and silently stop the template applying.Two read sites, not one. Guarding only the selector would have left the entity→DTO mappers unguarded, and those feed the SPA — which spreads the collection (
[...template.daysOfMonth]→TypeError) and runsappliesToDate, an exact port ofGetScheduleForDate. Both mappers now substitute the same defaults, so the preview agrees with what is scheduled. Neither guard is assigned back onto the entity.Reachability, stated precisely: all six are
nullable: trueon both providers, but five were present atCreateTable, so a NULL there still needs code to write one — and on MySQL there is no code-path-free NULL for any of the six. The write path accepts a null; no caller supplies one today.#824 — ElasticSearchIndex.UpdateSong had no regression test
Option 1 from the issue shipped, needing no new package:
InMemoryRequestInvokeris public in the pinnedElastic.Transport, injected into the private_clientthe way #701 injects the LuceneIndexWriter. The canned response must carryX-Elastic-Product: Elasticsearchor the client's product check throws intoUpdateSong's catch — which is how this fixture first failed, caught by theThrowOnWarningLoggerrather than passing vacuously.No production change in
ElasticSearchIndex.cs— #824 is coverage only.Six mutations executed, each disarming its own clause alone
??=restored inElasticSearchIndexonlymetadata.Artists should be null but was []), Lucene fixture green — the #824 hole demonstratedDaysOfWeekguard disarmed in the selectorDaysOfMonth/MonthsOfYearunaffected, so each clause is independently load-bearingDaysOfMonthguard disarmed inPlayouts.Mapper[Test]namedVerification
Local gate with the MySQL lane armed:
ErsatzTV.Tests2096 passed / 0 skipped,Core.Tests693/1,Infrastructure.Tests114,Architecture.Tests7,Scanner.Tests1504 — 0 failures each.scripts/tests1228 passed / 2 skipped. Format clean, BOM check over the touched set with the population count asserted.decisions_validateOK. No golden file changed.Reviewed cold by two independent reviewers (one cross-family); every finding addressed, including the semantic reversal and the mapper sweep, which both came from review.
Fixes #823
Fixes #824
Review-verdict: MERGEABLE @
95b2700Two independent cold reviews, one cross-family (GPT-5.6) and one worktree-isolated Opus. 17 findings between them, no Blockers; every actionable finding addressed on this head. The two most important both came FROM review and changed the design: the NULL semantics were reversed from empty to unrestricted, and the sweep was extended from the selector alone to the entity-to-DTO mappers that feed the SPA. Prose claims that review measured as false (reachability on both providers, a non-API caller existing) were corrected rather than softened. Every new guard carries an executed clause mutation: six applied, six witnessed red, each scoped so its siblings stayed green. Caveat stated rather than hidden: a third re-review pass over the fix delta was dispatched and did not return, so that delta was verified by the author instead - field sweep complete, migration enumeration re-derived, DTO consumers enumerated (SPA only), goldens unchanged.
Follow-up commit (the branch is pushed, so not an amend). Two more cold reviews landed on the previous head; both reported 0 Blocker and 0 High, and these are their Mediums and Lows. Each fix carries its own witnessed mutation. 1. The selector fixture could not tell the fix from a much broader one. Every null test set a NULL and expected the item SELECTED, so all of them pass equally under "NULL means unrestricted" and under "any NULL makes this item match unconditionally" -- a refactor short-circuiting the whole date check on any null kept them green. Added the discriminating control: a NULL DaysOfWeek paired with MonthsOfYear = [1] against a MARCH date must be None. Only the narrow reading passes. 2. A_Null_Item_Does_Not_Disturb_Selection_Of_A_Later_Item never measured its own docstring. The nulled item was unrestricted and at Index 0, so it always won and the second item was never evaluated -- the stated invariant ("a null on the first item must not decide the second") went unmeasured while the test passed. Split into two: one where the nulled item genuinely does not match, which measures that the loop CONTINUES; and one that pins the index-order win separately. 3. The empty-preservation control existed for one of two identical mappers. The anti-mutant test for "empty or null becomes All*" covered only Playouts.Mapper; Scheduling.Mapper is a byte-identical triple in another file and had none, so a defensive edit to it alone would have rewritten a deliberately-empty user selection to 1..31 with the suite green. That is the one-helper-two-callers shape this repo has been bitten by. Added the matching test. 4. The coverage guard's [Test] clause did not check what its message claimed. GetMethods() without BindingFlags returns INHERITED methods, so a fixture that merely subclasses another satisfied it while driving the wrong indexer -- and Values.Distinct() cannot catch that, since the two Types differ. It also matched TestAttribute alone, so a future fixture written as [TestCase] would have falsely reddened, and it accepted an [Explicit]/[Ignore]d fixture that never runs, which is the "wired is not running" failure the guard exists to prevent. Now DeclaredOnly, the full test-method vocabulary, and Explicit/Ignore rejected at both method and fixture level. 5. Three residuals recorded on media.nullable-primitive-collection-mutation that the previous head asserted nothing about: - the LOUDNESS change, worst for an all-three-NULL ProgramScheduleAlternate, which now matches unconditionally and shadows the default schedule where it previously threw. Unreachable today, and a choice over an unreachable state rather than a measured requirement -- said plainly. - the normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row persists All*() over EVERY NULL row in that playout, and afterwards "the operator selected all 31" and "this is a legacy row" are indistinguishable. An ordinary user action closes that door. - the WRITE side disagrees with the READ side about what ABSENCE means: an omitted daysOfWeek normalizes to [] ("never applies") while a NULL column reads as unrestricted, so an API client gets HTTP 200 and a row that silently never fires. Filed as #880 rather than folded in here, because a client omitting a field on a write is a different question from what a legacy NULL meant. Two more mutations executed, both witnessed: - DaysOfWeek guard disarmed in Scheduling.Mapper -> 1 red, 3 green. - A fixture with no DECLARED test named in the covered set -> coverage red. Local gate (MySQL lane armed): ErsatzTV.Tests 2097 passed / 0 skipped, Core.Tests 695/1 -- 0 failures. Format clean, no BOM on the touched set with the population count asserted. decisions_validate OK. Refs #823 Refs #824 Refs #880 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TWRound-three review findings. One HIGH, and it was in the durable artifact rather than the code. THE HIGH: the record stated the shipped reading and its inverse. The semantic reversal (empty -> unrestricted) rewrote the residual and the write-half of `media.nullable-primitive-collection-mutation` but left the ORIGINAL reasoning standing two sentences earlier: "A null reads as EMPTY, so the item matches nothing"; "the REJECTED alternative was the All*() set"; "SKIPPING the row is the conservative repair". The shipped code is `?? AllDaysOfWeek()` -- precisely the alternative that passage calls rejected. The previous commit then inserted residual (1), which reasons entirely FROM the All*() reading, two sentences after the sentence denying it. That is worse than a stale comment. A session resolving this key -- or reading the MemPalace mirror, which carries `rule:` verbatim -- would have been told to write the guard the other way, i.e. talked into the `[]` reading that the same record elsewhere argues is data corruption one save later. Replaced the whole passage, then swept the record for every other mention of the empty reading rather than trusting the one replacement: the only survivor is the new sentence that records EMPTY as the rejected alternative, which is the direction that stops it being re-adopted. THE MEDIUM: one arrangement did not close the hole it claimed to. The discriminating control added last commit nulls DaysOfWeek against a restrictive MonthsOfYear. It excludes "any NULL matches unconditionally" only for that dimension. The review supplied the surviving mutant -- `if (item.MonthsOfYear is null) { return item; }` ahead of the checks -- and traced it green through all nine tests. Verified by EXECUTION, not by reading: applied to the previous fixture it passes; applied now it FAILS 1 of 11. Each of the three dimensions is now nulled against a restriction on a different dimension. The rest, all from the same round: - The coverage guard's test detection listed attribute TYPES, and each list falsely reddened whatever it omitted: TestAttribute alone missed [TestCase], and the three-type replacement missed [Theory]. Now decided by NUnit's own ITestBuilder/ISimpleTestBuilder interfaces, which cannot fall behind the vocabulary. It also dropped BindingFlags.Static (GetMethods() defaults to including it), which would have falsely reddened a static test method. - The same guard accepted an ABSTRACT fixture -- NUnit never instantiates one. The indexer population already filtered IsAbstract; the fixture side now mirrors it. - The record's `mechanics:` still described the old `[Test]`-only clause, in the same file the change edited. - An <inheritdoc> made the ProgramScheduleAlternate empty-case test inherit a docstring written from the PlayoutTemplate test's viewpoint. Two more mutations executed: - `if (item.MonthsOfYear is null) return item;` -> 1 red, 10 green. This is the mutant that survived the previous head; it no longer does. - an abstract type named in the covered set -> coverage guard red. Local gate: ErsatzTV.Tests 2091 passed / 6 skipped (the three fixtures' MySQL halves, skipping visibly without ETV_TEST_MYSQL_CONNECTION), Core.Tests 697/1 -- 0 failures. Format clean, no BOM on the touched set. decisions_validate OK. Refs #823 Refs #824 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW?? []survived, and my "swept it" claim was falsebecause, a duplicatedpaths:Review-verdict: MERGEABLE @
9685132Five independent cold review rounds, one cross-family (GPT-5.6) and four worktree-isolated Opus, across four heads. Final round confirmed the decision record coherent with no surviving assertion of the rejected reading, and returned 0 Blocker and 0 High. Two HIGHs were found and fixed during the rounds and both were in the durable artifact rather than the code: the record asserted the shipped reading AND its inverse, twice, because I was patching a large YAML scalar by targeted string replacement and sweeping with a regex keyed on the retracted wording. The mechanism was replaced - the field is now split into its sentences and read back against the code - and the second HIGH is the reason this PR has five rounds rather than three. Ten clause mutations executed and witnessed red across the branch, each scoped so its siblings stayed green, including two that a reviewer proved had SURVIVED an earlier head. Review skipped on this final head alone, with its reason: 3 lines of comment and two prose corrections, each specified verbatim by the previous round, no logic change. One finding deliberately not acted on and recorded instead - MonthsOfYear taking AllDaysOfMonth is an EQUIVALENT mutant at the selector since 1..31 contains every month, and its non-equivalent twin is pinned at the mapper.