db.service inserted the 'deleted_permanent' feedback row and then deleted
the track, but feedback.track_id was ON DELETE CASCADE (verified on the
live DB: confdeltype = 'c'), so the audit row deleted itself. feedback
contains zero deleted_permanent rows.
feedback is an audit log and must outlive its subject: the FK becomes
ON DELETE SET NULL. track_id was already nullable, and nothing in backend/
or workers/ SELECTs from feedback — the only other reference is
mergeTracks()'s UPDATE feedback SET track_id, which re-points to the
survivor — so no caller assumed non-null.
Migration 20260730_feedback_track_id_set_null drops the constraint by
matching confdeltype rather than by name, since the live schema has
drifted. Verified on a scratch PG16: confdeltype flips 'c' -> 'n' and a
deleted_permanent row survives its track's deletion.
Correct under either resolution of the dislike-lifecycle decision, so it
lands independently of it.
REVIEW-2026-07-30.md finding 6 (cascade only).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
updateTrack/updateArtist/updateAlbum built their SET clause from
Object.keys(data) where data is `request.body as any`, interpolating
request-supplied keys straight into SQL as quoted identifiers:
fields.map((f, i) => `"${f}" = $${i + 2}`)
A crafted body key closes the quoted identifier and injects into the SET
list. Mitigated in practice only by the LAN/VPN-only proxy — which supplies
the auth token automatically, so any device on the LAN could reach it from
a browser.
Adds per-table UPDATABLE_COLUMNS plus an allowedFields() helper, applied in
all three methods. The allowlist lives in the service layer rather than the
routes so it covers every caller.
Unknown keys are dropped rather than rejected: the three routes do no error
mapping, so a throw surfaces as a bare 500, and the pre-existing "No fields
to update" error still fires for a payload rejected in its entirety.
Also closes plain mass-assignment. Excluded: path/hash/mtime
(scanner-owned; path is the only link to the read-only bind),
state/quarantined_at/deleted_at (dislike lifecycle and integrity sweep),
play_count/skip_count/dislike_count/last_played_at (learning signal —
forgeable counters poison the engine), and identity/generated columns.
The only callers are the three HTTP PUTs; the frontend's update* service
exports are dead code, so nothing relied on writing an excluded column.
REVIEW-2026-07-30.md finding 5.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
claims declared UNIQUE (..., source, user_id). Every objective claim has
user_id IS NULL, and under default NULLS DISTINCT semantics Postgres treats
those rows as unique, so the ON CONFLICT DO UPDATE / DO NOTHING clauses in
db.service and mb-spine-writer never fired. Re-enrichment inserted a fresh
duplicate every run instead of reinforcing.
Live data: 236 duplicate groups, 2110 excess rows, worst single claim 86
copies, ~15% of 13,910 claims. claim_fusion is SUM(trust * confidence *
recency), so one edge could carry 86x its intended weight — the likely
cause of repetitive recommendations, and almost certainly the root of
d497588 (claim_fusion MV duplicate-key failure).
Migration 20260730_claims_dedup_nulls_not_distinct, two phases in one
transaction. Dedup MUST precede the constraint or adding it fails.
Phase 1 collapses each group into its most recently reinforced row,
carrying forward MAX(last_reinforced_at), MAX(evidence_at) and
MAX(confidence) — reinforcement recency would otherwise be lost by simply
deleting extras. The MAX(...) OVER grp and ROW_NUMBER() OVER ordered
windows are deliberately separate: an ORDER BY inside the window makes the
default frame UNBOUNDED PRECEDING TO CURRENT ROW, which turns MAX() into a
running maximum and would silently keep the wrong confidence.
Phase 2 drops the old constraint by matching its definition rather than its
name, because the live DB has drifted and its autogenerated name is
truncated at 63 characters.
Verified on a scratch PG16 seeded with the old schema plus a 3-row
duplicate group, a distinct-source singleton and a real-user_id row:
UPDATE 3 / DELETE 2, keeper retained the group max of each field from three
different rows, re-run is a no-op, and a subsequent ON CONFLICT DO UPDATE
with user_id = NULL fires correctly.
REVIEW-2026-07-30.md finding 4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CTE was `WITH halflives AS (SELECT profile, CASE profile ...)` with no
FROM clause. Postgres rejects it with 42703 (column "profile" does not
exist) on every hourly invocation, so the temporal dimension of the
recommendation engine had never executed once — obsession (14d half-life)
and contextual (7d) never faded.
Rewritten as `WITH halflives(profile, halflife_sec) AS (VALUES ...)`,
half-lives preserved exactly.
One deliberate semantic change: the broken CASE had an `ELSE 30 * 86400`
fallback, so an unrecognised profile would have decayed on a 30-day
half-life. The VALUES join leaves unknown profiles undecayed instead.
Today that is a no-op (only `forgotten`, already excluded by the WHERE),
but a future profile added without a half-life will now conspicuously not
decay rather than quietly decaying at an arbitrary rate.
Verified against a scratch PG16 with one belief per profile aged exactly
one half-life: UPDATE 2, obsession and contextual halved, forgotten and a
fresh longterm untouched. Against the live DB (in a rolled-back
transaction) the fix reports UPDATE 843.
NOTE ON ROLLOUT: the first successful run applies ~23 days of accrued
decay at once, cutting obsession beliefs to ~0.32x. That is correct
behaviour, but recommendations will shift visibly. Expected, not a
regression.
REVIEW-2026-07-30.md finding 3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three separate insert paths made a fresh Postgres volume unusable. The live
database only works because its volume predates the constraints.
- scanner.service.resolveOrCreateArtist inserted only (name), but
schema.sql declares canonical_name NOT NULL with no default. Every
artist insert failed, and processFile swallows per-file errors, so a
scan reported success with 0 tracks and a permanently empty library.
- enrichment.service inserted explicitly into artists.normalized_name,
which is GENERATED ALWAYS AS (normalize_artist(name)) STORED:
"cannot insert a non-DEFAULT value into column" (428C9). All
enrichment artist creation failed on a fresh volume.
- db.service.createArtist omitted canonical_name, same failure.
canonical_name holds the raw tag name, not normalize_artist() output,
which truncates on `/` and a standalone `x` ("AC/DC" -> "AC"). That is the
convention createLocalArtist already used. The truncation bug in
artists.name is pre-existing and deliberately left untouched here.
Verified on a scratch postgres:16-alpine with the real schema: the old
statement reproduces the NOT NULL violation, the new path yields
artists/albums/tracks/track_artists rows.
REVIEW-2026-07-30.md finding 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The MV SELECTed COALESCE(user_id, zero-uuid) but GROUPed BY raw user_id,
so a global enrichment claim (user_id NULL) and a default-user behavior
claim (user_id = zero-uuid) for the same edge produced two rows that
collide on idx_claim_fusion_pk, breaking REFRESH ... CONCURRENTLY.
Exposed by the #219 listener_behavior same_scene_as/alias_of writes.
New migration rebuilds the MV grouping by the COALESCE'd user_id so the
two fuse into one row.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-user SET NX PX lock with Lua CAS release around the
getActivePlan->mutate->setActivePlan span so concurrent prefetch +
feedback requests serialize instead of losing one side's write.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
both generators ran N sequential queries per artist/album.
rewritten with ROW_NUMBER() OVER (PARTITION BY ...) to get per-group
limits in a single round-trip, preserving existing semantics:
- comfortGenerator: up to 2 tracks per artist (was 20 queries → 1)
- deepDiveGenerator: up to 5 tracks per album (was 20 queries → 1)
kami