mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-28 18:19:28 +00:00
fix(slack): resolve bare user targets to DMs across all adapter send paths
Widen the #19237 send_message fix to the live adapter: send, _upload_file, send_multiple_images, send_image, send_video, send_document, send_exec_approval, send_slash_confirm, and send_clarify now route bare Slack user IDs (U.../W...) through a shared _ensure_dm_conversation helper before calling chat.postMessage / files_upload_v2, which reject user IDs. This closes the gap in #17261 where an attachment worked when replying in a thread but failed when directed at a user DM, and extends the DM-open fallback to clarify/approval Block Kit prompts so gated actions can reach a user directly. Resolution uses the workspace-scoped client (multi-workspace installs open the DM with the right bot token), caches per (team, user), and records the opened D... channel in the channel→team map. On failure the original target passes through so the downstream API call surfaces the real Slack error. Fixes #17261 Refs #19236
This commit is contained in:
Teknium
parent
4ab4894f44
commit
994a405f2d
2 changed files with 231 additions and 0 deletions
|
|
@ -744,6 +744,8 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
# the primary client meanwhile.
|
||||
self._channel_team: Dict[str, str] = {}
|
||||
self._CHANNEL_TEAM_MAX = 10000
|
||||
# user target (team_id:user_id) → opened DM conversation ID (D...)
|
||||
self._dm_conversation_cache: Dict[str, str] = {}
|
||||
# Dedup cache: prevents duplicate bot responses when Socket Mode
|
||||
# reconnects redeliver events (#4777). The TTL must outlast Slack's
|
||||
# worst-case reconnect-redelivery gap, not just a few seconds — the
|
||||
|
|
@ -1966,6 +1968,7 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
self._team_clients = {}
|
||||
self._team_bot_user_ids = {}
|
||||
self._channel_team = {}
|
||||
self._dm_conversation_cache = {}
|
||||
|
||||
self._release_platform_lock()
|
||||
|
||||
|
|
@ -1992,6 +1995,49 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
return self._team_clients[team_id]
|
||||
return self._app.client # fallback to primary
|
||||
|
||||
async def _ensure_dm_conversation(
|
||||
self, chat_id: str, team_id: Optional[str] = None
|
||||
) -> str:
|
||||
"""Resolve a bare Slack user ID target to a DM conversation ID.
|
||||
|
||||
``chat.postMessage`` and ``files_upload_v2`` reject user IDs (U.../W...)
|
||||
— a DM must be opened first via ``conversations.open`` to obtain a D...
|
||||
conversation ID (#19236 / #17261). Conversation IDs (C/G/D...) pass
|
||||
through unchanged. Resolution goes through the workspace-scoped client
|
||||
so multi-workspace installs open the DM with the right bot token, and
|
||||
results are cached per (team, user) so repeated sends don't re-open.
|
||||
|
||||
Returns the resolved conversation ID, or the original ``chat_id`` when
|
||||
resolution is not applicable or fails (the downstream API call then
|
||||
surfaces the real Slack error).
|
||||
"""
|
||||
cid = str(chat_id or "")
|
||||
if not cid or cid[0] not in ("U", "W"):
|
||||
return chat_id
|
||||
cache_key = f"{team_id or ''}:{cid}"
|
||||
cached = self._dm_conversation_cache.get(cache_key)
|
||||
if cached:
|
||||
return cached
|
||||
try:
|
||||
response = await self._get_client(cid, team_id=team_id).conversations_open(
|
||||
users=cid
|
||||
)
|
||||
dm_id = ((response or {}).get("channel") or {}).get("id")
|
||||
if dm_id:
|
||||
self._dm_conversation_cache[cache_key] = dm_id
|
||||
# DM belongs to the same workspace as the user target.
|
||||
if team_id:
|
||||
self._channel_team[dm_id] = team_id
|
||||
return dm_id
|
||||
except Exception as e:
|
||||
logger.warning(
|
||||
"[Slack] conversations.open failed for user target %s: %s "
|
||||
"(check the bot's im:write scope)",
|
||||
cid,
|
||||
e,
|
||||
)
|
||||
return chat_id
|
||||
|
||||
async def send(
|
||||
self,
|
||||
chat_id: str,
|
||||
|
|
@ -2003,6 +2049,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
thread_ts = None
|
||||
try:
|
||||
# Check for a pending slash-command context. When the user ran a
|
||||
|
|
@ -2536,6 +2585,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
if not os.path.exists(file_path):
|
||||
raise FileNotFoundError(f"File not found: {file_path}")
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
thread_ts = self._resolve_thread_ts(reply_to, metadata)
|
||||
last_exc = None
|
||||
for attempt in range(3):
|
||||
|
|
@ -2586,6 +2638,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
if not images:
|
||||
return
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
try:
|
||||
import httpx as _httpx
|
||||
from urllib.parse import unquote as _unquote
|
||||
|
|
@ -3261,6 +3316,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
response.raise_for_status()
|
||||
|
||||
thread_ts = self._resolve_thread_ts(reply_to, metadata)
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
result = await self._get_client(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
).files_upload_v2(
|
||||
|
|
@ -3334,6 +3392,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
success=False, error=f"Video file not found: {video_path}"
|
||||
)
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
try:
|
||||
thread_ts = self._resolve_thread_ts(reply_to, metadata)
|
||||
last_exc = None
|
||||
|
|
@ -3396,6 +3457,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
|
||||
display_name = file_name or os.path.basename(file_path)
|
||||
thread_ts = self._resolve_thread_ts(reply_to, metadata)
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
|
||||
try:
|
||||
last_exc = None
|
||||
|
|
@ -5176,6 +5240,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
try:
|
||||
thread_ts = self._resolve_thread_ts(None, metadata)
|
||||
|
||||
|
|
@ -5270,6 +5337,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
try:
|
||||
thread_ts = self._resolve_thread_ts(None, metadata)
|
||||
# Same 3000-char section-block cap as send_exec_approval: budget
|
||||
|
|
@ -5374,6 +5444,9 @@ class SlackAdapter(BasePlatformAdapter):
|
|||
if not self._app:
|
||||
return SendResult(success=False, error="Not connected")
|
||||
|
||||
chat_id = await self._ensure_dm_conversation(
|
||||
chat_id, team_id=self._metadata_team_id(metadata)
|
||||
)
|
||||
try:
|
||||
thread_ts = self._resolve_thread_ts(None, metadata)
|
||||
|
||||
|
|
|
|||
|
|
@ -7021,3 +7021,161 @@ class TestTrackingStructureBounds:
|
|||
# can never remove a newer entry while an older one remains).
|
||||
for i in range(450, 500):
|
||||
assert f"{2000 + i}.000000" in adapter._bot_message_ts
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# TestEnsureDmConversation — bare user-ID targets resolve to DM channels
|
||||
# (#19236 / #17261: attachments and Block Kit prompts to U... targets)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestEnsureDmConversation:
|
||||
@pytest.mark.asyncio
|
||||
async def test_user_id_target_is_opened_as_dm(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
|
||||
resolved = await adapter._ensure_dm_conversation("U123ABCDEF")
|
||||
|
||||
assert resolved == "D999NEW"
|
||||
adapter._app.client.conversations_open.assert_awaited_once_with(
|
||||
users="U123ABCDEF"
|
||||
)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_conversation_ids_pass_through(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock()
|
||||
for cid in ("C123CHAN", "G123GROUP", "D123DM"):
|
||||
assert await adapter._ensure_dm_conversation(cid) == cid
|
||||
adapter._app.client.conversations_open.assert_not_awaited()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolution_is_cached_per_user(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
|
||||
first = await adapter._ensure_dm_conversation("U123ABCDEF")
|
||||
second = await adapter._ensure_dm_conversation("U123ABCDEF")
|
||||
|
||||
assert first == second == "D999NEW"
|
||||
adapter._app.client.conversations_open.assert_awaited_once()
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_failure_returns_original_target(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
side_effect=Exception("missing_scope")
|
||||
)
|
||||
|
||||
resolved = await adapter._ensure_dm_conversation("U123ABCDEF")
|
||||
|
||||
assert resolved == "U123ABCDEF"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_workspace_scoped_client_used_for_team_id(self, adapter):
|
||||
team_client = AsyncMock()
|
||||
team_client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D_TEAM2"}}
|
||||
)
|
||||
adapter._team_clients["T_SECOND"] = team_client
|
||||
adapter._app.client.conversations_open = AsyncMock()
|
||||
|
||||
resolved = await adapter._ensure_dm_conversation(
|
||||
"U123ABCDEF", team_id="T_SECOND"
|
||||
)
|
||||
|
||||
assert resolved == "D_TEAM2"
|
||||
team_client.conversations_open.assert_awaited_once_with(users="U123ABCDEF")
|
||||
adapter._app.client.conversations_open.assert_not_awaited()
|
||||
# The opened DM is recorded as belonging to the same workspace.
|
||||
assert adapter._channel_team["D_TEAM2"] == "T_SECOND"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_resolves_user_target_before_posting(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
adapter._app.client.chat_postMessage = AsyncMock(
|
||||
return_value={"ok": True, "ts": "111.222"}
|
||||
)
|
||||
|
||||
result = await adapter.send("U123ABCDEF", "hello there")
|
||||
|
||||
assert result.success is True
|
||||
post_kwargs = adapter._app.client.chat_postMessage.await_args.kwargs
|
||||
assert post_kwargs["channel"] == "D999NEW"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_upload_file_resolves_user_target(self, adapter, tmp_path):
|
||||
media = tmp_path / "report.pdf"
|
||||
media.write_bytes(b"%PDF-1.4 fake")
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
adapter._app.client.files_upload_v2 = AsyncMock(
|
||||
return_value={"ok": True, "file": {"id": "F1"}}
|
||||
)
|
||||
|
||||
result = await adapter._upload_file("U123ABCDEF", str(media))
|
||||
|
||||
assert result.success is True
|
||||
upload_kwargs = adapter._app.client.files_upload_v2.await_args.kwargs
|
||||
assert upload_kwargs["channel"] == "D999NEW"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_document_resolves_user_target(self, adapter, tmp_path):
|
||||
media = tmp_path / "notes.md"
|
||||
media.write_bytes(b"# notes")
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
adapter._app.client.files_upload_v2 = AsyncMock(
|
||||
return_value={"ok": True, "file": {"id": "F1"}}
|
||||
)
|
||||
|
||||
result = await adapter.send_document("U123ABCDEF", str(media))
|
||||
|
||||
assert result.success is True
|
||||
upload_kwargs = adapter._app.client.files_upload_v2.await_args.kwargs
|
||||
assert upload_kwargs["channel"] == "D999NEW"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_clarify_resolves_user_target(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
adapter._app.client.chat_postMessage = AsyncMock(
|
||||
return_value={"ok": True, "ts": "111.222"}
|
||||
)
|
||||
|
||||
result = await adapter.send_clarify(
|
||||
chat_id="U123ABCDEF",
|
||||
question="Which one?",
|
||||
choices=["a", "b"],
|
||||
clarify_id="cl-1",
|
||||
session_key="sk-1",
|
||||
)
|
||||
|
||||
assert result.success is True
|
||||
post_kwargs = adapter._app.client.chat_postMessage.await_args.kwargs
|
||||
assert post_kwargs["channel"] == "D999NEW"
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_send_exec_approval_resolves_user_target(self, adapter):
|
||||
adapter._app.client.conversations_open = AsyncMock(
|
||||
return_value={"ok": True, "channel": {"id": "D999NEW"}}
|
||||
)
|
||||
adapter._app.client.chat_postMessage = AsyncMock(
|
||||
return_value={"ok": True, "ts": "111.222"}
|
||||
)
|
||||
|
||||
result = await adapter.send_exec_approval(
|
||||
chat_id="U123ABCDEF",
|
||||
command="rm -rf /tmp/x",
|
||||
session_key="sk-1",
|
||||
)
|
||||
|
||||
assert result.success is True
|
||||
post_kwargs = adapter._app.client.chat_postMessage.await_args.kwargs
|
||||
assert post_kwargs["channel"] == "D999NEW"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue