MCP command_batch: draw_path and draw_shape run as steps (#2113)

A step whose id is draw_path or draw_shape runs that tool with its params,
checked as tools/call checks arguments; its result is the tool's, so later
steps can use "$N.id". The reply lists the ids the drawing steps made
under created. The unknown-argument check is shared with tools/call.

crates/mcp/src/lib.rs

@@ -35,6 +35,8 @@ pub const DEFAULT_ADDR: &str = "127.0.0.1:7979";
 #[cfg(test)]
 mod tests;
 #[cfg(test)]
+mod tests_batchdraw;
+#[cfg(test)]
 mod tests_conventions;
 #[cfg(test)]
 mod tests_distortkeys;

crates/mcp/src/server.rs

@@ -272,12 +272,8 @@ impl Server {
             "tools/call" => {
                 let name = params.get("name").and_then(Value::as_str).ok_or((INVALID_PARAMS, "missing tool `name`".to_string()))?;
                 let args = params.get("arguments").cloned().unwrap_or(Value::Null);
-                if let Some(arguments) = args.as_object()
-                    && let Some(def) = tool_definitions().into_iter().find(|t| t.get("name").and_then(Value::as_str) == Some(name))
-                    && let Some(properties) = def.get("inputSchema").and_then(|s| s.get("properties")).and_then(Value::as_object)
-                    && let Some(key) = arguments.keys().find(|k| !properties.contains_key(*k))
-                {
-                    return Err((INVALID_PARAMS, format!("unknown argument `{key}` for `{name}`")));
+                if let Some(arguments) = args.as_object() {
+                    crate::tools::check_tool_arguments(name, arguments).map_err(|e| (INVALID_PARAMS, e))?;
                 }
                 Ok(call_tool(self.backend.as_mut(), name, &args).to_value())
             }

crates/mcp/src/tests_batchdraw.rs

@@ -0,0 +1,97 @@
+//! `draw_path` and `draw_shape` as `command_batch` steps, with the new objects' ids in the reply
+//! (#2113).
+
+use crate::{Headless, Server};
+use serde_json::{Value, json};
+
+fn call(s: &mut Server, name: &str, arguments: Value) -> Value {
+    let r = s.handle(json!({"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":name,"arguments":arguments}})).unwrap();
+    assert!(r.get("error").is_none(), "{r}");
+    r["result"].clone()
+}
+
+fn payload(r: &Value) -> Value {
+    serde_json::from_str(r["content"][0]["text"].as_str().unwrap()).unwrap()
+}
+
+fn node(s: &mut Server, id: &Value) -> Value {
+    payload(&call(s, "run_command", json!({"command":"document.node","params":{"id":id}})))
+}
+
+#[test]
+fn drawing_tools_run_as_batch_steps() {
+    let mut s = Server::new(Box::new(Headless::with_document()));
+    let steps = json!([
+        {"id":"draw_path","params":{"d":"M0 0 L100 0 L50 80 Z","fill":"#550505","stroke":"none"}},
+        {"id":"draw_shape","params":{"shape":"rectangle","x":0,"y":0,"width":100,"height":80,"fill":"#000000"}},
+        {"id":"object.setProps","params":{"id":"$0.id","name":"Facet 1"}},
+        {"id":"draw_path","params":{"points":[[0,0],[10,10]],"stroke":"#c19a4b","strokeWidth":2}}
+    ]);
+    let r = call(&mut s, "command_batch", json!({"steps":steps,"atomic":true,"label":"Facets"}));
+    assert_eq!(r["isError"], false, "{r}");
+    let p = payload(&r);
+    assert_eq!(p["completed"], 4, "{p}");
+    let (path, rect, line) =
+        (p["results"][0]["result"]["id"].clone(), p["results"][1]["result"]["id"].clone(), p["results"][3]["result"]["id"].clone());
+    assert!(path.is_u64() && rect.is_u64() && line.is_u64(), "{p}");
+    // The step results are the tools' own: the paint applied, the shape drawn.
+    assert_eq!(p["results"][0]["result"]["paint"]["fill"], "#550505");
+    assert_eq!(p["results"][1]["result"]["shape"], "rectangle");
+    // A later step used the path's id.
+    assert_eq!(node(&mut s, &path)["name"], "Facet 1");
+    // The drawing steps' new objects, in order.
+    assert_eq!(p["created"], json!([path, rect, line]));
+    // One undo step takes the whole batch back.
+    let undone = payload(&call(&mut s, "run_command", json!({"command":"edit.undo"})));
+    assert_eq!(undone["undone"], "Facets");
+    assert_eq!(call(&mut s, "run_command", json!({"command":"document.node","params":{"id":path}}))["isError"], true);
+}
+
+#[test]
+fn a_batch_without_drawing_steps_lists_nothing_created() {
+    let mut s = Server::new(Box::new(Headless::with_document()));
+    let r = call(&mut s, "command_batch", json!({"steps":[{"id":"shape.rectangle","params":{"x":0,"y":0,"width":5,"height":5}}]}));
+    let p = payload(&r);
+    assert_eq!(p["completed"], 1);
+    assert_eq!(p["created"], json!([]));
+}
+
+/// Drawing steps check their arguments as `tools/call` does (#713): an unknown or misspelled one
+/// fails the step and draws nothing.
+#[test]
+fn drawing_steps_refuse_unknown_arguments() {
+    let mut s = Server::new(Box::new(Headless::with_document()));
+    let count = |s: &mut Server| payload(&call(s, "doc_inspect", json!({})))["objects"].as_u64().unwrap();
+    let before = count(&mut s);
+    for step in [
+        json!({"id":"draw_shape","params":{"shape":"rectangle","x":0,"y":0,"w":5,"h":5}}),
+        json!({"id":"draw_path","params":{"d":"M0 0 L5 5","colour":"#ff0000"}}),
+        json!({"id":"draw_path","params":{}}),
+        json!({"id":"draw_shape","params":{"shape":"hexagon"}}),
+    ] {
+        let r = call(&mut s, "command_batch", json!({"steps":[step]}));
+        assert_eq!(r["isError"], true, "{step}: {r}");
+        let p = payload(&r);
+        assert_eq!(p["failed"], 1, "{step}: {p}");
+        assert_eq!(p["created"], json!([]), "{step}: {p}");
+    }
+    let p = payload(&call(&mut s, "command_batch", json!({"steps":[{"id":"draw_shape","params":{"shape":"rectangle","x":0,"y":0,"w":5}}]})));
+    assert!(p["results"][0]["error"].as_str().unwrap().contains("unknown argument `w` for `draw_shape`"), "{p}");
+    assert_eq!(count(&mut s), before);
+}
+
+/// A drawing step's arguments can come from an earlier step's result.
+#[test]
+fn drawing_steps_take_earlier_results() {
+    let mut s = Server::new(Box::new(Headless::with_document()));
+    let steps = json!([
+        {"id":"shape.rectangle","params":{"x":40,"y":40,"width":20,"height":20}},
+        {"id":"document.node","params":{"id":"$0.id","summary":true}},
+        {"id":"draw_shape","params":{"shape":"rectangle","x":0,"y":0,"width":10,"height":10,"fill":"#ff0000"}},
+        {"id":"draw_path","params":{"d":"M0 0 L5 5","stroke":"$2.paint.fill"}}
+    ]);
+    let p = payload(&call(&mut s, "command_batch", json!({"steps":steps})));
+    assert_eq!(p["completed"], 4, "{p}");
+    assert_eq!(p["results"][3]["result"]["paint"]["stroke"], "#ff0000", "{p}");
+    assert_eq!(p["created"].as_array().unwrap().len(), 2);
+}

crates/mcp/src/tools.rs

@@ -116,9 +116,9 @@ pub fn tool_definitions() -> Vec<Value> {
         tool(
             "command_batch",
             "Run several commands",
-            "Run steps in order; each edit is its own undo step, or with atomic:true the whole batch is one (named `label`, else after its first edit). A param string that is exactly \"$N.path\" (N: 0-based step index) or \"$last.path\" takes that value from an earlier step's result, e.g. {\"id\":\"$1.id\"}; \"$$\" starts a literal \"$\". Returns completed, failed and results. Stops on the first error by default. At most 256 steps; kept step results total at most 8 MiB, past which the batch stops with the reason.",
+            "Run steps in order; each edit is its own undo step, or with atomic:true the whole batch is one (named `label`, else after its first edit). A step's id is a command id, or draw_path / draw_shape with that tool's arguments as params (its result is the tool's: the new object's `id`). A param string that is exactly \"$N.path\" (N: 0-based step index) or \"$last.path\" takes that value from an earlier step's result, e.g. {\"id\":\"$1.id\"}; \"$$\" starts a literal \"$\". Returns completed, failed, results and created (the ids draw_path / draw_shape steps made, in order). Stops on the first error by default. At most 256 steps; kept step results total at most 8 MiB, past which the batch stops with the reason.",
             obj(
-                json!({"steps": {"type":"array", "maxItems": 256, "items":obj(json!({"id":string("Command id"),"params":{"type":"object"}}), &["id"])}, "stop_on_error":{"type":"boolean", "default":true}, "atomic":{"type":"boolean", "default":false, "description":"Make the batch's edits one undo step (needs an open document)"}, "label":string("The undo step's name with atomic")}),
+                json!({"steps": {"type":"array", "maxItems": 256, "items":obj(json!({"id":string("Command id, or draw_path / draw_shape"),"params":{"type":"object"}}), &["id"])}, "stop_on_error":{"type":"boolean", "default":true}, "atomic":{"type":"boolean", "default":false, "description":"Make the batch's edits one undo step (needs an open document)"}, "label":string("The undo step's name with atomic")}),
                 &["steps"],
             ),
             false,
@@ -1038,6 +1038,17 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult {
 pub(crate) const MAX_COMMAND_BATCH_STEPS: usize = 256;
 pub(crate) const MAX_COMMAND_BATCH_BYTES: usize = 8 * 1024 * 1024;
 
+/// Refuses an argument the tool's schema doesn't list, as `tools/call` does (#713).
+pub(crate) fn check_tool_arguments(name: &str, args: &Args) -> Result<(), String> {
+    let defs = tool_definitions();
+    let props =
+        defs.iter().find(|t| t.get("name").and_then(Value::as_str) == Some(name)).and_then(|t| t.get("inputSchema")?.get("properties")?.as_object());
+    match props.and_then(|p| args.keys().find(|k| !p.contains_key(*k))) {
+        Some(key) => Err(format!("unknown argument `{key}` for `{name}`")),
+        None => Ok(()),
+    }
+}
+
 fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
     let steps = a.get("steps").and_then(Value::as_array).ok_or("`steps` must be an array")?;
     if steps.len() > MAX_COMMAND_BATCH_STEPS {
@@ -1063,6 +1074,8 @@ fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
     // The kept results all go back in the reply, so their total size is capped: one batch can't
     // pile up an unbounded answer (#1727).
     let mut kept_bytes = 0usize;
+    // The objects the drawing steps made (#2113).
+    let mut created: Vec<Value> = vec![];
     for step in steps {
         let result = vectorcraft_engine::guard::catch_panic(|| {
             let args = step.as_object().ok_or("each step must be an object")?;
@@ -1075,7 +1088,19 @@ fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
                 Some(v @ Value::Object(_)) => vectorcraft_engine::steps::resolve(v, &values)?,
                 Some(_) => return Err("`params` must be an object".into()),
             };
-            b.call("engine.execute", json!({"command":id,"params":params}))
+            match id {
+                // The drawing tools, with the arguments `tools/call` takes (#2113).
+                "draw_path" | "draw_shape" => {
+                    let args = params.as_object().cloned().unwrap_or_default();
+                    check_tool_arguments(id, &args)?;
+                    let made = if id == "draw_path" { draw_path(b, &args)? } else { draw_shape(b, &args)? };
+                    if let Some(new) = made.get("id").filter(|v| v.is_u64()) {
+                        created.push(new.clone());
+                    }
+                    Ok(made)
+                }
+                _ => b.call("engine.execute", json!({"command":id,"params":params})),
+            }
         })
         .unwrap_or_else(|e| Err(format!("internal error: {e}")));
         match result {
@@ -1110,7 +1135,7 @@ fn command_batch(b: &mut dyn Backend, a: &Args) -> Result<ToolResult, String> {
     if atomic {
         b.call("engine.execute", json!({"command":"edit.undoGroup.end","params":{}}))?;
     }
-    let mut r = ToolResult::json(&json!({"completed":completed,"failed":failed,"results":results}));
+    let mut r = ToolResult::json(&json!({"completed":completed,"failed":failed,"results":results,"created":created}));
     r.is_error = failed > 0;
     Ok(r)
 }

docs/mcp.md

@@ -260,7 +260,7 @@ The older names below remain listed because existing workflows use them.
 |---|---|
 | `command_list` | Optional `filter`, `enabled_only`; returns the command array |
 | `command_run` | `id`, optional `params`; runs the command through the existing backend. An unknown key in `params` is an error that names it and the nearest key the command takes (`invalid parameters for `file.new`: unknown parameter `widht`; did you mean `width`?`), so a misspelt key no longer runs the command with its default (see [Parameter keys](#parameter-keys)) |
-| `command_batch` | `steps: [{id, params?}]` (at most 256), optional `stop_on_error` (default true), `atomic` and `label`; returns `completed`, `failed`, `results: [{ok, result\|error}]`. Each edit has its own undo step, or with `atomic: true` the batch's edits are one undo step named `label` (else after its first edit; `edit.undoGroup.begin`/`end` underneath); failures set `isError`. A param string `"$N.path"` or `"$last.path"` takes a value from an earlier step's result (see `run` below). Kept step results total at most 8 MiB; past that the batch stops with the reason |
+| `command_batch` | `steps: [{id, params?}]` (at most 256), optional `stop_on_error` (default true), `atomic` and `label`; returns `completed`, `failed`, `results: [{ok, result\|error}]` and `created` (the ids the `draw_path` / `draw_shape` steps made, in order). A step's `id` is a command id, or `draw_path` / `draw_shape` with that tool's arguments as `params` (checked as `tools/call` checks them; the step's result is the tool's, so `"$N.id"` is the new object). Each edit has its own undo step, or with `atomic: true` the batch's edits are one undo step named `label` (else after its first edit; `edit.undoGroup.begin`/`end` underneath); failures set `isError`. A param string `"$N.path"` or `"$last.path"` takes a value from an earlier step's result (see `run` below). Kept step results total at most 8 MiB; past that the batch stops with the reason |
 | `doc_inspect` | Optional `depth`, `childLimit`; same summary as `inspect_document` |
 | `render_preview` | Optional `max_side` (1–4096, default 1024) and `artboard` (0-based, default 0); inline PNG of that artboard without editing it. Artboards too large to render within the allocation bound return a tool error |
 | `ui_inspect`, `ui_screenshot` | Connected desktop state/window capture; tool errors in headless mode |