From c5122161fe353bc24d91b1dd19cc9725a995864b Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Tue, 3 Nov 2026 04:00:17 +0000 Subject: [PATCH] Tailor MCP doctor remediation to detected problems (#59057) ## Why MCP checks recommended setting missing environment variables even when the problem was an invalid server configuration or an unreachable URL. ## What changed Separate configuration errors from missing environment variables and build remediation guidance from all detected issue types. Include server reachability advice for failed HTTP probes, while retaining the option to disable affected servers. ## Testing Add remediation snapshots for unreachable optional HTTP servers, missing required stdio commands, and a required HTTP server with both a missing token and an unreachable URL. GitOrigin-RevId: 8e8eb743414297561793be51cf3a25e5bd664f27 --- codex-rs/cli/src/doctor.rs | 66 +++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 16 deletions(-) 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",