From b37c62544c9cd12bb399832179ff33faafce69cf Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 12 Jun 2026 16:43:05 -0700 Subject: [PATCH] Restore remote stdio MCP cwd fallback --- codex-rs/codex-mcp/src/rmcp_client.rs | 14 ++++- codex-rs/codex-mcp/src/runtime.rs | 76 +++----------------------- codex-rs/config/src/mcp_types.rs | 25 --------- codex-rs/config/src/mcp_types_tests.rs | 56 ------------------- 4 files changed, 21 insertions(+), 150 deletions(-) diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index f4fc8b8072..b41490b94d 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -601,6 +601,18 @@ async fn make_rmcp_client( env_vars, cwd, } => { + let cwd = if is_local_environment { + cwd + } else { + let cwd = cwd.unwrap_or_else(|| runtime_context.stdio_fallback_cwd()); + if !cwd.is_absolute() { + return Err(StartupOutcomeError::from(anyhow!( + "remote stdio MCP server `{server_name}` requires an absolute cwd, got `{}`", + cwd.display() + ))); + } + Some(cwd) + }; let command_os: OsString = command.into(); let args_os: Vec = args.into_iter().map(Into::into).collect(); let env_os = env.map(|env| { @@ -613,7 +625,7 @@ async fn make_rmcp_client( // `ExecutorStdioServerLauncher` once the executor-backed path // preserves `LocalStdioServerLauncher` semantics. Arc::new(LocalStdioServerLauncher::new( - runtime_context.local_stdio_fallback_cwd(), + runtime_context.stdio_fallback_cwd(), )) as Arc } else { let Some(environment) = resolved_environment.as_ref() else { diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 5404bf1eb5..e54c9f2051 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -32,27 +32,24 @@ pub struct SandboxState { /// Runtime context used when resolving per-server MCP environments. /// /// `McpConfig` describes what servers exist. This value carries the canonical -/// environment registry plus the local stdio fallback cwd used when a local -/// stdio server omits its own working directory. +/// environment registry plus the stdio fallback cwd used when a server omits +/// its own working directory. #[derive(Clone)] pub struct McpRuntimeContext { environment_manager: Arc, - local_stdio_fallback_cwd: PathBuf, + stdio_fallback_cwd: PathBuf, } impl McpRuntimeContext { - pub fn new( - environment_manager: Arc, - local_stdio_fallback_cwd: PathBuf, - ) -> Self { + pub fn new(environment_manager: Arc, stdio_fallback_cwd: PathBuf) -> Self { Self { environment_manager, - local_stdio_fallback_cwd, + stdio_fallback_cwd, } } - pub(crate) fn local_stdio_fallback_cwd(&self) -> PathBuf { - self.local_stdio_fallback_cwd.clone() + pub(crate) fn stdio_fallback_cwd(&self) -> PathBuf { + self.stdio_fallback_cwd.clone() } pub(crate) fn resolve_server_environment( @@ -67,9 +64,6 @@ impl McpRuntimeContext { .environment_manager .get_environment(&config.environment_id) { - if !config.is_local_environment() { - ensure_remote_stdio_cwd(server_name, config)?; - } return Ok(Some(environment)); } @@ -89,27 +83,6 @@ impl McpRuntimeContext { } } -fn ensure_remote_stdio_cwd( - server_name: &str, - config: &codex_config::McpServerConfig, -) -> Result<(), String> { - let codex_config::McpServerTransportConfig::Stdio { cwd, .. } = &config.transport else { - return Ok(()); - }; - let Some(cwd) = cwd else { - return Err(format!( - "remote stdio MCP server `{server_name}` requires an absolute cwd" - )); - }; - if cwd.is_absolute() { - return Ok(()); - } - Err(format!( - "remote stdio MCP server `{server_name}` requires an absolute cwd, got `{}`", - cwd.display() - )) -} - pub(crate) fn emit_duration(metric: &str, duration: Duration, tags: &[(&str, &str)]) { if let Some(metrics) = codex_otel::global() { let _ = metrics.record_duration(metric, duration, tags); @@ -233,13 +206,8 @@ mod tests { PathBuf::from("/tmp"), ); - let mut remote_stdio = stdio_server("remote"); - let McpServerTransportConfig::Stdio { cwd, .. } = &mut remote_stdio.transport else { - unreachable!("stdio helper should build stdio transport"); - }; - *cwd = Some(std::env::temp_dir()); for resolved_runtime in [ - runtime_context.resolve_server_environment("stdio", &remote_stdio), + runtime_context.resolve_server_environment("stdio", &stdio_server("remote")), runtime_context.resolve_server_environment("http", &http_server("remote")), ] { let resolved_runtime = match resolved_runtime { @@ -265,32 +233,4 @@ mod tests { }; assert!(resolved_runtime.is_some()); } - - #[tokio::test] - async fn remote_stdio_requires_absolute_cwd() { - let runtime_context = McpRuntimeContext::new( - Arc::new( - EnvironmentManager::create_for_tests( - Some("ws://127.0.0.1:8765".to_string()), - /*local_runtime_paths*/ None, - ) - .await, - ), - PathBuf::from("/tmp"), - ); - let mut remote_stdio = stdio_server("remote"); - let McpServerTransportConfig::Stdio { cwd, .. } = &mut remote_stdio.transport else { - unreachable!("stdio helper should build stdio transport"); - }; - *cwd = Some(PathBuf::from("relative")); - - let error = match runtime_context.resolve_server_environment("stdio", &remote_stdio) { - Ok(_) => panic!("remote stdio MCP should require absolute cwd"), - Err(error) => error, - }; - assert_eq!( - error, - "remote stdio MCP server `stdio` requires an absolute cwd, got `relative`" - ); - } } diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index 0d30ba8b23..ccd344ab39 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -358,7 +358,6 @@ impl TryFrom for McpServerConfig { let environment_id = environment_id.unwrap_or_else(|| DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()); - validate_remote_stdio_cwd(&transport, &environment_id)?; Ok(Self { transport, @@ -395,30 +394,6 @@ const fn default_enabled() -> bool { true } -fn validate_remote_stdio_cwd( - transport: &McpServerTransportConfig, - environment_id: &str, -) -> Result<(), String> { - if environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID { - return Ok(()); - } - let McpServerTransportConfig::Stdio { cwd, .. } = transport else { - return Ok(()); - }; - let Some(cwd) = cwd else { - return Err(format!( - "remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`" - )); - }; - if cwd.is_absolute() { - return Ok(()); - } - Err(format!( - "remote stdio MCP servers require an absolute cwd when environment_id is `{environment_id}`, got `{}`", - cwd.display() - )) -} - #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema)] #[serde(untagged, deny_unknown_fields, rename_all = "snake_case")] pub enum McpServerTransportConfig { diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index 5f3933ce43..be321ba78e 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -51,62 +51,6 @@ fn deserialize_stdio_command_server_config_with_args() { assert!(cfg.enabled); } -#[test] -fn deserialize_remote_stdio_server_requires_absolute_cwd() { - let missing_cwd = toml::from_str::( - r#" - command = "echo" - environment_id = "remote" - "#, - ) - .expect_err("remote stdio MCP should require cwd"); - assert!( - missing_cwd - .to_string() - .contains("remote stdio MCP servers require an absolute cwd"), - "unexpected error: {missing_cwd}" - ); - - let relative_cwd = toml::from_str::( - r#" - command = "echo" - environment_id = "remote" - cwd = "relative" - "#, - ) - .expect_err("remote stdio MCP should require absolute cwd"); - assert!( - relative_cwd.to_string().contains("got `relative`"), - "unexpected error: {relative_cwd}" - ); -} - -#[test] -fn deserialize_remote_stdio_server_accepts_absolute_cwd() { - let cwd = std::env::temp_dir(); - let cfg: McpServerConfig = match toml::from_str(&format!( - r#" - command = "echo" - environment_id = "remote" - cwd = {cwd:?} - "# - )) { - Ok(cfg) => cfg, - Err(error) => panic!("remote stdio MCP should accept absolute cwd: {error}"), - }; - - assert_eq!( - cfg.transport, - McpServerTransportConfig::Stdio { - command: "echo".to_string(), - args: vec![], - env: None, - env_vars: Vec::new(), - cwd: Some(cwd), - } - ); -} - #[test] fn deserialize_stdio_command_server_config_with_arg_with_args_and_env() { let cfg: McpServerConfig = toml::from_str(