examples: fix setup and signaling inconsistencies - #970
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR moves WebRTC bootstrap tokens to URL fragments, sends them as bearer headers, filters public ICE configuration, and sanitizes redirects. It also adds isolated legacy LangChain checks and updates example types, installation guidance, and provider documentation. ChangesWebRTC authentication and browser flow
Legacy LangChain examples
Example contracts and guidance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant WebRTCClient
participant WebRTCRoutes
participant WebRTCSignaling
Browser->>WebRTCClient: Load URL with fragment token
WebRTCClient->>WebRTCSignaling: Request /config with bearer header
WebRTCSignaling-->>WebRTCClient: Return browser-safe ICE configuration
WebRTCClient->>WebRTCSignaling: Submit /offer with bearer header
WebRTCSignaling-->>Browser: Return WebRTC session response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 5
🤖 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 `@examples/function_tools_langchain.py`:
- Line 35: Move the trailing period outside the rendered command/code span in
examples/function_tools_langchain.py lines 35-35 and
examples/session_actions_langchain.py lines 35-35, so both displayed commands
can be copied without including punctuation.
In `@examples/README.md`:
- Line 82: Resolve the incompatible legacy LangChain dependency instructions by
isolating these examples in a separate environment with LangChain 0.3.x and a
compatible langchain-core range, or update the extra and lockfile accordingly.
Apply the fix to the install cells in examples/README.md lines 82-82 and 86-86,
and the setup commands in examples/function_tools_langchain.py lines 9-10 and
examples/session_actions_langchain.py lines 13-14, keeping all four locations
consistent.
In `@examples/webrtc_static/webrtc_client.html`:
- Around line 112-122: Update authHeaders and its callers fetchIceConfig and
negotiate to only attach the bearer token when the request URL matches the
trusted signaling origin, preventing authToken from location.search being
forwarded to different origins. Preserve existing extra headers and
unauthenticated behavior.
In `@examples/webrtc_static/webrtc_observability.html`:
- Around line 44-50: Update examples/webrtc_static/webrtc_observability.html
lines 44-50 to stop forwarding signalingToken through the iframe query string
and hand it to the same-origin WebRTC client via an explicit
postMessage/bootstrap mechanism. Update docs/deployment/production-servers.md
lines 213-218 to restrict URL-token forwarding to local development or document
the non-URL bootstrap. Update tests/examples/test_deploy_and_browser_docs.py
lines 652-662 to assert the safer token handoff rather than query-string
propagation.
In `@tests/examples/test_deploy_and_browser_docs.py`:
- Around line 652-662: The regression test
test_webrtc_observability_client_forwards_signaling_token currently requires URL
query-string token forwarding through webrtcParams.set("token", signalingToken).
Replace that assertion with checks for the selected non-URL, same-origin token
handoff in the observability flow, while retaining the existing bearer-header
assertions and other relevant behavior coverage.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6a9e5b9e-49a0-4037-b9d6-21303ba03516
📒 Files selected for processing (14)
docs/browser-playground.mddocs/deployment/production-servers.mddocs/teaching/13-swap-providers-and-transports/README.mdexamples/README.mdexamples/custom_transport.pyexamples/function_tools_langchain.pyexamples/noise_reduction_backends.pyexamples/session_actions_langchain.pyexamples/vad_backends.pyexamples/voice_app_twilio.pyexamples/webrtc_static/webrtc_client.htmlexamples/webrtc_static/webrtc_observability.htmltests/examples/test_deploy_and_browser_docs.pytests/examples/test_readme_matrix.py
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 @.github/workflows/ci.yml:
- Around line 191-198: Replace the incompatible `.[quickstart]` plus legacy
constraints in the legacy CI install step with a dedicated legacy extra or
environment specifying mutually compatible LangChain 0.3.x packages. Update the
README installation cells and `tests/test_dependency_policy.py` to reference and
validate this same legacy dependency contract, while keeping
`smoke_legacy_langchain_examples.py` running in that environment.
In `@tests/cli/test_serve.py`:
- Line 205: Update _playground_url() and its serve output path so token-bearing
URLs are not printed for non-loopback HTTP bindings. Require an explicitly
configured HTTPS public URL for remote token URLs, or restrict
token-authenticated direct HTTP to loopback, while preserving token output for
safe loopback and HTTPS cases.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2604465-e847-4401-a118-5ed9503775d1
📒 Files selected for processing (34)
.github/workflows/ci.ymlCONTRIBUTING.mddocs/browser-playground.mddocs/deployment/production-servers.mddocs/teaching/13-swap-providers-and-transports/README.mdexamples/README.mdexamples/custom_transport.pyexamples/ec2_webrtc/deploy.shexamples/noise_reduction_backends.pyexamples/vad_backends.pyexamples/voice_app_twilio.pyexamples/webrtc_observability_server.pyexamples/webrtc_server.pyexamples/webrtc_static/webrtc_client.htmlexamples/webrtc_static/webrtc_observability.htmljustfilescripts/smoke_legacy_langchain_examples.pysrc/easycat/cli/serve.pysrc/easycat/server/_webrtc_handlers.pysrc/easycat/server/webrtc_routes.pysrc/easycat/transports/_webrtc_config.pysrc/easycat/transports/static/webrtc_client.htmlsrc/easycat/transports/webrtc.pysrc/easycat/voice_app.pytests/cli/test_serve.pytests/examples/test_deploy_and_browser_docs.pytests/examples/test_example_imports.pytests/examples/test_readme_matrix.pytests/server/test_webrtc_routes.pytests/teaching/test_provider_matrix_measurement.pytests/test_dependency_policy.pytests/transports/test_voice_app_modes.pytests/transports/test_webrtc_auth_browser_playground.pytests/transports/test_webrtc_lifecycle_server.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/session_actions_langchain.py (1)
13-14: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the legacy LangChain guidance with the pinned
langchain-corefloor.Both setup paths add
langchain<1/langchain-openai<1, but this repo pinslangchain-core>=1.5.3forlangchain/alland the installed lock containslangchain_core==1.5.3. Legacy LangChain 0.3.x is not compatible withlangchain-core>=1.5.3, so these setup instructions can fail or leave an unusable dependency graph. Add an explicit constraint for bothlangchainpackages that matcheslangchain-core<1, or move this example into a separate dependency path.🤖 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 `@examples/session_actions_langchain.py` around lines 13 - 14, Update the setup guidance in session_actions_langchain.py so both installation paths constrain langchain and langchain-openai to versions compatible with the repository’s langchain-core requirement, using an explicit langchain-core<1 constraint alongside the existing package bounds.
🤖 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.
Outside diff comments:
In `@examples/session_actions_langchain.py`:
- Around line 13-14: Update the setup guidance in session_actions_langchain.py
so both installation paths constrain langchain and langchain-openai to versions
compatible with the repository’s langchain-core requirement, using an explicit
langchain-core<1 constraint alongside the existing package bounds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cfc9f391-7328-4828-b3e0-47e09d76cab6
📒 Files selected for processing (6)
docs/browser-playground.mddocs/teaching/15-operate-in-production/README.mdexamples/function_tools_langchain.pyexamples/session_actions_langchain.pysrc/easycat/cli/serve.pytests/cli/test_serve.py
|
Addressed in 607713f by using the separate dependency path suggested here, expanded into first-class support for both LangChain lines:
Validation: both isolated installs passed their example/runtime smoke, and the same 7 real-SDK bridge contract tests passed on LangChain 0.3 and 1.x. Dependency-policy, examples, docs, lint, formatting, typing, pre-commit, and lock checks also pass. This finding was emitted in the review body as an outside-diff comment, so GitHub did not create an inline thread that can be marked resolved. This reply records it as addressed. |
1 participant
Problem
Runnable examples, their setup guidance, and WebRTC signaling behavior had drifted across dependency versions, transport capabilities, authentication defaults, and TURN exposure semantics. Some gaps were security-relevant: browser bootstrap tokens traveled in query strings, root redirects repeated them, and hidden-credential TURN entries produced invalid browser ICE configuration.
Changes
#token=fragment, scrub it immediately, and ignore/remove legacy query bootstrapsturn:andturns:, and repair failed pre-registration WebRTC startup rollbackLangChainBridge, and runs a network-freeRunnableLambdaturnImpact
Authenticated browser links no longer put bearer tokens in HTTP requests or redirect history. Browser ICE configuration is always constructible, server-only TURN remains usable, example setup commands match their dependencies, and transport/provider claims are backed by runtime constants or regression coverage.
Root cause
Examples, browser assets, CI dependency lanes, and shared WebRTC serializers evolved independently, allowing duplicated assumptions and stale guidance to accumulate.
Verification
langchain==0.3.30andlangchain-openai==0.3.35; example load, bridge adaptation, and runtime event streaming passedgit diff --check, and targeted environment/path-sensitive reruns passedSummary by CodeRabbit
New Features
servedeployments.Bug Fixes
Documentation