From a06f50560d6ea6bb323c284f73d4785ffb8325f5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:04:03 -0700 Subject: [PATCH 01/28] The forward-step test pins its key through resolve, not by name composed() dispatches on the command name and ignores the event, so the ',' it was given pinned nothing: rebinding view-forward to '~' left the test green (improvise-3ij0). resolve() looks the key up in the shared keymap first, so the same rival now fails at that assertion. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 19 ++++++++++++++++--- crates/improvise-protocol/src/resolve.rs | 4 ++-- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index fd39ad65..0a804b57 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -3278,9 +3278,22 @@ over `Ctrl-R` is in `context/code/tui.md`), the test is renamed, and the second half now asserts the binding **exists** — with `view-back` kept as the control, which now guards the opposite failure: a table that binds everything, or a sweep that reports names it never read. A third assertion pins the key itself, -`composed(&state, ev(Char(',')), "view-forward", &[]) == [Command::ViewForward]`, -so the binding cannot drift onto some other key while the presence check stays -happy. +`resolve(&state, ev(Char(','))) == Local([Command::ViewForward])`, so the +binding cannot drift onto some other key while the presence check stays happy. + +**That third assertion was vacuous as first written, and the paragraph above +said it was not** (improvise-3ij0). It called `composed(&state, ev(Char(',')), +"view-forward", &[])`, and `composed` dispatches on the command **name**: the +`view-forward` arm never reads the event, so the `,` was decoration and the +assertion restated the first half of the test. Rebinding `view-forward` from `,` +to `~` in `keymap.rs` left it green; the key was pinned only by +`ui::app::tests::the_view_history_walks_both_ways_from_the_keyboard`, in another +crate. It now goes through `resolve`, which looks the key up in the shared +keymap before anything reads a name — so the rival fails **here**, at this +assertion (resolve.rs, "and the key is `,`"), and the pre-fix code under the +same rival was run and passes. The general shape: a helper that takes a key +*and* a name, and dispatches on the name, cannot pin the key; only the lookup +can. **The rival: bind `,` to `view-back` instead** — the misspelling the original control was written against, and the one a copy-paste actually produces. It diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0bece83a..947c1f91 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -2428,8 +2428,8 @@ mod tests { not vacuous over a table that binds everything or nothing" ); assert_eq!( - composed(&state, ev(KeyCode::Char(',')), "view-forward", &[]), - Some(vec![Command::ViewForward]), + resolve(&state, ev(KeyCode::Char(','))), + KeyOutcome::Local(vec![Command::ViewForward]), "and the key is `,` — the unshifted twin of `<`, so both halves of \ the history sit on one physical key" ); From 6bdd3ad7cf185c818575cfd03e9adb6ce91ca9d0 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:05:20 -0700 Subject: [PATCH 02/28] WIP wk18: failing test and prefixed key lookup Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/help.rs | 19 ++++++++++++++++++ crates/improvise-protocol/src/keymap.rs | 26 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 8143ec92..24039e17 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -886,6 +886,25 @@ mod tests { ); } + /// improvise-wk18: a key the pages name is looked up, never typed. The only + /// hand-written row left is an ex-command. See `context/test-evidence/protocol.md`. + #[test] + fn no_help_row_spells_a_key_by_hand() { + let spelled: Vec<&str> = pages() + .iter() + .flat_map(|page| page.lines.iter()) + .filter_map(|line| match line { + HelpLine::Literal { keys, .. } => Some(*keys), + _ => None, + }) + .filter(|keys| !keys.trim_start().starts_with(':')) + .collect(); + assert!( + spelled.is_empty(), + "these rows name keys the keymap never saw: {spelled:?}" + ); + } + /// `compact`'s three shapes, including the one no shipped mode reaches: /// keys that are all chords leave nothing to draw. #[test] diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index a375048b..6b7aef43 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -235,6 +235,32 @@ impl Keymap { keys } + /// Every two-key sequence here or in a parent that runs `command`: a + /// prefix key, then a key of the sub-keymap it opens (flat, gotcha 13). + pub fn prefixed_keys_running( + &self, + command: &str, + args: Option<&[String]>, + ) -> Vec<[(KeyCode, KeyModifiers); 2]> { + let mut keys: Vec<[(KeyCode, KeyModifiers); 2]> = self + .bindings + .iter() + .filter_map(|(pattern, binding)| match (pattern, binding) { + (KeyPattern::Key(code, mods), Binding::Prefix(sub)) => Some((*code, *mods, sub)), + _ => None, + }) + .flat_map(|(code, mods, sub)| { + sub.keys_running(command, args) + .into_iter() + .map(move |second| [(code, mods), second]) + }) + .collect(); + if let Some(parent) = &self.parent { + keys.extend(parent.prefixed_keys_running(command, args)); + } + keys + } + /// Look up the binding for a key in this keymap's own bindings. fn lookup_local(&self, key: KeyCode, mods: KeyModifiers) -> Option<&Binding> { self.bindings From 740511e0f96acf52cbf72634802be627fc07e24e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:13:40 -0700 Subject: [PATCH 03/28] Help rows name no key by hand, and the guard hears what advertised keys do wk18: gg/yy/ZZ and the page tip are keymap lookups (prefixed sequences included); ex-command rows can only be built by ex!, whose colon is its own. help_bindings() records every row the pages ask for, from the same builders. 3t5x: every_advertised_command_answers_in_its_own_mode walks the hint line AND the help pages, judges each key by what moved (a lone status is Declines, a disarm alone is Silent), and names every key that does not do what it says. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/help.rs | 176 +++++++++++------ crates/improvise-protocol/src/resolve.rs | 235 +++++++++++++++++++---- 2 files changed, 310 insertions(+), 101 deletions(-) diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index d3e63fb4..3fcb38bd 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -310,8 +310,8 @@ pub enum HelpLine { keys: Vec, description: &'static str, }, - /// A row naming something the keymap does not own: an ex-command, or a - /// prefix sequence, which is two addresses and not one binding. + /// An ex-command, which the keymap does not own. Built only by `ex!`, whose + /// colon is what stops a key being typed here (improvise-wk18). Literal { keys: &'static str, description: &'static str, @@ -330,35 +330,60 @@ pub struct HelpPage { /// row is dropped, so the page never advertises a key that does not exist. fn keys( mode: &ModeKey, - command: &str, + command: &'static str, args: Option<&[&str]>, description: &'static str, ) -> Option { let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); - let found = keys_for(mode, command, args.as_deref()); - match found.is_empty() { + let mut labels: Vec = keys_for(mode, command, args.as_deref()) + .iter() + .map(|(c, m)| key_label(c, *m)) + .collect(); + labels.extend(prefixed_labels(mode, command, args.as_deref())); + match labels.is_empty() { true => None, false => Some(HelpLine::Keys { - keys: found.iter().map(|(c, m)| key_label(c, *m)).collect(), + keys: labels, description, }), } } -fn literal(keys: &'static str, description: &'static str) -> HelpLine { - HelpLine::Literal { - keys, - description, - dim: false, +/// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap. +fn prefixed_labels(mode: &ModeKey, command: &str, args: Option<&[String]>) -> Vec { + let mut labels: Vec = match Keymaps::shared().get(mode) { + Some(keymap) => keymap.prefixed_keys_running(command, args), + None => Vec::new(), } + .iter() + .map(|[(c1, m1), (c2, m2)]| format!("{}{}", key_label(c1, *m1), key_label(c2, *m2))) + .collect(); + labels.sort(); + labels.dedup(); + labels } -fn dim(keys: &'static str, description: &'static str) -> HelpLine { - HelpLine::Literal { - keys, - description, - dim: true, - } +/// How a page asks for a keys row. `pages` answers with [`keys`]; a test +/// answers by recording the question, so a guard reads commands, not text. +type KeysRow<'a> = + &'a dyn Fn(&ModeKey, &'static str, Option<&[&str]>, &'static str) -> Option; + +/// An ex-command row. The leading colon is the macro's, not the caller's. +macro_rules! ex { + ($text:literal, $description:expr $(,)?) => { + HelpLine::Literal { + keys: concat!(":", $text), + description: $description, + dim: false, + } + }; + (dim $text:literal, $description:expr $(,)?) => { + HelpLine::Literal { + keys: concat!(" :", $text), + description: $description, + dim: true, + } + }; } /// The five pages, in order. `HELP_PAGE_COUNT` is their count and the bound @@ -367,30 +392,48 @@ pub fn pages() -> Vec { let pages = vec![ HelpPage { title: "Welcome", - lines: welcome(), + lines: welcome(&keys), }, HelpPage { title: "Navigation", - lines: navigation(), + lines: navigation(&keys), }, HelpPage { title: "Editing", - lines: editing(), + lines: editing(&keys), }, HelpPage { title: "Panels & Views", - lines: panels(), + lines: panels(&keys), }, HelpPage { title: "Commands", - lines: commands(), + lines: commands(&keys), }, ]; debug_assert_eq!(pages.len(), HELP_PAGE_COUNT); pages } -fn welcome() -> Vec { +/// Every keys row the pages ask for, as `(mode, command, args, description)` +/// — the same builders, so a guard over it cannot drift from what is drawn. +#[cfg(test)] +pub(crate) fn help_bindings() -> Vec<(ModeKey, &'static str, Option>, &'static str)> { + let asked = std::cell::RefCell::new(Vec::new()); + let record: KeysRow = &|mode, command, args, description| { + let args = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); + asked + .borrow_mut() + .push((mode.clone(), command, args, description)); + None + }; + for build in [welcome, navigation, editing, panels, commands] { + build(record); + } + asked.into_inner() +} + +fn welcome(keys: KeysRow) -> Vec { vec![ HelpLine::Blank, HelpLine::Banner(" improvise"), @@ -428,11 +471,18 @@ fn welcome() -> Vec { HelpLine::Text(" Tile The row/column/page assignment of a category."), HelpLine::Text(" Formula A computed item: derives its value from other items."), HelpLine::Blank, - dim("Tip: press Tab or l/n to go to the next page.", ""), ] + .into_iter() + .chain(keys( + &ModeKey::Help, + "help-page-next", + None, + "Next page of this help", + )) + .collect() } -fn navigation() -> Vec { +fn navigation(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; [ Some(HelpLine::Blank), @@ -445,7 +495,7 @@ fn navigation() -> Vec { None, "Move down, wrapping to the next column", ), - Some(literal("gg", "Jump to first row")), + keys(n, "jump-first-row", None, "Jump to first row"), keys(n, "jump-last-row", None, "Jump to last row"), keys(n, "jump-first-col", None, "Jump to first column"), keys(n, "jump-last-col", None, "Jump to last column"), @@ -512,7 +562,7 @@ fn navigation() -> Vec { .collect() } -fn editing() -> Vec { +fn editing(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; let e = &ModeKey::Editing; [ @@ -555,7 +605,7 @@ fn editing() -> Vec { Some(HelpLine::Blank), Some(HelpLine::Heading("Copy and paste")), Some(HelpLine::Blank), - Some(literal("yy", "Yank (copy) the current cell value")), + keys(n, "yank", None, "Yank (copy) the current cell value"), keys( n, "paste", @@ -593,7 +643,7 @@ fn editing() -> Vec { .collect() } -fn panels() -> Vec { +fn panels(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; [ Some(HelpLine::Blank), @@ -741,14 +791,14 @@ fn panels() -> Vec { "Toggle collapse of nearest group above cursor", ), keys(n, "hide-selected-row-item", None, "Hide current row item"), - Some(dim(" :show-item ", "Restore a hidden item")), + Some(ex!(dim "show-item ", "Restore a hidden item")), ] .into_iter() .flatten() .collect() } -fn commands() -> Vec { +fn commands(keys: KeysRow) -> Vec { let n = &ModeKey::Normal; [ Some(HelpLine::Blank), @@ -763,55 +813,40 @@ fn commands() -> Vec { Some(HelpLine::Blank), Some(HelpLine::Heading("File operations")), Some(HelpLine::Blank), - Some(literal( - ":w [path]", - "Save (path optional after first save)", - )), - Some(literal(":wq", "Save and quit")), - Some(literal(":q", "Quit (warns if unsaved changes)")), - Some(literal(":q!", "Force quit without saving")), - Some(literal("ZZ", "Save and quit (same as :wq)")), + Some(ex!("w [path]", "Save (path optional after first save)",)), + Some(ex!("wq", "Save and quit")), + Some(ex!("q", "Quit (warns if unsaved changes)")), + Some(ex!("q!", "Force quit without saving")), + keys(n, "wq", None, "Save and quit (same as :wq)"), keys(n, "save", None, "Save (same as :w)"), Some(HelpLine::Blank), Some(HelpLine::Heading("Import and export")), Some(HelpLine::Blank), - Some(literal(":import ", "Open the JSON/CSV import wizard")), - Some(literal( - ":export [path.csv]", - "Export the active view to CSV", - )), + Some(ex!("import ", "Open the JSON/CSV import wizard")), + Some(ex!("export [path.csv]", "Export the active view to CSV",)), Some(HelpLine::Blank), Some(HelpLine::Heading("Model building")), Some(HelpLine::Blank), - Some(literal(":add-cat ", "Add a new category")), - Some(literal( - ":add-item ", - "Add one item to a category", - )), - Some(literal( - ":add-items a b c ...", + Some(ex!("add-cat ", "Add a new category")), + Some(ex!("add-item ", "Add one item to a category",)), + Some(ex!( + "add-items a b c ...", "Add multiple items at once", )), - Some(literal( - ":formula \" = \"", - "Add a formula", - )), + Some(ex!("formula \" = \"", "Add a formula",)), Some(HelpLine::Blank), Some(HelpLine::Heading("Views")), Some(HelpLine::Blank), - Some(literal(":add-view [name]", "Create a new view")), + Some(ex!("add-view [name]", "Create a new view")), Some(HelpLine::Blank), Some(HelpLine::Heading("Display")), Some(HelpLine::Blank), - Some(literal( - ":set-format ", - "Set number format (e.g. ',.2')", - )), + Some(ex!("set-format ", "Set number format (e.g. ',.2')",)), Some(HelpLine::Blank), Some(HelpLine::Heading("Other keys")), Some(HelpLine::Blank), keys(n, "enter-mode", Some(&["help"]), "Open this help screen"), - Some(literal(":help", "Open this help screen")), + Some(ex!("help", "Open this help screen")), ] .into_iter() .flatten() @@ -915,6 +950,25 @@ mod tests { ); } + /// A row whose lookup finds no key is dropped silently, so a broken lookup + /// looks like a shorter page. See `context/test-evidence/protocol.md`. + #[test] + fn every_keys_row_the_pages_ask_for_finds_its_keys() { + let asked = help_bindings(); + assert!(asked.len() > 20, "only {} rows asked", asked.len()); + let dropped: Vec = asked + .iter() + .filter(|(mode, command, args, description)| { + let args: Option> = args + .as_ref() + .map(|a| a.iter().map(String::as_str).collect()); + keys(mode, command, args.as_deref(), description).is_none() + }) + .map(|(mode, command, _, _)| format!("{mode:?} {command}")) + .collect(); + assert!(dropped.is_empty(), "rows with no key: {dropped:?}"); + } + /// `compact`'s three shapes, including the one no shipped mode reaches: /// keys that are all chords leave nothing to draw. #[test] diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index fb253c49..99bda7e0 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -2045,7 +2045,20 @@ mod tests { /// A client whose every widget has something in it, so an `Upstream` names /// an **unhandled command** and never a state that said no (improvise-rmvx). fn furnished(mode: AppMode) -> ClientState { - let mut state = client_on(record_cell()); + furnished_on(mode, record_cell()) + } + + /// `furnished`, with `cell` under the cursor. A history in both directions + /// and a yanked value, so no `Cmd` is refused by an empty stack (3t5x). + fn furnished_on(mode: AppMode, cell: CellId) -> ClientState { + let mut state = client_on(cell); + let frame = |name: &str| crate::ViewFrame { + view_name: name.into(), + mode: AppMode::Normal, + }; + state.view.view_back_stack = vec![frame("Second")]; + state.view.view_forward_stack = vec![frame("Third")]; + state.view.yanked = Some(CellValue::Number(5.0)); // A records MODE over a non-records GRID is a state no host can // produce, so the fixture projects both (improvise-e5pf). crate::WidgetCache::apply(&mut state.grid, &GridUpdate::Records(mode.is_records())); @@ -2105,59 +2118,201 @@ mod tests { state } - /// improvise-rmvx. An advertised key that resolves upstream is a black hole - /// (gotcha 47). See `context/test-evidence/clients.md`. + /// What a user sees an advertised key do. A status alone is `Declines`: + /// honest, but not what the caption promised (improvise-3t5x). + #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] + enum Heard { + Acts, + Declines, + Silent, + } + + /// Judged by what moved, never by the variant: a chord's second key is + /// `Local` even when all it does is disarm the prefix. + fn heard(outcome: KeyOutcome) -> Heard { + let commands = match outcome { + KeyOutcome::Upstream(_) | KeyOutcome::Ignored => return Heard::Silent, + KeyOutcome::Request(_) => return Heard::Acts, + KeyOutcome::Local(commands) => commands, + }; + let said = commands.iter().any(|c| matches!(c, Command::SetStatus(_))); + let did = commands + .iter() + .any(|c| !matches!(c, Command::SetStatus(_) | Command::SetTransientPrefix(None))); + match (did, said) { + (true, _) => Heard::Acts, + (false, true) => Heard::Declines, + (false, false) => Heard::Silent, + } + } + + /// Every `(mode, search, command, args)` either table advertises: the + /// status-bar hint and the help pages (improvise-wk18). + fn advertised_everywhere() -> Vec<(AppMode, bool, &'static str, Option>)> { + let mut rows = Vec::new(); + for mode in AppMode::all() { + for (command, args, _) in crate::help::advertised(&mode) { + rows.push((mode.clone(), false, command, args)); + } + } + for (key, command, args, _) in crate::help::help_bindings() { + let (mode, search) = match key { + ModeKey::SearchMode => (AppMode::Normal, true), + key => match AppMode::all() + .into_iter() + .find(|m| ModeKey::from_app_mode(m, false).as_ref() == Some(&key)) + { + Some(mode) => (mode, false), + None => panic!("a help row names {key:?}, which no mode reaches"), + }, + }; + rows.push((mode, search, command, args)); + } + rows.sort_by_key(|(m, s, c, a)| (crate::mode_label(m), *s, *c, a.clone())); + rows.dedup(); + rows + } + + /// Every key sequence that runs `command` in `keymap`: one key, or a + /// prefix and the key after it. + fn sequences_running( + keymap: &Keymap, + command: &str, + args: Option<&[String]>, + ) -> Vec> { + let one = keymap + .keys_running(command, args) + .into_iter() + .map(|(code, mods)| vec![KeyEvent::new(code, mods)]); + let two = keymap + .prefixed_keys_running(command, args) + .into_iter() + .map(|pair| pair.map(|(code, mods)| KeyEvent::new(code, mods)).to_vec()); + one.chain(two).collect() + } + + /// What pressing `sequence` in `state` does, arming any prefix first. + fn press(mut state: ClientState, sequence: &[KeyEvent]) -> Heard { + let (last, prefix) = sequence.split_last().expect("a sequence has a key"); + for key in prefix { + if let KeyOutcome::Local(commands) = resolve(&state, *key) { + commands.iter().for_each(|c| state.fold(c)); + } + } + heard(resolve(&state, *last)) + } + + /// improvise-rmvx, wk18, 3t5x. Every advertised key does what it says, or + /// is named below saying why not. See `context/test-evidence/clients.md`. #[test] fn every_advertised_command_answers_in_its_own_mode() { - let mut unanswered = Vec::new(); - for mode in AppMode::all() { - let Some(mode_key) = ModeKey::from_app_mode(&mode, false) else { - continue; + let aggregate = CellId::Aggregate( + serde_json::from_str("\"aa4c8e5e9cbd578e0d388f4b2b5a7879\"").unwrap(), + ); + let derived = CellId::Derived { + target: "Profit".into(), + }; + // (mode, cell, command, heard) -> the keys that heard it. + let mut short: std::collections::BTreeMap<(String, &str, Heard), Vec> = + Default::default(); + for (mode, search, command, args) in advertised_everywhere() { + let key = ModeKey::from_app_mode(&mode, search).expect("a keyed mode"); + let keymap = Keymaps::shared().get(&key).expect("a keymap"); + let at = |cell: &CellId| { + let mut state = furnished_on(mode.clone(), cell.clone()); + state.view.search_mode = search; + state }; - let Some(keymap) = Keymaps::shared().get(&mode_key) else { - continue; - }; - let state = furnished(mode.clone()); - for (command, args, caption) in crate::help::advertised(&mode) { - for (code, mods) in keymap.keys_running(command, args.as_deref()) { - let event = KeyEvent::new(code, mods); - // BOTH silent variants. To a user `Ignored` is the same - // black hole as `Upstream` — nothing is on the screen. - let answered = !matches!( - resolve(&state, event), - KeyOutcome::Upstream(_) | KeyOutcome::Ignored - ); - if !answered { - unanswered.push(format!( - "{}: {:?} → `{command} {args:?}` ({caption})", - crate::mode_label(&mode), - code - )); + let label = crate::mode_label(&mode).to_string() + if search { " SEARCH" } else { "" }; + for sequence in sequences_running(keymap, command, args.as_deref()) { + let keys: String = sequence + .iter() + .map(|k| crate::keymap::key_label(&k.code, k.modifiers)) + .collect(); + let on_record = press(at(&record_cell()), &sequence); + let mut report = vec![(label.clone(), on_record)]; + // Only a grid mode stands on another kind of cell, and a cell + // matters only where it changes the answer. + if mode == AppMode::Normal { + for (on, cell) in [ + (" on an aggregate", &aggregate), + (" on a derived cell", &derived), + ] { + let got = press(at(cell), &sequence); + if got != on_record { + report.push((label.clone() + on, got)); + } } } + for (where_, got) in report.into_iter().filter(|(_, got)| *got != Heard::Acts) { + short + .entry((where_, command, got)) + .or_default() + .push(keys.clone()); + } } } - unanswered.sort(); - unanswered.dedup(); - // The five that remain, each with the reason it is not a black hole. - // Closing one has to come here and say so; opening a sixth fails. - let expected = [ + let short: Vec = short + .into_iter() + .map(|((at, command, got), mut keys)| { + keys.sort(); + keys.dedup(); + format!("{at}: {} {command} {got:?}", keys.join("/")) + }) + .collect(); + // Closing one has to come here and say so; adding one fails. A caption + // is a promise, and each line here is a promise a browser does not keep. + let expected: &[&str] = &[ // Reached only from a mode `declined` refuses at the door, so a - // browser is never standing in front of this hint. - "COMMAND: Enter → `execute-command None` (run)", - "EXPORT: Enter → `commit-export None` (export)", - "SAVE AS: Enter → `commit-save None` (save)", + // browser is never standing in front of these. + "COMMAND: Enter execute-command Silent", + "EXPORT: Enter commit-export Silent", + "SAVE AS: Enter commit-save Silent", + "NORMAL SEARCH: Enter/Esc exit-search-mode Silent", + // Commands the browser has no arm for: upstream, so a black hole + // (improvise-uln5). Each is also in the host-only list below. + "NORMAL: Enter enter-advance Silent", + "NORMAL: H hide-selected-row-item Silent", + "NORMAL: / search Silent", + "NORMAL: n search-navigate Silent", + "NORMAL: z toggle-group-under-cursor Silent", + // Honestly unavailable, and saying so in `declined`'s words. + "FORMULAS: Del/d delete-formula-at-cursor Declines", + "NORMAL: > drill-into-cell Declines", + "NORMAL: : enter-mode Declines", + "NORMAL: P toggle-prune-empty Declines", + "NORMAL: R toggle-records-mode Declines", + "NORMAL: ZZ wq Declines", + "RECORDS: o add-record-row Declines", + "RECORDS: a/i edit-or-drill Declines", + "RECORDS: : enter-mode Declines", + "RECORDS: P toggle-prune-empty Declines", + "RECORDS: R toggle-records-mode Declines", + // `i` on an aggregate IS drill in the TUI (improvise-s8h7), which + // the browser lacks; on a derived cell the TUI edits a shadowed + // cell and the browser names the formula instead (improvise-ir7l). + "NORMAL on an aggregate: a/i edit-or-drill Declines", + "NORMAL on a derived cell: a/i edit-or-drill Declines", + // No record under the cursor to write. The TUI writes onto the + // aggregate's partial key, which is its own bug (improvise-l4mp). + "NORMAL on an aggregate: p paste Silent", + "NORMAL on an aggregate: x clear-cell Silent", + "NORMAL on a derived cell: p paste Silent", + "NORMAL on a derived cell: x clear-cell Silent", // A records cell commit needs `record_col` (gotcha 1); only the two // creating columns resolve, and this fixture is on a record cell. - "RECORDS INSERT: Enter → `commit-cell-edit None` (commit)", - "RECORDS INSERT: Tab → `commit-and-advance-right None` (commit+right)", + "RECORDS INSERT: Tab commit-and-advance-right Silent", + "RECORDS INSERT: Enter commit-cell-edit Silent", ]; let mut expected: Vec = expected.iter().map(|s| s.to_string()).collect(); expected.sort(); + let mut short = short; + short.sort(); assert_eq!( - unanswered, expected, - "advertised and unanswerable: anything here that is not in the \ - named list above reads as broken rather than as unavailable" + short, + expected, + "advertised keys that do not do what they say, in full:\n{}", + short.join("\n") ); } From 299ade52d4a46efccd122a19cb46a94dc72f505a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:14:35 -0700 Subject: [PATCH 04/28] The edit-caption test pins Enter's row by command, not by phrase (jhdf) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/help.rs | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 3fcb38bd..ed01af8b 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -1071,16 +1071,23 @@ mod tests { /// See `context/test-evidence/clients.md`. #[test] fn only_the_edit_key_is_captioned_as_editing_the_cell() { - let edit = labels_of("edit-or-drill", Some(&["editing".to_string()])); - let claims: Vec<_> = rows() + let normal: Vec<_> = help_bindings() .into_iter() - .filter(|(_, d)| d.contains("Edit current cell")) + .filter(|(mode, ..)| *mode == ModeKey::Normal) .collect(); - assert!(!claims.is_empty(), "the check needs a row to check"); - for (keys, description) in claims { + // By COMMAND, not phrase (improvise-jhdf): any wording of "edit" on a + // Normal row that is not edit-or-drill's is the lie s8h7 removed. + let liars: Vec<_> = normal + .iter() + .filter(|(_, command, _, d)| { + d.to_lowercase().contains("edit") && *command != "edit-or-drill" + }) + .collect(); + assert!(liars.is_empty(), "captioned as editing but do not: {liars:?}"); + for command in ["edit-or-drill", "enter-advance"] { assert!( - keys.iter().all(|k| edit.contains(k)), - "{keys:?} is captioned {description:?} but only {edit:?} edit" + normal.iter().any(|(_, c, ..)| *c == command), + "{command} has no Normal help row, so the check above cannot see it" ); } } From 3cf37187647d875500b7f3c816d01a9315d35f2c Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:15:17 -0700 Subject: [PATCH 05/28] Evidence for wk18, 3t5x and jhdf Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 55 +++++++++++++++++++++++++++++++ context/test-evidence/protocol.md | 32 ++++++++++++++++++ 2 files changed, 87 insertions(+) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 684a84ef..07045b49 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1458,6 +1458,17 @@ is prose, and nothing checks it against the command it documents. That is a neighbour of improvise-wk18, not the same gap — wk18 is about `literal()` rows whose *keys* go unchecked; here the keys were right and the words were wrong. +**improvise-jhdf closed the paraphrase, and the paragraph above is what it +found.** Rewording Enter's caption to "Edit the cell (same as i)" — the same lie +in new words — passed both help tests. The test now asks by **command**, which +wk18's `help::help_bindings()` made possible: every Normal-mode help row whose +caption says "edit" in any case must run `edit-or-drill`. It fails on that +rival naming `(Normal, "enter-advance", None, "Edit the cell (same as i)")`. +Two controls stop it passing by looking away: `edit-or-drill` and +`enter-advance` must each still have a Normal row, or the filter has nothing to +catch. What it still cannot see is a false caption that avoids the word — "Opens +the cell" — which is the honest limit of any check over prose. + ### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked improvise-iioi, and it is the **same trap** as the category click one level @@ -2071,6 +2082,50 @@ mode `declined` refuses at the door, so a browser never stands in front of the hint; the other two are improvise-gcvb's records-cell commit, which needs `record_col`. +**improvise-3t5x and improvise-wk18 widened it again, in two directions, and the +list above is superseded by the one in the test.** + +*What "answers" means.* It counted any `Local` as answered, so a lone +`SetStatus` refusal passed — which is why improvise-s8h7's false "Edit current +cell" stayed green. Injected to show it: make `edit_or_drill`'s `Record` arm +return only `SetStatus("RIVAL")`, so `i` refuses on every cell — the old guard +**passed**. The line drawn now: a caption is a promise, so an advertised key +must *act*. A refusal is legitimate — a feature a browser honestly lacks should +say so rather than go silent — but it is not what the caption says, so it has to +be **named** in the list with its reason, like a black hole. The test therefore +sorts every key into three outcomes, `Acts`, `Declines` (said something, did +nothing) and `Silent`, and lists the last two. The same injection now fails +naming `NORMAL: a/i edit-or-drill Declines`. + +*Judged by what moved, not by the variant.* The first version of the new +classifier called anything but an all-status `Local` an action, and a rival that +unhooked `jump-first-row` from `composed` **passed** — because a chord's second +key always returns `Local([SetTransientPrefix(None), ..])`, the disarm, even +when nothing else happens. That is the laundering this entry already warns +about, one level down: `KeyOutcome::of` launders an empty vec into `Ignored`, +and `chord` launders a no-op into a `Local`. The disarm is now ignored when +deciding whether a key did anything, and the same rival fails naming `NORMAL: +gg jump-first-row Silent`. + +*Which keys.* It read `help::advertised`, the status-bar table, and the help +overlay is a second body of advertising the browser also paints +(improvise-uln5). It now also reads `help::help_bindings()`, which records every +keys row the page builders ask for — the same builders `pages()` renders, so the +two cannot drift — including prefix sequences (`gg`, `yy`, `ZZ`), which it +presses by arming the prefix first. That is what added the five host-only +black holes (`/`, `n`, `H`, `z`, Normal's `Enter`) to the list: each was already +in `the_command_names_that_still_need_the_host_are_these`, and the help page +advertised it anyway. + +*Which cells.* `furnished` stood on a record cell only, so `i` on an aggregate +(s8h7's whole subject) was outside its reach. Normal mode is now pressed on a +record, an aggregate and a derived cell, and a non-record cell is reported only +where it changes the answer, so the list does not triple. `furnished_on` also +carries a history in both directions and a yanked value, so `<`, `,` and `p` +are not refused by an empty stack — state refusals are correct behaviour and +not this test's subject, the same argument that gave `furnished` three of +everything. + ### a_locally_committed_creating_column_agrees_with_the_host improvise-gcvb. `resolve`'s commit returned `None` for the whole of records diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index e0c49021..f22fc469 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -969,3 +969,35 @@ That assertion is the discriminating one: a fold arm that removed the key would give `None` and fail. Asserting only that `"command"` survived would pass under either behaviour, which is what the old spelling amounted to for the cleared key. + +### help::tests::no_help_row_spells_a_key_by_hand + +improvise-wk18. The help pages had one kind of row whose keys the keymap never +saw: `literal("gg", …)`, `literal("yy", …)`, `literal("ZZ", …)`, and a dim tip +reading "press Tab or l/n". Every other key row comes off `keys_for`, so a +rebinding reaches it; these would have gone on saying `gg` after `g g` stopped +meaning anything, and no guard could see them because a string carries no +command name. At HEAD the test failed naming all four. + +The fix is structural rather than a second scanner. The three prefix rows are +ordinary `keys(..)` rows now — `keys` also asks `Keymap::prefixed_keys_running` +for two-key sequences — and the tip is a `keys` row in `ModeKey::Help`. +`literal()` and `dim()` are gone; the only way left to build a +`HelpLine::Literal` is the `ex!` macro, which writes the leading colon itself, so +what remains hand-written is ex-command text — checked against the parser by +`every_ex_command_the_help_pages_show_is_one_the_parser_accepts`, which reads +exactly the text a user would type. This test is what stops a direct +`HelpLine::Literal { keys: "gg", .. }` coming back past the macro. + +### help::tests::every_keys_row_the_pages_ask_for_finds_its_keys + +A `keys` row whose lookup finds nothing is **dropped**, by design — a page never +advertises a key that does not exist. The cost of that design is that a broken +lookup looks like a shorter page: `every_help_page_key_row_names_keys_that_exist` +only checks the rows that survived, plus a floor of twenty. So this asks the +question from the other end, over `help_bindings()` — every row the builders +*asked for* — and requires each to find a key. + +The injection: comment out the `prefixed_labels` extension in `keys`. The three +prefix rows vanish from the page, every other help test stays green, and this +fails naming `Normal jump-first-row`, `Normal yank` and `Normal wq`. From bdb01b876321981cf8a21310e2a49067695e8eeb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:15:41 -0700 Subject: [PATCH 06/28] context: the prefix rows are derived; the pages expose what they ask for Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/cli.md | 3 ++- context/code/protocol.md | 34 +++++++++++++++++++++++++++------- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/context/code/cli.md b/context/code/cli.md index e1c20ca3..dbaae8c1 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -101,7 +101,8 @@ The message states the **shape** rather than the arity, because ## 6 — the help's ex-commands were the unguarded surface `help.rs` derives `HelpLine::Keys` rows from the keymap and drops any row whose -command does not resolve, so a key row cannot go stale. `HelpLine::Literal` and +command does not resolve, so a key row cannot go stale. `HelpLine::Literal` (an +ex-command only, since improvise-wk18: built by `ex!`) and `HelpLine::Accent` rows are hand-written prose and had nothing checking them — which is how all three documented spellings of `:formula` came to be ones the parser refuses. diff --git a/context/code/protocol.md b/context/code/protocol.md index 7b797818..29525cff 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1833,14 +1833,34 @@ nothing in the table to look up, and its hint is a hand-written tail. That is a real hole and it is where it is visible, not papered over: a keymap cannot describe keys a command resolves privately. -**`gg`, `yy`, `ZZ` and the ex-commands.** A prefix sequence is two addresses and -not one binding (gotcha 13), and `:q`/`:w` are parsed text rather than keys. -Both are `HelpLine::Literal`, which says out loud that the row is spelled rather -than derived. +**The ex-commands.** `:q`/`:w` are parsed text rather than keys, so they are +`HelpLine::Literal` — built only by the `ex!` macro, whose leading colon is its +own — and the parser, not the keymap, is what checks them +(`every_ex_command_the_help_pages_show_is_one_the_parser_accepts`). -**The captions.** `i:edit` where `i` entered records mode would still pass every -test here. A caption is English beside a command name and only a reader judges -it — the same limit gotcha 14 recorded for the old hint test. +This paragraph used to say the same of `gg`, `yy` and `ZZ`: "a prefix sequence +is two addresses and not one binding". Two addresses is true and was no reason +to spell them by hand — `Keymap::prefixed_keys_running` looks the pair up the +way `keys_running` looks up one key, and `keys()` asks both (improvise-wk18). A +key string with no command name was also the one row no guard could reach. + +**Every row the pages ask for is visible to a guard.** The page builders take +the `keys` lookup as a parameter; `pages()` passes the real one, and +`help_bindings()` (tests only) passes one that records `(mode, command, args, +description)`. So a check over the help overlay reads the *same builders* the +overlay is drawn from, never a list kept beside them. +`resolve::tests::every_advertised_command_answers_in_its_own_mode` walks it +beside `advertised`, which is how the browser's black holes on the help page +became visible (improvise-uln5). + +**The captions.** `i:edit` where `i` entered records mode would still pass most +tests here: a caption is English beside a command name and only a reader judges +it — the same limit gotcha 14 recorded for the old hint test. Two checks now +bite at its edges. The browser guard requires an advertised key to *act*, so a +key whose caption promises an action and which only refuses must be named +(improvise-3t5x); and `only_the_edit_key_is_captioned_as_editing_the_cell` asks +by command that no other Normal row claims to edit (improvise-jhdf). A false +caption in words neither looks for still passes. ### The browser half From 3d03d9ff8f690cba94b43efdab534073216ea170 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:19:44 -0700 Subject: [PATCH 07/28] fmt, and the comment cap Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/help.rs | 5 ++++- crates/improvise-protocol/src/resolve.rs | 5 ++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index ed01af8b..878b8f88 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -1083,7 +1083,10 @@ mod tests { d.to_lowercase().contains("edit") && *command != "edit-or-drill" }) .collect(); - assert!(liars.is_empty(), "captioned as editing but do not: {liars:?}"); + assert!( + liars.is_empty(), + "captioned as editing but do not: {liars:?}" + ); for command in ["edit-or-drill", "enter-advance"] { assert!( normal.iter().any(|(_, c, ..)| *c == command), diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 99bda7e0..829a3a8d 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -2288,9 +2288,8 @@ mod tests { "RECORDS: : enter-mode Declines", "RECORDS: P toggle-prune-empty Declines", "RECORDS: R toggle-records-mode Declines", - // `i` on an aggregate IS drill in the TUI (improvise-s8h7), which - // the browser lacks; on a derived cell the TUI edits a shadowed - // cell and the browser names the formula instead (improvise-ir7l). + // `i` on an aggregate IS drill, which the browser lacks (s8h7); on a + // derived cell the browser names the formula instead (ir7l). "NORMAL on an aggregate: a/i edit-or-drill Declines", "NORMAL on a derived cell: a/i edit-or-drill Declines", // No record under the cursor to write. The TUI writes onto the From 60dbe970b0528eeb99c2f3559e9c00b8eb9895b0 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:37:55 -0700 Subject: [PATCH 08/28] hzjo: the determinism law, over every command and independent contexts Every registered command, under every argument list that builds it, must give equal effects in four contexts built independently from the shipped demo. Separately built Apps give each HashMap its own RandomState, so a command that follows hash order disagrees with itself; the old law ran execute twice on one context and could not fail. A companion test keeps that property honest, and the exemption list is checked both ways. Reasoning: context/code/command.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 43 ++++ context/test-evidence/command.md | 44 ++++ .../src/command/cmd/determinism.rs | 188 ++++++++++++++++++ .../improvise-command/src/command/cmd/mod.rs | 3 + .../src/command/cmd/registry.rs | 4 +- 5 files changed, 280 insertions(+), 2 deletions(-) create mode 100644 crates/improvise-command/src/command/cmd/determinism.rs diff --git a/context/code/command.md b/context/code/command.md index 987df348..7be2849d 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3076,3 +3076,46 @@ and every entry, so *any* append reframes and throws the cursor to the origin although nothing was renumbered. Placing the cursor masks that at this call site. Distinguishing "rows appended" from "rows renumbered" is a larger question and does not belong in a P1 data-loss fix. + +## A command is a function of its context — the determinism law (improvise-hzjo) + +`cmd/determinism.rs` holds the law the old `law_toggle_group_involution` claimed +and could not state. For **every** registered command and every argument list +that builds it, the effects `execute` returns must be equal, by `Debug` text, +across contexts built **independently from equal data**: four `App`s, each +parsed from `web/demo.improv` and given the same buffers, expanded categories, +yank, panels and cursor. + +**Why the instances must be distinct.** The old test called `execute` twice +against one `CmdContext`. Every `HashMap` it reached was the same instance with +the same `RandomState`, so it iterated the same way both times: `f(x) == f(x)`, +which nothing can fail. Separately built `App`s give every map its own seed. So a +command whose effects follow hash iteration order disagrees with itself. That is +the one kind of nondeterminism this repo has actually shipped (gotcha 18: the +drill snapshot in hash order, the analyzer's `HashSet`). The companion test +`only_distinct_instances_can_see_a_command_that_follows_hash_order` keeps that +property checked: a test-only command that emits `ctx.buffers` in iteration +order must agree with itself on one context and disagree across two. + +**Four instances, not two.** A rival that takes *the first* entry of a map, not +all of them, agrees across two seeds whenever both happen to put the same entry +first. With 21 buffers that is roughly one chance in 21, and three comparisons +make it negligible. The fixture's values are also chosen to stay distinct under +the edits commands make. A first version used `v0`…`v15`, which `pop()` collapses +to `v`, and so it let `command-mode-backspace` reading an arbitrary buffer pass. + +**What it cannot see.** Only commands and arguments the fixture reaches. The +registry's `candidate_args` are extended with names the demo really has, because +coordinates that match nothing never reach the records a drill or a fit would +order. A command whose nondeterminism needs some other state, such as records +mode, a wizard or a drill in progress, is outside this fixture. And `Debug` +equality is text equality: two effects that differ only in something their +`Debug` omits are equal here. + +**The exemption list.** `OUTSIDE_THE_CONTEXT` names commands that legitimately +depend on something outside the context, such as a clock or an entropy source, +each with its reason. It is empty: `fit-measure`'s seed is an argument with a +fixed default, not a clock read. The list is checked in both directions. An +unlisted command that diverges fails the test, and so does a listed one that +does not diverge. A stale entry therefore cannot linger, and a new registration +is covered the day it lands. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index ebfbb841..0342fc2d 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4972,3 +4972,47 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +## `command/cmd/determinism.rs` — the determinism law over every command (improvise-hzjo) + +The reasoning is in `context/code/command.md`, "A command is a function of its +context". This entry records what each test can see. + +### every_command_gives_equal_effects_in_independently_built_equal_contexts + +It runs all 89 registrations, 1500+ (command, args) pairs, in four contexts built +from four separately parsed copies of `web/demo.improv`, and requires equal +effect `Debug` text. It passed at `ae96b8c` with no exemptions. It is not a +regression test for a live bug. It restores a law, and the evidence is which +rivals it catches: + +- **`command-mode-backspace` reading `ctx.buffers.values().next()`** instead of + the `command` buffer: a real command following hash order. Injected, full + workspace run: this test is the **only** failure, because every + `text_buffer.rs` test builds a one-entry buffer map. With the comparison + switched to one reused context (`compare(&a, from_ref(&a))`) the same rival + **passes**. That is the half the ticket required: the distinct construction + is what does the work. +- **`DataStore::matching_positions` without its sort**: gotcha 18 re-shipped. + Injected, full workspace run: this test fails, reached through + `drill-into-cell` and `fit-measure`'s record order, alongside the three + `cell.rs` order tests that pin the store directly. +- **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. + It fails as "listed … but did not". + +**A vacuity this test itself had on its first version.** The fixture's filler +buffers held `v0`…`v15`. The `command-mode-backspace` rival pops a character, +so every value collapsed to `v` (or `v1`), and the rival passed across two +distinct contexts. This is the "two derivations the fixture makes agree" shape. +The values are now `value {i} of 16`. The comparison was also widened from one +other instance to three, because a take-the-first rival agrees across two seeds +about one time in 21. + +### only_distinct_instances_can_see_a_command_that_follows_hash_order + +The check-the-checker behind the law. A test-only `EmitsInHashOrder` emits one +`SetBuffer` per `ctx.buffers` entry in iteration order. It must equal itself on +one context, which is why the old single-context law was vacuous, and differ +across two independently built ones. If a future refactor made the fixture +share one map, or share one `RandomState`, the second assertion fails. Without +this test the law would then pass everything and look healthy. diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs new file mode 100644 index 00000000..7565c9ee --- /dev/null +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -0,0 +1,188 @@ +//! A command is a function of its context: two contexts built independently from +//! equal data give equal effects — `context/code/command.md`, improvise-hzjo. + +use std::collections::BTreeSet; + +use super::core::{Cmd, CmdContext}; +use super::registry::tests::candidate_args; +use crate::command::key::{KeyCode, KeyModifiers}; +use crate::model::cell::CellValue; +use crate::ui::app::App; +use crate::ui::effect::{self, Effect}; + +/// Commands whose effects depend on something outside the context, with why. +const OUTSIDE_THE_CONTEXT: &[(&str, &str)] = &[]; + +const DOCUMENT: &str = include_str!("../../../../../web/demo.improv"); + +/// One `App` over the shipped demo. Every context map holds several entries that +/// stay distinct under an edit like `pop()` — see the evidence. +fn fixture() -> App { + let workbook = crate::persistence::parse_md(DOCUMENT).expect("the demo parses"); + let mut app = App::new(workbook, None); + let vs = &mut app.view_state; + for i in 0..16 { + vs.buffers + .insert(format!("b{i}"), format!("value {i} of 16")); + } + for (name, text) in [ + ("edit", "42"), + ("formula", "Margin = Profit / Revenue"), + ("category", "Region"), + ("command", "add-item Product Mocha"), + ("export", "/tmp/improvise-hzjo.csv"), + ] { + vs.buffers.insert(name.into(), text.into()); + } + for cat in ["Product", "Quarter", "_Measure", "_Index", "_Dim"] { + vs.expanded_cats.insert(cat.into()); + } + vs.yanked = Some(CellValue::Number(7.0)); + vs.formula_panel_open = true; + vs.category_panel_open = true; + vs.view_panel_open = true; + vs.cat_panel_cursor = 2; + vs.view_panel_cursor = 1; + app.cursor_mut().selected = (1, 1); + app +} + +/// The registry's candidates, plus names the demo really has: a drill or a fit +/// over coordinates that match nothing never reaches the records it would order. +fn args_the_document_names() -> Vec> { + let mut all = candidate_args(); + all.extend( + [ + vec!["Revenue"], + vec!["Product/Latte", "Quarter/Q2"], + vec!["Product/Latte", "Quarter/Q2", "_Measure/Cost"], + vec![ + "editing", + "5", + "Product/Latte", + "Quarter/Q2", + "_Measure/Cost", + ], + ] + .into_iter() + .map(|v| v.into_iter().map(String::from).collect()), + ); + all +} + +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { + let cmd = ctx.registry.interactive(name, args, ctx).ok()?; + Some(format!("{:?}", cmd.execute(ctx))) +} + +/// What running every registered command under `a` and under each of `others` found. +#[derive(Default)] +struct Comparison { + diverged: BTreeSet<&'static str>, + shown: Vec, + never_built: Vec<&'static str>, + compared: usize, +} + +fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { + let mut found = Comparison::default(); + for name in a.registry.names().collect::>() { + let before = found.compared; + for args in args_the_document_names() { + let Some(left) = effects_of(name, &args, a) else { + continue; + }; + found.compared += 1; + for b in others { + let right = effects_of(name, &args, b).unwrap_or_default(); + if left != right { + found.diverged.insert(name); + found + .shown + .push(format!("{name} {args:?}:\n {left}\n {right}")); + } + } + } + if found.compared == before { + found.never_built.push(name); + } + } + found +} + +/// Evidence: `context/test-evidence/command.md`. +#[test] +fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { + let apps: Vec = (0..4).map(|_| fixture()).collect(); + let contexts: Vec = apps + .iter() + .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) + .collect(); + let found = compare(&contexts[0], &contexts[1..]); + assert!( + found.never_built.is_empty(), + "no candidate args build these in the fixture, so the law never ran them: {:?}", + found.never_built + ); + let listed: BTreeSet<&str> = OUTSIDE_THE_CONTEXT.iter().map(|(n, _)| *n).collect(); + let unlisted: Vec<&str> = found + .shown + .iter() + .filter(|s| !listed.iter().any(|n| s.starts_with(&format!("{n} ")))) + .map(String::as_str) + .collect(); + assert!( + unlisted.is_empty(), + "effects depend on more than the context ({} pairs compared):\n{}", + found.compared, + unlisted.join("\n") + ); + let stale: Vec<&&str> = listed + .iter() + .filter(|n| !found.diverged.contains(**n)) + .collect(); + assert!( + stale.is_empty(), + "listed as depending on something outside the context, but did not: {stale:?}" + ); +} + +/// Iterates a context `HashMap` and emits in that order: the one nondeterminism +/// this repo has shipped (gotcha 18). The law must see it; a reused context cannot. +#[derive(Debug)] +struct EmitsInHashOrder; +impl Cmd for EmitsInHashOrder { + fn name(&self) -> &'static str { + "emits-in-hash-order" + } + fn execute(&self, ctx: &CmdContext) -> Vec> { + ctx.buffers + .iter() + .map(|(name, value)| { + Box::new(effect::SetBuffer { + name: name.clone(), + value: value.clone(), + }) as Box + }) + .collect() + } +} + +/// Evidence: `context/test-evidence/command.md`. +#[test] +fn only_distinct_instances_can_see_a_command_that_follows_hash_order() { + let (first, second) = (fixture(), fixture()); + let a = first.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let b = second.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let run = |ctx: &CmdContext| format!("{:?}", EmitsInHashOrder.execute(ctx)); + assert_eq!( + run(&a), + run(&a), + "one reused context iterates its maps the same way twice" + ); + assert_ne!( + run(&a), + run(&b), + "two independently built contexts must iterate differently, or the law is vacuous" + ); +} diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index 0000d0fd..cf74c0c2 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -13,6 +13,9 @@ pub mod text_buffer; pub mod tile; pub mod wizard; +#[cfg(test)] +mod determinism; + // Re-export items used by external code pub use self::core::{Cmd, CmdContext, CmdRegistry}; pub use registry::default_registry; diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 6ef243dc..8503beaa 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -562,13 +562,13 @@ pub fn default_registry() -> CmdRegistry { } #[cfg(test)] -mod tests { +pub(super) mod tests { use super::*; use crate::command::cmd::test_helpers::*; /// Argument vectors to try when constructing a command, most specific /// first. A command is exercised by the first one its parser accepts. - fn candidate_args() -> Vec> { + pub(in crate::command::cmd) fn candidate_args() -> Vec> { [ vec![], vec!["1"], From e6d7d13e901c27cff13524f01ba75a39ef0c913a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:40:17 -0700 Subject: [PATCH 09/28] The advertised-key guard judges by what the fold moved, off every edge (6562) heard() compared command types, so a clamped move counted as acting. It now folds the outcome and compares the view slice (status and disarm aside); a Model command counts as sent. The fixture is a 3x3 grid with the cursor at (1,1), three tiles with the cursor at 1, help page 1. Direction is out of reach for any did-it-move check, so every grid key is also pressed from the centre and must land where its binding says. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/resolve.rs | 116 ++++++++++++++++++++--- 1 file changed, 101 insertions(+), 15 deletions(-) diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index ed774f2f..8ee97edf 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -2039,6 +2039,35 @@ mod tests { // ── what is bound and what is not ─────────────────────────────── + /// A three-by-three grid with `id` at (1,1) and the cursor on it: no move + /// from there is clamped by an edge (improvise-6562). + fn centred_on(id: CellId) -> ClientState { + let mut centre = CellDisplay::number("1200"); + centre.id = Some(id); + let mut cells: Vec<_> = (0..3) + .flat_map(|r| (0..3).map(move |c| ((r, c), CellDisplay::number("7")))) + .collect(); + cells[4].1 = centre; + let mut client = ClientState::default(); + client.bootstrap(Snapshot { + view: ViewState::default(), + seed: vec![ + Projection::Grid(GridUpdate::RowFrame(AxisFrame::flat([ + "North", "South", "East", + ]))), + Projection::Grid(GridUpdate::ColFrame(AxisFrame::flat([ + "Revenue", "Cost", "Profit", + ]))), + Projection::Grid(GridUpdate::Cells(cells)), + Projection::Grid(GridUpdate::Extent(GridExtent { rows: 3, cols: 3 })), + Projection::Basis(published()), + ], + }); + client.window = (3, 3); + client.view.cursor_mut().selected = (1, 1); + client + } + /// A client whose every widget has something in it, so an `Upstream` names /// an **unhandled command** and never a state that said no (improvise-rmvx). fn furnished(mode: AppMode) -> ClientState { @@ -2048,7 +2077,7 @@ mod tests { /// `furnished`, with `cell` under the cursor. A history in both directions /// and a yanked value, so no `Cmd` is refused by an empty stack (3t5x). fn furnished_on(mode: AppMode, cell: CellId) -> ClientState { - let mut state = client_on(cell); + let mut state = centred_on(cell); let frame = |name: &str| crate::ViewFrame { view_name: name.into(), mode: AppMode::Normal, @@ -2062,8 +2091,8 @@ mod tests { state.view.mode = mode; state.grid.page_coords = vec![crate::PageAxis { cat: "Quarter".into(), - selected: "Q1".into(), - items: Some(vec!["Q1".into(), "Q2".into()]), + selected: "Q2".into(), + items: Some(vec!["Q1".into(), "Q2".into(), "Q3".into()]), }]; // THREE of everything, every panel cursor seated in the MIDDLE: an edge // makes `move-panel-cursor` answer `Ignored` for the wrong reason. @@ -2095,10 +2124,15 @@ mod tests { axes: Vec::new(), }) .collect(); - state.tiles.tiles = vec![crate::TileEntry { - category: "Region".into(), - axis: Axis::Row, - }]; + state.tiles.tiles = ["Region", "Product", "Quarter"] + .into_iter() + .map(|category| crate::TileEntry { + category: category.into(), + axis: Axis::Row, + }) + .collect(); + state.view.tile_cat_idx = 1; + state.view.help_page = 1; state.view.cat_panel_cursor = 1; state.view.formula_cursor = 1; state.view.view_panel_cursor = 1; @@ -2124,19 +2158,24 @@ mod tests { Silent, } - /// Judged by what moved, never by the variant: a chord's second key is - /// `Local` even when all it does is disarm the prefix. - fn heard(outcome: KeyOutcome) -> Heard { + /// Judged by what the fold MOVED, never by a command's type: a clamped move + /// or a lone disarm changes nothing (improvise-6562). `Model` is sent. + fn heard(state: &mut ClientState, outcome: KeyOutcome) -> Heard { let commands = match outcome { KeyOutcome::Upstream(_) | KeyOutcome::Ignored => return Heard::Silent, KeyOutcome::Request(_) => return Heard::Acts, KeyOutcome::Local(commands) => commands, }; + let quiet = |view: &ViewState| ViewState { + status_msg: String::new(), + transient_prefix: None, + ..view.clone() + }; + let before = quiet(&state.view); + commands.iter().for_each(|c| state.fold(c)); + let sent = commands.iter().any(|c| matches!(c, Command::Model { .. })); let said = commands.iter().any(|c| matches!(c, Command::SetStatus(_))); - let did = commands - .iter() - .any(|c| !matches!(c, Command::SetStatus(_) | Command::SetTransientPrefix(None))); - match (did, said) { + match (sent || quiet(&state.view) != before, said) { (true, _) => Heard::Acts, (false, true) => Heard::Declines, (false, false) => Heard::Silent, @@ -2196,7 +2235,8 @@ mod tests { commands.iter().for_each(|c| state.fold(c)); } } - heard(resolve(&state, *last)) + let outcome = resolve(&state, *last); + heard(&mut state, outcome) } /// improvise-rmvx, wk18, 3t5x. Every advertised key does what it says, or @@ -2312,6 +2352,52 @@ mod tests { ); } + /// improvise-6562: "it moved" cannot see a move the WRONG way, so every grid + /// key is pressed from the centre and must land where it says. Evidence file. + #[test] + fn every_grid_move_lands_where_its_binding_says() { + let keymap = Keymaps::shared().get(&ModeKey::Normal).expect("Normal"); + let mut moves = Vec::new(); + bound_commands(keymap, &mut moves); + // A move's destination is its own args; a jump's is its name. + let mut expected: Vec<(String, Vec, (usize, usize))> = moves + .into_iter() + .filter(|(name, _)| name == "move-selection") + .map(|(name, args)| { + let d = |i: usize| args[i].parse::().expect("a delta"); + let to = ((1 + d(0)) as usize, (1 + d(1)) as usize); + (name, args, to) + }) + .collect(); + for (name, to) in [ + ("jump-first-row", (0, 1)), + ("jump-last-row", (2, 1)), + ("jump-first-col", (1, 0)), + ("jump-last-col", (1, 2)), + ] { + expected.push((name.to_string(), Vec::new(), to)); + } + let mut pressed = 0; + for (name, args, to) in expected { + let wanted = (!args.is_empty()).then_some(args.as_slice()); + for sequence in sequences_running(keymap, &name, wanted) { + let mut state = furnished(AppMode::Normal); + for key in &sequence { + if let KeyOutcome::Local(commands) = resolve(&state, *key) { + commands.iter().for_each(|c| state.fold(c)); + } + } + assert_eq!( + state.view.cursor().selected, + to, + "{sequence:?} runs `{name} {args:?}` from (1,1)" + ); + pressed += 1; + } + } + assert!(pressed >= 14, "only {pressed} keys pressed; the walk went blind"); + } + /// improvise-rmvx. `step` checks `declined` first, so a name answered /// there makes an arm in `composed` UNREACHABLE. Evidence file. #[test] From ea1f2d313b7759fe47f4b0cc6c8fb25889b4c080 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:10 -0700 Subject: [PATCH 10/28] improvise-lrrx/8nd9: view history does not survive a document swap install_workbook clears view_back_stack and view_forward_stack beside cursors, so `<` after LoadModel or AcceptWizardModel no longer walks the outgoing document's views. ViewBack and ViewForward now observe the switch as SwitchView does (improvise-swzk): a frame the document refuses is consumed, leaves no frame behind, and restores no mode. Each fix holds without the other. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 22 ++- context/test-evidence/command.md | 39 +++++ crates/improvise-command/src/ui/app.rs | 3 + crates/improvise-command/src/ui/effect.rs | 179 ++++++++++++++++++++-- 4 files changed, 221 insertions(+), 22 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 987df348..fc111c79 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -721,12 +721,22 @@ is gone rather than joined by a second one. A guard spelled here as copy of `switch_to_view`'s own condition, free to disagree with it the day that condition grows a clause. -Not fixed, and reachable only by construction: `ViewBack` and `ViewForward` call -`switch_to_view` just as unguardedly, pushing the current view onto the opposite -stack before the switch they cannot be sure of. They are safe **because -`ViewState::forget_view` purges both stacks when a view is deleted**, so a popped -frame always names a live view — a guard living one level away from the hazard, -which is the shape that keeps returning here. +`ViewBack` and `ViewForward` have the same shape now (improvise-8nd9). They used to +push the current view onto the opposite stack **before** a switch they could not be +sure of, and were safe only because `ViewState::forget_view` purges both stacks when +a view is deleted — a guard living one level away from the hazard. improvise-lrrx +proved the guard incomplete: `App::install_workbook` (`LoadModel`, +`AcceptWizardModel`) cleared `cursors` and not the history, so `<` after a load +walked the **outgoing** document's frames, landing on a view of the same name in the +incoming one that this client had never visited, or on a refused switch that still +left a phantom frame behind. Both halves are fixed, so neither relies on the other: +`install_workbook` clears both stacks beside `cursors`, for the same reason, and the +two steps go through `stepped_to`, which switches, observes whether `active_view` +moved, and only then restores the frame's mode and hands back the frame just left. +A frame the document refuses is consumed and leaves nothing. `reduce_view`'s +`stepped` is unchanged: the client holds no `Workbook` to observe against, and its +stacks cannot hold a dead name — `ForgetView` purges them and a load is a fresh +`Snapshot`. ### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..bbf5ac27 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5021,3 +5021,42 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +### loading_a_document_forgets_the_outgoing_documents_view_history + +improvise-lrrx: `App::install_workbook` cleared `cursors` but not +`view_back_stack`/`view_forward_stack`, so `<` after `LoadModel` walked the +outgoing document's views inside the incoming one. The history is two deep +(`Default`, `X`, then on `Z`), and the incoming document **has** a view named `X` +but opens on `Y`. Without that, a stale frame is a refused switch and looks the +same as "nothing happened" (the ticket's own fixture warning). Against the unfixed +code, `<` landed on `X`, a view this client never visited in the new document. +Rival injected: the two `clear()` lines removed from `install_workbook`, with the +improvise-8nd9 fix still in place. This test and its wizard twin failed at the +`view-back walked the outgoing document's history` assertion. + +### accepting_an_import_forgets_the_outgoing_documents_view_history + +improvise-lrrx, through the second door into `install_workbook`. A +wizard-built workbook has only `Default`, so the test creates an `X` in it after +accepting. Without that, the stale `X` frame would be refused and could not be +told apart from a cleared history. Same rival and same failure as the load test. + +### a_step_back_the_document_refuses_leaves_no_frame_behind + +improvise-8nd9: `ViewBack` pushed the current view onto the forward stack before +the guarded `switch_to_view`, so a frame naming a view the document lacks left a +phantom behind, and `,` would then "go forward" to where it already was. The +dead frame is pushed by hand, standing for any path that forgets +`forget_view`, with a live frame under it and the frame's mode set to `Help` so +that a restored mode is observable. Two rivals. First, the original code, which +pushes first and restores the mode unconditionally: it fails at `nothing moved, +so no mode`. Second, a push-first version that restores the mode only on a move, +so the mode assertion cannot hide the stack assertion: it fails at `a refused +step left X on the forward stack`. The last two assertions check that the live +frame beneath still works, so a fix that drops the whole stack cannot pass. + +### a_step_forward_the_document_refuses_leaves_no_frame_behind + +The `ViewForward` half of the test above, with the stacks' roles swapped. It fails +against the unfixed code at `nothing moved, so no mode`. diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 56f6fb8c..bf5151de 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -439,6 +439,9 @@ impl App { pub fn install_workbook(&mut self, workbook: Workbook) { self.view_state.active_view = workbook.active_view.clone(); self.view_state.cursors.clear(); + // The history names the outgoing document's views too (improvise-lrrx). + self.view_state.view_back_stack.clear(); + self.view_state.view_forward_stack.clear(); self.model_state.workbook = workbook; // Every client's mirror is scoped to the outgoing document too, and an // effect cannot reach the session list. The generation is how it hears. diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 62a4f6a0..494fedc1 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -354,19 +354,31 @@ impl Effect for SwitchView { } } +/// Switch to `frame`'s view and restore its mode, returning the frame just left +/// — or `None`, touching nothing, if the view did not move. +fn stepped_to(app: &mut App, frame: ViewFrame) -> Option { + let left = ViewFrame { + view_name: app.view_state.active_view.clone(), + mode: app.view_state.mode.clone(), + }; + app.switch_to_view(&frame.view_name); + (app.view_state.active_view != left.view_name).then(|| { + app.view_state.mode = frame.mode; + left + }) +} + /// Go back in view history (pop back stack, push current to forward stack). #[derive(Debug)] pub struct ViewBack; impl Effect for ViewBack { fn apply(&self, app: &mut App) { - if let Some(frame) = app.view_state.view_back_stack.pop() { - let current = app.view_state.active_view.clone(); - app.view_state.view_forward_stack.push(ViewFrame { - view_name: current, - mode: app.view_state.mode.clone(), - }); - app.switch_to_view(&frame.view_name); - app.view_state.mode = frame.mode; + // Observed, as `SwitchView` is: a frame the document refuses is dropped + // and leaves nothing behind (improvise-8nd9, `context/code/command.md`). + if let Some(frame) = app.view_state.view_back_stack.pop() + && let Some(left) = stepped_to(app, frame) + { + app.view_state.view_forward_stack.push(left); } } fn kind(&self) -> EffectKind { @@ -379,14 +391,11 @@ impl Effect for ViewBack { pub struct ViewForward; impl Effect for ViewForward { fn apply(&self, app: &mut App) { - if let Some(frame) = app.view_state.view_forward_stack.pop() { - let current = app.view_state.active_view.clone(); - app.view_state.view_back_stack.push(ViewFrame { - view_name: current, - mode: app.view_state.mode.clone(), - }); - app.switch_to_view(&frame.view_name); - app.view_state.mode = frame.mode; + // The mirror of `ViewBack`, observed the same way (improvise-8nd9). + if let Some(frame) = app.view_state.view_forward_stack.pop() + && let Some(left) = stepped_to(app, frame) + { + app.view_state.view_back_stack.push(left); } } fn kind(&self) -> EffectKind { @@ -1878,6 +1887,144 @@ mod tests { assert_eq!(app.view_state.active_view, before); } + /// A history two deep through `X` and `Z`: back is `[Default, X]`. + fn app_with_history(storage: Box) -> App { + let mut wb = Workbook::new("Outgoing"); + wb.create_view("X"); + wb.create_view("Z"); + let mut app = App::with_storage(wb, None, storage); + for name in ["X", "Z"] { + SwitchView(name.to_string()).apply(&mut app); + } + assert_eq!(app.view_state.view_back_stack.len(), 2, "setup: two deep"); + app + } + + /// Asserts `<` and `,` leave `view` put and the history empty. + fn assert_history_went_with_the_document(app: &mut App, view: &str) { + assert_eq!(app.view_state.active_view, view, "setup: opened on {view}"); + for (name, step) in [ + ("view-back", Box::new(ViewBack) as Box), + ("view-forward", Box::new(ViewForward)), + ] { + step.apply(app); + assert_eq!( + app.view_state.active_view, view, + "{name} walked the outgoing document's history in the incoming one" + ); + } + assert!(app.view_state.view_back_stack.is_empty()); + assert!(app.view_state.view_forward_stack.is_empty()); + } + + /// improvise-lrrx. Evidence: `context/test-evidence/command.md`. + #[test] + fn loading_a_document_forgets_the_outgoing_documents_view_history() { + use crate::storage::{MemoryFs, Storage}; + let fs = MemoryFs::new(); + // The incoming document HAS a view named `X`, and opens somewhere else. + let mut incoming = Workbook::new("Incoming"); + incoming.create_view("X"); + incoming.create_view("Y"); + incoming.active_view = "Y".to_string(); + let text = crate::persistence::format_md(&incoming); + fs.write(std::path::Path::new("b.improv"), text.as_bytes()) + .unwrap(); + let mut app = app_with_history(Box::new(fs)); + + LoadModel("b.improv".into()).apply(&mut app); + assert_eq!( + app.model_state.workbook.model.name, "Incoming", + "setup: loaded" + ); + assert_history_went_with_the_document(&mut app, "Y"); + } + + /// improvise-lrrx: the wizard is the other door into `install_workbook`. + #[test] + fn accepting_an_import_forgets_the_outgoing_documents_view_history() { + let mut app = app_with_history(Box::new(crate::storage::MemoryFs::new())); + let mut wizard = crate::import::wizard::ImportWizard::new(serde_json::json!([ + {"region": "East", "revenue": 100.0}, + {"region": "West", "revenue": 200.0}, + ])); + wizard.advance(); + wizard.advance(); + wizard.build(); + assert!(wizard.built_summary().is_some(), "setup: a built workbook"); + app.wizard = Some(wizard); + + AcceptWizardModel.apply(&mut app); + // The new document gains an `X` of its own, so a stale frame could land. + app.model_state.workbook.create_view("X"); + let opened = app.view_state.active_view.clone(); + assert_ne!(opened, "X", "setup: the import must not open on X"); + assert_history_went_with_the_document(&mut app, &opened); + } + + /// improvise-8nd9: a frame naming a view the document lacks is refused, and + /// a refused step leaves no frame behind. See `context/test-evidence/command.md`. + #[test] + fn a_step_back_the_document_refuses_leaves_no_frame_behind() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + // A path that forgot `forget_view`: the history names a dead view. + app.view_state.view_back_stack.push(ViewFrame { + view_name: "Gone".to_string(), + mode: AppMode::Help, + }); + + ViewBack.apply(&mut app); + assert_eq!( + app.view_state.active_view, "X", + "setup: the step is refused" + ); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "nothing moved, so no mode" + ); + assert!( + app.view_state.view_forward_stack.is_empty(), + "a refused step left X on the forward stack: `,` would go nowhere" + ); + ViewBack.apply(&mut app); + assert_eq!( + app.view_state.active_view, "Default", + "the live frame survives" + ); + assert_eq!(app.view_state.view_forward_stack.len(), 1); + } + + /// improvise-8nd9: the forward half of the step above. + #[test] + fn a_step_forward_the_document_refuses_leaves_no_frame_behind() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + ViewBack.apply(&mut app); + app.view_state.view_forward_stack.push(ViewFrame { + view_name: "Gone".to_string(), + mode: AppMode::Help, + }); + + ViewForward.apply(&mut app); + assert_eq!(app.view_state.active_view, "Default", "setup: refused"); + assert_eq!( + app.view_state.mode, + AppMode::Normal, + "nothing moved, so no mode" + ); + assert!( + app.view_state.view_back_stack.is_empty(), + "a refused step left Default on the back stack: `<` would go nowhere" + ); + ViewForward.apply(&mut app); + assert_eq!(app.view_state.active_view, "X", "the live frame survives"); + assert_eq!(app.view_state.view_back_stack.len(), 1); + } + #[test] fn create_and_delete_view() { let mut app = test_app(); From f8345ea8c6de359bb26d0334aed5bc0cee8d5ae3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:18 -0700 Subject: [PATCH 11/28] A keys row can only be looked up, and an ex row can name no key (v29m) HelpLine::Keys carries a KeyRow whose fields are private to a module whose only constructor looks the keys up; a builder that types one is a compile error. ex! asserts at compile time that every word after the command name is a placeholder, so ':q or press Q' does not build. ':add-items a b c ...' is now ':add-items ...'. Renderers read the row through accessors (src/ui/help.rs, render.rs). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-protocol/src/help.rs | 142 ++++++++++++++++----- crates/improvise-protocol/src/resolve.rs | 5 +- crates/improvise-wasm-client/src/render.rs | 6 +- src/ui/help.rs | 6 +- 4 files changed, 121 insertions(+), 38 deletions(-) diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 878b8f88..c00316b9 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -304,12 +304,9 @@ pub enum HelpLine { Heading(&'static str), Text(&'static str), Accent(&'static str), - /// A keys-and-description row whose keys came off the keymap, so it - /// cannot go stale against the bindings it documents. - Keys { - keys: Vec, - description: &'static str, - }, + /// A keys-and-description row whose keys came off the keymap. A [`KeyRow`] + /// has no public constructor, so no builder can type one (improvise-v29m). + Keys(KeyRow), /// An ex-command, which the keymap does not own. Built only by `ex!`, whose /// colon is what stops a key being typed here (improvise-wk18). Literal { @@ -326,8 +323,47 @@ pub struct HelpPage { pub lines: Vec, } -/// A keys row, looked up in `mode`. An unbound command yields no keys and the -/// row is dropped, so the page never advertises a key that does not exist. +pub use key_row::KeyRow; + +/// The one door to a keys row: its fields are private to this module, and its +/// only constructor looks the keys up. +mod key_row { + use super::{ModeKey, key_label, keys_for, prefixed_labels}; + + #[derive(Debug, Clone, PartialEq, Eq)] + pub struct KeyRow { + keys: Vec, + description: &'static str, + } + + impl KeyRow { + pub fn keys(&self) -> &[String] { + &self.keys + } + + pub fn description(&self) -> &'static str { + self.description + } + + /// An unbound command yields no keys and no row, so the page never + /// advertises a key that does not exist. + pub(super) fn looked_up( + mode: &ModeKey, + command: &str, + args: Option<&[String]>, + description: &'static str, + ) -> Option { + let mut keys: Vec = keys_for(mode, command, args) + .iter() + .map(|(c, m)| key_label(c, *m)) + .collect(); + keys.extend(prefixed_labels(mode, command, args)); + (!keys.is_empty()).then_some(Self { keys, description }) + } + } +} + +/// A keys row, looked up in `mode`. fn keys( mode: &ModeKey, command: &'static str, @@ -335,18 +371,7 @@ fn keys( description: &'static str, ) -> Option { let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); - let mut labels: Vec = keys_for(mode, command, args.as_deref()) - .iter() - .map(|(c, m)| key_label(c, *m)) - .collect(); - labels.extend(prefixed_labels(mode, command, args.as_deref())); - match labels.is_empty() { - true => None, - false => Some(HelpLine::Keys { - keys: labels, - description, - }), - } + KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys) } /// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap. @@ -368,22 +393,56 @@ fn prefixed_labels(mode: &ModeKey, command: &str, args: Option<&[String]>) -> Ve type KeysRow<'a> = &'a dyn Fn(&ModeKey, &'static str, Option<&[&str]>, &'static str) -> Option; -/// An ex-command row. The leading colon is the macro's, not the caller's. +/// An ex-command row. The colon is the macro's, and every word after the +/// command's name must be a placeholder, checked when the page compiles. macro_rules! ex { - ($text:literal, $description:expr $(,)?) => { + ($text:literal, $description:expr $(,)?) => {{ + const _: () = assert!(ex_usage_is_placeholders($text)); HelpLine::Literal { keys: concat!(":", $text), description: $description, dim: false, } - }; - (dim $text:literal, $description:expr $(,)?) => { + }}; + (dim $text:literal, $description:expr $(,)?) => {{ + const _: () = assert!(ex_usage_is_placeholders($text)); HelpLine::Literal { keys: concat!(" :", $text), description: $description, dim: true, } - }; + }}; +} + +/// After the name, every word opens with `<`, `[` or `"`, or is `...`: so an +/// ex row cannot also advertise a key ("q or press Q"). Quotes span spaces. +const fn ex_usage_is_placeholders(text: &str) -> bool { + let b = text.as_bytes(); + let mut i = 0; + while i < b.len() && b[i] != b' ' { + i += 1; + } + loop { + while i < b.len() && b[i] == b' ' { + i += 1; + } + if i == b.len() { + return true; + } + let start = i; + let mut quoted = false; + while i < b.len() && (quoted || b[i] != b' ') { + if b[i] == b'"' { + quoted = !quoted; + } + i += 1; + } + let ellipsis = + i - start == 3 && b[start] == b'.' && b[start + 1] == b'.' && b[start + 2] == b'.'; + if !(ellipsis || b[start] == b'<' || b[start] == b'[' || b[start] == b'"') { + return false; + } + } } /// The five pages, in order. `HELP_PAGE_COUNT` is their count and the bound @@ -830,7 +889,7 @@ fn commands(keys: KeysRow) -> Vec { Some(ex!("add-cat ", "Add a new category")), Some(ex!("add-item ", "Add one item to a category",)), Some(ex!( - "add-items a b c ...", + "add-items ...", "Add multiple items at once", )), Some(ex!("formula \" = \"", "Add a formula",)), @@ -919,8 +978,12 @@ mod tests { for page in &pages { assert!(!page.title.is_empty()); for line in &page.lines { - if let HelpLine::Keys { keys, description } = line { - assert!(!keys.is_empty(), "{description} resolved to no keys"); + if let HelpLine::Keys(row) = line { + assert!( + !row.keys().is_empty(), + "{} resolved to no keys", + row.description() + ); rows += 1; } } @@ -950,6 +1013,23 @@ mod tests { ); } + /// improvise-v29m: `ex!` refuses a usage with a bare word in it at compile + /// time; this pins the rule it asks. See `context/test-evidence/protocol.md`. + #[test] + fn an_ex_row_can_name_placeholders_and_nothing_else() { + for usage in [ + "w [path]", + "q!", + "formula \" = \"", + "add-items ...", + ] { + assert!(ex_usage_is_placeholders(usage), "{usage} is refused"); + } + for usage in ["q or press Q", "add-items a b c", "help ?"] { + assert!(!ex_usage_is_placeholders(usage), "{usage} is accepted"); + } + } + /// A row whose lookup finds no key is dropped silently, so a broken lookup /// looks like a shorter page. See `context/test-evidence/protocol.md`. #[test] @@ -1039,7 +1119,7 @@ mod tests { .into_iter() .flat_map(|page| page.lines) .filter_map(|line| match line { - HelpLine::Keys { keys, description } => Some((keys, description)), + HelpLine::Keys(row) => Some((row.keys().to_vec(), row.description())), _ => None, }) .collect() @@ -1107,8 +1187,8 @@ mod tests { println!("\n=== {} ===", page.title); for line in &page.lines { match line { - HelpLine::Keys { keys, description } => { - println!(" {:<28}{description}", keys.join(" ")) + HelpLine::Keys(row) => { + println!(" {:<28}{}", row.keys().join(" "), row.description()) } HelpLine::Literal { keys, description, .. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 8ee97edf..5cac1b3c 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -2395,7 +2395,10 @@ mod tests { pressed += 1; } } - assert!(pressed >= 14, "only {pressed} keys pressed; the walk went blind"); + assert!( + pressed >= 14, + "only {pressed} keys pressed; the walk went blind" + ); } /// improvise-rmvx. `step` checks `declined` first, so a name answered diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index fa48a6bd..dea97267 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -418,9 +418,9 @@ impl RenderHelpRow { L::Heading(t) => prose("heading", t), L::Text(t) => prose("text", t), L::Accent(t) => prose("accent", t), - L::Keys { keys, description } => Self { - keys: keys.join(" "), - description: description.to_string(), + L::Keys(row) => Self { + keys: row.keys().join(" "), + description: row.description().to_string(), kind: "keys", }, L::Literal { diff --git a/src/ui/help.rs b/src/ui/help.rs index 09055a17..30fffcfd 100644 --- a/src/ui/help.rs +++ b/src/ui/help.rs @@ -69,9 +69,9 @@ fn page_content(page: usize) -> Vec { HelpLine::Heading(text) => plain(text, s.heading), HelpLine::Text(text) => plain(text, s.normal), HelpLine::Accent(text) => plain(text, s.accent), - HelpLine::Keys { keys, description } => Row { - key: format!(" {}", keys.join(" ")), - desc: description, + HelpLine::Keys(row) => Row { + key: format!(" {}", row.keys().join(" ")), + desc: row.description(), style: s.key, }, HelpLine::Literal { From 327fa176add6b8cfe012d05f0fdef47aca5ce01e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:35 -0700 Subject: [PATCH 12/28] Evidence for 6562 and v29m Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 32 +++++++++++++++++++++++++++++++ context/test-evidence/protocol.md | 21 ++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 01aba0e1..5f96365b 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4458,3 +4458,35 @@ Records mode is deliberately not covered. There the TUI yanks `None` and its `p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key (improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep case there would assert the bug. + +### resolve::tests::every_grid_move_lands_where_its_binding_says + +improvise-6562, found reviewing PR #180 at 949810c, and it is the same +laundering as the disarm, one level over. `heard()` said it judged by what moved +and in fact judged by command **type**: any `Local` command besides a status or a +disarm counted as `Acts`. The fixture sat at every edge — a 2x1 grid with the +cursor at (0,0), one tile, help page 0 — so about twenty movement keys that +clamp to nothing were counted as acting. Rebinding `jump-last-col` to +`GridMove::ToStart(false)`, so `$` and `End` go to the *first* column, left the +whole workspace green. + +Two changes, because either alone is not enough. `heard()` now **folds** the +outcome and compares the view slice before and after (status and disarm aside), +counting a `Command::Model` as sent and a `Request` as an act; and `furnished_on` +stands on a 3x3 grid with the cursor at (1,1), three tiles with the cursor on +the middle one, a page axis on its middle item and help page 1, so no move is +clamped by an edge. Injecting `jump-last-col` as a `SetSelected` to where the +cursor already is now fails `every_advertised_command_answers_in_its_own_mode` +naming `NORMAL: $/End jump-last-col Silent`. + +**The reviewer's own rival still passes that guard, and should.** From (1,1), +`ToStart(false)` moves to (1,0) — the view moved, so the key "acts". No +did-anything-happen check can see *direction*; only an expected destination can. +This test supplies one without a second table: a `move-selection` binding's +expected landing is read off its own args (the deltas), and the four jumps are +named by what they are called. Every key that runs one is pressed from the +centre — fourteen of them, with a floor so a blind walk fails. Under the rival +it fails naming `End` and `$`. The general point: the fixture that made the +laundering invisible (the edge) is also the one that makes a wrong-direction +jump visible, so moving off the edge fixed one blind spot and opened the other, +and each needs its own question. diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 9207f5d5..9c5c35ea 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1061,3 +1061,24 @@ question from the other end, over `help_bindings()` — every row the builders The injection: comment out the `prefixed_labels` extension in `keys`. The three prefix rows vanish from the page, every other help test stays green, and this fails naming `Normal jump-first-row`, `Normal yank` and `Normal wq`. + +### help::tests::an_ex_row_can_name_placeholders_and_nothing_else + +improvise-v29m, found reviewing PR #180 at 949810c. `no_help_row_spells_a_key_by_hand` +read only `HelpLine::Literal`, and its premise — that `keys()` is the only way to +make a key row — was a convention: `HelpLine::Keys { keys: Vec, .. }` +was a public variant with public fields, and `help_bindings()` records only what +the builders *ask* for, so `HelpLine::Keys { keys: vec!["Q"], description: "Quit +instantly" }` typed straight into `panels()`, with `Q` bound to nothing, passed +the whole workspace. So did `ex!("q or press Q", ..)`: the parser test splits on +colons and `parse_line` accepts trailing words. + +Both are now **compile errors**, which is why this test is not where the guard +lives. `HelpLine::Keys` carries a `KeyRow` whose fields are private to a module +whose one constructor, `KeyRow::looked_up`, does the keymap lookup; the injected +row fails with E0559/E0451. `ex!` evaluates `ex_usage_is_placeholders` in a +`const` item, and the injected usage fails with `evaluation panicked: assertion +failed: ex_usage_is_placeholders("q or press Q")`. The rule cost one row: +`:add-items a b c ...` had three bare words and is now +`:add-items ...`. This test pins the rule the macro asks, in both +directions, including that shipped usage and the rival. From 544744cfd2d478fe7f9d0c19b75e53349d0cf4ab Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:43:51 -0700 Subject: [PATCH 13/28] context: the help rows' premise is a type Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/protocol.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/context/code/protocol.md b/context/code/protocol.md index 9a54b094..4103b16b 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -1951,7 +1951,11 @@ key string with no command name was also the one row no guard could reach. the `keys` lookup as a parameter; `pages()` passes the real one, and `help_bindings()` (tests only) passes one that records `(mode, command, args, description)`. So a check over the help overlay reads the *same builders* the -overlay is drawn from, never a list kept beside them. +overlay is drawn from, never a list kept beside them. That premise is a type, +not a convention (improvise-v29m): `HelpLine::Keys` carries a `KeyRow` whose +only constructor does the lookup, so a builder cannot type a key row the +recorder would never see; and `ex!` refuses at compile time any usage with a +bare word after the command name, so an ex row cannot smuggle a key either. `resolve::tests::every_advertised_command_answers_in_its_own_mode` walks it beside `advertised`, which is how the browser's black holes on the help page became visible (improvise-uln5). From 77893d830c6b783615da925399f84e112f156d6b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:44:27 -0700 Subject: [PATCH 14/28] improvise-t0ma: pin records_steps and drill_steps as steps, not as effects records_steps was guarded by one incidental persistence test. Two tests now assert the ViewStep lists directly: the records spine plus every non-index/dim category on None, and a drill's Filter-fixed coordinates plus its freed aggregated categories. The is_measure() rival fails the first; a Page-for-Filter rival fails the second. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/core.md | 23 +++++++++ crates/improvise-core/src/view/layout.rs | 62 ++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/context/test-evidence/core.md b/context/test-evidence/core.md index d27feb93..f51186c9 100644 --- a/context/test-evidence/core.md +++ b/context/test-evidence/core.md @@ -1361,3 +1361,26 @@ fold is the row `z` names the group from. Note the narrowing this implies: an **empty** collapsed group now has an empty fold row, which `prune_empty` drops, and its header goes with it. lhi4 kept that header because it was the only way back to the rows; the fold is now that way back. + +### records_steps_are_the_spine_then_every_non_index_dim_category_on_none + +improvise-t0ma. Before this, `records_steps` was guarded by one incidental test: the +`is_index_or_dim() → is_measure()` rival failed only +`save_with_unfinished_record_is_readable`, a persistence test that happens to enter +records mode. This asserts the **steps**, not the effects `cmd/grid.rs` translates +them into, because the steps are the model-side answer a browser will also consume. +The expected list filters `_Index`/`_Dim` by **name**, not by `CategoryKind`, so it +is not a second copy of the predicate under test, and the setup asserts `_Measure` +and a regular category are both present, since a model with neither could not tell +the two predicates apart. Rival: `is_measure()` in place of `is_index_or_dim()`. +This test fails, because `_Measure` is dropped and `_Index`/`_Dim` are put on `None` +after the spine that just placed them. + +### drill_steps_filter_each_coordinate_and_free_the_rest + +improvise-t0ma's companion for `drill_steps`. The fixture includes a `none_cat` that +the key also fixes (`Region`) and one that is virtual (`_Index`), so both exclusions +are exercised. It also lists the key's coordinates out of sorted order, which checks +that the steps follow `CellKey`'s canonical order. Rival: `Axis::Page` in place of +`Axis::Filter` for a fixed coordinate, which reopens improvise-dz8 (a drilled +coordinate the user could page away from). This test fails. diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 495c3967..b45766a5 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -1750,4 +1750,66 @@ mod tests { "Sep is in no group — Q2's GroupEnd closed Q1's re-opened run" ); } + + /// improvise-t0ma: the steps themselves, not their translation into effects — + /// see context/test-evidence/core.md. + #[test] + fn records_steps_are_the_spine_then_every_non_index_dim_category_on_none() { + use super::{ViewStep, records_spine, records_steps}; + let mut m = crate::model::Model::new("T"); + m.add_category("Region").unwrap(); + m.add_category("Product").unwrap(); + let none = |c: &str| ViewStep::Axis { + category: c.to_string(), + axis: Axis::None, + }; + let mut expected = records_spine(); + expected.extend( + m.categories + .keys() + .filter(|c| !matches!(c.as_str(), "_Index" | "_Dim")) + .map(|c| none(c)), + ); + assert!( + expected.contains(&none("_Measure")) && expected.contains(&none("Region")), + "setup: the model carries _Measure and a regular category" + ); + assert_eq!(records_steps(&m), expected); + } + + /// improvise-t0ma: a drill FIXES its coordinates on Filter and frees only the + /// unfixed, non-virtual aggregated categories. + #[test] + fn drill_steps_filter_each_coordinate_and_free_the_rest() { + use super::{ViewStep, drill_steps, records_spine}; + let key = CellKey::new(vec![ + ("Region".into(), "North".into()), + ("Month".into(), "Feb".into()), + ("_Measure".into(), "Revenue".into()), + ]); + let none_cats: Vec = ["Product", "Region", "_Index", "Customer"] + .map(String::from) + .to_vec(); + let fixed = |c: &str, i: &str| { + [ + ViewStep::Axis { + category: c.into(), + axis: Axis::Filter, + }, + ViewStep::PageSelection { + category: c.into(), + item: i.into(), + }, + ] + }; + let mut expected = records_spine(); + expected.extend(fixed("Month", "Feb")); + expected.extend(fixed("Region", "North")); + expected.extend(fixed("_Measure", "Revenue")); + expected.extend(["Product", "Customer"].map(|c| ViewStep::Axis { + category: c.into(), + axis: Axis::None, + })); + assert_eq!(drill_steps(&key, &none_cats), expected); + } } From 99046f7c630853432f2455112ed1198208c5b546 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:45:36 -0700 Subject: [PATCH 15/28] improvise-8nd9: a history step observes acceptance, not movement A frame naming the view you are on is still a place (view + mode), and the client's reduce_view::stepped steps onto it unconditionally. Observing "active_view changed" dropped that frame on the host and so split host from client; colon_view_back_respects_whether_history_exists caught it. App::switch_to_view now says whether the document accepted the view, and stepped_to asks that rather than re-deriving the condition. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 23 ++++++++++++----- context/test-evidence/command.md | 12 +++++++++ crates/improvise-command/src/ui/app.rs | 10 +++++--- crates/improvise-command/src/ui/effect.rs | 30 ++++++++++++++++++++--- 4 files changed, 62 insertions(+), 13 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index fc111c79..847aac2a 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -731,12 +731,23 @@ walked the **outgoing** document's frames, landing on a view of the same name in incoming one that this client had never visited, or on a refused switch that still left a phantom frame behind. Both halves are fixed, so neither relies on the other: `install_workbook` clears both stacks beside `cursors`, for the same reason, and the -two steps go through `stepped_to`, which switches, observes whether `active_view` -moved, and only then restores the frame's mode and hands back the frame just left. -A frame the document refuses is consumed and leaves nothing. `reduce_view`'s -`stepped` is unchanged: the client holds no `Workbook` to observe against, and its -stacks cannot hold a dead name — `ForgetView` purges them and a load is a fresh -`Snapshot`. +two steps go through `stepped_to`, which switches, observes whether the document +**accepted** the view (`App::switch_to_view` now returns that), and only then +restores the frame's mode and hands back the frame just left. A frame the document +refuses is consumed and leaves nothing. + +The observation is *accepted*, not *moved*, and that is the one place the sibling +shapes differ. `SwitchView` asks "did `active_view` change" because asking for the +view you are on is not a move. A history frame is a **place**, meaning a view and a +mode, so a live frame naming the view you are on still restores its mode. Such +frames exist: `forget_view` collapses `[A, B, A]` to `[A, A]`. The client's +`reduce_view::stepped` also steps onto them unconditionally. A "moved" test +would therefore drop a frame the client keeps, and the host and the client +would disagree. The first version of this fix made exactly that mistake. +`colon_view_back_respects_whether_history_exists` caught it, because its +fixture is such a frame. `reduce_view`'s `stepped` is unchanged. The client holds +no `Workbook` to observe against, and its stacks cannot hold a dead name: +`ForgetView` purges them, and a load arrives as a fresh `Snapshot`. ### `ReturnToNormalUnlessMoved` — observation, not prediction (improvise-toc) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index bbf5ac27..63bfa926 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5056,6 +5056,18 @@ so the mode assertion cannot hide the stack assertion: it fails at `a refused step left X on the forward stack`. The last two assertions check that the live frame beneath still works, so a fix that drops the whole stack cannot pass. +### a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds + +improvise-8nd9. This test holds the fix to the client's `reduce_view::stepped`. +A live frame naming the view you are on (a `forget_view` collapse leaves one) +is still a step. It restores its mode and leaves a forward frame, exactly as +the client folds it. Rival injected: `stepped_to` observing whether +`active_view` *changed* rather than whether the document *accepted* the view. +That was the first version of this fix. This test failed at the +mode assertion, as did +`command::cmd::text_buffer::tests::colon_view_back_respects_whether_history_exists`. +That test's fixture is such a frame, and it is what caught the first version. + ### a_step_forward_the_document_refuses_leaves_no_frame_behind The `ViewForward` half of the test above, with the stacks' roles swapped. It fails diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index bf5151de..faaa5277 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -564,12 +564,14 @@ impl App { .flat_map(move |r| (0..layout.col_count()).filter_map(move |c| layout.cell_key(r, c))) } - /// Point **this client's** slice at `name`, if the document has such a view. - /// The document's own default stays put (improvise-jytm, gotcha 29). - pub fn switch_to_view(&mut self, name: &str) { - if self.model_state.workbook.views.contains_key(name) { + /// Point **this client's** slice at `name`, if the document has such a view, + /// and say whether it did. The document's default stays put (gotcha 29). + pub fn switch_to_view(&mut self, name: &str) -> bool { + let accepted = self.model_state.workbook.views.contains_key(name); + if accepted { self.view_state.active_view = name.to_string(); } + accepted } /// What `name` names in this document, falling back to the default for a name diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 494fedc1..ec16e636 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -355,14 +355,13 @@ impl Effect for SwitchView { } /// Switch to `frame`'s view and restore its mode, returning the frame just left -/// — or `None`, touching nothing, if the view did not move. +/// — or `None`, touching nothing, if the document refused the view. fn stepped_to(app: &mut App, frame: ViewFrame) -> Option { let left = ViewFrame { view_name: app.view_state.active_view.clone(), mode: app.view_state.mode.clone(), }; - app.switch_to_view(&frame.view_name); - (app.view_state.active_view != left.view_name).then(|| { + app.switch_to_view(&frame.view_name).then(|| { app.view_state.mode = frame.mode; left }) @@ -1997,6 +1996,31 @@ mod tests { assert_eq!(app.view_state.view_forward_stack.len(), 1); } + /// improvise-8nd9: a live frame naming the view you are on is still a step, as + /// it is in the client's `reduce_view`. See `context/test-evidence/command.md`. + #[test] + fn a_step_back_to_the_view_you_are_on_is_the_step_the_client_folds() { + let mut app = test_app(); + app.model_state.workbook.create_view("X"); + SwitchView("X".to_string()).apply(&mut app); + app.view_state.view_back_stack.push(ViewFrame { + view_name: "X".to_string(), + mode: AppMode::Help, + }); + let mut mirror = app.view_state.clone(); + + ViewBack.apply(&mut app); + improvise_protocol::reduce_view(&mut mirror, &improvise_protocol::Command::ViewBack); + assert_eq!( + app.view_state.mode, + AppMode::Help, + "the frame's mode is restored" + ); + assert_eq!(app.view_state.view_back_stack, mirror.view_back_stack); + assert_eq!(app.view_state.view_forward_stack, mirror.view_forward_stack); + assert_eq!(app.view_state.mode, mirror.mode); + } + /// improvise-8nd9: the forward half of the step above. #[test] fn a_step_forward_the_document_refuses_leaves_no_frame_behind() { From ffd3ec80be469866b4b758a4eaebd8aec3878edc Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:46:55 -0700 Subject: [PATCH 16/28] jgzw: keep _Index/_Dim tileable, and pin the offered set by name The tile bar offering _Index and _Dim is a decision the user already made (improvise-kos), and it is the only way out of a records layout that was saved into a real view: R there emits ViewBack on an empty back stack and claims Pivot mode (filed as improvise-gj5p). A new test names the whole offered list with _Measure on a column, so a filter dropping _Measure, or one dropping _Index/_Dim, fails it. Reasoning: context/code/command.md, cmd/tile.rs. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 37 +++++++++++++++++++ context/test-evidence/command.md | 24 ++++++++++++ .../improvise-command/src/command/cmd/tile.rs | 20 ++++++++++ 3 files changed, 81 insertions(+) diff --git a/context/code/command.md b/context/code/command.md index 987df348..e656bb9a 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2770,6 +2770,43 @@ flag: tidy-up goes where an abort cannot reach it. stays in `TileSelect` mode so the user can adjust several tiles. `axis: None` cycles; `axis: Some(a)` sets. +**The tile bar offers `_Index` and `_Dim`, and that is deliberate (improvise-jgzw).** +Every tile call site passes `Model::category_names()`, virtual categories included, +so tile cursor 0 is `_Index`. The ticket asked whether that was a leak. It is not, +for three reasons, each checked rather than argued: + +- **It is a decision the user already made.** improvise-kos: "`_Dim` and `_Index` + should work as pivot axes outside Records/Drill view". A category that works as a + pivot axis is one the axis picker offers. +- **The tile bar is the only way out of a records layout that was saved.** + improvise-jgzw's "check what it does" found that hand-assigning `_Index` → Row and + `_Dim` → Column in a real view (tile bar or `:set-axis`) is **written** to the + `.improv`, because a real view persists every axis, and it reopens as records mode. + (`VIRTUAL_VIEW_NAMES`' "never-persisted `_Index`/`_Dim` on axes" is true only of + `_Records`/`_Drill`.) In that document, `R` (`toggle-records-mode`) emits + `ViewBack` on an empty back stack, so it leaves the layout as it was and sets the + status to "Pivot mode". That is filed as improvise-gj5p. Moving the two axes back through + the tile bar is what gets out. In the browser, which declines + `toggle-records-mode`, the tile bar is also the only way in, and improvise-bd9o + already made that route safe instead of closing it + (`the_tile_bar_route_into_records_mode_does_not_strand_the_user`). +- **What filtering would cost was measured.** With `View::tileable_cats` filtering + on `CategoryKind::is_index_or_dim`, that bd9o test and the TUI's + `renders_each_tile_with_its_axis_label` fail, as well as the two characterization + tests here. + +What the default cursor does cost, stated: one cycle key on tile 0 puts `_Index` +on Row alone, and in a pivot view that empties the grid (0 rows on +`web/demo.improv`). The status line says `_Index → Row`, and three more presses +undo it. If that turns out to matter, the fix is the order the tile bar lists +categories in, not taking entries out of it. + +**`_Measure` must stay tileable.** It is on an axis in every shipped document, so +`regular_category_names()`, the advice for *prompts*, is the wrong filter here. +`the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included` +names the whole offered list with `_Measure` on a column, and fails if either +decision changes. + ### `cmd/effect_cmds.rs` The parseable model-mutation commands are thin `Cmd` wrappers around effects, diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 511a8461..8c1dce99 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5021,3 +5021,27 @@ bracket-matched `#[…]` before reading the name. `an_attributed_or_wrapped_effect_is_still_an_effect` pins that on a fixture: a doc comment, two attributes (one with nested brackets), and a wrapped call. With the whitespace skip removed it fails with `[]`. + +## `command/cmd/tile.rs` — the tile bar's offered set, by name (improvise-jgzw) + +### the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included + +This test pins a decision, not a bug. The argument is in `context/code/command.md` +under `cmd/tile.rs`. Before this test, no tile assertion named the whole offered +set. The two older ones named only the category at cursor 0, so a filter that +changed what the bar offers past the cursor was invisible. The fixture puts +`_Measure` on **Column**, as every shipped document does. The test then walks the +cursor past the end and asserts the exact list +`[_Index, _Dim, _Measure, Type, Month]`. + +Rivals, injected, full workspace run: + +- **`regular_category_names()` in `TileAxisOp`**, the obvious "no virtual + categories in pickers" fix, which drops `_Measure` too. It fails this test + (`["Type", "Month"]`), the two characterization tests above it, and + `lockstep.rs::a_locally_resolved_tile_axis_change_agrees_with_the_host`. +- **`View::tileable_cats` filtering `is_index_or_dim`**, the ticket's option (b). + With the list asserted this test fails by construction, so the point is what + *else* fails: `renders_each_tile_with_its_axis_label` (TUI) and + `the_tile_bar_route_into_records_mode_does_not_strand_the_user`. The second one + is improvise-bd9o's own test of the route this decision keeps. diff --git a/crates/improvise-command/src/command/cmd/tile.rs b/crates/improvise-command/src/command/cmd/tile.rs index 1ff05418..c4a71cf9 100644 --- a/crates/improvise-command/src/command/cmd/tile.rs +++ b/crates/improvise-command/src/command/cmd/tile.rs @@ -59,6 +59,26 @@ mod tests { assert!(effects.is_empty()); } + /// improvise-jgzw: `_Index`/`_Dim` stay tileable on purpose, and `_Measure` on + /// a column must too — `context/test-evidence/command.md`. + #[test] + fn the_tile_bar_offers_every_category_by_name_measure_and_records_axes_included() { + let mut m = two_cat_model(); + m.active_view_mut().set_axis("_Measure", Axis::Column); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + let mut offered = Vec::new(); + for idx in 0..8 { + ctx.tile_cat_idx = idx; + let dbg = effects_debug(&TileAxisOp { axis: None }.execute(&ctx)); + if let Some(rest) = dbg.split("CycleAxis(Active, \"").nth(1) { + offered.push(rest.split('"').next().unwrap_or_default().to_string()); + } + } + assert_eq!(offered, ["_Index", "_Dim", "_Measure", "Type", "Month"]); + } + #[test] fn move_tile_cursor_right() { let m = two_cat_model(); From 7935b006eb0e0fde5c7d6a0595025124ac87f68a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:56:38 -0700 Subject: [PATCH 17/28] Drop the Welcome page's key tip rather than deriving it As a derived keys row it gave Welcome a row with a description, and harness/tests/help.spec.js holds Welcome to prose; CI failed on it at 544744c. Help mode's status hint already derives next-page. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/protocol.md | 8 +++++++- crates/improvise-protocol/src/help.rs | 11 +---------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 9c5c35ea..53729978 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1041,7 +1041,13 @@ command name. At HEAD the test failed naming all four. The fix is structural rather than a second scanner. The three prefix rows are ordinary `keys(..)` rows now — `keys` also asks `Keymap::prefixed_keys_running` -for two-key sequences — and the tip is a `keys` row in `ModeKey::Help`. +for two-key sequences. The tip is **gone** rather than derived: made a `keys` row +in `ModeKey::Help`, it gave the Welcome page its one row with a description, +and `harness/tests/help.spec.js` ("? opens help, and its rows carry the keys and +the words") failed in CI on `welcome.every(row => row.description === null)` — +Welcome is prose by contract. The Help mode's own status hint already derives +`next-page` from the keymap, so the literal was a second table (improvise-rr8n's +rule: keep the one derived from the thing it describes). `literal()` and `dim()` are gone; the only way left to build a `HelpLine::Literal` is the `ex!` macro, which writes the leading colon itself, so what remains hand-written is ex-command text — checked against the parser by diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index c00316b9..017bff2a 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -492,7 +492,7 @@ pub(crate) fn help_bindings() -> Vec<(ModeKey, &'static str, Option> asked.into_inner() } -fn welcome(keys: KeysRow) -> Vec { +fn welcome(_: KeysRow) -> Vec { vec![ HelpLine::Blank, HelpLine::Banner(" improvise"), @@ -529,16 +529,7 @@ fn welcome(keys: KeysRow) -> Vec { ), HelpLine::Text(" Tile The row/column/page assignment of a category."), HelpLine::Text(" Formula A computed item: derives its value from other items."), - HelpLine::Blank, ] - .into_iter() - .chain(keys( - &ModeKey::Help, - "help-page-next", - None, - "Next page of this help", - )) - .collect() } fn navigation(keys: KeysRow) -> Vec { From 1c4a27243d067a89c9cb147866754712ce0e7dcb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:07:02 -0700 Subject: [PATCH 18/28] Every grid move Normal binds must land where it says (w7qd) scroll-rows and page-scroll derive their landing from their argument as move-selection does; any bound command that moves the cursor with no stated landing fails; the numeric floor is gone. Reversed Ctrl-d/u and PgUp/PgDn, and a dropped jump table with $ sent to column 0, both fail. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 19 +++++ crates/improvise-protocol/src/resolve.rs | 92 +++++++++++++++--------- 2 files changed, 79 insertions(+), 32 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 78605e78..b0027ca9 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4505,3 +4505,22 @@ it fails naming `End` and `$`. The general point: the fixture that made the laundering invisible (the edge) is also the one that makes a wrong-direction jump visible, so moving off the edge fixed one blind spot and opened the other, and each needs its own question. + +**improvise-w7qd, and the floor was the hole.** As first written the test +covered `move-selection` and a four-entry jump table, behind `pressed >= 14`. +Two things passed the whole workspace, both found in re-review of #180 at +544744c: negating the `scroll-rows` and `page-scroll` arms, so `Ctrl-d`/`Ctrl-u` +and `PgDn`/`PgUp` all scroll the wrong way (advertised, and still "acting" from +the centre); and dropping the jump table *with* `$` sent to the first column, +because the sixteen `move-selection` sequences met the floor on their own. The +test now walks **every** command Normal binds. `scroll-rows` and `page-scroll` +state their landing from their own argument exactly as `move-selection` does — +from the centre of a 3x3 grid any step of at least one lands at `1 + d` +clamped, so a sign is visible whatever the page size — and a command that moves +the cursor (in the same view) without a stated landing **fails** rather than +being skipped. So a dropped table entry is caught by derivation, not by a count: +under the second rival it fails naming every jump as moving with nothing stating +where. The first rival fails on `PgUp` (`page-scroll ["-1"]`) and, alone, on +`Ctrl-u` (`scroll-rows ["-5"]`). A last assertion that each kind was pressed at +least once guards the other direction — a binding vanishing — and is a named +list on purpose, since that is the one thing no walk over bindings can see. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 5cac1b3c..178e3fb7 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -2357,47 +2357,75 @@ mod tests { #[test] fn every_grid_move_lands_where_its_binding_says() { let keymap = Keymaps::shared().get(&ModeKey::Normal).expect("Normal"); - let mut moves = Vec::new(); - bound_commands(keymap, &mut moves); - // A move's destination is its own args; a jump's is its name. - let mut expected: Vec<(String, Vec, (usize, usize))> = moves - .into_iter() - .filter(|(name, _)| name == "move-selection") - .map(|(name, args)| { - let d = |i: usize| args[i].parse::().expect("a delta"); - let to = ((1 + d(0)) as usize, (1 + d(1)) as usize); - (name, args, to) - }) - .collect(); - for (name, to) in [ - ("jump-first-row", (0, 1)), - ("jump-last-row", (2, 1)), - ("jump-first-col", (1, 0)), - ("jump-last-col", (1, 2)), - ] { - expected.push((name.to_string(), Vec::new(), to)); - } - let mut pressed = 0; - for (name, args, to) in expected { - let wanted = (!args.is_empty()).then_some(args.as_slice()); - for sequence in sequences_running(keymap, &name, wanted) { + let mut bound = Vec::new(); + bound_commands(keymap, &mut bound); + bound.sort(); + bound.dedup(); + // From (1,1) of a 3x3 grid any step of at least one lands at 1+d clamped, + // so a delta's sign is visible; a jump's destination is its name. + let step = |d: &String| (1 + d.parse::().expect("a delta")).clamp(0, 2) as usize; + let destination = |name: &str, args: &[String]| match name { + "move-selection" => Some((step(&args[0]), step(&args[1]))), + "scroll-rows" | "page-scroll" => Some((step(&args[0]), 1)), + "jump-first-row" => Some((0, 1)), + "jump-last-row" => Some((2, 1)), + "jump-first-col" => Some((1, 0)), + "jump-last-col" => Some((1, 2)), + _ => None, + }; + let mut unstated = Vec::new(); + let mut unpressed = Vec::new(); + let mut pressed = std::collections::BTreeSet::new(); + for (name, args) in &bound { + let to = destination(name, args); + let sequences = sequences_running(keymap, name, Some(args.as_slice())); + if to.is_some() && sequences.is_empty() { + unpressed.push(format!("{name} {args:?}")); + } + for sequence in sequences { let mut state = furnished(AppMode::Normal); + let view = state.view.active_view.clone(); for key in &sequence { if let KeyOutcome::Local(commands) = resolve(&state, *key) { commands.iter().for_each(|c| state.fold(c)); } } - assert_eq!( - state.view.cursor().selected, - to, - "{sequence:?} runs `{name} {args:?}` from (1,1)" - ); - pressed += 1; + let landed = state.view.cursor().selected; + if to.is_some() { + pressed.insert(name.as_str()); + } + match to { + Some(to) => { + assert_eq!(landed, to, "{sequence:?} runs `{name} {args:?}` from (1,1)") + } + None if state.view.active_view == view && landed != (1, 1) => { + unstated.push(format!("{name} {args:?} -> {landed:?}")) + } + None => {} + } } } assert!( - pressed >= 14, - "only {pressed} keys pressed; the walk went blind" + unpressed.is_empty(), + "stated and never pressed: {unpressed:?}" + ); + assert!( + unstated.is_empty(), + "these move the cursor and nothing states where to: {unstated:?}" + ); + let every = [ + "jump-first-col", + "jump-first-row", + "jump-last-col", + "jump-last-row", + "move-selection", + "page-scroll", + "scroll-rows", + ]; + assert_eq!( + pressed.into_iter().collect::>(), + every, + "each kind is pressed" ); } From 8772db2e7b8bb43550df61f5cbe96123ae4f7753 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:08:42 -0700 Subject: [PATCH 19/28] A keys row is drawn only if the builders asked for it (ssoe) KeyRow::looked_up is private to key_row, whose keys() is the lookup pages() hands the builders; the reviewer's direct call is E0624. A new test asserts the drawn keys rows equal help_bindings(), which catches the remaining self::keys bypass. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/protocol.md | 21 +++++++++++++++ crates/improvise-protocol/src/help.rs | 38 ++++++++++++++++++--------- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 53729978..db8c4ada 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1088,3 +1088,24 @@ failed: ex_usage_is_placeholders("q or press Q")`. The rule cost one row: `:add-items a b c ...` had three bare words and is now `:add-items ...`. This test pins the rule the macro asks, in both directions, including that shipped usage and the rival. + +### help::tests::every_keys_row_drawn_is_one_the_builders_asked_for + +improvise-ssoe, found in re-review of #180 at 544744c. `KeyRow` closed +hand-typed keys, but `KeyRow::looked_up` was `pub(super)`, so a page builder +could call it directly instead of through its `keys` parameter — and +`help_bindings()` records only what goes through the parameter. Injected into +`editing()`: `KeyRow::looked_up(n, "clear-cell", None, "Edit the current +cell")`, which captions `x` as editing the cell — the exact lie `jhdf`'s test +exists for — and the whole workspace stayed green, because both guards that +would object read `help_bindings()`. + +Two changes. `looked_up` is now private to the `key_row` module, whose one +caller is the module's own `keys` — the lookup `pages()` hands every builder — +so the reviewer's injection is `E0624: associated function looked_up is +private`. That narrows the door without closing it: a builder can still reach +the module-level `keys` by path (`self::keys(..)`) rather than through its +shadowing parameter. So this test states the premise directly — the number of +keys rows drawn equals the number of rows the builders asked for (every asked +row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) — +and the `self::keys` bypass fails it `51 != 50`. diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 017bff2a..9382a557 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -324,11 +324,12 @@ pub struct HelpPage { } pub use key_row::KeyRow; +use key_row::keys; /// The one door to a keys row: its fields are private to this module, and its /// only constructor looks the keys up. mod key_row { - use super::{ModeKey, key_label, keys_for, prefixed_labels}; + use super::{HelpLine, ModeKey, key_label, keys_for, prefixed_labels}; #[derive(Debug, Clone, PartialEq, Eq)] pub struct KeyRow { @@ -347,7 +348,7 @@ mod key_row { /// An unbound command yields no keys and no row, so the page never /// advertises a key that does not exist. - pub(super) fn looked_up( + fn looked_up( mode: &ModeKey, command: &str, args: Option<&[String]>, @@ -361,17 +362,18 @@ mod key_row { (!keys.is_empty()).then_some(Self { keys, description }) } } -} -/// A keys row, looked up in `mode`. -fn keys( - mode: &ModeKey, - command: &'static str, - args: Option<&[&str]>, - description: &'static str, -) -> Option { - let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); - KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys) + /// A keys row, looked up in `mode`: the one caller of [`KeyRow::looked_up`], + /// and the lookup `pages` hands every builder (improvise-ssoe). + pub(super) fn keys( + mode: &ModeKey, + command: &'static str, + args: Option<&[&str]>, + description: &'static str, + ) -> Option { + let args: Option> = args.map(|a| a.iter().map(|s| (*s).to_string()).collect()); + KeyRow::looked_up(mode, command, args.as_deref(), description).map(HelpLine::Keys) + } } /// `gg`, `yy`, `ZZ`: a prefix key and the key after it, both off the keymap. @@ -1021,6 +1023,18 @@ mod tests { } } + /// improvise-ssoe: a keys row drawn but never asked for through the builders' + /// parameter is invisible to every guard. See `context/test-evidence/protocol.md`. + #[test] + fn every_keys_row_drawn_is_one_the_builders_asked_for() { + let drawn = pages() + .iter() + .flat_map(|page| page.lines.iter()) + .filter(|line| matches!(line, HelpLine::Keys(_))) + .count(); + assert_eq!(drawn, help_bindings().len()); + } + /// A row whose lookup finds no key is dropped silently, so a broken lookup /// looks like a shorter page. See `context/test-evidence/protocol.md`. #[test] From 95a35e8cce6a83d74252f3da8185373ec8a1ac52 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:19:05 -0700 Subject: [PATCH 20/28] Every JSON parser reads numbers exactly, and the check derives its crates (improvise-6yvg, improvise-dz8c) improvise-6yvg reproduced. A JSON import in improvise-command and a legacy JSON .improv load in improvise-io stored 0.24815714285714285 as 0.24815714285714283 in each crate's own test build. The root binary's `improvise import` used the same lossy parser. Now float_roundtrip is on every normal serde_json dependency in the workspace: root, improvise-command, improvise-io, and the optional test-support dependency of improvise-protocol. improvise-dz8c: the hand-listed cargo-tree check over three frame-parsing crates is replaced by tests/json_numbers.rs. It reads `cargo metadata --no-deps` and names every package, including target-specific entries, whose normal serde_json dependency lacks the feature. The rule and why it beats "parses a frame", the build graph, and enabling the feature once in the protocol crate are in context/code/protocol.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- Cargo.toml | 4 +- context/code/protocol.md | 66 +++++++++++++------ context/test-evidence/clients.md | 24 ++----- context/test-evidence/command.md | 15 +++++ context/test-evidence/formula-and-io.md | 14 ++++ context/test-evidence/tooling.md | 38 +++++++++++ crates/improvise-command/Cargo.toml | 4 +- crates/improvise-command/src/ui/effect.rs | 32 +++++++++ crates/improvise-io/Cargo.toml | 4 +- crates/improvise-io/src/persistence/mod.rs | 22 +++++++ crates/improvise-protocol/Cargo.toml | 3 +- .../tests/browser_round_trip.rs | 38 ----------- tests/json_numbers.rs | 61 +++++++++++++++++ 13 files changed, 245 insertions(+), 80 deletions(-) create mode 100644 tests/json_numbers.rs diff --git a/Cargo.toml b/Cargo.toml index 86f54e65..bda00df3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,7 +40,9 @@ improvise-command = { path = "crates/improvise-command" } ratatui = "0.30" crossterm = "0.29" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } anyhow = "1" thiserror = "2" indexmap = { version = "2", features = ["serde"] } diff --git a/context/code/protocol.md b/context/code/protocol.md index 828b9d8a..30b54d38 100644 --- a/context/code/protocol.md +++ b/context/code/protocol.md @@ -702,28 +702,54 @@ coming back one ULP off, both in `CellDisplay::value` (host to client, so `yy` copied a neighbour) and in a `SetCell` (client to host, so a typed or pasted number landed as a neighbour). -The feature is on for **every crate that parses a frame**: `improvise-ws-server` -(`transport.rs`), `improvise-worker-server` (`worker.rs`) and -`improvise-wasm-client` (`socket.rs`). Each crate's own manifest names it, -because each browser bundle is built from one crate alone. The `wasm` CI job -and `wasm-pack` both invoke per crate, and feature unification cannot reach -across separate invocations (`context/repo-map.md`, `## Wasm`). Enabling it in -one crate and relying on the others to inherit it works in `cargo test` and -nowhere else. +The feature is on for **every workspace crate with a normal dependency on +serde_json** (improvise-dz8c). Each crate's own manifest names it, because each +build is made from one crate alone: the `wasm` CI job and `wasm-pack` invoke +per crate, and feature unification cannot reach across separate invocations +(`context/repo-map.md`, `## Wasm`). Enabling it in one crate and relying on the +others to inherit it works in `cargo test` and nowhere else. -That is also why the end-to-end tests cannot guard this alone. They run in one -unified build with `improvise-ws-server` as a dev-dependency, so they stay -green with the feature removed from the browser bundle. -`every_build_that_parses_a_frame_enables_float_roundtrip` asks `cargo tree` -per crate over normal edges instead. A new crate that parses frames belongs in -its list. +**Why "can parse" and not "parses a frame".** The first version of the rule +covered only the three crates that parse frames and listed them by hand. +Frames are not the only place a user's number crosses a JSON parser. +`improvise-io`'s legacy-JSON `.improv` load, `improvise-command`'s JSON import +(`StartImportWizard`, `ImportJsonHeadless`) and the root binary's +`improvise import` all parsed with the lossy default, and in the TUI binary +nothing enabled the feature (improvise-6yvg, reproduced: `0.24815714285714285` +stored as `…283`). The worker bundle happened to be exact through unification, +so one import gave different numbers in the two front ends. "Parses JSON input" +cannot be read off a manifest, but "has a normal serde_json dependency" can, and +it over-approximates in the cheap direction: the feature changes only parsing, +so a crate that only serializes pays nothing for it. Dev-dependencies are out, +because a test build ships nowhere. That also leaves out `gp-tree`, whose only +serde_json is a dev-dependency (its example CLI reads a saved model). Every +number it persists is an `f32`, and 3M random `f32`s all read back exactly +through the lossy parser, against about 30% of random `f64`s. -Not covered, and filed separately rather than widened into this change -(improvise-6yvg): `improvise-io`'s JSON import and `improvise-command`'s JSON -reads also parse numbers with the default parser. They are file formats, not -frames. In the TUI binary nothing enables the feature. In the worker bundle -unification happens to reach them, so one import can give different numbers -in the two front ends. +**Why a declaration and not the build graph.** The first check asked +`cargo tree` whether each listed crate's *build* had the feature. That answer +can be yes by accident. `improvise-command` depends on `improvise-io`, so with +the feature removed from command's manifest, command's own test build still +has it, and its import test stays green. The rule requires the crate's own +manifest to say it, so a later change to a dependency cannot take it away. + +**Why not enable it once, in `improvise-protocol`.** That removes the list, but +it would give the protocol a normal serde_json dependency it has no use for +(its repo-map entry says core and serde only). It would also reach only builds +that include the protocol, and `improvise-io` does not depend on it. Under this +rule the protocol's one serde_json dependency, the optional `test-support` one, +carries the feature too. That is off in every shipped build, so it costs +nothing. + +`every_crate_that_can_parse_json_enables_float_roundtrip` (`tests/json_numbers.rs`) +reads `cargo metadata --no-deps` and names every package, and every +target-specific entry, whose normal serde_json dependency lacks the feature. A +new crate is covered the day it declares the dependency. It does not reach +`harness/opfs-contract`, which is its own workspace and only serializes. + +The end-to-end tests cannot guard this alone. They run in one unified build +with `improvise-ws-server` as a dev-dependency, so they stay green with the +feature removed from the browser bundle. ### `AxisFrame` — the grid's frame, and why it is *one* type diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index e69e7008..38263dfd 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4442,21 +4442,9 @@ before #178 too, as the review noted. **Fault injected:** the same pre-fix tree. Fails: the host stored `4598108823619341467` in South/Cost. -### `every_build_that_parses_a_frame_enables_float_roundtrip` - -The two tests above run in **one** feature-unified test build, which includes -`improvise-ws-server` and `improvise-command` as dev-dependencies. So they pass -whenever **any** crate in that build enables the feature. The shipped browser -bundle is built from one crate alone, so it would still be lossy. That was -demonstrated: with the feature removed from `improvise-wasm-client` only, both -tests stayed green while `cargo tree` showed no `float_roundtrip` in the wasm -bundle. This test asks cargo instead, once per crate that parses a frame: -`cargo tree --locked --edges normal,features --invert serde_json --package -`, with `--target wasm32-unknown-unknown` for the two bundles. It uses -normal edges only, because a dev edge would unify the feature back in -(`context/repo-map.md`, `## Wasm`). - -**Faults injected:** the feature removed from `improvise-wasm-client` alone, -which fails naming that crate while the two tests above pass. Then removed from -`improvise-ws-server` alone, which fails naming it. The native path runs -without `--target`, so the two arms are different invocations. +**Retired: `every_build_that_parses_a_frame_enables_float_roundtrip`.** It asked +`cargo tree` about three crates listed by hand. improvise-dz8c replaced it with +`every_crate_that_can_parse_json_enables_float_roundtrip`, whose entry is in +`tooling.md`. The injections recorded for it still hold: with the feature +removed from `improvise-wasm-client` alone, the two tests above stay green, so +they cannot guard the bundle alone. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 8c6361fc..a8ff3223 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5110,3 +5110,18 @@ exactly that, because West had no record to drill into. It then asserts that the first message does not mention drilling. At `0ed2e41` it failed with the old text. The rival is restoring that text. Real drill paste, which means staging a typed `CellValue`, is still open as improvise-48jd. + +## JSON import numbers (improvise-6yvg) + +### `a_json_import_stores_its_numbers_bit_for_bit` + +`ImportJsonHeadless` imports `[{"Region":"East","Revenue":0.24815714285714285}]` +and the test reads Region=East, _Measure=Revenue back by `f64::to_bits`. +`StartImportWizard` parses through the same `serde_json::from_slice` call, so +one test covers the crate's feature. + +**Red at `1158c23`** in `cargo test -p improvise-command`: stored +`0.24815714285714283`. That reproduces improvise-6yvg. **Limitation:** this test +cannot see the feature removed from `improvise-command` alone, because +`improvise-io`, a normal dependency, enables it in the same build. That case is +held by `every_crate_that_can_parse_json_enables_float_roundtrip` (`tooling.md`). diff --git a/context/test-evidence/formula-and-io.md b/context/test-evidence/formula-and-io.md index daf7a527..48e8bd5a 100644 --- a/context/test-evidence/formula-and-io.md +++ b/context/test-evidence/formula-and-io.md @@ -1434,3 +1434,17 @@ rather than defaulted: the alternative — expanding the group in the export — reintroduce the iot6 divergence between what the user sees and what they export. On the pre-6qet tree the body is empty (the group had no row, and pruning left nothing), so the test fails there. + +## Legacy JSON numbers (improvise-6yvg) + +### `a_legacy_json_workbook_loads_its_numbers_bit_for_bit` + +A workbook with Food/Jan holding `0.24815714285714285` is serialized with +`serde_json` and loaded through `load_from`. A leading `{` sends it down the +legacy JSON path. The test first asserts that the text holds the exact decimal, +so what it measures is the parse. It compares `f64::to_bits`. Found by the +review of PR #190, not by the ticket. + +**Red at `1158c23`** in `cargo test -p improvise-io`: loaded +`0.24815714285714283`. Green once `improvise-io`'s own manifest enables +`float_roundtrip`, and red again with only that line reverted. diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 341a0a32..1ce9e851 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -610,3 +610,41 @@ to git. A later `add` is excused. After `git checkout --` and a hand loss, the its `.base`, `.ledger`, `.removed` and `.stamped` siblings and `.lock`. `git status --untracked-files=all` and `git add -A --dry-run` both show none of them, and `git check-ignore -v` names `.gitignore:40` (`prolog-sessions/*`). + +## Every JSON parser reads numbers exactly (improvise-dz8c) + +### `every_crate_that_can_parse_json_enables_float_roundtrip` + +It replaces a check over three crates listed by hand. It reads +`cargo metadata --no-deps --locked` and fails naming every workspace package, and +every target-specific entry, whose **normal** serde_json dependency lacks +`float_roundtrip`. The rule and why it was chosen over the alternatives are in +`context/code/protocol.md`. It is red at `1158c23`, naming `improvise`, +`improvise-command`, `improvise-io` and `improvise-protocol`. + +**Faults injected, each run:** + +- **The feature removed from `improvise-io` alone.** The check fails naming + `improvise-io (all targets)`, and so does + `a_legacy_json_workbook_loads_its_numbers_bit_for_bit`. + `a_json_import_stores_its_numbers_bit_for_bit` stays green, because + `improvise-command` enables the feature and unification reaches io in its + build. +- **The feature removed from `improvise-command` alone.** The check fails naming + `improvise-command (all targets)`. The command crate's own import test + **stays green**: `cargo tree -p improvise-command` shows io's declaration + switching the feature on. So the manifest check is the only guard that + sees this case. It is also the case the retired build-graph check would + have passed. +- **A throwaway workspace member**, `zz-throwaway`, with normal dependencies on + `improvise-protocol` and `serde_json` and no feature (the lockfile updated, + as a real new crate would be). The check fails naming + `zz-throwaway (all targets)`. With the unconditional dependency carrying the + feature and a `cfg(target_arch = "wasm32")` one lacking it, the check fails + naming `zz-throwaway (cfg(target_arch = "wasm32"))`. That is stricter than + the build needs, since a wasm build unifies the two entries, and cheap to + satisfy. The crate was then removed. + +The `checked.len() >= 2` assertion is the positive signal. If a cargo change +renamed the metadata fields, the check would find no serde_json dependency and +pass vacuously without it. diff --git a/crates/improvise-command/Cargo.toml b/crates/improvise-command/Cargo.toml index 5f1bd1d9..acc61a1f 100644 --- a/crates/improvise-command/Cargo.toml +++ b/crates/improvise-command/Cargo.toml @@ -14,7 +14,9 @@ improvise-io = { path = "../improvise-io" } improvise-gp = { path = "../improvise-gp" } anyhow = "1" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } unicode-width = "^0.2" # Instant::now() panics on wasm32-unknown-unknown; web_time re-exports # std::time off-wasm and uses performance.now() on it (improvise-ywd). diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index e7060309..2835cd0a 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -3080,6 +3080,38 @@ mod tests { assert_eq!(app.view_state.status_msg, "Imported successfully"); } + #[test] + fn a_json_import_stores_its_numbers_bit_for_bit() { + const LOSSY: f64 = 0.248_157_142_857_142_85; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("data.json"); + std::fs::write( + &path, + r#"[{"Region":"East","Revenue":0.24815714285714285}]"#, + ) + .unwrap(); + + let mut app = test_app(); + ImportJsonHeadless { + path, + model_name: None, + array_path: None, + } + .apply(&mut app); + + let key = CellKey::new(vec![ + ("Region".to_string(), "East".to_string()), + ("_Measure".to_string(), "Revenue".to_string()), + ]); + let Some(CellValue::Number(n)) = app.model_state.workbook.model.get_cell(&key) else { + panic!( + "the import must store the number; status {:?}", + app.view_state.status_msg + ); + }; + assert_eq!(n.to_bits(), LOSSY.to_bits(), "stored {n:?}, not {LOSSY:?}"); + } + // ── json_import_records (improvise-oaq helper) ────────────────────── #[test] diff --git a/crates/improvise-io/Cargo.toml b/crates/improvise-io/Cargo.toml index 1fdfcaf1..1ec96762 100644 --- a/crates/improvise-io/Cargo.toml +++ b/crates/improvise-io/Cargo.toml @@ -17,7 +17,9 @@ indexmap = { version = "2", features = ["serde"] } pest = "2.9.2" pest_derive = "2.9.2" serde = { version = "1", features = ["derive"] } -serde_json = "1" +# Parses JSON a user's numbers cross, so it must read back the f64 written: per crate, +# because each build stands alone (improvise-6yvg, context/code/protocol.md). +serde_json = { version = "1", features = ["float_roundtrip"] } # The browser backend. Only the wasm build pulls these in; the native # binary never sees them (improvise-i34). diff --git a/crates/improvise-io/src/persistence/mod.rs b/crates/improvise-io/src/persistence/mod.rs index ab355b1e..505dfa2c 100644 --- a/crates/improvise-io/src/persistence/mod.rs +++ b/crates/improvise-io/src/persistence/mod.rs @@ -205,6 +205,28 @@ mod tests { assert_eq!(m2.model.name, "Budget"); } + #[test] + fn a_legacy_json_workbook_loads_its_numbers_bit_for_bit() { + use crate::storage::{MemoryFs, Storage}; + const LOSSY: f64 = 0.248_157_142_857_142_85; + let key = coord(&[("Type", "Food"), ("Month", "Jan")]); + let mut wb = two_cat_model(); + wb.model.set_cell(key.clone(), CellValue::Number(LOSSY)); + let json = serde_json::to_string(&wb).unwrap(); + assert!( + json.contains("0.24815714285714285"), + "the file holds the exact text" + ); + let fs = MemoryFs::new(); + let path = std::path::Path::new("legacy.improv"); + fs.write(path, json.as_bytes()).unwrap(); + let loaded = super::load_from(&fs, path).unwrap(); + let Some(CellValue::Number(n)) = loaded.model.get_cell(&key) else { + panic!("the cell must load as a number"); + }; + assert_eq!(n.to_bits(), LOSSY.to_bits(), "loaded {n:?}, not {LOSSY:?}"); + } + // ── save/load roundtrip via file ──────────────────────────────────── #[test] diff --git a/crates/improvise-protocol/Cargo.toml b/crates/improvise-protocol/Cargo.toml index c114f10c..62c4a498 100644 --- a/crates/improvise-protocol/Cargo.toml +++ b/crates/improvise-protocol/Cargo.toml @@ -14,7 +14,8 @@ serde = { version = "1", features = ["derive"] } # `char`, and the TUI has always measured it this way. unicode-width = "^0.2" # Only for `variants`, which is off unless a test build asks for it. -serde_json = { version = "1", optional = true } +# float_roundtrip because every normal serde_json dep carries it (improvise-dz8c). +serde_json = { version = "1", optional = true, features = ["float_roundtrip"] } [features] # `variants` reaches OUT of this crate: improvise-command bounds its own diff --git a/crates/improvise-wasm-client/tests/browser_round_trip.rs b/crates/improvise-wasm-client/tests/browser_round_trip.rs index 56ae17a9..adbc2c7f 100644 --- a/crates/improvise-wasm-client/tests/browser_round_trip.rs +++ b/crates/improvise-wasm-client/tests/browser_round_trip.rs @@ -707,44 +707,6 @@ fn a_yanked_number_crosses_the_serialized_wire_exactly() { ); } -/// The two tests around this one share a feature-unified build, so neither can -/// see ONE frame-parsing crate lacking `float_roundtrip`. Evidence file. -#[test] -fn every_build_that_parses_a_frame_enables_float_roundtrip() { - for (krate, target) in [ - ("improvise-wasm-client", Some("wasm32-unknown-unknown")), - ("improvise-worker-server", Some("wasm32-unknown-unknown")), - ("improvise-ws-server", None), - ] { - let mut cargo = std::process::Command::new(env!("CARGO")); - cargo.current_dir(env!("CARGO_MANIFEST_DIR")).args([ - "tree", - "--locked", - "--edges", - "normal,features", - "--invert", - "serde_json", - "--package", - krate, - ]); - if let Some(target) = target { - cargo.args(["--target", target]); - } - let out = cargo.output().expect("cargo tree must run"); - assert!( - out.status.success(), - "{krate}: cargo tree failed: {}", - String::from_utf8_lossy(&out.stderr) - ); - let tree = String::from_utf8(out.stdout).expect("cargo tree prints UTF-8"); - assert!( - tree.contains(r#"serde_json feature "float_roundtrip""#), - "{krate} parses frames without float_roundtrip, so its numbers land \ - one ULP off:\n{tree}" - ); - } -} - /// A typed number is parsed exactly in the client and re-parsed from JSON by /// the host. Evidence file. #[test] diff --git a/tests/json_numbers.rs b/tests/json_numbers.rs new file mode 100644 index 00000000..e11985fb --- /dev/null +++ b/tests/json_numbers.rs @@ -0,0 +1,61 @@ +//! A number read through serde_json must be the number written (improvise-6yvg, +//! improvise-dz8c). The rule and its derivation: `context/code/protocol.md`. + +use serde_json::Value; + +/// Every NORMAL serde_json dependency in the workspace, and those lacking +/// `float_roundtrip`, each spelled `package (target)`. +fn lossy_json_dependencies(metadata: &Value) -> (Vec, Vec) { + let mut checked = vec![]; + let mut lossy = vec![]; + let packages = metadata["packages"] + .as_array() + .expect("metadata lists packages"); + for package in packages { + let name = package["name"].as_str().expect("a package has a name"); + let deps = package["dependencies"] + .as_array() + .expect("a package lists deps"); + for dep in deps { + if dep["name"] != "serde_json" || !dep["kind"].is_null() { + continue; + } + let target = dep["target"].as_str().unwrap_or("all targets"); + let spelled = format!("{name} ({target})"); + let features = dep["features"].as_array().expect("a dep lists features"); + if !features.iter().any(|f| f == "float_roundtrip") { + lossy.push(spelled.clone()); + } + checked.push(spelled); + } + } + (checked, lossy) +} + +/// Derived from `cargo metadata`, not listed: a new crate that can parse JSON +/// is covered the day it declares the dependency. Evidence file. +#[test] +fn every_crate_that_can_parse_json_enables_float_roundtrip() { + let out = std::process::Command::new(env!("CARGO")) + .current_dir(env!("CARGO_MANIFEST_DIR")) + .args(["metadata", "--no-deps", "--locked", "--format-version", "1"]) + .output() + .expect("cargo metadata must run"); + assert!( + out.status.success(), + "cargo metadata failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + let metadata: Value = serde_json::from_slice(&out.stdout).expect("metadata is JSON"); + let (checked, lossy) = lossy_json_dependencies(&metadata); + assert!( + checked.len() >= 2, + "the derivation found {checked:?}: too few to be reading the metadata right" + ); + assert!( + lossy.is_empty(), + "these parse JSON numbers with serde_json's lossy default, one ULP off \ + for roughly one decimal in seven; add features = [\"float_roundtrip\"] \ + to each crate's own manifest: {lossy:?}" + ); +} From c70c428c47555575e834f86bd53316d1e6311c23 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:22:29 -0700 Subject: [PATCH 21/28] WIP: determinism law drills every item of the document (improvise-e1sm) --- .../src/command/cmd/determinism.rs | 54 ++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 7565c9ee..60f97478 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -1,12 +1,14 @@ //! A command is a function of its context: two contexts built independently from //! equal data give equal effects — `context/code/command.md`, improvise-hzjo. -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashSet}; use super::core::{Cmd, CmdContext}; use super::registry::tests::candidate_args; use crate::command::key::{KeyCode, KeyModifiers}; -use crate::model::cell::CellValue; +use crate::model::Model; +use crate::model::cell::{CellKey, CellValue}; +use crate::model::record::record_of; use crate::ui::app::App; use crate::ui::effect::{self, Effect}; @@ -47,10 +49,43 @@ fn fixture() -> App { app } +/// Every item as a one-coordinate drill: each spans the other categories whole, +/// so the store has several records to order — see the evidence. +fn one_coordinate_drills(model: &Model) -> Vec> { + let mut drills = Vec::new(); + for cat in model.regular_category_names() { + for item in model.category(cat).expect("listed").ordered_item_names() { + drills.push(vec![format!("{cat}/{item}")]); + } + } + drills +} + +/// How many distinct records the widest all-`Cat/Item` candidate matches. +fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { + let coordinates = |args: &Vec| -> Option> { + args.iter() + .map(|a| a.split_once('/').map(|(c, i)| (c.to_string(), i.to_string()))) + .collect() + }; + candidates + .iter() + .filter(|args| !args.is_empty()) + .filter_map(coordinates) + .map(|partial| { + let cells = model.data.matching_cells(&partial); + let records: HashSet = cells.iter().map(|(k, _)| record_of(k)).collect(); + records.len() + }) + .max() + .unwrap_or(0) +} + /// The registry's candidates, plus names the demo really has: a drill or a fit /// over coordinates that match nothing never reaches the records it would order. -fn args_the_document_names() -> Vec> { +fn args_the_document_names(model: &Model) -> Vec> { let mut all = candidate_args(); + all.extend(one_coordinate_drills(model)); all.extend( [ vec!["Revenue"], @@ -86,15 +121,16 @@ struct Comparison { fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { let mut found = Comparison::default(); + let candidates = args_the_document_names(a.model); for name in a.registry.names().collect::>() { let before = found.compared; - for args in args_the_document_names() { - let Some(left) = effects_of(name, &args, a) else { + for args in &candidates { + let Some(left) = effects_of(name, args, a) else { continue; }; found.compared += 1; for b in others { - let right = effects_of(name, &args, b).unwrap_or_default(); + let right = effects_of(name, args, b).unwrap_or_default(); if left != right { found.diverged.insert(name); found @@ -118,6 +154,12 @@ fn every_command_gives_equal_effects_in_independently_built_equal_contexts() { .iter() .map(|app| app.cmd_context(KeyCode::Null, KeyModifiers::NONE)) .collect(); + let model = contexts[0].model; + let widest = widest_drill(model, &args_the_document_names(model)); + assert!( + widest >= 4, + "the widest drill orders {widest} records, so the store's order is nearly a coin toss" + ); let found = compare(&contexts[0], &contexts[1..]); assert!( found.never_built.is_empty(), From f24944b4372b74b425e77056b85e6ca3f7f43aa0 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:23:26 -0700 Subject: [PATCH 22/28] =?UTF-8?q?WIP:=20determinism=20law=20docs=20?= =?UTF-8?q?=E2=80=94=20measured=20catch=20rate,=20execute-not-apply=20scop?= =?UTF-8?q?e=20(improvise-e1sm)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- context/code/command.md | 43 ++++++++++++++++++++++++++------ context/test-evidence/command.md | 22 +++++++++++++--- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 7be2849d..a57e0f3f 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3104,13 +3104,42 @@ make it negligible. The fixture's values are also chosen to stay distinct under the edits commands make. A first version used `v0`…`v15`, which `pop()` collapses to `v`, and so it let `command-mode-backspace` reading an arbitrary buffer pass. -**What it cannot see.** Only commands and arguments the fixture reaches. The -registry's `candidate_args` are extended with names the demo really has, because -coordinates that match nothing never reach the records a drill or a fit would -order. A command whose nondeterminism needs some other state, such as records -mode, a wizard or a drill in progress, is outside this fixture. And `Debug` -equality is text equality: two effects that differ only in something their -`Debug` omits are equal here. +**The store needs several records to order, so the drills come from the +document (improvise-e1sm).** The registry's `candidate_args` are extended with +names the demo really has, because coordinates that match nothing never reach +the records a drill or a fit would order. A hand-picked name is not enough. The +first version's only real drill was `Product/Latte, Quarter/Q2`: one record, two +cells. With the store's sort removed, two contexts put those two cells in the +same order about half the time, and with three comparisons the law passed about +one run in eight (measured: 46 of 400). So the arguments now include **every +item of every regular category as a one-coordinate drill**, read off the parsed +document. A one-coordinate drill spans every other category whole, so it matches +the whole slice of records under that item, not one record picked by name. In +the demo that is four records and eight cells for each of the eight drills. The +test +also asserts that the widest candidate it actually passes matches at least four +records. That guard reads the argument list the law uses, not a copy of it, so +dropping the derived drills fails it, and so does a demo with too little data. +With the store's sort removed, the law now fails 400 of 400 runs. + +**What it cannot see.** Only commands and arguments the fixture reaches. A +command whose nondeterminism needs some other state, such as records mode, a +wizard or a drill in progress, is outside this fixture. And `Debug` equality is +text equality: two effects that differ only in something their `Debug` omits are +equal here. + +**Its scope is what `execute` returns, not what `Effect::apply` does.** The law +compares the effects a command hands back. Many effects are unit structs that +decide all their work inside `apply`: `ApplyAndClearDrill`, +`SortData`, `CleanEmptyRecords`, `Save`, `RebuildLayout`, `AcceptWizardModel`. +Hash-order iteration inside an `apply` is therefore invisible here, however +often the law runs. improvise-uo2p is the worked example. +`ApplyAndClearDrill::apply` walks `drill.pending_edits`, a `HashMap`, in hash +order. A measure edit and a category rename staged on one row can then write a +ghost record. The command that emits it passes this law, because the effect's +`Debug` is just its name. So "the one kind of nondeterminism this repo has +shipped" is guarded here only on the `execute` side. The `apply` side needs its +own test per effect, or a law over `App` state after `apply_effects`. **The exemption list.** `OUTSIDE_THE_CONTEXT` names commands that legitimately depend on something outside the context, such as a clock or an entropy source, diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 4340a824..38613ad6 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5043,9 +5043,25 @@ rivals it catches: **passes**. That is the half the ticket required: the distinct construction is what does the work. - **`DataStore::matching_positions` without its sort**: gotcha 18 re-shipped. - Injected, full workspace run: this test fails, reached through - `drill-into-cell` and `fit-measure`'s record order, alongside the three - `cell.rs` order tests that pin the store directly. + This entry first said the test catches it through `drill-into-cell` and + `fit-measure`'s record order. That was wrong (improvise-e1sm). `fit-measure` + never diverged, in 60 failing runs in review and in 354 here. And the catch + was a matter of luck. The only real drill was `Product/Latte, Quarter/Q2`, + which is one record with two cells, so each other context agrees with + probability 1/2. With the sort removed and the test binary run in a loop, the + law **passed 46 of 400 runs** at `fc0b13c`, and `drill-into-cell + ["Product/Latte", "Quarter/Q2"]` was the only pair that ever diverged. The + arguments now include every item of every regular category as a + one-coordinate drill, read off the parsed document (four records, eight cells + each in the demo). With the same injection the law **passed 0 of 400**, and + all eight new drills diverged in about 1195 of 1200 pairwise comparisons each. + Without the injection it passed 400 of 400. The three `cell.rs` order tests + still pin the store directly. +- **The derived drills dropped from the argument list** (the + `all.extend(one_coordinate_drills(model))` line commented out): the test + fails before comparing anything, with "the widest drill orders 1 records". The + width guard reads the same argument list the comparison uses, so it cannot + pass while the comparison has lost its wide drills. - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". From 0825f9984d18ef026a8d60e74f0f63fce339260a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:29:09 -0700 Subject: [PATCH 23/28] rustfmt the determinism law's width guard (improvise-e1sm) --- crates/improvise-command/src/command/cmd/determinism.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 60f97478..8f0a71e5 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -65,7 +65,10 @@ fn one_coordinate_drills(model: &Model) -> Vec> { fn widest_drill(model: &Model, candidates: &[Vec]) -> usize { let coordinates = |args: &Vec| -> Option> { args.iter() - .map(|a| a.split_once('/').map(|(c, i)| (c.to_string(), i.to_string()))) + .map(|a| { + a.split_once('/') + .map(|(c, i)| (c.to_string(), i.to_string())) + }) .collect() }; candidates From bc08a130389710a0069a52a8bd2bd022d50eaf27 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:30:55 -0700 Subject: [PATCH 24/28] Compose a drill's staged edits instead of replaying them in hash order (improvise-uo2p) ApplyAndClearDrill walked pending_edits, a HashMap, and applied each edit on its own. A rename moves the row's cells, so a measure edit replayed after it wrote the old key. That left a ghost record there and a stale value in the moved record. A second rename on the same row always found nothing and was dropped. Across rows, a swap lost one record's data in either order. RecordRewrite::of builds each row's final record once. It reads unedited values off the model before anything is written, and applies every rename to every cell. RecordRewrite::merged puts every clear before every write, in snapshot row order. The plain records path calls the same function with its one edit, so rename_record_coordinate and the drill's own copy are gone (gotcha 19). Reasoning: context/code/command.md, RecordRewrite. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 56 ++- context/test-evidence/command.md | 57 +++ .../src/command/cmd/commit.rs | 55 +-- crates/improvise-command/src/ui/effect.rs | 380 +++++++++++++++--- 4 files changed, 437 insertions(+), 111 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 3dd16fd3..1aba86c2 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -907,6 +907,52 @@ There is deliberately **no `dirty = true`** in it: it declares of `MarkDirty` — harmless, and a second home for the one thing that commit set out to give exactly one. +### `RecordRewrite` — a row's edits composed, not replayed (improvise-uo2p) + +A staged drill edit is a statement about the record **as the snapshot showed +it**. It is not a statement about the model as the previous edit left it. The +edits in one row do not commute when they are replayed one at a time. A rename +moves every cell in the row (gotcha 1). A measure edit replayed after it writes +the old key, which leaves a ghost record there and a stale value in the moved +record. A second rename replayed after the first finds nothing at the old keys, +so it is dropped. `ApplyAndClearDrill` replayed them in `HashMap` order, so the +outcome was different from one App to the next. That is gotcha 18's class of +bug, found in an effect rather than an index. + +**Sorting is not the fix.** "Measures before renames" repairs the ticket's own +case and still drops a second rename every time (rival 1 in the evidence). Any +fixed order makes the corruption reproducible instead of removing it. So the +rule is **build each row's final record once, then write it**: + +- `RecordRewrite::of(model, row, edits)` collects the row's renames and measure + edits into maps keyed by column, and reads each unedited value off the model + **before anything is written**. It then states the rewrite: clear every old + key, and write every cell at the key with *all* renames applied. A measure + edit that is staged replaces the carried value. The result does not depend on + the order of the edits. A drill stages at most one edit per `(row, column)`, + so no two edits in a row name the same column. A blank rename is refused, and + the row's other edits still land at the unrenamed coordinates, as before. +- `RecordRewrite::merged` concatenates the rows' rewrites **by phase**: every + refusal, then every item added, then every clear, then every write. The clears + come first so that a swap is a swap. Rows are taken in snapshot row order + (`BTreeMap`), so where two final records land on one cell, **the later row + wins**. That is the answer plain records mode gives for the same renames made + top to bottom. + +The collision rule is a **choice**, and it still loses the earlier row's values +silently. Plain records mode loses them the same way when a rename lands on an +existing record. Refusing the colliding rows instead would be a change to both +paths. + +**One copy.** The plain records path (`commit.rs::commit_plain_records_edit`) +calls the same `RecordRewrite::of` with its one edit, so `rename_record_coordinate` +and the drill's `apply_coordinate_rename`/`apply_measure_edit` are gone. This is +the consolidation improvise-fdk1 asked for, "regardless" of its wire half. For a +single edit the output is the effects the plain path used to emit, reordered from +`clear, set` per cell to all clears then all sets, which touches distinct keys. +One behaviour moved on the drill side: it used to discard `add_item`'s error, and +now goes through `effect::AddItem`, which refuses when the category has gone. + ### `Save` / `SaveAs` abort the batch (improvise-8tk / improvise-lae) A failed save aborts the rest of the batch, so the `Quit` that `wq`/`ZZ` chains @@ -2601,8 +2647,10 @@ At bottom-right `EnterAdvance` has no place to go, so it emits a single `AbortCh stages in drill pending edits while in drill mode, or applies directly in plain records mode; for real keys it writes to the model. -`rename_record_coordinate` renames one coordinate of a records row. In wide format a -row is a group of cells — one per measure — so the rename has to move **every** one +`commit_plain_records_edit` hands a coordinate or measure edit to +`effect::RecordRewrite::of`, the one home it shares with the drill (improvise-uo2p; +see `RecordRewrite` above). It replaced `rename_record_coordinate`. In wide format a +row is a group of cells — one per measure — so a rename has to move **every** one of them, not just the cell under the cursor (improvise-c5v, gotcha 1). Every cell in the row moves, and a row seeded by `add-record-row` has no measure cells yet: its coordinates live on a lone measure-less cell, which must move too or the row loses @@ -3122,8 +3170,8 @@ the cursor**. The result was silent corruption of an unrelated record: appending a row changes `view_frame`'s hash, the frame observation inside `apply_effects` invalidates the cursor to the origin *mid-batch* (gotcha 24), and `EnterEditAtCursor` then pre-fills the editor with row 0's category coordinate. -Enter commits through `rename_record_coordinate`, which by gotcha 1 moves every -measure cell of that row. +Enter commits through `rename_record_coordinate` (since improvise-uo2p, +`RecordRewrite::of`), which by gotcha 1 moves every measure cell of that row. Two things are worth carrying past this fix. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index d86e0500..445056ca 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5246,3 +5246,60 @@ refusal is recorded and the mode is not `Quit`. `import-json` failed first, before `refuse_and_stop` was applied to its arms. For `load`, the injected fault is removing its abort (`refuse_and_stop` back to `app.refuse`), and the test then fails with "`load absent.improv . q` quit having loaded nothing". + +### ui::effect::tests::a_drill_rows_measure_edits_follow_its_rename + +improvise-uo2p. `ApplyAndClearDrill` walked `pending_edits`, a `HashMap`, and +applied each edit alone against the model as the previous one left it. A rename +moves the row's cells. A measure edit then wrote `row.cell_key(measure)`, the +snapshot's old key. So when the rename came first, a ghost record appeared at the +old coordinates carrying the new value, and the moved record kept the stale one. +The reviewer saw this in 16 of 40 fresh Apps. The fixture stages two measure edits +and a rename on drill row 1, so a wrong answer needs only one of the three orders +that put the rename last. Each assertion runs over 32 fresh Apps and compares the +**whole** store, so the ghost and the stale value are both visible. Before the +fix it failed on five of five runs. Rival 3 (the rewrite addresses measure edits +at the unrenamed key) fails it. + +### ui::effect::tests::a_drill_row_renamed_twice_and_edited_lands_as_one_record + +improvise-uo2p. This is the case that fails **every** time, not with some +probability. Each rename read the snapshot's old keys. The second rename found +nothing there, because the first had moved it, so one rename was always dropped +and the row split into a `Clothing/Feb` record and a `Drink/Jan` record. Rival 1 +is the ticket's proposed fix direction: measures before renames, each edit +applied alone. It passes the ticket's own case and fails here. So "a fixed +order" is not a fix; the renames have to be composed. + +### ui::effect::tests::a_drill_that_swaps_two_records_coordinates_swaps_them + +improvise-uo2p. Row 0 is renamed onto row 1's coordinates and row 1 onto row 0's. +If the rows are applied in sequence, the first move overwrites the other record +and the second move carries the overwritten values back, so one record's data +is lost in either order. Rival 2 composes each row correctly but writes it +before reading the next row, and it fails here and nowhere else. That is why +`RecordRewrite::merged` reads every row off the model first, then clears every +old key, then writes. + +### ui::effect::tests::a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates + +improvise-uo2p. Row 1's staged `Cost` belongs to row 1's record, even though row +0 is being renamed away from coordinates and row 1 onto them. Before the fix, +some hash orders left `Clothing/Jan/Cost = 999` as a ghost. Rival 3 fails it. + +### ui::effect::tests::two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row + +improvise-uo2p. Two rows renamed onto the same coordinates collide per cell. The +rule chosen is that the later snapshot row wins, which is what plain records +mode gives when the same renames are made top to bottom. Before the fix the +winner was the hash order's choice. Rival 4 reverses the row order in `merged` +and fails here. The rule is a choice, and it still loses data silently. See +`context/code/command.md`, `RecordRewrite`. + +### ui::effect::tests::a_record_rewrite_is_the_same_under_every_order_of_its_edits + +improvise-uo2p. The composition is order-independent **by construction**, so it +is asserted over all 24 orders of four edits on one row: two renames, a measure +write and a measure clear. The test pins one value as well as the equality, so +a rewrite that is the same wrong answer in every order still fails. Rival 3 +fails it. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 6846f198..86e4e55d 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -1,8 +1,8 @@ use crate::model::category::MEASURE_CATEGORY; -use crate::model::cell::{CellKey, CellValue}; +use crate::model::cell::CellKey; use crate::ui::app::AppMode; use crate::ui::effect::{self, Effect}; -use crate::view::{RecordCol, RecordRow}; +use crate::view::RecordCol; use super::core::{Cmd, CmdContext, read_buffer}; use super::grid::AddRecordRow; @@ -14,6 +14,7 @@ mod tests { use super::*; use crate::command::cmd::test_helpers::*; + use crate::model::cell::CellValue; use crate::workbook::Workbook; fn commit(kind: CommitKind) -> CommitBuffer { @@ -622,47 +623,8 @@ fn stage_drill_edit(record_idx: usize, col: RecordCol, value: &str) -> Box>, -) { - if value.is_empty() { - effects.push(effect::refuse(effect::RECORD_COORDS_CANNOT_BE_EMPTY)); - return; - } - effects.push(Box::new(effect::AddItem { - category: category.to_string(), - item: value.to_string(), - })); - - // Every cell in the row moves. A row seeded by `add-record-row` carries its - // coordinates on a lone measure-less cell, which must move too. - let cells: Vec<(CellKey, CellValue)> = if row.values.is_empty() { - model - .get_cell(&row.coords) - .map(|v| vec![(row.coords.clone(), v.clone())]) - .unwrap_or_default() - } else { - row.values - .iter() - .map(|(measure, v)| (row.cell_key(measure), v.clone())) - .collect() - }; - for (old_key, cell_value) in cells { - effects.push(Box::new(effect::ClearCell(old_key.clone()))); - effects.push(Box::new(effect::SetCell( - old_key.with(category, value), - cell_value, - ))); - } -} - -/// Apply a records-mode edit directly to the model (no drill state). +/// Apply a records-mode edit directly to the model (no drill state). A drill's staged +/// edits go through the same `RecordRewrite`, all of a row's at once (gotcha 19). fn commit_plain_records_edit( ctx: &CmdContext, record_idx: usize, @@ -674,11 +636,8 @@ fn commit_plain_records_edit( return; }; match col { - RecordCol::Category(category) => { - rename_record_coordinate(ctx.model, row, category, value, effects); - } - RecordCol::Measure(measure) => { - commit_regular_cell_value(&row.cell_key(measure), value, effects); + RecordCol::Category(_) | RecordCol::Measure(_) => { + effects.extend(effect::RecordRewrite::of(ctx.model, row, [(col, value)]).effects()); } RecordCol::NewMeasure => add_measure(value, effects), RecordCol::NewCategory => add_category(value, effects), diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 9bc78b34..13c86523 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -1,3 +1,4 @@ +use std::collections::BTreeMap; use std::fmt::Debug; use std::path::PathBuf; @@ -655,24 +656,19 @@ impl Effect for ApplyAndClearDrill { let Some(drill) = app.drill_state.take() else { return; }; - if drill.pending_edits.is_empty() { - return; - } + let mut rows: BTreeMap> = BTreeMap::new(); for ((record_idx, col), new_value) in &drill.pending_edits { - let Some(row) = drill.records.get(*record_idx) else { - continue; - }; - match col { - RecordCol::Measure(measure) => { - apply_measure_edit(app, &row.cell_key(measure), new_value); - } - RecordCol::Category(category) => { - apply_coordinate_rename(app, row, category, new_value); - } - // Never staged: `commit_cell_value` applies a new measure or - // category immediately — a schema change is not a cell edit. - RecordCol::NewMeasure | RecordCol::NewCategory => {} - } + rows.entry(*record_idx) + .or_default() + .push((col, new_value.as_str())); + } + let model = &app.model_state.workbook.model; + let rewrite = RecordRewrite::merged(rows.iter().filter_map(|(record_idx, edits)| { + let row = drill.records.get(*record_idx)?; + Some(RecordRewrite::of(model, row, edits.iter().copied())) + })); + for effect in rewrite.effects() { + effect.apply(app); } // No `dirty = true` here: this declares `Model(CellData)` and // `App::apply_effects` raises the flag off that (improvise-gv9r). @@ -682,51 +678,114 @@ impl Effect for ApplyAndClearDrill { } } -/// Write one measure cell, or clear it when the edit is blank. -fn apply_measure_edit(app: &mut App, key: &CellKey, new_value: &str) { - let model = &mut app.model_state.workbook.model; - match new_value { - "" => { - model.clear_cell(key); - } - v => { - let value = v - .parse::() - .map(CellValue::Number) - .unwrap_or_else(|_| CellValue::Text(v.to_string())); - model.set_cell(key.clone(), value); - } - } +/// A records row's edits composed into the one record they mean, read off the model +/// before anything is written. Why composed: `context/code/command.md`, `RecordRewrite`. +#[derive(Debug, Default, PartialEq)] +pub(crate) struct RecordRewrite { + refused: bool, + items: Vec<(String, String)>, + clears: Vec, + sets: Vec<(CellKey, CellValue)>, } -/// Rename one coordinate of a records row. The row is a group of cells — -/// one per measure — so every one of them moves (improvise-c5v). -fn apply_coordinate_rename(app: &mut App, row: &RecordRow, category: &str, new_value: &str) { - if new_value.is_empty() { - app.refuse(RECORD_COORDS_CANNOT_BE_EMPTY); - return; - } - let keys: Vec = if row.values.is_empty() { - vec![row.coords.clone()] - } else { - row.values - .iter() - .map(|(measure, _)| row.cell_key(measure)) - .collect() - }; - let _ = app - .model_state - .workbook - .add_item(category, new_value.to_string()); - for old_key in keys { - let Some(value) = app.model_state.workbook.model.get_cell(&old_key).cloned() else { - continue; +impl RecordRewrite { + /// `row`'s edits, each naming a distinct column. The result does not depend on + /// their order: every coordinate is renamed before any cell is placed. + pub(crate) fn of<'a>( + model: &crate::model::Model, + row: &RecordRow, + edits: impl IntoIterator, + ) -> Self { + let mut refused = false; + let mut renames: BTreeMap<&str, &str> = BTreeMap::new(); + let mut measures: BTreeMap<&str, Option> = BTreeMap::new(); + for (col, value) in edits { + match col { + RecordCol::Category(_) if value.is_empty() => refused = true, + RecordCol::Category(category) => { + renames.insert(category.as_str(), value); + } + RecordCol::Measure(measure) => { + measures.insert(measure.as_str(), improvise_protocol::typed(value)); + } + // Never staged: a new measure or category is a schema change, + // applied at once by `commit_cell_value`. + RecordCol::NewMeasure | RecordCol::NewCategory => {} + } + } + let renamed = |key: &CellKey| { + renames.iter().fold(key.clone(), |key, (category, item)| { + key.with(*category, *item) + }) }; - app.model_state.workbook.model.clear_cell(&old_key); - app.model_state - .workbook - .model - .set_cell(old_key.with(category, new_value), value); + let moved: Vec<(Option<&str>, CellKey)> = match (renames.is_empty(), row.values.is_empty()) + { + (true, _) => Vec::new(), + (false, true) => vec![(None, row.coords.clone())], + (false, false) => row + .values + .iter() + .map(|(measure, _)| (Some(measure.as_str()), row.cell_key(measure))) + .collect(), + }; + let mut rewrite = RecordRewrite { + refused, + items: renames + .iter() + .map(|(category, item)| (category.to_string(), item.to_string())) + .collect(), + ..Self::default() + }; + for (measure, old) in &moved { + let value = match measure.and_then(|m| measures.remove(m)) { + Some(edit) => edit, + None => model.get_cell(old).cloned(), + }; + rewrite.clears.push(old.clone()); + rewrite.sets.extend(value.map(|v| (renamed(old), v))); + } + for (measure, edit) in measures { + let key = renamed(&row.cell_key(measure)); + match edit { + Some(value) => rewrite.sets.push((key, value)), + None => rewrite.clears.push(key), + } + } + rewrite + } + + /// Several rows' rewrites as one: every clear lands before any write, so a + /// swap is a swap, and where two rows land on one cell the later row wins. + pub(crate) fn merged(rewrites: impl IntoIterator) -> Self { + rewrites.into_iter().fold(Self::default(), |mut all, one| { + all.refused |= one.refused; + all.items.extend(one.items); + all.clears.extend(one.clears); + all.sets.extend(one.sets); + all + }) + } + + pub(crate) fn effects(self) -> Vec> { + let refusal = self.refused.then(|| refuse(RECORD_COORDS_CANNOT_BE_EMPTY)); + let items = self + .items + .into_iter() + .map(|(category, item)| Box::new(AddItem { category, item }) as Box); + let clears = self + .clears + .into_iter() + .map(|key| Box::new(ClearCell(key)) as Box); + let sets = self + .sets + .into_iter() + .map(|(key, value)| Box::new(SetCell(key, value)) as Box); + refusal + .into_iter() + .chain(items) + .chain(clears) + .chain(sets) + .collect() } } @@ -2900,6 +2959,209 @@ mod tests { assert_eq!(app.model_state.workbook.model.get_cell(&key), None); } + // ── Composing a drill's staged edits (improvise-uo2p) ─────────────── + + fn drill_cell(ty: &str, month: &str, measure: &str) -> CellKey { + CellKey::new(vec![ + ("Type".into(), ty.into()), + ("Month".into(), month.into()), + ("_Measure".into(), measure.into()), + ]) + } + + /// Two records, each carrying Revenue and Cost: row 0 is Food/Jan (1, 2), + /// row 1 is Clothing/Jan (42, 17). Every edit below is staged on row 1 at least. + fn two_record_drill() -> App { + let mut app = test_app(); + let cells = [ + (drill_cell("Food", "Jan", "Revenue"), CellValue::Number(1.0)), + (drill_cell("Food", "Jan", "Cost"), CellValue::Number(2.0)), + ( + drill_cell("Clothing", "Jan", "Revenue"), + CellValue::Number(42.0), + ), + ( + drill_cell("Clothing", "Jan", "Cost"), + CellValue::Number(17.0), + ), + ]; + for (key, value) in cells.clone() { + app.model_state.workbook.model.set_cell(key, value); + } + let records = RecordRow::group(cells); + assert_eq!(records.len(), 2, "one row per coordinate tuple"); + assert_eq!(records[1].coords.get("Type"), Some("Clothing")); + StartDrill(records).apply(&mut app); + app + } + + fn stage(app: &mut App, record_idx: usize, col: RecordCol, value: &str) { + SetDrillPendingEdit { + record_idx, + col, + new_value: value.to_string(), + } + .apply(app); + } + + /// The whole store, as `(Type, Month, measure) = n`, so a ghost cannot hide. + fn drill_store(app: &App) -> std::collections::BTreeSet { + app.model_state + .workbook + .model + .data + .iter_cells() + .map(|(key, value)| { + let coord = |c: &str| key.get(c).unwrap_or("-").to_string(); + let n = value.as_f64().expect("every drill cell here is a number"); + format!( + "{}/{}/{} = {n}", + coord("Type"), + coord("Month"), + coord("_Measure") + ) + }) + .collect() + } + + fn category(name: &str) -> RecordCol { + RecordCol::Category(name.into()) + } + + fn measure(name: &str) -> RecordCol { + RecordCol::Measure(name.into()) + } + + /// Stage `edits` and leave the drill, in 32 fresh Apps: each `HashMap` gets its + /// own `RandomState`, and one map iterates the same way twice. + fn assert_drill_commits_to(edits: &[(usize, RecordCol, &str)], expected: &[&str]) { + let expected: std::collections::BTreeSet = + expected.iter().map(|s| s.to_string()).collect(); + for attempt in 0..32 { + let mut app = two_record_drill(); + for (row, col, value) in edits { + stage(&mut app, *row, col.clone(), value); + } + app.apply_effects(vec![Box::new(ApplyAndClearDrill)]); + assert_eq!(drill_store(&app), expected, "fresh App #{attempt}"); + } + } + + #[test] + fn a_drill_row_renamed_twice_and_edited_lands_as_one_record() { + assert_drill_commits_to( + &[ + (1, measure("Cost"), "999"), + (1, category("Type"), "Drink"), + (1, category("Month"), "Feb"), + ], + &[ + "Food/Jan/Revenue = 1", + "Food/Jan/Cost = 2", + "Drink/Feb/Revenue = 42", + "Drink/Feb/Cost = 999", + ], + ); + } + + #[test] + fn a_drill_rows_measure_edits_follow_its_rename() { + assert_drill_commits_to( + &[ + (1, measure("Revenue"), "100"), + (1, measure("Cost"), "999"), + (1, category("Type"), "Drink"), + ], + &[ + "Food/Jan/Revenue = 1", + "Food/Jan/Cost = 2", + "Drink/Jan/Revenue = 100", + "Drink/Jan/Cost = 999", + ], + ); + } + + #[test] + fn a_drill_that_swaps_two_records_coordinates_swaps_them() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Clothing"), + (1, category("Type"), "Food"), + ], + &[ + "Clothing/Jan/Revenue = 1", + "Clothing/Jan/Cost = 2", + "Food/Jan/Revenue = 42", + "Food/Jan/Cost = 17", + ], + ); + } + + #[test] + fn a_drill_edit_addresses_its_own_record_not_the_one_renamed_onto_its_coordinates() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Drink"), + (1, category("Type"), "Food"), + (1, measure("Cost"), "999"), + ], + &[ + "Drink/Jan/Revenue = 1", + "Drink/Jan/Cost = 2", + "Food/Jan/Revenue = 42", + "Food/Jan/Cost = 999", + ], + ); + } + + #[test] + fn a_record_rewrite_is_the_same_under_every_order_of_its_edits() { + let app = two_record_drill(); + let row = app.drill_state.as_ref().unwrap().records[1].clone(); + let edits = [ + (measure("Cost"), "999"), + (category("Type"), "Drink"), + (measure("Revenue"), ""), + (category("Month"), "Feb"), + ]; + let model = &app.model_state.workbook.model; + let mut seen = Vec::new(); + for a in 0..4 { + for b in (0..4).filter(|&b| b != a) { + for c in (0..4).filter(|&c| c != a && c != b) { + let d = 6 - a - b - c; + let order = [a, b, c, d].map(|i| (&edits[i].0, edits[i].1)); + seen.push(RecordRewrite::of(model, &row, order)); + } + } + } + assert_eq!(seen.len(), 24); + let first = &seen[0]; + assert_eq!( + first.clears, + vec![ + drill_cell("Clothing", "Jan", "Revenue"), + drill_cell("Clothing", "Jan", "Cost") + ] + ); + assert_eq!( + first.sets, + vec![(drill_cell("Drink", "Feb", "Cost"), CellValue::Number(999.0))] + ); + assert!(seen.iter().all(|rewrite| rewrite == first)); + } + + #[test] + fn two_drill_rows_renamed_onto_one_record_resolve_to_the_later_row() { + assert_drill_commits_to( + &[ + (0, category("Type"), "Drink"), + (1, category("Type"), "Drink"), + ], + &["Drink/Jan/Revenue = 42", "Drink/Jan/Cost = 17"], + ); + } + // ── Toggle effects ────────────────────────────────────────────────── #[test] From 9512a2db1f38138daf6a011d33159f43a588df35 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:39:36 -0700 Subject: [PATCH 25/28] The grid-move walk stands far from every edge, so it sees magnitude (5kdr) 21x21 grid, 8x8 window, start (10,10): moves land at 10+d, page-scroll at 10+page_rows(8)*d, jumps at 0/20. grid_move's page rule is page_rows(), which the test reads rather than restating. Doubled move-selection, a one-row scroll-rows and a page-size-blind page-scroll each fail. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 18 ++++++++++ crates/improvise-protocol/src/resolve.rs | 44 +++++++++++++++++------- 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index a8e5dfcd..a901a482 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4659,3 +4659,21 @@ where. The first rival fails on `PgUp` (`page-scroll ["-1"]`) and, alone, on `Ctrl-u` (`scroll-rows ["-5"]`). A last assertion that each kind was pressed at least once guards the other direction — a binding vanishing — and is a named list on purpose, since that is the one thing no walk over bindings can see. + +**improvise-5kdr: the 3x3 fixture saw only a sign.** From the centre of a 3x3 +grid every step of one or more clamps to an edge, so the w7qd version stated +`page-scroll` as `1 + d` — one row, which is not what it does — and passed +because the true landing clamped to the same edge. Measured on #198 at 8772db2: +`page-scroll` ignoring the page size together with `scroll-rows` moving one row, +or `move-selection` moving `2d`, left the test green. The walk now stands at +(10,10) of a 21x21 grid with an 8x8 window, far from every edge, so a landing +shows magnitude: `10+d` for `move-selection` and `scroll-rows`, `10 + page·d` +for `page-scroll`, 0/20 for the jumps. The page size is not written into the +test: `grid_move`'s page rule moved into `page_rows(window_rows)` and the test +asks it, so the rule has one home. Rivals, each run: `move-selection` doubled +fails on `Up` (`move-selection ["-1","0"]`); the page-size-and-one-row pair +fails on `PgUp` (`page-scroll ["-1"]`); the one-row `scroll-rows` alone fails on +`Ctrl-u` (`scroll-rows ["-5"]`); the jump-table rival still fails naming every +jump. The residual, stated: because the test reads `page_rows`, a change to the +page rule itself moves the test with it — whether that rule is the TUI's is +`tests/lockstep.rs`'s question, not this one's. diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 07b81414..d88b4e23 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -619,6 +619,11 @@ enum GridMove { Page(i32), } +/// Rows one `page-scroll` step moves: three quarters of the window, at least one. +fn page_rows(window_rows: usize) -> i32 { + (window_rows as i32 * 3 / 4).max(1) +} + fn grid_move(state: &ClientState, kind: GridMove) -> Option> { let GridExtent { rows, cols } = state.grid.extent; let (window_rows, window_cols) = state.window; @@ -641,7 +646,7 @@ fn grid_move(state: &ClientState, kind: GridMove) -> Option> { GridMove::ToEnd(true) => (row_max as usize, cursor.selected.1), GridMove::ToEnd(false) => (cursor.selected.0, col_max as usize), GridMove::Page(dir) => { - let delta = (window_rows as i32 * 3 / 4).max(1) * dir; + let delta = page_rows(window_rows) * dir; ((row + delta).clamp(0, row_max) as usize, cursor.selected.1) } }; @@ -2366,16 +2371,21 @@ mod tests { bound_commands(keymap, &mut bound); bound.sort(); bound.dedup(); - // From (1,1) of a 3x3 grid any step of at least one lands at 1+d clamped, - // so a delta's sign is visible; a jump's destination is its name. - let step = |d: &String| (1 + d.parse::().expect("a delta")).clamp(0, 2) as usize; + // Far from every edge of a grid far larger than any step, so a landing + // shows magnitude as well as sign (improvise-5kdr); a jump's is its name. + const START: (usize, usize) = (10, 10); + const WINDOW: (usize, usize) = (8, 8); + let d = |a: &String| a.parse::().expect("a delta"); + let at = |base: usize, by: isize| base.checked_add_signed(by).expect("inside the grid"); + let page = page_rows(WINDOW.0) as isize; let destination = |name: &str, args: &[String]| match name { - "move-selection" => Some((step(&args[0]), step(&args[1]))), - "scroll-rows" | "page-scroll" => Some((step(&args[0]), 1)), - "jump-first-row" => Some((0, 1)), - "jump-last-row" => Some((2, 1)), - "jump-first-col" => Some((1, 0)), - "jump-last-col" => Some((1, 2)), + "move-selection" => Some((at(START.0, d(&args[0])), at(START.1, d(&args[1])))), + "scroll-rows" => Some((at(START.0, d(&args[0])), START.1)), + "page-scroll" => Some((at(START.0, page * d(&args[0])), START.1)), + "jump-first-row" => Some((0, START.1)), + "jump-last-row" => Some((20, START.1)), + "jump-first-col" => Some((START.0, 0)), + "jump-last-col" => Some((START.0, 20)), _ => None, }; let mut unstated = Vec::new(); @@ -2389,6 +2399,13 @@ mod tests { } for sequence in sequences { let mut state = furnished(AppMode::Normal); + state.grid.extent = GridExtent { rows: 21, cols: 21 }; + state.window = WINDOW; + *state.view.cursor_mut() = crate::ViewCursor { + selected: START, + row_offset: 5, + col_offset: 5, + }; let view = state.view.active_view.clone(); for key in &sequence { if let KeyOutcome::Local(commands) = resolve(&state, *key) { @@ -2401,9 +2418,12 @@ mod tests { } match to { Some(to) => { - assert_eq!(landed, to, "{sequence:?} runs `{name} {args:?}` from (1,1)") + assert_eq!( + landed, to, + "{sequence:?} runs `{name} {args:?}` from {START:?}" + ) } - None if state.view.active_view == view && landed != (1, 1) => { + None if state.view.active_view == view && landed != START => { unstated.push(format!("{name} {args:?} -> {landed:?}")) } None => {} From f75b58af914b002fafde20c1f13fcd548676ecfb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:41:06 -0700 Subject: [PATCH 26/28] The drawn keys rows must equal what the builders asked for, in order (aszt) A count let a bypass that also drops a real row pass; content does not. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/protocol.md | 11 +++++++++++ crates/improvise-protocol/src/help.rs | 21 ++++++++++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/context/test-evidence/protocol.md b/context/test-evidence/protocol.md index 16ab17b4..aa7d04d9 100644 --- a/context/test-evidence/protocol.md +++ b/context/test-evidence/protocol.md @@ -1180,3 +1180,14 @@ shadowing parameter. So this test states the premise directly — the number of keys rows drawn equals the number of rows the builders asked for (every asked row finds its keys, by `every_keys_row_the_pages_ask_for_finds_its_keys`) — and the `self::keys` bypass fails it `51 != 50`. + +**improvise-aszt: a count is not the premise.** The test above compared +*counts*, so a bypass that also drops one real row keeps them equal. Injected in +`editing()`: call the parameter `keys(e, "clear-buffer", …)` and discard it with +`let _ =`, then return `self::keys(n, "clear-cell", None, "Edit the current +cell")` in its place. The asked row is recorded and never drawn, the drawn row +is never recorded, 50 = 50, and the whole workspace stayed green. The test now +compares **content in order**: the drawn keys rows must equal +`help_bindings()` looked up through the same `keys`, row for row. The same +injection now fails. What still passes is a bypass that draws exactly what the +builders asked for, in the order they asked — which is harmless by definition. diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 9382a557..117f4fde 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -1027,12 +1027,23 @@ mod tests { /// parameter is invisible to every guard. See `context/test-evidence/protocol.md`. #[test] fn every_keys_row_drawn_is_one_the_builders_asked_for() { - let drawn = pages() - .iter() - .flat_map(|page| page.lines.iter()) + let drawn: Vec = pages() + .into_iter() + .flat_map(|page| page.lines) .filter(|line| matches!(line, HelpLine::Keys(_))) - .count(); - assert_eq!(drawn, help_bindings().len()); + .collect(); + // Content, in order, not a count: a bypass that also drops a real row + // keeps the count and changes this (improvise-aszt). + let asked: Vec = help_bindings() + .iter() + .filter_map(|(mode, command, args, description)| { + let args: Option> = args + .as_ref() + .map(|a| a.iter().map(String::as_str).collect()); + keys(mode, command, args.as_deref(), description) + }) + .collect(); + assert_eq!(drawn, asked); } /// A row whose lookup finds no key is dropped silently, so a broken lookup From cfcc9bff973a5da9aac291c57d4f4137f20c9c1a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:43:14 -0700 Subject: [PATCH 27/28] Correct the determinism law's width-guard and analyzer claims (improvise-b9f2) --- context/code/command.md | 21 +++++++++++++++------ context/test-evidence/command.md | 11 ++++++++--- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 63942b96..d77da9a6 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3276,9 +3276,12 @@ yank, panels and cursor. against one `CmdContext`. Every `HashMap` it reached was the same instance with the same `RandomState`, so it iterated the same way both times: `f(x) == f(x)`, which nothing can fail. Separately built `App`s give every map its own seed. So a -command whose effects follow hash iteration order disagrees with itself. That is -the one kind of nondeterminism this repo has actually shipped (gotcha 18: the -drill snapshot in hash order, the analyzer's `HashSet`). The companion test +command whose effects follow hash iteration order disagrees with itself. Hash +order is the one kind of nondeterminism this repo has actually shipped (gotcha +18). This law can see it where a command's `execute` builds it, as the drill +snapshot's record order was. It cannot see the other shipped instance, the +analyzer's `HashSet`: `analyze_records` is reached only from inside +`Effect::apply`, which is outside this law (see below). The companion test `only_distinct_instances_can_see_a_command_that_follows_hash_order` keeps that property checked: a test-only command that emits `ctx.buffers` in iteration order must agree with itself on one context and disagree across two. @@ -3304,9 +3307,15 @@ the whole slice of records under that item, not one record picked by name. In the demo that is four records and eight cells for each of the eight drills. The test also asserts that the widest candidate it actually passes matches at least four -records. That guard reads the argument list the law uses, not a copy of it, so -dropping the derived drills fails it, and so does a demo with too little data. -With the store's sort removed, the law now fails 400 of 400 runs. +records. That guard catches a demo with too little data, and it catches the +derived drills being dropped from the argument list. It does **not** catch the +comparison losing its wide drills some other way, because it measures what the +argument lists match, not what the built commands ordered (improvise-b9f2). The +review showed this with a rival: a `drill-into-cell` parser that refuses fewer +than two arguments sends every one-coordinate drill to the one-record cursor +fallback. The guard still reports four records, and with the store's sort +removed the law passed 38 of 300 runs. With the store's sort removed and the +parser intact, the law fails 400 of 400 runs. **What it cannot see.** Only commands and arguments the fixture reaches. A command whose nondeterminism needs some other state, such as records mode, a diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 62e797e6..cd28229f 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5375,9 +5375,14 @@ rivals it catches: still pin the store directly. - **The derived drills dropped from the argument list** (the `all.extend(one_coordinate_drills(model))` line commented out): the test - fails before comparing anything, with "the widest drill orders 1 records". The - width guard reads the same argument list the comparison uses, so it cannot - pass while the comparison has lost its wide drills. + fails before comparing anything, with "the widest drill orders 1 records". + That is the only way of losing the wide drills the guard can see. It measures + what the argument lists match, not what the built commands ordered, so it can + pass while the comparison has lost its wide drills (improvise-b9f2). Review + rival: `drill-into-cell`'s parser changed to refuse fewer than two arguments, + which sends every one-coordinate drill to the one-record cursor fallback. The + guard stays silent and, with gotcha 18 re-injected, the law passed 38 of 300 + runs. - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". From aadf5ddd354f6659e47393d9eaf496040243cb40 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:57:58 -0700 Subject: [PATCH 28/28] Drop --locked from the float_roundtrip check: --no-deps never reads the lockfile Review of #202 showed `cargo metadata --no-deps --locked` exits 0 against a stale lockfile, so the flag promised a check it does not make. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 2 +- tests/json_numbers.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 8d350768..cd8ff234 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -687,7 +687,7 @@ cannot see this rival. ### `every_crate_that_can_parse_json_enables_float_roundtrip` It replaces a check over three crates listed by hand. It reads -`cargo metadata --no-deps --locked` and fails naming every workspace package, and +`cargo metadata --no-deps` and fails naming every workspace package, and every target-specific entry, whose **normal** serde_json dependency lacks `float_roundtrip`. The rule and why it was chosen over the alternatives are in `context/code/protocol.md`. It is red at `1158c23`, naming `improvise`, diff --git a/tests/json_numbers.rs b/tests/json_numbers.rs index e11985fb..506a59ec 100644 --- a/tests/json_numbers.rs +++ b/tests/json_numbers.rs @@ -38,7 +38,7 @@ fn lossy_json_dependencies(metadata: &Value) -> (Vec, Vec) { fn every_crate_that_can_parse_json_enables_float_roundtrip() { let out = std::process::Command::new(env!("CARGO")) .current_dir(env!("CARGO_MANIFEST_DIR")) - .args(["metadata", "--no-deps", "--locked", "--format-version", "1"]) + .args(["metadata", "--no-deps", "--format-version", "1"]) .output() .expect("cargo metadata must run"); assert!(