diff --git a/codex-rs/cli/src/doctor.rs b/codex-rs/cli/src/doctor.rs index af478a9d12..03c4781946 100644 --- a/codex-rs/cli/src/doctor.rs +++ b/codex-rs/cli/src/doctor.rs @@ -1543,6 +1543,7 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D let mut details = Vec::new(); let mut transport_counts: BTreeMap<&'static str, usize> = BTreeMap::new(); let mut disabled = 0usize; + let mut invalid_config = Vec::new(); let mut missing_env = Vec::new(); let mut unreachable_required_http = Vec::new(); let mut unreachable_optional_http = Vec::new(); @@ -1566,43 +1567,44 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D } let command_is_empty = command.trim().is_empty(); if command_is_empty { - missing_env.push(format!("{name}: stdio command is empty")); + invalid_config.push(format!("{name}: stdio command is empty")); } if server.is_local_environment() { let host_native_cwd = cwd.as_ref().map(|cwd| Path::new(cwd.as_str())); if let Some(cwd) = host_native_cwd && !cwd.exists() { - missing_env.push(format!("{name}: cwd does not exist ({})", cwd.display())); + invalid_config + .push(format!("{name}: cwd does not exist ({})", cwd.display())); } if !command_is_empty && let Err(err) = stdio_command_resolves(command, host_native_cwd, env.as_ref()) { - missing_env.push(format!( + invalid_config.push(format!( "{name}: stdio command {command:?} is not resolvable ({err})" )); } } else { match cwd { Some(cwd) if cwd.to_inferred_path_uri().is_none() => { - missing_env + invalid_config .push(format!("{name}: remote stdio cwd is not absolute ({cwd})")); } - None => missing_env + None => invalid_config .push(format!("{name}: remote stdio requires an explicit cwd")), Some(_) => {} } } if let Some(env) = env { for key in env.keys().filter(|key| key.trim().is_empty()) { - missing_env.push(format!("{name}: empty env key {key}")); + invalid_config.push(format!("{name}: empty env key {key}")); } } for env_var in env_vars { if env_var.is_remote_source() { if server.is_local_environment() { - missing_env.push(format!( + invalid_config.push(format!( "{name}: env_vars entry `{}` uses source `remote`, which requires remote MCP stdio", env_var.name() )); @@ -1652,7 +1654,25 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D for (transport, count) in transport_counts { details.push(format!("{transport} servers: {count}")); } - details.extend(missing_env.iter().cloned()); + let mut hints = Vec::new(); + if !invalid_config.is_empty() { + hints.push("Correct the listed MCP server configuration errors."); + } + if !missing_env.is_empty() { + hints.push("Set the missing MCP env vars."); + } + if !unreachable_required_http.is_empty() || !unreachable_optional_http.is_empty() { + hints.push("Check that the MCP server is running and its URL is reachable."); + } + let required_missing = servers.iter().any(|(name, server)| { + server.required + && invalid_config + .iter() + .chain(&missing_env) + .any(|detail| detail.starts_with(&format!("{name}:"))) + }); + details.extend(invalid_config); + details.extend(missing_env); details.extend( unreachable_required_http .iter() @@ -1664,15 +1684,9 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D .map(|detail| format!("optional reachability failed: {detail}")), ); - let required_missing = servers.iter().any(|(name, server)| { - server.required - && missing_env - .iter() - .any(|missing| missing.starts_with(&format!("{name}:"))) - }); let status = if required_missing || !unreachable_required_http.is_empty() { CheckStatus::Fail - } else if !missing_env.is_empty() || !unreachable_optional_http.is_empty() { + } else if !hints.is_empty() { CheckStatus::Warning } else { CheckStatus::Ok @@ -1685,7 +1699,8 @@ async fn mcp_check_from_servers(servers: &HashMap) -> D let mut check = DoctorCheck::new("mcp.config", "mcp", status, summary).details(details); if status != CheckStatus::Ok { - check = check.remediation("Set the missing MCP env vars or disable the affected server."); + hints.push("Alternatively, disable the affected server."); + check = check.remediation(hints.join(" ")); } check } @@ -3612,6 +3627,7 @@ mod tests { assert_eq!(check.status, CheckStatus::Warning); assert_eq!(check.summary, "MCP configuration has optional issues"); + insta::assert_snapshot!(check.remediation.unwrap(), @"Check that the MCP server is running and its URL is reachable. Alternatively, disable the affected server."); assert!( check .details @@ -3620,6 +3636,23 @@ mod tests { ); } + #[tokio::test] + async fn mcp_check_reports_both_missing_token_and_unreachable_url() { + let server = toml::from_str( + r#" + url = "http://127.0.0.1:9/mcp" + bearer_token_env_var = "CODEX_DOCTOR_MISSING_MCP_TOKEN" + required = true + "#, + ) + .expect("required MCP config"); + let check = + mcp_check_from_servers(&HashMap::from([("required".to_string(), server)])).await; + + assert_eq!(check.status, CheckStatus::Fail); + insta::assert_snapshot!(check.remediation.unwrap(), @"Set the missing MCP env vars. Check that the MCP server is running and its URL is reachable. Alternatively, disable the affected server."); + } + #[tokio::test] async fn mcp_check_fails_required_remote_stdio_env_var() { let command = toml::Value::String( @@ -4195,6 +4228,7 @@ mod tests { check.summary, "MCP configuration has failing required inputs or reachability" ); + insta::assert_snapshot!(check.remediation.unwrap(), @"Correct the listed MCP server configuration errors. Alternatively, disable the affected server."); assert!(check.details.iter().any(|detail| { detail.contains( "required: stdio command \"definitely-missing-codex-doctor-mcp\" is not resolvable",