docs: add decision records for may 23 refactor batch

docs/decisions/2026-05-23-artist-suggestion-service.md

@@ -0,0 +1,57 @@
+# ADR: Extract ArtistSuggestionService from the artists Route Handler
+
+**Date:** 2026-05-23  
+**Status:** Accepted
+
+## Context
+
+`packages/backend/src/routes/artists.ts` (586 LOC) handles the `/artists` HTTP endpoints. It directly contains:
+
+- Spotify artist search (`searchSpotifyArtists`, `getSpotifyRelatedArtists`)
+- Redis caching (`FALLBACK_SUGGESTIONS_CACHE_KEY`, `USER_TOP_ARTISTS_CACHE_PREFIX`)
+- A hardcoded array of 80+ popular artist names used as a last-resort fallback
+- Batch preference handling and artist deduplication logic
+- Direct `getPrismaClient()` calls
+
+This makes the route handler 586 LOC — the largest file in `packages/backend/src/routes/`. The business logic (three-tier artist lookup: user preferences → Spotify → static fallback) is untestable without spinning up Express, mocking Redis, and mocking the Spotify client.
+
+Additionally, the bot has no way to call the same "suggest artists" logic — if a slash command ever needs it, the logic would be duplicated.
+
+## Decision
+
+Extract an `ArtistSuggestionService` (in `packages/backend/src/services/`) that owns the three-tier artist lookup:
+
+```
+Tier 1: User's saved preferences (UserArtistPreference, from Prisma)
+Tier 2: Spotify personalised suggestions (searchSpotifyArtists, getSpotifyRelatedArtists)
+Tier 3: Static popular fallback list (loaded from config, not hardcoded in the route)
+```
+
+The service accepts a `discordUserId` and optional `guildId`, returns `ArtistSuggestion[]`. It owns the caching contract (what TTL, what cache key structure). The route handler becomes a thin HTTP adapter: parse request → call service → format response.
+
+Move the 80+ hardcoded popular artist names to `packages/backend/src/config/popularArtists.ts` (a typed constant, not a runtime dependency).
+
+## Alternatives Considered
+
+**Move into `packages/shared`.** Deferred: the service depends on `SpotifyLink` (Account Link) and Redis, which are backend-only concerns. If the bot ever needs artist suggestions, the right approach is a shared interface with a backend implementation, not moving the backend service to shared.
+
+**Keep as-is, just add tests via supertest.** Rejected: integration tests at the route level still require Redis and Spotify mocks and don't isolate the business logic from the HTTP plumbing.
+
+## Consequences
+
+**Positive:**
+
+- `ArtistSuggestionService` tests: pure unit tests mocking only Spotify client + Prisma. No Express.
+- Route handler shrinks from ~586 LOC to ~50 LOC.
+- Hardcoded fallback list becomes a versioned config constant, easy to update without code changes.
+- Paves the way for bot slash commands to call the same service (via a shared interface, if/when needed).
+
+**Negative:**
+
+- One additional service class to maintain.
+- Caching logic moves from "invisible in the route" to "explicit in the service" — requires documenting the cache invalidation contract.
+
+## Revisit When
+
+- The bot needs artist suggestion logic (promotes `ArtistSuggestionService` to `packages/shared` with a `DiscordWriteAdapter`-style port).
+- Spotify Account Link is deprecated or replaced.

docs/decisions/2026-05-23-autoplay-context-value-object.md

@@ -0,0 +1,73 @@
+# ADR: Introduce AutoplayContext Value Object in the Autoplay Pipeline
+
+**Date:** 2026-05-23  
+**Status:** Accepted
+
+## Context
+
+The Autoplay pipeline (`packages/bot/src/utils/music/autoplay/`) totals ~10,000 LOC across 37 files. The central orchestrator, `replenisher.ts` (637 LOC), calls each Candidate Source collector with 15+ positional arguments. Two symptoms make this painful:
+
+1. **Calling convention brittleness.** `collectRecommendationCandidates` takes 20 positional arguments including `autoplayMode`, `artistFrequency`, `implicitDislikeKeys`, `implicitLikeKeys`, `sessionMood`, `currentFeatures`, `genreContext`, and `blockSertanejo`. Adding any new signal (e.g. "user's blocked genres") requires updating every collector signature, the replenisher call site, and every test that constructs arguments.
+
+2. **Module-level mutable caches.** `sessionMoodCache` and `replenishLocks` live as module-level `Map` instances. They are invisible to callers, survive across test runs without explicit reset, and make test isolation impossible without mocking the entire module.
+
+Both issues prevent unit-testing Candidate Sources in isolation: each collector test must either spin up a real `GuildQueue` (Discord Player dependency) or construct 15+ correct mock arguments.
+
+The vocabulary from `CONTEXT.md` defines **Autoplay**, **Candidate**, **Candidate Source**, and **Replenisher** — the value object codifies what state a replenish needs in terms the domain already names.
+
+## Decision
+
+Introduce an `AutoplayContext` value object that bundles all per-replenish session state:
+
+```typescript
+interface AutoplayContext {
+    readonly guildId: string
+    readonly seedTracks: Track[]
+    readonly excludedUrls: Set<string>
+    readonly excludedKeys: Set<string>
+    readonly trackHistory: TrackHistoryEntry[]
+    readonly sessionMood: SessionMood
+    readonly audioFeatures: SpotifyAudioFeatures | null
+    readonly genreContext: GenreContext | null
+    readonly artistFrequency: Map<string, number>
+    readonly dislikedWeights: Map<string, number>
+    readonly likedWeights: Map<string, number>
+    readonly preferredArtistKeys: Set<string>
+    readonly blockedArtistKeys: Set<string>
+    readonly implicitDislikeKeys: Set<string>
+    readonly implicitLikeKeys: Set<string>
+    readonly autoplayMode: AutoplayMode
+    readonly blockSertanejo: boolean
+    readonly replenishCount: number
+    readonly currentTrack: Track | null
+    readonly recentArtists: string[]
+}
+```
+
+Each Candidate Source collector changes from 15+ positional arguments to `(context: AutoplayContext)`. The Replenisher builds one `AutoplayContext` at the start of each replenish invocation and passes it through. Module-level caches are moved inside the `AutoplayContext` builder (constructed fresh per invocation) — the caches are not part of the value object itself but of the `AutoplayContextBuilder` that reads from stable per-guild state.
+
+## Alternatives Considered
+
+**Keep as-is.** Rejected: the argument-count problem compounds with each new signal. The Phase D roadmap (ADR `2026-05-21-autoplay-recommendation-roadmap.md`) adds at least two new context fields; adding them to 15+ function signatures is unacceptable.
+
+**Pass a plain `Record<string, unknown>` bag.** Rejected: loses type safety, making it harder to detect when a Candidate Source requires a new field.
+
+**Move replenish state to a class instance.** Rejected: mutable class instances have the same test-isolation problems as module-level caches. An immutable value object per invocation is preferable.
+
+## Consequences
+
+**Positive:**
+
+- Each Candidate Source's test becomes: "given this `AutoplayContext`, what Candidates come back?" — a pure function test.
+- Adding a new context field is one change to `AutoplayContext`; all callers get a type error if they fail to supply it.
+- Module-level cache state is no longer implicit; it lives in the builder and can be tested/reset independently.
+
+**Negative:**
+
+- Non-trivial migration: 37 files, ~10k LOC. Collector signatures change across the board. High risk of regressions in the replenish path.
+- `AutoplayContext` must be kept up to date as new Candidate Sources are added; the single type becomes a choke point for context evolution.
+
+## Revisit When
+
+- The Phase D autoplay roadmap adds new Candidate Sources that need context fields not currently in `AutoplayContext`.
+- The Autoplay pipeline is split across multiple workers/processes (would require serialisable context).

docs/decisions/2026-05-23-bot-test-reduction-phase4-replacement-strategy.md

@@ -0,0 +1,141 @@
+# ADR — Bot test reduction Phase 4: deletion + replacement strategy
+
+- **Date:** 2026-05-23
+- **Status:** Accepted
+- **Owner:** Lucas Santana
+- **Supplements:** `docs/decisions/2026-05-09-bot-test-suite-cleanup-strategy.md`
+- **Related:** PRs #938, #939, #940 (Phases 1–3), issue #909 (shared coverage threshold)
+
+## Context
+
+The 2026-05-09 ADR set a ≤1,500-test target for `packages/bot` and established the
+per-file, gate-checked cleanup protocol. Phases 1–3 (PRs #938–#940) executed that
+protocol and reached **2,893 tests** — essentially flat from the starting baseline of
+2,848 because new feature tests were added while delegation tests were removed.
+
+A `/research-and-decide` pass evaluated two strategies for closing the 2,893 → ≤1,500
+gap:
+
+**Strategy A — `it.each` consolidation:** Rewrite repetitive `it()` blocks as
+`it.each` tables. Reduces LOC and improves readability. Does NOT reduce Jest's
+reported test count (each table row is counted as one test). The existing ADR already
+documents this on line 53. Confirmed by independent critic review: Strategy A cannot
+achieve the ≤1,500 target.
+
+**Strategy B — Delete + replace with module-level flow tests:** Delete delegation-only
+test clusters from mixed spec files, paired with coarser module-level flow tests that
+maintain branch coverage with fewer cases. This is the path.
+
+The critic review identified the blocker: "replacement integration tests" was undefined.
+Until Phase 4 execution protocol and replacement test shape are specified, Strategy B
+stalls after Phases 1–3.
+
+## Decision
+
+### 1. The deletion target requires replacement tests — not just deletion
+
+Tests remaining after Phases 1–3 are no longer pure delegation (0 behavioral
+assertions). The mixed files (queueManipulation, autoplay, lastFmApi, etc.) contain
+delegation clusters interleaved with genuine behavioral tests. Deleting the delegation
+clusters without replacement WILL drop below the 65/60/60/65 gate.
+
+Protocol: **delete cluster → add replacement → gate check → commit**. Never commit a
+deletion without a corresponding replacement unless the pre-deletion coverage scan
+confirms zero branch contribution.
+
+### 2. "Replacement test" definition
+
+A replacement test is a **module-level flow test** that:
+
+- Calls the **real function under test** (no mock of the SUT itself).
+- Uses Jest fakes only at external boundaries: Discord.js client, HTTP clients
+  (axios/got/spotify), database services, `autoModService`, etc.
+- Has at least one **behavioral assertion** — a return value, a state change, an
+  emitted event, or a thrown error. `toHaveBeenCalled` alone does not qualify as a
+  replacement.
+- Exercises ≥1 distinct branch that the deleted tests collectively covered.
+
+One well-written replacement test can replace 5–15 delegation-only tests provided it
+covers the same branches via different inputs, not via separate `toHaveBeenCalledWith`
+assertions.
+
+### 3. Execution protocol per file
+
+1. Run `npx jest --coverage --collectCoverageFrom='src/path/to/module.ts'` to get
+   per-file branch baseline.
+2. Identify delegation clusters: consecutive tests where every `expect()` is
+   `toHaveBeenCalled` or `toHaveBeenCalledWith` and no return value is asserted.
+3. Write replacement test(s) first. Verify they pass and cover the same branch paths
+   (coverage delta ≥ 0 on the target file).
+4. Delete the delegation cluster.
+5. Verify the global gate holds:
+   `npx jest --silent --coverage --coverageReporters=text-summary`
+6. Commit the pair as a single atomic change.
+
+### 4. Revised targets for remaining high-count files
+
+| File                        | Current tests | Target | Approach                                                                                          |
+| --------------------------- | ------------- | ------ | ------------------------------------------------------------------------------------------------- |
+| `queueManipulation.spec.ts` | ~114          | ~55    | Delete `toHaveBeenCalled` chains; replace with queue-state assertions                             |
+| `lastFmApi.spec.ts`         | ~90           | ~40    | Delete per-endpoint delegation; replace with response-shape flow test                             |
+| `candidateScorer.spec.ts`   | ~75           | ~30    | Pure algorithmic — consolidate via `it.each` (this one DOES reduce count, it's pure input→output) |
+| `spotifyApi.spec.ts`        | ~70           | ~30    | Same as lastFmApi                                                                                 |
+| Command suites (15 files)   | ~300 total    | ~100   | One flow test per command covering happy path + rejection                                         |
+
+Estimated post-Phase-4 count: **~2,300 tests** (not ≤1,500 in one pass).
+
+### 5. Revised target timeline
+
+The ≤1,500 target remains the long-term goal but requires multiple Phase 4 passes,
+not one. Each pass targets one of the high-count file groups listed above. The
+intermediate checkpoint after the first pass is **≤2,300 tests** with all five groups
+addressed.
+
+Reaching ≤1,500 requires also addressing the autoplay module (~180 tests), scrobbler
+(~60 tests), and guild automation spec files as executors are added. This is ongoing
+work, not a single sprint.
+
+### 6. `it.each` use is allowed — as a secondary maintainability pass
+
+After the deletion+replacement cycle reduces a file's test count, `it.each`
+consolidation of the surviving algorithmic tests is a valid final step. Specifically
+for pure input→output functions (candidateScorer, queueManipulation utility functions),
+`it.each` consolidation DOES reduce test count because multiple previously-separate
+`it()` blocks are merged into one table (but count stays the same per row). This only
+helps when tests share the exact same code path with different inputs — then one
+`it.each` with N rows replaces N `it()` blocks, and the redundant rows can be trimmed
+to the minimal distinguishing set.
+
+## Consequences
+
+- **Positive:** Phase 4 is now unblocked — the replacement test shape and execution
+  protocol are defined.
+- **Positive:** The ≤1,500 target is preserved but explicitly staged: ≤2,300 after
+  first Phase 4 pass, continuing toward ≤1,500 across subsequent passes.
+- **Negative:** Writing replacement flow tests costs more per batch than pure deletion.
+  Estimate 3–4 hours per high-count file (audit + write + delete + verify), not the
+  20-minute batches of Phases 1–3.
+- **Neutral:** Coverage gate (65/60/60/65) remains unchanged. Tighten by 2-3 pp only
+  after the Phase 4 first pass lands and coverage delta is confirmed positive.
+
+## Alternatives considered
+
+- **Raise the target from ≤1,500 to ≤2,000** to reduce Phase 4 scope. Rejected: the
+  gap from 2,893 to 2,000 is achievable with the first Phase 4 pass alone and sets a
+  weak ceiling. Keeping ≤1,500 maintains pressure toward a suite proportional to the
+  codebase size.
+- **Lower the coverage gate temporarily to give deletion headroom.** Rejected by
+  original ADR; this constraint stands.
+- **Run Stryker first to identify which tests are genuinely protective.** Deferred
+  (also in original ADR). Stryker installation is its own project. Phase 4 proceeds
+  with the behavioral-assertion proxy instead.
+
+## Revisit when
+
+- After the first Phase 4 pass (all 5 high-count files addressed): check if ≤2,300
+  was achieved and whether coverage headroom increased enough to tighten the gate.
+- If new executor PRs (Roles, Channels, Onboarding, ReactionRoles, CommandAccess) each
+  add >50 tests, revisit whether the executor test pattern is generating delegation
+  bloat again.
+- If Stryker is installed, run a mutation pass on `autoplay/` and `lastfm/` to
+  validate Phase 4 replacement tests before the second pass.

docs/decisions/2026-05-23-guild-automation-orchestrator-repository-split.md

@@ -0,0 +1,72 @@
+# ADR: Split GuildAutomationService into Orchestrator and Repository
+
+**Date:** 2026-05-23  
+**Status:** Accepted
+
+## Context
+
+`packages/shared/src/services/guildAutomation/service.ts` (534 LOC) is the entry point for the Guild Automation subsystem. It currently mixes:
+
+- **Orchestration logic:** invoke Module Executors, aggregate Run results, handle partial-success
+- **Prisma data access:** upsert `GuildAutomationRun`, `GuildAutomationDrift`, `GuildAutomationManifest`
+- **Lock management:** optimistic locking around concurrent Run invocations
+- **Drift persistence:** writing Drift records after each Module Executor completes
+
+This conflation has two consequences:
+
+1. **Untestable orchestration.** Testing the Run lifecycle (what happens when one Module Executor fails? how is the partial-success result constructed?) requires mocking the entire Prisma layer and the locking mechanism. There is no seam between "Run the executors" and "Persist the outcome."
+
+2. **Premature lock for incomplete work.** Only 2 of 7 Module Executors are implemented (`autoMessagesExecutor`, `moderationExecutor`). Both are shallow pass-throughs. Until the remaining 5 executors (Roles, Channels, Onboarding, ReactionRoles, CommandAccess) exist, the orchestration logic can't be meaningfully tested — the complexity that should live in executors still lives in the service, making the orchestration/persistence conflation invisible.
+
+This ADR codifies the split that the existing `GuildAutomationExecutionService` refactor plan intended (referenced in `docs/decisions/2026-05-19-guild-automation-module-executors.md`) but which was not structurally separated.
+
+## Decision
+
+Split `service.ts` into two modules:
+
+**`GuildAutomationOrchestrator`** (pure logic, no Prisma import):
+
+- Accepts `(manifest, liveState, executors[], ctx)`
+- Calls each Module Executor's Capture/Diff/Apply phases
+- Aggregates per-executor `ExecutorApplyResult` into a Run-level `RunResult`
+- Returns `RunResult` — does not write to the database
+- Depends only on executor interfaces and the `GuildAutomationRepository` interface
+
+**`GuildAutomationRepository`** (Prisma + lock management):
+
+- Owns `GuildAutomationRun`, `GuildAutomationDrift`, `GuildAutomationManifest` reads/writes
+- Owns the optimistic lock protocol
+- Implements the `IGuildAutomationRepository` interface the orchestrator depends on
+
+The existing `service.ts` becomes a thin coordinator that:
+
+1. Calls `repository.acquireLock(guildId)`
+2. Calls `repository.getManifest(guildId)` → `repository.getLiveState(guildId)`
+3. Calls `orchestrator.run(manifest, liveState, executors)`
+4. Calls `repository.persistRunResult(runResult)` + `repository.releaseLock(guildId)`
+
+## Alternatives Considered
+
+**Keep as-is until all 7 executors are built.** Deferred and not rejected: a valid sequencing argument. The split is small enough (~150 LOC refactor) that doing it now avoids building the remaining 5 executors on top of the mixed-concern service.
+
+**Extract just the Prisma layer to a separate file.** Rejected: that's half the split. If we're separating concerns, the full Orchestrator/Repository boundary is cleaner and more testable than a partial extraction.
+
+**Use a transaction script pattern (no orchestrator).** Rejected: transaction scripts co-locate persistence and logic, which is the problem we're solving.
+
+## Consequences
+
+**Positive:**
+
+- `GuildAutomationOrchestrator` is unit-testable with stub executors and a stub repository — no Prisma in scope.
+- Lock protocol changes (e.g., moving from optimistic to advisory) land in `GuildAutomationRepository` only.
+- Each of the remaining 5 Module Executors can be built and tested against the orchestrator interface without touching the Prisma layer.
+
+**Negative:**
+
+- One more interface (`IGuildAutomationRepository`) to maintain.
+- The thin coordinator (`service.ts`) is almost trivially simple — may feel like unnecessary indirection until the full executor suite exists.
+
+## Revisit When
+
+- All 7 Module Executors are implemented and the test suite covers the full Run lifecycle — at that point, reconsider whether the thin coordinator can be collapsed back into the repository.
+- The lock protocol needs to change (e.g., distributed lock across multiple bot instances).