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