fix: protect admins/mods/owner from moderation actions (#174)#181
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable moderation "protectRoles" feature: schema, validation, types, dashboard UI, runtime protection check ( Changes
Possibly related PRs
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Adds configurable “protected targets” logic to prevent moderation actions (and some automated triage logging) from applying to privileged members (owner/admin/mod/custom roles), with corresponding web dashboard controls and validation updates.
Changes:
- Introduces
isProtectedTarget()and applies it inexecuteModActionplus triage moderation logging. - Adds web config UI + TypeScript types for
moderation.protectRoles, and extends API config validation/schema. - Expands test coverage across moderation actions, triage logging, and the new helper.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/types/config.ts | Adds ModerationProtectRoles type and protectRoles field on moderation config. |
| web/src/components/dashboard/config-sections/ModerationSection.tsx | Adds “Protect Roles from Moderation” UI controls (Switches + roleIds input). |
| web/src/components/dashboard/config-editor.tsx | Adds protect-roles editing to the config editor and a helper updater callback. |
| src/utils/modAction.js | Blocks self-moderation and protected-target moderation; logs a warning on block. |
| src/modules/moderation.js | Exports isProtectedTarget() implementing owner/admin/mod/custom role checks. |
| src/modules/triage-respond.js | Skips triage moderation logging when a flagged target is protected. |
| src/commands/untimeout.js | Allows /untimeout to bypass protection checks (skipProtection: true). |
| src/api/utils/configValidation.js | Adds validation schema for moderation.protectRoles. |
| config.json | Adds default moderation.protectRoles configuration. |
| tests/utils/modAction.test.js | Adds tests for protected-target/self-moderation behavior and warning logs. |
| tests/modules/triage-respond.test.js | Adds tests for skipping moderation logs when target is protected. |
| tests/modules/moderation.test.js | Adds unit tests for isProtectedTarget(). |
| tests/commands/*.test.js | Updates mocks to include isProtectedTarget. |
| .gitattributes | Adds LF enforcement and binary patterns (including *.lock). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/commands/kick.test.js (1)
9-16: 🧹 Nitpick | 🔵 TrivialMock is correct; consider adding test coverage for protected targets.
The
isProtectedTargetmock returningfalseis appropriate for testing the standard kick flow. However, consider adding a test case whereisProtectedTargetreturnstrueto verify that the kick command properly rejects moderation of protected users.💡 Suggested test for protected target scenario
it('should reject when target is a protected role', async () => { const { isProtectedTarget } = await import('../../src/modules/moderation.js'); isProtectedTarget.mockReturnValueOnce(true); const { interaction, mockMember } = createInteraction(); await execute(interaction); expect(interaction.editReply).toHaveBeenCalledWith( expect.stringContaining('protected'), ); expect(mockMember.kick).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/commands/kick.test.js` around lines 9 - 16, Add a new test that asserts the kick command rejects protected targets by mocking isProtectedTarget to return true, then calling execute with a created interaction and verifying interaction.editReply was called with a message containing "protected" and that mockMember.kick was not called; specifically update the tests to import isProtectedTarget from '../../src/modules/moderation.js' and use isProtectedTarget.mockReturnValueOnce(true) before invoking execute (use createInteraction to get interaction and mockMember, and assert on interaction.editReply and mockMember.kick).src/api/utils/configValidation.js (1)
96-122:⚠️ Potential issue | 🔴 CriticalSchema path mismatch:
protectRolesis nested incorrectly.The
protectRolesschema is placed undermoderation.logging.protectRoles, but inconfig.json(lines 108-114),protectRolesis a direct child ofmoderation(sibling tologging), not nested insidelogging.This mismatch will cause validation to either fail or not validate the
protectRolesconfig at all.🐛 Proposed fix: Move protectRoles to be a sibling of logging
logging: { type: 'object', properties: { channels: { type: 'object', properties: { default: { type: 'string', nullable: true }, warns: { type: 'string', nullable: true }, bans: { type: 'string', nullable: true }, kicks: { type: 'string', nullable: true }, timeouts: { type: 'string', nullable: true }, purges: { type: 'string', nullable: true }, locks: { type: 'string', nullable: true }, }, }, }, }, - }, + protectRoles: { + type: 'object', + properties: { + enabled: { type: 'boolean' }, + roleIds: { type: 'array' }, + includeAdmins: { type: 'boolean' }, + includeModerators: { type: 'boolean' }, + includeServerOwner: { type: 'boolean' }, + }, + }, + }, - protectRoles: { - type: 'object', - properties: { - enabled: { type: 'boolean' }, - roleIds: { type: 'array' }, - includeAdmins: { type: 'boolean' }, - includeModerators: { type: 'boolean' }, - includeServerOwner: { type: 'boolean' }, - }, - },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/utils/configValidation.js` around lines 96 - 122, The schema places protectRoles inside the moderation.logging object, but the actual config has protectRoles as a sibling of logging; move the protectRoles definition out of the logging properties and add it as a top-level property alongside logging in the moderation schema so the protectRoles object (with enabled, roleIds, includeAdmins, includeModerators, includeServerOwner) is validated correctly; update any references to the protectRoles symbol in the schema so it is no longer nested under logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.json`:
- Around line 108-114: Update the README.md Moderation section to document the
new protectRoles configuration: add five table rows to the moderation config
table describing protectRoles.enabled (boolean — Enable role protection
features), protectRoles.roleIds (string[] — Role IDs to protect from
modifications), protectRoles.includeAdmins (boolean — Include admin roles in
protection), protectRoles.includeModerators (boolean — Include moderator roles
in protection), and protectRoles.includeServerOwner (boolean — Include server
owner in protection); place these rows together in the Moderation table so they
clearly belong to the protectRoles block.
In `@src/modules/triage-respond.js`:
- Around line 85-103: The variable guildConfig is a redundant alias for config;
remove the extra declaration and update the protection check to call
isProtectedTarget(member, guild, config) directly (in the loop that fetches
members and calls isProtectedTarget) so you don't keep an unused alias named
guildConfig in triage-respond.js.
In `@web/src/components/dashboard/config-editor.tsx`:
- Around line 1087-1095: The current input normalizes roleIds on every keystroke
(value={(draftConfig.moderation?.protectRoles?.roleIds ?? []).join(', ')} with
onChange splitting/trim/filter), which removes trailing commas and disrupts
typing; introduce a protectRoleIdsRaw string state (initialized/synced like
roleMenuRaw/dmStepsRaw) and bind the input value to protectRoleIdsRaw, update
protectRoleIdsRaw onChange, and only parse/sync into
draftConfig.moderation.protectRoles.roleIds by splitting/trim/filter on blur or
save (use updateProtectRolesField('roleIds', parsedArray) there) so typing is
not normalized mid-entry.
In `@web/src/components/dashboard/config-sections/ModerationSection.tsx`:
- Around line 160-168: The input currently re-parses and re-serializes roleIds
on every keystroke (value={(draftConfig.moderation?.protectRoles?.roleIds ??
[]).join(', ')} with onChange calling onProtectRolesChange), which strips commas
while typing; change to maintain a local raw string state (e.g., roleIdsRaw) in
ModerationSection, bind the input value to that raw state and update it on every
onChange, and only parse/split/trim/filter and call
onProtectRolesChange('roleIds', parsedArray) on onBlur or when saving; also
initialize/sync the raw state from draftConfig.moderation?.protectRoles?.roleIds
and handle external updates to keep it in sync.
---
Outside diff comments:
In `@src/api/utils/configValidation.js`:
- Around line 96-122: The schema places protectRoles inside the
moderation.logging object, but the actual config has protectRoles as a sibling
of logging; move the protectRoles definition out of the logging properties and
add it as a top-level property alongside logging in the moderation schema so the
protectRoles object (with enabled, roleIds, includeAdmins, includeModerators,
includeServerOwner) is validated correctly; update any references to the
protectRoles symbol in the schema so it is no longer nested under logging.
In `@tests/commands/kick.test.js`:
- Around line 9-16: Add a new test that asserts the kick command rejects
protected targets by mocking isProtectedTarget to return true, then calling
execute with a created interaction and verifying interaction.editReply was
called with a message containing "protected" and that mockMember.kick was not
called; specifically update the tests to import isProtectedTarget from
'../../src/modules/moderation.js' and use
isProtectedTarget.mockReturnValueOnce(true) before invoking execute (use
createInteraction to get interaction and mockMember, and assert on
interaction.editReply and mockMember.kick).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
.gitattributesconfig.jsonsrc/api/utils/configValidation.jssrc/commands/untimeout.jssrc/modules/moderation.jssrc/modules/triage-respond.jssrc/utils/modAction.jstests/commands/ban.test.jstests/commands/kick.test.jstests/commands/softban.test.jstests/commands/tempban.test.jstests/commands/timeout.test.jstests/commands/warn.test.jstests/modules/moderation.test.jstests/modules/triage-respond.test.jstests/utils/modAction.test.jsweb/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/ModerationSection.tsxweb/src/types/config.ts
📜 Review details
⏰ 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: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (8)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules — useimport/export, neverrequire()
Always usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons in JavaScript code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Files:
tests/modules/moderation.test.jstests/modules/triage-respond.test.jssrc/modules/triage-respond.jstests/commands/softban.test.jstests/commands/timeout.test.jstests/commands/ban.test.jstests/utils/modAction.test.jstests/commands/kick.test.jstests/commands/tempban.test.jssrc/utils/modAction.jssrc/modules/moderation.jssrc/api/utils/configValidation.jssrc/commands/untimeout.jstests/commands/warn.test.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
All new code must include tests. Test coverage must maintain an 80% threshold on statements, branches, functions, and lines. Run
pnpm testbefore every commit
Files:
tests/modules/moderation.test.jstests/modules/triage-respond.test.jstests/commands/softban.test.jstests/commands/timeout.test.jstests/commands/ban.test.jstests/utils/modAction.test.jstests/commands/kick.test.jstests/commands/tempban.test.jstests/commands/warn.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston logger —import { info, warn, error } from '../logger.js'. NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files — replace any existing console calls with Winston equivalents
Pass structured metadata to Winston logs:info('Message processed', { userId, channelId })
Files:
src/modules/triage-respond.jssrc/utils/modAction.jssrc/modules/moderation.jssrc/api/utils/configValidation.jssrc/commands/untimeout.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Per-request modules (AI, spam, moderation) should call
getConfig(guildId)on every invocation for automatic config changes. Stateful resources should useonConfigChangelisteners for reactive updates
Files:
src/modules/triage-respond.jssrc/modules/moderation.js
web/src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Component files should integrate with Zustand stores for state management (e.g., discord-entities store for caching Discord channels and roles per guild)
Files:
web/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/ModerationSection.tsx
web/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript with type safety. Share contracts between dashboard UI and API responses via
web/src/types/analytics.tsand similar type definition files
Files:
web/src/types/config.ts
config.json
📄 CodeRabbit inference engine (AGENTS.md)
When adding a new config section or key, document it in README.md's config reference section
Files:
config.json
src/commands/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/commands/*.js: Slash commands must exportdata(SlashCommandBuilder) andexecute(interaction)function. ExportadminOnly = truefor mod-only commands
Moderation commands must follow the shared pattern:deferReply({ ephemeral: true }), validate inputs,sendDmNotification(), execute Discord action,createCase(),sendModLogEmbed(),checkEscalation()
Files:
src/commands/untimeout.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Always call `checkHierarchy(moderator, target)` before executing moderation actions to prevent moderating users with equal or higher roles
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to src/commands/*.js : Moderation commands must follow the shared pattern: `deferReply({ ephemeral: true })`, validate inputs, `sendDmNotification()`, execute Discord action, `createCase()`, `sendModLogEmbed()`, `checkEscalation()`
Applied to files:
tests/modules/moderation.test.jstests/modules/triage-respond.test.jssrc/modules/triage-respond.jsweb/src/types/config.tstests/utils/modAction.test.jssrc/utils/modAction.jstests/commands/warn.test.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Always call `checkHierarchy(moderator, target)` before executing moderation actions to prevent moderating users with equal or higher roles
Applied to files:
tests/modules/moderation.test.jssrc/modules/triage-respond.jstests/utils/modAction.test.jssrc/utils/modAction.jssrc/modules/moderation.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Use `splitMessage()` utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/modules/triage-respond.js
🧬 Code graph analysis (7)
tests/modules/moderation.test.js (1)
src/modules/moderation.js (4)
guild(306-306)guild(371-371)config(377-377)isProtectedTarget(452-475)
tests/modules/triage-respond.test.js (2)
src/modules/moderation.js (1)
isProtectedTarget(452-475)src/modules/triage-respond.js (1)
sendModerationLog(74-141)
src/modules/triage-respond.js (1)
src/modules/moderation.js (5)
guild(306-306)guild(371-371)config(377-377)member(307-307)isProtectedTarget(452-475)
web/src/components/dashboard/config-editor.tsx (2)
web/src/lib/config-utils.ts (1)
GuildConfig(4-4)web/src/components/dashboard/toggle-switch.tsx (1)
ToggleSwitch(14-32)
tests/utils/modAction.test.js (3)
src/modules/moderation.js (2)
isProtectedTarget(452-475)createCase(88-156)src/utils/modAction.js (1)
executeModAction(47-179)src/logger.js (1)
warn(238-240)
src/utils/modAction.js (3)
src/utils/safeSend.js (1)
safeEditReply(178-185)src/modules/moderation.js (2)
isProtectedTarget(452-475)config(377-377)src/logger.js (1)
warn(238-240)
web/src/components/dashboard/config-sections/ModerationSection.tsx (4)
src/utils/cronParser.js (1)
field(34-34)web/src/components/ui/label.tsx (1)
Label(21-21)web/src/components/ui/switch.tsx (1)
Switch(35-35)web/src/components/ui/input.tsx (1)
Input(21-21)
🔇 Additional comments (17)
.gitattributes (1)
1-19: LGTM!Good addition for ensuring consistent LF line endings across platforms. The binary file exclusions are comprehensive and cover common formats.
src/commands/untimeout.js (1)
24-38: LGTM!Adding
skipProtection: truefor the untimeout command is correct — removing a punishment from a protected user should not be blocked. This aligns well with the PR objectives.tests/commands/timeout.test.js (1)
9-16: LGTM!The mock addition is consistent with other command test files. Same suggestion as for kick.test.js applies — consider adding a test case for when a protected target is encountered.
tests/modules/triage-respond.test.js (2)
40-46: LGTM!Good setup for testing the
isProtectedTargetintegration with triage moderation logging.
269-331: Excellent test coverage for protected role handling.These tests comprehensively verify both scenarios:
- Moderation log is skipped and warning is emitted when target has a protected role
- Moderation log is sent normally when target is not protected
The assertions correctly check
safeSendcall behavior and the warning message content.tests/commands/ban.test.js (1)
9-16: LGTM!The mock addition is consistent with other command test files. The same suggestion about adding a protected target test case applies here as well.
tests/commands/warn.test.js (1)
17-17: LGTM!The
isProtectedTargetmock returningfalseby default is correctly added, ensuring existing tests continue to work without protection blocking the moderation flow.web/src/types/config.ts (1)
119-127: LGTM!The
ModerationProtectRolesinterface is well-structured with appropriate types for each field. The optionalprotectRolesproperty inModerationConfigcorrectly reflects the opt-in nature of this feature.tests/commands/softban.test.js (1)
14-14: LGTM!The mock follows the same pattern as other command tests, ensuring consistent test behavior across the moderation command suite.
src/utils/modAction.js (1)
97-118: LGTM!The protection guard implementation is well-structured:
- Self-moderation check prevents mods from acting on themselves
- Protected target check integrates with
isProtectedTargetfunction- Warning log includes comprehensive audit metadata (action, targetId, targetTag, moderatorId, guildId)
- User-facing error messages are clear and consistent with the codebase style
skipProtectionoption correctly allows lift-punishment commands (like/untimeout) to bypass protectionBased on learnings: The protection check correctly runs before
checkHierarchy(moderator, target), maintaining the established pattern while adding the new security layer.src/modules/moderation.js (1)
452-475: LGTM!The
isProtectedTargetfunction is well-implemented:
- Early return when protection is disabled avoids unnecessary computation
- Server owner check is correctly prioritized
- Role aggregation handles missing config values gracefully with optional chaining
- The
.filter(Boolean)correctly removes falsy values from the protected role IDs array- Membership check via
target.roles.cache.keys()correctly accesses Discord.js GuildMember role collectiontests/commands/tempban.test.js (1)
20-20: LGTM!Consistent mock addition following the established pattern across command tests.
tests/modules/moderation.test.js (1)
576-741: Comprehensive test coverage forisProtectedTarget.The test suite thoroughly covers:
- Feature toggle states (enabled/disabled/absent)
- All protection flags (includeServerOwner, includeAdmins, includeModerators)
- Custom role IDs
- Negative cases ensuring non-protected users aren't incorrectly flagged
- Edge case where no protected role IDs resolve
The
makeTargethelper correctly simulatesGuildMember.roles.cache.keys()behavior.web/src/components/dashboard/config-sections/ModerationSection.tsx (1)
16-17: Prop wiring foronProtectRolesChangelooks correct.The new callback is consistently threaded through props and component signature.
Also applies to: 26-27
web/src/components/dashboard/config-editor.tsx (1)
576-590:updateProtectRolesFieldfollows the existing nested update pattern cleanly.This is consistent with the other section updaters and keeps writes scoped to
moderation.protectRoles.tests/utils/modAction.test.js (2)
3-10: Mock/import updates for protection flow are solid.
isProtectedTargetandwarnare correctly wired for the new execution paths.Also applies to: 32-43
266-331: Good coverage for protection guard paths.These cases meaningfully cover protected-target block, warning audit, skipProtection bypass, and self-target behavior.
web/src/components/dashboard/config-sections/ModerationSection.tsx
Outdated
Show resolved
Hide resolved
|
| Filename | Overview |
|---|---|
| src/modules/moderation.js | Added isProtectedTarget() function with proper defaults and deep-merge logic for role protection configuration |
| src/utils/modAction.js | Integrated protection check into moderation flow, correctly ordered after self-moderation check |
| src/modules/triage-respond.js | Added optimization to skip member fetch loop when protection is explicitly disabled, preventing unnecessary API calls |
| web/src/components/dashboard/config-sections/ModerationSection.tsx | Added UI section for role protection with switches and role ID input, properly synced with draft config |
Last reviewed commit: 7fc24c3
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/components/dashboard/config-editor.tsx (1)
1089-1100:⚠️ Potential issue | 🟡 Minor
protectRoleIdsRawcan be missed by Ctrl/Cmd+S saves while the input is focused.Line 1092 only commits parsed IDs on blur. In this component, save can be triggered from keyboard without blurring first, so stale
draftConfig.moderation.protectRoles.roleIdsmay be persisted.💡 Proposed fix
<input type="text" value={protectRoleIdsRaw} - onChange={(e) => setProtectRoleIdsRaw(e.target.value)} - onBlur={() => - updateProtectRolesField( - 'roleIds', - protectRoleIdsRaw - .split(',') - .map((s) => s.trim()) - .filter(Boolean), - ) - } + onChange={(e) => { + const raw = e.target.value; + setProtectRoleIdsRaw(raw); + updateProtectRolesField( + 'roleIds', + raw + .split(',') + .map((s) => s.trim()) + .filter(Boolean), + ); + }} + onBlur={(e) => + setProtectRoleIdsRaw( + e.target.value + .split(',') + .map((s) => s.trim()) + .filter(Boolean) + .join(', '), + ) + } disabled={saving} className={inputClasses} placeholder="Role ID 1, Role ID 2" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboard/config-editor.tsx` around lines 1089 - 1100, protectRoleIdsRaw is only parsed and saved in the onBlur handler, so a Ctrl/Cmd+S save while the input is focused can persist stale draftConfig.moderation.protectRoles.roleIds; update the component to also commit the parsed IDs when a save keystroke or form submit occurs (or when Ctrl/Cmd+S is detected). Specifically, keep the existing onBlur parsing logic but also add a handler that listens for the save action (e.g., onKeyDown for the input or the global save handler used by the form) and calls the same parsing code and updateProtectRolesField('roleIds', parsedArray) (reusing the split/trim/filter logic), or programmatically blur the input by calling setProtectRoleIdsRaw and then triggering the onBlur behavior so values are synchronized before saving.
🤖 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/utils/modAction.js`:
- Around line 97-118: The self-moderation check is incorrectly guarded by
skipProtection, allowing actions on oneself when skipProtection is true; update
the logic in the mod action flow so the "Prevent mods from moderating
themselves" check (compare target.id to interaction.user.id and call
safeEditReply with 'You cannot moderate yourself.') runs unconditionally (or
before any skipProtection gating), while leaving the protected-role check
(isProtectedTarget(...)) still behind skipProtection as intended.
---
Duplicate comments:
In `@web/src/components/dashboard/config-editor.tsx`:
- Around line 1089-1100: protectRoleIdsRaw is only parsed and saved in the
onBlur handler, so a Ctrl/Cmd+S save while the input is focused can persist
stale draftConfig.moderation.protectRoles.roleIds; update the component to also
commit the parsed IDs when a save keystroke or form submit occurs (or when
Ctrl/Cmd+S is detected). Specifically, keep the existing onBlur parsing logic
but also add a handler that listens for the save action (e.g., onKeyDown for the
input or the global save handler used by the form) and calls the same parsing
code and updateProtectRolesField('roleIds', parsedArray) (reusing the
split/trim/filter logic), or programmatically blur the input by calling
setProtectRoleIdsRaw and then triggering the onBlur behavior so values are
synchronized before saving.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
.gitattributesREADME.mdsrc/modules/moderation.jssrc/modules/triage-respond.jssrc/utils/modAction.jstests/commands/kick.test.jsweb/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/ModerationSection.tsx
📜 Review details
⏰ 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). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (6)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules — useimport/export, neverrequire()
Always usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons in JavaScript code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Files:
src/modules/moderation.jstests/commands/kick.test.jssrc/utils/modAction.jssrc/modules/triage-respond.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston logger —import { info, warn, error } from '../logger.js'. NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files — replace any existing console calls with Winston equivalents
Pass structured metadata to Winston logs:info('Message processed', { userId, channelId })
Files:
src/modules/moderation.jssrc/utils/modAction.jssrc/modules/triage-respond.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Per-request modules (AI, spam, moderation) should call
getConfig(guildId)on every invocation for automatic config changes. Stateful resources should useonConfigChangelisteners for reactive updates
Files:
src/modules/moderation.jssrc/modules/triage-respond.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
All new code must include tests. Test coverage must maintain an 80% threshold on statements, branches, functions, and lines. Run
pnpm testbefore every commit
Files:
tests/commands/kick.test.js
web/src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Component files should integrate with Zustand stores for state management (e.g., discord-entities store for caching Discord channels and roles per guild)
Files:
web/src/components/dashboard/config-editor.tsxweb/src/components/dashboard/config-sections/ModerationSection.tsx
README.md
📄 CodeRabbit inference engine (AGENTS.md)
Keep README.md updated with setup instructions, architecture overview, config reference, and environment variables. Update when adding new commands, modules, or changing architecture
Files:
README.md
🧠 Learnings (5)
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Always call `checkHierarchy(moderator, target)` before executing moderation actions to prevent moderating users with equal or higher roles
Applied to files:
src/modules/moderation.jstests/commands/kick.test.jssrc/utils/modAction.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to src/commands/*.js : Moderation commands must follow the shared pattern: `deferReply({ ephemeral: true })`, validate inputs, `sendDmNotification()`, execute Discord action, `createCase()`, `sendModLogEmbed()`, `checkEscalation()`
Applied to files:
tests/commands/kick.test.jssrc/utils/modAction.jssrc/modules/triage-respond.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Moderation commands must DM the target before executing kicks/bans, because once a user is kicked/banned they cannot receive DMs from the bot
Applied to files:
tests/commands/kick.test.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to src/commands/*.js : Slash commands must export `data` (SlashCommandBuilder) and `execute(interaction)` function. Export `adminOnly = true` for mod-only commands
Applied to files:
tests/commands/kick.test.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Use `splitMessage()` utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/modules/triage-respond.js
🧬 Code graph analysis (5)
src/modules/moderation.js (3)
src/utils/modAction.js (1)
config(64-64)src/commands/config.js (3)
config(126-126)config(162-162)config(200-200)src/commands/modlog.js (2)
config(35-35)config(187-187)
tests/commands/kick.test.js (1)
src/modules/moderation.js (1)
isProtectedTarget(452-490)
web/src/components/dashboard/config-editor.tsx (1)
web/src/lib/config-utils.ts (1)
GuildConfig(4-4)
src/utils/modAction.js (3)
src/utils/safeSend.js (1)
safeEditReply(178-185)src/modules/moderation.js (2)
isProtectedTarget(452-490)config(377-377)src/logger.js (1)
warn(238-240)
src/modules/triage-respond.js (1)
src/modules/moderation.js (5)
guild(306-306)guild(371-371)member(307-307)isProtectedTarget(452-490)config(377-377)
🔇 Additional comments (7)
.gitattributes (1)
1-18: LGTM! Well-formed.gitattributesconfiguration.The file correctly enforces LF line endings for text files and marks common binary formats to prevent corruption. This is a standard best practice for cross-platform repositories.
Note for the team: After merging, developers may see line-ending changes in their working tree when they pull/checkout. This is expected behavior as Git normalizes existing files to LF. Running
git add --renormalize .can help apply the new rules cleanly.README.md (1)
203-207: Protect-roles config docs are clear and complete.These additions make the new
moderation.protectRolessettings discoverable and align with the implemented behavior.src/modules/triage-respond.js (1)
85-105: Protected-target guard is correctly integrated into triage moderation logging.The early return on protected members plus structured warning metadata is the right behavior for this flow.
tests/commands/kick.test.js (1)
84-92: Good coverage addition for protected-target behavior.This test closes an important regression path by asserting both user feedback and that the kick action is not executed.
web/src/components/dashboard/config-sections/ModerationSection.tsx (1)
30-37: Role ID input handling is much safer now.Using
roleIdsRawplus blur-time parsing avoids destructive normalization while typing and is a solid UX fix.Also applies to: 167-183
web/src/components/dashboard/config-editor.tsx (1)
579-593:updateProtectRolesFieldfollows the existing nested-update pattern well.The helper is clean and keeps protect-roles state mutations localized and consistent.
src/modules/moderation.js (1)
466-467: No changes needed—nested defaults forprotectRolesare already properly materialized.
getConfig(guildId)deep-merges guild overrides onto a full clone of global defaults (line 310 in src/modules/config.js). ThedeepMergefunction preserves keys from the global config that are not overridden by the guild config, so missing properties likeenabledretain their global default value oftrue. Additionally, the fallback at line 466 (config.moderation?.protectRoles ?? defaultProtectRoles) provides redundant protection. Per-field defaults are unnecessary.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* fix: protect admins/mods/owner from moderation actions (#174) - Add moderation.protectRoles config (enabled, includeAdmins, includeModerators, includeServerOwner, roleIds) - Export isProtectedTarget() from moderation.js — checks server owner, adminRoleId, moderatorRoleId, and custom roleIds - Guard executeModAction with protection check before hierarchy check; add skipProtection flag for lift-punishment commands - Add skipProtection: true to untimeout (removing punishment) - Extend configValidation.js schema with protectRoles shape - Add ModerationProtectRoles TypeScript interface to web types - Add isProtectedTarget unit tests (9 cases) in moderation.test.js - Add executeModAction protection path tests in modAction.test.js - Update moderation.js mock in all affected command tests * chore: add .gitattributes to enforce LF line endings on Windows Biome enforces LF line endings. Without this file, Git's core.autocrlf=true on Windows converts LF->CRLF on checkout causing 373+ format errors locally. * text=auto eol=lf overrides autocrlf and ensures all text files are checked out with LF regardless of OS or Git config. * Revert "chore: add .gitattributes to enforce LF line endings on Windows" This reverts commit 2b6262d. * chore: add .gitattributes to enforce LF line endings Fixes 373 Biome format errors on Windows caused by core.autocrlf=true converting LF->CRLF on checkout. * text=auto eol=lf ensures all text files are checked out with LF regardless of OS or local Git config. * fix: complete protect-roles implementation for #174 - Self-moderation: prevent mods from moderating themselves (both self-mod + protectRoles checks now share skipProtection flag so untimeout can still target self) - Audit log: warn() when protection blocks an action with action/targetId/moderatorId/guildId context - Triage: sendModerationLog skips embed when any flagged user is a protected role (admin/mod/owner); logs warn with userId/channelId - Dashboard UI: Protect Roles fieldset added to Moderation Card in config-editor.tsx (toggle + 3 checkboxes + role IDs text input) - ModerationSection component updated with onProtectRolesChange prop and matching UI - Tests: +5 tests (self-mod, warn log, skipProtection bypass, triage skip-protected, triage send-when-not-protected) * fix: address code review feedback on protect-roles (#181) * fix: separate self-mod from skipProtection, simplify error message * fix: deep-merge protectRoles defaults, remove dead code from ModerationSection --------- Co-authored-by: TheCoderBuilder-dev <brianinesh@gmail.com> Co-authored-by: Pip Build <pip@volvox.gg>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/components/dashboard/config-editor.tsx (1)
1098-1107:⚠️ Potential issue | 🟠 Major
Ctrl/Cmd+Scan save staleprotectRoles.roleIdswhen this field is focused.
roleIdsare only synced on blur. Keyboard save can run before blur, so the latest typed IDs may not persist.Proposed fix
<input type="text" value={protectRoleIdsRaw} - onChange={(e) => setProtectRoleIdsRaw(e.target.value)} - onBlur={() => - updateProtectRolesField( - 'roleIds', - protectRoleIdsRaw - .split(',') - .map((s) => s.trim()) - .filter(Boolean), - ) - } + onChange={(e) => { + const raw = e.target.value; + setProtectRoleIdsRaw(raw); + updateProtectRolesField( + 'roleIds', + raw + .split(',') + .map((s) => s.trim()) + .filter(Boolean), + ); + }} + onBlur={() => { + const parsed = protectRoleIdsRaw + .split(',') + .map((s) => s.trim()) + .filter(Boolean); + setProtectRoleIdsRaw(parsed.join(', ')); + }} disabled={saving} className={inputClasses} placeholder="Role ID 1, Role ID 2" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/dashboard/config-editor.tsx` around lines 1098 - 1107, The protect role IDs input only updates protectRoles.roleIds on blur, so using Ctrl/Cmd+S while the field is focused can save stale data; update the raw value before save by invoking the same sync logic (the split/trim/filter of protectRoleIdsRaw) when handling the global save shortcut or when detecting Ctrl/Cmd+S in the input. Locate the input handlers (setProtectRoleIdsRaw, updateProtectRolesField, protectRoleIdsRaw) and call updateProtectRolesField('roleIds', protectRoleIdsRaw.split(',').map(s => s.trim()).filter(Boolean)) from the save shortcut handler (or add an onKeyDown for Ctrl/Cmd+S that calls that sync) so the latest typed IDs are persisted before the save proceeds.
🤖 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/moderation.js`:
- Around line 452-469: The isProtectedTarget function currently uses the
caller-supplied config which can become stale; update isProtectedTarget to call
getConfig(guild.id) at the start of the function to fetch the latest
configuration per invocation, then use that returned config (not the passed-in
config) when building defaultProtectRoles and computing protectRoles (deep-merge
with moderation?.protectRoles). Ensure you reference getConfig(guild.id) and
replace usages of the incoming config variable with the fresh config inside
isProtectedTarget so protection checks reflect live config changes.
---
Duplicate comments:
In `@web/src/components/dashboard/config-editor.tsx`:
- Around line 1098-1107: The protect role IDs input only updates
protectRoles.roleIds on blur, so using Ctrl/Cmd+S while the field is focused can
save stale data; update the raw value before save by invoking the same sync
logic (the split/trim/filter of protectRoleIdsRaw) when handling the global save
shortcut or when detecting Ctrl/Cmd+S in the input. Locate the input handlers
(setProtectRoleIdsRaw, updateProtectRolesField, protectRoleIdsRaw) and call
updateProtectRolesField('roleIds', protectRoleIdsRaw.split(',').map(s =>
s.trim()).filter(Boolean)) from the save shortcut handler (or add an onKeyDown
for Ctrl/Cmd+S that calls that sync) so the latest typed IDs are persisted
before the save proceeds.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/modules/moderation.jsweb/src/components/dashboard/config-editor.tsx
📜 Review details
⏰ 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: Agent
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
web/src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Component files should integrate with Zustand stores for state management (e.g., discord-entities store for caching Discord channels and roles per guild)
Files:
web/src/components/dashboard/config-editor.tsx
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules — useimport/export, neverrequire()
Always usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons in JavaScript code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Files:
src/modules/moderation.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston logger —import { info, warn, error } from '../logger.js'. NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files — replace any existing console calls with Winston equivalents
Pass structured metadata to Winston logs:info('Message processed', { userId, channelId })
Files:
src/modules/moderation.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Per-request modules (AI, spam, moderation) should call
getConfig(guildId)on every invocation for automatic config changes. Stateful resources should useonConfigChangelisteners for reactive updates
Files:
src/modules/moderation.js
🧠 Learnings (1)
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Always call `checkHierarchy(moderator, target)` before executing moderation actions to prevent moderating users with equal or higher roles
Applied to files:
src/modules/moderation.js
🧬 Code graph analysis (1)
web/src/components/dashboard/config-editor.tsx (2)
web/src/lib/config-utils.ts (1)
GuildConfig(4-4)web/src/components/dashboard/toggle-switch.tsx (1)
ToggleSwitch(14-32)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip moderation log if any flagged user is a protected role (admin/mod/owner) | ||
| const guild = logChannel.guild; | ||
| if (guild && targets.length > 0) { | ||
| const seenUserIds = new Set(); | ||
| for (const t of targets) { | ||
| if (seenUserIds.has(t.userId)) continue; | ||
| seenUserIds.add(t.userId); | ||
| try { | ||
| const member = await guild.members.fetch(t.userId); | ||
| if (isProtectedTarget(member, guild, config)) { | ||
| warn('Triage skipped moderation log: target is a protected role', { | ||
| userId: t.userId, | ||
| channelId, | ||
| }); | ||
| return; | ||
| } | ||
| } catch { | ||
| // Member not in guild or fetch failed — proceed with logging | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
sendModerationLog always does guild.members.fetch() for each flagged user before calling isProtectedTarget(). When moderation.protectRoles.enabled is explicitly set to false, this extra fetch work is unnecessary. Add an early guard to skip the protected-role fetch loop entirely when protection is disabled (e.g., check config.moderation?.protectRoles?.enabled === false before iterating).
| protectRoles: { | ||
| type: 'object', | ||
| properties: { | ||
| enabled: { type: 'boolean' }, | ||
| roleIds: { type: 'array' }, | ||
| includeAdmins: { type: 'boolean' }, | ||
| includeModerators: { type: 'boolean' }, | ||
| includeServerOwner: { type: 'boolean' }, | ||
| }, |
There was a problem hiding this comment.
moderation.protectRoles.roleIds is validated only as type: 'array' here, so the API will accept arrays containing non-strings (numbers/objects), and validateValue() won’t catch it because items isn’t specified. Define items: { type: 'string' } for roleIds to ensure only role ID strings are accepted.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/moderation.js`:
- Around line 447-450: The JSDoc for isProtectedTarget documents a removed
parameter named config; update the comment block for the function
isProtectedTarget in src/modules/moderation.js by removing the stale "@param
{Object} config - Bot configuration" line (or replace it with any current params
if needed) so the JSDoc matches the actual function signature and return
description; ensure the remaining `@param` entries (target, guild) and the
`@returns` line remain accurate.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/modules/moderation.jssrc/modules/triage-respond.jssrc/utils/modAction.jstests/modules/moderation.test.js
📜 Review details
⏰ 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). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules — useimport/export, neverrequire()
Always usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons in JavaScript code
Use single quotes for strings (enforced by Biome)
Use 2-space indentation (enforced by Biome)
Files:
src/utils/modAction.jssrc/modules/moderation.jssrc/modules/triage-respond.jstests/modules/moderation.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston logger —import { info, warn, error } from '../logger.js'. NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files — replace any existing console calls with Winston equivalents
Pass structured metadata to Winston logs:info('Message processed', { userId, channelId })
Files:
src/utils/modAction.jssrc/modules/moderation.jssrc/modules/triage-respond.js
src/modules/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Per-request modules (AI, spam, moderation) should call
getConfig(guildId)on every invocation for automatic config changes. Stateful resources should useonConfigChangelisteners for reactive updates
Files:
src/modules/moderation.jssrc/modules/triage-respond.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
All new code must include tests. Test coverage must maintain an 80% threshold on statements, branches, functions, and lines. Run
pnpm testbefore every commit
Files:
tests/modules/moderation.test.js
🧠 Learnings (4)
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to src/commands/*.js : Moderation commands must follow the shared pattern: `deferReply({ ephemeral: true })`, validate inputs, `sendDmNotification()`, execute Discord action, `createCase()`, `sendModLogEmbed()`, `checkEscalation()`
Applied to files:
src/utils/modAction.jssrc/modules/triage-respond.jstests/modules/moderation.test.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Always call `checkHierarchy(moderator, target)` before executing moderation actions to prevent moderating users with equal or higher roles
Applied to files:
src/utils/modAction.jssrc/modules/moderation.jssrc/modules/triage-respond.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Applies to src/modules/*.js : Per-request modules (AI, spam, moderation) should call `getConfig(guildId)` on every invocation for automatic config changes. Stateful resources should use `onConfigChange` listeners for reactive updates
Applied to files:
src/modules/moderation.jssrc/modules/triage-respond.jstests/modules/moderation.test.js
📚 Learning: 2026-03-01T06:03:34.399Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-01T06:03:34.399Z
Learning: Use `splitMessage()` utility for messages exceeding Discord's 2000-character limit
Applied to files:
src/modules/triage-respond.js
🧬 Code graph analysis (4)
src/utils/modAction.js (3)
src/utils/safeSend.js (1)
safeEditReply(178-185)src/modules/moderation.js (1)
isProtectedTarget(452-494)src/logger.js (1)
warn(238-240)
src/modules/moderation.js (1)
src/modules/config.js (1)
getConfig(282-313)
src/modules/triage-respond.js (1)
src/modules/moderation.js (4)
guild(306-306)guild(371-371)member(307-307)isProtectedTarget(452-494)
tests/modules/moderation.test.js (2)
src/modules/moderation.js (3)
guild(306-306)guild(371-371)isProtectedTarget(452-494)src/modules/config.js (1)
getConfig(282-313)
🔇 Additional comments (3)
src/modules/triage-respond.js (1)
85-105: Protected-target triage guard is correctly applied.Line 94–Line 99 correctly short-circuits moderation-log emission when a flagged target is protected, and Line 95 logs an auditable warning with metadata.
tests/modules/moderation.test.js (1)
577-742: Coverage forisProtectedTargetscenarios is strong.The added cases exercise owner/admin/mod/custom-role and disabled/no-match paths, which is solid coverage for the new protection logic.
src/utils/modAction.js (1)
97-114: Self-moderation and protected-role guards are now correctly separated.Line 98–Line 100 blocks self-moderation unconditionally, while Line 103–Line 114 keeps protected-role blocking behind
skipProtectionas intended.
| * @param {import('discord.js').GuildMember} target - Target member to check | ||
| * @param {import('discord.js').Guild} guild - Discord guild | ||
| * @param {Object} config - Bot configuration | ||
| * @returns {boolean} True if the target should not be moderated |
There was a problem hiding this comment.
Remove stale JSDoc param from isProtectedTarget.
Line 449 documents a config parameter that no longer exists in the function signature, which is misleading.
♻️ Suggested doc fix
/**
* Check if a target member is protected from moderation actions.
* Protected members include the server owner, admins, moderators, and any custom role IDs
* configured under `moderation.protectRoles`.
* `@param` {import('discord.js').GuildMember} target - Target member to check
* `@param` {import('discord.js').Guild} guild - Discord guild
- * `@param` {Object} config - Bot configuration
* `@returns` {boolean} True if the target should not be moderated
*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/moderation.js` around lines 447 - 450, The JSDoc for
isProtectedTarget documents a removed parameter named config; update the comment
block for the function isProtectedTarget in src/modules/moderation.js by
removing the stale "@param {Object} config - Bot configuration" line (or replace
it with any current params if needed) so the JSDoc matches the actual function
signature and return description; ensure the remaining `@param` entries (target,
guild) and the `@returns` line remain accurate.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/modules/moderation.js
Outdated
| * configured under `moderation.protectRoles`. | ||
| * @param {import('discord.js').GuildMember} target - Target member to check | ||
| * @param {import('discord.js').Guild} guild - Discord guild | ||
| * @param {Object} config - Bot configuration |
There was a problem hiding this comment.
Remove this @param line - the function doesn't take a config parameter (it fetches config internally on line 454)
| * @param {Object} config - Bot configuration | |
| * @returns {boolean} True if the target should not be moderated |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/moderation.js
Line: 449
Comment:
Remove this `@param` line - the function doesn't take a `config` parameter (it fetches config internally on line 454)
```suggestion
* @returns {boolean} True if the target should not be moderated
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function isProtectedTarget(target, guild) { | ||
| // Fetch config per-invocation so live config edits take effect immediately. | ||
| const config = getConfig(guild.id); | ||
| /** | ||
| * When the protectRoles block is missing from persisted configuration, | ||
| * fall back to the intended defaults: protection enabled, include owner, | ||
| * admins, and moderators (matches config.json defaults and web UI defaults). | ||
| */ | ||
| const defaultProtectRoles = { | ||
| enabled: true, | ||
| includeAdmins: true, | ||
| includeModerators: true, | ||
| includeServerOwner: true, | ||
| roleIds: [], | ||
| }; | ||
|
|
||
| // Deep-merge defaults so a partial persisted object (e.g. only roleIds set) | ||
| // never leaves enabled/include* as undefined/falsy. | ||
| const protectRoles = { ...defaultProtectRoles, ...config.moderation?.protectRoles }; | ||
| if (!protectRoles.enabled) { | ||
| return false; | ||
| } | ||
|
|
||
| // Server owner is always protected when enabled | ||
| if (protectRoles.includeServerOwner && target.id === guild.ownerId) { | ||
| return true; | ||
| } | ||
|
|
||
| const protectedRoleIds = [ | ||
| ...(protectRoles.includeAdmins && config.permissions?.adminRoleId | ||
| ? [config.permissions.adminRoleId] | ||
| : []), | ||
| ...(protectRoles.includeModerators && config.permissions?.moderatorRoleId | ||
| ? [config.permissions.moderatorRoleId] | ||
| : []), | ||
| ...(Array.isArray(protectRoles.roleIds) ? protectRoles.roleIds : []), | ||
| ].filter(Boolean); | ||
|
|
||
| if (protectedRoleIds.length === 0) return false; | ||
|
|
||
| const memberRoleIds = [...target.roles.cache.keys()]; | ||
| return protectedRoleIds.some((roleId) => memberRoleIds.includes(roleId)); | ||
| } |
There was a problem hiding this comment.
isProtectedTarget() fetches a fresh config via getConfig(guild.id) on every call, but callers like executeModAction() and sendModerationLog() already have a guild config available. This adds extra deep-clone work in hot paths (and in triage it can happen inside a loop). Consider refactoring isProtectedTarget to accept an optional pre-fetched config (or a protectRoles+permissions snapshot) so callers can reuse the config they already loaded and avoid redundant getConfig() calls.
| // Skip moderation log if any flagged user is a protected role (admin/mod/owner) | ||
| const guild = logChannel.guild; | ||
| if (guild && targets.length > 0) { | ||
| // Skip the expensive member-fetch loop when protection is explicitly disabled. | ||
| if (config.moderation?.protectRoles?.enabled !== false) { | ||
| const seenUserIds = new Set(); | ||
| for (const t of targets) { | ||
| if (seenUserIds.has(t.userId)) continue; | ||
| seenUserIds.add(t.userId); | ||
| try { | ||
| const member = await guild.members.fetch(t.userId); | ||
| if (isProtectedTarget(member, guild)) { | ||
| warn('Triage skipped moderation log: target is a protected role', { | ||
| userId: t.userId, | ||
| channelId, | ||
| }); | ||
| return; | ||
| } | ||
| } catch { | ||
| // Member not in guild or fetch failed — proceed with logging | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Inside this protected-role loop, isProtectedTarget(member, guild) currently re-fetches config internally (via getConfig) for each member checked. Since sendModerationLog already receives config and may iterate multiple user IDs, this can cause repeated deep-clone/config work. Consider passing the existing config into isProtectedTarget (or adding a variant that accepts config) so the loop stays O(n) without extra config loads.
brian-mwirigi
force-pushed
the
fix/174-protect-privileged-roles
branch
from
0712e97 to
7fc24c3
Compare
Closes #174
What changed
isProtectedTarget()exported frommoderation.js— checks server owner, adminRoleId, moderatorRoleId, and custom roleIds/warn,/kick,/ban,/tempban,/softban,/timeout) blocked from targeting protected users viaexecuteModActionskipProtection: trueon/untimeout(removing a punishment should never be blocked)warn()fires with context whenever protection blocks an actionsendModerationLogskips the moderation embed if a flagged user is a protected roleconfigValidation.jsschema updatedModerationProtectRolesinterface)Config schema added