From 092a4020a3c1a0f4fb4766280e5daef3343e861b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:44:48 -0700 Subject: [PATCH 01/14] WIP improvise-ciw8: red tests, wire writes to a non-record are accepted Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/host.rs | 112 ++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index f31b1d08..0e15738c 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1337,6 +1337,118 @@ mod tests { ); } + fn month_cell(month: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Month".to_string(), month.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]) + } + + /// Months in two quarters on Row, Revenue/Cost and `Profit = Revenue - Cost` on + /// Column, every month funded, Q2 collapsed: rows `[Jan, Feb, Q2]`. + fn host_on_a_fold() -> Host { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] { + wb.add_item_in_group("Month", month, quarter).unwrap(); + for measure in ["Revenue", "Cost"] { + wb.add_item("_Measure", measure).unwrap(); + let value = CellValue::Number(10.0); + wb.model.set_cell(month_cell(month, measure), value); + } + } + let profit = crate::formula::parse_formula("Profit = Revenue - Cost", "_Measure"); + wb.model.add_formula(profit.unwrap()); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().toggle_group_collapse("Month", "Q2"); + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert!(app.layout.folds_a_group(2, 1), "setup: row 2 is the Q2 fold"); + Host::new(app) + } + + /// One wire write, as a hand-rolled client composes it: a surrogate built from + /// a key, and a basis claiming whatever the host says is current. + fn wire_write(host: &mut Host, key: &CellKey, clear: bool) -> Vec { + use improvise_protocol::ModelCommand; + let cell = RecordCell { + record: RecordHash::of(key), + measure: key.get("_Measure").map(String::from), + }; + let cmd = match clear { + true => ModelCommand::ClearCell(cell), + false => ModelCommand::SetCell { + cell, + value: CellValue::Number(7.0), + }, + }; + let (asker, _) = host.connect(10, 10); + let frame = crate::ui::reduce::caught_up(host.app(), cmd); + host.handle(asker, ClientFrame::Commands(vec![frame])) + } + + fn refused(answer: &[Addressed]) -> bool { + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::MutationRefused))) + } + + fn document_text(host: &Host) -> String { + crate::persistence::format_md(&host.app().model_state.workbook) + } + + /// improvise-ciw8 — see context/test-evidence/command.md. + #[test] + fn a_wire_write_to_a_collapsed_fold_is_rejected_and_stores_nothing() { + for clear in [false, true] { + let mut host = host_on_a_fold(); + let before = document_text(&host); + let answer = wire_write(&mut host, &month_cell("Q2", "Cost"), clear); + assert!(refused(&answer), "clear={clear}: not refused: {answer:?}"); + assert_eq!(document_text(&host), before, "clear={clear}: the fold was written"); + } + } + + /// improvise-ciw8: a formula's result is `CellId::Derived`, no write's target. + #[test] + fn a_wire_write_under_a_formula_is_rejected_and_stores_nothing() { + let mut host = host_on_a_fold(); + let before = document_text(&host); + let answer = wire_write(&mut host, &month_cell("Feb", "Profit"), false); + assert!(refused(&answer), "not refused: {answer:?}"); + assert_eq!(document_text(&host), before, "a value was stored under the formula"); + } + + /// improvise-ciw8: a key the None axis aggregates over is `CellId::Aggregate`. + #[test] + fn a_wire_write_to_a_none_axis_aggregate_is_rejected_and_stores_nothing() { + let mut host = host(); + host.app.model_state.workbook.add_category("Product").unwrap(); + host.app.model_state.workbook.add_item("Product", "Widget").unwrap(); + let view = host.app.model_state.workbook.active_view_mut(); + view.set_axis("Product", Axis::None); + host.app.rebuild_layout(); + let before = document_text(&host); + let answer = wire_write(&mut host, &cell("South"), false); + assert!(refused(&answer), "not refused: {answer:?}"); + assert_eq!(document_text(&host), before, "the partial key was written"); + } + + /// The control for the three above: the stored cell beside the fold is written. + #[test] + fn a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted() { + let mut host = host_on_a_fold(); + let answer = wire_write(&mut host, &month_cell("Feb", "Cost"), false); + assert!( + answer.iter().any(|a| matches!(a.msg, ServerFrame::Accepted)), + "{answer:?}" + ); + let model = &host.app().model_state.workbook.model; + let value = model.get_cell(&month_cell("Feb", "Cost")); + assert_eq!(value, Some(&CellValue::Number(7.0))); + } + /// Clause 4 must not fire on a frame that merely names a record it is /// creating. See `context/test-evidence/command.md`. #[test] From 104c10f5708cebd1b33ee3c3e4b3b4bbb2d600a9 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:39:23 -0700 Subject: [PATCH 02/14] improvise-ciw8: a wire write to a cell the host projects as no Record is refused Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/app.rs | 41 ++++++++++++++++++++-- crates/improvise-command/src/ui/project.rs | 2 +- crates/improvise-command/src/ui/reduce.rs | 24 ++++++------- 3 files changed, 51 insertions(+), 16 deletions(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 4969f2cb..3aa3bd11 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -27,7 +27,7 @@ use crate::ui::viewport::{compute_col_widths, compute_row_header_width, compute_ use crate::view::{GridLayout, View}; use crate::workbook::Workbook; use improvise_core::model::record::{AggregateHash, RecordHash}; -use improvise_protocol::RecordCell; +use improvise_protocol::{CellId, RecordCell}; /// A cell's coordinate tuple with `_Measure` dropped: the unit of atomicity /// ADR 001 settles on, and not the cell — see `context/code/command.md`. @@ -572,16 +572,51 @@ impl App { .map(|key| record_of(&key)) } + /// Why a wire write to `cell` is refused: the host itself projects it as no + /// `CellId::Record` (improvise-ciw8, ADR 002). `None` for a writable cell. + pub fn unwritable(&self, cell: &RecordCell) -> Option { + let key = self.resolve_cell(cell)?; + let model = &self.model_state.workbook.model; + let derived = model.formula_for(&key).map(|f| { + format!("{} is computed by a formula — edit the formula, not the cell", f.target) + }); + derived.or_else(|| self.aggregate_refusal(cell.record)) + } + + /// A record the store does not hold is known only by its layout position, and + /// is a record only where the projection there says `Record`, never `Aggregate`. + fn aggregate_refusal(&self, hash: RecordHash) -> Option { + let stored = self.model_state.workbook.model.data.resolve_record(hash); + let (row, col) = stored.map_or_else(|| self.record_position(hash), |_| None)?; + let projected = crate::ui::project::cell_id(self, &self.layout, row, col); + matches!(projected, Some(CellId::Aggregate(_))).then(|| { + "an aggregate combines several records, so there is no one cell to write".into() + }) + } + + /// Where in the current layout a cell of record `hash` sits, if anywhere. + fn record_position(&self, hash: RecordHash) -> Option<(usize, usize)> { + self.layout_cells() + .find(|(_, _, key)| RecordHash::of(key) == hash) + .map(|(row, col, _)| (row, col)) + } + /// Every cell key the current grid names. Records mode is excluded: its /// `cell_key` carries *positions*, the trap ADR 002 rejected `_Index` for. fn layout_cell_keys(&self) -> impl Iterator + '_ { + self.layout_cells().map(|(_, _, key)| key) + } + + /// [`Self::layout_cell_keys`] with each key's position. + fn layout_cells(&self) -> impl Iterator + '_ { let layout = &self.layout; let rows = match layout.is_records_mode() { true => 0, false => layout.row_count(), }; - (0..rows) - .flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c))) + (0..rows).flat_map(move |r| { + (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c).map(|k| (r, c, k))) + }) } /// Point **this client's** slice at `name`, if the document has such a view, diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index d42e46c7..4e3ed9ec 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -299,7 +299,7 @@ fn cell_display( /// What the cell at `(row, col)` *is*, as the surrogate the client echoes back. The /// fork is `EditOrDrill`'s, asked the same way — see `context/code/command.md`. -fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option { +pub(crate) fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option { let model = &app.model_state.workbook.model; if layout.is_records_mode() { // A measure can be BOTH an explicit `_Measure` item and a formula target, diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index e59e54cf..1558aa23 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -135,18 +135,18 @@ pub fn apply_command(app: &mut App, cmd: Checked<'_>) -> Dirty { /// was derived from (improvise-kmgs). Exhaustive on purpose. pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { let one: Box = match cmd { - ModelCommand::SetCell { cell, value } => { - let Some(key) = app.resolve_cell(cell) else { - return vec![]; - }; - Box::new(effect::SetCell(key, value.clone())) - } - ModelCommand::ClearCell(cell) => { - let Some(key) = app.resolve_cell(cell) else { - return vec![]; - }; - Box::new(effect::ClearCell(key)) - } + // A cell the host projects as no `Record` is refused, so the frame is + // Rejected rather than written (improvise-ciw8, ADR 002). + ModelCommand::SetCell { cell, value } => match (app.unwritable(cell), app.resolve_cell(cell)) { + (Some(why), _) => effect::refuse(why), + (None, None) => return vec![], + (None, Some(key)) => Box::new(effect::SetCell(key, value.clone())), + }, + ModelCommand::ClearCell(cell) => match (app.unwritable(cell), app.resolve_cell(cell)) { + (Some(why), _) => effect::refuse(why), + (None, None) => return vec![], + (None, Some(key)) => Box::new(effect::ClearCell(key)), + }, ModelCommand::AddItem { category, item } => Box::new(effect::AddItem { category: category.clone(), item: item.clone(), From 46e75c415fed863766c545224da831cbe675fa4e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:41:43 -0700 Subject: [PATCH 03/14] improvise-ciw8: pin the unresolved-write answer; document the rule and why vec![] stays Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 43 +++++++++++++ context/test-evidence/command.md | 37 +++++++++++ crates/improvise-command/src/ui/app.rs | 5 +- crates/improvise-command/src/ui/host.rs | 75 +++++++++++++++++++++-- crates/improvise-command/src/ui/reduce.rs | 12 ++-- 5 files changed, 160 insertions(+), 12 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index a1eef6cd..e4704362 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3396,3 +3396,46 @@ fixed default, not a clock read. The list is checked in both directions. An unlisted command that diverges fails the test, and so does a listed one that does not diverge. A stale entry therefore cannot linger, and a new registration is covered the day it lands. + +## A wire write reaches only a cell the host projects as a Record (improvise-ciw8) + +`App::resolve_record` answers "which record does this surrogate name". Its +fallback, `record_in_layout`, accepts any key the current layout names, and that +includes a collapsed group's fold and a key the None axis aggregates over. It +has to stay that permissive: it is also the basis check's question, and a record +nobody has stored yet (the empty cell a client is about to fill) is found only +there. But resolving is not the same as being writable. A hand-rolled frame +`SetCell { record: RecordHash::of({Month=Q2}), .. }` was answered `Accepted` and +stored a cell at `Month=Q2`, while the host's own projection called that +position `CellId::Aggregate`. A write under a formula (`Derived`) was accepted +the same way. + +So `model_effects` asks `App::unwritable` before a wire `SetCell` or `ClearCell` +builds its effect, and refuses through `effect::refuse`. That is the #181 path: +the refusal marks the batch, `Host::mutating_frame` undoes the frame, the asker +gets `Rejected(MutationRefused)`, and the reason is on the host's status line. +The question it asks is the projection's own, never a second derivation of it: + +- A cell a formula owns is refused. That is `cell_id`'s own `Derived` rule, + `formula_for(&key)`. It is asked directly because a stored record may sit on + no position in the host's current view. +- A record found **in the store** is a record, whatever view anyone is on. + `RecordHash` is document-scoped (gotcha 44). +- A record found **only through the layout** is asked of `project::cell_id` at + the position where it was found. `Aggregate`, a fold or a sum over a None-axis + category, is refused. + +**Why the `vec![]` arm stays.** A reviewer asked whether a `SetCell` whose cell +does not resolve at all, which `model_effects` turns into `vec![]` and so +applies nothing, should also be a `Rejection`. Through a `Host`, it already is, +before that arm is reached. A lone write naming a record that never existed gets +`RecordGone` up front, whether or not the basis claims it, because the create +exemption covers only a *later* command in a frame +(`a_wire_write_to_a_record_that_never_existed_is_rejected`). A later write whose +record nothing in the frame made resolvable gets `ChainBroken` from +`broken_chain` +(`a_frame_whose_write_would_land_nowhere_is_refused_rather_than_dropped`). So +the arm is unreachable from the wire. Making it a refusal would add a second +answer to "does this resolve" beside `broken_chain`, and that answer could only +ever fire after the first had already refused. It is left as the mutation +path's own behaviour for callers that do not go through the session door. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 0b3f4dbd..ac34354b 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5574,3 +5574,40 @@ store is exactly `{Clothing/Jan/Revenue = 1}`. Under the rival the clear lands on `Food/Jan/Profit`, where there is nothing to clear, and `Clothing/Jan/Profit = 5` survives. Injected alone into a full workspace run, the rival fails this test and nothing else. + +### a_wire_write_to_a_collapsed_fold_is_rejected_and_stores_nothing + +improvise-ciw8, through a real `Host` frame. Q2 is collapsed, and a hand-rolled +client sends `SetCell` (then, separately, `ClearCell`) for `{Month=Q2, _Measure=Cost}` +with a caught-up basis. At `ea119bd9` both were answered `Accepted`, and the +`SetCell` stored `Month=Q2, _Measure=Cost = 7`, the non-item coordinate +improvise-gw18 exists to prevent. The test asserts `Rejected(MutationRefused)` +and an unchanged document for both. Rival: the aggregate refusal disabled. This +test and the None-axis test fail. The control is +`a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted`, so a host that refused +every wire write cannot pass. + +### a_wire_write_under_a_formula_is_rejected_and_stores_nothing + +improvise-ciw8's `Derived` arm: `SetCell` on Feb × Profit, where +`Profit = Revenue - Cost`. At `ea119bd9` it was accepted and stored a value under +the formula. Rival: the formula refusal disabled. Only this test fails. + +### a_wire_write_to_a_none_axis_aggregate_is_rejected_and_stores_nothing + +improvise-ciw8's other `Aggregate`. With `Product` on `Axis::None`, the grid's +key `{Region=South, _Measure=Revenue}` is a partial key that no stored record +has, so it resolves only through the layout, where `cell_id` says `Aggregate`. +At `ea119bd9` it was accepted and stored the partial key. It fails under the +same rival as the fold test. + +### a_wire_write_to_a_record_that_never_existed_is_rejected + +The evidence for keeping `model_effects`' `vec![]` arm rather than turning it +into a refusal (`context/code/command.md`, "A wire write reaches only a cell the +host projects as a Record"). A lone write naming a record that never existed is +answered `RecordGone` before anything runs, both when a caught-up basis claims +it and when a hand-rolled basis omits it. The second half was expected to be +`ChainBroken`, and the test said otherwise: the create exemption covers only +commands after the first in a frame. That surprise is why it is asserted rather +than argued. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 3aa3bd11..b2c04003 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -578,7 +578,10 @@ impl App { let key = self.resolve_cell(cell)?; let model = &self.model_state.workbook.model; let derived = model.formula_for(&key).map(|f| { - format!("{} is computed by a formula — edit the formula, not the cell", f.target) + format!( + "{} is computed by a formula — edit the formula, not the cell", + f.target + ) }); derived.or_else(|| self.aggregate_refusal(cell.record)) } diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 0e15738c..e7d98541 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1364,7 +1364,10 @@ mod tests { wb.active_view_mut().toggle_group_collapse("Month", "Q2"); let mut app = App::new(wb, None); app.rebuild_layout(); - assert!(app.layout.folds_a_group(2, 1), "setup: row 2 is the Q2 fold"); + assert!( + app.layout.folds_a_group(2, 1), + "setup: row 2 is the Q2 fold" + ); Host::new(app) } @@ -1406,7 +1409,11 @@ mod tests { let before = document_text(&host); let answer = wire_write(&mut host, &month_cell("Q2", "Cost"), clear); assert!(refused(&answer), "clear={clear}: not refused: {answer:?}"); - assert_eq!(document_text(&host), before, "clear={clear}: the fold was written"); + assert_eq!( + document_text(&host), + before, + "clear={clear}: the fold was written" + ); } } @@ -1417,15 +1424,27 @@ mod tests { let before = document_text(&host); let answer = wire_write(&mut host, &month_cell("Feb", "Profit"), false); assert!(refused(&answer), "not refused: {answer:?}"); - assert_eq!(document_text(&host), before, "a value was stored under the formula"); + assert_eq!( + document_text(&host), + before, + "a value was stored under the formula" + ); } /// improvise-ciw8: a key the None axis aggregates over is `CellId::Aggregate`. #[test] fn a_wire_write_to_a_none_axis_aggregate_is_rejected_and_stores_nothing() { let mut host = host(); - host.app.model_state.workbook.add_category("Product").unwrap(); - host.app.model_state.workbook.add_item("Product", "Widget").unwrap(); + host.app + .model_state + .workbook + .add_category("Product") + .unwrap(); + host.app + .model_state + .workbook + .add_item("Product", "Widget") + .unwrap(); let view = host.app.model_state.workbook.active_view_mut(); view.set_axis("Product", Axis::None); host.app.rebuild_layout(); @@ -1435,13 +1454,57 @@ mod tests { assert_eq!(document_text(&host), before, "the partial key was written"); } + /// improvise-ciw8's second question: a write naming a record that never existed + /// is refused by the frame, not dropped — see context/test-evidence/command.md. + #[test] + fn a_wire_write_to_a_record_that_never_existed_is_rejected() { + use improvise_protocol::ModelCommand; + let mut host = host_on_a_fold(); + let before = document_text(&host); + // Claimed by a caught-up basis: structural staleness, answered up front. + let answer = wire_write(&mut host, &month_cell("Nowhere", "Cost"), false); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::RecordGone(_)))), + "claimed: {answer:?}" + ); + // Unclaimed: the create exemption covers only LATER commands in a frame, + // so a lone write is answered up front too. + let key = month_cell("Nowhere", "Cost"); + let cmd = ModelCommand::SetCell { + cell: RecordCell { + record: RecordHash::of(&key), + measure: Some("Cost".into()), + }, + value: CellValue::Number(7.0), + }; + let basis = improvise_protocol::Basis { + document_generation: host.app().document_generation(), + layout_version: host.app().layout_version(), + records: vec![], + }; + let (asker, _) = host.connect(10, 10); + let frame = ClientFrame::Commands(vec![Command::Model { basis, cmd }]); + let answer = host.handle(asker, frame); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::RecordGone(_)))), + "unclaimed: {answer:?}" + ); + assert_eq!(document_text(&host), before); + } + /// The control for the three above: the stored cell beside the fold is written. #[test] fn a_wire_write_to_a_stored_cell_beside_a_fold_is_accepted() { let mut host = host_on_a_fold(); let answer = wire_write(&mut host, &month_cell("Feb", "Cost"), false); assert!( - answer.iter().any(|a| matches!(a.msg, ServerFrame::Accepted)), + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)), "{answer:?}" ); let model = &host.app().model_state.workbook.model; diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 1558aa23..dcc8f070 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -137,11 +137,13 @@ pub fn model_effects(app: &App, cmd: &ModelCommand) -> Vec> { let one: Box = match cmd { // A cell the host projects as no `Record` is refused, so the frame is // Rejected rather than written (improvise-ciw8, ADR 002). - ModelCommand::SetCell { cell, value } => match (app.unwritable(cell), app.resolve_cell(cell)) { - (Some(why), _) => effect::refuse(why), - (None, None) => return vec![], - (None, Some(key)) => Box::new(effect::SetCell(key, value.clone())), - }, + ModelCommand::SetCell { cell, value } => { + match (app.unwritable(cell), app.resolve_cell(cell)) { + (Some(why), _) => effect::refuse(why), + (None, None) => return vec![], + (None, Some(key)) => Box::new(effect::SetCell(key, value.clone())), + } + } ModelCommand::ClearCell(cell) => match (app.unwritable(cell), app.resolve_cell(cell)) { (Some(why), _) => effect::refuse(why), (None, None) => return vec![], From f176c2b8cae1ae93c66ff381d7d81d5e39241e29 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:43:13 -0700 Subject: [PATCH 04/14] WIP: a legacy JSON workbook is refused by name (improvise-tt22) --- crates/improvise-io/src/persistence/mod.rs | 68 +++++++++++----------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/crates/improvise-io/src/persistence/mod.rs b/crates/improvise-io/src/persistence/mod.rs index 505dfa2c..ee8d579a 100644 --- a/crates/improvise-io/src/persistence/mod.rs +++ b/crates/improvise-io/src/persistence/mod.rs @@ -1,4 +1,4 @@ -//! `.improv` document lifecycle — save, load, gzip, legacy JSON — over a +//! `.improv` document lifecycle — save, load, gzip — over a //! [`Storage`] backend. The text format is in `format`, `parse` and `export`. mod export; @@ -96,11 +96,16 @@ pub fn load_from(storage: &dyn Storage, path: &Path) -> Result { }; let text = String::from_utf8(bytes).with_context(|| format!("{} is not UTF-8", path.display()))?; + // `.improv` text is the only native format; a JSON workbook is named, not + // handed to the grammar to fail on its first byte (improvise-tt22). if text.trim_start().starts_with('{') { - serde_json::from_str(&text).context("Failed to deserialize workbook") - } else { - parse_md(&text) + anyhow::bail!( + "{} is a legacy JSON workbook, which is no longer supported: \ + only .improv text can be opened", + path.display() + ); } + parse_md(&text) } pub fn autosave_path(path: &Path) -> std::path::PathBuf { @@ -194,37 +199,34 @@ mod tests { ); } + /// improvise-tt22: `.improv` text is the only native format, so a legacy + /// JSON workbook is refused by name. Evidence: `context/test-evidence/formula-and-io.md`. #[test] - fn load_dispatcher_detects_legacy_json_by_brace() { - // The load() function routes to JSON deserializer when text starts with '{' - let m = two_cat_model(); - let json = serde_json::to_string_pretty(&m).unwrap(); - assert!(json.trim_start().starts_with('{'), "sanity check"); - // Deserialise via the JSON path - let m2: Workbook = serde_json::from_str(&json).unwrap(); - assert_eq!(m2.model.name, "Budget"); - } - - #[test] - fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() { + fn a_legacy_json_workbook_is_refused_by_name() { use crate::storage::{MemoryFs, Storage}; - const LOSSY: f64 = 0.248_157_142_857_142_85; - let key = coord(&[("Type", "Food"), ("Month", "Jan")]); - let mut wb = two_cat_model(); - wb.model.set_cell(key.clone(), CellValue::Number(LOSSY)); - let json = serde_json::to_string(&wb).unwrap(); - assert!( - json.contains("0.24815714285714285"), - "the file holds the exact text" - ); - let fs = MemoryFs::new(); - let path = std::path::Path::new("legacy.improv"); - fs.write(path, json.as_bytes()).unwrap(); - let loaded = super::load_from(&fs, path).unwrap(); - let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else { - panic!("the cell must load as a number"); - }; - assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}"); + // The shape the removed loader read: `Workbook`'s serde form. + let legacy = r#" + {"model": {"name": "Budget", "categories": {}, "data": [], "formulas": []}, + "views": {}, "active_view": "Default"}"#; + for name in ["legacy.improv", "legacy.improv.gz"] { + let fs = MemoryFs::new(); + let path = std::path::Path::new(name); + let bytes = if name.ends_with(".gz") { + use std::io::Write; + let mut enc = + flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + enc.write_all(legacy.as_bytes()).unwrap(); + enc.finish().unwrap() + } else { + legacy.as_bytes().to_vec() + }; + fs.write(path, &bytes).unwrap(); + let err = format!("{:#}", super::load_from(&fs, path).unwrap_err()); + assert!( + err.contains("legacy JSON") && err.contains("no longer supported"), + "{name}: expected the format named and refused, got {err:?}" + ); + } } // ── save/load roundtrip via file ──────────────────────────────────── From 7fb6960cf88c5df16ad0edcdd947b74f84426a5a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:50:26 -0700 Subject: [PATCH 05/14] WIP: drop the serde derives only the legacy loader used; docs (improvise-tt22) --- Cargo.toml | 2 +- README.md | 5 +-- context/code/formula-and-io.md | 30 ++++++++++++++-- context/code/protocol.md | 2 +- context/test-evidence/core.md | 6 ---- context/test-evidence/formula-and-io.md | 38 +++++++++++++++------ context/test-evidence/tooling.md | 5 +-- crates/improvise-command/src/ui/effect.rs | 10 +++--- crates/improvise-core/Cargo.toml | 2 +- crates/improvise-core/src/model/category.rs | 10 +++--- crates/improvise-core/src/model/cell.rs | 38 +-------------------- crates/improvise-core/src/model/types.rs | 5 +-- crates/improvise-core/src/view/types.rs | 8 +---- crates/improvise-core/src/workbook.rs | 3 +- crates/improvise-formula/Cargo.toml | 1 - crates/improvise-formula/src/ast.rs | 12 +++---- crates/improvise-io/Cargo.toml | 2 +- 17 files changed, 84 insertions(+), 95 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bda00df3..259d8478 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -45,7 +45,7 @@ serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["float_roundtrip"] } anyhow = "1" thiserror = "2" -indexmap = { version = "2", features = ["serde"] } +indexmap = "2" chrono = { version = "0.4", features = ["serde"] } flate2 = "1" unicode-width = "^0.2" diff --git a/README.md b/README.md index 03ae07e1..76c15f5a 100644 --- a/README.md +++ b/README.md @@ -169,8 +169,9 @@ Measure: column format: ,.2f ``` -Gzip-compressed `.improv.gz` is also supported. Legacy JSON is auto-detected -for backward compatibility. +Gzip-compressed `.improv.gz` is also supported. `.improv` text is the only +native document format: the old JSON document format is no longer read, and +opening one fails with an error that says so. JSON *import* is unaffected. ## Import and scripting diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 00f0835b..3c9efe8f 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -471,8 +471,34 @@ document that decompressed perfectly well. Decoding once, afterwards, also leaves **one** answer to "is this UTF-8" rather than one per branch. The `.gz` suffix selects gzip regardless of backend: that is a property of the -document name, not of the filesystem (improvise-6mq). Legacy JSON is detected by -a leading `{` and is never written. +document name, not of the filesystem (improvise-6mq). + +**`.improv` text is the only native format (improvise-tt22).** A legacy JSON +workbook used to be detected by a leading `{` and loaded read-only through +`Workbook`'s serde derives. That path is gone, on the user's direction on PR #210. +A document that still starts with `{` is refused with an error that names the +format and says it is no longer supported. The test is +`a_legacy_json_workbook_is_refused_by_name`. The error is decided rather than +defaulted, because otherwise `parse_md` would report a grammar error at line 1, +which diagnoses the wrong thing: the file is not malformed `.improv`. The check +is on the decompressed text, so `.improv.gz` is covered too. + +The serde derives that existed only for that path went with it: `Workbook`, +`Model`, `View`, `Category`, `Item`, `Group`, `CategoryKind`, `CellKey`, +`DataStore`'s manual impls and the formula AST. So did indexmap's `serde` feature +in `improvise-core`, `improvise-io` and the root crate, and `improvise-formula`'s +serde dependency. Each was removed and the whole workspace rebuilt, so the +compiler named every other consumer. Two remain, and both are the wire: +`Axis` (whose `rename_all = "lowercase"` is its wire spelling) and `PageAxis`. +`CellValue` was never a candidate, since it is carried on the wire too. JSON +*import* (`StartImportWizard`, `ImportJsonHeadless`) is a separate feature and is +untouched. + +One test consumer had to move: the effect-classification tests fingerprinted the +model and the views through `serde_json`. They now use `Debug`. That is no weaker: +it renders every field, including the derived `formula_cache` the JSON skipped. +`format_md` was rejected as the replacement, because it skips the virtual views +and sorts the data, so it would have been a weaker fingerprint. `parse_number_cell` returns `CellValue::Error` naming the offending text rather than a fabricated `0.0`. No token the current grammar admits actually fails diff --git a/context/code/protocol.md b/context/code/protocol.md index 032feab5..3a9695e0 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -766,7 +766,7 @@ others to inherit it works in `cargo test` and nowhere else. **Why "can parse" and not "parses a frame".** The first version of the rule covered only the three crates that parse frames and listed them by hand. Frames are not the only place a user's number crosses a JSON parser. -`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import +`improvise-io`'s legacy-JSON `.improv` load (removed since, improvise-tt22), `improvise-command`'s JSON import (`StartImportWizard`, `ImportJsonHeadless`) and the root binary's `improvise import` all parsed with the lossy default, and in the TUI binary nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285` diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index c51bf735..edc96bf3 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -476,12 +476,6 @@ cell in the row). The pair `live_records` yields is not two facts that could drift apart: the surrogate is the record's own, so it is the one that resolves it back. -### a_deserialized_store_can_resolve_its_records - -The record index is derived state and is `#[serde(skip)]`-equivalent — it must -come back *with* the data, rebuilt through `set`, rather than being written -alongside it. - ## `model/record.rs` ### a_records_surrogate_is_pinned_to_a_literal diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 48e8bd5a..536effb1 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1435,16 +1435,34 @@ reintroduce the iot6 divergence between what the user sees and what they export. On the pre-6qet tree the body is empty (the group had no row, and pruning left nothing), so the test fails there. -## Legacy JSON numbers (improvise-6yvg) +## Legacy JSON documents (improvise-6yvg, improvise-tt22) -### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit` +`a_legacy_json_workbook_loads_its_numbers_bit_for_bit` (improvise-6yvg) pinned +that the legacy JSON load path read numbers exactly. improvise-tt22 removed that +path, so the test went with it. Its other role was to witness `improvise-io`'s own +`float_roundtrip` declaration. The manifest rule in +`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`) +still covers that, because `improvise-io` keeps a normal `serde_json` +dependency for its import types (`context/test-evidence/tooling.md`). -A workbook with Food/Jan holding `0.24815714285714285` is serialized with -`serde_json` and loaded through `load_from`. A leading `{` sends it down the -legacy JSON path. The test first asserts that the text holds the exact decimal, -so what it measures is the parse. It compares `f64::to_bits`. Found by the -review of PR #190, not by the ticket. +### `a_legacy_json_workbook_is_refused_by_name` -**Red at `1158c23`** in `cargo test -p improvise-io`: loaded -`0.24815714285714283`. Green once `improvise-io`'s own manifest enables -`float_roundtrip`, and red again with only that line reverted. +improvise-tt22, on the user's direction on PR #210: `.improv` text is the only +native format. A document starting with `{` used to be deserialized as a +`Workbook`. The test writes a JSON workbook in the shape the old loader read, +both plain and gzipped, and asserts the error names "legacy JSON" and says "no +longer supported". + +**Red at `52c6a05`**: `Failed to deserialize workbook: missing field +`next_category_id``. That is a parse error, which is exactly what the ticket said +the refusal must not be. A complete legacy document instead loaded successfully, +which the since-deleted bit-for-bit test above demonstrated. + +**Rivals, each run:** + +- **Deleting the refusal.** The text falls through to `parse_md`, and the test + fails on `legacy.improv` with "Parse error at line 1, column 1 (expected + version_line)". A parse error does not name the format. +- **Refusing only when the path is not gzipped.** The plain case passes and the + test fails on `legacy.improv.gz` with the same grammar error. So the gzip + case is what makes the check prove it runs on decompressed text. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 3304bd5e..b0517374 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -696,8 +696,9 @@ every target-specific entry, whose **normal** serde_json dependency lacks **Faults injected, each run:** - **The feature removed from `improvise-io` alone.** The check fails naming - `improvise-io (all targets)`, and so does - `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`. + `improvise-io (all targets)`. So did + `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`, until improvise-tt22 + removed the legacy load path and that test with it. `a_json_import_stores_its_numbers_bit_for_bit` stays green, because `improvise-command` enables the feature and unification reaches io in its build. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index b119b7eb..dbb66317 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1396,16 +1396,16 @@ mod tests { // ── Effect classification (improvise-mae) ─────────────────────────── - /// Serialized model data — everything a connected client renders. - /// Excludes the view slice, which is per-client. + /// Every field of the model, rendered — everything a connected client + /// renders. `Debug`, not serde: the model has no serde form (improvise-tt22). fn model_fingerprint(app: &App) -> String { - serde_json::to_string(&app.model_state.workbook.model).expect("model serializes") + format!("{:?}", app.model_state.workbook.model) } - /// Serialized view configuration. Kept for the NEGATIVE arm only, where a + /// Every field of every view. Kept for the NEGATIVE arm only, where a /// hash stricter than `document_config` is wanted (improvise-1xf4). fn view_config_fingerprint(app: &App) -> String { - serde_json::to_string(&app.model_state.workbook.views).expect("views serialize") + format!("{:?}", app.model_state.workbook.views) } /// improvise-mae: rather than restate each declaration, apply the effect and diff --git a/crates/improvise-core/Cargo.toml b/crates/improvise-core/Cargo.toml index 886ece4a..790fb647 100644 --- a/crates/improvise-core/Cargo.toml +++ b/crates/improvise-core/Cargo.toml @@ -9,7 +9,7 @@ repository = "https://github.com/fiddlerwoaroof/improvise" [dependencies] improvise-formula = { path = "../improvise-formula" } anyhow = "1" -indexmap = { version = "2", features = ["serde"] } +indexmap = "2" serde = { version = "1", features = ["derive"] } # SipHash-2-4 under a constant key: the record surrogate must not move # between processes, which rules out anything built on `RandomState` diff --git a/crates/improvise-core/src/model/category.rs b/crates/improvise-core/src/model/category.rs index 500ad680..e915e37f 100644 --- a/crates/improvise-core/src/model/category.rs +++ b/crates/improvise-core/src/model/category.rs @@ -1,10 +1,9 @@ use indexmap::IndexMap; -use serde::{Deserialize, Serialize}; pub type CategoryId = usize; pub type ItemId = usize; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Item { pub id: ItemId, pub name: String, @@ -27,7 +26,7 @@ impl Item { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Group { pub name: String, /// Parent group name for nested hierarchies @@ -50,7 +49,7 @@ impl Group { /// Regular categories store their items explicitly; virtual ones are /// synthesized at query time by the layout layer. context/code/core.md §9. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)] pub enum CategoryKind { #[default] Regular, @@ -125,7 +124,7 @@ impl CategoryKind { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Category { pub id: CategoryId, pub name: String, @@ -136,7 +135,6 @@ pub struct Category { /// Next item id counter next_item_id: ItemId, /// Whether this is a regular or virtual category - #[serde(default)] pub kind: CategoryKind, } diff --git a/crates/improvise-core/src/model/cell.rs b/crates/improvise-core/src/model/cell.rs index 0b143831..7a10edf1 100644 --- a/crates/improvise-core/src/model/cell.rs +++ b/crates/improvise-core/src/model/cell.rs @@ -7,7 +7,7 @@ use super::symbol::{Symbol, SymbolTable}; /// A cell key: (category, item) pairs, always sorted by category name. The /// constructors are the only way to build one — context/code/core.md §1. -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct CellKey(pub Vec<(String, String)>); impl CellKey { @@ -194,8 +194,6 @@ impl RecordEntry { } } -/// Serialized as a list of (key, value) pairs, so `CellKey` need not -/// serialize as a string for JSON object keys. #[derive(Debug, Clone, Default)] pub struct DataStore { /// Primary storage: interned keys, insertion-ordered so records mode can @@ -210,29 +208,6 @@ pub struct DataStore { records: HashMap, } -impl Serialize for DataStore { - fn serialize(&self, s: S) -> Result { - use serde::ser::SerializeSeq; - let mut seq = s.serialize_seq(Some(self.cells.len()))?; - for (k, v) in &self.cells { - let cell_key = self.to_cell_key(k); - seq.serialize_element(&(cell_key, v))?; - } - seq.end() - } -} - -impl<'de> Deserialize<'de> for DataStore { - fn deserialize>(d: D) -> Result { - let pairs: Vec<(CellKey, CellValue)> = Vec::deserialize(d)?; - let mut store = DataStore::default(); - for (key, value) in pairs { - store.set(key, value); - } - Ok(store) - } -} - impl DataStore { pub fn new() -> Self { Self::default() @@ -1181,15 +1156,4 @@ mod record_index { assert_eq!(store.resolve_record(hash), Some(record)); } } - - /// Derived state must come back *with* the data, rebuilt through `set`, - /// rather than be written alongside it. - #[test] - fn a_deserialized_store_can_resolve_its_records() { - let json = serde_json::to_string(&store()).expect("serializes"); - let back: DataStore = serde_json::from_str(&json).expect("deserializes"); - let east = key(&[("Product", "Shirts"), ("Region", "East")]); - assert_eq!(back.resolve_record(RecordHash::of(&east)), Some(&east)); - assert_eq!(back.live_records().count(), 2); - } } diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index ea394dbe..09dcad98 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use anyhow::{Result, anyhow}; use indexmap::{IndexMap, IndexSet}; -use serde::{Deserialize, Serialize}; use super::category::{Category, CategoryId, CategoryKind, MEASURE_CATEGORY}; use super::cell::{CellKey, CellValue, DataStore}; @@ -238,7 +237,7 @@ impl Aggregation { /// Pure-data document model: categories, cells, formulas. It knows nothing /// about views — see [`crate::workbook::Workbook`] and core.md §10. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Model { pub name: String, pub categories: IndexMap, @@ -247,10 +246,8 @@ pub struct Model { next_category_id: CategoryId, /// Per-measure aggregation, used when collapsing `Axis::None` /// categories. Defaults to `Sum`. - #[serde(default)] pub measure_agg: HashMap, /// Cached formula evaluation results. Recomputed by `recompute_formulas`. - #[serde(skip)] formula_cache: HashMap, } diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index 140061f7..1967bfb8 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -1,14 +1,9 @@ use indexmap::IndexMap; -use serde::{Deserialize, Serialize}; use std::collections::{HashMap, HashSet}; use super::axis::Axis; use crate::model::category::CategoryKind; -fn default_prune() -> bool { - true -} - /// Scratch views the app rebuilds on demand rather than persisting, because /// they put the never-persisted `_Index`/`_Dim` on axes (improvise-60z). pub const VIRTUAL_VIEW_NAMES: [&str; 2] = ["_Records", "_Drill"]; @@ -32,7 +27,7 @@ pub fn unused_view_name<'a>(existing: impl IntoIterator) -> Stri .expect("a name is free below usize::MAX, there being fewer views") } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct View { pub name: String, /// Axis assignment for each category @@ -46,7 +41,6 @@ pub struct View { /// Number format string (e.g. ",.0f" for comma-separated integer) pub number_format: String, /// When true, empty rows/columns are pruned from the display. - #[serde(default = "default_prune")] pub prune_empty: bool, } diff --git a/crates/improvise-core/src/workbook.rs b/crates/improvise-core/src/workbook.rs index 718e9067..0df64dc3 100644 --- a/crates/improvise-core/src/workbook.rs +++ b/crates/improvise-core/src/workbook.rs @@ -3,13 +3,12 @@ use anyhow::{Result, anyhow}; use indexmap::IndexMap; -use serde::{Deserialize, Serialize}; use crate::model::Model; use crate::model::category::{CategoryId, CategoryKind, ItemId}; use crate::view::{Axis, DEFAULT_VIEW, View}; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Workbook { pub model: Model, pub views: IndexMap, diff --git a/crates/improvise-formula/Cargo.toml b/crates/improvise-formula/Cargo.toml index 1912219f..d7459be8 100644 --- a/crates/improvise-formula/Cargo.toml +++ b/crates/improvise-formula/Cargo.toml @@ -10,7 +10,6 @@ repository = "https://github.com/fiddlerwoaroof/improvise" anyhow = "1" pest = "2.9.2" pest_derive = "2.9.2" -serde = { version = "1", features = ["derive"] } [dev-dependencies] pest_meta = "2.9.2" diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 1fe4bad1..8f3d4d6a 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -1,6 +1,5 @@ -use serde::{Deserialize, Serialize}; -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub enum AggFunc { Sum, Avg, @@ -11,7 +10,7 @@ pub enum AggFunc { /// Arithmetic and comparison operators. An enum rather than a `String`, so an /// invalid operator is caught at parse time — design-principles §2. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum BinOp { Add, Sub, @@ -26,13 +25,13 @@ pub enum BinOp { Ge, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Filter { pub category: String, pub item: String, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub enum Expr { Number(f64), Ref(String), @@ -45,7 +44,7 @@ pub enum Expr { Is(Filter), } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone)] pub struct Formula { /// The raw formula text, e.g. "Profit = Revenue - Cost" pub raw: String, @@ -59,7 +58,6 @@ pub struct Formula { pub filter: Option, /// How this formula's target is drawn, overriding the view's `format:` for /// that one measure. `None` means the view answers — `context/code/core.md`. - #[serde(default)] pub format: Option, } diff --git a/crates/improvise-io/Cargo.toml b/crates/improvise-io/Cargo.toml index 1ec96762..bb8d367f 100644 --- a/crates/improvise-io/Cargo.toml +++ b/crates/improvise-io/Cargo.toml @@ -13,7 +13,7 @@ anyhow = "1" chrono = { version = "0.4", features = ["serde"] } csv = "1" flate2 = "1" -indexmap = { version = "2", features = ["serde"] } +indexmap = "2" pest = "2.9.2" pest_derive = "2.9.2" serde = { version = "1", features = ["derive"] } From ec548862ac14ec234de7e8f841b9af5b56cb4faf Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:50:30 -0700 Subject: [PATCH 06/14] Cargo.lock: improvise-formula no longer depends on serde --- Cargo.lock | 3 --- 1 file changed, 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d1f6969c..b80ea5bd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -897,7 +897,6 @@ dependencies = [ "pest_derive", "pest_meta", "proptest", - "serde", ] [[package]] @@ -997,8 +996,6 @@ checksum = "cc4e190f5d26ca7051642629da2c52fc03bde85a03197c99408dcd291734c855" dependencies = [ "equivalent", "hashbrown 0.17.1", - "serde", - "serde_core", ] [[package]] From 56f8d37e95d03a3f8373d5ac321a16d87e9bcd53 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:53:20 -0700 Subject: [PATCH 07/14] fmt: leading blank line left by the removed serde import --- crates/improvise-formula/src/ast.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/improvise-formula/src/ast.rs b/crates/improvise-formula/src/ast.rs index 8f3d4d6a..57641035 100644 --- a/crates/improvise-formula/src/ast.rs +++ b/crates/improvise-formula/src/ast.rs @@ -1,4 +1,3 @@ - #[derive(Debug, Clone)] pub enum AggFunc { Sum, From 6e021699d48d527bc1f857be9ae2c6d3dcf5c844 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:07:38 -0700 Subject: [PATCH 08/14] =?UTF-8?q?improvise-sjdb:=20red=20=E2=80=94=20an=20?= =?UTF-8?q?empty=20cell=20beside=20a=20collapsed=20measure=20group?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The host projects Feb x Units as a Record, and #217 refuses the write: aggregate_refusal judges the record at its first column, the Money fold. Also pins that a stored record stays writable whatever the host view does. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/host.rs | 69 +++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index e7d98541..92fff1e7 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1512,6 +1512,75 @@ mod tests { assert_eq!(value, Some(&CellValue::Number(7.0))); } + /// improvise-sjdb: an empty record is judged at its own cell, not at the first + /// column carrying it — see context/test-evidence/command.md. + #[test] + fn an_empty_cell_beside_a_collapsed_measure_group_is_writable() { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + for month in ["Jan", "Feb"] { + wb.add_item("Month", month).unwrap(); + } + for measure in ["Revenue", "Cost"] { + wb.add_item_in_group("_Measure", measure, "Money").unwrap(); + } + wb.add_item("_Measure", "Units").unwrap(); + let jan = month_cell("Jan", "Revenue"); + wb.model.set_cell(jan, CellValue::Number(10.0)); + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().toggle_group_collapse("_Measure", "Money"); + let mut app = App::new(wb, None); + app.rebuild_layout(); + let target = month_cell("Feb", "Units"); + let units = (0..app.layout.col_count()) + .find(|&c| app.layout.cell_key(1, c).as_ref() == Some(&target)) + .expect("setup: Feb x Units is drawn"); + let fold = (0..app.layout.col_count()) + .find(|&c| app.layout.folds_a_group(1, c)) + .expect("setup: a folded measure column"); + assert!(fold < units, "setup: the fold comes first on Feb's row"); + let id = crate::ui::project::cell_id(&app, &app.layout, 1, units); + let projected = matches!(id, Some(improvise_protocol::CellId::Record(_))); + assert!(projected, "setup: Feb x Units projects as {id:?}"); + let mut host = Host::new(app); + let answer = wire_write(&mut host, &target, false); + let accepted = answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)); + assert!(accepted, "a projected Record was refused: {answer:?}"); + let model = &host.app().model_state.workbook.model; + assert_eq!(model.get_cell(&target), Some(&CellValue::Number(7.0))); + } + + /// A STORED record is writable whatever the host's view does to it — see + /// context/test-evidence/command.md (improvise-sjdb). + #[test] + fn a_stored_record_the_hosts_view_aggregates_is_writable() { + let mut wb = workbook(); + wb.model.set_cell(cell("South"), CellValue::Number(1.0)); + wb.add_category("Product").unwrap(); + wb.add_item("Product", "Widget").unwrap(); + wb.active_view_mut().set_axis("Product", Axis::None); + let mut app = App::new(wb, None); + app.rebuild_layout(); + let south = (0..app.layout.row_count()) + .flat_map(|r| (0..app.layout.col_count()).map(move |c| (r, c))) + .find(|&(r, c)| app.layout.cell_key(r, c).as_ref() == Some(&cell("South"))) + .expect("setup: South's cell is drawn"); + let id = crate::ui::project::cell_id(&app, &app.layout, south.0, south.1); + let aggregate = matches!(id, Some(improvise_protocol::CellId::Aggregate(_))); + assert!(aggregate, "setup: the host's view aggregates South: {id:?}"); + let mut host = Host::new(app); + let answer = wire_write(&mut host, &cell("South"), false); + let accepted = answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Accepted)); + assert!(accepted, "a stored record was refused: {answer:?}"); + let model = &host.app().model_state.workbook.model; + assert_eq!(model.get_cell(&cell("South")), Some(&CellValue::Number(7.0))); + } + /// Clause 4 must not fire on a frame that merely names a record it is /// creating. See `context/test-evidence/command.md`. #[test] From c312431154987c4d776cf0ac5b69dfd63235cec8 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:08:02 -0700 Subject: [PATCH 09/14] improvise-sjdb: judge an unstored record at its own cell, not its first column RecordHash drops _Measure, so every column of a row carries the same record, and with a collapsed _Measure group the first is the fold. aggregate_refusal now finds the position by the whole key the write names. Rival for the stored-record test: stored records through cell_id fails it alone. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 11 +++++++++-- context/test-evidence/command.md | 26 ++++++++++++++++++++++++++ crates/improvise-command/src/ui/app.rs | 15 ++++++++------- 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index e4704362..f63eb334 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3422,8 +3422,15 @@ The question it asks is the projection's own, never a second derivation of it: - A record found **in the store** is a record, whatever view anyone is on. `RecordHash` is document-scoped (gotcha 44). - A record found **only through the layout** is asked of `project::cell_id` at - the position where it was found. `Aggregate`, a fold or a sum over a None-axis - category, is refused. + the position of **its cell**: the one whose key is the whole key the write + names, record and measure together. `Aggregate`, a fold or a sum over a + None-axis category, is refused. It must be the whole key, not the first + position carrying the record (improvise-sjdb). `RecordHash` drops `_Measure`, + so every column of a row carries the same record, and with a collapsed + `_Measure` group the first of them can be the fold. That refused the empty + `Units` cell beside a `Money` fold, which the host itself projects as a + `Record`. A write whose whole key the host's grid does not draw is not + refused here, as it was not before this rule existed. **Why the `vec![]` arm stays.** A reviewer asked whether a `SetCell` whose cell does not resolve at all, which `model_effects` turns into `vec![]` and so diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index ac34354b..7e3ca124 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5611,3 +5611,29 @@ it and when a hand-rolled basis omits it. The second half was expected to be `ChainBroken`, and the test said otherwise: the create exemption covers only commands after the first in a frame. That surprise is why it is asserted rather than argued. + +### an_empty_cell_beside_a_collapsed_measure_group_is_writable + +improvise-sjdb, a regression PR #217 introduced and its review found. `_Measure` +is on Column with Revenue and Cost in a collapsed group `Money` and `Units` +ungrouped, and the Feb row is empty. The host projects Feb × Units as a +`Record`. A caught-up `SetCell` on it was answered `Rejected(MutationRefused)` +at `bc060f67`. It was accepted before #217. `aggregate_refusal` found the +unstored record by `RecordHash` alone, which drops `_Measure`, so it judged the +record at the first column on Feb's row, the `Money` fold, which `cell_id` calls +`Aggregate`. The setup asserts that the fold comes before `Units` on the row and +that the target projects as `Record`, so a fixture where the first column is +harmless cannot pass. The fix looks the cell up by its whole key. The rival is +the old lookup by record, which is this test's red commit, `6e021699`. + +### a_stored_record_the_hosts_view_aggregates_is_writable + +improvise-sjdb, from the #217 review. South/Revenue is stored, then `Product` is +added on `Axis::None` in the host's view, so the host projects South's position +as `Aggregate`. A stored record is a record wherever it is drawn, because +`RecordHash` is document-scoped (gotcha 44). A records-mode client, or one on +another view, may write it. The setup asserts the host really does project +`Aggregate` there, so the test cannot pass on a view that draws South plainly. +Rival: stored records sent through `cell_id` like unstored ones. That rival +fails this test and nothing else in `improvise-command`. Before this test, +nothing in the suite saw it. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b2c04003..0113359c 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -583,24 +583,25 @@ impl App { f.target ) }); - derived.or_else(|| self.aggregate_refusal(cell.record)) + derived.or_else(|| self.aggregate_refusal(cell.record, &key)) } /// A record the store does not hold is known only by its layout position, and - /// is a record only where the projection there says `Record`, never `Aggregate`. - fn aggregate_refusal(&self, hash: RecordHash) -> Option { + /// is a record only where the projection at ITS CELL says so (improvise-sjdb). + fn aggregate_refusal(&self, hash: RecordHash, key: &CellKey) -> Option { let stored = self.model_state.workbook.model.data.resolve_record(hash); - let (row, col) = stored.map_or_else(|| self.record_position(hash), |_| None)?; + let (row, col) = stored.map_or_else(|| self.cell_position(key), |_| None)?; let projected = crate::ui::project::cell_id(self, &self.layout, row, col); matches!(projected, Some(CellId::Aggregate(_))).then(|| { "an aggregate combines several records, so there is no one cell to write".into() }) } - /// Where in the current layout a cell of record `hash` sits, if anywhere. - fn record_position(&self, hash: RecordHash) -> Option<(usize, usize)> { + /// Where in the current layout the cell `key` sits, if anywhere. By the whole + /// key: a `RecordHash` drops `_Measure`, so a row's columns all share one. + fn cell_position(&self, key: &CellKey) -> Option<(usize, usize)> { self.layout_cells() - .find(|(_, _, key)| RecordHash::of(key) == hash) + .find(|(_, _, at)| at == key) .map(|(row, col, _)| (row, col)) } From 6026768ceb6419ae6a0ad3ffed4cc592be7fea63 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:10:36 -0700 Subject: [PATCH 10/14] rustfmt the sjdb tests Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/ui/host.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 92fff1e7..9502dec0 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1529,7 +1529,8 @@ mod tests { wb.model.set_cell(jan, CellValue::Number(10.0)); wb.active_view_mut().set_axis("Month", Axis::Row); wb.active_view_mut().set_axis("_Measure", Axis::Column); - wb.active_view_mut().toggle_group_collapse("_Measure", "Money"); + wb.active_view_mut() + .toggle_group_collapse("_Measure", "Money"); let mut app = App::new(wb, None); app.rebuild_layout(); let target = month_cell("Feb", "Units"); @@ -1578,7 +1579,10 @@ mod tests { .any(|a| matches!(a.msg, ServerFrame::Accepted)); assert!(accepted, "a stored record was refused: {answer:?}"); let model = &host.app().model_state.workbook.model; - assert_eq!(model.get_cell(&cell("South")), Some(&CellValue::Number(7.0))); + assert_eq!( + model.get_cell(&cell("South")), + Some(&CellValue::Number(7.0)) + ); } /// Clause 4 must not fire on a frame that merely names a record it is From 993418f1431947d2bbfb79555ea0a0cbb3450d6f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:11:37 -0700 Subject: [PATCH 11/14] improvise-d7aj: pin the fold memo to the whole key, and the diamond A formula met at two fold keys in one evaluation (an IF branch naming an item of another category) must be evaluated at each. A memo keyed by the formula alone read circular there and passed the whole suite; this test fails under it and nothing else does. The diamond A1 = B2 + C3 over D is pinned too. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/core.md | 27 +++++++++++++++++ crates/improvise-core/src/model/types.rs | 37 ++++++++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index d60ca91f..a83d7f08 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1481,6 +1481,33 @@ because the depth bound the fold path used before is gone. Without the marker the evaluation recurses until the stack runs out, which is why the marker is there rather than being an optimisation. +### one_formula_at_two_fold_keys_is_evaluated_at_each + +improvise-d7aj, found by the adversarial re-review of PR #203 at `b3379d1a`. The +fold memo in `fold_formula_once` is keyed by the whole `CellKey`, and nothing +pinned that: both tests above keep one key context throughout, so a memo keyed +by the formula alone passed the whole workspace. A reference to an item of +**another** category moves the key while `_Measure` names the same formula. +Stores A (Jan 3, Feb 4) and B (Jan 100, Feb 200), `G = Revenue * 2`, +`F = IF(Revenue > 50, G, G + B)`. At `(Q1, A, F)` Revenue is 7, so F is +`G + B`: G at A is 14, and `B` reads F at `(Q1, B)`, where Revenue is 300, so +that F is G at B, 600. The answer is **614**. Rival: the memo keyed by +`formula.target` alone. F is still in progress at `(Q1, A)` when F at `(Q1, B)` +is asked, so that key reads `circular`. Injected into a full workspace run, it +fails this test and nothing else. The reviewer's probe, now committed. + +### a_diamond_at_a_fold_is_not_circular + +improvise-d7aj's second case, which passed at `b3379d1a` and was pinned by +nothing. `A1 = B2 + C3`, `B2 = D * 2`, `C3 = D * 3`, `D = Revenue` reaches D by +two paths in one fold evaluation, so the second reach must find D *finished* +and not *in progress*. It asserts **35** over D = 7 and exactly four +evaluations, so a memo that re-evaluates D passes neither. Rival: never store a +finished value, so only the in-progress marker stays. This test fails with +`circular`, and so does `a_fold_evaluates_each_formula_it_reaches_once`, whose +doubling chain is also a diamond at every step. So this test is a named pin of +that shape rather than the only guard against it. + ### a_fold_answers_is_and_where_on_its_category_as_the_none_axis_does improvise-lnkb, ruled by the orchestrator: on a fold, the folded category reads diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index b3b5c6b7..73e86c21 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -1715,6 +1715,43 @@ mod model_tests { ); } + /// improvise-d7aj: the memo is per KEY, not per formula — one formula met at two + /// fold keys in one evaluation. Evidence: context/test-evidence/core.md. + #[test] + fn one_formula_at_two_fold_keys_is_evaluated_at_each() { + let mut m = quarter_model(&[ + ("Jan", "A", 3.0), + ("Feb", "A", 4.0), + ("Jan", "B", 100.0), + ("Feb", "B", 200.0), + ]); + for text in ["G = Revenue * 2", "F = IF(Revenue > 50, G, G + B)"] { + m.add_formula(crate::formula::parse_formula(text, "_Measure").unwrap()); + } + let key = coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "F")]); + assert_eq!( + m.evaluate_covering(&key, Aggregation::Exact, &|_, _| true), + Some(CellValue::Number(614.0)), + "G at A (14) plus F at B, which is G at B (600)" + ); + } + + /// improvise-d7aj: a formula reached twice by two paths is finished, not in + /// progress, the second time. Evidence: context/test-evidence/core.md. + #[test] + fn a_diamond_at_a_fold_is_not_circular() { + let mut m = quarter_model(&[("Jan", "A", 3.0), ("Feb", "A", 4.0)]); + for text in ["A1 = B2 + C3", "B2 = D * 2", "C3 = D * 3", "D = Revenue"] { + m.add_formula(crate::formula::parse_formula(text, "_Measure").unwrap()); + } + let key = coord(&[("Month", "Q1"), ("Store", "A"), ("_Measure", "A1")]); + super::FOLD_FORMULA_EVALUATIONS.with(|n| n.set(0)); + let value = m.evaluate_covering(&key, Aggregation::Exact, &|_, _| true); + let evaluations = super::FOLD_FORMULA_EVALUATIONS.with(|n| n.get()); + assert_eq!(value, Some(CellValue::Number(35.0)), "14 + 21 over D = 7"); + assert_eq!(evaluations, 4, "A1, B2, C3 and D once each"); + } + /// Evidence: context/test-evidence/core.md. #[test] fn a_groups_stored_values_fold_once_not_per_member() { From f85fddbb2f3dc357b08d66aa997344ee0eb8d2ad Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:13:25 -0700 Subject: [PATCH 12/14] harness/opfs-contract: refresh Cargo.lock for the dropped serde features (improvise-tt22) --- harness/opfs-contract/Cargo.lock | 3 --- 1 file changed, 3 deletions(-) diff --git a/harness/opfs-contract/Cargo.lock b/harness/opfs-contract/Cargo.lock index d20a9d11..385b3e29 100644 --- a/harness/opfs-contract/Cargo.lock +++ b/harness/opfs-contract/Cargo.lock @@ -196,7 +196,6 @@ dependencies = [ "anyhow", "pest", "pest_derive", - "serde", ] [[package]] @@ -228,8 +227,6 @@ checksum = "cc4e190f5d26ca7051642629da2c52fc03bde85a03197c99408dcd291734c855" dependencies = [ "equivalent", "hashbrown", - "serde", - "serde_core", ] [[package]] From 21e239dca5bc5cd6db2ea6ea86feac6beb77ad9a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:15:57 -0700 Subject: [PATCH 13/14] Model's Debug renders its HashMaps in key order, so the effect fingerprints cannot flake (improvise-pc3b) --- context/code/formula-and-io.md | 20 +++++++-- context/test-evidence/command.md | 26 +++++++++++ crates/improvise-command/src/ui/effect.rs | 54 ++++++++++++++++++++++- crates/improvise-core/src/model/types.rs | 22 ++++++++- 4 files changed, 116 insertions(+), 6 deletions(-) diff --git a/context/code/formula-and-io.md b/context/code/formula-and-io.md index 3c9efe8f..eacb06f1 100644 --- a/context/code/formula-and-io.md +++ b/context/code/formula-and-io.md @@ -495,10 +495,22 @@ compiler named every other consumer. Two remain, and both are the wire: untouched. One test consumer had to move: the effect-classification tests fingerprinted the -model and the views through `serde_json`. They now use `Debug`. That is no weaker: -it renders every field, including the derived `formula_cache` the JSON skipped. -`format_md` was rejected as the replacement, because it skips the virtual views -and sorts the data, so it would have been a weaker fingerprint. +model and the views through `serde_json`. They now use `Debug`. `format_md` was +rejected as the replacement, because it skips the virtual views and sorts the +data, so it would have been a weaker fingerprint. + +The first version of this paragraph said `Debug` was "no weaker" because it +renders every field, including the derived `formula_cache` the JSON skipped. +**That was wrong (improvise-pc3b).** A derived `Debug` prints a `HashMap` in hash +order, and `recompute_formulas` clears and refills the cache. So a correct +`ViewOnly` effect such as `RebuildLayout` changed the fingerprint in 90 of 200 +runs. The suite passed only because the fixture had no formulas, so the cache +was empty. `Model` now has a hand-written `Debug` that renders `formula_cache` +and `measure_agg` in key order. The fingerprint keeps every field, including the +derived one, and a refill in a different hash order is no longer read as a +change. `recomputing_formulas_leaves_the_model_fingerprint_alone` pins this. +`view_only_effects_leave_shared_state_alone` now runs over a formula-bearing +fixture, for the same reason. `parse_number_cell` returns `CellValue::Error` naming the offending text rather than a fabricated `0.0`. No token the current grammar admits actually fails diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 3ffb456a..d9409ff9 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5599,3 +5599,29 @@ that is deliberate. Its helpers seed a client *after* mutating the host (mode, buffers, cursor), so a `Snapshot` would drop exactly the state those tests stage. This test is what makes the copy faithful at startup. + +### ui::effect::tests::recomputing_formulas_leaves_the_model_fingerprint_alone + +improvise-pc3b, found by the review of PR #224 at `e62a289`. That PR moved the +effect-classification fingerprints from `serde_json` to `Debug`. The derived +`Debug` for `Model` prints `formula_cache`, a `HashMap` that `recompute_formulas` +clears and refills, so it prints in hash order. The reviewer measured a correct +`ViewOnly` effect (`RebuildLayout`) changing the fingerprint in 90 of 200 runs +once formulas were present. `test_app` has none, so the suite was green over a +latent flake. + +The test builds `formula_app` (two measures across a 2×2 grid, plus +`Profit = Revenue - Cost` and `Margin = Profit / Revenue`) 32 times. Each build +is a fresh `App`, so each gets fresh hash states. Each time, it asserts that +`RebuildLayout` leaves `model_fingerprint` unchanged. One flip in about two makes +32 independent tries fail near-certainly. + +**Red before the fix**: the two renderings differed only in `formula_cache`'s +entry order. The fix is a hand-written `Debug` for `Model` that renders +`formula_cache` and `measure_agg` in key order. + +**Rival, run:** keep the hand-written `Debug` but print `&self.formula_cache` +unsorted. This test fails. `view_only_effects_leave_shared_state_alone` now runs +over `formula_app` too, so every `ViewOnly` case is checked against a populated +cache. The two earlier injections (a `SetStatus` that writes the model, and one +that writes a view's `number_format`) still fail it. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index dbb66317..d9ac1d24 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1408,6 +1408,57 @@ mod tests { format!("{:?}", app.model_state.workbook.views) } + /// `test_app` plus measures, data and two chained formulas, so the derived + /// formula cache holds several entries for a recompute to refill. + fn formula_app() -> App { + let mut app = test_app(); + let wb = &mut app.model_state.workbook; + for m in ["Revenue", "Cost"] { + wb.add_item("_Measure", m).unwrap(); + } + let mut n = 1.0; + for t in ["Food", "Clothing"] { + for mo in ["Jan", "Feb"] { + for (m, scale) in [("Revenue", 10.0), ("Cost", 3.0)] { + let key = CellKey::new(vec![ + ("Type".into(), t.into()), + ("Month".into(), mo.into()), + ("_Measure".into(), m.into()), + ]); + wb.model.set_cell(key, CellValue::Number(n * scale)); + } + n += 1.0; + } + } + for raw in ["Profit = Revenue - Cost", "Margin = Profit / Revenue"] { + AddFormula { + raw: raw.into(), + target_category: "_Measure".into(), + } + .apply(&mut app); + } + app.rebuild_layout(); + app + } + + /// improvise-pc3b: the fingerprint must not read a derived cache's hash + /// order. Evidence: `context/test-evidence/command.md`. + #[test] + fn recomputing_formulas_leaves_the_model_fingerprint_alone() { + // Each App gets fresh hash states; one run observed ~45% flips, so 32 + // independent tries make a hash-ordered fingerprint fail near-certainly. + for _ in 0..32 { + let mut app = formula_app(); + let before = model_fingerprint(&app); + RebuildLayout.apply(&mut app); + assert_eq!( + model_fingerprint(&app), + before, + "a ViewOnly recompute changed the model fingerprint" + ); + } + } + /// improvise-mae: rather than restate each declaration, apply the effect and /// check the shared state it claims not to touch is genuinely unchanged. #[test] @@ -1445,7 +1496,8 @@ mod tests { EffectKind::ViewOnly, "{effect:?} is in the ViewOnly case list" ); - let mut app = test_app(); + // With formulas, so `RebuildLayout` refills a non-empty cache (pc3b). + let mut app = formula_app(); let model_before = model_fingerprint(&app); let views_before = view_config_fingerprint(&app); effect.apply(&mut app); diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 09dcad98..5973c1d1 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -237,7 +237,7 @@ impl Aggregation { /// Pure-data document model: categories, cells, formulas. It knows nothing /// about views — see [`crate::workbook::Workbook`] and core.md §10. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct Model { pub name: String, pub categories: IndexMap, @@ -251,6 +251,26 @@ pub struct Model { formula_cache: HashMap, } +/// Every field, with the two `HashMap`s in key order: a recompute refills the +/// cache in hash order, which is not a change (improvise-pc3b, gotcha 18). +impl std::fmt::Debug for Model { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut aggs: Vec<_> = self.measure_agg.iter().collect(); + aggs.sort_by(|a, b| a.0.cmp(b.0)); + let mut cache: Vec<_> = self.formula_cache.iter().collect(); + cache.sort_by(|a, b| a.0.0.cmp(&b.0.0)); + f.debug_struct("Model") + .field("name", &self.name) + .field("categories", &self.categories) + .field("data", &self.data) + .field("formulas", &self.formulas) + .field("next_category_id", &self.next_category_id) + .field("measure_agg", &aggs) + .field("formula_cache", &cache) + .finish() + } +} + impl Model { pub fn new(name: impl Into) -> Self { let name = name.into(); From 4d2252373e8346b01481ac4299a3f7e586359d84 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 08:26:48 -0700 Subject: [PATCH 14/14] every_shipped_document_loads walks what .gitignore un-ignores, harness/fixtures included (improvise-qlrh) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tui.md | 28 ++++++++++++++++-- tests/shipped_documents_load.rs | 52 +++++++++++++++++++++++---------- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index 080a27b0..48c7165e 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -428,9 +428,31 @@ inputs. This test is aimed at exactly that gap. **It is deliberately a directory walk rather than a list of names**: a list is a second place to remember, and the file that gets forgotten is the one nobody -added to it. The scope is the two directories `.gitignore` un-ignores — -`examples/` and `web/`; everything else matching `*.improv` is somebody's -working file. +added to it. The scope is the directories `.gitignore` un-ignores `*.improv` +in. Everything else matching `*.improv` is somebody's working file. + +**The directory list is read from `.gitignore`, not written in the test +(improvise-qlrh).** The test used to hard-code `["examples", "web"]`, a list of +exactly the kind the paragraph above warns against. When improvise-plcx +un-ignored `harness/fixtures/*.improv`, nothing added that directory to the +walk. Its two fixtures were then loaded only by the Playwright job, where a +missing seed degrades to an empty model and surfaces as a missing-label +failure, not a load error. `shipped_directories` now takes every +`!/*.improv` line, so un-ignoring a directory is what enlists it. + +The precondition is per directory: each derived directory must yield at least +one document. The old whole-walk `>= 2` could not see a directory silently lost. + +**Red, measured at `705c92d`:** `harness/fixtures/tall.improv` was replaced with +`{"legacy": true}`, and the old test still passed. **Rivals, each run against +the new test:** + +- **That same broken fixture.** Fails with "…/harness/fixtures/tall.improv does + not load". +- **A walk that drops every `harness` path.** Fails the per-directory + precondition, naming `harness/fixtures`. +- **A derivation that matches no `.gitignore` line.** Fails "…must un-ignore at + least one *.improv directory". **It matches on the full name, not on `Path::extension()`**: `.improv.gz` is the same document gzipped and `extension()` calls it `gz`, so matching on the diff --git a/tests/shipped_documents_load.rs b/tests/shipped_documents_load.rs index 5b4d5467..deb725e4 100644 --- a/tests/shipped_documents_load.rs +++ b/tests/shipped_documents_load.rs @@ -3,11 +3,21 @@ use std::path::{Path, PathBuf}; -fn shipped_documents() -> Vec { - let root = Path::new(env!("CARGO_MANIFEST_DIR")); - let mut found: Vec = ["examples", "web"] - .iter() - .flat_map(|dir| std::fs::read_dir(root.join(dir)).expect("a shipped-document directory")) +/// The directories `.gitignore` un-ignores `*.improv` in: the definition of +/// "shipped", read from the one place that states it (improvise-qlrh). +fn shipped_directories(root: &Path) -> Vec { + let gitignore = std::fs::read_to_string(root.join(".gitignore")).expect("a .gitignore"); + gitignore + .lines() + .filter_map(|line| line.trim().strip_prefix('!')) + .filter_map(|pattern| pattern.strip_suffix("/*.improv")) + .map(|dir| root.join(dir)) + .collect() +} + +fn documents_in(dir: &Path) -> Vec { + let mut found: Vec = std::fs::read_dir(dir) + .expect("a shipped-document directory") .map(|entry| entry.expect("a readable directory entry").path()) // `extension()` of `x.improv.gz` is `gz`, so matching on the extension // alone would skip it and put back the hole this test closes. @@ -22,21 +32,31 @@ fn shipped_documents() -> Vec { #[test] fn every_shipped_document_loads() { - let documents = shipped_documents(); + let root = Path::new(env!("CARGO_MANIFEST_DIR")); + let dirs = shipped_directories(root); assert!( - documents.len() >= 2, - "precondition: the walk must find the documents that exist, got {documents:?}" + !dirs.is_empty(), + "precondition: .gitignore must un-ignore at least one *.improv directory" ); - for path in documents { - let workbook = improvise::persistence::load(&path) - .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); + for dir in dirs { + let documents = documents_in(&dir); + // Per directory, so a walk that silently loses one cannot pass (qlrh). assert!( - workbook.views.contains_key(&workbook.active_view), - "{} loads but its initial view {:?} is not one of {:?}", - path.display(), - workbook.active_view, - workbook.views.keys().collect::>() + !documents.is_empty(), + "precondition: {} is un-ignored for *.improv but the walk found none there", + dir.display() ); + for path in documents { + let workbook = improvise::persistence::load(&path) + .unwrap_or_else(|e| panic!("{} does not load: {e}", path.display())); + assert!( + workbook.views.contains_key(&workbook.active_view), + "{} loads but its initial view {:?} is not one of {:?}", + path.display(), + workbook.active_view, + workbook.views.keys().collect::>() + ); + } } }