Fix #784: validate selection IDs and report the resulting selection

crates/engine/src/cmd/mod.rs

@@ -304,6 +304,21 @@ 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())
 }
+/// Validate all explicit targets before a command changes the document or selection.
+pub(crate) fn checked_ids_param(s: &Session, p: &Value, key: &str, command: &str) -> Result<Vec<NodeId>> {
+    let values =
+        p.get(key).and_then(Value::as_array).ok_or_else(|| bad(command, format!("{key} must be an array of non-negative integer object ids")))?;
+    values.iter().map(|value| checked_id(s, value, command)).collect()
+}
+
+pub(crate) fn checked_id(s: &Session, value: &Value, command: &str) -> Result<NodeId> {
+    let id = value.as_u64().map(NodeId).ok_or_else(|| bad(command, format!("invalid object id {value}: expected a non-negative integer")))?;
+    if s.doc()?.doc.node(id).is_none() {
+        return Err(bad(command, format!("no such object id {value}")));
+    }
+    Ok(id)
+}
+
 pub(crate) fn point_param(p: &Value, key: &str) -> Option<Point> {
     let a = p.get(key)?.as_array()?;
     Some(Point::new(a.first()?.as_f64()?, a.get(1)?.as_f64()?))

crates/engine/src/cmd/select.rs

@@ -25,14 +25,30 @@ pub fn specs() -> Vec<CommandSpec> {
         cmd!("select.inverse", "Inverse", ["Select"], None, "{}", has_doc, inverse),
         cmd!("select.nextAbove", "Next Object Above", ["Select"], Some("Cmd+Alt+]"), "{}", has_selection, |s, _| step(s, 1)),
         cmd!("select.nextBelow", "Next Object Below", ["Select"], Some("Cmd+Alt+["), "{}", has_selection, |s, _| step(s, -1)),
-        cmd!("select.set", "Select Objects", [], None, "{ids: [id…]}", has_doc, set),
-        cmd!("select.add", "Add to Selection", [], None, "{ids: [id…]}", has_doc, add),
+        cmd!(
+            "select.set",
+            "Select Objects",
+            [],
+            None,
+            "{ids: [id…]} all ids must be non-negative integers naming existing objects → {count, ids} (resulting selection)",
+            has_doc,
+            set
+        ),
+        cmd!(
+            "select.add",
+            "Add to Selection",
+            [],
+            None,
+            "{ids: [id…]} all ids must be non-negative integers naming existing objects → {count, ids} (resulting selection)",
+            has_doc,
+            add
+        ),
         cmd!(
             "select.toggle",
             "Toggle Selection",
             [],
             None,
-            "{id} or {ids: [id…]}: each selected one leaves the selection, the others join it",
+            "{id} or {ids: [id…]}: each selected one leaves the selection, the others join it; all ids must be non-negative integers naming existing objects → {count, ids} (resulting selection)",
             has_doc,
             toggle
         ),
@@ -225,29 +241,39 @@ fn step(s: &mut Session, dir: i64) -> Result<Value> {
 }
 
 fn set(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").ok_or_else(|| bad("select.set", "missing ids"))?;
-    s.select(|d, sel| sel.set(ids.iter().copied().filter(|i| d.node(*i).is_some())))?;
-    ok()
+    let ids = checked_ids_param(s, p, "ids", "select.set")?;
+    s.select(|_, sel| sel.set(ids.iter().copied()))?;
+    selection_result(s)
 }
 
 fn add(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").ok_or_else(|| bad("select.add", "missing ids"))?;
+    let ids = checked_ids_param(s, p, "ids", "select.add")?;
     s.select(|_, sel| {
         for i in ids {
             sel.add(i);
         }
     })?;
-    ok()
+    selection_result(s)
 }
 
 fn toggle(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").or_else(|| id_param(p, "id").map(|id| vec![id])).ok_or_else(|| bad("select.toggle", "missing id or ids"))?;
+    let ids = if p.get("ids").is_some() {
+        checked_ids_param(s, p, "ids", "select.toggle")?
+    } else {
+        let value = p.get("id").ok_or_else(|| bad("select.toggle", "missing id or ids"))?;
+        vec![checked_id(s, value, "select.toggle")?]
+    };
     s.select(|_, sel| {
         for id in ids {
             sel.toggle(id);
         }
     })?;
-    ok()
+    selection_result(s)
+}
+
+fn selection_result(s: &Session) -> Result<Value> {
+    let selection = &s.doc()?.selection;
+    Ok(json!({ "count": selection.len(), "ids": selection.objects }))
 }
 
 fn key(s: &mut Session, p: &Value) -> Result<Value> {

crates/engine/tests/selection_commands.rs

@@ -0,0 +1,75 @@
+//! Selection command results and validation for agent callers (#784).
+#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
+
+use serde_json::{Value, json};
+use vectorcraft_doc::NodeId;
+use vectorcraft_engine::{EngineError, Session};
+
+fn session() -> (Session, NodeId, NodeId) {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    let mut rectangle =
+        |x| NodeId(s.execute("shape.rectangle", &json!({"x": x, "y": 0, "width": 10, "height": 10})).unwrap()["id"].as_u64().unwrap());
+    let a = rectangle(0);
+    let b = rectangle(20);
+    (s, a, b)
+}
+
+#[test]
+fn selection_commands_reject_invalid_ids_without_changing_state() {
+    for command in ["select.set", "select.add", "select.toggle"] {
+        let (mut s, a, b) = session();
+        s.execute("select.set", &json!({"ids": [b.0, a.0]})).unwrap();
+        s.execute("select.anchors", &json!({"id": a.0, "anchors": [[0, 0]], "mode": "add"})).unwrap();
+        s.execute("select.key", &json!({"id": b.0})).unwrap();
+        for invalid in [json!(999), json!(-1), json!(2.5), json!("2"), Value::Null, json!(true), json!(u64::MAX)] {
+            let selection = s.active().unwrap().selection.clone();
+            let revision = s.active().unwrap().revision;
+            let journal = s.journal.clone();
+            let error = s.execute(command, &json!({"ids": [b.0, invalid]})).unwrap_err();
+            assert!(matches!(error, EngineError::BadParams { .. }), "{command}: {error}");
+            assert!(error.to_string().contains(&invalid.to_string()), "the error must identify {invalid}: {error}");
+            assert_eq!(s.active().unwrap().selection, selection, "{command}: {invalid}");
+            assert_eq!(s.active().unwrap().revision, revision);
+            assert_eq!(s.journal, journal);
+        }
+    }
+}
+
+#[test]
+fn selection_commands_require_an_id_array() {
+    for command in ["select.set", "select.add", "select.toggle"] {
+        let (mut s, _, _) = session();
+        for params in [json!({}), json!({"ids": null}), json!({"ids": 2}), json!({"ids": "2"}), json!({"ids": true})] {
+            let selection = s.active().unwrap().selection.clone();
+            assert!(matches!(s.execute(command, &params), Err(EngineError::BadParams { .. })), "{command}: {params}");
+            assert_eq!(s.active().unwrap().selection, selection);
+        }
+    }
+}
+
+#[test]
+fn selection_commands_report_the_resulting_selection() {
+    let (mut s, a, b) = session();
+    assert_eq!(s.execute("select.set", &json!({"ids": [b.0, a.0, b.0]})).unwrap(), json!({"count": 2, "ids": [b.0, a.0]}));
+    assert_eq!(s.execute("select.set", &json!({"ids": []})).unwrap(), json!({"count": 0, "ids": []}));
+    assert_eq!(s.execute("select.add", &json!({"ids": [a.0, a.0]})).unwrap(), json!({"count": 1, "ids": [a.0]}));
+    assert_eq!(s.execute("select.add", &json!({"ids": [b.0]})).unwrap(), json!({"count": 2, "ids": [a.0, b.0]}));
+    assert_eq!(s.execute("select.toggle", &json!({"id": a.0})).unwrap(), json!({"count": 1, "ids": [b.0]}));
+    assert_eq!(s.execute("select.toggle", &json!({"ids": [b.0, a.0]})).unwrap(), json!({"count": 1, "ids": [a.0]}));
+    for command in ["select.add", "select.toggle"] {
+        assert_eq!(s.execute(command, &json!({"ids": []})).unwrap(), json!({"count": 1, "ids": [a.0]}));
+    }
+}
+
+#[test]
+fn single_id_toggle_validates_before_mutating() {
+    let (mut s, a, _) = session();
+    for invalid in [json!(999), json!(-1), json!(2.5), json!("2"), Value::Null, json!(true)] {
+        let selection = s.active().unwrap().selection.clone();
+        assert!(matches!(s.execute("select.toggle", &json!({"id": invalid})), Err(EngineError::BadParams { .. })));
+        assert_eq!(s.active().unwrap().selection, selection);
+    }
+    // A malformed array must not silently fall back to a valid single id.
+    assert!(s.execute("select.toggle", &json!({"ids": "bad", "id": a.0})).is_err());
+}