Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 67 additions & 14 deletions gateway/channel_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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

Expand Down Expand Up @@ -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]]:
Expand Down Expand Up @@ -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")
Expand Down
14 changes: 11 additions & 3 deletions gateway/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -2182,7 +2182,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:
Expand Down Expand Up @@ -2466,7 +2466,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:
Expand Down Expand Up @@ -10886,7 +10886,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)

Expand Down
154 changes: 152 additions & 2 deletions tests/gateway/test_channel_directory.py
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -12,6 +14,7 @@
format_directory_for_display,
load_directory,
_build_from_sessions,
_build_slack,
DIRECTORY_PATH,
)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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": [
Expand Down Expand Up @@ -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 == []
34 changes: 34 additions & 0 deletions tests/tools/test_send_message_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,40 @@ 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_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."""

Expand Down
8 changes: 8 additions & 0 deletions tools/send_message_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@

_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
Expand Down Expand Up @@ -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:
Expand Down