Story #324: Consolidate configuration into Viper Config struct; make Lidarr optional

[joestump]

What / Why

Implements story #324 (part of epic #323, foundation story). ADR-0009's confirmation clause — no direct os.Getenv() for application configuration outside config.Load() — was violated in both binaries, and config validation made Lidarr accidentally mandatory, so the shipped docker-compose examples could not start and SPEC-0017's "submitter SHALL only start if Lidarr is configured" branch was dead code.

Changes

• Shutdown/concurrency keys moved into config (internal/config/config.go, cmd/server/main.go): SPOTTER_SHUTDOWN_TIMEOUT → shutdown_timeout (default 30s), SPOTTER_MAX_CONCURRENT_JOBS → max_concurrent_jobs (default 10). New GetShutdownTimeout() / GetMaxConcurrentJobs() helpers preserve the old fallback-on-invalid behavior. Env var names are unchanged (Viper key replacer maps them 1:1). Governing: ADR-0009, SPEC graceful-shutdown REQ-TMO-005 / REQ-SEM-002.
• cmd/admin uses config.Load() for database driver/DSN instead of hand-rolled os.Getenv + duplicated SQLite defaults; --db flag still overrides. Diff kept surgical to the main() config-loading region (key-rotation logic untouched for Replace IsEncrypted heuristic with explicit ciphertext marker (auth-row bricking) #335). Governing: ADR-0023, ADR-0009.
• Lidarr is optional: validation no longer hard-fails on unset lidarr.base_url/lidarr.api_key — it only rejects partial configuration (one set without the other). New Config.IsLidarrEnabled() gates the Lidarr enricher registration and the background submitter in cmd/server/main.go, and MetadataEnricherOrder() filters lidarr out when unconfigured (avoids per-run "unknown enricher type" warnings without touching internal/services/metadata.go, owned by Fix enrichment batch starvation, image collisions, best-image selection #343). Governing: SPEC-0014 compose scenarios, SPEC-0017 REQ "Background Submitter Goroutine".
• New server.base_url key (default empty) for sync-failure email links; consumed by the email-content story in EPIC: Configuration & Notifications #323. Governing: ADR-0026, SPEC-0015.
• New sync.history_lookback key (default 720h) wired into internal/services/sync.go — first-time history fetch now looks back 30 days instead of syncing from the Unix epoch. Governing: SPEC listen-playlist-sync REQ-SYNC-020.
• Docs: .env.example and docs-site/docs/getting-started/configuration.md updated for all new/moved keys, including the Lidarr both-or-neither rule.

Test Evidence

• make test passes: 27 packages ok, 0 failures (includes codegen).
• New config tests (all passing): TestLidarrOptional, TestLidarrEnabledWhenFullyConfigured, TestLidarrPartialConfigRejected, TestShutdownTimeout (default/override/invalid), TestMaxConcurrentJobs (default/override/non-positive), TestServerBaseURL, TestSyncHistoryLookback (default/override/invalid), TestMetadataEnricherOrderExcludesLidarrWhenUnconfigured — proving SPOTTER_* env vars still bind through Viper.
• Acceptance grep: grep -rn "os.Getenv" cmd/ internal/ --include='*.go' returns only comments — zero application-config reads outside internal/config.
• gofmt -l cmd internal is empty; go vet clean.
• Startup smoke tests (no Lidarr env vars):
  • postgres driver pointing at a closed port: config validation passes and the process reaches DB connection (failed to connect to database ... connection refused) — the SPEC-0014 compose "MUST start" gate.
  • sqlite full startup: logs lidarr enricher disabled (lidarr not configured) and lidarr queue submitter disabled (lidarr not configured), then starting server.

Notes for Reviewers

• cmd/admin rotate-key now requires the full server config env (Navidrome URL, OpenAI key, encryption key, JWT secret) since config.Load() validates required fields. In deployed environments the admin tool runs with the server's env so this holds; flagging in case a standalone-DSN-only usage matters.
• Partial Lidarr config (one of the two vars) is now an explicit startup error rather than silently required — deliberate fail-fast per ADR-0009.

Closes #324

Part of epic #323. Governing artifacts: ADR-0009 (Viper configuration), ADR-0023, ADR-0026, ADR-0015, SPEC-0014 (multi-database-support), SPEC-0015 (sync-failure-email-notifications), SPEC-0017 REQ "Background Submitter Goroutine", SPEC graceful-shutdown REQ-TMO-005 / REQ-SEM-002, SPEC listen-playlist-sync REQ-SYNC-020.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Verdict: CHANGES NEEDED — one real defect (the cmd/admin full-config regression); everything else verified sound. I independently re-ran the acceptance greps, the full test suite, and startup/CLI smoke tests on a detached checkout of 14b85ac.

Findings

1. MEDIUM (blocking): rotate-key no longer runs standalone — breaks the key-rotation spec's documented invocation

cmd/admin/main.go:57-61 now calls config.Load(), which enforces the server's full validation set. Verified empirically against a build of this branch:

$ env -i ./spotter-admin rotate-key --old-key=<64hex> --new-key=<64hex> --db="file:test.db?_fk=1"
Error: failed to load config: navidrome.base_url is required

docs/openspec/specs/key-rotation/spec.md (Scenario 1 and Implementation Notes) documents rotate-key as needing only --old-key/--new-key plus SPOTTER_DATABASE_DRIVER/SPOTTER_DATABASE_SOURCE/--db — server stopped, no other env. On main that invocation works (sqlite defaults); on this branch it exits 1 unless the operator also exports SPOTTER_NAVIDROME_BASE_URL, SPOTTER_OPENAI_API_KEY, SPOTTER_SECURITY_JWT_SECRET (32+ chars), and a valid 64-hex SPOTTER_SECURITY_ENCRYPTION_KEY — the last one being pure ceremony, since rotate-key takes both keys via flags. This is a credential-recovery tool; requiring an OpenAI API key to re-encrypt DB rows is a real ops regression, and the author's own reviewer note flags exactly this case.

Suggested fix that still satisfies issue #324's requirement (no hand-rolled os.Getenv, no duplicated SQLite defaults): a scoped loader in internal/config — e.g. LoadDatabase() returning the database section, sharing the same Viper instance setup, driver validation (SPEC-0014 REQ "Driver Validation"), and driver-specific default sources — and have cmd/admin call that. Alternatively, keep config.Load() and update docs/openspec/specs/key-rotation/spec.md to declare the new env contract — but that weakens a recovery-path tool and touches spec files #335 also depends on, so the scoped loader is the cleaner move.

2. LOW: non-numeric SPOTTER_MAX_CONCURRENT_JOBS now hard-fails startup; PR body overstates fallback preservation

Old code (strconv.Atoi err → keep 10) started fine with SPOTTER_MAX_CONCURRENT_JOBS=abc. Now v.Unmarshal fails first, so Load() returns a raw mapstructure decode error and the server exits — GetMaxConcurrentJobs() (internal/config/config.go:252) never sees it; its fallback only covers non-positive values (the only case tested, config_test.go:427). Fail-fast is defensible under ADR-0009, but (a) the PR body's "preserve the old fallback-on-invalid behavior" claim is not true for this input, and (b) the surfaced error is a decoder dump, not an ADR-0009-style clear message. Add a test pinning whichever behavior is intended, and consider wrapping the unmarshal error with the offending key.

3. LOW: negative durations pass validation and produce degenerate behavior

• SPOTTER_SHUTDOWN_TIMEOUT=-5s → GetShutdownTimeout() returns -5s (verified) → context.WithTimeout is already expired → zero-grace shutdown. Pre-existing hole (old code had it too), but this PR is the natural place to close it.
• SPOTTER_SYNC_HISTORY_LOOKBACK=-720h → GetSyncHistoryLookback() returns -720h (verified) → since = time.Now().Add(-lookback) at internal/services/sync.go:272 lands in the future → first sync silently fetches nothing. New surface introduced here.

Both helpers should treat d <= 0 like unparsable and fall back to the default.

4. LOW: SPOTTER_METADATA_ORDER=lidarr with Lidarr unconfigured yields an empty enricher order, silently

MetadataEnricherOrder() (internal/config/config.go:207) filters lidarr out with no signal; getActiveEnrichers then activates nothing and enrichment no-ops with only debug-level breadcrumbs. Verified: order comes back []. A startup Warn when the filter empties an explicitly-configured order would save someone a confusing afternoon. Non-blocking.

5. NIT: top-level shutdown_timeout / max_concurrent_jobs keys break the namespaced key scheme

Every other key is namespaced (server.*, sync.*); these two are top-level solely to keep the legacy env names. Understandable trade-off, and the env names must not change — but a v.BindEnv("server.shutdown_timeout", "SPOTTER_SHUTDOWN_TIMEOUT") alias would have allowed namespaced keys too. Worth a comment in Load() noting the constraint so a future config-file story doesn't "fix" it and break the env contract. Non-blocking.

6. NIT: no sync-level test for the first-sync watermark

sync_test.go's mockProvider.GetRecentListens ignores since, so nothing asserts the no-history path now passes ~now-720h instead of epoch — the riskiest behavioral change in the PR is covered only at the config-helper level. Also worth a look for #327: for server.base_url (#345), consider trimming a trailing / at load time so every consumer doesn't have to. Non-blocking.

Verified sound (independently reproduced)

• grep -rn "os.Getenv" cmd/ internal/ --include='*.go' → comments only. ✔
• make test passes end-to-end on the branch; gofmt -l cmd internal empty; go vet clean. ✔
• No-Lidarr startup: postgres driver at a closed port passes validation and reaches DB connect (SPEC-0014 gate); sqlite full start logs both "lidarr enricher disabled" and "lidarr queue submitter disabled" then "starting server". ✔
• Partial Lidarr (base URL only) fails fast with the both-or-neither error. ✔
• Legacy env binding: SPOTTER_SHUTDOWN_TIMEOUT/SPOTTER_MAX_CONCURRENT_JOBS bind through the Viper replacer (existing tests, plus probed empty-string env → defaults hold, matching the ADR-0009 empty-string caveat). ✔
• Lidarr-disabled blast radius is contained: templates already render no links for empty LidarrBaseURL (internal/views/components/track_table.templ:213), and the only LidarrQueue enqueue path is the now-gated enricher, so no orphaned queue growth. ✔
• .env.example correctly commented out both Lidarr placeholders — the old example (URL set, key empty) would have tripped the new xor validation. ✔
• API shape for in-flight/downstream PRs looks fine: metadata.go untouched (Fix enrichment batch starvation, image collisions, best-image selection #343), cfg.Server.BaseURL and GetSyncHistoryLookback() are reasonable for Fix sync-failure email links, subjects, and noop logging #345/Fix listen persistence: same-provider repeats, dedup key, history lookback #327; the cmd/admin diff is surgical around the region Replace IsEncrypted heuristic with explicit ciphertext marker (auth-row bricking) #335 touches.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Addressed the review findings in d44a92f:

BLOCKING — standalone rotate-key restored. Added a scoped config.LoadDatabase(): same Viper setup as Load() (SPOTTER_* prefix, dot-to-underscore replacer, registered defaults) plus driver validation and driver-specific default DSNs, but database-scope validation only. Load() was refactored to share newViper() and validateAndDefaultDatabase() so there is no duplicated logic. cmd/admin now calls LoadDatabase(), and it preserves the pre-consolidation empty-env-var fallbacks (Viper treats empty strings as set). Verified end-to-end: env -i ./spotter-admin rotate-key --old-key=... --new-key=... --db=file:rotate-test.db gets past config loading with a completely empty environment and proceeds to pre-rotation DB validation — SPEC key-rotation Scenario 1 holds again, with zero os.Getenv.

Strongly recommended items, all done:

• Non-numeric SPOTTER_MAX_CONCURRENT_JOBS now fails Load() with a clear validation error (max_concurrent_jobs must be an integer (set SPOTTER_MAX_CONCURRENT_JOBS), got "lots") via a pre-unmarshal check, instead of a raw mapstructure decode error. Chose fail-fast over silent fallback per ADR-0009. Test added.
• GetShutdownTimeout() and GetSyncHistoryLookback() treat d <= 0 as invalid and fall back to 30s / 720h — no more zero-grace shutdowns from -5s or future-dated first-sync watermarks from a negative lookback. Tests for negative and zero values added.
• MetadataEnricherOrder() now logs at WARN when filtering unconfigured enrichers empties the order (e.g. SPOTTER_METADATA_ORDER=lidarr with Lidarr unset), so a no-op enrichment pipeline is visible. Behavior test added.

Optional nits:

• Added CONSTRAINT comments on ShutdownTimeout/MaxConcurrentJobs explaining they are deliberately top-level: the published env var names from SPEC graceful-shutdown REQ-TMO-005/REQ-SEM-002 must remain stable, and under the dot-to-underscore replacer only top-level keys map to them.
• server.base_url trailing slashes are now trimmed in Load() (helps the Fix sync-failure email links, subjects, and noop logging #345 email-links consumer). Test added.
• Skipped the sync-level watermark test: the since computation is a two-line delegation to the now-tested GetSyncHistoryLookback(), and a Syncer-level test would need provider mocks disproportionate to the assertion.

make test: 27 packages ok, 0 failures. gofmt -l cmd internal empty, go vet clean.

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Verdict: LGTM — delta re-review of d44a92f, all findings resolved. Independently re-verified on a fresh detached checkout:

• Blocking finding fixed. env -i ./spotter-admin rotate-key --old-key=… --new-key=… --db=file:… now passes config loading with a completely empty environment and proceeds to pre-rotation DB validation (SPEC key-rotation Scenario 1 holds). Also probed: SPOTTER_DATABASE_DRIVER=postgres with empty source picks up the driver-specific default DSN; invalid driver gets the clear SPEC-0014 error; and LoadDatabase() is correctly insulated from broken server env (garbage SPOTTER_MAX_CONCURRENT_JOBS/short encryption key don't affect it).
• Helpers hardened as claimed. Non-numeric SPOTTER_MAX_CONCURRENT_JOBS → clear must be an integer … got "lots" error (empty string still defaults to 10; positive overrides unaffected). -5s shutdown and -720h lookback both fall back to defaults. server.base_url trailing slashes trimmed (verified with ///). Empty-after-filter enricher order emits the WARN.
• No behavior drift from the newViper()/validateAndDefaultDatabase() extraction — re-ran my original probe matrix (no-Lidarr load, partial-Lidarr rejection, empty-string env edges, env binding) plus make test (green), gofmt -l (empty), go vet (clean).

Good to merge.

🤖 Posted on behalf of @joestump by Claude.