Skip to content

Commit 707b657

Browse files
fix(slack): thread team_id to chat.startStream for Enterprise Grid (#95) (#176)
chat.startStream fails with `team_not_found` on Enterprise Grid orgs because the per-workspace bot token alone isn't enough to disambiguate the team — Grid requires an explicit workspace `team_id`. Non-streaming chat.postMessage succeeds without it, so only the streaming path regressed: `stream()` built its `chat_stream(...)` kwargs from channel / thread_ts / recipient_user_id / recipient_team_id / task_display_mode and never passed a workspace `team_id`. Fix: thread `team_id=options.recipient_team_id` into the chat_stream call. slack_sdk's AsyncChatStream stashes unknown kwargs in `_stream_args` and forwards them to BOTH chat.startStream call sites (the eager first-flush path and the lazy stop-without-flush path), so this reaches the only API call that needs it. chat.appendStream / chat.stopStream don't receive `_stream_args` and don't require `team_id`. recipient_team_id is the team.id extracted on the inbound path (the workspace where the interaction happened = the streaming target workspace). Harmless on non-Grid workspaces — a correct team_id is always valid. Tests: a Grid regression in tests/test_slack_api.py simulating an Enterprise Grid workspace where post_message succeeds without team_id but the streaming start raises team_not_found when team_id is absent and succeeds when present; plus a mutation-guard test proving the simulation fails without team_id. Both use a local SlackApiError stand-in so they don't depend on slack_sdk being installed (the dev group doesn't pull it in; a fake module is injected under CI). Also refreshes scripts/fidelity_baseline.json ts_parity 4.30.0 -> 4.31.0 (stale since the 0.4.31 release; content already 0-missing) and adds a Known Non-Parity row to docs/UPSTREAM_SYNC.md documenting that upstream has the same Grid gap. Live Grid verification needs a real Enterprise Grid workspace; the unit test simulates it.
1 parent 3ed4a9a commit 707b657

4 files changed

Lines changed: 166 additions & 2 deletions

File tree

docs/UPSTREAM_SYNC.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -649,6 +649,7 @@ stay explicit instead of being rediscovered in code review.
649649
| 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. |
650650
| Fallback streaming stream-exception capture (non-Teams adapters) | `_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. This divergence does not apply to Teams: Teams DMs stream natively through the SDK `IStreamer` (`_stream_via_emit`), and a non-cancel iterator exception propagates straight to the caller while the SDK closes the streamer after the handler returns. |
651651
| Slack `stream()` to a top-level DM (empty `thread_ts`) | Normalizes the empty `thread_ts` to `None` and degrades to a single accumulated `post_message` call so the streamed reply still lands (chat-sdk-python#94) | Passes the empty `thread_ts` straight to `chat.startStream` (`adapter-slack/src/index.ts` `stream()`), which Slack rejects (`invalid_thread_ts`) — the streamed DM reply is silently dropped | Top-level DM messages intentionally encode `threadTs=""` on both sides (`_handle_message_event` / `handleMessageEvent`, "matches openDM subscriptions") — that part is faithful to upstream and **not** a bug. The bug is that upstream's `stream()` never reconciled that legitimate value with `startStream`'s requirement for a non-empty `thread_ts`; `postMessage` accepts no `thread_ts` for DMs, so we degrade instead of erroring. Tracked for contribution upstream — remove this divergence once vercel/chat fixes `stream()` to handle empty-`thread_ts` DM thread ids. |
652+
| Slack `stream()` on Enterprise Grid (`chat.startStream` `team_not_found`) | Threads the workspace `team_id` into `client.chat_stream(...)` (= `options.recipient_team_id`, the `team.id` extracted on the inbound path), which slack_sdk forwards into `_stream_args` → `chat.startStream`. `chat.appendStream`/`chat.stopStream` don't receive `_stream_args` and don't need `team_id`. Harmless on non-Grid workspaces — a correct `team_id` is always valid. (chat-sdk-python#95) | Builds the `chat.startStream` args from `channel`/`threadTs`/`recipientUserId`/`recipientTeamId`/`taskDisplayMode` only (`adapter-slack/src/index.ts` `stream()`); never passes a workspace `team_id`. On Grid orgs `chat.startStream` then fails with `team_not_found` (the per-workspace bot token alone isn't sufficient to disambiguate the team), even though `chat.postMessage` on the same workspace succeeds without it. | Upstream has the same gap — its `stream()` never threads `team_id`, so streaming is broken on Grid while non-streaming posts work. `chat.startStream` requires `team_id` for Grid disambiguation; `chat.postMessage` does not, which is why only streaming regresses. We source `team_id` from the already-plumbed `recipient_team_id` (the workspace where the interaction happened = the streaming target workspace). Live verification needs a real Grid workspace; the unit regression (`tests/test_slack_api.py::TestStream::test_stream_threads_team_id_to_chat_stream_for_grid` + the `team_not_found` mutation guard) simulates Grid by raising `team_not_found` from the streamer's lazy `chat.startStream` when `team_id` is absent. Tracked for contribution upstream. |
652653
| Fallback streaming final SentMessage content (non-Teams adapters) | 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 DMs stream through the SDK `IStreamer` and the Teams accumulate-and-post path ships raw `accumulated` via `post_message`, matching upstream; this divergence applies only to the remaining adapters that still route through `_fallback_stream`. |
653654
| Teams group-chat / channel streaming via accumulate-and-post | `TeamsAdapter.stream` accumulates the full text and issues a single `post_message` (SDK-backed) instead of post+edit, even for group chats and channel threads | Same (`@chat-adapter/teams@4.30.0`: `if (activeStream && !activeStream.canceled) … else { accumulate; postMessage }`) — no divergence at the adapter level | Documented for clarity: the Python port matches upstream's behavior of avoiding the post+edit flicker where Teams doesn't support native streaming. The buffered fallback routes through the same SDK `App.send` path as a normal `post_message`. |
654655
| Teams native streaming via the SDK `IStreamer` (DMs) | `TeamsAdapter._handle_message_activity` captures a Teams SDK `IStreamer` (`microsoft_teams.apps.StreamerProtocol` / `HttpStream`) for DMs via `app.activity_sender.create_stream(ref)`, registers it in `_active_streams`, and `await`s a `processing_done` gate (a wrapped `wait_until` shim) so the streamer stays alive while the handler streams. `stream()` → `_stream_via_emit` calls `stream.emit(text)` per chunk and NEVER calls `close()`; the adapter's `_handle_message_activity` `finally` calls `stream.close()` once (the lifecycle-owner role the SDK App's `process_activity` plays upstream). | `@chat-adapter/teams@4.30.0` `index.ts` does exactly this: `this.activeStreams.set(threadId, ctx.stream)`, build `processingDone` + wrapped `waitUntil`, `await processingDone`, `streamViaEmit` calls `stream.emit(text)` and never `close()` (the SDK App auto-closes after the handler returns). | **No adapter-level divergence.** The only mechanical difference is the close call site: upstream lets the SDK `App` auto-close `ctx.stream` because the SDK owns dispatch; our bridge overrides `server.on_request`, so we own dispatch and reproduce the close in `_handle_message_activity`'s `finally`. The SDK `HttpStream.close` no-ops when the stream was canceled or had no content, so closing in both success and cancel paths is safe (matching the SDK App, which closes in both its success and `StreamCancelledError` branches). Cancellation is detected via `stream.canceled` (checked before each emit) and by catching `StreamCancelledError` (other exceptions re-raise). The first chunk id is captured via `on_chunk` and awaited only when text was emitted and the stream was not canceled. Replaces the prior hand-rolled wire format, the 1500ms emit throttle, and the `RawMessage.text` / `update_interval_ms` divergences (all unwound in #93 PR 3). |

scripts/fidelity_baseline.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
2-
"_comment": "Ratchet-down baseline for scripts/verify_test_fidelity.py. This repo ships at strict fidelity for mapped core files (0 missing) against chat@4.30.0, so the baseline is empty. Scope: the MAPPING dict in scripts/verify_test_fidelity.py is the authoritative list of TS files checked; it currently covers 13 of the packages/chat/src/*.test.ts files. Default CI mode runs --strict via .github/workflows/lint.yml; this file is retained for local workflows that want to opt back into baseline mode (e.g. during an upstream sync where several ports land in flight). To baseline genuinely-divergent tests, run scripts/verify_test_fidelity.py --update-baseline after documenting the divergence in docs/UPSTREAM_SYNC.md.",
3-
"ts_parity": "chat@4.30.0",
2+
"_comment": "Ratchet-down baseline for scripts/verify_test_fidelity.py. This repo ships at strict fidelity for mapped core files (0 missing) against chat@4.31.0, so the baseline is empty. Scope: the MAPPING dict in scripts/verify_test_fidelity.py is the authoritative list of TS files checked; it currently covers 13 of the packages/chat/src/*.test.ts files. Default CI mode runs --strict via .github/workflows/lint.yml; this file is retained for local workflows that want to opt back into baseline mode (e.g. during an upstream sync where several ports land in flight). To baseline genuinely-divergent tests, run scripts/verify_test_fidelity.py --update-baseline after documenting the divergence in docs/UPSTREAM_SYNC.md.",
3+
"ts_parity": "chat@4.31.0",
44
"total_ts_tests": 732,
55
"total_missing": 0,
66
"missing": {}

src/chat_sdk/adapters/slack/adapter.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3926,6 +3926,22 @@ async def stream(
39263926
"thread_ts": thread_ts,
39273927
"recipient_user_id": options.recipient_user_id,
39283928
"recipient_team_id": options.recipient_team_id,
3929+
# Enterprise Grid disambiguation (chat-sdk-python#95). On Grid
3930+
# orgs ``chat.startStream`` fails with ``team_not_found`` unless
3931+
# the workspace ``team_id`` is supplied explicitly — the
3932+
# per-workspace bot token alone is not sufficient (whereas
3933+
# ``chat.postMessage`` succeeds without it). slack_sdk's
3934+
# ``chat_stream`` stashes unknown kwargs in ``_stream_args`` and
3935+
# forwards them to BOTH ``chat.startStream`` call sites (the
3936+
# eager first-flush path and the lazy stop-without-flush path),
3937+
# so passing ``team_id`` here threads it through to the only API
3938+
# call that needs it. ``chat.appendStream``/``chat.stopStream``
3939+
# do not receive ``_stream_args`` and do not require ``team_id``.
3940+
# ``recipient_team_id`` is the ``team.id`` extracted on the
3941+
# inbound path (the workspace where the interaction happened =
3942+
# the streaming target workspace). Harmless on non-Grid
3943+
# workspaces: passing the correct ``team_id`` is always valid.
3944+
"team_id": options.recipient_team_id,
39293945
}
39303946
if options.task_display_mode:
39313947
stream_kwargs["task_display_mode"] = options.task_display_mode

tests/test_slack_api.py

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,25 @@ def __init__(self, data: dict[str, Any]) -> None:
8989
self.data = data
9090

9191

92+
class _FakeSlackApiError(Exception):
93+
"""Mirror of ``slack_sdk.errors.SlackApiError`` for offline tests.
94+
95+
The dev-group install (``uv sync --group dev``, what CI runs) does NOT
96+
pull in ``slack_sdk`` (it lives in the ``slack`` / ``all`` extras), and
97+
``test_slack_client_cache.py`` injects a bare ``slack_sdk`` ModuleType
98+
into ``sys.modules`` that has no ``errors`` submodule. So importing the
99+
real ``SlackApiError`` is order-dependent and breaks under CI. This
100+
stand-in reproduces the attributes the adapter inspects: ``str(error)``
101+
contains the Slack error code and ``error.response`` is a dict carrying
102+
``{"ok": False, "error": <code>}`` (matching ``SlackApiError``'s shape).
103+
"""
104+
105+
def __init__(self, message: str, response: dict[str, Any]) -> None:
106+
self.response = response
107+
server_error = response.get("error")
108+
super().__init__(f"{message}\nThe server responded with: {{'ok': False, 'error': '{server_error}'}}")
109+
110+
92111
# =============================================================================
93112
# Helpers
94113
# =============================================================================
@@ -1651,6 +1670,134 @@ async def text_gen() -> AsyncIterator[str]:
16511670
assert call.kwargs["token"] == "xoxb-workspace-token"
16521671
assert mock_streamer.stop.call_args.kwargs["token"] == "xoxb-workspace-token"
16531672

1673+
@pytest.mark.asyncio
1674+
async def test_stream_threads_team_id_to_chat_stream_for_grid(self):
1675+
"""Regression for Enterprise Grid issue #95.
1676+
1677+
On Grid orgs ``chat.startStream`` fails with ``team_not_found``
1678+
unless a workspace ``team_id`` is supplied — the per-workspace bot
1679+
token alone is not sufficient (whereas ``chat.postMessage``
1680+
succeeds without it). slack_sdk's ``chat_stream`` forwards unknown
1681+
kwargs (incl. ``team_id``) to the underlying ``chat.startStream``
1682+
call, so the adapter must pass ``team_id`` = ``recipient_team_id``.
1683+
1684+
This test simulates Grid: a ``post_message`` succeeds with no
1685+
``team_id``, while the streaming start raises ``team_not_found``
1686+
when ``team_id`` is absent and only succeeds when it is present.
1687+
It MUST FAIL on pre-fix code (which omitted ``team_id``).
1688+
"""
1689+
adapter, client, _ = await _init_adapter()
1690+
1691+
# Grid-aware streamer: the lazy ``chat.startStream`` (triggered on
1692+
# the first append or on stop, exactly as the real slack_sdk
1693+
# ``AsyncChatStream`` does) raises ``team_not_found`` unless the
1694+
# workspace ``team_id`` was threaded through ``chat_stream``.
1695+
class _GridStreamer:
1696+
def __init__(self, stream_kwargs: dict[str, Any]) -> None:
1697+
self._stream_kwargs = stream_kwargs
1698+
self._started = False
1699+
1700+
def _start_stream(self) -> None:
1701+
# Mirrors slack_sdk forwarding ``_stream_args`` (which holds
1702+
# ``chat_stream``'s kwargs) to ``chat.startStream``.
1703+
if not self._stream_kwargs.get("team_id"):
1704+
raise _FakeSlackApiError(
1705+
message="team_not_found",
1706+
response={"ok": False, "error": "team_not_found"},
1707+
)
1708+
self._started = True
1709+
1710+
async def append(self, **kwargs: Any) -> dict[str, Any]:
1711+
if not self._started:
1712+
self._start_stream()
1713+
return {"ok": True}
1714+
1715+
async def stop(self, **kwargs: Any) -> dict[str, Any]:
1716+
if not self._started:
1717+
self._start_stream()
1718+
return {"ok": True, "ts": "1234567890.951951"}
1719+
1720+
captured: dict[str, Any] = {}
1721+
1722+
async def chat_stream(**kwargs: Any) -> _GridStreamer:
1723+
captured.update(kwargs)
1724+
return _GridStreamer(kwargs)
1725+
1726+
client.chat_stream = AsyncMock(side_effect=chat_stream)
1727+
client.set_response("chat_postMessage", {"ok": True, "ts": "1234567890.000111"})
1728+
1729+
# Sanity: a non-streaming post_message has NO team_id requirement on
1730+
# the same Grid workspace (it succeeds without one).
1731+
post_result = await adapter.post_message(
1732+
"slack:C_GRID:1234567890.000000",
1733+
"plain post on grid", # type: ignore[arg-type]
1734+
)
1735+
assert post_result.id == "1234567890.000111" # post_message OK, no team_id
1736+
for call in client.calls:
1737+
if call["method"] == "chat_postMessage":
1738+
assert "team_id" not in call["kwargs"]
1739+
1740+
async def text_gen() -> AsyncIterator[str]:
1741+
yield "streamed hello on grid"
1742+
1743+
result = await adapter.stream(
1744+
"slack:C_GRID:1234567890.000000",
1745+
text_gen(),
1746+
StreamOptions(recipient_user_id="U_GRID", recipient_team_id="T_GRID_WS"),
1747+
)
1748+
1749+
# The stream completed (chat.startStream did NOT raise team_not_found)
1750+
# because the workspace team_id was threaded through.
1751+
assert result.id == "1234567890.951951"
1752+
# The fix passes team_id = recipient_team_id to chat_stream.
1753+
client.chat_stream.assert_awaited_once()
1754+
assert captured["team_id"] == "T_GRID_WS"
1755+
assert captured["recipient_team_id"] == "T_GRID_WS"
1756+
1757+
@pytest.mark.asyncio
1758+
async def test_stream_raises_team_not_found_without_team_id_on_grid(self):
1759+
"""Mutation guard for issue #95: prove the Grid simulation actually
1760+
fails when ``team_id`` is missing.
1761+
1762+
This drives the same Grid-aware streamer but with ``chat_stream``
1763+
stripped of any ``team_id`` (mimicking the pre-fix code path), and
1764+
asserts the stream start raises ``team_not_found``. This anchors the
1765+
positive test above: if the fix were reverted, the streamer would
1766+
raise here, so the positive test cannot pass vacuously.
1767+
"""
1768+
adapter, client, _ = await _init_adapter()
1769+
1770+
class _GridStreamer:
1771+
async def append(self, **kwargs: Any) -> dict[str, Any]:
1772+
raise _FakeSlackApiError(
1773+
message="team_not_found",
1774+
response={"ok": False, "error": "team_not_found"},
1775+
)
1776+
1777+
async def stop(self, **kwargs: Any) -> dict[str, Any]:
1778+
raise _FakeSlackApiError(
1779+
message="team_not_found",
1780+
response={"ok": False, "error": "team_not_found"},
1781+
)
1782+
1783+
async def chat_stream(**kwargs: Any) -> _GridStreamer:
1784+
# Simulate slack_sdk dropping team_id (pre-fix behavior): the
1785+
# underlying chat.startStream then fails on Grid.
1786+
kwargs.pop("team_id", None)
1787+
return _GridStreamer()
1788+
1789+
client.chat_stream = AsyncMock(side_effect=chat_stream)
1790+
1791+
async def text_gen() -> AsyncIterator[str]:
1792+
yield "streamed hello on grid"
1793+
1794+
with pytest.raises(_FakeSlackApiError, match="team_not_found"):
1795+
await adapter.stream(
1796+
"slack:C_GRID:1234567890.000000",
1797+
text_gen(),
1798+
StreamOptions(recipient_user_id="U_GRID", recipient_team_id="T_GRID_WS"),
1799+
)
1800+
16541801

16551802
# =============================================================================
16561803
# Public request-context accessors (issue #47)

0 commit comments

Comments
 (0)