refactor: remove the last command-name literals from the registry (improvise-9cn)

Modelled the registry in Prolog — 85 registration sites against 80 Cmd::name
impls, facts extracted from source — and let it classify the drift risk.
70 sites were already safe: register/register_pure/register_nullary read the
name off a prototype instance, not a string. Only 8 used NamedCmd("literal")
as a stand-in prototype, and Prolog confirmed all 8 currently agree with what
their closures build, so there was no live bug.

Those 8 now take a real prototype, which removes the literal rather than
wrapping it in a macro; NamedCmd is deleted and registry.rs holds no command
name literal at all.

Prolog also surfaced a class the ticket never mentions: ViewNavigate,
TileAxisOp, ToggleGroupAtCursor and CommitAndAdvance name themselves off a
field, so a prototype can name the wrong variant and still typecheck. No
prototype scheme reaches that, which is what improvise-61f is for.

61f as specified is insufficient, and injecting the fault proved it: flipping
the ViewNavigate prototype to forward: false while its closure still builds
true does NOT produce a name mismatch. The entry collides with its sibling
under "view-back", every surviving entry still agrees with its own name, and
"view-forward" simply disappears. Uniqueness is what catches it.

Four guards, each verified by injecting the fault it targets: name roundtrip
over both construction paths (asserting no entry escapes both), name
uniqueness, every keymap-bound name resolving in the registry, and aliases
pointing at real commands.

Closes improvise-61f.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V69PGQfMJuGRmtSWpfVAqn
This commit is contained in:
Edward Langley
2026-08-29 12:10:52 -07:00
parent abbffcca8e
commit 7955027473
5 changed files with 289 additions and 51 deletions
+9 -8
View File
@@ -143,14 +143,15 @@ Region=West, Measure=Revenue = |pending| # pipe-quoted text
8. **Minibuffer keymaps come from `TextEntry`.** The seven text-entry modes are built by the `TextEntry` builder in `keymap.rs`, which binds the shared spine (AnyChar→`append-char <buf>`, Backspace→`pop-char <buf>`, Esc→`[clear-buffer <buf>, enter-mode <exit>]`) and welds every commit key to its `clear-buffer` via `.commit(key, cmd, args)` (improvise-w3q). `TextEntry::new` takes the `AppMode` and reads `<buf>` off its `MinibufferConfig`, so the buffer the prompt names and the buffer the keys edit are the same string by construction (improvise-k8h) — the keymap never spells a buffer name. `.rebind` exists only for CommandMode's Backspace. `SearchMode` is hand-written on purpose — no commit key, and `exit-search-mode` owns the query.
9. **Panel keymaps come from `PanelKeymap`.** The three panel modes share a spine (Esc/Tab→Normal, arrows and j/k→`move-panel-cursor <panel>`, every panel's toggle key, `:`→command mode) built by `PanelKeymap::new(panel)`; the keys that act on the panel's contents arrive through `.bind`/`.bind_args`/`.delete` because they vary in shape, not just command name (improvise-4pg). `Panel::ALL`/`name()`/`key()` in `ui/app.rs` drive the toggle keys for Normal and all three panels, so a fourth panel reaches every keymap from one `ALL` entry. `name()` must spell what `parse_panel` accepts.
10. **Keybindings are per-mode.** `ModeKey::from_app_mode()` resolves the mode; Normal + `search_mode=true` maps to `SearchMode`. Minibuffer modes bind Enter/Esc as `Binding::Sequence` so `clear-buffer` fires alongside commit/cancel.
11. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic.
12. **Float equality inside formula eval is currently mixed** — `=`/`!=` use a `1e-10` epsilon but the div-by-zero guard checks `rv == 0.0` exactly. Don't assume one or the other; tracked for cleanup.
13. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this.
14. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior.
15. **Minibuffer commits go through one command.** `commit-buffer <formula|category|item>` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line.
16. **A failed `Save` aborts the batch.** The `Save` effect sets `abort_effects` when there's no file path or the write fails (improvise-8tk), so anything chained after it — the `Quit` in `wq`/`ZZ` — is skipped and the error stays on screen. A `Binding::Sequence` flattens into one effect vec, so the abort carries across sequence steps too.
17. **The import wizard's back path is a history stack, not a reverse table.** `advance` pushes the step it leaves onto `ImportWizard::history`; `previous` pops it, so back always retraces the path actually taken through the conditional transitions (improvise-907). Preconditions live in `validation_error()`, checked on the step where the user can still fix them — never deferred to `build_model`. `NameModel` is terminal for `advance`: Enter there calls `build()`, which parks the workbook in `built` on `Done`, and `AcceptWizardModel` takes it from there. Going back from `Done` discards `built`.
18. **`app.layout` goes stale mid-batch.** `apply_effects` rebuilds only *after* the whole batch, so an effect that reads the layout after an earlier effect changed the grid needs a `RebuildLayout` effect in front of it — the command decides the order (see `EnterEditAtCursorCmd`, `CommitAndAdvance`). Read the cursor's cell text via `App::display_value_at_cursor()`; don't build a `CmdContext` inside `apply` (it recomputes column widths).
11. **Registry names come from prototypes, not strings.** Every `register*` call reads the name off a prototype `Cmd` instance, so a typo is impossible (improvise-9cn) — there is no name literal in `registry.rs`. What types can't reach: four commands name themselves off a field (`ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`, `CommitAndAdvance`), so a prototype can name the wrong *variant* and still compile. That shows up as a **duplicate** registration, not a mismatch — the wrong-variant entry collides with its sibling and the sibling's name vanishes. `registered_names_are_unique` catches it; `every_bound_command_name_is_registered` catches the keymap side.
12. **`effect_cmd!` macro** wraps a pre-built effect as a parseable command. Use it for simple wrappers; reach for a hand-written `Cmd` once there's real decision logic.
13. **Float equality inside formula eval is currently mixed** — `=`/`!=` use a `1e-10` epsilon but the div-by-zero guard checks `rv == 0.0` exactly. Don't assume one or the other; tracked for cleanup.
14. **`IndexMap`** preserves insertion order for categories, views, and items. Persistence relies on this.
15. **Commit paths for cell edits** split across `commit_cell_value` → `commit_regular_cell_value` / `commit_plain_records_edit` in `command/cmd/commit.rs`. Keep the synthetic-records branch in sync with the plain branch when changing behavior.
16. **Minibuffer commits go through one command.** `commit-buffer <formula|category|item>` (`CommitBuffer { kind: CommitKind }`) owns buffer→trim→bail-on-empty→dirty→status for all three; the varying parts are methods on `CommitKind`. The add effect is emitted **last** so its failure message wins over the command's optimistic status (improvise-6t3). `CommitKind::Item` carries its target category, resolved from `AppMode::ItemAdd` by `parse_commit_kind` at registry time — so `commit-buffer item` is not constructible from a script line.
17. **A failed `Save` aborts the batch.** The `Save` effect sets `abort_effects` when there's no file path or the write fails (improvise-8tk), so anything chained after it — the `Quit` in `wq`/`ZZ` — is skipped and the error stays on screen. A `Binding::Sequence` flattens into one effect vec, so the abort carries across sequence steps too.
18. **The import wizard's back path is a history stack, not a reverse table.** `advance` pushes the step it leaves onto `ImportWizard::history`; `previous` pops it, so back always retraces the path actually taken through the conditional transitions (improvise-907). Preconditions live in `validation_error()`, checked on the step where the user can still fix them — never deferred to `build_model`. `NameModel` is terminal for `advance`: Enter there calls `build()`, which parks the workbook in `built` on `Done`, and `AcceptWizardModel` takes it from there. Going back from `Done` discards `built`.
19. **`app.layout` goes stale mid-batch.** `apply_effects` rebuilds only *after* the whole batch, so an effect that reads the layout after an earlier effect changed the grid needs a `RebuildLayout` effect in front of it — the command decides the order (see `EnterEditAtCursorCmd`, `CommitAndAdvance`). Read the cursor's cell text via `App::display_value_at_cursor()`; don't build a `CmdContext` inside `apply` (it recomputes column widths).
---
@@ -237,19 +237,6 @@ impl CmdRegistry {
}
}
/// Dummy prototype used only for name extraction in registry calls
/// where the real command struct is built by a closure.
#[derive(Debug)]
pub(super) struct NamedCmd(pub(super) &'static str);
impl Cmd for NamedCmd {
fn name(&self) -> &'static str {
self.0
}
fn execute(&self, _: &CmdContext) -> Vec<Box<dyn Effect>> {
vec![]
}
}
pub(super) fn require_args(word: &str, args: &[String], n: usize) -> Result<(), String> {
if args.len() < n {
Err(format!(
@@ -288,16 +288,26 @@ pub fn default_registry() -> CmdRegistry {
r.register_nullary(|| Box::new(SaveAndQuit));
r.register_nullary(|| Box::new(SaveCmd));
r.register_nullary(|| Box::new(EnterSearchMode));
r.register_pure(&NamedCmd("edit-or-drill"), |args| {
require_args("edit-or-drill", args, 1)?;
let edit_mode = parse_mode_name(&args[0])?;
Ok(Box::new(EditOrDrill { edit_mode }))
});
r.register_pure(&NamedCmd("enter-edit-at-cursor"), |args| {
require_args("enter-edit-at-cursor", args, 1)?;
let target_mode = parse_mode_name(&args[0])?;
Ok(Box::new(EnterEditAtCursorCmd { target_mode }))
});
r.register_pure(
&EditOrDrill {
edit_mode: AppMode::Normal,
},
|args| {
require_args("edit-or-drill", args, 1)?;
let edit_mode = parse_mode_name(&args[0])?;
Ok(Box::new(EditOrDrill { edit_mode }))
},
);
r.register_pure(
&EnterEditAtCursorCmd {
target_mode: AppMode::Normal,
},
|args| {
require_args("enter-edit-at-cursor", args, 1)?;
let target_mode = parse_mode_name(&args[0])?;
Ok(Box::new(EnterEditAtCursorCmd { target_mode }))
},
);
r.register_nullary(|| Box::new(EnterExportPrompt));
r.register_nullary(|| Box::new(EnterFormulaEdit));
r.register_nullary(|| Box::new(EnterTileSelect));
@@ -324,13 +334,13 @@ pub fn default_registry() -> CmdRegistry {
|_| Ok(Box::new(ViewNavigate { forward: true })),
|_, _| Ok(Box::new(ViewNavigate { forward: true })),
);
r.register_pure(&NamedCmd("enter-mode"), |args| {
r.register_pure(&EnterMode(AppMode::Normal), |args| {
require_args("enter-mode", args, 1)?;
Ok(Box::new(EnterMode(parse_mode_name(&args[0])?)))
});
// ── Search ───────────────────────────────────────────────────────────
r.register_pure(&NamedCmd("search-navigate"), |args| {
r.register_pure(&SearchNavigate(true), |args| {
let forward = args.first().map(|s| s != "backward").unwrap_or(true);
Ok(Box::new(SearchNavigate(forward)))
});
@@ -475,17 +485,22 @@ pub fn default_registry() -> CmdRegistry {
r.register_nullary(|| Box::new(DeleteViewAtCursor));
// ── Tile select ──────────────────────────────────────────────────────
r.register_pure(&NamedCmd("move-tile-cursor"), |args| {
r.register_pure(&MoveTileCursor(0), |args| {
require_args("move-tile-cursor", args, 1)?;
let delta = args[0].parse::<i32>().map_err(|e| e.to_string())?;
Ok(Box::new(MoveTileCursor(delta)))
});
r.register_nullary(|| Box::new(TileAxisOp { axis: None }));
r.register_pure(&NamedCmd("set-axis-for-tile"), |args| {
require_args("set-axis-for-tile", args, 1)?;
let axis = parse_axis(&args[0])?;
Ok(Box::new(TileAxisOp { axis: Some(axis) }))
});
r.register_pure(
&TileAxisOp {
axis: Some(crate::view::Axis::Row),
},
|args| {
require_args("set-axis-for-tile", args, 1)?;
let axis = parse_axis(&args[0])?;
Ok(Box::new(TileAxisOp { axis: Some(axis) }))
},
);
// ── Grid operations ──────────────────────────────────────────────────
r.register_nullary(|| Box::new(ToggleGroupAtCursor { is_row: true }));
@@ -497,18 +512,28 @@ pub fn default_registry() -> CmdRegistry {
r.register_nullary(|| Box::new(HideSelectedRowItem));
// ── Text buffer ──────────────────────────────────────────────────────
r.register_pure(&NamedCmd("append-char"), |args| {
require_args("append-char", args, 1)?;
Ok(Box::new(AppendChar {
buffer: args[0].clone(),
}))
});
r.register_pure(&NamedCmd("pop-char"), |args| {
require_args("pop-char", args, 1)?;
Ok(Box::new(PopChar {
buffer: args[0].clone(),
}))
});
r.register_pure(
&AppendChar {
buffer: String::new(),
},
|args| {
require_args("append-char", args, 1)?;
Ok(Box::new(AppendChar {
buffer: args[0].clone(),
}))
},
);
r.register_pure(
&PopChar {
buffer: String::new(),
},
|args| {
require_args("pop-char", args, 1)?;
Ok(Box::new(PopChar {
buffer: args[0].clone(),
}))
},
);
r.register_nullary(|| Box::new(CommandModeBackspace));
// ── Commit ───────────────────────────────────────────────────────────
@@ -602,3 +627,134 @@ pub fn default_registry() -> CmdRegistry {
r
}
#[cfg(test)]
mod tests {
use super::*;
use crate::command::cmd::test_helpers::*;
/// Argument vectors to try when constructing a command, most specific
/// first. A command is exercised by the first one its parser accepts.
fn candidate_args() -> Vec<Vec<String>> {
[
vec![],
vec!["1"],
vec!["-1"],
vec!["right"],
vec!["normal"],
vec!["editing"],
vec!["row"],
vec!["formula"],
vec!["backward"],
vec!["a"],
vec!["Region"],
vec!["Region", "East"],
vec!["Region", "East", "1"],
vec!["Profit = 1"],
vec![",.2f"],
vec!["/tmp/improvise-registry-test.improv"],
vec!["1", "1"],
vec!["formula", "1"],
vec!["editing", "5", "Region", "East"],
]
.into_iter()
.map(|v| v.into_iter().map(String::from).collect())
.collect()
}
/// improvise-9cn / improvise-61f: every registration derives its name
/// from a prototype instance, so a typo in the name is impossible. What
/// the types do *not* catch is the commands whose `name()` branches on a
/// field — `ViewNavigate`, `TileAxisOp`, `ToggleGroupAtCursor`,
/// `CommitAndAdvance`. Their prototype and the value their constructor
/// builds are the same type, so registering `ViewNavigate { forward:
/// true }` while building `forward: false` compiles cleanly and files
/// the command under the wrong name, where dispatch silently misses.
/// Constructing every entry both ways and comparing names is the only
/// check that reaches that class.
///
/// Some entries are deliberately one-path — `commit-and-advance-right`
/// reads the edit buffer and cursor, so its parse fn always errors — but
/// every entry must be reachable by at least one path, or its name goes
/// unchecked.
#[test]
fn every_entry_constructs_a_command_with_its_registered_name() {
let r = default_registry();
let wb = two_cat_model();
let layout = make_layout(&wb);
let reg = make_registry();
let ctx = make_ctx(&wb, &layout, &reg);
let mut unexercised: Vec<&'static str> = Vec::new();
for name in r.names().collect::<Vec<_>>() {
let parsed = candidate_args()
.iter()
.find_map(|args| r.parse(name, args).ok());
let interactive = candidate_args()
.iter()
.find_map(|args| r.interactive(name, args, &ctx).ok());
for (path, cmd) in [("parse", &parsed), ("interactive", &interactive)] {
if let Some(cmd) = cmd {
assert_eq!(
cmd.name(),
name,
"registered as \"{name}\" but {path} builds \"{}\"",
cmd.name()
);
}
}
if parsed.is_none() && interactive.is_none() {
unexercised.push(name);
}
}
assert!(
unexercised.is_empty(),
"no candidate args construct these by either path, \
so their names go unchecked: {unexercised:?}"
);
}
/// A variant mix-up does not show up as a name mismatch — it shows up as
/// a collision. Registering `ViewNavigate { forward: false }` where
/// `forward: true` belongs files a second entry under "view-back" and
/// removes "view-forward" from the registry entirely; `parse` takes the
/// first match, so every surviving entry still agrees with its own name.
/// Uniqueness is what catches it.
#[test]
fn registered_names_are_unique() {
let r = default_registry();
let mut seen: Vec<&'static str> = Vec::new();
let mut dupes: Vec<&'static str> = Vec::new();
for name in r.names() {
if seen.contains(&name) {
dupes.push(name);
}
seen.push(name);
}
assert!(
dupes.is_empty(),
"registered twice — one prototype names the wrong variant: {dupes:?}"
);
}
/// Aliases must resolve to a command that actually exists.
#[test]
fn every_alias_resolves_to_a_registered_command() {
let r = default_registry();
let names: Vec<&str> = r.names().collect();
for (alias, canonical) in [
("add-cat", "add-category"),
("formula", "add-formula"),
("add-view", "create-view"),
("q!", "force-quit"),
] {
assert!(
names.contains(&canonical),
"alias \"{alias}\" points at unregistered \"{canonical}\""
);
}
}
}
@@ -1161,6 +1161,75 @@ mod tests {
assert!(search.lookup(KeyCode::Esc, KeyModifiers::NONE).is_some());
}
/// improvise-9cn: `Keymap::dispatch` resolves a command name through the
/// registry with `.ok()?`, so a binding naming a command that is not
/// registered is a silent no-op key. This walks every binding in every
/// mode and requires the name to resolve — which also catches a registry
/// entry disappearing (a mis-variant prototype colliding with its
/// sibling) while a keymap still binds the vanished name.
#[test]
fn every_bound_command_name_is_registered() {
let ks = KeymapSet::default_keymaps();
let registered: Vec<&str> = ks.registry().names().collect();
let mut checked = 0usize;
let require = |name: &str, mode: &ModeKey| {
assert!(
registered.contains(&name),
"{mode:?} binds \"{name}\", which no registry entry provides"
);
};
for (mode, km) in &ks.mode_maps {
for key in probe_keys() {
for mods in [
KeyModifiers::NONE,
KeyModifiers::SHIFT,
KeyModifiers::CONTROL,
KeyModifiers::ALT,
] {
match km.lookup(key, mods) {
Some(Binding::Cmd { name, .. }) => {
require(name, mode);
checked += 1;
}
Some(Binding::Sequence(steps)) => {
for (name, _) in steps {
require(name, mode);
checked += 1;
}
}
Some(Binding::Prefix(_)) | None => {}
}
}
}
}
assert!(checked > 100, "expected to walk the keymaps, saw {checked}");
}
/// The key space the binding walks cover: every printable ASCII plus the
/// named keys the keymaps actually use.
fn probe_keys() -> Vec<KeyCode> {
let mut keys: Vec<KeyCode> = (0x20u8..0x7f).map(|c| KeyCode::Char(c as char)).collect();
keys.extend([
KeyCode::Esc,
KeyCode::Enter,
KeyCode::Up,
KeyCode::Down,
KeyCode::Left,
KeyCode::Right,
KeyCode::Tab,
KeyCode::BackTab,
KeyCode::Backspace,
KeyCode::Delete,
KeyCode::Home,
KeyCode::End,
KeyCode::PageUp,
KeyCode::PageDown,
]);
keys
}
/// The command and args `mode`'s keymap binds `key` to.
fn bound(ks: &KeymapSet, mode: &ModeKey, key: KeyCode) -> (String, Vec<String>) {
let km = ks.mode_maps.get(mode).expect("every mode has a keymap");
+25
View File
@@ -113,3 +113,28 @@ invariant(panel_keymap_spine,'the three panel modes share a spine built by Panel
invariant(panel_name_matches_parser,'Panel::name() must spell what parse_panel in cmd/core.rs accepts; pinned by panel_toggle_keys_dispatch_to_real_effects, since dispatch swallows an unresolvable command with .ok()?').
doctrine(binding_dump_diff,'when refactoring keymap construction, dump every (mode,key,modifier) lookup for all 16 modes before and after and diff: most bindings have no direct test, so a green suite is not evidence').
location(panel_keymap_builder,'crates/improvise-command/src/command/keymap.rs','PanelKeymap, bind_panel_toggles').
% ── Command-registry name-drift model (improvise-9cn / improvise-61f) ───────
% How a registration gets the name it files a command under.
name_source_kind(prototype_instance, safe, 'Cmd::name() is read off a real instance; the compiler fixes the type').
name_source_kind(literal_string, unsafe, 'a hand-written string with nothing tying it to the type the closure builds').
% A command whose name() branches on a field: prototype and constructed value
% share a TYPE but may differ in VARIANT, which no type check reaches.
variant_named_cmd('ViewNavigate', forward).
variant_named_cmd('TileAxisOp', axis).
variant_named_cmd('ToggleGroupAtCursor', is_row).
variant_named_cmd('CommitAndAdvance', advance).
% What each defence actually reaches.
catches(prototype_instance, typo_in_name).
catches(uniqueness_test, wrong_variant_prototype).
catches(name_roundtrip_test, constructor_builds_other_name).
catches(bound_names_test, keymap_references_missing_command).
% A wrong-variant prototype does NOT surface as a name mismatch: it collides
% with its sibling, so every surviving entry still agrees with its own name
% and the sibling's name disappears. Uniqueness is the check that reaches it.
insufficient_for(name_roundtrip_test, wrong_variant_prototype).
needs_runtime_check(T) :- variant_named_cmd(T, _).