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