fix(scripts): report only what generation changed, not the worktree

The codegen-check hook was `go generate` followed by a bare
`git diff --name-only`, which is the whole unstaged worktree rather than
the generators' output. So a commit whose staged changes were fine failed
whenever anything unrelated sat unstaged — notes, a plan document, the
next commit's files — reporting "Generated code is out of date" and then
a diffstat of files no generator has ever written. `make generate` fixed
nothing, because nothing was stale, so the message sent you looking for a
codegen problem that did not exist. Splitting one piece of work into
several commits is exactly the shape that triggers it.

The tree is snapshotted either side of `go generate` and only what moved
across it is reported. That is deliberately a snapshot rather than the
list of generated paths the issue offers as the other option: a fourth
generator is one //go:generate line away, and a path list is a second
place to remember it.

Two things it has to get right. The comparison is a *symmetric*
difference, because generation can push a file into the unstaged set or
pull it out of one — a hand-edited generated file that the generator puts
back is stale generated code just as much as a source change that
outdates it, and comparing one direction reports it as current. And the
snapshot is content, not names, or a generated file that was already
dirty and is then rewritten further keeps its name on both sides and
slips through.

Closes #131

backend/explore/prune_test.go

@@ -82,100 +82,6 @@ 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,19 +2562,6 @@ 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
@@ -2607,8 +2594,7 @@ func (si *SearchIndex) pruneStaleLocalCrossReferences() {
 		result, err := si.db.ExecContext(
 			`UPDATE explore_index
 			 SET in_library = 0, `+p.column+` = NULL
-			 WHERE entity_type = ?
+			 WHERE entity_type = ? AND `+p.column+` IS NOT NULL
-			   AND (`+p.column+` IS NOT NULL OR in_library = 1)
 			   AND NOT EXISTS (`+p.exists+`)`,
 			dbEntityType(p.entityType),
 		)

lefthook.yml

@@ -20,19 +20,14 @@ 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: |
+      run: ./scripts/codegen-check.sh
-        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

@@ -0,0 +1,81 @@
+#!/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"