Merge pull request #163 from fiddlerwoaroof/fix/k84e-rebase-rollback

Test clause 4 over the Applied::Rebased path (improvise-k84e)

context/test-evidence/command.md

@@ -4852,3 +4852,54 @@ The test also asserts `RemoveItem::kind()` is `Model(ModelChange::CellData)`
 before applying it, so reclassifying that effect fails here rather than silently
 turning this into a test of the `LayoutGeometry` arm that its sibling already
 covers.
+
+### ui::host::tests::a_rolled_back_load_tells_nobody_and_puts_every_mirror_back
+
+improvise-k84e, scoped to what remained after PR #109: the `Applied::Rebased`
+path proper. A frame whose `LoadModel` **succeeds**, emits a `Snapshot` to every
+client, and is *then* rolled back by a later command in the same frame.
+
+`a_rolled_back_load_leaves_no_ghost_view_and_no_false_status` already covers the
+document half — `active_view` and `status_msg` go back. This covers the
+**`Sessions`** half, which nothing touched: `mutating_frame` clones the whole
+`Sessions` before the batch and `undo_frame` puts the clone back, so the reseed
+`rebase` performed is undone with it and the computed `Snapshot`s are discarded
+rather than sent.
+
+It reproduced nothing — the mechanism is sound, which is what the bead predicted
+("should be sound", the phrasing this repo's gotcha list is made of). It is a
+regression guard, and its value is entirely in the fault it can see.
+
+Fault injected: `self.sessions = mirrors;` replaced by `let _ = mirrors;` in
+`undo_frame`. The test failed on the **projection** assertion, not its setup,
+naming the asker's mirror and printing the whole correcting batch — a `RowFrame`
+of `Region`/`North`/`South` and a `Document` of `"Test"`, i.e. the restored
+document being *re-sent* to a client whose mirror had been left holding the
+discarded one. That is precisely the user-visible symptom: a client hearing
+about a load that never happened. Reverted.
+
+Three things make it non-vacuous:
+
+- **The control is the load-bearing half**
+  (`the_load_this_fixture_performs_rebases_every_client`). If the fixture's
+  `LoadModel` silently failed, `Applied::Rebased` would never run and every
+  assertion here would pass over nothing. The control asserts the generation
+  moves and that *both* clients are owed a `Snapshot`.
+- **The rollback is asserted before anything is concluded from it** — the
+  `document_generation` equality is checked first, so "the frame was rejected"
+  cannot be confused with "the frame never got that far".
+- **A bystander is seated where a wrong relay could move it.** A single-client
+  fixture cannot tell "the mirrors were restored" from "there was one mirror and
+  it happened to be the asker's".
+
+The projections are compared as an emptiness claim over `Projection`s rather
+than over `Addressed`, because `project_all` can address a client with an empty
+`ServerFrame::Commands`; asserting on the `Addressed` list would pass for the
+wrong reason.
+
+### ui::host::tests::the_load_this_fixture_performs_rebases_every_client
+
+The control for the test above, and it is stated separately rather than folded
+in so that a change making the fixture's load stop rebasing fails *here*, where
+the message says what actually broke, rather than turning the rollback test
+green by emptying it.

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

@@ -1097,6 +1097,106 @@ mod tests {
         );
     }
 
+    /// Everything one client was told, flattened to the projections it carries.
+    fn projections(frames: &[Addressed], id: SessionId) -> Vec<Projection> {
+        to(frames, id)
+            .into_iter()
+            .flat_map(|msg| match msg {
+                ServerFrame::Commands(cmds) => cmds,
+                _ => Vec::new(),
+            })
+            .filter_map(|cmd| match cmd {
+                Command::Project(p) => Some(p),
+                _ => None,
+            })
+            .collect()
+    }
+
+    /// The control for the test below: the fixture's load really does rebase,
+    /// so a rollback of it has something to undo.
+    #[test]
+    fn the_load_this_fixture_performs_rebases_every_client() {
+        let (mut host, path) = host_over_a_loadable_document();
+        let (asker, _) = host.connect(10, 10);
+        let (bystander, _) = host.connect(10, 10);
+        let was = host.app().document_generation();
+
+        let answer = host.model(asker, ModelCommand::LoadModel(path));
+
+        assert_eq!(
+            host.app().document_generation(),
+            was + 1,
+            "the fixture's load did not replace the document: {answer:?}"
+        );
+        for (who, id) in [("asker", asker), ("bystander", bystander)] {
+            assert!(
+                to(&answer, id)
+                    .iter()
+                    .any(|m| matches!(m, ServerFrame::Snapshot(_))),
+                "{who} was owed a fresh Snapshot by the rebase: {answer:?}"
+            );
+        }
+    }
+
+    /// Clause 4 over the `Applied::Rebased` path: the Snapshots were computed
+    /// and are discarded. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn a_rolled_back_load_tells_nobody_and_puts_every_mirror_back() {
+        let (mut host, path) = host_over_a_loadable_document();
+        let (asker, _) = host.connect(10, 10);
+        let (bystander, _) = host.connect(10, 10);
+        let was_generation = host.app().document_generation();
+        let basis = frame_basis(&host, &["North"]);
+
+        // The load lands and rebases every mirror; the write then names a
+        // record the incoming document has never had, so the frame fails.
+        let answer = host.handle(
+            asker,
+            ClientFrame::Commands(vec![
+                Command::Model {
+                    basis: basis.clone(),
+                    cmd: ModelCommand::LoadModel(path),
+                },
+                Command::Model {
+                    basis,
+                    cmd: ModelCommand::SetCell {
+                        cell: addressed("North"),
+                        value: CellValue::Number(5.0),
+                    },
+                },
+            ]),
+        );
+
+        assert_eq!(
+            host.app().document_generation(),
+            was_generation,
+            "the document did not go back, so nothing below is about a rollback"
+        );
+        assert!(
+            !answer
+                .iter()
+                .any(|a| matches!(a.msg, ServerFrame::Snapshot(_))),
+            "a rolled-back load let its Snapshots out: {answer:?}"
+        );
+        assert!(
+            to(&answer, bystander).is_empty(),
+            "a client that asked for nothing was told about a frame that did \
+             nothing: {answer:?}"
+        );
+
+        // Nothing moved, so the next fan-out owes nobody anything. An
+        // unrestored mirror is corrected here instead — test-evidence.
+        let after = host.project_all();
+        for (who, id) in [("asker", asker), ("bystander", bystander)] {
+            assert!(
+                projections(&after, id).is_empty(),
+                "{who}'s mirror was left describing the discarded document, so \
+                 the next projection corrects it: {:?}",
+                projections(&after, id)
+            );
+        }
+    }
+
     /// `batch_aborted` is reset only inside `apply_effects`, so a frame that
     /// runs none reads a STALE flag. Evidence: `context/test-evidence/command.md`.
     #[test]