From bebaeb2d61a1009dbe5e85e598558c6608a3f569 Mon Sep 17 00:00:00 2001 From: "Claude (dbsp-sources)" Date: Sat, 10 Oct 2026 18:57:13 -0700 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/source.md | 11 +++-- context/test-evidence/core.md | 25 +++++++++++ crates/improvise-core/src/model/over_tests.rs | 42 +++++++++++++++++++ crates/improvise-core/src/model/types.rs | 16 +++++-- crates/improvise-core/src/view/layout.rs | 26 ++++++++++++ 5 files changed, 112 insertions(+), 8 deletions(-) diff --git a/context/code/source.md b/context/code/source.md index f83c467f..a04c1706 100644 --- a/context/code/source.md +++ b/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 - is set, because they write the owned store, which is then not read. A release build would drop the - write silently; the assert is the guard, not the design. The design is that a caller who holds a - source writes to the source (ADR 003: the trait has no writes), and the model is built over it. +1. **A model over a source is read-only, in every build.** `set_cell`, `clear_cell`, `remove_category` and + `remove_item` `assert!` that no source is set (a `debug_assert` first shipped and was lost in release: + the write went to the owned store, which nothing reads). A caller who holds a source writes to the + 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. diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 7698f16c..32e16dc9 100644 --- a/context/test-evidence/core.md +++ b/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). diff --git a/crates/improvise-core/src/model/over_tests.rs b/crates/improvise-core/src/model/over_tests.rs index d8078a5d..305e8822 100644 --- a/crates/improvise-core/src/model/over_tests.rs +++ b/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; diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index d9c161e8..0e5c0193 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/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. - /// Why: `context/code/source.md`. + /// This model, reading its raw cells from `source` and not `data`: read-only. Its own + /// 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) -> Result { @@ -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); } diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 5c8ae905..4a4520d9 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/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();