Skip to content

fix(desktop): use shell proxies for provider and backend requests - #15852

Closed
jieyuexing wants to merge 11 commits into
pingdotgg:mainfrom
jieyuexing:fix/desktop-shell-proxy-env
Closed

jieyuexing wants to merge 11 commits into
pingdotgg:mainfrom
jieyuexing:fix/desktop-shell-proxy-env

Conversation

@jieyuexing

@jieyuexing jieyuexing commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Desktop apps launched outside a terminal can miss login-shell proxy settings. Enabling Node's environment proxy handling also sends local backend requests through the proxy when the bypass list is missing or empty, breaking preview discovery and other loopback clients.

Change

Import proxy settings during existing POSIX shell preparation. Preserve inherited endpoint groups, allow an inherited bypass list to accompany imported endpoints, and remove blank aliases that would shadow valid settings. Add --use-env-proxy to the native backend's arguments only for a usable HTTP(S) proxy unless NODE_USE_ENV_PROXY=0; ALL_PROXY alone does not enable it. Windows shell preparation and WSL forwarding keep their existing behavior.

The backend's shared outbound dispatcher adds loopback bypass entries while retaining external proxy routing and the user's bypass rules. On older bundled undici versions it rebuilds the installed environment agent using that agent's own constructor; on undici 8 it uses the existing HTTP/1 dispatcher setup. It does not mutate process.env or add the backend flag to provider/terminal children. The scanner-specific transport layer was removed; preview discovery, readiness, API streams, WebSockets and managed OpenCode clients use the common transport policy. Undici's hostname suffix matching applies; this is not an exact-host-only guarantee.

Scope and approval

Thanks to @jieyuexing for the original diagnosis and implementation in #15852. Contributor commits are preserved, with main integrated through a merge. Maintainer-authorized takeover publication is complete; it does not grant landing approval. Human networking approval, current-head CI and bot review remain landing gates.

Verification

Published head: 418fb359. Two independent final full-diff reviews (Opus and Sol) found no code issues.

  • The earlier shell/configuration stage passed all 60 focused desktop tests, scoped typecheck, lint and formatting. The three added desktop cases retain main's existing coverage. The earlier sanitized shell/provider proof was executed at 1125c258; it supports unchanged shell hydration, precedence and child environment behavior only.
  • The independent production runtime matrix executed 27 cases and 710 assertions at 40585419 with Electron 44.4.5 / Node 24.21, Node 24.22 and Node 26.8.2. Real HTTP, TLS and WebSocket fixtures covered absent/empty/configured bypass settings, loopback aliases, wildcard, opt-out, shell-only settings, lowercase precedence and dummy credentials with trusted TLS. Shared readiness, device-style API/stream, OpenCode v1/v2 and production scanner paths worked; external requests still reached the disposable proxy. Scanner TLS trust, redirects and cancellation/timeout were exercised. Backend environment snapshots stayed unchanged; fixture processes and listeners were cleaned up.
  • Sanitized runtime results and source mapping. Production source at the published head differs from the executed matrix only by removal of one export keyword from an internal function the fixture never imports. The other intervening change is the transport-neutral regression test. Both final reviewers accepted this bounded reuse. The 27-case matrix was not rerun at the published head.
  • The corrected regression requires the loopback listener's direct body, the proxy's proxied external body, only the external authority at the proxy, and unchanged NO_PROXY. It handles both CONNECT and absolute-form proxy traffic. The test-fix and final-cleanup workers each reported Node 24: 5 passed / 1 HTTP/2 case skipped; Node 26: 6 passed. Removing the loopback repair made the test fail on both runtimes. Final scoped server typecheck, lint and formatting passed. Targeted knip passed before and after the private-export cleanup; an unused-export CI failure was not reproduced.

The separate node:http global agent remains proxied: its informational 403/502 results are not direct-routing passes. The source inventory found no newly affected owned in-process loopback consumer using it; those consumers use the shared fetch/WebSocket dispatcher or agent:false.

This is headless network/process proof, not full packaged-app startup or bootstrap/manager lifecycle proof. No macOS Finder/Dock, Windows/WSL, native device/hub, actual SSH session, vendor CLI/provider turn, Chromium preview, mobile, remote relay or tunnel was executed. Device handlers and process ownership use fixtures; scanner OS ownership is synthetic. The external host is mapped locally by the proxy. Dummy credential/TLS checks do not establish support for every enterprise proxy. Node 26 uses a substituted executable for compatibility; Node 22/undici 6 and future private dispatcher shapes were not runtime-tested. No final-head native execution or new CI result is claimed by this publication.

Implemented with Claude Fable 5.1; tests and independent reviews with Claude Opus 5.5 and GPT-6.1 Sol through T3 Code. Runtime proof and publication by GPT-6.1 Sol through the Codex harness.

When the desktop app is launched from the Dock or Finder, the process does not
inherit proxy variables that users export in their shell profile (for example
https_proxy in ~/.zshrc). Provider CLIs spawned by the backend then connect
directly; on networks that require a proxy, Claude fails with
"API Error: 403 Request not allowed".

Capture the proxy variables alongside PATH from the login shell and hydrate them
as one group, only when the process has none, so an inherited proxy keeps its
own NO_PROXY.

Refs pingdotgg#2709

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 5, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 5, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes default networking for desktop backends and the server-wide outbound fetch path, including provider, preview, device, and loopback services. Its cross-cutting runtime impact and proxy-default changes require human review despite the added focused tests.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9e39d144-8a82-4acf-8bd0-2f63531dec97
📥 Commits

Reviewing files that changed from the base of the PR and between d82c3e5 and 1125c25.

📒 Files selected for processing: 4
  • apps/desktop/src/backend/DesktopBackendConfiguration.test.ts
  • apps/desktop/src/backend/DesktopBackendConfiguration.ts
  • apps/desktop/src/shell/DesktopShellEnvironment.test.ts
  • apps/desktop/src/shell/DesktopShellEnvironment.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

Desktop startup captures proxy variables from the POSIX login shell and conditionally adds Node’s --use-env-proxy option to Windows-native launch arguments.

Changes: Proxy environment handling

Layer / File(s) Summary
Capture and apply proxy variables
apps/desktop/src/shell/DesktopShellEnvironment.ts, apps/desktop/src/shell/DesktopShellEnvironment.test.ts
The login-shell probe captures proxy endpoint and bypass variables. On POSIX, blank inherited proxy values are removed. Shell endpoint and bypass values are copied when their respective inherited groups contain no nonblank values. Tests cover inheritance, blank aliases, and shell values.
Select Node proxy launch option
apps/desktop/src/backend/DesktopBackendConfiguration.ts, apps/desktop/src/backend/DesktopBackendConfiguration.test.ts
Windows-native launch arguments include --use-env-proxy when an HTTP or HTTPS proxy variable is non-empty, unless NODE_USE_ENV_PROXY is "0". Tests cover flag selection and bootstrap argument placement.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 1125c

No demonstrated proxy regression currently blocks merging; the packaged-runtime behavior for scheme-less proxy values remains unverified.

Security architecture risk: 🔵 Low · up to 1125c

The change makes desktop requests follow locally configured proxies while preserving inherited settings and an explicit opt-out. No privilege expansion or new credential-exposure path was established. Routing of local requests and bypass matching still depends on runtime behavior that was not validated here.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant exposure is the desktop user's native backend and proxy-aware descendants, not only one provider request. The server supplies a shared fetch-based HTTP client, so activation can affect other backend consumers, including local HTTP health requests. Exact runtime routing and alternate transports remain unverified.

Security Findings and Attack Paths

  • observed — The inspected Grok billing consumer attaches its stored bearer credential to an explicit HTTPS URL through the injected HTTP client. The inspected daemon health request is plain HTTP to loopback but attaches no credential. These paths do not establish a newly introduced plaintext credential leak.

Trust Boundaries and Controls

  • observed — Proxy authority comes from the launching process or the user's login shell. Login-shell execution and shell-supplied CA configuration predate this PR; the change extends that existing trust relationship to proxy routing rather than introducing a remote source of proxy settings. Nonblank inherited endpoint and bypass groups remain authoritative.

Resilience and Maintainability Implications

  • inferred — Failed or timed-out shell commands discard captured output before hydration. Proxy mutations occur after probing and contain no effect yield between endpoint and bypass updates, limiting interruption-related partial proxy state. Concurrent and interrupted installation were assessed from source rather than directly exercised.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: using shell proxy settings for desktop provider and backend requests.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The pull request includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the expected behavior, implementation scope, approval context, focused tests, runtim…
Full details: Description check

Explanation

The pull request includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the expected behavior, implementation scope, approval context, focused tests, runtime coverage, limitations, and agent usage. The description is complete and specific.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

jieyuexing and others added 2 commits October 8, 2026 10:59
@jieyuexing jieyuexing changed the title fix(desktop): pass the login shell's proxy variables to provider CLIs fix(desktop): use shell proxies for provider and backend requests Oct 8, 2026
@github-actions github-actions Bot added size:M 30-99 changed lines (additions + deletions). and removed size:S 10-29 changed lines (additions + deletions). labels Oct 8, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 11, 2026 06:32

Dismissing prior approval to re-evaluate d82c3e5

@juliusmarminge juliusmarminge self-assigned this Oct 18, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

Taking over this one. I’ll reduce the tests to the useful coverage, update the implementation against current main, and verify it before publishing the changes.

juliusmarminge and others added 4 commits October 18, 2026 16:05
A desktop started from the Dock or Finder found provider CLIs through the
login shell's PATH but never saw the proxy variables that shell exports, so
their requests failed. The backend's own Node fetch also ignores HTTP(S)_PROXY
without Node's opt-in.

The login-shell probe now reads the proxy endpoint and bypass variables and
hydrates them as groups: an inherited endpoint keeps its own bypass list and
takes nothing from the shell, while an inherited bypass list alone still takes
the shell's endpoints. The locale group now shares the same helper. The local
Electron-as-Node backend gets --use-env-proxy when an HTTP(S) proxy variable is
set, unless NODE_USE_ENV_PROXY=0; ALL_PROXY alone does not enable it. Windows
inherits its environment from the registry and WSL forwarding is unchanged.

Tests cut from four to three, one per behavior, with process.env restored
through a scope finalizer so a failing assertion cannot leak proxy settings
into later tests.

Takeover of pingdotgg#15852 by jieyuexing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ive proxy

Node's --use-env-proxy reads https_proxy before HTTPS_PROXY and treats a
blank value as set. An inherited `https_proxy=""` therefore sent HTTPS
traffic directly past an imported or inherited uppercase endpoint, and a
whitespace value failed backend startup with ERR_INVALID_URL. The same
applied to no_proxy hiding NO_PROXY.

Drop blank proxy endpoint and bypass aliases before applying the shell's
proxy groups, so the effective environment matches the blank-means-unset
policy. Nonblank inherited endpoints and bypass lists are untouched.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
juliusmarminge and others added 4 commits October 19, 2026 12:40
Enabling `--use-env-proxy` for the desktop backend sent the preview
scanner's loopback probes through the configured proxy whenever NO_PROXY
was absent or empty. A proxy that refuses loopback made every running dev
server disappear from preview discovery, with the failed probes cached.

The scanner only ever talks to loopback, so it now gets its own scoped
`node:http` agents via NodeHttpClient at server composition. Fetch-based
external requests keep following the proxy, and the proxy environment
provider and terminal children inherit is unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Enabling `--use-env-proxy` for the desktop backend made Node's env proxy
agent route every loopback request through the configured proxy when
NO_PROXY was absent or did not list loopback. Besides the preview scanner,
that broke every server the backend owns on loopback: device hub and daemon
readiness, API and stream requests (local and SSH-forwarded), and managed
OpenCode servers through both the v2 HttpClient and the v1 SDK's global
fetch. A proxy that cannot reach the client's loopback failed them all.

The loopback bypass now lives in the outbound fetch setup, which already
owns the global dispatcher. When Node installed an env proxy agent, it is
rebuilt from the same class with localhost, 127.0.0.1 and [::1] appended to
the configured NO_PROXY (a `*` wildcard is left alone). External requests
keep following the proxy, process.env is untouched so provider and terminal
children inherit the user's settings unchanged, and the class comes from the
installed instance because Node's bundled undici can differ in major
version from the npm copy. The scanner-only NodeHttpClient layer is dropped
since the common policy covers it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… internals

The regression test for the loopback bypass asserted the global dispatcher's
constructor name and the error a refused CONNECT produces. Node 26 bundles
undici 8, which wraps the dispatcher and reaches an HTTP proxy with an
absolute-form request instead of a tunnel, so the test failed there while
production routing was correct on every runtime.

The test proxy now answers every request itself, by tunnel or absolute-form,
and the assertion reads what came back: the loopback probe gets the direct
listener's body, the external probe gets the proxy's body, the proxy saw
exactly the external authority, and NO_PROXY is untouched. Production code
is unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Only configureOutboundFetch in the same file calls it, and knip's --exports
check in CI reports exports without an importer.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Oct 19, 2026
@juliusmarminge juliusmarminge added the takeover t3 automation: takeover label Oct 24, 2026 — with GitCafe Production
@juliusmarminge juliusmarminge added the pr:in-progress A lead is working on this PR label Oct 24, 2026

Copy link
Copy Markdown
Member

Taking over this one

@juliusmarminge juliusmarminge added pr:needs-julius Needs Julius: route gate, unverifiable surface, or blocked and removed pr:in-progress A lead is working on this PR labels Oct 24, 2026
@jieyuexing

Copy link
Copy Markdown
Contributor Author

Note

Drafted by Claude Opus 5.5 in Claude Code; reviewed and posted by @jieyuexing

A heads-up for when this is rebased onto current main: #28556 (discover the shell environment without blocking startup) changes the timing this PR relies on.

On this branch, installIntoProcess resolves the login shell before resolvePrimaryStartConfig, so the proxy variables are already in process.env when the desktop decides whether to pass --use-env-proxy. After #28556:

  1. On a cache hit, the desktop applies shell-environment-cache.json and spawns the backend right away. CACHED_ENV_NAMES deliberately leaves the proxy variables out.
  2. If the proxy is only set in the user's shell profile (the usual case for an app launched from the Dock), the spawn-time check sees no proxy, so --use-env-proxy is not passed and the backend keeps a direct dispatcher.
  3. The proxy variables arrive a moment later in the fd4 shellEnvironment snapshot, and makeApplyDesktopShellEnvironmentSnapshot writes them into process.env. Nothing reinstalls the global dispatcher at that point, so the backend's own fetch stays direct for the rest of the session. Provider CLIs spawned afterwards do get the proxy, which makes this easy to miss.

What I observed in a build that carries this PR's earlier revision plus #21961 on top of current main (macOS, Dock launch, proxy only in ~/.zshrc, no direct route to Google): chats through the Antigravity provider work, but the limits probe's POST https://oauth2.googleapis.com/token times out after 10 s on every refresh, so the limits never show. The same request succeeds through the proxy.

A possible fix is to rerun the dispatcher setup when an applied snapshot changes HTTP(S)_PROXY / NO_PROXY (an EnvHttpProxyAgent when a proxy is present, a direct agent when it is removed), instead of deciding only at spawn. I'm adding that to my fork for now and am happy to adapt it to whatever shape you land here.

@t3dotgg

t3dotgg commented Nov 12, 2026

Copy link
Copy Markdown
Member

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Closing. This PR conflicts with main in 5 files, which is more than we will review as a rebase. If the change still matters, open a fresh PR from current main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews pr:needs-julius Needs Julius: route gate, unverifiable surface, or blocked size:L 100-499 changed lines (additions + deletions). takeover t3 automation: takeover vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants