diff --git a/SECURITY.md b/SECURITY.md index d6bf0ed8..aa46066a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -299,9 +299,9 @@ architecture the filter gates on. the question, ends it at the first question rather than answering it ([#225](https://github.com/danczw/sandbx/issues/225)). A pane too short for the whole question shows its first and last rows with a `…` row between, one - too small even for that counts as unable to show it, and a resize while a - question waits is not redrawn until it is answered - ([#508](https://github.com/danczw/sandbx/issues/508)). + too small even for that counts as unable to show it. A resize while one waits + redraws it within about a tenth of a second. A key pressed in that gap still + answers the old frame, and one pressed once the redraw begins does not. - **A terminal that goes away *during* a run is fail-closed and noticed.** The read fails rather than returning an answer, so that call and the ones behind it are refused, no further request is sent, and the process exits 3 rather diff --git a/context/decision-approval-gate.md b/context/decision-approval-gate.md index 3a4b5542..8bda3192 100644 --- a/context/decision-approval-gate.md +++ b/context/decision-approval-gate.md @@ -316,6 +316,7 @@ approve → Modal::ask 1 draw the question on the screen 2 Answers::arm ───────────────────────► open the question (see below) 3 Answers::wait (Condvar, parked) ◄── y / n / a · esc / ctrl-c · hangup + a resize wakes the wait: back to 1 4 take the question down, map the reply ``` @@ -335,17 +336,21 @@ descriptor no task feeds, and nothing else is in flight during `approve`, since - **Drawn, then armed.** The other order lets a key pressed before the question was visible answer it. A genuine press in the gap between the two is lost, and the operator presses again: the wrong way round costs a keystroke, not a - consent. + consent. A resize is the same gap again: the thread that would repaint is the + one parked, so `wait` returns `Reply::Resized` and `ask` draws and arms once + more, and a key pressed against the old frame is forgotten with it. The + reader looks for a resize every 100 ms because crossterm reports `SIGWINCH` + only from inside its own poll; a key pressed in that gap still answers the + old frame. - **Drawn means drawn.** A question the screen could not take, a draw that has failed or a lock a panic poisoned, is not asked: a key pressed meanwhile would answer something nobody saw, so the gate aborts instead of waiting. - **A short pane cuts the middle.** The rows naming the call and the row saying what the keys do are the two a question cannot lose, so a pane too short for all of it keeps those and marks the cut with a `…` row. The transcript above - it goes first. A pane that cannot keep both is not asked on: `question_fits` - says so, and the gate aborts as for a draw that failed. A resize while a - question waits is not redrawn (#508): the thread that paints is the one - parked. + it goes first. A pane that cannot keep both, at the first draw or after a + resize, is not asked on: `question_fits` says so, and the gate aborts as for + a draw that failed. - **The question opens when the reader is quiet.** The reader may already hold events it took off the terminal, and those predate the question. `arm` does not reach into crossterm for them: it marks the question pending, and the diff --git a/crates/sandbx-cli/src/tui_modal.rs b/crates/sandbx-cli/src/tui_modal.rs index c1f9d3de..27f39fa4 100644 --- a/crates/sandbx-cli/src/tui_modal.rs +++ b/crates/sandbx-cli/src/tui_modal.rs @@ -78,28 +78,33 @@ impl Operator for Modal { match self.replies.stopped() { Some(Reply::Interrupted) => return deny(INTERRUPTED), Some(Reply::Closed) => return abort(CLOSED), - Some(Reply::Key(_)) | None => {} + Some(Reply::Key(_) | Reply::Resized) | None => {} } if self.consent.covers(call.tool) { return ApprovalDecision::Allow; } let [head, tail] = question(call); - // Not asked over a question nobody saw: a key pressed meanwhile would answer it. - if !self.screen.show(&[&head, &tail]) { - self.screen.clear(); - return abort(UNDRAWN); - } + // Again after a resize, which leaves the first draw clipped or garbled at the new + // size: the thread that would repaint it is the one parked in `wait`. + let reply = loop { + // Not asked over a question nobody saw: a key pressed meanwhile would answer it. + if !self.screen.show(&[&head, &tail]) { + self.screen.clear(); + return abort(UNDRAWN); + } - // After the draw and never before: a key pressed while nothing was on screen to - // answer must not be taken for an answer to it, and `arm` is what forgets those. - let reply = match self.replies.arm(ANSWERS) { - Ok(()) => self.replies.wait(), - Err(_) => { + // After the draw and never before: a key pressed while nothing was on screen to + // answer must not be taken for an answer to it, and `arm` is what forgets those. + if self.replies.arm(ANSWERS).is_err() { self.screen.clear(); // Refused rather than asked anyway: the queue may already hold its answer. return abort(UNCLEARED); } + match self.replies.wait() { + Reply::Resized => {} + reply => break reply, + } }; self.screen.clear(); @@ -113,6 +118,8 @@ impl Operator for Modal { .unwrap_or_else(|| deny(REFUSED)) } Reply::Interrupted => deny(INTERRUPTED), + // Unreachable, the loop above taking it for a redraw; and not consent if reached. + Reply::Resized => deny(REFUSED), Reply::Closed => abort(CLOSED), } } diff --git a/crates/sandbx-cli/src/tui_modal_tests.rs b/crates/sandbx-cli/src/tui_modal_tests.rs index 02d1b8d6..2d510c4b 100644 --- a/crates/sandbx-cli/src/tui_modal_tests.rs +++ b/crates/sandbx-cli/src/tui_modal_tests.rs @@ -14,15 +14,17 @@ use super::{ANSWERS, Modal, Replies, Shows}; /// What happened, in order, across both fakes: the order is half of what is pinned. type Log = Rc>>; -/// Whether a question reaches the screen is the second field. -struct Screen(Log, bool); +/// How many more draws reach the screen is the second field. +struct Screen(Log, usize); impl Shows for Screen { fn show(&mut self, lines: &[&str]) -> bool { self.0 .borrow_mut() .push(format!("show {}", lines.join(" / "))); - self.1 + let reaches = self.1 > 0; + self.1 = self.1.saturating_sub(1); + reaches } fn clear(&mut self) { @@ -61,7 +63,7 @@ impl Replies for Keys { .borrow_mut() .pop_front() .expect("asked more often than the test scripted"); - if !matches!(reply, Reply::Key(_)) { + if matches!(reply, Reply::Interrupted | Reply::Closed) { self.stopped.set(Some(reply)); } reply @@ -81,7 +83,7 @@ fn modal(replies: &[Reply]) -> (Modal, Log) { stopped: std::cell::Cell::new(None), }; - (Modal::new(Screen(Rc::clone(&log), true), keys), log) + (Modal::new(Screen(Rc::clone(&log), usize::MAX), keys), log) } fn ask( @@ -129,6 +131,57 @@ fn the_question_is_drawn_then_armed_then_waited_on_then_taken_down() { ); } +/// The frame the operator is looking at is the one they answer: a resize puts the question +/// up again at the new size, and arms it only after, so a key pressed against the old +/// frame is not an answer to the new one. +#[test] +fn a_resize_redraws_the_question_and_asks_again() { + let (mut modal, log) = modal(&[Reply::Resized, Reply::Key('y')]); + + let verdict = ask(&mut modal, BuiltinTool::Write, &write()); + + assert_eq!(verdict, ApprovalDecision::Allow); + let log = log.borrow(); + let shown = "show sandbx: write /work/a.rs / sandbx: this call writes a file. allow it? \ + [y]es / [n]o / [a]ll `write` calls"; + assert_eq!( + *log, + [ + shown, + "arm ['y', 'n', 'a']", + "wait", + shown, + "arm ['y', 'n', 'a']", + "wait", + "clear", + ] + ); +} + +/// A pane resized too small for the question is one nobody can read it on, and is refused +/// as a question that could not be drawn at all. +#[test] +fn a_resize_that_leaves_no_room_for_the_question_aborts() { + let (mut modal, log) = modal(&[Reply::Resized]); + modal.screen.1 = 1; + + let verdict = ask(&mut modal, BuiltinTool::Write, &write()); + + match verdict { + ApprovalDecision::Abort { reason } => { + assert!(reason.contains("could not be drawn"), "{reason}"); + } + other => panic!("asked a question nobody could see: {other:?}"), + } + let log = log.borrow(); + assert_eq!( + log.iter().filter(|event| *event == "wait").count(), + 1, + "waited on a question that was no longer on screen: {log:?}" + ); + assert_eq!(log.last().map(String::as_str), Some("clear")); +} + #[test] fn an_n_refuses_and_tells_the_model_no_flag_lifts_it() { let (mut modal, _) = modal(&[Reply::Key('n')]); @@ -296,7 +349,7 @@ fn the_account_of_a_call_goes_where_the_question_was_asked() { #[test] fn a_question_that_could_not_be_drawn_is_never_asked() { let (mut modal, log) = modal(&[]); - modal.screen.1 = false; + modal.screen.1 = 0; let verdict = ask(&mut modal, BuiltinTool::Write, &write()); diff --git a/crates/sandbx-tui-input/public-api.txt b/crates/sandbx-tui-input/public-api.txt index 0269bd18..d947705f 100644 --- a/crates/sandbx-tui-input/public-api.txt +++ b/crates/sandbx-tui-input/public-api.txt @@ -3,6 +3,7 @@ pub enum sandbx_tui_input::Reply pub sandbx_tui_input::Reply::Closed pub sandbx_tui_input::Reply::Interrupted pub sandbx_tui_input::Reply::Key(char) +pub sandbx_tui_input::Reply::Resized impl core::clone::Clone for sandbx_tui_input::Reply pub fn sandbx_tui_input::Reply::clone(&self) -> sandbx_tui_input::Reply impl core::cmp::Eq for sandbx_tui_input::Reply diff --git a/crates/sandbx-tui-input/src/lib.rs b/crates/sandbx-tui-input/src/lib.rs index 2f992335..0134d54a 100644 --- a/crates/sandbx-tui-input/src/lib.rs +++ b/crates/sandbx-tui-input/src/lib.rs @@ -26,6 +26,12 @@ const GONE: PollFlags = PollFlags::POLLHUP .union(PollFlags::POLLERR) .union(PollFlags::POLLNVAL); +/// How long the reader blocks on the terminal before looking at crossterm's own signal source, +/// in milliseconds. crossterm reports a `SIGWINCH` as `Event::Resize` only from inside +/// `event::poll`, and a `poll` on the terminal alone is not woken by it: with no key typed +/// the resize would wait for one. This is the longest an operator waits for the redraw. +const RESIZE_LOOK: u16 = 100; + /// What the reader has seen so far; `stop` and `gone` are sticky, the channel's one slot /// losing either unread otherwise. #[derive(Debug, Clone, Copy, Default)] @@ -75,6 +81,10 @@ struct Slate { idle: bool, accepts: &'static [char], keys: VecDeque, + /// The terminal changed size since a wait last returned. Kept whether or not a question + /// is armed: one resized between its draw and its arm would otherwise leave the frame + /// it made stale on screen, and a spare redraw costs nothing. + resized: bool, /// The reader has exited, for whatever reason: nobody is left to answer. done: bool, } @@ -106,6 +116,11 @@ impl Wire { self.wake(); } + fn resized(&self) { + self.slate().resized = true; + self.wake.notify_all(); + } + fn pressed(&self, key: KeyEvent) { self.seen.send_modify(|seen| pressed(seen, key)); @@ -151,6 +166,10 @@ impl Wire { pub enum Reply { /// The operator pressed one of the keys asked for. Key(char), + /// The terminal changed size, so the question is no longer on screen as it was drawn. + /// Nothing typed so far is kept: it was an answer to the old frame. The caller is to + /// draw it again and [`Answers::arm`] again, in that order. + Resized, /// ctrl-c or escape, already latched for [`Keys::stop`]. Interrupted, /// Nobody can answer: the terminal hung up, or the reader stopped. @@ -227,7 +246,8 @@ impl Answers { /// /// No timeout, as a prompt on a terminal has none. An interrupt outranks an answer /// already pressed, and a hangup outranks both: both leave the call refused, which is - /// the direction a doubtful reading must lean. + /// the direction a doubtful reading must lean. A resize outranks an answer too, the + /// key having been pressed against a frame that is gone, and yields to both. #[must_use] pub fn wait(&self) -> Reply { let mut slate = self.wire.slate(); @@ -239,6 +259,9 @@ impl Answers { if seen.gone || slate.done { break Reply::Closed; } + if slate.resized { + break Reply::Resized; + } if let Some(answer) = slate.keys.pop_front() { break Reply::Key(answer); } @@ -252,6 +275,7 @@ impl Answers { slate.armed = false; slate.pending = false; + slate.resized = false; slate.keys.clear(); reply @@ -394,7 +418,7 @@ fn read(wire: &Wire, tty: BorrowedFd<'_>, out: BorrowedFd<'_>, source: &mut impl let wait = if buffered { PollTimeout::ZERO } else { - PollTimeout::NONE + PollTimeout::from(RESIZE_LOOK) }; // Quiet only across a blocking poll, and only once nothing is buffered: a question @@ -432,7 +456,11 @@ fn read(wire: &Wire, tty: BorrowedFd<'_>, out: BorrowedFd<'_>, source: &mut impl return; } } - // Resize, mouse, paste, focus: ignored; a redraw is driven by the event stream. + Ok(Some(Event::Resize(..))) => { + buffered = true; + wire.resized(); + } + // Mouse, paste, focus: ignored. Ok(Some(_)) => buffered = true, Ok(None) => buffered = false, Err(_) => return, diff --git a/crates/sandbx-tui-input/src/tui_input_tests.rs b/crates/sandbx-tui-input/src/tui_input_tests.rs index 4f6d82ba..13b0f617 100644 --- a/crates/sandbx-tui-input/src/tui_input_tests.rs +++ b/crates/sandbx-tui-input/src/tui_input_tests.rs @@ -549,3 +549,130 @@ fn a_hangup_already_seen_is_visible_without_waiting() { assert_eq!(answers.stopped(), Some(Reply::Closed)); } + +/// What was typed against the old frame is not an answer to the redrawn one, so a resize +/// ends the wait with the key forgotten, and the next arm starts clean. +#[test] +fn a_resize_wakes_a_wait_and_forgets_what_was_typed() { + let Pty { master: _m, slave } = pty(); + let answers = answers(slave); + answers.arm(&['y']).expect("armed"); + quiet(&answers); + answers.wire.pressed(typed('y')); + + answers.wire.resized(); + + assert_eq!(waited(&answers), Reply::Resized); + assert!( + answers.wire.slate().keys.is_empty(), + "a key typed against the old frame survived the resize" + ); + answers.arm(&['y']).expect("armed again"); + quiet(&answers); + answers.wire.pressed(typed('y')); + assert_eq!(waited(&answers), Reply::Key('y')); +} + +#[test] +fn an_interrupt_outranks_a_resize() { + let Pty { master: _m, slave } = pty(); + let answers = answers(slave); + answers.arm(&['y']).expect("armed"); + quiet(&answers); + answers.wire.resized(); + + answers + .wire + .pressed(key(KeyCode::Char('c'), KeyModifiers::CONTROL)); + + assert_eq!(waited(&answers), Reply::Interrupted); +} + +/// The window between the draw and the arm: a resize there finds nothing armed, and must +/// still be waiting for the wait, or the frame it left stale is the one that is answered. +#[test] +fn a_resize_before_the_arm_still_wakes_the_wait() { + let Pty { master: _m, slave } = pty(); + let answers = answers(slave); + answers.wire.resized(); + answers.arm(&['y']).expect("armed"); + quiet(&answers); + + assert_eq!(waited(&answers), Reply::Resized); +} + +/// Through the reader, which is what turns the terminal's resize into the wake. +#[test] +fn the_reader_reports_a_resize() { + struct Resizes(Vec>>); + + impl Source for Resizes { + fn next(&mut self) -> io::Result> { + self.0.remove(0) + } + } + + let Pty { master: _m, slave } = pty(); + let answers = answers(slave.try_clone().expect("a second handle")); + let _keys = answers.wire.seen.subscribe(); + let mut source = Resizes(vec![ + Ok(Some(Event::Resize(80, 24))), + Err(io::Error::from(io::ErrorKind::BrokenPipe)), + ]); + + read(&answers.wire, slave.as_fd(), slave.as_fd(), &mut source); + + assert!(answers.wire.slate().resized, "the resize was dropped"); +} + +/// crossterm turns a `SIGWINCH` into an `Event::Resize` only inside `event::poll`, which the +/// reader calls only after its own poll returns: with no key typed, a reader that blocks on +/// the terminal for good never hears of the resize. +#[test] +fn a_reader_with_nothing_typed_still_looks_for_a_resize() { + struct Looks(mpsc::Sender<()>); + + impl Source for Looks { + fn next(&mut self) -> io::Result> { + self.0.send(()).expect("the test is listening"); + Ok(None) + } + } + + let Pty { master: _m, slave } = pty(); + let answers = answers(slave.try_clone().expect("a second handle")); + let _keys = answers.wire.seen.subscribe(); + let (looked, looks) = mpsc::channel(); + let wire = Arc::clone(&answers.wire); + thread::spawn(move || read(&wire, slave.as_fd(), slave.as_fd(), &mut Looks(looked))); + + // Twice: the first look is the reader's own, before it first blocks. + for _ in 0..2 { + looks + .recv_timeout(Duration::from_secs(5)) + .expect("the reader never looked without a key"); + } +} + +/// The wake itself: a wait already parked is ended by the resize, not only one that starts +/// after it. +#[test] +fn a_resize_wakes_a_wait_already_parked() { + let Pty { master: _m, slave } = pty(); + let answers = answers(slave); + answers.arm(&['y']).expect("armed"); + quiet(&answers); + let (sent, received) = mpsc::channel(); + let waiting = answers.clone(); + thread::spawn(move || sent.send(waiting.wait()).expect("the test is listening")); + thread::sleep(Duration::from_millis(100)); + + answers.wire.resized(); + + assert_eq!( + received + .recv_timeout(Duration::from_secs(5)) + .expect("the wait never returned"), + Reply::Resized + ); +}