Merge pull request #7402 from dotCooCoo/fix/more-int-param-wrong-type

MCP: path, type, panel, library, style and ruler indices refuse a wrong-type value

crates/engine/src/cmd.rs

@@ -559,6 +559,15 @@ pub(crate) fn given_object_ids(p: &Value, command: &str) -> Result<Option<Vec<No
 pub(crate) fn index(v: u64) -> usize {
     usize::try_from(v).unwrap_or(usize::MAX)
 }
+/// The whole number of 0 or more that `p` gives for `key`, read with [`index`], or `None` when `p`
+/// leaves it out or gives null. Any other value, such as `"3"`, `2.5`, `-1` or `true`, is an error
+/// under `cmd`: "`{key}` must be a whole number".
+pub(crate) fn index_opt(p: &Value, key: &str, cmd: &str) -> Result<Option<usize>> {
+    match p.get(key) {
+        None | Some(Value::Null) => Ok(None),
+        Some(v) => v.as_u64().map(|n| Some(index(n))).ok_or_else(|| bad(cmd, format!("`{key}` must be a whole number"))),
+    }
+}
 /// 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>> {
     checked_nodes_param(s, p, key, command, false)

crates/engine/src/cmd/edit.rs

@@ -568,12 +568,11 @@ fn switch_units(s: &mut Session, _: &Value) -> Result<Value> {
     Ok(json!({ "units": next.label() }))
 }
 
-/// The `artboard` param of the ruler origin commands: an artboard index, 0 when not given.
+/// The `artboard` param of the ruler origin commands: an artboard index, 0 when left out or null. A
+/// value that is not a whole number is "`artboard` must be a whole number"; one past the artboards
+/// is "index out of range".
 fn artboard_param(s: &Session, p: &Value, c: &str) -> Result<usize> {
-    let index = match p.get("artboard") {
-        None => 0,
-        Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).ok_or_else(|| bad(c, "index out of range"))?,
-    };
+    let index = super::index_opt(p, "artboard", c)?.unwrap_or(0);
     if index >= s.doc()?.doc.artboards.len() {
         return Err(bad(c, "index out of range"));
     }
@@ -583,14 +582,15 @@ fn artboard_param(s: &Session, p: &Value, c: &str) -> Result<usize> {
 fn ruler_origin(s: &mut Session, p: &Value) -> Result<Value> {
     let d = &s.doc()?.doc;
     // A document without artboards measures from its global zero point.
-    let index = if d.artboards.is_empty() && p.get("artboard").is_none() { 0 } else { artboard_param(s, p, "document.rulerOrigin")? };
+    let index =
+        if d.artboards.is_empty() && p.get("artboard").is_none_or(Value::is_null) { 0 } else { artboard_param(s, p, "document.rulerOrigin")? };
     Ok(json!(s.doc()?.doc.ruler_origin_for(index)))
 }
 
 fn set_ruler_origin(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "document.setRulerOrigin";
     let origin = Point::new(f64_req(p, "x", C)?, f64_req(p, "y", C)?);
-    let index = if s.doc()?.doc.rulers == RulerMode::Global && p.get("artboard").is_none() { 0 } else { artboard_param(s, p, C)? };
+    let index = if s.doc()?.doc.rulers == RulerMode::Global && p.get("artboard").is_none_or(Value::is_null) { 0 } else { artboard_param(s, p, C)? };
     change_ruler_origin(s, index, Some(origin), "Ruler Origin")
 }
 

crates/engine/src/cmd/library.rs

@@ -21,7 +21,7 @@ use vectorcraft_color::{Color, GradientPaint, Paint};
 use vectorcraft_doc::{Appearance, Node, NodeId, TextStyleDef};
 
 use super::clipboard::SwatchChoices;
-use super::{CommandSpec, EngineError, Result, Session, always, bad, cmd, color_value, has_doc, ids_param, index, ok, point_param, str_param};
+use super::{CommandSpec, EngineError, Result, Session, always, bad, cmd, color_value, has_doc, ids_param, index_opt, ok, point_param, str_param};
 use crate::Clipboard;
 
 /// The extension of a library file.
@@ -1649,11 +1649,11 @@ fn move_group(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "library.moveGroup";
     let id = library_id(s, p, C)?;
     let group = group_param(p, C)?;
-    let to = p.get("to").and_then(Value::as_u64).ok_or_else(|| bad(C, "missing `to` (an index)"))?;
+    let to = index_opt(p, "to", C)?.ok_or_else(|| bad(C, "missing `to` (an index)"))?;
     let (name, at) = s.libraries.change(&id, |lib| {
         let i = group_at(lib, group, C)?;
         let g = lib.groups.remove(i);
-        let at = index(to).min(lib.groups.len());
+        let at = to.min(lib.groups.len());
         let name = g.name.clone();
         lib.groups.insert(at, g);
         Ok((name, at))

crates/engine/src/cmd/panelcmds.rs

@@ -88,7 +88,7 @@ pub fn specs() -> Vec<CommandSpec> {
 fn artboard_reorder(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "artboard.reorder";
     let i = artboard_index(&s.doc()?.doc, p, C)?;
-    let to = p.get("to").and_then(Value::as_u64).map(index).ok_or_else(|| bad(C, "missing to"))?;
+    let to = super::index_opt(p, "to", C)?.ok_or_else(|| bad(C, "missing to"))?;
     s.edit("Reorder Artboards", |d, _| {
         if i >= d.artboards.len() {
             return Err(EngineError::Other("no such artboard".into()));

crates/engine/src/cmd/path.rs

@@ -12,7 +12,7 @@ use super::draw2::usize_opt;
 use super::pathops::Beside;
 use super::{
     CommandSpec, Result, Session, bad, check_numbers, check_switches, cmd, f64_or, f64_req, finite_numbers, has_anchors, has_doc, has_selected_path,
-    has_selection, ids_param, index, number_or, object_param, ok, str_param, switch_or, targets,
+    has_selection, ids_param, index_opt, number_or, object_param, ok, str_param, switch_or, targets,
 };
 use crate::EngineError;
 
@@ -170,7 +170,7 @@ pub(super) fn path_mut(d: &mut vectorcraft_doc::Document, id: NodeId) -> Result<
 fn append_anchor(s: &mut Session, p: &Value) -> Result<Value> {
     let id = object_param(p, "id", "path.appendAnchor")?.ok_or_else(|| bad("path.appendAnchor", "missing id"))?;
     let a = anchor_from_json(p, "path.appendAnchor")?;
-    let subpath = p.get("subpath").and_then(Value::as_u64).map(|u| u as usize);
+    let subpath = index_opt(p, "subpath", "path.appendAnchor")?;
     s.edit("Pen", |d, sel| {
         let path = path_mut(d, id)?;
         let si = match subpath {
@@ -208,7 +208,7 @@ fn append_anchor(s: &mut Session, p: &Value) -> Result<Value> {
 /// `at` first moves the anchor, its handles with it (the Pen's closing drag with Space held).
 fn close(s: &mut Session, p: &Value) -> Result<Value> {
     let id = object_param(p, "id", "path.close")?.ok_or_else(|| bad("path.close", "missing id"))?;
-    let subpath = p.get("subpath").and_then(Value::as_u64).map(|u| u as usize);
+    let subpath = index_opt(p, "subpath", "path.close")?;
     let handle = |key: &str| match p.get(key).filter(|h| !h.is_null()) {
         None => Ok(None),
         Some(h) => finite_numbers(h).map(|[x, y]| Some(Point::new(x, y))).ok_or_else(|| bad("path.close", format!("`{key}` must be [x, y]"))),
@@ -310,7 +310,7 @@ pub(crate) fn transform_anchors(s: &mut Session, label: &str, xf: vectorcraft_ge
 fn set_handle(s: &mut Session, p: &Value) -> Result<Value> {
     let id = object_param(p, "id", "path.setHandle")?.ok_or_else(|| bad("path.setHandle", "missing id"))?;
     let si = usize_opt(p, "subpath", "path.setHandle")?.unwrap_or(0);
-    let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad("path.setHandle", "missing anchor"))?;
+    let ai = index_opt(p, "anchor", "path.setHandle")?.ok_or_else(|| bad("path.setHandle", "missing anchor"))?;
     let out = str_param(p, "which") != Some("in");
     let pos = Point::new(f64_req(p, "x", "path.setHandle")?, f64_req(p, "y", "path.setHandle")?);
     let independent = switch_or(p, "independent", false, "path.setHandle")?;
@@ -337,7 +337,7 @@ fn set_handle(s: &mut Session, p: &Value) -> Result<Value> {
 fn move_anchor(s: &mut Session, p: &Value) -> Result<Value> {
     let id = object_param(p, "id", "path.moveAnchor")?.ok_or_else(|| bad("path.moveAnchor", "missing id"))?;
     let si = usize_opt(p, "subpath", "path.moveAnchor")?.unwrap_or(0);
-    let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad("path.moveAnchor", "missing anchor"))?;
+    let ai = index_opt(p, "anchor", "path.moveAnchor")?.ok_or_else(|| bad("path.moveAnchor", "missing anchor"))?;
     let pos = Point::new(f64_req(p, "x", "path.moveAnchor")?, f64_req(p, "y", "path.moveAnchor")?);
     // An anchor already at (x, y) changes nothing and records no undo step.
     if let Some(a) = anchor_of(s, id, si, ai)?
@@ -453,7 +453,7 @@ pub(super) fn paths_in(d: &vectorcraft_doc::Document, ids: &[NodeId]) -> Vec<Nod
 fn reverse(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "path.reverse";
     let want = p.get("reversed").map(|v| v.as_bool().ok_or_else(|| bad(C, "`reversed` must be true or false"))).transpose()?;
-    let subpath = p.get("subpath").and_then(Value::as_u64).map(|u| u as usize);
+    let subpath = index_opt(p, "subpath", C)?;
     let ids = paths_in(&s.doc()?.doc, &targets(s, p)?);
     if ids.is_empty() {
         return Err(bad(C, "select paths"));
@@ -1784,7 +1784,7 @@ fn reshape(s: &mut Session, p: &Value) -> Result<Value> {
 fn insert_anchor(s: &mut Session, p: &Value) -> Result<Value> {
     let id = object_param(p, "id", "path.insertAnchor")?.ok_or_else(|| bad("path.insertAnchor", "missing id"))?;
     let si = usize_opt(p, "subpath", "path.insertAnchor")?.unwrap_or(0);
-    let seg = p.get("segment").and_then(Value::as_u64).map(index).ok_or_else(|| bad("path.insertAnchor", "missing segment"))?;
+    let seg = index_opt(p, "segment", "path.insertAnchor")?.ok_or_else(|| bad("path.insertAnchor", "missing segment"))?;
     let t = number_or(p, "t", 0.5, "path.insertAnchor")?.clamp(0.0, 1.0);
     let idx = s.edit("Add Anchor Point", |d, sel| {
         let path = path_mut(d, id)?;

crates/engine/src/cmd/style.rs

@@ -21,7 +21,7 @@ use vectorcraft_color::Paint;
 use vectorcraft_doc::{Appearance, DEFAULT_GRAPHIC_STYLE, Document, GraphicStyle, Node, NodeId, NodeKind, Shared};
 use vectorcraft_geom::Rect;
 
-use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, has_doc, index, object_param, ok, str_param, unique_name};
+use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, has_doc, index_opt, object_param, ok, str_param, unique_name};
 use crate::EngineError;
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -585,10 +585,10 @@ fn style_merge(s: &mut Session, p: &Value) -> Result<Value> {
 fn style_move(s: &mut Session, p: &Value) -> Result<Value> {
     let cmd = "graphicStyle.move";
     let name = str_param(p, "name").ok_or_else(|| bad(cmd, "missing name"))?;
-    let to = p.get("to").and_then(Value::as_u64).ok_or_else(|| bad(cmd, "missing `to` (an index)"))?;
+    let to = index_opt(p, "to", cmd)?.ok_or_else(|| bad(cmd, "missing `to` (an index)"))?;
     let d = &s.doc()?.doc;
     let from = style_index(d, name)?;
-    let to = index(to).min(d.graphic_styles.len() - 1);
+    let to = to.min(d.graphic_styles.len() - 1);
     if from == to {
         return ok();
     }

crates/engine/src/cmd/typecmd.rs

@@ -12,8 +12,8 @@ use vectorcraft_geom::{Affine, PathData, Vec2};
 use super::edit::selected_roots;
 use super::pathops::{length_given, number_given, shape_node};
 use super::{
-    CommandSpec, EngineError, Result, Session, bad, check_numbers, cmd, color_value, f64_req, has_doc, has_selection, index, number_or, object_param,
-    ok, str_param, switch_or, targets, text_param,
+    CommandSpec, EngineError, Result, Session, bad, check_numbers, cmd, color_value, f64_req, has_doc, has_selection, index, index_opt, number_or,
+    object_param, ok, str_param, switch_or, targets, text_param,
 };
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -1376,7 +1376,7 @@ fn set_path_handle(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "text.setPathHandle";
     let id = object_param(p, "id", C)?.ok_or_else(|| bad(C, "missing id"))?;
     let si = super::draw2::usize_opt(p, "subpath", C)?.unwrap_or(0);
-    let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad(C, "missing anchor"))?;
+    let ai = index_opt(p, "anchor", C)?.ok_or_else(|| bad(C, "missing anchor"))?;
     let out = super::object::choice(p, C, "which", &["in", "out"])? != Some("in");
     let pos = vectorcraft_geom::Point::new(f64_req(p, "x", C)?, f64_req(p, "y", C)?);
     let independent = switch_or(p, "independent", false, C)?;

crates/engine/src/tests/params.rs

@@ -849,3 +849,75 @@ fn journal_entries_pass_the_check() {
         assert!(check_params(id, params).is_ok(), "{id} {params}");
     }
 }
+
+/// An index of a path, type, panel, library, style or ruler command given as something other than a
+/// whole number of 0 or more (`"3"`, `2.5`, `-1`, `true`, `[]`, `{}`) is refused as the wrong type,
+/// naming its key. A required one is not reported as missing, and an optional one (`subpath` of
+/// `path.appendAnchor`, `path.close` and `path.reverse`, `artboard` of the ruler origin commands) is
+/// not read as its default. A required one left out or null is still missing, and an optional one left
+/// out or null keeps its default. Neither changes the document or records an undo step.
+#[test]
+fn a_malformed_index_is_not_missing_or_its_default() {
+    let mut s = Session::new();
+    s.execute("file.new", &json!({})).unwrap();
+    let id = s.execute("path.create", &json!({"d": "M0 0 L10 0 M20 0 L30 0"})).unwrap()["id"].clone();
+    s.execute("library.create", &json!({"name": "Brand"})).unwrap();
+    s.execute("library.createGroup", &json!({"name": "Warm"})).unwrap();
+    let doc = s.doc().unwrap().doc.clone();
+    let undo = s.doc().unwrap().history.undo.len();
+    let with = |base: &serde_json::Value, key: &str, v: Option<serde_json::Value>| {
+        let mut p = base.clone();
+        let o = p.as_object_mut().unwrap();
+        match v {
+            Some(v) => o.insert(key.into(), v),
+            None => o.remove(key),
+        };
+        p
+    };
+    let refused = |s: &mut Session, cmd: &str, p: &serde_json::Value, want: &str| match s.execute(cmd, p) {
+        Err(EngineError::BadParams { cmd: c, msg }) => assert_eq!((c.as_str(), msg.as_str()), (cmd, want), "{p}"),
+        other => panic!("{cmd} {p}: {other:?}"),
+    };
+    let malformed = [json!("3"), json!(2.5), json!(-1), json!(true), json!([]), json!({})];
+    let handle = json!({"id": id, "x": 5, "y": 5});
+    let required = [
+        ("path.setHandle", handle.clone(), "anchor", "missing anchor"),
+        ("path.moveAnchor", handle.clone(), "anchor", "missing anchor"),
+        ("path.insertAnchor", json!({"id": id}), "segment", "missing segment"),
+        ("text.setPathHandle", handle.clone(), "anchor", "missing anchor"),
+        ("artboard.reorder", json!({"index": 0}), "to", "missing to"),
+        ("library.moveGroup", json!({"group": "Warm"}), "to", "missing `to` (an index)"),
+        ("graphicStyle.move", json!({"name": "Default"}), "to", "missing `to` (an index)"),
+    ];
+    for (cmd, base, key, missing) in &required {
+        for v in &malformed {
+            refused(&mut s, cmd, &with(base, key, Some(v.clone())), &format!("`{key}` must be a whole number"));
+        }
+        for p in [with(base, key, None), with(base, key, Some(json!(null)))] {
+            refused(&mut s, cmd, &p, missing);
+        }
+    }
+    let optional = [
+        ("path.appendAnchor", handle.clone(), "subpath"),
+        ("path.close", json!({"id": id}), "subpath"),
+        ("path.reverse", json!({"ids": [id]}), "subpath"),
+        ("document.rulerOrigin", json!({}), "artboard"),
+        ("document.setRulerOrigin", json!({"x": 5, "y": 5}), "artboard"),
+        ("document.resetRulerOrigin", json!({}), "artboard"),
+    ];
+    for (cmd, base, key) in &optional {
+        for v in &malformed {
+            refused(&mut s, cmd, &with(base, key, Some(v.clone())), &format!("`{key}` must be a whole number"));
+        }
+    }
+    // A whole number that names no artboard is still out of range.
+    refused(&mut s, "document.resetRulerOrigin", &json!({"artboard": 1}), "index out of range");
+    assert_eq!(s.doc().unwrap().doc, doc);
+    assert_eq!(s.doc().unwrap().history.undo.len(), undo);
+    // Left out or null, an optional one keeps its default.
+    for (cmd, base, key) in &optional {
+        for p in [with(base, key, None), with(base, key, Some(json!(null)))] {
+            s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd} {p}: {e}"));
+        }
+    }
+}

docs/mcp.md

@@ -1804,6 +1804,13 @@ commands fail and change nothing, and `appearance.removeItem` removes nothing. A
 the item last. A text `start` or `end` is past the end of the text. `object.slice.divide` refuses such a `rows` or
 `columns`, `recolor.reduce` keeps each color in its own row for such a `colors`, and `layer.setProps` refuses such a
 `color`. `object.setBounds` reads a `reference` past 8 as 8, as `object.transformEach` does.
+An index or place given as a value that is not a whole number of 0 or more, such as `"3"`, `2.5`, `-1` or `true`, is
+refused with `` `anchor` must be a whole number ``, naming the key: `anchor` of `path.setHandle`, `path.moveAnchor` and
+`text.setPathHandle`, `segment` of `path.insertAnchor`, `to` of `artboard.reorder`, `library.moveGroup` and
+`graphicStyle.move`, `subpath` of `path.appendAnchor`, `path.close` and `path.reverse`, and `artboard` of
+`document.rulerOrigin`, `document.setRulerOrigin` and `document.resetRulerOrigin`. A required one left out or null is
+reported as missing (`missing anchor`). Left out or null, `subpath` takes the command's default and `artboard` is 0.
+Nothing changes and no undo step is recorded.
 The blend, envelope and gradient mesh commands refuse a string or another value that is not a number with the same
 message, as in `` `steps` must be a number, not "8" ``, and change nothing: `steps`, `distance` and `value` in
 `object.blend.make` and `object.blend.options`; `bend`, `h`, `v`, `horizontalDistortion` and `verticalDistortion` in

docs/target-app-parity.md

@@ -1119,6 +1119,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
     `path.curvatureEdit`, `path.removeAnchor`, `path.convertAnchor`, `path.reshapeSegment`, `path.split`, `path.setHandle`, `path.moveAnchor` and `path.insertAnchor` refuse a `subpath` that is not a whole number of 0 or more, and `path.split` refuses such an `anchor` or `segment`; without a `subpath` they edit subpath 0. A value given that is not a whole number, such as `"1"` or `1.5`, is refused with `` `subpath` must be a whole number ``, naming its key, as are a required index of the Appearance panel and Smart Pen commands, `sourceDocument` of `edit.dropSelection` and `index` and `to` of `document.activate` and `document.move`; a required one left out or null is reported as missing (`tests::params::a_malformed_required_integer_is_not_reported_as_missing`).
     `path.reshapeSegment` refuses a delta of 1e9 pt or longer, as `object.blend.spine.reshapeSegment` does, and `path.convertAnchor` and `path.freehand` refuse a point whose x or y is 1e9 pt or more in size.
     `path.reshapeSegment`, `object.blend.spine.reshapeSegment`, `path.convertAnchor`, `path.curvatureEdit`, `path.insertAnchor`, `path.moveAnchors`, `path.reshape`, `path.close` and the Pencil, Curvature, Knife, Eraser, Path Eraser, Smooth, Join and Blob Brush commands refuse a number, point or handle given as text, such as "10", and leave the document unchanged; `path.convertAnchor` refuses an `x` without a `y`, or a `y` without an `x`.
+    `path.setHandle`, `path.moveAnchor`, `text.setPathHandle` (`anchor`), `path.insertAnchor` (`segment`), `artboard.reorder`, `library.moveGroup`, `graphicStyle.move` (`to`), `path.appendAnchor`, `path.close`, `path.reverse` (`subpath`) and the ruler origin commands (`artboard`) refuse a value that is not a whole number of 0 or more, such as `"3"` or `2.5`, with `` `anchor` must be a whole number ``, naming the key, instead of reporting it as missing or reading it as the default subpath or artboard; a null `artboard` is artboard 0 (`tests::params::a_malformed_index_is_not_missing_or_its_default`).
     `path.close` and `path.setHandle` (`independent`), `path.moveAnchors` (`patterns`), the Pencil (`closed`, `keepSelected`, `fill`), Curvature (`closed` and a point's `corner`), `path.curvatureEdit` (`corner`), `path.reshapeSegment` (`perpendicular`) and the Blob Brush (`keepSelected`, `mergeOnlySelection`, `merge`) refuse a value other than true, false or null, such as "true", and leave the document unchanged.
     Direct Selection's marquee selects the frame anchors of area type that lie inside it, and a click on a frame anchor selects that one; Shift toggles them. The selected frame anchors show filled. A drag of one of them moves all of them, the arrow keys nudge them, and the type reflows at its size (`text.reshapeArea`, which takes `items` to move the anchors of several frames in one step), as [Create text](http://web.archive.org/web/20220127090642/https://helpx.adobe.com/illustrator/using/creating-text.html) and [Selecting objects](http://web.archive.org/web/20211230015528/https://helpx.adobe.com/illustrator/using/selecting-objects.html) describe. When the selection also holds other objects, path anchors or ruler guides, or when another change to the document or the selection follows the pick, the type is selected whole.
     A press on a selected object's centre point (Attributes panel › Show Center) selects the object whole, and a drag from it moves the object as one Move, held by that point (#4891; test `the_centre_point_selects_and_moves_the_object_whole`).