Merge pull request #1409 from avhn/feat/1386-batch-continue

M0.9: let batches continue after failed steps (#1386)

crates/engine/src/automation/batch.rs

@@ -5,7 +5,7 @@
 //! takes a `field`, a key of that result or a slash path into it (`ids/0`, `map/3`, `bounds/x`), the
 //! way `variable.bind` names `fills/0/color`. The CLI's script runner resolves its steps the same way.
 
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 
 use serde_json::{Map, Value};
 
@@ -57,6 +57,7 @@ pub fn refuse_unknown_step_keys(step: usize, json: &Value) -> Result<()> {
 pub struct BatchScope {
     names: HashMap<String, usize>,
     results: Vec<Value>,
+    failed: HashSet<usize>,
 }
 
 impl BatchScope {
@@ -69,10 +70,10 @@ impl BatchScope {
                 return Err(bad("command.batch", format!("step {i}: `as` \"{name}\" is already step {first}'s name")));
             }
         }
-        Ok(Self { names, results: Vec::with_capacity(steps.len()) })
+        Ok(Self { names, results: Vec::with_capacity(steps.len()), failed: HashSet::new() })
     }
 
-    /// The results of the steps run so far, in order.
+    /// The results of the steps run so far, in order (failed slots hold null).
     pub fn results(&self) -> &[Value] {
         &self.results
     }
@@ -90,6 +91,12 @@ impl BatchScope {
         self.results.push(result);
     }
 
+    pub(crate) fn push_failed(&mut self) {
+        // Keep positions even after a failure; a successful null result is not a failed step.
+        self.failed.insert(self.next_step());
+        self.results.push(Value::Null);
+    }
+
     /// `params` of the next step with every reference in it replaced.
     pub(crate) fn resolve(&self, params: &Value) -> Result<Value> {
         self.resolve_in(params, 0)
@@ -137,6 +144,9 @@ impl BatchScope {
             }
             (n, format!("step {n} (\"{name}\")"))
         };
+        if self.failed.contains(&n) {
+            return Err(fail(format!("references {who}, which failed")));
+        }
         let path = match o.get("field") {
             None => "id",
             Some(Value::String(f)) => f.as_str(),

crates/engine/src/batch_continue_events_tests.rs

@@ -0,0 +1,33 @@
+//! A batch that goes on after a failed step (`onError: "continue"`) reports the live events of the
+//! steps that stayed, not of the ones rolled back.
+
+use std::time::Duration;
+
+use serde_json::json;
+
+use crate::Session;
+
+#[test]
+fn a_continued_batch_reports_only_the_events_of_steps_that_stayed() {
+    let mut s = Session::new();
+    let before = s.events.latest();
+    let r = s
+        .execute(
+            "command.batch",
+            &json!({"onError": "continue", "commands": [
+                {"command": "comment.add", "params": {"x": 10, "y": 10, "text": "Stays"}},
+                {"command": "command.batch", "params": {"commands": [
+                    {"command": "comment.add", "params": {"x": 20, "y": 20, "text": "Rolled back"}},
+                    {"command": "node.set", "params": {"ids": [999_999], "props": {"name": "x"}}}
+                ]}},
+                {"command": "comment.add", "params": {"x": 30, "y": 30, "text": "Stays too"}}
+            ]}),
+        )
+        .unwrap();
+    let ok: Vec<bool> = r.as_array().unwrap().iter().map(|o| o["ok"].as_bool().unwrap()).collect();
+    assert_eq!(ok, [true, false, true]);
+    let texts: Vec<String> = s.doc().unwrap().comments.iter().map(|c| c.first.text.clone()).collect();
+    assert_eq!(texts, ["Stays", "Stays too"]);
+    let events = s.events.poll(before, &["comments".into()], 10, Duration::ZERO).events;
+    assert_eq!(events.iter().map(|e| e.kind.as_str()).collect::<Vec<_>>(), ["comment.added", "comment.added"]);
+}

crates/engine/src/batch_step_ref_tests.rs

@@ -280,3 +280,375 @@ fn a_failing_command_names_its_step_and_rolls_back() {
     assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, name);
     assert_eq!(s.ds().unwrap().history.undo.len(), undo);
 }
+
+#[test]
+fn a_continue_batch_can_undo_then_redo_between_steps() {
+    for mode in ["abort", "continue"] {
+        let mut s = Session::new();
+        let rect = id(&s.execute("node.create", &json!({"type": "rectangle"})).unwrap());
+        let before = s.doc().unwrap().clone();
+        let results =
+            s.execute("command.batch", &json!({"onError": mode, "commands": [{"command": "edit.undo"}, {"command": "edit.redo"}]})).unwrap();
+        let undo = if mode == "continue" { &results[0]["result"] } else { &results[0] };
+        let redo = if mode == "continue" { &results[1]["result"] } else { &results[1] };
+        assert_eq!(undo["undone"], true, "{mode}: {results}");
+        assert_eq!(redo["redone"], true, "{mode}: {results}");
+        assert!(s.doc().unwrap().contains(rect));
+        assert_eq!(s.doc().unwrap(), &before);
+    }
+}
+
+#[test]
+fn a_continue_batch_can_open_a_document_after_closing_the_last_tab() {
+    let mut s = Session::new();
+    let uid = s.ds().unwrap().uid;
+    let results = s
+        .execute(
+            "command.batch",
+            &json!({"onError": "continue", "commands": [
+                {"command": "file.close", "params": {"force": true}},
+                {"command": "file.new"},
+                {"command": "page.rename", "params": {"name": "New page"}}
+            ]}),
+        )
+        .unwrap();
+    assert!(results.as_array().unwrap().iter().all(|r| r["ok"] == true), "{results}");
+    assert_eq!(s.docs.len(), 1);
+    assert_ne!(s.ds().unwrap().uid, uid);
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "New page");
+    assert!(s.closed_in_group.is_empty());
+}
+
+#[test]
+fn a_continue_batch_repeats_the_last_successful_command_after_a_failed_step() {
+    let mut s = Session::new();
+    s.execute("node.create", &json!({"type": "rectangle"})).unwrap();
+    let last = s.last_command.clone();
+    let results = s
+        .execute(
+            "command.batch",
+            &json!({"onError": "continue", "commands": [
+                {"command": "command.batch", "params": {"commands": [
+                    {"command": "node.create", "params": {"type": "text", "text": "Rolled back"}},
+                    {"command": "no.such.command"}
+                ]}},
+                {"command": "edit.repeat"}
+            ]}),
+        )
+        .unwrap();
+    assert_eq!(results[0]["ok"], false);
+    assert_eq!(results[1]["ok"], true);
+    let repeated = s.doc().unwrap().node(id(&results[1]["result"])).unwrap();
+    assert!(matches!(repeated.kind, figcraft_doc::NodeKind::Rectangle), "{repeated:?}");
+    assert_eq!(s.last_command, last);
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().children.len(), 2);
+}
+
+#[test]
+fn a_continue_batch_keeps_successful_steps_as_one_undo_entry() {
+    let mut s = Session::new();
+    let frame = id(&s.execute("node.create", &json!({"type": "frame", "width": 50, "height": 50})).unwrap());
+    let text = id(&s.execute("node.create", &json!({"type": "text", "text": "orig"})).unwrap());
+    assert_eq!((frame.0, text.0), (2, 3));
+    let undo = s.undo_depth().0;
+    let name = s.doc().unwrap().node(s.page().unwrap()).unwrap().name.clone();
+    let results = s
+        .execute(
+            "command.batch",
+            &json!({"label": "Continue", "onError": "continue", "commands": [
+                {"command": "text.set", "params": {"ids": [3], "text": "novo"}},
+                {"command": "text.set", "params": {"ids": [2], "text": "erro"}},
+                {"command": "page.rename", "params": {"name": "Depois do erro"}}
+            ]}),
+        )
+        .unwrap();
+    assert_eq!(results.as_array().unwrap().len(), 3);
+    assert_eq!(results[0], json!({"step": 0, "command": "text.set", "ok": true, "result": {"ok": true}}));
+    assert_eq!(results[2], json!({"step": 2, "command": "page.rename", "ok": true, "result": {"ok": true}}));
+    assert_eq!(results[1]["step"], 1);
+    assert_eq!(results[1]["command"], "text.set");
+    assert_eq!(results[1]["ok"], false);
+    let error = results[1]["error"].as_str().unwrap();
+    assert!(error.contains("step 1:") && error.contains("is not text"), "{error}");
+    assert!(results[1].get("result").is_none());
+    undo_and_redo_the_continued_batch(&mut s, frame, text, &name, undo);
+}
+
+/// The successful steps of the batch above undo and redo as one step.
+fn undo_and_redo_the_continued_batch(s: &mut Session, frame: figcraft_doc::NodeId, text: figcraft_doc::NodeId, name: &str, undo: usize) {
+    assert!(matches!(&s.doc().unwrap().node(text).unwrap().kind, figcraft_doc::NodeKind::Text(t) if t.characters == "novo"));
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "Depois do erro");
+    assert_eq!(s.undo_depth(), (undo + 1, 0));
+    assert_eq!(s.undo_labels(1), ["Continue"]);
+    assert!(s.undo().unwrap());
+    assert!(matches!(&s.doc().unwrap().node(text).unwrap().kind, figcraft_doc::NodeKind::Text(t) if t.characters == "orig"));
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, name);
+    assert!(s.doc().unwrap().contains(frame), "undo leaves earlier edits alone");
+    assert_eq!(s.undo_depth(), (undo, 1));
+    assert!(s.redo().unwrap());
+    assert!(matches!(&s.doc().unwrap().node(text).unwrap().kind, figcraft_doc::NodeKind::Text(t) if t.characters == "novo"));
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "Depois do erro");
+    assert_eq!(s.undo_depth(), (undo + 1, 0));
+}
+
+#[test]
+fn a_continue_batch_rolls_back_partial_steps_and_keeps_reference_positions() {
+    let mut s = Session::new();
+    let text = id(&s.execute("node.create", &json!({"type": "text", "text": "orig"})).unwrap());
+    let frame = id(&s.execute("node.create", &json!({"type": "frame"})).unwrap());
+    let results = s
+        .execute(
+            "command.batch",
+            &json!({"onError": "continue", "commands": [
+                {"command": "text.set", "as": "bad", "params": {"ids": [text.0, frame.0], "text": "partial"}},
+                {"command": "node.create", "as": "card", "params": {"type": "frame"}},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$step": 0}}},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$ref": "bad"}}},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$step": 1}}},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$ref": "card"}}},
+                {"command": "node.set", "params": {"ids": [{"$step": 4}], "props": {"name": "Child"}}}
+            ]}),
+        )
+        .unwrap();
+    assert_eq!(results.as_array().unwrap().len(), 7);
+    for (i, result) in results.as_array().unwrap().iter().enumerate() {
+        assert_eq!(result["step"], i);
+        assert_eq!(result["ok"], ![0, 2, 3].contains(&i));
+    }
+    assert!(results[2]["error"].as_str().unwrap().contains("step 2: references step 0, which failed"));
+    assert!(results[3]["error"].as_str().unwrap().contains("step 3: references step 0 (\"bad\"), which failed"));
+    assert!(
+        matches!(&s.doc().unwrap().node(text).unwrap().kind, figcraft_doc::NodeKind::Text(t) if t.characters == "orig"),
+        "the failed step's partial edit was rolled back"
+    );
+    let card = id(&results[1]["result"]);
+    for i in [4, 5] {
+        assert_eq!(s.doc().unwrap().node(id(&results[i]["result"])).unwrap().parent, Some(card), "references use raw results, not outcomes");
+    }
+    assert_eq!(s.doc().unwrap().node(id(&results[4]["result"])).unwrap().name, "Child");
+}
+
+#[test]
+fn invalid_batch_policies_run_nothing() {
+    for mode in [json!("skip"), json!(""), json!(true), json!(3), json!(["continue"]), json!({})] {
+        let mut s = Session::new();
+        let before = s.doc().unwrap().clone();
+        let journal = s.journal.clone();
+        let error = s
+            .execute("command.batch", &json!({"onError": mode, "commands": [{"command": "page.rename", "params": {"name": "Changed"}}]}))
+            .unwrap_err();
+        assert!(matches!(error, EngineError::BadParams { .. }));
+        assert!(error.to_string().contains("`onError` must be \"abort\" or \"continue\""), "{error}");
+        assert_eq!(s.doc().unwrap(), &before);
+        assert_eq!(s.journal, journal);
+        assert_eq!(s.undo_depth(), (0, 0));
+    }
+}
+
+#[test]
+fn invalid_batch_structure_runs_nothing_in_either_mode() {
+    let rename = json!({"command": "page.rename", "params": {"name": "Changed"}});
+    for mode in ["abort", "continue"] {
+        for commands in [
+            json!(null),
+            json!({}),
+            json!([rename, null]),
+            json!([rename, {}]),
+            json!([rename, {"command": 3}]),
+            json!([rename, {"command": "page.rename", "as": ""}]),
+            json!([rename, {"command": "page.rename", "as": 3}]),
+            json!([{"command": "page.rename", "as": "same", "params": {"name": "Changed"}}, {"command": "select.none", "as": "same"}]),
+        ] {
+            let mut s = Session::new();
+            let before = s.doc().unwrap().clone();
+            let journal = s.journal.clone();
+            let result = s.execute("command.batch", &json!({"onError": mode, "commands": commands}));
+            assert!(matches!(result, Err(EngineError::BadParams { .. })), "{mode}: {result:?}");
+            assert_eq!(s.doc().unwrap(), &before);
+            assert_eq!(s.journal, journal);
+            assert_eq!(s.undo_depth(), (0, 0));
+        }
+    }
+}
+
+#[test]
+fn an_explicit_abort_batch_keeps_the_default_results_and_errors() {
+    let commands = json!([
+        {"command": "node.create", "params": {"type": "frame"}},
+        {"command": "node.create", "params": {"type": "text", "text": "orig"}}
+    ]);
+    let mut default = Session::new();
+    let mut abort = Session::new();
+    let raw = default.execute("command.batch", &json!({"commands": commands})).unwrap();
+    assert_eq!(abort.execute("command.batch", &json!({"onError": "abort", "commands": commands})).unwrap(), raw);
+    assert!(raw[0].get("id").is_some() && raw[0].get("ok").is_none(), "still raw results");
+    let commands = json!([
+        {"command": "text.set", "params": {"ids": [3], "text": "novo"}},
+        {"command": "text.set", "params": {"ids": [2], "text": "erro"}},
+        {"command": "page.rename", "params": {"name": "Depois do erro"}}
+    ]);
+    let before = abort.doc().unwrap().clone();
+    let undo = abort.undo_depth();
+    let error = default.execute("command.batch", &json!({"commands": commands})).unwrap_err();
+    let explicit = abort.execute("command.batch", &json!({"onError": "abort", "commands": commands})).unwrap_err();
+    assert_eq!(explicit.to_string(), error.to_string());
+    assert!(matches!(explicit, EngineError::BadParams { .. }));
+    assert_eq!(abort.doc().unwrap(), &before);
+    assert_eq!(abort.undo_depth(), undo);
+}
+
+#[test]
+fn nested_batch_error_policies_are_local() {
+    for outer in ["abort", "continue"] {
+        let mut s = Session::new();
+        let results = s
+            .execute(
+                "command.batch",
+                &json!({"onError": outer, "commands": [
+                    {"command": "command.batch", "as": "inner", "params": {"onError": "continue", "commands": [
+                        {"command": "node.create", "as": "card", "params": {"type": "frame"}},
+                        {"command": "no.such.command"},
+                        {"command": "node.create", "params": {"type": "text", "parent": {"$ref": "card"}}}
+                    ]}},
+                    {"command": "node.set", "params": {"ids": [{"$ref": "inner", "field": "2/result/id"}], "props": {"name": "Inner child"}}}
+                ]}),
+            )
+            .unwrap();
+        let inner = if outer == "continue" { &results[0]["result"] } else { &results[0] };
+        assert_eq!(inner[1]["ok"], false);
+        let child = s.doc().unwrap().node(id(&inner[2]["result"])).unwrap();
+        assert_eq!(child.name, "Inner child");
+        assert_eq!(child.parent, Some(id(&inner[0]["result"])));
+        assert_eq!(s.undo_depth(), (1, 0));
+        assert!(s.undo().unwrap());
+        assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().children.len(), 0);
+        assert!(s.redo().unwrap());
+        assert!(s.doc().unwrap().contains(id(&inner[2]["result"])));
+    }
+}
+
+#[test]
+fn an_outer_continue_batch_rolls_back_a_failed_inner_abort_batch() {
+    for explicit in [false, true] {
+        let mut s = Session::new();
+        let mut inner = json!({"commands": [
+            {"command": "file.close", "params": {"force": true}},
+            {"command": "file.new", "params": {"name": "Temporary"}},
+            {"command": "node.create", "params": {"type": "frame"}},
+            {"command": "no.such.command"}
+        ]});
+        if explicit {
+            inner["onError"] = json!("abort");
+        }
+        let uid = s.ds().unwrap().uid;
+        let results = s
+            .execute(
+                "command.batch",
+                &json!({"onError": "continue", "commands": [
+                    {"command": "page.rename", "params": {"name": "Before"}},
+                    {"command": "command.batch", "params": inner},
+                    {"command": "page.rename", "params": {"name": "After"}}
+                ]}),
+            )
+            .unwrap();
+        assert_eq!(results[1]["ok"], false);
+        assert!(results[1]["error"].as_str().unwrap().contains("unknown command"));
+        assert_eq!(results[2]["ok"], true);
+        assert_eq!(s.docs.len(), 1);
+        assert_eq!(s.ds().unwrap().uid, uid);
+        assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "After");
+        assert!(s.doc().unwrap().node(s.page().unwrap()).unwrap().children.is_empty());
+        assert!(s.closed_in_group.is_empty());
+        assert_eq!(s.undo_depth(), (1, 0));
+    }
+}
+
+#[test]
+fn an_outer_abort_batch_rolls_back_successes_of_an_inner_continue_batch() {
+    let mut s = Session::new();
+    let before = s.doc().unwrap().clone();
+    let result = s.execute(
+        "command.batch",
+        &json!({"commands": [
+            {"command": "command.batch", "params": {"onError": "continue", "commands": [
+                {"command": "node.create", "params": {"type": "frame"}},
+                {"command": "no.such.command"}
+            ]}},
+            {"command": "no.such.command"}
+        ]}),
+    );
+    assert!(matches!(result, Err(EngineError::UnknownCommand(_))));
+    assert_eq!(s.doc().unwrap(), &before);
+    assert_eq!(s.undo_depth(), (0, 0));
+}
+
+#[test]
+fn an_all_failed_continue_batch_adds_no_undo_and_keeps_redo() {
+    let mut s = Session::new();
+    let frame = id(&s.execute("node.create", &json!({"type": "frame"})).unwrap());
+    s.execute("page.rename", &json!({"name": "Redo name"})).unwrap();
+    assert!(s.undo().unwrap());
+    let depth = s.undo_depth();
+    let before = s.doc().unwrap().clone();
+    let results = s
+        .execute(
+            "command.batch",
+            &json!({"onError": "continue", "commands": [
+                {"command": "node.set", "params": {"ids": [frame.0, 999999], "props": {"name": "Partial"}}},
+                {"command": "no.such.command"},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$step": 0}}}
+            ]}),
+        )
+        .unwrap();
+    assert!(results.as_array().unwrap().iter().all(|r| r["ok"] == false));
+    assert_eq!(s.doc().unwrap(), &before);
+    assert_eq!(s.undo_depth(), depth);
+    assert!(s.redo().unwrap());
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "Redo name");
+}
+
+#[test]
+fn an_empty_continue_batch_returns_an_empty_array_without_undo() {
+    let mut s = Session::new();
+    assert_eq!(s.execute("command.batch", &json!({"onError": "continue", "commands": []})).unwrap(), json!([]));
+    assert_eq!(s.undo_depth(), (0, 0));
+}
+
+#[test]
+fn a_null_batch_result_is_not_a_failed_slot() {
+    let mut scope = crate::BatchScope::default();
+    scope.push(serde_json::Value::Null);
+    scope.push_failed();
+    let null = scope.resolve(&json!({"$step": 0})).unwrap_err().to_string();
+    assert!(null.contains("step 0's result has no `id`") && !null.contains("which failed"), "{null}");
+    let failed = scope.resolve(&json!({"$step": 1})).unwrap_err().to_string();
+    assert!(failed.contains("step 2: references step 1, which failed"), "{failed}");
+    assert_eq!(scope.next_step(), 2);
+}
+
+#[test]
+fn a_continue_batch_reports_runtime_errors_and_runs_later_steps() {
+    let mut s = Session::new();
+    let results = s
+        .execute(
+            "command.batch",
+            &json!({"onError": "continue", "commands": [
+                {"command": "no.such.command"},
+                {"command": "node.create", "params": {"type": "no-such-type"}},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$ref": "missing"}}},
+                {"command": "node.create", "params": {"type": "text", "parent": {"$step": 99}}},
+                {"command": "node.create", "params": {"type": "frame"}},
+                {"command": "node.set", "params": {"ids": [{"$step": 4, "field": "missing"}], "props": {"name": "No"}}},
+                {"command": "page.rename", "params": {"name": "Done"}}
+            ]}),
+        )
+        .unwrap();
+    assert_eq!(results.as_array().unwrap().len(), 7);
+    for (i, needle) in [(0, "unknown command"), (1, "unknown type"), (2, "names no step"), (3, "earlier step"), (5, "has no `missing`")] {
+        assert_eq!(results[i]["ok"], false);
+        assert!(results[i]["error"].as_str().unwrap().contains(needle), "{}", results[i]);
+    }
+    assert_eq!(results[4]["ok"], true);
+    assert_eq!(results[6]["ok"], true);
+    assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "Done");
+}

crates/engine/src/cmd/edit.rs

@@ -460,7 +460,14 @@ pub fn specs() -> Vec<CommandSpec> {
             }
             ok()
         }),
-        spec!("command.batch", "Run several commands", "{label?, commands: [{command, params, as?}]} (one undo step)", false, |s, p| {
+        spec!("command.batch", "Run several commands", "{label?, onError?, commands: [{command, params, as?}]} (one undo step)", false, |s, p| {
+            // `continue` rolls back only the failed steps and returns one outcome per step.
+            let continue_on_error = match get(p, "onError") {
+                None => false,
+                Some(Value::String(mode)) if mode == "abort" => false,
+                Some(Value::String(mode)) if mode == "continue" => true,
+                Some(_) => return Err(bad("command.batch", "`onError` must be \"abort\" or \"continue\"")),
+            };
             let label = string(p, "label").unwrap_or_else(|| "Batch".into());
             let cmds: Vec<BatchStep> = match get(p, "commands") {
                 Some(Value::Array(a)) => {
@@ -476,7 +483,7 @@ pub fn specs() -> Vec<CommandSpec> {
                 }
                 _ => return Err(bad("command.batch", "`commands` must be an array")),
             };
-            Ok(Value::Array(s.batch(&label, &cmds)?))
+            Ok(Value::Array(if continue_on_error { s.batch_continue(&label, &cmds)? } else { s.batch(&label, &cmds)? }))
         }),
         spec!("edit.repeat", "Repeat last command", "{}", true, |s, _| {
             let Some((id, p)) = s.last_command.clone().filter(|(id, _)| id != "edit.repeat") else {

crates/engine/src/lib.rs

@@ -179,6 +179,8 @@ mod arc_handles_tests;
 #[cfg(test)]
 mod autosave_tests;
 #[cfg(test)]
+mod batch_continue_events_tests;
+#[cfg(test)]
 mod batch_rollback_tests;
 #[cfg(test)]
 mod batch_step_ref_tests;

crates/engine/src/session/undo.rs

@@ -1,8 +1,29 @@
 use super::{
     Arc, BatchScope, BatchStep, DocState, Document, EngineError, HistoryEntry, Interaction, NodeId, PREVIEW_READ_ONLY, Result, Session, UiRequest,
-    Value, cmd, figcraft_collab, normalize, textpreview,
+    Value, cmd, figcraft_collab, json, normalize, textpreview,
 };
 
+struct GroupDocCheckpoint {
+    uid: u64,
+    doc: Arc<Document>,
+    selection: Vec<NodeId>,
+    page: NodeId,
+    undo_len: usize,
+    /// An undo inside pops undo steps; a failed group has to bring them back.
+    undo: Vec<HistoryEntry>,
+    redo: Vec<HistoryEntry>,
+    limit: usize,
+}
+
+struct GroupCheckpoint {
+    docs: Vec<GroupDocCheckpoint>,
+    active: usize,
+    journal_len: usize,
+    /// Live events buffered before the group: a failed one drops only its own.
+    pending_events: usize,
+    last_command: Option<(String, Value)>,
+}
+
 impl Session {
     /// How many undo and redo steps the active document has (the user's own in a live session).
     pub fn undo_depth(&self) -> (usize, usize) {
@@ -179,18 +200,45 @@ impl Session {
         })
     }
 
+    /// Run every step, rolling back failed steps and returning each outcome, still as one undo step.
+    pub(crate) fn batch_continue(&mut self, label: &str, steps: &[BatchStep]) -> Result<Vec<Value>> {
+        let mut scope = BatchScope::new(steps)?;
+        self.grouped(label, |s| {
+            let mut outcomes = Vec::with_capacity(steps.len());
+            for (i, step) in steps.iter().enumerate() {
+                let outcome = match s.checkpointed(None, |s| s.execute_batch_step(&scope, &step.command, &step.params)) {
+                    Ok(v) => {
+                        scope.push(v.clone());
+                        json!({"step": i, "command": step.command, "ok": true, "result": v})
+                    }
+                    Err(EngineError::Waiting) => return Err(EngineError::Waiting),
+                    Err(e) => {
+                        scope.push_failed();
+                        json!({"step": i, "command": step.command, "ok": false, "error": e.to_string()})
+                    }
+                };
+                outcomes.push(outcome);
+            }
+            Ok(outcomes)
+        })
+    }
+
     /// The next step of a batch: the references in `params` resolve against `scope` (the steps
     /// before it), the command runs and its result joins `scope`. [`Session::batch`] runs a list
     /// of these as one undo step; a caller with its own list — the CLI's script runner — uses this
     /// so its steps resolve references the same way, inside its own [`Session::grouped`].
     pub fn batch_step(&mut self, scope: &mut BatchScope, cmd: &str, params: &Value) -> Result<()> {
-        let p = if cmd == "command.batch" { params.clone() } else { scope.resolve(params)? };
-        // Say which step failed; a nested batch names its own steps.
-        let v = self.execute(cmd, &p).map_err(|e| if cmd == "command.batch" { e } else { e.at_step(scope.next_step()) })?;
+        let v = self.execute_batch_step(scope, cmd, params)?;
         scope.push(v);
         Ok(())
     }
 
+    fn execute_batch_step(&mut self, scope: &BatchScope, cmd: &str, params: &Value) -> Result<Value> {
+        let p = if cmd == "command.batch" { params.clone() } else { scope.resolve(params)? };
+        // Say which step failed; a nested batch names its own steps.
+        self.execute(cmd, &p).map_err(|e| if cmd == "command.batch" { e } else { e.at_step(scope.next_step()) })
+    }
+
     /// Take the tabs `pred` matches out of `docs`; inside a group they are kept so that a failed
     /// group can reopen them.
     pub(crate) fn remove_tabs(&mut self, mut pred: impl FnMut(&DocState) -> bool) {
@@ -205,80 +253,106 @@ impl Session {
     /// own, and when it fails the documents stay at their group-start state.
     pub fn grouped<T>(&mut self, label: &str, f: impl FnOnce(&mut Self) -> Result<T>) -> Result<T> {
         self.ds()?;
-        let pending_mark = self.pending_events.len();
-        let before: Vec<_> =
-            self.docs.iter().map(|ds| (ds.uid, ds.doc.clone(), ds.selection.clone(), ds.page, ds.history.undo.len(), ds.history.limit)).collect();
-        // An undo inside pops the undo steps; a failed group has to bring them back.
-        let undo_before: Vec<_> = self.docs.iter().map(|ds| ds.history.undo.clone()).collect();
-        // An edit inside clears the redo steps; a failed group has to bring them back.
-        let redo_before: Vec<_> = self.docs.iter().map(|ds| ds.history.redo.clone()).collect();
-        let active = self.active;
-        let journal_len = self.journal.len();
+        self.checkpointed(Some(label), f)
+    }
 
-        // The steps inside are dropped again below, so they mustn't push the oldest ones out.
-        for ds in &mut self.docs {
-            ds.history.limit = usize::MAX;
-        }
-        for (uid, _, _, _, _, _) in &before {
-            self.live_hold(*uid, true);
-        }
-        self.group_depth += 1;
+    /// Without a label, only roll back failures: leave successful steps' history to the outer group.
+    /// A checkpoint also works between closing the last tab and opening another one.
+    fn checkpointed<T>(&mut self, label: Option<&str>, f: impl FnOnce(&mut Self) -> Result<T>) -> Result<T> {
+        let before = self.group_checkpoint();
         let result = f(self);
         self.group_depth -= 1;
         let at_root = self.group_depth == 0;
-        for (uid, _, _, _, _, _) in &before {
-            self.live_hold(*uid, false);
+        for ds in &before.docs {
+            self.live_hold(ds.uid, false);
         }
 
         if result.is_err() {
-            self.pending_events.truncate(pending_mark);
-            // Tabs the group closed come back from the stash, and the group-start order returns.
-            let mut current = std::mem::take(&mut self.docs);
-            for (uid, ..) in &before {
-                let tab = match current.iter().position(|d| d.uid == *uid) {
-                    Some(i) => Some(current.swap_remove(i)),
-                    None => self.closed_in_group.iter().position(|d| d.uid == *uid).map(|i| self.closed_in_group.swap_remove(i)),
-                };
-                self.docs.extend(tab);
-            }
-            for (((uid, doc, selection, page, _, limit), undo), redo) in before.iter().zip(undo_before).zip(redo_before) {
-                if let Some(ds) = self.docs.iter_mut().find(|ds| ds.uid == *uid) {
-                    ds.history.redo = redo;
-                    ds.doc = doc.clone();
-                    ds.selection = selection.clone();
-                    ds.page = *page;
-                    ds.history.undo = undo;
-                    ds.history.limit = *limit;
+            self.restore_group_checkpoint(before, label.is_none());
+        } else {
+            let mut changed = Vec::new();
+            for saved in &before.docs {
+                let Some(ds) = self.docs.iter_mut().find(|ds| ds.uid == saved.uid) else { continue };
+                ds.history.limit = saved.limit;
+                if label.is_some() {
+                    if !Arc::ptr_eq(&saved.doc, &ds.doc) && ds.interaction.is_none() {
+                        changed.push((saved.uid, saved.doc.clone(), saved.selection.clone(), saved.page));
+                    }
+                    ds.history.undo.truncate(saved.undo_len);
                 }
             }
-            self.active = active;
-            self.journal.truncate(journal_len);
-            if self.group_depth == 0 {
-                self.closed_in_group.clear();
+            if let Some(label) = label {
+                for (uid, doc, selection, page) in changed {
+                    self.record(uid, Some(label), doc, selection, page);
+                }
             }
-            return result;
         }
         if at_root {
             self.closed_in_group.clear();
             self.flush_pending_events();
         }
-
-        let changed: Vec<_> = before
-            .iter()
-            .filter_map(|(uid, doc, selection, page, _, _)| {
-                let ds = self.docs.iter().find(|d| d.uid == *uid)?;
-                (!Arc::ptr_eq(doc, &ds.doc) && ds.interaction.is_none()).then(|| (*uid, doc.clone(), selection.clone(), *page))
-            })
-            .collect();
-        for ds in &mut self.docs {
-            if let Some((_, _, _, _, undo_len, limit)) = before.iter().find(|before| before.0 == ds.uid) {
-                ds.history.limit = *limit;
-                ds.history.undo.truncate(*undo_len);
-            }
-        }
-        for (uid, doc, selection, page) in changed {
-            self.record(uid, Some(label), doc, selection, page);
-        }
         result
     }
+
+    fn group_checkpoint(&mut self) -> GroupCheckpoint {
+        let docs = self
+            .docs
+            .iter()
+            .map(|ds| GroupDocCheckpoint {
+                uid: ds.uid,
+                doc: ds.doc.clone(),
+                selection: ds.selection.clone(),
+                page: ds.page,
+                undo_len: ds.history.undo.len(),
+                undo: ds.history.undo.clone(),
+                // An edit inside clears redo; a failed group has to bring it back.
+                redo: ds.history.redo.clone(),
+                limit: ds.history.limit,
+            })
+            .collect();
+        let before = GroupCheckpoint {
+            docs,
+            active: self.active,
+            journal_len: self.journal.len(),
+            pending_events: self.pending_events.len(),
+            last_command: self.last_command.clone(),
+        };
+        // The steps inside are dropped again below, so they mustn't push the oldest ones out.
+        for ds in &mut self.docs {
+            ds.history.limit = usize::MAX;
+        }
+        for ds in &before.docs {
+            self.live_hold(ds.uid, true);
+        }
+        self.group_depth += 1;
+        before
+    }
+
+    fn restore_group_checkpoint(&mut self, before: GroupCheckpoint, restore_last_command: bool) {
+        // Tabs the group closed come back from the stash, and the group-start order returns.
+        let mut current = std::mem::take(&mut self.docs);
+        for saved in &before.docs {
+            let tab = match current.iter().position(|d| d.uid == saved.uid) {
+                Some(i) => Some(current.swap_remove(i)),
+                None => self.closed_in_group.iter().position(|d| d.uid == saved.uid).map(|i| self.closed_in_group.swap_remove(i)),
+            };
+            self.docs.extend(tab);
+        }
+        for saved in before.docs {
+            if let Some(ds) = self.docs.iter_mut().find(|ds| ds.uid == saved.uid) {
+                ds.history.redo = saved.redo;
+                ds.doc = saved.doc;
+                ds.selection = saved.selection;
+                ds.page = saved.page;
+                ds.history.undo = saved.undo;
+                ds.history.limit = saved.limit;
+            }
+        }
+        self.active = before.active;
+        self.journal.truncate(before.journal_len);
+        self.pending_events.truncate(before.pending_events);
+        if restore_last_command {
+            self.last_command = before.last_command;
+        }
+    }
 }

crates/engine/src/sharing/background/tests.rs

@@ -129,6 +129,35 @@ fn published() -> Vec<u8> {
     archive(s.doc().unwrap().published.as_ref().unwrap()).unwrap()
 }
 
+#[test]
+fn a_continue_batch_waits_instead_of_skipping_background_work() {
+    for nested in [false, true] {
+        let (mut s, wire) = session(true);
+        let before = s.doc().unwrap().clone();
+        let request = json!({"command": "plugin.directory.list", "params": {"server": SERVER}});
+        let step = if nested { json!({"command": "command.batch", "params": {"onError": "continue", "commands": [request]}}) } else { request };
+        let params = json!({"onError": "continue", "commands": [
+            {"command": "page.rename", "params": {"name": "Before request"}},
+            step,
+            {"command": "page.rename", "params": {"name": "After request"}}
+        ]});
+        assert_eq!(s.start_background("command.batch", params).unwrap(), None);
+        assert!(s.waiting_for("command.batch"));
+        assert_eq!(s.doc().unwrap(), &before, "waiting rolls back the whole batch");
+        assert_eq!(s.undo_depth(), (0, 0));
+        assert_eq!(answer(&mut s, &wire), 1);
+        let (command, result) = s.take_background_done().unwrap();
+        assert_eq!(command, "command.batch");
+        let result = result.unwrap();
+        assert!(result.as_array().unwrap().iter().all(|r| r["ok"] == true));
+        let directory = if nested { &result[1]["result"][0]["result"] } else { &result[1]["result"] };
+        assert_eq!(directory["plugins"][0]["id"], ID);
+        assert_eq!(s.doc().unwrap().node(s.page().unwrap()).unwrap().name, "After request");
+        assert_eq!(s.undo_depth(), (1, 0));
+        assert!(!s.waiting_for("command.batch"));
+    }
+}
+
 #[test]
 fn browsing_and_installing_wait_for_nothing_and_scripts_still_wait() {
     let (mut s, wire) = session(true);

crates/mcp/src/tools.rs

@@ -129,11 +129,12 @@ pub fn tool_definitions() -> Vec<Value> {
         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. A later step can use what an earlier step returned: name a step with \"as\": \"card\", then anywhere in a later step's params {\"$ref\": \"card\"} stands for the `id` in its result ({\"$step\": n} does the same by zero-based position). Add \"field\" for another part of the result: a key or a slash path, e.g. {\"$ref\": \"copy\", \"field\": \"ids/0\"} for the first id edit.duplicate returned. Any object with a `$ref` or `$step` key is a reference. Example: [{\"command\":\"node.create\",\"as\":\"card\",\"params\":{\"type\":\"frame\"}}, {\"command\":\"node.create\",\"params\":{\"type\":\"text\",\"text\":\"Hi\",\"parent\":{\"$ref\":\"card\"}}}] builds a frame and a text inside it in one call.",
+            "Run several commands in order as ONE undo step (engine `command.batch`). By default (onError: \"abort\"), if any command fails, everything is rolled back and the error names the failing step; success returns an array of the results in order. With onError: \"continue\", a failed step is rolled back on its own, the later steps still run, the call is not an error, and the array instead holds one {step, command, ok, result} or {step, command, ok, error} object per step; a reference to a failed step fails that step too. A later step can use what an earlier step returned: name a step with \"as\": \"card\", then anywhere in a later step's params {\"$ref\": \"card\"} stands for the `id` in its result ({\"$step\": n} does the same by zero-based position). Add \"field\" for another part of the result: a key or a slash path, e.g. {\"$ref\": \"copy\", \"field\": \"ids/0\"} for the first id edit.duplicate returned. Any object with a `$ref` or `$step` key is a reference. Example: [{\"command\":\"node.create\",\"as\":\"card\",\"params\":{\"type\":\"frame\"}}, {\"command\":\"node.create\",\"params\":{\"type\":\"text\",\"text\":\"Hi\",\"parent\":{\"$ref\":\"card\"}}}] 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"}, "as": string("Name for this step, unique in the batch; later steps use its result with {\"$ref\": name}")}), &["command"])},
                     "label": string("Undo history label (default \"Batch\")"),
+                    "onError": {"type": "string", "enum": ["abort", "continue"], "description": "abort (default) rolls back the whole batch on the first failure; continue rolls back only the failed step, runs the rest and returns one {step, command, ok, result|error} object per step"},
                 }),
                 &["commands"],
             ),
@@ -580,7 +581,11 @@ fn batch(b: &mut dyn Backend, a: &Map<String, Value>) -> Result<ToolResult, Stri
         steps.push(step);
     }
     let label = s_arg(a, "label")?.unwrap_or("Batch");
-    exec(b, "command.batch", json!({"label": label, "commands": steps})).map(|v| ToolResult::json(&v))
+    let mut params = json!({"label": label, "commands": steps});
+    if let Some(mode) = a.get("onError") {
+        params["onError"] = mode.clone();
+    }
+    exec(b, "command.batch", params).map(|v| ToolResult::json(&v))
 }
 
 fn create_node(b: &mut dyn Backend, a: &Map<String, Value>) -> Result<ToolResult, String> {

crates/mcp/tests/it/protocol.rs

@@ -58,6 +58,56 @@ fn tools_list_is_complete_and_schemas_are_objects() {
     }
 }
 
+#[test]
+fn batch_schema_exposes_the_optional_error_policy() {
+    let mut c = Client::new();
+    let r = c.rpc("tools/list", json!({}));
+    let batch = r["result"]["tools"].as_array().unwrap().iter().find(|t| t["name"] == "batch").unwrap();
+    let schema = &batch["inputSchema"];
+    let mode = &schema["properties"]["onError"];
+    assert_eq!(mode["type"], "string");
+    assert_eq!(mode["enum"], json!(["abort", "continue"]));
+    assert!(mode["description"].as_str().unwrap().contains("default"));
+    assert!(!schema["required"].as_array().unwrap().contains(&json!("onError")));
+}
+
+#[test]
+fn a_continue_batch_returns_outcomes_without_a_tool_error() {
+    let mut c = Client::new();
+    let frame = c.create(json!({"type": "frame"}));
+    let text = c.create(json!({"type": "text", "text": "orig"}));
+    assert_eq!((frame.clone(), text.clone()), (json!(2), json!(3)));
+    let commands = json!([
+        {"command": "text.set", "params": {"ids": [3], "text": "novo"}},
+        {"command": "text.set", "as": "bad", "params": {"ids": [2], "text": "erro"}},
+        {"command": "page.rename", "params": {"name": "Depois do erro"}},
+        {"command": "node.create", "params": {"type": "text", "parent": {"$ref": "bad"}}}
+    ]);
+    let r = c.call("batch", json!({"onError": "continue", "commands": commands}));
+    assert_eq!(r["isError"], false);
+    let outcomes: Value = serde_json::from_str(r["content"][0]["text"].as_str().unwrap()).unwrap();
+    assert_eq!(outcomes.as_array().unwrap().len(), 4);
+    assert_eq!(outcomes[0], json!({"step": 0, "command": "text.set", "ok": true, "result": {"ok": true}}));
+    assert_eq!(outcomes[2], json!({"step": 2, "command": "page.rename", "ok": true, "result": {"ok": true}}));
+    assert_eq!(outcomes[1]["ok"], false);
+    assert_eq!(outcomes[1]["command"], "text.set");
+    assert_eq!(outcomes[1]["step"], 1);
+    assert!(outcomes[1]["error"].as_str().unwrap().contains("is not text"));
+    assert!(outcomes[3]["error"].as_str().unwrap().contains("step 3: references step 1 (\"bad\"), which failed"));
+    assert_eq!(c.node(&text)["characters"], "novo");
+    assert_eq!(c.doc()["pages"][0]["name"], "Depois do erro");
+    assert_eq!(c.page_children().len(), 2, "failed steps created nothing");
+    c.ok("undo", json!({}));
+    assert_eq!(c.node(&text)["characters"], "orig");
+    c.ok("redo", json!({}));
+    assert_eq!(c.node(&text)["characters"], "novo");
+    assert!(c.err("batch", json!({"onError": "skip", "commands": commands})).contains("`onError`"));
+    // A null policy is the default: the failed step aborts the whole batch.
+    assert!(c.err("batch", json!({"onError": null, "commands": commands})).contains("is not text"));
+    let run = c.run("command.batch", json!({"onError": "continue", "commands": [{"command": "no.such.command"}]}));
+    assert_eq!(run[0]["ok"], false, "run_command forwards the policy too");
+}
+
 #[test]
 fn tools_call_happy_and_error_paths() {
     let mut c = Client::new();

docs/cli.md

@@ -153,6 +153,10 @@ created ([Step references](mcp.md#step-references) has the rules):
 The result lines are printed once the script is through, so none of them reports work a later step
 took back.
 
+To keep going past a failed command, make a step a `command.batch` with `"onError": "continue"`
+([MCP server](mcp.md) has the rules). The script itself stays all-or-nothing: if a
+later top-level step fails, that batch's edits are rolled back with everything else.
+
 A failing step prints one line with `"ok": false`, the error and `"rolledBack": true`, writes
 nothing (`--out` is not saved, and the session's documents go back to how the script found them),
 and exits with code 1:

docs/control-protocol.md

@@ -109,6 +109,10 @@ naming it and the ones it takes, never ignored; `ids` and `id` are always taken.
 {"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]}}]}}}
 ```
 
+With `"onError": "continue"` in its params, `command.batch` rolls back only the steps that fail,
+runs the rest and returns one `{step, command, ok, result}` or `{step, command, ok, error}` object
+per step; the request succeeds even when some steps fail (see [MCP server](mcp.md)).
+
 Inside `command.batch`, a step can name itself with `"as"` and later steps use its result:
 `{"$ref": "name"}` in a step's params stands for the `id` in that step's result, and `{"$step": n}`
 for the `id` in step `n`'s result (zero-based). Either takes `"field"` for another key or a slash

docs/mcp.md

@@ -1983,6 +1983,26 @@ Align and distribute as one undo step:
   {"command":"distribute.horizontal","params":{"ids":[3,4,5]}}]}}}
 ```
 
+By default (`"onError": "abort"`) the first failure rolls back the whole batch, and success
+returns the raw results in order. With `"onError": "continue"` next to `commands`, a failed step's
+document edits are rolled back on their own, the later steps still run, and the result is instead
+one object per step:
+
+```json
+[
+  {"step": 0, "command": "text.set", "ok": true, "result": {"ok": true}},
+  {"step": 1, "command": "text.set", "ok": false, "error": "invalid parameters for `text.set`: step 1: node 2 is not text"},
+  {"step": 2, "command": "page.rename", "ok": true, "result": {"ok": true}}
+]
+```
+
+The tool call is not an error (`isError` is false) even when some steps fail. Successful document
+edits form one undo step; if every step fails, none is added. Unknown commands, bad params and bad
+references fail only their step, but a malformed `commands` list, an invalid `onError` or two steps
+with the same `as` still reject the whole batch before any step runs. A step that has to wait for
+background work makes the whole call wait, as without `onError`. A nested batch has its own
+`onError`.
+
 ### Step references
 
 A step in a `batch` can use what an earlier step returned. Give the step a name with `"as"` (next to
@@ -2020,7 +2040,10 @@ These fail the whole batch, which is rolled back, with an error naming the step:
 same `as` (before any step runs); a `$ref` to a name no step has (the error lists the names there
 are) or to the same or a later step; a `$step` to the same or a later step, or that is negative or
 not an integer; a `field` the result doesn't have (the error names the full path and, for a list,
-how many items it has); any key next to `$ref` or `$step` other than `field`. Only an object holding
+how many items it has); any key next to `$ref` or `$step` other than `field`. With `"onError":
+"continue"` all but the duplicate names fail only their step, and so does a reference to a step that
+failed (the error names both). Steps keep their positions, and references read the raw results. Only
+an object holding
 a `$ref` or `$step` key is a reference: text such as `"$ref card"` is left as written. Both keys are
 reserved in step params: an object holding either 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