From 902a77c174ae4adf191c154906a4dbe560658eb1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Mon, 21 Sep 2026 04:36:42 -0700 Subject: [PATCH] review: two corrections from the #92 adversarial review, and a P2 they found 1. THE JMJZ WIDTH ASSERTION COULD NOT SEE A WINDOWED WIDTH, and the evidence said it could. Injecting `if drawn { widen(..) }` -- widths from the window only -- left 1476 passing, 0 failing. Two reasons, the second structural: the measure was named `Revenue` (7 chars) against a value of `300021` (6), so the assertion passed on the header alone; and comparing grid_body's widths against compute_col_widths is INCAPABLE of testing windowing, because compute_col_widths IS grid_body with an empty window. Windowing degrades both sides identically. The same function twice is not a check, and it reads exactly like one. Fixture is now the reviewer's shape -- a total that cancels and a one-character header -- reordered once more so BOTH assertions discriminate: with the large values last, the windowed row and the cancelled total were both 1.0, so the totals assertion went blind to the row-window fault while the widths assertion happened to catch it. Large values first: for ri in rows.clone() -> totals [888888888.0] against [1.0] if drawn { widen(..) } -> widths [10] against [11] 10 rather than 5 is a smaller gap and a better witness: the missing character is the minus sign of a row outside the window. 2. THE T6DB none_cats PARAGRAPH REPORTED A NEGATIVE RESULT FROM A SETUP THAT LACKED THE CONDITION -- and correcting it found a P2. The fixture was said to put Product on Axis::None in one view and on Column in another. It did not: add_category defaults to Column and nothing moved it, so all three views had none_cats {_Index, _Dim} and the stated input to the hazard was absent. Setting it explicitly failed the property on the first run, and not on a cell value: Second loses BOTH Total columns when the host stands on Default. rebuild_layout fills Model::formula_cache for the HOST's view, and grid_in on the elsewhere path builds another view's layout against that stale cache -- the Total cells are keyed without a Product coordinate, the other view asks for one, finds nothing, and prune_empty drops the columns. Filed improvise-ac2z (P2). Not local: grid_in takes &App and recompute_formulas needs &mut Model. Guarded by a_hosts_none_cats_leak_into_another_views_formula_columns, a characterization test whose message says that failing means the bug is fixed. The property skips pairs whose none_cats differ and asserts compared > 0, so a future fixture cannot skip everything and pass. 3. Smaller, all taken. "Nothing else fails" on the t6db injection was wrong: it fails three tests, including the pre-existing prune-invalidation one -- the #78 fix had more coverage than it was credited with. And the 5ubs should_panic named one thing and exercised another: pointed at a struct it reached only the split_once branch, and all three panics shared one literal so `expected` could not discriminate. The parse is now a `parse_expected` seam fed hand-written mutated messages, each refusal with its own wording, plus a happy path. 1470 -> 1475 passing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/command.md | 33 +++++++- context/code/protocol.md | 10 ++- context/test-evidence/command.md | 97 +++++++++++++++++++--- context/test-evidence/protocol.md | 39 +++++++-- crates/improvise-command/src/ui/project.rs | 78 ++++++++++++++--- crates/improvise-protocol/src/variants.rs | 48 +++++++++-- 6 files changed, 265 insertions(+), 40 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index b5655bd0..d40b0a12 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -1157,6 +1157,29 @@ the new view's hash against the old view's — a near-certain mismatch, so a spurious reset. It re-bases rather than compares when the name moves. **And writing the test for all this found the bug that was hiding it.** +### The elsewhere path is only as good as the inputs it can rebuild (improvise-ac2z) + +`grid_in` on a view the host is not on rebuilds a `GridLayout` from the `Model` +and that view's config. Everything the *host* derived and cached stays the host's, +and `Model::formula_cache` is exactly that: `rebuild_layout` fills it with +`recompute_formulas(&none_cats)` for the **host's** view. A client on a view whose +`none_cats` differ therefore reads formula values keyed by somebody else's +coordinates — and since `prune_empty` drops what resolves to nothing, its formula +columns **vanish** rather than showing wrong numbers. + +`prune_empty` (improvise-t6db) was the first instance of this shape and was one +line. This is the second, and it is not local: `grid_in` takes `&App` while +`recompute_formulas` needs `&mut Model`, so it cannot be fixed where it is +observed. Filed rather than bodged; candidates are on the ticket. + +**How it was found is the part to carry.** The PR #78 hand-back reported this +hazard as "argued and *not* observed", from a fixture whose stated setup — +a category on `Axis::None` in one view and on Column in another — was **absent**: +`add_category` defaults a new category to Column and nothing moved it. A negative +result from a setup lacking its own condition reads identically to a real one, and +is worse than silence, because it stops the next person looking. Setting the axis +explicitly failed the property on the first run. + `project::grid_in` built a non-active view with `GridLayout::new`, which does not apply `prune_empty` — so a client on another view was projected rows its own config prunes, and its grid never lost a row, so the staleness was unobservable. @@ -1352,9 +1375,13 @@ next lever is caching *across* frames, which needs an invalidation story the The guard is `a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths`: with one walk serving three consumers, a window that leaked into the other two -would silently make a column total the sum of the visible rows. The obvious -"optimisation" — iterate `rows` instead of `0..row_count()` — is exactly that -fault, and it is what the test was proved against. +would silently make a column total the sum of the visible rows, or size a column +by what happens to be on screen. **Two** faults, then, and its first fixture could +see only one — see `context/test-evidence/command.md`, because the way it was +blind is more instructive than the fixture that fixed it. Briefly: the widths half +was asserted by comparing `grid_body` against `compute_col_widths`, which **is** +`grid_body` with an empty window, so windowing degrades both sides identically. +**The same function twice is not a check**, and it reads exactly like one. The benchmark carries `test = false`: it takes seconds by design, and the suite's rule is that no single test binary exceeds two of them. diff --git a/context/code/protocol.md b/context/code/protocol.md index 57753e97..6868b711 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -125,9 +125,13 @@ and diffs it against the names the cycle actually reached. Three things this costs, and they are the reason the module is defensive rather than three lines. The message is **parsed**, so a serde whose wording changes must fail loudly instead of reporting an empty list that passes every -cycle — hence the `unknown variant` prefix assertion, the `names.len() > 1` -floor and `a_message_that_is_not_an_unknown_variant_is_refused`, which points -it at a struct. A variant is identified by **its own serialization** rather +cycle — hence the `unknown variant` prefix assertion and the `names.len() > 1` +floor. Both live in `parse_expected`, a seam separated from the deserialization +**because a `should_panic` pointed at a real type reaches only one of the three +refusals** and all three then shared one panic message, so `expected = …` could +not say which had fired (PR #92 review). Each now carries its own wording and is +fed a hand-written mutated message; a happy-path case keeps the three refusals +from being the only evidence the parser runs. A variant is identified by **its own serialization** rather than by `Debug`, because an externally tagged unit variant is a string and a struct variant a one-key object, and reading a name off `Debug` would depend on the field list. And it checks only that every declared variant is diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 7d5c747c..f9e7a329 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -3257,18 +3257,29 @@ drill snapshot is the one thing `grid_in` cannot reproduce for a view the host i not on (`with_frozen_records(.., None)` on the elsewhere path). Fault injected 2026-09-21: restoring `GridLayout::new` in `grid_in` — the pre-#78 -line — fails this test at the `assert_eq!`, not at the precondition. Nothing else -in the workspace fails, which is the same evidence the original one-line fix had. +line — fails this test at the `assert_eq!`, not at the precondition. It also fails +`a_write_that_prunes_a_row_out_of_another_clients_view_invalidates_it` and +`a_hosts_none_cats_leak_into_another_views_formula_columns`: **three** tests, not +the one an earlier draft of this entry claimed. The #78 fix has more coverage than +it was credited with. -**One asymmetry probed and NOT reproduced**, recorded because the reasoning looks -sound and the measurement disagrees. `rebuild_layout` calls -`recompute_formulas(&none_cats)` for the **host's** view; `grid_in` on the -elsewhere path does not, so a client on a view with different `none_cats` reads a -`formula_cache` computed for somebody else's view. The fixture was widened to test -exactly that — `Total = SUM(Revenue)` with `Product` on None in `Default` and on -Column in `Second` — and the property still holds at every pair. So the hazard is -argued and unobserved at this shape; the formula stays in the fixture, which is -where a future divergence would surface. +**The `none_cats` asymmetry is REAL, and the first draft of this entry reported it +as unobserved from a fixture that lacked the condition.** That correction is worth +more than the test. The claim was that the fixture put `Product` on None in +`Default` and on Column in `Second`; it did not — `add_category` defaults a new +category to Column and nothing moved it, so all three views had `none_cats` +`{_Index, _Dim}`, the stated input to the hazard was absent, and `Default` and +`Second` were indistinguishable to `grid_in` apart from a `number_format` that is +itself a no-op. An unprobed path reported as a negative result, which is +`doctrine(an_injection_that_does_not_fail_names_an_untested_path)` applied to a +**setup**, and the expensive kind: it stops the next person looking. + +Setting `Product` to `Axis::None` explicitly made the property fail on the first +run, and not on the value of a cell — `Second` lost **both `Total` columns +entirely** when the host stood on `Default`. Filed as improvise-ac2z, pinned by +the characterization test below, and the property now skips pairs whose +`none_cats` differ. That skip is a **bug being worked around**, not a carve-out; +it asserts `compared > 0` so a future fixture cannot skip everything and pass. ### a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths @@ -3300,3 +3311,67 @@ Fault injected 2026-09-21: `for ri in rows.clone()` in place of are identical by construction, which is why all 1469 tests passed unchanged across the change. The evidence is the benchmark, before and after, in `context/code/command.md`: about 3x, the number of passes removed. + + +### a_hosts_none_cats_leak_into_another_views_formula_columns + +improvise-ac2z, and a **characterization** test: it asserts the bug, so it fails +when the bug is fixed and says so in its own message. + +`App::rebuild_layout` calls `recompute_formulas(&none_cats)` for the **host's** +view, populating `Model::formula_cache`. `project::grid_in` on the elsewhere path +builds the other view's `GridLayout` against that same, stale cache. The cache is +keyed by the coordinates the recompute used, so with `Product` on None in the +host's view the `Total` cells are keyed *without* a `Product` coordinate; a view +with `Product` on Column asks for `(Region, Product, _Measure=Total)`, finds +nothing, and — `prune_empty` being on — drops both `Total` columns. Not a wrong +number: absent columns. + + host on `Second`, projecting `Second` 6 columns, `Total` present + host on `Default`, projecting `Second` 4 columns, no `Total` at all + +It asserts the **precondition** — that `Second` on its own really does draw the +formula's columns — before asserting their absence, so a fixture that lost the +formula would fail rather than pass on two empties. + +The fix is not local and none is chosen: `grid_in` takes `&App` and +`recompute_formulas` needs `&mut Model`, so `grid_in` cannot recompute for the +view it is projecting. Candidates are on the ticket. + +### a_window_narrows_the_cells_and_neither_the_totals_nor_the_widths + +*(Superseding fixture, PR #92 review.)* The first version could not see a windowed +**width**, while this entry claimed it could. Two reasons, and the second is +structural. Its measure was named `Revenue` (7 chars) against a value of `300021` +(6), so `col_widths[0] > "300021".len()` passed **on the header alone**. And +comparing `grid_body`'s widths against `compute_col_widths` is **incapable** of +testing windowing, because `compute_col_widths` *is* `grid_body` with an empty +window — windowing degrades both sides identically, both collapsing to the +`MIN_COL_WIDTH` clamp. **The same function twice is not a check.** + +The fixture is now the reviewer's: values `1`, `888888888`, `-888888888` so the +total **cancels to 1.0** and cannot supply the width, and a one-character measure +name so the header cannot either. Only `-888888888`, a row outside the window, +can produce `11`. + +Fault injected 2026-09-21: `if drawn { widen(..) }` in `grid_body` — widths from +the window, totals left whole-grid — fails with `left: [5]` against +`right: [11]`, and fails nothing else in the workspace. The earlier fixture left +**1476 passing, 0 failing** under that same injection. + +**The fixture was reordered once more, and the reason is the point.** With the +reviewer's ordering (`1`, `888888888`, `-888888888`) the windowed row *is* `1.0` +and the cancelled total is *also* `1.0`, so the **totals** assertion went blind to +the row-window fault — the widths assertion happened to catch it instead. Two +assertions, one of them no longer discriminating, and nothing says which. The +large values now come **first**, so the window holds `888888888` and the total is +`1.0`, and each injection fails on the assertion that names it: + +- `for ri in rows.clone()` (the whole walk windowed) → totals `[888888888.0]` + against `[1.0]` +- `if drawn { widen(..) }` (widths only) → widths `[10]` against `[11]`, the + missing character being the **minus sign** of a row outside the window + +`10` rather than `5` is a smaller gap than the reviewer's fixture gave, and a +better witness: it can only come from `-888888888`, which the window does not +contain. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 6655fee0..fad9239c 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -489,15 +489,40 @@ compile with the probe, because `improvise-wasm-client/src/socket.rs` matches `Rejection` exhaustively — which is a *different* guard and does not catch the omission, since the omitted variant there is one it was forced to name.) -### variants::tests::a_message_that_is_not_an_unknown_variant_is_refused +### variants::tests::a_type_that_is_not_an_externally_tagged_enum_is_refused -The checker's own checker. A parse of serde's message that silently fails would +### variants::tests::a_message_with_no_expected_clause_is_refused + +### variants::tests::a_message_that_lists_something_other_than_variants_is_refused + +### variants::tests::a_list_that_collapses_to_one_name_is_refused + +### variants::tests::a_well_formed_message_yields_its_names + +The checker's own checker. A parse of serde's message that silently failed would return an empty name list, and an empty list is a subset of everything — every -cycle would pass and the guard would be worse than absent. Pointing `declared` -at `Basis`, a struct, produces a message with no `expected one of` clause; the -helper must panic rather than return. Asserting the panic *message* and not -merely that it panics, because a helper that panicked for some other reason -would satisfy a bare `should_panic`. +cycle would pass and the guard would be worse than absent. + +**The first version named one thing and exercised another**, found by the PR #92 +review. It pointed `declared` at `Basis`, a **struct**, so serde answers +`invalid type … expected struct Basis` — which reaches only the +`split_once → None` branch. The `starts_with("unknown variant")` and +`names.len() > 1` refusals were exercised by nothing, and all three panics shared +the literal `"serde no longer says"`, so `expected = …` could not discriminate +which had fired. **A `should_panic` whose message matches several panics is a +check that cannot say what it caught.** + +The parse is now `parse_expected(&str)`, a seam fed hand-written mutated +messages, each refusal carrying its own wording: + +- no `expected one of` clause → `serde stopped listing the variants` +- an `unknown field` list rather than a variant list → `serde stopped saying + \`unknown variant\`` +- a list that collapses to one name → `the variant list did not parse` + +plus a happy-path case, so the three refusals are not the only evidence the +parser runs at all. `a_type_that_is_not_an_externally_tagged_enum_is_refused` +keeps the end-to-end shape through a real `Deserialize`. ### render::tests::a_frame_that_lost_its_origin_does_not_claim_to_lead_the_axis diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 149deda2..ae9c0bb6 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -2379,7 +2379,12 @@ mod tests { v.set_axis("_Measure", Axis::Column); v.hide_item("_Measure", "Cost"); } - _ => v.set_axis("_Measure", Axis::Column), + // Explicitly None, which `add_category` does NOT default to: this + // is the only view whose `none_cats` differ, and the point of it. + _ => { + v.set_axis("Product", Axis::None); + v.set_axis("_Measure", Axis::Column); + } } // All three prune. Turning it OFF in one view is plenty of differing // config and proves nothing — t6db's first, passing, formulation. @@ -2399,13 +2404,19 @@ mod tests { let mut wb = Workbook::new("Totals"); wb.add_category("Region").unwrap(); - wb.add_item("_Measure", "Revenue").unwrap(); - for (region, value) in [("North", 1.0), ("South", 20.0), ("East", 300000.0)] { + // A ONE-CHARACTER measure name and a total that CANCELS to 1: neither the + // header nor the total can supply the width — the reviewer of PR #92. + wb.add_item("_Measure", "R").unwrap(); + for (region, value) in [ + ("North", 888888888.0), + ("South", -888888888.0), + ("East", 1.0), + ] { wb.add_item("Region", region).unwrap(); wb.model.set_cell( CellKey::new(vec![ ("Region".to_string(), region.to_string()), - ("_Measure".to_string(), "Revenue".to_string()), + ("_Measure".to_string(), "R".to_string()), ]), CellValue::Number(value), ); @@ -2425,19 +2436,20 @@ mod tests { ); assert_eq!( one_row.totals, - vec![300021.0], + vec![1.0], "a column total sums every row, including the ones scrolled past" ); + assert_eq!( + one_row.col_widths, + vec![11], + "the width is `-888888888` plus a gap — a row OUTSIDE the window, and \ + a character wider than the one inside it, so the sign is the witness" + ); assert_eq!( one_row.col_widths, compute_col_widths(model, &app.layout, false, 0), "the keypress path and the projection must size a column the same" ); - assert!( - one_row.col_widths[0] > "300021".len() as u16, - "and the width must have been decided by a row OUTSIDE the window, \ - or neither assertion above is discriminating" - ); } /// improvise-t6db's general property: which view the **host** happens to be on @@ -2469,10 +2481,21 @@ mod tests { below is between two answers that had nothing to disagree about" ); + let none_cats: std::collections::HashMap> = names + .iter() + .map(|n| (n.clone(), app.model_state.workbook.views[n].none_cats())) + .collect(); + let mut compared = 0usize; for host in &names { app.switch_to_view(host); app.rebuild_layout(); for (target, expected) in names.iter().zip(&authoritative) { + // The exception is a BUG and not a carve-out — improvise-ac2z, + // pinned by the characterization test below. + if none_cats[host] != none_cats[target] { + continue; + } + compared += (host != target) as usize; assert_eq!( &grid_in(&app, target, full), expected, @@ -2480,6 +2503,41 @@ mod tests { ); } } + assert!( + compared > 0, + "every pair was skipped, so this asserted nothing at all" + ); + } + + /// improvise-ac2z, found by correcting this fixture: the host's `formula_cache` + /// leaks into another view's grid. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_hosts_none_cats_leak_into_another_views_formula_columns() { + let mut app = app_with_three_views_and_an_empty_row(); + let full = full_viewport(); + + app.switch_to_view("Second"); + app.rebuild_layout(); + let authoritative = grid_in(&app, "Second", full); + assert!( + authoritative + .col_frame + .items() + .any(|levels| levels.contains(&"Total".to_string())), + "precondition: Second really does draw the formula's columns" + ); + + app.switch_to_view("Default"); + app.rebuild_layout(); + let elsewhere = grid_in(&app, "Second", full); + assert!( + !elsewhere + .col_frame + .items() + .any(|levels| levels.contains(&"Total".to_string())), + "IF THIS FAILS, improvise-ac2z IS FIXED: delete this test and drop \ + the none_cats guard from the property above" + ); } /// The commands that renumber the grid. A **shared** table: the cursor audience diff --git a/crates/improvise-protocol/src/variants.rs b/crates/improvise-protocol/src/variants.rs index 532c9f6c..3087ade7 100644 --- a/crates/improvise-protocol/src/variants.rs +++ b/crates/improvise-protocol/src/variants.rs @@ -14,12 +14,18 @@ pub fn declared() -> Vec { .err() .expect("a name no variant has must fail to deserialize") .to_string(); + parse_expected(&message) +} + +/// The parse, separated from the deserialization so each of its three refusals +/// can be exercised — §1, the PR #92 review. Panics rather than returning []. +fn parse_expected(message: &str) -> Vec { let (head, tail) = message .split_once(", expected one of ") - .unwrap_or_else(|| panic!("serde no longer says `unknown variant`: {message}")); + .unwrap_or_else(|| panic!("serde stopped listing the variants: {message}")); assert!( head.starts_with("unknown variant"), - "serde no longer says `unknown variant`: {message}" + "serde stopped saying `unknown variant`: {message}" ); let names: Vec = tail .split(", ") @@ -70,14 +76,44 @@ pub fn cycle_is_total(walked: &[T], what: &str) #[cfg(test)] mod tests { - /// The helper must refuse a message it did not parse rather than report an - /// empty list, which would pass every cycle. `context/code/protocol.md` §1. + /// A real `Deserialize` that is not an enum: the whole helper, end to end. + /// `context/code/protocol.md` §1. #[test] - #[should_panic(expected = "serde no longer says")] - fn a_message_that_is_not_an_unknown_variant_is_refused() { + #[should_panic(expected = "serde stopped listing the variants")] + fn a_type_that_is_not_an_externally_tagged_enum_is_refused() { super::declared::(); } + /// Each refusal reached by its own message, because all three would + /// otherwise be an empty list — and an empty list passes every cycle. + #[test] + #[should_panic(expected = "serde stopped listing the variants")] + fn a_message_with_no_expected_clause_is_refused() { + super::parse_expected("invalid type: string, expected a struct"); + } + + #[test] + #[should_panic(expected = "serde stopped saying `unknown variant`")] + fn a_message_that_lists_something_other_than_variants_is_refused() { + super::parse_expected("unknown field `x`, expected one of `a`, `b`"); + } + + #[test] + #[should_panic(expected = "the variant list did not parse")] + fn a_list_that_collapses_to_one_name_is_refused() { + super::parse_expected("unknown variant `z`, expected one of `Only`"); + } + + /// And the happy path, so the three refusals are not the only evidence the + /// parser runs at all. + #[test] + fn a_well_formed_message_yields_its_names() { + assert_eq!( + super::parse_expected("unknown variant `z`, expected one of `A`, `B` at line 1"), + vec!["A".to_string(), "B".to_string()] + ); + } + /// Both externally tagged shapes name themselves, so a struct variant is /// not silently skipped by [`super::cycle_is_total`]. #[test]