feat(403): make unsupported PlaybackOrder loud at build time + tripwire #457

Merged

timothy merged 2 commits from feat/403-loud-playbackorder into main 2026-07-18 19:10:36 +02:00

Conversation 1 Commits 2 Files Changed 10

+447 -17

timothy commented 2026-07-18 18:23:58 +02:00

Owner

Copy Link

Copy Source

Fixes #403.

What

Makes the three still-silent build-time PlaybackOrder dispatch sites loud but non-fatal, and adds a by-construction net so the next order added can't slip through.

• Classic (PlayoutBuilder default:) — was a // TODO silently returning Random; now logs a Warning, keeps the Random fallback (channel doesn't go dark).
• Playlist (PlaylistEnumerator.Create) — had no default: arm → null enumerator → item silently dropped. Create was static with no logger (that's why it couldn't report); added optional Option<ILogger>, loggered callers pass it, new default: arm warns.
• Block (BlockPlayoutBuilder) — silent allow-list continue now warns; added an explicit Random arm (previously reached an enumerator only by coincidence) + a loud defensive fallback.
• PlaybackOrderSupport matrix (per SchedulingEngineKind) + tripwire PlaybackOrderSupportTests: Supported ∪ Unsupported must partition the enum for every engine, so a new order fails the test until classified. Block consumes the matrix for its allow-list (removing a duplicated inline list).

Deliberately out of scope

• Write-path rejection unchanged — #70 already closed the persistence hole; decisions.md records that this perimeter has been wrong three times, so broadening it here is pure regression risk.
• Reverse _ => PlaybackOrder.None mappings reviewed and deferred: that path is hit by legit enumerator types (ShuffleInOrder/SeasonEpisode) so making it loud would false-warn. Different axis (enumerator class → order), not #403's 'unsupported order degrades silently'.

Verification

• ErsatzTV.Core.Tests full: 586 passed (incl. scheduler goldens — no behavior drift). Infra + app scheduling tests green. New behavioral test proves the Playlist drop is now logged.
• docs/decisions.md updated.

Merge gated on adversarial review (Done-when box 5) + CI green.

Fixes #403. ## What Makes the three still-silent build-time `PlaybackOrder` dispatch sites **loud but non-fatal**, and adds a by-construction net so the next order added can't slip through. - **Classic** (`PlayoutBuilder` `default:`) — was a `// TODO` silently returning Random; now logs a Warning, keeps the Random fallback (channel doesn't go dark). - **Playlist** (`PlaylistEnumerator.Create`) — had **no** `default:` arm → null enumerator → item silently dropped. `Create` was `static` with no logger (that's why it couldn't report); added optional `Option<ILogger>`, loggered callers pass it, new `default:` arm warns. - **Block** (`BlockPlayoutBuilder`) — silent allow-list `continue` now warns; added an explicit `Random` arm (previously reached an enumerator only by coincidence) + a loud defensive fallback. - **`PlaybackOrderSupport`** matrix (per `SchedulingEngineKind`) + **tripwire** `PlaybackOrderSupportTests`: `Supported ∪ Unsupported` must partition the enum for every engine, so a new order **fails the test until classified**. Block consumes the matrix for its allow-list (removing a duplicated inline list). ## Deliberately out of scope - **Write-path rejection unchanged** — #70 already closed the persistence hole; `decisions.md` records that this perimeter has been wrong three times, so broadening it here is pure regression risk. - **Reverse `_ => PlaybackOrder.None` mappings** reviewed and deferred: that path is hit by legit enumerator types (ShuffleInOrder/SeasonEpisode) so making it loud would false-warn. Different axis (enumerator class → order), not #403's 'unsupported order degrades silently'. ## Verification - `ErsatzTV.Core.Tests` full: 586 passed (incl. scheduler **goldens** — no behavior drift). Infra + app scheduling tests green. New behavioral test proves the Playlist drop is now logged. - `docs/decisions.md` updated. Merge gated on adversarial review (Done-when box 5) + CI green.

timothy added 1 commit 2026-07-18 18:23:59 +02:00

feat(403): make unsupported PlaybackOrder loud at build time + tripwire ... 0f34c86afa

Adding a new PlaybackOrder was unsafe by construction: three build-time
dispatch sites turned an unknown value into an enumerator silently.
Classic substituted RandomizedMediaCollectionEnumerator (the // TODO
default arm), PlaylistEnumerator had no default arm so the item was
dropped, and BlockPlayoutBuilder's allow-list continue skipped it.
(#70 already made YAML/Scripted log a warning and MultiCollectionGroup
throws.)

- each silent site now logs a Warning naming the order + engine + the
  fallback taken; the fallback itself is preserved so a live channel
  never goes dark on one misconfigured item and scheduler goldens do
  not move.
- PlaylistEnumerator.Create gained an optional Option<ILogger> (it was
  static with no logger -- why the drop was unreportable); loggered
  callers pass it.
- BlockPlayoutBuilder gained an explicit Random arm (it previously
  reached an enumerator only via the coincidental _ => fallback) and a
  loud defensive fallback.
- new PlaybackOrderSupport matrix (per SchedulingEngineKind) + tripwire
  PlaybackOrderSupportTests: Supported ∪ Unsupported must partition the
  enum for every engine, so a new order fails the test until classified.
  BlockPlayoutBuilder consumes the matrix for its allow-list.
- write-path rejection left unchanged (#70 closed the persistence hole;
  the perimeter has been wrong three times per decisions.md); reverse
  _ => None mappings reviewed and deferred (different axis; making them
  loud would warn on legit enumerator types).

docs/decisions.md updated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy added 1 commit 2026-07-18 18:32:39 +02:00

fix(403): accurate Classic fallback message + engine-coverage tripwire ... 1d95cbfee6

Review follow-ups (cold review of PR #457):
- Marathon can reach PlayoutBuilder's default arm via `goto default` when
  its enumerator can't be built; the new warning claimed "Marathon is not
  supported by classic scheduling", which is false. Distinguish
  supported-but-failed-to-build (logs "could not build") from genuinely
  unsupported using PlaybackOrderSupport, which also makes Classic a
  runtime consumer of the matrix.
- PlaybackOrderSupportTests now asserts every SchedulingEngineKind has a
  matrix entry, so a new engine kind fails the test instead of throwing
  KeyNotFoundException at runtime.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

timothy referenced this issue from a commit 2026-07-18 18:32:39 +02:00

fix(403): accurate Classic fallback message + engine-coverage tripwire

timothy commented 2026-07-18 18:34:09 +02:00

Author

Owner

Copy Link

Copy Source

Independent cold adversarial review (review-only agent, no implementation role) completed on the current head, including a focused re-review of the fix commit.

Verified: matrix Supported sets match each engine's real dispatch switch exactly; Block's allow-list semantics are byte-identical to the old hardcoded list; the explicit Random arm and the Playlist/Classic fallbacks are behavior-preserving (goldens don't move); the tripwire is non-vacuous (forces classification of a new order AND a new engine kind); no log-spam path; reverse _ => None deferral is correct. The two Low findings from the first pass (misleading Marathon message; engine-axis coverage gap) are fixed in 1d95cbfe with no regressions.

Review-verdict: MERGEABLE @ 1d95cbfee6

Independent cold adversarial review (review-only agent, no implementation role) completed on the current head, including a focused re-review of the fix commit. Verified: matrix Supported sets match each engine's real dispatch switch exactly; Block's allow-list semantics are byte-identical to the old hardcoded list; the explicit `Random` arm and the Playlist/Classic fallbacks are behavior-preserving (goldens don't move); the tripwire is non-vacuous (forces classification of a new order AND a new engine kind); no log-spam path; reverse `_ => None` deferral is correct. The two Low findings from the first pass (misleading Marathon message; engine-axis coverage gap) are fixed in 1d95cbfe with no regressions. Review-verdict: MERGEABLE @ 1d95cbfee6d833f1ba2e27696c4d1c144888799d

timothy merged commit 8152afead1 into main 2026-07-18 19:10:36 +02:00

timothy referenced this issue from a commit 2026-07-18 19:10:37 +02:00

Merge pull request 'feat(403): make unsupported PlaybackOrder loud at build time + tripwire' (#457) from feat/403-loud-playbackorder into main

timothy deleted branch feat/403-loud-playbackorder 2026-07-18 19:10:37 +02:00

timothy referenced this pull request 2026-07-18 19:11:14 +02:00

Scheduling: an unhandled PlaybackOrder fails silently at five of six dispatch sites #403

timothy referenced this pull request 2026-07-18 19:11:49 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

timothy referenced this pull request 2026-07-18 20:54:35 +02:00

feat(420): skip re-validating an already-green identical merge tree #455

timothy referenced this pull request 2026-07-18 21:19:47 +02:00

ChicoryTV rewrite — queue tracker (goal, arc, session protocol) #237

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#457