diff --git a/hermes_cli/slack_cli.py b/hermes_cli/slack_cli.py index a517da56c5c..6d9124c2c46 100644 --- a/hermes_cli/slack_cli.py +++ b/hermes_cli/slack_cli.py @@ -86,6 +86,7 @@ def _build_full_manifest( "im:write", "mpim:history", "mpim:read", + "reactions:read", "users:read", ] @@ -95,6 +96,7 @@ def _build_full_manifest( "message.groups", "message.im", "message.mpim", + "reaction_added", ] if messaging_experience == "assistant": diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index a606272a950..68c016bc937 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -17,7 +17,7 @@ import os import re import time from dataclasses import dataclass, field -from typing import Callable, Dict, Optional, Any, Tuple, List +from typing import Callable, ClassVar, Dict, Optional, Any, Tuple, List import aiohttp @@ -1773,13 +1773,15 @@ class SlackAdapter(BasePlatformAdapter): async def handle_file_change(event, say): pass - # Reactions are useful lightweight acknowledgements in Slack, but - # Hermes does not currently need to route them into the agent loop. - # Ack the events explicitly so high-traffic channels do not fill - # gateway.error.log with Slack Bolt "Unhandled request" warnings. + # Forward reaction_added events through the normal message + # pipeline (see _handle_slack_reaction). Skills that present + # confirmation-style proposals ("react 👍 to proceed") then work + # end-to-end. Registered explicitly so high-traffic channels do + # not fill gateway.error.log with Slack Bolt "Unhandled request" + # warnings. @self._app.event("reaction_added") async def handle_reaction_added(event, say): - pass + await self._handle_slack_reaction(event) @self._app.event("reaction_removed") async def handle_reaction_removed(event, say): @@ -4282,6 +4284,127 @@ class SlackAdapter(BasePlatformAdapter): team_id=metadata["team_id"], ) + # Common reaction names → unicode emoji. Used by ``_handle_slack_reaction`` + # so skills that match on ``text`` see the same character whether the user + # typed it or reacted with it. + _REACTION_EMOJI_MAP: ClassVar[Dict[str, str]] = { + "thumbsup": "👍", + "+1": "👍", + "thumbsdown": "👎", + "-1": "👎", + "white_check_mark": "✅", + "heavy_check_mark": "✅", + "x": "❌", + "no_entry": "⛔", + "warning": "⚠️", + "rotating_light": "🚨", + "eyes": "👀", + "rocket": "🚀", + "tada": "🎉", + "fire": "🔥", + "wave": "👋", + } + + async def _handle_slack_reaction(self, event: dict) -> None: + """Forward ``reaction_added`` events through the normal message pipeline. + + The reactor's user_id becomes the synthesized message's user, so the + downstream auth gate (``_is_user_authorized``) applies as it does for + any other message. The reacted-to message's ``thread_ts`` becomes + the synthesized message's ``thread_ts`` so the reaction lands in the + same thread as a regular reply would, letting skills that present + confirmation-style proposals (``react 👍 to proceed``) treat + reactions as real responses. + + Common reactions are translated to unicode emoji in the text field + (👍, 👎, ✅, etc.) so skill bodies that match on the typed character + also fire on the equivalent reaction without needing to know about + Slack-specific names. + + Self-reactions (the bot reacting to its own messages, e.g. the + :eyes: lifecycle reaction) are dropped here to prevent feedback + loops. file-targeted reactions are ignored — only ``item.type == + "message"`` is forwarded. Reactions on messages not sent by this + bot are dropped so a reaction on an unrelated human message can't + enter the agent loop. + """ + item = event.get("item") or {} + if item.get("type") != "message": + return + channel_id = item.get("channel") + msg_ts = item.get("ts") + reaction_name = event.get("reaction") or "" + user_id = event.get("user") + if not channel_id or not msg_ts or not user_id or not reaction_name: + return + # Drop self-reactions (lifecycle markers like :eyes: on incoming msgs). + if self._bot_user_id and user_id == self._bot_user_id: + return + team_id = self._channel_team.get(channel_id) or "" + if not team_id and self._team_clients: + team_id = next(iter(self._team_clients)) + client = self._team_clients.get(team_id) if team_id else None + + # Look up the reacted-to message so we can route the synthesized + # event into the right thread and verify the target belongs to this + # bot (matching the Feishu adapter's target-sender check). If the + # lookup fails, fall back to treating the reacted-to message as the + # thread parent — that's correct for top-level messages and + # degrades gracefully for in-thread reactions where we lose the + # parent linkage. + thread_ts: Optional[str] = msg_ts + if client is not None: + try: + history = await client.conversations_replies( + channel=channel_id, ts=msg_ts, limit=1, inclusive=True, + ) + messages = (history or {}).get("messages") or [] + if messages: + first = messages[0] + thread_ts = first.get("thread_ts") or first.get("ts") or msg_ts + # Verify the reacted-to message was sent by this bot + # (matching the Feishu adapter's target-sender check). + # ``item_user`` on the event is the author of the + # reacted-to message; if absent, fall back to the + # fetched message's ``user`` field. + item_user = event.get("item_user") or first.get("user") or "" + bot_uid = self._team_bot_user_ids.get(team_id) or self._bot_user_id + if item_user and bot_uid and item_user != bot_uid: + return + except Exception as e: # pragma: no cover - network path + logger.debug( + "[Slack] reaction thread_ts lookup failed for %s: %s", + msg_ts, e, + ) + + emoji_text = self._REACTION_EMOJI_MAP.get(reaction_name, f":{reaction_name}:") + + # Use the reaction's own event_ts as the synthesized message ts so + # the deduplicator in _handle_slack_message treats this reaction + # as a distinct event (it has nothing to do with the reacted-to + # message's ts). + synthetic_ts = event.get("event_ts") or f"reaction-{msg_ts}-{reaction_name}-{user_id}" + synthetic: dict = { + "type": "message", + "user": user_id, + "text": emoji_text, + "channel": channel_id, + "ts": synthetic_ts, + "thread_ts": thread_ts, + # Surfaced for any downstream code that wants to know this was a + # reaction rather than a typed message; not used by the default + # pipeline. + "_hermes_reaction": { + "name": reaction_name, + "reacted_to_ts": msg_ts, + "event_ts": event.get("event_ts"), + }, + } + if team_id: + synthetic["team"] = team_id + + await self._handle_slack_message(synthetic) + async def _handle_slack_file_shared( self, event: dict, body: Optional[dict] = None ) -> None: diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index f1427d8b8c5..8e622a7c2cb 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -1153,3 +1153,182 @@ class TestThreadEngagement: "1000.000003", "1000.000004", } + + +# =========================================================================== +# _handle_slack_reaction — reaction_added forwarding +# =========================================================================== + +class TestSlackReactionForwarding: + """Reactions should flow through the same pipeline as typed messages.""" + + @pytest.mark.asyncio + async def test_reaction_synthesizes_message_in_thread(self): + """A 👍 reaction on a message in a thread should produce a synthesized + MessageEvent that lands in that thread with text ``👍``, going through + _handle_slack_message so the auth gate, thread-context fetch, and + skill routing all apply unchanged.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + # Reacted-to message is itself a reply inside a thread; its thread_ts + # points at the thread parent. + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [ + {"ts": "2000.0", "thread_ts": "1000.0", "user": "U_BOT", "text": "Proposal"} + ] + }) + forwarded: list[dict] = [] + + async def _capture(event): + forwarded.append(event) + + with patch.object(adapter, "_handle_slack_message", new=_capture): + await adapter._handle_slack_reaction({ + "type": "reaction_added", + "user": "U1", + "reaction": "thumbsup", + "item": {"type": "message", "channel": "C1", "ts": "2000.0"}, + "item_user": "U_BOT", + "event_ts": "3000.0", + }) + + assert len(forwarded) == 1 + synth = forwarded[0] + assert synth["type"] == "message" + assert synth["user"] == "U1" + assert synth["text"] == "👍" + assert synth["channel"] == "C1" + # Threaded back to the parent of the reacted-to message, not to the + # reacted-to message itself. + assert synth["thread_ts"] == "1000.0" + # Distinct synthetic ts so dedup doesn't merge with anything else. + assert synth["ts"] == "3000.0" + # Reaction metadata preserved for downstream introspection. + assert synth["_hermes_reaction"]["name"] == "thumbsup" + assert synth["_hermes_reaction"]["reacted_to_ts"] == "2000.0" + + @pytest.mark.asyncio + async def test_self_reaction_dropped(self): + """The bot's own reactions (e.g. the :eyes: lifecycle marker on + incoming messages) must not feed back into the pipeline.""" + adapter = _make_adapter() + forwarded: list[dict] = [] + + async def _capture(event): + forwarded.append(event) + + with patch.object(adapter, "_handle_slack_message", new=_capture): + await adapter._handle_slack_reaction({ + "type": "reaction_added", + "user": "U_BOT", # matches adapter._bot_user_id + "reaction": "eyes", + "item": {"type": "message", "channel": "C1", "ts": "1000.0"}, + "item_user": "U1", + "event_ts": "3000.0", + }) + + assert forwarded == [] + + @pytest.mark.asyncio + async def test_unknown_reaction_uses_colon_name(self): + """Reactions outside the unicode emoji map still forward, with text + set to the Slack short name in colons. Skills can match on those.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [{"ts": "1000.0", "user": "U_BOT", "text": "Parent"}] + }) + forwarded: list[dict] = [] + + async def _capture(event): + forwarded.append(event) + + with patch.object(adapter, "_handle_slack_message", new=_capture): + await adapter._handle_slack_reaction({ + "type": "reaction_added", + "user": "U1", + "reaction": "moov-rocket", # custom workspace emoji + "item": {"type": "message", "channel": "C1", "ts": "1000.0"}, + "item_user": "U_BOT", + "event_ts": "3000.0", + }) + + assert len(forwarded) == 1 + assert forwarded[0]["text"] == ":moov-rocket:" + + @pytest.mark.asyncio + async def test_non_message_reaction_ignored(self): + """File reactions and other non-message item types are dropped — we + only forward message reactions.""" + adapter = _make_adapter() + forwarded: list[dict] = [] + + async def _capture(event): + forwarded.append(event) + + with patch.object(adapter, "_handle_slack_message", new=_capture): + await adapter._handle_slack_reaction({ + "type": "reaction_added", + "user": "U1", + "reaction": "thumbsup", + "item": {"type": "file", "file": "F123"}, + "event_ts": "3000.0", + }) + + assert forwarded == [] + + @pytest.mark.asyncio + async def test_top_level_message_threads_to_self(self): + """When the reacted-to message is itself the thread parent (no + thread_ts of its own), the synthesized event uses the message ts + as thread_ts.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [{"ts": "1000.0", "user": "U_BOT", "text": "Parent"}] + }) + forwarded: list[dict] = [] + + async def _capture(event): + forwarded.append(event) + + with patch.object(adapter, "_handle_slack_message", new=_capture): + await adapter._handle_slack_reaction({ + "type": "reaction_added", + "user": "U1", + "reaction": "+1", # alias for thumbsup + "item": {"type": "message", "channel": "C1", "ts": "1000.0"}, + "item_user": "U_BOT", + "event_ts": "3000.0", + }) + + assert len(forwarded) == 1 + assert forwarded[0]["text"] == "👍" + assert forwarded[0]["thread_ts"] == "1000.0" + + @pytest.mark.asyncio + async def test_reaction_on_non_bot_message_dropped(self): + """A reaction on a message not sent by this bot must not enter the + agent loop — matching the Feishu adapter's target-sender check.""" + adapter = _make_adapter() + mock_client = adapter._team_clients["T1"] + mock_client.conversations_replies = AsyncMock(return_value={ + "messages": [{"ts": "1000.0", "user": "U_OTHER", "text": "Not our bot"}] + }) + forwarded: list[dict] = [] + + async def _capture(event): + forwarded.append(event) + + with patch.object(adapter, "_handle_slack_message", new=_capture): + await adapter._handle_slack_reaction({ + "type": "reaction_added", + "user": "U1", + "reaction": "thumbsup", + "item": {"type": "message", "channel": "C1", "ts": "1000.0"}, + "item_user": "U_OTHER", # not our bot + "event_ts": "3000.0", + }) + + assert forwarded == [] + diff --git a/tests/hermes_cli/test_slack_cli.py b/tests/hermes_cli/test_slack_cli.py index 6de85a3007c..70a01d211d3 100644 --- a/tests/hermes_cli/test_slack_cli.py +++ b/tests/hermes_cli/test_slack_cli.py @@ -148,3 +148,22 @@ class TestSlackFullManifest: bot_events = manifest["settings"]["event_subscriptions"]["bot_events"] for event in ("message.im", "message.channels", "message.groups", "app_mention"): assert event in bot_events + + def test_reaction_scope_and_event_included(self): + """reaction_added event + reactions:read scope must be in the manifest + so the adapter can forward reactions into the message pipeline.""" + manifest = _build_full_manifest("Hermes", "Your Hermes agent on Slack") + + bot_scopes = manifest["oauth_config"]["scopes"]["bot"] + bot_events = manifest["settings"]["event_subscriptions"]["bot_events"] + assert "reactions:read" in bot_scopes + assert "reaction_added" in bot_events + + def test_reaction_scope_survives_no_assistant(self): + manifest = _build_full_manifest( + "Hermes", "Your Hermes agent on Slack", include_assistant=False + ) + bot_scopes = manifest["oauth_config"]["scopes"]["bot"] + bot_events = manifest["settings"]["event_subscriptions"]["bot_events"] + assert "reactions:read" in bot_scopes + assert "reaction_added" in bot_events