fix: adapter hygiene — Slack stream await, Teams divider, public worker APIs#48
Merged
Merged
Conversation
…er APIs 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
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughFixes Slack streaming by awaiting Changes
Sequence Diagram(s)sequenceDiagram
participant Adapter as SlackAdapter
participant Client as AsyncWebClient
participant Streamer as ChatStreamer
Adapter->>Client: call chat_stream(stream_kwargs)
Note right of Client: previously returned coroutine (not awaited)
Adapter-->>Client: await chat_stream(stream_kwargs)
Client->>Streamer: returns initialized streamer
Adapter->>Streamer: await Streamer.append(markdown_text=delta, token=token)
Streamer-->>Adapter: append completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a Slack streaming crash by awaiting the chat_stream coroutine and resolves a Teams divider rendering bug by hoisting the separator property to adjacent elements. It also adds public accessors for Slack credentials and a Chat.thread factory for worker reconstruction. Reviewer feedback recommends improving type safety for the Slack client property and replacing non-deterministic datetime.now() calls with fixed timestamps in both the implementation and tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_slack_api.py (1)
1011-1038: Add the missing no-auth test forcurrent_client.The new public accessor contract says
current_clientshould raiseAuthenticationErrorwhen no token is available, but onlycurrent_tokenexercises that path right now. That leaves half of the new public API able to regress unnoticed.💡 Suggested test
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 + + def test_current_client_raises_without_any_token(self): + from chat_sdk.shared.errors import AuthenticationError + + adapter = _make_adapter(bot_token=None) + with pytest.raises(AuthenticationError): + _ = adapter.current_clientAs per coding guidelines,
tests/**/*.py: "Every test must fail when the code is wrong."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_slack_api.py` around lines 1011 - 1038, Add a test that mirrors test_current_token_raises_without_any_token but for the public accessor current_client: create an adapter with _make_adapter(bot_token=None) and assert that accessing adapter.current_client raises AuthenticationError (imported from chat_sdk.shared.errors); this ensures current_client enforces the no-auth behavior like current_token and prevents regressions in the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/chat_sdk/chat.py`:
- Around line 1407-1427: The current Chat.thread() only checks the prefix of
thread_id (adapter_name) but allows empty adapter-specific portions like
"slack:" or "slack::", which later break adapter calls; update Chat.thread() to
validate that the portion after the first colon (the channel/thread identifier)
is non-empty (similar to channel()), and if it's empty raise ChatError (use the
same style as the existing ChatError messages) before looking up self._adapters
or constructing the stub_message; keep the existing adapter lookup
(self._adapters.get(adapter_name)) and return path to _create_thread unchanged
once validation passes.
In `@tests/test_chat_resolver.py`:
- Around line 431-493: Add assertions that the Thread returned by
Chat.thread(...) reuses the parent Chat's state/history; e.g., in
TestChatThreadFactory (methods like test_infers_adapter_from_thread_id_prefix
and test_propagates_explicit_current_message) assert that the returned thread
references the original chat (thread.chat is chat or thread._chat is chat) and
that the thread's history/state object is the same instance as the chat's
(thread.history is chat.history or thread._history is chat._history). Update
whichever tests create thread via Chat.thread(...) to include these identity
assertions so the factory cannot return a detached thread.
---
Nitpick comments:
In `@tests/test_slack_api.py`:
- Around line 1011-1038: Add a test that mirrors
test_current_token_raises_without_any_token but for the public accessor
current_client: create an adapter with _make_adapter(bot_token=None) and assert
that accessing adapter.current_client raises AuthenticationError (imported from
chat_sdk.shared.errors); this ensures current_client enforces the no-auth
behavior like current_token and prevents regressions in the public API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7efcc59-79a8-457f-a6fc-3c21cc53fb41
📒 Files selected for processing (8)
CHANGELOG.mddocs/UPSTREAM_SYNC.mdsrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/teams/cards.pysrc/chat_sdk/chat.pytests/test_chat_resolver.pytests/test_slack_api.pytests/test_teams_cards.py
This was referenced Apr 23, 2026
- **`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
…line 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).
- **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.
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)
Collaborator
Author
patrick-chinchill
left a comment
patrick-chinchill
left a comment
There was a problem hiding this comment.
Re-reviewed current head 3297b3b. This looks good to merge. Focused verification passed: uv run pytest tests/test_chat_resolver.py tests/test_slack_api.py tests/test_teams_cards.py -q (139 passed). I couldn't submit an approval because GitHub does not allow approving your own pull request.
3 tasks
This was referenced May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bundles the four open
chat-sdk-pythonissues into one PR. All four are small, adapter-surface hygiene items.Summary
await:AsyncWebClient.chat_stream(...)is a coroutine function. Without the await,streamerwas a coroutine and.append()raisedAttributeErroron the first chunk, so native Slack streaming failed for any consumer using the default adapter. One-line fix; existing tests updated to useAsyncMockforchat_streamso they actually mirror the real client.Containerwithseparator: Trueat zero height, so the separator line is effectively invisible. New behavior post-processes the body: a divider between siblings hoistsseparator: Trueonto the following element; a trailing divider emits a minimal non-empty Container. Internal marker never leaks into the outgoing payload. Upstream TS ships the same bug; documented as a divergence inUPSTREAM_SYNC.md.Chat.thread(thread_id, *, current_message=None): new public worker-reconstruction factory mirroring TSchat.thread(threadId). Adapter is inferred from the thread-ID prefix; state and message history come from the Chat instance.current_messageis preserved so Slack native streaming still works post-reconstruction (it readsrecipient_user_id/recipient_team_idfrom the current message).SlackAdapter.current_token/current_client: public@propertyaccessors for the request-context-bound bot token and a preconfiguredAsyncWebClient. Replaces underscore access (_get_token/_get_client) from consumer code that makes direct Slack Web API calls from inside a handler (email resolution, user profile fetches, etc.). Documented as a Python-only extension inUPSTREAM_SYNC.md.Test plan
uv run ruff check src/ tests/ scripts/— all checks passeduv run ruff format --check src/ tests/ scripts/— 191 files already formatteduv run python scripts/audit_test_quality.py— 0 hard failuresuv run python scripts/verify_test_fidelity.py— 0 missinguv run pyrefly check --baseline=.pyrefly-baseline.json— 0 errorsuv run pytest tests/ --tb=short -q— 3371 passed, 11 skipped, 1 pre-existing failure onmain(test_github_webhook.py::test_throws_when_no_auth, unrelated to this change)test_stream_awaits_chat_stream_coroutine) passes withAsyncMock(return_value=mock_streamer)— would fail without the awaitTestPublicContextAccessorsclass covers single-workspace default, ContextVar binding, mock-client pass-through, and AuthenticationError outside contextTestChatThreadFactoryclass covers adapter inference,current_messagepropagation, stub fallback, invalid-ID, and unregistered-adapter pathsCloses #44, #45, #46, #47.
https://claude.ai/code/session_01MXobPieFH93txkgm38oMpC
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests