add slack reaction based moderation alerts - #5034
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:
WalkthroughAdds Slack reaction-based moderation support with new rule and alert models, a reaction_added processing service, Slack app wiring, a moderation sync command, local ngrok origin settings, and matching tests and configuration updates. ChangesSlack Moderation Rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/slack/admin/moderation.py (1)
21-34: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
ModerationAlertAdminshould marknest_created_atas readonly to preserve audit integrity.The
nest_created_atfield is displayed inlist_display(line 30) but omitted fromreadonly_fields(line 33), allowing manual editing in the admin interface. Since alerts are audit records documenting when moderation thresholds were met, the creation timestamp should be immutable to prevent accidental or intentional tampering with the audit trail.🔒 Proposed fix to protect the audit timestamp
readonly_fields = ( "conversation", "message_ts", "report_type", "reaction_count", + "nest_created_at", )🤖 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 `@backend/apps/slack/admin/moderation.py` around lines 21 - 34, The `nest_created_at` field is displayed in the `list_display` tuple of the `ModerationAlertAdmin` class but is missing from the `readonly_fields` tuple, allowing it to be edited in the admin interface. To preserve audit integrity and prevent tampering with the creation timestamp, add `nest_created_at` to the `readonly_fields` tuple alongside the other immutable fields such as "conversation", "message_ts", "report_type", and "reaction_count".
🤖 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 `@backend/.env.example`:
- Line 16: The DJANGO_LOCAL_NGROK_ORIGIN environment variable in the
.env.example file lacks documentation about its required format. Add an inline
comment or hint next to the DJANGO_LOCAL_NGROK_ORIGIN setting to clarify that it
must include a URL scheme (e.g., https://abc123.ngrok-free.app), since this
value flows into Django's CSRF_TRUSTED_ORIGINS which requires a scheme. Without
this hint, users may provide only the hostname and encounter silent CSRF/CORS
check failures during setup.
In `@backend/apps/slack/events/reaction_added.py`:
- Around line 47-54: Wrap the client.reactions_get call in a try-except block to
handle potential Slack API failures gracefully. Catch SlackApiError exceptions
that may be raised from the reactions_get method call and log the error
appropriately before returning early from the function. This will prevent
unhandled exceptions from crashing the event handler when the Slack API call
fails.
- Around line 59-89: The ModerationAlert.objects.create() call is happening
before the Slack API calls (chat_getPermalink and chat_postMessage), which means
if either API call fails, an orphaned database record will be created and block
future retries due to the unique constraint. Move the
ModerationAlert.objects.create() call to after both chat_getPermalink() and
chat_postMessage() complete successfully, then immediately set the
alert_message_ts from the response and save it. This ensures the database record
is only persisted when all operations succeed, allowing proper retry behavior on
API failures.
---
Outside diff comments:
In `@backend/apps/slack/admin/moderation.py`:
- Around line 21-34: The `nest_created_at` field is displayed in the
`list_display` tuple of the `ModerationAlertAdmin` class but is missing from the
`readonly_fields` tuple, allowing it to be edited in the admin interface. To
preserve audit integrity and prevent tampering with the creation timestamp, add
`nest_created_at` to the `readonly_fields` tuple alongside the other immutable
fields such as "conversation", "message_ts", "report_type", and
"reaction_count".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 744b958f-d0e5-4ebe-bddc-95f92dfb67a8
📒 Files selected for processing (11)
backend/.env.examplebackend/apps/slack/MANIFEST.yamlbackend/apps/slack/admin/__init__.pybackend/apps/slack/admin/moderation.pybackend/apps/slack/events/__init__.pybackend/apps/slack/events/reaction_added.pybackend/apps/slack/events/url_verification.pybackend/apps/slack/migrations/0023_moderationalert_moderationrule.pybackend/apps/slack/models/__init__.pybackend/apps/slack/models/moderation.pybackend/settings/local.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 11 files
Confidence score: 2/5
- In
backend/apps/slack/events/reaction_added.py, the dedupe record is written before the Slack send, so transient Slack/API failures can mark an alert as already processed and permanently suppress future notifications for that event — make the dedupe write atomic with a successful send (or roll it back on send failure) before merging. - In
backend/apps/slack/admin/moderation.py,ModerationAlertAdminstill allows add/delete despite the “readonly-ish records” intent, which can lead to accidental duplicate records or loss of moderation history and notification integrity — disable add/delete for this admin model before merge. - In
backend/apps/slack/admin/moderation.py,alert_message_tsbeing editable while other tracking fields are readonly can let admins accidentally alter message linkage and make alert tracing unreliable — addalert_message_tstoreadonly_fieldsto keep record immutability consistent. - In
backend/apps/slack/events/url_verification.py,UrlVerification.handler()bypasseshandle_event(), leaving that method unreachable in production dispatch and increasing maintenance/regression risk if logic is updated in the wrong place — either route throughhandle_event()or remove the dead method to align behavior and intent.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5034 +/- ##
==========================================
- Coverage 98.83% 98.80% -0.04%
==========================================
Files 538 544 +6
Lines 17129 17354 +225
Branches 2460 2496 +36
==========================================
+ Hits 16930 17146 +216
- Misses 99 103 +4
- Partials 100 105 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
i have split the original |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/apps/slack/events/reaction_added.py`:
- Around line 40-60: The alert deduplication check happens before posting to
Slack, which creates a race condition where multiple concurrent workers can pass
the _alert_exists check and each post duplicate alerts before the _create_alert
call writes to the database. Refactor the code to use an atomic get_or_create
operation before calling _post_alert, rather than checking _alert_exists upfront
and creating later. Replace the _alert_exists check at the beginning with a
get_or_create call that atomically creates the alert record if it doesn't exist,
then only proceed to get permalink and post alert if the record was newly
created, and finally update the alert record with the alert_message_ts returned
from _post_alert after successful posting instead of at the end with
_create_alert.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95c028a6-876f-47dc-a5f8-0d0ed36420dc
📒 Files selected for processing (2)
backend/.env.examplebackend/apps/slack/events/reaction_added.py
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 3 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
42f1acc
1 similar comment
Signed-off-by: Mr-Rahul-Paul <179798584+Mr-Rahul-Paul@users.noreply.github.com>
Signed-off-by: Mr-Rahul-Paul <179798584+Mr-Rahul-Paul@users.noreply.github.com>
Signed-off-by: Mr-Rahul-Paul <179798584+Mr-Rahul-Paul@users.noreply.github.com>
Signed-off-by: Mr-Rahul-Paul <179798584+Mr-Rahul-Paul@users.noreply.github.com>
|
Signed-off-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 19 files (changes from recent commits).
Confidence score: 3/5
- In
backend/src/apps/slack/models/reaction_alert.py, the lock aroundchat_postMessagecan expire beforerecord()completes, allowing a second handler to post a duplicate alert and then have its lock deleted by the first handler; this creates concrete duplicate-notification risk under slow Slack API calls—switch to an ownership-aware lock (token + compare-and-delete) and ensure lock TTL/renewal covers the full post-and-record critical section.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/src/apps/slack/models/reaction_alert.py">
<violation number="1" location="backend/src/apps/slack/models/reaction_alert.py:9">
P1: If `chat_postMessage` runs longer than 30 seconds, another event can acquire this key and post a duplicate before the first `record()` runs. The first handler then deletes the successor’s lock unconditionally. Use an owner token with compare-and-delete and renew or hold the lease for the complete post.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| from apps.common.models import TimestampedModel | ||
| from apps.slack.models.conversation import Conversation | ||
|
|
||
| LOCK_TTL_SECONDS = 30 |
There was a problem hiding this comment.
P1: If chat_postMessage runs longer than 30 seconds, another event can acquire this key and post a duplicate before the first record() runs. The first handler then deletes the successor’s lock unconditionally. Use an owner token with compare-and-delete and renew or hold the lease for the complete post.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/src/apps/slack/models/reaction_alert.py, line 9:
<comment>If `chat_postMessage` runs longer than 30 seconds, another event can acquire this key and post a duplicate before the first `record()` runs. The first handler then deletes the successor’s lock unconditionally. Use an owner token with compare-and-delete and renew or hold the lease for the complete post.</comment>
<file context>
@@ -1,10 +1,13 @@
from apps.common.models import TimestampedModel
from apps.slack.models.conversation import Conversation
+LOCK_TTL_SECONDS = 30
+
</file context>
Signed-off-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 12 files (changes from recent commits).
Confidence score: 2/5
- In
backend/src/apps/slack/models/reaction_alert.py, the lock-release flow has a race where an expiring 120s lock can be re-acquired by another worker betweencache.getandcache.delete, and the finishing worker may then delete the new owner’s lock, leading to duplicate reaction-alert processing and noisy/incorrect Slack alerts — make lock release owner-safe (e.g., token-checked atomic compare-and-delete or a Lua script) before relying on this for deduplication.
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com>
|
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
4 participants
Proposed Change
Resolves #4577.
This PR adds a Phase 1 implementation for Slack reaction-based content reporting in NestBot.
When a configured reaction reaches its threshold on a Slack message, NestBot records the alert and posts one notification to a configured moderation channel. This phase is notification-only and does not delete, hide, warn, or DM users.
What Changed
ModerationRulefor admin-managed reaction rules and thresholds.ModerationAlertto record sent alerts and prevent duplicates.reaction_addedSlack event handler.reactions:readreaction_addedSetup Notes
For local testing, use a development Slack workspace and a local ngrok tunnel.
Slack needs a public HTTPS callback URL, so
ngrok is requiredfor local testing. Start ngrok, copythe
ForwardingURL, and use that URL in the Slack app manifest for the Slack callback URLs.Moderation rules are configured in Django admin.
owasp.org...to the ngrok link inhttps://app.slack.com/app-settings/.../.../app-manifesthttps://api.slack.com/apps/.../install-on-teamForwardingngrok link inbackend/.envRequired local values include:
SLACK_BOT_TOKEN_<WORKSPACE_ID>(which is same asDJANGO_SLACK_BOT_TOKEN) is needed when running the existingslack_sync_datacommand to create Conversation rows. For a single development workspace, it can reuse the same token value asDJANGO_SLACK_BOT_TOKEN.After updating the Slack manifest, the app must be reinstalled so the new event and scope are active.
Notes
todo
Add unit tests for reaction handling and duplicate-alert behavior.done.Improve validation for moderation rule fields.done.move the logic in a seperate servicedone.Checklist
make check-testlocally: all warnings addressed, tests passed.