Skip to content

fix(slack): don't thread block-action responses in DMs #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tony-chinchill-ai wants to merge 1 commit into from
Closed

fix(slack): don't thread block-action responses in DMs #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added .DS_Store
View file
Open in desktop
Binary file not shown.
Binary file added src/.DS_Store
View file
Open in desktop
Binary file not shown.
Binary file added src/chat_sdk/.DS_Store
View file
Open in desktop
Binary file not shown.
9 changes: 7 additions & 2 deletions src/chat_sdk/adapters/slack/adapter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1605,10 +1605,15 @@ def _handle_block_actions(self, payload: dict[str, Any], options: WebhookOptions

channel = (payload.get("channel") or {}).get("id") or (payload.get("container") or {}).get("channel_id")
message_ts = (payload.get("message") or {}).get("ts") or (payload.get("container") or {}).get("message_ts")
# DMs have no threads: a button click on a top-level DM message must not
# thread the response. Mirror _handle_message_event's DM handling — keep a
# real in-DM thread_ts, but never fall back to the clicked message's own
# ts, which would spawn a phantom "1 reply" thread in the DM.
is_dm = bool(channel) and channel.startswith("D")
thread_ts = (
(payload.get("message") or {}).get("thread_ts")
or (payload.get("container") or {}).get("thread_ts")
or message_ts
or ("" if is_dm else message_ts)
)

is_view_action = (payload.get("container") or {}).get("type") == "view"
Expand All @@ -1619,7 +1624,7 @@ def _handle_block_actions(self, payload: dict[str, Any], options: WebhookOptions

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))

@coderabbitai coderabbitai Bot Jun 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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


is_ephemeral = (payload.get("container") or {}).get("is_ephemeral") is True
response_url = payload.get("response_url")
Expand Down
36 changes: 36 additions & 0 deletions tests/test_slack_webhook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,42 @@ async def test_handles_block_actions(self):
response = await adapter.handle_webhook(req)
assert response["status"] == 200

@pytest.mark.asyncio
async def test_dm_block_action_does_not_thread(self):
"""A click on a top-level DM message must resolve to a DM-root thread_id.

Regression: _handle_block_actions fell back to message_ts for thread_ts
in DMs, so HITL approval result cards posted via event.thread.post became
phantom "1 reply" threads in the DM. DMs have no threads — the ActionEvent
must carry an empty thread_ts, mirroring _handle_message_event.
"""
adapter = _make_adapter()
chat = _make_mock_chat(_make_mock_state())
await adapter.initialize(chat)

req = self._make_interactive_req(
{
"type": "block_actions",
"user": {"id": "U123", "username": "testuser", "name": "Test User"},
"container": {"type": "message", "message_ts": "1234567890.123456", "channel_id": "D456"},
"channel": {"id": "D456", "name": "directmessage"},
"message": {"ts": "1234567890.123456"}, # top-level DM message: no thread_ts
"actions": [{"type": "button", "action_id": "approve_btn", "value": "approved"}],
}
)
response = await adapter.handle_webhook(req)
assert response["status"] == 200

chat.process_action.assert_called_once()
action_event = chat.process_action.call_args[0][0]
decoded = adapter.decode_thread_id(action_event.thread_id)
assert decoded.channel == "D456"
assert decoded.thread_ts == "", (
f"DM block action threaded under {decoded.thread_ts!r}; a top-level DM click "
"must resolve to a DM-root thread_id (empty thread_ts), else the approval "
"result card posts as a phantom reply thread."
)

@pytest.mark.asyncio
async def test_returns_400_for_missing_payload(self):
adapter = _make_adapter()
Expand Down
Loading