From 41834724c4c24f851bf862ffbc9f424f24db26e2 Mon Sep 17 00:00:00 2001 From: dubemoyibe-star Date: Sun, 13 Sep 2026 21:33:09 +0100 Subject: [PATCH 1/2] Fix TUI panic-safety and doc-accuracy gaps found in final QA pass --- docs/decisions/sandboxing.md | 6 +-- docs/reference/tui.md | 14 +++++-- sendra-cli/tests/version_and_help.rs | 62 ++++++++++++++++++++++++++++ sendra-tui/src/lib.rs | 13 +++++- sendra-tui/src/oauth_login.rs | 47 ++++++++++++++++++++- sendra-tui/src/run_request.rs | 2 +- 6 files changed, 134 insertions(+), 10 deletions(-) create mode 100644 sendra-cli/tests/version_and_help.rs diff --git a/docs/decisions/sandboxing.md b/docs/decisions/sandboxing.md index ee7ab3b..7787efa 100644 --- a/docs/decisions/sandboxing.md +++ b/docs/decisions/sandboxing.md @@ -30,6 +30,6 @@ the ones that explain the throw. That choice belongs to each front end, not to the library. `sendra-core` has no `println!` or `eprintln!` anywhere in it — running a script returns the lines it printed alongside its verdict, and `sendra-cli` decides they are stderr. -`sendra-tui` reuses the same crate and puts them somewhere a redrawn frame does -not wipe out, without the library ever writing over either front end's own -interface. +`sendra-tui` does not run `pre_request`/`post_request` scripts at all — see +[The interactive TUI](../reference/tui.md) — so this choice does not arise +there. diff --git a/docs/reference/tui.md b/docs/reference/tui.md index 89cbeaa..73b1f92 100644 --- a/docs/reference/tui.md +++ b/docs/reference/tui.md @@ -3,9 +3,10 @@ `sendra tui` opens a full-screen terminal app for browsing and running the requests in a collection (or a single request file), instead of one shell invocation per request. It reuses `sendra-core` directly — the same request -model, YAML loading, environment resolution, scripting, capture and HTTP -execution `run`/`test` use — so a collection behaves identically whether you -send it from the TUI or from the command line. +model, YAML loading, environment resolution, capture and HTTP execution +`run`/`test` use — with one exception: `pre_request`/`post_request` scripts +are not run from the TUI (see below). Everything else behaves identically +whether you send it from the TUI or from the command line. ## Installing and launching @@ -85,6 +86,13 @@ A request's status, once it has run, shows in the detail pane alongside its headers and body. Requests that have never been run yet just show their resolved request preview. +A request's `pre_request`/`post_request` scripts, if it has any, are **not** +run when sent from the TUI — only `sendra run`/`sendra test` run them. Every +other part of the send pipeline (environment substitution, config header +merging, auth/query/body resolution, assertions, capture) is identical +either way; this is the one honest gap, not a silent one. See +[Scripting](scripting.md). + ## Editing a request `i` opens the selected request in an edit form covering every part of the diff --git a/sendra-cli/tests/version_and_help.rs b/sendra-cli/tests/version_and_help.rs new file mode 100644 index 0000000..d5d701a --- /dev/null +++ b/sendra-cli/tests/version_and_help.rs @@ -0,0 +1,62 @@ +//! Proves `--version` and `--help` work through the compiled `sendra` +//! binary, including for the `tui` subcommand — clap derives both from +//! `Cli`'s `#[command(version, about)]` and each `Command` variant's doc +//! comments (see `sendra-cli/src/cli.rs`), but nothing else in this crate's +//! test suite spawns the binary with either flag, so a regression that broke +//! one (a missing `version` attribute, a `Tui` variant losing its doc +//! comment) would otherwise go unnoticed until someone ran it by hand. + +use std::process::Command; + +fn run(args: &[&str]) -> (bool, String, String) { + let output = Command::new(env!("CARGO_BIN_EXE_sendra")) + .args(args) + .output() + .expect("the compiled sendra binary must launch"); + ( + output.status.success(), + String::from_utf8(output.stdout).expect("stdout must be valid UTF-8"), + String::from_utf8(output.stderr).expect("stderr must be valid UTF-8"), + ) +} + +#[test] +fn version_prints_the_binary_name_and_cargo_version() { + let (ok, stdout, _) = run(&["--version"]); + assert!(ok, "`--version` must exit successfully"); + assert_eq!( + stdout.trim(), + format!("sendra {}", env!("CARGO_PKG_VERSION")) + ); + + // clap also offers the short form. + let (ok, stdout_short, _) = run(&["-V"]); + assert!(ok, "`-V` must exit successfully"); + assert_eq!(stdout_short, stdout, "`-V` and `--version` must agree"); +} + +#[test] +fn top_level_help_lists_every_subcommand() { + let (ok, stdout, _) = run(&["--help"]); + assert!(ok, "`--help` must exit successfully"); + for subcommand in ["run", "test", "init", "schema", "import", "tui"] { + assert!( + stdout.contains(subcommand), + "top-level --help must list `{subcommand}`: {stdout}" + ); + } + assert!( + stdout.contains("launch the interactive TUI") || stdout.contains("no subcommand"), + "top-level --help must mention the bare-invocation TUI shortcut: {stdout}" + ); +} + +#[test] +fn tui_help_documents_its_optional_path() { + let (ok, stdout, _) = run(&["tui", "--help"]); + assert!(ok, "`sendra tui --help` must exit successfully"); + assert!( + stdout.contains("PATH"), + "`sendra tui --help` must document the optional path argument: {stdout}" + ); +} diff --git a/sendra-tui/src/lib.rs b/sendra-tui/src/lib.rs index efbf3ce..598cb51 100644 --- a/sendra-tui/src/lib.rs +++ b/sendra-tui/src/lib.rs @@ -167,11 +167,20 @@ fn install_panic_hook() { })); } +/// Unwinds its own partial progress on failure, rather than relying on the +/// caller: `run` calls this with `?`, which on a mid-setup error propagates +/// immediately, before `run`'s own `restore_terminal()` (guarding +/// `event_loop`) is ever reached — so raw mode and the alternate screen have +/// to be left exactly as this function found them on every `Err` path, not +/// just the first one. fn init_terminal() -> io::Result>> { enable_raw_mode()?; let mut stdout = io::stdout(); - execute!(stdout, EnterAlternateScreen)?; - Terminal::new(CrosstermBackend::new(stdout)) + if let Err(err) = execute!(stdout, EnterAlternateScreen) { + let _ = disable_raw_mode(); + return Err(err); + } + Terminal::new(CrosstermBackend::new(stdout)).inspect_err(|_| restore_terminal()) } /// The only place allowed to touch crossterm event types directly — translates diff --git a/sendra-tui/src/oauth_login.rs b/sendra-tui/src/oauth_login.rs index f58033f..1a981bb 100644 --- a/sendra-tui/src/oauth_login.rs +++ b/sendra-tui/src/oauth_login.rs @@ -57,17 +57,43 @@ pub enum LoginOutcome { /// message, and every path through [`run_login`] below reaches its one /// `on_complete` call rather than leaving the caller's waiting state stuck /// forever. +/// +/// **Always calls `on_complete` exactly once, even if `run_login` panics** — +/// wrapped in `catch_unwind` the same way [`crate::run_request::spawn`] +/// wraps `execute`, so a bug anywhere in the login pipeline cannot unwind +/// this thread out from under `on_complete` and leave the caller's +/// `OAuthLoginState` stuck at `WaitingForBrowser` forever. pub fn spawn( oauth: OAuthAuth, cache: Arc, on_complete: impl FnOnce(LoginOutcome) + Send + 'static, ) { std::thread::spawn(move || { - let outcome = run_login(&oauth, &cache); + let outcome = + login_catching_panics(std::panic::AssertUnwindSafe(|| run_login(&oauth, &cache))); on_complete(outcome); }); } +/// Runs `f`, catching any panic and turning it into `LoginOutcome::Failed` +/// instead of letting it unwind past this point — the mechanism [`spawn`] +/// relies on to guarantee `on_complete` always runs exactly once, panic or +/// not. Factored out from `spawn` itself, the same way +/// [`crate::run_request::run_catching_panics`] is, so this recovery +/// behavior is directly unit-testable with a closure that deliberately +/// panics, rather than only reachable by getting a real bug to misfire +/// somewhere inside `run_login`'s real network/browser pipeline. +fn login_catching_panics( + f: impl FnOnce() -> LoginOutcome + std::panic::UnwindSafe, +) -> LoginOutcome { + std::panic::catch_unwind(f).unwrap_or_else(|payload| { + LoginOutcome::Failed(format!( + "the login thread panicked: {}", + crate::run_request::panic_payload_message(&payload) + )) + }) +} + /// The flow itself: PKCE pair and CSRF `state` generated fresh for this one /// attempt, a local listener bound to `oauth.redirect_uri`, the browser /// opened to `oauth.authorization_url`, then a wait — bounded by @@ -290,6 +316,25 @@ mod tests { use std::io::Read; use std::net::TcpStream; + /// Proof requirement for `spawn`'s doc comment: a panic anywhere in + /// `run_login` must still produce a `LoginOutcome` rather than unwinding + /// past `spawn`'s thread closure, the same guarantee + /// `run_request::spawn` already has (and is tested for) — see that + /// crate's `a_panic_on_the_run_thread_still_completes_instead_of_hanging_forever`. + #[test] + fn a_panic_in_run_login_still_produces_a_failed_outcome_instead_of_hanging_forever() { + let outcome = login_catching_panics(|| panic!("simulated bug in the login pipeline")); + match outcome { + LoginOutcome::Failed(message) => { + assert!( + message.contains("simulated bug in the login pipeline"), + "the panic message must survive into the outcome: {message}" + ); + } + LoginOutcome::Success => panic!("a panic must never read as a successful login"), + } + } + #[test] fn redirect_bind_addr_resolves_loopback_host_and_port() { let addr = redirect_bind_addr("http://127.0.0.1:8899/callback").expect("valid loopback"); diff --git a/sendra-tui/src/run_request.rs b/sendra-tui/src/run_request.rs index 0a03ba9..0fed9e0 100644 --- a/sendra-tui/src/run_request.rs +++ b/sendra-tui/src/run_request.rs @@ -181,7 +181,7 @@ fn run_catching_panics(f: impl FnOnce() -> RunOutcome + std::panic::UnwindSafe) /// genuinely is one, while the explicit `&*payload` deref downcasts /// correctly. Verified with a minimal repro outside this crate before /// settling on this signature, not assumed. -fn panic_payload_message(payload: &Box) -> String { +pub(crate) fn panic_payload_message(payload: &Box) -> String { let payload: &(dyn std::any::Any + Send) = &**payload; if let Some(message) = payload.downcast_ref::<&str>() { (*message).to_string() From 3f3b6f925861932404643d4cca4ca9447d926f66 Mon Sep 17 00:00:00 2001 From: dubemoyibe-star Date: Sun, 13 Sep 2026 21:38:36 +0100 Subject: [PATCH 2/2] trigger workflow --- sendra-tui/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sendra-tui/src/lib.rs b/sendra-tui/src/lib.rs index 598cb51..6eef84e 100644 --- a/sendra-tui/src/lib.rs +++ b/sendra-tui/src/lib.rs @@ -170,7 +170,7 @@ fn install_panic_hook() { /// Unwinds its own partial progress on failure, rather than relying on the /// caller: `run` calls this with `?`, which on a mid-setup error propagates /// immediately, before `run`'s own `restore_terminal()` (guarding -/// `event_loop`) is ever reached — so raw mode and the alternate screen have +/// `event_loop`) is ever reached, so raw mode and the alternate screen have /// to be left exactly as this function found them on every `Err` path, not /// just the first one. fn init_terminal() -> io::Result>> {