feat(779): test the deny path at the production config value, and derive full-replace field lists #808

Merged

timothy merged 5 commits from fix/779-deny-path-and-field-list into main 2026-08-21 21:22:31 +02:00

Conversation 1 Commits 5 Files Changed 6

+386 -55

5 Commits

Author	SHA1	Message	Date
timothyandClaude Opus 5	8387e16d98	docs(779): ReplacePlayoutAlternateScheduleItemsHandler is over ProgramScheduleAlternate, not PlayoutTemplate ... Round-3 review found a factual error introduced by round 3 itself, in the commit whose stated purpose was to stop shipping unverified assertions. Verified: ReplacePlayoutAlternateScheduleItemsHandler mutates playout.ProgramScheduleAlternates and constructs ProgramScheduleAlternate — a different entity from PlayoutTemplate. They share only the IAlternateScheduleItem interface, which is where the conflation came from. The same sentence also attributed those handlers' id-based reconcile to protecting progression state. That is unsupported: PlayoutTemplateId and ProgramScheduleAlternateId appear NOWHERE in the solution, model snapshot included, so no table holds an FK to either row's id. Only ReplaceProgramScheduleItemsHandler is demonstrably protecting identity-keyed state (PlayoutScheduleItemFillGroupIndex -> ProgramScheduleItemId). The record now says which of the three that applies to and declines to explain the others. Also drops a definite article that had survived the previous softening: "The mutation class that would redden True/TRUE" was still an unexecuted uniqueness claim in a record whose thesis forbids exactly those. Now "A mutation class". Independent reproduction of the two mutation results in that record, by the reviewer, in its own worktree: all-null -> GREEN; index-0-only -> RED with "Id differs, should be 4 but was 7". The new id confirms the row was inserted as new rather than updated. Refs #779 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 20:59:19 +02:00
timothyandClaude Opus 5	d0b2abb7ca	fix(779): stop generalising from one measured case — and pin the config cell the matrix forgot ... Third review round, third finding of the same class in the same paragraph. The correction in 5cabcf375 replaced a wrong mechanism with a wrong UNIVERSAL: "no id-less mutation of that fixture can redden the Id comparison" Measured, not argued this time. Two mutations of ToReplaceCommand: null for EVERY id -> GREEN (requestIds.Count == 0, positional fallback, which also reuses each row) null for INDEX 0 only -> RED, "Id differs" (id-based branch deletes the unreferenced row, inserts the id-less one new) So Id DOES discriminate — against precisely the regression where a partial-id payload causes a live row to be deleted and reinserted, taking its fill-group state with it. The paragraph now shows both results in a table and offers no account of why, because that is the fourth draft and three of the first three stories were contradicted by the code. The rule the record now carries: 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. Also, the deny-path record claimed "the whole matrix" while omitting the cell an operator actually hits. Measured: ConfigurationBinder returns the default ONLY for a null section value, so a present-but-non-boolean value ("", "1", "yes") reaches BooleanConverter and throws InvalidOperationException at construction. `Api__RequireKeyForReads=` with nothing after it in a compose file is a refusing app, not silent anonymous reads. Now pinned by A_Non_Boolean_Read_Gating_Value_Fails_Startup_Rather_Than_Reads, so a later switch to a lenient TryParse cannot turn a typo into a posture change. Remaining review points: - the GuideGroup hazard is no longer "narrowed by contiguous runs": the block path is ChannelGuideProjector.ProjectBlock, a plain GroupBy on {GuideStart, GuideFinish, GuideGroup} and NOT contiguity-sensitive. The contiguity logic is the flood projector, which never sees a TemplateItemId-derived GuideGroup - "both branches preserve ids" gains the missing same-COUNT qualifier: the positional fallback removes surplus rows and inserts surplus items - the id-reconciling handlers are named in full (ReplacePlayoutAlternateScheduleItemsHandler, ReplacePlayoutTemplateItemsHandler, both over PlayoutTemplate) so they are not read as the TemplateItem rows listed among the delete-and-reinsert four - "the only mutation that would redden True/TRUE" softened to "the mutation class" — another unmeasured universal, in a record about those - broken inline code span and a stray double blank line Refs #779 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 20:37:25 +02:00
timothyandClaude Opus 5	5cabcf3750	fix(779): stop narrating mechanisms — the third causal story was wrong too ... Re-review of the fix commit found that my correction repeated the very defect it was correcting. The commit message said "Measured, not assumed" about a mechanism I had not measured: "replacing the forwarded r.Id with null — which sends the handler down the create-new-rows path — leaves this test green, because a fresh database reassigns the same ids once the old rows are deleted" The observation (green) was right; both halves of the explanation were wrong. An all-null payload gives requestIds.Count == 0, which takes the POSITIONAL FALLBACK, not the create-new-rows path — and that branch does `built.Id = existing.Id` and SetValues, so no row is deleted and no id is reassigned. No fresh database is involved. Read from ReplaceProgramScheduleItemsHandler.cs:79-100 this time, not inferred. The conclusion survives and is stronger than written: BOTH branches preserve ids for a same-order, same-subtype payload, so no id-less mutation of this fixture can redden the Id comparison. What separates the branches is a REORDERING payload, which is what ReplaceProgramScheduleItemsReconcileTests pins in Reorder_ById_Should_Move_State_With_The_Logical_Item_Not_The_Slot and Insert_ById_In_Middle_Should_Keep_Existing_Ids_And_State — the previously cited "in-place: id preserved" assertion sits in a test whose payload has no ids, so it runs the positional path too and was the wrong citation. Three drafts of one paragraph each supplied a confident causal story for a correct observation and two were wrong, so the record now says the rule that generalises: record the measurement and the code path you actually read; do not narrate why the system behaves that way unless that was measured too. Also from the re-review: - the BlockItemId counterexample no longer asserts which branch is taken. Ids can be REUSED after delete-and-reinsert, in which case the lookup resolves to a DIFFERENT block item rather than to nothing — worse than the "loses its collection type and name" previously claimed. Which occurs was not measured, so the record claims only that the reference is orphaned - the GuideGroup collision is marked a HAZARD, not a demonstrated consequence: unmeasured, and narrowed by the projector grouping only contiguous runs that also share GuideStart/GuideFinish - detector F's own proof is now stated honestly in its record: flipping the default reddens ONLY the absent-cell test, so the true/True/TRUE cases are complete coverage but are not independently load-bearing against it. A complete matrix is not one in which every cell discriminates - elementwise comparison holds for records OR value types (two of the four collection properties are List<int>) - the floor-versus-pin wording notes it is strictly stronger than, not in conflict with, testing.guard-derives-population-from-source - "must still exist on the record" now names ScheduleItemResponseModel Empty exemption set with a live stale-exemption loop is kept deliberately: it is a universally-quantified invariant over a legitimately empty set, not a narrowed population, and it becomes live with the first exemption. Refs #779 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 20:21:44 +02:00
timothyandClaude Opus 5	02b8e37526	fix(779): the Id exemption's stated reason was the opposite of the code, and the record's negative was too strong ... Cold adversarial review found two false factual claims written into the new decision records by the change whose whole purpose is to make such records authoritative. Both are corrected; the code was sound. 1. The Id exemption. Its written reason was "the PUT replaces the item set, so B's rows are new rows with new ids". The request forwards each Id, so the handler takes its id-based reconcile path and updates rows in place. Removing the exemption leaves the test green, so Id is now compared and the exemption set is empty. The inherited "ignoring regenerated row ids" comment above the round trip said the same false thing and is corrected too. The reviewer's proposed replacement rationale — that comparing Id now pins #252's id-preservation invariant — is ALSO an overclaim, and the check is cheap: replacing the forwarded r.Id with null, which sends the handler down the create-new-rows path, leaves this test GREEN, because a fresh database reassigns the same ids once the old rows are deleted. Measured, not assumed. The comment now says what the assertion does not establish, and points at ReplaceProgramScheduleItemsReconcileTests, which does pin it. 2. The second clause's negative — "no persisted child state is keyed to those item ids" — is false. PlayoutItem.SchedulingContext serializes a BlockSchedulingContext carrying BlockItemId, and its consumer falls back to (id, null, null) once ReplaceBlockItemsHandler renumbers those ids, so the troubleshooting view silently loses collection type and name. GuideGroup is set to TemplateItemId and persisted while ReplaceTemplateItemsHandler renumbers it. Restated narrowly and dated: no PROGRESSION state is keyed to those ids; two diagnostic/grouping fields are, and are orphaned. Degradation, not data loss. Also from the review: - the anti-vacuity check is a PIN (ShouldBe(55)) rather than a >= floor, which was the one-sided version of the both-directions rule the record invokes; its message interpolates the expectation instead of a stale literal - the record now states that a complete COMPARER is not a complete FIXTURE: the seed builder is still hand-written, so a new nullable field is compared null-against-null until someone seeds it - ProviderWith appends the WriteKey entry LAST so a caller cannot override it to empty and send a unit test into the live config volume - the elementwise-comparison comment no longer implies value equality is intrinsic; it holds because those elements are records Mutation proofs re-witnessed after the clauses changed: pin 55->60 reddens, a bogus exemption name reddens, and the r.Id->null mutation is recorded as NOT reddening, which is why the Id claim is stated as it is. Refs #779 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 20:03:41 +02:00
timothyandClaude Opus 5	371ca2c7ee	feat(779): test the deny path at the production config value, and derive full-replace field lists ... Two conventions from #773's detector menu (F and G), each shipped as a decision record plus one gap actually filled rather than only described. Part 1 — deny path at the production value. Every assertion about the API read gating posture ran through a hand-written FakeApiKeyProvider that is HANDED the boolean, and no test in the repository constructed ApiKeyProvider at all. So the one line deriving that posture from configuration had never been executed by a test: mistype the key or flip the default and every green deny-path assertion stays green while the build serves anonymous reads. Now covered across the whole matrix — absent, true/True/TRUE, false/False — through the real provider. Part 2 — full-replace asserts its field list. ScheduleItemResponseRoundTripTests is the release gate for the flat schedule-item DTO, and its comparer was itself a hand-copied field list: complete on the day it was written, structurally unable to report the day it stopped being. A 55th field would be compared by nobody while the test named for losslessness stayed green. It now derives the property set by reflection with one named exemption (Id), asserts every exemption still names a real property, and carries an anti-vacuity floor. Mutation proofs witnessed, not assumed: - default true->false reddens Read_Gating_Is_Required_When_The_Setting_Is_Absent alone (1 failed, 13 passed) - a bogus exemption name reddens the stale-exemption assert - floor 50->60 reddens with "only 54 properties were compared" Residual SPA optional-field drift filed as #807 rather than folded in. Refs #779 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-21 19:46:22 +02:00