feat: hard gate to prevent @here and @everyone mentions#62
feat: hard gate to prevent @here and @everyone mentions#62BillChirico merged 22 commits intomainfrom
Conversation
Two-layer protection against accidental mass pings:
Layer 1 - Client-level allowedMentions:
- Set allowedMentions: { parse: ['users'] } on Discord.js Client constructor
- Globally blocks Discord from parsing @everyone, @here, and role mentions
Layer 2 - Text sanitization (defense-in-depth):
- sanitizeMentions() utility escapes @everyone/@here with zero-width space
- safeSend/safeReply/safeFollowUp/safeEditReply wrappers enforce both
content sanitization and allowedMentions on every outgoing message
- Normal <@userid> mentions are unaffected
Tests:
- 41 test suites covering sanitizeMentions, safeSend wrappers, and
Client constructor options verification
- All 648 tests passing, coverage >80% on all metrics
Closes #61
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds mention-sanitization and safe message-sending utilities; refactors commands, modules, and index to use these wrappers; exports DISCORD_MAX_LENGTH; and updates tests to mock and validate the new utilities and sanitizers. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 finished @BillChirico's task in 2m 12s —— View job PR Review Complete ✅
SummaryVerdict: Approved ✅ All critical issues from previous reviews have been addressed:
Implementation QualityThe two-layer defense-in-depth approach is well-implemented:
Key strengths:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 One nitpick found:
Documentation gap: New utility files safeSend.js and sanitizeMentions.js should be added to the Key Files table in AGENTS.md per project documentation conventions (AGENTS.md lines 145-161).
Overall assessment: The implementation is solid — clean code, proper JSDoc, good test coverage, correct Vitest patterns, and the two-layer defense-in-depth approach for mention sanitization is well-designed. The only issue is the missing documentation update.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Replace every raw interaction.reply/followUp/editReply and channel.send/message.reply call in src/ with the safe wrappers from src/utils/safeSend.js. This ensures defense-in-depth mention sanitization on every outgoing message. Files migrated: - All 15 command files (ban, case, config, history, kick, lock, modlog, ping, purge, slowmode, softban, status, tempban, timeout, unban, unlock, untimeout, warn) - src/index.js (error handlers) - src/modules/events.js (AI responses) - src/modules/chimeIn.js (organic responses) - src/modules/moderation.js (mod log + DM notifications) - src/modules/welcome.js (welcome messages) - src/modules/spam.js (spam alerts) Tests updated with passthrough mocks for safeSend module.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
AGENTS.md (1)
89-91: 🧹 Nitpick | 🔵 TrivialExample command template still uses
interaction.reply()directly.Since the PR mandates routing all outgoing messages through safe wrappers, this example should use
safeReplyto guide new contributors toward the correct pattern.Proposed fix
+import { safeReply } from '../utils/safeSend.js'; + export const data = new SlashCommandBuilder() .setName('yourcommand') .setDescription('What it does'); export async function execute(interaction) { - await interaction.reply('Hello!'); + await safeReply(interaction, 'Hello!'); }src/commands/ping.js (1)
9-14:⚠️ Potential issue | 🔴 CriticalLine 14 will fail at runtime:
interaction.reply()returns aMessageobject directly, not an object with aresource.messageproperty.The test passes only because its mock returns a custom
{ resource: { message } }structure that doesn't match Discord.js's actual API. In production,response.resourcewill beundefined, causing a runtime error when accessingresponse.resource.messageon line 14 andcreatedTimestampon line 15.The
withResponse: trueproperty on line 11 is silently ignored by Discord.js and serves no purpose. Access the message directly:const sent = response;instead ofconst sent = response.resource.message;
🤖 Fix all issues with AI agents
In `@src/commands/history.js`:
- Around line 73-76: In the catch block that currently calls await
safeEditReply(interaction, '❌ Failed to fetch moderation history.'), wrap the
safeEditReply call with a trailing .catch(() => {}) to swallow potential promise
rejections from expired interactions (matching the pattern used in kick.js and
tempban.js); leave the logError('Command error', { error: err.message, command:
'history' }) intact and only modify the safeEditReply invocation so it becomes
safeEditReply(...).catch(() => {}) or await safeEditReply(...).catch(() => {})
depending on surrounding async usage.
In `@src/modules/events.js`:
- Line 106: The call to safeReply(message, ...) uses safeReply which is
named/typed for Interaction objects but is being passed a Discord.js Message;
confirm whether duck-typing is intentional and either (a) update safeSend.js to
document that safeReply/safeSend also accept Message objects (mentioning
safeReply and safeSend by name and that they handle objects with .reply()), or
(b) restrict usage by adding a runtime type check in safeReply to ensure the
argument is an Interaction (or provide a separate safeMessageReply helper) and
update calls in the events handler (the uses at safeReply in the events module)
accordingly; include a brief note in safeSend.js describing the supported types
and expected semantics if you choose documentation.
In `@src/utils/safeSend.js`:
- Around line 55-57: safeSend currently forwards prepared options directly and
will fail for messages over Discord's 2000-char limit; update safeSend to detect
long text (inspect options.content or the result of
prepareOptions(options).content), use the splitMessage() utility to split
content into chunks, and send each chunk sequentially (calling channel.send for
each prepared chunk), ensuring attachments/embeds are included only on the
first/appropriate chunk as needed; keep the function async and return either the
array of sent messages or the final sent message consistently.
- Around line 55-93: The four wrapper functions safeSend, safeReply,
safeFollowUp, and safeEditReply currently call
channel.send/interaction.reply/followUp/editReply directly and leak unhandled
Discord.js errors; wrap each call in a try/catch, use "return await" on the
underlying promise (e.g., return await channel.send(prepareOptions(options))) so
rejections are caught, import and use the Winston loggers (info/warn/error) from
../logger.js to log contextual error messages including which wrapper, target
(channel or interaction id/user), and the prepared options, and then throw a
meaningful custom error from src/utils/errors.js (or re-throw a wrapped custom
error) so callers get typed errors while logs retain context; ensure
prepareOptions is still used before sending and do this pattern for all four
functions.
In `@src/utils/sanitizeMentions.js`:
- Line 20: The regex constant MENTION_PATTERN in sanitizeMentions.js lacks the
case-insensitive flag; update the pattern used in the MENTION_PATTERN constant
(currently /@(everyone|here)\b/g) to include the i flag (e.g.,
/@(everyone|here)\b/gi) so it matches case-insensitive variants as a
defense-in-depth measure while preserving the global behavior.
In `@tests/commands/purge.test.js`:
- Around line 3-9: The comment "// Mock logger" is stale; update it to
accurately describe what's being mocked by the vi.mock call — e.g., indicate
that the safeSend module's helpers (safeSend, safeReply, safeFollowUp,
safeEditReply) are being mocked to forward to channel/interaction methods — so
change the comment above the vi.mock to reference safeSend and its exported
functions (safeSend, safeReply, safeFollowUp, safeEditReply) instead of "Mock
logger".
In `@tests/modules/chimeIn.test.js`:
- Around line 3-9: The comment "Mock logger" above the vi.mock block is
misleading; update it to accurately describe what's being mocked (the safeSend
utilities). Change the comment to something like "Mock safeSend utilities" or
"Mock safeSend module" near the vi.mock and ensure references to functions
safeSend, safeReply, safeFollowUp, and safeEditReply remain clear so reviewers
know these helpers are being stubbed.
In `@tests/modules/events.test.js`:
- Around line 3-9: Update the stale comment "Mock logger" above the vi.mock
block to accurately describe the mocked utilities; locate the mock that defines
safeSend, safeReply, safeFollowUp, and safeEditReply in
tests/modules/events.test.js (the vi.mock callback) and change the comment to
something like "Mock safeSend/safeReply utilities" or "Mock messaging helpers"
so it correctly describes the mocked functions.
In `@tests/utils/safeSend.test.js`:
- Around line 33-42: Add tests mirroring the existing "should override existing
allowedMentions" case for safeReply, safeFollowUp, and safeEditReply: for each
function (safeReply, safeFollowUp, safeEditReply) call it with a mock
interaction or message object and an options object containing content and
allowedMentions: { parse: ['everyone','roles','users'] }, then assert the
underlying method was invoked with allowedMentions set to SAFE_ALLOWED_MENTIONS
(just like the safeSend test does); update tests to use the same
mockChannel/mockInteraction and SAFE_ALLOWED_MENTIONS constant to ensure the
override invariant is covered across all four wrappers.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (49)
AGENTS.mdsrc/commands/ban.jssrc/commands/case.jssrc/commands/config.jssrc/commands/history.jssrc/commands/kick.jssrc/commands/lock.jssrc/commands/modlog.jssrc/commands/ping.jssrc/commands/purge.jssrc/commands/slowmode.jssrc/commands/softban.jssrc/commands/status.jssrc/commands/tempban.jssrc/commands/timeout.jssrc/commands/unban.jssrc/commands/unlock.jssrc/commands/untimeout.jssrc/commands/warn.jssrc/index.jssrc/modules/chimeIn.jssrc/modules/events.jssrc/modules/moderation.jssrc/modules/spam.jssrc/modules/welcome.jssrc/utils/safeSend.jssrc/utils/sanitizeMentions.jstests/commands/ban.test.jstests/commands/case.test.jstests/commands/history.test.jstests/commands/kick.test.jstests/commands/lock.test.jstests/commands/modlog.test.jstests/commands/ping.test.jstests/commands/purge.test.jstests/commands/slowmode.test.jstests/commands/softban.test.jstests/commands/tempban.test.jstests/commands/timeout.test.jstests/commands/unban.test.jstests/commands/unlock.test.jstests/commands/untimeout.test.jstests/commands/warn.test.jstests/index.test.jstests/modules/chimeIn.test.jstests/modules/events.test.jstests/modules/welcome.test.jstests/utils/safeSend.test.jstests/utils/sanitizeMentions.test.js
🧰 Additional context used
📓 Path-based instructions (8)
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/modules/welcome.jssrc/commands/purge.jssrc/commands/unban.jssrc/commands/modlog.jssrc/commands/unlock.jssrc/commands/warn.jssrc/modules/events.jssrc/commands/untimeout.jssrc/commands/lock.jssrc/commands/kick.jssrc/modules/spam.jssrc/commands/status.jssrc/commands/ban.jssrc/commands/case.jssrc/utils/safeSend.jssrc/modules/moderation.jssrc/commands/timeout.jssrc/commands/config.jssrc/commands/slowmode.jssrc/utils/sanitizeMentions.jssrc/index.jssrc/commands/softban.jssrc/commands/ping.jssrc/commands/history.jssrc/modules/chimeIn.jssrc/commands/tempban.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js for all modules before processing in handler functions
Check config.moduleName.enabled before processing in module handler functions
Files:
src/modules/welcome.jssrc/modules/events.jssrc/modules/spam.jssrc/modules/moderation.jssrc/modules/chimeIn.js
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/modules/events.test.jstests/commands/untimeout.test.jstests/utils/sanitizeMentions.test.jstests/commands/case.test.jstests/commands/purge.test.jstests/commands/history.test.jstests/index.test.jstests/utils/safeSend.test.jstests/modules/chimeIn.test.jstests/commands/unlock.test.jstests/commands/tempban.test.jstests/commands/modlog.test.jstests/commands/softban.test.jstests/commands/slowmode.test.jstests/commands/ban.test.jstests/commands/ping.test.jstests/modules/welcome.test.jstests/commands/kick.test.jstests/commands/timeout.test.jstests/commands/lock.test.jstests/commands/warn.test.jstests/commands/unban.test.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/purge.jssrc/commands/unban.jssrc/commands/modlog.jssrc/commands/unlock.jssrc/commands/warn.jssrc/commands/untimeout.jssrc/commands/lock.jssrc/commands/kick.jssrc/commands/status.jssrc/commands/ban.jssrc/commands/case.jssrc/commands/timeout.jssrc/commands/config.jssrc/commands/slowmode.jssrc/commands/softban.jssrc/commands/ping.jssrc/commands/history.jssrc/commands/tempban.js
src/commands/mod*.js
📄 CodeRabbit inference engine (AGENTS.md)
Moderation commands must follow the pattern: deferReply → validate → sendDmNotification → execute action → createCase → sendModLogEmbed → checkEscalation
Files:
src/commands/modlog.js
src/modules/moderation.js
📄 CodeRabbit inference engine (AGENTS.md)
Case numbering is per-guild sequential and assigned atomically inside createCase() using COALESCE(MAX(case_number), 0) + 1 in a single INSERT
Files:
src/modules/moderation.js
src/commands/{timeout,tempban,slowmode}.js
📄 CodeRabbit inference engine (AGENTS.md)
Use parseDuration() from src/utils/duration.js for duration-based commands (timeout, tempban, slowmode)
Files:
src/commands/timeout.jssrc/commands/slowmode.jssrc/commands/tempban.js
src/commands/{timeout,slowmode}.js
📄 CodeRabbit inference engine (AGENTS.md)
Discord timeouts max at 28 days and slowmode caps at 6 hours (21600s); enforce these limits in command logic
Files:
src/commands/timeout.jssrc/commands/slowmode.js
🧬 Code graph analysis (28)
src/modules/welcome.js (1)
src/utils/safeSend.js (1)
safeSend(55-57)
src/commands/purge.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/commands/unban.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/commands/modlog.js (1)
src/utils/safeSend.js (2)
safeReply(67-69)safeEditReply(91-93)
src/commands/unlock.js (1)
src/utils/safeSend.js (2)
safeEditReply(91-93)safeSend(55-57)
src/commands/warn.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/modules/events.js (1)
src/utils/safeSend.js (2)
safeReply(67-69)safeSend(55-57)
src/commands/untimeout.js (2)
src/utils/safeSend.js (1)
safeEditReply(91-93)src/commands/timeout.js (4)
reason(46-46)hierarchyError(58-58)target(41-41)caseData(69-78)
src/commands/lock.js (1)
src/utils/safeSend.js (2)
safeEditReply(91-93)safeSend(55-57)
tests/utils/sanitizeMentions.test.js (1)
src/utils/sanitizeMentions.js (3)
ZWS(14-14)sanitizeMentions(32-38)sanitizeMessageOptions(49-62)
src/commands/kick.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/modules/spam.js (1)
src/utils/safeSend.js (1)
safeSend(55-57)
src/commands/status.js (1)
src/utils/safeSend.js (2)
safeReply(67-69)safeFollowUp(79-81)
src/commands/ban.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/commands/case.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/utils/safeSend.js (2)
tests/utils/safeSend.test.js (1)
SAFE_ALLOWED_MENTIONS(5-5)src/utils/sanitizeMentions.js (1)
sanitizeMessageOptions(49-62)
src/modules/moderation.js (1)
src/utils/safeSend.js (1)
safeSend(55-57)
src/commands/timeout.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/commands/config.js (1)
src/utils/safeSend.js (2)
safeReply(67-69)safeEditReply(91-93)
src/commands/slowmode.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
tests/utils/safeSend.test.js (3)
src/utils/sanitizeMentions.js (1)
ZWS(14-14)tests/utils/sanitizeMentions.test.js (1)
ZWS(4-4)src/utils/safeSend.js (5)
SAFE_ALLOWED_MENTIONS(16-16)safeSend(55-57)safeReply(67-69)safeFollowUp(79-81)safeEditReply(91-93)
src/utils/sanitizeMentions.js (3)
tests/utils/safeSend.test.js (1)
ZWS(4-4)tests/utils/sanitizeMentions.test.js (1)
ZWS(4-4)src/commands/purge.js (1)
text(130-130)
src/index.js (1)
src/utils/safeSend.js (2)
safeReply(67-69)safeFollowUp(79-81)
src/commands/softban.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/commands/ping.js (1)
src/utils/safeSend.js (2)
safeReply(67-69)safeEditReply(91-93)
src/commands/history.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
src/modules/chimeIn.js (1)
src/utils/safeSend.js (1)
safeSend(55-57)
src/commands/tempban.js (1)
src/utils/safeSend.js (1)
safeEditReply(91-93)
⏰ 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 (45)
src/utils/sanitizeMentions.js (1)
32-62: LGTM — clean implementation with good defensive handling of non-string types.Both
sanitizeMentionsandsanitizeMessageOptionshandle edge cases well (null, undefined, non-object types). The spread-copy insanitizeMessageOptionsavoids mutating the original options object.src/commands/purge.js (1)
10-10: LGTM — clean migration tosafeEditReply.Both the success and error paths correctly use the safe wrapper, and the
.catch(() => {})on the error path appropriately handles cases where the interaction may have expired.Also applies to: 167-176
tests/commands/slowmode.test.js (1)
3-8: LGTM — consistent safeSend mock pattern.The mock correctly delegates to the underlying methods, allowing existing assertions on
interaction.editReplyto continue working transparently.src/modules/chimeIn.js (1)
13-13: LGTM —safeSendcorrectly replaces directchannel.sendcalls.Both the chunked and non-chunked paths now route through the safe wrapper, consistent with the PR's goal of covering all outgoing message paths.
Also applies to: 266-269
src/commands/unban.js (1)
10-10: LGTM — straightforward migration tosafeEditReply.Also applies to: 57-66
tests/commands/ban.test.js (1)
3-8: LGTM — consistent mock pattern for safeSend utilities.src/modules/spam.js (1)
7-7: LGTM!Clean adoption of
safeSendwrapper — import and usage are correct and consistent with the utility's API.Also applies to: 56-56
tests/commands/history.test.js (1)
3-8: LGTM!Mock correctly delegates safe wrappers to their underlying Discord.js methods, keeping test assertions against
editReply/replyetc. valid.src/modules/moderation.js (1)
11-11: LGTM!Both call sites correctly use
safeSend—GuildMember.send()andTextChannel.send()are both valid targets for the wrapper.Also applies to: 197-197, 241-241
tests/commands/timeout.test.js (1)
3-8: LGTM!Consistent
safeSendmock pattern; test coverage for the timeout command is thorough.src/modules/welcome.js (1)
7-7: LGTM!
safeSend(channel, message)with a string argument is consistent withprepareOptionshandling both string and object inputs.Also applies to: 149-149
tests/commands/softban.test.js (1)
3-8: LGTM!Standard
safeSendmock; softban tests are comprehensive including retry and hierarchy scenarios.tests/commands/tempban.test.js (1)
3-8: LGTM!Consistent mock setup; tempban tests properly verify duration validation, hierarchy checks, and error handling.
src/commands/untimeout.js (1)
10-10: LGTM!All
editReplycalls cleanly migrated tosafeEditReply. The.catch(() => {})on the error-path reply (line 65) is good defensive practice for expired interactions.Also applies to: 33-33, 39-39, 56-59, 62-65
src/commands/ban.js (1)
16-16: LGTM!Clean migration to
safeEditReply. The hierarchy check, DM-before-ban ordering, and error handling with.catch(() => {})are all preserved correctly.Also applies to: 63-63, 88-97
src/commands/status.js (1)
11-11: LGTM!Correct use of
safeReply/safeFollowUpbased on interaction state. The earlyreturnafter the permission-denied reply (line 74) properly prevents fall-through.Also applies to: 70-74, 112-112, 138-138, 149-151
tests/commands/lock.test.js (1)
4-9: LGTM!The pass-through mock correctly delegates to the underlying interaction/channel methods, keeping assertions on the original mock functions valid.
src/commands/timeout.js (1)
17-17: LGTM!Consistent migration to
safeEditReply. Duration validation (28-day cap), hierarchy check, and DM-before-timeout ordering are all correctly preserved.Also applies to: 43-43, 50-50, 55-55, 60-60, 87-96
AGENTS.md (1)
39-40: Key Files table entries look good.src/commands/softban.js (1)
16-16: LGTM!Consistent
safeEditReplymigration. The retry logic for the unban step and the distinct success/warning messages for unban failure are well preserved.Also applies to: 47-47, 54-54, 100-115
tests/index.test.js (1)
3-8: LGTM!The new test at lines 293–297 properly validates that the Client is constructed with
allowedMentions: { parse: ['users'] }, directly verifying the Layer 1 protection from Issue#61. Capturing constructor options viamocks.clientOptionsis a clean approach.Also applies to: 81-89, 293-297
tests/commands/modlog.test.js (1)
3-8: LGTM!Consistent pass-through mock matching the pattern across all command test files.
src/commands/warn.js (1)
17-17: LGTM!Clean migration to
safeEditReply. All outgoing message paths are covered, and the.catch(() => {})guard in the error handler properly prevents unhandled rejections from secondary failures.Also applies to: 40-40, 46-46, 74-77, 80-83
tests/commands/kick.test.js (1)
3-8: LGTM!Mock correctly delegates to the underlying interaction methods, allowing existing assertions on
interaction.editReplyetc. to continue working.src/commands/unlock.js (1)
10-10: LGTM!All outgoing message paths (
safeEditReplyfor interaction replies,safeSendfor channel notification) are properly wrapped.Also applies to: 40-40, 53-53, 67-67, 70-73
src/commands/lock.js (1)
10-10: LGTM!Consistent with the
unlock.jspattern. All message paths properly wrapped.Also applies to: 40-40, 53-53, 67-67, 70-73
tests/commands/unban.test.js (1)
3-8: LGTM!Standard safeSend mock, consistent with the pattern across all command tests.
tests/commands/unlock.test.js (1)
4-9: LGTM!Good coverage of unlock scenarios including channel type validation and error handling.
tests/commands/case.test.js (1)
3-8: LGTM!Standard safeSend mock. Comprehensive test coverage across all subcommands with good edge case handling.
tests/commands/untimeout.test.js (1)
3-8: LGTM!Standard safeSend mock, consistent with other command test files.
src/commands/tempban.js (1)
1-119: Clean migration tosafeEditReply.All outgoing reply paths are correctly routed through
safeEditReply. DM notification is sent before the ban (line 78 before line 84), hierarchy is checked, andparseDurationis used — all consistent with coding guidelines.tests/commands/ping.test.js (1)
4-9: Mock is consistent with the project pattern.The pass-through mock correctly isolates the command logic from the sanitization layer, which is tested separately in
tests/utils/safeSend.test.js.tests/modules/welcome.test.js (1)
235-238: Assertions correctly verify the new message structure withallowedMentions.The updated expectations properly validate that welcome messages are sent as objects with
contentandallowedMentions: { parse: ['users'] }, consistent with the safeSend integration.Also applies to: 330-333
src/commands/kick.js (1)
1-77: Clean migration tosafeEditReply.All reply paths properly use
safeEditReply. DM is sent before the kick action (line 49 before line 52), and hierarchy is checked first. Consistent with the other command migrations.tests/commands/warn.test.js (1)
3-8: Mock is consistent with the established pattern across test files.src/index.js (2)
38-38: LGTM — Client-levelallowedMentionsand safe wrapper imports.The global
allowedMentions: { parse: ['users'] }on the Client constructor is the correct discord.js v14 approach to block@everyone,@here, and role mention parsing by default. Import ofsafeReply/safeFollowUpaligns with the defense-in-depth strategy.Also applies to: 58-69
171-203: LGTM — Interaction responses consistently routed through safe wrappers.All
interaction.replyandinteraction.followUpcalls in the command handler are replaced withsafeReply/safeFollowUp, maintaining the same control flow and.catch(() => {})error swallowing pattern.src/commands/config.js (1)
8-8: LGTM — Consistent migration to safe wrappers across all config subcommand paths.All reply and edit paths (view, set, reset, error handlers) correctly use
safeReply/safeEditReply. The deferred-reply check pattern (interaction.deferred ? safeEditReply : safeReply) is preserved correctly.Also applies to: 173-178, 258-266, 312-322, 347-357
src/commands/case.js (1)
11-11: LGTM — Correct use ofsafeEditReplyafterdeferReply.Since
executecallsdeferReplyat line 100, all subsequent responses correctly usesafeEditReply. The import is minimal and appropriate.Also applies to: 99-122
src/commands/modlog.js (1)
18-18: LGTM — Safe wrappers applied across all modlog paths.The replacements are consistent. The
.catch(() => {})guards on fallback replies (lines 48–50, 149, 158, 188, 212) properly handle cases where the interaction may already be expired or acknowledged.Also applies to: 46-51, 87-92, 146-150, 155-158, 182-189, 206-213
src/commands/slowmode.js (1)
11-11: LGTM — Straightforward migration tosafeEditReply.All response paths follow
deferReply, andsafeEditReplyis used consistently. Error handler retains the.catch(() => {})guard.Also applies to: 39-101
tests/utils/sanitizeMentions.test.js (1)
1-141: LGTM — Thorough test coverage for sanitization utilities.Good coverage of escaping, preservation, non-string passthrough,
sanitizeMessageOptionsfor both string and object inputs, and immutability verification. The edge cases (email addresses, null content, empty strings) are well-chosen.tests/utils/safeSend.test.js (1)
1-153: LGTM — Good functional coverage of all four safe wrappers.Tests verify sanitization,
allowedMentionsenforcement/override, user mention preservation, and return value propagation acrosssafeSend,safeReply,safeFollowUp, andsafeEditReply.src/utils/safeSend.js (2)
1-16: LGTM — clean module setup and correctallowedMentionsdefault.The constant correctly restricts parsing to
['users']only, which excludeseveryoneandrolesas intended by the linked issue.
38-45:allowedMentionsis unconditionally overwritten — confirm this is intentional.
prepareOptionsalways replaces any caller-suppliedallowedMentionswithSAFE_ALLOWED_MENTIONS. This is the right call for the security goal in#61— just confirming it's intentional since it means callers can never opt into role mentions even if a future command legitimately needs them. If that need arises, this would be the choke point to update.
Deep Review — PR #62: Hard Gate for @everyone/@here MentionsIndependent deep review of the final state across all 4 commits. Verified all callsite migrations, sanitization logic, and test coverage. New Findings — Bugs1. The
This is actively exploitable. The memory system stores user-controlled content via the external mem0 API:
The Claude bot review noted this file was "outside the scope of this PR" — but commit 3 migrated every other command and module file. Leaving the one command that handles untrusted external content unprotected creates a real vulnerability. Fix: Add 2. Email address false positive in regex — const MENTION_PATTERN = /@(everyone|here)\b/g;The New Findings — Design3. Role mentions globally disabled —
4. Custom New Findings — Test Quality5. Mock passthroughs don't verify safeSend usage — All command test files vi.mock('../../src/utils/safeSend.js', () => ({
safeReply: (t, opts) => t.reply(opts),
// ...
}));These are pure passthroughs — tests pass even if a command uses raw 6. Missing edge case tests — Not tested: email addresses ( Already Addressed by Existing Reviews
Summary
— Generated by Claude Code |
Prevents unhandled rejection if the interaction is already expired when we try to send the error message to the user.
Both Interaction.reply() and Message.reply() accept the same options shape (including allowedMentions), so safeReply works for both.
…wrappers - Long messages (>2000 chars) are automatically split into multiple sends - Each wrapper is wrapped in try/catch with Winston logger error logging - Updated JSDoc to document Message compatibility for safeReply
The comment said 'Mock logger' but the code below it mocks safeSend wrappers. Updated in purge.test.js, chimeIn.test.js, events.test.js.
- Verify all 4 wrappers override caller-supplied allowedMentions - Verify splitMessage integration splits long content automatically - Verify Winston error logging on send failures for all wrappers
There was a problem hiding this comment.
Review Summary
🔴 Critical Issue
src/commands/memory.js not migrated — bypasses BOTH protection layers
This file has 16+ direct interaction.reply() / interaction.editReply() / buttonInteraction.update() calls without using safeSend wrappers. This bypasses both defense layers:
- Layer 1 (
allowedMentionson Client constructor) does NOT apply to interaction reply methods - Layer 2 (safeSend sanitization) is never called
This is actively exploitable. The memory system stores user-controlled content via the external mem0 API. An attacker can:
- Say "Remember that I love @everyone and @here"
- Run
/memory view - Bot displays memory via raw
interaction.editReply()— unsanitized - Mass mention triggers
Required Fix: Import safeSend wrappers and migrate all direct interaction method calls in memory.js.
Other Issues Addressed in Latest Commits
The following issues flagged by CodeRabbit appear to have been addressed:
- ✅ Stale "Mock logger" comments in test files — now correctly say "Mock safeSend wrappers"
- ✅
splitMessageintegration added to safeSend.js - ✅ Winston error logging added to safeSend wrappers
- ✅
allowedMentionsoverride tests added for all four wrappers - ✅
.catch(() => {})added to error-pathsafeEditReplyin history.js
Design Notes (Non-blocking)
-
Role mentions globally disabled —
allowedMentions: { parse: ['users'] }blocks role mentions too. No commands currently need role mentions, but this restriction isn't documented. If a future feature needs role pings, the wrapper must be bypassed. -
Case-sensitivity documentation — Good to see the comment at line 20-22 in
sanitizeMentions.jsdocumenting that Discord is case-sensitive for @everyone/@here.
The PR cannot be approved until memory.js is migrated to use safeSend wrappers.
…I compatibility Interaction methods (reply/editReply/followUp) only support a single response — splitting into multiple calls breaks them. Changed safeReply, safeFollowUp, and safeEditReply to truncate content to 2000 chars instead of splitting. safeSend (channel.send) retains split behavior. Added truncateForInteraction helper and updated JSDoc on prepareOptions to document the intentional allowedMentions security override.
All 16 raw interaction.reply/editReply calls in memory.js now go through safeReply/safeEditReply. This prevents mem0-stored content containing @everyone/@here from triggering mass pings. Added safeSend mock spies in memory.test.js to verify wrappers are actually called.
Changed MENTION_PATTERN from /@(everyone|here)\b/g to /(?<!\w)@(everyone|here)\b/g — negative lookbehind prevents matching @everyone/@here preceded by word characters (e.g. user@everyone.com). Added edge case tests: email addresses, code blocks, double-sanitization idempotency, and multiple consecutive mentions.
Expanded the allowedMentions comment in index.js to explain that role mentions are intentionally disabled as defense-in-depth, and how to opt-in if needed in the future (add 'roles' to parse array + update SAFE_ALLOWED_MENTIONS in safeSend.js).
When sendOrSplit splits long content into multiple messages, it was
spreading all message options (embeds, components, files, etc.) into
every chunk via { ...prepared, content: chunk }. This caused embeds
and components to appear multiple times to the user.
Now only the last chunk carries the full payload (embeds, components,
files, etc.). All preceding chunks send only content + allowedMentions
(security property preserved on every chunk).
Added test verifying embeds/components only appear on the last chunk
when content is split into 3 parts.
BillChirico
dismissed stale reviews from claude[bot] and coderabbitai[bot]
via
15853d6
…n memory.js Add safeUpdate() to safeSend.js for component interaction updates (ButtonInteraction.update). This closes the last gap where memory.js bypassed mention sanitization — 6 buttonInteraction.update() calls in handleForgetAll and handleAdminClear now route through safeUpdate, which sanitizes @everyone/@here and enforces allowedMentions. Added tests: - safeUpdate unit tests (sanitize, truncate, override, error logging) - memory.test.js: 4 new tests verifying safeUpdate usage for all button interaction paths (forget confirm/cancel, admin clear confirm/cancel)
BillChirico
dismissed stale reviews from claude[bot] and coderabbitai[bot]
via
aec7f53
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — Two-layer mention protection implemented correctly.
Layer 1: Client-level allowedMentions: { parse: ['users'] } blocks @everyone, @here, and role mentions globally.
Layer 2: Defense-in-depth via sanitizeMentions.js (ZWS insertion) and safeSend.js wrappers.
All critical issues from previous reviews have been addressed:
- ✅ memory.js migrated to use safe wrappers (including
safeUpdatefor button interactions) - ✅ Email false positives prevented with negative lookbehind in regex
- ✅ Embeds/components only attached to last chunk when splitting messages
- ✅ AGENTS.md updated with new utility files
- ✅ Comprehensive test coverage with 40+ tests
Fixed Biome formatting issue in tests/utils/safeSend.test.js (commit 5bb5d52).
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/memory.js (1)
246-252:⚠️ Potential issue | 🟡 MinorMissing
.catch(() => {})on timeout-pathsafeEditReply.The catch block at line 248 handles button collector timeouts. While the interaction token is typically still valid at 30s, other commands in the codebase (e.g.,
history.jsline 75,kick.js,tempban.js) consistently guard error-pathsafeEditReplycalls with.catch(() => {}). This should follow the same pattern for resilience.The same issue applies to the admin clear timeout at line 424.
Proposed fix
} catch { // Timeout — no interaction received within 30 seconds - await safeEditReply(interaction, { + await safeEditReply(interaction, { content: '⏰ Confirmation timed out. No memories were deleted.', components: [], - }); + }).catch(() => {}); }And for the admin clear timeout (line 424):
} catch { // Timeout — no interaction received within 30 seconds - await safeEditReply(interaction, { + await safeEditReply(interaction, { content: '⏰ Confirmation timed out. No memories were deleted.', components: [], - }); + }).catch(() => {}); }
🤖 Fix all issues with AI agents
In `@src/utils/safeSend.js`:
- Around line 112-204: The current catch blocks in safeSend, safeReply,
safeFollowUp, safeEditReply, and safeUpdate only log err.message, losing
stack/context; update each catch to import and use the custom error helpers from
src/utils/errors.js (or wrap with those custom error classes when appropriate),
and call logError with the full error object or err.stack (e.g.,
logError('safeReply failed', { error: err, stack: err.stack })) before
re-throwing so the full stack and error context are preserved for debugging;
ensure you update all five functions (safeSend, safeReply, safeFollowUp,
safeEditReply, safeUpdate).
- Line 20: DISCORD_MAX_LENGTH is defined in multiple modules (safeSend.js and
splitMessage.js); remove the duplicate by extracting a single exported constant
and importing it where needed — create an exported DISCORD_MAX_LENGTH in a
shared module (e.g., a new or existing utils constants module), replace the
local const in safeSend.js and splitMessage.js with an import of that exported
symbol, and update any references inside the safeSend function and splitMessage
implementation to use the imported DISCORD_MAX_LENGTH to keep the value
canonical and in sync.
- Around line 71-77: The truncateForInteraction function currently slices
prepared.content to DISCORD_MAX_LENGTH with no indicator or log; change it to
detect truncation of content (prepared.content) and when truncating append an
ellipsis (e.g., "…") to the trimmed string and emit a warning log (use
console.warn or the module logger) including a reference to the message
id/summary (from prepared) and the original length so developers can trace data
loss; update the return to include the modified content while preserving other
prepared fields and ensure you account for the extra ellipsis when computing the
slice length relative to DISCORD_MAX_LENGTH.
In `@tests/utils/safeSend.test.js`:
- Line 17: The import statement that brings in safeEditReply, safeFollowUp,
safeReply, safeSend, and safeUpdate is on a single long line and fails the Biome
formatter; split the import across multiple lines so each imported symbol is on
its own line (or grouped sensibly) in the import from
'../../src/utils/safeSend.js' to satisfy formatting rules and restore CI.
- Around line 363-414: Replace the repeated dynamic imports of the logger mock
in the error-logging tests with a single top-level import: at the top of the
test file, import the mocked logger (e.g., const { error: mockLogError } = await
import('../../src/logger.js') replaced by a standard top-level import using the
mocked module via vi.mock), then remove each inline `const { error: mockLogError
} = await import('../../src/logger.js')` and reference `mockLogError` directly
in the tests for safeSend, safeReply, safeFollowUp, safeEditReply, and
safeUpdate; ensure vi.mock remains hoisted so the top-level import returns the
mocked logger.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/commands/history.jssrc/commands/memory.jssrc/index.jssrc/modules/events.jssrc/utils/safeSend.jssrc/utils/sanitizeMentions.jstests/commands/memory.test.jstests/commands/purge.test.jstests/modules/chimeIn.test.jstests/modules/events.test.jstests/utils/safeSend.test.jstests/utils/sanitizeMentions.test.js
🧰 Additional context used
📓 Path-based instructions (4)
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/modules/events.test.jstests/modules/chimeIn.test.jstests/utils/safeSend.test.jstests/utils/sanitizeMentions.test.jstests/commands/memory.test.jstests/commands/purge.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/modules/events.jssrc/utils/sanitizeMentions.jssrc/commands/memory.jssrc/utils/safeSend.jssrc/index.jssrc/commands/history.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/modules/*.js: Register event handlers in src/modules/events.js for all modules before processing in handler functions
Check config.moduleName.enabled before processing in module handler functions
Files:
src/modules/events.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/memory.jssrc/commands/history.js
🧠 Learnings (10)
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/modules/events.jssrc/utils/sanitizeMentions.jssrc/commands/memory.jssrc/utils/safeSend.jssrc/index.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/*.js : Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Applied to files:
src/commands/memory.jstests/commands/memory.test.jssrc/index.jssrc/commands/history.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Keep documentation up to date: README.md (setup, architecture, config, env vars), AGENTS.md (conventions, guides, pitfalls), CONTRIBUTING.md (workflow, commits), .env.example (env vars), config.json (documented in README)
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Update Key Files table in AGENTS.md when adding new commands or modules
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-11T17:18:14.598Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-11T17:18:14.598Z
Learning: See AGENTS.md for full project context, architecture, and coding guidelines
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to src/**/*.{ts,tsx} : Use `reportError(context, error)` from `src/lib/logger.ts` to report errors to Sentry with context metadata, falling back to console.error if Sentry is disabled
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/mod*.js : Moderation commands must follow the pattern: deferReply → validate → sendDmNotification → execute action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
src/index.jssrc/commands/history.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/*.js : DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Applied to files:
src/commands/history.js
🧬 Code graph analysis (9)
src/modules/events.js (3)
src/utils/safeSend.js (2)
safeReply(134-141)safeSend(112-119)src/utils/errors.js (2)
message(46-46)getUserFriendlyMessage(114-159)src/commands/ping.js (1)
response(9-12)
src/utils/sanitizeMentions.js (2)
tests/utils/safeSend.test.js (1)
ZWS(20-20)tests/utils/sanitizeMentions.test.js (1)
ZWS(4-4)
src/commands/memory.js (1)
src/utils/safeSend.js (3)
safeReply(134-141)safeEditReply(174-181)safeUpdate(197-204)
tests/utils/safeSend.test.js (3)
src/utils/sanitizeMentions.js (1)
ZWS(14-14)src/utils/safeSend.js (5)
safeSend(112-119)safeReply(134-141)safeFollowUp(154-161)safeEditReply(174-181)safeUpdate(197-204)src/utils/splitMessage.js (2)
needsSplitting(59-61)splitMessage(24-51)
tests/utils/sanitizeMentions.test.js (1)
src/utils/sanitizeMentions.js (3)
ZWS(14-14)sanitizeMentions(37-43)sanitizeMessageOptions(54-67)
tests/commands/memory.test.js (1)
src/utils/safeSend.js (3)
safeReply(134-141)safeEditReply(174-181)safeUpdate(197-204)
src/utils/safeSend.js (3)
tests/utils/safeSend.test.js (1)
SAFE_ALLOWED_MENTIONS(21-21)src/utils/sanitizeMentions.js (1)
sanitizeMessageOptions(54-67)src/utils/splitMessage.js (2)
needsSplitting(59-61)splitMessage(24-51)
src/index.js (1)
src/utils/safeSend.js (2)
safeReply(134-141)safeFollowUp(154-161)
src/commands/history.js (1)
src/utils/safeSend.js (1)
safeEditReply(174-181)
🪛 GitHub Actions: CI
tests/utils/safeSend.test.js
[error] 15-23: Formatter would have printed the following content: import { safeEditReply, safeFollowUp, safeReply, safeSend, safeUpdate } from '../../src/utils/safeSend.js'; (lint/formatting mismatch detected by biome check).
⏰ 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 (15)
tests/modules/chimeIn.test.js (1)
3-9: LGTM — safeSend mock correctly added with accurate comment.The passthrough mock and updated comment align with the pattern used across the test suite. The past stale-comment issue has been addressed.
tests/modules/events.test.js (1)
3-9: LGTM — safeSend mock and comment are correct.tests/commands/purge.test.js (1)
3-9: LGTM — safeSend mock correctly introduced.src/commands/history.js (1)
9-9: Clean migration to safeSend wrappers.All
interaction.editReplycalls have been replaced withsafeEditReply, and the error-path call on line 75 correctly includes.catch(() => {})to handle expired interactions.Also applies to: 34-34, 72-72, 75-75
src/modules/events.js (2)
9-12: Good documentation of the dual-type usage ofsafeReply.The comment clearly explains why
safeReplyworks with bothInteractionandMessageobjects, addressing the prior review concern about duck-typing semantics.
108-163: LGTM — all outgoing message paths migrated to safe wrappers.
safeReplyandsafeSendare correctly used for inline replies, thread sends, split chunks, and error fallback paths. The fallback at line 159 is properly guarded by the outer try/catch.src/index.js (2)
57-78: Excellent documentation of theallowedMentionsdesign decision.The comment clearly explains the intent, the security rationale, and how to opt-in to role mentions in the future (including the need to update
SAFE_ALLOWED_MENTIONSinsafeSend.js). This directly addresses the design concern raised in the PR review about undocumented global disabling of role mentions.
180-212: LGTM — interaction handler fully migrated to safe wrappers.Permission errors, missing commands, and command execution errors all route through
safeReply/safeFollowUpwith appropriate.catch(() => {})guards on error paths.tests/commands/memory.test.js (2)
193-199: Good: spy-based mocks properly verify safeSend wrapper usage.Unlike the passthrough mocks in other test files, these
vi.fn()spies allow the new "safeSend wrapper usage verification" suite (lines 897–1107) to assert thatsafeReply/safeEditReply/safeUpdatewere actually invoked. This pattern would catch the class of bug (missingsafeSendimport) that was flagged as critical in the PR review.Consider adopting this
vi.fn()spy pattern in other command test files for consistency.
897-1107: Solid wrapper verification suite — covers all major code paths.The 11 tests systematically verify that every outgoing message in
memory.jsroutes through the correct safe wrapper (safeReply,safeEditReply, orsafeUpdate), covering unavailable state, optout, view, forget flows (confirm/cancel/topic), admin flows, and permission denial.src/utils/sanitizeMentions.js (2)
1-43: Well-structured sanitization utility with solid regex design.The negative lookbehind
(?<!\w)correctly prevents false positives on email addresses likeuser@everyone.com, and the ZWS insertion ensures idempotency (double-sanitization produces the same result). The case-sensitivity rationale in the comment is accurate — Discord only parses exact lowercase@everyone/@here.
54-67:sanitizeMessageOptionshandles edge cases well — one minor gap with array inputs.The
'content' in optionscheck on Line 59 would also match arrays (sinceinworks on arrays checking indices). For example,sanitizeMessageOptions([1, 2])would enter the object branch and return a spread of an array plus acontentproperty. This is unlikely in practice since Discord.js doesn't accept arrays as message options, but it's worth noting.tests/utils/sanitizeMentions.test.js (1)
1-212: Comprehensive test suite covering all critical edge cases.Good coverage of email false positives (Lines 76–82, 151–165), idempotency (Lines 181–193), code blocks (Lines 168–179), and immutability (Lines 141–146). The edge cases flagged in the PR review are all addressed here.
src/utils/safeSend.js (2)
35-61:normalizeOptionsandprepareOptionspipeline is clean and well-documented.The security comment on Lines 45–49 explicitly documenting the intentional
allowedMentionsoverride is excellent — this was called out in the PR review as needing documentation and it's been addressed.
87-102:sendOrSplitcorrectly attaches embeds/components only to the last chunk.The chunking logic preserves message ordering (sequential
await) and only includes the full payload on the final chunk, which ensures embeds and components render after all text content is delivered.
- Export DISCORD_MAX_LENGTH from splitMessage.js; import in safeSend.js
instead of duplicating the constant
- Add truncation indicator ('… [truncated]') and warn log when
interaction content is truncated
- Include err.stack in all five wrapper catch-block error logs
- Replace dynamic logger imports in tests with top-level import
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/utils/safeSend.js`:
- Around line 52-59: prepareOptions currently only relies on
sanitizeMessageOptions which sanitizes options.content but leaves embed and
component string fields (e.g., Embed description, title, fields[].value,
footer.text and component labels/placeholders) untouched; update the sanitize
step so sanitizeMessageOptions (or a new helper it calls) recursively walks
embeds and components in the normalized options and sanitizes all string fields
that may contain user-supplied text before merging SAFE_ALLOWED_MENTIONS,
ensuring symbols: prepareOptions, sanitizeMessageOptions, and
SAFE_ALLOWED_MENTIONS are used to locate where to apply the recursive
sanitization; keep the existing allowedMentions override but add the
string-field sanitation for embed.description, embed.title, embed.footer.text,
embed.fields[].name/value and component labels/placeholders to close the
defense-in-depth gap.
In `@tests/utils/safeSend.test.js`:
- Around line 1-29: Add a module-level beforeEach that clears mocked function
state so tests don't leak calls between describes: at the top of the test file
add a beforeEach that calls vi.clearAllMocks() (or vi.resetAllMocks() if you
prefer full reset) so the mocks for mockLogError, mockLogWarn, needsSplitting
and splitMessage are cleared before each test; ensure this beforeEach is
declared at top-level (not inside a describe) so it runs for all tests.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/utils/safeSend.jssrc/utils/splitMessage.jstests/utils/safeSend.test.js
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/utils/safeSend.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/utils/splitMessage.jssrc/utils/safeSend.js
🧠 Learnings (10)
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Applied to files:
tests/utils/safeSend.test.jssrc/utils/splitMessage.jssrc/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use 2-space indentation (enforced by Biome)
Applied to files:
tests/utils/safeSend.test.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use single quotes for strings (enforced by Biome)
Applied to files:
tests/utils/safeSend.test.js
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to src/**/*.{ts,tsx} : Use `reportError(context, error)` from `src/lib/logger.ts` to report errors to Sentry with context metadata, falling back to console.error if Sentry is disabled
Applied to files:
tests/utils/safeSend.test.jssrc/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Applied to files:
tests/utils/safeSend.test.jssrc/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/{timeout,slowmode}.js : Discord timeouts max at 28 days and slowmode caps at 6 hours (21600s); enforce these limits in command logic
Applied to files:
src/utils/splitMessage.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Keep documentation up to date: README.md (setup, architecture, config, env vars), AGENTS.md (conventions, guides, pitfalls), CONTRIBUTING.md (workflow, commits), .env.example (env vars), config.json (documented in README)
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Update Key Files table in AGENTS.md when adding new commands or modules
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-11T17:18:14.598Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-11T17:18:14.598Z
Learning: See AGENTS.md for full project context, architecture, and coding guidelines
Applied to files:
src/utils/safeSend.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Applied to files:
src/utils/safeSend.js
🧬 Code graph analysis (1)
src/utils/safeSend.js (3)
tests/utils/safeSend.test.js (1)
SAFE_ALLOWED_MENTIONS(29-29)src/utils/sanitizeMentions.js (1)
sanitizeMessageOptions(54-67)src/utils/splitMessage.js (5)
DISCORD_MAX_LENGTH(9-9)DISCORD_MAX_LENGTH(9-9)needsSplitting(59-61)chunks(29-29)splitMessage(24-51)
⏰ 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 (6)
src/utils/splitMessage.js (1)
9-9: LGTM — exporting the constant for reuse is a clean DRY improvement.src/utils/safeSend.js (2)
91-106:sendOrSplitsanitizes content before splitting, but split chunks aren't re-sanitized.
prepareOptionsis called once beforesendOrSplit, so content is sanitized before splitting — this is correct. However,splitMessagecould theoretically split in the middle of an already-escaped@everyonesequence (between@and\u200B). In practice this is extremely unlikely since the ZWS is inserted right after@, but worth noting.No action needed — just flagging for awareness.
116-208: Well-structured wrappers with good error handling and documentation.All five wrappers follow a consistent pattern: prepare → truncate/split → send → catch/log/rethrow. The JSDoc is thorough, the security intent is clearly documented in
prepareOptions, and previous review feedback (stack traces, splitMessage integration, truncation indicator, DRY constant) has been addressed.tests/utils/safeSend.test.js (3)
257-302: Good coverage of split behavior including chunk payload structure.The tests correctly verify that non-last chunks contain only
content+allowedMentions, while the last chunk carries the full payload (embeds, components). This matches thesendOrSplitimplementation.
305-386: Truncation tests are thorough — nice coverage of the indicator, length, and warning log.Tests validate exact length (2000), suffix match, warning log arguments, no-truncation passthrough, and non-string content handling. Good.
388-442: Error logging tests cover all five wrappers consistently.Each wrapper's error path is tested for the log call (including stack trace) and rethrow. This addresses previous review feedback well.
…ions Extend sanitizeMessageOptions to sanitize @everyone/@here in embed fields (title, description, footer.text, author.name, fields[].name/value) and component fields (label, placeholder, options[].label/description). Preserves EmbedBuilder prototype chain so .toJSON() still works. Defense-in-depth: allowedMentions override already prevents Discord from parsing these mentions, but this ensures the raw text is also clean.
Add vi.clearAllMocks() in a top-level beforeEach to prevent mock state (mockLogError, mockLogWarn, splitMessage mocks) from leaking between describe blocks. Also add end-to-end test verifying embed sanitization flows through safeSend correctly.
…safeSend wrappers - Use main's checkAndRecoverMemory() function (replaces isMemoryAvailable) - Use main's batched deletion loop in handleForgetTopic for better reliability - Keep safeSend wrappers (safeReply, safeEditReply) for mention protection - Fix test file to use checkAndRecoverMemory instead of isMemoryAvailable
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM
All critical issues from previous reviews have been addressed:
- ✅ memory.js migrated to use safe wrappers (safeReply, safeEditReply, safeUpdate)
- ✅ All commands and modules migrated to safeSend wrappers
- ✅ Two-layer defense implemented (client-level allowedMentions + text sanitization)
- ✅ Email address false positives fixed with negative lookbehind regex
- ✅ Embeds and components sanitized (defense-in-depth)
- ✅ Comprehensive test coverage (40+ new tests)
- ✅ AGENTS.md updated with new utility files
- ✅ Biome formatting fixed (commit 6bb5eb7)
The implementation correctly prevents @everyone and @here mentions via both API-level protection and text sanitization.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/commands/memory.test.js`:
- Around line 977-1187: Tests currently verify wrapper usage for success flows
but miss asserting safeEditReply is used on timeout/catch paths; add tests that
simulate awaitMessageComponent rejecting (e.g.,
interaction._mockResponse.awaitMessageComponent.mockRejectedValue(new
Error(...))) when calling execute for the forget and admin clear flows so that
the timeout handlers in handleForgetAll and handleAdminClear exercise the catch
branches and you assert safeEditReply was called with the interaction and an
objectContaining content mentioning "timed out" and empty components; use
createMockInteraction({ subcommand: 'forget' }) and createMockInteraction({
subcommand: 'clear', subcommandGroup: 'admin', targetUser: {...},
hasManageGuild: true }) to locate the code paths and verify safeEditReply was
invoked.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/commands/memory.jssrc/index.jssrc/utils/sanitizeMentions.jstests/commands/memory.test.jstests/index.test.jstests/utils/safeSend.test.jstests/utils/sanitizeMentions.test.js
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Use ESM modules — import/export syntax; never use require()
Always use node: protocol for Node.js builtins (e.g., import { readFileSync } from 'node:fs')
Always use semicolons in code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Always use Winston for logging (import { info, warn, error } from '../logger.js'); never use console.log, console.warn, console.error, or any console.* method
Use custom error classes from src/utils/errors.js and log errors with context before re-throwing
Use getConfig() from src/modules/config.js to read config and setConfigValue(key, value) to update at runtime
Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Files:
src/commands/memory.jssrc/index.jssrc/utils/sanitizeMentions.js
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Call checkHierarchy(moderator, target) before executing any moderation action to prevent moderating users with equal or higher roles
DM moderation targets before executing kicks/bans since they cannot receive DMs once the action is executed
Files:
src/commands/memory.js
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Test coverage must maintain at least 80% threshold on statements, branches, functions, and lines
Files:
tests/commands/memory.test.jstests/utils/safeSend.test.jstests/index.test.jstests/utils/sanitizeMentions.test.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use splitMessage() utility for messages exceeding Discord's 2000-character limit
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use splitMessage() utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/commands/memory.jssrc/index.jssrc/utils/sanitizeMentions.jstests/utils/safeSend.test.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/*.js : Slash commands must export data (SlashCommandBuilder) and execute(interaction) function; optional adminOnly = true for mod-only commands
Applied to files:
src/commands/memory.jstests/commands/memory.test.jssrc/index.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/commands/mod*.js : Moderation commands must follow the pattern: deferReply → validate → sendDmNotification → execute action → createCase → sendModLogEmbed → checkEscalation
Applied to files:
src/index.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use 2-space indentation (enforced by Biome)
Applied to files:
tests/utils/safeSend.test.js
📚 Learning: 2026-02-15T03:14:33.915Z
Learnt from: CR
Repo: BillChirico/bills-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-15T03:14:33.915Z
Learning: Applies to src/**/*.js : Use single quotes for strings (enforced by Biome)
Applied to files:
tests/utils/safeSend.test.js
📚 Learning: 2026-02-04T02:20:09.131Z
Learnt from: CR
Repo: VolvoxCommunity/Volvox.Website PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-04T02:20:09.131Z
Learning: Applies to src/**/*.{ts,tsx} : Use `reportError(context, error)` from `src/lib/logger.ts` to report errors to Sentry with context metadata, falling back to console.error if Sentry is disabled
Applied to files:
tests/utils/safeSend.test.js
🧬 Code graph analysis (5)
src/commands/memory.js (1)
src/utils/safeSend.js (3)
safeReply(138-145)safeEditReply(178-185)safeUpdate(201-208)
tests/commands/memory.test.js (1)
src/utils/safeSend.js (3)
safeReply(138-145)safeEditReply(178-185)safeUpdate(201-208)
src/index.js (1)
src/utils/safeSend.js (2)
safeReply(138-145)safeFollowUp(158-165)
src/utils/sanitizeMentions.js (3)
tests/utils/sanitizeMentions.test.js (1)
ZWS(4-4)tests/utils/safeSend.test.js (1)
ZWS(28-28)src/commands/purge.js (1)
text(130-130)
tests/utils/safeSend.test.js (4)
tests/utils/sanitizeMentions.test.js (1)
ZWS(4-4)src/utils/sanitizeMentions.js (1)
ZWS(14-14)src/utils/safeSend.js (6)
SAFE_ALLOWED_MENTIONS(24-24)safeSend(116-123)safeReply(138-145)safeFollowUp(158-165)safeEditReply(178-185)safeUpdate(201-208)src/utils/splitMessage.js (2)
needsSplitting(59-61)splitMessage(24-51)
🔇 Additional comments (9)
src/commands/memory.js (1)
35-35: Migration to safe wrappers looks complete and correct.All 16 user-facing
reply,editReply, andupdatecallsites have been properly migrated tosafeReply,safeEditReply, andsafeUpdate. ThedeferReplycalls are correctly left unwrapped since they don't send user-visible content. This addresses the critical bug noted in the review comments regardingmemory.jsbypassing mention protections.src/index.js (2)
57-78: Well-documented client-level mention restriction.The extensive comment block clearly explains the rationale, scope (blocks
@everyone,@here, and role mentions), and how to opt in to role mentions in the future. This addresses the reviewer's design concern about undocumented global role-blocking.
180-212: Error-path replies correctly use safe wrappers.All error responses (permission denied, command not found, execution errors) now route through
safeReply/safeFollowUp.tests/commands/memory.test.js (1)
193-199: Passthrough mock is acceptable given the new explicit verification suite.The mock delegates to the underlying interaction methods, which means older tests (e.g.,
interaction.editReplyassertions) still pass regardless of whethersafeEditReplyis actually called. The newsafeSend wrapper usage verificationsuite at lines 977–1187 compensates by asserting directly on the wrapper spies. This is a reasonable two-layer test approach.tests/utils/safeSend.test.js (1)
1-461: Comprehensive test suite covering all safe wrappers.The tests thoroughly cover sanitization,
allowedMentionsoverride enforcement across all five wrappers, split/truncation behavior, and error logging. Previous review feedback (globalbeforeEach, top-level logger import, override tests for all wrappers) has been addressed.src/utils/sanitizeMentions.js (2)
14-25: Solid regex design with email-safe lookbehind.The negative lookbehind
(?<!\w)correctly prevents false positives on email addresses likeuser@everyone.comwhile still catching standalone@everyone/@here. The case-sensitivity decision is well-documented with the rationale that Discord only recognizes exact lowercase forms.
86-101: Good EmbedBuilder prototype preservation.Cloning via
Object.create(Object.getPrototypeOf(embed))+Object.assigncorrectly preserves the prototype chain so methods like.toJSON()continue working on the sanitized result.tests/index.test.js (1)
307-311: Good regression test for the client-level mention gate.Directly asserts that the
Clientconstructor receivesallowedMentions: { parse: ['users'] }, ensuring the Layer 1 protection from Issue#61doesn't regress.tests/utils/sanitizeMentions.test.js (1)
299-361: Thorough edge-case coverage addressing reviewer feedback.The edge-case tests for email false positives, code blocks, idempotency, and consecutive mentions directly address the gaps identified in the PR review comments. The idempotency tests at lines 331–343 are particularly valuable for verifying the ZWS insertion doesn't cause cascading mutations.
2 participants
Summary
Closes #61
Two-layer protection to ensure the bot never sends messages containing
@hereor@everyonepings.Layer 1: Client-level
allowedMentionsallowedMentions: { parse: ['users'] }to the Discord.jsClientconstructor insrc/index.js<@userId>) —@everyone,@here, and role mentions are ignored even if present in message textLayer 2: Text sanitization (defense-in-depth)
src/utils/sanitizeMentions.js—sanitizeMentions(text)escapes@everyoneand@hereby inserting a zero-width space after@, making them visually identical but unparseablesrc/utils/safeSend.js— Wrapper functions (safeSend,safeReply,safeFollowUp,safeEditReply) that:sanitizeMentionsallowedMentions: { parse: ['users'] }on every outgoing messageTests
tests/utils/sanitizeMentions.test.js— 27 tests covering escaping, preservation of normal content, edge casestests/utils/safeSend.test.js— 14 tests covering all wrapper functions with string/object inputstests/index.test.js— New test verifyingallowedMentionsis configured on the Client constructorFiles Changed
src/index.js— AddedallowedMentionsto Client constructorsrc/utils/sanitizeMentions.js— New sanitization utilitysrc/utils/safeSend.js— New safe message sending wrapperstests/index.test.js— Updated mock + new test for allowedMentionstests/utils/sanitizeMentions.test.js— New test filetests/utils/safeSend.test.js— New test file