From 02f47d3fb36414d99cdf34fff553826d587d1405 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 2 Sep 2026 02:38:27 +0000 Subject: [PATCH] Use native spawning for bare macOS MCP commands (#42192) ## What changed - Extend the macOS MCP launcher to use native spawning for bare command names as well as relative executable paths. - Resolve bare commands against the child's configured `PATH`, including empty entries and the default path when `PATH` is unset, while preserving `argv[0]` and script spelling. - Fall back to the existing command launcher for failed searches and executable text without a shebang so its errors and shell behavior remain unchanged. ## Testing Add macOS coverage for child `PATH` resolution, default-path lookup, launch errors, and shebang-free executable fallback. GitOrigin-RevId: 06e76bf8dbd515331b588da797eb57cb765a546d --- codex-rs/rmcp-client/src/macos_stdio.rs | 75 +++++++++--- codex-rs/rmcp-client/src/macos_stdio_tests.rs | 107 +++++++++++++++--- 2 files changed, 149 insertions(+), 33 deletions(-) diff --git a/codex-rs/rmcp-client/src/macos_stdio.rs b/codex-rs/rmcp-client/src/macos_stdio.rs index 78dc330ce2..af471320f3 100644 --- a/codex-rs/rmcp-client/src/macos_stdio.rs +++ b/codex-rs/rmcp-client/src/macos_stdio.rs @@ -1,11 +1,11 @@ -//! Native spawning for relative macOS MCP executables, without rewriting script paths. +//! Native spawning for macOS MCP executables, without rewriting script paths. //! //! Rust falls back to fork for a historical relative-path/cwd bug in Apple's //! `posix_spawnp`. Calling `posix_spawn` directly avoids that wrapper. This module only //! accepts the launcher's cleared-environment command shape, with piped stdio, -//! a new process group, and default `argv[0]`. PATH lookup and executable files -//! without shebangs retain the existing launcher. Each native child owns its PID -//! until it has been reaped. +//! a new process group, and default `argv[0]`. Bare commands search the child's +//! PATH. Unsuccessful searches and executable files without shebangs retain the +//! existing launcher. Each native child owns its PID until it has been reaped. use std::ffi::CString; use std::ffi::OsStr; @@ -42,12 +42,12 @@ enum ChildKind { } impl LocalChild { - /// Uses native spawning for relative paths, retaining Tokio's fallback for - /// PATH lookup and executable text without a shebang. + /// Uses native spawning for relative paths and bare names, retaining Tokio's + /// fallback for unsuccessful PATH searches and executable text without a shebang. pub(super) fn spawn(mut command: Command) -> io::Result { let program = command.as_std().get_program(); if Path::new(program).is_relative() - && program.as_bytes().contains(&b'/') + && !program.is_empty() && let Some((child, stdin, stdout, stderr)) = NativeChild::spawn(command.as_std())? { return Ok(Self { @@ -115,6 +115,7 @@ impl NativeChild { command: &std::process::Command, ) -> io::Result> { let program = c_string(command.get_program())?; + let search_path = !program.as_bytes().contains(&b'/'); let args = std::iter::once(command.get_program()) .chain(command.get_args()) .map(c_string) @@ -193,18 +194,56 @@ impl NativeChild { &mut attrs.0, (libc::POSIX_SPAWN_SETPGROUP | libc::POSIX_SPAWN_SETSIGDEF) as _, ))?; - libc::posix_spawn( - &mut pid, - program.as_ptr(), - &actions.0, - &attrs.0, - argv.as_ptr(), - envp.as_ptr(), - ) + let mut spawn = |executable: &CString| { + libc::posix_spawn( + &mut pid, + executable.as_ptr(), + &actions.0, + &attrs.0, + argv.as_ptr(), + envp.as_ptr(), + ) + }; + if !search_path { + spawn(&program) + } else { + // posix_spawnp searches the parent's PATH, not envp. Search the + // child's PATH ourselves, using Apple's default when it is unset. + let path = command + .get_envs() + .find(|(key, _)| *key == "PATH") + .and_then(|(_, value)| value) + .unwrap_or(OsStr::new("/usr/bin:/bin")); + let mut result = libc::ENOENT; + for directory in std::env::split_paths(path) { + let mut executable = directory.into_os_string(); + if executable.is_empty() { + executable.push("."); + } + // Preserve the spelling execvp would pass to a shebang + // interpreter, including empty entries and trailing slashes. + executable.push("/"); + executable.push(command.get_program()); + if executable.as_bytes().len() >= libc::PATH_MAX as usize { + return Ok(None); + } + result = spawn(&c_string(&executable)?); + if !matches!( + result, + libc::ENOENT + | libc::ENOTDIR + | libc::EACCES + | libc::ELOOP + | libc::ENAMETOOLONG + ) { + break; + } + } + result + } }; - // execvp also supports executable text files without a shebang. Leave - // that compatibility behavior to the existing Command-based launcher. - if result == libc::ENOEXEC { + // Retain Command's shell fallback and exact PATH search errors. + if result == libc::ENOEXEC || (search_path && result != 0) { return Ok(None); } cvt(result)?; diff --git a/codex-rs/rmcp-client/src/macos_stdio_tests.rs b/codex-rs/rmcp-client/src/macos_stdio_tests.rs index f475ba5e1d..4bf9ed0634 100644 --- a/codex-rs/rmcp-client/src/macos_stdio_tests.rs +++ b/codex-rs/rmcp-client/src/macos_stdio_tests.rs @@ -10,6 +10,72 @@ use std::time::Duration; use tokio::io::AsyncReadExt; use tokio::io::AsyncWriteExt; +async fn native_output(command: Command) -> anyhow::Result { + let mut child = crate::local_child::spawn(command)?; + assert!(matches!(child.inner, ChildKind::Native(_))); + drop(child.stdin.take()); + let mut stdout = child.stdout.take().expect("piped stdout"); + let mut stderr = child.stderr.take().expect("piped stderr"); + let mut output = Vec::new(); + let mut diagnostic = Vec::new(); + let (_, _, status) = tokio::try_join!( + stdout.read_to_end(&mut output), + stderr.read_to_end(&mut diagnostic), + child.wait() + )?; + Ok(std::process::Output { + status, + stdout: output, + stderr: diagnostic, + }) +} + +#[tokio::test] +async fn bare_script_search_matches_child_path_and_preserves_script_spelling() -> anyhow::Result<()> +{ + let root = tempfile::tempdir()?; + let program = std::ffi::OsString::from("server=é"); + let bin = root.path().join("bin"); + let blocked = root.path().join("blocked"); + fs::create_dir(&bin)?; + fs::create_dir(&blocked)?; + fs::write(blocked.join(&program), "not executable")?; + let script = bin.join(&program); + fs::write( + &script, + "#!/bin/sh\nprintf '%s\\n' \"$0\" \"$1\" \"$MCP_TEST\"; /bin/pwd; printf diagnostic >&2; exit 23\n", + )?; + fs::set_permissions(&script, fs::Permissions::from_mode(/*mode*/ 0o755))?; + symlink(&script, root.path().join(&program))?; + for path in [ + bin.into_os_string(), + "bin/".into(), + "missing:blocked:bin".into(), + "missing:".into(), + "".into(), + ] { + let mut command = Command::new(&program); + command + .current_dir(root.path()) + .env_clear() + .env("PATH", path) + .env("MCP_TEST", "kept") + .arg("spaces ; literal $arg"); + let expected = command.output().await?; + assert_eq!(native_output(command).await?, expected); + } + Ok(()) +} + +#[tokio::test] +async fn bare_executable_uses_default_path_and_preserves_argv0() -> anyhow::Result<()> { + let mut command = Command::new("sh"); + command.env_clear().args(["-c", "printf '%s' \"$0\""]); + let expected = command.output().await?; + assert_eq!(native_output(command).await?, expected); + Ok(()) +} + #[tokio::test] async fn relative_script_preserves_paths_stdio_environment_and_process_group() -> anyhow::Result<()> { @@ -137,11 +203,17 @@ async fn launch_failures_preserve_os_errors() -> anyhow::Result<()> { for (program, cwd, errno) in [ ("./missing", root.path().to_path_buf(), libc::ENOENT), ("./not-executable", root.path().to_path_buf(), libc::EACCES), + ("missing", root.path().to_path_buf(), libc::ENOENT), + ("not-executable", root.path().to_path_buf(), libc::EACCES), + ("", root.path().to_path_buf(), libc::ENOENT), ("/bin/sh", root.path().join("missing"), libc::ENOENT), ] { - let mut command = std::process::Command::new(program); - command.env_clear().current_dir(cwd); - let error = NativeChild::spawn(&command) + let mut command = Command::new(program); + command + .env_clear() + .env("PATH", root.path()) + .current_dir(cwd); + let error = crate::local_child::spawn(command) .err() .expect("spawn should fail"); assert_eq!(error.raw_os_error(), Some(errno)); @@ -155,18 +227,23 @@ async fn executable_text_without_shebang_retains_command_fallback() -> anyhow::R let script = root.path().join("server"); fs::write(&script, "printf '%s' \"$0\"\n")?; fs::set_permissions(script, fs::Permissions::from_mode(0o755))?; - let mut command = Command::new("./server"); - command.current_dir(root.path()).env_clear(); - let mut child = crate::local_child::spawn(command)?; - let mut output = Vec::new(); - child - .stdout - .take() - .expect("piped stdout") - .read_to_end(&mut output) - .await?; - assert!(child.wait().await?.success()); - assert_eq!(output, b"./server"); + for program in ["./server", "server"] { + let mut command = Command::new(program); + command + .current_dir(root.path()) + .env_clear() + .env("PATH", "."); + let mut child = crate::local_child::spawn(command)?; + let mut output = Vec::new(); + child + .stdout + .take() + .expect("piped stdout") + .read_to_end(&mut output) + .await?; + assert!(child.wait().await?.success()); + assert_eq!(output, b"./server"); + } Ok(()) }