diff --git a/crates/engine/src/batch_step_ref_tests.rs b/crates/engine/src/batch_step_ref_tests.rs new file mode 100644 index 00000000..ac69118e --- /dev/null +++ b/crates/engine/src/batch_step_ref_tests.rs @@ -0,0 +1,112 @@ +use serde_json::json; + +use crate::{EngineError, Session}; + +fn id(value: &serde_json::Value) -> crate::NodeId { + crate::params::id_of(&value["id"]).unwrap() +} + +#[test] +fn later_steps_target_layers_created_by_earlier_steps() { + let mut s = Session::new(); + let results = s + .execute( + "command.batch", + &json!({ + "label": "Card", + "commands": [ + {"command": "node.create", "params": {"type": "frame", "x": 0, "y": 0, "width": 200, "height": 100}}, + {"command": "node.create", "params": {"type": "rectangle", "parent": {"$step": 0}, "x": 10, "y": 10, "width": 20, "height": 20}}, + {"command": "node.set", "params": {"ids": [{"$step": 1}], "props": {"name": "Swatch"}}} + ] + }), + ) + .unwrap(); + + let frame = id(&results[0]); + let rect = id(&results[1]); + let doc = s.doc().unwrap(); + assert_eq!(doc.node(rect).unwrap().parent, Some(frame)); + assert_eq!(doc.node(rect).unwrap().name, "Swatch"); + assert_eq!(s.ds().unwrap().history.undo.len(), 1, "still one undo step"); +} + +#[test] +fn field_picks_another_part_of_the_result() { + let mut s = Session::new(); + let results = s + .execute( + "command.batch", + &json!({"commands": [ + {"command": "node.create", "params": {"type": "frame", "width": 50, "height": 50}}, + {"command": "node.create", "params": {"type": "text", "text": "hi", "parent": {"$step": 0, "field": "id"}}} + ]}), + ) + .unwrap(); + assert_eq!(s.doc().unwrap().node(id(&results[1])).unwrap().parent, Some(id(&results[0]))); +} + +#[test] +fn bad_references_fail_and_roll_back() { + let mut s = Session::new(); + let before = s.doc().unwrap().node(s.page().unwrap()).unwrap().children.len(); + let frame = json!({"command": "node.create", "params": {"type": "frame", "width": 10, "height": 10}}); + for (bad, needle) in [ + (json!({"$step": 1}), "earlier step"), + (json!({"$step": 5}), "earlier step"), + (json!({"$step": -1}), "step index"), + (json!({"$step": "0"}), "step index"), + (json!({"$step": 0, "field": "nope"}), "no `nope`"), + (json!({"$step": 0, "field": 3}), "`field` must be a string"), + (json!({"$step": 0, "extra": true}), "not `extra`"), + ] { + let r = s.execute("command.batch", &json!({"commands": [frame, {"command": "node.set", "params": {"ids": [bad], "props": {"name": "x"}}}]})); + match r { + Err(EngineError::BadParams { msg, .. }) => assert!(msg.contains(needle), "{msg} should mention {needle}"), + other => panic!("expected BadParams for {bad}, got {other:?}"), + } + assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().children.len(), before, "rolled back"); + } +} + +#[test] +fn deep_params_are_refused_not_overflowed() { + let mut s = Session::new(); + let mut deep = json!(1); + for _ in 0..200 { + deep = json!([deep]); + } + let r = s.execute("command.batch", &json!({"commands": [{"command": "select.none", "params": {"x": deep}}]})); + assert!(matches!(r, Err(EngineError::BadParams { .. }))); +} + +#[test] +fn text_that_looks_like_a_reference_is_left_alone() { + let mut s = Session::new(); + let results = s + .execute("command.batch", &json!({"commands": [{"command": "node.create", "params": {"type": "text", "text": "$step 0 {\"$step\": 0}"}}]})) + .unwrap(); + let n = s.doc().unwrap().node(id(&results[0])).unwrap().clone(); + assert!(matches!(n.kind, figcraft_doc::NodeKind::Text(t) if t.characters == "$step 0 {\"$step\": 0}")); +} + +#[test] +fn a_nested_batch_refers_to_its_own_steps() { + let mut s = Session::new(); + let frame = |name: &str| json!({"command": "node.create", "params": {"type": "frame", "name": name, "width": 100, "height": 100}}); + let results = s + .execute( + "command.batch", + &json!({"commands": [ + frame("Outer"), + {"command": "command.batch", "params": {"commands": [ + frame("Inner"), + {"command": "node.create", "params": {"type": "rectangle", "parent": {"$step": 0}, "width": 10, "height": 10}} + ]}} + ]}), + ) + .unwrap(); + let inner = &results[1]; + let rect = s.doc().unwrap().node(id(&inner[1])).unwrap().clone(); + assert_eq!(rect.parent, Some(id(&inner[0])), "in the nested batch's own first frame, not the outer one"); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index bf375be1..959a6b21 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -107,6 +107,47 @@ pub fn bad(cmd: &str, msg: impl Into) -> EngineError { EngineError::BadParams { cmd: cmd.to_string(), msg: msg.into() } } +/// Deepest params nesting `resolve_step_refs` walks before giving up. +const STEP_REF_MAX_DEPTH: usize = 64; + +/// Replace every `{"$step": n}` / `{"$step": n, "field": "…"}` in `v` with that field (default +/// `id`) of `results[n]`, the result of an earlier batch step. +fn resolve_step_refs(v: &Value, results: &[Value], step: usize, depth: usize) -> Result { + if depth > STEP_REF_MAX_DEPTH { + return Err(bad("command.batch", format!("step {step}: params nest deeper than {STEP_REF_MAX_DEPTH} levels"))); + } + match v { + Value::Object(o) if o.contains_key("$step") => { + if let Some(k) = o.keys().find(|k| *k != "$step" && *k != "field") { + return Err(bad("command.batch", format!("step {step}: a `$step` reference takes only `field`, not `{k}`"))); + } + let n = o + .get("$step") + .and_then(Value::as_u64) + .ok_or_else(|| bad("command.batch", format!("step {step}: `$step` must be a step index (0, 1, …)")))?; + let field = match o.get("field") { + None => "id", + Some(Value::String(f)) => f.as_str(), + Some(_) => return Err(bad("command.batch", format!("step {step}: `field` must be a string"))), + }; + let Some(result) = usize::try_from(n).ok().and_then(|n| results.get(n)) else { + let range = if step == 0 { "there is none before step 0".to_string() } else { format!("0 to {}", step - 1) }; + return Err(bad("command.batch", format!("step {step}: `$step` {n} must be an earlier step ({range})"))); + }; + result.get(field).cloned().ok_or_else(|| bad("command.batch", format!("step {step}: step {n}'s result has no `{field}`"))) + } + Value::Object(o) => { + let mut out = serde_json::Map::with_capacity(o.len()); + for (k, x) in o { + out.insert(k.clone(), resolve_step_refs(x, results, step, depth + 1)?); + } + Ok(Value::Object(out)) + } + Value::Array(a) => a.iter().map(|x| resolve_step_refs(x, results, step, depth + 1)).collect::>>().map(Value::Array), + _ => Ok(v.clone()), + } +} + /// One undo step. #[derive(Clone, Debug)] pub struct HistoryEntry { @@ -1085,9 +1126,20 @@ impl Session { self.ui_requests.push(UiRequest { kind: kind.to_string(), data }); } - /// Run several commands as one undo step. + /// Run several commands as one undo step. A param value `{"$step": n}` is replaced by the `id` + /// in the result of the earlier step `n` (`{"$step": n, "field": "…"}` picks another field), + /// so a step can target a layer that an earlier step of the same batch created. Any object + /// with a `$step` key in a step's params is such a reference, except inside a nested + /// `command.batch` step: its references are to its own steps. pub fn batch(&mut self, label: &str, cmds: &[(String, Value)]) -> Result> { - self.grouped(label, |s| cmds.iter().map(|(id, p)| s.execute(id, p)).collect()) + self.grouped(label, |s| { + let mut results: Vec = Vec::with_capacity(cmds.len()); + for (step, (id, p)) in cmds.iter().enumerate() { + let p = if id == "command.batch" { p.clone() } else { resolve_step_refs(p, &results, step, 0)? }; + results.push(s.execute(id, &p)?); + } + Ok(results) + }) } /// Run `f` as one undo step called `label`: the commands it executes push no steps of their @@ -1410,6 +1462,8 @@ mod autosave_tests; #[cfg(test)] mod batch_rollback_tests; #[cfg(test)] +mod batch_step_ref_tests; +#[cfg(test)] mod binding_lists_tests; #[cfg(test)] mod booleans_tests; diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 9f4d5b8f..9704aab1 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -128,7 +128,7 @@ pub fn tool_definitions() -> Vec { tool( "batch", "Run commands as one step", - "Run several commands in order as ONE undo step (engine `command.batch`). If any command fails, everything is rolled back and the error names the failing step. Returns an array of the results in order. Note: later commands cannot reference ids created by earlier ones in the same batch (ids are only known afterwards), so create containers first, then batch the edits.", + "Run several commands in order as ONE undo step (engine `command.batch`). If any command fails, everything is rolled back and the error names the failing step. Returns an array of the results in order. A later step can use a layer an earlier step created: anywhere in its params, {\"$step\": n} stands for the `id` in step n's result (zero-based; {\"$step\": n, \"field\": \"…\"} picks another field). Example: [{\"command\":\"node.create\",\"params\":{\"type\":\"frame\"}}, {\"command\":\"node.create\",\"params\":{\"type\":\"text\",\"text\":\"Hi\",\"parent\":{\"$step\":0}}}] builds a frame and a text inside it in one call.", obj( json!({ "commands": {"type": "array", "minItems": 1, "description": "Steps to run", "items": obj(json!({"command": string("Command id"), "params": {"type": "object"}}), &["command"])}, diff --git a/crates/mcp/tests/agent_tasks.rs b/crates/mcp/tests/agent_tasks.rs index cc7974ab..b35f3e7f 100644 --- a/crates/mcp/tests/agent_tasks.rs +++ b/crates/mcp/tests/agent_tasks.rs @@ -307,3 +307,40 @@ fn task_vector_edit_keys() { assert_eq!(c.page_children().len(), 1); assert!(c.call("run_command", json!({"command": "vector.edit.state", "params": {}}))["isError"] == true, "Enter left edit mode"); } + +/// A whole card in one `batch` call: later steps reach the frames earlier steps created through +/// `{"$step": n}`, so the tree is built in one round-trip and one undo step. +#[test] +fn task_card_in_one_batch() { + let mut c = Client::new(); + let out = c.ok( + "batch", + json!({"label": "Card", "commands": [ + {"command": "node.create", "params": {"type": "frame", "name": "Card", "width": 320, "height": 50, + "props": {"layout": "vertical", "padding": 24, "gap": 12, "sizingV": "hug"}}}, + {"command": "node.create", "params": {"type": "text", "parent": {"$step": 0}, "text": "Ada Lovelace"}}, + {"command": "node.create", "params": {"type": "frame", "parent": {"$step": 0}, "name": "Button", "height": 44, + "props": {"layout": "horizontal", "sizingH": "fill"}}}, + {"command": "node.create", "params": {"type": "text", "parent": {"$step": 2}, "text": "Follow"}}, + {"command": "node.set", "params": {"ids": [{"$step": 2}], "props": {"fill": "#3B5BDB"}}}, + ]}), + ); + let results = ids(&out); + assert_eq!(results.len(), 5); + let (card, button) = (results[0]["id"].clone(), results[2]["id"].clone()); + let n = c.node(&card); + assert_eq!(ids(&n["children"]).len(), 2, "title and button are inside the card"); + let b = c.node(&button); + assert_eq!(ids(&b["children"]).len(), 1, "the label is inside the button"); + assert_eq!(b["fills"][0]["color"].as_str().unwrap().to_ascii_uppercase(), "#3B5BDB"); + assert_eq!(c.doc()["history"]["undo"][0], "Card", "one undo step"); + // A forward reference fails the whole batch and names the step. + let e = c.err( + "batch", + json!({"commands": [ + {"command": "node.create", "params": {"type": "frame", "parent": {"$step": 1}}}, + {"command": "node.create", "params": {"type": "frame"}}, + ]}), + ); + assert!(e.contains("step 0") && e.contains("earlier step"), "{e}"); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 2351a9df..f751c95b 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -69,6 +69,15 @@ JSON or the gradient shorthand `{"type":"linearGradient","stops":["#hex","#hex"] {"id":3,"method":"engine.execute","params":{"command":"command.batch","params":{"label":"Tidy","commands":[{"command":"align.left","params":{"ids":[2,3]}},{"command":"distribute.vertical","params":{"ids":[2,3]}}]}}} ``` +Inside `command.batch`, `{"$step": n}` in a step's params stands for the `id` in step `n`'s result +(zero-based; `{"$step": n, "field": "…"}` for another field; any object with a `$step` key is read +as one, and a nested `command.batch` step keeps its own references to its own steps), so one batch +can create a frame and fill it: + +```json +{"id":4,"method":"engine.execute","params":{"command":"command.batch","params":{"commands":[{"command":"node.create","params":{"type":"frame","width":200,"height":100}},{"command":"node.create","params":{"type":"rectangle","parent":{"$step":0},"width":20,"height":20}}]}}} +``` + ### `engine.commands {}` Every command: `[{id, label, params, menu, shortcut, undoable}]`. diff --git a/docs/mcp.md b/docs/mcp.md index c16db025..2bfc6106 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -219,7 +219,7 @@ Or in a project's `.mcp.json`: |---|---| | `list_commands {filter?}` | Every command: id, label, menu, shortcut, undoable, params doc. | | `run_command {command, params}` | Run any command (e.g. `align.left`, `layout.add`, `component.create`, `instance.place` (drop an instance at a page point into the frame or auto layout slot under it), `component.exposeInstance` / `instance.exposed` (nested instance properties on instances), `component.makeSlot` / `instance.slotInsert` / `instance.resetSlot` (slots, below), `inspect.code` (CSS, HTML + CSS for a whole frame, React, Leptos, SwiftUI or Jetpack Compose for a layer; components take their properties as parameters and instances are calls, see Dev Mode below), `inspect.properties` (layout, auto layout, typography, fills, strokes and effects with their style and variable names; `colorFormat`: hex, rgb, css, hsl or hsb), `inspect.assets` (layers under the selection with export settings, icons and images, each with its `kind`), `inspect.code` takes `unit: rem` and `rootFontSize` for CSS, HTML, React and Leptos, `style.create`, `variable.bind`, `variable.update` (rename, describe, scope), `variable.set` with `{alias}`, `variable.renameMode` / `variable.removeMode`, `variable.deleteCollection`, `proto.interaction.add`, `codeConnect.set` (components linked to real code, see Dev Mode below)). | -| `batch {commands: [{command, params}], label?}` | Several commands as one undo step (`command.batch`); all-or-nothing for open documents and their document state. Failed inner commands are removed from the journal; preferences, clipboard, tool state and UI requests are not rolled back. | +| `batch {commands: [{command, params}], label?}` | Several commands as one undo step (`command.batch`); all-or-nothing for open documents and their document state. Failed inner commands are removed from the journal; preferences, clipboard, tool state and UI requests are not rolled back. A step can use what an earlier step returned: `{"$step": n}` anywhere in its params becomes the `id` in step n's result (zero-based), and `{"$step": n, "field": "…"}` another field of it (see Step references below). | | `create_node {type, x, y, width, height, parent, index, name, text, path, props}` | Create a frame, rectangle, ellipse, polygon, star, line, arrow, text, vector, section, component or slice; returns `{id, node}`. | | `set_properties {ids, props}` | `node.set` on layers (geometry, fills, strokes, effects, corner radius, auto layout, sizing, constraints, masks (`isMask`, `maskType: alpha\|vector\|luminance`), handoff status (`devStatus: none\|readyForDev\|completed`; marking again clears "Changed"), a section's `sectionContentsHidden`, text…); returns the updated layers. | | `inspect_document {depth, allPages}` | The document as JSON (layer tree, styles, selection, history). | @@ -1517,6 +1517,29 @@ Align and distribute as one undo step: {"command":"distribute.horizontal","params":{"ids":[3,4,5]}}]}}} ``` -The end-to-end tests in `crates/mcp/tests/agent_tasks.rs` are worked examples of ten realistic -tasks (auto layout, components and overrides, gradients and shadows, paint styles, Light/Dark -variables, align/distribute/group, boolean subtract + flatten, export, undo/redo, drawing with tools). +### Step references + +A step in a `batch` can use what an earlier step returned. `{"$step": n}` anywhere in a step's +params (a value, an item in `ids`, a nested field) is replaced by the `id` in the result of step `n` +(zero-based) before the step runs; `{"$step": n, "field": "name"}` takes another field of that +result. This builds a nested tree in one call and one undo step, without guessing ids: + +```json +{"jsonrpc":"2.0","id":9,"method":"tools/call","params":{"name":"batch","arguments":{"label":"Card","commands":[ + {"command":"node.create","params":{"type":"frame","name":"Card","props":{"layout":"vertical","gap":12}}}, + {"command":"node.create","params":{"type":"text","parent":{"$step":0},"text":"Ada Lovelace"}}, + {"command":"node.create","params":{"type":"frame","parent":{"$step":0},"name":"Button"}}, + {"command":"node.create","params":{"type":"text","parent":{"$step":2},"text":"Follow"}}]}}} +``` + +A reference to the same or a later step, a negative or non-integer index, a missing `field` or any +other key next to `$step` fails the whole batch, which is rolled back. Only an object whose keys are +`$step` (and optionally `field`) is a reference: text such as `"$step 0"` is left as written. The +`$step` key is reserved in step params: an object holding it anywhere (in plugin data or a variable +value too) is read as a reference. A step that is itself a `command.batch` is passed on as written, +so the references in its steps are to its own steps, not the outer batch's. + +The end-to-end tests in `crates/mcp/tests/agent_tasks.rs` are worked examples of realistic tasks +(auto layout, a card built in one batch with step references, components and overrides, gradients +and shadows, paint styles, Light/Dark variables, align/distribute/group, boolean subtract + flatten, +export, undo/redo, drawing with tools).