mirror of
https://github.com/hanzoai/dev.git
synced 2026-10-17 15:34:54 +00:00
fix(mcp): validate stdio command on PATH; improve spawn error context. Fixes #302
This commit is contained in:
@@ -61,6 +61,7 @@ use toml_edit::ArrayOfTables as TomlArrayOfTables;
|
||||
use toml_edit::DocumentMut;
|
||||
use toml_edit::Item as TomlItem;
|
||||
use toml_edit::Table as TomlTable;
|
||||
use which::which;
|
||||
|
||||
const OPENAI_DEFAULT_MODEL: &str = "gpt-5-codex";
|
||||
const OPENAI_DEFAULT_REVIEW_MODEL: &str = "gpt-5-codex";
|
||||
@@ -386,10 +387,27 @@ pub fn load_global_mcp_servers(
|
||||
return Ok(BTreeMap::new());
|
||||
};
|
||||
|
||||
servers_value
|
||||
let servers: BTreeMap<String, McpServerConfig> = servers_value
|
||||
.clone()
|
||||
.try_into()
|
||||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))
|
||||
.map_err(|e| std::io::Error::new(std::io::ErrorKind::InvalidData, e))?;
|
||||
|
||||
for (name, cfg) in &servers {
|
||||
if let McpServerTransportConfig::Stdio { command, .. } = &cfg.transport {
|
||||
let command_looks_like_path = {
|
||||
let path = Path::new(command);
|
||||
path.components().count() > 1 || path.is_absolute()
|
||||
};
|
||||
if !command_looks_like_path && which(command).is_err() {
|
||||
let msg = format!(
|
||||
"MCP server `{name}` command `{command}` not found on PATH. If the server is an npm package, set command = \"npx\" and keep the package name in args."
|
||||
);
|
||||
return Err(std::io::Error::new(ErrorKind::NotFound, msg));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(servers)
|
||||
}
|
||||
|
||||
pub fn write_global_mcp_servers(
|
||||
@@ -2609,6 +2627,35 @@ exclude_slash_tmp = true
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn load_global_mcp_servers_rejects_missing_command() -> anyhow::Result<()> {
|
||||
let code_home = TempDir::new()?;
|
||||
let config_path = code_home.path().join(CONFIG_TOML_FILE);
|
||||
|
||||
std::fs::write(
|
||||
&config_path,
|
||||
r#"[mcp_servers.context7-mcp]
|
||||
command = "nonexistent-cmd"
|
||||
args = ["-y", "@upstash/context7-mcp"]
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let err = load_global_mcp_servers(code_home.path())
|
||||
.expect_err("missing executables should surface a readable error");
|
||||
let msg = err.to_string();
|
||||
|
||||
assert!(
|
||||
msg.contains("context7-mcp"),
|
||||
"expected server name in error, got: {msg}"
|
||||
);
|
||||
assert!(
|
||||
msg.contains("command = \"npx\""),
|
||||
"expected hint suggesting command = \"npx\", got: {msg}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn persist_model_selection_updates_defaults() -> anyhow::Result<()> {
|
||||
let code_home = TempDir::new()?;
|
||||
|
||||
@@ -218,6 +218,7 @@ impl McpConnectionManager {
|
||||
let use_rmcp_client_flag = use_rmcp_client;
|
||||
join_set.spawn(async move {
|
||||
let McpServerConfig { transport, .. } = cfg;
|
||||
let server_name_for_error = server_name.clone();
|
||||
let params = mcp_types::InitializeRequestParams {
|
||||
capabilities: ClientCapabilities {
|
||||
experimental: None,
|
||||
@@ -241,6 +242,8 @@ impl McpConnectionManager {
|
||||
|
||||
let client = match transport {
|
||||
McpServerTransportConfig::Stdio { command, args, env } => {
|
||||
let command_for_error = command.clone();
|
||||
let args_for_error = args.clone();
|
||||
let command_os: OsString = command.into();
|
||||
let args_os: Vec<OsString> = args.into_iter().map(Into::into).collect();
|
||||
McpClientAdapter::new_stdio_client(
|
||||
@@ -252,6 +255,19 @@ impl McpConnectionManager {
|
||||
startup_timeout,
|
||||
)
|
||||
.await
|
||||
.with_context(|| {
|
||||
if args_for_error.is_empty() {
|
||||
format!(
|
||||
"failed to spawn MCP server `{}` using command `{}`",
|
||||
server_name_for_error, command_for_error
|
||||
)
|
||||
} else {
|
||||
format!(
|
||||
"failed to spawn MCP server `{}` using command `{}` with args {:?}",
|
||||
server_name_for_error, command_for_error, args_for_error
|
||||
)
|
||||
}
|
||||
})
|
||||
}
|
||||
McpServerTransportConfig::StreamableHttp { url, bearer_token } => {
|
||||
McpClientAdapter::new_streamable_http_client(
|
||||
@@ -498,4 +514,36 @@ mod tests {
|
||||
"my_server__yet_another_e1c3987bd9c50b826cbe1687966f79f0c602d19ca"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn stdio_spawn_error_mentions_server_and_command() {
|
||||
let mut servers = HashMap::new();
|
||||
servers.insert(
|
||||
"context7-mcp".to_string(),
|
||||
McpServerConfig {
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: "nonexistent-cmd".to_string(),
|
||||
args: Vec::new(),
|
||||
env: None,
|
||||
},
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
},
|
||||
);
|
||||
|
||||
let (_manager, errors) = McpConnectionManager::new(servers, false, HashSet::new())
|
||||
.await
|
||||
.expect("manager creation should succeed even when servers fail");
|
||||
|
||||
let err = errors
|
||||
.get("context7-mcp")
|
||||
.expect("missing executable should be reported under server name");
|
||||
let msg = format!("{err:#}");
|
||||
|
||||
assert!(msg.contains("context7-mcp"), "error should mention the server name");
|
||||
assert!(
|
||||
msg.contains("nonexistent-cmd"),
|
||||
"error should include the missing command, got: {msg}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user