diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index f3eb4f50a..05aeaf56f 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -2761,6 +2761,15 @@ class SlackAdapter(BasePlatformAdapter): if not channel_type and channel_id.startswith("D"): channel_type = "im" is_dm = channel_type in {"im", "mpim"} # Both 1:1 and group DMs + # A 1:1 IM is a private conversation with a single human — mention-exempt + # and safe to react to unconditionally, like any DM. An MPIM (group DM) + # is a SHARED surface: multiple humans can see and trigger the bot, so it + # must obey the same operator controls as a channel (allowed_channels / + # require_mention / strict_mention / free_response_channels) and must not + # get reaction noise on messages that don't address the bot. Only the 1:1 + # case earns the DM exemptions; session/thread scoping below still treats + # both as DM-style persistent conversations. + is_one_to_one_dm = channel_type == "im" # Build thread_ts for session keying. # In channels: fall back to ts so each top-level @mention starts a @@ -2823,7 +2832,7 @@ class SlackAdapter(BasePlatformAdapter): event_thread_ts = event.get("thread_ts") is_thread_reply = bool(event_thread_ts and event_thread_ts != ts) - if not is_dm and bot_uid: + if not is_one_to_one_dm and bot_uid: # Check allowed channels — if set, only respond in these channels (whitelist) allowed_channels = self._slack_allowed_channels() if allowed_channels and channel_id not in allowed_channels: @@ -3216,10 +3225,11 @@ class SlackAdapter(BasePlatformAdapter): auto_skill=_auto_skill, ) - # Only react when bot is directly addressed (DM or @mention). - # In listen-all channels (require_mention=false), reacting to every - # casual message would be noisy. - _should_react = (is_dm or is_mentioned) and self._reactions_enabled() + # Only react when bot is directly addressed (1:1 DM or @mention). + # MPIMs are shared surfaces: reacting to every group-DM message (even + # when unmentioned) is visible noise to the whole group, so they must + # be @mentioned to earn a reaction — same as any channel. + _should_react = (is_one_to_one_dm or is_mentioned) and self._reactions_enabled() if _should_react: self._reacting_message_ids.add(ts) diff --git a/tests/gateway/test_slack_mention.py b/tests/gateway/test_slack_mention.py index c23da97e6..69959ac94 100644 --- a/tests/gateway/test_slack_mention.py +++ b/tests/gateway/test_slack_mention.py @@ -243,12 +243,22 @@ def test_free_response_channels_int_list(): def _would_process(adapter, *, is_dm=False, channel_id=CHANNEL_ID, text="hello", mentioned=False, thread_reply=False, - active_session=False): + active_session=False, channel_type=None): """Simulate the mention gating logic from _handle_slack_message. Returns True if the message would be processed, False if it would be skipped (returned early). + + ``channel_type`` mirrors the real Slack payload ("im" = 1:1 DM, + "mpim" = group DM, "" = channel). When omitted it is derived from the + legacy ``is_dm`` flag as a 1:1 IM, preserving existing callers. Gating + keys off ``is_one_to_one_dm`` (only a true 1:1 IM is exempt); MPIMs are + shared surfaces and go through the same gating as channels. """ + if channel_type is None: + channel_type = "im" if is_dm else "" + is_one_to_one_dm = channel_type == "im" + bot_uid = adapter._team_bot_user_ids.get("T1", adapter._bot_user_id) if mentioned: text = f"<@{bot_uid}> {text}" @@ -257,7 +267,7 @@ def _would_process(adapter, *, is_dm=False, channel_id=CHANNEL_ID, or adapter._slack_message_matches_mention_patterns(text) ) - if not is_dm and bot_uid: + if not is_one_to_one_dm and bot_uid: # allowed_channels check (whitelist — must pass before other gating) allowed = adapter._slack_allowed_channels() if allowed and channel_id not in allowed: @@ -267,6 +277,8 @@ def _would_process(adapter, *, is_dm=False, channel_id=CHANNEL_ID, return True elif not adapter._slack_require_mention(): return True + elif adapter._slack_strict_mention() and not is_mentioned: + return False elif not is_mentioned: if thread_reply and active_session: return True @@ -306,6 +318,67 @@ def test_dm_always_processed_regardless_of_setting(): assert _would_process(adapter, is_dm=True, text="hello") is True +# --------------------------------------------------------------------------- +# Tests: MPIM / group-DM shared-surface gating (regression for the group-DM +# routing bug introduced by PRs #4633 / #54632 / #54663, which classified +# mpim as a DM and thereby exempted it from mention gating + reaction guards). +# --------------------------------------------------------------------------- + +def test_mpim_unmentioned_strict_mention_ignored(): + """MPIM, not mentioned, strict_mention on -> dropped (shared surface).""" + adapter = _make_adapter(require_mention=True, strict_mention=True, + free_response_channels=[]) + assert _would_process(adapter, channel_type="mpim", text="hello") is False + + +def test_mpim_unmentioned_require_mention_ignored(): + """MPIM, not mentioned, require_mention on (non-strict) -> dropped.""" + adapter = _make_adapter(require_mention=True) + assert _would_process(adapter, channel_type="mpim", text="hello") is False + + +def test_mpim_mentioned_processed(): + """MPIM with an @mention is processed like any addressed message.""" + adapter = _make_adapter(require_mention=True, strict_mention=True) + assert _would_process(adapter, channel_type="mpim", mentioned=True, + text="hello") is True + + +def test_mpim_not_in_allowed_channels_dropped(): + """MPIM absent from a non-empty allowed_channels whitelist is dropped, + even when mentioned.""" + adapter = _make_adapter(require_mention=True, allowed_channels=["C_ALLOWED"]) + assert _would_process(adapter, channel_type="mpim", channel_id="C_BLOCKED", + mentioned=True, text="hello") is False + + +def test_mpim_in_free_response_processed_without_mention(): + """An MPIM explicitly listed in free_response_channels still opts in.""" + adapter = _make_adapter(require_mention=True, + free_response_channels=["G_MPIM"]) + assert _would_process(adapter, channel_type="mpim", channel_id="G_MPIM", + text="hello") is True + + +def test_one_to_one_im_still_exempt(): + """1:1 IM behavior is preserved: mention-exempt regardless of settings.""" + adapter = _make_adapter(require_mention=True, strict_mention=True) + assert _would_process(adapter, channel_type="im", text="hello") is True + + +def test_mpim_unmentioned_does_not_react(): + """Reaction guard: only a 1:1 IM or an @mention earns a reaction. An + unmentioned MPIM message must NOT get :eyes:/:white_check_mark: noise.""" + def _should_react(channel_type, is_mentioned): + is_one_to_one_dm = channel_type == "im" + return is_one_to_one_dm or is_mentioned + + assert _should_react("mpim", False) is False # the reported spam case + assert _should_react("mpim", True) is True # addressed -> ok + assert _should_react("im", False) is True # 1:1 DM -> ok + assert _should_react("", False) is False # channel, unmentioned + + def test_mentioned_message_always_processed(): adapter = _make_adapter(require_mention=True) assert _would_process(adapter, mentioned=True, text="what's up") is True diff --git a/website/docs/user-guide/messaging/slack.md b/website/docs/user-guide/messaging/slack.md index fbfdca794..63e8033f3 100644 --- a/website/docs/user-guide/messaging/slack.md +++ b/website/docs/user-guide/messaging/slack.md @@ -412,14 +412,18 @@ Set this to `true` in busy workspaces where Slack's default "the bot remembers t ::: :::info -Slack supports both patterns: `@mention` required to start a conversation by default, but you can opt specific channels out via `SLACK_FREE_RESPONSE_CHANNELS` (comma-separated channel IDs) or `slack.free_response_channels` in `config.yaml`. Once the bot has an active session in a thread, subsequent thread replies do not require a mention. In DMs the bot always responds without needing a mention. +Slack supports both patterns: `@mention` required to start a conversation by default, but you can opt specific channels out via `SLACK_FREE_RESPONSE_CHANNELS` (comma-separated channel IDs) or `slack.free_response_channels` in `config.yaml`. Once the bot has an active session in a thread, subsequent thread replies do not require a mention. In **1:1 DMs** the bot always responds without needing a mention. +::: + +:::caution Group DMs (MPIMs) are shared surfaces, not 1:1 DMs +A **1:1 direct message** is a private conversation with one person, so it is mention-exempt. A **group DM (MPIM / multi-person DM)** is a *shared surface* — multiple people can see and trigger the bot — so it obeys the same operator controls as a channel: `require_mention`, `strict_mention`, `free_response_channels`, and `allowed_channels` all apply, and the bot only adds `:eyes:`/`:white_check_mark:` reactions when it is actually `@mentioned`. To let the bot respond freely in a specific group DM, add its channel ID (starts with `G`) to `free_response_channels`. ::: ### Channel allowlist (`allowed_channels`) Restrict the bot to a fixed set of Slack channels — useful when the bot is invited to many channels but should only respond in a few. When set, messages from channels NOT in this list are **silently ignored**, even if the bot is `@mentioned`. -**DMs are exempt** from this filter, so authorized users can always reach the bot in a direct message. +**1:1 DMs are exempt** from this filter, so authorized users can always reach the bot in a direct message. **Group DMs (MPIMs) are not exempt** — like channels, an MPIM must be on the allowlist (its ID starts with `G`) or its messages are dropped. ```yaml slack: