From f828cd28972c358b6677d2d6ccb5e427f62e93a2 Mon Sep 17 00:00:00 2001 From: Joonsoo Lee Date: Mon, 17 Nov 2025 06:41:10 +0900 Subject: [PATCH] fix: resolve Windows MCP server execution for script-based tools (#3828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- codex-rs/Cargo.lock | 1 + codex-rs/rmcp-client/Cargo.toml | 1 + codex-rs/rmcp-client/src/lib.rs | 1 + codex-rs/rmcp-client/src/program_resolver.rs | 222 +++++++++++++++++++ codex-rs/rmcp-client/src/rmcp_client.rs | 12 +- 5 files changed, 235 insertions(+), 2 deletions(-) create mode 100644 codex-rs/rmcp-client/src/program_resolver.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 38f015792..c3039453b 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1427,6 +1427,7 @@ dependencies = [ "tracing", "urlencoding", "webbrowser", + "which", ] [[package]] diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index e9f832e65..68ef4509b 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -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 } diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index ca99a7bb9..87ce86b46 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -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; diff --git a/codex-rs/rmcp-client/src/program_resolver.rs b/codex-rs/rmcp-client/src/program_resolver.rs new file mode 100644 index 000000000..e62336e26 --- /dev/null +++ b/codex-rs/rmcp-client/src/program_resolver.rs @@ -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) -> std::io::Result { + 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) -> std::io::Result { + // 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, + } + + impl TestExecutableEnv { + const TEST_PROGRAM: &'static str = "test_mcp_server"; + + fn new() -> Result { + 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}") + } + } + } +} diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index bc1980f1f..f21859dc9 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -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, ) -> io::Result { 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);