diff --git a/crates/mcp/src/lib.rs b/crates/mcp/src/lib.rs index 9f827330b..73a37272d 100644 --- a/crates/mcp/src/lib.rs +++ b/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; diff --git a/crates/mcp/src/server.rs b/crates/mcp/src/server.rs index 74004283b..ba5b23c13 100644 --- a/crates/mcp/src/server.rs +++ b/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()) } diff --git a/crates/mcp/src/tests_batchdraw.rs b/crates/mcp/src/tests_batchdraw.rs new file mode 100644 index 000000000..629e02834 --- /dev/null +++ b/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); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 5947034c5..1048810d3 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -116,9 +116,9 @@ pub fn tool_definitions() -> Vec { 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 { 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 { // 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 = 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 { 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 { 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) } diff --git a/docs/mcp.md b/docs/mcp.md index 7d24a2497..245f6f8b2 100644 --- a/docs/mcp.md +++ b/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 |