From a9346a0be21c58fd54d0866edb874c0ccec9f80a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:52:13 -0700 Subject: [PATCH 01/29] WIP improvise-gw18/yk2s: writers refuse a collapsed group's fold CmdContext::writable_cell_key refuses a fold, and the registry's cursor fallback for clear-cell, commit, paste and drill-into-cell goes through it; yank alone reads. H refuses on a fold row via GridLayout::row_folds_a_group. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/core.rs | 14 +++++ .../improvise-command/src/command/cmd/grid.rs | 62 +++++++++++++++++++ .../improvise-command/src/command/cmd/mod.rs | 55 ++++++++++++++++ .../src/command/cmd/registry.rs | 10 ++- .../improvise-command/src/command/keymap.rs | 44 +++++++++++++ crates/improvise-core/src/view/layout.rs | 7 ++- 6 files changed, 189 insertions(+), 3 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 491718cb..df76d7c7 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -10,6 +10,9 @@ use crate::ui::effect::{Effect, Panel}; use crate::view::{Axis, GridLayout, View}; use crate::workbook::Workbook; +/// What a command says when the cursor sits on a collapsed group's row (§14). +pub const FOLD_REFUSAL: &str = "Collapsed group — press z to expand it first"; + /// Read-only context available to commands for decision-making: a `&Model`, the /// active `&View`, and the `&Workbook` for the rare command that enumerates views. pub struct CmdContext<'a> { @@ -62,10 +65,21 @@ impl<'a> CmdContext<'a> { self.layout.is_records_mode() } + /// The cell under the cursor, for a command that READS it. A writer asks + /// [`Self::writable_cell_key`], which refuses a fold (core.md §14). pub fn cell_key(&self) -> Option { self.layout.cell_key(self.selected.0, self.selected.1) } + /// The cell under the cursor for a command that writes or drills it. A fold is + /// refused: its key names a group, which no record carries (core.md §14). + pub fn writable_cell_key(&self) -> Result { + match self.layout.folds_a_group(self.selected.0, self.selected.1) { + true => Err(FOLD_REFUSAL.to_string()), + false => self.cell_key().ok_or_else(|| "no cell at cursor".to_string()), + } + } + /// The formula that owns the cell under the cursor, if one does. In records /// mode the synthetic key names no measure, so the record's key is resolved. pub fn owning_formula(&self) -> Option<&'a crate::formula::Formula> { diff --git a/crates/improvise-command/src/command/cmd/grid.rs b/crates/improvise-command/src/command/cmd/grid.rs index 780d8e95..8ed82599 100644 --- a/crates/improvise-command/src/command/cmd/grid.rs +++ b/crates/improvise-command/src/command/cmd/grid.rs @@ -11,6 +11,64 @@ mod tests { use super::*; use crate::command::cmd::test_helpers::*; + fn press(app: &mut crate::ui::app::App, c: char) { + use crate::command::key::{KeyCode, KeyEvent, KeyModifiers}; + app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE)) + .unwrap(); + } + + /// improvise-gw18 — see context/test-evidence/command.md. + #[test] + fn drilling_into_a_fold_is_refused_and_o_after_it_writes_no_group_coordinate() { + let mut app = app_on_a_fold(); + let origin = app.view_state.active_view.clone(); + press(&mut app, '>'); + assert_eq!(app.view_state.active_view, origin, "no drill was opened"); + assert!( + app.view_state.status_msg.contains("ollapsed group"), + "the refusal is out loud, got: {:?}", + app.view_state.status_msg + ); + press(&mut app, 'o'); + + let (text, back) = saved_and_reloaded(&app.model_state.workbook); + let data = text.split("## Data").nth(1).expect("a data section"); + assert!(!data.contains("Q2"), "a group coordinate was saved:\n{text}"); + assert_eq!(non_item_coordinates(&back), Vec::<(String, String)>::new()); + } + + /// The control for the refusal above: the same key one row up drills. + #[test] + fn drilling_into_the_row_above_a_fold_still_opens_a_drill() { + let mut app = app_on_a_fold(); + app.cursor_mut().selected = (1, 1); + let origin = app.view_state.active_view.clone(); + press(&mut app, '>'); + assert_ne!(app.view_state.active_view, origin, "Feb × Cost drills"); + } + + /// improvise-yk2s — see context/test-evidence/command.md. + #[test] + fn hiding_a_folds_row_is_refused_and_persists_no_hidden_group() { + let mut app = app_on_a_fold(); + press(&mut app, 'H'); + let view = app.model_state.workbook.active_view(); + assert!(!view.is_hidden("Month", "Q2"), "no ghost hidden entry"); + assert!( + app.view_state.status_msg.contains("ollapsed group"), + "the refusal is out loud, got: {:?}", + app.view_state.status_msg + ); + let (text, _) = saved_and_reloaded(&app.model_state.workbook); + assert!(!text.contains("hidden:"), "a hidden entry was saved:\n{text}"); + + // The control: the row above is an item, and H hides it. + app.cursor_mut().selected = (1, 1); + press(&mut app, 'H'); + let view = app.model_state.workbook.active_view(); + assert!(view.is_hidden("Month", "Feb"), "H on Feb hides Feb"); + } + #[test] fn toggle_group_under_cursor_returns_empty_without_groups() { let m = two_cat_model(); @@ -505,6 +563,10 @@ impl Cmd for HideSelectedRowItem { let Some(cat_name) = ctx.layout.row_cats.first().cloned() else { return vec![]; }; + // A fold's row names a group, not an item, so there is nothing to hide (§14). + if ctx.layout.row_folds_a_group(ctx.selected.0) { + return vec![effect::refuse(super::core::FOLD_REFUSAL)]; + } let sel_row = ctx.selected.0; let Some(items) = ctx .layout diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index 0000d0fd..f895d64d 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -124,4 +124,59 @@ pub(super) mod test_helpers { pub fn make_registry() -> CmdRegistry { default_registry() } + + /// Two quarters of months on Row, two measures on Column, every cell funded, + /// Q2 collapsed: rows `[Jan, Feb, Q2]`, and the cursor on the fold at `(2, 1)`. + pub fn app_on_a_fold() -> crate::ui::app::App { + use crate::model::cell::{CellKey, CellValue}; + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Month").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] { + wb.add_item_in_group("Month", month, quarter).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + for (i, month) in ["Jan", "Feb", "Apr", "May"].into_iter().enumerate() { + for measure in ["Revenue", "Cost"] { + let key = CellKey::new(vec![ + ("Month".into(), month.into()), + ("_Measure".into(), measure.into()), + ]); + wb.model.set_cell(key, CellValue::Number(10.0 + i as f64)); + } + } + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().toggle_group_collapse("Month", "Q2"); + let mut app = crate::ui::app::App::new(wb, None); + app.rebuild_layout(); + app.cursor_mut().selected = (2, 1); + assert!(app.layout.folds_a_group(2, 1), "setup: the cursor is on the fold"); + assert_eq!(app.layout.row_label(2), "Q2", "setup: the fold is Q2's row"); + app + } + + /// Every stored coordinate naming no item of its category — what a write to a + /// fold leaves behind (core.md §14). + pub fn non_item_coordinates(wb: &Workbook) -> Vec<(String, String)> { + let model = &wb.model; + model + .data + .iter_cells() + .flat_map(|(key, _)| key.0.clone()) + .filter(|(cat, item)| { + !model + .category(cat) + .is_some_and(|c| c.items.contains_key(item.as_str())) + }) + .collect() + } + + /// The document saved and read back: what reaches the file, not the store. + pub fn saved_and_reloaded(wb: &Workbook) -> (String, Workbook) { + let text = crate::persistence::format_md(wb); + let back = crate::persistence::parse_md(&text).expect("the saved text reloads"); + (text, back) + } } diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 6ef243dc..75418f3d 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -23,8 +23,14 @@ use super::tile::*; use super::wizard::*; /// The cell under the cursor: what a coordinate command falls back to when given none. +/// Every such command writes or drills, so a fold is refused (core.md §14). fn cursor_key(ctx: &CmdContext) -> Result { - ctx.cell_key().clone().ok_or("no cell at cursor".into()) + ctx.writable_cell_key() +} + +/// [`cursor_key`] for the one command that only READS the cell: `yank`. +fn cursor_key_to_read(ctx: &CmdContext) -> Result { + ctx.cell_key().ok_or("no cell at cursor".into()) } /// A `Move` built from args takes the live cursor as its origin. @@ -249,7 +255,7 @@ pub fn default_registry() -> CmdRegistry { as_parsed, Some(|_, ctx| { Ok(YankCell { - key: cursor_key(ctx)?, + key: cursor_key_to_read(ctx)?, }) }), ); diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index d98b693c..189d733c 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -477,6 +477,50 @@ mod tests { out } + /// improvise-gw18: every Normal key, prefixes walked, pressed on a fold with a + /// value yanked — see context/test-evidence/command.md. + #[test] + fn no_normal_mode_key_pressed_on_a_fold_stores_a_group_coordinate() { + use improvise_protocol::key::KeyEvent; + let ks = KeymapSet::default_keymaps(); + let normal = ks.maps().get(&ModeKey::Normal).expect("a Normal keymap").clone(); + let mods = [ + KeyModifiers::NONE, + KeyModifiers::SHIFT, + KeyModifiers::CONTROL, + KeyModifiers::ALT, + ]; + let mut strokes: Vec> = Vec::new(); + for key in probe_keys() { + for m in mods { + match normal.lookup(key, m) { + Some(Binding::Prefix(sub)) => strokes.extend( + probe_keys() + .into_iter() + .flat_map(|k2| mods.map(|m2| (k2, m2))) + .filter(|&(k2, m2)| sub.lookup(k2, m2).is_some()) + .map(|second| vec![(key, m), second]), + ), + Some(_) => strokes.push(vec![(key, m)]), + None => {} + } + } + } + assert!( + strokes.iter().any(|s| s == &[(KeyCode::Char('p'), KeyModifiers::NONE)]), + "the sweep reaches paste" + ); + for stroke in &strokes { + let mut app = test_helpers::app_on_a_fold(); + app.view_state.yanked = Some(crate::model::cell::CellValue::Number(5.0)); + for &(key, m) in stroke { + let _ = app.handle_key(KeyEvent::new(key, m)); + } + let stray = test_helpers::non_item_coordinates(&app.model_state.workbook); + assert!(stray.is_empty(), "{stroke:?} on a fold stored {stray:?}"); + } + } + /// A representative `AppMode` for each keymap, so a test can put a /// `CmdContext` in the mode whose bindings it is walking. fn app_mode_for(mode: &ModeKey) -> AppMode { diff --git a/crates/improvise-core/src/view/layout.rs b/crates/improvise-core/src/view/layout.rs index 495c3967..4b9efd11 100644 --- a/crates/improvise-core/src/view/layout.rs +++ b/crates/improvise-core/src/view/layout.rs @@ -750,7 +750,12 @@ impl GridLayout { /// Whether the cell at `(row, col)` folds a collapsed group: an aggregate /// with no record under it. The one home of that answer (§14). pub fn folds_a_group(&self, row: usize, col: usize) -> bool { - folds(&self.row_items, row) || folds(&self.col_items, col) + self.row_folds_a_group(row) || folds(&self.col_items, col) + } + + /// The row half of [`Self::folds_a_group`], for a command about the row alone. + pub fn row_folds_a_group(&self, row: usize) -> bool { + folds(&self.row_items, row) } /// The first data row of `cat`/`group`'s run under `prefix` — where a toggle From 90b93d48747eb0dfd4e268e862046d71b1147fd5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:59:10 -0700 Subject: [PATCH 02/29] WIP improvise-lx7w/54g1/l4mp: clear and paste land where the browser's CellId says CmdContext::value_cell_key answers the cursor through project::cell_id, the question the browser is given: only a Record is written, a records position resolves to its real record, and a fold, an aggregate, a formula-owned cell, a coordinate or creating column and a drill are each declined out loud. clear-cell and paste take that door; commit and drill take the fold-only one. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/cell.rs | 109 ++++++++++++++++-- .../improvise-command/src/command/cmd/core.rs | 57 ++++++++- .../src/command/cmd/registry.rs | 15 ++- crates/improvise-command/src/ui/project.rs | 12 +- 4 files changed, 168 insertions(+), 25 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/cell.rs b/crates/improvise-command/src/command/cmd/cell.rs index a68c30b8..39f84a3c 100644 --- a/crates/improvise-command/src/command/cmd/cell.rs +++ b/crates/improvise-command/src/command/cmd/cell.rs @@ -3,7 +3,9 @@ use crate::model::cell::{CellKey, CellValue}; use crate::ui::effect::{self, Effect, ViewTarget}; use crate::view::synthetic_record_info; -use super::core::{Cmd, CmdContext}; +use super::core::{ + AGGREGATE_REFUSAL, Cmd, CmdContext, INSIDE_A_DRILL_REFUSAL, NO_VALUE_REFUSAL, +}; #[cfg(test)] mod tests { @@ -332,9 +334,9 @@ mod tests { app.cursor_mut().selected = (0, col_of(&app, "Revenue")); press(&mut app, 'p'); let in_drill = app.view_state.status_msg.clone(); - assert_eq!( - in_drill, PASTE_INSIDE_DRILL, - "setup: the drilled paste declines" + assert!( + in_drill.ends_with(INSIDE_A_DRILL_REFUSAL), + "setup: the drilled paste declines, got {in_drill:?}" ); assert!( !advice.to_lowercase().contains("drill"), @@ -389,6 +391,96 @@ mod tests { let dbg = effects_debug(&effects); assert!(dbg.contains("Save"), "Expected Save, got: {dbg}"); } + + /// improvise-lx7w — see context/test-evidence/command.md. + #[test] + fn x_in_records_mode_clears_the_real_record_under_the_cursor() { + let mut app = records_app(); + app.cursor_mut().selected = (1, col_of(&app, "Cost")); + press(&mut app, 'x'); + let wb = &app.model_state.workbook; + assert_eq!(wb.model.get_cell(&record("West", "Cost")), None, "West/Cost"); + assert_eq!( + wb.model.get_cell(&record("East", "Cost")), + Some(&CellValue::Number(700.0)), + "the neighbouring record is untouched" + ); + let (text, back) = saved_and_reloaded(wb); + assert!(!text.contains("West, _Measure=Cost"), "saved:\n{text}"); + assert_eq!(back.model.get_cell(&record("West", "Cost")), None); + assert_eq!(synthetic_cells(&back), Vec::::new()); + } + + /// improvise-lx7w: a coordinate column stores no value, so `x` declines there. + #[test] + fn x_on_a_records_coordinate_column_changes_nothing_and_says_so() { + let mut app = records_app(); + let before = crate::persistence::format_md(&app.model_state.workbook); + app.cursor_mut().selected = (1, col_of(&app, "Region")); + press(&mut app, 'x'); + assert_eq!(crate::persistence::format_md(&app.model_state.workbook), before); + assert!( + app.view_state.status_msg.ends_with(NO_VALUE_REFUSAL), + "declined out loud, got {:?}", + app.view_state.status_msg + ); + } + + /// Two regions by two raw measures and `Profit = Revenue - Cost`, the formula + /// in the third column, so the cursor on it is off row 0 and column 0. + fn formula_app() -> App { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.add_item("Region", "West").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + wb.model.add_formula( + crate::formula::parse_formula("Profit = Revenue - Cost", "_Measure").unwrap(), + ); + wb.active_view_mut() + .set_axis("Region", crate::view::Axis::Row); + wb.active_view_mut() + .set_axis("_Measure", crate::view::Axis::Column); + for (region, measure, n) in [ + ("East", "Revenue", 10.0), + ("East", "Cost", 4.0), + ("West", "Revenue", 9.0), + ("West", "Cost", 2.0), + ] { + wb.model + .set_cell(record(region, measure), CellValue::Number(n)); + } + let mut app = App::new(wb, None); + app.view_state.yanked = Some(CellValue::Number(99.0)); + app + } + + /// improvise-54g1 — see context/test-evidence/command.md. + #[test] + fn p_onto_a_formula_owned_cell_writes_nothing_and_names_the_formula() { + let mut app = formula_app(); + app.cursor_mut().selected = (1, 2); + assert_eq!(app.layout.col_label(2), "Profit", "setup: on the formula"); + press(&mut app, 'p'); + let (text, back) = saved_and_reloaded(&app.model_state.workbook); + assert!(!text.contains("_Measure=Profit"), "saved under the formula:\n{text}"); + assert_eq!(back.model.get_cell(&record("West", "Profit")), None); + assert!( + app.view_state.status_msg.contains("Profit is computed by a formula"), + "declined out loud, got {:?}", + app.view_state.status_msg + ); + + // The control: one column left is a stored cell, and p writes it. + app.cursor_mut().selected = (1, 1); + press(&mut app, 'p'); + let wb = &app.model_state.workbook; + assert_eq!( + wb.model.get_cell(&record("West", "Cost")), + Some(&CellValue::Number(99.0)) + ); + } } // ── Cell operations ────────────────────────────────────────────────────────── @@ -454,7 +546,7 @@ impl Cmd for PasteCell { fn paste_into(ctx: &CmdContext, key: &CellKey, value: CellValue) -> Box { match synthetic_record_info(key) { Some((row, col)) => paste_into_record(ctx, row, col, value), - None if aggregates(ctx, key) => effect::set_status(PASTE_ONTO_AGGREGATE), + None if aggregates(ctx, key) => effect::set_status(AGGREGATE_REFUSAL), None => Box::new(effect::SetCell(key.clone(), value)), } } @@ -466,9 +558,9 @@ fn paste_into_record( value: CellValue, ) -> Box { match (ctx.layout.record_cell_key(row, col), ctx.has_drill_state) { - (Some(_), true) => effect::set_status(PASTE_INSIDE_DRILL), + (Some(_), true) => effect::set_status(INSIDE_A_DRILL_REFUSAL), (Some(real), false) => Box::new(effect::SetCell(real, value)), - (None, _) => effect::set_status(PASTE_ONTO_NO_VALUE), + (None, _) => effect::set_status(NO_VALUE_REFUSAL), } } @@ -479,9 +571,6 @@ fn aggregates(ctx: &CmdContext, key: &CellKey) -> bool { }) } -const PASTE_ONTO_AGGREGATE: &str = "Cannot paste onto an aggregate — it combines several records, so there is no one cell to write"; -const PASTE_ONTO_NO_VALUE: &str = "Paste lands only on a measure column"; -const PASTE_INSIDE_DRILL: &str = "Paste is not staged inside a drill — edit the cell instead"; // ── View commands ──────────────────────────────────────────────────────────── diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index df76d7c7..826b70cf 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -9,9 +9,21 @@ use crate::ui::app::AppMode; use crate::ui::effect::{Effect, Panel}; use crate::view::{Axis, GridLayout, View}; use crate::workbook::Workbook; +use improvise_protocol::CellId; /// What a command says when the cursor sits on a collapsed group's row (§14). pub const FOLD_REFUSAL: &str = "Collapsed group — press z to expand it first"; +const NO_CELL: &str = "no cell at cursor"; +pub const AGGREGATE_REFUSAL: &str = + "Cannot write to an aggregate — it combines several records, so there is no one cell to write"; +pub const NO_VALUE_REFUSAL: &str = "Only a measure column holds a value"; +pub const INSIDE_A_DRILL_REFUSAL: &str = + "A value is not staged inside a drill — edit the cell instead"; + +/// What a command says when the cell it would write is a formula's result. +pub fn derived_refusal(target: &str) -> String { + format!("{target} is computed by a formula — edit the formula, not the cell") +} /// Read-only context available to commands for decision-making: a `&Model`, the /// active `&View`, and the `&Workbook` for the rare command that enumerates views. @@ -65,21 +77,54 @@ impl<'a> CmdContext<'a> { self.layout.is_records_mode() } - /// The cell under the cursor, for a command that READS it. A writer asks - /// [`Self::writable_cell_key`], which refuses a fold (core.md §14). + /// The cell under the cursor, for a command that only READS it. The writers' + /// doors: `context/code/command.md`, "The cursor's cell has three doors". pub fn cell_key(&self) -> Option { self.layout.cell_key(self.selected.0, self.selected.1) } - /// The cell under the cursor for a command that writes or drills it. A fold is - /// refused: its key names a group, which no record carries (core.md §14). - pub fn writable_cell_key(&self) -> Result { + /// The cursor's key for a command that resolves it itself (commit, drill), + /// refusing a fold: its key names a group, which no record carries (§14). + pub fn unfolded_cell_key(&self) -> Result { match self.layout.folds_a_group(self.selected.0, self.selected.1) { true => Err(FOLD_REFUSAL.to_string()), - false => self.cell_key().ok_or_else(|| "no cell at cursor".to_string()), + false => self.cell_key().ok_or_else(|| NO_CELL.to_string()), } } + /// The stored cell a VALUE written at the cursor lands on, answered by the + /// `CellId` the browser is given: `Record` is the one settable arm (ADR 002). + pub fn value_cell_key(&self) -> Result { + let (row, col) = self.selected; + match crate::ui::project::cell_id(self.model, self.layout, row, col) { + Some(CellId::Record(_)) => self.stored_cell_key(), + Some(CellId::Aggregate(_)) => Err(match self.layout.folds_a_group(row, col) { + true => FOLD_REFUSAL, + false => AGGREGATE_REFUSAL, + } + .to_string()), + Some(CellId::Derived { target }) => Err(derived_refusal(&target)), + Some(CellId::Create(_)) => Err(NO_VALUE_REFUSAL.to_string()), + None => Err(match self.cell_key() { + Some(_) => NO_VALUE_REFUSAL, + None => NO_CELL, + } + .to_string()), + } + } + + /// A `Record`'s real key: a records position resolves through its row, and a + /// drill's records are staged, never written directly (gotcha 1, 19). + fn stored_cell_key(&self) -> Result { + let (row, col) = self.selected; + let stored = match (self.is_records_mode(), self.has_drill_state) { + (true, true) => return Err(INSIDE_A_DRILL_REFUSAL.to_string()), + (true, false) => self.layout.record_cell_key(row, col), + (false, _) => self.cell_key(), + }; + stored.ok_or_else(|| NO_CELL.to_string()) + } + /// The formula that owns the cell under the cursor, if one does. In records /// mode the synthetic key names no measure, so the record's key is resolved. pub fn owning_formula(&self) -> Option<&'a crate::formula::Formula> { diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 75418f3d..72c049ce 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,10 +22,15 @@ use super::text_buffer::*; use super::tile::*; use super::wizard::*; -/// The cell under the cursor: what a coordinate command falls back to when given none. -/// Every such command writes or drills, so a fold is refused (core.md §14). +/// The cell under the cursor, for a command that resolves it itself (commit, +/// drill). Which command takes which door: `context/code/command.md`. fn cursor_key(ctx: &CmdContext) -> Result { - ctx.writable_cell_key() + ctx.unfolded_cell_key() +} + +/// The stored cell a value lands on, for `clear-cell` and `paste`. +fn cursor_value_key(ctx: &CmdContext) -> Result { + ctx.value_cell_key() } /// [`cursor_key`] for the one command that only READS the cell: `yank`. @@ -99,7 +104,7 @@ pub fn default_registry() -> CmdRegistry { as_parsed, Some(|_, ctx| { Ok(ClearCellCommand { - key: cursor_key(ctx)?, + key: cursor_value_key(ctx)?, }) }), ); @@ -274,7 +279,7 @@ pub fn default_registry() -> CmdRegistry { as_parsed, Some(|_, ctx| { Ok(PasteCell { - key: cursor_key(ctx)?, + key: cursor_value_key(ctx)?, }) }), ); diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 1fe8526e..cce7daf3 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -281,7 +281,7 @@ fn cell_display( text, numeric: matches!(value, Some(CellValue::Number(_))), error: matches!(value, Some(CellValue::Error(_))), - id: cell_id(app, layout, row, col), + id: cell_id(&app.model_state.workbook.model, layout, row, col), value: value.clone(), }; } @@ -290,7 +290,7 @@ fn cell_display( text: plan.at(key.as_ref()).value(value.as_ref()), numeric: matches!(value, Some(CellValue::Number(_))), error: matches!(value, Some(CellValue::Error(_))), - id: cell_id(app, layout, row, col), + id: cell_id(&app.model_state.workbook.model, layout, row, col), // The *unformatted* value the TUI seeds its edit buffer from. Projecting it // beats a client un-formatting `text` (gotcha 47). edit_text: key.map(|key| app.display_value_in(layout, &key)), @@ -300,8 +300,12 @@ fn cell_display( /// What the cell at `(row, col)` *is*, as the surrogate the client echoes back. The /// fork is `EditOrDrill`'s, asked the same way — see `context/code/command.md`. -fn cell_id(app: &App, layout: &GridLayout, row: usize, col: usize) -> Option { - let model = &app.model_state.workbook.model; +pub(crate) fn cell_id( + model: &Model, + layout: &GridLayout, + row: usize, + col: usize, +) -> Option { if layout.is_records_mode() { // A measure can be BOTH an explicit `_Measure` item and a formula target, // so the document is asked about the record's real key (improvise-ir7l). From aa959343a297157ec6769d22183f699172ffccdb Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:04:35 -0700 Subject: [PATCH 03/29] WIP: commit refuses a fold and a formula's result; lockstep for x, p and folds Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/cell.rs | 21 ++++ .../improvise-command/src/command/cmd/core.rs | 35 ++++-- .../improvise-command/src/command/cmd/grid.rs | 19 ++++ .../improvise-command/src/command/cmd/mode.rs | 5 +- .../src/command/cmd/registry.rs | 6 +- .../improvise-wasm-client/tests/lockstep.rs | 106 ++++++++++++++++++ 6 files changed, 178 insertions(+), 14 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/cell.rs b/crates/improvise-command/src/command/cmd/cell.rs index 39f84a3c..36e4a34f 100644 --- a/crates/improvise-command/src/command/cmd/cell.rs +++ b/crates/improvise-command/src/command/cmd/cell.rs @@ -481,6 +481,27 @@ mod tests { Some(&CellValue::Number(99.0)) ); } + + /// The second door onto a formula cell: Tab advances the editor onto it, where + /// `i` would have declined. See context/test-evidence/command.md. + #[test] + fn tab_advancing_onto_a_formula_cell_commits_nothing_under_it() { + let mut app = formula_app(); + app.cursor_mut().selected = (1, 1); + press(&mut app, 'i'); + assert!(app.view_state.mode.is_editing(), "setup: editing West/Cost"); + app.handle_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) + .unwrap(); + assert_eq!(app.cursor().selected, (1, 2), "setup: Tab moved onto Profit"); + for c in "7".chars() { + press(&mut app, c); + } + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + let (text, back) = saved_and_reloaded(&app.model_state.workbook); + assert!(!text.contains("_Measure=Profit"), "saved under the formula:\n{text}"); + assert_eq!(back.model.get_cell(&record("West", "Profit")), None); + } } // ── Cell operations ────────────────────────────────────────────────────────── diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 826b70cf..ec285fe3 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -92,17 +92,24 @@ impl<'a> CmdContext<'a> { } } + /// The cursor's key for a commit, which resolves a records position itself + /// (gotcha 19). An aggregate or a formula's result is refused, as `i` refuses it. + pub fn committable_cell_key(&self) -> Result { + match self.cursor_cell_id() { + Some(CellId::Aggregate(_)) => Err(self.aggregate_refusal()), + Some(CellId::Derived { target }) => Err(derived_refusal(&target)), + Some(CellId::Record(_) | CellId::Create(_)) | None => { + self.cell_key().ok_or_else(|| NO_CELL.to_string()) + } + } + } + /// The stored cell a VALUE written at the cursor lands on, answered by the /// `CellId` the browser is given: `Record` is the one settable arm (ADR 002). pub fn value_cell_key(&self) -> Result { - let (row, col) = self.selected; - match crate::ui::project::cell_id(self.model, self.layout, row, col) { + match self.cursor_cell_id() { Some(CellId::Record(_)) => self.stored_cell_key(), - Some(CellId::Aggregate(_)) => Err(match self.layout.folds_a_group(row, col) { - true => FOLD_REFUSAL, - false => AGGREGATE_REFUSAL, - } - .to_string()), + Some(CellId::Aggregate(_)) => Err(self.aggregate_refusal()), Some(CellId::Derived { target }) => Err(derived_refusal(&target)), Some(CellId::Create(_)) => Err(NO_VALUE_REFUSAL.to_string()), None => Err(match self.cell_key() { @@ -113,6 +120,20 @@ impl<'a> CmdContext<'a> { } } + /// What the cursor's cell is, asked exactly as the projection asks it. + fn cursor_cell_id(&self) -> Option { + crate::ui::project::cell_id(self.model, self.layout, self.selected.0, self.selected.1) + } + + /// An aggregate's refusal, naming the fold where the aggregate is one (§14). + fn aggregate_refusal(&self) -> String { + match self.layout.folds_a_group(self.selected.0, self.selected.1) { + true => FOLD_REFUSAL, + false => AGGREGATE_REFUSAL, + } + .to_string() + } + /// A `Record`'s real key: a records position resolves through its row, and a /// drill's records are staged, never written directly (gotcha 1, 19). fn stored_cell_key(&self) -> Result { diff --git a/crates/improvise-command/src/command/cmd/grid.rs b/crates/improvise-command/src/command/cmd/grid.rs index 8ed82599..c191daca 100644 --- a/crates/improvise-command/src/command/cmd/grid.rs +++ b/crates/improvise-command/src/command/cmd/grid.rs @@ -37,6 +37,25 @@ mod tests { assert_eq!(non_item_coordinates(&back), Vec::<(String, String)>::new()); } + /// The commit door onto a fold: Enter advances the editor onto it, and the next + /// commit is what must refuse. See context/test-evidence/command.md. + #[test] + fn enter_advancing_onto_a_fold_commits_nothing_there() { + use crate::command::key::{KeyCode, KeyEvent, KeyModifiers}; + let enter = KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE); + let mut app = app_on_a_fold(); + app.cursor_mut().selected = (1, 1); + press(&mut app, 'i'); + app.handle_key(enter).unwrap(); + assert_eq!(app.cursor().selected, (2, 1), "setup: Enter moved onto the fold"); + press(&mut app, '7'); + app.handle_key(enter).unwrap(); + let (text, back) = saved_and_reloaded(&app.model_state.workbook); + let data = text.split("## Data").nth(1).expect("a data section"); + assert!(!data.contains("Q2"), "a group coordinate was saved:\n{text}"); + assert_eq!(non_item_coordinates(&back), Vec::<(String, String)>::new()); + } + /// The control for the refusal above: the same key one row up drills. #[test] fn drilling_into_the_row_above_a_fold_still_opens_a_drill() { diff --git a/crates/improvise-command/src/command/cmd/mode.rs b/crates/improvise-command/src/command/cmd/mode.rs index a3b48f14..f87a2312 100644 --- a/crates/improvise-command/src/command/cmd/mode.rs +++ b/crates/improvise-command/src/command/cmd/mode.rs @@ -380,10 +380,7 @@ impl Cmd for EditOrDrill { // The same question `cell_id` asks, off the same predicate. In records mode // the synthetic key names no measure, so the record's key is resolved. if let Some(formula) = ctx.owning_formula() { - return vec![effect::refuse(format!( - "{} is computed by a formula — edit the formula, not the cell", - formula.target - ))]; + return vec![effect::refuse(super::core::derived_refusal(&formula.target))]; } vec![ Box::new(effect::SetBuffer { diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 72c049ce..acbae9ba 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,8 +22,8 @@ use super::text_buffer::*; use super::tile::*; use super::wizard::*; -/// The cell under the cursor, for a command that resolves it itself (commit, -/// drill). Which command takes which door: `context/code/command.md`. +/// The cell under the cursor for `drill-into-cell`, which opens aggregates but +/// not a fold. Which command takes which door: `context/code/command.md`. fn cursor_key(ctx: &CmdContext) -> Result { ctx.unfolded_cell_key() } @@ -66,7 +66,7 @@ fn commit_at_cursor( Ok(CommitAndAdvance { edit_mode: parse_mode_name(mode)?, value: read_buffer(ctx, "edit"), - key: cursor_key(ctx)?, + key: ctx.committable_cell_key()?, advance, cursor: CursorState::from_ctx(ctx), }) diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 39985d5c..4796ae44 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1830,3 +1830,109 @@ fn yy_over_an_aggregate_copies_the_sum_the_host_copies() { "client and host yanked different values over an aggregate" ); } + +// The cursor's cell as a write target (improvise-lx7w, improvise-54g1, improvise-gw18). + +/// improvise-lx7w: `x` on a records measure clears the real record on both ends. +/// Evidence: `context/test-evidence/clients.md`. +#[test] +fn x_in_records_mode_clears_the_record_the_browser_clears() { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let two_records = || { + let mut app = furnished_host(); + let wb = &mut app.model_state.workbook; + wb.add_item("_Measure", "Cost").unwrap(); + // Two measures, so clearing one leaves South's row where the cursor is. + for (measure, n) in [("Revenue", 900.0), ("Cost", 40.0)] { + let key = north_revenue() + .with("Region", "South") + .with("_Measure", measure); + wb.model + .set_cell(key, improvise_protocol::CellValue::Number(n)); + } + app.rebuild_layout(); + reduce_key(&mut app, r); + app + }; + let (mut app, mut twin) = (two_records(), two_records()); + let revenue = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Revenue") + .expect("a Revenue column"); + assert_eq!( + app.layout.record_row(1).unwrap().coords.get("Region"), + Some("South"), + "setup: row 1 is South" + ); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (1, revenue); + } + let mut client = records_client(&app); + client.view.cursor_mut().selected = (1, revenue); + + both_ends(&mut app, &mut twin, &mut client, "x"); + let model = &app.model_state.workbook.model; + assert_eq!(model.get_cell(&north_revenue().with("Region", "South")), None); + assert!(model.get_cell(&north_revenue()).is_some(), "North is untouched"); +} + +/// `key` at `at` on a host and on a client seeded from it: neither writes. +fn declines_on_both_ends(mut app: App, at: (usize, usize), key: char) { + let event = KeyEvent::new(KeyCode::Char(key), KeyModifiers::NONE); + app.view_state.yanked = Some(improvise_protocol::CellValue::Number(99.0)); + app.cursor_mut().selected = at; + let client = client_ready_to_edit(&app, (6, 6)); + assert!(!composes_a_write(&client, event), "the browser composed a write"); + let before = document_text(&app); + reduce_key(&mut app, event); + assert_eq!(document_text(&app), before, "the TUI wrote where the browser declines"); + assert!(!app.view_state.status_msg.is_empty(), "the TUI declined silently"); +} + +/// improvise-54g1: `p` on a formula-owned cell. The control is the stored cell +/// beside it. Evidence: `context/test-evidence/clients.md`. +#[test] +fn p_onto_a_formula_owned_cell_declines_on_both_ends() { + let app = yank_host(); + assert_eq!(app.layout.col_label(3), "Profit", "setup: column 3 is the formula"); + declines_on_both_ends(app, (1, 3), 'p'); + + let mut app = yank_host(); + app.view_state.yanked = Some(improvise_protocol::CellValue::Number(99.0)); + app.cursor_mut().selected = (1, 2); + let client = client_ready_to_edit(&app, (6, 6)); + let p = KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE); + assert!(composes_a_write(&client, p), "the control: Cost is settable"); +} + +/// improvise-gw18/l4mp: `p` and `x` on a collapsed group's fold, off the origin. +#[test] +fn a_write_onto_a_fold_declines_on_both_ends() { + let fold_host = || { + let mut wb = Workbook::new("Test"); + wb.add_category("Month").unwrap(); + for (month, quarter) in [("Jan", "Q1"), ("Feb", "Q1"), ("Apr", "Q2"), ("May", "Q2")] { + wb.add_item_in_group("Month", month, quarter).unwrap(); + } + wb.add_item("_Measure", "Revenue").unwrap(); + wb.add_item("_Measure", "Cost").unwrap(); + for month in ["Jan", "Feb", "Apr", "May"] { + for measure in ["Revenue", "Cost"] { + let key = improvise_command::model::cell::CellKey::new(vec![ + ("Month".to_string(), month.to_string()), + ("_Measure".to_string(), measure.to_string()), + ]); + wb.model + .set_cell(key, improvise_protocol::CellValue::Number(3.0)); + } + } + wb.active_view_mut().set_axis("Month", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + wb.active_view_mut().toggle_group_collapse("Month", "Q2"); + let mut app = App::new(wb, None); + app.rebuild_layout(); + assert_eq!(app.layout.row_label(2), "Q2", "setup: the fold is row 2"); + app + }; + declines_on_both_ends(fold_host(), (2, 1), 'p'); + declines_on_both_ends(fold_host(), (2, 1), 'x'); +} From 8fb5f6b14aa74826fc7c39b9dba7ca2112841eef Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:05:08 -0700 Subject: [PATCH 04/29] fmt Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/cell.rs | 36 +++++++++++++----- .../improvise-command/src/command/cmd/grid.rs | 21 +++++++++-- .../improvise-command/src/command/cmd/mod.rs | 5 ++- .../improvise-command/src/command/cmd/mode.rs | 4 +- .../improvise-command/src/command/keymap.rs | 10 ++++- .../improvise-wasm-client/tests/lockstep.rs | 37 +++++++++++++++---- 6 files changed, 88 insertions(+), 25 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/cell.rs b/crates/improvise-command/src/command/cmd/cell.rs index 36e4a34f..5675ab5f 100644 --- a/crates/improvise-command/src/command/cmd/cell.rs +++ b/crates/improvise-command/src/command/cmd/cell.rs @@ -3,9 +3,7 @@ use crate::model::cell::{CellKey, CellValue}; use crate::ui::effect::{self, Effect, ViewTarget}; use crate::view::synthetic_record_info; -use super::core::{ - AGGREGATE_REFUSAL, Cmd, CmdContext, INSIDE_A_DRILL_REFUSAL, NO_VALUE_REFUSAL, -}; +use super::core::{AGGREGATE_REFUSAL, Cmd, CmdContext, INSIDE_A_DRILL_REFUSAL, NO_VALUE_REFUSAL}; #[cfg(test)] mod tests { @@ -399,7 +397,11 @@ mod tests { app.cursor_mut().selected = (1, col_of(&app, "Cost")); press(&mut app, 'x'); let wb = &app.model_state.workbook; - assert_eq!(wb.model.get_cell(&record("West", "Cost")), None, "West/Cost"); + assert_eq!( + wb.model.get_cell(&record("West", "Cost")), + None, + "West/Cost" + ); assert_eq!( wb.model.get_cell(&record("East", "Cost")), Some(&CellValue::Number(700.0)), @@ -418,7 +420,10 @@ mod tests { let before = crate::persistence::format_md(&app.model_state.workbook); app.cursor_mut().selected = (1, col_of(&app, "Region")); press(&mut app, 'x'); - assert_eq!(crate::persistence::format_md(&app.model_state.workbook), before); + assert_eq!( + crate::persistence::format_md(&app.model_state.workbook), + before + ); assert!( app.view_state.status_msg.ends_with(NO_VALUE_REFUSAL), "declined out loud, got {:?}", @@ -464,10 +469,15 @@ mod tests { assert_eq!(app.layout.col_label(2), "Profit", "setup: on the formula"); press(&mut app, 'p'); let (text, back) = saved_and_reloaded(&app.model_state.workbook); - assert!(!text.contains("_Measure=Profit"), "saved under the formula:\n{text}"); + assert!( + !text.contains("_Measure=Profit"), + "saved under the formula:\n{text}" + ); assert_eq!(back.model.get_cell(&record("West", "Profit")), None); assert!( - app.view_state.status_msg.contains("Profit is computed by a formula"), + app.view_state + .status_msg + .contains("Profit is computed by a formula"), "declined out loud, got {:?}", app.view_state.status_msg ); @@ -492,14 +502,21 @@ mod tests { assert!(app.view_state.mode.is_editing(), "setup: editing West/Cost"); app.handle_key(KeyEvent::new(KeyCode::Tab, KeyModifiers::NONE)) .unwrap(); - assert_eq!(app.cursor().selected, (1, 2), "setup: Tab moved onto Profit"); + assert_eq!( + app.cursor().selected, + (1, 2), + "setup: Tab moved onto Profit" + ); for c in "7".chars() { press(&mut app, c); } app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) .unwrap(); let (text, back) = saved_and_reloaded(&app.model_state.workbook); - assert!(!text.contains("_Measure=Profit"), "saved under the formula:\n{text}"); + assert!( + !text.contains("_Measure=Profit"), + "saved under the formula:\n{text}" + ); assert_eq!(back.model.get_cell(&record("West", "Profit")), None); } } @@ -592,7 +609,6 @@ fn aggregates(ctx: &CmdContext, key: &CellKey) -> bool { }) } - // ── View commands ──────────────────────────────────────────────────────────── #[derive(Debug)] diff --git a/crates/improvise-command/src/command/cmd/grid.rs b/crates/improvise-command/src/command/cmd/grid.rs index c191daca..24a2fb65 100644 --- a/crates/improvise-command/src/command/cmd/grid.rs +++ b/crates/improvise-command/src/command/cmd/grid.rs @@ -33,7 +33,10 @@ mod tests { let (text, back) = saved_and_reloaded(&app.model_state.workbook); let data = text.split("## Data").nth(1).expect("a data section"); - assert!(!data.contains("Q2"), "a group coordinate was saved:\n{text}"); + assert!( + !data.contains("Q2"), + "a group coordinate was saved:\n{text}" + ); assert_eq!(non_item_coordinates(&back), Vec::<(String, String)>::new()); } @@ -47,12 +50,19 @@ mod tests { app.cursor_mut().selected = (1, 1); press(&mut app, 'i'); app.handle_key(enter).unwrap(); - assert_eq!(app.cursor().selected, (2, 1), "setup: Enter moved onto the fold"); + assert_eq!( + app.cursor().selected, + (2, 1), + "setup: Enter moved onto the fold" + ); press(&mut app, '7'); app.handle_key(enter).unwrap(); let (text, back) = saved_and_reloaded(&app.model_state.workbook); let data = text.split("## Data").nth(1).expect("a data section"); - assert!(!data.contains("Q2"), "a group coordinate was saved:\n{text}"); + assert!( + !data.contains("Q2"), + "a group coordinate was saved:\n{text}" + ); assert_eq!(non_item_coordinates(&back), Vec::<(String, String)>::new()); } @@ -79,7 +89,10 @@ mod tests { app.view_state.status_msg ); let (text, _) = saved_and_reloaded(&app.model_state.workbook); - assert!(!text.contains("hidden:"), "a hidden entry was saved:\n{text}"); + assert!( + !text.contains("hidden:"), + "a hidden entry was saved:\n{text}" + ); // The control: the row above is an item, and H hides it. app.cursor_mut().selected = (1, 1); diff --git a/crates/improvise-command/src/command/cmd/mod.rs b/crates/improvise-command/src/command/cmd/mod.rs index f895d64d..9c46d72e 100644 --- a/crates/improvise-command/src/command/cmd/mod.rs +++ b/crates/improvise-command/src/command/cmd/mod.rs @@ -152,7 +152,10 @@ pub(super) mod test_helpers { let mut app = crate::ui::app::App::new(wb, None); app.rebuild_layout(); app.cursor_mut().selected = (2, 1); - assert!(app.layout.folds_a_group(2, 1), "setup: the cursor is on the fold"); + assert!( + app.layout.folds_a_group(2, 1), + "setup: the cursor is on the fold" + ); assert_eq!(app.layout.row_label(2), "Q2", "setup: the fold is Q2's row"); app } diff --git a/crates/improvise-command/src/command/cmd/mode.rs b/crates/improvise-command/src/command/cmd/mode.rs index f87a2312..2a90c51c 100644 --- a/crates/improvise-command/src/command/cmd/mode.rs +++ b/crates/improvise-command/src/command/cmd/mode.rs @@ -380,7 +380,9 @@ impl Cmd for EditOrDrill { // The same question `cell_id` asks, off the same predicate. In records mode // the synthetic key names no measure, so the record's key is resolved. if let Some(formula) = ctx.owning_formula() { - return vec![effect::refuse(super::core::derived_refusal(&formula.target))]; + return vec![effect::refuse(super::core::derived_refusal( + &formula.target, + ))]; } vec![ Box::new(effect::SetBuffer { diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index 189d733c..5aca92a9 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -483,7 +483,11 @@ mod tests { fn no_normal_mode_key_pressed_on_a_fold_stores_a_group_coordinate() { use improvise_protocol::key::KeyEvent; let ks = KeymapSet::default_keymaps(); - let normal = ks.maps().get(&ModeKey::Normal).expect("a Normal keymap").clone(); + let normal = ks + .maps() + .get(&ModeKey::Normal) + .expect("a Normal keymap") + .clone(); let mods = [ KeyModifiers::NONE, KeyModifiers::SHIFT, @@ -507,7 +511,9 @@ mod tests { } } assert!( - strokes.iter().any(|s| s == &[(KeyCode::Char('p'), KeyModifiers::NONE)]), + strokes + .iter() + .any(|s| s == &[(KeyCode::Char('p'), KeyModifiers::NONE)]), "the sweep reaches paste" ); for stroke in &strokes { diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 4796ae44..c7aa4c1f 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1871,8 +1871,14 @@ fn x_in_records_mode_clears_the_record_the_browser_clears() { both_ends(&mut app, &mut twin, &mut client, "x"); let model = &app.model_state.workbook.model; - assert_eq!(model.get_cell(&north_revenue().with("Region", "South")), None); - assert!(model.get_cell(&north_revenue()).is_some(), "North is untouched"); + assert_eq!( + model.get_cell(&north_revenue().with("Region", "South")), + None + ); + assert!( + model.get_cell(&north_revenue()).is_some(), + "North is untouched" + ); } /// `key` at `at` on a host and on a client seeded from it: neither writes. @@ -1881,11 +1887,21 @@ fn declines_on_both_ends(mut app: App, at: (usize, usize), key: char) { app.view_state.yanked = Some(improvise_protocol::CellValue::Number(99.0)); app.cursor_mut().selected = at; let client = client_ready_to_edit(&app, (6, 6)); - assert!(!composes_a_write(&client, event), "the browser composed a write"); + assert!( + !composes_a_write(&client, event), + "the browser composed a write" + ); let before = document_text(&app); reduce_key(&mut app, event); - assert_eq!(document_text(&app), before, "the TUI wrote where the browser declines"); - assert!(!app.view_state.status_msg.is_empty(), "the TUI declined silently"); + assert_eq!( + document_text(&app), + before, + "the TUI wrote where the browser declines" + ); + assert!( + !app.view_state.status_msg.is_empty(), + "the TUI declined silently" + ); } /// improvise-54g1: `p` on a formula-owned cell. The control is the stored cell @@ -1893,7 +1909,11 @@ fn declines_on_both_ends(mut app: App, at: (usize, usize), key: char) { #[test] fn p_onto_a_formula_owned_cell_declines_on_both_ends() { let app = yank_host(); - assert_eq!(app.layout.col_label(3), "Profit", "setup: column 3 is the formula"); + assert_eq!( + app.layout.col_label(3), + "Profit", + "setup: column 3 is the formula" + ); declines_on_both_ends(app, (1, 3), 'p'); let mut app = yank_host(); @@ -1901,7 +1921,10 @@ fn p_onto_a_formula_owned_cell_declines_on_both_ends() { app.cursor_mut().selected = (1, 2); let client = client_ready_to_edit(&app, (6, 6)); let p = KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE); - assert!(composes_a_write(&client, p), "the control: Cost is settable"); + assert!( + composes_a_write(&client, p), + "the control: Cost is settable" + ); } /// improvise-gw18/l4mp: `p` and `x` on a collapsed group's fold, off the origin. From b76d8d6ef8c9725f01dfdd34427913ef704fd5a9 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:06:43 -0700 Subject: [PATCH 05/29] improvise-gw18: document the cursor's three doors and the evidence Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 66 +++++++++++++++++++++++-- context/code/core.md | 6 ++- context/test-evidence/clients.md | 21 ++++++++ context/test-evidence/command.md | 83 ++++++++++++++++++++++++++++++++ 4 files changed, 172 insertions(+), 4 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index ca241086..36df576c 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3120,6 +3120,66 @@ so `paste Region=West _Measure=Revenue Product=Gadget` from a script still writes where the current view happens to aggregate `Product` away: a key that names every coordinate is a record whatever the view shows. For the cursor's key the two questions agree, because a cursor key never carries a None-axis -category. The **collapsed-group fold** half of improvise-l4mp is not covered: -its key carries the group's name as an ordinary coordinate, so no question about -the key alone can see it — it needs `GridLayout::folds_a_group`. +category. The **collapsed-group fold** half of improvise-l4mp cannot be seen by +any question about the key alone, because the key carries the group's name as an +ordinary coordinate. It is answered at the cursor instead, by the next section. +Since then the checks in `paste_into` are reached only by a key a script spells +out: the cursor path hands `PasteCell` a key the door has already resolved. + +## The cursor's cell has three doors (improvise-gw18, lx7w, 54g1, yk2s) + +**The lesson of improvise-gw18 is that guarding one command did not close the +path.** PR #176 taught `EditOrDrill` to refuse a collapsed group's fold, and `>` +still drilled into it. The drill was empty, and `o` inside it wrote +`Month=Q1, _Measure=Revenue`, a coordinate that is not an item, into `## Data`. +The same shape turned up three more times, each in a command nobody had +listed. `p` wrote onto the fold (the fold half of improvise-l4mp). `x` in records +mode cleared the synthetic `(_Index, _Dim)` key, a silent no-op, while the +browser cleared the real record (improvise-lx7w). `p` onto a formula's cell +stored a value under the formula that survived in `## Data`, while the browser +declined (improvise-54g1). Each fix was one more guard in one more command. So +the answer is the **accessor**, not the commands: a command gets the cursor's +cell through a door that matches what it does with it. + +Every command that takes the cursor's cell, and its door: + +| command | door | on a fold | on a formula's cell | in records mode | +|---|---|---|---|---| +| `yank` | `cell_key` (reads) | yanks the fold's value | yanks the result | `YankCell` resolves the synthetic key | +| `paste`, `clear-cell` | `value_cell_key` | refused | refused | resolved to the real record; a coordinate or creating column and a drill are refused | +| `commit-cell-edit`, `commit-and-advance-right` | `committable_cell_key` | refused | refused | the synthetic key, which `commit_cell_value` resolves itself (gotcha 19) | +| `drill-into-cell` | `unfolded_cell_key` | refused | drills | unchanged | +| `edit-or-drill` | its own check, `folds_a_group` then `owning_formula` | refused | refused | unchanged | +| `hide-selected-row-item` (`H`) | `GridLayout::row_folds_a_group` | refused | n/a (hides a row item) | n/a | +| `add-record-row` (`o`) | none, it takes no cell | n/a | n/a | closed by the drill door: no drill of a fold exists to add a row in | + +**`value_cell_key` asks the browser's own question.** It calls +`project::cell_id`, the function that fills `CellDisplay::id` for the browser +(ADR 002), and writes only where the answer is `CellId::Record`. The browser's +`record_cell_at_cursor` is that same match on the projected id. The two ends +therefore agree because they read one answer, not because two derivations +happen to match. `tests/lockstep.rs` holds them together for all three tickets. +`cell_id` took `&App` only to reach the model, so it now takes `&Model`. That is +what let `CmdContext` ask it. + +**Commit has a door of its own**, and not `value_cell_key`, because commit is +the one writer that must see the synthetic key: renaming a coordinate column, +creating a column and staging a drill edit all resolve the records position +inside `commit_cell_value`. Its door refuses the two arms `i` refuses, an +aggregate (a fold included) and a formula's result. It needed one because +`Enter` and `Tab` advance the editor, and `EnterEditAtCursor` re-opens it at the +new cursor with no guard. `i` declines to edit `Profit`, yet `Tab` from `Cost` +landed the editor on it, and the next commit stored a value under the formula. +That is a fourth door nobody had listed. **The entry half is still open** +(improvise-enz6): the editor still opens on a fold or a formula's cell. Only the +write is refused, out loud, at commit. Closing the entry belongs in `ui/effect.rs`. + +**Drill keeps the narrowest door** because drilling into an aggregate is its +purpose. Only a fold is refused, since no record carries a group's name. The +ticket's alternative, expanding the fold into its members' records, would be the +more useful drill and is not built. + +**A text-path key is not covered.** `paste Month=Q1 _Measure=Revenue` typed as +coordinates still reaches `PasteCell` with a group name. The doors are about the +**cursor**, and a key a user spells out is a claim about the document that no +layout checked. The same is true of `set-cell`. diff --git a/context/code/core.md b/context/code/core.md index e27a3c18..d88b5180 100644 --- a/context/code/core.md +++ b/context/code/core.md @@ -900,7 +900,11 @@ regular on None projected the fold as `CellId::Record`, offered it for editing, and a `SetCell` would have written the coordinate `Month=Q1`, which is not an item. `render.rs::editable` is untouched (ADR 002: the variant decides). `EditOrDrill` asks the same predicate and declines out loud: drilling would filter -on `Month=Q1`, which no record carries, and show an empty list. +on `Month=Q1`, which no record carries, and show an empty list. Guarding that one +command left `>`, `p`, `H` and an `Enter` advanced onto the fold all still writing +or drilling there (improvise-gw18, yk2s). Every command that takes the cursor's +cell now goes through a door that asks this predicate, or asks `cell_id`, which +does: `context/code/command.md`, "The cursor's cell has three doors". ### The cursor, and why the order is the fix diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 3ce396f8..e078b4ca 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4405,3 +4405,24 @@ assertion. The tail yanks off the `Region` and `+` columns and requires the two registers to agree; a TUI-only blank filter (`Text("")` to `None`) fails it on `+`, which is why that filter was removed rather than kept. The evidence entries for the host-side tests are in `command.md`. + +### x_in_records_mode_clears_the_record_the_browser_clears + +improvise-lx7w, both ends. The browser composes `ClearCell` of the projected +`CellId::Record`. Before the fix the host's `x` cleared the synthetic key and +the documents diverged in `both_ends`. South carries **two** measures. With one, +clearing it removes South's whole records row, the host resets its cursor on +the reframe, and `both_ends` reports a cursor divergence that belongs to the +fixture and not to `x`. The first version of the test did exactly that. The test +also asserts that South/Revenue is gone and North is untouched, because two ends +that both do nothing also agree. + +### p_onto_a_formula_owned_cell_declines_on_both_ends + +improvise-54g1, both ends, on `yank_host`'s `Profit` column (row 1, column 3). +The client must not compose a `Command::Model`, and the host's document must not +move and must say why. The control, Cost at column 2, must compose a write, so +a client that never writes cannot pass. The fold case, +`a_write_onto_a_fold_declines_on_both_ends`, runs the same helper for `p` and `x` +at row 2. Rival for all three: the host's paste/clear door restored to +`ctx.cell_key()`, which was `gh/main`'s code. All three fail. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 6d36571e..c3c73087 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5087,3 +5087,86 @@ 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. + +The drilled paste now declines at the cursor door (`value_cell_key`) before +`PasteCell` runs, so the status carries the registry's `paste:` prefix and a +wording shared with `clear-cell`. The assertion was re-aimed from equality with +`PASTE_INSIDE_DRILL` to `ends_with(INSIDE_A_DRILL_REFUSAL)` (improvise-gw18's +doors). The advice half is unchanged. + +### drilling_into_a_fold_is_refused_and_o_after_it_writes_no_group_coordinate + +improvise-gw18. On `gh/main` at `800348a`, `>` on the Q2 fold (row 2, column 1) +opened a `_Drill` with no rows, and `o` in it stored `Month=Q2` in `## Data`. The +test presses both keys through `App::handle_key`, then saves and reloads, and +asserts that no coordinate names a non-item (`non_item_coordinates`, which asks +the category, not the spelling). It failed on the first assertion: the drill +opened. Rival: the registry's cursor fallback on the reading accessor +(`cell_key`) in place of `unfolded_cell_key`. This test and +`no_normal_mode_key_pressed_on_a_fold_stores_a_group_coordinate` fail. The +control, `drilling_into_the_row_above_a_fold_still_opens_a_drill`, holds the +refusal to the fold alone. + +### no_normal_mode_key_pressed_on_a_fold_stores_a_group_coordinate + +improvise-gw18's enumeration, made executable. Every key bound in Normal mode, +prefix sub-keys included, is pressed on a fresh app with the cursor on a fold +and a value yanked, and no stored coordinate may name a non-item. The sweep is +**enumerated** rather than hand-picked, which is how it found `p` (the fold half +of improvise-l4mp) without being told. The first run failed on +`[(Char('p'), NONE)] on a fold stored [("Month", "Q2")]`. It asserts that the +enumeration reaches `p`, so an empty sweep cannot pass. What it cannot see is a +write two keys deep (`>` then `o`). That is why the drill test above exists and +why a two-key sweep was measured and rejected as too slow. + +### hiding_a_folds_row_is_refused_and_persists_no_hidden_group + +improvise-yk2s. `H` on the fold emitted `HideItem { Month, "Q2" }`. `View` +accepts an unknown name, so a ghost `hidden:` line was saved, with no visible +change and no message. The test asserts no hidden entry, a message, and no +`hidden:` line in the saved text. The control hides `Feb` one row up. Rival: the +`row_folds_a_group` guard disabled (`if false && …`). Only this test fails. The +guard asks the row half of `folds_a_group`, so an inner-level fold +(`North/Q1`) is refused too, although `North` is a real item. That is a +deliberate over-refusal, taken to keep one predicate. + +### enter_advancing_onto_a_fold_commits_nothing_there + +The write half of improvise-enz6, closed by the commit door. `i` on Feb, then +`Enter`, which commits and advances onto the Q2 fold and re-opens the editor +there. That entry is still enz6's open half. Then `7` and `Enter`. At the +previous commit door (`cell_key`) the second commit stored `Month=Q2`. Rival: +`commit_at_cursor` on `cell_key` again. This test and +`tab_advancing_onto_a_formula_cell_commits_nothing_under_it` fail. + +### x_in_records_mode_clears_the_real_record_under_the_cursor + +improvise-lx7w. In records mode `x` built `ClearCell` from the synthetic +`(_Index, _Dim)` key, which names nothing stored, so it was a silent no-op. The +cursor is on West × Cost (row 1, column 3). The test asserts that West/Cost is +gone, that East/Cost is untouched (so a clear of the wrong row cannot pass), and +that nothing synthetic reaches the reloaded file. Rival: `clear-cell`'s cursor +fallback on the old door. This test and the coordinate-column one fail. + +### x_on_a_records_coordinate_column_changes_nothing_and_says_so + +improvise-lx7w's decline arm, the answer #184 gave paste: a coordinate column +stores no value, so `x` there changes nothing and says so. Before the fix it +changed nothing *silently*, and it fails on the message. + +### p_onto_a_formula_owned_cell_writes_nothing_and_names_the_formula + +improvise-54g1. `p` on West × Profit (`Profit = Revenue - Cost`, row 1, +column 2) stored `_Measure=Profit` under the formula. The recompute hid it, but +it survived in `## Data`. The test asserts that the saved text has no Profit +line, that the reload has no such cell, and that the status names the formula. +The control, one column left, must write, so a door that refused everything +cannot pass. Rival: the old door. It fails on the saved text. + +### tab_advancing_onto_a_formula_cell_commits_nothing_under_it + +A door the tickets did not list, found by working the enumeration. `i` declines +to edit a formula's cell, but `Tab` from West/Cost commits and advances onto +Profit, `EnterEditAtCursor` re-opens the editor there, and `Enter` stored +`7` under the formula. Found by this test failing at `90b93d4`, before +`committable_cell_key` existed. The rival is the one above. From 9cb7741c4b911a115e00a0cb88321f775303a1e4 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:39:53 -0700 Subject: [PATCH 06/29] WIP: pin every refuse_and_stop arm's abort (improvise-514o) --- crates/improvise-command/src/ui/app.rs | 24 +++++++++++++---- crates/improvise-command/src/ui/host.rs | 35 ++++++++++++++++--------- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 565280a6..25347ed7 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -2996,16 +2996,30 @@ mod tests { fn a_failed_load_or_import_does_not_run_the_quit_after_it() { use improvise_protocol::keymap::Keymap; - for (command, path) in [("load", "absent.improv"), ("import-json", "absent.json")] { + // One case per `refuse_and_stop` site (improvise-514o). + let cases: [(&str, &[&str], &str); 7] = [ + ("load", &["absent.improv"], "Load error"), + ("import-json", &["absent.json"], "Cannot read 'absent.json'"), + ("import-json", &["absent.csv"], "Cannot read 'absent.csv'"), + ("import-json", &["bad.csv"], "CSV error"), + ("import-json", &["bad.json"], "JSON parse error"), + ("import-json", &["nested.json", "M", "nope"], "No array at path"), + ("import-json", &["measures.json"], "Import error"), + ]; + for (command, args, arm) in cases { + let path = args.join(" "); let mut app = App::new(Workbook::new("T"), None); - app.storage = Box::new(crate::storage::MemoryFs::new()); + app.storage = Box::new(crate::ui::host::import_fixture_storage()); let key = KeyCode::Char('x'); let mods = KeyModifiers::NONE; let mut km = Keymap::new(); km.bind_seq( key, mods, - vec![(command, vec![path.to_string()]), ("force-quit", vec![])], + vec![ + (command, args.iter().map(|a| a.to_string()).collect()), + ("force-quit", vec![]), + ], ); let effects = { let ctx = app.cmd_context(key, mods); @@ -3015,8 +3029,8 @@ mod tests { app.apply_effects(effects); assert!( - app.last_batch_refused(), - "setup: `{command}` must actually have failed, got {:?}", + app.last_batch_refused() && app.view_state.status_msg.contains(arm), + "setup: `{command} {path}` must fail in the {arm:?} arm, got {:?}", app.view_state.status_msg ); assert_ne!( diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index f31b1d08..78aac189 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -266,6 +266,27 @@ impl Host { } } +/// One file per way `ImportJsonHeadless` can fail, plus one that imports; the +/// `absent.*` paths are left unwritten. Shared by the frame and key-path tests. +#[cfg(test)] +pub(crate) fn import_fixture_storage() -> crate::storage::MemoryFs { + let storage = crate::storage::MemoryFs::new(); + let files: [(&str, &[u8]); 5] = [ + // Every field numeric, so no category is accepted and the build fails. + ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), + ("bad.json", b"{not json"), + ("nested.json", br#"{"data": [{"Region": "North"}]}"#), + // A record that is not UTF-8 is a CSV read error, not a warning. + ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), + ("good.json", br#"[{"Region": "North", "Revenue": 1}]"#), + ]; + for (name, bytes) in files { + crate::storage::Storage::write(&storage, std::path::Path::new(name), bytes) + .expect("fixture writes"); + } + storage +} + #[cfg(test)] mod tests { use std::path::{Path, PathBuf}; @@ -1946,19 +1967,7 @@ mod tests { /// A host whose storage holds one file per way `ImportJsonHeadless` can /// fail, plus nothing at the two paths that must fail to read. fn host_with_import_fixtures() -> Host { - let storage = MemoryFs::new(); - let files: [(&str, &[u8]); 4] = [ - // Every field numeric, so no category is accepted and the build fails. - ("measures.json", br#"[{"x": 1}, {"x": 2}]"#), - ("bad.json", b"{not json"), - ("nested.json", br#"{"data": [{"Region": "North"}]}"#), - // A record that is not UTF-8 is a CSV read error, not a warning. - ("bad.csv", b"Region,Revenue\n\xff\xfe,1\n"), - ]; - for (name, bytes) in files { - crate::storage::Storage::write(&storage, Path::new(name), bytes) - .expect("fixture writes"); - } + let storage = super::import_fixture_storage(); // A second view, so deleting an absent one is not refused as "the last". let mut wb = workbook(); wb.create_view("Scratch"); From 09a5cdfa03a14309069aaee484b99c9f98241ef2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:41:54 -0700 Subject: [PATCH 07/29] WIP: a formula that does not parse keeps its editor and text (improvise-v4hg) --- crates/improvise-command/src/ui/app.rs | 49 +++++++++++++++++++++++ crates/improvise-command/src/ui/effect.rs | 10 ++--- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 25347ed7..7ba006c6 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -3041,6 +3041,55 @@ mod tests { } } + /// improvise-v4hg: Enter on a formula that does not parse threw the typed text + /// away. Evidence: `context/test-evidence/command.md`. + #[test] + fn a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text() { + let type_and_commit = |text: &str| { + let mut app = two_col_model(); + app.view_state.mode = AppMode::FormulaPanel; + let press = |app: &mut App, code| { + app.handle_key(KeyEvent::new(code, KeyModifiers::NONE)) + .unwrap() + }; + press(&mut app, KeyCode::Char('a')); + assert!( + matches!(app.view_state.mode, AppMode::FormulaEdit { .. }), + "setup: `a` in the formula panel opens the editor" + ); + for c in text.chars() { + press(&mut app, KeyCode::Char(c)); + } + press(&mut app, KeyCode::Enter); + app + }; + + // The control: a formula that parses is stored and the editor closes. + let ok = type_and_commit("Total = 1"); + assert_eq!(ok.view_state.mode, AppMode::FormulaPanel); + assert!( + ok.model_state.workbook.model.formulas().iter().any(|f| f.target == "Total"), + "the control formula must land" + ); + + let bad = type_and_commit("Total = ("); + assert!( + bad.last_batch_refused() && bad.view_state.status_msg.contains("Formula error"), + "the parse error must be reported, got {:?}", + bad.view_state.status_msg + ); + assert!( + matches!(bad.view_state.mode, AppMode::FormulaEdit { .. }), + "a refused formula must leave the user in the editor, got {:?}", + bad.view_state.mode + ); + assert_eq!( + bad.view_state.buffers.get("formula").map(String::as_str), + Some("Total = ("), + "and must keep what they typed" + ); + } + /// A prefix is consumed whether or not the next key matches, so a stray key /// after `g` does nothing rather than acting as a fresh Normal binding. #[test] diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 9bc78b34..f7786350 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -44,8 +44,8 @@ pub enum ViewTarget { Named(String), } -/// Refuse AND stop the batch, for a failed document swap: what follows it in -/// a `:` line assumes it happened, so `load x . q` must not quit (cz8d). +/// Refuse AND stop the batch, where what follows assumes the change happened: +/// `load x . q` must not quit, a bad formula must not close its editor (cz8d). fn refuse_and_stop(app: &mut App, why: impl Into) { app.refuse(why); app.abort_effects = true; @@ -218,9 +218,9 @@ impl Effect for AddFormula { } app.model_state.workbook.model.add_formula(formula); } - Err(e) => { - app.refuse(format!("Formula error: {e}")); - } + // Stop too: what follows it on the key path closes the editor and + // clears the buffer, which would throw the user's text away (v4hg). + Err(e) => refuse_and_stop(app, format!("Formula error: {e}")), } } fn kind(&self) -> EffectKind { From 877526786528ede9ca4209430c88145eccaaeb64 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:44:27 -0700 Subject: [PATCH 08/29] WIP: pin the refusal sites nothing else pinned (improvise-0v8l) --- crates/improvise-command/src/ui/app.rs | 345 ++++++++++++++++++++++++- 1 file changed, 343 insertions(+), 2 deletions(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 7ba006c6..a19e7aa8 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -3003,7 +3003,11 @@ mod tests { ("import-json", &["absent.csv"], "Cannot read 'absent.csv'"), ("import-json", &["bad.csv"], "CSV error"), ("import-json", &["bad.json"], "JSON parse error"), - ("import-json", &["nested.json", "M", "nope"], "No array at path"), + ( + "import-json", + &["nested.json", "M", "nope"], + "No array at path", + ), ("import-json", &["measures.json"], "Import error"), ]; for (command, args, arm) in cases { @@ -3068,7 +3072,12 @@ mod tests { let ok = type_and_commit("Total = 1"); assert_eq!(ok.view_state.mode, AppMode::FormulaPanel); assert!( - ok.model_state.workbook.model.formulas().iter().any(|f| f.target == "Total"), + ok.model_state + .workbook + .model + .formulas() + .iter() + .any(|f| f.target == "Total"), "the control formula must land" ); @@ -3090,6 +3099,338 @@ mod tests { ); } + /// Run a `:` line the way `improvise cmd` does, one batch per segment; true + /// when any segment refused, which is what its exit code reads. + fn run_line(app: &mut App, line: &str) -> bool { + let mut refused = false; + for segment in crate::command::parse::segments(line) { + let effects = { + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + crate::command::parse::parse_segment_interactive(segment, &ctx) + .unwrap_or_else(|e| panic!("`{segment}` must parse: {e}")) + .map(|cmd| cmd.execute(&ctx)) + .unwrap_or_default() + }; + app.apply_effects(effects); + refused |= app.last_batch_refused(); + } + refused + } + + /// Commit `text` in a minibuffer mode with Enter; true when that batch refused. + fn commit_in(app: &mut App, mode: AppMode, buffer: &str, text: &str) -> bool { + app.view_state.mode = mode; + app.view_state + .buffers + .insert(buffer.to_string(), text.to_string()); + app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)) + .unwrap(); + app.last_batch_refused() + } + + fn with_import_fixtures(mut app: App) -> App { + app.storage = Box::new(crate::ui::host::import_fixture_storage()); + app + } + + /// Eleven regular categories: one short of the limit. + fn one_category_short_of_the_limit() -> App { + let mut wb = Workbook::new("T"); + for n in 0..11 { + wb.add_category(&format!("C{n}")).unwrap(); + } + App::new(wb, None) + } + + /// Stage one coordinate rename in a drill over the records fixture, then commit it. + fn commit_drill_rename(app: &mut App, to: &str) -> bool { + let records = app.layout.records.clone().expect("records mode"); + let mut pending_edits = HashMap::new(); + pending_edits.insert( + (1, crate::view::RecordCol::Category("Region".into())), + to.into(), + ); + app.drill_state = Some(DrillState { + records, + pending_edits, + }); + app.apply_effects(vec![Box::new(crate::ui::effect::ApplyAndClearDrill)]); + app.last_batch_refused() + } + + /// Records mode with `Region` paged to `page`: `add-record-row` then names + /// the row whose coordinates are exactly that page. + fn records_paged_to(app: &mut App, page: &str) { + app.model_state.workbook.add_item("Region", "West").unwrap(); + let view = app + .view_mut_of(&ViewTarget::Active) + .expect("the active view"); + view.set_axis("Region", crate::view::Axis::Page); + view.set_page_selection("Region", page); + app.rebuild_layout(); + } + + /// A grid with a regular category on None, so every cell is an aggregate. + fn aggregated_grid(with_items: bool) -> App { + let mut wb = Workbook::new("T"); + for cat in ["Row", "Col", "Hidden"] { + wb.add_category(cat).unwrap(); + if with_items { + wb.add_item(cat, "A").unwrap(); + } + } + wb.active_view_mut() + .set_axis("Hidden", crate::view::Axis::None); + let mut app = App::new(wb, None); + app.rebuild_layout(); + app + } + + /// `Region` down, `_Measure` across: `Revenue` at column 0, then `Total`, + /// which a formula owns, at column 1. + fn a_formula_column() -> App { + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "North").unwrap(); + wb.add_item("_Measure", "Revenue").unwrap(); + wb.active_view_mut() + .set_axis("_Measure", crate::view::Axis::Column); + let mut app = App::new(wb, None); + app.apply_effects(vec![Box::new(crate::ui::effect::AddFormula { + raw: "Total = Revenue * 2".into(), + target_category: "_Measure".into(), + })]); + app + } + + fn with_a_cell(mut app: App) -> App { + app.model_state.workbook.model.set_cell( + CellKey::new(vec![("Row".into(), "A".into()), ("Col".into(), "X".into())]), + CellValue::Number(42.0), + ); + app.rebuild_layout(); + app + } + + fn with_a_second_view(mut app: App) -> App { + app.model_state.workbook.create_view("Second"); + app + } + + fn switch_to_second(app: &mut App) { + app.apply_effects(vec![Box::new(crate::ui::effect::SwitchView( + "Second".into(), + ))]); + } + + /// improvise-0v8l: one case per refusal site no other test pinned, each with a + /// succeeding twin. Evidence: `context/test-evidence/command.md`. + #[test] + fn every_refusal_site_refuses_its_failure_and_only_its_failure() { + type Setup = fn() -> App; + type Drive = fn(&mut App) -> bool; + let cases: Vec<(&str, Setup, Drive, &str, Setup, Drive)> = vec![ + ( + "add-category past the limit", + one_category_short_of_the_limit, + |app| run_line(app, "add-category A . add-category B"), + "Maximum of 12 categories", + one_category_short_of_the_limit, + |app| run_line(app, "add-category A"), + ), + ( + "a drill commit that empties a coordinate", + records_app_for_drill, + |app| commit_drill_rename(app, ""), + crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY, + records_app_for_drill, + |app| commit_drill_rename(app, "West"), + ), + ( + "import of a JSON that does not parse", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import bad.json"), + "JSON parse error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import good.json"), + ), + ( + "import of an absent file", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import absent.json"), + "Cannot read file", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import good.json"), + ), + ( + "import-json of an absent CSV", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json absent.csv"), + "Cannot read 'absent.csv'", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json of a CSV that is not text", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json bad.csv"), + "CSV error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json of JSON that does not parse", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json bad.json"), + "JSON parse error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json at a path with no array", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json nested.json M nope"), + "No array at path", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "import-json that builds no model", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json measures.json"), + "Import error", + || with_import_fixtures(two_col_model()), + |app| run_line(app, "import-json good.json"), + ), + ( + "a records coordinate edited to nothing", + records_model_with_two_rows, + |app| { + app.cursor_mut().selected = (1, 0); + commit_in(app, AppMode::records_editing(), "edit", "") + }, + crate::ui::effect::RECORD_COORDS_CANNOT_BE_EMPTY, + records_model_with_two_rows, + |app| { + app.cursor_mut().selected = (1, 0); + commit_in(app, AppMode::records_editing(), "edit", "West") + }, + ), + ( + "an export with no path", + || with_import_fixtures(two_col_model()), + |app| commit_in(app, AppMode::export_prompt(), "export", ""), + "not exported", + || with_import_fixtures(two_col_model()), + |app| commit_in(app, AppMode::export_prompt(), "export", "out.csv"), + ), + ( + "view-forward with nowhere to go", + || with_a_second_view(two_col_model()), + |app| run_line(app, "view-forward"), + "No forward view", + || with_a_second_view(two_col_model()), + |app| { + switch_to_second(app); + run_line(app, "view-back"); + run_line(app, "view-forward") + }, + ), + ( + "view-back with nowhere to go", + || with_a_second_view(two_col_model()), + |app| run_line(app, "view-back"), + "No previous view", + || with_a_second_view(two_col_model()), + |app| { + switch_to_second(app); + run_line(app, "view-back") + }, + ), + ( + "add-record-row outside records mode", + two_col_model, + |app| run_line(app, "add-record-row"), + "only works in records mode", + records_model_with_two_rows, + |app| run_line(app, "add-record-row"), + ), + ( + "add-record-row onto a row that is already there", + records_model_with_two_rows, + |app| { + records_paged_to(app, "South"); + run_line(app, "add-record-row") + }, + "already here", + records_model_with_two_rows, + |app| { + records_paged_to(app, "West"); + run_line(app, "add-record-row") + }, + ), + ( + "edit-or-drill on an aggregate grid with no cell", + || aggregated_grid(false), + |app| run_line(app, "edit-or-drill editing"), + "No cell at cursor", + || aggregated_grid(true), + |app| run_line(app, "edit-or-drill editing"), + ), + ( + "edit-or-drill on a cell a formula owns", + a_formula_column, + |app| { + app.cursor_mut().selected = (0, 1); + run_line(app, "edit-or-drill editing") + }, + "computed by a formula", + a_formula_column, + |app| { + app.cursor_mut().selected = (0, 0); + run_line(app, "edit-or-drill editing") + }, + ), + ( + "a search with no matches", + || with_a_cell(two_col_model()), + |app| { + app.view_state.search_query = "zzz".into(); + run_line(app, "search-navigate forward") + }, + "No matches", + || with_a_cell(two_col_model()), + |app| { + app.view_state.search_query = "42".into(); + run_line(app, "search-navigate forward") + }, + ), + ( + "a : line that does not parse", + two_col_model, + |app| commit_in(app, AppMode::command_mode(), "command", "no-such-command"), + ":no-such-command", + two_col_model, + |app| commit_in(app, AppMode::command_mode(), "command", "add-category Z"), + ), + ]; + for (what, fail_setup, fails, says, ok_setup, succeeds) in cases { + let mut app = fail_setup(); + let refused = fails(&mut app); + assert!( + refused && app.view_state.status_msg.contains(says), + "{what}: expected a refusal saying {says:?}, got refused={refused} {:?}", + app.view_state.status_msg + ); + let mut app = ok_setup(); + assert!( + !succeeds(&mut app), + "{what}: its succeeding twin was refused: {:?}", + app.view_state.status_msg + ); + } + } + /// A prefix is consumed whether or not the next key matches, so a stray key /// after `g` does nothing rather than acting as a fresh Normal binding. #[test] From 288db9b269f579c27fc573a6a2526f65efa43596 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 22:50:50 -0700 Subject: [PATCH 09/29] WIP: width guard counts the records the compared drills froze (improvise-b9f2) --- .../src/command/cmd/determinism.rs | 62 +++++++++---------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 8f0a71e5..66a0a223 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -1,14 +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, HashSet}; +use std::collections::BTreeSet; use super::core::{Cmd, CmdContext}; +use super::grid::DrillIntoCell; use super::registry::tests::candidate_args; use crate::command::key::{KeyCode, KeyModifiers}; 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}; @@ -61,27 +61,14 @@ fn one_coordinate_drills(model: &Model) -> Vec> { 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) +/// Records a compared drill's own effects freeze in a fresh `App`, counted only when +/// the drill narrowed the store: a whole-store drill never reads the index. +fn records_narrowed(effects: Vec>) -> usize { + let mut app = fixture(); + let store = app.model_state.workbook.model.data.live_records().count(); + app.apply_effects(effects); + let frozen = app.drill_state.map_or(0, |drill| drill.records.len()); + if frozen < store { frozen } else { 0 } } /// The registry's candidates, plus names the demo really has: a drill or a fit @@ -108,9 +95,9 @@ fn args_the_document_names(model: &Model) -> Vec> { all } -fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option { +fn effects_of(name: &str, args: &[String], ctx: &CmdContext) -> Option>> { let cmd = ctx.registry.interactive(name, args, ctx).ok()?; - Some(format!("{:?}", cmd.execute(ctx))) + Some(cmd.execute(ctx)) } /// What running every registered command under `a` and under each of `others` found. @@ -120,20 +107,30 @@ struct Comparison { shown: Vec, never_built: Vec<&'static str>, compared: usize, + widest_drill: usize, } fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { let mut found = Comparison::default(); let candidates = args_the_document_names(a.model); + let drill = DrillIntoCell { + key: CellKey::new(vec![]), + }; for name in a.registry.names().collect::>() { let before = found.compared; for args in &candidates { - let Some(left) = effects_of(name, args, a) else { + let Some(effects) = effects_of(name, args, a) else { continue; }; + let left = format!("{effects:?}"); + if name == drill.name() { + found.widest_drill = found.widest_drill.max(records_narrowed(effects)); + } found.compared += 1; for b in others { - let right = effects_of(name, args, b).unwrap_or_default(); + let right = effects_of(name, args, b) + .map(|e| format!("{e:?}")) + .unwrap_or_default(); if left != right { found.diverged.insert(name); found @@ -157,13 +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.widest_drill >= 4, + "the widest narrowing drill compared froze {} records, so the store's order is nearly a coin toss", + found.widest_drill + ); assert!( found.never_built.is_empty(), "no candidate args build these in the fixture, so the law never ran them: {:?}", From 13b998d7f6f3c00a7a5199fb4e1c6dadcfd1a544 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:00:07 -0700 Subject: [PATCH 10/29] improvise-v8ab: a fresh session's slice is the host's startup slice Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/command.md | 22 ++++++++++++++++++++++ crates/improvise-command/src/ui/project.rs | 12 ++++++++++++ 2 files changed, 34 insertions(+) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 39fb1410..9ee01d3f 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5446,3 +5446,25 @@ The review's rival was `RecordCol::Measure(m) => Some(Derived { target: m })` in Without it, the test fails and names all four measure cells as `Derived`. A fixture that does carry a formula is `a_records_column_a_formula_owns_is_derived_not_a_record`'s job, not this one's. + +### a_fresh_sessions_slice_is_the_hosts_startup_slice + +improvise-v8ab. A fresh session's slice is built twice: `App::with_storage` +builds the host's own, and `project.rs::fresh_view` builds a client's. Nothing +asserted they agree. `tests/lockstep.rs` seeds every client from +`app.view_state.clone()`, so it structurally could not see improvise-lxg0's +phantom frame, and cannot see the next drift between the two either. The test +connects to two documents, both opening on `Second`, one with a `Default` view +and one without. It asserts the whole `ViewState` equals the host's, not only +the back stack, so any field the two constructors disagree on fails it. The +rival is the old fold, restored in `fresh_view`. It failed on the +`["Default", "Second"]` case: the only difference was `view_back_stack: +[ViewFrame { view_name: "Default", mode: Normal }]`. + +The scope is **connect only**. A rebase deliberately resets the client's whole +slice, while `install_workbook` keeps the host's mode and panels, so equality +is not the contract there. `lockstep.rs` still copies `app.view_state`, and +that is deliberate. Its helpers seed a client *after* mutating the host (mode, +buffers, cursor), so a `Snapshot` would drop exactly the state those tests +stage. This test is what makes the copy faithful at startup. + diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index d42e46c7..73bd7c14 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -1602,6 +1602,18 @@ mod tests { } } + /// improvise-v8ab: a fresh client starts from the host's own startup slice. + /// See `context/test-evidence/command.md`. + #[test] + fn a_fresh_sessions_slice_is_the_hosts_startup_slice() { + for names in [&["Default", "Second"][..], &["Main", "Second"][..]] { + let app = App::new(opening_on(names, "Second"), None); + assert_eq!(app.view_state.active_view, "Second", "setup: {names:?}"); + let (_, snapshot) = Sessions::default().connect(&app, 10, 10); + assert_eq!(snapshot.view, app.view_state, "{names:?}"); + } + } + /// improvise-lxg0: a document swap reseeds through the same door. #[test] fn a_rebased_session_has_no_history_to_step_back_through() { From 5d56196f56eafe5a730d9dcd71594a60b9191a41 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:00:59 -0700 Subject: [PATCH 11/29] WIP: one scratch App for the width guard (improvise-b9f2) --- .../src/command/cmd/determinism.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 66a0a223..fd978aff 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -61,13 +61,12 @@ fn one_coordinate_drills(model: &Model) -> Vec> { drills } -/// Records a compared drill's own effects freeze in a fresh `App`, counted only when -/// the drill narrowed the store: a whole-store drill never reads the index. -fn records_narrowed(effects: Vec>) -> usize { - let mut app = fixture(); - let store = app.model_state.workbook.model.data.live_records().count(); - app.apply_effects(effects); - let frozen = app.drill_state.map_or(0, |drill| drill.records.len()); +/// Records a compared drill's own effects freeze in `scratch`, counted only when the +/// drill narrowed the store: a whole-store drill never reads the index. +fn records_narrowed(scratch: &mut App, effects: Vec>) -> usize { + let store = scratch.model_state.workbook.model.data.live_records().count(); + scratch.apply_effects(effects); + let frozen = scratch.drill_state.as_ref().map_or(0, |drill| drill.records.len()); if frozen < store { frozen } else { 0 } } @@ -116,6 +115,7 @@ fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { let drill = DrillIntoCell { key: CellKey::new(vec![]), }; + let mut scratch = fixture(); for name in a.registry.names().collect::>() { let before = found.compared; for args in &candidates { @@ -124,7 +124,7 @@ fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { }; let left = format!("{effects:?}"); if name == drill.name() { - found.widest_drill = found.widest_drill.max(records_narrowed(effects)); + found.widest_drill = found.widest_drill.max(records_narrowed(&mut scratch, effects)); } found.compared += 1; for b in others { From 13bccf772e29a1daf568adfaac0b84b933c0f04b Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:01:26 -0700 Subject: [PATCH 12/29] Docs: the width guard measures the drills it compared (improvise-b9f2) --- context/code/command.md | 37 +++++++++++++++++++++----------- context/test-evidence/command.md | 28 +++++++++++++++++------- 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index d77da9a6..d1e04866 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3304,18 +3304,31 @@ 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 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. +the demo that is four records and eight cells for each of the eight drills. With +the store's sort removed, the law fails 400 of 400 runs. + +**The width guard measures what the comparison compared (improvise-b9f2).** The +test asserts that some `drill-into-cell` the comparison actually built and +compared froze at least four records. For each one it applies those very effects +to one scratch `App` and counts the records in its `drill_state`. A first +version measured what the *argument lists* match in the store instead. That was +a parallel derivation, and the review beat it with a `drill-into-cell` parser +that refuses fewer than two arguments. Every one-coordinate drill then went to +the one-record cursor fallback, the argument lists still matched four records, +and with the store's sort removed the law passed 38 of 300 runs. + +Counting frozen records is not enough by itself. A drill over no coordinates +(any candidate with no `Cat/Item` argument, such as `["1", "1"]`) freezes +**every** record, and it reads them through `iter_cells` in insertion order, +never through the narrowing index where gotcha 18 lives. Under the parser rival +that whole-store drill still froze 16 records, and a guard counting it stayed +silent. So only a drill that **narrowed** the store counts: one that froze fewer +records than the store holds. With the rival in place, the guard now fires on +every run ("froze 1 records"). + +What the guard still cannot see is a narrowing drill whose records are frozen in +an order that does not come from the store. It checks that the law had a wide +drill to compare, not why the drill's order would differ. **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 cd28229f..2d590635 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5375,14 +5375,26 @@ 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". - 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. + fails with "the widest narrowing drill compared froze 1 records". +- **`drill-into-cell`'s parser changed to refuse fewer than two arguments** + (improvise-b9f2): every one-coordinate drill then goes to the one-record cursor + fallback. The first width guard measured what the argument lists match, so it + stayed silent. With gotcha 18 re-injected the law passed 38 of 300 runs in + review, and 31 of 300 here at `cfcc9bff`. The guard now applies the compared + drills' own effects to a scratch `App` and counts the records they froze. + Against this rival it fails on every run with "froze 1 records". + - A first version of the fix counted all frozen records and **still passed**. + A candidate with no `Cat/Item` argument drills the whole store (16 records), + and it reads them in insertion order through `iter_cells`, which the + injection cannot reach. So the guard counts only drills that froze fewer + records than the store holds. + - Measured on the final form, test binary in a loop: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + parser rival | **0 of 300** | all 300 | + | gotcha 18 alone | **0 of 300** | 0 (every failure a real divergence) | + | neither | 300 of 300 | 0 | - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". From 7dfa90f138d764866707c7ed3cbb755e605a4f80 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:04:13 -0700 Subject: [PATCH 13/29] rustfmt the width guard (improvise-b9f2) --- .../src/command/cmd/determinism.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index fd978aff..4bdc7222 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -64,9 +64,18 @@ fn one_coordinate_drills(model: &Model) -> Vec> { /// Records a compared drill's own effects freeze in `scratch`, counted only when the /// drill narrowed the store: a whole-store drill never reads the index. fn records_narrowed(scratch: &mut App, effects: Vec>) -> usize { - let store = scratch.model_state.workbook.model.data.live_records().count(); + let store = scratch + .model_state + .workbook + .model + .data + .live_records() + .count(); scratch.apply_effects(effects); - let frozen = scratch.drill_state.as_ref().map_or(0, |drill| drill.records.len()); + let frozen = scratch + .drill_state + .as_ref() + .map_or(0, |drill| drill.records.len()); if frozen < store { frozen } else { 0 } } @@ -124,7 +133,9 @@ fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { }; let left = format!("{effects:?}"); if name == drill.name() { - found.widest_drill = found.widest_drill.max(records_narrowed(&mut scratch, effects)); + found.widest_drill = found + .widest_drill + .max(records_narrowed(&mut scratch, effects)); } found.compared += 1; for b in others { From 345bb8cdb616b551d6cd2eda6c93af69655f367d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:06:11 -0700 Subject: [PATCH 14/29] improvise-q9eg: a key the cursor's door resolved is not re-vetted by its shape Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/cell.rs | 60 ++++++++++++++++--- .../src/command/cmd/registry.rs | 6 +- .../improvise-wasm-client/tests/lockstep.rs | 50 ++++++++++++++++ 3 files changed, 105 insertions(+), 11 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/cell.rs b/crates/improvise-command/src/command/cmd/cell.rs index 5675ab5f..d5f41a4b 100644 --- a/crates/improvise-command/src/command/cmd/cell.rs +++ b/crates/improvise-command/src/command/cmd/cell.rs @@ -66,7 +66,9 @@ mod tests { ("Type".into(), "Clothing".into()), ("Month".into(), "Feb".into()), ]); - let cmd = PasteCell { key: key.clone() }; + let cmd = PasteCell { + at: PasteAt::Spelled(key.clone()), + }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 1); let mut app = crate::ui::app::App::new(two_cat_model(), None); @@ -88,7 +90,9 @@ mod tests { ("Type".into(), "Food".into()), ("Month".into(), "Jan".into()), ]); - let cmd = PasteCell { key }; + let cmd = PasteCell { + at: PasteAt::Spelled(key), + }; let effects = cmd.execute(&ctx); assert!(effects.is_empty()); } @@ -353,7 +357,9 @@ mod tests { ctx.yanked = &yanked; assert!(!ctx.none_cats().is_empty(), "setup: the view aggregates"); let key = record("West", "Revenue").with("Product", "Gadget"); - let effects = PasteCell { key: key.clone() }.execute(&ctx); + let effects = PasteCell { + at: PasteAt::Spelled(key.clone()), + }.execute(&ctx); let mut app = aggregated_app(crate::view::Axis::None); app.apply_effects(effects); assert_eq!( @@ -390,6 +396,33 @@ mod tests { assert!(dbg.contains("Save"), "Expected Save, got: {dbg}"); } + /// improvise-q9eg: a record that lacks a later category is still a record — + /// see context/test-evidence/command.md. + #[test] + fn p_onto_a_sparse_record_writes_it() { + let mut app = records_app(); + app.model_state.workbook.add_category("Season").unwrap(); + app.rebuild_layout(); + assert!( + app.layout.none_cats.iter().any(|c| c == "Season"), + "setup: the new category is aggregated away, so every record lacks it" + ); + app.cursor_mut().selected = (0, col_of(&app, "Cost")); + press(&mut app, 'y'); + press(&mut app, 'y'); + app.cursor_mut().selected = (1, col_of(&app, "Cost")); + press(&mut app, 'p'); + assert_eq!( + app.model_state + .workbook + .model + .get_cell(&record("West", "Cost")), + Some(&CellValue::Number(700.0)), + "status: {:?}", + app.view_state.status_msg + ); + } + /// improvise-lx7w — see context/test-evidence/command.md. #[test] fn x_in_records_mode_clears_the_real_record_under_the_cursor() { @@ -562,20 +595,31 @@ impl Cmd for YankCell { } } +/// Where a paste lands: a key the cursor's door already resolved, or one a +/// script spelled out, which only the key's own shape can vet (improvise-q9eg). +#[derive(Debug)] +pub enum PasteAt { + Resolved(CellKey), + Spelled(CellKey), +} + /// Paste the yanked value into a cell. #[derive(Debug)] pub struct PasteCell { - pub key: crate::model::cell::CellKey, + pub at: PasteAt, } impl Cmd for PasteCell { fn name(&self) -> &'static str { "paste" } fn execute(&self, ctx: &CmdContext) -> Vec> { - match ctx.yanked.clone() { - Some(value) => vec![paste_into(ctx, &self.key, value)], - None => vec![], - } + let Some(value) = ctx.yanked.clone() else { + return vec![]; + }; + vec![match &self.at { + PasteAt::Resolved(key) => Box::new(effect::SetCell(key.clone(), value)), + PasteAt::Spelled(key) => paste_into(ctx, key, value), + }] } } diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 76771f93..f4b89cc9 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -254,19 +254,19 @@ pub fn default_registry() -> CmdRegistry { ); r.register( &PasteCell { - key: CellKey::new(vec![]), + at: PasteAt::Spelled(CellKey::new(vec![])), }, |args| { if args.is_empty() { return Err("paste requires at least one Cat/Item coordinate".into()); } Ok(PasteCell { - key: parse_cell_key_from_args(args), + at: PasteAt::Spelled(parse_cell_key_from_args(args)), }) }, Some(|_, ctx| { Ok(PasteCell { - key: cursor_value_key(ctx)?, + at: PasteAt::Resolved(cursor_value_key(ctx)?), }) }), ); diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 50ae9c96..7dd7f1a0 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -1881,6 +1881,56 @@ fn x_in_records_mode_clears_the_record_the_browser_clears() { ); } +/// improvise-q9eg: `yy` then `p` onto a record that lacks a later category, +/// on both ends. Evidence: `context/test-evidence/clients.md`. +#[test] +fn p_onto_a_sparse_record_writes_what_the_browser_writes() { + let r = KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE); + let sparse = || { + let mut app = furnished_host(); + let south = north_revenue().with("Region", "South"); + app.model_state + .workbook + .model + .set_cell(south, improvise_protocol::CellValue::Number(900.0)); + app.rebuild_layout(); + reduce_key(&mut app, r); + // As `+cat` does: every existing record now lacks `Season`. + app.model_state.workbook.add_category("Season").unwrap(); + app.rebuild_layout(); + app + }; + let (mut app, mut twin) = (sparse(), sparse()); + assert!( + app.layout.none_cats.iter().any(|c| c == "Season"), + "setup: the new category is aggregated away" + ); + let revenue = (0..app.layout.col_count()) + .find(|&c| app.layout.col_label(c) == "Revenue") + .expect("a Revenue column"); + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (0, revenue); + } + let mut client = records_client(&app); + client.view.cursor_mut().selected = (0, revenue); + both_ends(&mut app, &mut twin, &mut client, "y"); + both_ends(&mut app, &mut twin, &mut client, "y"); + + for host in [&mut app, &mut twin] { + host.cursor_mut().selected = (1, revenue); + } + client.view.cursor_mut().selected = (1, revenue); + both_ends(&mut app, &mut twin, &mut client, "p"); + assert_eq!( + app.model_state + .workbook + .model + .get_cell(&north_revenue().with("Region", "South")), + Some(&improvise_protocol::CellValue::Number(1200.0)), + "the paste must land, or both ends agreeing is both declining" + ); +} + /// `key` at `at` on a host and on a client seeded from it: neither writes. fn declines_on_both_ends(mut app: App, at: (usize, usize), key: char) { let event = KeyEvent::new(KeyCode::Char(key), KeyModifiers::NONE); From 42bb8a26a7d0e4725c75d6d58242f061062ff101 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:07:06 -0700 Subject: [PATCH 15/29] The fold sweep also holds the data section and the hidden set still Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- crates/improvise-command/src/command/keymap.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index 5aca92a9..5bdf9f8c 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -516,6 +516,14 @@ mod tests { .any(|s| s == &[(KeyCode::Char('p'), KeyModifiers::NONE)]), "the sweep reaches paste" ); + // No key on a fold may touch the stored data or the hidden set at all. + let touched = |app: &crate::ui::app::App| { + let wb = &app.model_state.workbook; + let text = crate::persistence::format_md(wb); + let data = text.split("## Data").nth(1).unwrap_or_default().to_string(); + (data, wb.active_view().hidden_items.clone()) + }; + let before = touched(&test_helpers::app_on_a_fold()); for stroke in &strokes { let mut app = test_helpers::app_on_a_fold(); app.view_state.yanked = Some(crate::model::cell::CellValue::Number(5.0)); @@ -524,6 +532,7 @@ mod tests { } let stray = test_helpers::non_item_coordinates(&app.model_state.workbook); assert!(stray.is_empty(), "{stroke:?} on a fold stored {stray:?}"); + assert_eq!(touched(&app), before, "{stroke:?} on a fold changed the document"); } } From 775f02b4e4387dcf8808d2d155d7d15aaa151708 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:08:27 -0700 Subject: [PATCH 16/29] improvise-q9eg: correct the false sentence in command.md; evidence Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 12 +++++++-- context/test-evidence/clients.md | 11 ++++++++ context/test-evidence/command.md | 26 +++++++++++++++++-- .../improvise-command/src/command/cmd/cell.rs | 3 ++- .../improvise-command/src/command/keymap.rs | 6 ++++- 5 files changed, 52 insertions(+), 6 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index bdb1e0cc..a2e8b709 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3197,8 +3197,16 @@ key the two questions agree, because a cursor key never carries a None-axis category. The **collapsed-group fold** half of improvise-l4mp cannot be seen by any question about the key alone, because the key carries the group's name as an ordinary coordinate. It is answered at the cursor instead, by the next section. -Since then the checks in `paste_into` are reached only by a key a script spells -out: the cursor path hands `PasteCell` a key the door has already resolved. +**The first version of that section wrote a false sentence here**: that the +checks in `paste_into` were reached "only by a key a script spells out". The +cursor path handed `PasteCell` the resolved record key, and `paste_into` ran +`aggregates` over it **again**. That is a second derivation beside `cell_id`, and +it reads a record lacking a later category (every record, once `+cat` adds one) +as an aggregate, so `p` refused where gh/main had pasted and the browser still +wrote (improvise-q9eg). The distinction is now a type: `PasteAt::Resolved` +carries a key the door answered and is written as it stands, and +`PasteAt::Spelled` carries a key a script typed, which only its shape can vet. +Nothing is asked twice. ## The cursor's cell has three doors (improvise-gw18, lx7w, 54g1, yk2s) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 2c847d86..3bff3a6a 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4561,3 +4561,14 @@ normal edges only, because a dev edge would unify the feature back in 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. + +### p_onto_a_sparse_record_writes_what_the_browser_writes + +improvise-q9eg, both ends, on a **sparse** record: `Season` is added after +entering records mode, so no record carries it. `yy` on North/Revenue, then `p` +on South/Revenue (row 1), through `both_ends`, which compares the host's +document with the one the client's composed commands build. The host refused +while the browser composed `SetCell`, so the documents differed. The final +assertion that South holds 1200 is the half that keeps the test from passing +when both ends decline. Rival: `PasteAt::Resolved` back through `paste_into`. +This test fails. diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index c5a32696..4660c50c 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5244,8 +5244,16 @@ and a value yanked, and no stored coordinate may name a non-item. The sweep is of improvise-l4mp) without being told. The first run failed on `[(Char('p'), NONE)] on a fold stored [("Month", "Q2")]`. It asserts that the enumeration reaches `p`, so an empty sweep cannot pass. What it cannot see is a -write two keys deep (`>` then `o`). That is why the drill test above exists and -why a two-key sweep was measured and rejected as too slow. +write two keys deep (`>` then `o`). That is why the drill test above exists. A +two-key sweep was **estimated**, not measured, at several seconds and not built. + +Widened after PR #197's review, which showed that `H` without its fold check +passed the sweep: a ghost `hidden:` entry is not a stored coordinate. Every +stroke must now also leave the `## Data` section and the view's hidden set +exactly as they were. With the `H` guard disabled, this test fails as well as +the yk2s one. What no sweep can see is `clear-cell` back on the reading +accessor, because clearing a key nothing is stored at changes nothing. The +lx7w tests and the fold lockstep case catch that. ### hiding_a_folds_row_is_refused_and_persists_no_hidden_group @@ -5345,3 +5353,17 @@ 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". + +### p_onto_a_sparse_record_writes_it + +improvise-q9eg, a regression introduced by PR #197 and found in its review. In +records mode a category is added (as `+cat` does), so every existing record +lacks it and it sits on `Axis::None`. `yy` on East/Cost, then `p` on West/Cost +(row 1): gh/main pasted 700, and #197 refused with the aggregate message. +`value_cell_key` handed `PasteCell` the real record key, and `paste_into` then +re-ran `aggregates(ctx, key)`. That key-shape check is a second derivation +beside `cell_id`, and it reads a record missing a None-axis category as an +aggregate. Every records fixture before this one was fully populated, which is +why nothing saw it. The fix is `PasteAt::{Resolved, Spelled}`: a resolved key is +written as it stands. Rival: routing `Resolved` back through `paste_into`. This +test and `p_onto_a_sparse_record_writes_what_the_browser_writes` fail. diff --git a/crates/improvise-command/src/command/cmd/cell.rs b/crates/improvise-command/src/command/cmd/cell.rs index d5f41a4b..4642aa97 100644 --- a/crates/improvise-command/src/command/cmd/cell.rs +++ b/crates/improvise-command/src/command/cmd/cell.rs @@ -359,7 +359,8 @@ mod tests { let key = record("West", "Revenue").with("Product", "Gadget"); let effects = PasteCell { at: PasteAt::Spelled(key.clone()), - }.execute(&ctx); + } + .execute(&ctx); let mut app = aggregated_app(crate::view::Axis::None); app.apply_effects(effects); assert_eq!( diff --git a/crates/improvise-command/src/command/keymap.rs b/crates/improvise-command/src/command/keymap.rs index 5bdf9f8c..50768f91 100644 --- a/crates/improvise-command/src/command/keymap.rs +++ b/crates/improvise-command/src/command/keymap.rs @@ -532,7 +532,11 @@ mod tests { } let stray = test_helpers::non_item_coordinates(&app.model_state.workbook); assert!(stray.is_empty(), "{stroke:?} on a fold stored {stray:?}"); - assert_eq!(touched(&app), before, "{stroke:?} on a fold changed the document"); + assert_eq!( + touched(&app), + before, + "{stroke:?} on a fold changed the document" + ); } } From 72f0c63fff3560e5f419b537eed756a3786d3105 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:33:53 -0700 Subject: [PATCH 17/29] docs: one home for the review rules; pin gh/main; audit prose fixes improvise-28ri: -d skip only silences "Is a directory" on the skills' symlinks, which git ls-files lists as files. grep -r never follows them, so nothing was ever read twice. The "spells no id" sentence now says what the recipe actually cites. improvise-oga4: the review-reply and head-SHA rules live only in agent-rules "GitHub" (plus the after-merge clause moved there); AGENTS.md points to it, and the code-review skill names it. An ADR's "Approved" is an extra gate on top of CI, every review answered and the adversarial review. improvise-diho: agent-rules rule 7, pin gh/main to a SHA at task start. Also: build a test-name list in a fresh CARGO_TARGET_DIR, because git archive's commit mtimes make a reused one return a stale list. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/code-review/SKILL.md | 1 + .agents/skills/wave/agent-rules.md | 22 +++++++++++++--- AGENTS.md | 41 +++++++++++++++++++---------- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 618d30c2..6512d580 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -28,6 +28,7 @@ them here would let the copy drift: | `context/repo-map.md` | The "where", plus **numbered gotchas** — read the ones your diff touches | | `prolog-sessions/kb.pl` | Executable knowledge: `invariant/2`, `doctrine/2`, `forbidden/2`, `decision/3`, and prior `work_done/3` | | `AGENTS.md` | Working rules: bug-first tests, `Option`/`Result`/`Box` at the edges, merges over rebases | +| `.agents/skills/wave/agent-rules.md`, "GitHub" | The reviewer's rule: what to read before your first comment, how to answer each existing review, and what every review you post must state. Read it before reviewing a PR | `kb.pl` is the highest-value and least-obvious of these. Grep it before reviewing: `grep -n "invariant(\|doctrine(\|decision(" prolog-sessions/kb.pl`. diff --git a/.agents/skills/wave/agent-rules.md b/.agents/skills/wave/agent-rules.md index 7a4d3f52..7358f704 100644 --- a/.agents/skills/wave/agent-rules.md +++ b/.agents/skills/wave/agent-rules.md @@ -32,9 +32,17 @@ not in a second copy. 6. **The Beads "Session Completion" block near the end of `AGENTS.md` does not set your git policy.** This repository does not opt in to the block's "team-maintainer" profile. On a wave branch that profile's pull and bare - `git push` would be refused (rule 4), or worse. Rules 1–5 are your git - policy. The block is bd's and bd regenerates it whole, so it is not edited - by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). + `git push` would be refused (rule 4), or worse. Rules 1–5 and 7 are your + git policy. The block is bd's and bd regenerates it whole, so it is not + edited by hand: `AGENTS.md`, "Repository policy over the Beads blocks" (improvise-x1uk). +7. **Pin `gh/main` to a literal SHA when the task starts.** Run + `git rev-parse gh/main` once. Use that SHA, not the ref name, for every + reset, merge-base and diff. Remote refs are shared by every worktree of the + repository, so `gh/main` moves whenever another session fetches. An agent + ran `git reset --soft gh/main` mid-task and staged a reverse diff of #196. + It noticed only because it read `git status` (improvise-diho). Re-resolve + only on purpose, for example when you mean to merge a newer main. Rule 3's + check before a PR is such a time. ## Commit early: a worktree is not durable storage @@ -113,6 +121,12 @@ out on purpose: check that no test *disappeared*. Diff test **names**, not counts: a textually clean merge can delete a test module with no conflict marker, and the other side's new tests mask the loss in a count. +- **Build each side's test list in a fresh `CARGO_TARGET_DIR`, every time.** If + a parent's tree comes from `git archive`, a reused target dir gives a wrong + answer, not just a slow one. Archive stamps every file with its commit time, + so cargo judges the sources older than the last build and lists the previous + tree's tests. On #188 that made three of the branch's own tests look + merge-only. - **The shell guard refusing a compound command means NEITHER part ran.** Split the command. After a refusal, re-check the state it was meant to produce. - **Re-run the full suite after reverting an injected fault.** A revert applied @@ -200,6 +214,8 @@ text for any of these in your hand-back. `./scripts/pr-reviews `, unpiped. Reply on its thread to every Codex review: fixed (name the commit), declined (say why), or already one of our tickets (name it). Every review you post states the head SHA it reviewed. +- **Answer a review even if the PR has already merged.** Reviews arrive late, + and the thread is still where the next reader looks. - **Do not merge, and do not close beads.** Your part ends at an open PR plus your hand-back. What "done" means is in `AGENTS.md`, "Repository policy over the Beads blocks". Avoid stacked PRs. If you must stack, never let the base diff --git a/AGENTS.md b/AGENTS.md index 8a11780c..db781f6c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -184,8 +184,8 @@ earlier head, and the finding may already be fixed — verify rather than assume in either direction. Reviews arrive **as the owner's quota allows**, so absence is not evidence. -Wait where a review is plausible, and when one is found, answer it on the -thread even if the PR has already merged: that is where the next reader looks. +Wait where a review is plausible. How a review is answered once found, by the +author and by a reviewer, is `.agents/skills/wave/agent-rules.md`, "GitHub". **Copilot is not reviewing this repository, and the request is a silent no-op.** Measured 2026-09-20: PRs #12, #21 and #27 carry 2, 8 and 4 Copilot reviews; #61, @@ -231,8 +231,8 @@ normally.) Across four PRs Copilot has found genuine bugs and no false positives, so its findings are worth the loop. **But judge each one against the repo's doctrine -rather than applying it reflexively**, and when you decline, reply on the thread -with the reason. The distinction that has mattered: a finding can be *correct* +rather than applying it reflexively**. How a declined finding is answered is +agent-rules, "GitHub". The distinction that has mattered: a finding can be *correct* and still be wrong to fix in that PR. On #12, a non-ASCII `truncate` panic was applied (byte-identical on ASCII, so it cost no evidence) while a hint-line padding bug in the same file was declined — fixing it would have rewritten the @@ -251,21 +251,27 @@ is to branch from `gh/main`, cherry-pick, open a follow-up PR, and delete the stray re-creation. Check `gh pr view --json state` *before* pushing a fix to a PR you have not looked at in a few minutes, rather than after. -Merge only once a review has passed — an adversarial agent's while Copilot is -absent, and it must name the head SHA it reviewed. Prefer a **merge commit** — +Merge only once a review of the PR's current head has passed — an adversarial +agent's while Copilot is absent. What a review must state is agent-rules, +"GitHub". Prefer a **merge commit** — `AGENTS.md` says merges over rebases, and a PR's commits are individually meaningful. ### ADR pull requests -An ADR (`context/adr/`) merges on the user's approval, not on a review. The +An ADR (`context/adr/`) merges on the user's approval, **in addition to** green +CI, every review answered, and a passing adversarial review ("Shipping a +wave"). The approval is an extra gate. It replaces none of the others. The steps, modelled on PR #122 (ADR 004): 1. The PR proposes the ADR with its **open questions numbered**. 2. The user answers them on the PR. 3. The answers are folded into the ADR, in a section headed **"What the user decided"**. -4. The PR merges only after the user comments **"Approved"**. +4. The PR merges only after the user comments **"Approved"**, and only while + the other gates still hold. "Read the PR's reviews before merging it" still + applies after the approval. On #122 the user's own review landed as the PR + merged. ### Never hand-edit `.beads/issues.jsonl` @@ -296,7 +302,7 @@ bd show $(cat /tmp/bd-audit-ids.txt) 2>&1 \ `git ls-files` picks the files, and grep only reads them. The file list is the tracked files plus the untracked ones, minus anything git ignores. That leaves -out three things a `grep -r` over the same roots reads: +out two things a `grep -r` over the same roots reads: - **Build output.** After `./harness/build.sh`, `harness/serve/` holds rendered copies of `context/`, and `grep -r` reported three dead ids from them @@ -304,9 +310,14 @@ out three things a `grep -r` over the same roots reads: - **Other agents' worktrees.** In the orchestrator's checkout `.claude/worktrees/` holds every agent's worktree, dozens of copies of the repository with their unpushed work. -- **The skills twice.** `.claude/skills/*` are symlinks to directories under - `.agents/skills/`. `-d skip` passes over them, so the skills are read once, - through `.agents`. + +`-d skip` is there for the skills' symlinks, and it changes no answer. The +`.claude/skills/*` entries are symlinks to directories under `.agents/skills/`. +`git ls-files` lists each one as a file, and grep, handed a symlink to a +directory, prints `grep: : Is a directory` on stderr. `-d skip` silences +those lines. The skills are read once, through `.agents`, with or without the +flag: neither BSD nor GNU `grep -r` follows a symlink it meets while recursing, +so no id is ever counted twice. Plain `git grep` is not a substitute: on macOS its `-E` silently ignores `\b` and finds a fraction of the ids. @@ -346,8 +357,10 @@ real defect through: added to the exclusion list in the same commit**, or it arrives as a dead id that no amount of `bd` archaeology explains. -Note the recipe's own prose deliberately spells no live or dead id: a document -that lists ids as examples makes itself a source of audit noise. +The recipe's own prose spells no id as an *example*, because a document that +lists example ids makes itself a source of audit noise. The ids it does cite, +improvise-8haj and improvise-oujb, are the tickets behind two of the rules +above. They resolve, and the audit checks them like any other. One environment note: in a worktree-isolated agent the shell guard refuses a loop that passes a runtime-computed argument to `bd`, so the recipe above From e2b132f594d9e997bcb1483a177aad0325acc5a1 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:34:05 -0700 Subject: [PATCH 18/29] The width guard counts only drills whose ordered records the law compared (improvise-btyw) --- crates/improvise-command/src/command/cmd/determinism.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 4bdc7222..3dd638d1 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -61,9 +61,9 @@ fn one_coordinate_drills(model: &Model) -> Vec> { drills } -/// Records a compared drill's own effects freeze in `scratch`, counted only when the -/// drill narrowed the store: a whole-store drill never reads the index. -fn records_narrowed(scratch: &mut App, effects: Vec>) -> usize { +/// Records a compared drill froze, counted only if it narrowed the store and its +/// ordered records are in `compared`, the text the law compared (improvise-btyw). +fn records_narrowed(scratch: &mut App, effects: Vec>, compared: &str) -> usize { let store = scratch .model_state .workbook @@ -75,6 +75,7 @@ fn records_narrowed(scratch: &mut App, effects: Vec>) -> usize { let frozen = scratch .drill_state .as_ref() + .filter(|drill| compared.contains(&format!("{:?}", drill.records))) .map_or(0, |drill| drill.records.len()); if frozen < store { frozen } else { 0 } } @@ -135,7 +136,7 @@ fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { if name == drill.name() { found.widest_drill = found .widest_drill - .max(records_narrowed(&mut scratch, effects)); + .max(records_narrowed(&mut scratch, effects, &left)); } found.compared += 1; for b in others { From 83c3c7d3923b2e2b136a77b6c5fc27bc65e59c4f Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:34:45 -0700 Subject: [PATCH 19/29] Docs: the width guard reads the representation the law compared (improvise-btyw) --- context/code/command.md | 22 ++++++++++++++++------ context/test-evidence/command.md | 17 ++++++++++++++++- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index d1e04866..5732b5ae 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -3307,10 +3307,19 @@ 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. With the store's sort removed, the law fails 400 of 400 runs. -**The width guard measures what the comparison compared (improvise-b9f2).** The -test asserts that some `drill-into-cell` the comparison actually built and -compared froze at least four records. For each one it applies those very effects -to one scratch `App` and counts the records in its `drill_state`. A first +**The width guard measures what the comparison compared (improvise-b9f2, +improvise-btyw).** The test asserts that some `drill-into-cell` the comparison +actually built and compared froze at least four records. For each one it applies +those very effects to one scratch `App` and counts the records in its +`drill_state`. It counts them only when their `Debug` text, which carries their +order, appears in the text the comparison compared. The comparison reads `Debug` +while the scratch `App` reads the applied state, and those are two +representations. The review showed they can disagree: give `StartDrill` a +`Debug` that prints only a row count, and the frozen records are still four +while the compared text no longer holds their order. With the store's sort +removed the law then passed 300 of 300 runs, with the guard green. Requiring +the ordered records in the compared text ties the guard to what the comparison +could actually see. A first version measured what the *argument lists* match in the store instead. That was a parallel derivation, and the review beat it with a `drill-into-cell` parser that refuses fewer than two arguments. Every one-coordinate drill then went to @@ -3323,8 +3332,9 @@ Counting frozen records is not enough by itself. A drill over no coordinates never through the narrowing index where gotcha 18 lives. Under the parser rival that whole-store drill still froze 16 records, and a guard counting it stayed silent. So only a drill that **narrowed** the store counts: one that froze fewer -records than the store holds. With the rival in place, the guard now fires on -every run ("froze 1 records"). +records than the store holds. With the parser rival in place, the guard fires on +every run ("froze 1 records"), and it fires on every run under the `Debug` rival +too ("froze 0 records"). What the guard still cannot see is a narrowing drill whose records are frozen in an order that does not come from the store. It checks that the law had a wide diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 2d590635..a93569ea 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5388,13 +5388,28 @@ rivals it catches: and it reads them in insertion order through `iter_cells`, which the injection cannot reach. So the guard counts only drills that froze fewer records than the store holds. - - Measured on the final form, test binary in a loop: + - Measured at `7dfa90f1`, test binary in a loop: | Faults | Law passes | Guard fired | |---|---|---| | gotcha 18 + parser rival | **0 of 300** | all 300 | | gotcha 18 alone | **0 of 300** | 0 (every failure a real divergence) | | neither | 300 of 300 | 0 | +- **`StartDrill`'s `Debug` changed to print only its row count** + (`StartDrill(4 rows)`, improvise-btyw): the law compares effects by their + `Debug` text, and this rival takes the record order out of that text, while + the frozen `drill_state` still holds four records. At `7dfa90f1`, with gotcha + 18 re-injected, the law **passed 200 of 200** runs and the guard stayed green. + The guard now counts a drill only when its frozen records, as `Debug` prints + them, appear in the text the comparison compared. The same measurements after + the repair, 200 runs each: + + | Faults | Law passes | Guard fired | + |---|---|---| + | gotcha 18 + `Debug` rival | **0 of 200** | all 200 ("froze 0 records") | + | gotcha 18 + parser rival | **0 of 200** | all 200 | + | gotcha 18 alone | **0 of 200** | 0 (every failure a real divergence) | + | neither | 200 of 200 | 0 | - **A stale exemption**: `("quit", "stale")` listed in `OUTSIDE_THE_CONTEXT`. It fails as "listed … but did not". From a00fffc94eab7e9ccc3ea8ce27eec2656a619508 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:36:49 -0700 Subject: [PATCH 20/29] rustfmt the width guard (improvise-btyw) --- crates/improvise-command/src/command/cmd/determinism.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/determinism.rs b/crates/improvise-command/src/command/cmd/determinism.rs index 3dd638d1..06135353 100644 --- a/crates/improvise-command/src/command/cmd/determinism.rs +++ b/crates/improvise-command/src/command/cmd/determinism.rs @@ -134,9 +134,10 @@ fn compare(a: &CmdContext, others: &[CmdContext]) -> Comparison { }; let left = format!("{effects:?}"); if name == drill.name() { - found.widest_drill = found - .widest_drill - .max(records_narrowed(&mut scratch, effects, &left)); + found.widest_drill = + found + .widest_drill + .max(records_narrowed(&mut scratch, effects, &left)); } found.compared += 1; for b in others { From 0e846b8e41474a268db9c0906ce9bc184d7d8203 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:40:13 -0700 Subject: [PATCH 21/29] WIP: evidence and design notes for v4hg, 514o, 0v8l --- context/code/command.md | 34 ++++++++++++--- context/test-evidence/command.md | 75 ++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 6 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index 3dd16fd3..73373c52 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -592,10 +592,23 @@ the client was told nothing at all. key-path batch assumes the failed one happened. `refuse_and_stop` does it for `LoadModel` and every `ImportJsonHeadless` arm, so `load x . q` and `import-json x . q` do not quit having loaded nothing. `AddCategory` keeps its - own inline abort (gotcha 21). The item, formula and view effects only refuse: - the frame is rolled back by the refusal alone. On the key path, a failed - `AddItem`, `AddFormula` or view edit no longer stops the rest of its batch, - which is what it did before bok4. + own inline abort (gotcha 21). The item and view effects, and `AddFormula`'s + missing-category arm, only refuse: the frame is rolled back by the refusal + alone. On the key path, a failed `AddItem` or view edit no longer stops the + rest of its batch, which is what it did before bok4. +- **`AddFormula`'s parse-error arm also stops (improvise-v4hg).** It is the one + item-or-formula failure a key-path batch was found to depend on. Enter in + `FormulaEdit` is `[SetStatus, AddFormula, ChangeMode(FormulaPanel), + ClearBuffer("formula")]`, and the last two assume the commit worked. Without + the stop, `Total = (` was reported and then the editor closed and the typed + text was cleared. With it, the user stays in `FormulaEdit` with the text, and + the error is drawn beside the prompt. `fit-measure` emits `AddFormula` last, so + there is nothing after it to stop. The missing-category arm is reachable only + from a wire frame or a `:` line naming another category, and nothing in either + depends on it, so it does not stop. PR #181's review searched for an + `AddItem` case the same way and found none, so `AddItem` does not stop either. + An abort is added where a later effect depends on the failed one, never for + symmetry. In a client's frame, any refusal becomes `Rejected(MutationRefused)`. The frame is undone whole, and the client is told, which is the channel the narrated @@ -615,8 +628,17 @@ Consequences: refusal, not by `status_msg`. The frame's rollback also rolls the host's status line back. - **Every refusal site has a test that fails when it is reverted** to a - status-only write (improvise-9fpl). Most are one table, - `every_failing_model_command_refuses_its_whole_frame`. That matters because + status-only write (improvise-9fpl, improvise-0v8l). Two tables carry most of + them. `every_failing_model_command_refuses_its_whole_frame` covers the wire. + `every_refusal_site_refuses_its_failure_and_only_its_failure` covers the key + path and the `:` line, which is what the headless exit code reads. The wire + table cannot see the refusal half of a `refuse_and_stop` site, because + `Host::mutating_frame` rejects on `refused || aborted`, and the abort alone + suffices. So each `refuse_and_stop` site needs a case in both tables: + the key-path table for its refusal, and + `a_failed_load_or_import_does_not_run_the_quit_after_it` for its abort + (improvise-514o). Each key-path case has a succeeding twin, so a site + that refused unconditionally would fail too. That matters because these are exactly the lines a merge conflict could silently drop. - **Not covered:** a no-op *inside* a view that does resolve. Examples are `SetAxis` on a category the view does not have, `HideItem` of an absent diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index d86e0500..85d5da10 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5246,3 +5246,78 @@ 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". + +#### improvise-514o: every `refuse_and_stop` arm + +The case list above covered `load` and the JSON cannot-read arm only. PR #181's +review reverted each `refuse_and_stop` to `app.refuse` one at a time, which +keeps the refusal and drops the abort. Five `ImportJsonHeadless` arms failed +nothing: CSV cannot-read, CSV parse, JSON parse, array path and build. The wire +table `every_failing_model_command_refuses_its_whole_frame` cannot see the +abort, because `Host::mutating_frame` rejects on `refused || aborted`, and the +refusal alone suffices. The list now has one case per arm, using the fixtures +the wire table uses, which now come from `ui::host::import_fixture_storage`. +Each case also asserts the arm's own status text, so a fixture that failed in a +different arm cannot stand in for the one it names. The same scripted sweep, +rerun after the change, fails this test at all seven sites. Before, it failed +at two. + +### ui::app::tests::a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text + +improvise-v4hg. In `FormulaEdit`, Enter on `Total = (` reported the parse error +and then did what a successful commit does: `CommitBuffer`'s +`ChangeMode(FormulaPanel)` closed the editor and the keymap's welded +`ClearBuffer("formula")` cleared the text. The user saw the error with their +formula gone. Driven through `App::handle_key` from `FormulaPanel`: `a`, the +characters, then Enter. The test failed first with "got FormulaPanel". The fix is +`refuse_and_stop` on the parse-error arm, and a `Binding::Sequence` is one +effect vec, so the stop also covers the `ClearBuffer` step. + +The mode assertion alone would be vacuous against a fix that stops the mode +change but still loses the text. That rival (clear the `formula` buffer, then +`refuse_and_stop`) was run and fails "and must keep what they typed" with +`left: None`. The control, `Total = 1`, asserts the ordinary path still stores +the formula and closes the editor, so a stop on every arm would fail it. After +the fix, the abort sweep (`refuse_and_stop` back to `app.refuse`) fails this +test at that site. + +### ui::app::tests::every_refusal_site_refuses_its_failure_and_only_its_failure + +improvise-0v8l. PR #185 split refusals (`App::refuse`, which the headless exit +code reads) from reports (`set_status`), and most of the refusal sites were +unpinned. The ticket counted 23 of 27. At gh/main `608ba6a` (after #181's wire +table), a script (`/tmp/refuse-fu/sweep.py`, not committed) reverted each of +the **32** production refusal sites in `ui/effect.rs` and `command/cmd/*.rs` +alone to a status-only write, and ran `cargo test --workspace --no-fail-fast`. +For `effect::refuse(x)` the revert is `effect::set_status(x)`. For +`app.refuse(x)` it is `app.view_state.status_msg = x.into()`. For +`refuse_and_stop` it is the status write with the abort kept. **13 of 32 failed +a test. 19 survived:** `AddCategory` at the limit, `apply_coordinate_rename`, +both `StartImportWizard` arms, the five `ImportJsonHeadless` arms the wire +table cannot see (the abort alone rejects the frame), `rename_record_coordinate`, +`CommitExport`, both `ViewNavigate` arms, both `AddRecordRow` arms, both +`EditOrDrill` arms, `SearchNavigate` and `ExecuteCommand`. + +This table has one case per survivor. Each case drives the failure through a +`:` line (`run_line`, which does what `improvise cmd` does, one batch per +segment) or through Enter in the site's own minibuffer mode. It asserts that the +batch refused and that the status names the site's own reason. Each case also +has a **succeeding twin** that must not refuse, so a site that refused +unconditionally fails too. That rival (`AddCategory` refusing on `Ok` as well) +was run and fails "its succeeding twin was refused". Rerun after the change, +the sweep catches **32 of 32**. The abort half, 10 sites before v4hg's eleventh, +went from 5 caught to 11 of 11. + +Three fixture choices. The second was found when the first run failed: the input meant to be refused was accepted. + +- **`AddCategory` past the limit** needs eleven categories and a two-segment + line. The first `add-category` fills the twelfth slot and the second is + refused, so the succeeding twin is the same fixture with one segment. +- **`add-record-row` onto an existing row** must page the category on the + view the client is actually on. Records mode switches to `_Records`, so + writing `Workbook::active_view_mut` (the document's default) changed a view + nobody was looking at, and the case "succeeded". It goes through + `App::view_mut_of(&ViewTarget::Active)` (gotcha 29). +- **The drill rename** edits record 1, not record 0, following the + cursor-at-index-0 rule in the wave agent rules. Nothing failed here: this is + a precaution, not a finding. From 39291960968a02d19ad58b058d610d730fd371a0 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:38:43 -0700 Subject: [PATCH 22/29] clippy: needless borrow in the limit fixture --- crates/improvise-command/src/ui/app.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 3ede1b1f..5d4bba98 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -3142,7 +3142,7 @@ mod tests { fn one_category_short_of_the_limit() -> App { let mut wb = Workbook::new("T"); for n in 0..11 { - wb.add_category(&format!("C{n}")).unwrap(); + wb.add_category(format!("C{n}")).unwrap(); } App::new(wb, None) } From 6ccad4b80d3f68ff3a01fc2469db850d06b4b40a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:40:13 -0700 Subject: [PATCH 23/29] docs: only the user's explicit approval accepts or merges an ADR The user's ruling: "An ADR isn't approved until I say it is." CI, the reviews and the adversarial review stay required as gates, but none of them is approval. SKILL.md's "not on a review" pointer is reworded to match. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/wave/SKILL.md | 5 +++-- AGENTS.md | 13 ++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index f52ce28a..43152160 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -80,8 +80,9 @@ defect is a test that passes. "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, and a test-merge, so it reviews the tree that will exist. Point it at `agent-rules.md`, "GitHub", for what a reviewer reads and answers first. -- An ADR PR merges on the user's approval, not on a review: - `AGENTS.md`, "ADR pull requests". +- Only the user's explicit approval accepts or merges an ADR PR. The review + gates still apply, and none of them is approval: `AGENTS.md`, "ADR pull + requests". - A re-review is of the **delta**, at a named SHA. A review of a superseded commit reads as a pass. - Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. Close diff --git a/AGENTS.md b/AGENTS.md index db781f6c..89b29d2f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -259,17 +259,20 @@ meaningful. ### ADR pull requests -An ADR (`context/adr/`) merges on the user's approval, **in addition to** green -CI, every review answered, and a passing adversarial review ("Shipping a -wave"). The approval is an extra gate. It replaces none of the others. The -steps, modelled on PR #122 (ADR 004): +**Only the user's explicit approval accepts an ADR (`context/adr/`), and only +it lets an ADR PR merge.** "An ADR isn't approved until I say it is" (the +user). Green CI, every review answered and a passing adversarial review +("Shipping a wave") are still required. They are gates, never approval: an ADR +PR that passes all of them is still unapproved and does not merge. The steps, +modelled on PR #122 (ADR 004): 1. The PR proposes the ADR with its **open questions numbered**. 2. The user answers them on the PR. 3. The answers are folded into the ADR, in a section headed **"What the user decided"**. 4. The PR merges only after the user comments **"Approved"**, and only while - the other gates still hold. "Read the PR's reviews before merging it" still + the gates above still hold. Nobody else's comment or review stands in for + it. "Read the PR's reviews before merging it" still applies after the approval. On #122 the user's own review landed as the PR merged. From 08b87edd49b8b0f1c0bf6db3fb9b83f17d749677 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:40:45 -0700 Subject: [PATCH 24/29] improvise-y5eo: a document opening on a records view opens the host in RecordsNormal App::with_storage now ends with settle_normal_mode, the rule every batch ends with, so the host's startup mode matches the one Sessions::connect hands a client. #207's startup-slice test gains a records-opening case. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/command.md | 23 ++++++++++++++++ crates/improvise-command/src/ui/app.rs | 31 ++++++++++++++++++++-- crates/improvise-command/src/ui/project.rs | 20 +++++++++++--- 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 3ffb456a..acc90819 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -5599,3 +5599,26 @@ that is deliberate. Its helpers seed a client *after* mutating the host (mode, buffers, cursor), so a `Snapshot` would drop exactly the state those tests stage. This test is what makes the copy faithful at startup. +### a_document_opening_on_a_records_view_opens_the_host_in_records_normal + +improvise-y5eo. `App::with_storage` set `view_state.mode` to `Normal` and never +settled it against the layout. The host settles only at the end of a batch +(`settle_normal_mode` in `apply_effects`). A session's mirror settles in +`project_diff`, so `Sessions::connect` handed a client `RecordsNormal` while +the host sat in `Normal` over the same records grid. The host's first `i` then +resolved through `Normal`'s keymap and entered plain `Editing`. The fixture's +active view puts `_Index` on rows and `_Dim` on columns, which the `.improv` +writer does persist. The test asserts the starting mode, then presses `i` and +asserts the host is still in a records mode. The fix calls +`settle_normal_mode` once at the end of construction, so the startup mode +comes from the same rule every batch ends with, and so from the same +`improvise_protocol::settled_mode` a client uses. + +Rival injected: that one call removed. The whole workspace had exactly two +failures. This test failed at the mode assertion (`left: Normal, right: +RecordsNormal`), and `a_fresh_sessions_slice_is_the_hosts_startup_slice` +failed on its new records case: the client's `RecordsNormal` against the +host's `Normal`, and nothing else different. `install_workbook` needs no +change of its own, because both of its callers (`LoadModel`, +`AcceptWizardModel`) run inside `apply_effects`, which settles after the batch. + diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index 4969f2cb..f96ac49b 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -386,7 +386,9 @@ impl App { let view = workbook.active_view(); GridLayout::with_frozen_records(&workbook.model, view, None) }; - Self { + // A document may open on a records grid, so the first mode is settled + // by the rule every batch ends with (improvise-y5eo). + let mut app = Self { model_state: ModelState { workbook, file_path, @@ -416,7 +418,9 @@ impl App { last_reframed_elsewhere: std::collections::BTreeSet::new(), keymap_set: KeymapSet::default_keymaps(), storage, - } + }; + app.settle_normal_mode(); + app } /// This client's cursor in the active view. Absent means the origin, so @@ -2758,6 +2762,29 @@ mod tests { } } + /// improvise-y5eo: a document that opens on a records grid opens the host in + /// `RecordsNormal`. See `context/test-evidence/command.md`. + #[test] + fn a_document_opening_on_a_records_view_opens_the_host_in_records_normal() { + use crate::view::Axis; + let mut wb = Workbook::new("T"); + wb.add_category("Region").unwrap(); + wb.add_item("Region", "East").unwrap(); + wb.active_view_mut().set_axis("_Index", Axis::Row); + wb.active_view_mut().set_axis("_Dim", Axis::Column); + let mut app = App::new(wb, None); + assert!(app.layout.is_records_mode(), "setup: the grid is records"); + assert_eq!(app.view_state.mode, AppMode::RecordsNormal); + + app.handle_key(KeyEvent::new(KeyCode::Char('i'), KeyModifiers::NONE)) + .unwrap(); + assert!( + app.view_state.mode.is_records(), + "the host's first `i` left records mode: {:?}", + app.view_state.mode + ); + } + /// The settle's `RecordsNormal` arm, driven from the mode it names. The /// `:` crossing above enters the settle in `Normal` both ways — the evidence. #[test] diff --git a/crates/improvise-command/src/ui/project.rs b/crates/improvise-command/src/ui/project.rs index 73bd7c14..e2d3c088 100644 --- a/crates/improvise-command/src/ui/project.rs +++ b/crates/improvise-command/src/ui/project.rs @@ -1606,11 +1606,25 @@ mod tests { /// See `context/test-evidence/command.md`. #[test] fn a_fresh_sessions_slice_is_the_hosts_startup_slice() { - for names in [&["Default", "Second"][..], &["Main", "Second"][..]] { - let app = App::new(opening_on(names, "Second"), None); + for (names, records) in [ + (&["Default", "Second"][..], false), + (&["Main", "Second"][..], false), + (&["Main", "Second"][..], true), + ] { + let mut wb = opening_on(names, "Second"); + if records { + // `Second` opens on a records grid (improvise-y5eo). + wb.active_view_mut().set_axis("_Index", Axis::Row); + wb.active_view_mut().set_axis("_Dim", Axis::Column); + } + let app = App::new(wb, None); assert_eq!(app.view_state.active_view, "Second", "setup: {names:?}"); + assert_eq!(app.layout.is_records_mode(), records, "setup: {names:?}"); let (_, snapshot) = Sessions::default().connect(&app, 10, 10); - assert_eq!(snapshot.view, app.view_state, "{names:?}"); + assert_eq!( + snapshot.view, app.view_state, + "{names:?}, records {records}" + ); } } From a8737f3fa5ca9bcf33f8ec922ba55bfb7c32319d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:41:03 -0700 Subject: [PATCH 25/29] One page rule for both ends; the help overlay shows how to page improvise-5xbq: resolve::page_rows is now the one home of the page-scroll rule; the TUI's Move calls it instead of repeating visible_rows * 3 / 4. A lockstep case pages (PgDn/PgUp, Ctrl-d/Ctrl-u) on a non-square window, 8 rows by 3 columns, and pins the TUI's landing rows as well as comparing host and client, since one home moves both ends together. Both faults the ticket injected (page_rows = window_rows / 2; paging by window_cols) now fail at the first PgDn, before and after the move to one home. improvise-1c5m: the help overlay draws the mode's own hint (hint_line, the string #hint carries) in a sticky footer, so a narrow window whose app bar wraps under #help still says how to page. Playwright case at 600px, red at base, and red again with the footer made non-sticky. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 55 ++++++++++++++++ .../src/command/cmd/navigation.rs | 2 +- crates/improvise-protocol/src/resolve.rs | 3 +- .../improvise-wasm-client/tests/lockstep.rs | 64 +++++++++++++++++++ harness/tests/help.spec.js | 26 ++++++++ web/app.js | 8 ++- web/style.css | 11 ++++ 7 files changed, 166 insertions(+), 3 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index 15ff6714..ebe02a28 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4749,3 +4749,58 @@ claims at commit. The commit was then *accepted*. With the refusal assertion disabled, the host held `Some(Number(42.0))`, so A's commit had re-created the cell B cleared. Under that rival, the 0j0f test above failed as well. Both files were restored. + +### a_locally_resolved_page_scroll_lands_where_the_host_would_put_it + +**What was missing (improvise-5xbq).** `tests/lockstep.rs` compared only `j` +and `G` against the host. The browser's page rule (`resolve::page_rows`) and +the TUI's (`Move`'s `Page` arm, `visible_rows * 3 / 4`) were two copies, and +two faults injected into the browser's copy left the whole workspace green: +`page_rows` returning `window_rows / 2`, and `grid_move` paging by +`window_cols`. The second was invisible for a further reason: the only window +the page-rule test used was square, `(8, 8)`. + +**The fixture.** 20 rows by 3 columns; the host sees 8 rows and the client +announces `(8, 3)`. The three candidate rules then give three different +steps: 6 (three quarters of the rows), 4 (half the rows) and 2 (three quarters +of the columns). The walk starts off the edge (two `j`), then presses `PgDn`, +`PgUp`, `Ctrl-d` and `Ctrl-u`, including a `PgDn` that clamps at row 19. + +**Why it pins the rows as well as comparing the ends.** `page_rows` is now the +one home (the TUI's `Move` calls it), so a wrong rule moves host and client +together, and a comparison alone passes. Each press therefore also asserts the +row the TUI's rule names. + +**Injected, before and after the move to one home.** + +| Fault | Two copies | One home | +|---|---|---| +| `page_rows` returns `window_rows / 2` | fails at press 2 (`PgDn`), "client and host disagree" | fails at press 2, "not the row the TUI's page rule names"; `app.rs`'s `page_down_scrolls_by_three_quarters_visible` and `page_up_scrolls_backward` fail too | +| `grid_move` pages by `window_cols` | fails at press 2, "client and host disagree" | fails at press 2, "client and host disagree" | + +`Ctrl-d`/`Ctrl-u` are `scroll-rows ±5`, not the page rule. They are in the walk +because the ticket named them, and they move the start point of the next page +turn to a row the fixed rules alone would not reach. + +### a narrow window still shows how to page through help — `harness/tests/help.spec.js` + +**What broke (improvise-1c5m).** Below the width at which the app bar wraps, +`#help` (fixed, `inset: 2rem`, `z-index: 30`) covers `#hint`, and since PR #180 +Welcome carries no paging instruction of its own. So a new user on Welcome at +phone width could not see how to reach page two. + +**The fix is option (b) from the ticket.** The overlay draws `model.hint` in a +footer. In Help mode that is already `hint_line(Help)`, the same string `#hint` +carries, so it adds no derivation, and it sits outside `.help-body`, so +"Welcome is prose" still holds. The footer is `position: sticky` because the +overlay itself scrolls and Welcome is longer than a phone-height overlay. + +**What the case asserts.** At 600x800 on Welcome, it reads the hint string from +`#hint`, so the spec spells no keys. It then requires some element showing +exactly that string to be the topmost element at its own centre, which is the +`elementFromPoint` measurement the ticket made by hand. + +**Red, then a rival.** At base it failed with "the paging hint is under the +help overlay". With the fix and `position: sticky` changed to `static`, it +fails the same way: the footer exists but sits below the fold of a scrolling +overlay, so presence alone cannot satisfy the case. diff --git a/crates/improvise-command/src/command/cmd/navigation.rs b/crates/improvise-command/src/command/cmd/navigation.rs index 5f13f2cb..14523b5c 100644 --- a/crates/improvise-command/src/command/cmd/navigation.rs +++ b/crates/improvise-command/src/command/cmd/navigation.rs @@ -118,7 +118,7 @@ impl Cmd for Move { } } MoveKind::Page(dir) => { - let delta = (self.cursor.visible_rows as i32 * 3 / 4).max(1) * dir; + let delta = improvise_protocol::resolve::page_rows(self.cursor.visible_rows) * dir; let nr = (self.cursor.row as i32 + delta).clamp(0, row_max) as usize; (nr, self.cursor.col) } diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index d88b4e23..fa22ee24 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -620,7 +620,8 @@ enum GridMove { } /// Rows one `page-scroll` step moves: three quarters of the window, at least one. -fn page_rows(window_rows: usize) -> i32 { +/// The one home: the TUI's `Move` calls it too (improvise-5xbq). +pub fn page_rows(window_rows: usize) -> i32 { (window_rows as i32 * 3 / 4).max(1) } diff --git a/crates/improvise-wasm-client/tests/lockstep.rs b/crates/improvise-wasm-client/tests/lockstep.rs index 294b3fc0..07e51ceb 100644 --- a/crates/improvise-wasm-client/tests/lockstep.rs +++ b/crates/improvise-wasm-client/tests/lockstep.rs @@ -266,6 +266,70 @@ fn a_locally_resolved_cursor_move_lands_where_the_host_would_put_it() { ); } +/// Twenty rows by three columns, the host's window eight rows by three: not +/// square, so a rule keyed off the wrong axis lands somewhere else. +fn paging_host() -> App { + let mut wb = Workbook::new("Test"); + wb.add_category("Region").unwrap(); + for i in 0..20 { + wb.add_item("Region", format!("R{i:02}")).unwrap(); + } + for m in ["A", "B", "C"] { + wb.add_item("_Measure", m).unwrap(); + } + wb.active_view_mut().set_axis("Region", Axis::Row); + wb.active_view_mut().set_axis("_Measure", Axis::Column); + let mut app = App::new(wb, None); + app.term_height = 8 + 8; + app.rebuild_layout(); + app +} + +/// Page and half-page scrolls land in the same place on both ends, and in the +/// place the TUI's rule names. Why the fixture and the pinned rows: the evidence file. +#[test] +fn a_locally_resolved_page_scroll_lands_where_the_host_would_put_it() { + let window = (8, 3); + let mut app = paging_host(); + let mut client = client_of(&app, window); + let key = |code, mods| KeyEvent::new(code, mods); + let none = KeyModifiers::NONE; + let ctrl = KeyModifiers::CONTROL; + let presses = [ + (key(KeyCode::Char('j'), none), 1), + (key(KeyCode::Char('j'), none), 2), + (key(KeyCode::PageDown, none), 8), + (key(KeyCode::PageDown, none), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::Char('d'), ctrl), 13), + (key(KeyCode::PageDown, none), 19), + (key(KeyCode::Char('u'), ctrl), 14), + (key(KeyCode::PageUp, none), 8), + (key(KeyCode::PageUp, none), 2), + ]; + for (press, (event, want_row)) in presses.into_iter().enumerate() { + match resolve(&client, event) { + KeyOutcome::Local(cmds) => { + for cmd in &cmds { + client.fold(cmd); + } + } + other => panic!("press {press}: the client must answer {event:?}, got {other:?}"), + } + reduce_key(&mut app, event); + assert_eq!( + client.view.cursor(), + app.view_state.cursor(), + "press {press} ({event:?}): client and host disagree" + ); + assert_eq!( + client.view.cursor().selected, + (want_row, 0), + "press {press} ({event:?}): not the row the TUI's page rule names" + ); + } +} + /// The same for `G`, the key `GridExtent` exists for: it is *defined* as "the /// last row", so a window-only client would have nothing to jump to. #[test] diff --git a/harness/tests/help.spec.js b/harness/tests/help.spec.js index 54b9c961..6a2893e6 100644 --- a/harness/tests/help.spec.js +++ b/harness/tests/help.spec.js @@ -132,4 +132,30 @@ test.describe("the help overlay draws the shared content", () => { await expect(rows_.last().locator("td.selected")).toHaveCount(1); await expect(page.locator("#grid tbody td.selected")).toHaveCount(1); }); + + // improvise-1c5m. Below the width at which the app bar wraps, `#help` covers + // `#hint`, and Welcome itself says nothing about paging. The contract: the + // mode's own hint is UNCOVERED somewhere while help is open. + test("a narrow window still shows how to page through help", async ({ page }) => { + await page.setViewportSize({ width: 600, height: 800 }); + await openHelp(page); + + // The string is the page's own `#hint`, i.e. `hint_line(Help)`; this spec + // spells no keys. Asserting it names paging keeps an empty hint out. + const hint = (await page.locator("#hint").textContent()).trim(); + expect(hint).toContain("next-page"); + + // An element showing exactly that string, topmost at its own centre: the + // overlay covering `#hint` is precisely what `elementFromPoint` sees. + const uncovered = await page.evaluate((text) => + [...document.querySelectorAll("body *")].some((node) => { + if (node.children.length > 0 || node.textContent.trim() !== text) return false; + const r = node.getBoundingClientRect(); + if (r.width === 0 || r.height === 0) return false; + const top = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2); + return top !== null && (top === node || node.contains(top)); + }), + hint); + expect(uncovered, "the paging hint is under the help overlay").toBe(true); + }); }); diff --git a/web/app.js b/web/app.js index 22e94a34..31289ce1 100644 --- a/web/app.js +++ b/web/app.js @@ -348,7 +348,13 @@ function paintHelp(model) { } } - el.help.replaceChildren(tabs, body); + // The mode's own hint again, as the TUI popup's footer: once the app bar + // wraps, the overlay covers `#hint` (improvise-1c5m). + const footer = document.createElement("p"); + footer.className = "help-footer"; + footer.textContent = model.hint; + + el.help.replaceChildren(tabs, body, footer); } /** The page bar: which item each fixed category is fixed to. Without it a diff --git a/web/style.css b/web/style.css index 88ed64cc..6d02e7d1 100644 --- a/web/style.css +++ b/web/style.css @@ -736,6 +736,17 @@ td.total { font-weight: 700; } +/* Sticky, so the paging keys stay in view however long the page scrolls. */ +.help-footer { + position: sticky; + bottom: -1.25rem; + margin: 0.75rem 0 0; + padding: 0.5rem 0; + border-top: 1px solid var(--line); + background: var(--bg); + color: var(--dim); +} + .help-body { display: grid; grid-template-columns: 16rem 1fr; From ebf3dd2ba2257e482ae0e2d5bde78827f57897b5 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:49:40 -0700 Subject: [PATCH 26/29] tooling: pr-reviews polls past the second poll; stale harness lock names its fix improvise-7epn: two cases on the one-tick-per-read clock. One resolves on poll 3, and one times out after polling up to the deadline. Replacing the post-poll deadline check with `break` now fails exactly those two (23 passed, 2 failed), where before it left the suite 23/23. improvise-lnsh: when `cargo metadata --locked` refuses the opfs-contract lock, build.sh now says not to remove --locked and names `cargo update -w --manifest-path harness/opfs-contract/Cargo.toml`. Checked by hand against a lock with pest pinned to 2.9.1. Evidence is in context/test-evidence/tooling.md. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 47 ++++++++++++++++++++++++++++++++ harness/build.sh | 6 +++- scripts/pr-reviews-test.sh | 33 ++++++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 3304bd5e..820e622c 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -763,3 +763,50 @@ to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html` is served, naming the fix. Injected by running `web/assemble.sh harness/serve` alone: 4 chromium failures, each carrying that message, where it used to fail with no hint of the cause. + +### Polling past the second poll — `scripts/pr-reviews-test.sh` (improvise-7epn) + +**What was unguarded.** Every multi-poll fixture resolved on poll 2, and the +timeout case expects exit 1 whether the loop polls twice or twenty times. So +replacing the post-poll deadline check with an unconditional `break` left the +suite at 23 passed, 0 failed, and `--wait N` would have polled at most twice. + +**Two cases, both on the one-tick-per-read clock.** "a review arriving on the +third poll is seen" empties polls 1 and 2 and puts an inline comment at the head +in `last`, with `--wait 10`, and asserts exit 0 *and* exactly 3 polls. "--wait +keeps polling until its deadline" runs the empty fixture with `--wait 5` and +asserts exit 1 after at least 5 polls. The second one is the timeout path. Its +exit code alone cannot tell a loop that stopped early from one that reached the +deadline, so it counts polls. + +**Rival, injected and watched.** `break` in place of +`[ "$(date +%s)" -lt "$deadline" ] || break`: 23 passed, 2 failed. The failures +were exactly the two new cases, each "exit 1 after 2 polls". The first attempt +to inject used `sed -i ''`, which the GNU sed on this PATH read as a filename. +Nothing changed and the suite passed 25/25, which looks like a vacuous rival. +The second attempt asserted the replacement landed before running. + +**Still not covered.** Deleting the check outright makes the timeout case loop +forever rather than fail. The suite has no wall-clock guard, so that fault shows +as a hang, not a FAIL. + +### A stale harness lock names its remedy — `harness/build.sh` (improvise-lnsh) + +**What broke.** With a stale `harness/opfs-contract/Cargo.lock`, `build.sh` +exited 101 and printed only cargo's text, whose `help:` line says to remove +`--locked`. That is the one change that brings improvise-mkbp back. The real +remedy was only in a source comment. + +**Reproduced, then fixed, by hand.** Changing `pest`'s locked version from +`2.9.2` to `2.9.1` in the lock reproduces the stale state. Before the fix: +exit 101 and cargo's hint only. After: exit 1, and the script's own two lines +follow cargo's, telling the reader not to remove `--locked` and naming +`cargo update -w --manifest-path harness/opfs-contract/Cargo.toml`. The lock +was untouched both times. Running that command on the stale lock restored it +byte for byte (`cmp` against the committed file). With the committed lock, +`build.sh` still runs to "harness/serve ready". + +**Not automated.** There is no suite for `harness/build.sh`. A test would have +to corrupt a tracked file and call cargo, which needs the crates.io index, from +`checkers.sh`, whose environment is scrubbed. The by-hand run above is the +evidence. diff --git a/harness/build.sh b/harness/build.sh index ed73ca64..666b1165 100755 --- a/harness/build.sh +++ b/harness/build.sh @@ -22,7 +22,11 @@ serve="$root/harness/serve" # re-resolves the lock itself before any `-- --locked` reaches cargo build. # Fix a failure with `cargo update -w` in harness/opfs-contract, and commit it. cargo metadata --locked --format-version 1 \ - --manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null + --manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null || { + echo "harness/build.sh: harness/opfs-contract/Cargo.lock is stale. Do not remove --locked." >&2 + echo " Run: cargo update -w --manifest-path harness/opfs-contract/Cargo.toml, and commit the lock." >&2 + exit 1 +} "$root/web/build.sh" >/dev/null "$root/web/assemble.sh" "$serve" harness >/dev/null diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index 520315fe..0bda68f8 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -217,6 +217,39 @@ got=$? [ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \ || bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0" +# Every fixture above resolves by poll 2, so a loop that stops after one +# re-poll passed them all (improvise-7epn). This one resolves on poll 3. +d3="$work/arrives-third" +for n in 1 2; do + mk "$d3" "$n.pr.json" < "$work/out" 2>&1 +got=$? +polls=$(cat "$d3/.poll" 2>/dev/null || echo 0) +[ "$got" = 0 ] && [ "$polls" = 3 ] && ok "a review arriving on the third poll is seen" \ + || bad "a review arriving on the third poll is seen" "exit $got after $polls polls, wanted 0 after 3" + +# The timeout path: nothing ever arrives, so --wait 5 on a clock that ticks +# once per read must keep polling until the deadline, then report exit 1. +rm -f "$work/empty/.poll" "$work/ticks" +CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$work/empty" \ + "$REVIEWS" 1 --wait 5 --interval 1 > "$work/out" 2>&1 +got=$? +polls=$(cat "$work/empty/.poll" 2>/dev/null || echo 0) +[ "$got" = 1 ] && [ "$polls" -ge 5 ] && ok "--wait keeps polling until its deadline" \ + || bad "--wait keeps polling until its deadline" "exit $got after $polls polls, wanted 1 after 5 or more" + # --------------------------------------------------------------------------- # `gh api --paginate` emits one array PER PAGE, so a thread past the page size # arrives as several concatenated documents. Passing that straight to From 5e37973fc09824e251ff48fd489b9a0c703c9661 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:51:50 -0700 Subject: [PATCH 27/29] The TUI help footer's keys come off the keymap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-17ak: render_footer hand-wrote l/Tab, h/S-Tab and q/Esc, a key table no check could read, and already short of the keymap (n, → and ? were bound and not advertised). It now draws the page indicator followed by hint_line(Help), truncated in cells. Derived rather than deleted: the status row carries the same hint, but draw_status prints status_msg instead of it whenever a message is standing, and nothing clears the message on entering Help, so deleting the footer's keys would leave a user who saved and then pressed ? with none. The guard renders the popup at 140 columns on every page and requires the footer row to contain hint_line(Help). Red at base; and a literal copy of today's string fails it as soon as the keymap drops a binding. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tui.md | 34 +++++++++++++ src/ui/help.rs | 92 +++++++++++++++--------------------- 2 files changed, 72 insertions(+), 54 deletions(-) diff --git a/context/test-evidence/tui.md b/context/test-evidence/tui.md index dd6462b9..080a27b0 100644 --- a/context/test-evidence/tui.md +++ b/context/test-evidence/tui.md @@ -836,3 +836,37 @@ tests pass — including both of the ones *aimed* at the suppression — and onl this one fails. That is the shape design-principles §7 names: the assertion next door is satisfied by the wrong fix, and the discriminating input is the mode the change was not supposed to touch. + +### the_help_footer_names_the_keys_the_keymap_binds + +**What was wrong (improvise-17ak).** `src/ui/help.rs::render_footer` +hand-wrote the popup's paging keys: `l/Tab: next`, `h/S-Tab: prev`, +`q/Esc: close`. No check read them, since `every_hint_key_resolves_in_its_mode` +cannot see a string literal in a widget (the blind spot improvise-rr8n and +improvise-wk18 name). The literal was also already incomplete: the keymap binds +`help-page-next` to `n` and `→` as well, and `?` closes help, and the footer +advertised none of them. + +**Derive rather than delete.** rr8n's rule is to keep whichever table is +derived from the thing it describes, and the derived one here is the status +row's `hint_line(Help)`. The popup's margins always leave that row uncovered, +which argues for deleting the footer's keys. But `draw_status` prints +`status_msg` *instead of* `hint_text()` whenever the message is non-empty, and +nothing clears the message on entering Help. So after any message ("Saved to +…", an error), a user who presses `?` would see no paging keys at all. The +footer therefore now draws the page indicator followed by `hint_line(Help)` +itself, truncated in display cells (gotcha 50). + +**The test.** It renders the popup at 140×30, wide enough that the whole hint +fits (the rr8n lesson: an 80-column render can hide the very thing asserted), +on every page. It requires the footer row to contain `hint_line(&AppMode::Help)` +exactly, and first asserts that hint is non-empty and pages. + +**Red, and the rival that matters.** +- At base it fails on page 0: `"page 1 of 5 l/Tab: next q/Esc: close"` is + not `"Tab/l/n/→:next-page S-Tab/h/p/←:prev-page ?/Esc/q:close"`. +- A footer holding a literal copy of today's derived string passes today, so it + was injected together with a rebinding (`n` removed from `help-page-next` in + the protocol keymap). The test then fails: the copy still advertises `n`, + which the keymap no longer binds. That rebinding case is exactly the one the + ticket said the old footer would lie through. diff --git a/src/ui/help.rs b/src/ui/help.rs index 30fffcfd..86e5863b 100644 --- a/src/ui/help.rs +++ b/src/ui/help.rs @@ -6,6 +6,7 @@ use ratatui::{ }; use crate::protocol::help::{self, HelpLine}; +use crate::ui::text::truncate; /// Number of help pages available (defined alongside the help-page state). pub use crate::ui::app::HELP_PAGE_COUNT; @@ -228,66 +229,49 @@ fn render_tab_bar(buf: &mut Buffer, x: u16, y: u16, width: u16, active_page: usi } } -/// Renders the footer with page navigation hints. +/// The page indicator, then Help mode's own hint off the keymap: a footer that +/// spelled its keys was a second table nothing checked (improvise-17ak). fn render_footer(buf: &mut Buffer, x: u16, y: u16, width: u16, page: usize) { let dim = Style::default().fg(Color::DarkGray); - let key_style = Style::default().fg(Color::Cyan); + let hint = crate::protocol::hint_line(&crate::ui::app::AppMode::Help); + let line = format!(" page {} of {} {hint}", page + 1, HELP_PAGE_COUNT); + buf.set_string(x, y, truncate(&line, width as usize), dim); +} - let page_indicator = format!(" page {} of {} ", page + 1, HELP_PAGE_COUNT); +#[cfg(test)] +mod tests { + use super::*; + use crate::ui::app::AppMode; - let nav_parts: Vec<(&str, Style)> = if page == 0 && HELP_PAGE_COUNT > 1 { - vec![ - (" ", dim), - ("l", key_style), - ("/", dim), - ("Tab", key_style), - (": next", dim), - ] - } else if page >= HELP_PAGE_COUNT - 1 { - vec![ - (" ", dim), - ("h", key_style), - ("/", dim), - ("S-Tab", key_style), - (": prev", dim), - ] - } else { - vec![ - (" ", dim), - ("h", key_style), - (": prev ", dim), - ("l", key_style), - (": next", dim), - ] - }; - - let close_parts: Vec<(&str, Style)> = vec![ - (" ", dim), - ("q", key_style), - ("/", dim), - ("Esc", key_style), - (": close", dim), - ]; - - // Render page indicator on the left - buf.set_string(x, y, &page_indicator, dim); - - // Render navigation hints after the page indicator - let mut col = x + page_indicator.len() as u16; - for (text, style) in &nav_parts { - if col >= x + width { - break; - } - buf.set_string(col, y, text, *style); - col += text.len() as u16; + fn rows(buf: &Buffer) -> Vec { + let w = buf.area().width as usize; + buf.content() + .chunks(w) + .map(|row| row.iter().map(|c| c.symbol()).collect::()) + .collect() } - // Render close hint - for (text, style) in &close_parts { - if col >= x + width { - break; + /// The footer's keys are `hint_line(Help)`'s, not a table of its own + /// (improvise-17ak). Why the width and every page: `context/test-evidence/tui.md`. + #[test] + fn the_help_footer_names_the_keys_the_keymap_binds() { + let hint = crate::protocol::hint_line(&AppMode::Help); + assert!( + hint.contains("next-page"), + "the derived hint pages: {hint:?}" + ); + let area = Rect::new(0, 0, 140, 30); + for page in 0..HELP_PAGE_COUNT { + let mut buf = Buffer::empty(area); + HelpWidget::new(page).render(area, &mut buf); + let rows = rows(&buf); + let marker = format!("page {} of", page + 1); + let footer = rows.iter().find(|row| row.contains(&marker)); + let footer = footer.unwrap_or_else(|| panic!("page {page}: no footer row")); + assert!( + footer.contains(&hint), + "page {page}: the footer is not the keymap's hint {hint:?}: {footer:?}" + ); } - buf.set_string(col, y, text, *style); - col += text.len() as u16; } } From 41aa4073f1abc7312a703d559d11c2aafb1e6d3a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:53:01 -0700 Subject: [PATCH 28/29] docs: -d skip reason measured; the ADR exception at both merge rules improvise-28ri, from the #211 review at 6ccad4b: the recipe's grep has no -r, so the skills' symlinks arrive as directory operands and are never read. -d skip only silences "Is a directory", and sort -u would absorb a duplicate anyway. Measured with BSD grep 2.6.0 and GNU grep 3.12: 3092 matches either way, 462 distinct ids. GNU grep -r follows those symlinks (3107), so -r must not be added. improvise-oga4: "Shipping a wave"'s opening and "Repository policy over the Beads blocks" now point at the ADR exception. The wave SKILL.md pointer is unwrapped, so a grep for "ADR pull requests" finds it. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .agents/skills/wave/SKILL.md | 6 +++--- AGENTS.md | 28 ++++++++++++++++------------ 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/.agents/skills/wave/SKILL.md b/.agents/skills/wave/SKILL.md index 43152160..43787a3b 100644 --- a/.agents/skills/wave/SKILL.md +++ b/.agents/skills/wave/SKILL.md @@ -80,9 +80,9 @@ defect is a test that passes. "pending". Dispatch an adversarial reviewer and give it the head SHA, the base, and a test-merge, so it reviews the tree that will exist. Point it at `agent-rules.md`, "GitHub", for what a reviewer reads and answers first. -- Only the user's explicit approval accepts or merges an ADR PR. The review - gates still apply, and none of them is approval: `AGENTS.md`, "ADR pull - requests". +- Only the user's explicit approval accepts an ADR, and lets its PR merge. The + review gates still apply, and none of them is approval: + `AGENTS.md`, "ADR pull requests". - A re-review is of the **delta**, at a named SHA. A review of a superseded commit reads as a pass. - Merge on GitHub, then `git fetch gh && git merge --ff-only gh/main`. Close diff --git a/AGENTS.md b/AGENTS.md index 89b29d2f..6e66e82c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -131,9 +131,10 @@ and watch the check fail. **Do not push `main` to `gh/main`.** Each agent's work reaches GitHub as its own branch and its own pull request, and lands on `main` by being merged there, once -CI is green and an adversarial review agent has passed the PR's head SHA. The -orchestrator does not integrate agent branches locally: its `main` only -fast-forwards to `gh/main` after a merge. The procedure is +CI is green and an adversarial review agent has passed the PR's head SHA. An ADR +PR also needs the user's explicit approval, and nothing else stands in for it: +"ADR pull requests" below. The orchestrator does not integrate agent branches +locally: its `main` only fast-forwards to `gh/main` after a merge. The procedure is `.agents/skills/wave/SKILL.md`, "Shepherding". `origin` and `gitea` are mirrors and take `main` directly. @@ -315,12 +316,14 @@ out two things a `grep -r` over the same roots reads: repository with their unpushed work. `-d skip` is there for the skills' symlinks, and it changes no answer. The -`.claude/skills/*` entries are symlinks to directories under `.agents/skills/`. -`git ls-files` lists each one as a file, and grep, handed a symlink to a -directory, prints `grep: : Is a directory` on stderr. `-d skip` silences -those lines. The skills are read once, through `.agents`, with or without the -flag: neither BSD nor GNU `grep -r` follows a symlink it meets while recursing, -so no id is ever counted twice. +`.claude/skills/*` entries are symlinks to directories under `.agents/skills/`, +and `git ls-files` lists each one, so each reaches grep as a command-line +operand. The grep has no `-r`, so it never reads a directory operand. It prints +`grep: : Is a directory` on stderr instead, and `-d skip` only silences +those lines. The output goes through `sort -u`, so a duplicate could not change +the answer anyway. Measured with BSD grep 2.6.0 and GNU grep 3.12: 3092 matches +with or without the flag, 462 distinct ids. Do not add `-r`: GNU grep then +follows those symlinks and reads the skills a second time (3107 matches). Plain `git grep` is not a substitute: on macOS its `-E` silently ignores `\b` and finds a fraction of the ids. @@ -392,9 +395,10 @@ regeneration. Repository policy lives here, outside them: - **Work is not done until it is in a pull request and that pull request has merged.** A pushed branch is not done, and neither is a green one. In a wave, the merge is the orchestrator's, after an adversarial review passes the head - SHA ("Shipping a wave"). So a wave agent's part ends at an open PR plus its - hand-back, and the ticket is done, and its bead closed, only when the merge - lands. + SHA ("Shipping a wave"). An ADR PR merges only on the user's explicit + approval as well ("ADR pull requests"). So a wave agent's part ends at an + open PR plus its hand-back, and the ticket is done, and its bead closed, only + when the merge lands. - **This repository does not opt in to the Beads "team-maintainer" profile.** So the pull (`git pull`, or `git pull --rebase` in bd's "full" profile) and the bare `git push` that the block lists for that profile are not From 7da905cdbb1079be0ea208a93cb5ba5d7e59fba2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Wed, 23 Sep 2026 07:59:08 -0700 Subject: [PATCH 29/29] command.md: say where each refuse_and_stop refusal is held (improvise-rtud) #213 merged before this correction was pushed; the push re-created a branch main never saw. This lands the final wording on gh/main. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/command.md | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/context/code/command.md b/context/code/command.md index ab715f0a..294c6d8e 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -634,12 +634,18 @@ Consequences: path and the `:` line, which is what the headless exit code reads. The wire table cannot see the refusal half of a `refuse_and_stop` site, because `Host::mutating_frame` rejects on `refused || aborted`, and the abort alone - suffices. So each `refuse_and_stop` site needs a case in both tables: - the key-path table for its refusal, and - `a_failed_load_or_import_does_not_run_the_quit_after_it` for its abort - (improvise-514o). Each key-path case has a succeeding twin, so a site - that refused unconditionally would fail too. That matters because - these are exactly the lines a merge conflict could silently drop. + suffices. So each `refuse_and_stop` site needs a key-path assertion for its + refusal as well as one for its abort. The abort is + `a_failed_load_or_import_does_not_run_the_quit_after_it` (improvise-514o). + Three sites are not in the key-path table (improvise-rtud). The quit test's + "setup:" assertion holds the refusal of `load` and `import-json absent.json`. + `a_formula_that_does_not_parse_keeps_the_user_in_the_editor_with_their_text` + holds `AddFormula`'s parse arm, which has no case in the quit test. The other + five import arms are in + `every_refusal_site_refuses_its_failure_and_only_its_failure`. + Each case in that table has a succeeding twin, so a site that refused + unconditionally would fail too. That matters because these are exactly the + lines a merge conflict could silently drop. - **Not covered:** a no-op *inside* a view that does resolve. Examples are `SetAxis` on a category the view does not have, `HideItem` of an absent item, and `RemoveItem`/`RemoveCategory`/`RemoveFormula` of something already