test(streaming): port 6 StreamingPlan option tests (#56)#74
test(streaming): port 6 StreamingPlan option tests (#56)#74patrick-chinchill merged 3 commits intomainfrom
Conversation
|
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 5 minutes and 48 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 (6)
📝 WalkthroughWalkthroughThis PR implements the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Thread
participant PostRouter as Post Router
participant StreamHandler as Stream Handler
participant Adapter
Caller->>Thread: post(StreamingPlan message)
Thread->>PostRouter: detect kind == "stream"
PostRouter->>StreamingPlan: get_post_data()
StreamingPlan-->>PostRouter: stream + options
PostRouter->>StreamHandler: _handle_stream(stream, extra_options)
StreamHandler->>StreamHandler: merge options from<br/>message context +<br/>extra_options
StreamHandler->>Adapter: stream(message, stream_options)
Adapter-->>StreamHandler: streaming result
StreamHandler-->>Thread: return message
Thread-->>Caller: return message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 introduces the StreamingPlan and StreamingPlanOptions classes to the Python SDK, mirroring the upstream Vercel Chat SDK. These additions allow developers to pass platform-specific streaming options, such as task grouping and stop blocks, through the Thread.post() method. The changes include updates to the Thread class to route these options into the streaming handler and comprehensive tests to ensure fidelity with the upstream implementation. Feedback was provided to use explicit 'is not None' checks when mapping these options to ensure that valid falsy values, such as an update interval of zero or an empty list of blocks, are correctly propagated.
| if group_tasks: | ||
| extra.task_display_mode = group_tasks | ||
| if end_with: | ||
| extra.stop_blocks = end_with | ||
| if update_interval_ms: | ||
| extra.update_interval_ms = update_interval_ms |
There was a problem hiding this comment.
Use is not None checks when mapping StreamingPlanOptions to StreamOptions. This ensures that falsy but valid values, such as update_interval_ms=0 or an empty list for end_with, are correctly propagated to the stream handler instead of being silently ignored.
| if group_tasks: | |
| extra.task_display_mode = group_tasks | |
| if end_with: | |
| extra.stop_blocks = end_with | |
| if update_interval_ms: | |
| extra.update_interval_ms = update_interval_ms | |
| if group_tasks is not None: | |
| extra.task_display_mode = group_tasks | |
| if end_with is not None: | |
| extra.stop_blocks = end_with | |
| if update_interval_ms is not None: | |
| extra.update_interval_ms = update_interval_ms |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 452b0ac8ab
ℹ️ 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".
| def get_fallback_text(self) -> str: | ||
| """StreamingPlan has no static fallback text -- streaming is handled | ||
| by Thread.post() directly via the native-or-fallback streaming path.""" | ||
| return "" |
There was a problem hiding this comment.
StreamingPlan.get_fallback_text() returns an empty string, but non-thread posting paths still treat it as a generic PostableObject fallback payload. In practice, ChannelImpl.post will post blank content (or attempt post_object("stream", ...) on adapters with object posting) instead of consuming the wrapped async stream, so callers who reuse StreamingPlan outside Thread.post silently lose the message body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ccb5fde — Option 2 (fail loud). StreamingPlan.get_fallback_text() and is_supported() now raise RuntimeError to prevent silent blank-posting via non-stream-aware paths. post_postable_object has a kind == "stream" short-circuit guard. Divergence documented in docs/UPSTREAM_SYNC.md.
| if update_interval_ms: | ||
| extra.update_interval_ms = update_interval_ms |
There was a problem hiding this comment.
The StreamingPlanOptions.update_interval_ms mapping uses a truthy check, so an explicit value of 0 is dropped as if it were unset. That means thread.post(StreamingPlan(..., update_interval_ms=0)) cannot propagate zero to adapter.stream(...) or fallback mode, and the code falls back to the default interval instead of honoring the caller’s per-message override.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ccb5fde — update_interval_ms=0 now propagates via is not None check (also fixed the parallel _fallback_stream interval guard at line 660 that had the same truthy-check bug).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/plan.py`:
- Around line 522-538: StreamingPlan should not advertise generic PostableObject
support; change get_fallback_text()/is_supported() so the shared post helper
doesn't try adapter.post_object with a stream. Specifically, update
StreamingPlan.is_supported to return False (or otherwise indicate unsupported by
adapters) and keep/get a clear fallback (e.g., empty or explicit sentinel) so
post_postable_object() will skip adapter.post_object(...) for kind == "stream"
and force callers to use Thread.post() for streaming; adjust the docstring on
get_fallback_text/is_supported to note Thread.post()‑only and reference
post_postable_object and Thread.post to ensure the shared helper special-cases
stream kinds or relies on is_supported == False.
In `@src/chat_sdk/thread.py`:
- Around line 471-480: The current truthiness checks on plan_options fields drop
explicit falsy values (e.g., end_with=[] or update_interval_ms=0); update the
conditional logic in the block handling plan_options so each field is checked
with "is not None" before assigning: check group_tasks is not None, end_with is
not None, and update_interval_ms is not None, and then assign to
extra.task_display_mode, extra.stop_blocks, and extra.update_interval_ms
respectively so explicit falsy choices are preserved.
In `@tests/test_thread_faithful.py`:
- Around line 943-973: Update the
test_should_route_streamingplan_through_fallback_when_adapter_has_no_native_streaming
to spy/mock the _fallback_stream function and assert that it is called with the
StreamingPlanOptions passed through the StreamingPlan; specifically capture the
options argument to _fallback_stream and assert its mapped fields
(task_display_mode, stop_blocks/stop_blocks flag or equivalent, and
update_interval_ms) match the values derived from
StreamingPlanOptions(group_tasks="plan", end_with=[{"type":"actions"}],
update_interval_ms=2000). Ensure you reference the actual symbols
_fallback_stream and StreamingPlanOptions and assert the mapped fields
(task_display_mode, stop_blocks, update_interval_ms) rather than just verifying
output edits so the test fails if options are dropped.
🪄 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: 5e14e8b3-76e0-4366-b52a-262513c3884b
📒 Files selected for processing (5)
CHANGELOG.mdsrc/chat_sdk/__init__.pysrc/chat_sdk/plan.pysrc/chat_sdk/thread.pytests/test_thread_faithful.py
- StreamingPlanOptions mapping: replace truthy checks with `is not None` so update_interval_ms=0 and end_with=[] propagate correctly (thread.py post dispatcher and _fallback_stream interval guard). Per CLAUDE.md Port Rule #1; diverges from upstream thread.ts, which has the same latent bug. - StreamingPlan: raise RuntimeError from is_supported() and get_fallback_text() so misroutes through post_postable_object / Channel.post fail loudly instead of posting "" or attempting a wrong-shape adapter.post_object("stream", ...). Also guard post_postable_object() with an early kind=="stream" check for a clearer error message. Diverges from upstream postable-object.ts, which silently posts the empty fallback string. - Fallback test: spy on _fallback_stream to capture the StreamOptions actually reaching the fallback path and assert task_display_mode, stop_blocks, and update_interval_ms all propagate -- previous test passed even when options were silently dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review verdict: LGTM. Refreshed and reviewed latest head 90c1339 against main. No issues found in StreamingPlan option propagation to native stream and fallback stream paths. Verification: tests/test_thread_faithful.py passed (133 passed) and ruff passed on src plus test_thread_faithful.py. Residual risk: standalone fidelity script remains nonzero because other PR parity gaps are not in this branch. Formal GitHub approval is blocked because the authenticated account owns this PR. |
Python port was missing the `StreamingPlan` PostableObject entirely, so `thread.post(StreamingPlan(stream, options))` silently dropped the options (`group_tasks`, `end_with`, `update_interval_ms`) -- there was no `kind == "stream"` branch in `Thread.post()` to map them onto `StreamOptions` before invoking `adapter.stream(...)` or the fallback `post+edit` path. Wiring fix: - Add `StreamingPlan` + `StreamingPlanOptions` in `chat_sdk.plan` (mirrors upstream `streaming-plan.ts`). - `Thread.post()` now detects `kind == "stream"` and maps `group_tasks -> task_display_mode`, `end_with -> stop_blocks`, `update_interval_ms -> update_interval_ms` onto `StreamOptions` before streaming. Works for both native (`adapter.stream`) and fallback paths. - `_handle_stream` accepts `extra_options` so the mapped fields are merged on top of the message-context defaults. Tests (6 new faithful translations from upstream `thread.test.ts` `[Streaming]` block): - should pass StreamingPlan PostableObject options to adapter.stream - should pass StreamingPlan with only groupTasks - should pass StreamingPlan with only endWith - should pass StreamingPlan with only updateIntervalMs - should route StreamingPlan through fallback when adapter has no native streaming - should still work without options (backward compat) Fidelity: 548 -> 554 matched (+6); 6 `[Streaming]` gaps closed. All 3,551 tests pass. Closes #56. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- StreamingPlanOptions mapping: replace truthy checks with `is not None` so update_interval_ms=0 and end_with=[] propagate correctly (thread.py post dispatcher and _fallback_stream interval guard). Per CLAUDE.md Port Rule #1; diverges from upstream thread.ts, which has the same latent bug. - StreamingPlan: raise RuntimeError from is_supported() and get_fallback_text() so misroutes through post_postable_object / Channel.post fail loudly instead of posting "" or attempting a wrong-shape adapter.post_object("stream", ...). Also guard post_postable_object() with an early kind=="stream" check for a clearer error message. Diverges from upstream postable-object.ts, which silently posts the empty fallback string. - Fallback test: spy on _fallback_stream to capture the StreamOptions actually reaching the fallback path and assert task_display_mode, stop_blocks, and update_interval_ms all propagate -- previous test passed even when options were silently dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ption coverage - Add non-parity row for StreamingPlan.is_supported/get_fallback_text raising vs upstream's silent true/"" return - Extend fallback test to explicitly cover update_interval_ms=0 and end_with=[], locking in the truthiness fix against future regression Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
patrick-chinchill
force-pushed
the
feat/streaming-tests-56
branch
from
90c1339 to
05095e6
Compare
…y baseline Two infra-level changes to the fidelity check: 1. `.github/workflows/lint.yml` — the `Clone upstream vercel/chat at pinned parity tag` step no longer carries `continue-on-error: true`. The clone is infrastructure the fidelity check depends on; if it fails, the job should fail there rather than swallow the failure and hope a later step catches it. Combined with the script-level guard from the previous commit, this is defense in depth. 2. CI now runs `scripts/verify_test_fidelity.py --strict`. Every `[post with Plan]` test that was baselined is now ported (PR #75 and PR #74 in the 0.4.26.2 bundle), so the repo ships at 0 missing. `scripts/fidelity_baseline.json` is reduced to `{"missing": {}}` (metadata retained so `--update-baseline` and the documented workflow still function for future upstream syncs). Closes self-review gap #2 on #72.
Per gemini-code-assist review on PR #83. Without the repo prefix, GitHub auto-links the upstream PR numbers to local PRs in chat-sdk-python, which collides with the local refs (#64, #66, #67, #74, #82) elsewhere in the file. Use vercel/chat#NNN so the upstream refs link correctly. https://claude.ai/code/session_01FyMxQn2BEAzmwKS1GZczKj
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
1 participant
Summary
StreamingPlanPostableObject entirely, sothread.post(StreamingPlan(stream, options))silently droppedgroup_tasks/end_with/update_interval_ms. There was nokind == "stream"branch inThread.post()to map them ontoStreamOptionsbefore invokingadapter.stream(...)or the fallbackpost+editpath — aStreamingPlan-shaped object would have hit the genericpost_postable_objecthelper, which callsadapter.post_message(..., obj.get_fallback_text())and throws the stream + options away.StreamingPlan+StreamingPlanOptionsinchat_sdk.plan(mirrors upstreamstreaming-plan.ts), and akind == "stream"branch inThread.post()that mapsgroup_tasks → task_display_mode,end_with → stop_blocks,update_interval_ms → update_interval_msontoStreamOptions. Options now flow through both the native and fallback streaming paths.[Streaming]StreamingPlanoption-variant tests faithfully fromthread.test.ts.Wiring verification outcome
Investigated before porting tests as required.
thread.tshas thekind === "stream"branch (lines 417–439) that builds aStreamOptionsfromdata.options.{updateIntervalMs, groupTasks, endWith}and callshandleStream(stream, options). The fallback path (fallbackStream) readsoptions?.updateIntervalMsso options propagate there too.StreamingPlanclass did not exist.Thread.post()(thread.py:461) handledis_postable_object(...)but had nokind == "stream"special case._handle_streamalso did not accept caller-supplied options. → Options silently dropped.StreamingPlan+StreamingPlanOptions, newkind == "stream"branch inpost,_handle_stream(..., extra_options=...); explicit fields win over message-context defaults.Fidelity impact
thread.test.ts, including all 6[Streaming]gaps).[Streaming]StreamingPlan gaps closed; other pre-existing gaps (e.g.[post with Plan],[Options Load]) untouched — they belong to other issues.Test plan
uv run ruff check src/ tests/ scripts/— cleanuv run ruff format --check src/ tests/ scripts/— 192 files already formatteduv run pyrefly check— 0 errorsuv run python scripts/audit_test_quality.py— 0 hard failures (pre-existing warnings only)TS_ROOT=/private/tmp/vercel-chat uv run python scripts/verify_test_fidelity.py— 6[Streaming]StreamingPlan gaps closeduv run pytest tests/ --tb=short -q— 3,551 passed, 2 skipped🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
StreamingPlanandStreamingPlanOptionspublic APIs for enhanced streaming control.