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.
This commit is contained in:
1 parent
96356dc544
commit
1473893b08
6 files changed
+221
-32
No files matched your search
@@ -424,13 +424,20 @@ fn cmd_mcp(args: &[String]) -> Result<(), String> {
|
||||
}
|
||||
let backend: Box<dyn solidcraft_mcp::Backend> = 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::<u16>().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::<u16>().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()),
|
||||
};
|
||||
|
||||
@@ -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}");
|
||||
}
|
||||
}
|
||||
+64
-16
@@ -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>, TcpStream)>,
|
||||
next_id: u64,
|
||||
/// The app's control token, sent with every request (docs/control-protocol.md).
|
||||
token: Option<String>,
|
||||
/// `--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<String>,
|
||||
}
|
||||
|
||||
/// 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<Self> {
|
||||
let port = addr.rsplit(':').next().and_then(|p| p.parse::<u16>().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::<u16>().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<String> {
|
||||
@@ -130,19 +166,22 @@ impl Backend for Remote {
|
||||
o.insert("allow_paths".into(), json!(scope.roots().iter().map(|r| r.to_string_lossy()).collect::<Vec<_>>()));
|
||||
}
|
||||
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
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -479,6 +479,8 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> 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");
|
||||
|
||||
+10
@@ -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.
|
||||
|
||||
|
||||
Reference in new issue
Block a user