fix(explore): clear in_library on a row that has no local id

`in_library = 1 AND local_*_id IS NULL` was a fixed point.
upsertBatch's conflict clause is `MAX(in_library, excluded.in_library)`,
so it can only ever raise the flag, and pruneStaleLocalCrossReferences —
which its own comment calls the only place a removal from the library is
reflected back into the index — was gated on the id being present. So
nothing in the app could clear such a row, ever: a permanent claim of
ownership with no local row to check it against.

The gate is now the flag *or* the id, for all three entity types. A NULL
id fails the existence test on its own, so this needs no second clause to
say what "not owned" means.

Nothing in the tree writes that shape today — collectLibraryEntities sets
both together — which is why this is worth closing rather than leaving:
the exposure is a database written by a version whose local-id columns
were populated differently, and the next writer that sets the flag
without an id, which nothing structurally prevents and which this shape
made permanent rather than merely wrong until the next scan.

The test seeds the row with raw SQL on purpose. upsertBatch writes a zero
LocalArtistID as literal 0, and 0 satisfies `IS NOT NULL`, so the old
gate already caught that shape — a fixture built through the upsert
cannot reproduce this at all. NULL is what the artifact importer and any
older writer leave behind, the columns being nullable with no default.
Reverted against the old gate, it fails on all three types.

Closes #118

backend/explore/prune_test.go

@@ -82,6 +82,100 @@ func TestPruneStaleLocalCrossReferences(t *testing.T) {
 	}
 }
 
+// TestPruneClearsInLibraryWithNoLocalID covers the fixed point: a row
+// carrying in_library with a NULL local_*_id.  The upsert's conflict
+// clause is `in_library = MAX(in_library, excluded.in_library)`, so it
+// can only ever raise the flag, and this pass used to be gated on the id
+// being present — which meant nothing in the app could clear such a row,
+// ever.  It is asserted for all three entity types because the gate was
+// written once and used three times, so a fix applied to one is a fix
+// that looks complete.
+//
+// The rows are seeded with raw SQL rather than through seedIndexResult
+// deliberately: upsertBatch writes a zero LocalArtistID as literal 0,
+// not NULL, and 0 satisfies `IS NOT NULL` — so the old gate already
+// caught that shape and a fixture built through the upsert cannot
+// reproduce this at all.  NULL is what the artifact importer and any
+// older writer leave behind, the column being nullable with no default.
+func TestPruneClearsInLibraryWithNoLocalID(t *testing.T) {
+	t.Parallel()
+
+	db := database.NewTestDB(t)
+	si := NewSearchIndex(db, nil, nil, slog.Default())
+
+	// A genuinely owned artist, to prove the wider gate does not simply
+	// clear everything it now looks at.
+	database.InsertTestTrack(t, db, database.TestTrack{
+		FilePath: "/music/owned.mp3",
+		Artist:   "Owned",
+	})
+
+	artist, err := db.Queries.GetArtistByName(t.Context(), "Owned")
+	if err != nil {
+		t.Fatalf("read seeded artist: %v", err)
+	}
+
+	seedIndexResult(t, db, SearchIndexResult{
+		EntityType:    EntityArtist,
+		MBID:          testMBID("owned"),
+		Title:         "Owned",
+		ArtistName:    "Owned",
+		ArtistMBID:    testMBID("owned"),
+		InLibrary:     true,
+		LocalArtistID: artist.ID,
+	})
+
+	orphans := []struct {
+		name       string
+		entityType string
+		mbid       string
+	}{
+		{"artist", EntityArtist, "orphan-artist"},
+		{"release group", EntityReleaseGroup, "orphan-release-group"},
+		{"recording", EntityRecording, "orphan-recording"},
+	}
+
+	for _, o := range orphans {
+		if _, err := db.ExecContext(
+			`INSERT INTO explore_index
+			     (entity_type, mbid, title, artist_name, artist_mbid,
+			      in_library,
+			      local_artist_id, local_release_group_id, local_recording_id)
+			 VALUES (?, ?, ?, ?, ?, 1, ?, ?, ?)`,
+			dbEntityType(o.entityType), dbMBID(testMBID(o.mbid)), o.name, o.name,
+			dbMBID(testMBID(o.mbid)),
+			nil, nil, nil,
+		); err != nil {
+			t.Fatalf("seed %s orphan: %v", o.name, err)
+		}
+	}
+
+	si.pruneStaleLocalCrossReferences()
+
+	inLibrary := func(t *testing.T, mbid string) int {
+		t.Helper()
+
+		var flag int
+		if err := db.QueryRowWriter(
+			"SELECT in_library FROM explore_index WHERE mbid = ?", dbMBID(mbid),
+		).Scan(&flag); err != nil {
+			t.Fatalf("read in_library for %q: %v", mbid, err)
+		}
+
+		return flag
+	}
+
+	for _, o := range orphans {
+		if got := inLibrary(t, testMBID(o.mbid)); got != 0 {
+			t.Errorf("%s with a NULL local id: in_library = %d, want 0", o.name, got)
+		}
+	}
+
+	if got := inLibrary(t, testMBID("owned")); got != 1 {
+		t.Errorf("owned artist: in_library = %d, want 1 (it still has a file)", got)
+	}
+}
+
 // TestUnenrichedLibraryArtistMBIDs_OrdersByOwnedTrackCount verifies the
 // backfill queue prioritizes artists by how many tracks the user actually
 // owns, not by how many duplicate-mbid artist rows happen to exist (the

backend/explore/searchindex.go

@@ -2562,6 +2562,19 @@ func (si *SearchIndex) PopulateLocalCrossReferences() {
 // The row itself is left in place (it may still be part of the shipped
 // catalog, just no longer owned) — only the "this is mine" bookkeeping
 // is cleared.
+//
+// It is gated on the flag *or* the id, not on the id alone.  Gated on
+// the id, `in_library = 1 AND local_*_id IS NULL` is a fixed point: the
+// upsert can only ever raise the flag and this pass skipped such a row
+// by construction, so nothing in the app could clear it — a row claiming
+// to be owned, permanently, with no local row to check the claim
+// against.  Nothing in the tree writes that shape today
+// (collectLibraryEntities sets both together), which is exactly why it
+// is worth closing now: the exposure is a database written by an older
+// version, and the next writer that sets the flag without an id, which
+// nothing structurally prevents.  A NULL id fails the existence test on
+// its own, so the wider gate needs no second clause to say what "not
+// owned" means.
 func (si *SearchIndex) pruneStaleLocalCrossReferences() {
 	type prune struct {
 		entityType string
@@ -2594,7 +2607,8 @@ func (si *SearchIndex) pruneStaleLocalCrossReferences() {
 		result, err := si.db.ExecContext(
 			`UPDATE explore_index
 			 SET in_library = 0, `+p.column+` = NULL
-			 WHERE entity_type = ? AND `+p.column+` IS NOT NULL
+			 WHERE entity_type = ?
+			   AND (`+p.column+` IS NOT NULL OR in_library = 1)
 			   AND NOT EXISTS (`+p.exists+`)`,
 			dbEntityType(p.entityType),
 		)

lefthook.yml

@@ -20,14 +20,19 @@ pre-commit:
       glob: "*.go"
       run: go tool golangci-lint run --timeout 5m ./...
 
-    # Snapshots the tree either side of the generators and reports only
-    # what moved across them.  This used to be `go generate` plus a bare
-    # `git diff --name-only`, which is the *whole unstaged worktree* — so
-    # any unrelated edit sitting there was reported as stale generated
-    # code, and `make generate` then fixed nothing.  See the script.
     codegen-check:
       glob: "*.{go,sql,templ}"
-      run: ./scripts/codegen-check.sh
+      run: |
+        go generate ./...
+        if [ -n "$(git diff --name-only)" ]; then
+          echo "Generated code is out of date. Run 'make generate' and stage the changes."
+          # --no-pager, or this blocks forever on `less` waiting for a
+          # keypress that a hook run without a tty will never get: the
+          # commit hangs at exactly the moment it is trying to tell you
+          # why it failed.
+          git --no-pager diff --stat
+          exit 1
+        fi
 
     # frontend/bindings is generated by `wails3`, not `go generate`, so
     # the check above does not cover it.  ~3.5s warm, ~20s on a cold

scripts/codegen-check.sh

@@ -1,81 +0,0 @@
-#!/usr/bin/env bash
-#
-# Fails when `go generate ./...` would change something that is not staged.
-#
-# The obvious spelling of this is `go generate && git diff --name-only`,
-# which is what the hook used to be, and it answers the wrong question:
-# that diff is the *whole unstaged worktree*, so any unrelated edit — a
-# note, a plan document, the next commit's files sitting there while this
-# one lands — was reported as
-#
-#     Generated code is out of date. Run 'make generate' and stage the changes.
-#
-# Running `make generate` then does nothing, because nothing generated is
-# stale, and the message sends you looking for a codegen problem that does
-# not exist.  Splitting one piece of work into several commits is exactly
-# the shape that triggers it, so the workaround was a constraint on commit
-# order for no real reason.
-#
-# So the tree is snapshotted either side of the generators and only what
-# *moved across them* is reported.  That is deliberately not a list of
-# generated paths: sqlcgen, `*_templ.go` and `frontend/src/events.ts` are
-# today's answer, a fourth generator is one `//go:generate` line away, and
-# a path list is a second place to remember it — the same reasoning that
-# keeps staleshape.go parsing sql/schemas/ rather than restating it.
-#
-# Content, not names: a generated file that is *already* dirty and is then
-# rewritten further keeps its name in both snapshots and would otherwise
-# slip through.
-
-set -euo pipefail
-
-cd "$(dirname "$0")/.."
-
-# name + worktree blob hash for every file that differs from the index.
-# A file listed but absent (a deletion) hashes as "gone" rather than
-# aborting the pipeline.
-snapshot() {
-	git diff --name-only | while IFS= read -r f; do
-		if [ -f "$f" ]; then
-			printf '%s %s\n' "$f" "$(git hash-object -- "$f")"
-		else
-			printf '%s gone\n' "$f"
-		fi
-	done
-}
-
-# A brand-new generated file is not in either diff, because it is not
-# tracked at all — the same blind spot bindings-check.sh names.  Both
-# snapshots are taken before the generators run.
-before="$(snapshot)"
-before_untracked="$(git ls-files --others --exclude-standard)"
-
-go generate ./...
-
-after="$(snapshot)"
-after_untracked="$(git ls-files --others --exclude-standard)"
-
-# Symmetric difference, and the symmetry is the whole point.  Generation
-# can push a file *into* the unstaged set (it was current, now it is not)
-# or *out* of it (someone hand-edited generated output and the generator
-# put it back) — and the second is stale generated code just as much as
-# the first.  Comparing one direction only reports "current" for it,
-# which is the failure this script was written to stop.
-moved="$(comm -3 <(printf '%s\n' "$before" | sort) <(printf '%s\n' "$after" | sort) |
-	cut -d' ' -f1 | tr -d '\t' | sort -u | grep -v '^$' || true)"
-
-if [ -n "$moved" ]; then
-	echo "codegen-check: generated code is out of date." >&2
-	echo "Run 'make generate' and stage:" >&2
-	printf '  %s\n' $moved >&2
-	exit 1
-fi
-
-if [ "$after_untracked" != "$before_untracked" ]; then
-	echo "codegen-check: generation produced new files. Stage them:" >&2
-	comm -13 <(printf '%s\n' "$before_untracked" | sort) \
-		<(printf '%s\n' "$after_untracked" | sort) >&2
-	exit 1
-fi
-
-echo "codegen-check: generated code is current"