From 1473893b084698f4f4642684bf542f2fe6a73ece Mon Sep 17 00:00:00 2001 From: Mateusz Sych <151755914+SychPL@users.noreply.github.com> Date: Thu, 15 Oct 2026 08:13:18 +0200 Subject: [PATCH] MCP: a bridged server starts without the app and connects once it runs (#1298) The bridged MCP server starts when no app is listening, says so in its instructions and in each tool error, and connects on the first call after the app starts. A batch that could not be sent says the app is not running. docs/mcp.md describes it. --- apps/solidcraft-cli/src/main.rs | 21 ++++-- apps/solidcraft-cli/tests/mcp_connect.rs | 83 +++++++++++++++++++++--- crates/mcp/src/backend.rs | 80 ++++++++++++++++++----- crates/mcp/src/tests.rs | 57 ++++++++++++++++ crates/mcp/src/tools.rs | 2 + docs/mcp.md | 10 +++ 6 files changed, 221 insertions(+), 32 deletions(-) diff --git a/apps/solidcraft-cli/src/main.rs b/apps/solidcraft-cli/src/main.rs index cf4e5c6d..b3096ff0 100644 --- a/apps/solidcraft-cli/src/main.rs +++ b/apps/solidcraft-cli/src/main.rs @@ -424,13 +424,20 @@ fn cmd_mcp(args: &[String]) -> Result<(), String> { } let backend: Box = match (flag(args, "--connect"), flag(args, "--in")) { (Some(_), Some(_)) => return Err("mcp: use either --connect or --in, not both".into()), - (Some(addr), None) => Box::new(solidcraft_mcp::Remote::connect_with_token(addr, flag(args, "--token")).map_err(|e| { - let port = addr.rsplit(':').next().filter(|p| p.parse::().is_ok()).unwrap_or("PORT"); - format!( - "cannot connect to {addr}: {e}\nno SolidCraft app is listening there; start it with \ - `solidcraft --control {port}` (or SOLIDCRAFT_CONTROL_PORT={port}) and retry" - ) - })?), + (Some(addr), None) => { + let Some(port) = addr.rsplit_once(':').and_then(|(_, p)| p.parse::().ok()).filter(|p| *p != 0) else { + return Err(format!("mcp: --connect wants HOST:PORT, such as 127.0.0.1:7878, not `{addr}`")); + }; + // Serve without the app too: the tools connect once it runs, and say it isn't until then. + let mut remote = solidcraft_mcp::Remote::new(addr, flag(args, "--token")); + if let Err(e) = remote.try_connect() { + eprintln!( + "solidcraft mcp: no SolidCraft app at {addr} yet ({e}); start it with `solidcraft --control {port}` \ + (or SOLIDCRAFT_CONTROL_PORT={port}), tool calls connect once it runs" + ); + } + Box::new(remote) + } (None, Some(path)) => Box::new(solidcraft_mcp::Headless::new(load(path)?)), (None, None) => Box::new(solidcraft_mcp::Headless::default()), }; diff --git a/apps/solidcraft-cli/tests/mcp_connect.rs b/apps/solidcraft-cli/tests/mcp_connect.rs index 92812d32..e384d660 100644 --- a/apps/solidcraft-cli/tests/mcp_connect.rs +++ b/apps/solidcraft-cli/tests/mcp_connect.rs @@ -1,18 +1,83 @@ -//! `mcp --connect` to a port where no app listens tells the user how to start one (#270). +//! `mcp --connect` to a port where no app listens yet (#270): the server still starts, its tools +//! say how to start the app, and once the app runs they reach it with that launch's token. +use std::io::{BufRead, BufReader, Write}; use std::net::TcpListener; use std::process::{Command, Stdio}; +use serde_json::{Value, json}; + #[test] -fn mcp_connect_without_a_listening_app_names_the_control_flag() { +fn mcp_connect_serves_before_the_app_runs_and_reaches_it_once_it_does() { // A port that was free a moment ago: nothing listens on it. let port = TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port(); - let output = Command::new(env!("CARGO_BIN_EXE_solidcraft-cli")) - .args(["mcp", "--connect", &format!("127.0.0.1:{port}")]) - .stdin(Stdio::null()) - .output() + let addr = format!("127.0.0.1:{port}"); + let control_dir = std::env::temp_dir().join(format!("solidcraft-mcp-connect-{}-{port}", std::process::id())); + std::fs::create_dir_all(&control_dir).unwrap(); + let mut child = Command::new(env!("CARGO_BIN_EXE_solidcraft-cli")) + .args(["mcp", "--connect", &addr, "--allow-any-path"]) + .env("SOLIDCRAFT_CONTROL_DIR", &control_dir) + .env_remove("SOLIDCRAFT_CONTROL_TOKEN") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() .unwrap(); - let stderr = String::from_utf8_lossy(&output.stderr); - assert!(!output.status.success(), "{stderr}"); - assert!(stderr.contains("--control"), "say how to start the app with a control port: {stderr}"); + let mut stdin = child.stdin.take().unwrap(); + let mut stdout = BufReader::new(child.stdout.take().unwrap()); + let mut ask = |id: u64, method: &str, params: Value| -> Value { + writeln!(stdin, "{}", json!({"jsonrpc": "2.0", "id": id, "method": method, "params": params})).unwrap(); + let mut line = String::new(); + stdout.read_line(&mut line).unwrap(); + serde_json::from_str(&line).unwrap_or_else(|e| panic!("{e}: {line:?}")) + }; + let measure = json!({"name": "measure", "arguments": {}}); + let text = |r: &Value| r["result"]["content"][0]["text"].as_str().unwrap_or_default().to_string(); + + // The server answers without the app, and its instructions say the app isn't running. + let init = ask(1, "initialize", json!({"protocolVersion": "2025-06-18"})); + let instructions = init["result"]["instructions"].as_str().unwrap_or_default(); + assert!(instructions.contains("not running") && instructions.contains(&format!("--control {port}")), "{instructions}"); + let tools = ask(2, "tools/list", json!({})); + assert!(tools["result"]["tools"].as_array().is_some_and(|t| !t.is_empty()), "{tools}"); + + // A tool call says the app is not running and how to start it. + let r = ask(3, "tools/call", measure.clone()); + assert_eq!(r["result"]["isError"], true, "{r}"); + assert!(text(&r).contains("not running") && text(&r).contains(&format!("solidcraft --control {port}")), "{r}"); + + // The app starts: a token file for this launch, and its control port. + let token = "0123456789abcdef".repeat(4); + std::fs::write(control_dir.join(format!("{port}.token")), &token).unwrap(); + let listener = TcpListener::bind(&addr).unwrap(); + let app = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let mut line = String::new(); + BufReader::new(stream.try_clone().unwrap()).read_line(&mut line).unwrap(); + let req: Value = serde_json::from_str(&line).unwrap(); + let reply = if req["token"] == json!(token) { + json!({"id": req["id"], "ok": true, "result": {"bodies": [], "method": req["method"]}}) + } else { + json!({"id": req["id"], "ok": false, "error": format!("unauthorized: token {}", req["token"])}) + }; + writeln!(&stream, "{reply}").unwrap(); + }); + let r = ask(4, "tools/call", measure); + assert_ne!(r["result"]["isError"], true, "{r}"); + assert!(text(&r).contains("engine.execute"), "{r}"); + app.join().unwrap(); + + drop(stdin); + assert!(child.wait().unwrap().success()); + std::fs::remove_dir_all(&control_dir).ok(); +} + +#[test] +fn mcp_connect_without_a_port_is_refused_at_start() { + for addr in ["localhost", "127.0.0.1:0"] { + let output = Command::new(env!("CARGO_BIN_EXE_solidcraft-cli")).args(["mcp", "--connect", addr]).stdin(Stdio::null()).output().unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(!output.status.success(), "{addr}: {stderr}"); + assert!(stderr.contains("HOST:PORT"), "{addr}: {stderr}"); + } } diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index a316d9fd..e17b350c 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -35,12 +35,18 @@ const BATCH_TIMEOUT: Duration = Duration::from_secs(3600); /// (`solidcraft --control PORT`). pub struct Remote { addr: String, + port: u16, conn: Option<(BufReader, TcpStream)>, next_id: u64, - /// The app's control token, sent with every request (docs/control-protocol.md). - token: Option, + /// `--token`, when given. Otherwise each request looks the token up again, since every + /// launch of the app makes a new one (docs/control-protocol.md). + explicit_token: Option, } +/// How a tool error starts when no app listens at the bridged address. The agent reads it, and +/// `batch` uses it to tell "never sent" from "sent, reply lost". +pub(crate) const NOT_RUNNING: &str = "no SolidCraft app is listening at"; + impl Remote { /// Connect to the app at `addr`, with its control token found the usual way /// (`SOLIDCRAFT_CONTROL_TOKEN`, else the app's token file for that port). @@ -50,17 +56,31 @@ impl Remote { /// [`Remote::connect`] with the token given (`--token`); `None` finds it the usual way. pub fn connect_with_token(addr: &str, token: Option<&str>) -> std::io::Result { - let port = addr.rsplit(':').next().and_then(|p| p.parse::().ok()).unwrap_or(0); - let token = solidcraft_engine::control_auth::client_token(port, token); - let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1, token }; + let mut r = Self::new(addr, token); r.reconnect()?; Ok(r) } + /// A bridge to the app at `addr` that connects when a call needs it: an MCP server started + /// before the app keeps its tools, and they reach the app once it runs. + pub fn new(addr: &str, token: Option<&str>) -> Self { + let port = addr.rsplit(':').next().and_then(|p| p.parse::().ok()).unwrap_or(0); + let explicit_token = token.map(str::to_string); + Self { addr: addr.to_string(), port, conn: None, next_id: 1, explicit_token } + } + + /// Connect now, if the app is there (`new` does not); the error says why not. + pub fn try_connect(&mut self) -> std::io::Result<()> { + self.reconnect() + } + + /// Failing to reach the app at all is `NotConnected`, so [`Remote::explain`] can tell it from + /// a connection lost during a call. fn reconnect(&mut self) -> std::io::Result<()> { self.conn = None; + let not_connected = |e: std::io::Error| std::io::Error::new(std::io::ErrorKind::NotConnected, e); let mut last = std::io::Error::new(std::io::ErrorKind::NotFound, format!("cannot resolve {}", self.addr)); - for sa in self.addr.to_socket_addrs()? { + for sa in self.addr.to_socket_addrs().map_err(not_connected)? { match TcpStream::connect_timeout(&sa, Duration::from_millis(800)) { Ok(s) => { s.set_nodelay(true).ok(); @@ -72,7 +92,23 @@ impl Remote { Err(e) => last = e, } } - Err(last) + Err(not_connected(last)) + } + + /// The tool error for a failed call: whether the app is not running (and how to start it) or + /// went away during the call. + fn explain(&self, e: &std::io::Error) -> String { + let (addr, port) = (&self.addr, self.port); + if e.kind() == std::io::ErrorKind::NotConnected { + format!( + "{NOT_RUNNING} {addr} ({e}): it is not running, or it quit. Start it with `solidcraft --control {port}` \ + (or ask the user to); the next call connects to it" + ) + } else { + format!( + "the SolidCraft app at {addr} did not answer ({e}): it may have quit or crashed during this call, after running it or not. Inspect the design before sending a change again; if the app quit, start it with `solidcraft --control {port}`" + ) + } } fn roundtrip(&mut self, line: &str) -> std::io::Result { @@ -130,19 +166,22 @@ impl Backend for Remote { o.insert("allow_paths".into(), json!(scope.roots().iter().map(|r| r.to_string_lossy()).collect::>())); } let mut msg = json!({"id": id, "method": method, "params": params}); - if let Some(t) = &self.token { + // The token goes with every request (an app restarted mid-session has a new one). + if let Some(t) = solidcraft_engine::control_auth::client_token(self.port, self.explicit_token.as_deref()) { msg["token"] = json!(t); } let line = msg.to_string(); let reply = if method == "engine.batch" { - self.send_batch(&line).map_err(|e| format!("SolidCraft at {}: {e}", self.addr))? + self.send_batch(&line) } else { - match self.roundtrip(&line) { - Ok(r) => r, - // One retry on a fresh connection (the app may have restarted). - Err(_) => self.roundtrip(&line).map_err(|e| format!("SolidCraft at {}: {e}", self.addr))?, - } - }; + // One retry on a fresh connection (the app may have restarted). When nothing listens + // any more, the app went away, maybe while running this request: say that, not + // just "not running". + self.roundtrip(&line).or_else(|lost| { + self.roundtrip(&line).map_err(|e| if e.kind() == std::io::ErrorKind::NotConnected && lost.kind() != e.kind() { lost } else { e }) + }) + } + .map_err(|e| self.explain(&e))?; let v: Value = serde_json::from_str(&reply).map_err(|e| format!("bad reply from the app: {e}"))?; if v.get("ok").and_then(Value::as_bool) == Some(true) { Ok(v.get("result").cloned().unwrap_or(Value::Null)) @@ -154,7 +193,16 @@ impl Backend for Remote { true } fn describe(&self) -> String { - format!("connected to the SolidCraft app at {} (the user watches the viewport)", self.addr) + if self.conn.is_some() { + format!("connected to the SolidCraft app at {} (the user watches the viewport)", self.addr) + } else { + // Still "connected to the SolidCraft app": the skills tell bridged from headless by it. + format!( + "connected to the SolidCraft app at {} once it runs (the user watches the viewport). It was not running \ + when this server started; until it is (`solidcraft --control {}`), tool calls say so", + self.addr, self.port + ) + } } } diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 1a14f58d..77ca859a 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -1120,6 +1120,63 @@ fn remote_engine_batch_is_not_resent_after_a_lost_reply() { } } +/// A bridge made before the app runs keeps its tools (#270): calls say the app is not running and +/// how to start it (a batch was never sent, so it may not "have run"), and reach the app once it +/// listens. +#[test] +fn a_bridge_made_before_the_app_runs_connects_once_it_does() { + use std::io::{BufRead, BufReader, Write}; + + use crate::backend::Backend; + let port = std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port(); + let addr = format!("127.0.0.1:{port}"); + let mut remote = crate::Remote::new(&addr, Some("0123456789abcdef0123456789abcdef")); + // Still "connected to the SolidCraft app", which the skills read as bridged. + assert!(remote.describe().starts_with("connected to the SolidCraft app") && remote.describe().contains("not running"), "{}", remote.describe()); + let e = remote.call("engine.execute", json!({"command": "inspect.measure"})).unwrap_err(); + assert!(e.starts_with(crate::backend::NOT_RUNNING) && e.contains(&format!("solidcraft --control {port}")), "{e}"); + let batch = crate::call_tool(&mut remote, "batch", &json!({"commands": [{"command": "solid.box"}]})); + let text = batch["content"][0]["text"].as_str().unwrap_or_default(); + assert_eq!(batch["isError"], true); + assert!(text.starts_with(crate::backend::NOT_RUNNING) && !text.contains("may have run"), "{text}"); + + let listener = std::net::TcpListener::bind(&addr).unwrap(); + let app = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let mut line = String::new(); + BufReader::new(stream.try_clone().unwrap()).read_line(&mut line).unwrap(); + let req: Value = serde_json::from_str(&line).unwrap(); + writeln!(&stream, "{}", json!({"id": req["id"], "ok": true, "result": {"answered": req["params"]["command"]}})).unwrap(); + }); + let got = remote.call("engine.execute", json!({"command": "inspect.measure"})).unwrap(); + assert_eq!(got["answered"], "inspect.measure"); + assert!(remote.describe().starts_with("connected") && !remote.describe().contains("not running"), "{}", remote.describe()); + app.join().unwrap(); +} + +/// An app that quits while it runs a request may have applied it: the retry then finds nothing +/// listening, and the error says the call may have run, not just that the app is not running. +#[test] +fn an_app_that_quits_during_a_call_may_have_run_it() { + use std::io::{BufRead, BufReader}; + + use crate::backend::Backend; + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let app = std::thread::spawn(move || { + let (stream, _) = listener.accept().unwrap(); + let mut line = String::new(); + BufReader::new(stream.try_clone().unwrap()).read_line(&mut line).unwrap(); + // Quit without answering: the port closes first, so the retry is refused. + drop(listener); + drop(stream); + }); + let mut remote = crate::Remote::connect(&addr).unwrap(); + let e = remote.call("engine.execute", json!({"command": "solid.box"})).unwrap_err(); + app.join().unwrap(); + assert!(!e.starts_with(crate::backend::NOT_RUNNING) && e.contains("may have quit") && e.contains("Inspect the design"), "{e}"); +} + /// Tool annotations never understate what a tool may do (#205): `screenshot` writes a file when /// given a `path`, and `execute`, `batch`, `save`, `export` and `open` can overwrite or delete /// files, so none of them is read-only and all are destructive and reach the file system. diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 9f977654..537f42e6 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -479,6 +479,8 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { Ok(r) => return batch_reply(&r, rollback, verbose), // Older apps and the headless session, or the app wants the steps one by one. Err(e) if e.starts_with("unknown method") || e.starts_with("engine.batch unavailable") => {} + // Never sent: no app to send it to. + Err(e) if e.starts_with(crate::backend::NOT_RUNNING) => return text_result(&Value::String(e), true), // The app may have run it: sending the steps again would apply them twice. Err(e) => { let msg = format!("the app did not answer engine.batch ({e}); it may have run: inspect the design before sending it again"); diff --git a/docs/mcp.md b/docs/mcp.md index e74c67ef..8e3e2f87 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -34,6 +34,16 @@ Bridged, the server finds the app's control token in the token file the app wrot it explicitly when the app runs in a sandbox whose data folder the server can't see (prefer the variable: command-line arguments are visible to other users in the process list). +A bridged server starts even when no app listens on that port yet, so an MCP client that +launches it before the app (or after the app was closed) keeps its tools. The server's +instructions then say the app was not running, and every tool call answers with an error that +starts `no SolidCraft app is listening at HOST:PORT` and says how to start it +(`solidcraft --control PORT`). The first call after the app starts connects to it, reading that +launch's token. When the app quits or crashes mid-session, the next call says the same; a call +that was running when it went away says the app did not answer and may have quit or crashed, +after running the call or not, so inspect the design before sending a change again. +A `batch` that could not be sent says the app is not running, so it is safe to send again. + stdout carries only protocol messages; errors go to stderr. Bridged screenshots are written to a temporary file by the app and read back by the server, so both must run on the same machine.