Engine: a command keeps its group's name only for a step that is exactly the group's

The record of the last group step held the document, the history length and a group epoch, and a command reused the group's label when the lengths matched and the epoch was newer. A direct edit after the group (no nested command, so the length still matches) then left the step named after the group alone.

The record now holds the workbook the group's step led to (a weak reference, so it keeps nothing alive). A command keeps the group's name only when the one entry it added starts from the command's own starting workbook and ends at its result, which is provenance by identity rather than by position: an entry a nested command made, a step of an earlier command undone since, and a group step the command edited past all name the step after the command. A later group that rolled back changes nothing, and the rollback no longer needs to clear the record.

crates/engine/src/lib.rs

@@ -97,6 +97,17 @@ struct OpenGroup {
     redo: Vec<HistoryEntry>,
 }
 
+/// The undo step the last [`Session::group_end`] made: its document and the workbook it led to.
+/// The entry itself holds the workbook it started from, so a command can check that the step is
+/// exactly its own (the same start and end as the command's) and keep the group's name for it.
+/// A weak reference: the record keeps no workbook alive, and a workbook that is gone can't be
+/// a command's result.
+#[derive(Debug)]
+struct GroupStep {
+    doc: u64,
+    after: std::sync::Weak<Workbook>,
+}
+
 /// The clipboard: a block of cells (with formulas and styles) or marching-ants reference.
 #[derive(Clone, Debug)]
 pub struct Clipboard {
@@ -405,8 +416,9 @@ pub struct Session {
     next_group: u64,
     /// Counts group begins and ends, so `execute` can tell that a command opened or closed one.
     group_epoch: u64,
-    /// The step the last group made: document, its undo length after the step, and the epoch.
-    group_step: Option<(u64, usize, u64)>,
+    /// The step the last `group_end` made, so `execute` can tell that a command's step is exactly
+    /// its group's.
+    group_step: Option<GroupStep>,
     /// Measures cell text for AutoFit Column Width. Without it (CLI, MCP) AutoFit uses
     /// [`cmd::format::approx_text_width`].
     pub text_width: Option<TextWidth>,
@@ -567,12 +579,14 @@ impl Session {
                 let mut label = spec.label.trim_end_matches('…').to_string();
                 if own_groups && let Some((_, len)) = start {
                     // A command whose only step is its group's keeps that name ("Run Macro: Totals").
-                    // The step must have been made by a group this command ended (its epoch is
-                    // past the one the command started with): an older record at the same length
-                    // belongs to an earlier command, undone since.
+                    // That is so only when the one entry the command added is a group's step from
+                    // the command's own start to its own result: an entry a nested command made,
+                    // a group step of an earlier command (undone since), or a group's step that
+                    // the command edited past all name the step after the command instead.
                     if d.undo.len() == len + 1
-                        && self.group_step.is_some_and(|(doc, n, epoch)| doc == uid && n == len + 1 && epoch > group_epoch)
                         && let Some(last) = d.undo.last()
+                        && Arc::ptr_eq(&last.wb, &wb)
+                        && self.group_step.as_ref().is_some_and(|g| g.doc == uid && g.after.upgrade().is_some_and(|after| Arc::ptr_eq(&after, &d.wb)))
                     {
                         label = last.label.clone();
                     }
@@ -646,7 +660,7 @@ impl Session {
         let changed = !Arc::ptr_eq(&d.wb, &g.wb);
         if changed && !outer {
             push_entry(&mut d.undo, g.label, g.wb, &d.wb, g.selection);
-            self.group_step = Some((g.doc, d.undo.len(), self.group_epoch));
+            self.group_step = Some(GroupStep { doc: g.doc, after: Arc::downgrade(&d.wb) });
             d.redo.clear();
             d.revision += 1;
         }
@@ -679,10 +693,6 @@ impl Session {
         d.undo.truncate(g.start);
         d.redo = g.redo;
         d.revision += 1;
-        // A step the rollback dropped is no longer anyone's to name.
-        if self.group_step.is_some_and(|(doc, n, _)| doc == g.doc && n > g.start) {
-            self.group_step = None;
-        }
         Ok(())
     }
 

crates/engine/src/tests/seams.rs

@@ -226,6 +226,27 @@ fn register_group_commands() {
     });
     // A command that is one group keeps the group's name.
     register("test.onlyGroup", |s, _| s.grouped("Named Step", |s| put(s, "D1", "x")).map(|_| json!({})));
+    // A group, then an edit the command makes itself (no nested command, so no undo entry).
+    register("test.groupThenEdit", |s, _| {
+        s.grouped("Named Step", |s| put(s, "D1", "x"))?;
+        crate::cmd::edit(s, |cx| {
+            let at = gridcraft_core::CellRef::parse("D2").ok_or_else(|| EngineError::Other("D2".into()))?;
+            cx.sheet_mut(0)?.set_value(at, gridcraft_core::Value::Text("y".into()));
+            cx.touch(0, at);
+            Ok(())
+        })?;
+        Ok(json!({}))
+    });
+    // A group with a change, then one that is rolled back: the step is still the first group's.
+    register("test.groupThenRolledBack", |s, _| {
+        s.grouped("Named Step", |s| put(s, "D1", "x"))?;
+        let failed: crate::Result<()> = s.grouped("Doomed", |s| {
+            put(s, "D3", "gone")?;
+            Err(EngineError::Other("no".into()))
+        });
+        assert!(failed.is_err());
+        Ok(json!({}))
+    });
     // An edit, then the rollback of the innermost group that was open before the command.
     register("test.rollbackOpenGroup", |s, _| {
         put(s, "E1", "gone")?;
@@ -266,9 +287,9 @@ fn a_command_that_runs_its_own_group_makes_one_step() {
 
 #[test]
 fn a_stale_group_step_does_not_name_a_later_command() {
-    // The record of the last group step (document, length, epoch) survives the Undo of that step.
-    // A later command that ends a group of its own at the same history length, after rolling
-    // another back, must not take the name of the nested edit it made.
+    // The record of the last group step survives the Undo of that step. A later command that
+    // ends a group of its own at the same history length, after rolling another back, must not
+    // take the name of the nested edit it made.
     register_group_commands();
     let mut s = s();
     s.execute("test.onlyGroup", json!({})).unwrap();
@@ -285,6 +306,27 @@ fn a_stale_group_step_does_not_name_a_later_command() {
     assert!(v(&s, "B2").is_empty() && v(&s, "D1").is_empty());
 }
 
+#[test]
+fn a_command_keeps_its_groups_name_only_for_exactly_that_step() {
+    // The name comes from the group only when the group's step is the command's whole step: an
+    // edit after the group (no nested command, so the history length matches) makes it the
+    // command's own, while a later group that rolled back changes nothing.
+    register_group_commands();
+    let mut s = s();
+    s.execute("test.groupThenEdit", json!({})).unwrap();
+    assert_eq!(undo_labels(&s), ["test.groupThenEdit"]);
+    assert_eq!((v(&s, "D1").display(), v(&s, "D2").display()), ("x".into(), "y".into()));
+    s.execute("edit.undo", json!({})).unwrap();
+    assert!(v(&s, "D1").is_empty() && v(&s, "D2").is_empty());
+    assert_eq!(undo_labels(&s), Vec::<String>::new());
+    s.execute("test.groupThenRolledBack", json!({})).unwrap();
+    assert_eq!(undo_labels(&s), ["Named Step"]);
+    assert_eq!(v(&s, "D1").display(), "x");
+    assert!(v(&s, "D3").is_empty());
+    s.execute("edit.undo", json!({})).unwrap();
+    assert!(v(&s, "D1").is_empty());
+}
+
 #[test]
 fn a_group_rolled_back_inside_a_command_keeps_the_redo_history() {
     // The debugger's Stop with Undo Changes rolls its group back inside a command. The rollback