Merge pull request #184 from storybored379/feat/mcp-conventions

MCP: complete tool hints and check nested argument keys

ROADMAP.md

@@ -469,6 +469,7 @@ Newest first.
 - **2026-10-22** · macOS: the app menu has a Language submenu after Settings… with each interface language in its own name and the current one checked, as in the suite's other apps; Preferences › General › Interface Language stays. No readiness percentage change.
 - **2026-10-23** · Type › Text Variables › Convert Variable to Text (`variables.convertToText`): the variable instances in the selected text, in a story, a table cell or a footnote, become plain text of the value each shows on its own page (a running header on page 3 becomes page 3's heading). `variables.convertAllToText {name}` does the same for every instance of one variable in the document, as the Text Variables dialog's Convert to Text button. The variable stays defined; one undo step. No readiness percentage change.
 - **2026-10-22** · macOS menu bar: the menus keep working after the interface language or Show CJK Features changes (a rebuilt menu bar lost every click, Quit included), and its key equivalents are the shortcuts in effect, so a shortcut moved in the Keyboard Shortcuts editor leaves its old menu item. No readiness percentage change.
+- **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-21** · Ruler guides are selected like objects (#1282): a click with the Selection tools selects a guide (also where it crosses an object), Shift-click adds or removes one, a new guide from the rulers is selected, a marquee that touches no object selects the guides it crosses, and Edit › Select All Guides (⌥⌘G, `guide.selectAll`) selects every unlocked guide on the spread. Selected guides draw thicker in their layer's color, drag together (all of them shown moving), nudge with the arrow keys and go with Delete, each as one undo step (`guide.select`, `guide.deselect`, `guide.moveBy`, `guide.delete {}`). With one guide selected the Control bar's X (vertical guide) or Y (horizontal guide) field and Properties › Transform show where it is, as the rulers measure, and typing a value moves it there. No readiness percentage change.
 - **2026-10-21** · Properties for points selected with Direct Selection, as InDesign's: the title is "Anchor Point(s)", and the panel shows Transform, Appearance, Convert Points (Plain, Corner, Smooth, Symmetrical) and the Delete Point(s) quick action (`path.deletePoints`, one undo step). Found in Zero2Graphic's *How to Page Layout design in Adobe InDesign 2022 CC*. No readiness percentage change.

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};
 
@@ -63,6 +63,10 @@ pub fn parse(text: &str) -> Result<Vec<Step>, String> {
 }
 
 fn step_of(v: &Value) -> Result<Step, String> {
+    // 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() {
@@ -205,6 +209,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());
+    }
+
     #[test]
     fn compact_lines_require_object_params() {
         for bad in ["a.b 5", "a.b [1]", "a.b \"x\"", "a.b=true"] {

crates/mcp/src/server.rs

@@ -180,13 +180,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}`"));
     }
@@ -216,3 +218,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<String> {
+    // 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,
+    }
+}

crates/mcp/src/tests.rs

@@ -699,3 +699,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<Value, String> {
+        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!({}));
+}

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()))),
     }
 }

docs/mcp.md

@@ -242,6 +242,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