diff --git a/docs/superpowers/specs/2026-07-13-cleanup-batch-design.md b/docs/superpowers/specs/2026-07-13-cleanup-batch-design.md new file mode 100644 index 0000000..bda8373 --- /dev/null +++ b/docs/superpowers/specs/2026-07-13-cleanup-batch-design.md @@ -0,0 +1,213 @@ +# Cleanup batch — Tier A (loose ends) + Tier B (design tradeoffs) + +**Date:** 2026-07-13 +**Status:** design approved, pending spec review +**Branch:** `cleanup/tier-a-b` + +## Goal + +Close the deferred backlog down to zero *fixes* so the codebase has no known loose +ends before we brainstorm net-new features (Tier C). This batch covers the clear +hardening/polish items (Tier A) and the two benign-but-real design tradeoffs the +user chose to fix properly (Tier B). Tier C (Last.fm source, crash-resume +mid-pipeline, per-track duration matching, UI error states) is explicitly **out of +scope** here — it goes to the feature brainstorm next. + +Non-goals: no behavior change to the acquisition pipeline, no new user-facing +features, no schema change beyond the two additive tables below. + +--- + +## Tier A — loose ends (web, mechanical) + +### A1. PATCH allowlist hardening +Both `web/src/app/api/monitor/config/route.ts` and +`web/src/app/api/discover/config/route.ts` PATCH handlers currently gate writes on +`k in DEFAULTS`, which walks the prototype chain. The DELETE `/api/config` handler +was already hardened to `Object.prototype.hasOwnProperty.call(DEFAULTS, k)`; apply +the same own-property check to both PATCH routes. This only ever prevented writing +harmless junk `Config` rows (neither PATCH deletes), so this is consistency +hardening, not a live vulnerability — but it removes the last prototype-chain gap. + +### A2. a11y + copy +- Add an `aria-label` to the Monitor tab `enabled` toggle (it currently has none; + every other control on the page is labelled and web tests key off labels). +- Update the Settings page eyebrow at `web/src/app/settings/settings-form.tsx:159` + from `"Credentials · library · discovery"` to cover all five tabs, e.g. + `"Credentials · library · monitor · discovery"`. + +### A3. DELETE allowlist test +Add a `?field=constructor` case to the DELETE `/api/config` allowlist test +(`web/src/app/api/config/route.test.ts`) alongside the existing `__proto__` case, so +the own-property guard is covered against both prototype-chain keys. Structural fix +already in place; this is belt-and-suspenders coverage. + +--- + +## Tier B1 — discovery scoring: per-seed contributions table (worker + migration) + +### Problem +`_upsert_artist` / `_upsert_album` upsert with `score = EXCLUDED.score` (**replace**), +but the intended semantic is *score = sum of every seed's contribution*. Since +`discover.maxSeeds` was retired and chunking is now the default, a single sweep +partitions its seeds across many worker iterations. A candidate surfaced by seeds in +different chunks has its score **overwritten** by the last chunk instead of summed +(item 4), and the same replace-not-accumulate breaks across throttled sweeps +(item 5). The score a suggestion shows reflects only its most-recent chunk, not the +whole library's affinity. + +### Design +Introduce a normalized contributions table so a suggestion's score is a derived +aggregate, independent of chunk and sweep boundaries. + +```prisma +model DiscoverySeedContribution { + id String @id @default(cuid()) + candidateMbid String + candidateName String + seedMbid String + score Float + sources String[] + updatedAt DateTime @updatedAt + + @@unique([candidateMbid, seedMbid]) + @@index([candidateMbid]) + @@index([seedMbid]) +} +``` + +`run_discovery`, per seed **S** processed in the chunk: + +1. `affected := SELECT DISTINCT "candidateMbid" WHERE "seedMbid" = S` — the + candidates S *used* to contribute to (so drop-offs are recomputed too). +2. `DELETE FROM "DiscoverySeedContribution" WHERE "seedMbid" = S`. +3. Build a per-seed aggregate over the live sources: for each similar candidate + (skipping already-followed mbids), sum score across sources and union source + names. `INSERT` one contribution row per (candidate, S). +4. `affected |= {candidate mbids newly inserted for S}`. + +After the chunk's seeds are processed, recompute every candidate in the union of all +`affected` sets: + +- `SELECT SUM(score), COUNT(*), , any candidateName + FROM "DiscoverySeedContribution" WHERE "candidateMbid" = ANY(affected) + GROUP BY "candidateMbid"`. + `COUNT(*)` is the seedCount (unique on `(candidate, seed)` → one row per seed). +- For each with `score >= min_score`, upsert `DiscoverySuggestion` exactly as today, + keeping the `WHERE status = 'pending'` guard (dismissed/wanted rows untouched) and + the rowcount-drives-album-derivation logic. Album suggestions inherit the + recomputed artist aggregate. + +### Properties +- Score is now **exactly** the sum of all seeds' latest contributions — no chunk + artifact, no partial-sweep reset. A sweep triggered by one newly-followed artist + updates only that seed's rows and leaves every other seed's contribution intact. +- Deleting-then-reinserting a seed's rows self-heals stale entries: if S stops + finding a candidate, S's row for it vanishes and the candidate is recomputed + (seedCount drops) because it was in `affected`. +- A candidate whose contributions fall to zero: its pending suggestion is **left as + is** (not auto-retracted), consistent with today's semantics — suggestions persist + until dismissed/actioned. Documented, not a bug. + +### Migration & cleanup +- Additive table only; no data backfill (existing suggestions keep their current + score and are corrected on the next sweep that touches their seeds). +- Consider a one-time note: existing `DiscoverySuggestion` scores stay stale until + re-swept — acceptable for an off-by-default feed. + +--- + +## Tier B2 — scan worklist table (worker + migration) + +### Problem +`scan_chunk` re-walks the whole `/music` tree every chunk and skips entries sorting +`<= cursor` — `O(N²/C)` directory stat ops over a full scan. The scan is +MB-rate-limited (~1 req/s), so this is ~2% overhead, but the only true `O(N)` fix is +a persisted worklist (a JSON-blob-in-Config would just move the cost to re-parsing +the blob each chunk). + +### Design +Persist the album worklist once at scan start and pop it by indexed cursor. + +```prisma +model ScanWorkItem { + id String @id @default(cuid()) + scanId String + artist String + album String + path String + done Boolean @default(false) + + @@unique([scanId, path]) + @@index([scanId, done, artist, album]) +} +``` + +`scan.py`: +- `scan_build_worklist(conn, scan_id, dest_root)` — walk once via the existing + `_iter_album_entries`, `INSERT` a row per album folder (`ON CONFLICT (scanId, path) + DO NOTHING`, so a rebuild is idempotent). Store `artist`, `album` (= album folder), + `path` so the chunk needs no re-derivation. +- `scan_chunk(conn, resolver, probe, browser, scan_id, limit)` — `SELECT` the next + `limit` rows `WHERE scanId = %s AND NOT done ORDER BY artist, album`, process each + via the existing `_process_album`, mark `done = true`. Returns + `(imported, skipped, done)` where `done` is True when no undone rows remain. +- `scan_library(conn, resolver, probe, browser, dest_root)` (test-only unbounded + path) — reimplemented on the worklist: build with a fixed scan id, drain fully, + delete that scan's rows, return `ScanResult`. Keeps existing tests meaningful. + +`main.maybe_run_scan`: +- On start (`scan.requested` and not in progress): set `scan.inProgress=true`, + `scan.requested=false`, `scan.progress="0/0"`, generate a `scan_id` + (uuid), store it in `scan.id`, and `scan_build_worklist`. +- Each iteration: read `scan.id`, pop one chunk, accumulate imported/skipped in + `scan.progress`. +- On drain: write `scan.result` (unchanged `"imported X, skipped Y"` format), clear + `scan.progress`/`scan.id`, set `scan.inProgress=false`, and + `DELETE FROM "ScanWorkItem" WHERE scanId = %s`. Preserve the existing + "rescan requested mid-scan triggers one more full pass" guarantee. +- `scan.cursor` is retired (worklist replaces the string cursor). + +### Properties +- `O(log N + C)` per chunk via the `(scanId, done, artist, album)` index; one tree + walk per scan total. +- Robust to folders appearing/disappearing mid-scan: the worklist is frozen at scan + start, so the set processed is deterministic. +- Resumable/idempotent: `done` flag + `ON CONFLICT` inserts + `ON CONFLICT` record + writes mean an interrupted worker resumes cleanly. + +--- + +## Migration + +One additive Prisma migration adds both tables (`DiscoverySeedContribution`, +`ScanWorkItem`). No column changes to existing tables, no backfill. Reaches live +`lyra` via the web entrypoint's `migrate deploy` on rebuild. + +## Testing + +- **Worker (`pytest`, against `lyra_test`):** + - Contribution accumulation: a candidate surfaced by seeds split across two + simulated chunks sums to the same score as a single-chunk sweep; a partial sweep + (one seed) preserves other seeds' contributions; a seed dropping a candidate + lowers its seedCount and recomputes the suggestion. + - Scan worklist: build → pop chunks → drain marks all done; resume after a partial + chunk; idempotent rebuild; `scan_library` unbounded path still returns the same + `ScanResult`. +- **Web (`vitest`):** + - PATCH `/api/monitor/config` and `/api/discover/config` reject a + prototype-chain key (`__proto__`, `constructor`) — no `Config` write. + - DELETE `/api/config` `?field=constructor` rejected (added to existing test). +- Full suites + `tsc` + `next build` green before merge. + +## Delivery + +Branch `cleanup/tier-a-b` → writing-plans → subagent-driven build (per-task review + +Opus whole-branch review) → ff-merge to `main` → `git push` → +`docker compose up -d --build web worker` (runs `migrate deploy`) → verify live +(web 200, worker healthy, migration applied). Then move to the Tier C feature +brainstorm. + +## Out of scope (→ Tier C brainstorm) +Last.fm second `SimilaritySource`, crash-resume mid-pipeline, per-track duration +matching, UI error states on mutations.