From ac651684175e2b4deff6fb9fe1aa0acaa2d6b8f4 Mon Sep 17 00:00:00 2001 From: Giuseppe Cautiero <147257121+giuseeee88@users.noreply.github.com> Date: Wed, 14 Oct 2026 04:30:16 +0200 Subject: [PATCH] Implement centralized transactional fix for command batch failures (#229) * Agent Host changes for agents/fix-command-batch-history-restore * Implement centralized transactional fix for command batch failures - #1 * The batch rollback tests compile, are formatted and expect the tab that was in front The new module used a private helper of plugins_tests and an unused import; the plugin test asserted tab 0 was in front, but it opens a second file first, which is the one in front when the plugin starts and the one the rollback puts back. --------- Co-authored-by: bflatastic --- crates/engine/src/batch_rollback_tests.rs | 63 ++++++++++++++++++++++ crates/engine/src/lib.rs | 65 +++++++++++++++++------ crates/engine/src/plugins_tests.rs | 4 +- crates/engine/src/tests.rs | 30 +++++++++++ docs/mcp.md | 2 +- 5 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 crates/engine/src/batch_rollback_tests.rs diff --git a/crates/engine/src/batch_rollback_tests.rs b/crates/engine/src/batch_rollback_tests.rs new file mode 100644 index 0000000..db0b796 --- /dev/null +++ b/crates/engine/src/batch_rollback_tests.rs @@ -0,0 +1,63 @@ +use serde_json::json; + +use crate::plugins_tests::{OnRefusal, install, manifest, script}; +use crate::{EngineError, Session}; + +fn id(value: &serde_json::Value) -> crate::NodeId { + crate::params::id_of(&value["id"]).unwrap() +} + +#[test] +fn failed_group_closes_documents_opened_inside_it() { + let mut s = Session::new(); + let original = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap()); + let before_journal = s.journal.clone(); + let before_history = s.ds().unwrap().history.undo.len(); + + let result = s.execute( + "command.batch", + &json!({ + "label": "Open then fail", + "commands": [ + {"command": "file.new", "params": {"name": "Temporary"}}, + {"command": "shape.rectangle", "params": {"x": 20, "y": 20, "width": 10, "height": 10}}, + {"command": "node.set", "params": {"id": 999999, "props": {"name": "must fail"}}} + ] + }), + ); + + assert!(result.is_err()); + assert_eq!(s.docs.len(), 1); + assert_eq!(s.active, 0); + assert!(s.doc().unwrap().contains(original)); + assert_eq!(s.ds().unwrap().history.undo.len(), before_history); + assert_eq!(s.journal, before_journal); +} + +#[test] +fn failed_plugin_run_restores_each_document_without_restoring_unrelated_state() { + let mut s = Session::new(); + s.execute("file.new", &json!({"name": "Second"})).unwrap(); + let before_history: Vec<_> = s.docs.iter().map(|ds| ds.history.undo.len()).collect(); + let calls = [ + ("shape.rectangle", json!({"x": 0, "y": 0, "width": 10, "height": 10})), + ("doc.nextTab", json!({})), + ("shape.rectangle", json!({"x": 20, "y": 0, "width": 10, "height": 10})), + ("node.set", json!({"id": 999999, "props": {"name": "must fail"}})), + ]; + install(&mut s, manifest("org.test.rollback", "write", &[]), &script(&calls, OnRefusal::Fail, "")); + s.message = Some("keep this".into()); + let before_journal = s.journal.clone(); + let before_message = s.message.clone(); + // The new file is the one in front when the plugin starts. + let before_active = s.active; + + let result = s.execute("plugin.run", &json!({"id": "org.test.rollback"})); + + assert!(matches!(result, Err(EngineError::Other(_)))); + assert_eq!(s.docs.iter().map(|ds| ds.doc.node(ds.page).map_or(0, |page| page.children.len())).collect::>(), vec![0, 0]); + assert_eq!(s.active, before_active, "the tab in front is put back"); + assert_eq!(s.docs.iter().map(|ds| ds.history.undo.len()).collect::>(), before_history); + assert_eq!(s.journal, before_journal); + assert_eq!(s.message, before_message); +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 7b601d0..c2dad90 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1066,24 +1066,57 @@ impl Session { } /// Run `f` as one undo step called `label`: the commands it executes push no steps of their - /// own, and when it fails the document, selection, page and history are put back. + /// 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 { - let ds = self.ds_mut()?; - let (before, sel, page, saved_len, uid) = (ds.doc.clone(), ds.selection.clone(), ds.page, ds.history.undo.len(), ds.uid); + self.ds()?; + 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(); + let active = self.active; + let journal_len = self.journal.len(); + // The steps inside are dropped again below, so they mustn't push the oldest ones out. - let limit = std::mem::replace(&mut ds.history.limit, usize::MAX); - self.live_hold(uid, true); + for ds in &mut self.docs { + ds.history.limit = usize::MAX; + } + for (uid, _, _, _, _, _) in &before { + self.live_hold(*uid, true); + } let result = f(self); - self.live_hold(uid, false); - let Some(ds) = self.docs.iter_mut().find(|d| d.uid == uid) else { return result }; - ds.history.limit = limit; - ds.history.undo.truncate(saved_len); + for (uid, _, _, _, _, _) in &before { + self.live_hold(*uid, false); + } + if result.is_err() { - ds.doc = before; - ds.selection = sel; - ds.page = page; - } else if !Arc::ptr_eq(&before, &ds.doc) { - self.record(uid, Some(label), before, sel, page); + self.docs.retain(|ds| before.iter().any(|(uid, _, _, _, _, _)| *uid == ds.uid)); + for (uid, doc, selection, page, undo_len, limit) in &before { + if let Some(ds) = self.docs.iter_mut().find(|ds| ds.uid == *uid) { + ds.doc = doc.clone(); + ds.selection = selection.clone(); + ds.page = *page; + ds.history.undo.truncate(*undo_len); + ds.history.limit = *limit; + } + } + self.active = active; + self.journal.truncate(journal_len); + return result; + } + + 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 } @@ -1347,6 +1380,8 @@ mod arc_handles_tests; #[cfg(test)] mod autosave_tests; #[cfg(test)] +mod batch_rollback_tests; +#[cfg(test)] mod binding_lists_tests; #[cfg(test)] mod booleans_tests; @@ -1477,7 +1512,7 @@ mod plugin_directory_tests; #[cfg(test)] mod plugin_panels_tests; #[cfg(test)] -mod plugins_tests; +pub(crate) mod plugins_tests; #[cfg(test)] mod progressive_blur_tests; #[cfg(test)] diff --git a/crates/engine/src/plugins_tests.rs b/crates/engine/src/plugins_tests.rs index ee77127..102d675 100644 --- a/crates/engine/src/plugins_tests.rs +++ b/crates/engine/src/plugins_tests.rs @@ -12,7 +12,7 @@ use figcraft_doc::NodeId; /// What a test plugin does when a call is refused: fail the run (status 1), or carry on and report /// the refusal as its output. #[derive(Clone, Copy, PartialEq)] -enum OnRefusal { +pub(crate) enum OnRefusal { Fail, Report, } @@ -26,7 +26,7 @@ pub(crate) fn lit(s: &str) -> String { /// A module that reads the selection, makes `calls` in order and outputs the selection it read /// (or the first refusal). `tail` runs after the calls (an endless loop, a trap…). -fn script(calls: &[(&str, Value)], on_refusal: OnRefusal, tail: &str) -> Vec { +pub(crate) fn script(calls: &[(&str, Value)], on_refusal: OnRefusal, tail: &str) -> Vec { let mut data = String::new(); let mut body = String::from("(local.set $sel (call $reply_read (i32.const 60000) (call $selection)))\n"); let mut at = 0usize; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 9be5b60..40da2f1 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -76,6 +76,36 @@ fn node_set_and_errors_roll_back() { assert!(s.execute("no.such", &json!({})).is_err()); } +#[test] +fn failed_batch_restores_all_documents_and_history() { + let mut s = sess(); + let original = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap()); + s.execute("file.new", &json!({"name": "Second"})).unwrap(); + s.execute("doc.nextTab", &json!({})).unwrap(); + let before_journal = s.journal.clone(); + let before_history: Vec<_> = s.docs.iter().map(|d| d.history.undo.len()).collect(); + + let result = s.execute( + "command.batch", + &json!({ + "label": "Broken batch", + "commands": [ + {"command": "doc.nextTab"}, + {"command": "shape.rectangle", "params": {"x": 20, "y": 20, "width": 10, "height": 10}}, + {"command": "node.set", "params": {"id": 999999, "props": {"name": "must fail"}}} + ] + }), + ); + + assert!(result.is_err()); + assert_eq!(s.active, 0, "the active document is part of the transaction"); + assert_eq!(s.docs.len(), 2, "a failed batch cannot leave a newly opened tab behind"); + assert_eq!(s.doc().unwrap().node(original).unwrap().name, "Rectangle"); + assert_eq!(s.docs[1].doc.node(s.docs[1].page).map_or(0, |page| page.children.len()), 0); + assert_eq!(s.docs.iter().map(|d| d.history.undo.len()).collect::>(), before_history); + assert_eq!(s.journal, before_journal, "failed inner commands are not journaled"); +} + #[test] fn group_ungroup_and_frame() { let mut s = sess(); diff --git a/docs/mcp.md b/docs/mcp.md index 502667d..c58afee 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -216,7 +216,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, 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 and React, `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. | +| `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. | | `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). |