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..d1f70e6 100644 --- a/src/chat_sdk/adapters/slack/adapter.py +++ b/src/chat_sdk/adapters/slack/adapter.py @@ -252,6 +252,43 @@ 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`. + + 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. + """ + return self._get_client() + # ------------------------------------------------------------------ # Token management # ------------------------------------------------------------------ @@ -2030,7 +2067,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..1e29972 100644 --- a/src/chat_sdk/chat.py +++ b/src/chat_sdk/chat.py @@ -1378,6 +1378,84 @@ 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. + """ + # Validate thread ID shape structurally, before calling the adapter. + # A valid ID is `{adapter}:{channel}[:{rest}...]` with: + # - non-empty adapter prefix + # - 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 or not remainder: + raise ChatError(f"Invalid thread ID: {thread_id}") + channel_segment, _, _ = remainder.partition(":") + if not channel_segment: + 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 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: + raise ChatError(f"Invalid 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), + # 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) + # ======================================================================== # Adapter inference # ======================================================================== diff --git a/tests/test_chat_resolver.py b/tests/test_chat_resolver.py index f755762..c89530e 100644 --- a/tests/test_chat_resolver.py +++ b/tests/test_chat_resolver.py @@ -426,3 +426,139 @@ 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), + # 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 + + 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_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` 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"] + adapter.persist_message_history = 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 + + chat = _make_chat("slack") + with pytest.raises(ChatError, match="Invalid thread ID"): + chat.thread("no-colon-here") + + def test_empty_remainder_raises(self): + """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") + 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) + + 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..c5fc7ed 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,76 @@ 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() + + 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(