Merge pull request #126 from fiddlerwoaroof/fix/command-layer

fix(command): two P1s in the key path, and both were about silence

context/code/command.md

@@ -2933,3 +2933,88 @@ it through `display_value_of`, and `grid_of` projects the committed value to
 everyone, **including that session's own grid**. The producer never reads
 `drill_state`, and teaching it to would put one client's uncommitted typing on
 another's screen.
+
+## A bound key that builds nothing (improvise-55l5)
+
+`KeymapSet::dispatch` returns `Option<Vec<Box<dyn Effect>>>`, and until this
+ticket that option carried **two** facts under one shape: *no binding matched*,
+and *a binding matched but its command would not construct*. `run` spelled the
+second `registry.interactive(name, args, ctx).ok()?`, and `App::dispatch_key`
+answers `None` with `Dirty::NOTHING` — so a bound key could be a black hole.
+
+The interesting half is which errors that actually hid, because a fix that
+surfaces errors nobody can act on is noise.
+
+**The class that reaches a user is `no cell at cursor`.** Six registrations in
+`cmd/registry.rs` read `ctx.cell_key()` and refuse without one: `clear-cell`,
+`yank-cell`, `paste-cell`, `drill-into-cell`, `commit-cell-edit` and
+`commit-and-advance-right`. It is *state-dependent*, so nothing checkable at
+registration time can see it, and the two commit entries are the worst case —
+the user is in `Editing`, has typed a value, presses Enter, and the batch is
+discarded with the minibuffer still up (which is where `draw_bottom_bar` paints
+instead of the status line, gotcha 21).
+
+**Everything else the constructors can return is a keymap-authoring error**:
+`Unknown command`, `require_args`, `Unknown mode` from `parse_mode_name`, and
+`commit-buffer item requires item-add mode` — which is the repro the ticket
+itself proposes. `every_bound_command_name_is_registered` already refuses all of
+these at test time, checking *both* halves of the old expression precisely
+because improvise-s13 made a typo'd mode name compile into a dead key
+(gotcha 15). They are therefore unreachable from a shipped keymap, and they
+surface now only because the mechanism is one rule rather than a list of
+blessed errors — which is the right direction: a list would be a second
+description of which failures matter, free to disagree with the first.
+
+So the rule is: **`None` means no binding matched, and nothing else.** A
+construction failure becomes `Some(vec![set_status(format!("{name}: {e}"))])`.
+The command name is in the message because the two classes need different
+readers — a user reads "no cell at cursor", a maintainer reads which binding
+produced it — and one string serves both without a second channel.
+
+A `Sequence` whose later step fails reports and runs **none** of the batch.
+That is `try_fold`'s shape and it is not a new rule: gotcha 21's abort is about
+effects that *ran*, this is about effects that were never built, and there is
+nothing to roll back.
+
+What this does **not** do, and should not: report an unbound key. `an_unbound_key_stays_silent`
+is the control, and the injected rival that makes `lookup`'s `None` a status
+fails it. Every unmapped character in every mode would otherwise write one.
+
+## `add-record-row` names the row it lands on (improvise-o9uq)
+
+`o` in records mode is `[add-record-row, enter-edit-at-cursor records-editing]`,
+and `AddRecordRow::execute` returned `SetCell` plus a status and **never placed
+the cursor**. The result was silent corruption of an unrelated record: appending
+a row changes `view_frame`'s hash, the frame observation inside `apply_effects`
+invalidates the cursor to the origin *mid-batch* (gotcha 24), and
+`EnterEditAtCursor` then pre-fills the editor with row 0's category coordinate.
+Enter commits through `rename_record_coordinate`, which by gotcha 1 moves every
+measure cell of that row.
+
+Two things are worth carrying past this fix.
+
+**The order is the whole repair, and the acceptance criteria did not say so.**
+"Append `viewport_effects` after its `SetCell`" is only correct because the
+observation runs inside the loop; the same two effects in the other order are
+the bug verbatim, and the two are indistinguishable by reading. The injected
+rival for the regression test is therefore the *swap*, not the omission.
+
+**`CommitAndAdvance`'s Tab path already did this**, and that is the tell to
+generalise: one of two call sites placed the cursor and the other did not, so
+the placement was a property of the *caller* where it is a property of the
+command. It is now the command's, and the Tab site's own later
+`viewport_effects` still wins because it is emitted after.
+
+**Where it lands** is `row_count()` — an append is last — unless the record
+already exists, in which case it is that record's index, found by scanning
+`record_row(r).coords`. Predicting `row_count()` unconditionally would put the
+cursor one past the end in that arm. That arm also stops claiming "Added new
+record row" for a row it did not add; it is not made a *refusal*, because the
+Tab path depends on seeding an empty-coordinate row and a refusal there would
+break bulk entry.
+
+**The deeper defect is untouched and filed.** `view_frame` hashes `entries.len()`
+and every entry, so *any* append reframes and throws the cursor to the origin
+although nothing was renumbered. Placing the cursor masks that at this call
+site. Distinguishing "rows appended" from "rows renumbered" is a larger question
+and does not belong in a P1 data-loss fix.

context/test-evidence/command.md

@@ -4217,6 +4217,111 @@ The neighbouring test `a_refused_switch_corrects_the_asker_and_nobody_else`
 compared `active_view` alone and stayed green through all of this: the check
 that answers a neighbouring question, in the place the doctrine warns about.
 
+## `command/keymap.rs` — a bound key that builds nothing (improvise-55l5)
+
+### a_bound_key_whose_command_cannot_be_built_says_so
+
+`dispatch` resolved a bound name with `registry.interactive(name, args, ctx).ok()?`,
+so a command that would not *construct* collapsed into the same `None` an
+**unbound** key returns — and `App::dispatch_key` answers `None` with
+`Dirty::NOTHING` and no status. The two are different facts and the option
+could carry only one of them.
+
+Which errors were invisible, and what each would have caught:
+
+- **`no cell at cursor`** — six registrations read `ctx.cell_key()`:
+  `clear-cell`, `yank-cell`, `paste-cell`, `drill-into-cell`,
+  `commit-cell-edit` and `commit-and-advance-right`. This is the class that
+  reaches a user: it is *state-dependent*, so no compile-time or registration
+  check can see it. `x` on a grid whose row axis carries an empty category
+  did nothing and said nothing.
+- **`Unknown command`, `require_args`, `Unknown mode`,
+  `commit-buffer item requires item-add mode`** — keymap-authoring errors.
+  `every_bound_command_name_is_registered` already refuses these at test time
+  (gotcha 15 checks *both* halves for exactly this reason), so they are
+  unreachable from a shipped keymap. They surface now because the mechanism is
+  one rule rather than a list, not because they were the complaint.
+
+The repro is the first class. `app_with_no_cells` puts an item-less category on
+`Axis::Row`, and the setup asserts `layout.cell_key(0, 0) == None` — so the
+fixture states the precondition rather than hoping for it. The test then asserts
+`x` **is bound** before pressing it, because a test that pressed an unbound key
+would pass against the bug.
+
+Injected rivals, one per test:
+
+- **`Some(built.unwrap_or_default())`** — return `Some` but swallow the error,
+  the smallest thing that looks like a fix. `a_bound_key_…_says_so` fails;
+  `an_unbound_key_stays_silent` passes, as it should.
+- **Report a failed *lookup* as well** (`None` from `Keymaps::lookup` becomes a
+  status). `an_unbound_key_stays_silent` fails; the other passes. That is the
+  neighbouring question this pair separates: *did a binding match* is not *did
+  the command build*, and a fix that answers both with a message is noise on
+  every typo and every unmapped key.
+
+A `Sequence` whose later step fails now reports and runs **none** of it, which
+is the existing all-or-nothing shape (`try_fold`), not a new rule — gotcha 21's
+abort is about effects that ran, this is about effects that were never built.
+
+### an_unbound_key_stays_silent
+
+The control. `None` still means, and only means, *no binding matched*. It presses
+a key the Normal keymap does not bind and asserts the status stays empty, and it
+asserts that key is unbound first — without that assertion the test would pass
+against a keymap in which it *was* bound to something silent.
+
+## `command/cmd/grid.rs` — `o` edits the row it added (improvise-o9uq)
+
+### o_in_records_mode_edits_the_row_it_added_and_leaves_the_others_alone
+
+`o` is `[add-record-row, enter-edit-at-cursor records-editing]`, and
+`AddRecordRow::execute` returned only `SetCell` + a status: it never placed the
+cursor. Appending a record changes `view_frame`'s hash, so the frame observation
+inside `apply_effects` (gotcha 24) invalidated the cursor to the origin
+*mid-batch*, and `EnterEditAtCursor` then opened an editor pre-filled with **row
+0's** `Region` coordinate. Typing appended to it and Enter went through
+`commit_plain_records_edit` → `rename_record_coordinate`, moving every measure
+cell of an unrelated record (gotcha 1: a category-column edit moves the whole
+row, which is why the basis unit is the record — gotcha 40).
+
+Measured before the fix, from `Region{East,West}` with `Revenue` 10 and 20, at
+`sel=(1,0)`, keys `o N o r t h Enter`: `East`'s `Revenue` cell was **gone** and
+the rows read `["0","1","2"]` — the East record had become `EastNorth`.
+
+The fix appends `viewport_effects(target_row, 0, ..)` **after** the `SetCell`,
+which is the whole content of the repair: the observation is inside the loop, so
+a placement emitted after the reframe survives and one emitted before does not.
+
+Injected rival: **move the `viewport_effects` ahead of the `SetCell`**. Both
+tests fail — the reframe then overwrites the placement, which is the exact bug.
+That rival is the one worth keeping, because the naive reading of the acceptance
+criteria ("append `viewport_effects`") does not say *where*, and the wrong order
+is indistinguishable from the right one by inspection.
+
+`target_row` is `row_count()` — the appended row is last — unless the record
+already exists, in which case it is that record's own index, found by scanning
+`record_row(r).coords`. Predicting `row_count()` unconditionally would put the
+cursor out of range in that arm.
+
+### o_in_records_mode_opens_the_editor_on_an_empty_coordinate
+
+The same defect one step earlier, and the cheaper witness: after `o` alone, the
+`edit` buffer must be empty. Pre-filled with another row's coordinate is the
+state from which any subsequent keystroke corrupts. It is a separate test
+because it fails without needing the commit path to run, so a regression in
+placement is diagnosed without reading through `rename_record_coordinate`.
+
+Two things this fix does **not** close, deliberately:
+
+- **`o` twice with no page-axis category still adds one row.** Both presses
+  build `CellKey::new(vec![])` — the same record — and the second now says so
+  rather than claiming "Added new record row". Refusing outright was rejected:
+  `CommitAndAdvance`'s Tab-at-bottom-right path calls the same command and
+  relies on seeding an empty-coordinate row.
+- **1b proper.** `view_frame` hashes `entries.len()` and every entry, so *any*
+  append reframes and invalidates the cursor although no existing row was
+  renumbered. Placing the cursor masks it at this call site; distinguishing
+  "appended" from "renumbered" is a separate question and stays filed.
 ---
 
 ## The format a projected cell is drawn with (improvise-e0ex)

crates/improvise-command/src/command/cmd/grid.rs

@@ -513,9 +513,33 @@ impl Cmd for AddRecordRow {
             .filter(|(_, v)| !v.is_empty())
             .collect();
         let key = crate::model::cell::CellKey::new(coords);
-        vec![
-            Box::new(effect::SetCell(key, CellValue::Number(0.0))),
-            effect::set_status("Added new record row"),
-        ]
+        // The row this lands on is named, not inherited — improvise-o9uq, and
+        // the ORDER is the fix: `context/code/command.md`.
+        let existing = (0..ctx.layout.row_count()).find(|&r| {
+            ctx.layout
+                .record_row(r)
+                .is_some_and(|rec| rec.coords == key)
+        });
+        let mut effects: Vec<Box<dyn Effect>> = Vec::new();
+        match existing {
+            None => effects.push(Box::new(effect::SetCell(key, CellValue::Number(0.0)))),
+            Some(_) => effects.push(effect::set_status(
+                "That record row is already here — a row is its coordinates, so \
+                 fill this one in first",
+            )),
+        }
+        let target_row = existing.unwrap_or_else(|| ctx.layout.row_count());
+        effects.extend(super::navigation::viewport_effects(
+            target_row,
+            0,
+            ctx.row_offset,
+            ctx.col_offset,
+            ctx.visible_rows,
+            ctx.visible_cols,
+        ));
+        if existing.is_none() {
+            effects.push(effect::set_status("Added new record row"));
+        }
+        effects
     }
 }

crates/improvise-command/src/command/keymap.rs

@@ -33,9 +33,11 @@ fn run(
     ctx: &CmdContext,
     name: &str,
     args: &[String],
-) -> Option<Vec<Box<dyn Effect>>> {
-    let cmd = registry.interactive(name, args, ctx).ok()?;
-    Some(cmd.execute(ctx))
+) -> Result<Vec<Box<dyn Effect>>, String> {
+    let cmd = registry
+        .interactive(name, args, ctx)
+        .map_err(|e| format!("{name}: {e}"))?;
+    Ok(cmd.execute(ctx))
 }
 
 /// Turn a binding into effects. A `Sequence` **flattens** into one batch, and
@@ -47,17 +49,19 @@ fn effects_of(
     key: KeyCode,
     mods: KeyModifiers,
 ) -> Option<Vec<Box<dyn Effect>>> {
-    match binding {
+    // `None` means NO BINDING MATCHED, and nothing else: a bound key whose
+    // command will not build reports it — improvise-55l5, `context/code/command.md`.
+    let built = match binding {
         Binding::Cmd { name, args } => run(registry, ctx, name, args),
-        Binding::Prefix(_) => Some(vec![Box::new(SetTransientPrefix(key, mods))]),
-        Binding::Sequence(steps) => {
-            let mut effects: Vec<Box<dyn Effect>> = Vec::new();
-            for (name, args) in steps {
-                effects.extend(run(registry, ctx, name, args)?);
-            }
-            Some(effects)
-        }
-    }
+        Binding::Prefix(_) => Ok(vec![
+            Box::new(SetTransientPrefix(key, mods)) as Box<dyn Effect>
+        ]),
+        Binding::Sequence(steps) => steps.iter().try_fold(Vec::new(), |mut acc, (name, args)| {
+            acc.extend(run(registry, ctx, name, args)?);
+            Ok(acc)
+        }),
+    };
+    Some(built.unwrap_or_else(|e| vec![crate::ui::effect::set_status(e)]))
 }
 
 /// The shared keymap table plus the registry that gives its names meaning.
@@ -1187,3 +1191,73 @@ mod tests_support {
         keys
     }
 }
+
+#[cfg(test)]
+mod construction_failure_tests {
+    use super::*;
+    use crate::ui::app::App;
+    use crate::workbook::Workbook;
+    use improvise_protocol::key::KeyEvent;
+
+    fn app_with_no_cells() -> App {
+        let mut wb = Workbook::new("T");
+        wb.add_category("Region").unwrap();
+        wb.active_view_mut()
+            .set_axis("Region", crate::view::Axis::Row);
+        let mut app = App::new(wb, None);
+        app.rebuild_layout();
+        assert_eq!(
+            app.layout.cell_key(0, 0),
+            None,
+            "setup: an empty category on Row means no cell under the cursor"
+        );
+        app
+    }
+
+    /// improvise-55l5: evidence in `context/test-evidence/command.md`.
+    #[test]
+    fn a_bound_key_whose_command_cannot_be_built_says_so() {
+        let mut app = app_with_no_cells();
+        assert!(
+            KeymapSet::default_keymaps()
+                .maps()
+                .lookup(&ModeKey::Normal, KeyCode::Char('x'), KeyModifiers::NONE)
+                .is_some(),
+            "setup: x must be BOUND, or this tests an unbound key"
+        );
+
+        app.handle_key(KeyEvent::new(KeyCode::Char('x'), KeyModifiers::NONE))
+            .unwrap();
+
+        assert!(
+            app.view_state.status_msg.contains("no cell at cursor"),
+            "a bound key resolved to nothing and said nothing: {:?}",
+            app.view_state.status_msg
+        );
+    }
+
+    /// improvise-55l5: evidence in `context/test-evidence/command.md`.
+    #[test]
+    fn an_unbound_key_stays_silent() {
+        let mut app = app_with_no_cells();
+        assert!(
+            KeymapSet::default_keymaps()
+                .maps()
+                .lookup(
+                    &ModeKey::Normal,
+                    KeyCode::Char('\u{00e9}'),
+                    KeyModifiers::NONE
+                )
+                .is_none(),
+            "setup: this key must be UNBOUND, or the test proves nothing"
+        );
+
+        app.handle_key(KeyEvent::new(KeyCode::Char('\u{00e9}'), KeyModifiers::NONE))
+            .unwrap();
+
+        assert_eq!(
+            app.view_state.status_msg, "",
+            "an unbound key is not a failure and owes the user no message"
+        );
+    }
+}

crates/improvise-command/src/ui/app.rs

@@ -3523,6 +3523,88 @@ mod tests {
             app.view_state.mode
         );
     }
+
+    fn two_region_records() -> App {
+        use crate::model::cell::{CellKey, CellValue};
+        let mut wb = Workbook::new("T");
+        wb.add_category("Region").unwrap();
+        wb.add_item("Region", "East").unwrap();
+        wb.add_item("Region", "West").unwrap();
+        wb.add_item("_Measure", "Revenue").unwrap();
+        for (region, n) in [("East", 10.0), ("West", 20.0)] {
+            wb.model.set_cell(
+                CellKey::new(vec![
+                    ("Region".into(), region.into()),
+                    ("_Measure".into(), "Revenue".into()),
+                ]),
+                CellValue::Number(n),
+            );
+        }
+        let mut app = App::new(wb, None);
+        app.handle_key(KeyEvent::new(KeyCode::Char('R'), KeyModifiers::NONE))
+            .unwrap();
+        assert!(app.layout.is_records_mode(), "setup: records mode");
+        assert_eq!(app.layout.row_count(), 2, "setup: East and West");
+        app
+    }
+
+    fn region_revenue(app: &App, region: &str) -> Option<String> {
+        use crate::model::cell::CellKey;
+        app.model_state
+            .workbook
+            .model
+            .get_cell(&CellKey::new(vec![
+                ("Region".into(), region.into()),
+                ("_Measure".into(), "Revenue".into()),
+            ]))
+            .map(|v| v.to_string())
+    }
+
+    /// improvise-o9uq: evidence in `context/test-evidence/command.md`.
+    #[test]
+    fn o_in_records_mode_edits_the_row_it_added_and_leaves_the_others_alone() {
+        let mut app = two_region_records();
+        app.cursor_mut().selected = (1, 0);
+
+        app.handle_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::NONE))
+            .unwrap();
+        for c in "North".chars() {
+            app.handle_key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::NONE))
+                .unwrap();
+        }
+        app.handle_key(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE))
+            .unwrap();
+
+        assert_eq!(
+            region_revenue(&app, "East").as_deref(),
+            Some("10"),
+            "adding a row renamed a record the user never touched: {:?}",
+            (0..app.layout.row_count())
+                .map(|r| app.layout.row_label(r))
+                .collect::<Vec<_>>()
+        );
+        assert_eq!(
+            region_revenue(&app, "West").as_deref(),
+            Some("20"),
+            "the row the cursor was on when `o` was pressed also moved"
+        );
+    }
+
+    /// improvise-o9uq: evidence in `context/test-evidence/command.md`.
+    #[test]
+    fn o_in_records_mode_opens_the_editor_on_an_empty_coordinate() {
+        let mut app = two_region_records();
+        app.cursor_mut().selected = (1, 0);
+
+        app.handle_key(KeyEvent::new(KeyCode::Char('o'), KeyModifiers::NONE))
+            .unwrap();
+
+        assert_eq!(
+            app.view_state.buffers.get("edit").map(String::as_str),
+            Some(""),
+            "the editor opened pre-filled with another row's coordinate"
+        );
+    }
 }
 
 /// Behavioural coverage for the import wizard's key handling (improvise-2wm),