M2: reject malformed explicit layer targets (#785)

crates/engine/src/cmd/layer.rs

@@ -189,15 +189,16 @@ pub fn specs() -> Vec<CommandSpec> {
 
 /// The rows a panel command acts on: `ids` or `id`, else the highlighted rows, else the current
 /// layer (capped at [`MAX_ROWS`]).
-pub(crate) fn rows_param(s: &Session, p: &Value) -> Result<Vec<NodeId>> {
+pub(crate) fn rows_param(s: &Session, p: &Value, command: &str) -> Result<Vec<NodeId>> {
-    let mut ids = match (ids_param(p, "ids"), id_param(p, "id")) {
+    // Presence is explicit intent: validate every target before fallback, truncation or edits.
-        (Some(ids), _) => ids,
+    let mut ids = if p.get("ids").is_some() {
-        (None, Some(id)) => vec![id],
+        checked_ids_param(s, p, "ids", command)?
-        (None, None) => {
+    } else if let Some(id) = p.get("id") {
-            let st = s.doc()?;
+        vec![checked_id(s, id, command)?]
-            let rows = st.highlighted_rows();
+    } else {
-            if rows.is_empty() { st.current_layer().into_iter().collect() } else { rows }
+        let st = s.doc()?;
-        }
+        let rows = st.highlighted_rows();
+        if rows.is_empty() { st.current_layer().into_iter().collect() } else { rows }
     };
     ids.truncate(MAX_ROWS);
     Ok(ids)
@@ -457,7 +458,7 @@ fn new_sublayer(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn delete_rows(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, "layer.delete")?;
     let st = s.doc()?;
     let ids = row_roots(&st.doc, &ids);
     if ids.is_empty() {
@@ -484,7 +485,7 @@ fn delete_rows(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn duplicate_rows(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, "layer.duplicate")?;
     let ids = row_roots(&s.doc()?.doc, &ids);
     if ids.is_empty() {
         return Err(bad("layer.duplicate", "no such rows"));
@@ -551,7 +552,7 @@ fn highlight(s: &mut Session, p: &Value) -> Result<Value> {
 fn set_props(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "layer.setProps";
     check_options(C, p)?;
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, C)?;
     if ids.is_empty() {
         return Err(bad(C, "no rows"));
     }
@@ -567,12 +568,11 @@ fn set_props(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn collect(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "layer.collectInNew";
-    let ids = match ids_param(p, "ids") {
+    let ids = if p.get("ids").is_some() {
-        Some(ids) => ids,
+        checked_ids_param(s, p, "ids", C)?
-        None => {
+    } else {
-            let rows = s.doc()?.highlighted_rows();
+        let rows = s.doc()?.highlighted_rows();
-            if rows.is_empty() { super::edit::selected_roots(s)? } else { rows }
+        if rows.is_empty() { super::edit::selected_roots(s)? } else { rows }
-        }
     };
     let ids = row_roots(&s.doc()?.doc, &ids);
     let Some(top) = ids.last().copied() else { return Err(bad(C, "select rows or objects to collect")) };

crates/engine/src/cmd/layerpanel.rs

@@ -84,7 +84,7 @@ pub fn specs() -> Vec<CommandSpec> {
             None,
             "{ids?} hide every top-level layer but those holding the highlighted rows (else the current layer), which show. One undo step → {count}",
             has_doc,
-            |s, p| others(s, p, Flag::Visible, false)
+            |s, p| others(s, p, Flag::Visible, false, "layer.hideOthers")
         ),
         cmd!("layer.showAll", "Show All Layers", ["Window", "Layers"], None, "{} show every layer and sublayer → {count}", has_doc, |s, _| all(
             s,
@@ -98,7 +98,7 @@ pub fn specs() -> Vec<CommandSpec> {
             None,
             "{ids?} turn Preview off (outline view) for every top-level layer but those holding the highlighted rows (else the current layer), which preview → {count}",
             has_doc,
-            |s, p| others(s, p, Flag::Preview, false)
+            |s, p| others(s, p, Flag::Preview, false, "layer.outlineOthers")
         ),
         cmd!(
             "layer.previewAll",
@@ -116,7 +116,7 @@ pub fn specs() -> Vec<CommandSpec> {
             None,
             "{ids?} lock every top-level layer but those holding the highlighted rows (else the current layer), which unlock → {count}",
             has_doc,
-            |s, p| others(s, p, Flag::Locked, true)
+            |s, p| others(s, p, Flag::Locked, true, "layer.lockOthers")
         ),
         cmd!(
             "layer.unlockAll",
@@ -196,16 +196,16 @@ fn set_flag(s: &mut Session, changes: Vec<(NodeId, bool)>, flag: Flag, label: &s
 }
 
 /// The top-level layers holding the rows a command acts on.
-fn kept_layers(s: &Session, p: &Value) -> Result<HashSet<NodeId>> {
+fn kept_layers(s: &Session, p: &Value, command: &str) -> Result<HashSet<NodeId>> {
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, command)?;
     let d = &s.doc()?.doc;
     Ok(ids.iter().filter_map(|id| d.layer_of(*id)).collect())
 }
 
 /// Every top-level layer but the kept ones set to `v`, and the kept ones to the opposite: Alt-click
 /// on a hidden layer's eye shows just that layer (#1158).
-fn others(s: &mut Session, p: &Value, flag: Flag, v: bool) -> Result<Value> {
+fn others(s: &mut Session, p: &Value, flag: Flag, v: bool, command: &str) -> Result<Value> {
-    let keep = kept_layers(s, p)?;
+    let keep = kept_layers(s, p, command)?;
     let changes = s.doc()?.doc.layers.iter().map(|l| (l.id, !keep.contains(&l.id) == v)).collect();
     set_flag(s, changes, flag, flag.label(v))
 }
@@ -221,7 +221,7 @@ fn all(s: &mut Session, flag: Flag, v: bool) -> Result<Value> {
 }
 
 fn template(s: &mut Session, p: &Value) -> Result<Value> {
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, "layer.template")?;
     let d = &s.doc()?.doc;
     let ids: Vec<NodeId> = ids.into_iter().filter(|id| d.node(*id).is_some_and(Node::is_layer)).collect();
     let Some(first) = ids.first().and_then(|id| d.node(*id)) else { return Err(bad("layer.template", "select a layer")) };
@@ -268,7 +268,7 @@ fn merge_into(d: &mut Document, into: NodeId, others: &[NodeId]) -> Result<()> {
 
 fn merge(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "layer.merge";
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, C)?;
     let st = s.doc()?;
     let d = &st.doc;
     let mut seen = HashSet::new();
@@ -300,7 +300,10 @@ fn flatten(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "layer.flatten";
     let st = s.doc()?;
     let d = &st.doc;
-    let from = id_param(p, "id").or_else(|| st.highlighted_rows().last().copied()).or(st.current_layer());
+    let from = match p.get("id") {
+        Some(id) => Some(checked_id(s, id, C)?),
+        None => st.highlighted_rows().last().copied().or(st.current_layer()),
+    };
     let into = from.and_then(|id| d.layer_of(id)).ok_or_else(|| bad(C, "no layer"))?;
     if !d.node(into).is_some_and(|n| n.visible) {
         return Err(EngineError::Other("the layer to flatten into is hidden".into()));
@@ -389,7 +392,7 @@ fn release(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn reverse(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "layer.reverse";
-    let ids = rows_param(s, p)?;
+    let ids = rows_param(s, p, C)?;
     let d = &s.doc()?.doc;
     let ids = row_roots(d, &ids);
     // Rows grouped by the layer or group holding them (None: top-level layers).

crates/engine/src/tests_layers.rs

@@ -614,3 +614,160 @@ fn the_clipping_mask_button_acts_on_the_highlighted_group() {
     assert_eq!(run(&mut s, "layer.clippingMask.toggle", json!({}))["clip"], true);
     assert!(node(&s, g).clips());
 }
+
+/// #785: explicit malformed layer targets must never become the current-layer default.
+#[test]
+fn layer_delete_rejects_invalid_explicit_targets() {
+    let mut unexpected = vec![];
+    for params in [json!({"id": "1"}), json!({"ids": [1, "2"]}), json!({"id": "$1.id"})] {
+        let mut s = session();
+        run(&mut s, "layer.new", json!({}));
+        run(&mut s, "layer.new", json!({}));
+        let before = layer_target_snapshot(&s);
+        let result = s.execute("layer.delete", &params);
+        if result.is_ok() {
+            let names: Vec<_> = s.doc().unwrap().doc.layers.iter().map(|l| l.display_name()).collect();
+            unexpected.push(format!("{params}: {result:?}; remaining layers {names:?}"));
+        } else {
+            assert_eq!(layer_target_snapshot(&s), before, "{params}");
+        }
+    }
+    assert!(unexpected.is_empty(), "malformed requests succeeded: {}", unexpected.join("; "));
+}
+
+fn layer_target_snapshot(s: &Session) -> Value {
+    let st = s.doc().unwrap();
+    let history =
+        |entries: &[HistoryEntry]| entries.iter().map(|e| json!({"label": e.label, "doc": e.doc, "selection": e.selection})).collect::<Vec<_>>();
+    json!({"doc": st.doc, "selection": st.selection, "revision": st.revision, "activeLayer": st.active_layer,
+        "rows": st.layer_rows, "undo": history(&st.history.undo), "redo": history(&st.history.redo)})
+}
+
+fn layer_target_fixture() -> (Session, Vec<NodeId>) {
+    let mut s = session();
+    let first = s.doc().unwrap().doc.layers[0].id;
+    rect(&mut s, 0.0);
+    let second = id(&run(&mut s, "layer.new", json!({})));
+    rect(&mut s, 40.0);
+    let third = id(&run(&mut s, "layer.new", json!({})));
+    rect(&mut s, 80.0);
+    run(&mut s, "layer.highlight", json!({"ids": [first.0, second.0]}));
+    (s, vec![first, second, third])
+}
+
+fn malformed_layer_targets(first: NodeId, second: NodeId) -> Vec<Value> {
+    let mut params = vec![];
+    for bad in [json!("1"), json!("$1.id"), json!(1.0), json!(1.5), json!(-1), Value::Null, json!(true), json!({}), json!([])] {
+        params.push(json!({"id": bad}));
+        params.push(json!({"ids": [first.0, bad]}));
+    }
+    for bad in [Value::Null, json!("1"), json!(1), json!(false), json!({})] {
+        params.push(json!({"ids": bad, "id": first.0}));
+    }
+    for missing in [0, u64::MAX] {
+        params.push(json!({"id": missing}));
+        params.push(json!({"ids": [first.0, second.0, missing]}));
+    }
+    params
+}
+
+fn assert_layer_target_error(s: &mut Session, command: &str, params: Value) {
+    let before = layer_target_snapshot(s);
+    let error = s.execute(command, &params).unwrap_err();
+    assert!(error.to_string().contains(command), "{command} {params}: {error}");
+    assert_eq!(layer_target_snapshot(s), before, "{command} {params}: rejected targets must leave all state unchanged");
+}
+
+/// #785: shared row commands reject malformed or unknown targets before any partial edits.
+#[test]
+fn layer_target_validation_keeps_document_selection_revision_and_history_unchanged() {
+    let (_, rows) = layer_target_fixture();
+    for command in [
+        "layer.delete",
+        "layer.duplicate",
+        "layer.setProps",
+        "layer.template",
+        "layer.merge",
+        "layer.reverse",
+        "layer.hideOthers",
+        "layer.outlineOthers",
+        "layer.lockOthers",
+    ] {
+        for mut params in malformed_layer_targets(rows[0], rows[1]) {
+            let (mut s, _) = layer_target_fixture();
+            if command == "layer.setProps" {
+                params["name"] = json!("Changed");
+            }
+            assert_layer_target_error(&mut s, command, params);
+        }
+    }
+}
+
+/// These commands have their own selectors, not the shared row-default parser.
+#[test]
+fn layer_collect_and_flatten_validate_their_own_explicit_targets() {
+    let (_, rows) = layer_target_fixture();
+    for params in malformed_layer_targets(rows[0], rows[1]) {
+        let command = if params.get("ids").is_some() { "layer.collectInNew" } else { "layer.flatten" };
+        let (mut s, _) = layer_target_fixture();
+        assert_layer_target_error(&mut s, command, params);
+    }
+}
+
+/// Truncation limits work, not validation: a malformed or unknown tail cannot be ignored.
+#[test]
+fn layer_target_arrays_are_fully_validated_before_the_row_limit() {
+    // The existing panel row limit is 100,000; exercise the public command beyond it.
+    const ROW_LIMIT: usize = 100_000;
+    for tail in [json!("$1.id"), json!(u64::MAX)] {
+        let (mut s, rows) = layer_target_fixture();
+        let mut values = vec![json!(rows[0].0); ROW_LIMIT + 1];
+        *values.last_mut().unwrap() = tail;
+        assert_layer_target_error(&mut s, "layer.delete", json!({"ids": values}));
+    }
+    let (mut s, rows) = layer_target_fixture();
+    let mut values = vec![json!(rows[0].0); ROW_LIMIT + 1];
+    *values.last_mut().unwrap() = json!(rows[1].0);
+    run(&mut s, "layer.setProps", json!({"ids": values, "name": "Within limit"}));
+    assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Within limit"));
+    assert_ne!(node(&s, rows[1]).name.as_deref(), Some("Within limit"), "the valid tail remains beyond the work cap");
+}
+
+#[test]
+fn layer_target_defaults_and_valid_explicit_precedence_are_preserved() {
+    let (mut s, rows) = layer_target_fixture();
+    run(&mut s, "layer.setProps", json!({"name": "Highlighted"}));
+    assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Highlighted"));
+    assert_eq!(node(&s, rows[1]).name.as_deref(), Some("Highlighted"));
+    run(&mut s, "layer.setProps", json!({"id": rows[2].0, "name": "Single"}));
+    assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Single"));
+    let before = undos(&s);
+    run(&mut s, "layer.setProps", json!({"ids": [rows[1].0, rows[0].0, rows[1].0], "id": "$1.id", "name": "Explicit"}));
+    assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Explicit"));
+    assert_eq!(node(&s, rows[1]).name.as_deref(), Some("Explicit"));
+    assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Single"));
+    assert_eq!(undos(&s), before + 1, "duplicates remain one edit");
+    assert_layer_target_error(&mut s, "layer.setProps", json!({"ids": [], "id": rows[2].0, "name": "Must not fall back"}));
+    run(&mut s, "layer.setCurrent", json!({"id": rows[2].0}));
+    run(&mut s, "layer.highlight", json!({"ids": []}));
+    run(&mut s, "layer.setProps", json!({"name": "Current"}));
+    assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Current"));
+    let before = undos(&s);
+    run(&mut s, "layer.delete", json!({}));
+    assert!(s.doc().unwrap().doc.node(rows[2]).is_none());
+    assert_eq!(undos(&s), before + 1);
+}
+
+#[test]
+fn layer_collect_and_flatten_keep_valid_and_omitted_target_behavior() {
+    let (mut s, rows) = layer_target_fixture();
+    let r = run(&mut s, "layer.flatten", json!({"id": rows[2].0}));
+    assert_eq!(r["id"], json!(rows[2].0));
+    assert_eq!(s.doc().unwrap().doc.layers.len(), 1);
+    let (mut s, _) = layer_target_fixture();
+    let selected = s.doc().unwrap().selection.objects.clone();
+    s.doc_mut().unwrap().layer_rows.clear();
+    let r = run(&mut s, "layer.collectInNew", json!({}));
+    let collected = id(&r);
+    assert!(selected.iter().all(|id| parent(&s, *id) == Some(collected)));
+}