diff --git a/apps/cadcraft-cli/src/main.rs b/apps/cadcraft-cli/src/main.rs index 73f5477..3f1ba80 100644 --- a/apps/cadcraft-cli/src/main.rs +++ b/apps/cadcraft-cli/src/main.rs @@ -264,6 +264,88 @@ fn perf(args: &[String]) -> Result<(), String> { Ok(()) } +/// The backend `cadcraft-cli mcp` serves from. +#[derive(Debug, PartialEq, Eq)] +enum McpTarget { + /// An in-process drawing session (no `--connect`). + Headless, + /// The running app's control channel at `HOST:PORT`. + Connect(String), +} + +/// Parses the arguments after `mcp`. `--connect` must be followed by a `HOST:PORT`; a missing, +/// empty or flag-like value is an error, never a silent fall back to a headless session (#103). +fn parse_mcp_args(args: &[String]) -> Result { + let mut target = McpTarget::Headless; + let mut it = args.iter(); + while let Some(a) = it.next() { + match a.as_str() { + "--connect" => { + let addr = it + .next() + .map(|v| v.trim()) + .filter(|v| !v.is_empty() && !v.starts_with('-')) + .ok_or("--connect needs HOST:PORT (the app's --control port, e.g. 127.0.0.1:7979)")?; + if target != McpTarget::Headless { + return Err("--connect given more than once".into()); + } + target = McpTarget::Connect(addr.to_string()); + } + other => return Err(format!("mcp: unknown argument {other}\n{USAGE}")), + } + } + Ok(target) +} + +fn mcp(args: &[String]) -> Result<(), String> { + let backend: Box = match parse_mcp_args(args)? { + McpTarget::Connect(addr) => match cadcraft_mcp::Remote::connect(&addr) { + Ok(r) => Box::new(r), + Err(e) => return Err(format!("cadcraft-cli: cannot connect to {addr}: {e}")), + }, + McpTarget::Headless => Box::new(cadcraft_mcp::Headless::default()), + }; + let stdin = std::io::stdin(); + let stdout = std::io::stdout(); + cadcraft_mcp::Server::new(backend).serve(stdin.lock(), stdout.lock()).map_err(|e| e.to_string()) +} + +#[cfg(test)] +mod mcp_args_tests { + use super::*; + + fn args(a: &[&str]) -> Vec { + a.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn mcp_without_connect_is_headless() { + assert_eq!(parse_mcp_args(&args(&[])), Ok(McpTarget::Headless)); + } + + #[test] + fn mcp_connect_with_address_selects_remote() { + assert_eq!(parse_mcp_args(&args(&["--connect", "127.0.0.1:7979"])), Ok(McpTarget::Connect("127.0.0.1:7979".into()))); + assert_eq!(parse_mcp_args(&args(&["--connect", "localhost:7979"])), Ok(McpTarget::Connect("localhost:7979".into()))); + } + + #[test] + fn mcp_connect_without_address_is_an_error_not_headless() { + for bad in [&["--connect"][..], &["--connect", ""], &["--connect", " "], &["--connect", "--foo"], &["--connect", "-v"]] { + let r = parse_mcp_args(&args(bad)); + let Err(e) = r else { panic!("{bad:?} should be rejected, got {r:?}") }; + assert!(e.contains("--connect needs HOST:PORT"), "{bad:?}: {e}"); + } + } + + #[test] + fn mcp_rejects_unknown_and_repeated_arguments() { + assert!(parse_mcp_args(&args(&["--conect", "127.0.0.1:7979"])).is_err()); + assert!(parse_mcp_args(&args(&["127.0.0.1:7979"])).is_err()); + assert!(parse_mcp_args(&args(&["--connect", "127.0.0.1:1", "--connect", "127.0.0.1:2"])).is_err()); + } +} + fn main() -> ExitCode { install_io(); let args: Vec = std::env::args().skip(1).collect(); @@ -291,21 +373,7 @@ fn main() -> ExitCode { println!("{}", serde_json::to_string_pretty(&v).unwrap_or_default()); Ok(()) } - Some("mcp") => { - let backend: Box = match rest.iter().position(|a| a == "--connect").and_then(|i| rest.get(i + 1)) { - Some(addr) => match cadcraft_mcp::Remote::connect(addr) { - Ok(r) => Box::new(r), - Err(e) => { - eprintln!("cadcraft-cli: cannot connect to {addr}: {e}"); - return ExitCode::FAILURE; - } - }, - None => Box::new(cadcraft_mcp::Headless::default()), - }; - let stdin = std::io::stdin(); - let stdout = std::io::stdout(); - cadcraft_mcp::Server::new(backend).serve(stdin.lock(), stdout.lock()).map_err(|e| e.to_string()) - } + Some("mcp") => mcp(&rest), Some("--version" | "-V") => { println!("cadcraft-cli {}", env!("CARGO_PKG_VERSION")); Ok(()) diff --git a/apps/cadcraft-cli/tests/mcp_connect.rs b/apps/cadcraft-cli/tests/mcp_connect.rs new file mode 100644 index 0000000..c12e71c --- /dev/null +++ b/apps/cadcraft-cli/tests/mcp_connect.rs @@ -0,0 +1,93 @@ +//! `cadcraft-cli mcp --connect` end to end: a missing address must fail startup instead of +//! serving drawing edits from a headless session (#103); a real address still bridges. + +use std::io::{BufRead, BufReader, Write}; +use std::net::TcpListener; +use std::process::{Command, Output, Stdio}; + +use serde_json::{Value, json}; + +const INIT: &str = r#"{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18"}}"#; +const CIRCLE: &str = r#"{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"execute","arguments":{"command":"circle","params":{"center":[5,5],"radius":2}}}}"#; + +fn mcp(args: &[&str], input: &str) -> Output { + let mut child = Command::new(env!("CARGO_BIN_EXE_cadcraft-cli")) + .arg("mcp") + .args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn cadcraft-cli"); + if let Some(mut stdin) = child.stdin.take() { + // The process may exit before reading stdin (that is the point); ignore a broken pipe. + let _ = stdin.write_all(input.as_bytes()); + } + child.wait_with_output().expect("wait for cadcraft-cli") +} + +fn replies(out: &Output) -> Vec { + String::from_utf8_lossy(&out.stdout).lines().filter_map(|l| serde_json::from_str(l).ok()).collect() +} + +#[test] +fn connect_without_address_fails_before_serving() { + let input = format!("{INIT}\n{CIRCLE}\n"); + for args in [&["--connect"][..], &["--connect", ""], &["--connect", "--foo"]] { + let out = mcp(args, &input); + assert!(!out.status.success(), "{args:?} must exit non-zero"); + assert!(out.stdout.is_empty(), "{args:?} must not answer MCP requests: {}", String::from_utf8_lossy(&out.stdout)); + let err = String::from_utf8_lossy(&out.stderr); + assert!(err.contains("--connect needs HOST:PORT"), "{args:?}: {err}"); + } +} + +#[test] +fn no_connect_still_serves_headless() { + let out = mcp(&[], &format!("{INIT}\n{CIRCLE}\n")); + assert!(out.status.success(), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + let r = replies(&out); + let instructions = r.iter().find(|v| v["id"] == 1).and_then(|v| v["result"]["instructions"].as_str()).unwrap_or_default(); + assert!(instructions.contains("Backend: headless"), "{instructions}"); + let circle = r.iter().find(|v| v["id"] == 2).map(|v| &v["result"]); + assert!(circle.is_some_and(|c| c.is_object() && c.get("isError").is_none()), "{r:?}"); +} + +#[test] +fn connect_with_address_routes_to_that_endpoint() { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind loopback"); + let addr = listener.local_addr().expect("local addr").to_string(); + let server = std::thread::spawn(move || { + let (conn, _) = listener.accept().expect("accept"); + let mut reader = BufReader::new(conn.try_clone().expect("clone")); + let mut line = String::new(); + reader.read_line(&mut line).expect("read request"); + let req: Value = serde_json::from_str(&line).expect("json request"); + let reply = json!({"id": req["id"], "ok": true, "result": {"fixture": "loopback"}}); + let mut w = conn; + writeln!(w, "{reply}").expect("write reply"); + req + }); + let call = r#"{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"ui_inspect","arguments":{}}}"#; + let out = mcp(&["--connect", &addr], &format!("{INIT}\n{call}\n")); + assert!(out.status.success(), "stderr: {}", String::from_utf8_lossy(&out.stderr)); + let req = server.join().expect("server thread"); + assert_eq!(req["method"], "ui.inspect"); + let r = replies(&out); + let instructions = r.iter().find(|v| v["id"] == 1).and_then(|v| v["result"]["instructions"].as_str()).unwrap_or_default(); + assert!(instructions.contains(&format!("connected to CADCraft at {addr}")), "{instructions}"); + let text = r.iter().find(|v| v["id"] == 2).and_then(|v| v["result"]["content"][0]["text"].as_str()).unwrap_or_default(); + assert!(text.contains("loopback"), "{r:?}"); +} + +#[test] +fn connect_to_closed_port_fails() { + let addr = { + let l = TcpListener::bind("127.0.0.1:0").expect("bind loopback"); + l.local_addr().expect("local addr").to_string() + }; + let out = mcp(&["--connect", &addr], &format!("{INIT}\n")); + assert!(!out.status.success()); + assert!(out.stdout.is_empty()); + assert!(String::from_utf8_lossy(&out.stderr).contains("cannot connect to")); +} diff --git a/docs/mcp.md b/docs/mcp.md index ade22dd..c383854 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -13,6 +13,10 @@ cadcraft --control 7979 & cadcraft-cli mcp --connect 127.0.0.1:7979 ``` +`--connect` needs the app's `HOST:PORT`. Without one (or with an unknown argument) `cadcraft-cli mcp` +exits with an error instead of falling back to a headless session, so a client meant for the app +never edits a separate drawing by mistake. If the app can't be reached, startup fails too. + Example client configuration (Claude Code / Claude Desktop): ```json