Skip to content

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

Merged
nikitaBarkov merged 1 commit into
sync/upstream-2026-07-27from
nikita.barkov/slack-sdk-response-not-dict
Jul 30, 2026
Merged

fix(slack): read real SDK responses instead of gating on isinstance dict#18
nikitaBarkov merged 1 commit into
sync/upstream-2026-07-27from
nikita.barkov/slack-sdk-response-not-dict

Conversation

@nikitaBarkov

@nikitaBarkov nikitaBarkov commented Jul 30, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes a regression that arrived with the upstream sync: Slack user and channel names collapse to raw IDs, so the agent sees [U0BCE4NRVKN | Slack user <@U0BCE4NRVKN>] instead of [Nikita | Slack user <@U0BCE4NRVKN>].

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. Upstream commit 3f08201ba ("Fix Slack peer bot status routing loops", PR NousResearch#51627) added isinstance(result, dict) guards around those responses, so at runtime the guard is always False and every call site takes its "unexpected shape" degradation branch:

  • _resolve_user_name → name becomes the user id, and the wrong value is cached for the lifetime of the gateway process
  • _resolve_user_is_bot → every user resolves as a non-bot, which defeats the allow_bots loop guard the same commit was meant to add
  • _resolve_channel_name → channel name degrades to C0…
  • _post_ephemeral_fallback → successful slash-command replies are reported as unexpected_response failures
  • _standalone_upload_file / standalone chat.postMessagemessage_id is lost, caption fallback is never marked delivered

The fix normalizes every Slack response through one helper — _slack_response_payload(): a plain dict passes through, an SDK response yields .data, anything else (including a binary .data) yields {} so callers keep their existing fallbacks. No behavior other than the degradation is changed.

The premise was verified before writing the fix, not assumed:

  • In both the installed slack_sdk (3.40.1) and the version pinned in pyproject.toml (3.43.0), SlackResponse/AsyncSlackResponse MRO ends at object — they are not dict subclasses. The return type is fixed in the SDK signatures (users_info(...) -> AsyncSlackResponse), and Bolt hands out the same AsyncWebClient. Nothing about bot settings, scopes, the relay, or a Slack-side API change can flip this.
  • The symptom was captured on the live agent: the request dump sent to the model and the stored session history both contain [U0BCE4NRVKN | Slack user <@U0BCE4NRVKN>].
  • The relay and scopes were ruled out: a direct users.info with the bot token through our proxy returns display_name: "Nikita".
  • 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.

Note this branch targets sync/upstream-2026-07-27, not main: commit 3f08201ba is not contained in origin/main, so the regression only exists on the sync branch (verified with git branch -r --contains 3f08201ba).

Related Issue

No fork issue. Upstream has no open issue for this; the only PR with the same diagnosis (NousResearch#72062) was closed by its own author without maintainer review, so the defect is still present in upstream/main. Submitted upstream separately as a standalone fix.

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 _slack_response_payload() normalizer, and every Slack Web API call site routed 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 standalone chat.postMessage including the caption fallback. All isinstance(resp, dict) gates on SDK responses are gone.
  • tests/gateway/test_slack_sdk_response.py (new): every behavioral case is parametrized and runs both against a hand-rolled stand-in and against a real AsyncSlackResponse. Injecting plain dicts is exactly what hid this bug from the existing Slack suite for a month, so the real SDK shape is now part of the contract. Includes the normalizer's contract (dict passthrough, .data, binary .data{}, unknown shape → {}) and preserves the "genuinely unreadable response still falls back to the id" degradation.

How to Test

  1. Reproduce on the branch point: with a real Slack workspace on sync/upstream-2026-07-27, send a message in a shared channel — the prefix the model receives is [U… | Slack user <@U…>] (name replaced by the id).
  2. Apply this branch, restart the gateway (the wrong name is cached per process) and start a new session — the prefix becomes [Nikita | 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 -q528 passed, 0 failed (on this branch's base). The full Slack surface (29 suites + tests/tools/test_send_message_slack.py) was also run green: 876 passed. tests/gateway/ as a whole has 8 pre-existing failures unrelated to this change (wecom_callback ×3, background_command, readiness, shutdown_forensics, api_server, systemd_notify) — the same 8 fail with this change stashed.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits — the PR title (fix(slack): …) is intended as the squash-merge message
  • I searched for existing PRs to make sure this isn't a duplicate
  • 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 — N/A (restores documented behavior, no user-facing surface changed)
  • 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 seen by the model, taken from the live agent's request dump):

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

After:

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

Side effect worth knowing: _resolve_user_is_bot now actually recognizes bots, so the allow_bots policy starts working the way commit 3f08201ba intended.

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.
@nikitaBarkov
nikitaBarkov force-pushed the nikita.barkov/slack-sdk-response-not-dict branch from ca0b0f4 to 4289f26 Compare July 30, 2026 06:57
@nikitaBarkov

Copy link
Copy Markdown
Author

Hermes PR: NousResearch#74658

@nikitaBarkov
nikitaBarkov merged commit c334fd8 into sync/upstream-2026-07-27 Jul 30, 2026
36 of 41 checks passed
@AlexanderPrendota
Alexander Prendota (AlexanderPrendota) deleted the nikita.barkov/slack-sdk-response-not-dict branch July 30, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant