From 1c4a27243d067a89c9cb147866754712ce0e7dcb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:07:02 -0700 Subject: [PATCH 1/4] 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 2/4] 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 9512a2db1f38138daf6a011d33159f43a588df35 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:39:36 -0700 Subject: [PATCH 3/4] 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 4/4] 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