feat(teams): native streaming for DMs via emit (vercel/chat#416)#88
Conversation
Port upstream `feat(adapter-teams): native streaming for DMs via emit` (commit ed46bae). DM threads now dispatch chunks through the Bot Framework streaming protocol (typing activities with `channelData.streamType=streaming` + `streamSequence`, then a final `message` activity with `streamType=final`) instead of routing through `Thread._fallback_stream`. Group chats / channels accumulate the stream and post a single message — matching upstream's post-#416 behavior of avoiding the post+edit flicker where Teams doesn't support native streaming. Lifecycle: - `_handle_message_activity` registers a `_TeamsStreamSession` for DMs and `await`s the chat handler so the session stays alive through `stream()`. The handler is fire-and-forget for non-DM threads. - `_stream_via_emit` emits cumulative text per chunk; `streamId` is captured from the first send and threaded through subsequent chunks (Hazard #7 — the first chunk omits the key entirely). - `_close_stream_session` posts the final activity. Skipped on cancellation or zero-chunk streams (no orphan finalizers). - Iterator exceptions cancel the session and re-raise; transient send failures (e.g. 429 mid-stream) cancel the session and return the partial RawMessage instead of bubbling up. Tests: `tests/test_teams_native_streaming.py` covers wire-format invariants, dispatch decisions, cancellation paths, error mid-stream, very-short streams, two concurrent DMs, and the end-to-end webhook → process_message → stream → close lifecycle. Existing `TestStream` cases in `test_teams_coverage.py` and `test_teams_extended.py` updated to reflect the new accumulate-and-post semantics for group chats. Non-parity: updated existing "Fallback streaming" rows in `docs/UPSTREAM_SYNC.md` to scope the divergence to non-Teams adapters, and added two new rows documenting Teams-specific divergences (group chats now accumulate-and-post; native streaming soft-cancels on send failure). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native Bot Framework streaming for Teams DMs with per-thread ChangesTeams Native Streaming Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Adapter as TeamsAdapter
participant Bot as BotFramework
participant Session as _TeamsStreamSession
Client->>Adapter: stream() / emits chunks
Adapter->>Bot: send typing activity (first chunk without streamId)
Bot-->>Adapter: accepted (id)
Adapter->>Session: record streamId & cumulative text
Adapter->>Bot: send subsequent typing (include streamId, increment streamSequence)
Bot-->>Adapter: accepted
Adapter->>Adapter: _close_stream_session -> send final message (streamType="final")
Bot-->>Adapter: final accepted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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 |
There was a problem hiding this comment.
Code Review
This pull request implements native streaming for Microsoft Teams DMs using the Bot Framework protocol, while transitioning group chats and channels to an accumulate-and-post strategy to eliminate UI flicker. It introduces a session management system for tracking in-flight DM streams and includes comprehensive test coverage for the new streaming lifecycle. Feedback suggests simplifying the stream finalization logic by removing a redundant check for empty content, as the presence of a stream ID already implies that content was sent.
| """ | ||
| if session.canceled: | ||
| return | ||
| if session.stream_id is None or not session._text: # noqa: SLF001 |
There was a problem hiding this comment.
The check not session._text is redundant here. session.stream_id is only set when a chunk with content is sent, which also guarantees session._text will be non-empty. Therefore, checking for session.stream_id is None is sufficient.
| if session.stream_id is None or not session._text: # noqa: SLF001 | |
| if session.stream_id is None: |
Address gemini-code-assist review on PR #88 (line 1218). ``session.stream_id`` is only assigned after a successful chunk send, and empty chunks are skipped before the send call — so ``stream_id is not None`` implies ``_text`` is non-empty. Drop the redundant ``not session._text`` check; the single condition expresses the invariant more clearly. Behavior unchanged. Tests still pass. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
patrick-chinchill
left a comment
There was a problem hiding this comment.
Code review — feat(teams): native streaming for DMs via emit (vercel/chat#416)
Compared HEAD (58b7fc7, including the follow-up _text-check refactor) to upstream ed46bae at f55378a (packages/adapter-teams/src/index.ts). Walked the porting hazards (especially #5/#7) and the SELF_REVIEW adversarial checks (pass-interaction, emit/parse symmetry, sentinels).
✅ Looks good
- Hazard #7 (omit vs
None) is honored: the first chunk'schannelDatais constructed without astreamIdkey (if session.stream_id is not None: channel_data["streamId"] = ...), andtest_first_chunk_omits_stream_idpins the wire shape withassert "streamId" not in first_payload["channelData"]. streamSequencestarts at 1 and increments per emitted chunk (session.sequence += 1before building the payload, after the empty-skip), matching the Bot Framework protocol and pinned bytest_subsequent_chunks_carry_stream_id_and_increment_sequence.- DM detection matches upstream byte-for-byte — both Python
is_dmand TSisDMusenot conversationId.startsWith("19:")(notconversationType == "personal"as the brief implied). Group-chat IDs (19:...@thread.skype) correctly fall through to accumulate-and-post. - Invariant in the follow-up refactor holds:
stream_idis only set after a successful_teams_sendAND only whenresult.get("id")is non-empty (line 1174–1178); empty chunks are skipped beforeaccumulated += text. Sostream_id is not None ⇒ _text non-empty. Thenot session._textcheck was indeed redundant. - Cleanup-path safety: the finally in
_handle_message_activityusesif current is sessionbefore popping (so a concurrent re-registration on the same DM thread isn't clobbered), and_close_stream_sessionis wrapped in try/except so its failure can't leak the registry entry. Cancel/iterator-exception paths both callsession.cancel()before re-raising, so_close_stream_sessioncorrectly skips the final activity. - Soft-cancel-on-send-failure divergence is documented in
docs/UPSTREAM_SYNC.mdnon-parity table with rationale and tested bytest_emit_send_failure_cancels_session(verifies no exception bubbles,result.raw["text"]carries pre-failure content). - Group-chat accumulate-and-post is a parity behavior with upstream post-#416, not a divergence — the doc table row marks it "no divergence at the adapter level" for clarity, which is the right framing.
🟡 Medium
- Concurrent-DM test doesn't exercise
_active_streamsrace —test_two_concurrent_dm_streams_have_independent_sessionsconstructs two sessions for two distinct thread IDs and calls_stream_via_emitdirectly with explicit session args, bypassing_active_streamsentirely. The realistic race (two near-simultaneous webhooks for the same DM thread overwriting_active_streams[tid]) isn't covered. This race exists in upstream too (activeStreams.set(threadId, ctx.stream)overwrites identically), so it's parity behavior — but the test name oversells what's being verified. Suggest renaming or adding a same-thread test that drives two_handle_message_activitycalls underasyncio.gather. - First-chunk send returns
id="": if Teams accepts the typing activity but returns an empty id,session.stream_idstaysNone(line 1177 guard),_textbecomes non-empty, and_close_stream_sessionskips because of the (now-tightened)stream_id is Nonecheck. Net effect: user sees streamed chunks but no finalmessageactivity — the streaming UI stays running until Teams times it out client-side. Upstream has the same shape (messageId = "") but would still attempt the final send. Worth a one-line comment at the close-skip site explaining this, and ideally a test pinning the empty-idbehavior.
🔵 Nit
_chained_wait_untilinvokesupstream_wait_until(task)before_resolve_processing(task). If the caller-suppliedwait_untilraises, the exception propagates back throughprocess_message, hits the adapter's outer try, and falls intofinally(no hang —await processing_doneisn't reached). That's fine, but reversing the order — resolve first, then call upstream — would make the deadlock-immunity argument trivially obvious to a future reader._TeamsStreamSession._textis read from outside the class viasession._textwith# noqa: SLF001— a publictextattribute (or method) would be cleaner since the leading underscore isn't really enforcing privacy.
Posted by an automated reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
- Add a same-thread concurrent _handle_message_activity test that exercises the realistic _active_streams race (two near-simultaneous DM webhooks for the same thread). Pins upstream's plain-Map clobber semantics: the second registration overwrites the first, both in-flight handlers observe the later session, and the registry ends empty after both finish. The original distinct-threads test is renamed to make clear it covers session ISOLATION, not the registry race. - Empty-id final-send fallback: when Teams accepts streaming chunks but returns id="" on the first activity, _close_stream_session now ships the final message anyway (omitting streamId from channelData) instead of skipping and leaving the streaming UI spinning until Teams times the session out client-side. Mirrors upstream's looser check (text non-empty → ship the final). Adds a regression test and a non-parity row in docs/UPSTREAM_SYNC.md. - _chained_wait_until: resolve our internal processing_done gate BEFORE invoking the caller-supplied waitUntil, so the deadlock-immunity argument is trivially obvious (a misbehaving upstream callback can't starve the await on processing_done). - _TeamsStreamSession: add a public read-only `text` property so external callers (now _close_stream_session) read through it instead of the underscore-prefixed _text attribute. _stream_via_emit retains the direct _text write as the canonical mutator.
patrick-chinchill
left a comment
There was a problem hiding this comment.
Re-review of fixes in 2e96fbb
Verified the four fixes against ed46bae (vercel/chat#416) and the broader adapter surface.
Verified — fixes land cleanly
test_same_thread_concurrent_handlers_clobber_active_stream(tests/test_teams_native_streaming.py:722) — drives two_handle_message_activitytasks for the same thread under a barrier, snapshots the registry post-overlap, and pins[second_session, second_session]. Matches upstream'sMapclobber semantics. The renamedtest_distinct_dm_threads_each_have_isolated_session_statemakes the isolation-vs-race split explicit.- Empty-
idfinal-send (adapter.py:1239–1271, test 317–362) —_close_stream_sessionnow ships the finalmessagewhentextis non-empty even ifstream_id is None, omittingstreamIdfromchannelData(rather than serializingNone— Hazard #7). Regression test asserts"streamId" not in final_payload["channelData"]. Non-parity row at UPSTREAM_SYNC.md:492 is accurate. _chained_wait_untilorder (adapter.py:462–471) —_resolve_processing(task)runs beforeupstream_wait_until(task), with a comment explaining the deadlock-immunity argument.- Public
textproperty (adapter.py:95–103, 1243, 1260) — read-onlytextaccessor exposed; reads in_close_stream_sessiongo through it; only the canonical mutator at adapter.py:1216 retains# noqa: SLF001. Tests still poke_textdirectly (lines 176, 300) — fine, they're inside the test module's own laxer scope and lint-clean.
Findings
Nit (parity gap, pre-existing across multiple adapters)
_stream_via_emit and the accumulate-and-post fallback both check isinstance(chunk, dict) and chunk.get("type") == "markdown_text" (adapter.py:1081, 1139). A real MarkdownTextChunk dataclass instance silently drops to "" because isinstance(MarkdownTextChunk(...), dict) is False. Other adapters (google_chat, whatsapp, slack, github) use hasattr(chunk, "type") for this. Same gap exists in discord/linear, so out of scope here, but worth a follow-up issue — the StreamChunk union in types.py:798 is dataclass-typed.
Nit (divergence not in UPSTREAM_SYNC.md)
_chained_wait_until resolves the internal gate before invoking the caller-supplied wait_until. Upstream TS does the opposite (baseOptions?.waitUntil?.(task) first, then task.then(...)). Functionally equivalent (both register callbacks; neither blocks), but it IS a deliberate ordering divergence and the commit message frames it as a hardening defense — consider a one-line entry in the non-parity table so future syncs don't "correct" it back.
Nit (forward-looking)
chained_options = WebhookOptions(wait_until=_chained_wait_until) (adapter.py:473) drops any other fields that may exist on the inbound options. Today WebhookOptions only has wait_until, so no observable bug — but upstream uses {...baseOptions, waitUntil: ...} for a reason. A replace(options, wait_until=...) (or explicit comment "WebhookOptions has only wait_until — re-spread if it grows") would future-proof.
Hunt results — no other issues
- Wire format:
streamSequence(1-indexed, ++ per emit),streamType("streaming"/"final"),streamId(omitted on first chunk + when server returned empty id), parallelstreaminfoentity — all match the Bot Framework streaming protocol that upstream'sIStreameremits under the hood. - Cleanup:
try/finallyin_handle_message_activitymirrors upstream'stry { await processingDone } finally { activeStreams.delete };asyncio.CancelledErrorcancels the session and re-raises (covered bytest_cancelled_error_propagates_and_marks_session_canceled). - DM detection:
is_dmchecks onlynot conversationId.startswith("19:")— matchesthread-id.ts:30exactly. - Test fidelity: upstream PR #416 added zero new
it()blocks inindex.test.ts; only assertion swaps inreplay-streaming.test.ts. Python's 23 native-streaming tests + 159 teams-streaming-related tests all pass; no missing TS coverage. - Hazard #5: no untracked tasks in the new code.
process_messagetracks via_active_tasks,_chained_wait_untilonly callsadd_done_callback, no barecreate_task/ensure_future.
Re-review verdict: PASS
The three nits are real but non-blocking (one is pre-existing, one is hardening with no observable behavior change, one is forward-looking). Fixes for the previous round all landed correctly.
Posted by an automated re-reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
Final upstream-coverage audit before merging the 7 sync PRs (#84-#90) identified one undocumented N/A item: vercel/chat#415 (Teams SDK 2.0.8 + User-Agent) is a JS-only botbuilder dependency bump. The Python Teams adapter uses raw aiohttp (no botbuilder), so there is no equivalent dependency to bump. The optional User-Agent: Vercel.ChatSDK header on the ~9 outbound aiohttp call sites is a defense-in-depth nice-to-have; deferred as a follow-up rather than landed in this sync. Updates: - CHANGELOG.md: tick all completed items and link them to their PRs (#84, #85, #86, #87, #88, #89, #90, plus already-merged PR #74). Document #415 inline as N/A. - docs/UPSTREAM_SYNC.md non-parity table: add row for Teams User-Agent header divergence so future syncers don't try to "port" the JS bump. Item #6 (concurrency.maxConcurrent) is already implementation-covered in the Python port (existing divergence row at L492). The 4 new TS concurrency tests in chat.test.ts have Python-specific equivalents at test_chat_faithful.py L2969-3055 that don't name-match — leaving as deferred fidelity-baseline polish since the behavior is verified. Verdict from the coverage audit: all 18 substantive ports across PRs #84-#90 are upstream-verified. No commits in chat@4.26.0..f55378a were missed. Ready to start merging. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e96fbb242
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address Codex P2 finding on PR #88. The previous flow updated ``accumulated`` and ``session.sequence`` BEFORE awaiting ``_teams_send``, so a 429/network failure mid-stream would leave the partial RawMessage returned to the caller (and ``session._text`` feeding the final close- activity) carrying text that Teams never displayed to the user. Fix: - Build ``candidate_accumulated = accumulated + text`` and ``next_sequence = session.sequence + 1`` as candidates. - Send the activity with the candidate values. - Commit ``accumulated`` and ``session.sequence`` only after the send succeeds. The soft-cancel path (logged warn + session.cancel + break) now genuinely returns the high-water mark of successfully-sent text. Matches the documented "RawMessage carries pre-failure content" guarantee in ``docs/UPSTREAM_SYNC.md`` non-parity table. Updated test_emit_send_failure_cancels_session: previously asserted ``result.raw["text"] == "helloworld"`` (which contained the rejected "world" chunk); now asserts ``"hello"`` and ``session.sequence == 1``. Skipped Codex P2 #2 (active_streams clobber timing) — matches upstream TS behavior; deferring fix as a Python-only improvement would be a divergence and is design-significant. File as follow-up if desired. 23 native-streaming tests pass. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07df4b778b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/chat_sdk/adapters/teams/adapter.py`:
- Around line 437-493: _active_streams is keyed only by thread_id causing
concurrent handlers on the same thread to overwrite each other's
_TeamsStreamSession; instead of registering the session in the shared dict and
re-resolving by thread_id, keep the session in handler-local context (e.g. a
ContextVar) or pass the session explicitly into stream-related calls so each
webhook handler uses its own _TeamsStreamSession instance; update code paths
that access _active_streams (creation in this block, stream() consumer, and
cleanup via _close_stream_session) to use the handler-local session, ensure
_chained_wait_until / processing_done still wire to that session, and remove or
restrict shared lookups by thread_id to avoid stale/overwritten sessions.
- Around line 442-478: The webhook can deadlock if Chat.process_message never
creates a task (so _chained_wait_until is never called) because processing_done
remains unresolved; after the call to self._chat.process_message(self,
thread_id, message, chained_options) ensure processing_done is always unblocked
by adding a finally block that (1) catches asyncio.CancelledError around the
await processing_done to call session.cancel() and re-raise, and (2) after the
try/except/cleanup, checks if processing_done.done() is False and calls
processing_done.set_result(None) to guarantee resolution; reference
_resolve_processing, _chained_wait_until, WebhookOptions.wait_until,
processing_done, and self._chat.process_message when applying this change.
🪄 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: c107c5b6-a50f-47f3-8e14-0ee87262958c
📒 Files selected for processing (5)
docs/UPSTREAM_SYNC.mdsrc/chat_sdk/adapters/teams/adapter.pytests/test_teams_coverage.pytests/test_teams_extended.pytests/test_teams_native_streaming.py
Address CodeRabbit Critical on PR #88. ``Chat.process_message`` invokes ``wait_until`` synchronously only if a chat task is actually scheduled. If the message is deduped, dropped by the concurrency strategy, or otherwise short-circuited, no task is created and ``wait_until`` is never called — leaving ``processing_done`` unresolved and ``await processing_done`` hanging the webhook indefinitely. After ``process_message`` returns, check whether ``processing_done`` was resolved (which can only happen via ``_chained_wait_until``, which is only invoked when a task is scheduled). If not, resolve immediately — there's no in-flight handler to wait on, no streaming will happen, and ``await processing_done`` should fall through to the ``finally`` cleanup. Also addresses Codex P2 (lines 471): caller-supplied ``WebhookOptions.wait_until`` raising synchronously now logs and continues instead of escaping through ``Chat.process_message``, preventing the same-class deadlock-via-finally issue. Both fixes together close the two paths where the streaming session could be torn down while the chat task was still scheduled (or never scheduled): a synchronous wait_until exception, and a deduped/dropped message. Tests: - New ``test_caller_wait_until_raise_does_not_kill_native_streaming`` verifies the wait_until-raise path keeps the session alive long enough for the chat task to call ``thread.stream()`` natively. - Existing 23 native-streaming tests still pass. Skipped CodeRabbit's other comment (``_active_streams`` thread-id keying) — same as Codex P2 #2, matches upstream TS behavior and would be a heavy-lift Python-only divergence. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
…messages The previous fix (17009a9) used ``processing_done.done()`` to detect whether a chat task was scheduled, but ``_resolve_processing`` registers a ``done_callback`` on the task that fires only on task COMPLETION — not at the moment ``wait_until`` is invoked. So my check prematurely resolved ``processing_done`` immediately after ``process_message`` returned (before the task had a chance to run), which broke 2 tests: - test_caller_wait_until_raise_does_not_kill_native_streaming - test_same_thread_concurrent_handlers_clobber_active_stream Fix: track a separate ``wait_until_invoked`` flag inside ``_chained_wait_until``. After ``process_message`` returns, only resolve ``processing_done`` if ``wait_until`` was NEVER called — that's the true signal that no chat task was scheduled (deduped/dropped). 23 native-streaming tests pass; full suite passes (modulo the pre-existing unrelated github_webhook failure). https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Three separate review bots flagged the thread-id keying of ``_active_streams`` as a potential cross-handler corruption risk on the DM streaming path. Add an inline comment at the registration site pointing readers at the upstream parallel and the concurrency invariant that makes per-thread keying safe today, plus a note that a per-handler ContextVar is a tracked Python-only follow-up rather than in-scope for the parity sync.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6acc796ae3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…y matches user-visible text The previous soft-cancel behaviour in ``_stream_via_emit`` kept the adapter's internal ``accumulated`` buffer in sync with what Teams had actually accepted, but ``Thread.stream`` wraps the iterator in its own generator that updates an *outer* ``accumulated`` BEFORE yielding each chunk to the adapter. Because the soft-cancel path returned a partial ``RawMessage`` without raising, that outer buffer (which already includes the rejected chunk) was used as the SentMessage body and appended to ``_message_history`` — so the recorded message and the text the user actually saw could disagree on a 429 / network failure mid-stream. Propagate the send exception instead. ``_wrapped_stream`` re-raises through ``Thread.stream``, which short-circuits the post-stream ``_create_sent_message`` + history append so the SDK's record matches the user's view of the conversation. The session is still cancelled first so ``_close_stream_session`` doesn't post a final activity, and the failure is still logged at ``warn``. Re-aligns the Teams emit-failure path with how upstream chat-teams surfaces send errors, removing the Python-only soft-cancel divergence introduced in 07df4b7. Test updated to match: ``test_emit_send_failure_propagates_and_cancels_session`` now uses ``pytest.raises`` and drops the partial-RawMessage assertion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_teams_native_streaming.py`:
- Around line 694-727: The test
test_handler_exception_still_drops_session_and_closes only asserts the session
is removed and never verifies that _close_stream_session (or the final "close"
activity sent via adapter._teams_send) is invoked; update the test to actually
stream at least one chunk before the handler raises and then assert that the
close behavior occurred (e.g., inspect adapter._teams_send calls for a final
close activity or assert a call to _close_stream_session) while still keeping
the check that tid is removed from adapter._active_streams after
adapter._handle_message_activity finishes.
- Around line 607-658: The DM lifecycle block beginning at the comment needs to
be split into its own async pytest test so it doesn't run inside
test_caller_wait_until_raise_does_not_kill_native_streaming; extract the whole
scenario (adapter setup, _dm_thread_id usage, chat/process_message mock,
activity creation, await adapter._handle_message_activity, and assertions) into
a new function decorated with `@pytest.mark.asyncio` and named e.g.
test_dm_session_register_and_finalize (or similar), ensuring you copy the
adapter/_chat setup and AsyncMocking of _teams_send exactly so the new test is
independent and verifies that the session is removed and final stream message is
sent.
🪄 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: 6f529b35-d50e-4f29-ac4e-834638e406ca
📒 Files selected for processing (2)
src/chat_sdk/adapters/teams/adapter.pytests/test_teams_native_streaming.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/chat_sdk/adapters/teams/adapter.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c95dbde8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| result = await self._teams_send(decoded, activity_payload) |
There was a problem hiding this comment.
Throttle native Teams emits to Teams' quota
For normal LLM/token streams that yield more than once per second, this loop sends one Bot Framework streaming request per chunk. Microsoft’s Teams REST streaming docs state the streaming throttle is 1 request/second and recommend buffering tokens for about 1.5–2 seconds, so native DM streams will quickly hit 429 and cancel mid-response unless chunks are coalesced/throttled before _teams_send (e.g. honoring StreamOptions.update_interval_ms). See https://learn.microsoft.com/microsoftteams/platform/bots/streaming-ux#switch-to-streaming-response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit b29efe0. The emit loop now gates per-chunk sends on a configurable throttle window:
- New
TeamsAdapterConfig.native_stream_min_emit_interval_ms(default 1500ms per Microsoft's 1.5-2s recommendation) - Honors
StreamOptions.update_interval_mswhen the caller passes one (e.g. from aStreamingPlan) - Chunks arriving within the window are accumulated locally and shipped together on the next eligible emit
- Text is never lost:
session.textaccumulates every chunk regardless of coalescing, and the finalmessageactivity ships the full cumulative text — so even if the iterator ends inside a throttle window, the user sees the complete response
Tests in TestNativeStreamingThrottle pin the behavior with a deterministic injectable clock (no real sleeps):
test_chunks_within_throttle_interval_are_coalesced— fast LLM streams collapse to one emittest_chunks_beyond_throttle_interval_emit_individually— slow streams still emit per chunktest_caller_update_interval_ms_overrides_default—StreamOptionsoverride workstest_coalesced_text_ships_in_final_close_activity— regression test that buffered chunks survive in the close path
Long-term: this hand-rolled throttle is documented as transitional in UPSTREAM_SYNC.md. microsoft-teams-apps (Python SDK, GA 2026-05-01) provides IStreamer.emit() which handles throttling internally; migration is planned for 0.4.28 / future Python-only.
Generated by Claude Code
…ception coverage CodeRabbit caught two real defects in the test additions from this PR: 1. ``test_caller_wait_until_raise_does_not_kill_native_streaming`` contained an orphan docstring followed by a complete second DM lifecycle scenario (adapter setup → process_message → assertions on final activity). The missing function header meant pytest executed the second scenario as a tail-end of the first test, so the register/finalize path only ran when the wait-until-raise scenario passed, and any failure attributed to the wrong test name. Split the second body into its own ``test_dm_message_activity_registers_session_and_finalizes`` so each scenario fails independently. 2. ``test_handler_exception_still_drops_session_and_closes`` only ever asserted ``tid not in adapter._active_streams`` — the "and_closes" part of the name was unverified because the handler raised before any chunk was streamed, leaving ``session.text`` empty and ``_close_stream_session`` early-returning. Stream one chunk before the raise and assert the final ``message`` activity actually shipped with ``streamType: final`` and the user-visible text. Renamed to ``test_handler_exception_after_partial_stream_drops_session_and_closes`` so the name matches what is now actually exercised.
… to 1 req/sec Two P1 wire-format defects in the hand-rolled native Teams DM streaming, both surfaced by Codex review on PR #88: 1. **streamId missing from streaminfo entity.** The Bot Framework REST streaming contract requires ``streamId`` on BOTH ``channelData`` and the ``streaminfo`` entity for subsequent and final activities. We only set it on ``channelData``, which Teams may treat as a malformed continuation and detach from the original stream. Add the field to the entity in ``_stream_via_emit`` (subsequent chunks) and ``_close_stream_session`` (final). First chunk still omits it everywhere — the server has not yet assigned an id. 2. **No emit throttle.** Teams' streaming endpoint enforces ~1 request/second; Microsoft recommends 1.5-2s buffering. The previous loop shipped one ``typing`` activity per source chunk, so any real-world LLM token stream (10+ tokens/s) would 429 within the first second and cancel the response mid-flight. Add a throttle gate keyed on a monotonic ms-clock: chunks within ``native_stream_min_emit_interval_ms`` of the previous emit are accumulated locally and shipped together on the next eligible emit (or in the final close activity if the iterator ends inside the window). Default 1500ms; overridable per-stream via ``StreamOptions.update_interval_ms`` and per-adapter via the new ``TeamsAdapterConfig.native_stream_min_emit_interval_ms`` field. Text is never lost — ``session.text`` accumulates every chunk and feeds the final ``message`` activity regardless of coalescing. Adds 6 tests across two new classes: - ``TestNativeStreamingThrottle`` — within-window coalescing, across-window per-chunk emit, ``StreamOptions.update_interval_ms`` override, and "coalesced chunks survive in the final close activity" regression. - ``TestStreamInfoEntityContract`` — entity-level ``streamId`` on subsequent chunks and on the final ``message`` activity. Strengthens ``test_first_chunk_omits_stream_id`` and ``test_close_session_sends_final_message`` to also assert entity-level ``streamId`` presence/absence. Test clock: tests now use ``_advancing_clock(step_ms)`` to drive the throttle deterministically without real sleeps. The shared ``_make_adapter`` helper defaults to ``step_ms=2000`` (> 1500ms throttle) so existing per-chunk-emit assertions still hold; throttle tests pass ``clock_step_ms=0`` to land all chunks in a single window. ``UPSTREAM_SYNC.md`` row 491 is rewritten: the prior soft-cancel-on-send-failure entry was replaced by re-raise in 86c95db, and the row now documents both the hand-rolled wire format and the tracked migration to ``microsoft-teams-apps`` (Python SDK, GA 2026-05-01). A new row documents the entity-level ``streamId`` contract for clarity.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c18154740c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P1 on commit c181547: the end-of-stream flush in ``_stream_via_emit`` shipped buffered text as a ``typing`` activity immediately when the iterator finished, without honoring the throttle window relative to the last successful emit. For a fast LLM stream that finishes inside ``native_stream_min_emit_interval_ms`` (default 1500ms) after an earlier emit, the flush would 429 the Bot Framework streaming endpoint (~1 req/sec quota) and cancel the stream mid-flight — exactly the failure the throttle was added to prevent. Fix: compute ``elapsed_ms = clock_now - last_emit_at_ms`` and, if it's less than ``emit_interval_ms``, await the remaining window before emitting the flush. Cancellation during the wait is honored — caller may ``session.cancel()`` while we sleep, in which case we skip the flush emit and return the partial RawMessage (the cumulative text is still recorded for the caller's history, matching the in-loop cancellation shape). Injects a new ``_stream_sleep_ms`` callable on the adapter that defaults to ``asyncio.sleep(ms / 1000.0)`` and is overridden to an AsyncMock in ``_make_adapter`` so existing tests don't actually wait the configured interval. 3 new regression tests in ``TestFlushThrottle``: - ``test_flush_waits_for_throttle_window_when_iterator_ends_in_window``: pins that with ``clock_step_ms=0`` (every chunk inside the window), the flush awaits ``_stream_sleep_ms`` exactly once for the full 1500ms. - ``test_flush_does_not_wait_when_window_already_elapsed``: pins that when all chunks emitted in-loop (no buffered text), the flush block is skipped entirely, so no sleep call. - ``test_flush_skips_emit_if_session_canceled_during_wait``: pins the cancellation-during-wait path — sleep side-effect cancels the session, flush bails without emitting, but RawMessage still carries the cumulative text. Tests: 36 pass in ``test_teams_native_streaming.py`` (was 33, +3 new), 217 in the full Teams test sweep. Lint + format clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbf3461225
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session._text = accumulated # noqa: SLF001 | ||
| return RawMessage( | ||
| id=session.first_chunk_id, | ||
| thread_id=thread_id, | ||
| raw={"text": accumulated}, |
There was a problem hiding this comment.
Return only emitted text when flush is canceled
When a cancellation arrives during the throttle wait after some chunks were coalesced, this branch skips the flush and the close path also skips the final message because session.canceled is true, so Teams has only seen last_committed_text. Returning accumulated here lets Thread.stream record buffered text that was deliberately not emitted; in this scenario return the last committed text or propagate cancellation instead of committing the unsent suffix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — and the same defect existed in the in-loop cancellation path too (the if session.canceled: break at the top of the loop falls through to the same bottom return, so a coalesced chunk that buffered before cancellation would also be returned as RawMessage.raw["text"]).
Fixed in commit 52dc032 with a unified return-text selection at the bottom of _stream_via_emit:
final_text = last_committed_text if session.canceled else accumulated
session._text = final_text
return RawMessage(..., raw={"text": final_text})Happy-path streams (no cancellation) collapse to accumulated because the flush above commits the final batch, setting last_committed_text == accumulated. Canceled paths surface only what Teams actually accepted, regardless of whether cancellation arrived in-loop or during the flush throttle wait.
Also removed the early-return I'd added in the during-wait branch — the flush just skips the emit on canceled, falls through to the unified return, and the bottom does the right thing.
Tests updated:
test_flush_skips_emit_if_session_canceled_during_waitnow assertsresult.raw["text"] == "Hello"(was "Hello world") to pin Codex's contract for the during-wait variant.- New
test_in_loop_cancellation_returns_only_committed_textpins the in-loop variant: yields a chunk, coalesces a second, cancels, then breaks on the next iter's cancel-check; the return must carry only the first emit's text.
Tests: 37 streaming (was 36, +1 new), 218 in the full Teams sweep. Lint + format clean.
Generated by Claude Code
…ight Codex P2 on commit cbf3461: when cancellation arrived during the end-of-stream flush's throttle wait, the early-return surfaced ``accumulated`` (including the locally-buffered suffix the user explicitly canceled out of) as ``RawMessage.raw["text"]`` and ``session._text``. ``Thread.stream`` builds the ``SentMessage`` from that value, so the SDK's recorded history would carry text Teams never shipped. The same defect existed in the in-loop cancellation path: a coalesced chunk could leave ``accumulated > last_committed_text``, then ``if session.canceled: break`` would exit the loop without flushing, fall through to ``session._text = accumulated``, and return the buffered suffix. Unified fix at the bottom return block: final_text = last_committed_text if session.canceled else accumulated Now both cancellation paths surface only what Teams actually accepted. Happy-path streams (no cancellation) collapse to ``accumulated`` because the flush above commits the final batch, setting ``last_committed_text == accumulated``. Also removed the early-return from the during-wait cancellation branch — the flush block just skips the emit on canceled, falls through to the unified return, and the bottom does the right thing. Updated ``test_flush_skips_emit_if_session_canceled_during_wait`` to assert ``result.raw["text"] == "Hello"`` instead of "Hello world" (captures Codex's contract). Added new test ``test_in_loop_cancellation_returns_only_committed_text`` covering the in-loop cancellation variant — yields a chunk, coalesces a second, cancels, then breaks on the cancel-check; the return must carry only the first chunk. Tests: 37 streaming (was 36, +1 new), 218 in the full Teams sweep. Lint + format clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52dc032cef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # ran to completion, ``last_committed_text`` and ``accumulated`` | ||
| # are equal (the flush above committed the final batch), so this | ||
| # collapses to ``accumulated`` in the happy path. | ||
| final_text = last_committed_text if session.canceled else accumulated |
There was a problem hiding this comment.
Trim Thread.stream history on canceled native streams
When cancellation happens after a coalesced chunk was yielded, setting final_text to last_committed_text here does not keep normal Thread.stream callers consistent: Thread._handle_stream accumulates each chunk before yielding it to the adapter and then creates the SentMessage from its own accumulated buffer instead of raw_result.raw (src/chat_sdk/thread.py:635-650). Fresh evidence for re-raising this issue: those Thread lines are unchanged, so this new RawMessage text is ignored and a canceled buffered suffix can still be recorded even though Teams never received it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — my prior fix only corrected the adapter-level state. The actual SentMessage body comes from Thread._handle_stream's local accumulated buffer in _wrapped_stream (src/chat_sdk/thread.py:633-654), not from the RawMessage returned by the adapter. Without plumbing the corrected text through, the cancellation defect persisted at the SDK boundary.
Fixed in commit 7809b7e with a two-layer change:
-
RawMessagegains an optionaltext: str | None = Nonefield — adapter-authoritative override for the recorded text. Adapters whose internal state (cancellation, throttling, partial commits) makes their local-accumulator view diverge from what the platform actually accepted set this; adapters that don't need it leave the defaultNoneandThread.streamfalls back to its local accumulator. Backward-compatible by construction. -
Thread._handle_stream(native-streaming branch) prefersraw_result.textwhen not None, else uses the local accumulator:
recorded_text = raw_result.text if raw_result.text is not None else accumulated
sent = self._create_sent_message(
raw_result.id,
PostableMarkdown(markdown=recorded_text),
raw_result.thread_id,
)- Teams
_stream_via_emitsetstext=final_texton every returnedRawMessage(wherefinal_text = last_committed_text if session.canceled else accumulated). Happy path: override == accumulated, no behavior change. Cancellation paths: override carries only what Teams shipped,Thread.streamrecords that, the SentMessage and message history match the user's view.
Tests added (4 net new):
- Teams:
test_canceled_stream_sets_raw_message_text_override— adapter populates the override on cancellation - Teams:
test_happy_path_stream_also_sets_raw_message_text— symmetry, adapter always sets it - Thread:
test_should_prefer_raw_message_text_override_over_local_accumulator— Thread.stream uses the override when set - Thread:
test_should_fall_back_to_local_accumulator_when_text_override_is_none— backward compatibility for adapters that don't set it
Tests: 39 streaming, 199 in the streaming + thread file pair, 3,709 in the full suite. 1 pre-existing GitHub-adapter failure unrelated. Lint + format clean.
Generated by Claude Code
…orded SentMessage Codex P2 follow-up on commit 52dc032: the prior fix corrected ``RawMessage.raw["text"]`` and ``session._text`` at the adapter boundary, but ``Thread._handle_stream`` builds the recorded ``SentMessage`` body from its own local ``accumulated`` buffer (in ``_wrapped_stream``, ``src/chat_sdk/thread.py:633-654``), not from the adapter's return value. So a canceled native stream with buffered text would still record the buffered suffix in ``SentMessage.text`` and the ``_message_history.append``, even though Teams never shipped it. Fix in two layers: 1. ``RawMessage`` gains an optional ``text: str | None = None`` field — an adapter-authoritative override for the recorded text. Adapters whose internal state (cancellation, throttling, partial commits) makes their local-accumulator view diverge from what the platform actually accepted set this; adapters that don't need it leave the default ``None`` and ``Thread.stream`` falls back to its local accumulator. 2. ``Thread._handle_stream`` (the native-streaming branch) prefers ``raw_result.text`` when not None, else uses the local accumulator. Backward-compatible: Slack, Discord, GitHub, Linear, Google Chat, Telegram, WhatsApp don't set the override, so their behavior is unchanged. 3. Teams ``_stream_via_emit`` sets ``text=final_text`` on the returned ``RawMessage``, where ``final_text`` is ``last_committed_text`` when canceled and ``accumulated`` otherwise. So: - Happy path: override == accumulated, no behavior change. - Cancellation (in-loop break or during-wait): override carries only what Teams shipped, ``Thread.stream`` records that, the SentMessage and message history match the user's view. Tests added (4 net new): - ``test_canceled_stream_sets_raw_message_text_override`` (Teams) — pins that the adapter populates ``RawMessage.text`` on cancellation. - ``test_happy_path_stream_also_sets_raw_message_text`` (Teams) — pins symmetry: the adapter always sets the override, Thread.stream always honors it. - ``test_should_prefer_raw_message_text_override_over_local_accumulator`` (Thread) — pins that Thread.stream uses the override instead of its local accumulator when set. - ``test_should_fall_back_to_local_accumulator_when_text_override_is_none`` (Thread) — pins backward compatibility for adapters that don't set the override. Tests: 39 streaming, 199 in the streaming + thread file pair, 3,709 in the full suite (was 3,701 — net +8 across both files). 1 pre-existing GitHub-adapter failure unrelated. Lint + format clean.
The flush-window throttle wait (``_stream_sleep_ms``) and clock read sit outside the in-loop try/except in ``_stream_via_emit``. A supervisor-initiated task cancellation that fires while the flush is awaiting the throttle window propagates ``CancelledError`` out of the method while ``session.canceled`` is still ``False``. The finally-block close path would then proceed as if the stream ran to completion — violating the invariant "any exception leaving ``_stream_via_emit`` implies ``session.canceled``" that the close path relies on when deciding whether to ship a final ``message`` activity. Wrap the flush block in the same ``except (CancelledError, ...) / except Exception`` shape as the in-loop loop. ``_emit_streaming_activity`` already self-cancels on its own send failures (covered by ``test_flush_failure_propagates_and_cancels_session``); this commit covers the throttle-wait path that previously had no guard. Two new regression tests pin both the CancelledError and the generic exception cases. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
…rumb The new optional ``text`` field on ``RawMessage`` (added by 7809b7e to plumb the adapter-authoritative text override through to ``Thread.stream``'s recorded ``SentMessage``) is a Python-only divergence from upstream's ``RawMessage`` interface (``packages/chat/src/types.ts`` — fields are ``id``, ``raw``, ``threadId`` only). Per ``docs/SELF_REVIEW.md`` principle 5 ("Divergence budget"), every divergence needs a row in ``docs/UPSTREAM_SYNC.md`` AND a breadcrumb at the code site. Adds both: a new row under the Teams native streaming cluster documenting the why (cancellation-text reconciliation lives inside ``@microsoft/teams.apps``'s ``IStreamer.emit`` upstream; we hand-roll the wire format until ``microsoft-teams-apps`` Python is in reach) and referencing the existing regression tests, plus a "Divergence from upstream — see docs/UPSTREAM_SYNC.md" comment on the field itself. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Replying to the latest Codex review-summary on Ran a self-review pass over the cancellation-text patch chain ( Findings
No defect on the other adversarial checks
Validation The single failure is the pre-existing Pushed as Posted by an automated self-reviewer agent. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22a1b7e336
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| await self._teams_send(decoded, final_activity) | ||
| except Exception as exc: |
There was a problem hiding this comment.
Throttle the final stream activity
For short DM streams, the last successful typing emit can happen immediately before this final message send (for example, a one-chunk response emits in _stream_via_emit, then _handle_message_activity closes the session right away). The Teams streaming REST contract treats the final message as the next streaming API request and documents a 1 request/second quota, so this immediate final call can 429 under the default 1500 ms throttle; because the exception is swallowed below, Teams may never receive the final message while the SDK records the response as sent. Track the last emit time and wait the remaining interval before sending the final activity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — same root cause as the in-stream flush throttle (1 req/sec quota), just shifted to the close path. For short DM streams (one chunk emitted at t=0, then immediate close), the final message activity would land within the throttle window of the last typing and 429.
Fixed in commit 0509b3d by plumbing the timestamp + interval through to the close path:
-
_TeamsStreamSessiongains two fields:last_emit_at_ms: float = float("-inf")— mirror of the in-loop local, written after each successful emit (in-loop AND flush) so the close path can compute elapsed against it.emit_interval_ms: int | None = None— cache of the per-stream interval set at the top of_stream_via_emit. Honors caller-suppliedStreamOptions.update_interval_msoverrides without_close_stream_sessionneeding access tooptions.Nonefalls back to the adapter default for defensive paths (e.g. sessions constructed manually in tests).
-
_stream_via_emitwrites both fields:emit_interval_msonce at the top;last_emit_at_msafter each successful in-loop emit (usingnow_ms) and after the flush emit (re-reading the clock since the wait may have elapsed real time). -
_close_stream_sessioncomputeselapsed_ms = clock_now - session.last_emit_at_msand, if less than the interval, awaits_stream_sleep_ms(remaining)before sending the final activity. Supervisor-initiated cancellation during the wait is honored (mark session.canceled + re-raise);session.cancel()arriving from another task is re-checked after the sleep returns and skips the final emit.
6 new regression tests in TestCloseStreamThrottle:
test_close_waits_for_throttle_window_when_emit_was_recent— pins the wait whenlast_emit_at_msis freshtest_close_does_not_throttle_when_window_already_elapsed— pins no-wait when window has passedtest_close_honors_session_interval_override—update_interval_ms=0propagates throughtest_close_falls_back_to_adapter_default_when_session_interval_unset— defensive: manual sessionstest_close_does_not_throttle_when_no_emit_happened— defensive:last_emit_at_ms == -inftest_close_cancellation_during_wait_skips_final_emit— cancellation invariant during wait
Tests: 47 streaming (was 39, +8 — 6 new close-throttle + 2 from the self-review's earlier flush-wait CancelledError pair), 3,715 in the full suite, 1 pre-existing GitHub-adapter failure unrelated. Lint + format clean.
This closes out the throttle story: in-loop emits, end-of-stream flush, AND the close-path final activity all honor the same 1 req/sec quota.
Generated by Claude Code
…ing quota Codex P2 on commit 22a1b7e: Teams' streaming endpoint rate-limits ALL activities sharing a ``streamId`` together — the streaming ``typing`` activities AND the final ``message`` activity. For short DM streams (one chunk, emitted immediately), ``_handle_message_activity`` closes the session right after ``_stream_via_emit`` returns, and ``_close_stream_session`` ships the final ``message`` within milliseconds of the last ``typing`` activity. That lands two requests in the same second and risks a 429, which the close-path swallows fail-soft — leaving Teams' streaming UI stuck while the SDK records the response as sent. Fix: plumb the last-emit timestamp and the per-stream interval through to the close path via the session, then throttle. - ``_TeamsStreamSession`` gains two fields: ``last_emit_at_ms`` (mirror of the in-loop local for the close path's clock check) and ``emit_interval_ms`` (cache of the per-stream interval, honoring ``StreamOptions.update_interval_ms`` overrides without the close path needing access to ``options``). - ``_stream_via_emit`` writes both fields: ``emit_interval_ms`` at the top, ``last_emit_at_ms`` after each successful in-loop emit and the flush emit. - ``_close_stream_session`` computes ``elapsed_ms = clock_now - session.last_emit_at_ms`` and, if less than the interval, awaits ``_stream_sleep_ms`` before sending the final activity. - Cancellation during the close-path wait is honored: supervisor- initiated ``CancelledError`` cancels the session and re-raises (same shape as the flush wait); ``session.cancel()`` arriving via another task is re-checked after the sleep returns and skips the final emit. Six new regression tests in ``TestCloseStreamThrottle``: - ``test_close_waits_for_throttle_window_when_emit_was_recent`` — pins the throttle wait when ``last_emit_at_ms`` is fresh. - ``test_close_does_not_throttle_when_window_already_elapsed`` — pins no-wait when the window has passed. - ``test_close_honors_session_interval_override`` — caller's ``update_interval_ms=0`` propagates through to close. - ``test_close_falls_back_to_adapter_default_when_session_interval_unset`` — defensive: sessions constructed without ``emit_interval_ms`` (e.g. manually in tests) use the adapter default. - ``test_close_does_not_throttle_when_no_emit_happened`` — defensive: ``last_emit_at_ms == -inf`` (no emit ever) skips the wait. - ``test_close_cancellation_during_wait_skips_final_emit`` — cancellation invariant during the close-path wait. Tests: 45 streaming (was 39, +6 new), 3,715 in the full suite, 1 pre-existing GitHub-adapter failure unrelated. Lint + format clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0509b3dd34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session = self._active_streams.get(thread_id) | ||
| if session is not None and not session.canceled: | ||
| return await self._stream_via_emit(thread_id, text_stream, session, options) |
There was a problem hiding this comment.
Create a fresh native stream for each DM stream() call
stream() always reuses the thread-scoped _TeamsStreamSession when one is present, so if a single DM handler calls thread.stream() (or thread.post() with a stream) more than once, the second call is sent on the same Teams stream state instead of starting a new message. In this path _stream_via_emit overwrites session._text and returns the original session.first_chunk_id, so later calls can overwrite what the user saw from the first streamed reply while the SDK still records multiple sent messages; this is a behavior regression from the previous per-call post/edit flow.
Useful? React with 👍 / 👎.
|
@codex review 5 commits since the last Codex review (
Worth fresh eyes on the cancellation-text chain end-to-end (in-loop break, flush throttle wait, close-path throttle wait) and the new Generated by Claude Code |
…cumulator Self-review on commit 7809b7e (RawMessage.text happy-path override): the adapter's ``_stream_via_emit`` only extracted text from ``str`` and ``dict[type=markdown_text]`` chunks while ``Thread.stream``'s ``_wrapped_stream`` also handles ``MarkdownTextChunk`` dataclass instances (any object with ``.type == "markdown_text"``). Before 7809b7e this asymmetry was masked: ``Thread.stream`` recorded the ``SentMessage`` body from its own local accumulator (which DID include dataclass chunks), so even though Teams never shipped any streaming activities for object-form chunks, the recorded text matched the user's intent. After 7809b7e the Teams adapter sets ``RawMessage.text=final_text`` on the happy path too, and ``Thread.stream`` prefers that override over its own accumulator. ``final_text`` from Teams is the adapter's view (empty when only dataclass chunks were yielded), so the recorded ``SentMessage`` and ``_message_history`` entry would silently become an empty string while the user sees nothing in Teams. Fix: mirror ``Thread.stream``'s third extraction branch in the Teams adapter's chunk loop — ``getattr(chunk, "type", None) == "markdown_text"`` pulls ``.text``, matching the dataclass form. Non-text ``StreamChunk`` variants (``TaskUpdateChunk``, ``PlanUpdateChunk``) still fall through with ``text == ""`` and are skipped, identical to Thread's behavior. Adds ``test_markdown_text_chunk_dataclass_extract_text`` pinning the behavior with a mixed stream of ``MarkdownTextChunk``, ``TaskUpdateChunk``, and ``PlanUpdateChunk`` — would fail without the new branch. Tests: 208 in streaming + thread file pair (was 207, +1), 3,718 in the full suite (was 3,717), 1 pre-existing GitHub-adapter failure unrelated. Lint + format + pyrefly clean. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Ports upstream vercel/chat#416 (commit
ed46bae) —feat(adapter-teams): native streaming for DMs via emit— into the Python Teams adapter.DM threads now dispatch chunks through the Bot Framework streaming protocol (typing activities with
channelData.streamType=streaming+streamSequence, then a finalmessageactivity withstreamType=final) instead of routing throughThread._fallback_stream. Group chats / channels accumulate the stream and post a single message — matching upstream's post-#416 behavior of avoiding the post+edit flicker where Teams doesn't support native streaming.Wire format
typingactivity withchannelData.streamType=streaming+channelData.streamSequence=1.streamIdis omitted from bothchannelDataand thestreaminfoentity (server hasn't assigned one yet).streamId(captured from the first send's REST response) appears on BOTHchannelDataand thestreaminfoentity, per the Bot Framework streaming contract.message(nottyping),streamType=final, withstreamIdon bothchannelDataand thestreaminfoentity.Throttling
Teams' streaming endpoint enforces ~1 request/second; Microsoft recommends 1.5-2s buffering. The emit loop gates per-chunk sends on a configurable throttle window:
TeamsAdapterConfig.native_stream_min_emit_interval_ms(default 1500ms)StreamOptions.update_interval_msper-call (e.g. from aStreamingPlan)typingactivity. Without this, buffered text would only ship in the close-pathmessageactivity and a close-path failure would leaveThread.streamrecording text Teams never accepted.Send-failure contract (re-raise)
A
_teams_sendfailure mid-stream (429, network error) cancels the session, logs atwarn, and re-raises.Thread.stream's outer accumulator builds theSentMessagefrom each chunk before yielding to the adapter — re-raising short-circuits the post-stream history append so the SDK's record matches what the user actually saw via prior accepted typing activities. Same contract applies to the end-of-stream flush.The close-path final
messageactivity is allowed to fail-soft: by the time it runs, every byte ofsession.textwas confirmed-accepted via the flush, so a failure here is a stale-streaming-UI cost (until client timeout) rather than a recording inconsistency.Lifecycle (hazard #5: async task lifecycle)
_handle_message_activityregisters a_TeamsStreamSessionfor DMs andawaits the chat handler so the session stays alive throughstream(). The handler is fire-and-forget for non-DM threads._stream_via_emitemits cumulative text per chunk;streamIdis captured from the first send and threaded through all subsequent activities._close_stream_sessionposts the final activity. Skipped on cancellation or zero-chunk streams (no orphan finalizers).Documentation
docs/UPSTREAM_SYNC.md:_fallback_streamafter feat(adapter-teams): native streaming for DMs via emit vercel/chat#416).microsoft-teams-apps(Python SDK, GA 2026-05-01) whereIStreamer.emit()handles the wire format internally.streamIdplacement on bothchannelDataand thestreaminfoentity (Bot Framework REST contract).Migration tracking issue: #93 — Migrate Teams adapter to microsoft-teams-apps. Headlines a 0.4.28 / point release; out of scope for 4.27.0 (SDK only 2 weeks GA, requires Python 3.12 floor bump).
Tests
tests/test_teams_native_streaming.py(33 tests):streamIdeverywhere; subsequent chunks carrystreamSequenceincrements andstreamIdonchannelData; final close activity shipsstreamType=final.streamIdon thestreaminfoentity (not justchannelData).StreamOptions.update_interval_msoverrides the adapter default.idwas empty.process_message→stream()→_close_stream_session; handler exception with prior partial stream still drops session and ships close.Existing
TestStreamcases intest_teams_coverage.pyandtest_teams_extended.pyupdated for accumulate-and-post on group chats.Test plan
uv run ruff check src/ tests/ scripts/uv run ruff format --check src/ tests/ scripts/uv run python scripts/audit_test_quality.pyuv run pytest tests/ --tb=short -q— 3,701 pass, 1 pre-existing failure (unrelated to this PR), 2 skippedReview history (resolved)
Codex P1:
streamInfo.streamIdplacement (subsequent + final activities) → fixed inb29efe0Codex P1: throttle native emits (Bot Framework 1 req/sec quota) → fixed in
b29efe0Codex P2: re-raise on send failure (was silently corrupting
Thread.streamhistory) → fixed in86c95dbCodex P2: end-of-stream flush for throttle-buffered text (close-path failure variant of the above) → fixed in
114eddeCodeRabbit nitpick: duplicate final-streaminfo test → consolidated in
b9f018eDeferred Python-only follow-ups (tracked, out of scope for parity sync):
ContextVarinstead of_active_streams[thread_id])wait_untilcallable raising synchronously edge casemicrosoft-teams-appsSDK (issue Migrate Teams adapter to microsoft-teams-apps (official MS Python SDK) #93)Upstream ref: vercel/chat#416 (commit
ed46bae)https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
Generated by Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests