cadcraft-cli mcp: --connect without HOST:PORT is an error, not a headless session (fixes #103) (#167)
* cadcraft-cli mcp: --connect without HOST:PORT is an error, not a headless session (#103) `mcp --connect` with no address (or an empty or flag-like one) fell through to the headless backend, so a client meant for the app got successful edits in a separate drawing. The mcp arguments now go through parse_mcp_args: --connect must be followed by a non-empty value that doesn't start with `-`, unknown arguments and a repeated --connect are rejected, and every error exits non-zero before any MCP request is served. Plain `mcp` is still headless; a real address still connects, and an unreachable one still fails. - Unit tests for the parser; integration tests (tests/mcp_connect.rs) run the binary: missing/empty/flag-like address, headless control, loopback endpoint routing, closed port. - docs/mcp.md says what happens without an address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Move tests away from other PRs' hunks to avoid merge conflicts Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Valentin Klein
GitHub
parent
6c145ed733
commit
bcbbc5e8a4
|
|
@ -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<McpTarget, String> {
|
||||
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<dyn cadcraft_mcp::Backend> = 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<String> {
|
||||
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<String> = 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<dyn cadcraft_mcp::Backend> = 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(())
|
||||
|
|
|
|||
|
|
@ -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<Value> {
|
||||
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"));
|
||||
}
|
||||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue