diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index 8b6fe5a7..5804fee6 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -305,7 +305,7 @@ fn get(s: &mut Session, p: &Value) -> Result { fn painter_pick(s: &mut Session, p: &Value) -> Result { 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"))?; diff --git a/crates/engine/src/cmd/arrange.rs b/crates/engine/src/cmd/arrange.rs index 0c91bbaa..06e21e5a 100644 --- a/crates/engine/src/cmd/arrange.rs +++ b/crates/engine/src/cmd/arrange.rs @@ -129,7 +129,7 @@ fn order_list(list: &mut Vec, 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 { - 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) { diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 30437b15..8d744acf 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -353,7 +353,10 @@ fn deselect(s: &mut Session, _p: &Value) -> Result { } fn select(s: &mut Session, p: &Value) -> Result { - 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 = ids.into_iter().filter(|i| s.active().is_some_and(|d| d.shape(*i).is_some())).collect(); diff --git a/crates/engine/src/cmd/inspect.rs b/crates/engine/src/cmd/inspect.rs index db162a38..fb666abc 100644 --- a/crates/engine/src/cmd/inspect.rs +++ b/crates/engine/src/cmd/inspect.rs @@ -136,7 +136,8 @@ fn slide(s: &mut Session, p: &Value) -> Result { fn shape(s: &mut Session, p: &Value) -> Result { 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})) diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index ce909b02..dcb3e8b7 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/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 { 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 { - 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> { + 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> { 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> { { 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()) diff --git a/crates/engine/src/cmd/review.rs b/crates/engine/src/cmd/review.rs index 8e3a532c..72616274 100644 --- a/crates/engine/src/cmd/review.rs +++ b/crates/engine/src/cmd/review.rs @@ -60,7 +60,7 @@ fn now() -> String { fn add(s: &mut Session, p: &Value) -> Result { 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"))?); diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index 780c0053..b4a0e31f 100644 --- a/crates/engine/src/cmd/table.rs +++ b/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 { - 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 { } fn select_cells(s: &mut Session, p: &Value) -> Result { - 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"))?, }; diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 545ce54c..e07e9cb2 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -218,7 +218,7 @@ fn edit(s: &mut Session, p: &Value) -> Result { })?; 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 { 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 { fn get(s: &mut Session, p: &Value) -> Result { 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, diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index f49a81bf..dd3a8094 100644 --- a/crates/mcp/src/tests.rs +++ b/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()));