M0.9: commands refuse parameter names they don't take (#1413)

A misspelt parameter name was ignored and the command reported success,
answering as if the parameter hadn't been given (`comment.list
{"filterr": "open"}` listed resolved threads). `Session::execute` now
refuses names the command's params doc doesn't list, naming them and
what the command takes, before the command runs, so every path into a
command (CLI, batches, control channel, MCP, plugins, the UI) gets the
check.

The names come from the existing docs, parsed once with the registry:
each top-level item's first word and the words joined to it by `|`, `/`
or `+`, bracketed text and what follows `:`, `=` or `→` being
description, alternative groups (`{x, y} | {points}`) included. `ids`
and `id` are always taken (`params::targets` reads either); node.set,
whose doc is its props list, keeps its own check.

The junk sweep runs each junk object as given and cut down to the names
a command takes, so the junk values still reach the parsers. Tests that
passed names their command never read (comment.resolve's author,
collab.host's link, node.get's depth) pass only what it takes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

crates/engine/src/cmd.rs

@@ -63,6 +63,8 @@ macro_rules! spec {
 struct Registry {
     list: Vec<CommandSpec>,
     index: HashMap<&'static str, usize>,
+    /// The parameter names each command's doc lists (`list` order; see [`doc_param_names`]).
+    names: Vec<Option<Vec<&'static str>>>,
 }
 
 fn registry() -> &'static Registry {
@@ -123,7 +125,8 @@ fn registry() -> &'static Registry {
                 log::warn!("duplicate command id {}", s.id);
             }
         }
-        Registry { list, index }
+        let names = list.iter().map(|s| doc_param_names(s.params)).collect();
+        Registry { list, index, names }
     })
 }
 
@@ -136,6 +139,113 @@ pub fn find_command(id: &str) -> Option<&'static CommandSpec> {
     r.index.get(id).and_then(|i| r.list.get(*i))
 }
 
+/// The parameter names command `id` takes, from its doc; `None` when the doc isn't a `{…}` list
+/// (`node.set` documents its props, and refuses the ones it doesn't know itself).
+pub fn param_names(id: &str) -> Option<&'static [&'static str]> {
+    let r = registry();
+    r.index.get(id).and_then(|i| r.names.get(*i)).and_then(|n| n.as_deref())
+}
+
+/// Refuse params naming something the command doesn't take: a misspelt name would otherwise be
+/// ignored and the command answer as if it hadn't been given. `ids` and `id` are always taken:
+/// the layers a command acts on (`params::targets` reads either).
+pub fn refuse_unknown_params(id: &str, params: &Value) -> Result<()> {
+    let (Some(names), Some(given)) = (param_names(id), params.as_object()) else { return Ok(()) };
+    let taken = |k: &str| matches!(k, "ids" | "id") || names.contains(&k);
+    let unknown: Vec<String> = given.keys().filter(|k| !taken(k)).map(|k| format!("`{k}`")).collect();
+    if unknown.is_empty() {
+        return Ok(());
+    }
+    let s = if unknown.len() == 1 { "" } else { "s" };
+    let takes = if names.is_empty() { "none".to_string() } else { names.join(", ") };
+    Err(crate::bad(id, format!("unknown parameter{s} {} (it takes {takes})", unknown.join(", "))))
+}
+
+/// The top-level items of a `{…}` params doc (`{ids?, direction? (horizontal|vertical)}` →
+/// `ids?`, ` direction? (horizontal|vertical)`), with those of the alternatives after it
+/// (`{x, y} | {points}`); `None` when the doc doesn't start with one.
+pub(crate) fn doc_items(doc: &'static str) -> Option<Vec<&'static str>> {
+    let mut body = doc.trim_start().strip_prefix('{')?;
+    let (mut items, mut depth, mut start) = (Vec::new(), 0usize, 0);
+    loop {
+        let mut rest = None;
+        for (i, c) in body.char_indices() {
+            match c {
+                '{' | '[' | '(' => depth += 1,
+                '}' | ']' | ')' if depth > 0 => depth -= 1,
+                '}' | ',' if depth == 0 => {
+                    items.push(body.get(start..i).unwrap_or_default());
+                    start = i + c.len_utf8();
+                    if c == '}' {
+                        rest = body.get(start..);
+                        break;
+                    }
+                }
+                _ => {}
+            }
+        }
+        let rest = rest?;
+        match rest.trim_start().strip_prefix('|').and_then(|r| r.trim_start().strip_prefix('{')) {
+            Some(next) => (body, start) = (next, 0),
+            None => return Some(items),
+        }
+    }
+}
+
+/// The names one doc item gives: its first word and the words joined to it by `|`, `/` or `+`
+/// (`id?/ids?`, `path | data (base64) + name`, `value? + newValue?`), without the `?` of optional
+/// ones. Bracketed text describes a name, and what follows a `:`, `=` or `→` its value.
+pub(crate) fn item_names(item: &'static str) -> Vec<&'static str> {
+    let mut outside = Vec::new();
+    let (mut depth, mut start) = (0usize, None);
+    for (i, c) in item.char_indices() {
+        let value_follows = depth == 0 && matches!(c, ':' | '=' | '→');
+        let in_word = depth == 0 && !value_follows && !c.is_whitespace() && !matches!(c, '(' | '[' | '{' | ')' | ']' | '}');
+        match c {
+            '(' | '[' | '{' => depth += 1,
+            ')' | ']' | '}' => depth = depth.saturating_sub(1),
+            _ => {}
+        }
+        match (in_word, start) {
+            (true, None) => start = Some(i),
+            (false, Some(s)) => {
+                outside.push(item.get(s..i).unwrap_or_default());
+                start = None;
+            }
+            _ => {}
+        }
+        if value_follows {
+            break;
+        }
+    }
+    if let Some(s) = start {
+        outside.push(item.get(s..).unwrap_or_default());
+    }
+    let is_name =
+        |n: &&str| n.chars().next().is_some_and(|c| c.is_ascii_alphabetic() || c == '_') && n.chars().all(|c| c.is_ascii_alphanumeric() || c == '_');
+    let mut words = outside.into_iter();
+    let (mut names, mut word) = (Vec::new(), words.next());
+    while let Some(w) = word {
+        names.extend(w.split(['/', '|']).map(|n| n.trim_end_matches('?')).filter(is_name));
+        word = match words.next() {
+            Some("|" | "/" | "+") => words.next(),
+            _ => None,
+        };
+    }
+    names
+}
+
+/// Every name a `{…}` params doc lists, once each (see [`item_names`]).
+fn doc_param_names(doc: &'static str) -> Option<Vec<&'static str>> {
+    let mut names = Vec::new();
+    for n in doc_items(doc)?.into_iter().flat_map(item_names) {
+        if !names.contains(&n) {
+            names.push(n);
+        }
+    }
+    Some(names)
+}
+
 /// `{"ok": true}` shorthand.
 pub fn ok() -> Result<Value> {
     Ok(serde_json::json!({"ok": true}))

crates/engine/src/history_behaviour_tests.rs

@@ -188,7 +188,7 @@ fn threads_replies_and_resolving() {
             |c| c.replies.iter().map(|m| m.text.as_str()).eq(["Sure", "Done"]),
             "replies stay in the order they were written",
         ),
-        ("comment.resolve", json!({"id": c, "author": "Sam"}), |c| c.resolved, "anyone in the file can resolve a thread, not just its author"),
+        ("comment.resolve", json!({"id": c}), |c| c.resolved, "anyone in the file can resolve a thread, not just its author (it takes no author)"),
         ("comment.resolve", json!({"id": c, "resolved": false}), |c| !c.resolved, "a resolved thread can be reopened"),
         ("comment.resolve", json!({"id": c}), |c| c.resolved, "resolving again marks it resolved"),
         (

crates/engine/src/lib.rs

@@ -484,6 +484,8 @@ mod tiff_tests;
 #[cfg(test)]
 mod transform_behaviour_tests;
 #[cfg(test)]
+mod unknown_params_tests;
+#[cfg(test)]
 mod variable_tests;
 #[cfg(test)]
 mod variables_behaviour_tests;

crates/engine/src/live_tests.rs

@@ -158,8 +158,8 @@ fn viewport(page: NodeId, x: f64, y: f64, w: f64, h: f64) -> Presence {
 #[test]
 fn without_a_link_live_sessions_are_unavailable() {
     let mut s = Session::new();
-    for cmd in ["collab.host", "collab.join"] {
-        let e = s.execute(cmd, &json!({"link": "figcraft://join?host=a:1&file=f&token=t"})).unwrap_err();
+    for (cmd, p) in [("collab.host", json!({})), ("collab.join", json!({"link": "figcraft://join?host=a:1&file=f&token=t"}))] {
+        let e = s.execute(cmd, &p).unwrap_err();
         assert!(matches!(e, EngineError::Disabled(..)), "{cmd}: {e}");
     }
     assert_eq!(s.execute("collab.leave", &json!({})).unwrap()["ended"], false);

crates/engine/src/session/exec.rs

@@ -18,6 +18,7 @@ impl Session {
     /// as it was.
     pub fn execute(&mut self, id: &str, params: &Value) -> Result<Value> {
         let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?;
+        crate::cmd::refuse_unknown_params(id, params)?;
         board::refuse_on_board(self, id)?;
         // Revert replaces the document's uid: refuse before ending previews or syncing branches.
         if id == "file.revert" && self.live().doc_uid.is_some() {

crates/engine/src/tests.rs

@@ -478,11 +478,19 @@ fn every_command_survives_junk_params() {
             continue;
         }
         for j in &junk {
-            let mut s = sess();
-            let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10}));
-            let _ = s.execute("text.create", &json!({"text": "t"}));
-            if let Err(EngineError::Internal { cmd, msg }) = s.execute(spec.id, j) {
-                panic!("{cmd} panicked on {j}: {msg}");
+            // Each junk object as given (names the command doesn't take are refused) and cut down
+            // to the names it takes, so the junk values still reach its parsers.
+            let mut taken = j.clone();
+            if let (Some(names), Some(o)) = (crate::cmd::param_names(spec.id), taken.as_object_mut()) {
+                o.retain(|k, _| matches!(k.as_str(), "ids" | "id") || names.contains(&k.as_str()));
+            }
+            for params in if taken == *j { vec![j] } else { vec![j, &taken] } {
+                let mut s = sess();
+                let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10}));
+                let _ = s.execute("text.create", &json!({"text": "t"}));
+                if let Err(EngineError::Internal { cmd, msg }) = s.execute(spec.id, params) {
+                    panic!("{cmd} panicked on {params}: {msg}");
+                }
             }
         }
     }

crates/engine/src/unknown_params_tests.rs

@@ -0,0 +1,85 @@
+//! A parameter name a command doesn't take is refused, not ignored: a misspelt name would make the
+//! command answer as if it hadn't been given (#1413).
+
+use serde_json::{Value, json};
+
+use crate::Session;
+use crate::cmd::{all_commands, param_names};
+
+fn err(s: &mut Session, cmd: &str, params: Value) -> String {
+    match s.execute(cmd, &params) {
+        Err(e) => e.to_string(),
+        Ok(v) => panic!("{cmd} {params} ran: {v}"),
+    }
+}
+
+/// The issue's repro: a misspelt `filter` listed the resolved thread as if no filter were given.
+#[test]
+fn a_misspelt_parameter_is_refused_and_named() {
+    let mut s = Session::new();
+    s.execute("comment.add", &json!({"x": 10, "y": 10, "text": "a"})).unwrap();
+    s.execute("comment.resolve", &json!({"id": 1})).unwrap();
+    let e = err(&mut s, "comment.list", json!({"filterr": "open"}));
+    assert!(e.contains("unknown parameter `filterr`") && e.contains("it takes page, filter, sort"), "{e}");
+    assert_eq!(s.execute("comment.list", &json!({"filter": "open"})).unwrap()["comments"].as_array().map(Vec::len), Some(0));
+    let e = err(&mut s, "edit.undo", json!({"steps": 2, "stepz": 1}));
+    assert!(e.contains("unknown parameters `steps`, `stepz` (it takes none)"), "{e}");
+}
+
+/// `ids` and `id` name the layers a command acts on, documented or not (`params::targets` reads
+/// either), so `{id}` keeps working where a doc only says `ids?`.
+#[test]
+fn ids_and_id_are_always_taken() {
+    let mut s = Session::new();
+    let r = crate::params::id_of(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap()["id"]).unwrap();
+    assert_eq!(param_names("align.left"), Some(&["ids"][..]));
+    s.execute("align.left", &json!({"id": r})).unwrap();
+    s.execute("edit.undo", &json!({"ids": []})).unwrap();
+}
+
+/// Names a doc gives as alternatives (`id? | ids?`, `id?/ids?`, `path | data`) are all taken, and
+/// `null` params or no params at all stay fine.
+#[test]
+fn documented_alternatives_and_empty_params_are_taken() {
+    assert_eq!(param_names("vector.fillRule").map(|n| n.contains(&"id") && n.contains(&"ids")), Some(true));
+    assert_eq!(param_names("text.setFeatures").map(|n| n.contains(&"id") && n.contains(&"ids")), Some(true));
+    assert_eq!(param_names("file.import").map(|n| n.contains(&"path") && n.contains(&"data") && n.contains(&"name")), Some(true));
+    assert_eq!(param_names("file.export").map(|n| n.contains(&"scale") && n.contains(&"size")), Some(true));
+    assert_eq!(param_names("collab.share").map(|n| n.contains(&"via") && !n.contains(&"local")), Some(true));
+    assert_eq!(param_names("component.editProperty").map(|n| n.contains(&"value") && n.contains(&"newValue")), Some(true));
+    // Docs whose shape used to hide names: a stray brace, a comma in an example, a second object.
+    assert_eq!(param_names("proto.interaction.add").map(|n| n.contains(&"delayMs") && n.contains(&"dest")), Some(true));
+    assert_eq!(param_names("collab.join"), Some(&["code", "link"][..]));
+    assert_eq!(param_names("proto.interaction.set").map(|n| n.contains(&"trigger") && n.contains(&"interaction")), Some(true));
+    // Alternative groups: `{x, y} | {points: …}`.
+    assert_eq!(param_names("vector.edit.paint"), Some(&["x", "y", "points"][..]));
+    assert_eq!(param_names("vector.edit.cut"), Some(&["segment", "t", "vertex", "from", "to"][..]));
+    // `node.set` documents its props and checks them itself.
+    assert_eq!(param_names("node.set"), None);
+    let mut s = Session::new();
+    for p in [json!(null), json!({})] {
+        s.execute("select.all", &p).unwrap();
+    }
+}
+
+/// Every top-level item of every `{…}` params doc gives a name, so no parameter a doc describes is
+/// refused.
+#[test]
+fn every_doc_item_names_a_parameter() {
+    let nameless: Vec<String> = all_commands()
+        .iter()
+        .flat_map(|c| crate::cmd::doc_items(c.params).unwrap_or_default().into_iter().map(move |i| (c.id, i)))
+        .filter(|(_, i)| !i.trim().is_empty() && crate::cmd::item_names(i).is_empty())
+        .map(|(id, i)| format!("{id}: `{i}`"))
+        .collect();
+    assert!(nameless.is_empty(), "doc items without a name: {nameless:#?}");
+}
+
+/// A batch step's unknown parameter is refused, naming the step.
+#[test]
+fn batch_steps_refuse_unknown_parameters() {
+    let mut s = Session::new();
+    let e =
+        err(&mut s, "command.batch", json!({"commands": [{"command": "select.all"}, {"command": "comment.list", "params": {"filterr": "open"}}]}));
+    assert!(e.contains("comment.list") && e.contains("step 1: unknown parameter `filterr`"), "{e}");
+}