From d791098e51569fc923f078c964ae21e8237e8651 Mon Sep 17 00:00:00 2001 From: Alex Clarke Date: Fri, 14 Aug 2026 11:07:56 -0600 Subject: [PATCH] feat: reason-specific warnings for MCP servers that fail OAuth at startup Distinguish why an OAuth MCP server was not started: never authenticated (no stored credentials), stored token expired and refresh failed, or the server rejected a token that looked valid. McpTokenStatus replaces the Option return of load_or_refresh_mcp_token, and McpAuthRequired carries the reason across the error boundary via anyhow context. --- src/config/mcp_factory.rs | 20 +++--- src/config/request_context.rs | 15 ++-- src/mcp/mod.rs | 132 ++++++++++++++++++++++++++++++---- src/mcp/oauth.rs | 68 ++++++++++++++---- 4 files changed, 192 insertions(+), 43 deletions(-) diff --git a/src/config/mcp_factory.rs b/src/config/mcp_factory.rs index ef9f3f4..4a46a35 100644 --- a/src/config/mcp_factory.rs +++ b/src/config/mcp_factory.rs @@ -1,6 +1,7 @@ +use crate::mcp::oauth::McpTokenStatus; use crate::mcp::{ - ConnectedServer, JsonField, McpServer, McpTransportType, is_auth_required_error, oauth, - spawn_mcp_server, + ConnectedServer, JsonField, McpAuthReason, McpAuthRequired, McpServer, McpTransportType, + is_auth_required_error, oauth, spawn_mcp_server, }; use anyhow::Result; @@ -102,19 +103,20 @@ impl McpFactory { return Ok(existing); } - let bearer_token = if spec.is_remote() { + let token_status = if spec.is_remote() { oauth::load_or_refresh_mcp_token(name).await } else { - None + McpTokenStatus::NotAuthenticated }; - let handle = spawn_mcp_server(spec, log_path, bearer_token) + let auth_reason = McpAuthReason::from_token_status(&token_status); + let handle = spawn_mcp_server(spec, log_path, token_status.into_token()) .await .map_err(|e| { if is_auth_required_error(&e) { - e.context(format!( - "MCP server '{name}' requires OAuth authentication. \ - Run `coyote --auth-mcp {name}` or `.mcp auth {name}` in the REPL to authenticate." - )) + e.context(McpAuthRequired { + server: name.to_string(), + reason: auth_reason, + }) } else { e } diff --git a/src/config/request_context.rs b/src/config/request_context.rs index 6f9c4ed..606cc48 100644 --- a/src/config/request_context.rs +++ b/src/config/request_context.rs @@ -22,7 +22,7 @@ use crate::function::{ }; use crate::mcp::{ MCP_DESCRIBE_META_FUNCTION_NAME_PREFIX, MCP_INVOKE_META_FUNCTION_NAME_PREFIX, - MCP_SEARCH_META_FUNCTION_NAME_PREFIX, is_auth_required_error, + MCP_SEARCH_META_FUNCTION_NAME_PREFIX, McpAuthReason, McpAuthRequired, is_auth_required_error, }; use crate::rag::Rag; use crate::supervisor::Supervisor; @@ -3427,7 +3427,11 @@ impl RequestContext { { Ok(handle) => handles.push((id.clone(), handle)), Err(e) if is_auth_required_error(&e) => { - auth_required.push(id.clone()) + let reason = e + .downcast_ref::() + .map(|a| a.reason) + .unwrap_or(McpAuthReason::NotAuthenticated); + auth_required.push((id.clone(), reason)); } Err(e) => return Err(e), } @@ -3444,11 +3448,8 @@ impl RequestContext { for (id, handle) in handles { mcp_runtime.insert(id, handle); } - for id in auth_required { - eprintln!( - "Warning: MCP server '{id}' requires OAuth authentication and was not started. \ - Run `.mcp auth {id}` (or `coyote --auth-mcp {id}`) to authenticate and attach it." - ); + for (id, reason) in auth_required { + eprintln!("Warning: {}", McpAuthRequired { server: id, reason }); } } } diff --git a/src/mcp/mod.rs b/src/mcp/mod.rs index b826b85..4b8baf4 100644 --- a/src/mcp/mod.rs +++ b/src/mcp/mod.rs @@ -21,6 +21,8 @@ use rmcp::{RoleClient, ServiceExt}; use serde::{Deserialize, Serialize}; use sse_transport::LegacySseTransport; use std::collections::{HashMap, HashSet}; +use std::fmt; +use std::fmt::Display; use std::fs::OpenOptions; use std::path::{Path, PathBuf}; use std::process::Stdio; @@ -326,23 +328,29 @@ impl McpRegistry { .and_then(|c| c.mcp_servers.get(&id)) .with_context(|| format!("MCP server not found in config: {id}"))?; - let bearer_token = if spec.is_remote() { + let token_status = if spec.is_remote() { oauth::load_or_refresh_mcp_token(&id).await } else { - None + oauth::McpTokenStatus::NotAuthenticated }; + let auth_reason = McpAuthReason::from_token_status(&token_status); - let service = match spawn_mcp_server(spec, self.log_path.as_deref(), bearer_token).await { - Ok(s) => s, - Err(e) if is_auth_required_error(&e) => { - warn!( - "MCP server '{id}' requires OAuth authentication. \ - Run `coyote --auth-mcp {id}` or `.mcp auth {id}` in the REPL to authenticate." - ); - return Ok(None); - } - Err(e) => return Err(e), - }; + let service = + match spawn_mcp_server(spec, self.log_path.as_deref(), token_status.into_token()).await + { + Ok(s) => s, + Err(e) if is_auth_required_error(&e) => { + warn!( + "{}", + McpAuthRequired { + server: id, + reason: auth_reason, + } + ); + return Ok(None); + } + Err(e) => return Err(e), + }; let tools = service.list_tools(None).await?; debug!("Available tools for MCP server {id}: {tools:?}"); @@ -458,9 +466,59 @@ fn merge_bearer_token( } } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum McpAuthReason { + NotAuthenticated, + RefreshFailed, + TokenRejected, +} + +impl McpAuthReason { + pub(crate) fn from_token_status(status: &oauth::McpTokenStatus) -> Self { + match status { + oauth::McpTokenStatus::Token(_) => Self::TokenRejected, + oauth::McpTokenStatus::NotAuthenticated => Self::NotAuthenticated, + oauth::McpTokenStatus::RefreshFailed => Self::RefreshFailed, + } + } +} + +#[derive(Debug)] +pub(crate) struct McpAuthRequired { + pub server: String, + pub reason: McpAuthReason, +} + +impl Display for McpAuthRequired { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let server = &self.server; + match self.reason { + McpAuthReason::NotAuthenticated => write!( + f, + "MCP server '{server}' requires OAuth authentication and was not started \ + (no stored credentials). Run `.mcp auth {server}` (or `coyote --auth-mcp \ + {server}`) to authenticate and attach it." + ), + McpAuthReason::RefreshFailed => write!( + f, + "MCP server '{server}' was not started: stored OAuth token has expired and \ + automatic refresh failed. Run `.mcp auth {server}` (or `coyote --auth-mcp \ + {server}`) to re-authenticate and attach it." + ), + McpAuthReason::TokenRejected => write!( + f, + "MCP server '{server}' was not started: the server rejected the stored OAuth \ + token. Run `.mcp auth {server}` (or `coyote --auth-mcp {server}`) to \ + re-authenticate and attach it." + ), + } + } +} + pub(crate) fn is_auth_required_error(e: &Error) -> bool { - e.chain() - .any(|cause| cause.to_string().contains("Auth required")) + e.downcast_ref::().is_some() + || e.chain() + .any(|cause| cause.to_string().contains("Auth required")) } async fn spawn_http_mcp_server( @@ -1074,4 +1132,48 @@ mod tests { assert!(is_auth_required_error(&e)); } + + #[test] + fn auth_reason_maps_token_status() { + assert_eq!( + McpAuthReason::from_token_status(&oauth::McpTokenStatus::Token("tok".into())), + McpAuthReason::TokenRejected + ); + assert_eq!( + McpAuthReason::from_token_status(&oauth::McpTokenStatus::NotAuthenticated), + McpAuthReason::NotAuthenticated + ); + assert_eq!( + McpAuthReason::from_token_status(&oauth::McpTokenStatus::RefreshFailed), + McpAuthReason::RefreshFailed + ); + } + + #[test] + fn mcp_auth_required_context_downcasts_with_reason() { + let e = anyhow!("Auth required, when send initialize request").context(McpAuthRequired { + server: "github".into(), + reason: McpAuthReason::RefreshFailed, + }); + + assert!(is_auth_required_error(&e)); + let ctx = e.downcast_ref::().unwrap(); + assert_eq!(ctx.server, "github"); + assert_eq!(ctx.reason, McpAuthReason::RefreshFailed); + } + + #[test] + fn mcp_auth_required_display_is_reason_specific() { + let msg = |reason| { + McpAuthRequired { + server: "github".into(), + reason, + } + .to_string() + }; + + assert!(msg(McpAuthReason::NotAuthenticated).contains("no stored credentials")); + assert!(msg(McpAuthReason::RefreshFailed).contains("expired and automatic refresh failed")); + assert!(msg(McpAuthReason::TokenRejected).contains("rejected the stored OAuth token")); + } } diff --git a/src/mcp/oauth.rs b/src/mcp/oauth.rs index 99b15f4..bd2f89e 100644 --- a/src/mcp/oauth.rs +++ b/src/mcp/oauth.rs @@ -202,34 +202,54 @@ pub async fn run_mcp_oauth_flow( run_oauth_flow(&provider, &mcp_token_key(server_name)).await } -pub async fn load_or_refresh_mcp_token(server_name: &str) -> Option { +#[derive(Debug, PartialEq, Eq)] +pub enum McpTokenStatus { + Token(String), + NotAuthenticated, + RefreshFailed, +} + +impl McpTokenStatus { + pub fn into_token(self) -> Option { + match self { + Self::Token(token) => Some(token), + Self::NotAuthenticated | Self::RefreshFailed => None, + } + } +} + +pub async fn load_or_refresh_mcp_token(server_name: &str) -> McpTokenStatus { let key = mcp_token_key(server_name); - let tokens = load_oauth_tokens(&key)?; + let Some(tokens) = load_oauth_tokens(&key) else { + return McpTokenStatus::NotAuthenticated; + }; if Utc::now().timestamp() < tokens.expires_at { - return Some(tokens.access_token); + return McpTokenStatus::Token(tokens.access_token); } if in_refresh_failure_backoff(server_name) { debug!("Skipping token refresh for MCP server '{server_name}': recent attempt failed"); - return None; + return McpTokenStatus::RefreshFailed; } let lock = refresh_lock(server_name); let _guard = lock.lock().await; // A concurrent caller may have refreshed while we waited for the lock. - let tokens = load_oauth_tokens(&key)?; + let Some(tokens) = load_oauth_tokens(&key) else { + return McpTokenStatus::NotAuthenticated; + }; if Utc::now().timestamp() < tokens.expires_at { - return Some(tokens.access_token); + return McpTokenStatus::Token(tokens.access_token); } if in_refresh_failure_backoff(server_name) { debug!("Skipping token refresh for MCP server '{server_name}': recent attempt failed"); - return None; + return McpTokenStatus::RefreshFailed; } match refresh_mcp_token(server_name, &key, &tokens).await { - Ok(access_token) => Some(access_token), + Ok(access_token) => McpTokenStatus::Token(access_token), Err(e) => { note_refresh_failure(server_name); warn!( @@ -240,7 +260,7 @@ pub async fn load_or_refresh_mcp_token(server_name: &str) -> Option { "Token refresh error for MCP server '{server_name}': {}", redact_refresh_error(&e) ); - None + McpTokenStatus::RefreshFailed } } } @@ -975,7 +995,7 @@ mod tests { #[test] #[serial] - fn expired_token_with_old_format_registration_returns_none() { + fn expired_token_with_old_format_registration_reports_refresh_failed() { with_temp_cache(|| { let dir = paths::oauth_tokens_dir(); fs::create_dir_all(&dir).unwrap(); @@ -994,12 +1014,36 @@ mod tests { .enable_all() .build() .unwrap(); - let token = rt.block_on(load_or_refresh_mcp_token("legacyref")); + let status = rt.block_on(load_or_refresh_mcp_token("legacyref")); - assert_eq!(token, None); + assert_eq!(status, McpTokenStatus::RefreshFailed); }); } + #[test] + #[serial] + fn missing_token_file_reports_not_authenticated() { + with_temp_cache(|| { + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let status = rt.block_on(load_or_refresh_mcp_token("never-authed")); + + assert_eq!(status, McpTokenStatus::NotAuthenticated); + }); + } + + #[test] + fn token_status_into_token_extracts_only_token_variant() { + assert_eq!( + McpTokenStatus::Token("tok".into()).into_token(), + Some("tok".to_string()) + ); + assert_eq!(McpTokenStatus::NotAuthenticated.into_token(), None); + assert_eq!(McpTokenStatus::RefreshFailed.into_token(), None); + } + #[test] fn refresh_failure_backoff_memoizes_per_server() { assert!(!in_refresh_failure_backoff("backoff-test-server"));