From d996b4f02a204b36060de4711e106e8ecc7be9ac Mon Sep 17 00:00:00 2001 From: Andrei Eternal Date: Thu, 10 Sep 2026 00:52:04 +0000 Subject: [PATCH] Report OAuth authentication failures in MCP status snapshots (#44359) ## Why Status-only discovery could retain an `OAuth` status based on stored credentials even after the connection attempt failed authentication. This discovery path has no event channel to report the failure. ## What changed Update the snapshot to report `NotLoggedIn` for servers previously marked `OAuth` whose connection attempts failed authentication. Preserve other authentication statuses and OAuth statuses for failures unrelated to authentication. ## Testing Add a regression test covering OAuth authentication failure, successful OAuth startup, an unrelated provider error, and a bearer-token authentication failure. GitOrigin-RevId: 9df68fb1f818e40acab0c71bb4fda118f03a7611 --- .../codex-mcp/src/connection_manager_tests.rs | 8 ++- codex-rs/codex-mcp/src/mcp/mod.rs | 11 +++- codex-rs/codex-mcp/src/mcp/mod_tests.rs | 64 +++++++++++++++++++ 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/codex-rs/codex-mcp/src/connection_manager_tests.rs b/codex-rs/codex-mcp/src/connection_manager_tests.rs index 62bfbeeb74..f8f8a468a8 100644 --- a/codex-rs/codex-mcp/src/connection_manager_tests.rs +++ b/codex-rs/codex-mcp/src/connection_manager_tests.rs @@ -125,7 +125,11 @@ impl McpConnectionSet { } } - fn insert_test_client(&mut self, name: impl Into, client: AsyncManagedClient) { + pub(crate) fn insert_test_client( + &mut self, + name: impl Into, + client: AsyncManagedClient, + ) { let name = name.into(); self.servers.insert( name, @@ -540,7 +544,7 @@ async fn prepared_call_timeout_includes_trusted_access_lookup() { assert!(format!("{error:#}").contains("timed out awaiting tools/call after 1s")); } -async fn create_ready_async_managed_client(tools: Vec) -> AsyncManagedClient { +pub(crate) async fn create_ready_async_managed_client(tools: Vec) -> AsyncManagedClient { AsyncManagedClient { client: futures::future::ready::>(Ok( create_test_managed_client(tools).await, diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index a0bec56444..33b47cadde 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -802,13 +802,22 @@ async fn collect_mcp_server_status_snapshot_from_manager( .insert(tool_name, tool); } + // Status-only discovery has no event channel. Report OAuth failures from the completed + // connection attempt instead of retaining the credential-presence status read beforehand. + let mut auth_statuses = auth_statuses_from_entries(&auth_status_entries); + for server_name in mcp_connection_manager.authentication_failed_servers().await { + if auth_statuses.get(&server_name) == Some(&McpAuthStatus::OAuth) { + auth_statuses.insert(server_name, McpAuthStatus::NotLoggedIn); + } + } + McpServerStatusSnapshot { server_infos, tools_by_server, tools_errors, resources: convert_mcp_resources(resources), resource_templates: convert_mcp_resource_templates(resource_templates), - auth_statuses: auth_statuses_from_entries(&auth_status_entries), + auth_statuses, server_names, } } diff --git a/codex-rs/codex-mcp/src/mcp/mod_tests.rs b/codex-rs/codex-mcp/src/mcp/mod_tests.rs index 8c267d4db7..e6e5313717 100644 --- a/codex-rs/codex-mcp/src/mcp/mod_tests.rs +++ b/codex-rs/codex-mcp/src/mcp/mod_tests.rs @@ -1,6 +1,9 @@ use super::*; use crate::McpPluginAttribution; use crate::McpServerRegistration; +use crate::connection_manager::tests::create_ready_async_managed_client; +use crate::mcp::auth::McpAuthStatusEntry; +use crate::rmcp_client::StartupOutcomeError; use codex_config::Constrained; use codex_config::types::AppToolApproval; use codex_config::types::AuthKeyringBackendKind; @@ -12,12 +15,73 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; +use codex_rmcp_client::McpAuthState; +use futures::FutureExt; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; +#[tokio::test] +async fn status_snapshot_only_downgrades_oauth_authentication_failures() { + let auth_failure = StartupOutcomeError::Failed { + error: "OAuth refresh token was rejected".to_string(), + is_authentication_required: true, + }; + let mut manager = McpConnectionSet::empty(/*prefix_mcp_tool_names*/ true); + let mut auth_status_entries = HashMap::new(); + for (name, auth_state, startup_error) in [ + ( + "oauth-failed", + McpAuthState::OAuth, + Some(auth_failure.clone()), + ), + ("oauth-ready", McpAuthState::OAuth, None), + ( + "oauth-provider-error", + McpAuthState::OAuth, + Some(StartupOutcomeError::Failed { + error: "provider temporarily unavailable".to_string(), + is_authentication_required: false, + }), + ), + ("bearer", McpAuthState::BearerToken, Some(auth_failure)), + ] { + let mut client = create_ready_async_managed_client(Vec::new()).await; + if let Some(error) = startup_error { + client.client = futures::future::ready(Err(error)).boxed().shared(); + } + manager.insert_test_client(name, client); + auth_status_entries.insert( + name.to_string(), + McpAuthStatusEntry { + config: None, + auth_state, + }, + ); + } + + let server_names = auth_status_entries.keys().cloned().collect(); + let snapshot = collect_mcp_server_status_snapshot_from_manager( + &manager, + auth_status_entries, + server_names, + McpSnapshotDetail::ToolsAndAuthOnly, + ) + .await; + + assert_eq!( + snapshot.auth_statuses, + HashMap::from([ + ("oauth-failed".to_string(), McpAuthStatus::NotLoggedIn), + ("oauth-ready".to_string(), McpAuthStatus::OAuth), + ("oauth-provider-error".to_string(), McpAuthStatus::OAuth), + ("bearer".to_string(), McpAuthStatus::BearerToken), + ]), + ); +} + pub(crate) fn test_mcp_config(codex_home: PathBuf) -> McpConfig { McpConfig { chatgpt_base_url: "https://chatgpt.com".to_string(),