From 71f05675dabd081c014e9b9ace0ae8e82499d3d2 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 18:25:02 -0700 Subject: [PATCH 1/3] test(navigation): a sequence folds in order; the old law was a tautology (improvise-gjat) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `law_sequence_associativity` asserted `effects_debug((a ++ b) ++ c)` equals `effects_debug(a ++ (b ++ c))` — the associativity of `Vec::extend`, true for any deterministic `execute`, and never applying an effect. No command behaviour could move it. `doctrine(fold_tests_must_test_order_not_the_axiom)` names exactly this shape. Replaced with the property production relies on: a `Binding::Sequence` flattens into one effect vec (gotcha 21), the batch folds onto the `App` in order, and each `Move` composed against the same ctx emits an absolute `SetSelected` — so the moves do not compose and the last write wins. Four orderings are asserted, two of which land somewhere the others do not. Two injected faults, each leaving the old assertion passing and failing the new one: `Move::execute` skipping a move that moves nothing (old ok, new left: (0, 1) right: (0, 0)), and `App::apply_effects` folding in reverse (old ok, new left: (1, 0) right: (0, 0)). The second is why the batch is flattened rather than applied per command — a first version applied each separately and could not see it. Evidence: context/test-evidence/command.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 35 ++++++++++++ .../src/command/cmd/navigation.rs | 57 ++++++++----------- 2 files changed, 60 insertions(+), 32 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 2529868f..d0e3983a 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4496,3 +4496,38 @@ So the discriminating input is the **buffer**, not the column: an empty commit creates nothing, inserts no column, and must still start the next row. Injecting the value-blind predicate above turns the branch off entirely and this test fails with `row_count` 1 where 2 is owed, while the headline test above stays green. + +### a_sequence_of_moves_folds_in_order_and_the_last_write_wins + +Replaces `law_sequence_associativity` (improvise-gjat), which could not fail. That +test built three effect vectors from one `CmdContext` and asserted +`effects_debug((a ++ b) ++ c) == effects_debug(a ++ (b ++ c))` — the associativity +of `Vec::extend`, true for any deterministic `execute`, and +`doctrine(fold_tests_must_test_order_not_the_axiom)` names exactly this shape. It +never applied an effect, so no command behaviour could move it. + +The property that can fail is the one production actually relies on: a +`Binding::Sequence` flattens into **one** effect vec (gotcha 21), the batch folds +onto the `App` in order, and each `Move` composed against the *same* ctx emits an +**absolute** `SetSelected` — so the moves do not compose and the last write wins. +The test folds one batch per ordering and asserts the resulting cursor, with +`c, b, a` landing somewhere `a, b, c` does not, so an implementation that ignores +order is separated from one that honours it. + +Two faults were injected, and both leave the old assertion passing: + +- **`Move::execute` returns `Vec::new()` when the computed position equals the + current one** — a plausible "skip a move that moves nothing" optimisation, and + one that would break `tests/lockstep.rs`, since the browser resolver mirrors + `Move` line for line including where it clamps (gotcha 47). The old test still + passed (`c` contributed nothing to either grouping, so the two remained equal); + the new one failed `left: (0, 1) right: (0, 0)` — `b` had become the last write. +- **`App::apply_effects` folds the batch in reverse.** The old test passed + untouched, because it formats the vector and never applies it; the new one + failed `left: (1, 0) right: (0, 0)`. + +The second fault is why the batch is flattened rather than applied one command at +a time. A first version of this test called `apply_effects` per command; each +single-`Move` batch reverses to the same cursor, so the reversed fold was +invisible and the test passed against it. One batch is also the shape a +`Binding::Sequence` really has. diff --git a/crates/improvise-command/src/command/cmd/navigation.rs b/crates/improvise-command/src/command/cmd/navigation.rs index 3d54c5b0..5e156473 100644 --- a/crates/improvise-command/src/command/cmd/navigation.rs +++ b/crates/improvise-command/src/command/cmd/navigation.rs @@ -384,52 +384,45 @@ mod tests { assert_eq!(first, second, "ToStart(Row) should be idempotent"); } + /// Evidence: `context/test-evidence/command.md`. #[test] - fn law_sequence_associativity() { + fn a_sequence_of_moves_folds_in_order_and_the_last_write_wins() { let m = two_cat_model(); let layout = make_layout(&m); let reg = make_registry(); let ctx = make_ctx(&m, &layout, ®); + assert!(ctx.row_count() >= 2 && ctx.col_count() >= 2); - let mk_a = || { + let mk = |kind: MoveKind, cmd_name: &'static str| { Move { - kind: MoveKind::Relative(1, 0), + kind, cursor: CursorState::from_ctx(&ctx), - cmd_name: "move-selection", + cmd_name, } .execute(&ctx) }; - let mk_b = || { - Move { - kind: MoveKind::Relative(0, 1), - cursor: CursorState::from_ctx(&ctx), - cmd_name: "move-selection", - } - .execute(&ctx) - }; - let mk_c = || { - Move { - kind: MoveKind::ToStart(true), - cursor: CursorState::from_ctx(&ctx), - cmd_name: "jump-first-row", - } - .execute(&ctx) + let a = || mk(MoveKind::Relative(1, 0), "move-selection"); + let b = || mk(MoveKind::Relative(0, 1), "move-selection"); + let c = || mk(MoveKind::ToStart(true), "jump-first-row"); + + // A `Binding::Sequence` flattens into ONE effect vec (gotcha 21), so the + // fold under test is a single batch. + let seq = |steps: Vec>>| { + let batch: Vec> = steps.into_iter().flatten().collect(); + assert!(!batch.is_empty()); + let mut app = crate::ui::app::App::new(two_cat_model(), None); + app.apply_effects(batch); + app.cursor().selected }; - let mut ab_c = mk_a(); - ab_c.extend(mk_b()); - ab_c.extend(mk_c()); + assert_eq!(seq(vec![a()]), (1, 0)); + assert_eq!(seq(vec![b()]), (0, 1)); + assert_eq!(seq(vec![c()]), (0, 0)); - let mut bc = mk_b(); - bc.extend(mk_c()); - let mut a_bc = mk_a(); - a_bc.extend(bc); - - assert_eq!( - effects_debug(&ab_c), - effects_debug(&a_bc), - "Sequence concatenation should be associative" - ); + assert_eq!(seq(vec![a(), b(), c()]), (0, 0)); + assert_eq!(seq(vec![c(), b(), a()]), (1, 0)); + assert_eq!(seq(vec![b(), a(), c()]), (0, 0)); + assert_eq!(seq(vec![c(), a(), b()]), (0, 1)); } #[test] From 64bcb821da6731718af5db315f8933f96959933e Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 18:59:08 -0700 Subject: [PATCH 2/3] fix(cli): a script's command is built against the cursor it will run against (improvise-6uva) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `improvise cmd` and `improvise script` resolved every command through `CmdRegistry::parse` — the pure arm, which builds `CursorState::default()`: row 0, col 0, and a grid of zero rows and zero columns. The interactive arm, which builds `CursorState::from_ctx(ctx)`, was never reached from a script. So every `Move` and `enter-advance` snapshotted a zeroed cursor at PARSE time, clamped to (0, 0) and emitted `SetSelected(0, 0)` — worse than a no-op, since it RESETS the cursor. Nothing could position it, so every cursor-reading command was pinned to the origin. Reproduced at a9cad15 on a document with Region on row carrying North, South, East: `move-selection 2 0 . hide-selected-row-item` and a bare `hide-selected-row-item` both wrote `hidden: Region/North`. After the fix the first writes `hidden: Region/East`. The repair is `parse::segments` + `parse::parse_segment_interactive`, with the headless loop re-parsing per segment, because command n must see command n-1's cursor. Line atomicity is preserved by keeping the existing whole-line `parse_line` as a syntax gate, so a line that parsed before still parses and the only behavioural change is which cursor the commands see. Two things the ticket did not predict. The TUI's `:` line had the identical defect — `ExecuteCommand` also used the pure arm — and is fixed the same way. And doing that surfaced a second instance of the same shape one level down: `register_pure` built an interactive arm that refused EMPTY args while its own pure arm accepted them, so `w` with no path parsed one way and refused the other, breaking `improvise cmd 'w'` and `:w` the moment either re-parsed interactively. Both arms are now `parse`. Evidence: context/test-evidence/cli.md; reasoning: context/code/cli.md section 7. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/cli.md | 40 ++++++ context/test-evidence/cli.md | 78 ++++++++++++ .../improvise-command/src/command/cmd/core.rs | 12 +- .../src/command/cmd/text_buffer.rs | 8 +- crates/improvise-command/src/command/parse.rs | 120 +++++++++++++++++- src/main.rs | 52 +++++--- tests/headless_cli.rs | 51 ++++++++ 7 files changed, 331 insertions(+), 30 deletions(-) diff --git a/context/code/cli.md b/context/code/cli.md index 061d3480..e1c20ca3 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -115,3 +115,43 @@ What it still cannot see, and should be closed if the welcome text moves again: `draw.rs::draw_welcome` builds its lines as a local array inside a ratatui render function, so nothing can walk them. Its `:formula` line was corrected by hand. + +## 7 — a script's command is built against the cursor it will run against + +`improvise cmd` and `improvise script` used to resolve every command through +`CmdRegistry::parse` — the **pure** arm, which builds `CursorState::default()`: +row 0, col 0, and a grid of zero rows and zero columns. The interactive arm, +which builds `CursorState::from_ctx(ctx)`, was never reached from a script. + +So every `Move` (`move-selection`, `scroll-rows`, `page-scroll`, the four +`jump-*`) and `enter-advance` snapshotted a zeroed cursor at **parse** time, +clamped to `(0, 0)`, and emitted `SetSelected(0, 0)`. That is worse than a +no-op: it *resets* the cursor. Nothing could position it, so every +cursor-reading command — `clear-cell`, `yank`, `paste`, `cycle-axis-at-cursor`, +`hide-selected-row-item`, `edit-or-drill`, `drill-into-cell`, +`toggle-group-under-cursor`, the three `delete-*-at-cursor`, +`commit-and-advance` — was pinned to the origin (improvise-6uva). It was +introduced with `CursorState` and the two-arm registry in April and predates the +browser work by five months. + +**A registry with two construction arms silently demotes every caller of the +weaker arm, and nothing type-checks which arm a caller deserves.** That is the +shape to remember, and it has a second instance in the same fix: the +`register_pure` entries had an interactive arm that refused **empty** args while +their pure arm accepted them, so `:w` (save to the current path) parsed one way +and refused the other. Both arms are now `parse`, which is what "needs no +interactive context" was supposed to mean; +`a_pure_commands_two_arms_agree_on_no_arguments` is the guard. + +The repair is `parse::segments` plus `parse::parse_segment_interactive`, and the +loop re-parses **per segment**, because command *n* must see command *n-1*'s +cursor — which means construction cannot be hoisted out of the loop. Line +atomicity is preserved by keeping the existing whole-line `parse_line` as a +**syntax gate** before anything in the line runs: a line that parsed before +still parses, and a line that did not still does not, so the only behavioural +change is which cursor the commands see. + +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). diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 0e965a1b..668dce11 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -135,3 +135,81 @@ spawned child at a stable path. Runs `add-category Region` to create the document. It asserts the file exists afterwards rather than trusting the seed, because every test that follows would otherwise misreport as "the command did nothing" when the seed was what failed. + +### a_script_can_move_the_cursor_and_the_next_command_sees_it + +`improvise cmd` and `improvise script` resolved every command through +`CmdRegistry::parse` — the **pure** arm, which builds `CursorState::default()` +(row 0, col 0, zero rows, zero columns). The interactive arm, which builds +`CursorState::from_ctx(ctx)`, was never reached from a script, so every `Move` +snapshotted a zeroed cursor at *parse* time, clamped to `(0, 0)` and emitted +`SetSelected(0, 0)` — worse than a no-op, since it **resets** the cursor +(improvise-6uva). The reasoning is `context/code/cli.md` section 7. + +Reproduced at `a9cad15` before the fix, on a seeded document with `Region` on +row carrying `North, South, East`: `move-selection 2 0 . hide-selected-row-item` +and a bare `hide-selected-row-item` both wrote `hidden: Region/North`. After the +fix the first writes `hidden: Region/East`. + +**Why the test has two halves.** Asserting only that the moved script hides +`East` cannot separate "the move worked" from "the origin happens to be East". +The second document asserts the origin is `North`, so the first assertion is +discriminating, and its message says what breaks if that ever changes — three +identical replays of a fixture whose moves are silent no-ops prove nothing, +which is how this survived five months. + +Two faults were injected, one per layer, and both fail this test: + +- **`parse_segment_interactive` calls `ctx.registry.parse` instead of + `.interactive`** — the bug itself, one level down. + `left: "hidden: Region/North" right: "hidden: Region/East"`. +- **`run_headless_commands` builds the segment up front with `parse_line`** + rather than per segment against a live `CmdContext` — the composition half. + Same failure. The unit test in `parse.rs` passes under this second one, which + is why both tests exist. + +### a_script_cursor_move_survives_the_line_that_made_it + +The same property across two `-f` arguments rather than one dot-separated line, +because `improvise cmd` takes several and `improvise script` is one per line: +the cursor is `App` state and a line boundary is not a reset. + +### a_segment_is_built_against_the_cursor_it_will_run_against + +The unit test for the layer that owns the bug. It seats a `CmdContext` at +`(1, 0)` in a two-by-two grid and asserts `move-selection 0 1` executes to +`SetSelected(1, 1)`. + +Its second half is what keeps it honest: it builds the *same* command through +`registry.parse` and asserts that one gives `SetSelected(0, 0)`. If the pure arm +ever learns the cursor, the first assertion stops discriminating and this half +fails first, saying so. + +### a_pure_commands_two_arms_agree_on_no_arguments + +A second instance of the same shape, found by the first fix rather than +predicted by the ticket. `CmdRegistry::register_pure` built an interactive arm +that refused **empty** args while its own pure arm accepted them, so `w` with no +path — save to the current document — parsed one way and refused the other. Once +a script re-parsed through the interactive arm, `improvise cmd 'w'` and `:w` +both broke; the suite caught it as +`a_failed_colon_write_lands_in_normal_where_its_error_is_drawn` reporting +`":w — w: this command requires arguments"`. + +Both arms are now `parse`, which is what "needs no interactive context" was +supposed to mean. Restoring the empty-args guard fails this test on the first +`expect`: the interactive arm refuses a bare `w` the pure arm accepts. + +### segments_drops_comments_and_blanks + +`segments` is the split half of `parse_line_with`, extracted so the headless +loop can re-parse one segment at a time. It answers the same three cases the +whole-line parser did — a `#` comment, a `//` comment and a blank line are no +commands — plus the empty segment in `a . . b`. + +### an_unknown_segment_is_an_error_not_a_silent_skip + +`parse_segment_interactive` returns `Ok(None)` for a segment with no word, which +is a shape an unknown *word* must not be able to reach: a silently skipped +command in a script is the partial-and-silent failure this repo keeps finding. +The error also names the word, matching what `CmdRegistry::parse` does. diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index e0d85709..4af2470f 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -193,19 +193,13 @@ impl CmdRegistry { }); } - /// Register a command that needs no interactive context: with args it delegates - /// to `parse`, and without them it errors. + /// Register a command that needs no interactive context: BOTH arms are + /// `parse`, so neither can demote the other — `context/code/command.md`. pub fn register_pure(&mut self, prototype: &dyn Cmd, parse: ParseFn) { self.entries.push(CmdEntry { name: prototype.name(), parse: Box::new(parse), - interactive: Box::new(move |args, _ctx| { - if args.is_empty() { - Err("this command requires arguments".into()) - } else { - parse(args) - } - }), + interactive: Box::new(move |args, _ctx| parse(args)), }); } diff --git a/crates/improvise-command/src/command/cmd/text_buffer.rs b/crates/improvise-command/src/command/cmd/text_buffer.rs index 5a373986..5628dac4 100644 --- a/crates/improvise-command/src/command/cmd/text_buffer.rs +++ b/crates/improvise-command/src/command/cmd/text_buffer.rs @@ -359,7 +359,13 @@ impl Cmd for ExecuteCommand { return vec![effect::change_mode(AppMode::Normal)]; } - match crate::command::parse::parse_line_with(ctx.registry, &raw) { + // Built against the live cursor, exactly as a script's is — improvise-6uva. + let built: Result>, String> = crate::command::parse::segments(&raw) + .into_iter() + .map(|seg| crate::command::parse::parse_segment_interactive(seg, ctx)) + .collect::, _>>() + .map(|built| built.into_iter().flatten().collect()); + match built { Ok(cmds) => { // Leave the command line **first**, before anything that can fail: // an abort must stop the chain, never the bookkeeping (gotcha 21). diff --git a/crates/improvise-command/src/command/parse.rs b/crates/improvise-command/src/command/parse.rs index 8a40a4f8..3f236d47 100644 --- a/crates/improvise-command/src/command/parse.rs +++ b/crates/improvise-command/src/command/parse.rs @@ -1,7 +1,7 @@ //! Quasi-lisp prefix command parser: `word arg1 arg2 …`, `.` between commands, //! `Category/Item` coordinate pairs, `"quoted strings"`. -use super::cmd::{Cmd, CmdRegistry, default_registry}; +use super::cmd::{Cmd, CmdContext, CmdRegistry, default_registry}; /// Parse a line into commands using the default registry. pub fn parse_line(line: &str) -> Result>, String> { @@ -33,6 +33,40 @@ pub fn parse_line_with(registry: &CmdRegistry, line: &str) -> Result Vec<&str> { + let line = line.trim(); + if line.is_empty() || line.starts_with('#') || line.starts_with("//") { + return Vec::new(); + } + split_on_dot(line) + .into_iter() + .map(str::trim) + .filter(|s| !s.is_empty()) + .collect() +} + +/// Build one segment's command against a LIVE context — `context/code/cli.md`. +pub fn parse_segment_interactive( + segment: &str, + ctx: &CmdContext, +) -> Result>, String> { + let tokens = tokenize(segment.trim()); + let Some(word) = tokens.first() else { + return Ok(None); + }; + ctx.registry + .interactive(word, &tokens[1..], ctx) + .map(Some) + .map_err(|err| { + if err.starts_with("Unknown command") { + err + } else { + format!("{word}: {err}") + } + }) +} + /// Split a line on ` . ` separators (dot must be a standalone word, /// surrounded by whitespace or at line boundaries). Respects quoted strings. fn split_on_dot(line: &str) -> Vec<&str> { @@ -229,4 +263,88 @@ mod tests { assert!(parse_line("add-items").is_err()); assert!(parse_line("add-items Region").is_err()); } + + // ── a segment is built against the cursor it will run against ──────── + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_segment_is_built_against_the_cursor_it_will_run_against() { + use crate::command::cmd::test_helpers::*; + + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + let mut ctx = make_ctx(&m, &layout, ®); + ctx.selected = (1, 0); + assert!(ctx.row_count() >= 2 && ctx.col_count() >= 2); + + let built = parse_segment_interactive("move-selection 0 1", &ctx) + .expect("move-selection parses") + .expect("a segment with a word builds a command"); + let dbg = format!("{:?}", built.execute(&ctx)); + assert!( + dbg.contains("SetSelected(1, 1)"), + "a segment built against the live cursor must move FROM it, not \ + from the origin (improvise-6uva); got: {dbg}" + ); + + // The pure arm is what a script used to go through, and it is what the + // interactive arm must not be mistaken for. + let pure = reg + .parse("move-selection", &["0".to_string(), "1".to_string()]) + .expect("the pure arm parses too"); + assert!( + format!("{:?}", pure.execute(&ctx)).contains("SetSelected(0, 0)"), + "if the pure arm ever learns the cursor, the assertion above stops \ + discriminating" + ); + } + + #[test] + fn segments_drops_comments_and_blanks() { + assert_eq!(segments("# a comment"), Vec::<&str>::new()); + assert_eq!(segments("// also a comment"), Vec::<&str>::new()); + assert_eq!(segments(" "), Vec::<&str>::new()); + assert_eq!(segments("a . . b"), vec!["a", "b"]); + assert_eq!(segments("move-selection 2 0"), vec!["move-selection 2 0"]); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_pure_commands_two_arms_agree_on_no_arguments() { + use crate::command::cmd::test_helpers::*; + + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + let ctx = make_ctx(&m, &layout, ®); + + // `w` with no path saves to the current document. The interactive arm + // used to refuse exactly that, which is the same demotion one level down. + let by_parse = reg + .parse("w", &[]) + .expect("the pure arm accepts a bare `w`"); + let by_ctx = parse_segment_interactive("w", &ctx) + .expect("so must the interactive arm") + .expect("a segment with a word builds a command"); + assert_eq!(by_parse.name(), by_ctx.name()); + assert_eq!( + format!("{:?}", by_parse.execute(&ctx)), + format!("{:?}", by_ctx.execute(&ctx)), + "the two arms of a pure command must build the same thing" + ); + } + + #[test] + fn an_unknown_segment_is_an_error_not_a_silent_skip() { + use crate::command::cmd::test_helpers::*; + + let m = two_cat_model(); + let layout = make_layout(&m); + let reg = make_registry(); + let ctx = make_ctx(&m, &layout, ®); + let err = parse_segment_interactive("no-such-command", &ctx) + .expect_err("an unknown word is refused"); + assert!(err.contains("no-such-command"), "got: {err}"); + } } diff --git a/src/main.rs b/src/main.rs index 62499e61..c714bd49 100644 --- a/src/main.rs +++ b/src/main.rs @@ -344,29 +344,43 @@ fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> let mut changed = false; for line in cmds { - match command::parse_line(line) { - Ok(parsed_cmds) => { - for cmd in &parsed_cmds { - let effects = { - let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); - cmd.execute(&ctx) - }; - app.apply_effects(effects); - changed |= app.model_state.dirty; - // The status line is the only channel a command has to - // report anything — `context/code/cli.md`. - let said = std::mem::take(&mut app.view_state.status_msg); - if !said.trim().is_empty() { - eprintln!("{said}"); - } - if app.last_batch_aborted() { - exit_code = 1; + // The whole line is syntax-checked before any of it runs, exactly as + // before; only CONSTRUCTION moved — `context/code/cli.md`. + if let Err(e) = command::parse_line(line) { + eprintln!("Parse error: {e}"); + exit_code = 1; + continue; + } + for segment in command::parse::segments(line) { + let mut failed = None; + let effects = { + // Built against the cursor it is about to run against, so a + // command that snapshots one sees command n-1's (improvise-6uva). + let ctx = app.cmd_context(KeyCode::Null, KeyModifiers::NONE); + match command::parse::parse_segment_interactive(segment, &ctx) { + Ok(Some(cmd)) => cmd.execute(&ctx), + Ok(None) => Vec::new(), + Err(e) => { + failed = Some(e); + Vec::new() } } - } - Err(e) => { + }; + if let Some(e) = failed { eprintln!("Parse error: {e}"); exit_code = 1; + break; + } + app.apply_effects(effects); + changed |= app.model_state.dirty; + // The status line is the only channel a command has to + // report anything — `context/code/cli.md`. + let said = std::mem::take(&mut app.view_state.status_msg); + if !said.trim().is_empty() { + eprintln!("{said}"); + } + if app.last_batch_aborted() { + exit_code = 1; } } } diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 5e50217c..99c9427b 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -266,3 +266,54 @@ fn a_run_that_changed_the_document_still_writes_it() { "the -f document lost an edit because `:w` elsewhere cleared the flag: {text}" ); } + +/// A document whose row axis carries three items, so a cursor move has +/// somewhere to go and the three landing places are distinguishable. +fn three_rows(scratch: &Scratch, name: &str) -> PathBuf { + let doc = scratch.path(name); + run_cmd(&["add-category Region"], &doc); + run_cmd(&["add-items Region North South East"], &doc); + doc +} + +fn hidden_line(doc: &Path) -> String { + std::fs::read_to_string(doc) + .expect("the document") + .lines() + .find(|l| l.starts_with("hidden:")) + .unwrap_or("(nothing hidden)") + .to_string() +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_script_can_move_the_cursor_and_the_next_command_sees_it() { + let scratch = Scratch::new("cursor"); + let moved = three_rows(&scratch, "moved.improv"); + let still = three_rows(&scratch, "still.improv"); + + run_cmd(&["move-selection 2 0 . hide-selected-row-item"], &moved); + run_cmd(&["hide-selected-row-item"], &still); + + assert_eq!( + hidden_line(&still), + "hidden: Region/North", + "without a move the cursor is at the origin; if this changes, the other \ + half of this test stops meaning anything" + ); + assert_eq!( + hidden_line(&moved), + "hidden: Region/East", + "`move-selection 2 0` must move the cursor two rows down before the \ + next command reads it (improvise-6uva)" + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_script_cursor_move_survives_the_line_that_made_it() { + let scratch = Scratch::new("cursor-lines"); + let doc = three_rows(&scratch, "lines.improv"); + run_cmd(&["move-selection 1 0", "hide-selected-row-item"], &doc); + assert_eq!(hidden_line(&doc), "hidden: Region/South"); +} From b855faf3c5249fde6f54e043204284a1bb544c40 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 19:07:33 -0700 Subject: [PATCH 3/3] fix(cli): an explicit coordinate still beats the cursor (improvise-6uva) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the ChatGPT Codex review of PR #151 at head 64bcb82, and correct. `clear-cell`, `yank`, `paste` and `drill-into-cell` are registered with `register`, and their INTERACTIVE arms took `|_args, ctx|`: they discarded the arguments and used `ctx.cell_key()`. That was harmless while a script only ever reached the pure arm, which reads the coordinates. The moment the headless loop re-parsed interactively, `clear-cell Region/East` passed the syntax gate and then cleared whatever the cursor was sitting on — the origin, `Region/North`. So this ticket's own lesson has a third instance, inside 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 cursor is the fallback, which is what the keymap path wants anyway since a binding carries no arguments. `an_explicit_coordinate_still_beats_the_cursor` writes three cells and clears the one the cursor is NOT on, so it separates "cleared the named cell" from "cleared the cursor cell". Restoring the arg-discarding arm fails it. Evidence: context/test-evidence/cli.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/cli.md | 25 ++++++++++++ .../src/command/cmd/registry.rs | 25 ++++++++---- tests/headless_cli.rs | 40 +++++++++++++++++++ 3 files changed, 82 insertions(+), 8 deletions(-) diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 668dce11..bf5f7818 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -213,3 +213,28 @@ commands — plus the empty segment in `a . . b`. is a shape an unknown *word* must not be able to reach: a silently skipped command in a script is the partial-and-silent failure this repo keeps finding. The error also names the word, matching what `CmdRegistry::parse` does. + +### an_explicit_coordinate_still_beats_the_cursor + +Found by the ChatGPT Codex review of PR #151 at head `64bcb82`, and correct: the +first version of improvise-6uva's fix introduced a regression it was not looking +for. Four registry entries — `clear-cell`, `yank`, `paste`, `drill-into-cell` — +were registered with `register`, and their **interactive** arms took `|_args, +ctx|`: they discarded the arguments and used `ctx.cell_key()`. That was harmless +while a script only ever reached the *pure* arm, which reads the coordinates. The +moment the headless loop re-parsed interactively, `clear-cell Region/East` passed +the syntax gate and then cleared whatever the cursor was sitting on — the origin, +`Region/North`. + +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 +cursor is the fallback, which is also what the keymap path wants since a binding +carries no arguments. + +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"]`. diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index 99905588..de1d2985 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -22,6 +22,15 @@ 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)) + } +} + /// Build the default command registry with all commands. /// Registry names MUST match the `Cmd::name()` return value. pub fn default_registry() -> CmdRegistry { @@ -46,8 +55,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(ClearCellCommand { key })) }, ); @@ -240,8 +249,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(YankCell { key })) }, ); @@ -257,8 +266,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(PasteCell { key })) }, ); @@ -308,8 +317,8 @@ pub fn default_registry() -> CmdRegistry { key: parse_cell_key_from_args(args), })) }, - |_args, ctx| { - let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; + |args, ctx| { + let key = key_from_args_or_cursor(args, ctx)?; Ok(Box::new(DrillIntoCell { key })) }, ); diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index 99c9427b..f5ae9bb6 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -317,3 +317,43 @@ fn a_script_cursor_move_survives_the_line_that_made_it() { run_cmd(&["move-selection 1 0", "hide-selected-row-item"], &doc); assert_eq!(hidden_line(&doc), "hidden: Region/South"); } + +fn cell_lines(doc: &Path) -> Vec { + std::fs::read_to_string(doc) + .expect("the document") + .lines() + .filter(|l| l.contains(" = ")) + .map(|l| l.trim().to_string()) + .collect() +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn an_explicit_coordinate_still_beats_the_cursor() { + let scratch = Scratch::new("explicit"); + let doc = scratch.path("cells.improv"); + run_cmd(&["add-category Region"], &doc); + run_cmd(&["add-items Region North South East"], &doc); + 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"); + + // The cursor is at the origin, which is North. Clearing East by name must + // clear East, not whatever the cursor happens to be sitting on. + run_cmd(&["clear-cell Region/East"], &doc); + let left = cell_lines(&doc); + assert!( + left.iter().any(|l| l.contains("North")), + "the cursor cell was cleared instead of the named one: {left:?}" + ); + assert!( + !left.iter().any(|l| l.contains("East")), + "the named cell survived: {left:?}" + ); +}