fix(adapters): accept dict StreamChunks in slack/github/google_chat streams#97
Conversation
…treams
`_from_full_stream` (thread.py:1218-1226) explicitly forwards
`{"type": "markdown_text" | "task_update" | "plan_update", ...}` dict
chunks unchanged, so downstream adapters MUST accept both the dataclass
and dict shapes of `StreamChunk`. The Teams adapter already does
(adapter.py:927), but slack/github/google_chat only check
`hasattr(chunk, "type")` — which is False for dicts in Python — so dict
chunks were silently dropped or, worse, raised AttributeError inside
Slack's `send_structured_chunk` and downgraded the entire stream to
text-only with a misleading warning about missing assistant_view scopes.
Changes:
- slack/adapter.py: route dict markdown_text through the renderer; add
a `_read(name)` helper in `send_structured_chunk` so dict and dataclass
field access share one code path; tighten the fallback warning to
enumerate the actual possible causes (missing scope OR malformed
chunk OR transient Slack API error) instead of pointing solely at
the manifest.
- github/adapter.py: accept dict markdown_text chunks in the accumulator.
- google_chat/adapter.py: same.
- test_slack_api.py: two regression tests in TestStream covering the two
distinct adapter branches affected (renderer dispatch + structured
chunk forwarding). Both fail when the dict branches are reverted.
This brings the Python adapters in line with the SDK's own internal
contract — adapters do not enforce a dataclass-only shape for chunks
that `_from_full_stream` is documented and tested to pass through as
dicts.
|
Warning Review limit reached
More reviews will be available in 25 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR extends all three SDK adapters to accept streamed chunks as dictionaries with ChangesDict-shaped chunk support across adapters
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_slack_api.py`:
- Around line 967-1051: The failing CI is due to formatting errors in the test
file affecting the two tests test_stream_accepts_dict_markdown_text_chunks and
test_stream_accepts_dict_task_update_chunks; fix by running the project's
formatter (ruff) over the tests to apply the required style rules (e.g., run the
equivalent of `ruff format` on the test sources) and re-commit the updated
formatting so ruff format --check passes. Ensure the signatures and
async/generator yields in those tests remain unchanged while only fixing
whitespace/line-break/indentation per ruff.
🪄 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: 5006498d-029d-4bd5-8142-ec2f6bc6ba7e
📒 Files selected for processing (4)
src/chat_sdk/adapters/github/adapter.pysrc/chat_sdk/adapters/google_chat/adapter.pysrc/chat_sdk/adapters/slack/adapter.pytests/test_slack_api.py
| @pytest.mark.asyncio | ||
| async def test_stream_accepts_dict_markdown_text_chunks(self): | ||
| """Dict-shaped markdown_text chunks must flow into the renderer the | ||
| same as the dataclass form — `_from_full_stream` in thread.py | ||
| forwards them unchanged, so adapters must accept both shapes. | ||
| """ | ||
| adapter, client, _ = await _init_adapter() | ||
|
|
||
| mock_streamer = MagicMock() | ||
| mock_streamer.append = AsyncMock() | ||
| mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "666.666"}}) | ||
| client.chat_stream = AsyncMock(return_value=mock_streamer) | ||
|
|
||
| async def chunk_gen() -> AsyncIterator[Any]: | ||
| yield {"type": "markdown_text", "text": "Hello "} | ||
| yield {"type": "markdown_text", "text": "world"} | ||
|
|
||
| result = await adapter.stream( | ||
| "slack:C123:1234567890.000000", | ||
| chunk_gen(), | ||
| StreamOptions(recipient_user_id="U1", recipient_team_id="T1"), | ||
| ) | ||
|
|
||
| assert result.id == "666.666" | ||
| appended_text = "".join( | ||
| call.kwargs.get("markdown_text", "") | ||
| for call in mock_streamer.append.call_args_list | ||
| ) | ||
| assert "Hello world" in appended_text, ( | ||
| "dict markdown_text chunks were not routed through the " | ||
| f"markdown renderer; streamer.append received {appended_text!r}" | ||
| ) | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_stream_accepts_dict_task_update_chunks(self): | ||
| """Dict-shaped task_update chunks must be forwarded as structured | ||
| chunks to `streamer.append(chunks=...)`, matching the dataclass form. | ||
| """ | ||
| adapter, client, _ = await _init_adapter() | ||
|
|
||
| mock_streamer = MagicMock() | ||
| mock_streamer.append = AsyncMock() | ||
| mock_streamer.stop = AsyncMock(return_value={"message": {"ts": "555.555"}}) | ||
| client.chat_stream = AsyncMock(return_value=mock_streamer) | ||
|
|
||
| async def chunk_gen() -> AsyncIterator[Any]: | ||
| yield "Starting " | ||
| yield { | ||
| "type": "task_update", | ||
| "id": "task1", | ||
| "title": "Search", | ||
| "status": "in_progress", | ||
| } | ||
| yield { | ||
| "type": "task_update", | ||
| "id": "task1", | ||
| "title": "Search", | ||
| "status": "complete", | ||
| "output": "Found 5", | ||
| } | ||
|
|
||
| result = await adapter.stream( | ||
| "slack:C123:1234567890.000000", | ||
| chunk_gen(), | ||
| StreamOptions(recipient_user_id="U1", recipient_team_id="T1"), | ||
| ) | ||
|
|
||
| assert result.id == "555.555" | ||
| structured_calls = [ | ||
| call for call in mock_streamer.append.call_args_list | ||
| if "chunks" in call.kwargs | ||
| ] | ||
| assert structured_calls, ( | ||
| "dict-shaped task_update chunks were not forwarded as " | ||
| "structured chunks to streamer.append(chunks=...)" | ||
| ) | ||
| all_forwarded_chunks = [ | ||
| chunk | ||
| for call in structured_calls | ||
| for chunk in call.kwargs["chunks"] | ||
| ] | ||
| assert any(c.get("type") == "task_update" for c in all_forwarded_chunks), ( | ||
| "no forwarded chunk had type=task_update; got: " | ||
| f"{all_forwarded_chunks!r}" | ||
| ) |
There was a problem hiding this comment.
Fix formatting to pass CI.
The pipeline indicates ruff format --check failed for this file. Run uv run ruff format tests/ to apply the required formatting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_slack_api.py` around lines 967 - 1051, The failing CI is due to
formatting errors in the test file affecting the two tests
test_stream_accepts_dict_markdown_text_chunks and
test_stream_accepts_dict_task_update_chunks; fix by running the project's
formatter (ruff) over the tests to apply the required style rules (e.g., run the
equivalent of `ruff format` on the test sources) and re-commit the updated
formatting so ruff format --check passes. Ensure the signatures and
async/generator yields in those tests remain unchanged while only fixing
whitespace/line-break/indentation per ruff.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the GitHub, Google Chat, and Slack adapters to support stream chunks provided as dictionaries, ensuring compatibility with normalized stream data. The changes include updating the stream methods to handle dictionary-shaped chunks and refactoring the Slack adapter's send_structured_chunk function with a unified _read helper for attribute and key access. Additionally, new unit tests were added to verify the Slack adapter's handling of these dictionary chunks. Feedback was provided regarding the use of truthiness checks for text_value and chunk_type in the Slack adapter, which could lead to valid empty strings being incorrectly ignored; it is recommended to use explicit None or type checks instead.
| # `{type: "markdown_text", ...}` items unchanged, so adapters | ||
| # must handle them the same as the dataclass form. | ||
| text_value = chunk.get("text") | ||
| if isinstance(text_value, str) and text_value: |
There was a problem hiding this comment.
The truthiness check and text_value will silently ignore empty string chunks (""). While an empty string is a no-op for the markdown renderer, it is still a valid value. Per the general rules, use is not None (or rely on the isinstance(text_value, str) check which already excludes None) to maintain consistency with the dataclass branch and other adapters in this PR.
| if isinstance(text_value, str) and text_value: | |
| if isinstance(text_value, str): |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
| return getattr(chunk, name, None) | ||
|
|
||
| chunk_type = _read("type") | ||
| if not chunk_type: |
There was a problem hiding this comment.
Using a truthiness check if not chunk_type: on an optional value violates the general rule to use is not None. If chunk_type is an empty string, it will be silently ignored and the function will return early without processing the chunk.
| if not chunk_type: | |
| if chunk_type is None: |
References
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
|
Replaced by #105 (could not push the Generated by Claude Code |
…treams (#105) Aligns slack/github/google_chat stream loops with the dict-shaped StreamChunk contract that thread.py:1218 and the Teams adapter already honor. Port-correctness fix: Python's hasattr check is object-only, so dict-shaped chunks forwarded by _from_full_stream silently fell through to send_structured_chunk, raised AttributeError on the first field access, got caught by a broad except, and downgraded the rest of the stream to text-only with a misleading "check your Slack scopes" warning. - Slack: dispatch loop gets isinstance(chunk, dict) and chunk.get("type") == "markdown_text" branch; send_structured_chunk rewritten to use a _read() helper that handles both dict and dataclass shapes; fallback warning rewritten to name the actual possible causes - GitHub & Google Chat: same isinstance(chunk, dict) branch in each stream accumulator - 2 new regression tests in TestStream covering both dispatch branches Original authorship: @tony-chinchill-ai. Replaces #97.
2 participants
Summary
The SDK's
_from_full_stream(thread.py:1218-1226) is documented and implemented to forward dict-shapedStreamChunks —{"type": "markdown_text" | "task_update" | "plan_update", ...}— to downstream adapters unchanged. The Teams adapter honors that contract correctly (adapters/teams/adapter.py:927), but slack, github, and google_chat only checkhasattr(chunk, \"type\"), which is always False for plain dicts in Python.Concrete failure mode in Slack: a dict-shaped chunk would miss the dispatch loop's
hasattrcheck, fall tosend_structured_chunk, raiseAttributeError(\"'dict' object has no attribute 'type'\")on the first field access, get caught by a broad except that flipsstructured_chunks_supported = False, and log a warning that points the operator at a Slack OAuth scope issue — even though scopes have nothing to do with the failure. Every subsequent chunk in the stream is silently downgraded to text-only.This PR aligns slack/github/google_chat with the dict-handling contract that thread.py already promises.
Changes
adapters/slack/adapter.pyisinstance(chunk, dict) and chunk.get(\"type\") == \"markdown_text\"branch that routes through the markdown renderer, mirroring the existing dataclass branch.send_structured_chunk: replaced the hardcodedchunk.type/hasattr(chunk, ...)accesses with a small_read(name)helper that handles both dict and dataclass shapes through one code path. Also added an early-return + log whentypeis missing entirely, instead of letting that case fall into the fallback warning.assistant_view/assistant:writescope OR malformed chunk payload OR transient Slack API error) instead of pinning the blame on the app manifest.adapters/github/adapter.py&adapters/google_chat/adapter.pyisinstance(chunk, dict) and chunk.get(\"type\") == \"markdown_text\"branch in each adapter's stream accumulator.tests/test_slack_api.pyTestStreamthat exercise the two distinct adapter branches affected (renderer dispatch + structured chunk forwarding). Both fail when the dict branches are reverted, and their assert messages name what broke.Why this is the right shape for the fix
hasattrcheck is strictly object-only, so the Python port has to add the dict branch explicitly to preserve the same behavior.thread.py:640,thread.py:661,thread.py:1218,adapters/teams/adapter.py:927). It does not introduce a new mode or option.Test plan
uv run pytest tests/test_slack_api.py::TestStream— 8/8 pass (6 existing + 2 new).uv run pytest— full SDK suite: 3584 passed, 11 skipped, no regressions.uv run ruff check src/ tests/— clean.🤖 Generated with Claude Code
Summary by CodeRabbit