From f47297c3a7db1bde719a17be53c1c217eaf457e7 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:42:00 -0700 Subject: [PATCH 01/15] fix(core): a collapsed group's header survives pruning, and so does its GroupEnd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-lhi4 and improvise-u6gc: two defects in one match block in prune_axis_items, pulling in opposite directions. The naive repair of the first deepens the second, so they are fixed together. lhi4: a collapsed group contributes ZERO AxisEntry::DataItems — expand_category emits the GroupHeader and then continues past every item — so a keeper that kept a header only if a data item under it survived always dropped it. With prune_empty on, the group left the grid entirely and `z` could not reach it: collapse became one-way, recoverable only through :toggle-group. u6gc: dropping a pruned header's GroupEnd re-opens an EARLIER run, so the nearest-preceding walk in row_group_for/col_group_for — and AxisFrame::group_of on the wire — answered with a group that had already ended. That is exactly the inheritance improvise-gd83 added GroupEnd to prevent, so gd83 was regressed whenever prune_empty was on. The two emptinesses are distinguishable from the entry stream alone, so the keeper needs no &View (which GridLayout has already dropped): a run holding no DataItem at all was never expanded — it is collapsed, and its header stays — a run whose items were all rejected is genuinely empty and goes. A GroupEnd is kept whenever a run is open in the output, which closes a re-opened earlier header even though the header between them was pruned. gotcha 1's shape is unchanged: still a predicate over the data index, no flag vector, no parallel length a caller could size wrongly. Three tests, because each is satisfied by a fix that misses another; a_header_whose_items_all_prune_away_is_dropped passes before the change and is the discriminator lhi4's acceptance criteria ask for. Evidence, including the three injected faults and which pair each one fails, is in context/test-evidence/core.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-core/src/view/layout.rs | 134 +++++++++++++++++++++-- 1 file changed, 126 insertions(+), 8 deletions(-) diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 4cdd314c..b5591f4a 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -126,27 +126,52 @@ fn data_index_to_visual(items: &[AxisEntry], data_idx: usize) -> Option { .nth(data_idx) } +/// The run a header opened has ended. A header still pending saw either no +/// item at all — it is collapsed, and survives — or only rejected ones. +fn close_pending_run( + kept: &mut Vec, + pending: Option, + had_item: bool, +) -> bool { + pending + .filter(|_| !had_item) + .map(|h| kept.push(h)) + .is_some() +} + /// Drop the data items `keep` rejects, keeping a header only if an item under -/// it survives. A predicate, not a flag vector — why: core.md §6. +/// it survives or the group is collapsed. A predicate — why: core.md §6. fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { - let mut kept = Vec::new(); + let mut kept: Vec = Vec::new(); let mut pending_header: Option = None; + let mut run_had_item = false; + let mut run_open = false; let mut data_idx = 0usize; for entry in items.drain(..) { match &entry { + // A header ends the previous run: another group's header is where + // `expand_category` stops emitting a `GroupEnd` (improvise-gd83). AxisEntry::GroupHeader { .. } => { + run_open |= close_pending_run(&mut kept, pending_header.take(), run_had_item); pending_header = Some(entry); + run_had_item = false; + } + // Kept whenever a run is open in `kept`, even where the header it + // closed was pruned: an earlier one is then re-opened (u6gc). + AxisEntry::GroupEnd => { + run_open |= close_pending_run(&mut kept, pending_header.take(), run_had_item); + run_had_item = false; + if run_open { + kept.push(entry); + run_open = false; + } } - // The run it closes may have been pruned away entirely, in which - // case the header is still pending and both are dropped together. - AxisEntry::GroupEnd => match pending_header.take() { - Some(_) => {} - None => kept.push(entry), - }, AxisEntry::DataItem(_) => { + run_had_item = true; if keep(data_idx) { if let Some(h) = pending_header.take() { kept.push(h); + run_open = true; } kept.push(entry); } @@ -154,6 +179,7 @@ fn prune_axis_items(items: &mut Vec, keep: impl Fn(usize) -> bool) { } } } + close_pending_run(&mut kept, pending_header.take(), run_had_item); *items = kept; } @@ -1316,4 +1342,96 @@ mod tests { let layout = GridLayout::new(&wb.model, wb.active_view()); assert_eq!(layout.col_group_for(0), None); } + + /// Fund exactly the `(Month, Type=Food)` cells named, so `prune_empty` + /// keeps the rows the fixture pays for. context/test-evidence/core.md. + fn grouped_month_book(months: &[(&str, Option<&str>)], funded: &[&str]) -> Workbook { + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + wb.add_category("Type").unwrap(); + for (name, group) in months { + match group { + Some(g) => wb.add_item_in_group("Month", *name, *g).unwrap(), + None => wb.add_item("Month", *name).unwrap(), + }; + } + wb.add_item("Type", "Food").unwrap(); + for m in funded { + wb.model.set_cell( + CellKey::new(vec![ + ("Month".into(), (*m).into()), + ("Type".into(), "Food".into()), + ]), + CellValue::Number(1.0), + ); + } + wb.active_view_mut().prune_empty = true; + wb + } + + fn has_header(items: &[AxisEntry], group: &str) -> bool { + items + .iter() + .any(|e| matches!(e, AxisEntry::GroupHeader { group_name, .. } if group_name == group)) + } + + /// improvise-lhi4: a COLLAPSED group contributes zero data items, so the + /// pruner must not read its empty run as "pruned away". + #[test] + fn a_collapsed_groups_header_survives_pruning() { + let mut wb = grouped_month_book(&[("Jan", Some("Q1")), ("Sep", None)], &["Jan", "Sep"]); + wb.active_view_mut().toggle_group_collapse("Month", "Q1"); + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 1, "only Sep is a data row"); + assert!( + has_header(&layout.row_items, "Q1"), + "Q1 is collapsed, not empty — its header is the only way back" + ); + assert_eq!( + layout.row_group_for(0), + None, + "Sep must not inherit the collapsed run" + ); + } + + /// improvise-lhi4's other half, and what separates the two cases: a header + /// whose items really did prune away still goes. + #[test] + fn a_header_whose_items_all_prune_away_is_dropped() { + let wb = grouped_month_book(&[("Jan", Some("Q1")), ("Mar", Some("Q2"))], &["Jan"]); + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 1, "only Jan is funded"); + assert!(has_header(&layout.row_items, "Q1"), "Jan survives under Q1"); + assert!( + !has_header(&layout.row_items, "Q2"), + "Q2's only item pruned away, so its header goes with it" + ); + } + + /// improvise-u6gc: pruning the intervening header re-opens the earlier + /// run, so the `GroupEnd` that closed it must survive. Regresses gd83. + #[test] + fn an_ungrouped_item_after_a_pruned_group_belongs_to_no_group() { + let wb = grouped_month_book( + &[ + ("Jan", Some("Q1")), + ("Feb", Some("Q1")), + ("Mar", Some("Q2")), + ("Sep", None), + ], + &["Jan", "Feb", "Sep"], + ); + let layout = GridLayout::with_frozen_records(&wb.model, wb.active_view(), None); + assert_eq!(layout.row_count(), 3, "Mar pruned; Jan, Feb, Sep remain"); + assert_eq!( + layout.row_group_for(0), + Some(("Month".into(), "Q1".into())), + "precondition: Jan is in Q1, so the walk has somewhere wrong to land" + ); + assert_eq!( + layout.row_group_for(2), + None, + "Sep is in no group — Q2's GroupEnd closed Q1's re-opened run" + ); + } } From 09e43c1fcc9caf03d85454d540c555f5813b86dd Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:42:08 -0700 Subject: [PATCH 02/15] fix(command,protocol): cap a column width while it is still usize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-yae4. grid_body narrowed every measured width to u16 and only then added the one-character gap and clamped to MAX_COL_WIDTH, so the arithmetic ran at full width before the cap that makes it safe. A stored 65535-column text value panicked at `w + 1` in debug; in release the sum wrapped to 0 and the column was drawn at MIN_COL_WIDTH — the narrowest column, for the widest value. A 65536-column value never reached the addition: `as u16` truncated it to 0 first, so it wrapped in debug builds too, silently. Widths are now capped while still usize, in the one place every measurement enters (`widen`), which is why header_level_seed folds through it rather than keeping a second `as u16`. The answer is unchanged for every input that did not overflow: (min(w,32) + 1).clamp(5,32) and (w + 1).clamp(5,32) agree everywhere, both saturating at 32 for any w >= 31. improvise_protocol::header_level_widths carried the identical defect on the row-header axis — a 65535-column item label — and is fixed the same way. It is the one home for that rule (gotcha 44), so the row header and the column widths cannot end up with two answers. wide_cell_clamps_without_overflow is un-ignored and extended to the 65536 arm and to an ordinary 20-column value. It asserts `widths == vec![expect]` rather than `all(<= 32)`: the length pins that the fixture really produces ONE column, and the ordinary arm pins that the single column is the one carrying the value — a fixture in which no column was wide would read MIN_COL_WIDTH there and pass a `<= 32` check. Both injected faults are recorded in context/test-evidence/core.md. Not fixed, and out of this ticket's scope: compute_visible_cols accumulates `acc + w` in u16, reachable only from a terminal ~65535 columns wide. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/core.md | 90 +++++++++++++++++++++ crates/improvise-command/src/ui/viewport.rs | 14 ++-- crates/improvise-protocol/src/render.rs | 3 +- tests/bug_hunt_reproductions.rs | 49 ++++++----- 4 files changed, 129 insertions(+), 27 deletions(-) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index eb9d8a53..5993462b 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -612,6 +612,96 @@ Fault injected by deleting either emission: the corresponding test fails with `Some(("Month", "Q1"))` / `Some(("Recipient", "Family"))` against `None`, and `the_two_front_ends_draw_the_same_column_header` fails with it. +### a_collapsed_groups_header_survives_pruning + +### a_header_whose_items_all_prune_away_is_dropped + +### an_ungrouped_item_after_a_pruned_group_belongs_to_no_group + +improvise-lhi4 and improvise-u6gc: two defects in one `match` block, and they +pull in opposite directions, which is why they were fixed together. The naive +repair of the first — stop pruning headers — deepens the second. + +`prune_axis_items` kept a header only if a data item under it survived the +predicate, and dropped a `GroupEnd` whenever the header it closed was still +pending. Both readings are wrong about a case the entry stream can distinguish. + +**lhi4**: a collapsed group contributes **zero** `DataItem`s — `expand_category` +emits the `GroupHeader` and then `continue`s past every item — so its header +was always pruned and the group vanished from the grid entirely. `z` +(toggle-group-under-cursor) resolves the group from `row_group_for(cursor)`, +so with no header and no items no row named it: collapse became one-way from +the grid, recoverable only through `:toggle-group `. + +**u6gc**: dropping a pruned header's `GroupEnd` **re-opens an earlier run**. +With `Jan[Q1], Feb[Q1], Mar[Q2], Sep` and `Mar` empty, the kept list became +`[H(Q1), Jan, Feb, Sep]` and the nearest-preceding walk answered `Q1` for +`Sep` — exactly the inheritance improvise-gd83 added `GroupEnd` to prevent, so +the regression was live whenever `prune_empty` was on. + +The fix distinguishes the two emptinesses **without consulting the `View`**, +which `GridLayout` has already dropped: a run that contained no `DataItem` at +all was never expanded, i.e. collapsed, and its header survives; a run that +contained items the predicate rejected is genuinely empty and goes. A +`GroupEnd` is kept whenever a run is open in the output, so a re-opened earlier +header is closed even though the header between them was pruned. That keeps +gotcha 1's shape — still a predicate over the data index, no `&View` +parameter, no flag vector. + +The three tests are separate because each is satisfied by a fix that misses +another. `a_header_whose_items_all_prune_away_is_dropped` **passes before the +change** and is the discriminator: it is what stops "keep every header" from +counting as a fix for lhi4, and it is the acceptance criterion lhi4 names. + +Faults injected, each against the fixed code, and each fails a **different** +pair — which is the evidence that no one of the three tests stands in for the +others: +- stop keeping the `GroupEnd` (drop the `run_open` push): both membership + tests fail with `Some(("Month", "Q1"))` against `None`; + `a_header_whose_items_all_prune_away_is_dropped` still passes. +- drop the `!had_item` condition in `close_pending_run`, keeping every pending + header: only `a_header_whose_items_all_prune_away_is_dropped` fails. +- invert it to `had_item`, keeping only headers whose items all pruned: + `a_collapsed_groups_header_survives_pruning` and + `a_header_whose_items_all_prune_away_is_dropped` fail, and the `GroupEnd` + test still passes. + +Both fixtures fund their cells explicitly, so `prune_empty` really prunes: on +an unfunded fixture every row prunes and the tests would assert over an empty +grid. `row_count()` is asserted first in each for that reason. + +### wide_cell_clamps_without_overflow + +improvise-yae4. `grid_body` narrowed each measured width to `u16` and only then +added the one-character gap and clamped to `MAX_COL_WIDTH`, so the arithmetic +ran at full width before the cap that makes it safe. A stored 65535-column text +value panicked at `w + 1` in debug; in release the sum wrapped to `0` and the +column was drawn at `MIN_COL_WIDTH` — the *narrowest* column, for the widest +value. A 65536-column value never reached the addition at all: `as u16` +truncated it to `0` first, so it wrapped in debug builds too, silently. + +Widths are now capped while still `usize`, in the one place every measurement +enters (`widen`), which is also why `header_level_seed` folds through it rather +than keeping a second `as u16`. The answer is unchanged for every input that +did not overflow: `(min(w, 32) + 1).clamp(5, 32)` and `(w + 1).clamp(5, 32)` +agree everywhere, because both saturate at 32 for any `w >= 31`. + +`improvise_protocol::header_level_widths` carried the identical defect on the +row-header axis — a 65535-column *item label* — and is fixed the same way. It +is the one home for that rule (gotcha 44), so the row header and the column +widths do not get two answers. + +The test asserts `widths == vec![expect]`, not `all(<= 32)`: the length pins +that the fixture really produces **one** column, and the ordinary 20-column arm +pins that the single column is the one carrying the value — a fixture in which +no column was wide would read `MIN_COL_WIDTH` there and pass a `<= 32` check. + +Faults injected, each against the fixed code: +- `width as u16` in `widen`, debug: the 65535 arm panics with + `attempt to add with overflow`. +- the same, release: the 65536 arm fails with `left: [5]`, `right: [32]` — + the wrap reported as the minimum width. + ### a_trailing_conversion_letter_does_not_erase_the_precision improvise-711n. `parse_number_format` read the precision as diff --git a/crates/improvise-command/src/ui/viewport.rs b/crates/improvise-command/src/ui/viewport.rs index 27f25a95..c6ca72c0 100644 --- a/crates/improvise-command/src/ui/viewport.rs +++ b/crates/improvise-command/src/ui/viewport.rs @@ -62,7 +62,7 @@ pub fn grid_body( true => value.as_ref().map(|v| v.to_string()).unwrap_or_default(), false => crate::format::format_value(value.as_ref(), fmt_comma, fmt_decimals), }; - widen(&mut widths, ci, text.width() as u16); + widen(&mut widths, ci, text.width()); if let Some(sum) = totals.get_mut(ci) { *sum += value.as_ref().and_then(|v| v.as_f64()).unwrap_or(0.0); } @@ -75,7 +75,7 @@ pub fn grid_body( } } for (ci, total) in totals.iter().enumerate() { - let width = format_f64(*total, fmt_comma, fmt_decimals).width() as u16; + let width = format_f64(*total, fmt_comma, fmt_decimals).width(); widen(&mut widths, ci, width); } @@ -114,14 +114,18 @@ fn header_level_seed(layout: &GridLayout, n: usize) -> Vec { AxisEntry::GroupHeader { .. } | AxisEntry::GroupEnd => None, }); for (ci, levels) in items.enumerate().take(n) { - widths[ci] = levels.iter().map(|s| s.width() as u16).max().unwrap_or(0); + for level in levels { + widen(&mut widths, ci, level.width()); + } } widths } -fn widen(widths: &mut [u16], col: usize, width: u16) { +/// Every measured width enters through here and is capped **while still +/// `usize`**, so the `+ 1` gap below cannot overflow (improvise-yae4). +fn widen(widths: &mut [u16], col: usize, width: usize) { if let Some(slot) = widths.get_mut(col) { - *slot = (*slot).max(width); + *slot = (*slot).max(width.min(MAX_COL_WIDTH as usize) as u16); } } diff --git a/crates/improvise-protocol/src/render.rs b/crates/improvise-protocol/src/render.rs index b48f730a..88bed55e 100644 --- a/crates/improvise-protocol/src/render.rs +++ b/crates/improvise-protocol/src/render.rs @@ -396,7 +396,8 @@ pub fn header_level_widths<'a>( for tuple in items { for (level, w) in widest.iter_mut().enumerate() { if let Some(label) = tuple.get(level) { - *w = (*w).max(UnicodeWidthStr::width(label.as_str()) as u16); + let measured = UnicodeWidthStr::width(label.as_str()); + *w = (*w).max(measured.min(MAX_ROW_HEADER_W as usize) as u16); } } } diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 968d87e3..a610db9a 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -363,32 +363,39 @@ fn circular_formula_not_a_plausible_cached_number() { ); } #[test] -#[ignore = "improvise-yae4: known bug reproduction"] fn wide_cell_clamps_without_overflow() { use improvise::{ model::cell::{CellKey, CellValue}, ui::app::App, }; - let mut w = revenue_book(); - w.model.set_cell( - CellKey::new(vec![ - ("Region".into(), "East".into()), - ("_Measure".into(), "Revenue".into()), - ]), - CellValue::Text("x".repeat(65535)), - ); - let app = App::new(w, None); - let widths = improvise::ui::viewport::compute_col_widths( - &app.model_state.workbook.model, - &app.layout, - false, - 0, - ); - assert_eq!( - widths, - vec![improvise::ui::viewport::MAX_COL_WIDTH], - "the Revenue column must reach the width cap" - ); + // 65535 overflows the `+ 1` gap; 65536 truncates the `as u16` cast to 0. + // 20 is the ordinary case the cap must leave alone, gap included. + for (len, expect) in [ + (20usize, 21u16), + (65535, improvise::ui::viewport::MAX_COL_WIDTH), + (65536, improvise::ui::viewport::MAX_COL_WIDTH), + ] { + let mut w = revenue_book(); + w.model.set_cell( + CellKey::new(vec![ + ("Region".into(), "East".into()), + ("_Measure".into(), "Revenue".into()), + ]), + CellValue::Text("x".repeat(len)), + ); + let app = App::new(w, None); + let widths = improvise::ui::viewport::compute_col_widths( + &app.model_state.workbook.model, + &app.layout, + false, + 0, + ); + assert_eq!( + widths, + vec![expect], + "a {len}-column value must yield exactly one column, of width {expect}" + ); + } } #[test] fn ordinary_controls() { From e383aac076ec1a27c756e970c571afed8faaa2b2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:42:19 -0700 Subject: [PATCH 03/15] fix(formula): the walker spells every agg_func and cmp_op, and refuses the rest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-492x. parse_agg_func ended `_ => AggFunc::Count` and parse_cmp_op ended `_ => BinOp::Ge`, so a new alternative in formula.pest would have parsed as COUNT or as >= with no error: MEDIAN(Revenue) evaluating as a count. One of three beads that are the same defect (with improvise-vyrx and improvise-4lgj); design-principles §2 is the rule all three break. A compile error is not on offer here, because agg_func is one pest rule over five string alternatives and pest_derive generates no per-alternative Rule — so the walker gets the idiom this file already uses for a grammar/walker desync, GRAMMAR_INVARIANT, and the accepted set is written down in Rust as a test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/formula-and-io.md | 49 +++++++++++++++++++++++++ crates/improvise-formula/src/parser.rs | 47 +++++++++++++++++++++--- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 4fe396c3..30c3b2e9 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -15,6 +15,55 @@ helper under `###` would pass today and fail the moment that is tightened. ## `improvise-formula/src/parser.rs` +### a_comparison_the_walker_does_not_know_is_not_ge + +The `cmp_op` half of improvise-492x. `parse_cmp_op` ended `_ => BinOp::Ge`, so +a `<>` alternative added to `cmp_op` in `formula.pest` would have parsed as +`>=`: a filter that silently means something else. Same shape and same fix as +the aggregate half above; the token `<>` is chosen because it is the spelling a +future grammar would most plausibly add. + +### an_aggregate_the_walker_does_not_know_is_not_count + +improvise-492x, one of three beads that are the same defect — with +improvise-vyrx (`eval_bool_cached` wildcarding `BinOp` where `eval_bool` lists +it) and improvise-4lgj (the `.improv` pest walker's `_ => {}` arms silently +skipping unhandled rules). design-principles §2 states the rule they all break: +prefer an exhaustive match, so that adding a variant produces a compile error +that names every site needing an update. Each of the three is a place where a +wildcard turned that compile error into a **silent** wrong answer. + +Here the wildcard was `_ => AggFunc::Count`. Adding `| ^"MEDIAN"` to `agg_func` +in `formula.pest` would have made `MEDIAN(Revenue)` parse and evaluate as +`COUNT(Revenue)` — a plausible number, no error, nothing to notice. + +**A compile error is not available and that is a property of the input, not a +choice.** `agg_func` is one pest rule matching five string alternatives, so +`pest_derive` generates no per-alternative `Rule` variant for Rust to match +exhaustively over; the walker's input is a `&str`. So the refusal is the next +best thing and it is the idiom this file already uses for exactly this +condition: `GRAMMAR_INVARIANT`, whose text is "parser out of sync with +formula.pest". The module's own header comment — "tree walkers: infallible, +because pest validated the pair first" — is what makes a panic here the honest +shape rather than a `Result` threaded through every walker for a state no user +input can reach. + +The test injects the ticket's own fault: it calls `parse_agg_func("MEDIAN")` +directly and requires a panic. On the pre-fix code that call returns +`AggFunc::Count` and the test fails. Its companion, +`every_grammar_aggregate_and_comparison_is_spelled_out`, is the anti-clamp +direction — a walker that panicked on *everything* would satisfy the two +`should_panic` tests, so the five aggregates and six comparisons are asserted to +still map to what the grammar says they mean. + +### every_grammar_aggregate_and_comparison_is_spelled_out + +The anti-clamp half of improvise-492x — see +`an_aggregate_the_walker_does_not_know_is_not_count`. It also stands as the +list that must be edited when `formula.pest` gains an alternative: the walker +refuses the new token, and this test is where the accepted set is written down +in Rust rather than only in the grammar. + ### parse_sum_with_inline_where_filter Regression: a `WHERE` inside the aggregate's parens must parse as the diff --git a/crates/improvise-formula/src/parser.rs b/crates/improvise-formula/src/parser.rs index 29497863..f5aced6b 100644 --- a/crates/improvise-formula/src/parser.rs +++ b/crates/improvise-formula/src/parser.rs @@ -154,8 +154,9 @@ fn parse_agg_func(s: &str) -> AggFunc { "AVG" => AggFunc::Avg, "MIN" => AggFunc::Min, "MAX" => AggFunc::Max, - // COUNT is the only remaining alternative. - _ => AggFunc::Count, + "COUNT" => AggFunc::Count, + // A new agg_func alternative must be spelled here (improvise-492x). + other => unreachable!("{GRAMMAR_INVARIANT}: agg_func {other}"), } } @@ -185,8 +186,9 @@ fn parse_cmp_op(s: &str) -> BinOp { "<" => BinOp::Lt, ">" => BinOp::Gt, "<=" => BinOp::Le, - // ">=" is the only remaining alternative. - _ => BinOp::Ge, + ">=" => BinOp::Ge, + // A new cmp_op alternative must be spelled here (improvise-492x). + other => unreachable!("{GRAMMAR_INVARIANT}: cmp_op {other}"), } } @@ -266,9 +268,44 @@ where #[cfg(test)] mod tests { - use super::parse_formula; + use super::{parse_agg_func, parse_cmp_op, parse_formula}; use crate::{AggFunc, BinOp, Expr}; + /// improvise-492x — see context/test-evidence/formula-and-io.md. + #[test] + fn every_grammar_aggregate_and_comparison_is_spelled_out() { + assert!(matches!(parse_agg_func("SUM"), AggFunc::Sum)); + assert!(matches!(parse_agg_func("AVG"), AggFunc::Avg)); + assert!(matches!(parse_agg_func("MIN"), AggFunc::Min)); + assert!(matches!(parse_agg_func("MAX"), AggFunc::Max)); + assert!(matches!(parse_agg_func("COUNT"), AggFunc::Count)); + for (token, want) in [ + ("=", BinOp::Eq), + ("!=", BinOp::Ne), + ("<", BinOp::Lt), + (">", BinOp::Gt), + ("<=", BinOp::Le), + (">=", BinOp::Ge), + ] { + assert_eq!(parse_cmp_op(token), want, "cmp_op {token}"); + } + } + + /// improvise-492x: a new `agg_func` alternative refuses loudly rather + /// than becoming COUNT — see context/test-evidence/formula-and-io.md. + #[test] + #[should_panic(expected = "grammar invariant violated")] + fn an_aggregate_the_walker_does_not_know_is_not_count() { + let _ = parse_agg_func("MEDIAN"); + } + + /// improvise-492x: and a new `cmp_op` alternative is not `>=`. + #[test] + #[should_panic(expected = "grammar invariant violated")] + fn a_comparison_the_walker_does_not_know_is_not_ge() { + let _ = parse_cmp_op("<>"); + } + #[test] fn parse_simple_subtraction() { let f = parse_formula("Profit = Revenue - Cost", "Foo").unwrap(); From 20030cac0d1feda1dee97c19345d9e1096169e6c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:42:19 -0700 Subject: [PATCH 04/15] fix(core): four evaluator and display bugs, and one exhaustive match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-s08t: eval_agg narrowed its partial key only where find_item_category resolved, so SUM(Typo) left the key unconstrained and aggregated every numeric measure matching the context. An unknown name now errors ref:{name} before the raw fast path. improvise-s2w3: the same fast path assumed a bare Ref names a STORED cell. A formula target has no raw cell behind it (improvise-nrs), so SUM(Double) summed nothing and answered 0 where SUM(Double + 0) folded and answered 20. core.md §13's defence of the specialisation holds — what failed is its premise — so eval_agg asks formula_for, the one home of "is this cell derived", and a derived ref falls into the fold. improvise-7iww: recompute_formulas stopped at MAX_EVAL_DEPTH and left the last iteration in formula_cache, so a circular formula read Error("circular") directly and Number(26) through the cached path — which is the DISPLAYED path, since default views put the virtual categories on Axis::None. Cells still moving when the bound runs out are now marked circular, which is the same bound evaluate_depth already enforces. improvise-vyrx: eval_bool_cached wildcarded BinOp where eval_bool lists its arms. Verified by adding a BinOp::Mod: four sites now fail the build where three did before. improvise-pikk: round_half_away scaled by 10^decimals before truncating, so format_f64(1e307, false, 2) rendered "inf" for a finite number. improvise-f86q: add_category checked MAX_CATEGORIES before contains_key, so re-adding an existing name at the limit was refused with a false message — and per gotcha 21 aborted the rest of the batch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/core.md | 47 ++++- context/test-evidence/core.md | 113 ++++++++++++ crates/improvise-core/src/format.rs | 21 ++- crates/improvise-core/src/model/types.rs | 210 +++++++++++++++++++++-- tests/bug_hunt_reproductions.rs | 3 - 5 files changed, 369 insertions(+), 25 deletions(-) diff --git a/context/code/core.md b/context/code/core.md index f5d5abf0..bda0a18b 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -94,7 +94,27 @@ target category and every `none_cat` stripped), and iterates `stem × formula target` until nothing changes, bounded by `MAX_EVAL_DEPTH = 16`. Chained formulas (`Margin = Profit / Revenue` over `Profit = Revenue - Cost`) converge naturally; circular references converge to `CellValue::Error("circular")` -rather than overflowing the stack. The stems are an `IndexSet`: dedupe is O(1) +rather than overflowing the stack. + +**A cell still moving when the bound runs out has not converged, and the cached +path had to be told so** (improvise-7iww). The direct evaluator gets this free: +`evaluate_depth` bottoms out at `depth == 0` and returns `Error("circular")`. +The fixed point had no such arm — it simply stopped iterating and left the +sixteenth iteration in `formula_cache`, which `evaluate_aggregated` then served +as a valid number. `Revenue = Revenue + 1` over a stored `Revenue = 10` read +`Error("circular")` directly and `Number(26)` through the cache, and the cached +one is the **displayed** path, because the default views put the virtual +categories on `Axis::None`. So a circular formula looked like a real calculated +value. `recompute_formulas` now records the keys that changed on each pass and, +if the last pass still changed any, writes `Error("circular")` over them. + +Note what this costs and why it is the right cost: the fixed point can only +*confirm* convergence with a pass that changes nothing, so a chain needing all +sixteen passes is now reported circular. That is the same bound the direct +evaluator already enforces — a sixteen-deep chain exhausts `depth` there too — +so the two paths agree rather than diverge, which is the property §3 exists to +protect. The anti-clamp guard is +`a_chained_formula_still_converges_after_the_circular_marker`. The stems are an `IndexSet`: dedupe is O(1) per cell and insertion order is preserved (improvise-6os replaced an O(n²) `Vec::contains`). @@ -541,7 +561,30 @@ fold routes through `eval_expr`, the cached arm is now reached **only by a bare The `Expr::Ref` fast path **stays**, and it is the same fold specialised: a record's `Ref` is that record's own cell, so summing them is summing the -matching values. It differs in exactly one place — a record with no such cell is +matching values. **Two of its premises were unstated and both were false** +(improvise-s08t, improvise-s2w3), which is worth recording as a shape: a +specialisation defended by an argument is only as good as the conditions the +argument assumed, and neither condition was being checked. + +*The name must resolve.* The narrowing was written `if let Expr::Ref(name) = +inner && let Some(cat) = find_item_category(name)`, so a name that resolved to +nothing left the partial key **unconstrained** — `SUM(Typo)` aggregated every +numeric measure matching the context and answered a plausible total. A missing +identifier now fails before the fast path is reached, with the same `ref:{name}` +the record-level evaluator uses. + +*The name must be stored.* A formula target is a `_Measure` item that nothing +wrote (improvise-nrs), so there are no raw cells behind it and the raw scan +returns nothing: `SUM(Double)` answered `0` where `SUM(Double + 0)` folded and +answered `20`. The premise of the specialisation — "a record's `Ref` is that +record's own cell" — is exactly what fails there, so the answer is to ask +whether it holds rather than to delete the path. `formula_for` is the one home +of "is this cell derived", so the check is that call and not a second +derivation; a derived ref falls into the fold, where `eval_expr`'s own `Ref` +arm routes through `evaluate_depth` and sees the formula. A target that also has +stored cells reads as the formula, because `formula_for` wins there too. + +It differs in exactly one place — a record with no such cell is *skipped* there and an **error** in the fold. That is deliberate: a bare `SUM(Revenue)` asks for the values that exist, while `SUM()` asks the expression of each member, and an expression that cannot be evaluated is an diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index eb9d8a53..3a531c2a 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -12,6 +12,87 @@ none. ## `model/types.rs` +### a_chained_formula_still_converges_after_the_circular_marker + +The anti-clamp half of improvise-7iww. `recompute_formulas` now publishes +`Error("circular")` for any cell still moving after `MAX_EVAL_DEPTH` passes, and +a guard that refuses everything would satisfy the reproduction just as well as +the fix does. This test runs a three-deep chain — `Cost`, `Profit`, `Margin` — +that converges in two passes and asserts the *numbers*, so a marker that fired +on a converging chain fails here. Inject the fault by marking every cell +evaluated in the last pass rather than only the ones that changed. + +### a_derived_measure_with_stored_cells_does_not_leak_the_raw_value + +improvise-s2w3's stated edge case. The fix routes a bare `SUM(Double)` through +the per-record fold when `Double` is a formula target, and the interesting case +is a target that *also* has stored cells: `formula_for` is the one home of "is +this cell derived", so the fold reads `20` (the formula) and not `999` (the raw +cell). Fails if the fast path is reinstated for a derived name, or if the fold +were built to prefer raw data over `formula_for`. + +### an_aggregate_over_a_derived_measure_sees_the_formula_in_both_evaluators + +improvise-s2w3, widened past the reproduction. `eval_agg` specialised +`Expr::Ref` into a raw `DataStore` lookup; a formula target has no raw cell +behind it (improvise-nrs), so `SUM(Double)` summed nothing and answered `0` +while `SUM(Double + 0)` folded per record and answered `20`. core.md §13 +defends that fast path and the defence still holds — it is the fold specialised +to a ref naming a *stored* cell. What failed is the premise, not the +specialisation, so the fix asks `formula_for` rather than deleting the path. + +The test runs `SUM`/`AVG`/`MIN`/`MAX` and both evaluators (direct `evaluate`, +and `evaluate_aggregated(OverHidden)` after `recompute_formulas`), because §3 +says a rule stated in one evaluator and not the other is a live bug. `COUNT` is +excluded deliberately: `COUNT(Double)` and `COUNT(Double + 0)` both count +members and agreed even before the fix, so including it would have added a +passing case that discriminates nothing. Inject the fault by hard-coding +`derived = false` in `eval_agg`. + +### an_unknown_name_inside_an_aggregate_errors_for_every_function + +improvise-s08t. `eval_agg` narrowed its partial key only `if let Some(cat) = +find_item_category(name)`; when the lookup failed the key was left +**unconstrained**, so `SUM(Typo)` aggregated every numeric measure matching the +context and returned a plausible wrong total. A typo is silent and looks like a +number. The test asserts the error *text* (`ref:Typo`) across all five aggregate +functions and both evaluators, so a fix that errors with some other message — or +that errors only where the raw scan happened to be empty — still fails. Inject +the fault by restoring `unwrap_or("_Measure")` in place of the `ok_or_else`. + +### both_evaluators_answer_every_comparison_the_same_way + +improvise-vyrx's *behavioural* half, and it is deliberately the smaller half. +The ticket's defect is that `eval_bool_cached` wildcarded `BinOp` where +`eval_bool` listed its arms, so a new variant would error at runtime in one +evaluator and fail to compile in the other — an asymmetry no runtime test can +see today, because both paths answer `Err("type")` for every arithmetic +operator. **The guard is the compiler**: with the wildcard removed, adding a +`BinOp::Mod` fails the build at four sites (`eval_expr`, `eval_bool`, +`eval_expr_cached`, `eval_bool_cached`) where it previously failed at three, and +that was verified by adding the variant and reading the errors. + +What this test adds is the neighbouring property the exhaustive arms do not +give: that the six comparison arms *agree in value* between the two evaluators. +It runs each operator against a true and a false right-hand side, so an arm +mistyped in one path (`Lt => lv <= rv`) fails. Both directions matter — a single +right-hand side is satisfied by an evaluator that answers a constant. + +### re_adding_an_existing_category_at_the_limit_creates_nothing_and_succeeds + +improvise-f86q. `Model::add_category` checked `MAX_CATEGORIES` *before* +`contains_key`, so at twelve regular categories re-adding an existing name was +refused with "Maximum of 12 categories reached" — a false statement, since the +category exists and nothing would be created. Per gotcha 21 `effect::AddCategory` +sets `abort_effects` on refusal, so typing an existing name into the records-mode +`+cat` column at the limit aborted the rest of the batch: the improvise-2zq +failure mode by another route. + +The fix is a hoist. The test asserts three things, and the third is what stops it +from being satisfied by deleting the limit: the returned id is the existing +category's, the category count did not move, and `add_category("C12")` — a name +that really would create something — is still refused. + ### a_hidden_dimension_list_reduces_to_one_of_two_states improvise-5jm5. `evaluate_aggregated` took `none_cats: &[String]` and consulted @@ -42,6 +123,15 @@ difference is visible. improvise-kdp named both after they had been duplicated inline in two evaluators; the test pins the contract directly rather than only through formula eval. +Its **vehicle changed** with improvise-s08t. It reached "no rows" through +`Expr::Ref("Nonexistent")` — a name that resolves to nothing — which was +precisely the bug s08t names: an unresolvable ref left the selection +unconstrained instead of erroring. So the test was standing on the defect to +demonstrate a neighbouring property. It now uses a name that **resolves** and +has no cells (`Revenue`, declared and never written), which is the emptiness +the entry is about; an unknown name is now an error and is pinned separately by +`an_unknown_name_inside_an_aggregate_errors_for_every_function`. + ### find_item_category_falls_back_to_formula_targets `add_formula` never inserts its target into `_Measure` (the item is *implied* by @@ -164,6 +254,29 @@ that the display really does round (so the fixture is discriminating) and that `Total = Price + Tax` is `10.75` and not `11`. Without the first assertion the test would pass on a fixture where rounding changes nothing. +## `format.rs` + +### a_finite_number_never_displays_as_infinity + +improvise-pikk. `round_half_away` scales by `10^decimals` before truncating, so +`n * factor` overflows to infinity for a large finite `n` and any nonzero +precision: `format_f64(1e307, false, 2)` rendered `"inf"`. The model held the +number correctly and the display invented an infinity. Gotcha 8 is why nothing +else caught it — `format_f64` is the whole display path and never feeds back +into eval, so this is a wrong pixel with no wrong number behind it to trip a +formula test. + +Reachability is low and that is not a reason to leave it: the shipped default +format is `",.0"`, whose factor is `1`, so it needs both an absurd magnitude and +an explicit `.N` format. + +The test carries its own anti-clamp assertion, because "return `n` unchanged" +satisfies the overflow case *and* every other case. `1e307 * 100` overflows and +`1e306 * 100` does not, which pins the threshold; and `format_f64(2.5, false, 0) +== "3"` pins that the rounding path is still reached at all — half-away-from- +zero, where a passthrough would hand the value to `{:.0}` and get Rust's +half-to-even `"2"`. Inject the fault by dropping the `is_finite` arm. + ## `model/cell.rs` ### matching_cells_yields_the_primary_insertion_order diff --git a/crates/improvise-core/src/format.rs b/crates/improvise-core/src/format.rs index cbc1909a..cad9232a 100644 --- a/crates/improvise-core/src/format.rs +++ b/crates/improvise-core/src/format.rs @@ -25,10 +25,16 @@ pub fn parse_number_format(fmt: &str) -> (bool, u8) { (comma, precision.parse::().unwrap_or(0)) } -/// Round half away from zero (the "normal" rounding people expect). +/// Round half away from zero (the "normal" rounding people expect). Scaling +/// can overflow a finite `n`, and display never invents an infinity (pikk). fn round_half_away(n: f64, decimals: u8) -> f64 { let factor = 10_f64.powi(decimals as i32); - (n * factor + n.signum() * 0.5).trunc() / factor + let scaled = n * factor; + match scaled.is_finite() { + true => (scaled + n.signum() * 0.5).trunc() / factor, + // Beyond the scale, n has no fractional part left to round. + false => n, + } } /// Format an f64 with optional comma grouping and decimal places. @@ -272,4 +278,15 @@ mod tests { fn format_value_none() { assert_eq!(format_value(None, true, 2), ""); } + + /// improvise-pikk — see context/test-evidence/core.md. + #[test] + fn a_finite_number_never_displays_as_infinity() { + // 1e307 * 100 overflows f64; 1e306 * 100 does not. + assert_eq!(format_f64(1e307, false, 2), format!("{:.2}", 1e307)); + assert_ne!(format_f64(1e306, false, 2), "inf"); + // The guard did not swallow the rounding path: half-away, not + // Rust's half-to-even, which would print "2". + assert_eq!(format_f64(2.5, false, 0), "3"); + } } diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index 723b07a5..a734ecf2 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -63,24 +63,36 @@ fn eval_agg( eval_at: &dyn Fn(&Expr, &CellKey) -> Result, ) -> Result { let mut partial = context.without(target_category); - if let Expr::Ref(item_name) = inner - && let Some(cat) = model.find_item_category(item_name) - { - partial = partial.with(cat, item_name.as_str()); - } + // The fast path is the fold specialised to a ref that names a STORED + // cell; where it does not, the premise fails — §13, s08t, s2w3. + let select_raw = match inner { + Expr::Ref(item_name) => { + let cat = model + .find_item_category(item_name) + .ok_or_else(|| format!("ref:{item_name}"))?; + let selected = partial.clone().with(cat, item_name.as_str()); + // `formula_for` is the one home of "is this cell derived". + let derived = model.formula_for(&selected).is_some(); + if !derived { + partial = selected; + } + !derived + } + _ => false, + }; if let Some(f) = agg_filter { partial = partial.with(&f.category, &f.item); } - let values: Vec = match inner { + let values: Vec = match select_raw { // A bare ref asks for the values that exist; anything else asks the // expression of each member record and folds the answers — §13. - Expr::Ref(_) => model + true => model .data .matching_values(&partial.0) .into_iter() .filter_map(|v| v.as_f64()) .collect(), - _ => member_records(model, &partial) + false => member_records(model, &partial) .into_iter() .map(|record| { let at = record @@ -271,6 +283,11 @@ impl Model { /// The caller registers it on every view — normally `Workbook`. pub fn add_category(&mut self, name: impl Into) -> Result { let name = name.into(); + // Idempotence first: re-adding an existing name creates nothing, so + // the limit has nothing to refuse (improvise-f86q). + if self.categories.contains_key(&name) { + return Ok(self.categories[&name].id); + } // Only regular pivot categories count against the limit (§9). let regular_count = self .categories @@ -280,9 +297,6 @@ impl Model { if regular_count >= MAX_CATEGORIES { return Err(anyhow!("Maximum of {MAX_CATEGORIES} categories reached")); } - if self.categories.contains_key(&name) { - return Ok(self.categories[&name].id); - } let id = self.next_category_id; self.next_category_id += 1; self.categories @@ -547,8 +561,9 @@ impl Model { } // Fixed-point iteration. + let mut moving: Vec = Vec::new(); for _pass in 0..Self::MAX_EVAL_DEPTH { - let mut changed = false; + moving.clear(); for (target_cat, target_name) in &formula_cats { for stem in &stems { let key = stem.clone().with(target_cat, target_name); @@ -557,17 +572,23 @@ impl Model { let new_val = self.evaluate_formula_cell(&key, none_cats); let old_val = self.formula_cache.get(&key); if old_val != new_val.as_ref() { - changed = true; + moving.push(key.clone()); if let Some(v) = new_val { self.formula_cache.insert(key, v); } } } } - if !changed { + if moving.is_empty() { break; } } + // A cell still moving after the bound never converged: publishing the + // last iteration is publishing a plausible wrong number (§3, 7iww). + for key in moving.drain(..) { + self.formula_cache + .insert(key, CellValue::Error("circular".into())); + } } /// One formula cell for the fixed-point pass: raw aggregation for a @@ -697,7 +718,9 @@ impl Model { BinOp::Gt => Ok(lv > rv), BinOp::Le => Ok(lv <= rv), BinOp::Ge => Ok(lv >= rv), - _ => Err("type".into()), + BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Pow => { + Err("type".into()) + } } } _ => Err("type".into()), @@ -791,11 +814,13 @@ mod agg_helper_tests { /// — see context/test-evidence/core.md. #[test] fn eval_agg_over_no_rows_is_zero_only_where_an_identity_exists() { - let model = Model::new("T"); - let missing = Expr::Ref("Nonexistent".to_string()); + let mut model = Model::new("T"); + model.category_mut("_Measure").unwrap().add_item("Revenue"); + // A name that RESOLVES and has no cells: an unknown one is s08t. + let empty = Expr::Ref("Revenue".to_string()); let ctx = CellKey::new(vec![]); let never = |_: &Expr, _: &CellKey| unreachable!("a bare Ref never folds per record"); - let run = |func| eval_agg(&model, &ctx, "_Measure", &func, &missing, None, &never); + let run = |func| eval_agg(&model, &ctx, "_Measure", &func, &empty, None, &never); assert_eq!(run(AggFunc::Sum), Ok(0.0)); assert_eq!(run(AggFunc::Count), Ok(0.0)); @@ -846,6 +871,155 @@ mod model_tests { ) } + /// One stored `Revenue = 10` under `Region=East`, the shape every bug + /// in this group was demonstrated on. + fn revenue_model() -> Model { + let mut m = Model::new("T"); + m.add_category("Region").unwrap(); + m.category_mut("Region").unwrap().add_item("East"); + m.category_mut("_Measure").unwrap().add_item("Revenue"); + m.set_cell( + coord(&[("Region", "East"), ("_Measure", "Revenue")]), + CellValue::Number(10.0), + ); + m + } + + /// improvise-f86q — see context/test-evidence/core.md. + #[test] + fn re_adding_an_existing_category_at_the_limit_creates_nothing_and_succeeds() { + let mut m = Model::new("T"); + for i in 0..super::MAX_CATEGORIES { + m.add_category(format!("C{i}")).unwrap(); + } + let before = m.categories.len(); + let id = m.add_category("C0").expect("C0 already exists"); + assert_eq!(id, m.categories["C0"].id); + assert_eq!(m.categories.len(), before, "nothing was created"); + // The limit still refuses a category that WOULD be created. + assert!(m.add_category("C12").is_err(), "the limit still holds"); + } + + /// improvise-s08t — see context/test-evidence/core.md. + #[test] + fn an_unknown_name_inside_an_aggregate_errors_for_every_function() { + for func in ["SUM", "AVG", "MIN", "MAX", "COUNT"] { + let mut m = revenue_model(); + m.add_formula( + crate::formula::parse_formula(&format!("Total = {func}(Typo)"), "_Measure") + .unwrap(), + ); + let key = coord(&[("Region", "East"), ("_Measure", "Total")]); + assert!( + matches!(m.evaluate(&key), Some(CellValue::Error(ref e)) if e == "ref:Typo"), + "{func}(Typo) direct: {:?}", + m.evaluate(&key) + ); + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + let cached = m.evaluate_aggregated(&key, Aggregation::OverHidden); + assert!( + matches!(cached, Some(CellValue::Error(ref e)) if e == "ref:Typo"), + "{func}(Typo) cached: {cached:?}" + ); + } + } + + /// improvise-s2w3 — see context/test-evidence/core.md. + #[test] + fn an_aggregate_over_a_derived_measure_sees_the_formula_in_both_evaluators() { + for func in ["SUM", "AVG", "MIN", "MAX"] { + let mut m = revenue_model(); + for raw in [ + "Double = Revenue * 2".to_string(), + format!("A = {func}(Double)"), + format!("B = {func}(Double + 0)"), + ] { + m.add_formula(crate::formula::parse_formula(&raw, "_Measure").unwrap()); + } + let stem = coord(&[("Region", "East")]); + let a = stem.clone().with("_Measure", "A"); + let b = stem.with("_Measure", "B"); + assert_eq!( + m.evaluate(&a), + Some(CellValue::Number(20.0)), + "{func}(Double) direct" + ); + assert_eq!(m.evaluate(&a), m.evaluate(&b), "{func} direct vs identity"); + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + assert_eq!( + m.evaluate_aggregated(&a, Aggregation::OverHidden), + m.evaluate_aggregated(&b, Aggregation::OverHidden), + "{func} cached vs identity" + ); + } + } + + /// improvise-s2w3: a formula target shadowing stored data reads as the + /// formula — see context/test-evidence/core.md. + #[test] + fn a_derived_measure_with_stored_cells_does_not_leak_the_raw_value() { + let mut m = revenue_model(); + m.category_mut("_Measure").unwrap().add_item("Double"); + m.set_cell( + coord(&[("Region", "East"), ("_Measure", "Double")]), + CellValue::Number(999.0), + ); + m.add_formula(crate::formula::parse_formula("Double = Revenue * 2", "_Measure").unwrap()); + m.add_formula(crate::formula::parse_formula("A = SUM(Double)", "_Measure").unwrap()); + let a = coord(&[("Region", "East"), ("_Measure", "A")]); + assert_eq!(m.evaluate(&a), Some(CellValue::Number(20.0))); + } + + /// improvise-7iww: the non-convergence marker does not swallow a chain + /// that converges — see context/test-evidence/core.md. + #[test] + fn a_chained_formula_still_converges_after_the_circular_marker() { + let mut m = revenue_model(); + for raw in [ + "Cost = Revenue / 2", + "Profit = Revenue - Cost", + "Margin = Profit / Revenue", + ] { + m.add_formula(crate::formula::parse_formula(raw, "_Measure").unwrap()); + } + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + let stem = coord(&[("Region", "East")]); + for (target, want) in [("Cost", 5.0), ("Profit", 5.0), ("Margin", 0.5)] { + assert_eq!( + m.evaluate_aggregated( + &stem.clone().with("_Measure", target), + Aggregation::OverHidden + ), + Some(CellValue::Number(want)), + "{target}" + ); + } + } + + /// improvise-vyrx: the two evaluators must agree arm for arm — see + /// context/test-evidence/core.md. + #[test] + fn both_evaluators_answer_every_comparison_the_same_way() { + for op in ["=", "!=", "<", ">", "<=", ">="] { + for rhs in ["10", "5"] { + let mut m = revenue_model(); + m.add_formula( + crate::formula::parse_formula( + &format!("Flag = IF(Revenue {op} {rhs}, 1, 0)"), + "_Measure", + ) + .unwrap(), + ); + let key = coord(&[("Region", "East"), ("_Measure", "Flag")]); + let direct = m.evaluate(&key); + m.recompute_formulas(&["_Index".into(), "_Dim".into()]); + let cached = m.evaluate_aggregated(&key, Aggregation::OverHidden); + assert_eq!(direct, cached, "Revenue {op} {rhs}"); + assert!(matches!(direct, Some(CellValue::Number(_))), "{op} {rhs}"); + } + } + } + /// improvise-2lh: what makes `of_name` sound as a substitute — see /// context/test-evidence/core.md. #[test] diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 968d87e3..186e4fc0 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -232,7 +232,6 @@ fn revenue_book() -> Workbook { w } #[test] -#[ignore = "improvise-s2w3: known bug reproduction"] fn sum_of_derived_measure_agrees_with_identity_expression() { use improvise_io::{formula::parse_formula, model::cell::CellKey}; let mut w = revenue_book(); @@ -249,7 +248,6 @@ fn sum_of_derived_measure_agrees_with_identity_expression() { assert_eq!(a, b); } #[test] -#[ignore = "improvise-s08t: known bug reproduction"] fn unknown_aggregate_reference_is_not_all_measures() { use improvise_io::{ formula::parse_formula, @@ -337,7 +335,6 @@ fn save_with_unfinished_record_is_readable() { } #[test] -#[ignore = "improvise-7iww: known bug reproduction"] fn circular_formula_not_a_plausible_cached_number() { use improvise_io::{ formula::parse_formula, From 5a0ba067e343e74c20ea9c50b7d71acbc85a8ce7 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:42:59 -0700 Subject: [PATCH 05/15] fix(import): a nonfinite CSV token is not zero, and a repeated header is not one column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-jc95 (P1): `serde_json::Number::from_f64` has no answer for NaN or the infinities, and `parse_csv_field` answered that `None` with `.unwrap_or(Number::from(0))`. So `NaN`, `inf`, `-inf` and `1e999` imported as a real measured zero — not a blank, not an error, a number that sums. The token now falls through to the existing string arm. improvise-5hfh: headers key a `serde_json::Map`, so `A,B,A` wrote the third column over the first under one key and the record kept two fields where the file had three. Later occurrences are renamed to a suffix nothing else in the file claims, and each rename is reported. Also the half of improvise-k8i (closed) that its fix did not reach: a row LONGER than the header had every extra field dropped by `headers.get(i)` returning None, silently. The values are still dropped — preserving them means inventing column names, which is a wizard decision — but the parser now counts the rows, names the header width and reports the widest row seen. Evidence in the following commit, keyed by test name. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-io/src/import/csv_parser.rs | 146 +++++++++++++++++-- 1 file changed, 133 insertions(+), 13 deletions(-) diff --git a/crates/improvise-io/src/import/csv_parser.rs b/crates/improvise-io/src/import/csv_parser.rs index 3171320c..0075740b 100644 --- a/crates/improvise-io/src/import/csv_parser.rs +++ b/crates/improvise-io/src/import/csv_parser.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::path::Path; use anyhow::{Context, Result}; @@ -41,23 +42,33 @@ pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec, Vec = Vec::new(); + let mut header_warnings: Vec = Vec::new(); if has_headers { - headers = reader + let raw: Vec = reader .headers() .with_context(|| "Failed to read CSV headers")? .iter() .map(|s| s.to_string()) .collect(); + (headers, header_warnings) = disambiguate_headers(raw); } // Per-column count of rows that were too short to provide a value. let mut missing_counts = vec![0usize; headers.len()]; + // Rows carrying more fields than the header names, and the widest seen. + let mut long_rows = 0usize; + let mut widest_row = 0usize; for result in reader.records() { let record = result.with_context(|| "Failed to read CSV record")?; let mut map = serde_json::Map::new(); + if has_headers && record.len() > headers.len() { + long_rows += 1; + widest_row = widest_row.max(record.len()); + } + for (i, field) in record.iter().enumerate() { let json_value: Value = parse_csv_field(field); if has_headers { @@ -81,18 +92,56 @@ pub fn parse_csv_bytes_with_warnings(bytes: &[u8]) -> Result<(Vec, Vec 0) - .map(|(header, count)| { - format!("column '{header}' missing from {count} row(s); filled with empty values") - }) - .collect(); + let mut warnings = header_warnings; + if long_rows > 0 { + warnings.push(format!( + "{long_rows} row(s) carried more fields than the {} header column(s) \ + (widest row: {widest_row} fields); the extra values were dropped", + headers.len() + )); + } + warnings.extend( + headers + .iter() + .zip(&missing_counts) + .filter(|&(_, &count)| count > 0) + .map(|(header, count)| { + format!("column '{header}' missing from {count} row(s); filled with empty values") + }), + ); Ok((records, warnings)) } +/// A header name keys the record object, so two equal names were one column and +/// the later one won silently (improvise-5hfh). Rename and report the later ones. +fn disambiguate_headers(raw: Vec) -> (Vec, Vec) { + let mut claimed: HashSet = raw.iter().cloned().collect(); + let mut seen: HashSet = HashSet::new(); + let mut headers = Vec::with_capacity(raw.len()); + let mut warnings = Vec::new(); + + for (i, name) in raw.into_iter().enumerate() { + if seen.insert(name.clone()) { + headers.push(name); + continue; + } + let renamed = (2usize..) + .map(|n| format!("{name}_{n}")) + .find(|candidate| !claimed.contains(candidate)) + .expect("an unclaimed suffix always exists"); + warnings.push(format!( + "duplicate header '{name}' at column {} renamed to '{renamed}'", + i + 1 + )); + claimed.insert(renamed.clone()); + seen.insert(renamed.clone()); + headers.push(renamed); + } + + (headers, warnings) +} + /// Parse multiple CSV files and merge into a single JSON array. /// Each record gets a "File" field set to the filename stem (e.g., "sales" from "sales.csv"). pub fn merge_csvs(paths: &[impl AsRef]) -> Result> { @@ -125,10 +174,12 @@ fn parse_csv_field(field: &str) -> Value { return Value::Number(serde_json::Number::from(num)); } - if let Ok(num) = field.parse::() { - return Value::Number( - serde_json::Number::from_f64(num).unwrap_or(serde_json::Number::from(0)), - ); + // NaN and the infinities have no JSON number. A fabricated 0 is an invented + // measurement (improvise-jc95), so the token falls through and stays text. + if let Ok(num) = field.parse::() + && let Some(number) = serde_json::Number::from_f64(num) + { + return Value::Number(number); } // Otherwise treat as string @@ -440,6 +491,75 @@ mod tests { } } + /// improvise-jc95: `Number::from_f64` has no answer for these, and the old + /// `unwrap_or(0)` invented a measurement. Evidence: the test-evidence doc. + #[test] + fn a_nonfinite_numeric_token_stays_text_rather_than_becoming_zero() { + for token in ["NaN", "inf", "-inf", "1e999", "-1e999"] { + assert!( + token.parse::().is_ok_and(|n| !n.is_finite()), + "precondition: {token} parses as a non-finite f64" + ); + assert_eq!(parse_csv_field(token), Value::String(token.to_string())); + } + } + + /// The control that makes the test above an assertion about non-finiteness + /// rather than about the float branch being dead. + #[test] + fn an_ordinary_float_is_still_a_number() { + assert_eq!( + parse_csv_field("1000.50"), + Value::Number(serde_json::Number::from_f64(1000.50).unwrap()) + ); + assert_eq!(parse_csv_field("0"), Value::Number(0.into())); + } + + /// improvise-5hfh: two equal header names are one JSON key, so the later + /// column overwrote the earlier one. Evidence: the test-evidence doc. + #[test] + fn a_repeated_header_is_renamed_rather_than_overwriting_its_twin() { + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B,A\n1,2,3\n").unwrap(); + let map = records[0].as_object().unwrap(); + assert_eq!(map["A"], Value::Number(1.into())); + assert_eq!(map["B"], Value::Number(2.into())); + assert_eq!(map["A_2"], Value::Number(3.into())); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("'A'") && warnings[0].contains("'A_2'")); + assert!(warnings[0].contains("column 3"), "{}", warnings[0]); + } + + /// The rename must not collide with a name the file already uses, or the + /// repair reintroduces the very overwrite it exists to prevent. + #[test] + fn a_rename_skips_a_suffix_the_file_already_claims() { + let (records, _) = parse_csv_bytes_with_warnings(b"A,A_2,A\n1,2,3\n").unwrap(); + let map = records[0].as_object().unwrap(); + assert_eq!(map.len(), 3, "{map:?}"); + assert_eq!(map["A_2"], Value::Number(2.into())); + assert_eq!(map["A_3"], Value::Number(3.into())); + } + + #[test] + fn distinct_headers_are_not_renamed_and_warn_nothing() { + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B\n1,2\n").unwrap(); + assert!(warnings.is_empty()); + let keys: Vec<&String> = records[0].as_object().unwrap().keys().collect(); + assert_eq!(keys, ["A", "B"]); + } + + /// improvise-k8i's remaining half: a row wider than the header had its + /// extra fields dropped by `headers.get(i)` returning `None`, silently. + #[test] + fn a_row_wider_than_the_header_is_reported() { + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B\n1,2\n1,2,3\n").unwrap(); + assert_eq!(records.len(), 2); + assert_eq!(records[1].as_object().unwrap().len(), 2); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("1 row(s)"), "{}", warnings[0]); + assert!(warnings[0].contains("widest row: 3"), "{}", warnings[0]); + } + #[test] fn parse_checking_csv_format() { // Simulates the format of /Users/edwlan/Downloads/Checking1.csv From 79b8501a664fdb0478b47f5006412cdcab60983e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:43:04 -0700 Subject: [PATCH 06/15] fix(import): infer a field kind from the values that are present, over all of them improvise-l1f5: `all_numeric` and `all_string` ran over every value including `Value::Null`, and `parse_csv_field` maps a blank CSV field to Null. One blank cell in a numeric column therefore proposed Label rather than Measure, and on a default import the numbers arrived as category item NAMES rather than as cell data. Both bindings now run over the present values; the `!is_empty()` guard is what keeps an all-null column from satisfying a vacuous `all()`, and it falls through to Label deliberately. improvise-jcfs: `detect_date_format` sampled the first ten values, so a date that can only be read day-first was never seen if it appeared later, and the column silently imported month-first with every derived _Month/_Quarter wrong. The cap is gone; `all()` short-circuits, so a format that does not fit still costs one parse. `the_detected_date_format_does_not_depend_on_the_hash_seed` expected `%m/%d/%Y` and now expects `%d/%m/%Y`. Its fixture was built so the disambiguating value sits OUTSIDE the ten-sample window, so an assertion written to prove determinism had made the neighbouring defect a specification. Nothing about hash-seed independence changed; only the constant did. Evidence in the following commit, keyed by test name. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-io/src/import/analyzer.rs | 78 +++++++++++++++++++--- 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/crates/improvise-io/src/import/analyzer.rs b/crates/improvise-io/src/import/analyzer.rs index a066534a..e688bef0 100644 --- a/crates/improvise-io/src/import/analyzer.rs +++ b/crates/improvise-io/src/import/analyzer.rs @@ -64,10 +64,10 @@ pub fn detect_date_format(samples: &[&str]) -> Option { if samples.is_empty() { return None; } - // Try up to 10 samples for efficiency - let test_samples: Vec<&str> = samples.into_iter().take(10).collect(); + // Every sample, not a window: a disambiguating value after a 10-value window + // is what decides between %m/%d/%Y and %d/%m/%Y (improvise-jcfs). for fmt in DATE_FORMATS { - if test_samples + if samples .iter() .all(|s| NaiveDate::parse_from_str(s, fmt).is_ok()) { @@ -118,8 +118,11 @@ pub fn analyze_records(records: &[Value]) -> Vec { .map(|field| { let values: Vec<&Value> = records.iter().filter_map(|r| r.get(&field)).collect(); - let all_numeric = values.iter().all(|v| v.is_number()); - let all_string = values.iter().all(|v| v.is_string()); + // Infer from the values PRESENT: one Null used to demote a numeric + // column to a Label; all-null falls through to Label (improvise-l1f5). + let present: Vec<&Value> = values.iter().copied().filter(|v| !v.is_null()).collect(); + let all_numeric = !present.is_empty() && present.iter().all(|v| v.is_number()); + let all_string = !present.is_empty() && present.iter().all(|v| v.is_string()); if all_numeric { return FieldProposal { @@ -136,7 +139,7 @@ pub fn analyze_records(records: &[Value]) -> Vec { // First-seen order, not hash order (gotcha 18, improvise-7yx): // the set deduplicates and is never iterated. let mut seen: HashSet<&str> = HashSet::new(); - let distinct_vec: Vec = values + let distinct_vec: Vec = present .iter() .filter_map(|v| v.as_str()) .filter(|s| seen.insert(s)) @@ -344,15 +347,15 @@ mod tests { } } - /// The same bug reaching the detected format, because only the first ten - /// samples are inspected. Evidence: the test-evidence doc. + /// The same bug reaching the detected format. The expected answer moved + /// with improvise-jcfs — see the test-evidence doc. #[test] fn the_detected_date_format_does_not_depend_on_the_hash_seed() { let mut vals: Vec = (1..=12).map(|d| format!("01/{d:02}/2025")).collect(); vals.push("25/12/2025".to_string()); assert!( vals.len() > 10, - "precondition: more values than detect_date_format looks at" + "precondition: more values than the old ten-sample window looked at" ); let records: Vec = vals .iter() @@ -362,7 +365,62 @@ mod tests { for _ in 0..5 { let proposals = analyze_records(&records); let when = proposals.iter().find(|p| p.field == "When").unwrap(); - assert_eq!(when.date_format, Some("%m/%d/%Y".to_string())); + assert_eq!(when.date_format, Some("%d/%m/%Y".to_string())); } } + + /// improvise-jcfs: `detect_date_format` sampled the first ten values, so a + /// disambiguating date after them could not be seen. + #[test] + fn a_disambiguating_date_after_the_eleventh_value_still_decides() { + let mut samples: Vec = (1..=11).map(|d| format!("01/{d:02}/2025")).collect(); + samples.push("25/12/2025".to_string()); + for s in samples.iter().take(11) { + assert!( + NaiveDate::parse_from_str(s, "%m/%d/%Y").is_ok() + && NaiveDate::parse_from_str(s, "%d/%m/%Y").is_ok(), + "precondition: {s} is ambiguous, so only the twelfth value can decide" + ); + } + let refs: Vec<&str> = samples.iter().map(String::as_str).collect(); + assert_eq!(detect_date_format(&refs), Some("%d/%m/%Y".to_string())); + } + + /// improvise-l1f5: `all_numeric` counted `Value::Null`, so one blank field + /// demoted an otherwise numeric column to a Label. + #[test] + fn a_blank_value_does_not_demote_a_numeric_column() { + let records: Vec = vec![ + serde_json::json!({"Revenue": 10, "Region": "East"}), + serde_json::json!({"Revenue": null, "Region": "West"}), + ]; + let proposals = analyze_records(&records); + let revenue = proposals.iter().find(|p| p.field == "Revenue").unwrap(); + assert_eq!(revenue.kind, FieldKind::Measure); + } + + /// The same defect one branch over: a blank category value made the whole + /// column a Label, and its items vanished from the proposal. + #[test] + fn a_blank_value_does_not_demote_a_string_column() { + let records: Vec = vec![ + serde_json::json!({"Region": "East"}), + serde_json::json!({"Region": null}), + serde_json::json!({"Region": "West"}), + ]; + let proposals = analyze_records(&records); + let region = proposals.iter().find(|p| p.field == "Region").unwrap(); + assert_eq!(region.kind, FieldKind::Category); + assert_eq!(region.distinct_values, ["East", "West"]); + } + + /// The all-null column has no evidence either way. Stated, because both + /// `all_numeric` and `all_string` are vacuously true over nothing. + #[test] + fn an_all_null_column_is_a_label() { + let records: Vec = vec![serde_json::json!({"Note": null, "Region": "East"})]; + let proposals = analyze_records(&records); + let note = proposals.iter().find(|p| p.field == "Note").unwrap(); + assert_eq!(note.kind, FieldKind::Label); + } } From e3ecd21c3a75b871a57899c4b467b5880fc7ea33 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:43:12 -0700 Subject: [PATCH 07/15] fix(import): repeated coordinate tuples are summed, not overwritten MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-2cw9 (P1): each record called `set_cell` directly, so two source rows with identical category values were one cell and the second overwrote the first. `Region,Revenue / East,10 / East,20` imported as 20 with no warning; the first row simply did not exist afterwards. The policy was deliberately left open by the ticket. The model cannot PRESERVE both — a cell is named by its coordinate tuple and there is no second address — so the lossless options are aggregating or inventing a row-identity category, and the latter changes the shape of every imported document. Summing is what a pivot table does to repeated records and is `measure_agg`'s own default. The warning is what keeps it from being silent, because summing is NOT right for a measure the user later sets to Avg; if that case matters the follow-up is a wizard step that asks, not a change to this default. The accumulator is a `Vec` with a `HashMap` beside it that only deduplicates: writing the cells in hash order would have been gotcha 18's bug in a third place, introduced by the fix for a different one. `set_cell` writes into an IndexMap whose order persistence depends on, so the same CSV would have produced a different .improv per process. Pinned by a five-iteration test. The collision label is `CellKey`'s own Display, not a second formatter. Evidence in the following commit, keyed by test name. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-io/src/import/wizard.rs | 141 ++++++++++++++++++++++- 1 file changed, 136 insertions(+), 5 deletions(-) diff --git a/crates/improvise-io/src/import/wizard.rs b/crates/improvise-io/src/import/wizard.rs index 3fbe3682..e942240a 100644 --- a/crates/improvise-io/src/import/wizard.rs +++ b/crates/improvise-io/src/import/wizard.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use anyhow::{Result, anyhow}; use serde_json::Value; @@ -185,6 +187,10 @@ impl ImportPipeline { // value; a record is charged to the first category it is missing. let mut skipped_per_category = vec![0usize; categories.len()]; let mut skipped_non_objects = 0usize; + // Two rows sharing every coordinate are one cell; last-row-wins lost the + // earlier one (improvise-2cw9). The map only deduplicates — gotcha 18. + let mut cells: Vec = vec![]; + let mut cell_index: HashMap = HashMap::new(); for record in &self.records { if let Value::Object(map) = record { @@ -232,8 +238,21 @@ impl ImportPipeline { if let Some(val) = map.get(&measure.field).and_then(|v| v.as_f64()) { let mut cell_coords = coords.clone(); cell_coords.push(("_Measure".to_string(), measure.field.clone())); - wb.model - .set_cell(CellKey::new(cell_coords), CellValue::Number(val)); + let key = CellKey::new(cell_coords); + match cell_index.get(&key) { + Some(&at) => { + cells[at].total += val; + cells[at].contributors += 1; + } + None => { + cell_index.insert(key.clone(), cells.len()); + cells.push(PendingCell { + key, + total: val, + contributors: 1, + }); + } + } } } } else { @@ -241,6 +260,15 @@ impl ImportPipeline { } } + let collisions: Vec<(String, usize)> = cells + .iter() + .filter(|cell| cell.contributors > 1) + .map(|cell| (cell.key.to_string(), cell.contributors)) + .collect(); + for cell in cells { + wb.model.set_cell(cell.key, CellValue::Number(cell.total)); + } + // Parse and add formulas // Formulas target the "_Measure" category by default. let formula_cat: String = "_Measure".to_string(); @@ -250,17 +278,46 @@ impl ImportPipeline { } } - let warnings = skip_warnings( + let mut warnings = skip_warnings( self.records.len(), &categories, &skipped_per_category, skipped_non_objects, ); + warnings.extend(collision_warning(&collisions)); Ok((wb, warnings)) } } +/// One imported cell mid-accumulation: the running total and how many source +/// records reached it, so a collision can be reported rather than swallowed. +struct PendingCell { + key: CellKey, + total: f64, + contributors: usize, +} + +/// Repeated coordinate tuples are summed, never dropped, and the user is told +/// which cells that happened to (improvise-2cw9). Empty when none repeated. +fn collision_warning(collisions: &[(String, usize)]) -> Option { + let (first, rest) = collisions.split_at(collisions.len().min(3)); + let mut details: Vec = first + .iter() + .map(|(label, n)| format!("{label} ({n} records)")) + .collect(); + if !rest.is_empty() { + details.push(format!("and {} more", rest.len())); + } + (!collisions.is_empty()).then(|| { + format!( + "Summed {} cell(s) built from duplicate coordinates: {}", + collisions.len(), + details.join("; ") + ) + }) +} + /// The item name a JSON scalar contributes, or `None` when it carries no value. /// `Value::Null` and a blank string are absences, not names (improvise-ych). fn scalar_name(v: &Value) -> Option { @@ -696,9 +753,9 @@ impl ImportWizard { #[cfg(test)] mod tests { - use super::ImportPipeline; + use super::{ImportPipeline, collision_warning}; use crate::import::analyzer::FieldKind; - use serde_json::json; + use serde_json::{Value, json}; #[test] fn flat_array_auto_selected() { @@ -1670,4 +1727,78 @@ mod tests { Some(100.0) ); } + + /// improvise-2cw9: two rows sharing every coordinate are one cell, and the + /// later one overwrote the earlier. Evidence: the test-evidence doc. + #[test] + fn build_model_sums_records_that_share_every_coordinate() { + use crate::model::cell::CellKey; + let raw = json!([ + {"region": "East", "revenue": 10.0}, + {"region": "East", "revenue": 20.0}, + {"region": "West", "revenue": 5.0}, + ]); + let (wb, warnings) = ImportPipeline::new(raw) + .build_model_with_warnings() + .unwrap(); + + let key = |r: &str| { + CellKey::new(vec![ + ("region".into(), r.into()), + ("_Measure".into(), "revenue".into()), + ]) + }; + assert_eq!( + wb.model.get_cell(&key("East")).and_then(|v| v.as_f64()), + Some(30.0) + ); + assert_eq!( + wb.model.get_cell(&key("West")).and_then(|v| v.as_f64()), + Some(5.0), + "a coordinate reached once must not be touched by the repair" + ); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!(warnings[0].contains("region=East"), "{}", warnings[0]); + assert!(warnings[0].contains("(2 records)"), "{}", warnings[0]); + assert!( + !warnings[0].contains("West"), + "only the colliding cell is named: {}", + warnings[0] + ); + } + + /// The cells are written in first-seen order, not in the accumulator's hash + /// order (gotcha 18) — the same defect improvise-7yx fixed one file over. + #[test] + fn build_model_writes_cells_in_first_seen_order() { + let regions = ["hotel", "alpha", "golf", "bravo", "foxtrot", "charlie"]; + let raw = Value::Array( + regions + .iter() + .map(|r| json!({"region": r, "revenue": 1.0})) + .collect(), + ); + for _ in 0..5 { + let wb = ImportPipeline::new(raw.clone()).build_model().unwrap(); + let order: Vec = wb + .model + .data + .iter_cells() + .filter_map(|(k, _)| k.get("region").map(str::to_string)) + .collect(); + assert_eq!(order, regions); + } + } + + /// `collision_warning` names at most three cells and counts the rest, so a + /// wide collision cannot render an unreadable status line. + #[test] + fn a_long_collision_list_is_truncated_with_a_count() { + let collisions: Vec<(String, usize)> = (0..5).map(|i| (format!("c={i}"), 2)).collect(); + let warning = collision_warning(&collisions).unwrap(); + assert!(warning.contains("Summed 5 cell(s)"), "{warning}"); + assert!(warning.contains("and 2 more"), "{warning}"); + assert!(!warning.contains("c=3"), "{warning}"); + assert_eq!(collision_warning(&[]), None); + } } From dcc0addfb4ccdd3433d4a0f036d8bdc1d77579e7 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:43:19 -0700 Subject: [PATCH 08/15] test(import): un-ignore five reproductions and record what each is evidence of MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The five reproductions for improvise-jc95, 5hfh, l1f5, 2cw9 and the long-row half of k8i now run. Removing `#[ignore]` is part of the fix: an ignored reproduction is an open bug, and the module doc now says so rather than claiming the file is report-only. Three of the five asserted warning PROSE against a module that emitted no warnings at all, so they specified wording with no convention to match — and `"overwritten"` does not contain `"overwrite"`, so the most natural wording would have failed its own assertion. Each now asserts the OBSERVABLE outcome first and the prose only as a companion: - duplicate_csv_headers_not_silent: three fields with values 1, 2, 3 in order. - repeated_import_coordinates_not_silently_overwritten: exactly one cell at 30.0. - long_csv_row_is_not_silently_dropped: no row discarded, plus a warning that counts the affected rows and names the widest, because the values here really are still dropped and there is no observable outcome to assert instead. Every fix was verified by injecting its own fault and watching the test fail: the `unwrap_or(0)` restored, `disambiguate_headers` short-circuited, the long-row counter disabled, both `all_` bindings restored to the full slice, `take(10)` restored, the accumulate arm changed back to last-wins, and the cell write order sorted. Each named test failed; each control did not. Evidence in context/test-evidence/formula-and-io.md, keyed by test name, including the correction to `the_detected_date_format_does_not_depend_on_the_hash_seed` — its fixture had made the ten-sample truncation a specification. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/formula-and-io.md | 190 +++++++++++++++++++++++- tests/bug_hunt_reproductions.rs | 93 ++++++------ 2 files changed, 236 insertions(+), 47 deletions(-) diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 4fe396c3..d3f37772 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -520,9 +520,65 @@ whenever the eleventh-and-later values are the ones that rule a format out. Twelve values are ambiguous and one (`25/12/2025`) is day-first only. In hash order it landed inside the first ten about half the time, so the column imported as `%d/%m/%Y` on some runs and `%m/%d/%Y` on others — every date in it off by the -difference. In first-seen order it is last, deliberately: the answer is -`%m/%d/%Y` **stably**, which is the property asserted. The ten-sample truncation -itself is unchanged and is still a real limit. +difference. In first-seen order it is last, deliberately, and the answer is +**stable**, which is the property asserted. + +**The expected value moved with improvise-jcfs, and the reason is worth keeping.** +This entry used to say the answer was `%m/%d/%Y` and that the ten-sample +truncation "is still a real limit". That is exactly backwards as a test: the +fixture was built so the disambiguating value sits *outside* the window, so the +assertion was pinning the truncation as correct behaviour — a test written to +prove determinism had, as a side effect, made the neighbouring defect a +specification. With the window gone the twelfth value is seen and the answer is +`%d/%m/%Y`. Nothing about hash-seed independence changed; only the constant did. +The general form: **a fixture chosen to exercise bug A can silently enshrine +bug B**, and the tell is that the expected value is surprising for a reason the +entry does not explain. + +### a_disambiguating_date_after_the_eleventh_value_still_decides + +improvise-jcfs. `detect_date_format` took `samples.into_iter().take(10)`, so a +date that can only be read day-first was never examined if it appeared after the +tenth *distinct* value. `%m/%d/%Y` precedes `%d/%m/%Y` in `DATE_FORMATS`, so the +column silently imported month-first and every derived `_Month`/`_Quarter` value +was wrong. The cap is removed; `all()` short-circuits on the first failure, so a +format that does not fit costs one parse either way. + +**Fault injected:** `take(10)` restored. The test fails, as does +`the_detected_date_format_does_not_depend_on_the_hash_seed`. The eleven leading +values are asserted to parse under *both* readings first, so this is a statement +about a value outside the old window deciding, not about one format merely +fitting — the same precondition `an_ambiguous_date_is_read_american_first` uses. + +### a_blank_value_does_not_demote_a_numeric_column + +improvise-l1f5. `all_numeric` was `values.iter().all(|v| v.is_number())` over +*every* value including `Value::Null`, and `parse_csv_field` maps a blank CSV +field to `Null`. So one blank cell in a thousand-row Revenue column proposed +`Label` instead of `Measure`, and on a default import the numbers then arrived as +**category item names** rather than as cell data. Inference now runs over the +values that are present. + +**Fault injected:** both `all_` bindings restored to the full `values` slice. +This test and its string sibling fail; `an_all_null_column_is_a_label` does not, +which is correct — it is the control. + +### a_blank_value_does_not_demote_a_string_column + +The same defect one branch over, and the reason the fix touched two lines rather +than one. `all_string` had the identical shape, so a Region column with one blank +became a `Label`: not merely a wrong proposal, but a column whose +`distinct_values` were then computed over a slice the branch had already +rejected. Fixing only the numeric half would have left a bug with the same cause, +the same shape and no entry here. + +### an_all_null_column_is_a_label + +Stated because `all()` is vacuously true over an empty slice: with the `Null`s +filtered out, a column that is entirely blank would satisfy `all_numeric` *and* +`all_string` and be proposed `Measure` on the strength of no evidence at all. +The `!present.is_empty()` guard is what makes it fall through to `Label`, and +this test is the only thing that names that as a decision. --- @@ -585,6 +641,84 @@ The degenerate shape for everything downstream that splits a row into coordinates and a measure: one field, present, and **not** mistaken for a short row. +### a_nonfinite_numeric_token_stays_text_rather_than_becoming_zero + +improvise-jc95, rated P1. `serde_json::Number::from_f64` returns `None` for NaN +and the infinities — JSON has no spelling for them — and `parse_csv_field` +answered that `None` with `.unwrap_or(serde_json::Number::from(0))`. So `NaN`, +`inf`, `-inf` and any overflowing literal such as `1e999` imported as a **real +measured zero**: not a blank, not an error, a number that sums and averages and +is indistinguishable from data. The token now falls through to the existing +string arm, which is what an unparseable field already does. + +**Fault injected:** the `unwrap_or` restored. The test fails on the first token. +The precondition inside the loop asserts each token really does parse as a +non-finite `f64`, so a future `f64` parser that rejected `inf` outright would +fail the precondition rather than pass the test vacuously. + +The consequence downstream is deliberate and worth stating: a column carrying +`NaN` is no longer all-numeric, so the analyzer proposes it as a Category or +Label rather than a Measure. That is the honest answer — the column is not +numeric — and it is visible in the wizard, where a fabricated zero was not. + +### an_ordinary_float_is_still_a_number + +The control for the entry above. Guarding non-finiteness by deleting the float +branch entirely would also pass that test, so this one pins that `1000.50` is +still a `Number` and that `0` — the value the bug invented — is still reachable +when the file really says so. + +### a_repeated_header_is_renamed_rather_than_overwriting_its_twin + +improvise-5hfh. Headers key a `serde_json::Map`, so `A,B,A` inserted the third +column's value over the first column's under one key: the record kept two fields +where the file had three, with no error and no warning, and the analyzer then +saw a column that did not exist in the source. Later occurrences are now renamed +`A_2`, `A_3`, … and each rename is reported. + +**Fault injected:** `disambiguate_headers` short-circuited to `(raw, vec![])`. +The test fails on `map["A_2"]` being absent, and so does +`duplicate_csv_headers_not_silent`. + +The warning names the original, the 1-based column and the new name, because a +user's next question is which of two identically-named columns became `A_2`. +Note the enabled reproduction asserts the **observable** outcome first — three +fields, values `1`, `2`, `3` in order — and only then that a warning mentions +it; its original form asserted prose alone, against a module that emitted no +warnings at all and so had no wording convention to match. + +### a_rename_skips_a_suffix_the_file_already_claims + +The repair's own failure mode. `A,A_2,A` renaming the third column to `A_2` +would collide with the real second column and reintroduce exactly the overwrite +the fix exists to prevent — a bug strictly worse than the original, because the +warning would say the data was preserved. The candidate suffix is checked +against every name the file claims, so the third column becomes `A_3`. + +### distinct_headers_are_not_renamed_and_warn_nothing + +The negative arm: an ordinary file must not acquire suffixes or warnings. +Without it, renaming every column would satisfy the two tests above. + +### a_row_wider_than_the_header_is_reported + +The half of improvise-k8i that was not closed with it. `.flexible(true)` also +permits a row *longer* than the header, and the insert loop guards with +`headers.get(i)`, which returns `None` — so every field past the header width was +dropped with no error, no warning and no record of it. The short-row half got +padding and a per-column warning in f0b9227; the long-row half got nothing, +because the ticket's title said "row widths mismatch" and only one direction was +reproduced. + +The values are **still dropped** — preserving them means inventing column names, +which changes the imported model's shape and is a wizard decision rather than a +parser one. What changed is that it is no longer silent: the warning counts the +affected rows, names the header width and gives the widest row seen, which is +enough for a user to tell a stray delimiter from a genuinely different file. + +**Fault injected:** the `record.len() > headers.len()` counter disabled. Both +this test and `long_csv_row_is_not_silently_dropped` fail. + --- ## `improvise-io/src/import/wizard.rs` @@ -648,6 +782,56 @@ the CSV parser already turns a blank field into `Value::Null`, so the two inputs must agree. An item with an empty name is unnameable in the grid, so it is treated as missing rather than created. +### build_model_sums_records_that_share_every_coordinate + +improvise-2cw9, rated P1. Each record called `set_cell` directly, so two source +rows with identical category values — `East,10` then `East,20` — were **one** +cell and the second overwrote the first. A sum over the import read 20 where the +file said 30, with no error and no warning, and nothing downstream could tell +that a row had existed. + +**The policy is a decision, and the ticket deliberately left it open**: preserve, +aggregate, or reject. The model cannot preserve — a cell is named by its +coordinate tuple and there is no second address for a second record, so the only +lossless options are aggregating or inventing a row-identity category, and the +latter changes the shape of every imported document. Summing is what a pivot +table does to repeated records and is `measure_agg`'s own default, so it is the +answer here; the warning is what keeps it from being silent, because it is *not* +right for a measure the user will later set to `Avg` (that value is now +pre-summed). If that case matters, the follow-up is a wizard step that asks, not +a change to this default. + +**Fault injected:** the accumulate arm changed to `total = val; contributors = 1` +— last-row-wins again. This test fails on `Some(30.0)`, as does +`repeated_import_coordinates_not_silently_overwritten`. + +The `West` cell is asserted alongside, because a repair that summed *every* cell +into one, or that dropped the singleton path, would otherwise pass. The warning +is asserted **not** to name `West`, so the collision report cannot be a list of +every cell in the document. + +### build_model_writes_cells_in_first_seen_order + +Not a reported bug — the hazard the 2cw9 fix introduced. Accumulating means a +map keyed by `CellKey`, and gotcha 18 is exactly this: `Model::set_cell` writes +into an `IndexMap` whose order persistence depends on, so writing the cells in a +`HashMap`'s iteration order would have given the same CSV a different `.improv` +file per process. That is improvise-7yx's bug, in a third place, introduced by +the fix for a different one. The `HashMap` here only deduplicates; the order is +carried by a `Vec` beside it. + +**Fault injected:** `cells.sort_by_key(|c| c.key.to_string())` before the write +loop — a *deterministic* reordering, which is the weaker fault and still caught. +Five iterations, because each `HashMap` gets its own `RandomState` and one run +of a hash order can happen to be right. + +### a_long_collision_list_is_truncated_with_a_count + +The warning is rendered into a one-line status bar. A file with ten thousand +duplicate coordinates would otherwise compose a warning naming every one. Three +cells are named and the rest counted; the empty case returns `None` rather than +a warning that says nothing happened. + ### build_model_warns_about_records_skipped_for_missing_category improvise-5j9: skipping a record for a missing category value used to be silent, diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 968d87e3..5511ccd6 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -1,5 +1,5 @@ -//! Report-only bug-hunt reproductions: assert the intended behavior, without fixes. -//! Run with `cargo test --test bug_hunt_reproductions -- --ignored --nocapture`. +//! Bug-hunt reproductions. An `#[ignore]`d one is still open; un-ignoring it is +//! part of the fix. Open ones: `cargo test --test bug_hunt_reproductions -- --ignored`. use improvise_io::import::csv_parser::parse_csv_bytes_with_warnings; use improvise_io::{ @@ -46,30 +46,37 @@ fn deleted_default_stays_deleted() { ); } #[test] -#[ignore = "improvise-jc95: known bug reproduction"] fn csv_nonfinite_is_not_zero() { for token in ["NaN", "inf", "-inf", "1e999"] { let (r, _) = parse_csv_bytes_with_warnings(format!("Value\n{token}\n").as_bytes()).unwrap(); assert_ne!(r[0]["Value"].as_f64(), Some(0.0), "{token} became zero"); + assert_eq!( + r[0]["Value"].as_str(), + Some(token), + "{token} must survive as its own text, not as some other number" + ); } } #[test] -#[ignore = "improvise-5hfh: known bug reproduction"] fn duplicate_csv_headers_not_silent() { - let result = parse_csv_bytes_with_warnings(b"A,A\n1,2\n"); + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,A\n1,2\n").unwrap(); + let fields = records[0].as_object().unwrap(); + assert_eq!( + fields.len(), + 2, + "both columns must survive as separate fields: {fields:?}" + ); + let values: Vec> = fields.values().map(|v| v.as_f64()).collect(); + assert_eq!(values, [Some(1.0), Some(2.0)]); assert!( - match result { - Err(error) => error.to_string().to_lowercase().contains("duplicate"), - Ok((_, warnings)) => warnings.iter().any(|warning| { - warning.to_lowercase().contains("duplicate") && warning.contains("A") - }), - }, - "duplicate headers silently overwrite a field" + warnings + .iter() + .any(|warning| warning.to_lowercase().contains("duplicate") && warning.contains("A")), + "the rename must be reported: {warnings:?}" ); } #[test] -#[ignore = "improvise-l1f5: known bug reproduction"] fn blank_measure_still_infers_numeric() { use improvise_io::import::{analyzer::FieldKind, wizard::ImportPipeline}; let (records, _) = parse_csv_bytes_with_warnings(b"Region,Revenue\nEast,10\nWest,\n").unwrap(); @@ -78,7 +85,6 @@ fn blank_measure_still_infers_numeric() { assert_eq!(proposal.kind, FieldKind::Measure); } #[test] -#[ignore = "improvise-2cw9: known bug reproduction"] fn repeated_import_coordinates_not_silently_overwritten() { use improvise_io::import::wizard::ImportPipeline; let (records, _) = @@ -86,24 +92,23 @@ fn repeated_import_coordinates_not_silently_overwritten() { let (wb, warnings) = ImportPipeline::new(serde_json::Value::Array(records)) .build_model_with_warnings() .unwrap(); - let total: f64 = wb + let values: Vec = wb .model .data .iter_cells() .filter_map(|(_, v)| v.as_f64()) - .sum(); - assert!( - total == 30.0 - || warnings.iter().any(|warning| { - let warning = warning.to_lowercase(); - (warning.contains("duplicate") - || warning.contains("overwrit") - || warning.contains("overwrot")) - && warning.contains("east") - && warning.contains("revenue") - }), - "total {total}, no warning {warnings:?}" + .collect(); + assert_eq!( + values, + [30.0], + "the two records are one cell and neither may be dropped" ); + let warning = warnings + .iter() + .find(|w| w.to_lowercase().contains("duplicate")) + .unwrap_or_else(|| panic!("summing is a decision and is reported: {warnings:?}")); + assert!(warning.contains("Region=East"), "{warning}"); + assert!(warning.contains("_Measure=Revenue"), "{warning}"); } #[test] #[ignore = "improvise-55v2: known bug reproduction"] @@ -168,25 +173,25 @@ fn label_stays_off_axes_in_new_view() { assert_eq!(w.create_view("Other").axis_of("Description"), Axis::None); } #[test] -#[ignore = "improvise-k8i: known bug reproduction"] fn long_csv_row_is_not_silently_dropped() { - let r = parse_csv_bytes_with_warnings(b"A,B\n1,2,3\n"); + let (records, warnings) = parse_csv_bytes_with_warnings(b"A,B\n1,2,3\n4,5\n1,2,3,4\n").unwrap(); + assert_eq!(records.len(), 3, "no row is discarded outright"); + let warning = warnings + .iter() + .find(|w| w.contains("more fields")) + .unwrap_or_else(|| panic!("extra values dropped silently: {warnings:?}")); assert!( - match r { - Err(error) => { - let message = error.to_string().to_lowercase(); - message.contains("3") - && message.contains("2") - && (message.contains("field") || message.contains("column")) - } - Ok((_, warnings)) => warnings.iter().any(|warning| { - let warning = warning.to_lowercase(); - warning.contains("3") - && warning.contains("2") - && (warning.contains("field") || warning.contains("column")) - }), - }, - "extra value 3 dropped silently" + warning.contains("2 row(s)") && warning.contains("2 header"), + "the warning counts the affected rows and the header width: {warning}" + ); + assert!( + warning.contains("widest row: 4"), + "the warning says how much was dropped: {warning}" + ); + let (_, clean) = parse_csv_bytes_with_warnings(b"A,B\n1,2\n").unwrap(); + assert!( + clean.is_empty(), + "control: a full row is not long: {clean:?}" ); } // Passing control: embedded carriage returns already survive persistence. From 424dfb98016afaec7fdda7ff7c93f9114a69d9b1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:44:17 -0700 Subject: [PATCH 09/15] =?UTF-8?q?feat(adr-001):=20deliver=20clause=204=20?= =?UTF-8?q?=E2=80=94=20a=20frame=20applies=20whole=20or=20not=20at=20all?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-yqpd. `Host::commands` snapshots the document and every session mirror before a frame and restores both if any command in it fails, so a chain whose later mutation cannot run leaves the document exactly as its stated basis described. Two failure signals, because a command can fail two ways. An effect that sets `abort_effects` is `Rejection::MutationRefused`. A command whose record no longer resolves is `Rejection::ChainBroken` — and that is the half the ticket costed wrongly. It argued that interleaving the record check with the applies would close clause 4's silence but would sell the legitimate double-write. That is true of the record tier's COUNTER half and not of its STRUCTURAL half: writing a record twice cannot make it stop resolving. So the tier splits — "does it resolve" is asked again before every mutation, "is its counter what you claimed" stays pre-frame — and clause 4 gets its loud failure at no cost to clause 2. `a_later_write_in_a_chain_still_answers_for_its_own_record` arm 3 still passes, which is the guard that the trade was not taken by accident. The snapshot primitive is `App::snapshot_document`/`restore_document`, so a future RunScript opens the same transaction; only the trigger is at the frame. It is deliberately NOT taken inside `apply_effects`, which runs on every keypress — an unconditional Workbook clone there would put a full document copy on the held-key path. `a_frame_whose_later_mutation_fails_is_not_yet_undone` is inverted to `..._is_undone_whole`; all three assertions flip and a fourth is added that the record counter goes back too. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 100 +++++++++++- crates/improvise-command/src/ui/app.rs | 34 ++++ crates/improvise-command/src/ui/host.rs | 173 +++++++++++++++++++-- crates/improvise-command/src/ui/project.rs | 4 +- crates/improvise-command/src/ui/reduce.rs | 35 ++++- crates/improvise-protocol/src/basis.rs | 14 +- crates/improvise-wasm-client/src/socket.rs | 9 ++ 7 files changed, 340 insertions(+), 29 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index a559e63a..9ca56f41 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2205,11 +2205,60 @@ exists would pass against a host that applied the first command and dropped the second — which is exactly what the previous rule did, loudly, and what the pre-frame check did, silently. -### a_frame_whose_later_mutation_fails_is_not_yet_undone +### a_frame_whose_later_mutation_fails_is_undone_whole -**Characterization of the open hole (clause 4), not an endorsement.** +**ADR 001 clause 4, delivered (improvise-yqpd). This entry keeps the hole it +characterized, because the inversion is only legible against it.** -The contract says a frame is all-or-nothing. It is not yet: `App::apply_effects` +The three assertions flipped. The removal is **undone**, the cell reads its +**pre-frame** value — `Some(Number(1.0))`, which is neither `5.0` (the frame's own +write landing) nor `None` (the removal surviving), and that middle value is the +whole difference between a rollback and an abort — and the frame is **refused out +loud** as `Rejection::ChainBroken`. A fourth assertion was added that the previous +version could not have: the **record counter** goes back too, or the client's next +batch is refused for a write that never happened. + +**The mechanism, and where it deviates from the ticket.** The ticket asked for +snapshot-and-restore at `App::apply_effects`. What landed is `App::snapshot_document` +/ `App::restore_document` — the workbook, its dirty marker and the two counted tiers, +the record tier riding inside the workbook's `DataStore` — driven from +`Host::commands`, which takes the snapshot before the frame and restores it on +failure. It is **not** taken inside `apply_effects` itself, and the reason is cost: +`apply_effects` runs on every keypress, and an unconditional `Workbook` clone there +would put a full document copy on the held-key path that gotcha 24's `mcq4` work +exists to keep cheap. The primitive is on `App`, so a future `RunScript` opens the +same transaction; only the *trigger* is at the frame. + +**What counts as a failure, and the split the ticket did not name.** Two things: +an effect that sets `abort_effects` (`Rejection::MutationRefused`), and a command +whose record no longer resolves (`Rejection::ChainBroken`). The second is the +interesting one. The ticket says interleaving the record check with the applies +would close clause 4's silence for free but would sell the legitimate double-write — +a frame writing one record twice works only because the second write is compared to +the record's *pre-frame* counter. That is true of the **counter** half of the record +tier and **not** of the **structural** half: writing a record twice cannot make it +stop resolving. So the record tier splits — *does it resolve* is asked again before +every mutation (`reduce::broken_chain`), *is its counter what you claimed* stays +pre-frame — and clause 4 gets its loud failure without paying the price the ticket +costed. `a_later_write_in_a_chain_still_answers_for_its_own_record` arm 3 still +passes, which is the guard that the trade was not taken by accident. + +**Injected faults, each watched to fail.** Making `undo_frame` restore nothing +fails this test alone. Making `broken_chain` return `None` unconditionally fails +this test and the mirror test. What did **not** discriminate: removing the +frame-creates-it exemption from `broken_chain` fails nothing, because +`App::resolve_record` falls back to `record_in_layout`, so a record whose item an +earlier command in the frame added already resolves through the layout before any +cell exists. The exemption is therefore correctness-preserving and **not currently +observable**; it is written as the *same* predicate the pre-frame check exempts by +(`will_be_created_by_the_frame`) rather than a second copy, so the two cannot drift +even though no test holds them together. + +--- + +The hole this replaced, kept because the inversion is only legible against it: + +The contract says a frame is all-or-nothing. It was not: `App::apply_effects` aborts without undoing (gotcha 21 — an abort is not a rollback), so a chain whose later mutation fails leaves the earlier one standing. `[RemoveItem("North"), SetCell(the North record)]` is the shape: the removal lands, @@ -2231,12 +2280,49 @@ It is unreachable from any client here — the only producer of a multi-command is `keys::commit`, which emits one mutation among view commands — which is what makes it safe to document rather than rush. -**This test must be inverted when clause 4 lands**: all three assertions flip, -because then nothing at all happens. - The frame claims North at its current counter, so it passes the check and fails at *apply* time — which is the only way to reach the hole. A frame refused by the check -never applies anything and would be testing clause 2 instead. +never applies anything and would be testing clause 2 instead. That fixture note +survives the inversion unchanged: it is still the only route to the arm. + +### a_failed_frame_puts_the_askers_mirror_back_on_the_view_it_was_on + +Clause 4's least obvious cost, in the ticket's own words: **rolling back the `App` +is not enough.** `Host::command` projects per command and `apply_switch` / +`adopt_deleted_views` / `adopt_cursor_epoch` move mirrors inline, so by the time a +later command fails, every session's mirror has already moved. `Host::commands` +therefore snapshots the whole `Sessions` beside the document and restores both. + +**The first version of this test was vacuous, and the injection is what showed it.** +It seated a bystander on a second view and asserted the bystander was told nothing +and its mirror was unmoved — both of which hold when `undo_frame` restores *nothing +at all*, because the frame's projections are discarded from `out` either way and +nothing ever moved that bystander. It answered *did the frame return early* where it +meant *was the mirror put back*. + +The rewrite aims at the one mirror move a failed frame really makes and that +**nothing in the stream can ever correct**: the asker's own `SwitchView`. An +accepted switch is relayed to nobody (gotcha 36), so a mirror left on the switched-to +view diverges from a client that was told the frame was refused, permanently and +silently. The fixture asserts up front that the mirror starts on `Default` — the +direction the default cannot supply — so the closing assertion is not satisfied by a +mirror nothing ever wrote. Removing `self.sessions = mirrors` while keeping the +document restore fails this test and only this test. + +### a_frame_that_creates_the_record_it_writes_is_not_a_broken_chain + +The other direction: clause 4 must not fire on `[AddItem("West"), SetCell(West)]`, +which is the exemption ADR 001 states as a **property** — a record that neither +resolves nor is claimed is one the frame is creating. + +**Be clear about what this does and does not catch.** It catches `broken_chain` +being asked against the *pre-frame* document instead of the running one, which is +the plausible implementation error (passing the snapshot where the live `App` +belongs) and would refuse every data-dependent chain. It does **not** catch the +exemption filter being dropped, for the reason recorded above: `resolve_record` +falls back to the layout, so `West` already resolves once `AddItem` has run. A +fixture that discriminated the filter would need a created record that never enters +the layout; none exists in this workspace and one was not invented for it. #### Helper — `frame_basis` diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b6dd980a..cf381065 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -342,6 +342,16 @@ pub struct App { pub storage: Box, } +/// The document a transaction was opened against: the workbook, its dirty marker +/// and the two counted tiers. What is NOT here: `context/code/command.md`. +#[derive(Debug, Clone)] +pub struct DocumentSnapshot { + workbook: Workbook, + dirty: bool, + document_generation: u64, + layout_version: u64, +} + impl App { /// Build an app backed by the real filesystem. pub fn new(workbook: Workbook, file_path: Option) -> Self { @@ -432,6 +442,30 @@ impl App { self.batch_aborted } + /// The document and its basis tiers, kept so a failed frame can put them + /// back (ADR 001 clause 4). See `context/code/command.md`. + pub fn snapshot_document(&self) -> DocumentSnapshot { + DocumentSnapshot { + workbook: self.model_state.workbook.clone(), + dirty: self.model_state.dirty, + document_generation: self.document_generation, + layout_version: self.layout_version, + } + } + + /// Put the document back as `snapshot` described it. **The document and its + /// tiers only** — `view_state` is deliberately untouched (see the doc). + pub fn restore_document(&mut self, snapshot: DocumentSnapshot) { + self.model_state.workbook = snapshot.workbook; + self.model_state.dirty = snapshot.dirty; + self.document_generation = snapshot.document_generation; + self.layout_version = snapshot.layout_version; + self.last_reshaped_views.clear(); + self.last_reframed_views.clear(); + self.last_reframed_elsewhere.clear(); + self.rebuild_layout(); + } + /// How many times this app's document has been replaced. A session that /// was seeded at an earlier generation is holding another document. pub fn document_generation(&self) -> u64 { diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 51ff2e03..c46d4d17 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -4,7 +4,7 @@ use crate::ui::app::App; use crate::ui::effect; use crate::ui::project::{Applied, Broadcast, Dirty, SessionId, Sessions}; -use crate::ui::reduce::{Checked, CheckedBatch}; +use crate::ui::reduce::{self, Checked, CheckedBatch}; use improvise_protocol::{Command, Rejection}; use improvise_protocol::{ClientFrame, ServerFrame}; @@ -139,13 +139,44 @@ impl Host { }]; } }; + // Clause 4: the document AND every mirror go back if any of it fails, + // so nothing a failed frame did is observable — `context/code/command.md`. + let undo = self.app.snapshot_document(); + let mirrors = self.sessions.clone(); + let basis = batch.basis().cloned(); let mut out = Vec::new(); for cmd in batch { + let broke = basis + .as_ref() + .and_then(|basis| reduce::broken_chain(&self.app, basis, cmd.command())); + if let Some(reason) = broke { + return self.undo_frame(id, undo, mirrors, reason); + } out.extend(self.command(id, cmd)); + if self.app.last_batch_aborted() { + return self.undo_frame(id, undo, mirrors, Rejection::MutationRefused); + } } out } + /// Put the document and every mirror back, and tell the asker alone: the + /// frame did nothing, so nobody else is owed anything (ADR 001 clause 4). + fn undo_frame( + &mut self, + id: SessionId, + undo: crate::ui::app::DocumentSnapshot, + mirrors: Sessions, + reason: Rejection, + ) -> Vec { + self.app.restore_document(undo); + self.sessions = mirrors; + vec![Addressed { + to: id, + msg: ServerFrame::Rejected(reason), + }] + } + /// The one door (gotcha 36); the match that follows is the mechanism (gotcha 39): /// asking for the `Dirty` means naming the case where a `Snapshot` is owed. fn command(&mut self, id: SessionId, cmd: Checked<'_>) -> Vec { @@ -781,10 +812,10 @@ mod tests { ); } - /// **Characterization of the open hole (clause 4), not an endorsement.** Invert - /// all three assertions when clause 4 lands — see the evidence. + /// ADR 001 clause 4. The inversion of the hole this characterized until + /// improvise-yqpd — see `context/test-evidence/command.md`. #[test] - fn a_frame_whose_later_mutation_fails_is_not_yet_undone() { + fn a_frame_whose_later_mutation_fails_is_undone_whole() { use improvise_protocol::ModelCommand; let mut host = host(); @@ -798,6 +829,7 @@ mod tests { ); // The frame claims North at its current counter, so it passes the check and // fails at *apply* time — the only way to reach the hole. + let before_version = host.app().record_version_of(addressed("North").record); let basis = frame_basis(&host, &["North"]); let answer = host.handle( asker, @@ -820,14 +852,14 @@ mod tests { ); assert!( - !host - .app() + host.app() .model_state .workbook .model .category("Region") .is_some_and(|c| c.ordered_item_names().contains(&"North")), - "TODAY: the first command lands. Under clause 4 it must be undone." + "the first command must be UNDONE: the frame failed, so the base \ + it was composed against is what the document must read as" ); assert_eq!( host.app() @@ -835,19 +867,130 @@ mod tests { .workbook .model .get_cell(&cell("North")), - None, - "TODAY: the second mutation does NOT land — its surrogate no \ - longer resolves. **This is the assertion that makes the test \ - about the hole**: without it the test passes against a frame \ - that succeeded completely, which is how it was found to be \ - vacuous." + Some(&CellValue::Number(1.0)), + "and the cell reads its PRE-FRAME value — not 5.0 (the frame's \ + own write landing) and not None (the removal surviving). This is \ + the assertion that separates a rollback from an abort." ); + assert_eq!( + host.app().record_version_of(addressed("North").record), + before_version, + "the record tier goes back too, or the client's next batch is \ + refused for a write that never happened" + ); + assert!( + answer + .iter() + .any(|a| matches!(a.msg, ServerFrame::Rejected(Rejection::ChainBroken(_)))), + "and the frame is refused OUT LOUD, naming the chain it broke: {answer:?}" + ); + } + + /// The rollback reaches the MIRRORS, not only the document — the cost the + /// ticket called its least obvious. See `context/test-evidence/command.md`. + #[test] + fn a_failed_frame_puts_the_askers_mirror_back_on_the_view_it_was_on() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + let (asker, _) = host.connect(10, 10); + host.app_mut() + .model_state + .workbook + .views + .insert("Second".to_string(), crate::view::View::new("Second")); + host.model( + asker, + ModelCommand::SetCell { + cell: addressed("North"), + value: CellValue::Number(1.0), + }, + ); + let was = host + .sessions + .get_mut(asker) + .map(|s| s.view().active_view.clone()); + assert_eq!( + was, + Some("Default".to_string()), + "the fixture must start somewhere the switch can move it FROM, or \ + the assertion below holds for a mirror nothing ever wrote" + ); + // A switch moves ONLY the asker's mirror and is relayed to nobody + // (gotcha 36), so nothing in the stream can ever correct a stale one. + let basis = frame_basis(&host, &["North"]); + host.handle( + asker, + ClientFrame::Commands(vec![ + Command::SwitchView("Second".into()), + Command::Model { + basis: basis.clone(), + cmd: ModelCommand::RemoveItem { + category: "Region".into(), + item: "North".into(), + }, + }, + Command::Model { + basis, + cmd: ModelCommand::SetCell { + cell: addressed("North"), + value: CellValue::Number(5.0), + }, + }, + ]), + ); + + assert_eq!( + host.sessions + .get_mut(asker) + .map(|s| s.view().active_view.clone()), + was, + "the switch is part of the frame, so a refused frame leaves the \ + mirror where it started — and an accepted switch is relayed to \ + nobody, so a surviving one diverges from the client permanently" + ); + } + + /// Clause 4 must not fire on a frame that merely names a record it is + /// creating. See `context/test-evidence/command.md`. + #[test] + fn a_frame_that_creates_the_record_it_writes_is_not_a_broken_chain() { + use improvise_protocol::ModelCommand; + + let mut host = host(); + let (asker, _) = host.connect(10, 10); + let add = crate::ui::reduce::caught_up( + host.app(), + ModelCommand::AddItem { + category: "Region".into(), + item: "West".into(), + }, + ); + let write = crate::ui::reduce::caught_up( + host.app(), + ModelCommand::SetCell { + cell: addressed("West"), + value: CellValue::Number(7.0), + }, + ); + + let answer = host.handle(asker, ClientFrame::Commands(vec![add, write])); + assert!( !answer .iter() .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), - "TODAY: the frame is reported accepted although half of it did \ - nothing. Under clause 4 it must be refused: {answer:?}" + "the exemption is a property — a record neither resolving nor \ + claimed is one the frame creates: {answer:?}" + ); + assert_eq!( + host.app() + .model_state + .workbook + .model + .get_cell(&cell("West")), + Some(&CellValue::Number(7.0)), + "and clause 4 must not have rolled it back" ); } diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 3acd0663..f36cce7b 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -674,7 +674,7 @@ pub fn grid_updates(mirror: &GridCache, desired: &GridCache) -> Vec /// One connected client, as the server sees it: the mirror of its [`ViewState`], /// kept by folding the same commands it folds, and the grid it is believed to hold. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct ClientSession { view: ViewState, extent: (usize, usize), @@ -826,7 +826,7 @@ impl ClientSession { pub struct SessionId(pub u64); /// Every client attached to one authoritative `App`. -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub struct Sessions { next_id: u64, sessions: BTreeMap, diff --git a/crates/improvise-command/src/ui/reduce.rs b/crates/improvise-command/src/ui/reduce.rs index 5b76fe06..a93e2993 100644 --- a/crates/improvise-command/src/ui/reduce.rs +++ b/crates/improvise-command/src/ui/reduce.rs @@ -16,9 +16,20 @@ pub struct Checked<'a>(&'a Command); /// A whole frame, answered before any of it is applied (improvise-955n/08wb, /// ADR 001). Its four clauses are in `context/code/command.md`. -pub struct CheckedBatch<'a>(Vec>); +pub struct CheckedBatch<'a> { + checked: Vec>, + /// The one basis the frame stated, if any of its commands stated one. + /// Clause 4 asks it again at apply time — see `context/code/command.md`. + basis: Option, +} impl<'a> CheckedBatch<'a> { + /// What this frame was composed against. `None` for a frame that stated + /// none, which claims no record and so can break no chain. + pub fn basis(&self) -> Option<&Basis> { + self.basis.as_ref() + } + /// The document tiers are answered once, on the first command that /// carries a basis; the record tier is asked of every mutation. pub fn against(app: &App, cmds: &'a [Command]) -> Result { @@ -38,16 +49,34 @@ impl<'a> CheckedBatch<'a> { }; checked.push(one); } - Ok(Self(checked)) + Ok(Self { + checked, + basis: frame_basis.cloned(), + }) } } +/// The **structural** half of the record tier, asked again just before each +/// mutation runs. Clause 4's loud failure — `context/code/command.md`. +pub fn broken_chain(app: &App, basis: &Basis, cmd: &Command) -> Option { + let Command::Model { cmd: model, .. } = cmd else { + return None; + }; + records_named(model) + .into_iter() + // The SAME predicate the pre-frame check exempts by, not a second copy + // of it: two answers to "is the frame creating this" would drift. + .filter(|hash| !will_be_created_by_the_frame(app, basis, *hash)) + .find(|hash| app.resolve_record(*hash).is_none()) + .map(Rejection::ChainBroken) +} + impl<'a> IntoIterator for CheckedBatch<'a> { type Item = Checked<'a>; type IntoIter = std::vec::IntoIter>; fn into_iter(self) -> Self::IntoIter { - self.0.into_iter() + self.checked.into_iter() } } diff --git a/crates/improvise-protocol/src/basis.rs b/crates/improvise-protocol/src/basis.rs index b4d839bb..4a908268 100644 --- a/crates/improvise-protocol/src/basis.rs +++ b/crates/improvise-protocol/src/basis.rs @@ -48,6 +48,12 @@ pub enum Rejection { /// The batch names a record its basis says nothing about. A well-formed /// client cannot produce this; it closes force-by-omission. RecordUnstated(RecordHash), + /// A record an EARLIER command in the same frame removed. Clause 4's own + /// refusal — `context/code/protocol.md` §5 says why it is not `RecordGone`. + ChainBroken(RecordHash), + /// A mutation in the frame refused to apply at all. The frame is undone; + /// the refusal's own explanation is in the host's status line. + MutationRefused, /// A [`ClientFrame::Save`](crate::ClientFrame::Save) reached a host with /// no path (improvise-368h). Decided before anything runs. @@ -101,7 +107,9 @@ impl Rejection { | Self::LayoutMoved | Self::RecordGone(_) | Self::RecordMoved(_) - | Self::RecordUnstated(_) => true, + | Self::RecordUnstated(_) + | Self::ChainBroken(_) + | Self::MutationRefused => true, Self::NoDocumentPath | Self::SaveFailed => false, } } @@ -126,7 +134,9 @@ impl Rejection { Self::LayoutMoved => Self::RecordGone(sample), Self::RecordGone(_) => Self::RecordMoved(sample), Self::RecordMoved(_) => Self::RecordUnstated(sample), - Self::RecordUnstated(_) => Self::NoDocumentPath, + Self::RecordUnstated(_) => Self::ChainBroken(sample), + Self::ChainBroken(_) => Self::MutationRefused, + Self::MutationRefused => Self::NoDocumentPath, Self::NoDocumentPath => Self::SaveFailed, Self::SaveFailed => Self::DocumentReplaced, } diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index 6ec4b26a..2f20be7d 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -21,6 +21,15 @@ fn rejection_message(rejection: &Rejection) -> String { Rejection::RecordUnstated(_) => { "your edit named a row this client had not been told about — it was not applied".into() } + // Clause 4: the whole frame was undone, so the wording is about the + // batch rather than the one command that could not run. + Rejection::ChainBroken(_) => { + "an earlier change in the same batch removed that row — the batch was not applied" + .into() + } + Rejection::MutationRefused => { + "part of that batch was refused — the batch was not applied".into() + } // Not staleness, so neither offers a retry: they are worded as facts // about the host, because that is what they are (improvise-368h). Rejection::NoDocumentPath => { From ba3e4bd14ebff554b6afe5ebc9f1bebc8a1572c7 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:44:47 -0700 Subject: [PATCH 10/15] fix(core): the Expr match in both eval_bool halves is exhaustive too A sweep for the SHAPE of improvise-vyrx rather than the site it named. Both eval_bool and eval_bool_cached also wildcarded the outer Expr match as `_ => Err("type")`, so a new Expr variant that ought to be usable as a condition would have become a silent type error in both. Not vyrx's asymmetry -- the two agreed -- but vyrx's defect: a new variant answered wrongly with nothing to notice. Adding an Expr::And now fails the build at four sites where it previously failed at two; verified by adding the variant. The sweep's other half is which wildcards to LEAVE, recorded in the evidence: eval_agg's `match inner { Expr::Ref(..) => .., _ => false }` falls through to the GENERAL path (the per-record fold), which is right for any expression that is not a bare ref, including one that does not exist yet. The dangerous wildcard is the one falling through to a refusal or to a specialisation. CellValue::as_f64 and CategoryKind::of_name clear on the same reading. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/core.md | 20 ++++++++++++++++++++ crates/improvise-core/src/model/types.rs | 14 ++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index 3a531c2a..04aacfd1 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -72,6 +72,26 @@ operator. **The guard is the compiler**: with the wildcard removed, adding a `eval_expr_cached`, `eval_bool_cached`) where it previously failed at three, and that was verified by adding the variant and reading the errors. +**A sweep for the shape found a second instance one level up**, in the same two +functions: both `eval_bool` and `eval_bool_cached` also wildcarded the outer +`Expr` match as `_ => Err("type")`, so a new `Expr` variant that ought to be +usable as a condition — an `And`, say — would have become a silent type error in +*both*. That is not vyrx's asymmetry (the two agreed), but it is vyrx's defect: +a new variant answered wrongly with nothing to notice. Both now list the six +non-comparison variants, and adding an `Expr::And` fails the build at four sites +where it previously failed at two. Verified the same way. + +**What the sweep also settled is which wildcards to leave**, which is the more +useful half. `eval_agg`'s own `match inner { Expr::Ref(..) => …, _ => false }` +is deliberately *not* exhaustive: it falls through to the **general** path — the +per-record fold — which is the right answer for any expression that is not a +bare ref, including one that does not exist yet. The dangerous wildcard is the +one that falls through to a **refusal** or to a **specialisation**; a wildcard +that falls through to the general case is safe by construction. The same reading +clears `CellValue::as_f64`'s `_ => None` and `CategoryKind::of_name`'s +`_ => Regular`, whose inputs are a value that is not a number and a name that is +not reserved. + What this test adds is the neighbouring property the exhaustive arms do not give: that the six comparison arms *agree in value* between the two evaluators. It runs each operator against a true and a false right-hand side, so an arm diff --git a/crates/improvise-core/src/model/types.rs b/crates/improvise-core/src/model/types.rs index a734ecf2..49d767ae 100644 --- a/crates/improvise-core/src/model/types.rs +++ b/crates/improvise-core/src/model/types.rs @@ -200,7 +200,12 @@ fn eval_bool( } } } - _ => Err("type".into()), + Expr::Number(_) + | Expr::Ref(_) + | Expr::UnaryMinus(_) + | Expr::Agg(..) + | Expr::Is(_) + | Expr::If(..) => Err("type".into()), } } @@ -723,7 +728,12 @@ impl Model { } } } - _ => Err("type".into()), + Expr::Number(_) + | Expr::Ref(_) + | Expr::UnaryMinus(_) + | Expr::Agg(..) + | Expr::Is(_) + | Expr::If(..) => Err("type".into()), } } From 5dd203e6a32b8521ec134cd482a89bd5dc735df3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:45:50 -0700 Subject: [PATCH 11/15] fix(core): a view's hidden/collapsed state outlived the thing it named MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-qx1z, two arms of one asymmetry. Arm 2: View::on_category_removed cleared category_axes, page_selections and hidden_items but not collapsed_groups — the other three lines being right there is what makes it an omission. Re-adding the category brought the group back pre-collapsed. Arm 1: Model::remove_item touches no view at all, so hidden_items kept the removed name. Re-adding the item put it in the model and the category panel while leaving it absent from the grid with no indication why. The sweep is View::on_item_removed, called from Workbook::remove_item over every view — Workbook is already the stated home of cross-view ops (gotcha 6). It also answers the third question the ticket asked to check while there: a page selection naming the removed item goes too. Both reachable from shipped keys. Each test fails under an injected fault, and each fixture carries a second item or view so that "forget this item" is separated from "forget this category". Also lifts the "Default" view name to view::DEFAULT_VIEW, which parse_md needs in order to tell a seeded view from one a file asked for. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-core/src/view/mod.rs | 2 +- crates/improvise-core/src/view/types.rs | 68 +++++++++++++++++++++++++ crates/improvise-core/src/workbook.rs | 30 +++++++++-- 3 files changed, 96 insertions(+), 4 deletions(-) diff --git a/crates/improvise-core/src/view/mod.rs b/crates/improvise-core/src/view/mod.rs index 5e061ce2..b6f45941 100644 --- a/crates/improvise-core/src/view/mod.rs +++ b/crates/improvise-core/src/view/mod.rs @@ -7,4 +7,4 @@ pub use layout::{ AxisEntry, GridLayout, NEW_CATEGORY_COL, NEW_MEASURE_COL, RecordCol, RecordRow, synthetic_record_info, }; -pub use types::{DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View}; +pub use types::{DEFAULT_VIEW, DRILL_VIEW, RECORDS_VIEW, VIRTUAL_VIEW_NAMES, View}; diff --git a/crates/improvise-core/src/view/types.rs b/crates/improvise-core/src/view/types.rs index eace86e6..1efeeb3e 100644 --- a/crates/improvise-core/src/view/types.rs +++ b/crates/improvise-core/src/view/types.rs @@ -13,6 +13,10 @@ fn default_prune() -> bool { /// they put the never-persisted `_Index`/`_Dim` on axes (improvise-60z). pub const VIRTUAL_VIEW_NAMES: [&str; 2] = ["_Records", "_Drill"]; +/// The view `Workbook::new` seeds. `parse_md` must name the same one to tell a +/// seeded view from one the file asked for (improvise-m4rv). +pub const DEFAULT_VIEW: &str = "Default"; + /// Name of the view records mode switches to. pub const RECORDS_VIEW: &str = VIRTUAL_VIEW_NAMES[0]; /// Name of the view drill-down switches to. @@ -94,6 +98,22 @@ impl View { self.category_axes.shift_remove(cat_name); self.page_selections.remove(cat_name); self.hidden_items.remove(cat_name); + self.collapsed_groups.remove(cat_name); + } + + /// Forget an item this view names. A hidden or paged-to item that outlives + /// its removal comes back hidden when re-added (improvise-qx1z). + pub fn on_item_removed(&mut self, cat_name: &str, item_name: &str) { + if let Some(items) = self.hidden_items.get_mut(cat_name) { + items.remove(item_name); + } + if self + .page_selections + .get(cat_name) + .is_some_and(|s| s == item_name) + { + self.page_selections.remove(cat_name); + } } pub fn set_axis(&mut self, cat_name: &str, axis: Axis) { @@ -475,6 +495,54 @@ mod tests { assert!(!v.is_group_collapsed("Cat1", "G2")); } + /// Arm 2 of improvise-qx1z: the other three lines of `on_category_removed` + /// were right there. Evidence: `context/test-evidence/core.md`. + #[test] + fn on_category_removed_forgets_a_collapsed_group() { + let mut v = View::new("V"); + v.on_category_added("Month"); + v.toggle_group_collapse("Month", "Q1"); + v.on_category_removed("Month"); + v.on_category_added("Month"); + assert!( + !v.is_group_collapsed("Month", "Q1"), + "a re-added category came back with its group collapsed" + ); + } + + /// Arm 1 of improvise-qx1z, at the `View` level: removing an item must not + /// leave it hidden, or re-adding it brings back an invisible row. + #[test] + fn on_item_removed_forgets_a_hidden_item() { + let mut v = View::new("V"); + v.on_category_added("Region"); + v.hide_item("Region", "East"); + v.hide_item("Region", "West"); + v.on_item_removed("Region", "East"); + assert!(!v.is_hidden("Region", "East"), "East is still hidden"); + assert!( + v.is_hidden("Region", "West"), + "on_item_removed must forget one item, not the category" + ); + } + + /// The third question improvise-qx1z asked to check while there: a page + /// selection naming a removed item does not survive it either. + #[test] + fn on_item_removed_forgets_a_page_selection_naming_it() { + let mut v = View::new("V"); + v.on_category_added("Region"); + v.set_page_selection("Region", "East"); + v.on_item_removed("Region", "West"); + assert_eq!( + v.page_selection("Region"), + Some("East"), + "removing a different item must leave the selection alone" + ); + v.on_item_removed("Region", "East"); + assert_eq!(v.page_selection("Region"), None); + } + #[test] fn hide_and_show_item() { let mut v = View::new("Test"); diff --git a/crates/improvise-core/src/workbook.rs b/crates/improvise-core/src/workbook.rs index b47e120d..243708de 100644 --- a/crates/improvise-core/src/workbook.rs +++ b/crates/improvise-core/src/workbook.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use crate::model::Model; use crate::model::category::{CategoryId, ItemId}; -use crate::view::{Axis, View}; +use crate::view::{Axis, DEFAULT_VIEW, View}; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Workbook { @@ -22,11 +22,11 @@ impl Workbook { pub fn new(name: impl Into) -> Self { let model = Model::new(name); let mut views = IndexMap::new(); - views.insert("Default".to_string(), View::new("Default")); + views.insert(DEFAULT_VIEW.to_string(), View::new(DEFAULT_VIEW)); let mut wb = Self { model, views, - active_view: "Default".to_string(), + active_view: DEFAULT_VIEW.to_string(), }; for view in wb.views.values_mut() { for cat_name in wb.model.categories.keys() { @@ -91,6 +91,9 @@ impl Workbook { /// ignored, matching [`Model::remove_item`]. pub fn remove_item(&mut self, cat_name: &str, item_name: &str) { self.model.remove_item(cat_name, item_name); + for view in self.views.values_mut() { + view.on_item_removed(cat_name, item_name); + } } fn category_mut(&mut self, cat_name: &str) -> Result<&mut crate::model::category::Category> { @@ -260,6 +263,27 @@ mod tests { ); } + /// Arm 1 of improvise-qx1z through the facade: `Model::remove_item` touches + /// no view, so the cross-view sweep is `Workbook`'s job (gotcha 6). + #[test] + fn remove_item_unhides_it_in_every_view() { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.create_view("Other"); + for name in ["Default", "Other"] { + wb.views.get_mut(name).unwrap().hide_item("Region", "East"); + } + wb.remove_item("Region", "East"); + wb.add_item("Region", "East").unwrap(); + for name in ["Default", "Other"] { + assert!( + !wb.views[name].is_hidden("Region", "East"), + "{name} brought East back hidden" + ); + } + } + #[test] fn remove_item_on_unknown_category_is_a_no_op() { let mut wb = Workbook::new("Test"); From c84707261c2fd9ecdcb60b093baf327d2ab9406e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:46:15 -0700 Subject: [PATCH 12/15] fix(io): five .improv persistence defects, grammar first improvise-pnf5 (P1). A cell keyed by the empty CellKey is a legal model state -- what AddRecordRow leaves while a records row is unfinished -- and format_md wrote it as " = 0" while coord_list required at least one coordinate. Saving in records mode therefore overwrote a good document with one the application could not reopen. Given a spelling rather than discarded, because the ticket is explicit that legitimate data must not be dropped: data_coords makes the coordinate list optional and the scalar cell is written "= 42". Unambiguous under PEG, since bare_name cannot start with "=". improvise-m4rv. Workbook::new seeds a Default view and on_category_added fills it, so a file naming only Plan parsed into two views -- the phantom at index 0, and appended to the user's document on any save. parse_md now drops Default when the file named at least one view and none was Default. A file naming no view, or naming Default itself, is untouched. improvise-3yv3. format_md collected every ungrouped item and emitted the whole list before any grouped one, so an interleaved category came back reordered -- and grid axis ordering consumes item insertion order (gotcha 18). Items are now written in insertion order, a run of bare items sharing a line. improvise-qu4a. View::prune_empty never reached disk. The line is spelled with a value rather than by its presence, so the grammar generator can emit either; absent means false, the compatible reading. improvise-ppru. Model::measure_agg was lost by the roundtrip. An Aggregation section carries it, sorted -- measure_agg is a HashMap and hash order is not an order (gotcha 18), so two saves of one document must be made to agree. improvise-4lgj. The three wildcard arms in the walker silently skipped any grammar rule they did not handle, so extending improv.pest caused silent data loss rather than a loud mismatch. They now bail, and a new exhaustive match over Rule names the walker that reads each one -- a new rule stops that test compiling, which is design-principles section 2's "the compiler finds every call site" applied to a pest-derived enum. It earned its keep immediately by naming Rule::section, which the first draft had missed. That arm is also what makes the change safe: the grammar-walking generator reaches both new rules -- proved by injecting a bail into each and watching generated_file_parses fail -- so an unwalked rule is now a test failure. Un-ignores the five reproductions in tests/bug_hunt_reproductions.rs. improvise-nvq4 did NOT reproduce and is kept as a passing control. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- crates/improvise-io/src/persistence/format.rs | 48 +++++- .../improvise-io/src/persistence/improv.pest | 27 +++- crates/improvise-io/src/persistence/parse.rs | 135 +++++++++++++++-- .../src/persistence/roundtrip_tests.rs | 141 ++++++++++++++++++ tests/bug_hunt_reproductions.rs | 27 +++- 5 files changed, 349 insertions(+), 29 deletions(-) diff --git a/crates/improvise-io/src/persistence/format.rs b/crates/improvise-io/src/persistence/format.rs index 0ccd3dd3..1997009c 100644 --- a/crates/improvise-io/src/persistence/format.rs +++ b/crates/improvise-io/src/persistence/format.rs @@ -7,6 +7,7 @@ use crate::workbook::Workbook; // The one home of the pipe-quoting rule (improvise-i2m5); a name written here // must also survive `parse_formula`. See `context/code/formula-and-io.md`. +use crate::formula::AggFunc; use crate::formula::quote::{pipe_quote, quote_name}; /// A name as a section header spells it: the whole line, so no pipes where a @@ -22,6 +23,18 @@ fn quote_header(name: &str) -> String { } } +/// The on-disk keyword for an aggregation, exhaustively — a new `AggFunc` +/// variant fails the build here rather than being written as something else. +fn agg_keyword(agg: &AggFunc) -> &'static str { + match agg { + AggFunc::Sum => "sum", + AggFunc::Avg => "avg", + AggFunc::Min => "min", + AggFunc::Max => "max", + AggFunc::Count => "count", + } +} + fn format_number(n: f64) -> String { if n.is_infinite() { return if n.is_sign_positive() { @@ -108,6 +121,9 @@ pub fn format_md(workbook: &Workbook) -> String { if !view.number_format.is_empty() { w!(out, "format: {}", view.number_format); } + if view.prune_empty { + w!(out, "prune: true"); + } for (prefix, map) in [ ("hidden", &view.hidden_items), ("collapsed", &view.collapsed_groups), @@ -137,6 +153,19 @@ pub fn format_md(workbook: &Workbook) -> String { } } + // ── Aggregation ────────────────────────────────────────────────── + + // Sorted: `measure_agg` is a `HashMap` and hash order is not an order + // (gotcha 18) — two saves of one document must agree. + let mut aggs: Vec<_> = model.measure_agg.iter().collect(); + aggs.sort_by_key(|(name, _)| name.as_str()); + if !aggs.is_empty() { + w!(out, "\n## Aggregation"); + for (name, agg) in aggs { + w!(out, "- {}: {}", quote_name(name), agg_keyword(agg)); + } + } + // ── Categories (items comma-separated on one line) ─────────────── // Collect formula targets so we can exclude them from _Measure items let formula_targets: std::collections::HashSet<&str> = model @@ -152,8 +181,9 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } w!(out, "\n## Category: {}", quote_header(&cat.name)); + // Items are written in insertion order, because that is the order the + // grid's axes consume (gotcha 18). A run of bare items shares a line. let mut bare: Vec = Vec::new(); - let mut grouped: Vec = Vec::new(); for item in cat.items.values() { // For _Measure, skip items that are formula targets // (they'll be recreated from the ## Formulas section) @@ -163,16 +193,19 @@ pub fn format_md(workbook: &Workbook) -> String { continue; } match &item.group { - Some(g) => grouped.push(format!("{}[{}]", quote_name(&item.name), quote_name(g))), None => bare.push(quote_name(&item.name)), + Some(g) => { + if !bare.is_empty() { + w!(out, "- {}", bare.join(", ")); + bare.clear(); + } + w!(out, "- {}[{}]", quote_name(&item.name), quote_name(g)); + } } } if !bare.is_empty() { w!(out, "- {}", bare.join(", ")); } - for g_item in &grouped { - w!(out, "- {g_item}"); - } for g in &cat.groups { if let Some(parent) = &g.parent { w!(out, "> {}[{}]", quote_name(&g.name), quote_name(parent)); @@ -190,7 +223,10 @@ pub fn format_md(workbook: &Workbook) -> String { CellValue::Number(n) => format_number(*n), CellValue::Text(s) | CellValue::Error(s) => pipe_quote(s), }; - w!(out, "{} = {}", coord_str(&key), val_str); + match coord_str(&key) { + coords if coords.is_empty() => w!(out, "= {val_str}"), + coords => w!(out, "{coords} = {val_str}"), + } } } diff --git a/crates/improvise-io/src/persistence/improv.pest b/crates/improvise-io/src/persistence/improv.pest index 8bacdb7c..094086a3 100644 --- a/crates/improvise-io/src/persistence/improv.pest +++ b/crates/improvise-io/src/persistence/improv.pest @@ -24,6 +24,7 @@ initial_view = { "Initial View: " ~ header_name ~ NEWLINE ~ blank_lines } section = _{ category_section | formulas_section + | aggregation_section | data_section | view_section } @@ -69,6 +70,19 @@ formula_line = { formula_target = _{ " "* ~ "[" ~ name ~ "]" } formula_raw = @{ (!(formula_target? ~ NEWLINE) ~ ANY)+ } +// ── Aggregation ────────────────────────────────────────────────────────────── +// +// How a `_Measure` item aggregates over the cells a grid position covers. +// Absent means `sum`, which is `Model::agg_for`'s fallback. + +aggregation_section = { + "## Aggregation" ~ NEWLINE ~ blank_lines ~ + aggregation_line* +} + +aggregation_line = { "- " ~ name ~ ": " ~ agg_kind ~ NEWLINE ~ blank_lines } +agg_kind = @{ "sum" | "avg" | "min" | "max" | "count" } + // ── Data ───────────────────────────────────────────────────────────────────── data_section = { @@ -76,11 +90,14 @@ data_section = { data_line* } +// A cell with NO coordinates is a legal model state — the scalar cell, and the +// placeholder an unfinished records row leaves. It is spelled `= 42`. data_line = { - coord_list ~ " = " ~ cell_value ~ NEWLINE ~ blank_lines + data_coords ~ "= " ~ cell_value ~ NEWLINE ~ blank_lines } -coord_list = { coord ~ (", " ~ coord)* } +data_coords = _{ (coord_list ~ " ")? } +coord_list = { coord ~ (", " ~ coord)* } coord = { name ~ "=" ~ name } cell_value = _{ number | pipe_quoted | bare_value } @@ -98,7 +115,7 @@ view_section = { view_entry* } -view_entry = _{ format_line | hidden_line | collapsed_line | axis_line } +view_entry = _{ format_line | prune_line | hidden_line | collapsed_line | axis_line } axis_line = { name ~ ": " ~ axis_kind ~ (", " ~ name)? ~ NEWLINE ~ blank_lines @@ -107,6 +124,10 @@ axis_line = { axis_kind = @{ "row" | "column" | "page" | "filter" | "none" } format_line = { "format: " ~ rest_of_line ~ NEWLINE ~ blank_lines } +// Absent means false — the compatible reading for a file written before it +// existed, and what the writer emits for a view that does not prune. +prune_line = { "prune: " ~ bool_kw ~ NEWLINE ~ blank_lines } +bool_kw = @{ "true" | "false" } hidden_line = { "hidden: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines } collapsed_line = { "collapsed: " ~ name ~ "/" ~ name ~ NEWLINE ~ blank_lines } diff --git a/crates/improvise-io/src/persistence/parse.rs b/crates/improvise-io/src/persistence/parse.rs index 156cbf90..963ed3b0 100644 --- a/crates/improvise-io/src/persistence/parse.rs +++ b/crates/improvise-io/src/persistence/parse.rs @@ -6,7 +6,7 @@ use pest::Parser; use pest::iterators::{Pair, Pairs}; use pest_derive::Parser; -use crate::formula::parse_formula; +use crate::formula::{AggFunc, parse_formula}; use crate::model::category::Group; use crate::model::cell::{CellKey, CellValue}; use crate::view::Axis; @@ -35,6 +35,7 @@ struct PView { axes: Vec<(String, Axis)>, page_selections: Vec<(String, String)>, format: String, + prune_empty: bool, hidden: Vec<(String, String)>, collapsed: Vec<(String, String)>, } @@ -46,6 +47,7 @@ struct ParsedFile { initial_view: Option, categories: Vec, formulas: Vec<(String, String)>, + measure_agg: Vec<(String, AggFunc)>, data: Vec<(CellKey, CellValue)>, views: Vec, } @@ -177,12 +179,39 @@ fn walk_category_section(pair: Pair<'_, Rule>) -> Result { Rule::group_hierarchy => { pc.group_parents.push(extract_name_pair(entry)?); } - _ => {} + other => bail!("unhandled rule in category_section: {other:?}"), } } Ok(pc) } +/// The on-disk keyword for an aggregation, exhaustively — the reader's half of +/// `format.rs::agg_keyword`. +fn parse_agg_kind(kind: &str) -> Result { + match kind { + "sum" => Ok(AggFunc::Sum), + "avg" => Ok(AggFunc::Avg), + "min" => Ok(AggFunc::Min), + "max" => Ok(AggFunc::Max), + "count" => Ok(AggFunc::Count), + _ => bail!("Unknown aggregation: {kind}"), + } +} + +/// `## Aggregation`: `- Measure: avg` lines as `(item, AggFunc)` pairs. +fn walk_aggregation_section(pair: Pair<'_, Rule>) -> Result> { + let mut out = Vec::new(); + for line in pair.into_inner() { + if line.as_rule() == Rule::aggregation_line { + let mut parts = line.into_inner(); + let name = extract_name(next(&mut parts, "aggregation_line")?)?; + let kind = next(&mut parts, "aggregation_line")?; + out.push((name, parse_agg_kind(kind.as_str())?)); + } + } + Ok(out) +} + /// `## Formulas`: `(raw text, target category)` pairs. A line with no /// `[Category]` suffix targets `_Measure`. fn walk_formulas_section(pair: Pair<'_, Rule>) -> Result> { @@ -231,21 +260,28 @@ fn walk_data_section(pair: Pair<'_, Rule>) -> Result> for dl in pair.into_inner() { if dl.as_rule() == Rule::data_line { let mut dl_inner = dl.into_inner(); - let coord_list = next(&mut dl_inner, "data_line coords")?; - let value_pair = next(&mut dl_inner, "data_line value")?; - - let coords: Vec<_> = coord_list - .into_inner() - .filter(|p| p.as_rule() == Rule::coord) - .map(extract_name_pair) - .collect::>()?; - + let first = next(&mut dl_inner, "data_line")?; + // `coord_list` is optional in the grammar: a cell with no + // coordinates is the scalar cell, spelled `= 42` (improvise-pnf5). + let (coords, value_pair) = match first.as_rule() { + Rule::coord_list => (walk_coord_list(first)?, next(&mut dl_inner, "data_line")?), + _ => (Vec::new(), first), + }; out.push((CellKey::new(coords), walk_cell_value(value_pair)?)); } } Ok(out) } +/// The `Cat=Item, …` prefix of a data line. +fn walk_coord_list(coord_list: Pair<'_, Rule>) -> Result> { + coord_list + .into_inner() + .filter(|p| p.as_rule() == Rule::coord) + .map(extract_name_pair) + .collect() +} + /// The keyword after `Category:` on an axis line. fn parse_axis_kind(kind_str: &str) -> Result { match kind_str { @@ -281,6 +317,7 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result { axes: Vec::new(), page_selections: Vec::new(), format: String::new(), + prune_empty: false, hidden: Vec::new(), collapsed: Vec::new(), }; @@ -288,9 +325,10 @@ fn walk_view_section(pair: Pair<'_, Rule>) -> Result { match entry.as_rule() { Rule::axis_line => walk_axis_line(entry, &mut pv)?, Rule::format_line => pv.format = first_str(entry)?, + Rule::prune_line => pv.prune_empty = first_str(entry)? == "true", Rule::hidden_line => pv.hidden.push(extract_name_pair(entry)?), Rule::collapsed_line => pv.collapsed.push(extract_name_pair(entry)?), - _ => {} + other => bail!("unhandled rule in view_section: {other:?}"), } } Ok(pv) @@ -310,9 +348,10 @@ fn walk_file(file: Pair<'_, Rule>) -> Result { } Rule::category_section => parsed.push_category(walk_category_section(pair)?), Rule::formulas_section => parsed.formulas.extend(walk_formulas_section(pair)?), + Rule::aggregation_section => parsed.measure_agg.extend(walk_aggregation_section(pair)?), Rule::data_section => parsed.data.extend(walk_data_section(pair)?), Rule::view_section => parsed.views.push(walk_view_section(pair)?), - _ => {} + other => bail!("unhandled rule in file: {other:?}"), } } Ok(parsed) @@ -367,6 +406,7 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> { if !pv.format.is_empty() { view.number_format = pv.format.clone(); } + view.prune_empty = pv.prune_empty; for (cat, item) in &pv.hidden { view.hide_item(cat, item); } @@ -376,6 +416,16 @@ fn build_view(wb: &mut Workbook, pv: &PView) -> Result<()> { Ok(()) } +/// `Workbook::new` seeds a `Default` view and `on_category_added` fills it, so +/// a file naming its own views gained a phantom one it never had (improvise-m4rv). +fn drop_unnamed_default(wb: &mut Workbook, named: &[&PView]) { + let default_is_a_phantom = + !named.is_empty() && !named.iter().any(|pv| pv.name == crate::view::DEFAULT_VIEW); + if default_is_a_phantom { + wb.delete_view(crate::view::DEFAULT_VIEW).ok(); + } +} + /// Turn pass 1's collectors into a [`Workbook`]. fn build_workbook(parsed: ParsedFile) -> Result { let name = parsed @@ -389,13 +439,19 @@ fn build_workbook(parsed: ParsedFile) -> Result { // A file written before improvise-60z may carry the scratch views; drop // them, and `initial_view` then falls through to the default. - for pv in parsed + let named: Vec<&PView> = parsed .views .iter() .filter(|pv| !crate::view::VIRTUAL_VIEW_NAMES.contains(&pv.name.as_str())) - { + .collect(); + for pv in &named { build_view(&mut wb, pv)?; } + drop_unnamed_default(&mut wb, &named); + + for (item, agg) in parsed.measure_agg { + wb.model.measure_agg.insert(item, agg); + } if let Some(iv) = &parsed.initial_view && wb.views.contains_key(iv) @@ -420,6 +476,55 @@ pub fn parse_md(text: &str) -> Result { build_workbook(walk_file(parse_tree(text)?)?) } +#[cfg(test)] +mod grammar_coverage { + use super::Rule; + + /// Which walker reads each grammar rule. A new rule in `improv.pest` adds a + /// `Rule` variant and this match stops compiling — improvise-4lgj. + fn walker_of(rule: Rule) -> &'static str { + match rule { + Rule::file | Rule::EOI => "walk_file", + Rule::version_line => "walk_file (skipped: the version is the literal)", + Rule::model_name | Rule::initial_view => "walk_file", + Rule::section => "silent: hoisted into file", + Rule::category_section => "walk_category_section", + Rule::item_list | Rule::grouped_item | Rule::group_hierarchy => "walk_category_section", + Rule::category_entry => "silent: hoisted into category_section", + Rule::formulas_section | Rule::formula_line | Rule::formula_raw => { + "walk_formulas_section" + } + Rule::formula_target => "silent: hoisted into formula_line", + Rule::aggregation_section | Rule::aggregation_line | Rule::agg_kind => { + "walk_aggregation_section" + } + Rule::data_section | Rule::data_line => "walk_data_section", + Rule::data_coords => "silent: hoisted into data_line", + Rule::coord_list | Rule::coord => "walk_coord_list", + Rule::cell_value => "silent: hoisted into data_line", + Rule::number | Rule::bare_value => "walk_cell_value", + Rule::view_section => "walk_view_section", + Rule::axis_line | Rule::axis_kind => "walk_axis_line", + Rule::format_line | Rule::prune_line | Rule::bool_kw => "walk_view_section", + Rule::hidden_line | Rule::collapsed_line => "walk_view_section", + Rule::view_entry => "silent: hoisted into view_section", + Rule::name => "silent: hoisted into its parent", + Rule::pipe_quoted | Rule::pipe_inner | Rule::bare_name => "extract_name", + Rule::header_name | Rule::line_name => "header_name", + Rule::rest_of_line => "first_str", + Rule::blank_lines => "silent: whitespace", + } + } + + /// The compile is the test: this call only type-checks while every `Rule` + /// variant is classified above. The assertions are a smoke check. + #[test] + fn every_grammar_rule_names_the_walker_that_reads_it() { + assert_eq!(walker_of(Rule::prune_line), "walk_view_section"); + assert_eq!(walker_of(Rule::coord_list), "walk_coord_list"); + } +} + #[cfg(test)] mod tests { use super::parse_md; diff --git a/crates/improvise-io/src/persistence/roundtrip_tests.rs b/crates/improvise-io/src/persistence/roundtrip_tests.rs index 9f4f80e1..e85b180c 100644 --- a/crates/improvise-io/src/persistence/roundtrip_tests.rs +++ b/crates/improvise-io/src/persistence/roundtrip_tests.rs @@ -920,3 +920,144 @@ fn a_bare_item_name_with_a_space_is_rejected() { "precondition: the same line quoted must parse" ); } + +// ── prune_empty (improvise-qu4a) ───────────────────────────────────────── + +/// The FLAG is asserted, not the output's stability: both sides dropped it. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn prune_empty_survives_the_roundtrip() { + let mut wb = two_cat_model(); + wb.active_view_mut().prune_empty = true; + let text = format_md(&wb); + assert!(text.contains("prune: true"), "no prune line:\n{text}"); + assert!(parse_md(&text).unwrap().active_view().prune_empty); +} + +/// The direction the default supplies, which is why it is the discriminating +/// one to state separately: a writer that always emits `prune: true` fails it. +#[test] +fn a_view_that_does_not_prune_writes_no_prune_line() { + let mut wb = two_cat_model(); + wb.active_view_mut().prune_empty = false; + let text = format_md(&wb); + assert!(!text.contains("prune:"), "unexpected prune line:\n{text}"); + assert!(!parse_md(&text).unwrap().active_view().prune_empty); +} + +/// An older file carries no `prune:` line; `false` is the compatible reading. +#[test] +fn a_file_without_a_prune_line_does_not_prune() { + let text = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n"; + assert!(!parse_md(text).unwrap().active_view().prune_empty); +} + +// ── The seeded Default view (improvise-m4rv) ───────────────────────────── + +/// A file that names its own views must not gain the one `Workbook::new` +/// seeds. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_file_naming_its_own_views_gains_no_default() { + let text = "v2025-04-09\n# T\nInitial View: Plan\n\n## View: Plan\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!(wb.views.keys().collect::>(), vec!["Plan"]); + assert_eq!(wb.active_view, "Plan"); +} + +/// The control the fix must not break: a file naming NO view still opens on +/// the seeded one, and a file that names `Default` itself keeps it. +#[test] +fn a_file_naming_no_view_keeps_the_seeded_default() { + let text = "v2025-04-09\n# T\n\n## Category: C\n- a\n"; + let wb = parse_md(text).unwrap(); + assert_eq!(wb.views.keys().collect::>(), vec!["Default"]); + + let named = "v2025-04-09\n# T\n\n## View: Default\n\n## Category: C\n- a\n"; + assert_eq!( + parse_md(named).unwrap().views.keys().collect::>(), + vec!["Default"] + ); +} + +// ── Aggregation (improvise-ppru) ───────────────────────────────────────── + +/// Two saves of one document must agree: `measure_agg` is a `HashMap`, and +/// hash order is not an order (gotcha 18). +#[test] +fn the_aggregation_section_is_written_in_a_stable_order() { + let mut wb = Workbook::new("T"); + for (item, agg) in [ + ("Zebra", crate::formula::AggFunc::Min), + ("Apple", crate::formula::AggFunc::Count), + ("Mango", crate::formula::AggFunc::Avg), + ] { + wb.add_item("_Measure", item).unwrap(); + wb.model.measure_agg.insert(item.into(), agg); + } + let text = format_md(&wb); + let lines: Vec<&str> = text + .lines() + .skip_while(|l| *l != "## Aggregation") + .skip(1) + .take(3) + .collect(); + assert_eq!( + lines, + vec!["- Apple: count", "- Mango: avg", "- Zebra: min"] + ); + assert_eq!(format_md(&parse_md(&text).unwrap()), text); +} + +// ── Interleaved items (improvise-3yv3) ─────────────────────────────────── + +/// The writer emits items in insertion order, splitting a bare run where a +/// grouped item interrupts it. Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn an_interleaved_category_keeps_its_item_order() { + let mut wb = Workbook::new("T"); + wb.add_category("C").unwrap(); + for name in ["a", "b"] { + wb.add_item("C", name).unwrap(); + } + wb.add_item_in_group("C", "g1", "G").unwrap(); + wb.add_item("C", "c").unwrap(); + wb.add_item_in_group("C", "g2", "G").unwrap(); + + let text = format_md(&wb); + let body: Vec<&str> = text + .lines() + .skip_while(|l| *l != "## Category: C") + .skip(1) + .take(4) + .collect(); + assert_eq!(body, vec!["- a, b", "- g1[G]", "- c", "- g2[G]"]); + assert_eq!( + parse_md(&text) + .unwrap() + .model + .category("C") + .unwrap() + .ordered_item_names(), + wb.model.category("C").unwrap().ordered_item_names() + ); +} + +// ── The scalar cell (improvise-pnf5) ───────────────────────────────────── + +/// A cell with no coordinates is a legal model state and now has a spelling. +/// Evidence: `context/test-evidence/formula-and-io.md`. +#[test] +fn a_cell_with_no_coordinates_round_trips() { + use crate::model::cell::CellKey; + let mut wb = two_cat_model(); + wb.model + .set_cell(CellKey::new(vec![]), CellValue::Number(42.0)); + let text = format_md(&wb); + assert!(text.contains("\n= 42\n"), "expected a scalar line:\n{text}"); + let back = parse_md(&text).unwrap(); + assert_eq!( + back.model.data.get(&CellKey::new(vec![])), + Some(&CellValue::Number(42.0)) + ); + assert_eq!(format_md(&back), text); +} diff --git a/tests/bug_hunt_reproductions.rs b/tests/bug_hunt_reproductions.rs index 968d87e3..d7836ebf 100644 --- a/tests/bug_hunt_reproductions.rs +++ b/tests/bug_hunt_reproductions.rs @@ -8,7 +8,6 @@ use improvise_io::{ workbook::Workbook, }; #[test] -#[ignore = "improvise-3yv3: known bug reproduction"] fn grouped_item_order_survives_save() { let mut w = Workbook::new("test"); w.add_category("C").unwrap(); @@ -21,7 +20,6 @@ fn grouped_item_order_survives_save() { ); } #[test] -#[ignore = "improvise-ppru: known bug reproduction"] fn aggregation_survives_save() { let mut w = Workbook::new("test"); w.add_item("_Measure", "Price").unwrap(); @@ -34,7 +32,6 @@ fn aggregation_survives_save() { ); } #[test] -#[ignore = "improvise-m4rv: known bug reproduction"] fn deleted_default_stays_deleted() { let mut w = Workbook::new("test"); w.create_view("Custom"); @@ -203,7 +200,6 @@ fn literal_cr_name_survives_roundtrip() { ); } #[test] -#[ignore = "improvise-pnf5: known bug reproduction"] fn scalar_cell_survives_roundtrip() { use improvise_io::model::cell::{CellKey, CellValue}; let mut w = Workbook::new("test"); @@ -314,7 +310,6 @@ fn formula_panel_handles_zero_inner_width() { } #[test] -#[ignore = "improvise-pnf5: known bug reproduction"] fn save_with_unfinished_record_is_readable() { use improvise::{ command::{ @@ -417,3 +412,25 @@ fn ordinary_controls() { ); assert!(parse_md(&format_md(&w)).is_ok()); } + +// Passing control (improvise-nvq4): quote_name already pipe-quotes a name +// containing `/`, so hidden/collapsed round-trip. The report did not reproduce. +#[test] +fn slash_in_hidden_name_survives_roundtrip() { + let mut w = Workbook::new("test"); + w.add_category("C").unwrap(); + w.add_item("C", "A/B").unwrap(); + w.add_item_in_group("C", "x", "G/H").unwrap(); + w.active_view_mut().hide_item("C", "A/B"); + w.active_view_mut().toggle_group_collapse("C", "G/H"); + let text = format_md(&w); + let r = parse_md(&text).unwrap_or_else(|e| panic!("unparseable: {e}\n{text}")); + assert!( + r.active_view().is_hidden("C", "A/B"), + "hidden lost:\n{text}" + ); + assert!( + r.active_view().is_group_collapsed("C", "G/H"), + "collapsed lost:\n{text}" + ); +} From 72b77566c50bdc461d3b8a96143c699d1e9fcf0e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:46:24 -0700 Subject: [PATCH 13/15] fix(web,harness): the demo's second view was the phantom improvise-m4rv removed CROSSES TWO BOUNDARIES OUTSIDE THIS AGENT'S GROUND, deliberately and reported: web/demo.improv and harness/tests/contract.spec.js. Dropping the invented Default view takes the shipped demo from two views to one, and the spec asserted the phantom in two places -- improvise-m4rv's acceptance criteria name the spec directly. The load-bearing half is the improvise-iioi click test, which needs the view panel to hold two rows so that a click can name a DIFFERENT row than the cursor does; with one view both readings give the same answer and the test stops discriminating. The phantom was supplying that second row by accident. So the demo gains a real second view, Season -- a genuine alternate pivot (Quarter by measure, paged by Product) rather than an auto-arrangement. Plan stays first and stays Initial View, so the cursor still starts at index 0 and tests/web_demo_model.rs is unaffected: every visible cell of the ACTIVE view is still a Record (gotcha 46). Verified by cargo. The spec's axis assertion at index 0 moves from the phantom's "R:Product C:Quarter" to Plan's own "R:Product P:Quarter", derived from viewmodel::view_axis_summary rather than guessed. NOT VERIFIED: harness/ has no node_modules and web/pkg is unbuilt here, so Playwright could not be run. These edits are mechanical substitutions and need a browser-job run before they are believed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- harness/tests/contract.spec.js | 24 ++++++++++++------------ web/demo.improv | 8 ++++++++ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/harness/tests/contract.spec.js b/harness/tests/contract.spec.js index fc4c5f70..8da1d071 100644 --- a/harness/tests/contract.spec.js +++ b/harness/tests/contract.spec.js @@ -73,7 +73,7 @@ test.describe("every field the render model projects reaches the screen", () => // The axis line is the shared `view_axis_summary`; its absence would mean // a second derivation had crept in. await expect(page.locator("#views li").nth(0).locator(".axes")).toHaveText( - "R:Product C:Quarter", + "R:Product P:Quarter", ); await page.keyboard.press("Escape"); @@ -196,21 +196,21 @@ test.describe("a pointer event reaches the client and its answer is painted", () // A view row (improvise-iioi). The activation must read the CLICKED index, // not `view_panel_cursor` — so the cursor must not already be on the row - // clicked, or both readings give the same answer. `Default` is index 0 and - // holds the cursor by default, so move to it and click `Plan`. + // clicked, or both readings give the same answer. `Plan` is index 0 and + // holds the cursor by default, so move to it and click `Season`. await page.keyboard.press("Shift+V"); - await expect(page.locator("#views li.selected")).toContainText("Default"); - // Switch to Default by KEYBOARD, leaving the cursor on it. The click then + await expect(page.locator("#views li.selected")).toContainText("Plan"); + // Switch to Plan by KEYBOARD, leaving the cursor on it. The click then // names a different row than the cursor does, which is the whole point. await page.keyboard.press("Enter"); - await expect(page.locator("#view-name")).toHaveText("Default"); - - const plan = page.locator("#views li").filter({ hasText: "Plan" }).first(); - await plan.click(); - // Reading the cursor instead of the index re-selects Default and fails. await expect(page.locator("#view-name")).toHaveText("Plan"); - await expect(page.locator("#views li.active")).toContainText("Plan"); - await expect(page.locator("#views li.selected")).toContainText("Plan"); + + const season = page.locator("#views li").filter({ hasText: "Season" }).first(); + await season.click(); + // Reading the cursor instead of the index re-selects Plan and fails. + await expect(page.locator("#view-name")).toHaveText("Season"); + await expect(page.locator("#views li.active")).toContainText("Season"); + await expect(page.locator("#views li.selected")).toContainText("Season"); // A formula row: navigation only, because FormulaPanel binds no Enter and // a click mirrors Enter. The Rust guard is on the KEYMAP — clients.md. diff --git a/web/demo.improv b/web/demo.improv index b2eb845b..b1e14a28 100644 --- a/web/demo.improv +++ b/web/demo.improv @@ -10,6 +10,14 @@ _Index: none _Dim: none format: ,.0 +## View: Season +Quarter: row +_Measure: column +Product: page, Latte +_Index: none +_Dim: none +format: ,.0 + ## Formulas - Profit = Revenue - Cost From 98be603cb9b9f16161798ff282613f70c2f2d00a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:46:32 -0700 Subject: [PATCH 14/15] docs(test-evidence): what this wave's persistence and view tests are evidence of Keyed by test name per design-principles section 7, recording for each one the bug it reproduced, the fault injected to watch it fail, and -- where a test has a neighbour that looks like it -- the input that separates the two. Three entries are about that separation rather than about a bug: - a_view_that_does_not_prune_writes_no_prune_line is the direction the default supplies, so a writer emitting "prune: true" unconditionally passes the real test and fails this one. - an_interleaved_category_keeps_its_item_order explains why the ticket's own two-item reproduction cannot discriminate: with one item of each kind, both the right and the wrong order are a single bare run followed by a single grouped one. - slash_in_hidden_name_survives_roundtrip records that improvise-nvq4 does NOT reproduce, and is kept as a passing control so the claim cannot be re-filed without someone first watching it pass. core.md is touched for the improvise-qx1z entries, which are View-level and belong there rather than in formula-and-io.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/core.md | 35 ++++++++ context/test-evidence/formula-and-io.md | 112 ++++++++++++++++++++++++ 2 files changed, 147 insertions(+) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index eb9d8a53..aa4b984d 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -351,6 +351,41 @@ test flips the category back to `Page` afterwards, so it distinguishes "omits The fixture puts a category on each of `Page`, `Filter`, `Row` and `None` so the result separates the two questions rather than merely counting. +### on_category_removed_forgets_a_collapsed_group + +improvise-qx1z arm 2. `View::on_category_removed` cleared `category_axes`, +`page_selections` and `hidden_items` — but not `collapsed_groups`. The other +three lines being right there is what makes this an omission rather than a +decision: collapse `Month/Q1`, delete the category, re-add it with `Jan[Q1]`, +and the group comes back **pre-collapsed** with nothing saying why. Reachable +from shipped keys, through the category panel. + +Fault injected: delete the added line; the test fails at +`is_group_collapsed("Month", "Q1")`. The test re-adds the category after +removing it, because a view that merely *forgot the axis* would also report +`false` for a category it no longer knows — asking the question after the +re-add is what separates "cleared" from "not currently registered". + +### on_item_removed_forgets_a_hidden_item + +improvise-qx1z arm 1, at the `View` level. `Model::remove_item` removes the item +and its cells and **touches no view at all**, so `hidden_items["Region"]` kept +`"East"`: re-adding the item put it in the model and in the category panel while +leaving it absent from the grid, with no indication why. + +The fixture hides **two** items and removes one. Without the second, a fix that +dropped the whole category entry — `hidden_items.remove(cat_name)` — would pass +while destroying unrelated state; the surviving `West` assertion is the input +that separates "forget this item" from "forget this category". + +### on_item_removed_forgets_a_page_selection_naming_it + +The third question improvise-qx1z asked to check while there, and the answer was +the same: a page selection naming a removed item survived it. Asserted in both +directions in one test — removing a *different* item must leave the selection +alone — because a hook that cleared the selection unconditionally would satisfy +the removal half and silently reset the user's page on every unrelated delete. + ## `view/layout.rs` ### prune_axis_items_indexes_data_positions_not_visual_ones diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index 4fe396c3..1b5a3bb3 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -442,6 +442,118 @@ honest — without it, "the writer quotes it" would be consistent with the reade also accepting the unquoted form. The trailing precondition (the same line quoted *does* parse) stops it passing because of some unrelated syntax error. +### prune_empty_survives_the_roundtrip + +improvise-qu4a. `View::prune_empty` was never written to disk: `format.rs` wrote +`format:`, `hidden:` and `collapsed:` for a view and nothing else, and +`improv.pest`'s `view_entry` had no `prune` alternative. Pressing `P`, saving +and reopening brought every pruned row and column back — and the toggle marks +the document dirty, so the user is invited to save the setting that is about to +be discarded. + +The reason nothing caught it is the shape worth remembering: it is **invisible +to `fmt(parse(x)) == fmt(parse(fmt(parse(x))))`, because both sides drop it** +(`doctrine(a_roundtrip_equality_is_blind_to_a_loss_both_sides_share)`). So this +test asserts the **flag**, never the output's stability. Fault injected: delete +the two-line `w!` in `format_md`; the test fails at the `prune: true` assertion, +and the two controls below still pass — which is exactly why they are controls +and not the evidence. + +#### a_view_that_does_not_prune_writes_no_prune_line + +The control, stated separately because it is the direction the default supplies. +A writer that emitted `prune: true` unconditionally passes the test above and +fails this one. Not evidence of the bug; evidence the fix is not one-directional. + +#### a_file_without_a_prune_line_does_not_prune + +The compatibility reading, decided rather than defaulted: a file written before +the line existed means `false`, which is what happens today. `improv.pest` +spells `prune: true|false` rather than making the line's presence the boolean, +so the grammar-walking generator can emit `prune: false` and it round-trips to +`false` through a writer that omits it. + +### a_file_naming_its_own_views_gains_no_default + +improvise-m4rv. `Workbook::new` seeds a `Default` view and `on_category_added` +assigns every parsed category into **every** existing view, so a file naming +only `Plan` parsed into *two* views — the second auto-laid-out and never in the +file. Two consequences, the second worse: the shipped demo's view panel showed a +phantom row **at index 0**, so the first `V`+`Enter` a visitor pressed took them +off the curated view; and open-and-save with no edits rewrote the user's +document, appending the whole `## View: Default` block permanently. + +`examples/demo.improv` was unaffected only because its view *is* named +`Default`, which is why the bug survived the shipped-document tests. + +Fault injected: remove the `drop_unnamed_default` call in `build_workbook`; the +test fails with `["Default", "Plan"]`. The fix is narrow on purpose — it drops +`Default` only when the file named **at least one** view and none of them was +`Default`, so the two cases in the control below are untouched. + +#### a_file_naming_no_view_keeps_the_seeded_default + +The control the fix must not break, in both directions: a file with no `## View` +section still opens on the seeded view (the legacy reading), and a file that +names `Default` itself keeps it. Without this, "drop Default" could have been +implemented as "always drop Default" and the test above would not have noticed. + +### the_aggregation_section_is_written_in_a_stable_order + +improvise-ppru's companion. `Model::measure_agg` is a `HashMap`, and an index +whose iteration order is hash order is not an order (gotcha 18) — writing it +unsorted would give a different `.improv` file per process, the same class of +defect as `analyze_records`. The test pins the three lines in name order and +then asserts `format(parse(text)) == text`. Fault injected: drop the +`sort_by_key`; the assertion fails on most runs, which is itself the argument +for pinning it rather than trusting a single green run. + +### an_interleaved_category_keeps_its_item_order + +improvise-3yv3. `format_md` collected every ungrouped item into `bare` and +emitted that whole list *before* any grouped item, so a category built as +`[grouped, ungrouped]` came back `[ungrouped, grouped]`. Grid axis ordering +consumes item insertion order (gotcha 18), so reopening a document silently +rearranged its rows and columns. + +The fixture interleaves deliberately — `a, b, g1[G], c, g2[G]` — because the +ticket's own two-item reproduction cannot tell "writes grouped items last" from +"writes each run in place": with one item of each kind, *both* orders are a +single bare run followed by a single grouped one. The test asserts the four +emitted lines, not just the parsed order, so a writer that happened to re-sort +correctly by accident still fails. Fault injected: restore the two-vector split; +the line assertion fails with `["- a, b, c", "- g1[G]", "- g2[G]"]`. + +### a_cell_with_no_coordinates_round_trips + +improvise-pnf5, the P1. A cell keyed by the empty `CellKey` is a legal model +state — it is what `AddRecordRow` leaves while a records row is unfinished, and +it is reachable from the shipped `r` then `o` keys. `format_md` wrote it as +`" = 0"` and `coord_list` required at least one coordinate, so **saving in +records mode overwrote a good document with one the application could not +reopen**. `CleanEmptyRecords` runs on leaving records mode, not before `Save`. + +The fix is a spelling rather than a discard, because the ticket is explicit that +legitimate data must not be dropped: `data_coords = _{ (coord_list ~ " ")? }` +gives the scalar cell the line `= 42`. It is unambiguous under PEG because +`coord` needs a `name`, and `bare_name` cannot start with `=`, so the optional +group fails cleanly and the `"= "` literal matches. + +Fault injected: revert the writer's `match` to the old `"{} = {}"`; the test +fails at the `\n= 42\n` assertion, and `parse_md` then rejects its own writer's +output. The trailing `format_md(&back) == text` is what makes this a round trip +rather than a parse check. + +#### slash_in_hidden_name_survives_roundtrip + +**Not a bug.** improvise-nvq4 (external report) claimed a `/` in an item or +group name corrupts the `hidden:`/`collapsed:` line, whose format is +`Cat/Item`. It does not reproduce: `hidden_line` reads two `name`s, a `name` is +`pipe_quoted | bare_name`, and `bare_name` excludes `/` — so `quote_name` +already writes `hidden: C/|A/B|` and `extract_name_pair` reads it back. The test +is kept as a **passing control** over both maps, so the claim cannot be re-filed +without someone first watching this pass. + --- ## `improvise-io/src/persistence/export.rs` From 388f7c65004407b063049fd7d7dd34b652eecf89 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 14:46:52 -0700 Subject: [PATCH 15/15] fix(project): the mirror settles its mode against every grid it is projected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-el9h. `ClientSession::fold` settled `view.mode` against the grid that session was LAST projected, and `project_diff` then assigned `self.grid = desired` and never settled again. The client has the other shape — `ClientState::fold` settles on every fold, `Command::Project` included — so for `[SwitchView(records-shaped view), ChangeMode(Normal)]` the mirror kept `Normal` while the client ended on `RecordsNormal`. The test came first, as the ticket demands, and failed exactly that way. `settle_mode` is now called from `fold` and at the end of `project_diff` once the grid has moved, which also covers the three doors that write the mirror with bare `reduce_view`: whatever they leave, the next projection settles. The fixture's own precondition earned its place on the first run — a view built with `View::new` carries no axis entry for `_Index`/`_Dim`, so `is_records_mode` can never fire on one, and without the assertion the test would have compared two `Normal`s and passed against the bug. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 30 ++++++++ crates/improvise-command/src/ui/project.rs | 80 +++++++++++++++++++++- 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 9ca56f41..aca67235 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2285,6 +2285,36 @@ The frame claims North at its current counter, so it passes the check and fails never applies anything and would be testing clause 2 instead. That fixture note survives the inversion unchanged: it is still the only route to the arm. +### the_mirror_settles_its_mode_against_every_grid_it_is_later_projected + +improvise-el9h. `ClientSession::fold` settled `view.mode` against `self.grid` — the +grid that session was **last projected** — and `project_diff` then assigned +`self.grid = desired` and never settled again. The client does not have that shape: +`ClientState::fold` calls `settle_mode()` on every fold including +`Command::Project`, so when the new view's grid arrives the client re-settles and +the mirror does not. + +`[SwitchView(records-shaped view), ChangeMode(Normal)]` is the shape — precisely the +case PR #98's settle exists to serve. **Demonstrated before the fix**: the client +ended on `RecordsNormal` and the mirror on `Normal`. The fix is `settle_mode` called +from `fold` *and* at the end of `project_diff` once the grid has moved, which also +closes the three bypass doors (`apply_switch`, `adopt_deleted_views`, +`adopt_cursor_epoch` write the mirror with bare `reduce_view`): whatever they leave, +the next projection settles. + +**Two preconditions, and one of them caught a broken fixture on its first run.** The +test asserts the client starts on `Normal`, so the settle has somewhere to move the +mode *from*. And `app_with_a_records_shaped_second_view` asserts the second view +really reaches records mode — which failed immediately, because the view was built +with `View::new` and a fresh `View` carries no axis entry for `_Index`/`_Dim`, so +`is_records_mode` can never fire on one. Without that assertion the test would have +compared two modes that were both `Normal` for a reason having nothing to do with +the bug, and passed against the unfixed code. + +The yardstick is a real `improvise_protocol::ClientState` fed the same projections +in arrival order, not a hand-written expectation: the property is that the mirror +and the client run the same function over the same stream. + ### a_failed_frame_puts_the_askers_mirror_back_on_the_view_it_was_on Clause 4's least obvious cost, in the ticket's own words: **rolling back the `App` diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index f36cce7b..a63c125c 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -712,8 +712,12 @@ impl ClientSession { /// same function the client runs. Private: [`Sessions::apply`] is the door. fn fold(&mut self, cmd: &Command) { reduce_view(&mut self.view, cmd); - // The mirror settles exactly as the client does, against the grid this - // session was last projected (improvise-e5pf). + self.settle_mode(); + } + + /// Settle against the grid this mirror **holds**, after a fold and after + /// every projection that moves it (improvise-e5pf/el9h). + fn settle_mode(&mut self) { self.view.mode = improvise_protocol::settled_mode(&self.view.mode, self.grid.records); } @@ -769,6 +773,9 @@ impl ClientSession { self.grid = desired; self.projected = Some(viewport); self.projected_view = Some(self.view.active_view.clone()); + // The client re-settles on every fold, this projection included, so + // a mirror that settled only in `fold` diverges (improvise-el9h). + self.settle_mode(); } out.extend(self.basis_update(app).map(Projection::Basis)); out @@ -1436,6 +1443,75 @@ mod tests { app } + /// A pivot-shaped default view plus a records-shaped second one, so a + /// switch between them crosses the boundary `settled_mode` watches. + fn app_with_a_records_shaped_second_view() -> App { + let mut app = app_with_data(); + // Cloned, not `View::new`: a fresh view has no axis entry for the + // virtual categories, so `is_records_mode` never fires on one. + let mut records = app.model_state.workbook.active_view().clone(); + records.name = "Records".to_string(); + records.set_axis("_Index", Axis::Row); + records.set_axis("_Dim", Axis::Column); + records.set_axis("Region", Axis::None); + records.set_axis("_Measure", Axis::None); + app.model_state + .workbook + .views + .insert("Records".to_string(), records); + app.rebuild_layout(); + assert!( + grid_in(&app, "Records", full_viewport()).records, + "the second view must really reach records mode, or the test \ + asserts a settle that had nothing to settle" + ); + app + } + + /// improvise-el9h: the mirror must settle against the grid it ends up + /// holding. Evidence: `context/test-evidence/command.md`. + #[test] + fn the_mirror_settles_its_mode_against_every_grid_it_is_later_projected() { + use improvise_protocol::AppMode; + + let mut app = app_with_a_records_shaped_second_view(); + let mut sessions = Sessions::default(); + let (id, snapshot) = sessions.connect(&app, 100, 100); + let mut client = improvise_protocol::ClientState::default(); + client.bootstrap(snapshot); + assert_eq!( + client.view.mode, + AppMode::Normal, + "the fixture must START on the pivot view, or the settle has \ + nowhere to move the mode from" + ); + + for cmd in [ + Command::SwitchView("Records".to_string()), + Command::ChangeMode(AppMode::Normal), + ] { + sessions.apply(&mut app, id, &cmd); + } + for (_, projections) in sessions.project_all(&app, Dirty::EVERYTHING) { + for projection in projections { + client.fold(&Command::Project(projection)); + } + } + + assert_eq!( + client.view.mode, + AppMode::RecordsNormal, + "the CLIENT re-settles on every fold, the projected grid included, \ + so it is the yardstick and must have moved" + ); + assert_eq!( + sessions.get_mut(id).map(|s| s.view().mode.clone()).as_ref(), + Some(&client.view.mode), + "and the mirror is the same function run on the same stream — it \ + settled against the grid it was LAST projected and never again" + ); + } + /// The yardstick `server_truth` used to be, spelled out instead of derived: /// see `context/test-evidence/command.md`. #[test]