feat(update):Add proxy option to update command#2281
Conversation
Update docs to introduce update command proxy options
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for a --proxy argument in the codewhale update command, allowing users to perform self-updates through a proxy. It updates the documentation across multiple languages, adds CLI argument parsing, integrates the proxy configuration into the HTTP client builder, and includes corresponding unit tests. The review feedback highlights critical issues with the proxy implementation: the blocking connectivity test against GitHub's API should be removed to prevent failures when using custom mirrors, avoid wasting API rate limits, and reduce latency. Additionally, typos in the test names (udpate instead of update) and an invalid URL format in the tests should be corrected.
| let proxy = if let Some(proxy_str) = &args.proxy { | ||
| validate_and_build_proxy(proxy_str)? | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
The proxy connectivity test in validate_and_build_proxy is performed against LATEST_RELEASE_URL (GitHub API). However, if a user is using a mirror (e.g., via CODEWHALE_USE_CNB_MIRROR or CODEWHALE_RELEASE_BASE_URL because GitHub is blocked or slow), this connectivity test will fail and prevent the update from proceeding, even if the mirror is perfectly reachable through the proxy.
Additionally, this extra blocking request consumes an unnecessary GitHub API rate limit point and adds latency to every update command.
We should simplify validate_and_build_proxy to only parse and build the proxy without performing a blocking connectivity test.
let proxy = if let Some(proxy_str) = &args.proxy {
Some(validate_and_build_proxy(proxy_str)?)
} else {
None
};| fn validate_and_build_proxy(proxy_str: &str) -> Result<Option<Proxy>> { | ||
| let valid_url = reqwest::Url::parse(proxy_str).with_context(|| { | ||
| format!( | ||
| "invalid proxy URL: {proxy_str}\n\ | ||
| Expected format: http://host:port, https://host:port, or socks5://host:port" | ||
| ) | ||
| })?; | ||
|
|
||
| let proxy = reqwest::Proxy::all(valid_url)?; | ||
|
|
||
| // Quick connectivity test through the proxy | ||
| let client = reqwest::blocking::Client::builder() | ||
| .proxy(proxy.clone()) | ||
| .user_agent(UPDATE_USER_AGENT) | ||
| .timeout(Duration::from_secs(10)) | ||
| .build() | ||
| .context("Could not build proxy HTTP client")?; | ||
|
|
||
| match client.head(LATEST_RELEASE_URL).send() { | ||
| Ok(_) => Ok(Some(proxy)), | ||
| Err(e) => { | ||
| // Give a clear actionable error rather than a raw reqwest error | ||
| let hint = if e.is_timeout() || e.is_connect() { | ||
| "could not connect to the proxy server" | ||
| } else if e.is_request() { | ||
| "the request was sent but no response was received" | ||
| } else { | ||
| "an unexpected network error occurred" | ||
| }; | ||
| bail!( | ||
| "proxy connectivity failed: {hint}\n\ | ||
| Proxy URL: {proxy_str}\n\ | ||
| Details: {e}\n\ | ||
| Please verify:\n\ | ||
| - The proxy URL is correct\n\ | ||
| - The proxy server is running and reachable\n\ | ||
| - The proxy allows outbound connections to api.github.com" | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Simplify validate_and_build_proxy by removing the blocking connectivity test. This avoids:
- Breaking updates when using a mirror in environments where GitHub is blocked.
- Wasting GitHub API rate limits on an extra round-trip request.
- Unnecessary latency during the update process.
fn validate_and_build_proxy(proxy_str: &str) -> Result<Proxy> {
let valid_url = reqwest::Url::parse(proxy_str).with_context(|| {
format!("invalid proxy URL: {proxy_str}. Expected format: http://host:port, https://host:port, or socks5://host:port")
})?;
let proxy = reqwest::Proxy::all(valid_url)?;
Ok(proxy)
}| #[test] | ||
| fn udpate_parse_with_proxy() { | ||
| let cli = parse_ok(&["deepseek", "update", "--proxy", "http:localhost:7897"]); | ||
|
|
||
| let args = match cli.command { | ||
| Some(Commands::Update(args)) => args, | ||
| other => panic!("expected Update with proxy, got {other:?}"), | ||
| }; | ||
| assert_eq!( | ||
| args.proxy.expect("should have proxy"), | ||
| "http:localhost:7897" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix the typo in the test name (udpate -> update) and use a valid URL format (http://localhost:7897 instead of http:localhost:7897) to ensure standard URL parsing behavior.
| #[test] | |
| fn udpate_parse_with_proxy() { | |
| let cli = parse_ok(&["deepseek", "update", "--proxy", "http:localhost:7897"]); | |
| let args = match cli.command { | |
| Some(Commands::Update(args)) => args, | |
| other => panic!("expected Update with proxy, got {other:?}"), | |
| }; | |
| assert_eq!( | |
| args.proxy.expect("should have proxy"), | |
| "http:localhost:7897" | |
| ); | |
| } | |
| #[test] | |
| fn update_parse_with_proxy() { | |
| let cli = parse_ok(&["deepseek", "update", "--proxy", "http://localhost:7897"]); | |
| let args = match cli.command { | |
| Some(Commands::Update(args)) => args, | |
| other => panic!("expected Update with proxy, got {other:?}"), | |
| }; | |
| assert_eq!( | |
| args.proxy.expect("should have proxy"), | |
| "http://localhost:7897" | |
| ); | |
| } |
| #[test] | ||
| fn udpate_parse_without_proxy() { | ||
| let cli = parse_ok(&["deepseek", "update"]); | ||
|
|
||
| let args = match cli.command { | ||
| Some(Commands::Update(args)) => args, | ||
| other => panic!("expected Update, got {other:?}"), | ||
| }; | ||
| assert!(args.proxy.is_none()); | ||
| } |
There was a problem hiding this comment.
Fix the typo in the test name (udpate -> update).
| #[test] | |
| fn udpate_parse_without_proxy() { | |
| let cli = parse_ok(&["deepseek", "update"]); | |
| let args = match cli.command { | |
| Some(Commands::Update(args)) => args, | |
| other => panic!("expected Update, got {other:?}"), | |
| }; | |
| assert!(args.proxy.is_none()); | |
| } | |
| #[test] | |
| fn update_parse_without_proxy() { | |
| let cli = parse_ok(&["deepseek", "update"]); | |
| let args = match cli.command { | |
| Some(Commands::Update(args)) => args, | |
| other => panic!("expected Update, got {other:?}"), | |
| }; | |
| assert!(args.proxy.is_none()); | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Independent review: merged into a fresh worktree on top of origin/main; Findings greptile did not raise:
v0.8.48 (#2256) compatibility: clean (both touch |
reimplement validate_and_build_proxy and add test
| /// Validate the proxy URL and optionally test connectivity before proceeding. | ||
| pub(crate) fn validate_and_build_proxy(proxy_str: &str) -> Result<Proxy> { | ||
| let valid_url = reqwest::Url::parse(proxy_str).with_context(|| { | ||
| format!( | ||
| "invalid proxy URL: {proxy_str}\n\ | ||
| Expected format: http://host:port, https://host:port, or socks5://host:port" | ||
| ) | ||
| })?; | ||
|
|
||
| let proxy = reqwest::Proxy::all(valid_url)?; | ||
| Ok(proxy) | ||
| } |
There was a problem hiding this comment.
The doc comment on UpdateArgs::proxy and the error message inside validate_and_build_proxy both list socks5://host:port as a supported format. However, the reqwest workspace dependency is declared with default-features = false and only ["json", "rustls", "blocking"] features are enabled — the "socks" feature that gates SOCKS5 tunnel support is absent. Proxy::all("socks5://...") will return Ok at validation time (the URI is stored without scheme validation), but when a request is made through the proxy, reqwest has no SOCKS5 connector and will fail with a confusing runtime error. Either add "socks" to the reqwest features list in crates/cli/Cargo.toml (and the workspace), or remove socks5://host:port from all user-facing hints so users aren't misled.
2 participants
Summary
Add proxy support to update command
Accept an optional --proxy argument in the self update command,
it's validate the proxy URL, test connectivity, and pass the proxy through all update HTTP
requests made during the update workflow.
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR adds an optional
--proxyargument to thecodewhale updatecommand, routing all update-related HTTP requests (release metadata fetches and binary downloads) through the specified proxy. It also enables thesocksreqwest feature so SOCKS5 proxies work correctly at runtime.validate_and_build_proxyparses and validates the proxy URL, then threads the resultingreqwest::Proxythroughupdate_http_client,fetch_latest_release, anddownload_url— the connectivity pre-check from an earlier iteration was correctly removed.Cargo.tomladds the"socks"feature to the workspace reqwest dependency, closing the gap between the advertised SOCKS5 support and what reqwest could actually deliver.lib.rscover CLI parsing with and without--proxy, as well as valid/invalid inputs tovalidate_and_build_proxy.Confidence Score: 5/5
Safe to merge — proxy option is purely additive, the opt-in feature flag is correct, and the HTTP client is properly threaded throughout the update workflow.
The change is narrowly scoped: a new optional CLI flag that, when absent, leaves all existing behaviour unchanged. The socks feature gap is closed, the problematic connectivity pre-check from earlier review rounds has been removed, and all three HTTP paths (release metadata, checksum manifest, binary download) correctly receive the proxy. No correctness regressions identified.
No files require special attention.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant CLI as codewhale CLI (lib.rs) participant Update as run_update (update.rs) participant Proxy as validate_and_build_proxy participant HTTP as update_http_client participant GitHub as GitHub API / CDN User->>CLI: codewhale update --proxy socks5://host:1080 CLI->>Update: run_update(beta, Some("socks5://host:1080")) Update->>Proxy: validate_and_build_proxy("socks5://host:1080") Proxy-->>Update: Ok(reqwest::Proxy) Update->>HTTP: "update_http_client(&Some(proxy))" HTTP-->>Update: blocking::Client (proxy configured) Update->>GitHub: GET /repos/.../releases/latest GitHub-->>Update: Release JSON Update->>HTTP: "update_http_client(&Some(proxy))" HTTP-->>Update: blocking::Client Update->>GitHub: GET checksum manifest URL GitHub-->>Update: SHA256 bytes Update->>HTTP: "update_http_client(&Some(proxy))" HTTP-->>Update: blocking::Client Update->>GitHub: GET binary asset URL GitHub-->>Update: binary bytes Update-->>User: Updated successfullyReviews (3): Last reviewed commit: "enable reqwest socks features" | Re-trigger Greptile