Repository navigation
fix(host,runner): honor mandatory egress proxies on the WebSocket tunnels - #6752
omni-resolve-agent[bot] wants to merge 22 commits into
Conversation
…nels In a sandbox whose only network path is a CONNECT proxy (HTTP_PROXY/ HTTPS_PROXY/ALL_PROXY set, no direct DNS or TCP egress), every proxy-honoring HTTP client reaches the server fine, but the pinned websockets<15 client has no proxy support at all: the host and runner tunnels dialed the origin directly and looped on 'Temporary failure in name resolution' forever, so the host never came online. Give the tunnels the same env-proxy semantics as the HTTP clients (omnigent/util/ws_proxy.py): pick the proxy the standard variables configure for the tunnel URL (honoring NO_PROXY), establish the CONNECT tunnel ourselves, and hand the connected socket to websockets via its sock= parameter. No dependency bump is required (the websockets<15 pin stays, deliberately kept for the macOS >=15 handshake regression), nothing changes when no proxy is configured, and the explicit socket keeps working if the pin is ever lifted. Covered by an end-to-end reproduction (tests/e2e/ test_host_mandatory_proxy_sandbox_e2e.py: a real 'omnigent host' inside an emulated proxy-mandatory sandbox must register online through a CONNECT the proxy actually served), unit tests for the proxy selection and CONNECT dialer, and connect-kwargs tests for both tunnel call sites.
github-actions
Bot
added
P1-high
|
UI Preview for this PR has been removed. |
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Resolves conflicts with main's async connect-header build (_run_host_subprocess_in_thread), the tunnel_limits import split, and tests appended to test_connect.py. The proxy CONNECT dial keeps the merged open_timeout local; both sides' tests are kept.
Both files drove the identical journey (mandatory CONNECT proxy, host must come online) with the same test name. Keep the newer reproduction test: it additionally counts the tunnel's CONNECT attempts vs the host's proxied REST calls in its failure message and fails fast once the direct-DNS reconnect loop is proven, instead of waiting out the full online deadline.
|
Resolve-agent reviewed this contributor PR as the candidate fix. Future actionable maintainer review feedback may be remediated automatically. |
![omni-resolve-agent[bot]](https://avatars.githubusercontent.com/in/4711376?s=60&v=4){kind=small}
There was a problem hiding this comment.
Resolve-agent review outcome: fixed.
Summary
PR #6752's approach, kept as the fix: new stdlib-only omnigent/util/ws_proxy.py mirrors standard env-proxy semantics (ws→http_proxy, wss→https_proxy, all_proxy fallback, full NO_PROXY handling, basic auth, redacted logging) and establishes the CONNECT tunnel itself; both tunnel connect sites hand the connected socket to websockets via sock=, keeping the deliberate websockets<15 pin with zero behavior change without proxy env. My review commits on the PR branch: merged latest origin/main (resolving conflicts with main's async _build_connect_headers thread hand-off) and consolidated the PR's duplicate e2e test into the stronger reproduction test.
Root cause
The pinned websockets<15 client has no proxy support, and neither tunnel connect site (omnigent/host/connect.py _connect_and_serve, omnigent/runner/transports/ws_tunnel/serve.py _serve_tunnel_once) passed any proxy handling, so in a proxy-mandatory sandbox the WebSocket tunnel dialed the origin directly and looped on name resolution while the host's httpx REST calls (trust_env) reached the server through the proxy.
CI
Local focused validation green on committed HEAD 29d885a (repro e2e, tests/host/test_connect.py 228 passed, ws_tunnel 66 passed, tests/test_ws_proxy.py, ruff + project lint). PR #6752's previous CI (old head 4bd162b): 2 failing shards both unrelated to this diff — E2E shard 2/4 test_repl_cancel_re_arms_for_next_turn (REPL Ctrl-C snapshot; PR touches no REPL code) and E2E UI shard 3/4 test_initial_prompt_stays_bound_to_origin_session_after_switch (Playwright click timeout; PR touches no web code). The branch was mergeable:CONFLICTING; the merge commit 7aba21a resolves it against latest origin/main. Fresh CI runs after the workflow-owned publisher pushes; GitHub writes are publisher-owned in this job.
Automated review
Not dispatched — this CI agent job is implementation-only (no GitHub writes); no real Polly review was obtained for the new head, so no approving review is warranted (I also pushed commits to this PR, which rules out self-approval regardless). The post-publication workflow owns the Polly loop on the pushed head.
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
Signed-off-by: Dhruv Gupta <dhruv0811@gmail.com>
dhruv0811
left a comment
dhruv0811
left a comment
There was a problem hiding this comment.
I'd like these two connection cases fixed before merging. The change otherwise looks reasonably contained.
The 88 relevant tests passed locally. Could we also add one compact test for a real, verified wss:// connection through CONNECT? The current end-to-end test uses plain ws://.
|
🤖 Otto review remediation started Otto is addressing requested changes from @dhruv0811. |
…iables Review follow-ups for the mandatory-egress proxy support on the WebSocket tunnels: - ws_env_proxy_url() dials loopback targets directly (via is_loopback_url), matching the trust_env guard on the server-bound HTTP clients. A local host with HTTP_PROXY or ALL_PROXY set and no loopback NO_PROXY entry no longer sends its tunnel to a proxy that cannot reach localhost. - A lowercase proxy variable that is set but empty (http_proxy="", no_proxy="") now suppresses its uppercase form instead of falling through to it, matching urllib and httpx. - tests/test_ws_proxy.py gains a real, verified wss:// connection through CONNECT: TLS is layered on the proxied socket, the certificate is checked against the tunnel URL's host (which only the proxy resolves), and a certificate for another name is rejected. - The runner tunnel closes the proxied socket when websockets.connect() fails before the event loop adopts it, mirroring the host tunnel, with a regression test. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
@dhruv0811 thanks for the review. All three requests are addressed in 7fd323b:
I also closed Polly's blocking finding from the earlier head: Validation on 7fd323b: |
|
🔍 OpenCodeReview found 10 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
| Category | Files | Added | Deleted | Share of changed text lines |
|---|---|---|---|---|
| Tests and test support | 6 | 550 | 0 | 60.2% |
| Documentation | 0 | 0 | 0 | 0.0% |
| Dependencies and lockfiles | 0 | 0 | 0 | 0.0% |
| Implementation / other | 3 | 349 | 15 | 39.8% |
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements.
Changes
The PR adds environment-based proxy selection, bypass matching, authenticated HTTP CONNECT, and redacted logging, then integrates the resulting socket into both tunnel clients. The remaining changes provide regression coverage and isolate tests from ambient proxy settings. There are no dependency, lockfile, documentation, database, or generated-file changes.
The size is justified by the two integration points and the need to exercise real CONNECT and TLS behavior. No substantial implementation removal is warranted. The small, directory-scoped environment fixtures are preferable to broadening test isolation globally merely to remove duplication.
Tests
Test-by-test assessment
The frozen snapshot and its checksum were verified. Tests and lint were not independently run at the target revision; the reported passing results remain author-provided evidence.
| Test / case | Behavior protected | Layer | Needed? | Action / rationale |
|---|---|---|---|---|
tests/host/conftest.py:_no_ambient_proxy_env |
Host tests do not inherit unrelated proxy configuration | Fixture | keep | Directory scope makes existing host tests deterministic without changing unrelated suites. |
tests/runner/transports/ws_tunnel/conftest.py:_no_ambient_proxy_env |
Runner tunnel tests do not inherit unrelated proxy configuration | Fixture | keep | Applies the same isolation at the other affected integration point. |
tests/host/test_connect.py:test_connect_and_serve_proxy_socket — direct case |
Direct connections pass no socket and do not invoke CONNECT | Unit | keep | Protects unchanged behavior when proxy configuration is absent. |
tests/host/test_connect.py:test_connect_and_serve_proxy_socket — proxy case |
Host passes the selected socket and timeout, then closes it on upgrade failure | Unit | keep | Protects host-specific integration and ownership; extend with SSL-context construction failure. |
tests/runner/transports/ws_tunnel/test_serve.py:test_serve_tunnel_once_sends_bearer_header; _ConnectKwargs.sock and updated fake-connect expectation |
Existing handshake arguments remain correct with sock=None |
Unit / support | keep | Updates the existing argument contract rather than replacing its authentication assertions. |
tests/runner/transports/ws_tunnel/test_serve.py:_capture_connect_kwargs and _StubConnect |
Captures one tunnel attempt without starting a real connection | Support | keep | Small helper shared by the runner selection cases. |
tests/runner/transports/ws_tunnel/test_serve.py:test_serve_tunnel_proxy_socket — no proxy |
Runner avoids the CONNECT dialer and passes no socket | Unit | keep | Verifies absence of dialing, not just the final connection arguments. |
tests/runner/transports/ws_tunnel/test_serve.py:test_serve_tunnel_proxy_socket — configured proxy |
Runner passes the proxy socket and configured timeout | Unit | keep | Protects the runner integration independently of the host. |
tests/runner/transports/ws_tunnel/test_serve.py:test_serve_tunnel_proxy_socket — matching no_proxy |
Runner respects bypass selection | Unit | keep | Confirms the caller actually uses the helper’s bypass result. |
tests/runner/transports/ws_tunnel/test_serve.py:test_serve_tunnel_closes_proxy_socket_when_connect_fails |
Socket closes when WebSocket setup rejects it before adoption | Unit | keep | Direct regression coverage for the runner ownership fix. |
tests/test_ws_proxy.py:test_proxy_selection — scheme-specific variables and ALL_PROXY fallback |
Correct proxy selection for ws and wss |
Unit | keep | Covers distinct environment-selection rules. |
tests/test_ws_proxy.py:test_proxy_selection — lowercase precedence, empty values, and fallback combinations |
Empty lowercase variables suppress uppercase equivalents | Unit | keep | Protects the explicitly reported environment-precedence regressions. |
tests/test_ws_proxy.py:test_proxy_selection — absent configuration, bare proxy address, unsupported schemes, malformed target |
Selection and rejection at configuration boundaries | Unit | keep | These cases exercise different input branches with little setup. |
tests/test_ws_proxy.py:test_proxy_selection — uppercase NO_PROXY and empty lowercase override |
Bypass-variable precedence | Unit | keep | Separately protects precedence for bypass configuration. |
tests/test_ws_proxy.py:test_proxy_selection — localhost, IPv4 loopback, IPv6 loopback |
Local connections are never proxied | Unit | keep | Covers all three relevant loopback representations. |
tests/test_ws_proxy.py:test_no_proxy — exact domain, suffix, boundary mismatch, leading dot, wildcard prefix, * |
Domain matching and global bypass | Unit | keep | Exercises supported matching forms and guards against accidental partial-label matches. |
tests/test_ws_proxy.py:test_no_proxy — matching and mismatching ports |
Port-qualified bypass | Unit | keep | Protects a distinct selection boundary. |
tests/test_ws_proxy.py:test_no_proxy — IPv4, bare IPv6, bracketed IPv6 with port, unrelated target |
Address-form bypass matching | Unit | keep | Add the IP-suffix case to make the selected urllib-versus-HTTPX semantics explicit. |
tests/test_ws_proxy.py:test_redact_proxy_url — with and without userinfo |
Logging removes proxy credentials without changing ordinary URLs | Unit | keep | Small, security-relevant assertions. |
tests/test_ws_proxy.py:_connect_proxy and nested respond |
Captures CONNECT requests and supplies controlled responses | Socket-test support | keep | Shared setup supports framing, authentication, and error cases. |
tests/test_ws_proxy.py:test_connect_tunnel — anonymous |
CONNECT framing, usable tunnel, and cleared socket timeout | Socket integration | keep | Verifies actual byte exchange rather than mocked return values. |
tests/test_ws_proxy.py:test_connect_tunnel — authenticated |
Percent-decoded credentials produce the expected Basic authorization | Socket integration | keep | Protects request encoding at the proxy boundary. |
tests/test_ws_proxy.py:test_connect_error — 407 |
Proxy refusal raises an error | Socket integration | keep | Covers a material authentication/failure response. |
tests/test_ws_proxy.py:test_connect_error — trailing garbage |
Unexpected bytes after CONNECT headers are rejected | Socket integration | keep | Protects the framing boundary before WebSocket or TLS traffic starts. |
tests/test_ws_proxy.py:test_connect_error — EOF |
Incomplete CONNECT response fails | Socket integration | keep | Covers connection loss during negotiation; extend the parameterization with malformed status and oversized-header cases. |
tests/test_ws_proxy.py:_self_signed_cert; _forwarding_proxy, relay, and handle |
Local certificates and a raw tunnel for TLS verification | TLS-test support | keep | Establishes the real proxy-to-TLS boundary without external services. |
tests/test_ws_proxy.py:test_connect_tunnel_wss — matching certificate; nested echo |
Successful verified TLS/WebSocket exchange using the origin authority and SNI | TLS integration | keep | Protects the critical interaction between a preconnected socket and origin-host verification. |
tests/test_ws_proxy.py:test_connect_tunnel_wss — mismatched certificate |
A certificate for another hostname is rejected | TLS integration | keep | Provides the necessary negative verification case. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py:_SITECUSTOMIZE and embedded _getaddrinfo |
Direct origin DNS fails inside the real host process | E2E support | keep | Prevents accidental direct connectivity from making the regression test pass. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py:_mandatory_proxy and Handler.handle |
HTTP setup and CONNECT reach only the expected server | E2E support | keep | Supplies both transport paths required by the real host workflow. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py:test_host_comes_online_through_mandatory_proxy |
Real CLI process becomes online when only the proxy can resolve the origin | E2E | keep | Uniquely checks subprocess environment inheritance, HTTP setup, WebSocket tunneling, and server-observed online state. |
Existing authentication-redirect handling remains intact: HTTP(S) login redirects still raise InvalidURI before the WebSocket library checks the preconnected socket. The focused runner tests are sufficient for its integration; a second full E2E is not required merely to mirror the host scenario.
The concrete coverage additions are the deadline, host SSL-context failure, response-parser boundaries, and explicit IP-suffix semantics described above. None independently demonstrates a blocking bug.
For manual verification, run the host against a server reachable only through the configured CONNECT proxy and confirm it appears online. Then set an unreachable HTTP_PROXY, remove NO_PROXY, and run against a local server at 127.0.0.1; the host should still connect directly.
Scope
All changes support restoring host and runner connectivity in proxy-mandatory environments. No unrelated changes were found. The existing routing headers and affinity paths are unchanged, and no incompatible client/server contract changes were identified.
The database reference was read in full. This diff changes no schema, queries, transactions, or storage paths, so no database requirements apply.
Automated review by Polly · workflow run
Follow-ups from the automated reviews of the proxy support: - The CONNECT timeout is now one budget for the dial and the whole handshake (monotonic deadline per read), so a proxy that drips its reply cannot stretch an attempt across many per-read timeouts. - A non-numeric proxy or tunnel port raises the documented OSError instead of escaping as ValueError; a non-HTTP status line is rejected; the header cap is enforced after each read, including the terminating chunk. - IP-literal no_proxy entries match that address exactly (httpx semantics) instead of also matching hostnames that end with the address. - The host builds its SSL context before opening the proxied socket so a CA-bundle failure cannot leave the socket unowned; the runner constructs websockets.connect() inside the cleanup block for websockets versions that parse the URI in the constructor. - The e2e proxy lets setup errors surface through socketserver, only suppressing OSError in the relay loop, and ignores malformed request lines. Comments trimmed to the three-line policy. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
Dispositions for the automated reviews (OCR run 37402241105 and Polly run 37402239216, both on 7fd323b), implemented in 1d7fe97: OCR summary findings
OCR inline findings — replied individually on each thread: SSL context before the proxy dial, Polly (7fd323b)
|
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
📄
|
| proxy_sock: socket.socket | None = None | ||
| if proxy_url is None: | ||
| _logger.info("Connecting to %s", url) | ||
| else: | ||
| _logger.info("Connecting to %s via CONNECT proxy %s", url, redact_proxy_url(proxy_url)) | ||
| proxy_sock = await open_proxy_connect_socket(proxy_url, url, timeout=open_timeout) |
There was a problem hiding this comment.
The CONNECT dial consumes up to the full open_timeout budget, and websockets.connect(..., open_timeout=open_timeout) then applies the same budget again for TLS + WS upgrade. Behind a slow proxy a single attempt can take ~2× the intended budget (20s initial / 6s reconnect), delaying failure detection in the reconnect loop. Consider deducting the elapsed dial time from the open_timeout passed to connect() so each attempt stays within a single budget.
Suggestion:
| proxy_sock: socket.socket | None = None | |
| if proxy_url is None: | |
| _logger.info("Connecting to %s", url) | |
| else: | |
| _logger.info("Connecting to %s via CONNECT proxy %s", url, redact_proxy_url(proxy_url)) | |
| proxy_sock = await open_proxy_connect_socket(proxy_url, url, timeout=open_timeout) | |
| proxy_sock: socket.socket | None = None | |
| if proxy_url is None: | |
| _logger.info("Connecting to %s", url) | |
| else: | |
| _logger.info("Connecting to %s via CONNECT proxy %s", url, redact_proxy_url(proxy_url)) | |
| dial_started = time.monotonic() | |
| proxy_sock = await open_proxy_connect_socket(proxy_url, url, timeout=open_timeout) | |
| open_timeout = max(0.1, open_timeout - (time.monotonic() - dial_started)) |
There was a problem hiding this comment.
Not changing this one. The two phases are each bounded by open_timeout (the CONNECT phase is now a hard total, including the send, as of 06621b1), so a proxied attempt takes at most twice that budget (20 s initial / 6 s reconnect) before the reconnect loop sees a failure, and only when the proxy is slow and the WebSocket upgrade then also stalls. That is latency on an already-failing path rather than a correctness issue. Deducting the elapsed dial time with a floor would hand the TLS and upgrade handshake a near-zero budget behind a slow proxy and turn one slow attempt into a guaranteed failure plus another reconnect, so a shared deadline needs a floor policy that is worth deciding separately if reconnect latency is observed in practice.
…d httpx - _env() follows urllib's two-pass selection: any capitalisation of the variable counts, a spelling ending in lowercase _proxy wins, and such a spelling that is present but empty suppresses the others. No_Proxy=... now bypasses the proxy as it does for the HTTP clients. - no_proxy port matching drops the scheme's default port first, as httpx does, so example.com:80 matches neither ws://example.com nor ws://example.com:80. - The unsupported-scheme warning names the variable that supplied the value, including the all_proxy fallback. - Add a host test for a connect() constructor failure closing the proxied socket; shorten the call-site comments to the ownership invariant. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
Dispositions for the sixth automated-review round (OCR run 37410857402 and Polly run 37410854822, both on 4b94db9), implemented in 107883c: Polly (4b94db9)
OCR summary (4b94db9)
|
|
🔍 OpenCodeReview found 2 issue(s) in this PR.
📄
|
|
Dispositions for OCR run 37412789111 on 107883c (two low, test-only findings in the e2e proxy harness), both left as is:
|
|
| Category | Files | Added | Deleted | Share of changed text lines |
|---|---|---|---|---|
| Tests and test support | 6 | 772 | 0 | 60.8% |
| Documentation | 0 | 0 | 0 | 0.0% |
| Dependencies and lockfiles | 0 | 0 | 0 | 0.0% |
| Implementation / other | 3 | 473 | 25 | 39.2% |
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements.
Changes
The PR adds a shared proxy-selection and CONNECT utility and integrates it into both WebSocket tunnels. It also adds focused tests, environment-isolation fixtures, local proxy helpers, and a host-process regression scenario. There are no documentation, dependency, lockfile, generated-file, or database changes.
The size is broadly justified by the protocol parsing, authentication, TLS, deadline, cancellation, and socket-ownership boundaries involved. The concrete reduction opportunity is repeated host failure-path setup identified in the notes; no unnecessary production subsystem was found.
Tests
Test-by-test assessment
Assessment covers the frozen snapshot, whose checksum matched the supplied value. Target-revision tests were not executed: the supplied revisions were unavailable locally, and the trusted checkout is not the PR head. Offline library/stdlib comparisons informed the proxy-parity assessment; they are not evidence that the PR test suite passes.
| Test / case | Behavior protected | Layer | Needed? | Action / rationale |
|---|---|---|---|---|
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py — _SITECUSTOMIZE._getaddrinfo |
Origin DNS fails inside the actual host process | E2E helper | keep | Establishes the original regression trigger without mocking the production dialer. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py — _mandatory_proxy, Handler.handle |
Proxy resolves the synthetic origin and handles HTTP plus CONNECT | E2E helper | keep | Host registration and tunnel establishment require both paths. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py — test_host_comes_online_through_mandatory_proxy |
A real host registers and comes online despite failed local origin DNS | E2E | keep | Uniquely verifies subprocess environment, HTTP registration, proxy resolution, and tunnel wiring together. |
tests/host/conftest.py — _no_ambient_proxy_env |
Ambient proxy settings cannot redirect unrelated host tests | Fixture | keep | Directory-scoped isolation avoids environment-dependent failures. |
tests/host/test_connect.py — test_connect_and_serve_proxy_socket[False] |
Direct mode avoids CONNECT and supplies no preconnected socket | Unit | keep | Protects unchanged behavior without proxy configuration. |
tests/host/test_connect.py — test_connect_and_serve_proxy_socket[True] |
Host forwards the proxy socket and timeout, then closes on context-entry failure | Unit | keep | Covers host-specific wiring and ownership transfer. |
tests/host/test_connect.py — test_connect_and_serve_builds_ssl_context_before_dialing_proxy |
CA-context failure occurs before opening a proxy socket | Unit | keep | Protects a distinct resource-leak boundary. |
tests/host/test_connect.py — test_connect_and_serve_closes_proxy_socket_when_connect_constructor_fails |
Constructor failure closes the proxy socket | Unit | consolidate | Add a failure-stage axis to test_connect_and_serve_proxy_socket; retain this assertion. |
tests/runner/transports/ws_tunnel/conftest.py — _no_ambient_proxy_env |
Ambient proxy settings cannot redirect runner tunnel tests | Fixture | keep | Preserves scoped, deterministic test behavior. |
tests/runner/transports/ws_tunnel/test_serve.py — _ConnectKwargs.sock |
Captured connection arguments represent the new socket argument | Test helper | keep | Necessary support for existing argument assertions; not a separate test case. |
tests/runner/transports/ws_tunnel/test_serve.py — modified test_serve_tunnel_once_sends_bearer_header |
Existing handshake arguments remain correct with sock=None |
Unit | keep | Updates existing authentication and routing coverage for the unchanged direct path. |
tests/runner/transports/ws_tunnel/test_serve.py — test_serve_tunnel_proxy_socket, no-proxy case |
Runner does not invoke CONNECT without proxy configuration | Unit | keep | Protects direct-path wiring. |
tests/runner/transports/ws_tunnel/test_serve.py — test_serve_tunnel_proxy_socket, configured-proxy case |
Runner invokes CONNECT and forwards its socket | Unit | keep | Protects the runner’s integration with the shared helper. |
tests/runner/transports/ws_tunnel/test_serve.py — test_serve_tunnel_proxy_socket, NO_PROXY case |
Bypass selection suppresses CONNECT at the runner call site | Unit | keep | A small additional case verifies selector-to-caller integration. |
tests/runner/transports/ws_tunnel/test_serve.py — test_serve_tunnel_closes_proxy_socket_when_connect_fails[constructor,enter] |
Both pre-adoption failures close the socket | Unit | keep | Distinct ownership boundaries share useful setup. |
tests/test_ws_proxy.py — test_proxy_selection, scheme/fallback/parsing cases |
WS/WSS select the corresponding variable, use fallback, and handle invalid or unsupported proxy URLs | Unit | keep | Covers distinct selector and parsing boundaries. |
tests/test_ws_proxy.py — test_proxy_selection, case/empty-variable cases |
Case precedence and empty lowercase overrides | Unit | keep | Protects explicit environment contracts; the CGI-style discrepancy needs the clarification described above. |
tests/test_ws_proxy.py — test_proxy_selection, NO_PROXY precedence cases |
Correct bypass variable is selected | Unit | keep | Separates environment precedence from hostname matching. |
tests/test_ws_proxy.py — test_proxy_selection, loopback cases |
Localhost and loopback addresses remain direct | Unit | keep | Verifies integration with the existing loopback exemption. |
tests/test_ws_proxy.py — test_no_proxy, plain-domain/leading-dot cases |
Apex, subdomain, and suffix-lookalike boundaries | Unit | keep | Distinguishes the documented domain-matching rules. |
tests/test_ws_proxy.py — test_no_proxy, wildcard cases |
Lone * bypasses all; other *-prefixed entries do not match |
Unit | keep | Protects non-obvious compatibility behavior. |
tests/test_ws_proxy.py — test_no_proxy, port cases |
Port-specific matching and default-port normalization | Unit | keep | Covers both explicit and omitted default-port boundaries. |
tests/test_ws_proxy.py — test_no_proxy, IP/IPv6/exact-text cases |
IP entries do not accidentally match hostname suffixes | Unit | keep | Protects bypass boundaries; add the IP-with-port exception noted above. |
tests/test_ws_proxy.py — test_redact_proxy_url |
Credentials and unrelated URL components are omitted from diagnostics | Unit | keep | Direct protection against credential exposure. |
tests/test_ws_proxy.py — test_unsupported_scheme_warns_once_without_credentials |
Unsupported schemes warn once without exposing credentials | Unit | keep | Covers fallback diagnostics and warning deduplication. |
tests/test_ws_proxy.py — _connect_proxy, respond |
Deterministic CONNECT requests and scripted replies | Socket-test helper | keep | Shared setup supports meaningful protocol assertions. |
tests/test_ws_proxy.py — test_connect_tunnel, anonymous/basic-auth cases |
Correct CONNECT authority, headers, decoded credentials, and usable returned socket | Socket integration | keep | Verifies actual request bytes and socket behavior. |
tests/test_ws_proxy.py — test_connect_error |
Refusal, malformed status, oversized headers, trailing bytes, and premature EOF are rejected | Socket integration | keep | Distinct material failures are efficiently parameterized. |
tests/test_ws_proxy.py — test_connect_tunnel_idna_host |
CONNECT uses an ASCII authority without resolving the origin locally | Socket integration | keep | Protects hostname encoding and proxy-side resolution. |
tests/test_ws_proxy.py — test_connect_rejects_malformed_proxy_url |
Invalid ports and IPv6 syntax become the documented error type | Unit/async boundary | keep | Verifies failure classification before dialing. |
tests/test_ws_proxy.py — test_connect_closes_socket_when_awaiter_is_cancelled, slow_connect |
A result arriving after cancellation is closed | Async unit | keep | Deterministically exercises the orphaned-socket race. |
tests/test_ws_proxy.py — test_connect_timeout_bounds_a_slow_dial, slow_dial |
The total budget includes dialing and late results are closed | Async unit | keep | Protects the overall deadline rather than individual socket operations. |
tests/test_ws_proxy.py — test_connect_timeout_race_closes_completed_dial, wait_for_after_completion |
Simultaneous completion and timeout cannot leak a socket | Async unit | keep | The deterministic race test is justified; extend its diagnostic assertion as noted above. |
tests/test_ws_proxy.py — test_connect_timeout_bounds_the_whole_handshake, drip |
Dripped response bytes cannot extend the total deadline | Socket integration | keep | Real I/O exercises the cumulative-budget boundary. |
tests/test_ws_proxy.py — _self_signed_cert |
Certificates support success and hostname-mismatch scenarios | TLS helper | keep | Generates local test material without external certificate services. |
tests/test_ws_proxy.py — _forwarding_proxy, relay, handle |
TLS bytes traverse CONNECT without proxy termination | TLS helper | keep | Necessary to test end-to-end origin verification. |
tests/test_ws_proxy.py — test_connect_tunnel_wss, matching-name case |
TLS-over-CONNECT uses the origin hostname for SNI and verification | TLS/WebSocket integration | keep | Establishes the security-critical successful path. |
tests/test_ws_proxy.py — test_connect_tunnel_wss, mismatched-name case |
An incorrect certificate hostname is rejected | TLS/WebSocket integration | keep | Proves proxying does not disable hostname validation. |
Existing host upgrade-rejection and reconnect-classification tests cover surrounding failure handling. Existing runner handshake tests cover authentication, routing, and direct connections; tests/frontends/sdk/test_http.py already covers broader loopback forms. These complement the new tests without requiring another runner E2E.
The concrete coverage improvements are the parity exceptions and timeout-message assertion identified in Non-blocking notes. No additional missing coverage establishes a correctness blocker.
For human verification, start a host against a non-loopback server in an environment where only the HTTP proxy can resolve or reach that server. Confirm the host becomes online and the proxy records CONNECT. Then verify a normally reachable server still connects with proxy variables unset. Do not include credentials in captured logs.
Scope
All changes support the stated outcome: making host and runner WebSocket tunnels work through environment-configured HTTP CONNECT proxies. No unrelated changes were found. Existing routing headers, affinity handling, endpoints, and wire formats remain unchanged.
The full database reference was read and applied. No changed schema, query, transaction, or storage code engages its mandatory rules.
Automated review by Polly · workflow run
- _env() ignores HTTP_PROXY when REQUEST_METHOD marks a CGI environment, as urllib does; a lowercase-suffixed http_proxy still applies. - The completion-race branch raises the descriptive deadline TimeoutError instead of wait_for's message-less one, so the runner's retry reason stays meaningful; a worker-raised error keeps its own message. - Document that IP-literal no_proxy entries stay exact even with a port (httpx suffix-matches those) and cover it; fold the host constructor failure into the parametrized proxy-socket test. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
Dispositions for Polly run 37412787239 on 107883c (no blocking issues), implemented in 7f90bff:
|
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
📄
|
|
Disposition for OCR run 37414507230 on 7f90bff (one low maintainability finding), left as is:
|
|
| Tunnel URL | NO_PROXY |
Corresponding HTTP request | New tunnel behavior |
|---|---|---|---|
ws://example.com/t |
http://example.com |
Direct | Proxied |
wss://example.com/t |
https://example.com |
Direct | Proxied |
| Either scheme | all://example.com |
Direct | Proxied |
For example, ws_env_proxy_url("wss://example.com/t", {"https_proxy": "http://p:1", "no_proxy": "https://example.com"}) returns the proxy instead of bypassing it.
This breaks the stated HTTP/WebSocket bypass-parity contract and changes previously direct tunnel connections into proxied connections despite an explicit bypass. If the proxy cannot reach that destination, HTTP registration succeeds while the tunnel fails again. Handle URL-form entries with the corresponding HTTP scheme, host, and port semantics. Extend tests/test_ws_proxy.py::test_no_proxy with these examples and a scheme-mismatch case that must remain proxied.
Security vulnerabilities
No additional confirmed vulnerabilities were found. The bypass-selection defect above can send traffic through a proxy that the configuration explicitly excludes; it is reported once under Blocking issues.
Non-blocking notes
- Qualify the parity claim.
omnigent/util/ws_proxy.py:89promises that HTTP requests and tunnels never disagree, buttests/test_ws_proxy.py:105intentionally enforces stricter IP-with-port matching than httpx. Document that exception rather than weakening the safer matching behavior. - Trim redundant direct-path cases. In
tests/host/test_connect.py::test_connect_and_serve_proxy_socket, consolidate the twouse_proxy=Falsefailure-phase cases into one; constructor-versus-entry cleanup matters only when a proxy socket exists. Intests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket, the(False, "")case duplicates the directsock=Noneassertion intest_serve_tunnel_once_sends_bearer_header.
Approach
The shared CONNECT utility is appropriate while the existing websockets<15 pin remains necessary. It centralizes proxy selection, authentication, deadlines, and socket ownership without changing the server protocol. Upgrading to native proxy support would be simpler, but the pin documents a macOS hang, so that is not a safe shortcut for this PR.
Summary
Diff size (computed): 9 files; +1235 / -25 lines (1260 changed text lines); 0 binary files.
| Category | Files | Added | Deleted | Share of changed text lines |
|---|---|---|---|---|
| Tests and test support | 6 | 756 | 0 | 60.0% |
| Documentation | 0 | 0 | 0 | 0.0% |
| Dependencies and lockfiles | 0 | 0 | 0 | 0.0% |
| Implementation / other | 3 | 479 | 25 | 40.0% |
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements.
Changes
The PR adds a shared HTTP CONNECT implementation and integrates it into both host and runner WebSocket tunnels. The remaining changes are tests and supporting fixtures; there are no documentation, dependency, lockfile, schema, or generated-file changes.
The implementation size is broadly justified by environment-variable handling, bypass matching, authenticated CONNECT, bounded response parsing, cancellation, and socket cleanup. No substantial implementation block appears unrelated or removable without losing behavior. The concrete reduction opportunities are redundant parameter cases identified below.
No client/server wire contract or routing-affinity change was found. The database reference was read in full; no changed schema, query, transaction, or storage code makes its requirements applicable.
Tests
Test-by-test assessment
The assessment covers all six changed test/support files. Parameterized cases are grouped where their purpose and recommendation match.
| Test / case | Behavior protected | Layer | Needed? | Action / rationale |
|---|---|---|---|---|
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py — _SITECUSTOMIZE, _getaddrinfo |
Makes the origin hostname unresolvable inside the real host process | E2E support | keep | Establishes the regression trigger without disabling proxy resolution. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py — _mandatory_proxy, Handler.handle |
Relays HTTP and CONNECT to the test server and records requests | E2E fixture | keep | Supplies the mandatory-egress boundary and evidence that both transports use it. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py::test_host_comes_online_through_mandatory_proxy |
A real host registers and becomes online without direct origin DNS | E2E | keep | Uniquely exercises subprocess environment inheritance, registration, DNS failure, and tunnel wiring together. |
tests/host/conftest.py::_no_ambient_proxy_env |
Isolates host tests from machine-specific proxy settings | Test fixture | keep | Appropriately scoped now that connection code consumes these variables. |
tests/host/test_connect.py::test_connect_and_serve_proxy_socket — proxied constructor/entry failures |
Passes the CONNECT socket and closes it at both pre-adoption failure boundaries | Unit | keep | The two ownership failure phases are distinct. |
tests/host/test_connect.py::test_connect_and_serve_proxy_socket — direct constructor/entry failures |
Does not dial a proxy and passes sock=None |
Unit | consolidate | One direct case preserves this assertion; the failure-phase cross-product adds no proxy-resource coverage. |
tests/host/test_connect.py::test_connect_and_serve_builds_ssl_context_before_dialing_proxy |
CA-bundle failure happens before allocating a proxy socket | Unit | keep | Protects a material resource-ownership ordering constraint. |
tests/runner/transports/ws_tunnel/conftest.py::_no_ambient_proxy_env |
Isolates runner tests from ambient proxy settings | Test fixture | keep | Scoped isolation avoids changing unrelated suites. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_once_sends_bearer_header — _ConnectKwargs.sock and expected kwargs |
Preserves the existing direct runner connection contract | Unit/support | keep | Extends an existing exact-kwargs assertion with sock=None. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — (True, "") |
Dials the selected proxy and forwards its socket | Unit | keep | Protects runner integration with the shared utility. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — (False, "") |
Uses sock=None without proxy configuration |
Unit | remove | Duplicates the modified existing direct-connection assertion. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — (True, "server.sandbox.test") |
Applies NO_PROXY at the runner call site |
Unit | keep | Checks that runner wiring actually consumes the selector’s bypass result. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_closes_proxy_socket_when_connect_fails — constructor/entry failures |
Closes an unadopted socket in both failure phases | Unit | keep | Protects distinct leak paths, including older client constructors. |
tests/test_ws_proxy.py — _TLS_HOST, _TUNNEL_URL, _OK |
Shared origin and CONNECT-response expectations | Test support | keep | Small constants used across protocol assertions. |
tests/test_ws_proxy.py::test_proxy_selection — scheme selection, fallback, absent configuration |
Chooses HTTP/HTTPS/ALL proxy variables appropriately | Unit | keep | Covers the core environment-selection contract. |
tests/test_ws_proxy.py::test_proxy_selection — case precedence, empty values, mixed case, CGI, NO_PROXY precedence |
Matches urllib’s environment-variable interpretation | Unit | keep | Exercises distinct precedence and suppression branches. |
tests/test_ws_proxy.py::test_proxy_selection — bare proxy address, unsupported schemes, malformed URLs |
Handles unsupported or unusable configuration without leaking credentials | Unit | keep | Protects normalization and configuration-error boundaries. |
tests/test_ws_proxy.py::test_proxy_selection — localhost, IPv4 loopback, IPv6 loopback |
Never proxies loopback destinations | Unit | keep | Covers each supported loopback representation. |
tests/test_ws_proxy.py::test_no_proxy — names, suffixes, leading dots, wildcard forms |
Applies hostname bypass rules without unintended suffix matches | Unit | keep | Meaningful matching boundaries; extend with URL-form entries to cover the demonstrated bug. |
tests/test_ws_proxy.py::test_no_proxy — explicit, mismatched, and default ports |
Applies port-sensitive bypass semantics | Unit | keep | Protects subtle default-port normalization behavior. |
tests/test_ws_proxy.py::test_no_proxy — IPv4/IPv6, bracket forms, textual variants, hostile suffixes |
Matches literal addresses without unsafe suffix bypass | Unit | keep | Protects address boundaries; retain stricter behavior and document its httpx exception. |
tests/test_ws_proxy.py::test_redact_proxy_url — with/without userinfo |
Removes credentials and other unnecessary URL components from logs | Unit | keep | Direct assertion of credential redaction. |
tests/test_ws_proxy.py::test_unsupported_scheme_warns_once_without_credentials — http_proxy, ALL_PROXY |
Warns once without exposing authentication material | Unit | keep | Covers both primary and fallback proxy settings. |
tests/test_ws_proxy.py — _connect_proxy, nested respond |
Captures CONNECT requests and provides controlled replies/echoing | Integration helper | keep | Reused across success and parser-failure cases. |
tests/test_ws_proxy.py::test_connect_tunnel — authenticated/unauthenticated |
Sends the correct authority, Host header, decoded Basic auth, and returns a usable socket | Integration | keep | Real socket assertions cover both authentication paths and timeout reset. |
tests/test_ws_proxy.py::test_connect_error[refused] |
Reports CONNECT rejection | Integration | keep | Covers the material proxy-authentication/refusal boundary. |
tests/test_ws_proxy.py::test_connect_error[non-http], test_connect_error[non-decimal-status] |
Rejects malformed status lines and codes | Integration | keep | Distinct parser failures share a compact parameterized test. |
tests/test_ws_proxy.py::test_connect_error[oversized] |
Bounds response-header memory use | Integration | keep | Tests the enforced header-size limit. |
tests/test_ws_proxy.py::test_connect_error[trailing-bytes], test_connect_error[eof] |
Rejects stream residue and premature closure | Integration | keep | Covers unusable tunnel handoff and truncated handshakes. |
tests/test_ws_proxy.py::test_connect_tunnel_idna_host |
Encodes Unicode origin names in CONNECT authority | Integration | keep | Distinct authority-encoding behavior. |
tests/test_ws_proxy.py::test_connect_rejects_malformed_proxy_url — invalid port/brackets |
Rejects malformed proxy configuration before dialing | Unit | keep | Covers separate URL-parser and lazy-port-validation failures. |
tests/test_ws_proxy.py::test_connect_closes_socket_when_awaiter_is_cancelled |
Closes a socket returned after cancellation | Async unit | keep | Protects cleanup of non-interruptible background work. |
tests/test_ws_proxy.py::test_connect_timeout_bounds_a_slow_dial |
Bounds caller wait time and closes late results | Async unit | keep | Covers the dial phase rather than only handshake reads. |
tests/test_ws_proxy.py::test_connect_timeout_race_closes_completed_dial |
Prevents a leak when completion races the deadline | Async unit | keep | Deterministically targets a distinct ownership race. |
tests/test_ws_proxy.py::test_connect_timeout_bounds_the_whole_handshake |
Prevents drip-fed replies from extending the total deadline | Integration | keep | Real socket behavior establishes the whole-handshake budget. |
tests/test_ws_proxy.py — _self_signed_cert, _forwarding_proxy, nested relay/handle |
Supplies hostname-specific certificates and a raw TLS relay | Integration helpers | keep | Required to test origin verification through CONNECT locally. |
tests/test_ws_proxy.py::test_connect_tunnel_wss — matching certificate |
Preserves origin SNI, certificate verification, and WebSocket frame transfer | TLS integration | keep | Establishes real sock= adoption rather than relying on mocks. |
tests/test_ws_proxy.py::test_connect_tunnel_wss — wrong-host certificate |
Rejects a certificate for another origin | TLS integration | keep | Detects disabled or incorrectly targeted hostname verification. |
Coverage: Existing connection tests plus the new utility and call-site tests cover the principal success and cleanup paths. The concrete missing coverage is scheme-qualified NO_PROXY matching and scheme mismatch, addressed by the blocking finding. A second full runner subprocess E2E is not required merely to duplicate the shared CONNECT/TLS assertions.
Execution: The frozen snapshot checksum was verified. The patch applied cleanly to an isolated copy of the trusted checkout. Byte-compilation, repository-local lint scripts, and standalone offline probes covered CONNECT/authentication, malformed replies, timeout/cancellation cleanup, TLS/SNI, and the httpx comparison. These are not full-suite results: pytest and the standard Ruff/typecheck gates were unavailable, and neither referenced commit object was present locally.
After the fix, run python -m pytest -q tests/test_ws_proxy.py tests/host/test_connect.py tests/runner/transports/ws_tunnel/test_serve.py in the configured test environment. For human verification, use a proxy-only environment where direct origin DNS fails, start the host, and confirm it reaches online through CONNECT. Repeat with a reachable destination explicitly excluded by a scheme-qualified NO_PROXY entry; both HTTP and WebSocket traffic should bypass the proxy.
Scope
All changed files support the stated host/runner proxy-connectivity outcome. No clearly unrelated changes were found. Socket cleanup, timeout handling, TLS verification, and scoped test-environment isolation are necessary supporting work rather than independent features.
Automated review by Polly · workflow run
- A no_proxy entry such as https://example.com or all://*.example.com is matched by scheme (all, or the HTTP scheme the tunnel scheme maps to), httpx host-pattern rules and default-port-normalised port, so an explicit bypass that works for the HTTP client also keeps the tunnel direct. - State the one deliberate parity exception (IP literal entries with a port stay exact) in the matcher docstring. - Drop the redundant direct/constructor host test case. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
- A URL-form entry without a port matches any target port, as an httpx mount does; the port is only compared when the pattern names one. - A wildcard-host entry (all://*, http://*) bypasses nothing unless it is port-qualified, since only then does it outrank httpx's proxy mounts. Both rules were cross-checked against httpx.Client(trust_env=True). Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
Dispositions for Polly run 37414505356 on 7f90bff, implemented in afa4b3c and 4d944d2:
|
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
📄
|
|
Disposition for OCR run 37416508542 on 4d944d2 (one low, test-only finding), left as is:
|
|
| Environment | HTTP behavior | New tunnel behavior |
|---|---|---|
all_proxy=http://p:1, no_proxy=http://* |
http://example.com connects directly |
ws://example.com uses the proxy |
all_proxy=http://p:1, no_proxy=https://* |
https://example.com connects directly |
wss://example.com uses the proxy |
A local reproduction with a reachable non-loopback origin and an unavailable proxy confirmed that HTTP succeeds while the tunnel receives ConnectionRefusedError. Both tunnel callers use the selector’s result, so reconnecting does not resolve the mismatch.
This breaks the supported URL-form bypass contract documented at omnigent/util/ws_proxy.py:97. It is separate from the explicitly documented stricter matching of IP literals with ports.
Requested change: Preserve whether selection used the all_proxy fallback and account for that source when evaluating scheme-wildcard bypass entries. Extend tests/test_ws_proxy.py::test_proxy_selection with both failing configurations and retain the neighboring scheme-specific-proxy and all://* cases.
Security vulnerabilities
No additional security vulnerabilities found. Credential redaction and destination-host TLS verification remain intact. The stricter IP-literal bypass behavior is an explicitly documented exception, not a finding.
Non-blocking notes
- Complete environment isolation in the fixtures.
tests/host/conftest.py:7andtests/runner/transports/ws_tunnel/conftest.py:7remove only lowercase and uppercase proxy-variable spellings, but production also accepts mixed-case forms such asHttp_Proxy. Remove existing keys using case-insensitive comparison. The duplicate cleanup logic can share a helper while retaining its current suite scope.
Approach
The shared CONNECT helper is appropriate for the pinned WebSocket client and avoids separate host and runner implementations. The main consistency risk is reproducing proxy-selection precedence manually; selection tests should cover both the bypass pattern and the proxy variable supplying the connection.
Summary
Diff size (computed): 9 files; +1304 / -25 lines (1329 changed text lines); 0 binary files.
| Category | Files | Added | Deleted | Share of changed text lines |
|---|---|---|---|---|
| Tests and test support | 6 | 781 | 0 | 58.8% |
| Documentation | 0 | 0 | 0 | 0.0% |
| Dependencies and lockfiles | 0 | 0 | 0 | 0.0% |
| Implementation / other | 3 | 523 | 25 | 41.2% |
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements.
Changes
The PR adds a shared environment-proxy selector and HTTP CONNECT dialer, then integrates the resulting socket into host and runner tunnels. It also adds environment-isolation fixtures and coverage for selection, authentication, TLS, cancellation, ownership, and proxy-mandatory connectivity. No documentation, dependency, lockfile, or generated-file changes are included.
The size is broadly justified by the supported selection rules and resource-lifecycle boundaries. The duplicated environment-cleanup fixtures are a concrete consolidation opportunity; no broad implementation reduction is necessary.
Tests
Test-by-test assessment
The snapshot checksum was verified. Focused local reproductions exercised transport behavior and confirmed the blocking selection mismatch. The normal pytest suite, E2E suite, and lint/typecheck gates were not run because required tooling or dependencies were unavailable offline; these are not reported as passing.
| Test / case | Behavior protected | Layer | Needed? | Action / rationale |
|---|---|---|---|---|
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py::test_host_comes_online_through_mandatory_proxy |
A real host subprocess becomes online when only the proxy can resolve the server | E2E | keep | Uniquely covers CLI/configuration loading, HTTP traffic, CONNECT wiring, and host registration together. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py — _SITECUSTOMIZE._getaddrinfo, _mandatory_proxy, nested Handler.handle |
Enforces direct-DNS failure and provides forwarding HTTP/CONNECT infrastructure | Test helpers | keep | Necessary to make the mandatory-proxy regression meaningful; these are helpers, not additional cases. |
tests/host/conftest.py::_no_ambient_proxy_env |
Isolates host tests from inherited proxy settings | Fixture | consolidate | Share cleanup logic with the runner fixture and remove mixed-case spellings without broadening fixture scope. |
tests/host/test_connect.py::test_connect_and_serve_proxy_socket — (False, "enter") |
Direct connections avoid the proxy dialer and pass sock=None |
Unit | keep | Protects unchanged behavior without proxy configuration. |
tests/host/test_connect.py::test_connect_and_serve_proxy_socket — (True, "constructor"), (True, "enter") |
Transfers the proxy socket and closes it when WebSocket acquisition fails | Unit | keep | Constructor and context-entry failures are distinct ownership boundaries. |
tests/host/test_connect.py::test_connect_and_serve_builds_ssl_context_before_dialing_proxy |
CA setup failure occurs before proxy-socket acquisition | Unit | keep | Directly protects against an unowned socket on local SSL setup failure. |
tests/runner/transports/ws_tunnel/conftest.py::_no_ambient_proxy_env |
Isolates runner tests from inherited proxy settings | Fixture | consolidate | Same cleanup change as the host fixture; preserve suite-local application. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_once_sends_bearer_header — including modified _ConnectKwargs and _fake_connect |
Preserves authentication arguments and adds the direct-path sock=None assertion |
Unit / test support | keep | Extends existing coverage rather than introducing a duplicate test. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — (False, "") |
Unconfigured runner avoids proxy dialing | Unit | keep | Covers the runner entry point independently of the shared selector. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — (True, "") |
Runner supplies the CONNECT socket and expected timeout | Unit | keep | Protects runner-specific integration. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — (True, "server.sandbox.test") |
Runner honors an environment bypass | Unit | keep | Checks bypass wiring rather than duplicating matcher internals. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_closes_proxy_socket_when_connect_fails — constructor, enter |
Closes the runner’s socket at both acquisition-failure boundaries | Unit | keep | Both failure locations require cleanup. |
tests/test_ws_proxy.py::test_proxy_selection — scheme mapping, ALL_PROXY, empty environment, bare p:1, wrong-scheme variable |
Selects the appropriate proxy or direct route | Unit | keep | Extend this selection table with the two source-sensitive wildcard regressions described in Blocking issues. |
tests/test_ws_proxy.py::test_proxy_selection — unsupported schemes and malformed target/proxy URLs |
Handles unsupported and malformed configuration | Unit | keep | Exercises distinct parsing and supported-scheme boundaries. |
tests/test_ws_proxy.py::test_proxy_selection — capitalization, empty values, fallback, and REQUEST_METHOD |
Matches environment-variable precedence and CGI handling | Unit | keep | Protects compatibility with urllib-style environment interpretation. |
tests/test_ws_proxy.py::test_proxy_selection — URL-form bypass entries |
Maps HTTP/HTTPS bypass schemes to WS/WSS | Unit | keep | Distinct from plain hostname matching. |
tests/test_ws_proxy.py::test_proxy_selection — localhost, 127.0.0.1, ::1 |
Never proxies loopback targets | Unit | keep | Protects local-host operation when ambient proxy variables exist. |
tests/test_ws_proxy.py::test_no_proxy — domain, subdomain, leading-dot, *.example.com, and global * cases |
Applies hostname and wildcard bypass rules | Unit | keep | Positive and negative cases distinguish the supported matching forms. |
tests/test_ws_proxy.py::test_no_proxy — plain hostname/port cases |
Honors explicit ports and normalized default ports | Unit | keep | Covers boundaries that hostname-only cases cannot establish. |
tests/test_ws_proxy.py::test_no_proxy — URL-form scheme, wildcard-host, and port cases |
Applies URL-pattern matching | Unit | keep | Useful matcher coverage, but source-sensitive mount precedence also requires selector-level cases. |
tests/test_ws_proxy.py::test_no_proxy — IP/IPv6, localhost lists, and evil.10.1.2.3 negatives, with and without ports |
Matches IP text exactly and avoids suffix false positives | Unit | keep | Preserves the documented security-motivated exception to httpx behavior. |
tests/test_ws_proxy.py::test_redact_proxy_url — empty and populated userinfo |
Removes credentials and other sensitive URL components from log values | Unit | keep | Directly asserts the redaction boundary. |
tests/test_ws_proxy.py::test_unsupported_scheme_warns_once_without_credentials — http_proxy, ALL_PROXY |
Deduplicates warnings without exposing passwords | Unit | keep | Covers both selection sources and emitted log content. |
tests/test_ws_proxy.py::_connect_proxy and nested respond |
Captures CONNECT requests and supplies controlled responses | Test helper | keep | Shared infrastructure for transport and response-parser assertions. |
tests/test_ws_proxy.py::test_connect_tunnel — unauthenticated and percent-encoded credentials |
Formats CONNECT, sends decoded Basic authentication, and transports bytes | Socket integration | keep | The two cases protect distinct authentication behavior using real I/O. |
tests/test_ws_proxy.py::test_connect_error — refused, non-http, non-decimal-status, oversized, trailing-bytes, eof |
Rejects unsuccessful or malformed CONNECT responses | Socket integration | keep | Each case exercises a distinct response-validation boundary. |
tests/test_ws_proxy.py::test_connect_tunnel_idna_host |
Encodes the destination authority using IDNA | Socket integration | keep | Checks the actual CONNECT request rather than a mocked call. |
tests/test_ws_proxy.py::test_connect_rejects_malformed_proxy_url — invalid port, malformed IPv6 |
Produces controlled failures for invalid proxy authorities | Unit | keep | Covers lazy port validation and URL parse failure separately. |
tests/test_ws_proxy.py::test_connect_closes_socket_when_awaiter_is_cancelled |
Closes a socket returned after cancellation | Concurrency unit | keep | Protects the late-result ownership boundary. |
tests/test_ws_proxy.py::test_connect_timeout_bounds_a_slow_dial |
Bounds the caller’s wait and cleans up a late socket | Concurrency unit | keep | Tests deadline behavior separately from response parsing. |
tests/test_ws_proxy.py::test_connect_timeout_race_closes_completed_dial |
Closes a socket when completion races with timeout | Concurrency unit | keep | Covers a distinct cleanup race. |
tests/test_ws_proxy.py::test_connect_timeout_bounds_the_whole_handshake |
Prevents drip-fed response headers from extending the caller’s deadline | Socket integration | keep | Real I/O validates the aggregate handshake budget. |
tests/test_ws_proxy.py::_self_signed_cert, _forwarding_proxy, nested relay and handle |
Supplies TLS identities and a raw CONNECT relay | Test helpers | keep | Necessary infrastructure for the WSS verification cases. |
tests/test_ws_proxy.py::test_connect_tunnel_wss — server.sandbox.test |
Verifies TLS hostname/SNI and exchanges WebSocket frames through CONNECT | Protocol integration | keep | Establishes the secure transport boundary that mocked callers cannot prove. |
tests/test_ws_proxy.py::test_connect_tunnel_wss — other.sandbox.test |
Rejects a certificate for the wrong destination hostname | Protocol integration | keep | Provides the security-negative counterpart to successful WSS tunneling. |
No tests were removed. Existing entry-point coverage plus the new shared-helper tests provides appropriate layering; a second runner E2E test is not necessary for the same transport assertions. The concrete missing coverage is proxy-source-dependent wildcard bypass selection.
After fixing it, run:
python -m pytest -q tests/test_ws_proxy.py tests/host/test_connect.py tests/runner/transports/ws_tunnel/test_serve.py
python -m pytest -q tests/e2e/test_host_tunnel_mandatory_proxy_e2e.pyFor manual verification, start a host in the affected proxy-mandatory environment and confirm that it becomes online without repeated tunnel-resolution failures.
Scope
All changed files support restoring host and runner connectivity through environment-configured proxies. No unrelated changes were found. Tunnel URLs, authentication contracts, and routing identifiers remain unchanged.
The database reference was read in full; no schema, query, transaction, or storage changes invoke its requirements. No dependency pins or optional extras changed.
Automated review by Polly · workflow run
- Selection records whether the all_proxy fallback supplied the proxy and the bypass matcher accounts for it: http://* or https://* bypasses when the proxy is httpx's scheme-less all:// mount, still not when a scheme-specific proxy variable is set, and all://* bypasses nothing. - The host and runner test conftests strip proxy variables of any capitalisation, matching what the selector honors. Cross-checked against httpx.Client(trust_env=True) under scheme-specific, all_proxy-only and mixed proxy environments. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
Dispositions for Polly run 37416506530 and OCR run 37416508542 on 4d944d2, implemented in 8689fd5: Polly (4d944d2)
OCR (4d944d2) — header |
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
📄
|
|
| Category | Files | Added | Deleted | Share of changed text lines |
|---|---|---|---|---|
| Tests and test support | 6 | 798 | 0 | 58.7% |
| Documentation | 0 | 0 | 0 | 0.0% |
| Dependencies and lockfiles | 0 | 0 | 0 | 0.0% |
| Implementation / other | 3 | 537 | 25 | 41.3% |
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements.
Changes
The PR adds shared authenticated HTTP CONNECT support and integrates it into both WebSocket tunnels. It handles proxy selection, bypass rules, redacted diagnostics, total connection deadlines, response limits, and socket cleanup.
The remaining changes are tests and supporting fixtures; there are no documentation, dependency, lockfile, or generated-file changes. The size is broadly justified by the compatibility rules and asynchronous failure paths. The concrete reduction opportunities are duplicated fixture implementation and overlapping direct-connection assertions, rather than the number of files or tests alone.
Tests
Test-by-test assessment
The assessment covers the frozen snapshot and relevant existing tests. No pytest cases were executed because pytest was unavailable; this is not a passing-suite claim. The requested commit objects were unavailable locally, so review used the supplied, checksum-verified diff rather than a checked-out PR head.
| Test / case | Behavior protected | Layer | Needed? | Action / rationale |
|---|---|---|---|---|
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py:27 — _SITECUSTOMIZE._getaddrinfo |
Makes origin DNS fail in the host process while permitting proxy resolution. | E2E helper | keep | Establishes the actual regression condition without changing system DNS. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py:42 — _mandatory_proxy, including handler and relay |
Forwards HTTP and CONNECT traffic for the synthetic origin and records proxy use. | E2E fixture | keep | Supplies the mandatory-egress boundary needed by the real-process test. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py:117 — test_host_comes_online_through_mandatory_proxy |
A real host completes HTTP setup, establishes its tunnel through CONNECT, and becomes online. | E2E | keep | Uniquely verifies CLI/process, HTTP, WebSocket, and server-registration wiring; utility tests cannot establish this. |
tests/host/conftest.py:11 — _no_ambient_proxy_env |
Prevents inherited proxy variables from changing host tests. | Suite fixture | consolidate | Share implementation with the runner fixture while retaining scoped activation. |
tests/host/test_connect.py:8285 — test_connect_and_serve_proxy_socket, direct parameter |
Does not dial a proxy and supplies no socket without proxy configuration. | Component | keep | Protects the host’s unchanged direct-connection path within the existing parameterized test. |
tests/host/test_connect.py:8285 — test_connect_and_serve_proxy_socket, constructor-failure parameter |
Passes the proxy socket and closes it if the WebSocket constructor raises. | Component | keep | Covers ownership before an async context manager exists. |
tests/host/test_connect.py:8285 — test_connect_and_serve_proxy_socket, context-entry-failure parameter |
Closes the proxy socket when connection/upgrade entry fails. | Component | keep | Covers a distinct failure boundary after constructor success. |
tests/host/test_connect.py:8324 — test_connect_and_serve_builds_ssl_context_before_dialing_proxy |
A CA/context failure occurs before allocating a proxy socket. | Component | keep | Directly protects ordering that prevents an unowned socket. |
tests/runner/transports/ws_tunnel/conftest.py:11 — _no_ambient_proxy_env |
Isolates runner tunnel tests from ambient proxy configuration. | Suite fixture | consolidate | Shares identical behavior and implementation with the host fixture. |
tests/runner/transports/ws_tunnel/test_serve.py:717 — _ConnectKwargs.sock |
Represents the added socket argument in the existing connection spy. | Test helper | keep | Necessary support for the updated exact-argument assertion. |
tests/runner/transports/ws_tunnel/test_serve.py:708 — test_serve_tunnel_once_sends_bearer_header |
Preserves the direct connection’s authentication and connection arguments, including sock=None. |
Component | keep | Extends an existing assertion instead of requiring a separate default-path test. |
tests/runner/transports/ws_tunnel/test_serve.py:2142 — test_serve_tunnel_proxy_socket, configured-proxy parameter |
Selects and dials the proxy with the runner timeout, then forwards its socket. | Component | keep | Protects runner-specific wiring to the shared helper. |
tests/runner/transports/ws_tunnel/test_serve.py:2142 — test_serve_tunnel_proxy_socket, no-proxy-config parameter |
Supplies no socket on a direct connection. | Component | consolidate | The modified test_serve_tunnel_once_sends_bearer_header already asserts this connection argument. |
tests/runner/transports/ws_tunnel/test_serve.py:2142 — test_serve_tunnel_proxy_socket, explicit-bypass parameter |
Honors no_proxy before invoking the CONNECT dialer. |
Component | keep | Exercises the bypass outcome at the runner integration boundary. |
tests/runner/transports/ws_tunnel/test_serve.py:2168 — test_serve_tunnel_closes_proxy_socket_when_connect_fails, constructor parameter |
Closes the socket on synchronous construction failure. | Component | keep | Protects ownership before the context stack adopts the connection. |
tests/runner/transports/ws_tunnel/test_serve.py:2168 — test_serve_tunnel_closes_proxy_socket_when_connect_fails, entry parameter |
Closes the socket on asynchronous connection-entry failure. | Component | keep | Covers TLS/upgrade failure separately from construction. |
tests/test_ws_proxy.py:36 — test_proxy_selection, scheme and fallback cases |
Maps WS/WSS to the corresponding proxy variables and ALL_PROXY fallback. |
Unit | keep | Protects selection for both supported tunnel schemes. |
tests/test_ws_proxy.py:36 — test_proxy_selection, capitalization, precedence, empty-value, and bare-proxy cases |
Matches urllib/httpx environment precedence and normalization. | Unit | keep | These cases determine whether configured proxy requirements are honored or suppressed. |
tests/test_ws_proxy.py:36 — test_proxy_selection, malformed and unsupported cases |
Handles invalid target/proxy URLs and unsupported schemes without crashing selection. | Unit | keep | Protects the documented selection fallback behavior. |
tests/test_ws_proxy.py:36 — test_proxy_selection, CGI cases |
Ignores uppercase HTTP_PROXY under CGI while respecting lowercase configuration. |
Unit/security | keep | Guards the security-sensitive environment-variable distinction. |
tests/test_ws_proxy.py:36 — test_proxy_selection, bypass and URL-pattern cases |
Applies bypass variables, empty-value suppression, scheme patterns, and mount precedence. | Unit | keep | Covers environment selection behavior beyond isolated hostname matching. |
tests/test_ws_proxy.py:36 — test_proxy_selection, loopback cases |
Keeps localhost, IPv4 loopback, and IPv6 loopback direct. | Unit | keep | Preserves the existing local-endpoint trust convention. |
tests/test_ws_proxy.py:100 — test_no_proxy, hostname and wildcard cases |
Distinguishes exact names, subdomains, leading dots, wildcard forms, and non-suffixes. | Unit | keep | Protects the core bypass matching rules; add the documented CIDR non-match boundary. |
tests/test_ws_proxy.py:100 — test_no_proxy, plain host/port cases |
Matches explicit ports and normalizes default ports consistently. | Unit | keep | Default-port behavior differs from a simple string comparison. |
tests/test_ws_proxy.py:100 — test_no_proxy, URL-form cases |
Matches scheme, host pattern, and port together. | Unit | keep | Exercises behavior absent from plain-domain bypass entries. |
tests/test_ws_proxy.py:100 — test_no_proxy, IP and IPv6 cases |
Uses exact literal matching and rejects hostname-suffix tricks. | Unit/security | keep | Prevents unexpectedly broad bypasses for IP entries. |
tests/test_ws_proxy.py:146 — test_redact_proxy_url, both parameters |
Removes userinfo, path, query, and fragment from logged proxy URLs. | Unit/security | keep | Directly protects sensitive URL components. |
tests/test_ws_proxy.py:152 — test_unsupported_scheme_warns_once_without_credentials, both variable parameters |
Warns once, identifies the configuration source, and omits credentials. | Unit/security | keep | Covers primary/fallback variable diagnostics; add an unsupported TLS-to-proxy scheme parameter. |
tests/test_ws_proxy.py:168 — _connect_proxy |
Captures requests, returns scripted responses, and relays payload bytes. | Socket-test helper | keep | Provides a real local socket boundary for success and failure cases. |
tests/test_ws_proxy.py:189 — test_connect_tunnel, unauthenticated and authenticated parameters |
Produces correct CONNECT headers, decodes credentials, and returns a usable socket. | Socket integration | keep | Verifies actual bytes and post-handshake usability rather than mocked success alone. |
tests/test_ws_proxy.py:226 — test_connect_error, refused, non-http, non-decimal-status, oversized, trailing-bytes, and eof |
Rejects unsuccessful, malformed, oversized, or incomplete responses. | Socket integration | keep | Each parameter protects a distinct CONNECT rejection boundary. |
tests/test_ws_proxy.py:232 — test_connect_tunnel_idna_host |
Encodes the origin authority correctly without resolving it locally. | Socket integration | keep | Protects internationalized hostnames in proxy-only environments. |
tests/test_ws_proxy.py:248 — test_connect_rejects_malformed_proxy_url, both parameters |
Converts malformed proxy URL/port errors into OSError. |
Unit | keep | Protects stable failure handling before socket allocation; extend to malformed tunnel ports. |
tests/test_ws_proxy.py:253 — test_connect_closes_socket_when_awaiter_is_cancelled |
Closes a worker-returned socket after its caller has been cancelled. | Concurrency unit | keep | Protects late-result ownership across the thread boundary. |
tests/test_ws_proxy.py:278 — test_connect_timeout_bounds_a_slow_dial |
Returns promptly on deadline and later closes the worker’s socket. | Concurrency unit | keep | Covers caller timeout and delayed cleanup together. |
tests/test_ws_proxy.py:300 — test_connect_timeout_race_closes_completed_dial |
Closes a completed socket when timeout delivery wins the race. | Concurrency unit | keep | Exercises a race not reliably reached by the slow-dial case. |
tests/test_ws_proxy.py:321 — test_connect_timeout_bounds_the_whole_handshake and drip helper |
Prevents incremental response bytes from extending the total deadline. | Socket integration | keep | Distinguishes a total budget from a timeout reset on each read. |
tests/test_ws_proxy.py:343 — _self_signed_cert |
Creates matching and mismatched certificate identities. | TLS-test helper | keep | Enables local certificate-validation tests without external services. |
tests/test_ws_proxy.py:373 — _forwarding_proxy, including handler and relay |
Resolves the synthetic origin through the proxy and relays TLS bytes. | TLS-test helper | keep | Reproduces the proxy-only topology for a real TLS handshake. |
tests/test_ws_proxy.py:402 — test_connect_tunnel_wss, matching-certificate parameter |
Carries WebSocket traffic over CONNECT with correct SNI and certificate validation. | TLS integration | keep | Protects the successful secure-tunnel contract. |
tests/test_ws_proxy.py:402 — test_connect_tunnel_wss, mismatched-certificate parameter |
Rejects a certificate for another hostname. | TLS integration/security | keep | Proves proxying does not disable origin identity verification. |
Existing connection tests and the added focused cases cover the main integration and failure paths. The three requested additions target explicit documented boundaries, not a demonstrated production bug. No test removals or weakened assertions were identified.
For manual verification, start a host where the server origin cannot resolve directly but the configured HTTP proxy can reach it. Confirm the host becomes online through CONNECT, then repeat against a loopback server with proxy variables still set and confirm it connects directly.
Scope
All nine changed files support the single outcome: enabling host and runner WebSocket tunnels in HTTP-proxy-mandatory environments without changing direct connections or weakening TLS. No unrelated changes or unresolved scope questions were found. The full database reference was reviewed; this diff introduces no database changes.
Automated review by Polly · workflow run
- The unsupported-scheme warning test also covers an https:// proxy value. - test_no_proxy asserts a CIDR entry does not bypass, as in httpx. - The malformed-URL dialer test also covers a bad tunnel port and URL. - The two proxy-isolation conftests state their package-wide scope. Signed-off-by: omni-resolve-agent[bot] <omni-resolve-agent[bot]@users.noreply.github.com>
|
Dispositions for Polly run 37418397410 and OCR run 37418399558 on 8689fd5 (no blocking issues from either), test-only follow-ups in c434245: Polly (8689fd5)
OCR (8689fd5)
|
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
📄
|
|
| Category | Files | Added | Deleted | Share of changed text lines |
|---|---|---|---|---|
| Tests and test support | 6 | 808 | 0 | 59.0% |
| Documentation | 0 | 0 | 0 | 0.0% |
| Dependencies and lockfiles | 0 | 0 | 0 | 0.0% |
| Implementation / other | 3 | 537 | 25 | 41.0% |
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements.
Changes
The change makes host and runner WebSocket tunnels honor HTTP proxy environment settings, including authentication and bypass rules. It adds a shared proxy-selection and CONNECT implementation, integrates socket ownership into both callers, and adds regression tests and scoped environment-isolation fixtures.
There are no dependency, lockfile, documentation, or generated-asset changes. The size is justified by environment compatibility, protocol validation, and asynchronous cleanup requirements. No substantial reduction was identified that would preserve those behaviors and their distinct failure-path coverage.
Tests
Test-by-test assessment
The frozen diff was reviewed alongside relevant existing tests. The standard pytest suite was not run against the exact PR head; the supplied revisions are unavailable locally. Focused offline checks do not replace that validation.
| Test / case | Behavior protected | Layer | Needed? | Action / rationale |
|---|---|---|---|---|
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py::_SITECUSTOMIZE, _getaddrinfo |
Origin DNS fails outside the proxy | E2E support | keep | Makes direct dialing fail for the original regression rather than merely making a proxy available. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py::_mandatory_proxy, Handler.handle |
HTTP setup and CONNECT reach the live server through the proxy | E2E fixture | keep | Establishes the proxy-only topology and records actual CONNECT usage. |
tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py::test_host_comes_online_through_mandatory_proxy |
A real host process becomes online when direct origin resolution fails | E2E | keep | Uniquely exercises process environment, HTTP setup, tunnel establishment, and server presence together. |
tests/host/conftest.py::_no_ambient_proxy_env |
Ambient proxy variables cannot alter unrelated host tests | Fixture | keep | Local scope provides isolation without suppressing intentional proxy tests elsewhere. |
tests/host/test_connect.py::test_connect_and_serve_proxy_socket — direct case |
No proxy means no CONNECT dial and sock=None |
Unit | keep | Protects unchanged direct behavior. |
tests/host/test_connect.py::test_connect_and_serve_proxy_socket — constructor and context-entry failures |
Host supplies the proxy socket and closes it on either failure phase | Unit | keep | Both phases matter because supported WebSocket implementations can reject a connection before context entry. Add the log assertion noted above. |
tests/host/test_connect.py::test_connect_and_serve_builds_ssl_context_before_dialing_proxy |
TLS-context failure occurs before opening a proxy socket | Unit | keep | Protects against an unowned socket when CA configuration fails. |
tests/runner/transports/ws_tunnel/conftest.py::_no_ambient_proxy_env |
Ambient proxy variables cannot alter unrelated runner tests | Fixture | keep | Separate subtree scope is useful despite the small duplication with the host fixture. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_once_sends_bearer_header — _ConnectKwargs, _fake_connect, and expected kwargs updates |
Existing direct runner handshake explicitly passes sock=None |
Unit/test support | keep | Extends the existing contract assertion to account for the new argument. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_proxy_socket — absent proxy, configured proxy, and no_proxy cases |
Runner selects direct or CONNECT dialing and hands off the selected socket | Unit | keep | Covers caller integration rather than only the selector in isolation. Add the log assertion noted above. |
tests/runner/transports/ws_tunnel/test_serve.py::test_serve_tunnel_closes_proxy_socket_when_connect_fails — constructor and context-entry cases |
Runner closes the socket before successful transport ownership | Unit | keep | Protects distinct synchronous and asynchronous failure phases. |
tests/test_ws_proxy.py::test_proxy_selection — scheme mapping, absent proxy, ALL_PROXY, and bare-address cases |
Correct proxy source and fallback for ws and wss |
Unit | keep | Exercises the central environment-selection contract without network setup. |
tests/test_ws_proxy.py::test_proxy_selection — unsupported schemes and malformed URLs |
Unsupported or unusable configuration follows the stated fallback behavior | Unit | keep | Covers parsing and support boundaries separately from CONNECT failures. |
tests/test_ws_proxy.py::test_proxy_selection — capitalization, empty values, precedence, and CGI cases |
Environment handling follows urllib precedence, including HTTP_PROXY suppression under CGI |
Unit/security | keep | These cases protect subtle deployment and security semantics. |
tests/test_ws_proxy.py::test_proxy_selection — URL-form bypass and wildcard-priority cases |
Bypass selection respects scheme-specific versus all_proxy precedence |
Unit | keep | Protects non-obvious mount-priority behavior. |
tests/test_ws_proxy.py::test_proxy_selection — localhost, IPv4 loopback, and IPv6 loopback |
Loopback never uses the proxy | Unit | keep | Covers the required bypass across address forms. |
tests/test_ws_proxy.py::test_no_proxy — hostname cases |
Exact names, subdomains, leading dots, and wildcard handling | Unit | keep | Parameterization already consolidates the distinct matching rules. |
tests/test_ws_proxy.py::test_no_proxy — port and URL-form cases |
Default-port normalization and scheme/host/port matching | Unit | keep | Protects behavior that simple suffix matching would miss. |
tests/test_ws_proxy.py::test_no_proxy — IP and CIDR cases |
Exact IP matching, bracketed IPv6, unsupported CIDR, and misleading hostname suffixes | Unit/security | keep | Prevents unintended bypass while documenting supported input forms. |
tests/test_ws_proxy.py::test_redact_proxy_url — with and without userinfo |
Logs omit credentials, paths, queries, and fragments | Unit/security | keep | Covers sensitive URL components beyond passwords alone. |
tests/test_ws_proxy.py::test_unsupported_scheme_warns_once_without_credentials — all parameters |
Unsupported proxy schemes warn once without exposing credentials | Unit/security | keep | Verifies warning behavior for the supported environment-source combinations. |
tests/test_ws_proxy.py::_connect_proxy, respond |
Controlled CONNECT replies and post-handshake echo | Integration helper | keep | Shared setup supports meaningful success and failure assertions. |
tests/test_ws_proxy.py::test_connect_tunnel — unauthenticated and authenticated cases |
CONNECT returns a usable socket and correctly encodes decoded credentials | Integration | keep | Covers both ordinary and authenticated proxy operation. |
tests/test_ws_proxy.py::test_connect_error — refused, non-http, non-decimal-status, and eof |
Authentication refusal, malformed responses, and premature disconnects are rejected | Integration | keep | Each parameter covers a distinct protocol failure. |
tests/test_ws_proxy.py::test_connect_error — oversized and trailing-bytes |
Headers stay bounded and unexpected post-header data is rejected | Integration/security | keep | Protects resource bounds and protocol synchronization. |
tests/test_ws_proxy.py::test_connect_tunnel_idna_host |
CONNECT authority uses the encoded destination hostname | Integration | keep | Ensures proxy-side DNS receives the correct name. |
tests/test_ws_proxy.py::test_connect_rejects_malformed_proxy_url — all four cases |
Invalid ports and malformed IPv6 in proxy or target URLs produce the documented failure | Unit/boundary | keep | Selector coverage alone does not establish the dialer’s error contract. |
tests/test_ws_proxy.py::test_connect_closes_socket_when_awaiter_is_cancelled, slow_connect |
A worker returning after cancellation cannot leak its socket | Async unit | keep | The helper deterministically reaches late-result cleanup. |
tests/test_ws_proxy.py::test_connect_timeout_bounds_a_slow_dial, slow_dial |
The caller deadline includes dialing and late results are closed | Async unit | keep | Establishes the externally visible total deadline without depending on real DNS timing. |
tests/test_ws_proxy.py::test_connect_timeout_race_closes_completed_dial, wait_for_after_completion |
Simultaneous completion and timeout cannot orphan a socket | Async unit | keep | Deterministic race injection protects a path ordinary timeout tests may miss. |
tests/test_ws_proxy.py::test_connect_timeout_bounds_the_whole_handshake, drip |
Incremental response bytes cannot reset the timeout | Integration | keep | Verifies one overall handshake budget rather than independent read budgets. |
tests/test_ws_proxy.py::_self_signed_cert |
Supplies valid and hostname-mismatched TLS certificates | TLS helper | keep | Enables local verification without external certificate infrastructure. |
tests/test_ws_proxy.py::_forwarding_proxy, relay, handle |
Forwards tunneled TLS and records the requested authority | Integration helper | keep | Separates the proxy endpoint from the destination identity being verified. |
tests/test_ws_proxy.py::test_connect_tunnel_wss, echo — matching certificate |
CONNECT supports verified TLS, destination SNI, and actual WebSocket traffic | TLS integration | keep | Tests the security-critical mechanism beyond mocked socket handoff. |
tests/test_ws_proxy.py::test_connect_tunnel_wss — mismatched certificate |
A certificate for another hostname is rejected | TLS integration/security | keep | Negative verification is distinct from the successful TLS case. |
No critical coverage gap was demonstrated. Existing connection tests, the new caller-level assertions, and real CONNECT/TLS tests provide complementary coverage; another full runner E2E is not necessary for the same assertions. The production-log assertion gap is the focused improvement identified above.
For manual verification, start a host with the server reachable only through an HTTP CONNECT proxy. Confirm it reaches Online, reconnects after interruption, and never prints proxy credentials. Also confirm a loopback server still connects directly with proxy variables set.
Scope
All changes support the stated outcome: making host and runner WebSocket tunnels work through environment-configured HTTP proxies. No unrelated changes were found. Unsupported proxy schemes remain explicitly outside scope. The PR changes neither server protocols nor routing/sharding behavior, and it introduces no database changes.
Automated review by Polly · workflow run
|
Dispositions for Polly run 37419827395 and OCR run 37419829676 on c434245 (no blocking issues from either), both left as is:
|
|
@dhruv0811 this PR is ready for your re-review. All three requested changes are in, CI is green on Validate the fix live (this fix runs in the host/runner process, so check out the PR — attaching a local runner to the UI preview at https://omnigent-ui-preview-pr-6752-3272836215725701.aws.databricksapps.com would run an unfixed runner):
|
|
🤖 Otto review remediation complete No new commits were pushed in this run: the previous remediation run for the same review fingerprint (d9efd874a2b76c87) had already pushed 7fd323b..c434245, replied on both threads, and requested re-review before its agent job hit its deadline; this retry independently re-verified that work on the current head and completed the handoff. State of PR #6752 at c434245: ws_env_proxy_url() returns None for loopback tunnel URLs via omnigent_client._http.is_loopback_url (dhruv0811 item 1, 7fd323b); _env() uses urllib's two-pass, case-insensitive precedence so a present-but-empty lowercase variable suppresses the uppercase one, with urllib's CGI REQUEST_METHOD rule (item 2, 7fd323b + 107883c + 7f90bff); tests/test_ws_proxy.py::test_connect_tunnel_wss runs a TLS websockets server behind a forwarding CONNECT proxy, verifies the certificate against the tunnel host (SNI asserted) and rejects a certificate for another name (item 3, 7fd323b). Follow-on hardening from the Polly/OCR rounds: SSL context built before the proxy dial; runner constructs websockets.connect() inside its cleanup block; CONNECT dial shielded with a done-callback that closes a socket returned after cancellation or a deadline race; one asyncio.wait_for deadline over resolution + every address attempt + handshake; HTTP/ status-line check, 64 KiB cap after every read, IDNA authority, OSError for malformed URLs/ports; authority-only proxy logging; no_proxy matching mirrors httpx 0.28 including URL-form entries and all_proxy mount precedence (one documented exception: IP literal entries with a port stay exact). This run also replied on and resolved the stale OCR cancellation thread (4190839904) that 06621b1 had implemented. Useful follow-ups noted, not blockers: prefer the worker's own exception in the one-tick deadline race (OCR low finding on c434245); caplog assertions at the two call sites for proxy-URL redaction; surface the ignored-proxy hint in the connection-failure log; share the e2e online-poll helper. The existing PR branch has been updated and is ready for human re-review. |
A proxy-mandatory sandbox injects HTTP_PROXY/HTTPS_PROXY/ALL_PROXY/NO_PROXY (and their lowercase forms) into the host's environment, but _build_runner_env strips everything outside its allowlist before launching a runner subprocess. The runner's WebSocket tunnel and harness HTTP clients then have no route out and loop on name-resolution failures, so a session bound to a proxied host never gets an online runner. Allowlist the proxy variables so the runner inherits the same egress route as the host.
This reverts commit 24f52f3.
This branch has not been deployed
1 participant
Related issue
Closes #6746
Resolves OMNI-6799
Summary
Hosts and runners in proxy-only environments could reach the server over HTTP but repeatedly failed their WebSocket tunnels: the pinned
websockets<15client has no proxy support, so it dialed the origin directly and looped on name resolution.ELI5: Both tunnels now honor the standard proxy and bypass environment settings through a shared CONNECT implementation (
omnigent/util/ws_proxy.py), including authenticated proxies and redacted logging. Behavior without proxy configuration is unchanged.Scope notes:
http://CONNECT proxies are supported. A sandbox that mandates a SOCKS or TLS-to-proxy scheme is warned about once and dialed direct, which keeps the previous behavior for those schemes; supporting them is separate work.no_proxymatching mirrors httpx, the host's own HTTP client, so HTTP requests and the tunnel never disagree about the proxy: a plain name matches itself and its subdomains, a leading dot matches subdomains only, a*-prefixed entry matches nothing (as httpx mounts it), IP literals andlocalhostmatch their exact text (deliberately also when the entry carries a port, where httpx falls back to suffix matching),host:portrequires the port after the scheme's default port is dropped (soexample.com:443never matches a default-portwss://URL, as in httpx), and URL-form entries such ashttps://example.comorall://*.example.commatch by scheme, host pattern and port like an httpx mount (a bare scheme wildcard such ashttp://*only bypasses when the proxy itself came fromall_proxy, mirroring httpx mount precedence). CIDR entries are not interpreted, as they are not by httpx.Review follow-ups (commits 7fd323b through c434245):
ws_env_proxy_url()dials loopback targets directly via the existingis_loopback_url()helper, matching thetrust_envguard on the server-bound HTTP clients. A local host withHTTP_PROXY/ALL_PROXYset and no loopbackNO_PROXYentry keeps working.No_Proxy=…bypasses), a spelling ending in lowercase_proxytakes precedence, and such a spelling that is present but empty suppresses the others (http_proxy=""disablesHTTP_PROXY,no_proxy=""disablesNO_PROXY), andHTTP_PROXYis ignored under a CGIREQUEST_METHODenvironment as urllib does. Verified againsturllib.request.getproxies_environment().wss://connection through CONNECT is covered by one compact test: TLS is layered on the proxied socket, the certificate is verified against the tunnel URL's host (which only the proxy can resolve, and which is the SNI the server receives), and a certificate for another name is rejected.websockets.connect()fails before the event loop adopts it (InvalidURI,ssl/scheme mismatch), mirroring the host tunnel. This closes Polly's blocking finding.websockets.connect()inside the cleanup block (older pinnedwebsocketsreleases parse the URI in the constructor).asyncio.wait_for, so name resolution, every address attempt and the whole handshake share one deadline (the worker also clamps each send/read to the remaining budget), and a proxy that drips its reply or resolves to several unreachable addresses cannot stretch an attempt. A non-numeric proxy or tunnel port raises the documentedOSError, a status line that is notHTTP/...is rejected, and the 64 KiB header cap is enforced after every read.no_proxyfollows httpx's rules exactly (measured against httpx 0.28 in this environment):10.1.2.3no longer bypassesevil.10.1.2.3,.example.comno longer bypasses the apex,*.example.combypasses nothing, and IP literals compare as text. A timeout that fires just as the dial completes still closes the returned socket, and a non-decimal status token is reported as a refused CONNECT instead of raisingValueError.OSErrorin its relay loop, so setup failures surface throughsocketserver, and it ignores malformed request lines.open_proxy_connect_socket()shields the worker-thread dial; if the awaiting task is cancelled, a done-callback closes any socket the thread still hands back. The CONNECT request send also runs under the remaining budget.scheme://host[:port]): userinfo and any path, query, or fragment are dropped before the proxy URL reaches the INFO logs, and the once-only unsupported-scheme warning is tested to never include credentials.bücher.example→xn--bcher-kva.example), and an unencodable host raises the documentedOSError.urlsplit()rejects is handled like the other malformed inputs: selection warns once per variable (name only) and dials direct, the dialer raises the documentedOSError; the runner assigns its connection id before dialing so the dial stays the last step beforeconnect().InvalidURI, which the module docstring now states.Test Plan
From the PR checkout with the repository test dependencies installed (
uv sync --group test):Results on c434245 (Linux, Python 3.12, websockets 14.2, httpx 0.28.1, clean checkout):
tests/test_ws_proxy.py: 92 passed. Proxy selection including mixed-case, CGI, empty-variable, loopback, malformed-URL andall_proxy-fallback rows,no_proxyparsing with httpx's apex/leading-dot/wildcard/port/default-port/IP-literal/CIDR/URL-form results, the once-only unsupported-scheme warning (SOCKS and TLS-to-proxy values) naming the supplying variable, authority-only redaction, CONNECT framing (including an IDNA-encoded host), basic auth, refusal/non-HTTP/non-decimal/oversized/garbage/hangup handling, malformed proxy and tunnel URLs/ports, the overall deadline against a slow dial, a dripping reply and the completion race (with its message), socket cleanup when the awaiting task is cancelled, and the verifiedwss://round trip with matching and mismatched certificates.tests/runner/transports/ws_tunnel/test_serve.py, the host proxy tests intests/host/test_connect.py(direct/context-entry, proxied/constructor, proxied/context-entry failures, plus SSL-context ordering),tests/test_loopback_proxy_guards.py,tests/test_tls.py, andtests/frontends/sdk/test_http.py: 180 passed. The fulltests/host/test_connect.py: 266 passed.tests/e2e/test_host_tunnel_mandatory_proxy_e2e.py: 1 passed. A realomnigent hostprocess whose only route to the server is a CONNECT proxy comes online.HTTP_PROXY/Http_Proxy/http_proxy/ALL_PROXY/NO_PROXY/No_Proxyvalues exported: 115 passed, so the suites (whose conftests now strip proxy variables of any capitalisation) do not depend on the machine's environment._env()withurllib.request.getproxies_environment()for fourteen mixed-case/empty/CGI environments andws_env_proxy_url()with a realhttpx.Client(trust_env=True)for 414 proxy-environment × target ×no_proxycombinations (scheme-specific,all_proxy-only and mixed proxy variables; plain, dotted, wildcard, ported, IP and URL-form entries;http,https, subdomain and non-default-port targets), with no disagreement; the one deliberate exception, exact IP matching for IP entries that carry a port, is documented and tested.no_proxyrows including URL-form andall_proxywildcard entries, the host SSL-context ordering, the cancelled-dial socket close, and the IDNA authority).Manual check for the loopback case: export
HTTP_PROXY=http://unreachable.invalid:3128with noNO_PROXY, start a local server, and runomnigent host --server http://127.0.0.1:<port> --non-interactive --no-open. The host must print✓ Connected as ...without the proxy being contacted.Source reports and verification runs
Original resolve workflow
Latest resolve-agent update
Latest resolve workflow
Review remediation run
Demo
Recordings from the resolve-agent update; Linear sign-in required. Captions describe the recorded environment, not a new test run.
omnigent host --server <url> --non-interactive --no-open-> the tunnel loops 'Host tunnel disconnected: [Errno -3] Temporary failure in name resolution' and the host never comes onlinebefore-host-tunnel-mandatory-proxy.mp4
omnigent host --server <url> --non-interactive --no-open-> the tunnel now honors the proxy and the host prints '✓ Connected as ...' and comes onlineafter-host-tunnel-mandatory-proxy.mp4
Type of change
Test coverage
Coverage notes
The e2e test drives a real host process through a mandatory CONNECT proxy; the unit tests cover proxy selection (including loopback and empty-variable semantics), the CONNECT handshake, verified
wss://over the tunnel, and socket cleanup on connect failure for both tunnels.Release notes
Should this change be included in the release notes? Choose exactly one.
Changelog
Host and runner WebSocket tunnels honor
HTTP_PROXY/HTTPS_PROXY/ALL_PROXYandNO_PROXY, so they connect from proxy-only sandboxes; loopback servers are never proxied.