Skip to content

fix(slack): route bot mentions from legacy attachments - #86225

Open
JoshSnider wants to merge 1 commit into
NousResearch:mainfrom
withVariable:fix/slack-attachment-mentions
Open

fix(slack): route bot mentions from legacy attachments#86225
JoshSnider wants to merge 1 commit into
NousResearch:mainfrom
withVariable:fix/slack-attachment-mentions

Conversation

@JoshSnider

Copy link
Copy Markdown

Summary

  • recognize explicit Slack user mentions authored inside legacy message attachments
  • enable attachment mention routing only after the sender is independently classified as a bot/app
  • keep human attachments and message unfurls inert

Problem

Apps such as Datadog and PagerDuty post an empty top-level text field and place the actual notification inside legacy Slack attachments. Hermes already renders that content for the agent, but allow_bots: mentions checks only top-level text and Block Kit. An explicit mention inside a bot-authored attachment is therefore dropped before routing.

Security boundary

Attachment mentions are opt-in to _slack_mention_detection_text and are enabled only on the verified bot/app path. Fallback and message-unfurl attachment content are excluded, so a human-shared preview cannot manufacture a routing mention.

Validation

scripts/run_tests.sh tests/gateway/test_slack_mention.py tests/gateway/test_slack.py -q

196 tests passed.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 14, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #75312 is the broader open Block Kit and legacy-attachment mention-detection implementation. This PR is a focused bot-authored attachment-routing variant; the two should be reviewed together rather than treated as duplicates.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(slack): route bot mentions from legacy attachments

  1. _SLACK_USER_MENTION_TOKEN_RE = r"<@[A-Za-z0-9_-]+(?:\|[^>\n]*)?>" truncates at the first >: a label containing a nested > (e.g. <@U123|foo <@U456>>) would mis-match. Slack labels rarely contain >, so risk is low — but a short comment stating the assumption (and that the token regex is shared by block + attachment scanning) would help future readers.
  2. The routing gate is correctly scoped: attachment mentions only count when sender_is_bot (routing path) or inside the allow_bots == "mentions" branch. Skipping is_msg_unfurl attachments and fallback text prevents a human-shared link preview from manufacturing a mention — good security posture.
  3. _collect_slack_attachment_mentions also descends into attachment blocks via _collect_slack_block_mentions. Worth a quick double-check that quoted/forwarded mentions inside attachment blocks stay inert the same way top-level quoted Block Kit mentions do ([Bug]: Slack mention detection (allow_bots=mentions, is_mentioned, require_mention) only inspects flat top-level text, so @mentions present only in Block Kit blocks are silently dropped #52387 behavior).
  4. Dedup uses an O(n²) in list scan per attachment — negligible at Slack mention counts; fine as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants