From 8afe928a76fd9013a0127edae5ed2de7508036e2 Mon Sep 17 00:00:00 2001 From: dob-oai Date: Sat, 14 Nov 2026 01:11:02 +0000 Subject: [PATCH] Avoid starting dormant REPL servers for cleanup hooks (#61437) ## Why Bundled `turn_ended` hooks can start an unused REPL server solely to clean it up, even though its deferred startup means there is no process to clean up. ## What changed Allow bundled cleanup calls for `node_repl` and `cua_repl` to return an empty successful result when startup has never been requested. Limit this behavior to configured, unshared local stdio connections using the legacy MCP protocol. Other calls continue to wait for server startup. ## Testing Add coverage for the connection eligibility checks and extend subagent hook tests to verify that bundled cleanup leaves unused REPL servers dormant while ordinary hooks can still start cached tools hidden from the model. GitOrigin-RevId: 0a07772fccd670c53348ff7a702b4bee353e263c --- codex-rs/codex-mcp/src/connection_manager.rs | 28 +++++++ .../codex-mcp/src/connection_manager_tests.rs | 65 +++++++++++++++ codex-rs/codex-mcp/src/lib.rs | 1 + codex-rs/codex-mcp/src/runtime.rs | 37 +++++++-- codex-rs/core/src/hook_mcp_executor.rs | 7 +- codex-rs/core/src/session/mod.rs | 3 +- codex-rs/core/tests/suite/hooks_mcp.rs | 82 +++++++++++++++---- codex-rs/core/tests/suite/mcp_auth_refresh.rs | 3 +- codex-rs/hooks/src/engine/mcp_runner.rs | 4 + codex-rs/hooks/src/engine/mcp_runner_tests.rs | 1 + codex-rs/hooks/src/engine/mod_tests.rs | 8 ++ codex-rs/hooks/src/mcp.rs | 2 + 12 files changed, 214 insertions(+), 27 deletions(-) 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,