fix(tui): enable proxy env for js execution#3331
Conversation
Signed-off-by: cyq <15000851237@163.com>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring Node.js proxy environment variables (such as HTTP_PROXY, HTTPS_PROXY, and NO_PROXY) when executing JavaScript tools, ensuring that Node.js honors standard proxy settings. Feedback was provided regarding the test implementation, which uses unsafe std::env::set_var and std::env::remove_var to modify process-wide environment variables. This approach can cause undefined behavior in multi-threaded test environments. It is recommended to refactor the environment lookup logic to accept a closure, enabling pure unit testing with mock environments without modifying the actual process environment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn first_non_empty_env(keys: &[&str]) -> Option<OsString> { | ||
| keys.iter() | ||
| .filter_map(std::env::var_os) | ||
| .find(|value| !value.is_empty()) | ||
| } | ||
|
|
||
| fn node_proxy_env_overrides() -> Vec<(&'static str, OsString)> { | ||
| let all_proxy = first_non_empty_env(&["ALL_PROXY", "all_proxy"]); | ||
| let proxy_configured = all_proxy.is_some() | ||
| || NODE_PROXY_PAIRS | ||
| .iter() | ||
| .any(|(upper, lower)| first_non_empty_env(&[upper, lower]).is_some()); | ||
|
|
||
| let mut overrides = Vec::new(); | ||
| if proxy_configured && first_non_empty_env(&[NODE_USE_ENV_PROXY]).is_none() { | ||
| overrides.push((NODE_USE_ENV_PROXY, OsString::from("1"))); | ||
| } | ||
|
|
||
| for (upper, lower) in NODE_PROXY_PAIRS { | ||
| if first_non_empty_env(&[upper]).is_none() | ||
| && let Some(value) = first_non_empty_env(&[lower]).or_else(|| all_proxy.clone()) | ||
| { | ||
| overrides.push((*upper, value)); | ||
| } | ||
| } | ||
|
|
||
| if first_non_empty_env(&["NO_PROXY"]).is_none() | ||
| && let Some(value) = first_non_empty_env(&["no_proxy"]) | ||
| { | ||
| overrides.push(("NO_PROXY", value)); | ||
| } | ||
|
|
||
| overrides | ||
| } |
There was a problem hiding this comment.
Modifying the process-wide environment variables using std::env::set_var and std::env::remove_var in multi-threaded tests is a well-known source of undefined behavior, race conditions, and segmentation faults (especially on Unix-like systems where setenv is not thread-safe). Rust 1.81+ explicitly marked these functions as unsafe for this reason.
To make the tests completely safe, robust, and parallelizable without any locks or unsafe blocks, we can refactor node_proxy_env_overrides to accept a generic environment getter closure. This allows us to write pure unit tests for all proxy-mirroring combinations using mock environments, without ever modifying the real process environment.
fn first_non_empty_env<F>(keys: &[&str], get_env: &F) -> Option<OsString>
where
F: Fn(&str) -> Option<OsString>,
{
keys.iter()
.filter_map(|k| get_env(k))
.find(|value| !value.is_empty())
}
fn node_proxy_env_overrides_impl<F>(get_env: F) -> Vec<(&'static str, OsString)>
where
F: Fn(&str) -> Option<OsString>,
{
let all_proxy = first_non_empty_env(&["ALL_PROXY", "all_proxy"], &get_env);
let proxy_configured = all_proxy.is_some()
|| NODE_PROXY_PAIRS
.iter()
.any(|(upper, lower)| first_non_empty_env(&[upper, lower], &get_env).is_some());
let mut overrides = Vec::new();
if proxy_configured && first_non_empty_env(&[NODE_USE_ENV_PROXY], &get_env).is_none() {
overrides.push((NODE_USE_ENV_PROXY, OsString::from("1")));
}
for (upper, lower) in NODE_PROXY_PAIRS {
if first_non_empty_env(&[upper], &get_env).is_none()
&& let Some(value) = first_non_empty_env(&[lower], &get_env).or_else(|| all_proxy.clone())
{
overrides.push((*upper, value));
}
}
if first_non_empty_env(&["NO_PROXY"], &get_env).is_none()
&& let Some(value) = first_non_empty_env(&["no_proxy"], &get_env)
{
overrides.push(("NO_PROXY", value));
}
overrides
}
fn node_proxy_env_overrides() -> Vec<(&'static str, OsString)> {
node_proxy_env_overrides_impl(std::env::var_os)
}Signed-off-by: cyq <15000851237@163.com>
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Fixes #3273
Summary
js_executionwhen proxy environment variables are presentALL_PROXY, into the uppercase names Node readsNode native env-proxy support is available in recent Node releases; older runtimes ignore
NODE_USE_ENV_PROXYand keep the previous behavior.Verification
cargo fmt --check --allcargo test -p codewhale-tui js_execution -- --nocapturecargo clippy --workspace --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or -A clippy::assertions_on_constantscargo test --workspace --all-features --locked js_execution -- --nocapture