timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

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

New Issue
Closed
opened 2026-08-13 19:58:48 +02:00 by timothy · 4 comments
timothy commented 2026-08-13 19:58:48 +02:00
Owner
Copy Link
Copy Source

Spawned by the #773 root-cause analysis (docs/defect-shapes-773.md §3, §4 detectors F and G). Two small conventions, filed together because both are "a test that never exercised the dangerous value."

Part 1 — fail-open by default

A surface that defaults to permissive when config is absent or a step vanishes:

  • #280 — API writes fail open when Api:WriteKey is null/empty, which is the shipped default
  • #751 / #756 / #768 — a dropped step is fail-CLOSED in review-verdict.yml but fail-OPEN in docker-build.yml's required jobs
  • #647 — a verdict classifier entirely inert on jq 1.6

#756's lesson generalises: a fixture that omits a field tests only the default, so a fail-open in the production value stays invisible — 30 green tests did not see it. The rule: parametrise the whole matrix, including "config absent", and assert the deny path with the production value, not just the allow path.

Part 2 — destructive replace

A full-replace path that silently destroys state a reconcile-by-id would have kept:

  • #252 — a schedule PUT reset fill-group progression; the fix was a "scoped positional/no-op reconcile in ReplaceProgramScheduleItemsHandler", with the full stable-identity design deferred
  • #754 — a hand-maintained full-replace wrapper drifted from a 28-property DTO by one field and silently cleared it: HTTP 200, no error, missing pixels hours later
  • #500 — a dedup fix turned a duplicate row into permanent data loss because the remove filter used a different key

The rule: a full-replace endpoint asserts its complete field list in a test, and prefers reconcile-by-id over delete-and-reinsert wherever child state exists.

Done-when

  • Both conventions written as decision records and the catalog regenerated — testing.deny-path-at-production-config-value, testing.full-replace-asserts-field-list, catalog regenerated, docs/README.md task-signal rows added (PR #808)
  • Existing full-replace endpoints/wrappers audited for a field-list assertion; gaps filled or tracked — 13 sites enumerated across handlers + DTO mirrors. ToolCatalogTests already covers the MCP mirror; the gap was ScheduleItemResponseRoundTripTests's own comparer (a hand-copied field list in the test whose job is catching exactly that), now derived by reflection with a count pin. SPA optional-field drift tracked as #807.
  • Existing config-gated guards audited for a deny-path test at the production value; gaps filled or tracked — population derived from git ls-files over .claude/hooks/, .husky/, scripts/, .gitea/workflows/. Gap found and filled: no test in the repo constructed ApiKeyProvider, so Api:RequireKeyForReads was covered only through fakes handed the resolved bool. Now covered absent / true|True|TRUE / false|False / non-boolean-throws. Remaining permissive branches are all hook exit 0 escape hatches already carrying NONE-proof rows in docs/guard-inventory.md and owned by #785.
  • Adversarial review passed — four cold rounds (worktree-isolated, review-only). Three returned BLOCKED, each on an unmeasured/false claim in a decision record; all fixed. Final round independently reproduced both mutation results and returned MERGEABLE. Review-verdict/h10 = success @ 8387e16.
Spawned by the #773 root-cause analysis (`docs/defect-shapes-773.md` §3, §4 detectors F and G). Two small conventions, filed together because both are "a test that never exercised the dangerous value." ## Part 1 — fail-open by default A surface that defaults to permissive when config is absent or a step vanishes: - #280 — API writes fail **open** when `Api:WriteKey` is null/empty, which is the shipped default - #751 / #756 / #768 — a dropped step is fail-CLOSED in `review-verdict.yml` but fail-**OPEN** in `docker-build.yml`'s required jobs - #647 — a verdict classifier entirely inert on jq 1.6 #756's lesson generalises: **a fixture that omits a field tests only the default**, so a fail-open in the production value stays invisible — 30 green tests did not see it. The rule: parametrise the whole matrix, including "config absent", and assert the **deny** path with the production value, not just the allow path. ## Part 2 — destructive replace A full-replace path that silently destroys state a reconcile-by-id would have kept: - #252 — a schedule PUT reset fill-group progression; the fix was a "scoped positional/no-op reconcile in `ReplaceProgramScheduleItemsHandler`", with the full stable-identity design deferred - #754 — a hand-maintained full-replace wrapper drifted from a 28-property DTO by one field and silently cleared it: HTTP 200, no error, missing pixels hours later - #500 — a dedup fix turned a duplicate row into permanent data loss because the remove filter used a different key The rule: a full-replace endpoint asserts its **complete field list** in a test, and prefers reconcile-by-id over delete-and-reinsert wherever child state exists. ## Done-when - [x] Both conventions written as decision records and the catalog regenerated — `testing.deny-path-at-production-config-value`, `testing.full-replace-asserts-field-list`, catalog regenerated, `docs/README.md` task-signal rows added (PR #808) - [x] Existing full-replace endpoints/wrappers audited for a field-list assertion; gaps filled or tracked — 13 sites enumerated across handlers + DTO mirrors. `ToolCatalogTests` already covers the MCP mirror; the gap was `ScheduleItemResponseRoundTripTests`'s own comparer (a hand-copied field list in the test whose job is catching exactly that), now derived by reflection with a count pin. SPA optional-field drift tracked as #807. - [x] Existing config-gated guards audited for a deny-path test at the production value; gaps filled or tracked — population derived from `git ls-files` over `.claude/hooks/`, `.husky/`, `scripts/`, `.gitea/workflows/`. Gap found and filled: **no test in the repo constructed `ApiKeyProvider`**, so `Api:RequireKeyForReads` was covered only through fakes handed the resolved bool. Now covered absent / `true|True|TRUE` / `false|False` / non-boolean-throws. Remaining permissive branches are all hook `exit 0` escape hatches already carrying `NONE`-proof rows in `docs/guard-inventory.md` and owned by #785. - [x] Adversarial review passed — four cold rounds (worktree-isolated, review-only). Three returned BLOCKED, each on an unmeasured/false claim in a decision record; all fixed. Final round independently reproduced both mutation results and returned MERGEABLE. `Review-verdict/h10` = success @ 8387e16.
timothy added the ci-cdpriority: medium labels 2026-08-13 19:58:48 +02:00
timothy added this to the Defect-shape hardening milestone 2026-08-13 20:22:30 +02:00
timothy added the in-progress label 2026-08-21 19:06:34 +02:00
timothy commented 2026-08-21 19:06:34 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (Claude Code session, continuing from #778). Four-way check clear: no open PR references it (the three open PRs are Renovate), git ls-remote --heads origin '*779*' empty, no prior comments, origin/main freshly fetched at ad0bed458.

Adjacency worth stating: #778 just landed and touched the same merge-consent hook, adding deny-path tests at production config values — which is Part 1 of this issue applied to one surface. This issue asks for that as a convention plus an audit of the others.

Claiming this (Claude Code session, continuing from #778). Four-way check clear: no open PR references it (the three open PRs are Renovate), `git ls-remote --heads origin '*779*'` empty, no prior comments, `origin/main` freshly fetched at ad0bed458. Adjacency worth stating: #778 just landed and touched the same merge-consent hook, adding deny-path tests at production config values — which is Part 1 of this issue applied to one surface. This issue asks for that as a *convention* plus an audit of the others.
timothy removed the in-progress label 2026-08-21 19:07:47 +02:00
timothy commented 2026-08-21 19:07:47 +02:00
Author
Owner
Copy Link
Copy Source

Releasing this claim — claimed in error at the tail of the #778 session, which was scoped to that one issue. No work was started: no branch, no commits, nothing beyond the claim itself. in-progress removed; free for the next session to pick up.

One handoff note worth keeping, from the bundle scan done before releasing: #787 is a strong sibling for whoever takes this. It needs MARKED_JOBS in test_ci_dropped_step_guard.py checked against live status_check_contexts instead of a dated comment, and #778 (merged as ad0bed458) just built exactly that read in .claude/hooks/pretooluse-merge-consent.sh — a GET /branch_protections with a three-way present/unreadable/absent outcome and an explicit no-silent-fail-open stance. That is the mechanism #787 says it wants a real decision about, and it pairs naturally with this issue's Part 1 (config-gated guards audited for a deny-path test at the production value).

**Releasing this claim** — claimed in error at the tail of the #778 session, which was scoped to that one issue. No work was started: no branch, no commits, nothing beyond the claim itself. `in-progress` removed; free for the next session to pick up. One handoff note worth keeping, from the bundle scan done before releasing: **#787 is a strong sibling for whoever takes this.** It needs `MARKED_JOBS` in `test_ci_dropped_step_guard.py` checked against live `status_check_contexts` instead of a dated comment, and #778 (merged as ad0bed458) just built exactly that read in `.claude/hooks/pretooluse-merge-consent.sh` — a `GET /branch_protections` with a three-way present/unreadable/absent outcome and an explicit no-silent-fail-open stance. That is the mechanism #787 says it wants a real decision about, and it pairs naturally with this issue's Part 1 (config-gated guards audited for a deny-path test at the production value).
timothy added the in-progress label 2026-08-21 19:31:15 +02:00
timothy commented 2026-08-21 19:31:20 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (fresh Claude Code session). Four-way check clear at origin/main = ad0bed458: no open PR references #779 (the three open PRs are Renovate: #761, #804, #805), git ls-remote --heads origin '*779*' empty, the only prior comments are the previous session's claim and its explicit release.

Bundle scan: #787 deliberately NOT folded in. It is the same milestone and adjacent in spirit, but its Done-when asks for a design decision about querying live branch protection from a credential-less test context — that is not the same shape as this issue's "test the deny path at the production value", and bundling it would let a real decision ride along on an audit. Leaving it for its own session.

Claiming this (fresh Claude Code session). Four-way check clear at `origin/main` = ad0bed458: no open PR references #779 (the three open PRs are Renovate: #761, #804, #805), `git ls-remote --heads origin '*779*'` empty, the only prior comments are the previous session's claim and its explicit release. Bundle scan: **#787 deliberately NOT folded in.** It is the same milestone and adjacent in spirit, but its Done-when asks for a *design decision* about querying live branch protection from a credential-less test context — that is not the same shape as this issue's "test the deny path at the production value", and bundling it would let a real decision ride along on an audit. Leaving it for its own session.
timothy commented 2026-08-21 21:23:14 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Both conventions shipped as decision records, plus one real gap filled per part. PR #808 (squash-merged as 8387e16d9 head; branch deleted).

  • Part 1 → testing.deny-path-at-production-config-value
  • Part 2 → testing.full-replace-asserts-field-list

Root cause: Not a bug fix, but each part found a live instance of the shape it describes.

Part 1 — the interesting variant is not "someone forgot to test the production value". Every assertion about the API read-gating posture went through a hand-written FakeApiKeyProvider that is handed the boolean, at requireKeyForReads: true, i.e. the production value, asserting the deny branch. Those are good tests. And no test in the repository constructed ApiKeyProvider, so configuration.GetValue(RequireKeyForReadsConfigurationKey, true) 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. The double stood exactly where the untested code was.

Part 2 — the hand-copied field list was inside ScheduleItemResponseRoundTripTests, the release gate whose job is catching exactly that loss. It covered every property but Id, so it was complete and structurally unable to report the day it stopped being.

Decisions/conventions changed: two records added (testing.deny-path-at-production-config-value, testing.full-replace-asserts-field-list); catalog regenerated; docs/README.md task-signal map gained a row for each. None superseded.

Reusable knowledge:

  1. A test double placed where the untested code is defeats a correct fixture. Ask not only "which value did the test use" but "which code did that value flow through". grep -c 'new <Type>(' over the test projects is a cheap detector for a class nothing ever constructs.
  2. ConfigurationBinder returns the default ONLY for a null section value. A present-but-non-boolean ("", 1, yes) reaches BooleanConverter and throws at construction. Api__RequireKeyForReads= with nothing after it in a compose file is a refusing app, not silent anonymous reads. Fail-closed, now pinned.
  3. The expensive lesson, and it cost four review rounds: three of them BLOCKED on a decision record asserting a mechanism the code contradicted — inside the PR whose subject is that defect. Each draft paired a correct observation with a confident causal story. Sequence: "the PUT replaces the item set so rows get new ids" (false, it reconciles in place) → "an all-null payload takes the create-new-rows path, a fresh DB reassigns the same ids" (false, it takes the positional fallback, which reuses rows) → "no id-less mutation can redden Id" (false universal; a partial id-less payload takes the id-based branch and reddens with Id differs). Only the third round's withdrawal of the narrative — a two-row table of executed results and no explanation — survived. State the measurement and the code path you read; do not generalise from one executed case to a class of cases, and do not explain a mechanism you did not measure. A record is where such a story does the most damage, because it reads as checked.
  4. A per-site derivation beats a repo-wide lint here. "Every full-replace path" is a population of sites in code, which testing.guard-derives-population-from-source scopes out (#777). A name matcher over Replace*Handler misses the ones that don't match and flags the ones that are already correct.
  5. ReplacePlayoutAlternateScheduleItemsHandler operates on ProgramScheduleAlternate, not PlayoutTemplate. They share only IAlternateScheduleItem; the names invite the conflation (I made it). And no table holds an FK to PlayoutTemplateId or ProgramScheduleAlternateId — verified against the model snapshot, not just a grep.

Verification:

  • ErsatzTV.Tests 2005 passed / 0 failed / 4 skipped; scripts/tests 729 passed / 2 skipped (unchanged from baseline).
  • Mutation proofs, all executed and witnessed red: default true→false reddens only the absent-cell test (1 failed / 13 passed); a bogus exemption name reddens the stale-exemption clause; pin 55→60 reddens with the real count; r.Id→null for index 0 reddens with Id differs, should be 4 but was 7. The all-null variant is recorded as green — a negative result kept because it is the one that falsified two drafts.
  • CI fully green on 8387e16d9: 12 success, 1 skipped. Real durations, not fast-path skips — Build & test 8m48s, EF migrations 6m17s, Script tests 4m26s, Functional E2E 6m11s. (Formatting / API-docs-in-sync / Functional E2E had been cancelled by successive pushes on every earlier sha and only completed here — ci.cancelled-is-not-a-verdict applies to those, they were never real reds.)
  • review-verdict/h10 = success @ 8387e16, read back from the API rather than trusted from the script's exit code.

Deferred:

  • #807 — SPA hand-built request objects can silently drop an optional DTO field across six screens. #754's mechanism, narrowed to optionals since tsc catches required ones; no SPA-side equivalent of the MCP write-tool field test. Highest-probability recurrence of the shape today.
  • #787 — deliberately not bundled: it wants a design decision about querying live branch protection from a credential-less test context, not an audit.
  • #785 — the remaining untested permissive branches are all 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). Each already carries a NONE-proof row in docs/guard-inventory.md. Splitting one guard's proof across two issues is how a row ends up claiming coverage twice and holding none.
  • docs/defect-shapes-773.md §4 still grades detectors F and G rule-missing; this PR is what creates their rules. Stale-after-merge, no false statement inside either record.

Docs updated: docs/decisions/records/testing/deny-path-at-production-config-value.md (new), docs/decisions/records/testing/full-replace-asserts-field-list.md (new), docs/decisions/README.md (regenerated), docs/README.md (two task-signal rows).

## Closing record **Outcome:** Both conventions shipped as decision records, plus one real gap filled per part. PR #808 (squash-merged as `8387e16d9` head; branch deleted). - Part 1 → `testing.deny-path-at-production-config-value` - Part 2 → `testing.full-replace-asserts-field-list` **Root cause:** Not a bug fix, but each part found a live instance of the shape it describes. *Part 1* — the interesting variant is **not** "someone forgot to test the production value". Every assertion about the API read-gating posture went through a hand-written `FakeApiKeyProvider` that is *handed* the boolean, at `requireKeyForReads: true`, i.e. the production value, asserting the deny branch. Those are good tests. And **no test in the repository constructed `ApiKeyProvider`**, so `configuration.GetValue(RequireKeyForReadsConfigurationKey, true)` 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. The double stood exactly where the untested code was. *Part 2* — the hand-copied field list was inside `ScheduleItemResponseRoundTripTests`, the release gate whose job is catching exactly that loss. It covered every property but `Id`, so it was complete and structurally unable to report the day it stopped being. **Decisions/conventions changed:** two records added (`testing.deny-path-at-production-config-value`, `testing.full-replace-asserts-field-list`); catalog regenerated; `docs/README.md` task-signal map gained a row for each. None superseded. **Reusable knowledge:** 1. **A test double placed where the untested code is defeats a correct fixture.** Ask not only "which value did the test use" but "which code did that value flow through". `grep -c 'new <Type>('` over the test projects is a cheap detector for a class nothing ever constructs. 2. **`ConfigurationBinder` returns the default ONLY for a null section value.** A present-but-non-boolean (`""`, `1`, `yes`) reaches `BooleanConverter` and throws at construction. `Api__RequireKeyForReads=` with nothing after it in a compose file is a *refusing app*, not silent anonymous reads. Fail-closed, now pinned. 3. **The expensive lesson, and it cost four review rounds:** three of them BLOCKED on a *decision record* asserting a mechanism the code contradicted — inside the PR whose subject is that defect. Each draft paired a correct observation with a confident causal story. Sequence: "the PUT replaces the item set so rows get new ids" (false, it reconciles in place) → "an all-null payload takes the create-new-rows path, a fresh DB reassigns the same ids" (false, it takes the *positional fallback*, which reuses rows) → "no id-less mutation can redden `Id`" (false universal; a **partial** id-less payload takes the id-based branch and reddens with `Id differs`). Only the third round's withdrawal of the narrative — a two-row table of executed results and no explanation — survived. **State the measurement and the code path you read; do not generalise from one executed case to a class of cases, and do not explain a mechanism you did not measure.** A record is where such a story does the most damage, because it reads as checked. 4. **A per-site derivation beats a repo-wide lint here.** "Every full-replace path" is a population of *sites in code*, which `testing.guard-derives-population-from-source` scopes out (#777). A name matcher over `Replace*Handler` misses the ones that don't match and flags the ones that are already correct. 5. **`ReplacePlayoutAlternateScheduleItemsHandler` operates on `ProgramScheduleAlternate`, not `PlayoutTemplate`.** They share only `IAlternateScheduleItem`; the names invite the conflation (I made it). And no table holds an FK to `PlayoutTemplateId` or `ProgramScheduleAlternateId` — verified against the model snapshot, not just a grep. **Verification:** - `ErsatzTV.Tests` 2005 passed / 0 failed / 4 skipped; `scripts/tests` 729 passed / 2 skipped (unchanged from baseline). - Mutation proofs, all **executed and witnessed red**: default `true`→`false` reddens only the absent-cell test (1 failed / 13 passed); a bogus exemption name reddens the stale-exemption clause; pin 55→60 reddens with the real count; `r.Id`→`null` for index 0 reddens with `Id differs, should be 4 but was 7`. The all-null variant is recorded as **green** — a negative result kept because it is the one that falsified two drafts. - CI fully green on `8387e16d9`: 12 success, 1 skipped. Real durations, not fast-path skips — Build & test 8m48s, EF migrations 6m17s, Script tests 4m26s, Functional E2E 6m11s. (Formatting / API-docs-in-sync / Functional E2E had been *cancelled* by successive pushes on every earlier sha and only completed here — `ci.cancelled-is-not-a-verdict` applies to those, they were never real reds.) - `review-verdict/h10` = success @ `8387e16`, read back from the API rather than trusted from the script's exit code. **Deferred:** - **#807** — SPA hand-built request objects can silently drop an *optional* DTO field across six screens. #754's mechanism, narrowed to optionals since `tsc` catches required ones; no SPA-side equivalent of the MCP write-tool field test. Highest-probability recurrence of the shape today. - **#787** — deliberately not bundled: it wants a *design decision* about querying live branch protection from a credential-less test context, not an audit. - **#785** — the remaining untested permissive branches are all 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`). Each already carries a `NONE`-proof row in `docs/guard-inventory.md`. Splitting one guard's proof across two issues is how a row ends up claiming coverage twice and holding none. - `docs/defect-shapes-773.md` §4 still grades detectors F and G **rule-missing**; this PR is what creates their rules. Stale-after-merge, no false statement inside either record. **Docs updated:** `docs/decisions/records/testing/deny-path-at-production-config-value.md` (new), `docs/decisions/records/testing/full-replace-asserts-field-list.md` (new), `docs/decisions/README.md` (regenerated), `docs/README.md` (two task-signal rows).
timothy removed the in-progress label 2026-08-21 21:23:25 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
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 ci-cd priority: medium
Milestone
No items
No Milestone Defect-shape hardening
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#779