From 75bae2556c35a16d3f29dcfecb3bb4fb72091263 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Thu, 30 Jul 2026 10:29:54 +0200 Subject: [PATCH] =?UTF-8?q?docs(specs):=20design-principles=20audit=20?= =?UTF-8?q?=E2=80=94=20five=20remediation=20specs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit of the principles in CLAUDE.md and docs/architecture/ against the actual code. Principles with a working automated check (poison-tolerant locking, Android source sync, one-directional playback state, graceful backend init, reachability-from-traffic) all held up. The two that drifted are exactly the two whose checks were broken or too narrow: - traceability-gate-repair: CI divided by hardcoded denominators (UR/39, IR/24, DR/48, JA/3, total 114) while requirements.md had grown to 211, reporting 158% coverage — the 50% threshold was unreachable and the job could not fail. - req-coverage-script-removal: check-req-coverage.sh reports "1 requirement" and prints "all requirements have implementations". - scoped-search-boundary-implementation: the founding boundary incident was specced but never built; the leak is still live. - boundary-tripwire-hardening: check:boundary passes on that same leak — the pattern is anchored to the query site, so a named const evades it. - player-facade-enforcement: 52 direct commands.player* call sites outside the facade, and no automated check at all. Each spec follows SPEC-TEMPLATE.md with a filled-in Layer assignment table and is checked against SPEC-REVIEW-CHECKLIST.md. --- docs/specs/boundary-tripwire-hardening.md | 215 +++++++++++++++ docs/specs/player-facade-enforcement.md | 258 ++++++++++++++++++ docs/specs/req-coverage-script-removal.md | 161 +++++++++++ .../scoped-search-boundary-implementation.md | 254 +++++++++++++++++ docs/specs/traceability-gate-repair.md | 238 ++++++++++++++++ 5 files changed, 1126 insertions(+) create mode 100644 docs/specs/boundary-tripwire-hardening.md create mode 100644 docs/specs/player-facade-enforcement.md create mode 100644 docs/specs/req-coverage-script-removal.md create mode 100644 docs/specs/scoped-search-boundary-implementation.md create mode 100644 docs/specs/traceability-gate-repair.md diff --git a/docs/specs/boundary-tripwire-hardening.md b/docs/specs/boundary-tripwire-hardening.md new file mode 100644 index 00000000..78be5e01 --- /dev/null +++ b/docs/specs/boundary-tripwire-hardening.md @@ -0,0 +1,215 @@ +# Spec: Harden the frontend boundary tripwire + +**Status:** Implemented +**Requirements:** DR-094 +**UX spec:** n/a — developer tooling. +**Supersedes / revises:** revises the detection rule in +[scripts/check-frontend-boundary.sh](../../scripts/check-frontend-boundary.sh); +the boundary *policy* in [scoped-search-boundary.md](scoped-search-boundary.md) +is unchanged. + +## Summary + +`bun run check:boundary` passes on a tree that contains the exact leak it was +built to catch. It matches a multi-type array only when written **inline at the +query site**, so assigning the same array to a named const evades it entirely — +which is how [searchScope.ts](../../src/lib/utils/searchScope.ts) has kept a +category→item-type mapping through every green CI run. This spec broadens the +match to item-type array literals anywhere in `src/`, and resolves the handful +of legitimate hits that broadening surfaces. + +## Motivation + +The current pattern is anchored to the `includeItemTypes:` key: + +```sh +PATTERN='includeItemTypes:[[:space:]]*\[[^]]*,[^]]*\]' +``` + +The live leak is not written that way: + +```ts +// src/lib/utils/searchScope.ts:29 — invisible to the tripwire +const SCOPE_ITEM_TYPES = { music: ["MusicAlbum", "MusicArtist", "Audio", "Playlist"], … }; +``` + +The taxonomy and the query are one indirection apart, and the grep only sees the +query. The script's own header is admirably honest that it is "a TRIPWIRE, NOT A +PROOF" — but the gap here is not a subtle judgment call it was designed to +defer to human review. It is the *crudest form* of the violation, one `const` +away from the shape it does match, in the very file the founding incident was +written about. + +Broadening the pattern to any item-type array literal finds it, with a +manageable number of other hits (measured, not estimated): + +| Site | Verdict | +|------|---------| +| `searchScope.ts:30,32` | 🔴 The leak. Removed by [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md). | +| `PersonDetailView.svelte:30` | Already allowlisted, with a recorded reason. | +| `DownloadedBrowse.svelte:95` | Borderline — `["MusicAlbum","Series","Season","BoxSet"].includes(item.type)` as an "is this a container?" predicate. | +| `GenericMediaListPage.svelte:298` | Borderline — `["MusicAlbum","MusicArtist","Audio","Playlist"].includes(config.itemType)` as a music-styling predicate. | +| 6 hits in `*.test.ts` | Excluded; tests legitimately name types. | + +Four non-test sites total. This is a tractable change, not a boil-the-ocean one. + +## Layer assignment + +Tooling only — no application logic, nothing crosses IPC. The two borderline +*application* sites do get a layer decision, below. + +| Logic / responsibility | Layer | Why it belongs there | +|------------------------|-------|----------------------| +| Detecting item-type array literals in `src/` | Build tooling (`scripts/`) | Static analysis of repo source; belongs beside the existing check. | +| "Is this item a container?" (`DownloadedBrowse`) | **Rust** (recommended) | Containers-vs-leaves is Jellyfin structure, and the set grows when Jellyfin adds a container type — the litmus test's "yes". Prefer a `MediaItem.isContainer` boolean from the backend over a type-set predicate in a component. | +| "Is this music content?" (`GenericMediaListPage`) | **Frontend, allowlisted** | Selects a grid *style*. It reads `config.itemType`, a value the page already declares about itself, and changes only if the UI is redesigned — the litmus test's "no". Single-type presentation is explicitly not the target of the rule. | + +`DownloadedBrowse` defaults to Rust per the checklist's borderline rule; see +Out of scope for why the migration itself is deferred rather than bundled. + +## Design + +### 1. Broaden the pattern + +Replace the key-anchored pattern with one matching an array literal of two or +more known Jellyfin item types, wherever it appears: + +```sh +# Two or more adjacent item-type string literals inside a bracket. +TYPES='Movie|Series|Episode|Audio|MusicAlbum|MusicArtist|MusicVideo|Season|BoxSet|Playlist|Book|AudioBook|Video|Person|Folder|CollectionFolder|TvChannel|LiveTvChannel' +PATTERN="\[[[:space:]]*\"($TYPES)\"[[:space:]]*,[[:space:]]*\"($TYPES)\"" +``` + +Properties worth stating, because each is a deliberate trade: + +- **Not anchored to any key**, so a named const, a function return, a `Record` + value, or an inline query all match equally. +- **Requires two adjacent type literals**, preserving the existing and correct + carve-out that single-type presentation (`itemType: "Movie"`) is legitimate. +- **Requires string literals**, so `item.type === "Audio"` (display inspection) + still does not match. +- **Explicit type list**, not `[A-Z][a-z]+`, so arbitrary string arrays + (`["High","Low"]`, `["Songs","Albums"]`) do not produce noise. + +Keep `grep -rInE`, the `*.test.*` exclusion, and the allowlist mechanism as they +are — all three work. + +### 2. Resolve the surfaced sites + +- `PersonDetailView.svelte` — already allowlisted; entry unchanged. +- `GenericMediaListPage.svelte` — **add to the allowlist** with the reason from + the layer table (grid styling over a self-declared `itemType`). +- `DownloadedBrowse.svelte` — **add to the allowlist with a `TODO` naming the + preferred fix** (backend `isContainer`). An allowlist entry that records a + known-borderline decision is honest; silently broadening the pattern to miss + it would not be. +- `searchScope.ts` — **not allowlisted.** It is the leak, and + [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md) + deletes it. + +### 3. 🔴 Sequencing + +**This spec must land after Stage 1 of +[scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md).** +Hardening the tripwire first turns `master` red on a violation with no fix +available, and the only ways out are reverting the hardening or allowlisting the +leak — the second of which is exactly how a boundary rule dies. + +### 4. Keep the allowlist honest + +The script already warns that a growing allowlist means the boundary is eroding. +This change takes it from 1 entry to 3, which is close to that line. Add a hard +cap so drift is caught mechanically rather than by whoever notices: + +```sh +MAX_ALLOWLIST=4 +if [ "${#ALLOWLIST[@]}" -gt "$MAX_ALLOWLIST" ]; then + echo "❌ Allowlist has ${#ALLOWLIST[@]} entries (max $MAX_ALLOWLIST)." + echo " Push taxonomy into Rust instead of appending here." + exit 1 +fi +``` + +The cap is deliberately just above the current count: the next exception forces +a conversation instead of a one-line append. + +### 5. Restate the limits + +The header's "tripwire, not a proof" caveat stays and gets sharper. The broadened +pattern still cannot see: + +- a type set built at run time (`[...musicTypes, "Playlist"]`), +- types split across variables (`const A = "Audio"; [A, B]`), +- taxonomy expressed as a `switch` or chained `||` rather than an array. + +The spec-review checklist remains the real gate. This raises the floor; it does +not close the class. + +## Out of scope + +- **Migrating `DownloadedBrowse` to a backend `isContainer` flag.** It touches + `MediaItem`, `bindings.ts`, and the offline path — its own spec. Allowlisted + with a TODO here so it is recorded, not forgotten. +- The scoped-search fix itself — [scoped-search-boundary-implementation.md](scoped-search-boundary-implementation.md). +- Detecting the run-time-construction cases listed above. +- Extending the check to Rust or Kotlin (the rule is about `src/`). +- Changing the boundary *policy* in CLAUDE.md. + +## Acceptance criteria + +- [ ] With `searchScope.ts` reverted to its leaking form, `bun run check:boundary` + **fails** and names `src/lib/utils/searchScope.ts`. This is the criterion + that proves the fix — verify it explicitly before landing. +- [ ] On the post-fix tree, `bun run check:boundary` passes. +- [ ] A newly introduced `const X = ["Movie", "Series"]` in any non-test `src/` + file fails the check (regression test for the const-indirection evasion). +- [ ] `itemType: "Movie"` and `item.type === "Audio"` do **not** trip the check. +- [ ] `["High", "Low"]` and other non-item-type arrays do **not** trip it. +- [ ] Test files are still excluded (the 6 known test hits stay silent). +- [ ] The allowlist has exactly 3 entries, each with a written reason; a 5th + entry fails the check via `MAX_ALLOWLIST`. +- [ ] The script header still states it is a tripwire, not a proof, and names the + evasions it cannot see. +- [ ] `bun run check` and `bun run test` pass. +- [ ] `bun run test:all` passes. + +## Testing + +The script is bash and has no test harness. Verify by construction — each is a +temporary edit, run, revert: + +1. Reintroduce the `SCOPE_ITEM_TYPES` const → **must fail**. +2. Add `const T = ["Movie","Series"]` to a scratch `.svelte` file → **must fail**. +3. Add the same to a `.test.ts` file → **must pass** (exclusion holds). +4. Add `itemType: "Movie"` → **must pass**. +5. Add a 5th allowlist entry → **must fail** on the cap. + +Record the five results in the PR description. A grep-based gate that has never +been observed failing is indistinguishable from one that cannot fail — which is +the precise condition this whole spec exists to correct. + +## TRACES + +Allocate in `requirements.md`: + +- **DR-094** — "Frontend boundary tripwire detects Jellyfin item-type array + literals anywhere in `src/` (not only inline at an `includeItemTypes:` query + site), so a category→type mapping cannot evade the check via a named const; + allowlist is capped to force taxonomy into Rust rather than accumulating + exceptions." Category: Tooling. Status: Done on merge. + +Shell scripts carry no `TRACES:` comment convention in this repo; reference +DR-094 in the script header comment instead. + +## Notes for the implementer + +- A parallel Claude session may be active in this repo — `git diff` before + "repairing" unexpected changes (CLAUDE.md §Gotchas). +- **Land after Stage 1 of the scoped-search fix** — see §3. This is the one + ordering constraint that will break `master` if ignored. +- Test the regex against the current tree *before* committing: + `grep -rInE "$PATTERN" src/ | grep -v '\.test\.'` should return exactly the + four sites in the Motivation table. +- The `TYPES` list will need occasional extension as Jellyfin adds types. + That is acceptable for a tripwire — an unlisted type produces a false + negative, never a false positive, so the check degrades safely. diff --git a/docs/specs/player-facade-enforcement.md b/docs/specs/player-facade-enforcement.md new file mode 100644 index 00000000..a9ed6662 --- /dev/null +++ b/docs/specs/player-facade-enforcement.md @@ -0,0 +1,258 @@ +# Spec: Enforce the unified player boundary + +**Status:** Proposed +**Requirements:** DR-095 (new); relates to UR-005 and the unified-player-boundary +principle in CLAUDE.md and [02-svelte-frontend.md](../architecture/02-svelte-frontend.md) +**UX spec:** n/a — refactor, no user-visible change. +**Supersedes / revises:** n/a + +## Summary + +The stated principle is that UI controls playback **only** through +`playerController` ([src/lib/player/index.ts](../../src/lib/player/index.ts)), +never by calling `commands.player*` directly. There are **52 direct call sites +outside** that facade. This spec routes the genuine playback-control calls +through the facade, narrows the principle's wording so it stops forbidding +things it never meant to forbid, and adds the lint rule that keeps it true — +because this rule is the one design principle in the audit with **no automated +check at all**, and it is also the one that drifted furthest. + +## Motivation + +Direct `commands.player*` usage outside `src/lib/player/`, by file: + +| File | Sites | +|---|---| +| [queue.ts](../../src/lib/stores/queue.ts) | 10 | +| [player/[id]/+page.svelte](../../src/routes/player/[id]/+page.svelte) | 9 | +| [VideoPlayer.svelte](../../src/lib/components/player/VideoPlayer.svelte) | 8 | +| [settings/+page.svelte](../../src/routes/settings/+page.svelte) | 5 | +| [sleepTimer.ts](../../src/lib/stores/sleepTimer.ts) / [auth.ts](../../src/lib/stores/auth.ts) / [autoplay.ts](../../src/lib/api/autoplay.ts) | 4 each | +| [preload.ts](../../src/lib/services/preload.ts) | 3 | +| [library/[id]](../../src/routes/library/[id]/+page.svelte), [playerEvents.ts](../../src/lib/services/playerEvents.ts), [playbackMode.ts](../../src/lib/stores/playbackMode.ts) | 1–2 each | + +These are **not** equivalent violations, and treating them as one number is why +the rule has been easy to ignore. Three distinct groups: + +**(a) Genuine violations — playback control with a facade method that already +exists.** `playerStop` ×6, `playerPlayTracks` ×4, `playerSeek` ×2, +`playerPlayAlbumTrack` ×2, `playerNext`, `playerPrevious`, `playerSkipTo`, +`playerToggleShuffle`, `playerCycleRepeat`, `playerRemoveFromQueue`, +`playerMoveInQueue`, `playerAddTrackById`, `playerAddTracksByIds`, +`playerSetSubtitleTrack`, `playerPlayItem`. The facade exposes `stop()`, +`seek()`, `next()`, `previous()`, `skipTo()`, `toggleShuffle()`, +`cycleRepeat()`, `removeFromQueue()`, `moveInQueue()`, `addTrackById()`, +`addTracksByIds()`, `setSubtitleTrack()`, `playTracks()`, `playAlbumTrack()`, +`playItem()` — every one of these has a facade equivalent that is simply not +being called. `queue.ts` is the starkest case: it imports `commands` directly +and re-implements ten methods the facade already provides. + +**(b) Playback control with no facade method.** `playerPlayQueue`, +`playerGetQueue`, `playerGetStatus`, `playerEnterBackgroundAudio`, +`playerExitBackgroundAudio`, `playerSetSleepTimer`, `playerCancelSleepTimer`, +`playerPlayNextEpisode`, `playerCancelAutoplayCountdown`. In scope for the +principle, but currently *impossible* to comply with — the facade has no surface +for them. A rule that cannot be followed is not being broken so much as it is +unfinished. + +**(c) Not playback control.** `playerConfigureJellyfin` ×3, +`playerDisableJellyfin`, `playerGet/SetAudioSettings`, +`playerGet/SetVideoSettings`, `playerGetEqPresets`, +`playerGet/SetAutoplaySettings`, `playerGet/SetCacheConfig`, +`playerPreloadUpcoming`. These are configuration and lifecycle calls that happen +to live under the `player_` command prefix. The principle is about *who is +authoritative for playback state* — settings CRUD isn't that. + +The audit's read: the rule as written is violated 52 times, which makes real +drift indistinguishable from acceptable usage, and that ambiguity is what lets +group (a) persist. Note also that the principle **is** well-honoured where it +matters most — the read side is clean, with UI reading state exclusively from +the facade's re-exported stores. The write side is what drifted. + +## Layer assignment + +Frontend-internal refactor. No domain logic moves and nothing new crosses IPC — +the same Rust commands are called, through one module instead of many. + +| Logic / responsibility | Layer | Why it belongs there | +|------------------------|-------|----------------------| +| Playback command dispatch (adapter routing: native vs HTML5) | Frontend — `src/lib/player/` **only** | Presentation-layer plumbing, but must be centralised: the facade picks between the native backend and the HTML5 `