Address Gadfly review findings on Phase 0

Applied the fixes warranted by the adversarial review of PR #21 (all
findings graded in the gadfly store):

Store (highest impact):
- buildDSN always merges busy_timeout/journal_mode/foreign_keys pragmas
  into the DSN, even for file: URIs or paths with a query string — the
  prior passthrough silently disabled FK enforcement for those PANSY_DB
  values. Also escape '#' in the path and tighten in-memory detection to
  an exact ':memory:' token or mode=memory param (no substring misfire).
- migrate.go: detect duplicate migration versions with a clear error;
  wrap appliedVersions' rows.Err() with the store: prefix.

API:
- SetTrustedProxies failure now falls back to trust-none instead of
  leaving gin's trust-everyone default (X-Forwarded-For spoofing).
- SPA: 404 embedded directories (was a directory listing), 404 bare /api,
  add X-Content-Type-Options: nosniff, cache index.html bytes once at
  startup, single fs.Stat existence check, shared writeAPIError helper.
- Move gin.SetMode out of package init() into New().

Config: validate PANSY_PORT range (fall back to 8080 with a warning).

Web:
- api.ts: serialize the request body before the fetch try so a
  JSON.stringify failure isn't misreported as a network error.
- AppShell: move state-specific/conflicting utilities into
  active/inactiveProps so concatenated Tailwind classes don't collide.

Tests: added store DSN-pragma/memory-detection cases and SPA
directory-404 case. go build/vet/test clean (CGO off); web tsc + build
clean; re-verified in a browser (SPA, deep links, /assets 404, nav).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi

internal/api/api.go

@@ -15,21 +15,22 @@ import (
 	"gitea.stevedudenhoeffer.com/steve/pansy/internal/config"
 )
 
-func init() {
-	// Quiet gin's default debug output; we log via slog-gin instead.
-	gin.SetMode(gin.ReleaseMode)
-}
-
 // New builds the gin engine with the standard middleware stack and registers the
 // API routes. The embedded SPA fallback is registered separately by the caller
 // via RegisterSPA (see spa.go) so the API can be built and tested without a web
 // build present.
 func New(cfg *config.Config) *gin.Engine {
+	gin.SetMode(gin.ReleaseMode)
+
 	r := gin.New()
 	r.Use(sloggin.New(slog.Default()), gin.Recovery())
 
 	if err := r.SetTrustedProxies(cfg.TrustedProxies); err != nil {
-		slog.Warn("api: failed to set trusted proxies", "error", err)
+		// Do not leave gin's trust-everyone default active on a parse failure —
+		// that would let any client spoof X-Forwarded-For. Fall back to trusting
+		// no proxies, which is also the behavior when none are configured.
+		slog.Error("api: invalid trusted proxies, trusting none", "error", err)
+		_ = r.SetTrustedProxies(nil)
 	}
 
 	v1 := r.Group("/api/v1")

internal/api/spa.go

@@ -17,30 +17,44 @@ import (
 func RegisterSPA(r *gin.Engine, dist fs.FS) {
 	fileServer := http.FileServer(http.FS(dist))
 
+	// index.html is served on every client-route deep link; read it once at
+	// startup rather than re-reading the embedded FS per request. A build without
+	// it is a packaging bug, so fail loudly.
+	index, err := fs.ReadFile(dist, "index.html")
+	if err != nil {
+		panic("api: embedded web build missing index.html: " + err.Error())
+	}
+
 	r.NoRoute(func(c *gin.Context) {
 		reqPath := c.Request.URL.Path
 
-		if strings.HasPrefix(reqPath, "/api/") {
-			c.JSON(http.StatusNotFound, gin.H{"error": gin.H{
-				"code": "NOT_FOUND", "message": "no such endpoint",
-			}})
+		// API paths (including a bare "/api") are never the SPA: a miss is a real
+		// JSON 404, not the app shell.
+		if reqPath == "/api" || strings.HasPrefix(reqPath, "/api/") {
+			writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "no such endpoint")
 			return
 		}
 
 		if c.Request.Method != http.MethodGet && c.Request.Method != http.MethodHead {
-			c.JSON(http.StatusMethodNotAllowed, gin.H{"error": gin.H{
-				"code": "METHOD_NOT_ALLOWED", "message": "method not allowed",
-			}})
+			writeAPIError(c, http.StatusMethodNotAllowed, "METHOD_NOT_ALLOWED", "method not allowed")
 			return
 		}
 
+		c.Header("X-Content-Type-Options", "nosniff")
+
 		name := strings.TrimPrefix(path.Clean(reqPath), "/")
 		if name == "" {
-			name = "index.html"
+			serveIndex(c, index)
+			return
 		}
 
-		if f, err := dist.Open(name); err == nil {
-			f.Close()
+		if info, statErr := fs.Stat(dist, name); statErr == nil {
+			// An embedded directory is neither a client route nor a servable file;
+			// serving it would leak a directory listing, so it is a 404.
+			if info.IsDir() {
+				writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "not found")
+				return
+			}
 			// Vite emits content-hashed filenames under assets/, so those are safe
 			// to cache forever; index.html must always revalidate to pick up new builds.
 			if strings.HasPrefix(name, "assets/") {
@@ -56,23 +70,22 @@ func RegisterSPA(r *gin.Engine, dist fs.FS) {
 		// falling back to index.html would return HTML with a 200 for a .js/.css
 		// request, which the browser rejects with a confusing MIME-type error.
 		if strings.HasPrefix(name, "assets/") {
-			c.JSON(http.StatusNotFound, gin.H{"error": gin.H{
-				"code": "NOT_FOUND", "message": "asset not found",
-			}})
+			writeAPIError(c, http.StatusNotFound, "NOT_FOUND", "asset not found")
 			return
 		}
 
-		serveIndex(c, dist)
+		serveIndex(c, index)
 	})
 }
 
-// serveIndex writes index.html as the SPA fallback for an unmatched client route.
-func serveIndex(c *gin.Context, dist fs.FS) {
-	data, err := fs.ReadFile(dist, "index.html")
-	if err != nil {
-		c.String(http.StatusInternalServerError, "index.html missing from build")
-		return
-	}
+// serveIndex writes the cached index.html as the SPA fallback (no-cache so new
+// builds are picked up). net/http suppresses the body for HEAD requests.
+func serveIndex(c *gin.Context, index []byte) {
 	c.Header("Cache-Control", "no-cache")
-	c.Data(http.StatusOK, "text/html; charset=utf-8", data)
+	c.Data(http.StatusOK, "text/html; charset=utf-8", index)
+}
+
+// writeAPIError writes pansy's standard JSON error envelope.
+func writeAPIError(c *gin.Context, status int, code, message string) {
+	c.JSON(status, gin.H{"error": gin.H{"code": code, "message": message}})
 }

internal/api/spa_test.go

@@ -77,6 +77,16 @@ func TestSPAMissingAssetIs404(t *testing.T) {
 	}
 }
 
+func TestSPADirectoryIs404NotListing(t *testing.T) {
+	// A request that resolves to an embedded directory must 404, never a listing.
+	for _, p := range []string{"/assets", "/assets/"} {
+		w := do(t, testEngine(), http.MethodGet, p)
+		if w.Code != http.StatusNotFound {
+			t.Errorf("GET %s status = %d, want 404", p, w.Code)
+		}
+	}
+}
+
 func TestSPAUnmatchedAPIIs404JSON(t *testing.T) {
 	w := do(t, testEngine(), http.MethodGet, "/api/v1/nope")
 	if w.Code != http.StatusNotFound {

internal/config/config.go

@@ -84,6 +84,11 @@ func Load() *Config {
 		cfg.Registration = RegistrationOpen
 	}
 
+	if cfg.Port < 1 || cfg.Port > 65535 {
+		slog.Warn("config: PANSY_PORT out of range, using default", "value", cfg.Port, "default", 8080)
+		cfg.Port = 8080
+	}
+
 	return cfg
 }
 

internal/store/migrate.go

@@ -69,7 +69,10 @@ func (d *DB) appliedVersions(ctx context.Context) (map[int]bool, error) {
 		}
 		applied[v] = true
 	}
-	return applied, rows.Err()
+	if err := rows.Err(); err != nil {
+		return nil, fmt.Errorf("store: iterate schema_migrations: %w", err)
+	}
+	return applied, nil
 }
 
 func (d *DB) applyMigration(ctx context.Context, m migration) error {
@@ -99,6 +102,7 @@ func loadMigrations() ([]migration, error) {
 	}
 
 	var migrations []migration
+	seen := map[int]string{}
 	for _, e := range entries {
 		if e.IsDir() || !strings.HasSuffix(e.Name(), ".sql") {
 			continue
@@ -107,6 +111,10 @@ func loadMigrations() ([]migration, error) {
 		if err != nil {
 			return nil, err
 		}
+		if prev, dup := seen[version]; dup {
+			return nil, fmt.Errorf("store: duplicate migration version %d: %q and %q", version, prev, e.Name())
+		}
+		seen[version] = e.Name()
 		body, err := fs.ReadFile(migrationsFS, "migrations/"+e.Name())
 		if err != nil {
 			return nil, fmt.Errorf("store: read migration %s: %w", e.Name(), err)

internal/store/sqlite.go

@@ -13,6 +13,16 @@ import (
 	_ "modernc.org/sqlite" // pure-Go driver, registered as "sqlite"
 )
 
+// requiredPragmas are applied to every connection (they are per-connection in
+// SQLite): wait rather than fail on a contended write, use WAL journaling, and
+// enforce foreign keys. They are attached to the DSN so the whole pool inherits
+// them.
+var requiredPragmas = []string{
+	"busy_timeout(5000)",
+	"journal_mode(WAL)",
+	"foreign_keys(1)",
+}
+
 // DB is a handle to the SQLite database backing pansy. Safe for concurrent use:
 // WAL mode allows many readers with one writer, and busy_timeout makes a
 // contended writer wait rather than fail with SQLITE_BUSY.
@@ -22,10 +32,11 @@ type DB struct {
 
 // Open opens (creating if absent) the SQLite database at path and returns a DB.
 // The pragmas — busy_timeout, WAL journal, and foreign-key enforcement — are
-// applied per connection via the DSN so every pooled connection inherits them.
-// A path of ":memory:" yields an ephemeral in-memory database (useful in tests);
-// the pool is pinned to a single connection so all queries see the same DB.
-// The caller owns the returned DB and must Close it.
+// applied per connection via the DSN so every pooled connection inherits them,
+// regardless of whether path is a bare filename, a path with query parameters,
+// or a full file: URI. A path of ":memory:" yields an ephemeral in-memory
+// database (useful in tests); the pool is pinned to a single connection so all
+// queries see the same DB. The caller owns the returned DB and must Close it.
 func Open(path string) (*DB, error) {
 	dsn, memory := buildDSN(path)
 	sqldb, err := sql.Open("sqlite", dsn)
@@ -45,20 +56,47 @@ func Open(path string) (*DB, error) {
 	return &DB{sql: sqldb}, nil
 }
 
-// buildDSN turns a filesystem path (or ":memory:") into a modernc DSN carrying
-// the pragmas we want on every connection, and reports whether it is in-memory.
-// A path that already looks like a DSN (has a scheme or query) is passed through
-// untouched.
+// buildDSN converts the configured path into a modernc DSN that always carries
+// the required pragmas, and reports whether it is an in-memory database. It
+// accepts a bare filename, a path with existing query parameters, or a full
+// file: URI, and merges the pragmas without clobbering any the user supplied.
 func buildDSN(path string) (dsn string, memory bool) {
-	memory = path == ":memory:" || strings.Contains(path, ":memory:") || strings.Contains(path, "mode=memory")
-	if strings.HasPrefix(path, "file:") || strings.Contains(path, "?") {
-		return path, memory
+	body := strings.TrimPrefix(path, "file:")
+
+	filePart, rawQuery, _ := strings.Cut(body, "?")
+	query := url.Values{}
+	if q, err := url.ParseQuery(rawQuery); err == nil {
+		query = q
 	}
-	pragmas := url.Values{}
-	pragmas.Add("_pragma", "busy_timeout(5000)")
-	pragmas.Add("_pragma", "journal_mode(WAL)")
-	pragmas.Add("_pragma", "foreign_keys(1)")
-	return "file:" + path + "?" + pragmas.Encode(), memory
+
+	// Detect in-memory precisely: the exact ":memory:" token, or an explicit
+	// mode=memory parameter. A substring match would misfire on real file paths.
+	memory = filePart == ":memory:" || query.Get("mode") == "memory"
+
+	have := make(map[string]bool, len(query["_pragma"]))
+	for _, p := range query["_pragma"] {
+		have[pragmaName(p)] = true
+	}
+	for _, p := range requiredPragmas {
+		if !have[pragmaName(p)] {
+			query.Add("_pragma", p)
+		}
+	}
+
+	// A '#' in a filesystem path would otherwise start a URI fragment and drop
+	// the query string that carries the pragmas.
+	filePart = strings.ReplaceAll(filePart, "#", "%23")
+	return "file:" + filePart + "?" + query.Encode(), memory
+}
+
+// pragmaName extracts the pragma's name from a "_pragma" value such as
+// "busy_timeout(5000)" or "foreign_keys=1", lowercased for case-insensitive
+// de-duplication against a user-supplied value.
+func pragmaName(p string) string {
+	if i := strings.IndexAny(p, "(="); i >= 0 {
+		p = p[:i]
+	}
+	return strings.ToLower(strings.TrimSpace(p))
 }
 
 // Close closes the underlying database.

internal/store/store_test.go

@@ -3,6 +3,7 @@ package store
 import (
 	"context"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -102,6 +103,63 @@ func TestCheckConstraintRejectsBadEnum(t *testing.T) {
 	}
 }
 
+func TestForeignKeysEnforcedForFileURIDSN(t *testing.T) {
+	// A file: URI (or any path with query params) must still get foreign_keys(1):
+	// the earlier passthrough silently dropped the pragmas for these DSNs.
+	p := filepath.Join(t.TempDir(), "uri.db")
+	db, err := Open("file:" + p + "?_pragma=synchronous(1)")
+	if err != nil {
+		t.Fatalf("Open file: URI: %v", err)
+	}
+	t.Cleanup(func() { db.Close() })
+	if err := db.Migrate(context.Background()); err != nil {
+		t.Fatalf("Migrate: %v", err)
+	}
+	_, err = db.SQL().Exec(
+		`INSERT INTO plantings (object_id, plant_id, x_cm, y_cm, radius_cm) VALUES (999, 999, 0, 0, 10)`,
+	)
+	if err == nil {
+		t.Fatal("expected foreign-key violation for file: URI DSN, got nil")
+	}
+}
+
+func TestBuildDSNAlwaysIncludesPragmas(t *testing.T) {
+	for _, in := range []string{
+		"./pansy.db",
+		"/data/pansy.db",
+		"file:/data/pansy.db",
+		"file:/data/pansy.db?cache=shared",
+		"/tmp/weird?name.db",
+	} {
+		dsn, _ := buildDSN(in)
+		for _, want := range []string{"busy_timeout", "journal_mode", "foreign_keys"} {
+			if !strings.Contains(dsn, want) {
+				t.Errorf("buildDSN(%q) = %q, missing %s pragma", in, dsn, want)
+			}
+		}
+	}
+}
+
+func TestBuildDSNDoesNotDuplicateUserPragma(t *testing.T) {
+	// A user-supplied busy_timeout must not be duplicated by our default.
+	dsn, _ := buildDSN("file:/data/x.db?_pragma=busy_timeout(9000)")
+	if strings.Count(dsn, "busy_timeout") != 1 {
+		t.Errorf("buildDSN kept both user and default busy_timeout: %q", dsn)
+	}
+}
+
+func TestBuildDSNMemoryDetection(t *testing.T) {
+	if _, mem := buildDSN(":memory:"); !mem {
+		t.Error(":memory: not detected as in-memory")
+	}
+	if _, mem := buildDSN("file:x.db?mode=memory"); !mem {
+		t.Error("mode=memory not detected as in-memory")
+	}
+	if _, mem := buildDSN("/var/lib/pansy/pansy.db"); mem {
+		t.Error("file path wrongly detected as in-memory")
+	}
+}
+
 func TestInMemoryDBIsCoherentAcrossQueries(t *testing.T) {
 	// A bare ":memory:" DSN gives each connection its own DB; Open must pin the
 	// pool so the migrated schema is visible to subsequent queries.

web/src/components/layout/AppShell.tsx

@@ -1,11 +1,17 @@
 import { Link, Outlet } from '@tanstack/react-router'
-import { cn } from '@/lib/cn'
 
 const navLinks = [
   { to: '/gardens', label: 'Gardens' },
   { to: '/plants', label: 'Plants' },
 ] as const
 
+// TanStack Router concatenates the base className with activeProps/inactiveProps,
+// so state-specific and conflicting utilities (text-muted vs text-fg) live in the
+// state props — never in the base — to avoid ambiguous overrides.
+const navLinkBase = 'rounded-md px-3 py-1.5 text-sm font-medium transition-colors'
+const navLinkActive = 'bg-border/60 text-fg'
+const navLinkInactive = 'text-muted hover:bg-border/60 hover:text-fg'
+
 /** Top-level chrome: a sticky nav bar plus the routed page in an <Outlet>. */
 export function AppShell() {
   return (
@@ -20,10 +26,9 @@ export function AppShell() {
               <Link
                 key={l.to}
                 to={l.to}
-                className={cn(
-                  'rounded-md px-3 py-1.5 text-sm font-medium text-muted transition-colors hover:bg-border/60 hover:text-fg',
-                )}
-                activeProps={{ className: 'bg-border/60 text-fg' }}
+                className={navLinkBase}
+                activeProps={{ className: navLinkActive }}
+                inactiveProps={{ className: navLinkInactive }}
               >
                 {l.label}
               </Link>

web/src/lib/api.ts

@@ -84,6 +84,10 @@ function messageFrom(body: unknown, status: number): string {
 export async function apiFetch<T>(path: string, opts: RequestOptions = {}): Promise<T> {
   const { method = 'GET', body, params, signal } = opts
 
+  // Serialize before the try so a JSON.stringify failure (e.g. a circular value)
+  // surfaces as itself, not as a misleading "cannot reach the server" error.
+  const requestBody = body !== undefined ? JSON.stringify(body) : undefined
+
   let res: Response
   try {
     res = await fetch(buildUrl(path, params), {
@@ -94,7 +98,7 @@ export async function apiFetch<T>(path: string, opts: RequestOptions = {}): Prom
         accept: 'application/json',
         ...(body !== undefined ? { 'content-type': 'application/json' } : {}),
       },
-      body: body !== undefined ? JSON.stringify(body) : undefined,
+      body: requestBody,
     })
   } catch (err) {
     if ((err as Error)?.name === 'AbortError') throw err