feat: per-guild configuration (multi-tenancy)#74
Conversation
Closes #71 - Migrate config table: guild_id column with composite PK (guild_id, key) - configCache is now Map<string, Object> keyed by guild_id - getConfig(guildId?) returns deep merge of global + guild overrides - setConfigValue/resetConfig accept optional guildId param - All 43+ call sites updated with guildId from interaction/message context - REST API uses actual guild ID for config endpoints - onConfigChange callbacks receive guildId as 4th argument - Full backward compat: getConfig() with no args returns global config - Comprehensive tests for guild isolation, deep merge, fallback behavior - All 1003 tests passing
|
Claude finished @BillChirico's task in 6m 16s —— View job Code Review: PR #74 — Per-Guild Configuration (Multi-Tenancy)Status: Review complete — changes requested Review ScopeReviewed all 38 changed files:
Issues Found
Both warnings are one-line fixes: |
📝 WalkthroughSummary by CodeRabbit
WalkthroughConfig system changed to support per-guild overrides merged onto global defaults; config APIs, caches, DB schema, and many modules/commands were updated to accept and propagate an optional guildId, with tests and docs adjusted accordingly. Changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
🔴 Critical
- Documentation not updated — AGENTS.md documents
getConfig()andsetConfigValue(key, value)but the signatures have changed togetConfig(guildId?)andsetConfigValue(path, value, guildId?). The "Config Hot-Reload Behavior" section and Config conventions also need updating. Per AGENTS.md's own rules: "Keep docs up to date — this is non-negotiable."
🟡 Warnings
-
guildIdnot threaded through ai.js call sites —getHistoryLength(guildId)andgetHistoryTTLDays(guildId)now acceptguildIdbut 4 call sites inai.jsstill call without it (hydrateHistory,addToHistory,initConversationHistory,runCleanup). Per-guild overrides of these values will be silently ignored. -
Unbounded
configCacheMap — No eviction or size cap on guild entries. EverygetConfig(guildId)with overrides performsstructuredClone+deepMergeon every call (hot path). Consider caching merged results or adding an LRU cap. -
DB migration assumes
config_pkeyconstraint name —ALTER TABLE config DROP CONSTRAINT config_pkeywill fail if the PK has a non-default name. UseDROP CONSTRAINT IF EXISTSfor robustness. -
Asymmetric return semantics —
getConfig()returns a live mutable reference;getConfig(guildId)returns a detached clone. This is intentional for backward compat but should be called out more clearly in comments.
🔵 Nitpicks
- Eager loading of all guild configs —
loadConfig()fetches all rows for all guilds at startup. Acceptable now but a scaling concern.
What looks correct
- SQL injection prevention: all queries use parameterized
$1/$2/$3placeholders ✓ - Prototype pollution protection via
validatePathSegments/DANGEROUS_KEYS✓ - Guild isolation via composite PK
(guild_id, key)✓ - Deep merge with
structuredCloneprevents shared references ✓ - All 13 command files correctly pass
interaction.guildId✓ - REST API validates guild exists via
validateGuildmiddleware before config operations ✓ - Test coverage for guild isolation, deep merge, fallback, reset, and multi-guild ✓
- Config change events correctly pass
guildIdas 4th argument ✓
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/modules/ai.js (2)
214-226:⚠️ Potential issue | 🟡 Minor
addToHistoryhasguildIdbut doesn't pass it togetHistoryLength().Line 221 calls
getHistoryLength()without arguments, even thoughguildIdis available as a parameter on Line 214. This means in-memory trimming always uses the global history length rather than the guild-specific override.Proposed fix
- const maxHistory = getHistoryLength(); + const maxHistory = getHistoryLength(guildId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/ai.js` around lines 214 - 226, addToHistory uses the guild-scoped parameter but calls getHistoryLength() without passing guildId, so trimming uses the global value; update the addToHistory function to call getHistoryLength(guildId) (and handle if guildId is undefined by allowing getHistoryLength to return the global default) so the in-memory history trimming for conversationHistory per channel respects guild-specific overrides; locate addToHistory and replace the getHistoryLength call accordingly, ensuring variable names (conversationHistory, history, getHistoryLength, guildId) are used consistently.
134-139: 🧹 Nitpick | 🔵 Trivial
hydrateHistoryalso callsgetHistoryLength()without a guild context.The function has no
guildIdparameter and no way to determine the guild from a channel ID alone, so this will always use global defaults. This is acceptable for now, but worth noting as a gap if guilds configure different history lengths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/ai.js` around lines 134 - 139, hydrateHistory calls getHistoryLength() with no guild context, so it always uses global defaults; update hydrateHistory to accept a guildId (or derive it from the channelId passed in) and call getHistoryLength(guildId) instead of the no-arg version. Concretely: modify the hydrateHistory signature to include guildId (or compute guildId inside using your channel->guild lookup), replace the getHistoryLength() call with getHistoryLength(guildId), and update all callers of hydrateHistory to supply the guildId (or the channelId used to derive it); keep the existing pool/getPool and historyRef usage intact.src/index.js (1)
186-195:⚠️ Potential issue | 🟠 MajorUse guild-specific config for permission checks to honor per-guild overrides.
The permission check on line 188 passes the module-level
config(initialized once at startup) tohasPermission, which ignores guild-specific overrides likeadminRoleIdor per-guildallowedCommands. Every other command handler insrc/commands/already usesgetConfig(interaction.guildId)to fetch merged guild configuration;src/index.jsshould follow the same pattern.Proposed fix
- if (!hasPermission(member, commandName, config)) { + if (!hasPermission(member, commandName, getConfig(interaction.guildId))) {Ensure
getConfigis imported fromsrc/modules/config.js.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.js` around lines 186 - 195, The permission check is using the module-level config instead of a guild-merged config; call getConfig(interaction.guildId) and pass that result to hasPermission so guild-specific overrides (like adminRoleId and allowedCommands) are honored, and ensure getConfig is imported from src/modules/config.js; update the block around hasPermission (and any related calls like getPermissionError if it accepts config) to use the guild config variable before calling safeReply/return.src/commands/config.js (1)
334-344:⚠️ Potential issue | 🟡 MinorReset message may mislead for guild-scoped resets.
Per
resetConfig(section, guildId)docs, a guild reset deletes guild overrides so global defaults take effect again. But the embed description says "reset to defaults from config.json" — which is only accurate for a global reset. For guild resets, something like "reset to global defaults" would be more precise.Proposed fix
const embed = new EmbedBuilder() .setColor(0xfee75c) .setTitle('🔄 Config Reset') .setDescription( section - ? `Section **${escapeInlineCode(section)}** has been reset to defaults from config.json.` - : 'All configuration has been reset to defaults from config.json.', + ? `Section **${escapeInlineCode(section)}** has been reset to global defaults.` + : 'All configuration has been reset to global defaults.', )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.js` around lines 334 - 344, The reset embed text is misleading for guild-scoped resets: when calling resetConfig(section, interaction.guildId) the operation removes guild overrides so the effective values come from global defaults, not necessarily from config.json; update the EmbedBuilder description logic (the block building the embed using section and escapeInlineCode(section)) to choose different wording for guild resets vs. global resets — e.g., if interaction.guildId is present and section is provided, set the description to "Section **X** has been reset to global defaults" (or for a full guild reset: "All guild overrides have been removed; global defaults will apply"), otherwise keep the existing "reset to defaults from config.json" message for non-guild/global resets.tests/api/routes/guilds.test.js (1)
140-144: 🧹 Nitpick | 🔵 TrivialTest doesn't verify guild-scoped config retrieval.
getConfigis asserted with.toHaveBeenCalled()but not.toHaveBeenCalledWith('guild1'). Since the route now callsgetConfig(req.params.id), the test should verify the guild ID is passed.Proposed fix
- expect(getConfig).toHaveBeenCalled(); + expect(getConfig).toHaveBeenCalledWith('guild1');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/routes/guilds.test.js` around lines 140 - 144, The test for guild config doesn't assert that the guild ID is passed to getConfig; update the assertion in tests/api/routes/guilds.test.js so that getConfig is verified to be called with the route param (e.g., expect(getConfig).toHaveBeenCalledWith('guild1')), ensuring the test checks the new behavior where the route calls getConfig(req.params.id); keep the rest of assertions intact.src/modules/memory.js (1)
164-168:⚠️ Potential issue | 🟠 Major
isMemoryAvailable()andcheckAndRecoverMemory()ignore guild-level config.Both functions call
getMemoryConfig()without passingguildId, so they always check only the globalmemory.enabledflag. Per-guild config disabling memory will be ignored. The root cause:addMemory,searchMemories,buildMemoryContext, and all dependent functions lackguildIdparameters entirely, making it impossible to thread guild context through the call chain. To respect per-guild memory toggles, addguildIdparameters to these public APIs and pass them through togetMemoryConfig(guildId).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/memory.js` around lines 164 - 168, isMemoryAvailable and checkAndRecoverMemory currently call getMemoryConfig() without guild context, so per-guild memory toggles are ignored; update these functions (isMemoryAvailable, checkAndRecoverMemory) to accept a guildId parameter and call getMemoryConfig(guildId) instead, and propagate guildId through the public APIs that touch memory (addMemory, searchMemories, buildMemoryContext and any callers) so the guildId flows down to getMemoryConfig(guildId) everywhere; ensure function signatures and call sites are updated consistently to thread guildId through the call chain.src/modules/threading.js (1)
364-365:⚠️ Potential issue | 🟡 Minor
sweepExpiredThreadsuses global config, but threads may belong to guilds with different reuse windows.
getThreadConfig()is called without a guild ID, so the sweep uses the globalreuseWindowMsfor all entries. If a guild overrides a longer reuse window, its threads will still be evicted based on the shorter global default.Consider storing the guild ID in the
activeThreadscache entries (set duringcreateThread/findExistingThread) and passing it togetThreadConfigduring the sweep so each entry is evaluated against its guild's config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/threading.js` around lines 364 - 365, sweepExpiredThreads currently calls getThreadConfig() without a guild context causing all threads to be evaluated against the global reuseWindowMs; update the activeThreads cache entries to include the guild ID when threads are created/found (in createThread and findExistingThread), and then change sweepExpiredThreads to read the guild ID from each activeThreads entry and call getThreadConfig(guildId) so the reuseWindowMs used for eviction is the per-guild value; ensure existing activeThreads shape changes are handled across any consumers.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (28)
src/api/routes/guilds.jssrc/commands/ban.jssrc/commands/case.jssrc/commands/config.jssrc/commands/kick.jssrc/commands/lock.jssrc/commands/modlog.jssrc/commands/purge.jssrc/commands/slowmode.jssrc/commands/softban.jssrc/commands/tempban.jssrc/commands/timeout.jssrc/commands/unban.jssrc/commands/unlock.jssrc/commands/untimeout.jssrc/commands/warn.jssrc/db.jssrc/index.jssrc/modules/ai.jssrc/modules/config.jssrc/modules/memory.jssrc/modules/moderation.jssrc/modules/threading.jstests/api/routes/guilds.test.jstests/commands/config.test.jstests/commands/modlog.test.jstests/modules/config-events.test.jstests/modules/config-guild.test.js
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
src/index.jstests/api/routes/guilds.test.jssrc/commands/untimeout.jssrc/commands/lock.jssrc/commands/ban.jssrc/db.jssrc/modules/threading.jssrc/commands/purge.jssrc/commands/timeout.jstests/modules/config-guild.test.jstests/modules/config-events.test.jssrc/modules/ai.jssrc/commands/kick.jssrc/commands/softban.jssrc/modules/memory.jssrc/commands/config.jstests/commands/modlog.test.jssrc/api/routes/guilds.jssrc/modules/moderation.jssrc/commands/unban.jssrc/commands/unlock.jssrc/commands/modlog.jssrc/commands/tempban.jssrc/modules/config.jssrc/commands/warn.jssrc/commands/slowmode.jssrc/commands/case.jstests/commands/config.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/index.jssrc/commands/untimeout.jssrc/commands/lock.jssrc/commands/ban.jssrc/db.jssrc/modules/threading.jssrc/commands/purge.jssrc/commands/timeout.jssrc/modules/ai.jssrc/commands/kick.jssrc/commands/softban.jssrc/modules/memory.jssrc/commands/config.jssrc/api/routes/guilds.jssrc/modules/moderation.jssrc/commands/unban.jssrc/commands/unlock.jssrc/commands/modlog.jssrc/commands/tempban.jssrc/modules/config.jssrc/commands/warn.jssrc/commands/slowmode.jssrc/commands/case.js
src/index.js
📄 CodeRabbit inference engine (AGENTS.md)
src/index.js: The Database.js pool must be initialized before command loading; the startup sequence insrc/index.jsis: initialize pool → load config → register clients → load commands → set up heartbeat
Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Files:
src/index.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export adataobject usingSlashCommandBuilderand anexecute(interaction)async function
ExportadminOnly = truefrom command files to restrict execution to moderators
Files:
src/commands/untimeout.jssrc/commands/lock.jssrc/commands/ban.jssrc/commands/purge.jssrc/commands/timeout.jssrc/commands/kick.jssrc/commands/softban.jssrc/commands/config.jssrc/commands/unban.jssrc/commands/unlock.jssrc/commands/modlog.jssrc/commands/tempban.jssrc/commands/warn.jssrc/commands/slowmode.jssrc/commands/case.js
src/commands/*{ban,kick,warn,timeout,mute}*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*{ban,kick,warn,timeout,mute}*.js: Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Always callcheckHierarchy(moderator, target)fromsrc/utils/permissions.jsbefore executing moderation actions to prevent role hierarchy violations
Files:
src/commands/untimeout.jssrc/commands/ban.jssrc/commands/timeout.jssrc/commands/kick.jssrc/commands/softban.jssrc/commands/unban.jssrc/commands/tempban.jssrc/commands/warn.js
src/commands/*{timeout,tempban,slowmode}*.js
📄 CodeRabbit inference engine (AGENTS.md)
Use
parseDuration()fromsrc/utils/duration.jsto parse duration strings like '1h', '7d' for timeout, tempban, and slowmode commands
Files:
src/commands/untimeout.jssrc/commands/timeout.jssrc/commands/tempban.jssrc/commands/slowmode.js
src/commands/*timeout*.js
📄 CodeRabbit inference engine (AGENTS.md)
Discord timeouts have a maximum duration of 28 days; enforce this cap in timeout commands
Files:
src/commands/untimeout.jssrc/commands/timeout.js
src/commands/*{ban,kick}*.js
📄 CodeRabbit inference engine (AGENTS.md)
DM the target user before executing kicks/bans in moderation commands; once a user is kicked/banned they cannot receive DMs
Files:
src/commands/ban.jssrc/commands/kick.jssrc/commands/softban.jssrc/commands/unban.jssrc/commands/tempban.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Check
config.yourModule.enabledbefore processing any module logic to allow runtime enable/disable
Files:
src/modules/threading.jssrc/modules/ai.jssrc/modules/memory.jssrc/modules/moderation.jssrc/modules/config.js
src/modules/{welcome,ai,chimeIn}.js
📄 CodeRabbit inference engine (AGENTS.md)
Use
sanitizeMentions()fromsrc/utils/sanitizeMentions.jsto strip@everyone/@here from outgoing text via zero-width space insertion
Files:
src/modules/ai.js
src/modules/moderation.js
📄 CodeRabbit inference engine (AGENTS.md)
Case numbering must be per-guild sequential and assigned atomically using
COALESCE(MAX(case_number), 0) + 1in a single INSERT statement withincreateCase()
Files:
src/modules/moderation.js
src/commands/*slowmode*.js
📄 CodeRabbit inference engine (AGENTS.md)
Discord slowmode has a maximum duration of 6 hours (21600 seconds); enforce this cap in slowmode commands
Files:
src/commands/slowmode.js
🧠 Learnings (14)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Prefer the per-request `getConfig()` pattern for modules; only add reactive `onConfigChange` listeners for stateful resources (e.g., Winston transports) that cannot re-read config on each invocation
Applied to files:
src/index.jstests/modules/config-events.test.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `getConfig()` from `src/modules/config.js` to read runtime configuration
Applied to files:
src/index.jssrc/commands/kick.jssrc/commands/config.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/api/routes/guilds.test.jssrc/commands/untimeout.jssrc/commands/timeout.jstests/modules/config-guild.test.jssrc/commands/softban.jssrc/commands/config.jstests/commands/modlog.test.jssrc/modules/config.jstests/commands/config.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{timeout,tempban,slowmode}*.js : Use `parseDuration()` from `src/utils/duration.js` to parse duration strings like '1h', '7d' for timeout, tempban, and slowmode commands
Applied to files:
src/commands/untimeout.jssrc/commands/timeout.jssrc/commands/softban.jssrc/commands/tempban.jssrc/commands/slowmode.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*timeout*.js : Discord timeouts have a maximum duration of 28 days; enforce this cap in timeout commands
Applied to files:
src/commands/untimeout.jssrc/commands/timeout.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick}*.js : DM the target user *before* executing kicks/bans in moderation commands; once a user is kicked/banned they cannot receive DMs
Applied to files:
src/commands/ban.jssrc/commands/timeout.jssrc/commands/softban.jssrc/commands/unban.jssrc/commands/tempban.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
src/commands/ban.jssrc/commands/timeout.jssrc/commands/softban.jssrc/modules/moderation.jssrc/commands/unban.jssrc/commands/modlog.jssrc/commands/tempban.jssrc/commands/warn.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Always call `checkHierarchy(moderator, target)` from `src/utils/permissions.js` before executing moderation actions to prevent role hierarchy violations
Applied to files:
src/commands/ban.jssrc/commands/softban.jssrc/commands/unban.jssrc/commands/tempban.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Use Vitest for unit and integration tests; run `pnpm test` and `pnpm test:coverage` before every commit
Applied to files:
tests/modules/config-guild.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `setConfigValue(key, value)` from `src/modules/config.js` to update configuration at runtime
Applied to files:
tests/modules/config-events.test.jssrc/commands/config.jssrc/api/routes/guilds.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/moderation.js : Case numbering must be per-guild sequential and assigned atomically using `COALESCE(MAX(case_number), 0) + 1` in a single INSERT statement within `createCase()`
Applied to files:
src/modules/moderation.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Slash commands must export a `data` object using `SlashCommandBuilder` and an `execute(interaction)` async function
Applied to files:
src/commands/unban.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : The Database.js pool must be initialized before command loading; the startup sequence in `src/index.js` is: initialize pool → load config → register clients → load commands → set up heartbeat
Applied to files:
src/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*slowmode*.js : Discord slowmode has a maximum duration of 6 hours (21600 seconds); enforce this cap in slowmode commands
Applied to files:
src/commands/slowmode.js
🧬 Code graph analysis (22)
tests/api/routes/guilds.test.js (1)
src/modules/config.js (1)
setConfigValue(282-381)
src/commands/untimeout.js (16)
src/commands/ban.js (1)
config(44-44)src/commands/case.js (1)
config(222-222)src/commands/config.js (1)
config(124-124)src/commands/kick.js (1)
config(36-36)src/commands/lock.js (1)
config(55-55)src/commands/modlog.js (1)
config(177-177)src/commands/purge.js (1)
config(155-155)src/commands/slowmode.js (1)
config(67-67)src/commands/softban.js (1)
config(44-44)src/commands/tempban.js (1)
config(49-49)src/commands/timeout.js (1)
config(40-40)src/commands/unban.js (1)
config(31-31)src/commands/unlock.js (1)
config(55-55)src/commands/warn.js (1)
config(37-37)src/index.js (2)
config(57-57)interaction(182-182)src/modules/config.js (1)
getConfig(180-193)
src/commands/lock.js (3)
src/commands/ban.js (1)
config(44-44)src/commands/kick.js (1)
config(36-36)src/modules/config.js (1)
getConfig(180-193)
src/commands/ban.js (2)
src/commands/case.js (1)
config(222-222)src/modules/config.js (1)
getConfig(180-193)
src/db.js (1)
src/modules/config.js (5)
pool(81-81)pool(98-98)pool(310-310)pool(394-394)pool(444-444)
src/modules/threading.js (1)
src/modules/config.js (1)
getConfig(180-193)
src/commands/purge.js (2)
src/commands/ban.js (1)
config(44-44)src/modules/config.js (1)
getConfig(180-193)
src/commands/timeout.js (6)
src/commands/ban.js (1)
config(44-44)src/commands/case.js (1)
config(222-222)src/commands/config.js (1)
config(124-124)src/commands/kick.js (1)
config(36-36)src/index.js (2)
config(57-57)interaction(182-182)src/modules/config.js (1)
getConfig(180-193)
src/modules/ai.js (2)
src/api/routes/guilds.js (1)
config(99-99)src/modules/config.js (1)
getConfig(180-193)
src/commands/softban.js (1)
src/modules/config.js (1)
getConfig(180-193)
src/modules/memory.js (6)
src/index.js (2)
guildId(157-157)config(57-57)src/api/routes/guilds.js (1)
config(99-99)src/commands/ban.js (1)
config(44-44)src/commands/config.js (2)
config(124-124)config(189-189)src/modules/ai.js (2)
config(36-36)config(52-52)src/modules/config.js (1)
getConfig(180-193)
src/commands/config.js (1)
src/modules/config.js (4)
getConfig(180-193)setConfigValue(282-381)resetConfig(391-536)section(291-291)
tests/commands/modlog.test.js (1)
src/modules/config.js (1)
setConfigValue(282-381)
src/api/routes/guilds.js (1)
src/modules/config.js (2)
getConfig(180-193)setConfigValue(282-381)
src/modules/moderation.js (17)
src/commands/ban.js (1)
config(44-44)src/commands/case.js (1)
config(222-222)src/commands/config.js (1)
config(124-124)src/commands/kick.js (1)
config(36-36)src/commands/lock.js (1)
config(55-55)src/commands/modlog.js (1)
config(177-177)src/commands/purge.js (1)
config(155-155)src/commands/slowmode.js (1)
config(67-67)src/commands/softban.js (1)
config(44-44)src/commands/tempban.js (1)
config(49-49)src/commands/timeout.js (1)
config(40-40)src/commands/unban.js (1)
config(31-31)src/commands/unlock.js (1)
config(55-55)src/commands/untimeout.js (1)
config(30-30)src/commands/warn.js (1)
config(37-37)src/index.js (1)
config(57-57)src/modules/config.js (1)
getConfig(180-193)
src/commands/unlock.js (16)
src/commands/ban.js (1)
config(44-44)src/commands/case.js (1)
config(222-222)src/commands/config.js (1)
config(124-124)src/commands/kick.js (1)
config(36-36)src/commands/lock.js (1)
config(55-55)src/commands/modlog.js (1)
config(177-177)src/commands/purge.js (1)
config(155-155)src/commands/slowmode.js (1)
config(67-67)src/commands/softban.js (1)
config(44-44)src/commands/tempban.js (1)
config(49-49)src/commands/timeout.js (1)
config(40-40)src/commands/unban.js (1)
config(31-31)src/commands/untimeout.js (1)
config(30-30)src/commands/warn.js (1)
config(37-37)src/index.js (2)
config(57-57)interaction(182-182)src/modules/config.js (1)
getConfig(180-193)
src/commands/modlog.js (1)
src/commands/config.js (1)
config(124-124)
src/commands/tempban.js (2)
src/commands/ban.js (1)
config(44-44)src/modules/config.js (1)
getConfig(180-193)
src/modules/config.js (4)
src/db.js (3)
pool(13-13)client(83-83)getPool(240-245)src/index.js (2)
client(79-88)guildId(157-157)src/logger.js (1)
info(217-219)src/deploy-commands.js (1)
guildId(26-26)
src/commands/warn.js (1)
src/modules/config.js (1)
getConfig(180-193)
src/commands/slowmode.js (16)
src/commands/ban.js (1)
config(44-44)src/commands/case.js (1)
config(222-222)src/commands/config.js (1)
config(124-124)src/commands/kick.js (1)
config(36-36)src/commands/lock.js (1)
config(55-55)src/commands/modlog.js (1)
config(177-177)src/commands/purge.js (1)
config(155-155)src/commands/softban.js (1)
config(44-44)src/commands/tempban.js (1)
config(49-49)src/commands/timeout.js (1)
config(40-40)src/commands/unban.js (1)
config(31-31)src/commands/unlock.js (1)
config(55-55)src/commands/untimeout.js (1)
config(30-30)src/commands/warn.js (1)
config(37-37)src/index.js (2)
config(57-57)interaction(182-182)src/modules/config.js (1)
getConfig(180-193)
src/commands/case.js (16)
src/commands/ban.js (1)
config(44-44)src/commands/config.js (1)
config(124-124)src/commands/kick.js (1)
config(36-36)src/commands/lock.js (1)
config(55-55)src/commands/modlog.js (1)
config(177-177)src/commands/purge.js (1)
config(155-155)src/commands/slowmode.js (1)
config(67-67)src/commands/softban.js (1)
config(44-44)src/commands/tempban.js (1)
config(49-49)src/commands/timeout.js (1)
config(40-40)src/commands/unban.js (1)
config(31-31)src/commands/unlock.js (1)
config(55-55)src/commands/untimeout.js (1)
config(30-30)src/commands/warn.js (1)
config(37-37)src/index.js (2)
config(57-57)interaction(182-182)src/modules/config.js (1)
getConfig(180-193)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (37)
src/index.js (1)
375-385: LGTM — comment accurately reflects the per-request config pattern.src/commands/kick.js (1)
36-36: LGTM — guild-scoped config retrieval is correct.src/commands/ban.js (1)
44-44: LGTM — guild-scoped config retrieval follows the established pattern.src/commands/unban.js (1)
31-31: LGTM.src/commands/untimeout.js (1)
30-30: LGTM.src/commands/tempban.js (1)
49-49: LGTM — guild-scoped config retrieval is correct.src/commands/slowmode.js (1)
67-67: LGTM.src/commands/case.js (1)
222-222: LGTM!Correctly passes
interaction.guildIdtogetConfig()for guild-scoped config retrieval, consistent with all other command files.src/commands/unlock.js (1)
55-55: LGTM!Guild-scoped config retrieval is consistent with the lock command and other command files.
src/commands/timeout.js (1)
40-40: LGTM!Guild-scoped config retrieval is correctly placed before DM notification and mod log operations. The timeout command properly enforces the 28-day cap and follows the expected moderation flow pattern.
src/modules/moderation.js (1)
377-377: LGTM!Using
row.guild_idfrom the scheduled action ensures the correct guild-specific mod log channels are used when processing expired tempbans.src/commands/lock.js (1)
55-55: LGTM!Guild-scoped config retrieval is consistent with the unlock command counterpart and other command files.
src/db.js (1)
102-115: Migration relies on the default PostgreSQL constraint nameconfig_pkey.The
DROP CONSTRAINT config_pkeyat line 111 assumes the primary key constraint follows PostgreSQL's default{table}_pkeynaming convention. This is correct for standardCREATE TABLEDDL, but could fail if the constraint was created with a custom name or if the table was renamed. Since this is an internal bot database unlikely to have custom DDL, the risk is low—just noting for awareness.tests/modules/config-events.test.js (1)
73-73: LGTM!All callback assertions correctly include
'global'as the 4th argument, matching the defaultguildIdwhensetConfigValueis called without an explicit guild parameter.Also applies to: 83-83, 167-167, 176-176, 212-212
tests/commands/modlog.test.js (1)
39-39: LGTM!The test interaction correctly includes
guildId: 'test-guild', and allsetConfigValueassertions properly verify the guild ID is propagated through the disable and setup flows.Also applies to: 113-147, 224-228
src/commands/warn.js (1)
37-37: LGTM! Correct per-guild config retrieval. The moderation flow pattern (deferReply → validate → DM → createCase → modLog → escalation) is properly followed.src/commands/purge.js (1)
155-155: LGTM! Per-guild config correctly passed for mod log embedding.tests/api/routes/guilds.test.js (1)
165-165: LGTM! Correctly asserts the new 3-argument signature forsetConfigValue.src/commands/softban.js (1)
44-44: LGTM! Per-guild config correctly retrieved. DM is properly sent before the ban action.src/commands/config.js (1)
124-124: LGTM! All config read/write operations correctly passinteraction.guildIdfor per-guild scoping.Also applies to: 189-189, 278-278, 290-290
src/modules/memory.js (1)
140-155: LGTM on the signature change.getMemoryConfignow correctly accepts and forwardsguildIdtogetConfig.src/api/routes/guilds.js (2)
96-110: LGTM! GET config correctly returns per-guild merged config withguildIdin the response payload. Readable keys are properly filtered.
148-156: LGTM! PATCH correctly writes guild-specific overrides viasetConfigValue(path, value, req.params.id).src/commands/modlog.js (2)
128-137: LGTM!setConfigValuecorrectly passesinteraction.guildIdfor per-guild modlog channel routing, and the log includes structuredguildIdmetadata.
177-177: LGTM! View correctly reads per-guild config.src/modules/threading.js (2)
62-67: LGTM! Clean addition of the optionalguildIdparameter, correctly threaded through togetConfig(guildId).
100-101: LGTM! Correct use of optional chaining (message.guild?.id) to gracefully handle DMs (whereguildisnull), which then falls back to global config.tests/commands/config.test.js (3)
35-35: LGTM! All interaction mocks consistently includeguildId: 'test-guild', and assertions are correctly updated to verify the guild ID is propagated tosetConfigValueandresetConfig.Also applies to: 52-52, 71-71, 88-88, 105-105, 138-138, 163-163, 190-190, 216-216, 237-237, 262-262, 290-290, 319-319, 336-336, 353-353, 373-373, 397-397
229-231: LGTM! Correctly verifies the new 3-arg signaturesetConfigValue('ai.model', 'new-model', 'test-guild').
329-329: LGTM! Both reset paths verified — section reset (resetConfig('ai', 'test-guild')) and full reset (resetConfig(undefined, 'test-guild')) match the updated signatures.Also applies to: 346-346
tests/modules/config-guild.test.js (1)
1-271: Comprehensive and well-structured test suite. Covers guild isolation, deep merge, fallback, set/reset semantics, event emission with guildId, and multi-guild concurrency. Good use ofvi.resetModules()with dynamic imports for module isolation.src/modules/config.js (6)
33-42: LGTM — cleandeepMergeimplementation. UsesstructuredCloneon individual leaf values to prevent shared references, and theisPlainObjectguard avoids merging arrays or other non-plain types.
92-94: LGTM! Cache initialization correctly usesnew Map()withstructuredClone(fileConfig)for the global entry across all fallback paths, ensuring no accidental sharing withfileConfigCache.Also applies to: 124-125, 166-167
282-282: LGTM —setConfigValueextended with guild-aware persistence.The function correctly:
- Defaults
guildIdto'global'- Locks the DB row for the specific
(guild_id, key)pair before writing- Updates only the guild-specific cache entry
- Emits change events with the guild context
391-431: LGTM — guild reset logic is clean and correct.The guild reset path properly handles both section-level and full resets, with appropriate DB cleanup and cache eviction. The
DELETE FROM config WHERE guild_id = $1correctly scopes to the specific guild since the guard at line 393 ensuresguildId !== 'global'.
246-246: LGTM!emitConfigChangeEventscorrectly propagatesguildIdto listeners as the 4th callback argument.
97-158: Thevaluecolumn is correctly defined as JSONB, sorow.valuewill be auto-parsed by thepgdriver.The database schema in
src/db.js(lines 93–100) defines theconfigtable withvalue JSONB NOT NULL, which ensures that thepgdriver automatically parses the JSON on retrieval. The round-trip in lines 119 and 143 (JSON.stringify()on write, direct assignment on read) is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/modlog.js`:
- Around line 210-213: The disable path logs mod logging being disabled via the
info(...) call but omits the guild identifier; update the info call in the
disable branch (the call after
setConfigValue(`moderation.logging.channels.${key}`, null, interaction.guildId))
to include the guildId in the structured metadata alongside moderator (use
interaction.guildId and interaction.user.tag) so it matches the setup log's
format and provides consistent multi-guild context.
In `@src/db.js`:
- Around line 117-120: Remove the redundant standalone index creation for
idx_config_guild_id: the composite primary key on config (guild_id, key) already
provides a b-tree index with guild_id as the leading column, so delete the SQL
block that calls pool.query(...) to CREATE INDEX IF NOT EXISTS
idx_config_guild_id (the snippet that runs CREATE INDEX IF NOT EXISTS
idx_config_guild_id ON config (guild_id)); keep the rest of the
migration/initialization logic intact.
In `@src/modules/config.js`:
- Around line 495-501: The cleanup now deletes only global rows whose keys are
not in fileConfig (using fileKeys and client.query) but leaves per-guild
override rows orphaned, which breaks getConfig(guildId) merging; update the
logic to either (a) also delete config rows for all guilds that reference any
key removed from fileKeys, or (b) detect those orphaned keys and write a warning
log listing affected guild_ids and keys so they can be reviewed; implement the
chosen fix near the existing deletion block (referencing fileKeys, client.query,
and getConfig) so the DB state and merged results remain consistent.
- Around line 180-193: getConfig currently returns the raw global cache object
but returns clones for guilds, which creates a mutation asymmetry; change
getConfig to return a defensive copy for the global path too (e.g., use
structuredClone on configCache.get('global') || {} or a frozen shallow copy) so
callers can't accidentally mutate the in-memory cache. Update the global branch
in getConfig (the code paths using configCache.get('global')) to return a
cloned/frozen object and verify setConfigValue and resetConfig (which
intentionally mutate the cache) still operate on the actual cache entries rather
than on the returned object.
In `@tests/modules/config-guild.test.js`:
- Around line 72-89: Add a mutation-safety test for the global path: call
configModule.getConfig() (no args), mutate a top-level property (e.g., set
returned.ai.model = 'changed-model'), then call configModule.getConfig() again
(or configModule.getConfig('global')) and assert that the mutation is reflected
(since getConfig() returns the raw cache reference); this documents the
asymmetry with getConfig(guildId) which clones and guards against regressions.
---
Outside diff comments:
In `@src/commands/config.js`:
- Around line 334-344: The reset embed text is misleading for guild-scoped
resets: when calling resetConfig(section, interaction.guildId) the operation
removes guild overrides so the effective values come from global defaults, not
necessarily from config.json; update the EmbedBuilder description logic (the
block building the embed using section and escapeInlineCode(section)) to choose
different wording for guild resets vs. global resets — e.g., if
interaction.guildId is present and section is provided, set the description to
"Section **X** has been reset to global defaults" (or for a full guild reset:
"All guild overrides have been removed; global defaults will apply"), otherwise
keep the existing "reset to defaults from config.json" message for
non-guild/global resets.
In `@src/index.js`:
- Around line 186-195: The permission check is using the module-level config
instead of a guild-merged config; call getConfig(interaction.guildId) and pass
that result to hasPermission so guild-specific overrides (like adminRoleId and
allowedCommands) are honored, and ensure getConfig is imported from
src/modules/config.js; update the block around hasPermission (and any related
calls like getPermissionError if it accepts config) to use the guild config
variable before calling safeReply/return.
In `@src/modules/ai.js`:
- Around line 214-226: addToHistory uses the guild-scoped parameter but calls
getHistoryLength() without passing guildId, so trimming uses the global value;
update the addToHistory function to call getHistoryLength(guildId) (and handle
if guildId is undefined by allowing getHistoryLength to return the global
default) so the in-memory history trimming for conversationHistory per channel
respects guild-specific overrides; locate addToHistory and replace the
getHistoryLength call accordingly, ensuring variable names (conversationHistory,
history, getHistoryLength, guildId) are used consistently.
- Around line 134-139: hydrateHistory calls getHistoryLength() with no guild
context, so it always uses global defaults; update hydrateHistory to accept a
guildId (or derive it from the channelId passed in) and call
getHistoryLength(guildId) instead of the no-arg version. Concretely: modify the
hydrateHistory signature to include guildId (or compute guildId inside using
your channel->guild lookup), replace the getHistoryLength() call with
getHistoryLength(guildId), and update all callers of hydrateHistory to supply
the guildId (or the channelId used to derive it); keep the existing pool/getPool
and historyRef usage intact.
In `@src/modules/memory.js`:
- Around line 164-168: isMemoryAvailable and checkAndRecoverMemory currently
call getMemoryConfig() without guild context, so per-guild memory toggles are
ignored; update these functions (isMemoryAvailable, checkAndRecoverMemory) to
accept a guildId parameter and call getMemoryConfig(guildId) instead, and
propagate guildId through the public APIs that touch memory (addMemory,
searchMemories, buildMemoryContext and any callers) so the guildId flows down to
getMemoryConfig(guildId) everywhere; ensure function signatures and call sites
are updated consistently to thread guildId through the call chain.
In `@src/modules/threading.js`:
- Around line 364-365: sweepExpiredThreads currently calls getThreadConfig()
without a guild context causing all threads to be evaluated against the global
reuseWindowMs; update the activeThreads cache entries to include the guild ID
when threads are created/found (in createThread and findExistingThread), and
then change sweepExpiredThreads to read the guild ID from each activeThreads
entry and call getThreadConfig(guildId) so the reuseWindowMs used for eviction
is the per-guild value; ensure existing activeThreads shape changes are handled
across any consumers.
In `@tests/api/routes/guilds.test.js`:
- Around line 140-144: The test for guild config doesn't assert that the guild
ID is passed to getConfig; update the assertion in
tests/api/routes/guilds.test.js so that getConfig is verified to be called with
the route param (e.g., expect(getConfig).toHaveBeenCalledWith('guild1')),
ensuring the test checks the new behavior where the route calls
getConfig(req.params.id); keep the rest of assertions intact.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
The configCache Map grew unbounded — every guild that called getConfig() was cached forever. Added MAX_GUILD_CACHE_SIZE (500) cap with LRU eviction for guild entries (never evicts 'global'). Also cache the merged result per guild so repeated getConfig(guildId) doesn't re-clone+re-merge every time. Merged cache is invalidated on setConfigValue/resetConfig for the affected guild. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… functions hydrateHistory(), initConversationHistory(), and runCleanup() call getHistoryLength()/getHistoryTTLDays() without guildId. These functions operate at the channel level across all guilds — guildId is intentionally not available. Added JSDoc comments explaining the design decision and noting that the guild-aware path is through generateResponse(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
getConfig() with no guildId returns a live mutable cache reference (intentional for backward compatibility). getConfig(guildId) returns a deep-cloned merged copy to prevent cross-guild contamination. Added prominent JSDoc and inline comments explaining this contract. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The migration assumed the constraint was named 'config_pkey', which fails if the constraint has a different name. Now queries pg_constraint to find the actual primary key constraint name before dropping it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
loadConfig() eagerly fetches all config rows for all guilds into memory. Added a comment noting this is acceptable for <1000 guilds and suggesting lazy-loading or pagination for larger deployments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The setup path logged guildId but handleDisable() did not, creating an asymmetry. Added guildId to the info() call for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The composite PK (guild_id, key) already covers guild_id-only queries via leftmost prefix, making the standalone guild_id index redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When capturing oldValue for a guild, it reads from the guild override cache, not the effective (merged) value. This is correct behavior but was undocumented. Added a comment explaining that listeners see the previous override value, not the previous merged value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When resetting all global config, stale global keys are deleted but per-guild rows referencing those keys were silently left behind. Now logs a warning listing affected guild_id:key pairs so admins are aware of orphaned data without silently deleting guild overrides. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verifies the documented contract that getConfig() (global path) returns a live mutable cache reference where mutations are visible to subsequent callers. This is intentional behavior for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The merged config cache must still return clones to maintain the contract that each getConfig(guildId) call returns a distinct object. The cache avoids the expensive deep-merge step while structuredClone ensures no shared references. Also guard orphanResult.rows with optional chaining for robustness. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/modules/events.js (1)
135-142: 🧹 Nitpick | 🔵 TrivialUnnecessary optional chaining on
message.guild?.id.Line 73 already returns early when
message.guildis falsy, somessage.guild.idon line 141 is always safe. The?.is harmless but misleadingly suggests the guild could benullat this point.♻️ Suggested fix
const response = await generateResponse( historyId, cleanContent, message.author.username, healthMonitor, message.author.id, - message.guild?.id, + message.guild.id, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/events.js` around lines 135 - 142, The optional chaining on message.guild?.id in the call to generateResponse is unnecessary because earlier logic already returns when message.guild is falsy; update the invocation to use message.guild.id instead. Locate the generateResponse call that passes historyId, cleanContent, message.author.username, healthMonitor, message.author.id, and message.guild?.id and replace the last argument with message.guild.id to avoid the misleading ?. while preserving the same argument order and behavior.src/modules/config.js (2)
446-481:⚠️ Potential issue | 🟡 MinorLog the transaction error before re-throwing — coding guideline violation.
Line 480 re-throws
txErrwithout a priorlogError(...)call, violating the project rule "Always log errors with context before re-throwing". Thepool.connect()failure at line 447 has the same issue: it is outside thetryblock, so a failed connection propagates silently to the caller.🛡️ Proposed fix
+ } catch (connectErr) { + logError('Failed to acquire DB connection for config write', { path, guildId, error: connectErr.message }); + throw connectErr; + } + + // existing `if (pool)` block below...} catch (txErr) { try { await client.query('ROLLBACK'); } catch { /* ignore rollback failure */ } + logError('Transaction error in setConfigValue — config not persisted', { + path, + guildId, + error: txErr.message, + }); throw txErr; } finally {As per coding guidelines: "Always log errors with context before re-throwing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/config.js` around lines 446 - 481, The transaction error (txErr) is re-thrown without logging and pool.connect() is outside the transaction try/catch so its failure isn't logged; wrap the connection and transaction in an error-handling block or add a dedicated try/catch around pool.connect() to call logError(...) with contextual info (include guildId and section) before re-throwing, and in the existing catch(txErr) call logError(txErr, { guildId, section, action: 'upsertConfig' }) (or similar context) before attempting ROLLBACK and re-throwing txErr so all errors are logged with context.
530-694:⚠️ Potential issue | 🟠 Major
resetConfignever firesemitConfigChangeEvents— stateful listeners are left with stale config.
setConfigValuecorrectly callsawait emitConfigChangeEvents(...)after every write, butresetConfig(guild path line 568, global section path line 623, global full reset line 686) invalidates the merged cache and bumps the generation counter without notifying any registered listeners.Any stateful resource that relies on
onConfigChange(e.g., a Winston transport that reacts tologger.*changes) will continue running with the pre-reset settings until the process restarts. This breaks the event contract thatsetConfigValueestablishes.Suggested fix: emit change events for every affected path after the in-memory state is updated. For guild resets the paths come from the deleted overrides; for global resets iterate
fileConfigentries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/config.js` around lines 530 - 694, resetConfig currently mutates configCache/mergedConfigCache and increments globalConfigGeneration but never calls emitConfigChangeEvents, leaving onConfigChange listeners stale; update resetConfig (both guild branch and global section/full reset paths) to call await emitConfigChangeEvents for each affected config path after the in-memory state changes (use the keys removed/updated from configCache for guild resets and iterate fileConfig keys for global section/full reset) so listeners see the exact changed paths, keeping the behavior consistent with setConfigValue which calls await emitConfigChangeEvents; ensure calls happen after mutating caches and before returning and keep mergedConfigCache.clear() and globalConfigGeneration++ where present.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/modules/config.jssrc/modules/events.jstests/modules/ai.test.jstests/modules/events.test.js
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
tests/modules/ai.test.jssrc/modules/events.jstests/modules/events.test.jssrc/modules/config.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/modules/events.jssrc/modules/config.js
src/modules/events.js
📄 CodeRabbit inference engine (AGENTS.md)
Register module event handlers in
src/modules/events.jsviaclient.on()and passconfigto handler functions
Files:
src/modules/events.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Check
config.yourModule.enabledbefore processing any module logic to allow runtime enable/disable
Files:
src/modules/events.jssrc/modules/config.js
🧠 Learnings (8)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/modules/ai.test.jssrc/modules/events.jstests/modules/events.test.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/events.js : Register module event handlers in `src/modules/events.js` via `client.on()` and pass `config` to handler functions
Applied to files:
src/modules/events.jstests/modules/events.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `splitMessage()` utility from `src/utils/splitMessage.js` to split messages exceeding Discord's 2000-character limit
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `getConfig()` from `src/modules/config.js` to read runtime configuration
Applied to files:
src/modules/events.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/{welcome,ai,chimeIn}.js : Use `sanitizeMentions()` from `src/utils/sanitizeMentions.js` to strip everyone/here from outgoing text via zero-width space insertion
Applied to files:
src/modules/events.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Prefer the per-request `getConfig()` pattern for modules; only add reactive `onConfigChange` listeners for stateful resources (e.g., Winston transports) that cannot re-read config on each invocation
Applied to files:
src/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : The Database.js pool must be initialized before command loading; the startup sequence in `src/index.js` is: initialize pool → load config → register clients → load commands → set up heartbeat
Applied to files:
src/modules/config.js
🧬 Code graph analysis (4)
tests/modules/ai.test.js (3)
src/modules/config.js (2)
getConfig(274-305)i(724-724)src/modules/ai.js (6)
addToHistory(217-249)history(221-221)history(413-413)getHistoryAsync(196-206)reply(472-472)generateResponse(402-502)src/modules/memory.js (2)
buildMemoryContext(493-519)extractAndStoreMemories(532-574)
src/modules/events.js (2)
src/modules/config.js (4)
guildConfig(424-424)guildConfig(559-559)getConfig(274-305)err(86-86)src/modules/ai.js (1)
guildConfig(412-412)
tests/modules/events.test.js (2)
src/modules/config.js (7)
client(153-153)client(447-447)client(451-454)client(627-627)guildConfig(424-424)guildConfig(559-559)getConfig(274-305)src/modules/events.js (1)
registerGuildMemberAddHandler(56-61)
src/modules/config.js (2)
src/db.js (3)
pool(13-13)client(83-83)getPool(246-251)src/index.js (2)
client(79-88)guildId(157-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (10)
src/modules/events.js (3)
16-16: LGTM —getConfigimport for per-guild resolution.Clean addition, consistent with the ESM coding guidelines and the config module API.
54-61: LGTM —registerGuildMemberAddHandlercorrectly resolves per-guild config.The
_configconvention communicates backward compatibility clearly, andgetConfig(member.guild.id)is safe sinceGuildMemberAddalways carries a valid guild.
69-77: LGTM — per-guild config resolution inregisterMessageCreateHandler.Guard on line 73 (
if (!message.guild) return) guaranteesmessage.guild.idon line 76 is safe.tests/modules/events.test.js (3)
57-64: LGTM —getConfigmock and import correctly wired.The mock returns an empty object by default, and individual tests override it as needed. Clean setup.
133-147: LGTM — per-guild config properly asserted forGuildMemberAdd.Good: the test asserts both that
getConfigwas called with the correct guild ID and thatsendWelcomeMessagereceived the resolved guild config.
438-446: LGTM —generateResponseargument list verified includingguildId.The threading test correctly asserts that
generateResponsereceives'g1'as the last argument, validating the guild ID pass-through.tests/modules/ai.test.js (3)
160-174: LGTM — directly addresses the prior feedback on weak assertions.The test now verifies both that
getConfigwas called with the guild ID and that history was actually trimmed to the configured length, with a content check on the oldest retained message.
379-418: LGTM — solid coverage for guildId propagation throughgenerateResponse.Lines 379–403 confirm end-to-end guildId threading into both memory operations; lines 405–418 use the corrected test name matching the actual assertion. Both address previous review feedback.
279-284: No action required —guildIdparameter is correctly defaulted tonull.The function signature at line 408 of
src/modules/ai.jsdeclaresguildId = nullas the default parameter. When the test callsgenerateResponsewith 5 arguments (omittingguildId), it defaults tonull, which is then correctly forwarded to bothbuildMemoryContext(line 426) andextractAndStoreMemories(line 489). The test assertions expectingnullas the final argument are correct and will pass.src/modules/config.js (1)
200-231: Column type confirmed as JSONB — code is correct, noJSON.parse()needed.The
config.valuecolumn is defined asJSONB NOT NULLin src/db.js (line 96). The PostgreSQL pg driver automatically parses JSONB columns and returns them as JavaScript objects. Therefore,row.valueat lines 213, 230, and 181 is already a parsed object, and the code is correct as written.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/modules/config.js`:
- Around line 446-481: The transaction error (txErr) is re-thrown without
logging and pool.connect() is outside the transaction try/catch so its failure
isn't logged; wrap the connection and transaction in an error-handling block or
add a dedicated try/catch around pool.connect() to call logError(...) with
contextual info (include guildId and section) before re-throwing, and in the
existing catch(txErr) call logError(txErr, { guildId, section, action:
'upsertConfig' }) (or similar context) before attempting ROLLBACK and
re-throwing txErr so all errors are logged with context.
- Around line 530-694: resetConfig currently mutates
configCache/mergedConfigCache and increments globalConfigGeneration but never
calls emitConfigChangeEvents, leaving onConfigChange listeners stale; update
resetConfig (both guild branch and global section/full reset paths) to call
await emitConfigChangeEvents for each affected config path after the in-memory
state changes (use the keys removed/updated from configCache for guild resets
and iterate fileConfig keys for global section/full reset) so listeners see the
exact changed paths, keeping the behavior consistent with setConfigValue which
calls await emitConfigChangeEvents; ensure calls happen after mutating caches
and before returning and keep mergedConfigCache.clear() and
globalConfigGeneration++ where present.
In `@src/modules/events.js`:
- Around line 135-142: The optional chaining on message.guild?.id in the call to
generateResponse is unnecessary because earlier logic already returns when
message.guild is falsy; update the invocation to use message.guild.id instead.
Locate the generateResponse call that passes historyId, cleanContent,
message.author.username, healthMonitor, message.author.id, and message.guild?.id
and replace the last argument with message.guild.id to avoid the misleading ?.
while preserving the same argument order and behavior.
---
Duplicate comments:
In `@tests/modules/events.test.js`:
- Around line 169-171: Tests for MessageCreate use
getConfig.mockReturnValue(config) but lack an assertion that getConfig was
called with the guild id; add an expectation in at least one representative
MessageCreate test (e.g., the spam detection or bot-mention test) to assert
getConfig was invoked with 'g1' by calling
expect(getConfig).toHaveBeenCalledWith('g1') after triggering the MessageCreate
handler, mirroring the GuildMemberAdd test's pattern so regressions calling
getConfig() with no args will fail.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Add DANGEROUS_KEYS filtering in deepMerge to skip __proto__, constructor, and prototype keys during object iteration. This prevents prototype pollution when untrusted JSON values containing these keys are merged via getConfig(guildId). The loadConfig DB-loading paths already filter DANGEROUS_KEYS on row.key. Adds two tests verifying deepMerge skips dangerous keys during merge.
BillChirico
dismissed stale reviews from coderabbitai[bot] and claude[bot]
via
cb7ae74
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/modules/config.jstests/modules/config-guild.test.js
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
tests/modules/config-guild.test.jssrc/modules/config.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/modules/config.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Check
config.yourModule.enabledbefore processing any module logic to allow runtime enable/disable
Files:
src/modules/config.js
🧠 Learnings (4)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/modules/config-guild.test.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Prefer the per-request `getConfig()` pattern for modules; only add reactive `onConfigChange` listeners for stateful resources (e.g., Winston transports) that cannot re-read config on each invocation
Applied to files:
src/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `getConfig()` from `src/modules/config.js` to read runtime configuration
Applied to files:
src/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : The Database.js pool must be initialized before command loading; the startup sequence in `src/index.js` is: initialize pool → load config → register clients → load commands → set up heartbeat
Applied to files:
src/modules/config.js
🧬 Code graph analysis (1)
src/modules/config.js (2)
src/db.js (2)
pool(13-13)getPool(246-251)src/modules/moderation.js (1)
pool(89-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
🔇 Additional comments (11)
src/modules/config.js (5)
66-77: LGTM —deepMergewith prototype-pollution guard.Clean recursive merge.
DANGEROUS_KEYSfiltering andstructuredClonefor leaf values correctly prevent pollution and shared references.
105-251: LGTM —loadConfigwith guild-aware DB loading and DANGEROUS_KEYS validation.All three DB-loading paths (seed, normal global rows, guild rows) now consistently check
DANGEROUS_KEYS. Guild overrides are correctly loaded during the seed path as well (lines 171–184).
276-307: LGTM —getConfigwith generation-based merged cache and LRU tracking.The generation-based staleness check, LRU refresh via delete/re-set, and the separate code paths for no-override vs. override guilds are correct. The documented asymmetry between global (live reference) and guild (cloned) return semantics is clear.
412-522: LGTM —setConfigValuewith guild-aware DB persistence, cache invalidation, and event emission.Row-level locking, per-guild INSERT/UPDATE, proper cache invalidation (global clears all merged entries; guild-specific deletes only that guild), and structured logging are all solid.
646-657: Good addition — orphan warning for per-guild rows referencing deleted global keys.This addresses the previously raised concern about stale guild overrides surviving global key cleanup. Warning-only is appropriate as a first step; the orphaned rows don't cause crashes (guild overrides just apply without a global baseline), and automatic deletion could be surprising.
tests/modules/config-guild.test.js (6)
72-98: Good — mutation-safety test for the global path now included.The test at lines 90–97 explicitly documents and verifies the live-reference contract for
getConfig()(no args). This addresses the previously suggested improvement.
100-127: Good — generation-based cache invalidation is well-tested.Both
setConfigValue(global change) andresetConfig(section reset) correctly invalidate the merged guild cache, and the tests verify guild configs reflect the updated global defaults.
129-159: Good — guild isolation tests are thorough.Tests cover bidirectional isolation: guild→global, guild→guild, and confirm global remains untouched.
257-289: Good — setConfigValue with guildId tests cover DB default, guild-specific writes, and event emission with both guild and global IDs.
291-336: Good — resetConfig tests cover section reset, full reset, and cross-guild isolation during resets.
338-356: Good — multi-guild concurrency test validates isolation at scale.10 guilds with independent overrides and verified global preservation. Coverage is appropriate for the in-memory-only test mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/config.js`:
- Around line 532-574: resetConfig currently mutates
configCache/mergedConfigCache but does not notify listeners, so consumers
subscribed via onConfigChange won't see resets; update resetConfig to call
emitConfigChangeEvents (the same helper used by setConfigValue) for each
affected section/key when a guild or global reset occurs — determine the list of
sections removed by inspecting configCache before deletion (e.g., keys in
guildConfig or top-level keys for global), then invoke emitConfigChangeEvents
with those section identifiers (e.g., "ai.*" style or the section key) after
performing DB and in-memory removals so listeners receive the change; ensure
mergedConfigCache deletion still occurs and that you handle the case of
section-specific vs full resets consistently.
In `@tests/modules/config-guild.test.js`:
- Around line 193-238: The tests never exercise deepMerge's DANGEROUS_KEYS
branch because ai.threadMode is absent from global config so deepMerge uses
structuredClone; add a test that creates a global ai.threadMode object (e.g.,
via configModule.setConfigValue('ai.threadMode', '{"safe":"yes"}')) and then
sets a guild override containing dangerous keys (e.g., '{"__proto__":
{"polluted":"yes"}, "extra":"val"}'), call configModule.getConfig(guildId) to
trigger deepMerge and assert Object.prototype.polluted is undefined; reference
deepMerge and DANGEROUS_KEYS in the test to ensure the recursive path is
exercised and cleaned up via configModule.resetConfig afterwards.
- Around line 30-44: The beforeEach block incorrectly re-registers hoisted mocks
by calling vi.mock() again; remove the vi.mock(...) calls inside beforeEach and
keep only vi.resetModules() so the top-level mocks remain in effect, and if you
need per-test variants use vi.doMock() within individual it() blocks instead;
update the beforeEach that contains beforeEach, vi.resetModules, and
vi.mock(...) to only call vi.resetModules().
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| @@ -176,7 +184,7 @@ async function handleSetup(interaction) { | |||
| */ | |||
There was a problem hiding this comment.
Bug (same pattern as config.js): The permission check at the top of execute() (line 35) still calls getConfig() without interaction.guildId, so it always evaluates permissions against the global config. This was missed when the sub-handlers (handleView, handleSetup, handleDisable) were correctly updated to use getConfig(interaction.guildId).
If a guild has customized permissions.adminRoleId or permissions.allowedCommands.modlog, the override is ignored during the permission gate, potentially denying or granting access incorrectly.
The fix is the same as what needs to happen in config.js:161:
const config = getConfig(interaction.guildId);
| const focusedOption = interaction.options.getFocused(true); | ||
| const focusedValue = focusedOption.value.toLowerCase().trim(); | ||
| const config = getConfig(); | ||
| const config = getConfig(interaction.guildId); |
There was a problem hiding this comment.
Bug: The autocomplete handler was correctly updated to use getConfig(interaction.guildId) here, but the execute() function's permission check at line 161 was not updated — it still calls getConfig() (no guildId). This means the permission gate always evaluates against global config, ignoring any per-guild permissions.adminRoleId or permissions.allowedCommands.config overrides.
Line 161 should be:
const config = getConfig(interaction.guildId);
There was a problem hiding this comment.
Review Summary
After reviewing all 38 changed files across this per-guild configuration PR, the architecture is solid — deep merge with structuredClone isolation, generation-based staleness detection for mergedConfigCache, prototype pollution protection via DANGEROUS_KEYS, and proper DB transaction isolation with FOR UPDATE row locking. The test coverage in config-guild.test.js is thorough, covering guild isolation, deep merge behavior, prototype pollution, cache invalidation, and reset event emission.
However, there are two instances of the same bug that need to be fixed before merging:
Issues Found
1. src/commands/config.js:161 — Permission check uses global config instead of guild config
execute() calls getConfig() (no guildId) for the permission check, while all sub-handlers (handleView, handleSet, handleReset) correctly use getConfig(interaction.guildId). If a guild has customized permissions.adminRoleId or permissions.allowedCommands.config, those overrides are ignored at the permission gate.
2. src/commands/modlog.js:35 — Same pattern: permission check uses global config
Identical issue. The sub-handlers (handleView, handleSetup, handleDisable) were correctly updated to use getConfig(interaction.guildId), but the top-level permission check in execute() was missed.
Both fixes are one-line changes: getConfig() → getConfig(interaction.guildId).
Reviewed files: AGENTS.md, src/modules/config.js, src/db.js, src/index.js, src/modules/events.js, src/modules/ai.js, src/modules/memory.js, src/modules/threading.js, src/modules/moderation.js, src/api/routes/guilds.js, src/commands/config.js, src/commands/modlog.js, src/commands/warn.js, tests/modules/config-guild.test.js, tests/commands/config.test.js, and spot-checked remaining command files.
There was a problem hiding this comment.
Review Summary
Thorough review of all 38 changed files covering core config module, DB migration, event handlers, AI/memory/threading modules, 16 command files, API routes, moderation module, and test suite.
The architecture is well-designed: two-tier caching (authoritative configCache + LRU mergedConfigCache), generation-based staleness detection, prototype pollution protection, and proper cache invalidation on global vs guild changes. Most guildId threading is correct throughout the codebase.
Issues Found
🟡 Warning: src/commands/config.js:161 — getConfig() missing guildId
The execute() permission guard calls getConfig() without interaction.guildId, reading global defaults instead of per-guild merged config. The PR correctly updated autocomplete, handleView, handleSet, and handleReset in this same file, but missed the execute() entry point.
If a guild customises permissions.allowedCommands.config, this check will evaluate against global defaults rather than the guild's config. The central permission check in src/index.js:188 is correct, so the outer gate works — but the inner gate here could diverge.
Fix:
// src/commands/config.js:161
const config = getConfig(interaction.guildId);🟡 Warning: src/commands/modlog.js:35 — getConfig() missing guildId
Same issue as above. The execute() permission guard calls getConfig() without interaction.guildId. The PR correctly updated handleSetup, handleView, and handleDisable in this same file, but missed the execute() entry point.
Fix:
// src/commands/modlog.js:35
const config = getConfig(interaction.guildId);🔵 Nitpick: src/utils/permissions.js:112 — Stale TODO referencing this PR
isGuildAdmin has a TODO(#71) comment saying to check guild-scoped admin roles once per-guild config lands. Since this PR implements #71, either resolve the TODO or leave a note that it's deferred to a follow-up.
Both warnings are the same class of bug: permission checks in command execute() functions not passing guildId to getConfig(). The fixes are one-line each. The rest of the PR is solid — good test coverage, correct cache invalidation, proper prototype pollution protection, and consistent guildId threading through all other modules.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/modlog.js (1)
34-42:⚠️ Potential issue | 🟡 MinorPermission check uses global config instead of guild-specific config.
getConfig()at line 35 returns global defaults, so guild-specific permission overrides are ignored for this command's own permission check. Other commands likeban.jscorrectly usegetConfig(interaction.guildId).Note: The index.js
interactionCreatehandler also runs a guild-aware permission check, so this isn't exploitable — but it creates an inconsistency where the two checks could disagree (e.g., a guild enables modlog for moderators but the global config restricts it to admins, causing confusing error messages from this inner check).Proposed fix
export async function execute(interaction) { - const config = getConfig(); + const config = getConfig(interaction.guildId); if (!hasPermission(interaction.member, 'modlog', config)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/modlog.js` around lines 34 - 42, The permission check in execute currently calls getConfig() which returns global defaults; change it to use the guild-specific config by calling getConfig(interaction.guildId) before invoking hasPermission so the modlog permission check uses guild overrides; keep the rest of the flow (using hasPermission, computing permLevel from config.permissions?.allowedCommands?.modlog, and replying via safeReply with getPermissionError and ephemeral: true).src/commands/config.js (1)
160-168:⚠️ Potential issue | 🟡 MinorPermission check uses global config — same issue as
modlog.js.
getConfig()at line 161 returns global defaults, bypassing any guild-specific permission overrides. This mirrors the same inconsistency flagged insrc/commands/modlog.jsline 35.Proposed fix
export async function execute(interaction) { - const config = getConfig(); + const config = getConfig(interaction.guildId); if (!hasPermission(interaction.member, 'config', config)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/config.js` around lines 160 - 168, The code calls getConfig() which returns global defaults and bypasses guild-specific overrides; update execute to use the guild-specific config (e.g., call getConfig(interaction.guild?.id) or the project's getGuildConfig(interaction.guildId)) and use that returned config for hasPermission and deriving permLevel so permission checks respect per-guild settings; ensure you replace the current getConfig() usage in the execute function and keep safeReply/getPermissionError unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/api/routes/guilds.jssrc/commands/config.jssrc/commands/modlog.jssrc/index.jssrc/modules/config.jstests/api/routes/guilds.test.jstests/commands/config.test.jstests/commands/modlog.test.jstests/modules/config-guild.test.jstests/modules/config.test.js
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,mjs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,mjs}: Use ESM modules withimport/exportsyntax; never userequire()
Always usenode:protocol prefix when importing Node.js built-in modules (e.g.,import { readFileSync } from 'node:fs')
Always use semicolons at the end of statements
Use single quotes for string literals; double quotes are not permitted
Use 2-space indentation for all code
Files:
tests/modules/config-guild.test.jstests/commands/modlog.test.jssrc/commands/config.jstests/commands/config.test.jssrc/commands/modlog.jstests/api/routes/guilds.test.jssrc/index.jssrc/api/routes/guilds.jstests/modules/config.test.jssrc/modules/config.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: NEVER useconsole.log(),console.warn(),console.error(), or anyconsole.*method in src/ files; always use Winston logger instead viaimport { info, warn, error } from '../logger.js'
Pass structured metadata to Winston logging calls (e.g.,info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig()fromsrc/modules/config.jsto read runtime configuration
UsesetConfigValue(key, value)fromsrc/modules/config.jsto update configuration at runtime
Always usesafeSend()wrapper fromsrc/utils/safeSend.jsfor sending messages to enforce allowedMentions and prevent mention exploits
UsesplitMessage()utility fromsrc/utils/splitMessage.jsto split messages exceeding Discord's 2000-character limit
Files:
src/commands/config.jssrc/commands/modlog.jssrc/index.jssrc/api/routes/guilds.jssrc/modules/config.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export adataobject usingSlashCommandBuilderand anexecute(interaction)async function
ExportadminOnly = truefrom command files to restrict execution to moderators
Files:
src/commands/config.jssrc/commands/modlog.js
src/index.js
📄 CodeRabbit inference engine (AGENTS.md)
src/index.js: The Database.js pool must be initialized before command loading; the startup sequence insrc/index.jsis: initialize pool → load config → register clients → load commands → set up heartbeat
Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Files:
src/index.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Check
config.yourModule.enabledbefore processing any module logic to allow runtime enable/disable
Files:
src/modules/config.js
🧠 Learnings (11)
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Test config structure, command exports, and utility functions; avoid testing live Discord connections (smoke/unit tests only)
Applied to files:
tests/modules/config-guild.test.jstests/commands/modlog.test.jstests/commands/config.test.jstests/api/routes/guilds.test.jssrc/index.jstests/modules/config.test.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Use Vitest for unit and integration tests; run `pnpm test` and `pnpm test:coverage` before every commit
Applied to files:
tests/modules/config-guild.test.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `setConfigValue(key, value)` from `src/modules/config.js` to update configuration at runtime
Applied to files:
src/commands/config.jssrc/api/routes/guilds.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Moderation commands must follow the pattern: deferReply → validate inputs → sendDmNotification → execute Discord action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
src/commands/config.jssrc/commands/modlog.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Prefer the per-request `getConfig()` pattern for modules; only add reactive `onConfigChange` listeners for stateful resources (e.g., Winston transports) that cannot re-read config on each invocation
Applied to files:
src/index.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/**/*.js : Use `getConfig()` from `src/modules/config.js` to read runtime configuration
Applied to files:
src/index.jssrc/modules/config.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : Enable Discord intents for MessageContent, GuildMembers, and GuildVoiceStates in client initialization
Applied to files:
src/index.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*{ban,kick,warn,timeout,mute}*.js : Always call `checkHierarchy(moderator, target)` from `src/utils/permissions.js` before executing moderation actions to prevent role hierarchy violations
Applied to files:
src/index.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/commands/*.js : Slash commands must export a `data` object using `SlashCommandBuilder` and an `execute(interaction)` async function
Applied to files:
src/index.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/modules/events.js : Register module event handlers in `src/modules/events.js` via `client.on()` and pass `config` to handler functions
Applied to files:
src/index.js
📚 Learning: 2026-02-17T15:54:40.545Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-17T15:54:40.545Z
Learning: Applies to src/index.js : The Database.js pool must be initialized before command loading; the startup sequence in `src/index.js` is: initialize pool → load config → register clients → load commands → set up heartbeat
Applied to files:
tests/modules/config.test.jssrc/modules/config.js
🧬 Code graph analysis (9)
tests/modules/config-guild.test.js (11)
src/api/routes/guilds.js (1)
config(285-285)src/commands/config.js (3)
config(125-125)config(161-161)config(199-199)src/commands/modlog.js (2)
config(35-35)config(187-187)src/index.js (3)
config(57-57)guildId(157-157)guildConfig(188-188)src/modules/memory.js (1)
config(142-142)src/modules/threading.js (2)
config(67-67)config(371-371)src/commands/ban.js (1)
config(44-44)src/commands/kick.js (1)
config(36-36)src/commands/case.js (1)
config(222-222)src/commands/lock.js (1)
config(55-55)src/modules/config.js (6)
guildConfig(494-494)guildConfig(631-631)globalConfig(206-206)globalConfig(295-295)globalConfig(670-670)i(808-808)
tests/commands/modlog.test.js (1)
src/modules/config.js (1)
setConfigValue(480-590)
src/commands/config.js (4)
src/api/routes/guilds.js (1)
config(285-285)src/commands/modlog.js (2)
config(35-35)config(187-187)src/commands/ban.js (1)
config(44-44)src/modules/config.js (1)
getConfig(277-308)
tests/commands/config.test.js (2)
src/modules/config.js (2)
setConfigValue(480-590)resetConfig(600-778)src/index.js (1)
interaction(182-182)
src/commands/modlog.js (3)
src/modules/config.js (2)
setConfigValue(480-590)getConfig(277-308)src/api/routes/guilds.js (1)
config(285-285)src/commands/ban.js (1)
config(44-44)
tests/api/routes/guilds.test.js (1)
src/modules/config.js (2)
getConfig(277-308)setConfigValue(480-590)
src/index.js (3)
src/modules/config.js (5)
guildConfig(494-494)guildConfig(631-631)getConfig(277-308)onConfigChange(348-350)path(425-425)src/modules/ai.js (1)
guildConfig(412-412)src/utils/permissions.js (1)
hasPermission(61-97)
src/api/routes/guilds.js (1)
src/modules/config.js (3)
getConfig(277-308)setConfigValue(480-590)path(425-425)
tests/modules/config.test.js (3)
src/index.js (1)
guildConfig(188-188)src/modules/config.js (2)
guildConfig(494-494)guildConfig(631-631)src/modules/ai.js (1)
guildConfig(412-412)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cursor Bugbot
- GitHub Check: claude-review
- GitHub Check: claude-review
🔇 Additional comments (33)
tests/modules/config.test.js (4)
143-168: Well-structured test for guild override persistence during seeding.The test correctly verifies that pre-existing guild overrides in the DB are preserved when global config is seeded from
config.json, and thatgetConfig('guild-99')returns a deep-merged result with the guild override forai.modeland the global default forai.enabled.
170-206: Solid prototype pollution guard test.Good coverage of dangerous key filtering (
__proto__,constructor,prototype) during recursivedeepMergeof guild overrides. Thetry/finallycleanup ofObject.prototype.pollutedis the right approach. Line 199's assertion thatguildConfig.ai.constructorisObject(the built-in) rather than the injected value is a particularly useful check.
257-282: Good cache invalidation test.This properly verifies that reloading config clears the merged guild cache so that stale entries don't persist. The two-call pattern with
.mockResolvedValueOncechaining is clean.
208-223: The mock rows are correctly testing graceful handling of missingguild_id.The
loadConfigfunction explicitly handles undefinedguild_idby treating it as'global'(line 145:!r.guild_id || r.guild_id === 'global'). The code comment at line 144 confirms this is intentional: "Treat rows with missing/undefined guild_id as 'global' (handles unmigrated DBs)." The current test mock withoutguild_idcorrectly validates this backward-compatibility behavior. Addingguild_id: 'global'to the mock would remove coverage of that graceful handling case.Likely an incorrect or invalid review comment.
src/index.js (3)
188-190: Correct per-guild permission check at the command dispatch level.Using
getConfig(interaction.guildId)here ensures guild-specific permission overrides are respected for all commands dispatched throughinteractionCreate. This is the right place for this change.
378-380: Good guard against per-guild changes affecting the global logging transport.The early return for non-global
guildIdcorrectly prevents guild-specific config changes from triggering logging transport operations. This is necessary because the PostgreSQL logging transport is a singleton shared resource.
390-400: LGTM — observability listeners now include guildId context.Including
guildIdin the structured log metadata is essential for debugging per-guild config changes in a multi-tenant environment.tests/commands/modlog.test.js (2)
44-44: Correctly propagatedguildIdin the test interaction factory.Adding
guildId: 'test-guild'tocreateInteractionensures all modlog tests exercise the guild-scoped config path consistently.
173-207: Test expectations properly updated for guild-scopedsetConfigValue.All seven channel disable calls now correctly assert the third
'test-guild'argument, matching the updatedsetConfigValue(path, value, guildId)signature.src/commands/modlog.js (3)
138-147: Guild-scoped config writes and logging in handleSetup look correct.
setConfigValueandinfoboth correctly includeinteraction.guildId, ensuring per-guild channel routing configuration and audit trail.
187-187: Correctly uses guild-scoped config for handleView.
220-223: handleDisable correctly passes guildId and includes it in the log.The previous review flagged the missing
guildIdin the disable log — this has been addressed.src/commands/config.js (2)
125-125: Guild-scoped config access throughout autocomplete, view, set, and reset is correct.All internal handlers properly propagate
interaction.guildIdtogetConfig,setConfigValue, andresetConfig.Also applies to: 199-199, 288-288, 300-300
344-352: Reset messages correctly describe guild override behavior.The updated wording ("Guild overrides for section X have been cleared; global defaults will apply") accurately reflects what
resetConfig(section, guildId)does for non-global guilds.src/api/routes/guilds.js (2)
284-296: Per-guild config read is correct and well-scoped.
getConfig(req.params.id)returns the merged config, and the response correctly filters toREADABLE_CONFIG_KEYSonly. IncludingguildIdin the response improves API clarity.
334-339: PATCH correctly writes per-guild overrides and returns effective config.The updated flow — write via
setConfigValue(path, value, req.params.id), then read back the effective merged section viagetConfig(req.params.id)— is a good API design improvement. The caller sees the actual effective config after the update rather than just the raw override value.tests/commands/config.test.js (3)
113-113: Guild context correctly propagated across all autocomplete and view test interactions.Also applies to: 130-130, 149-149, 166-166
304-318: Set subcommand tests correctly assert guild-scopedsetConfigValuecalls.Line 318 properly verifies the three-argument signature
('ai.model', 'new-model', 'test-guild').Also applies to: 325-325, 350-350, 378-378
407-434: Reset subcommand tests correctly assert guild-scopedresetConfigcalls.Line 417 (
resetConfig('ai', 'test-guild')) and line 434 (resetConfig(undefined, 'test-guild')) correctly match the command code atconfig.jsline 344:resetConfig(section || undefined, interaction.guildId).tests/api/routes/guilds.test.js (2)
390-394: GET config test correctly verifies guild-scoped config retrieval.The assertion at line 394 (
getConfigcalled with'guild1') confirms the route uses per-guild config lookup. The updatedaiexpectation at line 390 matches the enriched mock at line 13.
409-421: PATCH config test correctly verifies guild-scoped write and effective config response.The test now:
- Mocks
getConfigto return the full effective section (line 409–411)- Asserts the response body matches the effective merged section (line 419)
- Verifies
setConfigValuereceives'guild1'as the guildId (line 420)- Verifies
getConfigis called with'guild1'for the response (line 421)This accurately reflects the updated PATCH handler behavior.
tests/modules/config-guild.test.js (3)
1-57: Well-structured test setup with proper module isolation.The pattern of top-level
vi.mock()+vi.resetModules()inbeforeEach+ dynamicawait import(...)correctly ensures each test gets a fresh config module instance with clean internal state. The mock config covers multiple sections (ai, spam, moderation, welcome) providing good breadth for merge/isolation tests.
77-84: Good: mutation-safety test for the global path documents the intentional asymmetry.This test explicitly captures the contract that
getConfig()(no args) returns a live mutable reference, which is the correct behavior per the JSDoc ongetConfiginconfig.js. This guards against accidental regressions if someone tries to clone the global path.
226-257: RecursivedeepMergeDANGEROUS_KEYS coverage is now thorough.This test properly sets a global
ai.threadModeas a plain object (forcing the recursive merge path) and then overlays a guild override with__proto__,constructor, andprototypeat the nested level. The assertions at lines 248–251 verify that all three dangerous keys are filtered. This directly exercises theDANGEROUS_KEYSguard insidedeepMerge.src/modules/config.js (9)
17-54: Thorough documentation of caching strategy and trade-offs.The JSDoc comments on
configCache(unbounded, source-of-truth) andglobalConfigGeneration(deprecation warning for in-place mutations) clearly communicate the design intent and limitations. This makes it easy for future contributors to understand the boundaries.
59-78:deepMergeis correct and well-guarded.Uses
Object.keys(own-enumerable only), filtersDANGEROUS_KEYS, recurses on matching plain objects, andstructuredClones non-object overrides. This prevents prototype pollution while preserving guild override semantics.
277-308:getConfigguild path is correctly isolated with generation-based staleness detection.The LRU refresh on cache hit (delete+set), generation-based invalidation on global changes, and
structuredCloneon every guild return path all ensure proper isolation. The documented asymmetry between global (live reference) and guild (cloned) paths is a reasonable backward-compatibility trade-off.
377-403: Event emission correctly snapshots the listener list and propagatesguildId.Spreading
[...listeners]on line 378 prevents ConcurrentModificationException-style issues if a callback registers/unregisters listeners. The async error handling (lines 388–393 for promises, 395–399 for sync) is robust.
556-589: In-memory cache update and invalidation logic is correct.The guild-specific invalidation (line 584:
mergedConfigCache.delete(guildId)) vs. global invalidation (lines 581–582:mergedConfigCache.clear()+ generation bump) correctly reflects the dependency graph — all merged entries depend on global, but only one depends on a specific guild's overrides.
600-650: Guild reset correctly captures before/after effective configs for event emission.The pattern of calling
getConfig(guildId)before and after the reset (lines 603, 642) to get independent cloned snapshots, then computing leaf diffs viagetChangedLeafEvents, ensures listeners receive accurate change notifications — including values that revert to global defaults.
836-838:isPlainObjectis sufficient for JSON-derived config values.The check returns
truefor any non-null, non-array object (including Date, RegExp, etc.), but since config values come fromJSON.parseorparseValue, only true plain objects reach this path. No issue in practice.
106-141:loadConfigproperly handles all configuration source scenarios.The three paths — no DB (file fallback, line 132–135), empty DB (seed + load guild overrides, lines 148–200), populated DB (lines 201–240) — are well-structured. The upfront
mergedConfigCache.clear()+globalConfigGeneration++(lines 109–110) correctly invalidates all stale merged entries before rebuilding. The scalability note on line 138–140 is a good callout.
715-734: Orphan warning is a good defense-in-depth measure.Logging orphaned per-guild rows (lines 723–734) rather than silently ignoring them helps operators identify stale config entries after a global reset removes sections from
config.json. The decision to warn-only (not auto-delete) is reasonable — guild overrides may be intentionally preserved for sections that could be re-added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/config.js`:
- Around line 316-324: The cacheMergedResult function currently calls
mergedConfigCache.set(guildId, ...) which does not refresh insertion order for
existing keys; change cacheMergedResult to first remove any existing entry for
guildId (mergedConfigCache.delete(guildId)) before calling
mergedConfigCache.set(guildId, {...}) so rebuilding a stale entry refreshes its
LRU position consistent with the getConfig cache-hit path and prevents premature
eviction of recently accessed guilds.
In `@tests/modules/config-guild.test.js`:
- Around line 389-407: Add a cross-guild reset step to the "multiple guilds
simultaneously" test: after setting and asserting per-guild models via
configModule.setConfigValue and configModule.getConfig, call the guild-specific
reset function (e.g., configModule.resetConfig(targetGuildId)) for one guild and
then re-check that the reset guild's ai.model reverted to the global default
while all other guilds still return their original `model-${guildId}` values;
use the same guildIds array and reference configModule.setConfigValue,
configModule.getConfig, and configModule.resetConfig to locate and implement the
assertions.
- Around line 292-308: Add a brief inline comment above the first assertion in
tests/modules/config-guild.test.js explaining that the expected oldValue is
undefined because the guild had no prior override (so the emitted oldValue
represents the guild-specific previous value, not the global default); reference
the test helpers configModule.onConfigChange and configModule.setConfigValue to
locate the block and place the comment immediately before the
expect(cb).toHaveBeenCalledWith('new-model', undefined, 'ai.model', 'guild-123')
assertion.
---
Outside diff comments:
In `@src/commands/config.js`:
- Around line 160-168: The code calls getConfig() which returns global defaults
and bypasses guild-specific overrides; update execute to use the guild-specific
config (e.g., call getConfig(interaction.guild?.id) or the project's
getGuildConfig(interaction.guildId)) and use that returned config for
hasPermission and deriving permLevel so permission checks respect per-guild
settings; ensure you replace the current getConfig() usage in the execute
function and keep safeReply/getPermissionError unchanged.
In `@src/commands/modlog.js`:
- Around line 34-42: The permission check in execute currently calls getConfig()
which returns global defaults; change it to use the guild-specific config by
calling getConfig(interaction.guildId) before invoking hasPermission so the
modlog permission check uses guild overrides; keep the rest of the flow (using
hasPermission, computing permLevel from
config.permissions?.allowedCommands?.modlog, and replying via safeReply with
getPermissionError and ephemeral: true).
| function cacheMergedResult(guildId, merged) { | ||
| mergedConfigCache.set(guildId, { generation: globalConfigGeneration, data: merged }); | ||
|
|
||
| // Evict least-recently-used guild entries when cap is exceeded | ||
| if (mergedConfigCache.size > MAX_GUILD_CACHE_SIZE) { | ||
| const firstKey = mergedConfigCache.keys().next().value; | ||
| mergedConfigCache.delete(firstKey); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LRU insertion order not refreshed when rebuilding a stale entry.
When getConfig(guildId) encounters a stale cached entry (generation mismatch), it falls through to rebuild and calls cacheMergedResult. Since Map.set() on an existing key preserves the original insertion order, the rebuilt entry keeps its old position, making it a premature eviction candidate despite being just accessed.
The cache-hit path (lines 289–290) correctly uses delete+set to refresh order. Applying the same pattern here would keep the LRU behavior consistent.
♻️ Proposed fix
function cacheMergedResult(guildId, merged) {
+ mergedConfigCache.delete(guildId);
mergedConfigCache.set(guildId, { generation: globalConfigGeneration, data: merged });
// Evict least-recently-used guild entries when cap is exceeded
if (mergedConfigCache.size > MAX_GUILD_CACHE_SIZE) {
const firstKey = mergedConfigCache.keys().next().value;
mergedConfigCache.delete(firstKey);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/config.js` around lines 316 - 324, The cacheMergedResult function
currently calls mergedConfigCache.set(guildId, ...) which does not refresh
insertion order for existing keys; change cacheMergedResult to first remove any
existing entry for guildId (mergedConfigCache.delete(guildId)) before calling
mergedConfigCache.set(guildId, {...}) so rebuilding a stale entry refreshes its
LRU position consistent with the getConfig cache-hit path and prevents premature
eviction of recently accessed guilds.
| it('should emit config change events with guildId', async () => { | ||
| const cb = vi.fn(); | ||
| configModule.onConfigChange('ai.model', cb); | ||
|
|
||
| await configModule.setConfigValue('ai.model', 'new-model', 'guild-123'); | ||
|
|
||
| expect(cb).toHaveBeenCalledWith('new-model', undefined, 'ai.model', 'guild-123'); | ||
| }); | ||
|
|
||
| it('should emit global guildId for global changes', async () => { | ||
| const cb = vi.fn(); | ||
| configModule.onConfigChange('ai.model', cb); | ||
|
|
||
| await configModule.setConfigValue('ai.model', 'new-model'); | ||
|
|
||
| expect(cb).toHaveBeenCalledWith('new-model', 'claude-3', 'ai.model', 'global'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the oldValue semantics in the event emission test.
Line 298 asserts oldValue is undefined (because the guild had no prior override), while line 307 asserts oldValue is 'claude-3' (because the global cache held that value). This asymmetry is consistent with the documented contract in config.js (lines 562–564), but a brief inline comment on line 298 explaining why undefined is expected (guild override didn't exist yet, so old override is undefined) would make the test more self-documenting for future contributors.
📝 Suggested comment
- expect(cb).toHaveBeenCalledWith('new-model', undefined, 'ai.model', 'guild-123');
+ // oldValue is undefined because the guild had no prior override for ai.model
+ // (events reflect the override-level change, not the effective merged value)
+ expect(cb).toHaveBeenCalledWith('new-model', undefined, 'ai.model', 'guild-123');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/config-guild.test.js` around lines 292 - 308, Add a brief
inline comment above the first assertion in tests/modules/config-guild.test.js
explaining that the expected oldValue is undefined because the guild had no
prior override (so the emitted oldValue represents the guild-specific previous
value, not the global default); reference the test helpers
configModule.onConfigChange and configModule.setConfigValue to locate the block
and place the comment immediately before the
expect(cb).toHaveBeenCalledWith('new-model', undefined, 'ai.model', 'guild-123')
assertion.
| describe('multiple guilds simultaneously', () => { | ||
| it('should handle many guilds without interference', async () => { | ||
| const guildIds = Array.from({ length: 10 }, (_, i) => `guild-${i}`); | ||
|
|
||
| // Set different models for each guild | ||
| for (const guildId of guildIds) { | ||
| await configModule.setConfigValue('ai.model', `model-${guildId}`, guildId); | ||
| } | ||
|
|
||
| // Verify each guild has its own model | ||
| for (const guildId of guildIds) { | ||
| const config = configModule.getConfig(guildId); | ||
| expect(config.ai.model).toBe(`model-${guildId}`); | ||
| } | ||
|
|
||
| // Verify global is untouched | ||
| expect(configModule.getConfig().ai.model).toBe('claude-3'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Multi-guild stress test is good; consider adding a cross-guild reset scenario.
The 10-guild simultaneous test verifies isolation nicely. Optionally, you could extend it to reset one guild mid-way and verify the others remain unaffected — this would complement the existing resetConfig tests with a higher-cardinality scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/modules/config-guild.test.js` around lines 389 - 407, Add a cross-guild
reset step to the "multiple guilds simultaneously" test: after setting and
asserting per-guild models via configModule.setConfigValue and
configModule.getConfig, call the guild-specific reset function (e.g.,
configModule.resetConfig(targetGuildId)) for one guild and then re-check that
the reset guild's ai.model reverted to the global default while all other guilds
still return their original `model-${guildId}` values; use the same guildIds
array and reference configModule.setConfigValue, configModule.getConfig, and
configModule.resetConfig to locate and implement the assertions.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // Note: oldValue is captured from the guild's override cache, not the effective (merged) value. | ||
| // This means listeners see the previous override value (or undefined if no prior override existed), | ||
| // not the previous merged value that getConfig(guildId) would have returned. | ||
| const rawOld = getNestedValue(cacheEntry[section], nestedParts); |
There was a problem hiding this comment.
Inconsistent oldValue semantics in guild config events
Low Severity
For guild-specific config changes, setConfigValue captures oldValue from the guild's override cache (yielding undefined when no prior override existed), while resetConfig captures oldValue from the effective merged config via getConfig(guildId). This means onConfigChange listeners receive inconsistent oldValue semantics depending on the operation. A set followed by a reset for the same path emits (newVal, undefined, ...) then (globalDefault, overrideVal, ...), making it impossible for listeners to reliably track effective-value transitions using oldValue.
Additional Locations (1)
| // Remove expired entries | ||
| // Remove expired entries using per-guild config | ||
| for (const [key, entry] of activeThreads) { | ||
| const config = getThreadConfig(entry.guildId); |
There was a problem hiding this comment.
Redundant per-entry config lookups in thread sweep
Low Severity
sweepExpiredThreads calls getThreadConfig(entry.guildId) for every entry in the activeThreads map. Each call invokes getConfig(guildId) which performs a structuredClone of the merged config. With up to 1000 entries, this produces up to 1000 clones every 5 minutes, when a simple per-guild-id memoization within the loop would reduce it to one clone per unique guild.
1 participant
Summary
Closes #71
Refactors the bot's configuration system from a single global config to per-guild configuration with deep merge support. Guild-specific overrides are merged onto global defaults at read time.
Changes
Core (
src/modules/config.js)configCacheis now aMap<string, Object>keyed by guild_id ('global'for defaults)getConfig(guildId?)— returns deep merge of global + guild overrides (backward compat: no args = global)setConfigValue(path, value, guildId?)— writes to guild-specific rows (defaults to'global')resetConfig(section, guildId?)— resets guild overrides (for guilds) or config.json defaults (for global)onConfigChangecallbacks receiveguildIdas 4th argumentstructuredCloneto avoid shared referencesDB Migration (
src/db.js)configtable gainsguild_id TEXT NOT NULL DEFAULT 'global'column(guild_id, key)guild_idCommand Updates (13 files)
All slash commands pass
interaction.guildIdtogetConfig():ban, case, config, kick, lock, modlog, purge, slowmode, softban, tempban, timeout, unban, unlock, untimeout, warn
Module Updates
ai.js—getHistoryLength(guildId),getHistoryTTLDays(guildId)memory.js—getMemoryConfig(guildId)threading.js—getThreadConfig(guildId), threading functions usemessage.guild?.idmoderation.js— usesrow.guild_idfor tempban scheduler config lookupsREST API (
src/api/routes/guilds.js)GET /:id/configreturns per-guild merged configPATCH /:id/configwrites guild-specific overridesAcceptance Criteria
getConfig()with no args still returns global (backward compat)Tests
tests/modules/config-guild.test.jswith 20 new tests covering:getConfig()with no args)setConfigValue/resetConfigwith guildId