fix(slack): route empty-DM thread_ts fetch to conversations.history (#138)#175
Conversation
…138) 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).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe Slack adapter's ChangesSlack DM root fetch routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where fetching messages for top-level Slack DMs (which have an empty thread_ts) fails because they are routed to conversations.replies instead of conversations.history. The changes route empty thread_ts fetches to conversations.history in both fetch_messages and fetch_message, accompanied by new unit tests and documentation updates. The reviewer identified a bug where helper methods for channel history fallback to using the message's own ts for the thread_id, causing a discrepancy (e.g., slack:Dxxx:ts instead of slack:Dxxx:). A code suggestion was provided to post-process the fetched messages and ensure they are assigned the correct thread_id.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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) |
There was a problem hiding this comment.
When thread_ts is empty (such as for top-level DM roots), fetch_messages routes the request to _fetch_channel_messages_forward or _fetch_channel_messages_backward. However, these helper methods dynamically construct the message thread_id using msg.get('thread_ts') or msg.get('ts', ''). For top-level DM messages, this falls back to the message's own ts, resulting in a thread ID like slack:Dxxx:ts instead of the expected slack:Dxxx: (with an empty thread_ts). This creates a discrepancy between real-time messages (which correctly use slack:Dxxx:) and fetched history messages.
We can resolve this by post-processing the fetched messages to ensure that any top-level DM messages (those without an explicit thread_ts in their raw payload) are assigned the correct thread_id.
| 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 not thread_ts: | |
| if direction == "forward": | |
| result = await self._fetch_channel_messages_forward(channel, limit, cursor) | |
| else: | |
| result = await self._fetch_channel_messages_backward(channel, limit, cursor) | |
| for msg in result.messages: | |
| if msg.raw and not msg.raw.get("thread_ts"): | |
| msg.thread_id = thread_id | |
| return result |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d8791d7f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await self._fetch_channel_messages_forward(channel, limit, cursor) | ||
| return await self._fetch_channel_messages_backward(channel, limit, cursor) |
There was a problem hiding this comment.
For an empty-DM thread like slack:D999:, routing through the channel-history helpers returns messages parsed with f"slack:{channel}:{msg.get('thread_ts') or msg.get('ts', '')}" (see the helper below), so every top-level DM message comes back with thread_id like slack:D999:<message-ts> instead of the requested slack:D999:. That is inconsistent with _handle_message_event, open_dm, and the new fetch_message branch, and any consumer that reuses a fetched message's threadId to post/stream will start addressing a Slack DM thread rather than the unthreaded DM conversation this change is trying to preserve.
Useful? React with 👍 / 👎.
Python-only patch on 4.31.0 (UPSTREAM_PARITY unchanged). - Slack #138: empty-DM thread_ts fetch routes to conversations.history (PR #175) - Slack #95: thread team_id to chat.startStream for Enterprise Grid (PR #176) - docs: mark Linear agent-sessions mode experimental (unverified vs live tenant, #151) - version 0.4.31 -> 0.4.31.1; CHANGELOG; CLAUDE.md mapping
1 participant
Summary
Fixes Slack issue #138 — a top-level Slack DM root legitimately encodes an empty
thread_ts(slack:Dxxx:, faithful to_handle_message_event/ upstreamhandleMessageEvent). The fetch paths previously calledconversations.replies(ts="")unconditionally, which Slack answers with no replies — silently losing the DM root context for every DM history/single-message fetch (and the #137 DM block-action consumer).Change
src/chat_sdk/adapters/slack/adapter.py:fetch_messages— whenthread_tsis empty/falsy (DM root), route to the existing channel-history helpers (_fetch_channel_messages_forward/_fetch_channel_messages_backward, bothconversations.history) instead of the thread-reply helpers. Direction, limit, and cursor semantics are preserved. For a DM the channel is the conversation, so the channel-history mapping is correct. Noconversations.historylogic is duplicated.fetch_message— whenthread_tsis empty, fetch the single message viaconversations.history(channel, latest=message_id, inclusive=True, limit=1), mirroring the inner link-previewfetch_message. Non-empty path is byte-identical.thread_tsstays byte-identical — stillconversations.replies. Only the empty-thread_tsbranch changes.Divergence
This is a divergence ahead of upstream: upstream's
fetchMessages(adapter-slack/src/index.ts:4135→fetchMessagesForward/Backwardat:4187/:4250) andfetchMessage(:4350) callconversations.replies({ ts: threadTs })with no empty-thread_tsguard either. A new Known Non-Parity row indocs/UPSTREAM_SYNC.mddocuments this, notes it covers DM messages and the #137 DM block-action path uniformly (superseding the "separate follow-up" noted in the #133/#137 row), and flags it as a candidate to file upstream so it can be removed on a future sync.Tests (
tests/test_slack_api.py)fetch_messages(forward + backward): assertsconversations.historyis called for the channel andconversations.repliesis never called (esp. not withts=""); returns the DM messages.thread_tsfetch_message: assertsconversations.history(latest=message_id,inclusive,limit=1) is used, notconversations.replies.thread_tsfetch_messages(forward + backward) andfetch_message: regression guards assertingconversations.repliesis still used andconversations.historyis not (fails if the routing over-triggers).Mutation proofs: removing the empty-guard routing → the empty-DM
fetch_messages/fetch_messagetests fail (conversations.replies(ts="")yields nothing). Over-triggering the guard to fire for allthread_ts→ all three non-empty regression guards fail.Gauntlet (all green)
ruff check/ruff format --check— passaudit_test_quality.py— 0 hard failurespyrefly check src/— 0 errorsverify_test_fidelity.py --strict(TS_ROOT=chat@4.31.0) — 732/732 matched (100%)pytest tests/— 5252 passed, 4 skipped (pre-existing);pytest tests/test_slack*.py— 669 passedSummary by CodeRabbit
Bug Fixes
Tests