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