From d693cc8e90bd24ca0582a8e8d1f32369d5cf8cdb Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:48:02 +0000 Subject: [PATCH 1/3] Resolve @handle owner/subscriber mentions in Slack web integration Co-Authored-By: Noya Offer --- .../messaging_integrations/slack_web.py | 46 +++++++- .../messaging_integrations/test_slack_web.py | 103 ++++++++++++++++++ 2 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 tests/unit/messages/messaging_integrations/test_slack_web.py diff --git a/elementary/messages/messaging_integrations/slack_web.py b/elementary/messages/messaging_integrations/slack_web.py index f83b2ae51..b122e4a4d 100644 --- a/elementary/messages/messaging_integrations/slack_web.py +++ b/elementary/messages/messaging_integrations/slack_web.py @@ -51,6 +51,7 @@ def __init__( self.client = client self.tracking = tracking self._email_to_user_id_cache: Dict[str, str] = {} + self._handle_to_user_id_cache: Optional[Dict[str, str]] = None self.reply_broadcast = reply_broadcast @classmethod @@ -77,7 +78,7 @@ def supports_actions(self) -> bool: def send_message( self, destination: Channel, body: MessageBody ) -> MessageSendResult[SlackWebMessageContext]: - formatted_message = format_block_kit(body, self.get_user_id_from_email) + formatted_message = format_block_kit(body, self.resolve_user_id) return self._send_message(destination, formatted_message) def reply_to_message( @@ -86,7 +87,7 @@ def reply_to_message( message_context: SlackWebMessageContext, body: MessageBody, ) -> MessageSendResult[SlackWebMessageContext]: - formatted_message = format_block_kit(body, self.get_user_id_from_email) + formatted_message = format_block_kit(body, self.resolve_user_id) return self._send_message( destination, formatted_message, @@ -187,6 +188,47 @@ def _join_channel(self, channel_id: str) -> None: self.tracking.record_internal_exception(e) raise MessagingIntegrationError(f"Failed to join channel {channel_id}") + def resolve_user_id(self, user: str) -> Optional[str]: + if user.startswith("@"): + return self.get_user_id_from_handle(user[1:]) + return self.get_user_id_from_email(user) + + def get_user_id_from_handle(self, handle: str) -> Optional[str]: + if self._handle_to_user_id_cache is None: + self._handle_to_user_id_cache = self._build_handle_to_user_id_map() + return self._handle_to_user_id_cache.get(handle.lower()) + + def _build_handle_to_user_id_map(self) -> Dict[str, str]: + email_prefix_to_user_id: Dict[str, str] = {} + username_to_user_id: Dict[str, str] = {} + try: + for user in self._iter_users(): + if user.get("deleted") or user.get("is_bot"): + continue + email = (user.get("profile") or {}).get("email") + if email: + email_prefix = email.split("@")[0].lower() + email_prefix_to_user_id.setdefault(email_prefix, user["id"]) + if user.get("name"): + username_to_user_id.setdefault(user["name"].lower(), user["id"]) + except SlackApiError as err: + if self.tracking: + self.tracking.record_internal_exception(err) + logger.error(f"Unable to list Slack users: {err}.") + return {**username_to_user_id, **email_prefix_to_user_id} + + @sleep_and_retry + @limits(calls=20, period=ONE_MINUTE) + def _iter_users(self, cursor: Optional[str] = None) -> Iterator[dict]: + response = self.client.users_list(cursor=cursor, limit=200) + yield from response["members"] + response_metadata = response.get("response_metadata") or {} + next_cursor = response_metadata.get("next_cursor") + if next_cursor: + if not isinstance(next_cursor, str): + raise ValueError("Next cursor is not a string") + yield from self._iter_users(next_cursor) + @sleep_and_retry @limits(calls=50, period=ONE_MINUTE) def get_user_id_from_email(self, email: str) -> Optional[str]: diff --git a/tests/unit/messages/messaging_integrations/test_slack_web.py b/tests/unit/messages/messaging_integrations/test_slack_web.py new file mode 100644 index 000000000..e341e36fc --- /dev/null +++ b/tests/unit/messages/messaging_integrations/test_slack_web.py @@ -0,0 +1,103 @@ +from unittest.mock import MagicMock + +from slack_sdk.errors import SlackApiError + +from elementary.messages.blocks import LineBlock, LinesBlock, MentionBlock +from elementary.messages.message_body import MessageBody +from elementary.messages.messaging_integrations.slack_web import ( + SlackWebMessagingIntegration, +) + +USERS_PAGE_1 = { + "members": [ + { + "id": "U_JESSICA", + "name": "jjones", + "profile": {"email": "jessica.jones@marvel.com"}, + }, + {"id": "U_BOT", "name": "bot", "is_bot": True, "profile": {}}, + ], + "response_metadata": {"next_cursor": "page2"}, +} +USERS_PAGE_2 = { + "members": [ + {"id": "U_LUKE", "name": "luke", "profile": {"email": "lcage@marvel.com"}}, + { + "id": "U_DELETED", + "name": "deleted", + "deleted": True, + "profile": {"email": "deleted@marvel.com"}, + }, + ], + "response_metadata": {"next_cursor": ""}, +} + + +def _build_integration() -> SlackWebMessagingIntegration: + client = MagicMock() + client.users_list.side_effect = lambda cursor=None, limit=None: ( + USERS_PAGE_2 if cursor == "page2" else USERS_PAGE_1 + ) + client.users_lookupByEmail.side_effect = lambda email: { + "user": {"id": f"U_EMAIL_{email}"} + } + return SlackWebMessagingIntegration(client) + + +def test_resolve_user_id_by_email_prefix_handle(): + integration = _build_integration() + assert integration.resolve_user_id("@jessica.jones") == "U_JESSICA" + assert integration.resolve_user_id("@Jessica.Jones") == "U_JESSICA" + + +def test_resolve_user_id_by_username_handle(): + integration = _build_integration() + assert integration.resolve_user_id("@jjones") == "U_JESSICA" + assert integration.resolve_user_id("@luke") == "U_LUKE" + + +def test_resolve_user_id_skips_bots_deleted_and_unknown_handles(): + integration = _build_integration() + assert integration.resolve_user_id("@bot") is None + assert integration.resolve_user_id("@deleted") is None + assert integration.resolve_user_id("@unknown") is None + + +def test_users_are_listed_once(): + integration = _build_integration() + integration.resolve_user_id("@jessica.jones") + integration.resolve_user_id("@luke") + assert integration.client.users_list.call_count == 2 + + +def test_resolve_user_id_by_email(): + integration = _build_integration() + assert ( + integration.resolve_user_id("jessica.jones@marvel.com") + == "U_EMAIL_jessica.jones@marvel.com" + ) + integration.client.users_list.assert_not_called() + + +def test_resolve_user_id_when_users_list_fails(): + integration = _build_integration() + integration.client.users_list.side_effect = SlackApiError( + "error", MagicMock(data={"error": "missing_scope"}) + ) + assert integration.resolve_user_id("@jessica.jones") is None + + +def test_send_message_resolves_handle_mentions(): + integration = _build_integration() + integration.client.chat_postMessage.return_value = { + "ts": "123.456", + "channel": "C1", + } + body = MessageBody( + blocks=[ + LinesBlock(lines=[LineBlock(inlines=[MentionBlock(user="@jessica.jones")])]) + ] + ) + integration.send_message("C1", body) + sent = integration.client.chat_postMessage.call_args.kwargs + assert "<@U_JESSICA>" in sent["blocks"] + sent["attachments"] From 5e94b1dca83442dcf432a8c8d352e426482f5d71 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:56:52 +0000 Subject: [PATCH 2/3] Stop Slack user pagination when the cursor does not advance Co-Authored-By: Noya Offer --- .../messages/messaging_integrations/slack_web.py | 4 +++- .../messages/messaging_integrations/test_slack_web.py | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/elementary/messages/messaging_integrations/slack_web.py b/elementary/messages/messaging_integrations/slack_web.py index b122e4a4d..9f19aa5b9 100644 --- a/elementary/messages/messaging_integrations/slack_web.py +++ b/elementary/messages/messaging_integrations/slack_web.py @@ -211,7 +211,7 @@ def _build_handle_to_user_id_map(self) -> Dict[str, str]: email_prefix_to_user_id.setdefault(email_prefix, user["id"]) if user.get("name"): username_to_user_id.setdefault(user["name"].lower(), user["id"]) - except SlackApiError as err: + except (SlackApiError, ValueError) as err: if self.tracking: self.tracking.record_internal_exception(err) logger.error(f"Unable to list Slack users: {err}.") @@ -227,6 +227,8 @@ def _iter_users(self, cursor: Optional[str] = None) -> Iterator[dict]: if next_cursor: if not isinstance(next_cursor, str): raise ValueError("Next cursor is not a string") + if next_cursor == cursor: + raise ValueError("Next cursor did not advance") yield from self._iter_users(next_cursor) @sleep_and_retry diff --git a/tests/unit/messages/messaging_integrations/test_slack_web.py b/tests/unit/messages/messaging_integrations/test_slack_web.py index e341e36fc..cc487a4f3 100644 --- a/tests/unit/messages/messaging_integrations/test_slack_web.py +++ b/tests/unit/messages/messaging_integrations/test_slack_web.py @@ -101,3 +101,13 @@ def test_send_message_resolves_handle_mentions(): integration.send_message("C1", body) sent = integration.client.chat_postMessage.call_args.kwargs assert "<@U_JESSICA>" in sent["blocks"] + sent["attachments"] + + +def test_resolve_user_id_when_users_list_cursor_does_not_advance(): + integration = _build_integration() + integration.client.users_list.side_effect = lambda cursor=None, limit=None: { + "members": USERS_PAGE_1["members"], + "response_metadata": {"next_cursor": "stuck"}, + } + assert integration.resolve_user_id("@jessica.jones") == "U_JESSICA" + assert integration.client.users_list.call_count == 2 From cd29c7c590bc1ce37c083ffd43f8202a2923c50a Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:13:15 +0000 Subject: [PATCH 3/3] Harden Slack handle resolution against non-Slack errors and guest collisions Co-Authored-By: Noya Offer --- .../messaging_integrations/slack_web.py | 59 +++++++++++------ .../messaging_integrations/test_slack_web.py | 65 ++++++++++++++++--- 2 files changed, 96 insertions(+), 28 deletions(-) diff --git a/elementary/messages/messaging_integrations/slack_web.py b/elementary/messages/messaging_integrations/slack_web.py index 9f19aa5b9..99c2bf723 100644 --- a/elementary/messages/messaging_integrations/slack_web.py +++ b/elementary/messages/messaging_integrations/slack_web.py @@ -1,7 +1,7 @@ import json import ssl import time -from typing import Any, Dict, Iterator, Optional +from typing import Any, Dict, Iterator, List, Optional from ratelimit import limits, sleep_and_retry from slack_sdk import WebClient @@ -52,6 +52,9 @@ def __init__( self.tracking = tracking self._email_to_user_id_cache: Dict[str, str] = {} self._handle_to_user_id_cache: Optional[Dict[str, str]] = None + self._list_users_page = sleep_and_retry( + limits(calls=20, period=ONE_MINUTE)(self._request_users_page) + ) self.reply_broadcast = reply_broadcast @classmethod @@ -199,37 +202,55 @@ def get_user_id_from_handle(self, handle: str) -> Optional[str]: return self._handle_to_user_id_cache.get(handle.lower()) def _build_handle_to_user_id_map(self) -> Dict[str, str]: - email_prefix_to_user_id: Dict[str, str] = {} - username_to_user_id: Dict[str, str] = {} + members: List[dict] = [] + guests: List[dict] = [] + # Never block an alert on mention resolution. On failure, the users listed + # so far are still cached so a failing workspace isn't re-crawled per mention. try: for user in self._iter_users(): if user.get("deleted") or user.get("is_bot"): continue - email = (user.get("profile") or {}).get("email") - if email: - email_prefix = email.split("@")[0].lower() - email_prefix_to_user_id.setdefault(email_prefix, user["id"]) - if user.get("name"): - username_to_user_id.setdefault(user["name"].lower(), user["id"]) - except (SlackApiError, ValueError) as err: + if user.get("is_restricted") or user.get("is_ultra_restricted"): + guests.append(user) + else: + members.append(user) + except Exception as err: if self.tracking: self.tracking.record_internal_exception(err) logger.error(f"Unable to list Slack users: {err}.") + + email_prefix_to_user_id: Dict[str, str] = {} + username_to_user_id: Dict[str, str] = {} + for user in members + guests: + email = (user.get("profile") or {}).get("email") + if email: + email_prefix = email.split("@")[0].lower() + email_prefix_to_user_id.setdefault(email_prefix, user["id"]) + if user.get("name"): + username_to_user_id.setdefault(user["name"].lower(), user["id"]) + if (members or guests) and not email_prefix_to_user_id: + logger.warning( + "No Slack user emails are visible, so @ mentions can't " + "be resolved. Make sure the Slack app has the users:read.email scope." + ) return {**username_to_user_id, **email_prefix_to_user_id} - @sleep_and_retry - @limits(calls=20, period=ONE_MINUTE) - def _iter_users(self, cursor: Optional[str] = None) -> Iterator[dict]: - response = self.client.users_list(cursor=cursor, limit=200) - yield from response["members"] - response_metadata = response.get("response_metadata") or {} - next_cursor = response_metadata.get("next_cursor") - if next_cursor: + def _iter_users(self) -> Iterator[dict]: + cursor: Optional[str] = None + while True: + response = self._list_users_page(cursor) + yield from response["members"] + next_cursor = (response.get("response_metadata") or {}).get("next_cursor") + if not next_cursor: + return if not isinstance(next_cursor, str): raise ValueError("Next cursor is not a string") if next_cursor == cursor: raise ValueError("Next cursor did not advance") - yield from self._iter_users(next_cursor) + cursor = next_cursor + + def _request_users_page(self, cursor: Optional[str]) -> Any: + return self.client.users_list(cursor=cursor, limit=200) @sleep_and_retry @limits(calls=50, period=ONE_MINUTE) diff --git a/tests/unit/messages/messaging_integrations/test_slack_web.py b/tests/unit/messages/messaging_integrations/test_slack_web.py index cc487a4f3..62449ef2f 100644 --- a/tests/unit/messages/messaging_integrations/test_slack_web.py +++ b/tests/unit/messages/messaging_integrations/test_slack_web.py @@ -1,10 +1,12 @@ from unittest.mock import MagicMock +import pytest from slack_sdk.errors import SlackApiError from elementary.messages.blocks import LineBlock, LinesBlock, MentionBlock from elementary.messages.message_body import MessageBody from elementary.messages.messaging_integrations.slack_web import ( + SlackWebMessageContext, SlackWebMessagingIntegration, ) @@ -87,20 +89,65 @@ def test_resolve_user_id_when_users_list_fails(): assert integration.resolve_user_id("@jessica.jones") is None -def test_send_message_resolves_handle_mentions(): - integration = _build_integration() +HANDLE_MENTION_BODY = MessageBody( + blocks=[ + LinesBlock(lines=[LineBlock(inlines=[MentionBlock(user="@jessica.jones")])]) + ] +) + + +def _send(integration: SlackWebMessagingIntegration, reply: bool) -> str: integration.client.chat_postMessage.return_value = { "ts": "123.456", "channel": "C1", } - body = MessageBody( - blocks=[ - LinesBlock(lines=[LineBlock(inlines=[MentionBlock(user="@jessica.jones")])]) - ] - ) - integration.send_message("C1", body) + if reply: + context = SlackWebMessageContext(id="111.222", channel="C1") + integration.reply_to_message("C1", context, HANDLE_MENTION_BODY) + else: + integration.send_message("C1", HANDLE_MENTION_BODY) sent = integration.client.chat_postMessage.call_args.kwargs - assert "<@U_JESSICA>" in sent["blocks"] + sent["attachments"] + return sent["blocks"] + sent["attachments"] + + +@pytest.mark.parametrize("reply", [False, True]) +def test_send_resolves_handle_mentions(reply): + integration = _build_integration() + assert "<@U_JESSICA>" in _send(integration, reply) + + +@pytest.mark.parametrize("reply", [False, True]) +def test_send_keeps_plain_handle_when_users_list_raises(reply): + integration = _build_integration() + integration.client.users_list.side_effect = ConnectionError("connection reset") + sent = _send(integration, reply) + assert "@jessica.jones" in sent + assert "<@" not in sent + + +def test_resolve_user_id_collisions(): + integration = _build_integration() + integration.client.users_list.side_effect = lambda cursor=None, limit=None: { + "members": [ + { + "id": "U_GUEST", + "name": "john", + "is_restricted": True, + "profile": {"email": "john@partner.com"}, + }, + {"id": "U_JOHN", "name": "jdoe", "profile": {"email": "john@company.com"}}, + {"id": "U_JOHN_2", "name": "jd", "profile": {"email": "john@other.com"}}, + { + "id": "U_JANE", + "name": "jane.doe", + "profile": {"email": "jd@company.com"}, + }, + ], + } + # Full members win over guests, then the first listed user wins. + assert integration.resolve_user_id("@john") == "U_JOHN" + # Email prefix matches win over username matches. + assert integration.resolve_user_id("@jd") == "U_JANE" def test_resolve_user_id_when_users_list_cursor_does_not_advance():