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
This commit is contained in:
dob-oai
2026-11-14 01:34:12 +00:00
committed by copyberry
parent 9ac8c3f8d7
commit 8afe928a76
12 changed files with 214 additions and 27 deletions
@@ -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
@@ -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;
+1
View File
@@ -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;
+28 -9
View File
@@ -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<serde_json::Value>,
meta: Option<serde_json::Value>,
requested_timeout: Option<Duration>,
wait_for_server: bool,
startup: McpToolStartup,
) -> anyhow::Result<McpToolCallResult> {
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.
+6 -1
View File
@@ -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
+2 -1
View File
@@ -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()
+67 -15
View File
@@ -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!(
@@ -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));
+4
View File
@@ -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,
@@ -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,
+8
View File
@@ -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,
+2
View File
@@ -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<String>,