fix(Slack): resolve Slack channels by raw ID and enumerate joined channels

send_message(target='slack:<channel_id>') failed with "Could not
resolve" because _parse_target_ref had no Slack branch — Slack's
uppercase alphanumeric IDs fell through to channel-name resolution,
which only matched by name. As a fallback, the agent would retry with
bare target='slack' and post to the home channel instead.

Three fixes:

- _parse_target_ref recognizes Slack IDs (C/G/D/U/W prefix) as
  explicit targets so the name-resolver is bypassed entirely.
- resolve_channel_name tries a case-sensitive raw-ID match before
  the existing name match, so any platform's IDs resolve cleanly.
- _build_slack now actually calls users.conversations against each
  workspace's AsyncWebClient (paginated), instead of only returning
  session-history entries. This populates the directory with public
  and private channels the bot has joined, so action='list' shows
  them and they can also be addressed by name. Errors from one
  workspace don't block others.

build_channel_directory becomes async (Slack web calls require it).
The two async-context callers in gateway/run.py are awaited; the
cron ticker thread call bridges via asyncio.run_coroutine_threadsafe.

Slack bot needs channels:read and groups:read scopes for full
enumeration; missing scopes degrade gracefully per-workspace.

addressing #15927
This commit is contained in:
hhuang91 2026-04-26 03:51:20 -04:00 committed by Teknium
parent 541cd732e8
commit 802c7acb81
5 changed files with 272 additions and 19 deletions

View file

@ -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")

View file

@ -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)

View file

@ -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 == []

View file

@ -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."""

View file

@ -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: