From 3ca12525715a7feee8e265152ac3cb4def8e6995 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:36:09 -0400 Subject: [PATCH 1/3] Engine: a group a panicking command ended keeps its step A group open before a command and ended or rolled back inside it lost its undo step when the command then panicked: the restored history held no entry for the group's changes, so Undo skipped the state before the group. The panic path now pushes the outermost such group's step from its start to the restored workbook. The group stays ended, because its token went with the command. Regression test for a group ended and one rolled back. --- crates/engine/src/lib.rs | 18 +++++++++++++++ crates/engine/src/tests/seams.rs | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index aaa8db4f6..09e39523b 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -536,6 +536,13 @@ impl Session { // made or undid, and the redo history they cleared, come back. A length is not enough, // because a nested `edit.undo` removes entries. let history_before = before.as_ref().and_then(|(uid, ..)| self.docs.iter().find(|d| d.uid == *uid).map(|d| (d.undo.clone(), d.redo.clone()))); + // The groups open on the document before the command (usually none), for the same reason. + let groups_before: Vec<(u64, String, Arc, Selection)> = match &before { + Some((uid, ..)) => { + self.groups.iter().filter(|g| g.doc == *uid).map(|g| (g.id, g.label.clone(), g.wb.clone(), g.selection.clone())).collect() + } + None => Vec::new(), + }; // The recorder keeps top-level calls with the place they ran in. let place = (self.depth == 0 && self.recorder.is_some()).then(|| match self.active() { Some(d) => (d.uid, d.wb.active_sheet, d.wb.active().map(|sh| sh.name.clone()).unwrap_or_default(), d.selection.clone()), @@ -558,6 +565,7 @@ impl Session { // Restore the workbook and its history as they were, and drop the undo groups the // command opened. self.groups.retain(|g| g.id < first_group); + let group_open = before.as_ref().is_some_and(|(uid, ..)| self.groups.iter().any(|g| g.doc == *uid)); if let Some((uid, wb, sel)) = &before && let Some(d) = self.docs.iter_mut().find(|d| d.uid == *uid) { @@ -567,6 +575,16 @@ impl Session { d.undo = undo; d.redo = redo; } + // A group open before the command and ended or rolled back inside it stays + // ended (its token went with the command), and keeps a step from its start to + // the restored workbook, so Undo still returns to the state before the group. + if !group_open + && let Some((_, label, wb, selection)) = groups_before.into_iter().min_by_key(|g| g.0) + && !Arc::ptr_eq(&wb, &d.wb) + { + push_entry(&mut d.undo, label, wb, &d.wb, selection); + d.redo.clear(); + } d.calc.rebuild(&d.wb); } if self.depth == 0 { diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index 9c9aeb6ad..48105da25 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -579,6 +579,45 @@ fn a_command_that_panics_leaves_the_history_as_it_found_it() { assert_eq!(v(&s, "A1").display(), "1"); } +#[test] +fn a_group_ended_by_a_command_that_then_panics_keeps_its_step() { + // The command takes a token the test left for it, ends or rolls back that group, and panics. + // The group's token went with the command, so the group stays ended; its step runs from the + // group's start to the restored workbook, so Undo still returns to the state before the group. + static TOKEN: std::sync::Mutex> = std::sync::Mutex::new(None); + crate::cmd::register_undoable_test_command("test.endGroupThenPanic", |s, _| { + let token = TOKEN.lock().map_err(|_| EngineError::Other("lock".into()))?.take(); + if let Some(token) = token { + s.group_end(token)?; + } + panic!("after ending a group"); + }); + crate::cmd::register_undoable_test_command("test.rollBackGroupThenPanic", |s, _| { + let token = TOKEN.lock().map_err(|_| EngineError::Other("lock".into()))?.take(); + if let Some(token) = token { + s.group_rollback(token)?; + } + panic!("after rolling back a group"); + }); + for cmd in ["test.endGroupThenPanic", "test.rollBackGroupThenPanic"] { + let mut s = s(); + set(&mut s, "A1", "before"); + let token = s.group_begin("Outer").unwrap(); + set(&mut s, "A1", "inside"); + set(&mut s, "A2", "inside too"); + *TOKEN.lock().unwrap() = Some(token); + assert!(s.execute(cmd, json!({})).is_err()); + let uid = s.doc().unwrap().uid; + assert!(!s.in_group(uid), "{cmd}: the group stays ended"); + assert_eq!(undo_labels(&s), ["Enter Cell", "Outer"], "{cmd}"); + assert_eq!(v(&s, "A1").display(), "inside", "{cmd}: the workbook is as the command found it"); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!((v(&s, "A1").display(), v(&s, "A2").display()), ("before".into(), String::new()), "{cmd}"); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(v(&s, "A2").display(), "inside too", "{cmd}"); + } +} + // ---------------------------------------------------------------- recording buffer fn recorded_ids(s: &Session) -> Vec { From 7ee9d77483c124e3ee4bcbf6135b7ca73762169f Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:36:09 -0400 Subject: [PATCH 2/3] Docs: list every paste that carries a file's trust The architecture note named formulas and validation only; it now lists the Paste Special kinds and linked pastes that write formulas, and the kinds that carry nothing. --- docs/architecture.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index aa5e1e64d..f51f3a61b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -66,8 +66,11 @@ Actions made of many commands (macros, a debugger that runs one step per call) u Commands run by other commands are not recorded, and the buffer is independent of the journal (Automate › Record Actions still records through the journal). - **Document trust.** `DocState::trust` is `Local` for workbooks made in the session and - `FromFile` for workbooks opened from a file, and for workbooks that received formulas or - validation pasted from one. Active content in a `FromFile` workbook asks before it runs. This + `FromFile` for workbooks opened from a file, and for workbooks that received a paste from one + that writes formulas or validation: Paste, Paste Special › All, All Except Borders, Formulas, + Formulas and Number Formats or Validation, and any linked paste other than column widths or + notes. Values, formats, column widths and notes carry nothing active. Active content in a + `FromFile` workbook asks before it runs. This is provenance, which no decision changes; `DocState::scripts_trusted` is the separate per-session consent for stored `automate.*` scripts (`automate.trustScripts`). - **Workbook data in hidden names** (`cmd/stash.rs`). An item is split over hidden From 712bb766ecdf8063d28f74f94303bd5efa63ce6e Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Fri, 23 Oct 2026 21:46:25 -0400 Subject: [PATCH 3/3] Engine: a grouped action that panics is rolled back before the panic goes on Session::grouped ran its action without a panic guard, so a caller outside a command that caught the panic found the group still open: Undo and Redo disabled, the partial edits kept, and the next grouped action nested in the abandoned group. The group is now rolled back and the panic resumed. Regression test. --- crates/engine/src/lib.rs | 12 +++++++++--- crates/engine/src/tests/seams.rs | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 09e39523b..8a8d8bcfa 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -680,20 +680,26 @@ impl Session { pub fn grouped(&mut self, label: &str, f: impl FnOnce(&mut Session) -> Result) -> Result { let token = self.group_begin(label)?; let doc = token.doc(); - match f(self) { - Ok(r) => match self.group_end(token) { + // A panic in `f` rolls the group back before it goes on, so a caller that catches it + // (outside `execute`, which drops a command's groups itself) finds no group left open. + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(self))) { + Ok(Ok(r)) => match self.group_end(token) { Ok(_) => Ok(r), // `f` closed the group's document (`file.close`, or `file.open` taking the place of // an untouched new workbook), and the group went with it: nothing is left to undo. Err(_) if !self.docs.iter().any(|d| d.uid == doc) => Ok(r), Err(e) => Err(e), }, - Err(e) => { + Ok(Err(e)) => { // The error from `f` is the one to report. A rollback fails only when the group // has already ended, and then there is nothing left to undo. let _ = self.group_rollback(token); Err(e) } + Err(panic) => { + let _ = self.group_rollback(token); + std::panic::resume_unwind(panic) + } } } diff --git a/crates/engine/src/tests/seams.rs b/crates/engine/src/tests/seams.rs index 48105da25..0f1bc3072 100644 --- a/crates/engine/src/tests/seams.rs +++ b/crates/engine/src/tests/seams.rs @@ -579,6 +579,31 @@ fn a_command_that_panics_leaves_the_history_as_it_found_it() { assert_eq!(v(&s, "A1").display(), "1"); } +#[test] +fn a_grouped_action_that_panics_is_rolled_back_before_the_panic_goes_on() { + // Outside a command, a caller that catches the panic finds the group rolled back and no + // group left open, so Undo is enabled and the next grouped action does not nest. + let mut s = s(); + set(&mut s, "A1", "before"); + let caught = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + s.grouped::<()>("Doomed", |s| { + set(s, "A1", "half done"); + panic!("inside the group"); + }) + })); + assert!(caught.is_err(), "the panic goes on to the caller"); + let uid = s.doc().unwrap().uid; + assert!(!s.in_group(uid)); + assert_eq!(v(&s, "A1").display(), "before"); + assert_eq!(undo_labels(&s), ["Enter Cell"]); + s.grouped("Next", |s| { + set(s, "A1", "after"); + Ok(()) + }) + .unwrap(); + assert_eq!(undo_labels(&s), ["Enter Cell", "Next"]); +} + #[test] fn a_group_ended_by_a_command_that_then_panics_keeps_its_step() { // The command takes a token the test left for it, ends or rolls back that group, and panics.