Conversation
DOsinga
requested review from
codefromthecrypt and
Copilot
and removed request for
Copilot
|
goose tells me we need these in the cargo files to make it work. it does work for me on mac this way and before did not. |
There was a problem hiding this comment.
Pull request overview
Adds the reqwest system-proxy feature across Rust crates to support honoring OS/system proxy settings (intended to address issue #6835: proxy env vars not honored in the Rust backend).
Changes:
- Enable
reqwest’ssystem-proxyfeature ingoose,goose-server, andgoose-mcp. - Update
Cargo.lockto include new transitive dependencies required for system proxy support on macOS/Windows.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/goose/Cargo.toml | Adds reqwest system-proxy feature for the main backend crate. |
| crates/goose-server/Cargo.toml | Adds reqwest system-proxy feature for the server crate. |
| crates/goose-mcp/Cargo.toml | Adds reqwest system-proxy feature for the MCP crate. |
| Cargo.lock | Updates dependency graph for the new reqwest feature. |
crates/goose/Cargo.toml
Outdated
| [build-dependencies] | ||
| tokio = { workspace = true } | ||
| reqwest = { workspace = true, features = ["json", "rustls-tls-native-roots"], default-features = false } | ||
| reqwest = { workspace = true, features = ["json", "rustls-tls-native-roots", "system-proxy"], default-features = false } |
There was a problem hiding this comment.
Enabling reqwest's "system-proxy" feature here won’t have any effect unless the code opts in when building clients (e.g., via ClientBuilder::use_system_proxy()/system proxy configuration), so this PR as-is likely won’t fix #6835.
| serde_yaml = "0.9.34" | ||
| utoipa = { version = "4.1", features = ["axum_extras", "chrono"] } | ||
| reqwest = { workspace = true, features = ["json", "rustls-tls", "blocking", "multipart"], default-features = false } | ||
| reqwest = { workspace = true, features = ["json", "rustls-tls", "blocking", "multipart", "system-proxy"], default-features = false } |
There was a problem hiding this comment.
Same as in goose: adding the "system-proxy" feature alone doesn’t enable proxy usage unless the runtime code opts in when constructing reqwest::Clients, so this change likely doesn’t achieve the PR goal by itself.
| indoc = "2.0.5" | ||
| xcap = "=0.4.0" | ||
| reqwest = { workspace = true, features = ["json", "rustls-tls-native-roots"], default-features = false } | ||
| reqwest = { workspace = true, features = ["json", "rustls-tls-native-roots", "system-proxy"], default-features = false } |
There was a problem hiding this comment.
Same as in goose: adding the "system-proxy" feature alone doesn’t enable proxy usage unless the runtime code opts in when constructing reqwest::Clients, so this change likely doesn’t achieve the PR goal by itself.
|
Copilot seems to be wrong; system proxies are enabled by default so building with the feature should be enough: https://docs.rs/reqwest/latest/reqwest/#proxies |
jamadeo
left a comment
jamadeo
left a comment
There was a problem hiding this comment.
Interesting to see the other default features that we're not enabling (default-features = false): https://docs.rs/crate/reqwest/0.12.28/features#default / https://docs.rs/reqwest/latest/reqwest/#optional-features
http2 and charset seem like features we should probably also add?
| reqwest = { workspace = true, features = [ | ||
| "json", | ||
| "rustls-tls-native-roots", | ||
| "system-proxy", |
There was a problem hiding this comment.
Enabling the system-proxy feature alone doesn't fix the proxy issue. The code must also call .use_system_proxy() on each Client::builder() instance. Based on the issue description, at minimum these files need the actual implementation:
- crates/goose/src/providers/api_client.rs (lines 211 and 233)
- crates/goose/src/providers/githubcopilot.rs (line 161)
- crates/goose/src/providers/gcpvertexai.rs (line 166)
- crates/goose/src/providers/toolshim.rs (line 73)
Co-authored-by: Douwe Osinga <douwe@squareup.com>
Because of feature unification across the workspace, I think they do end up enabled, because the |
|
aha, got it, thanks @jh-block |
Co-authored-by: Douwe Osinga <douwe@squareup.com>
Co-authored-by: Douwe Osinga <douwe@squareup.com>
3 participants
Summary
Fixes: #6835