MCP batch runs as one request in the app (#743) (#885)

The app's control protocol gets engine.batch: the steps run in one request (so one frame), each as engine.execute runs it, stopping at the first failure and rolling back to the checkpoint by default. The MCP batch tool sends it to a running app (with the control token), is never resent after a lost reply, and falls back to step-by-step when the app refuses (agent cursor shown, or a step loads a file). A batch may take up to an hour to answer.

crates/mcp/src/backend.rs

@@ -25,6 +25,12 @@ pub trait Backend {
 /// Longest reply line accepted from the app.
 const MAX_REPLY: u64 = 64 << 20;
 
+/// How long a reply may take: kernel operations (booleans, fillets) on big parts can take a while.
+const READ_TIMEOUT: Duration = Duration::from_secs(300);
+
+/// How long the app may take to run an `engine.batch` (its control server waits as long).
+const BATCH_TIMEOUT: Duration = Duration::from_secs(3600);
+
 /// A running SolidCraft app reached through its loopback control port
 /// (`solidcraft --control PORT`).
 pub struct Remote {
@@ -58,8 +64,7 @@ impl Remote {
             match TcpStream::connect_timeout(&sa, Duration::from_millis(800)) {
                 Ok(s) => {
                     s.set_nodelay(true).ok();
-                    // Kernel operations (booleans, fillets) on big parts can take a while.
-                    s.set_read_timeout(Some(Duration::from_secs(300))).ok();
+                    s.set_read_timeout(Some(READ_TIMEOUT)).ok();
                     let read = s.try_clone()?;
                     self.conn = Some((BufReader::new(read), s));
                     return Ok(());
@@ -75,7 +80,8 @@ impl Remote {
             self.reconnect()?;
         }
         let Some((reader, writer)) = self.conn.as_mut() else { return Err(std::io::Error::other("not connected")) };
-        let sent = writer.write_all(line.as_bytes()).and_then(|()| writer.write_all(b"\n")).and_then(|()| writer.flush());
+        // One write: a request that loses its newline still reaches the app as a request.
+        let sent = writer.write_all(format!("{line}\n").as_bytes()).and_then(|()| writer.flush());
         if let Err(e) = sent {
             self.conn = None;
             return Err(e);
@@ -95,6 +101,23 @@ impl Remote {
         }
         Ok(reply)
     }
+
+    fn set_read_timeout(&self, t: Duration) {
+        if let Some((_, writer)) = &self.conn {
+            writer.set_read_timeout(Some(t)).ok();
+        }
+    }
+
+    /// `engine.batch` is never sent twice (#743): it goes on a fresh connection (an idle one may
+    /// have been closed by an app restart; connecting again sends nothing), and once it is being
+    /// written any error is final, as the app may have run it.
+    fn send_batch(&mut self, line: &str) -> std::io::Result<String> {
+        self.reconnect().or_else(|_| self.reconnect())?;
+        self.set_read_timeout(BATCH_TIMEOUT);
+        let r = self.roundtrip(line);
+        self.set_read_timeout(READ_TIMEOUT);
+        r
+    }
 }
 
 impl Backend for Remote {
@@ -111,10 +134,14 @@ impl Backend for Remote {
             msg["token"] = json!(t);
         }
         let line = msg.to_string();
-        let reply = 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))?,
+        let reply = if method == "engine.batch" {
+            self.send_batch(&line).map_err(|e| format!("SolidCraft at {}: {e}", self.addr))?
+        } 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))?,
+            }
         };
         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) {

crates/mcp/src/tests.rs

@@ -867,6 +867,161 @@ fn instructions_name_only_real_tools() {
     }
 }
 
+/// A fake app that records what it is asked and answers `engine.batch` as configured.
+struct Recorder {
+    calls: Vec<(String, Value)>,
+    batch: Result<Value, String>,
+}
+
+impl crate::backend::Backend for Recorder {
+    fn call(&mut self, method: &str, params: Value) -> Result<Value, String> {
+        self.calls.push((method.to_string(), params.clone()));
+        match method {
+            "engine.batch" => self.batch.clone(),
+            "engine.execute" => match params["command"].as_str() {
+                Some("edit.checkpoint") => Ok(json!({"checkpoint": 3})),
+                Some(c) => Ok(json!({"ran": c})),
+                None => Err("missing `command`".into()),
+            },
+            "document.inspect" => Ok(json!({"undo": 0})),
+            other => Err(format!("unknown method `{other}`")),
+        }
+    }
+    fn has_ui(&self) -> bool {
+        true
+    }
+    fn describe(&self) -> String {
+        "recorder".into()
+    }
+}
+
+fn recorded_batch(batch: Result<Value, String>, args: Value) -> (Recorder, Value, bool) {
+    let mut b = Recorder { calls: Vec::new(), batch };
+    let r = crate::tools::call_tool(&mut b, "batch", &args);
+    let body = serde_json::from_str(r["content"][0]["text"].as_str().unwrap()).unwrap_or_else(|_| r["content"][0]["text"].clone());
+    let is_error = r["isError"] == json!(true);
+    (b, body, is_error)
+}
+
+fn methods(b: &Recorder) -> Vec<&str> {
+    b.calls.iter().map(|(m, _)| m.as_str()).collect()
+}
+
+/// #743: bridged to an app that has `engine.batch`, a batch is one control request.
+#[test]
+fn batch_is_one_engine_batch_call() {
+    let reply = json!({"completed": 2, "results": [{"feature": 1, "bodies": ["B"], "created_bodies": ["B"]}, {"x": 2}], "undone": 0});
+    let steps = json!([{"command": "a.one", "params": {"v": 1}}, {"command": "a.two"}]);
+    let (b, body, is_error) = recorded_batch(Ok(reply.clone()), json!({"commands": steps}));
+    assert_eq!(methods(&b), ["engine.batch"]);
+    assert_eq!(b.calls[0].1, json!({"commands": steps, "rollback": true}));
+    assert!(!is_error);
+    assert_eq!(body, json!({"ok": true, "completed": 2, "results": reply["results"]}));
+    // `verbose: false` compacts each step's result as the per-step path does.
+    let (_, body, _) = recorded_batch(Ok(reply), json!({"commands": steps, "verbose": false}));
+    assert_eq!(body["results"][0], json!({"feature": 1, "created_bodies": ["B"]}), "{body}");
+}
+
+#[test]
+fn batch_reports_a_failed_engine_batch_like_before() {
+    let reply = json!({"completed": 1, "results": [{"x": 1}], "failed": {"index": 1, "command": "a.two", "error": "boom"}, "undone": 1});
+    let (b, body, is_error) = recorded_batch(Ok(reply), json!({"commands": [{"command": "a.one"}, {"command": "a.two"}]}));
+    assert_eq!(methods(&b), ["engine.batch"]);
+    assert!(is_error);
+    assert_eq!(
+        body,
+        json!({"ok": false, "completed": 1, "failed": {"index": 1, "command": "a.two", "error": "boom"}, "rolled_back": true, "undone": 1, "results": [{"x": 1}]})
+    );
+}
+
+#[test]
+fn batch_falls_back_when_the_app_lacks_engine_batch() {
+    let (b, body, is_error) =
+        recorded_batch(Err("unknown method `engine.batch`".into()), json!({"commands": [{"command": "a.one"}, {"command": "a.two"}]}));
+    assert!(!is_error, "{body}");
+    let ran: Vec<&str> = b.calls.iter().filter(|(m, _)| m == "engine.execute").map(|(_, p)| p["command"].as_str().unwrap_or("")).collect();
+    assert!(ran.contains(&"a.one") && ran.contains(&"a.two"), "{ran:?}");
+    assert_eq!(body["results"], json!([{"ran": "a.one"}, {"ran": "a.two"}]));
+}
+
+#[test]
+fn batch_falls_back_while_the_agent_cursor_is_shown() {
+    let (b, body, is_error) =
+        recorded_batch(Err("engine.batch unavailable: the agent cursor is shown".into()), json!({"commands": [{"command": "a.one"}]}));
+    assert!(!is_error, "{body}");
+    assert!(b.calls.iter().any(|(m, p)| m == "engine.execute" && p["command"] == "a.one"), "{:?}", b.calls);
+}
+
+/// The app may have run the batch: sending the steps again would apply them twice.
+#[test]
+fn batch_does_not_resend_after_a_transport_error() {
+    let (b, body, is_error) =
+        recorded_batch(Err("SolidCraft at 127.0.0.1:1: timed out".into()), json!({"commands": [{"command": "a.one"}, {"command": "a.two"}]}));
+    assert_eq!(methods(&b), ["engine.batch"]);
+    assert!(is_error);
+    assert!(body.as_str().is_some_and(|t| t.contains("may have run") && t.contains("timed out")), "{body}");
+}
+
+/// A batch longer than one control line goes step by step instead of being refused unrun.
+#[test]
+fn batch_too_large_for_one_control_line_goes_per_step() {
+    let big = "x".repeat(solidcraft_engine::MAX_LINE);
+    let (b, body, is_error) = recorded_batch(Ok(json!({})), json!({"commands": [{"command": "a.one", "params": {"text": big}}]}));
+    assert!(!is_error, "{body}");
+    assert!(!methods(&b).contains(&"engine.batch"), "{:?}", methods(&b));
+    assert!(b.calls.iter().any(|(m, p)| m == "engine.execute" && p["command"] == "a.one"));
+}
+
+/// A fake app on a loopback port: counts connections and `engine.batch` lines, never replies to
+/// them (`close_first`: close a connection as soon as it is accepted).
+fn silent_app(close_first: bool) -> (String, std::sync::Arc<std::sync::Mutex<(usize, usize)>>) {
+    use std::io::BufRead;
+    let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
+    let addr = listener.local_addr().unwrap().to_string();
+    let seen = std::sync::Arc::new(std::sync::Mutex::new((0usize, 0usize)));
+    let counts = seen.clone();
+    std::thread::spawn(move || {
+        for stream in listener.incoming().flatten() {
+            counts.lock().unwrap().0 += 1;
+            let counts = counts.clone();
+            std::thread::spawn(move || {
+                if close_first && counts.lock().unwrap().0 > 1 {
+                    return;
+                }
+                let mut r = std::io::BufReader::new(stream);
+                let mut line = String::new();
+                while r.read_line(&mut line).is_ok_and(|n| n > 0) {
+                    if line.contains("engine.batch") {
+                        counts.lock().unwrap().1 += 1;
+                        return;
+                    }
+                    line.clear();
+                }
+            });
+        }
+    });
+    (addr, seen)
+}
+
+#[test]
+fn remote_engine_batch_is_not_resent_after_a_lost_reply() {
+    use crate::backend::Backend;
+    for close_first in [false, true] {
+        let (addr, seen) = silent_app(close_first);
+        let mut remote = crate::Remote::connect(&addr).unwrap();
+        let r = remote.call("engine.batch", json!({"commands": [{"command": "solid.box"}]}));
+        assert!(r.is_err(), "{r:?}");
+        std::thread::sleep(std::time::Duration::from_millis(300));
+        let (connections, batches) = *seen.lock().unwrap();
+        // The connection from `connect`, and one fresh one for the batch: no resend.
+        assert_eq!(connections, 2, "close_first {close_first}: {connections} connections");
+        assert!(batches <= 1, "close_first {close_first}: the batch arrived {batches} times");
+        if !close_first {
+            assert_eq!(batches, 1);
+        }
+    }
+}
+
 /// 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.

crates/mcp/src/tools.rs

@@ -464,6 +464,22 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Value {
     }
     let rollback = a.get("rollback").and_then(Value::as_bool).unwrap_or(true);
     let verbose = a.get("verbose").and_then(Value::as_bool).unwrap_or(true);
+    // An app that runs batches itself answers the whole batch in one request, so one frame
+    // (#743). One too long for a control line goes step by step: the app would refuse it unrun.
+    // The margin covers the envelope and the `allow_paths` the backend adds.
+    let params = json!({"commands": steps, "rollback": rollback});
+    if params.to_string().len() + 64 * 1024 <= solidcraft_engine::MAX_LINE {
+        match b.call("engine.batch", params) {
+            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") => {}
+            // 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");
+                return text_result(&Value::String(msg), true);
+            }
+        }
+    }
     let start = if rollback { Undo::begin(b) } else { Undo::None };
     let mut results = Vec::with_capacity(steps.len());
     for (i, step) in steps.iter().enumerate() {
@@ -494,6 +510,26 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Value {
     text_result(&json!({"ok": true, "completed": results.len(), "results": results}), false)
 }
 
+/// The `batch` reply from the app's `engine.batch` answer, shaped as the per-step path shapes it.
+fn batch_reply(r: &Value, rollback: bool, verbose: bool) -> Value {
+    let results: Vec<Value> =
+        r["results"].as_array().cloned().unwrap_or_default().into_iter().map(|v| if verbose { v } else { compact(v) }).collect();
+    match r.get("failed").filter(|f| !f.is_null()) {
+        None => text_result(&json!({"ok": true, "completed": results.len(), "results": results}), false),
+        Some(failed) => text_result(
+            &json!({
+                "ok": false,
+                "completed": r["completed"],
+                "failed": failed,
+                "rolled_back": rollback,
+                "undone": r["undone"],
+                "results": results,
+            }),
+            true,
+        ),
+    }
+}
+
 /// A command's result without what the design already had (`execute` or `batch` with `verbose: false`): a
 /// feature's full body list (its created, changed and removed bodies stay), and the sketch ids
 /// a sketch command repeats outside `created`.