fix: SwitchView is a document mutation, and a save frame is not retried

Copilot's third round of PR #26, and the first finding is the **third** time
the same mistake has been caught here: an argument that holds for the command
that came to mind.

**1. `SwitchView` counts.** I excluded it on the grounds that it carries no
basis and a record surrogate is view-independent, so a switch cannot
invalidate an edit beside it. That is true of `SetCell` and `ClearCell` — and
false of *every other* model command, because `TransposeAxes`, `SetAxis`,
`HideItem`, `SetNumberFormat` and their siblings all act on the **active
view**. So `[SwitchView("Second"), TransposeAxes]` passed both checks and
then transposed `Second` rather than the view the transpose was composed
against.

The rule is now "at most one **document mutation** per frame", where
`mutates_the_document` is the two `Command` variants `Sessions::apply` routes
away from the mirror. Deliberately the *document* question rather than "does
it carry a basis", which is exactly the distinction I got wrong. No client
here batches a `SwitchView` with anything, so it costs nothing.

**2. An explicit `ClientFrame::Save` is not also auto-persisted.** A failed
write leaves `dirty` set — deliberately, so the next frame retries — and the
worker's automatic trigger runs after *every* frame. A save frame therefore
attempted the same write twice. On a permanent failure that is duplicate I/O;
on a transient one it is worse, because the answer was composed from the
first attempt, so the client was told `SaveFailed` about a document that was
now on disk. A per-frame question is the right shape here precisely because
the automatic write exists for edits **nobody asked** to save.

**3.** `Rejection`'s summary line now says it answers a *frame*, since two of
its three classes are not refused commands at all.

Evidence, both faults injected:

- `a_frame_carrying_two_document_mutations_is_refused_whole` gains a third
  shape. Dropping `SwitchView` from `mutates_the_document` fails it.
- `an_explicit_save_is_not_retried_by_the_automatic_write` asserts the
  **number of write attempts**, which took a counting, always-failing
  `Storage`. Both properties are load-bearing: "it was refused" is true
  whether the write happened once or twice, and a *successful* save clears
  `dirty` so the redundant attempt never happens — the bug is invisible from
  both the refusal and the happy path. Restoring the retry gives 2 where the
  test requires 1.

`a_refused_batch_leaves_the_commands_before_it_unapplied` now leads with
`SetSelected` rather than `SwitchView`, which the new rule would refuse
first. That is not a concession: a view command is the shape a real frame
has, and it is still observable in the asker's mirror.

1208 passing; clippy, fmt and the wasm gate clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

@@ -643,35 +643,28 @@ mod tests {
     /// 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 a real frame has, since a frame may carry
+    /// at most one document mutation, and it is still observable — the host
+    /// folds it into that client's mirror.
+    ///
     /// **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
-    /// `SwitchView` followed by a command whose basis is *current*, and
-    /// requires both to land. Without it, a `SwitchView` that simply never
+    /// `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;
 
-        // A host whose document has somewhere to switch *to*: the earlier
-        // command must be one the document would really accept, or "it did
-        // not apply" is true for the wrong reason.
-        let two_views = || {
-            let mut host = host();
-            host.app_mut()
-                .model_state
-                .workbook
-                .views
-                .insert("Second".to_string(), crate::view::View::new("Second"));
-            host
-        };
         let edit = |value: f64| ModelCommand::SetCell {
             cell: addressed("North"),
             value: CellValue::Number(value),
         };
 
-        let mut refused = two_views();
+        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
@@ -687,13 +680,19 @@ mod tests {
         let stale = Command::Model { basis, cmd };
         let answer = refused.handle(
             asker,
-            ClientFrame::Commands(vec![Command::SwitchView("Second".into()), stale]),
+            ClientFrame::Commands(vec![Command::SetSelected { row: 1, col: 0 }, stale]),
         );
 
         assert_eq!(
-            refused.app().model_state.workbook.active_view,
-            "Default",
-            "the command before the refused one reached the document, so the \
+            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!(
@@ -706,18 +705,33 @@ mod tests {
 
         // The other history: the same leading command, a basis that is
         // current, and both must land.
-        let mut accepted = two_views();
+        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::SwitchView("Second".into()), fresh]),
+            ClientFrame::Commands(vec![Command::SetSelected { row: 1, col: 0 }, fresh]),
         );
         assert_eq!(
-            accepted.app().model_state.workbook.active_view,
-            "Second",
+            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 half Copilot found on PR #26 — twice, because the
@@ -740,9 +754,13 @@ mod tests {
     /// may carry at most one `Command::Model`**. The first fix refused only
     /// the document-*replacing* case, which is the same bug seen through one
     /// variant — `[LoadModel, TransposeAxes]` — instead of through the rule.
-    /// Both shapes are driven here, because a guard written for one of them
-    /// is what got us here twice
+    /// **Three** shapes are driven here, because a guard written for the one
+    /// that came to mind is what got this wrong three times running
     /// (`doctrine(one_arm_of_a_symmetric_rule_is_evidence_about_that_arm_only)`).
+    /// The third is `SwitchView`, which a first version excluded on the
+    /// grounds that a record surrogate is view-independent — true of
+    /// `SetCell` and `ClearCell` and false of every other model command,
+    /// because they all act on the *active view*.
     ///
     /// The accepting arms are the point of the test as much as the refusals:
     /// one mutation surrounded by view commands must land whole, or
@@ -810,6 +828,37 @@ mod tests {
             "the document must not have been replaced"
         );
 
+        // Shape 3: `SwitchView` is a document mutation too, although it
+        // carries no basis. Every model command *except* the two that name a
+        // record by surrogate acts on the **active view**, so a switch
+        // beside one of them redirects it: this pair transposed `Second`
+        // rather than the view the transpose was composed against.
+        let mut switcher = host();
+        switcher
+            .app_mut()
+            .model_state
+            .workbook
+            .views
+            .insert("Second".to_string(), crate::view::View::new("Second"));
+        let (asker, _) = switcher.connect(10, 10);
+        let reshape = crate::ui::reduce::caught_up(switcher.app(), ModelCommand::TransposeAxes);
+        let answer = switcher.handle(
+            asker,
+            ClientFrame::Commands(vec![Command::SwitchView("Second".into()), reshape]),
+        );
+        assert!(
+            matches!(
+                to(&answer, asker).as_slice(),
+                [ServerFrame::Rejected(Rejection::FrameNotAtomic)]
+            ),
+            "{answer:?}"
+        );
+        assert_eq!(
+            switcher.app().model_state.workbook.active_view,
+            "Default",
+            "nothing in the frame may have been applied"
+        );
+
         // The accepting arm: one mutation, surrounded by the view commands a
         // locally-resolved keypress really does batch with it.
         let mut accepted = host();

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

@@ -127,8 +127,8 @@ pub struct Checked<'a>(&'a Command);
 /// and never during one. So no client can hold a basis describing the
 /// document as it would be halfway through its own frame.
 ///
-/// **That forces the rule: a frame may carry at most one
-/// [`Command::Model`].** Checking every command against the pre-frame `App`
+/// **That forces the rule: a frame may carry at most one document
+/// mutation** — a [`Command::Model`] or a [`Command::SwitchView`].** Checking every command against the pre-frame `App`
 /// is right for the claims that can exist, and it is *only* right while no
 /// command in the frame can invalidate another's. Two document mutations in
 /// one frame break that, and the failure is silent rather than loud:
@@ -151,12 +151,18 @@ pub struct Checked<'a>(&'a Command);
 /// untouched, because a view command carries no basis and cannot invalidate
 /// one.
 ///
-/// `Command::SwitchView` is deliberately **not** counted, although it moves
-/// the document: it carries no basis (ADR 001 names that hole and leaves it
-/// open), and a record surrogate is view-independent, so a switch cannot
-/// invalidate an edit beside it. Counting it would reject the useful frames
-/// for staleness their own siblings caused, which is the mistake in the
-/// other direction.
+/// **`Command::SwitchView` counts too**, although it carries no basis of its
+/// own (ADR 001 names that hole and leaves it open). A first version excluded
+/// it, on the grounds that a record surrogate is view-independent so a switch
+/// cannot invalidate an edit beside it — true of `SetCell` and `ClearCell`,
+/// and **false of every other model command**, because `TransposeAxes`,
+/// `SetAxis`, `HideItem`, `SetNumberFormat` and their siblings all act on the
+/// *active view*. `[SwitchView("Second"), TransposeAxes]` passed both checks
+/// and then transposed `Second` rather than the view the transpose was
+/// composed against. Copilot found that one too, which makes three rounds of
+/// the same mistake: an argument that holds for the command that came to mind.
+/// The rule is now "a document mutation", not "a `Command::Model`", and what
+/// is left uncounted is only what genuinely cannot reach the document.
 ///
 /// With that, atomicity is not an argument: at most one command in a frame
 /// can be refused, and it is refused before anything is applied.
@@ -176,10 +182,7 @@ impl<'a> CheckedBatch<'a> {
     /// no basis can describe — there is nothing to check it *against* before
     /// deciding that.
     pub fn against(app: &App, cmds: &'a [Command]) -> Result<Self, Rejection> {
-        let mutations = cmds
-            .iter()
-            .filter(|cmd| matches!(cmd, Command::Model { .. }))
-            .count();
+        let mutations = cmds.iter().filter(|cmd| mutates_the_document(cmd)).count();
         match mutations > 1 {
             true => Err(Rejection::FrameNotAtomic),
             false => cmds
@@ -191,6 +194,18 @@ impl<'a> CheckedBatch<'a> {
     }
 }
 
+/// Does this command change the shared document?
+///
+/// The two `Command` variants `Sessions::apply` routes away from the mirror
+/// (gotchas 36, 38), and it is deliberately the *document* question rather
+/// than "does it carry a basis": `SwitchView` carries none and still moves
+/// `Workbook::active_view`, which every view-config model command is
+/// relative to. Anything else in the vocabulary is the asking client's own
+/// view slice and cannot invalidate a sibling's claim.
+fn mutates_the_document(cmd: &Command) -> bool {
+    matches!(cmd, Command::Model { .. } | Command::SwitchView(_))
+}
+
 impl<'a> IntoIterator for CheckedBatch<'a> {
     type Item = Checked<'a>;
     type IntoIter = std::vec::IntoIter<Checked<'a>>;

crates/improvise-protocol/src/basis.rs

@@ -89,7 +89,12 @@ impl Basis {
     }
 }
 
-/// Why a command was refused, and nothing happened.
+/// Why a **frame** was refused, and nothing happened.
+///
+/// The frame, not the command: since improvise-368h and improvise-955n this
+/// also answers a `ClientFrame::Save` the host cannot honour and a
+/// `ClientFrame::Commands` no single basis can describe, neither of which is
+/// a refused *command*.
 ///
 /// A **normal outcome**, not an error: the client already holds the new
 /// state through projections and re-composes against it.

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

@@ -166,12 +166,19 @@ impl Worker {
             // nothing to apply and nothing to answer.
             (None, _) => Vec::new(),
             (Some(id), 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);
-                // 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
+                // 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 self.host.app().model_state.dirty {
+                if !asked && self.host.app().model_state.dirty {
                     self.persist();
                 }
                 out
@@ -552,6 +559,78 @@ mod tests {
         );
     }
 
+    /// 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]