fix(core): a model over a source refuses every write in every build and drops its own cells (improvise-rnak)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze

context/code/source.md

@@ -65,10 +65,13 @@ the owned `DataStore`. That is the whole evaluator hook (improvise-rnak, bw4p.14
 there is still one evaluator, `Model`'s, and it computes every formula value, over a source whose
 cells it never owns.
 
-1. **A model over a source is read-only.** `set_cell` and `clear_cell` `debug_assert` that no source
+1. **A model over a source is read-only, in every build.** `set_cell`, `clear_cell`, `remove_category` and
-   is set, because they write the owned store, which is then not read. A release build would drop the
+   `remove_item` `assert!` that no source is set (a `debug_assert` first shipped and was lost in release:
-   write silently; the assert is the guard, not the design. The design is that a caller who holds a
+   the write went to the owned store, which nothing reads). A caller who holds a source writes to the
-   source writes to the source (ADR 003: the trait has no writes), and the model is built over it.
+   source (ADR 003: the trait has no writes). `Model::over` drops the model's own cells, so no stale copy
+   is left for `get_cell`, persistence or the harness to read. `data` is `pub`, so a caller can still write
+   it directly; it is not read, and nothing prevents that. Structural edits (a category or item removed)
+   are refused too, because the source's cells would still carry them.
 2. **A shared source changes behind its readers by interior mutability of its own.** The model holds
    an `Arc`, so a database connection or a test double behind a lock is the same object the
    backend feeds from.

context/test-evidence/core.md

@@ -2397,3 +2397,28 @@ Extremes with no numbers, and a NaN as an extreme, are dropped by `Numbers::from
 Review of #356 (Codex): `DataStore::keys` went through `cells` and cloned every value. It reads keys
 only now; the test pins the keys and their order against `cells`. No clone-count test: a value clone is
 not observable from outside, so the equality is all that can be asserted.
+
+### a_model_over_a_source_refuses_a_clear
+
+Review of #357: the `clear_cell` guard had no test. The refusal is an `assert!`, so the test holds in a
+release build too (`cargo test --release -p improvise-core --lib over_tests` was run: the earlier
+`debug_assert!` failed all four refusal tests there, `a_model_over_a_source_refuses_a_write` among
+them, and was the bug). Rival: the guard removed from `clear_cell` fails this test only.
+
+### a_model_over_a_source_refuses_to_remove_a_category
+
+`remove_category` swept the owned store with no guard. Rival: the guard removed.
+
+### a_model_over_a_source_refuses_to_remove_an_item
+
+The same for `remove_item`.
+
+### putting_a_populated_model_over_a_source_drops_its_own_cells
+
+`Model::over` empties the owned store, so `get_cell` on an over-model is `None` for a cell the
+source holds, and the evaluator still reads the source's value.
+
+### records_mode_reads_the_records_from_a_source
+
+Review of #357: a records-mode `GridLayout` over a source. `group_records` reading the owned store
+left all 563 tests green; with this test it fails (the grid has no rows).

crates/improvise-core/src/model/over_tests.rs

@@ -219,6 +219,48 @@ fn a_model_over_a_source_refuses_a_write() {
     over.set_cell(key("East", "A", "Revenue"), CellValue::Number(1.0));
 }
 
+#[test]
+#[should_panic(expected = "read-only")]
+fn a_model_over_a_source_refuses_a_clear() {
+    let mut over = lent(&fixture());
+    over.clear_cell(&key("East", "A", "Revenue"));
+}
+
+#[test]
+#[should_panic(expected = "read-only")]
+fn a_model_over_a_source_refuses_to_remove_a_category() {
+    let mut over = lent(&fixture());
+    over.remove_category("Product");
+}
+
+#[test]
+#[should_panic(expected = "read-only")]
+fn a_model_over_a_source_refuses_to_remove_an_item() {
+    let mut over = lent(&fixture());
+    over.remove_item("Product", "A");
+}
+
+#[test]
+fn putting_a_populated_model_over_a_source_drops_its_own_cells() {
+    let owned = fixture();
+    assert!(owned.data.iter_cells().count() > 0);
+    let rows = owned
+        .data
+        .iter_cells()
+        .map(|(k, v)| (k, v.clone()))
+        .collect();
+    let over = owned.clone().over(Arc::new(TableSource { rows }));
+    assert_eq!(over.data.iter_cells().count(), 0, "no stale second copy");
+    let cell = key("East", "A", "Revenue");
+    assert_eq!(
+        over.get_cell(&cell),
+        None,
+        "the owned store is empty, the source is not"
+    );
+    let read = over.evaluate_aggregated(&cell, Aggregation::Exact);
+    assert_eq!(read, owned.evaluate_aggregated(&cell, Aggregation::Exact));
+}
+
 #[test]
 fn the_records_a_drill_freezes_are_read_from_the_source() {
     use crate::view::layout::drill_snapshot;

crates/improvise-core/src/model/types.rs

@@ -430,13 +430,19 @@ impl Model {
         }
     }
 
-    /// This model, reading its raw cells from `source` and not `data`: read-only.
+    /// This model, reading its raw cells from `source` and not `data`: read-only. Its own
-    /// Why: `context/code/source.md`.
+    /// cells are dropped, so no stale second copy is left to read. Why: `context/code/source.md`.
     pub fn over(mut self, source: SharedSource) -> Self {
+        self.data = DataStore::new();
         self.external = Some(source);
         self
     }
 
+    /// Refuses, in every build, a write the source would never see.
+    fn writable(&self) {
+        assert!(self.external.is_none(), "{READ_ONLY}");
+    }
+
     /// Add a pivot category, enforcing `MAX_CATEGORIES` over regular ones.
     /// The caller registers it on every view — normally `Workbook`.
     pub fn add_category(&mut self, name: impl Into<String>) -> Result<CategoryId> {
@@ -480,6 +486,7 @@ impl Model {
     /// Remove a category, its cells and the formulas targeting it. The caller
     /// removes it from the views — normally `Workbook`.
     pub fn remove_category(&mut self, name: &str) {
+        self.writable();
         if !self.categories.contains_key(name) || CategoryKind::of_name(name).is_virtual() {
             return;
         }
@@ -502,6 +509,7 @@ impl Model {
     /// An item, the formulas targeting it and the cells naming it. The sweep
     /// is `remove_category`'s one level down — `context/code/core.md` §9.
     pub fn remove_item(&mut self, cat_name: &str, item_name: &str) {
+        self.writable();
         if let Some(cat) = self.categories.get_mut(cat_name) {
             cat.remove_item(item_name);
         }
@@ -552,12 +560,12 @@ impl Model {
     }
 
     pub fn set_cell(&mut self, key: CellKey, value: CellValue) {
-        debug_assert!(self.external.is_none(), "{READ_ONLY}");
+        self.writable();
         self.data.set(key, value);
     }
 
     pub fn clear_cell(&mut self, key: &CellKey) {
-        debug_assert!(self.external.is_none(), "{READ_ONLY}");
+        self.writable();
         self.data.remove(key);
     }
 

crates/improvise-core/src/view/layout.rs

@@ -1348,6 +1348,32 @@ mod tests {
         assert_eq!(layout.col_label(0), "X");
     }
 
+    /// The records a records-mode grid draws come from the source the model is over.
+    #[test]
+    fn records_mode_reads_the_records_from_a_source() {
+        use crate::model::source::TableSource;
+        let mut wb = records_workbook();
+        let v = wb.active_view_mut();
+        v.set_axis("_Index", Axis::Row);
+        v.set_axis("_Dim", Axis::Column);
+        let rows = wb.model.data.iter_cells().map(|(k, v)| (k, v.clone()));
+        let over = wb.model.clone().over(std::sync::Arc::new(TableSource {
+            rows: rows.collect(),
+        }));
+        let (owned, lent) = (
+            GridLayout::new(&wb.model, wb.active_view()),
+            GridLayout::new(&over, wb.active_view()),
+        );
+        assert!(lent.is_records_mode());
+        assert_eq!(lent.row_count(), 1, "the source holds the records");
+        for col in 0..owned.col_count() {
+            assert_eq!(lent.records_display(0, col), owned.records_display(0, col));
+        }
+        let revenue = (0..lent.col_count()).find(|&c| lent.col_label(c) == "Revenue");
+        let revenue = revenue.unwrap();
+        assert_eq!(lent.records_display(0, revenue).as_deref(), Some("100"));
+    }
+
     #[test]
     fn records_mode_activated_when_index_and_dim_on_axes() {
         let mut wb = records_workbook();