diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 87c6d4c931..4e47313497 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -233,6 +233,13 @@ impl McpServerConnection { .as_ref() .is_some_and(|startup_trigger| !*startup_trigger.borrow()) } + + fn is_dormant_and_private(&self) -> bool { + self.startup_is_dormant() + && !self.client.cancel_token.is_cancelled() + && self.client.connection_slot.is_none() + && self.legacy_fallback.is_none() + } } impl Drop for McpServerConnection { @@ -1463,6 +1470,27 @@ impl McpConnectionSet { Ok(server_key) } + /// A private, configured local helper whose startup gate is still closed has no process. + pub(crate) fn is_dormant_private_stdio(&self, key: &McpServerKey) -> bool { + let view = &self.servers[key]; + if !view.connection.is_dormant_and_private() + || view.protocol_mode != McpProtocolMode::Legacy + { + return false; + } + self.config + .as_ref() + .and_then(|config| config.mcp_server_catalog.server(key)) + .is_some_and(|server| { + matches!(server.source(), McpServerSource::Config) + && server.config().is_local_environment() + && matches!( + server.config().transport, + McpServerTransportConfig::Stdio { .. } + ) + }) + } + /// Validates an exact client target before startup or execution. pub(crate) fn validate_tool_call(&self, server: &McpServerKey, tool: &str) -> Result<()> { let view = self diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 803419255f..c67609000a 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -683,6 +683,71 @@ async fn stalled_apps_client_publication_expires_at_the_configured_startup_timeo ); } +#[tokio::test] +async fn cleanup_skips_only_an_unrequested_private_local_stdio_connection() { + let name = "node_repl"; + let key = McpServerKey::unscoped(name); + let mut manager = McpConnectionSet::empty(/*prefix_mcp_tool_names*/ true); + manager.insert_test_client(name, create_ready_async_managed_client(Vec::new()).await); + let (trigger, _receiver) = watch::channel(false); + Arc::get_mut( + &mut manager + .servers + .get_mut(&key) + .expect("registered REPL view") + .connection, + ) + .expect("unique test connection") + .startup_trigger = Some(trigger.clone()); + let stdio: McpServerConfig = + serde_json::from_value(serde_json::json!({"command":"test-helper"})) + .expect("valid test stdio config"); + let http = reusable_server_config("https://example.com/mcp"); + let mut remote = stdio.clone(); + remote.environment_id = "remote".to_string(); + let configured = |config| crate::McpServerRegistration::from_config(name.to_string(), config); + let plugin = crate::McpServerRegistration::from_plugin( + name.to_string(), + crate::McpPluginAttribution::new("other@example".into(), "Other".into()), + /*plugin_order*/ 0, + stdio.clone(), + ); + for (registration, expected) in [ + (configured(http), false), + (configured(remote), false), + (plugin, false), + (configured(stdio), true), + ] { + let mut runtime_config = crate::mcp::tests::test_mcp_config(std::env::temp_dir()); + let mut catalog = crate::ResolvedMcpCatalog::builder(); + catalog.register(registration); + runtime_config.mcp_server_catalog = catalog.build(); + manager.config = Some(Arc::new(runtime_config)); + assert_eq!(manager.is_dormant_private_stdio(&key), expected); + } + let view = manager.servers.get_mut(&key).expect("registered REPL view"); + view.protocol_mode = crate::McpProtocolMode::V20260728; + assert!(!manager.is_dormant_private_stdio(&key)); + let view = manager.servers.get_mut(&key).expect("registered REPL view"); + view.protocol_mode = crate::McpProtocolMode::Legacy; + Arc::get_mut(&mut view.connection) + .expect("unique test connection") + .client + .connection_slot = Some(Arc::default()); + assert!(!manager.is_dormant_private_stdio(&key)); + let connection = Arc::get_mut( + &mut manager + .servers + .get_mut(&key) + .expect("registered REPL view") + .connection, + ) + .expect("unique test connection"); + connection.client.connection_slot = None; + trigger.send_replace(true); + assert!(!manager.is_dormant_private_stdio(&key)); +} + #[tokio::test] async fn connection_statuses_observe_clients_without_starting_them() { use codex_protocol::mcp::McpServerConnectionStatus as Status; diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 3d5944841b..667ad78491 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -37,6 +37,7 @@ pub use runtime::McpRuntimeContext; pub use runtime::McpRuntimeInput; pub use runtime::McpStartupPolicy; pub use runtime::McpToolCallResult; +pub use runtime::McpToolStartup; pub use runtime::apply_http_headers_helper; pub use sandbox_metadata::MCP_SANDBOX_LAUNCHER_META_KEY; pub use sandbox_metadata::MCP_SANDBOX_STATE_CAPABILITY; diff --git a/codex-rs/codex-mcp/src/runtime.rs b/codex-rs/codex-mcp/src/runtime.rs index 55b3b50551..e74311509a 100644 --- a/codex-rs/codex-mcp/src/runtime.rs +++ b/codex-rs/codex-mcp/src/runtime.rs @@ -87,6 +87,13 @@ pub enum McpStartupPolicy { DeferCachedServers, } +/// Startup behavior for a host-requested MCP tool call. +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum McpToolStartup { + Wait, + SkipDormantReplCleanup, +} + /// A host-requested tool result and its identity in the published MCP catalog. pub struct McpToolCallResult { pub result: CallToolResult, @@ -883,8 +890,8 @@ impl McpRuntime { } /// Calls a tool for the host, retaining attribution while its catalogs stay unchanged. - /// Waiting calls capture a binding after the selected server starts; non-waiting calls - /// use the cached binding. Missing or changed catalogs leave the namespace unknown. + /// Capture a binding after the selected server starts. Missing or changed catalogs leave + /// the namespace unknown. #[allow(clippy::too_many_arguments)] pub async fn latest_call_tool( &self, @@ -894,18 +901,30 @@ impl McpRuntime { arguments: Option, meta: Option, requested_timeout: Option, - wait_for_server: bool, + startup: McpToolStartup, ) -> anyhow::Result { let current = self.current.load_full(); let server_key = current .connections .resolve_tool_call_server(server, tool, environment_id)?; - let attribution = if wait_for_server - && current - .connections - .wait_for_server_startup(server_key) - .await + if startup == McpToolStartup::SkipDormantReplCleanup + && current.connections.is_dormant_private_stdio(server_key) + { + return Ok(McpToolCallResult { + result: CallToolResult { + content: Vec::new(), + structured_content: None, + is_error: None, + meta: None, + }, + source_tool_namespace: None, + }); + } + let attribution = if current + .connections + .wait_for_server_startup(server_key) + .await && let Some(revisions) = current .connections .stable_catalog_revisions( @@ -939,7 +958,7 @@ impl McpRuntime { arguments, meta, requested_timeout, - wait_for_server, + /*wait_for_server*/ true, ) .await?; // Host calls use live connections, unlike model calls pinned to a binding. diff --git a/codex-rs/core/src/hook_mcp_executor.rs b/codex-rs/core/src/hook_mcp_executor.rs index c65df603e5..3cc721803d 100644 --- a/codex-rs/core/src/hook_mcp_executor.rs +++ b/codex-rs/core/src/hook_mcp_executor.rs @@ -6,6 +6,7 @@ use codex_hooks::HookMcpCall; use codex_hooks::HookMcpExecutor; use codex_hooks::HookMcpOutput; use codex_mcp::McpRuntime; +use codex_mcp::McpToolStartup; use codex_protocol::ThreadId; use futures::FutureExt; use futures::future::BoxFuture; @@ -35,7 +36,11 @@ impl HookMcpExecutor for CoreHookMcpExecutor { Some(Value::Object(call.input)), Some(Value::Object(metadata)), Some(call.timeout), - /*wait_for_server*/ true, + if call.skip_dormant_repl_cleanup { + McpToolStartup::SkipDormantReplCleanup + } else { + McpToolStartup::Wait + }, ), ) .await diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index a82aac213c..7c11e6d4ee 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -98,6 +98,7 @@ use codex_mcp::McpResourceClient; use codex_mcp::McpRuntime; use codex_mcp::McpRuntimeContext; use codex_mcp::McpRuntimeInput; +use codex_mcp::McpToolStartup; use codex_models_manager::manager::ModelsManager; use codex_models_manager::manager::RefreshStrategy; use codex_models_manager::manager::SharedModelsManager; @@ -5205,7 +5206,7 @@ impl Session { "threadId": self.thread_id().to_string(), })), /*requested_timeout*/ None, - /*wait_for_server*/ true, + McpToolStartup::Wait, ) .await .ok() diff --git a/codex-rs/core/tests/suite/hooks_mcp.rs b/codex-rs/core/tests/suite/hooks_mcp.rs index b14bfd4521..873b3305a6 100644 --- a/codex-rs/core/tests/suite/hooks_mcp.rs +++ b/codex-rs/core/tests/suite/hooks_mcp.rs @@ -20,6 +20,7 @@ use codex_protocol::models::ContentItemMetadata; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; @@ -40,6 +41,7 @@ use core_test_support::skip_if_wine_exec; use core_test_support::stdio_server_bin; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; use serde_json::Value; @@ -520,11 +522,13 @@ async fn mcp_tool_hook_interpolates_prompt_and_runs_without_tool_approval() -> R Ok(()) } -#[test_case::test_case(McpStartupReadiness::Connection; "connection")] -#[test_case::test_case(McpStartupReadiness::Catalog; "catalog")] +#[test_case::test_case(McpStartupReadiness::Connection, "browser", "node_repl"; "connection")] +#[test_case::test_case(McpStartupReadiness::Catalog, "unified-computer-use", "cua_repl"; "catalog")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn mcp_tool_hook_starts_cached_model_hidden_tools( startup_readiness: McpStartupReadiness, + plugin: &'static str, + repl: &'static str, ) -> Result<()> { skip_if_no_network!(Ok(())); @@ -541,7 +545,7 @@ async fn mcp_tool_hook_starts_cached_model_hidden_tools( let rmcp_test_server_bin = stdio_server_bin()?; let test = test_codex() .with_model_info_override("gpt-5.5", |model| model.use_responses_lite = true) - .with_pre_build_hook(|home| { + .with_pre_build_hook(move |home| { let hooks = json!({ "hooks": { "UserPromptSubmit": [{ @@ -557,8 +561,35 @@ async fn mcp_tool_hook_starts_cached_model_hidden_tools( }); fs::write(home.join("hooks.json"), hooks.to_string()) .expect("write model-hidden MCP tool hook config"); + let manifest_dir = home.join(format!( + "plugins/cache/openai-bundled/{plugin}/local/.codex-plugin" + )); + fs::create_dir_all(&manifest_dir).expect("create bundled plugin fixture"); + let manifest = json!({ + "name": plugin, + "hooks": {"hooks": {"SubagentStop": [{"hooks": [{ + "type": "mcp_tool", + "server": repl, + "tool": "turn_ended", + }]}]}}, + }); + fs::write(manifest_dir.join("plugin.json"), manifest.to_string()) + .expect("write bundled cleanup hook"); + fs::write( + home.join("config.toml"), + format!("[plugins.\"{plugin}@openai-bundled\"]\nenabled = true\n"), + ) + .expect("enable bundled plugin fixture"); }) .with_config(move |config| { + config + .features + .enable(Feature::Plugins) + .expect("enable plugins"); + config + .features + .disable(Feature::ExecutorCapabilityDiscovery) + .expect("use legacy MCP for the local fixtures"); enable_hooks_and_rmcp_server( config, rmcp_test_server_bin, @@ -569,13 +600,24 @@ async fn mcp_tool_hook_starts_cached_model_hidden_tools( let mcp_server = servers.get_mut(RMCP_SERVER).expect("test MCP server"); mcp_server.enabled_tools = Some(vec!["thread_hint".to_string()]); mcp_server.startup_readiness = startup_readiness; - let unused_server = mcp_server.clone(); - servers.insert("unused".to_string(), unused_server); + let mut cleanup_server = mcp_server.clone(); + cleanup_server.enabled_tools = None; + servers.insert(repl.to_string(), cleanup_server); config.mcp_servers.set(servers).expect("test MCP servers"); }) .build(&server) .await?; - wait_for_mcp_server(&test.codex, RMCP_SERVER).await?; + let startup = wait_for_event_match(&test.codex, |event| match event { + EventMsg::McpStartupComplete(summary) => Some(summary.clone()), + _ => None, + }) + .await; + for server in [RMCP_SERVER, repl] { + assert!( + startup.ready.iter().any(|ready| ready == server), + "expected {server} to be ready: {startup:?}", + ); + } let subagent = test .thread_manager @@ -598,14 +640,24 @@ async fn mcp_tool_hook_starts_cached_model_hidden_tools( .thread .current_mcp_config_and_runtime_context() .await; - assert_eq!( - subagent - .thread - .mcp_connection_statuses(&mcp_config) - .await - .get(&McpServerKey::unscoped(RMCP_SERVER)), - Some(&McpServerConnectionStatus::NotStarted), + let plugin_id = format!("{plugin}@openai-bundled"); + assert!( + subagent.thread.list_hooks().hooks.iter().any(|hook| { + hook.builtin + && hook.enabled + && hook.event_name == HookEventName::SubagentStop + && hook.plugin_id.as_deref() == Some(plugin_id.as_str()) + }), + "the child must load the bundled cleanup hook", ); + let repl_key = McpServerKey::unscoped(repl); + let statuses = subagent.thread.mcp_connection_statuses(&mcp_config).await; + for key in [McpServerKey::unscoped(RMCP_SERVER), repl_key.clone()] { + assert_eq!( + statuses.get(&key), + Some(&McpServerConnectionStatus::NotStarted), + ); + } subagent .thread .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { @@ -631,9 +683,9 @@ async fn mcp_tool_hook_starts_cached_model_hidden_tools( .thread .mcp_connection_statuses(&mcp_config) .await - .get(&McpServerKey::unscoped("unused")), + .get(&repl_key), Some(&McpServerConnectionStatus::NotStarted), - "a hook must leave unrelated cached servers dormant", + "bundled cleanup must leave an unused cached REPL dormant", ); let request = response.single_request(); let expected_context = format!( diff --git a/codex-rs/core/tests/suite/mcp_auth_refresh.rs b/codex-rs/core/tests/suite/mcp_auth_refresh.rs index 9f8f3690d1..afc8693937 100644 --- a/codex-rs/core/tests/suite/mcp_auth_refresh.rs +++ b/codex-rs/core/tests/suite/mcp_auth_refresh.rs @@ -19,6 +19,7 @@ use codex_mcp::McpRuntimeContext; use codex_mcp::McpRuntimeInput; use codex_mcp::McpStartupPolicy; use codex_mcp::McpToolCatalogCache; +use codex_mcp::McpToolStartup; use codex_protocol::mcp::ClientMcpExtensions; use codex_protocol::protocol::AskForApproval; use core_test_support::apps_test_server::AppsTestServer; @@ -142,7 +143,7 @@ async fn hosted_plugin_runtime_ps_mcp_tool_calls_use_current_auth_manager_token( })), /*meta*/ None, /*requested_timeout*/ None, - /*wait_for_server*/ true, + McpToolStartup::Wait, ) .await?; assert_eq!(tool_result.result.is_error, Some(false)); diff --git a/codex-rs/hooks/src/engine/mcp_runner.rs b/codex-rs/hooks/src/engine/mcp_runner.rs index 2d3cdc6d92..9d1285eb0c 100644 --- a/codex-rs/hooks/src/engine/mcp_runner.rs +++ b/codex-rs/hooks/src/engine/mcp_runner.rs @@ -4,6 +4,7 @@ use std::time::Instant; use anyhow::Context; use anyhow::Result; +use codex_protocol::mcp::is_node_repl_backed_server; use codex_protocol::models::ContentItemMetadata; use regex::Regex; use serde_json::Map; @@ -73,6 +74,9 @@ pub(crate) async fn run_mcp_tool( } executor .execute(HookMcpCall { + skip_dormant_repl_cleanup: handler.builtin + && is_node_repl_backed_server(server) + && tool == "turn_ended", server: server.to_string(), tool: tool.to_string(), environment_id, diff --git a/codex-rs/hooks/src/engine/mcp_runner_tests.rs b/codex-rs/hooks/src/engine/mcp_runner_tests.rs index ea3fdb7359..3408b751b1 100644 --- a/codex-rs/hooks/src/engine/mcp_runner_tests.rs +++ b/codex-rs/hooks/src/engine/mcp_runner_tests.rs @@ -134,6 +134,7 @@ async fn mcp_tool_results_use_command_hook_output_contract() { assert_eq!( *calls.lock().expect("lock calls"), vec![HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "scan".to_string(), environment_id: None, diff --git a/codex-rs/hooks/src/engine/mod_tests.rs b/codex-rs/hooks/src/engine/mod_tests.rs index 4e93a0075d..93b065a169 100644 --- a/codex-rs/hooks/src/engine/mod_tests.rs +++ b/codex-rs/hooks/src/engine/mod_tests.rs @@ -2425,6 +2425,7 @@ fn executor_stop_hook_fixture() -> ( }; let expected_executor_call = HookMcpCall { + skip_dormant_repl_cleanup: true, server: "node_repl".to_string(), tool: "turn_ended".to_string(), environment_id: Some("executor-a".to_string()), @@ -2560,6 +2561,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { vec![ expected_executor_call.clone(), HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "check".to_string(), environment_id: None, @@ -2598,6 +2600,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { vec![ expected_executor_call.clone(), HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "check".to_string(), environment_id: None, @@ -2606,6 +2609,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { timeout: Duration::from_secs(30), }, HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "check".to_string(), environment_id: None, @@ -2614,6 +2618,7 @@ async fn executor_stop_hooks_run_unless_regular_hooks_block_without_stopping() { timeout: Duration::from_secs(30), }, HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "terminate".to_string(), environment_id: None, @@ -2747,6 +2752,7 @@ async fn memory_consolidation_stop_preserves_policy_and_executor_cleanup() { executor_stop_hook_fixture(); request.target = StopHookTarget::MemoryConsolidation; let policy_call = HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "check".to_string(), environment_id: None, @@ -2913,6 +2919,7 @@ async fn mcp_tool_hooks_expand_event_input_and_apply_pre_tool_decisions() { assert_eq!( *calls.lock().expect("lock MCP calls"), vec![HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "scan".to_string(), environment_id: None, @@ -3015,6 +3022,7 @@ async fn mcp_interrupt_hooks_expand_event_input_and_bound_timeout() { assert_eq!( *calls.lock().expect("lock MCP calls"), vec![HookMcpCall { + skip_dormant_repl_cleanup: false, server: "security".to_string(), tool: "notify".to_string(), environment_id: None, diff --git a/codex-rs/hooks/src/mcp.rs b/codex-rs/hooks/src/mcp.rs index ca95d7b0cb..249ccfb945 100644 --- a/codex-rs/hooks/src/mcp.rs +++ b/codex-rs/hooks/src/mcp.rs @@ -7,6 +7,8 @@ use serde_json::Value; /// One MCP tool call requested by a configured hook handler. #[derive(Debug, Clone, PartialEq, Eq)] pub struct HookMcpCall { + /// A bundled REPL cleanup may skip a private local transport whose startup was never requested. + pub skip_dormant_repl_cleanup: bool, pub server: String, pub tool: String, pub environment_id: Option,