From 487f0b6fef36a1c91a6b708db3c96a9f96d50d8d Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:18:25 -0700 Subject: [PATCH 1/3] improvise-up8g: a headless run's extent is fixed, not read off the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_headless_commands` read `crossterm::terminal::size()`, and the page size decides where `page-scroll` lands, so one script wrote a different record in a different window — and CI (no tty, 80x24 default) disagreed with a shell. Pin it to 80x24 in `HEADLESS_EXTENT`. The test drives two sizes through LINES/ COLUMNS with the child detached by `setsid`, and asserts the same record. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- Cargo.lock | 1 + Cargo.toml | 2 ++ README.md | 4 +++ context/code/cli.md | 22 ++++++++++++++ context/test-evidence/cli.md | 24 +++++++++++++++ src/main.rs | 10 ++++--- tests/headless_cli.rs | 58 ++++++++++++++++++++++++++++++++++++ 7 files changed, 117 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cb982440..d1f6969c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -845,6 +845,7 @@ dependencies = [ "improvise-protocol", "improvise-wasm-client", "indexmap", + "libc", "pest", "pest_derive", "pest_meta", diff --git a/Cargo.toml b/Cargo.toml index 82165d52..86f54e65 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,6 +59,8 @@ enum_dispatch = "0.3.13" # natively, which is how its own tests already run. improvise-wasm-client = { path = "crates/improvise-wasm-client" } improvise-protocol = { path = "crates/improvise-protocol" } +# `tests/headless_cli.rs` detaches the binary from any terminal with `setsid`. +libc = "0.2" pest = "2.9.2" pest_derive = "2.9.2" pest_meta = "2.9.2" diff --git a/README.md b/README.md index 2fd11560..d3b1e4c2 100644 --- a/README.md +++ b/README.md @@ -210,6 +210,10 @@ improvise cmd 'formula Product "Total = Widget + Gadget"' -f model.improv 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 behaves as if its terminal were 80x24, whatever it runs in.** A page + of rows is a real distance — `page-scroll` moves the cursor by one — so + reading the actual window would make the same script write a different record + in a different terminal, and a different one again in CI. - **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. diff --git a/context/code/cli.md b/context/code/cli.md index d4cc9a68..f51008b5 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -223,3 +223,25 @@ Guards: `every_argument_list_the_gate_accepts_the_run_accepts_too` enumerates cursor sits on no cell — the context in which the old `commit-cell-edit` arm refused. It lists no commands, so a new registration is covered the day it lands. It is backed by the process-level tests in `tests/headless_cli.rs`. + +## 9 — the extent is fixed, because it reaches the document + +A headless run used to read `crossterm::terminal::size()`, and the page size +derived from it decides where `page-scroll` lands and therefore which record the +next cursor-reading command writes (improvise-up8g). So one script wrote a +different document in a different window, and with no tty — CI, cron — the read +failed and left `App`'s default, so CI and a shell disagreed too. It was +invisible until §7 let a script see its cursor. + +`HEADLESS_EXTENT` in `main.rs` pins it to 80x24 — the size CI already had, so +nothing that passed there moves. There is **no flag** to change it, for §2's +reason: a second knob with nothing asking for it. A script that wants a +particular row says so with `move-selection` or `jump-*`, which do not depend on +any window. + +What this does not close: the **TUI** still takes its extent from the terminal, +and that is correct there, because the window really is an input to an +interactive session. The ticket's wider claim — that a replayable log needs the +extent in it (improvise-ilvl) — is about that log, which does not exist yet; a +headless run has nothing to replay *except* its script, so pinning is the whole +fix for this surface. diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 4c89ddee..346ef8b7 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -320,3 +320,27 @@ on a **closed** panel it opened it. The fixture closes every panel first, because on an open-and-focused panel the toggle also closes it and the test would pass against the bug. Rival: `execute` ignoring `target` and always toggling — fails this test for every panel. + +### a_script_writes_the_same_record_whatever_terminal_ran_it + +improvise-up8g. `run_headless_commands` read `crossterm::terminal::size()` into +`App::term_{width,height}`, and `CmdContext::visible_rows` derives from the +height, so `page-scroll 1 . hide-selected-row-item` over a 60-item row axis hid +`R13` at 24 rows and `R40` at 60 — one script, two documents. It became live +when #151 let a script's commands see the cursor (improvise-6uva); before that +the cursor was always the origin and every run hid `R01`. + +The test runs the same script against two copies of one document at `24x80` +and `60x200` and asserts the same record is hidden. One size cannot see the +bug, which is the ticket's own requirement. The size is driven through `LINES` +and `COLUMNS` with the child detached from any terminal by `setsid`: crossterm +asks `/dev/tty` first and falls back to `tput`, which honours those variables, +so without the detach a developer's real terminal would override both sizes and +the test would pass against the bug on a laptop and fail it only in CI. + +Two guards against vacuity. The hidden record must not be `R01`, because a +cursor that never moved agrees at every size (the pre-#151 shape, and the shape +of a fix that re-broke 6uva). And something must be hidden at all. Failed at +`9e78051` with `R13` against `R40`. It cannot see a *width* leak, and does not +need to: width moves only `col_offset`, which no command today turns into a +written record. diff --git a/src/main.rs b/src/main.rs index c714bd49..a29a9603 100644 --- a/src/main.rs +++ b/src/main.rs @@ -329,15 +329,17 @@ fn get_import_data(paths: &[PathBuf]) -> Option { // ── Headless command execution ─────────────────────────────────────────────── +/// The `(columns, rows)` every headless run behaves as if it had. +const HEADLESS_EXTENT: (u16, u16) = (80, 24); + fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> { use improvise::command::key::{KeyCode, KeyModifiers}; let workbook = get_initial_workbook(file)?; let mut app = ui::app::App::new(workbook, file.clone()); - if let Ok((w, h)) = crossterm::terminal::size() { - app.term_width = w; - app.term_height = h; - } + // Fixed, never read off the terminal: a page's size reaches the document, + // so a script must not depend on the window it ran in (improvise-up8g). + (app.term_width, app.term_height) = HEADLESS_EXTENT; 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`. diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index f0751f16..df00cddc 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -440,3 +440,61 @@ fn a_malformed_commit_line_is_refused_whole() { "the first half of a refused line landed: {text}" ); } + +/// Runs `improvise cmd` with no controlling terminal and the given `LINES`, +/// so the only size the process can discover is the one handed to it here. +#[cfg(unix)] +fn run_cmd_in_a_terminal_of(rows: u16, cols: u16, args: &[&str], file: &Path) -> Output { + use std::os::unix::process::CommandExt; + let mut cmd = Command::new(bin()); + cmd.arg("cmd") + .args(args) + .arg("-f") + .arg(file) + .env("TERM", "xterm") + .env("LINES", rows.to_string()) + .env("COLUMNS", cols.to_string()) + .stdin(std::process::Stdio::null()); + // SAFETY: `setsid` is async-signal-safe, and leaving the runner's terminal + // is what makes `LINES` the only size the child can find. + unsafe { + cmd.pre_exec(|| { + libc::setsid(); + Ok(()) + }); + } + cmd.output().expect("the improvise binary runs") +} + +/// Evidence: `context/test-evidence/cli.md`. +#[cfg(unix)] +#[test] +fn a_script_writes_the_same_record_whatever_terminal_ran_it() { + let scratch = Scratch::new("extent"); + let items: Vec = (1..=60).map(|i| format!("R{i:02}")).collect(); + let seed = scratch.path("seed.improv"); + run_cmd(&["add-category Region"], &seed); + run_cmd(&[&format!("add-items Region {}", items.join(" "))], &seed); + let script = ["page-scroll 1 . hide-selected-row-item"]; + + let mut hidden = Vec::new(); + for (rows, cols) in [(24, 80), (60, 200)] { + let doc = scratch.path(&format!("{rows}x{cols}.improv")); + std::fs::copy(&seed, &doc).expect("a copy of the seed"); + let out = run_cmd_in_a_terminal_of(rows, cols, &script, &doc); + assert!(out.status.success(), "{}", stderr_of(&out)); + hidden.push(hidden_line(&doc)); + } + assert_ne!( + hidden[0], "hidden: Region/R01", + "the cursor never moved, so the page size was never read" + ); + assert!( + hidden[0].starts_with("hidden: Region/R"), + "the script hid nothing, so agreement proves nothing: {hidden:?}" + ); + assert_eq!( + hidden[0], hidden[1], + "the same script hid a different record at a different terminal size" + ); +} From ffe8aed6b44c7ec67a42484787969ef6fd65b5e3 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:28:44 -0700 Subject: [PATCH 2/3] improvise-c5ip, improvise-udt2: exit 1 on a refusal, never on an abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The headless exit code read `last_batch_aborted()`, which answers "did a chain stop", not "did the request happen" — wrong both ways. `commit-cell-edit` at bottom-right lands its write, saves, and then `EnterAdvance`'s `AbortChain` exited 1 (c5ip); `add-item` into a missing category refuses without aborting and exited 0 (udt2). `App` gains a per-batch `batch_refused`, reset beside `batch_aborted` and written only by `App::refuse(msg)`, which sets the status and the flag together; commands reach it through `effect::refuse`. `main.rs` exits 1 on `last_batch_refused()` only. Not a severity on `status_msg`: that would be portable ViewState and would go stale across its ~60 writers. Strays into ui/app.rs (four small additions) — flagged in the hand-back. Unconverted refusals are filed as improvise-i3o5; headless `q` as improvise-ep5o. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- README.md | 13 ++- context/code/cli.md | 66 ++++++++++++--- context/code/command.md | 4 +- context/test-evidence/cli.md | 51 +++++++++++ .../src/command/cmd/commit.rs | 8 +- .../improvise-command/src/command/cmd/fit.rs | 15 +++- .../improvise-command/src/command/cmd/grid.rs | 10 +-- .../improvise-command/src/command/cmd/mode.rs | 4 +- .../src/command/cmd/search.rs | 2 +- .../src/command/cmd/text_buffer.rs | 2 +- crates/improvise-command/src/ui/app.rs | 21 +++++ crates/improvise-command/src/ui/effect.rs | 84 +++++++++++++++---- src/main.rs | 4 +- tests/headless_cli.rs | 50 +++++++++++ 14 files changed, 281 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index d3b1e4c2..03ae07e1 100644 --- a/README.md +++ b/README.md @@ -203,13 +203,12 @@ improvise cmd 'formula Product "Total = Widget + Gadget"' -f model.improv 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. +- **Exit 1** means a line did not parse, or a command refused what it was + asked — a failed `:w`, `add-item` naming a category that does not exist. + A command that did its work and then had nowhere further to go — a + `commit-cell-edit` on the last cell, whose advance stops — is not a refusal + and exits 0. A few refusals are not yet classified and still exit 0 + (improvise-i3o5), so check stderr as well as `$?` when it matters. - **A run behaves as if its terminal were 80x24, whatever it runs in.** A page of rows is a real distance — `page-scroll` moves the cursor by one — so reading the actual window would make the same script write a different record diff --git a/context/code/cli.md b/context/code/cli.md index f51008b5..372597e6 100644 --- a/context/code/cli.md +++ b/context/code/cli.md @@ -50,19 +50,16 @@ matches every other diagnostic `main.rs` already emits (`Saved to …`, **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 +## 3 — the exit code (superseded by §10) -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 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. +This section used to say: exit 1 on a parse error or on +`App::last_batch_aborted()`, and a refusal that only writes a status exits 0, +because closing that needed a severity beside `status_msg` — a `ViewState` +change, and so a wire change (gotcha 25). Both halves were wrong in a way worth +keeping. The abort half made a *success* exit 1 (improvise-c5ip), and the +severity half named the wrong home: the question an exit code asks is about one +batch, not about a message, so it belongs beside `batch_aborted` on `App` and +never reaches the wire. §10 is what replaced it. ## 4 — a run that changed nothing does not write @@ -245,3 +242,48 @@ interactive session. The ticket's wider claim — that a replayable log needs th extent in it (improvise-ilvl) — is about that log, which does not exist yet; a headless run has nothing to replay *except* its script, so pinning is the whole fix for this surface. + +## 10 — the exit code asks whether the change happened, never whether a chain stopped + +`App::last_batch_aborted()` answered the wrong question for an exit code, in +both directions at once. + +- **A success that exited 1** (improvise-c5ip). `commit-cell-edit` ends in + `EnterAdvance`, which emits `AbortChain` when the cursor is already + bottom-right or the grid is empty: *the advance's premise no longer holds*. + The write had already landed and was saved, and the run exited 1 with + nothing on stderr. Before §7 that was every scripted commit. +- **A refusal that exited 0** (improvise-udt2). `AddItem` into a missing + category writes the error and does not abort, because nothing after it + depends on the item. So the flag never saw it. + +An abort is a statement about the **chain**; a refusal is a statement about the +**request**. `AbortChain` stops a chain without refusing anything, and +`AddItem` refuses without stopping anything. The two coincide only for +`AddCategory` and the file writes, which is why reading one for the other +looked right for so long. + +So `App` carries a second per-batch outcome, `batch_refused`, reset where +`batch_aborted` is and written by exactly one method, `App::refuse(msg)`, which +sets the status and the flag together — a refusal cannot write its message and +forget the flag. Commands reach it through `effect::refuse`, the `Refuse` +effect, beside `effect::set_status` for reports. `main.rs` exits 1 on +`last_batch_refused()` and no longer reads `last_batch_aborted()` at all. + +**Why not a severity on `status_msg`**, which udt2 proposed. A severity on the +message would live in `ViewState`, so it would be portable state sent to every +client in a `Snapshot`, and it would go **stale**: every one of the ~60 writers +of `status_msg` would have to reset it or leave the previous message's severity +in place. A per-batch flag cannot go stale, because `apply_effects` clears it +before every batch, and it is not in `ViewState`, so nothing on the wire +carries it or has to agree with it. The TUI colouring the ticket hoped for is +still possible later, and would want the severity *on the message*; that is a +separate change with a separate staleness problem. + +**The classification is per writer, and it is not total.** Every refusal in +`ui/effect.rs` and in `cmd/{commit,fit,grid,mode,search,text_buffer}.rs` goes +through `refuse`. Those still written with `set_status` exit 0 as before, and +are listed in improvise-i3o5. `q` on a dirty document is deliberately left a +report: headless, its text is false — the CLI saves anyway (improvise-ep5o). +The two rivals udt2 named both stay green: a successful `:w` and a successful +`fit-measure` report through `set_status` and exit 0. diff --git a/context/code/command.md b/context/code/command.md index 987df348..e1bed0a7 100644 --- a/context/code/command.md +++ b/context/code/command.md @@ -990,7 +990,9 @@ Its one consumer is `Host::save`, where the batch is literally `[Save]` and so improvise-368h). **Naming the real call site matters** because that identity is what makes the flag readable at all: over a longer batch it says that *something* refused, never which. `App::last_batch_aborted` is the accessor and carries the -same caveat. +same caveat. It is **not** the refusal flag: `AbortChain` aborts without +refusing and `AddItem` refuses without aborting, so "did the request happen" is +`App::last_batch_refused`, written only by `App::refuse` (`context/code/cli.md` §10). ### `App::new` recomputes formulas before the first layout diff --git a/context/test-evidence/cli.md b/context/test-evidence/cli.md index 346ef8b7..d978d2c9 100644 --- a/context/test-evidence/cli.md +++ b/context/test-evidence/cli.md @@ -344,3 +344,54 @@ of a fix that re-broke 6uva). And something must be hidden at all. Failed at `9e78051` with `R13` against `R40`. It cannot see a *width* leak, and does not need to: width moves only `col_offset`, which no command today turns into a written record. + +### a_commit_whose_advance_has_nowhere_to_go_still_exits_zero + +improvise-c5ip. `commit-cell-edit` ends in `EnterAdvance`, which emits +`AbortChain` when the cursor is already bottom-right; the CLI mapped +`last_batch_aborted()` to exit 1, so a write that landed **and was saved** +exited 1 with nothing on stderr. Two cases: the cursor moved to the last of +three rows and the write named explicitly, and a commit on a grid that is empty +until the same line creates it. The value is asserted **first**, so the test +cannot pass by the write simply not happening. Failed at `487f0b6` on the exit +assertion with `South = 9` already written. Rival: keeping +`|| last_batch_aborted()` in `main.rs` beside the new refusal check — fails +here, at the first exit assertion. + +### a_refusal_that_only_writes_a_status_exits_non_zero + +improvise-udt2. `add-item Nonexistent Thing` writes the workbook's error to the +status and does not abort, so it exited 0. It asserts the refusal was printed +before asserting the exit, so it cannot pass on a run that failed for some other +reason. Rival: `AddItem` writing `status_msg` directly instead of calling +`App::refuse` — fails here. The two rivals the ticket named are the existing +`a_command_that_succeeds_while_reporting_still_exits_zero` (a successful `:w` +calling `refuse` fails it, measured) and `a_fit_that_reports_its_seed_is_not_a_refusal`. + +### a_refusal_marks_only_its_own_batch + +A refusing batch followed by a reporting one. The second assertion is the one +the default cannot supply: a flag that is never reset passes the first half and +fails the second. Rival: deleting the reset in `apply_effects` — fails at the +second assertion. Rival: `AddItem` not calling `refuse` — fails at the first. + +### a_chain_with_nowhere_to_go_aborts_without_refusing + +`AbortChain` must abort — asserted, so the test cannot pass on a batch that did +nothing — and must not refuse. Rival: `AbortChain` calling `refuse` — fails +here, and fails the process-level c5ip test too. + +### a_failed_save_is_a_refusal_as_well_as_an_abort + +The one place the two flags coincide, and the reason the old exit code looked +right: a failed file write both stops the chain (gotcha 21) and refuses. It +keeps `an_aborted_batch_exits_non_zero_and_says_why` meaningful now that the CLI +reads only the refusal. + +### a_fit_that_reports_its_seed_is_not_a_refusal + +udt2's second rival: `fit-measure` speaks on **success**, carrying the seed. +It asserts the status mentions the seed first, so a fit that said nothing +cannot pass. Rival: the success arm emitting `refuse` instead of `set_status` — +fails here. The refusal arm is asserted in +`a_refusal_names_what_went_wrong_and_stores_nothing`. diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 4815d166..0e67b061 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -343,7 +343,7 @@ mod tests { !dbg.contains("SaveAs"), "an empty path must not reach SaveAs, got: {dbg}" ); - assert!(dbg.contains("SetStatus"), "Expected a status, got: {dbg}"); + assert!(dbg.contains("Refuse"), "Expected a refusal, got: {dbg}"); } /// A one-record records grid with `Region` as its only category and `Revenue` as @@ -507,7 +507,7 @@ fn rename_record_coordinate( effects: &mut Vec>, ) { if value.is_empty() { - effects.push(effect::set_status(effect::RECORD_COORDS_CANNOT_BE_EMPTY)); + effects.push(effect::refuse(effect::RECORD_COORDS_CANNOT_BE_EMPTY)); return; } effects.push(Box::new(effect::AddItem { @@ -836,7 +836,7 @@ impl Cmd for CommitSave { let path = read_buffer(ctx, "save").trim().to_string(); if path.is_empty() { return vec![ - effect::set_status("No path given — not saved"), + effect::refuse("No path given — not saved"), effect::change_mode(AppMode::Normal), ]; } @@ -860,7 +860,7 @@ impl Cmd for CommitExport { let path = read_buffer(ctx, "export").trim().to_string(); if path.is_empty() { return vec![ - effect::set_status("No path given — not exported"), + effect::refuse("No path given — not exported"), effect::change_mode(AppMode::Normal), ]; } diff --git a/crates/improvise-command/src/command/cmd/fit.rs b/crates/improvise-command/src/command/cmd/fit.rs index a45c76ef..da7fb3c3 100644 --- a/crates/improvise-command/src/command/cmd/fit.rs +++ b/crates/improvise-command/src/command/cmd/fit.rs @@ -83,7 +83,7 @@ impl Cmd for FitMeasure { ], // Flattened: a refusal interpolates a document name, and a name may // legally contain a newline (Copilot on PR #25). - Err(e) => vec![effect::set_status(one_line(format!("fit-measure: {e}")))], + Err(e) => vec![effect::refuse(one_line(format!("fit-measure: {e}")))], } } } @@ -221,6 +221,19 @@ mod tests { app.view_state.status_msg ); assert!(app.model_state.workbook.model.formulas().is_empty()); + assert!(app.last_batch_refused(), "a refused fit exits non-zero"); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_fit_that_reports_its_seed_is_not_a_refusal() { + let app = run(&["Score"]); + assert!( + app.view_state.status_msg.contains("seed"), + "the fit said nothing, so this proves nothing: {:?}", + app.view_state.status_msg + ); + assert!(!app.last_batch_refused()); } /// gotcha 37b: a different seed really does search differently. It asserts on the diff --git a/crates/improvise-command/src/command/cmd/grid.rs b/crates/improvise-command/src/command/cmd/grid.rs index ef471310..85ee20e7 100644 --- a/crates/improvise-command/src/command/cmd/grid.rs +++ b/crates/improvise-command/src/command/cmd/grid.rs @@ -398,13 +398,13 @@ impl Cmd for ViewNavigate { fn execute(&self, ctx: &CmdContext) -> Vec> { if self.forward { if ctx.view_forward_stack.is_empty() { - vec![effect::set_status("No forward view")] + vec![effect::refuse("No forward view")] } else { vec![Box::new(effect::ViewForward)] } } else { if ctx.view_back_stack.is_empty() { - vec![effect::set_status("No previous view")] + vec![effect::refuse("No previous view")] } else { vec![ Box::new(effect::ApplyAndClearDrill), @@ -545,9 +545,7 @@ impl Cmd for AddRecordRow { } fn execute(&self, ctx: &CmdContext) -> Vec> { if !ctx.is_records_mode() { - return vec![effect::set_status( - "add-record-row only works in records mode", - )]; + return vec![effect::refuse("add-record-row only works in records mode")]; } // Build a CellKey from the current page filters let view = ctx.view; @@ -575,7 +573,7 @@ impl Cmd for AddRecordRow { let mut effects: Vec> = Vec::new(); match existing { None => effects.push(Box::new(effect::SetCell(key, CellValue::Number(0.0)))), - Some(_) => effects.push(effect::set_status( + Some(_) => effects.push(effect::refuse( "That record row is already here — a row is its coordinates, so \ fill this one in first", )), diff --git a/crates/improvise-command/src/command/cmd/mode.rs b/crates/improvise-command/src/command/cmd/mode.rs index 043e4b73..a74dfa17 100644 --- a/crates/improvise-command/src/command/cmd/mode.rs +++ b/crates/improvise-command/src/command/cmd/mode.rs @@ -334,14 +334,14 @@ impl Cmd for EditOrDrill { let is_aggregated = !is_synthetic && regular_none; if is_aggregated { let Some(key) = ctx.cell_key().clone() else { - return vec![effect::set_status(self.nothing_at_cursor())]; + return vec![effect::refuse(self.nothing_at_cursor())]; }; return DrillIntoCell { key }.execute(ctx); } // The same question `cell_id` asks, off the same predicate. In records mode // the synthetic key names no measure, so the record's key is resolved. if let Some(formula) = ctx.owning_formula() { - return vec![effect::set_status(format!( + return vec![effect::refuse(format!( "{} is computed by a formula — edit the formula, not the cell", formula.target ))]; diff --git a/crates/improvise-command/src/command/cmd/search.rs b/crates/improvise-command/src/command/cmd/search.rs index d2fb9371..528cadc6 100644 --- a/crates/improvise-command/src/command/cmd/search.rs +++ b/crates/improvise-command/src/command/cmd/search.rs @@ -127,7 +127,7 @@ impl Cmd for SearchNavigate { .collect(); if matches.is_empty() { - return vec![effect::set_status(format!( + return vec![effect::refuse(format!( "No matches for '{}'", ctx.search_query ))]; diff --git a/crates/improvise-command/src/command/cmd/text_buffer.rs b/crates/improvise-command/src/command/cmd/text_buffer.rs index 01af4e78..5b89f812 100644 --- a/crates/improvise-command/src/command/cmd/text_buffer.rs +++ b/crates/improvise-command/src/command/cmd/text_buffer.rs @@ -379,7 +379,7 @@ impl Cmd for ExecuteCommand { } Err(msg) => { vec![ - effect::set_status(format!(":{raw} — {msg}")), + effect::refuse(format!(":{raw} — {msg}")), effect::change_mode(AppMode::Normal), ] } diff --git a/crates/improvise-command/src/ui/app.rs b/crates/improvise-command/src/ui/app.rs index b026516f..afc79c2c 100644 --- a/crates/improvise-command/src/ui/app.rs +++ b/crates/improvise-command/src/ui/app.rs @@ -313,6 +313,9 @@ pub struct App { /// Whether the **last** `apply_effects` batch stopped early. Complete only /// for a one-effect batch — see `context/code/command.md`. batch_aborted: bool, + /// Whether an effect in the **last** batch refused what it was asked, through + /// [`App::refuse`]. Not an abort: `context/code/cli.md` §10. + batch_refused: bool, /// **Session-local**: how many times the whole document has been replaced. /// Bumped by `install_workbook` and nothing else (improvise-cqq). document_generation: u64, @@ -354,6 +357,7 @@ pub struct AppSnapshot { view_state: ViewState, abort_effects: bool, batch_aborted: bool, + batch_refused: bool, last_reshaped_views: std::collections::BTreeSet, last_reframed_views: std::collections::BTreeSet, written_view_frames: std::collections::BTreeMap, @@ -401,6 +405,7 @@ impl App { layout, abort_effects: false, batch_aborted: false, + batch_refused: false, document_generation: 0, layout_version: 0, cursor_epoch: 0, @@ -450,6 +455,19 @@ impl App { self.batch_aborted } + /// Did an effect in the last batch refuse what it was asked? The question an + /// exit code asks, which `last_batch_aborted` does not answer (improvise-c5ip). + pub fn last_batch_refused(&self) -> bool { + self.batch_refused + } + + /// Say why the requested change did not happen, and record that it did not. + /// The one writer of `batch_refused`, so a refusal cannot forget the flag. + pub fn refuse(&mut self, msg: impl Into) { + self.view_state.status_msg = msg.into(); + self.batch_refused = true; + } + /// The document and its basis tiers, kept so a failed frame can put them /// back (ADR 001 clause 4). See `context/code/command.md`. pub fn snapshot(&self) -> AppSnapshot { @@ -462,6 +480,7 @@ impl App { view_state: self.view_state.clone(), abort_effects: self.abort_effects, batch_aborted: self.batch_aborted, + batch_refused: self.batch_refused, last_reshaped_views: self.last_reshaped_views.clone(), last_reframed_views: self.last_reframed_views.clone(), written_view_frames: self.written_view_frames.clone(), @@ -480,6 +499,7 @@ impl App { self.view_state = snapshot.view_state; self.abort_effects = snapshot.abort_effects; self.batch_aborted = snapshot.batch_aborted; + self.batch_refused = snapshot.batch_refused; self.last_reshaped_views = snapshot.last_reshaped_views; self.last_reframed_views = snapshot.last_reframed_views; self.written_view_frames = snapshot.written_view_frames; @@ -817,6 +837,7 @@ impl App { pub fn apply_effects(&mut self, effects: Vec>) -> Dirty { self.abort_effects = false; self.batch_aborted = false; + self.batch_refused = false; // The `Dirty` is read off the `EffectKind`; the two lower basis tiers are // **observed** around the loop (improvise-75ld, improvise-pdvm). let touches_model = effects diff --git a/crates/improvise-command/src/ui/effect.rs b/crates/improvise-command/src/ui/effect.rs index 62a4f6a0..e7060309 100644 --- a/crates/improvise-command/src/ui/effect.rs +++ b/crates/improvise-command/src/ui/effect.rs @@ -83,7 +83,7 @@ impl Effect for AddCategory { match app.model_state.workbook.add_category(&self.0) { Ok(_) => {} Err(e) => { - app.view_state.status_msg = e.to_string(); + app.refuse(e.to_string()); app.abort_effects = true; } } @@ -105,7 +105,7 @@ impl Effect for AddItem { .workbook .add_item(&self.category, &self.item) { - app.view_state.status_msg = e.to_string(); + app.refuse(e.to_string()); } } fn kind(&self) -> EffectKind { @@ -126,7 +126,7 @@ impl Effect for AddItemInGroup { .workbook .add_item_in_group(&self.category, &self.item, &self.group) { - app.view_state.status_msg = e.to_string(); + app.refuse(e.to_string()); } } fn kind(&self) -> EffectKind { @@ -192,7 +192,7 @@ impl Effect for AddFormula { app.model_state.workbook.model.add_formula(formula); } Err(e) => { - app.view_state.status_msg = format!("Formula error: {e}"); + app.refuse(format!("Formula error: {e}")); } } } @@ -558,6 +558,19 @@ impl Effect for ReturnToNormalUnlessMoved { simple_effect!(SetStatus(String) => |app| app.view_state.status_msg, EffectKind::ViewOnly); +/// A command's own refusal: the status line, plus the record that nothing it was +/// asked for happened. [`SetStatus`] is for reports — `context/code/cli.md` §10. +#[derive(Debug)] +pub struct Refuse(pub String); +impl Effect for Refuse { + fn apply(&self, app: &mut App) { + app.refuse(self.0.clone()); + } + fn kind(&self) -> EffectKind { + EffectKind::ViewOnly + } +} + simple_effect!(SetYanked(Option) => |app| app.view_state.yanked, EffectKind::ViewOnly); simple_effect!(SetSearchQuery(String) => |app| app.view_state.search_query, EffectKind::ViewOnly); @@ -656,7 +669,7 @@ fn apply_measure_edit(app: &mut App, key: &CellKey, new_value: &str) { /// one per measure — so every one of them moves (improvise-c5v). fn apply_coordinate_rename(app: &mut App, row: &RecordRow, category: &str, new_value: &str) { if new_value.is_empty() { - app.view_state.status_msg = RECORD_COORDS_CANNOT_BE_EMPTY.to_string(); + app.refuse(RECORD_COORDS_CANNOT_BE_EMPTY); return; } let keys: Vec = if row.values.is_empty() { @@ -726,7 +739,7 @@ fn report_file_write(app: &mut App, outcome: Result) { match outcome { Ok(msg) => app.view_state.status_msg = msg, Err(msg) => { - app.view_state.status_msg = msg; + app.refuse(msg); app.abort_effects = true; } } @@ -878,11 +891,11 @@ impl Effect for StartImportWizard { app.view_state.mode = AppMode::ImportWizard; } Err(e) => { - app.view_state.status_msg = format!("JSON parse error: {e}"); + app.refuse(format!("JSON parse error: {e}")); } }, Err(e) => { - app.view_state.status_msg = format!("Cannot read file: {e}"); + app.refuse(format!("Cannot read file: {e}")); } } } @@ -922,7 +935,7 @@ impl Effect for LoadModel { app.view_state.status_msg = format!("Loaded from {}", self.0.display()); } Err(e) => { - app.view_state.status_msg = format!("Load error: {e}"); + app.refuse(format!("Load error: {e}")); } } } @@ -980,8 +993,7 @@ impl Effect for ImportJsonHeadless { let bytes = match app.storage.read(&self.path) { Ok(b) => b, Err(e) => { - app.view_state.status_msg = - format!("Cannot read '{}': {e}", self.path.display()); + app.refuse(format!("Cannot read '{}': {e}", self.path.display())); return; } }; @@ -992,7 +1004,7 @@ impl Effect for ImportJsonHeadless { (recs, raw) } Err(e) => { - app.view_state.status_msg = format!("CSV error: {e}"); + app.refuse(format!("CSV error: {e}")); return; } } @@ -1000,22 +1012,21 @@ impl Effect for ImportJsonHeadless { let content = match app.storage.read(&self.path) { Ok(c) => c, Err(e) => { - app.view_state.status_msg = - format!("Cannot read '{}': {e}", self.path.display()); + app.refuse(format!("Cannot read '{}': {e}", self.path.display())); return; } }; let value: serde_json::Value = match serde_json::from_slice(&content) { Ok(v) => v, Err(e) => { - app.view_state.status_msg = format!("JSON parse error: {e}"); + app.refuse(format!("JSON parse error: {e}")); return; } }; match json_import_records(&value, self.array_path.as_deref()) { Ok(recs) => (recs, value), Err(msg) => { - app.view_state.status_msg = msg; + app.refuse(msg); return; } } @@ -1054,7 +1065,7 @@ impl Effect for ImportJsonHeadless { }; } Err(e) => { - app.view_state.status_msg = format!("Import error: {e}"); + app.refuse(format!("Import error: {e}")); } } } @@ -1138,6 +1149,10 @@ pub fn set_status(msg: impl Into) -> Box { Box::new(SetStatus(msg.into())) } +pub fn refuse(msg: impl Into) -> Box { + Box::new(Refuse(msg.into())) +} + pub fn change_mode(mode: AppMode) -> Box { Box::new(ChangeMode(mode)) } @@ -2053,6 +2068,41 @@ mod tests { assert_eq!(app.view_state.status_msg, "next-batch"); } + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_refusal_marks_only_its_own_batch() { + let mut app = test_app(); + app.apply_effects(vec![Box::new(AddItem { + category: "Nonexistent".into(), + item: "Thing".into(), + })]); + assert!(app.last_batch_refused(), "{:?}", app.view_state.status_msg); + app.apply_effects(vec![Box::new(SetStatus("a report".into()))]); + assert!( + !app.last_batch_refused(), + "a report after a refusal inherited the refusal" + ); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_chain_with_nowhere_to_go_aborts_without_refusing() { + let mut app = test_app(); + app.apply_effects(vec![Box::new(AbortChain)]); + assert!(app.last_batch_aborted(), "the fixture must abort"); + assert!(!app.last_batch_refused()); + } + + /// Evidence: `context/test-evidence/cli.md`. + #[test] + fn a_failed_save_is_a_refusal_as_well_as_an_abort() { + let mut app = test_app(); + assert!(app.model_state.file_path.is_none()); + app.apply_effects(vec![Box::new(Save)]); + assert!(app.last_batch_aborted()); + assert!(app.last_batch_refused()); + } + /// BUG (improvise-8tk): `Save` reported failure only to the status bar, so /// the chained `Quit` landed anyway and the session was discarded. #[test] diff --git a/src/main.rs b/src/main.rs index a29a9603..06869e9d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -381,7 +381,9 @@ fn run_headless_commands(cmds: &[String], file: &Option) -> Result<()> if !said.trim().is_empty() { eprintln!("{said}"); } - if app.last_batch_aborted() { + // Refused, not aborted: an advance with nowhere to go aborts after + // its write landed (improvise-c5ip) — `context/code/cli.md` §10. + if app.last_batch_refused() { exit_code = 1; } } diff --git a/tests/headless_cli.rs b/tests/headless_cli.rs index df00cddc..b40437d0 100644 --- a/tests/headless_cli.rs +++ b/tests/headless_cli.rs @@ -498,3 +498,53 @@ fn a_script_writes_the_same_record_whatever_terminal_ran_it() { "the same script hid a different record at a different terminal size" ); } + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_commit_whose_advance_has_nowhere_to_go_still_exits_zero() { + let scratch = Scratch::new("commit-tail"); + let doc = three_cells(&scratch, "tail.improv"); + let out = run_cmd( + &["move-selection 2 0 . commit-cell-edit editing 9 Region/South"], + &doc, + ); + assert_eq!( + value_of(&doc, "South").as_deref(), + Some("9"), + "the write did not land" + ); + assert!( + out.status.success(), + "the write landed and was saved, yet the run exited non-zero: {:?}", + stderr_of(&out) + ); + + let empty = scratch.path("empty.improv"); + let out = run_cmd( + &["add-category Foo . commit-cell-edit editing 5 Foo/x"], + &empty, + ); + assert!( + out.status.success(), + "a commit on an empty grid landed and exited non-zero: {:?}", + stderr_of(&out) + ); +} + +/// Evidence: `context/test-evidence/cli.md`. +#[test] +fn a_refusal_that_only_writes_a_status_exits_non_zero() { + let scratch = Scratch::new("refusal-exit"); + let doc = seeded(&scratch, "m.improv"); + let out = run_cmd(&["add-item Nonexistent Thing"], &doc); + assert!( + stderr_of(&out).contains("Nonexistent"), + "the refusal was not reported, so its exit code proves nothing: {:?}", + stderr_of(&out) + ); + assert!( + !out.status.success(), + "a refused command exited 0: {:?}", + stderr_of(&out) + ); +} From 8d51ad70f27e1cf96356c8c24b9d521183d3a2c4 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 21:31:01 -0700 Subject: [PATCH 3/3] Give the Refuse effect its wire verdict: its text travels as SetStatus MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gh/main's vocabulary_is_complete requires every effect to be on the wire or named with a verdict. Refuse's portable half is its status text; the flag is one host batch's outcome (context/code/cli.md §10). Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- .../improvise-command/src/ui/reduce/vocabulary_is_complete.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index b91ed80d..1f8fa1f8 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -26,6 +26,8 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ ("HelpPageNext", Verdict::Command("HelpPage")), ("HelpPagePrev", Verdict::Command("HelpPage")), ("HelpPageSet", Verdict::Command("HelpPage")), + // Its text; the refusal flag is one host batch's outcome (context/code/cli.md §10). + ("Refuse", Verdict::Command("SetStatus")), ("Save", Verdict::Frame("Save")), ( "SaveAs",