From 978e1356c05239c84bcebd909ec1f2048d081f65 Mon Sep 17 00:00:00 2001 From: teknium1 Date: Thu, 12 Mar 2026 16:22:39 -0700 Subject: [PATCH] =?UTF-8?q?feat:=20Slack=20adapter=20improvements=20?= =?UTF-8?q?=E2=80=94=20formatting,=20reactions,=20user=20resolution,=20com?= =?UTF-8?q?mands?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Markdown → mrkdwn conversion (format_message override): - **bold** → *bold*, *italic* → _italic_ - ## Headers → *Headers* (bold) - [link](url) → - ~~strike~~ → ~strike~ - Code blocks and inline code preserved unchanged - Placeholder-based approach (same pattern as Telegram) 2. Message length splitting: - send() now calls format_message() + truncate_message() - Long responses split at natural boundaries (newlines, spaces) - Code blocks properly closed/reopened across chunks - Chunk indicators (1/N) appended for multi-part messages 3. Reaction-based acknowledgment: - 👀 (eyes) reaction added on message receipt - Replaced with ✅ (white_check_mark) when response is complete - Graceful error handling (missing scopes, already-reacted) - Serves as visual feedback since Slack has no bot typing API 4. User identity resolution: - Resolves Slack user IDs to display names via users.info API - LRU-style in-memory cache (one API call per user) - Fallback chain: display_name → real_name → user_id - user_name now included in MessageEvent source 5. Expanded slash commands (/hermes ): - Added: compact, compress, resume, background, usage, insights, title, reasoning, provider, rollback - Arguments preserved (e.g. /hermes resume my session) 6. reply_broadcast config option: - When gateway.slack.reply_broadcast is true, first response in a thread also appears in the main channel - Disabled by default — thread = session stays clean 30 new tests covering all features. --- gateway/platforms/slack.py | 211 +++++++++++++++++++++++++-- tests/gateway/test_slack.py | 274 ++++++++++++++++++++++++++++++++++++ 2 files changed, 473 insertions(+), 12 deletions(-) diff --git a/gateway/platforms/slack.py b/gateway/platforms/slack.py index 6052af0a7..314cbbe90 100644 --- a/gateway/platforms/slack.py +++ b/gateway/platforms/slack.py @@ -66,13 +66,14 @@ class SlackAdapter(BasePlatformAdapter): - Typing indicators (not natively supported by Slack bots) """ - MAX_MESSAGE_LENGTH = 4000 # Slack's limit is higher but mrkdwn can inflate + MAX_MESSAGE_LENGTH = 3900 # Slack hard limit is 4000 but leave room for mrkdwn def __init__(self, config: PlatformConfig): super().__init__(config, Platform.SLACK) self._app: Optional[AsyncApp] = None self._handler: Optional[AsyncSocketModeHandler] = None self._bot_user_id: Optional[str] = None + self._user_name_cache: Dict[str, str] = {} # user_id → display name async def connect(self) -> bool: """Connect to Slack via Socket Mode.""" @@ -152,22 +153,36 @@ class SlackAdapter(BasePlatformAdapter): return SendResult(success=False, error="Not connected") try: - kwargs = { - "channel": chat_id, - "text": content, - } + # Convert standard markdown → Slack mrkdwn + formatted = self.format_message(content) + + # Split long messages, preserving code block boundaries + chunks = self.truncate_message(formatted, self.MAX_MESSAGE_LENGTH) - # Reply in thread if thread context is available. thread_ts = self._resolve_thread_ts(reply_to, metadata) - if thread_ts: - kwargs["thread_ts"] = thread_ts + last_result = None - result = await self._app.client.chat_postMessage(**kwargs) + # reply_broadcast: also post thread replies to the main channel. + # Controlled via platform config: gateway.slack.reply_broadcast + broadcast = self.config.extra.get("reply_broadcast", False) + + for i, chunk in enumerate(chunks): + kwargs = { + "channel": chat_id, + "text": chunk, + } + if thread_ts: + kwargs["thread_ts"] = thread_ts + # Only broadcast the first chunk of the first reply + if broadcast and i == 0: + kwargs["reply_broadcast"] = True + + last_result = await self._app.client.chat_postMessage(**kwargs) return SendResult( success=True, - message_id=result.get("ts"), - raw_response=result, + message_id=last_result.get("ts") if last_result else None, + raw_response=last_result, ) except Exception as e: # pragma: no cover - defensive logging @@ -201,7 +216,11 @@ class SlackAdapter(BasePlatformAdapter): return SendResult(success=False, error=str(e)) async def send_typing(self, chat_id: str, metadata=None) -> None: - """Slack doesn't have a direct typing indicator API for bots.""" + """Slack doesn't support typing indicators for bot users. + + The reactions system (👀 on receipt, ✅ on completion) serves as + the visual feedback mechanism instead. + """ pass def _resolve_thread_ts( @@ -221,6 +240,154 @@ class SlackAdapter(BasePlatformAdapter): return metadata["thread_ts"] return reply_to + # ----- Markdown → mrkdwn conversion ----- + + def format_message(self, content: str) -> str: + """Convert standard markdown to Slack mrkdwn format. + + Protected regions (code blocks, inline code) are extracted first so + their contents are never modified. Standard markdown constructs + (headers, bold, italic, links) are translated to mrkdwn syntax. + """ + if not content: + return content + + placeholders: dict = {} + counter = [0] + + def _ph(value: str) -> str: + """Stash value behind a placeholder that survives later passes.""" + key = f"\x00SL{counter[0]}\x00" + counter[0] += 1 + placeholders[key] = value + return key + + text = content + + # 1) Protect fenced code blocks (``` ... ```) + text = re.sub( + r'(```(?:[^\n]*\n)?[\s\S]*?```)', + lambda m: _ph(m.group(0)), + text, + ) + + # 2) Protect inline code (`...`) + text = re.sub(r'(`[^`]+`)', lambda m: _ph(m.group(0)), text) + + # 3) Convert markdown links [text](url) → + text = re.sub( + r'\[([^\]]+)\]\(([^)]+)\)', + lambda m: _ph(f'<{m.group(2)}|{m.group(1)}>'), + text, + ) + + # 4) Convert headers (## Title) → *Title* (bold) + def _convert_header(m): + inner = m.group(1).strip() + # Strip redundant bold markers inside a header + inner = re.sub(r'\*\*(.+?)\*\*', r'\1', inner) + return _ph(f'*{inner}*') + + text = re.sub( + r'^#{1,6}\s+(.+)$', _convert_header, text, flags=re.MULTILINE + ) + + # 5) Convert bold: **text** → *text* (Slack bold) + text = re.sub( + r'\*\*(.+?)\*\*', + lambda m: _ph(f'*{m.group(1)}*'), + text, + ) + + # 6) Convert italic: _text_ stays as _text_ (already Slack italic) + # Single *text* → _text_ (Slack italic) + text = re.sub( + r'(? text → > text (same syntax, just ensure + # no extra escaping happens to the > character) + # Slack uses the same > prefix, so this is a no-op for content. + + # 9) Restore placeholders in reverse order + for key in reversed(list(placeholders.keys())): + text = text.replace(key, placeholders[key]) + + return text + + # ----- Reactions ----- + + async def _add_reaction( + self, channel: str, timestamp: str, emoji: str + ) -> bool: + """Add an emoji reaction to a message. Returns True on success.""" + if not self._app: + return False + try: + await self._app.client.reactions_add( + channel=channel, timestamp=timestamp, name=emoji + ) + return True + except Exception as e: + # Don't log as error — may fail if already reacted or missing scope + logger.debug("[Slack] reactions.add failed (%s): %s", emoji, e) + return False + + async def _remove_reaction( + self, channel: str, timestamp: str, emoji: str + ) -> bool: + """Remove an emoji reaction from a message. Returns True on success.""" + if not self._app: + return False + try: + await self._app.client.reactions_remove( + channel=channel, timestamp=timestamp, name=emoji + ) + return True + except Exception as e: + logger.debug("[Slack] reactions.remove failed (%s): %s", emoji, e) + return False + + # ----- User identity resolution ----- + + async def _resolve_user_name(self, user_id: str) -> str: + """Resolve a Slack user ID to a display name, with caching.""" + if not user_id: + return "" + if user_id in self._user_name_cache: + return self._user_name_cache[user_id] + + if not self._app: + return user_id + + try: + result = await self._app.client.users_info(user=user_id) + user = result.get("user", {}) + # Prefer display_name → real_name → user_id + profile = user.get("profile", {}) + name = ( + profile.get("display_name") + or profile.get("real_name") + or user.get("real_name") + or user.get("name") + or user_id + ) + self._user_name_cache[user_id] = name + return name + except Exception as e: + logger.debug("[Slack] users.info failed for %s: %s", user_id, e) + self._user_name_cache[user_id] = user_id + return user_id + async def send_image_file( self, chat_id: str, @@ -551,12 +718,16 @@ class SlackAdapter(BasePlatformAdapter): except Exception as e: # pragma: no cover - defensive logging logger.warning("[Slack] Failed to cache document from %s: %s", url, e, exc_info=True) + # Resolve user display name (cached after first lookup) + user_name = await self._resolve_user_name(user_id) + # Build source source = self.build_source( chat_id=channel_id, chat_name=channel_id, # Will be resolved later if needed chat_type="dm" if is_dm else "group", user_id=user_id, + user_name=user_name, thread_id=thread_ts, ) @@ -571,8 +742,15 @@ class SlackAdapter(BasePlatformAdapter): reply_to_message_id=thread_ts if thread_ts != ts else None, ) + # Add 👀 reaction to acknowledge receipt + await self._add_reaction(channel_id, ts, "eyes") + await self.handle_message(msg_event) + # Replace 👀 with ✅ when done + await self._remove_reaction(channel_id, ts, "eyes") + await self._add_reaction(channel_id, ts, "white_check_mark") + async def _handle_slash_command(self, command: dict) -> None: """Handle /hermes slash command.""" text = command.get("text", "").strip() @@ -586,6 +764,15 @@ class SlackAdapter(BasePlatformAdapter): "help": "/help", "model": "/model", "personality": "/personality", "retry": "/retry", "undo": "/undo", + "compact": "/compress", "compress": "/compress", + "resume": "/resume", + "background": "/background", + "usage": "/usage", + "insights": "/insights", + "title": "/title", + "reasoning": "/reasoning", + "provider": "/provider", + "rollback": "/rollback", } first_word = text.split()[0] if text else "" if first_word in subcommand_map: diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index efdb62ce4..d2b7643ea 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -530,3 +530,277 @@ class TestMessageRouting: } await adapter._handle_slack_message(event) adapter.handle_message.assert_not_called() + + +# --------------------------------------------------------------------------- +# TestFormatMessage — Markdown → mrkdwn conversion +# --------------------------------------------------------------------------- + + +class TestFormatMessage: + """Test markdown to Slack mrkdwn conversion.""" + + def test_bold_conversion(self, adapter): + assert adapter.format_message("**hello**") == "*hello*" + + def test_italic_asterisk_conversion(self, adapter): + assert adapter.format_message("*hello*") == "_hello_" + + def test_italic_underscore_preserved(self, adapter): + assert adapter.format_message("_hello_") == "_hello_" + + def test_header_to_bold(self, adapter): + assert adapter.format_message("## Section Title") == "*Section Title*" + + def test_header_with_bold_content(self, adapter): + # **bold** inside a header should not double-wrap + assert adapter.format_message("## **Title**") == "*Title*" + + def test_link_conversion(self, adapter): + result = adapter.format_message("[click here](https://example.com)") + assert result == "" + + def test_strikethrough(self, adapter): + assert adapter.format_message("~~deleted~~") == "~deleted~" + + def test_code_block_preserved(self, adapter): + code = "```python\nx = **not bold**\n```" + assert adapter.format_message(code) == code + + def test_inline_code_preserved(self, adapter): + text = "Use `**raw**` syntax" + assert adapter.format_message(text) == "Use `**raw**` syntax" + + def test_mixed_content(self, adapter): + text = "**Bold** and *italic* with `code`" + result = adapter.format_message(text) + assert "*Bold*" in result + assert "_italic_" in result + assert "`code`" in result + + def test_empty_string(self, adapter): + assert adapter.format_message("") == "" + + def test_none_passthrough(self, adapter): + assert adapter.format_message(None) is None + + +# --------------------------------------------------------------------------- +# TestReactions +# --------------------------------------------------------------------------- + + +class TestReactions: + """Test emoji reaction methods.""" + + @pytest.mark.asyncio + async def test_add_reaction_calls_api(self, adapter): + adapter._app.client.reactions_add = AsyncMock() + result = await adapter._add_reaction("C123", "ts1", "eyes") + assert result is True + adapter._app.client.reactions_add.assert_called_once_with( + channel="C123", timestamp="ts1", name="eyes" + ) + + @pytest.mark.asyncio + async def test_add_reaction_handles_error(self, adapter): + adapter._app.client.reactions_add = AsyncMock(side_effect=Exception("already_reacted")) + result = await adapter._add_reaction("C123", "ts1", "eyes") + assert result is False + + @pytest.mark.asyncio + async def test_remove_reaction_calls_api(self, adapter): + adapter._app.client.reactions_remove = AsyncMock() + result = await adapter._remove_reaction("C123", "ts1", "eyes") + assert result is True + + @pytest.mark.asyncio + async def test_reactions_in_message_flow(self, adapter): + """Reactions should be added on receipt and swapped on completion.""" + adapter._app.client.reactions_add = AsyncMock() + adapter._app.client.reactions_remove = AsyncMock() + adapter._app.client.users_info = AsyncMock(return_value={ + "user": {"profile": {"display_name": "Tyler"}} + }) + + event = { + "text": "hello", + "user": "U_USER", + "channel": "C123", + "channel_type": "im", + "ts": "1234567890.000001", + } + await adapter._handle_slack_message(event) + + # Should have added 👀, then removed 👀, then added ✅ + add_calls = adapter._app.client.reactions_add.call_args_list + remove_calls = adapter._app.client.reactions_remove.call_args_list + assert len(add_calls) == 2 + assert add_calls[0].kwargs["name"] == "eyes" + assert add_calls[1].kwargs["name"] == "white_check_mark" + assert len(remove_calls) == 1 + assert remove_calls[0].kwargs["name"] == "eyes" + + +# --------------------------------------------------------------------------- +# TestUserNameResolution +# --------------------------------------------------------------------------- + + +class TestUserNameResolution: + """Test user identity resolution.""" + + @pytest.mark.asyncio + async def test_resolves_display_name(self, adapter): + adapter._app.client.users_info = AsyncMock(return_value={ + "user": {"profile": {"display_name": "Tyler", "real_name": "Tyler B"}} + }) + name = await adapter._resolve_user_name("U123") + assert name == "Tyler" + + @pytest.mark.asyncio + async def test_falls_back_to_real_name(self, adapter): + adapter._app.client.users_info = AsyncMock(return_value={ + "user": {"profile": {"display_name": "", "real_name": "Tyler B"}} + }) + name = await adapter._resolve_user_name("U123") + assert name == "Tyler B" + + @pytest.mark.asyncio + async def test_caches_result(self, adapter): + adapter._app.client.users_info = AsyncMock(return_value={ + "user": {"profile": {"display_name": "Tyler"}} + }) + await adapter._resolve_user_name("U123") + await adapter._resolve_user_name("U123") + # Only one API call despite two lookups + assert adapter._app.client.users_info.call_count == 1 + + @pytest.mark.asyncio + async def test_handles_api_error(self, adapter): + adapter._app.client.users_info = AsyncMock(side_effect=Exception("rate limited")) + name = await adapter._resolve_user_name("U123") + assert name == "U123" # Falls back to user_id + + @pytest.mark.asyncio + async def test_user_name_in_message_source(self, adapter): + """Message source should include resolved user name.""" + adapter._app.client.users_info = AsyncMock(return_value={ + "user": {"profile": {"display_name": "Tyler"}} + }) + adapter._app.client.reactions_add = AsyncMock() + adapter._app.client.reactions_remove = AsyncMock() + + event = { + "text": "hello", + "user": "U_USER", + "channel": "C123", + "channel_type": "im", + "ts": "1234567890.000001", + } + await adapter._handle_slack_message(event) + + # Check the source in the MessageEvent passed to handle_message + msg_event = adapter.handle_message.call_args[0][0] + assert msg_event.source.user_name == "Tyler" + + +# --------------------------------------------------------------------------- +# TestSlashCommands — expanded command set +# --------------------------------------------------------------------------- + + +class TestSlashCommands: + """Test slash command routing.""" + + @pytest.mark.asyncio + async def test_compact_maps_to_compress(self, adapter): + command = {"text": "compact", "user_id": "U1", "channel_id": "C1"} + await adapter._handle_slash_command(command) + msg = adapter.handle_message.call_args[0][0] + assert msg.text == "/compress" + + @pytest.mark.asyncio + async def test_resume_command(self, adapter): + command = {"text": "resume my session", "user_id": "U1", "channel_id": "C1"} + await adapter._handle_slash_command(command) + msg = adapter.handle_message.call_args[0][0] + assert msg.text == "/resume my session" + + @pytest.mark.asyncio + async def test_background_command(self, adapter): + command = {"text": "background run tests", "user_id": "U1", "channel_id": "C1"} + await adapter._handle_slash_command(command) + msg = adapter.handle_message.call_args[0][0] + assert msg.text == "/background run tests" + + @pytest.mark.asyncio + async def test_usage_command(self, adapter): + command = {"text": "usage", "user_id": "U1", "channel_id": "C1"} + await adapter._handle_slash_command(command) + msg = adapter.handle_message.call_args[0][0] + assert msg.text == "/usage" + + @pytest.mark.asyncio + async def test_reasoning_command(self, adapter): + command = {"text": "reasoning", "user_id": "U1", "channel_id": "C1"} + await adapter._handle_slash_command(command) + msg = adapter.handle_message.call_args[0][0] + assert msg.text == "/reasoning" + + +# --------------------------------------------------------------------------- +# TestMessageSplitting +# --------------------------------------------------------------------------- + + +class TestMessageSplitting: + """Test that long messages are split before sending.""" + + @pytest.mark.asyncio + async def test_long_message_split_into_chunks(self, adapter): + """Messages over MAX_MESSAGE_LENGTH should be split.""" + long_text = "x" * 5000 + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "ts1"} + ) + await adapter.send("C123", long_text) + # Should have been called multiple times + assert adapter._app.client.chat_postMessage.call_count >= 2 + + @pytest.mark.asyncio + async def test_short_message_single_send(self, adapter): + """Short messages should be sent in one call.""" + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "ts1"} + ) + await adapter.send("C123", "hello world") + assert adapter._app.client.chat_postMessage.call_count == 1 + + +# --------------------------------------------------------------------------- +# TestReplyBroadcast +# --------------------------------------------------------------------------- + + +class TestReplyBroadcast: + """Test reply_broadcast config option.""" + + @pytest.mark.asyncio + async def test_broadcast_disabled_by_default(self, adapter): + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "ts1"} + ) + await adapter.send("C123", "hi", metadata={"thread_id": "parent_ts"}) + kwargs = adapter._app.client.chat_postMessage.call_args.kwargs + assert "reply_broadcast" not in kwargs + + @pytest.mark.asyncio + async def test_broadcast_enabled_via_config(self, adapter): + adapter.config.extra["reply_broadcast"] = True + adapter._app.client.chat_postMessage = AsyncMock( + return_value={"ts": "ts1"} + ) + await adapter.send("C123", "hi", metadata={"thread_id": "parent_ts"}) + kwargs = adapter._app.client.chat_postMessage.call_args.kwargs + assert kwargs.get("reply_broadcast") is True