Engine: a recorded script run keeps the steps that ran

The recorder expanded an automate.runScript step from the workbook as it
stood when the recording stopped, so a script edited or deleted during the
recording changed the saved macro, or dropped the step. The run now leaves
the steps it ran with the recorder (Recorder::ran, by step index, within
the byte budget), and the conversion reads those, falling back to the
workbook only when they could not be kept.

crates/engine/src/cmd/extra.rs

@@ -1074,6 +1074,9 @@ fn run_script(s: &mut Session, p: &Json) -> Result<Json> {
     }
     let _sandbox = crate::io::ScriptSandbox::enter();
     let mut n = 0;
+    // What ran, for a recorder in progress: the recording keeps these steps with the run, so
+    // editing or deleting the script before the recording stops does not change what it saved.
+    let mut ran_steps: Vec<(String, Json)> = Vec::new();
     for st in &steps {
         let id = st["command"].as_str().unwrap_or("");
         if id.starts_with("automate.") {
@@ -1092,6 +1095,12 @@ fn run_script(s: &mut Session, p: &Json) -> Result<Json> {
         }
         ran?;
         n += 1;
+        if s.recorder.is_some() {
+            ran_steps.push((id.to_string(), st["params"].clone()));
+        }
+    }
+    if s.recorder.is_some() {
+        s.script_ran = Some(ran_steps);
     }
     Ok(json!({"ran": n}))
 }

crates/engine/src/cmd/macros.rs

@@ -234,7 +234,7 @@ fn save_recording(s: &mut Session, p: &Json, rec: &crate::record::Recorder, star
         Some(n) => crate::macros::unique_name(&wb, n),
         None => crate::macros::next_name(&wb),
     };
-    let mut converted = crate::macros::recorder::convert(&name, &start, &rec.steps, &wb);
+    let mut converted = crate::macros::recorder::convert(&name, &start, &rec.steps, &rec.ran, &wb);
     // Steps past the macro limit, and ones recorded in another workbook, are left out and counted
     // with the ones the recorder dropped.
     let cut = converted.cut.saturating_add(converted.mac.steps.len().saturating_sub(gridcraft_model::MAX_MACRO_STEPS));

crates/engine/src/lib.rs

@@ -426,6 +426,9 @@ pub struct Session {
     pub recorder: Option<record::Recorder>,
     /// How many `execute` calls are running (0 between top-level commands).
     depth: u32,
+    /// The steps the last successful `automate.runScript` ran while a recorder was on, for the
+    /// recorder to keep with that step (see [`record::Recorder::ran`]); taken by the recording hook.
+    pub(crate) script_ran: Option<Vec<(String, Json)>>,
     /// Macros the person allowed to run in this session: (document uid, the macro's content as
     /// canonical JSON, from [`macros::fingerprint`]).
     pub macro_approvals: std::collections::HashSet<(u64, String)>,
@@ -691,7 +694,15 @@ impl Session {
                 {
                     p.insert("on".into(), on.clone());
                 }
-                rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params });
+                let kept = rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params });
+                // A script run is kept with the steps it ran, so editing or deleting the script
+                // before the recording stops does not change what was recorded.
+                if let Some(ran) = self.script_ran.take()
+                    && kept
+                    && id == "automate.runScript"
+                {
+                    rec.attach_ran(ran);
+                }
             }
             self.push_journal(id, cmd::journal_params(&params));
         }

crates/engine/src/macros/recorder.rs

@@ -17,6 +17,7 @@
 use gridcraft_core::{CellRef, RangeRef, Value};
 use gridcraft_model::{ColType, InputKind, MAX_MACRO_STEPS, Macro, NamedInput, Step, StepKind, Workbook};
 use serde_json::{Map, Value as Json, json};
+use std::collections::BTreeMap;
 
 use super::policy::{Policy, macro_policy};
 use crate::Selection;
@@ -47,7 +48,9 @@ pub struct Converted {
 }
 
 /// Builds a macro called `name` from the recorded steps.
-pub fn convert(name: &str, start: &RecordingStart, steps: &[RecordedStep], wb: &Workbook) -> Converted {
+/// `ran` is what a script step ran, by step index ([`crate::record::Recorder::ran`]); a script step
+/// without an entry is read from `wb`.
+pub fn convert(name: &str, start: &RecordingStart, steps: &[RecordedStep], ran: &BTreeMap<usize, Vec<(String, Json)>>, wb: &Workbook) -> Converted {
     let mut skipped = Vec::new();
     // Commands as they'll be played back: (id, params, sheet before, selection before, whether
     // they came from another macro and stay as written). At most the macro's step limit are
@@ -63,7 +66,7 @@ pub fn convert(name: &str, start: &RecordingStart, steps: &[RecordedStep], wb: &
             cut = cut.saturating_add(1);
         }
     };
-    for st in steps.iter().filter(|st| st.doc == start.doc) {
+    for (index, st) in steps.iter().enumerate().filter(|(_, st)| st.doc == start.doc) {
         let id = st.id.as_str();
         if id == "macro.run" {
             // Another macro's steps, when they stand on their own: no inputs, no output and only
@@ -85,8 +88,10 @@ pub fn convert(name: &str, start: &RecordingStart, steps: &[RecordedStep], wb: &
             continue;
         }
         if id == "automate.runScript" {
-            // An Automate script's steps, as the script runs them.
-            match st.params.get("name").and_then(Json::as_str).and_then(|n| crate::cmd::extra::script_steps(wb, n)) {
+            // An Automate script's steps as they ran, or as the script stands now when the recorder
+            // could not keep them.
+            let now = || st.params.get("name").and_then(Json::as_str).and_then(|n| crate::cmd::extra::script_steps(wb, n));
+            match ran.get(&index).cloned().or_else(now) {
                 Some(steps) => {
                     for (cid, params) in steps {
                         if refused(&cid, &params) {

crates/engine/src/record.rs

@@ -8,6 +8,7 @@
 
 use serde::Serialize;
 use serde_json::Value as Json;
+use std::collections::BTreeMap;
 
 use crate::Selection;
 
@@ -46,25 +47,43 @@ pub struct Recorder {
     /// Once a step is dropped, every later one is too, so `steps` is always the beginning of
     /// what ran, never a sequence with gaps.
     pub dropped: usize,
-    /// The size of `steps` serialized as JSON.
+    /// The size of `steps` and `ran` serialized as JSON.
     bytes: usize,
+    /// What a step ran, by its index in `steps`, for a command that runs steps stored in the
+    /// workbook (`automate.runScript`): the commands as they ran, so a recording keeps them even
+    /// when the script is edited or deleted before the recording stops. A step without an entry
+    /// is read from the workbook when the recording is used.
+    pub ran: BTreeMap<usize, Vec<(String, Json)>>,
 }
 
 impl Recorder {
-    /// Adds a step, or counts it as dropped once the buffer is full.
-    pub fn push(&mut self, step: RecordedStep) {
+    /// Adds a step, or counts it as dropped once the buffer is full; `true` when it was kept.
+    pub fn push(&mut self, step: RecordedStep) -> bool {
         // Once a step has been dropped, or the step count is full, the rest are counted without
         // being serialized (a `file.open` step holds a whole file).
         if self.dropped > 0 || self.steps.len() >= RECORD_LIMIT {
             self.dropped = self.dropped.saturating_add(1);
-            return;
+            return false;
         }
         let size = serde_json::to_vec(&step).map_or(0, |v| v.len());
         if self.bytes.saturating_add(size) <= RECORD_MAX_BYTES {
             self.bytes = self.bytes.saturating_add(size);
             self.steps.push(step);
+            true
         } else {
             self.dropped = self.dropped.saturating_add(1);
+            false
+        }
+    }
+
+    /// Attaches what the last kept step ran ([`Recorder::ran`]), within the byte budget: past it,
+    /// the step stays and its script is read from the workbook when the recording is used.
+    pub fn attach_ran(&mut self, steps: Vec<(String, Json)>) {
+        let Some(index) = self.steps.len().checked_sub(1) else { return };
+        let size = serde_json::to_vec(&steps).map_or(0, |v| v.len());
+        if self.bytes.saturating_add(size) <= RECORD_MAX_BYTES {
+            self.bytes = self.bytes.saturating_add(size);
+            self.ran.insert(index, steps);
         }
     }
 }

crates/engine/src/tests/macros.rs

@@ -1372,6 +1372,29 @@ fn a_recorded_script_run_leaves_out_the_steps_a_macro_may_not_run() {
     s.execute("macro.run", json!({"name": "Rec", "onConflict": "overwrite"})).unwrap();
 }
 
+#[test]
+fn a_recorded_script_run_keeps_the_steps_that_ran() {
+    // One script is edited and another deleted before the recording stops: the macro holds the
+    // steps as they ran, not the scripts as they stand when the recording is saved.
+    let mut s = s();
+    let step = |input: &str| json!([{"command": "cell.set", "params": {"cell": "B1", "input": input}}]);
+    s.execute("automate.newScript", json!({"name": "Edited", "commands": step("first")})).unwrap();
+    s.execute("automate.newScript", json!({"name": "Gone", "commands": step("kept")})).unwrap();
+    s.execute("macro.record", json!({"relative": false})).unwrap();
+    s.execute("automate.runScript", json!({"name": "Edited"})).unwrap();
+    s.execute("automate.runScript", json!({"name": "Gone"})).unwrap();
+    s.execute("automate.newScript", json!({"name": "Edited", "commands": step("changed")})).unwrap();
+    s.execute("automate.deleteScript", json!({"name": "Gone"})).unwrap();
+    let r = s.execute("macro.record", json!({"stop": true, "name": "Rec"})).unwrap();
+    let steps = recorded(&mut s, "Rec");
+    assert_eq!(
+        steps.get(..2),
+        Some(&["cell.set {\"cell\":\"B1\",\"input\":\"first\"}".to_string(), "cell.set {\"cell\":\"B1\",\"input\":\"kept\"}".to_string()][..]),
+        "{steps:?}"
+    );
+    assert!(!r["skipped"].as_array().unwrap().contains(&json!("automate.runScript")), "{r}");
+}
+
 #[test]
 fn a_recording_is_saved_in_the_workbook_where_it_started() {
     let mut s = s();

docs/macros.md

@@ -155,7 +155,8 @@ inferred. Toggles such as Bold are recorded with the state they set (`"on": true
 them back sets them rather than flipping them. Commands a macro may not run are left out and
 listed in the result, also from the steps of a macro or script run while recording; a macro run
 while recording is replaced by its steps when it has no inputs, no output and only plain commands
-(otherwise it is left out and listed), and an Automate script run by the script's steps.
+(otherwise it is left out and listed), and an Automate script run by the steps it ran, kept when
+it ran: editing or deleting the script before the recording stops changes nothing.
 
 ## Other commands and storage