fix(cli): a script's command is built against the cursor it will run against (improvise-6uva)

`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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-22 18:59:08 -07:00
parent 6e10bb3afa
commit 64bcb821da
7 changed files with 331 additions and 30 deletions
+40
View File
@@ -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).
+78
View File
@@ -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.
@@ -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)),
});
}
@@ -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<Vec<Box<dyn Cmd>>, String> = crate::command::parse::segments(&raw)
.into_iter()
.map(|seg| crate::command::parse::parse_segment_interactive(seg, ctx))
.collect::<Result<Vec<_>, _>>()
.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).
+119 -1
View File
@@ -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<Vec<Box<dyn Cmd>>, String> {
@@ -33,6 +33,40 @@ pub fn parse_line_with(registry: &CmdRegistry, line: &str) -> Result<Vec<Box<dyn
Ok(commands)
}
/// The command segments of a line, with comments and blank segments dropped.
pub fn segments(line: &str) -> 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<Option<Box<dyn Cmd>>, 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, &reg);
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, &reg);
// `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, &reg);
let err = parse_segment_interactive("no-such-command", &ctx)
.expect_err("an unknown word is refused");
assert!(err.contains("no-such-command"), "got: {err}");
}
}
+33 -19
View File
@@ -344,29 +344,43 @@ fn run_headless_commands(cmds: &[String], file: &Option<PathBuf>) -> 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;
}
}
}
+51
View File
@@ -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");
}