Review round 2: the narrowing bought its speed with a correctness hole

Merge of `gh/main` at 8bb1d53 (#83), plus the four findings from the adversarial
review of #78. The P1 is the one that matters, and it is mine.

1 (P1). improvise-mcq4 narrowed the cursor's frame fingerprint from every view
to the active one. I measured that the narrowing moved 12.41ms to 12.56ms, kept
it "as the honest shape", and reported that it bought nothing. It bought a
multi-client invalidation: a `CellData` write that prunes a row out of ANOTHER
client's view became invisible, because `view_shapes` hashes the `prune_empty`
FLAG rather than which rows survive. Gotcha 24's permanently-stale case.

**A measurement showing a change is free is not evidence that it is correct.**
I asked what it cost and reported the answer as though it had also answered
whether it was right.

The arms are now split by the thing that differs: `LayoutGeometry` keeps the
cheap one-view frame (`view_mut()` writes only the active view, and the
structural cases are already covered by `view_shapes`); `CellData` restores the
all-views frame. Measured here, not cited: **0%** on the held-key path, **+5.7%**
on an 18k two-view `SetCell`, scaling linearly in view count. Trade accepted and
recorded on improvise-mcq4 along with the absolute — this PR still roughly
doubles held-key latency on large pruned documents.

**Writing that regression test found the bug that was hiding it.** The
precondition failed: `project::grid_in` built a non-active view with
`GridLayout::new`, which does not apply `prune_empty`, so a bystander was
projected three rows where its own config gives two. Its grid never lost a row,
so the staleness was unobservable — a bug hiding a bug, and the THIRD instance of
the `new`-versus-`with_frozen_records` trap. Fixed; broke no other test. Filed as
improvise-t6db because the finding outlives the one-line fix.

2 (P2). `frame_basis` was taken once at batch open and compared across a
`SwitchView`, which is `ViewOnly` and so did not re-base it — the next model
effect hashed the new view against the old view's value. It is keyed by view name
now and re-bases when the name moves.

3 (P2). The `drill_state.is_none()` guard on the free opening basis was correct,
load-bearing, and tested by nothing — reverting it left all tests green. It has
exactly one test now, and removing the guard fails that test and nothing else.

4 (P3). `assert_eq!(cases.len(), 13)` claimed a totality it does not have; the
reviewer added a fourteenth effect and the suite stayed green. The comment now
says it is a count. Filed under improvise-5ubs as a fourth instance — inside the
PR that found improvise-5ubs.

THREE CORRECTIONS TO MY OWN REPORTING, all understatements:
- Disabling the observation fails **13** tests, not the ten I claimed (the review
  said 12; the P1 probe adds one).
- Moving the check after the loop: with the arms now split, moving the
  `LayoutGeometry` arm alone fails 2; the review measured 4 against the unified
  arm, and both are more than the three I named.
- `4c55341`'s subject said "Merge #81" and it was single-parent; the real merge is
  `ec236da`. Subject corrected by amend.

1456 tests. Both spellings of the comment-run script run separately. Test function
names diffed by name across the merge including `harness/`: nothing lost.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH

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

@@ -119,6 +119,16 @@ fn frame_of(layout: &crate::view::layout::GridLayout, view: &View) -> u64 {
     h.finish()
 }
 
+/// Every view's drawn frame, keyed by view name. The `CellData` arm's basis:
+/// a cell write can prune a row out of a view nobody is looking at.
+fn view_frames(workbook: &Workbook) -> std::collections::BTreeMap<String, u64> {
+    workbook
+        .views
+        .iter()
+        .map(|(name, view)| (name.clone(), view_frame(&workbook.model, view)))
+        .collect()
+}
+
 /// The names whose value differs between two shape maps. The union of the key
 /// sets is the domain: a view gained or lost differs at that name.
 fn differing_keys(
@@ -560,6 +570,10 @@ impl App {
 
     /// The same frame at the START of a batch, where `self.layout` is still the
     /// one this view draws. Stale mid-batch (gotcha 23), so opening use only.
+    fn opening_view_frame_keyed(&self) -> (String, u64) {
+        (self.view_name().to_string(), self.opening_view_frame())
+    }
+
     fn opening_view_frame(&self) -> u64 {
         let (_, view) = self.resolve_view(self.view_state.active_view.as_str());
         match self.drill_state.is_none() {
@@ -712,9 +726,10 @@ impl App {
         // cycle and hide/show must not walk 18,000 cells (improvise-748f).
         let mut content_basis = writes_cells.then(|| document_content(&self.model_state.workbook));
         let mut config_basis = moves_geometry.then(|| document_config(&self.model_state.workbook));
-        // The cursor's own basis, and it is `touches_model` rather than either of
-        // theirs: a `CellData` write under `prune_empty` reframes (improvise-ni6k).
-        let mut frame_basis = touches_model.then(|| self.opening_view_frame());
+        // The cursor's own basis. The cheap one-view form on the held-key path,
+        // and every view where a cell write can prune one — `context/code/command.md`.
+        let mut frame_basis = moves_geometry.then(|| self.opening_view_frame_keyed());
+        let mut frames_basis = writes_cells.then(|| view_frames(&self.model_state.workbook));
         self.last_reframed_views.clear();
         let mut dirty = Dirty::NOTHING;
         for effect in effects {
@@ -753,16 +768,31 @@ impl App {
             }
             // Inside the loop, or a batch that reframes and then places the
             // cursor loses the placement — `context/code/command.md`.
-            if matches!(kind, EffectKind::Model(_))
-                && let Some(basis) = frame_basis
+            if let Some((was, basis)) = frame_basis.as_ref()
+                && matches!(kind, EffectKind::Model(ModelChange::LayoutGeometry))
             {
                 let now = self.active_view_frame();
-                if now != basis {
+                // A `SwitchView` earlier in the batch re-bases rather than compares:
+                // two views' hashes differ for no reason (improvise-mcq4's sibling).
+                if was == self.view_name() && now != *basis {
                     self.reset_cursor();
                     self.last_reframed_views
                         .insert(self.view_name().to_string());
                 }
-                frame_basis = Some(now);
+                frame_basis = Some((self.view_name().to_string(), now));
+            }
+            // A cell write can prune a row out of ANY view, so this arm pays for
+            // every one of them (improvise-mcq4's sting).
+            if let Some(basis) = frames_basis.as_ref()
+                && matches!(kind, EffectKind::Model(ModelChange::CellData))
+            {
+                let now = view_frames(&self.model_state.workbook);
+                let moved = differing_keys(basis, &now);
+                if moved.contains(self.view_name()) {
+                    self.reset_cursor();
+                }
+                self.last_reframed_views.extend(moved);
+                frames_basis = Some(now);
             }
             if self.abort_effects {
                 // An abort-setting effect asked for the rest to be skipped. Reset
@@ -1215,6 +1245,113 @@ mod tests {
         })]);
     }
 
+    /// A records-mode document with several records, so a drill can freeze a
+    /// strictly narrower set than the live query returns.
+    fn records_app_for_drill() -> App {
+        let mut wb = Workbook::new("Drill");
+        wb.add_category("Region").unwrap();
+        for region in ["North", "South", "East"] {
+            wb.add_item("Region", region).unwrap();
+        }
+        wb.add_item("_Measure", "Revenue").unwrap();
+        for region in ["North", "South", "East"] {
+            wb.model.set_cell(
+                CellKey::new(vec![
+                    ("Region".to_string(), region.to_string()),
+                    ("_Measure".to_string(), "Revenue".to_string()),
+                ]),
+                CellValue::Number(1.0),
+            );
+        }
+        {
+            let v = wb.active_view_mut();
+            v.set_axis("_Index", crate::view::Axis::Row);
+            v.set_axis("_Dim", crate::view::Axis::Column);
+            v.set_axis("Region", crate::view::Axis::None);
+            v.set_axis("_Measure", crate::view::Axis::None);
+        }
+        let mut app = App::new(wb, None);
+        app.rebuild_layout();
+        assert!(
+            app.layout.is_records_mode(),
+            "the fixture must be records mode"
+        );
+        app
+    }
+
+    /// The opening basis may be read off `App::layout` only when no drill froze
+    /// it. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn a_frozen_drill_does_not_take_its_opening_frame_from_the_held_layout() {
+        let mut app = records_app_for_drill();
+        let live = app.layout.records.as_ref().map(|r| r.len()).unwrap_or(0);
+        // Freeze ONE record where the live query finds several: the held layout
+        // is then not what `view_frame` would build for this view.
+        let frozen = std::rc::Rc::new(vec![
+            app.layout
+                .records
+                .as_ref()
+                .expect("records mode")
+                .first()
+                .expect("at least one record")
+                .clone(),
+        ]);
+        assert!(
+            live > frozen.len(),
+            "the fixture only says anything if the freeze really narrows the set"
+        );
+        app.drill_state = Some(DrillState {
+            records: frozen,
+            pending_edits: std::collections::HashMap::new(),
+        });
+        app.rebuild_layout();
+        let epoch = app.cursor_epoch();
+
+        // A batch that reframes nothing. With the guard removed, the opening
+        // basis is the FROZEN layout and the in-loop hash is the LIVE one.
+        app.apply_effects(vec![Box::new(super::super::effect::SetNumberFormat(
+            ",.3f".to_string(),
+        ))]);
+
+        assert_eq!(
+            app.cursor_epoch(),
+            epoch,
+            "a format change reframed nothing, but the frozen and live layouts \
+             were compared against each other"
+        );
+    }
+
+    /// A `SwitchView` mid-batch re-bases the frame rather than comparing across
+    /// two views. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn a_switch_then_a_geometry_effect_compares_within_one_view() {
+        let mut app = paged_model(false);
+        app.model_state.workbook.create_view("Second");
+        app.model_state.workbook.views["Second"]
+            .clone_into(&mut app.model_state.workbook.views["Default"].clone());
+        app.rebuild_layout();
+        let epoch = app.cursor_epoch();
+
+        // Switch to a view whose frame differs, then a geometry effect that
+        // changes nothing. Comparing across the switch is a near-certain mismatch.
+        app.apply_effects(vec![
+            Box::new(super::super::effect::SwitchView("Second".to_string())),
+            Box::new(super::super::effect::SetNumberFormat(",.2f".to_string())),
+        ]);
+
+        assert_eq!(
+            app.view_name(),
+            "Second",
+            "the fixture only says anything if the switch actually landed"
+        );
+        assert_eq!(
+            app.cursor_epoch(),
+            epoch,
+            "nothing reframed — the mismatch was between two different views' \
+             hashes, which is not a reshape"
+        );
+    }
+
     /// BUG (improvise-7kvw): pruning depends on the page, so a page flip renumbers
     /// the grid and left both the host's cursor and every client's stale.
     #[test]

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

@@ -1444,9 +1444,9 @@ mod tests {
             Box::new(SetNumberFormat(",.3f".into())),
             Box::new(TogglePruneEmpty),
         ];
-        // All thirteen, counted off the `impl Effect` blocks rather than trusted:
-        // a fourteenth declaring LayoutGeometry must come here and say so.
-        assert_eq!(cases.len(), 13, "the LayoutGeometry effects are thirteen");
+        // A COUNT, not a totality check: nothing forces a fourteenth here, and a
+        // probe variant proved it — improvise-5ubs, `context/code/command.md`.
+        assert_eq!(cases.len(), 13, "the LayoutGeometry effects were thirteen");
         for effect in cases {
             assert_eq!(
                 effect.kind(),

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

@@ -132,7 +132,10 @@ pub fn grid_of(app: &App, viewport: Viewport) -> GridCache {
 pub fn grid_in(app: &App, view_name: &str, viewport: Viewport) -> GridCache {
     let workbook = &app.model_state.workbook;
     let (name, view) = app.resolve_view(view_name);
-    let elsewhere = (name != app.view_name()).then(|| GridLayout::new(&workbook.model, view));
+    // `with_frozen_records`, not `new`: `new` skips `prune_empty`, so a client on
+    // another view was projected rows its own config prunes — `context/code/command.md`.
+    let elsewhere = (name != app.view_name())
+        .then(|| GridLayout::with_frozen_records(&workbook.model, view, None));
     let layout = elsewhere.as_ref().unwrap_or(&app.layout);
     let (comma, decimals) = parse_number_format(&view.number_format);
     let rows = viewport.rows_in(layout.row_count());
@@ -2242,6 +2245,92 @@ mod tests {
         format!("{:?}|{:?}", app.layout.row_items, app.layout.col_items)
     }
 
+    /// Two views, both pruning, so a cell write can prune a row out of the view
+    /// the host is NOT on. Evidence: `context/test-evidence/command.md`.
+    fn app_with_two_pruning_views() -> App {
+        let mut wb = Workbook::new("Pruned");
+        wb.add_category("Region").unwrap();
+        for region in ["North", "Service", "South"] {
+            wb.add_item("Region", region).unwrap();
+        }
+        wb.add_item("_Measure", "Revenue").unwrap();
+        for region in ["North", "Service", "South"] {
+            wb.model.set_cell(
+                CellKey::new(vec![
+                    ("Region".to_string(), region.to_string()),
+                    ("_Measure".to_string(), "Revenue".to_string()),
+                ]),
+                CellValue::Number(1.0),
+            );
+        }
+        for name in ["Default", "Second"] {
+            if name != "Default" {
+                wb.create_view(name);
+            }
+            let v = &mut wb.views[name];
+            v.set_axis("Region", Axis::Row);
+            v.set_axis("_Measure", Axis::Column);
+            v.prune_empty = true;
+        }
+        wb.switch_view("Default").unwrap();
+        let mut app = App::new(wb, None);
+        app.rebuild_layout();
+        app
+    }
+
+    /// improvise-mcq4's sting: a `CellData` write prunes a row out of ANOTHER
+    /// client's view. Evidence: `context/test-evidence/command.md`.
+    #[test]
+    fn a_write_that_prunes_a_row_out_of_another_clients_view_invalidates_it() {
+        use improvise_protocol::ModelCommand;
+
+        let mut app = app_with_two_pruning_views();
+        let full = full_viewport();
+        let before = format!("{:?}", grid_in(&app, "Second", full).row_frame);
+        let mut sessions = Sessions::default();
+        let (asker, _) = sessions.connect(&app, 100, 100);
+        let (bystander, _) = sessions.connect(&app, 100, 100);
+        sessions.apply(&mut app, bystander, &Command::SwitchView("Second".into()));
+        sessions.apply(&mut app, bystander, &Command::SetRowOffset(1));
+
+        // Clearing Service's only cell prunes its row out of BOTH views; the host
+        // is on Default, so only the bystander's view is the one nobody checks.
+        let applied = sessions.apply_caught_up(
+            &mut app,
+            asker,
+            ModelCommand::ClearCell(RecordCell {
+                record: RecordHash::of(&CellKey::new(vec![(
+                    "Region".to_string(),
+                    "Service".to_string(),
+                )])),
+                measure: Some("Revenue".to_string()),
+            }),
+        );
+
+        assert_ne!(
+            before,
+            format!("{:?}", grid_in(&app, "Second", full).row_frame),
+            "the bystander's view must really lose a row, or this proves nothing"
+        );
+        assert_eq!(
+            sessions
+                .get_mut(bystander)
+                .unwrap()
+                .view()
+                .cursor()
+                .row_offset,
+            0,
+            "a row vanished under the bystander and its mirror was left positioned \
+             on the old grid — gotcha 24's permanently-stale case"
+        );
+        assert!(
+            diff(applied)
+                .1
+                .contains(&Broadcast::To(bystander, Command::ResetCursor)),
+            "and it was never told"
+        );
+    }
+
     /// The commands that renumber the grid. A **shared** table: the cursor audience
     /// and the two shape counters must answer for the same cases (improvise-pdvm).
     fn reshaping_commands() -> Vec<improvise_protocol::ModelCommand> {