From 58608579378798be2f574042dc9f92b30c884ec4 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:32:37 -0400 Subject: [PATCH 1/7] Engine: a macro run checks each step for writes over data; the check before the run moves to its own pull request The review asked for the runner to be split. This pull request keeps the policy, the input checks, the step machine and `macro.run`; working out before a run where a macro writes (the footprint dry run, shifting data to make room, `macro.check`) moves to the pull request above it. - Unless the conflict rule is `overwrite`, every run now checks each step as it runs, which the run did before only after a partial footprint: a step that writes over cells outside the inputs that held data when the run started, or deletes sheets or objects that were there, stops the run, which asks (`ask`) or fails, and the run rolls back. - `shiftDown` and `shiftRight` need the footprint, so here they stop the run as `fail` does. - Moved: `Footprint`, `Saved`, `footprint`, `diff`, `make_room`, `macro.check`, the dry-run branches of the machine, `policy::previewable`, `DocState::touched` and the touch log, `Session::journal_dropped` and `truncate_journal`, and the tests of those. - Test: `each_step_is_checked_for_writes_over_data` (the review's counterexample: a permitted file step or Solver, then a write over B1, under `fail` and `ask`). --- crates/engine/src/cmd/macros.rs | 34 +-- crates/engine/src/cmd/mod.rs | 12 - crates/engine/src/lib.rs | 23 -- crates/engine/src/macros/policy.rs | 7 - crates/engine/src/macros/run.rs | 472 ++--------------------------- crates/engine/src/tests/macros.rs | 375 +++-------------------- docs/macros.md | 37 +-- 7 files changed, 100 insertions(+), 860 deletions(-) diff --git a/crates/engine/src/cmd/macros.rs b/crates/engine/src/cmd/macros.rs index 53856e720..d81902b1b 100644 --- a/crates/engine/src/cmd/macros.rs +++ b/crates/engine/src/cmd/macros.rs @@ -6,26 +6,15 @@ use serde_json::Value as Json; use super::*; pub fn specs() -> Vec { - vec![ - cmd!( - "macro.run", - "Run Macro", - [], - None, - "{name, inputs?: {Input: \"B2:D9\"} | \"B2:D9\", onConflict?: ask|fail|overwrite|shiftDown|shiftRight, overwrite?: bool, approve?: bool, timeLimit?: seconds} runs as one undo step; any error rolls it all back → {macro, ran, inputs, stopped?, shifted?}", - has_doc, - run - ), - cmd!( - query "macro.check", - "Check Macro", - [], - None, - "{name, inputs?, timeLimit?: seconds} validates the inputs and dry-runs the macro on the workbook (then puts everything back) → {ok, errors?, needsApproval?, inputs, output?, conflicts, footprint}", - has_doc, - check - ), - ] + vec![cmd!( + "macro.run", + "Run Macro", + [], + None, + "{name, inputs?: {Input: \"B2:D9\"} | \"B2:D9\", onConflict?: ask|fail|overwrite|shiftDown|shiftRight, overwrite?: bool, approve?: bool, timeLimit?: seconds} runs as one undo step; any error rolls it all back → {macro, ran, inputs, stopped?, shifted?}", + has_doc, + run + )] } fn name_param<'a>(cmd: &str, p: &'a Json) -> Result<&'a str> { @@ -36,8 +25,3 @@ fn run(s: &mut Session, p: &Json) -> Result { let name = name_param("macro.run", p)?; crate::macros::run::run(s, name, p) } - -fn check(s: &mut Session, p: &Json) -> Result { - let name = name_param("macro.check", p)?; - crate::macros::run::check(s, name, p) -} diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 90fe4ccc8..6f58f18ee 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -366,13 +366,6 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) if !structural { check_arrays(&d.wb, &wb)?; } - if let Some(log) = d.touched.as_mut() { - for (si, c) in changed.iter().take(TOUCH_LOG_LIMIT.saturating_sub(log.len())) { - if let Some(sh) = wb.sheet(*si) { - log.push((sh.name.clone(), *c)); - } - } - } if changed.len() <= 200_000 { fit_rows.extend(changed.iter().map(|(s, c)| (*s, c.row))); } @@ -400,11 +393,6 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) Ok(r) } -/// The most written cells a macro's footprint check records. -/// The most cells a dry run's touch log keeps; a footprint whose log is full may be missing writes -/// that kept a cell's value, so a shift is refused then. -pub(crate) const TOUCH_LOG_LIMIT: usize = 1_000_000; - /// Excel's message for an edit of a locked cell on a protected sheet. pub(crate) const PROTECTED: &str = "The cell or chart you're trying to change is on a protected sheet."; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 9d4d2f93b..151c2bc06 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -206,9 +206,6 @@ pub struct DocState { /// workbook is opened from a file, carried by content pasted from such a workbook, and never /// changed by a trust decision. pub trust: Trust, - /// While a macro's footprint is measured: every cell a command wrote, by sheet name, even - /// where it wrote what was already there. - pub touched: Option>, /// The references `edit.goTo` with `record: true` went to in this document, newest first, /// without names, each area written with its sheet (`Sheet1!B14:C22,Sheet1!F19:G30`). pub go_to_recent: Vec, @@ -302,7 +299,6 @@ impl DocState { content_kind: None, scripts_trusted, trust, - touched: None, go_to_recent: vec![], } } @@ -438,9 +434,6 @@ pub struct Session { /// Serialized size of each journal entry, measured once when it is pushed, and their sum. journal_sizes: Vec, journal_bytes: usize, - /// Entries the journal has dropped from its front to stay within its limits, ever; a dry run - /// that puts the journal back needs to know how many went while it ran. - journal_dropped: u64, pub ui_requests: Vec, /// Format painter source style (sticky when double-clicked). pub format_painter: Option<(Arc, usize, RangeRef, bool)>, @@ -565,26 +558,10 @@ impl Session { self.journal.drain(..n); self.journal_sizes.drain(..n); self.journal_bytes = kept; - self.journal_dropped = self.journal_dropped.saturating_add(n as u64); self.recording = self.recording.map(|r| r.saturating_sub(n)); } } - /// Entries dropped from the journal's front so far, for a caller that puts the journal back. - pub(crate) fn journal_dropped(&self) -> u64 { - self.journal_dropped - } - - /// Cuts the journal back to its first `keep` entries, with its size bookkeeping. - pub(crate) fn truncate_journal(&mut self, keep: usize) { - if self.journal_sizes.len() == self.journal.len() { - self.journal_sizes.truncate(keep); - self.journal_bytes = self.journal_sizes.iter().sum(); - } - self.journal.truncate(keep); - self.recording = self.recording.map(|r| r.min(self.journal.len())); - } - pub fn commands(&self) -> Vec { command_specs().iter().map(|c| c.info(self)).collect() } diff --git a/crates/engine/src/macros/policy.rs b/crates/engine/src/macros/policy.rs index 8f23a5b40..57d974920 100644 --- a/crates/engine/src/macros/policy.rs +++ b/crates/engine/src/macros/policy.rs @@ -73,12 +73,6 @@ pub fn macro_policy(id: &str, params: &Json, trust: Trust) -> Policy { } } -/// Whether the footprint dry run can preview a command. Solver can take long, so a dry run stops -/// before it and reports the footprint as partial. -pub fn previewable(id: &str) -> bool { - id != "data.solver" -} - #[cfg(test)] mod tests { use super::*; @@ -129,7 +123,6 @@ mod tests { assert_eq!(macro_policy(id, &none, Trust::Local), Policy::Allow, "{id}"); assert!(matches!(macro_policy(id, &none, Trust::FromFile), Policy::Ask(_)), "{id}"); } - assert!(!previewable("data.solver") && previewable("cell.set")); } #[test] diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index f45bcfc25..8ae1e3496 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -6,12 +6,12 @@ //! approves it (once per macro content, for the session). //! 2. **Inputs**: each declared input is bound to a range (given, its default, or the selection) //! and checked against its shape and column types, with messages that name the cells. -//! 3. **Footprint**: unless the macro may overwrite, it first runs on the real workbook in dry-run -//! mode, the changes are compared with `CellStore::diff`, and everything is put back. Cells it -//! would write outside its inputs that already hold data are conflicts, handled by the output -//! rule: ask, fail, overwrite, or shift the data down or right (and check again). A dry run -//! that stops early (a file step, Solver) proves nothing about later steps, so the run then -//! checks each step as it goes. +//! 3. **Conflicts**: unless the macro may overwrite, each step is checked as it runs. A step that +//! writes over data outside the inputs (cells that held data when the run started, sheets and +//! objects it deletes) stops the run, which asks (`ask`) or fails (the other rules), and the +//! run rolls back. Shifting the data to make room (`shiftDown`, `shiftRight`) needs to know +//! where the macro writes before it runs, which this version doesn't work out, so those rules +//! stop the run as `fail` does. //! 4. **Run**: one undo step (`Session::grouped`); an error at any step rolls everything back and //! names the step. //! @@ -26,9 +26,9 @@ use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, S use serde::Serialize; use serde_json::{Value as Json, json}; -use super::policy::{Policy, macro_policy, previewable}; +use super::policy::{Policy, macro_policy}; use super::resolve::{self, Env, Place, ResolveError}; -use crate::{EngineError, Result, Selection, Session, Trust, UiRequest}; +use crate::{EngineError, Result, Session, Trust, UiRequest}; /// The most steps one run executes (counting each pass through a Repeat). pub const MAX_EXECUTED_STEPS: u64 = 100_000; @@ -37,7 +37,7 @@ pub const MAX_EXECUTED_STEPS: u64 = 100_000; pub const DEFAULT_TIME_LIMIT_MS: f64 = 60_000.0; /// The most cells an input may have for its content to be checked. pub const MAX_INPUT_CELLS: u64 = 1_000_000; -/// The most ranges a footprint lists per sheet (the counts stay exact). +/// The most ranges a list of cells gives per sheet (the counts stay exact). const MAX_LISTED_RANGES: usize = 500; /// An input bound to a range: name, where, whether its first row is a header. @@ -421,37 +421,28 @@ pub struct Machine { pub executed: u64, clock: Stopwatch, pub time_limit_ms: f64, - /// Dry run (the footprint check): steps that would touch files or can't be previewed stop - /// the run early instead of running. - pub dry_run: bool, /// A Stop step ended the run: its message, and whether it asked for a rollback. pub stopped: Option<(String, bool)>, - /// A dry run stopped before this step (path, reason): what follows isn't in the footprint. - pub partial: Option<(String, String)>, /// The last step's resolved parameters (for the debugger). pub last_params: Option, - /// Set when the footprint was partial: each step is checked for writes over data as it runs. + /// Set unless the macro may overwrite: each step is checked for writes over data as it runs. guard: Option, } -/// Checks each step of a real run for writes over data the footprint couldn't see. +/// Checks each step of a run for writes over data the workbook held when the run started. #[derive(Clone, Debug)] struct Guard { /// The workbook when the run started, and for each sheet now, its index there (`None` for a /// sheet the run added). start: Arc, origin: Vec>, - /// Where the footprint stopped: "step 2 (file.exportCsv writes a file)". - partial: String, /// The conflict rule is ask. ask: bool, } impl Guard { - fn new(start: Arc, partial: &Json, ask: bool) -> Guard { - let text = |k: &str| partial.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); - let partial = format!("step {} ({})", text("step"), text("reason")); - Guard { origin: (0..start.sheets.len()).map(Some).collect(), start, partial, ask } + fn new(start: Arc, ask: bool) -> Guard { + Guard { origin: (0..start.sheets.len()).map(Some).collect(), start, ask } } /// What one step wrote over that held data when the run started: cells outside the inputs, @@ -512,7 +503,7 @@ fn deleted(start: &[T], prev: &[T], after: &[T], id: impl Fn(&T) -> u32) -> u } impl Machine { - pub fn new(p: &Prepared, dry_run: bool) -> Machine { + pub fn new(p: &Prepared) -> Machine { let mut m = Machine { name: p.mac.name.clone(), doc: p.doc, @@ -524,9 +515,7 @@ impl Machine { executed: 0, clock: Stopwatch::start(), time_limit_ms: DEFAULT_TIME_LIMIT_MS, - dry_run, stopped: None, - partial: None, last_params: None, guard: None, }; @@ -677,19 +666,9 @@ impl Machine { if !matches!(written, Policy::NeedsPermission(_)) { return Err(fail(format!("{id} {what} only once its parameters are filled in, so it wasn't approved."))); } - if self.dry_run { - self.partial = Some((path.to_string(), format!("{id} {what}"))); - self.frames.clear(); - return Ok(()); - } } (_, Policy::Allow | Policy::Ask(_)) => {} } - if self.dry_run && !previewable(id) { - self.partial = Some((path.to_string(), format!("{id} can't be previewed"))); - self.frames.clear(); - return Ok(()); - } self.last_params = Some(resolved.clone()); let prev = self.guard.as_ref().and_then(|_| s.docs.iter().find(|d| d.uid == self.doc)).map(|d| d.wb.clone()); let queued = s.ui_requests.len(); @@ -714,7 +693,7 @@ impl Machine { let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { - let msg = format!("It would write over data ({what}); the check before the run stopped at {} and couldn't see this.", g.partial); + let msg = format!("It would write over data ({what})."); return Err(StepError { confirm: g.ask, ..fail(msg) }); } } @@ -764,255 +743,7 @@ fn compare(v: &Value, op: CompareOp, want: &Json) -> bool { } } -// ---------------------------------------------------------------- footprint - -/// What a macro would change, from a dry run on the real workbook. -#[derive(Clone, Debug, Default, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct Footprint { - /// Steps executed by the end of the dry run that made this footprint (the run's one budget). - #[serde(skip)] - pub executed: u64, - /// The dry run wrote more cells than the touch log keeps, so writes that kept a cell's value - /// may be missing from `cells`: a shift cannot trust the block. - #[serde(skip_serializing_if = "std::ops::Not::not")] - pub log_full: bool, - pub sheets: Vec, - pub sheets_added: Vec, - pub sheets_removed: Vec, - pub objects_added: usize, - pub objects_changed: usize, - pub objects_removed: usize, - /// The dry run stopped early before this step; later steps aren't in the footprint. - #[serde(skip_serializing_if = "Option::is_none")] - pub partial: Option, - /// The step the run would fail at. - #[serde(skip_serializing_if = "Option::is_none")] - pub error: Option, - /// A Stop step's message. - #[serde(skip_serializing_if = "Option::is_none")] - pub stopped: Option, -} - -/// The cells a macro would write on one sheet, outside its inputs. -#[derive(Clone, Debug, Default, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct SheetFootprint { - pub sheet: String, - #[serde(skip)] - pub index: usize, - /// The written cells as ranges (at most 500 listed), and how many cells. - pub cells: Vec, - pub changed: usize, - /// Written cells that hold data now. - pub conflicts: Vec, - pub conflict_count: usize, - /// The block that holds every written cell. - #[serde(skip_serializing_if = "Option::is_none")] - pub bounds: Option, - #[serde(skip)] - pub bounds_range: Option, -} - -impl Footprint { - /// Cells that hold data and would be written, plus sheets and objects that would go. - pub fn conflict_count(&self) -> usize { - self.sheets.iter().map(|s| s.conflict_count).sum::() + self.sheets_removed.len() + self.objects_removed - } - - /// "C2:C9 on Sheet1, the sheet Old, 2 objects". - pub fn describe_conflicts(&self) -> String { - let mut parts: Vec = self - .sheets - .iter() - .filter(|s| s.conflict_count > 0) - .map(|s| { - let mut listed = s.conflicts.iter().take(3).cloned().collect::>().join(", "); - if s.conflicts.len() > 3 { - listed.push_str(", …"); - } - format!("{listed} on {}", s.sheet) - }) - .collect(); - parts.extend(self.sheets_removed.iter().map(|n| format!("the sheet {n}"))); - if self.objects_removed > 0 { - parts.push(format!("{} object{}", self.objects_removed, if self.objects_removed == 1 { "" } else { "s" })); - } - parts.join("; ") - } -} - -/// Session state a dry run may touch, put back afterwards. The workbook, its selection and its -/// history are put back by the dry run's undo group. -struct Saved { - active: usize, - docs: Vec, - /// The workbook as last saved: `sheet.activate` and the view commands move it to the - /// current workbook on a clean document, and the rollback puts the workbook back but not this. - saved: Option>, - sheet_selections: Vec, - unlocked_ranges: Vec<(String, String)>, - trust: Trust, - clipboard: Option, - prefs: crate::Prefs, - mode: crate::Mode, - /// The journal's length and how many entries it had dropped from its front; a dry run that - /// pushes past the journal's limits drops more, which a length alone cannot see. - journal: usize, - journal_dropped: u64, - recording: Option, - ui_requests: usize, - format_painter: Option<(Arc, usize, RangeRef, bool)>, - last_find: Option, - hide_ruler: bool, - hidden_windows: Vec, - draw: (String, String, f32), - comments_pane: bool, - show_all_notes: bool, -} - -impl Saved { - fn take(s: &mut Session, doc: u64) -> Saved { - let d = s.docs.iter().find(|d| d.uid == doc); - Saved { - active: s.active, - docs: s.docs.iter().map(|d| d.uid).collect(), - saved: d.map(|d| d.saved.clone()), - sheet_selections: d.map(|d| d.sheet_selections.clone()).unwrap_or_default(), - unlocked_ranges: d.map(|d| d.unlocked_ranges.clone()).unwrap_or_default(), - trust: d.map_or(Trust::Local, |d| d.trust), - clipboard: s.clipboard.clone(), - prefs: s.prefs.clone(), - mode: s.mode, - journal: s.journal.len(), - journal_dropped: s.journal_dropped(), - recording: s.recording, - ui_requests: s.ui_requests.len(), - format_painter: s.format_painter.clone(), - last_find: s.last_find.clone(), - hide_ruler: s.hide_ruler, - hidden_windows: s.hidden_windows.clone(), - draw: (s.draw_tool.clone(), s.draw_color.clone(), s.draw_width), - comments_pane: s.comments_pane, - show_all_notes: s.show_all_notes, - } - } - - fn restore(self, s: &mut Session, doc: u64) { - s.docs.retain(|d| self.docs.contains(&d.uid)); - if let Some(d) = s.docs.iter_mut().find(|d| d.uid == doc) { - if let Some(saved) = self.saved { - d.saved = saved; - } - d.sheet_selections = self.sheet_selections; - d.unlocked_ranges = self.unlocked_ranges; - d.trust = self.trust; - } - s.active = self.active.min(s.docs.len().saturating_sub(1)); - s.clipboard = self.clipboard; - s.prefs = self.prefs; - s.mode = self.mode; - // Entries the dry run pushed go; entries the journal dropped meanwhile are gone for good, - // and the recording's start moves back with them. - let dropped = usize::try_from(s.journal_dropped().saturating_sub(self.journal_dropped)).unwrap_or(usize::MAX); - s.truncate_journal(self.journal.saturating_sub(dropped)); - s.recording = self.recording.map(|r| r.saturating_sub(dropped)); - s.ui_requests.truncate(self.ui_requests); - s.format_painter = self.format_painter; - s.last_find = self.last_find; - s.hide_ruler = self.hide_ruler; - s.hidden_windows = self.hidden_windows; - (s.draw_tool, s.draw_color, s.draw_width) = self.draw; - s.comments_pane = self.comments_pane; - s.show_all_notes = self.show_all_notes; - } -} - -/// Runs the macro in dry-run mode on its workbook, puts everything back, and says what it would -/// change. The dry run stops once `time_limit_ms` has passed on `clock`. -pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: f64, executed: u64) -> Result { - let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?; - s.set_active(i); - let saved = Saved::take(s, p.doc); - let before = s.doc()?.wb.clone(); - let token = s.group_begin("Check Macro")?; - if let Some(d) = s.docs.iter_mut().find(|d| d.uid == p.doc) { - d.touched = Some(Vec::new()); - } - let mut m = Machine::new(p, true); - (m.clock, m.time_limit_ms, m.executed) = (clock, time_limit_ms, executed); - let outcome = m.run_to_end(s); - let (after, touched) = match s.docs.iter_mut().find(|d| d.uid == p.doc) { - Some(d) => (d.wb.clone(), d.touched.take().unwrap_or_default()), - None => (before.clone(), Vec::new()), - }; - let rolled = s.group_rollback(token); - saved.restore(s, p.doc); - rolled?; - let (after, touched) = if matches!(m.stopped, Some((_, true))) { (before.clone(), Vec::new()) } else { (after, touched) }; - let mut fp = diff(&before, &after, &p.inputs, &touched); - fp.log_full = touched.len() >= crate::cmd::TOUCH_LOG_LIMIT; - fp.executed = m.executed; - fp.error = outcome.err(); - fp.stopped = m.stopped.map(|(msg, _)| msg); - fp.partial = m.partial.map(|(step, reason)| json!({"step": step, "reason": reason})); - Ok(fp) -} - -/// Compares the workbook before and after a dry run. `touched` adds the cells commands wrote -/// without changing them (a formula written again), so a shift moves everything the macro writes. -fn diff(before: &Workbook, after: &Workbook, inputs: &[Bound], touched: &[(String, CellRef)]) -> Footprint { - let mut fp = Footprint::default(); - let (pairs, added) = pair_sheets(before, after); - fp.sheets_added = added.into_iter().filter_map(|a| after.sheet(a).map(|s| s.name.clone())).collect(); - for (bi, ai) in pairs { - let Some(bs) = before.sheets.get(bi) else { continue }; - let Some(ai) = ai else { - fp.sheets_removed.push(bs.name.clone()); - continue; - }; - let Some(after_sheet) = after.sheets.get(ai) else { continue }; - let rewritten: BTreeSet = touched.iter().filter(|(n, _)| *n == after_sheet.name).map(|(_, c)| *c).collect(); - if Arc::ptr_eq(bs, after_sheet) && rewritten.is_empty() { - continue; - } - let (added, changed, removed) = objects(bs, after_sheet); - fp.objects_added += added; - fp.objects_changed += changed; - fp.objects_removed += removed; - let in_input = |c: CellRef| inputs.iter().any(|(_, p, _)| p.sheet == bi && p.range.contains(c)); - let mut written = Vec::new(); - let mut conflicts = Vec::new(); - let mut cells: BTreeSet = bs.cells.diff(&after_sheet.cells).into_iter().collect(); - cells.extend(rewritten.iter().copied()); - for c in cells { - if in_input(c) { - continue; - } - let (b, a) = (bs.cell(c), after_sheet.cell(c)); - if recalculated_only(b, a) && !rewritten.contains(&c) { - continue; - } - written.push(c); - if b.is_some_and(has_content) && content_differs(b, a) { - conflicts.push(c); - } - } - let Some(first) = written.first() else { continue }; - let bounds = written.iter().fold(RangeRef::cell(*first), |r, c| r.union(&RangeRef::cell(*c))); - fp.sheets.push(SheetFootprint { - sheet: bs.name.clone(), - index: bi, - changed: written.len(), - conflict_count: conflicts.len(), - cells: ranges(&mut written), - conflicts: ranges(&mut conflicts), - bounds: Some(bounds.a1()), - bounds_range: Some(bounds), - }); - } - fp -} +// ---------------------------------------------------------------- comparing workbooks /// Each sheet of `before` with its index in `after` (`None` when deleted), and the sheets of /// `after` that are new. Sheets are matched by name; what's left is matched in order (a renamed @@ -1054,18 +785,6 @@ fn content_differs(b: Option<&Cell>, a: Option<&Cell>) -> bool { } } -/// Objects added, changed and removed between two versions of a sheet. -fn objects(b: &Sheet, a: &Sheet) -> (usize, usize, usize) { - fn count(b: &[T], a: &[T], id: impl Fn(&T) -> u32) -> (usize, usize, usize) { - let added = a.iter().filter(|x| !b.iter().any(|y| id(y) == id(x))).count(); - let removed = b.iter().filter(|x| !a.iter().any(|y| id(y) == id(x))).count(); - let changed = b.iter().filter(|x| a.iter().any(|y| id(y) == id(x) && y != *x)).count(); - (added, changed, removed) - } - let parts = [count(&b.charts, &a.charts, |o| o.id), count(&b.images, &a.images, |o| o.id), count(&b.shapes, &a.shapes, |o| o.id)]; - parts.iter().fold((0, 0, 0), |t, p| (t.0 + p.0, t.1 + p.1, t.2 + p.2)) -} - /// Cells as A1 ranges: runs along each row, then runs with the same columns on following rows. pub(crate) fn ranges(cells: &mut [CellRef]) -> Vec { cells.sort_unstable_by_key(|c| (c.row, c.col)); @@ -1092,47 +811,6 @@ pub(crate) fn ranges(cells: &mut [CellRef]) -> Vec { merged.iter().take(MAX_LISTED_RANGES).map(RangeRef::a1).collect() } -/// Inserts blank cells where the footprint lands (moving what is there down or right), so the -/// macro writes into empty cells. The block is every written cell's, so a row the macro writes -/// stays together. A shift that would move one of the macro's inputs is refused: the inputs were -/// checked and bound where they are, and only the block's columns (or rows) would move. -fn make_room(s: &mut Session, fp: &Footprint, p: &Prepared, down: bool) -> Result<()> { - if !fp.sheets_removed.is_empty() || fp.objects_removed > 0 { - return Err(EngineError::Other("Shifting can't make room: the macro deletes sheets or objects.".into())); - } - if fp.log_full { - return Err(EngineError::Other("Shifting can't make room: the macro writes more cells than the footprint can follow.".into())); - } - let wb = s.doc()?.wb.clone(); - let env = Env { wb: &wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; - let mut moves = Vec::new(); - for sh in fp.sheets.iter().filter(|s| s.conflict_count > 0) { - let Some(block) = sh.bounds_range else { continue }; - // What the insertion moves: the block's columns from its top row down, or its rows from - // its left column rightwards. - let moved = |r: &RangeRef| { - if down { - r.start.col <= block.end.col && r.end.col >= block.start.col && r.end.row >= block.start.row - } else { - r.start.row <= block.end.row && r.end.row >= block.start.row && r.end.col >= block.start.col - } - }; - // The output is where the macro writes, so moving what is there is the point; the second - // footprint catches data the shift moved into it. - if let Some((name, pl)) = p.inputs.iter().map(|(n, pl, _)| (n.as_str(), *pl)).find(|(_, pl)| pl.sheet == sh.index && moved(&pl.range)) { - return Err(EngineError::Other(format!( - "Shifting the data would move {name} ({}). Run with onConflict overwrite or fail, or make room first.", - resolve::a1(pl, &env) - ))); - } - moves.push(resolve::a1(Place { sheet: sh.index, range: block }, &env)); - } - for range in moves { - s.execute("home.insertCells", json!({"range": range, "shift": if down { "down" } else { "right" }}))?; - } - Ok(()) -} - // ---------------------------------------------------------------- run /// A macro that passed its checks and is about to run. @@ -1162,62 +840,19 @@ pub fn begin(s: &mut Session, name: &str, p: &Json) -> Result { Ok(Begun { mac: m, prepared, rule, time_limit_ms, inputs }) } -/// The footprint check and the output rule, inside the run's undo group: an error when the macro -/// would fail or write over data it may not, after shifting the data if the rule says so. Returns -/// the machine for the run and whether it shifted. The dry runs count towards the run's time -/// limit. -/// -/// A partial footprint proves nothing about the steps it didn't reach, so then the machine -/// checks each step as it runs and stops the run at one that writes over data. -pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { - let (m, rule) = (&b.mac, b.rule); - let mut machine = Machine::new(&b.prepared, false); +/// The output rule, inside the run's undo group: the machine for the run. Unless the macro may +/// overwrite, the machine checks each step as it runs and stops the run at one that writes over +/// data the workbook held when the run started (asking under `ask`, failing otherwise); the run's +/// undo group then rolls back. Shifting needs to know where the macro writes before it runs, so +/// `shiftDown` and `shiftRight` stop the run as `fail` does. +pub fn make_way(s: &mut Session, b: &Begun) -> Result { + let mut machine = Machine::new(&b.prepared); machine.time_limit_ms = b.time_limit_ms; - if rule == OnConflict::Overwrite { - return Ok((machine, false)); - } - // The dry runs and the run share one step budget as they share one clock. - let mut fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?; - machine.executed = fp.executed; - if let Some(e) = fp.error.take() { - return Err(e.into_engine(&m.name)); - } - let mut shifted = false; - if fp.conflict_count() > 0 { - let what = fp.describe_conflicts(); - match rule { - OnConflict::Ask => { - return Err(EngineError::Confirm(format!( - "Macro '{}' would write over cells that hold data ({what}). Overwrite them, or run again with onConflict shiftDown, shiftRight or fail", - m.name - ))); - } - OnConflict::Fail | OnConflict::Overwrite => { - return Err(EngineError::Other(format!("Macro '{}' would write over cells that hold data ({what}). Nothing was changed.", m.name))); - } - OnConflict::ShiftDown | OnConflict::ShiftRight => { - make_room(s, &fp, &b.prepared, rule == OnConflict::ShiftDown)?; - fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?; - machine.executed = fp.executed; - if let Some(e) = fp.error.take() { - return Err(e.into_engine(&m.name)); - } - if fp.conflict_count() > 0 { - return Err(EngineError::Other(format!( - "Macro '{}': shifting the data didn't make room ({}). Nothing was changed.", - m.name, - fp.describe_conflicts() - ))); - } - shifted = true; - } - } - } - if let Some(p) = &fp.partial { + if b.rule != OnConflict::Overwrite { let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); - machine.guard = Some(Guard::new(start, p, rule == OnConflict::Ask)); + machine.guard = Some(Guard::new(start, b.rule == OnConflict::Ask)); } - Ok((machine, shifted)) + Ok(machine) } /// Runs macro `name` on the active workbook: `{inputs?, onConflict?, overwrite?, approve?, @@ -1226,21 +861,16 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result { let b = begin(s, name, p)?; let label = format!("Run Macro: {}", b.mac.name); s.grouped(&label, |s| { - let (mut machine, shifted) = make_way(s, &b)?; + let mut machine = make_way(s, &b)?; let name = &b.mac.name; - // `ran` counts the run's own steps; the budget also holds the dry runs'. - let before_run = machine.executed; machine.run_to_end(s).map_err(|e| e.into_engine(name))?; match machine.stopped { Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} Nothing was changed."))), stopped => { - let mut out = json!({"macro": name, "ran": machine.executed.saturating_sub(before_run), "inputs": b.inputs}); + let mut out = json!({"macro": name, "ran": machine.executed, "inputs": b.inputs}); if let Some((msg, _)) = stopped { out["stopped"] = json!(msg); } - if shifted { - out["shifted"] = json!(b.rule.name()); - } Ok(out) } } @@ -1265,49 +895,3 @@ fn conflict_rule(p: &Json, m: &Macro) -> Result { } Ok(rule) } - -/// `macro.check`: what running the macro would do, without changing anything. -pub fn check(s: &mut Session, name: &str, p: &Json) -> Result { - let d = s.doc()?; - let (doc, trust) = (d.uid, d.trust); - let m = super::find(&d.wb, name) - .and_then(|i| d.wb.macros.get(i)) - .cloned() - .ok_or_else(|| EngineError::Other(format!("There is no macro called '{name}'.")))?; - let mut out = json!({"macro": m.name, "ok": false, "onConflict": m.output.on_conflict.name()}); - let asks = match gate(&m, trust) { - Ok(a) => a, - Err(e) => { - out["errors"] = json!([e.to_string()]); - return Ok(out); - } - }; - let approved = s.macro_approvals.contains(&(doc, super::fingerprint(&m))); - if !asks.is_empty() && !approved { - // Nothing from an unapproved macro runs, not even in a dry run. - out["needsApproval"] = json!(asks); - } - let prepared = match prepare(s, &m, p.get("inputs")) { - Ok(pr) => pr, - Err(e) => { - out["errors"] = json!([e.to_string()]); - return Ok(out); - } - }; - out["inputs"] = prepared.inputs_json(&s.doc()?.wb); - if let Some(o) = prepared.output { - let wb = &s.doc()?.wb; - out["output"] = json!(resolve::a1(o, &Env { wb, active_sheet: wb.active_sheet, inputs: &[], output: None, item: None })); - } - if !asks.is_empty() && !approved { - return Ok(out); - } - let fp = footprint(s, &prepared, Stopwatch::start(), time_limit(p), 0)?; - out["ok"] = json!(fp.error.is_none()); - out["conflicts"] = json!(fp.conflict_count()); - if let Some(e) = &fp.error { - out["errors"] = json!([e.describe()]); - } - out["footprint"] = serde_json::to_value(&fp).unwrap_or(Json::Null); - Ok(out) -} diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 672c28d24..262f50af9 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -1,4 +1,5 @@ -//! Macros: running, input checks, the footprint dry run, the policy and the limits. +//! Macros: running, input checks, checking each step for writes over data, the policy and the +//! limits. use std::sync::Arc; @@ -156,23 +157,18 @@ fn conflicts_ask_fail_or_overwrite() { let start = s.doc().unwrap().wb.clone(); let r = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5"})); let Err(EngineError::Confirm(q)) = r else { panic!("{r:?}") }; - assert!(q.contains("would write over cells that hold data (E3 on Sheet1)"), "{q}"); + assert!(q.contains("Step 2 (cell.set): It would write over data (E3 on Sheet1)"), "{q}"); assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "programmatic runs default to failing"); let err = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": "fail"})).unwrap_err().to_string(); assert!(err.contains("Nothing was changed"), "{err}"); - assert!(s.execute("macro.run", json!({"name": "Totals", "onConflict": "sideways"})).is_err()); - - // macro.check shows the footprint and the conflict, and changes nothing. - let journal = s.journal.len(); - let sel = s.doc().unwrap().selection.clone(); - let c = s.execute("macro.check", json!({"name": "Totals", "inputs": "B2:C5"})).unwrap(); - assert_eq!(c["ok"], true); - assert_eq!(c["conflicts"], 1); - assert_eq!(c["output"], "E2"); - assert_eq!(c["footprint"]["sheets"][0]["cells"], json!(["E2:E3"])); - assert_eq!(c["footprint"]["sheets"][0]["conflicts"], json!(["E3"])); assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); - assert_eq!((s.journal.len(), &s.doc().unwrap().selection), (journal, &sel)); + // Shifting needs to know where the macro writes before it runs: it stops as fail does. + for rule in ["shiftDown", "shiftRight"] { + let err = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": rule})).unwrap_err().to_string(); + assert!(err.contains("would write over data (E3 on Sheet1)"), "{rule}: {err}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{rule}"); + } + assert!(s.execute("macro.run", json!({"name": "Totals", "onConflict": "sideways"})).is_err()); // The answer to the question overwrites. s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "overwrite": true})).unwrap(); @@ -181,181 +177,45 @@ fn conflicts_ask_fail_or_overwrite() { } #[test] -fn shifting_makes_room_and_is_checked_again() { - for (rule, moved) in [("shiftDown", ["E4", "E5"]), ("shiftRight", ["F2", "F3"])] { - let mut s = s(); - data(&mut s); - set(&mut s, "E2", "old 1"); - set(&mut s, "E3", "old 2"); - put(&mut s, totals()); - let r = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": rule})).unwrap(); - assert_eq!(r["shifted"], rule); - assert_eq!((v(&s, "E2").display(), v(&s, "E3").display()), ("Total".into(), "400".into()), "{rule}"); - assert_eq!((v(&s, moved[0]).display(), v(&s, moved[1]).display()), ("old 1".into(), "old 2".into()), "{rule}"); - // The shift and the run are one undo step. - s.execute("edit.undo", json!({})).unwrap(); - assert_eq!(v(&s, "E2").display(), "old 1", "{rule}"); - assert!(v(&s, moved[1]).is_empty(), "{rule}"); +fn each_step_is_checked_for_writes_over_data() { + let dir = std::env::temp_dir().join(format!("gc-macro-guard-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let csv = dir.join("out.csv").to_string_lossy().to_string(); + let export = json!({"command": "file.exportCsv", "params": {"path": csv}}); + let solver = json!({"command": "data.solver", "params": {"objective": "C1", "variables": "A1", "constraints": ["A1 <= 5"], "method": "simplex", "saveModel": false}}); + let late = json!({"command": "cell.set", "params": {"cell": "B1", "input": "over"}}); + // A file step or Solver, then a step that writes over B1: the run stops there. + for first in [&export, &solver] { + let mut s = super::s(); + set(&mut s, "A1", "1"); + set(&mut s, "C1", "=A1"); + set(&mut s, "B1", "keep"); + put(&mut s, json!({"name": "Late", "inputs": [{"name": "Input"}], "permissions": ["file.exportCsv"], "steps": [first, late]})); + let start = s.doc().unwrap().wb.clone(); + for rule in ["fail", "ask"] { + let r = s.execute("macro.run", json!({"name": "Late", "inputs": "A1", "onConflict": rule, "approve": true})); + let e = match (rule, r) { + ("fail", Err(EngineError::Other(e))) | ("ask", Err(EngineError::Confirm(e))) => e, + (_, r) => panic!("{first} {rule}: {r:?}"), + }; + assert!(e.contains("Step 2 (cell.set): It would write over data (B1 on Sheet1)"), "{e}"); + assert_eq!(v(&s, "B1").display(), "keep", "{first} {rule}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{first} {rule}: rolled back"); + } + // The answer to the question overwrites. + s.execute("macro.run", json!({"name": "Late", "inputs": "A1", "overwrite": true})).unwrap(); + assert_eq!(v(&s, "B1").display(), "over"); } - // A cell written with what it already holds isn't a conflict, but it moves with the others, - // so rows stay together. + // A step that writes over what the macro itself wrote isn't a conflict. let mut s = super::s(); - for (cell, input) in [("F2", "x"), ("G2", "b"), ("F3", "f3"), ("G3", "g3")] { - set(&mut s, cell, input); - } + let again = |input: &str| json!({"command": "cell.set", "params": {"cell": "E1", "input": input}}); put( &mut s, - json!({"name": "Pair", "output": {"onConflict": "shiftDown"}, "steps": [ - {"command": "cell.set", "params": {"cell": "F2", "input": "a"}}, - {"command": "cell.set", "params": {"cell": "G2", "input": "b"}} - ]}), + json!({"name": "Twice", "permissions": ["file.exportCsv"], "output": {"onConflict": "fail"}, "steps": [again("a"), export, again("b")]}), ); - let c = s.execute("macro.check", json!({"name": "Pair"})).unwrap(); - assert_eq!((c["conflicts"].as_u64(), &c["footprint"]["sheets"][0]["cells"]), (Some(1), &json!(["F2:G2"]))); - s.execute("macro.run", json!({"name": "Pair"})).unwrap(); - let row = |s: &Session, r: u32| (v(s, &format!("F{r}")).display(), v(s, &format!("G{r}")).display()); - assert_eq!(row(&s, 2), ("a".into(), "b".into())); - assert_eq!(row(&s, 3), ("x".into(), "b".into())); - assert_eq!(row(&s, 4), ("f3".into(), "g3".into())); - // An anchor that moves with the shifted data still lands on it: the second check refuses. - let mut s = super::s(); - data(&mut s); - set(&mut s, "E2", "old"); - s.execute("formulas.defineName", json!({"name": "Target", "refersTo": "=Sheet1!$E$2"})).unwrap(); - put( - &mut s, - json!({"name": "Named", "steps": [{"command": "cell.set", "params": {"cell": "Name:Target", "input": "new"}}], "output": {"onConflict": "shiftDown"}}), - ); - let start = s.doc().unwrap().wb.clone(); - let err = s.execute("macro.run", json!({"name": "Named"})).unwrap_err().to_string(); - assert!(err.contains("shifting the data didn't make room (E3 on Sheet1)"), "{err}"); - assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); -} - -#[test] -fn a_shift_that_would_move_an_input_is_refused() { - // A shift inserts cells over the conflicting block, which moves the block's columns (or rows) - // from there on. When one of the macro's own inputs lies in the way, its data would move and - // shear while the run keeps the old coordinates, so the shift is refused and nothing changes. - let mut s = s(); - data(&mut s); - set(&mut s, "B1", "Old title"); - put( - &mut s, - json!({"name": "Title", "inputs": [{"name": "Input", "header": true}], "steps": [ - {"command": "cell.set", "params": {"cell": "Input.cell(0,1)", "input": "Report"}} - ]}), - ); - let start = s.doc().unwrap().wb.clone(); - let err = s.execute("macro.run", json!({"name": "Title", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap_err().to_string(); - assert!(err.contains("would move Input (") && err.contains("B2:C5"), "{err}"); - assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "nothing changed"); - assert_eq!((v(&s, "B1").display(), v(&s, "B3").display()), ("Old title".into(), "Ann".into())); - // Writing left of the input and shifting right would move it too. - put( - &mut s, - json!({"name": "Left", "inputs": [{"name": "Input", "header": true}], "steps": [ - {"command": "cell.set", "params": {"cell": "Input.offset(1,-1).cell(1,1)", "input": "x"}} - ]}), - ); - set(&mut s, "A3", "in the way"); - let err = s.execute("macro.run", json!({"name": "Left", "inputs": "B2:C5", "onConflict": "shiftRight"})).unwrap_err().to_string(); - assert!(err.contains("would move Input"), "{err}"); - // The output counts as well. - put( - &mut s, - json!({"name": "Out", "inputs": [{"name": "Input", "header": true}], "output": {"anchor": "Input.below(1)"}, "steps": [ - {"command": "cell.set", "params": {"cell": "Input.cell(0,1)", "input": "Report"}} - ]}), - ); - let err = s.execute("macro.run", json!({"name": "Out", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap_err().to_string(); - assert!(err.contains("would move Input"), "{err}"); - // A shift that moves nothing of the macro's own still works, over the block of every written - // cell, so a row the macro writes stays together. - set(&mut s, "H1", "elsewhere"); - put( - &mut s, - json!({"name": "Wide", "inputs": [{"name": "Input", "header": true}], "steps": [ - {"command": "cell.set", "params": {"cell": "H1", "input": "over"}}, - {"command": "cell.set", "params": {"cell": "H9", "input": "far"}} - ]}), - ); - s.execute("macro.run", json!({"name": "Wide", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap(); - assert_eq!((v(&s, "H1").display(), v(&s, "H9").display(), v(&s, "H10").display()), ("over".into(), "far".into(), "elsewhere".into())); - assert_eq!(v(&s, "B3").display(), "Ann", "the input did not move"); -} - -#[test] -fn a_shift_is_refused_when_the_footprint_cannot_follow_every_write() { - // The touch log keeps at most a million writes; past that, a write that kept a cell's value - // is missing from the footprint, so a shift could split a row the macro writes. The footprint - // says so and the shift is refused (overwrite, with no dry run, still runs). - let mut s = s(); - set(&mut s, "A1", "x"); - set(&mut s, "E2", "old"); - put( - &mut s, - json!({"name": "Flood", "steps": [ - {"command": "edit.fillDown", "params": {"range": "A1:A1000001"}}, - {"command": "cell.set", "params": {"cell": "E2", "input": "new"}} - ]}), - ); - let c = s.execute("macro.check", json!({"name": "Flood"})).unwrap(); - assert_eq!(c["footprint"]["logFull"], true, "{c}"); - let err = s.execute("macro.run", json!({"name": "Flood", "onConflict": "shiftDown"})).unwrap_err().to_string(); - assert!(err.contains("more cells than the footprint can follow"), "{err}"); - assert_eq!(v(&s, "E2").display(), "old", "nothing changed"); -} - -#[test] -fn a_dry_run_leaves_a_clean_workbook_clean() { - // `view.zoom` and `sheet.activate` move the saved mark to the current workbook on a clean - // document; the dry run's rollback puts the workbook back, so the mark must come back too. - let mut s = s(); - data(&mut s); - let d = s.doc_mut().unwrap(); - d.saved = d.wb.clone(); - assert!(!s.doc().unwrap().is_dirty()); - put(&mut s, json!({"name": "Look", "steps": [{"command": "view.zoom", "params": {"percent": 120}}]})); - s.execute("macro.check", json!({"name": "Look"})).unwrap(); - assert!(!s.doc().unwrap().is_dirty(), "a dry run changed nothing"); - // A run refused by the conflict rule changed nothing either. - put(&mut s, totals()); - set(&mut s, "E3", "taken"); - let d = s.doc_mut().unwrap(); - d.saved = d.wb.clone(); - assert!(s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": "fail"})).is_err()); - assert!(!s.doc().unwrap().is_dirty()); -} - -#[test] -fn a_dry_run_puts_the_journal_back_even_after_it_trimmed_it() { - // Every nested command a dry run executes is journaled, so a long macro pushes the journal - // past its limit and the journal drops its oldest entries. Putting the journal back by its - // old length would then keep entries from the dry run and point a recording at the wrong - // place; the restore counts what was dropped. - let mut s = s(); - data(&mut s); - s.execute("selection.set", json!({"range": "A1"})).unwrap(); - let journal = s.journal.clone(); - s.recording = Some(journal.len().saturating_sub(1)); - put( - &mut s, - json!({"name": "Long", "steps": [{"type": "repeat", "over": "A1:A200", "body": [ - {"type": "repeat", "over": "A1:A60", "body": [{"command": "cell.set", "params": {"cell": "Row.cell(1,1)", "input": "x"}}]} - ]}]}), - ); - s.execute("macro.check", json!({"name": "Long"})).unwrap(); - let sets = |j: &[(String, Json)]| j.iter().filter(|(id, _)| id == "cell.set").count(); - assert!(sets(&s.journal) <= sets(&journal), "no dry-run entry stays"); - assert!(s.journal.len() <= journal.len(), "{} entries, had {}", s.journal.len(), journal.len()); - assert!(journal.ends_with(&s.journal), "what stays is the journal's own tail"); - assert!(s.recording.is_some_and(|r| r <= s.journal.len()), "{:?}", s.recording); - // A short dry run leaves the journal exactly as it was. - put(&mut s, json!({"name": "Short", "steps": [{"command": "cell.set", "params": {"cell": "H1", "input": "x"}}]})); - let (journal, recording) = (s.journal.clone(), s.recording); - s.execute("macro.check", json!({"name": "Short"})).unwrap(); - assert_eq!((&s.journal, s.recording), (&journal, recording)); + s.execute("macro.run", json!({"name": "Twice", "approve": true})).unwrap(); + assert_eq!(v(&s, "E1").display(), "b"); + let _ = std::fs::remove_dir_all(&dir); } #[test] @@ -372,66 +232,6 @@ fn the_answer_to_the_question_counts_however_ask_was_given() { assert_eq!(v(&s, "E3").display(), "400"); } -#[test] -fn the_step_limit_is_one_budget_across_the_dry_run_and_the_run() { - // 300 x 300 passes is under the limit for one phase; the dry run and the run together are not. - let mut s = s(); - put( - &mut s, - json!({"name": "Spin", "steps": [{"type": "repeat", "over": "A1:A300", "body": [ - {"type": "repeat", "over": "A1:A300", "body": [{"type": "if", "cell": "A1", "op": "empty"}]} - ]}]}), - ); - let err = s.execute("macro.run", json!({"name": "Spin"})).unwrap_err().to_string(); - assert!(err.contains("more than 100000 steps"), "{err}"); - // With no dry run (overwrite), the same macro fits. - s.execute("macro.run", json!({"name": "Spin", "onConflict": "overwrite"})).unwrap(); -} - -#[test] -fn the_footprint_covers_other_sheets_objects_and_edges() { - let mut s = s(); - data(&mut s); - s.execute("home.insertSheet", json!({"name": "Notes"})).unwrap(); - set(&mut s, "A1", "note"); - s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); - s.execute("insert.chart", json!({"range": "B2:C5", "type": "column", "at": "H2"})).unwrap(); - let chart = s.doc().unwrap().wb.sheet(0).unwrap().charts[0].id; - put( - &mut s, - json!({"name": "Wide", "output": {"onConflict": "fail"}, "steps": [ - {"command": "cell.set", "params": {"cell": "Notes!A1", "input": "x"}}, - {"command": "cell.set", "params": {"cell": "Notes!B2", "input": "y"}}, - {"command": "home.insertSheet", "params": {"name": "Extra"}}, - {"command": "sheet.activate", "params": {"sheet": "Sheet1"}}, - {"command": "insert.shape", "params": {"kind": "ellipse", "at": "J2"}}, - {"command": "object.delete", "params": {"kind": "chart", "id": chart}} - ]}), - ); - let c = s.execute("macro.check", json!({"name": "Wide", "inputs": {}})).unwrap(); - let fp = &c["footprint"]; - assert_eq!( - fp["sheets"], - json!([{"sheet": "Notes", "cells": ["A1", "B2"], "changed": 2, "conflicts": ["A1"], "conflictCount": 1, "bounds": "A1:B2"}]) - ); - assert_eq!(fp["sheetsAdded"], json!(["Extra"])); - assert_eq!((fp["objectsAdded"].as_u64(), fp["objectsRemoved"].as_u64()), (Some(1), Some(1))); - assert_eq!(c["conflicts"], 2, "the deleted chart and the note"); - assert_eq!(s.doc().unwrap().wb.sheets.len(), 2, "the dry run's new sheet went away"); - let err = s.execute("macro.run", json!({"name": "Wide"})).unwrap_err().to_string(); - assert!(err.contains("(A1 on Notes; 1 object)"), "{err}"); - - // Running past the edge of the sheet fails before anything changes. - put( - &mut s, - json!({"name": "Edge", "inputs": [{"name": "Input"}], "steps": [{"command": "cell.set", "params": {"cell": "Input.offset(-3,0)", "input": "x"}}]}), - ); - let err = s.execute("macro.run", json!({"name": "Edge", "inputs": "B2"})).unwrap_err().to_string(); - assert!(err.contains("Step 1 (cell.set): 'Input.offset(-3,0)' runs past the top of the sheet."), "{err}"); - let c = s.execute("macro.check", json!({"name": "Edge", "inputs": "B2"})).unwrap(); - assert_eq!((c["ok"].as_bool(), c["footprint"]["error"]["edge"].as_bool()), (Some(false), Some(true))); -} - #[test] fn repeat_if_and_stop() { let mut s = s(); @@ -512,7 +312,7 @@ fn macros_are_bounded() { assert!(err.contains("ran more than 100000 steps"), "{err}"); let err = s.execute("macro.run", json!({"name": "Spin", "timeLimit": 0.001})).unwrap_err().to_string(); assert!(err.contains("ran longer than"), "{err}"); - // The footprint's dry run keeps to the time limit too, and so does the check's. + // A run that checks each step keeps to the time limit too. put( &mut s, json!({"name": "Spin", "output": {"onConflict": "fail"}, "steps": [{"type": "repeat", "over": "A1:A400", "body": [ @@ -521,8 +321,6 @@ fn macros_are_bounded() { ); let err = s.execute("macro.run", json!({"name": "Spin", "timeLimit": 0.001})).unwrap_err().to_string(); assert!(err.contains("ran longer than"), "{err}"); - let c = s.execute("macro.check", json!({"name": "Spin", "timeLimit": 0.001})).unwrap(); - assert!(c["errors"][0].as_str().unwrap().contains("ran longer than"), "{c}"); } #[test] @@ -535,33 +333,22 @@ fn a_token_in_a_path_needs_the_permission_and_approval() { for path in [json!({"$ref": "Input"}), json!(in_dir), json!(" ")] { let step = json!({"command": "file.exportCsv", "params": {"path": path}}); put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step]})); - // Without the permission nothing runs, not even the check's dry run. + // Without the permission nothing runs. let err = s.execute("macro.run", json!({"name": "Export"})).unwrap_err().to_string(); assert!(err.contains("Step 1 (file.exportCsv) writes a file. Add file.exportCsv to the macro's permissions"), "{path}: {err}"); - let c = s.execute("macro.check", json!({"name": "Export"})).unwrap(); - assert!(c["errors"][0].as_str().unwrap().contains("writes a file"), "{path}: {c}"); - assert!(c.get("footprint").is_none()); - // With it, the person approves first; the check doesn't run it before then. + // With it, the person approves first. put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step], "permissions": ["file.exportCsv"]})); let r = s.execute("macro.run", json!({"name": "Export"})); let Err(EngineError::Approve(q)) = &r else { panic!("{path}: {r:?}") }; assert!(q.contains("file.exportCsv, which writes a file"), "{q}"); - let c = s.execute("macro.check", json!({"name": "Export"})).unwrap(); - assert_eq!(c["needsApproval"], json!(["runs file.exportCsv, which writes a file"]), "{path}"); - assert!(c.get("footprint").is_none()); assert_eq!(files(&dir), 0, "{path}"); assert!(!std::path::Path::new("A1.csv").exists() && !std::path::Path::new(" .csv").exists()); } - // Once approved, the check's dry run stops before the step, and only the run writes the file. + // Once approved, the run writes the file. let step = json!({"command": "file.exportCsv", "params": {"path": in_dir}}); put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step], "permissions": ["file.exportCsv"]})); s.execute("macro.run", json!({"name": "Export", "approve": true, "inputs": "B2"})).unwrap(); assert_eq!(files(&dir), 1); - std::fs::remove_dir_all(&dir).unwrap(); - std::fs::create_dir_all(&dir).unwrap(); - let c = s.execute("macro.check", json!({"name": "Export", "inputs": "B2"})).unwrap(); - assert_eq!(c["footprint"]["partial"]["step"], "1", "{c}"); - assert_eq!(files(&dir), 0, "the check writes nothing"); let _ = std::fs::remove_dir_all(&dir); } @@ -604,16 +391,10 @@ fn the_policy_keeps_macros_away_from_files_and_the_app() { let Err(EngineError::Approve(q)) = &r else { panic!("{r:?}") }; assert_eq!(q, "Run macro 'Import'? It runs data.fromTextCsv, which reads a file."); assert_eq!(s.doc().unwrap().wb.sheets.len(), 1); - let c = s.execute("macro.check", json!({"name": "Import"})).unwrap(); - assert_eq!(c["needsApproval"], json!(["runs data.fromTextCsv, which reads a file"])); - assert!(c.get("footprint").is_none(), "nothing unapproved runs, even dry"); s.execute("macro.run", json!({"name": "Import", "approve": true})).unwrap(); assert_eq!(s.doc().unwrap().wb.sheets.len(), 2); s.execute("edit.undo", json!({})).unwrap(); s.execute("macro.run", json!({"name": "Import"})).unwrap(); - // The dry run skips the file step and says so. - let c = s.execute("macro.check", json!({"name": "Import"})).unwrap(); - assert_eq!(c["footprint"]["partial"]["step"], "1"); // Changing the macro asks again. put(&mut s, json!({"name": "Import", "description": "changed", "steps": [step], "permissions": ["data.fromTextCsv"]})); assert!(matches!(s.execute("macro.run", json!({"name": "Import"})), Err(EngineError::Approve(_)))); @@ -658,63 +439,3 @@ fn macros_from_a_file_ask_before_running() { let b = crate::macros::parse_macro(&json!({"steps": [{"params": {"input": "x", "cell": "A1"}, "command": "cell.set"}], "name": "M"})).unwrap(); assert_eq!(crate::macros::fingerprint(&a), crate::macros::fingerprint(&b)); } - -#[test] -fn a_partial_footprint_doesnt_clear_later_writes() { - let dir = std::env::temp_dir().join(format!("gc-macro-partial-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); - let csv = dir.join("out.csv").to_string_lossy().to_string(); - let export = json!({"command": "file.exportCsv", "params": {"path": csv}}); - let solver = json!({"command": "data.solver", "params": {"objective": "C1", "variables": "A1", "constraints": ["A1 <= 5"], "method": "simplex", "saveModel": false}}); - let late = json!({"command": "cell.set", "params": {"cell": "B1", "input": "over"}}); - // The dry run stops at an approved file step or at Solver; the step after it writes over B1. - for first in [&export, &solver] { - let mut s = super::s(); - set(&mut s, "A1", "1"); - set(&mut s, "C1", "=A1"); - set(&mut s, "B1", "keep"); - put(&mut s, json!({"name": "Late", "inputs": [{"name": "Input"}], "permissions": ["file.exportCsv"], "steps": [first, late]})); - let start = s.doc().unwrap().wb.clone(); - let c = s.execute("macro.check", json!({"name": "Late", "inputs": "A1"})).unwrap(); - if c.get("needsApproval").is_none() { - assert_eq!((c["conflicts"].as_u64(), &c["footprint"]["partial"]["step"]), (Some(0), &json!("1")), "{c}"); - } - for rule in ["fail", "ask"] { - let r = s.execute("macro.run", json!({"name": "Late", "inputs": "A1", "onConflict": rule, "approve": true})); - let e = match (rule, r) { - ("fail", Err(EngineError::Other(e))) | ("ask", Err(EngineError::Confirm(e))) => e, - (_, r) => panic!("{first} {rule}: {r:?}"), - }; - assert!(e.contains("Step 2 (cell.set): It would write over data (B1 on Sheet1)"), "{e}"); - assert_eq!(v(&s, "B1").display(), "keep", "{first} {rule}"); - assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{first} {rule}: rolled back"); - } - // The answer to the question overwrites. - s.execute("macro.run", json!({"name": "Late", "inputs": "A1", "overwrite": true})).unwrap(); - assert_eq!(v(&s, "B1").display(), "over"); - } - // A step that writes over what the macro itself wrote isn't a conflict. - let mut s = super::s(); - let again = |input: &str| json!({"command": "cell.set", "params": {"cell": "E1", "input": input}}); - put( - &mut s, - json!({"name": "Twice", "permissions": ["file.exportCsv"], "output": {"onConflict": "fail"}, "steps": [again("a"), export, again("b")]}), - ); - s.execute("macro.run", json!({"name": "Twice", "approve": true})).unwrap(); - assert_eq!(v(&s, "E1").display(), "b"); - // After a shift, the steps past the file step are checked too. - let mut s = super::s(); - set(&mut s, "C1", "old"); - set(&mut s, "D1", "keep"); - let step = |cell: &str| json!({"command": "cell.set", "params": {"cell": cell, "input": "new"}}); - put( - &mut s, - json!({"name": "Shift", "permissions": ["file.exportCsv"], "output": {"onConflict": "shiftDown"}, "steps": [step("C1"), export, step("D1")]}), - ); - let start = s.doc().unwrap().wb.clone(); - let err = s.execute("macro.run", json!({"name": "Shift", "approve": true})).unwrap_err().to_string(); - assert!(err.contains("Step 3 (cell.set): It would write over data (D1 on Sheet1)"), "{err}"); - assert_eq!((v(&s, "C1").display(), v(&s, "D1").display()), ("old".into(), "keep".into())); - assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "the shift is rolled back too"); - let _ = std::fs::remove_dir_all(&dir); -} diff --git a/docs/macros.md b/docs/macros.md index 4b653ff7a..daf0a2c35 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -1,16 +1,17 @@ # Macros -> **Last updated:** 2026-10-24 · **Change:** minor (running and checking macros: `macro.run`, `macro.check`) +> **Last updated:** 2026-10-25 · **Change:** minor (running macros: `macro.run`) GridCraft macros are typed, declarative lists of commands, not VBA. Every step runs a GridCraft command (the same ids the ribbon, CLI and MCP use) or one of three bounded control steps. A macro declares the inputs it works on, runs as one undo step, rolls all workbook changes back on an -error (a file it already wrote stays), checks where its results will land before it writes them, -and does not touch files without permission. +error (a file it already wrote stays), stops before it writes over data it may not, and does not +touch files without permission. Agents run macros with `macro.run` like any other command. -This page describes the macro format, its tokens, and running and checking macros. The engine has -no macro debugger, recorder or management commands in this version, and no macro interface. +This page describes the macro format, its tokens, and running macros. The engine has no check of a +macro before it runs, no macro debugger, recorder or management commands in this version, and no +macro interface. ## A macro @@ -59,9 +60,8 @@ no macro debugger, recorder or management commands in this version, and no macro whole macro (4,194,304 characters of JSON). A macro read from a file that breaks any of these is left out with a warning; it is never changed to fit. When a macro runs, a Repeat whose range has more rows or columns than its `max` is refused, and a run executes at most 100,000 steps (each - pass through a Repeat counts) and takes at most 60 seconds, both shared by the footprint dry - runs and the run itself (`timeLimit` to change the time; the web build has no clock, so there - only the step limit applies). + pass through a Repeat counts) and takes at most 60 seconds (`timeLimit` to change the time; + the web build has no clock, so there only the step limit applies). ## Tokens @@ -108,24 +108,17 @@ refused rather than read as the header. are when the step runs. A macro that touches files, or that came with a workbook opened from a file, runs once the person approves it: the call fails with `approve: …` and runs with `approve: true`. Approval lasts for the session, until the macro changes. -2. **Footprint.** Unless the conflict rule is `overwrite`, the macro first runs in dry-run mode on - the real workbook, and everything is put back. Cells it would write outside its - inputs that hold data, and sheets and objects it would - delete, are conflicts, handled by `onConflict` (or the macro's `output.onConflict`): `ask` (the - call fails with `confirm: …`; `overwrite: true` answers it), `fail`, `overwrite`, `shiftDown` or - `shiftRight` (Insert Cells makes room, then the footprint is checked again). Steps that touch - files, and Solver, end the dry run early; the footprint then says it is partial, and unless - the rule is `overwrite`, the run checks every step as it goes. A step that writes over cells, - sheets or objects that held data when the run started stops the run and rolls the workbook - back (a file already written stays): it asks (`confirm: …`) under `ask` and fails otherwise, - since shifting can't make room that late. +2. **Conflicts.** Unless the conflict rule is `overwrite`, each step is checked as it runs. A + step that writes over cells outside the inputs that held data when the run started, or deletes + sheets or objects that were there, stops the run, which rolls the workbook back (a file already + written stays). The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call + fails with `confirm: …`; `overwrite: true` answers it), `fail` or `overwrite`. Shifting the + data to make room (`shiftDown`, `shiftRight`) needs to know where the macro writes before it + runs, which this version doesn't work out, so those rules stop the run as `fail` does. 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside an If). -`macro.check {name, inputs?, timeLimit?}` reports the bound inputs, the output anchor, any -approval needed and the footprint, without changing anything. - ## Storage Macros are saved in the workbook: as a field in GridCraft's JSON format, and in xlsx as hidden From 0719458114b646e1d3d978e4e00a5ea8918f159f Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:33:26 -0400 Subject: [PATCH 2/7] Engine: macro.check and the footprint: where a macro writes, worked out before it runs Split from the pull request below at the review's request; the code is the runner's footprint as reviewed there, unchanged. - `macro.run` first runs the macro in dry-run mode on the real workbook (unless the rule is `overwrite`) and puts everything back: the workbook, its selection and history through the dry run's undo group, and the session state a dry run may touch (`Saved`: the active workbook, open documents, the saved mark, selections, unlocked ranges, trust, clipboard, preferences, mode, journal and recording, queued requests, format painter, find, ruler, hidden windows, drawing tool and the comment panes). - Cells it would write outside its inputs that hold data, and sheets and objects it would delete, are conflicts: `ask` asks before anything runs, `fail` fails, and `shiftDown` and `shiftRight` insert cells over the block of every written cell and check again. A shift is refused when it would move an input, when the macro deletes sheets or objects, or when the touch log is full. - A dry run stops before a file step and before Solver; the footprint is then partial, and the run checks each step as it goes. - The dry runs and the run share one clock, one time limit and one step budget. - `macro.check` reports the inputs, the output anchor, approval needed and the footprint. - Tests: the footprint tests that moved with this code. --- crates/engine/src/cmd/macros.rs | 34 ++- crates/engine/src/cmd/mod.rs | 12 + crates/engine/src/lib.rs | 23 ++ crates/engine/src/macros/policy.rs | 7 + crates/engine/src/macros/run.rs | 474 +++++++++++++++++++++++++++-- crates/engine/src/tests/macros.rs | 349 ++++++++++++++++++++- docs/macros.md | 37 ++- 7 files changed, 869 insertions(+), 67 deletions(-) diff --git a/crates/engine/src/cmd/macros.rs b/crates/engine/src/cmd/macros.rs index d81902b1b..53856e720 100644 --- a/crates/engine/src/cmd/macros.rs +++ b/crates/engine/src/cmd/macros.rs @@ -6,15 +6,26 @@ use serde_json::Value as Json; use super::*; pub fn specs() -> Vec { - vec![cmd!( - "macro.run", - "Run Macro", - [], - None, - "{name, inputs?: {Input: \"B2:D9\"} | \"B2:D9\", onConflict?: ask|fail|overwrite|shiftDown|shiftRight, overwrite?: bool, approve?: bool, timeLimit?: seconds} runs as one undo step; any error rolls it all back → {macro, ran, inputs, stopped?, shifted?}", - has_doc, - run - )] + vec![ + cmd!( + "macro.run", + "Run Macro", + [], + None, + "{name, inputs?: {Input: \"B2:D9\"} | \"B2:D9\", onConflict?: ask|fail|overwrite|shiftDown|shiftRight, overwrite?: bool, approve?: bool, timeLimit?: seconds} runs as one undo step; any error rolls it all back → {macro, ran, inputs, stopped?, shifted?}", + has_doc, + run + ), + cmd!( + query "macro.check", + "Check Macro", + [], + None, + "{name, inputs?, timeLimit?: seconds} validates the inputs and dry-runs the macro on the workbook (then puts everything back) → {ok, errors?, needsApproval?, inputs, output?, conflicts, footprint}", + has_doc, + check + ), + ] } fn name_param<'a>(cmd: &str, p: &'a Json) -> Result<&'a str> { @@ -25,3 +36,8 @@ fn run(s: &mut Session, p: &Json) -> Result { let name = name_param("macro.run", p)?; crate::macros::run::run(s, name, p) } + +fn check(s: &mut Session, p: &Json) -> Result { + let name = name_param("macro.check", p)?; + crate::macros::run::check(s, name, p) +} diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 6f58f18ee..90fe4ccc8 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -366,6 +366,13 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) if !structural { check_arrays(&d.wb, &wb)?; } + if let Some(log) = d.touched.as_mut() { + for (si, c) in changed.iter().take(TOUCH_LOG_LIMIT.saturating_sub(log.len())) { + if let Some(sh) = wb.sheet(*si) { + log.push((sh.name.clone(), *c)); + } + } + } if changed.len() <= 200_000 { fit_rows.extend(changed.iter().map(|(s, c)| (*s, c.row))); } @@ -393,6 +400,11 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) Ok(r) } +/// The most written cells a macro's footprint check records. +/// The most cells a dry run's touch log keeps; a footprint whose log is full may be missing writes +/// that kept a cell's value, so a shift is refused then. +pub(crate) const TOUCH_LOG_LIMIT: usize = 1_000_000; + /// Excel's message for an edit of a locked cell on a protected sheet. pub(crate) const PROTECTED: &str = "The cell or chart you're trying to change is on a protected sheet."; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 151c2bc06..9d4d2f93b 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -206,6 +206,9 @@ pub struct DocState { /// workbook is opened from a file, carried by content pasted from such a workbook, and never /// changed by a trust decision. pub trust: Trust, + /// While a macro's footprint is measured: every cell a command wrote, by sheet name, even + /// where it wrote what was already there. + pub touched: Option>, /// The references `edit.goTo` with `record: true` went to in this document, newest first, /// without names, each area written with its sheet (`Sheet1!B14:C22,Sheet1!F19:G30`). pub go_to_recent: Vec, @@ -299,6 +302,7 @@ impl DocState { content_kind: None, scripts_trusted, trust, + touched: None, go_to_recent: vec![], } } @@ -434,6 +438,9 @@ pub struct Session { /// Serialized size of each journal entry, measured once when it is pushed, and their sum. journal_sizes: Vec, journal_bytes: usize, + /// Entries the journal has dropped from its front to stay within its limits, ever; a dry run + /// that puts the journal back needs to know how many went while it ran. + journal_dropped: u64, pub ui_requests: Vec, /// Format painter source style (sticky when double-clicked). pub format_painter: Option<(Arc, usize, RangeRef, bool)>, @@ -558,10 +565,26 @@ impl Session { self.journal.drain(..n); self.journal_sizes.drain(..n); self.journal_bytes = kept; + self.journal_dropped = self.journal_dropped.saturating_add(n as u64); self.recording = self.recording.map(|r| r.saturating_sub(n)); } } + /// Entries dropped from the journal's front so far, for a caller that puts the journal back. + pub(crate) fn journal_dropped(&self) -> u64 { + self.journal_dropped + } + + /// Cuts the journal back to its first `keep` entries, with its size bookkeeping. + pub(crate) fn truncate_journal(&mut self, keep: usize) { + if self.journal_sizes.len() == self.journal.len() { + self.journal_sizes.truncate(keep); + self.journal_bytes = self.journal_sizes.iter().sum(); + } + self.journal.truncate(keep); + self.recording = self.recording.map(|r| r.min(self.journal.len())); + } + pub fn commands(&self) -> Vec { command_specs().iter().map(|c| c.info(self)).collect() } diff --git a/crates/engine/src/macros/policy.rs b/crates/engine/src/macros/policy.rs index 57d974920..8f23a5b40 100644 --- a/crates/engine/src/macros/policy.rs +++ b/crates/engine/src/macros/policy.rs @@ -73,6 +73,12 @@ pub fn macro_policy(id: &str, params: &Json, trust: Trust) -> Policy { } } +/// Whether the footprint dry run can preview a command. Solver can take long, so a dry run stops +/// before it and reports the footprint as partial. +pub fn previewable(id: &str) -> bool { + id != "data.solver" +} + #[cfg(test)] mod tests { use super::*; @@ -123,6 +129,7 @@ mod tests { assert_eq!(macro_policy(id, &none, Trust::Local), Policy::Allow, "{id}"); assert!(matches!(macro_policy(id, &none, Trust::FromFile), Policy::Ask(_)), "{id}"); } + assert!(!previewable("data.solver") && previewable("cell.set")); } #[test] diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 8ae1e3496..f45bcfc25 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -6,12 +6,12 @@ //! approves it (once per macro content, for the session). //! 2. **Inputs**: each declared input is bound to a range (given, its default, or the selection) //! and checked against its shape and column types, with messages that name the cells. -//! 3. **Conflicts**: unless the macro may overwrite, each step is checked as it runs. A step that -//! writes over data outside the inputs (cells that held data when the run started, sheets and -//! objects it deletes) stops the run, which asks (`ask`) or fails (the other rules), and the -//! run rolls back. Shifting the data to make room (`shiftDown`, `shiftRight`) needs to know -//! where the macro writes before it runs, which this version doesn't work out, so those rules -//! stop the run as `fail` does. +//! 3. **Footprint**: unless the macro may overwrite, it first runs on the real workbook in dry-run +//! mode, the changes are compared with `CellStore::diff`, and everything is put back. Cells it +//! would write outside its inputs that already hold data are conflicts, handled by the output +//! rule: ask, fail, overwrite, or shift the data down or right (and check again). A dry run +//! that stops early (a file step, Solver) proves nothing about later steps, so the run then +//! checks each step as it goes. //! 4. **Run**: one undo step (`Session::grouped`); an error at any step rolls everything back and //! names the step. //! @@ -26,9 +26,9 @@ use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, S use serde::Serialize; use serde_json::{Value as Json, json}; -use super::policy::{Policy, macro_policy}; +use super::policy::{Policy, macro_policy, previewable}; use super::resolve::{self, Env, Place, ResolveError}; -use crate::{EngineError, Result, Session, Trust, UiRequest}; +use crate::{EngineError, Result, Selection, Session, Trust, UiRequest}; /// The most steps one run executes (counting each pass through a Repeat). pub const MAX_EXECUTED_STEPS: u64 = 100_000; @@ -37,7 +37,7 @@ pub const MAX_EXECUTED_STEPS: u64 = 100_000; pub const DEFAULT_TIME_LIMIT_MS: f64 = 60_000.0; /// The most cells an input may have for its content to be checked. pub const MAX_INPUT_CELLS: u64 = 1_000_000; -/// The most ranges a list of cells gives per sheet (the counts stay exact). +/// The most ranges a footprint lists per sheet (the counts stay exact). const MAX_LISTED_RANGES: usize = 500; /// An input bound to a range: name, where, whether its first row is a header. @@ -421,28 +421,37 @@ pub struct Machine { pub executed: u64, clock: Stopwatch, pub time_limit_ms: f64, + /// Dry run (the footprint check): steps that would touch files or can't be previewed stop + /// the run early instead of running. + pub dry_run: bool, /// A Stop step ended the run: its message, and whether it asked for a rollback. pub stopped: Option<(String, bool)>, + /// A dry run stopped before this step (path, reason): what follows isn't in the footprint. + pub partial: Option<(String, String)>, /// The last step's resolved parameters (for the debugger). pub last_params: Option, - /// Set unless the macro may overwrite: each step is checked for writes over data as it runs. + /// Set when the footprint was partial: each step is checked for writes over data as it runs. guard: Option, } -/// Checks each step of a run for writes over data the workbook held when the run started. +/// Checks each step of a real run for writes over data the footprint couldn't see. #[derive(Clone, Debug)] struct Guard { /// The workbook when the run started, and for each sheet now, its index there (`None` for a /// sheet the run added). start: Arc, origin: Vec>, + /// Where the footprint stopped: "step 2 (file.exportCsv writes a file)". + partial: String, /// The conflict rule is ask. ask: bool, } impl Guard { - fn new(start: Arc, ask: bool) -> Guard { - Guard { origin: (0..start.sheets.len()).map(Some).collect(), start, ask } + fn new(start: Arc, partial: &Json, ask: bool) -> Guard { + let text = |k: &str| partial.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); + let partial = format!("step {} ({})", text("step"), text("reason")); + Guard { origin: (0..start.sheets.len()).map(Some).collect(), start, partial, ask } } /// What one step wrote over that held data when the run started: cells outside the inputs, @@ -503,7 +512,7 @@ fn deleted(start: &[T], prev: &[T], after: &[T], id: impl Fn(&T) -> u32) -> u } impl Machine { - pub fn new(p: &Prepared) -> Machine { + pub fn new(p: &Prepared, dry_run: bool) -> Machine { let mut m = Machine { name: p.mac.name.clone(), doc: p.doc, @@ -515,7 +524,9 @@ impl Machine { executed: 0, clock: Stopwatch::start(), time_limit_ms: DEFAULT_TIME_LIMIT_MS, + dry_run, stopped: None, + partial: None, last_params: None, guard: None, }; @@ -666,9 +677,19 @@ impl Machine { if !matches!(written, Policy::NeedsPermission(_)) { return Err(fail(format!("{id} {what} only once its parameters are filled in, so it wasn't approved."))); } + if self.dry_run { + self.partial = Some((path.to_string(), format!("{id} {what}"))); + self.frames.clear(); + return Ok(()); + } } (_, Policy::Allow | Policy::Ask(_)) => {} } + if self.dry_run && !previewable(id) { + self.partial = Some((path.to_string(), format!("{id} can't be previewed"))); + self.frames.clear(); + return Ok(()); + } self.last_params = Some(resolved.clone()); let prev = self.guard.as_ref().and_then(|_| s.docs.iter().find(|d| d.uid == self.doc)).map(|d| d.wb.clone()); let queued = s.ui_requests.len(); @@ -693,7 +714,7 @@ impl Machine { let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { - let msg = format!("It would write over data ({what})."); + let msg = format!("It would write over data ({what}); the check before the run stopped at {} and couldn't see this.", g.partial); return Err(StepError { confirm: g.ask, ..fail(msg) }); } } @@ -743,7 +764,255 @@ fn compare(v: &Value, op: CompareOp, want: &Json) -> bool { } } -// ---------------------------------------------------------------- comparing workbooks +// ---------------------------------------------------------------- footprint + +/// What a macro would change, from a dry run on the real workbook. +#[derive(Clone, Debug, Default, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct Footprint { + /// Steps executed by the end of the dry run that made this footprint (the run's one budget). + #[serde(skip)] + pub executed: u64, + /// The dry run wrote more cells than the touch log keeps, so writes that kept a cell's value + /// may be missing from `cells`: a shift cannot trust the block. + #[serde(skip_serializing_if = "std::ops::Not::not")] + pub log_full: bool, + pub sheets: Vec, + pub sheets_added: Vec, + pub sheets_removed: Vec, + pub objects_added: usize, + pub objects_changed: usize, + pub objects_removed: usize, + /// The dry run stopped early before this step; later steps aren't in the footprint. + #[serde(skip_serializing_if = "Option::is_none")] + pub partial: Option, + /// The step the run would fail at. + #[serde(skip_serializing_if = "Option::is_none")] + pub error: Option, + /// A Stop step's message. + #[serde(skip_serializing_if = "Option::is_none")] + pub stopped: Option, +} + +/// The cells a macro would write on one sheet, outside its inputs. +#[derive(Clone, Debug, Default, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct SheetFootprint { + pub sheet: String, + #[serde(skip)] + pub index: usize, + /// The written cells as ranges (at most 500 listed), and how many cells. + pub cells: Vec, + pub changed: usize, + /// Written cells that hold data now. + pub conflicts: Vec, + pub conflict_count: usize, + /// The block that holds every written cell. + #[serde(skip_serializing_if = "Option::is_none")] + pub bounds: Option, + #[serde(skip)] + pub bounds_range: Option, +} + +impl Footprint { + /// Cells that hold data and would be written, plus sheets and objects that would go. + pub fn conflict_count(&self) -> usize { + self.sheets.iter().map(|s| s.conflict_count).sum::() + self.sheets_removed.len() + self.objects_removed + } + + /// "C2:C9 on Sheet1, the sheet Old, 2 objects". + pub fn describe_conflicts(&self) -> String { + let mut parts: Vec = self + .sheets + .iter() + .filter(|s| s.conflict_count > 0) + .map(|s| { + let mut listed = s.conflicts.iter().take(3).cloned().collect::>().join(", "); + if s.conflicts.len() > 3 { + listed.push_str(", …"); + } + format!("{listed} on {}", s.sheet) + }) + .collect(); + parts.extend(self.sheets_removed.iter().map(|n| format!("the sheet {n}"))); + if self.objects_removed > 0 { + parts.push(format!("{} object{}", self.objects_removed, if self.objects_removed == 1 { "" } else { "s" })); + } + parts.join("; ") + } +} + +/// Session state a dry run may touch, put back afterwards. The workbook, its selection and its +/// history are put back by the dry run's undo group. +struct Saved { + active: usize, + docs: Vec, + /// The workbook as last saved: `sheet.activate` and the view commands move it to the + /// current workbook on a clean document, and the rollback puts the workbook back but not this. + saved: Option>, + sheet_selections: Vec, + unlocked_ranges: Vec<(String, String)>, + trust: Trust, + clipboard: Option, + prefs: crate::Prefs, + mode: crate::Mode, + /// The journal's length and how many entries it had dropped from its front; a dry run that + /// pushes past the journal's limits drops more, which a length alone cannot see. + journal: usize, + journal_dropped: u64, + recording: Option, + ui_requests: usize, + format_painter: Option<(Arc, usize, RangeRef, bool)>, + last_find: Option, + hide_ruler: bool, + hidden_windows: Vec, + draw: (String, String, f32), + comments_pane: bool, + show_all_notes: bool, +} + +impl Saved { + fn take(s: &mut Session, doc: u64) -> Saved { + let d = s.docs.iter().find(|d| d.uid == doc); + Saved { + active: s.active, + docs: s.docs.iter().map(|d| d.uid).collect(), + saved: d.map(|d| d.saved.clone()), + sheet_selections: d.map(|d| d.sheet_selections.clone()).unwrap_or_default(), + unlocked_ranges: d.map(|d| d.unlocked_ranges.clone()).unwrap_or_default(), + trust: d.map_or(Trust::Local, |d| d.trust), + clipboard: s.clipboard.clone(), + prefs: s.prefs.clone(), + mode: s.mode, + journal: s.journal.len(), + journal_dropped: s.journal_dropped(), + recording: s.recording, + ui_requests: s.ui_requests.len(), + format_painter: s.format_painter.clone(), + last_find: s.last_find.clone(), + hide_ruler: s.hide_ruler, + hidden_windows: s.hidden_windows.clone(), + draw: (s.draw_tool.clone(), s.draw_color.clone(), s.draw_width), + comments_pane: s.comments_pane, + show_all_notes: s.show_all_notes, + } + } + + fn restore(self, s: &mut Session, doc: u64) { + s.docs.retain(|d| self.docs.contains(&d.uid)); + if let Some(d) = s.docs.iter_mut().find(|d| d.uid == doc) { + if let Some(saved) = self.saved { + d.saved = saved; + } + d.sheet_selections = self.sheet_selections; + d.unlocked_ranges = self.unlocked_ranges; + d.trust = self.trust; + } + s.active = self.active.min(s.docs.len().saturating_sub(1)); + s.clipboard = self.clipboard; + s.prefs = self.prefs; + s.mode = self.mode; + // Entries the dry run pushed go; entries the journal dropped meanwhile are gone for good, + // and the recording's start moves back with them. + let dropped = usize::try_from(s.journal_dropped().saturating_sub(self.journal_dropped)).unwrap_or(usize::MAX); + s.truncate_journal(self.journal.saturating_sub(dropped)); + s.recording = self.recording.map(|r| r.saturating_sub(dropped)); + s.ui_requests.truncate(self.ui_requests); + s.format_painter = self.format_painter; + s.last_find = self.last_find; + s.hide_ruler = self.hide_ruler; + s.hidden_windows = self.hidden_windows; + (s.draw_tool, s.draw_color, s.draw_width) = self.draw; + s.comments_pane = self.comments_pane; + s.show_all_notes = self.show_all_notes; + } +} + +/// Runs the macro in dry-run mode on its workbook, puts everything back, and says what it would +/// change. The dry run stops once `time_limit_ms` has passed on `clock`. +pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: f64, executed: u64) -> Result { + let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?; + s.set_active(i); + let saved = Saved::take(s, p.doc); + let before = s.doc()?.wb.clone(); + let token = s.group_begin("Check Macro")?; + if let Some(d) = s.docs.iter_mut().find(|d| d.uid == p.doc) { + d.touched = Some(Vec::new()); + } + let mut m = Machine::new(p, true); + (m.clock, m.time_limit_ms, m.executed) = (clock, time_limit_ms, executed); + let outcome = m.run_to_end(s); + let (after, touched) = match s.docs.iter_mut().find(|d| d.uid == p.doc) { + Some(d) => (d.wb.clone(), d.touched.take().unwrap_or_default()), + None => (before.clone(), Vec::new()), + }; + let rolled = s.group_rollback(token); + saved.restore(s, p.doc); + rolled?; + let (after, touched) = if matches!(m.stopped, Some((_, true))) { (before.clone(), Vec::new()) } else { (after, touched) }; + let mut fp = diff(&before, &after, &p.inputs, &touched); + fp.log_full = touched.len() >= crate::cmd::TOUCH_LOG_LIMIT; + fp.executed = m.executed; + fp.error = outcome.err(); + fp.stopped = m.stopped.map(|(msg, _)| msg); + fp.partial = m.partial.map(|(step, reason)| json!({"step": step, "reason": reason})); + Ok(fp) +} + +/// Compares the workbook before and after a dry run. `touched` adds the cells commands wrote +/// without changing them (a formula written again), so a shift moves everything the macro writes. +fn diff(before: &Workbook, after: &Workbook, inputs: &[Bound], touched: &[(String, CellRef)]) -> Footprint { + let mut fp = Footprint::default(); + let (pairs, added) = pair_sheets(before, after); + fp.sheets_added = added.into_iter().filter_map(|a| after.sheet(a).map(|s| s.name.clone())).collect(); + for (bi, ai) in pairs { + let Some(bs) = before.sheets.get(bi) else { continue }; + let Some(ai) = ai else { + fp.sheets_removed.push(bs.name.clone()); + continue; + }; + let Some(after_sheet) = after.sheets.get(ai) else { continue }; + let rewritten: BTreeSet = touched.iter().filter(|(n, _)| *n == after_sheet.name).map(|(_, c)| *c).collect(); + if Arc::ptr_eq(bs, after_sheet) && rewritten.is_empty() { + continue; + } + let (added, changed, removed) = objects(bs, after_sheet); + fp.objects_added += added; + fp.objects_changed += changed; + fp.objects_removed += removed; + let in_input = |c: CellRef| inputs.iter().any(|(_, p, _)| p.sheet == bi && p.range.contains(c)); + let mut written = Vec::new(); + let mut conflicts = Vec::new(); + let mut cells: BTreeSet = bs.cells.diff(&after_sheet.cells).into_iter().collect(); + cells.extend(rewritten.iter().copied()); + for c in cells { + if in_input(c) { + continue; + } + let (b, a) = (bs.cell(c), after_sheet.cell(c)); + if recalculated_only(b, a) && !rewritten.contains(&c) { + continue; + } + written.push(c); + if b.is_some_and(has_content) && content_differs(b, a) { + conflicts.push(c); + } + } + let Some(first) = written.first() else { continue }; + let bounds = written.iter().fold(RangeRef::cell(*first), |r, c| r.union(&RangeRef::cell(*c))); + fp.sheets.push(SheetFootprint { + sheet: bs.name.clone(), + index: bi, + changed: written.len(), + conflict_count: conflicts.len(), + cells: ranges(&mut written), + conflicts: ranges(&mut conflicts), + bounds: Some(bounds.a1()), + bounds_range: Some(bounds), + }); + } + fp +} /// Each sheet of `before` with its index in `after` (`None` when deleted), and the sheets of /// `after` that are new. Sheets are matched by name; what's left is matched in order (a renamed @@ -785,6 +1054,18 @@ fn content_differs(b: Option<&Cell>, a: Option<&Cell>) -> bool { } } +/// Objects added, changed and removed between two versions of a sheet. +fn objects(b: &Sheet, a: &Sheet) -> (usize, usize, usize) { + fn count(b: &[T], a: &[T], id: impl Fn(&T) -> u32) -> (usize, usize, usize) { + let added = a.iter().filter(|x| !b.iter().any(|y| id(y) == id(x))).count(); + let removed = b.iter().filter(|x| !a.iter().any(|y| id(y) == id(x))).count(); + let changed = b.iter().filter(|x| a.iter().any(|y| id(y) == id(x) && y != *x)).count(); + (added, changed, removed) + } + let parts = [count(&b.charts, &a.charts, |o| o.id), count(&b.images, &a.images, |o| o.id), count(&b.shapes, &a.shapes, |o| o.id)]; + parts.iter().fold((0, 0, 0), |t, p| (t.0 + p.0, t.1 + p.1, t.2 + p.2)) +} + /// Cells as A1 ranges: runs along each row, then runs with the same columns on following rows. pub(crate) fn ranges(cells: &mut [CellRef]) -> Vec { cells.sort_unstable_by_key(|c| (c.row, c.col)); @@ -811,6 +1092,47 @@ pub(crate) fn ranges(cells: &mut [CellRef]) -> Vec { merged.iter().take(MAX_LISTED_RANGES).map(RangeRef::a1).collect() } +/// Inserts blank cells where the footprint lands (moving what is there down or right), so the +/// macro writes into empty cells. The block is every written cell's, so a row the macro writes +/// stays together. A shift that would move one of the macro's inputs is refused: the inputs were +/// checked and bound where they are, and only the block's columns (or rows) would move. +fn make_room(s: &mut Session, fp: &Footprint, p: &Prepared, down: bool) -> Result<()> { + if !fp.sheets_removed.is_empty() || fp.objects_removed > 0 { + return Err(EngineError::Other("Shifting can't make room: the macro deletes sheets or objects.".into())); + } + if fp.log_full { + return Err(EngineError::Other("Shifting can't make room: the macro writes more cells than the footprint can follow.".into())); + } + let wb = s.doc()?.wb.clone(); + let env = Env { wb: &wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + let mut moves = Vec::new(); + for sh in fp.sheets.iter().filter(|s| s.conflict_count > 0) { + let Some(block) = sh.bounds_range else { continue }; + // What the insertion moves: the block's columns from its top row down, or its rows from + // its left column rightwards. + let moved = |r: &RangeRef| { + if down { + r.start.col <= block.end.col && r.end.col >= block.start.col && r.end.row >= block.start.row + } else { + r.start.row <= block.end.row && r.end.row >= block.start.row && r.end.col >= block.start.col + } + }; + // The output is where the macro writes, so moving what is there is the point; the second + // footprint catches data the shift moved into it. + if let Some((name, pl)) = p.inputs.iter().map(|(n, pl, _)| (n.as_str(), *pl)).find(|(_, pl)| pl.sheet == sh.index && moved(&pl.range)) { + return Err(EngineError::Other(format!( + "Shifting the data would move {name} ({}). Run with onConflict overwrite or fail, or make room first.", + resolve::a1(pl, &env) + ))); + } + moves.push(resolve::a1(Place { sheet: sh.index, range: block }, &env)); + } + for range in moves { + s.execute("home.insertCells", json!({"range": range, "shift": if down { "down" } else { "right" }}))?; + } + Ok(()) +} + // ---------------------------------------------------------------- run /// A macro that passed its checks and is about to run. @@ -840,19 +1162,62 @@ pub fn begin(s: &mut Session, name: &str, p: &Json) -> Result { Ok(Begun { mac: m, prepared, rule, time_limit_ms, inputs }) } -/// The output rule, inside the run's undo group: the machine for the run. Unless the macro may -/// overwrite, the machine checks each step as it runs and stops the run at one that writes over -/// data the workbook held when the run started (asking under `ask`, failing otherwise); the run's -/// undo group then rolls back. Shifting needs to know where the macro writes before it runs, so -/// `shiftDown` and `shiftRight` stop the run as `fail` does. -pub fn make_way(s: &mut Session, b: &Begun) -> Result { - let mut machine = Machine::new(&b.prepared); +/// The footprint check and the output rule, inside the run's undo group: an error when the macro +/// would fail or write over data it may not, after shifting the data if the rule says so. Returns +/// the machine for the run and whether it shifted. The dry runs count towards the run's time +/// limit. +/// +/// A partial footprint proves nothing about the steps it didn't reach, so then the machine +/// checks each step as it runs and stops the run at one that writes over data. +pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { + let (m, rule) = (&b.mac, b.rule); + let mut machine = Machine::new(&b.prepared, false); machine.time_limit_ms = b.time_limit_ms; - if b.rule != OnConflict::Overwrite { - let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); - machine.guard = Some(Guard::new(start, b.rule == OnConflict::Ask)); + if rule == OnConflict::Overwrite { + return Ok((machine, false)); } - Ok(machine) + // The dry runs and the run share one step budget as they share one clock. + let mut fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?; + machine.executed = fp.executed; + if let Some(e) = fp.error.take() { + return Err(e.into_engine(&m.name)); + } + let mut shifted = false; + if fp.conflict_count() > 0 { + let what = fp.describe_conflicts(); + match rule { + OnConflict::Ask => { + return Err(EngineError::Confirm(format!( + "Macro '{}' would write over cells that hold data ({what}). Overwrite them, or run again with onConflict shiftDown, shiftRight or fail", + m.name + ))); + } + OnConflict::Fail | OnConflict::Overwrite => { + return Err(EngineError::Other(format!("Macro '{}' would write over cells that hold data ({what}). Nothing was changed.", m.name))); + } + OnConflict::ShiftDown | OnConflict::ShiftRight => { + make_room(s, &fp, &b.prepared, rule == OnConflict::ShiftDown)?; + fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?; + machine.executed = fp.executed; + if let Some(e) = fp.error.take() { + return Err(e.into_engine(&m.name)); + } + if fp.conflict_count() > 0 { + return Err(EngineError::Other(format!( + "Macro '{}': shifting the data didn't make room ({}). Nothing was changed.", + m.name, + fp.describe_conflicts() + ))); + } + shifted = true; + } + } + } + if let Some(p) = &fp.partial { + let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); + machine.guard = Some(Guard::new(start, p, rule == OnConflict::Ask)); + } + Ok((machine, shifted)) } /// Runs macro `name` on the active workbook: `{inputs?, onConflict?, overwrite?, approve?, @@ -861,16 +1226,21 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result { let b = begin(s, name, p)?; let label = format!("Run Macro: {}", b.mac.name); s.grouped(&label, |s| { - let mut machine = make_way(s, &b)?; + let (mut machine, shifted) = make_way(s, &b)?; let name = &b.mac.name; + // `ran` counts the run's own steps; the budget also holds the dry runs'. + let before_run = machine.executed; machine.run_to_end(s).map_err(|e| e.into_engine(name))?; match machine.stopped { Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} Nothing was changed."))), stopped => { - let mut out = json!({"macro": name, "ran": machine.executed, "inputs": b.inputs}); + let mut out = json!({"macro": name, "ran": machine.executed.saturating_sub(before_run), "inputs": b.inputs}); if let Some((msg, _)) = stopped { out["stopped"] = json!(msg); } + if shifted { + out["shifted"] = json!(b.rule.name()); + } Ok(out) } } @@ -895,3 +1265,49 @@ fn conflict_rule(p: &Json, m: &Macro) -> Result { } Ok(rule) } + +/// `macro.check`: what running the macro would do, without changing anything. +pub fn check(s: &mut Session, name: &str, p: &Json) -> Result { + let d = s.doc()?; + let (doc, trust) = (d.uid, d.trust); + let m = super::find(&d.wb, name) + .and_then(|i| d.wb.macros.get(i)) + .cloned() + .ok_or_else(|| EngineError::Other(format!("There is no macro called '{name}'.")))?; + let mut out = json!({"macro": m.name, "ok": false, "onConflict": m.output.on_conflict.name()}); + let asks = match gate(&m, trust) { + Ok(a) => a, + Err(e) => { + out["errors"] = json!([e.to_string()]); + return Ok(out); + } + }; + let approved = s.macro_approvals.contains(&(doc, super::fingerprint(&m))); + if !asks.is_empty() && !approved { + // Nothing from an unapproved macro runs, not even in a dry run. + out["needsApproval"] = json!(asks); + } + let prepared = match prepare(s, &m, p.get("inputs")) { + Ok(pr) => pr, + Err(e) => { + out["errors"] = json!([e.to_string()]); + return Ok(out); + } + }; + out["inputs"] = prepared.inputs_json(&s.doc()?.wb); + if let Some(o) = prepared.output { + let wb = &s.doc()?.wb; + out["output"] = json!(resolve::a1(o, &Env { wb, active_sheet: wb.active_sheet, inputs: &[], output: None, item: None })); + } + if !asks.is_empty() && !approved { + return Ok(out); + } + let fp = footprint(s, &prepared, Stopwatch::start(), time_limit(p), 0)?; + out["ok"] = json!(fp.error.is_none()); + out["conflicts"] = json!(fp.conflict_count()); + if let Some(e) = &fp.error { + out["errors"] = json!([e.describe()]); + } + out["footprint"] = serde_json::to_value(&fp).unwrap_or(Json::Null); + Ok(out) +} diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 262f50af9..2f5b71a61 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -1,5 +1,4 @@ -//! Macros: running, input checks, checking each step for writes over data, the policy and the -//! limits. +//! Macros: running, input checks, the footprint dry run, the policy and the limits. use std::sync::Arc; @@ -157,25 +156,208 @@ fn conflicts_ask_fail_or_overwrite() { let start = s.doc().unwrap().wb.clone(); let r = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5"})); let Err(EngineError::Confirm(q)) = r else { panic!("{r:?}") }; - assert!(q.contains("Step 2 (cell.set): It would write over data (E3 on Sheet1)"), "{q}"); + assert!(q.contains("would write over cells that hold data (E3 on Sheet1)"), "{q}"); assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "programmatic runs default to failing"); let err = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": "fail"})).unwrap_err().to_string(); assert!(err.contains("Nothing was changed"), "{err}"); - assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); - // Shifting needs to know where the macro writes before it runs: it stops as fail does. - for rule in ["shiftDown", "shiftRight"] { - let err = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": rule})).unwrap_err().to_string(); - assert!(err.contains("would write over data (E3 on Sheet1)"), "{rule}: {err}"); - assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{rule}"); - } assert!(s.execute("macro.run", json!({"name": "Totals", "onConflict": "sideways"})).is_err()); + // macro.check shows the footprint and the conflict, and changes nothing. + let journal = s.journal.len(); + let sel = s.doc().unwrap().selection.clone(); + let c = s.execute("macro.check", json!({"name": "Totals", "inputs": "B2:C5"})).unwrap(); + assert_eq!(c["ok"], true); + assert_eq!(c["conflicts"], 1); + assert_eq!(c["output"], "E2"); + assert_eq!(c["footprint"]["sheets"][0]["cells"], json!(["E2:E3"])); + assert_eq!(c["footprint"]["sheets"][0]["conflicts"], json!(["E3"])); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); + assert_eq!((s.journal.len(), &s.doc().unwrap().selection), (journal, &sel)); + // The answer to the question overwrites. s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "overwrite": true})).unwrap(); assert_eq!(v(&s, "E3").display(), "400"); assert_eq!(v(&s, "H1").display(), "400"); } +#[test] +fn shifting_makes_room_and_is_checked_again() { + for (rule, moved) in [("shiftDown", ["E4", "E5"]), ("shiftRight", ["F2", "F3"])] { + let mut s = s(); + data(&mut s); + set(&mut s, "E2", "old 1"); + set(&mut s, "E3", "old 2"); + put(&mut s, totals()); + let r = s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": rule})).unwrap(); + assert_eq!(r["shifted"], rule); + assert_eq!((v(&s, "E2").display(), v(&s, "E3").display()), ("Total".into(), "400".into()), "{rule}"); + assert_eq!((v(&s, moved[0]).display(), v(&s, moved[1]).display()), ("old 1".into(), "old 2".into()), "{rule}"); + // The shift and the run are one undo step. + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(v(&s, "E2").display(), "old 1", "{rule}"); + assert!(v(&s, moved[1]).is_empty(), "{rule}"); + } + // A cell written with what it already holds isn't a conflict, but it moves with the others, + // so rows stay together. + let mut s = super::s(); + for (cell, input) in [("F2", "x"), ("G2", "b"), ("F3", "f3"), ("G3", "g3")] { + set(&mut s, cell, input); + } + put( + &mut s, + json!({"name": "Pair", "output": {"onConflict": "shiftDown"}, "steps": [ + {"command": "cell.set", "params": {"cell": "F2", "input": "a"}}, + {"command": "cell.set", "params": {"cell": "G2", "input": "b"}} + ]}), + ); + let c = s.execute("macro.check", json!({"name": "Pair"})).unwrap(); + assert_eq!((c["conflicts"].as_u64(), &c["footprint"]["sheets"][0]["cells"]), (Some(1), &json!(["F2:G2"]))); + s.execute("macro.run", json!({"name": "Pair"})).unwrap(); + let row = |s: &Session, r: u32| (v(s, &format!("F{r}")).display(), v(s, &format!("G{r}")).display()); + assert_eq!(row(&s, 2), ("a".into(), "b".into())); + assert_eq!(row(&s, 3), ("x".into(), "b".into())); + assert_eq!(row(&s, 4), ("f3".into(), "g3".into())); + // An anchor that moves with the shifted data still lands on it: the second check refuses. + let mut s = super::s(); + data(&mut s); + set(&mut s, "E2", "old"); + s.execute("formulas.defineName", json!({"name": "Target", "refersTo": "=Sheet1!$E$2"})).unwrap(); + put( + &mut s, + json!({"name": "Named", "steps": [{"command": "cell.set", "params": {"cell": "Name:Target", "input": "new"}}], "output": {"onConflict": "shiftDown"}}), + ); + let start = s.doc().unwrap().wb.clone(); + let err = s.execute("macro.run", json!({"name": "Named"})).unwrap_err().to_string(); + assert!(err.contains("shifting the data didn't make room (E3 on Sheet1)"), "{err}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); +} + +#[test] +fn a_shift_that_would_move_an_input_is_refused() { + // A shift inserts cells over the conflicting block, which moves the block's columns (or rows) + // from there on. When one of the macro's own inputs lies in the way, its data would move and + // shear while the run keeps the old coordinates, so the shift is refused and nothing changes. + let mut s = s(); + data(&mut s); + set(&mut s, "B1", "Old title"); + put( + &mut s, + json!({"name": "Title", "inputs": [{"name": "Input", "header": true}], "steps": [ + {"command": "cell.set", "params": {"cell": "Input.cell(0,1)", "input": "Report"}} + ]}), + ); + let start = s.doc().unwrap().wb.clone(); + let err = s.execute("macro.run", json!({"name": "Title", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap_err().to_string(); + assert!(err.contains("would move Input (") && err.contains("B2:C5"), "{err}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "nothing changed"); + assert_eq!((v(&s, "B1").display(), v(&s, "B3").display()), ("Old title".into(), "Ann".into())); + // Writing left of the input and shifting right would move it too. + put( + &mut s, + json!({"name": "Left", "inputs": [{"name": "Input", "header": true}], "steps": [ + {"command": "cell.set", "params": {"cell": "Input.offset(1,-1).cell(1,1)", "input": "x"}} + ]}), + ); + set(&mut s, "A3", "in the way"); + let err = s.execute("macro.run", json!({"name": "Left", "inputs": "B2:C5", "onConflict": "shiftRight"})).unwrap_err().to_string(); + assert!(err.contains("would move Input"), "{err}"); + // The output counts as well. + put( + &mut s, + json!({"name": "Out", "inputs": [{"name": "Input", "header": true}], "output": {"anchor": "Input.below(1)"}, "steps": [ + {"command": "cell.set", "params": {"cell": "Input.cell(0,1)", "input": "Report"}} + ]}), + ); + let err = s.execute("macro.run", json!({"name": "Out", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap_err().to_string(); + assert!(err.contains("would move Input"), "{err}"); + // A shift that moves nothing of the macro's own still works, over the block of every written + // cell, so a row the macro writes stays together. + set(&mut s, "H1", "elsewhere"); + put( + &mut s, + json!({"name": "Wide", "inputs": [{"name": "Input", "header": true}], "steps": [ + {"command": "cell.set", "params": {"cell": "H1", "input": "over"}}, + {"command": "cell.set", "params": {"cell": "H9", "input": "far"}} + ]}), + ); + s.execute("macro.run", json!({"name": "Wide", "inputs": "B2:C5", "onConflict": "shiftDown"})).unwrap(); + assert_eq!((v(&s, "H1").display(), v(&s, "H9").display(), v(&s, "H10").display()), ("over".into(), "far".into(), "elsewhere".into())); + assert_eq!(v(&s, "B3").display(), "Ann", "the input did not move"); +} + +#[test] +fn a_shift_is_refused_when_the_footprint_cannot_follow_every_write() { + // The touch log keeps at most a million writes; past that, a write that kept a cell's value + // is missing from the footprint, so a shift could split a row the macro writes. The footprint + // says so and the shift is refused (overwrite, with no dry run, still runs). + let mut s = s(); + set(&mut s, "A1", "x"); + set(&mut s, "E2", "old"); + put( + &mut s, + json!({"name": "Flood", "steps": [ + {"command": "edit.fillDown", "params": {"range": "A1:A1000001"}}, + {"command": "cell.set", "params": {"cell": "E2", "input": "new"}} + ]}), + ); + let c = s.execute("macro.check", json!({"name": "Flood"})).unwrap(); + assert_eq!(c["footprint"]["logFull"], true, "{c}"); + let err = s.execute("macro.run", json!({"name": "Flood", "onConflict": "shiftDown"})).unwrap_err().to_string(); + assert!(err.contains("more cells than the footprint can follow"), "{err}"); + assert_eq!(v(&s, "E2").display(), "old", "nothing changed"); +} + +#[test] +fn a_dry_run_leaves_a_clean_workbook_clean() { + // `view.zoom` and `sheet.activate` move the saved mark to the current workbook on a clean + // document; the dry run's rollback puts the workbook back, so the mark must come back too. + let mut s = s(); + data(&mut s); + let d = s.doc_mut().unwrap(); + d.saved = d.wb.clone(); + assert!(!s.doc().unwrap().is_dirty()); + put(&mut s, json!({"name": "Look", "steps": [{"command": "view.zoom", "params": {"percent": 120}}]})); + s.execute("macro.check", json!({"name": "Look"})).unwrap(); + assert!(!s.doc().unwrap().is_dirty(), "a dry run changed nothing"); + // A run refused by the conflict rule changed nothing either. + put(&mut s, totals()); + set(&mut s, "E3", "taken"); + let d = s.doc_mut().unwrap(); + d.saved = d.wb.clone(); + assert!(s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "onConflict": "fail"})).is_err()); + assert!(!s.doc().unwrap().is_dirty()); +} + +#[test] +fn a_dry_run_puts_the_journal_back_even_after_it_trimmed_it() { + // Every nested command a dry run executes is journaled, so a long macro pushes the journal + // past its limit and the journal drops its oldest entries. Putting the journal back by its + // old length would then keep entries from the dry run and point a recording at the wrong + // place; the restore counts what was dropped. + let mut s = s(); + data(&mut s); + s.execute("selection.set", json!({"range": "A1"})).unwrap(); + let journal = s.journal.clone(); + s.recording = Some(journal.len().saturating_sub(1)); + put( + &mut s, + json!({"name": "Long", "steps": [{"type": "repeat", "over": "A1:A200", "body": [ + {"type": "repeat", "over": "A1:A60", "body": [{"command": "cell.set", "params": {"cell": "Row.cell(1,1)", "input": "x"}}]} + ]}]}), + ); + s.execute("macro.check", json!({"name": "Long"})).unwrap(); + let sets = |j: &[(String, Json)]| j.iter().filter(|(id, _)| id == "cell.set").count(); + assert!(sets(&s.journal) <= sets(&journal), "no dry-run entry stays"); + assert!(s.journal.len() <= journal.len(), "{} entries, had {}", s.journal.len(), journal.len()); + assert!(journal.ends_with(&s.journal), "what stays is the journal's own tail"); + assert!(s.recording.is_some_and(|r| r <= s.journal.len()), "{:?}", s.recording); + // A short dry run leaves the journal exactly as it was. + put(&mut s, json!({"name": "Short", "steps": [{"command": "cell.set", "params": {"cell": "H1", "input": "x"}}]})); + let (journal, recording) = (s.journal.clone(), s.recording); + s.execute("macro.check", json!({"name": "Short"})).unwrap(); + assert_eq!((&s.journal, s.recording), (&journal, recording)); +} + #[test] fn each_step_is_checked_for_writes_over_data() { let dir = std::env::temp_dir().join(format!("gc-macro-guard-{}", std::process::id())); @@ -232,6 +414,66 @@ fn the_answer_to_the_question_counts_however_ask_was_given() { assert_eq!(v(&s, "E3").display(), "400"); } +#[test] +fn the_step_limit_is_one_budget_across_the_dry_run_and_the_run() { + // 300 x 300 passes is under the limit for one phase; the dry run and the run together are not. + let mut s = s(); + put( + &mut s, + json!({"name": "Spin", "steps": [{"type": "repeat", "over": "A1:A300", "body": [ + {"type": "repeat", "over": "A1:A300", "body": [{"type": "if", "cell": "A1", "op": "empty"}]} + ]}]}), + ); + let err = s.execute("macro.run", json!({"name": "Spin"})).unwrap_err().to_string(); + assert!(err.contains("more than 100000 steps"), "{err}"); + // With no dry run (overwrite), the same macro fits. + s.execute("macro.run", json!({"name": "Spin", "onConflict": "overwrite"})).unwrap(); +} + +#[test] +fn the_footprint_covers_other_sheets_objects_and_edges() { + let mut s = s(); + data(&mut s); + s.execute("home.insertSheet", json!({"name": "Notes"})).unwrap(); + set(&mut s, "A1", "note"); + s.execute("sheet.activate", json!({"sheet": "Sheet1"})).unwrap(); + s.execute("insert.chart", json!({"range": "B2:C5", "type": "column", "at": "H2"})).unwrap(); + let chart = s.doc().unwrap().wb.sheet(0).unwrap().charts[0].id; + put( + &mut s, + json!({"name": "Wide", "output": {"onConflict": "fail"}, "steps": [ + {"command": "cell.set", "params": {"cell": "Notes!A1", "input": "x"}}, + {"command": "cell.set", "params": {"cell": "Notes!B2", "input": "y"}}, + {"command": "home.insertSheet", "params": {"name": "Extra"}}, + {"command": "sheet.activate", "params": {"sheet": "Sheet1"}}, + {"command": "insert.shape", "params": {"kind": "ellipse", "at": "J2"}}, + {"command": "object.delete", "params": {"kind": "chart", "id": chart}} + ]}), + ); + let c = s.execute("macro.check", json!({"name": "Wide", "inputs": {}})).unwrap(); + let fp = &c["footprint"]; + assert_eq!( + fp["sheets"], + json!([{"sheet": "Notes", "cells": ["A1", "B2"], "changed": 2, "conflicts": ["A1"], "conflictCount": 1, "bounds": "A1:B2"}]) + ); + assert_eq!(fp["sheetsAdded"], json!(["Extra"])); + assert_eq!((fp["objectsAdded"].as_u64(), fp["objectsRemoved"].as_u64()), (Some(1), Some(1))); + assert_eq!(c["conflicts"], 2, "the deleted chart and the note"); + assert_eq!(s.doc().unwrap().wb.sheets.len(), 2, "the dry run's new sheet went away"); + let err = s.execute("macro.run", json!({"name": "Wide"})).unwrap_err().to_string(); + assert!(err.contains("(A1 on Notes; 1 object)"), "{err}"); + + // Running past the edge of the sheet fails before anything changes. + put( + &mut s, + json!({"name": "Edge", "inputs": [{"name": "Input"}], "steps": [{"command": "cell.set", "params": {"cell": "Input.offset(-3,0)", "input": "x"}}]}), + ); + let err = s.execute("macro.run", json!({"name": "Edge", "inputs": "B2"})).unwrap_err().to_string(); + assert!(err.contains("Step 1 (cell.set): 'Input.offset(-3,0)' runs past the top of the sheet."), "{err}"); + let c = s.execute("macro.check", json!({"name": "Edge", "inputs": "B2"})).unwrap(); + assert_eq!((c["ok"].as_bool(), c["footprint"]["error"]["edge"].as_bool()), (Some(false), Some(true))); +} + #[test] fn repeat_if_and_stop() { let mut s = s(); @@ -312,7 +554,7 @@ fn macros_are_bounded() { assert!(err.contains("ran more than 100000 steps"), "{err}"); let err = s.execute("macro.run", json!({"name": "Spin", "timeLimit": 0.001})).unwrap_err().to_string(); assert!(err.contains("ran longer than"), "{err}"); - // A run that checks each step keeps to the time limit too. + // The footprint's dry run keeps to the time limit too, and so does the check's. put( &mut s, json!({"name": "Spin", "output": {"onConflict": "fail"}, "steps": [{"type": "repeat", "over": "A1:A400", "body": [ @@ -321,6 +563,8 @@ fn macros_are_bounded() { ); let err = s.execute("macro.run", json!({"name": "Spin", "timeLimit": 0.001})).unwrap_err().to_string(); assert!(err.contains("ran longer than"), "{err}"); + let c = s.execute("macro.check", json!({"name": "Spin", "timeLimit": 0.001})).unwrap(); + assert!(c["errors"][0].as_str().unwrap().contains("ran longer than"), "{c}"); } #[test] @@ -333,22 +577,33 @@ fn a_token_in_a_path_needs_the_permission_and_approval() { for path in [json!({"$ref": "Input"}), json!(in_dir), json!(" ")] { let step = json!({"command": "file.exportCsv", "params": {"path": path}}); put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step]})); - // Without the permission nothing runs. + // Without the permission nothing runs, not even the check's dry run. let err = s.execute("macro.run", json!({"name": "Export"})).unwrap_err().to_string(); assert!(err.contains("Step 1 (file.exportCsv) writes a file. Add file.exportCsv to the macro's permissions"), "{path}: {err}"); - // With it, the person approves first. + let c = s.execute("macro.check", json!({"name": "Export"})).unwrap(); + assert!(c["errors"][0].as_str().unwrap().contains("writes a file"), "{path}: {c}"); + assert!(c.get("footprint").is_none()); + // With it, the person approves first; the check doesn't run it before then. put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step], "permissions": ["file.exportCsv"]})); let r = s.execute("macro.run", json!({"name": "Export"})); let Err(EngineError::Approve(q)) = &r else { panic!("{path}: {r:?}") }; assert!(q.contains("file.exportCsv, which writes a file"), "{q}"); + let c = s.execute("macro.check", json!({"name": "Export"})).unwrap(); + assert_eq!(c["needsApproval"], json!(["runs file.exportCsv, which writes a file"]), "{path}"); + assert!(c.get("footprint").is_none()); assert_eq!(files(&dir), 0, "{path}"); assert!(!std::path::Path::new("A1.csv").exists() && !std::path::Path::new(" .csv").exists()); } - // Once approved, the run writes the file. + // Once approved, the check's dry run stops before the step, and only the run writes the file. let step = json!({"command": "file.exportCsv", "params": {"path": in_dir}}); put(&mut s, json!({"name": "Export", "inputs": [{"name": "Input"}], "steps": [step], "permissions": ["file.exportCsv"]})); s.execute("macro.run", json!({"name": "Export", "approve": true, "inputs": "B2"})).unwrap(); assert_eq!(files(&dir), 1); + std::fs::remove_dir_all(&dir).unwrap(); + std::fs::create_dir_all(&dir).unwrap(); + let c = s.execute("macro.check", json!({"name": "Export", "inputs": "B2"})).unwrap(); + assert_eq!(c["footprint"]["partial"]["step"], "1", "{c}"); + assert_eq!(files(&dir), 0, "the check writes nothing"); let _ = std::fs::remove_dir_all(&dir); } @@ -391,10 +646,16 @@ fn the_policy_keeps_macros_away_from_files_and_the_app() { let Err(EngineError::Approve(q)) = &r else { panic!("{r:?}") }; assert_eq!(q, "Run macro 'Import'? It runs data.fromTextCsv, which reads a file."); assert_eq!(s.doc().unwrap().wb.sheets.len(), 1); + let c = s.execute("macro.check", json!({"name": "Import"})).unwrap(); + assert_eq!(c["needsApproval"], json!(["runs data.fromTextCsv, which reads a file"])); + assert!(c.get("footprint").is_none(), "nothing unapproved runs, even dry"); s.execute("macro.run", json!({"name": "Import", "approve": true})).unwrap(); assert_eq!(s.doc().unwrap().wb.sheets.len(), 2); s.execute("edit.undo", json!({})).unwrap(); s.execute("macro.run", json!({"name": "Import"})).unwrap(); + // The dry run skips the file step and says so. + let c = s.execute("macro.check", json!({"name": "Import"})).unwrap(); + assert_eq!(c["footprint"]["partial"]["step"], "1"); // Changing the macro asks again. put(&mut s, json!({"name": "Import", "description": "changed", "steps": [step], "permissions": ["data.fromTextCsv"]})); assert!(matches!(s.execute("macro.run", json!({"name": "Import"})), Err(EngineError::Approve(_)))); @@ -439,3 +700,63 @@ fn macros_from_a_file_ask_before_running() { let b = crate::macros::parse_macro(&json!({"steps": [{"params": {"input": "x", "cell": "A1"}, "command": "cell.set"}], "name": "M"})).unwrap(); assert_eq!(crate::macros::fingerprint(&a), crate::macros::fingerprint(&b)); } + +#[test] +fn a_partial_footprint_doesnt_clear_later_writes() { + let dir = std::env::temp_dir().join(format!("gc-macro-partial-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let csv = dir.join("out.csv").to_string_lossy().to_string(); + let export = json!({"command": "file.exportCsv", "params": {"path": csv}}); + let solver = json!({"command": "data.solver", "params": {"objective": "C1", "variables": "A1", "constraints": ["A1 <= 5"], "method": "simplex", "saveModel": false}}); + let late = json!({"command": "cell.set", "params": {"cell": "B1", "input": "over"}}); + // The dry run stops at an approved file step or at Solver; the step after it writes over B1. + for first in [&export, &solver] { + let mut s = super::s(); + set(&mut s, "A1", "1"); + set(&mut s, "C1", "=A1"); + set(&mut s, "B1", "keep"); + put(&mut s, json!({"name": "Late", "inputs": [{"name": "Input"}], "permissions": ["file.exportCsv"], "steps": [first, late]})); + let start = s.doc().unwrap().wb.clone(); + let c = s.execute("macro.check", json!({"name": "Late", "inputs": "A1"})).unwrap(); + if c.get("needsApproval").is_none() { + assert_eq!((c["conflicts"].as_u64(), &c["footprint"]["partial"]["step"]), (Some(0), &json!("1")), "{c}"); + } + for rule in ["fail", "ask"] { + let r = s.execute("macro.run", json!({"name": "Late", "inputs": "A1", "onConflict": rule, "approve": true})); + let e = match (rule, r) { + ("fail", Err(EngineError::Other(e))) | ("ask", Err(EngineError::Confirm(e))) => e, + (_, r) => panic!("{first} {rule}: {r:?}"), + }; + assert!(e.contains("Step 2 (cell.set): It would write over data (B1 on Sheet1)"), "{e}"); + assert_eq!(v(&s, "B1").display(), "keep", "{first} {rule}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{first} {rule}: rolled back"); + } + // The answer to the question overwrites. + s.execute("macro.run", json!({"name": "Late", "inputs": "A1", "overwrite": true})).unwrap(); + assert_eq!(v(&s, "B1").display(), "over"); + } + // A step that writes over what the macro itself wrote isn't a conflict. + let mut s = super::s(); + let again = |input: &str| json!({"command": "cell.set", "params": {"cell": "E1", "input": input}}); + put( + &mut s, + json!({"name": "Twice", "permissions": ["file.exportCsv"], "output": {"onConflict": "fail"}, "steps": [again("a"), export, again("b")]}), + ); + s.execute("macro.run", json!({"name": "Twice", "approve": true})).unwrap(); + assert_eq!(v(&s, "E1").display(), "b"); + // After a shift, the steps past the file step are checked too. + let mut s = super::s(); + set(&mut s, "C1", "old"); + set(&mut s, "D1", "keep"); + let step = |cell: &str| json!({"command": "cell.set", "params": {"cell": cell, "input": "new"}}); + put( + &mut s, + json!({"name": "Shift", "permissions": ["file.exportCsv"], "output": {"onConflict": "shiftDown"}, "steps": [step("C1"), export, step("D1")]}), + ); + let start = s.doc().unwrap().wb.clone(); + let err = s.execute("macro.run", json!({"name": "Shift", "approve": true})).unwrap_err().to_string(); + assert!(err.contains("Step 3 (cell.set): It would write over data (D1 on Sheet1)"), "{err}"); + assert_eq!((v(&s, "C1").display(), v(&s, "D1").display()), ("old".into(), "keep".into())); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "the shift is rolled back too"); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/docs/macros.md b/docs/macros.md index daf0a2c35..a56bd77a9 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -1,17 +1,16 @@ # Macros -> **Last updated:** 2026-10-25 · **Change:** minor (running macros: `macro.run`) +> **Last updated:** 2026-10-25 · **Change:** minor (checking macros before they run: the footprint, shifting, `macro.check`) GridCraft macros are typed, declarative lists of commands, not VBA. Every step runs a GridCraft command (the same ids the ribbon, CLI and MCP use) or one of three bounded control steps. A macro declares the inputs it works on, runs as one undo step, rolls all workbook changes back on an -error (a file it already wrote stays), stops before it writes over data it may not, and does not -touch files without permission. +error (a file it already wrote stays), checks where its results will land before it writes them, +and does not touch files without permission. Agents run macros with `macro.run` like any other command. -This page describes the macro format, its tokens, and running macros. The engine has no check of a -macro before it runs, no macro debugger, recorder or management commands in this version, and no -macro interface. +This page describes the macro format, its tokens, and running and checking macros. The engine has +no macro debugger, recorder or management commands in this version, and no macro interface. ## A macro @@ -60,8 +59,9 @@ macro interface. whole macro (4,194,304 characters of JSON). A macro read from a file that breaks any of these is left out with a warning; it is never changed to fit. When a macro runs, a Repeat whose range has more rows or columns than its `max` is refused, and a run executes at most 100,000 steps (each - pass through a Repeat counts) and takes at most 60 seconds (`timeLimit` to change the time; - the web build has no clock, so there only the step limit applies). + pass through a Repeat counts) and takes at most 60 seconds, both shared by the footprint dry + runs and the run itself (`timeLimit` to change the time; the web build has no clock, so there + only the step limit applies). ## Tokens @@ -108,17 +108,24 @@ refused rather than read as the header. are when the step runs. A macro that touches files, or that came with a workbook opened from a file, runs once the person approves it: the call fails with `approve: …` and runs with `approve: true`. Approval lasts for the session, until the macro changes. -2. **Conflicts.** Unless the conflict rule is `overwrite`, each step is checked as it runs. A - step that writes over cells outside the inputs that held data when the run started, or deletes - sheets or objects that were there, stops the run, which rolls the workbook back (a file already - written stays). The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call - fails with `confirm: …`; `overwrite: true` answers it), `fail` or `overwrite`. Shifting the - data to make room (`shiftDown`, `shiftRight`) needs to know where the macro writes before it - runs, which this version doesn't work out, so those rules stop the run as `fail` does. +2. **Footprint.** Unless the conflict rule is `overwrite`, the macro first runs in dry-run mode on + the real workbook, and everything is put back. Cells it would write outside its + inputs that hold data, and sheets and objects it would + delete, are conflicts, handled by `onConflict` (or the macro's `output.onConflict`): `ask` (the + call fails with `confirm: …`; `overwrite: true` answers it), `fail`, `overwrite`, `shiftDown` or + `shiftRight` (Insert Cells makes room, then the footprint is checked again). Steps that touch + files, and Solver, end the dry run early; the footprint then says it is partial, and unless + the rule is `overwrite`, the run checks every step as it goes. A step that writes over cells, + sheets or objects that held data when the run started stops the run and rolls the workbook + back (a file already written stays): it asks (`confirm: …`) under `ask` and fails otherwise, + since shifting can't make room that late. 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside an If). +`macro.check {name, inputs?, timeLimit?}` reports the bound inputs, the output anchor, any +approval needed and the footprint, without changing anything. + ## Storage Macros are saved in the workbook: as a field in GridCraft's JSON format, and in xlsx as hidden From d053b1f54820781ebb5a2bd6f6195c600dd3fed7 Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:35:02 -0400 Subject: [PATCH 3/7] Engine: a run is checked step by step even when its footprint was complete A footprint is not proof of what the run will do. RAND gives the run other values than the dry run before it (the recalculation's random sequence goes on), and NOW moves on, so an If can take another branch in the run and write over data the dry run never reached; under `fail` the run went ahead and wrote over it. - Unless the rule is `overwrite`, every run checks each step as it goes, as a run after a partial footprint already did; a write the footprint didn't see stops the run, which asks under `ask` and fails otherwise, and rolls back. - Test: `a_run_that_takes_another_path_than_its_check_is_still_checked` (an If on `=RAND()` over 40 runs under `fail`; before, one of them wrote over B1). --- crates/engine/src/macros/run.rs | 40 ++++++++++++++++++------------- crates/engine/src/tests/macros.rs | 25 +++++++++++++++++++ docs/macros.md | 6 +++-- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index f45bcfc25..9719e1e09 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -9,9 +9,9 @@ //! 3. **Footprint**: unless the macro may overwrite, it first runs on the real workbook in dry-run //! mode, the changes are compared with `CellStore::diff`, and everything is put back. Cells it //! would write outside its inputs that already hold data are conflicts, handled by the output -//! rule: ask, fail, overwrite, or shift the data down or right (and check again). A dry run -//! that stops early (a file step, Solver) proves nothing about later steps, so the run then -//! checks each step as it goes. +//! rule: ask, fail, overwrite, or shift the data down or right (and check again). The +//! footprint can miss a write (a dry run stops early at a file step or Solver, and RAND or NOW +//! can send the run another way than the dry run), so the run also checks each step as it goes. //! 4. **Run**: one undo step (`Session::grouped`); an error at any step rolls everything back and //! names the step. //! @@ -430,27 +430,30 @@ pub struct Machine { pub partial: Option<(String, String)>, /// The last step's resolved parameters (for the debugger). pub last_params: Option, - /// Set when the footprint was partial: each step is checked for writes over data as it runs. + /// Set unless the macro may overwrite: each step is checked for writes over data as it runs, + /// since the footprint can miss a write (it stopped early, or the run took another path). guard: Option, } -/// Checks each step of a real run for writes over data the footprint couldn't see. +/// Checks each step of a real run for writes over data the footprint didn't see. #[derive(Clone, Debug)] struct Guard { /// The workbook when the run started, and for each sheet now, its index there (`None` for a /// sheet the run added). start: Arc, origin: Vec>, - /// Where the footprint stopped: "step 2 (file.exportCsv writes a file)". - partial: String, + /// Where the footprint stopped, if it did: "step 2 (file.exportCsv writes a file)". + partial: Option, /// The conflict rule is ask. ask: bool, } impl Guard { - fn new(start: Arc, partial: &Json, ask: bool) -> Guard { - let text = |k: &str| partial.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); - let partial = format!("step {} ({})", text("step"), text("reason")); + fn new(start: Arc, partial: Option<&Json>, ask: bool) -> Guard { + let partial = partial.map(|p| { + let text = |k: &str| p.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); + format!("step {} ({})", text("step"), text("reason")) + }); Guard { origin: (0..start.sheets.len()).map(Some).collect(), start, partial, ask } } @@ -714,7 +717,11 @@ impl Machine { let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { - let msg = format!("It would write over data ({what}); the check before the run stopped at {} and couldn't see this.", g.partial); + let msg = match &g.partial { + Some(at) => format!("It would write over data ({what}); the check before the run stopped at {at} and couldn't see this."), + // A value such as RAND or NOW differed from the check's, and the run went another way. + None => format!("It would write over data ({what}); the check before the run didn't see this write."), + }; return Err(StepError { confirm: g.ask, ..fail(msg) }); } } @@ -1167,8 +1174,9 @@ pub fn begin(s: &mut Session, name: &str, p: &Json) -> Result { /// the machine for the run and whether it shifted. The dry runs count towards the run's time /// limit. /// -/// A partial footprint proves nothing about the steps it didn't reach, so then the machine -/// checks each step as it runs and stops the run at one that writes over data. +/// The footprint proves nothing about steps it didn't reach (it stopped early) or a path the run +/// takes and the dry run didn't (RAND, NOW), so the machine also checks each step as it runs and +/// stops the run at one that writes over data. pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { let (m, rule) = (&b.mac, b.rule); let mut machine = Machine::new(&b.prepared, false); @@ -1213,10 +1221,8 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { } } } - if let Some(p) = &fp.partial { - let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); - machine.guard = Some(Guard::new(start, p, rule == OnConflict::Ask)); - } + let start = s.docs.iter().find(|d| d.uid == b.prepared.doc).ok_or(EngineError::NoDocument)?.wb.clone(); + machine.guard = Some(Guard::new(start, fp.partial.as_ref(), rule == OnConflict::Ask)); Ok((machine, shifted)) } diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 2f5b71a61..94902ee7b 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -1,5 +1,6 @@ //! Macros: running, input checks, the footprint dry run, the policy and the limits. +use std::collections::BTreeSet; use std::sync::Arc; use serde_json::{Value as Json, json}; @@ -400,6 +401,30 @@ fn each_step_is_checked_for_writes_over_data() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn a_run_that_takes_another_path_than_its_check_is_still_checked() { + // RAND gives the run other values than the dry run before it, so an If can go the other way: + // the write the footprint didn't see must still not go over data. + let mut s = s(); + let steps = json!([ + {"command": "cell.set", "params": {"cell": "A1", "input": "=RAND()"}}, + {"type": "if", "cell": "A1", "op": "gt", "value": 0.5, "then": [{"command": "cell.set", "params": {"cell": "B1", "input": "over"}}]} + ]); + put(&mut s, json!({"name": "Coin", "output": {"onConflict": "fail"}, "steps": steps})); + let mut outcomes = BTreeSet::new(); + for _ in 0..40 { + set(&mut s, "B1", "keep"); + let r = s.execute("macro.run", json!({"name": "Coin"})); + assert_eq!(v(&s, "B1").display(), "keep", "{r:?}"); + outcomes.insert(match r { + Ok(_) => "ran", + Err(e) if e.to_string().contains("would write over") => "refused", + Err(e) => panic!("{e}"), + }); + } + assert_eq!(outcomes.len(), 2, "both paths were taken: {outcomes:?}"); +} + #[test] fn the_answer_to_the_question_counts_however_ask_was_given() { let mut s = s(); diff --git a/docs/macros.md b/docs/macros.md index a56bd77a9..fbda2f69a 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -114,8 +114,10 @@ refused rather than read as the header. delete, are conflicts, handled by `onConflict` (or the macro's `output.onConflict`): `ask` (the call fails with `confirm: …`; `overwrite: true` answers it), `fail`, `overwrite`, `shiftDown` or `shiftRight` (Insert Cells makes room, then the footprint is checked again). Steps that touch - files, and Solver, end the dry run early; the footprint then says it is partial, and unless - the rule is `overwrite`, the run checks every step as it goes. A step that writes over cells, + files, and Solver, end the dry run early; the footprint then says it is partial. Unless the + rule is `overwrite`, the run also checks every step as it goes, since the footprint can miss a + write: it is partial, or a value such as RAND or NOW sends the run another way than the dry + run. A step that writes over cells, sheets or objects that held data when the run started stops the run and rolls the workbook back (a file already written stays): it asks (`confirm: …`) under `ask` and fails otherwise, since shifting can't make room that late. From ad79b071ce8dfa22647cb4910530735c7cbb988b Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:40:21 -0400 Subject: [PATCH 4/7] Engine: a macro's inputs, output and Repeat ranges follow their cells when a step moves them Inputs, the output and a Repeat's range were bound once, by sheet index and coordinates. A step that inserted a sheet in front of an input's sheet sent every later token to the new sheet, and rows inserted above an input left the tokens on the cells that moved into its place; the run wrote into the wrong cells and reported success. - Commands record the structural edits they make as they rewrite references (`Ctx::edits`; while a macro step runs, `DocState::edits` collects them). - After each step the machine matches the sheets as the run's guard does and moves the inputs, the output and the ranges of the Repeats in progress by those edits, with the formula engine's own adjustment. - A step that deletes an input's or the output's cells, or their sheet, or changes the size of a range a Repeat is going over, stops the run, which rolls back. - Test: `inputs_and_the_output_follow_their_cells_when_a_step_moves_them` (a sheet inserted in front, rows inserted above, an input deleted, a Repeat's range grown). --- crates/engine/src/cmd/edit.rs | 1 + crates/engine/src/cmd/insert.rs | 2 +- crates/engine/src/cmd/mod.rs | 18 +++++++- crates/engine/src/cmd/sheet.rs | 18 ++++---- crates/engine/src/lib.rs | 4 ++ crates/engine/src/macros/run.rs | 68 ++++++++++++++++++++++++++++++- crates/engine/src/tests/macros.rs | 53 ++++++++++++++++++++++++ docs/macros.md | 5 ++- 8 files changed, 155 insertions(+), 14 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 102edc2d3..556d41759 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1386,6 +1386,7 @@ fn paste_special(s: &mut Session, p: &Json) -> Result { gridcraft_formula::adjust::Edit::MoveToSheet { from: src, to_sheet: dest_name, to_row: at.row, to_col: at.col } }; rewrite_all_formulas(&mut cx.wb, &src_sheet_name, &e); + cx.edits.push((src_sheet_name.clone(), e)); cx.structural = true; } let end = at.offset_clamped((tiles_r * h) as i64 - 1, (tiles_c * w) as i64 - 1); diff --git a/crates/engine/src/cmd/insert.rs b/crates/engine/src/cmd/insert.rs index 3c45031af..0078cb4fe 100644 --- a/crates/engine/src/cmd/insert.rs +++ b/crates/engine/src/cmd/insert.rs @@ -294,7 +294,7 @@ fn insert_table(s: &mut Session, p: &Json) -> Result { let e = Edit::InsertCells { axis: Axis::Rows, range: RangeRef::new(r.start, CellRef::new(r.start.row, r.end.col)) }; shift_cell_features(sh, &e); let name = sh.name.clone(); - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); let sh = cx.sheet_mut(sheet)?; for (i, c) in (r.start.col..=r.end.col).enumerate() { sh.set_value(CellRef::new(r.start.row, c), gridcraft_core::Value::text(format!("Column{}", i + 1))); diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 6f58f18ee..55da0377e 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -340,6 +340,9 @@ pub struct Ctx<'a> { /// The command applied sheet protection itself (row/column inserts and deletes the /// protection allows), so cells it shifts aren't refused as edits of locked cells. pub protection_checked: bool, + /// The structural edits the command made, in order (the sheet's name, the edit), as + /// references were rewritten for them. + pub edits: Vec<(String, gridcraft_formula::adjust::Edit)>, } pub(crate) fn edit(s: &mut Session, f: impl FnOnce(&mut Ctx) -> Result) -> Result { @@ -357,15 +360,26 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) #[allow(clippy::disallowed_methods)] let t0 = if cfg!(target_arch = "wasm32") { None } else { std::env::var_os("GRIDCRAFT_PROFILE").map(|_| std::time::Instant::now()) }; let mut sel = d.selection.clone(); - let mut ctx = Ctx { wb: (*d.wb).clone(), changed: Vec::new(), structural: false, sel: &mut sel, fit_rows: Vec::new(), protection_checked: false }; + let mut ctx = Ctx { + wb: (*d.wb).clone(), + changed: Vec::new(), + structural: false, + sel: &mut sel, + fit_rows: Vec::new(), + protection_checked: false, + edits: Vec::new(), + }; let r = f(&mut ctx)?; - let Ctx { mut wb, changed, structural, mut fit_rows, protection_checked, .. } = ctx; + let Ctx { mut wb, changed, structural, mut fit_rows, protection_checked, edits, .. } = ctx; if !protection_checked { check_protection(&d.wb, &wb, &d.unlocked_ranges)?; } if !structural { check_arrays(&d.wb, &wb)?; } + if let Some(log) = d.edits.as_mut() { + log.extend(edits); + } if changed.len() <= 200_000 { fit_rows.extend(changed.iter().map(|(s, c)| (*s, c.row))); } diff --git a/crates/engine/src/cmd/sheet.rs b/crates/engine/src/cmd/sheet.rs index e6caa1dc7..dfab93736 100644 --- a/crates/engine/src/cmd/sheet.rs +++ b/crates/engine/src/cmd/sheet.rs @@ -223,7 +223,9 @@ fn shift_sheet_features(sh: &mut Sheet, axis: Axis, at: u32, count: u32, insert: } /// Rewrites every formula reference (and chart series) for a structural edit on `target`. -pub(super) fn rewrite(wb: &mut gridcraft_model::Workbook, target: &str, e: &Edit) { +pub(super) fn rewrite(cx: &mut Ctx, target: &str, e: &Edit) { + cx.edits.push((target.to_string(), e.clone())); + let wb = &mut cx.wb; super::edit::rewrite_all_formulas(wb, target, e); for si in 0..wb.sheets.len() { let host = wb.sheets.get(si).map(|s| s.name.clone()).unwrap_or_default(); @@ -307,7 +309,7 @@ fn insert_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } } } - rewrite(&mut cx.wb, &name, &Edit::Insert { axis, at, count }); + rewrite(cx, &name, &Edit::Insert { axis, at, count }); cx.structural = true; Ok(Json::Null) })?; @@ -372,7 +374,7 @@ fn delete_lines(s: &mut Session, p: &Json, axis: Axis) -> Result { } shift_sheet_features(sh, axis, at, count, false); } - rewrite(&mut cx.wb, &name, &Edit::Delete { axis, at, count }); + rewrite(cx, &name, &Edit::Delete { axis, at, count }); } cx.structural = true; Ok(Json::Null) @@ -424,7 +426,7 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { sh.cells.shift_cols(before, count as i64); shift_sheet_features(sh, Axis::Cols, before, count, true); } - rewrite(&mut cx.wb, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); + rewrite(cx, &name, &Edit::Insert { axis: Axis::Cols, at: before, count }); let mut touched = Vec::new(); { @@ -462,14 +464,14 @@ fn move_columns(s: &mut Session, p: &Json) -> Result { for cell in touched { cx.touch(sheet, cell); } - rewrite(&mut cx.wb, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); + rewrite(cx, &name, &Edit::Move { from: source, to_row: 0, to_col: destination }); { let sh = cx.sheet_mut(sheet)?; sh.cells.shift_cols(delete_at + count, -(count as i64)); shift_sheet_features(sh, Axis::Cols, delete_at, count, false); } - rewrite(&mut cx.wb, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); + rewrite(cx, &name, &Edit::Delete { axis: Axis::Cols, at: delete_at, count }); *cx.sel = crate::Selection { active: CellRef::new(active_row, final_start), anchor: CellRef::new(active_row, final_start), @@ -514,7 +516,7 @@ fn insert_cells(s: &mut Session, p: &Json) -> Result { } shift_cell_features(sh, &e); let name = cx.wb.sheet(sheet).map(|s| s.name.clone()).unwrap_or_default(); - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); cx.structural = true; Ok(Json::Null) })?; @@ -549,7 +551,7 @@ fn delete_cells(s: &mut Session, p: &Json) -> Result { shift_cell_features(sh, &e); } // References into the deleted cells become #REF!; the rest move. - rewrite(&mut cx.wb, &name, &e); + rewrite(cx, &name, &e); cx.structural = true; Ok(Json::Null) })?; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 151c2bc06..23a09c9ee 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -206,6 +206,9 @@ pub struct DocState { /// workbook is opened from a file, carried by content pasted from such a workbook, and never /// changed by a trust decision. pub trust: Trust, + /// While a macro step runs: the structural edits its command made (the sheet's name, the + /// edit), so the macro's inputs, output and Repeat ranges follow their cells as formulas do. + pub edits: Option>, /// The references `edit.goTo` with `record: true` went to in this document, newest first, /// without names, each area written with its sheet (`Sheet1!B14:C22,Sheet1!F19:G30`). pub go_to_recent: Vec, @@ -299,6 +302,7 @@ impl DocState { content_kind: None, scripts_trusted, trust, + edits: None, go_to_recent: vec![], } } diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 8ae1e3496..36c4cae00 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -22,6 +22,7 @@ use std::collections::BTreeSet; use std::sync::Arc; use gridcraft_core::{CellRef, RangeRef, Value}; +use gridcraft_formula::adjust::Edit; use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, Sheet, Step, StepKind, Workbook}; use serde::Serialize; use serde_json::{Value as Json, json}; @@ -645,6 +646,34 @@ impl Machine { } } + /// After a step that inserted, deleted or moved sheets, rows, columns or cells, moves the + /// inputs, the output and the ranges of the Repeats in progress with their cells, as the + /// references in formulas move. A step that deletes one of them, or changes the size of a + /// range a Repeat is going over, stops the run. + fn follow(&mut self, before: &Workbook, after: &Workbook, edits: &[(String, Edit)]) -> std::result::Result<(), String> { + let same_sheets = before.sheets.len() == after.sheets.len() && before.sheets.iter().zip(&after.sheets).all(|(b, a)| b.name == a.name); + if same_sheets && edits.is_empty() { + return Ok(()); + } + let (pairs, _) = pair_sheets(before, after); + let follow = |what: &str, p: Place| follow_place(before, after, &pairs, edits, p).ok_or_else(|| format!("It deleted the cells of {what}.")); + for (name, place, _) in self.inputs.iter_mut() { + *place = follow(name, *place)?; + } + if let Some(o) = self.output { + self.output = Some(follow("the output anchor", o)?); + } + for f in self.frames.iter_mut() { + let Some(l) = f.repeat.as_mut() else { continue }; + let over = follow(&format!("the range Repeat {} goes over", f.prefix), l.over)?; + if (over.range.height(), over.range.width()) != (l.over.range.height(), l.over.range.width()) { + return Err(format!("It changed the size of the range Repeat {} goes over.", f.prefix)); + } + l.over = over; + } + Ok(()) + } + fn env<'a>(&'a self, wb: &'a Workbook) -> Env<'a> { Env { wb, active_sheet: wb.active_sheet, inputs: &self.inputs, output: self.output, item: self.item() } } @@ -670,9 +699,13 @@ impl Machine { (_, Policy::Allow | Policy::Ask(_)) => {} } self.last_params = Some(resolved.clone()); - let prev = self.guard.as_ref().and_then(|_| s.docs.iter().find(|d| d.uid == self.doc)).map(|d| d.wb.clone()); + let prev = s.docs.iter_mut().find(|d| d.uid == self.doc).map(|d| { + d.edits = Some(Vec::new()); + d.wb.clone() + }); let queued = s.ui_requests.len(); let r = s.execute(id, resolved); + let edits = s.docs.iter_mut().find(|d| d.uid == self.doc).and_then(|d| d.edits.take()).unwrap_or_default(); // Macros never open dialogs or web pages, and never start editing a cell; messages for // the person stay queued. let mut blocked = None; @@ -689,8 +722,11 @@ impl Machine { if let Some(b) = blocked { return Err(fail(b)); } + let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; + if let Some(prev) = &prev { + self.follow(prev, &after, &edits).map_err(fail)?; + } if let (Some(g), Some(prev)) = (self.guard.as_mut(), prev) { - let after = s.docs.iter().find(|d| d.uid == self.doc).map(|d| d.wb.clone()).ok_or_else(|| fail("The workbook was closed.".into()))?; let what = g.check(&prev, &after, &self.inputs); if !what.is_empty() { let msg = format!("It would write over data ({what})."); @@ -701,6 +737,34 @@ impl Machine { } } +/// Where the cells of `p` are after a step: on the same sheet wherever it moved (`pairs`, from +/// [`pair_sheets`]), and moved by each structural edit the step made there, or `None` when the +/// step deleted them (or their sheet). +fn follow_place(before: &Workbook, after: &Workbook, pairs: &[(usize, Option)], edits: &[(String, Edit)], p: Place) -> Option { + let sheet = pairs.iter().find(|(b, _)| *b == p.sheet).and_then(|(_, a)| *a)?; + let mut name = after.sheet(sheet)?.name.clone(); + let old_name = before.sheet(p.sheet).map(|s| s.name.as_str()).unwrap_or_default(); + let mut text = format!("{}!{}", resolve::quote_sheet(&name), p.range.a1()); + for (target, e) in edits { + if target != &name && target != old_name { + continue; + } + let expr = gridcraft_formula::parse(&text).ok()?; + text = gridcraft_formula::print(&gridcraft_formula::adjust::adjust(expr, target, target, e)); + if text.contains("#REF!") { + return None; + } + // A cut and paste can take the cells to another sheet. + let env = Env { wb: after, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + name = after.sheet(resolve::resolve_ref(&text, &env).ok()?.sheet)?.name.clone(); + } + let env = Env { wb: after, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + if text.contains('$') { + text = text.replace('$', ""); + } + resolve::resolve_ref(&text, &env).ok() +} + /// If's comparison of a cell's value with a step's `value`, the way Excel's `=`, `<`… compare. fn compare(v: &Value, op: CompareOp, want: &Json) -> bool { let empty = v.is_empty() || v.as_text() == Some(""); diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 262f50af9..68d0b428a 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -3,6 +3,7 @@ use std::sync::Arc; +use gridcraft_core::CellRef; use serde_json::{Value as Json, json}; use super::{s, v}; @@ -176,6 +177,58 @@ fn conflicts_ask_fail_or_overwrite() { assert_eq!(v(&s, "H1").display(), "400"); } +/// The value of `cell` on the sheet called `sheet`. +fn on(s: &Session, sheet: &str, cell: &str) -> String { + let wb = &s.doc().unwrap().wb; + wb.sheet(wb.sheet_index(sheet).unwrap()).unwrap().value(CellRef::parse(cell).unwrap()).display() +} + +#[test] +fn inputs_and_the_output_follow_their_cells_when_a_step_moves_them() { + // A sheet inserted in front: the input is still on Sheet1, not on whatever sheet is first now. + let mut s = s(); + let steps = json!([ + {"command": "home.insertSheet", "params": {"name": "Summary", "before": 0}}, + {"command": "cell.set", "params": {"cell": "Input", "input": "x"}} + ]); + put(&mut s, json!({"name": "Front", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "overwrite"}, "steps": steps})); + s.execute("macro.run", json!({"name": "Front", "inputs": "Sheet1!B2"})).unwrap(); + assert_eq!((on(&s, "Sheet1", "B2"), on(&s, "Summary", "B2")), ("x".into(), "".into())); + + // Rows inserted above: the input and the output move down with their cells. + let mut s = super::s(); + set(&mut s, "B5", "in"); + let steps = json!([ + {"command": "home.insertRows", "params": {"rows": "2:3"}}, + {"command": "cell.set", "params": {"cell": "Input", "input": "x"}}, + {"command": "cell.set", "params": {"cell": "Output", "input": "y"}} + ]); + put( + &mut s, + json!({"name": "Down", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"anchor": "D5", "onConflict": "overwrite"}, "steps": steps}), + ); + s.execute("macro.run", json!({"name": "Down", "inputs": "B5"})).unwrap(); + assert_eq!((v(&s, "B7").display(), v(&s, "D7").display()), ("x".into(), "y".into())); + assert_eq!((v(&s, "B5").display(), v(&s, "D5").display()), ("".into(), "".into())); + + // A step that deletes an input's cells stops the run. + let mut s = super::s(); + set(&mut s, "B5", "in"); + let steps = json!([{"command": "home.deleteRows", "params": {"rows": "5"}}, {"command": "cell.set", "params": {"cell": "Input", "input": "x"}}]); + put(&mut s, json!({"name": "Gone", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "overwrite"}, "steps": steps})); + let start = s.doc().unwrap().wb.clone(); + let err = s.execute("macro.run", json!({"name": "Gone", "inputs": "B5"})).unwrap_err().to_string(); + assert!(err.contains("Step 1 (home.deleteRows): It deleted the cells of Input."), "{err}"); + assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); + + // A step inside a Repeat that changes the size of the range it goes over stops the run. + let mut s = super::s(); + let body = json!([{"command": "home.insertRows", "params": {"rows": "2"}}]); + put(&mut s, json!({"name": "Grow", "output": {"onConflict": "overwrite"}, "steps": [{"type": "repeat", "over": "A1:A3", "body": body}]})); + let err = s.execute("macro.run", json!({"name": "Grow"})).unwrap_err().to_string(); + assert!(err.contains("Step 1.1 (home.insertRows): It changed the size of the range Repeat 1 goes over."), "{err}"); +} + #[test] fn each_step_is_checked_for_writes_over_data() { let dir = std::env::temp_dir().join(format!("gc-macro-guard-{}", std::process::id())); diff --git a/docs/macros.md b/docs/macros.md index daf0a2c35..184cc16ad 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -117,7 +117,10 @@ refused rather than read as the header. runs, which this version doesn't work out, so those rules stop the run as `fail` does. 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside - an If). + an If). When a step inserts, deletes or moves sheets, rows, columns or cells, the inputs, the + output and the range of a Repeat in progress move with their cells, as references in formulas + do; a step that deletes one of them, or changes the size of a range a Repeat is going over, + stops the run. ## Storage From 65cbcdd6bd96c415700c861510e66a4b6b6a284c Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:44:15 -0400 Subject: [PATCH 5/7] Engine: a macro run names the files it kept, refuses parameters it would ignore, and bounds its work - A run that rolls back after writing a file no longer says "Nothing was changed.": the message says the workbook was put back and names the step whose file stays (`StepError::wrote_files`, also after a Stop with rollback). - `macro.run` refuses an input the macro doesn't have (a misspelt input bound the selection), an input that isn't reference text, and `onConflict`, `overwrite`, `approve` or `timeLimit` of the wrong kind, which it used to ignore; `null` still counts as not given. - A step no longer copies itself, and Repeat and If no longer copy their bodies, on every pass: a body is copied once per run. A run passes its commands at most 256 MB of parameters, all steps together, which bounds a run's work on the web build, where there is no clock. - Every step except an approved file step runs inside the scripts' file sandbox, so a command that touches a file under a parameter other than `path` can't do so from a macro. - Docs: moving cells that hold data counts as writing over them. - Tests: `run_parameters_that_would_be_ignored_are_refused`, `a_run_passes_a_bounded_amount_of_parameters`, and the rollback message in `each_step_is_checked_for_writes_over_data`. --- crates/engine/src/io.rs | 2 +- crates/engine/src/macros/policy.rs | 13 ++- crates/engine/src/macros/run.rs | 132 ++++++++++++++++++++++++++--- crates/engine/src/tests/macros.rs | 44 ++++++++++ docs/macros.md | 17 ++-- 5 files changed, 189 insertions(+), 19 deletions(-) diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index b8ef1abe9..d47b78520 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -611,7 +611,7 @@ impl Drop for ScriptSandbox { /// thread is being torn down). fn script_sandbox_check(path: &str) -> Result<()> { if SCRIPT_SANDBOX.try_with(|c| c.get() > 0).unwrap_or(true) { - return Err(EngineError::Other(format!("{path}: scripts can't read or write files"))); + return Err(EngineError::Other(format!("{path}: scripts, and macro steps without the permission, can't read or write files"))); } Ok(()) } diff --git a/crates/engine/src/macros/policy.rs b/crates/engine/src/macros/policy.rs index 57d974920..24e7724ad 100644 --- a/crates/engine/src/macros/policy.rs +++ b/crates/engine/src/macros/policy.rs @@ -1,6 +1,8 @@ -//! What a macro step may run. Macros run commands, so this is the one place that keeps a macro -//! from saving, opening or closing files, driving the app, running other macros, or reading and -//! writing files without the person knowing. +//! What a macro step may run. Macros run commands, so this keeps a macro from saving, opening or +//! closing files, driving the app, running other macros, or reading and writing files without the +//! person knowing. It goes by command id and the `path` parameter; the runner also runs every +//! step except an approved file step inside the scripts' file sandbox, so a command that touches a +//! file under another parameter can't either. use serde_json::Value as Json; @@ -47,6 +49,11 @@ const READS_FILES: &[&str] = &["data.getData", "data.fromTextCsv", "insert.pictu /// File commands that only produce bytes unless given a `path` (then they write a file). const EXPORTS: &[&str] = &["file.exportPdf", "file.exportCsv", "file.exportHtml", "file.print", "file.printPreview", "file.saveBytes"]; +/// Whether command `id` with `params` writes a file (an export or print given a `path`). +pub fn writes_file(id: &str, params: &Json) -> bool { + EXPORTS.contains(&id) && params.get("path").is_some_and(|p| !p.is_null()) +} + /// The policy for running command `id` with `params` as a macro step in a document with `trust`. /// /// Any `path` other than `null` counts as touching a file: a token such as `{"$ref": "Input"}` diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index 36c4cae00..3d293e609 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -18,7 +18,7 @@ //! The steps run on a small machine with an explicit frame stack, so the debugger can stop //! between any two steps and pick up again on a later frame. -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use std::sync::Arc; use gridcraft_core::{CellRef, RangeRef, Value}; @@ -27,7 +27,7 @@ use gridcraft_model::{Cell, CompareOp, InputKind, Macro, OnConflict, RepeatOf, S use serde::Serialize; use serde_json::{Value as Json, json}; -use super::policy::{Policy, macro_policy}; +use super::policy::{Policy, macro_policy, writes_file}; use super::resolve::{self, Env, Place, ResolveError}; use crate::{EngineError, Result, Session, Trust, UiRequest}; @@ -38,6 +38,9 @@ pub const MAX_EXECUTED_STEPS: u64 = 100_000; pub const DEFAULT_TIME_LIMIT_MS: f64 = 60_000.0; /// The most cells an input may have for its content to be checked. pub const MAX_INPUT_CELLS: u64 = 1_000_000; +/// The most bytes of parameters one run passes to its commands, all steps together: on the web +/// build, where there is no clock, this and the step limit bound the work a run does. +pub const MAX_PARAM_BYTES: u64 = 256 * 1024 * 1024; /// The most ranges a list of cells gives per sheet (the counts stay exact). const MAX_LISTED_RANGES: usize = 500; @@ -58,11 +61,22 @@ pub struct StepError { /// The step would write over data and the conflict rule is ask: overwriting answers it. #[serde(skip_serializing_if = "std::ops::Not::not")] pub confirm: bool, + /// Files the run wrote before the step failed ("step 1 (file.exportCsv)"): rolling back the + /// workbook leaves them written. + #[serde(skip_serializing_if = "Vec::is_empty")] + pub wrote_files: Vec, } impl StepError { fn new(step: &str, command: Option<&str>, message: impl Into) -> StepError { - StepError { step: step.to_string(), command: command.map(str::to_string), message: message.into(), edge: false, confirm: false } + StepError { + step: step.to_string(), + command: command.map(str::to_string), + message: message.into(), + edge: false, + confirm: false, + wrote_files: Vec::new(), + } } fn resolve(step: &str, command: Option<&str>, e: ResolveError) -> StepError { @@ -79,11 +93,30 @@ impl StepError { } pub fn into_engine(self, name: &str) -> EngineError { - let text = format!("Macro '{name}' stopped. {} Nothing was changed.", self.describe()); + let text = format!("Macro '{name}' stopped. {} {}", self.describe(), rolled_back(&self.wrote_files)); if self.confirm { EngineError::Confirm(text) } else { EngineError::Other(text) } } } +/// What a run that rolled back leaves: "Nothing was changed.", or the files it wrote before. +pub fn rolled_back(wrote_files: &[String]) -> String { + match wrote_files { + [] => "Nothing was changed.".into(), + [one] => format!("The workbook was put back; the file {one} wrote stays."), + many => format!("The workbook was put back; the files {} wrote stay.", join_and(many)), + } +} + +/// The size of `v` as JSON text, near enough to bound how much a run passes to its commands. +fn json_bytes(v: &Json) -> u64 { + match v { + Json::String(t) => t.len() as u64 + 2, + Json::Array(a) => a.iter().map(json_bytes).fold(2, u64::saturating_add), + Json::Object(o) => o.iter().map(|(k, v)| (k.len() as u64 + 3).saturating_add(json_bytes(v))).fold(2, u64::saturating_add), + _ => 8, + } +} + // ---------------------------------------------------------------- gate /// Checks every enabled step against the policy: denied commands and undeclared file access are @@ -426,6 +459,13 @@ pub struct Machine { pub stopped: Option<(String, bool)>, /// The last step's resolved parameters (for the debugger). pub last_params: Option, + /// Files the run wrote ("step 1 (file.exportCsv)"): rolling the run back doesn't take them back. + pub wrote_files: Vec, + /// Repeat and If bodies by the path of their frame: a body is copied the first time its step + /// runs, not on every pass. + bodies: HashMap>>, + /// Bytes of parameters passed to commands so far, against [`MAX_PARAM_BYTES`]. + param_bytes: u64, /// Set unless the macro may overwrite: each step is checked for writes over data as it runs. guard: Option, } @@ -518,6 +558,9 @@ impl Machine { time_limit_ms: DEFAULT_TIME_LIMIT_MS, stopped: None, last_params: None, + wrote_files: Vec::new(), + bodies: HashMap::new(), + param_bytes: 0, guard: None, }; m.settle(); @@ -574,12 +617,15 @@ impl Machine { /// Runs the current step and moves to the next one. pub fn step(&mut self, s: &mut Session) -> std::result::Result<(), StepError> { - let Some((path, st)) = self.current().map(|(p, st)| (p, st.clone())) else { return Ok(()) }; + // The frame's steps are shared, so the step (and its bodies) isn't copied to run it. + let Some((path, steps, i)) = self.frames.last().map(|f| (child_path(&f.prefix, f.next), Arc::clone(&f.steps), f.next)) else { return Ok(()) }; + let Some(st) = steps.get(i) else { return Ok(()) }; if let Some(f) = self.frames.last_mut() { f.next += 1; } - let r = self.exec(s, &path, &st); - if r.is_err() { + let mut r = self.exec(s, &path, st); + if let Err(e) = r.as_mut() { + e.wrote_files.clone_from(&self.wrote_files); self.frames.clear(); } self.settle(); @@ -618,7 +664,8 @@ impl Machine { } if items > 0 && body.iter().any(|b| b.enabled) { let repeat = Some(Loop { of: *of, over: place, at: 0, count: items }); - self.frames.push(Frame { steps: Arc::new(body.clone()), next: 0, prefix: path.to_string(), repeat }); + let steps = self.body(path.to_string(), body); + self.frames.push(Frame { steps, next: 0, prefix: path.to_string(), repeat }); } Ok(()) } @@ -634,7 +681,9 @@ impl Machine { }; let (branch, name) = if compare(&v, *op, value) { (then, "then") } else { (otherwise, "else") }; if branch.iter().any(|b| b.enabled) { - self.frames.push(Frame { steps: Arc::new(branch.clone()), next: 0, prefix: format!("{path}.{name}"), repeat: None }); + let prefix = format!("{path}.{name}"); + let steps = self.body(prefix.clone(), branch); + self.frames.push(Frame { steps, next: 0, prefix, repeat: None }); } Ok(()) } @@ -674,6 +723,11 @@ impl Machine { Ok(()) } + /// The shared copy of the body that runs under `prefix`. + fn body(&mut self, prefix: String, body: &[Step]) -> Arc> { + Arc::clone(self.bodies.entry(prefix).or_insert_with(|| Arc::new(body.to_vec()))) + } + fn env<'a>(&'a self, wb: &'a Workbook) -> Env<'a> { Env { wb, active_sheet: wb.active_sheet, inputs: &self.inputs, output: self.output, item: self.item() } } @@ -698,13 +752,26 @@ impl Machine { } (_, Policy::Allow | Policy::Ask(_)) => {} } + self.param_bytes = self.param_bytes.saturating_add(json_bytes(&resolved)); + if self.param_bytes > MAX_PARAM_BYTES { + return Err(fail(format!( + "The macro passed more than {} MB of parameters to its steps and was stopped.", + MAX_PARAM_BYTES / (1024 * 1024) + ))); + } + let file_step = matches!(macro_policy(id, &resolved, self.trust), Policy::NeedsPermission(_)); + let writes = writes_file(id, &resolved); self.last_params = Some(resolved.clone()); let prev = s.docs.iter_mut().find(|d| d.uid == self.doc).map(|d| { d.edits = Some(Vec::new()); d.wb.clone() }); let queued = s.ui_requests.len(); + // Only an approved file step reaches the file system: any other step runs as a script's + // does, so a command that reads or writes a file under another parameter name can't. + let sandbox = (!file_step).then(crate::io::ScriptSandbox::enter); let r = s.execute(id, resolved); + drop(sandbox); let edits = s.docs.iter_mut().find(|d| d.uid == self.doc).and_then(|d| d.edits.take()).unwrap_or_default(); // Macros never open dialogs or web pages, and never start editing a cell; messages for // the person stay queued. @@ -719,6 +786,9 @@ impl Machine { } } r.map_err(|e| fail(e.to_string()))?; + if writes { + self.wrote_files.push(format!("step {path} ({id})")); + } if let Some(b) = blocked { return Err(fail(b)); } @@ -895,6 +965,7 @@ pub fn begin(s: &mut Session, name: &str, p: &Json) -> Result { .and_then(|i| d.wb.macros.get(i)) .cloned() .ok_or_else(|| EngineError::Other(format!("There is no macro called '{name}'.")))?; + check_params(p, &m)?; let asks = gate(&m, trust)?; approve(s, &m, doc, &asks, p.get("approve").and_then(Json::as_bool).unwrap_or(false))?; let prepared = prepare(s, &m, p.get("inputs"))?; @@ -929,7 +1000,7 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result { let name = &b.mac.name; machine.run_to_end(s).map_err(|e| e.into_engine(name))?; match machine.stopped { - Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} Nothing was changed."))), + Some((msg, true)) => Err(EngineError::Other(format!("Macro '{name}' stopped: {msg} {}", rolled_back(&machine.wrote_files)))), stopped => { let mut out = json!({"macro": name, "ran": machine.executed, "inputs": b.inputs}); if let Some((msg, _)) = stopped { @@ -941,6 +1012,47 @@ pub fn run(s: &mut Session, name: &str, p: &Json) -> Result { }) } +/// Refuses run parameters that would otherwise be ignored: an input the macro doesn't have, an +/// input given as something other than reference text, and `onConflict`, `overwrite`, `approve` +/// or `timeLimit` of the wrong kind (`null` counts as not given). +pub fn check_params(p: &Json, m: &Macro) -> Result<()> { + let refuse = |msg: String| Err(EngineError::Other(format!("Macro '{}': {msg}", m.name))); + let names = || m.inputs.iter().map(|i| i.name.clone()).collect::>().join(", "); + match p.get("inputs") { + None | Some(Json::Null) => {} + Some(Json::String(_)) if !m.inputs.is_empty() => {} + Some(Json::String(_)) => return refuse("it takes no inputs.".into()), + Some(Json::Object(o)) => { + for (k, v) in o { + if !m.inputs.iter().any(|i| i.name.eq_ignore_ascii_case(k)) { + return refuse(if m.inputs.is_empty() { + format!("it has no input called '{k}'; it takes no inputs.") + } else { + format!("it has no input called '{k}' (its inputs: {}).", names()) + }); + } + if !v.is_string() { + return refuse(format!("give the input {k} as reference text, such as \"B2:D9\".")); + } + } + } + Some(_) => return refuse("give `inputs` as reference text, such as \"B2:D9\", or as {\"Input\": \"B2:D9\"}.".into()), + } + let given = |k: &str| p.get(k).filter(|v| !v.is_null()); + if given("onConflict").is_some_and(|v| !v.is_string()) { + return refuse("`onConflict` must be ask, fail, overwrite, shiftDown or shiftRight.".into()); + } + for k in ["overwrite", "approve"] { + if given(k).is_some_and(|v| !v.is_boolean()) { + return refuse(format!("`{k}` must be true or false.")); + } + } + if given("timeLimit").is_some_and(|v| !v.as_f64().is_some_and(|t| t > 0.0)) { + return refuse("`timeLimit` must be a number of seconds above 0.".into()); + } + Ok(()) +} + /// The run's `timeLimit` (seconds) in milliseconds, or the default. fn time_limit(p: &Json) -> f64 { p.get("timeLimit").and_then(Json::as_f64).map(|t| (t * 1000.0).clamp(1.0, 600_000.0)).unwrap_or(DEFAULT_TIME_LIMIT_MS) diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index 68d0b428a..0df51122c 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -252,6 +252,10 @@ fn each_step_is_checked_for_writes_over_data() { (_, r) => panic!("{first} {rule}: {r:?}"), }; assert!(e.contains("Step 2 (cell.set): It would write over data (B1 on Sheet1)"), "{e}"); + // The workbook is put back; a file the run wrote stays, and the message says so. + let left = + if first == &export { "The workbook was put back; the file step 1 (file.exportCsv) wrote stays." } else { "Nothing was changed." }; + assert!(e.ends_with(left), "{e}"); assert_eq!(v(&s, "B1").display(), "keep", "{first} {rule}"); assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start), "{first} {rule}: rolled back"); } @@ -271,6 +275,46 @@ fn each_step_is_checked_for_writes_over_data() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn run_parameters_that_would_be_ignored_are_refused() { + let mut s = s(); + data(&mut s); + put(&mut s, totals()); + for (p, says) in [ + (json!({"inputs": {"Inptu": "B2:C5"}}), "it has no input called 'Inptu' (its inputs: Input)."), + (json!({"inputs": {"Input": 5}}), "give the input Input as reference text"), + (json!({"inputs": ["B2:C5"]}), "give `inputs` as reference text"), + (json!({"inputs": "B2:C5", "onConflict": 1}), "`onConflict` must be"), + (json!({"inputs": "B2:C5", "overwrite": "yes"}), "`overwrite` must be true or false."), + (json!({"inputs": "B2:C5", "approve": 1}), "`approve` must be true or false."), + (json!({"inputs": "B2:C5", "timeLimit": "30"}), "`timeLimit` must be a number of seconds above 0."), + (json!({"inputs": "B2:C5", "timeLimit": 0}), "`timeLimit` must be a number of seconds above 0."), + ] { + let mut p = p; + p["name"] = json!("Totals"); + let err = s.execute("macro.run", p.clone()).unwrap_err().to_string(); + assert!(err.contains(&format!("Macro 'Totals': {says}")), "{p}: {err}"); + } + put(&mut s, json!({"name": "None", "steps": []})); + let err = s.execute("macro.run", json!({"name": "None", "inputs": "A1"})).unwrap_err().to_string(); + assert!(err.contains("Macro 'None': it takes no inputs."), "{err}"); + // `null` is the same as not given, and an input's name matches in any case. + let p = json!({"name": "Totals", "inputs": {"input": "B2:C5"}, "onConflict": null, "overwrite": true, "timeLimit": null}); + s.execute("macro.run", p).unwrap(); +} + +#[test] +fn a_run_passes_a_bounded_amount_of_parameters() { + // A step with a 3 MB parameter in a Repeat of 100 rows would hand its command 300 MB; the run + // stops at 256 MB, which matters on the web build, where there is no time limit. + let mut s = s(); + let big = "x".repeat(3_000_000); + let body = json!([{"command": "home.bold", "params": {"range": "A1", "note": big}}]); + put(&mut s, json!({"name": "Big", "output": {"onConflict": "overwrite"}, "steps": [{"type": "repeat", "over": "A1:A100", "body": body}]})); + let err = s.execute("macro.run", json!({"name": "Big"})).unwrap_err().to_string(); + assert!(err.contains("passed more than 256 MB of parameters to its steps"), "{err}"); +} + #[test] fn the_answer_to_the_question_counts_however_ask_was_given() { let mut s = s(); diff --git a/docs/macros.md b/docs/macros.md index 184cc16ad..a3f18ebb6 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -60,8 +60,9 @@ macro interface. whole macro (4,194,304 characters of JSON). A macro read from a file that breaks any of these is left out with a warning; it is never changed to fit. When a macro runs, a Repeat whose range has more rows or columns than its `max` is refused, and a run executes at most 100,000 steps (each - pass through a Repeat counts) and takes at most 60 seconds (`timeLimit` to change the time; - the web build has no clock, so there only the step limit applies). + pass through a Repeat counts), passes its commands at most 256 MB of parameters, all steps + together, and takes at most 60 seconds (`timeLimit` to change the time; the web build has no + clock, so there only the step and parameter limits apply). ## Tokens @@ -107,17 +108,23 @@ refused rather than read as the header. including a blank one and a token, and the policy is checked again on the parameters as they are when the step runs. A macro that touches files, or that came with a workbook opened from a file, runs once the person approves it: the call fails with `approve: …` and runs with - `approve: true`. Approval lasts for the session, until the macro changes. + `approve: true`. Approval lasts for the session, until the macro changes. Every step other + than an approved file step runs inside the file sandbox that scripts use, so no other + parameter reaches a file either. 2. **Conflicts.** Unless the conflict rule is `overwrite`, each step is checked as it runs. A step that writes over cells outside the inputs that held data when the run started, or deletes sheets or objects that were there, stops the run, which rolls the workbook back (a file already - written stays). The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call + written stays). Moving such cells, as inserting or deleting rows or cells above them does, + counts as writing over them: the check compares cells by place. The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call fails with `confirm: …`; `overwrite: true` answers it), `fail` or `overwrite`. Shifting the data to make room (`shiftDown`, `shiftRight`) needs to know where the macro writes before it runs, which this version doesn't work out, so those rules stop the run as `fail` does. 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside - an If). When a step inserts, deletes or moves sheets, rows, columns or cells, the inputs, the + an If). The message ends "Nothing was changed.", or names the files the run already wrote, + which stay. An input the macro doesn't have, an input that isn't reference text, and + `onConflict`, `overwrite`, `approve` or `timeLimit` of the wrong kind are refused rather than + ignored. When a step inserts, deletes or moves sheets, rows, columns or cells, the inputs, the output and the range of a Repeat in progress move with their cells, as references in formulas do; a step that deletes one of them, or changes the size of a range a Repeat is going over, stops the run. From 3708c80168ac8023cc551ed0a7efc98c4d2f213b Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:49:35 -0400 Subject: [PATCH 6/7] Engine: macro.check says whether the run would go ahead; the question names what the answer covers - `ok` in `macro.check` meant only that the dry run hit no error. It now says whether `macro.run` with the same parameters would go ahead without a question, and `errors` says why not: a failing step, a Stop step that rolls back, data written over under `ask` or `fail`, a shift that can't make room (the refusals `make_room` makes, now in `shift_refusal`), or a run past the step or parameter limit once the dry runs before it count. `macro.check` takes `onConflict` and `overwrite` as `macro.run` does, reports the rule it used, and refuses the parameters `macro.run` refuses. - After a partial footprint, the question under `ask` says where the check stopped and that overwriting also lets the later steps write over data unchecked. - The dry runs and the run share the parameter budget as they share the step budget. - A dry run puts back the Go To list (`edit.goTo` with `record: true`). - The footprint groups the touch log by sheet once, instead of filtering a million entries once per sheet. - Docs: what `ok` means; RAND's sequence moves on after a check, as after any recalculation. - Tests: `the_check_says_whether_the_run_would_go_ahead`, `the_question_after_a_partial_footprint_says_what_the_answer_covers`, `a_dry_run_puts_back_the_go_to_list`, the shared parameter budget, and `ok` under `ask` in `conflicts_ask_fail_or_overwrite`. --- crates/engine/src/cmd/macros.rs | 2 +- crates/engine/src/cmd/mod.rs | 1 - crates/engine/src/macros/run.rs | 153 +++++++++++++++++++++++------- crates/engine/src/tests/macros.rs | 79 ++++++++++++++- docs/macros.md | 9 +- 5 files changed, 203 insertions(+), 41 deletions(-) diff --git a/crates/engine/src/cmd/macros.rs b/crates/engine/src/cmd/macros.rs index 53856e720..b33107d16 100644 --- a/crates/engine/src/cmd/macros.rs +++ b/crates/engine/src/cmd/macros.rs @@ -21,7 +21,7 @@ pub fn specs() -> Vec { "Check Macro", [], None, - "{name, inputs?, timeLimit?: seconds} validates the inputs and dry-runs the macro on the workbook (then puts everything back) → {ok, errors?, needsApproval?, inputs, output?, conflicts, footprint}", + "{name, inputs?, onConflict?, overwrite?, timeLimit?: seconds} validates the inputs and dry-runs the macro on the workbook (then puts everything back) → {ok (macro.run with these parameters would go ahead without a question), errors?, needsApproval?, onConflict, inputs, output?, conflicts, footprint}", has_doc, check ), diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 5e2afcddb..bcf77d4d0 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -414,7 +414,6 @@ pub(crate) fn commit(d: &mut DocState, f: impl FnOnce(&mut Ctx) -> Result) Ok(r) } -/// The most written cells a macro's footprint check records. /// The most cells a dry run's touch log keeps; a footprint whose log is full may be missing writes /// that kept a cell's value, so a shift is refused then. pub(crate) const TOUCH_LOG_LIMIT: usize = 1_000_000; diff --git a/crates/engine/src/macros/run.rs b/crates/engine/src/macros/run.rs index a6caaaafb..d9c714352 100644 --- a/crates/engine/src/macros/run.rs +++ b/crates/engine/src/macros/run.rs @@ -911,9 +911,15 @@ fn compare(v: &Value, op: CompareOp, want: &Json) -> bool { #[derive(Clone, Debug, Default, Serialize)] #[serde(rename_all = "camelCase")] pub struct Footprint { - /// Steps executed by the end of the dry run that made this footprint (the run's one budget). + /// Steps executed and bytes of parameters passed by the end of the dry run that made this + /// footprint (the run's one budget). #[serde(skip)] pub executed: u64, + #[serde(skip)] + pub param_bytes: u64, + /// The Stop step that ended the dry run asks for the run to be rolled back. + #[serde(skip)] + pub rolls_back: bool, /// The dry run wrote more cells than the touch log keeps, so writes that kept a cell's value /// may be missing from `cells`: a shift cannot trust the block. #[serde(skip_serializing_if = "std::ops::Not::not")] @@ -994,6 +1000,7 @@ struct Saved { sheet_selections: Vec, unlocked_ranges: Vec<(String, String)>, trust: Trust, + go_to_recent: Vec, clipboard: Option, prefs: crate::Prefs, mode: crate::Mode, @@ -1022,6 +1029,7 @@ impl Saved { sheet_selections: d.map(|d| d.sheet_selections.clone()).unwrap_or_default(), unlocked_ranges: d.map(|d| d.unlocked_ranges.clone()).unwrap_or_default(), trust: d.map_or(Trust::Local, |d| d.trust), + go_to_recent: d.map(|d| d.go_to_recent.clone()).unwrap_or_default(), clipboard: s.clipboard.clone(), prefs: s.prefs.clone(), mode: s.mode, @@ -1048,6 +1056,7 @@ impl Saved { d.sheet_selections = self.sheet_selections; d.unlocked_ranges = self.unlocked_ranges; d.trust = self.trust; + d.go_to_recent = self.go_to_recent; } s.active = self.active.min(s.docs.len().saturating_sub(1)); s.clipboard = self.clipboard; @@ -1070,8 +1079,9 @@ impl Saved { } /// Runs the macro in dry-run mode on its workbook, puts everything back, and says what it would -/// change. The dry run stops once `time_limit_ms` has passed on `clock`. -pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: f64, executed: u64) -> Result { +/// change. The dry run spends from `run`'s budget: its clock and time limit, its steps and the +/// bytes of parameters it passed. +pub fn footprint(s: &mut Session, p: &Prepared, run: &Machine) -> Result { let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?; s.set_active(i); let saved = Saved::take(s, p.doc); @@ -1081,7 +1091,7 @@ pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: d.touched = Some(Vec::new()); } let mut m = Machine::new(p, true); - (m.clock, m.time_limit_ms, m.executed) = (clock, time_limit_ms, executed); + (m.clock, m.time_limit_ms, m.executed, m.param_bytes) = (run.clock, run.time_limit_ms, run.executed, run.param_bytes); let outcome = m.run_to_end(s); let (after, touched) = match s.docs.iter_mut().find(|d| d.uid == p.doc) { Some(d) => (d.wb.clone(), d.touched.take().unwrap_or_default()), @@ -1093,7 +1103,8 @@ pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: let (after, touched) = if matches!(m.stopped, Some((_, true))) { (before.clone(), Vec::new()) } else { (after, touched) }; let mut fp = diff(&before, &after, &p.inputs, &touched); fp.log_full = touched.len() >= crate::cmd::TOUCH_LOG_LIMIT; - fp.executed = m.executed; + (fp.executed, fp.param_bytes) = (m.executed, m.param_bytes); + fp.rolls_back = matches!(m.stopped, Some((_, true))); fp.error = outcome.err(); fp.stopped = m.stopped.map(|(msg, _)| msg); fp.partial = m.partial.map(|(step, reason)| json!({"step": step, "reason": reason})); @@ -1104,6 +1115,11 @@ pub fn footprint(s: &mut Session, p: &Prepared, clock: Stopwatch, time_limit_ms: /// without changing them (a formula written again), so a shift moves everything the macro writes. fn diff(before: &Workbook, after: &Workbook, inputs: &[Bound], touched: &[(String, CellRef)]) -> Footprint { let mut fp = Footprint::default(); + // One pass over the touch log (up to a million writes), not one per sheet. + let mut by_sheet: HashMap<&str, BTreeSet> = HashMap::new(); + for (n, c) in touched { + by_sheet.entry(n.as_str()).or_default().insert(*c); + } let (pairs, added) = pair_sheets(before, after); fp.sheets_added = added.into_iter().filter_map(|a| after.sheet(a).map(|s| s.name.clone())).collect(); for (bi, ai) in pairs { @@ -1113,7 +1129,7 @@ fn diff(before: &Workbook, after: &Workbook, inputs: &[Bound], touched: &[(Strin continue; }; let Some(after_sheet) = after.sheets.get(ai) else { continue }; - let rewritten: BTreeSet = touched.iter().filter(|(n, _)| *n == after_sheet.name).map(|(_, c)| *c).collect(); + let rewritten = by_sheet.remove(after_sheet.name.as_str()).unwrap_or_default(); if Arc::ptr_eq(bs, after_sheet) && rewritten.is_empty() { continue; } @@ -1238,15 +1254,33 @@ pub(crate) fn ranges(cells: &mut [CellRef]) -> Vec { /// stays together. A shift that would move one of the macro's inputs is refused: the inputs were /// checked and bound where they are, and only the block's columns (or rows) would move. fn make_room(s: &mut Session, fp: &Footprint, p: &Prepared, down: bool) -> Result<()> { + let wb = s.doc()?.wb.clone(); + if let Some(why) = shift_refusal(&wb, fp, p, down) { + return Err(EngineError::Other(why)); + } + let env = Env { wb: &wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; + let moves: Vec = fp + .sheets + .iter() + .filter(|s| s.conflict_count > 0) + .filter_map(|sh| sh.bounds_range.map(|block| resolve::a1(Place { sheet: sh.index, range: block }, &env))) + .collect(); + for range in moves { + s.execute("home.insertCells", json!({"range": range, "shift": if down { "down" } else { "right" }}))?; + } + Ok(()) +} + +/// Why shifting the data can't make room for `fp`, or `None`: the macro deletes sheets or +/// objects, the footprint couldn't follow every write, or the shift would move an input. +fn shift_refusal(wb: &Workbook, fp: &Footprint, p: &Prepared, down: bool) -> Option { if !fp.sheets_removed.is_empty() || fp.objects_removed > 0 { - return Err(EngineError::Other("Shifting can't make room: the macro deletes sheets or objects.".into())); + return Some("Shifting can't make room: the macro deletes sheets or objects.".into()); } if fp.log_full { - return Err(EngineError::Other("Shifting can't make room: the macro writes more cells than the footprint can follow.".into())); + return Some("Shifting can't make room: the macro writes more cells than the footprint can follow.".into()); } - let wb = s.doc()?.wb.clone(); - let env = Env { wb: &wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; - let mut moves = Vec::new(); + let env = Env { wb, active_sheet: usize::MAX, inputs: &[], output: None, item: None }; for sh in fp.sheets.iter().filter(|s| s.conflict_count > 0) { let Some(block) = sh.bounds_range else { continue }; // What the insertion moves: the block's columns from its top row down, or its rows from @@ -1261,17 +1295,13 @@ fn make_room(s: &mut Session, fp: &Footprint, p: &Prepared, down: bool) -> Resul // The output is where the macro writes, so moving what is there is the point; the second // footprint catches data the shift moved into it. if let Some((name, pl)) = p.inputs.iter().map(|(n, pl, _)| (n.as_str(), *pl)).find(|(_, pl)| pl.sheet == sh.index && moved(&pl.range)) { - return Err(EngineError::Other(format!( + return Some(format!( "Shifting the data would move {name} ({}). Run with onConflict overwrite or fail, or make room first.", resolve::a1(pl, &env) - ))); + )); } - moves.push(resolve::a1(Place { sheet: sh.index, range: block }, &env)); } - for range in moves { - s.execute("home.insertCells", json!({"range": range, "shift": if down { "down" } else { "right" }}))?; - } - Ok(()) + None } // ---------------------------------------------------------------- run @@ -1319,9 +1349,9 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { if rule == OnConflict::Overwrite { return Ok((machine, false)); } - // The dry runs and the run share one step budget as they share one clock. - let mut fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?; - machine.executed = fp.executed; + // The dry runs and the run share one budget (steps, parameters) as they share one clock. + let mut fp = footprint(s, &b.prepared, &machine)?; + (machine.executed, machine.param_bytes) = (fp.executed, fp.param_bytes); if let Some(e) = fp.error.take() { return Err(e.into_engine(&m.name)); } @@ -1330,8 +1360,17 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { let what = fp.describe_conflicts(); match rule { OnConflict::Ask => { + // Overwriting answers for every step, also the ones a partial footprint didn't reach. + let unseen = fp.partial.as_ref().map_or(String::new(), |p| { + let text = |k: &str| p.get(k).and_then(Json::as_str).unwrap_or_default().to_string(); + format!( + " The check stopped at step {} ({}), so overwriting also lets the steps after it write over data unchecked.", + text("step"), + text("reason") + ) + }); return Err(EngineError::Confirm(format!( - "Macro '{}' would write over cells that hold data ({what}). Overwrite them, or run again with onConflict shiftDown, shiftRight or fail", + "Macro '{}' would write over cells that hold data ({what}).{unseen} Overwrite them, or run again with onConflict shiftDown, shiftRight or fail", m.name ))); } @@ -1340,8 +1379,8 @@ pub fn make_way(s: &mut Session, b: &Begun) -> Result<(Machine, bool)> { } OnConflict::ShiftDown | OnConflict::ShiftRight => { make_room(s, &fp, &b.prepared, rule == OnConflict::ShiftDown)?; - fp = footprint(s, &b.prepared, machine.clock, b.time_limit_ms, machine.executed)?; - machine.executed = fp.executed; + fp = footprint(s, &b.prepared, &machine)?; + (machine.executed, machine.param_bytes) = (fp.executed, fp.param_bytes); if let Some(e) = fp.error.take() { return Err(e.into_engine(&m.name)); } @@ -1457,12 +1496,21 @@ pub fn check(s: &mut Session, name: &str, p: &Json) -> Result { .cloned() .ok_or_else(|| EngineError::Other(format!("There is no macro called '{name}'.")))?; let mut out = json!({"macro": m.name, "ok": false, "onConflict": m.output.on_conflict.name()}); + let refused = |mut out: Json, e: String| { + out["errors"] = json!([e]); + Ok(out) + }; + if let Err(e) = check_params(p, &m) { + return refused(out, e.to_string()); + } + let rule = match conflict_rule(p, &m) { + Ok(r) => r, + Err(e) => return refused(out, e.to_string()), + }; + out["onConflict"] = json!(rule.name()); let asks = match gate(&m, trust) { Ok(a) => a, - Err(e) => { - out["errors"] = json!([e.to_string()]); - return Ok(out); - } + Err(e) => return refused(out, e.to_string()), }; let approved = s.macro_approvals.contains(&(doc, super::fingerprint(&m))); if !asks.is_empty() && !approved { @@ -1471,10 +1519,7 @@ pub fn check(s: &mut Session, name: &str, p: &Json) -> Result { } let prepared = match prepare(s, &m, p.get("inputs")) { Ok(pr) => pr, - Err(e) => { - out["errors"] = json!([e.to_string()]); - return Ok(out); - } + Err(e) => return refused(out, e.to_string()), }; out["inputs"] = prepared.inputs_json(&s.doc()?.wb); if let Some(o) = prepared.output { @@ -1484,11 +1529,47 @@ pub fn check(s: &mut Session, name: &str, p: &Json) -> Result { if !asks.is_empty() && !approved { return Ok(out); } - let fp = footprint(s, &prepared, Stopwatch::start(), time_limit(p), 0)?; - out["ok"] = json!(fp.error.is_none()); - out["conflicts"] = json!(fp.conflict_count()); + let mut budget = Machine::new(&prepared, false); + budget.time_limit_ms = time_limit(p); + let fp = footprint(s, &prepared, &budget)?; + // `ok`: the run would go ahead with this rule, without a question. + let mut problems = Vec::new(); if let Some(e) = &fp.error { - out["errors"] = json!([e.describe()]); + problems.push(e.describe()); + } + if let (true, Some(msg)) = (fp.rolls_back, &fp.stopped) { + problems.push(format!("A Stop step ends the run and rolls it back: {msg}")); + } + let shifts = matches!(rule, OnConflict::ShiftDown | OnConflict::ShiftRight); + if fp.conflict_count() > 0 { + let what = fp.describe_conflicts(); + match rule { + OnConflict::Ask => problems.push(format!("It would write over cells that hold data ({what}); the run asks first.")), + OnConflict::Fail => problems.push(format!("It would write over cells that hold data ({what}).")), + OnConflict::Overwrite => {} + OnConflict::ShiftDown | OnConflict::ShiftRight => { + if let Some(why) = shift_refusal(&s.doc()?.wb, &fp, &prepared, rule == OnConflict::ShiftDown) { + problems.push(why); + } + } + } + } + // The run spends its budget on the dry runs before it too: one, or two when it shifts. + let passes: u64 = match rule { + OnConflict::Overwrite => 1, + _ if shifts && fp.conflict_count() > 0 => 3, + _ => 2, + }; + if fp.executed.saturating_mul(passes) > MAX_EXECUTED_STEPS { + problems.push(format!("The run would execute more than {MAX_EXECUTED_STEPS} steps, counting the check before it ({} here).", fp.executed)); + } + if fp.param_bytes.saturating_mul(passes) > MAX_PARAM_BYTES { + problems.push(format!("The run would pass more than {} MB of parameters, counting the check before it.", MAX_PARAM_BYTES / (1024 * 1024))); + } + out["ok"] = json!(problems.is_empty()); + out["conflicts"] = json!(fp.conflict_count()); + if !problems.is_empty() { + out["errors"] = json!(problems); } out["footprint"] = serde_json::to_value(&fp).unwrap_or(Json::Null); Ok(out) diff --git a/crates/engine/src/tests/macros.rs b/crates/engine/src/tests/macros.rs index a9d6c6179..eaa4f527b 100644 --- a/crates/engine/src/tests/macros.rs +++ b/crates/engine/src/tests/macros.rs @@ -168,13 +168,22 @@ fn conflicts_ask_fail_or_overwrite() { let journal = s.journal.len(); let sel = s.doc().unwrap().selection.clone(); let c = s.execute("macro.check", json!({"name": "Totals", "inputs": "B2:C5"})).unwrap(); - assert_eq!(c["ok"], true); + // Under ask the run would ask first, so it wouldn't go ahead as it is. + assert_eq!((&c["ok"], &c["onConflict"]), (&json!(false), &json!("ask"))); + assert_eq!(c["errors"], json!(["It would write over cells that hold data (E3 on Sheet1); the run asks first."])); assert_eq!(c["conflicts"], 1); assert_eq!(c["output"], "E2"); assert_eq!(c["footprint"]["sheets"][0]["cells"], json!(["E2:E3"])); assert_eq!(c["footprint"]["sheets"][0]["conflicts"], json!(["E3"])); assert!(Arc::ptr_eq(&s.doc().unwrap().wb, &start)); assert_eq!((s.journal.len(), &s.doc().unwrap().selection), (journal, &sel)); + // With the answer given, or with the rule overwrite, it would. + for p in [json!({"overwrite": true}), json!({"onConflict": "overwrite"})] { + let mut p = p; + (p["name"], p["inputs"]) = (json!("Totals"), json!("B2:C5")); + let c = s.execute("macro.check", p.clone()).unwrap(); + assert_eq!((&c["ok"], &c["onConflict"], c.get("errors")), (&json!(true), &json!("overwrite"), None), "{p}"); + } // The answer to the question overwrites. s.execute("macro.run", json!({"name": "Totals", "inputs": "B2:C5", "overwrite": true})).unwrap(); @@ -520,6 +529,74 @@ fn a_run_passes_a_bounded_amount_of_parameters() { put(&mut s, json!({"name": "Big", "output": {"onConflict": "overwrite"}, "steps": [{"type": "repeat", "over": "A1:A100", "body": body}]})); let err = s.execute("macro.run", json!({"name": "Big"})).unwrap_err().to_string(); assert!(err.contains("passed more than 256 MB of parameters to its steps"), "{err}"); + // 50 rows are 150 MB: the run alone stays under the limit, the dry run and the run together + // go past it, as they share the step budget. + let body = json!([{"command": "home.bold", "params": {"range": "A1", "note": "x".repeat(3_000_000)}}]); + for (rule, ok) in [("overwrite", true), ("fail", false)] { + let repeat = json!({"type": "repeat", "over": "A1:A50", "body": body}); + put(&mut s, json!({"name": "Half", "output": {"onConflict": rule}, "steps": [repeat]})); + assert_eq!(s.execute("macro.run", json!({"name": "Half"})).is_ok(), ok, "{rule}"); + } +} + +#[test] +fn the_check_says_whether_the_run_would_go_ahead() { + let mut s = s(); + // A Stop step that rolls the run back. + put(&mut s, json!({"name": "Halt", "output": {"onConflict": "fail"}, "steps": [{"type": "stop", "message": "Not today.", "rollback": true}]})); + let c = s.execute("macro.check", json!({"name": "Halt"})).unwrap(); + assert_eq!((&c["ok"], &c["errors"]), (&json!(false), &json!(["A Stop step ends the run and rolls it back: Not today."]))); + // A shift that would move an input. + set(&mut s, "B1", "taken"); + set(&mut s, "B3", "in"); + let step = json!({"command": "cell.set", "params": {"cell": "B1", "input": "new"}}); + put(&mut s, json!({"name": "Move", "inputs": [{"name": "Input", "kind": "cell"}], "output": {"onConflict": "shiftDown"}, "steps": [step]})); + let c = s.execute("macro.check", json!({"name": "Move", "inputs": "B3"})).unwrap(); + assert_eq!(c["ok"], false); + assert!(c["errors"][0].as_str().unwrap().starts_with("Shifting the data would move Input (Sheet1!B3)."), "{c}"); + // A macro whose dry run takes 90,000 steps passes its own dry run, but the run spends the + // budget twice under fail, so the check says it wouldn't finish. + let spin = json!([{"type": "repeat", "over": "A1:A300", "body": [{"type": "repeat", "over": "A1:A300", "body": [{"type": "if", "cell": "A1", "op": "empty"}]}]}]); + for (rule, ok) in [("fail", false), ("overwrite", true)] { + put(&mut s, json!({"name": "Spin", "output": {"onConflict": rule}, "steps": spin})); + let c = s.execute("macro.check", json!({"name": "Spin"})).unwrap(); + assert_eq!(c["ok"], ok, "{rule}: {c}"); + assert_eq!(s.execute("macro.run", json!({"name": "Spin"})).is_ok(), ok, "{rule}: the run agrees"); + } + let c = s.execute("macro.check", json!({"name": "Spin", "onConflict": "fail"})).unwrap(); + assert!(c["errors"][0].as_str().unwrap().starts_with("The run would execute more than 100000 steps, counting the check before it"), "{c}"); + // Parameters the run would refuse, the check reports. + let c = s.execute("macro.check", json!({"name": "Spin", "onConflict": 5})).unwrap(); + assert!(c["errors"][0].as_str().unwrap().contains("`onConflict` must be"), "{c}"); +} + +#[test] +fn the_question_after_a_partial_footprint_says_what_the_answer_covers() { + let dir = std::env::temp_dir().join(format!("gc-macro-unseen-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let csv = dir.join("out.csv").to_string_lossy().to_string(); + let mut s = s(); + set(&mut s, "B1", "keep"); + set(&mut s, "C1", "also"); + let steps = json!([ + {"command": "cell.set", "params": {"cell": "B1", "input": "over"}}, + {"command": "file.exportCsv", "params": {"path": csv}}, + {"command": "cell.set", "params": {"cell": "C1", "input": "over"}} + ]); + put(&mut s, json!({"name": "Two", "permissions": ["file.exportCsv"], "steps": steps})); + let r = s.execute("macro.run", json!({"name": "Two", "approve": true})); + let Err(EngineError::Confirm(q)) = r else { panic!("{r:?}") }; + assert!(q.contains("(B1 on Sheet1). The check stopped at step 2 (file.exportCsv writes a file), so overwriting also lets the steps after it write over data unchecked."), "{q}"); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn a_dry_run_puts_back_the_go_to_list() { + let mut s = s(); + let step = json!({"command": "edit.goTo", "params": {"reference": "D4", "record": true}}); + put(&mut s, json!({"name": "Go", "steps": [step]})); + s.execute("macro.check", json!({"name": "Go"})).unwrap(); + assert_eq!(s.doc().unwrap().go_to_recent, Vec::::new()); } #[test] diff --git a/docs/macros.md b/docs/macros.md index 994006c85..c3a9c2198 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -135,8 +135,13 @@ refused rather than read as the header. do; a step that deletes one of them, or changes the size of a range a Repeat is going over, stops the run. -`macro.check {name, inputs?, timeLimit?}` reports the bound inputs, the output anchor, any -approval needed and the footprint, without changing anything. +`macro.check {name, inputs?, onConflict?, overwrite?, timeLimit?}` reports the bound inputs, the +output anchor, any approval needed and the footprint, and puts back everything the dry run +changed. `ok` says whether `macro.run` with the same parameters would go ahead without a question; +when it wouldn't, `errors` says why: the run would fail at a step, a Stop step rolls it back, it +would write over data under `ask` or `fail`, shifting can't make room, or the run would go past +the step or parameter limit counting the dry runs before it. The random numbers RAND gives move +on, as after any recalculation. ## Storage From 2cf5b808b3caec40dc4e5694092326d742ea8e5b Mon Sep 17 00:00:00 2001 From: prxnvt <53838599+prxnvt@users.noreply.github.com> Date: Sun, 25 Oct 2026 02:49:42 -0400 Subject: [PATCH 7/7] Docs: the macro page's conflict rules wrap at the page's width --- docs/macros.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/macros.md b/docs/macros.md index a3f18ebb6..c0a09890f 100644 --- a/docs/macros.md +++ b/docs/macros.md @@ -115,10 +115,11 @@ refused rather than read as the header. step that writes over cells outside the inputs that held data when the run started, or deletes sheets or objects that were there, stops the run, which rolls the workbook back (a file already written stays). Moving such cells, as inserting or deleting rows or cells above them does, - counts as writing over them: the check compares cells by place. The rule is `onConflict` (or the macro's `output.onConflict`): `ask` (the call - fails with `confirm: …`; `overwrite: true` answers it), `fail` or `overwrite`. Shifting the - data to make room (`shiftDown`, `shiftRight`) needs to know where the macro writes before it - runs, which this version doesn't work out, so those rules stop the run as `fail` does. + counts as writing over them: the check compares cells by place. The rule is `onConflict` (or + the macro's `output.onConflict`): `ask` (the call fails with `confirm: …`; `overwrite: true` + answers it), `fail` or `overwrite`. Shifting the data to make room (`shiftDown`, + `shiftRight`) needs to know where the macro writes before it runs, which this version doesn't + work out, so those rules stop the run as `fail` does. 3. **Run.** One undo step, "Run Macro: Totals". Any error rolls the whole run back and names the step: `Step 3.1 (cell.set): …` (paths are 1-based; `2.1` is inside a Repeat, `4.then.1` inside an If). The message ends "Nothing was changed.", or names the files the run already wrote,