mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-22 16:25:58 +00:00
fix(slack): open DMs for user send targets
This commit is contained in:
davesecops
Teknium
parent
99cafc8eac
commit
3997b0907e
2 changed files with 250 additions and 41 deletions
|
|
@ -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."""
|
||||
|
||||
|
|
|
|||
|
|
@ -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 "<conversation_id>:<thread_ts>".
|
||||
_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:<handle>``). 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.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue