Merge gh/main into chore/orch-beads

crates/improvise-command/src/command/cmd/cell.rs

@@ -9,7 +9,7 @@ mod tests {
     use crate::model::cell::{CellKey, CellValue};
 
     #[test]
-    fn clear_selected_cell_produces_clear_and_dirty() {
+    fn clear_selected_cell_produces_clear() {
         let mut m = two_cat_model();
         let key = CellKey::new(vec![
             ("Type".to_string(), "Food".to_string()),
@@ -23,7 +23,7 @@ mod tests {
             key: ctx.cell_key().clone().unwrap(),
         };
         let effects = cmd.execute(&ctx);
-        assert_eq!(effects.len(), 2);
+        assert_eq!(effects.len(), 1);
     }
 
     #[test]
@@ -65,7 +65,7 @@ mod tests {
         ]);
         let cmd = PasteCell { key };
         let effects = cmd.execute(&ctx);
-        assert_eq!(effects.len(), 2);
+        assert_eq!(effects.len(), 1);
         let dbg = effects_debug(&effects);
         assert!(dbg.contains("SetCell"), "Expected SetCell, got: {dbg}");
     }
@@ -86,13 +86,13 @@ mod tests {
     }
 
     #[test]
-    fn transpose_produces_transpose_and_dirty() {
+    fn transpose_produces_transpose() {
         let m = two_cat_model();
         let layout = make_layout(&m);
         let reg = make_registry();
         let ctx = make_ctx(&m, &layout, &reg);
         let effects = TransposeAxes.execute(&ctx);
-        assert_eq!(effects.len(), 2);
+        assert_eq!(effects.len(), 1);
         let dbg = effects_debug(&effects);
         assert!(
             dbg.contains("TransposeAxes"),
@@ -127,10 +127,7 @@ impl Cmd for ClearCellCommand {
         "clear-cell"
     }
     fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
-        vec![
-            Box::new(effect::ClearCell(self.key.clone())),
-            effect::mark_dirty(),
-        ]
+        vec![Box::new(effect::ClearCell(self.key.clone()))]
     }
 }
 
@@ -163,10 +160,7 @@ impl Cmd for PasteCell {
     }
     fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
         if let Some(value) = ctx.yanked.clone() {
-            vec![
-                Box::new(effect::SetCell(self.key.clone(), value)),
-                effect::mark_dirty(),
-            ]
+            vec![Box::new(effect::SetCell(self.key.clone(), value))]
         } else {
             vec![]
         }
@@ -182,7 +176,7 @@ impl Cmd for TransposeAxes {
         "transpose"
     }
     fn execute(&self, _ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
-        vec![Box::new(effect::TransposeAxes), effect::mark_dirty()]
+        vec![Box::new(effect::TransposeAxes)]
     }
 }
 

crates/improvise-command/src/command/cmd/commit.rs

@@ -269,7 +269,6 @@ fn commit_regular_cell_value(key: &CellKey, value: &str, effects: &mut Vec<Box<d
             CellValue::Text(value.to_string()),
         )));
     }
-    effects.push(effect::mark_dirty());
 }
 
 /// Stage a synthetic edit in drill state so it can be applied atomically on exit.
@@ -321,7 +320,6 @@ fn rename_record_coordinate(
             cell_value,
         )));
     }
-    effects.push(effect::mark_dirty());
 }
 
 /// Apply a records-mode edit directly to the model (no drill state).
@@ -359,7 +357,6 @@ fn add_measure(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
         category: MEASURE_CATEGORY.to_string(),
         item: name.to_string(),
     }));
-    effects.push(effect::mark_dirty());
 }
 
 /// Typing a name into the `+cat` column creates that category, which gives
@@ -382,7 +379,6 @@ fn add_category(value: &str, effects: &mut Vec<Box<dyn Effect>>) {
         return;
     }
     effects.push(Box::new(effect::AddCategory(name.to_string())));
-    effects.push(effect::mark_dirty());
 }
 
 fn commit_cell_value(
@@ -651,7 +647,6 @@ impl Cmd for CommitBuffer {
             return vec![effect::change_mode(self.kind.on_empty())];
         }
         let mut effects: Vec<Box<dyn Effect>> = vec![
-            effect::mark_dirty(),
             effect::set_status(self.kind.status(name)),
             // The add goes last on purpose: it is the only party that knows
             // whether it worked, so the optimistic status above must not

crates/improvise-command/src/command/cmd/effect_cmds.rs

@@ -352,10 +352,7 @@ effect_cmd!(
     "set-format",
     |args: &[String]| require_args("set-format", args, 1),
     |args: &Vec<String>, _ctx: &CmdContext| -> Vec<Box<dyn Effect>> {
-        vec![
-            Box::new(effect::SetNumberFormat(args.join(" "))),
-            effect::mark_dirty(),
-        ]
+        vec![Box::new(effect::SetNumberFormat(args.join(" ")))]
     }
 );
 

crates/improvise-command/src/command/cmd/fit.rs

@@ -145,7 +145,6 @@ impl Cmd for FitMeasure {
             // screen rather than this command's optimistic one (gotcha 20).
             Ok(outcome) => vec![
                 effect::set_status(outcome.status_line()),
-                Box::new(effect::MarkDirty),
                 Box::new(effect::AddFormula {
                     raw: outcome.formula.raw.clone(),
                     target_category: outcome.formula.target_category.clone(),

crates/improvise-command/src/command/cmd/grid.rs

@@ -222,13 +222,10 @@ impl Cmd for ToggleGroupAtCursor {
         let Some((cat, group)) = lookup else {
             return vec![];
         };
-        vec![
-            Box::new(effect::ToggleGroup {
-                category: cat,
-                group,
-            }),
-            effect::mark_dirty(),
-        ]
+        vec![Box::new(effect::ToggleGroup {
+            category: cat,
+            group,
+        })]
     }
 }
 
@@ -260,13 +257,10 @@ impl Cmd for HideSelectedRowItem {
             return vec![];
         };
         let item_name = items[0].clone();
-        vec![
-            Box::new(effect::HideItem {
-                category: cat_name,
-                item: item_name,
-            }),
-            effect::mark_dirty(),
-        ]
+        vec![Box::new(effect::HideItem {
+            category: cat_name,
+            item: item_name,
+        })]
     }
 }
 
@@ -512,7 +506,6 @@ impl Cmd for AddRecordRow {
         let key = crate::model::cell::CellKey::new(coords);
         vec![
             Box::new(effect::SetCell(key, CellValue::Number(0.0))),
-            effect::mark_dirty(),
             effect::set_status("Added new record row"),
         ]
     }

crates/improvise-command/src/command/cmd/panel.rs

@@ -494,13 +494,10 @@ impl Cmd for DeleteFormulaAtCursor {
         let cursor = ctx.formula_cursor.min(formulas.len().saturating_sub(1));
         if cursor < formulas.len() {
             let f = &formulas[cursor];
-            let mut effects: Vec<Box<dyn Effect>> = vec![
-                Box::new(effect::RemoveFormula {
-                    target: f.target.clone(),
-                    target_category: f.target_category.clone(),
-                }),
-                effect::mark_dirty(),
-            ];
+            let mut effects: Vec<Box<dyn Effect>> = vec![Box::new(effect::RemoveFormula {
+                target: f.target.clone(),
+                target_category: f.target_category.clone(),
+            })];
             if cursor > 0 {
                 effects.push(Box::new(effect::SetPanelCursor {
                     panel: Panel::Formula,
@@ -619,7 +616,6 @@ impl Cmd for DeleteCategoryAtCursor {
             Some(CatTreeEntry::Category { name, .. }) => {
                 vec![
                     Box::new(effect::RemoveCategory(name.clone())),
-                    effect::mark_dirty(),
                     effect::set_status(format!("Deleted category '{name}'")),
                 ]
             }
@@ -632,7 +628,6 @@ impl Cmd for DeleteCategoryAtCursor {
                         category: cat_name.clone(),
                         item: item_name.clone(),
                     }),
-                    effect::mark_dirty(),
                     effect::set_status(format!("Deleted item '{item_name}' from '{cat_name}'")),
                 ]
             }
@@ -687,7 +682,6 @@ impl Cmd for CreateAndSwitchView {
         vec![
             Box::new(effect::CreateView(name.clone())),
             Box::new(effect::SwitchView(name)),
-            effect::mark_dirty(),
             effect::change_mode(AppMode::Normal),
         ]
     }
@@ -703,10 +697,8 @@ impl Cmd for DeleteViewAtCursor {
     fn execute(&self, ctx: &CmdContext) -> Vec<Box<dyn Effect>> {
         let view_names: Vec<String> = ctx.workbook.views.keys().cloned().collect();
         if let Some(name) = view_names.get(ctx.view_panel_cursor) {
-            let mut effects: Vec<Box<dyn Effect>> = vec![
-                Box::new(effect::DeleteView(name.clone())),
-                effect::mark_dirty(),
-            ];
+            let mut effects: Vec<Box<dyn Effect>> =
+                vec![Box::new(effect::DeleteView(name.clone()))];
             if ctx.view_panel_cursor > 0 {
                 effects.push(Box::new(effect::SetPanelCursor {
                     panel: Panel::View,

crates/improvise-command/src/command/cmd/tile.rs

@@ -131,11 +131,7 @@ impl Cmd for TileAxisOp {
                 None => Box::new(effect::CycleAxis(name.to_string())),
             };
             let status = format!("{} → {}", name, new_axis.plain_label());
-            vec![
-                axis_effect,
-                effect::mark_dirty(),
-                effect::set_status(status),
-            ]
+            vec![axis_effect, effect::set_status(status)]
         } else {
             vec![]
         }

crates/improvise-command/src/ui/app.rs

@@ -177,6 +177,21 @@ pub struct App {
     /// "advance at bottom-right" short-circuits the trailing
     /// `EnterEditAtCursor` in a `CommitAndAdvance` chain.
     pub abort_effects: bool,
+    /// Whether the **last** `apply_effects` batch stopped early.
+    ///
+    /// `abort_effects` is transient by design — it is reset the moment the
+    /// loop breaks, so nothing outside `apply_effects` can ever observe it —
+    /// and that is right for the mechanism but leaves the *outcome*
+    /// unreadable. A caller that ran a one-effect batch and needs to know
+    /// whether that effect refused has no other way to ask: `Effect::apply`
+    /// returns `()` on purpose, so an effect reports by writing the `App`.
+    ///
+    /// Its one consumer is `Host::save`, where the batch is literally
+    /// `[Save]` and so "the batch stopped" and "the save failed" are the same
+    /// fact (gotcha 21, improvise-368h). **Naming the real call site matters**
+    /// because that identity is what makes the flag readable at all: over a
+    /// longer batch it says that *something* refused, never which.
+    batch_aborted: bool,
     /// **Session-local**: how many times the whole document has been
     /// replaced. Bumped by `install_workbook` and by nothing else, so a
     /// client mirror can tell that the document it was attached to is gone
@@ -258,6 +273,7 @@ impl App {
             term_height: 24,
             layout,
             abort_effects: false,
+            batch_aborted: false,
             document_generation: 0,
             layout_version: 0,
             cursor_epoch: 0,
@@ -301,6 +317,14 @@ impl App {
         self.document_generation += 1;
     }
 
+    /// Did the last [`App::apply_effects`] batch stop before the end?
+    ///
+    /// See the field. The one place this is a *complete* answer is a
+    /// one-effect batch.
+    pub fn last_batch_aborted(&self) -> bool {
+        self.batch_aborted
+    }
+
     /// How many times this app's document has been replaced. A session that
     /// was seeded at an earlier generation is holding another document.
     pub fn document_generation(&self) -> u64 {
@@ -558,6 +582,7 @@ impl App {
     /// aborted batch must not claim the effects it never ran.
     pub fn apply_effects(&mut self, effects: Vec<Box<dyn super::effect::Effect>>) -> Dirty {
         self.abort_effects = false;
+        self.batch_aborted = false;
         // The two lower basis tiers are read off the same `EffectKind` the
         // `Dirty` is (ADR 001, improvise-xwi6) — one classification with two
         // readers in one place, never a second table to drift from gotcha
@@ -567,6 +592,35 @@ impl App {
         // reason the `Dirty` is: an aborted batch must not claim the effects
         // it never ran (gotcha 21).
         //
+        // `model_state.dirty` is read there too, and for the reason the other
+        // two are: it is a fact about the document, so the classification the
+        // effect already carries is the only honest source for it. It used to
+        // be a separate `MarkDirty` effect that each *command* composed
+        // alongside its mutation — reasonable on the key path and unreachable
+        // from anywhere else, because improvise-kmgs derived `ModelCommand`
+        // from the effects one at a time and so put `SetCell` on the wire
+        // without its companion (improvise-gv9r). A derivation correct per
+        // element and wrong for a set. Raising it here instead makes it
+        // unrepresentable rather than validated: there is no way to compose a
+        // document change that omits it.
+        //
+        // All three `ModelChange` levels count, `WholeWorkbook` included. A
+        // replaced document is not the document `file_path` names, whether it
+        // arrived from the import wizard or from a `LoadModel` — which does
+        // not move `file_path` — so in both cases the bytes on disk and the
+        // bytes in memory have parted company, which is all `dirty` claims.
+        //
+        // `Save` is `ViewOnly` and clears the flag itself, so a batch that
+        // edits and then saves ends clean: the order in the loop is the
+        // order the effects were composed in.
+        //
+        // It over-approximates in the same direction and for the same reason
+        // `layout_version` does: an effect that *declares* a document change
+        // and turns out to do nothing — a `LoadModel` that cannot read its
+        // path, an `AcceptWizardModel` with nothing built — leaves the flag
+        // raised. That costs one redundant write. The other direction costs
+        // an edit.
+        //
         // `record_versions` cannot be, because an `EffectKind` says *that* a
         // batch writes cells, never *which* — so the records are read off
         // the data itself, fingerprinted around the batch. Two consequences
@@ -585,14 +639,19 @@ impl App {
             let kind = effect.kind();
             effect.apply(self);
             dirty = dirty.union(Dirty::of(kind));
+            if matches!(kind, EffectKind::Model(_)) {
+                self.model_state.dirty = true;
+            }
             if matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry)) {
                 self.layout_version += 1;
             }
             if self.abort_effects {
                 // AbortChain (or another abort-setting effect) requested
                 // that the rest of this batch be skipped. Reset the flag so
-                // the next dispatch starts clean.
+                // the next dispatch starts clean, and record that it
+                // happened where a caller can still read it.
                 self.abort_effects = false;
+                self.batch_aborted = true;
                 break;
             }
         }
@@ -3072,7 +3131,8 @@ mod basis_tiers {
     use super::*;
     use crate::model::cell::CellValue;
     use crate::ui::effect::{
-        AbortChain, ApplyAndClearDrill, Effect, SetAxis, SetCell, SetDrillPendingEdit, StartDrill,
+        AbortChain, ApplyAndClearDrill, Effect, SetAxis, SetCell, SetDrillPendingEdit, SetSelected,
+        SetStatus, StartDrill,
     };
     use crate::view::layout::RecordRow;
     use crate::view::{Axis, RecordCol};
@@ -3151,6 +3211,76 @@ mod basis_tiers {
         }
     }
 
+    /// **A loaded document is dirty, and that is a behaviour change with a
+    /// reason** (improvise-gv9r, raised by the independent review of PR #26).
+    ///
+    /// `LoadModel` is `Model(WholeWorkbook)`, so `apply_effects` raises the
+    /// flag — where before this branch `:e other.improv` left the document
+    /// reporting clean. Clean was the wrong answer: `LoadModel` does **not**
+    /// move `model_state.file_path`, so after it the in-memory document is a
+    /// different one from the file the path names, and the bytes on disk and
+    /// the bytes in memory really have parted company. That is all `dirty`
+    /// claims.
+    ///
+    /// It is worth a test rather than a comment because of what it gates:
+    /// `autosave_if_needed` and the `:q` refusal both read this flag, so a
+    /// user who loads and quits is now asked rather than silently dropping a
+    /// document whose path is stale. `:q!` still discards.
+    ///
+    /// The path-staleness itself is pre-existing and is reported separately;
+    /// this pins only that the flag follows from it.
+    #[test]
+    fn a_loaded_document_is_dirty_because_its_path_still_names_the_old_one() {
+        use crate::storage::MemoryFs;
+
+        let storage = MemoryFs::new();
+        let mut other = Workbook::new("Other");
+        other.add_category("Product").unwrap();
+        crate::persistence::save_to(&storage, &other, std::path::Path::new("other.improv"))
+            .expect("the fixture document must save");
+
+        let mut app = App::new(Workbook::new("Test"), Some("here.improv".into()));
+        app.storage = Box::new(storage);
+        app.rebuild_layout();
+        assert!(!app.model_state.dirty, "the fixture starts clean");
+
+        app.apply_effects(vec![Box::new(crate::ui::effect::LoadModel(
+            "other.improv".into(),
+        ))]);
+
+        assert_eq!(
+            app.model_state.workbook.model.name, "Other",
+            "the fixture must actually load, or the flag proves nothing"
+        );
+        assert_eq!(
+            app.model_state.file_path,
+            Some("here.improv".into()),
+            "LoadModel does not move the path — which is exactly why the \
+             document is now out of step with it"
+        );
+        assert!(app.model_state.dirty);
+    }
+
+    /// The negative arm of improvise-gv9r's raise, and it must be aimed at
+    /// `apply_effects` rather than at a `Command`: a view-slice command is
+    /// folded straight into `ViewState` and never reaches this loop, so a
+    /// negative written against the session door would pass however
+    /// unconditional the raise was. It is a `ViewOnly` effect **run through
+    /// the loop** that discriminates.
+    ///
+    /// What it pins: a client's cursor, status line or panel does not put
+    /// the document out of step with the bytes on disk, so it must not
+    /// arm an unsaved-changes prompt or an autosave.
+    #[test]
+    fn a_view_only_batch_leaves_the_document_clean() {
+        let mut app = app_with_two_records();
+        app.apply_effects(vec![
+            Box::new(SetStatus("hello".into())),
+            Box::new(SetSelected(1, 0)),
+        ]);
+        assert!(!app.model_state.dirty);
+    }
+
     /// Tier 3 moves for the record written and for no other, and tier 2
     /// stays put: a cell edit does not invalidate batches that merely share
     /// the document.

crates/improvise-command/src/ui/effect.rs

@@ -590,17 +590,6 @@ impl Effect for ReturnToNormalUnlessMoved {
 
 simple_effect!(SetStatus(String) => |app| app.view_state.status_msg, EffectKind::ViewOnly);
 
-#[derive(Debug)]
-pub struct MarkDirty;
-impl Effect for MarkDirty {
-    fn apply(&self, app: &mut App) {
-        app.model_state.dirty = true;
-    }
-    fn kind(&self) -> EffectKind {
-        EffectKind::ViewOnly
-    }
-}
-
 simple_effect!(SetYanked(Option<CellValue>) => |app| app.view_state.yanked, EffectKind::ViewOnly);
 
 simple_effect!(SetSearchQuery(String) => |app| app.view_state.search_query, EffectKind::ViewOnly);
@@ -675,7 +664,11 @@ impl Effect for ApplyAndClearDrill {
                 RecordCol::NewMeasure | RecordCol::NewCategory => {}
             }
         }
-        app.model_state.dirty = true;
+        // No `dirty = true` here: this declares `Model(CellData)` and
+        // `App::apply_effects` raises the flag off that (improvise-gv9r). The
+        // line that used to be here was the last survivor of `MarkDirty` —
+        // harmless, and a second home for the one thing that commit set out
+        // to give exactly one.
     }
     fn kind(&self) -> EffectKind {
         EffectKind::Model(ModelChange::CellData)
@@ -899,7 +892,6 @@ impl Effect for AcceptWizardModel {
         if let Some((workbook, warnings)) = built {
             app.install_workbook(workbook);
             app.view_state.formula_cursor = 0;
-            app.model_state.dirty = true;
             app.view_state.status_msg = if warnings.is_empty() {
                 "Import successful! Press :w <path> to save.".to_string()
             } else {
@@ -1205,10 +1197,6 @@ impl Effect for CleanEmptyRecords {
 
 // ── Convenience constructors ─────────────────────────────────────────────────
 
-pub fn mark_dirty() -> Box<dyn Effect> {
-    Box::new(MarkDirty)
-}
-
 pub fn set_status(msg: impl Into<String>) -> Box<dyn Effect> {
     Box::new(SetStatus(msg.into()))
 }
@@ -1381,7 +1369,6 @@ mod tests {
         let cases: Vec<Box<dyn Effect>> = vec![
             Box::new(ChangeMode(AppMode::Help)),
             Box::new(SetStatus("hi".into())),
-            Box::new(MarkDirty),
             Box::new(SetYanked(Some(CellValue::Number(1.0)))),
             Box::new(SetSearchQuery("q".into())),
             Box::new(SetSearchMode(true)),
@@ -2216,14 +2203,6 @@ mod tests {
         assert_eq!(app.view_state.status_msg, "hello");
     }
 
-    #[test]
-    fn mark_dirty_effect() {
-        let mut app = test_app();
-        assert!(!app.model_state.dirty);
-        MarkDirty.apply(&mut app);
-        assert!(app.model_state.dirty);
-    }
-
     #[test]
     fn set_yanked_effect() {
         let mut app = test_app();
@@ -2349,9 +2328,23 @@ mod tests {
         assert!(app.drill_state.is_some());
 
         // Apply with no pending edits — should just clear state
-        ApplyAndClearDrill.apply(&mut app);
+        // Through `apply_effects`, not `apply`: the dirty flag is raised
+        // off the `EffectKind` now (improvise-gv9r), so driving the effect
+        // directly would assert a flag nothing in production sets.
+        app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
         assert!(app.drill_state.is_none());
-        assert!(!app.model_state.dirty); // no edits → not dirty
+        assert!(
+            app.model_state.dirty,
+            "a drill exited with no edits still marks the document dirty, \
+             because `apply_effects` raises the flag off the declared \
+             `EffectKind` and this effect declares `Model(CellData)` whether \
+             or not it wrote anything (improvise-gv9r). That is the same \
+             over-approximation a failed `LoadModel` gets, in the same \
+             direction: one redundant write rather than a lost edit. It has \
+             been true since the flag moved; this test could not see it \
+             because it drove `apply` directly and so measured the effect in \
+             isolation rather than the path production takes."
+        );
     }
 
     /// A staged edit to a measure column writes that one cell (improvise-c5v).
@@ -2380,7 +2373,10 @@ mod tests {
         }
         .apply(&mut app);
 
-        ApplyAndClearDrill.apply(&mut app);
+        // Through `apply_effects`, not `apply`: the dirty flag is raised
+        // off the `EffectKind` now (improvise-gv9r), so driving the effect
+        // directly would assert a flag nothing in production sets.
+        app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
         assert!(app.drill_state.is_none());
         assert!(app.model_state.dirty);
         assert_eq!(
@@ -2425,7 +2421,10 @@ mod tests {
         }
         .apply(&mut app);
 
-        ApplyAndClearDrill.apply(&mut app);
+        // Through `apply_effects`, not `apply`: the dirty flag is raised
+        // off the `EffectKind` now (improvise-gv9r), so driving the effect
+        // directly would assert a flag nothing in production sets.
+        app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
         assert!(app.model_state.dirty);
         let renamed = |measure: &str| {
             CellKey::new(vec![
@@ -2483,7 +2482,10 @@ mod tests {
         }
         .apply(&mut app);
 
-        ApplyAndClearDrill.apply(&mut app);
+        // Through `apply_effects`, not `apply`: the dirty flag is raised
+        // off the `EffectKind` now (improvise-gv9r), so driving the effect
+        // directly would assert a flag nothing in production sets.
+        app.apply_effects(vec![Box::new(ApplyAndClearDrill)]);
         assert_eq!(app.model_state.workbook.model.get_cell(&key), None);
     }
 

crates/improvise-command/src/ui/host.rs

@@ -45,8 +45,10 @@
 //! ADR describes the starting point of.
 
 use crate::ui::app::App;
+use crate::ui::effect;
 use crate::ui::project::{Applied, Broadcast, Dirty, SessionId, Sessions};
-use improvise_protocol::Command;
+use crate::ui::reduce::{Checked, CheckedBatch};
+use improvise_protocol::{Command, Rejection};
 
 use improvise_protocol::{ClientFrame, ServerFrame};
 
@@ -157,9 +159,48 @@ impl Host {
             // compose the command itself — see `improvise-wasm-client::keys`,
             // which carries the list of what is still upstream.
             ClientFrame::Key { .. } => Vec::new(),
+            ClientFrame::Save => self.save(id),
         }
     }
 
+    /// Write the document back, because a client asked (improvise-368h).
+    ///
+    /// **No session state moves and nobody else is told**, which is why this
+    /// does not go through `Sessions` at all: saving changes no document
+    /// state and no client's view slice, so there is no mirror to adopt and
+    /// nothing to broadcast. What moved is bytes in *this host's* storage —
+    /// the same fact that makes `effect::Save` `EffectKind::ViewOnly` and the
+    /// reason the `ModelCommand` derivation could never reach it (gotchas 3,
+    /// 38). Success is silent, on the same terms as any accepted command that
+    /// dirties no widget.
+    ///
+    /// Both refusals are the host's to answer, not `Checked`'s: neither is
+    /// about a basis. The missing path is decided **before** anything runs,
+    /// because a save that cannot land should be refused rather than
+    /// attempted and then reported. The failed write can only be found by
+    /// trying, and `Save` signals it by aborting its batch (gotcha 21) — the
+    /// batch here is exactly `[Save]`, so an abort is that failure and
+    /// nothing else. Reporting a failed write as success is the thing this
+    /// exists to stop.
+    fn save(&mut self, id: SessionId) -> Vec<Addressed> {
+        let refused = match self.app.model_state.file_path.is_none() {
+            true => Some(Rejection::NoDocumentPath),
+            false => {
+                self.app.apply_effects(vec![Box::new(effect::Save)]);
+                self.app
+                    .last_batch_aborted()
+                    .then_some(Rejection::SaveFailed)
+            }
+        };
+        refused
+            .map(|reason| Addressed {
+                to: id,
+                msg: ServerFrame::Rejected(reason),
+            })
+            .into_iter()
+            .collect()
+    }
+
     /// A resize dirties no document state at all — but it moves this
     /// client's viewport, and `ClientSession::project` emits the grid when
     /// the viewport moved even under `Dirty::NOTHING`. So the fan-out is the
@@ -175,33 +216,37 @@ impl Host {
         self.project(Dirty::NOTHING)
     }
 
-    /// A batch, applied in order and **abandoned at the first rejection**.
+    /// A batch: **answered whole, then applied whole** (improvise-955n).
     ///
     /// Order is why a batch exists at all: one keypress resolved on a client
     /// is `SetSelected` plus the scroll offsets that follow it, and a host
     /// that folded the selection but not the offset projects that client the
-    /// wrong window. Stopping at a rejection rather than skipping past it is
-    /// the conservative reading of ADR 001 — the commands after a refused one
-    /// were composed against a picture that has now been contradicted, so
-    /// applying them is inventing a basis nobody claimed.
+    /// wrong window.
     ///
-    /// It does leave a hole ADR 001's "not even partially" does not have:
-    /// a batch's *earlier* commands stay applied. No client written against
-    /// this workspace can reach it — the locally-resolved batches are
-    /// view-only and a `Command::Model` is composed one at a time — so it is
-    /// filed rather than closed here, because closing it properly means the
-    /// check moving to the batch and that is a different ticket.
+    /// It used to apply in order and abandon at the first rejection, which
+    /// left a hole ADR 001's "not even partially" does not have: a refused
+    /// frame's *earlier* commands stayed applied. The check has moved from
+    /// the command to the frame, so there is no longer a moment at which
+    /// part of a frame has landed and the rest has been refused — the
+    /// refusal, if any, happens before anything is applied, and
+    /// `Sessions::apply_checked` cannot produce one.
+    /// [`CheckedBatch`] carries what a batch's basis *means* and the one
+    /// residue this does not close.
     fn commands(&mut self, id: SessionId, cmds: &[Command]) -> Vec<Addressed> {
-        let mut out = Vec::new();
-        for cmd in cmds {
-            let answer = self.command(id, cmd);
-            let refused = answer
-                .iter()
-                .any(|a| matches!(a.msg, ServerFrame::Rejected(_)));
-            out.extend(answer);
-            if refused {
-                break;
+        let batch = match CheckedBatch::against(&self.app, cmds) {
+            Ok(batch) => batch,
+            // Addressed to the asker alone, exactly as a single command's
+            // refusal is: nothing happened, so nobody else is owed anything.
+            Err(reason) => {
+                return vec![Addressed {
+                    to: id,
+                    msg: ServerFrame::Rejected(reason),
+                }];
             }
+        };
+        let mut out = Vec::new();
+        for cmd in batch {
+            out.extend(self.command(id, cmd));
         }
         out
     }
@@ -209,8 +254,8 @@ impl Host {
     /// The one door (gotcha 36), and the match that follows it is the
     /// mechanism (gotcha 39): asking for the `Dirty` means naming the case
     /// where a `Snapshot` is owed instead.
-    fn command(&mut self, id: SessionId, cmd: &Command) -> Vec<Addressed> {
-        match self.sessions.apply(&mut self.app, id, cmd) {
+    fn command(&mut self, id: SessionId, cmd: Checked<'_>) -> Vec<Addressed> {
+        match self.sessions.apply_checked(&mut self.app, id, cmd) {
             // The document was replaced. Nothing to diff against and nothing
             // to broadcast — each snapshot already carries a whole view slice
             // and the seed that reproduces the new document.
@@ -234,10 +279,13 @@ impl Host {
                 out.extend(self.project(dirty));
                 out
             }
-            // Nothing was applied, so nothing is owed to anybody else — and
-            // the asker is owed no state either, because it already holds
-            // what is current. All it is told is that its batch did not
-            // land, and which class of staleness that was.
+            // Unreachable through this door since improvise-955n — a frame
+            // is refused before any of it is applied, by `commands` — but
+            // `Applied` is the shape `Sessions::apply` answers in and a host
+            // that wants the `Dirty` must name every case (gotcha 39). The
+            // answer is the same one `commands` gives: nothing was applied,
+            // so nothing is owed to anybody else, and the asker is told only
+            // that its batch did not land and which class that was.
             Applied::Rejected { reason } => vec![Addressed {
                 to: id,
                 msg: ServerFrame::Rejected(reason),
@@ -351,6 +399,36 @@ mod tests {
         }
     }
 
+    /// The basis a caught-up client would state for a **frame** naming
+    /// `regions`: the two document tiers plus each named record's current
+    /// counter.
+    ///
+    /// `reduce::caught_up` cannot stand in for this, and the difference is
+    /// exactly what a frame is: it builds the claim for **one command**, so a
+    /// frame's records beyond the first are simply absent from it and come
+    /// back `RecordUnstated`. A real client calls `BasisCache::claim` once
+    /// with every record its frame names, which is what this reproduces.
+    ///
+    /// **Name only records that exist.** `claim` *omits* a record the cache
+    /// has never heard of; this always includes one, so pointed at a record
+    /// that is not there it fabricates `(hash, 0)` — a claim no real client
+    /// could compose, and precisely the input that flips the create
+    /// exemption in `CheckedBatch`. For a record the frame is *creating*,
+    /// leave it out, which is what a client would do.
+    fn frame_basis(host: &Host, regions: &[&str]) -> improvise_protocol::Basis {
+        improvise_protocol::Basis {
+            document_generation: host.app().document_generation(),
+            layout_version: host.app().layout_version(),
+            records: regions
+                .iter()
+                .map(|r| {
+                    let hash = addressed(r).record;
+                    (hash, host.app().record_version_of(hash))
+                })
+                .collect(),
+        }
+    }
+
     /// Everything one client was told, in arrival order.
     fn to(frames: &[Addressed], id: SessionId) -> Vec<ServerFrame> {
         frames
@@ -551,8 +629,10 @@ mod tests {
         );
     }
 
-    /// A batch stops at the first refusal rather than carrying on past a
-    /// basis the document has contradicted.
+    /// A refused batch applies nothing *after* the refusal. The stronger
+    /// half — nothing *before* it either — is
+    /// `a_refused_batch_leaves_the_commands_before_it_unapplied`; this is
+    /// kept because the two halves fail independently.
     #[test]
     fn a_batch_is_abandoned_at_the_first_rejection() {
         use improvise_protocol::{Basis, ModelCommand};
@@ -584,6 +664,451 @@ mod tests {
         );
     }
 
+    /// improvise-955n / improvise-08wb. **The bug**: ADR 001 is emphatic
+    /// that a stale batch is never applied, *not even partially*, and
+    /// improvise-ondj went to the trouble of a type (`Checked`) to make the
+    /// unchecked path unconstructible. But `Checked` is **per command**, and
+    /// a `ClientFrame::Commands` is a sequence of them: `Host::commands`
+    /// applied in order and abandoned at the first rejection, so a frame
+    /// whose *second* command was refused had already applied its first.
+    /// The guarantee was true of a single command and false of a batch.
+    ///
+    /// The earlier command is a **view** command, which is not a
+    /// concession: it is the shape the one real producer emits
+    /// (`keys::commit` — one mutation among view commands), and it is still
+    /// observable, because the host folds it into that client's mirror.
+    /// A frame may carry more than one mutation; what it may not do is
+    /// apply some of them and refuse the rest, which is clause 4 and open.
+    ///
+    /// **A refusal test needs an accepting one**
+    /// (`doctrine(a_helper_that_builds_the_world_the_code_wants_tests_nothing)`:
+    /// pair a refusal with an acceptance, or the refusal can pass by
+    /// refusing everything). The second half runs the same leading
+    /// `SetSelected` followed by a command whose basis is *current*, and
+    /// requires both to land. Without it, a `SetSelected` that simply never
+    /// worked would clear the first assertion and prove nothing at all.
+    #[test]
+    fn a_refused_batch_leaves_the_commands_before_it_unapplied() {
+        use improvise_protocol::ModelCommand;
+
+        let edit = |value: f64| ModelCommand::SetCell {
+            cell: addressed("North"),
+            value: CellValue::Number(value),
+        };
+
+        let mut refused = host();
+        let (asker, _) = refused.connect(10, 10);
+        // A claim that is current in every respect but one, so the refusal
+        // can only be about the tier that was moved. Building it from
+        // `caught_up` and then bumping a single tier keeps the rest
+        // well-formed — a hand-built basis that is stale in three ways at
+        // once cannot tell which check answered.
+        let Command::Model { mut basis, cmd } =
+            crate::ui::reduce::caught_up(refused.app(), edit(42.0))
+        else {
+            unreachable!("caught_up builds a Command::Model")
+        };
+        basis.document_generation += 1;
+        let stale = Command::Model { basis, cmd };
+        let answer = refused.handle(
+            asker,
+            ClientFrame::Commands(vec![Command::SetSelected { row: 1, col: 0 }, stale]),
+        );
+
+        assert_eq!(
+            refused
+                .sessions
+                .get_mut(asker)
+                .unwrap()
+                .view()
+                .cursor()
+                .selected,
+            (0, 0),
+            "the command before the refused one reached the mirror, so the \
+             batch was applied partially"
+        );
+        assert!(
+            matches!(
+                to(&answer, asker).as_slice(),
+                [ServerFrame::Rejected(Rejection::DocumentReplaced)]
+            ),
+            "and the answer must be the refusal and nothing else: {answer:?}"
+        );
+
+        // The other history: the same leading command, a basis that is
+        // current, and both must land.
+        let mut accepted = host();
+        let (asker, _) = accepted.connect(10, 10);
+        let fresh = crate::ui::reduce::caught_up(accepted.app(), edit(7.0));
+        accepted.handle(
+            asker,
+            ClientFrame::Commands(vec![Command::SetSelected { row: 1, col: 0 }, fresh]),
+        );
+        assert_eq!(
+            accepted
+                .sessions
+                .get_mut(asker)
+                .unwrap()
+                .view()
+                .cursor()
+                .selected,
+            (1, 0),
+            "an accepted batch must still apply its earlier commands"
+        );
+        assert_eq!(
+            accepted
+                .app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("North")),
+            Some(&CellValue::Number(7.0)),
+        );
+    }
+
+    /// improvise-955n. **The regression 90ea26e introduced, and the reason
+    /// clause 2 cannot be applied to all three tiers.**
+    ///
+    /// Hoisting the basis check to the frame hoisted *all* of it, so every
+    /// mutation after the first was admitted with no record check at all.
+    /// Two clients, one record: A composes `[SetCell(North), SetCell(South)]`
+    /// against an honest current basis, B writes South, A's frame arrives —
+    /// and A's write to South landed on top of B's **silently**, which is
+    /// exactly the lost update ADR 001's record tier exists to prevent. At
+    /// the base commit the second command was checked and the frame refused.
+    ///
+    /// **The per-command check was doing two jobs and the commit noticed
+    /// one.** "Is the client's picture of the *document* current" is a
+    /// frame-level fact and hoists correctly. "Is the client's picture of
+    /// *this record* current" is not: which records are at stake is a
+    /// property of the command, which is why `records_named` is an exhaustive
+    /// match over `ModelCommand` (gotcha 43). The incapability argument that
+    /// motivated clause 3 is true of records a frame **creates** and was
+    /// over-generalised to records it merely **writes** — and those are the
+    /// ones the tier is for.
+    ///
+    /// Three arms, because a fix that refuses everything would pass the first
+    /// alone and a fix that checks nothing would pass the last two.
+    #[test]
+    fn a_later_write_in_a_chain_still_answers_for_its_own_record() {
+        use improvise_protocol::ModelCommand;
+
+        let write = |region: &str, value: f64| ModelCommand::SetCell {
+            cell: addressed(region),
+            value: CellValue::Number(value),
+        };
+
+        // ── arm 1: the second record moved under the frame ──────────────
+        let mut moved = host();
+        let (a, _) = moved.connect(10, 10);
+        let (b, _) = moved.connect(10, 10);
+        // A seeds both records so its basis can state a counter for each,
+        // then composes a frame naming both.
+        moved.model(a, write("North", 1.0));
+        moved.model(a, write("South", 2.0));
+        let basis = frame_basis(&moved, &["North", "South"]);
+        let frame = vec![
+            Command::Model {
+                basis: basis.clone(),
+                cmd: write("North", 10.0),
+            },
+            Command::Model {
+                basis,
+                cmd: write("South", 20.0),
+            },
+        ];
+
+        // B moves South. `SetCell` is `CellData`, so neither the document nor
+        // the layout tier moves — only the record tier can discriminate here,
+        // which is what makes this a test of the record tier and not of the
+        // hoist in general.
+        moved.model(b, write("South", 99.0));
+
+        let answer = moved.handle(a, ClientFrame::Commands(frame));
+
+        assert!(
+            to(&answer, a)
+                .iter()
+                .any(|f| matches!(f, ServerFrame::Rejected(Rejection::RecordMoved(_)))),
+            "A's frame names a record B has moved and must be refused: {answer:?}"
+        );
+        assert_eq!(
+            moved
+                .app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("South")),
+            Some(&CellValue::Number(99.0)),
+            "B's write must survive — this is the lost update the tier exists for"
+        );
+        assert_eq!(
+            moved
+                .app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("North")),
+            Some(&CellValue::Number(1.0)),
+            "and the refusal is whole-frame: the *first* command must not have \
+             landed either"
+        );
+
+        // ── arm 2: nothing moved, so a multi-record frame lands ─────────
+        let mut fresh = host();
+        let (asker, _) = fresh.connect(10, 10);
+        fresh.model(asker, write("North", 1.0));
+        fresh.model(asker, write("South", 2.0));
+        let basis = frame_basis(&fresh, &["North", "South"]);
+        let answer = fresh.handle(
+            asker,
+            ClientFrame::Commands(vec![
+                Command::Model {
+                    basis: basis.clone(),
+                    cmd: write("North", 10.0),
+                },
+                Command::Model {
+                    basis,
+                    cmd: write("South", 20.0),
+                },
+            ]),
+        );
+        assert!(
+            !answer
+                .iter()
+                .any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
+            "a frame whose records are all current must land: {answer:?}"
+        );
+        assert_eq!(
+            fresh
+                .app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("South")),
+            Some(&CellValue::Number(20.0)),
+        );
+
+        // ── arm 3: the same record twice in one chain ───────────────────
+        // Legitimate and likely — two measures on one record is one record —
+        // and it works for a reason worth pinning rather than rediscovering:
+        // **every check happens before any command is applied**, so the
+        // second write is compared to the record's *pre-frame* counter, not
+        // to the one its own sibling just bumped. Were the checks
+        // interleaved with the applies, this frame would refuse itself with
+        // `RecordMoved`.
+        //
+        // **Read this before closing clause 4.** Interleaving is tempting
+        // there, because it would close the *silence*:
+        // `[RemoveItem(North), SetCell(North)]` under interleaving is refused
+        // `RecordGone` instead of silently half-applying, which is half of
+        // what clause 4 owes. So check-before-apply is not a free win — it
+        // buys the legitimate double-write below and **sells** the loud
+        // failure on a broken chain, and clause 4 has to pay that back some
+        // other way. Measured rather than argued: injecting the interleaving
+        // fails exactly two tests, this one and
+        // `a_frame_whose_later_mutation_fails_is_not_yet_undone`, and the
+        // second failing *is* the trade. This arm is what stops someone
+        // taking the trade without noticing they made it.
+        let mut twice = host();
+        let (asker, _) = twice.connect(10, 10);
+        twice.model(asker, write("North", 1.0));
+        let basis = frame_basis(&twice, &["North"]);
+        let answer = twice.handle(
+            asker,
+            ClientFrame::Commands(vec![
+                Command::Model {
+                    basis: basis.clone(),
+                    cmd: write("North", 10.0),
+                },
+                Command::Model {
+                    basis,
+                    cmd: write("North", 11.0),
+                },
+            ]),
+        );
+        assert!(
+            !answer
+                .iter()
+                .any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
+            "a chain may write one record twice: {answer:?}"
+        );
+        assert_eq!(
+            twice
+                .app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("North")),
+            Some(&CellValue::Number(11.0)),
+            "and the last write in the chain is the one that stands"
+        );
+    }
+
+    /// **The capability the contract is for**: a chain of data-dependent
+    /// commands, applied in order, each seeing what the previous one left.
+    ///
+    /// `[AddItem("East"), SetCell(the East record)]` is the archetype, and it
+    /// is the case a per-command basis check **cannot** admit however it is
+    /// ordered: a client cannot claim a counter for a record its own chain is
+    /// about to create, because `BasisCache::claim` filters to records the
+    /// cache has been told about. So the basis is answered once, on the first
+    /// command that carries one, and the rest of the chain runs against what
+    /// its predecessors left.
+    ///
+    /// The second assertion is the one that earns the test. Asserting only
+    /// that the item exists would pass against a host that applied the first
+    /// command and dropped the second — which is exactly what the previous
+    /// rule did, loudly, and what the pre-frame check did, silently.
+    #[test]
+    fn a_data_dependent_chain_lands_in_order() {
+        use improvise_protocol::ModelCommand;
+
+        let mut host = host();
+        let (asker, _) = host.connect(10, 10);
+        let add = crate::ui::reduce::caught_up(
+            host.app(),
+            ModelCommand::AddItem {
+                category: "Region".into(),
+                item: "East".into(),
+            },
+        );
+        let write = crate::ui::reduce::caught_up(
+            host.app(),
+            ModelCommand::SetCell {
+                cell: addressed("East"),
+                value: CellValue::Number(9.0),
+            },
+        );
+
+        let answer = host.handle(asker, ClientFrame::Commands(vec![add, write]));
+
+        assert!(
+            !answer
+                .iter()
+                .any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
+            "a data-dependent chain must not be refused: {answer:?}"
+        );
+        assert!(
+            host.app()
+                .model_state
+                .workbook
+                .model
+                .category("Region")
+                .is_some_and(|c| c.ordered_item_names().contains(&"East")),
+            "the first command must land"
+        );
+        assert_eq!(
+            host.app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("East")),
+            Some(&CellValue::Number(9.0)),
+            "and the second must land on top of it — a write to a record the \
+             frame itself created is the whole point"
+        );
+    }
+
+    /// **Characterization of the open hole (clause 4), not an endorsement.**
+    ///
+    /// The contract says a frame is all-or-nothing. It is not yet:
+    /// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is
+    /// not a rollback), so a chain whose later mutation fails leaves the
+    /// earlier one standing. `[RemoveItem("North"), SetCell(the North
+    /// record)]` is the shape: the removal lands, the write then resolves to
+    /// nothing, `model_effects` returns an **empty** batch, and the frame is
+    /// reported accepted having applied half of itself. Both the partial
+    /// application and the silence are open.
+    ///
+    /// **This test did not pin that, and the way it failed is the repo's own
+    /// signature defect sitting on the one test whose job is to fail loudly
+    /// if anyone believes the hole is closed.** It asserted that North was
+    /// removed and that nothing was rejected — both true of any *wholly
+    /// successful* frame. The independent reviewer retargeted the second
+    /// command from the removed North to the live South, making the frame
+    /// succeed completely, and the test still passed. It could not tell
+    /// half-applied from fully-applied, so it would never have inverted when
+    /// clause 4 landed; it would have gone on passing. The missing assertion
+    /// is the one that names the hole: **the second mutation did not land.**
+    ///
+    /// It is unreachable from any client here — the only producer of a
+    /// multi-command frame is `keys::commit`, which emits one mutation among
+    /// view commands — which is what makes it safe to document rather than
+    /// rush.
+    ///
+    /// **This test must be inverted when clause 4 lands**: all three
+    /// assertions flip, because then nothing at all happens.
+    #[test]
+    fn a_frame_whose_later_mutation_fails_is_not_yet_undone() {
+        use improvise_protocol::ModelCommand;
+
+        let mut host = host();
+        let (asker, _) = host.connect(10, 10);
+        host.model(
+            asker,
+            ModelCommand::SetCell {
+                cell: addressed("North"),
+                value: CellValue::Number(1.0),
+            },
+        );
+        // The frame claims North at its current counter, so it passes the
+        // check and fails at *apply* time — which is the only way to reach
+        // the hole. A frame refused by the check never applies anything and
+        // would be testing clause 2 instead.
+        let basis = frame_basis(&host, &["North"]);
+        let answer = host.handle(
+            asker,
+            ClientFrame::Commands(vec![
+                Command::Model {
+                    basis: basis.clone(),
+                    cmd: ModelCommand::RemoveItem {
+                        category: "Region".into(),
+                        item: "North".into(),
+                    },
+                },
+                Command::Model {
+                    basis,
+                    cmd: ModelCommand::SetCell {
+                        cell: addressed("North"),
+                        value: CellValue::Number(5.0),
+                    },
+                },
+            ]),
+        );
+
+        assert!(
+            !host
+                .app()
+                .model_state
+                .workbook
+                .model
+                .category("Region")
+                .is_some_and(|c| c.ordered_item_names().contains(&"North")),
+            "TODAY: the first command lands. Under clause 4 it must be undone."
+        );
+        assert_eq!(
+            host.app()
+                .model_state
+                .workbook
+                .model
+                .get_cell(&cell("North")),
+            None,
+            "TODAY: the second mutation does NOT land — its surrogate no \
+             longer resolves. **This is the assertion that makes the test \
+             about the hole**: without it the test passes against a frame \
+             that succeeded completely, which is how it was found to be \
+             vacuous."
+        );
+        assert!(
+            !answer
+                .iter()
+                .any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
+            "TODAY: the frame is reported accepted although half of it did \
+             nothing. Under clause 4 it must be refused: {answer:?}"
+        );
+    }
+
     // ── the broadcast ───────────────────────────────────────────────────
 
     /// The bug Copilot found on PR #10: a bystander must fold the accepted
@@ -762,6 +1287,128 @@ mod tests {
         );
     }
 
+    // ── saving ──────────────────────────────────────────────────────────
+
+    /// A host over `MemoryFs` that already knows where its document lives,
+    /// so a client's `Save` has somewhere to land.
+    fn host_with_a_path() -> (Host, PathBuf) {
+        let path = PathBuf::from("doc.improv");
+        let mut app = App::new(workbook(), Some(path.clone()));
+        app.storage = Box::new(MemoryFs::new());
+        app.rebuild_layout();
+        (Host::new(app), path)
+    }
+
+    /// What is actually on this host's storage, if anything.
+    fn stored(host: &Host, path: &Path) -> Option<String> {
+        host.app()
+            .storage
+            .read(path)
+            .ok()
+            .map(|bytes| String::from_utf8(bytes).expect("the fixture writes text"))
+    }
+
+    /// improvise-368h. **The bug**: `effect::Save` is `EffectKind::ViewOnly`
+    /// — correctly, since it writes *this host's* path — so the derivation
+    /// that builds `ModelCommand` out of the `EffectKind::Model` effects
+    /// could not reach it (gotcha 38). There was no save on the wire at all, so
+    /// **no client on any transport could cause its host to write**: a
+    /// websocket session edited the document in memory and the file on disk
+    /// never moved unless a human was at a TUI that was not there.
+    ///
+    /// The derivation was not wrong; it was complete over the mutations and
+    /// saving is not one. So the variant is *added* rather than derived, and
+    /// it carries no path: a client cannot see the host's storage, and which
+    /// document a host is hosting is the host's own fact.
+    #[test]
+    fn a_client_can_ask_its_host_to_save() {
+        let (mut host, path) = host_with_a_path();
+        let (client, _) = host.connect(10, 10);
+        host.model(
+            client,
+            ModelCommand::SetCell {
+                cell: addressed("North"),
+                value: CellValue::Number(41.0),
+            },
+        );
+        assert!(
+            stored(&host, &path).is_none(),
+            "nothing should have been written before the client asked"
+        );
+
+        let answer = host.handle(client, ClientFrame::Save);
+
+        let written = stored(&host, &path).expect("the client's save must reach storage");
+        assert!(
+            written.contains("41"),
+            "the written document must carry the client's edit, got:\n{written}"
+        );
+        assert!(
+            !answer
+                .iter()
+                .any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
+            "a save that landed is not a rejection: {answer:?}"
+        );
+    }
+
+    /// improvise-368h: the "what if there is no path" half. Not a panic and
+    /// not a silent no-op — the two answers the ticket ruled out — but the
+    /// vocabulary that already exists for "nothing happened and here is
+    /// why", addressed to the asker alone (gotcha 39).
+    ///
+    /// The second client is not decoration: a refusal that leaked to the
+    /// bystanders would be telling them about a failure of somebody else's
+    /// host-local request, and this asserts it does not.
+    #[test]
+    fn a_save_with_nowhere_to_write_is_refused_and_told_only_to_the_asker() {
+        let mut host = host();
+        let (asker, _) = host.connect(10, 10);
+        let (bystander, _) = host.connect(10, 10);
+        assert!(
+            host.app().model_state.file_path.is_none(),
+            "the fixture must genuinely have no path, or this proves nothing"
+        );
+
+        let answer = host.handle(asker, ClientFrame::Save);
+
+        assert_eq!(
+            to(&answer, asker),
+            vec![ServerFrame::Rejected(Rejection::NoDocumentPath)],
+        );
+        assert!(
+            to(&answer, bystander).is_empty(),
+            "nothing happened, so nobody else is owed anything: {answer:?}"
+        );
+    }
+
+    /// improvise-368h: a write that was attempted and failed must not be
+    /// reported as a success. `Save` signals failure by aborting its batch
+    /// (gotcha 21), and the batch here is exactly `[Save]`, so an abort is
+    /// that failure and nothing else.
+    ///
+    /// The fixture fails for a reason that is real rather than mocked: a
+    /// `PhysicalFs` pointed at a directory that does not exist. It creates
+    /// nothing, so the test leaves no trace on disk.
+    #[test]
+    fn a_write_that_fails_is_refused_rather_than_reported_as_saved() {
+        let absent = std::env::temp_dir().join("improvise-no-such-directory-368h");
+        assert!(
+            !absent.exists(),
+            "the fixture only fails if the directory really is absent"
+        );
+        let mut app = App::new(workbook(), Some(absent.join("doc.improv")));
+        app.rebuild_layout();
+        let mut host = Host::new(app);
+        let (client, _) = host.connect(10, 10);
+
+        let answer = host.handle(client, ClientFrame::Save);
+
+        assert_eq!(
+            to(&answer, client),
+            vec![ServerFrame::Rejected(Rejection::SaveFailed)],
+        );
+    }
+
     // ── disconnect ──────────────────────────────────────────────────────
 
     /// AC 4: a dropped client stops being projected to, and a late frame

crates/improvise-command/src/ui/project.rs

@@ -990,13 +990,26 @@ impl Sessions {
     /// the [`Checked`] every mutation below needs — the check is not a step
     /// this function could forget (improvise-ondj).
     pub fn apply(&mut self, app: &mut App, id: SessionId, cmd: &Command) -> Applied {
-        let checked = match Checked::against(app, cmd) {
-            Ok(checked) => checked,
-            Err(reason) => return Applied::Rejected { reason },
-        };
-        match cmd {
-            Command::SwitchView(_) => self.apply_switch(app, checked),
-            Command::Model { .. } => self.apply_model(app, checked),
+        match Checked::against(app, cmd) {
+            Ok(checked) => self.apply_checked(app, id, checked),
+            Err(reason) => Applied::Rejected { reason },
+        }
+    }
+
+    /// The same door, for a command whose answer is already in hand.
+    ///
+    /// A frame is a *sequence* of commands and ADR 001's atomicity is the
+    /// frame's, so `Host::commands` answers for all of them with a
+    /// [`CheckedBatch`](crate::ui::reduce::CheckedBatch) and then applies
+    /// them through here — re-asking per command would reject the later ones
+    /// for staleness their own siblings caused (improvise-955n).
+    ///
+    /// It cannot return [`Applied::Rejected`]: the refusal happened, if it
+    /// happened, before the batch was built.
+    pub fn apply_checked(&mut self, app: &mut App, id: SessionId, cmd: Checked<'_>) -> Applied {
+        match cmd.command() {
+            Command::SwitchView(_) => self.apply_switch(app, cmd),
+            Command::Model { .. } => self.apply_model(app, cmd),
             other => {
                 if let Some(session) = self.sessions.get_mut(&id) {
                     session.fold(other);

crates/improvise-command/src/ui/reduce.rs

@@ -45,8 +45,11 @@
 //! only way to obtain a `Checked` over a `Command::Model` is
 //! [`Checked::against`], which runs [`stale_basis`] against the
 //! authoritative `App`. A host that wants to apply a client's batch must
-//! face the [`Rejection`], and [`Sessions::apply`](super::project::Sessions::apply)
-//! is still the door that does it for you.
+//! face the [`Rejection`], and `Sessions::apply_checked` is the door a host
+//! goes through — `Host::commands` answers the whole frame first with
+//! [`CheckedBatch::against`], so `Sessions::apply` (which answers one command
+//! and then dispatches) now has no production caller and is kept for the
+//! single-command case its tests drive.
 
 use improvise_core::model::record::RecordHash;
 use improvise_protocol::{Basis, Command, ModelCommand, Rejection, reduce_view};
@@ -69,8 +72,216 @@ use crate::ui::project::{Dirty, document_projections};
 /// hole this closes, because a `Sessions` is already bound to one document's
 /// generation sequence (ADR 001) and a `Registry` hands out one pair at a
 /// time.
+///
+/// # Why the lifetime does not close it (improvise-7v62)
+///
+/// The obvious answer — make the lifetime the `&App`'s, so the borrow
+/// checker forbids checking one document and applying to another — was
+/// tried, and it is not merely inconvenient: it is **contradictory with what
+/// the type is for**. A `Checked` is a proof discharged *before* a
+/// mutation, and its whole job is to be carried across that mutation; a
+/// borrow of the `App` is a claim that must stay live *during* it. The two
+/// cannot both hold. Tying them makes every call site an `E0502`, starting
+/// with the canonical one:
+///
+/// ```text
+/// error[E0502]: cannot borrow `*app` as mutable because it is also borrowed as immutable
+///     match Checked::against(app, cmd) {
+///                            --- immutable borrow occurs here
+///         Ok(checked) => self.apply_checked(app, id, checked),
+///                        ^^^^^ mutable borrow occurs here
+/// ```
+///
+/// A generative brand would express it without borrowing, but that needs a
+/// closure-scoped API and would distort every host. The remaining option is
+/// carrying a document identity, and `DocumentId` lives in the *server's*
+/// registry: an `App` does not have one and does not want one, precisely
+/// because the unit of authority is one document per `App`. So this is a
+/// **deliberate won't-fix**, not an oversight. What keeps it unreachable is
+/// stated above and is load-bearing — improvise-j31x made this a
+/// multi-document server on purpose, and ADR 001 never evicts a document so
+/// that a reconnecting client cannot have a stale basis accepted as current.
 pub struct Checked<'a>(&'a Command);
 
+/// A whole [`ClientFrame::Commands`](improvise_protocol::ClientFrame) the
+/// authoritative state has answered for — **every** command in it, before
+/// any of them is applied (improvise-955n/improvise-08wb, ADR 001).
+///
+/// [`Checked`] alone was not enough, and the gap was exactly one level up
+/// from the one improvise-ondj closed. A `Checked` answers for *one*
+/// command against the `App` **as it stands**; a frame is a sequence of
+/// them, applied in order, so a frame whose second command was refused had
+/// already applied its first. ADR 001 says a stale batch is never applied,
+/// *not even partially*, and that was true of a single command and false of
+/// a batch.
+///
+/// # One basis, checked once, and then the chain runs
+///
+/// The contract, as the user stated it: *"a sequence of commands sent to the
+/// server should assume that it's played in order from start to end and that
+/// each command sees the state after the previous command runs"*, sent as
+/// *"a single transaction from a given base"*. Four clauses:
+///
+/// 1. A frame states **one basis**, describing the base the client composed
+///    against.
+/// 2. That basis is checked **once**, against that base, before anything is
+///    applied.
+/// 3. Commands are applied **in order**, and command N sees the state command
+///    N-1 left. Data dependence between them is expected, not an error.
+/// 4. **All or nothing.** If any command fails, the frame leaves the document
+///    exactly as the base described.
+///
+/// Clauses 1–3 are here. **Clause 4 is not yet implemented** — see the hole
+/// below and the design note on PR #26.
+///
+/// # Two of the three tiers hoist, and the third does not
+///
+/// Clause 2 hoists the check, but **not every tier is a frame-level fact**,
+/// and a first version of this hoisted all three. That was a lost update:
+/// `document_generation` and `layout_version` describe the *document*, so
+/// answering them once for the frame is exactly right — but the record tier
+/// asks "is the client's picture of **this record** current", and which
+/// records are at stake is a property of the *command*. That is why
+/// [`records_named`] is an exhaustive match over [`ModelCommand`] and not
+/// something a frame could summarise (gotcha 43).
+///
+/// Skipping it for later commands let `[SetCell(North), SetCell(South)]`
+/// overwrite a South that somebody else had moved, silently, with the frame
+/// reported accepted — the precise loss ADR 001's record tier exists to
+/// prevent, and invisible to the other two tiers because a `SetCell` is
+/// `CellData` and moves neither of them. So **every** mutation answers for
+/// its own records, against the **frame's** basis: the one the first
+/// basis-carrying command stated, not each command's own field, so a later
+/// command cannot state a fresher claim than the frame was composed under.
+///
+/// ## Except the records the frame is creating
+///
+/// The argument that motivated clause 3 is true, and it is true of a
+/// narrower thing than it was first applied to. A client cannot claim a
+/// counter for a record its own chain is about to **create**, because
+/// [`BasisCache::claim`](improvise_protocol::BasisCache::claim) filters to
+/// records the cache has been told about — so `[AddItem(East),
+/// SetCell(East)]` would be refused by a naive per-command check however it
+/// is ordered. But that argument was then stretched to records the frame
+/// merely **writes**, and those are the ones the tier is for.
+///
+/// So the exemption is stated over the *property* rather than over a list of
+/// commands: a record that **neither resolves in the document nor is claimed
+/// by the basis** is one the frame is about to create, and is not answered
+/// for. Every other combination still is — including a record that does not
+/// resolve but *is* claimed, which is a client believing in something that
+/// has been removed, and is `RecordGone` as before. See
+/// [`will_be_created_by_the_frame`].
+///
+/// What the base means is therefore: **what the client saw**. The transaction
+/// promises the whole chain lands on top of it or none of it does. The
+/// chain's internal coherence is the transaction's job, not the basis
+/// check's.
+///
+/// # The hole clause 4 leaves, stated rather than hidden
+///
+/// `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a
+/// rollback), so a frame whose second mutation fails leaves the first
+/// applied. `[RemoveItem("North"), SetCell(the North record)]` is the
+/// example: the removal lands, the write then resolves to nothing and
+/// `model_effects` returns an **empty** batch, so the frame is reported
+/// accepted having applied half of itself. Both halves of that are open —
+/// the partial application and the silence.
+///
+/// The create exemption has a residue of the same kind and it is the same
+/// hole: a later command naming a record that is neither present nor claimed
+/// is *presumed* to be one the frame creates, so if the frame does not in
+/// fact create it, the write resolves to nothing and silently does not
+/// happen. That is `model_effects`' empty batch again, not a second defect,
+/// and it closes when clause 4 makes a mid-chain failure reportable.
+///
+/// Both are **unreachable from any client in this workspace**: the only
+/// producer of a multi-command frame is
+/// `improvise-wasm-client::keys::commit`, which emits one mutation among view
+/// commands. **Reachability is a reason not to rush a fix and not a reason
+/// not to notice one** — the lost update above had the same reachability
+/// argument as this hole and none of its documentation, which is how it
+/// shipped. That is what makes it safe to
+/// document rather than rush, and it is why an earlier "at most one document
+/// mutation per frame" rule — which closed the hole by forbidding the
+/// capability — was rejected.
+///
+/// # Two things the rule that was rejected got wrong, worth keeping
+///
+/// **Arity is not a property of a variant.** A future `Command::RunScript` is
+/// one variant performing an unbounded number of mutations, so no count over
+/// the vocabulary can be right: a classifier can honestly answer *whether* a
+/// command changes the document, never *how many times*. A
+/// `mutates_the_document` predicate written for the rejected rule is deleted
+/// with it rather than kept unused — but it was made an **exhaustive match**
+/// on review, because a `matches!` answers `false` for a future variant in
+/// silence, and clause 4 will need that classifier back. Write it exhaustive
+/// when it returns.
+///
+/// **The transaction boundary is not the frame.** Because one command may
+/// already be a chain, clause 4 is a property the system needs *inside* a
+/// command as much as across a frame of them, which is why the design note
+/// puts the mechanism at `App::apply_effects` rather than here.
+pub struct CheckedBatch<'a>(Vec<Checked<'a>>);
+
+impl<'a> CheckedBatch<'a> {
+    /// Answer for every command in a frame, against the state the frame was
+    /// composed against — which is the state *before* any of them runs.
+    ///
+    /// The refusal is the frame's answer and nothing is applied. It is a
+    /// `Result` rather than a per-command verdict for the same reason
+    /// [`Checked`] is a type rather than a guard: the only enforceable
+    /// version of "answer before applying" is not being able to reach the
+    /// commands without having done so.
+    ///
+    /// **The two document tiers are answered once**, on the first command
+    /// that carries a basis — clause 2 — and every later command is admitted
+    /// without re-asking them, which is clause 3. **The record tier is asked
+    /// of every mutation**, against that same frame basis, because it is not
+    /// a frame-level fact. A frame with no mutation answers trivially.
+    ///
+    /// The `Basis` on a *later* `Command::Model` is ignored rather than
+    /// compared, and that is an honest interim rather than the end state:
+    /// clause 1 says the **frame** states one basis, but the wire still
+    /// carries a `Basis` inside each `Command::Model` — right when one
+    /// command was one transaction, and now one field per command describing
+    /// something that belongs to the frame. Moving it to
+    /// `ClientFrame::Commands` would make "one basis per frame"
+    /// unrepresentable-otherwise instead of a convention, and that protocol
+    /// change belongs with clause 4.
+    pub fn against(app: &App, cmds: &'a [Command]) -> Result<Self, Rejection> {
+        // The frame's basis: the one its first mutation stated. Later
+        // commands carry a `Basis` field too, and it is deliberately *not*
+        // read — clause 1 says the frame states one base, and reading each
+        // command's own would let a chain claim a fresher picture than it was
+        // composed under.
+        let mut frame_basis: Option<&Basis> = None;
+        let mut checked = Vec::with_capacity(cmds.len());
+        for cmd in cmds {
+            let one = match frame_basis {
+                Some(basis) => Checked::within_a_frame(app, basis, cmd)?,
+                None => {
+                    if let Command::Model { basis, .. } = cmd {
+                        frame_basis = Some(basis);
+                    }
+                    Checked::against(app, cmd)?
+                }
+            };
+            checked.push(one);
+        }
+        Ok(Self(checked))
+    }
+}
+
+impl<'a> IntoIterator for CheckedBatch<'a> {
+    type Item = Checked<'a>;
+    type IntoIter = std::vec::IntoIter<Checked<'a>>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.0.into_iter()
+    }
+}
+
 impl<'a> Checked<'a> {
     /// Answer for a command, against the state it would be applied to.
     ///
@@ -80,6 +291,10 @@ impl<'a> Checked<'a> {
     /// workbook simply refuses a view it does not have (ADR 001 names that
     /// second hole and leaves it open deliberately).
     ///
+    /// Saving is not in this vocabulary and deliberately so: it is
+    /// `ClientFrame::Save`, answered by the host, because it mutates no
+    /// shared state and has no basis to answer for (improvise-368h).
+    ///
     /// There is no argument that suppresses the answer. ADR 001's force is a
     /// fresh batch under a fresh basis, composed by the client; it is not a
     /// way past this constructor.
@@ -93,6 +308,40 @@ impl<'a> Checked<'a> {
             None => Ok(Self(cmd)),
         }
     }
+
+    /// A command in a frame whose **document** tiers have already been
+    /// answered (clause 2), answering only for its own records.
+    ///
+    /// The two document tiers are not re-asked, which is clause 3: they
+    /// describe a document this command is not running against, since its
+    /// predecessors have moved it. The record tier *is* asked, because it is
+    /// a fact about the command rather than the frame — see
+    /// [`CheckedBatch`], where the lost update that taught this is recorded.
+    ///
+    /// Private, and that is what keeps it from being a bypass: from outside
+    /// this module the only doors are still [`Checked::against`] and
+    /// [`CheckedBatch::against`], and the only caller is the loop that has
+    /// just answered the frame's basis.
+    fn within_a_frame(app: &App, basis: &Basis, cmd: &'a Command) -> Result<Self, Rejection> {
+        let refused = match cmd {
+            Command::Model { cmd: model, .. } => stale_records_in_a_frame(app, basis, model),
+            _ => None,
+        };
+        match refused {
+            Some(reason) => Err(reason),
+            None => Ok(Self(cmd)),
+        }
+    }
+
+    /// The command this proof is about.
+    ///
+    /// It hands back a `&'a Command` rather than one borrowed from `&self`,
+    /// so a caller can dispatch on the variant and still move the proof into
+    /// the function it dispatched to. Crate-private: a proof that could be
+    /// unwrapped from outside would be a proof nobody had to obtain.
+    pub(crate) fn command(&self) -> &'a Command {
+        self.0
+    }
 }
 
 /// Apply one checked wire command to the authoritative state, and say what
@@ -347,6 +596,37 @@ pub fn stale_basis(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejec
     })
 }
 
+/// The record tier for a command inside a frame whose document tiers are
+/// already answered.
+///
+/// It reuses [`stale_record`] unchanged — one home for what each of the four
+/// combinations means — and differs only in *which* records it asks about:
+/// the ones the frame is about to create are skipped.
+fn stale_records_in_a_frame(app: &App, basis: &Basis, cmd: &ModelCommand) -> Option<Rejection> {
+    records_named(cmd)
+        .into_iter()
+        .filter(|hash| !will_be_created_by_the_frame(app, basis, *hash))
+        .find_map(|hash| stale_record(app, basis, hash))
+}
+
+/// Is this a record the frame is about to create?
+///
+/// **A property, not a list of commands.** The document does not have it and
+/// the basis claims nothing about it — which is precisely what "I am creating
+/// this" looks like from here, because `BasisCache::claim` can only state a
+/// counter for a record the client has been told about.
+///
+/// The near miss is the other half of that conjunction: a record that does
+/// not resolve but *is* claimed is a client believing in something that has
+/// been removed, and stays [`Rejection::RecordGone`]. Only the pair exempts.
+///
+/// It is asked **only inside a frame**, never of a lone command: a first
+/// command has no predecessor that could have created anything, so
+/// [`Checked::against`] still refuses a write to a record that is not there.
+fn will_be_created_by_the_frame(app: &App, basis: &Basis, hash: RecordHash) -> bool {
+    app.resolve_record(hash).is_none() && basis.record(hash).is_none()
+}
+
 fn stale_record(app: &App, basis: &Basis, hash: RecordHash) -> Option<Rejection> {
     match (app.resolve_record(hash), basis.record(hash)) {
         (None, _) => Some(Rejection::RecordGone(hash)),
@@ -1050,4 +1330,80 @@ mod tests {
             Dirty::of(EffectKind::Model(ModelChange::LayoutGeometry))
         );
     }
+
+    /// improvise-gv9r. **The bug**: a client's edit left the authoritative
+    /// document reporting *clean*, on every transport.
+    ///
+    /// `MarkDirty` used to be a separate effect that the *command* layer
+    /// composed alongside the mutation, and improvise-kmgs derived
+    /// `ModelCommand` from the effects **one at a time** — so the wire got
+    /// `SetCell` without its companion. A derivation correct per element and
+    /// wrong for a set. "Save if dirty" was therefore a trigger that never
+    /// fired, and a host built on it loses every client edit silently; the
+    /// browser worker found that out the hard way and had to over-approximate
+    /// off the command variant instead.
+    ///
+    /// The fix is the unrepresentable one: dirtiness is raised by
+    /// `App::apply_effects` off the same `EffectKind` the `Dirty` and the two
+    /// basis tiers are read from (gotcha 40), so no path can compose a
+    /// document change without it. There is nothing left to forget, and
+    /// `MarkDirty` is gone rather than left as a second answer.
+    ///
+    /// The three levels are asserted separately because they are three
+    /// different effects: an effect that is only ever reached by one of them
+    /// would otherwise pass on a sibling's evidence. The negative arm — that
+    /// a `ViewOnly` effect leaves the document clean — is
+    /// `a_view_only_batch_leaves_the_document_clean` in `ui/app.rs`, and it
+    /// has to live there: a view-slice `Command` never reaches
+    /// `apply_effects` at all (`apply_command` folds it straight into
+    /// `ViewState`), so a negative written against this door would pass
+    /// however unconditional the raise was.
+    #[test]
+    fn a_wire_model_command_marks_the_document_dirty() {
+        for cmd in [
+            ModelCommand::SetCell {
+                cell: RecordCell {
+                    record: RecordHash::of(&crate::model::cell::CellKey::new(vec![(
+                        "Region".to_string(),
+                        "North".to_string(),
+                    )])),
+                    measure: Some("Revenue".to_string()),
+                },
+                value: CellValue::Number(1.0),
+            },
+            ModelCommand::AddCategory("Product".into()),
+            ModelCommand::TransposeAxes,
+        ] {
+            let mut app = app();
+            assert!(!app.model_state.dirty, "the fixture starts clean");
+            let wire = caught_up(&app, cmd.clone());
+            apply_checked(&mut app, &wire);
+            assert!(
+                app.model_state.dirty,
+                "{cmd:?} changed the document and left it reporting clean"
+            );
+        }
+    }
+
+    /// `SwitchView` is the one command with a foot in both halves, and it is
+    /// the reason this matters beyond cell edits: `Workbook::active_view`
+    /// persists as the `Initial View` line, so a switch really does put the
+    /// document out of step with the file.
+    ///
+    /// It is tested on its own because the browser worker's persist trigger
+    /// now depends on it (improvise-gv9r): the worker used to name
+    /// `SwitchView` explicitly in a table of variants and now asks the
+    /// document. If this effect were ever reclassified `ViewOnly`, the
+    /// worker would silently stop writing view switches back.
+    #[test]
+    fn a_wire_view_switch_marks_the_document_dirty() {
+        let mut app = app();
+        assert!(!app.model_state.dirty, "the fixture starts clean");
+        apply_checked(&mut app, &Command::SwitchView("Second".into()));
+        assert_eq!(
+            app.model_state.workbook.active_view, "Second",
+            "the fixture must actually switch, or this proves nothing"
+        );
+        assert!(app.model_state.dirty);
+    }
 }

crates/improvise-protocol/src/basis.rs

@@ -89,12 +89,44 @@ impl Basis {
     }
 }
 
-/// Why a batch was refused.
+/// Why a **frame** was refused, and nothing happened.
 ///
-/// A **normal outcome**, not an error: the client already holds the new
-/// state through projections and re-composes against it. The variants are
-/// not decoration — ADR 001 turns on the difference between them, because
-/// what a *force* can mean differs by case:
+/// The frame, not the command: since improvise-368h a
+/// `ClientFrame::Save` the host cannot honour is answered here too, and that
+/// is not a refused *command* at all. A `ClientFrame::Commands` is refused
+/// as a whole, on the basis its first mutation stated.
+///
+/// A **normal outcome**, not an error. For the staleness class the client
+/// already holds the current state through the projections it has been
+/// folding, so re-composing is cheap and is what it does. A failed save has
+/// nothing to re-compose *against*: it is a fact about the host.
+///
+/// **Two classes live here**, and the doc enumerates them because the
+/// name says "basis" and only one of them is about a basis:
+///
+/// 1. **Staleness** (ADR 001) — [`Rejection::DocumentReplaced`],
+///    [`Rejection::LayoutMoved`] and the three record variants. The batch was
+///    composed against a picture of the document that has since moved. This
+///    is the only class where ADR 001's *force* means anything.
+/// 2. **A request the host cannot honour** —
+///    [`Rejection::NoDocumentPath`] and [`Rejection::SaveFailed`], answering
+///    a [`ClientFrame::Save`](crate::ClientFrame::Save) (improvise-368h).
+///    Nothing went stale; there is nothing to re-compose.
+///
+/// A third class is coming and is not here yet: a **transaction that was
+/// rolled back** because a later command in a chain could not be applied
+/// (clause 4 of the frame contract — see `CheckedBatch` in
+/// `improvise-command`). Until rollback exists there is nothing to report,
+/// because nothing is undone.
+///
+/// They share this type rather than getting refusal channels of their own,
+/// for the reason the type exists: a refusal is "nothing happened, here is
+/// why", addressed to the asker alone, and a second channel would be a
+/// second set of ordering and addressing rules for one idea.
+///
+/// The staleness variants are not decoration — ADR 001 turns on the
+/// difference between them, because what a *force* can mean differs by
+/// case:
 ///
 /// - [`Rejection::RecordMoved`] is class 1. Somebody wrote the record since
 ///   this client read it. Force is coherent and should be one keystroke: the
@@ -135,6 +167,27 @@ pub enum Rejection {
     /// tell a user — "you lost a race" against "you were never in one" — and
     /// because naming it is what closes the force-by-omission hole.
     RecordUnstated(RecordHash),
+
+    /// A [`ClientFrame::Save`](crate::ClientFrame::Save) reached a host that has no
+    /// path to write to (improvise-368h).
+    ///
+    /// Not a panic and not a silent no-op, which were the two things the
+    /// ticket ruled out. It is decided **before** anything runs, by the same
+    /// constructor that answers for a basis, so a save that cannot land is
+    /// refused rather than attempted.
+    ///
+    /// What the asker should do about it is the asker's business and differs
+    /// by front end: a TUI prompts for a path (improvise-lae), a browser
+    /// worker whose document *is* its storage entry never sees this at all.
+    NoDocumentPath,
+    /// A [`ClientFrame::Save`](crate::ClientFrame::Save) whose write failed.
+    ///
+    /// Carries no message, because [`Rejection`] is `Copy` and an I/O
+    /// error's text is not. The host keeps the text — `effect::Save` puts it
+    /// in the host's own status line — and what crosses the wire is the one
+    /// fact a client can act on: the document it is editing is not on disk.
+    /// Reporting a failed write as success is the thing this exists to stop.
+    SaveFailed,
 }
 
 /// The basis a client holds, folded from the stream like any other widget

crates/improvise-protocol/src/frame.rs

@@ -76,13 +76,71 @@ pub enum ClientFrame {
     /// whole. `Commands(vec![c])` expresses everything a singleton variant
     /// could, so there is one variant and not two.
     ///
-    /// The host applies them in order and abandons the batch at the first
-    /// rejection, so a refused batch may have applied its earlier commands.
-    /// That is not reachable from a client written against this crate — a
-    /// locally-resolved batch is view-only, and a `Command::Model` is
-    /// composed one at a time — but it is a hole in ADR 001's "never applied,
-    /// not even partially" and is filed rather than hidden.
+    /// **The frame is the unit of atomicity, not the command** (ADR 001,
+    /// improvise-955n). The host answers for *every* command in it before
+    /// applying any, so a frame is never applied partially **because of a
+    /// refusal** — a refused frame applies nothing at all. It used to apply
+    /// in order and abandon at the first rejection, which left a hole ADR
+    /// 001's "never applied, not even partially" does not have. The other
+    /// way a frame can stop half-way is still open; see the last paragraph.
+    ///
+    /// Two consequences worth knowing here rather than only on the host.
+    ///
+    /// **The frame states one base**, and it is the only claim a client can
+    /// construct: a basis is published between batches and never during one,
+    /// so no client can hold one describing the document as it would be
+    /// half-way through its own frame. Commands then run in order, each
+    /// against what its predecessor left, and data dependence between them is
+    /// expected — a chain may create a record and then write it.
+    ///
+    /// **But only two of the three tiers are answered once.**
+    /// `document_generation` and `layout_version` describe the *document*, so
+    /// answering them on the frame's first mutation is right. The **record
+    /// tier is asked of every mutation**, because which records are at stake
+    /// is a property of the command rather than of the frame — skipping it
+    /// for later commands let one client's chain overwrite a record another
+    /// had moved, silently. The exemption that keeps data-dependent chains
+    /// working is narrower than "later commands are not checked": a record
+    /// that **neither exists nor is claimed** is one the frame is about to
+    /// create, and only that combination is exempt. A record that exists is
+    /// answered for however the claim is written. `CheckedBatch` in
+    /// `improvise-command` carries the derivation.
+    ///
+    /// **All or nothing is the contract and is not yet delivered.** That is
+    /// the other way a frame stops half-way, and it is not a refusal: a chain
+    /// whose later command *fails at apply time* leaves its predecessors
+    /// applied, because `App::apply_effects` aborts without undoing.
+    /// `CheckedBatch` carries the four clauses and the open hole.
     Commands(Vec<Command>),
+    /// Write the document back to wherever this host keeps it
+    /// (improvise-368h).
+    ///
+    /// **A request, not an instruction, and it carries no path** — because a
+    /// client cannot see the host's storage. `effect::Save` writes to *this
+    /// host's* `file_path` through *this host's* `Storage` (gotcha 2), and
+    /// which document a host is hosting is the host's own fact; a client
+    /// naming a path would be naming a namespace it cannot read, the same
+    /// mistake `ModelCommand::SetCell` avoids by taking a surrogate rather
+    /// than a `CellKey` (ADR 002).
+    ///
+    /// **It is a frame and not a [`Command`], and the reason is atomicity.**
+    /// It began as a `Command`, which put a *runtime-fallible* operation
+    /// inside a batch that [`ClientFrame::Commands`] guarantees is applied
+    /// whole or not at all — so `[Save, SetCell]` reported a failed write and
+    /// then applied the edit anyway, and a client reading "refused" could not
+    /// tell which. Saving also fails the test every `Command` passes: it
+    /// mutates no shared state and no client's slice, so `reduce_view` has
+    /// nothing to fold and `Checked` has no basis to answer for. Here it
+    /// cannot be composed with a mutation at all — the batching question
+    /// stops being a rule and becomes unconstructible — and it sits beside
+    /// [`ClientFrame::Viewport`], which is the other session input no
+    /// `Command` carries.
+    ///
+    /// A host that cannot honour it answers [`ServerFrame::Rejected`] with
+    /// [`Rejection::NoDocumentPath`] or [`Rejection::SaveFailed`], addressed
+    /// to the asker alone. Success is silent: the document did not move, so
+    /// no widget is dirty and nobody else is owed anything.
+    Save,
     /// A keypress this client could not resolve. The host runs it through the
     /// keymap against the authoritative `App`.
     ///
@@ -135,8 +193,15 @@ pub enum ServerFrame {
     /// `GridUpdate::Cells` that overtakes its labels is dropped (gotcha 33)
     /// — so a transport must deliver a batch whole and in sequence.
     Commands(Vec<Command>),
-    /// The last batch was composed against a picture of the document that
-    /// had moved, and nothing was applied (ADR 001, improvise-fkfg).
+    /// Nothing was applied, and here is why.
+    ///
+    /// Two causes, both refusals: the batch was composed against a picture of
+    /// the document that had moved (ADR 001, improvise-fkfg), or it was a
+    /// [`ClientFrame::Save`] this host cannot honour (improvise-368h). They
+    /// share a frame because they are the same message — nothing happened,
+    /// addressed to the asker alone — and the [`Rejection`] says which, so
+    /// its variants are the list to match exhaustively rather than this
+    /// sentence. A third will arrive with transactional rollback.
     ///
     /// A third variant because `Sessions::apply` grew a third case, exactly
     /// as `Snapshot`/`Commands` is. It is addressed to the one client that
@@ -176,6 +241,7 @@ mod tests {
                 KeyModifiers::CONTROL | KeyModifiers::ALT,
             )),
             ClientFrame::key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)),
+            ClientFrame::Save,
         ] {
             let json = serde_json::to_string(&frame).expect("serializes");
             assert_eq!(

crates/improvise-wasm-client/src/keys.rs

@@ -363,8 +363,10 @@ fn step(state: &ClientState, event: KeyEvent, name: &str, args: &[String]) -> Op
 
         // ── the view's shape ─────────────────────────────────────────
         //
-        // `TransposeAxes` also emits `mark_dirty`, which is host-local
-        // bookkeeping and has no wire variant (gotcha 45). The cursor reset
+        // `TransposeAxes` used to be emitted alongside a `mark_dirty` this
+        // client had no wire variant for; since improvise-gv9r dirtiness is
+        // raised by the host off the effect's own `EffectKind`, so there is
+        // nothing here to mirror and nothing missing. The cursor reset
         // is not this client's to send either: the effect calls
         // `App::reset_cursor`, and `adopt_cursor_epoch` broadcasts
         // `Command::ResetCursor` to *everyone* in answer (gotcha 24) — so

crates/improvise-wasm-client/src/socket.rs

@@ -52,6 +52,13 @@ fn rejection_message(rejection: &Rejection) -> String {
         Rejection::RecordUnstated(_) => {
             "your edit named a row this client had not been told about — it was not applied".into()
         }
+        // Not staleness, so neither of these offers a retry: retrying changes
+        // nothing about the host's storage. They are worded as facts about
+        // the host because that is what they are (improvise-368h).
+        Rejection::NoDocumentPath => {
+            "this host has nowhere to write the document — it was not saved".into()
+        }
+        Rejection::SaveFailed => "the document could not be written".into(),
     }
 }
 

crates/improvise-wasm-client/tests/browser_round_trip.rs

@@ -236,3 +236,91 @@ fn a_resize_reprojects_the_grid_at_the_new_extent() {
     );
     assert_eq!(client.model().col_labels, vec!["Revenue", "Cost"]);
 }
+
+/// improvise-955n: **the one real producer of a multi-command frame must
+/// still be accepted.**
+///
+/// `CheckedBatch::against` answers a frame's basis before applying any of it,
+/// and every mutation in the frame answers for its own records. Getting that
+/// check right is the easy half; the expensive half is not tightening it
+/// until the browser stops working. A commit keypress is exactly the shape
+/// at risk — one `SetCell` plus the mode change, the scroll offsets and the
+/// re-seeded buffer around it — and it is produced by `keys.rs::commit`,
+/// which no test in `improvise-command` can reach.
+///
+/// So this drives a **real keypress** through a **real `Host`** and asserts
+/// the frame is accepted *and* lands: `ServerFrame::Rejected` anywhere in
+/// the answer, or an unchanged cell, is the regression. Asserting only "no
+/// rejection" would pass against a host that dropped the frame on the floor.
+#[test]
+fn a_commit_keypress_is_one_mutation_among_view_commands_and_is_accepted() {
+    use improvise_protocol::ServerFrame;
+
+    let mut host = host();
+    let mut client = Client::new();
+    let handshake: ClientFrame = serde_json::from_str(&client.set_viewport(10, 10))
+        .expect("the viewport frame must be one the server reads");
+    let ClientFrame::Viewport { rows, cols } = handshake else {
+        panic!("set_viewport must produce a Viewport frame");
+    };
+    let (id, snapshot) = host.connect(rows, cols);
+    deliver(&mut client, &[snapshot]);
+
+    // Type into the cell under the cursor the way the page does: enter edit
+    // mode, type, commit.
+    for key in ["i", "8", "8", "8"] {
+        if let Some(frame) = client.on_key_event(key, false, false, false, false) {
+            send(&mut host, id, &frame);
+        }
+    }
+    let commit = client
+        .on_key_event("Enter", false, false, false, false)
+        .expect("a commit must compose a frame");
+
+    // The shape under test: several commands, exactly one of them a
+    // mutation. Asserted rather than assumed, because if `commit` ever
+    // stopped batching, this test would still pass while testing nothing.
+    let parsed: ClientFrame = serde_json::from_str(&commit).expect("the commit frame parses");
+    let ClientFrame::Commands(cmds) = &parsed else {
+        panic!("a commit must be a Commands frame, got {parsed:?}");
+    };
+    assert!(cmds.len() > 1, "the shape under test is a batch: {cmds:?}");
+    // **A characterization of what `keys::commit` emits, not a protocol
+    // constraint.** A frame may carry more than one mutation; this asserts
+    // the shape actually produced, so that if `commit` ever started batching
+    // two the test above would be exercising something else and should be
+    // re-read rather than silently re-scoped.
+    assert_eq!(
+        cmds.iter()
+            .filter(|c| matches!(c, improvise_protocol::Command::Model { .. }))
+            .count(),
+        1,
+        "keys::commit is expected to emit exactly one mutation: {cmds:?}"
+    );
+
+    let answer = send(&mut host, id, &commit);
+    assert!(
+        !answer
+            .iter()
+            .any(|a| matches!(a.msg, ServerFrame::Rejected(_))),
+        "the batch a real keypress produces must not be refused: {answer:?}"
+    );
+    assert_eq!(
+        host.app()
+            .model_state
+            .workbook
+            .model
+            .get_cell(&CellKey::new(vec![
+                ("Region".into(), "North".into()),
+                ("_Measure".into(), "Revenue".into()),
+            ])),
+        // `i` seeds the edit buffer from the cell it lands on, exactly as
+        // the TUI does, so typing 888 after it appends rather than replaces.
+        // Asserting the *appended* value is what makes this a round trip
+        // rather than a coincidence: 888 alone would also be produced by a
+        // client that ignored the seed.
+        Some(&CellValue::Number(1_200_888.0)),
+        "and it must actually land — 'not refused' alone would pass against \
+         a host that dropped the frame"
+    );
+}

crates/improvise-worker-server/src/worker.rs

@@ -62,31 +62,54 @@
 //! the projection, one at a time, not by wiring this arm up. See
 //! `improvise-wasm-client::keys`, which carries the list.
 //!
-//! # Persistence is the worker's, because saving is not on the wire
+//! # Persistence is the worker's, whether or not anybody asks
 //!
-//! There is no `Command::Save` and no `ModelCommand::Save`, and that is not an
-//! oversight: `effect::Save` is `EffectKind::ViewOnly` (gotcha 3 — it writes
-//! the document to *this host's* file path and clears *this host's* dirty
-//! flag; it mutates no document state), so the derivation that produces
+//! There is no `ModelCommand::Save`, and that is not an oversight:
+//! `effect::Save` is `EffectKind::ViewOnly` (gotcha 3 — it writes the
+//! document to *this host's* file path and clears *this host's* dirty flag;
+//! it mutates no document state), so the derivation that produces
 //! `ModelCommand` from the model effects cannot reach it
 //! (`decision(improvise-cqi, command_set_derived_not_invented)`, gotcha 38).
-//! **Consequently no wire client — browser or socket — can ask its host to
-//! save.** That is a real gap and it is reported rather than patched: adding a
-//! wire variant for it is a protocol decision, not a worker one.
+//! Saving was therefore unreachable from any client on any transport until
+//! improvise-368h added `ClientFrame::Save` — a *request to the host*, framed
+//! rather than commanded precisely because it mutates nothing shared.
 //!
-//! It does not block the browser, because in a browser it is the **wrong
-//! question**. There is no file the user owns and no Ctrl-S semantics to
+//! **A browser still never sends it**, and that is the right answer rather
+//! than a gap: there is no file the user owns and no Ctrl-S semantics to
+//! honour, so asking would be the **wrong question**. There is no file the user owns and no Ctrl-S semantics to
 //! honour: the document *is* the OPFS entry, and a page that loses an edit
 //! because nobody pressed a key has simply lost it. So the worker persists on
 //! its own behalf: [`Worker::persist`] runs the one existing `Save` effect
 //! through `App::apply_effects`, so effects remain the only mutation site and
-//! the save path is not duplicated, after any frame [`touches_document`]
-//! accepts and the host did not refuse. Two costs, both stated rather than
-//! hidden: `Storage` is whole-document by construction (gotcha 41), so OPFS
-//! rewrites the file per accepted model command — correct, not yet cheap; and
-//! the trigger is the command *variant* rather than an observation of the
-//! document, because the flag that would be the observation is never set on
-//! this path. [`touches_document`] carries that argument.
+//! the save path is not duplicated.
+//!
+//! **The trigger is an observation of the document, not a table of command
+//! variants.** It used to be the latter, and the reason was honest: a wire
+//! `ModelCommand::SetCell` reached `effect::SetCell`, which did not raise
+//! `model_state.dirty` — that was a separate `MarkDirty` effect only the key
+//! path composed — so "save if dirty" was a trigger that never fired
+//! (improvise-gv9r). With dirtiness raised by `App::apply_effects` off the
+//! same `EffectKind` the `Dirty` is, the flag *is* the observation, and
+//! asking it is better than enumerating variants for the same reason the
+//! cursor reset watches `cursor_epoch` rather than matching on `CycleAxis`
+//! (gotcha 24) and the rebase watches `document_generation` rather than
+//! matching on `LoadModel` (gotcha 39): a table of variants is a second
+//! description of which commands change the document, free to disagree with
+//! the first. It is also strictly tighter — a refused batch changes nothing,
+//! so it raises nothing, and the "was anything rejected" special case that
+//! used to guard this went with it.
+//!
+//! One cost remains and is stated rather than hidden: `Storage` is
+//! whole-document by construction (gotcha 41), so OPFS rewrites the file per
+//! accepted model command — correct, not yet cheap. And a failed write leaves
+//! the flag raised, so the next frame retries: right for a transient failure,
+//! and harmless for a permanent one (a worker with no `file_path`) because the
+//! only thing a failed `Save` produces is a status line, and nothing in a
+//! worker renders one.
+//!
+//! So this is the browser's own answer, and `ClientFrame::Save` is the other
+//! one — for a socket client editing a TUI-hosted document, where there *is*
+//! a file somebody owns.
 //!
 //! # Ordering
 //!
@@ -101,36 +124,7 @@ use improvise_command::ui::app::App;
 use improvise_command::ui::effect;
 use improvise_command::ui::host::{Addressed, Host};
 use improvise_command::ui::project::SessionId;
-use improvise_protocol::{ClientFrame, Command, ServerFrame};
-
-/// Whether a frame can have changed the document, and so whether the worker
-/// owes it a write.
-///
-/// The two variants `Sessions::apply` routes to the document (gotchas 36, 38):
-/// a `Model` command is a document mutation by construction, and a
-/// `SwitchView` moves `Workbook::active_view`, which persists as the
-/// `Initial View` line. Everything else in `Command` is the asking client's
-/// own view slice and belongs to nobody else, let alone to storage.
-///
-/// **It is a per-variant question, not a per-batch observation, and that is a
-/// deliberate over-approximation.** The obvious trigger — "save if
-/// `model_state.dirty`" — does not work, and the reason is worth knowing: a
-/// wire `ModelCommand::SetCell` goes through `reduce::model_effects` to
-/// `effect::SetCell`, which writes the cell and **does not** set the flag.
-/// Dirtiness is raised by a separate `MarkDirty` effect that the *command*
-/// layer emits on the key path, and no wire command carries one. So the
-/// authoritative document is never marked dirty by a client's edit on any
-/// transport — reported as a gap rather than papered over here. Over-saving
-/// costs one redundant write; keying off a flag that is never set would have
-/// lost every client edit, silently.
-fn touches_document(frame: &ClientFrame) -> bool {
-    match frame {
-        ClientFrame::Commands(cmds) => cmds
-            .iter()
-            .any(|cmd| matches!(cmd, Command::Model { .. } | Command::SwitchView(_))),
-        ClientFrame::Viewport { .. } | ClientFrame::Key { .. } => false,
-    }
-}
+use improvise_protocol::{ClientFrame, ServerFrame};
 
 /// One document, one port, one client.
 ///
@@ -172,10 +166,19 @@ impl Worker {
             // nothing to apply and nothing to answer.
             (None, _) => Vec::new(),
             (Some(id), frame) => {
-                let touched = touches_document(&frame);
+                // The one frame this does not apply to: a client that asked
+                // to save has already been answered, and a failed write
+                // leaves `dirty` set — so retrying here would repeat the I/O
+                // and, on a transient failure, tell the client `SaveFailed`
+                // about a document that is now on disk. The automatic write
+                // exists for edits *nobody asked* to save.
+                let asked = matches!(frame, ClientFrame::Save);
                 let out = self.frames_for(id, frame);
-                let refused = out.iter().any(|f| matches!(f, ServerFrame::Rejected(_)));
-                if touched && !refused {
+                // Otherwise the document itself is asked, never the frame.
+                // See the module doc: a refused batch changed nothing and so
+                // raises nothing, and `Save` clears the flag, so this neither
+                // double-writes nor needs a second guard.
+                if !asked && self.host.app().model_state.dirty {
                     self.persist();
                 }
                 out
@@ -493,6 +496,141 @@ mod tests {
         );
     }
 
+    /// improvise-955n's **second-order effect**, which nobody had written
+    /// down until it was found while declining the ticket: `on_frame` skipped
+    /// `persist()` when any frame in the answer was `Rejected`, so a batch
+    /// that *did* apply its earlier command before being refused left the
+    /// document moved in memory and not written back — disk and worker
+    /// disagreeing, silently.
+    ///
+    /// Two changes close it independently, which is why this is worth a test
+    /// of its own rather than trusting either: the batch no longer applies
+    /// partially at all (improvise-955n), and the persist trigger is now the
+    /// document's own dirtiness rather than "was anything rejected"
+    /// (improvise-gv9r), so a batch that *did* change something would be
+    /// written whatever else the answer contained.
+    ///
+    /// The fixture is a refused frame with a command behind the refusal, and
+    /// the assertion is that the axes on disk and the axes in memory agree.
+    /// Under the settled contract the refusal comes from the **frame's**
+    /// basis — answered once, on the first command that carries one — so
+    /// that is where the staleness is injected; a later command carries no
+    /// answer of its own.
+    #[test]
+    fn a_partly_refused_batch_does_not_leave_disk_and_memory_disagreeing() {
+        let (mut worker, path) = worker_over_storage();
+        connect(&mut worker);
+
+        // The frame's basis is the FIRST command's, answered once (clause 2),
+        // so that is where the staleness must be injected: a later command
+        // carries no answer of its own any more.
+        let Command::Model { mut basis, cmd } =
+            reduce::caught_up(worker.app(), ModelCommand::TransposeAxes)
+        else {
+            unreachable!("caught_up builds a Command::Model")
+        };
+        basis.document_generation += 1;
+        let stale = Command::Model { basis, cmd };
+        let follows = reduce::caught_up(
+            worker.app(),
+            ModelCommand::AddItem {
+                category: "Region".to_string(),
+                item: "East".to_string(),
+            },
+        );
+
+        let out = worker.on_frame(ClientFrame::Commands(vec![stale, follows]));
+        assert!(
+            matches!(out.as_slice(), [ServerFrame::Rejected(_)]),
+            "the fixture must actually be refused, got {out:?}"
+        );
+
+        let reopened = vfs::open(&*worker.app().storage, &Opening::named(&path))
+            .expect("the document reopens through the same VFS");
+        assert_eq!(
+            reopened.active_view().category_axes,
+            worker
+                .app()
+                .model_state
+                .workbook
+                .active_view()
+                .category_axes,
+            "the worker moved the document and never wrote it back"
+        );
+    }
+
+    /// An explicit `ClientFrame::Save` is **not** also auto-persisted
+    /// (found by Copilot on PR #26).
+    ///
+    /// A failed write leaves `model_state.dirty` set — deliberately, so the
+    /// next frame retries — and the automatic trigger runs after *every*
+    /// frame. A save frame therefore attempted the same write twice: extra
+    /// I/O on a permanent failure, and on a transient one something worse,
+    /// because the answer is composed from the *first* attempt, so the client
+    /// is told `SaveFailed` about a document that is now on disk.
+    ///
+    /// The fix is a per-frame question rather than a per-document one, and
+    /// that is the right shape here precisely because the automatic write
+    /// exists for edits **nobody asked** to save; one that was asked for has
+    /// already been answered.
+    #[test]
+    fn an_explicit_save_is_not_retried_by_the_automatic_write() {
+        // A backend whose writes always fail, and that says how many were
+        // attempted. **Counting is the point**: "it was refused" is true
+        // whether the write was attempted once or twice, so a test asserting
+        // only the refusal would pass against the bug. **Failing is the
+        // point too**: a successful save clears `dirty`, so the redundant
+        // second attempt never happens and the bug is invisible from the
+        // happy path.
+        let counter = std::rc::Rc::new(std::cell::Cell::new(0usize));
+        struct CountingFailingFs(std::rc::Rc<std::cell::Cell<usize>>);
+        impl improvise_command::storage::Storage for CountingFailingFs {
+            fn read(&self, _path: &std::path::Path) -> anyhow::Result<Vec<u8>> {
+                anyhow::bail!("no")
+            }
+            fn write(&self, _path: &std::path::Path, _bytes: &[u8]) -> anyhow::Result<()> {
+                self.0.set(self.0.get() + 1);
+                anyhow::bail!("this backend never writes")
+            }
+            fn list(&self, _dir: &std::path::Path) -> anyhow::Result<Vec<std::path::PathBuf>> {
+                Ok(Vec::new())
+            }
+            fn delete(&self, _path: &std::path::Path) -> anyhow::Result<()> {
+                anyhow::bail!("no")
+            }
+        }
+
+        let mut app = App::new(populated(), Some(std::path::PathBuf::from("doc.improv")));
+        app.storage = Box::new(CountingFailingFs(counter.clone()));
+        app.rebuild_layout();
+        let mut worker = Worker::new(app);
+        connect(&mut worker);
+
+        // Dirty the document, so the automatic trigger genuinely would fire
+        // if it were consulted. That write fails and is counted.
+        let wire = reduce::caught_up(worker.app(), ModelCommand::TransposeAxes);
+        worker.on_frame(ClientFrame::command(wire));
+        assert!(
+            worker.app().model_state.dirty,
+            "a failed automatic write must leave the flag raised, or this \
+             test cannot tell a retry from the absence of one"
+        );
+        let before = counter.get();
+        assert!(before > 0, "the fixture must actually attempt writes");
+
+        let out = worker.on_frame(ClientFrame::Save);
+
+        assert!(
+            matches!(out.as_slice(), [ServerFrame::Rejected(_)]),
+            "the save must still be refused, got {out:?}"
+        );
+        assert_eq!(
+            counter.get() - before,
+            1,
+            "one save frame must attempt exactly one write"
+        );
+    }
+
     /// A resize changes no document state, but it moves this client's window,
     /// so it must still be answered with the grid the new extent can show.
     #[test]