Merge PR #97 from mikl0s: Commands: an invalid shape id is an error instead of the selection

crates/engine/src/cmd/animation.rs

@@ -305,7 +305,7 @@ fn get(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn painter_pick(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
-    let id = id_param(p, "id")
+    let id = id_param(p, "id", "animation.painter")?
         .or_else(|| st.selection.shapes.first().copied())
         .ok_or_else(|| bad("animation.painter", "select an animated shape first"))?;
     let sl = st.current_slide().ok_or_else(|| bad("animation.painter", "no slide"))?;

crates/engine/src/cmd/arrange.rs

@@ -129,7 +129,7 @@ fn order_list(list: &mut Vec<Shape>, ids: &[ShapeId], how: i32) {
 /// Move one shape to `index` among its siblings (0 = back): the Selection pane's drag and drop.
 /// A shape stays in the list that holds it, so a group's child stays in its group.
 fn reorder(s: &mut Session, p: &Value) -> Result<Value> {
-    let id = id_param(p, "id").ok_or_else(|| bad("arrange.reorder", "missing `id`"))?;
+    let id = id_param(p, "id", "arrange.reorder")?.ok_or_else(|| bad("arrange.reorder", "missing `id`"))?;
     let to = usize_param(p, "index").ok_or_else(|| bad("arrange.reorder", "missing `index`"))?;
     let st = s.doc()?;
     let siblings: &[Shape] = match deckcraft_model::parent_of(st.shapes(), id) {

crates/engine/src/cmd/edit.rs

@@ -353,7 +353,10 @@ fn deselect(s: &mut Session, _p: &Value) -> Result<Value> {
 }
 
 fn select(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = ids_param(p, "ids").or_else(|| id_param(p, "id").map(|i| vec![i])).unwrap_or_default();
+    let ids = match ids_param(p, "ids") {
+        Some(v) => v,
+        None => id_param(p, "id", "edit.select")?.map(|i| vec![i]).unwrap_or_default(),
+    };
     let add = bool_or(p, "add", false);
     let toggle = bool_or(p, "toggle", false);
     let valid: Vec<ShapeId> = ids.into_iter().filter(|i| s.active().is_some_and(|d| d.shape(*i).is_some())).collect();

crates/engine/src/cmd/inspect.rs

@@ -136,7 +136,8 @@ fn slide(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn shape(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
-    let id = id_param(p, "id").or_else(|| st.selection.shapes.first().copied()).ok_or_else(|| bad("shape.inspect", "missing `id`"))?;
+    let id =
+        id_param(p, "id", "shape.inspect")?.or_else(|| st.selection.shapes.first().copied()).ok_or_else(|| bad("shape.inspect", "missing `id`"))?;
     let sh = st.shape(id).ok_or_else(|| bad("shape.inspect", format!("no shape {id}")))?;
     let x = xfrm_of(&st.doc, &st.selection, sh);
     Ok(json!({"shape": serde_json::to_value(sh).unwrap_or_default(), "box": x}))

crates/engine/src/cmd/mod.rs

@@ -173,8 +173,17 @@ pub(crate) fn str_param<'a>(p: &'a Value, key: &str) -> Option<&'a str> {
 pub(crate) fn usize_param(p: &Value, key: &str) -> Option<usize> {
     p.get(key).and_then(Value::as_u64).and_then(|v| usize::try_from(v).ok())
 }
-pub(crate) fn id_param(p: &Value, key: &str) -> Option<ShapeId> {
-    p.get(key).and_then(Value::as_u64).and_then(|v| u32::try_from(v).ok()).map(ShapeId)
+/// A shape id param: `Ok(None)` when absent (callers may fall back to the selection), `Err` when
+/// present but not a valid id (negative, non-integer, beyond `u32`), so a bad id never edits the selection.
+pub(crate) fn id_param(p: &Value, key: &str, cmd: &str) -> Result<Option<ShapeId>> {
+    match p.get(key) {
+        None | Some(Value::Null) => Ok(None),
+        Some(v) => v
+            .as_u64()
+            .and_then(|v| u32::try_from(v).ok())
+            .map(|v| Some(ShapeId(v)))
+            .ok_or_else(|| bad(cmd, format!("`{key}` is not a valid shape id: {v}"))),
+    }
 }
 pub(crate) fn ids_param(p: &Value, key: &str) -> Option<Vec<ShapeId>> {
     p.get(key).and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_u64).filter_map(|v| u32::try_from(v).ok()).map(ShapeId).collect())
@@ -198,7 +207,7 @@ pub(crate) fn targets(s: &Session, p: &Value) -> Result<Vec<ShapeId>> {
     {
         return Ok(ids);
     }
-    if let Some(id) = id_param(p, "id") {
+    if let Some(id) = id_param(p, "id", "shape")? {
         return Ok(vec![id]);
     }
     Ok(s.doc()?.selection.shapes.clone())

crates/engine/src/cmd/review.rs

@@ -60,7 +60,7 @@ fn now() -> String {
 fn add(s: &mut Session, p: &Value) -> Result<Value> {
     let text = str_param(p, "text").ok_or_else(|| bad("comment.add", "missing `text`"))?.to_string();
     let author = s.prefs.author.clone();
-    let shape = id_param(p, "id").or_else(|| s.active().and_then(|d| d.selection.shapes.first().copied()));
+    let shape = id_param(p, "id", "comment.add")?.or_else(|| s.active().and_then(|d| d.selection.shapes.first().copied()));
     let (x, y) = (f64_or(p, "x", 10.0), f64_or(p, "y", 10.0));
     s.edit(|doc, sel| {
         let sl = Arc::make_mut(doc.slides.get_mut(sel.slide).ok_or_else(|| bad("comment.add", "no slide"))?);

crates/engine/src/cmd/table.rs

@@ -78,7 +78,7 @@ fn at(s: &Session, p: &Value) -> (usize, usize) {
 }
 
 fn with_table(s: &mut Session, p: &Value, cmd: &str, f: impl Fn(&mut Table) -> Result<()>) -> Result<Value> {
-    let id = match id_param(p, "id") {
+    let id = match id_param(p, "id", cmd)? {
         Some(i) => i,
         None => {
             let st = s.doc()?;
@@ -257,7 +257,7 @@ fn row_height(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn select_cells(s: &mut Session, p: &Value) -> Result<Value> {
-    let id = match id_param(p, "id") {
+    let id = match id_param(p, "id", "table.selectCells")? {
         Some(i) => i,
         None => s.doc()?.selection.shapes.first().copied().ok_or_else(|| bad("table.selectCells", "select a table"))?,
     };

crates/engine/src/cmd/text.rs

@@ -218,7 +218,7 @@ fn edit(s: &mut Session, p: &Value) -> Result<Value> {
         })?;
         return ok();
     }
-    let id = match id_param(p, "id") {
+    let id = match id_param(p, "id", "text.edit")? {
         Some(i) => i,
         None => s.doc()?.selection.shapes.first().copied().ok_or_else(|| bad("text.edit", "select a shape with text"))?,
     };
@@ -703,7 +703,7 @@ pub(crate) fn apply_leading_tabs(para: &mut deckcraft_model::text::Paragraph) ->
 
 fn set(s: &mut Session, p: &Value) -> Result<Value> {
     let text = str_param(p, "text").ok_or_else(|| bad("text.set", "missing `text`"))?.to_string();
-    let id = match id_param(p, "id") {
+    let id = match id_param(p, "id", "text.set")? {
         Some(i) => i,
         None => s.doc()?.selection.shapes.first().copied().ok_or_else(|| bad("text.set", "missing `id` (or select a shape)"))?,
     };
@@ -739,7 +739,7 @@ fn set(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn get(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
-    let id = match id_param(p, "id") {
+    let id = match id_param(p, "id", "text.get")? {
         Some(i) => i,
         None => match &st.selection.text {
             Some(t) => t.shape,

crates/mcp/src/tests.rs

@@ -79,6 +79,47 @@ fn agent_builds_a_deck() {
     assert_eq!(n2, n1 + 1);
 }
 
+#[test]
+fn set_text_unknown_id_is_error() {
+    let mut s = Server::new(Box::new(Headless::new()));
+    tool(&mut s, "new_presentation", json!({"theme": "Harbor"}));
+    let title_id = tool(&mut s, "inspect_slide", json!({}))["shapes"][0]["id"].as_u64().unwrap();
+    tool(&mut s, "select", json!({"ids": [title_id]}));
+    // -1 and 2^32 are not shape ids; they must not fall back to the selection.
+    for bad in [json!(999999), json!(-1), json!(4294967296u64)] {
+        let r = call(&mut s, 1, "tools/call", json!({"name": "set_text", "arguments": {"id": bad, "text": "x"}}));
+        assert_eq!(r["result"]["isError"], true, "{bad}: {r}");
+    }
+    // A real id, and a just-created shape, still work; omitting id still uses the selection.
+    tool(&mut s, "set_text", json!({"id": title_id, "text": "Real"}));
+    let sid = tool(&mut s, "add_shape", json!({"preset": "rect", "x": 10, "y": 10, "w": 50, "h": 50}))["id"].as_u64().unwrap();
+    tool(&mut s, "set_text", json!({"id": sid, "text": "Fresh"}));
+    tool(&mut s, "select", json!({"ids": [title_id]}));
+    tool(&mut s, "set_text", json!({"text": "Selected"}));
+    let sl = tool(&mut s, "inspect_slide", json!({}));
+    let text_of = |id: u64| sl["shapes"].as_array().unwrap().iter().find(|x| x["id"].as_u64() == Some(id)).unwrap()["text"].clone();
+    assert_eq!(text_of(sid), "Fresh");
+    assert_eq!(text_of(title_id), "Selected");
+}
+
+#[test]
+fn invalid_id_never_falls_back_to_selection() {
+    let mut s = Server::new(Box::new(Headless::new()));
+    tool(&mut s, "new_presentation", json!({"theme": "Harbor"}));
+    let title_id = tool(&mut s, "inspect_slide", json!({}))["shapes"][0]["id"].as_u64().unwrap();
+    tool(&mut s, "select", json!({"ids": [title_id]}));
+    for cmd in ["text.edit", "text.get", "shape.inspect", "table.selectCells", "arrange.reorder"] {
+        for bad in [json!(-1), json!(1.5), json!("7"), json!(4294967296u64)] {
+            let r = call(&mut s, 1, "tools/call", json!({"name": "run_command", "arguments": {"command": cmd, "params": {"id": bad, "index": 0}}}));
+            assert_eq!(r["result"]["isError"], true, "{cmd} {bad}: {r}");
+        }
+    }
+    // Absent id still means the selection, and a real id still works.
+    tool(&mut s, "run_command", json!({"command": "text.get", "params": {}}));
+    tool(&mut s, "run_command", json!({"command": "text.get", "params": {"id": title_id}}));
+    tool(&mut s, "run_command", json!({"command": "shape.inspect", "params": {"id": title_id}}));
+}
+
 #[test]
 fn errors_are_reported_not_panics() {
     let mut s = Server::new(Box::new(Headless::new()));