Slack send_message channel targeting and ID resolution

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #16198 fix(slack): resolve channels by raw ID + enumerate joined channels for send_message

  merged to main

  files: gateway/channel_directory.py, gateway/run.py, scripts/release.py, tests/gateway/test_channel_directory.py, tests/tools/test_send_message_tool.py, tools/send_message_tool.py

  Show the diff

  • 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()
    +
    +    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
     
    -    # Fallback to session data
    -    return _build_from_sessions("slack")
    +    # 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")

  • gateway/run.py

    @@ -2334,7 +2334,7 @@ async def start(self) -> bool:
             # 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 @@ async def _platform_reconnect_watcher(self) -> None:
                             # 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)
     

  • scripts/release.py

    @@ -120,6 +120,7 @@
         "[email protected]": "sgaofen",
         "[redacted:email]": "zhiyanliu",
         "[redacted:email]": "hhhonzik",
    +    "[email protected]": "hhuang91",
         "[email protected]": "nocoo",
         "[email protected]": "n-WN",
         "[redacted:email]": "hiddenpuppy",

  • 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 @@
         format_directory_for_display,
         load_directory,
         _build_from_sessions,
    +    _build_slack,
         DIRECTORY_PATH,
     )
     
    @@ -62,7 +65,7 @@ def broken_dump(data, fp, *args, **kwargs):
             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 @@ def test_topic_name_resolves_to_composite_id(self, tmp_path):
             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 @@ def test_channel_without_type_key_returns_none(self, tmp_path):
             }
             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 == []

  • tests/tools/test_send_message_tool.py

    @@ -810,6 +810,44 @@ def test_e164_prefix_only_matches_phone_platforms(self):
             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_not_explicit(self):
    +        """Slack user IDs (U...) and workspace IDs (W...) are NOT explicit send
    +        targets. chat.postMessage rejects them — a DM must be opened first via
    +        conversations.open to obtain a D... conversation ID.
    +        """
    +        assert _parse_target_ref("slack", "U123ABCDEF")[2] is False
    +        assert _parse_target_ref("slack", "W123ABCDEF")[2] is False
    +
    +    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."""
     

  • tools/send_message_tool.py

    @@ -20,6 +20,13 @@
     
     _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 conversation IDs: C (public channel), G (private/group channel), D (DM).
    +# Must be uppercase alphanumeric, 9+ chars. User IDs (U...) and workspace IDs
    +# (W...) are NOT valid chat.postMessage channel values — posting to them fails
    +# because the API requires a conversation ID. To DM a user you must first call
    +# conversations.open to obtain a D... ID. Without this gate, Slack IDs fall
    +# through to channel-name resolution, which only matches by name and fails.
    +_SLACK_TARGET_RE = re.compile(r"^\s*([CGD][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 +325,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: