From 0a12e571443e01e17736992efaa3ac743d2ef03b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 23 Apr 2026 09:04:54 +0000 Subject: [PATCH 1/5] =?UTF-8?q?fix:=20adapter=20hygiene=20=E2=80=94=20slac?= =?UTF-8?q?k=20stream=20await,=20teams=20divider,=20public=20worker=20APIs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundles four open adapter issues into one PR. - #44 SlackAdapter.stream(): await chat_stream(...). Without the await the code called .append on an unawaited coroutine and native Slack streaming failed on the first chunk. Existing tests were updated to use AsyncMock for chat_stream so they mirror the real AsyncWebClient. - #45 Teams divider now renders a visible separator. Dividers between siblings hoist separator: True onto the next element; a trailing divider emits a minimal non-empty Container. An empty Container with separator: True renders at zero height in Microsoft Teams. Documented as a divergence from upstream (same bug in TS). - #46 New Chat.thread(thread_id, *, current_message=None) public factory. Mirrors TS chat.thread(threadId). Lets worker processes rebuild a Thread without reaching into ThreadImpl/_ThreadImplConfig internals. current_message is preserved for Slack native streaming (it populates recipient_user_id / recipient_team_id). - #47 New SlackAdapter.current_token / current_client properties. Public accessors for the request-context-bound token and a preconfigured AsyncWebClient. Replaces underscore-prefixed _get_token / _get_client access from consumer code that makes direct Slack Web API calls from inside a handler. Tests and CHANGELOG updated. UPSTREAM_SYNC.md divergence table updated for #45 and #47. https://claude.ai/code/session_01MXobPieFH93txkgm38oMpC --- CHANGELOG.md | 10 ++++ docs/UPSTREAM_SYNC.md | 2 + src/chat_sdk/adapters/slack/adapter.py | 33 ++++++++++- src/chat_sdk/adapters/teams/cards.py | 47 ++++++++++++++- src/chat_sdk/chat.py | 48 ++++++++++++++++ tests/test_chat_resolver.py | 65 +++++++++++++++++++++ tests/test_slack_api.py | 79 +++++++++++++++++++++++++- tests/test_teams_cards.py | 73 +++++++++++++++++++++++- 8 files changed, 349 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e236453..a3f4c7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # Changelog +## Unreleased + +### Fixes +- **Slack native streaming no longer crashes on first chunk** (issue #44): `SlackAdapter.stream()` now awaits `AsyncWebClient.chat_stream(...)`; the previous code called `.append()` on an unawaited coroutine, raising `AttributeError` and forcing callers onto the post+edit fallback. Existing tests were updated to use `AsyncMock` for `chat_stream` so they mirror the real client. +- **Teams divider now renders a visible separator line** (issue #45): `card_to_adaptive_card` previously emitted an empty `Container` with `separator: True`, which Microsoft Teams renders at zero height. The new behavior hoists `separator: True` onto the following sibling (or emits a minimal non-empty Container for a trailing divider). Upstream TS ships the same bug; documented as a divergence in [UPSTREAM_SYNC.md](docs/UPSTREAM_SYNC.md). + +### Python-only additions +- **`SlackAdapter.current_token` / `current_client`** (issue #47): public `@property` accessors that return the request-context-bound bot token and a preconfigured `AsyncWebClient`. Replaces reaching into `_get_token()` / `_get_client()` from consumer code that needs to call the Slack Web API directly from inside a handler (email resolution, user profile fetches, etc.). TS keeps `getToken()` private; documented as a Python-only extension in [UPSTREAM_SYNC.md](docs/UPSTREAM_SYNC.md). +- **`Chat.thread(thread_id, *, current_message=None)`** (issue #46): public worker-reconstruction factory mirroring TS `chat.thread(threadId)`. Adapter is inferred from the thread ID prefix; state and message history come from the Chat instance. Pass `current_message` when the worker needs Slack native streaming (it populates `recipient_user_id` / `recipient_team_id`). + ## 0.4.26 (2026-04-16) Synced to [Vercel Chat 4.26.0](https://github.com/vercel/chat). diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 9bda8ed..1d2d89b 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -455,6 +455,8 @@ stay explicit instead of being rediscovered in code review. | Google Chat image rendering | Images emit as `{alt} ({url})` or bare `url` | No image branch — falls through to default which concatenates children only, dropping the URL | Upstream silently drops image URLs when rendering to Google Chat text. We preserve the URL so the message content isn't lost. | | Fallback streaming stream-exception capture | `_fallback_stream` captures exceptions from the stream iterator, flushes whatever content was already rendered, awaits `pending_edit`, and re-raises after cleanup | `try/finally` only — exception propagates immediately, `pendingEdit` is un-awaited, and the placeholder is stranded as `"..."` | Upstream leaves a hard UX failure when streams crash mid-flight (common: LLM connection drops): placeholder visible forever, orphan background task. We flush + clean up before re-raising so the caller still sees the original error and users see the partial content instead of a spinner. | | Fallback streaming final SentMessage content | SentMessage + final edit carry `final_content` (remend'd — inline markers auto-closed) | SentMessage + final edit carry raw `accumulated` | Narrow UX refinement. If a stream ends with an unclosed `*`/`~~`/etc., upstream ships the unclosed marker; we run `_remend` so the user sees a clean final message. Not observable in the common case where streams close their own markers. | +| Teams divider rendering | `card_to_adaptive_card` hoists `separator: True` onto the next sibling (or emits a non-empty Container for a trailing divider) | `convertDividerToElement` emits an empty `Container` with `separator: True` | Upstream shares the same bug: Microsoft Teams renders an empty Container at zero height, so the separator line is effectively invisible. Python port fixes locally (issue #45) rather than blocking on upstream. | +| `SlackAdapter.current_token` / `current_client` | Public `@property` accessors that return the request-context-bound token and a preconfigured `AsyncWebClient` | Not exposed (`getToken()` is private on the TS `SlackAdapter`) | Python-only addition (issue #47). Downstream code that calls Slack Web APIs from inside a handler — email resolution, user profile fetches, reaction bookkeeping — otherwise depends on underscore-prefixed helpers. | ### Platform-specific gaps diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index 884fa77..b072679 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -252,6 +252,37 @@ def lock_scope(self) -> str: def persist_message_history(self) -> bool: return self._persist_message_history + # ------------------------------------------------------------------ + # Public request-context accessors + # + # These are Python-only extensions to the Adapter surface. They let + # code running inside a handler call the Slack Web API directly — + # e.g. ``users.info`` for caller-email resolution — without + # reaching into the underscore-prefixed ``_get_token`` / + # ``_get_client`` helpers. See docs/UPSTREAM_SYNC.md. + # ------------------------------------------------------------------ + + @property + def current_token(self) -> str: + """Return the bot token bound to the current request context. + + In multi-workspace mode this is the token resolved by the + ``InstallationStore`` for the current request; in single-workspace + mode it is the default bot token. Raises + :class:`AuthenticationError` when called outside a request context + with no default token configured. + """ + return self._get_token() + + @property + def current_client(self) -> Any: + """Return an ``AsyncWebClient`` preconfigured with :attr:`current_token`. + + The returned client is LRU-cached by token. Raises + :class:`AuthenticationError` when no token is available. + """ + return self._get_client() + # ------------------------------------------------------------------ # Token management # ------------------------------------------------------------------ @@ -2030,7 +2061,7 @@ async def stream( if options.task_display_mode: stream_kwargs["task_display_mode"] = options.task_display_mode - streamer = client.chat_stream(**stream_kwargs) + streamer = await client.chat_stream(**stream_kwargs) first = True last_appended = "" diff --git a/src/chat_sdk/adapters/teams/cards.py b/src/chat_sdk/adapters/teams/cards.py index fc58692..d3c42a3 100644 --- a/src/chat_sdk/adapters/teams/cards.py +++ b/src/chat_sdk/adapters/teams/cards.py @@ -31,6 +31,10 @@ # Used when a card has select/radio_select inputs but no submit button. AUTO_SUBMIT_ACTION_ID = "__auto_submit" +# Internal marker key for divider placeholders. Stripped by +# ``_hoist_dividers`` before the card is serialized — never reaches Teams. +_DIVIDER_MARKER = "__chatSdkDivider" + def _convert_emoji(text: str) -> str: """Convert emoji placeholders to Teams format.""" @@ -96,6 +100,8 @@ def card_to_adaptive_card(card: CardElement) -> dict[str, Any]: body.extend(result["elements"]) actions.extend(result["actions"]) + body = _hoist_dividers(body) + adaptive_card: dict[str, Any] = { "type": "AdaptiveCard", "$schema": ADAPTIVE_CARD_SCHEMA, @@ -121,7 +127,14 @@ def _convert_child_to_adaptive(child: CardChild) -> dict[str, Any]: if child_type == "image": return {"elements": [_convert_image_to_element(child)], "actions": []} # type: ignore[arg-type] if child_type == "divider": - return {"elements": [{"type": "Container", "separator": True, "items": []}], "actions": []} + # Emit an internal marker instead of the final Container. The + # post-processing pass (_hoist_dividers) either moves ``separator`` + # onto the next sibling (preferred — renders as a full-width line) + # or, for a trailing divider with no next sibling, replaces it with + # a minimal non-empty Container so the separator is visible. An + # empty ``Container`` with ``separator: True`` renders at zero + # height in Microsoft Teams. + return {"elements": [{_DIVIDER_MARKER: True}], "actions": []} if child_type == "actions": return _convert_actions_to_elements(child) # type: ignore[arg-type] if child_type == "section": @@ -150,6 +163,37 @@ def _convert_child_to_adaptive(child: CardChild) -> dict[str, Any]: return {"elements": [], "actions": []} +def _hoist_dividers(elements: list[dict[str, Any]]) -> list[dict[str, Any]]: + """Replace internal divider markers with ``separator: True`` on the next sibling. + + An Adaptive Card ``Container`` with ``separator: True`` and no ``items`` + renders at zero height in Microsoft Teams — the separator line is + effectively invisible. Instead, hoist the separator onto the following + sibling so it renders as a full-width line above that element. For a + trailing divider with no next sibling, emit a non-empty Container so + the separator is still visible. + """ + result: list[dict[str, Any]] = [] + pending = False + for el in elements: + if el.get(_DIVIDER_MARKER): + pending = True + continue + if pending: + el = {**el, "separator": True} + pending = False + result.append(el) + if pending: + result.append( + { + "type": "Container", + "separator": True, + "items": [{"type": "TextBlock", "text": " ", "wrap": False}], + } + ) + return result + + def _convert_text_to_element(element: TextElement) -> dict[str, Any]: """Convert a text element to an Adaptive Card TextBlock.""" content = element.get("content", "") @@ -319,6 +363,7 @@ def _convert_section_to_elements(element: SectionElement) -> dict[str, Any]: container_items.extend(result["elements"]) actions.extend(result["actions"]) + container_items = _hoist_dividers(container_items) if container_items: elements.append({"type": "Container", "items": container_items}) diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 3a79ef1..a0eddd8 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -1378,6 +1378,54 @@ def channel(self, channel_id: str) -> ChannelImpl: ) ) + def thread( + self, + thread_id: str, + *, + current_message: Message | None = None, + ) -> ThreadImpl: + """Get a Thread by its thread ID (e.g. 'slack:C123ABC:1234567890.123456'). + + The adapter is resolved from the thread ID prefix; state and message + history come from this Chat instance. This is the public + construction path for worker processes — use it instead of + reaching into ``ThreadImpl`` / ``_ThreadImplConfig`` directly. + + Parameters + ---------- + thread_id: + Fully-qualified thread ID. Must start with a registered + adapter name followed by ``:``. + current_message: + Optional reference to the message the worker is responding to. + Required for Slack native streaming, which populates + ``recipient_user_id`` / ``recipient_team_id`` from the author + of this message. Omit for post-only worker flows. + + Mirrors ``chat.thread(threadId)`` from the upstream TS SDK. + """ + adapter_name = thread_id.split(":")[0] if ":" in thread_id else "" + if not adapter_name: + raise ChatError(f"Invalid thread ID: {thread_id}") + adapter = self._adapters.get(adapter_name) + if adapter is None: + raise ChatError(f'Adapter "{adapter_name}" not found for thread ID "{thread_id}"') + + stub_message = ( + current_message + if current_message is not None + else Message( + id="", + thread_id=thread_id, + text="", + formatted={"type": "root", "children": []}, + raw=None, + author=Author(user_id="", user_name="", full_name="", is_bot=False, is_me=False), + metadata=MessageMetadata(date_sent=datetime.now(tz=timezone.utc), edited=False), + ) + ) + return self._create_thread(adapter, thread_id, stub_message, False) + # ======================================================================== # Adapter inference # ======================================================================== diff --git a/tests/test_chat_resolver.py b/tests/test_chat_resolver.py index f755762..ef4c4f8 100644 --- a/tests/test_chat_resolver.py +++ b/tests/test_chat_resolver.py @@ -426,3 +426,68 @@ async def task(chat: Chat, name: str) -> None: await asyncio.gather(task(chat_a, "ca"), task(chat_b, "cb")) assert results["ca"] == "ca" assert results["cb"] == "cb" + + +class TestChatThreadFactory: + """``Chat.thread(thread_id)`` — public worker-reconstruction path (issue #46). + + Mirrors ``chat.thread(threadId)`` from the upstream TS SDK. Lets worker + processes rebuild a Thread bound to this Chat's state and the adapter + inferred from the thread ID prefix — no need to reach into + ``ThreadImpl`` / ``_ThreadImplConfig`` directly. + """ + + def setup_method(self): + clear_chat_singleton() + + def teardown_method(self): + clear_chat_singleton() + + def test_infers_adapter_from_thread_id_prefix(self): + chat = _make_chat("slack") + thread = chat.thread("slack:C123:1234567890.123456") + assert thread.adapter.name == "slack" + assert thread.id == "slack:C123:1234567890.123456" + + def test_propagates_explicit_current_message(self): + """Slack native streaming reads ``current_message`` to populate + recipient IDs; the public factory must let workers supply it. + """ + from datetime import datetime, timezone + + from chat_sdk import Author, Message, MessageMetadata + + chat = _make_chat("slack") + thread_id = "slack:C123:1234567890.123456" + msg = Message( + id="M1", + thread_id=thread_id, + text="hi", + formatted={"type": "root", "children": []}, + raw=None, + author=Author(user_id="U1", user_name="alice", full_name="Alice", is_bot=False, is_me=False), + metadata=MessageMetadata(date_sent=datetime.now(tz=timezone.utc), edited=False), + ) + thread = chat.thread(thread_id, current_message=msg) + assert thread._current_message is msg + + def test_omitting_current_message_stubs_a_placeholder(self): + """Workers that only post (no streaming) can omit ``current_message``.""" + chat = _make_chat("slack") + thread = chat.thread("slack:C123:1234567890.123456") + assert thread._current_message is not None + assert thread._current_message.id == "" + + def test_invalid_thread_id_raises(self): + from chat_sdk.errors import ChatError + + chat = _make_chat("slack") + with pytest.raises(ChatError, match="Invalid thread ID"): + chat.thread("no-colon-here") + + def test_unregistered_adapter_raises(self): + from chat_sdk.errors import ChatError + + chat = _make_chat("slack") + with pytest.raises(ChatError, match='Adapter "teams" not found'): + chat.thread("teams:foo:bar") diff --git a/tests/test_slack_api.py b/tests/test_slack_api.py index 07d7822..925d773 100644 --- a/tests/test_slack_api.py +++ b/tests/test_slack_api.py @@ -897,7 +897,7 @@ async def test_stream_with_markdown_text_chunks(self): mock_streamer = MagicMock() mock_streamer.append = AsyncMock() mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "999.999"}}) - client.chat_stream = MagicMock(return_value=mock_streamer) + client.chat_stream = AsyncMock(return_value=mock_streamer) from chat_sdk.types import MarkdownTextChunk @@ -922,7 +922,7 @@ async def test_stream_with_task_update_chunks(self): mock_streamer = MagicMock() mock_streamer.append = AsyncMock() mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "888.888"}}) - client.chat_stream = MagicMock(return_value=mock_streamer) + client.chat_stream = AsyncMock(return_value=mock_streamer) from chat_sdk.types import TaskUpdateChunk @@ -947,7 +947,7 @@ async def test_stream_with_plan_update_chunks(self): mock_streamer = MagicMock() mock_streamer.append = AsyncMock() mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "777.777"}}) - client.chat_stream = MagicMock(return_value=mock_streamer) + client.chat_stream = AsyncMock(return_value=mock_streamer) from chat_sdk.types import PlanUpdateChunk @@ -964,6 +964,79 @@ async def chunk_gen() -> AsyncIterator[StreamChunk | str]: assert result.id == "777.777" + @pytest.mark.asyncio + async def test_stream_awaits_chat_stream_coroutine(self): + """Regression test for issue #44: ``AsyncWebClient.chat_stream`` is a + coroutine function. Without ``await``, ``streamer`` is a coroutine + and calling ``.append`` raises ``AttributeError``. This test uses + an ``AsyncMock`` (mirroring the real client) so the fix is required + to pass. + """ + adapter, client, _ = await _init_adapter() + + mock_streamer = MagicMock() + mock_streamer.append = AsyncMock() + mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "444.444"}}) + client.chat_stream = AsyncMock(return_value=mock_streamer) + + async def text_gen() -> AsyncIterator[str]: + yield "hello" + + result = await adapter.stream( + "slack:C123:1234567890.000000", + text_gen(), + StreamOptions(recipient_user_id="U1", recipient_team_id="T1"), + ) + + assert result.id == "444.444" + client.chat_stream.assert_awaited_once() + assert mock_streamer.append.await_count >= 1 + + +# ============================================================================= +# Public request-context accessors (issue #47) +# ============================================================================= + + +class TestPublicContextAccessors: + """``current_token`` / ``current_client`` expose the same values as the + underscore-prefixed helpers without forcing callers into private API. + """ + + @pytest.mark.asyncio + async def test_current_token_returns_default_bot_token_in_single_workspace(self): + adapter, _, _ = await _init_adapter() + assert adapter.current_token == "xoxb-test-token" + + @pytest.mark.asyncio + async def test_current_client_returns_preconfigured_client(self): + adapter, mock_client, _ = await _init_adapter() + # ``_patch_client`` redirects ``_get_client`` to the MockSlackClient, + # so the public accessor must route through the same path. + assert adapter.current_client is mock_client + + @pytest.mark.asyncio + async def test_current_token_honors_per_request_context(self): + adapter, _, _ = await _init_adapter() + + async def within_ctx() -> str: + return adapter.current_token + + from chat_sdk.adapters.slack.types import RequestContext + + token = adapter._request_context.set(RequestContext(token="xoxb-per-request", bot_user_id="U_PER")) + try: + assert adapter.current_token == "xoxb-per-request" + finally: + adapter._request_context.reset(token) + + def test_current_token_raises_without_any_token(self): + from chat_sdk.shared.errors import AuthenticationError + + adapter = _make_adapter(bot_token=None) + with pytest.raises(AuthenticationError): + _ = adapter.current_token + # ============================================================================= # parseMessage -- complex edge cases diff --git a/tests/test_teams_cards.py b/tests/test_teams_cards.py index de851e0..021bea4 100644 --- a/tests/test_teams_cards.py +++ b/tests/test_teams_cards.py @@ -93,11 +93,78 @@ def test_image_elements(self): "size": "auto", } - def test_divider_elements(self): - card = Card(children=[Divider()]) + def test_divider_hoists_separator_onto_next_sibling(self): + """Regression test for issue #45: a divider between siblings should set + ``separator: True`` on the following element rather than emitting an + empty Container (which Teams renders at zero height). + """ + card = Card( + children=[ + CardText("above"), + Divider(), + CardText("below"), + ] + ) + adaptive = card_to_adaptive_card(card) + assert len(adaptive["body"]) == 2 + assert adaptive["body"][0] == {"type": "TextBlock", "text": "above", "wrap": True} + assert adaptive["body"][1] == { + "type": "TextBlock", + "text": "below", + "wrap": True, + "separator": True, + } + + def test_divider_leading_hoists_onto_first_sibling(self): + card = Card(children=[Divider(), CardText("only")]) adaptive = card_to_adaptive_card(card) assert len(adaptive["body"]) == 1 - assert adaptive["body"][0] == {"type": "Container", "separator": True, "items": []} + assert adaptive["body"][0] == { + "type": "TextBlock", + "text": "only", + "wrap": True, + "separator": True, + } + + def test_divider_trailing_falls_back_to_non_empty_container(self): + """A divider with no following sibling must still be visible — an + empty Container with ``separator: True`` renders at zero height, so + emit a minimal non-empty Container instead. + """ + card = Card(children=[CardText("above"), Divider()]) + adaptive = card_to_adaptive_card(card) + assert len(adaptive["body"]) == 2 + assert adaptive["body"][0] == {"type": "TextBlock", "text": "above", "wrap": True} + trailing = adaptive["body"][1] + assert trailing["type"] == "Container" + assert trailing["separator"] is True + assert trailing["items"], "trailing-divider Container must not be empty" + + def test_divider_never_leaks_internal_marker_key(self): + """The internal marker key used during conversion must never appear + in the final Adaptive Card payload sent to Teams. + """ + card = Card( + children=[ + CardText("a"), + Divider(), + Divider(), + CardText("b"), + Divider(), + ] + ) + adaptive = card_to_adaptive_card(card) + + def _walk(node): + if isinstance(node, dict): + for key, value in node.items(): + assert not key.startswith("__chatSdk"), f"leaked marker key: {key}" + _walk(value) + elif isinstance(node, list): + for item in node: + _walk(item) + + _walk(adaptive) def test_actions_with_buttons(self): card = Card( From cd3a45d64fba61c0a081810d9c6f1df50920bd91 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 13:36:30 -0700 Subject: [PATCH 2/5] address review comments on PR #48 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - **`Chat.thread()`**: validate adapter-specific remainder of thread ID, not just the prefix. Previously `slack:` or `slack::` would construct a ThreadImpl with an empty channel ID that blew up later on the first adapter call — raise ChatError at construction time instead. Also defer to `adapter.channel_id_from_thread_id()` to catch invalid platform-specific shapes (CodeRabbit minor). - **Stub message date_sent**: use `datetime.fromtimestamp(0, tz=UTC)` instead of `datetime.now()` — makes the method deterministic and keeps tests repeatable (Gemini medium). - **Test for empty remainder**: new `test_empty_remainder_raises` covering `"slack:"` and `"slack::"`. - **Test for shared state/history**: new `test_reuses_parent_chat_state_and_history` asserting the factory binds the new Thread to the parent Chat's `_state_adapter` and `_message_history`. This surfaced a real finding — `_create_thread` skips message_history when `adapter.persist_message_history` is falsy. Added `test_omits_history_when_adapter_does_not_persist` to lock in that intentional behavior (CodeRabbit minor, actually caught a real contract gap in tests). - **Test datetime**: fixed timestamp `datetime(2024, 1, 1, ...)` in `test_propagates_explicit_current_message` (Gemini medium). - **Remove unused `within_ctx` local function** from `test_current_token_honors_per_request_context` (CodeQL). - **`AsyncWebClient` forward ref**: `current_client` now returns `"AsyncWebClient"` with a TYPE_CHECKING import, replacing `Any` (Gemini medium). Ruff auto-stripped the quotes since `from __future__ import annotations` is in effect. Validation: - `uv run ruff check src/ tests/ scripts/` — clean - `uv run ruff format --check src/ tests/ scripts/` — clean - `uv run pytest tests/ --tb=short -q` — 3485 passed, 2 skipped --- src/chat_sdk/adapters/slack/adapter.py | 7 +++- src/chat_sdk/chat.py | 22 +++++++++-- tests/test_chat_resolver.py | 54 +++++++++++++++++++++++++- tests/test_slack_api.py | 3 -- 4 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index b072679..df2b290 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -21,7 +21,10 @@ from collections.abc import AsyncIterable, Awaitable, Callable from contextvars import ContextVar from datetime import datetime, timezone -from typing import Any +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from slack_sdk.web.async_client import AsyncWebClient from urllib.parse import parse_qs from chat_sdk.adapters.slack.cards import ( @@ -275,7 +278,7 @@ def current_token(self) -> str: return self._get_token() @property - def current_client(self) -> Any: + def current_client(self) -> AsyncWebClient: """Return an ``AsyncWebClient`` preconfigured with :attr:`current_token`. The returned client is LRU-cached by token. Raises diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index a0eddd8..2d89f13 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -1404,13 +1404,27 @@ def thread( Mirrors ``chat.thread(threadId)`` from the upstream TS SDK. """ - adapter_name = thread_id.split(":")[0] if ":" in thread_id else "" - if not adapter_name: + # Validate thread ID shape: must be `{adapter}:{remainder}` with a + # non-empty adapter prefix AND non-empty remainder. Without the + # remainder check, `"slack:"` or `"slack::"` would construct a + # ThreadImpl with an empty channel ID that blows up on the first + # adapter call — surface the error here instead. + adapter_name, sep, remainder = thread_id.partition(":") + if not sep or not adapter_name or not remainder: raise ChatError(f"Invalid thread ID: {thread_id}") adapter = self._adapters.get(adapter_name) if adapter is None: raise ChatError(f'Adapter "{adapter_name}" not found for thread ID "{thread_id}"') + # Defer to the adapter to derive channel_id — it knows the + # platform-specific encoding. Empty channel_id also means malformed. + try: + derived_channel_id = adapter.channel_id_from_thread_id(thread_id) + except Exception as exc: + raise ChatError(f"Invalid thread ID: {thread_id}") from exc + if not derived_channel_id or derived_channel_id == f"{adapter_name}:": + raise ChatError(f"Invalid thread ID: {thread_id}") + stub_message = ( current_message if current_message is not None @@ -1421,7 +1435,9 @@ def thread( formatted={"type": "root", "children": []}, raw=None, author=Author(user_id="", user_name="", full_name="", is_bot=False, is_me=False), - metadata=MessageMetadata(date_sent=datetime.now(tz=timezone.utc), edited=False), + # Deterministic timestamp for the stub message — `datetime.now()` + # makes this method non-deterministic and harder to test. + metadata=MessageMetadata(date_sent=datetime.fromtimestamp(0, tz=timezone.utc), edited=False), ) ) return self._create_thread(adapter, thread_id, stub_message, False) diff --git a/tests/test_chat_resolver.py b/tests/test_chat_resolver.py index ef4c4f8..448dbb5 100644 --- a/tests/test_chat_resolver.py +++ b/tests/test_chat_resolver.py @@ -466,7 +466,8 @@ def test_propagates_explicit_current_message(self): formatted={"type": "root", "children": []}, raw=None, author=Author(user_id="U1", user_name="alice", full_name="Alice", is_bot=False, is_me=False), - metadata=MessageMetadata(date_sent=datetime.now(tz=timezone.utc), edited=False), + # Fixed timestamp — `datetime.now()` makes tests non-deterministic. + metadata=MessageMetadata(date_sent=datetime(2024, 1, 1, tzinfo=timezone.utc), edited=False), ) thread = chat.thread(thread_id, current_message=msg) assert thread._current_message is msg @@ -478,6 +479,44 @@ def test_omitting_current_message_stubs_a_placeholder(self): assert thread._current_message is not None assert thread._current_message.id == "" + def test_reuses_parent_chat_state_and_history(self): + """The factory must bind the new Thread to the parent Chat's state + adapter and message history. This is the core contract of + `chat.thread()` — worker processes reconstruct a Thread that + shares state with the original Chat instance, not a fresh + detached thread. Without this, state writes/reads from the worker + wouldn't be visible to the Chat in-process. + + For adapters that don't persist message history (persist_message_history + falsy), `_create_thread` intentionally skips the history bind — + there's no cache to share. This test uses a persisting adapter to + exercise the positive case. + """ + chat = _make_chat("slack") + # Opt the mock adapter into message history so the factory binds it + adapter = chat._adapters["slack"] + adapter.persist_message_history = True + + thread = chat.thread("slack:C123:1234567890.123456") + # State adapter must be the exact same instance, not a copy + assert thread._state_adapter is chat._state_adapter + # Message history must be the exact same instance too + assert thread._message_history is chat._message_history + + def test_omits_history_when_adapter_does_not_persist(self): + """Adapters with `persist_message_history=False`/None opt out of + the shared history cache. `Chat.thread()` respects that: the + Thread gets `None` for history rather than a shared cache the + adapter won't populate. + """ + chat = _make_chat("slack") + adapter = chat._adapters["slack"] + assert not adapter.persist_message_history # default on mock is None + + thread = chat.thread("slack:C123:1234567890.123456") + assert thread._state_adapter is chat._state_adapter + assert thread._message_history is None + def test_invalid_thread_id_raises(self): from chat_sdk.errors import ChatError @@ -485,6 +524,19 @@ def test_invalid_thread_id_raises(self): with pytest.raises(ChatError, match="Invalid thread ID"): chat.thread("no-colon-here") + def test_empty_remainder_raises(self): + """`slack:` or `slack::` would create a thread with empty channel + ID that blows up on the first adapter call — surface the error + at construction time instead. + """ + from chat_sdk.errors import ChatError + + chat = _make_chat("slack") + with pytest.raises(ChatError, match="Invalid thread ID"): + chat.thread("slack:") + with pytest.raises(ChatError, match="Invalid thread ID"): + chat.thread("slack::") + def test_unregistered_adapter_raises(self): from chat_sdk.errors import ChatError diff --git a/tests/test_slack_api.py b/tests/test_slack_api.py index 925d773..c5fc7ed 100644 --- a/tests/test_slack_api.py +++ b/tests/test_slack_api.py @@ -1019,9 +1019,6 @@ async def test_current_client_returns_preconfigured_client(self): async def test_current_token_honors_per_request_context(self): adapter, _, _ = await _init_adapter() - async def within_ctx() -> str: - return adapter.current_token - from chat_sdk.adapters.slack.types import RequestContext token = adapter._request_context.set(RequestContext(token="xoxb-per-request", bot_user_id="U_PER")) From f839e7a0e2bc66a903b4ae5be62000da186dad20 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 13:43:02 -0700 Subject: [PATCH 3/5] =?UTF-8?q?revert=20slack=20current=5Fclient=20forward?= =?UTF-8?q?=20ref=20=E2=80=94=20trips=20PR=20#48's=20pyrefly=20baseline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #48 is based off main, which doesn't yet have PR #49's replace-imports-with-any submodule wildcards (slack_sdk.*). The TYPE_CHECKING import of AsyncWebClient from slack_sdk.web.async_client therefore tripped missing-import on this branch. Reverted to 'current_client -> Any' with a docstring capturing the actual runtime type. Once PR #49 merges, we can promote to a typed forward ref in a follow-up (or in the next release). --- src/chat_sdk/adapters/slack/adapter.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/chat_sdk/adapters/slack/adapter.py b/src/chat_sdk/adapters/slack/adapter.py index df2b290..d1f70e6 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -21,10 +21,7 @@ from collections.abc import AsyncIterable, Awaitable, Callable from contextvars import ContextVar from datetime import datetime, timezone -from typing import TYPE_CHECKING, Any - -if TYPE_CHECKING: - from slack_sdk.web.async_client import AsyncWebClient +from typing import Any from urllib.parse import parse_qs from chat_sdk.adapters.slack.cards import ( @@ -278,9 +275,15 @@ def current_token(self) -> str: return self._get_token() @property - def current_client(self) -> AsyncWebClient: + def current_client(self) -> Any: """Return an ``AsyncWebClient`` preconfigured with :attr:`current_token`. + Return type is ``Any`` (rather than the concrete + ``AsyncWebClient``) because ``slack_sdk`` is an optional + dependency — consumers who install the SDK without the `slack` + extra shouldn't pay a type-check-time import cost. Docstring + captures the actual runtime type for tooling that reads it. + The returned client is LRU-cached by token. Raises :class:`AuthenticationError` when no token is available. """ From 3edf2e50367019a94dbc136aa09b04d97d165216 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 13:52:15 -0700 Subject: [PATCH 4/5] address self-review findings on PR #48 - **Structural thread-ID validation**: reject `slack::`, `slack:::`, `slack::::` etc. via `any(seg for seg in remainder.split(":"))` BEFORE calling the adapter. Previously relied on `adapter.channel_id_from_thread_id()` returning exactly `"{adapter}:"` to catch the empty-remainder case, but different adapters return different things for malformed input (MockAdapter returns `"slack:"`, real Slack regex-matches and returns the full string unchanged, etc.). The structural check doesn't depend on adapter-specific behavior, so it's a single source of truth. - **Expand `test_empty_remainder_raises`** to cover `slack:::` and `slack::::` variants. - **Clean up `test_omits_history_when_adapter_does_not_persist`**: explicitly set `adapter.persist_message_history = None` instead of asserting on the mock's default. Prevents silent test regression if the mock default ever changes, and makes intent unambiguous. --- src/chat_sdk/chat.py | 27 ++++++++++++++++++--------- tests/test_chat_resolver.py | 31 +++++++++++++++++++------------ 2 files changed, 37 insertions(+), 21 deletions(-) diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 2d89f13..1bf2f5b 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -1404,25 +1404,34 @@ def thread( Mirrors ``chat.thread(threadId)`` from the upstream TS SDK. """ - # Validate thread ID shape: must be `{adapter}:{remainder}` with a - # non-empty adapter prefix AND non-empty remainder. Without the - # remainder check, `"slack:"` or `"slack::"` would construct a - # ThreadImpl with an empty channel ID that blows up on the first - # adapter call — surface the error here instead. + # Validate thread ID shape structurally, before calling the adapter. + # A valid ID is `{adapter}:{segment}[:{segment}...]` with: + # - non-empty adapter prefix + # - at least one non-empty segment after the prefix + # Rejects `"slack:"`, `"slack::"`, `"slack::::"` etc. at the SDK + # boundary so we don't rely on each adapter's + # `channel_id_from_thread_id` doing its own defense. (Different + # adapters return different things for malformed input; we need + # a single source of truth.) adapter_name, sep, remainder = thread_id.partition(":") - if not sep or not adapter_name or not remainder: + if not sep or not adapter_name: raise ChatError(f"Invalid thread ID: {thread_id}") + # Remainder is empty or only colons → no actual content. + if not remainder or not any(seg for seg in remainder.split(":")): + raise ChatError(f"Invalid thread ID: {thread_id}") + adapter = self._adapters.get(adapter_name) if adapter is None: raise ChatError(f'Adapter "{adapter_name}" not found for thread ID "{thread_id}"') - # Defer to the adapter to derive channel_id — it knows the - # platform-specific encoding. Empty channel_id also means malformed. + # Defer to the adapter to derive channel_id as a secondary sanity + # check — some platform-specific patterns can pass the structural + # check but still be malformed (e.g. wrong number of segments). try: derived_channel_id = adapter.channel_id_from_thread_id(thread_id) except Exception as exc: raise ChatError(f"Invalid thread ID: {thread_id}") from exc - if not derived_channel_id or derived_channel_id == f"{adapter_name}:": + if not derived_channel_id: raise ChatError(f"Invalid thread ID: {thread_id}") stub_message = ( diff --git a/tests/test_chat_resolver.py b/tests/test_chat_resolver.py index 448dbb5..f25d0ad 100644 --- a/tests/test_chat_resolver.py +++ b/tests/test_chat_resolver.py @@ -504,14 +504,18 @@ def test_reuses_parent_chat_state_and_history(self): assert thread._message_history is chat._message_history def test_omits_history_when_adapter_does_not_persist(self): - """Adapters with `persist_message_history=False`/None opt out of - the shared history cache. `Chat.thread()` respects that: the - Thread gets `None` for history rather than a shared cache the - adapter won't populate. + """Adapters with `persist_message_history` falsy opt out of the + shared history cache. `Chat.thread()` respects that: the Thread + gets `None` for history rather than a shared cache the adapter + won't populate. + + Explicitly set `persist_message_history = None` rather than + relying on the mock's default — prevents silent test regression + if the mock default ever changes. """ chat = _make_chat("slack") adapter = chat._adapters["slack"] - assert not adapter.persist_message_history # default on mock is None + adapter.persist_message_history = None thread = chat.thread("slack:C123:1234567890.123456") assert thread._state_adapter is chat._state_adapter @@ -525,17 +529,20 @@ def test_invalid_thread_id_raises(self): chat.thread("no-colon-here") def test_empty_remainder_raises(self): - """`slack:` or `slack::` would create a thread with empty channel - ID that blows up on the first adapter call — surface the error - at construction time instead. + """`slack:` or `slack::`, or any string where every segment after + the adapter prefix is empty, would create a thread with no real + content. Surface the error at construction time instead of + relying on adapter-specific `channel_id_from_thread_id` to + catch it (adapters differ in what they return for malformed + input — some return the input, some return a partial prefix — + so we validate structurally before dispatching to the adapter). """ from chat_sdk.errors import ChatError chat = _make_chat("slack") - with pytest.raises(ChatError, match="Invalid thread ID"): - chat.thread("slack:") - with pytest.raises(ChatError, match="Invalid thread ID"): - chat.thread("slack::") + for bad in ("slack:", "slack::", "slack:::", "slack::::"): + with pytest.raises(ChatError, match="Invalid thread ID"): + chat.thread(bad) def test_unregistered_adapter_raises(self): from chat_sdk.errors import ChatError From 3297b3b457ed05d1ede0de84c2fb32b951cf9018 Mon Sep 17 00:00:00 2001 From: patrick-chinchill Date: Thu, 23 Apr 2026 14:06:18 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix=20channel-segment=20check=20=E2=80=94?= =?UTF-8?q?=20`slack::thread`=20was=20bypassing=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught: 'any segment non-empty' accepted empty-channel IDs like `slack::thread` because the thread segment was non-empty. The intent is specifically to require a non-empty **channel** segment. Fix: after splitting off the adapter prefix, partition the remainder on ':' and check the first piece. Added 2 regression cases: - `slack::thread` (empty channel, non-empty thread) - `slack::foo:bar` (empty channel, multi-segment rest) --- src/chat_sdk/chat.py | 19 ++++++++++++------- tests/test_chat_resolver.py | 28 ++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/chat_sdk/chat.py b/src/chat_sdk/chat.py index 1bf2f5b..1e29972 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -1405,19 +1405,24 @@ def thread( Mirrors ``chat.thread(threadId)`` from the upstream TS SDK. """ # Validate thread ID shape structurally, before calling the adapter. - # A valid ID is `{adapter}:{segment}[:{segment}...]` with: + # A valid ID is `{adapter}:{channel}[:{rest}...]` with: # - non-empty adapter prefix - # - at least one non-empty segment after the prefix - # Rejects `"slack:"`, `"slack::"`, `"slack::::"` etc. at the SDK - # boundary so we don't rely on each adapter's + # - non-empty **channel** segment (the part between the first + # and second colon, if there's a second colon; or the whole + # remainder if not) + # Rejects `"slack:"`, `"slack::"`, `"slack::thread"`, etc. at + # the SDK boundary so we don't rely on each adapter's # `channel_id_from_thread_id` doing its own defense. (Different # adapters return different things for malformed input; we need # a single source of truth.) + # + # Checking just "any segment is non-empty" would wrongly accept + # `"slack::thread"` (empty channel, non-empty thread). adapter_name, sep, remainder = thread_id.partition(":") - if not sep or not adapter_name: + if not sep or not adapter_name or not remainder: raise ChatError(f"Invalid thread ID: {thread_id}") - # Remainder is empty or only colons → no actual content. - if not remainder or not any(seg for seg in remainder.split(":")): + channel_segment, _, _ = remainder.partition(":") + if not channel_segment: raise ChatError(f"Invalid thread ID: {thread_id}") adapter = self._adapters.get(adapter_name) diff --git a/tests/test_chat_resolver.py b/tests/test_chat_resolver.py index f25d0ad..c89530e 100644 --- a/tests/test_chat_resolver.py +++ b/tests/test_chat_resolver.py @@ -529,18 +529,30 @@ def test_invalid_thread_id_raises(self): chat.thread("no-colon-here") def test_empty_remainder_raises(self): - """`slack:` or `slack::`, or any string where every segment after - the adapter prefix is empty, would create a thread with no real - content. Surface the error at construction time instead of - relying on adapter-specific `channel_id_from_thread_id` to - catch it (adapters differ in what they return for malformed - input — some return the input, some return a partial prefix — - so we validate structurally before dispatching to the adapter). + """IDs where the channel segment is empty — even when later + segments are populated — would create a thread with no channel + context. Surface the error at construction time rather than + relying on adapter-specific `channel_id_from_thread_id` (which + may legitimately return empty, raise, or pass through the + original input depending on adapter). + + Crucial cases: `slack::thread` and `slack::foo:bar` have an + empty channel segment but non-empty later segments. An "any + segment non-empty" check would accept these; the structural + validation checks the channel segment specifically. """ from chat_sdk.errors import ChatError chat = _make_chat("slack") - for bad in ("slack:", "slack::", "slack:::", "slack::::"): + bad_ids = ( + "slack:", + "slack::", + "slack:::", + "slack::::", + "slack::thread", # empty channel, non-empty thread + "slack::foo:bar", # empty channel, multi-segment rest + ) + for bad in bad_ids: with pytest.raises(ChatError, match="Invalid thread ID"): chat.thread(bad)