From ecb2fc1676f19f48a2e4ec743d7efefdedc9a2ef Mon Sep 17 00:00:00 2001 From: storybored379 Date: Tue, 20 Oct 2026 19:28:00 -0700 Subject: [PATCH] MCP: complete tool hints and check nested argument keys Extend the existing MCP tools instead of adding a second set (#259): - every tool states destructiveHint and idempotentHint next to readOnlyHint/openWorldHint; input schemas are closed - the #776 unknown-key check walks the schema, so a misspelt key in a batch step, a pointer event or mods is -32602 with its path instead of being ignored - script::parse refuses JSON step keys other than command/params - a panic outside a command (rendering, inspection, the app connection) is a tool error and the server keeps running Co-Authored-By: Claude Opus 5.5 (1M context) --- ROADMAP.md | 1 + crates/engine/src/script.rs | 16 +++++++- crates/mcp/src/server.rs | 38 +++++++++++++++-- crates/mcp/src/tests.rs | 81 +++++++++++++++++++++++++++++++++++++ crates/mcp/src/tools.rs | 32 ++++++++++++--- docs/mcp.md | 13 ++++++ 6 files changed, 171 insertions(+), 10 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 5d91d43f9..e93af6dd9 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -348,6 +348,7 @@ list) · [roadmap.md](docs/roadmap.md) (milestones, plan) · [parity-checklist.m Newest first. +- **2026-10-20** · MCP: every tool states all four behaviour hints (read-only, destructive, idempotent, open-world), input schemas are closed, and a misspelt key inside a `batch` step, a `pointer` event or `mods` is refused (`-32602`) like a top-level one instead of being ignored; a JSON script step with a key other than `command`/`params` is an error. A panic outside a command is a tool error instead of ending the server. No readiness percentage change. - **2026-10-22** · Paragraph rules: Rule Above's offset is measured from the first line's baseline to the rule's bottom edge, as in InDesign. Rules sat one ascent too high and crossed the previous paragraph's text (reported from an imported INDD document). No readiness percentage change. - **2026-10-16** · Type › Reload Fonts (`font.reload`): fonts installed, edited or removed while DesignCraft runs are read again without a restart, and the open documents recompose with them (#328, #547). An edited font file is noticed by its length and modification time; bundled, added and document fonts stay as they are. InDesign does this by itself, so noticing changes automatically is still open (gap 6). No readiness percentage change. - **2026-10-16** · Parents can be deleted from their Pages-panel context menu or `layout.parents.delete {id}`. Applied-page and based-on references are cleared, local override copies survive, and Undo restores the parent. Addresses the deletion portion of #956; no readiness percentage change. diff --git a/crates/engine/src/script.rs b/crates/engine/src/script.rs index 4a66ff87f..1e216f93d 100644 --- a/crates/engine/src/script.rs +++ b/crates/engine/src/script.rs @@ -11,7 +11,7 @@ //! **Script text** ([`parse`]): a JSON array of steps, JSON lines (`{"command": …, "params": …}`), //! or compact lines `command.id {json}` / `command.id={json}` / `command.id`; blank lines and //! lines starting with `#` or `//` are ignored. A leading byte order mark and CRLF line endings are -//! accepted. +//! accepted. A JSON step takes only `command` and `params`; any other key is an error. use serde_json::{Value, json}; @@ -59,6 +59,10 @@ pub fn parse(text: &str) -> Result, String> { } fn step_of(v: &Value) -> Result { + // A misspelt key (`parms`) would otherwise run the command with its defaults. + if let Some(key) = v.as_object().and_then(|o| o.keys().find(|k| !matches!(k.as_str(), "command" | "params"))) { + return Err(format!("unknown key `{key}` (a step takes `command` and `params`)")); + } let command = v.get("command").and_then(Value::as_str).filter(|s| !s.is_empty()).ok_or("missing `command`")?.to_string(); let params = v.get("params").cloned().filter(|p| !p.is_null()).unwrap_or(json!({})); if !params.is_object() { @@ -195,6 +199,16 @@ mod tests { assert!(parse("frame.create {bad").unwrap_err().contains("line 1")); } + /// A misspelt step key ran the command with its defaults. + #[test] + fn a_step_with_an_unknown_key_is_refused() { + for text in [r#"[{"command": "file.new", "parms": {"width": 300}}]"#, r#"{"command": "file.new", "parms": {"width": 300}}"#] { + let e = parse(text).unwrap_err(); + assert!(e.contains("unknown key `parms`"), "{e}"); + } + assert!(parse(r#"[{"command": "file.new", "params": {"width": 300}}]"#).is_ok()); + } + /// Windows editors and PowerShell 5.1 save "UTF-8 with BOM" with CRLF line endings; the BOM /// became part of the first command id (#654). #[test] diff --git a/crates/mcp/src/server.rs b/crates/mcp/src/server.rs index 328df76d3..a4426915b 100644 --- a/crates/mcp/src/server.rs +++ b/crates/mcp/src/server.rs @@ -173,13 +173,15 @@ impl Server { } /// Arguments a tool doesn't take, and command params a command doesn't take, are invalid params -/// (#776): a misspelt key is refused instead of being ignored. `execute` and `batch` are checked -/// against the params descriptions of the commands they run, every step before the first runs. +/// (#776): a misspelt key is refused instead of being ignored. Nested objects with listed +/// properties (a `batch` step, a `pointer` event, `mods`) are checked the same way. `execute` and +/// `batch` are checked against the params descriptions of the commands they run, every step +/// before the first runs. fn check_arguments(name: &str, args: &Value) -> Result<(), String> { let Some(arguments) = args.as_object() else { return Ok(()) }; if 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)) + && let Some(schema) = def.get("inputSchema") + && let Some(key) = unknown_key(schema, args, "", 0) { return Err(format!("unknown argument `{key}` for `{name}`")); } @@ -209,3 +211,31 @@ fn check_arguments(name: &str, args: &Value) -> Result<(), String> { _ => Ok(()), } } + +/// The path of the first key in `value` that `schema` doesn't list (`commands[0].parms`). Objects +/// without listed properties (command `params`, `dialog_set`'s `value`) take any key; their +/// contents are checked elsewhere or by the receiver. +fn unknown_key(schema: &Value, value: &Value, at: &str, depth: usize) -> Option { + // The tool schemas are a few levels deep; this only bounds hostile nesting in arguments. + if depth > 8 { + return None; + } + match value { + Value::Object(o) => { + let properties = schema.get("properties").and_then(Value::as_object)?; + for (k, v) in o { + let path = if at.is_empty() { k.clone() } else { format!("{at}.{k}") }; + let Some(sub) = properties.get(k) else { return Some(path) }; + if let Some(bad) = unknown_key(sub, v, &path, depth + 1) { + return Some(bad); + } + } + None + } + Value::Array(items) => { + let sub = schema.get("items")?; + items.iter().enumerate().find_map(|(i, v)| unknown_key(sub, v, &format!("{at}[{i}]"), depth + 1)) + } + _ => None, + } +} diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 21270e5e8..1f7612921 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -677,3 +677,84 @@ fn every_forwarded_key_is_in_the_tool_schema() { } assert!(missing.is_empty(), "keys missing from tool schemas: {missing:#?}"); } + +/// A misspelt key inside a `batch` step, a `pointer` event or `mods` was ignored: the step ran +/// with its defaults and the modifier was dropped. It is refused like a top-level one. +#[test] +fn unknown_nested_keys_are_invalid_params() { + let mut s = server(); + let before = ok(&mut s, "inspect_document", json!({})); + for (id, name, args, key) in [ + (1, "batch", json!({"commands": [{"command": "file.new", "parms": {"width": 300}}]}), "commands[0].parms"), + (2, "pointer", json!({"tool": "rectangle", "events": [{"kind": "down", "x": 10, "y": 10, "mods": {"shfit": true}}]}), "events[0].mods.shfit"), + (3, "pointer", json!({"tool": "rectangle", "events": [{"kind": "down", "x": 10, "y": 10, "click": 2}]}), "events[0].click"), + (4, "pointer", json!({"tool": "rectangle", "events": [{"kind": "down", "x": 10, "y": 10}], "mods": {"Shift": true}}), "mods.Shift"), + ] { + let v = rpc(&mut s, id, "tools/call", json!({"name": name, "arguments": args})); + assert_eq!(v["error"]["code"], -32602, "{v}"); + assert_eq!(v["error"]["message"], format!("unknown argument `{key}` for `{name}`"), "{v}"); + } + // The same step as script text is the tool's error, and nothing runs either. + let r = call(&mut s, "batch", json!({"script": "{\"command\": \"file.new\", \"parms\": {\"width\": 300}}"})); + assert_eq!(r["isError"], true, "{r}"); + assert!(text_of(&r).contains("unknown key `parms`"), "{r}"); + assert_eq!(ok(&mut s, "inspect_document", json!({})), before); + // The listed keys still work, nested ones included. + ok(&mut s, "batch", json!({"commands": [{"command": "frame.create", "params": {"rect": [0, 0, 50, 50]}}]})); + let gesture = json!({"tool": "rectangle", "events": [ + {"kind": "down", "x": 100, "y": 100, "mods": {"shift": true}, "clicks": 1, "space": "doc"}, + {"kind": "drag", "x": 200, "y": 150}, + {"kind": "up", "x": 200, "y": 150}, + ], "mods": {"shift": true}}); + ok(&mut s, "pointer", gesture); +} + +/// Every tool states all four behaviour hints, and every input schema is closed. +#[test] +fn tool_annotations_are_complete() { + for t in tool_definitions() { + let name = t["name"].as_str().unwrap(); + let a = &t["annotations"]; + for hint in ["readOnlyHint", "destructiveHint", "idempotentHint", "openWorldHint"] { + assert!(a[hint].is_boolean(), "{name}: {hint} missing"); + } + if a["readOnlyHint"] == true { + assert_eq!((&a["destructiveHint"], &a["idempotentHint"]), (&json!(false), &json!(true)), "{name}"); + } + assert_eq!(t["inputSchema"]["additionalProperties"], false, "{name}"); + } + let hints = |name: &str| { + let t = tool_definitions().into_iter().find(|t| t["name"] == name).unwrap(); + (t["annotations"]["destructiveHint"].as_bool().unwrap(), t["annotations"]["idempotentHint"].as_bool().unwrap()) + }; + assert_eq!(hints("execute"), (true, false)); + assert_eq!(hints("batch"), (true, false)); + assert_eq!(hints("new_document"), (false, false)); + assert_eq!(hints("set_story_text"), (true, true)); + assert_eq!(hints("select_tool"), (false, true)); +} + +/// A backend that panics on every call, like a bug outside a command (rendering, inspection). +struct Panicky; + +impl Backend for Panicky { + fn call(&mut self, method: &str, _params: Value) -> Result { + panic!("boom in {method}") + } + fn has_ui(&self) -> bool { + false + } + fn describe(&self) -> String { + "panicky".into() + } +} + +/// A panic that escapes the backend is a tool error; the server keeps answering. +#[test] +fn an_escaped_panic_is_a_tool_error() { + let mut s = Server::new(Box::new(Panicky)); + let r = call(&mut s, "inspect_document", json!({})); + assert_eq!(r["isError"], true, "{r}"); + assert!(text_of(&r).contains("internal error: boom in document.inspect"), "{r}"); + assert_eq!(rpc(&mut s, 2, "ping", json!({}))["result"], json!({})); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 0d943b780..60f34c2c6 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -60,20 +60,38 @@ fn mods_schema() -> Value { "additionalProperties": false }) } +/// An object schema. It is closed (`additionalProperties: false`): the server refuses keys it +/// doesn't list, at any depth (see `server::check_arguments`). fn obj(props: Value, required: &[&str]) -> Value { - let mut o = json!({"type": "object", "properties": props}); + let mut o = json!({"type": "object", "properties": props, "additionalProperties": false}); if !required.is_empty() { o["required"] = json!(required); } o } +/// Tools that change state without destroying any: they add a document, switch the tool or change +/// UI state, or close a dialog without applying it. +const NON_DESTRUCTIVE: &[&str] = &["new_document", "open_document", "select_tool", "ui_set", "dialog_cancel"]; +/// Tools that change state, but calling them again with the same arguments changes nothing more: +/// they set a value, or write the same file again. +const IDEMPOTENT: &[&str] = + &["set_story_text", "save_document", "export_png", "render_page", "screenshot", "select_tool", "ui_set", "dialog_set", "dialog_cancel"]; + fn tool(name: &str, title: &str, desc: &str, schema: Value, read_only: bool) -> Value { + let destructive = !read_only && !NON_DESTRUCTIVE.contains(&name); + let idempotent = read_only || IDEMPOTENT.contains(&name); json!({ "name": name, "title": title, "description": desc, "inputSchema": schema, - "annotations": {"title": title, "readOnlyHint": read_only, "openWorldHint": false}, + "annotations": { + "title": title, + "readOnlyHint": read_only, + "destructiveHint": destructive, + "idempotentHint": idempotent, + "openWorldHint": false, + }, }) } @@ -650,8 +668,12 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult { Value::Null => &empty, _ => return ToolResult::error("tool arguments must be a JSON object"), }; - match dispatch(b, name, a) { - Ok(r) => r, - Err(e) => ToolResult::error(format!("{name}: {e}")), + // Commands have the engine's own guard; this one also covers what runs outside a command + // (rendering, inspection, the connection to the app), so a panic there is a tool error and the + // server, with the headless document, keeps running. + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| dispatch(b, name, a))) { + Ok(Ok(r)) => r, + Ok(Err(e)) => ToolResult::error(format!("{name}: {e}")), + Err(payload) => ToolResult::error(format!("{name}: internal error: {}", designcraft_engine::guard::panic_message(payload.as_ref()))), } } diff --git a/docs/mcp.md b/docs/mcp.md index 7cf8227da..8abef375c 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -233,6 +233,19 @@ Use `execute` with `object.fit` to fit a selected graphic frame (or pass `ids`): Tool failures come back as results with `isError: true` and a message the model can act on; protocol errors use JSON-RPC error codes. Resources: `designcraft://document` (document.inspect) and `designcraft://commands`. +Every input schema is closed (`additionalProperties: false`), and the server checks keys at every level it lists +them: a misspelt key in a `batch` step, a `pointer` event or `mods` is a `-32602` error naming its path, e.g. +``unknown argument `commands[0].parms` for `batch` ``. Command `params` are checked against the command's own +description, as above. A `script` step written as JSON takes only `command` and `params`. + +Every tool carries all four MCP hints. The read-only tools (`list_commands`, `inspect_document`, `get_story`, +`menu_list`, `ui_inspect`) are idempotent and not destructive. `new_document`, `open_document`, `select_tool`, +`ui_set` and `dialog_cancel` change state without destroying any; the other tools that edit or write may. +`set_story_text`, `save_document`, `export_png`, `render_page`, `screenshot`, `select_tool`, `ui_set`, +`dialog_set` and `dialog_cancel` are idempotent. A panic that escapes outside a command (rendering, inspection, +the connection to the app) is reported as a tool error and the server keeps running. Exports complete +synchronously, so there is no progress or cancellation to report. + ## Example ```text