From 9d8791d7f76856f8455ae48967d7d30432a9eb4e Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Sun, 21 Jun 2026 12:20:40 -0700 Subject: [PATCH] fix(slack): route empty-DM thread_ts fetch to conversations.history (#138) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A top-level Slack DM root encodes an empty thread_ts (slack:Dxxx:). The fetch paths previously called conversations.replies(ts="") unconditionally, which returns no replies and loses the DM root context for every DM. - fetch_messages: when thread_ts is empty (falsy), route to the existing channel-history helpers (_fetch_channel_messages_forward / _fetch_channel_messages_backward, both conversations.history) instead of the thread-reply helpers. Direction, limit, and cursor semantics are preserved; for a DM the channel IS the conversation, so this is correct. - fetch_message: when thread_ts is empty, fetch the single message via conversations.history(latest=message_id, inclusive=True, limit=1), mirroring the inner link-preview fetch_message. - Non-empty thread_ts stays byte-identical (still conversations.replies). This is a divergence ahead of upstream — upstream's fetchMessages / fetchMessage call conversations.replies(ts: threadTs) with no empty-thread_ts guard either. Documented in docs/UPSTREAM_SYNC.md as a candidate to file upstream; it also covers the #137 DM block-action consumer uniformly. Tests: empty-DM fetch_messages (forward + backward) and fetch_message assert conversations.history is used and conversations.replies(ts="") is never called; non-empty thread_ts regression guards assert conversations.replies is still used (so the routing cannot over-trigger). --- docs/UPSTREAM_SYNC.md | 3 +- src/chat_sdk/adapters/slack/adapter.py | 31 +++-- tests/test_slack_api.py | 174 +++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 10 deletions(-) diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index d8f499e..78b7ea5 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -667,7 +667,8 @@ stay explicit instead of being rediscovered in code review. | `_rehydrate_message` with `Message` input | Falls through to the `rehydrate_attachment` pass even when the dequeued entry is already a `Message` instance | Early-returns on `raw instanceof Message` before rehydration | The Python port's Redis + Postgres `dequeue()` upgrade raw JSON to `Message.from_json(...)` before returning (upstream's dequeue returns the raw JSON.parse'd dict). Upstream's `instanceof Message` shortcut therefore only fires for in-memory state, but ours would fire for persistent backends too, leaving `fetch_data` stripped forever. The rehydrate pass still skips any attachment that already has `fetch_data`, so in-memory callers pay no cost. | | Slack Socket Mode reconnect loop | Outer reconnect loop on top of `slack_sdk.socket_mode.aiohttp.SocketModeClient` (which itself has `auto_reconnect_enabled=True`). Exponential backoff (1s → 30s) with explicit shutdown signaling and a tracked `asyncio.Task` so `disconnect()` can cancel cleanly | Single `SocketModeClient` instance from `@slack/socket-mode`; relies entirely on the package's internal reconnect | Hazard #5 (async task lifecycle): a long-lived WebSocket needs an explicit shutdown path so `disconnect()` doesn't leak the loop, and a guarded outer reconnect path so the adapter survives `connect()` itself raising (which the inner client doesn't retry). Inner auto-reconnect still runs; the outer loop is belt-and-suspenders, not a divergence in observable behavior. | | Slack Socket Mode listener serverless variant | Not ported | `startSocketModeListener()` / `runSocketModeListener()` open a transient socket for `durationMs` and forward events via HTTP POST | Vercel-specific pattern (cron-triggered ephemeral listener with `waitUntil`). The forwarded-event receiver (`x-slack-socket-token` handling in `handle_webhook`) is ported so a separate Python process can run the long-lived listener; the deployment glue itself isn't part of the SDK. | -| Slack DM block-action threading (#133/#137) | `_handle_block_actions` sets `thread_ts=""` for a top-level DM button click (never falls back to the clicked message's own `ts`), so a handler's `event.thread.post(...)` does not spawn a phantom "1 reply" thread in the DM. Mirrors `_handle_message_event`'s DM handling (`thread_ts=""` for top-level DMs). | `handleBlockActions` (`adapter-slack/src/index.ts:1455-1456,1470`) computes `thread_ts \|\| container.thread_ts \|\| messageTs` and encodes `threadTs \|\| messageTs \|\| ""` — it falls back to the clicked message's `ts` even for DMs, so a DM button click spawns a phantom reply thread. Upstream's `handleMessageEvent` *does* empty-case DMs (`:2158`), but `handleBlockActions` does **not** — an upstream internal inconsistency. | Hard UX failure with no workaround (phantom "1 reply" threads on DM button clicks). We extend upstream's own DM-message convention to the block-action path. The resulting empty DM `thread_ts` is consumed unguarded by `fetch_messages` → `conversations.replies(ts="")` — identical to upstream (`fetchMessages` `:4178` has no empty-`thread_ts` guard) and to the faithful DM-message path; a `conversations.history` fallback for empty DM `thread_ts` is a separate, codebase-wide follow-up. The block-action fix should be contributed upstream (cf. PR #107's stream() divergence) to restore parity. | +| Slack DM block-action threading (#133/#137) | `_handle_block_actions` sets `thread_ts=""` for a top-level DM button click (never falls back to the clicked message's own `ts`), so a handler's `event.thread.post(...)` does not spawn a phantom "1 reply" thread in the DM. Mirrors `_handle_message_event`'s DM handling (`thread_ts=""` for top-level DMs). | `handleBlockActions` (`adapter-slack/src/index.ts:1455-1456,1470`) computes `thread_ts \|\| container.thread_ts \|\| messageTs` and encodes `threadTs \|\| messageTs \|\| ""` — it falls back to the clicked message's `ts` even for DMs, so a DM button click spawns a phantom reply thread. Upstream's `handleMessageEvent` *does* empty-case DMs (`:2158`), but `handleBlockActions` does **not** — an upstream internal inconsistency. | Hard UX failure with no workaround (phantom "1 reply" threads on DM button clicks). We extend upstream's own DM-message convention to the block-action path. The resulting empty DM `thread_ts` is consumed by `fetch_messages` → now routed to `conversations.history` for empty `thread_ts` (see the #138 row below); the block-action fix should be contributed upstream (cf. PR #107's stream() divergence) to restore parity. | +| Slack empty-DM `thread_ts` fetch routing (#138) | `fetch_messages` routes an empty (falsy) `thread_ts` — every top-level DM root, encoded `slack:Dxxx:` — to the channel-history path (`_fetch_channel_messages_forward` / `_fetch_channel_messages_backward`, both `conversations.history`) instead of `conversations.replies(ts="")`, preserving direction/limit/cursor. `fetch_message` likewise reads a single empty-`thread_ts` message via `conversations.history(channel, latest=message_id, inclusive=True, limit=1)` (mirroring the inner link-preview `fetch_message` at `slack/adapter.py:3293`). Non-empty `thread_ts` stays byte-identical on `conversations.replies`. | `fetchMessages` (`adapter-slack/src/index.ts:4135` → `fetchMessagesForward`/`Backward` `:4187`/`:4250`) and `fetchMessage` (`:4350`) call `conversations.replies({ ts: threadTs })` with **no** empty-`thread_ts` guard. With `ts=""` Slack returns no replies and the DM root context is lost. | Hard UX failure on **every** DM root: history/single-message fetches over a DM silently return nothing (or lose the root) because the DM root legitimately encodes `threadTs=""` (faithful to `_handle_message_event` / `handleMessageEvent`, "matches openDM subscriptions"). For a DM the channel **is** the conversation, so `conversations.history` is the correct source. This supersedes the "separate follow-up" noted in the #133/#137 row and covers DM message fetches **and** the #137 DM block-action consumer uniformly. Candidate to file upstream against vercel/chat (an empty-`thread_ts` guard in `fetchMessages`/`fetchMessage`); remove this divergence once upstream adds it. | | `GitHubAdapter.octokit` native client getter (vercel/chat#459, #478) | Not exposed | `get octokit(): Octokit` (plus deprecated `client` alias) returns the underlying Octokit — fixed instance in PAT/single-tenant App mode, per-installation client resolved from `AsyncLocalStorage` inside a webhook handler in multi-tenant mode | The Python adapter is hand-rolled over raw `aiohttp` (`_github_api_request`) with PyJWT for App JWTs and an installation-token cache; the `github` extra is `pyjwt[crypto]` only — there is no Octokit-equivalent object to return, and exposing the raw session or an invented facade under the name `octokit` would misrepresent the surface. Revisit if the adapter adopts an octokit-style SDK (e.g. `githubkit`) as an optional dependency per hazard #10's "prefer official SDKs" sub-rule; the getter (and the GitHub `fetch_subject` half of #459) ports cleanly then. | | `LinearAdapter.linear_client` native client getter (vercel/chat#459, #478) | Not exposed | `get linearClient(): LinearClient` (plus deprecated `client` alias) returns the `@linear/sdk` `LinearClient`, per-org from `AsyncLocalStorage` in multi-tenant OAuth mode | `@linear/sdk` is TypeScript-only and no official Linear Python SDK exists; the adapter issues GraphQL directly over `aiohttp` (`_graphql_query`) and already documents that stance. Nothing honest to put behind the name. Revisit only if Linear ships an official Python SDK (the Linear `fetch_subject` half of #459 is blocked on the same). | | `@chat-adapter/tests` adapter test kit (vercel/chat#470) | Not ported | New TS package with test utilities for adapter authors | Python already ships `chat_sdk.testing` (`MockAdapter`, `MockStateAdapter`, `create_test_message()`) covering the same surface for this repo's adapter tests; mirroring the TS kit verbatim would duplicate it. Revisit if upstream's kit grows capabilities ours lacks (e.g. recorded replay fixtures for third-party adapter authors). | diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index b9b6574..cffd0e4 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -4317,15 +4317,21 @@ async def fetch_messages(self, thread_id: str, options: FetchOptions | None = No thread_ts = decoded.thread_ts direction = getattr(opts, "direction", "backward") or "backward" limit = getattr(opts, "limit", 100) if getattr(opts, "limit", 100) is not None else 100 + cursor = getattr(opts, "cursor", None) + # Divergence (chat-sdk-python#138): a top-level DM root encodes an empty + # thread_ts (slack:Dxxx:). conversations.replies(ts="") returns no replies + # and loses the DM root context, so route empty thread_ts to the channel + # history path (conversations.history), where the channel *is* the + # conversation. Upstream's fetchMessages has no empty-thread_ts guard. try: + if not thread_ts: + if direction == "forward": + return await self._fetch_channel_messages_forward(channel, limit, cursor) + return await self._fetch_channel_messages_backward(channel, limit, cursor) if direction == "forward": - return await self._fetch_messages_forward( - channel, thread_ts, thread_id, limit, getattr(opts, "cursor", None) - ) - return await self._fetch_messages_backward( - channel, thread_ts, thread_id, limit, getattr(opts, "cursor", None) - ) + return await self._fetch_messages_forward(channel, thread_ts, thread_id, limit, cursor) + return await self._fetch_messages_backward(channel, thread_ts, thread_id, limit, cursor) except Exception as error: self._handle_slack_error(error) @@ -4383,9 +4389,16 @@ async def fetch_message(self, thread_id: str, message_id: str) -> Message | None try: client = self._get_client() - result = await client.conversations_replies( - channel=channel, ts=thread_ts, oldest=message_id, inclusive=True, limit=1 - ) + # Divergence (chat-sdk-python#138): a DM root encodes an empty + # thread_ts, so conversations.replies(ts="") cannot locate the + # message. Fetch the single message from conversations.history + # instead (mirrors the link-preview fetch_message at ~3293). + if not thread_ts: + result = await client.conversations_history(channel=channel, latest=message_id, inclusive=True, limit=1) + else: + result = await client.conversations_replies( + channel=channel, ts=thread_ts, oldest=message_id, inclusive=True, limit=1 + ) messages = result.get("messages", []) target = next((m for m in messages if m.get("ts") == message_id), None) if not target: diff --git a/tests/test_slack_api.py b/tests/test_slack_api.py index 0d5c3f6..7d946f2 100644 --- a/tests/test_slack_api.py +++ b/tests/test_slack_api.py @@ -612,6 +612,180 @@ async def test_fetch_with_limit(self): # Should return at most limit messages assert len(result.messages) <= 10 + @pytest.mark.asyncio + async def test_empty_dm_thread_ts_backward_uses_history_not_replies(self): + """A DM root (slack:Dxxx:) encodes thread_ts="" — backward fetch must + route to conversations.history (the channel IS the conversation), not + conversations.replies(ts="") which returns nothing for a DM (#138).""" + adapter, client, _ = await _init_adapter() + client.set_response( + "conversations_history", + { + "ok": True, + "messages": [ + {"ts": "1234567890.000002", "text": "DM 2", "user": "U2"}, + {"ts": "1234567890.000001", "text": "DM 1", "user": "U1"}, + ], + "has_more": False, + }, + ) + + result = await adapter.fetch_messages("slack:D999:") + + # The DM root messages come back via conversations.history. + assert len(result.messages) == 2 + history_calls = client.get_calls("conversations_history") + assert len(history_calls) == 1 + assert history_calls[0]["kwargs"]["channel"] == "D999" + # conversations.replies must NOT be hit at all (esp. not with ts=""). + replies_calls = client.get_calls("conversations_replies") + assert replies_calls == [] + + @pytest.mark.asyncio + async def test_empty_dm_thread_ts_forward_uses_history_not_replies(self): + """Forward direction over a DM root also routes to conversations.history, + preserving cursor/limit semantics, never conversations.replies(ts="").""" + adapter, client, _ = await _init_adapter() + client.set_response( + "conversations_history", + { + "ok": True, + "messages": [ + {"ts": "1234567890.000002", "text": "Newer", "user": "U2"}, + {"ts": "1234567890.000001", "text": "Older", "user": "U1"}, + ], + "has_more": True, + }, + ) + + result = await adapter.fetch_messages( + "slack:D999:", + FetchOptions(direction="forward", cursor="1234567890.000000", limit=50), + ) + + assert len(result.messages) == 2 + history_calls = client.get_calls("conversations_history") + assert len(history_calls) == 1 + assert history_calls[0]["kwargs"]["channel"] == "D999" + # Forward cursor maps to oldest= on conversations.history (channel-history path). + assert history_calls[0]["kwargs"]["oldest"] == "1234567890.000000" + assert history_calls[0]["kwargs"]["limit"] == 50 + # has_more + slack messages → next_cursor from the newest ts. + assert result.next_cursor == "1234567890.000002" + assert client.get_calls("conversations_replies") == [] + + @pytest.mark.asyncio + async def test_non_empty_thread_ts_still_uses_replies_backward(self): + """Regression guard: a real thread root (non-empty thread_ts) MUST keep + using conversations.replies — the empty-DM routing must not over-trigger.""" + adapter, client, _ = await _init_adapter() + client.set_response( + "conversations_replies", + { + "ok": True, + "messages": [ + {"ts": "1234567890.000001", "text": "Reply 1", "user": "U1"}, + ], + "has_more": False, + }, + ) + + result = await adapter.fetch_messages("slack:C123:1234567890.000000") + + assert len(result.messages) == 1 + replies_calls = client.get_calls("conversations_replies") + assert len(replies_calls) == 1 + assert replies_calls[0]["kwargs"]["ts"] == "1234567890.000000" + # Channel-history path must NOT be used for a real thread. + assert client.get_calls("conversations_history") == [] + + @pytest.mark.asyncio + async def test_non_empty_thread_ts_still_uses_replies_forward(self): + """Regression guard (forward): non-empty thread_ts keeps conversations.replies.""" + adapter, client, _ = await _init_adapter() + client.set_response( + "conversations_replies", + { + "ok": True, + "messages": [ + {"ts": "1234567890.000001", "text": "First", "user": "U1"}, + ], + "response_metadata": {"next_cursor": "cur"}, + }, + ) + + result = await adapter.fetch_messages( + "slack:C123:1234567890.000000", + FetchOptions(direction="forward"), + ) + + assert len(result.messages) == 1 + replies_calls = client.get_calls("conversations_replies") + assert len(replies_calls) == 1 + assert replies_calls[0]["kwargs"]["ts"] == "1234567890.000000" + assert client.get_calls("conversations_history") == [] + + +# ============================================================================= +# fetchMessage (single) Tests +# ============================================================================= + + +class TestFetchSingleMessage: + @pytest.mark.asyncio + async def test_non_empty_thread_ts_uses_replies(self): + """A single-message fetch on a real thread uses conversations.replies + (oldest=message_id) — byte-identical to the pre-#138 path.""" + adapter, client, _ = await _init_adapter() + client.set_response( + "conversations_replies", + { + "ok": True, + "messages": [ + {"ts": "1234567890.000050", "text": "Target", "user": "U1"}, + ], + }, + ) + + msg = await adapter.fetch_message("slack:C123:1234567890.000000", "1234567890.000050") + + assert msg is not None + assert msg.id == "1234567890.000050" + replies_calls = client.get_calls("conversations_replies") + assert len(replies_calls) == 1 + assert replies_calls[0]["kwargs"]["ts"] == "1234567890.000000" + assert replies_calls[0]["kwargs"]["oldest"] == "1234567890.000050" + assert client.get_calls("conversations_history") == [] + + @pytest.mark.asyncio + async def test_empty_dm_thread_ts_uses_history(self): + """A single-message fetch on a DM root (empty thread_ts) reads from + conversations.history (latest=message_id), NOT conversations.replies(ts="") + which cannot locate the message (#138).""" + adapter, client, _ = await _init_adapter() + client.set_response( + "conversations_history", + { + "ok": True, + "messages": [ + {"ts": "1234567890.000050", "text": "DM message", "user": "U1"}, + ], + }, + ) + + msg = await adapter.fetch_message("slack:D999:", "1234567890.000050") + + assert msg is not None + assert msg.id == "1234567890.000050" + history_calls = client.get_calls("conversations_history") + assert len(history_calls) == 1 + assert history_calls[0]["kwargs"]["channel"] == "D999" + assert history_calls[0]["kwargs"]["latest"] == "1234567890.000050" + assert history_calls[0]["kwargs"]["inclusive"] is True + assert history_calls[0]["kwargs"]["limit"] == 1 + # conversations.replies must NOT be called with ts="". + assert client.get_calls("conversations_replies") == [] + # ============================================================================= # fetchThread Tests