From f6fbd733d9b9bdb1a4b81a4aeae4ec98d7516082 Mon Sep 17 00:00:00 2001 From: joshka-oai Date: Fri, 17 Oct 2025 11:54:45 -0700 Subject: [PATCH] Improve MCP startup error messaging --- codex-rs/core/src/codex.rs | 13 +- codex-rs/core/src/mcp_connection_manager.rs | 126 ++++++++++++++++++-- 2 files changed, 128 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 5e28a8170e..61a811653c 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -59,6 +59,7 @@ use crate::executor::Executor; use crate::executor::ExecutorConfig; use crate::executor::normalize_exec_result; use crate::mcp::auth::compute_auth_statuses; +use crate::mcp_connection_manager::ClientStartError; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_family::find_family_for_model; use crate::openai_model_info::get_model_info; @@ -418,10 +419,16 @@ impl Session { // Surface individual client start-up failures to the user. if !failed_clients.is_empty() { for (server_name, err) in failed_clients { + let ClientStartError { + source, + display_message, + } = err; let log_message = - format!("MCP client for `{server_name}` failed to start: {err:#}"); + format!("MCP client for `{server_name}` failed to start: {source:#}"); error!("{log_message}"); - let display_message = if matches!( + let display_message = if let Some(message) = display_message { + message + } else if matches!( auth_statuses.get(&server_name), Some(McpAuthStatus::NotLoggedIn) ) { @@ -429,7 +436,7 @@ impl Session { "The {server_name} MCP server is not logged in. Run `codex mcp login {server_name}` to log in." ) } else { - log_message + log_message.clone() }; post_session_configured_error_events.push(Event { id: INITIAL_SUBMIT_ID.to_owned(), diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 768c6b01a6..3cde89d107 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -49,7 +49,31 @@ const DEFAULT_TOOL_TIMEOUT: Duration = Duration::from_secs(60); /// Map that holds a startup error for every MCP server that could **not** be /// spawned successfully. -pub type ClientStartErrors = HashMap; +pub type ClientStartErrors = HashMap; + +/// Detailed information about why an MCP client failed to start. +pub struct ClientStartError { + /// The underlying error used for logging/debugging. + pub source: anyhow::Error, + /// Optional user-facing message with concrete remediation steps. + pub display_message: Option, +} + +impl ClientStartError { + fn new(source: anyhow::Error) -> Self { + Self { + source, + display_message: None, + } + } + + fn with_display_message(source: anyhow::Error, display_message: String) -> Self { + Self { + source, + display_message: Some(display_message), + } + } +} fn qualify_tools(tools: Vec) -> HashMap { let mut used_names = HashSet::new(); @@ -101,6 +125,16 @@ enum McpClientAdapter { Rmcp(Arc), } +#[derive(Clone)] +enum TransportInfo { + Stdio, + StreamableHttp { + url: String, + bearer_token_env_var: Option, + provided_bearer_token: bool, + }, +} + impl McpClientAdapter { async fn new_stdio_client( use_rmcp_client: bool, @@ -203,7 +237,7 @@ impl McpConnectionManager { let error = anyhow::anyhow!( "invalid server name '{server_name}': must match pattern ^[a-zA-Z0-9_-]+$" ); - errors.insert(server_name, error); + errors.insert(server_name, ClientStartError::new(error)); continue; } @@ -218,10 +252,32 @@ impl McpConnectionManager { McpServerTransportConfig::StreamableHttp { bearer_token_env_var, .. - } => resolve_bearer_token(&server_name, bearer_token_env_var.as_deref()), - _ => Ok(None), + } => match resolve_bearer_token(&server_name, bearer_token_env_var.as_deref()) { + Ok(token) => token, + Err(error) => { + errors.insert(server_name, ClientStartError::new(error)); + continue; + } + }, + _ => None, }; + let transport_info = match &cfg.transport { + McpServerTransportConfig::Stdio { .. } => TransportInfo::Stdio, + McpServerTransportConfig::StreamableHttp { + url, + bearer_token_env_var, + .. + } => TransportInfo::StreamableHttp { + url: url.clone(), + bearer_token_env_var: bearer_token_env_var.clone(), + provided_bearer_token: resolved_bearer_token.is_some(), + }, + }; + + let resolved_bearer_token_for_spawn = resolved_bearer_token.clone(); + let transport_info_for_spawn = transport_info.clone(); + join_set.spawn(async move { let McpServerConfig { transport, .. } = cfg; let params = mcp_types::InitializeRequestParams { @@ -263,7 +319,7 @@ impl McpConnectionManager { McpClientAdapter::new_streamable_http_client( server_name.clone(), url, - resolved_bearer_token.unwrap_or_default(), + resolved_bearer_token_for_spawn, params, startup_timeout, store_mode, @@ -273,14 +329,17 @@ impl McpConnectionManager { } .map(|c| (c, startup_timeout)); - ((server_name, tool_timeout), client) + ( + (server_name, tool_timeout, transport_info_for_spawn), + client, + ) }); } let mut clients: HashMap = HashMap::with_capacity(join_set.len()); while let Some(res) = join_set.join_next().await { - let ((server_name, tool_timeout), client_res) = match res { + let ((server_name, tool_timeout, transport_info), client_res) = match res { Ok(result) => result, Err(e) => { warn!("Task panic when starting MCP server: {e:#}"); @@ -300,7 +359,8 @@ impl McpConnectionManager { ); } Err(e) => { - errors.insert(server_name, e); + let start_error = classify_start_error(&server_name, transport_info, e); + errors.insert(server_name, start_error); } } } @@ -381,6 +441,56 @@ fn resolve_bearer_token( } } +fn classify_start_error( + server_name: &str, + transport: TransportInfo, + error: anyhow::Error, +) -> ClientStartError { + match transport { + TransportInfo::Stdio => ClientStartError::new(error), + TransportInfo::StreamableHttp { + url, + bearer_token_env_var, + provided_bearer_token, + } => { + if is_auth_required_error(&error) { + let mut message = format!( + "Codex could not authenticate with the `{server_name}` MCP server at {url}." + ); + + if let Some(env_var) = bearer_token_env_var { + if provided_bearer_token { + message.push_str(&format!( + " The bearer token from `{env_var}` was rejected. Update the token value or run `codex mcp login {server_name}` to generate a new one." + )); + } else { + message.push_str(&format!( + " Set `{env_var}` with a valid bearer token or run `codex mcp login {server_name}` to complete authentication." + )); + } + } else { + message.push_str(&format!( + " Run `codex mcp login {server_name}` to authorize Codex or configure `bearer_token_env_var` with a valid token in your MCP server config." + )); + } + + ClientStartError::with_display_message(error, message) + } else { + ClientStartError::new(error) + } + } + } +} + +fn is_auth_required_error(error: &anyhow::Error) -> bool { + error.chain().any(|cause| { + let message = cause.to_string().to_ascii_lowercase(); + message.contains("auth required") + || message.contains("unauthorized") + || message.contains("401") + }) +} + /// Query every server for its available tools and return a single map that /// contains **all** tools. Each key is the fully-qualified name for the tool. async fn list_all_tools(clients: &HashMap) -> Result> {