fix: hot-attach to MCP servers that require auth after running .mcp auth <name>
This commit is contained in:
@@ -3,7 +3,7 @@ use crate::mcp::{
|
|||||||
spawn_mcp_server,
|
spawn_mcp_server,
|
||||||
};
|
};
|
||||||
|
|
||||||
use anyhow::{Result, anyhow};
|
use anyhow::Result;
|
||||||
use parking_lot::Mutex;
|
use parking_lot::Mutex;
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
@@ -111,10 +111,10 @@ impl McpFactory {
|
|||||||
.await
|
.await
|
||||||
.map_err(|e| {
|
.map_err(|e| {
|
||||||
if is_auth_required_error(&e) {
|
if is_auth_required_error(&e) {
|
||||||
anyhow!(
|
e.context(format!(
|
||||||
"MCP server '{name}' requires OAuth authentication. \
|
"MCP server '{name}' requires OAuth authentication. \
|
||||||
Run `coyote --auth-mcp {name}` or `.mcp auth {name}` in the REPL to authenticate."
|
Run `coyote --auth-mcp {name}` or `.mcp auth {name}` in the REPL to authenticate."
|
||||||
)
|
))
|
||||||
} else {
|
} else {
|
||||||
e
|
e
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -21,7 +21,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,
|
MCP_SEARCH_META_FUNCTION_NAME_PREFIX, is_auth_required_error,
|
||||||
};
|
};
|
||||||
use crate::rag::Rag;
|
use crate::rag::Rag;
|
||||||
use crate::supervisor::Supervisor;
|
use crate::supervisor::Supervisor;
|
||||||
@@ -3161,18 +3161,25 @@ impl RequestContext {
|
|||||||
let app_ref = &self.app;
|
let app_ref = &self.app;
|
||||||
let acquire_all = async {
|
let acquire_all = async {
|
||||||
let mut handles = Vec::new();
|
let mut handles = Vec::new();
|
||||||
|
let mut auth_required = Vec::new();
|
||||||
for id in &server_ids {
|
for id in &server_ids {
|
||||||
if let Some(spec) = mcp_config.mcp_servers.get(id) {
|
if let Some(spec) = mcp_config.mcp_servers.get(id) {
|
||||||
let handle = app_ref
|
match app_ref
|
||||||
.mcp_factory
|
.mcp_factory
|
||||||
.acquire(id, spec, app_ref.mcp_log_path.as_deref())
|
.acquire(id, spec, app_ref.mcp_log_path.as_deref())
|
||||||
.await?;
|
.await
|
||||||
handles.push((id.clone(), handle));
|
{
|
||||||
|
Ok(handle) => handles.push((id.clone(), handle)),
|
||||||
|
Err(e) if is_auth_required_error(&e) => {
|
||||||
|
auth_required.push(id.clone())
|
||||||
|
}
|
||||||
|
Err(e) => return Err(e),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok::<_, Error>(handles)
|
Ok::<_, Error>((handles, auth_required))
|
||||||
};
|
};
|
||||||
let handles = abortable_run_with_spinner(
|
let (handles, auth_required) = abortable_run_with_spinner(
|
||||||
acquire_all,
|
acquire_all,
|
||||||
"Loading MCP servers",
|
"Loading MCP servers",
|
||||||
abort_signal.clone(),
|
abort_signal.clone(),
|
||||||
@@ -3181,6 +3188,12 @@ 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 {
|
||||||
|
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."
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -3281,9 +3294,18 @@ impl RequestContext {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
let prev_role = self.role.clone();
|
||||||
|
let prev_session = self.session.clone();
|
||||||
self.use_role_obj(role)?;
|
self.use_role_obj(role)?;
|
||||||
self.rebuild_tool_scope(app, mcp_servers, abort_signal)
|
if let Err(e) = self
|
||||||
|
.rebuild_tool_scope(app, mcp_servers, abort_signal)
|
||||||
.await
|
.await
|
||||||
|
{
|
||||||
|
self.role = prev_role;
|
||||||
|
self.session = prev_session;
|
||||||
|
return Err(e);
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
pub async fn use_session(
|
pub async fn use_session(
|
||||||
@@ -4311,6 +4333,14 @@ mod tests {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn app_state_with_mcp_config(mcp_server_support: bool, server_names: &[&str]) -> Arc<AppState> {
|
fn app_state_with_mcp_config(mcp_server_support: bool, server_names: &[&str]) -> Arc<AppState> {
|
||||||
|
app_state_with_mcp_command(mcp_server_support, server_names, "echo")
|
||||||
|
}
|
||||||
|
|
||||||
|
fn app_state_with_mcp_command(
|
||||||
|
mcp_server_support: bool,
|
||||||
|
server_names: &[&str],
|
||||||
|
command: &str,
|
||||||
|
) -> Arc<AppState> {
|
||||||
let app_config = AppConfig {
|
let app_config = AppConfig {
|
||||||
mcp_server_support,
|
mcp_server_support,
|
||||||
..AppConfig::default()
|
..AppConfig::default()
|
||||||
@@ -4325,7 +4355,7 @@ mod tests {
|
|||||||
name.to_string(),
|
name.to_string(),
|
||||||
McpServer {
|
McpServer {
|
||||||
transport_type: McpTransportType::Stdio,
|
transport_type: McpTransportType::Stdio,
|
||||||
command: Some("echo".to_string()),
|
command: Some(command.to_string()),
|
||||||
args: None,
|
args: None,
|
||||||
env: None,
|
env: None,
|
||||||
cwd: None,
|
cwd: None,
|
||||||
@@ -4374,6 +4404,33 @@ mod tests {
|
|||||||
assert!(ctx.tool_scope.mcp_runtime.is_empty());
|
assert!(ctx.tool_scope.mcp_runtime.is_empty());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
#[serial]
|
||||||
|
fn use_role_rolls_back_when_mcp_startup_fails() {
|
||||||
|
let _guard = TestConfigDirGuard::new();
|
||||||
|
let roles_dir = paths::roles_dir();
|
||||||
|
create_dir_all(&roles_dir).unwrap();
|
||||||
|
write(
|
||||||
|
roles_dir.join("broken_mcp.md"),
|
||||||
|
"---\nenabled_mcp_servers: failing\n---\nYou use MCP servers.",
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let app_state =
|
||||||
|
app_state_with_mcp_command(true, &["failing"], "/nonexistent/coyote-test-mcp-binary");
|
||||||
|
let mut ctx = RequestContext::new(app_state, WorkingMode::Cmd);
|
||||||
|
let app = ctx.app.config.clone();
|
||||||
|
let abort = utils::create_abort_signal();
|
||||||
|
|
||||||
|
let result = run_async(ctx.use_role(&app, "broken_mcp", abort));
|
||||||
|
|
||||||
|
assert!(result.is_err());
|
||||||
|
assert!(
|
||||||
|
ctx.role.is_none(),
|
||||||
|
"role must be rolled back when MCP startup fails"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
#[serial]
|
#[serial]
|
||||||
fn rebuild_tool_scope_no_enabled_servers_yields_empty_runtime() {
|
fn rebuild_tool_scope_no_enabled_servers_yields_empty_runtime() {
|
||||||
|
|||||||
@@ -1061,4 +1061,14 @@ mod tests {
|
|||||||
|
|
||||||
assert!(!is_auth_required_error(&e));
|
assert!(!is_auth_required_error(&e));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn is_auth_required_error_survives_context_wrapping() {
|
||||||
|
let e = anyhow!("Auth required, when send initialize request").context(
|
||||||
|
"MCP server 'github' requires OAuth authentication. \
|
||||||
|
Run `coyote --auth-mcp github` or `.mcp auth github` in the REPL to authenticate.",
|
||||||
|
);
|
||||||
|
|
||||||
|
assert!(is_auth_required_error(&e));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -698,6 +698,26 @@ pub async fn run_repl_command(
|
|||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
println!("Authentication saved.");
|
println!("Authentication saved.");
|
||||||
|
if ctx.app.config.mcp_server_support {
|
||||||
|
let app = Arc::clone(&ctx.app.config);
|
||||||
|
ctx.bootstrap_tools(
|
||||||
|
app.as_ref(),
|
||||||
|
true,
|
||||||
|
abort_signal.clone(),
|
||||||
|
)
|
||||||
|
.await?;
|
||||||
|
if ctx.tool_scope.mcp_runtime.get(server_name).is_some()
|
||||||
|
{
|
||||||
|
println!(
|
||||||
|
"✓ MCP server '{server_name}' started and attached to the current context."
|
||||||
|
);
|
||||||
|
} else {
|
||||||
|
println!(
|
||||||
|
"MCP server '{server_name}' is not enabled in the current context. \
|
||||||
|
Run `.mcp enable {server_name}` to attach it."
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user