fix(slack): don't thread block-action responses in DMs#133
fix(slack): don't thread block-action responses in DMs#133tony-chinchill-ai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 50 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSlack adapter's ChangesDM-aware thread ID computation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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. 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 modifies the Slack adapter to prevent block actions on top-level Direct Messages (DMs) from creating phantom reply threads, as DMs do not support threading. It introduces a check to identify DM channels and ensures that the thread timestamp does not fall back to the message timestamp in those cases, while preserving the existing threading behavior for regular channels. Unit tests have been added to verify both behaviors. No review comments were provided, so there is no additional feedback to address.
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: The value thread_ts may be None and violates the
SlackThreadId.thread_ts: str contract; before constructing SlackThreadId in the
encode_thread_id call, normalize thread_ts to a guaranteed string (e.g., convert
non-None to str and provide a safe default like an empty string when None) so
SlackThreadId(thread_ts=...) always receives a str; update the code path that
assigns thread_id (the line calling encode_thread_id and SlackThreadId) to use
this normalized thread_ts.
🪄 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: 6a67aea4-47be-4797-b98c-2a491619672f
📒 Files selected for processing (2)
src/chat_sdk/adapters/slack/adapter.pytests/test_slack_webhook.py
| 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)) |
There was a problem hiding this comment.
Normalize thread_ts to a guaranteed str before SlackThreadId construction.
CI is failing here because thread_ts is inferred as possibly None. This breaks the SlackThreadId.thread_ts: str contract and blocks type checks.
Suggested fix
- thread_ts = (
+ thread_ts_raw = (
(payload.get("message") or {}).get("thread_ts")
or (payload.get("container") or {}).get("thread_ts")
or ("" if is_dm else message_ts)
)
+ thread_ts = str(thread_ts_raw) if thread_ts_raw is not None else ""
is_view_action = (payload.get("container") or {}).get("type") == "view"
if not (is_view_action or channel):
self._logger.warn("Missing channel in block_actions", {"channel": channel})
return
@@
thread_id = ""
if channel and (thread_ts or message_ts):
thread_id = self.encode_thread_id(SlackThreadId(channel=channel, thread_ts=thread_ts))🧰 Tools
🪛 GitHub Actions: Lint & Type Check / 0_Lint & Type Check.txt
[error] 1627-1627: Type checking failed (pyrefly). Argument Literal[''] | Unknown | None is not assignable to parameter thread_ts with type str in chat_sdk.adapters.slack.types.SlackThreadId.__init__ [bad-argument-type].
🪛 GitHub Actions: Lint & Type Check / Lint & Type Check
[error] 1627-1627: pyrefly type check failed with [bad-argument-type]: Argument Literal[''] | Unknown | None is not assignable to parameter thread_ts with type str in chat_sdk.adapters.slack.types.SlackThreadId.__init__.
🤖 Prompt for 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.
In `@src/chat_sdk/adapters/slack/adapter.py` at line 1627, The value thread_ts may
be None and violates the SlackThreadId.thread_ts: str contract; before
constructing SlackThreadId in the encode_thread_id call, normalize thread_ts to
a guaranteed string (e.g., convert non-None to str and provide a safe default
like an empty string when None) so SlackThreadId(thread_ts=...) always receives
a str; update the code path that assigns thread_id (the line calling
encode_thread_id and SlackThreadId) to use this normalized thread_ts.
Source: Pipeline failures
_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>
tony-chinchill-ai
force-pushed
the
fix/slack-dm-block-action-threading
branch
from
1a5697a to
32d99f5
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
) 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.
…) (#137) * fix(slack): don't thread block-action responses in DMs _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> * fix(slack): restore thread_ts str-coalesce + drop committed .DS_Store 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. * fix(slack): use explicit None-coalesce for DM thread_ts at encode time 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. * docs(sync): record Slack DM block-action threading divergence (#133/#137) 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. --------- Co-authored-by: tony-chinchill <tony@chinchill.ai> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
…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)
1 participant
Problem
When a HITL approval button is clicked in a Slack DM, anything the handler posts via
event.thread.post(...)(e.g. an approval result card) becomes a phantom "1 reply" thread in the DM. DMs should never have reply threads.Root cause
_handle_block_actionssetthread_tsby falling back to the clicked message's ownts, with no DM special-casing — unlike its sibling_handle_message_event, which already setsthread_ts=""for top-level DM messages. The two had diverged.Fix
Mirror
_handle_message_event's DM handling in_handle_block_actions: detect DMs (channelstarts withD) and never fall back tomessage_tsfor a top-level DM click. A real in-DMthread_ts(a click inside an existing DM thread) is still preserved; channel behavior is unchanged.Tests (
tests/test_slack_webhook.py)test_dm_block_action_does_not_thread— regression: a DM click resolves to a DM-rootthread_id(emptythread_ts); fails when the fix is reverted.test_channel_block_action_threads_under_clicked_message— guards that channel threading is preserved.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes