add proxy support for realtime calls (websocket based) - #5788
Open
akshaydeo wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughWebSocket dialing now accepts provider proxy configuration for HTTP, SOCKS5, and environment-backed proxies. Realtime and Responses handlers pass proxy settings to direct and pooled connections. Tests cover proxy configuration and tunneled WebSocket flows. The provider form documents proxy coverage. ChangesWebSocket proxy support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Handler
participant WebSocketPool
participant ConfigureWebSocketProxy
participant Upstream
Handler->>WebSocketPool: pass provider ProxyConfig
WebSocketPool->>ConfigureWebSocketProxy: configure new dialer
ConfigureWebSocketProxy-->>WebSocketPool: configured dialer
WebSocketPool->>Upstream: establish WebSocket connection
Upstream-->>Handler: return pooled connection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Ping-pong health check failed Comment |
akshaydeo
marked this pull request as ready for review
August 3, 2026 06:34
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@core/providers/utils/utils.go`:
- Around line 661-665: Update the custom CA handling in the proxy configuration
flow around createTLSConfigWithCA: when TLS configuration creation fails, return
or propagate the configuration error instead of only logging it and continuing
with an unset dialer.TLSClientConfig; retain the successful path that assigns
tlsConfig to the dialer.
- Around line 635-638: The explicit HTTPProxy or Socks5Proxy path must fail
closed when proxyConfig.URL.GetValue() is empty instead of returning the direct
dialer. Update the proxy setup logic in core/providers/utils/utils.go:635-638 to
return a clear invalid-proxy-configuration error, and update the corresponding
expectations in core/providers/utils/proxy_test.go:156-170 to assert that error
rather than direct-dial behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f24be127-653b-4f67-963e-58cd44eb336c
📒 Files selected for processing (8)
core/providers/utils/proxy_test.gocore/providers/utils/utils.gotransports/bifrost-http/handlers/wsrealtime.gotransports/bifrost-http/handlers/wsresponses.gotransports/bifrost-http/websocket/connection.gotransports/bifrost-http/websocket/pool.gotransports/bifrost-http/websocket/pool_test.goui/app/workspace/providers/fragments/proxyFormFragment.tsx
akshaydeo
force-pushed
the
08-02-add_proxy_support_for_realtime_calls_websocket_based_
branch
from
August 3, 2026 07:27
3cacee3 to
8ff1080
Compare
akshaydeo
force-pushed
the
08-02-add_proxy_support_for_realtime_calls_websocket_based_
branch
from
August 3, 2026 07:35
8ff1080 to
661c00c
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/providers/utils/utils.go (1)
632-639: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed instead of silently dialing direct when an explicit proxy has no URL.
For
HTTPProxyandSocks5Proxy, ifproxyURLValueis empty, the code logs a warning and returnsdialer, nilat Line 638, leavingdialer.Proxyunset. The caller then dials directly, bypassing the configured proxy.This is the same pattern flagged in a previous review round on these lines, which was reported as "Addressed in commit 8ff1080" with the fix of returning an error instead of falling back to direct dialing. The code shown here still contains the pre-fix behavior, so either the fix did not land on this branch or was reverted. Confirm whether the intended fix is present in this branch, and if not, reapply it.
🔒 Proposed fix
proxyURLValue := proxyConfig.URL.GetValue() if proxyURLValue == "" { - getLogger().Warn("Warning: proxy URL is required for setting up WebSocket proxy") - return dialer, nil + return nil, fmt.Errorf("invalid proxy configuration: proxy URL is required for WebSocket proxy") }🤖 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 `@core/providers/utils/utils.go` around lines 632 - 639, Update the empty proxy URL handling in the HTTPProxy and Socks5Proxy setup paths around proxyURLValue so an explicitly configured proxy fails closed: return a descriptive error instead of logging a warning and returning dialer with no Proxy. Preserve the existing secret-reference validation and normal proxy setup behavior for non-empty URLs.
🧹 Nitpick comments (1)
core/providers/utils/utils.go (1)
623-670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing URL/credential-resolution logic with
ConfigureProxy.
ConfigureWebSocketProxyduplicates the secret-resolution check, URL parsing, and username/password merging already present inConfigureProxy(lines 530-553, 556-580). The doc comment at Line 614 correctly explains why the two functions differ in failure behavior (fail-fast dial func vs. direct error return), but the URL/credential-resolution portion itself does not need to differ.Extracting a small shared helper, e.g.
resolveProxyURL(proxyConfig, fieldName) (*url.URL, error), that performs the secret check,GetValue(),url.Parse, andUserPasswordmerge, would let both callers apply the same validation. This also reduces the risk of the two code paths drifting apart, which is part of why the fail-open regression above only affects the WebSocket path and notConfigureProxy.🤖 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 `@core/providers/utils/utils.go` around lines 623 - 670, Extract the duplicated proxy URL and credential resolution from ConfigureProxy and ConfigureWebSocketProxy into a shared helper such as resolveProxyURL, including secret-reference validation, value retrieval, URL parsing, and username/password merging. Update both callers to use the helper while preserving their existing distinct error-handling behavior and the WebSocket-specific empty URL handling.
🤖 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.
Duplicate comments:
In `@core/providers/utils/utils.go`:
- Around line 632-639: Update the empty proxy URL handling in the HTTPProxy and
Socks5Proxy setup paths around proxyURLValue so an explicitly configured proxy
fails closed: return a descriptive error instead of logging a warning and
returning dialer with no Proxy. Preserve the existing secret-reference
validation and normal proxy setup behavior for non-empty URLs.
---
Nitpick comments:
In `@core/providers/utils/utils.go`:
- Around line 623-670: Extract the duplicated proxy URL and credential
resolution from ConfigureProxy and ConfigureWebSocketProxy into a shared helper
such as resolveProxyURL, including secret-reference validation, value retrieval,
URL parsing, and username/password merging. Update both callers to use the
helper while preserving their existing distinct error-handling behavior and the
WebSocket-specific empty URL handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 224caeec-5f93-4c58-a248-aa43708e01fd
⛔ Files ignored due to path filters (14)
core/go.sumis excluded by!**/*.sumframework/go.sumis excluded by!**/*.sumplugins/compat/go.sumis excluded by!**/*.sumplugins/governance/go.sumis excluded by!**/*.sumplugins/jsonparser/go.sumis excluded by!**/*.sumplugins/logging/go.sumis excluded by!**/*.sumplugins/maxim/go.sumis excluded by!**/*.sumplugins/mocker/go.sumis excluded by!**/*.sumplugins/modelcatalogresolver/go.sumis excluded by!**/*.sumplugins/otel/go.sumis excluded by!**/*.sumplugins/prompts/go.sumis excluded by!**/*.sumplugins/semanticcache/go.sumis excluded by!**/*.sumplugins/telemetry/go.sumis excluded by!**/*.sumtransports/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
Makefilecore/go.modcore/providers/utils/proxy_test.gocore/providers/utils/utils.goframework/go.modplugins/compat/go.modplugins/governance/go.modplugins/jsonparser/go.modplugins/logging/go.modplugins/maxim/go.modplugins/mocker/go.modplugins/modelcatalogresolver/go.modplugins/otel/go.modplugins/prompts/go.modplugins/semanticcache/go.modplugins/telemetry/go.modtests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtransports/bifrost-http/handlers/wsrealtime.gotransports/bifrost-http/handlers/wsresponses.gotransports/bifrost-http/websocket/connection.gotransports/bifrost-http/websocket/pool.gotransports/bifrost-http/websocket/pool_test.gotransports/go.modui/app/workspace/providers/fragments/proxyFormFragment.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/cmd/seed/go.mod
- transports/bifrost-http/handlers/wsrealtime.go
- tests/cmd/seedvks/go.mod
- transports/bifrost-http/websocket/pool.go
- transports/bifrost-http/handlers/wsresponses.go
- ui/app/workspace/providers/fragments/proxyFormFragment.tsx
- tests/cmd/e2eseed/go.mod
- Makefile
- transports/bifrost-http/websocket/connection.go
- transports/bifrost-http/websocket/pool_test.go
- core/providers/utils/proxy_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WebSocket connections (Realtime and Responses) were not routing through the provider-level proxy configuration. HTTP requests already respected
ProxyConfig, but the WebSocket dial path bypassed it entirely, causing WebSocket traffic to go direct regardless of what proxy was configured.Changes
ConfigureWebSocketProxyincore/providers/utils/utils.gothat mirrorsConfigureProxyfor*ws.Dialer, supporting HTTP, SOCKS5, env-based, and no-proxy configurations. UnlikeConfigureProxy, it returns an error directly rather than swapping in a failing dial func, since WebSocket dials are resolved fresh on every call.Dial,DialUpstream,Pool.Get, andPool.dialin the WebSocket transport to accept and apply a*schemas.ProxyConfig.WSRealtimeHandler.runRealtimeSessionandWSResponsesHandler.tryNativeWSUpstreamto look up the provider'sProxyConfigand pass it through to the dial path.ConfigureWebSocketProxycovering literal URL, env-backed URL, empty env value (fail-fast), nil config,NoProxy, and SOCKS5 cases.pool_test.gowith a real CONNECT-based forward proxy to verify dials actually route through the proxy (TestPoolGetDialsThroughConfiguredHTTPProxy) and that an unreachable proxy fails the dial rather than falling back silently (TestPoolGetFailsWithUnreachableProxy).Type of change
Affected areas
How to test
Configure a provider with an HTTP or SOCKS5 proxy and open a Realtime or Responses WebSocket session. Verify traffic routes through the proxy (e.g., via proxy access logs or by pointing at a local intercepting proxy). Confirm that setting an unreachable proxy URL causes the connection to fail rather than silently connecting directly.
Breaking changes
Pool.Get,Pool.dial,Dial, andDialUpstreamall gained aproxyConfig *schemas.ProxyConfigparameter. Passingnilpreserves the previous direct-dial behavior.Security considerations
Proxy credentials (
Username,Password) are sourced fromSecretVarand embedded into the parsed proxy URL only when both are non-empty. CA certificate PEM for proxy TLS is validated at dial time and fails fast if an env-backed secret resolves to an empty value, preventing silent misconfiguration.Checklist
docs/contributing/README.mdand followed the guidelines