diff --git a/tests/tools/test_send_message_tool.py b/tests/tools/test_send_message_tool.py index 55c5165901b6..c733cefd4eb1 100644 --- a/tests/tools/test_send_message_tool.py +++ b/tests/tools/test_send_message_tool.py @@ -29,6 +29,7 @@ from gateway.config import Platform from tools.send_message_tool import ( _is_telegram_thread_not_found, _parse_target_ref, + _resolve_slack_user_target, _send_matrix_via_adapter, _send_signal, _send_telegram, @@ -1620,7 +1621,7 @@ class TestParseTargetRefWhatsAppJID: class TestParseTargetRefSlack: - """_parse_target_ref recognizes Slack channel/user IDs as explicit.""" + """_parse_target_ref recognizes Slack conversation and user targets.""" def test_thread_target_is_explicit(self): chat_id, thread_id, is_explicit = _parse_target_ref("slack", "C0B0QV5434G:171.000001") @@ -1640,12 +1641,22 @@ class TestParseTargetRefSlack: 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 + def test_user_id_is_explicit_dm_target(self): + """Slack user IDs are explicit user targets that must be opened as DMs.""" + chat_id, thread_id, is_explicit = _parse_target_ref("slack", "U123ABCDEF") + assert chat_id == "user:U123ABCDEF" + assert thread_id is None + assert is_explicit is True + + def test_at_username_is_explicit_dm_target(self): + """slack:@username targets resolve to DMs through users.list + conversations.open.""" + chat_id, thread_id, is_explicit = _parse_target_ref("slack", "@alice") + assert chat_id == "user_name:alice" + assert thread_id is None + assert is_explicit is True + + def test_workspace_id_is_not_explicit(self): + """Slack workspace IDs (W...) are not sendable conversation/user targets.""" assert _parse_target_ref("slack", "W123ABCDEF")[2] is False def test_whitespace_is_stripped(self): @@ -1747,6 +1758,129 @@ class TestEmailHomeChannelErrorHint: assert "TELEGRAM_HOME_CHANNEL" in result["error"] +class TestResolveSlackUserTargets: + """_resolve_slack_user_target opens user targets as DMs before sending. + + Adapted from #19237's ``_send_slack`` tests: main moved Slack delivery to + the plugin's ``_standalone_send`` (#41112), so the salvaged DM-open logic + lives in a resolution helper that runs before any send path. + """ + + @staticmethod + def _mock_response(data): + response = MagicMock() + response.json = AsyncMock(return_value=data) + response.__aenter__ = AsyncMock(return_value=response) + response.__aexit__ = AsyncMock(return_value=None) + return response + + @staticmethod + def _mock_session(*responses): + session = MagicMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=None) + session.post = MagicMock(side_effect=responses) + return session + + def test_conversation_ids_pass_through_without_api_calls(self): + for cid in ("C0B0QV5434G", "G123ABCDEF", "D123ABCDEF"): + chat_id, err = asyncio.run(_resolve_slack_user_target("tok", cid)) + assert chat_id == cid + assert err is None + + def test_user_id_target_opens_dm(self): + session = self._mock_session( + self._mock_response({"ok": True, "channel": {"id": "D123ABCDEF"}}), + ) + + with patch("aiohttp.ClientSession", return_value=session): + chat_id, err = asyncio.run( + _resolve_slack_user_target("tok", "user:U123ABCDEF") + ) + + assert err is None + assert chat_id == "D123ABCDEF" + open_payload = session.post.call_args_list[0].kwargs["json"] + assert open_payload == {"users": "U123ABCDEF"} + + def test_username_target_resolves_user_then_opens_dm(self): + session = self._mock_session( + self._mock_response({ + "ok": True, + "members": [ + {"id": "UOTHER123", "name": "someone", "profile": {"display_name": "Other", "real_name": "Other User"}}, + {"id": "U123ABCDEF", "name": "alice", "profile": {"display_name": "Alice", "real_name": "Alice Example"}}, + ], + "response_metadata": {}, + }), + self._mock_response({"ok": True, "channel": {"id": "D123ABCDEF"}}), + ) + + with patch("aiohttp.ClientSession", return_value=session): + chat_id, err = asyncio.run( + _resolve_slack_user_target("tok", "user_name:alice") + ) + + assert err is None + assert chat_id == "D123ABCDEF" + assert session.post.call_args_list[1].kwargs["json"] == {"users": "U123ABCDEF"} + + def test_username_target_does_not_match_display_or_real_name(self): + session = self._mock_session( + self._mock_response({ + "ok": True, + "members": [ + {"id": "U123ABCDEF", "name": "notalice", "profile": {"display_name": "alice", "real_name": "alice"}}, + ], + "response_metadata": {}, + }), + ) + + with patch("aiohttp.ClientSession", return_value=session): + chat_id, err = asyncio.run( + _resolve_slack_user_target("tok", "user_name:alice") + ) + + assert chat_id is None + assert "Could not resolve Slack user '@alice'" in err["error"] + assert session.post.call_count == 1 + + def test_ambiguous_username_returns_error_without_opening_dm(self): + session = self._mock_session( + self._mock_response({ + "ok": True, + "members": [ + {"id": "U111AAAAA", "name": "alice", "profile": {}}, + {"id": "U222BBBBB", "name": "alice", "profile": {}}, + ], + "response_metadata": {}, + }), + ) + + with patch("aiohttp.ClientSession", return_value=session): + chat_id, err = asyncio.run( + _resolve_slack_user_target("tok", "user_name:alice") + ) + + assert chat_id is None + assert "matched multiple Slack users" in err["error"] + assert session.post.call_count == 1 + + def test_conversations_open_failure_surfaces_error(self): + session = self._mock_session( + self._mock_response({"ok": False, "error": "missing_scope"}), + ) + + with patch("aiohttp.ClientSession", return_value=session): + chat_id, err = asyncio.run( + _resolve_slack_user_target("tok", "user:U123ABCDEF") + ) + + assert chat_id is None + assert "missing_scope" in err["error"] + assert "im:write" in err["error"] + + class TestSendDiscordThreadId: """_send_discord uses thread_id when provided.""" diff --git a/tools/send_message_tool.py b/tools/send_message_tool.py index 41e99dfb2ebb..1719158f8b71 100644 --- a/tools/send_message_tool.py +++ b/tools/send_message_tool.py @@ -22,12 +22,15 @@ 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 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*([CGDU][A-Z0-9]{8,})\s*$") +# Must be uppercase alphanumeric, 9+ chars. User IDs (U...) are parsed as +# explicit user targets (``user:U...``) and are converted to D... conversations +# via conversations.open before chat.postMessage — posting directly to a U/W +# ID fails because the API requires a conversation ID. ``@handle`` targets are +# resolved through users.list first (``user_name:...``). +_SLACK_TARGET_RE = re.compile(r"^\s*([CGD][A-Z0-9]{8,})\s*$") +_SLACK_USER_ID_RE = re.compile(r"^\s*(U[A-Z0-9]{8,})\s*$") +_SLACK_USER_NAME_RE = re.compile(r"^\s*@([A-Za-z0-9._-]{1,80})\s*$") +_SLACK_MENTION_RE = re.compile(r"^\s*<@(U[A-Z0-9]{8,})(?:\|[^>]+)?>\s*$") # Session-derived Slack thread targets use ":". _SLACK_THREAD_TARGET_RE = re.compile(r"^\s*([CGD][A-Z0-9]{8,}):([^\s:]+)\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*$") @@ -465,27 +468,22 @@ def _handle_send(args): if duplicate_skip: return json.dumps(duplicate_skip) - # Slack: resolve user IDs (U...) to DM channel IDs via conversations.open - if platform_name == "slack" and chat_id and chat_id.startswith("U"): - try: - import aiohttp - async def _open_slack_dm(token, user_id): - url = "https://slack.com/api/conversations.open" - headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"} - async with aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) as session: - async with session.post(url, headers=headers, json={"users": [user_id]}) as resp: - data = await resp.json() - if data.get("ok"): - return data["channel"]["id"] - return None + # Slack: resolve user targets to DM channel IDs before sending. + # _parse_target_ref emits internal ``user:U...`` / ``user_name:@handle`` + # targets; a bare U... id can also arrive from session metadata or the + # home-channel config. All are opened via conversations.open (fixes #19236). + if platform_name == "slack" and chat_id: + _slack_dm_target = chat_id + if _slack_dm_target.startswith("U") and _SLACK_USER_ID_RE.fullmatch(_slack_dm_target): + _slack_dm_target = f"user:{_slack_dm_target}" + if _slack_dm_target.startswith(("user:", "user_name:")): from model_tools import _run_async - dm_channel = _run_async(_open_slack_dm(pconfig.token, chat_id)) - if dm_channel: - chat_id = dm_channel - else: - return json.dumps({"error": f"Could not open DM with Slack user {chat_id}. Check bot permissions (im:write)."}) - except Exception as e: - return json.dumps({"error": f"Failed to open Slack DM: {e}"}) + _resolved, _resolve_err = _run_async( + _resolve_slack_user_target(pconfig.token, _slack_dm_target) + ) + if _resolve_err: + return json.dumps(_resolve_err) + chat_id = _resolved try: from model_tools import _run_async @@ -556,14 +554,13 @@ def _parse_target_ref(platform_name: str, target_ref: str): return match.group(1), match.group(2), True match = _SLACK_TARGET_RE.fullmatch(target_ref) if match: - chat_id = match.group(1) - # Slack user IDs (U...) and workspace IDs (W...) are NOT valid - # explicit send targets — chat.postMessage rejects them. A DM - # must be opened first via conversations.open to get a D... - # conversation ID. Caller still gets the chat_id so the U→D - # resolution path in send_message() can run. - is_explicit = chat_id[0] not in {"U", "W"} - return chat_id, None, is_explicit + return match.group(1), None, True + match = _SLACK_USER_ID_RE.fullmatch(target_ref) or _SLACK_MENTION_RE.fullmatch(target_ref) + if match: + return f"user:{match.group(1)}", None, True + match = _SLACK_USER_NAME_RE.fullmatch(target_ref) + if match: + return f"user_name:{match.group(1)}", None, True if platform_name == "matrix": trimmed = target_ref.strip() split_idx = trimmed.rfind(":$") @@ -1500,6 +1497,84 @@ async def _registry_standalone_send(platform_name, pconfig, chat_id, message, th # wired via standalone_sender_fn and reached through _registry_standalone_send. #41112. +async def _resolve_slack_user_target(token, chat_id): + """Resolve a Slack user target to a D... DM conversation ID. + + ``chat_id`` may be a Slack conversation ID (C/G/D...) — returned unchanged — + or an internal user target (``user:U...`` / ``user_name:``). User + targets are opened as DMs via conversations.open because Slack + chat.postMessage requires a conversation ID. ``user_name:`` targets are + first resolved to a user ID through users.list (stable handle match only). + + Returns ``(chat_id, None)`` on success or ``(None, error_dict)`` on failure. + """ + if not (chat_id.startswith("user:") or chat_id.startswith("user_name:")): + return chat_id, None + try: + import aiohttp + except ImportError: + return None, {"error": "aiohttp not installed. Run: pip install aiohttp"} + try: + from gateway.platforms.base import resolve_proxy_url, proxy_kwargs_for_aiohttp + _proxy = resolve_proxy_url() + _sess_kw, _req_kw = proxy_kwargs_for_aiohttp(_proxy) + base_url = "https://slack.com/api" + headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"} + + async def post_api(session, method, payload): + async with session.post(f"{base_url}/{method}", headers=headers, json=payload, **_req_kw) as resp: + return await resp.json() + + async def resolve_user_name(session, name): + query = name.strip().lstrip("@").lower() + matches = [] + cursor = None + for _page in range(20): + payload = {"limit": 200} + if cursor: + payload["cursor"] = cursor + data = await post_api(session, "users.list", payload) + if not data.get("ok"): + return None, f"Slack users.list error: {data.get('error', 'unknown')}" + for member in data.get("members", []): + if member.get("deleted") or member.get("is_bot"): + continue + # ``@name`` should match the stable Slack handle only. Display + # and real names are mutable/non-unique enough that using them + # could DM the wrong person with sensitive content. + if str(member.get("name", "")).strip().lower() == query: + matches.append(member) + cursor = (data.get("response_metadata") or {}).get("next_cursor") + if not cursor: + break + if not matches: + return None, f"Could not resolve Slack user '@{name}'." + if len(matches) > 1: + return None, f"Slack user '@{name}' matched multiple Slack users. Use a Slack user ID instead." + return matches[0].get("id"), None + + async with aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=30), **_sess_kw) as session: + if chat_id.startswith("user_name:"): + user_id, error = await resolve_user_name(session, chat_id[len("user_name:"):]) + if error: + return None, _error(error) + chat_id = f"user:{user_id}" + + user_id = chat_id[len("user:"):] + opened = await post_api(session, "conversations.open", {"users": user_id}) + if not opened.get("ok"): + return None, _error( + f"Slack conversations.open error: {opened.get('error', 'unknown')}. " + "Check bot permissions (im:write)." + ) + dm_id = (opened.get("channel") or {}).get("id") + if not dm_id: + return None, _error("Slack conversations.open did not return a DM channel ID") + return dm_id, None + except Exception as e: + return None, _error(f"Slack DM resolution failed: {e}") + + async def _send_signal(extra, chat_id, message, media_files=None): """Send via signal-cli JSON-RPC API.