feat(slack): add BotInteraction model and reaction feedback handler#5035
Open
mohityadav8 wants to merge 13 commits into
Open
feat(slack): add BotInteraction model and reaction feedback handler#5035mohityadav8 wants to merge 13 commits into
mohityadav8 wants to merge 13 commits into
Conversation
Contributor
|
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 a ChangesBot Interaction Persistence and Reaction Feedback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
mohityadav8
force-pushed
the
feat/nestbot-bot-interaction
branch
from
e10eddf to
baa224b
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Confidence score: 2/5
- In
backend/apps/slack/events/reaction_added/bot_feedback.py, reaction matching is keyed only byts, so identical Slack timestamps across channels can attach 👍/👎 to the wrong interaction (or fail on duplicates), which is a direct feedback-integrity risk in production — scope the lookup by both channel and timestamp before merging. - In
backend/apps/slack/models/bot_interaction.py,slack_reply_tsis used for reaction correlation but has no DB index, so lookup cost will degrade toward full-table scans as data grows and can slow or stall event handling — add an index onslack_reply_ts(or a composite index matching the final lookup) before merge. - In
backend/apps/slack/migrations/0023_bot_interaction.py, allowing nullable/emptyslack_reply_tspermits rows that can never be matched fromreaction_added, creating permanently un-linkable feedback records — make the field required (or enforce a safe backfill/validation path) before merging. - In
backend/tests/unit/apps/slack/events/reaction_added/bot_feedback_test.py, the fallback test currently checks falsy-response behavior rather than the missing-tspath, so it can miss regressions in timestamp extraction — change the fixture to{"ok": True}to verify the intended branch and de-risk the fix.
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/apps/slack/migrations/0023_bot_interaction.py">
<violation number="1" location="backend/apps/slack/migrations/0023_bot_interaction.py:40">
P2: `slack_reply_ts` should not be optional because it is the key used to attach 👍/👎 feedback. Allowing empty values stores interactions that can never be correlated back from reactions.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ( | ||
| "intent_category", | ||
| models.CharField( | ||
| blank=True, |
Contributor
There was a problem hiding this comment.
P2: slack_reply_ts should not be optional because it is the key used to attach 👍/👎 feedback. Allowing empty values stores interactions that can never be correlated back from reactions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/slack/migrations/0023_bot_interaction.py, line 40:
<comment>`slack_reply_ts` should not be optional because it is the key used to attach 👍/👎 feedback. Allowing empty values stores interactions that can never be correlated back from reactions.</comment>
<file context>
@@ -0,0 +1,83 @@
+ (
+ "intent_category",
+ models.CharField(
+ blank=True,
+ default="",
+ max_length=64,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/unit/apps/slack/services/message_auto_reply_test.py (1)
36-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMock message fixture missing
raw_dataattribute.The service implementation accesses
message.raw_data.get("user", "")to extract the user ID, but themock_messagefixture doesn't setraw_data. This causes the tests to pass with a Mock object asuser_idinstead of validating the actual string value.Compare with the fixture in
bot_feedback_test.pyline 214, which correctly sets:message.raw_data = {"user": "U789"}🔧 Proposed fix
`@pytest.fixture` def mock_message(self, mock_conversation): """Mock message object.""" message = Mock(spec=Message) message.id = 1 message.slack_message_id = "1234567890.123456" message.text = "What is OWASP?" + message.raw_data = {"user": "U123456"} message.conversation = mock_conversation return message🤖 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/tests/unit/apps/slack/services/message_auto_reply_test.py` around lines 36 - 43, The mock_message method is missing the raw_data attribute that the service implementation accesses via message.raw_data.get("user", ""). Add a raw_data dictionary attribute to the Mock message object containing a "user" key with a user ID string value (such as "U789"), similar to how it's properly configured in the bot_feedback_test.py fixture at line 214. This ensures the tests validate the actual string user_id value instead of passing a Mock object.
🤖 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/admin/__init__.py`:
- Line 8: The file is missing a trailing newline at the end, which is required
by PEP 8 and checked by Ruff W292. Add a newline character at the very end of
the file after the import statement for WorkspaceAdmin to comply with the PEP 8
style guide.
In `@backend/apps/slack/admin/bot_interaction.py`:
- Around line 73-76: In the short_message method, first update the docstring to
use imperative mood instead of descriptive mood (e.g., "Truncate" or "Return
truncated" instead of "Truncated"). Second, extract the magic number 60 as a
named constant at the module or class level (e.g., MAX_MESSAGE_LENGTH = 60) and
use that constant in place of the hardcoded value. Third, add a None check for
obj.user_message before attempting to slice it to prevent potential TypeError if
the field is ever null, handling the null case appropriately (returning empty
string or a default value).
- Line 80: The file is missing a trailing newline at the end, which violates PEP
8 standards and triggers the Ruff W292 linting rule. Add a newline character at
the very end of the file after the last line of code to comply with static
analysis requirements.
In `@backend/apps/slack/events/reaction_added/bot_feedback.py`:
- Line 22: Remove trailing whitespace from the blank lines in this file. The
lines 22, 26, and 30 contain only whitespace characters (spaces or tabs) which
cause Ruff W293 violations; delete all trailing spaces on these blank lines
while preserving the blank lines themselves (keep the newlines) to pass
pre-commit checks.
- Around line 45-47: The BotInteraction.objects.get() call on line 45 queries
using slack_reply_ts, which is not a unique field in the model contract, so it
can raise MultipleObjectsReturned when duplicate rows exist. This exception is
not currently being caught, causing feedback processing to fail. Either add a
try-except block to catch the MultipleObjectsReturned exception alongside the
DoesNotExist exception and handle it gracefully (similar to the non-bot message
case), or switch to using .filter() with slack_reply_ts and handle the case
where multiple objects are returned by selecting the most recent interaction or
another disambiguating field. Ensure the fix prevents updating the wrong
interaction when collisions occur.
In `@backend/apps/slack/models/bot_interaction.py`:
- Around line 52-58: The slack_reply_ts field in the bot_interaction model is
used for matching reaction_added events but lacks database indexing, causing
full table scans as data grows. Add the db_index=True parameter to the
slack_reply_ts CharField definition to create a database index on this field,
which will significantly improve lookup performance for feedback correlation
operations.
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 58-64: The BotInteraction.objects.create() call can fail after
chat_postMessage succeeds, causing the entire method to raise an exception
despite the reply being successfully sent. Wrap the
BotInteraction.objects.create() statement in a try-except block to isolate
database persistence failures from the reply path. In the except block, log the
error appropriately without re-raising the exception, ensuring that database
creation failures do not trigger job retries when the actual Slack message reply
has already succeeded.
In `@backend/tests/unit/apps/slack/events/reaction_added/bot_feedback_test.py`:
- Line 324: The file is missing a trailing newline character at the end, which
causes a POSIX compliance warning. Add a blank newline at the very end of the
file after the last assertion statement (the line containing assert
kwargs["slack_reply_ts"] == "") to ensure the file properly terminates with a
newline character.
- Line 1: The test directory containing bot_feedback_test.py is missing an
__init__ file, which prevents Python from recognizing it as a package and causes
the pipeline to fail. Create an empty __init__ file in the same directory as
bot_feedback_test.py to properly establish it as a Python package and allow the
test runner to discover and execute the tests correctly.
- Around line 143-151: In the test_thumbs_down_sets_false method, update the
mock_interaction.save assertion to include verification of the update_fields
argument for consistency with test_thumbs_up_sets_true. Change the
mock_interaction.save.assert_called_once() call to
assert_called_once_with(update_fields=["thumbs_up", "nest_updated_at"]) to match
the pattern and level of detail used in the corresponding thumbs-up test.
In `@backend/tests/unit/apps/slack/services/message_auto_reply_test.py`:
- Line 230: The file is missing a trailing newline at the end, which violates
POSIX standards and triggers the W292 linter warning. Add a newline character at
the very end of the file, after the last assertion statement
mock_client.chat_postMessage.assert_not_called().
---
Outside diff comments:
In `@backend/tests/unit/apps/slack/services/message_auto_reply_test.py`:
- Around line 36-43: The mock_message method is missing the raw_data attribute
that the service implementation accesses via message.raw_data.get("user", "").
Add a raw_data dictionary attribute to the Mock message object containing a
"user" key with a user ID string value (such as "U789"), similar to how it's
properly configured in the bot_feedback_test.py fixture at line 214. This
ensures the tests validate the actual string user_id value instead of passing a
Mock object.
🪄 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: aac0327b-2914-445e-a67d-59b791efb60b
📒 Files selected for processing (12)
backend/apps/slack/admin/__init__.pybackend/apps/slack/admin/bot_interaction.pybackend/apps/slack/events/__init__.pybackend/apps/slack/events/member_joined_channel/__init__.pybackend/apps/slack/events/reaction_added/__init__.pybackend/apps/slack/events/reaction_added/bot_feedback.pybackend/apps/slack/migrations/0023_bot_interaction.pybackend/apps/slack/models/__init__.pybackend/apps/slack/models/bot_interaction.pybackend/apps/slack/services/message_auto_reply.pybackend/tests/unit/apps/slack/events/reaction_added/bot_feedback_test.pybackend/tests/unit/apps/slack/services/message_auto_reply_test.py
Contributor
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 4 unresolved issues from previous reviews.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/slack/models/bot_interaction.py (1)
17-59: 🧹 Nitpick | 🔵 TrivialConsider a data-retention/PII policy for stored interactions.
BotInteractionpersistsuser_id,user_message, andbot_responseindefinitely. Since these are user-identifiable content captured for a feedback loop, consider documenting/implementing a retention or purge policy (and confirming alignment with GDPR/CCPA) so this table doesn't accumulate PII without bound.🤖 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/models/bot_interaction.py` around lines 17 - 59, Add a retention/purge policy for BotInteraction, since the model stores user_id, user_message, and bot_response as user-identifiable content. Update the BotInteraction model or its related cleanup path to support scheduled deletion or anonymization after a defined period, and document the policy/PII handling so the storage approach is explicitly aligned with GDPR/CCPA expectations.
🤖 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.
Outside diff comments:
In `@backend/apps/slack/models/bot_interaction.py`:
- Around line 17-59: Add a retention/purge policy for BotInteraction, since the
model stores user_id, user_message, and bot_response as user-identifiable
content. Update the BotInteraction model or its related cleanup path to support
scheduled deletion or anonymization after a defined period, and document the
policy/PII handling so the storage approach is explicitly aligned with GDPR/CCPA
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 976f18aa-cb63-4e4b-9990-58c37cee2128
📒 Files selected for processing (8)
backend/apps/slack/admin/__init__.pybackend/apps/slack/admin/bot_interaction.pybackend/apps/slack/events/reaction_added/bot_feedback.pybackend/apps/slack/migrations/0023_bot_interaction.pybackend/apps/slack/models/bot_interaction.pybackend/apps/slack/services/message_auto_reply.pybackend/tests/unit/apps/slack/events/reaction_added/bot_feedback_test.pybackend/tests/unit/apps/slack/services/message_auto_reply_test.py
Contributor
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Removed comment about generated migration for BotInteraction model.
Contributor
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
arkid15r
requested changes
Jun 25, 2026
arkid15r
left a comment
arkid15r
left a comment
Collaborator
There was a problem hiding this comment.
Please fix pre-commit before requesting review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/apps/slack/migrations/0023_bot_interaction.py">
<violation number="1" location="backend/apps/slack/migrations/0023_bot_interaction.py:40">
P2: `slack_reply_ts` should not be optional because it is the key used to attach 👍/👎 feedback. Allowing empty values stores interactions that can never be correlated back from reactions.</violation>
</file>
<file name="backend/tests/unit/apps/slack/services/message_auto_reply_test.py">
<violation number="1" location="backend/tests/unit/apps/slack/services/message_auto_reply_test.py:100">
P2: BotInteraction.objects.create is asserted with assert_called_once() but arguments are not verified; this could miss incorrect data being persisted for the new feedback loop model.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
0 issues found across 6 files (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5035 +/- ##
==========================================
- Coverage 98.74% 98.71% -0.03%
==========================================
Files 539 542 +3
Lines 17069 17137 +68
Branches 2421 2427 +6
==========================================
+ Hits 16854 16917 +63
- Misses 123 128 +5
Partials 92 92
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #908
This PR implements the feedback loop for NestBot AI replies by adding:
BotInteractionmodel — logs every AI-generated reply with fields forchannel_id,user_id,user_message,bot_response,intent_category,confidence_score,thumbs_up,tokens_used, andslack_reply_ts0023_bot_interaction— creates theslack_bot_interactionstablereaction_addedevent handler (bot_feedback.py) — listens for 👍/👎 reactions on bot replies and updates thethumbs_upfield on the matchingBotInteractionrowmessage_auto_reply.py— afterchat_postMessage, creates aBotInteractionrow with the reply'stsso reactions can be matched backBotInteractionis registered with search, filter, and readonly fieldsChecklist
make check-testlocally: all warnings addressed, tests passed