From 12b10a3489092753ee2f83864089efa3f93eb59a Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Wed, 22 Apr 2026 13:41:40 -0700 Subject: [PATCH] Fix relative stdio cwd resolution --- .../app-server/src/codex_message_processor.rs | 4 +- .../codex-mcp/src/mcp_connection_manager.rs | 8 ++- codex-rs/core/tests/suite/rmcp_client.rs | 63 +++++++++++++++++++ codex-rs/rmcp-client/src/program_resolver.rs | 21 ++++--- .../rmcp-client/src/stdio_server_launcher.rs | 29 ++++++--- .../tests/process_group_cleanup.rs | 2 +- codex-rs/rmcp-client/tests/resources.rs | 2 +- 7 files changed, 107 insertions(+), 22 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 51b1c48c83e3..a542ced62184 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -5675,8 +5675,8 @@ impl CodexMessageProcessor { let auth = auth_manager.auth().await; let runtime_environment = match self.thread_manager.environment_manager().current().await { Ok(Some(environment)) => { - // Status listing has no turn cwd. This fallback is used only - // by executor-backed stdio MCPs whose config omits `cwd`. + // Status listing has no turn cwd. This fallback is used by + // stdio MCPs whose config omits `cwd`. McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) } Ok(None) => McpRuntimeEnvironment::new( diff --git a/codex-rs/codex-mcp/src/mcp_connection_manager.rs b/codex-rs/codex-mcp/src/mcp_connection_manager.rs index 10c48e040e85..b7178fdc308b 100644 --- a/codex-rs/codex-mcp/src/mcp_connection_manager.rs +++ b/codex-rs/codex-mcp/src/mcp_connection_manager.rs @@ -666,8 +666,8 @@ pub struct McpConnectionManager { /// servers should run for the current caller. Keep it explicit at manager /// construction time so status/snapshot paths and real sessions make the same /// local-vs-remote decision. `fallback_cwd` is not a per-server override; it is -/// used only when an executor-backed stdio server omits `cwd` and the executor -/// API still needs a concrete process working directory. +/// used when a stdio server omits `cwd` and the launcher needs a concrete +/// process working directory. #[derive(Clone)] pub struct McpRuntimeEnvironment { environment: Arc, @@ -1573,7 +1573,9 @@ async fn make_rmcp_client( runtime_environment.fallback_cwd(), )) } else { - Arc::new(LocalStdioServerLauncher) as Arc + Arc::new(LocalStdioServerLauncher::new( + runtime_environment.fallback_cwd(), + )) as Arc }; // `RmcpClient` always sees a launched MCP stdio server. The diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 7875f09810ab..3d3c43ce64a5 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -616,6 +616,69 @@ async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() - Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +#[serial(mcp_cwd)] +async fn local_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> anyhow::Result<()> +{ + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let server_name = "rmcp_local_fallback_cwd"; + let expected_cwd = Arc::new(Mutex::new(None::)); + let expected_cwd_for_config = Arc::clone(&expected_cwd); + let rmcp_test_server_bin = cargo_bin("test_stdio_server")?; + let relative_server_path = PathBuf::from("mcp-bin").join( + rmcp_test_server_bin + .file_name() + .expect("test stdio server binary should have a file name"), + ); + let relative_command = relative_server_path.to_string_lossy().into_owned(); + + let fixture = test_codex() + .with_config(move |config| { + *expected_cwd_for_config + .lock() + .expect("expected cwd lock should not be poisoned") = + Some(config.cwd.to_path_buf()); + + let target_bin = config.cwd.join(&relative_server_path).into_path_buf(); + let target_dir = target_bin + .parent() + .expect("relative test server path should include a parent"); + fs::create_dir_all(target_dir).expect("create relative MCP bin directory"); + fs::copy(&rmcp_test_server_bin, &target_bin).expect("copy test stdio server"); + + insert_mcp_server( + config, + server_name, + stdio_transport( + relative_command, + Some(HashMap::from([( + "MCP_TEST_VALUE".to_string(), + "local-fallback-cwd".to_string(), + )])), + Vec::new(), + ), + TestMcpServerOptions::default(), + ); + }) + .build(&server) + .await?; + + let expected_cwd = expected_cwd + .lock() + .expect("expected cwd lock should not be poisoned") + .clone() + .expect("test config should record runtime fallback cwd"); + let structured = + call_cwd_tool(&server, &fixture, server_name, "call-local-fallback-cwd").await?; + + assert_cwd_tool_output(&structured, &expected_cwd); + server.verify().await; + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/rmcp-client/src/program_resolver.rs b/codex-rs/rmcp-client/src/program_resolver.rs index c20cac3747b8..60a8af40a8ba 100644 --- a/codex-rs/rmcp-client/src/program_resolver.rs +++ b/codex-rs/rmcp-client/src/program_resolver.rs @@ -12,6 +12,7 @@ use std::collections::HashMap; use std::ffi::OsString; +use std::path::Path; /// Resolves a program to its executable path on Unix systems. /// @@ -19,7 +20,11 @@ use std::ffi::OsString; /// 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 { +pub fn resolve( + program: OsString, + _env: &HashMap, + _cwd: &Path, +) -> std::io::Result { Ok(program) } @@ -33,16 +38,16 @@ pub fn resolve(program: OsString, _env: &HashMap) -> std::io /// 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 = std::env::current_dir() - .map_err(|e| std::io::Error::other(format!("Failed to get current directory: {e}")))?; - +pub fn resolve( + program: OsString, + env: &HashMap, + cwd: &Path, +) -> std::io::Result { // Extract PATH from environment for search locations let search_path = env.get(std::ffi::OsStr::new("PATH")); // Attempt resolution via which crate - match which::which_in(&program, search_path, &cwd) { + match which::which_in(&program, search_path, cwd) { Ok(resolved) => { tracing::debug!("Resolved {program:?} to {resolved:?}"); Ok(resolved.into_os_string()) @@ -119,7 +124,7 @@ mod tests { let program = OsString::from(&env.program_name); // Apply platform-specific resolution - let resolved = resolve(program, &env.mcp_env)?; + let resolved = resolve(program, &env.mcp_env, std::env::current_dir()?.as_path())?; // Verify resolved path executes successfully let mut cmd = Command::new(resolved); diff --git a/codex-rs/rmcp-client/src/stdio_server_launcher.rs b/codex-rs/rmcp-client/src/stdio_server_launcher.rs index 4e9020e36970..b3d3b849d019 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -162,14 +162,27 @@ impl StdioServerCommand { /// process spawns the configured command and rmcp talks to the child's local /// stdin/stdout pipes directly. #[derive(Clone)] -pub struct LocalStdioServerLauncher; +pub struct LocalStdioServerLauncher { + fallback_cwd: PathBuf, +} + +impl LocalStdioServerLauncher { + /// Creates a local stdio launcher. + /// + /// `fallback_cwd` is used when the MCP server config omits `cwd`, so + /// relative commands resolve from the caller's runtime working directory. + pub fn new(fallback_cwd: PathBuf) -> Self { + Self { fallback_cwd } + } +} impl StdioServerLauncher for LocalStdioServerLauncher { fn launch( &self, command: StdioServerCommand, ) -> BoxFuture<'static, io::Result> { - async move { Self::launch_server(command) }.boxed() + let fallback_cwd = self.fallback_cwd.clone(); + async move { Self::launch_server(command, fallback_cwd) }.boxed() } } @@ -193,7 +206,10 @@ mod private { impl private::Sealed for LocalStdioServerLauncher {} impl LocalStdioServerLauncher { - fn launch_server(command: StdioServerCommand) -> io::Result { + fn launch_server( + command: StdioServerCommand, + fallback_cwd: PathBuf, + ) -> io::Result { let StdioServerCommand { program, args, @@ -203,22 +219,21 @@ impl LocalStdioServerLauncher { } = command; let program_name = program.to_string_lossy().into_owned(); let envs = create_env_for_mcp_server(env, &env_vars).map_err(io::Error::other)?; + let cwd = cwd.unwrap_or(fallback_cwd); let resolved_program = - program_resolver::resolve(program, &envs).map_err(io::Error::other)?; + program_resolver::resolve(program, &envs, &cwd).map_err(io::Error::other)?; let mut command = Command::new(resolved_program); command .kill_on_drop(true) .stdin(Stdio::piped()) .stdout(Stdio::piped()) + .current_dir(cwd) .env_clear() .envs(envs) .args(args); #[cfg(unix)] command.process_group(0); - if let Some(cwd) = cwd { - command.current_dir(cwd); - } let (transport, stderr) = TokioChildProcess::builder(command) .stderr(Stdio::piped()) diff --git a/codex-rs/rmcp-client/tests/process_group_cleanup.rs b/codex-rs/rmcp-client/tests/process_group_cleanup.rs index aad28ac0ac88..5f742f98167e 100644 --- a/codex-rs/rmcp-client/tests/process_group_cleanup.rs +++ b/codex-rs/rmcp-client/tests/process_group_cleanup.rs @@ -80,7 +80,7 @@ async fn drop_kills_wrapper_process_group() -> Result<()> { )])), &[], /*cwd*/ None, - Arc::new(LocalStdioServerLauncher), + Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)), ) .await?; diff --git a/codex-rs/rmcp-client/tests/resources.rs b/codex-rs/rmcp-client/tests/resources.rs index b23c34df138e..f2e4c49911bd 100644 --- a/codex-rs/rmcp-client/tests/resources.rs +++ b/codex-rs/rmcp-client/tests/resources.rs @@ -63,7 +63,7 @@ async fn rmcp_client_can_list_and_read_resources() -> anyhow::Result<()> { /*env*/ None, &[], /*cwd*/ None, - Arc::new(LocalStdioServerLauncher), + Arc::new(LocalStdioServerLauncher::new(std::env::current_dir()?)), ) .await?;