fix(slack): NestBot async UX, Slack-safe AI replies, and deferred path hardening#4319
Draft
rajnisk wants to merge 3 commits intoOWASP:feature/nestbot-ai-assistantfrom
Draft
Conversation
Use async processing for mentions and /ai commands, tighten event boundaries, and stabilize reaction/thinking cleanup across success, no-answer, and error paths. Made-with: Cursor
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughEnqueues Slack AI queries to an RQ background job across slash command, app_mention, and message-posted handlers; converts synchronous block rendering and image-downloads into async processing; adds Slack-sized markdown block splitting, fallback truncation, reaction/error handling, and extensive corresponding test updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- There is a concrete regression risk in
backend/apps/slack/events/message_posted.py: returning onauth_testfailure can skip question detection, message persistence, and enqueueing, so non-mention OWASP questions may be missed during transient Slack auth failures. backend/apps/slack/events/app_mention.pynow only rescuesSlackApiErroraroundreactions_add; other SDK/transport exceptions can bubble up and aborthandle_event, which can block app-mention enqueueing.- Given the medium severity and fairly confident, user-facing processing impact in event handlers, this looks mergeable only with some caution rather than low-risk.
- Pay close attention to
backend/apps/slack/events/message_posted.pyandbackend/apps/slack/events/app_mention.py- exception handling changes may drop or abort Slack event processing paths.
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/events/app_mention.py">
<violation number="1" location="backend/apps/slack/events/app_mention.py:73">
P2: `reactions_add` now only catches SlackApiError; other Slack SDK client/transport exceptions can bubble out and abort `handle_event`, preventing the enqueue of the app-mention processing. This is a regression from the previous broad exception guard and can drop mentions on transient request errors.</violation>
</file>
<file name="backend/apps/slack/events/message_posted.py">
<violation number="1" location="backend/apps/slack/events/message_posted.py:78">
P2: Returning on auth_test failure drops all subsequent message processing (question detection, message persistence, enqueueing), causing non-mention OWASP questions to be missed whenever Slack API auth_test transiently fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/slack/commands/ai.py`:
- Around line 18-25: The custom handler method in ai.py overrides
CommandBase.handler and bypasses the global Slack commands gate; restore that
gate by either calling super().handler(ack, command, client) at the start of
ai.Command.handler or by explicitly checking settings.SLACK_COMMANDS_ENABLED
(the same logic used in CommandBase.handler) before ack/enqueuing work; ensure
the check prevents further handling when disabled and keep the existing
channel_id/user_id/query extraction after the gate passes.
In `@backend/apps/slack/events/app_mention.py`:
- Around line 96-103: The handler is synchronously downloading/base64-encoding
attachments and enqueuing large blobs as image_uris; change the enqueue call
that invokes django_rq.get_queue("ai").enqueue(process_ai_query_async, ...) to
pass lightweight references (e.g., slack file IDs or signed URLs) instead of the
full base64 image_uris, and move the download + base64-encoding logic into
process_ai_query_async so the worker fetches and encodes attachments when
running the job; update the parameter name (e.g., images -> image_refs) in both
the enqueue call and the process_ai_query_async signature to match.
In `@backend/apps/slack/events/message_posted.py`:
- Around line 59-78: The handler currently aborts on SlackApiError after calling
client.auth_test(), which causes normal messages to be dropped; change the
except SlackApiError block in MessagePosted (around client.auth_test() usage) to
not return—log the error with details (channel_id, team_id) and fall back to a
best-effort behavior by leaving bot_user_id unset (or setting a sentinel) so the
rest of the message handling proceeds and uses an alternative mention check
instead of relying on auth_test(); update any logic that uses
MessagePosted._bot_user_id_by_team and bot_user_id to tolerate a missing
bot_user_id and perform the approximate mention suppression accordingly.
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 219-241: The success branch always posts publicly via
client.chat_postMessage; change it to mirror the no-answer branch routing so
`/ai` slash-command responses stay private: after building blocks =
_format_response_blocks(ai_response_text), if is_app_mention send
client.chat_postMessage to channel_id with thread_ts or message_ts, elif user_id
send client.chat_postEphemeral to channel_id for user_id (with text/blocks),
otherwise fallback to client.chat_postMessage; wrap API calls in
contextlib.suppress(SlackApiError) as done in the error path to match existing
behavior.
🪄 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: CHILL
Plan: Pro
Run ID: acf9bbaa-1fae-4028-9e9b-caf39cdca709
📒 Files selected for processing (4)
backend/apps/slack/commands/ai.pybackend/apps/slack/events/app_mention.pybackend/apps/slack/events/message_posted.pybackend/apps/slack/services/message_auto_reply.py
- Async mentions/slash: reactions, thinking message cleanup, DM for /ai - Defer image download to worker; pass slack_image_files metadata - message_posted: skip when bot mentioned; auth_test failure continues - Split AI mrkdwn into ≤3000-char blocks; truncate chat.postMessage fallback text - Tests for markdown_blocks and message_auto_reply flows Made-with: Cursor
Contributor
There was a problem hiding this comment.
7 issues found across 10 files (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/events/app_mention.py">
<violation number="1" location="backend/apps/slack/events/app_mention.py:71">
P2: `already_reacted` is no longer treated as a benign Slack idempotency case, so expected duplicates/retries are now logged as exceptions.</violation>
</file>
<file name="backend/apps/slack/events/message_posted.py">
<violation number="1" location="backend/apps/slack/events/message_posted.py:78">
P2: On `auth_test` failure, message handler now continues as non-mention, so mention messages may bypass mention guard and be processed in addition to `app_mention`, causing duplicate AI handling.</violation>
</file>
<file name="backend/tests/apps/slack/services/message_auto_reply_test.py">
<violation number="1" location="backend/tests/apps/slack/services/message_auto_reply_test.py:98">
P2: Success-path test no longer verifies the final AI reply channel, allowing channel-routing regressions to pass undetected.</violation>
<violation number="2" location="backend/tests/apps/slack/services/message_auto_reply_test.py:184">
P2: The Slack API error test assertion is overly permissive (`>= 1`) and can miss regressions where only the temporary "Thinking..." message is posted but the final AI response is not.</violation>
</file>
<file name="backend/apps/slack/utils.py">
<violation number="1" location="backend/apps/slack/utils.py:88">
P2: `truncate_for_slack_fallback` can exceed `max_len` when `max_len` is smaller than the truncation suffix, breaking truncation correctness.</violation>
</file>
<file name="backend/apps/slack/blocks.py">
<violation number="1" location="backend/apps/slack/blocks.py:85">
P2: `markdown_blocks` can raise `IndexError` when `max_blocks` is 0 (or negative) because it slices `parts` to empty and then accesses `parts[-1]`.</violation>
<violation number="2" location="backend/apps/slack/blocks.py:94">
P2: Truncation appends ellipsis without reserving space, so final section text can exceed the 3000-character limit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| assert len(block_posts) == 1 | ||
| assert block_posts[0].kwargs["blocks"] == response_blocks | ||
| assert block_posts[0].kwargs["text"] == truncate_for_slack_fallback(ai_text) | ||
| assert block_posts[0].kwargs["thread_ts"] == mock_message.slack_message_id |
Contributor
There was a problem hiding this comment.
P2: Success-path test no longer verifies the final AI reply channel, allowing channel-routing regressions to pass undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/apps/slack/services/message_auto_reply_test.py, line 98:
<comment>Success-path test no longer verifies the final AI reply channel, allowing channel-routing regressions to pass undetected.</comment>
<file context>
@@ -80,24 +86,16 @@ def test_generate_ai_reply_success(
+ assert len(block_posts) == 1
+ assert block_posts[0].kwargs["blocks"] == response_blocks
+ assert block_posts[0].kwargs["text"] == truncate_for_slack_fallback(ai_text)
+ assert block_posts[0].kwargs["thread_ts"] == mock_message.slack_message_id
@patch("apps.slack.services.message_auto_reply.Message.objects.get")
</file context>
Suggested change
| assert block_posts[0].kwargs["thread_ts"] == mock_message.slack_message_id | |
| assert block_posts[0].kwargs["channel"] == mock_message.conversation.slack_channel_id | |
| assert block_posts[0].kwargs["thread_ts"] == mock_message.slack_message_id |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/slack/blocks.py`:
- Around line 87-95: The truncation for the last block doesn't reserve space for
the three-dot ellipsis so the final string can exceed
SLACK_SECTION_MRKDWN_MAX_CHARS; when computing room in the max_blocks branch
(variables: parts, max_blocks, suffix, SLACK_SECTION_MRKDWN_MAX_CHARS), subtract
the length of the ellipsis as well (or otherwise ensure the final concatenation
"..."+suffix fits), update the room check to compare against that reduced budget
and adjust the fallback when room < 1 accordingly so parts[-1] never produces a
section longer than SLACK_SECTION_MRKDWN_MAX_CHARS.
In `@backend/apps/slack/common/handlers/ai.py`:
- Around line 36-40: The current flow checks ai_response before formatting, but
format_ai_response_for_slack(ai_response) can normalize a non-empty input to an
empty string, causing markdown_blocks(formatted_response) to return an empty
payload; change the logic to format first and then check formatted_response for
truthiness: call format_ai_response_for_slack(ai_response), store to
formatted_response, and if formatted_response is empty/falsey return
get_error_blocks(), otherwise return markdown_blocks(formatted_response);
reference the existing symbols ai_response, format_ai_response_for_slack,
formatted_response, markdown_blocks, and get_error_blocks when making this
change.
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 50-78: _post_slash_command_dm currently swallows SlackApiError and
returns None so callers (e.g., process_ai_query_async) can’t detect delivery
failures; change _post_slash_command_dm to return an explicit success/failure
signal (bool) or re-raise the caught SlackApiError so the caller can act on it —
update callers (like process_ai_query_async) to handle the boolean/exception and
implement the proposed fallback path (e.g., send ephemeral message or log and
surface error to user) when delivery fails; refer to function name
_post_slash_command_dm and the caller process_ai_query_async to locate and
update the code paths.
- Around line 31-33: The current code returns formatted blocks directly from
_format_response_blocks(text) without treating outputs that normalize to empty
as "no answer"; call format_ai_response_for_slack(text) first, assign to a
normalized_text variable, strip surrounding whitespace and code-fence markers
(as format_ai_response_for_slack does), then if normalized_text is empty treat
it as no-answer (trigger the 🤷 fallback) instead of returning blank blocks;
update _format_response_blocks to accept/return only when normalized_text is
non-empty and apply the same normalized_text check inside process_ai_query_async
(and the other similar flows referenced) so any model output that becomes empty
after normalization is handled as "no answer".
- Around line 36-47: The helper _slack_files_to_image_uris downloads and
base64-encodes entire Slack files in memory; to prevent worker OOM during
process_ai_query(), add a strict size guard: introduce a constant like
MAX_SLACK_IMAGE_BYTES and for each file use download_file in streaming/chunked
mode (or check Content-Length before fetching) to abort if the file exceeds the
limit, log/skipping oversized files, and only base64-encode files that completed
within the size cap; update references to download_file usage in
_slack_files_to_image_uris to support a max_bytes parameter or streaming API and
ensure the function still returns uris or None.
In `@backend/tests/apps/slack/blocks_test.py`:
- Around line 52-58: The test test_markdown_blocks_max_blocks_truncation_notice
currently only checks for the "truncated" notice; update it to also assert the
truncated block's text length respects Slack's 3000-character cap by adding an
assertion that len(blocks[-1]["text"]["text"]) <= 3000 (or equivalent
safe-length check) after calling markdown_blocks; this ensures the truncation
logic in markdown_blocks (see the truncation code path around the truncation
logic) prevents overflow.
🪄 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: CHILL
Plan: Pro
Run ID: b9a9b85f-0905-4e7c-8b45-2d03f59109ef
📒 Files selected for processing (10)
backend/apps/slack/blocks.pybackend/apps/slack/commands/ai.pybackend/apps/slack/common/handlers/ai.pybackend/apps/slack/events/app_mention.pybackend/apps/slack/events/message_posted.pybackend/apps/slack/services/message_auto_reply.pybackend/apps/slack/utils.pybackend/tests/apps/slack/blocks_test.pybackend/tests/apps/slack/common/handlers/ai_test.pybackend/tests/apps/slack/services/message_auto_reply_test.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/slack/events/message_posted.py
- backend/apps/slack/events/app_mention.py
rajnisk
changed the title
- Enqueue process_ai_query_async for app mentions and /ai; reactions and worker-side images - Split mrkdwn into Slack-sized blocks; truncate fallback text; format pipeline guards - message_posted: mention boundary and conversation fetch hardening - Update Slack tests (events, commands, auto-reply, blocks, utils, handlers) Made-with: Cursor
|
Contributor
There was a problem hiding this comment.
2 issues found across 13 files (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/tests/apps/slack/events/app_mention_test.py">
<violation number="1" location="backend/tests/apps/slack/events/app_mention_test.py:19">
P2: Assertion helper uses `kwargs.get()` so expected `None` values don’t verify key presence, allowing missing enqueue kwargs to go undetected.</violation>
</file>
<file name="backend/apps/slack/services/message_auto_reply.py">
<violation number="1" location="backend/apps/slack/services/message_auto_reply.py:98">
P2: Missing/invalid Slack file size metadata is treated as 0, allowing oversized images to be fully downloaded before rejection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| args, kwargs = mock_queue.enqueue.call_args | ||
| assert args[0] is process_ai_query_async | ||
| for key, value in expected.items(): | ||
| assert kwargs.get(key) == value |
Contributor
There was a problem hiding this comment.
P2: Assertion helper uses kwargs.get() so expected None values don’t verify key presence, allowing missing enqueue kwargs to go undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/apps/slack/events/app_mention_test.py, line 19:
<comment>Assertion helper uses `kwargs.get()` so expected `None` values don’t verify key presence, allowing missing enqueue kwargs to go undetected.</comment>
<file context>
@@ -3,10 +3,20 @@
+ args, kwargs = mock_queue.enqueue.call_args
+ assert args[0] is process_ai_query_async
+ for key, value in expected.items():
+ assert kwargs.get(key) == value
</file context>
Suggested change
| assert kwargs.get(key) == value | |
| assert key in kwargs | |
| assert kwargs[key] == value |
| try: | ||
| size = int(file.get("size") or 0) | ||
| except (TypeError, ValueError): | ||
| size = 0 |
Contributor
There was a problem hiding this comment.
P2: Missing/invalid Slack file size metadata is treated as 0, allowing oversized images to be fully downloaded before rejection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/slack/services/message_auto_reply.py, line 98:
<comment>Missing/invalid Slack file size metadata is treated as 0, allowing oversized images to be fully downloaded before rejection.</comment>
<file context>
@@ -28,20 +31,86 @@
+ try:
+ size = int(file.get("size") or 0)
+ except (TypeError, ValueError):
+ size = 0
+ if not url or not mime.startswith("image/"):
+ continue
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/apps/slack/services/message_auto_reply.py (2)
264-280: Consider using distinct error messages for ValueError vs SlackApiError.Currently, both exceptions post
ERROR_POSTING_RESPONSE, but aValueErrorfromprocess_ai_queryis a processing error, not a posting failure. Consider catching them separately:♻️ Suggested improvement
- except (ValueError, SlackApiError): + except SlackApiError: logger.exception( "Error posting AI response", extra={"channel_id": channel_id, "message_ts": message_ts}, ) with contextlib.suppress(SlackApiError): client.chat_postMessage( channel=channel_id, text=ERROR_POSTING_RESPONSE, thread_ts=message_ts, ) with contextlib.suppress(SlackApiError): client.reactions_remove( channel=channel_id, timestamp=message_ts, name="eyes", ) + except ValueError: + logger.exception( + "Error processing AI query", + extra={"channel_id": channel_id, "message_ts": message_ts}, + ) + with contextlib.suppress(SlackApiError): + client.chat_postMessage( + channel=channel_id, + text=ERROR_UNEXPECTED_PROCESSING, + thread_ts=message_ts, + ) + with contextlib.suppress(SlackApiError): + client.reactions_remove( + channel=channel_id, + timestamp=message_ts, + name="eyes", + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/slack/services/message_auto_reply.py` around lines 264 - 280, Split the combined except block into two distinct handlers: one catching ValueError (raised by process_ai_query) and one catching SlackApiError (raised by client.chat_postMessage or client.reactions_remove). For the ValueError handler, log the exception with context (channel_id, message_ts) and send a different user-facing message (e.g., ERROR_PROCESSING_RESPONSE) via client.chat_postMessage in the thread; for the SlackApiError handler, keep the current logger.exception and continue to send ERROR_POSTING_RESPONSE and attempt reactions_remove inside contextlib.suppress(SlackApiError). Update the exception logging messages to reflect the specific failure (processing vs posting) and reference process_ai_query, client.chat_postMessage, client.reactions_remove, ERROR_POSTING_RESPONSE and the new ERROR_PROCESSING_RESPONSE constant/name.
324-326: Document precedence when bothimagesandslack_image_filesare provided.If both parameters are supplied,
imagesis silently discarded. While this may not happen in practice (app_mention providesslack_image_files, slash commands provide neither), clarifying the precedence in the docstring would prevent confusion.📝 Suggested docstring update
def process_ai_query_async( ... ): - """Process an AI query asynchronously (app mention or slash command).""" + """Process an AI query asynchronously (app mention or slash command). + + If both `images` (pre-encoded data URIs) and `slack_image_files` (metadata requiring + download) are provided, `slack_image_files` takes precedence and `images` is ignored. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/slack/services/message_auto_reply.py` around lines 324 - 326, The function that accepts the parameters images and slack_image_files currently sets image_uris = images and then overrides it when slack_image_files is present (via _slack_files_to_image_uris), silently discarding images; update the function's docstring to explicitly state the precedence: when both images and slack_image_files are provided, slack_image_files wins and images will be ignored (and that image_uris will be derived from slack_image_files via _slack_files_to_image_uris). Mention the parameter names images and slack_image_files and the image_uris behavior so callers understand the override.backend/apps/slack/events/message_posted.py (1)
21-21: Consider adding TTL or eviction to the bot user ID cache.The class-level
_bot_user_id_by_teamcache persists for the process lifetime. Per the Workspace model context, there's no signal-based invalidation when bot tokens rotate or workspaces change. While bot user IDs rarely change and process restarts clear this, a TTL-based approach (e.g., using@lru_cachewithmaxsizeor a timestamped dict) would be more resilient for long-running workers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/slack/events/message_posted.py` at line 21, The module-level cache _bot_user_id_by_team currently lives for the process lifetime; replace it with an eviction-capable cache to avoid stale bot IDs (e.g., use functools.lru_cache with a bounded maxsize or a TTL cache such as cachetools.TTLCache) or implement per-entry timestamps and expire entries after a short TTL; update the code paths that read/write _bot_user_id_by_team (the functions that fetch and store bot user IDs in this module) to use the new cache API so entries are evicted/refreshed when tokens rotate or after the TTL elapses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/slack/common/handlers/ai.py`:
- Around line 51-63: The current gate inspects str(result) but returns the
original result, allowing wrapped sentinels or non-str types to slip through;
change the logic in this block (variables: result, text, stripped, channel_id,
logger) to normalize to a string up-front (e.g., coerce result to str, then
strip and casefold for checks) and if it passes, return the normalized stripped
string instead of the original result (or return None when empty or a bare
"yes"/"no"); ensure all subsequent code receives a plain str not the original
object.
In `@backend/apps/slack/utils.py`:
- Around line 101-113: The fence-stripping regexes in utils.py currently use
[\w]* and thus leave non-word language tags (e.g., "c++", "objective-c",
"python3.12") behind; update the logic that handles text starting/ending with
triple backticks and the code_block_pattern so the info string consumes all
characters up to the line break (no backticks or newlines). Replace the patterns
used when trimming the opening fence (currently in the if text.startswith("```")
and text.endswith("```") branch) and the code_block_pattern (used with
replace_code_block) to use a pattern like [^`\r\n]*(?:\r?\n)? for the opening
fence and (?:\r?\n) before the capture group so the full info string is removed
for functions/variables text, code_block_pattern, and replace_code_block.
---
Nitpick comments:
In `@backend/apps/slack/events/message_posted.py`:
- Line 21: The module-level cache _bot_user_id_by_team currently lives for the
process lifetime; replace it with an eviction-capable cache to avoid stale bot
IDs (e.g., use functools.lru_cache with a bounded maxsize or a TTL cache such as
cachetools.TTLCache) or implement per-entry timestamps and expire entries after
a short TTL; update the code paths that read/write _bot_user_id_by_team (the
functions that fetch and store bot user IDs in this module) to use the new cache
API so entries are evicted/refreshed when tokens rotate or after the TTL
elapses.
In `@backend/apps/slack/services/message_auto_reply.py`:
- Around line 264-280: Split the combined except block into two distinct
handlers: one catching ValueError (raised by process_ai_query) and one catching
SlackApiError (raised by client.chat_postMessage or client.reactions_remove).
For the ValueError handler, log the exception with context (channel_id,
message_ts) and send a different user-facing message (e.g.,
ERROR_PROCESSING_RESPONSE) via client.chat_postMessage in the thread; for the
SlackApiError handler, keep the current logger.exception and continue to send
ERROR_POSTING_RESPONSE and attempt reactions_remove inside
contextlib.suppress(SlackApiError). Update the exception logging messages to
reflect the specific failure (processing vs posting) and reference
process_ai_query, client.chat_postMessage, client.reactions_remove,
ERROR_POSTING_RESPONSE and the new ERROR_PROCESSING_RESPONSE constant/name.
- Around line 324-326: The function that accepts the parameters images and
slack_image_files currently sets image_uris = images and then overrides it when
slack_image_files is present (via _slack_files_to_image_uris), silently
discarding images; update the function's docstring to explicitly state the
precedence: when both images and slack_image_files are provided,
slack_image_files wins and images will be ignored (and that image_uris will be
derived from slack_image_files via _slack_files_to_image_uris). Mention the
parameter names images and slack_image_files and the image_uris behavior so
callers understand the override.
🪄 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: CHILL
Plan: Pro
Run ID: 63f38e4b-7b55-4a6b-971d-48b5aa17d416
📒 Files selected for processing (13)
backend/apps/slack/blocks.pybackend/apps/slack/common/handlers/ai.pybackend/apps/slack/events/app_mention.pybackend/apps/slack/events/message_posted.pybackend/apps/slack/services/message_auto_reply.pybackend/apps/slack/utils.pybackend/tests/apps/slack/blocks_test.pybackend/tests/apps/slack/commands/ai_test.pybackend/tests/apps/slack/common/handlers/ai_test.pybackend/tests/apps/slack/events/app_mention_test.pybackend/tests/apps/slack/events/message_posted_test.pybackend/tests/apps/slack/services/message_auto_reply_test.pybackend/tests/apps/slack/utils_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/apps/slack/blocks_test.py
- backend/apps/slack/events/app_mention.py
- backend/tests/apps/slack/services/message_auto_reply_test.py
Comment on lines
+51
to
+63
| if result is None: | ||
| return None | ||
| text = str(result) | ||
| stripped = text.strip() | ||
| if not stripped: | ||
| return None | ||
| if stripped.casefold() in {"yes", "no"}: | ||
| logger.info( | ||
| "Ignoring bare YES/NO pipeline output", | ||
| extra={"channel_id": channel_id}, | ||
| ) | ||
| ] | ||
| return None | ||
| return result |
Contributor
There was a problem hiding this comment.
Base the no-answer gate on normalized text, and don’t return raw result.
Right now this branch classifies str(result) but returns the original object. That lets wrapped sentinels like `YES` / NO slip through, and it also means a non-str pipeline value can still leak into the later string-only Slack path.
Proposed fix
- text = str(result)
- stripped = text.strip()
- if not stripped:
+ text = str(result)
+ normalized = format_ai_response_for_slack(text).strip()
+ if not normalized:
return None
- if stripped.casefold() in {"yes", "no"}:
+ if normalized.casefold() in {"yes", "no"}:
logger.info(
"Ignoring bare YES/NO pipeline output",
extra={"channel_id": channel_id},
)
return None
- return result
+ return text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/apps/slack/common/handlers/ai.py` around lines 51 - 63, The current
gate inspects str(result) but returns the original result, allowing wrapped
sentinels or non-str types to slip through; change the logic in this block
(variables: result, text, stripped, channel_id, logger) to normalize to a string
up-front (e.g., coerce result to str, then strip and casefold for checks) and if
it passes, return the normalized stripped string instead of the original result
(or return None when empty or a bare "yes"/"no"); ensure all subsequent code
receives a plain str not the original object.
Comment on lines
101
to
+113
| if text.startswith("```") and text.endswith("```"): | ||
| # Extract content from code block wrapper | ||
| # Remove first ``` and optional language identifier | ||
| text = re.sub(r"^```[\w]*\n?", "", text, count=1) | ||
| # Remove trailing ``` | ||
| text = re.sub(r"\n?```$", "", text) | ||
| text = text.strip() | ||
|
|
||
| # Remove markdown code blocks (```language\ncode\n```) and convert to plain text | ||
| # This regex matches code blocks with optional language identifier | ||
| # Pattern: ```optional_lang\ncontent\n``` | ||
| code_block_pattern = re.compile(r"```[\w]*\n(.*?)```", re.DOTALL) | ||
|
|
||
| def replace_code_block(match): | ||
| # Convert code block content to plain text | ||
| # This prevents Slack from rendering it as a code block | ||
| # Preserve Slack channel links that might be inside code blocks | ||
| return match.group(1).strip() | ||
|
|
||
| text = code_block_pattern.sub(replace_code_block, text) | ||
|
|
||
| # Remove any remaining triple backticks that might have been missed | ||
| # (handles edge cases where regex didn't match) | ||
| # Also handle cases where backticks are on separate lines | ||
| text = re.sub(r"```+", "", text) | ||
|
|
||
| # Remove single backticks that might wrap inline code | ||
| # But preserve Slack channel/user links (format: <#...|...> or <@...|...>) | ||
| # Pattern: `text` but not part of Slack link syntax | ||
| text = re.sub(r"`([^`<]+)`", r"\1", text) | ||
|
|
||
| # Preserve Slack channel links (format: <#channel_id|channel_name>) | ||
| # These should not be modified by format_links_for_slack | ||
| # Convert markdown links to Slack format (but preserve existing Slack links) | ||
| return format_links_for_slack(text) | ||
|
|
||
|
|
||
| # Import get_news_data and get_staff_data from owasp utils | ||
| return re.sub(r"`([^`<]+)`", r"\1", text) |
Contributor
There was a problem hiding this comment.
Fence stripping should consume the full info string, not just \w.
LLM outputs frequently use fence labels like c++, objective-c, or python3.12. With [\w]*, those cases leave the language tag behind, so an “empty” fenced response can end up posting c++/objective-c instead of falling back.
Proposed fix
- if text.startswith("```") and text.endswith("```"):
- text = re.sub(r"^```[\w]*\n?", "", text, count=1)
- text = re.sub(r"\n?```$", "", text)
+ if text.startswith("```") and text.endswith("```"):
+ text = re.sub(r"^```[^`\r\n]*(?:\r?\n)?", "", text, count=1)
+ text = re.sub(r"(?:\r?\n)?```$", "", text)
text = text.strip()
- code_block_pattern = re.compile(r"```[\w]*\n(.*?)```", re.DOTALL)
+ code_block_pattern = re.compile(r"```[^`\r\n]*(?:\r?\n)(.*?)```", re.DOTALL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/apps/slack/utils.py` around lines 101 - 113, The fence-stripping
regexes in utils.py currently use [\w]* and thus leave non-word language tags
(e.g., "c++", "objective-c", "python3.12") behind; update the logic that handles
text starting/ending with triple backticks and the code_block_pattern so the
info string consumes all characters up to the line break (no backticks or
newlines). Replace the patterns used when trimming the opening fence (currently
in the if text.startswith("```") and text.endswith("```") branch) and the
code_block_pattern (used with replace_code_block) to use a pattern like
[^`\r\n]*(?:\r?\n)? for the opening fence and (?:\r?\n) before the capture group
so the full info string is removed for functions/variables text,
code_block_pattern, and replace_code_block.
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
1 participant
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
Base:
feature/nestbot-ai-assistantResolves: #3693
Delivers NestBot Slack interaction UX (async work, clear event boundaries, reaction / “Thinking…” lifecycle) and posting / formatting hardening so long answers and edge-case model output don’t break
chat.postMessageor double-invoke the AI on deferred replies.Interaction & async UX
/ai: enqueueprocess_ai_query_asyncon theaiqueue; empty/aigets an ephemeral usage hint; enqueue failure surfaces an error and cleans up reactions where applicable.message_postedskips messages where the bot is @mentioned soapp_mentionowns mentions (avoids duplicate handling).finally./aireplies: DM viaconversations_open+chat_postMessage(private); failures fall back to ephemeral error when DM post fails.slack_image_filesmetadata into the worker; download / base64 in the worker (with sensible file count / size limits).Slack posting & deferred auto-reply
get_blocks/pipeline again on the answer).markdown_blocks: split mrkdwn so each section stays under Slack’s 3000-character limit; cap blocks per message with a safe truncation path.chat.postMessagetext: fallback / notification text uses formatted body +truncate_for_slack_fallback(aligned with blocks, within API limits).format_ai_response_for_slack: normalizes fences / backticks only; markdown links converted once when building blocks (no doubleformat_links_for_slack).yes/nopipeline output; treat whitespace-only / empty-after-format as no-answer (🤷 + user message / error blocks where appropriate).Tests
message_auto_reply/aihandler flows, YES/NO and empty-format cases.Related
Checklist
make check-testlocally: all warnings addressed, tests passed