fix(desktop): macOS system proxy support and missing native dependency - #586
Conversation
reqwest only reads HTTP_PROXY/HTTPS_PROXY env vars, ignoring macOS system proxy settings (Surge, ClashX, V2Ray). This caused the desktop app to fail loading the ACP agent registry behind a system proxy. Read the proxy via `scutil --proxy` and apply it to the reqwest client. Also preserve real error messages from Tauri invoke (string errors were silently replaced with a generic "failed to load" message), and use resolveApiPath instead of raw fetch in the web fallback path. Co-authored-by: Crush (GLM 5.2) <crush@local>
The native module was not being installed as an optional dependency, causing "Module not found: Can't resolve 'better-sqlite3'" at runtime. Bump the version range to pick up the latest prebuilt binaries. Co-authored-by: Crush (GLM 5.2) <crush@local>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughACP registry and binary downloads now use proxy-aware HTTP clients, web-mode API paths are resolved through a helper, string errors are preserved in the UI, and tests cover these flows. The optional ChangesACP installation and proxy support
SQLite optional dependency
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ACPInstall as install_acp_agent
participant HttpClient as build_http_client
participant SystemProxy as read_macos_system_proxy
participant Registry as ACP_REGISTRY_URL
participant BinaryManager as AcpBinaryManager
ACPInstall->>HttpClient: Build configured client
HttpClient->>SystemProxy: Read macOS proxy settings
HttpClient-->>ACPInstall: Return reqwest client
ACPInstall->>Registry: Fetch registry
ACPInstall->>BinaryManager: Install binary with client
BinaryManager->>Registry: Download archive with client
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/client/components/agent-install-panel.tsx`:
- Around line 202-203: Update the registry request in the surrounding refresh
flow to call desktopAwareFetch instead of fetch, while preserving the existing
resolveApiPath URL construction and refresh query behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d3881840-0f94-45ca-8aee-29a4eb73624f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/lib.rspackage.jsonsrc/client/components/agent-install-panel.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c448fc926
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : t.agents.failedToLoad); | ||
| const msg = err instanceof Error ? err.message : typeof err === "string" ? err : t.agents.failedToLoad; |
There was a problem hiding this comment.
The new string-preserving conversion only runs in fetchAgents; the Tauri install/uninstall handlers still catch install_acp_agent/uninstall_acp_agent rejections with the old err instanceof Error ? ... : t.agents.installFailed/uninstallFailed pattern. When a Tauri command returns Err(String) after the registry has loaded, such as a binary download failure or unsupported platform, the actual message is still replaced by the generic install error, so the desktop diagnostic fix remains ineffective for the install scenario it calls out. Reuse the same string fallback in those handlers.
Useful? React with 👍 / 👎.
| }, | ||
| "optionalDependencies": { | ||
| "better-sqlite3": "^12.6.2", | ||
| "better-sqlite3": "^12.11.1", |
There was a problem hiding this comment.
Keeping this package only under optionalDependencies means installs that omit optional deps, such as npm ci --omit=optional or environments where the optional native install is skipped, still won't have better-sqlite3, and the desktop/SQLite path can still throw the same “Ensure better-sqlite3 is installed” error. The lockfile's added root dependency entry is regenerated from package.json and is not a durable source of truth; if desktop requires the module, add it to dependencies instead of only bumping the optional entry.
Useful? React with 👍 / 👎.
| // Fetch from CDN | ||
| let response = reqwest::get(ACP_REGISTRY_URL) | ||
| // Fetch from CDN (with macOS system proxy support) | ||
| let client = build_http_client()?; |
There was a problem hiding this comment.
When installing a binary-distributed agent on macOS behind a system proxy, this proxy-aware client is only used for the registry request; the binary branch still calls AcpBinaryManager::install_binary, whose archive download uses a plain reqwest::get(url), so the actual agent binary bypasses the macOS proxy and fails after the registry has loaded. Pass the same proxy configuration into the binary download path as well.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| // Fall back to HTTP proxy if HTTPS proxy is not configured |
There was a problem hiding this comment.
This fallback only checks HTTPS* and HTTP* entries from scutil --proxy; when macOS is configured with only a SOCKS proxy (SOCKSEnable/SOCKSProxy/SOCKSPort), read_macos_system_proxy() returns None and the registry request still goes direct. That leaves common SOCKS-only proxy setups unsupported by the new desktop proxy path, so parse the SOCKS fields as well and enable reqwest's SOCKS support for that URL.
Useful? React with 👍 / 👎.
Route registry requests through the desktop-aware fetch path, isolate and test macOS system proxy parsing, preserve Tauri string errors in tests, and keep the optional dependency lock metadata canonical. Co-authored-by: Codex (GPT 5.5) <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4aea30ea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match read_macos_system_proxy().and_then(|proxy_url| reqwest::Proxy::all(proxy_url).ok()) { | ||
| Some(proxy) => builder.proxy(proxy), |
There was a problem hiding this comment.
When the desktop process has HTTP_PROXY/HTTPS_PROXY or NO_PROXY configured while macOS also reports a system proxy, adding an explicit proxy with ClientBuilder::proxy disables reqwest's automatic environment-proxy handling. This regresses the previous reqwest::get behavior by silently replacing the process-specific proxy or bypass rules with the macOS proxy; use the scutil result only as a fallback when no environment proxy applies, or explicitly carry the environment and no-proxy configuration into the client.
Useful? React with 👍 / 👎.
Normalize string rejections from Tauri registry, install, and uninstall commands so actionable desktop failures are not replaced by generic UI messages. Co-authored-by: Codex (GPT 5.5) <codex@openai.com>
Pass the desktop proxy-aware reqwest client into binary archive downloads and support SOCKS-only macOS system proxy configurations. Co-authored-by: Codex (GPT 5.5) <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04eebb8a4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| /// Build an HTTP client that respects the macOS system proxy when configured. | ||
| pub(crate) fn build_http_client() -> Result<reqwest::Client, String> { | ||
| let builder = reqwest::Client::builder().timeout(Duration::from_secs(15)); |
There was a problem hiding this comment.
When a binary archive takes more than 15 seconds to connect and fully transfer—especially over the slower proxies this change targets—Reqwest applies this client timeout as a total deadline through completion of the response body. install_acp_agent now passes this same client to install_binary_with_client, so desktop installs abort after 15 seconds even though the web/server path and the previous reqwest::get path had no total timeout. Use separate clients or apply a connect/read timeout that does not cap the complete archive transfer.
AGENTS.md reference: AGENTS.md:L3-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/routa-core/src/acp/binary_manager.rs (1)
35-44: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove the dead
install_binarymethod.
install_binarystill builds a barereqwest::Client::new(), but there are no remaining callers of it in the codebase. Delete the legacy path rather than keeping a reachable entry point with no proxy support.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/routa-core/src/acp/binary_manager.rs` around lines 35 - 44, Remove the unused public install_binary method from the binary manager, including its bare reqwest::Client construction; retain install_binary_with_client as the supported installation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/routa-core/src/acp/binary_manager.rs`:
- Around line 35-44: Remove the unused public install_binary method from the
binary manager, including its bare reqwest::Client construction; retain
install_binary_with_client as the supported installation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 37088e6d-d2f2-4849-b344-e02d67c315d9
📒 Files selected for processing (6)
apps/desktop/src-tauri/Cargo.tomlapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/system_proxy.rscrates/routa-core/src/acp/binary_manager.rssrc/client/components/__tests__/agent-install-panel.test.tsxsrc/client/components/agent-install-panel.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src-tauri/src/lib.rs
- apps/desktop/src-tauri/src/system_proxy.rs
Co-authored-by: Codex (GPT 5.5) <codex@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src-tauri/src/system_proxy.rs`:
- Around line 33-36: Update the proxy construction flow around
read_macos_system_proxy and NoProxy::from_env to also honor macOS ExceptionsList
entries and ExcludeSimpleHostnames when selecting the proxy. Parse the system
settings, apply compatible exclusions or a custom proxy selector while
preserving existing environment-based exclusions, and add fixtures covering both
bypass cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a714c9f1-822f-41e1-8d75-756a4f245791
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/system_proxy.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/lib.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efb86412b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| https | ||
| .url("http") | ||
| .or_else(|| http.url("http")) | ||
| .or_else(|| socks.url("socks5h")) |
There was a problem hiding this comment.
When macOS has only Web Proxy (HTTP) enabled while Secure Web Proxy (HTTPS) is disabled, this fallback still returns the HTTP configuration, and build_http_client installs it with Proxy::all; the always-HTTPS ACP registry request is therefore forced through a proxy that macOS explicitly does not select for HTTPS. This can make registry loading fail in that configuration, while distinct HTTP/HTTPS settings also route HTTP binary URLs through the wrong proxy. Keep the HTTP and HTTPS matchers scheme-specific rather than treating HTTP as a fallback for HTTPS.
Useful? React with 👍 / 👎.
Co-authored-by: Codex (GPT 5.5) <codex@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0436b6df0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "SOCKSEnable" => socks.enabled = value == "1", | ||
| "SOCKSProxy" => socks.host = Some(value.to_string()), | ||
| "SOCKSPort" => socks.port = value.parse().ok(), | ||
| _ => {} |
There was a problem hiding this comment.
When macOS uses Automatic Proxy Configuration or auto-discovery instead of a static HTTP/HTTPS/SOCKS proxy, scutil --proxy reports fields such as ProxyAutoConfigEnable and ProxyAutoConfigURLString, but this match discards them. has_proxy() then remains false and both registry and binary requests fall back to a client that does not evaluate the PAC file, so these installations still connect directly and fail on networks where the PAC proxy is mandatory. Handle the automatic proxy fields or delegate routing to a PAC-aware system client.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes three desktop runtime issues discovered on macOS:
ACP agent registry fails to load behind system proxy —
reqwestonly readsHTTP_PROXY/HTTPS_PROXYenv vars and ignores macOS system proxy settings. Proxy software like Surge, ClashX, and V2Ray configure the proxy vianetworksetup(System Preferences), which reqwest never sees. Addedbuild_http_client()that reads the system proxy viascutil --proxyand applies it to the reqwest client.Agent install errors silently replaced with generic message — Tauri invoke returns errors as strings, but
err instanceof Errorwasfalse, so real errors like "Failed to fetch registry: ..." were silently replaced with a generic message. Now preserves the actual error. Also switched the web-mode fallback from rawfetchtoresolveApiPathper project conventions."Module not found: Can't resolve 'better-sqlite3'" — The native module was listed as
optionalDependenciesbut not installed on some machines. Bumped version range to^12.11.1to pick up latest prebuilt binaries.Commits
fix(desktop): respect macOS system proxy for ACP registry fetchfix(deps): bump better-sqlite3 to ^12.11.1Test plan
cargo checkpasses inapps/desktop/src-tauriagent-install-panel.test.tsxbetter-sqlite3loads after install💘 Generated with Crush
Summary by CodeRabbit