diff --git a/docs/decisions/2026-05-23-artist-suggestion-service.md b/docs/decisions/2026-05-23-artist-suggestion-service.md new file mode 100644 index 000000000..58e28fda4 --- /dev/null +++ b/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. diff --git a/docs/decisions/2026-05-23-autoplay-context-value-object.md b/docs/decisions/2026-05-23-autoplay-context-value-object.md new file mode 100644 index 000000000..6adc15967 --- /dev/null +++ b/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 + readonly excludedKeys: Set + readonly trackHistory: TrackHistoryEntry[] + readonly sessionMood: SessionMood + readonly audioFeatures: SpotifyAudioFeatures | null + readonly genreContext: GenreContext | null + readonly artistFrequency: Map + readonly dislikedWeights: Map + readonly likedWeights: Map + readonly preferredArtistKeys: Set + readonly blockedArtistKeys: Set + readonly implicitDislikeKeys: Set + readonly implicitLikeKeys: Set + 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` 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). diff --git a/docs/decisions/2026-05-23-bot-test-reduction-phase4-replacement-strategy.md b/docs/decisions/2026-05-23-bot-test-reduction-phase4-replacement-strategy.md new file mode 100644 index 000000000..361c09646 --- /dev/null +++ b/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. diff --git a/docs/decisions/2026-05-23-guild-automation-orchestrator-repository-split.md b/docs/decisions/2026-05-23-guild-automation-orchestrator-repository-split.md new file mode 100644 index 000000000..d6e2998bc --- /dev/null +++ b/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). diff --git a/docs/decisions/2026-05-23-message-pipeline-handler-chain.md b/docs/decisions/2026-05-23-message-pipeline-handler-chain.md new file mode 100644 index 000000000..46e9961cb --- /dev/null +++ b/docs/decisions/2026-05-23-message-pipeline-handler-chain.md @@ -0,0 +1,71 @@ +# ADR: Replace messageHandler Kitchen-Sink with a MessagePipeline Chain + +**Date:** 2026-05-23 +**Status:** Accepted + +## Context + +`packages/bot/src/handlers/messageHandler.ts` (322 LOC) is a single Discord `messageCreate` listener that sequentially runs: + +- AutoMod enforcement +- Spam detection +- CustomCommand matching +- XP/leveling reward + +These four concerns share no state but are hard-coupled in a single function. Consequences: + +1. **Implicit ordering.** AutoMod can delete a message, but the remaining concerns still run — there is no early-exit contract. +2. **Test isolation is impossible.** Testing XP leveling requires mocking `AutoModService`, `CustomCommandService`, and spam detection, because they all live in the same function scope. +3. **Feature toggle checks are inline.** If the DB is down during the `isEnabled()` check, AutoMod silently skips. The fallback behaviour is not visible at the handler level. +4. **Adding a new concern** (e.g., birthday detection) means editing the existing function rather than adding a new handler alongside it. + +## Decision + +Replace the monolithic handler with a `MessagePipeline` that runs an ordered list of typed message handlers: + +```typescript +interface MessageHandler { + name: string + canHandle(message: Message, context: MessageContext): Promise + handle( + message: Message, + context: MessageContext, + ): Promise +} + +interface MessageHandlerResult { + stop: boolean // true = do not run subsequent handlers +} +``` + +A `MessagePipeline` runs handlers in declared order. Each handler's `canHandle()` guard is responsible for the feature-toggle check (one place per concern). If any handler returns `stop: true`, the pipeline halts. The four current concerns become four `MessageHandler` implementations: `AutoModHandler`, `SpamHandler`, `CustomCommandHandler`, `XpHandler`. + +`MessageContext` is a value object built once per message: resolved `Guild`, `Member`, `GuildSettings`, and the feature-toggle map for the message's Guild. + +## Alternatives Considered + +**Event bus (publish/subscribe).** Rejected: an event bus decouples producers from consumers but makes ordering non-deterministic. AutoMod must run before XP leveling (a deleted message should not grant XP). Explicit ordered chains preserve the ordering contract. + +**Keep as-is, just add more functions.** Rejected: compounds the existing isolation problem; every new concern inherits the full mock surface of every prior concern. + +**Move to a Saga / state machine.** Rejected: over-engineered for a linear pipeline. The chain pattern (as used in Express middleware) is well-understood, simpler, and sufficient. + +## Consequences + +**Positive:** + +- Each handler's unit test only needs its own mocks — no cross-concern leakage. +- Adding a new concern is a new file; `messageHandler.ts` (which becomes `messagePipeline.ts`) is never edited. +- The `stop: true` contract makes AutoMod's early-exit explicit and testable. +- Feature-toggle failures are isolated to each handler's `canHandle()` — a DB outage in AutoMod doesn't affect XP leveling. + +**Negative:** + +- More files (one per handler + pipeline runner). +- `MessageContext` becomes a new type to maintain as new fields are needed. +- Ordering is now configuration, not code — developers must know where to insert a new handler in the chain. + +## Revisit When + +- A handler needs to communicate state to a later handler (e.g., AutoMod detection result used by XP). Extend `MessageContext` to carry it. +- The number of message handlers exceeds ~10 (consider a priority-sorted registry instead of a static array). diff --git a/docs/decisions/2026-05-23-recommendation-engine-single-entrypoint.md b/docs/decisions/2026-05-23-recommendation-engine-single-entrypoint.md new file mode 100644 index 000000000..5d4f3c46a --- /dev/null +++ b/docs/decisions/2026-05-23-recommendation-engine-single-entrypoint.md @@ -0,0 +1,71 @@ +# ADR: Collapse Recommendation Engine to a Single Public Entry Point + +**Date:** 2026-05-23 +**Status:** Accepted + +## Context + +`packages/bot/src/services/musicRecommendation/index.ts` (214 LOC) exposes four public functions: + +- `generateRecommendations()` +- `generateUserPreferenceRecommendations()` +- `generateHistoryBasedRecommendations()` +- `getContextualRecommendations()` + +All four ultimately call the same internal `generateRecommendations()` scoring loop. The distinction between them is which input signals are populated — but callers cannot tell this from the function names alone. Additionally, internal helpers (`blendRecommendations`, `applySpanishLanguagePenalty`) are exported from `index.ts`, widening the public interface surface beyond its intended boundary. + +A second problem: `RecommendationEngine` (the class) accepts a `config: Partial` in its constructor and exposes `updateConfig()` for runtime mutation. Because one engine instance is shared across all Guilds, concurrent replenish invocations can race on `updateConfig()` — a latent multi-tenancy bug. + +These problems combine to make the Recommendation Engine's interface broader than its implementation. Callers face two questions that should not be their concern: (1) which function to call, and (2) whether config is stable. + +## Decision + +Collapse the public API to a single function: + +```typescript +function recommendTracks( + context: RecommendationContext, +): Promise + +interface RecommendationContext { + guildId: string + seedTracks: Track[] + trackHistory: TrackHistoryEntry[] + userPreferences: UserArtistPreference[] + strategy: 'history' | 'preference' | 'contextual' | 'auto' + limit: number +} +``` + +The `strategy` field carries what was previously encoded in the choice of function. `'auto'` (the default) lets the engine choose based on what signals are populated — the same fallback logic that currently lives in each of the four functions. + +Un-export `blendRecommendations` and `applySpanishLanguagePenalty` — these are implementation details. + +Make `RecommendationConfig` immutable at construction time: remove `updateConfig()`. Config is injected once at startup and never mutated. Per-Guild customisation (if needed) goes through `RecommendationContext`, not engine mutation. + +## Alternatives Considered + +**Keep four functions, just fix the export leak.** Rejected: the four-function API still forces callers to choose the right entry point. The caller (`replenisher.ts`) already constructs all the signals anyway — strategy selection inside the engine is strictly more informative. + +**Strategy pattern (four classes, one interface).** Rejected: over-engineered. The scoring logic is shared across strategies; separate classes would duplicate the shared path. `RecommendationContext.strategy` achieves the same routing without the class hierarchy. + +**Remove the Recommendation Engine and inline it in replenisher.** Rejected: the engine has a testable interface and value beyond the autoplay pipeline (e.g., the `/recommendations` dashboard endpoint calls it directly). Keeping it as an independent module is correct. + +## Consequences + +**Positive:** + +- Callers have one call site; the interface is one type to understand. +- `blendRecommendations` and `applySpanishLanguagePenalty` become private — no external code can depend on them. +- Config immutability eliminates the `updateConfig()` race condition. +- Adding a new strategy is a new `strategy` enum value + case in the dispatch; no new public function. + +**Negative:** + +- `RecommendationContext` becomes a new type to maintain. If future strategies need signals not present today, `RecommendationContext` grows. +- Callers that currently call `generateHistoryBasedRecommendations()` explicitly (expressing intent) lose that explicitness — they must now set `strategy: 'history'`. + +## Revisit When + +- A new strategy requires context fields incompatible with `RecommendationContext` (split context types, or introduce strategy-specific context extensions). +- The engine is moved to `packages/shared` for use by the backend dashboard — at that point, reconsider whether `RecommendationContext` needs a port/adapter split for DB access.