pr3828

Author: 25621

codex-rs/Cargo.lock

@@ -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 }

codex-rs/rmcp-client/Cargo.toml

@@ -91,13 +91,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);
@@ -412,3 +419,216 @@ async fn create_oauth_transport_and_runtime(
 
     Ok((transport, runtime))
 }
+
+/// Platform-specific program resolution for MCP server execution.
+///
+/// Resolves executable paths differently based on the operating system:
+/// - **Windows**: Uses `which` crate to find executables including scripts (.cmd, .bat)
+/// - **Unix**: Returns the program unchanged (native PATH resolution handles it)
+mod program_resolver {
+    use super::*;
+    use tracing::debug;
+
+    /// Resolves a program to its executable path for the current platform.
+    ///
+    /// On Windows, `Command::new()` cannot execute scripts (e.g., `.cmd`, `.bat`)
+    /// directly without their extension. This function uses the `which` crate to
+    /// search the `PATH` and find the full path to the executable, including
+    /// any necessary script extensions defined in `PATHEXT`.
+    ///
+    /// On Unix, the kernel handles script execution natively (via shebangs), so
+    /// this function is a no-op and returns the program name unchanged.
+    #[cfg(windows)]
+    pub fn resolve(program: OsString, env: &HashMap<String, String>) -> std::io::Result<OsString> {
+        use std::env;
+
+        // 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)
+            }
+        }
+    }
+
+    /// Unix systems handle PATH resolution natively.
+    ///
+    /// The OS can execute scripts directly, so no resolution needed.
+    #[cfg(unix)]
+    pub fn resolve(program: OsString, _env: &HashMap<String, String>) -> std::io::Result<OsString> {
+        Ok(program)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::program_resolver;
+    use super::*;
+    use anyhow::Result;
+    use std::collections::HashMap;
+    use std::fs;
+    use std::path::Path;
+    use tempfile::TempDir;
+
+    /// 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 = program_resolver::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}")
+            }
+        }
+    }
+}