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"); +}