diff --git a/.gitignore b/.gitignore index 4c15006..f4c5502 100644 --- a/.gitignore +++ b/.gitignore @@ -20,3 +20,4 @@ uv.lock coverage.xml htmlcov/ AGENTS.md +.DS_Store diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 79f4cf4..78ee097 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -637,6 +637,7 @@ 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. | | `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 446289c..857574e 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -1842,10 +1842,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" @@ -1856,7 +1861,12 @@ 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 if thread_ts is not None else "", + ) + ) is_ephemeral = (payload.get("container") or {}).get("is_ephemeral") is True response_url = payload.get("response_url") diff --git a/tests/test_slack_webhook.py b/tests/test_slack_webhook.py index 2638789..d200ec1 100644 --- a/tests/test_slack_webhook.py +++ b/tests/test_slack_webhook.py @@ -337,6 +337,78 @@ 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_channel_block_action_threads_under_clicked_message(self): + """A click on a top-level channel message must thread under that message. + + Counterpart to the DM case: channels DO have threads, so a button click on + a top-level (no thread_ts) channel message must fall back to the clicked + message's own ts so the response card threads under it. The DM-only guard + must not leak into channels. + """ + 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": "C456"}, + "channel": {"id": "C456", "name": "general"}, + "message": {"ts": "1234567890.123456"}, # top-level channel 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 == "C456" + assert decoded.thread_ts == "1234567890.123456", ( + f"channel block action resolved to thread_ts {decoded.thread_ts!r}; a top-level " + "channel click must thread under the clicked message's own ts, not a DM-root " + "empty thread_ts." + ) + @pytest.mark.asyncio async def test_returns_400_for_missing_payload(self): adapter = _make_adapter()