improvise-up8g: a headless run's extent is fixed, not read off the terminal
`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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Generated
+1
@@ -845,6 +845,7 @@ dependencies = [
|
||||
"improvise-protocol",
|
||||
"improvise-wasm-client",
|
||||
"indexmap",
|
||||
"libc",
|
||||
"pest",
|
||||
"pest_derive",
|
||||
"pest_meta",
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
+6
-4
@@ -329,15 +329,17 @@ fn get_import_data(paths: &[PathBuf]) -> Option<Value> {
|
||||
|
||||
// ── 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<PathBuf>) -> 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`.
|
||||
|
||||
@@ -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<String> = (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"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user