Phase 3: a storage seam behind catalog, and one consumer threaded onto it

EXPAND ONLY, per the migration order (expand, migrate, verify, contract). No
Postgres implementation, no behaviour change.

internal/catalog now exposes:

  type Store interface {
      Item / ItemFor / Files / FilesFor / FileByID / FirstFile / FileCount
  }
  type SQL struct{ db *sql.DB }; func New(db *sql.DB) *SQL

The seven bodies are the old functions moved verbatim. ItemFor still returns the
same ErrNotFound for absent and forbidden, so the security property is
unchanged. A compile-time `var _ Store = (*SQL)(nil)` keeps the implementation
honest.

Deliberately NOT moved: browse lists, home shelves and subsonic album queries.
Those are each package's own presentation concern, and pulling all of them into
the seam is exactly the 186-statement rewrite the seam exists to avoid. The
package doc says so.

Proven substitutable, not just declared: store_test.go implements a fake Store
in memory with no database at all and drives a caller through the interface. A
seam nobody can substitute proves nothing.

The agent's own risk note was right and I acted on it: the seam only pays off
once something actually threads it, and initially nothing did. So internal/
playback — the smallest consumer — now takes a catalog.Store instead of *sql.DB
and is wired with catalog.New(db) at the composition point. It has no database
reference left outside a comment. The remaining consumers (media, jellyfin,
subsonic, metadata) still call the package-level wrappers; threading them is
mechanical, and I would rather do it deliberately than in one sweep.

Verified: gofmt clean, build exit 0, vet exit 0, 25 packages pass, arch rules
pass. End to end against a running server: /playback/resolve still returns
vcodec:hevc for html5 and engine mpv when mpv is offered, an unknown item 404s,
/home and Jellyfin /Items still answer.

Known gap, unchanged by this commit: internal/arch does not enforce "new code
must not take *sql.DB", so the seam can erode by drift. That rule would be the
natural next step if this is to hold.

cmd/vellum/main.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"github.com/mithral/vellum/internal/auth"
+	"github.com/mithral/vellum/internal/catalog"
 	"github.com/mithral/vellum/internal/cli"
 	"github.com/mithral/vellum/internal/core"
 	"github.com/mithral/vellum/internal/home"
@@ -116,7 +117,7 @@ func run(cfg core.Config, log *slog.Logger) error {
 	// Playback engine registry + selection. Vellum never transcodes, so the
 	// server reports what is inside a file and matches it against the engines the
 	// CLIENT declares — a browser offers "html5", a native client can offer mpv.
-	playback.NewHandlers(db, log, authenticator).Register(api.Mux())
+	playback.NewHandlers(catalog.New(db), log, authenticator).Register(api.Mux())
 
 	// Client compatibility layers. Neither speaks the session cookie; both
 	// authenticate with an api_tokens credential and then go through the same

internal/catalog/catalog.go

@@ -13,6 +13,23 @@
 // this package's surface instead of editing 186 statements scattered across the
 // tree.
 //
+// # What the Store covers, and what it deliberately does not
+//
+// Store is the seam, and it holds the per-item load path only: Item, ItemFor,
+// Files, FilesFor, FileByID, FirstFile, FileCount. Those are the queries whose
+// correctness is a security property (ItemFor is the authorization chokepoint)
+// and whose shape is identical for every media kind, so one implementation is
+// worth having.
+//
+// Aggregate and list queries — browse lists, home shelves, Subsonic albums —
+// deliberately stay OUT of it. Each is a presentation concern of the package
+// that owns the surface, with its own columns, ordering and JSON contract, and
+// none of them is shared. Pulling them in would mean rewriting all 186
+// statements to match one interface, which is the exact cost this seam exists to
+// avoid, and the result would be an interface nobody could implement without
+// writing the whole application's query layer twice. The seam buys a swappable
+// per-item load; it is not a repository.
+//
 // This package holds no HTTP knowledge. It returns typed errors and lets the
 // caller choose a status code.
 package catalog
@@ -80,6 +97,22 @@ type File struct {
 	Height     int    `json:"height"`
 }
 
+// Store is the storage seam. Implement it to run this package's per-item load
+// path against something other than SQLite.
+//
+// The contract is the one the SQL implementation has always had: absent item and
+// forbidden item both come back as ErrNotFound, and every other failure is a
+// wrapped error. A caller that can distinguish the two has a bug.
+type Store interface {
+	Item(ctx context.Context, id int64) (Item, error)
+	ItemFor(ctx context.Context, u auth.User, id int64) (Item, error)
+	Files(ctx context.Context, itemID int64) ([]File, error)
+	FilesFor(ctx context.Context, u auth.User, itemID int64) ([]File, error)
+	FileByID(ctx context.Context, itemID, fileID int64) (File, error)
+	FirstFile(ctx context.Context, itemID int64) (File, error)
+	FileCount(ctx context.Context, itemID int64) (int, error)
+}
+
 const itemSelect = `
 	SELECT i.id, i.library_id, i.kind, i.path, i.title, COALESCE(i.year, 0), i.missing, l.root_path,
 	       COALESCE((SELECT f.mtime FROM media_files f WHERE f.item_id = i.id ORDER BY f.id LIMIT 1), '')
@@ -87,15 +120,25 @@ const itemSelect = `
 	JOIN libraries l ON l.id = i.library_id
 	WHERE i.id = ?`
 
-// LoadUnchecked reads an item WITHOUT checking access. Prefer LoadFor in any handler
+// SQL is the SQLite implementation of Store, and the only thing in this package
-// that serves a request: this exists for the scanner, jobs and tests, which
+// that knows the query text.
-// have already established the right to touch the row.
+type SQL struct{ db *sql.DB }
-func LoadUnchecked(ctx context.Context, db *sql.DB, id int64) (Item, error) {
+
+var _ Store = (*SQL)(nil)
+
+// New returns a Store backed by db. The db must already be migrated; this
+// package never creates schema.
+func New(db *sql.DB) *SQL { return &SQL{db: db} }
+
+// Item reads an item WITHOUT checking access. Prefer ItemFor in any handler that
+// serves a request: this exists for the scanner, jobs and tests, which have
+// already established the right to touch the row.
+func (s *SQL) Item(ctx context.Context, id int64) (Item, error) {
 	var (
 		it    Item
 		mtime string
 	)
-	err := db.QueryRowContext(ctx, itemSelect, id).Scan(
+	err := s.db.QueryRowContext(ctx, itemSelect, id).Scan(
 		&it.ID, &it.LibraryID, &it.Kind, &it.Path, &it.Title,
 		&it.Year, &it.Missing, &it.RootPath, &mtime)
 	if errors.Is(err, sql.ErrNoRows) {
@@ -110,17 +153,17 @@ func LoadUnchecked(ctx context.Context, db *sql.DB, id int64) (Item, error) {
 	return it, nil
 }
 
-// LoadFor reads an item and confirms the user may read its library.
+// ItemFor reads an item and confirms the user may read its library.
 //
 // This is the authorization chokepoint. Every HTTP surface — the web app, the
 // modules, Jellyfin, Subsonic — is expected to go through it, so there is one
 // implementation to audit rather than five to keep in sync.
-func LoadFor(ctx context.Context, db *sql.DB, u auth.User, id int64) (Item, error) {
+func (s *SQL) ItemFor(ctx context.Context, u auth.User, id int64) (Item, error) {
-	it, err := LoadUnchecked(ctx, db, id)
+	it, err := s.Item(ctx, id)
 	if err != nil {
 		return Item{}, err
 	}
-	allowed, err := auth.CanAccessLibrary(ctx, db, u, it.LibraryID)
+	allowed, err := auth.CanAccessLibrary(ctx, s.db, u, it.LibraryID)
 	if err != nil {
 		return Item{}, fmt.Errorf("catalog: check access: %w", err)
 	}
@@ -132,8 +175,8 @@ func LoadFor(ctx context.Context, db *sql.DB, u auth.User, id int64) (Item, erro
 }
 
 // Files returns an item's files in order, with codec facts.
-func Files(ctx context.Context, db *sql.DB, itemID int64) ([]File, error) {
+func (s *SQL) Files(ctx context.Context, itemID int64) ([]File, error) {
-	rows, err := db.QueryContext(ctx, `
+	rows, err := s.db.QueryContext(ctx, `
 		SELECT id, path, size, container, missing,
 		       duration_ms, vcodec, acodec, width, height
 		FROM media_files WHERE item_id = ? ORDER BY id`, itemID)
@@ -156,18 +199,18 @@ func Files(ctx context.Context, db *sql.DB, itemID int64) ([]File, error) {
 
 // FilesFor returns an item's files after confirming access. Callers that
 // already hold an authorized Item should use Files to avoid a second check.
-func FilesFor(ctx context.Context, db *sql.DB, u auth.User, itemID int64) ([]File, error) {
+func (s *SQL) FilesFor(ctx context.Context, u auth.User, itemID int64) ([]File, error) {
-	if _, err := LoadFor(ctx, db, u, itemID); err != nil {
+	if _, err := s.ItemFor(ctx, u, itemID); err != nil {
 		return nil, err
 	}
-	return Files(ctx, db, itemID)
+	return s.Files(ctx, itemID)
 }
 
 // FileByID returns one file of an item, scoped to that item so a file id from
 // another item cannot be fetched through it.
-func FileByID(ctx context.Context, db *sql.DB, itemID, fileID int64) (File, error) {
+func (s *SQL) FileByID(ctx context.Context, itemID, fileID int64) (File, error) {
 	var f File
-	err := db.QueryRowContext(ctx, `
+	err := s.db.QueryRowContext(ctx, `
 		SELECT id, path, size, container, missing,
 		       duration_ms, vcodec, acodec, width, height
 		FROM media_files WHERE item_id = ? AND id = ?`, itemID, fileID).Scan(
@@ -184,9 +227,9 @@ func FileByID(ctx context.Context, db *sql.DB, itemID, fileID int64) (File, erro
 
 // FirstFile returns the item's first file, which is what single-file playback
 // and cover extraction both want.
-func FirstFile(ctx context.Context, db *sql.DB, itemID int64) (File, error) {
+func (s *SQL) FirstFile(ctx context.Context, itemID int64) (File, error) {
 	var f File
-	err := db.QueryRowContext(ctx, `
+	err := s.db.QueryRowContext(ctx, `
 		SELECT id, path, size, container, missing,
 		       duration_ms, vcodec, acodec, width, height
 		FROM media_files WHERE item_id = ? ORDER BY id LIMIT 1`, itemID).Scan(
@@ -202,9 +245,9 @@ func FirstFile(ctx context.Context, db *sql.DB, itemID int64) (File, error) {
 }
 
 // FileCount reports how many files back an item.
-func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) {
+func (s *SQL) FileCount(ctx context.Context, itemID int64) (int, error) {
 	var n int
-	err := db.QueryRowContext(ctx,
+	err := s.db.QueryRowContext(ctx,
 		`SELECT count(*) FROM media_files WHERE item_id = ?`, itemID).Scan(&n)
 	if err != nil {
 		return 0, fmt.Errorf("catalog: file count: %w", err)
@@ -212,6 +255,42 @@ func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) {
 	return n, nil
 }
 
+// The functions below are the original package API, kept so that existing
+// callers compile and behave exactly as before. Each one is a one-line
+// delegation to the SQL implementation above.
+//
+// NEW CODE SHOULD TAKE A Store, NOT A *sql.DB. A caller that wants to be
+// swappable off SQLite takes the Store interface and receives New(db) at the
+// wiring point; reaching for these functions bakes the engine back in.
+
+func LoadUnchecked(ctx context.Context, db *sql.DB, id int64) (Item, error) {
+	return New(db).Item(ctx, id)
+}
+
+func LoadFor(ctx context.Context, db *sql.DB, u auth.User, id int64) (Item, error) {
+	return New(db).ItemFor(ctx, u, id)
+}
+
+func Files(ctx context.Context, db *sql.DB, itemID int64) ([]File, error) {
+	return New(db).Files(ctx, itemID)
+}
+
+func FilesFor(ctx context.Context, db *sql.DB, u auth.User, itemID int64) ([]File, error) {
+	return New(db).FilesFor(ctx, u, itemID)
+}
+
+func FileByID(ctx context.Context, db *sql.DB, itemID, fileID int64) (File, error) {
+	return New(db).FileByID(ctx, itemID, fileID)
+}
+
+func FirstFile(ctx context.Context, db *sql.DB, itemID int64) (File, error) {
+	return New(db).FirstFile(ctx, itemID)
+}
+
+func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) {
+	return New(db).FileCount(ctx, itemID)
+}
+
 // parseUnixNano parses a base-10 nanosecond timestamp, returning 0 on anything
 // that is not all digits.
 func parseUnixNano(s string) int64 {

internal/catalog/store_test.go

@@ -0,0 +1,208 @@
+package catalog
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/mithral/vellum/internal/auth"
+)
+
+// This file exists to keep the Store seam honest. An interface that no one can
+// substitute is decoration, so fakeStore implements Store entirely in memory —
+// no database, no SQL — and the test drives a caller through it. If a method is
+// added to Store later, this file stops compiling, which is the intended alarm.
+//
+// What is NOT tested here: the query text. That is the SQL implementation's job
+// and catalog_test.go covers it against a real database.
+
+// fakeStore is an in-memory Store. Its authorization model mirrors auth's: an
+// admin sees everything, everyone else sees only granted libraries.
+type fakeStore struct {
+	items   map[int64]Item
+	files   map[int64][]File
+	granted map[int64][]int64 // user id -> library ids
+	admins  map[int64]bool
+
+	// calls records which methods ran, so the test can prove the caller went
+	// through the interface rather than reaching for a database.
+	calls []string
+}
+
+var _ Store = (*fakeStore)(nil)
+
+func newFakeStore() *fakeStore {
+	return &fakeStore{
+		items:   map[int64]Item{},
+		files:   map[int64][]File{},
+		granted: map[int64][]int64{},
+		admins:  map[int64]bool{},
+	}
+}
+
+func (f *fakeStore) allowed(u auth.User, libID int64) bool {
+	if f.admins[u.ID] {
+		return true
+	}
+	for _, id := range f.granted[u.ID] {
+		if id == libID {
+			return true
+		}
+	}
+	return false
+}
+
+func (f *fakeStore) Item(_ context.Context, id int64) (Item, error) {
+	f.calls = append(f.calls, "Item")
+	it, ok := f.items[id]
+	if !ok {
+		return Item{}, ErrNotFound
+	}
+	return it, nil
+}
+
+func (f *fakeStore) ItemFor(_ context.Context, u auth.User, id int64) (Item, error) {
+	f.calls = append(f.calls, "ItemFor")
+	it, ok := f.items[id]
+	if !ok || !f.allowed(u, it.LibraryID) {
+		// The whole point: absent and forbidden are the same error.
+		return Item{}, ErrNotFound
+	}
+	return it, nil
+}
+
+func (f *fakeStore) Files(_ context.Context, itemID int64) ([]File, error) {
+	f.calls = append(f.calls, "Files")
+	return f.files[itemID], nil
+}
+
+func (f *fakeStore) FilesFor(ctx context.Context, u auth.User, itemID int64) ([]File, error) {
+	f.calls = append(f.calls, "FilesFor")
+	if _, err := f.ItemFor(ctx, u, itemID); err != nil {
+		return nil, err
+	}
+	return f.Files(ctx, itemID)
+}
+
+func (f *fakeStore) FileByID(_ context.Context, itemID, fileID int64) (File, error) {
+	f.calls = append(f.calls, "FileByID")
+	for _, fl := range f.files[itemID] { // scoped to the item, like the SQL
+		if fl.ID == fileID {
+			return fl, nil
+		}
+	}
+	return File{}, ErrNotFound
+}
+
+func (f *fakeStore) FirstFile(_ context.Context, itemID int64) (File, error) {
+	f.calls = append(f.calls, "FirstFile")
+	fs := f.files[itemID]
+	if len(fs) == 0 {
+		return File{}, ErrNotFound
+	}
+	return fs[0], nil
+}
+
+func (f *fakeStore) FileCount(_ context.Context, itemID int64) (int, error) {
+	f.calls = append(f.calls, "FileCount")
+	return len(f.files[itemID]), nil
+}
+
+// serveItem is a stand-in for a real caller (media.playback, jellyfin.playback)
+// written against the Store interface instead of *sql.DB. It is here because
+// that is what the seam is FOR: this function cannot be told it needs SQLite.
+func serveItem(ctx context.Context, s Store, u auth.User, id int64) (Item, File, error) {
+	it, err := s.ItemFor(ctx, u, id)
+	if err != nil {
+		return Item{}, File{}, err
+	}
+	f, err := s.FirstFile(ctx, id)
+	if err != nil {
+		return Item{}, File{}, err
+	}
+	return it, f, nil
+}
+
+func fixture() (*fakeStore, auth.User, auth.User, int64) {
+	s := newFakeStore()
+	const libID = 42
+	s.items[7] = Item{ID: 7, LibraryID: libID, Kind: "ebooks", Path: "Thing.epub",
+		Title: "Thing", Year: 1999, RootPath: "/media/A",
+		ModTime: time.Unix(1700000000, 0).UTC()}
+	s.files[7] = []File{{ID: 700, Path: "Thing.epub", Size: 100, Container: "mp4"}}
+	s.files[8] = []File{{ID: 800, Path: "Other.epub", Size: 100}}
+
+	granted := auth.User{ID: 1, Username: "granted"}
+	outsider := auth.User{ID: 2, Username: "outsider"}
+	s.granted[granted.ID] = []int64{libID}
+	return s, granted, outsider, 7
+}
+
+func TestStoreIsSubstitutableWithoutADatabase(t *testing.T) {
+	s, granted, _, itemID := fixture()
+	it, f, err := serveItem(context.Background(), s, granted, itemID)
+	if err != nil {
+		t.Fatalf("in-memory Store rejected a granted user: %v", err)
+	}
+	if it.Title != "Thing" || it.AbsPath() != "/media/A/Thing.epub" {
+		t.Errorf("item lost in translation: %+v", it)
+	}
+	if f.ID != 700 {
+		t.Errorf("FirstFile = %d, want 700", f.ID)
+	}
+	if want := []string{"ItemFor", "FirstFile"}; len(s.calls) != 2 ||
+		s.calls[0] != want[0] || s.calls[1] != want[1] {
+		t.Errorf("calls = %v, want %v", s.calls, want)
+	}
+}
+
+// The security property has to survive the seam: a caller that could tell a
+// forbidden item from an absent one would leak another user's library.
+func TestStoreKeepsForbiddenIndistinguishableFromAbsent(t *testing.T) {
+	s, _, outsider, itemID := fixture()
+	ctx := context.Background()
+
+	_, _, ferr := serveItem(ctx, s, outsider, itemID)
+	_, _, aerr := serveItem(ctx, s, outsider, itemID+9999)
+	if !errors.Is(ferr, ErrNotFound) || !errors.Is(aerr, ErrNotFound) {
+		t.Fatalf("errors = %v / %v, want ErrNotFound for both", ferr, aerr)
+	}
+	if ferr.Error() != aerr.Error() {
+		t.Errorf("errors differ and would leak existence: %q vs %q", ferr, aerr)
+	}
+}
+
+func TestStoreAdminSeesEverythingWithoutAGrant(t *testing.T) {
+	s, _, _, itemID := fixture()
+	admin := auth.User{ID: 3, Username: "root", IsAdmin: true}
+	s.admins[admin.ID] = true
+
+	if _, err := s.ItemFor(context.Background(), admin, itemID); err != nil {
+		t.Errorf("admin denied: %v", err)
+	}
+	if _, err := s.FilesFor(context.Background(), admin, itemID); err != nil {
+		t.Errorf("admin FilesFor denied: %v", err)
+	}
+	if n, err := s.FileCount(context.Background(), itemID); err != nil || n != 1 {
+		t.Errorf("FileCount = %d, %v; want 1, nil", n, err)
+	}
+}
+
+// FileByID must stay scoped to its item through the interface too, or the
+// in-memory Store would be laxer than the SQL one it stands in for.
+func TestStoreFileByIDIsScopedToItsItem(t *testing.T) {
+	s, _, _, itemID := fixture()
+	ctx := context.Background()
+
+	other, err := s.FirstFile(ctx, itemID+1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := s.FileByID(ctx, itemID, other.ID); !errors.Is(err, ErrNotFound) {
+		t.Errorf("file id leaked across items: %v", err)
+	}
+	if _, err := s.FileByID(ctx, itemID+1, other.ID); err != nil {
+		t.Errorf("legitimate lookup failed: %v", err)
+	}
+}

internal/playback/handlers.go

@@ -9,7 +9,6 @@
 package playback
 
 import (
-	"database/sql"
 	"errors"
 	"log/slog"
 	"net/http"
@@ -22,14 +21,17 @@ import (
 
 // Handlers serves the playback selection endpoints.
 type Handlers struct {
-	db   *sql.DB
+	// items is the catalog seam. This package talks to a Store rather than to
-	log  *slog.Logger
+	// *sql.DB, so the storage engine is swappable without touching the handler —
-	auth *auth.Authenticator
+	// and a test can substitute a fake with no database at all.
+	items catalog.Store
+	log   *slog.Logger
+	auth  *auth.Authenticator
 }
 
 // NewHandlers builds the playback HTTP layer.
-func NewHandlers(db *sql.DB, log *slog.Logger, a *auth.Authenticator) *Handlers {
+func NewHandlers(items catalog.Store, log *slog.Logger, a *auth.Authenticator) *Handlers {
-	return &Handlers{db: db, log: log, auth: a}
+	return &Handlers{items: items, log: log, auth: a}
 }
 
 // Register mounts the routes. Both require an authenticated user; the resolve
@@ -68,7 +70,7 @@ func (h *Handlers) handleResolve(w http.ResponseWriter, r *http.Request, u auth.
 	// item and confirms the caller may read its library, returning the same
 	// ErrNotFound for a missing item and an unreadable one. That keeps an
 	// inaccessible item a 404, never a 403, so its existence is not disclosed.
-	if _, err := catalog.LoadFor(r.Context(), h.db, u, req.ItemID); err != nil {
+	if _, err := h.items.ItemFor(r.Context(), u, req.ItemID); err != nil {
 		if errors.Is(err, catalog.ErrNotFound) {
 			httpx.WriteError(w, http.StatusNotFound, "item not found")
 			return
@@ -80,7 +82,7 @@ func (h *Handlers) handleResolve(w http.ResponseWriter, r *http.Request, u auth.
 	// The first file carries the codec facts the player will try. An item with
 	// no file cannot be played, but is still a 200 with a resolution so the UI
 	// has one shape to render.
-	f, err := catalog.FirstFile(r.Context(), h.db, req.ItemID)
+	f, err := h.items.FirstFile(r.Context(), req.ItemID)
 	if errors.Is(err, catalog.ErrNotFound) {
 		httpx.WriteJSON(w, http.StatusOK, engine.Resolution{
 			Playable: false,

internal/playback/handlers_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/mithral/vellum/internal/auth"
+	"github.com/mithral/vellum/internal/catalog"
 	"github.com/mithral/vellum/internal/core"
 	"github.com/mithral/vellum/internal/engine"
 	"github.com/mithral/vellum/migrations"
@@ -53,7 +54,7 @@ func newHandlers(t *testing.T, db *sql.DB) *Handlers {
 	t.Helper()
 	log := slog.New(slog.NewTextHandler(io.Discard, nil))
 	a := auth.NewAuthenticator(db, "vellum_session", 0, false, log)
-	return NewHandlers(db, log, a)
+	return NewHandlers(catalog.New(db), log, a)
 }
 
 // fixture builds two libraries with one item each and a user granted only the