Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 11s
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 11s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 17s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 17s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m49s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 6m21s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Move the Playouts UI, route wrapper, helpers, and detailed behavior tests into a colocated screen module while preserving App-level navigation coverage. Refs #245 Co-Authored-By: OpenAI Codex <codex@openai.com>
1812 lines
151 KiB
Markdown
1812 lines
151 KiB
Markdown
# Decisions — append-only log
|
||
|
||
Purpose: why the codebase does what it does, so agents don't "fix" an established convention or
|
||
relitigate a settled call. Append new entries at the bottom in date order (plus a TOC line in the
|
||
Index); **update this doc in the same PR that changes any fact below** (or that establishes a new
|
||
convention worth recording).
|
||
|
||
**Append-only, enforced (ersatztv#303 H9).** A commit or PR that *deletes or modifies* an existing
|
||
line is blocked by the Husky `commit-msg` hook and the CI `decisions-guard` job; insertions anywhere
|
||
are always allowed. The block is lifted only by the **`[decisions-edit]`** token in the commit
|
||
message, for the two legitimate reasons to touch history:
|
||
- **Fix a factual error** in a past entry.
|
||
- **Supersede a reversed decision** — add the new dated entry, then prepend a
|
||
`> **Superseded YYYY-MM by <new entry title>.**` banner to the old entry and tag its Index line
|
||
`(superseded)`. Keep the old entry — *why we changed our mind* is the point; never silently rewrite it.
|
||
|
||
**Consolidation** (prune/merge superseded entries, refresh the Index) is primarily a step in the
|
||
release checklist (`docs/ci-cd.md` → Versioning & releases), done at each release/milestone with
|
||
`[decisions-edit]` — not ad hoc mid-arc. A **size floor** backstops it between releases: the
|
||
`decisions-guard` CI job warns (non-blocking) once this file exceeds **1800 lines** — the point where
|
||
it no longer fits one default 2000-line agent Read — so append-only can't grow past what an agent can
|
||
read in a pass. The metric is the file's read cost (line count), not the entry count. Together these
|
||
keep append-only from accreting stale, contradictory, or unreadably-large history.
|
||
|
||
---
|
||
|
||
## Index
|
||
|
||
- [2026-06 — REST API wraps existing MediatR handlers 1:1, no service layer](#2026-06--rest-api-wraps-existing-mediatr-handlers-11-no-service-layer)
|
||
- [2026-06 — UI rebuild is a React SPA (ChicoryTV) on the REST API, not a Blazor reskin](#2026-06--ui-rebuild-is-a-react-spa-chicorytv-on-the-rest-api-not-a-blazor-reskin)
|
||
- [2026-07 — Response DTOs live in `ErsatzTV.Core/Api`, file-scoped `#nullable enable`](#2026-07--response-dtos-live-in-ersatztvcoreapi-file-scoped-nullable-enable)
|
||
- [2026-07 — PUT-replace list endpoints derive `Index` from array order; alternate-schedules last row = catch-all default](#2026-07--put-replace-list-endpoints-derive-index-from-array-order-alternate-schedules-last-row--catch-all-default)
|
||
- [2026-07 — Templates editor in the SPA is a table, not Blazor's drag-calendar](#2026-07--templates-editor-in-the-spa-is-a-table-not-blazors-drag-calendar)
|
||
- [2026-07-07 — API artwork contract: rooted URLs produced server-side](#2026-07-07--api-artwork-contract-rooted-urls-produced-server-side)
|
||
- [2026-07-07 — Decode-style endpoints take a row id and look up server-side](#2026-07-07--decode-style-endpoints-take-a-row-id-and-look-up-server-side)
|
||
- [2026-07-07 — Season/episode/music-video drill-in via `parentId`, not new child-listing endpoints](#2026-07-07--seasonepisodemusic-video-drill-in-via-parentid-not-new-child-listing-endpoints)
|
||
- [2026-07-07 — Convention docs read at session start, updated in-PR](#2026-07-07--convention-docs-read-at-session-start-updated-in-pr)
|
||
- [2026-07-09 — Playback-troubleshooting completion feedback: poll status, no push channel](#2026-07-09--playback-troubleshooting-completion-feedback-poll-status-no-push-channel)
|
||
- [2026-07-09 — datetime-local instead of Chronic natural-language start parsing](#2026-07-09--datetime-local-instead-of-chronic-natural-language-start-parsing)
|
||
- [2026-07-09 — SPA gates Download Media Sample while a session is active](#2026-07-09--spa-gates-download-media-sample-while-a-session-is-active)
|
||
- [2026-07-09 — OpenAPI spec mirrors the runtime Newtonsoft serializer (#198)](#2026-07-09--openapi-spec-mirrors-the-runtime-newtonsoft-serializer-198)
|
||
- [2026-07-09 — YAML playout validator: paste-textarea instead of a server file path](#2026-07-09--yaml-playout-validator-paste-textarea-instead-of-a-server-file-path)
|
||
- [2026-07-09 — Channel numbers: prompt-driven sequential renumber instead of drag-to-reorder](#2026-07-09--channel-numbers-prompt-driven-sequential-renumber-instead-of-drag-to-reorder)
|
||
- [2026-07-09 — "Table, not calendar" convention also covers the deco-templates editor](#2026-07-09--table-not-calendar-convention-also-covers-the-deco-templates-editor)
|
||
- [2026-07-11 — Trash "See all" reuses library-browse paging; search stays capped per kind (#213)](#2026-07-11--trash-see-all-reuses-library-browse-paging-search-stays-capped-per-kind-213)
|
||
- [2026-07-11 — Logs page-size is a client-local preference, not a server ConfigElement](#2026-07-11--logs-page-size-is-a-client-local-preference-not-a-server-configelement)
|
||
- [2026-07-11 — Logs column sorting: allow-listed `sortField`/`sortDirection` on `GET /api/logs`](#2026-07-11--logs-column-sorting-allow-listed-sortfieldsortdirection-on-get-apilogs)
|
||
- [2026-07-09 — Per-playout "Schedule reset" button dropped; Reset uses the server-default build mode](#2026-07-09--per-playout-schedule-reset-button-dropped-reset-uses-the-server-default-build-mode)
|
||
- [2026-07-09 — Collection custom order: move up/down buttons, any-kind collections](#2026-07-09--collection-custom-order-move-updown-buttons-any-kind-collections)
|
||
- [2026-07-10 — Shared "Add to…" layer lives in `web/src/media/addTo/`; select-mode is an explicit toggle](#2026-07-10--shared-add-to-layer-lives-in-websrcmediaaddto-select-mode-is-an-explicit-toggle)
|
||
- [2026-07-10 — Schedule-item GET returns a flat, non-polymorphic DTO (`ScheduleItemResponseModel`)](#2026-07-10--schedule-item-get-returns-a-flat-non-polymorphic-dto-scheduleitemresponsemodel)
|
||
- [2026-07-10 — Playout API mutations return 409 while the build lock is held (#215)](#2026-07-10--playout-api-mutations-return-409-while-the-build-lock-is-held-215)
|
||
- [2026-07-11 — Schedules SPA editor: draft/explicit-Save over instant-persist; Copy includes multi/smart/rerun; shuffled-GET normalization preserved](#2026-07-11--schedules-spa-editor-draftexplicit-save-over-instant-persist-copy-includes-multismartrerun-shuffled-get-normalization-preserved)
|
||
- [2026-07-11 — Channel editor: bare-create entry point + external-logo mutual exclusion (#212)](#2026-07-11--channel-editor-bare-create-entry-point--external-logo-mutual-exclusion-212)
|
||
- [2026-07-11 — EntityLocker: atomic flags + single-owner release discipline, no owner tokens (#231)](#2026-07-11--entitylocker-atomic-flags--single-owner-release-discipline-no-owner-tokens-231)
|
||
- [2026-07-11 — Channels screen extraction (#244): single-file screen, no sibling helper dir (epic #243 phase 1)](#2026-07-11--channels-screen-extraction-244-single-file-screen-no-sibling-helper-dir-epic-243-phase-1)
|
||
- [2026-07-14 — Playouts screen extraction (#245): screen-owned route wrapper (epic #243 phase 2)](#2026-07-14--playouts-screen-extraction-245-screen-owned-route-wrapper-epic-243-phase-2)
|
||
- [2026-07-11 — Media-source management REST write API + SPA (#202)](#2026-07-11--media-source-management-rest-write-api--spa-202)
|
||
- [2026-07-11 — Legacy→SPA redirect matcher: exact map + ordered segment-template patterns (#204)](#2026-07-11--legacyspa-redirect-matcher-exact-map--ordered-segment-template-patterns-204)
|
||
- [2026-07-11 — Blazor removal auth posture: no new exposure beyond phase (a); real auth deferred to #197 (#206)](#2026-07-11--blazor-removal-auth-posture-no-new-exposure-beyond-phase-a-real-auth-deferred-to-197-206)
|
||
- [2026-07-11 — Pre-removal Blazor rollback tag `blazor-final` (#205)](#2026-07-11--pre-removal-blazor-rollback-tag-blazor-final-205)
|
||
- [2026-07-11 — Async-op API contract normalization + playout build observability + F9 scan endpoints (#235)](#2026-07-11--async-op-api-contract-normalization--playout-build-observability--f9-scan-endpoints-235)
|
||
- [2026-07-11 — Optimistic-concurrency contract for replace-all PUTs (#253 PR1: infra + Block reference)](#2026-07-11--optimistic-concurrency-contract-for-replace-all-puts-253-pr1-infra--block-reference)
|
||
- [2026-07-11 — Post-commit side effects run on `CancellationToken.None` (generalized from #251 to #254)](#2026-07-11--post-commit-side-effects-run-on-cancellationtokennone-generalized-from-251-to-254)
|
||
- [2026-07-11 — #253 PR3: Diff + Scalar concurrency fan-out (Collection / Playout×2 / MultiCollection / RerunCollection)](#2026-07-11--253-pr3-diff--scalar-concurrency-fan-out-collection--playout2--multicollection--reruncollection)
|
||
- [2026-07-12 — External-collections scans get an authoritative status surface (#271); the SPA timeout is retired](#2026-07-12--external-collections-scans-get-an-authoritative-status-surface-271-the-spa-timeout-is-retired)
|
||
- [2026-07-11 — Blazor Server UI removed (#91 phase b)](#2026-07-11--blazor-server-ui-removed-91-phase-b)
|
||
- [2026-07-11 — Stable child identity for schedule-item replace (#259, split from #252/#253)](#2026-07-11--stable-child-identity-for-schedule-item-replace-259-split-from-252253)
|
||
- [2026-07-11 — Baseline security response headers + Phase-0 API hardening (#197, PR #279)](#2026-07-11--baseline-security-response-headers--phase-0-api-hardening-197-pr-279)
|
||
- [2026-07-12 — Artwork content-type is sniffed, never reflected (#283, S4/S9 stored XSS)](#2026-07-12--artwork-content-type-is-sniffed-never-reflected-283-s4s9-stored-xss)
|
||
- [2026-07-12 — Fail-closed API auth + sensitive-read tier + CORS/ForwardedHeaders lockdown (#197 Bundle A, PR #292)](#2026-07-12--fail-closed-api-auth--sensitive-read-tier--corsforwardedheaders-lockdown-197-bundle-a-pr-292)
|
||
- [2026-07-12 (#197 Bundle C — contract-freeze honesty)](#2026-07-12-197-bundle-c--contract-freeze-honesty)
|
||
- [2026-07-12 (#269 — non-If-Match root writers force-write past a concurrent Version bump)](#2026-07-12-269--non-if-match-root-writers-force-write-past-a-concurrent-version-bump)
|
||
- [2026-07-12 — Browser SPA session auth: `/api` accepts session OR machine key (#295 PR1, server-only)](#2026-07-12--browser-spa-session-auth-api-accepts-session-or-machine-key-295-pr1-server-only)
|
||
- [2026-07-12 — Merge-consent derived from state via a `## Done-when` issue checklist (#303 H6)](#2026-07-12--merge-consent-derived-from-state-via-a--done-when-issue-checklist-303-h6)
|
||
- [2026-07-12 — Blocking CI gate for API-contract artifacts (#303 H4/H5)](#2026-07-12--blocking-ci-gate-for-api-contract-artifacts-303-h45)
|
||
- [2026-07-12 — decisions.md is append-only, enforced; root-screenshot guard (#303 H9/H3)](#2026-07-12--decisionsmd-is-append-only-enforced-root-screenshot-guard-303-h9h3)
|
||
- [2026-07-12 — Review-verdict merge-gate: latest commit must be reviewed (#303 H10)](#2026-07-12--review-verdict-merge-gate-latest-commit-must-be-reviewed-303-h10)
|
||
- [2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)](#2026-07-12--cross-editor-etag-rotation-completed-for-collectionplayout-config-siblings-269)
|
||
- [2026-07-12 — Live-E2E is a required step for API write-path handler changes (#303)](#2026-07-12--live-e2e-is-a-required-step-for-api-write-path-handler-changes-303)
|
||
- [2026-07-12 — Formatting-as-you-touch, enforced; rebase-not-merge for PR branches (#311 H11 + format CI)](#2026-07-12--formatting-as-you-touch-enforced-rebase-not-merge-for-pr-branches-311-h11--format-ci)
|
||
- [2026-07-12 — Merge-consent gate auto-grants when satisfied (no redundant prompt); state IS the consent (#314)](#2026-07-12--merge-consent-gate-auto-grants-when-satisfied-no-redundant-prompt-state-is-the-consent-314)
|
||
- [2026-07-12 — Release path rehearses migrations on a prod-DB copy before promoting (#315)](#2026-07-12--release-path-rehearses-migrations-on-a-prod-db-copy-before-promoting-315)
|
||
- [2026-07-12 — #295 PR2: SPA session cutover + #301 side-effecting-GET POST-ification](#2026-07-12--295-pr2-spa-session-cutover--301-side-effecting-get-post-ification)
|
||
- [2026-07-12 — Enforcing CSP + Permissions-Policy on the host (#319, ZAP baseline)](#2026-07-12--enforcing-csp--permissions-policy-on-the-host-319-zap-baseline)
|
||
- [2026-07-12 — If-Match evaluates per RFC 7232: valid-but-non-matching → 412, only grammar violations → 400 (#265)](#2026-07-12--if-match-evaluates-per-rfc-7232-valid-but-non-matching--412-only-grammar-violations--400-265)
|
||
- [2026-07-12 — TopBar primary-action button: wire creates, drop the rest (#238)](#2026-07-12--topbar-primary-action-button-wire-creates-drop-the-rest-238)
|
||
- [2026-07-13 — API versioning: the whole `/api` surface is mounted at `/api/v1`, additive-only after freeze (#286)](#2026-07-13--api-versioning-the-whole-api-surface-is-mounted-at-apiv1-additive-only-after-freeze-286)
|
||
- [2026-07-13 — Scheduling API hardening: null-name 500s, duplicate template items, unreachable 404 (#172)](#2026-07-13--scheduling-api-hardening-null-name-500s-duplicate-template-items-unreachable-404-172)
|
||
- [2026-07-13 — Release promotion: floating `:prod`, exact-image scan before manual deploy (#335)](#2026-07-13--release-promotion-floating-prod-exact-image-scan-before-manual-deploy-335)
|
||
|
||
---
|
||
|
||
## 2026-06 — REST API wraps existing MediatR handlers 1:1, no service layer
|
||
|
||
The REST API (#2, `docs/rest-api.md`) is thin controllers over the existing MediatR
|
||
Create/Update/Delete handlers — no new service/business-logic layer was introduced, since nearly
|
||
every handler already returns `Either<BaseError, T>`, which maps cleanly to HTTP status codes.
|
||
Latent handler bugs (missing existence checks, `KeyNotFoundException` risk, etc.) are fixed **at
|
||
the handler**, converting what would have 500'd into a proper 404/422 — not papered over in the
|
||
controller. Established across the #2a–#2e gap-issue PRs. Deep FK ids nested inside item-list
|
||
request bodies (e.g. a schedule item's `CollectionId`) are deliberately **not** existence-checked at
|
||
that depth, to avoid N+1 validation queries — precedent set by the schedules endpoints (#172); see
|
||
`docs/api-conventions.md` §3 for the up-to-date statement of this rule.
|
||
|
||
## 2026-06 — UI rebuild is a React SPA (ChicoryTV) on the REST API, not a Blazor reskin
|
||
|
||
#59 committed to a full SPA rebuild rather than reskinning Blazor Server pages. Blazor removal is
|
||
split into two phases under #91: **(a)** root-flip (SPA becomes `/`) + legacy-route redirects —
|
||
DONE, merged via PR #148 (`ErsatzTV/LegacyUiRedirects.cs`, `feat/91-cutover` → main). **(b)** full
|
||
Blazor removal — gated on every route having an SPA equivalent; tracked route-by-route in
|
||
`docs/blazor-route-parity.md`.
|
||
|
||
## 2026-07 — Response DTOs live in `ErsatzTV.Core/Api`, file-scoped `#nullable enable`
|
||
|
||
New REST response DTOs go in `ErsatzTV.Core/Api/<Domain>/*ResponseModel.cs` and mirror the shape of
|
||
the corresponding Application-layer ViewModel — controllers never expose VM types directly. Because
|
||
`ErsatzTV.Core.csproj` sets `<Nullable>disable</Nullable>` project-wide, any response-model file
|
||
with an optional member needs its own `#nullable enable` pragma at the top (most already have one).
|
||
`ErsatzTV.Application` has no nullable context at all — do not add `?` annotations to types living
|
||
there; that's a Core/Api-layer-only convention. Full detail: `docs/api-conventions.md` §2.
|
||
|
||
## 2026-07 — PUT-replace list endpoints derive `Index` from array order; alternate-schedules last row = catch-all default
|
||
|
||
For "replace the whole list" endpoints (PUT over a collection — schedule items, template items,
|
||
etc.), the item's `Index` is derived from its position in the request array, not from a
|
||
client-supplied index/order field — established by `ReplaceScheduleItemsRequest.ToCommand`
|
||
(`Items.Select((item, index) => item.ToReplaceCommand(index))`). Separately, `ProgramScheduleAlternate`
|
||
and `PlayoutTemplate` rows (both `IAlternateScheduleItem`) are evaluated in `Index` order,
|
||
first-match-wins; the convention is to place the least-conditional (or unconditional) row **last**
|
||
so it acts as the catch-all default. Established by the alternate-schedules work (PR #179,
|
||
`AlternateScheduleSelector.cs`).
|
||
|
||
## 2026-07 — Templates editor in the SPA is a table, not Blazor's drag-calendar
|
||
|
||
The legacy Blazor `TemplateEditor.razor` used a drag-and-drop day-grid calendar UI. The SPA
|
||
equivalent (`/app/templates/{id}`, PR #173) renders the same day/block assignment as a table
|
||
instead. This is an accepted, deliberate parity deviation — don't "fix" it to match Blazor's
|
||
interaction model without discussing it first.
|
||
|
||
## 2026-07-07 — API artwork contract: rooted URLs produced server-side
|
||
|
||
API response DTOs return artwork as rooted, directly-usable URLs (`/artwork/posters/...`,
|
||
`/artwork/thumbnails/...`, `/artwork/fanart/...`), plus passthrough for absolute `http(s)://` URLs
|
||
and Jellyfin/Emby proxy variants. Established by PR #181
|
||
(`ErsatzTV.Application/LibraryBrowse/Queries/GetLibraryBrowseItemsHandler.cs`, private `Artwork(...)`
|
||
helper — comment: *"Returns a rooted, directly-usable artwork URL for the SPA's `<img src>`... the
|
||
SPA [needs it pre-rooted]"*), then generalized into the reusable `ApiArtwork` helper
|
||
(`ErsatzTV.Core/Api/ApiArtwork.cs`, PR #183). Root cause: the SPA has no `<base href>`, unlike
|
||
Blazor, so relative artwork paths that worked for Blazor pages 404 in the SPA. Do **not** reuse the
|
||
Application-layer Mappers used by Blazor (e.g. `MediaCards`/`Television` mappers) for new API
|
||
DTOs — those still return old Blazor-convention relative paths; map from the domain/VM directly and
|
||
root the path via `ApiArtwork`.
|
||
|
||
## 2026-07-07 — Decode-style endpoints take a row id and look up server-side
|
||
|
||
Endpoints that decode/expand opaque stored state accept a database row id and resolve server-side,
|
||
rather than accepting client-supplied serialized state to decode. Established by
|
||
`GET /api/playouts/history/{id}` (`PlayoutController.GetHistoryDetails`, PR #182) — the row's raw
|
||
JSON (`Key`/`Details`) is decoded server-side into `PlayoutHistoryDetailsResponseModel`, the client
|
||
never round-trips the raw payload itself.
|
||
|
||
## 2026-07-07 — Season/episode/music-video drill-in via `parentId`, not new child-listing endpoints
|
||
|
||
Rather than adding dedicated child-listing endpoints per media kind (e.g. "list episodes of a
|
||
season"), the library-browse endpoint takes an optional `parentId` query param and the SPA drills
|
||
in by re-querying with it. Established across PRs #181/#183 (library-picker season drill-in, then
|
||
media-detail's season/episode/artist/music-video browsing). Avoids a combinatorial explosion of
|
||
per-kind child endpoints.
|
||
|
||
## 2026-07-07 — Convention docs read at session start, updated in-PR
|
||
|
||
`docs/api-conventions.md`, `docs/spa-conventions.md`, `docs/e2e-local.md`,
|
||
`docs/blazor-route-parity.md`, `docs/domain-model.md`, `docs/decisions.md`, and `docs/README.md`
|
||
are the standing reference set every ChicoryTV session should read before starting work, and each
|
||
one carries an explicit "update this doc in the same PR" rule rather than deferring doc updates to
|
||
a follow-up. These docs **replace per-session recon** — an agent reads the index
|
||
(`docs/README.md`) and the relevant convention doc instead of re-deriving conventions from the code
|
||
each time it starts API/SPA/E2E/parity work. A testing map and a generated-endpoint index are
|
||
tracked as still-to-come under #185. Drafting this doc set also surfaced a drift in
|
||
`ApiControllerSecurityTests.cs`'s hardcoded controller registry (several controllers under
|
||
`ErsatzTV/Controllers/Api/` are missing from it — see `docs/api-conventions.md` §6) — tracked as a
|
||
follow-up under #184 rather than fixed inline, since it's a pre-existing gap, not something this
|
||
doc-drafting pass caused.
|
||
|
||
## 2026-07-09 — Playback-troubleshooting completion feedback: poll status, no push channel
|
||
|
||
The SPA playback-troubleshooting screen (`PlaybackTroubleshootingScreen.tsx`, #145) reports FFmpeg
|
||
completion by **polling `GET /api/troubleshoot/playback/status` every ~2s** while a session is
|
||
running (plus one poll on mount so a session started elsewhere still gates Play), rather than a
|
||
server push. The status endpoint returns `{ state, exitCode, speed, logs }`; the screen captures the
|
||
running→completed/failed transition in local component state and surfaces a completion notice
|
||
(success on exit 0, warning otherwise) — the SPA equivalent of the Blazor page's MediatR
|
||
`ICourier`/`ISnackbar` `PlaybackTroubleshootingCompletedNotification`. Chosen over SignalR/SSE
|
||
because the SPA has **no push channel** and troubleshooting sessions are short and user-initiated, so
|
||
a lightweight poll (started on Play, stopped on settle/unmount) is simpler than standing up a new
|
||
real-time transport. Speed thresholds and the "(Speed: Nx)" badge colors are copied verbatim from the
|
||
Blazor `GetSpeedClass` (red <0.9, green >1.1, amber otherwise).
|
||
|
||
## 2026-07-09 — datetime-local instead of Chronic natural-language start parsing
|
||
|
||
The channel-mode "Date and Time" input in the SPA playback-troubleshooting screen uses a native
|
||
`<input type="datetime-local">`, a **deliberate deviation** from the Blazor page, which parsed a
|
||
free-text field with `Chronic.Core.Parser` (natural language like "yesterday at 8pm"). The SPA has no
|
||
Chronic dependency and a picker is unambiguous; the selected local datetime is sent to
|
||
`playback.m3u8` as an ISO-8601 `start` param via `new Date(value).toISOString()`, which the
|
||
controller binds to `DateTimeOffset?` exactly as the Blazor round-trip (`"o"`) format did.
|
||
|
||
## 2026-07-09 — SPA gates Download Media Sample while a session is active
|
||
|
||
Minor intentional deviation: the SPA playback-troubleshooting screen disables **Download Media
|
||
Sample** (alongside Download Results) while a troubleshooting session is starting/running; Blazor
|
||
only gated Download Results. Both downloads compete with the live transcode for I/O and the sample
|
||
archiver reads the same media file, so gating both during a session is strictly safer and costs
|
||
nothing (sessions are short).
|
||
|
||
## 2026-07-09 — OpenAPI spec mirrors the runtime Newtonsoft serializer (#198)
|
||
|
||
The generated OpenAPI document is made to follow the **runtime** JSON contract, not the reverse. Runtime
|
||
`/api/*` responses are serialized by Newtonsoft via `CustomContractResolver`/`CustomNamingStrategy`
|
||
(camelCase + a `FFmpegProfileId`→`ffmpegProfileId` special case + `[JsonProperty]` overrides such as
|
||
`ChannelResponseModel.FFmpegProfile`→`ffmpegProfile`), while `Microsoft.AspNetCore.OpenApi` generates the
|
||
spec from System.Text.Json metadata, whose camelCase drifted (`fFmpegProfileId`, `fFmpegProfile`). That
|
||
drift fed the SPA the wrong key. Rather than hand-patch the spec or change the wire format (breaking clients),
|
||
we added `NewtonsoftSchemaNamingTransformer` — an OpenAPI schema transformer registered on all three
|
||
documents that renames each schema property through the *same* Newtonsoft contract resolver the runtime uses,
|
||
so the spec matches the wire format by construction. A contract test
|
||
(`OpenApiSerializerContractTests`) serializes representative DTOs through the real runtime settings and pins
|
||
the spec property sets to them. Decision: **the wire format is the source of truth; the spec follows it via the
|
||
real contract resolver.** This also fixed a latent SPA bug (the channel-list "FFmpeg profile" column read
|
||
`fFmpegProfile` and always showed "Unassigned"). Issue #198.
|
||
|
||
## 2026-07-09 — YAML playout validator: paste-textarea instead of a server file path
|
||
|
||
The legacy Blazor YAML playout validator took a **server-side file path** (read directly off the
|
||
container's filesystem). The SPA's `YamlValidatorScreen.tsx` instead uses a paste `<textarea>` — a
|
||
deliberate deviation, not an oversight. The SPA runs entirely client-side against `/api/*` and has
|
||
no access to the server's filesystem, so a file-path field would either need a new
|
||
filesystem-browsing endpoint or silently fail; pasting the YAML directly is simpler and matches how
|
||
every other SPA editor already round-trips content through the API instead of the disk.
|
||
|
||
## 2026-07-09 — Channel numbers: prompt-driven sequential renumber instead of drag-to-reorder
|
||
|
||
The legacy Blazor channel list let you drag-and-drop rows to reorder channel numbers. The SPA
|
||
(`App.tsx`) instead offers a "Renumber" action that walks the list and asks for each channel's new
|
||
number via a sequence of native `prompt()` calls. Deliberate deviation: drag-to-reorder needs a
|
||
dedicated drag library and a bespoke reorder-persistence endpoint; a sequential prompt reuses the
|
||
existing per-channel update call and needs no new UI dependency. Revisit only if channel counts grow
|
||
large enough that prompt-per-channel becomes tedious.
|
||
|
||
## 2026-07-09 — "Table, not calendar" convention also covers the deco-templates editor
|
||
|
||
Extends the 2026-07 "Templates editor in the SPA is a table, not Blazor's drag-calendar" entry
|
||
above (not editing that entry — this generalizes it): the deco-templates editor
|
||
(`DecoTemplatesScreen.tsx`) follows the same convention, rendering its day/deco assignment as a
|
||
table rather than reproducing Blazor's drag-and-drop calendar grid. Same rationale, same
|
||
accepted-deviation status — don't "fix" either editor to match Blazor's interaction model without
|
||
discussing it first.
|
||
|
||
## 2026-07-11 — Trash "See all" reuses library-browse paging; search stays capped per kind (#213)
|
||
|
||
`GET /api/v1/search` still returns at most 100 items per media kind, which is the cheap first page for
|
||
the common case. For an overflowing kind, the SPA's "See all N …" action pages `GET
|
||
/api/v1/library/browse` with `query=state:FileNotFound`, `mediaType`, `pageNum`, and `pageSize=100`, then
|
||
appends the results client-side. This reuses the same `GetLibraryBrowseItems` query behind search,
|
||
adds no API surface, and only pays for follow-up requests when a kind exceeds the first-page cap.
|
||
|
||
## 2026-07-11 — Logs page-size is a client-local preference, not a server ConfigElement
|
||
|
||
The legacy Blazor Logs page persisted the user's chosen rows-per-page via
|
||
`ConfigElementKey.LogsPageSize` (`SaveConfigElementByKey`/`GetConfigElementByKey`), a
|
||
per-server-instance setting stored in the DB. `LogsScreen.tsx` instead persists it to
|
||
`window.localStorage` under `ctv-logs-page-size` (same wrapped-`Storage` pattern as
|
||
`designSystem.ts`'s theme preference: try/catch getter, validated against the known option set,
|
||
falls back to a default) and restores it on mount. Deliberate deviation: this is a per-browser UI
|
||
preference, not server/business state — no other client should see or be affected by it, so there
|
||
is no reason to round-trip it through the API and grow a new `/api/*` surface (or reuse the
|
||
generic config-element endpoints) just to store a page-size number. Follows the existing SPA
|
||
localStorage convention (`designSystem.ts` theme, `auth.ts` token) rather than introducing a new
|
||
persistence mechanism.
|
||
|
||
## 2026-07-11 — Logs column sorting: allow-listed `sortField`/`sortDirection` on `GET /api/logs`
|
||
|
||
Parity for `Logs.razor`'s `MudTableSortLabel` columns (Timestamp, Level — Message was never
|
||
sortable in Blazor either). `LogsController.GetLogs` adds `sortField` (`timestamp` | `level`,
|
||
default `timestamp`) and `sortDirection` (`asc` | `desc`, default `desc`) query params, normalized
|
||
server-side the same way `pageNum`/`pageSize` are clamped rather than rejected with a 422: an
|
||
unrecognized `sortField` silently falls back to `timestamp`, an unrecognized `sortDirection` falls
|
||
back to `desc` — the pre-existing default behavior (newest-first) is unreachable to break via a bad
|
||
query string. `LogsScreen.tsx` renders the two sortable headers as buttons with a chevron
|
||
indicating the active field/direction; clicking the active column toggles direction, clicking the
|
||
other column switches to it ascending.
|
||
|
||
## 2026-07-09 — Per-playout "Schedule reset" button dropped; Reset uses the server-default build mode
|
||
|
||
Blazor's playouts page had both a per-playout **Reset** and a separate **Schedule Reset** control
|
||
(setting the daily rebuild time). The SPA keeps Reset — `POST
|
||
/api/channels/{channelNumber}/playout/reset` with no `mode` param, so the server picks the same
|
||
default Blazor used (Classic → Refresh, everything else → Reset) — but drops the dedicated
|
||
"Schedule reset" button: the daily rebuild time is already editable through the playout's
|
||
Edit-details flow, so a second entry point would duplicate an existing capability. Deliberate
|
||
deviation, not a lost capability. Issue #210.
|
||
|
||
## 2026-07-09 — Collection custom order: move up/down buttons, any-kind collections
|
||
|
||
Blazor reordered collection items with SortableJS drag-and-drop and only enabled custom ordering
|
||
for movies-only collections. The SPA (`CollectionsScreen.tsx`) uses per-row **Move up / Move
|
||
down** buttons in an explicit reorder mode instead of drag (no new drag dependency; the mode loads
|
||
ALL items first because `PUT /api/collections/{id}/custom-order` replaces the whole order from
|
||
array position — submitting a partial page would scramble the rest), and does **not** replicate
|
||
the movies-only gate: the API and the playback-side `CustomOrderCollectionEnumerator` sort by
|
||
`CustomIndex` regardless of item kind, so the SPA offers reorder for any manual collection with
|
||
custom order enabled. Issue #211.
|
||
|
||
## 2026-07-10 — Shared "Add to…" layer lives in `web/src/media/addTo/`; select-mode is an explicit toggle
|
||
|
||
The media mutation surface (#208/#209) is built on one reusable component group,
|
||
`web/src/media/addTo/` (media-domain components, like `MediaPosterCard` — not generic
|
||
`components/`): `AddToCollectionDialog` (existing-collection Select + inline "(New collection)"
|
||
create, Blazor `AddToCollectionDialog.razor` parity), `AddToPlaylistDialog` (group → playlist
|
||
Selects, no inline create), `AddToScheduleDialog` (schedule Select; payload replicates Blazor's
|
||
`AddProgramScheduleItem.ForMediaItem` defaults — see `addTo/scheduleItem.ts`),
|
||
`SaveAsSmartCollectionDialog`, and `AddToMenu` (the drop-in popover for cards/detail pages via
|
||
`MediaPosterCard`'s `actions` slot). New screens wanting add-to affordances use this layer —
|
||
don't build screen-local pickers. Two deliberate deviations from Blazor, applied consistently on
|
||
the search and browse screens: **(1) multi-select is an explicit screen-level "Select" toggle**
|
||
(off = cards open, on = cards select) rather than Blazor's always-on corner-select, because
|
||
`MediaPosterCard`'s select handler takes over the card's single click gesture; **(2) the
|
||
per-card menu offers collection/playlist for a single item of any kind, plus schedule only for
|
||
shows/seasons/artists** — collection/playlist is a superset of Blazor's per-card collection-only
|
||
menu, while the schedule target is gated to exactly the kinds Blazor's
|
||
`AddProgramScheduleItem.ForMediaItem` call sites offer, because the server validator
|
||
(`ProgramScheduleItemCommandBase.CollectionTypeMustBeValid`) accepts only the
|
||
TelevisionShow/TelevisionSeason/Artist per-media-item CollectionTypes and 422s the rest.
|
||
"Add All" (query-wide) mirrors Blazor's two-step: materialize ids
|
||
via `GET /api/search/all-items`, then reuse the id-list add endpoints — no query-based add
|
||
command exists server-side. Issues #208/#209.
|
||
|
||
## 2026-07-10 — Schedule-item GET returns a flat, non-polymorphic DTO (`ScheduleItemResponseModel`)
|
||
|
||
`GET/POST/PUT /api/schedules/{id}/items` return `ScheduleItemResponseModel` /
|
||
`ScheduleItemsResponseModel` (`ErsatzTV.Core/Api/Scheduling/`), **not** the Application-layer
|
||
`ProgramScheduleItemViewModel` hierarchy (One/Flood/Multiple/Duration subtypes). The polymorphic VM
|
||
only described its base shape in OpenAPI, so the SPA couldn't see the subtype fields (issue #126).
|
||
The flat DTO promotes every subtype field to a nullable top-level member — `multipleMode`,
|
||
`multipleCount` (renamed from the VM's `Count`), `playoutDuration`, `tailMode`,
|
||
`discardToFillAttempts` — mapped by pattern-matching the concrete VM in
|
||
`ScheduleItemResponseMapper` (`ErsatzTV.Application/ProgramSchedules/`). Its **mutation fields are
|
||
named 1:1 with `ScheduleItemRequest`** so a GET maps losslessly back to a PUT/POST
|
||
(`ScheduleItemResponseRoundTripTests` is the release gate proving the fixed point). It also carries
|
||
picker-hydration fields the editor needs: `collectionName`/`smartCollectionName`/…/`playlistName`,
|
||
`playlistGroupId` (to preselect the playlist's group), per-filler names, `watermarks` /
|
||
`graphicsElements` as `NamedIdResponseModel` lists, the computed `name`, and `durationEstimate`.
|
||
`GetProgramScheduleItemsHandler.EnforceProperties` still rewrites StartType→Dynamic, Flood→One and
|
||
Playlist/Rerun→PlaybackOrder None when `ShuffleScheduleItems` is on — that lossy normalization is
|
||
deliberate and lives on the read side (documented + tested). New shared `NamedIdResponseModel`
|
||
(`ErsatzTV.Core/Api/`) is the generic `{id, name}` embed for API responses. Issues #126/#207/#212.
|
||
|
||
## 2026-07-10 — Playout API mutations return 409 while the build lock is held (#215)
|
||
|
||
Blazor disabled per-playout Reset/Erase/Delete/Edit while a `BuildPlayout` was in flight
|
||
(`EntityLocker.IsPlayoutLocked`, `Playouts.razor` + per-kind editors); the REST API had no
|
||
equivalent, so a client could race an in-flight build with a destructive `ExecuteDelete` and leave
|
||
a half-built playout. Adversarial-reviewer#18 promoted this to a #91-phase-(b) removal gate: after
|
||
Blazor is deleted the invariant would vanish entirely.
|
||
|
||
Decision: enforce the invariant **server-side** on the API rather than re-implementing a live push
|
||
channel. `PlayoutController` and `ChannelController` inject `IEntityLocker`; every id-keyed mutation
|
||
— `PUT /api/playouts/{id}`, `.../deco`, `.../alternate-schedules`, `.../templates`,
|
||
`POST .../erase-items`, `.../erase-items-and-history`, `DELETE /api/playouts/{id}`, and
|
||
`POST /api/channels/{channelNumber}/playout/reset` — checks `IsPlayoutLocked(id)` first and returns
|
||
**409 Conflict** (`ApiResults.ConflictProblem`, new shared helper mirroring `NotFoundProblem`) while
|
||
the build lock is held. The PUTs are gated too (not just the destructive ops): the target invariant
|
||
is "no mutation during a build", matching Blazor's edit-disable.
|
||
|
||
- **The guard is advisory check-then-act, not mutual exclusion** — same posture as Blazor's disabled
|
||
buttons. A `BuildPlayout` already sitting in the worker queue can take the lock a few milliseconds
|
||
after the check passes, so the original race is *narrowed*, not eliminated; consequences remain
|
||
self-healing (the next rebuild corrects a half-mutated playout). True prevention — having each
|
||
mutation acquire the playout lock for its duration — was deliberately not taken: `LockPlayout`
|
||
publishes `PlayoutUpdatedNotification` (UI churn per mutation) and would make mutations block
|
||
builds, a semantics change out of scope for restoring Blazor parity.
|
||
|
||
- **`reset-all` is deliberately NOT gated** — it stays 202. `ResetAllPlayoutsHandler` already
|
||
*silently skips* locked playouts, which matches Blazor and the handler semantics; a fire-and-forget
|
||
bulk enqueue always accepts.
|
||
- **SPA mirrors the lock via data, not a push channel** — `PlayoutListItemResponseModel` gains an
|
||
`IsLocked` bool (set from `IsPlayoutLocked` in the controller's list projection). The playouts
|
||
screen disables Reset/Erase/Erase-and-history/Delete for a locked row and shows a "Building…"
|
||
Badge; on a 409 from any mutation it surfaces the error and calls `query.refresh()` so the row
|
||
picks up the flag. No new polling was added (the existing 30s channel-state poll is unchanged).
|
||
|
||
Precedent for the 409 shape: `TraktController` (left as-is with its own private `ConflictProblem()`
|
||
to keep the diff small). Convention recorded in `api-conventions.md` §3a.
|
||
|
||
## 2026-07-11 — Schedules SPA editor: draft/explicit-Save over instant-persist; Copy includes multi/smart/rerun; shuffled-GET normalization preserved
|
||
|
||
The ChicoryTV schedules editor (`web/src/screens/SchedulesScreen.tsx` + `web/src/schedules/`,
|
||
issue #207) rebuilds the schedule-item lineup to full mutation parity with the legacy Blazor editor.
|
||
Three deliberate decisions:
|
||
|
||
**(a) Draft model with one explicit Save, replacing instant-persist.** All add/edit/copy/remove/
|
||
reorder mutate a **local draft list only**; a single **Save** issues one
|
||
`PUT /api/schedules/{id}/items` (the replace endpoint). This is intentional because that PUT is
|
||
**destructive server-side** — it deletes and recreates every item row (new ids) and triggers playout
|
||
rebuilds — so batching edits into one flush (vs. the old per-action POST/DELETE/PUT) minimizes churn
|
||
and gives the user a Discard/dirty affordance. On 422/network error the draft is kept and the error
|
||
surfaced; on success the draft is replaced with the server response. A **Discard** action and a dirty
|
||
guard (native `confirm()` on schedule-switch + in-app nav via `navigationGuard.ts`, plus a
|
||
`beforeunload` listener) protect the draft. See `docs/spa-conventions.md` §8. This retires the old
|
||
inline `ScheduleScreen` in `App.tsx` and its instant-persist add/delete/reorder tests.
|
||
|
||
**(b) Copy item deep-copies ALL source references, including multi/smart/rerun collections.** Blazor's
|
||
`CopyItem` omitted the multi-collection / smart-collection / rerun-collection references when
|
||
duplicating an item (copying only the plain collection/media-item/playlist refs) — a latent bug. The
|
||
SPA's `copyDraftItem` (`web/src/schedules/itemRules.ts`) copies every source field + display name, so
|
||
copying a MultiCollection/SmartCollection/Rerun item preserves its source. Deliberate deviation
|
||
fixing the Blazor omission.
|
||
|
||
**(c) The shuffled-schedule GET normalization (`EnforceProperties`) is preserved lossiness, matching
|
||
Blazor.** When a schedule has `ShuffleScheduleItems`, `GET .../items` still rewrites startType→Dynamic,
|
||
Flood→One, and Playlist/Rerun playbackOrder→None (and zeroes discardToFillAttempts for non-random
|
||
Duration items). The SPA does **not** fight this — it hides the Fixed start type and Flood playout
|
||
mode from the option lists (and disables the reorder arrows) for shuffled schedules, mirroring Blazor,
|
||
so a GET→edit→PUT round-trip stays consistent with the server's read-side normalization. Issue #207.
|
||
|
||
## 2026-07-11 — Channel editor: bare-create entry point + external-logo mutual exclusion (#212)
|
||
|
||
Two Blazor-parity decisions closing the channel-editor gaps (`ChannelEditor.razor` +
|
||
`ChannelEditViewModel`):
|
||
|
||
1. **Bare-channel create lives on the channels list, not a form-first route.** Blazor's
|
||
`/channels` (no `Id`) is a full add form; the SPA instead adds a "New blank channel" action next
|
||
to "Add Channel" on `ChannelsScreen` (`web/src/App.tsx`) that POSTs `CreateChannelRequest` with
|
||
Blazor's computed add-mode defaults (`(max existing int-parsed channel number) + 1`, `name: "New
|
||
Channel"`, `group: "ErsatzTV"`, `ffmpegProfileId` = `GET /api/settings/ffmpeg`'s
|
||
`defaultFFmpegProfileId`, `streamingMode: "TransportStreamHybrid"`, `isEnabled`/`showInEpg: true`,
|
||
every other field at its C# `default(T)` — see `ChannelEditor.razor`'s `else` branch for the
|
||
source of truth) directly, then navigates to `/app/edit-channel/{id}` for the rest of the fields.
|
||
This is a deliberate equivalent, not a parity gap: it reuses the existing full editor instead of
|
||
duplicating its ~20 fields into a second form. "Add Channel" (`/app/new-channel`, the
|
||
library-to-lineup `ChannelBuilder` flow) is unrelated and untouched.
|
||
2. **External logo URL wins over an uploaded logo, mirroring
|
||
`ChannelEditViewModel.ToUpdate/ToCreate`.** The channel's logo is `ArtworkContentTypeModel {
|
||
path, contentType, isExternalUrl }`; on hydration, `isExternalUrl: true` populates a separate
|
||
"External logo URL" field and the uploaded-logo draft state is treated as empty (`EMPTY_LOGO =
|
||
{ path: '', contentType: '' }` in `ChannelEditScreen.tsx`) so the two never disagree. On submit,
|
||
a non-blank URL always wins: `logo: { path: url, contentType: '' }`, exactly matching
|
||
`ExternalLogoUrl`'s precedence in the C# view model. Uploading a file clears the URL field (the
|
||
last-set field wins, Blazor parity via `UploadLogo`'s `_model.ExternalLogoUrl = null`). The URL
|
||
is validated as http(s) client-side before save is enabled (Blazor has no equivalent validation;
|
||
added because the field is free text with no server-side format check surfaced to the SPA).
|
||
|
||
Also landed with #212: `preferredAudioLanguageCode`/`preferredSubtitleLanguageCode`,
|
||
`musicVideoCreditsTemplate`, and `streamSelector` moved from free-text `Input`s to `Select`s fed by
|
||
`GET /api/languages` / `/api/channels/music-video-credits-templates` /
|
||
`/api/channels/stream-selectors`. Each keeps the channel's currently-stored value selectable even if
|
||
it's absent from the reference list (`optionsKeepingCurrent` in `ChannelEditScreen.tsx`) so loading
|
||
an existing channel never silently changes the value out from under an unmanaged language code or a
|
||
template/selector file removed from disk since save.## 2026-07-11 — Queue state lives in the pinned Gitea tracker (#237), not in the handoff file
|
||
|
||
With multiple sessions/agents working the repo in parallel, the old protocol — every session
|
||
wholesale-rewrites `docs/handoffs/chicorytv-issue-queue.md` on main (session state + queue +
|
||
next-session prompt) — became a last-writer-wins race. New protocol: **volatile queue state
|
||
moved to Gitea**, which is concurrency-safe by construction. Pinned tracker issue **#237**
|
||
holds the goal + ordered arc in its body (edited rarely, only on arc changes, re-read before
|
||
edit) and an append-only session-comment log (fixed template: Closed / Filed / Triage /
|
||
Arc change / Recommended next). Milestone `Blazor removal (#91 phase b)` + the `review` and
|
||
`in-progress` labels are the machine-queryable view. Sessions **claim** an issue before working
|
||
it (`in-progress` label + claim comment; the tiny read→claim race window is accepted, later
|
||
claimant backs off; stale claims — no commits/comments ~48h — may be taken over with a comment).
|
||
Every new issue gets an explicit end-of-session triage verdict — gate-blocker (milestone + arc
|
||
slot) or backlog (label only) — so review findings adjust the queue only through that step and
|
||
the arc doesn't drift. The handoff file keeps only the **static kickoff prompt** and the
|
||
**append-only Lessons lore** (per-session prompts are gone; task context lives in issue bodies).
|
||
|
||
## 2026-07-11 — EntityLocker: atomic flags + single-owner release discipline, no owner tokens (#231)
|
||
|
||
`EntityLocker` (process-wide singleton, `ErsatzTV.Infrastructure/Locking/EntityLocker.cs`) is the
|
||
advisory "operation on entity X is in progress" mutex layer. Adversarial review (#20/F5, →#231)
|
||
found three defects: six plain-`bool` flags with a non-atomic check-then-set (two threads could both
|
||
acquire and both return `true`), tokenless `Unlock*` letting any caller release another owner's lock
|
||
(e.g. `BuildPlayoutHandler` ignored `LockPlayout`'s return then unconditionally unlocked in
|
||
`finally`), and one batch taking a single `LockLibrary` released after the *first* of two enqueued
|
||
scan units (so the second ran unlocked).
|
||
|
||
**Model chosen: tokenless atomic flag + documented single-owner release discipline.** The six bools
|
||
became `int`s guarded by `Interlocked.CompareExchange` (0/1), so `Lock*`'s `bool` return is now a
|
||
reliable "this call won the transition" signal and the change event fires exactly once per
|
||
transition. The contract (XML-doc'd on `IEntityLocker`): a `true` from `Lock*` confers ownership of
|
||
exactly one release — performed either in the acquiring scope (`finally`, gated on the captured
|
||
bool) or by the single designated releaser the acquirer hands off to (the consumer of the message
|
||
enqueued while holding the lock, with a compensating unlock if the enqueue throws — the established
|
||
`TraktController.EnqueueWithTraktLock` / scheduler `unlock: last` tail pattern). Callers must never
|
||
release a lock they did not acquire. `Unlock*` on an already-unlocked slot returns `false`, fires no
|
||
event, and logs a Warning — the loud tripwire for double-release bugs; it does not throw or
|
||
`Debug.Assert`, because an advisory flag must stay safe to release in `finally` paths. The three
|
||
`ConcurrentDictionary`-backed kinds (Library/Playout/RemoteMediaSource) were already atomic
|
||
(`TryAdd`/`TryRemove`) and kept their semantics (the redundant `ContainsKey` pre-checks were dropped
|
||
as tidy-up); the interface signature is unchanged across its ~40 call sites.
|
||
|
||
**Rejected:** owner tokens/leases — the acquirer and releaser for Library/Trakt/Plex/Collections
|
||
locks are different code correlated only by entity id across in-memory `Channel<T>` queues, so a
|
||
token would have to travel inside ~8 background-request message types for a defect that discipline
|
||
plus the now-trustworthy atomic return value already prevents. Counted/reentrant locks — wrong
|
||
semantics: these are exclusive in-progress flags; two holders is the failure mode, not a feature.
|
||
Call-site fixes this model prescribes land separately: #232 (scan lifecycle — enqueue only after a
|
||
successful lock, compensating unlock on enqueue failure, one release per acquisition in batches) and
|
||
#234 (BuildPlayout/subtitles gate their `finally` unlock on the captured acquire result).
|
||
|
||
## 2026-07-11 — Channels screen extraction (#244): single-file screen, no sibling helper dir (epic #243 phase 1)
|
||
|
||
First bounded extraction under the App.tsx modularization epic (#243): the Channels domain moved
|
||
verbatim out of `web/src/App.tsx` into `web/src/screens/ChannelsScreen.tsx` (zero-prop, self-sufficient,
|
||
mirroring the SchedulesScreen extraction), with its behavior tests moved to a colocated
|
||
`ChannelsScreen.test.tsx` that owns its own scoped fetch mock (spa-conventions §6). Pure structural
|
||
move: no API/route/CSS/visual change; `App.tsx` retains only the import + the `<ChannelsScreen />`
|
||
dispatch clause.
|
||
|
||
**Decision — no `web/src/channels/` sibling helper directory** (unlike Schedules' `web/src/schedules/`).
|
||
Channels' pure logic (`stateByChannelId`, `sortedChannels`, `groupedChannels`, `progressFromChannelState`,
|
||
`formatChannelNumber`) totals ~30 lines with no independent business-rule layer comparable to Schedules'
|
||
`itemRules.ts` (~470 lines, separately unit-tested). Keeping it inside the single screen file matches the
|
||
Blocks/Decos/Templates precedent. Revisit only if a later #243 phase adds substantial pure Channels logic
|
||
worth isolating.
|
||
|
||
**One cross-domain helper inlined, not shared:** the Dashboard-owned `progressFromNowPlaying` (still in
|
||
`App.tsx`, used by `OnAirCard`) was structurally reused by the Channels `progressFromChannelState`. Rather
|
||
than export it from App or create a shared module, its ~5-line start/finish/now percentage math was inlined
|
||
into the moved `progressFromChannelState` (behavior-identical — `ChannelState.nowPlaying` carries the same
|
||
`startUtc`/`finishUtc` shape), so `ChannelsScreen.tsx` has no import back into `App.tsx`.
|
||
|
||
**#238 (TopBar `primaryAction` dead button) left untouched** — the `channels` route's inert `ctv:primary-action`
|
||
dispatch is #238's owned bug and out of scope for a behavior-preserving extraction; the shell/action redesign
|
||
is deferred to #247 (epic phase 4).
|
||
|
||
## 2026-07-11 — Media-source management REST write API + SPA (#202)
|
||
|
||
Replaced the Blazor `/media/sources/{local,plex,jellyfin,emby}/...` pages (14 routes) with SPA
|
||
screens under `/app/libraries/*` over new write controllers (`LocalLibrariesController`,
|
||
`Plex|Jellyfin|EmbyMediaSourcesController`), wrapping existing MediatR commands 1:1 (no new
|
||
commands, no DB migration). Full design + adversarial-review reconciliation:
|
||
`docs/handoffs/` session record and issue #202. The design surfaced and fixed several pre-existing
|
||
Application/Infrastructure bugs newly reachable from a programmatic client; each is recorded here
|
||
because it changes documented behavior, not just adds a route.
|
||
|
||
**Secure `apiKey` contract (Jellyfin/Emby connection).** The connection GET
|
||
(`RemoteConnectionResponseModel`) returns `{ address, hasApiKey }` — the stored key **never**
|
||
leaves the server, closing a leak where the old design would have served the raw key from an
|
||
unauthenticated GET under any-origin CORS. On the connection PUT, a blank/omitted `apiKey` means
|
||
*retain the existing key*; a non-blank value sets a new one; the key is **required on first
|
||
connect** (no existing secret) → 422. Rationale: GETs aren't behind `X-Api-Key`
|
||
(`ApiKeyAuthorizationFilter` only guards mutating verbs), so a secret-bearing GET is a real
|
||
exposure regardless of how obscure the route is. **Stated explicitly as an input to #197** (the
|
||
planned read-side-auth review for secret-bearing GETs) — #197 should treat "does any GET return a
|
||
credential" as one of its checks, not just this one instance.
|
||
|
||
**Three list-replace identity contracts, not one uniform one.** An earlier draft assumed a single
|
||
"`Id<1`=add / missing=delete / id-preserved" contract across all three PUT-replace families; source
|
||
inspection proved that false for two of them:
|
||
- *Remote library sync preferences* (`PUT .../{id}/libraries`) — the command carries no source id
|
||
and the handler toggles only the ids present in the body; a row **absent** from the request is
|
||
left untouched, not deleted (libraries are sync-discovered, never created via this PUT, so there
|
||
are no `Id=0` adds either). The controller validates the submitted id set against
|
||
`Get{Family}LibrariesBySourceId(id)` (422 on any id not owned by the route's source — closes a
|
||
cross-source hole). Identity is **not stable across a disable**: `Disable{Family}LibrarySync`
|
||
removes and re-adds the row with a fresh id, so the SPA keys its draft to `(name, mediaKind)`,
|
||
never to `Id`, and refetches after every save (the PUT returns the reloaded list).
|
||
- *Path replacements* (`PUT .../{id}/path-replacements`) — id-based (existing `Id`=update,
|
||
`Id<1`=add, absent=delete) as documented, **but** the repo UPDATE SQL had no source-id predicate
|
||
(`WHERE Id = @id`, no `AND {Family}MediaSourceId = @id`), so a PUT to source A could silently
|
||
overwrite source B's row with the same numeric id. Fixed with a handler-level ownership guard
|
||
(reject any incoming positive id not in this source's current set → 422, no partial mutation)
|
||
**and** the repo SQL predicate itself (defense-in-depth for any other caller of that repo method).
|
||
- *Local library paths* (`PUT /api/libraries/local/{id}`) — identity is the **normalized path
|
||
string** (full path, trailing-separator/case-insensitive), not `Id`; `Id` in the request is
|
||
advisory. Renaming a path is delete-old+add-new under the hood (its `LibraryPath.Id` changes).
|
||
Kept as-is (matches the entrenched, tested Blazor behavior and how the SPA edits by value); not
|
||
rewritten to id-based identity, which would be a bigger, riskier change out of #202's scope.
|
||
|
||
**Plex pin-flow as REST: poll until the lock releases, exception-safe non-handoff unlock.** The
|
||
SPA polls `GET /api/media-sources/plex` rather than a per-pin status resource (no pin-addressable
|
||
server state exists to expose; SSE/push was already rejected, 2026-07-09). Polling contract is
|
||
`isLocked && !isAuthorized` = waiting on the user; `isLocked && isAuthorized` = finalizing
|
||
(discovering servers — **do not** stop here, the server list is still empty);
|
||
`!isLocked && isAuthorized` = success; `!isLocked && !isAuthorized` = timed out/abandoned. This
|
||
required fixing a **latent lock-leak bug**: `TryCompletePlexPinFlowHandler` threw
|
||
`OperationCanceledException` on its 2-minute timeout instead of returning `false`, and nothing
|
||
unlocked on that path — an abandoned sign-in wedged the Plex lock until restart or manual sign-out.
|
||
Fix releases `UnlockPlex()` on the timeout-throw, a poll-exception, and an enqueue-exception — but
|
||
**deliberately not** in an unconditional `finally`: on success the lock is handed off to
|
||
`SynchronizePlexMediaSources`, the sole releaser after server discovery; a blanket `finally` would
|
||
double-release and release *before* discovery completes, re-opening the same race the fix closes.
|
||
This is the same non-owner-token discipline as the #231 `EntityLocker` model (2026-07-11 entry
|
||
above), applied to the pin-flow's handoff-vs-terminal distinction specifically.
|
||
|
||
**404 comes from the controller pre-check, not the handler.** `Apply`/`ToEitherAsync` both
|
||
`.Join()` errors, which flattens any `NotFoundError` inside a joined `Validation` down to a plain
|
||
422. So every id-taking endpoint's real 404 is a controller-side pre-check
|
||
(`Get...ById(id)`-is-`None` → `ApiResults.NotFoundProblem`, the `TemplateController.DeleteGroup`
|
||
pattern), not a handler-level conversion — converting the joined validators to `NotFoundError`
|
||
would be dead code, since the join discards the distinction anyway. This is check-then-act (a
|
||
delete racing between the pre-check and the command falls through to the handler's own 422, not a
|
||
404); accepted and tested against the actual runtime error type rather than a hoped-for handler 404.
|
||
|
||
**"Scan All" dropped, not implemented.** The disabled SPA header button on the libraries hub was
|
||
speculative UI with **no Blazor equivalent** (`Libraries.razor` only ever supported per-library
|
||
scan). Removed rather than backed with a new bulk-scan endpoint; per-library scan (#232) and the
|
||
new per-source refresh-libraries endpoints (P8/J9/E9) cover the real capability set.
|
||
|
||
**App-owned popstate for guarded sub-path routes.** `LocalLibraryEditScreen` and the other
|
||
`/app/libraries/*` editors are the first screens to both register a dirty-navigation guard *and*
|
||
track their own sub-path pathname — the combination `spa-conventions.md` §8 had flagged as
|
||
unvalidated. React commits child passive effects before parent ones, so a sub-path wrapper that
|
||
self-registers `popstate` would fire (and switch sub-screen) *before* App's guard-restore listener
|
||
could veto. Resolution: App owns `popstate` centrally for the `libraries` route and only pushes an
|
||
approved sub-path down to the wrapper (which no longer self-registers `popstate`); on a vetoed pop
|
||
App re-pushes the pre-pop URL and the wrapper never sees the rejected path. This is scoped to the
|
||
`libraries` route only (gated on `activeRoute === 'libraries'`) so unguarded sub-path routes
|
||
(Playouts, Media) stay byte-identical. See `spa-conventions.md` §2/§8 for the updated exemplar
|
||
list and the resolved caveat text.
|
||
|
||
## 2026-07-11 — Legacy→SPA redirect matcher: exact map + ordered segment-template patterns (#204)
|
||
|
||
`LegacyUiRedirects.TryGetRedirect` grew from a single exact-path dictionary to a **two-tier matcher**
|
||
behind the unchanged `(PathString, out string)` signature. **Tier 1** is the existing
|
||
`OrdinalIgnoreCase` `Map` (now 52 entries — the parameterless (A)/(B) routes plus the (E-base) browse
|
||
roots whose *targets* carry `?kind=…`). **Tier 2** is an ordered `IReadOnlyList<PatternRule>` of 36
|
||
segment-template rules ((C)/(C2)/(D)/(E-page)), consulted only on a Tier-1 miss; declaration order is
|
||
match order (first-match-wins).
|
||
|
||
Template tokens are minimal: `{id}` matches a **strict positive integer**
|
||
(`int.TryParse(seg, NumberStyles.None, InvariantCulture, out id) && id > 0` — rejects signs,
|
||
whitespace, separators, `0`, negatives, and overflow like `999999999999`; the raw segment text,
|
||
e.g. `007`, is substituted, not re-formatted); `{any}` matches any non-empty segment and is dropped
|
||
(only `/playouts/add/{any}`); everything else is a literal compared `OrdinalIgnoreCase`. The request
|
||
path is split with `StringSplitOptions.None` and **empty segments are rejected** (load-bearing: so
|
||
`/channels//5` cannot match `/channels/{id}`); templates themselves use `RemoveEmptyEntries`. The
|
||
existing single-trailing-slash normalization runs before both tiers, so `/channels/5/` matches.
|
||
|
||
The set is **collision-free by construction** — exact-before-pattern plus strict numeric `{id}` means
|
||
no two tiers/rules can match the same path. **Guard invariant** (comment + `Map`-keys meta-test): no
|
||
Tier-1 key or Tier-2 template may begin with `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`,
|
||
or `/media/sources`; rules are always full, specific templates — **never prefix wildcards** (a bare
|
||
`/media/{any}` rule is forbidden). The blazor branch does not prefix-guard `/api|/artwork|/docs|
|
||
/openapi`, so the matcher's specificity is part of their protection.
|
||
|
||
**Query-string merge**: the incoming request query is now merged into the target via a new public
|
||
`AppendQueryString(target, QueryString)` helper (one-line Startup change:
|
||
`context.Request.PathBase + LegacyUiRedirects.AppendQueryString(target, context.Request.QueryString)`).
|
||
A target that already carries `?` (the `?kind=…` browse roots) is `&`-joined instead of producing a
|
||
malformed double `?`; plain targets keep verbatim-append behavior byte-for-byte. A duplicated key
|
||
after a merge (`?kind=movies` + incoming `?kind=shows`) is first-wins in the SPA
|
||
(`URLSearchParams.get` returns the first value) — acceptable. Extracting the merge into
|
||
`LegacyUiRedirects` keeps it unit-testable without a TestServer while preserving the PathBase
|
||
re-application invariant the Startup source-text test protects.
|
||
|
||
**Rejected**: regex pairs (harder to audit for the `/api`/`/artwork` greediness invariant, noisier
|
||
tests, no benefit — every parameterized route here is "fixed segments + one variable segment");
|
||
ASP.NET `TemplateMatcher`/`RouteMatcher` (pulls routing machinery into a static helper for 36 rules);
|
||
a single unified rule list (loses the O(1) dictionary hit for the ~52 exact routes that dominate real
|
||
traffic). No `/api` change, no OpenAPI regen, no SPA change.
|
||
|
||
## 2026-07-11 — Blazor removal auth posture: no new exposure beyond phase (a); real auth deferred to #197 (#206)
|
||
|
||
Sign-off for the #91 phase (b) removal-gate item #206 ("deleting the last challenged Blazor page leaves
|
||
only the open SPA"). The actual authorization wiring in `ErsatzTV/Startup.cs` + `ErsatzTV/Pages` was
|
||
enumerated in code (not assumed) before clearing the gate.
|
||
|
||
**What is gated today**
|
||
- **OIDC** (`OidcHelper.IsEnabled` — active only when `Authority`/`ClientId`/`ClientSecret` are configured):
|
||
`AddAuthentication` (cookie default, `oidc` challenge) + `AddAuthorization` `DefaultPolicy =
|
||
RequireAuthenticatedUser` + **`AddRazorPages(… AuthorizeFolder("/"))`** (Startup.cs:379-385) +
|
||
`blazor.UseAuthentication()/UseAuthorization()` inside the Blazor `MapWhen` branch (Startup.cs:764-770).
|
||
`AuthorizeFolder("/")` gates **Razor Pages only**, and the sole user-facing Razor Page is
|
||
`Pages/_Host.cshtml` — the Blazor Server host (the other `.cshtml`, `Shared/_Favicons.cshtml`, is a
|
||
cosmetic partial). **So the OIDC challenge protects exactly the Blazor UI and nothing else.**
|
||
- **`/app` (SPA)** is served by its own `MapWhen(path=/app)` static-file branch (Startup.cs:701-714) with
|
||
**no authentication/authorization middleware** — open since phase (a) (`/`→`/app`, PR #148).
|
||
- **`/api/*` controllers** carry no `[Authorize]` (verified: zero attributes in `Controllers/`); the
|
||
Razor-Pages `AuthorizeFolder`/`DefaultPolicy` never reach them. Their only optional gate is the
|
||
per-endpoint `ApiKeyAuthorizationFilter` (API-key on mutating JSON endpoints), independent of OIDC/Blazor.
|
||
- **`/iptv/*`** is gated by `ConditionalIptvAuthorizeFilter` (JWT `JwtOnlyScheme`, active only when
|
||
`JwtHelper.IsEnabled`) in its own `MapWhen` branch (Startup.cs:797-803) — independent of Blazor.
|
||
|
||
**Posture after Blazor removal.** Removing `Pages/_Host.cshtml`, `AddRazorPages`/`AuthorizeFolder("/")`,
|
||
`blazor.UseAuthentication/UseAuthorization`, `MapBlazorHub`, and `MapFallbackToPage("/_Host")` deletes the
|
||
OIDC challenge's **only attachment point** — no user-facing surface remains challenged. **No capability is
|
||
lost:** every Blazor-served capability already has an open SPA equivalent (the #91 parity effort), and the
|
||
SPA was already the unauthenticated path since phase (a), so removal exposes nothing a user could not already
|
||
reach via `/app`.
|
||
|
||
**The one honest caveat (not a regression introduced by removal):** an OIDC-configured operator's *Blazor*
|
||
admin UI sits behind a login today; after removal there is no login-gated admin UI at all (the SPA admin UI
|
||
is open). That exposure delta already happened at **phase (a)** (the open SPA became the default admin
|
||
surface); removal only deletes the now-redundant challenged duplicate. Designing real SPA/API authentication
|
||
is deliberately deferred to **#197** (cold API security review — a HARD GATE before any remote exposure).
|
||
|
||
**Removal-PR must-not-break (independent gates that survive):** `ConditionalIptvAuthorizeFilter` (`/iptv/*`
|
||
JWT), `ApiKeyAuthorizationFilter` (mutating `/api/*`), and `JwtHelper` access_token query support. **Leave
|
||
the OIDC service registrations in place** (conditional on config, inert once no Razor Page consumes them) —
|
||
ripping OIDC out is a #197 decision, not a removal-PR one. The removal PR removes only the Blazor-attached
|
||
pieces above; `MapControllers()` + `/docs` (Scalar), currently co-hosted in the Blazor `MapWhen` branch, must
|
||
survive the surgical reduction.
|
||
|
||
## 2026-07-11 — Pre-removal Blazor rollback tag `blazor-final` (#205)
|
||
|
||
Removal-gate item #205: the removal PR deletes both the Blazor reference implementation and the
|
||
`/system/health` escape hatch, so a post-deletion parity gap would otherwise be an archaeology exercise
|
||
(guessing which release tag still matches `main` minus Blazor). Decision + procedure, to run as the **first
|
||
action of the Step 2 deletion PR merge** (not before — `main` moves until then):
|
||
|
||
1. On the `main` commit **immediately preceding** the removal merge (the last commit that still contains
|
||
`ErsatzTV/Pages/**`), cut an annotated tag and push it:
|
||
`git tag -a blazor-final -m "Last commit with the legacy Blazor Server UI (pre-#91-phase-b removal)"`
|
||
then `git push origin blazor-final`. The tag name is **`blazor-final`** (not `v*`) so it does **not**
|
||
trigger the `v*` prod-release build in `.gitea/workflows/docker-build.yml`.
|
||
2. **Restore path** (if a gap surfaces post-removal): `git checkout blazor-final` → `docker build -f
|
||
docker/Dockerfile -t ersatztv:blazor-final .` → pin the **test** container to that image while the gap is
|
||
fixed forward on `main`. Alternatively `git revert` the single deletion merge commit (keep the deletion as
|
||
one squash/merge commit specifically to make this a one-liner).
|
||
3. Document the tag + restore path in the removal PR body; update this entry with the tag's commit sha when
|
||
cut.
|
||
|
||
Not cut this session — `main` still carries Blazor and will advance before the removal PR.
|
||
|
||
## 2026-07-11 — Async-op API contract normalization + playout build observability + F9 scan endpoints (#235)
|
||
|
||
Reviewer#20 F7/F8/F9. Normalizes the queue-triggering `/api/*` endpoints onto one contract, closes the two
|
||
F9 `Libraries.razor` parity gaps, and hardens the Trakt batch-lock lifecycle. Much of the F8 surface was
|
||
**already normalized** by #232 (library scan → `QueueLibraryScanResult` 202/404/409/422) and #215 (per-id
|
||
playout mutations + reset → 409 lock guard) — this issue finished the remaining outliers.
|
||
|
||
**Normalized async-op contract** (queue-triggering endpoints): **202 Accepted** = work queued; **404
|
||
ProblemDetails** = entity missing (controller pre-check); **409 ProblemDetails** = lock held (the running
|
||
job, or a mutation racing it — §3a/§3b); **422 ProblemDetails** = domain precondition (sync disabled /
|
||
unsupported / start failed). Trakt was the reference implementation. Changes made:
|
||
- `MaintenanceController.EmptyTrash` — error path **500 text/plain → 404/422 ProblemDetails** (`ToErrorResult`).
|
||
- `MaintenanceController.CleanArtwork` — silent **200 → 202** (fire-and-forget enqueue). No SPA consumer.
|
||
- `LibrariesController.ScanShow` — conflated **400 `{error}` → 202/404/409/422** via a new
|
||
`QueueShowScanResult` enum (6 outcomes incl. an honest `ScanFailed`→422, distinct from `Unsupported`).
|
||
- `ChannelController.ResetPlayout` — **200 → 202** (queue-triggering; 404/409 unchanged).
|
||
- `PlayoutController.ResetAll` — **202 (no body) → 202 + `ResetAllPlayoutsResponseModel`** reporting
|
||
`queuedPlayoutIds` / `skippedLocked` / `skippedUnsupported` (replaces the silent skip; still 202, still
|
||
skips locked/ExternalJson by design per §3a — now it *reports* what it skipped).
|
||
- `TroubleshootController.TroubleshootPlayback` — bare body-less `NotFound()` → **404/422 ProblemDetails**
|
||
with distinguishing detail. **Status codes the SPA HLS player depends on were preserved** — verified
|
||
`HlsPlayer.tsx` never branches on this endpoint's status (playback state comes from the separate
|
||
`/api/troubleshoot/playback/status` poll); only the error *body* was enriched.
|
||
|
||
**Playout build observability**: the list endpoint (`GET /api/playouts`) already stamped `isLocked` +
|
||
`BuildStatus` on `PlayoutListItemResponseModel` (#215); this issue adds **`isLocked` to the single-playout
|
||
`GET /api/playouts/{id}`** (`PlayoutResponseModel`), so the detail poll surface carries the §3a lock flag
|
||
too. No dedicated `GET /api/playouts/{id}/status` push channel was added — the flag on the existing GETs is
|
||
the HTTP-observable substitute for Blazor's live lock event, matching the `GET /api/trakt/status` precedent.
|
||
|
||
**F9 parity endpoints** (the `Libraries.razor` deletion gate — #202 did NOT close these):
|
||
- **Deep scan**: `POST /api/libraries/{id}/scan` gains `?deep=false`, threaded through
|
||
`QueueLibraryScanByLibraryId(LibraryId, DeepScan=false)` into `ForceSynchronize{Plex,Jellyfin,Emby}LibraryById(id, deep)`
|
||
(was hardcoded `false`). Non-breaking: existing callers omit it.
|
||
- **External-collections scan**: new `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=false`
|
||
on the three #202 media-source controllers, dispatching `Synchronize{X}Collections(id, ForceScan:true, deep)`.
|
||
Each pre-checks source existence (404), acquires the per-source **collections** lock (`Lock{X}Collections()` —
|
||
the lock *is* the running scan, so a false = **409**), then enqueues and returns 202; the controller
|
||
compensating-unlocks in a `catch` if the enqueue throws (§3b), and `ScannerService` releases in its `finally`.
|
||
Thin SPA clients shipped (`scanLibrary(id, deep)`, `scanCollections`); **the SPA deep-scan / collections
|
||
buttons are the removal PR's remaining parity work** (parity doc §5).
|
||
|
||
**F7 Trakt batch-lock leak fix**: the global Trakt lock was released only when the *terminal* batch message
|
||
(`Unlock: true`) was processed; a `WorkerService` shutdown/cancellation before that message leaked the lock
|
||
permanently (subsequent Trakt ops 409 until restart — same class as #231/#233/#234). Fix: `WorkerService`
|
||
now releases the Trakt lock in a `finally` on read-loop exit if still held. Non-vacuous regression test proven
|
||
against an inverted-condition control.
|
||
|
||
**Accepted-by-design** (per the issue's decision-record ask): the worker's channels are **unbounded** and
|
||
there is **no shutdown drain** — messages still queued at process exit are dropped. This is acceptable because
|
||
the entity locks are **in-memory singletons that die with the process**, so a dropped message can't strand a
|
||
lock across restarts (the F7 `finally` covers the *within-process* shutdown-break leak, which is the only way
|
||
a lock outlives its batch while the process keeps running). Adding a bounded-channel backpressure / graceful
|
||
drain is out of scope and would not fix a correctness bug.
|
||
## 2026-07-11 — Optimistic-concurrency contract for replace-all PUTs (#253 PR1: infra + Block reference)
|
||
|
||
Replace-all aggregate PUTs had **no** optimistic concurrency — a stale second tab silently overwrote a
|
||
fresher edit (200, no signal) across ~10 aggregate surfaces. PR1 lands the shared contract on the Block
|
||
reference aggregate; PRs 2–4 fan it out. The full ratified design + independent-review hardening is
|
||
[#253#issuecomment-8472](http://192.168.1.95:3000/timothy/ersatztv/issues/253#issuecomment-8472);
|
||
the mechanics live in `api-conventions.md` §7a. Decisions frozen here:
|
||
|
||
- **Token = uniform plain `int Version`** on each root implementing `IVersionedAggregate`, EF-mapped
|
||
`.IsConcurrencyToken()`, one dual-provider migration (`AddAggregateVersions`, `defaultValue: 0`). **Not**
|
||
a reused `DateUpdated` (tick-collision, SQLite TEXT precision, couples UI cosmetics to correctness) and
|
||
**not** a MySQL-native rowversion (portability over provider-native).
|
||
- **412 Precondition Failed**, not 409 — 409 stays the §3a EntityLocker "build in progress" guard;
|
||
distinct codes → distinct SPA UX. New `PreconditionFailedError : BaseError` → 412 in `ApiResults.ToErrorResult`.
|
||
- **Pre-check AND EF token both required.** The handler pre-check (a standalone `Either` introduced AFTER
|
||
the validation pipeline — never via `Apply`, which `Join()`-flattens the subtype to 422) gives a clean
|
||
412; the unconditional `root.Version++` + `IsConcurrencyToken` UPDATE-guard + a `SaveChangesWithConcurrencyGuard`
|
||
backstop closes the residual load→save TOCTOU (`DbUpdateConcurrencyException` → 412).
|
||
- **Unconditional bump** (not "only when a child changed"): EF writes the root row only when a scalar
|
||
differs, so a no-op PUT-back must still bump to fire the token and rotate every other client's ETag.
|
||
- **Config-only aggregate boundary**: every mutating handler of a root's *editor-visible config state*
|
||
bumps `Version` (incl. bulk `ExecuteUpdate/Delete` writers via `.SetProperty`); regenerated build output
|
||
(playout items/history) is outside the token — neither bumped nor guarded.
|
||
- **Header-only ETag**, strong tag of the decimal `Version`; parsed/emitted by `ConcurrencyHeaders`. The
|
||
successful PUT returns the new ETag (else a same-tab second save 412s against its own write).
|
||
- **Phasing**: Phase 1 (this arc) = a missing `If-Match` force-writes (zero breakage) while the SPA starts
|
||
echoing; Phase 2 (a later PR) flips missing → **428** after every editor echoes and one release soaks.
|
||
`If-Match: *` stays the scripted force-write escape hatch.
|
||
- **Child stable-identity is OUT of #253** (the "moved fill-group item inherits the wrong slot's state"
|
||
concern on the positional reconcile) — root-anchored versioning is orthogonal to it; split to **#259**.
|
||
- **If-Match status semantics** (non-canonical/weak/list → 400) are fail-safe; the stricter RFC 7232
|
||
"valid-but-non-matching → 412" refinement is deferred to #197 (**#265**).
|
||
|
||
## 2026-07-11 — Post-commit side effects run on `CancellationToken.None` (generalized from #251 to #254)
|
||
|
||
Audit #22 (adversarial-reviewer) found ~20 command handlers threading the request `cancellationToken`
|
||
into work that runs **after** `SaveChangesAsync` commits — the post-commit `WriteAsync` enqueue that
|
||
rebuilds/refreshes the affected entity, `mediator.Publish`, search-index reindex, cache refresh. A late
|
||
HTTP-client disconnect cancels that token, so the *already-committed* mutation throws on the way out
|
||
**and silently drops its side effect** (the playout rebuild is never queued → the persisted edit never
|
||
takes visible effect until a manual Reset). #251 fixed this for the deco handlers; #254 generalizes the
|
||
policy across the codebase.
|
||
|
||
**Decision.** Once a mutation has committed, the *entire* compensating side effect — enqueues, publishes,
|
||
reindexes, cache refreshes, and any post-commit lookup that **gates** one of those enqueues — runs on
|
||
`CancellationToken.None`. The commit is the point of no return; past it the side effect must not be
|
||
half-abortable. Full convention + the two boundaries in `docs/api-conventions.md` §7b.
|
||
|
||
**Scope of the #254 sweep (this PR).** Swept the single-`SaveChanges` handlers under `MediaCollections/`,
|
||
`ProgramSchedules/`, `Playouts/`, `Channels/` (20 handlers). Deliberately **excluded**:
|
||
- **`BuildPlayoutHandler`** — a background/worker handler; its token is the worker shutdown token, not a
|
||
client-disconnect token, so its downstream enqueues *correctly* honor cancellation.
|
||
- **`UpdateFFmpegSettingsHandler` + the two `Configuration/` settings handlers** — they commit via several
|
||
sequential `IConfigElementRepository.Upsert` calls with an interleaved enqueue; "when is it committed"
|
||
is a partial-commit-under-cancellation question broader than the clean single-`SaveChanges` F4 pattern.
|
||
Left for a separate follow-up.
|
||
- **Response-projection reloads** (`ReplaceProgramScheduleItemsHandler` / `AddProgramScheduleItemHandler`
|
||
post-commit graph reload that builds the *returned* view model) keep the request token — a cancelled
|
||
response after a durable commit + `None`-enqueue loses nothing.
|
||
- Handlers a no-token `WriteAsync()` already makes behaviorally correct (`default` == `None`) were left
|
||
alone (explicit-`None` there is cosmetic).
|
||
|
||
Also folded in the two other #254 items on the same handlers: the channel-guide `{number}.xml` delete in
|
||
`DeleteChannelHandler`/`DeletePlayoutHandler` now routes through `IFileSystem.File.Delete` (observable
|
||
under `MockFileSystem`) **before** the commit (a post-commit delete orphans the xml on a crash; the guide
|
||
xml is regenerable on demand, so a pre-commit delete is the safe ordering), and
|
||
`ReplacePlayoutAlternateScheduleItemsHandler` now rejects an empty item list in the handler (not only at
|
||
the controller pre-guard) so a direct caller can't trip the `Max()`-on-empty crash.
|
||
|
||
**Coordination note for #253 PR2–PR4.** Those PRs add `Version++` (pre-commit) to the mutating handlers of
|
||
the versioned aggregates — several of which this sweep also touched (post-commit token, a different line
|
||
region). Low git-conflict risk, but merge `main` in and expect to see the `CancellationToken.None`
|
||
convention already present on the post-commit enqueues.
|
||
|
||
## 2026-07-11 — #253 PR3: Diff + Scalar concurrency fan-out (Collection / Playout×2 / MultiCollection / RerunCollection)
|
||
|
||
**Context.** PR3 of the #253 optimistic-concurrency arc fans the frozen Block recipe (api-conventions §7a)
|
||
across the five Diff/Scalar aggregates. Three judgment calls beyond the mechanical copy:
|
||
|
||
**H1 — Playout `catch(Exception)`→422.** The two Playout replace handlers wrap `SaveChangesAsync` in a
|
||
`catch(Exception)` that maps any exception to a bare `BaseError` (→ 422). Rather than let the guard's
|
||
concurrency failure be reshaped into a 422, the guarded save (`SaveChangesWithConcurrencyGuard`) returns a
|
||
`PreconditionFailedError` **Left as a value** and the handler returns it before the post-commit block —
|
||
so it never reaches the catch. Proven by the pre-check-subtype tests (a `.Apply` flatten would fail
|
||
`ShouldBeOfType<PreconditionFailedError>`) plus a non-vacuous Playout racing-save test.
|
||
|
||
**M2 — the `SaveChangesAsync() > 0` gates.** RerunCollection and Collection-custom-order run their
|
||
playout-refresh **unconditionally** on a successful save (the unconditional `Version++` makes the old gate
|
||
always-true; the "nothing changed" branch is dead). MultiCollection is the exception: it saved the name
|
||
first specifically so a name-only change wouldn't rebuild playouts, so we bump `Version` on that **first**
|
||
save and leave the **second** (items) save's `> 0` gate intact — a name-only edit still bumps + rotates the
|
||
ETag but does not rebuild. Enumerating every behavior the gate provided before reworking it (the #232 lesson).
|
||
|
||
**Sibling-writer scope (deferred).** §7a's config-only boundary says every writer of an aggregate's
|
||
editor-visible config bumps `Version`. PR3 ships the five primary endpoints' full contract + the one
|
||
design-named bulk writer (`UpdateDefaultDecoHandler`, safe via `.SetProperty`). It **defers** the other
|
||
same-root non-bulk config writers (`UpdateCollectionHandler`, `RemoveItemsFromCollectionHandler`,
|
||
`UpdatePlayoutHandler`, the `ScheduleFile` handlers) and the repository-mediated `Add*ToCollection` family.
|
||
Rationale: the primary endpoints' own bump+guard fully cover the two-tab lost-update the issue targets;
|
||
the deferred writers only affect cross-editor ETag *rotation*, and adding an unconditional bump to a handler
|
||
that uses plain `SaveChangesAsync` (not the guard) converts a latent lost-update into a **new 500**
|
||
(`DbUpdateConcurrencyException`) — doing it safely needs a uniform guard+bump+412 pass of its own, better
|
||
done with the #197 contract work. Tracked as a follow-up issue.
|
||
|
||
**VMs.** `Playout.Version` surfaces via `PlayoutNameViewModel` (required arg); the three collection VMs
|
||
(`MediaCollectionViewModel`, `MultiCollectionViewModel`, `RerunCollectionViewModel`) carry `int Version = 0`
|
||
(defaulted — 0 for the selection-placeholder constructions, real value from the Mapper projection).
|
||
Header-only via ETag, never echoed in a response body (the Block precedent).
|
||
|
||
**Post-merge addendum (PR3 review, #269).** Activating the `Version` token means EF guards *every* root
|
||
UPDATE, so non-participating root-scalar writers that use plain `SaveChangesAsync` (playout settings /
|
||
schedule-file / on-demand-checkpoint, collection name) would 500 on a concurrent bump. The realistic
|
||
UPDATE writers were fixed in-PR with `ConcurrencyExtensions.SaveChangesForcingVersion` (Phase-1
|
||
force-write on conflict: adopt the stored token, retry, never revert the concurrent bump). The deferral
|
||
above is re-scoped to the DELETE handlers + repository `Add*` writers only (→ #269).
|
||
|
||
## 2026-07-12 — External-collections scans get an authoritative status surface (#271); the SPA timeout is retired
|
||
|
||
**External Collections rows stay client-derived.** The SPA derives them from `GET
|
||
/api/v1/media-sources`: remote sources expose only sync-enabled libraries, so a non-empty `libraries` list is
|
||
equivalent to Blazor's `Libraries.Any(ShouldSyncItems)` filter. No second listing endpoint or fetch is needed.
|
||
|
||
The first scan-button implementation used a bounded optimistic timeout because collections locks had no HTTP
|
||
mirror. #271 replaces that temporary ceiling with the authoritative status contract below.
|
||
|
||
**Decision 1 — one family-global status endpoint, reading `IEntityLocker`.** New
|
||
`GET /api/v1/media-sources/collections-scan-status` (`MediaSourcesController` → `GetCollectionsScanStatus`
|
||
handler) returns one `CollectionsScanStatusResponseModel { family }` entry per media-source family
|
||
(`plex`/`jellyfin`/`emby`) whose collections lock is currently held, and only for active ones — the direct
|
||
counterpart to `GET /api/v1/libraries/scan-status`. It reads `IEntityLocker.Are{X}CollectionsLocked()` (the lock
|
||
*is* the running scan — the scan-collections controllers acquire it before enqueueing and the scanner releases
|
||
it on completion), analogous to how the library endpoint reads `IScannerProxyService.GetActiveScans()`. Two
|
||
deliberate shape differences from libraries: (a) **family-global, not per-source** — the collections lock takes
|
||
no source id (`LockPlexCollections()`), so an entry means *every* source of that family is scanning, matching
|
||
Blazor's all-rows-disabled behavior (per the PR #272 review note); (b) **no percent** — collections scans
|
||
expose only a boolean lock, not progress.
|
||
|
||
**Decision 2 — the SPA reconciles authoritatively; the fixed timeout is removed.** `useCollectionsScan` now
|
||
polls the new endpoint (seeding on mount, so a scan already running when the screen opens disables the buttons
|
||
immediately — the old timeout couldn't) and reconciles optimistic pending against the active-family set using
|
||
the **same `pruneGraceExpiredPending` grace-tick helper** the library hook uses (now generic over the pending
|
||
key type). A row shows "Scanning" when its family is in the active set **or** it has a still-in-grace optimistic
|
||
pending key. The grace window is kept (not the old wholesale timeout) to absorb the click→observed-active lag and
|
||
the fast-scan-between-polls race — the same bounded-pending discipline #232/#230 established for library scans.
|
||
`COLLECTIONS_PENDING_TIMEOUT_MS` is gone.
|
||
|
||
## 2026-07-11 — Blazor Server UI removed (#91 phase b)
|
||
|
||
The #91 phase (b) removal PR deletes the legacy Blazor Server UI now that the ChicoryTV SPA has parity
|
||
(all gates cleared: #145, #151/#152/#153/#155, #202, #207, #212/#213, #235 F9). The SPA is the only UI.
|
||
|
||
**Deleted.** `ErsatzTV/Pages/**` (all `.razor`, incl. `_Host.cshtml`, `FragmentNavigationBase.cs`,
|
||
`MultiSelectBase.cs`), `ErsatzTV/Shared/**` (all `.razor` + `_Favicons.cshtml`), `ErsatzTV/ViewModels/**`
|
||
(39 Blazor edit-form VMs), `ErsatzTV/Validators/**` (10 Blazor edit-VM FluentValidation validators),
|
||
`App.razor`, `_Imports.razor`, `ErsatzTV/Locals/Shared/**` + `ErsatzTV/Locals/Pages/**` (Blazor
|
||
localization resx — `ErsatzTV/Locals/Resources.*` is KEPT), `ErsatzTV/wwwroot/css/**` (site.css),
|
||
`ErsatzTV/wwwroot/lib/**` (jquery, jqueryui, sortablejs, hls, media-chrome, roboto), `libman.json`, and
|
||
`ErsatzTV.Tests/Pages/MultiSelectBaseTests.cs`.
|
||
|
||
**9 packages pruned** (from both `Directory.Packages.props` and `ErsatzTV/ErsatzTV.csproj`; each verified
|
||
to have zero remaining consumers after the Blazor deletion): **MudBlazor**, **Heron.MudCalendar**,
|
||
**Blazored.FluentValidation**, **BlazorSortable** — unambiguous Blazor UI; **MediatR.Courier.DependencyInjection**
|
||
— `ICourier` was consumed only by the deleted pages, and the app's `mediator.Publish` notification path is
|
||
plain MediatR (unaffected by the `AddCourier` removal); **Markdig**, **HtmlSanitizer**, **Chronic.Core**,
|
||
**NaturalSort.Extension** — verified zero non-Blazor consumers post-deletion.
|
||
|
||
**Startup surgical reduction.** Removed `AddRazorPages` (+`AuthorizeFolder("/")`), `AddServerSideBlazor`,
|
||
`AddMudServices`, `AddSortable`, `AddCourier`, the Blazor-attached `UseAuthentication`/`UseAuthorization`,
|
||
`MapBlazorHub`, and `MapFallbackToPage("/_Host")`. The former "blazor" `MapWhen` branch (lambda param
|
||
renamed `blazor`→`legacy`) is KEPT — it still co-hosts `MapControllers()`, `/docs` (Scalar), dev
|
||
`MapOpenApi()`, and the `LegacyUiRedirects` middleware. `MapFallbackToPage("/_Host")` is REPLACED by a
|
||
catch-all `endpoints.MapFallback(...)` that 302-redirects any unmatched path to `PathBase + "/app"`
|
||
EXCEPT paths under `/api`, `/artwork`, `/docs`, `/openapi` (those get a genuine 404, per #204's design).
|
||
**KEPT** (not removed): the OIDC/JWT/API-key SERVICE registrations (inert unless configured; real auth is
|
||
#197), `ConditionalIptvAuthorizeFilter` (`/iptv/*`), and `ApiKeyAuthorizationFilter` (mutating `/api/*`) —
|
||
per the #206 auth-posture sign-off (deleting the Blazor page challenged nothing beyond phase (a)).
|
||
|
||
**LegacyUiRedirects.** Added redirects for all 14 `/media/sources/*` routes → their `/app/libraries/*`
|
||
SPA screens (7 Tier-1 exact + 7 Tier-2 `{id}` patterns; the last Section-2 rows in
|
||
`blazor-route-parity.md`), and LIFTED the #204-era `/media/sources` forbidden-prefix guard (its Blazor
|
||
pages were replaced by #202's SPA screens). The forbidden-prefix guard now covers only `/api`, `/artwork`,
|
||
`/docs`, `/openapi`, `/iptv`, `/app`.
|
||
|
||
Also removed the now-dead ersatztv#25 razor-Sonar `<NoWarn>S6966;S3267;…</NoWarn>` line from
|
||
`ErsatzTV.csproj` — those Sonar rules only needed suppression in `.razor` `@code`; on `.cs` they run at
|
||
`suggestion` via `.editorconfig`, so removal is safe (closes part of #25's burn-down).
|
||
|
||
**Rollback.** The tag `blazor-final` was cut on the pre-removal `main` commit as the first step (see the
|
||
2026-07-11 "Pre-removal Blazor rollback tag `blazor-final` (#205)" entry above for the exact command +
|
||
restore path). Not a `v*` tag → no prod release build.
|
||
|
||
## 2026-07-11 — Stable child identity for schedule-item replace (#259, split from #252/#253)
|
||
|
||
`PUT /api/schedules/{id}/items` now reconciles by an optional round-tripped child id, not by array
|
||
position, so an item's persisted fill-group/shuffle state (`PlayoutScheduleItemFillGroupIndex`, FK
|
||
`OnDelete(Cascade)`) follows the logical item across reorders/inserts instead of being inherited by
|
||
whatever previously held its new slot. Contract + rules in **api-conventions §7c**. Key decisions:
|
||
|
||
- **`ScheduleItemRequest.Id` (`int?`)**: null/absent/`0` ⇒ new item (controller normalizes `0`→null so the
|
||
handler is two-state). Any id present ⇒ id-based reconcile; a fully id-less payload keeps the verbatim
|
||
positional fallback (legacy; retires with the §7a Phase-2 `If-Match`→428 flip).
|
||
- **Unknown or duplicate id ⇒ 422, nothing persisted**; the guards live in the handler **after** §7a
|
||
`CheckVersion`, so **412 precedes 422** — a client that is both version-stale and id-stale gets the reload
|
||
signal, not a payload-bug signal. Rationale for reject-not-insert on an unknown id: under Phase-1
|
||
force-write a stale id is a live lost-update signal, so silently inserting-as-new would duplicate the item
|
||
and return a different id than the client sent (the exact class §7a exists to surface). This is also the
|
||
correct #197 posture — never honor an unrecognized identifier.
|
||
- **Scope = schedule items only.** Blocks/templates/deco-templates/playlists stay positional: their children
|
||
are stateless config rows (no FK'd state to misattribute; #3/#4 have no GET child id). Child ids are added
|
||
only where a child row anchors server-side state; the contract can be retrofitted per-endpoint later
|
||
(field stays optional) — so this is not #197 ossification pressure.
|
||
- **TPT subtype change at a matched id** stays delete+insert (EF can't retype in place); state resets and a
|
||
new id is returned, so the SPA must re-seed item state from the PUT response (a stale id on a second save
|
||
now 422s).
|
||
|
||
## 2026-07-11 — Baseline security response headers + Phase-0 API hardening (#197, PR #279)
|
||
|
||
Phase-0 of the #197 remediation — the posture-**independent** safe subset, shipped ahead of the
|
||
fail-closed/CORS/versioning posture work tracked in #280–#289.
|
||
|
||
- **Baseline security headers on every response.** New `ErsatzTV/Middleware/SecurityHeadersMiddleware`,
|
||
registered **first** in the pipeline (before the `/iptv` `MapWhen` branch and `UseCors`), so it covers
|
||
`/api`, `/iptv`, `/artwork`, static, the SPA fallback, and filter-produced 4xx alike — which is why it's
|
||
middleware, not an MVC filter. It sets `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, and
|
||
`Referrer-Policy: strict-origin-when-cross-origin`. `nosniff` is the standing backstop for the artwork
|
||
content-type MIME-sniffing risk (#283). **CSP and HSTS are deliberately NOT included here**: CSP must be
|
||
validated against the ChicoryTV SPA's inline assets, and HSTS is a proxy/TLS-termination decision — both
|
||
belong to the #197 posture design (#284/roadmap), not this baseline. Headers are set eagerly (not via
|
||
`Response.OnStarting`); safe today because the pipeline has no `UseExceptionHandler`/`UseStatusCodePages`
|
||
that would `Response.Clear()` — switch to `OnStarting` if one is ever added.
|
||
- **Constant-time API-key comparison.** `ApiKeyAuthorizationFilter` compares `X-Api-Key` with
|
||
`CryptographicOperations.FixedTimeEquals` (over UTF-8 bytes) instead of ordinal `string.Equals`, removing
|
||
the response-timing oracle on the write key. Accept/reject behavior is otherwise identical.
|
||
- **Playout pagination clamped.** `GET /api/playouts` and `GET /api/playouts/{id}/items` now clamp
|
||
`Math.Clamp(pageSize, 1, 100)` + `Math.Max(0, pageNum)` before the query — applying the api-conventions §1
|
||
clamp convention the other paged endpoints already follow (these two were passing the raw client value
|
||
straight to EF `Take()`).
|
||
|
||
The larger #197 posture (fail-closed writes, sensitive-read auth tier, CORS lockdown, `/api/v1` versioning,
|
||
the OpenAPI security scheme) is decomposed into #280–#289 with the phased roadmap on #197; those PRs will
|
||
append their own decisions here as they land.
|
||
|
||
## 2026-07-12 — Artwork content-type is sniffed, never reflected (#283, S4/S9 stored XSS)
|
||
|
||
The artwork upload/serve path trusted client-supplied content types at both ends, giving a stored-XSS
|
||
chain on **unauthenticated** GET sinks: upload `<script>` bytes declared `image/png` →
|
||
`GET /iptv/logos/{hash}?contentType=text/html` served them as HTML in the ErsatzTV origin. The #279
|
||
`nosniff` header is not a fix here — the server was *explicitly declaring* `text/html`, which the browser
|
||
honors regardless of `nosniff`. The trust was the bug; the fix removes it at both ends.
|
||
|
||
- **Upload derives the content type from the bytes, never the declared value.** `UploadArtworkHandler`
|
||
buffers the (size-bounded) upload and calls `ErsatzTV.Core/Images/ImageContentTypes.DetectContentType`,
|
||
which uses SkiaSharp's `SKCodec` to identify the format from the image header only — pixels are **not**
|
||
decoded, so this can't be turned into a decompression-bomb vector. A payload that isn't one of the
|
||
accepted raster formats (png/jpeg/gif/webp) is rejected 422; the declared `Content-Type` is no longer
|
||
read at all (the field was dropped from the `UploadArtwork` command).
|
||
- **Serve sniffs the stored file; the `?contentType=` reflection is gone.** `GetCachedImagePath` no longer
|
||
carries a `ContentType`, and `GetImage` (`/iptv/logos`) / `GetWatermark` (`/artwork/watermarks`) dropped
|
||
their `[FromQuery] contentType` binding. `GetCachedImagePathHandler` always derives the MIME type from the
|
||
file (`MimeTypes.GetMimeTypeFromFile`) and **clamps it to the image allow-list** (`ImageContentTypes.IsAccepted`),
|
||
serving `application/octet-stream` for anything else — so a file whose bytes are not an accepted image (a
|
||
legacy cache entry poisoned before the upload sniff landed, or a hypothetical polyglot) is a non-renderable
|
||
download, never HTML/script. The removal is **structural** — there is no longer any request path that lets a
|
||
client choose the served `Content-Type`. `ArtworkContentTypeModel.UrlWithContentType` now returns the bare path, and the SPA
|
||
watermark/logo previews no longer append the query.
|
||
- **Defense-in-depth on the persisted JSON DTOs.** The `{path, contentType}` bodies (channel logo, watermark)
|
||
run their content type through `ArtworkContentTypeModel.Sanitized()`, which blanks anything outside the
|
||
image allow-list before it is stored — so a stale/hostile value can't be reflected by any future code path
|
||
even though the serve route already ignores it.
|
||
- **S9 upload-size DoS.** Kestrel `Limits.MaxRequestBodySize` is now set from `ETV_MAXIMUM_UPLOAD_MB`, so an
|
||
oversized body is rejected as it is read rather than only after the controller's post-binding `file.Length`
|
||
check (kept as the friendly-error backstop). This is a global bound; the app has no other large inbound
|
||
body (streaming is outbound GET).
|
||
|
||
`ImageContentTypes` is the single source of truth for the accepted image types (the allow-list previously
|
||
duplicated in `UploadArtworkHandler`). Both serve sinks are `[ApiExplorerSettings(IgnoreApi = true)]`, so
|
||
none of this changes the OpenAPI document.
|
||
|
||
## 2026-07-12 — Fail-closed API auth + sensitive-read tier + CORS/ForwardedHeaders lockdown (#197 Bundle A, PR #292)
|
||
|
||
Phase-1 of the #197 remediation — the auth posture that must land before any remote exposure.
|
||
Owner decisions (confirmed this session): **single API key** (no read/write split), and
|
||
**`Api:RequireKeyForReads` defaults `true`** (the whole `/api` surface requires the key). This does
|
||
**not** affect Jellyfin/streaming: `/iptv/*` (playlist/guide/streams/logos) and `/artwork/*` are outside
|
||
the filter's `/api` scope and keep their own optional access-token; only the management API the SPA talks
|
||
to is gated.
|
||
|
||
- **Fail-closed writes (#280, S1).** The empty-key "open" branch is deleted; there is no open mode. New
|
||
`IApiKeyProvider` (`ErsatzTV/Services/ApiKeyProvider.cs`, singleton, resolved once at startup) yields a
|
||
never-empty key: `Api:WriteKey` if set, else a key persisted at `FileSystemLayout.ApiKeyPath`
|
||
(`/config/api.key`, `0600`, path logged not value), else a generated 256-bit hex key. Every mutating
|
||
`/api` request now requires `X-Api-Key`.
|
||
- **Sensitive-read tier (#282, S3/S5).** Reads are gated by `Api:RequireKeyForReads` (default true) OR a
|
||
new `[RequiresApiKey]` marker (mirror of `[SkipApiKeyAuthorization]`) applied to
|
||
`Troubleshoot`/`Logs`/`Settings`/`Maintenance`, so that tier stays gated even if an operator opts reads
|
||
open. `OPTIONS` preflight is exempt (CORS middleware owns it). `ApiControllerSecurityTests` asserts the
|
||
tier reflectively.
|
||
- **Delete dead non-`/api` mutation surfaces (#281, S2).** `SortController`
|
||
(`POST media/collections/{id}/items`, dead Blazor SortableJS residue — the SPA uses
|
||
`PUT /api/collections/{id}/custom-order`) and `AccountController` (`POST account/logout`, dead OIDC)
|
||
bypassed the key because they sat outside `/api`. Removed rather than guarded.
|
||
- **CORS opt-in (#284, S6).** `AllowAnyOrigin/Method/Header` is replaced by the `ApiCors` policy: an
|
||
exact-origin allowlist from `Api:CorsAllowedOrigins` (semicolon list) that permits `X-Api-Key`/`If-Match`
|
||
and exposes `ETag`; with no origins configured there is no cross-origin access (the SPA is same-origin).
|
||
- **ForwardedHeaders trust + scanner loopback (#285, S7/S10).** `GET /api/maintenance/gc` → `POST`
|
||
(crawler-triggerable GC; spec regenerated). `ForwardedHeaders` trust is configurable via
|
||
`ForwardedHeaders:KnownProxies`/`KnownNetworks` — **unconfigured preserves the current trust-all
|
||
behavior but logs a warning** (flipping the default to loopback-only would break reverse-proxy scheme/host
|
||
detection and thus M3U/XMLTV absolute URLs — the operator must name their proxy network). `ScannerController`
|
||
gains `[LocalhostOnly]` (the scanner always calls back over `http://localhost:{UiPort}/api/scan/...`), which
|
||
is only spoof-resistant once ForwardedHeaders trust is restricted — the two interlock. `search/all-items`
|
||
DoS-paging is **deferred** (it feeds the SPA "add all" flow and needs coordinated pagination; the unauth
|
||
exposure is already closed by read-gating).
|
||
- **SPA (`web/`).** The client sends the stored key (`ctv-api-key`) on **every** method (not just
|
||
mutations); a new keyless **API Key** screen (`/app/api-key`) lets the user paste the generated key, and a
|
||
shell-level banner points there on any 401. See spa-conventions §5e. **First-run/upgrade UX:** with reads
|
||
gated by default, the SPA shows no data until the key (from `/config/api.key`) is entered — an intended
|
||
consequence of the strict default.
|
||
|
||
Phase-2 (contract freeze) — the declarative OpenAPI security scheme, global 401 docs, and `/api/v1`
|
||
versioning — remains #286/#287/#288. Phase-3 follow-ups: #265, #269, #172 remainder, `search/all-items`
|
||
paging, per-key rate limiting.
|
||
|
||
## 2026-07-12 (#197 Bundle C — contract-freeze honesty)
|
||
|
||
**#287 — OpenAPI contract honesty by construction.** The "v1" document now emits the `ApiKey` security
|
||
scheme plus per-operation `security`/`401` derived from the *same*
|
||
`ApiKeyAuthorizationFilter.EndpointRequiresKey` predicate the runtime filter enforces, so declared auth
|
||
can never drift from enforced auth. Every operation also gets a synthesized stable `operationId` (the
|
||
framework only assigned one when `Name=` was set — ~90 were missing), and body/param-binding operations
|
||
get the documented `400 ValidationProblemDetails` they actually return. `DayOfWeek` is now a string enum
|
||
in the schema (added to `Startup.UseStringEnumSchemas`), removing the SPA's `WithDayNames` wart. Pinned by
|
||
in-process document generation in tests (`OpenApiContractHonestyTests`) rather than the committed `v1.json`.
|
||
|
||
**#288 — Wrap the last raw ViewModels; reverse the §7a "intentional `version` leak."** Minted
|
||
`MediaCollectionResponseModel`, `ProgramScheduleResponseModel`, and `ChannelDetailResponseModel` (all
|
||
`#nullable enable`) and routed `CollectionController` / `ScheduleController` / `SmartCollectionController` /
|
||
`ResolutionController.GetResolutionByName` / the channel detail GET+writes through ResponseModels, so no
|
||
`/api/*` action returns an Application VM. This reverses the earlier §7a judgment that a ResponseModel
|
||
"purely to hide one field was disproportionate": `Version` is now header-only (ETag) on every aggregate
|
||
body — confirmed safe by grepping `web/src` (the SPA reads `version` from the ETag header, never the
|
||
response body). `ChannelDetailResponseModel` is the *full editable* field set the channel editor needs
|
||
(distinct from the lean list `ChannelResponseModel`; drops only the derived `webEncodedName`). Also flipped
|
||
`#nullable enable` onto the remaining 24 lagging `ErsatzTV.Core/Api/` files for schema honesty, and added
|
||
`pageNum` paging to `GET /api/search`.
|
||
|
||
**Channel REST resources are keyed by database `Id`, never by `Number`.** `Channel.Number` is user-mutable
|
||
(editable on update, bulk-renumbered via `/api/channels/bulk/renumber`, transiently invalid mid-renumber),
|
||
so the immutable int PK is the canonical key for all `/api/channels/*` single-item routes, sub-resources
|
||
(including `playout/reset`, re-keyed from `{channelNumber}` to `{id:int}` in Bundle C), and `Location`
|
||
headers. `Number` remains the identity on broadcast surfaces only (IPTV/M3U/XMLTV), a separate contract. A
|
||
number-based lookup endpoint may be added additively later; `UniqueId` (Guid) stays out of the REST
|
||
contract absent a federation requirement.
|
||
|
||
## 2026-07-12 (#269 — non-If-Match root writers force-write past a concurrent Version bump)
|
||
|
||
**Routing the aggregate delete handlers + `UpdateProgramScheduleHandler` through `SaveChangesForcingVersion`.**
|
||
Once #253 made each replace-all root's `Version` an `IsConcurrencyToken`, EF started guarding *every*
|
||
UPDATE **and DELETE** of that row with `WHERE Version=@orig` — so any writer that is not part of the
|
||
If-Match contract but still saves via plain `SaveChangesAsync` throws an unhandled
|
||
`DbUpdateConcurrencyException`→**500** if a replace-all editor bumps the row in its narrow load→save window.
|
||
PR3 already force-wrote the exposed *UPDATE* siblings (Playout settings/`ScheduleFile`/checkpoint,
|
||
`UpdateCollectionHandler`); a completeness sweep for #269 found the gap was wider than reported —
|
||
**18 writers** in total, all on plain `SaveChangesAsync`. **The correct exposure filter is "any handler
|
||
that leaves a versioned root `Modified` or `Deleted`", NOT just `Version`-bumpers + deletes** — an early
|
||
sweep used the narrower filter and a review of PR #302 caught what it missed (`ErasePlayoutHistory` below):
|
||
- the **nine versioned-root delete handlers** (`DeletePlayout`/`DeleteCollection`/`DeleteMultiCollection`/
|
||
`DeleteRerunCollection`/`DeletePlaylist`/`DeleteBlock`/`DeleteTemplate`/`DeleteDecoTemplate`/
|
||
`DeleteProgramSchedule`) — a DELETE is now token-guarded too;
|
||
- `UpdateProgramScheduleHandler` (bumps `Version` then saved plainly — the ProgramSchedule case PR3 only
|
||
*suspected*);
|
||
- the **seven item add/remove bumpers** that PR2 wired to bump their root but left on plain save —
|
||
`AddProgramScheduleItem`/`DeleteProgramScheduleItem` and the five
|
||
`Add{Items,Movie,Show,Season,Episode}ToPlaylist` handlers;
|
||
- **`ErasePlayoutHistoryHandler`** — modifies Playout root **scalars** (`Seed`/`Anchor`/`OnDemandCheckpoint`)
|
||
**without** bumping `Version`, inside an explicit transaction with no try/catch → the one the bumper-only
|
||
filter missed; reachable via `POST /api/playouts/{id}/erase-items-and-history`.
|
||
|
||
All now save through `ConcurrencyExtensions.SaveChangesForcingVersion`.
|
||
|
||
**Two deliberate boundaries (documented, not gaps):** (1) the background build/time-shift Playout-scalar
|
||
writers (`BuildPlayoutHandler` via `PlayoutBuilder`'s `Anchor`/`Seed`; `PlayoutTimeShifter`'s
|
||
`OnDemandCheckpoint`) are token-guarded too but **intentionally left on plain save** — they never surface a
|
||
request-path 500 (`BuildPlayoutHandler` catches → a build-failure `BaseError`; `PlayoutTimeShifter` runs only
|
||
via the background worker), and force-writing would be *wrong*: a concurrent config edit that bumped
|
||
`Version` also enqueues a rebuild, so failing the in-flight build and letting the rebuild redo it with fresh
|
||
config is correct (force-writing would persist output built from stale config). (2)
|
||
Item-add force-write can leave a duplicate/gap `Index` (accepted Phase-1 effect): the handler computes the
|
||
new index from its stale child list, so if a concurrent replace-all grew the list the item lands at a
|
||
now-colliding index (no unique constraint on `PlaylistItem.Index`/`ProgramScheduleItem.Index`) — non-
|
||
corrupting, self-correcting on the next edit, still strictly better than the pre-#269 500; a
|
||
reload-and-recompute-on-conflict refinement is a candidate for #197. Decision:
|
||
**force-write, not 412** — these endpoints take no `If-Match` (an unconditional DELETE/settings-edit should
|
||
win over a concurrent editor), matching the Phase-1 force-write posture. A delete has no ETag to rotate, so
|
||
it needs only the force-write, not a `Version` bump. A genuine row-deletion race (two concurrent deletes)
|
||
still surfaces as a `DbUpdateConcurrencyException` — accepted (rare, non-corrupting, the resource is already
|
||
gone). **Still deferred to #197:** *cross-editor ETag rotation* for the non-bumping config siblings and the
|
||
scanner-shared `Add*ToCollection` family (they don't 500 — they insert children / `ExecuteDelete`, neither
|
||
of which is token-guarded — they just don't rotate an open editor's ETag). Non-vacuously tested by racing a
|
||
bump *through the handler* via a pre-tracked context (`RootWriterForceVersionTests`), plus an explicit
|
||
negative control proving the plain-save path throws.
|
||
## 2026-07-12 — Browser SPA session auth: `/api` accepts session OR machine key (#295 PR1, server-only)
|
||
|
||
Implements the ratified #295 design (Fable [PLAN-MODE] pass, issue comment 9548). Supersedes the #206
|
||
"OIDC wiring stays inert until #197" note: the retained OIDC service registration is now **revived**, and a
|
||
cookie session becomes a first-class `/api` credential alongside the machine `X-Api-Key`. **PR1 is
|
||
server-only and backward compatible** — the SPA keeps sending its stored key; the SPA login flow, the
|
||
`ApiKeyScreen`→machine-key repurpose, and `spa-conventions §5e` land in **PR2**.
|
||
|
||
**One gate, evolved (not `[Authorize]`-per-controller).** `ApiKeyAuthorizationFilter` → `ApiAuthorizationFilter`,
|
||
same fail-closed-by-omission logic (a forgotten `[Authorize]` fails *open* — the #280 failure mode — so the
|
||
global filter stays the gate). It now accepts a request when a valid `X-Api-Key` matches **OR** the principal
|
||
is an authenticated session; the "does this endpoint need auth?" decision is still the single shared
|
||
`EndpointRequiresKey(...)` predicate (also drives OpenAPI, so the spec can't drift). Attributes renamed to
|
||
match the widened meaning: `[RequiresApiKey]`→`[RequiresAuthentication]`, `[SkipApiKeyAuthorization]`→
|
||
`[SkipApiAuthorization]`. `IApiKeyProvider`, the `X-Api-Key` header, and `Api:WriteKey`/`Api:RequireKeyForReads`
|
||
are unchanged — **machine/key behavior is byte-identical** (verified: no OpenAPI drift, existing filter tests
|
||
still green).
|
||
|
||
**CSRF (session only).** The machine key is CSRF-immune (a browser can't set a custom header cross-origin
|
||
without a credentialed CORS grant we never issue). A cookie session is not: a session-authenticated **mutation**
|
||
must carry the `X-CSRF` header (presence-only — a custom header forces a CORS preflight a cross-site page can't
|
||
satisfy) or is rejected **403**. Reinforced by `SameSite=Lax` + CORS without `AllowCredentials` (cross-origin
|
||
cookie auth is impossible by design). No antiforgery-token machinery.
|
||
|
||
**Cookie `ctv-session`.** Always registered (local login works with no IdP); OIDC handler added only when
|
||
`OIDC:*` is configured. `HttpOnly`, `SameSite=Lax`, `SecurePolicy=SameAsRequest` (so a plain-HTTP LAN isn't
|
||
bricked), 14-day sliding. `/api` XHR gets **401/403, not a redirect** (`OnRedirectToLogin`/`AccessDenied`).
|
||
The `UseAuthentication`/`UseAuthorization` middleware — deleted with Blazor in #91b — is **revived in the
|
||
`legacy` `MapWhen` branch only** (hosts `/api` + OIDC `/callback` + `/docs`; `/iptv` and `/app` untouched).
|
||
|
||
**Local store = `ConfigElement` rows, single admin, NO migration** (owner ruling F2):
|
||
`AuthLocalAdminUsername`, `AuthLocalAdminPasswordHash` (ASP.NET `PasswordHasher`, PBKDF2, via
|
||
`Microsoft.Extensions.Identity.Core`), `AuthSecurityStamp`. A password change rotates the stamp; the cookie
|
||
`OnValidatePrincipal` (`CookieSecurityStampValidator`) compares the claim to the stored stamp and rejects a
|
||
stale session (revocation). OIDC sessions carry an `etv:auth_method=oidc` claim and skip the stamp check
|
||
(governed by the IdP).
|
||
|
||
**Fail-closed out of the box + recovery.** An unconfigured instance keeps `/api` gated (the key still works);
|
||
first-run is a **setup-claim** (`POST /api/auth/setup`, first-claim-wins, only valid while unconfigured —
|
||
owner ruling F1). Recovery without the browser: `Auth:LocalAdmin:Password` env seed (`LocalAdminSeedService`,
|
||
overwrites + rotates the stamp on startup) or the machine key. Login hardening: per-IP rate limit
|
||
(`[EnableRateLimiting("auth")]`, 10 / 5 min) on login/setup/password, dummy-hash verify on unknown/unconfigured
|
||
user (no enumeration).
|
||
|
||
**Authelia = app-owned OIDC session; never trust proxy identity headers** (owner ruling F3): the container is
|
||
LAN-reachable bypassing the proxy, so `Remote-User`/`Remote-Email` header trust is spoofable. OIDC→Authelia
|
||
gives SSO without a double login. **`ForwardedHeaders` behaviour is kept unchanged from #285** (trust any peer
|
||
with a warning; restrict via `KnownProxies`/`:KnownNetworks`). A stricter "ignore `X-Forwarded-*` unless a proxy
|
||
is configured" default was implemented and then **reverted** after review (cold fork M1): the forwarded
|
||
scheme/host feed `/iptv` M3U/XMLTV/HLS absolute-URL generation (`Request.Scheme` in `GetChannelGuideHandler`/
|
||
`IptvController`), so ignoring them would regress stream URLs to `http`/internal-host for a proxied deployment
|
||
that hasn't set `KnownProxies`. **Deployment coordination:** operators behind a proxy should set
|
||
`ForwardedHeaders:KnownProxies`/`:KnownNetworks` — it gives the login rate limiter an unspoofable client IP and
|
||
marks the session cookie `Secure` behind TLS. The residual (a direct LAN peer can spoof `X-Forwarded-For` to
|
||
evade the per-IP login limit when unrestricted) is accepted defense-in-depth loss, mitigated by PBKDF2 +
|
||
no-enumeration.
|
||
|
||
**Review hardening (fork + independent Codex pass, folded into PR1).** Codex caught concurrency defects the
|
||
fork missed — folded in: (a) **atomic first-claim-wins** — setup writes the three credential rows in one
|
||
transaction guarded by the unique `ConfigElement.Key` index (a lost race → `DbUpdateException` → 409), so a
|
||
concurrent claim can't produce a mixed-state credential; (b) **consistent login snapshot** — login reads the
|
||
hash + stamp in one query and no longer rehashes-on-verify, so a login racing a password change can't capture a
|
||
newer stamp than the hash it verified (a concurrent change either fails the old password or leaves the issued
|
||
cookie carrying the pre-change stamp → revoked next request); (c) **env-seed waits on
|
||
`SystemStartup.WaitForDatabase`** (the migrator is a `BackgroundService`, so registration order alone didn't
|
||
guarantee the schema existed) — moved to `Services/RunOnce/`. Also: **logout + password require `X-CSRF`**
|
||
(the `[SkipApiAuthorization]` auth surface isn't covered by the filter's CSRF check → forced-logout CSRF), and
|
||
input length caps on username/password. **Logout rotates the security stamp** when called from a local session
|
||
(E2E-caught: `SignOutAsync` alone only clears the *client* cookie, leaving the stateless encrypted ticket
|
||
replayable server-side) — so signing out actually ends the session server-side; for the single admin this
|
||
revokes all local sessions ("log out everywhere"). Gated on an authenticated session so an unauthenticated
|
||
caller can't force-revoke the admin. **Deferred with a tracked gate:** side-effecting `[RequiresAuthentication]`
|
||
GETs (troubleshoot playback/archive) aren't CSRF-covered — **#301**, gating PR2 (latent in PR1: the SPA still
|
||
uses the machine key). OIDC-session revocation lever (no local stamp) noted for PR3 operator docs.
|
||
|
||
A **fix-commit re-review** (Codex, #242 discipline) then confirmed the above resolved and caught a second round:
|
||
(a) **HIGH — env-seed vs. setup race**: an attacker could claim admin in the startup window before
|
||
`LocalAdminSeedService` runs, and the seed's insert would then be swallowed (attacker's credential persists,
|
||
defeating the env recovery path). Fixed structurally: **the setup-claim endpoint is closed whenever
|
||
`Auth:LocalAdmin:Password` is configured** — the env seed owns the credential, so there is no claim to race
|
||
(this also strengthens the setup-claim TOFU posture: an operator on an untrusted network sets the env password
|
||
and browser setup is disabled). (b) **LOW**: a concurrent setup race-loser now returns **409** (not 422), and
|
||
`ClaimLocalAdmin`'s `DbUpdateException` catch re-checks existence and **rethrows genuine/transient DB errors**
|
||
rather than masking them as "already configured". (c) **MEDIUM — accepted**: two *simultaneous* authenticated
|
||
password changes are a non-serializable lost-update (last-write-wins; the loser's cookie may be immediately
|
||
revoked). Accepted for a **single-admin** system: it needs two concurrent authenticated sessions both submitting
|
||
the correct current password at the same instant, and the outcome is self-healing (re-login). Adding EF
|
||
optimistic concurrency to the credential rows is disproportionate here.
|
||
|
||
**OpenAPI = `ApiKey`-only; `/api/auth/*` excluded** (owner ruling F4): the spec's audience is machine/MCP
|
||
clients, and a browser-interactive cookie login isn't something a generated client drives, so the cookie path
|
||
is an additional accepted credential the doc needn't express. `AuthController` is `[ApiExplorerSettings(IgnoreApi
|
||
= true)]`. Verified: no `v1.json`/`v1.d.ts`/`endpoint-index` drift from this PR.
|
||
|
||
**Phasing.** PR1 = this (server only, no migration). PR2 = SPA (drop the key header for browser calls + add
|
||
`X-CSRF`, `AuthContext` + boot gate, login/setup screens, `ApiKeyScreen`→machine-key management, E2E,
|
||
`spa-conventions §5e`). PR3 = key rotation + operator docs (Authelia client + env reference). Rollout: PR1→PR2
|
||
same release, then a manual Authelia round-trip checklist before the prod pin bump.
|
||
|
||
---
|
||
|
||
## 2026-07-12 — Merge-consent derived from state via a `## Done-when` issue checklist (#303 H6)
|
||
|
||
**An issue's `## Done-when` checklist (in the issue body) is the machine-readable source of truth for whether
|
||
its PR may merge; consent is *derived*, not asserted.** Rationale: DONE/OPEN status used to live in
|
||
append-only prose that lags live Gitea state (the queue-drift #303 fixes) — so the completion gate moves out
|
||
of memory and into a checklist two hooks read. Convention: the issue body carries a `## Done-when` section
|
||
(always an "adversarial review passed" box, plus per-issue criteria); a merge is allowed only when the PR's CI
|
||
is green **and** every box on the linked issue (`fixes #N`) is ticked.
|
||
|
||
Enforcement (both fail *safe*, never a silent pass):
|
||
- `pretooluse-merge-consent.sh` — Claude PreToolUse on `mcp__gitea__pull_request_write` merge: **deny** on an
|
||
unticked box or non-green CI; **allow** when both satisfied; **ask** (human prompt) when state isn't
|
||
derivable (no linked issue, no `## Done-when`, no creds, Gitea unreachable). Docs-only PRs exempt.
|
||
- `.husky/pre-push` → `prepush-donewhen.sh` — backstop for a direct `git push origin main`; fail-*open* (a
|
||
git hook has no "ask"), blocks only on a positively-proven unticked box.
|
||
|
||
Both authenticate to Gitea from env only (`ETV_GITEA_BASICAUTH` / `ETV_GITEA_TOKEN`, `ETV_GITEA_URL`) — no
|
||
creds committed; without them the gate degrades to today's manual confirmation. Rollout is non-breaking: until
|
||
issues adopt `## Done-when`, the merge hook simply *asks* rather than auto-allowing. See CLAUDE.md → Task
|
||
Completion Protocol. (H6 lives with H1/H2/H8 in `.claude/settings.json`; H7 worktree-owner guard is its
|
||
sibling Wave-2 hook.)
|
||
|
||
---
|
||
|
||
## 2026-07-12 — Blocking CI gate for API-contract artifacts (#303 H4/H5)
|
||
|
||
**A PR whose diff touches `ErsatzTV/Controllers/Api/**` or `ErsatzTV.Core/Api/**` must ship the
|
||
regenerated OpenAPI artifacts in that same diff, enforced by a blocking `api-docs` CI job.** It rebuilds
|
||
`ErsatzTV/wwwroot/openapi/v1.json`, `web/src/api/generated/v1.d.ts`, and `docs/endpoint-index.md` from
|
||
source and fails on any drift. This mechanizes the previously prose-only "docs-update in the same PR"
|
||
rule for the API contract (the `docs-reminder` job stays a non-blocking nudge for the route-parity doc).
|
||
Path-gated *inside* the job (not via a top-level `if:`) so it always reports a status on every PR — API-free
|
||
PRs skip the expensive regen and pass trivially, so it is safe as a required check. Rationale: generation
|
||
is deterministic from a **fresh** build (verified — a clean checkout reproduces the committed spec exactly,
|
||
including the 244 auth `security`/`401` blocks), so CI can trust regenerate-and-diff. The one caveat is
|
||
local-only: `update-openapi.sh` runs `dotnet-getdocument` against the already-built assembly, so a stale
|
||
`bin/` silently yields a stale spec — always `dotnet build` first (see `api-conventions.md` §5). CI is
|
||
immune (no `bin/` on a fresh checkout).
|
||
|
||
## 2026-07-12 — decisions.md is append-only, enforced; root-screenshot guard (#303 H9/H3)
|
||
|
||
**This log is append-only by construction, not just by convention.** A commit or PR that deletes or
|
||
modifies an existing line of `docs/decisions.md` is blocked — by the Husky `commit-msg` hook
|
||
(`.claude/hooks/decisions-guard.sh staged`) locally and the blocking `decisions-guard` CI job (same
|
||
script, `range` mode) on PRs. Shared detection, deliberately different granularity: the Husky hook
|
||
gates **each commit** (its own message must carry the token); CI gates the **PR-wide** net diff
|
||
(token in any commit of the range suffices), so the local hook is the stricter primary gate and CI the
|
||
push/bypass backstop. Insertions anywhere are always allowed, so a normal new entry (TOC line
|
||
near the top + a block appended at the bottom, both pure insertions) passes untouched. Detection is
|
||
`git diff --numstat` deleted-count > 0, which is robust to markdown `-` list markers (a byte-level `-`
|
||
prefix would false-match). The block is lifted only by the literal **`[decisions-edit]`** token in the
|
||
commit message, reserved for two cases: fixing a factual error, and superseding a reversed decision
|
||
(add the new entry, prepend a `> **Superseded …**` banner to the old one, tag its Index line
|
||
`(superseded)` — keep the old rationale, never silently rewrite). **Consolidation** of superseded
|
||
entries is a release-checklist step (`docs/ci-cd.md` → Versioning & releases), backstopped by a
|
||
non-blocking 1800-line **size floor** in the `decisions-guard` job (the read-cost point past which the
|
||
log no longer fits one default agent Read), so append-only doesn't accrete contradictory *or
|
||
unreadably-large* history between releases (Timothy's call, 2026-07-12: mark-and-keep on reversal,
|
||
consolidate at each milestone, size-floor backstop).
|
||
|
||
Companion guard **H3**: the Husky `pre-commit` hook refuses a staged **root-level `*.png`** (a
|
||
review/debug screenshot dropped at the repo root) — belt-and-suspenders with the `.gitignore` rule, so
|
||
a forced `git add -f` still can't land one. Nested `*.png` (real assets) are unaffected. Rationale for
|
||
both: the methodology review (#303) — make the process rules derivations/hooks, not prose to remember.
|
||
|
||
---
|
||
|
||
## 2026-07-12 — Review-verdict merge-gate: latest commit must be reviewed (#303 H10)
|
||
|
||
**A PR may not merge until a `Review-verdict:` comment on it references the PR's CURRENT head sha** —
|
||
so the *latest* commit is proven-reviewed, not a stale earlier diff. This mechanizes the ersatztv#242
|
||
lesson ("re-review the fix commit, not just the initial PR diff": a review of an earlier revision does
|
||
not license merging a head that carries un-reviewed follow-up commits). It folds into the existing H6
|
||
`pretooluse-merge-consent.sh` as condition (c), reusing its PR fetch, docs-only exemption, and
|
||
Gitea-auth-from-env (no second hook → no detection drift, per the #303 methodology review).
|
||
|
||
Convention: after reviewing a PR (or its latest fix commit), post a PR **comment** (issue-style, not a
|
||
Gitea formal-review body — the gate reads `issues/{pr}/comments`) whose line **starts with** the marker:
|
||
`Review-verdict: <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> @ <head-sha>` (short ≥7-char or full sha).
|
||
The gate counts a line as a verdict only when the marker is at line-start (after optional indent) —
|
||
a comment that merely *quotes* the template mid-sentence (an instruction "please post: Review-verdict:
|
||
MERGEABLE @ …", or the gate's own suggestion text echoed back) does **not** self-approve the merge
|
||
(adversarial re-review false-open, folded pre-merge). It then classifies each verdict line by the sha
|
||
in its `@ <sha>` field, matched to the head by **git short-sha prefix semantics** (head *begins with*
|
||
the token, token ≥7 chars) — NOT a loose substring test, so an older sha that merely contains the head
|
||
prefix, or the head prefix appearing in an unrelated URL on the line, does not count:
|
||
- a MERGEABLE/APPROVED/LGTM verdict whose `@ <sha>` is the current head → **allow**;
|
||
- a **negative** verdict (BLOCKED/NOT-MERGEABLE) *on the head* → **deny**, and it *wins over* a positive
|
||
one on the same head (a later BLOCKED retracts an earlier MERGEABLE; to retract, re-review head and
|
||
post BLOCKED @ head). Staleness is **symmetric on purpose**: a negative for an *older* commit is stale
|
||
exactly like a positive for an older commit, and does NOT override a fresh head-positive — otherwise a
|
||
pre-fix `BLOCKED @ oldsha` would block forever even after the fix changes the sha and earns a fresh
|
||
`MERGEABLE @ head` (the normal flow). So a genuine block must reference head, per the convention;
|
||
- verdict comment(s) exist but reference only *older* commits → **deny** — the stale-review case #242
|
||
targets;
|
||
- a `Review-verdict:` marker with **no `@ <sha>`** at all → **ask** (a lazy/quoted marker; not
|
||
mislabelled as stale);
|
||
- no `Review-verdict:` comment at all → **ask** (graceful adoption, mirrors H6's "no Done-when →
|
||
ask": surface, don't hard-block a PR that hasn't adopted the convention yet);
|
||
- comments unfetchable / head sha unresolvable → **ask**.
|
||
|
||
Scope: the Claude PreToolUse gate on the Gitea merge tool only. A direct `git push origin main` has no
|
||
PR comments to check, so the `.husky/pre-push` backstop is not extended for H10 (the merge tool is the
|
||
real merge path; docs-only PRs remain exempt via H6's file-set exemption). Rationale, as with the whole
|
||
Wave-1/2/3 hook set: make the process rule a derivation/hook, not prose to remember (#303).
|
||
|
||
---
|
||
|
||
## 2026-07-12 — Cross-editor ETag rotation completed for Collection/Playout config siblings (#269)
|
||
|
||
The #253 optimistic-concurrency contract (§7a) had a documented tail: the non-If-Match config-sibling
|
||
writers of a versioned root mutated editor-visible state **without** bumping `Version`, so editing through
|
||
them did not rotate an open editor's ETag (a cross-editor invalidation gap — never a lost-update or a 500,
|
||
which the primary endpoints' bump+guard already cover). #269's first slice (PR #302) removed the 500 exposure
|
||
by routing those writers through `SaveChangesForcingVersion`; this slice completes the **rotation**.
|
||
|
||
Handlers now bumping `Version` (all via `SaveChangesForcingVersion`, since they take no `If-Match` → a
|
||
concurrent replace-all bump force-writes, never 412/500): the Collection `Add*ToCollection` family (11
|
||
handlers) and `RemoveItemsFromCollectionHandler` bump `Collection.Version`; `UpdateCollectionHandler`
|
||
(name/flag), `UpdatePlayoutHandler` (`DailyRebuildTime`), and the three `ScheduleFile` writers
|
||
(`UpdateSequential`/`UpdateScripted`/`UpdateExternalJsonPlayout`) — which already force-wrote — now also bump.
|
||
|
||
Decisions frozen (ratified with Fable before implementation, feeding the #197 contract freeze):
|
||
|
||
- **Rotate on every editor-visible config change, no per-aggregate carve-outs.** §7a's config-only boundary
|
||
("every mutating handler of a root's editor-visible config bumps `Version`") already held for Playlist
|
||
`Add*`/schedule item writers; the Collection/Playout siblings were an inconsistency, not a judgment call. A
|
||
membership add rotating an open custom-order editor's ETag (→ 412 → reload) is correct: its list is genuinely
|
||
stale. Blast radius of the aggressive-but-safe rotation is a reload, never data loss.
|
||
- **No-op idempotence — the trap Fable caught.** These handlers gate their reindex/`BuildPlayout` fan-out on
|
||
`SaveChanges() > 0`. An *unconditional* bump makes that gate always-true, so an idempotent re-add / same-value
|
||
re-submit would fire spurious rebuilds across every playout using the aggregate. Fix: short-circuit a genuine
|
||
no-op **before** the bump — the Add handlers by an explicit membership check (which also fixes the latent
|
||
duplicate-`CollectionItem` insert on a *sequential* re-add; two *concurrent* same-item adds can still both
|
||
pass the check and the loser 500s on the composite-PK unique violation — `SaveChangesForcingVersion` catches
|
||
only `DbUpdateConcurrencyException`, not `DbUpdateException`. That race is narrow and pre-existing, deferred
|
||
to #308), the scalar writers (`UpdateCollection`, `UpdatePlayout`, the
|
||
three `ScheduleFile` writers) by `ChangeTracker.HasChanges()`. A no-op neither bumps nor rebuilds nor rotates
|
||
the ETag — which is itself correct (nothing changed).
|
||
- **The `Add*ToCollection` family is not repository-mediated.** #269's original framing ("repository-mediated,
|
||
shared with the scanner hot path") was wrong: `IMediaCollectionRepository` is read-only; each handler loads
|
||
the `Collection` into its own `dbContext` and writes directly. So the rotation bump is a pure API-layer
|
||
concern and the scanner's separate membership-write path is untouched — a background scan does **not** rotate
|
||
the editor ETag (correct: background indexing is not an editor action).
|
||
- **Force-write rebases the bump, never adopts the stored token verbatim (Codex review of this PR).**
|
||
`SaveChangesForcingVersion` originally resolved a conflict by setting current=original=stored — which
|
||
silently *discarded* a sibling's pending `Version++` when a versioned writer committed in its load→save
|
||
window (sibling loads 1, bumps to pending 2, concurrent PUT commits 2 → retry wrote 2, so the concurrent
|
||
writer's ETag "2" stayed valid and the rotation was lost under exactly the race it exists for). Fixed in
|
||
this PR (it affects all 25 bumpers routed through the helper, including the pre-existing playlist/schedule
|
||
ones): the retry now rebases — original = stored, current = stored + (pending current − pending original) —
|
||
so a bumper lands at stored+1 and a non-bumper (delta 0, e.g. `ErasePlayoutHistory`) adopts stored unchanged.
|
||
The race tests assert the post-race Version (3, not 2) and fail against the verbatim-adopt implementation.
|
||
- **No new status codes.** These endpoints take no `If-Match` and force-write, so they never 412; no
|
||
`[ProducesResponseType(...412...)]` and no OpenAPI regen (response types unchanged). Only §7a prose changes.
|
||
|
||
Tests: `CollectionEtagRotationTests` (rotation + no-op-without-bump-or-rebuild + force-write-past-concurrent-bump
|
||
for Add/Remove/Update) and `PlayoutScheduleFileEtagRotationTests` (ScheduleFile rotation + no-op-without-refresh),
|
||
the no-op guard proven non-vacuous by inverting the membership check. The `#265` RFC-7232 If-Match parser
|
||
refinement (valid-but-non-matching/weak/list → 412 not 400) is a **separate** PR (disjoint surface: the shared
|
||
parser + `CheckVersion`, not the handler saves). Refs #253 #269 #197 · `api-conventions.md` §7a.
|
||
|
||
---
|
||
|
||
## 2026-07-12 — Live-E2E is a required step for API write-path handler changes (#303)
|
||
|
||
**A PR that changes an API write-path handler (a `POST`/`PUT`/`DELETE` `/api/*` command that mutates
|
||
state and reloads it through the read path) MUST include a live-E2E pass** — driving the real endpoint
|
||
or its SPA screen and confirming the mutation round-trips through a subsequent read — not only unit /
|
||
characterization tests. Rationale: this class has a **correlated blind spot** unit and characterization
|
||
tests share. A green fixed-point test passed while the write-path returned a production 500 because the
|
||
handler returned a *lazy* LanguageExt `Map` the test never enumerated (#229; reload-through-read-path
|
||
mechanics in `api-conventions.md` §7); the failure surfaces only when the result is materialised, which
|
||
the SPA does and the test did not. Live driving is the only reliable net for it.
|
||
|
||
Non-write-path (pure-SPA/read-only) and docs PRs don't need it. The requirement is auditable, not
|
||
silent: the PR/close comment states that live-E2E ran, or — for a non-write-path change — that it
|
||
wasn't required (the same stated-exemption discipline as the review skip rubric). Recipe +
|
||
"When live-E2E is required": `docs/e2e-local.md`. This formalizes the #229 lore bullet ("live E2E
|
||
remains the only net for this class") into a standing convention.
|
||
|
||
---
|
||
|
||
## 2026-07-12 — Formatting-as-you-touch, enforced; rebase-not-merge for PR branches (#311 H11 + format CI)
|
||
|
||
Two coupled process decisions, prompted when a stale docs branch *merged main in*, dragged ~17
|
||
legacy-BOM `.cs` files it never touched into the merge commit, and the pre-commit `dotnet format`
|
||
hook then blocked on code that wasn't the author's (#309 session; the BOM backlog is #310).
|
||
|
||
**1. Formatting-as-you-touch is the standing rule, and it is now *enforced* (not just prose).** ~2500
|
||
of ~3900 `.cs` files carry a legacy UTF-8 BOM that violates `.editorconfig`'s `charset=utf-8`. We do
|
||
**not** mass-reformat (a repo-wide normalization stays an unmade, separate decision). Instead, a file
|
||
you modify for other work must be normalized (`dotnet format`, incl. BOM strip) in that same PR.
|
||
Enforcement — previously only the `--no-verify`-bypassable pre-commit hook, which is how #269 landed 17
|
||
BOM files (CI never checked charset):
|
||
- a **blocking `format` CI job** runs `dotnet format --verify-no-changes` **scoped to the PR's changed
|
||
`.cs`** (vs the merge-base) — so it demands conformance only of files the PR touched, never the
|
||
untouched legacy 2500; a `.cs`-free PR skips the expensive steps and passes (always reports a status,
|
||
safe as a required check). This closes the "CI never verifies charset/format" gap.
|
||
- `docs/contributing.md` §7 documents the rule.
|
||
|
||
**2. Keep a PR branch current by REBASING on `origin/main`, never merging main in (H11).** A merge
|
||
commit pulls in *every* file main changed — including files the author never touched — which then trip
|
||
the format hook/CI on code that isn't theirs; rebasing keeps the diff to exactly what changed.
|
||
Enforced by `.claude/hooks/prepush-rebase-check.sh` wired into `.husky/pre-push`: a push from a branch
|
||
that is behind `origin/main` (origin/main not an ancestor of HEAD) is **blocked** with
|
||
`git rebase origin/main` guidance. Fail-open (offline / no origin/main / not a repo → allow, since a
|
||
git hook has no "ask"); deliberate escape `ETV_SKIP_REBASE_CHECK=1`. This supersedes the old lore
|
||
guidance to "merge main into your PR branch." (After a rebase that conflicts in *generated* artifacts —
|
||
v1.json/v1.d.ts/endpoint-index — regenerate, don't hand-resolve; `npm run check:api` guards.)
|
||
|
||
Rationale, as with the whole hook program: make the process rule a derivation/hook, not prose to
|
||
remember (#303 methodology review). Tracked: #311; sibling #312 (H12 issue-qualification audit).
|
||
|
||
## 2026-07-12 — Merge-consent gate auto-grants when satisfied (no redundant prompt); state IS the consent (#314)
|
||
|
||
Completes the #303 H6/H10 intent — *derive merge-consent from state* — which the original hook only
|
||
half-delivered. The rule the user set: **merge permission is auto-granted for the session when the
|
||
linked issue's `## Done-when` boxes are all ticked, a fresh positive `Review-verdict` references the
|
||
current head, and CI is green** — no separate confirmation, conversational or mechanical.
|
||
|
||
**Root cause of the bug this fixes:** `pretooluse-merge-consent.sh`'s satisfied path did a bare
|
||
`exit 0`. A PreToolUse hook that exits 0 with no JSON does **not** auto-approve — it only declines to
|
||
block, so control falls through to the normal permission system and the raw MCP permission prompt
|
||
still fires (the merge tool isn't allow-listed). So the gate only ever *added* a deny/ask net; it never
|
||
*removed* the baseline prompt on the happy path. Net effect for the operator: a ready-to-merge PR was
|
||
confirmed twice — once conversationally (the per-session merge-consent norm) and again by a redundant
|
||
mechanical prompt the gate was supposed to have subsumed.
|
||
|
||
**Fix:** ONLY the genuinely-satisfied merge path (a+b+c all true) now emits
|
||
`{"hookSpecificOutput":{"permissionDecision":"allow", ...}}` (a new `grant` decision), which actually
|
||
suppresses the prompt. Deny (unticked/red/negative/stale) and ask (non-derivable: no creds, Gitea
|
||
down, no linked issue, no `## Done-when`, no verdict) are unchanged — the gate still fails closed, not
|
||
open. Two paths deliberately do **not** auto-grant and keep the bare `exit 0` **passthrough** (normal
|
||
permissioning → one prompt): non-merge `pull_request_write` methods (auto-grant is scoped to
|
||
method=merge only), and the **docs/process-only exemption**. The exemption is a file-TYPE bypass, not
|
||
the a+b+c "provably reviewed & ready" proof, so it must not *silently* self-merge — critically, its set
|
||
includes `.claude/`/`.gitea/`/`.husky/` (the gate, CI workflows, and git hooks themselves), so a PR
|
||
that weakens the gate still gets a human prompt (ersatztv#317 review nit). Verified by 8 pipe tests
|
||
(satisfied→allow, docs-only→passthrough, unticked→deny, stale→deny, red-CI→deny, no-verdict→ask,
|
||
no-creds→ask, non-merge→passthrough).
|
||
|
||
**Process consequence:** the state-derived gate *is* the consent on the satisfied path — do **not**
|
||
also ask conversationally to merge a PR whose gate auto-grants. A separate human confirmation is still
|
||
warranted only when the gate **asks** (state not derivable). This supersedes the "always confirm merge
|
||
consent in-conversation per session" phrasing in the kickoff HARD CONSTRAINTS (updated in the same PR).
|
||
|
||
## 2026-07-12 — Release path rehearses migrations on a prod-DB copy before promoting (#315)
|
||
|
||
The CI `migrations` job proves a migration is well-formed against a **fresh, empty** DB (model-drift +
|
||
apply-to-fresh, per provider). That is necessary but not sufficient: it never exercises the migration —
|
||
or ErsatzTV's startup data steps (`DatabaseMigratorService` → `DbInitializer` + `PopulatePathHashes`
|
||
over the real `MediaFile` table) — against the **accumulated prod SQLite**, where row volume and
|
||
historical values differ. A migration green on a fresh DB can still fail or corrupt on prod, discovered
|
||
only mid-deploy after the container recreates.
|
||
|
||
Decision: before promoting a migration-bearing release, **rehearse** the new image's migrations against
|
||
a **throwaway copy of the latest prod backup** via `scripts/migration-smoke.sh` — boot the new image
|
||
against the copy, gate PASS on the `Done applying database migrations` log line (the migrator is a
|
||
`BackgroundService` running concurrently with Kestrel, so HTTP-readiness alone does *not* prove
|
||
migrations finished), FAIL on early container exit / a migration exception / timeout / not serving
|
||
afterwards. Always operates on a copy, never the live DB. Home: the script + docs are ours; wiring it
|
||
into the Komodo **pre-deploy** step (which already produces the backup) is a server-management concern.
|
||
Rationale: data-plane rigor — catch a bad migration on a disposable copy, not on live prod data.
|
||
See `docs/ci-cd.md` → Migration-on-prod-copy smoke. Cross-repo wiring tracked in server-management.
|
||
---
|
||
|
||
## 2026-07-12 — #295 PR2: SPA session cutover + #301 side-effecting-GET POST-ification
|
||
|
||
PR1 shipped the server side (previous entry): `/api` accepts a session cookie OR the machine `X-Api-Key`, with
|
||
`X-CSRF` required on session-authenticated mutations. **PR2 is the SPA cutover** — the browser now authenticates
|
||
with the session only — plus **#301** (a session-cookie CSRF hole in side-effecting GETs).
|
||
|
||
**Browser is cookie-only; the machine key is external/MCP-only.** `web/src/api/client.ts` no longer attaches
|
||
`X-Api-Key`; it relies on the same-origin session cookie and sets `X-Csrf: '1'` on every mutating verb centrally.
|
||
The former "paste your key" `ApiKeyScreen` is repurposed to **machine-key management**: it reads the server key
|
||
from the new `GET /api/auth/machine-key` (session-gated; masked with Reveal + Copy) so an operator can hand it to
|
||
MCP / external REST clients — the browser itself never sends it again. *Why:* one credential per audience (the
|
||
ratified #295 model); leaving a browser key path alive would keep a CSRF-immune bypass around and defeat the
|
||
point.
|
||
|
||
**Boot gate, not a route** (`web/src/AuthGate.tsx`, wrapping `<App/>` in `main.tsx`): on load it calls the public
|
||
`GET /api/auth/config` then `GET /api/auth/session` and renders Setup (first-run local-admin claim) / Login
|
||
(local form + an OIDC "Sign in with SSO" button when `oidcEnabled`) / the app. Login and Setup mint **no URL** —
|
||
the gate renders them at whatever `/app/*` path was requested, so a deep link survives login for free and **no
|
||
`blazor-route-parity.md`/`domain-model.md` route rows are added**. It publishes `AuthContext`
|
||
(`{ username, method, signOut, requireLogin }`); the 401 signal (`notifyUnauthorized`) now drives re-login via a
|
||
passive shell banner (never yanks a dirty draft — it consults the navigation guard first). Auth flows that expect
|
||
a 401 inline (login, change-password) pass `suppressUnauthorizedSignal`.
|
||
|
||
**#301 — POST-ify, don't gate-the-GET.** A side-effecting GET is a CSRF vector once a `SameSite=Lax` cookie is a
|
||
normal credential (it rides a cross-site top-level navigation). The three offenders became mutating verbs so the
|
||
existing filter CSRF gate covers them with zero new machinery: `GET /api/troubleshoot/playback.m3u8` →
|
||
**`POST /api/troubleshoot/playback/start`** returning `200 { url }` (the open `/iptv` manifest the player then
|
||
loads — so hls.js/native-HLS needs no header injection, strictly better than X-CSRF-on-GET); the archive and
|
||
sample GETs → **POST** (SPA downloads them via a fetch-blob helper, never `window.open`). Removing the HEAD
|
||
variants also fixed a latent bug: a HEAD opened the `DeleteOnClose` stream and destroyed the artifact. Standing
|
||
rule added to `api-conventions.md §9`: **never add a side-effecting GET/HEAD under `/api`.**
|
||
|
||
**Machine-key GET discloses the key to any authenticated session** — deliberate: the session principal is the
|
||
single admin (local or OIDC), same-origin policy blocks a cross-site page from reading the response body, and it
|
||
is how the "copy the key for MCP" UX works without a rotation endpoint (rotation is a later PR). **Accepted
|
||
residual (OIDC logout):** `POST /api/auth/logout` ends the *app* cookie but not the IdP session, so an OIDC user
|
||
who clicks "Sign out" then "Sign in with SSO" returns without re-entering credentials — a `returnUrl`/RP-initiated
|
||
logout is a future nicety. Docs: `spa-conventions.md §5e` (SPA seams), `api-conventions.md §9`, `e2e-local.md`
|
||
(browser setup/login flow). Refs #295 #301 #197.
|
||
|
||
## 2026-07-12 — Enforcing CSP + Permissions-Policy on the host (#319, ZAP baseline)
|
||
|
||
Completes the CSP that the #279 baseline-headers entry deferred ("CSP must be validated against the ChicoryTV
|
||
SPA's inline assets"). Surfaced by the #314 out-of-ecosystem ZAP baseline (missing CSP/Permissions-Policy WARNs);
|
||
a **#197 exit item**. `SecurityHeadersMiddleware` now also sets `Permissions-Policy` (deny-all for
|
||
camera/microphone/geolocation/payment/usb) and an **enforcing** `Content-Security-Policy`.
|
||
|
||
- **Enforce, not report-only.** Report-only was the issue's acceptable fallback, but the SPA's asset graph is
|
||
small and fully knowable, so we ship an enforcing policy (report-only leaves the ZAP WARN and provides no real
|
||
protection). The policy: `default-src 'self'`; `script-src 'self' '<sha256 of the inline theme-bootstrap
|
||
script>'` (**no** `'unsafe-inline'`/`'unsafe-eval'` — the real XSS win); `style-src 'self' 'unsafe-inline'
|
||
https://fonts.googleapis.com`; `img-src 'self' data: blob:`; `font-src 'self' data: https://fonts.gstatic.com`;
|
||
`connect-src 'self'`; `object-src 'none'`; `base-uri 'self'`; `frame-ancestors 'none'`; `form-action 'self'`.
|
||
- **Why each relaxation.** The SPA is a static file, so a per-response nonce is impossible → the one inline
|
||
theme-bootstrap `<script>` is allow-listed **by hash**; `SecurityHeadersMiddlewareTests.Csp_Script_Hash_Should_
|
||
Match_The_Spa_Index` hashes the built `wwwroot/app/index.html` when present (else the committed `web/index.html`
|
||
source, since the built artifact is gitignored/absent in CI — Vite copies the inline script verbatim) and fails
|
||
if it drifts from the middleware constant. `style-src 'unsafe-inline'` covers
|
||
React's inline `style=""` attributes (no CSS-in-JS lib to hash). The **Google Fonts** hosts are required — the
|
||
SPA CSS `@import`s the Geist web font (caught by **live-E2E**, which the static grep missed); self-hosting the
|
||
font to drop the Google dependency is a follow-on hardening, not this issue. `img-src data: blob:` covers
|
||
favicon/generated-image data URIs and object-URL upload previews.
|
||
- **Scoped: `/docs` (Scalar) and `/openapi` are excluded.** The Scalar API-reference UI relies on inline bootstrap
|
||
scripts/styles a strict CSP would break; it keeps the baseline headers (nosniff/frame/referrer) but no CSP.
|
||
Hardening that admin surface (self-hosted Scalar or a Scalar-tuned CSP) is a #197 follow-up. Everything else —
|
||
SPA, `/api`, `/artwork`, `/iptv` — gets the CSP (non-HTML responses simply never exercise the script/style
|
||
directives). Verified by live-E2E (SPA renders clean, zero CSP violations) + curl (CSP present on `/app`/`/api`,
|
||
absent on `/docs`/`/openapi`). HSTS remains out (proxy/TLS decision). Refs #319 #314 #197.
|
||
|
||
## 2026-07-12 — If-Match evaluates per RFC 7232: valid-but-non-matching → 412, only grammar violations → 400 (#265)
|
||
|
||
Closing the last #253 concurrency-contract piece. `ConcurrencyHeaders.ParseIfMatch` previously classified
|
||
**any** non-canonical/weak/list `If-Match` value as `Malformed → 400` (a deliberate fail-safe: reject rather
|
||
than risk a stale write, deferred from the reference-aggregate PR). That was RFC-incorrect. Per **RFC 7232
|
||
§3.1**, a syntactically-valid entity-tag that simply doesn't strong-match must return **412 Precondition
|
||
Failed**, and **400** is reserved for a genuine grammar violation.
|
||
|
||
**What changed.** The parser is now a real RFC 7232 entity-tag/list parser (`If-Match = "*" / 1#entity-tag`).
|
||
It **scans** the list (it does *not* `Split(',')` — a comma is a valid `etagc`, so it can appear inside a quoted
|
||
opaque-tag: `"3,5"` is ONE tag, and a comma separates members only outside the quotes), trims only RFC OWS
|
||
(SP/HTAB — not `string.Trim()`, which would strip NBSP and let `" * "` masquerade as the `*` force-write),
|
||
validates each member as `[ "W/" ] DQUOTE *etagc DQUOTE`, and collects the versions of the **strong** members
|
||
whose opaque text is the exact canonical decimal we emit. Outcomes:
|
||
- **weak** (`W/"3"`), **empty** (`""`), **non-canonical** (`"03"`, `"3.0"`, `"+3"`), **out-of-range**
|
||
(`"99999999999999999999"`) → valid tags that contribute no version → **412** (a `Version`-kind with an
|
||
*empty* candidate set is a guaranteed no-match).
|
||
- **list** (`"3", "5"`) → any strong member that matches proceeds; weak/non-canonical members drop out.
|
||
- genuine grammar violations (unquoted `3`, SP inside the tag `" 3 "`, unterminated `"3`, `garbage`, a
|
||
separator-only header) → **400**.
|
||
|
||
**Type reshape.** `IfMatchCondition.ExpectedVersion : Option<int>` → `ExpectedVersions : Option<Seq<int>>`
|
||
(`None` = force-write; `Some(set)` = strong-match against the set, empty ⇒ always 412), and
|
||
`VersionedAggregateExtensions.CheckVersion(Option<int>)` → `CheckVersion(Option<Seq<int>>)` = set membership.
|
||
This threads through all 10 replace/update commands + handlers + request mappers + 9 controllers uniformly; no
|
||
wire-contract change (400 and 412 were already declared on every PUT; the field is header-derived and internal,
|
||
so no OpenAPI/DTO change).
|
||
|
||
*Why now, not #197:* it is the shared parser all replace-all PUTs copy, and the 412-vs-404 ordering the issue
|
||
worried about was already correct (each handler loads/validates → 404 before `CheckVersion`). *Why safe:* the
|
||
first-party SPA only ever echoes the single canonical strong tag we emit, so no shipped client changes behavior;
|
||
the change only makes a hand-written/tooling `If-Match` get the RFC-correct status. Docs: `api-conventions.md`
|
||
§7a. Refs #265 #253 #197.
|
||
|
||
## 2026-07-12 — TopBar primary-action button: wire creates, drop the rest (#238)
|
||
|
||
The shell TopBar rendered a prominent top-right primary-action button (Plus icon) for **every** screen, but
|
||
only `SchedulesScreen` had ever subscribed to its `ctv:primary-action` event — so on every other screen the
|
||
button was **dead** (either a labelled no-op like "Save Changes"/"Add Channel", or, for the ~10 routes whose
|
||
`primaryAction` was `''`, a bare labelless "+"; the button rendered unconditionally). Issue #238 (from a #229
|
||
live-E2E finding, pre-existing since the TopBar's introduction).
|
||
|
||
**Decision — the Plus-icon button is a "create new item" affordance; keep+wire it only where that fits:**
|
||
|
||
- **TopBar renders the button only when the active route declares a non-empty `primaryAction`** (was:
|
||
unconditional). This alone removes every empty-`''` dead "+".
|
||
- **WIRE** (8 list screens with a single unambiguous create flow) via a shared `usePrimaryAction(routeId,
|
||
handler)` hook (`web/src/primaryAction.ts`), each delegating to the same top-level create handler its in-body
|
||
control uses (`navigateToPath('/app/new-channel')`, `setEditing({kind:'new'})`, `navigateToPath('…/add')`,
|
||
etc.): channels, schedules (refactored onto the hook), multiCollections, rerunCollections, traktLists,
|
||
fillerPresets, ffmpegProfiles, watermarks.
|
||
- **DROP** (`primaryAction: ''`, no button) everywhere else, for one of four reasons: (1) the action isn't a
|
||
create so the "+" is wrong and a correct in-body control already exists — editChannel (savebar), settings
|
||
(savebar), guide (Jump to now), playouts (Reset all), logs/troubleshooting/blockPlayoutTroubleshooting
|
||
(Refresh), playbackTroubleshooting (Play), yamlValidator (Validate); (2) ambiguous — collections (two create
|
||
types behind tabs); (3) a silent no-op — builder ("Create Channel" is disabled until the form is valid),
|
||
playlists (needs a group first); (4) semantically misplaced — dashboard (status page), libraries ("Scan" is
|
||
per-library-row, no global target).
|
||
|
||
**Why not wire everything** (the SchedulesScreen precedent): the recon found every screen already carries a
|
||
correct, disabled-state-aware, context-aware in-body control, and several banner actions are unreachable
|
||
without refactoring handlers out from under early returns, or would render a silent no-op — reintroducing the
|
||
very dead-button class #238 fixes. Wiring only the single-create-flow screens gives one explainable rule
|
||
("primary button = create a new item on a list screen") and needs no risky refactors.
|
||
|
||
**Also fixed here:** the `apiKey` route's `primaryAction: 'Save key'` + its description/comment were stale
|
||
post-#295 (the screen displays the machine key + changes the local password; there is nothing to "save") —
|
||
relabelled to reflect the #295 reality. Convention + hook documented in `spa-conventions.md` §10. The
|
||
implicit route-label ⟺ screen-subscription coupling is the kind of thing #247 (shell extraction) will
|
||
formalize; #238 keeps it a documented convention guarded by a data-driven `App.test.tsx` test over the URL-navigating
|
||
create screens (a typo'd route id → the banner navigates nowhere → red). Refs #238 #247.
|
||
## 2026-07-13 — API versioning: the whole `/api` surface is mounted at `/api/v1`, additive-only after freeze (#286)
|
||
|
||
The #197 cold review's C1 **BLOCKER**: `/api/*` was entirely unversioned (`info.version` was cosmetic), so the
|
||
first breaking change would silently break the SPA and any external/MCP client with no negotiation path. This is
|
||
the Phase-2 contract-freeze gate — versioning can't be added compatibly *after* the contract ossifies, so it
|
||
lands before freeze.
|
||
|
||
**What changed.** Every route under `/api` was swept to `/api/v1` — all 251 controller route attributes, the ~24
|
||
`Location`-header literals, the scanner callback URL (`CallLibraryScannerHandler.GetBaseUrl`), and the
|
||
`Startup` request-log path literal. This is **uniform**: the machine JSON API, the browser-session auth surface
|
||
(`/api/v1/auth/*`, still `IgnoreApi`), the internal loopback callbacks (`/api/v1/scan/*`) and the scripted-build
|
||
surface (`/api/v1/scripted/*`) are all versioned, so there is no unversioned corner and the compat rewrite needs
|
||
no exclusion list. The OpenAPI `v1.json` (160 paths), `endpoint-index.md`, and the SPA (945 request literals +
|
||
its test mocks, incl. regex/positional URL parsers) were regenerated/swept in lockstep. **No wire-DTO or
|
||
status-code change** — only the path prefix moved.
|
||
|
||
**Legacy compat = an in-pipeline rewrite, NOT a redirect** (`ApiVersionRewriteMiddleware`, sequenced before
|
||
`UseRouting` in the API branch). A legacy caller hitting an unversioned `/api/foo` has its request *path*
|
||
rewritten to `/api/v1/foo` and continues in-pipeline — method, body, auth headers and query string all survive,
|
||
so curl / the future MCP server / bookmarked URLs keep working with no round-trip (a 307/308 redirect would have
|
||
been fragile for non-GET + custom-header clients). Rewritten (legacy) responses carry RFC 8594 `Deprecation: true`
|
||
+ `Link: </docs>; rel="deprecation"`, and a `Sunset` header when `Api:LegacyRoutesSunset` is configured. An
|
||
already-versioned path (`/api/v1/*`) passes through untouched; a future `/api/v2/*` is **not** forced back to v1
|
||
(the middleware only fills in a *missing* version).
|
||
|
||
**Freeze semantics (owner decisions):** once shipped, `/api/v1` is **additive-only** — new endpoints/optional
|
||
fields are fine; renaming/removing/retyping an existing one requires a new `/api/v2`, never an in-place break.
|
||
The legacy-rewrite compat shim has a **2-release sunset window** (owner-chosen) before removal; the actual removal
|
||
is a tracked Phase-3 follow-up, not this PR. Existing pre-freeze warts (e.g. channel `{id}` vs `{channelNumber}`,
|
||
the synthesized negative-id "(none)" group rows) are frozen as-is per their own prior decisions.
|
||
|
||
**Route-convention standardization (#286, owner-requested).** The leading-slash inconsistency (238 absolute
|
||
`"/api/…"` method routes vs 13 relative `"api/…"`) is resolved: the standard is a **leading-slash absolute route
|
||
on each method's `[Http*]` attribute, no class-level `[Route]`** — except the two controllers where many actions
|
||
share a parametrized prefix (`ScannerController` `{scanId}`, `ScriptedScheduleController` `{buildId}`, ~40
|
||
methods), which keep a leading-slash absolute **class** `[Route("/api/v1/…")]` with relative method segments (the
|
||
right tool for a shared prefix). Enforced by `ApiRouteVersioningTests`: it reflects over every `[ApiController]`
|
||
in `Controllers.Api`, computes each action's *effective* route (ASP.NET's class+method combination rule), and
|
||
asserts it matches `^/api/v\d+/` — so a new controller that drifts (relative or unversioned) fails CI, the
|
||
"fix-it-while-you're-in-the-file" gate the `dotnet format` rules use. Browser-nav endpoints deliberately outside
|
||
`/api` (e.g. `GET /auth/oidc/login`) are out of scope for the test. Docs: `api-conventions.md` §1/§9. Refs #286 #197.
|
||
|
||
## 2026-07-13 — Scheduling API hardening: null-name 500s, duplicate template items, unreachable 404 (#172)
|
||
|
||
Cleared the still-live findings from issue #172 (consolidated non-blocking nits from the #144 S1/S2
|
||
reviews). Most of the 2026-07-07 list had already been ratified deliberate (§8 "(none)" synthesized
|
||
rows; §3b deep-FK non-existence-check) or fixed since (the unauthenticated `/api/logs` +
|
||
`/api/troubleshoot/info` GETs now carry `[RequiresAuthentication]` per §9; the Trakt matched-items link
|
||
points at the live `/app/search`; `GET /api/search` already fans out via `Task.WhenAll`). Three were
|
||
genuinely live:
|
||
|
||
- **Null/empty `name` → 500 (10 handlers).** Create + Replace/Update handlers for Block, Template,
|
||
DecoTemplate, Deco (8, all genuine 500s), plus `UpdateFFmpegProfile` (genuine 500; `CreateFFmpegProfile`
|
||
was already guarded) and `CreatePlaylist` (its DTO coalesces `null`→`""`, so an empty-name persist, not a
|
||
500) all did `if (request.Name.Length > 50)` on a client-nullable `string Name` → unhandled
|
||
`NullReferenceException`. Fixed to `if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length >
|
||
50)` — kills the NRE, and also rejects empty/whitespace names (matching the group-create handlers'
|
||
`NotEmpty` behavior, closing a latent "block/template named ''" gap). Chose the one-line guard over
|
||
refactoring each handler onto the `NotEmpty`/`NotLongerThan` combinator to keep the blast radius tiny and
|
||
preserve each handler's existing error message + 422 mapping. Convention captured in `api-conventions.md`
|
||
§3b handler-hardening checklist.
|
||
- **Exact-duplicate template items bypassed overlap validation.** `ReplaceTemplateItemsHandler`'s O(n²)
|
||
overlap loop skipped on `item == otherItem`, but `BlockTemplateItem` is a `record`, so two value-identical
|
||
items (same BlockId + StartTime → same computed EndTime) were value-equal and skipped — both persisted
|
||
unvalidated. Switched to index-based iteration (`i != j`) so identical items at distinct positions are
|
||
compared and register as a (self-)intersection → rejected 422. (The SPA's index-based check already caught
|
||
this client-side; it was an API-only gap.)
|
||
- **Unreachable 404 on create-group actions.** `POST /api/blocks/groups` and `POST /api/templates/groups`
|
||
declared `[ProducesResponseType(ProblemDetails, 404)]` copied from precedent, but a create has no parent
|
||
lookup that can 404 (only 201/422). Trimmed — OpenAPI spec regenerated.
|
||
|
||
Deliberately **not** fixed (documented as accepted): the §8 "(none)" synthetic rows, the §3b deep-FK
|
||
non-existence-check, and the missing `Name=` on `PlayoutController` Create/Delete/Update (moot — the
|
||
"v1"-doc `OperationIdOpenApiTransformer` (#197 Bundle C) already synthesizes stable operationIds for
|
||
`Name=`-less ops, and adding `Name=` would risk renaming generated SPA client methods). Refs #172 #197.
|
||
|
||
## 2026-07-13 — Release promotion: floating `:prod`, exact-image scan before manual deploy (#335)
|
||
|
||
Prod keeps the floating `:prod` image reference; PR #191's workflow-driven immutable pin bump is
|
||
closed as superseded. server-management#585 proved that automatic and manual promotions share
|
||
`DeployStack` and made a changed `:prod` digest trigger the fail-closed backup; #589 added the
|
||
prod-copy migration smoke before live recreation. Tagging and promotion remain separate: scan the
|
||
tag build's immutable `:<version>` image, then deploy manually. Daily auto-update is only a fallback,
|
||
so cut tags with enough runway before 03:00 to prevent an unscanned promotion. Refs #335 and
|
||
server-management#585/#589.
|
||
|
||
## 2026-07-13 — Cross-origin resource policy: `same-origin` on every response (#330)
|
||
|
||
The authenticated #314 ZAP scan found that ErsatzTV's baseline response posture omitted
|
||
`Cross-Origin-Resource-Policy`. `SecurityHeadersMiddleware` now sends
|
||
`Cross-Origin-Resource-Policy: same-origin` on every response, including `/docs` and `/openapi`.
|
||
Those two paths remain exempt only from the strict CSP that would break Scalar's inline bootstrap;
|
||
CORP has no equivalent rendering conflict and belongs with the middleware's path-independent baseline
|
||
headers.
|
||
|
||
`same-origin` requires the browser request and response to share the exact scheme, host, and port. It
|
||
blocks cross-origin `no-cors` loads, so direct browser embedding of ErsatzTV artwork or media from an
|
||
alternate origin is deliberately unsupported. It does not reject an allowed CORS-mode API fetch, so the
|
||
explicit `Api:CorsAllowedOrigins` machine-client path continues to work. It is also not enforced by
|
||
server-side HTTP clients, so Jellyfin's `/iptv/*` requests are unaffected; same-origin SPA artwork and
|
||
IPTV requests remain allowed. This is defense in depth for browser embedding and does not replace CORS
|
||
or authentication. Refs #330 #319 #314.
|
||
|
||
## 2026-07-14 — Playouts screen extraction (#245): screen-owned route wrapper (epic #243 phase 2)
|
||
|
||
Second bounded extraction under the App.tsx modularization epic (#243): the Playouts domain moved from
|
||
`web/src/App.tsx` into `web/src/screens/PlayoutsScreen.tsx`, including its loading/error/empty states,
|
||
dialogs, mutations, timeline/filter helpers, and the existing `PlayoutsRouteScreen`. The dedicated
|
||
`PlayoutScheduleEditors.tsx` modules remain separate. This is a pure structural move: no API, route, CSS,
|
||
or runtime behavior changed; `App.tsx` retains only the import and `<PlayoutsRouteScreen />` dispatch.
|
||
|
||
**The unguarded route wrapper moves with the screen.** Playouts owns two sibling sub-path editors and,
|
||
per `spa-conventions.md` §2, must keep its local pathname state plus `popstate` listener because App's
|
||
allow-sub-path route object is reference-stable. Colocating the wrapper keeps that screen-specific route
|
||
ownership beside the base screen while App-level tests retain the cross-route navigation assertions.
|
||
|
||
**Temporal mutation behavior stays verbatim.** `mutatingRef`, `runMutation`, and the explicit
|
||
`query.refresh()` after a 409 moved as one unit. The extraction deliberately does not add mount/current
|
||
guards to these pre-existing promise completions; changing those semantics belongs to a separate issue.
|
||
Detailed behavior tests now render `PlayoutsScreen` directly with a scoped fetch mock, including the
|
||
zero-playout Add Playout affordance, lock/409 handling, refresh/poll ownership, action and kind gates, and
|
||
dialog flows. Refs #245 #243.
|