fix(slack): read real SDK responses instead of gating on isinstance dict

[nikitaBarkov]

What does this PR do?

Slack user names, channel names and bot detection are silently broken on main: the agent receives [U0BCE4NRVKN | Slack user <@U0BCE4NRVKN>] instead of [Nikita | Slack user <@U0BCE4NRVKN>], every user resolves as a non-bot, and some send paths report success as failure.

Root cause. slack_sdk Web API calls return SlackResponse / AsyncSlackResponse. Those objects are mapping-like (they expose .get() and .data) but they are not dict subclasses. Commit 3f08201ba ("Fix Slack peer bot status routing loops", #51627) added isinstance(result, dict) guards around those responses, so at runtime the guard is always False and every guarded call site takes its "unexpected shape" degradation branch:

• _resolve_user_name → the name becomes the raw user id, and that wrong value is cached for the lifetime of the gateway process. This is the user-visible symptom.
• _resolve_user_is_bot → every user resolves as a non-bot, which defeats the allow_bots peer-bot loop guard that 3f08201ba was written to add.
• _resolve_channel_name → the channel name degrades to C0….
• _post_ephemeral_fallback → a successful ephemeral reply is reported as an unexpected_response failure.
• _standalone_upload_file and the standalone chat.postMessage path → message_id is lost (breaking threading of follow-up sends) and the caption fallback is never marked delivered.

Fix. Normalize every Slack response through one helper, _slack_response_payload(): a plain dict passes through, an SDK response yields .data, and anything else (including a binary .data, which files_* can return) yields {} so callers keep their existing fallbacks. This fixes the whole bug class rather than the one reported site — all eight guarded call sites are converted.

Why the test suite did not catch it. The existing Slack tests inject plain dicts into the mocked client, so the guard is True in tests and False in production. The new suite parametrizes every behavioral case to run against a real AsyncSlackResponse as well, which is what makes the defect (and any future recurrence) visible.

The premise was verified against the runtime, not assumed:

• SlackResponse/AsyncSlackResponse MRO ends at object in both slack_sdk 3.40.1 and 3.43.0 (the version pinned in pyproject.toml); the return type is fixed in the SDK signatures (users_info(...) -> AsyncSlackResponse), and Bolt hands out that same AsyncWebClient. No bot setting, OAuth scope, or Slack-side API change can flip this.
• The symptom was captured on a live gateway (the request dump sent to the model contained the id-for-name prefix), and a direct users.info with the same bot token returned the correct display_name, ruling out scopes/transport.
• Pre-fix behavior was reproduced against a real AsyncSlackResponse: _resolve_user_name returned U_HUMAN instead of Nikita; after the fix it returns the name.

Related Issue

No open issue — the symptom ("Slack display names became user IDs") does not appear to be reported. The only PR with the same diagnosis, #72062, was closed by its own author without maintainer review; it also bundled two unrelated changes (_apply_yaml_config and success_reaction / other_agent_patterns). This PR is the isolated fix, extended to the remaining call sites of the same bug class and to a test that exercises the real SDK response shape.

Fixes the regression introduced by #51627 (3f08201ba).

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• plugins/platforms/slack/adapter.py: new module-level _slack_response_payload() normalizer; all Slack Web API call sites read through it — _resolve_user_name, _resolve_user_is_bot, _resolve_channel_name, _post_ephemeral_fallback, the thread seed-post ts read, _standalone_upload_file, and the standalone chat.postMessage path including the caption fallback. Every isinstance(resp, dict) gate on an SDK response is removed.
• tests/gateway/test_slack_sdk_response.py (new, 23 tests): the normalizer's contract (dict passthrough, .data, binary .data → {}, unknown shape → {}), the identity/channel resolution paths, and the send paths — each parametrized over a hand-rolled stand-in and a real AsyncSlackResponse (skipped automatically when the slack extra is not installed). Includes an explicit assertion of the bug's premise (the runtime object fails an isinstance dict gate) and keeps coverage for the intended degradation on a genuinely unreadable response.

How to Test

1. With a real Slack workspace, run the gateway in a shared channel on main and send a message: the text handed to the model is prefixed with [U… | Slack user <@U…>] — the display name has been replaced by the id.
2. Apply this PR, restart the gateway (the wrong name is cached per process) and start a new session: the prefix becomes [Alice | Slack user <@U…>].
3. Automated: scripts/run_tests.sh tests/gateway/test_slack_sdk_response.py tests/gateway/test_slack.py tests/gateway/test_slack_mention.py tests/tools/test_send_message_slack.py -q → 212 passed, 0 failed (branch cut from current main).

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(slack): …)
• I searched for existing PRs to make sure this isn't a duplicate — closest is the author-closed fix(slack): read real SDK responses and deliver YAML plugin settings #72062, see "Related Issue"
• My PR contains only changes related to this fix (no unrelated commits) — 1 commit, 2 files
• I've run the affected suites via scripts/run_tests.sh and all tests pass
• I've added tests for my changes
• I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — N/A (restores documented behavior; the new helper is documented in its docstring)
• I've updated cli-config.yaml.example if I added/changed config keys — N/A (no config keys)
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) — N/A (pure Python response handling)
• I've updated tool descriptions/schemas if I changed tool behavior — N/A (no tool schema changed)

Screenshots / Logs

Before (prefix the model receives, from a live gateway's request dump):

[U0BCE4NRVKN | Slack user <@U0BCE4NRVKN>] ...

After:

[Nikita | Slack user <@U0BCE4NRVKN>] ...

Behavioral side effect worth calling out for review: with the guard removed, _resolve_user_is_bot actually recognizes bots again, so the allow_bots policy starts enforcing as #51627 intended.

[teknium1]

Thanks for isolating this to the response-shape regression. The premise holds on current main: plugins/platforms/slack/adapter.py:3803-3807 rejects a non-dict users.info response and caches the raw user ID, while the pinned Slack SDK 3.43.0 defines AsyncSlackResponse as a standalone container with response data in .data.

Problems

• tests/gateway/test_slack_sdk_response.py:190-250 covers SDK-shaped identity, ephemeral, and upload responses, but does not cover the other changed response reads: create_handoff_thread (plugins/platforms/slack/adapter.py:2262) or standalone chat_postMessage and caption fallback (plugins/platforms/slack/adapter.py:8718-8751). Those paths can regress back to losing ts without this suite detecting it.

Suggested changes

• Add SDK-shaped-response tests for the handoff seed timestamp and standalone text/caption-fallback delivery paths.

Automated hermes-sweeper review.

[teknium1]

Please add SDK-shaped-response coverage for this standalone chat_postMessage branch and the caption fallback below. The new suite currently reaches _standalone_upload_file, but not either of these changed ts reads.

[chenwei791129]

Confirming this on a production deployment that upgraded v2026.7.20 → v2026.7.30. Of the nine gates, _resolve_user_name is the damaging one, because there the mis-read result gets cached permanently.

It is a regression inside that tag range

Gateway logs going back seven weeks show sender names resolving on every single day up to the upgrade, then breaking within the hour it was deployed (counting how many inbound Slack messages logged a human name vs. a bare user ID):

2026-07-08 .. 07-30   bare_id=0    human_name=6..59   (per day)
2026-07-31 03:43:29   user=Alice          <- last message before the upgrade
2026-07-31 10:23:39   user=U0123456789    <- first message after it

git blame on v2026.7.30 attributes the gates to three commits, all landed 2026-07-23, i.e. between the two tags:

line	function	commit	PR
3804	_resolve_user_name	3f08201bac	#69483
3974	_resolve_user_is_bot	3f08201bac	#69483
3857	_resolve_channel_name	8685fea0ce	#70196
1630, 1633	_post_ephemeral_fallback	700c8bfc5c	#69479

Worth flagging that #70196 is titled "resolve channel IDs to human-readable names" — the gate it introduced stops that feature from ever taking effect.

For contrast, v2026.7.20 had no gate in _resolve_user_name and worked correctly:

result = await client.users_info(user=user_id)
user = result.get("user", {})     # SlackResponse supports .get(), so this was fine

Why this one site is worse than the other eight

The negative caching itself predates the regression — v2026.7.20 already wrote the bare ID into _user_name_cache in its except branch. That was harmless because it only ran on genuine API failures. The isinstance gate turns that rare path into the only path: every successful call now writes the bare ID, and because the cache has no TTL and does not distinguish negative entries, the early return

cached_name = self._user_name_cache.get(cache_key)
if cached_name is not None:
    return cached_name

means users.info is never called again for that user for the lifetime of the process. One lookup poisons the entry permanently.

Measured on the affected deployment: users.info answered ok:true with real_name populated, while the adapter kept returning the bare ID — and across a 17-minute window the egress proxy in front of the gateway logged zero users.info requests, because every entry was already cached. Restarting the gateway is not a workaround either: it empties the cache, and the very next lookup is mis-read again and re-poisons it.

Observable impact

With require_mention in a shared channel, gateway/run.py builds the sender prefix from source.user_name, so the model receives

[U0123456789 | Slack user <@U0123456789>] @U0987654321 <message text>

instead of

[Alice | Slack user <@U0123456789>] @BotName <message text>

In a session with no history there is nothing left to tell the model that the mention refers to itself, so it reads the message as an unrelated channel broadcast and emits NO_REPLY with tool_turns=0. Every fresh @mention goes unanswered, while threads that already have history often survive on conversational context alone — which makes it look intermittent.

Nothing surfaces in the logs: the only record is logger.debug("[Slack] users.info failed for %s: %s", ...), and no exception is raised, because the API call genuinely succeeded.

Minimal reproduction

from slack_sdk.web.async_slack_response import AsyncSlackResponse
issubclass(AsyncSlackResponse, dict)   # False
AsyncSlackResponse.__mro__             # (AsyncSlackResponse, object)

payload = {"ok": True, "user": {"real_name": "Alice", "profile": {"real_name": "Alice"}}}
r = AsyncSlackResponse(client=None, http_verb="GET", api_url="x", req_args={},
                       data=payload, headers={}, status_code=200)

isinstance(r, dict)                      # False -> failure branch, caches the bare ID
isinstance(getattr(r, "data", r), dict)  # True  -> resolves "Alice"

Still reproducible on main, with all nine gates present.

On the approach

_slack_response_payload() looks like the right shape to me — one helper beats normalizing at each gate, which is what we did locally as a stopgap and it already means two places to keep in sync. One argument for keeping an isinstance check inside the helper: .data is a str when the body is not JSON, which is a real failure that should still land in the error branch rather than becoming an empty dict that reads as ok: false for the wrong reason.

Happy to test a build of this branch against the workload that surfaced it if that would help move the review along.

[ksylvan]

Ran into this myself and my Hermes agent diagnosed this same fix. Thanks for this @nikitaBarkov

[nikitaBarkov]

Pushed 8c1d073 — review feedback addressed, branch rebased onto current main.

@teknium1 (sweeper) — missing coverage on the other changed read paths. Correct, the suite stopped at identity/ephemeral/upload. Added:

• TestHandoffThread — create_handoff_thread (adapter.py:2262) must return the seed message's ts; without it every handoff send lands in the channel instead of the thread. Plus the opaque-response case, which must still degrade to None.
• TestStandaloneSendMediaPath — the two chat_postMessage reads in the standalone media branch: the text post's ts (:8721) becoming message_id, its error being surfaced instead of silently proceeding to upload, and the caption fallback for a missing media file (:8749).

Both new classes run against the hand-rolled stand-in and the real AsyncSlackResponse. I verified they are genuine regression tests: reverting adapter.py to main (keeping only the helper so the module imports) makes all six TestStandaloneSendMediaPath cases fail — the caption-only delivery comes back as "No deliverable text or media remained after processing", and the text post's ts is lost. TestHandoffThread passes pre-fix too (that site had a getattr(result, "get", ...) fallback), so it is coverage for the refactor rather than a reproduction — keeping it so the path can't silently regress again.

@chenwei791129 — thanks for the independent production confirmation, that's the most useful thing in this thread. On your suggestion to keep the isinstance check inside the helper: agreed, and that's already how it's written —

data = getattr(response, "data", None)
return data if isinstance(data, dict) else {}

so a non-JSON body (.data as str/bytes) yields {} and the caller keeps its existing fallback rather than blowing up on .get. TestSlackResponsePayload::test_binary_response_is_not_mistaken_for_data pins exactly that. If you meant something beyond the str/bytes case — e.g. a SlackResponse whose .data is a JSON list — say the word and I'll extend the normalizer.

Please do run the branch against your load; a second real-workspace confirmation would be worth more than anything else I can add here.

@ksylvan — thanks for the confirmation.

Suite: scripts/run_tests.sh tests/gateway/test_slack_sdk_response.py tests/gateway/test_slack.py tests/tools/test_slack_send_message_media.py tests/tools/test_send_message_slack.py — green (32 tests in the new file).

[nikitaBarkov]

Re-review requested — corrected head 8c1d073

I don't have push access here, so I can't set the reviewer field; requesting re-review by comment instead. @teknium1 — the review from 2026-07-30 is COMMENTED and still the latest, which keeps mergeStateStatus: BLOCKED even though the PR is MERGEABLE with no conflicts against current main.

What changed since that review (head 8c1d073):

• Coverage gap closed for _standalone_send. New tests in tests/gateway/test_slack_sdk_response.py cover the standalone chat_postMessage text path and the caption fallback. Verified in both directions: with adapter.py reverted to main (keeping only the helper so the module imports) these fail — caption-only delivery reported "nothing to send" and the message ts was lost. Real regressions, not shape assertions.
• create_handoff_thread: the premise did not hold, and I said so instead of dressing it up. Pre-fix that path was not a bare isinstance gate — it went through getattr(result, "get", lambda ...), which AsyncSlackResponse satisfies. The tests I added for it are green on pre-fix code too, so they are refactor coverage, not a reproduction. Flagging it explicitly so the "missing coverage" note isn't read as an unfixed bug.
• @chenwei791129's point about keeping the isinstance check inside the helper was already implemented (data if isinstance(data, dict) else {}) — answered with the code rather than agreement.

Nothing new has landed on main in this area (no _slack_response_payload upstream), so the fix is still needed as-is. Happy to add anything else you want covered — just naming the exact head so the re-review lands on the right commit.

[calebhicks]

Confirming this is still live in v0.20.0 (build 2026.8.3): _resolve_user_name / _resolve_user_is_bot in plugins/platforms/slack/adapter.py still gate on isinstance(result, dict), and slack_sdk's users_info returns a SlackResponse, so every sender resolves to the raw member ID and gets cached that way for the gateway process lifetime. Downstream effect: multi-user Slack deployments see [U…| Slack user <@U…>] prefixes and single-user sessions get a raw ID in the session-context User: line, which pushes speaker identification onto prompt-side directories. Would love to see this land — the .data/duck-typing approach here looks right.

[teknium1]

Merged via PR #85463 with your commits cherry-picked onto current main — authorship preserved in git log. Both commits landed, including the follow-up test coverage for the handoff-thread ts and standalone media send paths. Thanks for the whole-class fix and the production confirmation trail.