RomanNum3ral
/
hermes-agent
mirror of https://github.com/NousResearch/hermes-agent.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Nikita Barkov 2026-07-30 08:47:56 +02:00 committed by Teknium
parent 91e550b0cf
commit 9cf2cbd382
2 changed files with 297 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

75
plugins/platforms/slack/adapter.py
View File

@ -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"<!(?:everyone|channel|here)(?:\|[^>\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:

250
tests/gateway/test_slack_sdk_response.py Normal file
View File

@ -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"]