Fix #3965: a malformed or unknown id is refused under the command's name, never replaced by the selection

crates/engine/src/cmd/gradient.rs

@@ -586,7 +586,9 @@ fn set_gradient_geom(s: &mut Session, p: &Value) -> Result<Value> {
     let stroke = edits_stroke(s, p, item, !s.fill_active, C)?;
     let item = item.of_kind(s, !stroke);
     let targets = match item {
-        ItemTarget::Top => leaf_targets(s, &super::given_objects(p).map_or_else(|| selected_roots(s), |ids| Ok(outermost(&s.doc()?.doc, ids)))?)?,
+        ItemTarget::Top => {
+            leaf_targets(s, &super::given_objects(s, p, C)?.map_or_else(|| selected_roots(s), |ids| Ok(outermost(&s.doc()?.doc, ids)))?)?
+        }
         _ => item.targets(s, p)?,
     };
     if targets.is_empty() {

crates/engine/src/cmd/mod.rs

@@ -367,12 +367,20 @@ pub(crate) fn id_param(p: &Value, key: &str) -> Option<NodeId> {
 pub(crate) fn ids_param(p: &Value, key: &str) -> Option<Vec<NodeId>> {
     p.get(key).and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).map(NodeId).collect())
 }
-/// The objects `p` gives: its `ids`, else its `id` as `ids` of one (as [`targets`] reads them);
-/// `None` when it gives neither.
-pub(crate) fn given_objects(p: &Value) -> Option<Vec<NodeId>> {
-    match p.get("ids").filter(|v| !v.is_null()) {
-        Some(_) => ids_param(p, "ids"),
-        None => id_param(p, "id").map(|id| vec![id]),
+/// The objects `p` gives `command`: its `ids`, else its `id` as `ids` of one (as [`targets`] reads
+/// them), each checked ([`checked_id`]): a malformed one, or one naming no object, is an error,
+/// never the selection. `None` when it gives neither (or gives them as `null`).
+pub(crate) fn given_objects(s: &Session, p: &Value, command: &str) -> Result<Option<Vec<NodeId>>> {
+    if p.get("ids").is_some_and(|v| !v.is_null()) {
+        return checked_ids_param(s, p, "ids", command).map(Some);
+    }
+    match p.get("id").filter(|v| !v.is_null()) {
+        None => Ok(None),
+        // A step reference a batch couldn't resolve, as `targets` reports it.
+        Some(Value::String(r)) if r.starts_with('$') => {
+            Err(EngineError::Other(format!("`id` is the step reference {r:?}, which only `run` and `command_batch` resolve")))
+        }
+        Some(v) => Ok(Some(vec![checked_id(s, v, command)?])),
     }
 }
 /// `v` as an index, a count or a byte offset: past `usize::MAX` reads as `usize::MAX`, past the end of any list.

crates/engine/src/cmd/paint.rs

@@ -684,7 +684,7 @@ fn default_paint(s: &mut Session, p: &Value) -> Result<Value> {
 /// active item of the proxy's kind is recoloured in each selected object's own stack instead.
 fn proxy_recolor(s: &mut Session, p: &Value, cmd: &str, label: &str, f: &dyn Fn(Color) -> Color) -> Result<Value> {
     let stroke = stroke_param(s, p, cmd)?;
-    let ids = match super::given_objects(p) {
+    let ids = match super::given_objects(s, p, cmd)? {
         Some(ids) => ids,
         None => super::edit::selected_roots(s)?,
     };

crates/engine/src/cmd/params.rs

@@ -161,6 +161,32 @@ pub(crate) fn takes_key(doc: &'static str, key: &str) -> bool {
     doc_keys(doc).is_ok_and(|parsed| parsed.keys.contains(&key)) || accepted(doc, 0).is_some_and(|keys| keys.contains(&key))
 }
 
+/// Does the description `doc` take `id` as `ids` of one object (`{ids?|id?}`, `{id?, ids?: …
+/// (instead of id)}`, `{id} or {ids: […]}`): it names both keys, and no params object of it names
+/// `ids` beside a required `id`, which is something else (`plugin.run {id, ids?}`: the plug-in's)?
+pub(crate) fn id_stands_for_ids(doc: &'static str) -> bool {
+    fn head(alternative: &str) -> &str {
+        alternative.split([':', '=', '(']).next().unwrap_or_default().trim()
+    }
+    if !takes_key(doc, "ids") || !takes_key(doc, "id") {
+        return false;
+    }
+    let mut rest = doc;
+    while let Ok((body, after)) = object_body(rest) {
+        let entries = split_top(body, ',');
+        let required_id = entries.iter().any(|e| split_top(e, '|').len() == 1 && head(e) == "id");
+        let names_ids = entries.iter().any(|e| split_top(e, '|').iter().any(|a| head(a).trim_end_matches('?').trim_end() == "ids"));
+        if required_id && names_ids {
+            return false;
+        }
+        match after.trim_start().strip_prefix("or ").or_else(|| after.trim_start().strip_prefix("| ")).filter(|r| r.starts_with('{')) {
+            Some(next) => rest = next,
+            None => break,
+        }
+    }
+    true
+}
+
 /// The edits (insert, delete, replace, swap of neighbours) that turn `a` into `b`, ignoring case.
 fn distance(a: &str, b: &str) -> usize {
     let (a, b): (Vec<char>, Vec<char>) = (a.chars().map(|c| c.to_ascii_lowercase()).collect(), b.chars().map(|c| c.to_ascii_lowercase()).collect());

crates/engine/src/lib.rs

@@ -1760,6 +1760,12 @@ impl Session {
         // passes that its command's description doesn't name fails the test.
         #[cfg(test)]
         self.check_params(id, params)?;
+        // Objects given as `id` (where it stands for `ids` of one) or as `ids` beside it are checked
+        // before anything else: a malformed one, or one naming no object, is an error under this
+        // command's id, never the selection in its place.
+        if cmd::params::id_stands_for_ids(spec.params) && self.active().is_some() {
+            cmd::given_objects(self, params, id)?;
+        }
         // Clear's menu needs a selection; an agent supplying explicit targets only needs a
         // document. The command validates those targets before making any edit.
         let given = given_ids(spec.params, params);
@@ -2791,12 +2797,15 @@ pub(crate) const SELECTS_GIVEN_IDS: &[&str] = &[
 
 /// The non-empty `ids` given to a command whose params description names `ids` as a key (the
 /// key callers outside the app may pass it, [`cmd::params::takes_key`]), or its `id` as `ids` of
-/// one when the description names that key too and no `ids` are given, as [`cmd::targets`] reads
-/// them.
+/// one when no `ids` are given and the description takes `id` so
+/// ([`cmd::params::id_stands_for_ids`]), as [`cmd::targets`] reads them.
 fn given_ids(description: &'static str, params: &Value) -> Option<Vec<NodeId>> {
-    let by_id = params.get("ids").is_none_or(Value::is_null);
-    if !cmd::params::takes_key(description, "ids") || (by_id && !cmd::params::takes_key(description, "id")) {
+    if !cmd::params::takes_key(description, "ids") {
         return None;
     }
-    cmd::given_objects(params).filter(|ids| !ids.is_empty())
+    match params.get("ids").filter(|v| !v.is_null()) {
+        Some(_) => cmd::ids_param(params, "ids"),
+        None => cmd::id_param(params, "id").filter(|_| cmd::params::id_stands_for_ids(description)).map(|id| vec![id]),
+    }
+    .filter(|ids| !ids.is_empty())
 }

crates/engine/src/tests_ids_without_selection.rs

@@ -62,6 +62,12 @@ fn only_an_ids_key_counts() {
     assert_eq!(given_ids("{ids?|id?}", &json!({"id": "a1"})), None, "not an object id");
     assert_eq!(given_ids("{ids?|id?}", &json!({"id": 3, "ids": [4]})), Some(vec![NodeId(4)]), "`ids` first");
     assert_eq!(given_ids("{ids?|id?}", &json!({"id": 3, "ids": null})), Some(vec![NodeId(3)]));
+    // A required `id` beside `ids` is something else (`plugin.run`'s plug-in); `{id} or {ids}` is
+    // one object or several.
+    assert_eq!(given_ids("{id, params?: {…}, ids?: [..]}", &one), None, "a plug-in's id");
+    assert!(!cmd::params::id_stands_for_ids("{id, params?: {…}, ids?: [..]}"));
+    assert!(cmd::params::id_stands_for_ids("{id} or {ids: [id…]}: toggles them"));
+    assert!(cmd::params::id_stands_for_ids("{ids|id, target}"));
 }
 
 /// `{"id": x}` with params besides it, and the same as `{"ids": [x]}`.
@@ -102,7 +108,7 @@ fn id_does_what_ids_of_one_does_with_nothing_selected() {
     let (fresh, [a, inner, _, d], t) = doc_with_type();
     let (mut checked, mut failures, mut refused, mut refused_ran) = (vec![], vec![], vec![], vec![]);
     for spec in command_specs() {
-        if !cmd::params::takes_key(spec.params, "ids") || !cmd::params::takes_key(spec.params, "id") {
+        if !cmd::params::id_stands_for_ids(spec.params) {
             continue;
         }
         // Those the menus refuse with nothing selected are the ones that refused `id`.
@@ -127,6 +133,46 @@ fn id_does_what_ids_of_one_does_with_nothing_selected() {
     assert!(refused.len() >= 26 && refused_ran.len() >= 14, "{refused_ran:?} ran of {refused:?}; checked {}", checked.len());
 }
 
+/// Every command that takes `id` as `ids` of one ([`cmd::params::id_stands_for_ids`]), with an
+/// object selected: an `id` that is malformed (a string, negative, a fraction, an object) or names
+/// no object is refused under the command's id, as the same value in `ids` is, and the document
+/// and the selection stay as they were. It never stands for the selection instead.
+#[test]
+fn a_bad_id_is_refused_as_the_same_ids_are_and_changes_nothing() {
+    let (_, [.., d], _) = doc_with_type();
+    let (mut failures, mut checked) = (vec![], 0);
+    for spec in command_specs() {
+        if !cmd::params::id_stands_for_ids(spec.params) {
+            continue;
+        }
+        checked += 1;
+        for bad in [json!("bad"), json!(-1), json!(1.5), json!({"id": 1}), json!(999_999)] {
+            let run = |key: &str, value: serde_json::Value| {
+                let (mut s, ..) = doc_with_type();
+                s.execute("select.set", &json!({ "ids": [d] })).unwrap();
+                let before = s.doc().unwrap().doc.clone();
+                let mut p = params_for(spec.id, d);
+                p[key] = value;
+                let r = s.execute(spec.id, &p).map_err(|e| e.to_string());
+                let st = s.doc().unwrap();
+                (r, st.doc == before && st.selection.objects == vec![NodeId(d)])
+            };
+            let (with_id, with_ids) = (run("id", bad.clone()), run("ids", json!([bad.clone()])));
+            if with_id.0.is_ok() || !with_id.1 || with_id != with_ids {
+                failures.push(format!(
+                    "{} {bad}: with id {:.160} (unchanged: {}); with ids {:.160}",
+                    spec.id,
+                    format!("{:?}", with_id.0),
+                    with_id.1,
+                    format!("{:?}", with_ids.0)
+                ));
+            }
+        }
+    }
+    assert!(failures.is_empty(), "{} fail:\n{}", failures.len(), failures.join("\n"));
+    assert!(checked >= 100, "{checked} commands");
+}
+
 /// Every command that names `ids` and is refused with nothing selected: given the object as `ids`
 /// it does what it does with the object selected (the same result or the same error, and the same
 /// document), so no command is let through to run on an empty selection.

docs/mcp.md

@@ -349,8 +349,9 @@ Coordinates are points in document space: y points down, the origin is the first
 document is 612 × 792 (US Letter). New objects become the selection. Most commands act on the selection or on
 explicit `ids`, which stand in for the selection, so they work with nothing selected. Where a command's params say
 `ids?|id?` (or `id?, ids?: … (instead of id)`), `id` is `ids` of one object: `{"id": 7}` does what `{"ids": [7]}`
-does, with or without a selection. A command that leaves out an object selected inside a selected group also leaves
-it out when `ids` list it together with a group that holds it.
+does, with or without a selection, and an `id` or `ids` that is malformed (not a non-negative integer) or names no
+object is refused under the command's name, never replaced by the selection. A command that leaves out an object
+selected inside a selected group also leaves it out when `ids` list it together with a group that holds it.
 
 Paint values (`fill`, `stroke`) accept `"#rrggbb"`, `"none"`, `[r,g,b]` (0..1), `{"c","m","y","k"}`, `{"gray"}`,
 or a full `paint.setFill` params object (`{"gradient": …}`, `{"swatch": "name"}`).

docs/target-app-parity.md

@@ -1092,6 +1092,7 @@ applies to fills). Tests: `color::gradient::tests` (perceptual midpoint, expansi
 
 | Date | Change | Summary |
 |---|---|---|
+| 2026-10-24 | minor | Commands: a command whose params take `ids?|id?` (or `id?, ids?: … (instead of id)`) takes `id` as `ids` of one with nothing selected too, instead of refusing it as "nothing selected" (29 such commands needed a selection); Invert, Complement, Gradient Vector and Make Pixel Perfect act on the object given as `id`; an `id` or `ids` that is malformed or names no object is refused under the command's name instead of the selection standing in for it (`plugin.run`'s required `id`, the plug-in's, is not an object) (`tests_ids_without_selection::id_does_what_ids_of_one_does_with_nothing_selected`, `a_bad_id_is_refused_as_the_same_ids_are_and_changes_nothing`); scores unchanged |
 | 2026-10-24 | minor | Commands: a command whose params take `ids?|id?` (or `id?, ids?: … (instead of id)`) takes `id` as `ids` of one with nothing selected too, instead of refusing it as "nothing selected" (29 such commands needed a selection); Invert, Complement, Gradient Vector and Make Pixel Perfect act on the object given as `id` (`tests_ids_without_selection::id_does_what_ids_of_one_does_with_nothing_selected`); scores unchanged |
 | 2026-10-24 | minor | Type advanced: ruby (ふりがな) is kept, where it is laid out, by Type › Create Outlines and in EPS, EMF/WMF and legacy `.ai` exports (it was dropped there), see [type-parity.md](type-parity.md); scores and hours unchanged |
 | 2026-10-24 | minor | Path operations: Shape Builder pieces: the planar map contracts the sweep's stubs between two cuts of a line's passes a rounding error apart (bounded by each cluster's extent), so dense hatches at exact binary steps no longer add a speck per crossing; Path operations stays 80% and Shape Builder 70% while #937 is open (estimated) |