-
Notifications
You must be signed in to change notification settings - Fork 6.2k
fix(windows-path): preserve PATH order; include core env vars #5579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
eaf1fdf
12ad730
2fcff53
e2d234f
f9787a3
b5eba7c
185cff5
9266740
5a2b5db
e7a1f68
28c5630
ad54e6b
655fdb7
9a08f38
7fd616e
a660432
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,13 +76,37 @@ pub(crate) fn create_env_for_mcp_server( | |
| extra_env: Option<HashMap<String, String>>, | ||
| env_vars: &[String], | ||
| ) -> HashMap<String, String> { | ||
| DEFAULT_ENV_VARS | ||
| #[cfg(windows)] | ||
| let mut map: HashMap<String, String> = DEFAULT_ENV_VARS | ||
| .iter() | ||
| .copied() | ||
| .chain(env_vars.iter().map(String::as_str)) | ||
| .filter_map(|var| env::var(var).ok().map(|value| (var.to_string(), value))) | ||
| .chain(extra_env.unwrap_or_default()) | ||
| .collect() | ||
| .collect(); | ||
|
|
||
| #[cfg(not(windows))] | ||
| let map: HashMap<String, String> = DEFAULT_ENV_VARS | ||
| .iter() | ||
| .copied() | ||
| .chain(env_vars.iter().map(String::as_str)) | ||
| .filter_map(|var| env::var(var).ok().map(|value| (var.to_string(), value))) | ||
| .chain(extra_env.unwrap_or_default()) | ||
| .collect(); | ||
|
|
||
| #[cfg(windows)] | ||
| { | ||
| // Some Windows programs look up the canonical-cased key `Path`. | ||
| // Ensure both `PATH` and `Path` are present with identical values. | ||
| if let Some(path_val) = map.get("PATH").cloned() { | ||
| map.entry("Path".to_string()).or_insert(path_val); | ||
| } else if let Ok(system_path) = env::var("PATH") { | ||
| map.insert("PATH".to_string(), system_path.clone()); | ||
| map.entry("Path".to_string()).or_insert(system_path); | ||
| } | ||
| } | ||
|
|
||
| map | ||
| } | ||
|
|
||
| pub(crate) fn build_default_headers( | ||
|
|
@@ -171,13 +195,33 @@ pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ | |
|
|
||
| #[cfg(windows)] | ||
| pub(crate) const DEFAULT_ENV_VARS: &[&str] = &[ | ||
| // Core path resolution | ||
| "PATH", | ||
| "PATHEXT", | ||
| // Shell and system roots | ||
| "COMSPEC", | ||
| "SYSTEMROOT", | ||
| "SYSTEMDRIVE", | ||
| // User context and profiles | ||
| "USERNAME", | ||
| "USERDOMAIN", | ||
| "USERPROFILE", | ||
| "HOMEDRIVE", | ||
| "HOMEPATH", | ||
| // Program locations | ||
| "PROGRAMFILES", | ||
| "PROGRAMFILES(X86)", | ||
| "PROGRAMW6432", | ||
| "PROGRAMDATA", | ||
| // App data and caches | ||
| "LOCALAPPDATA", | ||
| "APPDATA", | ||
| // Temp locations | ||
| "TEMP", | ||
| "TMP", | ||
| // Common shells/pwsh hints | ||
| "POWERSHELL", | ||
| "PWSH", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cpjet64 if we separate these changes, i'm happy to stamp the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dylan-hurd-oai Happy to do so but was hoping it would pass since my next PR for this issue is virtual shells like Python venv and such and the non-PATH fix is the foundation. I will make this PR into a dedicated path fix in the meantime. Thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not opposed to switching to appending here, but the impacts of the path change are just more subtle, and might impact the success rate of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dylan-hurd-oai Understood and I agree the env var change is definitely more low risk because I didn't consider the apply-patch impact in that manner. I'll split them now and get you a new commit in a few minutes! Later I'll dig more into apply_patch to make sure before I push a new PR for it. Thanks for that! |
||
| ]; | ||
|
|
||
| #[cfg(test)] | ||
|
|
@@ -223,6 +267,16 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(windows)] | ||
| fn windows_path_alias_is_present() { | ||
| let _guard = EnvVarGuard::set("PATH", r"C:\\Windows;C:\\Windows\\System32"); | ||
| let env = create_env_for_mcp_server(None, &[]); | ||
| assert!(env.contains_key("PATH")); | ||
| assert!(env.contains_key("Path")); | ||
| assert_eq!(env.get("PATH"), env.get("Path")); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn create_env_honors_overrides() { | ||
| let value = "custom".to_string(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.