diff --git a/gateway/slash_commands_goals.py b/gateway/slash_commands_goals.py index 2aa7317c3898..15ab44f7943f 100644 --- a/gateway/slash_commands_goals.py +++ b/gateway/slash_commands_goals.py @@ -84,6 +84,7 @@ def _enqueue_goal_turn( source=event.source, message_id=event.message_id if kickoff else None, channel_prompt=event.channel_prompt if kickoff else None, + auto_skill=event.auto_skill if kickoff else None, ) self._enqueue_fifo(quick_key, turn, adapter) except Exception as exc: diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index 48294ada58da..718b0a412ae5 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -1056,6 +1056,10 @@ def __init__(self, config: PlatformConfig): # posts lacking bot_id/bot_message markers; DM channel IDs are per-user, hence bounded). self._user_name_cache: Dict[Tuple[str, str], str] = {} self._channel_name_cache: Dict[Tuple[str, str], str] = {} + # Slash commands and Block Kit interactions do not carry Slack channel_type. Cache + # successful workspace-scoped classification for ambiguous G... ids (MPIM vs private + # channel); unresolved classification is never cached and callers fail closed. + self._conversation_kind_cache: Dict[Tuple[str, str], Tuple[bool, bool]] = {} self._user_is_bot_cache: Dict[Tuple[str, str], bool] = {} # channel_id → owning team_id (bounded; re-learned on the next event, _get_client falls # back to primary). Kept only while exactly one workspace claims the id — _channel_teams @@ -1885,7 +1889,7 @@ def _hint_allow_bots(self) -> None: async def create_handoff_thread(self, parent_chat_id: str, name: str) -> Optional[str]: """Post a seed message and return its ``ts`` as the handoff ``thread_id``. Slack threads anchor to a parent message, not a channel-level object. Returns ``None`` on failure.""" - if not self._app: + if not getattr(self, "_app", None): return None try: client = self._get_client(parent_chat_id) @@ -3322,6 +3326,132 @@ async def _resolve_user_name(self, user_id: str, chat_id: str = "", team_id: str self._trim_oldest_dict_entries(self._user_name_cache, self._USER_NAME_CACHE_MAX) return name + def _cached_conversation_classification( + self, channel_id: str, team_id: str = "", + ) -> Optional[Tuple[bool, bool]]: + """Lookup-free conversation class: direct prefixes or a prior successful G... resolution.""" + channel_id = str(channel_id or "") + if channel_id.startswith("D"): + return True, True + if not channel_id.startswith("G"): + return False, False + team_id = str(team_id or getattr(self, "_channel_team", {}).get(channel_id, "")) + if not team_id: + return None + return self._lazy_attr("_conversation_kind_cache", dict).get((team_id, channel_id)) + + def _session_key_conversation_classification( + self, session_key: str, channel_id: str, team_id: str, + ) -> Optional[Tuple[bool, bool]]: + """Recover an ambiguous G... class from a trusted session key without Slack I/O. + + Approval button values and active-session keys are gateway-owned. Parse them through the + gateway's canonical same-chat slot helper instead of reconstructing a key from the current + caller: another authorized operator may click an approval created by the session owner. + """ + cached = self._cached_conversation_classification(channel_id, team_id) + if cached is not None: + return cached + channel_id = str(channel_id or "") + session_key = str(session_key or "") + if not channel_id.startswith("G") or not session_key: + return None + + parts = session_key.split(":", 2) + if len(parts) < 3 or parts[0] != "agent": + return None + namespace = ":".join(parts[:2]) + try: + from gateway.run_busy import _same_chat_key_slots + parsed = _same_chat_key_slots( + session_key, + prefix=f"{namespace}:{self.platform.value}:", + chat_id=channel_id, + scope_id=str(team_id or "") or None, + ) + except Exception: + logger.debug( + "[Slack] Could not parse session-key classification for %s", + channel_id, exc_info=True, + ) + return None + if parsed is None: + return None + chat_type = parsed[0] + if chat_type == "dm": + return True, False + if chat_type == "group": + return False, False + return None + + def _active_session_conversation_classification( + self, channel_id: str, team_id: str, + ) -> Optional[Tuple[bool, bool]]: + """Resolve a control's G... class from the currently active session, without network I/O.""" + cached = self._cached_conversation_classification(channel_id, team_id) + if cached is not None: + return cached + matches = { + classification + for session_key in tuple(getattr(self, "_active_sessions", {})) + if (classification := self._session_key_conversation_classification( + session_key, channel_id, team_id + )) is not None + } + return next(iter(matches)) if len(matches) == 1 else None + + async def _classify_conversation( + self, channel_id: str, team_id: str = "", + ) -> Optional[Tuple[bool, bool]]: + """Return (is_dm, is_one_to_one_dm) for Slack surfaces without channel_type. + + D... is unambiguously a 1:1 IM. G... is ambiguous (MPIM or legacy private channel), so + resolve it with conversations.info and cache only a successful answer. Returning None + is deliberate fail-closed behavior for ordinary ingress; control paths first consult + trusted session state so a metadata outage cannot deadlock an active session. + """ + cached = self._cached_conversation_classification(channel_id, team_id) + if cached is not None: + return cached + channel_id = str(channel_id or "") + if not channel_id.startswith("G") or not self._app: + return None + + team_id = str(team_id or getattr(self, "_channel_team", {}).get(channel_id, "")) + if not team_id: + return None + cache_key = (team_id, channel_id) + cache = self._lazy_attr("_conversation_kind_cache", dict) + + try: + response = await self._get_client( + channel_id, team_id=team_id or None + ).conversations_info(channel=channel_id) + payload = _slack_response_payload(response) + if not payload.get("ok"): + return None + channel = payload.get("channel") or {} + if "is_mpim" not in channel: + return None + result = (bool(channel.get("is_mpim")), False) + + # This lookup can also satisfy later cosmetic channel-name enrichment. + names = self._lazy_attr("_channel_name_cache", dict) + names[cache_key] = ( + channel.get("name") or channel.get("name_normalized") or channel_id + ) + self._trim_oldest_dict_entries(names, self._CHANNEL_NAME_CACHE_MAX) + except Exception as exc: + logger.debug( + "[Slack] Could not classify conversation %s in workspace %s: %s", + channel_id, team_id, exc, + ) + return None + + cache[cache_key] = result + self._trim_oldest_dict_entries(cache, self._CHANNEL_NAME_CACHE_MAX) + return result + async def _resolve_channel_name(self, channel_id: str, team_id: str = "") -> str: """Channel ID → name (cached): channel name, or the peer's display name for DMs. Falls back to the raw id on any error so message handling never breaks.""" @@ -4418,6 +4548,36 @@ def _channel_prompt_with_identity(self, channel_id: str, team_id: str) -> Option else identity_prompt) return channel_prompt + def _cached_turn_prompt_inputs( + self, channel_id: str, team_id: str, user_id: str, + ) -> Tuple[str, str, Optional[str], Optional[List[str]]]: + """Prompt inputs without network I/O, for admission-only and urgent-control turns.""" + team_id = str(team_id or getattr(self, "_channel_team", {}).get(channel_id, "")) + channel_name = self._lazy_attr("_channel_name_cache", dict).get( + (team_id, str(channel_id)), str(channel_id) + ) + user_name = self._lazy_attr("_user_name_cache", dict).get( + (team_id, str(user_id)), str(user_id) + ) + from gateway.platforms.base import resolve_channel_skills + return ( + channel_name, + user_name, + self._channel_prompt_with_identity(channel_id, team_id), + resolve_channel_skills(self.config.extra, channel_id, None), + ) + + async def _resolve_turn_prompt_inputs( + self, channel_id: str, team_id: str, user_id: str, + ) -> Tuple[str, str, Optional[str], Optional[List[str]]]: + """Resolve prompt-sensitive inputs shared by Slack turn producers.""" + user_name = await self._resolve_user_name(user_id, chat_id=channel_id, team_id=team_id) + channel_name = await self._resolve_channel_name(channel_id, team_id=team_id) + _cached_channel, _cached_user, channel_prompt, auto_skill = ( + self._cached_turn_prompt_inputs(channel_id, team_id, user_id) + ) + return channel_name, user_name, channel_prompt, auto_skill + def _track_reacting_message(self, team_id: str, ts: str) -> None: """Mark ``ts`` for the reaction lifecycle, evicting oldest-ts-first past the cap.""" self._reacting_message_ids.add(self._workspace_message_marker(team_id, ts)) @@ -4699,8 +4859,8 @@ async def _build_message_event( if is_command_text: text = command_probe_text msg_type = MessageType.COMMAND if is_command_text else self._media_message_type(media_types) - user_name = await self._resolve_user_name(user_id, chat_id=channel_id, team_id=team_id) - channel_name = await self._resolve_channel_name(channel_id, team_id=team_id) + channel_name, user_name, channel_prompt, auto_skill = ( + await self._resolve_turn_prompt_inputs(channel_id, team_id, user_id)) # Best-effort: title the DM thread from the prompt for Slack's AI Agent Messages tab. if is_dm and thread_ts and msg_type != MessageType.COMMAND: await self._set_assistant_thread_title( @@ -4717,7 +4877,6 @@ async def _build_message_event( # Workflow/app posts have user=None; flag them so the SLACK_ALLOW_BOTS bypass can # authorize them. Same predicate as the drop gate (api_human_users stay human). is_bot=self._event_declares_bot_sender(event)) - from gateway.platforms.base import resolve_channel_skills # Remaining ``<@UID>`` are OTHER participants (own mention stripped # above); render as ``@DisplayName`` so the agent knows who is addressed. text = await self._humanize_user_mentions(text, chat_id=channel_id, team_id=team_id) @@ -4731,12 +4890,12 @@ async def _build_message_event( media_types=media_types, media_text_inlined=media_text_inlined, reply_to_message_id=thread_ts if thread_ts != ts else None, - channel_prompt=self._channel_prompt_with_identity(channel_id, team_id), + channel_prompt=channel_prompt, channel_context=channel_context, reply_expected=reply_expected, # thread_ts is the thread root, not an explicit reply (root is in channel_context). reply_to_text=None, - auto_skill=resolve_channel_skills(self.config.extra, channel_id, None), + auto_skill=auto_skill, metadata={ "slack_team_id": team_id, "slack_channel_id": channel_id, "slack_thread_ts": thread_ts}) @@ -5235,16 +5394,9 @@ async def _handle_model_picker_action(self, ack, body, action) -> None: user_name = body.get("user", {}).get("name", "unknown") user_id = body.get("user", {}).get("id", "") - if not self._is_interactive_user_authorized( - user_id, - channel_id=channel_id, - user_name=user_name, - team_id=team_id, + if not await self._authorize_interaction_user( + "model picker", user_id, user_name, channel_id, team_id ): - logger.warning( - "[Slack] Unauthorized model picker click by %s (%s) - ignoring", - user_name, user_id, - ) return # Look up the picker state. The send path may have stored it under a @@ -5461,12 +5613,12 @@ def _build() -> Tuple[str, list]: def _is_interactive_user_authorized( self, user_id: str, *, channel_id: str = "", user_name: Optional[str] = None, - team_id: str = "") -> bool: + team_id: str = "", chat_type: Optional[str] = None) -> bool: """Return whether a Slack interactive caller may perform gated actions.""" normalized_user_id = str(user_id or "").strip() if not normalized_user_id: return False - chat_type = "dm" if str(channel_id or "").startswith("D") else "group" + chat_type = chat_type or ("dm" if str(channel_id or "").startswith("D") else "group") # Preferred: the injected profile-bound check (``set_authorization_check``); unlike the # ``__self__`` introspection below it works under multiplex (handler is a closure). # getattr: object.__new__ test doubles never ran BasePlatformAdapter.__init__. @@ -5507,6 +5659,43 @@ def _is_interactive_user_authorized( return "*" in allowed_ids or normalized_user_id in allowed_ids return _env("GATEWAY_ALLOW_ALL_USERS").lower() in {"true", "1", "yes"} + async def _authorize_interaction_user( + self, kind: str, user_id: str, user_name: str, channel_id: str, team_id: str, *, + team_scoped: bool = True, session_key_hint: str = "", + ) -> bool: + """Authorize a Block Kit caller under the same conversation policy as message ingress.""" + if self._is_ignored_channel(channel_id): + return False + classification = self._session_key_conversation_classification( + session_key_hint, channel_id, team_id + ) if session_key_hint else None + if classification is None: + classification = await self._classify_conversation(channel_id, team_id) + if classification is None: + logger.warning( + "[Slack] Refusing %s interaction because conversation %s could not be classified", + kind, channel_id, + ) + return False + is_dm, is_one_to_one_dm = classification + if is_dm and self._slack_disable_dms(): + return False + allowed_channels = self._slack_allowed_channels() + if not is_one_to_one_dm and allowed_channels and channel_id not in allowed_channels: + return False + authorized = self._is_interactive_user_authorized( + user_id, + channel_id=channel_id, + user_name=user_name, + team_id=team_id if team_scoped else "", + chat_type="dm" if is_dm else "group", + ) + if not authorized: + logger.warning( + "[Slack] Unauthorized %s click by %s (%s) - ignoring", kind, user_name, user_id + ) + return authorized + @staticmethod def _interaction_fields(body: dict, action: dict) -> Tuple[str, str, dict, str, str, str, str]: """Unpack a Block Kit interaction payload into @@ -5527,12 +5716,10 @@ async def _begin_interaction( team_id = self._event_team_id({}, body) action_id, value, message, msg_ts, channel_id, user_name, user_id = ( self._interaction_fields(body, action)) - auth_kwargs: Dict[str, Any] = {"channel_id": channel_id, "user_name": user_name} - if team_scoped: - auth_kwargs["team_id"] = team_id - if not self._is_interactive_user_authorized(user_id, **auth_kwargs): - logger.warning( - "[Slack] Unauthorized %s click by %s (%s) - ignoring", kind, user_name, user_id) + if not await self._authorize_interaction_user( + kind, user_id, user_name, channel_id, team_id, team_scoped=team_scoped, + session_key_hint=value if kind == "approval" else "", + ): return None return team_id, action_id, value, message, msg_ts, channel_id, user_name, user_id @@ -6043,8 +6230,7 @@ async def _collect_thread_root_images( return media_urls, media_types async def _handle_slash_command(self, command: dict) -> None: - """Slash commands: native ``/ [args]`` for every COMMAND_REGISTRY entry, or - ``/hermes [args]``; other text after ``/hermes`` is a regular message.""" + """Slash commands: native / [args] for registry entries, or /hermes text.""" user_id = command.get("user_id", "") channel_id = command.get("channel_id", "") team_id = command.get("team_id", "") @@ -6052,26 +6238,70 @@ async def _handle_slash_command(self, command: dict) -> None: self._remember_channel_team(channel_id, team_id) text = self._slash_command_text(command) thread_id = self._slash_thread_id(command) - is_dm = str(channel_id).startswith("D") + command_name = text[1:].split(None, 1)[0] if text.startswith("/") else None + from hermes_cli.commands import is_interrupt_then_dispatch, resolve_command + command_def = resolve_command(command_name) if command_name else None + urgent_control = bool(command_name and is_interrupt_then_dispatch(command_name)) + lookup_free_control = urgent_control or bool( + command_def and command_def.name in {"approve", "deny"} + ) + + # Slash payloads omit channel_type. Ordinary ingress may resolve G... via Slack, but + # active-session escape hatches must not depend on metadata I/O: recover their exact + # class from the live session key (or an already successful cached resolution). + if self._is_ignored_channel(channel_id): + return + classification = ( + self._active_session_conversation_classification(channel_id, team_id) + if lookup_free_control else await self._classify_conversation(channel_id, team_id) + ) + if classification is None: + logger.warning( + "[Slack] Ignoring slash command because conversation %s could not be classified%s", + channel_id, + " from trusted active/cached state" if lookup_free_control else "", + ) + return + is_dm, is_one_to_one_dm = classification if is_dm and self._slack_disable_dms(): logger.info( "[Slack] Ignoring slash command from DM because Slack DMs are disabled: channel=%s user=%s", channel_id, user_id) return + allowed_channels = self._slack_allowed_channels() + if not is_one_to_one_dm and allowed_channels and channel_id not in allowed_channels: + logger.debug("[Slack] Ignoring slash command in non-allowed channel: %s", channel_id) + return + + denied = self._early_reject_unauthorized(user_id, channel_id, is_dm) + # Unauthorized channel callers are a pure drop. Unauthorized DMs must still reach the + # gateway pair/decline/ignore policy, without spending Slack metadata I/O first. + if denied and not is_dm: + return + + if denied or lookup_free_control: + channel_name, user_name, channel_prompt, auto_skill = ( + self._cached_turn_prompt_inputs(channel_id, team_id, user_id) + ) + else: + channel_name, user_name, channel_prompt, auto_skill = ( + await self._resolve_turn_prompt_inputs(channel_id, team_id, user_id) + ) + source = self.build_source( - chat_id=channel_id, chat_type="dm" if is_dm else "group", user_id=user_id, + chat_id=channel_id, chat_name=channel_name, + chat_type="dm" if is_dm else "group", user_id=user_id, user_name=user_name, thread_id=thread_id, scope_id=team_id or None) event = MessageEvent( text=text, message_type=(MessageType.COMMAND if text.startswith("/") else MessageType.TEXT), - source=source, raw_message=command) - # Stash response_url so the first reply for this channel+user goes ephemeral. COMMAND - # events only: free-form "/hermes " replies must stay public. + source=source, raw_message=command, + channel_prompt=channel_prompt, auto_skill=auto_skill) + # Store response context before gateway admission so denied-DM policy/control replies can + # finish the ephemeral Running acknowledgement. response_url = command.get("response_url", "") if response_url and user_id and channel_id and text.startswith("/"): self._stash_slash_context(team_id, channel_id, user_id, response_url) - # ContextVar lets send() match the right response_url under - # concurrent slashes from multiple users. _slash_user_id_token = _slash_user_id.set(user_id or None) try: await self.handle_message(event) diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py index fe2c8ed13e47..e390abed3c21 100644 --- a/tests/gateway/test_slack.py +++ b/tests/gateway/test_slack.py @@ -249,6 +249,484 @@ async def test_channel_slash_command_uses_group_session_semantics(self, adapter) assert event.source.user_id == "U123" assert event.source.scope_id == "T123" + @pytest.mark.asyncio + async def test_slash_turn_matches_message_prompt_inputs(self, adapter): + adapter.config.extra.update({ + "channel_prompts": {"C123": "Answer in haiku."}, + "channel_skill_bindings": [{"id": "C123", "skill": "triage"}], + }) + adapter._bot_display_name = "HermesBot" + adapter._app.client.conversations_info = AsyncMock( + return_value={"ok": True, "channel": {"id": "C123", "name": "ops"}}) + adapter.set_authorization_check(lambda _user, _type, _chat: True) + + await adapter._handle_slash_command({ + "command": "/hermes", "text": "what broke?", "user_id": "U123", + "channel_id": "C123", "team_id": "T123", + }) + slash = adapter.handle_message.await_args.args[0] + + message = await adapter._build_message_event( + {"user": "U123", "channel": "C123"}, + text="what broke?", original_text="what broke?", command_probe_text="what broke?", + is_command_text=False, channel_id="C123", team_id="T123", ts="171.001", + user_id="U123", thread_ts=None, is_dm=False, media_urls=[], media_types=[], + media_text_inlined=[], channel_context=None) + + assert message.channel_prompt + assert 'bot "@HermesBot"' in message.channel_prompt + assert "Answer in haiku." in message.channel_prompt + assert slash.channel_prompt == message.channel_prompt + assert slash.auto_skill == message.auto_skill == ["triage"] + assert (slash.source.chat_name, slash.source.user_name) == ( + message.source.chat_name, message.source.user_name) == ("ops", "Test User") + + @pytest.mark.asyncio + @pytest.mark.parametrize("behavior", ["pair", "decline", "ignore"]) + async def test_unauthorized_slash_stops_before_name_enrichment( + self, adapter, behavior, + ): + adapter.config.extra["unauthorized_dm_behavior"] = behavior + adapter.set_authorization_check(lambda _user, _type, _chat: False) + adapter._app.client.conversations_info = AsyncMock( + return_value={"ok": True, "channel": {"id": "C123", "name": "ops"}}) + adapter._app.client.users_info.reset_mock() + + await adapter._handle_slash_command({ + "command": "/hermes", "text": "what broke?", "user_id": "U_BAD", + "channel_id": "C123", "team_id": "T123", + }) + + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + adapter.handle_message.assert_not_awaited() + + + @pytest.mark.asyncio + @pytest.mark.parametrize("behavior", ["pair", "decline", "ignore"]) + async def test_denied_dm_reaches_real_gateway_policy_without_name_enrichment( + self, adapter, behavior, tmp_path, monkeypatch, caplog, + ): + """Denied DMs reach the real pair/decline/ignore sink without Slack metadata I/O.""" + import logging + from gateway.pairing import PairingStore + + monkeypatch.setattr("gateway.pairing.PAIRING_DIR", tmp_path) + adapter.config.extra["unauthorized_dm_behavior"] = behavior + adapter.set_authorization_check(lambda _user, _type, _chat: False) + adapter._app.client.conversations_info = AsyncMock( + side_effect=AssertionError("denied DM performed channel metadata I/O") + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("denied DM performed user metadata I/O") + ) + adapter.send = AsyncMock() + + runner = object.__new__(GatewayRunner) + runner.config = GatewayConfig(platforms={Platform.SLACK: adapter.config}) + runner.adapters = {Platform.SLACK: adapter} + runner.pairing_store = PairingStore() + runner.pairing_stores = {} + runner._intake_adapter_for = lambda _source: adapter + runner._delivery_adapter_for = lambda _source: adapter + runner._scale_to_zero_note_real_inbound = lambda: None + + async def passthrough(event, _source): + return event + + runner._hm_pre_gateway_dispatch_hook = passthrough + runner._is_user_authorized_for_source = lambda _source: False + + adapter.handle_message = runner._hm_admit_event + with caplog.at_level(logging.WARNING): + await adapter._handle_slash_command({ + "command": "/help", + "text": "", + "user_id": "U_STRANGER", + "channel_id": "D123", + "team_id": "T123", + "response_url": "https://hooks.slack.test/response", + }) + + pending = runner.pairing_store.list_pending("slack") + declined = runner.pairing_store.has_recent_decline("slack", "U_STRANGER") + if behavior == "pair": + assert pending and pending[0]["user_id"] == "U_STRANGER" + assert declined is False + adapter.send.assert_awaited_once() + elif behavior == "decline": + assert pending == [] + assert declined is True + adapter.send.assert_awaited_once() + else: + assert pending == [] + assert declined is False + adapter.send.assert_not_awaited() + assert any( + "Unauthorized user (ignored)" in record.getMessage() + for record in caplog.records + ) + + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + assert adapter._slash_command_contexts + + @pytest.mark.asyncio + async def test_stop_dispatches_without_waiting_for_name_metadata(self, adapter): + """Interrupt controls must reach BasePlatformAdapter before cosmetic Slack metadata.""" + adapter.set_authorization_check(lambda _user, _type, _chat: True) + adapter._resolve_turn_prompt_inputs = AsyncMock( + side_effect=AssertionError("/stop waited for prompt metadata") + ) + adapter._app.client.conversations_info = AsyncMock( + side_effect=AssertionError("/stop performed channel metadata I/O") + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("/stop performed user metadata I/O") + ) + + await adapter._handle_slash_command({ + "command": "/stop", + "text": "", + "user_id": "U123", + "channel_id": "C123", + "team_id": "T123", + }) + + adapter.handle_message.assert_awaited_once() + event = adapter.handle_message.await_args.args[0] + assert event.text == "/stop" + assert event.source.chat_name == "C123" + assert event.source.user_name == "U123" + adapter._resolve_turn_prompt_inputs.assert_not_awaited() + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + + @pytest.mark.asyncio + async def test_stop_cancels_real_active_session_without_metadata_io(self, adapter): + """The real BasePlatformAdapter stop lane cancels work before Slack name I/O can matter.""" + from gateway.platforms.base import BasePlatformAdapter + from gateway.platforms.event import MessageEvent, MessageType + + adapter.set_authorization_check(lambda _user, _type, _chat: True) + adapter._app.client.conversations_info = AsyncMock( + side_effect=AssertionError("/stop performed channel metadata I/O") + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("/stop performed user metadata I/O") + ) + adapter._message_handler = AsyncMock() + adapter.handle_message = BasePlatformAdapter.handle_message.__get__( + adapter, type(adapter) + ) + adapter._dispatch_inline_reply = AsyncMock(return_value=None) + + source = adapter.build_source( + chat_id="C123", + chat_type="group", + user_id="U123", + scope_id="T123", + ) + seed = MessageEvent( + text="working", + message_type=MessageType.TEXT, + source=source, + ) + session_key = adapter._event_session_key(seed) + blocker = asyncio.Event() + + async def running_work(): + await blocker.wait() + + task = asyncio.create_task(running_work()) + await asyncio.sleep(0) + adapter._active_sessions[session_key] = asyncio.Event() + adapter._session_tasks[session_key] = task + + await adapter._handle_slash_command({ + "command": "/stop", + "text": "", + "user_id": "U123", + "channel_id": "C123", + "team_id": "T123", + }) + + assert task.done() + assert task.cancelled() + adapter._dispatch_inline_reply.assert_awaited_once() + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + + + @pytest.mark.asyncio + async def test_approve_dispatches_active_g_private_without_metadata_io(self, adapter): + """Approval controls recover a legacy G... private-channel class from the active key.""" + from gateway.platforms.base import BasePlatformAdapter + from gateway.platforms.event import MessageEvent, MessageType + + adapter.set_authorization_check( + lambda user, chat_type, _chat: user == "U_GROUP" and chat_type == "group" + ) + adapter._app.client.conversations_info = AsyncMock( + side_effect=AssertionError("/approve performed channel metadata I/O") + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("/approve performed user metadata I/O") + ) + adapter._message_handler = AsyncMock() + adapter.handle_message = BasePlatformAdapter.handle_message.__get__( + adapter, type(adapter) + ) + adapter._dispatch_inline_reply = AsyncMock(return_value=None) + + source = adapter.build_source( + chat_id="G_PRIVATE", + chat_type="group", + user_id="U_GROUP", + scope_id="T123", + ) + seed = MessageEvent(text="working", message_type=MessageType.TEXT, source=source) + session_key = adapter._event_session_key(seed) + adapter._active_sessions[session_key] = asyncio.Event() + + await adapter._handle_slash_command({ + "command": "/approve", + "text": "session", + "user_id": "U_GROUP", + "channel_id": "G_PRIVATE", + "team_id": "T123", + }) + + adapter._dispatch_inline_reply.assert_awaited_once() + dispatched = adapter._dispatch_inline_reply.await_args.args[0] + assert dispatched.source.chat_type == "group" + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + + @pytest.mark.asyncio + async def test_stop_uses_active_mpim_session_class_without_metadata_io(self, adapter): + """A G... MPIM /stop stays lookup-free while retaining DM authorization.""" + from gateway.platforms.base import BasePlatformAdapter + from gateway.platforms.event import MessageEvent, MessageType + + adapter.set_authorization_check( + lambda user, chat_type, _chat: user == "U_DM" and chat_type == "dm" + ) + adapter._app.client.conversations_info = AsyncMock( + side_effect=AssertionError("/stop performed conversations.info") + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("/stop performed users.info") + ) + adapter._message_handler = AsyncMock() + adapter.handle_message = BasePlatformAdapter.handle_message.__get__( + adapter, type(adapter) + ) + adapter._dispatch_inline_reply = AsyncMock(return_value=None) + + source = adapter.build_source( + chat_id="G_DM", + chat_type="dm", + user_id="U_DM", + scope_id="T123", + ) + seed = MessageEvent(text="working", message_type=MessageType.TEXT, source=source) + session_key = adapter._event_session_key(seed) + blocker = asyncio.Event() + + async def running_work(): + await blocker.wait() + + task = asyncio.create_task(running_work()) + await asyncio.sleep(0) + adapter._active_sessions[session_key] = asyncio.Event() + adapter._session_tasks[session_key] = task + + await adapter._handle_slash_command({ + "command": "/stop", + "text": "", + "user_id": "U_DM", + "channel_id": "G_DM", + "team_id": "T123", + }) + + assert task.done() and task.cancelled() + adapter._dispatch_inline_reply.assert_awaited_once() + dispatched = adapter._dispatch_inline_reply.await_args.args[0] + assert dispatched.source.chat_type == "dm" + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + + @pytest.mark.asyncio + async def test_native_slash_enforces_allowed_channels_before_enrichment(self, adapter): + adapter.config.extra["allowed_channels"] = ["C_ALLOWED"] + adapter.set_authorization_check(lambda _user, _type, _chat: True) + adapter._app.client.conversations_info = AsyncMock( + side_effect=AssertionError("excluded channel performed name enrichment") + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("excluded channel performed user enrichment") + ) + + await adapter._handle_slash_command({ + "command": "/hermes", + "text": "what broke?", + "user_id": "U123", + "channel_id": "C_BLOCKED", + "team_id": "T123", + }) + + adapter.handle_message.assert_not_awaited() + adapter._app.client.conversations_info.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + + @pytest.mark.asyncio + async def test_ambiguous_g_conversation_fails_closed(self, adapter): + adapter.set_authorization_check(lambda _user, _type, _chat: True) + adapter._app.client.conversations_info = AsyncMock( + return_value={"ok": True, "channel": {"id": "G_UNKNOWN", "name": "ambiguous"}} + ) + adapter._app.client.users_info = AsyncMock( + side_effect=AssertionError("ambiguous conversation performed user enrichment") + ) + + await adapter._handle_slash_command({ + "command": "/hermes", + "text": "hello", + "user_id": "U_OWNER", + "channel_id": "G_UNKNOWN", + "team_id": "T123", + }) + + adapter.handle_message.assert_not_awaited() + adapter._app.client.users_info.assert_not_awaited() + assert not await adapter._authorize_interaction_user( + "approval", "U_OWNER", "owner", "G_UNKNOWN", "T123" + ) + + @pytest.mark.asyncio + async def test_mpim_slash_and_interactions_share_dm_classification(self, adapter): + """G ids are classified, not prefix-guessed; private channels remain group policy.""" + async def conversations_info(*, channel): + if channel == "G_DM": + return {"ok": True, "channel": {"id": channel, "is_mpim": True, "name": "mpdm"}} + if channel == "G_PRIVATE": + return {"ok": True, "channel": {"id": channel, "is_mpim": False, "name": "private"}} + raise AssertionError(channel) + + adapter._app.client.conversations_info = AsyncMock(side_effect=conversations_info) + adapter.set_authorization_check( + lambda user, chat_type, _chat: + (user == "U_DM" and chat_type == "dm") + or (user == "U_GROUP" and chat_type == "group") + ) + + await adapter._handle_slash_command({ + "command": "/hermes", + "text": "hello", + "user_id": "U_DM", + "channel_id": "G_DM", + "team_id": "T123", + }) + slash = adapter.handle_message.await_args.args[0] + assert slash.source.chat_type == "dm" + + assert await adapter._authorize_interaction_user( + "approval", "U_DM", "dm-user", "G_DM", "T123" + ) + assert not await adapter._authorize_interaction_user( + "approval", "U_GROUP", "group-user", "G_DM", "T123" + ) + assert await adapter._authorize_interaction_user( + "approval", "U_GROUP", "group-user", "G_PRIVATE", "T123" + ) + assert not await adapter._authorize_interaction_user( + "approval", "U_DM", "dm-user", "G_PRIVATE", "T123" + ) + + adapter.config.extra["disable_dms"] = True + assert not await adapter._authorize_interaction_user( + "approval", "U_DM", "dm-user", "G_DM", "T123" + ) + assert await adapter._authorize_interaction_user( + "approval", "U_GROUP", "group-user", "G_PRIVATE", "T123" + ) + + @pytest.mark.asyncio + async def test_message_slash_message_keep_same_prompt_pins(self, adapter): + """Consumer-boundary regression: the same Slack session stays A -> A -> A.""" + from gateway.session import build_session_context, build_session_key + + adapter.config.extra.update({ + "channel_prompts": {"C123": "Answer in haiku."}, + "channel_skill_bindings": [{"id": "C123", "skill": "triage"}], + }) + adapter._bot_display_name = "HermesBot" + adapter._app.client.conversations_info = AsyncMock( + return_value={"ok": True, "channel": {"id": "C123", "name": "ops"}} + ) + adapter.set_authorization_check(lambda _user, _type, _chat: True) + + async def ordinary(ts): + return await adapter._build_message_event( + {"user": "U123", "channel": "C123"}, + text="what broke?", original_text="what broke?", + command_probe_text="what broke?", is_command_text=False, + channel_id="C123", team_id="T123", ts=ts, + user_id="U123", thread_ts=None, is_dm=False, + media_urls=[], media_types=[], media_text_inlined=[], + channel_context=None, + ) + + first = await ordinary("171.001") + await adapter._handle_slash_command({ + "command": "/hermes", + "text": "what broke?", + "user_id": "U123", + "channel_id": "C123", + "team_id": "T123", + }) + middle = adapter.handle_message.await_args.args[0] + last = await ordinary("171.003") + + runner = object.__new__(GatewayRunner) + runner._session_ephemeral_pin = {} + runner._session_vc_last = {} + runner._pending_turn_sidecar_notes = {} + runner._session_model_overrides = {} + runner._session_reasoning_overrides = {} + runner.adapters = {} + runner.session_store = MagicMock() + runner.config = GatewayConfig(platforms={Platform.SLACK: adapter.config}) + + context_pins = [] + channel_pins = [] + keys = [] + for event in (first, middle, last): + key = build_session_key(event.source) + keys.append(key) + context = build_session_context(event.source, runner.config) + context_pins.append( + runner._pinned_session_context_prompt(context, False, key) + ) + channel_prompt, _source = runner._pinned_channel_inputs( + key, event.channel_prompt, event.source, internal=False + ) + channel_pins.append(channel_prompt) + + assert len(set(keys)) == 1 + assert context_pins[1] is context_pins[0] + assert context_pins[2] is context_pins[0] + assert channel_pins == [channel_pins[0]] * 3 + assert channel_pins[0] and "Answer in haiku." in channel_pins[0] + assert [event.auto_skill for event in (first, middle, last)] == [ + ["triage"], ["triage"], ["triage"] + ] + assert { + (event.source.chat_name, event.source.user_name) + for event in (first, middle, last) + } == {("ops", "Test User")} + class TestSlackWorkspaceCollisionIsolation: @pytest.mark.asyncio diff --git a/tests/gateway/test_slack_approval_buttons.py b/tests/gateway/test_slack_approval_buttons.py index 2d436f473cb8..13b3128dc8ad 100644 --- a/tests/gateway/test_slack_approval_buttons.py +++ b/tests/gateway/test_slack_approval_buttons.py @@ -206,6 +206,126 @@ async def test_global_allowlist_blocks_unauthorized_click(self, monkeypatch): mock_resolve.assert_not_called() + @pytest.mark.asyncio + async def test_conversation_classification_gates_real_approval_resolution(self): + adapter = _make_adapter() + client = adapter._team_clients["T1"] + client.chat_update = AsyncMock() + + async def conversations_info(*, channel): + if channel == "G_DM": + return {"ok": True, "channel": {"id": channel, "is_mpim": True, "name": "mpdm"}} + if channel == "G_PRIVATE": + return {"ok": True, "channel": {"id": channel, "is_mpim": False, "name": "private"}} + raise AssertionError(channel) + + client.conversations_info = AsyncMock(side_effect=conversations_info) + adapter.set_authorization_check( + lambda user, chat_type, _chat: + (user == "U_DM" and chat_type == "dm") + or (user == "U_GROUP" and chat_type == "group") + ) + + async def click(channel_id, user_id, ts): + adapter._approval_resolved[ + adapter._workspace_message_marker("T1", ts) + ] = False + ack = AsyncMock() + body = { + "team": {"id": "T1"}, + "message": { + "ts": ts, + "blocks": [{ + "type": "section", + "text": {"type": "mrkdwn", "text": "Approve?"}, + }], + }, + "channel": {"id": channel_id}, + "user": {"name": user_id.lower(), "id": user_id}, + } + action = { + "action_id": "hermes_approve_once", + "value": "agent:main:slack:test", + } + with patch("tools.approval.resolve_gateway_approval", return_value=1) as resolve: + await adapter._handle_approval_action(ack, body, action) + ack.assert_awaited_once() + return resolve + + assert (await click("G_DM", "U_DM", "1.1")).call_count == 1 + assert (await click("G_DM", "U_GROUP", "1.2")).call_count == 0 + assert (await click("G_PRIVATE", "U_GROUP", "1.3")).call_count == 1 + assert (await click("G_PRIVATE", "U_DM", "1.4")).call_count == 0 + # A true 1:1 DM is unambiguous and uses the same DM principal policy. + assert (await click("D_DIRECT", "U_DM", "1.5")).call_count == 1 + + # The prompt may have been posted while DMs were enabled. Authorization is live at click + # time, so disabling DMs must prevent that outstanding MPIM approval from resolving. + pending_ts = "1.6" + adapter._approval_resolved[ + adapter._workspace_message_marker("T1", pending_ts) + ] = False + adapter.config.extra["disable_dms"] = True + assert (await click("G_DM", "U_DM", pending_ts)).call_count == 0 + # Private channels are not DMs and remain governed by group policy. + assert (await click("G_PRIVATE", "U_GROUP", "1.7")).call_count == 1 + + + + @pytest.mark.asyncio + async def test_approval_button_uses_session_key_when_g_metadata_is_unavailable(self): + """The prompt's own session key is authoritative for G... control authorization.""" + adapter = _make_adapter() + client = adapter._team_clients["T1"] + client.chat_update = AsyncMock() + client.conversations_info = AsyncMock( + side_effect=AssertionError("approval click performed conversations.info") + ) + adapter.set_authorization_check( + lambda user, chat_type, _chat: + (user == "U_GROUP" and chat_type == "group") + or (user == "U_DM_ONLY" and chat_type == "dm") + ) + + async def click(chat_type, user_id, ts): + source = adapter.build_source( + chat_id="G_PRIVATE" if chat_type == "group" else "G_DM", + chat_type=chat_type, + user_id="U_SESSION_OWNER", + scope_id="T1", + ) + session_key = adapter._source_session_key(source) + channel_id = source.chat_id + adapter._approval_resolved[ + adapter._workspace_message_marker("T1", ts) + ] = False + ack = AsyncMock() + body = { + "team": {"id": "T1"}, + "message": { + "ts": ts, + "blocks": [{ + "type": "section", + "text": {"type": "mrkdwn", "text": "Approve?"}, + }], + }, + "channel": {"id": channel_id}, + "user": {"name": user_id.lower(), "id": user_id}, + } + action = {"action_id": "hermes_approve_once", "value": session_key} + with patch("tools.approval.resolve_gateway_approval", return_value=1) as resolve: + await adapter._handle_approval_action(ack, body, action) + ack.assert_awaited_once() + return resolve + + # Legitimate legacy private-channel approval remains available with zero metadata I/O. + assert (await click("group", "U_GROUP", "2.1")).call_count == 1 + # Negative control: a key proving DM classification must not grant group-only privileges. + assert (await click("dm", "U_GROUP", "2.2")).call_count == 0 + # The DM-authorized principal still succeeds on the exact same DM-classified key path. + assert (await click("dm", "U_DM_ONLY", "2.3")).call_count == 1 + client.conversations_info.assert_not_awaited() + class TestSlackInteractiveAuth: def test_delegates_to_gateway_runner_auth(self): adapter = _make_adapter() diff --git a/tests/hermes_cli/test_goal_dispatch.py b/tests/hermes_cli/test_goal_dispatch.py index b718424d8e4d..e980a4a6104c 100644 --- a/tests/hermes_cli/test_goal_dispatch.py +++ b/tests/hermes_cli/test_goal_dispatch.py @@ -86,6 +86,75 @@ def test_surface_goal_state_matches_cli(surface, command, monkeypatch): assert snapshots[0] == snapshots[1] +def test_gateway_goal_kickoff_preserves_auto_skill_but_continuation_does_not(monkeypatch): + """Only the first queued goal turn consumes the triggering channel skill binding.""" + from gateway.config import Platform + from gateway.platforms.event import MessageEvent, MessageType + from gateway.session import SessionSource + from gateway.slash_commands_goals import GatewayGoalCommandsMixin + + runner = object.__new__(GatewayGoalCommandsMixin) + queued = [] + adapter = object() + runner._adapter_and_key_for = lambda _event: (adapter, "sk") + runner._enqueue_fifo = lambda key, turn, target: queued.append((key, turn, target)) + + event = MessageEvent( + text="/goal build it", + message_type=MessageType.COMMAND, + source=SessionSource( + platform=Platform.SLACK, + chat_id="C123", + chat_type="group", + user_id="U123", + ), + message_id="171.001", + channel_prompt="Answer in haiku.", + auto_skill=["triage"], + ) + + runner._enqueue_goal_turn(event, "kickoff", label="test", kickoff=True) + runner._enqueue_goal_turn(event, "continuation", label="test", kickoff=False) + + kickoff = queued[0][1] + continuation = queued[1][1] + assert kickoff.channel_prompt == "Answer in haiku." + assert kickoff.auto_skill == ["triage"] + assert kickoff.message_id == "171.001" + assert continuation.channel_prompt is None + assert continuation.auto_skill is None + assert continuation.message_id is None + + # Consumer boundary: the queued kickoff is the event _hmwa_prepare_turn feeds to the + # auto-skill loader on a fresh session. Exercise that real loader, not only the copied field. + monkeypatch.setattr( + "agent.skill_commands._load_skill_payload", + lambda name, task_id=None: (object(), "/tmp/triage", name), + ) + monkeypatch.setattr( + "agent.skill_commands._build_skill_message", + lambda _skill, _skill_dir, _header: "[SKILL SENTINEL: triage]", + ) + from gateway.run import GatewayRunner + consumer = object.__new__(GatewayRunner) + consumer._hmwa_auto_load_skills( + kickoff, kickoff.auto_skill, "quick-key", "session-key" + ) + assert kickoff.text == "[SKILL SENTINEL: triage]\n\nkickoff" + + # Direct free-form turn is the positive sibling through the same consumer. + direct = MessageEvent( + text="direct", + message_type=MessageType.TEXT, + source=event.source, + auto_skill=["triage"], + ) + consumer._hmwa_auto_load_skills( + direct, direct.auto_skill, "quick-key", "session-key" + ) + assert direct.text == "[SKILL SENTINEL: triage]\n\ndirect" + + @pytest.mark.parametrize('surface', ['cli', 'gateway', 'tui']) @pytest.mark.parametrize('draft_result', ['contract', 'unavailable', 'error']) def test_drafts_start_work_but_inspection_and_literal_prefixes_do_not_draft(