refactor(395): dedup Scripted≡YAML enumerator construction into ContentEnumeratorBuilder #566

Merged

timothy merged 2 commits from issue-395-dedup-enumerator into main 2026-07-22 20:52:24 +02:00

Conversation 2 Commits 2 Files Changed 7

+217 -45

timothy commented 2026-07-22 20:22:26 +02:00

Owner

Copy Link

Copy Source

Fixes #395.

What

The Scripted engine (SchedulingEngine.EnumeratorForContent) and the Sequential/YAML engine (EnumeratorCache.GetEnumeratorForContent) built their stateless-index IMediaCollectionEnumerator with byte-identical switch bodies. This extracts that switch into one static per-family seam, ContentEnumeratorBuilder.ForContent(items, state, playbackOrder, multiPart), mirroring #380's ShuffleSourceBuilder. Both engines delegate; each keeps its own "not supported" warning on the None branch (per-engine message unchanged).

The blocker premise, corrected

#395's original Done-when box 1 assumed #381 would land a Scripted characterization golden. It didn't — #381 shipped the Sequential (YAML) golden and recorded testing.scripted-playout-golden-deferred, deferring Scripted to an integration harness (#563). On re-examination that record overstated the constraint: ScriptedScheduleController is a 1:1 pass-through, and the scheduling logic — including this exact switch — lives in the in-process SchedulingEngine, which is directly unit-testable. So the real regression net this refactor needs is a direct helper test, not a Scripted golden (which is why #563 is not a prerequisite). This PR:

• corrects the testing.scripted-playout-golden-deferred record (transport-vs-engine line) + docs/testing.md to match, and
• adds ContentEnumeratorBuilderTests as that net.

Safety net

• ContentEnumeratorBuilderTests pins the two documented traps: Shuffle → BlockPlayoutShuffledMediaCollectionEnumerator (exact type, never Classic's ShuffledMediaCollectionEnumerator), and every one of the 8 unsupported PlaybackOrder values → None (the #70 contract).
• The existing Sequential (YAML) golden (PlayoutBuildGoldenTests, #381) covers the YAML call-site end-to-end — green before (on origin/main via #564) and after.
• Scripted call-sites are unchanged trivial delegations; multiPart wiring preserved exactly (Scripted defaults false; YAML forwards content.MultiPart).

Verification

• Full solution build: 0 errors.
• ErsatzTV.Core.Tests: 644 passed, 1 pre-existing skip (incl. new 11 cases, Sequential golden, SchedulingEngineTests).
• dotnet format (touched .cs): clean; BOM check clean.
• Cold-context adversarial review: MERGEABLE, no substantive defects (equivalence confirmed against both origin/main originals; warnings preserved and correctly gated; test non-vacuous with complete unsupported-order coverage).

Docs

• docs/decisions.md (testing.scripted-playout-golden-deferred reframed; rationale-prose correction → [decisions-edit], same key, not a supersession), docs/decisions/README.md (regenerated), docs/testing.md.

Also filed #565 (blue-sky Auto-Tune→Scripted bridge) from the same discussion.

Fixes #395. ## What The Scripted engine (`SchedulingEngine.EnumeratorForContent`) and the Sequential/YAML engine (`EnumeratorCache.GetEnumeratorForContent`) built their stateless-index `IMediaCollectionEnumerator` with **byte-identical** switch bodies. This extracts that switch into one static per-family seam, `ContentEnumeratorBuilder.ForContent(items, state, playbackOrder, multiPart)`, mirroring #380's `ShuffleSourceBuilder`. Both engines delegate; each keeps its **own** "not supported" warning on the `None` branch (per-engine message unchanged). ## The blocker premise, corrected #395's original Done-when box 1 assumed #381 would land a **Scripted** characterization golden. It didn't — #381 shipped the Sequential (YAML) golden and recorded `testing.scripted-playout-golden-deferred`, deferring Scripted to an integration harness (#563). On re-examination that record **overstated** the constraint: `ScriptedScheduleController` is a 1:1 pass-through, and the scheduling logic — including this exact switch — lives in the in-process `SchedulingEngine`, which is directly unit-testable. So the real regression net this refactor needs is a **direct helper test**, not a Scripted golden (which is why #563 is *not* a prerequisite). This PR: - corrects the `testing.scripted-playout-golden-deferred` record (transport-vs-engine line) + `docs/testing.md` to match, and - adds `ContentEnumeratorBuilderTests` as that net. ## Safety net - **`ContentEnumeratorBuilderTests`** pins the two documented traps: Shuffle → `BlockPlayoutShuffledMediaCollectionEnumerator` (exact type, **never** Classic's `ShuffledMediaCollectionEnumerator`), and every one of the 8 unsupported `PlaybackOrder` values → `None` (the #70 contract). - The existing **Sequential (YAML) golden** (`PlayoutBuildGoldenTests`, #381) covers the YAML call-site end-to-end — green before (on `origin/main` via #564) and after. - Scripted call-sites are unchanged trivial delegations; `multiPart` wiring preserved exactly (Scripted defaults false; YAML forwards `content.MultiPart`). ## Verification - Full solution build: 0 errors. - `ErsatzTV.Core.Tests`: **644 passed**, 1 pre-existing skip (incl. new 11 cases, Sequential golden, `SchedulingEngineTests`). - `dotnet format` (touched `.cs`): clean; BOM check clean. - Cold-context adversarial review: **MERGEABLE**, no substantive defects (equivalence confirmed against both `origin/main` originals; warnings preserved and correctly gated; test non-vacuous with complete unsupported-order coverage). ## Docs - `docs/decisions.md` (`testing.scripted-playout-golden-deferred` reframed; rationale-prose correction → `[decisions-edit]`, same key, not a supersession), `docs/decisions/README.md` (regenerated), `docs/testing.md`. Also filed #565 (blue-sky Auto-Tune→Scripted bridge) from the same discussion.

timothy added 1 commit 2026-07-22 20:22:26 +02:00

refactor(395): dedup Scripted≡YAML enumerator construction into ContentEnumeratorBuilder ... f54c9ae195

Extract the byte-identical PlaybackOrder -> IMediaCollectionEnumerator switch
shared by SchedulingEngine.EnumeratorForContent (Scripted) and
EnumeratorCache.GetEnumeratorForContent (Sequential/YAML) into one static
per-family seam, mirroring #380's ShuffleSourceBuilder. Each engine keeps its own
"not supported" warning on the None branch, so the per-engine message is unchanged.
Adds ContentEnumeratorBuilderTests pinning the block-shuffle-not-classic trap and
the unsupported-order -> None (#70) contract across all 8 unsupported orders.

Corrects the testing.scripted-playout-golden-deferred decision record: Scripted's
external-process + HTTP pipeline is integration-only (deferred to #563), but the
in-process SchedulingEngine it drives IS unit-testable (ScriptedScheduleController
is a 1:1 pass-through) -- the earlier "un-golden-able by construction" framing
conflated transport with engine. docs/testing.md reframed to match.

[decisions-edit]

fixes #395

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

timothy commented 2026-07-22 20:22:52 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ f54c9ae1

Cold-context, review-only adversarial pass over the full diff (both call sites vs their origin/main originals). Verified: switch bodies semantically identical (the dropped keepMultiPartEpisodesTogether local was a pure alias); each engine's "not supported" warning preserved verbatim and correctly re-gated on IsNone (fires only for unsupported orders, exactly as before); Shuffle→BlockPlayoutShuffledMediaCollectionEnumerator trap enforced in helper + test; multiPart wiring exact (Scripted defaults false, YAML forwards content.MultiPart); test non-vacuous with complete 8/8 unsupported-order coverage. Build 0 warnings/errors, 11 new tests pass. No substantive defects.

Review-verdict: MERGEABLE @ f54c9ae1 Cold-context, review-only adversarial pass over the full diff (both call sites vs their `origin/main` originals). Verified: switch bodies semantically identical (the dropped `keepMultiPartEpisodesTogether` local was a pure alias); each engine's "not supported" warning preserved verbatim and correctly re-gated on `IsNone` (fires only for unsupported orders, exactly as before); Shuffle→`BlockPlayoutShuffledMediaCollectionEnumerator` trap enforced in helper + test; `multiPart` wiring exact (Scripted defaults false, YAML forwards `content.MultiPart`); test non-vacuous with complete 8/8 unsupported-order coverage. Build 0 warnings/errors, 11 new tests pass. No substantive defects.

timothy added 1 commit 2026-07-22 20:29:12 +02:00

docs(395): keep the decision record's original heading (fix decisions-lifecycle) ... 7b77a37f0f

The lifecycle validator matches records by their `##` heading for removal
detection (gone = base_headings - head_headings), ungated by [decisions-edit].
Renaming the heading read as an unlogged record removal. Restore the exact
original heading -- which stays literally true ("excluded from the golden net":
#395 adds a unit test, not a golden) -- and keep the rationale correction in the
Rule/Signals/body, which is what [decisions-edit] authorizes.

[decisions-edit]

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

timothy commented 2026-07-22 20:29:25 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 7b77a37f

CI on the first head (f54c9ae1) flagged only decisions lifecycle red — and this was NOT the known flake: the validator matches decision records by their ## heading for removal-detection (gone = base_headings − head_headings, ungated by [decisions-edit]), so my renaming the record's heading read as an unlogged record removal. Reproduced locally, root-caused, fixed by restoring the exact original heading (which stays literally true — "excluded from the golden net": #395 adds a unit test, not a golden) while keeping the rationale correction in the Rule/Signals/body where [decisions-edit] authorizes it.

Delta vs the reviewed f54c9ae1 is docs-only (one heading line + regenerated catalog row) — no code change — so the prior equivalence review stands. Both decisions lifecycle steps now pass locally: decisions_validate.py --base origin/main --head HEAD → OK; build_decisions_catalog.py --check → up to date. All other jobs were green/running on the prior head.

Review-verdict: MERGEABLE @ 7b77a37f CI on the first head (f54c9ae1) flagged **only** `decisions lifecycle` red — and this was NOT the known flake: the validator matches decision records by their `##` heading for removal-detection (`gone = base_headings − head_headings`, ungated by `[decisions-edit]`), so my renaming the record's heading read as an unlogged record removal. Reproduced locally, root-caused, fixed by restoring the exact original heading (which stays literally true — "excluded from the golden net": #395 adds a unit test, not a golden) while keeping the rationale correction in the Rule/Signals/body where `[decisions-edit]` authorizes it. Delta vs the reviewed f54c9ae1 is **docs-only** (one heading line + regenerated catalog row) — no code change — so the prior equivalence review stands. Both `decisions lifecycle` steps now pass locally: `decisions_validate.py --base origin/main --head HEAD` → OK; `build_decisions_catalog.py --check` → up to date. All other jobs were green/running on the prior head.

timothy merged commit d759a9fc83 into main 2026-07-22 20:52:24 +02:00

timothy deleted branch issue-395-dedup-enumerator 2026-07-22 20:52:24 +02:00

timothy referenced this issue from a commit 2026-07-22 20:52:25 +02:00

Merge pull request 'refactor(395): dedup Scripted≡YAML enumerator construction into ContentEnumeratorBuilder' (#566) from issue-395-dedup-enumerator into main

timothy referenced this pull request 2026-07-22 20:52:54 +02:00

Refactor: dedup Scripted≡YAML enumerator construction (deferred from #380, gated on #381 goldens) #395

timothy referenced this pull request 2026-07-25 12:11:13 +02:00

Active decisions corpus is at its 5600-line budget — schedule the consolidation #595

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