fix: resolve Windows MCP server execution for script-based tools (#3828)
## What? Fixes MCP server initialization failures on Windows when using script-based tools like `npx`, `pnpm`, and `yarn` that rely on `.cmd`/`.bat` files rather than `.exe` binaries. Fixes #2945 ## Why? Windows users encounter "program not found" errors when configuring MCP servers with commands like `npx` in their `~/.codex/config.toml`. This happens because: - Tools like `npx` are batch scripts (`npx.cmd`) on Windows, not executable binaries - Rust's `std::process::Command` bypasses the shell and cannot execute these scripts directly - The Windows shell normally handles this by checking `PATHEXT` for executable extensions Without this fix, Windows users must specify full paths or add `.cmd` extensions manually, which breaks cross-platform compatibility. ## How? Added platform-specific program resolution using the `which` crate to find the correct executable path: - **Windows**: Resolves programs through PATH/PATHEXT to find `.cmd`/`.bat` scripts - **Unix**: Returns the program unchanged (no-op, as Unix handles scripts natively) ### Changes - Added `which = "6"` dependency to `mcp-client/Cargo.toml` - Implemented `program_resolver` module in `mcp_client.rs` with platform-specific resolution - Added comprehensive tests for both Windows and Unix behavior ### Testing Added platform-specific tests to verify: - Unix systems execute scripts without extensions - Windows fails without proper extensions - Windows succeeds with explicit extensions - Cross-platform resolution enables successful execution **Tested on:** - Windows 11 (NT 10.0.26100.0 x64) - PowerShell 5.1 & 7+, CMD, Git Bash - MCP servers: playwright, context7, supabase - WSL (verified no regression) **Local checks passed:** ```bash cargo test && cargo clippy --tests && cargo fmt -- --config imports_granularity=Item ``` ### Results **Before:** ``` 🖐 MCP client for `playwright` failed to start: program not found ``` **After:** ``` 🖐 MCP client for `playwright` failed to start: request timed out ``` Windows users can now use simple commands like `npx` in their config without specifying full paths or extensions. The timeout issue is a separate concern that will be addressed in a follow-up PR. --------- Co-authored-by: Eric Traut <etraut@openai.com>
This commit is contained in:
parent
326c1e0a7e
commit
f828cd2897
5 changed files with 235 additions and 2 deletions
1
codex-rs/Cargo.lock
generated
1
codex-rs/Cargo.lock
generated
|
|
@ -1427,6 +1427,7 @@ dependencies = [
|
|||
"tracing",
|
||||
"urlencoding",
|
||||
"webbrowser",
|
||||
"which",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
|
|
|||
|
|
@ -56,6 +56,7 @@ tokio = { workspace = true, features = [
|
|||
tracing = { workspace = true, features = ["log"] }
|
||||
urlencoding = { workspace = true }
|
||||
webbrowser = { workspace = true }
|
||||
which = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
escargot = { workspace = true }
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ mod find_codex_home;
|
|||
mod logging_client_handler;
|
||||
mod oauth;
|
||||
mod perform_oauth_login;
|
||||
mod program_resolver;
|
||||
mod rmcp_client;
|
||||
mod utils;
|
||||
|
||||
|
|
|
|||
222
codex-rs/rmcp-client/src/program_resolver.rs
Normal file
222
codex-rs/rmcp-client/src/program_resolver.rs
Normal file
|
|
@ -0,0 +1,222 @@
|
|||
//! Platform-specific program resolution for MCP server execution.
|
||||
//!
|
||||
//! This module provides a unified interface for resolving executable paths
|
||||
//! across different operating systems. The key challenge it addresses is that
|
||||
//! Windows cannot execute script files (e.g., `.cmd`, `.bat`) directly through
|
||||
//! `Command::new()` without their file extensions, while Unix systems handle
|
||||
//! scripts natively through shebangs.
|
||||
//!
|
||||
//! The `resolve` function abstracts these platform differences:
|
||||
//! - On Unix: Returns the program unchanged (OS handles script execution)
|
||||
//! - On Windows: Uses the `which` crate to resolve full paths including extensions
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::ffi::OsString;
|
||||
|
||||
#[cfg(windows)]
|
||||
use std::env;
|
||||
#[cfg(windows)]
|
||||
use tracing::debug;
|
||||
|
||||
/// Resolves a program to its executable path on Unix systems.
|
||||
///
|
||||
/// Unix systems handle PATH resolution and script execution natively through
|
||||
/// the kernel's shebang (`#!`) mechanism, so this function simply returns
|
||||
/// the program name unchanged.
|
||||
#[cfg(unix)]
|
||||
pub fn resolve(program: OsString, _env: &HashMap<String, String>) -> std::io::Result<OsString> {
|
||||
Ok(program)
|
||||
}
|
||||
|
||||
/// Resolves a program to its executable path on Windows systems.
|
||||
///
|
||||
/// Windows requires explicit file extensions for script execution. This function
|
||||
/// uses the `which` crate to search the `PATH` environment variable and find
|
||||
/// the full path to the executable, including necessary script extensions
|
||||
/// (`.cmd`, `.bat`, etc.) defined in `PATHEXT`.
|
||||
///
|
||||
/// This enables tools like `npx`, `pnpm`, and `yarn` to work correctly on Windows
|
||||
/// without requiring users to specify full paths or extensions in their configuration.
|
||||
#[cfg(windows)]
|
||||
pub fn resolve(program: OsString, env: &HashMap<String, String>) -> std::io::Result<OsString> {
|
||||
// Get current directory for relative path resolution
|
||||
let cwd = env::current_dir()
|
||||
.map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?;
|
||||
|
||||
// Extract PATH from environment for search locations
|
||||
let search_path = env.get("PATH");
|
||||
|
||||
// Attempt resolution via which crate
|
||||
match which::which_in(&program, search_path, &cwd) {
|
||||
Ok(resolved) => {
|
||||
debug!("Resolved {:?} to {:?}", program, resolved);
|
||||
Ok(resolved.into_os_string())
|
||||
}
|
||||
Err(e) => {
|
||||
debug!(
|
||||
"Failed to resolve {:?}: {}. Using original path",
|
||||
program, e
|
||||
);
|
||||
// Fallback to original program - let Command::new() handle the error
|
||||
Ok(program)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::utils::create_env_for_mcp_server;
|
||||
use anyhow::Result;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use tempfile::TempDir;
|
||||
use tokio::process::Command;
|
||||
|
||||
/// Unix: Verifies the OS handles script execution without file extensions.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn test_unix_executes_script_without_extension() -> Result<()> {
|
||||
let env = TestExecutableEnv::new()?;
|
||||
let mut cmd = Command::new(&env.program_name);
|
||||
cmd.envs(&env.mcp_env);
|
||||
|
||||
let output = cmd.output().await;
|
||||
assert!(output.is_ok(), "Unix should execute scripts directly");
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Windows: Verifies scripts fail to execute without the proper extension.
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn test_windows_fails_without_extension() -> Result<()> {
|
||||
let env = TestExecutableEnv::new()?;
|
||||
let mut cmd = Command::new(&env.program_name);
|
||||
cmd.envs(&env.mcp_env);
|
||||
|
||||
let output = cmd.output().await;
|
||||
assert!(
|
||||
output.is_err(),
|
||||
"Windows requires .cmd/.bat extension for direct execution"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Windows: Verifies scripts with an explicit extension execute correctly.
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn test_windows_succeeds_with_extension() -> Result<()> {
|
||||
let env = TestExecutableEnv::new()?;
|
||||
// Append the `.cmd` extension to the program name
|
||||
let program_with_ext = format!("{}.cmd", env.program_name);
|
||||
let mut cmd = Command::new(&program_with_ext);
|
||||
cmd.envs(&env.mcp_env);
|
||||
|
||||
let output = cmd.output().await;
|
||||
assert!(
|
||||
output.is_ok(),
|
||||
"Windows should execute scripts when the extension is provided"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Verifies program resolution enables successful execution on all platforms.
|
||||
#[tokio::test]
|
||||
async fn test_resolved_program_executes_successfully() -> Result<()> {
|
||||
let env = TestExecutableEnv::new()?;
|
||||
let program = OsString::from(&env.program_name);
|
||||
|
||||
// Apply platform-specific resolution
|
||||
let resolved = resolve(program, &env.mcp_env)?;
|
||||
|
||||
// Verify resolved path executes successfully
|
||||
let mut cmd = Command::new(resolved);
|
||||
cmd.envs(&env.mcp_env);
|
||||
let output = cmd.output().await;
|
||||
|
||||
assert!(
|
||||
output.is_ok(),
|
||||
"Resolved program should execute successfully"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// Test fixture for creating temporary executables in a controlled environment.
|
||||
struct TestExecutableEnv {
|
||||
// Held to prevent the temporary directory from being deleted.
|
||||
_temp_dir: TempDir,
|
||||
program_name: String,
|
||||
mcp_env: HashMap<String, String>,
|
||||
}
|
||||
|
||||
impl TestExecutableEnv {
|
||||
const TEST_PROGRAM: &'static str = "test_mcp_server";
|
||||
|
||||
fn new() -> Result<Self> {
|
||||
let temp_dir = TempDir::new()?;
|
||||
let dir_path = temp_dir.path();
|
||||
|
||||
Self::create_executable(dir_path)?;
|
||||
|
||||
// Build a clean environment with the temp dir in the PATH.
|
||||
let mut extra_env = HashMap::new();
|
||||
extra_env.insert("PATH".to_string(), Self::build_path(dir_path));
|
||||
|
||||
#[cfg(windows)]
|
||||
extra_env.insert("PATHEXT".to_string(), Self::ensure_cmd_extension());
|
||||
|
||||
let mcp_env = create_env_for_mcp_server(Some(extra_env), &[]);
|
||||
|
||||
Ok(Self {
|
||||
_temp_dir: temp_dir,
|
||||
program_name: Self::TEST_PROGRAM.to_string(),
|
||||
mcp_env,
|
||||
})
|
||||
}
|
||||
|
||||
/// Creates a simple, platform-specific executable script.
|
||||
fn create_executable(dir: &Path) -> Result<()> {
|
||||
#[cfg(windows)]
|
||||
{
|
||||
let file = dir.join(format!("{}.cmd", Self::TEST_PROGRAM));
|
||||
fs::write(&file, "@echo off\nexit 0")?;
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let file = dir.join(Self::TEST_PROGRAM);
|
||||
fs::write(&file, "#!/bin/sh\nexit 0")?;
|
||||
Self::set_executable(&file)?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn set_executable(path: &Path) -> Result<()> {
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
let mut perms = fs::metadata(path)?.permissions();
|
||||
perms.set_mode(0o755);
|
||||
fs::set_permissions(path, perms)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Prepends the given directory to the system's PATH variable.
|
||||
fn build_path(dir: &Path) -> String {
|
||||
let current = std::env::var("PATH").unwrap_or_default();
|
||||
let sep = if cfg!(windows) { ";" } else { ":" };
|
||||
format!("{}{sep}{current}", dir.to_string_lossy())
|
||||
}
|
||||
|
||||
/// Ensures `.CMD` is in the `PATHEXT` variable on Windows for script discovery.
|
||||
#[cfg(windows)]
|
||||
fn ensure_cmd_extension() -> String {
|
||||
let current = std::env::var("PATHEXT").unwrap_or_default();
|
||||
if current.to_uppercase().contains(".CMD") {
|
||||
current
|
||||
} else {
|
||||
format!(".CMD;{current}")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -47,6 +47,7 @@ use crate::logging_client_handler::LoggingClientHandler;
|
|||
use crate::oauth::OAuthCredentialsStoreMode;
|
||||
use crate::oauth::OAuthPersistor;
|
||||
use crate::oauth::StoredOAuthTokens;
|
||||
use crate::program_resolver;
|
||||
use crate::utils::apply_default_headers;
|
||||
use crate::utils::build_default_headers;
|
||||
use crate::utils::convert_call_tool_result;
|
||||
|
|
@ -91,13 +92,20 @@ impl RmcpClient {
|
|||
cwd: Option<PathBuf>,
|
||||
) -> io::Result<Self> {
|
||||
let program_name = program.to_string_lossy().into_owned();
|
||||
let mut command = Command::new(&program);
|
||||
|
||||
// Build environment for program resolution and subprocess
|
||||
let envs = create_env_for_mcp_server(env, env_vars);
|
||||
|
||||
// Resolve program to executable path (platform-specific)
|
||||
let resolved_program = program_resolver::resolve(program, &envs)?;
|
||||
|
||||
let mut command = Command::new(resolved_program);
|
||||
command
|
||||
.kill_on_drop(true)
|
||||
.stdin(Stdio::piped())
|
||||
.stdout(Stdio::piped())
|
||||
.env_clear()
|
||||
.envs(create_env_for_mcp_server(env, env_vars))
|
||||
.envs(envs)
|
||||
.args(&args);
|
||||
if let Some(cwd) = cwd {
|
||||
command.current_dir(cwd);
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue