diff --git a/apps/solidcraft/src/control_server.rs b/apps/solidcraft/src/control_server.rs index bb670a6f..e8dd82d3 100644 --- a/apps/solidcraft/src/control_server.rs +++ b/apps/solidcraft/src/control_server.rs @@ -156,12 +156,14 @@ fn serve(stream: TcpStream, token: &str, tx: Sender, ctx: egui:: let id = msg.get("id").cloned().unwrap_or(Value::Null); let method = msg.get("method").and_then(Value::as_str).unwrap_or("").to_string(); let params = msg.get("params").cloned().unwrap_or(json!({})); + // A batch replaces many short requests: the whole of it may take long (#743). + let wait = Duration::from_secs(if method == "engine.batch" { 3600 } else { 60 }); let (req, rrx) = ControlRequest::new(method, params); if tx.send(req).is_err() { break; } ctx.request_repaint(); - let mut r = rrx.recv_timeout(Duration::from_secs(60)).unwrap_or_else(|_| json!({"ok": false, "error": "timeout"})); + let mut r = rrx.recv_timeout(wait).unwrap_or_else(|_| json!({"ok": false, "error": "timeout"})); if let Some(o) = r.as_object_mut() { o.insert("id".into(), id); } diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index e3811dba..33ce7b0d 100644 --- a/crates/mcp/src/backend.rs +++ b/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 { + 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) { diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 66664669..24aa1e76 100644 --- a/crates/mcp/src/tests.rs +++ b/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, +} + +impl crate::backend::Backend for Recorder { + fn call(&mut self, method: &str, params: Value) -> Result { + 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, 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>) { + 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. diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 1545e23e..92466d88 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -464,6 +464,22 @@ fn batch(b: &mut dyn Backend, a: &Map) -> 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) -> 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 = + 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`. diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 7d77fb02..47f89d6e 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -3,6 +3,10 @@ //! Methods (JSON lines over the host's transport, `{"id", "method", "params"}`): //! - `engine.execute {command, params}`: run any command with JSON parameters (no dialogs) //! - `engine.script {commands: [...]}`: run a command script +//! - `engine.batch {commands: [...], rollback?}`: the steps in one request, each as +//! `engine.execute` runs it; stops at the first failure and goes back to the checkpoint +//! (per-step results, `failed`, `undone`). Refused (`engine.batch unavailable: …`) while the +//! agent cursor is shown or when a step loads a file //! - `engine.commands`: every command with tab, panel, params and enablement //! - `document.inspect {measure?}`: the design (parameters, timeline, bodies, sketches) //! - `ui.inspect`: UI state, viewport rect, camera, tool and dialog @@ -86,6 +90,75 @@ fn key_from(name: &str) -> Option { }) } +/// Run one command as `engine.execute` does: through `app.run`, then the first-body fit and the +/// camera the command asked for. +fn execute(app: &mut SolveApp, id: &str, params: Value) -> Result { + let r = app.run(id, params); + if r.is_ok() + && id != "view.navigate" + && app.session.active_sketch.is_none() + && app.session.world_state().bodies.len() == 1 + && app.session.undo.len() <= 2 + { + app.fit_view(); + } + // After the first-body fit, so a camera the command asked for (`view.apply`) wins. + app.follow_view_request(false); + r +} + +/// A batch step's command id. +fn command(step: &Value) -> Option<&str> { + step.get("command").or_else(|| step.get("id")).and_then(Value::as_str) +} + +/// Most steps in one `engine.batch` (as in the MCP `batch` tool). +const MAX_BATCH: usize = 10_000; + +/// Commands that load a file: through `engine.execute` they go to the app's file loader +/// (`file_load::control`: off the window thread, with progress and Cancel), which a batch would +/// bypass. +const LOADS_A_FILE: [&str; 3] = ["doc.open", "file.insert_step", "file.insert_mesh"]; + +/// `engine.batch {commands, rollback?}`: the steps in one request, so in one frame (#743). Each +/// step runs as `engine.execute` runs it; the first failure stops the batch and, with +/// `rollback` (the default), the design goes back to its checkpoint. Step failures are part of +/// the reply; refusals (`engine.batch unavailable: …`) come before anything runs, so the client +/// can send the steps one by one instead. +fn batch(app: &mut SolveApp, p: &Value) -> Outcome { + let Some(steps) = p.get("commands").and_then(Value::as_array).filter(|s| s.len() <= MAX_BATCH) else { + return err(format!("engine.batch: commands must be an array of at most {MAX_BATCH} steps")); + }; + // The agent cursor acts out each `engine.execute` on its own. + if app.ui.agent_cursor.show { + return err("engine.batch unavailable: the agent cursor is shown"); + } + if let Some((i, c)) = steps.iter().enumerate().find_map(|(i, step)| command(step).filter(|c| LOADS_A_FILE.contains(c)).map(|c| (i, c))) { + return err(format!("engine.batch unavailable: step {i} ({c}) loads a file")); + } + let rollback = p.get("rollback").and_then(Value::as_bool).unwrap_or(true); + let checkpoint = if rollback { app.run("edit.checkpoint", json!({})).ok().and_then(|v| v["checkpoint"].as_u64()) } else { None }; + let mut results = Vec::with_capacity(steps.len()); + for (i, step) in steps.iter().enumerate() { + let r = match command(step) { + Some(id) => execute(app, id, step.get("params").cloned().filter(|v| !v.is_null()).unwrap_or(json!({}))), + None => Err("missing `command`".to_string()), + }; + match r { + Ok(v) => results.push(v), + Err(e) => { + let restored = checkpoint.is_some_and(|id| app.run("edit.restore_checkpoint", json!({"checkpoint": id})).is_ok()); + let failed = json!({"index": i, "command": command(step), "error": e}); + return ok(json!({"completed": i, "results": results, "failed": failed, "undone": if restored { i } else { 0 }})); + } + } + } + if let Some(id) = checkpoint { + let _ = app.run("edit.restore_checkpoint", json!({"checkpoint": id, "forget": true})); + } + ok(json!({"completed": results.len(), "results": results, "undone": 0})) +} + pub fn handle(app: &mut SolveApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { // A request from an MCP server carries the folders its agent may use; every file the // request reaches must lie inside them (docs/mcp.md). @@ -110,20 +183,9 @@ fn dispatch(app: &mut SolveApp, ctx: &egui::Context, method: &str, p: &Value) -> match method { "engine.execute" | "command" => { let Some(id) = s("command").or(s("id")) else { return err("missing `command`") }; - let params = p.get("params").cloned().filter(|v| !v.is_null()).unwrap_or(json!({})); - let r = app.run(id, params); - if r.is_ok() - && id != "view.navigate" - && app.session.active_sketch.is_none() - && app.session.world_state().bodies.len() == 1 - && app.session.undo.len() <= 2 - { - app.fit_view(); - } - // After the first-body fit, so a camera the command asked for (`view.apply`) wins. - app.follow_view_request(false); - wrap(r) + wrap(execute(app, id, p.get("params").cloned().filter(|v| !v.is_null()).unwrap_or(json!({})))) } + "engine.batch" => batch(app, p), "ui.agent_cursor" => { let s = &mut app.ui.agent_cursor; if let Some(v) = p.get("show").and_then(Value::as_bool) { @@ -642,6 +704,154 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + fn batch(h: &mut crate::scenario::Harness, params: serde_json::Value) -> serde_json::Value { + h.call("engine.batch", params) + } + + fn inspect(h: &mut crate::scenario::Harness) -> serde_json::Value { + h.call("document.inspect", json!({}))["result"].clone() + } + + /// Timing fields (`ms`) differ from run to run. + fn without_timings(v: &serde_json::Value) -> serde_json::Value { + match v { + serde_json::Value::Object(o) => o.iter().filter(|(k, _)| *k != "ms").map(|(k, v)| (k.clone(), without_timings(v))).collect(), + serde_json::Value::Array(a) => a.iter().map(without_timings).collect(), + v => v.clone(), + } + } + + /// #743: a batch is one request through the app's queue, answered within a frame, however + /// many steps it has. + #[test] + fn engine_batch_runs_in_one_frame() { + let mut h = crate::scenario::Harness::new(); + let steps: Vec<_> = (0..40) + .flat_map(|_| [json!({"command": "sketch.create", "params": {"plane": "XY"}}), json!({"command": "sketch.finish", "params": {}})]) + .collect(); + let (v, frames) = h.call_queued("engine.batch", json!({"commands": steps})); + assert_eq!(v["ok"], true, "{v}"); + assert_eq!(v["result"]["completed"], 80, "{v}"); + assert_eq!(v["result"]["results"].as_array().map(Vec::len), Some(80)); + assert!(v["result"].get("failed").is_none(), "{v}"); + assert!(frames <= 2, "a batch of 80 steps took {frames} frames"); + assert_eq!(inspect(&mut h)["sketches"].as_array().map(Vec::len), Some(40)); + } + + #[test] + fn engine_batch_rolls_back_at_the_first_failure() { + let mut h = crate::scenario::Harness::new(); + assert_eq!(h.call("engine.execute", json!({"command": "solid.box", "params": {"length": 10, "width": 10, "height": 10}}))["ok"], true); + let before = inspect(&mut h); + let v = batch( + &mut h, + json!({"commands": [ + {"command": "solid.sphere", "params": {"radius": 3, "center": [50, 0, 0]}}, + {"command": "solid.extrude", "params": {"distance": "bogus +"}}, + {"command": "solid.sphere", "params": {"radius": 3, "center": [80, 0, 0]}}, + ]}), + ); + assert_eq!(v["ok"], true, "{v}"); + let r = &v["result"]; + assert_eq!((r["completed"].clone(), r["failed"]["index"].clone(), r["undone"].clone()), (json!(1), json!(1), json!(1)), "{v}"); + assert_eq!(r["failed"]["command"], "solid.extrude"); + let after = inspect(&mut h); + assert_eq!(after["undo"], before["undo"], "{after}"); + assert_eq!(after["bodies"], before["bodies"]); + assert_eq!(without_timings(&after["timeline"]), without_timings(&before["timeline"])); + } + + #[test] + fn engine_batch_without_rollback_keeps_completed_steps() { + let mut h = crate::scenario::Harness::new(); + let v = batch( + &mut h, + json!({"rollback": false, "commands": [ + {"command": "solid.sphere", "params": {"radius": 3}}, + {"command": "NoSuchCommand"}, + ]}), + ); + assert_eq!(v["result"]["failed"]["index"], 1, "{v}"); + assert_eq!(v["result"]["undone"], 0, "{v}"); + assert_eq!(inspect(&mut h)["bodies"].as_array().map(Vec::len), Some(1)); + } + + #[test] + fn engine_batch_step_without_a_command_fails_that_step() { + let mut h = crate::scenario::Harness::new(); + let v = batch(&mut h, json!({"commands": [{"params": {}}]})); + assert_eq!(v["result"]["failed"]["index"], 0, "{v}"); + assert!(v["result"]["failed"]["error"].as_str().is_some_and(|e| e.contains("missing `command`")), "{v}"); + } + + #[test] + fn engine_batch_is_refused_when_a_step_loads_a_file() { + let mut h = crate::scenario::Harness::new(); + let v = batch( + &mut h, + json!({"commands": [ + {"command": "solid.box", "params": {"length": 10, "width": 10, "height": 10}}, + {"command": "doc.open", "params": {"path": "elsewhere.solidcraft"}}, + ]}), + ); + assert_eq!(v["ok"], false, "{v}"); + assert_eq!(v["error"], "engine.batch unavailable: step 1 (doc.open) loads a file", "{v}"); + assert_eq!(inspect(&mut h)["bodies"].as_array().map(Vec::len), Some(0), "nothing may run before the refusal"); + } + + #[test] + fn engine_batch_is_refused_while_the_agent_cursor_is_shown() { + let mut h = crate::scenario::Harness::new(); + assert_eq!(h.call("ui.agent_cursor", json!({"show": true}))["ok"], true); + let v = batch(&mut h, json!({"commands": [{"command": "solid.box", "params": {"length": 10, "width": 10, "height": 10}}]})); + assert_eq!(v["ok"], false, "{v}"); + assert!(v["error"].as_str().is_some_and(|e| e.starts_with("engine.batch unavailable")), "{v}"); + assert_eq!(inspect(&mut h)["bodies"].as_array().map(Vec::len), Some(0)); + } + + /// Every step of a batch from an MCP server stays inside the agent's folders (#205), as an + /// `engine.execute` would. + #[test] + fn engine_batch_stays_inside_allow_paths() { + let mut h = crate::scenario::Harness::new(); + let base = std::env::temp_dir().join(format!("solidcraft-batch-scope-{}", std::process::id())); + let (root, out) = (base.join("root"), base.join("out")); + std::fs::create_dir_all(&root).unwrap(); + std::fs::create_dir_all(&out).unwrap(); + let export = |d: &std::path::Path| json!({"command": "file.export", "params": {"path": d.join("a.step").to_string_lossy()}}); + let v = batch( + &mut h, + json!({"allow_paths": [root.to_string_lossy()], "commands": [ + {"command": "solid.box", "params": {"length": 10, "width": 10, "height": 10}}, + export(&root), + export(&out), + ]}), + ); + assert_eq!(v["result"]["failed"]["index"], 2, "{v}"); + assert!(v["result"]["failed"]["error"].as_str().is_some_and(|e| e.contains("outside the folders this agent may use")), "{v}"); + assert!(root.join("a.step").exists()); + assert_eq!(std::fs::read_dir(&out).unwrap().count(), 0, "nothing written outside"); + let _ = std::fs::remove_dir_all(&base); + } + + /// Each step runs as `engine.execute` runs it: same results, same design. + #[test] + fn engine_batch_matches_engine_execute() { + let steps = [ + json!({"command": "sketch.create", "params": {"plane": "XY"}}), + json!({"command": "sketch.rectangle.two_point", "params": {"p0": [0, 0], "p1": [40, 30]}}), + json!({"command": "sketch.finish", "params": {}}), + json!({"command": "solid.extrude", "params": {"distance": 20}}), + ]; + let mut a = crate::scenario::Harness::new(); + let batched = batch(&mut a, json!({"commands": steps})); + let mut b = crate::scenario::Harness::new(); + let one_by_one: Vec<_> = steps.iter().map(|s| b.call("engine.execute", s.clone())["result"].clone()).collect(); + assert_eq!(without_timings(&batched["result"]["results"]), without_timings(&json!(one_by_one)), "{batched}"); + assert_eq!(without_timings(&inspect(&mut a)["timeline"]), without_timings(&inspect(&mut b)["timeline"])); + assert_eq!(a.app.cam, b.app.cam, "the first-body fit must happen as for engine.execute"); + } + /// A request carrying `allow_paths` (from an MCP server, #205) reaches files only inside /// those folders; the same request without it, like the user's own dialogs, is unconfined. #[test] diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 425ec980..c817716d 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -1134,7 +1134,7 @@ impl SolveApp { let Some(req) = held.or_else(|| rx.try_recv().ok()) else { break }; // A file is loading: commands that change the design wait for it (the window, // inspection and screenshots still answer). - if self.file_load.busy() && matches!(req.method.as_str(), "engine.execute" | "command" | "engine.script") { + if self.file_load.busy() && matches!(req.method.as_str(), "engine.execute" | "command" | "engine.script" | "engine.batch") { if self.control_held.len() < MAX_HELD { self.control_held.push_back(req); } else { diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 12a0b52f..c6f85b44 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -47,8 +47,8 @@ with `{"ok": false, "error": "request too large"}` and the connection is closed, connections that send eight lines that are not JSON. At most 16 connections are served at once (further ones get `{"ok": false, "error": "too many connections"}`), at most 64 queued requests run per frame so a flood cannot starve drawing, and a request that the UI thread has not -answered within 60 seconds is replied to with `{"ok": false, "error": "timeout"}`. The same -16 MiB line limit applies to the MCP server. +answered within 60 seconds (3600 seconds for `engine.batch`) is replied to with +`{"ok": false, "error": "timeout"}`. The same 16 MiB line limit applies to the MCP server. Any request's `params` may carry `allow_paths: ["/abs/folder", …]`: every file that request reads, writes or deletes (command paths such as `file.export`'s `path` or `doc.recovery_*`'s @@ -65,6 +65,7 @@ MCP server sends its folders this way on every request ([mcp.md](mcp.md)). Witho |---|---|---| | `engine.execute` | `command`: command id, `params`: JSON object | the command's result | | `engine.script` | `commands`: `[{command, params}, …]` | results, stops at the first error | +| `engine.batch` | `commands`: `[{command, params}, …]` (at most 10 000), `rollback?`: bool (default true) | `{completed, results, undone}`, plus `failed: {index, command, error}` when a step fails. Runs the steps in one request, so within one frame, each as `engine.execute` runs it. The first failure stops the batch, and with `rollback` the design goes back to how it was before it (`undone`: the steps taken back). Refused before anything runs, with an error starting `engine.batch unavailable`, while the agent cursor is shown or when a step loads a file (`doc.open`, `file.insert_step`, `file.insert_mesh`). Send such steps one by one. Answered when done, for up to 3600 s | | `engine.commands` | — | every command: id, label, tab, panel, icon, shortcut, params doc, enabled | | `document.inspect` | `measure?`: bool | parameters, timeline (errors, timings), bodies, sketches, selection | diff --git a/docs/mcp.md b/docs/mcp.md index 1408c3f7..317eb642 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -118,7 +118,7 @@ They cover the workflow, the command shapes, how to verify with `measure`, and t |---|---|---| | `list_commands` | `filter?` | The command catalog: id, label, tab/panel, shortcut, parameter docs, enabled now. `filter` matches id, label, tab and panel. | | `execute` | `command`, `params?`, `verbose?` (default true) | Run any command with JSON parameters (`list_commands` shows them). Failures leave the design unchanged. A feature command replies with the feature (`feature`, `name`), every body (`bodies`) and the bodies it created, changed and removed (`created_bodies`, `changed_bodies`, `removed_bodies`). `verbose: false` uses the same compact reply as `batch`. | -| `batch` | `commands: [{command, params}]`, `rollback?` (default true), `verbose?` (default true) | Run steps in order, stop at the first error and undo the completed steps. The reply names the failing step (`failed.index`, `failed.command`, `failed.error`) and every result before it. `verbose: false` keeps the reply small in large designs: feature steps leave out the full body list (`created_bodies`, `changed_bodies`, `removed_bodies` stay) and sketch steps the ids they repeat outside `created`. | +| `batch` | `commands: [{command, params}]`, `rollback?` (default true), `verbose?` (default true) | Run steps in order, stop at the first error and undo the completed steps. The reply names the failing step (`failed.index`, `failed.command`, `failed.error`) and every result before it. `verbose: false` keeps the reply small in large designs: feature steps leave out the full body list (`created_bodies`, `changed_bodies`, `removed_bodies` stay) and sketch steps the ids they repeat outside `created`. Bridged to an app that has `engine.batch`, the whole batch is one control request (one frame of the app). Older apps, and apps showing the agent cursor, get the steps one by one. | | `inspect_design` | `measure?` (default true), `sketches?` (default true) | Parameters, timeline (type, suppressed, rolled back, error, warning, timing), bodies (volume, area, centre of mass, bbox, face/edge/vertex counts, visible), sketches (curves, points, constraints, profiles, DOF, solve status, visible), selection, undo depth. Hide helper sketches, bodies or construction planes with `execute` `browser.visibility` (`{items, visible}` or `{folder, visible}`). | | `measure` | `body?` | Volume (mm³), area (mm²), centre of mass, bbox and topology counts per body, with totals. | | `body_topology` | `body` | Edges (index, midpoint, length, ends) and faces (index, area, centroid, normal). An edge midpoint is the `[x,y,z]` that picks that edge for fillet and chamfer. |