diff --git a/gateway/channel_directory.py b/gateway/channel_directory.py index 2489b718f8..94936ac9dd 100644 --- a/gateway/channel_directory.py +++ b/gateway/channel_directory.py @@ -57,7 +57,7 @@ def _session_entry_name(origin: Dict[str, Any]) -> str: # Build / refresh # --------------------------------------------------------------------------- -def build_channel_directory(adapters: Dict[Any, Any]) -> Dict[str, Any]: +async def build_channel_directory(adapters: Dict[Any, Any]) -> Dict[str, Any]: """ Build a channel directory from connected platform adapters and session data. @@ -72,7 +72,7 @@ def build_channel_directory(adapters: Dict[Any, Any]) -> Dict[str, Any]: if platform == Platform.DISCORD: platforms["discord"] = _build_discord(adapter) elif platform == Platform.SLACK: - platforms["slack"] = _build_slack(adapter) + platforms["slack"] = await _build_slack(adapter) except Exception as e: logger.warning("Channel directory: failed to build %s: %s", platform.value, e) @@ -136,21 +136,66 @@ def _build_discord(adapter) -> List[Dict[str, str]]: return channels -def _build_slack(adapter) -> List[Dict[str, str]]: - """List Slack channels the bot has joined.""" - # Slack adapter may expose a web client - client = getattr(adapter, "_app", None) or getattr(adapter, "_client", None) - if not client: +async def _build_slack(adapter) -> List[Dict[str, Any]]: + """List Slack channels the bot has joined across all workspaces. + + Uses ``users.conversations`` against each workspace's web client. Pulls + public + private channels the bot is a member of, then merges in DMs + discovered from session history (IMs aren't useful to enumerate + proactively). + """ + team_clients = getattr(adapter, "_team_clients", None) or {} + if not team_clients: return _build_from_sessions("slack") - try: - from tools.send_message_tool import _send_slack # noqa: F401 - # Use the Slack Web API directly if available - except Exception: - pass + channels: List[Dict[str, Any]] = [] + seen_ids: set = set() - # Fallback to session data - return _build_from_sessions("slack") + for team_id, client in team_clients.items(): + try: + cursor: Optional[str] = None + for _page in range(20): # safety cap on pagination + response = await client.users_conversations( + types="public_channel,private_channel", + exclude_archived=True, + limit=200, + cursor=cursor, + ) + if not response.get("ok"): + logger.warning( + "Channel directory: users.conversations not ok for team %s: %s", + team_id, + response.get("error", "unknown"), + ) + break + for ch in response.get("channels", []): + cid = ch.get("id") + name = ch.get("name") + if not cid or not name or cid in seen_ids: + continue + seen_ids.add(cid) + channels.append({ + "id": cid, + "name": name, + "type": "private" if ch.get("is_private") else "channel", + }) + cursor = (response.get("response_metadata") or {}).get("next_cursor") + if not cursor: + break + except Exception as e: + logger.warning( + "Channel directory: failed to list Slack channels for team %s: %s", + team_id, e, + ) + continue + + # Merge in DM/group entries discovered from session history. + for entry in _build_from_sessions("slack"): + if entry.get("id") not in seen_ids: + channels.append(entry) + seen_ids.add(entry.get("id")) + + return channels def _build_from_sessions(platform_name: str) -> List[Dict[str, str]]: @@ -223,6 +268,14 @@ def resolve_channel_name(platform_name: str, name: str) -> Optional[str]: if not channels: return None + # 0. Exact ID match — case-sensitive, no normalization. Lets callers pass + # raw platform IDs (e.g. Slack "C0B0QV5434G") even when the format guard + # in _parse_target_ref hasn't recognized them as explicit. + raw = name.strip() + for ch in channels: + if ch.get("id") == raw: + return ch["id"] + query = _normalize_channel_query(name) # 1. Exact name match, including the display labels shown by send_message(action="list") diff --git a/gateway/run.py b/gateway/run.py index ea768ca6e0..23be3793d7 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -2334,7 +2334,7 @@ class GatewayRunner: # Build initial channel directory for send_message name resolution try: from gateway.channel_directory import build_channel_directory - directory = build_channel_directory(self.adapters) + directory = await build_channel_directory(self.adapters) ch_count = sum(len(chs) for chs in directory.get("platforms", {}).values()) logger.info("Channel directory built: %d target(s)", ch_count) except Exception as e: @@ -2618,7 +2618,7 @@ class GatewayRunner: # Rebuild channel directory with the new adapter try: from gateway.channel_directory import build_channel_directory - build_channel_directory(self.adapters) + await build_channel_directory(self.adapters) except Exception: pass else: @@ -10978,7 +10978,15 @@ def _start_cron_ticker(stop_event: threading.Event, adapters=None, loop=None, in if tick_count % CHANNEL_DIR_EVERY == 0 and adapters: try: from gateway.channel_directory import build_channel_directory - build_channel_directory(adapters) + if loop is not None: + # build_channel_directory is async (Slack web calls), and + # this ticker runs in a background thread. Schedule onto + # the gateway event loop and wait briefly for completion + # so refresh failures are still logged via the except. + fut = asyncio.run_coroutine_threadsafe( + build_channel_directory(adapters), loop + ) + fut.result(timeout=30) except Exception as e: logger.debug("Channel directory refresh error: %s", e) diff --git a/tests/gateway/test_channel_directory.py b/tests/gateway/test_channel_directory.py index 6c1b8fc731..cdaf2c540c 100644 --- a/tests/gateway/test_channel_directory.py +++ b/tests/gateway/test_channel_directory.py @@ -1,9 +1,11 @@ """Tests for gateway/channel_directory.py — channel resolution and display.""" +import asyncio import json import os from pathlib import Path -from unittest.mock import patch +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch from gateway.channel_directory import ( build_channel_directory, @@ -12,6 +14,7 @@ from gateway.channel_directory import ( format_directory_for_display, load_directory, _build_from_sessions, + _build_slack, DIRECTORY_PATH, ) @@ -62,7 +65,7 @@ class TestBuildChannelDirectoryWrites: monkeypatch.setattr(json, "dump", broken_dump) with patch("gateway.channel_directory.DIRECTORY_PATH", cache_file): - build_channel_directory({}) + asyncio.run(build_channel_directory({})) result = load_directory() assert result == previous @@ -142,6 +145,21 @@ class TestResolveChannelName: with self._setup(tmp_path, platforms): assert resolve_channel_name("telegram", "Coaching Chat / topic 17585") == "-1001:17585" + def test_id_match_takes_precedence_over_name(self, tmp_path): + """A raw channel ID resolves to itself, even when a different + channel happens to be named the same string. Case-sensitive: Slack + IDs are uppercase and must not be normalized away.""" + platforms = { + "slack": [ + {"id": "C0B0QV5434G", "name": "engineering", "type": "channel"}, + {"id": "C99", "name": "c0b0qv5434g", "type": "channel"}, + ] + } + with self._setup(tmp_path, platforms): + assert resolve_channel_name("slack", "C0B0QV5434G") == "C0B0QV5434G" + # Lowercase still falls through to name matching (case-insensitive) + assert resolve_channel_name("slack", "c0b0qv5434g") == "C99" + def test_display_label_with_type_suffix_resolves(self, tmp_path): platforms = { "telegram": [ @@ -332,3 +350,135 @@ class TestLookupChannelType: } with self._setup(tmp_path, platforms): assert lookup_channel_type("discord", "300") is None + + +def _make_slack_adapter(team_clients): + """Build a stand-in for SlackAdapter exposing only ``_team_clients``.""" + return SimpleNamespace(_team_clients=team_clients) + + +def _make_slack_client(pages): + """Build an AsyncWebClient mock whose ``users_conversations`` returns pages.""" + client = MagicMock() + client.users_conversations = AsyncMock(side_effect=pages) + return client + + +class TestBuildSlack: + """_build_slack actually calls users.conversations on each workspace client.""" + + def test_no_team_clients_falls_back_to_sessions(self, tmp_path): + sessions_path = tmp_path / "sessions" / "sessions.json" + sessions_path.parent.mkdir(parents=True) + sessions_path.write_text(json.dumps({ + "s1": {"origin": {"platform": "slack", "chat_id": "D123", "chat_name": "Alice"}}, + })) + + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({}))) + + assert len(entries) == 1 + assert entries[0]["id"] == "D123" + + def test_lists_channels_from_users_conversations(self, tmp_path): + client = _make_slack_client([ + { + "ok": True, + "channels": [ + {"id": "C0B0QV5434G", "name": "engineering", "is_private": False}, + {"id": "G123ABCDEF", "name": "secret-chat", "is_private": True}, + ], + "response_metadata": {}, + }, + ]) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) + + ids = {e["id"] for e in entries} + assert ids == {"C0B0QV5434G", "G123ABCDEF"} + types = {e["id"]: e["type"] for e in entries} + assert types["C0B0QV5434G"] == "channel" + assert types["G123ABCDEF"] == "private" + client.users_conversations.assert_awaited_once() + + def test_paginates_via_response_metadata_cursor(self, tmp_path): + client = _make_slack_client([ + { + "ok": True, + "channels": [{"id": "C001", "name": "first", "is_private": False}], + "response_metadata": {"next_cursor": "cur1"}, + }, + { + "ok": True, + "channels": [{"id": "C002", "name": "second", "is_private": False}], + "response_metadata": {"next_cursor": ""}, + }, + ]) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) + + assert {e["id"] for e in entries} == {"C001", "C002"} + assert client.users_conversations.await_count == 2 + + def test_per_workspace_error_does_not_block_others(self, tmp_path): + bad = MagicMock() + bad.users_conversations = AsyncMock(side_effect=RuntimeError("boom")) + good = _make_slack_client([ + { + "ok": True, + "channels": [{"id": "C999", "name": "ok-channel", "is_private": False}], + "response_metadata": {}, + }, + ]) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({"BAD": bad, "GOOD": good}))) + + assert {e["id"] for e in entries} == {"C999"} + + def test_session_dms_merged_when_not_in_api_results(self, tmp_path): + sessions_path = tmp_path / "sessions" / "sessions.json" + sessions_path.parent.mkdir(parents=True) + sessions_path.write_text(json.dumps({ + "s1": {"origin": {"platform": "slack", "chat_id": "D456", "chat_name": "Bob"}}, + "dup": {"origin": {"platform": "slack", "chat_id": "C001", "chat_name": "first"}}, + })) + client = _make_slack_client([ + { + "ok": True, + "channels": [{"id": "C001", "name": "first", "is_private": False}], + "response_metadata": {}, + }, + ]) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) + + ids = {e["id"] for e in entries} + assert "C001" in ids and "D456" in ids + # Channel ID from API should not be duplicated by the session merge + assert sum(1 for e in entries if e["id"] == "C001") == 1 + + def test_skips_channels_with_no_id_or_name(self, tmp_path): + client = _make_slack_client([ + { + "ok": True, + "channels": [ + {"id": "C001", "name": "good", "is_private": False}, + {"id": "", "name": "no-id"}, + {"id": "C002"}, # no name (e.g. IM) + ], + "response_metadata": {}, + }, + ]) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) + + assert {e["id"] for e in entries} == {"C001"} + + def test_response_not_ok_breaks_pagination_for_that_workspace(self, tmp_path): + client = _make_slack_client([ + {"ok": False, "error": "missing_scope"}, + ]) + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + entries = asyncio.run(_build_slack(_make_slack_adapter({"T1": client}))) + + assert entries == [] diff --git a/tests/tools/test_send_message_tool.py b/tests/tools/test_send_message_tool.py index 626179de19..60f71af69d 100644 --- a/tests/tools/test_send_message_tool.py +++ b/tests/tools/test_send_message_tool.py @@ -810,6 +810,40 @@ class TestParseTargetRefE164: assert _parse_target_ref("matrix", "+15551234567")[2] is False +class TestParseTargetRefSlack: + """_parse_target_ref recognizes Slack channel/user IDs as explicit.""" + + def test_public_channel_id_is_explicit(self): + chat_id, thread_id, is_explicit = _parse_target_ref("slack", "C0B0QV5434G") + assert chat_id == "C0B0QV5434G" + assert thread_id is None + assert is_explicit is True + + def test_private_channel_id_is_explicit(self): + assert _parse_target_ref("slack", "G123ABCDEF")[2] is True + + def test_dm_id_is_explicit(self): + assert _parse_target_ref("slack", "D123ABCDEF")[2] is True + + def test_user_id_is_explicit(self): + assert _parse_target_ref("slack", "U123ABCDEF")[2] is True + assert _parse_target_ref("slack", "W123ABCDEF")[2] is True + + def test_whitespace_is_stripped(self): + chat_id, _, is_explicit = _parse_target_ref("slack", " C0B0QV5434G ") + assert chat_id == "C0B0QV5434G" + assert is_explicit is True + + def test_lowercase_or_short_id_is_not_explicit(self): + assert _parse_target_ref("slack", "c0b0qv5434g")[2] is False + assert _parse_target_ref("slack", "C123")[2] is False + assert _parse_target_ref("slack", "X0B0QV5434G")[2] is False + + def test_slack_id_not_explicit_for_other_platforms(self): + assert _parse_target_ref("discord", "C0B0QV5434G")[2] is False + assert _parse_target_ref("telegram", "C0B0QV5434G")[2] is False + + class TestSendDiscordThreadId: """_send_discord uses thread_id when provided.""" diff --git a/tools/send_message_tool.py b/tools/send_message_tool.py index 19da4f55af..cbf7e042e1 100644 --- a/tools/send_message_tool.py +++ b/tools/send_message_tool.py @@ -20,6 +20,10 @@ logger = logging.getLogger(__name__) _TELEGRAM_TOPIC_TARGET_RE = re.compile(r"^\s*(-?\d+)(?::(\d+))?\s*$") _FEISHU_TARGET_RE = re.compile(r"^\s*((?:oc|ou|on|chat|open)_[-A-Za-z0-9]+)(?::([-A-Za-z0-9_]+))?\s*$") +# Slack channel/user IDs: C (public), G (private/group), D (DM), U/W (user). +# Always uppercase alphanumeric, 9+ chars. Without this, Slack IDs fall through +# to channel-name resolution, which only matches by name and fails. +_SLACK_TARGET_RE = re.compile(r"^\s*([CGDUW][A-Z0-9]{8,})\s*$") _WEIXIN_TARGET_RE = re.compile(r"^\s*((?:wxid|gh|v\d+|wm|wb)_[A-Za-z0-9_-]+|[A-Za-z0-9._-]+@chatroom|filehelper)\s*$") # Discord snowflake IDs are numeric, same regex pattern as Telegram topic targets. _NUMERIC_TOPIC_RE = _TELEGRAM_TOPIC_TARGET_RE @@ -318,6 +322,10 @@ def _parse_target_ref(platform_name: str, target_ref: str): match = _NUMERIC_TOPIC_RE.fullmatch(target_ref) if match: return match.group(1), match.group(2), True + if platform_name == "slack": + match = _SLACK_TARGET_RE.fullmatch(target_ref) + if match: + return match.group(1), None, True if platform_name == "weixin": match = _WEIXIN_TARGET_RE.fullmatch(target_ref) if match: