diff --git a/crates/engine/src/automation/batch.rs b/crates/engine/src/automation/batch.rs index aa11a2bb1..103177455 100644 --- a/crates/engine/src/automation/batch.rs +++ b/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, results: Vec, + failed: HashSet, } 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 { 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(), diff --git a/crates/engine/src/batch_continue_events_tests.rs b/crates/engine/src/batch_continue_events_tests.rs new file mode 100644 index 000000000..280f9a696 --- /dev/null +++ b/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 = r.as_array().unwrap().iter().map(|o| o["ok"].as_bool().unwrap()).collect(); + assert_eq!(ok, [true, false, true]); + let texts: Vec = 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::>(), ["comment.added", "comment.added"]); +} diff --git a/crates/engine/src/batch_step_ref_tests.rs b/crates/engine/src/batch_step_ref_tests.rs index 47d6e8766..17d9fc725 100644 --- a/crates/engine/src/batch_step_ref_tests.rs +++ b/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"); +} diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 9679e19be..1b9466e2b 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -460,7 +460,14 @@ pub fn specs() -> Vec { } 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 = match get(p, "commands") { Some(Value::Array(a)) => { @@ -476,7 +483,7 @@ pub fn specs() -> Vec { } _ => 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 { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index a14e86fe8..b8335abb0 100644 --- a/crates/engine/src/lib.rs +++ b/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; diff --git a/crates/engine/src/session/undo.rs b/crates/engine/src/session/undo.rs index 6e911c20e..285fc8a69 100644 --- a/crates/engine/src/session/undo.rs +++ b/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, + selection: Vec, + page: NodeId, + undo_len: usize, + /// An undo inside pops undo steps; a failed group has to bring them back. + undo: Vec, + redo: Vec, + limit: usize, +} + +struct GroupCheckpoint { + docs: Vec, + 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> { + 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 { + 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(&mut self, label: &str, f: impl FnOnce(&mut Self) -> Result) -> Result { 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(&mut self, label: Option<&str>, f: impl FnOnce(&mut Self) -> Result) -> Result { + 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; + } + } } diff --git a/crates/engine/src/sharing/background/tests.rs b/crates/engine/src/sharing/background/tests.rs index c3e5c48e6..d71a44bef 100644 --- a/crates/engine/src/sharing/background/tests.rs +++ b/crates/engine/src/sharing/background/tests.rs @@ -129,6 +129,35 @@ fn published() -> Vec { 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); diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 12e3593f7..9166141d8 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -129,11 +129,12 @@ 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. 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) -> Result) -> Result { diff --git a/crates/mcp/tests/it/protocol.rs b/crates/mcp/tests/it/protocol.rs index 4dc2672e8..f4f076376 100644 --- a/crates/mcp/tests/it/protocol.rs +++ b/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(); diff --git a/docs/cli.md b/docs/cli.md index 80bab2a4e..58436b3ef 100644 --- a/docs/cli.md +++ b/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: diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 02388acea..6c6f20cf1 100644 --- a/docs/control-protocol.md +++ b/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 diff --git a/docs/mcp.md b/docs/mcp.md index 1d3495ccb..be9c392bd 100644 --- a/docs/mcp.md +++ b/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