-
Notifications
You must be signed in to change notification settings - Fork 6.2k
fix: resolve Windows MCP server execution for script-based tools #3828
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
base: main
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
# Preserve PATH precedence & fix Windows MCP env propagation ## Problem & intent Preserve user PATH precedence and reduce Windows setup friction for MCP servers by avoiding PATH reordering and ensuring Windows child processes receive essential env vars. - Addresses: #4180 #5225 #2945 #3245 #3385 #2892 #3310 #3457 #4370 - Supersedes: #4182, #3866, #3828 (overlapping/inferior once this merges) - Notes: #2626 / #2646 are the original PATH-mutation sources being corrected. --- ## Before / After **Before** - PATH was **prepended** with an `apply_patch` helper dir (Rust + Node wrapper), reordering tools and breaking virtualenvs/shims on macOS/Linux. - On Windows, MCP servers missed core env vars and often failed to start without explicit per-server env blocks. **After** - Helper dir is **appended** to PATH (preserves user/tool precedence). - Windows MCP child env now includes common core variables and mirrors `PATH` → `Path`, so typical CLIs/plugins work **without** per-server env blocks. --- ## Scope of change ### `codex-rs/arg0/src/lib.rs` - Append temp/helper dir to `PATH` instead of prepending. ### `codex-cli/bin/codex.js` - Mirror the same append behavior for the Node wrapper. ### `codex-rs/rmcp-client/src/utils.rs` - Expand Windows `DEFAULT_ENV_VARS` (e.g., `COMSPEC`, `SYSTEMROOT`, `PROGRAMFILES*`, `APPDATA`, etc.). - Mirror `PATH` → `Path` for Windows child processes. - Small unit test; conditional `mut` + `clippy` cleanup. --- ## Security effects No broadened privileges. Only environment propagation for well-known Windows keys on stdio MCP child processes. No sandbox policy changes and no network additions. --- ## Testing evidence **Static** - `cargo fmt` - `cargo clippy -p codex-arg0 -D warnings` → **clean** - `cargo clippy -p codex-rmcp-client -D warnings` → **clean** - `cargo test -p codex-rmcp-client` → **13 passed** **Manual** - Local verification on Windows PowerShell 5/7 and WSL (no `unused_mut` warnings on non-Windows targets). --- ## Checklist - [x] Append (not prepend) helper dir to PATH in Rust and Node wrappers - [x] Windows MCP child inherits core env vars; `PATH` mirrored to `Path` - [x] `cargo fmt` / `clippy` clean across touched crates - [x] Unit tests updated/passing where applicable - [x] Cross-platform behavior preserved (macOS/Linux PATH precedence intact)
…#5579) # Preserve PATH precedence & fix Windows MCP env propagation ## Problem & intent Preserve user PATH precedence and reduce Windows setup friction for MCP servers by avoiding PATH reordering and ensuring Windows child processes receive essential env vars. - Addresses: openai#4180 openai#5225 openai#2945 openai#3245 openai#3385 openai#2892 openai#3310 openai#3457 openai#4370 - Supersedes: openai#4182, openai#3866, openai#3828 (overlapping/inferior once this merges) - Notes: openai#2626 / openai#2646 are the original PATH-mutation sources being corrected. --- ## Before / After **Before** - PATH was **prepended** with an `apply_patch` helper dir (Rust + Node wrapper), reordering tools and breaking virtualenvs/shims on macOS/Linux. - On Windows, MCP servers missed core env vars and often failed to start without explicit per-server env blocks. **After** - Helper dir is **appended** to PATH (preserves user/tool precedence). - Windows MCP child env now includes common core variables and mirrors `PATH` → `Path`, so typical CLIs/plugins work **without** per-server env blocks. --- ## Scope of change ### `codex-rs/arg0/src/lib.rs` - Append temp/helper dir to `PATH` instead of prepending. ### `codex-cli/bin/codex.js` - Mirror the same append behavior for the Node wrapper. ### `codex-rs/rmcp-client/src/utils.rs` - Expand Windows `DEFAULT_ENV_VARS` (e.g., `COMSPEC`, `SYSTEMROOT`, `PROGRAMFILES*`, `APPDATA`, etc.). - Mirror `PATH` → `Path` for Windows child processes. - Small unit test; conditional `mut` + `clippy` cleanup. --- ## Security effects No broadened privileges. Only environment propagation for well-known Windows keys on stdio MCP child processes. No sandbox policy changes and no network additions. --- ## Testing evidence **Static** - `cargo fmt` - `cargo clippy -p codex-arg0 -D warnings` → **clean** - `cargo clippy -p codex-rmcp-client -D warnings` → **clean** - `cargo test -p codex-rmcp-client` → **13 passed** **Manual** - Local verification on Windows PowerShell 5/7 and WSL (no `unused_mut` warnings on non-Windows targets). --- ## Checklist - [x] Append (not prepend) helper dir to PATH in Rust and Node wrappers - [x] Windows MCP child inherits core env vars; `PATH` mirrored to `Path` - [x] `cargo fmt` / `clippy` clean across touched crates - [x] Unit tests updated/passing where applicable - [x] Cross-platform behavior preserved (macOS/Linux PATH precedence intact)
|
Thanks for the contribution, and apologies for the slow response. We received a large number of PRs, and we're just now getting through the backlog. It looks like this PR has gone stale. If this PR is still relevant, could you please resolve the merge conflicts and make sure the tests still run? Thanks! |
b12b045 to
5c75710
Compare
…ed tools This ports the fix from the original PR openai#3828 to rmcp-client, which replaced mcp-client in PR openai#5529. On Windows, script-based MCP servers (e.g., npx, pnpm, yarn) that rely on .cmd or .bat files fail to execute. This happens because Rust's Command does not resolve scripts via PATHEXT like the shell does. This fix uses the which crate to resolve the full executable path (including .cmd/.bat extensions) before spawning the process, allowing these tools to run correctly. Changes: - Add which crate dependency to rmcp-client/Cargo.toml - Implement program_resolver module with platform-specific logic: * Windows: Resolves .cmd/.bat scripts using which::which_in * Unix: No-op (OS handles shebangs natively) - Modify new_stdio_client to resolve program path before Command::new - Add unit tests for program resolution on both platforms Fixes: openai#2945
5c75710 to
74e4237
Compare
|
Hi @etraut-openai, Thank you for reviewing! I've resolved the conflicts and updated the PR:
The Windows script resolution logic remains unchanged - using the Ready for merge. Let me know if you need any adjustments! |
|
@25621, looks like there are some lint ( |
…ed tools This ports the fix from the original PR openai#3828 to rmcp-client, which replaced mcp-client in PR openai#5529. On Windows, script-based MCP servers (e.g., npx, pnpm, yarn) that rely on .cmd or .bat files fail to execute. This happens because Rust's Command does not resolve scripts via PATHEXT like the shell does. This fix uses the which crate to resolve the full executable path (including .cmd/.bat extensions) before spawning the process, allowing these tools to run correctly. Changes: - Add which crate dependency to rmcp-client/Cargo.toml - Implement program_resolver module with platform-specific logic: * Windows: Resolves .cmd/.bat scripts using which::which_in * Unix: No-op (OS handles shebangs natively) - Modify new_stdio_client to resolve program path before Command::new - Add unit tests for program resolution on both platforms Fixes: openai#2945
74e4237 to
577b2a6
Compare
What?
Fixes MCP server initialization failures on Windows when using script-based tools like
npx,pnpm, andyarnthat rely on.cmd/.batfiles rather than.exebinaries.Fixes #2945
Why?
Windows users encounter "program not found" errors when configuring MCP servers with commands like
npxin their~/.codex/config.toml. This happens because:npxare batch scripts (npx.cmd) on Windows, not executable binariesstd::process::Commandbypasses the shell and cannot execute these scripts directlyPATHEXTfor executable extensionsWithout this fix, Windows users must specify full paths or add
.cmdextensions manually, which breaks cross-platform compatibility.How?
Added platform-specific program resolution using the
whichcrate to find the correct executable path:.cmd/.batscriptsChanges
which = "6"dependency tomcp-client/Cargo.tomlprogram_resolvermodule inmcp_client.rswith platform-specific resolutionTesting
Added platform-specific tests to verify:
Tested on:
Local checks passed:
Results
Before:
After:
Windows users can now use simple commands like
npxin their config without specifying full paths or extensions. The timeout issue is a separate concern that will be addressed in a follow-up PR.