From d9fe008db8eb315174b039849454c5008e04e568 Mon Sep 17 00:00:00 2001 From: replygirl Date: Thu, 23 Jul 2026 08:20:37 -0700 Subject: [PATCH] fix(slack): prefer live send adapter and try multi-workspace tokens individually MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related Slack delivery fixes for send_message text sends: - Route Slack text delivery through _send_via_adapter so the live in-process gateway adapter (multi-workspace aware, channel→client mapping, adapter-side gates) is preferred, with the plugin's _standalone_send as the out-of-process fallback — matching how the media path already behaves. - _standalone_send: SLACK_BOT_TOKEN can be a comma-separated list in multi-workspace installs and slack_tokens.json carries OAuth per-workspace tokens; the standalone Web-API path used to send the literal comma-joined string, which Slack rejects as invalid_auth. Try each token individually, retrying on token-scoped errors (invalid_auth / not_in_channel / channel_not_found …) and stopping on terminal ones. User-DM resolution (U…/W… targets) also tries each token. Adapted from #47547 by @replygirl — the original patched the legacy tools/send_message_tool.py::_send_slack helper, which moved to the Slack plugin's _standalone_send in #41112. Salvaged from #47547 --- plugins/platforms/slack/adapter.py | 62 ++++++++-- tests/tools/test_send_message_slack.py | 163 +++++++++++++++++++++++++ tools/send_message_tool.py | 20 +-- 3 files changed, 224 insertions(+), 21 deletions(-) create mode 100644 tests/tools/test_send_message_slack.py diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index f8c486d12fad3..a34c995cc81f5 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -8606,9 +8606,29 @@ async def _standalone_send( ``chat.postMessage``. """ del force_document # signature parity with other standalone senders - token = getattr(pconfig, "token", None) or os.getenv("SLACK_BOT_TOKEN", "") - if not token: + raw_token = getattr(pconfig, "token", None) or os.getenv("SLACK_BOT_TOKEN", "") + + # ``SLACK_BOT_TOKEN`` can be a comma-separated list in multi-workspace + # gateways, and OAuth installs persist per-workspace tokens in + # slack_tokens.json. The standalone path has no team→client map, so try + # each token individually instead of sending the literal comma-joined + # string, which Slack rejects as ``invalid_auth`` (#47547). + tokens = [t.strip() for t in str(raw_token or "").split(",") if t.strip()] + try: + from hermes_constants import get_hermes_home + + _tokens_file = get_hermes_home() / "slack_tokens.json" + if _tokens_file.exists(): + _saved = json.loads(_tokens_file.read_text(encoding="utf-8")) + for _entry in _saved.values(): + _tok = _entry.get("token", "") if isinstance(_entry, dict) else "" + if _tok and _tok not in tokens: + tokens.append(_tok) + except Exception: + pass + if not tokens: return {"error": "Slack send failed: SLACK_BOT_TOKEN not configured"} + token = tokens[0] # User-targeted delivery: chat.postMessage / files_upload_v2 reject bare # user IDs (U.../W...) — resolve to a DM conversation ID (D...) first via @@ -8616,7 +8636,12 @@ async def _standalone_send( # instead of failing with channel_not_found (#17444). chat_id = str(chat_id or "") if chat_id[:1] in ("U", "W"): - resolved = await _resolve_slack_user_dm(token, chat_id) + resolved = None + for _tok in tokens: + resolved = await _resolve_slack_user_dm(_tok, chat_id) + if resolved is not None: + token = _tok + break if resolved is None: return { "error": ( @@ -8771,20 +8796,32 @@ async def _standalone_send( _proxy = resolve_proxy_url() _sess_kw, _req_kw = proxy_kwargs_for_aiohttp(_proxy) url = "https://slack.com/api/chat.postMessage" - headers = { - "Authorization": f"Bearer {token}", - "Content-Type": "application/json", + # Errors that mean "wrong workspace token for this channel" — worth + # retrying with the next token. Anything else is terminal. + retryable_token_errors = { + "invalid_auth", + "not_authed", + "token_revoked", + "account_inactive", + "not_in_channel", + "channel_not_found", } + last_error = "unknown" async with aiohttp.ClientSession( timeout=aiohttp.ClientTimeout(total=30), **_sess_kw ) as session: payload = {"channel": chat_id, "text": formatted, "mrkdwn": True} if thread_id: payload["thread_ts"] = thread_id - async with session.post( - url, headers=headers, json=payload, **_req_kw - ) as resp: - data = await resp.json() + for tok in tokens: + headers = { + "Authorization": f"Bearer {tok}", + "Content-Type": "application/json", + } + async with session.post( + url, headers=headers, json=payload, **_req_kw + ) as resp: + data = await resp.json() if data.get("ok"): return { "success": True, @@ -8792,7 +8829,10 @@ async def _standalone_send( "chat_id": chat_id, "message_id": data.get("ts"), } - return {"error": f"Slack API error: {data.get('error', 'unknown')}"} + last_error = data.get("error", "unknown") + if last_error not in retryable_token_errors: + break + return {"error": f"Slack API error: {last_error}"} except Exception as e: return {"error": f"Slack send failed: {e}"} diff --git a/tests/tools/test_send_message_slack.py b/tests/tools/test_send_message_slack.py new file mode 100644 index 0000000000000..ab1381353efd9 --- /dev/null +++ b/tests/tools/test_send_message_slack.py @@ -0,0 +1,163 @@ +"""Slack-specific send_message delivery regressions. + +Salvaged from #47547 and adapted to the post-#41112 plugin layout: the legacy +``_send_slack`` helper moved to ``plugins/platforms/slack/adapter.py:: +_standalone_send`` and text sends now route through ``_send_via_adapter`` +(live adapter first, registry standalone fallback). +""" + +import asyncio +import sys +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from gateway.config import Platform +from tools.send_message_tool import _send_to_platform + + +def _ensure_slack_mock(monkeypatch): + """Install lightweight Slack modules when optional Slack deps are absent.""" + if "slack_bolt" in sys.modules and hasattr(sys.modules["slack_bolt"], "__file__"): + return + + slack_bolt = MagicMock() + slack_bolt.async_app.AsyncApp = MagicMock + slack_bolt.adapter.socket_mode.async_handler.AsyncSocketModeHandler = MagicMock + + slack_sdk = MagicMock() + slack_sdk.web.async_client.AsyncWebClient = MagicMock + + for name, mod in [ + ("slack_bolt", slack_bolt), + ("slack_bolt.async_app", slack_bolt.async_app), + ("slack_bolt.adapter", slack_bolt.adapter), + ("slack_bolt.adapter.socket_mode", slack_bolt.adapter.socket_mode), + ("slack_bolt.adapter.socket_mode.async_handler", slack_bolt.adapter.socket_mode.async_handler), + ("slack_sdk", slack_sdk), + ("slack_sdk.web", slack_sdk.web), + ("slack_sdk.web.async_client", slack_sdk.web.async_client), + ]: + monkeypatch.setitem(sys.modules, name, mod) + + +def test_slack_send_to_platform_routes_through_send_via_adapter(monkeypatch): + """Slack text sends go through _send_via_adapter (live adapter first).""" + _ensure_slack_mock(monkeypatch) + + live_send = AsyncMock(return_value={"success": True, "message_id": "live-ts"}) + + with patch("tools.send_message_tool._send_via_adapter", live_send): + result = asyncio.run( + _send_to_platform( + Platform.SLACK, + SimpleNamespace(enabled=True, token="bad-token,good-token", extra={}), + "C123", + "**hello** from Hermes", + thread_id="171.1", + ) + ) + + assert result == {"success": True, "message_id": "live-ts"} + live_send.assert_awaited_once() + call = live_send.await_args + assert call.args[0] == Platform.SLACK + assert call.args[2] == "C123" + assert call.kwargs["thread_id"] == "171.1" + + +class _SlackResponse: + def __init__(self, payload): + self._payload = payload + + async def json(self): + return self._payload + + +class _SlackPostContext: + def __init__(self, response): + self._response = response + + async def __aenter__(self): + return self._response + + async def __aexit__(self, exc_type, exc, tb): + return False + + +class _SlackSession: + """Fake aiohttp session whose good-token posts succeed.""" + + def __init__(self): + self.calls = [] + + async def __aenter__(self): + return self + + async def __aexit__(self, exc_type, exc, tb): + return False + + def post(self, url, *, headers, json, **kwargs): + token = headers["Authorization"].removeprefix("Bearer ") + self.calls.append((token, json)) + if token == "good-token": + payload = {"ok": True, "ts": "171.123"} + else: + payload = {"ok": False, "error": "invalid_auth"} + return _SlackPostContext(_SlackResponse(payload)) + + +@pytest.fixture +def _standalone_send(monkeypatch): + _ensure_slack_mock(monkeypatch) + from plugins.platforms.slack import adapter as slack_adapter + + return slack_adapter._standalone_send + + +def test_standalone_send_tries_comma_separated_tokens_individually( + monkeypatch, _standalone_send +): + """Multi-workspace token lists must not be sent as one literal token.""" + fake_session = _SlackSession() + monkeypatch.setattr( + "aiohttp.ClientSession", lambda *args, **kwargs: fake_session + ) + + pconfig = SimpleNamespace(enabled=True, token="bad-token, good-token", extra={}) + result = asyncio.run(_standalone_send(pconfig, "C123", "hello")) + + assert result == { + "success": True, + "platform": "slack", + "chat_id": "C123", + "message_id": "171.123", + } + assert [token for token, _payload in fake_session.calls] == [ + "bad-token", + "good-token", + ] + + +def test_standalone_send_stops_on_non_token_error(monkeypatch, _standalone_send): + """Terminal errors (not token-scoped) must not burn the remaining tokens.""" + + class _FatalSession(_SlackSession): + def post(self, url, *, headers, json, **kwargs): + token = headers["Authorization"].removeprefix("Bearer ") + self.calls.append((token, json)) + return _SlackPostContext( + _SlackResponse({"ok": False, "error": "msg_too_long"}) + ) + + fake_session = _FatalSession() + monkeypatch.setattr( + "aiohttp.ClientSession", lambda *args, **kwargs: fake_session + ) + + pconfig = SimpleNamespace(enabled=True, token="tok-a,tok-b", extra={}) + result = asyncio.run(_standalone_send(pconfig, "C123", "hello")) + + assert result == {"error": "Slack API error: msg_too_long"} + assert len(fake_session.calls) == 1 diff --git a/tools/send_message_tool.py b/tools/send_message_tool.py index bf02b607ade17..9c1d41eaf0fd7 100644 --- a/tools/send_message_tool.py +++ b/tools/send_message_tool.py @@ -1071,20 +1071,20 @@ async def _send_to_platform(platform, pconfig, chat_id, message, thread_id=None, last_result = result return last_result - # --- Slack: route both text and native files through the plugin's - # standalone sender. This path is used by out-of-process cron runs where - # no live gateway adapter is available; dropping ``media_files`` here made - # MEDIA directives disappear while the text delivery still reported - # success. + # --- Slack: prefer the live gateway adapter, then the plugin's + # standalone sender. The live adapter is multi-workspace aware (it maps + # channels to the workspace client that owns them) and honors adapter-side + # gates like ignored_channels; the standalone Web-API path may only have a + # comma-separated token list. ``_send_via_adapter`` tries the in-process + # adapter first and falls back to the registry standalone sender for + # out-of-process cron runs, preserving MEDIA delivery on the fallback + # (media-bearing sends were already intercepted by the branch above). if platform == Platform.SLACK: - from gateway.platform_registry import platform_registry - entry = platform_registry.get("slack") - if entry is None or entry.standalone_sender_fn is None: - return {"error": "Slack plugin not registered or missing standalone_sender_fn"} last_result = None for i, chunk in enumerate(chunks): is_last = i == len(chunks) - 1 - result = await entry.standalone_sender_fn( + result = await _send_via_adapter( + platform, pconfig, chat_id, chunk,