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<String> return of load_or_refresh_mcp_token, and McpAuthRequired carries the reason across the error boundary via anyhow context.
This commit is contained in:
@@ -1,6 +1,7 @@
|
|||||||
|
use crate::mcp::oauth::McpTokenStatus;
|
||||||
use crate::mcp::{
|
use crate::mcp::{
|
||||||
ConnectedServer, JsonField, McpServer, McpTransportType, is_auth_required_error, oauth,
|
ConnectedServer, JsonField, McpAuthReason, McpAuthRequired, McpServer, McpTransportType,
|
||||||
spawn_mcp_server,
|
is_auth_required_error, oauth, spawn_mcp_server,
|
||||||
};
|
};
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::Result;
|
||||||
@@ -102,19 +103,20 @@ impl McpFactory {
|
|||||||
return Ok(existing);
|
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
|
oauth::load_or_refresh_mcp_token(name).await
|
||||||
} else {
|
} 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
|
.await
|
||||||
.map_err(|e| {
|
.map_err(|e| {
|
||||||
if is_auth_required_error(&e) {
|
if is_auth_required_error(&e) {
|
||||||
e.context(format!(
|
e.context(McpAuthRequired {
|
||||||
"MCP server '{name}' requires OAuth authentication. \
|
server: name.to_string(),
|
||||||
Run `coyote --auth-mcp {name}` or `.mcp auth {name}` in the REPL to authenticate."
|
reason: auth_reason,
|
||||||
))
|
})
|
||||||
} else {
|
} else {
|
||||||
e
|
e
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -22,7 +22,7 @@ use crate::function::{
|
|||||||
};
|
};
|
||||||
use crate::mcp::{
|
use crate::mcp::{
|
||||||
MCP_DESCRIBE_META_FUNCTION_NAME_PREFIX, MCP_INVOKE_META_FUNCTION_NAME_PREFIX,
|
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::rag::Rag;
|
||||||
use crate::supervisor::Supervisor;
|
use crate::supervisor::Supervisor;
|
||||||
@@ -3427,7 +3427,11 @@ impl RequestContext {
|
|||||||
{
|
{
|
||||||
Ok(handle) => handles.push((id.clone(), handle)),
|
Ok(handle) => handles.push((id.clone(), handle)),
|
||||||
Err(e) if is_auth_required_error(&e) => {
|
Err(e) if is_auth_required_error(&e) => {
|
||||||
auth_required.push(id.clone())
|
let reason = e
|
||||||
|
.downcast_ref::<McpAuthRequired>()
|
||||||
|
.map(|a| a.reason)
|
||||||
|
.unwrap_or(McpAuthReason::NotAuthenticated);
|
||||||
|
auth_required.push((id.clone(), reason));
|
||||||
}
|
}
|
||||||
Err(e) => return Err(e),
|
Err(e) => return Err(e),
|
||||||
}
|
}
|
||||||
@@ -3444,11 +3448,8 @@ impl RequestContext {
|
|||||||
for (id, handle) in handles {
|
for (id, handle) in handles {
|
||||||
mcp_runtime.insert(id, handle);
|
mcp_runtime.insert(id, handle);
|
||||||
}
|
}
|
||||||
for id in auth_required {
|
for (id, reason) in auth_required {
|
||||||
eprintln!(
|
eprintln!("Warning: {}", McpAuthRequired { server: id, reason });
|
||||||
"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."
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+117
-15
@@ -21,6 +21,8 @@ use rmcp::{RoleClient, ServiceExt};
|
|||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
use sse_transport::LegacySseTransport;
|
use sse_transport::LegacySseTransport;
|
||||||
use std::collections::{HashMap, HashSet};
|
use std::collections::{HashMap, HashSet};
|
||||||
|
use std::fmt;
|
||||||
|
use std::fmt::Display;
|
||||||
use std::fs::OpenOptions;
|
use std::fs::OpenOptions;
|
||||||
use std::path::{Path, PathBuf};
|
use std::path::{Path, PathBuf};
|
||||||
use std::process::Stdio;
|
use std::process::Stdio;
|
||||||
@@ -326,23 +328,29 @@ impl McpRegistry {
|
|||||||
.and_then(|c| c.mcp_servers.get(&id))
|
.and_then(|c| c.mcp_servers.get(&id))
|
||||||
.with_context(|| format!("MCP server not found in config: {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
|
oauth::load_or_refresh_mcp_token(&id).await
|
||||||
} else {
|
} 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 {
|
let service =
|
||||||
Ok(s) => s,
|
match spawn_mcp_server(spec, self.log_path.as_deref(), token_status.into_token()).await
|
||||||
Err(e) if is_auth_required_error(&e) => {
|
{
|
||||||
warn!(
|
Ok(s) => s,
|
||||||
"MCP server '{id}' requires OAuth authentication. \
|
Err(e) if is_auth_required_error(&e) => {
|
||||||
Run `coyote --auth-mcp {id}` or `.mcp auth {id}` in the REPL to authenticate."
|
warn!(
|
||||||
);
|
"{}",
|
||||||
return Ok(None);
|
McpAuthRequired {
|
||||||
}
|
server: id,
|
||||||
Err(e) => return Err(e),
|
reason: auth_reason,
|
||||||
};
|
}
|
||||||
|
);
|
||||||
|
return Ok(None);
|
||||||
|
}
|
||||||
|
Err(e) => return Err(e),
|
||||||
|
};
|
||||||
|
|
||||||
let tools = service.list_tools(None).await?;
|
let tools = service.list_tools(None).await?;
|
||||||
debug!("Available tools for MCP server {id}: {tools:?}");
|
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 {
|
pub(crate) fn is_auth_required_error(e: &Error) -> bool {
|
||||||
e.chain()
|
e.downcast_ref::<McpAuthRequired>().is_some()
|
||||||
.any(|cause| cause.to_string().contains("Auth required"))
|
|| e.chain()
|
||||||
|
.any(|cause| cause.to_string().contains("Auth required"))
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn spawn_http_mcp_server(
|
async fn spawn_http_mcp_server(
|
||||||
@@ -1074,4 +1132,48 @@ mod tests {
|
|||||||
|
|
||||||
assert!(is_auth_required_error(&e));
|
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::<McpAuthRequired>().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"));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
+56
-12
@@ -202,34 +202,54 @@ pub async fn run_mcp_oauth_flow(
|
|||||||
run_oauth_flow(&provider, &mcp_token_key(server_name)).await
|
run_oauth_flow(&provider, &mcp_token_key(server_name)).await
|
||||||
}
|
}
|
||||||
|
|
||||||
pub async fn load_or_refresh_mcp_token(server_name: &str) -> Option<String> {
|
#[derive(Debug, PartialEq, Eq)]
|
||||||
|
pub enum McpTokenStatus {
|
||||||
|
Token(String),
|
||||||
|
NotAuthenticated,
|
||||||
|
RefreshFailed,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl McpTokenStatus {
|
||||||
|
pub fn into_token(self) -> Option<String> {
|
||||||
|
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 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 {
|
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) {
|
if in_refresh_failure_backoff(server_name) {
|
||||||
debug!("Skipping token refresh for MCP server '{server_name}': recent attempt failed");
|
debug!("Skipping token refresh for MCP server '{server_name}': recent attempt failed");
|
||||||
return None;
|
return McpTokenStatus::RefreshFailed;
|
||||||
}
|
}
|
||||||
|
|
||||||
let lock = refresh_lock(server_name);
|
let lock = refresh_lock(server_name);
|
||||||
let _guard = lock.lock().await;
|
let _guard = lock.lock().await;
|
||||||
|
|
||||||
// A concurrent caller may have refreshed while we waited for the lock.
|
// 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 {
|
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) {
|
if in_refresh_failure_backoff(server_name) {
|
||||||
debug!("Skipping token refresh for MCP server '{server_name}': recent attempt failed");
|
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 {
|
match refresh_mcp_token(server_name, &key, &tokens).await {
|
||||||
Ok(access_token) => Some(access_token),
|
Ok(access_token) => McpTokenStatus::Token(access_token),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
note_refresh_failure(server_name);
|
note_refresh_failure(server_name);
|
||||||
warn!(
|
warn!(
|
||||||
@@ -240,7 +260,7 @@ pub async fn load_or_refresh_mcp_token(server_name: &str) -> Option<String> {
|
|||||||
"Token refresh error for MCP server '{server_name}': {}",
|
"Token refresh error for MCP server '{server_name}': {}",
|
||||||
redact_refresh_error(&e)
|
redact_refresh_error(&e)
|
||||||
);
|
);
|
||||||
None
|
McpTokenStatus::RefreshFailed
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -975,7 +995,7 @@ mod tests {
|
|||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[serial]
|
#[serial]
|
||||||
fn expired_token_with_old_format_registration_returns_none() {
|
fn expired_token_with_old_format_registration_reports_refresh_failed() {
|
||||||
with_temp_cache(|| {
|
with_temp_cache(|| {
|
||||||
let dir = paths::oauth_tokens_dir();
|
let dir = paths::oauth_tokens_dir();
|
||||||
fs::create_dir_all(&dir).unwrap();
|
fs::create_dir_all(&dir).unwrap();
|
||||||
@@ -994,12 +1014,36 @@ mod tests {
|
|||||||
.enable_all()
|
.enable_all()
|
||||||
.build()
|
.build()
|
||||||
.unwrap();
|
.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]
|
#[test]
|
||||||
fn refresh_failure_backoff_memoizes_per_server() {
|
fn refresh_failure_backoff_memoizes_per_server() {
|
||||||
assert!(!in_refresh_failure_backoff("backoff-test-server"));
|
assert!(!in_refresh_failure_backoff("backoff-test-server"));
|
||||||
|
|||||||
Reference in New Issue
Block a user