From afba3fa0a6fd7a831c8888f8040a58a8c87283ee Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:50:48 -0700 Subject: [PATCH 1/9] WIP improvise-2bac: interactive arm is parse-then-contextualize Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/command/cmd/core.rs | 32 +- .../src/command/cmd/panel.rs | 20 +- .../src/command/cmd/registry.rs | 314 +++++++----------- tests/headless_cli.rs | 79 +++++ 4 files changed, 238 insertions(+), 207 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 4af2470f..491718cb 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -139,9 +139,12 @@ pub trait Cmd: Debug + Send + Sync { /// Factory that constructs a Cmd from text arguments (headless/script). pub type ParseFn = fn(&[String]) -> Result, String>; -/// Factory constructing a Cmd from the interactive context. It takes both the keymap -/// args and the context, so a command can combine text args with runtime state. -pub type InteractiveFn = fn(&[String], &CmdContext) -> Result, String>; +/// Fills the context-only fields of a command `parse` already built. It never +/// sees the args, so it cannot discard them — `context/code/cli.md` §8. +pub type ContextualizeFn = fn(C, &CmdContext) -> C; + +/// Builds a command from context, reached ONLY for args `parse` refused. +pub type FallbackFn = fn(&[String], &CmdContext) -> Result; type BoxParseFn = Box Result, String>>; type BoxInteractiveFn = Box Result, String>>; @@ -183,13 +186,26 @@ impl CmdRegistry { name } - /// Register a command with both a text parser and an interactive constructor. - /// The name is derived from a prototype command instance. - pub fn register(&mut self, prototype: &dyn Cmd, parse: ParseFn, interactive: InteractiveFn) { + /// Register a command whose interactive arm is `parse` then `contextualize`, so + /// args `parse` accepts are honoured whole — `context/code/cli.md` §8. + pub fn register( + &mut self, + prototype: &C, + parse: fn(&[String]) -> Result, + contextualize: ContextualizeFn, + fallback: Option>, + ) { self.entries.push(CmdEntry { name: prototype.name(), - parse: Box::new(parse), - interactive: Box::new(interactive), + parse: Box::new(move |args| parse(args).map(|c| Box::new(c) as Box)), + interactive: Box::new(move |args, ctx| { + let built = match (parse(args), fallback) { + (Ok(cmd), _) => Ok(contextualize(cmd, ctx)), + (Err(_), Some(fallback)) => fallback(args, ctx), + (Err(err), None) => Err(err), + }; + built.map(|c| Box::new(c) as Box) + }), }); } diff --git a/crates/improvise-command/src/command/cmd/panel.rs b/crates/improvise-command/src/command/cmd/panel.rs index 29d1153d..1d27a42f 100644 --- a/crates/improvise-command/src/command/cmd/panel.rs +++ b/crates/improvise-command/src/command/cmd/panel.rs @@ -20,8 +20,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, - open: true, - focused: true, + target: Some((true, true)), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); @@ -40,8 +39,7 @@ mod tests { let ctx = make_ctx(&m, &layout, ®); let cmd = TogglePanelAndFocus { panel: effect::Panel::Formula, - open: false, - focused: false, + target: Some((false, false)), }; let effects = cmd.execute(&ctx); assert_eq!(effects.len(), 2); @@ -531,20 +529,24 @@ mod tests { #[derive(Debug)] pub struct TogglePanelAndFocus { pub panel: Panel, - pub open: bool, - pub focused: bool, + /// Explicit `(open, focused)`; `None` toggles: close if open and focused, else open and focus. + pub target: Option<(bool, bool)>, } impl Cmd for TogglePanelAndFocus { fn name(&self) -> &'static str { "toggle-panel-and-focus" } - fn execute(&self, _ctx: &CmdContext) -> Vec> { + fn execute(&self, ctx: &CmdContext) -> Vec> { + let (open, focused) = self.target.unwrap_or_else(|| { + let shown = ctx.panel_open(self.panel) && ctx.mode.panel() == Some(self.panel); + (!shown, !shown) + }); let mut effects: Vec> = Vec::new(); effects.push(Box::new(effect::SetPanelOpen { panel: self.panel, - open: self.open, + open, })); - if self.focused { + if focused { effects.push(effect::change_mode(self.panel.mode())); } else { effects.push(effect::change_mode(AppMode::Normal)); diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index de1d2985..d9f1d47a 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,15 +22,50 @@ use super::text_buffer::*; use super::tile::*; use super::wizard::*; -/// Explicit coordinates win; the cursor is the fallback — `context/code/cli.md`. -fn key_from_args_or_cursor(args: &[String], ctx: &CmdContext) -> Result { - if args.is_empty() { - ctx.cell_key().clone().ok_or("no cell at cursor".into()) - } else { - Ok(parse_cell_key_from_args(args)) +/// The cell under the cursor: what a coordinate command falls back to when given none. +fn cursor_key(ctx: &CmdContext) -> Result { + ctx.cell_key().clone().ok_or("no cell at cursor".into()) +} + +/// A `Move` built from args takes the live cursor as its origin. +fn move_from_cursor(cmd: Move, ctx: &CmdContext) -> Move { + Move { + cursor: CursorState::from_ctx(ctx), + ..cmd } } +/// A commit built from args still advances from the live cursor. +fn commit_from_cursor(cmd: CommitAndAdvance, ctx: &CmdContext) -> CommitAndAdvance { + CommitAndAdvance { + cursor: CursorState::from_ctx(ctx), + ..cmd + } +} + +/// `` alone: commit the edit buffer to the cell under the cursor. +fn commit_at_cursor( + args: &[String], + ctx: &CmdContext, + advance: AdvanceDir, +) -> Result { + let [mode] = args else { + return Err("takes a mode alone, or a mode, a value and coordinates".into()); + }; + Ok(CommitAndAdvance { + edit_mode: parse_mode_name(mode)?, + value: read_buffer(ctx, "edit"), + key: cursor_key(ctx)?, + advance, + cursor: CursorState::from_ctx(ctx), + }) +} + +/// For a command with no context-only fields: `parse`'s result is used as built. +fn as_parsed(cmd: C, _ctx: &CmdContext) -> C { + cmd +} + /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -51,14 +86,12 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("clear-cell requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(ClearCellCommand { + Ok(ClearCellCommand { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(ClearCellCommand { key })) + }) }, + as_parsed, + Some(|_, ctx| Ok(ClearCellCommand { key: cursor_key(ctx)? })), ); r.register_pure(&AddFormulaCmd(vec![]), AddFormulaCmd::parse); r.register_pure(&FitMeasure::prototype(), FitMeasure::parse); @@ -80,16 +113,8 @@ pub fn default_registry() -> CmdRegistry { r.register_pure(&ExportCmd(vec![]), ExportCmd::parse); r.register_pure(&WriteCmd(vec![]), WriteCmd::parse); r.register_pure(&HelpCmd(vec![]), HelpCmd::parse); - r.register( - &HelpPageNextCmd(vec![]), - HelpPageNextCmd::parse, - |_args, _ctx| Ok(Box::new(HelpPageNextCmd(vec![]))), - ); - r.register( - &HelpPagePrevCmd(vec![]), - HelpPagePrevCmd::parse, - |_args, _ctx| Ok(Box::new(HelpPagePrevCmd(vec![]))), - ); + r.register_pure(&HelpPageNextCmd(vec![]), HelpPageNextCmd::parse); + r.register_pure(&HelpPagePrevCmd(vec![]), HelpPagePrevCmd::parse); // ── Navigation (unified Move) ────────────────────────────────────── r.register( @@ -102,22 +127,14 @@ pub fn default_registry() -> CmdRegistry { require_args(args, 2)?; let dr = args[0].parse::().map_err(|e| e.to_string())?; let dc = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Relative(dr, dc), cursor: CursorState::default(), cmd_name: "move-selection", - })) - }, - |args, ctx| { - require_args(args, 2)?; - let dr = args[0].parse::().map_err(|e| e.to_string())?; - let dc = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Relative(dr, dc), - cursor: CursorState::from_ctx(ctx), - cmd_name: "move-selection", - })) + }) }, + move_from_cursor, + None, ); // Jump-to-edge commands: first/last row/col macro_rules! reg_jump { @@ -133,7 +150,7 @@ pub fn default_registry() -> CmdRegistry { cmd_name: $name, }, |_| { - Ok(Box::new(Move { + Ok(Move { kind: if $to_end { MoveKind::ToEnd($is_row) } else { @@ -141,19 +158,10 @@ pub fn default_registry() -> CmdRegistry { }, cursor: CursorState::default(), cmd_name: $name, - })) - }, - |_, ctx| { - Ok(Box::new(Move { - kind: if $to_end { - MoveKind::ToEnd($is_row) - } else { - MoveKind::ToStart($is_row) - }, - cursor: CursorState::from_ctx(ctx), - cmd_name: $name, - })) + }) }, + move_from_cursor, + None, ); }; } @@ -170,21 +178,14 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let n = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Relative(n, 0), cursor: CursorState::default(), cmd_name: "scroll-rows", - })) - }, - |args, ctx| { - require_args(args, 1)?; - let n = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Relative(n, 0), - cursor: CursorState::from_ctx(ctx), - cmd_name: "scroll-rows", - })) + }) }, + move_from_cursor, + None, ); r.register( &Move { @@ -195,28 +196,21 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let dir = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { + Ok(Move { kind: MoveKind::Page(dir), cursor: CursorState::default(), cmd_name: "page-scroll", - })) - }, - |args, ctx| { - require_args(args, 1)?; - let dir = args[0].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(Move { - kind: MoveKind::Page(dir), - cursor: CursorState::from_ctx(ctx), - cmd_name: "page-scroll", - })) + }) }, + move_from_cursor, + None, ); r.register( &EnterAdvance { cursor: CursorState::default(), }, |_| { - Ok(Box::new(EnterAdvance { + Ok(EnterAdvance { cursor: CursorState { row: 0, col: 0, @@ -227,13 +221,12 @@ pub fn default_registry() -> CmdRegistry { visible_rows: 20, visible_cols: 8, }, - })) + }) }, - |_, ctx| { - Ok(Box::new(EnterAdvance { - cursor: CursorState::from_ctx(ctx), - })) + |_, ctx| EnterAdvance { + cursor: CursorState::from_ctx(ctx), }, + None, ); // ── Cell operations ────────────────────────────────────────────────── @@ -245,14 +238,12 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("yank requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(YankCell { + Ok(YankCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(YankCell { key })) + }) }, + as_parsed, + Some(|_, ctx| Ok(YankCell { key: cursor_key(ctx)? })), ); r.register( &PasteCell { @@ -262,14 +253,12 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("paste requires at least one Cat/Item coordinate".into()); } - Ok(Box::new(PasteCell { + Ok(PasteCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(PasteCell { key })) + }) }, + as_parsed, + Some(|_, ctx| Ok(PasteCell { key: cursor_key(ctx)? })), ); // clear-cell is registered above (unified: ctx.cell_key() or explicit coords) @@ -313,21 +302,15 @@ pub fn default_registry() -> CmdRegistry { if args.is_empty() { return Err("drill-into-cell requires Cat/Item coordinates".into()); } - Ok(Box::new(DrillIntoCell { + Ok(DrillIntoCell { key: parse_cell_key_from_args(args), - })) - }, - |args, ctx| { - let key = key_from_args_or_cursor(args, ctx)?; - Ok(Box::new(DrillIntoCell { key })) + }) }, + as_parsed, + Some(|_, ctx| Ok(DrillIntoCell { key: cursor_key(ctx)? })), ); r.register_nullary(|| Box::new(ViewNavigate { forward: false })); - r.register( - &ViewNavigate { forward: true }, - |_| Ok(Box::new(ViewNavigate { forward: true })), - |_, _| Ok(Box::new(ViewNavigate { forward: true })), - ); + r.register_nullary(|| Box::new(ViewNavigate { forward: true })); r.register_pure(&EnterMode(AppMode::Normal), |args| { require_args(args, 1)?; Ok(Box::new(EnterMode(parse_mode_name(&args[0])?))) @@ -345,38 +328,20 @@ pub fn default_registry() -> CmdRegistry { r.register( &TogglePanelAndFocus { panel: Panel::Formula, - open: true, - focused: true, + target: None, }, |args| { // Parse: toggle-panel-and-focus [open] [focused] require_args(args, 1)?; let panel = parse_panel(&args[0])?; - let open = args.get(1).map(|s| s == "true").unwrap_or(true); - let focused = args.get(2).map(|s| s == "true").unwrap_or(open); - Ok(Box::new(TogglePanelAndFocus { - panel, - open, - focused, - })) - }, - |args, ctx| { - require_args(args, 1)?; - let panel = parse_panel(&args[0])?; - // Default interactive: if already open+focused -> close, else open+focus - let currently_open = ctx.panel_open(panel); - let currently_focused = ctx.mode.panel() == Some(panel); - let (open, focused) = if currently_open && currently_focused { - (false, false) // close - } else { - (true, true) // open + focus - }; - Ok(Box::new(TogglePanelAndFocus { - panel, - open, - focused, - })) + let target = args.get(1).map(|s| { + let open = s == "true"; + (open, args.get(2).map(|s| s == "true").unwrap_or(open)) + }); + Ok(TogglePanelAndFocus { panel, target }) }, + as_parsed, + None, ); r.register( &TogglePanelVisibility { @@ -386,20 +351,16 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let panel = parse_panel(&args[0])?; - Ok(Box::new(TogglePanelVisibility { + Ok(TogglePanelVisibility { panel, currently_open: false, - })) + }) }, - |args, ctx| { - require_args(args, 1)?; - let panel = parse_panel(&args[0])?; - let currently_open = ctx.panel_open(panel); - Ok(Box::new(TogglePanelVisibility { - panel, - currently_open, - })) + |cmd, ctx| TogglePanelVisibility { + currently_open: ctx.panel_open(cmd.panel), + ..cmd }, + None, ); r.register( &CyclePanelFocus { @@ -408,19 +369,18 @@ pub fn default_registry() -> CmdRegistry { view_open: false, }, |_| { - Ok(Box::new(CyclePanelFocus { + Ok(CyclePanelFocus { formula_open: false, category_open: false, view_open: false, - })) + }) }, - |_, ctx| { - Ok(Box::new(CyclePanelFocus { - formula_open: ctx.formula_panel_open, - category_open: ctx.category_panel_open, - view_open: ctx.view_panel_open, - })) + |_, ctx| CyclePanelFocus { + formula_open: ctx.formula_panel_open, + category_open: ctx.category_panel_open, + view_open: ctx.view_panel_open, }, + None, ); r.register( &MovePanelCursor { @@ -433,29 +393,26 @@ pub fn default_registry() -> CmdRegistry { require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; - Ok(Box::new(MovePanelCursor { + Ok(MovePanelCursor { panel, delta, current: 0, max: 0, - })) + }) }, - |args, ctx| { - require_args(args, 2)?; - let panel = parse_panel(&args[0])?; - let delta = args[1].parse::().map_err(|e| e.to_string())?; - let (current, max) = match panel { + |cmd, ctx| { + let (current, max) = match cmd.panel { Panel::Formula => (ctx.formula_cursor, ctx.model.formulas().len()), Panel::Category => (ctx.cat_panel_cursor, ctx.cat_tree_len()), Panel::View => (ctx.view_panel_cursor, ctx.workbook.views.len()), }; - Ok(Box::new(MovePanelCursor { - panel, - delta, + MovePanelCursor { current, max, - })) + ..cmd + } }, + None, ); r.register_nullary(|| Box::new(DeleteFormulaAtCursor)); r.register_nullary(|| Box::new(AddRecordRow)); @@ -490,11 +447,7 @@ pub fn default_registry() -> CmdRegistry { // ── Grid operations ────────────────────────────────────────────────── r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: true })); - r.register( - &ToggleGroupAtCursor { is_row: false }, - |_| Ok(Box::new(ToggleGroupAtCursor { is_row: false })), - |_, _| Ok(Box::new(ToggleGroupAtCursor { is_row: false })), - ); + r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: false })); r.register_nullary(|| Box::new(HideSelectedRowItem)); // ── Text buffer ────────────────────────────────────────────────────── @@ -537,27 +490,16 @@ pub fn default_registry() -> CmdRegistry { return Err("commit-cell-edit requires a mode, value, and coords".into()); } let edit_mode = parse_mode_name(&args[0])?; - Ok(Box::new(CommitAndAdvance { + Ok(CommitAndAdvance { key: parse_cell_key_from_args(&args[2..]), value: args[1].clone(), advance: AdvanceDir::Down, cursor: CursorState::default(), edit_mode, - })) - }, - |args, ctx| { - require_args(args, 1)?; - let edit_mode = parse_mode_name(&args[0])?; - let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; - Ok(Box::new(CommitAndAdvance { - key, - value, - advance: AdvanceDir::Down, - cursor: CursorState::from_ctx(ctx), - edit_mode, - })) + }) }, + commit_from_cursor, + Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Down)), ); r.register( &CommitAndAdvance { @@ -568,19 +510,8 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::editing(), }, |_| Err("commit-and-advance-right requires context".into()), - |args, ctx| { - require_args(args, 1)?; - let edit_mode = parse_mode_name(&args[0])?; - let value = read_buffer(ctx, "edit"); - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; - Ok(Box::new(CommitAndAdvance { - key, - value, - advance: AdvanceDir::Right, - cursor: CursorState::from_ctx(ctx), - edit_mode, - })) - }, + commit_from_cursor, + Some(|args, ctx| commit_at_cursor(args, ctx, AdvanceDir::Right)), ); r.register( &CommitBuffer { @@ -589,13 +520,14 @@ pub fn default_registry() -> CmdRegistry { |args| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; - Ok(Box::new(CommitBuffer { kind })) + Ok(CommitBuffer { kind }) }, - |args, ctx| { + as_parsed, + Some(|args, ctx| { require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; - Ok(Box::new(CommitBuffer { kind })) - }, + Ok(CommitBuffer { kind }) + }), ); r.register_nullary(|| Box::new(CommitExport)); r.register_nullary(|| Box::new(CommitSave)); @@ -880,6 +812,7 @@ mod panel_open_tests { ctx.registry .interactive("toggle-panel-and-focus", &args, &ctx) .expect("toggle-panel-and-focus resolves") + .execute(&ctx) ); assert!( dbg.contains("open: false"), @@ -897,6 +830,7 @@ mod panel_open_tests { ctx.registry .interactive("toggle-panel-and-focus", &args, &ctx) .expect("toggle-panel-and-focus resolves") + .execute(&ctx) ); assert!( dbg.contains("open: true"), diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index f5ae9bb6..1789f9c6 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -357,3 +357,82 @@ fn an_explicit_coordinate_still_beats_the_cursor() { "the named cell survived: {left:?}" ); } + +/// North=1, South=2, East=3 on the row axis; the cursor starts on North. +fn three_cells(scratch: &Scratch, name: &str) -> PathBuf { + let doc = three_rows(scratch, name); + run_cmd( + &[ + "set-cell 1 Region/North", + "set-cell 2 Region/South", + "set-cell 3 Region/East", + ], + &doc, + ); + assert_eq!(cell_lines(&doc).len(), 3, "the fixture wrote three cells"); + doc +} + +fn value_of(doc: &Path, item: &str) -> Option { + cell_lines(doc) + .into_iter() + .find(|l| l.contains(&format!("Region={item}"))) + .and_then(|l| l.rsplit(" = ").next().map(str::to_string)) +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn commit_cell_edit_writes_the_named_cell_and_spares_the_cursors() { + let scratch = Scratch::new("commit-explicit"); + let doc = three_cells(&scratch, "commit.improv"); + + let out = run_cmd( + &["move-selection 1 0 . commit-cell-edit editing 5 Region/East"], + &doc, + ); + assert!(out.status.success(), "the line failed: {}", stderr_of(&out)); + let cells = cell_lines(&doc); + assert_eq!(value_of(&doc, "East").as_deref(), Some("5"), "{cells:?}"); + assert_eq!( + value_of(&doc, "South").as_deref(), + Some("2"), + "the cell under the cursor was touched: {cells:?}" + ); + assert_eq!( + value_of(&doc, "North").as_deref(), + Some("1"), + "the origin cell was touched: {cells:?}" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_line_whose_gate_passes_runs_whole() { + let scratch = Scratch::new("commit-atomic"); + let doc = scratch.path("atomic.improv"); + let out = run_cmd(&["add-category Foo . commit-cell-edit editing 5 Foo/x"], &doc); + assert!( + !stderr_of(&out).contains("Parse error"), + "a line the gate accepted was refused part-way: {}", + stderr_of(&out) + ); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!( + text.lines().any(|l| l.contains("Foo=x") && l.ends_with("= 5")), + "the commit did not land: {text}" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_malformed_commit_line_is_refused_whole() { + let scratch = Scratch::new("commit-malformed"); + let doc = seeded(&scratch, "malformed.improv"); + let out = run_cmd(&["add-category Foo . commit-cell-edit editing 5"], &doc); + assert!(!out.status.success(), "a malformed line exited 0"); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!( + !text.contains("Foo"), + "the first half of a refused line landed: {text}" + ); +} From 7cc52bdb1d1c5b6d12e4a774a71f834dbe9d2f2a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:51:59 -0700 Subject: [PATCH 2/9] fmt-workspaces-test: one fault case per fixture package, root included (improvise-90ea) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- scripts/fmt-workspaces-test.sh | 55 +++++++++++++++++----------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh index 7e2f292d..8ad50dd0 100755 --- a/scripts/fmt-workspaces-test.sh +++ b/scripts/fmt-workspaces-test.sh @@ -76,36 +76,37 @@ run_case() { scaffold run_case "both workspaces clean is a pass" 0 "$work/repo" -# The load-bearing case. `cargo fmt --all` at the outer root cannot reach this -# file, and `cargo fmt -p inner-pkg` from the outer root exits 1 saying `not a -# member of the workspace` -- the usage error this whole script exists to split -# from a real verdict. +# One fault case PER PACKAGE, enumerated from the fixture rather than +# hand-picked: three hand-picked shapes each missed a rival, and the fourth +# missed the root package itself (improvise-90ea). Reasoning: +# context/test-evidence/tooling.md. The enumeration is a find over +# `[package]` manifests, deliberately NOT the script's own `[workspace]` + +# `cargo metadata` discovery, which is the thing under test. scaffold -printf '%s' "$DIRTY" > "$work/repo/nested/src/lib.rs" -if run_case "a fault in the NESTED workspace is reported" 1 "$work/repo"; then - case "$OUT" in - *nested/src/lib.rs*) ok " and the nested file is named" ;; - *) bad " and the nested file is named" "output did not name it: ${OUT:0:300}" ;; - esac +pkg_manifests=() +while IFS= read -r m; do pkg_manifests+=("$m"); done < <( + cd "$work/repo" && grep -rl --include=Cargo.toml '^\[package\]' . | sort) +# An empty or shrunken enumeration would make the loop below vacuous. +if [ "${#pkg_manifests[@]}" = 3 ]; then + ok "the fixture enumerates all three packages (root, member, nested)" +else + bad "the fixture enumerates all three packages" "found: ${pkg_manifests[*]:-none}" fi - -scaffold -printf '%s' "$DIRTY" > "$work/repo/a/src/lib.rs" -if run_case "a fault in the OUTER workspace is reported" 1 "$work/repo"; then - case "$OUT" in - *a/src/lib.rs*) ok " and the outer file is named" ;; - *) bad " and the outer file is named" "output did not name it: ${OUT:0:300}" ;; - esac - # The rival: spelling the per-root check `--all` instead of `-p ` - # walks PAST the nested workspace boundary (measured), so the OUTER file is - # reported a second time under the inner root. Counting is what sees that. - n="$(printf '%s\n' "$OUT" | grep -c 'a/src/lib.rs')" - if [ "$n" = 1 ]; then - ok " and it is named ONCE -- no root reaches past its own boundary" - else - bad " and it is named ONCE" "named $n times; a root is walking past its boundary" +for m in "${pkg_manifests[@]}"; do + rel="$(dirname "${m#./}")/src/lib.rs"; rel="${rel#./}" + scaffold + printf '%s' "$DIRTY" > "$work/repo/$rel" + if run_case "a fault in $rel is reported" 1 "$work/repo"; then + # Counting, not matching: `--all` for `-p` walks PAST the nested boundary + # (measured) and names an outer file a second time under the inner root. + n="$(printf '%s\n' "$OUT" | grep -cF "$work/repo/$rel")" + if [ "$n" = 1 ]; then + ok " and $rel is named exactly once" + else + bad " and $rel is named exactly once" "named $n times; output: ${OUT:0:300}" + fi fi -fi +done # A usage error must NOT wear a formatting failure's exit code. This is the # whole ticket: `cargo fmt -p opfs-contract` from the repo root exits 1. From 334e218c7b40cd47812a90a4d42c9054ae6d5c4d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:53:18 -0700 Subject: [PATCH 3/9] s8h7: i on an aggregate names drill, in help and in the browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the TUI, EditOrDrill hands an aggregated cell to DrillIntoCell, so `i` on an aggregate is drill by another door. The help page called it only "Edit current cell", and the browser refused it in words unrelated to `>`'s, so a user could not tell the two were one missing feature. - help.rs: the edit-or-drill row says it drills into an aggregate. - help.rs: Normal-mode Enter was captioned "Edit current cell (same as i)" but is bound to enter-advance alone, which only moves down; the row moves to Cursor movement and says so. - resolve.rs: the Aggregate arm now reads "an aggregate is edited by drilling in — " + not_yet("drill"), the same sentence `>` uses. - context/code/clients.md and the evidence entry reverse the recorded "different sentences on purpose" rationale, which was false about the TUI. - harness/tests/drill.spec.js follows the new wording. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/clients.md | 19 +++++- context/test-evidence/clients.md | 84 ++++++++++++++++++------ crates/improvise-protocol/src/help.rs | 66 ++++++++++++++++++- crates/improvise-protocol/src/resolve.rs | 51 ++++++++------ harness/tests/drill.spec.js | 17 ++--- 5 files changed, 186 insertions(+), 51 deletions(-) diff --git a/context/code/clients.md b/context/code/clients.md index c9daea61..b535916a 100644 --- a/context/code/clients.md +++ b/context/code/clients.md @@ -891,8 +891,23 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it. broadcasts nothing, so it reaches this user and no one else. This argument arrived as `edit_or_drill`'s, i.e. about `i`/`a` on an aggregate, and for a while it read as doctrine for `>` while `>` was still silent. Both doors now - refuse, and **they refuse with different sentences on purpose**: "this key is - not built yet" and "this cell is not yours to write" are different facts. + refuse, and **they name the same missing feature in the same words** — + `not_yet("drill")` — because they reach the same one. In the TUI, + `EditOrDrill` hands an aggregate straight to `DrillIntoCell`, so an aggregate + *is* writable there, by drilling; `i` on it is drill by another door. `i`'s + sentence adds only *why* it drilled rather than edited ("an aggregate is + edited by drilling in — …"), and `>` still says nothing about aggregates, + because it is refused whatever the cell is. + + This **reverses** what this bullet said until improvise-s8h7: that the two + refused with different sentences on purpose, "this key is not built yet" + against "this cell is not yours to write". The second half was false about + the application — nothing refuses a write to an aggregate; the TUI drills — + and the separate wording hid from a browser user that `i` and `>` were one + missing feature. The help page's `edit-or-drill` caption carried the same + error, calling `i` only "Edit current cell"; it now says it drills into an + aggregate, which is true in both front ends because the browser's refusal + names drill. Delivering drill is `improvise-obtu`'s, not this bullet's. - **The commit's write is not spelled again here.** It is `Client::typed_cell`'s, the same one click-to-edit uses, so a browser keyboard commit and a browser mouse commit cannot disagree about what `""` or `"42"` diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index fd39ad65..e130266a 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -1389,28 +1389,74 @@ happened* passes against a page that has stopped listening, so it presses `i` afterwards and requires `#status` to change. Without that, this file would be folklore with a green tick. -### the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons +### an_aggregate_edit_names_the_same_missing_drill_as_the_drill_key -The Rust half of the interim, and the assertion that carries the whole weight. -Two keys on the same aggregate cell must both refuse and must **not** refuse -with the same sentence: `>` says drill is unavailable, `i` says drilling into an -aggregate needs the host. Collapsing them into one message would be the -tempting tidy-up and would lose the distinction between "this key does nothing -here yet" and "this cell is not one you may write". +improvise-s8h7. Until it, this test was +`the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons` and asserted +the opposite: that `i` and `>` on an aggregate must refuse with *different* +sentences, "this key does nothing here yet" against "this cell is not one you +may write". The second half was false about the application. `EditOrDrill` +hands an aggregated cell to `DrillIntoCell`, so in the TUI `i` on an aggregate +**is** drill — nothing refuses the write, it drills — and the browser's +separate wording ("drilling into an aggregate needs the host") hid that `i` and +`>` are one missing feature. The reasoning is `context/code/clients.md`. -**The injection, and why it is not the obvious one.** Replacing `>`'s sentence -with the *aggregate* sentence looks like it should fail on the first assertion — -and does not: that sentence contains both "drill" and "browser", so it clears -the substring check. What fires is the anti-aggregate assertion lower down. The -claim that the second assertion is the load-bearing one was doubted on review, -tested, and confirmed; record it here because the first assertion reads like the -discriminating one and is not. +So the test now asserts four things on one aggregate fixture: `>` names drill +and the browser; `>` does **not** mention aggregates (it is refused whatever the +cell is); `i`'s sentence **contains** `>`'s sentence verbatim; and `i`'s +sentence mentions "aggregate", so it still says why it drilled rather than +edited. The fixture is `client_on(CellId::Aggregate(..))` rather than the old +empty `in_mode` state, because `i` needs a cell under the cursor to reach the +`Aggregate` arm at all — on an empty grid it answers nothing and only `>` was +ever exercised. -The harness sibling (`drill.spec.js`) asserts the same distinction in a browser, -but its `not.toContainText("aggregate")` is **decoration under the only -injection that exists** — Playwright throws at the positive assertion above it -first, so that line is never reached. The Rust assertion is the guard; the -harness one is end-to-end confirmation that the string survives the wire. +**Three injections, each failing at a different assertion.** Run, not reasoned: + +- `i` says only `not_yet("drill")` — the "why" dropped. Fails at the last + assertion, "and `i` must still say why". This is the tempting over-tidy fix. +- `>`'s `declined` subject becomes "drilling into an aggregate". Fails at the + anti-aggregate assertion on `>`. Note that `i` still contains `>`'s *old* + sentence here, because `edit_or_drill` calls `not_yet("drill")` itself rather + than going through `declined` — so the containment assertion alone would not + have caught a `>` that started blaming the cell. +- `i` paraphrases ("… drill needs the host"). Fails at the containment + assertion: the same feature named in different words is exactly what the + ticket reported. + +The harness sibling (`drill.spec.js`) asserts the same wording in a browser. +Its `not.toContainText("aggregate")` on `>` is still **decoration under the +only injection that exists** — Playwright throws at the positive assertion +above it first — so the Rust assertion is the guard and the harness is +end-to-end confirmation that the string survives the wire. + +### the_edit_key_row_says_it_drills_into_an_aggregate + +improvise-s8h7, the help-page half. The `edit-or-drill` row on the Editing page +read "Edit current cell", while `i`/`a` on an aggregate drill in the TUI and +name drill in the browser. The row is found by its **keys** — the labels +`keys_for(Normal, "edit-or-drill", ["editing"])` returns — not by its caption, +and exactly one row must carry them, so a caption fix that also moved or +duplicated the row fails on the `let [..]` pattern rather than passing on +whichever copy was found first. Failed at HEAD on the caption; the injection +"Edit current cell, or drill" (drill named, aggregate not) fails it too, since +`>` is also drill and the row must say *when* `i` drills. + +### only_the_edit_key_is_captioned_as_editing_the_cell + +Found while fixing improvise-s8h7, not predicted by it. The row below the edit +key read `Enter Edit current cell (same as i)`, but Normal-mode Enter is bound +to `enter-advance` alone — not a `Binding::Sequence` — and `EnterAdvance` only +moves the cursor down, wrapping to the next column. The test takes every help +row captioned "Edit current cell" and requires its keys to be a subset of +`edit-or-drill`'s; at HEAD it failed naming `["Enter"]`. The fix moved the row +to the Navigation page's "Cursor movement" section with a caption that says +what it does. + +Its reach is stated rather than implied: it checks the one phrase that was +false. A caption that misdescribes Enter in *other* words passes it: a caption +is prose, and nothing checks it against the command it documents. That is a +neighbour of improvise-wk18, not the same gap — wk18 is about `literal()` rows +whose *keys* go unchecked; here the keys were right and the words were wrong. ### a_view_row_click_focuses_moves_and_switches_to_the_row_clicked diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index 8143ec92..724a373c 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -439,6 +439,12 @@ fn navigation() -> Vec { Some(HelpLine::Heading("Cursor movement")), Some(HelpLine::Blank), keys(n, "move-selection", None, "Move one cell"), + keys( + n, + "enter-advance", + None, + "Move down, wrapping to the next column", + ), Some(literal("gg", "Jump to first row")), keys(n, "jump-last-row", None, "Jump to last row"), keys(n, "jump-first-col", None, "Jump to first column"), @@ -513,8 +519,12 @@ fn editing() -> Vec { Some(HelpLine::Blank), Some(HelpLine::Heading("Entering edit mode")), Some(HelpLine::Blank), - keys(n, "edit-or-drill", Some(&["editing"]), "Edit current cell"), - keys(n, "enter-advance", None, "Edit current cell (same as i)"), + keys( + n, + "edit-or-drill", + Some(&["editing"]), + "Edit current cell (drill into an aggregate)", + ), keys( e, "clear-buffer", @@ -950,6 +960,58 @@ mod tests { "an undescribed name falls back to itself rather than to silence" ); } + + fn rows() -> Vec<(Vec, &'static str)> { + pages() + .into_iter() + .flat_map(|page| page.lines) + .filter_map(|line| match line { + HelpLine::Keys { keys, description } => Some((keys, description)), + _ => None, + }) + .collect() + } + + fn labels_of(command: &str, args: Option<&[String]>) -> Vec { + keys_for(&ModeKey::Normal, command, args) + .iter() + .map(|(c, m)| key_label(c, *m)) + .collect() + } + + /// improvise-s8h7: `i` on an aggregate drills in the TUI, so the row + /// that documents it must say so. See `context/test-evidence/clients.md`. + #[test] + fn the_edit_key_row_says_it_drills_into_an_aggregate() { + let edit = labels_of("edit-or-drill", Some(&["editing".to_string()])); + let found: Vec<_> = rows().into_iter().filter(|(k, _)| *k == edit).collect(); + let [(_, description)] = &found[..] else { + panic!("expected one help row keyed {edit:?}, got {found:?}"); + }; + assert!( + description.contains("drill") && description.contains("aggregate"), + "{edit:?} drills into an aggregate and the row must say so: {description}" + ); + } + + /// improvise-s8h7: Normal's Enter is `enter-advance`, which only moves. + /// See `context/test-evidence/clients.md`. + #[test] + fn only_the_edit_key_is_captioned_as_editing_the_cell() { + let edit = labels_of("edit-or-drill", Some(&["editing".to_string()])); + let claims: Vec<_> = rows() + .into_iter() + .filter(|(_, d)| d.contains("Edit current cell")) + .collect(); + assert!(!claims.is_empty(), "the check needs a row to check"); + for (keys, description) in claims { + assert!( + keys.iter().all(|k| edit.contains(k)), + "{keys:?} is captioned {description:?} but only {edit:?} edit" + ); + } + } + /// Not an assertion: a printout for a human to read once. `cargo test -p /// improvise-protocol eyeball -- --nocapture --ignored`. #[test] diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0bece83a..be549bf3 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -748,9 +748,10 @@ fn edit_or_drill(state: &ClientState, mode: AppMode) -> Option> { }, Command::ChangeMode(mode), ]), - CellId::Aggregate(_) => Some(vec![Command::SetStatus( - "drilling into an aggregate needs the host — not in the browser yet".to_string(), - )]), + CellId::Aggregate(_) => Some(vec![Command::SetStatus(format!( + "an aggregate is edited by drilling in — {}", + not_yet("drill") + ))]), CellId::Derived { target } => Some(vec![Command::SetStatus(format!( "{target} is computed by a formula — edit the formula, not the cell" ))]), @@ -2435,28 +2436,38 @@ mod tests { ); } - /// The advertised key must say what is actually true. Two refusals, two - /// subjects — see `context/test-evidence/clients.md`. + /// `i` on an aggregate IS drill in the TUI, so both doors name one missing + /// feature; only `i` says why. See `context/test-evidence/clients.md`. #[test] - fn the_drill_key_and_an_aggregate_edit_refuse_for_different_reasons() { - let mut state = in_mode(AppMode::Normal); - state.grid.extent = GridExtent { rows: 2, cols: 2 }; - state.window = (2, 2); - - let KeyOutcome::Local(drill) = resolve(&state, ev(KeyCode::Char('>'))) else { - panic!("`>` is advertised in the hint line; it must answer"); - }; - let [Command::SetStatus(said)] = &drill[..] else { - panic!("expected one status, got {drill:?}"); + fn an_aggregate_edit_names_the_same_missing_drill_as_the_drill_key() { + let aggregate = CellId::Aggregate( + serde_json::from_str("\"aa4c8e5e9cbd578e0d388f4b2b5a7879\"").unwrap(), + ); + let state = client_on(aggregate); + let said = |key| match resolve(&state, ev(key)) { + KeyOutcome::Local(commands) => match &commands[..] { + [Command::SetStatus(said)] => said.clone(), + other => panic!("expected one status, got {other:?}"), + }, + other => panic!("{key:?} is advertised; it must answer, got {other:?}"), }; + let (drill, edit) = (said(KeyCode::Char('>')), said(KeyCode::Char('i'))); assert!( - said.contains("drill") && said.contains("browser"), - "the status must name drill and say it is the browser that lacks it: {said}" + drill.contains("drill") && drill.contains("browser"), + "the status must name drill and say it is the browser that lacks it: {drill}" ); assert!( - !said.contains("aggregate"), - "a sentence about aggregates is TRUE-SOUNDING about the wrong thing — \ - `>` is refused whatever the cell is: {said}" + !drill.contains("aggregate"), + "`>` is refused whatever the cell is, so it must not blame the cell: {drill}" + ); + assert!( + edit.contains(&drill), + "`i` reached the same missing feature and must name it the same way: \ + {edit:?} does not contain {drill:?}" + ); + assert!( + edit.contains("aggregate"), + "and `i` must still say why it drilled rather than edited: {edit}" ); } diff --git a/harness/tests/drill.spec.js b/harness/tests/drill.spec.js index d38124c3..c1c12039 100644 --- a/harness/tests/drill.spec.js +++ b/harness/tests/drill.spec.js @@ -36,12 +36,14 @@ async function anAggregateGrid(page) { test.describe("drill is refused in the browser, not broken", () => { // The honest half: `i`/`a` on an aggregate declines OUT LOUD, because // `edit_or_drill` answers the `CellId::Aggregate` arm with a status. - test("editing an aggregate says drill needs the host", async ({ page }) => { + test("editing an aggregate says it is edited by drilling, and drill is missing", async ({ + page, + }) => { await anAggregateGrid(page); await page.keyboard.press("i"); await expect(page.locator("#toast")).toContainText( - "drilling into an aggregate needs the host", + "an aggregate is edited by drilling in — drill is not available in the browser yet", ); // And it really is a refusal: no editor opened, no mode change. await expect(page.locator("#mode")).toHaveText("NORMAL"); @@ -63,16 +65,15 @@ test.describe("drill is refused in the browser, not broken", () => { ); await expect(page.locator("#mode")).toHaveText("NORMAL"); - // The two refusals have two SUBJECTS, and conflating them would be a - // true-sounding sentence about the wrong thing — worse than silence, - // because it stops the reader looking. `>` must not talk about aggregates. + // `>` is refused whatever the cell is, so it must not blame the cell. await expect(page.locator("#toast")).not.toContainText("aggregate"); - // And `i` still gives the OTHER refusal, which is what proves the two are - // distinct rather than one message reached by two keys. + // `i` reaches the SAME missing feature (in the TUI it drills), so it names + // it the same way and adds only why — improvise-s8h7, clients.md. await page.keyboard.press("i"); + await expect(page.locator("#toast")).toContainText("an aggregate is edited by drilling in"); await expect(page.locator("#toast")).toContainText( - "drilling into an aggregate needs the host", + "drill is not available in the browser yet", ); }); }); From 36063da3c98643908196668bf30ad209be02d679 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:53:45 -0700 Subject: [PATCH 4/9] WIP improvise-2bac: tests for explicit args and gate/run agreement Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../src/command/cmd/registry.rs | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index d9f1d47a..e8f8e04d 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -573,6 +573,9 @@ mod tests { vec!["1", "1"], vec!["formula", "1"], vec!["editing", "5", "Region", "East"], + vec!["editing", "5", "Region/East"], + vec!["formula", "false", "false"], + vec!["Region/East"], ] .into_iter() .map(|v| v.into_iter().map(String::from).collect()) @@ -734,6 +737,62 @@ mod tests { "only {checked} ex-command examples were found; the scan has stopped seeing them" ); } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor() { + let mut app = crate::ui::app::App::new(two_cat_model(), None); + app.view_state + .buffers + .insert("edit".into(), "decoy".into()); + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + let target = CellKey::new(vec![ + ("Month".into(), "Feb".into()), + ("Type".into(), "Clothing".into()), + ]); + assert_ne!(ctx.cell_key().as_ref(), Some(&target), "the cursor is elsewhere"); + let args: Vec = ["editing", "5", "Type/Clothing", "Month/Feb"] + .map(String::from) + .into(); + let dbg = format!( + "{:?}", + ctx.registry + .interactive("commit-cell-edit", &args, &ctx) + .expect("commit-cell-edit resolves") + ); + assert!(dbg.contains(&format!("key: {target:?}")), "wrong cell: {dbg}"); + assert!(dbg.contains("value: \"5\""), "wrong value: {dbg}"); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn every_argument_list_the_gate_accepts_the_run_accepts_too() { + let r = default_registry(); + let mut wb = crate::workbook::Workbook::new("Empty"); + wb.add_category("Foo").expect("a category with no items"); + let app = crate::ui::app::App::new(wb, None); + let ctx = app.cmd_context( + crate::command::key::KeyCode::Null, + crate::command::key::KeyModifiers::NONE, + ); + assert!(ctx.cell_key().is_none(), "the cursor must sit on no cell"); + let mut accepted = 0; + for name in r.names().collect::>() { + for args in candidate_args() { + if r.parse(name, &args).is_err() { + continue; + } + accepted += 1; + if let Err(e) = r.interactive(name, &args, &ctx) { + panic!("the gate accepts `{name} {args:?}` but the run refuses it: {e}"); + } + } + } + assert!(accepted > 50, "only {accepted} accepted pairs were tried"); + } } #[cfg(test)] @@ -839,4 +898,29 @@ mod panel_open_tests { ); } } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn explicit_open_and_focus_arguments_beat_the_toggle() { + for panel in Panel::ALL { + let mut app = App::new(two_cat_model(), None); + app.view_state.formula_panel_open = false; + app.view_state.category_panel_open = false; + app.view_state.view_panel_open = false; + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + let args = [panel.name().to_string(), "false".into(), "false".into()]; + let dbg = format!( + "{:?}", + ctx.registry + .interactive("toggle-panel-and-focus", &args, &ctx) + .expect("toggle-panel-and-focus resolves") + .execute(&ctx) + ); + assert!( + dbg.contains("open: false") && !dbg.contains("open: true"), + "`{} false false` on a closed panel toggled it open: {dbg}", + panel.name() + ); + } + } } From bdf9bff47244f8844e652b03ae906eaa400a3263 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:53:51 -0700 Subject: [PATCH 5/9] The editor overlay and Enter read one buffer (improvise-ktlk) PR #162 (improvise-xg1z) made render.rs paint edit_seed when Editing arrived without its edit buffer, but commit and append-char still read the absent buffer as the empty string. So the overlay showed the value, Enter composed ClearCell, and a typed character replaced the text instead of extending it: two derivations of what is in the editor. The fold now seeds the missing buffer (ClientState::settle_editor, next to settle_mode), and render.rs reads the buffer through read_buffer, as commit does. edit_seed goes back to pub(crate) and no renderer calls it. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/clients.md | 58 ++++++++++++++++++---- crates/improvise-protocol/src/client.rs | 18 +++++++ crates/improvise-protocol/src/lib.rs | 2 +- crates/improvise-protocol/src/resolve.rs | 58 ++++++++++++++++++++-- crates/improvise-wasm-client/src/render.rs | 26 +++++----- 5 files changed, 132 insertions(+), 30 deletions(-) diff --git a/context/test-evidence/clients.md b/context/test-evidence/clients.md index fd39ad65..db4787d9 100644 --- a/context/test-evidence/clients.md +++ b/context/test-evidence/clients.md @@ -4041,17 +4041,18 @@ Demonstrated at `e7de19d`: the first test below reports `Some("")`. ### `an_editing_mode_with_no_edit_buffer_shows_the_cell_rather_than_a_blank` -The fallback is `improvise_protocol::edit_seed`, which is the **existing** home -of "what an editor opened here starts with" — `seed_at` at the cursor, the -function `enter-advance` already seeds from. A second reconciliation of the -scroll offset inside `render.rs` would have been gotcha 42's own trap, so the -one-line fallback cost one `pub` rather than a copy. +**Superseded in part by improvise-ktlk** (entry at the end of this file). As +first landed, the fallback lived in `render.rs`: `editing` fell back to +`edit_seed` while `commit` and `append-char` still read the absent buffer as +`""`, so the overlay showed `1200` and Enter cleared it. The fallback now lives +in the fold (`ClientState::settle_editor`), which seeds the buffer itself, and +`render.rs` reads the same buffer `commit` does. The test's first assertion +changed with it: it used to check that the buffer was still absent after the +fold, and now checks that the fold seeded it. -The test asserts three things in order, and the middle one is why it is not -vacuous: that the buffer really is absent, that the cell really holds `1200`, -and only then what `editing` says. Without the first two a fixture that happened -to carry an `edit` buffer, or a grid whose cell was blank anyway, would pass -against the bug. +The test asserts three things in order: what the fold put in the buffer, that +the cell really holds `1200`, and only then what `editing` says. Without the +second, a grid whose cell was blank anyway would pass against the bug. **Fault injected:** `unwrap_or_default()`, the original expression. Fails with `Some("")`. @@ -4177,3 +4178,40 @@ and converged. **Fault injected:** the same `Binding::Prefix` → `Upstream` reversion. Fails on the second press. + +## improvise-ktlk — the overlay and the commit read one buffer + +PR #162 (improvise-xg1z) changed what the overlay **draws** for an `Editing` +mode that arrived without its `edit` buffer, and not what Enter **commits**. +`render.rs` painted `edit_seed(state)`, while `resolve.rs::commit` and +`append-char` read `read_buffer(&state.view, "edit")`, which is `""` for an +absent key. So the overlay showed the value, Enter composed `ClearCell`, and one +typed character replaced the shown text rather than extending it. #162's tests +only looked at the drawing, and none of them pressed a key. + +The fix gives the fact one home. The fold seeds the missing buffer +(`ClientState::settle_editor`, called after every fold, bootstrap and refusal, +beside `settle_mode`), and `render.rs` reads the buffer through `read_buffer`, +as `commit` does. `edit_seed` is `pub(crate)` again and no renderer calls it. +Seeding happens only when there is a cell under the cursor that has +`edit_text`. With no such cell the buffer stays absent, which reads as `""` +on both paths, and `commit` declines anyway for want of a cell. + +### `an_editor_entered_without_its_buffer_commits_and_extends_the_cell` + +The fixture's display text (`1,200.50`) and edit text (`1200.5`) **differ**. +With `CellDisplay::number("1200")`, as #162 used, a fallback that seeds from the +formatted `text` passes too (gotcha 8). The test folds `ChangeMode(editing)` +with no `SetBuffer`, then presses the keys: Enter must compose +`SetCell(Number(1200.5))`, and after a typed `5`, Enter must compose +`SetCell(Number(1200.55))`. + +**Faults injected, each run:** +- The tree before this fix (render-only fallback). The buffer assertion fails + with `""`. With that assertion removed, the Enter assertion fails on its own + with `None`, because the first command is `ClearCell`. +- `edit_seed` returning the drawn `text` instead of `edit_text`. Fails with + `"1,200.50"`. +- The over-broad seed, "absent **or empty**, refill". This test passes, and + `an_edit_buffer_the_user_has_emptied_is_not_refilled_from_the_cell` fails, + as it did against the same rival in `render.rs`. diff --git a/crates/improvise-protocol/src/client.rs b/crates/improvise-protocol/src/client.rs index 8010e920..599f21e4 100644 --- a/crates/improvise-protocol/src/client.rs +++ b/crates/improvise-protocol/src/client.rs @@ -74,6 +74,7 @@ impl ClientState { self.apply_projection(projection); } self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -85,6 +86,7 @@ impl ClientState { other => reduce_view(&mut self.view, other), } self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -124,6 +126,7 @@ impl ClientState { } self.last_rejection = Some(rejection); self.settle_mode(); + self.settle_editor(); self.revision += 1; } @@ -158,6 +161,21 @@ impl ClientState { self.view.mode = settled_mode(&self.view.mode, self.grid.records); } + /// An editor that arrived without its buffer is seeded here, so what is + /// drawn and what Enter commits read one buffer (improvise-ktlk). + fn settle_editor(&mut self) { + let absent = self + .view + .mode + .minibuffer() + .filter(|_| self.view.mode.is_editing()) + .map(|mb| mb.buffer_key.clone()) + .filter(|key| !self.view.buffers.contains_key(key)); + if let (Some(key), Some(seed)) = (absent, crate::resolve::edit_seed(self)) { + self.view.buffers.insert(key, seed); + } + } + /// The widget dispatch: a projection names the widget it is addressed to. /// The `CategoryTreeCache`/`TileBarCache` overlap is deliberate (gotcha 32). fn apply_projection(&mut self, projection: &Projection) { diff --git a/crates/improvise-protocol/src/lib.rs b/crates/improvise-protocol/src/lib.rs index 53544924..8abc5ec4 100644 --- a/crates/improvise-protocol/src/lib.rs +++ b/crates/improvise-protocol/src/lib.rs @@ -34,7 +34,7 @@ pub use render::{ WizardProposal, WizardStepKind, header_level_widths, }; pub use resolve::{ - Click, KeyOutcome, Request, click, edit_seed, read_buffer, resolve, typed, typed_cell_command, + Click, KeyOutcome, Request, click, read_buffer, resolve, typed, typed_cell_command, }; pub use viewmodel::{CatTreeEntry, flatten_cat_tree, view_axis_summary}; // `improvise-core`'s, because the index that resolves one lives in diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 0bece83a..b7647270 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -954,11 +954,10 @@ fn record_at_cursor(state: &ClientState) -> Option { } } -/// What an editor opened on the cursor starts with. The one home, so a -/// renderer does not reconcile the scroll offset again (gotcha 42). -pub fn edit_seed(state: &ClientState) -> String { - let (row, col) = state.view.cursor().selected; - seed_at(state, row, col) +/// What an editor opened on the cursor starts with, or `None` with no cell +/// there to seed from. Read by the fold, never a renderer (improvise-ktlk). +pub(crate) fn edit_seed(state: &ClientState) -> Option { + cell_at_cursor(state)?.edit_text.clone() } /// The display text at an absolute grid position, as the cache holds it. @@ -3017,6 +3016,55 @@ mod tests { )); } + /// improvise-ktlk: what an editor shows and what Enter commits are one + /// fact. See `context/test-evidence/clients.md`. + #[test] + fn an_editor_entered_without_its_buffer_commits_and_extends_the_cell() { + let mut client = client_on(record_cell()); + let mut cell = CellDisplay::number("1,200.50"); + cell.edit_text = Some("1200.5".into()); + cell.id = Some(record_cell()); + client.fold(&Command::Project(Projection::Grid(GridUpdate::Cells( + vec![((0, 0), cell)], + )))); + client.fold(&Command::ChangeMode(AppMode::editing())); + assert_eq!( + read_buffer(&client.view, "edit"), + "1200.5", + "the buffer is what the overlay paints and what a commit reads" + ); + + let committed = |client: &ClientState| { + let KeyOutcome::Local(commands) = resolve(client, ev(KeyCode::Enter)) else { + panic!("Enter in Editing must resolve here"); + }; + match &commands[0] { + Command::Model { + cmd: ModelCommand::SetCell { value, .. }, + .. + } => Some(value.clone()), + _ => None, + } + }; + assert_eq!( + committed(&client), + Some(CellValue::Number(1200.5)), + "Enter on an editor showing the cell must not clear it" + ); + + let KeyOutcome::Local(typed) = resolve(&client, ev(KeyCode::Char('5'))) else { + panic!("a character in Editing is append-char"); + }; + for command in &typed { + client.fold(command); + } + assert_eq!( + committed(&client), + Some(CellValue::Number(1200.55)), + "a typed character extends what the editor showed, not replaces it" + ); + } + /// At the bottom-right corner `EnterAdvance` returns `AbortChain`, so the /// commit leaves the user in normal mode. See the evidence file. #[test] diff --git a/crates/improvise-wasm-client/src/render.rs b/crates/improvise-wasm-client/src/render.rs index b73785d0..8025d36b 100644 --- a/crates/improvise-wasm-client/src/render.rs +++ b/crates/improvise-wasm-client/src/render.rs @@ -294,16 +294,13 @@ pub fn render_model(state: &ClientState) -> RenderModel { help: help_rows(&state.view), help_page: state.view.help_page, help_titles: help_titles(&state.view), - // An ABSENT buffer is not an empty one: blanking the overlay over a - // real value is one Enter from clearing it (improvise-xg1z). - editing: state.view.mode.is_editing().then(|| { - state - .view - .buffers - .get("edit") - .cloned() - .unwrap_or_else(|| improvise_protocol::edit_seed(state)) - }), + // The buffer Enter commits, never a second derivation of it: the fold + // seeds an absent one (improvise-xg1z, improvise-ktlk). + editing: state + .view + .mode + .is_editing() + .then(|| improvise_protocol::read_buffer(&state.view, "edit")), minibuffer: state.view.mode.minibuffer().map(|mb| RenderMinibuffer { prompt: mb.prompt.clone(), value: improvise_protocol::read_buffer(&state.view, &mb.buffer_key), @@ -912,10 +909,11 @@ mod tests { client .state_mut() .fold(&Command::ChangeMode(improvise_protocol::AppMode::editing())); - assert!( - !client.state().view.buffers.contains_key("edit"), - "the fixture must really be missing the buffer, or this proves \ - nothing about the fallback" + assert_eq!( + client.state().view.buffers.get("edit").map(String::as_str), + Some("1200"), + "the fold seeds the missing buffer, so the overlay and a commit \ + read one thing (improvise-ktlk)" ); assert_eq!( client.model().rows[0].cells()[0].text, From 808cbb7d92e9920ccba616612faed0e111596633 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:55:25 -0700 Subject: [PATCH 6/9] fmt-workspaces: refuse a package no workspace root reaches (improvise-90ea) The fixture loop covers every package the fixture has; this covers every package the tree has. The review's skip-the-root rival now fails the real repo's own gate (exit 2 naming Cargo.toml), not only the test suite. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 63 ++++++++++++++++++++++++++++++++ scripts/fmt-workspaces-test.sh | 26 +++++++++++++ scripts/fmt-workspaces.sh | 59 +++++++++++++++++++++--------- 3 files changed, 131 insertions(+), 17 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index 5d2b0a1e..bda78aae 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -391,6 +391,8 @@ green suite over a vacuous assertion. **What is not covered.** The script asks `cargo metadata` for members, so a root whose metadata succeeds but whose member list is wrong is not detectable here. +*Narrowed by improvise-90ea, below:* a list that **omits** a package in the tree +is now refused; one that names a package it should not is still not detected. And `harness/opfs-contract`'s own `cargo fmt -p opfs-contract` step in the `harness-lint` job is left as it is: it resolves correctly because that step sets `working-directory`, and the ticket's own note confirms CI as written was @@ -438,3 +440,64 @@ same way: the suite passed *under a rival*. The generalisable rule is that a stub must fail in the **shape** of the real fault, not merely fail — a stub that breaks more than the real thing breaks satisfies the test through the wrong arm, and reads exactly like coverage. + +#### The root package, and why the fourth fix is structural — `scripts/fmt-workspaces-test.sh` + +A post-merge review of PR #159 found a **fourth** fixture vacuity +(improvise-90ea). The fixture's outer root is a package (`root-pkg`) as well as +a workspace, which is right, but no case ever dirtied **the root package's own +`src/lib.rs`**. Only the member `a/` and `nested/` were dirtied. So a script +that formats every member *except* the root package passed 17/17. In this repo +the root package is `improvise`, the whole TUI. Reproduced at `40b8913` with +the review's rival in `members_of` (drop the package whose manifest sits at +`workspace_root`): the suite passed 17/17 and `--list` left `improvise` out. + +Adding one more hand-picked case would have fixed the fourth and not the fifth. +Four vacuities in one checker all have the same cause: **the cases were chosen +by hand, one per hazard somebody had thought of.** So the fix is structural, +and it has two halves because it has to cover two sets of packages: + +- **Every package the fixture has.** The two hand-picked fault cases are + replaced by a loop over every `[package]` manifest in the fixture. Each one + gets its own misformatted `src/lib.rs`, must exit 1, and must be named + **exactly once**, so the `--all` boundary check now covers every package + rather than one. The enumeration is a `grep` for `[package]`, deliberately + *not* the script's `[workspace]` + `cargo metadata` discovery, since that + discovery is what is under test. A count assertion (three packages) stops + the loop going vacuous if the fixture loses a package. A package added to + the fixture gets its case automatically. +- **Every package the tree has.** The loop proves only that the script handles + the fixture's *shapes*. A real tree can hold a package no root lists (an + `exclude`d crate, say), and nothing in the fixture would say so. So the gate + itself now checks an invariant **before it formats anything**: every + `[package]` manifest the find reaches must be a member of some root, or it + exits **2** and names the package. That is exit 2 and not 1 because no + verdict was produced for that package, which is what exit 2 means here. + Paths are compared through `realpath`, because on macOS `mktemp` lives under + `/var` while cargo reports `/private/var`. + +**Rivals injected, and where each failed** (the suite went from 17 to 21 cases): + +- **The review's skip-the-root rival.** It now fails the fixture at `a fault in + src/lib.rs is reported` (exit 0). With the invariant in place it fails + **six** cases (exit 2, "no workspace root reaches"). It also fails the + **real repo** with no fixture involved: `fmt-workspaces.sh` exits 2 naming + `Cargo.toml`. That last one is the property worth having. The gate refuses + its own blindness in CI, not only in its test suite. +- **Disabling the invariant** (`if false`). It fails `a package no workspace + root reaches is exit 2, not a pass` with **exit 0**, even though that + package's `src/lib.rs` is misformatted. That exit 0 *is* the hazard: an + unreached package is silently clean. +- **The two earlier rivals, re-run against the loop.** `--all` for `-p` fails + both `a/src/lib.rs` and `src/lib.rs` "named exactly once". The second of + those was not predicted: the nested root's `--all` also reaches the root + package. The bare manifest-path form fails `a fault in a/src/lib.rs is + reported`. + +The real-repo control from the ticket still holds: a misformatted fn appended +to `src/ui/text.rs` is reported (`Diff in …/src/ui/text.rs:72`). + +**What is still not covered.** A root whose member list *includes* something it +should not is not detected, and a package in a directory the find excludes +(`target/`, `.claude/`, `node_modules/`) is outside the invariant by +construction. diff --git a/scripts/fmt-workspaces-test.sh b/scripts/fmt-workspaces-test.sh index 8ad50dd0..ce15d6ce 100755 --- a/scripts/fmt-workspaces-test.sh +++ b/scripts/fmt-workspaces-test.sh @@ -122,6 +122,32 @@ run_case "a tree with no workspace root is a usage error, not a pass" 2 "$work/e run_case "--help is a usage error, so a typo cannot pass" 2 "$work/repo" --help +# The loop above covers every package the FIXTURE has; this covers every +# package the TREE has. A package no root lists is checked by nothing, so the +# gate must refuse a verdict -- and name the package -- rather than pass. +scaffold +mkdir -p "$work/repo/orphan/src" +printf '[package]\nname = "orphan-pkg"\nversion = "0.1.0"\nedition = "2021"\n' \ + > "$work/repo/orphan/Cargo.toml" +printf '%s' "$DIRTY" > "$work/repo/orphan/src/lib.rs" +cat > "$work/repo/Cargo.toml" <<'EOF' +[workspace] +members = ["a"] +exclude = ["orphan"] +resolver = "2" + +[package] +name = "root-pkg" +version = "0.1.0" +edition = "2021" +EOF +if run_case "a package no workspace root reaches is exit 2, not a pass" 2 "$work/repo"; then + case "$OUT" in + *"no workspace root reaches"*orphan/Cargo.toml*) ok " and the unreached package is named" ;; + *) bad " and the unreached package is named" "output: ${OUT:0:300}" ;; + esac +fi + # The narrowed contract, pinned so it is not merely prose (ChatGPT Codex on # PR #159). rustfmt exits 1 for a source it cannot parse, exactly as for a # diff -- so exit 1 is "rustfmt did not certify this root clean", and what diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh index abb6fec9..982d2211 100755 --- a/scripts/fmt-workspaces.sh +++ b/scripts/fmt-workspaces.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Format-checks every Cargo workspace root in the repo, from any directory. -# `cargo fmt --all` reaches only ONE of them. Reasoning: context/code/tooling.md -# section 4 -- including why `-p` scoped by `--manifest-path` is the only +# `cargo fmt --all` reaches only ONE of them. Reasoning: context/test-evidence/ +# tooling.md, "The two-workspace fmt gate" -- including why `-p` scoped by `--manifest-path` is the only # spelling that is neither blind nor boundary-crossing (improvise-b4ks). set -uo pipefail @@ -18,8 +18,8 @@ exit 1 rustfmt did not certify a root clean: a file differs from rustfmt's output, or rustfmt refused to parse one. Either way a FILE IS NAMED and the tree is what is wrong. exit 2 the gate did not run: usage error, no cargo, no rustfmt, cargo - metadata failed, or no [workspace] root found. NO verdict was - produced and NO file is named. + metadata failed, no [workspace] root found, or a [package] that no + root has as a member. NO verdict was produced and NO file is named. The split is "did the gate run", not "is every exit-1 cause a diff". That is narrower than it sounds and it is deliberate -- see the note on rc=1 below. @@ -38,8 +38,8 @@ EOF # The find is RELATIVE to the tree being scanned: an absolute `-path` filter # matches the ancestors too, and this repo is itself checked out under # `.claude/worktrees/` in every agent worktree, where that excluded everything. -workspace_roots() { - local root="$1" +manifests_with() { + local root="$1" section="$2" ( cd "$root" && find . -name Cargo.toml \ -not -path './target/*' -not -path '*/target/*' \ -not -path './.git/*' \ @@ -47,11 +47,12 @@ workspace_roots() { -not -path './.claude/*' ) \ | sort \ | while IFS= read -r rel; do - if grep -q '^\[workspace\]' "$root/${rel#./}"; then + if grep -q "^\\[$section\\]" "$root/${rel#./}"; then printf '%s\n' "$root/${rel#./}" fi done } +workspace_roots() { manifests_with "$1" workspace; } # The members of ONE workspace, named by cargo rather than parsed out of the # manifest: a `members = [...]` list may carry globs and path dependencies. @@ -61,10 +62,10 @@ members_of() { printf '%s' "$meta" | python3 -c ' import json, sys doc = json.load(sys.stdin) -names = [p["name"] for p in doc["packages"]] -if not names: +rows = [p["name"] + "\t" + p["manifest_path"] for p in doc["packages"]] +if not rows: sys.exit(1) -print("\n".join(names)) +print("\n".join(rows)) ' || return 1 } @@ -106,15 +107,39 @@ main() { return 2 fi - local rc=0 manifest member - for manifest in "${roots[@]}"; do - local -a pkgs=() - while IFS= read -r member; do [ -n "$member" ] && pkgs+=("$member"); done \ - < <(members_of "$manifest") - if [ ${#pkgs[@]} -eq 0 ]; then - printf 'fmt-workspaces: cargo metadata failed for %s\n' "$manifest" >&2 + # Pass 1: every root's members, before anything is formatted, so that a + # package NO root reaches is exit 2 rather than silently never checked. + local rc=0 manifest member i m + local -a members=() covered=() + for i in "${!roots[@]}"; do + local rows + rows="$(members_of "${roots[$i]}")" || rows="" + if [ -z "$rows" ]; then + printf 'fmt-workspaces: cargo metadata failed for %s\n' "${roots[$i]}" >&2 return 2 fi + members[$i]="$(printf '%s\n' "$rows" | cut -f1 | tr '\n' ' ')" + while IFS= read -r m; do covered+=("$m"); done < <(printf '%s\n' "$rows" | cut -f2) + done + local orphans + orphans="$(manifests_with "$root" package | python3 -c ' +import os, sys +covered = {os.path.realpath(p) for p in sys.argv[1:]} +for line in sys.stdin: + m = line.rstrip("\n") + if m and os.path.realpath(m) not in covered: + print(m) +' "${covered[@]}")" + if [ -n "$orphans" ]; then + printf 'fmt-workspaces: no workspace root reaches these packages, so nothing checks them:\n%s\n' \ + "$orphans" >&2 + return 2 + fi + + for i in "${!roots[@]}"; do + manifest="${roots[$i]}" + local -a pkgs=() + read -r -a pkgs <<< "${members[$i]}" if [ "$list" = 1 ]; then printf '%s: %s\n' "$manifest" "${pkgs[*]}" From d9b10905d0eaad361391af7d89f3b501b18dae07 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 20:58:34 -0700 Subject: [PATCH 7/9] improvise-2bac: context docs, evidence, fmt Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/code/cli.md | 68 +++++++++++++++ context/code/command.md | 13 +-- context/test-evidence/cli.md | 86 ++++++++++++++++++- .../src/command/cmd/registry.rs | 39 +++++++-- tests/headless_cli.rs | 8 +- 5 files changed, 195 insertions(+), 19 deletions(-) diff --git a/context/code/cli.md b/context/code/cli.md index e1c20ca3..d4cc9a68 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -151,7 +151,75 @@ atomicity is preserved by keeping the existing whole-line `parse_line` as a still parses, and a line that did not still does not, so the only behavioural change is which cursor the commands see. +**That atomicity claim was false as written** (improvise-2bac), and §8 is what +makes it true. The gate runs the pure arm; the loop runs the interactive arm. A +gate is only a gate if *every argument list the pure arm accepts, the +interactive arm accepts too* — and `commit-cell-edit` broke it: +`add-category Foo . commit-cell-edit editing 5 Foo/x` on an empty document +passed the gate, applied the first segment, and then refused the second with +`no cell at cursor`. Half the line landed. + The TUI's `:` line had the same defect and is fixed the same way, with one limitation that is not new: `ExecuteCommand` returns one effect batch, so the segments of `:a . b` all see the pre-batch cursor, exactly as a `Binding::Sequence` does (gotchas 21 and 23). + +## 8 — the interactive arm is `parse`, then context; it is never a second parser + +#151 fixed three instances of *a registry with two construction arms demotes +whichever caller reaches the weaker one*, one patch each. A post-merge review +found the fourth (improvise-2bac): `commit-cell-edit `, +whose interactive arm read only ``, took the value from the `edit` buffer +and the key from the cursor. So a script that wrote 5 to `Region/East` +**deleted the cell under the cursor** — the empty buffer, committed — with exit +0 and nothing on stderr. The fifth was already in the tree: +`toggle-panel-and-focus false false` ignored both flags and toggled. + +The shape rather than the instances is what changed. `CmdRegistry::register` +used to take two free functions, `parse(args)` and `interactive(args, ctx)`, +and nothing related them; each command re-derived its arguments twice and every +disagreement was silent. It now takes three pieces, and the interactive arm is +**built from `parse`**, not beside it: + +- `parse(args) -> C` — the only function that reads arguments a caller supplied. +- `contextualize(C, &CmdContext) -> C` — fills the fields that are context and + never argument (a `Move`'s origin cursor, a `MovePanelCursor`'s bound). It is + handed the built command and **not the args**, so it cannot discard them. +- `fallback: Option<(args, ctx) -> C>` — reached **only** when `parse` refused. + This is where "no coordinates means the cursor" lives: `clear-cell` bare, and + `commit-cell-edit ` bare, which is what the keymap binds. + +Two consequences hold by construction rather than by test. **Whatever `parse` +accepts, the interactive arm accepts** — `contextualize` is infallible — so the +§7 gate cannot pass a segment the loop then refuses. And **an accepted argument +list cannot be overridden by the cursor**, because the only function that sees +both args and context never runs on an accepted list. `register_pure` and +`register_nullary` already had one arm; they are the `contextualize = identity, +fallback = None` case. + +What is still representable, stated rather than hidden: + +- `contextualize` can overwrite a field `parse` set. It would have to name the + field, which is a visible line in review, where the old `|_args, ctx|` was an + invisible omission. `commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor` + fails if `commit-cell-edit`'s does. +- A `fallback` sees the args `parse` refused and could ignore some of them. + `commit-cell-edit`'s refuses anything but a lone mode, so the malformed + `commit-cell-edit editing 5` is an error on the `:` line — which has no pure + gate — rather than a commit of the edit buffer to the cursor cell. +- The gate is **stricter** than the loop wherever a fallback exists: a bare + `clear-cell` is refused in a script. That is the safe direction — the whole + line is refused — and it is unchanged from before #151. + +`toggle-panel-and-focus` did not fit `contextualize`, because its pure arm +defaulted the omitted flags to `true` and a contextualizer cannot tell an +omitted flag from an explicit one. So the omission is now in the type — +`target: Option<(bool, bool)>`, `None` meaning *toggle* — and the toggle is +resolved in `execute` against the same context. That is the honest home: the +decision depends on context, and `execute` is where context is. + +Guards: `every_argument_list_the_gate_accepts_the_run_accepts_too` enumerates +**every registration** against every candidate argument list, in a context whose +cursor sits on no cell — the context in which the old `commit-cell-edit` arm +refused. It lists no commands, so a new registration is covered the day it +lands. It is backed by the process-level tests in `tests/headless_cli.rs`. diff --git a/context/code/command.md b/context/code/command.md index 02de2469..987df348 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -2489,12 +2489,13 @@ line separated by `.`, coordinate pairs written `Category/Item`, and quoted stri available for the rare command that needs to enumerate all views, such as the view panel. -`InteractiveFn` constructs a `Cmd` from the interactive context (keymap dispatch). -It receives both the keymap args and the interactive context, so commands can -combine text arguments (a panel name) with runtime state (whether that panel is -currently open). `register_pure` registers a command that does not need interactive -context: called interactively with args it delegates to `parse`, and without args it -errors. +The interactive arm (keymap dispatch, scripts, the `:` line) is built **from** +`parse`: `register` takes `parse`, a `contextualize` that fills context-only +fields of the command `parse` built without seeing the args, and an optional +`fallback` reached only for argument lists `parse` refused. So an accepted +argument list is never overridden by context (improvise-2bac; the argument is +`context/code/cli.md` §8). `register_pure` is the case with nothing to +contextualize and no fallback: both arms are `parse`. `write_buffer` is the effect that writes a named buffer — the mirror of `read_buffer`, and the only place that knows "search" is not an entry in the buffers diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index bf5f7818..4c89ddee 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -229,12 +229,94 @@ the syntax gate and then cleared whatever the cursor was sitting on — the orig So improvise-6uva's own lesson had a **third** instance in its own fix: two construction arms that mean different things demote whichever caller reaches the weaker one, and here the "weaker" one was the arm that had the context. The four -arms now go through `key_from_args_or_cursor`: explicit coordinates win, the +arms went through `key_from_args_or_cursor`: explicit coordinates win, the cursor is the fallback, which is also what the keymap path wants since a binding -carries no arguments. +carries no arguments. That helper is gone since improvise-2bac: the cursor is +now the registry's `fallback`, reached only when `parse` refused, so explicit +coordinates win by construction (`context/code/cli.md` §8). The fixture writes three cells and clears the one the cursor is **not** on, so the assertion separates "cleared the named cell" from "cleared the cursor cell" — clearing `North` by name would have passed against the broken code. Restoring the arg-discarding arm fails it: `the cursor cell was cleared instead of the named one: ["Region=East = 3", "Region=South = 2"]`. + +### commit_cell_edit_writes_the_named_cell_and_spares_the_cursors + +improvise-2bac, found by the post-merge adversarial review of PR #151: the +fourth instance of that PR's own lesson. `commit-cell-edit` was registered with +two free arms. The pure arm read ` `; the interactive arm +read only ``, took the value from the `edit` buffer and the key from the +cursor. Once #151 made scripts construct through the interactive arm, +`commit-cell-edit editing 5 Region/East` passed the syntax gate and then +committed an **empty** buffer to the cursor cell — deleting it — with exit 0 +and nothing on stderr. + +Reproduced at `40b8913` before the fix, through the real binary: the document +came back as `["Region=East = 3", "Region=North = 1"]` — `South`, the cell the +cursor had been moved onto, was gone and `East` was unchanged. + +The fixture writes three cells and **moves the cursor to `South`** in the same +line before committing to `East`, so the cursor sits on neither the target nor +the origin: a test with the cursor on the target is right by accident, and one +that only asserts the target misses the destruction. It asserts all three. + +Injected rivals, run: (a) `contextualize` for `CommitAndAdvance` overwriting +`key` and `value` from the cursor and the buffer — fails this test and +`a_line_whose_gate_passes_runs_whole`; (b) the registry dispatching to the +fallback first whenever one exists (the pre-fix shape) — fails this test, +`a_line_whose_gate_passes_runs_whole`, `an_explicit_coordinate_still_beats_the_cursor` +and both registry unit tests below. + +### a_line_whose_gate_passes_runs_whole + +improvise-2bac's second symptom, reported on the ticket: `context/code/cli.md` +§7 said the whole-line `parse_line` gate preserved line atomicity. It did not, +wherever the interactive arm refused an argument list the pure arm accepted. +`add-category Foo . commit-cell-edit editing 5 Foo/x` on an empty document +printed `Parse error: commit-cell-edit: no cell at cursor` after `Foo` had +already been added and saved: half the line landed. + +The assertion is on `Parse error` and on the committed cell, **not** on the exit +code. The line exits 1 both before #151 and after this fix, for a different and +pre-existing reason: the grid has no rows, so `commit-cell-edit`'s advance +emits `AbortChain`, and the CLI maps an aborted batch to exit 1 +(`context/code/cli.md` §3). Asserting success would have made this test fail on +the correct behaviour. + +### a_malformed_commit_line_is_refused_whole + +The other half of atomicity, which must survive the fix: a segment the gate +refuses (`commit-cell-edit editing 5` — a value and no coordinates) refuses the +whole line, so the preceding `add-category Foo` never lands. It passed before +the fix and must keep passing; its job is to catch a "fix" that loosens the +pure arm to make the gate agree. + +### commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor + +The unit-level form of the headless test, against `CmdRegistry::interactive` +directly: cursor on `Type=Food, Month=Jan`, a decoy `edit` buffer, and an +explicit `5` for `Type/Clothing Month/Feb`. It asserts the built command's key +and value. Rival (a) above — a `contextualize` that overwrites the parsed +fields — fails it; this is the one hole the new registry shape leaves +representable, so it is the one that needs a test. + +### every_argument_list_the_gate_accepts_the_run_accepts_too + +Enumerates **every** registration (`CmdRegistry::names`, no hand-kept list) +against every candidate argument list, and requires the interactive arm to +accept whatever the pure arm accepts, in a context whose cursor sits on **no +cell** — a category with no items. That context is what made the old +`commit-cell-edit` arm refuse, so a context with a cell under the cursor would +have passed the bug. The assertion that the cursor has no cell is checked +first; a plain `Workbook::new` does not satisfy it, which is how the fixture +came to add the empty category. Rival (b) fails it. + +### explicit_open_and_focus_arguments_beat_the_toggle + +The fifth instance, named on improvise-2bac as plausible and not yet run: +`toggle-panel-and-focus false false` ignored both flags and toggled, so +on a **closed** panel it opened it. The fixture closes every panel first, +because on an open-and-focused panel the toggle also closes it and the test +would pass against the bug. Rival: `execute` ignoring `target` and always +toggling — fails this test for every panel. diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index e8f8e04d..6ef243dc 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -91,7 +91,11 @@ pub fn default_registry() -> CmdRegistry { }) }, as_parsed, - Some(|_, ctx| Ok(ClearCellCommand { key: cursor_key(ctx)? })), + Some(|_, ctx| { + Ok(ClearCellCommand { + key: cursor_key(ctx)?, + }) + }), ); r.register_pure(&AddFormulaCmd(vec![]), AddFormulaCmd::parse); r.register_pure(&FitMeasure::prototype(), FitMeasure::parse); @@ -243,7 +247,11 @@ pub fn default_registry() -> CmdRegistry { }) }, as_parsed, - Some(|_, ctx| Ok(YankCell { key: cursor_key(ctx)? })), + Some(|_, ctx| { + Ok(YankCell { + key: cursor_key(ctx)?, + }) + }), ); r.register( &PasteCell { @@ -258,7 +266,11 @@ pub fn default_registry() -> CmdRegistry { }) }, as_parsed, - Some(|_, ctx| Ok(PasteCell { key: cursor_key(ctx)? })), + Some(|_, ctx| { + Ok(PasteCell { + key: cursor_key(ctx)?, + }) + }), ); // clear-cell is registered above (unified: ctx.cell_key() or explicit coords) @@ -307,7 +319,11 @@ pub fn default_registry() -> CmdRegistry { }) }, as_parsed, - Some(|_, ctx| Ok(DrillIntoCell { key: cursor_key(ctx)? })), + Some(|_, ctx| { + Ok(DrillIntoCell { + key: cursor_key(ctx)?, + }) + }), ); r.register_nullary(|| Box::new(ViewNavigate { forward: false })); r.register_nullary(|| Box::new(ViewNavigate { forward: true })); @@ -742,9 +758,7 @@ mod tests { #[test] fn commit_cell_edit_honours_an_explicit_value_and_cell_over_the_cursor() { let mut app = crate::ui::app::App::new(two_cat_model(), None); - app.view_state - .buffers - .insert("edit".into(), "decoy".into()); + app.view_state.buffers.insert("edit".into(), "decoy".into()); let ctx = app.cmd_context( crate::command::key::KeyCode::Null, crate::command::key::KeyModifiers::NONE, @@ -753,7 +767,11 @@ mod tests { ("Month".into(), "Feb".into()), ("Type".into(), "Clothing".into()), ]); - assert_ne!(ctx.cell_key().as_ref(), Some(&target), "the cursor is elsewhere"); + assert_ne!( + ctx.cell_key().as_ref(), + Some(&target), + "the cursor is elsewhere" + ); let args: Vec = ["editing", "5", "Type/Clothing", "Month/Feb"] .map(String::from) .into(); @@ -763,7 +781,10 @@ mod tests { .interactive("commit-cell-edit", &args, &ctx) .expect("commit-cell-edit resolves") ); - assert!(dbg.contains(&format!("key: {target:?}")), "wrong cell: {dbg}"); + assert!( + dbg.contains(&format!("key: {target:?}")), + "wrong cell: {dbg}" + ); assert!(dbg.contains("value: \"5\""), "wrong value: {dbg}"); } diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 1789f9c6..f0751f16 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -410,7 +410,10 @@ fn commit_cell_edit_writes_the_named_cell_and_spares_the_cursors() { fn a_line_whose_gate_passes_runs_whole() { let scratch = Scratch::new("commit-atomic"); let doc = scratch.path("atomic.improv"); - let out = run_cmd(&["add-category Foo . commit-cell-edit editing 5 Foo/x"], &doc); + let out = run_cmd( + &["add-category Foo . commit-cell-edit editing 5 Foo/x"], + &doc, + ); assert!( !stderr_of(&out).contains("Parse error"), "a line the gate accepted was refused part-way: {}", @@ -418,7 +421,8 @@ fn a_line_whose_gate_passes_runs_whole() { ); let text = std::fs::read_to_string(&doc).expect("the document"); assert!( - text.lines().any(|l| l.contains("Foo=x") && l.ends_with("= 5")), + text.lines() + .any(|l| l.contains("Foo=x") && l.ends_with("= 5")), "the commit did not land: {text}" ); } From 912e25a0acf391e9096c4501fd4f805886ea4942 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:04:07 -0700 Subject: [PATCH 8/9] Rolled-back-load tests witness that the load ran (improvise-gho5) Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/command.md | 36 ++++++++++++++++++++++--- crates/improvise-command/src/ui/host.rs | 32 ++++++++++++++++++---- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index efcda9b0..fcbee537 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -2382,6 +2382,31 @@ keystroke to land in. Injected fault: dropping `self.view_state = snapshot.view_state` from `App::restore` fails this test and only this test. +**Addendum (improvise-gho5).** Improvise-gho5 found that this +test and `a_rolled_back_load_tells_nobody_and_puts_every_mirror_back` passed +against a host that **never ran the load**. The review's fault, reproduced +here, was placed at the top of `Host::commands`: refuse any multi-command frame +that contains a `LoadModel` with `Rejected(DocumentReplaced)`, before +`CheckedBatch` runs. Both tests stayed green. A frame that never got that far +leaves the generation, the view, the status line and every mirror untouched, +which is exactly what the tests asserted. + +Both tests now assert that the asker's whole reply is +`the_load_ran_then_the_write_broke()`, which is `Rejected(ChainBroken(North))`. +They also assert, before the frame, that `North` resolves. Together these make +the rejection a witness that the load ran. `ChainBroken` comes only from +`reduce::broken_chain`, which runs inside `mutating_frame` just before each +mutation. It names `North` only once the document no longer holds that +record, and only an applied `LoadModel` in this frame can have removed it. + +With the same fault re-injected, both tests fail. The left side is +`[Rejected(DocumentReplaced)]` and the right is +`[Rejected(ChainBroken())]`. The fault was reverted. + +The residual: a rogue up-front check that forged `ChainBroken` specifically +would still pass. That is the same forgery a hand-rolled client could commit +against any basis check, and a test cannot guard against it. + ### a_refused_mutation_rolls_back_its_predecessor_and_the_abort_flag The `Rejection::MutationRefused` arm — the one the first hand-back wrongly called @@ -4885,9 +4910,14 @@ Three things make it non-vacuous: `LoadModel` silently failed, `Applied::Rebased` would never run and every assertion here would pass over nothing. The control asserts the generation moves and that *both* clients are owed a `Snapshot`. -- **The rollback is asserted before anything is concluded from it** — the - `document_generation` equality is checked first, so "the frame was rejected" - cannot be confused with "the frame never got that far". +- **The load is witnessed to have RUN before anything is concluded from the + rollback** (improvise-gho5). The asker's whole reply must be + `Rejected(ChainBroken(North))`. `North` is asserted to resolve before the + frame, so only `broken_chain` can give that answer, and only after the + `LoadModel` has replaced the document. `CheckedBatch` never produces + `ChainBroken`. This bullet used to say the `document_generation` equality + did the job, and that was false: generation equality is exactly what holds + when the frame never got that far. - **A bystander is seated where a wrong relay could move it.** A single-client fixture cannot tell "the mirrors were restored" from "there was one mirror and it happened to be the asker's". diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 50d7b392..166f16e0 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -1049,6 +1049,12 @@ mod tests { let was_view = host.app().view_state.active_view.clone(); let was_status = host.app().view_state.status_msg.clone(); let basis = frame_basis(&host, &["North"]); + assert!( + host.app() + .resolve_record(addressed("North").record) + .is_some(), + "North must resolve BEFORE the load, or a refusal naming it proves nothing" + ); // The load lands, then the write names a record the incoming document // has never had — so the frame fails at its second command. @@ -1069,11 +1075,10 @@ mod tests { ]), ); - assert!( - answer - .iter() - .any(|a| matches!(a.msg, ServerFrame::Rejected(_))), - "the fixture must actually refuse the frame: {answer:?}" + assert_eq!( + to(&answer, asker), + vec![the_load_ran_then_the_write_broke()], + "only a load that RAN can refuse the write this way: {answer:?}" ); assert_eq!( host.app().view_state.active_view, @@ -1097,6 +1102,12 @@ mod tests { ); } + /// `North` resolves in the outgoing document, so only `broken_chain` AFTER the + /// load applied can answer this. Evidence: `context/test-evidence/command.md`. + fn the_load_ran_then_the_write_broke() -> ServerFrame { + ServerFrame::Rejected(Rejection::ChainBroken(addressed("North").record)) + } + /// Everything one client was told, flattened to the projections it carries. fn projections(frames: &[Addressed], id: SessionId) -> Vec { to(frames, id) @@ -1147,6 +1158,12 @@ mod tests { let (bystander, _) = host.connect(10, 10); let was_generation = host.app().document_generation(); let basis = frame_basis(&host, &["North"]); + assert!( + host.app() + .resolve_record(addressed("North").record) + .is_some(), + "North must resolve BEFORE the load, or a refusal naming it proves nothing" + ); // The load lands and rebases every mirror; the write then names a // record the incoming document has never had, so the frame fails. @@ -1167,6 +1184,11 @@ mod tests { ]), ); + assert_eq!( + to(&answer, asker), + vec![the_load_ran_then_the_write_broke()], + "only a load that RAN can refuse the write this way: {answer:?}" + ); assert_eq!( host.app().document_generation(), was_generation, From 625c11319a93b01c7822300ca3e0c1fb714c06b2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:09:46 -0700 Subject: [PATCH 9/9] fmt-workspaces: exit 2 means no verdict, not no output (improvise-ximf) The unreached-package arm exits 2 and names the package's Cargo.toml, so the usage text's "NO file is named" was false for it. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 8 +++++--- scripts/fmt-workspaces.sh | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index bda78aae..c49691d2 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -417,9 +417,11 @@ getting wrong. The line the split actually needs is **"did the gate run"**: - exit 1 — rustfmt did not certify a root clean. A diff or a refusal to parse; either way **a file is named** and the tree is what is wrong. - exit 2 — the gate did not run. Usage, no cargo, no rustfmt, `cargo metadata` - failed, no `[workspace]` root. **No file is named.** Every such cause is - checked *above* the loop, which is what makes the narrowing true rather than - hopeful. + failed, no `[workspace]` root. **No source file is named.** Every such cause + is checked *above* the loop, which is what makes the narrowing true rather + than hopeful. (Since improvise-90ea, a package no root reaches is also exit 2, + and it names that package's `Cargo.toml`, which is the reason no verdict + exists. The line is "no verdict", not "no output"; improvise-ximf.) Both directions are now pinned, and two further rivals were injected: diff --git a/scripts/fmt-workspaces.sh b/scripts/fmt-workspaces.sh index 982d2211..98bc8370 100755 --- a/scripts/fmt-workspaces.sh +++ b/scripts/fmt-workspaces.sh @@ -19,7 +19,8 @@ exit 1 rustfmt did not certify a root clean: a file differs from rustfmt's and the tree is what is wrong. exit 2 the gate did not run: usage error, no cargo, no rustfmt, cargo metadata failed, no [workspace] root found, or a [package] that no - root has as a member. NO verdict was produced and NO file is named. + root has as a member. NO verdict was produced. No SOURCE file is + named; the unreached-package case names its Cargo.toml. The split is "did the gate run", not "is every exit-1 cause a diff". That is narrower than it sounds and it is deliberate -- see the note on rc=1 below.