From 2ee9116d4dcae72582783441f71d7c0d4fcc1c39 Mon Sep 17 00:00:00 2001 From: kami Date: Thu, 30 Jul 2026 23:56:49 +0400 Subject: [PATCH] docs: add the 2026-07-30 engineering review and CLAUDE.md, drop AUDIT.md REVIEW-2026-07-30.md is the source for the preceding commits. AUDIT.md was its superseded predecessor. Co-Authored-By: Claude Opus 5 --- AUDIT.md | 37 ---- CLAUDE.md | 62 ++++++ REVIEW-2026-07-30.md | 454 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 516 insertions(+), 37 deletions(-) delete mode 100644 AUDIT.md create mode 100644 CLAUDE.md create mode 100644 REVIEW-2026-07-30.md diff --git a/AUDIT.md b/AUDIT.md deleted file mode 100644 index 34d21d3..0000000 --- a/AUDIT.md +++ /dev/null @@ -1,37 +0,0 @@ -# Muzick — audit journal - -## Goal -Audit the muzick project at `/home/kami/apps/muzick/`, file Vikunja tasks for findings, and work on autonomous items. - -## Progress - -### 2026-07-14 — Initial audit - -**Git**: initialized, initial state committed (737bf19). Fixes committed in 42474c6. - -**Project structure**: 196 files — TypeScript/Fastify backend, React/Vite frontend, BullMQ workers, PostgreSQL + Redis + Typesense. - -### Fixes applied -1. **AGENTS.md tech stack**: `python/fastapi` → `fastify/typescript` (was wrong) -2. **CORS**: Added `@fastify/cors` plugin to backend with env-based origin config -3. **`.env.example`**: Created with placeholder values (secrets were only in `.env` which is gitignored) -4. **`backend/src/index.ts`**: Removed dead code (empty file, `server.ts` is real entry point) - -### Tasks filed -- See Homelab infra project. Key items: - - #109: pin Docker images (NEEDS FIX — minio:latest etc) - - #110: N+1 queries in generators (PERFORMANCE) - - #111: image proxy SSRF guard (SECURITY) - -### New critical issues found -1. **No auth on any API** — `x-user-id` header with hardcoded fallback UUID is the only identity -2. **Admin routes unprotected** — anyone can trigger scan/reindex/delete -3. **Postgres password "password"** hardcoded in docker-compose.yml -4. **Typesense API key "muzick-key"** hardcoded in docker-compose.yml -5. **SOCKS proxy IP** `192.168.1.104` exposed in .env and AGENTS.md -6. **No tests for any worker service** (1311-line enrichment.service.ts has 0 tests) -7. **Frontend never typechecked in CI** -8. **No input validation** on many routes (admin, library — `as any` casts) - -### Remaining autonomous work items -- #109 — Pin Docker images in docker-compose.yml — CAN DO diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..ab90eb2 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,62 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## What this is + +muzick is a self-hosted music player + recommendation engine (deployed at `muzick.kvmx.ru:5174`). Three deployable units — `backend/` (Fastify API), `frontend/` (Vite React SPA), `workers/` (BullMQ job processor) — plus Postgres, Redis, and Typesense. All wired together by `docker-compose.yml`. + +## Commands + +Each unit is its own npm package; `cd` into it first. + +```bash +# backend/ and workers/ +npm run dev # tsx watch +npm run typecheck # tsc --noEmit (also runs as prebuild) +npm run build # tsc + +# backend/ only +npm test # vitest run +npm run test:watch +npx vitest run src/services/generators.test.ts # single file +npx vitest run -t "comfortGenerator" # single test by name + +# frontend/ +npm run dev # vite +npm run build # vite build +npm run typecheck + +# whole stack +docker-compose up -d --build +``` + +There is no lint step. `typecheck` is the gate; the `prebuild` hook fails the build on type errors. + +## Architecture + +**Split by process, sharing one Postgres database.** The backend serves the API and the frontend consumes it; the worker runs offline enrichment/analysis. They communicate only through Postgres (source of truth) and Redis (BullMQ queue). There is no shared code package — `workers/` and `backend/` each carry their own copy of things like `queue.ts` and pg clients. + +- **backend/src/app.ts** — the real entry point (`server.ts` just calls `buildApp`). Registers all routes, connects pg/redis/typesense, and runs several `setInterval` background jobs directly in-process: claim-fusion materialized-view refresh (10s), belief decay (hourly), forgotten-profile derivation (nightly). Auth is a single `onRequest` hook keyed on `MUZICK_API_KEY` / `MUZICK_ADMIN_KEY` (both optional — no keys means open); `/api/admin/*` requires the admin key specifically, `/api/health` is always exempt. +- **backend/src/services/** — business logic. `db.service.ts` owns schema application (`ensureSchema`/`runMigrations` run on every boot — the docker init-mount only fires on a fresh volume, so migrations live here). `session-director.service.ts` and `generators.service.ts` implement the recommendation/vibe logic. +- **workers/src/index.ts** — one BullMQ `Worker` with a `switch` on job name (scan_library, metadata refresh, audio analysis, artist similarity, image enrichment…). Also registers cron repeatables (integrity sweep, dislike cleanup, stale-session reaper). External metadata clients live in `workers/src/integrations/`. +- **frontend/src/** — TanStack Router + TanStack Query. `services/api.ts` is the axios base; per-domain service files wrap endpoints. Zustand stores in `store/` hold playback/vibe/toast state. `components/ethos/` is the shared Ethos design-system UI. + +### Domain concepts (from README + spec comments) + +- **Rolling Vibe** — continuous stream interleaving owned library tracks with "probation" external discoveries. Managed by the session-director. +- **Belief / claim fusion** — enrichment produces `claims` from multiple sources fused into a materialized view; beliefs decay over time. The in-process timers in `app.ts` keep this fresh. +- **Dislike lifecycle** — multi-stage state machine; the worker's cleanup sweep advances it. + +## Deployment gotchas (see AGENTS.md) + +- **Typesense is pinned to 0.25.1** — do not bump casually, the API breaks across majors. +- **Worker uses `network_mode: host` + a SOCKS5 proxy** (`SOCKS_PROXY_URL`) for all external metadata calls (Last.fm, Discogs, MusicBrainz, etc.). +- **DB schema** is `backend/src/db/schema.sql`, dropped into `docker-entrypoint-initdb.d` — only applied on a fresh volume. Schema changes for existing volumes must go through `db.service.ts` migrations. +- Music dir is a **read-only** bind from `/mnt/hdd1/media/Music` → `/music`. +- Redis host inside compose is `infra-redis` (external `infra-net` network); the host-mode worker reaches it at `127.0.0.1:6379`. +- An older `muzick.service` systemd unit runs the pre-docker backend on port 5213 and may conflict — docker compose is the active deployment. + +## Working docs + +`PLANS.md`, `AUDIT.md`, `progress.md`, and dated `SESSION-*.md` files track in-flight work and are not part of the running system. diff --git a/REVIEW-2026-07-30.md b/REVIEW-2026-07-30.md new file mode 100644 index 0000000..afa1c23 --- /dev/null +++ b/REVIEW-2026-07-30.md @@ -0,0 +1,454 @@ +# muzick — engineering review, 2026-07-30 + +Scope: full-project review (backend, workers, frontend, schema, deployment, docs) against +live runtime state. Every finding marked **confirmed** was verified directly against the +code, the live database, or a throwaway container — not inferred. + +Live context at time of review: containers up 8 days, 3,962 tracks, 13,910 claims, +843 beliefs, 68 plays in the preceding 7 days (last: 2026-07-29). Actively used. + +--- + +## verdict + +A genuinely good system that is quietly broken in ways its own dashboards cannot show. +The architecture is sound and proportionate to the problem. The failures are not design +failures — roughly six of the most interesting subsystems are dead or silently degrading +in production, and nothing surfaces that. + +**Repair. Do not rewrite.** + +## what the project is now + +A single-user, actively-used self-hosted music player with an ambitious belief/claim +recommendation engine. 17.2k LOC across three deployable units. Reachable only from +LAN/VPN — `/etc/nginx/sites-available/muzick.kvmx.ru` has +`allow 10.42.0.0/24; allow 192.168.1.0/24; deny all` and listens on LAN/VPN interfaces +only, not `0.0.0.0`. + +## what it should become + +The same system, with its ambitious half either **working or removed**, and with a +fresh-volume rebuild that actually works. At present it is a recommendation engine whose +learning loop is partly disconnected, with no way to notice. + +## classification + +`fragile` + `misaligned`, plus `overbuilt` in one specific place (System D diversity +budgets, discovery walk). + +Not stale, not abandoned, not better replaced. + +--- + +## first assessment + +| | | +|---|---| +| purpose | self-hosted music player + recommendation engine over a local library | +| intended users | single user (owner) | +| actual users | single user, actively — 68 plays in 7 days | +| critical workflows | browse/play library; Rolling Vibe session; enrichment; quarantine | +| current state | live and serving, with several subsystems silently inert | +| known failures | see findings 1–10 | +| maintenance burden | moderate; concentrated in 3 oversized files and absent verification | +| technical constraints | `/music` is a **read-only** bind; worker needs SOCKS5 for egress; Typesense pinned 0.25.1 | +| personal constraints | homelab, single operator, LAN/VPN-only exposure | +| what still works well | playback, library browsing, scanning, enrichment writes, migration registry, cron registration | +| what has become obsolete | discovery graph walk, System D budgets, `workers` shadow schema, dead frontend components | + +--- + +## main findings + +### 1. The project cannot be rebuilt from scratch — confirmed + +**Problem.** A fresh Postgres volume produces a permanently empty library. + +**Evidence.** Booted a throwaway `postgres:16-alpine` with the real +`backend/src/db/schema.sql` and ran the scanner's exact insert +(`workers/src/scanner.service.ts:241` — `INSERT INTO artists (name) VALUES ($1)`): + +``` +ERROR: null value in column "canonical_name" of relation "artists" + violates not-null constraint +``` + +`schema.sql:50` declares `canonical_name TEXT NOT NULL` with no default. The **live** +database has that column nullable — the volume predates the constraint. That is the only +reason anything currently works. + +**Impact.** Highest severity. Every artist insert fails on a fresh volume, and +`processFile` swallows the error per-file (`scanner.service.ts:199`), so a scan reports +**success with 0 tracks detected**. There is no disaster recovery, and `docker-compose up -d` +— the documented setup path in the README — silently yields an empty library. + +**Action.** Repair: insert `canonical_name` in the scanner, or give the column a default. +Then prove it with a scratch-volume rebuild. + +### 2. Every admin action in the UI returns 403 — confirmed + +**Problem.** The entire admin surface of the SPA has been non-functional since auth landed. + +**Evidence.** Confirmed at three layers: +- `frontend/nginx.conf.template:16` injects only `Authorization: Bearer ${MUZICK_API_KEY}` +- `docker-compose.yml` passes only `MUZICK_API_KEY` to the frontend container + (verified inside it: `API=set ADMIN=`) +- `backend/src/app.ts:108` requires `token === adminKey` for `/api/admin/*` +- the two keys differ (35 vs 41 chars); `frontend/src/services/api.ts` is 5 lines and + sets no headers at all + +**Impact.** All 10 admin call sites are dead: the 477-line Jobs page (polling 403s every +3s/5s forever, rendering a blank Overview with no error state) and every Settings library +action — Scan, Reindex, Reprocess artists, Re-enrich, Duplicates merge. Introduced by +commits `5ed8d9e` / `3bc9f2d` without updating the frontend path. + +**Action.** Repair: inject the admin key for `location /api/admin/` in the nginx template. +Honest context — the outer proxy already forges credentials for anything reaching +`location /api`, so behind a LAN-only proxy this key split buys nothing while costing the +whole admin surface. + +### 3. Belief decay has never once run — confirmed + +**Problem.** The temporal dimension of the recommendation engine is entirely inert. + +**Evidence.** `backend/src/services/db.service.ts:1788` builds a CTE with +`SELECT profile, CASE profile ...` and **no `FROM` clause**. Postgres rejects it: +`column "profile" does not exist` (SQLSTATE 42703), thrown hourly for 8+ days. + +Rewritten as `WITH halflives(profile, halflife_sec) AS (VALUES ...)` and run against the +live DB inside a transaction: **`UPDATE 843`**, then `ROLLBACK`. No data was changed. + +**Impact.** `obsession` (14-day half-life) and `contextual` (7-day) never fade, so old +fixations stay maximally weighted forever. Three of six spec'd profiles — `discovery`, +`contextual`, `forgotten` — have **zero rows**. + +**Action.** Repair (fix verified). Warning: the first successful run applies ~23 days of +accrued decay at once, cutting `obsession` beliefs to ~0.32×. That is correct behaviour, +but it will visibly change recommendations. + +### 4. Claim de-duplication is broken and is actively corrupting scores — confirmed + +**Problem.** Re-enrichment inserts duplicate claims instead of reinforcing them, inflating +fusion weights. + +**Evidence.** `schema.sql:343` declares `UNIQUE (..., source, user_id)`. Postgres treats +NULLs as distinct, and every objective claim has `user_id IS NULL` — so +`ON CONFLICT DO UPDATE` / `DO NOTHING` (`db.service.ts:1425`, +`workers/src/mb-spine-writer.ts:54`) **never fires**. Live data: + +``` +dup_groups: 236 | excess_rows: 2110 | worst single claim: 86 copies +``` + +`claim_fusion` is `SUM(trust * confidence * recency)`, so one edge can carry **86× its +intended weight**. ~15% of the 13,910 claims are re-enrichment duplicates. + +**Impact.** The recommendation graph is measurably skewed toward whatever was re-enriched +most — the most likely cause of repetitive recommendations. Grows with every re-enrich. +Almost certainly the root cause behind commit `d497588` ("claim_fusion MV duplicate-key +failure"). + +**Action.** Repair: `NULLS NOT DISTINCT` (PG15+; running 16) or store the zero-UUID, plus a +one-time dedup migration. **Dedup must run before the constraint is added.** + +### 5. SQL injection via column names — confirmed + +**Problem.** Request-body keys are interpolated into SQL as column identifiers. + +**Evidence.** `db.service.ts:1237`: + +```ts +const setClause = fields.map((f, i) => `"${f}" = $${i + 2}`).join(', '); +``` + +`fields = Object.keys(data)`, and `data` is `request.body as any` +(`library.routes.ts:176`). No allowlist. Same shape in `updateArtist` / `updateAlbum`. + +**Impact.** A crafted body key closes the quoted identifier and injects into the SET list; +also plain mass-assignment of `path`, `state`, `play_count`. Mitigated in practice only by +the LAN/VPN-only proxy — which *supplies the auth token automatically*, so any device on +the LAN can reach it from a browser. + +**Action.** Repair: allowlist columns. + +### 6. The dislike lifecycle is architecturally impossible as specified — confirmed + +**Problem.** The spec's terminal state cannot be reached in this deployment. + +**Evidence.** `docs/architecture/02-invariants-and-risks.md` §C makes hard file deletion the +final step. `/music` is mounted `:ro` in both services. `workers/src/cleanup.service.ts:72` +calls `unlink()` → `EROFS` → `continue`. Live state: **37 tracks stuck `WARNED` since +2026-07-03** (27 days), **296 EROFS errors in the last 24h**, and `feedback` contains zero +`deleted_permanent` rows. + +It fails *closed* — no DB row is deleted — which is the good outcome. But it will never +progress, and it is by far the loudest log source. + +Related, independent of the mount: `db.service.ts:909` inserts the `deleted_permanent` +audit row and *then* deletes the track, but `feedback.track_id` is `ON DELETE CASCADE` +(verified: `confdeltype = c`) — so that audit row destroys itself. + +**Action.** Requires a decision — see *open decisions* below. Fix the cascade regardless. + +### 7. Prev and repeat-all are permanently broken — confirmed + +**Problem.** `next()` destroys the queue history it needs. + +**Evidence.** `frontend/src/store/usePlaybackStore.ts:102` — `next()` does +`queue.slice(idx + 1)`, so the new track is always `queue[0]`. `prev()` requires +`idx > 0` (`:123`) → always resolves null. `repeat: 'all'` jumps to `queue[0]`, which is +the current last track. + +**Impact.** After any auto-advance, Previous does nothing, ever. At the end of an album, +repeat-all loops the final track instead of restarting. + +**Action.** Repair: track a `currentIndex` instead of trimming. One change fixes both. +Separately, the **uncommitted** `usePlaybackStore.ts` change in the working tree is a +correct fix for a real end-of-queue auto-resume loop — commit it. + +### 8. MusicBrainz rate limiting is not enforced — confirmed + +**Problem.** The 1 req/s throttle is a TOCTOU race and does not hold. + +**Evidence.** `workers/src/integrations/http.ts:153-161` reads `lastRequestAt`, then +`await delay(...)`, then writes — no mutex, no queue, with `concurrency: 10`. Ten jobs read +the same timestamp, sleep the same duration, and fire in the same tick. + +**Impact.** Up to ~10 req/s against MusicBrainz's 1 req/s policy, risking an IP block. And +because `musicbrainz.client.ts:280` catches `HttpError` and returns `null`, a rate-limited +MusicBrainz is indistinguishable from "no data for your library" while every job reports +success. + +**Action.** Repair: serialize `throttle` per host with a promise chain. + +### 9. Expensive work computed and discarded — confirmed + +**Problem.** Two spec'd subsystems cost real time and produce nothing. + +**Evidence.** +- `session-director.service.ts:242-270` `getBudgets` runs 7+ aggregate queries per replan. + `rankCandidates` (`:677`) never reads its `budgets` or `state` parameters, and + `detectAntiLoop` ignores its first three. System D budget enforcement is unimplemented. +- `discovery.service.ts:86` writes `source: 'graph_exploration'`, which is absent from + `source_trust` (verified — only 7 keys: `curated, mb, cover_art_archive, discogs, + lastfm, listener_behavior, tag`). `POST /api/discovery/walk` therefore **always** 500s on + an FK violation, after having already committed an orphan candidate row. + +**Action.** Delete, or finish. The discovery walk has never worked. + +### 10. Verification is largely theatre — confirmed + +**Problem.** Nothing would have caught findings 1, 3, or 4. + +**Evidence.** +- Frontend `npm run typecheck` **cannot run** — `frontend/node_modules/typescript` is a + partial install (no `package.json`, dangling `.bin/tsc` symlink). +- `.github/workflows/typecheck.yml` matrixes only `[backend, workers]` and has **no test + job** — the 33 backend tests never run in CI. +- Those 33 tests pass in 440ms with zero database. All three files mock + `{ query: vi.fn() }` and largely assert that a substring appears in a SQL template. + Several cannot fail: `expect(results).toBeDefined()`, and assertions wrapped in + `if (results.length > 0)`. +- `frontend/Dockerfile` uses `npm install`, not `npm ci`, so the lockfile is ignored. + Backend `node_modules` has `@fastify/cors@11.3.0` installed against a + `^9.0.1` declaration — local and built images do not agree. + +Every confirmed bug above lives in untested code. + +--- + +## secondary findings + +- **Integrity sweep has no sanity guard.** `workers/src/integrity.service.ts:188` marks + every unreachable track `MISSING` with no percentage threshold and no check that + `MUSIC_DIR` is mounted. If `/mnt/hdd1` is unmounted when the 03:00 sweep runs, the + **entire library** is flipped to `MISSING` with no automatic path back. It also only ever + checks an arbitrary 5,000 tracks (`:74` — `LIMIT` with no `ORDER BY`, no pagination). +- **Worker shares one pg connection across `concurrency: 10`.** `workers/src/index.ts:24` + creates a single `Client`; `cleanup.service.ts:82` issues bare `BEGIN`/`COMMIT`/`ROLLBACK` + on it. Another job's query can be enrolled in — and discarded by — that transaction. This + is the exact hazard `backend/src/app.ts:35-40` documents as its reason for using a `Pool`. +- **No BullMQ retries anywhere.** Zero `attempts`/`backoff` in either package (default is + 1 attempt). Combined with `jobId: meta-` and `removeOnFail: {age: 86400}`, + re-enqueueing a failed track within 24h is a **silent no-op**. +- **Image proxy SSRF.** `backend/src/routes/images.routes.ts:79` validates redirect hop 1, + then re-fetches *without* `redirect: 'manual'` — following up to 20 further hops + unvalidated. Directly contradicts the comment above it. Any open redirect on an + allowlisted host (`commons.wikimedia.org`, `*.musicbrainz.org`) becomes a full SSRF + primitive. +- **`split-collab-artists.ts` cascade-deletes tracks.** `:125-136` leaves colliding albums + on the doomed artist, then deletes it; `albums.artist_id` and `tracks.album_id` are both + `ON DELETE CASCADE`. Do not run as written. `dedup-artists-albums.ts` is the correct + implementation. +- **Play recording only happens inside a Vibe session.** `historyService.recordPlay` / + `recordSkip` / `feedback` have no callers in the frontend (verified by grep — only + `historyService.list()` is used). The sole write path is `v2.routes.ts:137` on a Vibe + `completed`. Live: `sum(play_count) = 1445` = exactly the `play_history` row count, and + only 535/3,962 tracks (13.5%) have any plays. Browsing/album playback contributes no + signal, yet Home's "Most Played" and Vibe's seed list both sort by `play_count`. +- **No `error` listener on the audio element** (`AudioEngine.tsx:89-93`), and + `play().catch(() => {})` swallows failures. A 404 stream leaves `isPlaying: true`, the + scrubber at 0:00, and no toast — playback hangs silently. +- **Nine bare `usePlaybackStore()` calls** with no selector re-render on every state change; + `setPosition` fires ~4×/s, so 50 `TrackRow`s re-render four times a second during playback. +- **`workers`' own `ensureSchema()`** (`enrichment.service.ts:78-156`) is 79 lines of shadow + schema that recreates a non-unique `idx_artists_mbid` on every boot — an index the backend + migration `20260709_artists_mbid_unique` deliberately dropped. +- **`runMigrations` has no advisory lock** (`db.service.ts:573`). Concurrent boots can both + run a pending migration; several bodies are not concurrency-safe. +- **No keyboard control of playback at all** — no Space, no arrow seek, no next/prev. + `TrackRow.tsx:59` is a `
` with no `tabIndex`/`role`/`onKeyDown`, so no list + in the app is playable by keyboard. + +--- + +## corrected claims + +Recorded so these are not acted on at the wrong priority. + +- **`dedup-albums` grouping by title alone** (`admin.routes.ts:41`, duplicated in + `enrichment.service.ts:1264` — omits `artist_id` from the `GROUP BY`) is real in code, but + **blast radius today is zero**: no two albums in the library share a lowercase title. A + landmine that fires on the next `reprocess_artists` if one appears — fix it, but it is not + active data loss. +- **`dislikes.grace_hours` being NULL** is a non-issue — the column has `DEFAULT 48`. +- **Mixed `timestamp` / `timestamptz`** (19 naive vs 12 aware; `tracks` has both) is latent, + not active — DB and backend are both `Etc/UTC`. Refactor-later. +- **"Tracks missing from disk"** — an initial check stat'd container paths from the host and + was wrong. Re-run inside the container: **60/60 present**. Invariant A holds. +- **`normalize_artist` truncation** is real (`AC/DC` → `AC`, `Felix Mendelssohn` → `Feli`) + but affects only 2 live rows and produces 0 collisions. Low priority. + +--- + +## keep + +The three-process split; Postgres as source of truth; the migration registry +(`schema_migrations` is recorded, ordered, individually atomic, append-only by convention — +genuinely well done); `upsertJobScheduler` cron registration (correctly idempotent across +restarts); graceful worker shutdown; the backend's `Pool`-not-`Client` reasoning; the Ethos +design system; `Artwork`'s gradient fallback; `useDislikeTrack` (the one flow with full +success/error/undo feedback); the `docs/architecture/` spec set — it is the reason these +bugs are identifiable *as* bugs. + +## remove + +- `discovery.service.walkGraphForDiscovery` — never worked +- System D budget computation — or wire it in +- `frontend/src/components/Inspector.tsx` — 169 lines, unreachable (`setInspector` is only + ever called by `closeInspector`) +- `frontend/src/components/PanelHeader.tsx` — 36 lines, zero importers, documenting a + refactor that was never applied +- `frontend/nginx.conf` — dead, unreferenced by the Dockerfile, and the *insecure* variant +- ~20 dead frontend service exports +- `workers`' `ensureSchema()` — shadow schema +- the stale `muzick.service` note in `AGENTS.md` and `CLAUDE.md` — verified: no such systemd + unit exists + +## repair now, in order + +1. `canonical_name` — restores rebuildability (finding 1) +2. nginx admin-key injection — restores the entire admin UI (finding 2) +3. `decayBeliefs` CTE — fix verified (finding 3) +4. claim dedup + `NULLS NOT DISTINCT` (finding 4) +5. column allowlist in the three `update*` methods (finding 5) +6. `currentIndex` in the playback store; commit the pending fix (finding 7) +7. serialize `throttle` per host (finding 8) +8. integrity-sweep abort threshold (secondary) +9. worker `Client` → `Pool` (secondary) + +## refactor later + +Decompose `enrichment.service.ts` (1,311 lines, six unrelated concerns); move the 328-line +inline `MIGRATIONS` array out of `db.service.ts`; add selectors to the nine bare +`usePlaybackStore()` call sites; unify `Genres.tsx` / `Discover.tsx` (~250 near-duplicate +lines); normalize timestamp types; advisory lock around `runMigrations`. + +## rewrite only if + +Nothing here justifies a rewrite. Every finding is a local repair in structurally sound +code. + +The one thing worth reconsidering from first principles is **System A/B (claims → fusion → +beliefs)** — but only *after* findings 3 and 4 are fixed, because that subsystem has never +once been observed running correctly. Judge the design on working behaviour, not on the +current state. + +## verification plan + +- `docker run` a scratch Postgres with `schema.sql`, run a real scan, assert + `count(tracks) > 0` — turns finding 1 into a permanent regression test +- add `frontend` to the CI matrix; add a `npm test` job; reinstall frontend `node_modules`; + switch the frontend Dockerfile to `npm ci` +- backend integration tests against a real container for `withTransaction`, + `runMigrations`, `recordPlay`, and the auth hook +- post-fix assertions: + - `claims` duplicate groups → 0 + - `max(last_decayed_at)` advances hourly + - zero EROFS errors in 24h + - Jobs page renders stats + +## open decisions + +**The dislike lifecycle.** The spec says delete the file; the mount is read-only. Pick one: + +- **(a) Drop hard deletion.** Terminal state becomes `HIDDEN`; amend invariant §C; delete + `finalizeDeleted`. Safe, honest, and the library stays immutable. **Recommended** — the + library is a read-only bind for a reason. +- **(b) Make deletion real.** Requires an `rw` mount. `cleanup.service.ts` currently unlinks + *before* its DB transaction and has no dry-run and no per-sweep cap; that ordering must be + fixed first. + +Either way, clear the 37 stuck `WARNED` rows — they generate ~300 errors/day. + +## uncertainties + +- No HTTP endpoint could be exercised: the review sandbox blocked egress (identical 503s + that never reached the backend, confirmed absent from its logs). The auth and SSRF + findings are established from configuration and code, not from a live request. The SSRF + one is unambiguous in code. +- `docker-compose.yml` sets **no resource limits**, though + `docs/architecture/02-invariants-and-risks.md` §B names cgroup limits as the mitigation + for Essentia starving the API. Unquantified in practice. +- Whether the `discovery` / `contextual` profiles are empty because decay never ran or + because nothing ever wrote them is not yet separable; re-evaluate after finding 3 is fixed. + +--- + +## second-opinion review, 2026-07-30 (Claude) + +Independently spot-checked the load-bearing findings against the code. **Verdict endorsed: +repair, do not rewrite.** + +Verified directly: + +- **Finding 1** — `schema.sql:50` declares `canonical_name TEXT NOT NULL` with no default; + `scanner.service.ts:241` inserts only `(name)`. Confirmed. +- **Finding 3** — the `decayBeliefs` CTE is `WITH halflives AS (SELECT profile, CASE ...)` + with no `FROM` clause; Postgres rejects it on every hourly run. Confirmed; the + `VALUES`-based rewrite is the right fix. +- **Finding 4** — `UNIQUE (..., source, user_id)` with nullable `user_id` means + `ON CONFLICT` never fires for objective claims. Confirmed; `NULLS NOT DISTINCT` (PG16) + is correct, and dedup must indeed precede the constraint. +- **Finding 5** — `updateTrack` interpolates `Object.keys(body)` into the SET clause. + Confirmed. +- **Finding 7** — `next()` does `queue.slice(idx + 1)`, so the current track is always + index 0; `prev()`'s `idx > 0` guard can never pass after an auto-advance, and repeat-all + restarts from the track that just ended. Confirmed. The uncommitted end-of-queue fix in + the working tree is correct and independent of this bug. +- **Finding 2** — `nginx.conf.template` injects only `Bearer ${MUZICK_API_KEY}` for all of + `/api`, no admin-key location block. Confirmed at the config layer. + +Judgment calls also endorsed: the repair ordering (rebuildability → admin UI → learning +loop), the "corrected claims" discipline, and option (a) for the dislike lifecycle — the +`:ro` mount is intentional. + +Two additions: + +1. **Finding 3 rollout:** the first successful decay run applies ~23 days of accrued decay + at once (obsession beliefs → ~0.32×). Correct behaviour, but recommendations will shift + visibly — expected, not a regression. +2. **Finding 4 dedup:** the one-time migration should collapse each duplicate group keeping + `MAX(last_reinforced_at)` (and ideally max confidence), not merely delete extra rows, + or reinforcement recency is lost.