fix(slack): read real SDK responses instead of gating on isinstance dict (salvage #74658) - #85463
Merged
Conversation
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.
…se reads Review on #74658 flagged that the response-shape suite exercised identity, ephemeral and upload paths but left two changed call sites untested: - create_handoff_thread's seed-message ts (adapter.py:2262), which anchors every subsequent handoff send onto the thread; - the standalone media branch's chat_postMessage reads (adapter.py:8721 text post, :8749 caption fallback), where an SDK-shaped reply used to drop the ts and report a caption-only delivery as 'nothing deliverable'. Both new cases run against the hand-rolled stand-in and the real AsyncSlackResponse. Verified they fail against the pre-fix adapter. Co-authored-by: Junie <junie@jetbrains.com>
Contributor
૮ >ﻌ< ა ci reviewran on 8c6a0fd — test(slack): cover handoff-thread ts and standalone media se
|
alt-glitch
added
type/bug
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Slack adapter now reads real slack_sdk response objects — user name resolution, bot detection, ephemeral send confirmation, and thread
tsreads all work again. Salvage of #74658 by @nikitaBarkov onto current main, authorship preserved.Root cause: commits from #69483/#70196/#69479 added
isinstance(resp, dict)gates on Web API responses, butSlackResponse/AsyncSlackResponseare not dict subclasses — every gate was always False at runtime while dict-injecting tests stayed green.Changes
plugins/platforms/slack/adapter.py: new_slack_response_payload()normalizer (dict passthrough →.dataif dict →{}), routed through all 8 gated call sites — whole-class fixtests/gateway/test_slack_sdk_response.py: 18 tests parametrized over a stand-in AND a realAsyncSlackResponse, including the handoff-thread seed-ts and standalone media-send paths flagged in reviewValidation
Infographic