fix: drop misleading bot_permissions preflights, guard configured-channel sends#79
Conversation
…veaway add_reactions Config-setter commands (welcome set_welcome_channel/set_auto_role, starboard_channel, verification_setup, verification_panel) store data then respond ephemeral — they never perform the privileged Discord op at invocation, so bot_permissions checked the wrong scope. Dropped from all five. giveaway: removed add_reactions (button-based flow, reactions never called). _command_callbacks: corrected misleading comment — sweep is not rescheduled at first dispatch if get_running_loop raises at decoration time.
…l (B-021) The dropped bot_permissions preflight only ever checked the invocation channel; these sends target a *configured* channel and were never actually covered: - verification_panel: Forbidden/HTTPException now respond with an ephemeral error instead of escaping the command; nothing persisted when the panel never posted (Codex P2 on PR #78) - welcome/goodbye event sends: except HTTPException pass, matching the auto-role guard — event path must not raise into the dispatcher (same class as B-014/B-015/B-016) Regression tests for both; bugs.md B-021 filed.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Reviewer's GuideRemoves misleading decorator-level bot_permissions checks for configuration-only commands and adds direct guards around sends to configured channels, especially for the verification panel and welcome/goodbye handlers, with regression tests and documentation updates. Sequence diagram for guarded verification_panel send to configured channelsequenceDiagram
actor User
participant VerificationPlugin as VerificationPlugin
participant Context as Context
participant ConfiguredChannel as ConfiguredChannel
User->>VerificationPlugin: verification_panel(ctx)
VerificationPlugin->>VerificationPlugin: _get_guild_config(guild_id)
VerificationPlugin->>ConfiguredChannel: channel.send(embed, view)
alt discord.Forbidden
VerificationPlugin->>VerificationPlugin: logger.error(...)
VerificationPlugin->>Context: respond(ephemeral error, ephemeral=True)
VerificationPlugin-->>User: ephemeral error shown
else discord.HTTPException
VerificationPlugin->>VerificationPlugin: logger.error(...)
VerificationPlugin->>Context: respond(generic failure, ephemeral=True)
VerificationPlugin-->>User: ephemeral error shown
else success
VerificationPlugin->>VerificationPlugin: bot.add_view(view, message_id)
VerificationPlugin-->>User: verification panel posted
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds a shared ChangesGuarded Discord delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant VerificationPlugin
participant send_safe
participant DiscordChannel
participant CommandInvoker
participant PanelStore
VerificationPlugin->>send_safe: Send verification embed and view
send_safe->>DiscordChannel: channel.send(...)
DiscordChannel-->>send_safe: Message or Discord exception
send_safe-->>VerificationPlugin: Message or None
alt send succeeds
VerificationPlugin->>PanelStore: Persist panel_message_id
else send fails
VerificationPlugin->>CommandInvoker: Send ephemeral error response
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
bug
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the welcome/goodbye event handlers, swallowing all
discord.HTTPExceptionwithout logging makes debugging real failures difficult; consider either narrowing this toForbiddenor at least emitting a debug log when the send fails. - You now have several places that implement the same "configured channel send with Forbidden/HTTPException handling" pattern (verification panel, welcome/goodbye, auto-role); consider extracting a small helper for this to keep the behavior consistent and reduce duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the welcome/goodbye event handlers, swallowing all `discord.HTTPException` without logging makes debugging real failures difficult; consider either narrowing this to `Forbidden` or at least emitting a debug log when the send fails.
- You now have several places that implement the same "configured channel send with Forbidden/HTTPException handling" pattern (verification panel, welcome/goodbye, auto-role); consider extracting a small helper for this to keep the behavior consistent and reduce duplication.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Covers the remaining B-021 guard branches flagged by codecov/patch.
Review feedback on the B-021 guards: - send_safe() in helpers/channel.py sends to a configured channel, absorbs Forbidden/HTTPException with a logged warning, and returns the message or None; adopted by verification_panel and the welcome/goodbye event sends (previously except-pass with no log) - welcome auto-role failure now logs a warning instead of a silent pass (add_roles, so the send helper doesn't apply) - unit tests for all three helper branches member_logging._log_to_channel has the same shape but lives in PR #78's diff; adopting it there is a post-merge follow-up.
…-send-guards # Conflicts: # bugs.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bugs.md`:
- Around line 27-28: Insert a blank line between the B-021 heading and the
following “Where” list item in bugs.md, leaving the heading text and list
content unchanged.
In `@easycord/plugins/welcome.py`:
- Around line 79-84: Extract the inline auto-role assignment handling near the
welcome event into a shared add_roles_safe-style helper, analogous to send_safe,
with the helper owning the Discord call, warning log, and HTTPException handling
so exceptions cannot escape the dispatcher. Route the auto-role path through
this helper and add a regression test asserting the warning is emitted on
assignment failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: abeee6ea-3776-4a18-95fc-e4b82e98512e
📒 Files selected for processing (14)
.planning/config.jsonAGENTS.mdCLAUDE.mdbugs.mdeasycord/_command_callbacks.pyeasycord/helpers/__init__.pyeasycord/helpers/channel.pyeasycord/plugins/giveaway.pyeasycord/plugins/starboard.pyeasycord/plugins/verification.pyeasycord/plugins/welcome.pytests/test_helpers.pytests/test_plugin_commands.pytests/test_verification.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: CodeRabbit
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: For questions about EasyCord architecture, module relationships, data flows, or cross-cutting patterns, querygraphify-out/graph.jsonwith/graphifybefore manually reading source files; do not rebuild the graph or read deprecated vault files.
The CI PR gate runs critical Ruff checks, advisory full Ruff, release metadata validation, plugin-test verification, and pytest across Python 3.10, 3.11, and 3.12 in that order.
Before making changes, verify repository state withgit status --short --branchandgit log --oneline -5 --decorate.
Files:
easycord/helpers/__init__.pyAGENTS.mdeasycord/plugins/starboard.pyeasycord/_command_callbacks.pyeasycord/plugins/giveaway.pyCLAUDE.mdtests/test_helpers.pybugs.mdeasycord/helpers/channel.pytests/test_verification.pytests/test_plugin_commands.pyeasycord/plugins/verification.pyeasycord/plugins/welcome.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Install development dependencies withpip install -e ".[dev]".
Async tests usepytest-asynciowithasyncio_mode = "auto"; do not manually set up an event loop.
Build the distribution package withpython -m build.
Files:
easycord/helpers/__init__.pyeasycord/plugins/starboard.pyeasycord/_command_callbacks.pyeasycord/plugins/giveaway.pytests/test_helpers.pyeasycord/helpers/channel.pytests/test_verification.pytests/test_plugin_commands.pyeasycord/plugins/verification.pyeasycord/plugins/welcome.py
{easycord,tests}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
The blocking lint gate runs
ruff check easycord tests --select E9,F63,F7,F82andpython scripts/verify_plugin_tests.py.
Files:
easycord/helpers/__init__.pyeasycord/plugins/starboard.pyeasycord/_command_callbacks.pyeasycord/plugins/giveaway.pytests/test_helpers.pyeasycord/helpers/channel.pytests/test_verification.pytests/test_plugin_commands.pyeasycord/plugins/verification.pyeasycord/plugins/welcome.py
easycord/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
easycord/**/*.py: Bot-level behavior must be added to the appropriate bot mixin file:_bot_commands.py,_bot_events.py,_bot_guild.py, or_bot_plugins.py;bot.pydefinesBotthrough multiple inheritance.
Context functionality belongs incontext.pyor the relevant_context_channels.py,_context_moderation.py, or_context_ui.pymixin.
Route AI orchestration throughorchestrator.pyand provider adapters inplugins/_ai_providers.py; register tools withToolRegistry, enforceToolSafety, and await everyToolLimitermethod.
Per-guild state must always go through the database layer and must not be stored directly on theBotinstance.
Look up localization keys throughLocalizationManager; do not hardcode strings in plugin responses.
Usectx.userorctx.member;ctx.authordoes not exist.
Treatctx.is_adminas a property and do not call it asctx.is_admin().
Cooldown sentinels must default tofloat("-inf"), not0.0, so first-message events pass on fresh runners.
easycord/**/*.py: Import public APIs fromeasycord; do not import internal_-prefixed modules directly.
Usectx.userorctx.member, neverctx.author; treatctx.is_adminas a property and never call it.
Store per-guild plugin state in the database layer, not on thePlugininstance.
Never hardcode plugin response strings; look them up throughctx.t(...).
Always await asynchronousToolLimitermethods such ascheck_limit,reset_user, andreset_tool.
Require an explicitToolSafetyannotation when registering an@ai_tool.
Narrow channel types withSENDABLE_CHANNEL_TYPESbefore calling.send()on a channel obtained from context or Discord.
InitializeLevelsPlugin._cooldownssentinels withfloat("-inf"), not0.0, so the first message passes.
Route every destructive action in event-path plugins through one governed method that owns rate limiting, channel narrowing, and Discord error handling; Discord exceptions must not escape the dispatc...
Files:
easycord/helpers/__init__.pyeasycord/plugins/starboard.pyeasycord/_command_callbacks.pyeasycord/plugins/giveaway.pyeasycord/helpers/channel.pyeasycord/plugins/verification.pyeasycord/plugins/welcome.py
**/*.{py,md}
📄 CodeRabbit inference engine (CLAUDE.md)
For questions about EasyCord architecture, module relationships, data flows, or cross-cutting patterns, query
C:\Users\Tom\Code projects\EasyCord\graphify-out\graph.jsonbefore manually reading source files. Do not rebuild the graph or read deprecated vault files.
Files:
easycord/helpers/__init__.pyAGENTS.mdeasycord/plugins/starboard.pyeasycord/_command_callbacks.pyeasycord/plugins/giveaway.pyCLAUDE.mdtests/test_helpers.pybugs.mdeasycord/helpers/channel.pytests/test_verification.pytests/test_plugin_commands.pyeasycord/plugins/verification.pyeasycord/plugins/welcome.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run the test suite with
pytest tests/; individual tests may be run with pytest node IDs.
tests/**/*.py: Useeasycord.testinghelpers such asinvoke,FakeContextBuilder, andPluginTestSuitefor command and plugin tests.
When constructing plugins in tests, use__new__, assign_bot, and then callPlugin.__init__; do not assignbotdirectly.
Files:
tests/test_helpers.pytests/test_verification.pytests/test_plugin_commands.py
🪛 markdownlint-cli2 (0.22.1)
bugs.md
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (13)
easycord/helpers/channel.py (1)
4-6: LGTM!Also applies to: 16-40
easycord/helpers/__init__.py (1)
2-2: LGTM!Also applies to: 16-16
tests/test_helpers.py (1)
1-10: LGTM!Also applies to: 149-183
easycord/plugins/giveaway.py (1)
263-263: LGTM!easycord/plugins/starboard.py (1)
262-262: LGTM!.planning/config.json (1)
61-61: LGTM!AGENTS.md (1)
5-17: LGTM!CLAUDE.md (1)
9-9: LGTM!easycord/_command_callbacks.py (1)
87-93: 🩺 Stability & AvailabilityVerify cooldown-sweep startup when no event loop is running.
This path permanently skips cleanup when
build_slash_callbackis constructed outside an active loop; first dispatch does not start the sweep. Confirm registration always occurs inside a running loop, or lazily create the sweep task on first invocation to prevent stale buckets from accumulating.easycord/plugins/verification.py (1)
11-11: LGTM!Also applies to: 228-228, 265-265, 299-311
tests/test_verification.py (1)
288-317: LGTM!Also applies to: 318-341
easycord/plugins/welcome.py (1)
4-10: LGTM!Also applies to: 20-21, 103-103, 122-126, 150-150
tests/test_plugin_commands.py (1)
279-303: LGTM!Also applies to: 305-322
| ## B-021 — Unguarded sends in verification_panel and welcome event handlers | ||
| - **Where:** `easycord/plugins/verification.py` `verification_panel` (panel send to the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the B-021 heading.
markdownlint reports MD022 because the list starts immediately after ## B-021 ...; insert one blank line before - **Where:**.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bugs.md` around lines 27 - 28, Insert a blank line between the B-021 heading
and the following “Where” list item in bugs.md, leaving the heading text and
list content unchanged.
Source: Linters/SAST tools
| except discord.HTTPException as exc: | ||
| # bot may lack manage_roles or the role may be above its top role | ||
| logger.warning( | ||
| "Failed to assign auto-role %s in guild %s: %s", | ||
| role.id, member.guild.id, exc, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider a shared helper for role-assignment failures, mirroring send_safe.
Logging instead of silently swallowing is a good fix, but the auto-role assignment is still handled with an ad-hoc inline try/except, unlike the message sends in this same file which are now routed through the shared send_safe helper. This diverges from the pattern this PR just established for guarded Discord operations, and there's no dedicated test asserting the new warning-log behavior (only the send-failure paths gained regression tests).
Consider extracting an analogous add_roles_safe-style helper (owning the try/except + logging) for consistency and to make this path independently testable, and add a regression test for the auto-role failure log path.
As per path instructions, "Route every destructive action in event-path plugins through one governed method that owns rate limiting, channel narrowing, and Discord error handling; Discord exceptions must not escape the dispatcher."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@easycord/plugins/welcome.py` around lines 79 - 84, Extract the inline
auto-role assignment handling near the welcome event into a shared
add_roles_safe-style helper, analogous to send_safe, with the helper owning the
Discord call, warning log, and HTTPException handling so exceptions cannot
escape the dispatcher. Route the auto-role path through this helper and add a
regression test asserting the warning is emitted on assignment failure.
Source: Path instructions
2 participants
Carries two commits split out of #78 (they were unpushed local-main commits that leaked into that PR's diff), plus the fix for the genuine review finding they surfaced.
Changes
Cherry-picked from local main:
380e61a→ dropbot_permissionsfrom config-setter commands (welcomeset_welcome_channel/set_auto_role,starboard_channel,verification_setup,verification_panel) and giveawayadd_reactions(button-based flow, no reaction code). These preflights validated the invocation channel while the privileged operation targets a configured channel (or happens later on a different event) — wrong scope.5690a05→ CLAUDE.md/AGENTS.md planning-config sync.New (B-021), addressing the Codex P2 review on #78:
verification_panelreally does send at invocation — to the configured channel.Forbidden/HTTPExceptionare now caught with an ephemeral error response, and nothing is persisted when the panel never posted.except HTTPException: passguards, matching the auto-role guard above them (same event-path class as B-014/B-015/B-016).Review findings dispositions (from #78 bots)
Notes
Test plan
🤖 Generated with Claude Code
Summary by Sourcery
Guard configured-channel sends in verification and welcome flows while removing misleading bot_permissions preflights.
Bug Fixes:
Enhancements:
Documentation:
Tests: