From c504b328b7d835c0b33fea762d9a3d796ec336dc Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 16:06:19 -0700 Subject: [PATCH 01/19] Make MCP connection startup fallible --- .../src/request_processors/mcp_processor.rs | 3 +- codex-rs/codex-mcp/src/connection_manager.rs | 99 ++++++++++++----- .../codex-mcp/src/connection_manager_tests.rs | 105 ++++++------------ codex-rs/codex-mcp/src/mcp/mod.rs | 21 ++-- codex-rs/core/src/connectors.rs | 7 +- codex-rs/core/src/mcp_tool_call_tests.rs | 7 +- codex-rs/core/src/session/mcp.rs | 32 +++--- codex-rs/core/src/session/session.rs | 58 ++-------- 8 files changed, 159 insertions(+), 173 deletions(-) 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 facd4facc63f..36832fc56f7c 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -279,7 +279,8 @@ impl McpRequestProcessor { runtime_context, detail, ) - .await; + .await + .map_err(|err| internal_error(format!("failed to collect MCP server status: {err:#}")))?; let McpServerStatusSnapshot { server_infos, diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 5558916e5bc3..69abbe6d9232 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -68,6 +68,8 @@ use serde_json::Value as JsonValue; use tokio::task::JoinSet; use tokio_util::sync::CancellationToken; use tracing::Instrument; +use tracing::Span; +use tracing::info_span; use tracing::instrument; use tracing::trace; use tracing::trace_span; @@ -121,6 +123,7 @@ impl McpConnectionManager { approval_policy: &Constrained, submit_id: String, tx_event: Sender, + startup_cancellation_token: CancellationToken, initial_permission_profile: PermissionProfile, runtime_context: McpRuntimeContext, codex_home: PathBuf, @@ -131,8 +134,25 @@ impl McpConnectionManager { tool_plugin_provenance: ToolPluginProvenance, auth: Option<&CodexAuth>, elicitation_reviewer: Option, - ) -> (Self, CancellationToken) { - let cancel_token = CancellationToken::new(); + ) -> Result { + let enabled_mcp_server_count = mcp_servers + .values() + .filter(|server| server.enabled()) + .count(); + let mut required_servers = mcp_servers + .iter() + .filter(|(_, server)| server.enabled() && server.required()) + .map(|(name, _)| name.clone()) + .collect::>(); + required_servers.sort(); + Span::current().record( + "session_init.enabled_mcp_server_count", + enabled_mcp_server_count, + ); + Span::current().record( + "session_init.required_mcp_server_count", + required_servers.len(), + ); let mut clients = HashMap::new(); let mut server_metadata = HashMap::new(); let mut join_set = JoinSet::new(); @@ -152,7 +172,7 @@ impl McpConnectionManager { .filter(|(_, server)| server.enabled()) { server_metadata.insert(server_name.clone(), McpServerMetadata::from(&server)); - let cancel_token = cancel_token.child_token(); + let cancel_token = startup_cancellation_token.child_token(); let _ = emit_update( startup_submit_id.as_str(), &tx_event, @@ -241,7 +261,7 @@ impl McpConnectionManager { host_owned_codex_apps_enabled, prefix_mcp_tool_names, elicitation_requests: elicitation_requests.clone(), - startup_cancellation_token: cancel_token.clone(), + startup_cancellation_token: startup_cancellation_token.clone(), }; tokio::spawn(async move { let outcomes = join_set.join_all().await; @@ -265,7 +285,26 @@ impl McpConnectionManager { }) .await; }); - (manager, cancel_token) + let failures = manager + .required_startup_failures(&required_servers) + .instrument(info_span!( + "session_init.required_mcp_wait", + otel.name = "session_init.required_mcp_wait", + session_init.required_mcp_server_count = required_servers.len(), + )) + .await; + if !failures.is_empty() { + startup_cancellation_token.cancel(); + let details = failures + .iter() + .map(|failure| format!("{}: {}", failure.server, failure.error)) + .collect::>() + .join("; "); + return Err(anyhow!( + "required MCP servers failed to initialize: {details}" + )); + } + Ok(manager) } pub fn new_uninitialized_with_permission_profile( @@ -374,31 +413,6 @@ impl McpConnectionManager { } } - pub async fn required_startup_failures( - &self, - required_servers: &[String], - ) -> Vec { - let mut failures = Vec::new(); - for server_name in required_servers { - let Some(async_managed_client) = self.clients.get(server_name).cloned() else { - failures.push(McpStartupFailure { - server: server_name.clone(), - error: format!("required MCP server `{server_name}` was not initialized"), - }); - continue; - }; - - match async_managed_client.client().await { - Ok(_) => {} - Err(error) => failures.push(McpStartupFailure { - server: server_name.clone(), - error: startup_outcome_error_message(error), - }), - } - } - failures - } - /// Returns all tools with model-visible names normalized. #[instrument(level = "trace", skip_all, fields(mcp_server_count = self.clients.len()))] pub async fn list_all_tools(&self) -> Vec { @@ -775,6 +789,31 @@ impl McpConnectionManager { .context("failed to get client") } + async fn required_startup_failures( + &self, + required_servers: &[String], + ) -> Vec { + let mut failures = Vec::new(); + for server_name in required_servers { + let Some(async_managed_client) = self.clients.get(server_name).cloned() else { + failures.push(McpStartupFailure { + server: server_name.clone(), + error: format!("required MCP server `{server_name}` was not initialized"), + }); + continue; + }; + + match async_managed_client.client().await { + Ok(_) => {} + Err(error) => failures.push(McpStartupFailure { + server: server_name.clone(), + error: startup_outcome_error_message(error), + }), + } + } + failures + } + #[cfg(test)] fn new_uninitialized( approval_policy: &Constrained, diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 60a0026eeb20..d75d9f7a3ad7 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1108,72 +1108,46 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { } #[tokio::test] -async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { +async fn required_local_stdio_without_local_runtime_fails_manager_construction() { let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let (tx_event, rx_event) = async_channel::unbounded(); drop(rx_event); let codex_home = tempdir().expect("tempdir"); - let mcp_servers = HashMap::from([ - ( - "stdio".to_string(), - EffectiveMcpServer::configured(McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "echo".to_string(), - args: Vec::new(), - env: None, - env_vars: Vec::new(), - cwd: None, - }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }), - ), - ( - "http".to_string(), - EffectiveMcpServer::configured(McpServerConfig { - transport: McpServerTransportConfig::StreamableHttp { - url: "http://127.0.0.1:1".to_string(), - bearer_token_env_var: None, - http_headers: None, - env_http_headers: None, - }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), - enabled: true, - required: false, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }), - ), - ]); + let mcp_servers = HashMap::from([( + "stdio".to_string(), + EffectiveMcpServer::configured(McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "echo".to_string(), + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: true, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }), + )]); - let (manager, cancel_token) = McpConnectionManager::new( + let result = McpConnectionManager::new( &mcp_servers, OAuthCredentialsStoreMode::default(), HashMap::new(), &approval_policy, String::new(), tx_event, + CancellationToken::new(), PermissionProfile::default(), McpRuntimeContext::new( Arc::new(EnvironmentManager::without_environments()), @@ -1194,23 +1168,14 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { ) .await; - assert!(manager.clients.contains_key("stdio")); - assert!(manager.clients.contains_key("http")); - assert!( - !manager - .wait_for_server_ready("stdio", Duration::from_millis(10)) - .await - ); - let failures = manager - .required_startup_failures(&["stdio".to_string()]) - .await; - assert_eq!(failures.len(), 1); - assert_eq!(failures[0].server, "stdio"); + let error = match result { + Ok(_) => panic!("required MCP startup should fail"), + Err(error) => error, + }; assert_eq!( - failures[0].error, - "local stdio MCP server `stdio` requires a local environment" + error.to_string(), + "required MCP servers failed to initialize: stdio: local stdio MCP server `stdio` requires a local environment" ); - cancel_token.cancel(); } #[test] diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index a74f95bae30a..253f7ba405ee 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -35,6 +35,7 @@ use rmcp::model::ElicitationCapability; use rmcp::model::ReadResourceRequestParams; use rmcp::model::ReadResourceResult; use serde_json::Value; +use tokio_util::sync::CancellationToken; use crate::codex_apps::codex_apps_tools_cache_key; use crate::connection_manager::McpConnectionManager; @@ -293,13 +294,15 @@ pub async fn read_mcp_resource( .await; let (tx_event, rx_event) = unbounded(); drop(rx_event); - let (manager, cancel_token) = McpConnectionManager::new( + let cancel_token = CancellationToken::new(); + let manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, auth_statuses, &config.approval_policy, String::new(), tx_event, + cancel_token.clone(), PermissionProfile::default(), runtime_context, config.codex_home.clone(), @@ -311,7 +314,7 @@ pub async fn read_mcp_resource( auth, /*elicitation_reviewer*/ None, ) - .await; + .await?; let result = manager .read_resource(server, ReadResourceRequestParams::new(uri)) @@ -336,19 +339,19 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( submit_id: String, runtime_context: McpRuntimeContext, detail: McpSnapshotDetail, -) -> McpServerStatusSnapshot { +) -> anyhow::Result { let mcp_servers = effective_mcp_servers(config, auth); let host_owned_codex_apps_enabled = host_owned_codex_apps_enabled(config, auth); let tool_plugin_provenance = tool_plugin_provenance(config); if mcp_servers.is_empty() { - return McpServerStatusSnapshot { + return Ok(McpServerStatusSnapshot { server_infos: HashMap::new(), tools_by_server: HashMap::new(), resources: HashMap::new(), resource_templates: HashMap::new(), auth_statuses: HashMap::new(), server_names: Vec::new(), - }; + }); } let auth_status_entries = compute_auth_statuses( @@ -363,13 +366,15 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( let (tx_event, rx_event) = unbounded(); drop(rx_event); - let (mcp_connection_manager, cancel_token) = McpConnectionManager::new( + let cancel_token = CancellationToken::new(); + let mcp_connection_manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, auth_status_entries.clone(), &config.approval_policy, submit_id, tx_event, + cancel_token.clone(), PermissionProfile::default(), runtime_context, config.codex_home.clone(), @@ -381,7 +386,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( auth, /*elicitation_reviewer*/ None, ) - .await; + .await?; let snapshot = collect_mcp_server_status_snapshot_from_manager( &mcp_connection_manager, @@ -393,7 +398,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( cancel_token.cancel(); - snapshot + Ok(snapshot) } /// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 623cb4421620..e058cfc8cbe3 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -17,6 +17,7 @@ use codex_protocol::models::PermissionProfile; use codex_tools::DiscoverableTool; use rmcp::model::ToolAnnotations; use serde::Deserialize; +use tokio_util::sync::CancellationToken; use tracing::warn; use crate::config::Config; @@ -285,13 +286,15 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( let (tx_event, rx_event) = unbounded(); drop(rx_event); - let (mut mcp_connection_manager, cancel_token) = McpConnectionManager::new( + let cancel_token = CancellationToken::new(); + let mut mcp_connection_manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, auth_status_entries, &config.permissions.approval_policy, INITIAL_SUBMIT_ID.to_owned(), tx_event, + cancel_token.clone(), PermissionProfile::default(), // Connector discovery is threadless. Use an actually configured env if // one exists, but do not reintroduce the old hidden-local fallback. @@ -305,7 +308,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( auth.as_ref(), /*elicitation_reviewer*/ None, ) - .await; + .await?; let refreshed_tools = if force_refetch { match mcp_connection_manager diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index bd71c3a0546e..141f72564a0e 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -46,6 +46,7 @@ use std::path::Path; use std::path::PathBuf; use std::sync::Arc; use tempfile::tempdir; +use tokio_util::sync::CancellationToken; use tracing::Instrument; use tracing::Level; use tracing_subscriber::fmt::format::FmtSpan; @@ -1256,13 +1257,14 @@ 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 (manager, _cancel_token) = codex_mcp::McpConnectionManager::new( + let manager = codex_mcp::McpConnectionManager::new( &HashMap::new(), turn_context.config.mcp_oauth_credentials_store_mode, HashMap::new(), &turn_context.approval_policy, turn_context.sub_id.clone(), session.get_tx_event(), + CancellationToken::new(), turn_context.permission_profile(), codex_mcp::McpRuntimeContext::new(Arc::clone(&session.services.environment_manager), { #[allow(deprecated)] @@ -1277,7 +1279,8 @@ async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: auth.as_ref(), /*elicitation_reviewer*/ None, ) - .await; + .await + .expect("construct MCP connection manager"); *session.services.mcp_connection_manager.write().await = manager; } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index f2643d0e5c9f..c3d0e7282c07 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -20,6 +20,7 @@ use rmcp::model::CreateElicitationRequestParams; use rmcp::model::ElicitationAction; use rmcp::model::Meta; use serde_json::Map; +use std::mem; const MCP_ELICITATION_DECLINE_MESSAGE_KEY: &str = "message"; const TOOL_SUGGESTION_ACTION_INSTALL: &str = "install"; @@ -338,18 +339,20 @@ impl Session { turn_context.cwd.to_path_buf(), ), }; - { + let (mcp_startup_cancellation_token, previous_mcp_startup_cancellation_token) = { let mut guard = self.services.mcp_startup_cancellation_token.lock().await; - guard.cancel(); - *guard = CancellationToken::new(); - } - let (refreshed_manager, cancel_token) = McpConnectionManager::new( + let cancel_token = CancellationToken::new(); + let previous_cancel_token = mem::replace(&mut *guard, cancel_token.clone()); + (cancel_token, previous_cancel_token) + }; + let manager_result = McpConnectionManager::new( &mcp_servers, store_mode, auth_statuses, &turn_context.approval_policy, turn_context.sub_id.clone(), self.get_tx_event(), + mcp_startup_cancellation_token, turn_context.permission_profile(), mcp_runtime_context, config.codex_home.to_path_buf(), @@ -362,21 +365,22 @@ impl Session { elicitation_reviewer, ) .await; + let refreshed_manager = match manager_result { + Ok(manager) => manager, + Err(err) => { + *self.services.mcp_startup_cancellation_token.lock().await = + previous_mcp_startup_cancellation_token; + warn!("failed to refresh MCP servers: {err:#}"); + return; + } + }; { let current_manager = self.services.mcp_connection_manager.read().await; refreshed_manager.set_elicitations_auto_deny(current_manager.elicitations_auto_deny()); } - { - let mut guard = self.services.mcp_startup_cancellation_token.lock().await; - if guard.is_cancelled() { - cancel_token.cancel(); - } - *guard = cancel_token; - } - let mut old_manager = { let mut manager = self.services.mcp_connection_manager.write().await; - std::mem::replace(&mut *manager, refreshed_manager) + mem::replace(&mut *manager, refreshed_manager) }; old_manager.shutdown().await; } diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 50ff984231ac..1fa9c473975e 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -16,6 +16,7 @@ use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::protocol::TurnEnvironmentSelections; use std::sync::OnceLock; use tokio::sync::Semaphore; +use tracing::field::Empty; /// Context for an initialized model agent /// @@ -1116,15 +1117,6 @@ impl Session { sess.send_event_raw(event).await; } - let mut required_mcp_servers: Vec = mcp_servers - .iter() - .filter(|(_, server)| server.enabled() && server.required()) - .map(|(name, _)| name.clone()) - .collect(); - required_mcp_servers.sort(); - let enabled_mcp_server_count = - mcp_servers.values().filter(|server| server.enabled()).count(); - let required_mcp_server_count = required_mcp_servers.len(); let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config.as_ref()).await; let host_owned_codex_apps_enabled = config .features @@ -1137,11 +1129,13 @@ impl Session { } else { ElicitationCapability::default() }; - { + let mcp_startup_cancellation_token = { let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await; cancel_guard.cancel(); - *cancel_guard = CancellationToken::new(); - } + let cancel_token = CancellationToken::new(); + *cancel_guard = cancel_token.clone(); + cancel_token + }; let turn_environment = crate::environment_selection::resolve_environment_selections( sess.services.environment_manager.as_ref(), session_configuration.environment_selections(), @@ -1164,13 +1158,14 @@ impl Session { session_configuration.cwd().to_path_buf(), ), }; - let (mcp_connection_manager, cancel_token) = McpConnectionManager::new( + let mcp_connection_manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, - auth_statuses.clone(), + auth_statuses, &session_configuration.approval_policy, INITIAL_SUBMIT_ID.to_owned(), tx_event.clone(), + mcp_startup_cancellation_token, session_configuration.permission_profile(), mcp_runtime_context, config.codex_home.to_path_buf(), @@ -1185,43 +1180,14 @@ impl Session { .instrument(info_span!( "session_init.mcp_manager_init", otel.name = "session_init.mcp_manager_init", - session_init.enabled_mcp_server_count = enabled_mcp_server_count, - session_init.required_mcp_server_count = required_mcp_server_count, + session_init.enabled_mcp_server_count = Empty, + session_init.required_mcp_server_count = Empty, )) - .await; + .await?; { let mut manager_guard = sess.services.mcp_connection_manager.write().await; *manager_guard = mcp_connection_manager; } - { - let mut cancel_guard = sess.services.mcp_startup_cancellation_token.lock().await; - if cancel_guard.is_cancelled() { - cancel_token.cancel(); - } - *cancel_guard = cancel_token; - } - if !required_mcp_servers.is_empty() { - let failures = sess - .services - .mcp_connection_manager - .read() - .await - .required_startup_failures(&required_mcp_servers) - .instrument(info_span!( - "session_init.required_mcp_wait", - otel.name = "session_init.required_mcp_wait", - session_init.required_mcp_server_count = required_mcp_server_count, - )) - .await; - if !failures.is_empty() { - let details = failures - .iter() - .map(|failure| format!("{}: {}", failure.server, failure.error)) - .collect::>() - .join("; "); - anyhow::bail!("required MCP servers failed to initialize: {details}"); - } - } sess.schedule_startup_prewarm(session_configuration.base_instructions.clone()) .await; let session_start_source = match &initial_history { From 6b4aa397aae0f0283f11b0d8da02da2e2ac88711 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 16:15:52 -0700 Subject: [PATCH 02/19] Cover MCP status startup error responses --- .../tests/suite/v2/mcp_server_status.rs | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 2c34684ada17..43335dedd9d2 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -10,6 +10,7 @@ use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; use app_test_support::write_mock_responses_config_toml; use axum::Router; +use codex_app_server_protocol::ExperimentalFeatureListParams; use codex_app_server_protocol::ListMcpServerStatusParams; use codex_app_server_protocol::ListMcpServerStatusResponse; use codex_app_server_protocol::McpServerStatusDetail; @@ -114,6 +115,71 @@ url = "{mcp_server_url}/mcp" Ok(()) } +#[tokio::test] +async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running() -> Result<()> { + let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; + let codex_home = TempDir::new()?; + write_mock_responses_config_toml( + codex_home.path(), + &server.uri(), + &BTreeMap::new(), + /*auto_compact_limit*/ 1024, + /*requires_openai_auth*/ None, + "mock_provider", + "compact", + )?; + + let config_path = codex_home.path().join("config.toml"); + let mut config_toml = std::fs::read_to_string(&config_path)?; + config_toml.push_str( + r#" +[mcp_servers.required_broken] +command = "codex-definitely-not-a-real-binary" +required = true +"#, + ); + std::fs::write(config_path, config_toml)?; + + let mut mcp = TestAppServer::new(codex_home.path()).await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_list_mcp_server_status_request(ListMcpServerStatusParams { + cursor: None, + limit: None, + detail: None, + thread_id: None, + }) + .await?; + let error = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert_eq!( + ( + error.error.code, + error + .error + .message + .contains("required MCP servers failed to initialize: required_broken"), + error.error.data, + ), + (-32603, true, None), + ); + + let follow_up_request_id = mcp + .send_experimental_feature_list_request(ExperimentalFeatureListParams::default()) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(follow_up_request_id)), + ) + .await??; + + Ok(()) +} + #[tokio::test] async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; From 67da7c5e3b76018780d3cbc7ef3119c56ce5e32f Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 16:21:36 -0700 Subject: [PATCH 03/19] Make MCP startup telemetry explicit --- codex-rs/codex-mcp/src/connection_manager.rs | 13 ------------- codex-rs/core/src/session/session.rs | 11 ++++++++--- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 69abbe6d9232..07f59d20911e 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -68,7 +68,6 @@ use serde_json::Value as JsonValue; use tokio::task::JoinSet; use tokio_util::sync::CancellationToken; use tracing::Instrument; -use tracing::Span; use tracing::info_span; use tracing::instrument; use tracing::trace; @@ -135,24 +134,12 @@ impl McpConnectionManager { auth: Option<&CodexAuth>, elicitation_reviewer: Option, ) -> Result { - let enabled_mcp_server_count = mcp_servers - .values() - .filter(|server| server.enabled()) - .count(); let mut required_servers = mcp_servers .iter() .filter(|(_, server)| server.enabled() && server.required()) .map(|(name, _)| name.clone()) .collect::>(); required_servers.sort(); - Span::current().record( - "session_init.enabled_mcp_server_count", - enabled_mcp_server_count, - ); - Span::current().record( - "session_init.required_mcp_server_count", - required_servers.len(), - ); let mut clients = HashMap::new(); let mut server_metadata = HashMap::new(); let mut join_set = JoinSet::new(); diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 1fa9c473975e..b212244ae5ff 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -16,7 +16,6 @@ use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::protocol::TurnEnvironmentSelections; use std::sync::OnceLock; use tokio::sync::Semaphore; -use tracing::field::Empty; /// Context for an initialized model agent /// @@ -1158,6 +1157,12 @@ impl Session { session_configuration.cwd().to_path_buf(), ), }; + let enabled_mcp_server_count = + mcp_servers.values().filter(|server| server.enabled()).count(); + let required_mcp_server_count = mcp_servers + .values() + .filter(|server| server.enabled() && server.required()) + .count(); let mcp_connection_manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, @@ -1180,8 +1185,8 @@ impl Session { .instrument(info_span!( "session_init.mcp_manager_init", otel.name = "session_init.mcp_manager_init", - session_init.enabled_mcp_server_count = Empty, - session_init.required_mcp_server_count = Empty, + session_init.enabled_mcp_server_count = enabled_mcp_server_count, + session_init.required_mcp_server_count = required_mcp_server_count, )) .await?; { From eb7002afa00aecddc79b7b2b032a15a78e336101 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 16:23:47 -0700 Subject: [PATCH 04/19] Drop MCP startup count telemetry --- codex-rs/core/src/session/session.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index b212244ae5ff..5cb9923d4d56 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1157,12 +1157,6 @@ impl Session { session_configuration.cwd().to_path_buf(), ), }; - let enabled_mcp_server_count = - mcp_servers.values().filter(|server| server.enabled()).count(); - let required_mcp_server_count = mcp_servers - .values() - .filter(|server| server.enabled() && server.required()) - .count(); let mcp_connection_manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, @@ -1185,8 +1179,6 @@ impl Session { .instrument(info_span!( "session_init.mcp_manager_init", otel.name = "session_init.mcp_manager_init", - session_init.enabled_mcp_server_count = enabled_mcp_server_count, - session_init.required_mcp_server_count = required_mcp_server_count, )) .await?; { From 34ec33192dfec437986360b687d9a0a888ca521d Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 16:28:50 -0700 Subject: [PATCH 05/19] Only replace MCP state after refresh succeeds --- codex-rs/core/src/session/mcp.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index c3d0e7282c07..e114d49ecc6f 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -339,20 +339,15 @@ impl Session { turn_context.cwd.to_path_buf(), ), }; - let (mcp_startup_cancellation_token, previous_mcp_startup_cancellation_token) = { - let mut guard = self.services.mcp_startup_cancellation_token.lock().await; - let cancel_token = CancellationToken::new(); - let previous_cancel_token = mem::replace(&mut *guard, cancel_token.clone()); - (cancel_token, previous_cancel_token) - }; - let manager_result = McpConnectionManager::new( + let mcp_startup_cancellation_token = CancellationToken::new(); + let refreshed_manager = match McpConnectionManager::new( &mcp_servers, store_mode, auth_statuses, &turn_context.approval_policy, turn_context.sub_id.clone(), self.get_tx_event(), - mcp_startup_cancellation_token, + mcp_startup_cancellation_token.clone(), turn_context.permission_profile(), mcp_runtime_context, config.codex_home.to_path_buf(), @@ -364,12 +359,10 @@ impl Session { auth.as_ref(), elicitation_reviewer, ) - .await; - let refreshed_manager = match manager_result { + .await + { Ok(manager) => manager, Err(err) => { - *self.services.mcp_startup_cancellation_token.lock().await = - previous_mcp_startup_cancellation_token; warn!("failed to refresh MCP servers: {err:#}"); return; } @@ -379,6 +372,9 @@ impl Session { refreshed_manager.set_elicitations_auto_deny(current_manager.elicitations_auto_deny()); } let mut old_manager = { + let mut cancellation_token = self.services.mcp_startup_cancellation_token.lock().await; + cancellation_token.cancel(); + *cancellation_token = mcp_startup_cancellation_token; let mut manager = self.services.mcp_connection_manager.write().await; mem::replace(&mut *manager, refreshed_manager) }; From 41dda768e5f3d9d339982a94b8d6a21399d68c01 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:03:29 -0700 Subject: [PATCH 06/19] Reuse session MCP connections for status listing --- codex-rs/app-server/src/request_processors.rs | 2 +- .../src/request_processors/mcp_processor.rs | 96 +++++++------------ .../tests/suite/v2/mcp_server_status.rs | 87 +++++++++-------- codex-rs/codex-mcp/src/connection_manager.rs | 21 ++++ codex-rs/codex-mcp/src/lib.rs | 3 +- codex-rs/codex-mcp/src/mcp/mod.rs | 72 +++----------- codex-rs/core/src/codex_thread.rs | 9 ++ codex-rs/core/src/session/mcp.rs | 12 +++ codex-rs/core/src/session/mod.rs | 3 + 9 files changed, 144 insertions(+), 161 deletions(-) diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index 00f17937ba0e..f90c9054ac47 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -354,7 +354,7 @@ use codex_login::run_login_server; 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::collect_configured_mcp_server_status_snapshot; use codex_mcp::discover_supported_scopes; use codex_mcp::read_mcp_resource as read_mcp_resource_without_thread; use codex_mcp::resolve_oauth_scopes; 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 36832fc56f7c..11118b567cd2 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -2,6 +2,14 @@ use super::*; const MCP_TOOL_THREAD_ID_META_KEY: &str = "threadId"; +enum McpServerStatusSource { + Config { + config: codex_mcp::McpConfig, + auth: Option, + }, + Thread(Arc), +} + #[derive(Clone)] pub(crate) struct McpRequestProcessor { auth_manager: Arc, @@ -203,40 +211,25 @@ impl McpRequestProcessor { let request = request_id.clone(); let outgoing = Arc::clone(&self.outgoing); - let config = match params.thread_id.as_deref() { + let source = match params.thread_id.as_deref() { Some(thread_id) => { let (_, thread) = self.load_thread(thread_id).await?; - let thread_config = thread.config().await; - self.config_manager - .load_latest_config_for_thread(thread_config.as_ref()) - .await - .map_err(|err| internal_error(format!("failed to reload config: {err}")))? + McpServerStatusSource::Thread(thread) + } + None => { + let config = self.load_latest_config(/*fallback_cwd*/ None).await?; + let config = self + .thread_manager + .mcp_manager() + .runtime_config(&config) + .await; + let auth = self.auth_manager.auth().await; + McpServerStatusSource::Config { config, auth } } - None => self.load_latest_config(/*fallback_cwd*/ None).await?, }; - let mcp_config = self - .thread_manager - .mcp_manager() - .runtime_config(&config) - .await; - let auth = self.auth_manager.auth().await; - let environment_manager = self.thread_manager.environment_manager(); - // This status path has no turn-selected environment. Use config cwd - // 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, - mcp_config, - auth, - runtime_context, - ) - .await; + Self::list_mcp_server_status_task(outgoing, request, params, source).await; }); Ok(()) } @@ -245,43 +238,28 @@ impl McpRequestProcessor { outgoing: Arc, request_id: ConnectionRequestId, params: ListMcpServerStatusParams, - mcp_config: codex_mcp::McpConfig, - auth: Option, - runtime_context: McpRuntimeContext, + source: McpServerStatusSource, ) { - let result = Self::list_mcp_server_status_response( - request_id.request_id.to_string(), - params, - mcp_config, - auth, - runtime_context, - ) - .await; - outgoing.send_result(request_id, result).await; - } - - async fn list_mcp_server_status_response( - request_id: String, - params: ListMcpServerStatusParams, - mcp_config: codex_mcp::McpConfig, - auth: Option, - runtime_context: McpRuntimeContext, - ) -> Result { let detail = match params.detail.unwrap_or(McpServerStatusDetail::Full) { McpServerStatusDetail::Full => McpSnapshotDetail::Full, McpServerStatusDetail::ToolsAndAuthOnly => McpSnapshotDetail::ToolsAndAuthOnly, }; + let snapshot = match source { + McpServerStatusSource::Config { config, auth } => { + collect_configured_mcp_server_status_snapshot(&config, auth.as_ref()).await + } + McpServerStatusSource::Thread(thread) => { + thread.mcp_server_status_snapshot(detail).await + } + }; + let result = Self::list_mcp_server_status_response(params, snapshot); + outgoing.send_result(request_id, result).await; + } - let snapshot = collect_mcp_server_status_snapshot_with_detail( - &mcp_config, - auth.as_ref(), - request_id, - runtime_context, - detail, - ) - .await - .map_err(|err| internal_error(format!("failed to collect MCP server status: {err:#}")))?; - + fn list_mcp_server_status_response( + params: ListMcpServerStatusParams, + snapshot: McpServerStatusSnapshot, + ) -> Result { let McpServerStatusSnapshot { server_infos, tools_by_server, diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 43335dedd9d2..7842c63bde2a 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; @@ -10,9 +11,10 @@ use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; use app_test_support::write_mock_responses_config_toml; use axum::Router; -use codex_app_server_protocol::ExperimentalFeatureListParams; use codex_app_server_protocol::ListMcpServerStatusParams; use codex_app_server_protocol::ListMcpServerStatusResponse; +use codex_app_server_protocol::McpAuthStatus; +use codex_app_server_protocol::McpServerStatus; use codex_app_server_protocol::McpServerStatusDetail; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ThreadStartParams; @@ -70,13 +72,14 @@ url = "{mcp_server_url}/mcp" let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_thread(&mut mcp, ThreadStartParams::default()).await?; let request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { cursor: None, limit: None, detail: None, - thread_id: None, + thread_id: Some(thread_id), }) .await?; let response = timeout( @@ -116,7 +119,7 @@ url = "{mcp_server_url}/mcp" } #[tokio::test] -async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running() -> Result<()> { +async fn threadless_mcp_server_status_list_does_not_start_configured_servers() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let codex_home = TempDir::new()?; write_mock_responses_config_toml( @@ -151,37 +154,32 @@ required = true thread_id: None, }) .await?; - let error = timeout( + let response = timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), ) .await??; + let response: ListMcpServerStatusResponse = to_response(response)?; assert_eq!( - ( - error.error.code, - error - .error - .message - .contains("required MCP servers failed to initialize: required_broken"), - error.error.data, - ), - (-32603, true, None), + response, + ListMcpServerStatusResponse { + data: vec![McpServerStatus { + name: "required_broken".to_string(), + server_info: None, + tools: HashMap::new(), + resources: Vec::new(), + resource_templates: Vec::new(), + auth_status: McpAuthStatus::Unsupported, + }], + next_cursor: None, + } ); - let follow_up_request_id = mcp - .send_experimental_feature_list_request(ExperimentalFeatureListParams::default()) - .await?; - timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(follow_up_request_id)), - ) - .await??; - Ok(()) } #[tokio::test] -async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> { +async fn mcp_server_status_list_uses_thread_connection_manager() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let (mcp_server_url, mcp_server_handle) = start_mcp_server("project_lookup").await?; let codex_home = TempDir::new()?; @@ -201,19 +199,6 @@ async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - let thread_start_id = mcp - .send_thread_start_request(ThreadStartParams { - cwd: Some(workspace.path().to_string_lossy().into_owned()), - ..Default::default() - }) - .await?; - let thread_start_response = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)), - ) - .await??; - let ThreadStartResponse { thread, .. } = to_response(thread_start_response)?; - let project_config_dir = workspace.path().join(".codex"); std::fs::create_dir_all(&project_config_dir)?; std::fs::write( @@ -225,6 +210,15 @@ url = "{mcp_server_url}/mcp" "# ), )?; + let thread_id = start_thread( + &mut mcp, + ThreadStartParams { + cwd: Some(workspace.path().to_string_lossy().into_owned()), + ..Default::default() + }, + ) + .await?; + std::fs::write(project_config_dir.join("config.toml"), "")?; let threadless_request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { @@ -247,7 +241,7 @@ url = "{mcp_server_url}/mcp" cursor: None, limit: None, detail: Some(McpServerStatusDetail::ToolsAndAuthOnly), - thread_id: Some(thread.id), + thread_id: Some(thread_id), }) .await?; let thread_response = timeout( @@ -272,6 +266,17 @@ url = "{mcp_server_url}/mcp" Ok(()) } +async fn start_thread(mcp: &mut TestAppServer, params: ThreadStartParams) -> Result { + let request_id = mcp.send_thread_start_request(params).await?; + let response = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response(response)?; + Ok(thread.id) +} + #[derive(Clone)] struct McpStatusServer { tool_name: Arc, @@ -404,13 +409,14 @@ url = "{mcp_server_url}/mcp" let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_thread(&mut mcp, ThreadStartParams::default()).await?; let request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { cursor: None, limit: None, detail: Some(McpServerStatusDetail::ToolsAndAuthOnly), - thread_id: None, + thread_id: Some(thread_id), }) .await?; let response = timeout( @@ -469,13 +475,14 @@ url = "{underscore_server_url}/mcp" let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_id = start_thread(&mut mcp, ThreadStartParams::default()).await?; let request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { cursor: None, limit: None, detail: None, - thread_id: None, + thread_id: Some(thread_id), }) .await?; let response = timeout( diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 07f59d20911e..c2a15f4fbce4 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -50,6 +50,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::McpAuthStatus; use codex_protocol::protocol::McpStartupCompleteEvent; use codex_protocol::protocol::McpStartupFailure; use codex_protocol::protocol::McpStartupStatus; @@ -105,6 +106,8 @@ pub fn tool_is_model_visible(tool: &ToolInfo) -> bool { /// A thin wrapper around a set of running [`RmcpClient`] instances. pub struct McpConnectionManager { clients: HashMap, + auth_statuses: HashMap, + status_server_names: Vec, server_metadata: HashMap, tool_plugin_provenance: Arc, host_owned_codex_apps_enabled: bool, @@ -134,6 +137,12 @@ impl McpConnectionManager { auth: Option<&CodexAuth>, elicitation_reviewer: Option, ) -> Result { + let auth_statuses = auth_entries + .iter() + .map(|(name, entry)| (name.clone(), entry.auth_status)) + .collect(); + let mut status_server_names = mcp_servers.keys().cloned().collect::>(); + status_server_names.sort(); let mut required_servers = mcp_servers .iter() .filter(|(_, server)| server.enabled() && server.required()) @@ -243,6 +252,8 @@ impl McpConnectionManager { } let manager = Self { clients, + auth_statuses, + status_server_names, server_metadata, tool_plugin_provenance, host_owned_codex_apps_enabled, @@ -301,6 +312,8 @@ impl McpConnectionManager { ) -> Self { Self { clients: HashMap::new(), + auth_statuses: HashMap::new(), + status_server_names: Vec::new(), server_metadata: HashMap::new(), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), host_owned_codex_apps_enabled: false, @@ -318,6 +331,14 @@ impl McpConnectionManager { !self.clients.is_empty() } + pub(crate) fn status_server_names(&self) -> Vec { + self.status_server_names.clone() + } + + pub(crate) fn auth_statuses(&self) -> HashMap { + self.auth_statuses.clone() + } + /// Drain all MCP clients from this manager and return a future that stops /// them and terminates their stdio server processes. pub fn begin_shutdown(&mut self) -> impl std::future::Future + Send + 'static { diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index a56365277a2d..c5d8d275914f 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -34,7 +34,8 @@ pub use mcp::with_codex_apps_mcp; pub use mcp::McpServerStatusSnapshot; pub use mcp::McpSnapshotDetail; -pub use mcp::collect_mcp_server_status_snapshot_with_detail; +pub use mcp::collect_configured_mcp_server_status_snapshot; +pub use mcp::collect_mcp_server_status_snapshot; pub use mcp::read_mcp_resource; pub use mcp::McpAuthStatusEntry; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 253f7ba405ee..26d5fbce4a75 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -333,72 +333,26 @@ pub struct McpServerStatusSnapshot { pub server_names: Vec, } -pub async fn collect_mcp_server_status_snapshot_with_detail( +pub async fn collect_configured_mcp_server_status_snapshot( config: &McpConfig, auth: Option<&CodexAuth>, - submit_id: String, - runtime_context: McpRuntimeContext, - detail: McpSnapshotDetail, -) -> anyhow::Result { +) -> McpServerStatusSnapshot { let mcp_servers = effective_mcp_servers(config, auth); - let host_owned_codex_apps_enabled = host_owned_codex_apps_enabled(config, auth); - let tool_plugin_provenance = tool_plugin_provenance(config); - if mcp_servers.is_empty() { - return Ok(McpServerStatusSnapshot { - server_infos: HashMap::new(), - tools_by_server: HashMap::new(), - resources: HashMap::new(), - resource_templates: HashMap::new(), - auth_statuses: HashMap::new(), - server_names: Vec::new(), - }); - } - let auth_status_entries = compute_auth_statuses( mcp_servers.iter(), config.mcp_oauth_credentials_store_mode, auth, ) .await; - let server_names = mcp_servers.keys().cloned().collect(); - - let (tx_event, rx_event) = unbounded(); - drop(rx_event); - - let cancel_token = CancellationToken::new(); - let mcp_connection_manager = McpConnectionManager::new( - &mcp_servers, - config.mcp_oauth_credentials_store_mode, - auth_status_entries.clone(), - &config.approval_policy, - submit_id, - tx_event, - cancel_token.clone(), - PermissionProfile::default(), - runtime_context, - config.codex_home.clone(), - codex_apps_tools_cache_key(auth), - host_owned_codex_apps_enabled, - config.prefix_mcp_tool_names, - config.client_elicitation_capability.clone(), - tool_plugin_provenance, - auth, - /*elicitation_reviewer*/ None, - ) - .await?; - - let snapshot = collect_mcp_server_status_snapshot_from_manager( - &mcp_connection_manager, - auth_status_entries, + McpServerStatusSnapshot { + server_infos: HashMap::new(), + tools_by_server: HashMap::new(), + resources: HashMap::new(), + resource_templates: HashMap::new(), + auth_statuses: auth_statuses_from_entries(&auth_status_entries), server_names, - detail, - ) - .await; - - cancel_token.cancel(); - - Ok(snapshot) + } } /// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. @@ -594,10 +548,8 @@ fn convert_mcp_resource_templates( .collect::>() } -async fn collect_mcp_server_status_snapshot_from_manager( +pub async fn collect_mcp_server_status_snapshot( mcp_connection_manager: &McpConnectionManager, - auth_status_entries: HashMap, - server_names: Vec, detail: McpSnapshotDetail, ) -> McpServerStatusSnapshot { let (tools, resources, resource_templates) = tokio::join!( @@ -637,8 +589,8 @@ async fn collect_mcp_server_status_snapshot_from_manager( tools_by_server, resources: convert_mcp_resources(resources), resource_templates: convert_mcp_resource_templates(resource_templates), - auth_statuses: auth_statuses_from_entries(&auth_status_entries), - server_names, + auth_statuses: mcp_connection_manager.auth_statuses(), + server_names: mcp_connection_manager.status_server_names(), } } diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index ab1f6df03ed2..75c7e25d73c0 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -4,6 +4,8 @@ use crate::session::Codex; use crate::session::SessionSettingsUpdate; use crate::session::SteerInputError; use codex_features::Feature; +use codex_mcp::McpServerStatusSnapshot; +use codex_mcp::McpSnapshotDetail; use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; @@ -531,6 +533,13 @@ impl CodexThread { self.codex.session.get_config().await } + pub async fn mcp_server_status_snapshot( + &self, + detail: McpSnapshotDetail, + ) -> McpServerStatusSnapshot { + self.codex.session.mcp_server_status_snapshot(detail).await + } + pub fn multi_agent_version(&self) -> Option { self.codex.session.multi_agent_version() } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index e114d49ecc6f..296eb273c119 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -79,6 +79,18 @@ impl Session { Arc::new(GuardianMcpElicitationReviewer::new(self)) } + #[expect( + clippy::await_holding_invalid_type, + reason = "MCP inventory reads are serialized through the session-owned manager guard" + )] + pub(crate) async fn mcp_server_status_snapshot( + &self, + detail: McpSnapshotDetail, + ) -> McpServerStatusSnapshot { + let manager = self.services.mcp_connection_manager.read().await; + collect_mcp_server_status_snapshot(&manager, detail).await + } + #[expect( clippy::await_holding_invalid_type, reason = "active turn checks and turn state updates must remain atomic" diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 3e71afee8a66..56c73e705ac7 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -316,6 +316,9 @@ use crate::unified_exec::UnifiedExecProcessManager; use crate::windows_sandbox::WindowsSandboxLevelExt; use codex_core_plugins::PluginsManager; use codex_git_utils::get_git_repo_root; +use codex_mcp::McpServerStatusSnapshot; +use codex_mcp::McpSnapshotDetail; +use codex_mcp::collect_mcp_server_status_snapshot; use codex_mcp::compute_auth_statuses; use codex_mcp::effective_mcp_servers_from_configured; use codex_mcp::host_owned_codex_apps_enabled; From abfe7ab2d0ad692c724a49bddfd64e40c836307b Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:05:14 -0700 Subject: [PATCH 07/19] Revert "Reuse session MCP connections for status listing" This reverts commit 41dda768e5f3d9d339982a94b8d6a21399d68c01. --- codex-rs/app-server/src/request_processors.rs | 2 +- .../src/request_processors/mcp_processor.rs | 96 ++++++++++++------- .../tests/suite/v2/mcp_server_status.rs | 87 ++++++++--------- codex-rs/codex-mcp/src/connection_manager.rs | 21 ---- codex-rs/codex-mcp/src/lib.rs | 3 +- codex-rs/codex-mcp/src/mcp/mod.rs | 72 +++++++++++--- codex-rs/core/src/codex_thread.rs | 9 -- codex-rs/core/src/session/mcp.rs | 12 --- codex-rs/core/src/session/mod.rs | 3 - 9 files changed, 161 insertions(+), 144 deletions(-) diff --git a/codex-rs/app-server/src/request_processors.rs b/codex-rs/app-server/src/request_processors.rs index f90c9054ac47..00f17937ba0e 100644 --- a/codex-rs/app-server/src/request_processors.rs +++ b/codex-rs/app-server/src/request_processors.rs @@ -354,7 +354,7 @@ use codex_login::run_login_server; use codex_mcp::McpRuntimeContext; use codex_mcp::McpServerStatusSnapshot; use codex_mcp::McpSnapshotDetail; -use codex_mcp::collect_configured_mcp_server_status_snapshot; +use codex_mcp::collect_mcp_server_status_snapshot_with_detail; use codex_mcp::discover_supported_scopes; use codex_mcp::read_mcp_resource as read_mcp_resource_without_thread; use codex_mcp::resolve_oauth_scopes; 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 11118b567cd2..36832fc56f7c 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -2,14 +2,6 @@ use super::*; const MCP_TOOL_THREAD_ID_META_KEY: &str = "threadId"; -enum McpServerStatusSource { - Config { - config: codex_mcp::McpConfig, - auth: Option, - }, - Thread(Arc), -} - #[derive(Clone)] pub(crate) struct McpRequestProcessor { auth_manager: Arc, @@ -211,25 +203,40 @@ impl McpRequestProcessor { let request = request_id.clone(); let outgoing = Arc::clone(&self.outgoing); - let source = match params.thread_id.as_deref() { + let config = match params.thread_id.as_deref() { Some(thread_id) => { let (_, thread) = self.load_thread(thread_id).await?; - McpServerStatusSource::Thread(thread) - } - None => { - let config = self.load_latest_config(/*fallback_cwd*/ None).await?; - let config = self - .thread_manager - .mcp_manager() - .runtime_config(&config) - .await; - let auth = self.auth_manager.auth().await; - McpServerStatusSource::Config { config, auth } + let thread_config = thread.config().await; + self.config_manager + .load_latest_config_for_thread(thread_config.as_ref()) + .await + .map_err(|err| internal_error(format!("failed to reload config: {err}")))? } + None => self.load_latest_config(/*fallback_cwd*/ None).await?, }; + let mcp_config = self + .thread_manager + .mcp_manager() + .runtime_config(&config) + .await; + let auth = self.auth_manager.auth().await; + let environment_manager = self.thread_manager.environment_manager(); + // This status path has no turn-selected environment. Use config cwd + // 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, source).await; + Self::list_mcp_server_status_task( + outgoing, + request, + params, + mcp_config, + auth, + runtime_context, + ) + .await; }); Ok(()) } @@ -238,28 +245,43 @@ impl McpRequestProcessor { outgoing: Arc, request_id: ConnectionRequestId, params: ListMcpServerStatusParams, - source: McpServerStatusSource, + mcp_config: codex_mcp::McpConfig, + auth: Option, + runtime_context: McpRuntimeContext, ) { - let detail = match params.detail.unwrap_or(McpServerStatusDetail::Full) { - McpServerStatusDetail::Full => McpSnapshotDetail::Full, - McpServerStatusDetail::ToolsAndAuthOnly => McpSnapshotDetail::ToolsAndAuthOnly, - }; - let snapshot = match source { - McpServerStatusSource::Config { config, auth } => { - collect_configured_mcp_server_status_snapshot(&config, auth.as_ref()).await - } - McpServerStatusSource::Thread(thread) => { - thread.mcp_server_status_snapshot(detail).await - } - }; - let result = Self::list_mcp_server_status_response(params, snapshot); + let result = Self::list_mcp_server_status_response( + request_id.request_id.to_string(), + params, + mcp_config, + auth, + runtime_context, + ) + .await; outgoing.send_result(request_id, result).await; } - fn list_mcp_server_status_response( + async fn list_mcp_server_status_response( + request_id: String, params: ListMcpServerStatusParams, - snapshot: McpServerStatusSnapshot, + mcp_config: codex_mcp::McpConfig, + auth: Option, + runtime_context: McpRuntimeContext, ) -> Result { + let detail = match params.detail.unwrap_or(McpServerStatusDetail::Full) { + McpServerStatusDetail::Full => McpSnapshotDetail::Full, + McpServerStatusDetail::ToolsAndAuthOnly => McpSnapshotDetail::ToolsAndAuthOnly, + }; + + let snapshot = collect_mcp_server_status_snapshot_with_detail( + &mcp_config, + auth.as_ref(), + request_id, + runtime_context, + detail, + ) + .await + .map_err(|err| internal_error(format!("failed to collect MCP server status: {err:#}")))?; + let McpServerStatusSnapshot { server_infos, tools_by_server, diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 7842c63bde2a..43335dedd9d2 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -1,7 +1,6 @@ use std::borrow::Cow; use std::collections::BTreeMap; use std::collections::BTreeSet; -use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; @@ -11,10 +10,9 @@ use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; use app_test_support::write_mock_responses_config_toml; use axum::Router; +use codex_app_server_protocol::ExperimentalFeatureListParams; use codex_app_server_protocol::ListMcpServerStatusParams; use codex_app_server_protocol::ListMcpServerStatusResponse; -use codex_app_server_protocol::McpAuthStatus; -use codex_app_server_protocol::McpServerStatus; use codex_app_server_protocol::McpServerStatusDetail; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ThreadStartParams; @@ -72,14 +70,13 @@ url = "{mcp_server_url}/mcp" let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - let thread_id = start_thread(&mut mcp, ThreadStartParams::default()).await?; let request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { cursor: None, limit: None, detail: None, - thread_id: Some(thread_id), + thread_id: None, }) .await?; let response = timeout( @@ -119,7 +116,7 @@ url = "{mcp_server_url}/mcp" } #[tokio::test] -async fn threadless_mcp_server_status_list_does_not_start_configured_servers() -> Result<()> { +async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let codex_home = TempDir::new()?; write_mock_responses_config_toml( @@ -154,32 +151,37 @@ required = true thread_id: None, }) .await?; - let response = timeout( + let error = timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), ) .await??; - let response: ListMcpServerStatusResponse = to_response(response)?; assert_eq!( - response, - ListMcpServerStatusResponse { - data: vec![McpServerStatus { - name: "required_broken".to_string(), - server_info: None, - tools: HashMap::new(), - resources: Vec::new(), - resource_templates: Vec::new(), - auth_status: McpAuthStatus::Unsupported, - }], - next_cursor: None, - } + ( + error.error.code, + error + .error + .message + .contains("required MCP servers failed to initialize: required_broken"), + error.error.data, + ), + (-32603, true, None), ); + let follow_up_request_id = mcp + .send_experimental_feature_list_request(ExperimentalFeatureListParams::default()) + .await?; + timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(follow_up_request_id)), + ) + .await??; + Ok(()) } #[tokio::test] -async fn mcp_server_status_list_uses_thread_connection_manager() -> Result<()> { +async fn mcp_server_status_list_uses_thread_project_local_config() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let (mcp_server_url, mcp_server_handle) = start_mcp_server("project_lookup").await?; let codex_home = TempDir::new()?; @@ -199,6 +201,19 @@ async fn mcp_server_status_list_uses_thread_connection_manager() -> Result<()> { let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + let thread_start_id = mcp + .send_thread_start_request(ThreadStartParams { + cwd: Some(workspace.path().to_string_lossy().into_owned()), + ..Default::default() + }) + .await?; + let thread_start_response = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)), + ) + .await??; + let ThreadStartResponse { thread, .. } = to_response(thread_start_response)?; + let project_config_dir = workspace.path().join(".codex"); std::fs::create_dir_all(&project_config_dir)?; std::fs::write( @@ -210,15 +225,6 @@ url = "{mcp_server_url}/mcp" "# ), )?; - let thread_id = start_thread( - &mut mcp, - ThreadStartParams { - cwd: Some(workspace.path().to_string_lossy().into_owned()), - ..Default::default() - }, - ) - .await?; - std::fs::write(project_config_dir.join("config.toml"), "")?; let threadless_request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { @@ -241,7 +247,7 @@ url = "{mcp_server_url}/mcp" cursor: None, limit: None, detail: Some(McpServerStatusDetail::ToolsAndAuthOnly), - thread_id: Some(thread_id), + thread_id: Some(thread.id), }) .await?; let thread_response = timeout( @@ -266,17 +272,6 @@ url = "{mcp_server_url}/mcp" Ok(()) } -async fn start_thread(mcp: &mut TestAppServer, params: ThreadStartParams) -> Result { - let request_id = mcp.send_thread_start_request(params).await?; - let response = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), - ) - .await??; - let ThreadStartResponse { thread, .. } = to_response(response)?; - Ok(thread.id) -} - #[derive(Clone)] struct McpStatusServer { tool_name: Arc, @@ -409,14 +404,13 @@ url = "{mcp_server_url}/mcp" let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - let thread_id = start_thread(&mut mcp, ThreadStartParams::default()).await?; let request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { cursor: None, limit: None, detail: Some(McpServerStatusDetail::ToolsAndAuthOnly), - thread_id: Some(thread_id), + thread_id: None, }) .await?; let response = timeout( @@ -475,14 +469,13 @@ url = "{underscore_server_url}/mcp" let mut mcp = TestAppServer::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - let thread_id = start_thread(&mut mcp, ThreadStartParams::default()).await?; let request_id = mcp .send_list_mcp_server_status_request(ListMcpServerStatusParams { cursor: None, limit: None, detail: None, - thread_id: Some(thread_id), + thread_id: None, }) .await?; let response = timeout( diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index c2a15f4fbce4..07f59d20911e 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -50,7 +50,6 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; -use codex_protocol::protocol::McpAuthStatus; use codex_protocol::protocol::McpStartupCompleteEvent; use codex_protocol::protocol::McpStartupFailure; use codex_protocol::protocol::McpStartupStatus; @@ -106,8 +105,6 @@ pub fn tool_is_model_visible(tool: &ToolInfo) -> bool { /// A thin wrapper around a set of running [`RmcpClient`] instances. pub struct McpConnectionManager { clients: HashMap, - auth_statuses: HashMap, - status_server_names: Vec, server_metadata: HashMap, tool_plugin_provenance: Arc, host_owned_codex_apps_enabled: bool, @@ -137,12 +134,6 @@ impl McpConnectionManager { auth: Option<&CodexAuth>, elicitation_reviewer: Option, ) -> Result { - let auth_statuses = auth_entries - .iter() - .map(|(name, entry)| (name.clone(), entry.auth_status)) - .collect(); - let mut status_server_names = mcp_servers.keys().cloned().collect::>(); - status_server_names.sort(); let mut required_servers = mcp_servers .iter() .filter(|(_, server)| server.enabled() && server.required()) @@ -252,8 +243,6 @@ impl McpConnectionManager { } let manager = Self { clients, - auth_statuses, - status_server_names, server_metadata, tool_plugin_provenance, host_owned_codex_apps_enabled, @@ -312,8 +301,6 @@ impl McpConnectionManager { ) -> Self { Self { clients: HashMap::new(), - auth_statuses: HashMap::new(), - status_server_names: Vec::new(), server_metadata: HashMap::new(), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), host_owned_codex_apps_enabled: false, @@ -331,14 +318,6 @@ impl McpConnectionManager { !self.clients.is_empty() } - pub(crate) fn status_server_names(&self) -> Vec { - self.status_server_names.clone() - } - - pub(crate) fn auth_statuses(&self) -> HashMap { - self.auth_statuses.clone() - } - /// Drain all MCP clients from this manager and return a future that stops /// them and terminates their stdio server processes. pub fn begin_shutdown(&mut self) -> impl std::future::Future + Send + 'static { diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index c5d8d275914f..a56365277a2d 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -34,8 +34,7 @@ pub use mcp::with_codex_apps_mcp; pub use mcp::McpServerStatusSnapshot; pub use mcp::McpSnapshotDetail; -pub use mcp::collect_configured_mcp_server_status_snapshot; -pub use mcp::collect_mcp_server_status_snapshot; +pub use mcp::collect_mcp_server_status_snapshot_with_detail; pub use mcp::read_mcp_resource; pub use mcp::McpAuthStatusEntry; diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 26d5fbce4a75..253f7ba405ee 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -333,26 +333,72 @@ pub struct McpServerStatusSnapshot { pub server_names: Vec, } -pub async fn collect_configured_mcp_server_status_snapshot( +pub async fn collect_mcp_server_status_snapshot_with_detail( config: &McpConfig, auth: Option<&CodexAuth>, -) -> McpServerStatusSnapshot { + submit_id: String, + runtime_context: McpRuntimeContext, + detail: McpSnapshotDetail, +) -> anyhow::Result { let mcp_servers = effective_mcp_servers(config, auth); + let host_owned_codex_apps_enabled = host_owned_codex_apps_enabled(config, auth); + let tool_plugin_provenance = tool_plugin_provenance(config); + if mcp_servers.is_empty() { + return Ok(McpServerStatusSnapshot { + server_infos: HashMap::new(), + tools_by_server: HashMap::new(), + resources: HashMap::new(), + resource_templates: HashMap::new(), + auth_statuses: HashMap::new(), + server_names: Vec::new(), + }); + } + let auth_status_entries = compute_auth_statuses( mcp_servers.iter(), config.mcp_oauth_credentials_store_mode, auth, ) .await; + let server_names = mcp_servers.keys().cloned().collect(); - McpServerStatusSnapshot { - server_infos: HashMap::new(), - tools_by_server: HashMap::new(), - resources: HashMap::new(), - resource_templates: HashMap::new(), - auth_statuses: auth_statuses_from_entries(&auth_status_entries), + + let (tx_event, rx_event) = unbounded(); + drop(rx_event); + + let cancel_token = CancellationToken::new(); + let mcp_connection_manager = McpConnectionManager::new( + &mcp_servers, + config.mcp_oauth_credentials_store_mode, + auth_status_entries.clone(), + &config.approval_policy, + submit_id, + tx_event, + cancel_token.clone(), + PermissionProfile::default(), + runtime_context, + config.codex_home.clone(), + codex_apps_tools_cache_key(auth), + host_owned_codex_apps_enabled, + config.prefix_mcp_tool_names, + config.client_elicitation_capability.clone(), + tool_plugin_provenance, + auth, + /*elicitation_reviewer*/ None, + ) + .await?; + + let snapshot = collect_mcp_server_status_snapshot_from_manager( + &mcp_connection_manager, + auth_status_entries, server_names, - } + detail, + ) + .await; + + cancel_token.cancel(); + + Ok(snapshot) } /// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. @@ -548,8 +594,10 @@ fn convert_mcp_resource_templates( .collect::>() } -pub async fn collect_mcp_server_status_snapshot( +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!( @@ -589,8 +637,8 @@ pub async fn collect_mcp_server_status_snapshot( tools_by_server, resources: convert_mcp_resources(resources), resource_templates: convert_mcp_resource_templates(resource_templates), - auth_statuses: mcp_connection_manager.auth_statuses(), - server_names: mcp_connection_manager.status_server_names(), + auth_statuses: auth_statuses_from_entries(&auth_status_entries), + server_names, } } diff --git a/codex-rs/core/src/codex_thread.rs b/codex-rs/core/src/codex_thread.rs index 75c7e25d73c0..ab1f6df03ed2 100644 --- a/codex-rs/core/src/codex_thread.rs +++ b/codex-rs/core/src/codex_thread.rs @@ -4,8 +4,6 @@ use crate::session::Codex; use crate::session::SessionSettingsUpdate; use crate::session::SteerInputError; use codex_features::Feature; -use codex_mcp::McpServerStatusSnapshot; -use codex_mcp::McpSnapshotDetail; use codex_otel::SessionTelemetry; use codex_protocol::ThreadId; use codex_protocol::config_types::ApprovalsReviewer; @@ -533,13 +531,6 @@ impl CodexThread { self.codex.session.get_config().await } - pub async fn mcp_server_status_snapshot( - &self, - detail: McpSnapshotDetail, - ) -> McpServerStatusSnapshot { - self.codex.session.mcp_server_status_snapshot(detail).await - } - pub fn multi_agent_version(&self) -> Option { self.codex.session.multi_agent_version() } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 296eb273c119..e114d49ecc6f 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -79,18 +79,6 @@ impl Session { Arc::new(GuardianMcpElicitationReviewer::new(self)) } - #[expect( - clippy::await_holding_invalid_type, - reason = "MCP inventory reads are serialized through the session-owned manager guard" - )] - pub(crate) async fn mcp_server_status_snapshot( - &self, - detail: McpSnapshotDetail, - ) -> McpServerStatusSnapshot { - let manager = self.services.mcp_connection_manager.read().await; - collect_mcp_server_status_snapshot(&manager, detail).await - } - #[expect( clippy::await_holding_invalid_type, reason = "active turn checks and turn state updates must remain atomic" diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 56c73e705ac7..3e71afee8a66 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -316,9 +316,6 @@ use crate::unified_exec::UnifiedExecProcessManager; use crate::windows_sandbox::WindowsSandboxLevelExt; use codex_core_plugins::PluginsManager; use codex_git_utils::get_git_repo_root; -use codex_mcp::McpServerStatusSnapshot; -use codex_mcp::McpSnapshotDetail; -use codex_mcp::collect_mcp_server_status_snapshot; use codex_mcp::compute_auth_statuses; use codex_mcp::effective_mcp_servers_from_configured; use codex_mcp::host_owned_codex_apps_enabled; From e5c5f0dca5345d8c3e611d4fdaeb2de8b6bdd08c Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:25:49 -0700 Subject: [PATCH 08/19] Include required MCP failures in status listing --- .../codex_app_server_protocol.schemas.json | 7 +++ .../codex_app_server_protocol.v2.schemas.json | 7 +++ .../json/v2/ListMcpServerStatusResponse.json | 7 +++ .../schema/typescript/v2/McpServerStatus.ts | 6 ++- .../src/protocol/v2/mcp.rs | 2 + .../src/protocol/v2/tests.rs | 4 ++ codex-rs/app-server/README.md | 2 +- .../src/request_processors/mcp_processor.rs | 5 +- .../tests/suite/v2/mcp_server_status.rs | 49 ++++++++++++++----- codex-rs/codex-mcp/src/connection_manager.rs | 48 ++++++++++++++---- codex-rs/codex-mcp/src/mcp/mod.rs | 25 ++++++++-- codex-rs/tui/src/app/background_requests.rs | 2 + codex-rs/tui/src/app/tests.rs | 1 + codex-rs/tui/src/history_cell/mcp.rs | 3 ++ ..._statuses_renders_status_only_servers.snap | 1 + codex-rs/tui/src/history_cell/tests.rs | 2 + 16 files changed, 145 insertions(+), 26 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index d65c043a7d0c..60e71164f052 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -11455,6 +11455,13 @@ } ] }, + "startupError": { + "description": "Startup error for a required server that failed to initialize.", + "type": [ + "string", + "null" + ] + }, "tools": { "additionalProperties": { "$ref": "#/definitions/v2/Tool" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 8271dc485d6e..a428962f73b7 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -7957,6 +7957,13 @@ } ] }, + "startupError": { + "description": "Startup error for a required server that failed to initialize.", + "type": [ + "string", + "null" + ] + }, "tools": { "additionalProperties": { "$ref": "#/definitions/Tool" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json index 0dc2f5e2e4b4..31705a5c89a3 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json @@ -81,6 +81,13 @@ } ] }, + "startupError": { + "description": "Startup error for a required server that failed to initialize.", + "type": [ + "string", + "null" + ] + }, "tools": { "additionalProperties": { "$ref": "#/definitions/Tool" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts b/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts index d2e99ce96fd5..6950acb2dffb 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts @@ -7,4 +7,8 @@ import type { ResourceTemplate } from "../ResourceTemplate"; import type { Tool } from "../Tool"; import type { McpAuthStatus } from "./McpAuthStatus"; -export type McpServerStatus = { name: string, serverInfo: McpServerInfo | null, tools: { [key in string]?: Tool }, resources: Array, resourceTemplates: Array, authStatus: McpAuthStatus, }; +export type McpServerStatus = { name: string, serverInfo: McpServerInfo | null, tools: { [key in string]?: Tool }, resources: Array, resourceTemplates: Array, authStatus: McpAuthStatus, +/** + * Startup error for a required server that failed to initialize. + */ +startupError: string | null, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs b/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs index 37197a223768..fa7bdcbb283b 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs @@ -59,6 +59,8 @@ pub struct McpServerStatus { pub resources: Vec, pub resource_templates: Vec, pub auth_status: McpAuthStatus, + /// Startup error for a required server that failed to initialize. + pub startup_error: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index c570b87ce4a6..8ec5de9ac8e5 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -2044,6 +2044,7 @@ fn mcp_server_status_serializes_absent_server_info_as_null() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: McpAuthStatus::Unsupported, + startup_error: None, }], next_cursor: None, }; @@ -2058,6 +2059,7 @@ fn mcp_server_status_serializes_absent_server_info_as_null() { "resources": [], "resourceTemplates": [], "authStatus": "unsupported", + "startupError": null, }], "nextCursor": null, }) @@ -2108,6 +2110,7 @@ fn mcp_server_status_serializes_absent_server_info_metadata_as_null() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: McpAuthStatus::Unsupported, + startup_error: None, }], next_cursor: None, }; @@ -2129,6 +2132,7 @@ fn mcp_server_status_serializes_absent_server_info_metadata_as_null() { "resources": [], "resourceTemplates": [], "authStatus": "unsupported", + "startupError": null, }], "nextCursor": null, }) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index e09866e1968e..9bea75cf4580 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -221,7 +221,7 @@ Example with notification opt-out: - `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes. - `tool/requestUserInput` — prompt the user with 1–3 short questions for a tool call and return their answers (experimental). - `config/mcpServer/reload` — reload MCP server config from disk and queue a refresh for loaded threads (applied on each thread's next active turn); returns `{}`. Use this after editing `config.toml` without restarting the server. -- `mcpServerStatus/list` — enumerate configured MCP servers with their tools, auth status, server info, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`. +- `mcpServerStatus/list` — enumerate configured MCP servers with their tools, auth status, server info, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`. Each row's nullable `startupError` reports a required server that failed to initialize without failing the whole list request. - `mcpServer/resource/read` — read a resource from a configured MCP server by optional `threadId`, `server`, and `uri`, returning text/blob resource `contents`. If `threadId` is omitted, the server reads from the latest MCP config directly. - `mcpServer/tool/call` — call a tool on a thread's configured MCP server by `threadId`, `server`, `tool`, optional `arguments`, and optional `_meta`, returning the MCP tool result. - `windowsSandbox/setupStart` — start Windows sandbox setup for the selected mode (`elevated` or `unelevated`); accepts an optional absolute `cwd` to target setup for a specific workspace, returns `{ started: true }` immediately, and later emits `windowsSandbox/setupCompleted`. 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 36832fc56f7c..b5cc88247385 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -289,13 +289,15 @@ impl McpRequestProcessor { resource_templates, auth_statuses, mut server_names, + startup_failures, } = snapshot; server_names.extend( auth_statuses .keys() .cloned() .chain(resources.keys().cloned()) - .chain(resource_templates.keys().cloned()), + .chain(resource_templates.keys().cloned()) + .chain(startup_failures.keys().cloned()), ); server_names.sort(); server_names.dedup(); @@ -332,6 +334,7 @@ impl McpRequestProcessor { .cloned() .unwrap_or(CoreMcpAuthStatus::Unsupported) .into(), + startup_error: startup_failures.get(name).cloned(), }) .collect(); diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 43335dedd9d2..ea5ba3df3630 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -118,6 +118,7 @@ url = "{mcp_server_url}/mcp" #[tokio::test] async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; + let (mcp_server_url, mcp_server_handle) = start_mcp_server("healthy_lookup").await?; let codex_home = TempDir::new()?; write_mock_responses_config_toml( codex_home.path(), @@ -131,13 +132,16 @@ async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running let config_path = codex_home.path().join("config.toml"); let mut config_toml = std::fs::read_to_string(&config_path)?; - config_toml.push_str( + config_toml.push_str(&format!( r#" +[mcp_servers.healthy] +url = "{mcp_server_url}/mcp" + [mcp_servers.required_broken] command = "codex-definitely-not-a-real-binary" required = true "#, - ); + )); std::fs::write(config_path, config_toml)?; let mut mcp = TestAppServer::new(codex_home.path()).await?; @@ -151,21 +155,41 @@ required = true thread_id: None, }) .await?; - let error = timeout( + let response = timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), ) .await??; + let response: ListMcpServerStatusResponse = to_response(response)?; + let statuses = response + .data + .iter() + .map(|status| (status.name.as_str(), status)) + .collect::>(); + let healthy = statuses.get("healthy").copied(); + let required_broken = statuses.get("required_broken").copied(); assert_eq!( ( - error.error.code, - error - .error - .message - .contains("required MCP servers failed to initialize: required_broken"), - error.error.data, + response.next_cursor.as_deref(), + statuses.keys().copied().collect::>(), + healthy.map(|status| status.tools.keys().cloned().collect::>()), + healthy.and_then(|status| status.startup_error.as_deref()), + required_broken.map(|status| ( + status.server_info.as_ref(), + status.tools.is_empty(), + status + .startup_error + .as_deref() + .is_some_and(|error| !error.is_empty()), + )), + ), + ( + None, + vec!["healthy", "required_broken"], + Some(BTreeSet::from(["healthy_lookup".to_string()])), + None, + Some((None, true, true)), ), - (-32603, true, None), ); let follow_up_request_id = mcp @@ -177,6 +201,9 @@ required = true ) .await??; + mcp_server_handle.abort(); + let _ = mcp_server_handle.await; + Ok(()) } diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 07f59d20911e..585b48345bbd 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -7,6 +7,7 @@ //! `codex-core`. use std::collections::HashMap; +use std::fmt; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::Ordering; @@ -113,6 +114,43 @@ pub struct McpConnectionManager { startup_cancellation_token: CancellationToken, } +pub(crate) struct McpConnectionManagerStartupError { + manager: McpConnectionManager, + failures: Vec, +} + +impl McpConnectionManagerStartupError { + pub(crate) fn into_parts(self) -> (McpConnectionManager, Vec) { + (self.manager, self.failures) + } +} + +impl fmt::Debug for McpConnectionManagerStartupError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("McpConnectionManagerStartupError") + .field("failures", &self.failures) + .finish_non_exhaustive() + } +} + +impl fmt::Display for McpConnectionManagerStartupError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + let details = self + .failures + .iter() + .map(|failure| format!("{}: {}", failure.server, failure.error)) + .collect::>() + .join("; "); + write!( + formatter, + "required MCP servers failed to initialize: {details}" + ) + } +} + +impl std::error::Error for McpConnectionManagerStartupError {} + impl McpConnectionManager { #[allow(clippy::new_ret_no_self, clippy::too_many_arguments)] pub async fn new( @@ -281,15 +319,7 @@ impl McpConnectionManager { )) .await; if !failures.is_empty() { - startup_cancellation_token.cancel(); - let details = failures - .iter() - .map(|failure| format!("{}: {}", failure.server, failure.error)) - .collect::>() - .join("; "); - return Err(anyhow!( - "required MCP servers failed to initialize: {details}" - )); + return Err(McpConnectionManagerStartupError { manager, failures }.into()); } Ok(manager) } diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 253f7ba405ee..455798ecac04 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -39,6 +39,7 @@ use tokio_util::sync::CancellationToken; use crate::codex_apps::codex_apps_tools_cache_key; use crate::connection_manager::McpConnectionManager; +use crate::connection_manager::McpConnectionManagerStartupError; use crate::runtime::McpRuntimeContext; use crate::server::EffectiveMcpServer; @@ -331,6 +332,7 @@ pub struct McpServerStatusSnapshot { pub resource_templates: HashMap>, pub auth_statuses: HashMap, pub server_names: Vec, + pub startup_failures: HashMap, } pub async fn collect_mcp_server_status_snapshot_with_detail( @@ -351,6 +353,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( resource_templates: HashMap::new(), auth_statuses: HashMap::new(), server_names: Vec::new(), + startup_failures: HashMap::new(), }); } @@ -367,7 +370,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( drop(rx_event); let cancel_token = CancellationToken::new(); - let mcp_connection_manager = McpConnectionManager::new( + let manager_result = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, auth_status_entries.clone(), @@ -386,15 +389,30 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( auth, /*elicitation_reviewer*/ None, ) - .await?; + .await; + let (mcp_connection_manager, startup_failures) = match manager_result { + Ok(manager) => (manager, HashMap::new()), + Err(err) => match err.downcast::() { + Ok(err) => { + let (manager, failures) = err.into_parts(); + let failures = failures + .into_iter() + .map(|failure| (failure.server, failure.error)) + .collect(); + (manager, failures) + } + Err(err) => return Err(err), + }, + }; - let snapshot = collect_mcp_server_status_snapshot_from_manager( + let mut snapshot = collect_mcp_server_status_snapshot_from_manager( &mcp_connection_manager, auth_status_entries, server_names, detail, ) .await; + snapshot.startup_failures = startup_failures; cancel_token.cancel(); @@ -639,6 +657,7 @@ async fn collect_mcp_server_status_snapshot_from_manager( resource_templates: convert_mcp_resource_templates(resource_templates), auth_statuses: auth_statuses_from_entries(&auth_status_entries), server_names, + startup_failures: HashMap::new(), } } diff --git a/codex-rs/tui/src/app/background_requests.rs b/codex-rs/tui/src/app/background_requests.rs index 456676f7963e..5cb8b0ef06b0 100644 --- a/codex-rs/tui/src/app/background_requests.rs +++ b/codex-rs/tui/src/app/background_requests.rs @@ -1084,6 +1084,7 @@ mod tests { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, + startup_error: None, }, McpServerStatus { name: "disabled".to_string(), @@ -1092,6 +1093,7 @@ mod tests { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, + startup_error: None, }, ]; diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index ed89c7287801..96e5fb83bec1 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -153,6 +153,7 @@ async fn handle_mcp_inventory_result_respects_origin_thread() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, + startup_error: None, }]), McpServerStatusDetail::ToolsAndAuthOnly, /*thread_id*/ None, diff --git a/codex-rs/tui/src/history_cell/mcp.rs b/codex-rs/tui/src/history_cell/mcp.rs index 599a44f092d8..df763b610d33 100644 --- a/codex-rs/tui/src/history_cell/mcp.rs +++ b/codex-rs/tui/src/history_cell/mcp.rs @@ -545,6 +545,9 @@ pub(crate) fn new_mcp_tools_output_from_statuses( let header: Vec> = vec![" • ".into(), status.name.clone().into()]; lines.push(header.into()); + if let Some(error) = &status.startup_error { + lines.push(vec![" • Error: ".into(), error.clone().red()].into()); + } let auth_status = match status.auth_status { codex_app_server_protocol::McpAuthStatus::Unsupported => McpAuthStatus::Unsupported, codex_app_server_protocol::McpAuthStatus::NotLoggedIn => McpAuthStatus::NotLoggedIn, diff --git a/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap b/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap index 52bd34c3ac3a..0b230c7767a1 100644 --- a/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap +++ b/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap @@ -7,5 +7,6 @@ expression: rendered 🔌 MCP Tools • plugin_docs + • Error: required MCP server failed • Auth: Unsupported • Tools: lookup diff --git a/codex-rs/tui/src/history_cell/tests.rs b/codex-rs/tui/src/history_cell/tests.rs index ec3ae49f2159..929301b88e2c 100644 --- a/codex-rs/tui/src/history_cell/tests.rs +++ b/codex-rs/tui/src/history_cell/tests.rs @@ -878,6 +878,7 @@ fn mcp_tools_output_from_statuses_renders_status_only_servers() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, + startup_error: Some("required MCP server failed".to_string()), }]; let cell = @@ -925,6 +926,7 @@ fn mcp_tools_output_from_statuses_renders_verbose_inventory() { mime_type: None, }], auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, + startup_error: None, }]; let cell = new_mcp_tools_output_from_statuses(&statuses, McpServerStatusDetail::Full); From 6385c6b3b2620c233e771995c328a62a4ef5ceb6 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:27:12 -0700 Subject: [PATCH 09/19] Revert "Include required MCP failures in status listing" This reverts commit e5c5f0dca5345d8c3e611d4fdaeb2de8b6bdd08c. --- .../codex_app_server_protocol.schemas.json | 7 --- .../codex_app_server_protocol.v2.schemas.json | 7 --- .../json/v2/ListMcpServerStatusResponse.json | 7 --- .../schema/typescript/v2/McpServerStatus.ts | 6 +-- .../src/protocol/v2/mcp.rs | 2 - .../src/protocol/v2/tests.rs | 4 -- codex-rs/app-server/README.md | 2 +- .../src/request_processors/mcp_processor.rs | 5 +- .../tests/suite/v2/mcp_server_status.rs | 49 +++++-------------- codex-rs/codex-mcp/src/connection_manager.rs | 48 ++++-------------- codex-rs/codex-mcp/src/mcp/mod.rs | 25 ++-------- codex-rs/tui/src/app/background_requests.rs | 2 - codex-rs/tui/src/app/tests.rs | 1 - codex-rs/tui/src/history_cell/mcp.rs | 3 -- ..._statuses_renders_status_only_servers.snap | 1 - codex-rs/tui/src/history_cell/tests.rs | 2 - 16 files changed, 26 insertions(+), 145 deletions(-) diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 60e71164f052..d65c043a7d0c 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -11455,13 +11455,6 @@ } ] }, - "startupError": { - "description": "Startup error for a required server that failed to initialize.", - "type": [ - "string", - "null" - ] - }, "tools": { "additionalProperties": { "$ref": "#/definitions/v2/Tool" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index a428962f73b7..8271dc485d6e 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -7957,13 +7957,6 @@ } ] }, - "startupError": { - "description": "Startup error for a required server that failed to initialize.", - "type": [ - "string", - "null" - ] - }, "tools": { "additionalProperties": { "$ref": "#/definitions/Tool" diff --git a/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json b/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json index 31705a5c89a3..0dc2f5e2e4b4 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/ListMcpServerStatusResponse.json @@ -81,13 +81,6 @@ } ] }, - "startupError": { - "description": "Startup error for a required server that failed to initialize.", - "type": [ - "string", - "null" - ] - }, "tools": { "additionalProperties": { "$ref": "#/definitions/Tool" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts b/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts index 6950acb2dffb..d2e99ce96fd5 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/McpServerStatus.ts @@ -7,8 +7,4 @@ import type { ResourceTemplate } from "../ResourceTemplate"; import type { Tool } from "../Tool"; import type { McpAuthStatus } from "./McpAuthStatus"; -export type McpServerStatus = { name: string, serverInfo: McpServerInfo | null, tools: { [key in string]?: Tool }, resources: Array, resourceTemplates: Array, authStatus: McpAuthStatus, -/** - * Startup error for a required server that failed to initialize. - */ -startupError: string | null, }; +export type McpServerStatus = { name: string, serverInfo: McpServerInfo | null, tools: { [key in string]?: Tool }, resources: Array, resourceTemplates: Array, authStatus: McpAuthStatus, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs b/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs index fa7bdcbb283b..37197a223768 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/mcp.rs @@ -59,8 +59,6 @@ pub struct McpServerStatus { pub resources: Vec, pub resource_templates: Vec, pub auth_status: McpAuthStatus, - /// Startup error for a required server that failed to initialize. - pub startup_error: Option, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] diff --git a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs index 8ec5de9ac8e5..c570b87ce4a6 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/tests.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/tests.rs @@ -2044,7 +2044,6 @@ fn mcp_server_status_serializes_absent_server_info_as_null() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: McpAuthStatus::Unsupported, - startup_error: None, }], next_cursor: None, }; @@ -2059,7 +2058,6 @@ fn mcp_server_status_serializes_absent_server_info_as_null() { "resources": [], "resourceTemplates": [], "authStatus": "unsupported", - "startupError": null, }], "nextCursor": null, }) @@ -2110,7 +2108,6 @@ fn mcp_server_status_serializes_absent_server_info_metadata_as_null() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: McpAuthStatus::Unsupported, - startup_error: None, }], next_cursor: None, }; @@ -2132,7 +2129,6 @@ fn mcp_server_status_serializes_absent_server_info_metadata_as_null() { "resources": [], "resourceTemplates": [], "authStatus": "unsupported", - "startupError": null, }], "nextCursor": null, }) diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 9bea75cf4580..e09866e1968e 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -221,7 +221,7 @@ Example with notification opt-out: - `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes. - `tool/requestUserInput` — prompt the user with 1–3 short questions for a tool call and return their answers (experimental). - `config/mcpServer/reload` — reload MCP server config from disk and queue a refresh for loaded threads (applied on each thread's next active turn); returns `{}`. Use this after editing `config.toml` without restarting the server. -- `mcpServerStatus/list` — enumerate configured MCP servers with their tools, auth status, server info, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`. Each row's nullable `startupError` reports a required server that failed to initialize without failing the whole list request. +- `mcpServerStatus/list` — enumerate configured MCP servers with their tools, auth status, server info, plus resources/resource templates for `full` detail; supports optional `threadId` and cursor+limit pagination. If `threadId` is omitted, the server reads from the latest global config directly. If `detail` is omitted, the server defaults to `full`. - `mcpServer/resource/read` — read a resource from a configured MCP server by optional `threadId`, `server`, and `uri`, returning text/blob resource `contents`. If `threadId` is omitted, the server reads from the latest MCP config directly. - `mcpServer/tool/call` — call a tool on a thread's configured MCP server by `threadId`, `server`, `tool`, optional `arguments`, and optional `_meta`, returning the MCP tool result. - `windowsSandbox/setupStart` — start Windows sandbox setup for the selected mode (`elevated` or `unelevated`); accepts an optional absolute `cwd` to target setup for a specific workspace, returns `{ started: true }` immediately, and later emits `windowsSandbox/setupCompleted`. 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 b5cc88247385..36832fc56f7c 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -289,15 +289,13 @@ impl McpRequestProcessor { resource_templates, auth_statuses, mut server_names, - startup_failures, } = snapshot; server_names.extend( auth_statuses .keys() .cloned() .chain(resources.keys().cloned()) - .chain(resource_templates.keys().cloned()) - .chain(startup_failures.keys().cloned()), + .chain(resource_templates.keys().cloned()), ); server_names.sort(); server_names.dedup(); @@ -334,7 +332,6 @@ impl McpRequestProcessor { .cloned() .unwrap_or(CoreMcpAuthStatus::Unsupported) .into(), - startup_error: startup_failures.get(name).cloned(), }) .collect(); diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index ea5ba3df3630..43335dedd9d2 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -118,7 +118,6 @@ url = "{mcp_server_url}/mcp" #[tokio::test] async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; - let (mcp_server_url, mcp_server_handle) = start_mcp_server("healthy_lookup").await?; let codex_home = TempDir::new()?; write_mock_responses_config_toml( codex_home.path(), @@ -132,16 +131,13 @@ async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running let config_path = codex_home.path().join("config.toml"); let mut config_toml = std::fs::read_to_string(&config_path)?; - config_toml.push_str(&format!( + config_toml.push_str( r#" -[mcp_servers.healthy] -url = "{mcp_server_url}/mcp" - [mcp_servers.required_broken] command = "codex-definitely-not-a-real-binary" required = true "#, - )); + ); std::fs::write(config_path, config_toml)?; let mut mcp = TestAppServer::new(codex_home.path()).await?; @@ -155,41 +151,21 @@ required = true thread_id: None, }) .await?; - let response = timeout( + let error = timeout( DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), ) .await??; - let response: ListMcpServerStatusResponse = to_response(response)?; - let statuses = response - .data - .iter() - .map(|status| (status.name.as_str(), status)) - .collect::>(); - let healthy = statuses.get("healthy").copied(); - let required_broken = statuses.get("required_broken").copied(); assert_eq!( ( - response.next_cursor.as_deref(), - statuses.keys().copied().collect::>(), - healthy.map(|status| status.tools.keys().cloned().collect::>()), - healthy.and_then(|status| status.startup_error.as_deref()), - required_broken.map(|status| ( - status.server_info.as_ref(), - status.tools.is_empty(), - status - .startup_error - .as_deref() - .is_some_and(|error| !error.is_empty()), - )), - ), - ( - None, - vec!["healthy", "required_broken"], - Some(BTreeSet::from(["healthy_lookup".to_string()])), - None, - Some((None, true, true)), + error.error.code, + error + .error + .message + .contains("required MCP servers failed to initialize: required_broken"), + error.error.data, ), + (-32603, true, None), ); let follow_up_request_id = mcp @@ -201,9 +177,6 @@ required = true ) .await??; - mcp_server_handle.abort(); - let _ = mcp_server_handle.await; - Ok(()) } diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 585b48345bbd..07f59d20911e 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -7,7 +7,6 @@ //! `codex-core`. use std::collections::HashMap; -use std::fmt; use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::Ordering; @@ -114,43 +113,6 @@ pub struct McpConnectionManager { startup_cancellation_token: CancellationToken, } -pub(crate) struct McpConnectionManagerStartupError { - manager: McpConnectionManager, - failures: Vec, -} - -impl McpConnectionManagerStartupError { - pub(crate) fn into_parts(self) -> (McpConnectionManager, Vec) { - (self.manager, self.failures) - } -} - -impl fmt::Debug for McpConnectionManagerStartupError { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - formatter - .debug_struct("McpConnectionManagerStartupError") - .field("failures", &self.failures) - .finish_non_exhaustive() - } -} - -impl fmt::Display for McpConnectionManagerStartupError { - fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - let details = self - .failures - .iter() - .map(|failure| format!("{}: {}", failure.server, failure.error)) - .collect::>() - .join("; "); - write!( - formatter, - "required MCP servers failed to initialize: {details}" - ) - } -} - -impl std::error::Error for McpConnectionManagerStartupError {} - impl McpConnectionManager { #[allow(clippy::new_ret_no_self, clippy::too_many_arguments)] pub async fn new( @@ -319,7 +281,15 @@ impl McpConnectionManager { )) .await; if !failures.is_empty() { - return Err(McpConnectionManagerStartupError { manager, failures }.into()); + startup_cancellation_token.cancel(); + let details = failures + .iter() + .map(|failure| format!("{}: {}", failure.server, failure.error)) + .collect::>() + .join("; "); + return Err(anyhow!( + "required MCP servers failed to initialize: {details}" + )); } Ok(manager) } diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 455798ecac04..253f7ba405ee 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -39,7 +39,6 @@ use tokio_util::sync::CancellationToken; use crate::codex_apps::codex_apps_tools_cache_key; use crate::connection_manager::McpConnectionManager; -use crate::connection_manager::McpConnectionManagerStartupError; use crate::runtime::McpRuntimeContext; use crate::server::EffectiveMcpServer; @@ -332,7 +331,6 @@ pub struct McpServerStatusSnapshot { pub resource_templates: HashMap>, pub auth_statuses: HashMap, pub server_names: Vec, - pub startup_failures: HashMap, } pub async fn collect_mcp_server_status_snapshot_with_detail( @@ -353,7 +351,6 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( resource_templates: HashMap::new(), auth_statuses: HashMap::new(), server_names: Vec::new(), - startup_failures: HashMap::new(), }); } @@ -370,7 +367,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( drop(rx_event); let cancel_token = CancellationToken::new(); - let manager_result = McpConnectionManager::new( + let mcp_connection_manager = McpConnectionManager::new( &mcp_servers, config.mcp_oauth_credentials_store_mode, auth_status_entries.clone(), @@ -389,30 +386,15 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( auth, /*elicitation_reviewer*/ None, ) - .await; - let (mcp_connection_manager, startup_failures) = match manager_result { - Ok(manager) => (manager, HashMap::new()), - Err(err) => match err.downcast::() { - Ok(err) => { - let (manager, failures) = err.into_parts(); - let failures = failures - .into_iter() - .map(|failure| (failure.server, failure.error)) - .collect(); - (manager, failures) - } - Err(err) => return Err(err), - }, - }; + .await?; - let mut snapshot = collect_mcp_server_status_snapshot_from_manager( + let snapshot = collect_mcp_server_status_snapshot_from_manager( &mcp_connection_manager, auth_status_entries, server_names, detail, ) .await; - snapshot.startup_failures = startup_failures; cancel_token.cancel(); @@ -657,7 +639,6 @@ async fn collect_mcp_server_status_snapshot_from_manager( resource_templates: convert_mcp_resource_templates(resource_templates), auth_statuses: auth_statuses_from_entries(&auth_status_entries), server_names, - startup_failures: HashMap::new(), } } diff --git a/codex-rs/tui/src/app/background_requests.rs b/codex-rs/tui/src/app/background_requests.rs index 5cb8b0ef06b0..456676f7963e 100644 --- a/codex-rs/tui/src/app/background_requests.rs +++ b/codex-rs/tui/src/app/background_requests.rs @@ -1084,7 +1084,6 @@ mod tests { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, - startup_error: None, }, McpServerStatus { name: "disabled".to_string(), @@ -1093,7 +1092,6 @@ mod tests { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, - startup_error: None, }, ]; diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 96e5fb83bec1..ed89c7287801 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -153,7 +153,6 @@ async fn handle_mcp_inventory_result_respects_origin_thread() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, - startup_error: None, }]), McpServerStatusDetail::ToolsAndAuthOnly, /*thread_id*/ None, diff --git a/codex-rs/tui/src/history_cell/mcp.rs b/codex-rs/tui/src/history_cell/mcp.rs index df763b610d33..599a44f092d8 100644 --- a/codex-rs/tui/src/history_cell/mcp.rs +++ b/codex-rs/tui/src/history_cell/mcp.rs @@ -545,9 +545,6 @@ pub(crate) fn new_mcp_tools_output_from_statuses( let header: Vec> = vec![" • ".into(), status.name.clone().into()]; lines.push(header.into()); - if let Some(error) = &status.startup_error { - lines.push(vec![" • Error: ".into(), error.clone().red()].into()); - } let auth_status = match status.auth_status { codex_app_server_protocol::McpAuthStatus::Unsupported => McpAuthStatus::Unsupported, codex_app_server_protocol::McpAuthStatus::NotLoggedIn => McpAuthStatus::NotLoggedIn, diff --git a/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap b/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap index 0b230c7767a1..52bd34c3ac3a 100644 --- a/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap +++ b/codex-rs/tui/src/history_cell/snapshots/codex_tui__history_cell__tests__mcp_tools_output_from_statuses_renders_status_only_servers.snap @@ -7,6 +7,5 @@ expression: rendered 🔌 MCP Tools • plugin_docs - • Error: required MCP server failed • Auth: Unsupported • Tools: lookup diff --git a/codex-rs/tui/src/history_cell/tests.rs b/codex-rs/tui/src/history_cell/tests.rs index 929301b88e2c..ec3ae49f2159 100644 --- a/codex-rs/tui/src/history_cell/tests.rs +++ b/codex-rs/tui/src/history_cell/tests.rs @@ -878,7 +878,6 @@ fn mcp_tools_output_from_statuses_renders_status_only_servers() { resources: Vec::new(), resource_templates: Vec::new(), auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, - startup_error: Some("required MCP server failed".to_string()), }]; let cell = @@ -926,7 +925,6 @@ fn mcp_tools_output_from_statuses_renders_verbose_inventory() { mime_type: None, }], auth_status: codex_app_server_protocol::McpAuthStatus::Unsupported, - startup_error: None, }]; let cell = new_mcp_tools_output_from_statuses(&statuses, McpServerStatusDetail::Full); From dfca54a28e98ef7df5407e66ef45ec53bf93eff6 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:39:48 -0700 Subject: [PATCH 10/19] Validate required MCP servers at session boundary --- .../src/request_processors/mcp_processor.rs | 3 +- .../tests/suite/v2/mcp_server_status.rs | 90 +++++-------------- codex-rs/codex-mcp/src/connection_manager.rs | 43 +++++---- .../codex-mcp/src/connection_manager_tests.rs | 5 +- codex-rs/codex-mcp/src/mcp/mod.rs | 12 +-- codex-rs/core/src/connectors.rs | 2 +- codex-rs/core/src/mcp_tool_call_tests.rs | 3 +- codex-rs/core/src/session/mcp.rs | 1 + codex-rs/core/src/session/session.rs | 3 +- 9 files changed, 64 insertions(+), 98 deletions(-) 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 36832fc56f7c..facd4facc63f 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -279,8 +279,7 @@ impl McpRequestProcessor { runtime_context, detail, ) - .await - .map_err(|err| internal_error(format!("failed to collect MCP server status: {err:#}")))?; + .await; let McpServerStatusSnapshot { server_infos, diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 43335dedd9d2..4b9db3c8ef5d 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -10,7 +10,6 @@ use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; use app_test_support::write_mock_responses_config_toml; use axum::Router; -use codex_app_server_protocol::ExperimentalFeatureListParams; use codex_app_server_protocol::ListMcpServerStatusParams; use codex_app_server_protocol::ListMcpServerStatusResponse; use codex_app_server_protocol::McpServerStatusDetail; @@ -44,7 +43,7 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10); #[tokio::test] -async fn mcp_server_status_list_returns_raw_server_and_tool_names() -> Result<()> { +async fn mcp_server_status_list_includes_healthy_and_failed_required_servers() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let (mcp_server_url, mcp_server_handle) = start_mcp_server("look-up.raw").await?; let codex_home = TempDir::new()?; @@ -64,6 +63,10 @@ async fn mcp_server_status_list_returns_raw_server_and_tool_names() -> Result<() r#" [mcp_servers.some-server] url = "{mcp_server_url}/mcp" + +[mcp_servers.required_broken] +command = "codex-definitely-not-a-real-binary" +required = true "# )); std::fs::write(config_path, config_toml)?; @@ -87,8 +90,12 @@ url = "{mcp_server_url}/mcp" let response: ListMcpServerStatusResponse = to_response(response)?; assert_eq!(response.next_cursor, None); - assert_eq!(response.data.len(), 1); - let status = &response.data[0]; + assert_eq!(response.data.len(), 2); + let status = response + .data + .iter() + .find(|status| status.name == "some-server") + .expect("healthy MCP server status"); assert_eq!(status.name, "some-server"); assert_eq!( status.tools.keys().cloned().collect::>(), @@ -108,74 +115,23 @@ url = "{mcp_server_url}/mcp" .and_then(|info| info.title.as_deref()), Some("Lookup Server") ); - - mcp_server_handle.abort(); - let _ = mcp_server_handle.await; - - Ok(()) -} - -#[tokio::test] -async fn mcp_server_status_list_returns_startup_failure_and_keeps_server_running() -> Result<()> { - let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; - let codex_home = TempDir::new()?; - write_mock_responses_config_toml( - codex_home.path(), - &server.uri(), - &BTreeMap::new(), - /*auto_compact_limit*/ 1024, - /*requires_openai_auth*/ None, - "mock_provider", - "compact", - )?; - - let config_path = codex_home.path().join("config.toml"); - let mut config_toml = std::fs::read_to_string(&config_path)?; - config_toml.push_str( - r#" -[mcp_servers.required_broken] -command = "codex-definitely-not-a-real-binary" -required = true -"#, - ); - std::fs::write(config_path, config_toml)?; - - let mut mcp = TestAppServer::new(codex_home.path()).await?; - timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; - - let request_id = mcp - .send_list_mcp_server_status_request(ListMcpServerStatusParams { - cursor: None, - limit: None, - detail: None, - thread_id: None, - }) - .await?; - let error = timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_error_message(RequestId::Integer(request_id)), - ) - .await??; + let required_broken = response + .data + .iter() + .find(|status| status.name == "required_broken") + .expect("failed required MCP server status"); assert_eq!( ( - error.error.code, - error - .error - .message - .contains("required MCP servers failed to initialize: required_broken"), - error.error.data, + required_broken.server_info.as_ref(), + required_broken.tools.is_empty(), + required_broken.resources.is_empty(), + required_broken.resource_templates.is_empty(), ), - (-32603, true, None), + (None, true, true, true), ); - let follow_up_request_id = mcp - .send_experimental_feature_list_request(ExperimentalFeatureListParams::default()) - .await?; - timeout( - DEFAULT_READ_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(follow_up_request_id)), - ) - .await??; + mcp_server_handle.abort(); + let _ = mcp_server_handle.await; Ok(()) } diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 07f59d20911e..3a83d878a68a 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -106,6 +106,7 @@ pub fn tool_is_model_visible(tool: &ToolInfo) -> bool { pub struct McpConnectionManager { clients: HashMap, server_metadata: HashMap, + required_startup_failures: Vec, tool_plugin_provenance: Arc, host_owned_codex_apps_enabled: bool, prefix_mcp_tool_names: bool, @@ -114,7 +115,7 @@ pub struct McpConnectionManager { } impl McpConnectionManager { - #[allow(clippy::new_ret_no_self, clippy::too_many_arguments)] + #[allow(clippy::too_many_arguments)] pub async fn new( mcp_servers: &HashMap, store_mode: OAuthCredentialsStoreMode, @@ -133,7 +134,7 @@ impl McpConnectionManager { tool_plugin_provenance: ToolPluginProvenance, auth: Option<&CodexAuth>, elicitation_reviewer: Option, - ) -> Result { + ) -> Self { let mut required_servers = mcp_servers .iter() .filter(|(_, server)| server.enabled() && server.required()) @@ -241,9 +242,10 @@ impl McpConnectionManager { (server_name, outcome) }); } - let manager = Self { + let mut manager = Self { clients, server_metadata, + required_startup_failures: Vec::new(), tool_plugin_provenance, host_owned_codex_apps_enabled, prefix_mcp_tool_names, @@ -272,26 +274,32 @@ impl McpConnectionManager { }) .await; }); - let failures = manager - .required_startup_failures(&required_servers) + manager.required_startup_failures = manager + .collect_required_startup_failures(&required_servers) .instrument(info_span!( "session_init.required_mcp_wait", otel.name = "session_init.required_mcp_wait", session_init.required_mcp_server_count = required_servers.len(), )) .await; - if !failures.is_empty() { - startup_cancellation_token.cancel(); - let details = failures - .iter() - .map(|failure| format!("{}: {}", failure.server, failure.error)) - .collect::>() - .join("; "); - return Err(anyhow!( - "required MCP servers failed to initialize: {details}" - )); + manager + } + + /// Returns this manager only if every required server initialized successfully. + pub fn validate_required_servers(self) -> Result { + if self.required_startup_failures.is_empty() { + return Ok(self); } - Ok(manager) + + let details = self + .required_startup_failures + .iter() + .map(|failure| format!("{}: {}", failure.server, failure.error)) + .collect::>() + .join("; "); + Err(anyhow!( + "required MCP servers failed to initialize: {details}" + )) } pub fn new_uninitialized_with_permission_profile( @@ -302,6 +310,7 @@ impl McpConnectionManager { Self { clients: HashMap::new(), server_metadata: HashMap::new(), + required_startup_failures: Vec::new(), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), host_owned_codex_apps_enabled: false, prefix_mcp_tool_names, @@ -776,7 +785,7 @@ impl McpConnectionManager { .context("failed to get client") } - async fn required_startup_failures( + async fn collect_required_startup_failures( &self, required_servers: &[String], ) -> Vec { diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index d75d9f7a3ad7..f5d6be840159 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1108,7 +1108,7 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { } #[tokio::test] -async fn required_local_stdio_without_local_runtime_fails_manager_construction() { +async fn required_local_stdio_without_local_runtime_fails_validation() { let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let (tx_event, rx_event) = async_channel::unbounded(); drop(rx_event); @@ -1166,7 +1166,8 @@ async fn required_local_stdio_without_local_runtime_fails_manager_construction() /*auth*/ None, /*elicitation_reviewer*/ None, ) - .await; + .await + .validate_required_servers(); let error = match result { Ok(_) => panic!("required MCP startup should fail"), diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 253f7ba405ee..adc2cf615e01 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -314,7 +314,7 @@ pub async fn read_mcp_resource( auth, /*elicitation_reviewer*/ None, ) - .await?; + .await; let result = manager .read_resource(server, ReadResourceRequestParams::new(uri)) @@ -339,19 +339,19 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( submit_id: String, runtime_context: McpRuntimeContext, detail: McpSnapshotDetail, -) -> anyhow::Result { +) -> McpServerStatusSnapshot { let mcp_servers = effective_mcp_servers(config, auth); let host_owned_codex_apps_enabled = host_owned_codex_apps_enabled(config, auth); let tool_plugin_provenance = tool_plugin_provenance(config); if mcp_servers.is_empty() { - return Ok(McpServerStatusSnapshot { + return McpServerStatusSnapshot { server_infos: HashMap::new(), tools_by_server: HashMap::new(), resources: HashMap::new(), resource_templates: HashMap::new(), auth_statuses: HashMap::new(), server_names: Vec::new(), - }); + }; } let auth_status_entries = compute_auth_statuses( @@ -386,7 +386,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( auth, /*elicitation_reviewer*/ None, ) - .await?; + .await; let snapshot = collect_mcp_server_status_snapshot_from_manager( &mcp_connection_manager, @@ -398,7 +398,7 @@ pub async fn collect_mcp_server_status_snapshot_with_detail( cancel_token.cancel(); - Ok(snapshot) + snapshot } /// The Responses API requires tool names to match `^[a-zA-Z0-9_-]+$`. diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index e058cfc8cbe3..6d6ca52d8cb6 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -308,7 +308,7 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_mcp_manager( auth.as_ref(), /*elicitation_reviewer*/ None, ) - .await?; + .await; let refreshed_tools = if force_refetch { match mcp_connection_manager diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 141f72564a0e..da54fe187371 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1279,8 +1279,7 @@ async fn install_host_owned_codex_apps_manager(session: &Session, turn_context: auth.as_ref(), /*elicitation_reviewer*/ None, ) - .await - .expect("construct MCP connection manager"); + .await; *session.services.mcp_connection_manager.write().await = manager; } diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index e114d49ecc6f..d18fdfc4c9fd 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -360,6 +360,7 @@ impl Session { elicitation_reviewer, ) .await + .validate_required_servers() { Ok(manager) => manager, Err(err) => { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 5cb9923d4d56..a9f1fc2d2d73 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1180,7 +1180,8 @@ impl Session { "session_init.mcp_manager_init", otel.name = "session_init.mcp_manager_init", )) - .await?; + .await + .validate_required_servers()?; { let mut manager_guard = sess.services.mcp_connection_manager.write().await; *manager_guard = mcp_connection_manager; From 71772262f3766a508575724d94cec2322ca06796 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:44:40 -0700 Subject: [PATCH 11/19] Drop redundant MCP status test change --- .../tests/suite/v2/mcp_server_status.rs | 28 ++----------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index 4b9db3c8ef5d..2c34684ada17 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -43,7 +43,7 @@ use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10); #[tokio::test] -async fn mcp_server_status_list_includes_healthy_and_failed_required_servers() -> Result<()> { +async fn mcp_server_status_list_returns_raw_server_and_tool_names() -> Result<()> { let server = create_mock_responses_server_sequence_unchecked(Vec::new()).await; let (mcp_server_url, mcp_server_handle) = start_mcp_server("look-up.raw").await?; let codex_home = TempDir::new()?; @@ -63,10 +63,6 @@ async fn mcp_server_status_list_includes_healthy_and_failed_required_servers() - r#" [mcp_servers.some-server] url = "{mcp_server_url}/mcp" - -[mcp_servers.required_broken] -command = "codex-definitely-not-a-real-binary" -required = true "# )); std::fs::write(config_path, config_toml)?; @@ -90,12 +86,8 @@ required = true let response: ListMcpServerStatusResponse = to_response(response)?; assert_eq!(response.next_cursor, None); - assert_eq!(response.data.len(), 2); - let status = response - .data - .iter() - .find(|status| status.name == "some-server") - .expect("healthy MCP server status"); + assert_eq!(response.data.len(), 1); + let status = &response.data[0]; assert_eq!(status.name, "some-server"); assert_eq!( status.tools.keys().cloned().collect::>(), @@ -115,20 +107,6 @@ required = true .and_then(|info| info.title.as_deref()), Some("Lookup Server") ); - let required_broken = response - .data - .iter() - .find(|status| status.name == "required_broken") - .expect("failed required MCP server status"); - assert_eq!( - ( - required_broken.server_info.as_ref(), - required_broken.tools.is_empty(), - required_broken.resources.is_empty(), - required_broken.resource_templates.is_empty(), - ), - (None, true, true, true), - ); mcp_server_handle.abort(); let _ = mcp_server_handle.await; From a4439ecce64c51bd084dca079c150294a2f44394 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 17:49:10 -0700 Subject: [PATCH 12/19] Validate required MCP servers on demand --- codex-rs/codex-mcp/src/connection_manager.rs | 75 +++++++++---------- .../codex-mcp/src/connection_manager_tests.rs | 3 +- codex-rs/core/src/session/mcp.rs | 1 + codex-rs/core/src/session/session.rs | 3 +- 4 files changed, 39 insertions(+), 43 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 3a83d878a68a..e1991b5c16da 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -106,7 +106,7 @@ pub fn tool_is_model_visible(tool: &ToolInfo) -> bool { pub struct McpConnectionManager { clients: HashMap, server_metadata: HashMap, - required_startup_failures: Vec, + required_servers: Vec, tool_plugin_provenance: Arc, host_owned_codex_apps_enabled: bool, prefix_mcp_tool_names: bool, @@ -242,10 +242,10 @@ impl McpConnectionManager { (server_name, outcome) }); } - let mut manager = Self { + let manager = Self { clients, server_metadata, - required_startup_failures: Vec::new(), + required_servers, tool_plugin_provenance, host_owned_codex_apps_enabled, prefix_mcp_tool_names, @@ -274,25 +274,43 @@ impl McpConnectionManager { }) .await; }); - manager.required_startup_failures = manager - .collect_required_startup_failures(&required_servers) - .instrument(info_span!( - "session_init.required_mcp_wait", - otel.name = "session_init.required_mcp_wait", - session_init.required_mcp_server_count = required_servers.len(), - )) - .await; manager } /// Returns this manager only if every required server initialized successfully. - pub fn validate_required_servers(self) -> Result { - if self.required_startup_failures.is_empty() { + pub async fn validate_required_servers(self) -> Result { + let failures = async { + let mut failures = Vec::new(); + for server_name in &self.required_servers { + let Some(async_managed_client) = self.clients.get(server_name).cloned() else { + failures.push(McpStartupFailure { + server: server_name.clone(), + error: format!("required MCP server `{server_name}` was not initialized"), + }); + continue; + }; + + match async_managed_client.client().await { + Ok(_) => {} + Err(error) => failures.push(McpStartupFailure { + server: server_name.clone(), + error: startup_outcome_error_message(error), + }), + } + } + failures + } + .instrument(info_span!( + "session_init.required_mcp_wait", + otel.name = "session_init.required_mcp_wait", + session_init.required_mcp_server_count = self.required_servers.len(), + )) + .await; + if failures.is_empty() { return Ok(self); } - let details = self - .required_startup_failures + let details = failures .iter() .map(|failure| format!("{}: {}", failure.server, failure.error)) .collect::>() @@ -310,7 +328,7 @@ impl McpConnectionManager { Self { clients: HashMap::new(), server_metadata: HashMap::new(), - required_startup_failures: Vec::new(), + required_servers: Vec::new(), tool_plugin_provenance: Arc::new(ToolPluginProvenance::default()), host_owned_codex_apps_enabled: false, prefix_mcp_tool_names, @@ -785,31 +803,6 @@ impl McpConnectionManager { .context("failed to get client") } - async fn collect_required_startup_failures( - &self, - required_servers: &[String], - ) -> Vec { - let mut failures = Vec::new(); - for server_name in required_servers { - let Some(async_managed_client) = self.clients.get(server_name).cloned() else { - failures.push(McpStartupFailure { - server: server_name.clone(), - error: format!("required MCP server `{server_name}` was not initialized"), - }); - continue; - }; - - match async_managed_client.client().await { - Ok(_) => {} - Err(error) => failures.push(McpStartupFailure { - server: server_name.clone(), - error: startup_outcome_error_message(error), - }), - } - } - failures - } - #[cfg(test)] fn new_uninitialized( approval_policy: &Constrained, diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index f5d6be840159..24ba5afb4665 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1167,7 +1167,8 @@ async fn required_local_stdio_without_local_runtime_fails_validation() { /*elicitation_reviewer*/ None, ) .await - .validate_required_servers(); + .validate_required_servers() + .await; let error = match result { Ok(_) => panic!("required MCP startup should fail"), diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index d18fdfc4c9fd..2a0949b49b9d 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -361,6 +361,7 @@ impl Session { ) .await .validate_required_servers() + .await { Ok(manager) => manager, Err(err) => { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index a9f1fc2d2d73..5eedbc5b7380 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1181,7 +1181,8 @@ impl Session { otel.name = "session_init.mcp_manager_init", )) .await - .validate_required_servers()?; + .validate_required_servers() + .await?; { let mut manager_guard = sess.services.mcp_connection_manager.write().await; *manager_guard = mcp_connection_manager; From cc2754bbba0afeac1bb785bc1577c6349ab38da5 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 18:56:45 -0700 Subject: [PATCH 13/19] Keep MCP refresh non-strict and cancellable --- codex-rs/core/src/session/mcp.rs | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 2a0949b49b9d..779813641843 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -20,7 +20,6 @@ use rmcp::model::CreateElicitationRequestParams; use rmcp::model::ElicitationAction; use rmcp::model::Meta; use serde_json::Map; -use std::mem; const MCP_ELICITATION_DECLINE_MESSAGE_KEY: &str = "message"; const TOOL_SUGGESTION_ACTION_INSTALL: &str = "install"; @@ -339,15 +338,21 @@ impl Session { turn_context.cwd.to_path_buf(), ), }; - let mcp_startup_cancellation_token = CancellationToken::new(); - let refreshed_manager = match McpConnectionManager::new( + let mcp_startup_cancellation_token = { + let mut guard = self.services.mcp_startup_cancellation_token.lock().await; + guard.cancel(); + let cancellation_token = CancellationToken::new(); + *guard = cancellation_token.clone(); + cancellation_token + }; + let refreshed_manager = McpConnectionManager::new( &mcp_servers, store_mode, auth_statuses, &turn_context.approval_policy, turn_context.sub_id.clone(), self.get_tx_event(), - mcp_startup_cancellation_token.clone(), + mcp_startup_cancellation_token, turn_context.permission_profile(), mcp_runtime_context, config.codex_home.to_path_buf(), @@ -359,26 +364,14 @@ impl Session { auth.as_ref(), elicitation_reviewer, ) - .await - .validate_required_servers() - .await - { - Ok(manager) => manager, - Err(err) => { - warn!("failed to refresh MCP servers: {err:#}"); - return; - } - }; + .await; { let current_manager = self.services.mcp_connection_manager.read().await; refreshed_manager.set_elicitations_auto_deny(current_manager.elicitations_auto_deny()); } let mut old_manager = { - let mut cancellation_token = self.services.mcp_startup_cancellation_token.lock().await; - cancellation_token.cancel(); - *cancellation_token = mcp_startup_cancellation_token; let mut manager = self.services.mcp_connection_manager.write().await; - mem::replace(&mut *manager, refreshed_manager) + std::mem::replace(&mut *manager, refreshed_manager) }; old_manager.shutdown().await; } From a48cb410f214216e21c198be03aa601c01ab19c3 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 19:03:19 -0700 Subject: [PATCH 14/19] Preserve MCP mixed-runtime test coverage --- .../codex-mcp/src/connection_manager_tests.rs | 94 +++++++++++++------ 1 file changed, 63 insertions(+), 31 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 24ba5afb4665..cedf5390e497 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1108,39 +1108,66 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { } #[tokio::test] -async fn required_local_stdio_without_local_runtime_fails_validation() { +async fn required_local_stdio_without_local_runtime_fails_validation_but_keeps_http_server() { let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let (tx_event, rx_event) = async_channel::unbounded(); drop(rx_event); let codex_home = tempdir().expect("tempdir"); - let mcp_servers = HashMap::from([( - "stdio".to_string(), - EffectiveMcpServer::configured(McpServerConfig { - transport: McpServerTransportConfig::Stdio { - command: "echo".to_string(), - args: Vec::new(), - env: None, - env_vars: Vec::new(), - cwd: None, - }, - environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), - enabled: true, - required: true, - supports_parallel_tool_calls: false, - disabled_reason: None, - startup_timeout_sec: None, - tool_timeout_sec: None, - default_tools_approval_mode: None, - enabled_tools: None, - disabled_tools: None, - scopes: None, - oauth: None, - oauth_resource: None, - tools: HashMap::new(), - }), - )]); + let mcp_servers = HashMap::from([ + ( + "stdio".to_string(), + EffectiveMcpServer::configured(McpServerConfig { + transport: McpServerTransportConfig::Stdio { + command: "echo".to_string(), + args: Vec::new(), + env: None, + env_vars: Vec::new(), + cwd: None, + }, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: true, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }), + ), + ( + "http".to_string(), + EffectiveMcpServer::configured(McpServerConfig { + transport: McpServerTransportConfig::StreamableHttp { + url: "http://127.0.0.1:1".to_string(), + bearer_token_env_var: None, + http_headers: None, + env_http_headers: None, + }, + environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), + enabled: true, + required: false, + supports_parallel_tool_calls: false, + disabled_reason: None, + startup_timeout_sec: None, + tool_timeout_sec: None, + default_tools_approval_mode: None, + enabled_tools: None, + disabled_tools: None, + scopes: None, + oauth: None, + oauth_resource: None, + tools: HashMap::new(), + }), + ), + ]); - let result = McpConnectionManager::new( + let manager = McpConnectionManager::new( &mcp_servers, OAuthCredentialsStoreMode::default(), HashMap::new(), @@ -1166,11 +1193,16 @@ async fn required_local_stdio_without_local_runtime_fails_validation() { /*auth*/ None, /*elicitation_reviewer*/ None, ) - .await - .validate_required_servers() .await; - let error = match result { + assert!(manager.clients.contains_key("stdio")); + assert!(manager.clients.contains_key("http")); + assert!( + !manager + .wait_for_server_ready("stdio", Duration::from_millis(10)) + .await + ); + let error = match manager.validate_required_servers().await { Ok(_) => panic!("required MCP startup should fail"), Err(error) => error, }; From 04b6ef2a7812f427e1f88790e3f4dc3a5b24415b Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 19:05:47 -0700 Subject: [PATCH 15/19] Keep MCP mixed-runtime test intent --- .../codex-mcp/src/connection_manager_tests.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index cedf5390e497..1392163e9291 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1108,7 +1108,7 @@ async fn list_all_tools_adds_server_metadata_to_cached_tools() { } #[tokio::test] -async fn required_local_stdio_without_local_runtime_fails_validation_but_keeps_http_server() { +async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { let approval_policy = Constrained::allow_any(AskForApproval::OnFailure); let (tx_event, rx_event) = async_channel::unbounded(); drop(rx_event); @@ -1126,7 +1126,7 @@ async fn required_local_stdio_without_local_runtime_fails_validation_but_keeps_h }, environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(), enabled: true, - required: true, + required: false, supports_parallel_tool_calls: false, disabled_reason: None, startup_timeout_sec: None, @@ -1167,6 +1167,7 @@ async fn required_local_stdio_without_local_runtime_fails_validation_but_keeps_h ), ]); + let cancel_token = CancellationToken::new(); let manager = McpConnectionManager::new( &mcp_servers, OAuthCredentialsStoreMode::default(), @@ -1174,7 +1175,7 @@ async fn required_local_stdio_without_local_runtime_fails_validation_but_keeps_h &approval_policy, String::new(), tx_event, - CancellationToken::new(), + cancel_token.clone(), PermissionProfile::default(), McpRuntimeContext::new( Arc::new(EnvironmentManager::without_environments()), @@ -1202,14 +1203,7 @@ async fn required_local_stdio_without_local_runtime_fails_validation_but_keeps_h .wait_for_server_ready("stdio", Duration::from_millis(10)) .await ); - let error = match manager.validate_required_servers().await { - Ok(_) => panic!("required MCP startup should fail"), - Err(error) => error, - }; - assert_eq!( - error.to_string(), - "required MCP servers failed to initialize: stdio: local stdio MCP server `stdio` requires a local environment" - ); + cancel_token.cancel(); } #[test] From 0588a59e00333db60c45a80602c47061df309690 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 19:10:33 -0700 Subject: [PATCH 16/19] Retain MCP startup error assertion --- codex-rs/codex-mcp/src/connection_manager_tests.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 1392163e9291..4b90337ac278 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -1203,6 +1203,20 @@ async fn no_local_runtime_fails_local_stdio_but_keeps_local_http_server() { .wait_for_server_ready("stdio", Duration::from_millis(10)) .await ); + let error = match manager + .clients + .get("stdio") + .expect("stdio client") + .client() + .await + { + Ok(_) => panic!("local stdio MCP startup should fail"), + Err(error) => error, + }; + assert_eq!( + startup_outcome_error_message(error), + "local stdio MCP server `stdio` requires a local environment" + ); cancel_token.cancel(); } From 4052ad0f2d8095406635faefa30522324067cd14 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 19:27:04 -0700 Subject: [PATCH 17/19] Install MCP manager before required validation --- codex-rs/codex-mcp/src/connection_manager.rs | 6 +++--- codex-rs/core/src/session/session.rs | 11 ++++------- codex-rs/core/src/state/service.rs | 15 +++++++++++++++ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index e1991b5c16da..3390f18ca96e 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -277,8 +277,8 @@ impl McpConnectionManager { manager } - /// Returns this manager only if every required server initialized successfully. - pub async fn validate_required_servers(self) -> Result { + /// Validates that every required server initialized successfully. + pub async fn validate_required_servers(&self) -> Result<()> { let failures = async { let mut failures = Vec::new(); for server_name in &self.required_servers { @@ -307,7 +307,7 @@ impl McpConnectionManager { )) .await; if failures.is_empty() { - return Ok(self); + return Ok(()); } let details = failures diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 5eedbc5b7380..9ae17b907110 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -1180,13 +1180,10 @@ impl Session { "session_init.mcp_manager_init", otel.name = "session_init.mcp_manager_init", )) - .await - .validate_required_servers() - .await?; - { - let mut manager_guard = sess.services.mcp_connection_manager.write().await; - *manager_guard = mcp_connection_manager; - } + .await; + sess.services + .install_mcp_connection_manager(mcp_connection_manager) + .await?; sess.schedule_startup_prewarm(session_configuration.base_instructions.clone()) .await; let session_start_source = match &initial_history { diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 59def9c7abac..6de688c12101 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -15,6 +15,7 @@ use crate::tools::code_mode::CodeModeService; use crate::tools::network_approval::NetworkApprovalService; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecProcessManager; +use anyhow::Result; use arc_swap::ArcSwap; use arc_swap::ArcSwapOption; use codex_analytics::AnalyticsEventsClient; @@ -82,3 +83,17 @@ pub(crate) struct SessionServices { /// the same manager through child-thread spawn paths without reconstructing it. pub(crate) environment_manager: Arc, } + +impl SessionServices { + pub(crate) async fn install_mcp_connection_manager( + &self, + manager: McpConnectionManager, + ) -> Result<()> { + *self.mcp_connection_manager.write().await = manager; + self.mcp_connection_manager + .read() + .await + .validate_required_servers() + .await + } +} From c4e6566a709a825200b021c42952decc6a398895 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 19:29:20 -0700 Subject: [PATCH 18/19] Document MCP manager installation ordering --- codex-rs/codex-mcp/src/connection_manager.rs | 5 ++++- codex-rs/core/src/state/service.rs | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 3390f18ca96e..5b7b6b3e81f2 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -277,7 +277,10 @@ impl McpConnectionManager { manager } - /// Validates that every required server initialized successfully. + /// Waits for every required server and reports their startup failures together. + /// + /// Callers must make the manager reachable to request handlers before awaiting this method, + /// because server initialization may require client elicitation. pub async fn validate_required_servers(&self) -> Result<()> { let failures = async { let mut failures = Vec::new(); diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 6de688c12101..970efac0166b 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -85,6 +85,8 @@ pub(crate) struct SessionServices { } impl SessionServices { + /// Installs the manager before validating required servers so startup-time elicitation can + /// resolve through the session's manager while validation waits. pub(crate) async fn install_mcp_connection_manager( &self, manager: McpConnectionManager, From 6fa2495bc95331e9f06340cd2011e8aad794a384 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Tue, 9 Jun 2026 20:32:17 -0700 Subject: [PATCH 19/19] codex: fix MCP install lint expectation --- codex-rs/core/src/session/session.rs | 4 ---- codex-rs/core/src/state/service.rs | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 9ae17b907110..98d19957c046 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -469,10 +469,6 @@ impl Session { #[instrument(name = "session_init", level = "info", skip_all)] #[allow(clippy::too_many_arguments)] - #[expect( - clippy::await_holding_invalid_type, - reason = "session initialization must serialize access through session-owned manager guards" - )] pub(crate) async fn new( mut session_configuration: SessionConfiguration, config: Arc, diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 970efac0166b..9bc4d739152e 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -87,6 +87,10 @@ pub(crate) struct SessionServices { impl SessionServices { /// Installs the manager before validating required servers so startup-time elicitation can /// resolve through the session's manager while validation waits. + #[expect( + clippy::await_holding_invalid_type, + reason = "required MCP validation keeps the installed manager reachable for startup-time elicitation" + )] pub(crate) async fn install_mcp_connection_manager( &self, manager: McpConnectionManager,