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

timothy commented 2026-08-21 19:47:43 +02:00

Owner

Copy Link

Copy Source

fixes #779

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 config value

New record testing.deny-path-at-production-config-value.

The gap it found is the variant worth remembering: every assertion about the API read-gating posture ran through a hand-written FakeApiKeyProvider that is handed the boolean (ApiAuthorizationFilterTests, ApiKeyEndpointRequiresKeyTests). Those are good tests — they cover the deny branch, at requireKeyForReads: true, which is the production value. 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 configuration key or flip the default and every one of those green deny-path assertions stays green while the shipped build serves anonymous reads.

So: 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.

Now covered across the whole matrix through the real provider: absent, true/True/TRUE, false/False.

Part 2 — a full-replace path asserts its complete field list

New record testing.full-replace-asserts-field-list.

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 comparer was itself a hand-copied field list. 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 the drift, one altitude up.

It now derives the property set by reflection, with one named exemption (Id, server-assigned), an assertion that every exemption still names a real property (a stale exemption exempts nothing while reading in review as a considered decision), and an anti-vacuity floor.

Mutation proofs — witnessed, not assumed

Mutation	Result
ApiKeyProvider default true → false	RED, and only Read_Gating_Is_Required_When_The_Setting_Is_Absent (1 failed, 13 passed)
bogus name added to RoundTripExemptProperties	RED — 'NoSuchProperty' … is not a property of ScheduleItemResponseModel
anti-vacuity floor 50 → 60	RED — only 54 properties were compared

The third also produced the real count (54 compared + Id), which corrected a figure taken from a text-grep during drafting.

Deliberately not in scope

• #787 — same milestone, but its Done-when asks for a design decision about querying live branch protection from a credential-less test context. Bundling it would let a real decision ride along on an audit.
• The hook exit 0 escape hatches (ETV_ALLOW_DIRTY_PUSH, ETV_SKIP_REBASE_CHECK, absent creds in prepush-donewhen.sh, absent python3 in decisions-guard.sh, and memory_pressure being macOS-only in pretooluse-agent-ram.sh) are each already a NONE-proof row in docs/guard-inventory.md and inside #785's scope. Splitting one guard's proof across two issues is how a row ends up claiming coverage twice and holding none.
• SPA optional-field drift filed as #807 — #754's mechanism, narrowed to optional fields, with no SPA-side equivalent of the MCP write-tool test.

Verification

• ErsatzTV.Tests: 2002 passed, 4 skipped, 0 failed
• scripts/tests: 729 passed, 2 skipped (baseline unchanged)
• decisions_validate.py: OK; catalog regenerated; neither new record exceeds the 60-line ceiling
• format gate (--folder --include on both touched .cs) clean; BOM check clean
• Docs updated in-PR: docs/README.md task-signal map gains a row per record

🤖 Generated with Claude Code

fixes #779 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 config value New record `testing.deny-path-at-production-config-value`. The gap it found is the variant worth remembering: every assertion about the API read-gating posture ran through a hand-written `FakeApiKeyProvider` that is **handed** the boolean (`ApiAuthorizationFilterTests`, `ApiKeyEndpointRequiresKeyTests`). Those are good tests — they cover the deny branch, at `requireKeyForReads: true`, which is the production value. 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 configuration key or flip the default and every one of those green deny-path assertions stays green while the shipped build serves anonymous reads. So: 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. Now covered across the whole matrix through the real provider: absent, `true`/`True`/`TRUE`, `false`/`False`. ## Part 2 — a full-replace path asserts its complete field list New record `testing.full-replace-asserts-field-list`. `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 comparer was itself a hand-copied field list. 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 the drift, one altitude up. It now derives the property set by reflection, with one named exemption (`Id`, server-assigned), an assertion that every exemption still names a real property (a stale exemption exempts nothing while reading in review as a considered decision), and an anti-vacuity floor. ## Mutation proofs — witnessed, not assumed | Mutation | Result | |---|---| | `ApiKeyProvider` default `true` → `false` | **RED**, and only `Read_Gating_Is_Required_When_The_Setting_Is_Absent` (1 failed, 13 passed) | | bogus name added to `RoundTripExemptProperties` | **RED** — `'NoSuchProperty' … is not a property of ScheduleItemResponseModel` | | anti-vacuity floor 50 → 60 | **RED** — `only 54 properties were compared` | The third also produced the real count (54 compared + `Id`), which corrected a figure taken from a text-grep during drafting. ## Deliberately not in scope - **#787** — same milestone, but its Done-when asks for a *design decision* about querying live branch protection from a credential-less test context. Bundling it would let a real decision ride along on an audit. - **The hook `exit 0` escape hatches** (`ETV_ALLOW_DIRTY_PUSH`, `ETV_SKIP_REBASE_CHECK`, absent creds in `prepush-donewhen.sh`, absent `python3` in `decisions-guard.sh`, and `memory_pressure` being macOS-only in `pretooluse-agent-ram.sh`) are each already a `NONE`-proof row in `docs/guard-inventory.md` and inside **#785**'s scope. Splitting one guard's proof across two issues is how a row ends up claiming coverage twice and holding none. - **SPA optional-field drift** filed as **#807** — #754's mechanism, narrowed to optional fields, with no SPA-side equivalent of the MCP write-tool test. ## Verification - `ErsatzTV.Tests`: 2002 passed, 4 skipped, 0 failed - `scripts/tests`: 729 passed, 2 skipped (baseline unchanged) - `decisions_validate.py`: OK; catalog regenerated; neither new record exceeds the 60-line ceiling - format gate (`--folder --include` on both touched `.cs`) clean; BOM check clean - Docs updated in-PR: `docs/README.md` task-signal map gains a row per record 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 1 commit 2026-08-21 19:47:44 +02:00

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

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>

timothy added 1 commit 2026-08-21 20:03:52 +02:00

fix(779): the Id exemption's stated reason was the opposite of the code, and the record's negative was too strong ... 02b8e37526

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>

timothy added 1 commit 2026-08-21 20:21:48 +02:00

fix(779): stop narrating mechanisms — the third causal story was wrong too ... 5cabcf3750

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>

timothy added 1 commit 2026-08-21 20:37:29 +02:00

fix(779): stop generalising from one measured case — and pin the config cell the matrix forgot ... d0b2abb7ca

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>

timothy added 1 commit 2026-08-21 20:59:22 +02:00

docs(779): ReplacePlayoutAlternateScheduleItemsHandler is over ProgramScheduleAlternate, not PlayoutTemplate ... 8387e16d98

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>

timothy referenced this pull request 2026-08-21 21:00:24 +02:00

Test the deny path with the production config value, and assert full-replace field lists #779

timothy commented 2026-08-21 21:21:58 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 8387e16

Four review rounds; three blocked on unmeasured claims in the decision records, each fixed and the last verified by independent reproduction of both mutations (all-null GREEN; index-0-only RED, Id differs 4->7). All CI green on 8387e16d9.

Review-verdict: MERGEABLE @ 8387e16 Four review rounds; three blocked on unmeasured claims in the decision records, each fixed and the last verified by independent reproduction of both mutations (all-null GREEN; index-0-only RED, Id differs 4->7). All CI green on 8387e16d9.

timothy merged commit 649acd12ee into main 2026-08-21 21:22:31 +02:00

timothy deleted branch fix/779-deny-path-and-field-list 2026-08-21 21:22:33 +02:00

timothy referenced this pull request 2026-08-21 21:23:14 +02:00

Test the deny path with the production config value, and assert full-replace field lists #779

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#808