mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(buzz-acp): use shlex for MCP command split and add fallback tests
F1: Replace `split_whitespace()` with `shlex::split()` in `configured_mcp_server` so quoted paths with internal spaces (e.g. `uv run "/My Bot/jambot"`) survive the split. On unmatched quotes, fall back to verbatim command — D1 surfaces the spawn failure as a warning instead of a dead agent. Added `shlex` 1.x as a direct dep (already in Cargo.lock as a transitive dep). F2: Add focused regression tests for `create_session_and_apply_model` MCP fallback path — scripted AcpClient returns an MCP-flavored error on first session/new, success on retry with empty mcpServers. Also covers non-MCP AgentError propagation (no retry triggered).
This commit is contained in:
Will Pfleger
Generated
+1
@@ -751,6 +751,7 @@ dependencies = [
|
||||
"serde",
|
||||
"serde_json",
|
||||
"sha2 0.11.0",
|
||||
"shlex",
|
||||
"thiserror 2.0.18",
|
||||
"tokio",
|
||||
"tokio-tungstenite 0.29.0",
|
||||
|
||||
@@ -71,6 +71,9 @@ toml = "1.0"
|
||||
# Filter expressions
|
||||
evalexpr = { workspace = true }
|
||||
|
||||
# Shell-word splitting (MCP server command field)
|
||||
shlex = "1"
|
||||
|
||||
# Process-group kill (safe wrapper around killpg) — Unix-only; kill_process_group
|
||||
# has a #[cfg(not(unix))] fallback in acp.rs.
|
||||
[target.'cfg(unix)'.dependencies]
|
||||
|
||||
@@ -3454,12 +3454,17 @@ fn build_mcp_servers(config: &Config) -> Vec<McpServer> {
|
||||
}
|
||||
|
||||
fn configured_mcp_server(server: &config::ConfiguredMcpServer) -> McpServer {
|
||||
// Shell-split: users may type `uv run /path/to/jambot` as the command.
|
||||
// First whitespace-delimited token is the executable; remaining tokens
|
||||
// are prepended before `server.args`.
|
||||
let mut parts = server.command.split_whitespace();
|
||||
let command = parts.next().unwrap_or_default().to_string();
|
||||
let prefix_args: Vec<String> = parts.map(String::from).collect();
|
||||
// Shell-word split: users may type `uv run "/My Bot/jambot"` as the command.
|
||||
// shlex handles quoting so paths with spaces survive. On parse failure
|
||||
// (unmatched quote), pass the whole string as the command verbatim — the
|
||||
// spawn will fail, and D1 surfaces MCP spawn errors as warnings.
|
||||
let (command, prefix_args) = match shlex::split(&server.command) {
|
||||
Some(mut tokens) if !tokens.is_empty() => {
|
||||
let cmd = tokens.remove(0);
|
||||
(cmd, tokens)
|
||||
}
|
||||
_ => (server.command.clone(), vec![]),
|
||||
};
|
||||
|
||||
let mut args = prefix_args;
|
||||
args.extend(server.args.iter().cloned());
|
||||
@@ -4110,6 +4115,39 @@ mod build_mcp_servers_tests {
|
||||
assert_eq!(result.command, "npx");
|
||||
assert_eq!(result.args, vec!["github-mcp"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_mcp_server_quoted_path_with_spaces() {
|
||||
let server = config::ConfiguredMcpServer {
|
||||
name: "jambot".into(),
|
||||
command: r#"uv run "/Users/me/My Bot/.venv/bin/jambot""#.into(),
|
||||
args: vec!["--port".into(), "8080".into()],
|
||||
env: vec![],
|
||||
};
|
||||
|
||||
let result = super::configured_mcp_server(&server);
|
||||
assert_eq!(result.command, "uv");
|
||||
assert_eq!(
|
||||
result.args,
|
||||
vec!["run", "/Users/me/My Bot/.venv/bin/jambot", "--port", "8080"],
|
||||
"quoted path must preserve internal spaces and strip quotes"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn configured_mcp_server_unmatched_quote_falls_back_to_verbatim() {
|
||||
let server = config::ConfiguredMcpServer {
|
||||
name: "broken".into(),
|
||||
command: r#"uv run "/unclosed/path"#.into(),
|
||||
args: vec!["--flag".into()],
|
||||
env: vec![],
|
||||
};
|
||||
|
||||
let result = super::configured_mcp_server(&server);
|
||||
// Unmatched quote → shlex returns None → verbatim fallback
|
||||
assert_eq!(result.command, r#"uv run "/unclosed/path"#);
|
||||
assert_eq!(result.args, vec!["--flag"]);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -4900,4 +4900,190 @@ mod tests {
|
||||
"timestamp must not use +00:00 offset"
|
||||
);
|
||||
}
|
||||
|
||||
// ── MCP fallback (D1) regression tests ───────────────────────────────────
|
||||
|
||||
use crate::acp::{AcpClient, McpServer};
|
||||
use crate::relay::RestClient;
|
||||
|
||||
/// Spawn a bash script as an AcpClient, matching the acp.rs test helper.
|
||||
async fn spawn_script(script: &str) -> AcpClient {
|
||||
AcpClient::spawn("bash", &["-c".into(), script.into()], &[], false)
|
||||
.await
|
||||
.expect("failed to spawn test script")
|
||||
}
|
||||
|
||||
/// Build a minimal `PromptContext` with the given MCP servers.
|
||||
/// Only `mcp_servers` and `cwd` are exercised by `create_session_and_apply_model`;
|
||||
/// other fields are inert defaults.
|
||||
fn test_prompt_context(mcp_servers: Vec<McpServer>) -> PromptContext {
|
||||
let keys = Keys::generate();
|
||||
PromptContext {
|
||||
mcp_servers,
|
||||
initial_message: None,
|
||||
idle_timeout: Duration::from_secs(60),
|
||||
max_turn_duration: Duration::from_secs(60),
|
||||
turn_liveness_interval: Duration::ZERO,
|
||||
dedup_mode: crate::config::DedupMode::Drop,
|
||||
system_prompt: None,
|
||||
heartbeat_prompt: None,
|
||||
base_prompt: None,
|
||||
cwd: "/tmp".to_string(),
|
||||
rest_client: RestClient {
|
||||
http: reqwest::Client::new(),
|
||||
base_url: String::new(),
|
||||
keys: keys.clone(),
|
||||
auth_tag_json: None,
|
||||
},
|
||||
channel_info: std::collections::HashMap::new(),
|
||||
context_message_limit: 0,
|
||||
max_turns_per_session: 0,
|
||||
permission_mode: crate::config::PermissionMode::Default,
|
||||
agent_keys: keys,
|
||||
agent_owner_pubkey: None,
|
||||
memory_enabled: false,
|
||||
harness_name: "test".to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Build a minimal `OwnedAgent` from a pre-initialized `AcpClient`.
|
||||
fn test_owned_agent(acp: AcpClient) -> OwnedAgent {
|
||||
OwnedAgent {
|
||||
index: 0,
|
||||
acp,
|
||||
state: SessionState {
|
||||
sessions: HashMap::new(),
|
||||
heartbeat_session: None,
|
||||
turn_counts: HashMap::new(),
|
||||
heartbeat_turn_count: 0,
|
||||
core_sections: HashMap::new(),
|
||||
canvas_sections: HashMap::new(),
|
||||
},
|
||||
model_capabilities: None,
|
||||
desired_model: None,
|
||||
model_overridden: false,
|
||||
protocol_version: 1,
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_mcp_spawn_error_retries_without_mcp_servers() {
|
||||
// Script: respond to initialize, then return an MCP error on the first
|
||||
// session/new, and a valid session on the second (which should carry
|
||||
// empty mcpServers).
|
||||
let script = r#"
|
||||
read -t 2 _init
|
||||
echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}'
|
||||
read -t 2 REQ1
|
||||
echo '{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"mcp: spawn jambot: No such file or directory"}}'
|
||||
read -t 2 REQ2
|
||||
echo '{"jsonrpc":"2.0","id":2,"result":{"sessionId":"ses_fallback","_retryRequest":'"$REQ2"'}}'
|
||||
sleep 1
|
||||
"#;
|
||||
let mut client = spawn_script(script).await;
|
||||
client
|
||||
.initialize()
|
||||
.await
|
||||
.expect("initialize should succeed");
|
||||
|
||||
let mut agent = test_owned_agent(client);
|
||||
let ctx = test_prompt_context(vec![McpServer {
|
||||
name: "jambot".into(),
|
||||
command: "jambot".into(),
|
||||
args: vec![],
|
||||
env: vec![],
|
||||
}]);
|
||||
|
||||
let result = create_session_and_apply_model(&mut agent, &ctx, None, None).await;
|
||||
let session_id = result.expect("fallback session should succeed");
|
||||
assert_eq!(session_id, "ses_fallback");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_mcp_retry_sends_empty_mcp_servers() {
|
||||
// Capture the retry request to a temp file and assert mcpServers: [].
|
||||
let capture_file =
|
||||
std::env::temp_dir().join(format!("buzz_mcp_retry_test_{}.json", std::process::id()));
|
||||
let capture_path = capture_file.display().to_string();
|
||||
let script = format!(
|
||||
r#"
|
||||
read -t 2 _init
|
||||
echo '{{"jsonrpc":"2.0","id":0,"result":{{"protocolVersion":1,"agentCapabilities":{{}}}}}}'
|
||||
read -t 2 _req1
|
||||
echo '{{"jsonrpc":"2.0","id":1,"error":{{"code":-32000,"message":"mcp: spawn failed"}}}}'
|
||||
read -t 2 REQ2
|
||||
echo "$REQ2" > {capture_path}
|
||||
echo '{{"jsonrpc":"2.0","id":2,"result":{{"sessionId":"ses_ok"}}}}'
|
||||
sleep 1
|
||||
"#
|
||||
);
|
||||
let mut client = spawn_script(&script).await;
|
||||
client
|
||||
.initialize()
|
||||
.await
|
||||
.expect("initialize should succeed");
|
||||
|
||||
let mut agent = test_owned_agent(client);
|
||||
let ctx = test_prompt_context(vec![McpServer {
|
||||
name: "broken".into(),
|
||||
command: "broken-server".into(),
|
||||
args: vec![],
|
||||
env: vec![],
|
||||
}]);
|
||||
|
||||
let result = create_session_and_apply_model(&mut agent, &ctx, None, None).await;
|
||||
let session_id = result.expect("retry should succeed");
|
||||
assert_eq!(session_id, "ses_ok");
|
||||
|
||||
// Read the captured retry request and verify mcpServers is empty.
|
||||
let captured = std::fs::read_to_string(&capture_file)
|
||||
.expect("retry request should have been written to capture file");
|
||||
std::fs::remove_file(&capture_file).ok();
|
||||
let parsed: serde_json::Value =
|
||||
serde_json::from_str(captured.trim()).expect("captured request should be valid JSON");
|
||||
let mcp_servers = parsed
|
||||
.pointer("/params/mcpServers")
|
||||
.expect("retry request must contain params.mcpServers");
|
||||
assert_eq!(
|
||||
mcp_servers,
|
||||
&serde_json::json!([]),
|
||||
"retry must send empty mcpServers"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_non_mcp_agent_error_propagates() {
|
||||
// Non-MCP AgentError must NOT trigger the retry — it should propagate.
|
||||
let script = r#"
|
||||
read -t 2 _init
|
||||
echo '{"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentCapabilities":{}}}'
|
||||
read -t 2 _req
|
||||
echo '{"jsonrpc":"2.0","id":1,"error":{"code":-32000,"message":"authentication failed"}}'
|
||||
sleep 1
|
||||
"#;
|
||||
let mut client = spawn_script(script).await;
|
||||
client
|
||||
.initialize()
|
||||
.await
|
||||
.expect("initialize should succeed");
|
||||
|
||||
let mut agent = test_owned_agent(client);
|
||||
let ctx = test_prompt_context(vec![McpServer {
|
||||
name: "server".into(),
|
||||
command: "some-server".into(),
|
||||
args: vec![],
|
||||
env: vec![],
|
||||
}]);
|
||||
|
||||
let result = create_session_and_apply_model(&mut agent, &ctx, None, None).await;
|
||||
assert!(
|
||||
result.is_err(),
|
||||
"non-MCP error must propagate, not trigger retry"
|
||||
);
|
||||
let err = result.unwrap_err();
|
||||
assert!(
|
||||
err.to_string().contains("authentication failed"),
|
||||
"error message must be preserved: {err}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
+8
-8
@@ -708,10 +708,10 @@ packages:
|
||||
dependency: transitive
|
||||
description:
|
||||
name: meta
|
||||
sha256: "23f08335362185a5ea2ad3a4e597f1375e78bce8a040df5c600c8d3552ef2394"
|
||||
sha256: "1741988757a65eb6b36abe716829688cf01910bbf91c34354ff7ec1c3de2b349"
|
||||
url: "https://pub.dev"
|
||||
source: hosted
|
||||
version: "1.17.0"
|
||||
version: "1.18.0"
|
||||
mime:
|
||||
dependency: transitive
|
||||
description:
|
||||
@@ -1137,26 +1137,26 @@ packages:
|
||||
dependency: transitive
|
||||
description:
|
||||
name: test
|
||||
sha256: "280d6d890011ca966ad08df7e8a4ddfab0fb3aa49f96ed6de56e3521347a9ae7"
|
||||
sha256: "8d9ceddbab833f180fbefed08afa76d7c03513dfdba87ffcec2718b02bbcbf20"
|
||||
url: "https://pub.dev"
|
||||
source: hosted
|
||||
version: "1.30.0"
|
||||
version: "1.31.0"
|
||||
test_api:
|
||||
dependency: transitive
|
||||
description:
|
||||
name: test_api
|
||||
sha256: "8161c84903fd860b26bfdefb7963b3f0b68fee7adea0f59ef805ecca346f0c7a"
|
||||
sha256: "949a932224383300f01be9221c39180316445ecb8e7547f70a41a35bf421fb9e"
|
||||
url: "https://pub.dev"
|
||||
source: hosted
|
||||
version: "0.7.10"
|
||||
version: "0.7.11"
|
||||
test_core:
|
||||
dependency: transitive
|
||||
description:
|
||||
name: test_core
|
||||
sha256: "0381bd1585d1a924763c308100f2138205252fb90c9d4eeaf28489ee65ccde51"
|
||||
sha256: "1991d4cfe85d5043241acac92962c3977c8d2f2add1ee73130c7b286417d1d34"
|
||||
url: "https://pub.dev"
|
||||
source: hosted
|
||||
version: "0.6.16"
|
||||
version: "0.6.17"
|
||||
tuple:
|
||||
dependency: transitive
|
||||
description:
|
||||
|
||||
Reference in New Issue
Block a user