fix(slack): don't thread block-action responses in DMs (supersedes #133)#137
Merged
Conversation
_handle_block_actions fell back to the clicked message's ts for thread_ts in DMs, so HITL approval result cards posted via event.thread.post became phantom "1 reply" threads in the DM. Mirror _handle_message_event's DM handling: keep a real in-DM thread_ts but never fall back to message_ts for a top-level DM click. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address two defects from local review of the DM block-action threading fix: 1. pyrefly bad-argument-type regression: the block-action handler replaced the established `thread_ts or message_ts or ""` coalesce with bare `thread_ts`, letting `str | None` flow into SlackThreadId(thread_ts: str). Restore the coalesce as `thread_ts or ""`, which both preserves the DM empty-thread_ts intent and guarantees `str` (thread_ts already folds in the message_ts fallback for channels). pyrefly src/ back to 0 errors. 2. Remove three committed .DS_Store binaries (./, src/, src/chat_sdk/) and add .DS_Store to .gitignore so they cannot return. Also add a channel-path counterpart test (test_channel_block_action_threads_under_clicked_message) so the DM-only guard is proven not to leak into channels, alongside the existing DM regression test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Slack adapter's ChangesDM block_actions thread_id phantom-reply fix
Sequence DiagramsequenceDiagram
participant Slack
participant SlackAdapter
participant ActionEvent
Slack->>SlackAdapter: block_actions (DM, no thread_ts)
SlackAdapter->>SlackAdapter: detect is_dm=True, set thread_ts=""
SlackAdapter->>ActionEvent: encode thread_id (channel, thread_ts="")
Slack->>SlackAdapter: block_actions (channel, no thread_ts)
SlackAdapter->>SlackAdapter: detect is_dm=False, set thread_ts=message_ts
SlackAdapter->>ActionEvent: encode thread_id (channel, thread_ts=message_ts)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the Slack adapter to prevent button clicks on top-level direct messages (DMs) from spawning phantom reply threads, ensuring they resolve to a DM-root thread ID with an empty thread timestamp. It also adds corresponding unit tests to verify this behavior for both DMs and standard channels, and updates the .gitignore file to ignore .DS_Store files. There are no review comments, and I have no feedback to provide.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/chat_sdk/adapters/slack/adapter.py`:
- Line 1627: Replace the truthiness-based fallback `thread_ts or ""` in the
encode_thread_id call with explicit None coalescing to comply with coding
guidelines. Change the expression to use `thread_ts if thread_ts is not None
else ""` instead of the `or` operator, which avoids truthiness traps by
explicitly checking for None rather than relying on truthiness evaluation. This
applies specifically to the thread_ts parameter being passed to SlackThreadId
within the encode_thread_id method call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c368d0f8-a1dd-4eea-9937-06af6c4c6f95
📒 Files selected for processing (3)
.gitignoresrc/chat_sdk/adapters/slack/adapter.pytests/test_slack_webhook.py
![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: 2b6c092c1c
ℹ️ 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".
| thread_id = "" | ||
| if channel and (thread_ts or message_ts): | ||
| thread_id = self.encode_thread_id(SlackThreadId(channel=channel, thread_ts=thread_ts or message_ts or "")) | ||
| thread_id = self.encode_thread_id(SlackThreadId(channel=channel, thread_ts=thread_ts or "")) |
There was a problem hiding this comment.
When a button is clicked on a top-level DM, this now emits an action thread_id like slack:D456:. That fixes event.thread.post(...), but action handlers still receive a normal thread; if they call event.thread.refresh() or event.open_modal(), the existing Slack fetch paths decode this id and unconditionally call conversations_replies with ts=decoded.thread_ts. With the newly empty thread_ts that sends ts='', so those DM action paths fail or lose the original message context. Please either teach the fetch paths to use channel history for empty DM thread IDs or avoid exposing an unfetchable thread id here.
Useful? React with 👍 / 👎.
Per CLAUDE.md port rule, prefer 'x if x is not None else default' over 'x or default'. Functionally identical here (only falsy value is the intended empty string) and keeps the SlackThreadId(thread_ts: str) guarantee that pyrefly checks.
) Upstream handleBlockActions falls back to messageTs even for DMs (phantom thread); we empty-case DMs to match upstream's own handleMessageEvent convention (index.ts:2158). Documented per the project divergence rule; cf. PR #107.
…ding-fix # Conflicts: # docs/UPSTREAM_SYNC.md
patrick-chinchill
added a commit
that referenced
this pull request
Jun 19, 2026
CHANGELOG: - Drop false gchat file-delivery bullet (PR #112 never merged; upstream itself does not implement gchat outbound file delivery) - Fix Slack DM block-action citation to merged PR #137 (supersedes #133) - Re-point 0.4.27.1 vehicle from unmerged #120 to tag v0.4.27.1 + #117 - Drop numberless Twilio 'scaffolding PR' (only #142 exists) UPSTREAM_SYNC: - Reframe Google Chat outbound file delivery as parity (upstream also logs 'not yet supported' + media.upload TODO, index.ts:1282-1289) - Correct Teams cert-auth row: TS is not 'Supported' — config throws at startup in both SDKs (types.ts:31, config.ts:13); parity, issue #58 - Add Messenger get_user row: upstream has no getUser; raising stub is parity, Graph-API impl tracked as #132
patrick-chinchill
added a commit
that referenced
this pull request
Jun 19, 2026
…PR 4/4) (#146) * chore(release): cut 0.4.30 — Teams SDK migration + 4.30.0 parity (#93 PR 4/4) Final PR of the Teams SDK migration (issue #93). PRs 1-3 (inbound+auth, outbound, native streaming) are merged; this cuts the 0.4.30 release. - Version bump: pyproject 0.4.29 -> 0.4.30; UPSTREAM_PARITY "4.29.0" -> "4.30.0". - Fidelity re-pin chat@4.29.0 -> chat@4.30.0 in lint.yml + verify_test_fidelity.py (docstring, default parity fallback, clone hint). packages/chat/src is byte-for-byte identical between the two tags, so zero new test ports: strict fidelity stays 100% (732/732, 0 missing) against chat@4.30.0. Baseline regenerated (ts_parity -> chat@4.30.0; the recorded total_ts_tests literal 731 -> 732 corrects a stale count from the merged adapter waves, not the re-pin — the count is identical against both tags). - Docs: project-instructions version map + fidelity pin; README status line; CHANGELOG 0.4.30 entry (Twilio adapter, Telegram streaming, Slack subpaths, WhatsApp/Slack/gchat fixes, Teams #93 PRs 1-4); UPSTREAM_SYNC.md parity header + the Teams deferral row flipped to delivered. - Version-label normalization: malformed `adapter-teams@chat@4.30.0` and loose `adapter-teams@4.30.0` -> `@chat-adapter/teams@4.30.0` in adapter.py (5), bridge.py (1), UPSTREAM_SYNC.md (4). Comment/doc-only. Does NOT tag/publish — the release is a separate maintainer-gated step (live Teams 429 streaming check + PyPI authorization). * docs(release): clarify 0.4.30 core-parity wording + finish label normalization Review fast-follow for PR 4. The CHANGELOG now states the mapped core is content-identical *between the chat@4.29.0 and chat@4.30.0 upstream tags* (verified: thread.ts/types.ts/thread.test.ts and the full packages/chat/src tree are byte-identical) — the prior 'unchanged from 4.29.0' phrasing was accurate but misread as a claim about our code. Also sweeps the two remaining old-style `adapter-teams@chat@4.30.0` labels in the Teams test docstrings to the canonical `@chat-adapter/teams@4.30.0` npm tag, completing the normalization the PR's scope called for. No logic change. * docs(release): document 0.4.30 parity-audit wave + remaining exceptions CHANGELOG: add 'Pre-existing parity gaps closed (4.30 audit)' subsection covering the 7 ported fixes (PRs #147-#150) and a documented-exceptions note (Linear agent-sessions #151 deferred to 4.31 / #152; adapter-web and the GitHub/Linear native-client + message.subject halves stay Known Non-Parity). UPSTREAM_SYNC: - file-mapping table: add state-ioredis -> redis.py (IoRedisStateAdapter) and state-pg -> postgres.py rows (both ported + tested, were missing). - Known Non-Parity (platform gaps): add Linear agent-sessions row (#151) and an adapter-web row that splits the portable server-side WebAdapter (deferred) from the genuinely browser-only client subpaths, correcting the earlier over-broad 'browser-only; no Python runtime' note. - Correct the Google Chat file-uploads row: inbound attachment parsing is fully implemented (_create_attachment), so the gap is outbound file delivery only (post_message still logs 'not yet supported'). * docs(release): correct phantom-feature framing before PyPI cut CHANGELOG: - Drop false gchat file-delivery bullet (PR #112 never merged; upstream itself does not implement gchat outbound file delivery) - Fix Slack DM block-action citation to merged PR #137 (supersedes #133) - Re-point 0.4.27.1 vehicle from unmerged #120 to tag v0.4.27.1 + #117 - Drop numberless Twilio 'scaffolding PR' (only #142 exists) UPSTREAM_SYNC: - Reframe Google Chat outbound file delivery as parity (upstream also logs 'not yet supported' + media.upload TODO, index.ts:1282-1289) - Correct Teams cert-auth row: TS is not 'Supported' — config throws at startup in both SDKs (types.ts:31, config.ts:13); parity, issue #58 - Add Messenger get_user row: upstream has no getUser; raising stub is parity, Graph-API impl tracked as #132 * docs(changelog): clarify 0.4.30 audit-gap count (8 closed + 1 deferred = 9)
patrick-chinchill
added a commit
that referenced
this pull request
Jun 21, 2026
…138) (#175) 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).
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.
Clean re-roll of #133 with the two local-review defects fixed. #133's head is on an external fork (
tony-chinchill-ai/chat-sdk-python) outside this session's push access, so the fixes land here on a main-repo branch instead. Supersedes #133 (please close #133 in favor of this).Original change (credit @tony-chinchill-ai, from #133)
When a HITL approval button is clicked in a Slack DM, anything the handler posts via
event.thread.post(...)became a phantom "1 reply" thread._handle_block_actionsfell back to the clicked message's ownts, unlike_handle_message_eventwhich already setsthread_ts=""for top-level DMs. This mirrors the DM handling into the block-action path; channel behavior is unchanged.Fixes applied on top (from the local review of #133)
thread_ts=thread_ts or ""with barethread_ts=thread_ts, lettingNoneflow intoSlackThreadId(thread_ts: str)(1 pyrefly bad-argument-type error). Restoredthread_ts or ""— keeps the DM intent and thestrguarantee..DS_Store— fix(slack): don't thread block-action responses in DMs #133 committed 3 macOS junk files; removed all three and added.DS_Storeto.gitignore.test_channel_block_action_threads_under_clicked_messagebut never included it; added it (asserts a channel click still threads under the clicked message'sts).Validation
pyrefly check src/: 0 errors (regression gone)pytest tests/test_slack_webhook.py: 74 passed;-k slack: 625 passedaudit_test_quality.py: 0 hard failurestest_dm_block_action_does_not_threadfail; restored.Independently re-reviewed clean (CLEAN-MERGE) against the current train tip.
https://claude.ai/code/session_013zwTcMek5rNqBTQvs2oF64
Generated by Claude Code
Summary by CodeRabbit
.gitignoreto ignore macOS.DS_Storefiles.This is a deliberate divergence from upstream, not merely an internal-consistency fix as the summary above implies. Upstream's
handleBlockActions(adapter-slack/src/index.ts:1455-1456,1470) computesthread_ts || container.thread_ts || messageTsand encodesthreadTs || messageTs || ""— it falls back to the clicked message'stseven for DMs, so upstream also spawns the phantom DM reply thread. The inconsistency exists in upstream too: itshandleMessageEventempty-cases DMs (:2158), buthandleBlockActionsdoes not. This PR extends upstream's own DM-message convention to the block-action path.docs/UPSTREAM_SYNC.md(this PR, commitb290251).stream()divergence) to restore parity.thread_tsis consumed unguarded byfetch_messages→conversations.replies(ts="")— identical to upstream (fetchMessagesindex.ts:4178has no empty-thread_tsguard) and to the already-faithful DM-message path. Aconversations.historyfallback for empty DMthread_tsis a separate, codebase-wide follow-up tracked in Slack: empty DM thread_ts hits conversations.replies(ts='') in fetch paths — add conversations.history fallback #138.