From 9cf2cbd38245574d994dbb64fdb0ca30315cc711 Mon Sep 17 00:00:00 2001 From: Nikita Barkov Date: Thu, 30 Jul 2026 08:47:56 +0200 Subject: [PATCH] fix(slack): read real SDK responses instead of gating on isinstance dict Slack Web API calls return `SlackResponse`/`AsyncSlackResponse`, which are mapping-like but not `dict` subclasses, so every `isinstance(resp, dict)` gate took its "unexpected shape" branch at runtime: user and channel names collapsed to raw IDs, every user resolved as a non-bot (defeating the allow_bots loop guard), ephemeral replies were reported as failures, and uploads/caption fallbacks lost their message_id. Normalize responses through a single `_slack_response_payload()` helper (dict passes through, SDK response yields `.data`, anything else yields `{}` so callers keep their fallbacks) and use it at every call site. Existing Slack tests injected plain dicts, which is why the defect was invisible; the new tests run each behavioral case against a real `AsyncSlackResponse` as well. --- plugins/platforms/slack/adapter.py | 75 ++++--- tests/gateway/test_slack_sdk_response.py | 250 +++++++++++++++++++++++ 2 files changed, 297 insertions(+), 28 deletions(-) create mode 100644 tests/gateway/test_slack_sdk_response.py diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py index ecc90bb969259..624e9e6fd3070 100644 --- a/plugins/platforms/slack/adapter.py +++ b/plugins/platforms/slack/adapter.py @@ -107,6 +107,22 @@ async def _read_error_text_limited( return str(text)[:limit] +def _slack_response_payload(response: Any) -> Dict[str, Any]: + """Return a Slack Web API response as a plain dict. + + ``slack_sdk`` returns ``SlackResponse``/``AsyncSlackResponse``, which is + mapping-like but is **not** a ``dict``, while tests inject plain dicts. + Callers must normalize here instead of gating on ``isinstance(resp, dict)`` + — such a gate is always False at runtime and silently degrades results + (user/channel names collapsing to raw IDs, sends reported as failures). + An unrecognized shape yields ``{}`` so callers keep their fallbacks. + """ + if isinstance(response, dict): + return response + data = getattr(response, "data", None) + return data if isinstance(data, dict) else {} + + _SLACK_SPECIAL_MENTION_RE = re.compile( r"\n]*)?>", re.IGNORECASE ) @@ -1638,10 +1654,11 @@ class SlackAdapter(BasePlatformAdapter): user=user_id, text=chunk, ) - if not (isinstance(result, dict) and result.get("ok")): + payload = _slack_response_payload(result) + if not payload.get("ok"): err = ( - result.get("error", "unknown_error") - if isinstance(result, dict) + payload.get("error", "unknown_error") + if payload else "unexpected_response" ) return SendResult( @@ -2256,11 +2273,7 @@ class SlackAdapter(BasePlatformAdapter): channel=parent_chat_id, text=seed_text, ) - ts = ( - result.get("ts") - if isinstance(result, dict) - else getattr(result, "get", lambda _k, _d=None: None)("ts") - ) + ts = _slack_response_payload(result).get("ts") if ts: return str(ts) except Exception as exc: @@ -3812,11 +3825,12 @@ class SlackAdapter(BasePlatformAdapter): else self._app.client ) result = await client.users_info(user=user_id) - if not isinstance(result, dict): + payload = _slack_response_payload(result) + if not payload: self._user_is_bot_cache[cache_key] = False self._user_name_cache[cache_key] = user_id return user_id - user = result.get("user", {}) + user = payload.get("user", {}) profile = user.get("profile", {}) if isinstance(user, dict) else {} self._user_is_bot_cache[cache_key] = bool( user.get("is_bot") @@ -3865,10 +3879,11 @@ class SlackAdapter(BasePlatformAdapter): resp = await self._get_client( channel_id, team_id=team_id or None ).conversations_info(channel=channel_id) - if not isinstance(resp, dict) or not resp.get("ok"): + payload = _slack_response_payload(resp) + if not payload.get("ok"): name = channel_id else: - ch = resp.get("channel") or {} + ch = payload.get("channel") or {} if ch.get("is_im"): peer_user = ch.get("user", "") name = ( @@ -3992,11 +4007,12 @@ class SlackAdapter(BasePlatformAdapter): else self._app.client ) result = await client.users_info(user=user_id) - if not isinstance(result, dict): + payload = _slack_response_payload(result) + if not payload: self._user_is_bot_cache[cache_key] = False self._user_name_cache.setdefault(cache_key, user_id) return False - user = result.get("user", {}) + user = payload.get("user", {}) profile = user.get("profile", {}) if isinstance(user, dict) else {} is_bot = bool( user.get("is_bot") @@ -8581,12 +8597,13 @@ async def _standalone_upload_file( if thread_id: kwargs["thread_ts"] = thread_id result = await client.files_upload_v2(**kwargs) - if isinstance(result, dict) and result.get("ok") is False: - return {"error": f"Slack API error: {result.get('error', 'unknown')}"} + payload = _slack_response_payload(result) + if payload.get("ok") is False: + return {"error": f"Slack API error: {payload.get('error', 'unknown')}"} # files_upload_v2 responses vary by sdk version; prefer file timestamp when present. message_id = None - if isinstance(result, dict): - file_obj = result.get("file") or {} + if payload: + file_obj = payload.get("file") or {} shares = file_obj.get("shares") or {} for share_bucket in shares.values(): if isinstance(share_bucket, dict): @@ -8596,7 +8613,7 @@ async def _standalone_upload_file( break if message_id: break - message_id = message_id or file_obj.get("timestamp") or result.get("ts") + message_id = message_id or file_obj.get("timestamp") or payload.get("ts") return {"success": True, "message_id": message_id, "raw": result} @@ -8728,14 +8745,14 @@ async def _standalone_send( if thread_id: post_kwargs["thread_ts"] = thread_id try: - post_resp = await client.chat_postMessage(**post_kwargs) - if isinstance(post_resp, dict) and not post_resp.get("ok", True): - return { - "error": f"Slack API error: {post_resp.get('error', 'unknown')}" - } - last_message_id = ( - post_resp.get("ts") if isinstance(post_resp, dict) else None + post_payload = _slack_response_payload( + await client.chat_postMessage(**post_kwargs) ) + if not post_payload.get("ok", True): + return { + "error": f"Slack API error: {post_payload.get('error', 'unknown')}" + } + last_message_id = post_payload.get("ts") except Exception as e: return {"error": f"Slack send failed: {e}"} @@ -8756,8 +8773,10 @@ async def _standalone_send( } if thread_id: fallback_kwargs["thread_ts"] = thread_id - fb = await client.chat_postMessage(**fallback_kwargs) - if isinstance(fb, dict) and fb.get("ok", True): + fb = _slack_response_payload( + await client.chat_postMessage(**fallback_kwargs) + ) + if fb.get("ok", True): last_message_id = fb.get("ts") or last_message_id caption_pending = False except Exception: diff --git a/tests/gateway/test_slack_sdk_response.py b/tests/gateway/test_slack_sdk_response.py new file mode 100644 index 0000000000000..c1844de7c025d --- /dev/null +++ b/tests/gateway/test_slack_sdk_response.py @@ -0,0 +1,250 @@ +"""Slack Web API responses must be read as real SDK responses, not dicts. + +``slack_sdk`` returns ``SlackResponse``/``AsyncSlackResponse`` — mapping-like +objects that are **not** ``dict`` subclasses. Code that gated on +``isinstance(resp, dict)`` therefore took its "unexpected shape" branch on +every real call, collapsing user/channel names to raw IDs, treating every user +as a non-bot, and reporting successful sends as failures. Existing Slack tests +injected plain dicts, so the defect was invisible to them; every case here +exercises the SDK-shaped response as well. +""" + +import asyncio +import sys +from unittest.mock import AsyncMock, MagicMock + +import pytest + + +# Import the real response class first: the mock installer below fills +# sys.modules with MagicMocks, which would mask an installed slack_sdk. +try: + from slack_sdk.web.async_slack_response import ( # noqa: E402 + AsyncSlackResponse as _AsyncSlackResponse, + ) +except Exception: # pragma: no cover - slack extra not installed + _AsyncSlackResponse = None + + +# --------------------------------------------------------------------------- +# Mock slack-bolt if not installed (same pattern as test_slack_mention.py) +# --------------------------------------------------------------------------- + +def _ensure_slack_mock(): + 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), + ]: + sys.modules.setdefault(name, mod) + + +_ensure_slack_mock() + +import plugins.platforms.slack.adapter as _slack_mod # noqa: E402 + +_slack_mod.SLACK_AVAILABLE = True + +from plugins.platforms.slack.adapter import ( # noqa: E402 + SlackAdapter, + _slack_response_payload, + _standalone_upload_file, +) + + +class _SdkLikeResponse: + """Stand-in for ``AsyncSlackResponse``: mapping-like, but not a ``dict``.""" + + def __init__(self, data): + self.data = data + + def get(self, key, default=None): + return self.data.get(key, default) + + def __getitem__(self, key): + return self.data.get(key) + + +def _real_sdk_response(data): + """Build an actual ``AsyncSlackResponse`` — the shape production sees.""" + return _AsyncSlackResponse( + client=None, + http_verb="POST", + api_url="https://slack.com/api/users.info", + req_args={}, + data=data, + headers={}, + status_code=200, + ) + + +# Every behavioral case runs against both the hand-rolled stand-in and, when +# installed, the real SDK class — mocks alone are what hid this bug. +_FACTORIES = [pytest.param(_SdkLikeResponse, id="sdk-like")] +if isinstance(_AsyncSlackResponse, type): + _FACTORIES.append(pytest.param(_real_sdk_response, id="slack_sdk")) + +response_shape = pytest.mark.parametrize("make_response", _FACTORIES) + + +def _make_adapter(): + # object.__new__ skips __init__ (heavy setup) — established slack-test pattern. + adapter = object.__new__(SlackAdapter) + adapter._app = MagicMock() + adapter._user_name_cache = {} + adapter._user_is_bot_cache = {} + adapter._channel_name_cache = {} + adapter._channel_team = {} + adapter._USER_NAME_CACHE_MAX = 5000 + adapter._CHANNEL_NAME_CACHE_MAX = 5000 + return adapter + + +# ── the normalizer's contract ─────────────────────────────────────────────── + + +class TestSlackResponsePayload: + def test_plain_dict_passes_through(self): + payload = {"ok": True} + assert _slack_response_payload(payload) is payload + + @response_shape + def test_sdk_response_yields_its_data(self, make_response): + assert _slack_response_payload(make_response({"ok": True})) == {"ok": True} + + @response_shape + def test_sdk_response_is_not_a_dict(self, make_response): + """The premise of the bug: the runtime object fails an isinstance dict gate.""" + assert not isinstance(make_response({"ok": True}), dict) + + @response_shape + def test_binary_response_is_not_mistaken_for_data(self, make_response): + """``SlackResponse.data`` may be bytes; callers need their fallback then.""" + assert _slack_response_payload(make_response(b"\x89PNG")) == {} + + def test_unknown_shape_yields_empty(self): + assert _slack_response_payload(object()) == {} + assert _slack_response_payload(None) == {} + + +# ── the call sites that silently degraded ────────────────────────────────── + + +class TestIdentityResolution: + @response_shape + def test_user_name_resolves(self, make_response): + """The reported symptom: names collapsed to the raw user id.""" + adapter = _make_adapter() + adapter._app.client.users_info = AsyncMock( + return_value=make_response( + {"ok": True, "user": {"profile": {"display_name": "Nikita"}}} + ) + ) + name = asyncio.run(adapter._resolve_user_name("U_HUMAN")) + assert name == "Nikita" + + @response_shape + def test_user_is_bot_resolves(self, make_response): + """allow_bots policy depends on this; a wrong False re-opens routing loops.""" + adapter = _make_adapter() + adapter._app.client.users_info = AsyncMock( + return_value=make_response( + {"ok": True, "user": {"is_bot": True, "profile": {}}} + ) + ) + assert asyncio.run(adapter._resolve_user_is_bot("U_PEER_BOT")) is True + + @response_shape + def test_channel_name_resolves(self, make_response): + adapter = _make_adapter() + client = MagicMock() + client.conversations_info = AsyncMock( + return_value=make_response({"ok": True, "channel": {"name": "general"}}) + ) + adapter._get_client = lambda *_a, **_kw: client + assert asyncio.run(adapter._resolve_channel_name("C_GEN")) == "general" + + def test_unknown_shape_still_falls_back_to_the_id(self): + """Degradation for genuinely unreadable responses must be preserved.""" + adapter = _make_adapter() + adapter._app.client.users_info = AsyncMock(return_value=object()) + assert asyncio.run(adapter._resolve_user_name("U_HUMAN")) == "U_HUMAN" + + +class TestSendPaths: + @response_shape + def test_ephemeral_reply_is_reported_as_delivered(self, make_response): + """Slash-command replies were reported as 'unexpected_response' failures.""" + adapter = _make_adapter() + client = MagicMock() + client.chat_postEphemeral = AsyncMock( + return_value=make_response({"ok": True}) + ) + adapter._get_client = lambda *_a, **_kw: client + result = asyncio.run( + adapter._post_ephemeral_fallback("C_GEN", {"user_id": "U_HUMAN"}, "hi") + ) + assert result.success is True + + @response_shape + def test_ephemeral_error_is_surfaced(self, make_response): + adapter = _make_adapter() + client = MagicMock() + client.chat_postEphemeral = AsyncMock( + return_value=make_response({"ok": False, "error": "channel_not_found"}) + ) + adapter._get_client = lambda *_a, **_kw: client + result = asyncio.run( + adapter._post_ephemeral_fallback("C_GEN", {"user_id": "U_HUMAN"}, "hi") + ) + assert result.success is False + assert "channel_not_found" in result.error + + @response_shape + def test_upload_returns_the_message_id(self, make_response, tmp_path): + """A lost message_id breaks threading of follow-up sends.""" + media = tmp_path / "note.txt" + media.write_text("x") + client = MagicMock() + client.files_upload_v2 = AsyncMock( + return_value=make_response( + {"ok": True, "file": {"timestamp": "123.456"}} + ) + ) + result = asyncio.run( + _standalone_upload_file(client, "C_GEN", str(media)) + ) + assert result == { + "success": True, + "message_id": "123.456", + "raw": client.files_upload_v2.return_value, + } + + @response_shape + def test_upload_error_is_surfaced(self, make_response, tmp_path): + media = tmp_path / "note.txt" + media.write_text("x") + client = MagicMock() + client.files_upload_v2 = AsyncMock( + return_value=make_response({"ok": False, "error": "not_in_channel"}) + ) + result = asyncio.run( + _standalone_upload_file(client, "C_GEN", str(media)) + ) + assert "not_in_channel" in result["error"]