docs(specs): design-principles audit — five remediation specs
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.
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user