Skip to content

Commit 916e532

Browse files
committed
fix(slack): accumulate-and-post for empty thread_ts in stream() (closes #94)
The vercel/chat#292 empty-thread_ts guard correctly avoided chat.startStream's invalid_thread_ts rejection, but raising ValidationError silently dropped top-level Slack DM streaming replies because Thread._handle_stream does not catch and _fallback_stream only triggers when the adapter has no stream method at all. Replace the raise with the production-tested accumulate-and-post path Tony described in chat-sdk-python#94: degrade DMs to a single non-streamed post_message call (Slack accepts an empty thread_ts on chat.postMessage) and keep native streaming for channels. This lets chinchill-api retire its MultiTenantSlackAdapter subclass override.
1 parent 636d1e8 commit 916e532

2 files changed

Lines changed: 85 additions & 21 deletions

File tree

src/chat_sdk/adapters/slack/adapter.py

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
ModalResponse,
8484
ModalSubmitEvent,
8585
OptionsLoadEvent,
86+
PostableMarkdown,
8687
RawMessage,
8788
ReactionEvent,
8889
ScheduledMessage,
@@ -2543,14 +2544,23 @@ async def stream(
25432544
decoded = self.decode_thread_id(thread_id)
25442545
channel = decoded.channel
25452546
# Normalize empty thread_ts to None to avoid Slack API "invalid_thread_ts" errors.
2546-
# Stream requires a real thread context — bail out when missing.
2547+
# ``chat.startStream`` rejects an empty thread_ts (top-level DMs have no
2548+
# parent thread to attach to), but ``chat.postMessage`` accepts it.
2549+
# Degrade DMs to a single accumulated post_message call so streaming
2550+
# replies aren't silently dropped (chat-sdk-python#94).
25472551
thread_ts = decoded.thread_ts or None
25482552
if not thread_ts:
2549-
self._logger.debug("Slack: stream skipped - no thread context")
2550-
raise ValidationError(
2551-
"slack",
2552-
"Slack streaming requires a valid thread context (non-empty thread_ts)",
2553+
self._logger.debug(
2554+
"Slack: stream degraded to post_message - no thread context",
2555+
{"channel": channel},
25532556
)
2557+
accumulated = ""
2558+
async for chunk in text_stream:
2559+
if isinstance(chunk, str):
2560+
accumulated += chunk
2561+
elif hasattr(chunk, "type") and chunk.type == "markdown_text": # type: ignore[union-attr]
2562+
accumulated += chunk.text # type: ignore[union-attr]
2563+
return await self.post_message(thread_id, PostableMarkdown(markdown=accumulated))
25542564
self._logger.debug("Slack: starting stream", {"channel": channel, "threadTs": thread_ts})
25552565

25562566
token = self._get_token()

tests/test_slack_api.py

Lines changed: 70 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1365,9 +1365,11 @@ class TestEmptyThreadTsGuards:
13651365
13661366
What to fix if this fails: each Slack API call (``chat_postMessage``,
13671367
``chat_postEphemeral``, ``chat_scheduleMessage``) must pass ``None``
1368-
instead of the empty string for ``thread_ts``. ``stream`` has stricter
1369-
requirements — it must raise ``ValidationError`` because the streaming
1370-
API has no top-level-DM concept.
1368+
instead of the empty string for ``thread_ts``. ``stream`` must degrade
1369+
to a single accumulated ``post_message`` call when ``thread_ts`` is
1370+
empty — ``chat.startStream`` rejects empty thread_ts but
1371+
``chat.postMessage`` accepts it, and raising would silently drop
1372+
top-level DM streaming replies (chat-sdk-python#94).
13711373
"""
13721374

13731375
@pytest.mark.asyncio
@@ -1386,27 +1388,79 @@ async def test_schedule_message_with_empty_thread_ts_normalizes_to_none(self):
13861388
assert calls[0]["kwargs"]["thread_ts"] is None
13871389

13881390
@pytest.mark.asyncio
1389-
async def test_stream_with_empty_thread_ts_raises_validation_error(self):
1391+
async def test_stream_with_empty_thread_ts_degrades_to_post_message(self):
1392+
"""Top-level DMs (empty ``thread_ts``) must accumulate the stream and
1393+
post a single non-streamed message instead of raising.
1394+
1395+
What to fix if this fails: ``SlackAdapter.stream`` must, when
1396+
``thread_ts`` is empty, drain ``text_stream`` (concatenating
1397+
``str`` chunks and ``markdown_text`` chunk text), call
1398+
``post_message`` exactly once with a ``PostableMarkdown``
1399+
wrapping the accumulated text, and never invoke
1400+
``chat_stream``. Raising here silently drops top-level DM
1401+
replies because ``Thread._handle_stream`` does not catch
1402+
adapter exceptions (chat-sdk-python#94).
1403+
"""
13901404
adapter, client, _ = await _init_adapter()
1391-
# Stream needs a real thread context — top-level DMs can't stream.
1392-
# The guard must fire BEFORE chat_stream is invoked, otherwise
1393-
# Slack returns a generic invalid_thread_ts that's hard to debug.
1405+
# Streaming API mock that must NOT be touched on the empty-thread path.
13941406
mock_streamer = MagicMock()
13951407
mock_streamer.append = AsyncMock()
13961408
mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "0"}})
13971409
client.chat_stream = AsyncMock(return_value=mock_streamer)
1410+
client.set_response("chat_postMessage", {"ok": True, "ts": "5555.5555"})
13981411

1399-
async def text_gen() -> AsyncIterator[str]:
1400-
yield "should not be sent"
1412+
from chat_sdk.types import MarkdownTextChunk
14011413

1402-
with pytest.raises(ValidationError, match="thread"):
1403-
await adapter.stream(
1404-
"slack:C123:",
1405-
text_gen(),
1406-
StreamOptions(recipient_user_id="U1", recipient_team_id="T1"),
1407-
)
1408-
# The mock should never have been touched.
1414+
async def text_gen() -> AsyncIterator[str | StreamChunk]:
1415+
yield "Hello "
1416+
yield MarkdownTextChunk(text="from ")
1417+
yield "DM"
1418+
1419+
result = await adapter.stream(
1420+
"slack:C123:",
1421+
text_gen(),
1422+
StreamOptions(recipient_user_id="U1", recipient_team_id="T1"),
1423+
)
1424+
1425+
# Native streaming API must not have been used.
14091426
assert not client.chat_stream.called
1427+
# post_message was invoked exactly once with the accumulated text.
1428+
post_calls = client.get_calls("chat_postMessage")
1429+
assert len(post_calls) == 1
1430+
assert post_calls[0]["kwargs"]["channel"] == "C123"
1431+
assert post_calls[0]["kwargs"]["thread_ts"] is None
1432+
assert post_calls[0]["kwargs"]["text"] == "Hello from DM"
1433+
assert result.id == "5555.5555"
1434+
1435+
@pytest.mark.asyncio
1436+
async def test_stream_with_thread_ts_uses_native_streaming(self):
1437+
"""Non-empty ``thread_ts`` must keep using ``chat_stream`` so the
1438+
channel-thread streaming path is not regressed by the DM fallback.
1439+
1440+
What to fix if this fails: the empty-``thread_ts`` guard in
1441+
``SlackAdapter.stream`` must only fire when ``thread_ts`` is
1442+
falsy. For real thread contexts, ``chat_stream`` must still be
1443+
awaited and ``chat_postMessage`` must NOT be called.
1444+
"""
1445+
adapter, client, _ = await _init_adapter()
1446+
mock_streamer = MagicMock()
1447+
mock_streamer.append = AsyncMock()
1448+
mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "9.9"}})
1449+
client.chat_stream = AsyncMock(return_value=mock_streamer)
1450+
1451+
async def text_gen() -> AsyncIterator[str]:
1452+
yield "channel reply"
1453+
1454+
result = await adapter.stream(
1455+
"slack:C123:1234567890.000000",
1456+
text_gen(),
1457+
StreamOptions(recipient_user_id="U1", recipient_team_id="T1"),
1458+
)
1459+
1460+
assert client.chat_stream.called
1461+
assert client.chat_stream.call_args.kwargs["thread_ts"] == "1234567890.000000"
1462+
assert client.get_calls("chat_postMessage") == []
1463+
assert result.id == "9.9"
14101464

14111465
@pytest.mark.asyncio
14121466
async def test_post_message_with_zero_string_thread_ts_passes_through(self):

0 commit comments

Comments
 (0)