From c13466f1d9dad2cda8a516b82fa71cefc52f061a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 08:18:17 -0700 Subject: [PATCH 1/4] fix(cli): the headless commands were silent whether they worked or not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-q914 and improvise-j6x8 are one bug seen twice, and the second measured it: `run_headless_commands` applied effects and never read `app.view_state.status_msg`, setting `exit_code` only for parse errors. A successful command and a refused one therefore produced byte-identical output — nothing — and exit 0 for both. `improvise script` is the same function, so it shared the hole exactly. Three decisions, argued in `context/code/cli.md`: **Everything to stderr.** The tempting split — stdout for a report, stderr for a refusal — cannot be built honestly, because `status_msg` is a bare `String` with no severity and both candidate classifiers are disproved by code already in the tree. "A refusal aborted" is false: `AddItem` writes its `Err` to the status and does not abort, and that is q914's own example. "A refusal is anything that spoke" is false too: `cmd/fit.rs` reports the seed that reproduces a fit on **success**. Guessing between them is the adjacent check design-principles §7 names. **Exit 1 on a parse error or an aborted batch, and no further.** Both are real flags. A refusal that only writes a status is now printed but still exits 0, because nothing can classify it; the README says so plainly rather than over-promising, and closing it needs a severity on `ViewState`, which is a wire change reaching the browser. **No quiet flag.** Silence is never the default and `2>/dev/null` already exists. Also: a run that changed nothing no longer rewrites the document. Saving unconditionally normalised it — `1e20` to its twenty digits — which turns a hand-edited fixture into a silent no-op. The gate accumulates `model_state.dirty` per command rather than reading it once at the end, because `effect::Save` clears the flag and `[add-item, w elsewhere]` would otherwise drop the edit. And the three clap doc strings advertised JSON, which the parser refuses. `tests/headless_cli.rs` pins all of it at the process boundary, where an exit code and a stream exist. Six of its twelve tests fail at bf83294. The rival worth injecting — print on the happy path, stay silent on the refusal — fails five of them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- README.md | 24 ++++ context/code/cli.md | 116 +++++++++++++++ context/test-evidence/cli.md | 137 ++++++++++++++++++ src/main.rs | 30 +++- tests/headless_cli.rs | 268 +++++++++++++++++++++++++++++++++++ 5 files changed, 569 insertions(+), 6 deletions(-) create mode 100644 context/code/cli.md create mode 100644 context/test-evidence/cli.md create mode 100644 tests/headless_cli.rs diff --git a/README.md b/README.md index ef395bfe..2fd11560 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,30 @@ improvise cmd 'add-cat Region' 'add-item Region East' -f model.improv improvise script setup.txt -f model.improv ``` +The language is the `:` command line, exactly as typed in the TUI — not JSON. +Arguments split on whitespace, so anything containing a space is quoted: + +```sh +improvise cmd 'formula Product "Total = Widget + Gadget"' -f model.improv +``` + +### What the headless commands promise + +- **Everything a command reports goes to stderr**, as it would appear on the + TUI's status line, and stdout is left empty. A run that had nothing to report + prints nothing, so `2>/dev/null` is the whole suppression story and there is + no quiet flag. +- **Exit 1** means a line did not parse, or an effect aborted the rest of its + batch — a failed `:w`, say. **Exit 0 means neither of those happened**, and + it does *not* promise every command did what it was asked: a refusal that + only writes a status message (`add-item` naming a category that does not + exist) is printed but still exits 0, because a status message carries no + severity for the exit code to read. Check stderr as well as `$?` when it + matters. +- **A run that changed nothing does not rewrite the document.** Writing it back + regardless silently normalises it — `1e20` becomes its twenty digits — which + turns a hand-edited fixture into a no-op on the next run. + ## In the browser The browser page is at ****, diff --git a/context/code/cli.md b/context/code/cli.md new file mode 100644 index 00000000..67063a98 --- /dev/null +++ b/context/code/cli.md @@ -0,0 +1,116 @@ +# The headless CLI + +`improvise cmd` and `improvise script` are one function — `run_headless_commands` +in `src/main.rs`; `run_headless_script` reads the file and delegates. Every claim +below therefore holds for both, and a fix to one is a fix to both by +construction rather than by remembering. + +## 1 — the status line is the only channel, so it must be printed + +`App` has no return channel for "what happened". A command's outcome — a +refusal, a warning, a `fit-measure`'s reproducing seed — is written to +`ViewState::status_msg` and rendered by the TUI's bottom bar. Headless, nothing +read it, so **success and refusal were byte-identical: silence and exit 0** +(improvise-q914, improvise-j6x8). Two things followed that are worth keeping +in mind, because both were mistaken for correct behaviour by agents working +above this layer: + +- A P1's carefully-worded refusal can be written, tested at the effect, and + still reach nobody, because the test asserts the *effect's* status and the + surface that would show it drops it. That is this repo's characteristic + defect one layer out, and the only cure is a test at the process boundary — + `tests/headless_cli.rs`, which spawns the real binary. +- `improvise cmd 'fit-measure …'` looked like it did nothing whether it worked + or not, which made the whole GP feature read as broken from a script. + +The loop takes the message with `std::mem::take` and clears it per command, so +one command's status cannot be reported twice or attributed to its successor. + +## 2 — stderr, all of it, and why no split was possible + +The tempting design is stdout for a report and stderr for a refusal. **It +cannot be built honestly**, because `status_msg` is a bare `String` with no +severity, and both candidate classifiers are disproved by code in the tree: + +- *"A refusal is a batch that aborted."* Only three effects set + `abort_effects` (gotcha 21). `AddItem` writes `workbook.add_item`'s `Err` + straight to the status and does not abort — and that is q914's own worked + example of a refusal. So this classifier puts a refusal on stdout labelled a + success. +- *"A refusal is anything that spoke."* `cmd/fit.rs` sets a status on + **success**, carrying the seed that reproduces the fit. So this one labels a + deliverable a failure. + +Guessing between them would be precisely the adjacent-check defect +`context/design-principles.md` §7 names. So everything goes to stderr: it is +the diagnostics stream, it leaves stdout free for a future data output, and it +matches every other diagnostic `main.rs` already emits (`Saved to …`, +`warning: …`, `Parse error: …`). + +**No `--quiet` flag.** Silence is never the default and `2>/dev/null` is the +standard suppression; a flag would be a second knob with nothing asking for it. + +## 3 — the exit code, and the hole left open deliberately + +Exit 1 on a parse error (pre-existing) or on `App::last_batch_aborted()`. Both +are real flags read off state the code already keeps, so neither is a guess. + +What this does **not** cover, stated rather than papered over: a refusal that +only writes a status still exits 0. Closing it needs a severity beside +`status_msg`, which lives in `ViewState` and is therefore a wire change +(gotcha 25) reaching the browser client — out of proportion to the CLI, and +filed separately. Until then the README says plainly that exit 0 means "nothing +aborted", not "everything worked", because a contract that over-promises is +worse than a narrow one. + +## 4 — a run that changed nothing does not write + +`persistence::save` used to run unconditionally, so `improvise cmd` normalised +the document on every invocation — `1e20` expanded to twenty digits. The cost +was not cosmetic: an agent editing a fixture by string-replacing a value after +one run got a silent no-op, and two misleading test results out of it. + +The gate is `changed`, accumulated from `model_state.dirty` **after each +command**, not read once at the end. Reading it at the end is wrong because +`effect::Save` clears the flag, so `[add-item …, w other.improv]` would look +unchanged and drop the edit from the `-f` document. `!path.exists()` is or-ed +in so that naming a new file still creates it. + +Over-approximation is the safe direction here, the same one `Dirty::of` takes +(gotcha 35): a `Model(_)` effect that happens to write the value already there +still costs one redundant save. + +## 5 — the refusal names the word the user typed + +`CmdRegistry::resolve` maps `formula` → `add-formula` *before* the arity check +runs, so every validator that spelled its own name reported a command nobody +typed (improvise-z05h). The fix moves the name to the one place that knows +which of the two the user wrote: `require_args` no longer takes a `word`, and +`CmdRegistry::parse` prefixes `"{typed}: "` onto any `Err`. + +That removed 41 name literals from `cmd/`, which is the same argument gotcha 15 +makes about `registry.rs` having no name literals — a name spelled twice can be +spelled wrong once. `interactive` deliberately does **not** prefix: `dispatch` +in `command/keymap.rs` already formats `"{name}: {e}"` with the *bound* name +(improvise-55l5, PR #126), and prefixing in both would double it. + +The message states the **shape** rather than the arity, because +"requires 1-2 argument(s), got 6" does not tell anyone to quote anything. + +## 6 — the help's ex-commands were the unguarded surface + +`help.rs` derives `HelpLine::Keys` rows from the keymap and drops any row whose +command does not resolve, so a key row cannot go stale. `HelpLine::Literal` and +`HelpLine::Accent` rows are hand-written prose and had nothing checking them — +which is how all three documented spellings of `:formula` came to be ones the +parser refuses. + +`every_ex_command_the_help_pages_show_is_one_the_parser_accepts` closes that +surface rather than the one instance: every help line whose trimmed text opens +with `:` is split on its colons and each piece handed to `parse_line`. The +split exists because one row shows two examples side by side. + +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. diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md new file mode 100644 index 00000000..0e965a1b --- /dev/null +++ b/context/test-evidence/cli.md @@ -0,0 +1,137 @@ +# Test evidence — the headless CLI (`src/main.rs`, `tests/headless_cli.rs`) + +Keyed by exact test name, per design-principles §7. A renamed test orphans its +entry; `python3 scripts/check-comment-runs.py --check-evidence` catches a +heading that names no test. + +--- + +## `tests/headless_cli.rs` — the contract at the process boundary + +improvise-q914 / improvise-j6x8 / improvise-z05h. The reasoning is +`context/code/cli.md`; this file records what broke and what each test can see. + +**Why a spawned binary rather than a unit test.** The three things under test +are an exit code, a choice of stream, and whether a file was written — none of +which exists below `std::process::exit`. Per design-principles §7 this is a +boundary contract, so it is pinned once, slowly and honestly, and the +`command/cmd/` unit tests assume it. Measured on the orchestrator's machine the +whole binary runs in ~0.5s, twelve process spawns included, which is well +inside the per-binary budget. + +**What was broken.** `run_headless_commands` applied effects and never read +`app.view_state.status_msg`, and set `exit_code` only for parse errors. So a +successful command and a refused one produced byte-identical output — nothing — +and exit 0 for both. Measured at `bf83294` before the fix: six of the twelve +tests below failed. + +### a_refused_command_says_what_went_wrong + +`add-item Nonexistent Thing` against a document that has no such category. +`Workbook::add_item`'s `Err` reaches `status_msg` and the effect does **not** +set `abort_effects`, which is what makes this the load-bearing case: it is a +genuine refusal that no flag in the codebase classifies as one. + +Asserts the category's name appears, not merely that something was printed — +"something was printed" is satisfied by a fixed `ok` on every command. + +### a_command_with_nothing_to_say_says_nothing + +**The rival this exists to kill**, and it is the one the brief named: a fix +that prints on the happy path and stays silent on the refusal. Injected as +`if said.trim().is_empty() { eprintln!("ok") }` — the exact inversion — and it +fails this test *and* `a_refused_command_says_what_went_wrong`, five tests in +all. A test that only asked "was anything printed" passes that rival. + +`add-category Region` is the input because it sets no status at all, so the +assertion is about the writer rather than about this command's vocabulary. + +### an_aborted_batch_exits_non_zero_and_says_why + +`:w` into a directory that does not exist. `effect::Save` reports through +`report_file_write`, which sets the status **and** `abort_effects` (gotcha 21), +so this is the one arm the exit code can honestly read. Before the fix this +exited 0 with no output, which is what made `improvise cmd … && next-step` +proceed over a failed write. + +### a_command_that_succeeds_while_reporting_still_exits_zero + +**The rival this exists to kill**: making any emitted status a non-zero exit, +which is the obvious way to widen the previous test and is wrong. `effect::Save` +on **success** also writes a status, as does `cmd/fit.rs` — whose status carries +the seed that reproduces the fit. So "it spoke" cannot mean "it failed". + +The first assertion is a check-the-checker: if the save ever stops reporting, +the test proves nothing and says so rather than passing quietly. + +### a_parse_error_still_exits_non_zero + +The pre-existing arm, kept so the widening cannot silently drop it. Also +asserts the message names the word typed, which the `Unknown command` arm +reports post-alias-resolution. + +### what_a_command_says_goes_to_stderr_and_stdout_stays_empty + +Pins the stream choice itself. Both halves are asserted: a rival that writes to +both streams passes an stderr-only check. + +### a_script_reports_exactly_as_cmd_does + +`improvise script` is the same function, so this cannot fail while `cmd` +passes — which is the point. It is here because the ticket asked whether the +hole was shared, and a claim that two paths are one is worth one cheap +assertion rather than a sentence. The script also carries a `#` comment, so the +comment-skipping arm is exercised on the way through. + +### a_run_that_changes_nothing_does_not_rewrite_the_document + +`move-cursor down` is `ViewOnly`, so nothing is dirty. **This test passed +before the fix** — a freshly written document re-serialises byte-identically — +and it is kept as the control that says so. The discriminating one is below. + +### a_no_op_run_leaves_an_unnormalised_literal_alone + +The reported instance. A hand-written document carrying `1e20` is rewritten as +`100000000000000000000` by a run asked to change nothing, because +`persistence::save` ran unconditionally. It fails before the fix where its +sibling does not, which is exactly why both are here: the sibling cannot see a +normalisation, only a reordering. + +The first assertion checks the fixture still contains `1e20` before the run, so +a future grammar change that rejects the literal fails loudly instead of making +the test vacuous. + +### a_run_that_changed_the_document_still_writes_it + +The regression the dirty-gate could have introduced. `effect::Save` sets +`dirty = false`, so a gate reading the flag once at the end would see +`[add-item East, w other.improv]` as unchanged and silently drop the edit from +the `-f` document. Accumulating `changed` per command is what this asserts; a +gate written the obvious way fails it. + +### the_cmd_help_does_not_advertise_a_language_it_refuses + +Three clap doc strings said JSON three times over. Checks both subcommands, +because `script`'s `` help carried the same claim and a fix to `cmd` +alone reads as complete. + +### the_language_the_help_names_is_the_one_it_accepts + +The other direction, and the reason the test above is not merely a spell check: +it demonstrates the `:` command line works end to end *and* that the JSON form +the help advertised is genuinely refused. Without this half, deleting the word +"JSON" from the help would satisfy the check while the CLI still accepted +neither language. + +#### Scratch (helper) + +A per-test temp directory named from the test's own tag and the process id, so +the twelve tests cannot collide, with a `Drop` that removes it. Not +`tempfile::TempDir` only because the fixture also has to be readable by a +spawned child at a stable path. + +#### seeded (helper) + +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. diff --git a/src/main.rs b/src/main.rs index 0ac5405d..62499e61 100644 --- a/src/main.rs +++ b/src/main.rs @@ -43,7 +43,7 @@ trait Runnable { enum Commands { /// Import JSON or CSV data, then open TUI (or save with --output) Import(ImportArgs), - /// Run a JSON command headless (repeatable) + /// Run a command line headless, as typed after `:` (repeatable) Cmd(CmdArgs), /// Run commands from a script file headless Script(ScriptArgs), @@ -99,8 +99,9 @@ struct ImportArgs { #[derive(clap::Args)] struct CmdArgs { - /// JSON command strings - json: Vec, + /// Command lines, in the `:` command language (e.g. `add-cat Region`) + #[arg(value_name = "COMMAND")] + commands: Vec, /// Model file to load/save #[arg(short, long)] @@ -109,7 +110,7 @@ struct CmdArgs { #[derive(clap::Args)] struct ScriptArgs { - /// Script file (one JSON command per line, # comments) + /// Script file (one command line per line, `#` comments) path: PathBuf, /// Model file to load/save @@ -155,7 +156,7 @@ impl Runnable for ImportArgs { impl Runnable for CmdArgs { fn run(self, _model_file: Option) -> Result<()> { - run_headless_commands(&self.json, &self.file) + run_headless_commands(&self.commands, &self.file) } } @@ -338,6 +339,9 @@ fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> app.term_height = h; } let mut exit_code = 0; + // `Save` clears `dirty`, so a run that edits and then `:w`s elsewhere would + // otherwise look unchanged — `context/code/cli.md`. + let mut changed = false; for line in cmds { match command::parse_line(line) { @@ -348,6 +352,16 @@ fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> 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; + } } } Err(e) => { @@ -357,7 +371,11 @@ fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> } } - if let Some(path) = file { + // Writing back a document no command changed normalises it silently, which + // turns a hand-edited fixture into a no-op — `context/code/cli.md`. + if let Some(path) = file + && (changed || !path.exists()) + { persistence::save(&app.model_state.workbook, path)?; } diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs new file mode 100644 index 00000000..bed6d1b4 --- /dev/null +++ b/tests/headless_cli.rs @@ -0,0 +1,268 @@ +//! The `improvise cmd` / `improvise script` contract, pinned at the process +//! boundary. Why a spawned binary: `context/test-evidence/cli.md`. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +fn bin() -> &'static str { + env!("CARGO_BIN_EXE_improvise") +} + +struct Scratch(PathBuf); + +impl Scratch { + fn new(tag: &str) -> Self { + let dir = std::env::temp_dir().join(format!("improvise-cli-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("a scratch directory"); + Self(dir) + } + + fn path(&self, name: &str) -> PathBuf { + self.0.join(name) + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn run_cmd(args: &[&str], file: &Path) -> Output { + Command::new(bin()) + .arg("cmd") + .args(args) + .arg("-f") + .arg(file) + .output() + .expect("the improvise binary runs") +} + +fn stderr_of(out: &Output) -> String { + String::from_utf8_lossy(&out.stderr).to_string() +} + +fn stdout_of(out: &Output) -> String { + String::from_utf8_lossy(&out.stdout).to_string() +} + +/// A document with one category, so `add-item` has somewhere legitimate to go. +fn seeded(scratch: &Scratch, name: &str) -> PathBuf { + let doc = scratch.path(name); + let out = run_cmd(&["add-category Region"], &doc); + assert!(doc.exists(), "the seed run wrote a document: {out:?}"); + doc +} + +#[test] +fn a_refused_command_says_what_went_wrong() { + let scratch = Scratch::new("refused"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["add-item Nonexistent Thing"], &doc); + let said = stderr_of(&out); + assert!( + !said.trim().is_empty(), + "a refused command said nothing at all; stdout={:?}", + stdout_of(&out) + ); + assert!( + said.contains("Nonexistent"), + "the refusal does not name the category that does not exist: {said:?}" + ); +} + +/// The rival this separates from: a fix that reports on the happy path and +/// stays silent on the refusal. `context/test-evidence/cli.md`. +#[test] +fn a_command_with_nothing_to_say_says_nothing() { + let scratch = Scratch::new("quiet"); + let doc = scratch.path("m.improv"); + let out = run_cmd(&["add-category Region"], &doc); + assert!( + stderr_of(&out).trim().is_empty(), + "a command that succeeded with nothing to report spoke anyway: {:?}", + stderr_of(&out) + ); + assert!( + out.status.success(), + "a command that succeeded did not exit 0" + ); +} + +#[test] +fn an_aborted_batch_exits_non_zero_and_says_why() { + let scratch = Scratch::new("abort"); + let doc = seeded(&scratch, "m.improv"); + let bad = scratch.path("no-such-dir").join("out.improv"); + let out = run_cmd(&[&format!("w {}", bad.display())], &doc); + let said = stderr_of(&out); + assert!( + !said.trim().is_empty(), + "a failed save said nothing: stdout={:?}", + stdout_of(&out) + ); + assert!( + !out.status.success(), + "a failed save exited 0; stderr={said:?}" + ); +} + +#[test] +fn a_parse_error_still_exits_non_zero() { + let scratch = Scratch::new("parse"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["no-such-command-at-all"], &doc); + assert!(!out.status.success(), "an unparseable line exited 0"); + assert!( + stderr_of(&out).contains("no-such-command-at-all"), + "the parse error does not name the word typed: {:?}", + stderr_of(&out) + ); +} + +/// Diagnostics go to stderr so that stdout stays free for a data output, and +/// so `2>/dev/null` is the whole suppression story. `context/code/cli.md`. +#[test] +fn what_a_command_says_goes_to_stderr_and_stdout_stays_empty() { + let scratch = Scratch::new("stream"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["add-item Nonexistent Thing"], &doc); + assert!( + stdout_of(&out).trim().is_empty(), + "a diagnostic reached stdout: {:?}", + stdout_of(&out) + ); + assert!(!stderr_of(&out).trim().is_empty(), "stderr was empty too"); +} + +#[test] +fn a_script_reports_exactly_as_cmd_does() { + let scratch = Scratch::new("script"); + let doc = seeded(&scratch, "m.improv"); + let script = scratch.path("s.txt"); + std::fs::write(&script, "# a comment\nadd-item Nonexistent Thing\n").expect("a script file"); + let out = Command::new(bin()) + .arg("script") + .arg(&script) + .arg("-f") + .arg(&doc) + .output() + .expect("the improvise binary runs"); + assert!( + stderr_of(&out).contains("Nonexistent"), + "`script` swallowed the refusal `cmd` reports: {:?}", + stderr_of(&out) + ); +} + +/// A run that changed nothing must leave the bytes alone: a fixture edited by +/// hand between runs is otherwise silently normalised. `context/code/cli.md`. +#[test] +fn a_run_that_changes_nothing_does_not_rewrite_the_document() { + let scratch = Scratch::new("noop"); + let doc = seeded(&scratch, "m.improv"); + let before = std::fs::read(&doc).expect("the seeded document"); + let out = run_cmd(&["move-cursor down"], &doc); + let after = std::fs::read(&doc).expect("the document after a view-only command"); + assert_eq!( + before, + after, + "a view-only command rewrote the document; stderr={:?}", + stderr_of(&out) + ); +} + +/// The reported instance: `1e20` is expanded to its twenty digits by a run that +/// was asked to change nothing at all. +#[test] +fn a_no_op_run_leaves_an_unnormalised_literal_alone() { + let scratch = Scratch::new("literal"); + let doc = scratch.path("m.improv"); + std::fs::write( + &doc, + "v2025-04-09\n# Big\nInitial View: Default\n\n\ + ## View: Default\nRegion: row\n_Measure: column\n\n\ + ## Category: Region\n- North\n\n\ + ## Category: _Measure\n- Score\n\n\ + ## Data\nRegion=North, _Measure=Score = 1e20\n", + ) + .expect("a hand-written document"); + let before = std::fs::read_to_string(&doc).expect("the document"); + assert!(before.contains("1e20"), "the fixture itself is wrong"); + let out = run_cmd(&["move-cursor down"], &doc); + let after = std::fs::read_to_string(&doc).expect("the document after"); + assert!( + after.contains("1e20"), + "a no-op run normalised the literal; stderr={:?}, after={after:?}", + stderr_of(&out) + ); +} + +#[test] +fn the_cmd_help_does_not_advertise_a_language_it_refuses() { + for sub in ["cmd", "script"] { + let out = Command::new(bin()) + .args([sub, "--help"]) + .output() + .expect("the improvise binary runs"); + let text = format!("{}{}", stdout_of(&out), stderr_of(&out)); + assert!( + !text.contains("JSON") && !text.contains("json"), + "`{sub} --help` still advertises JSON, which it refuses: {text}" + ); + } +} + +/// The language it does accept, demonstrated end to end so the help above has +/// something true to point at. +#[test] +fn the_language_the_help_names_is_the_one_it_accepts() { + let scratch = Scratch::new("lang"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["add-item Region East"], &doc); + assert!(out.status.success(), "the command line form was refused"); + let text = std::fs::read_to_string(&doc).expect("the document"); + assert!(text.contains("East"), "the item was not added: {text}"); + + let json = run_cmd(&[r#"{"cmd":"add-item","args":["Region","West"]}"#], &doc); + assert!( + !json.status.success(), + "the JSON form the help used to advertise was accepted after all" + ); +} + +/// The rival this separates from: making any emitted status a non-zero exit. +/// `fit-measure` reports its reproducing seed the same way — `context/code/cli.md`. +#[test] +fn a_command_that_succeeds_while_reporting_still_exits_zero() { + let scratch = Scratch::new("reports"); + let doc = seeded(&scratch, "m.improv"); + let save = format!("w {}", doc.display()); + let out = run_cmd(&["add-item Region East", &save], &doc); + assert!( + !stderr_of(&out).trim().is_empty(), + "a successful save reported nothing, so this test proves nothing" + ); + assert!( + out.status.success(), + "a successful command that reported was treated as a failure: {:?}", + stderr_of(&out) + ); +} + +/// A run that did change the document still writes it, even when the last thing +/// it did was save somewhere else and clear the dirty flag. +#[test] +fn a_run_that_changed_the_document_still_writes_it() { + let scratch = Scratch::new("elsewhere"); + let doc = seeded(&scratch, "m.improv"); + let other = scratch.path("other.improv"); + let save = format!("w {}", other.display()); + run_cmd(&["add-item Region East", &save], &doc); + let text = std::fs::read_to_string(&doc).expect("the -f document"); + assert!( + text.contains("East"), + "the -f document lost an edit because `:w` elsewhere cleared the flag: {text}" + ); +} From b8e1c138eb2a22784f6dbbd1cd6cddcc293955d3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 08:18:34 -0700 Subject: [PATCH 2/4] fix(command): a refusal named a command the user did not type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit improvise-z05h, three bugs stacked on one surface. `CmdRegistry::resolve` rewrites `formula` to `add-formula` *before* the arity check runs, so `formula Product Total = Widget + Gadget` was refused with "add-formula requires 1-2 argument(s), got 6" — a command nobody typed, and an arity where the user needed to be told to quote. The name now lives in the one place that knows which of the two was written: `require_args` no longer takes a `word` and `CmdRegistry::parse` prefixes `"{typed}: "` onto any `Err`. That removes 41 name literals from `cmd/`, which is the argument gotcha 15 already makes about `registry.rs`. The `AddFormulaCmd` validator states the shape rather than the arity. `interactive` deliberately does not prefix — `dispatch` in `command/keymap.rs` already formats `"{name}: {e}"` with the bound name (improvise-55l5, PR #126), and doing both would double it. All three documented spellings of `:formula` taught the unquoted form and so were ones the parser refuses. help.rs carried the quick-start line **twice**, on two different pages, which is gotcha 48's "changing a fact stated twice is a grep, not an edit". `every_ex_command_the_help_pages_show_is_one_the_parser_accepts` closes the surface rather than the instance: `HelpLine::Keys` rows are derived from the keymap and cannot go stale, but `Literal` and `Accent` rows are prose with nothing checking them, and that is the whole unguarded surface. Run against bf83294 it fails naming the Welcome page and the exact line. What it still cannot see, stated in `context/code/cli.md` §6: `draw.rs::draw_welcome` builds its lines inside a ratatui render function, so no walk reaches them; its line was corrected by hand. Checked and not needed: none of the four asciinema casts in `docs/` shows the `:formula` ex-command — `formulas.cast` uses the formula panel — so none inherits this and none needs re-recording. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/test-evidence/command.md | 54 ++++++++ .../improvise-command/src/command/cmd/core.rs | 15 +-- .../src/command/cmd/effect_cmds.rs | 39 +++--- .../src/command/cmd/registry.rs | 123 ++++++++++++++---- crates/improvise-protocol/src/help.rs | 9 +- src/draw.rs | 4 +- 6 files changed, 189 insertions(+), 55 deletions(-) diff --git a/context/test-evidence/command.md b/context/test-evidence/command.md index 827e9a69..7186592d 100644 --- a/context/test-evidence/command.md +++ b/context/test-evidence/command.md @@ -4373,3 +4373,57 @@ Fault injected: `plan.at(layout.cell_key(0, ci).as_ref())` in place of `layout.column_key(ci)` — the rejected version, restored. Fails with `left: ["14.5000", "16.5000"] right: ["14.50", "16.50"]`, which is the bug in one line: row order decided the spelling. + +--- + +## `command/cmd/registry.rs` — a refusal names the word that was typed + +improvise-z05h. Reasoning: `context/code/cli.md` §5 and §6. + +### a_refusal_names_the_command_as_typed_through_an_alias + +`CmdRegistry::resolve` rewrites `formula` to `add-formula` before the arity +check runs, so the refusal for `formula Product Total = Widget + Gadget` named +a command the user had not typed. Both directions are asserted — that the +message opens with `formula:` **and** that `add-formula` does not appear in it, +because prefixing the typed name onto a message that still spells the canonical +one reads as fixed and is not. + +The alias is what makes this discriminating; its sibling below covers the case +where the two names coincide, so neither can pass by accident. + +### a_refusal_from_a_canonical_name_is_named_as_typed + +The control. `add-item` is not aliased, so a fix that only rewrote aliased +names would pass the test above and fail here. Together they say the name comes +from the registry in every case, which is what removing 41 name literals from +`require_args` call sites bought. + +### a_formula_refusal_states_the_shape_and_asks_for_quoting + +The third of z05h's stacked bugs, and the one a user actually needs. +"requires 1-2 argument(s), got 6" is true and unactionable: nothing in it +suggests that the formula must be quoted. Asserts the message names the shape +and contains the word `quote`, so a refusal that merely restates the arity with +a friendlier name still fails. + +### every_ex_command_the_help_pages_show_is_one_the_parser_accepts + +The surface z05h identified as unguarded. `help.rs` derives `HelpLine::Keys` +rows from the keymap and drops any whose command does not resolve, so key rows +cannot go stale — but `Literal` and `Accent` rows are prose, and all three +documented spellings of `:formula` were ones the parser refuses. + +This closes the surface rather than the instance: every help line whose trimmed +text opens with `:` is split on its colons — one row shows two `:add-cat` +examples side by side — and each piece is handed to `parse_line`. Run against +`bf83294` it fails naming the Welcome page and the exact line. + +The `checked >= 8` floor is a check-the-checker: a future refactor that stops +`pages()` emitting `Accent`/`Literal` rows, or that indents them differently, +would otherwise leave the test scanning nothing and passing. + +What it cannot see, stated because the gap is one edit away from mattering: +`draw.rs::draw_welcome` builds its lines as a local array inside a ratatui +render function, so no walk reaches them. Its `:formula` line was corrected by +hand in the same change. diff --git a/crates/improvise-command/src/command/cmd/core.rs b/crates/improvise-command/src/command/cmd/core.rs index 5aec7858..e0d85709 100644 --- a/crates/improvise-command/src/command/cmd/core.rs +++ b/crates/improvise-command/src/command/cmd/core.rs @@ -222,10 +222,12 @@ impl CmdRegistry { /// Construct a command from text arguments (script/headless). pub fn parse(&self, name: &str, args: &[String]) -> Result, String> { - let name = self.resolve(name); + // The registry is the only thing that knows the word the user typed, so + // it is the only thing that may name it — `context/code/command.md`. + let canonical = self.resolve(name); for e in &self.entries { - if e.name == name { - return (e.parse)(args); + if e.name == canonical { + return (e.parse)(args).map_err(|err| format!("{name}: {err}")); } } Err(format!("Unknown command: {name}")) @@ -254,12 +256,9 @@ impl CmdRegistry { } } -pub(super) fn require_args(word: &str, args: &[String], n: usize) -> Result<(), String> { +pub(super) fn require_args(args: &[String], n: usize) -> Result<(), String> { if args.len() < n { - Err(format!( - "{word} requires {n} argument(s), got {}", - args.len() - )) + Err(format!("requires {n} argument(s), got {}", args.len())) } else { Ok(()) } diff --git a/crates/improvise-command/src/command/cmd/effect_cmds.rs b/crates/improvise-command/src/command/cmd/effect_cmds.rs index 4051a1b1..e4cce17b 100644 --- a/crates/improvise-command/src/command/cmd/effect_cmds.rs +++ b/crates/improvise-command/src/command/cmd/effect_cmds.rs @@ -112,7 +112,7 @@ macro_rules! effect_cmd { effect_cmd!( AddCategoryCmd, "add-category", - |args: &[String]| require_args("add-category", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::AddCategory(args[0].clone()))] } @@ -121,7 +121,7 @@ effect_cmd!( effect_cmd!( AddItemCmd, "add-item", - |args: &[String]| require_args("add-item", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::AddItem { category: args[0].clone(), @@ -157,7 +157,7 @@ effect_cmd!( effect_cmd!( AddItemInGroupCmd, "add-item-in-group", - |args: &[String]| require_args("add-item-in-group", args, 3), + |args: &[String]| require_args(args, 3), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::AddItemInGroup { category: args[0].clone(), @@ -200,8 +200,9 @@ effect_cmd!( "add-formula", |args: &[String]| { if args.is_empty() || args.len() > 2 { + // The arity alone does not tell anyone to quote — improvise-z05h. return Err(format!( - "add-formula requires 1-2 argument(s), got {}", + "expected [] \" = \" — got {} word(s); quote the formula", args.len() )); } @@ -225,7 +226,7 @@ effect_cmd!( effect_cmd!( ClearBufferCmd, "clear-buffer", - |args: &[String]| require_args("clear-buffer", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![super::core::write_buffer(&args[0], String::new())] } @@ -234,7 +235,7 @@ effect_cmd!( effect_cmd!( RemoveFormulaCmd, "remove-formula", - |args: &[String]| require_args("remove-formula", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::RemoveFormula { target_category: args[0].clone(), @@ -246,7 +247,7 @@ effect_cmd!( effect_cmd!( CreateViewCmd, "create-view", - |args: &[String]| require_args("create-view", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::CreateView(args[0].clone()))] } @@ -255,7 +256,7 @@ effect_cmd!( effect_cmd!( DeleteViewCmd, "delete-view", - |args: &[String]| require_args("delete-view", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::DeleteView(args[0].clone()))] } @@ -264,7 +265,7 @@ effect_cmd!( effect_cmd!( SwitchViewCmd, "switch-view", - |args: &[String]| require_args("switch-view", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::SwitchView(args[0].clone()))] } @@ -273,7 +274,7 @@ effect_cmd!( effect_cmd!( SetAxisCmd, "set-axis", - |args: &[String]| require_args("set-axis", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { let axis = match args[1].to_lowercase().as_str() { "row" => Axis::Row, @@ -293,7 +294,7 @@ effect_cmd!( effect_cmd!( SetPageCmd, "set-page", - |args: &[String]| require_args("set-page", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::SetPageSelection { view: ViewTarget::Active, @@ -306,7 +307,7 @@ effect_cmd!( effect_cmd!( ToggleGroupCmd, "toggle-group", - |args: &[String]| require_args("toggle-group", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::ToggleGroup { view: ViewTarget::Active, @@ -319,7 +320,7 @@ effect_cmd!( effect_cmd!( HideItemCmd, "hide-item", - |args: &[String]| require_args("hide-item", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::HideItem { view: ViewTarget::Active, @@ -332,7 +333,7 @@ effect_cmd!( effect_cmd!( ShowItemCmd, "show-item", - |args: &[String]| require_args("show-item", args, 2), + |args: &[String]| require_args(args, 2), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::ShowItem { view: ViewTarget::Active, @@ -345,7 +346,7 @@ effect_cmd!( effect_cmd!( SaveAsCmd, "save-as", - |args: &[String]| require_args("save-as", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::SaveAs(std::path::PathBuf::from(&args[0])))] } @@ -354,7 +355,7 @@ effect_cmd!( effect_cmd!( SetFormatCmd, "set-format", - |args: &[String]| require_args("set-format", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::SetNumberFormat( ViewTarget::Active, @@ -366,7 +367,7 @@ effect_cmd!( effect_cmd!( ImportCmd, "import", - |args: &[String]| require_args("import", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::StartImportWizard(args[0].clone()))] } @@ -428,7 +429,7 @@ effect_cmd!( effect_cmd!( LoadModelCmd, "load", - |args: &[String]| require_args("load", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::LoadModel(std::path::PathBuf::from( &args[0], @@ -439,7 +440,7 @@ effect_cmd!( effect_cmd!( ExportCsvCmd, "export-csv", - |args: &[String]| require_args("export-csv", args, 1), + |args: &[String]| require_args(args, 1), |args: &Vec, _ctx: &CmdContext| -> Vec> { vec![Box::new(effect::ExportCsv(std::path::PathBuf::from( &args[0], diff --git a/crates/improvise-command/src/command/cmd/registry.rs b/crates/improvise-command/src/command/cmd/registry.rs index d288c0bc..99905588 100644 --- a/crates/improvise-command/src/command/cmd/registry.rs +++ b/crates/improvise-command/src/command/cmd/registry.rs @@ -90,7 +90,7 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "move-selection", }, |args| { - require_args("move-selection", args, 2)?; + require_args(args, 2)?; let dr = args[0].parse::().map_err(|e| e.to_string())?; let dc = args[1].parse::().map_err(|e| e.to_string())?; Ok(Box::new(Move { @@ -100,7 +100,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("move-selection", args, 2)?; + require_args(args, 2)?; let dr = args[0].parse::().map_err(|e| e.to_string())?; let dc = args[1].parse::().map_err(|e| e.to_string())?; Ok(Box::new(Move { @@ -159,7 +159,7 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "scroll-rows", }, |args| { - require_args("scroll-rows", args, 1)?; + require_args(args, 1)?; let n = args[0].parse::().map_err(|e| e.to_string())?; Ok(Box::new(Move { kind: MoveKind::Relative(n, 0), @@ -168,7 +168,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("scroll-rows", args, 1)?; + require_args(args, 1)?; let n = args[0].parse::().map_err(|e| e.to_string())?; Ok(Box::new(Move { kind: MoveKind::Relative(n, 0), @@ -184,7 +184,7 @@ pub fn default_registry() -> CmdRegistry { cmd_name: "page-scroll", }, |args| { - require_args("page-scroll", args, 1)?; + require_args(args, 1)?; let dir = args[0].parse::().map_err(|e| e.to_string())?; Ok(Box::new(Move { kind: MoveKind::Page(dir), @@ -193,7 +193,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("page-scroll", args, 1)?; + require_args(args, 1)?; let dir = args[0].parse::().map_err(|e| e.to_string())?; Ok(Box::new(Move { kind: MoveKind::Page(dir), @@ -280,7 +280,7 @@ pub fn default_registry() -> CmdRegistry { edit_mode: AppMode::Normal, }, |args| { - require_args("edit-or-drill", args, 1)?; + require_args(args, 1)?; let edit_mode = parse_mode_name(&args[0])?; Ok(Box::new(EditOrDrill { edit_mode })) }, @@ -290,7 +290,7 @@ pub fn default_registry() -> CmdRegistry { target_mode: AppMode::Normal, }, |args| { - require_args("enter-edit-at-cursor", args, 1)?; + require_args(args, 1)?; let target_mode = parse_mode_name(&args[0])?; Ok(Box::new(EnterEditAtCursorCmd { target_mode })) }, @@ -320,7 +320,7 @@ pub fn default_registry() -> CmdRegistry { |_, _| Ok(Box::new(ViewNavigate { forward: true })), ); r.register_pure(&EnterMode(AppMode::Normal), |args| { - require_args("enter-mode", args, 1)?; + require_args(args, 1)?; Ok(Box::new(EnterMode(parse_mode_name(&args[0])?))) }); @@ -341,7 +341,7 @@ pub fn default_registry() -> CmdRegistry { }, |args| { // Parse: toggle-panel-and-focus [open] [focused] - require_args("toggle-panel-and-focus", args, 1)?; + require_args(args, 1)?; let panel = parse_panel(&args[0])?; let open = args.get(1).map(|s| s == "true").unwrap_or(true); let focused = args.get(2).map(|s| s == "true").unwrap_or(open); @@ -352,7 +352,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("toggle-panel-and-focus", args, 1)?; + require_args(args, 1)?; let panel = parse_panel(&args[0])?; // Default interactive: if already open+focused -> close, else open+focus let currently_open = ctx.panel_open(panel); @@ -375,7 +375,7 @@ pub fn default_registry() -> CmdRegistry { currently_open: false, }, |args| { - require_args("toggle-panel-visibility", args, 1)?; + require_args(args, 1)?; let panel = parse_panel(&args[0])?; Ok(Box::new(TogglePanelVisibility { panel, @@ -383,7 +383,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("toggle-panel-visibility", args, 1)?; + require_args(args, 1)?; let panel = parse_panel(&args[0])?; let currently_open = ctx.panel_open(panel); Ok(Box::new(TogglePanelVisibility { @@ -421,7 +421,7 @@ pub fn default_registry() -> CmdRegistry { max: 0, }, |args| { - require_args("move-panel-cursor", args, 2)?; + require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; Ok(Box::new(MovePanelCursor { @@ -432,7 +432,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("move-panel-cursor", args, 2)?; + require_args(args, 2)?; let panel = parse_panel(&args[0])?; let delta = args[1].parse::().map_err(|e| e.to_string())?; let (current, max) = match panel { @@ -463,7 +463,7 @@ pub fn default_registry() -> CmdRegistry { // ── Tile select ────────────────────────────────────────────────────── r.register_pure(&MoveTileCursor(0), |args| { - require_args("move-tile-cursor", args, 1)?; + require_args(args, 1)?; let delta = args[0].parse::().map_err(|e| e.to_string())?; Ok(Box::new(MoveTileCursor(delta))) }); @@ -473,7 +473,7 @@ pub fn default_registry() -> CmdRegistry { axis: Some(crate::view::Axis::Row), }, |args| { - require_args("set-axis-for-tile", args, 1)?; + require_args(args, 1)?; let axis = parse_axis(&args[0])?; Ok(Box::new(TileAxisOp { axis: Some(axis) })) }, @@ -494,7 +494,7 @@ pub fn default_registry() -> CmdRegistry { buffer: String::new(), }, |args| { - require_args("append-char", args, 1)?; + require_args(args, 1)?; Ok(Box::new(AppendChar { buffer: args[0].clone(), })) @@ -505,7 +505,7 @@ pub fn default_registry() -> CmdRegistry { buffer: String::new(), }, |args| { - require_args("pop-char", args, 1)?; + require_args(args, 1)?; Ok(Box::new(PopChar { buffer: args[0].clone(), })) @@ -537,7 +537,7 @@ pub fn default_registry() -> CmdRegistry { })) }, |args, ctx| { - require_args("commit-cell-edit", args, 1)?; + require_args(args, 1)?; let edit_mode = parse_mode_name(&args[0])?; let value = read_buffer(ctx, "edit"); let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; @@ -560,7 +560,7 @@ pub fn default_registry() -> CmdRegistry { }, |_| Err("commit-and-advance-right requires context".into()), |args, ctx| { - require_args("commit-and-advance-right", args, 1)?; + require_args(args, 1)?; let edit_mode = parse_mode_name(&args[0])?; let value = read_buffer(ctx, "edit"); let key = ctx.cell_key().clone().ok_or("no cell at cursor")?; @@ -578,12 +578,12 @@ pub fn default_registry() -> CmdRegistry { kind: CommitKind::Category, }, |args| { - require_args("commit-buffer", args, 1)?; + require_args(args, 1)?; let kind = parse_commit_kind(&args[0], &AppMode::Normal)?; Ok(Box::new(CommitBuffer { kind })) }, |args, ctx| { - require_args("commit-buffer", args, 1)?; + require_args(args, 1)?; let kind = parse_commit_kind(&args[0], ctx.mode)?; Ok(Box::new(CommitBuffer { kind })) }, @@ -716,6 +716,83 @@ mod tests { ); } } + + /// A refusal names the word the user typed, not the one the alias table + /// resolved it to — `context/test-evidence/command.md`. + #[test] + fn a_refusal_names_the_command_as_typed_through_an_alias() { + let err = crate::command::parse_line("formula Product Total = Widget + Gadget") + .expect_err("an unquoted formula is refused"); + assert!( + err.starts_with("formula:"), + "the refusal does not open with the word typed: {err:?}" + ); + assert!( + !err.contains("add-formula"), + "the refusal names a command the user did not type: {err:?}" + ); + } + + /// Stating the arity does not tell anyone what to do; stating the shape does. + #[test] + fn a_formula_refusal_states_the_shape_and_asks_for_quoting() { + let err = crate::command::parse_line("formula Product Total = Widget + Gadget") + .expect_err("an unquoted formula is refused"); + assert!( + err.contains("quote"), + "the refusal does not suggest quoting, so it is unactionable: {err:?}" + ); + assert!( + err.contains(" = "), + "the refusal does not state the shape it wanted: {err:?}" + ); + } + + /// Every other refusal is named too, without the alias table in the way. + #[test] + fn a_refusal_from_a_canonical_name_is_named_as_typed() { + let err = crate::command::parse_line("add-item").expect_err("add-item needs arguments"); + assert!( + err.starts_with("add-item:"), + "the refusal does not open with the word typed: {err:?}" + ); + } + + /// The ex-commands the help pages print are `HelpLine::Literal`/`Accent` + /// rows, which no derivation checks — `context/test-evidence/command.md`. + #[test] + fn every_ex_command_the_help_pages_show_is_one_the_parser_accepts() { + let mut checked = 0; + for page in improvise_protocol::help::pages() { + for line in &page.lines { + let text = match line { + improvise_protocol::help::HelpLine::Accent(s) => *s, + improvise_protocol::help::HelpLine::Literal { keys, .. } => *keys, + _ => continue, + }; + let text = text.trim(); + if !text.starts_with(':') { + continue; + } + // One row may show two examples side by side, each opening + // with its own colon. + for example in text.split(':').filter(|s| !s.trim().is_empty()) { + if let Err(e) = crate::command::parse_line(example.trim()) { + panic!( + "help page {:?} shows `:{}`, which the parser refuses: {e}", + page.title, + example.trim() + ); + } + checked += 1; + } + } + } + assert!( + checked >= 8, + "only {checked} ex-command examples were found; the scan has stopped seeing them" + ); + } } #[cfg(test)] diff --git a/crates/improvise-protocol/src/help.rs b/crates/improvise-protocol/src/help.rs index c367f719..d7d0566b 100644 --- a/crates/improvise-protocol/src/help.rs +++ b/crates/improvise-protocol/src/help.rs @@ -412,7 +412,7 @@ fn welcome() -> Vec { HelpLine::Text(" 3. Navigate with hjkl or arrow keys and press i to edit cells."), HelpLine::Blank, HelpLine::Text(" 4. Add formulas to compute values automatically:"), - HelpLine::Accent(" :formula Product Total = Widget + Gadget"), + HelpLine::Accent(" :formula Product \"Total = Widget + Gadget\""), HelpLine::Blank, HelpLine::Text(" 5. Save your work:"), HelpLine::Accent(" :w mymodel.improv"), @@ -568,7 +568,7 @@ fn editing() -> Vec { " Example: in a Product category with items Widget and Gadget:", )), Some(HelpLine::Accent( - " :formula Product Total = Widget + Gadget", + " :formula Product \"Total = Widget + Gadget\"", )), Some(HelpLine::Blank), Some(HelpLine::Text(" Supported operators: + - * /")), @@ -780,7 +780,10 @@ fn commands() -> Vec { ":add-items a b c ...", "Add multiple items at once", )), - Some(literal(":formula ", "Add a formula")), + Some(literal( + ":formula \" = \"", + "Add a formula", + )), Some(HelpLine::Blank), Some(HelpLine::Heading("Views")), Some(HelpLine::Blank), diff --git a/src/draw.rs b/src/draw.rs index fb46dfff..6e28b667 100644 --- a/src/draw.rs +++ b/src/draw.rs @@ -483,11 +483,11 @@ fn draw_welcome(f: &mut Frame, area: Rect) { Style::default().fg(Color::Cyan), ), ( - ":formula Add a formula, e.g.:", + ":formula \" = \" Add a formula, e.g.:", Style::default().fg(Color::Cyan), ), ( - " Profit = Revenue - Cost", + " :formula _Measure \"Profit = Revenue - Cost\"", Style::default().fg(Color::Green), ), ( From 42cbf0932b35def1021d530477ea81466c5b5f5a Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 08:20:48 -0700 Subject: [PATCH 3/4] docs: name the follow-up, and stop the scratch dir looking like a ticket id improvise-udt2 is the ticket context/code/cli.md section 3 points at: a status message carries no severity, so a refusal that does not abort cannot set a non-zero exit. It names both disproved classifiers, so the next attempt does not rediscover them. And the per-test scratch directory was named `improvise-cli-`, which matches the ticket-id shape the AGENTS.md audit greps for. An ID-shaped non-ID is worse than a dead ID, because the diagnosis is identical and the repair is not. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/cli.md | 3 ++- tests/headless_cli.rs | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/context/code/cli.md b/context/code/cli.md index 67063a98..061d3480 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -59,7 +59,8 @@ What this does **not** cover, stated rather than papered over: a refusal that only writes a status still exits 0. Closing it needs a severity beside `status_msg`, which lives in `ViewState` and is therefore a wire change (gotcha 25) reaching the browser client — out of proportion to the CLI, and -filed separately. Until then the README says plainly that exit 0 means "nothing +filed as improvise-udt2, which names both rivals so the next attempt cannot +rediscover them. Until then the README says plainly that exit 0 means "nothing aborted", not "everything worked", because a contract that over-promises is worse than a narrow one. diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index bed6d1b4..5e50217c 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -12,7 +12,7 @@ struct Scratch(PathBuf); impl Scratch { fn new(tag: &str) -> Self { - let dir = std::env::temp_dir().join(format!("improvise-cli-{tag}-{}", std::process::id())); + let dir = std::env::temp_dir().join(format!("improv_cli_{tag}-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).expect("a scratch directory"); Self(dir) From 6034a216e157436775e18e929c72d33f88d81411 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 08:26:12 -0700 Subject: [PATCH 4/4] docs(site): publish the two new context docs, and de-number two stale counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `build` job failed, correctly: `scripts/build-doc-site.mjs` requires every `context/**/*.md` to be either in the SECTIONS manifest or a named omission (improvise-res9's cure), and the two files this branch added were neither. Both are now published, beside the TUI in each section. While there: `context/code/doc-site.md` asserted 'the seven `context/code/*.md` documents' when there were already twelve, and '501 entries' when the checker reports 801. Both were stale before this branch and this branch makes them staler, so they are de-numbered rather than re-asserted — a count in prose that nothing checks is a fact with no owner, and the shape that stops drifting is not having one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH --- context/code/doc-site.md | 8 ++++---- scripts/build-doc-site.mjs | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/context/code/doc-site.md b/context/code/doc-site.md index 92f8965d..44a8a934 100644 --- a/context/code/doc-site.md +++ b/context/code/doc-site.md @@ -21,7 +21,7 @@ Published, in four sections: | Start here | `docs/design-notes.md`, `context/design-principles.md`, `context/repo-map.md` | What the project is for, the doctrine, and the single best explanation of how it is built. | | Decision records | `context/adr/*.md` | An ADR is written for an outside reader by construction — it states the options that were rejected, which is the part that is worth reading and the part that is nowhere else. | | The arguments, by area | `context/code/*.md` | This is the reasoning that used to live in code comments and was moved out under the two-line cap (design-principles §6). It is the most valuable prose in the repository and it is addressed to exactly the reader a documentation site has. | -| Test evidence | `context/test-evidence/*.md` | 501 entries, one per test whose narrative is longer than its code. | +| Test evidence | `context/test-evidence/*.md` | One entry per test whose narrative is longer than its code. | Not published: @@ -62,9 +62,9 @@ says `context/code/*.md` is published; the manifest disagreed and nothing could see it. That is the whole failure mode in one file. **Test evidence is the one judgement call worth stating.** The argument for -leaving it out is that 501 entries about individual tests are noise to a reader -who wanted to know what a pivot table is. The argument for keeping it is -stronger: every one of the seven `context/code/*.md` documents points at its +leaving it out is that hundreds of entries about individual tests are noise +to a reader who wanted to know what a pivot table is. The argument for keeping it is +stronger: every one of the `context/code/*.md` documents points at its evidence file by name, so omitting it would break the published prose in exactly the way this site is otherwise careful not to; and the evidence file is the most distinctive artefact the repository has — a written record of the diff --git a/scripts/build-doc-site.mjs b/scripts/build-doc-site.mjs index 1a847d4e..24bb82b4 100644 --- a/scripts/build-doc-site.mjs +++ b/scripts/build-doc-site.mjs @@ -58,6 +58,7 @@ const SECTIONS = [ ["context/code/command.md", "code/command", "improvise-command"], ["context/code/clients.md", "code/clients", "The clients"], ["context/code/tui.md", "code/tui", "The TUI"], + ["context/code/cli.md", "code/cli", "The headless CLI"], ["context/code/gp.md", "code/gp", "gp-tree and improvise-gp"], ["context/code/kb-tool.md", "code/kb-tool", "The kb tool"], ["context/code/tooling.md", "code/tooling", "The repository's own checkers"], @@ -75,6 +76,7 @@ const SECTIONS = [ ["context/test-evidence/command.md", "test-evidence/command", "improvise-command"], ["context/test-evidence/clients.md", "test-evidence/clients", "The clients"], ["context/test-evidence/tui.md", "test-evidence/tui", "The TUI"], + ["context/test-evidence/cli.md", "test-evidence/cli", "The headless CLI"], ["context/test-evidence/gp.md", "test-evidence/gp", "gp-tree and improvise-gp"], ["context/test-evidence/tooling.md", "test-evidence/tooling", "The repository's own gates"], ],