From 298e5cfce18a51ddcbf31c41af03c7e1a5f4a965 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 21 May 2026 08:19:54 -0700 Subject: [PATCH] Route MCP servers through explicit environments (#23583) ## Summary - route each configured MCP server through an explicit per-server `environment_id` instead of a manager-wide remote toggle - default omitted `environment_id` to `local`, resolve named ids through `EnvironmentManager`, and fail only the affected MCP server when an explicit id is unknown - keep local stdio on the existing local launcher path for now, while named-environment stdio uses the selected environment backend and requires an absolute `cwd` - allow local HTTP MCP servers to keep using the ambient HTTP client when no local `Environment` is configured; named-environment HTTP MCPs use that environment's HTTP client ## Validation - devbox Bazel build: `bazel build --bes_backend= --bes_results_url= //codex-rs/cli:codex //codex-rs/rmcp-client:test_stdio_server //codex-rs/rmcp-client:test_streamable_http_server` - devbox app-server config matrix with real `config.toml` / `environments.toml` files covering omitted local, explicit local, omitted local under remote default, explicit remote stdio, local HTTP without local env, explicit remote HTTP, local stdio without local env, unknown explicit env, and remote stdio without `cwd` --- codex-rs/app-server/src/request_processors.rs | 3 +- .../src/request_processors/apps_processor.rs | 2 +- .../request_processors/config_processor.rs | 2 +- .../src/request_processors/mcp_processor.rs | 64 ++-- .../src/request_processors/plugins.rs | 21 +- codex-rs/cli/src/mcp_cmd.rs | 2 +- codex-rs/codex-mcp/src/connection_manager.rs | 6 +- .../codex-mcp/src/connection_manager_tests.rs | 14 +- codex-rs/codex-mcp/src/lib.rs | 2 +- codex-rs/codex-mcp/src/mcp/mod.rs | 19 +- codex-rs/codex-mcp/src/mcp/mod_tests.rs | 4 +- codex-rs/codex-mcp/src/rmcp_client.rs | 71 ++--- codex-rs/codex-mcp/src/runtime.rs | 297 ++++++++++-------- codex-rs/config/src/lib.rs | 1 + codex-rs/config/src/mcp_edit.rs | 4 +- codex-rs/config/src/mcp_edit_tests.rs | 4 +- codex-rs/config/src/mcp_types.rs | 46 ++- codex-rs/config/src/mcp_types_tests.rs | 58 +++- codex-rs/core-plugins/src/manager_tests.rs | 8 +- codex-rs/core/config.schema.json | 2 +- codex-rs/core/src/config/config_tests.rs | 40 +-- codex-rs/core/src/config/edit.rs | 4 +- codex-rs/core/src/config/edit_tests.rs | 14 +- codex-rs/core/src/connectors.rs | 12 +- codex-rs/core/src/mcp_skill_dependencies.rs | 4 +- codex-rs/core/src/mcp_tool_call_tests.rs | 17 +- codex-rs/core/src/session/mcp.rs | 16 +- codex-rs/core/src/session/mod.rs | 2 +- codex-rs/core/src/session/session.rs | 14 +- codex-rs/core/tests/suite/code_mode.rs | 2 +- codex-rs/core/tests/suite/hooks_mcp.rs | 2 +- codex-rs/core/tests/suite/rmcp_client.rs | 100 ++---- codex-rs/core/tests/suite/search_tool.rs | 6 +- codex-rs/core/tests/suite/sqlite_state.rs | 2 +- codex-rs/core/tests/suite/truncation.rs | 6 +- .../rmcp-client/src/stdio_server_launcher.rs | 23 +- 36 files changed, 463 insertions(+), 431 deletions(-) diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index 838dd8adf..311ab29d5 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -340,12 +340,11 @@ use codex_login::complete_device_code_login; use codex_login::login_with_api_key; use codex_login::request_device_code; use codex_login::run_login_server; -use codex_mcp::McpRuntimeEnvironment; +use codex_mcp::McpRuntimeContext; use codex_mcp::McpServerStatusSnapshot; use codex_mcp::McpSnapshotDetail; use codex_mcp::collect_mcp_server_status_snapshot_with_detail; use codex_mcp::discover_supported_scopes; -use codex_mcp::effective_mcp_servers; use codex_mcp::read_mcp_resource as read_mcp_resource_without_thread; use codex_mcp::resolve_oauth_scopes; use codex_memories_write::clear_memory_roots_contents; diff --git a/codex-rs/app-server/src/request_processors/apps_processor.rs b/codex-rs/app-server/src/request_processors/apps_processor.rs index 5a1c9ffe1..49a6615f1 100644 --- a/codex-rs/app-server/src/request_processors/apps_processor.rs +++ b/codex-rs/app-server/src/request_processors/apps_processor.rs @@ -171,7 +171,7 @@ impl AppsRequestProcessor { connectors::list_accessible_connectors_from_mcp_tools_with_environment_manager( &accessible_config, force_refetch, - &environment_manager, + Arc::clone(&environment_manager), ) .await .map_err(|err| format!("failed to load accessible apps: {err}")); diff --git a/codex-rs/app-server/src/request_processors/config_processor.rs b/codex-rs/app-server/src/request_processors/config_processor.rs index dd6e8e3c9..8f50a3c21 100644 --- a/codex-rs/app-server/src/request_processors/config_processor.rs +++ b/codex-rs/app-server/src/request_processors/config_processor.rs @@ -220,7 +220,7 @@ impl ConfigRequestProcessor { connectors::list_accessible_connectors_from_mcp_tools_with_environment_manager( &config, /*force_refetch*/ true, - &environment_manager, + Arc::clone(&environment_manager), ), ); let all_connectors = match all_connectors_result { diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 4bb9431f4..2d60ed135 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -205,24 +205,20 @@ impl McpRequestProcessor { .await; let auth = self.auth_manager.auth().await; let environment_manager = self.thread_manager.environment_manager(); - // Status listing has no turn cwd. Prefer the configured default env, - // then configured local if present; do not manufacture a hidden local - // env in no-local modes. - let runtime_environment = McpRuntimeEnvironment::new( - environment_manager.default_or_local_environment(), - environment_manager.try_local_environment(), - config.cwd.to_path_buf(), - ); + // This threadless status path has no turn cwd or turn-selected + // environment. Use config cwd only as the local stdio fallback; named + // environment stdio MCPs must declare their own absolute cwd. + let runtime_context = + McpRuntimeContext::new(Arc::clone(&environment_manager), config.cwd.to_path_buf()); tokio::spawn(async move { Self::list_mcp_server_status_task( outgoing, request, params, - config, mcp_config, auth, - runtime_environment, + runtime_context, ) .await; }); @@ -233,18 +229,16 @@ impl McpRequestProcessor { outgoing: Arc, request_id: ConnectionRequestId, params: ListMcpServerStatusParams, - config: Config, mcp_config: codex_mcp::McpConfig, auth: Option, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, ) { let result = Self::list_mcp_server_status_response( request_id.request_id.to_string(), params, - config, mcp_config, auth, - runtime_environment, + runtime_context, ) .await; outgoing.send_result(request_id, result).await; @@ -253,10 +247,9 @@ impl McpRequestProcessor { async fn list_mcp_server_status_response( request_id: String, params: ListMcpServerStatusParams, - config: Config, mcp_config: codex_mcp::McpConfig, auth: Option, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, ) -> Result { let detail = match params.detail.unwrap_or(McpServerStatusDetail::Full) { McpServerStatusDetail::Full => McpSnapshotDetail::Full, @@ -267,31 +260,25 @@ impl McpRequestProcessor { &mcp_config, auth.as_ref(), request_id, - runtime_environment, + runtime_context, detail, ) .await; - let effective_servers = effective_mcp_servers(&mcp_config, auth.as_ref()); let McpServerStatusSnapshot { tools_by_server, resources, resource_templates, auth_statuses, + mut server_names, } = snapshot; - - let mut server_names: Vec = config - .mcp_servers - .keys() - .cloned() - // Include runtime-added/plugin MCP servers that are present in the - // effective runtime config even when they are not user-declared in - // `config.mcp_servers`. - .chain(effective_servers.keys().cloned()) - .chain(auth_statuses.keys().cloned()) - .chain(resources.keys().cloned()) - .chain(resource_templates.keys().cloned()) - .collect(); + server_names.extend( + auth_statuses + .keys() + .cloned() + .chain(resources.keys().cloned()) + .chain(resource_templates.keys().cloned()), + ); server_names.sort(); server_names.dedup(); @@ -367,21 +354,18 @@ impl McpRequestProcessor { .await; let auth = self.auth_manager.auth().await; let environment_manager = self.thread_manager.environment_manager(); - // Resource reads without a thread have no turn cwd. Prefer the - // configured default env, then configured local if present; do not - // manufacture a hidden local env in no-local modes. - let runtime_environment = McpRuntimeEnvironment::new( - environment_manager.default_or_local_environment(), - environment_manager.try_local_environment(), - config.cwd.to_path_buf(), - ); + // This threadless resource-read path has no turn cwd or turn-selected + // environment. Use config cwd only as the local stdio fallback; named + // environment stdio MCPs must declare their own absolute cwd. + let runtime_context = + McpRuntimeContext::new(Arc::clone(&environment_manager), config.cwd.to_path_buf()); let request_id = request_id.clone(); tokio::spawn(async move { let result = read_mcp_resource_without_thread( &mcp_config, auth.as_ref(), - runtime_environment, + runtime_context, &server, &uri, ) diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 61fd363b2..fd45a8a73 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -959,9 +959,12 @@ impl PluginRequestProcessor { None => None, }; let environment_manager = self.thread_manager.environment_manager(); - let app_summaries = - load_plugin_app_summaries(&config, &outcome.plugin.apps, &environment_manager) - .await; + let app_summaries = load_plugin_app_summaries( + &config, + &outcome.plugin.apps, + Arc::clone(&environment_manager), + ) + .await; let visible_skills = outcome .plugin .skills @@ -1037,8 +1040,12 @@ impl PluginRequestProcessor { .map(codex_plugin::AppConnectorId) .collect::>(); let environment_manager = self.thread_manager.environment_manager(); - let app_summaries = - load_plugin_app_summaries(&config, &plugin_apps, &environment_manager).await; + let app_summaries = load_plugin_app_summaries( + &config, + &plugin_apps, + Arc::clone(&environment_manager), + ) + .await; remote_plugin_detail_to_info(remote_detail, app_summaries) } }; @@ -1506,7 +1513,7 @@ impl PluginRequestProcessor { connectors::list_accessible_connectors_from_mcp_tools_with_environment_manager( config, /*force_refetch*/ true, - &environment_manager + Arc::clone(&environment_manager) ), ); @@ -1772,7 +1779,7 @@ impl PluginRequestProcessor { async fn load_plugin_app_summaries( config: &Config, plugin_apps: &[codex_plugin::AppConnectorId], - environment_manager: &EnvironmentManager, + environment_manager: Arc, ) -> Vec { if plugin_apps.is_empty() { return Vec::new(); diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 5c66a27d8..f52d24160 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -301,7 +301,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re let new_entry = McpServerConfig { transport: transport.clone(), - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 8144f5b22..f647aee1b 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -27,7 +27,7 @@ use crate::rmcp_client::MCP_TOOLS_LIST_DURATION_METRIC; use crate::rmcp_client::ManagedClient; use crate::rmcp_client::StartupOutcomeError; use crate::rmcp_client::list_tools_for_client_uncached; -use crate::runtime::McpRuntimeEnvironment; +use crate::runtime::McpRuntimeContext; use crate::runtime::emit_duration; use crate::server::EffectiveMcpServer; use crate::server::McpServerMetadata; @@ -176,7 +176,7 @@ impl McpConnectionManager { submit_id: String, tx_event: Sender, initial_permission_profile: PermissionProfile, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, codex_home: PathBuf, codex_apps_tools_cache_key: CodexAppsToolsCacheKey, host_owned_codex_apps_enabled: bool, @@ -248,7 +248,7 @@ impl McpConnectionManager { elicitation_requests.clone(), codex_apps_tools_cache_context, Arc::clone(&tool_plugin_provenance), - runtime_environment.clone(), + runtime_context.clone(), runtime_auth_provider, client_elicitation_capability.clone(), ); diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 952bc8207..181d95d0e 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -18,6 +18,7 @@ use crate::tools::normalize_tools_for_model; use crate::tools::tool_with_model_visible_input_schema; use codex_config::Constrained; use codex_config::McpServerConfig; +use codex_exec_server::EnvironmentManager; use codex_protocol::ToolName; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::GranularApprovalConfig; @@ -864,7 +865,7 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -889,7 +890,7 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -915,9 +916,8 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { String::new(), tx_event, PermissionProfile::default(), - McpRuntimeEnvironment::new( - /*environment*/ None, - /*local_environment*/ None, + McpRuntimeContext::new( + Arc::new(EnvironmentManager::without_environments()), PathBuf::from("/tmp"), ), codex_home.path().to_path_buf(), @@ -988,7 +988,7 @@ fn mcp_init_error_display_prompts_for_github_pat() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -1041,7 +1041,7 @@ fn mcp_init_error_display_reports_generic_errors() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 9d1fe223d..7b45bcc7a 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -3,7 +3,7 @@ pub use elicitation::ElicitationReviewRequest; pub use elicitation::ElicitationReviewer; pub use elicitation::ElicitationReviewerHandle; pub use rmcp_client::MCP_SANDBOX_STATE_META_CAPABILITY; -pub use runtime::McpRuntimeEnvironment; +pub use runtime::McpRuntimeContext; pub use runtime::SandboxState; pub use tools::ToolInfo; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 3d1969de0..1a06d9b9a 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -38,7 +38,7 @@ use serde_json::Value; use crate::codex_apps::codex_apps_tools_cache_key; use crate::connection_manager::McpConnectionManager; -use crate::runtime::McpRuntimeEnvironment; +use crate::runtime::McpRuntimeContext; use crate::server::EffectiveMcpServer; pub const CODEX_APPS_MCP_SERVER_NAME: &str = "codex_apps"; @@ -261,7 +261,7 @@ pub fn tool_plugin_provenance(config: &McpConfig) -> ToolPluginProvenance { pub async fn read_mcp_resource( config: &McpConfig, auth: Option<&CodexAuth>, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, server: &str, uri: &str, ) -> anyhow::Result { @@ -284,7 +284,7 @@ pub async fn read_mcp_resource( String::new(), tx_event, PermissionProfile::default(), - runtime_environment, + runtime_context, config.codex_home.clone(), codex_apps_tools_cache_key(auth), host_owned_codex_apps_enabled, @@ -314,13 +314,14 @@ pub struct McpServerStatusSnapshot { pub resources: HashMap>, pub resource_templates: HashMap>, pub auth_statuses: HashMap, + pub server_names: Vec, } pub async fn collect_mcp_server_status_snapshot_with_detail( config: &McpConfig, auth: Option<&CodexAuth>, submit_id: String, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, detail: McpSnapshotDetail, ) -> McpServerStatusSnapshot { let mcp_servers = effective_mcp_servers(config, auth); @@ -332,6 +333,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( resources: HashMap::new(), resource_templates: HashMap::new(), auth_statuses: HashMap::new(), + server_names: Vec::new(), }; } @@ -342,6 +344,8 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( ) .await; + let server_names = mcp_servers.keys().cloned().collect(); + let (tx_event, rx_event) = unbounded(); drop(rx_event); @@ -353,7 +357,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( submit_id, tx_event, PermissionProfile::default(), - runtime_environment, + runtime_context, config.codex_home.clone(), codex_apps_tools_cache_key(auth), host_owned_codex_apps_enabled, @@ -367,6 +371,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( let snapshot = collect_mcp_server_status_snapshot_from_manager( &mcp_connection_manager, auth_status_entries, + server_names, detail, ) .await; @@ -451,7 +456,7 @@ fn codex_apps_mcp_server_config(config: &McpConfig) -> McpServerConfig { http_headers, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -575,6 +580,7 @@ fn convert_mcp_resource_templates( async fn collect_mcp_server_status_snapshot_from_manager( mcp_connection_manager: &McpConnectionManager, auth_status_entries: HashMap, + server_names: Vec, detail: McpSnapshotDetail, ) -> McpServerStatusSnapshot { let (tools, resources, resource_templates) = tokio::join!( @@ -613,6 +619,7 @@ async fn collect_mcp_server_status_snapshot_from_manager( resources: convert_mcp_resources(resources), resource_templates: convert_mcp_resource_templates(resource_templates), auth_statuses: auth_statuses_from_entries(&auth_status_entries), + server_names, } } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 7566bf9bd..a8260de6c 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -316,7 +316,7 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -341,7 +341,7 @@ async fn effective_mcp_servers_preserve_user_servers_and_add_codex_apps() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index 687576dfa..c75895842 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -28,7 +28,7 @@ use crate::codex_apps::write_cached_codex_apps_tools_if_needed; use crate::elicitation::ElicitationRequestManager; use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::mcp::ToolPluginProvenance; -use crate::runtime::McpRuntimeEnvironment; +use crate::runtime::McpRuntimeContext; use crate::runtime::emit_duration; use crate::server::EffectiveMcpServer; use crate::server::McpServerLaunch; @@ -142,7 +142,7 @@ impl AsyncManagedClient { elicitation_requests: ElicitationRequestManager, codex_apps_tools_cache_context: Option, tool_plugin_provenance: Arc, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, runtime_auth_provider: Option, client_elicitation_capability: ElicitationCapability, ) -> Self { @@ -170,7 +170,7 @@ impl AsyncManagedClient { &server_name, server.clone(), store_mode, - runtime_environment, + runtime_context, runtime_auth_provider, ) .await?, @@ -560,29 +560,17 @@ async fn make_rmcp_client( server_name: &str, server: EffectiveMcpServer, store_mode: OAuthCredentialsStoreMode, - runtime_environment: McpRuntimeEnvironment, + runtime_context: McpRuntimeContext, runtime_auth_provider: Option, ) -> Result { let config = match server.launch() { McpServerLaunch::Configured(config) => config.as_ref().clone(), }; - if let Some(reason) = runtime_environment.startup_unavailable_reason(server_name, &config) { - return Err(StartupOutcomeError::from(anyhow!(reason))); - } - let McpServerConfig { - transport, - experimental_environment, - .. - } = config; - let remote_environment = match experimental_environment.as_deref() { - None | Some("local") => false, - Some("remote") => true, - Some(environment) => { - return Err(StartupOutcomeError::from(anyhow!( - "unsupported experimental_environment `{environment}` for MCP server `{server_name}`" - ))); - } - }; + let resolved_environment = runtime_context + .resolve_server_environment(server_name, &config) + .map_err(|err| StartupOutcomeError::from(anyhow!(err)))?; + let is_local_environment = config.is_local_environment(); + let McpServerConfig { transport, .. } = config; match transport { McpServerTransportConfig::Stdio { @@ -599,27 +587,24 @@ async fn make_rmcp_client( .map(|(key, value)| (key.into(), value.into())) .collect::>() }); - let launcher = if remote_environment { - // Preflight should reject this first, but keep client startup - // defensive if optional runtime placement is mis-threaded. - let Some(environment) = runtime_environment.environment() else { - return Err(StartupOutcomeError::from(anyhow!( - "remote MCP server requires a runtime environment" - ))); + let launcher = if is_local_environment { + // TODO(starr): Unify local stdio MCP launch with + // `ExecutorStdioServerLauncher` once the executor-backed path + // preserves `LocalStdioServerLauncher` semantics. + Arc::new(LocalStdioServerLauncher::new( + runtime_context.local_stdio_fallback_cwd(), + )) as Arc + } else { + let Some(environment) = resolved_environment.as_ref() else { + unreachable!( + "non-local stdio MCP servers resolve an environment before launch" + ); }; Arc::new(ExecutorStdioServerLauncher::new( environment.get_exec_backend(), - runtime_environment.fallback_cwd(), - )) - } else { - Arc::new(LocalStdioServerLauncher::new( - runtime_environment.fallback_cwd(), )) as Arc }; - // `RmcpClient` always sees a launched MCP stdio server. The - // launcher hides whether that means a local child process or an - // executor process whose stdin/stdout bytes cross the process API. RmcpClient::new_stdio_client(command_os, args_os, env_os, &env_vars, cwd, launcher) .await .map_err(|err| StartupOutcomeError::from(anyhow!(err))) @@ -630,16 +615,10 @@ async fn make_rmcp_client( env_http_headers, bearer_token_env_var, } => { - let http_client: Arc = if remote_environment { - let Some(environment) = runtime_environment.environment() else { - return Err(StartupOutcomeError::from(anyhow!( - "remote MCP server requires a runtime environment" - ))); - }; - environment.get_http_client() - } else { - Arc::new(ReqwestHttpClient) - }; + let http_client = resolved_environment.as_ref().map_or_else( + || Arc::new(ReqwestHttpClient) as Arc, + |environment| environment.get_http_client(), + ); let resolved_bearer_token = match resolve_bearer_token(server_name, bearer_token_env_var.as_deref()) { Ok(token) => token, diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index dc1fcec21..5404bf1eb 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -10,6 +10,7 @@ use std::sync::Arc; use std::time::Duration; use codex_exec_server::Environment; +use codex_exec_server::EnvironmentManager; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SandboxPolicy; @@ -28,83 +29,87 @@ pub struct SandboxState { pub use_legacy_landlock: bool, } -/// Runtime placement information used when starting MCP server transports. +/// Runtime context used when resolving per-server MCP environments. /// -/// `McpConfig` describes what servers exist. This value describes which -/// selected/default environment MCP servers should use for the current caller. -/// Keep it explicit at manager construction time so status/snapshot paths and -/// real sessions make the same placement decision. `fallback_cwd` is not a -/// per-server override; it is used when a stdio server omits `cwd` and the -/// launcher needs a concrete process working directory. `local_environment` -/// is separate because a remote selected/default environment can coexist with -/// an explicitly configured local environment that may launch local stdio MCPs. +/// `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. #[derive(Clone)] -pub struct McpRuntimeEnvironment { - environment: Option>, - local_environment: Option>, - fallback_cwd: PathBuf, +pub struct McpRuntimeContext { + environment_manager: Arc, + local_stdio_fallback_cwd: PathBuf, } -impl McpRuntimeEnvironment { +impl McpRuntimeContext { pub fn new( - environment: Option>, - local_environment: Option>, - fallback_cwd: PathBuf, + environment_manager: Arc, + local_stdio_fallback_cwd: PathBuf, ) -> Self { Self { - environment, - local_environment, - fallback_cwd, + environment_manager, + local_stdio_fallback_cwd, } } - pub(crate) fn environment(&self) -> Option> { - self.environment.as_ref().map(Arc::clone) + pub(crate) fn local_stdio_fallback_cwd(&self) -> PathBuf { + self.local_stdio_fallback_cwd.clone() } - pub(crate) fn fallback_cwd(&self) -> PathBuf { - self.fallback_cwd.clone() - } - - pub(crate) fn startup_unavailable_reason( + pub(crate) fn resolve_server_environment( &self, server_name: &str, config: &codex_config::McpServerConfig, - ) -> Option { - // This is intentionally narrower than "no env means no MCP": local - // stdio needs a local process launcher, while local HTTP can still use - // the ambient HTTP client with no local environment configured. - match config.experimental_environment.as_deref() { - None | Some("local") => { - // Local stdio only needs an explicitly configured local - // launcher. The selected/default MCP environment can be remote - // when both local and remote environments are configured. - if self.local_environment.is_none() - && matches!( - config.transport, - codex_config::McpServerTransportConfig::Stdio { .. } - ) - { - Some(format!( - "local stdio MCP server `{server_name}` requires a local environment" - )) - } else { - None - } + ) -> Result>, String> { + // Resolve `"local"` through the shared registry when available. Local + // HTTP is the one current exception: it can use the ambient HTTP client + // even when no local Environment is configured. + if let Some(environment) = self + .environment_manager + .get_environment(&config.environment_id) + { + if !config.is_local_environment() { + ensure_remote_stdio_cwd(server_name, config)?; } - Some("remote") => match self.environment.as_ref() { - Some(environment) if environment.is_remote() => None, - _ => Some(format!( - "remote MCP server `{server_name}` requires a remote environment" - )), - }, - Some(environment) => Some(format!( - "unsupported experimental_environment `{environment}` for MCP server `{server_name}`" - )), + return Ok(Some(environment)); } + + if config.is_local_environment() { + return match config.transport { + codex_config::McpServerTransportConfig::Stdio { .. } => Err(format!( + "local stdio MCP server `{server_name}` requires a local environment" + )), + codex_config::McpServerTransportConfig::StreamableHttp { .. } => Ok(None), + }; + } + + Err(format!( + "MCP server `{server_name}` references unknown environment id `{}`", + config.environment_id + )) } } +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); @@ -115,13 +120,15 @@ pub(crate) fn emit_duration(metric: &str, duration: Duration, tags: &[(&str, &st mod tests { use std::collections::HashMap; + use codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; use codex_config::McpServerConfig; use codex_config::McpServerTransportConfig; + use codex_exec_server::EnvironmentManager; use pretty_assertions::assert_eq; use super::*; - fn stdio_server(experimental_environment: Option<&str>) -> McpServerConfig { + fn stdio_server(environment_id: &str) -> McpServerConfig { McpServerConfig { transport: McpServerTransportConfig::Stdio { command: "echo".to_string(), @@ -130,7 +137,7 @@ mod tests { env_vars: Vec::new(), cwd: None, }, - experimental_environment: experimental_environment.map(str::to_string), + environment_id: environment_id.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -147,7 +154,7 @@ mod tests { } } - fn http_server(experimental_environment: Option<&str>) -> McpServerConfig { + fn http_server(environment_id: &str) -> McpServerConfig { McpServerConfig { transport: McpServerTransportConfig::StreamableHttp { url: "http://127.0.0.1:1".to_string(), @@ -155,111 +162,135 @@ mod tests { http_headers: None, env_http_headers: None, }, - experimental_environment: experimental_environment.map(str::to_string), - ..stdio_server(/*experimental_environment*/ None) + environment_id: environment_id.to_string(), + ..stdio_server(environment_id) } } #[test] fn local_stdio_requires_local_stdio_availability() { - let runtime_environment = McpRuntimeEnvironment::new( - /*environment*/ None, - /*local_environment*/ None, + let runtime_context = McpRuntimeContext::new( + Arc::new(EnvironmentManager::without_environments()), PathBuf::from("/tmp"), ); + let error = match runtime_context + .resolve_server_environment("stdio", &stdio_server(DEFAULT_MCP_SERVER_ENVIRONMENT_ID)) + { + Ok(_) => panic!("local stdio MCP should require a local environment"), + Err(error) => error, + }; assert_eq!( - runtime_environment.startup_unavailable_reason( - "stdio", - &stdio_server(/*experimental_environment*/ None) - ), - Some("local stdio MCP server `stdio` requires a local environment".to_string()) + error, + "local stdio MCP server `stdio` requires a local environment" ); } #[test] fn local_http_does_not_require_local_stdio_availability() { - let runtime_environment = McpRuntimeEnvironment::new( - /*environment*/ None, - /*local_environment*/ None, + let runtime_context = McpRuntimeContext::new( + Arc::new(EnvironmentManager::without_environments()), PathBuf::from("/tmp"), ); - assert_eq!( - runtime_environment.startup_unavailable_reason( - "http", - &http_server(/*experimental_environment*/ None) - ), - None - ); + let resolved_runtime = match runtime_context + .resolve_server_environment("http", &http_server(DEFAULT_MCP_SERVER_ENVIRONMENT_ID)) + { + Ok(resolved_runtime) => resolved_runtime, + Err(error) => panic!("local HTTP MCP should resolve: {error}"), + }; + assert!(resolved_runtime.is_none()); } #[test] - fn remote_stdio_requires_remote_environment() { - let runtime_environment = McpRuntimeEnvironment::new( - /*environment*/ None, - /*local_environment*/ None, + fn unknown_explicit_environment_is_rejected() { + let runtime_context = McpRuntimeContext::new( + Arc::new(EnvironmentManager::without_environments()), PathBuf::from("/tmp"), ); + let error = + match runtime_context.resolve_server_environment("stdio", &stdio_server("remote")) { + Ok(_) => panic!("unknown MCP environment should fail"), + Err(error) => error, + }; assert_eq!( - runtime_environment.startup_unavailable_reason( - "stdio", - &stdio_server(/*experimental_environment*/ Some("remote")), - ), - Some("remote MCP server `stdio` requires a remote environment".to_string()) - ); - } - - #[test] - fn remote_stdio_and_http_accept_remote_environment() { - let environment = Arc::new( - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - ); - let runtime_environment = McpRuntimeEnvironment::new( - Some(environment), - /*local_environment*/ None, - PathBuf::from("/tmp"), - ); - - assert_eq!( - runtime_environment.startup_unavailable_reason( - "stdio", - &stdio_server(/*experimental_environment*/ Some("remote")), - ), - None - ); - assert_eq!( - runtime_environment.startup_unavailable_reason( - "http", - &http_server(/*experimental_environment*/ Some("remote")), - ), - None + error, + "MCP server `stdio` references unknown environment id `remote`" ); } #[tokio::test] - async fn local_stdio_accepts_remote_runtime_when_local_environment_exists() { - let remote_environment = Arc::new( - Environment::create_for_tests(Some("ws://127.0.0.1:8765".to_string())) - .expect("remote environment"), - ); - let local_environment = Arc::new( - Environment::create_for_tests(/*exec_server_url*/ None).expect("local environment"), - ); - let runtime_environment = McpRuntimeEnvironment::new( - Some(remote_environment), - Some(local_environment), + async fn explicit_remote_stdio_and_http_accept_named_environment() { + 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"), ); - assert_eq!( - runtime_environment.startup_unavailable_reason( - "stdio", - &stdio_server(/*experimental_environment*/ None), + 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("http", &http_server("remote")), + ] { + let resolved_runtime = match resolved_runtime { + Ok(resolved_runtime) => resolved_runtime, + Err(error) => panic!("remote MCP should resolve: {error}"), + }; + assert!(resolved_runtime.is_some()); + } + } + + #[tokio::test] + async fn local_stdio_accepts_local_environment_when_available() { + let runtime_context = McpRuntimeContext::new( + Arc::new(EnvironmentManager::default_for_tests()), + PathBuf::from("/tmp"), + ); + + let resolved_runtime = match runtime_context + .resolve_server_environment("stdio", &stdio_server(DEFAULT_MCP_SERVER_ENVIRONMENT_ID)) + { + Ok(resolved_runtime) => resolved_runtime, + Err(error) => panic!("local stdio MCP should resolve: {error}"), + }; + 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, ), - None + 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/lib.rs b/codex-rs/config/src/lib.rs index 32065e368..5ae268803 100644 --- a/codex-rs/config/src/lib.rs +++ b/codex-rs/config/src/lib.rs @@ -94,6 +94,7 @@ pub use marketplace_edit::remove_user_marketplace_config; pub use mcp_edit::ConfigEditsBuilder; pub use mcp_edit::load_global_mcp_servers; pub use mcp_types::AppToolApproval; +pub use mcp_types::DEFAULT_MCP_SERVER_ENVIRONMENT_ID; pub use mcp_types::McpServerConfig; pub use mcp_types::McpServerDisabledReason; pub use mcp_types::McpServerEnvVar; diff --git a/codex-rs/config/src/mcp_edit.rs b/codex-rs/config/src/mcp_edit.rs index aaad3444f..7e062e299 100644 --- a/codex-rs/config/src/mcp_edit.rs +++ b/codex-rs/config/src/mcp_edit.rs @@ -175,8 +175,8 @@ fn serialize_mcp_server(config: &McpServerConfig) -> TomlItem { if !config.enabled { entry["enabled"] = value(false); } - if let Some(environment) = &config.experimental_environment { - entry["experimental_environment"] = value(environment.clone()); + if !config.is_local_environment() { + entry["environment_id"] = value(config.environment_id.clone()); } if config.required { entry["required"] = value(true); diff --git a/codex-rs/config/src/mcp_edit_tests.rs b/codex-rs/config/src/mcp_edit_tests.rs index 0215e7b80..030b0a023 100644 --- a/codex-rs/config/src/mcp_edit_tests.rs +++ b/codex-rs/config/src/mcp_edit_tests.rs @@ -23,7 +23,7 @@ async fn replace_mcp_servers_serializes_per_tool_approval_overrides() -> anyhow: env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: crate::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: true, @@ -101,7 +101,7 @@ async fn replace_mcp_servers_serializes_oauth_client_id() -> anyhow::Result<()> http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: crate::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index 8dfceec37..0d30ba8b2 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -13,6 +13,9 @@ use serde::de::Error as SerdeError; use crate::RequirementSource; +/// Effective MCP environment id when config omits `environment_id`. +pub const DEFAULT_MCP_SERVER_ENVIRONMENT_ID: &str = "local"; + #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, Default, JsonSchema)] #[serde(rename_all = "snake_case")] pub enum AppToolApproval { @@ -128,9 +131,8 @@ pub struct McpServerConfig { #[serde(flatten)] pub transport: McpServerTransportConfig, - /// Experimental environment selector for where Codex should start this MCP server. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub experimental_environment: Option, + /// Effective environment id for where Codex should start this MCP server. + pub environment_id: String, /// When `false`, Codex skips initializing this MCP server. #[serde(default = "default_enabled")] @@ -190,6 +192,10 @@ pub struct McpServerConfig { } impl McpServerConfig { + pub fn is_local_environment(&self) -> bool { + self.environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID + } + pub fn oauth_client_id(&self) -> Option<&str> { self.oauth .as_ref() @@ -231,7 +237,7 @@ pub struct RawMcpServerConfig { // shared #[serde(default)] - pub experimental_environment: Option, + pub environment_id: Option, #[serde(default)] pub startup_timeout_sec: Option, #[serde(default)] @@ -279,7 +285,7 @@ impl TryFrom for McpServerConfig { url, bearer_token, bearer_token_env_var, - experimental_environment, + environment_id, startup_timeout_sec, startup_timeout_ms, tool_timeout_sec, @@ -350,9 +356,13 @@ impl TryFrom for McpServerConfig { return Err("invalid transport".to_string()); }; + 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, - experimental_environment, + environment_id, startup_timeout_sec, tool_timeout_sec, enabled: enabled.unwrap_or_else(default_enabled), @@ -385,6 +395,30 @@ 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 f39711542..5f3933ce4 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -51,6 +51,62 @@ 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( @@ -402,7 +458,7 @@ fn deserialize_ignores_unknown_server_fields() { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: crate::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index fa0b485fa..71495d1e4 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -235,7 +235,7 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -738,7 +738,7 @@ async fn load_plugins_uses_manifest_configured_component_paths() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -850,7 +850,7 @@ async fn load_plugins_ignores_manifest_component_paths_without_dot_slash() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -1012,7 +1012,7 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index e810ce5f7..6f85785f2 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -2304,7 +2304,7 @@ }, "type": "array" }, - "experimental_environment": { + "environment_id": { "default": null, "type": "string" }, diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 46f29b32a..9f40aec67 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -132,7 +132,7 @@ fn stdio_mcp(command: &str) -> McpServerConfig { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -157,7 +157,7 @@ fn http_mcp(url: &str) -> McpServerConfig { http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5144,9 +5144,9 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { args: vec!["hello".to_string()], env: None, env_vars: Vec::new(), - cwd: None, + cwd: Some(codex_home.path().to_path_buf()), }, - experimental_environment: Some("remote".to_string()), + environment_id: "remote".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5184,13 +5184,13 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> { assert_eq!(args, &vec!["hello".to_string()]); assert!(env.is_none()); assert!(env_vars.is_empty()); - assert!(cwd.is_none()); + assert_eq!(cwd, &Some(codex_home.path().to_path_buf())); } other => panic!("unexpected transport {other:?}"), } assert_eq!(docs.startup_timeout_sec, Some(Duration::from_secs(3))); assert_eq!(docs.tool_timeout_sec, Some(Duration::from_secs(5))); - assert_eq!(docs.experimental_environment.as_deref(), Some("remote")); + assert_eq!(docs.environment_id, "remote"); assert!(docs.enabled); let empty = BTreeMap::new(); @@ -5493,7 +5493,7 @@ async fn replace_mcp_servers_serializes_env_sorted() -> anyhow::Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5570,7 +5570,7 @@ async fn replace_mcp_servers_serializes_env_vars() -> anyhow::Result<()> { env_vars: vec!["ALPHA".into(), "BETA".into()], cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5632,7 +5632,7 @@ async fn replace_mcp_servers_serializes_sourced_env_vars() -> anyhow::Result<()> ], cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5684,7 +5684,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> { env_vars: Vec::new(), cwd: Some(cwd_path.clone()), }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5739,7 +5739,7 @@ async fn replace_mcp_servers_streamable_http_serializes_bearer_token() -> anyhow http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5810,7 +5810,7 @@ async fn replace_mcp_servers_streamable_http_serializes_custom_headers() -> anyh "DOCS_AUTH".to_string(), )])), }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5893,7 +5893,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh "DOCS_AUTH".to_string(), )])), }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -5929,7 +5929,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -6000,7 +6000,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers() "DOCS_AUTH".to_string(), )])), }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -6026,7 +6026,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers() env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -6115,7 +6115,7 @@ async fn replace_mcp_servers_serializes_disabled_flag() -> anyhow::Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: false, required: false, supports_parallel_tool_calls: false, @@ -6166,7 +6166,7 @@ async fn replace_mcp_servers_serializes_required_flag() -> anyhow::Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: true, supports_parallel_tool_calls: false, @@ -6217,7 +6217,7 @@ async fn replace_mcp_servers_serializes_tool_filters() -> anyhow::Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -6272,7 +6272,7 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index f467c76cf..1b5cd879c 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -300,8 +300,8 @@ mod document_helpers { if !config.enabled { entry["enabled"] = value(false); } - if let Some(environment) = &config.experimental_environment { - entry["experimental_environment"] = value(environment.clone()); + if !config.is_local_environment() { + entry["environment_id"] = value(config.environment_id.clone()); } if config.required { entry["required"] = value(true); diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index 30f82db29..740d819ae 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -945,7 +945,7 @@ fn blocking_replace_mcp_servers_round_trips() { env_vars: vec!["FOO".into()], cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: true, @@ -975,7 +975,7 @@ fn blocking_replace_mcp_servers_round_trips() { ), env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: false, required: false, supports_parallel_tool_calls: false, @@ -1047,7 +1047,7 @@ fn blocking_replace_mcp_servers_serializes_tool_approval_overrides() { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -1112,7 +1112,7 @@ foo = { command = "cmd" } env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -1167,7 +1167,7 @@ foo = { command = "cmd" } # keep me env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: false, required: false, supports_parallel_tool_calls: false, @@ -1221,7 +1221,7 @@ foo = { command = "cmd", args = ["--flag"] } # keep me env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -1276,7 +1276,7 @@ foo = { command = "cmd" } env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: false, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index de41919a8..8593686c7 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -35,7 +35,7 @@ use codex_login::CodexAuth; use codex_login::default_client::originator; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::McpConnectionManager; -use codex_mcp::McpRuntimeEnvironment; +use codex_mcp::McpRuntimeContext; use codex_mcp::ToolInfo; use codex_mcp::ToolPluginProvenance; use codex_mcp::codex_apps_tools_cache_key; @@ -203,7 +203,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status( list_accessible_connectors_from_mcp_tools_with_environment_manager( config, force_refetch, - &environment_manager, + Arc::new(environment_manager), ) .await } @@ -211,7 +211,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status( pub async fn list_accessible_connectors_from_mcp_tools_with_environment_manager( config: &Config, force_refetch: bool, - environment_manager: &EnvironmentManager, + environment_manager: Arc, ) -> anyhow::Result { let auth_manager = AuthManager::shared_from_config(config, /*enable_codex_api_key_env*/ false).await; @@ -272,11 +272,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_environment_manager( PermissionProfile::default(), // Connector discovery is threadless. Use an actually configured env if // one exists, but do not reintroduce the old hidden-local fallback. - McpRuntimeEnvironment::new( - environment_manager.default_or_local_environment(), - environment_manager.try_local_environment(), - config.cwd.to_path_buf(), - ), + McpRuntimeContext::new(environment_manager, config.cwd.to_path_buf()), config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth.as_ref()), host_owned_codex_apps_enabled, diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index b8552596b..936f1b5fb 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -361,7 +361,7 @@ fn mcp_dependency_to_server_config( http_headers: None, env_http_headers: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -391,7 +391,7 @@ fn mcp_dependency_to_server_config( env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index f9984c7f2..0d6b8ff1a 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1255,11 +1255,6 @@ fn codex_apps_auth_failure_metadata() -> McpToolApprovalMetadata { async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: &TurnContext) { let auth = session.services.auth_manager.auth().await; - let environment = session - .services - .environment_manager - .default_or_local_environment() - .expect("test session should have an MCP runtime environment"); let (manager, _cancel_token) = codex_mcp::McpConnectionManager::new( &HashMap::new(), turn_context.config.mcp_oauth_credentials_store_mode, @@ -1268,14 +1263,10 @@ async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: turn_context.sub_id.clone(), session.get_tx_event(), turn_context.permission_profile(), - codex_mcp::McpRuntimeEnvironment::new( - Some(environment), - session.services.environment_manager.try_local_environment(), - { - #[allow(deprecated)] - turn_context.cwd.to_path_buf() - }, - ), + codex_mcp::McpRuntimeContext::new(Arc::clone(&session.services.environment_manager), { + #[allow(deprecated)] + turn_context.cwd.to_path_buf() + }), turn_context.config.codex_home.to_path_buf(), codex_mcp::codex_apps_tools_cache_key(auth.as_ref()), /*host_owned_codex_apps_enabled*/ true, diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 738a32867..40cb45d2a 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -325,17 +325,13 @@ impl Session { host_owned_codex_apps_enabled(&mcp_config, auth.as_ref()); let auth_statuses = compute_auth_statuses(mcp_servers.iter(), store_mode, auth.as_ref()).await; - let mcp_runtime_environment = match turn_context.environments.primary() { - Some(turn_environment) => McpRuntimeEnvironment::new( - Some(Arc::clone(&turn_environment.environment)), - self.services.environment_manager.try_local_environment(), + let mcp_runtime_context = match turn_context.environments.primary() { + Some(turn_environment) => McpRuntimeContext::new( + Arc::clone(&self.services.environment_manager), turn_environment.cwd.to_path_buf(), ), - None => McpRuntimeEnvironment::new( - self.services - .environment_manager - .default_or_local_environment(), - self.services.environment_manager.try_local_environment(), + None => McpRuntimeContext::new( + Arc::clone(&self.services.environment_manager), #[allow(deprecated)] turn_context.cwd.to_path_buf(), ), @@ -353,7 +349,7 @@ impl Session { turn_context.sub_id.clone(), self.get_tx_event(), turn_context.permission_profile(), - mcp_runtime_environment, + mcp_runtime_context, config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth.as_ref()), host_owned_codex_apps_enabled, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index c12c29e7e..c73b43a83 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -61,7 +61,7 @@ use codex_login::CodexAuth; use codex_login::auth_env_telemetry::collect_auth_env_telemetry; use codex_login::default_client::originator; use codex_mcp::McpConnectionManager; -use codex_mcp::McpRuntimeEnvironment; +use codex_mcp::McpRuntimeContext; use codex_mcp::codex_apps_tools_cache_key; use codex_models_manager::manager::RefreshStrategy; use codex_models_manager::manager::SharedModelsManager; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index cac4806be..8ed010172 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1135,15 +1135,13 @@ impl Session { })? .primary() .cloned(); - let mcp_runtime_environment = match turn_environment { - Some(turn_environment) => McpRuntimeEnvironment::new( - Some(Arc::clone(&turn_environment.environment)), - sess.services.environment_manager.try_local_environment(), + let mcp_runtime_context = match turn_environment { + Some(turn_environment) => McpRuntimeContext::new( + Arc::clone(&sess.services.environment_manager), turn_environment.cwd.to_path_buf(), ), - None => McpRuntimeEnvironment::new( - sess.services.environment_manager.default_or_local_environment(), - sess.services.environment_manager.try_local_environment(), + None => McpRuntimeContext::new( + Arc::clone(&sess.services.environment_manager), session_configuration.cwd.to_path_buf(), ), }; @@ -1155,7 +1153,7 @@ impl Session { INITIAL_SUBMIT_ID.to_owned(), tx_event.clone(), session_configuration.permission_profile(), - mcp_runtime_environment, + mcp_runtime_context, config.codex_home.to_path_buf(), codex_apps_tools_cache_key(auth), host_owned_codex_apps_enabled, diff --git a/codex-rs/core/tests/suite/code_mode.rs b/codex-rs/core/tests/suite/code_mode.rs index b733d7bde..406981fad 100644 --- a/codex-rs/core/tests/suite/code_mode.rs +++ b/codex-rs/core/tests/suite/code_mode.rs @@ -242,7 +242,7 @@ async fn run_code_mode_turn_with_rmcp_config( env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index 80290e31b..5ee2cb05d 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -182,7 +182,7 @@ fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 8e85c5d5e..94222deab 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -139,11 +139,13 @@ enum McpCallEvent { const REMOTE_MCP_ENVIRONMENT: &str = "remote"; -fn remote_aware_experimental_environment() -> Option { +fn remote_aware_environment_id() -> String { // These tests run locally in normal CI and against the Docker-backed // executor in full-ci. Match that shared test environment instead of // parameterizing each stdio MCP test with its own local/remote cases. - std::env::var_os(remote_env_env_var()).map(|_| REMOTE_MCP_ENVIRONMENT.to_string()) + std::env::var_os(remote_env_env_var()) + .map(|_| REMOTE_MCP_ENVIRONMENT.to_string()) + .unwrap_or_else(|| codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string()) } /// Returns the stdio MCP test server command path for the active test placement. @@ -285,13 +287,22 @@ async fn wait_for_mcp_server(fixture: &TestCodex, server_name: &str) -> anyhow:: Ok(()) } -#[derive(Default)] struct TestMcpServerOptions { - experimental_environment: Option, + environment_id: String, supports_parallel_tool_calls: bool, tool_timeout_sec: Option, } +impl Default for TestMcpServerOptions { + fn default() -> Self { + Self { + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + supports_parallel_tool_calls: false, + tool_timeout_sec: None, + } + } +} + fn stdio_transport( command: String, env: Option>, @@ -326,7 +337,7 @@ fn insert_mcp_server( server_name.to_string(), McpServerConfig { transport, - experimental_environment: options.experimental_environment, + environment_id: options.environment_id, enabled: true, required: false, supports_parallel_tool_calls: options.supports_parallel_tool_calls, @@ -480,7 +491,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> { Vec::new(), ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -603,7 +614,7 @@ async fn stdio_server_uses_configured_cwd_before_runtime_fallback() -> anyhow::R Some(configured_cwd), ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -623,59 +634,6 @@ async fn stdio_server_uses_configured_cwd_before_runtime_fallback() -> anyhow::R Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -#[serial(mcp_cwd)] -async fn remote_stdio_server_uses_runtime_fallback_cwd_when_config_omits_cwd() -> anyhow::Result<()> -{ - skip_if_no_network!(Ok(())); - if std::env::var_os(remote_env_env_var()).is_none() { - return Ok(()); - } - - let server = responses::start_mock_server().await; - let server_name = "rmcp_fallback_cwd"; - let expected_cwd = Arc::new(Mutex::new(None::)); - let expected_cwd_for_config = Arc::clone(&expected_cwd); - let rmcp_test_server_bin = remote_aware_stdio_server_bin()?; - - 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()); - insert_mcp_server( - config, - server_name, - stdio_transport( - rmcp_test_server_bin, - Some(HashMap::from([( - "MCP_TEST_VALUE".to_string(), - "fallback-cwd".to_string(), - )])), - Vec::new(), - ), - TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), - ..Default::default() - }, - ); - }) - .build_with_remote_env(&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-fallback-cwd").await?; - - assert_cwd_tool_output(&structured, &expected_cwd); - server.verify().await; - Ok(()) -} - #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 1)] #[serial(mcp_cwd)] @@ -775,7 +733,7 @@ async fn stdio_mcp_tool_call_includes_sandbox_state_meta() -> anyhow::Result<()> server_name, stdio_transport(rmcp_test_server_bin, /*env*/ None, Vec::new()), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -872,7 +830,7 @@ async fn stdio_mcp_parallel_tool_calls_default_false_runs_serially() -> anyhow:: server_name, stdio_transport(rmcp_test_server_bin, /*env*/ None, Vec::new()), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), tool_timeout_sec: Some(Duration::from_secs(2)), ..Default::default() }, @@ -989,7 +947,7 @@ async fn stdio_mcp_parallel_tool_calls_opt_in_runs_concurrently() -> anyhow::Res server_name, stdio_transport(rmcp_test_server_bin, /*env*/ None, Vec::new()), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), supports_parallel_tool_calls: true, tool_timeout_sec: Some(Duration::from_secs(2)), }, @@ -1071,7 +1029,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> { Vec::new(), ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -1204,7 +1162,7 @@ async fn stdio_image_responses_preserve_original_detail_metadata() -> anyhow::Re server_name, stdio_transport(rmcp_test_server_bin, /*env*/ None, Vec::new()), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -1340,7 +1298,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re Vec::new(), ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -1442,7 +1400,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> { vec!["MCP_TEST_VALUE".into()], ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -1560,7 +1518,7 @@ async fn stdio_server_propagates_explicit_local_env_var_source() -> anyhow::Resu }], ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -1652,7 +1610,7 @@ async fn remote_stdio_env_var_source_does_not_copy_local_env() -> anyhow::Result }], ), TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -1835,7 +1793,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> { env_http_headers: None, }, TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); @@ -2021,7 +1979,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { env_http_headers: None, }, TestMcpServerOptions { - experimental_environment: remote_aware_experimental_environment(), + environment_id: remote_aware_environment_id(), ..Default::default() }, ); diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index 706ff4871..26531e9d2 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -1012,7 +1012,7 @@ async fn tool_search_indexes_only_enabled_non_app_mcp_tools() -> Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, disabled_reason: None, @@ -1141,7 +1141,7 @@ async fn tool_search_surfaced_mcp_tool_errors_are_returned_to_model() -> Result< env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, disabled_reason: None, @@ -1288,7 +1288,7 @@ async fn tool_search_uses_non_app_mcp_server_instructions_as_namespace_descripti env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, disabled_reason: None, diff --git a/codex-rs/core/tests/suite/sqlite_state.rs b/codex-rs/core/tests/suite/sqlite_state.rs index f576e55a3..f5dd75670 100644 --- a/codex-rs/core/tests/suite/sqlite_state.rs +++ b/codex-rs/core/tests/suite/sqlite_state.rs @@ -376,7 +376,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result< env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/core/tests/suite/truncation.rs b/codex-rs/core/tests/suite/truncation.rs index be9267934..b98c438f4 100644 --- a/codex-rs/core/tests/suite/truncation.rs +++ b/codex-rs/core/tests/suite/truncation.rs @@ -387,7 +387,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()> env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -487,7 +487,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, @@ -775,7 +775,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> { env_vars: Vec::new(), cwd: None, }, - experimental_environment: None, + environment_id: "local".to_string(), enabled: true, required: false, supports_parallel_tool_calls: false, diff --git a/codex-rs/rmcp-client/src/stdio_server_launcher.rs b/codex-rs/rmcp-client/src/stdio_server_launcher.rs index 9928511bb..8b3250669 100644 --- a/codex-rs/rmcp-client/src/stdio_server_launcher.rs +++ b/codex-rs/rmcp-client/src/stdio_server_launcher.rs @@ -438,20 +438,12 @@ impl Drop for StdioServerProcessHandleInner { #[derive(Clone)] pub struct ExecutorStdioServerLauncher { exec_backend: Arc, - fallback_cwd: PathBuf, } impl ExecutorStdioServerLauncher { /// Creates a stdio server launcher backed by the executor process API. - /// - /// `fallback_cwd` is used only when the MCP server config omits `cwd`. - /// Executor `process/start` requires an explicit working directory, unlike - /// local `tokio::process::Command`, which can inherit the orchestrator cwd. - pub fn new(exec_backend: Arc, fallback_cwd: PathBuf) -> Self { - Self { - exec_backend, - fallback_cwd, - } + pub fn new(exec_backend: Arc) -> Self { + Self { exec_backend } } } @@ -461,8 +453,7 @@ impl StdioServerLauncher for ExecutorStdioServerLauncher { command: StdioServerCommand, ) -> BoxFuture<'static, io::Result> { let exec_backend = Arc::clone(&self.exec_backend); - let fallback_cwd = self.fallback_cwd.clone(); - async move { Self::launch_server(command, exec_backend, fallback_cwd).await }.boxed() + async move { Self::launch_server(command, exec_backend).await }.boxed() } } @@ -474,7 +465,6 @@ impl ExecutorStdioServerLauncher { async fn launch_server( command: StdioServerCommand, exec_backend: Arc, - fallback_cwd: PathBuf, ) -> io::Result { let StdioServerCommand { program, @@ -483,6 +473,11 @@ impl ExecutorStdioServerLauncher { env_vars, cwd, } = command; + let Some(cwd) = cwd else { + return Err(io::Error::other( + "executor stdio server requires an explicit cwd", + )); + }; let program_name = program.to_string_lossy().into_owned(); let envs = create_env_overlay_for_remote_mcp_server(env, &env_vars); let remote_env_vars = remote_mcp_env_var_names(&env_vars); @@ -500,7 +495,7 @@ impl ExecutorStdioServerLauncher { .start(ExecParams { process_id, argv, - cwd: cwd.unwrap_or(fallback_cwd), + cwd, env_policy: Some(Self::remote_env_policy(&remote_env_vars)), env, tty: false,