From 515eb8f744300f9815c3ef7e7797f7b4071850de Mon Sep 17 00:00:00 2001 From: judy Date: Tue, 11 Aug 2026 17:31:32 +0000 Subject: [PATCH] fix(tui): single-owner keyboard input, crash-safe terminal lifecycle, and diagnostics visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the three highest-impact defect clusters in the `adr-bot` TUI, plus adversarial-review and production-review hardening on top. 1. Unified keyboard input Two threads previously read the same crossterm event queue — a nav-only poller thread (alive through the project-confirm prompt) and the prompts' own blocking readers. Whichever reader won got the key, so at the confirm prompt 'y' (accept) could be consumed as "copy output" and Enter could be silently dropped. Replaced both with a single input-hub thread that owns event::read() for the whole run and routes by a shared flag: nav/quit/resize during execution, verbatim forwarding to the active prompt otherwise. Adjacent fixes delivered by the same architecture: - Cancellation outside tailoring: quit keys abort the API fetch and its 503 backoff immediately, and take effect at stage boundaries after the synchronous analysis/signals phases and before file writes. Previously nothing before tailoring could be cancelled. - Resize handling: prompt loops and review mode redraw immediately on resize (previously discarded); the 503 backoff sleeps in 1-second slices with a live countdown. 2. Crash-safe terminal lifecycle - Panic hook (installed once, when TUI mode starts): restores the terminal before delegating to the previous hook, so panic messages reach the normal screen instead of being erased by teardown. - try_setup_tui unwind: partial-failure branches undo exactly what they set up — no more raw mode silently stuck on after a fallback to plain mode. - Non-ASCII ADR-id panic: format_adr_diffs now truncates by character instead of byte-slicing &id[..8]. 3. Adversarial-review fixes Two independent review passes over the diff surfaced and confirmed: - Esc/fullscreen collision: the hint line advertises "Esc exit fullscreen", but the exec route treated Esc as a quit key — losing a whole run to a keypress the UI suggested. The renderer now mirrors fullscreen state to the hub; Esc exits fullscreen when active, cancels otherwise. - Sticky cancel latch: a quit key racing prompt entry latched cancellation forever, killing the run *after* the user explicitly accepted the prompt (reported as "API request failed: User cancelled"). The hub clears the latch on the exec-to-prompt transition, and fetch_error_message now reports a cancel as "Cancelled". - Stale nav commands leaking into prompts/review (could overflow review scroll arithmetic — debug-build panic): drained at prompt entry, plus saturating_add. - Drain-before-flip ordering in take_prompt_source (a fresh type-ahead key could be discarded as stale). - KeyEventKind::Release filtering (kitty protocol/Windows double-fire; Repeat still scrolls). - Plain-fallback hardening: a mid-run draw failure now stops the hub thread and clears the TUI-active flag so dialoguer prompts never compete with the hub. - Test-flake fix: the tests mutating the global TUI_ACTIVE are serialized. 4. Diagnostics visibility - Error paths no longer erase the transcript: the run wrapper holds the TUI open ("press any key to close") on failure as well as success, so subprocess stderr and API errors written to the log pane can actually be read. UserCancelled skips the pause; Plain/Quiet never block. - --verbose is visible in TUI mode: the four diagnostic sites previously wrote through suspend(|| eprintln!(...)) — a screen flash with no readable output. They now write to the log pane (TuiRenderer::eprintln, which also routes correctly to stderr in plain mode, preserving the documented stdout/stderr contract for scripted consumption). 5. ETXTBSY flake fix Both claude-binary spawn sites in auth.rs now retry on ETXTBSY ("Text file busy") — a transient fork/exec race that reddened CI intermittently and is unrelated to the TUI changes above. Also covers the production case of the claude binary being upgraded concurrently with an auth probe. 6. Production-hardening pass - Hub-thread fail-safe: if the input thread exits for any reason other than a requested stop (panic, or a future bug breaking the loop), it sets the cancel signal and logs an error before dying. Raw mode eats Ctrl+C's SIGINT while the TUI is live, so a silently dead input thread previously meant no cancel path at all. - Cancellation observability: tracing on the three decision points the hub introduced (quit-key latch set, latch cleared by a prompt opening, and a checkpoint consuming the cancel), so unexpected cancellations are debuggable from the log file. - Renderer-hub contract consolidated: the four separately-plumbed shared handles (route, fullscreen, stop, channels) become one HubShared / HubConnection handed over via a single connect_input_hub call, so the wiring cannot be half-done. Behavior changes to note: - q/Q/Esc/Ctrl+C cancel the run during Environment/Analysis/Fetch and write-prep (previously ignored there); Esc exits fullscreen first when fullscreen is active. Cancellation during synchronous stages lands at the next stage boundary; writes are never aborted mid-file. - On failure in TUI mode, the run pauses for a keypress before closing so the diagnostics stay readable. Testing: ~50 new unit tests across the hub (routing matrix incl. fullscreen/Release/latch semantics, wait_cancelled), renderer (resize in all prompt loops, prompt-channel take/restore/stale-drain, panic-hook restore, fullscreen mirror sync), pipeline (cancel_checkpoint, countdown backoff, cancelled-fetch message), auth (ETXTBSY retry), and a first co-located test module for adr_utils.rs. Full suite passes except pre-existing run-as-root artifacts (verified identical on the base commit). cargo fmt --check and cargo clippy -- -D warnings clean. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01V9Rz9BcK1xb1jiGNER1FeU --- src/cli/commands/auth.rs | 74 ++- src/cli/commands/sync/adr_utils.rs | 58 ++ src/cli/commands/sync/pipeline.rs | 281 +++++++--- src/cli/commands/sync_kb_poller.rs | 628 +++++++++++++++++----- src/cli/ui/diff.rs | 42 +- src/cli/ui/tui/renderer.rs | 830 ++++++++++++++++++++++++++++- 6 files changed, 1656 insertions(+), 257 deletions(-) diff --git a/src/cli/commands/auth.rs b/src/cli/commands/auth.rs index bdaaa1dc..fb370d76 100644 --- a/src/cli/commands/auth.rs +++ b/src/cli/commands/auth.rs @@ -64,6 +64,40 @@ fn build_tokio_runtime() -> Result { /// Inner async logic for [`check_auth_with_timeout`]. /// /// Separated for testability with `#[tokio::test]`. +/// `ETXTBSY`: exec of a file that some process still holds open for writing. +/// Same value (26) on Linux and macOS; Windows never produces it. +const ETXTBSY: i32 = 26; + +/// Attempts for [`spawn_with_etxtbsy_retry`]; the race window is +/// microseconds, so a couple of short retries always clears it. +const SPAWN_ATTEMPTS: u32 = 3; +const SPAWN_RETRY_DELAY: std::time::Duration = std::time::Duration::from_millis(20); + +/// Spawn with a brief retry on ETXTBSY ("Text file busy"). +/// +/// Between an executable being written and exec'd, a subprocess forked by +/// any other thread briefly inherits the writer's file descriptor and the +/// exec fails with ETXTBSY. The condition is always transient (routine in +/// the parallel test suite, which writes fake binaries while other tests +/// spawn processes; possible in production when the claude binary is being +/// updated concurrently), so retry before giving up. +async fn spawn_with_etxtbsy_retry( + cmd: &mut tokio::process::Command, + attempts: u32, + delay: std::time::Duration, +) -> std::io::Result { + let mut attempt = 0; + loop { + attempt += 1; + match cmd.spawn() { + Err(e) if e.raw_os_error() == Some(ETXTBSY) && attempt < attempts => { + tokio::time::sleep(delay).await; + } + result => return result, + } + } +} + async fn check_auth_async( binary_path: &Path, timeout: std::time::Duration, @@ -74,11 +108,11 @@ async fn check_auth_async( // When the timeout future is dropped, the Child is dropped. With // kill_on_drop(true) tokio sends SIGKILL, preventing orphaned processes. cmd.kill_on_drop(true); + cmd.stdout(std::process::Stdio::piped()); + cmd.stderr(std::process::Stdio::piped()); - let child = cmd - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() + let child = spawn_with_etxtbsy_retry(&mut cmd, SPAWN_ATTEMPTS, SPAWN_RETRY_DELAY) + .await .map_err(|e| ActualError::RunnerFailed { message: format!("failed to spawn claude: {e}"), stderr: String::new(), @@ -142,11 +176,11 @@ async fn check_auth_async_no_json( cmd.args(["auth", "status"]); cmd.stdin(std::process::Stdio::null()); cmd.kill_on_drop(true); + cmd.stdout(std::process::Stdio::piped()); + cmd.stderr(std::process::Stdio::piped()); - let child = cmd - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() + let child = spawn_with_etxtbsy_retry(&mut cmd, SPAWN_ATTEMPTS, SPAWN_RETRY_DELAY) + .await .map_err(|e| ActualError::RunnerFailed { message: format!("failed to spawn claude: {e}"), stderr: String::new(), @@ -238,6 +272,30 @@ mod tests { script } + /// ETXTBSY retry: while any handle holds the script open for writing, + /// exec deterministically fails with ETXTBSY (checked against all open + /// write descriptions, including our own) — so the retry loop runs to + /// exhaustion and surfaces the original error. + #[cfg(unix)] + #[tokio::test] + async fn test_spawn_retry_exhausts_on_persistent_etxtbsy() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let script = dir.path().join("busy-claude"); + std::fs::write(&script, "#!/bin/sh\nexit 0\n").unwrap(); + std::fs::set_permissions(&script, std::fs::Permissions::from_mode(0o755)).unwrap(); + let _writer = std::fs::OpenOptions::new() + .append(true) + .open(&script) + .unwrap(); + + let mut cmd = tokio::process::Command::new(&script); + let result = + spawn_with_etxtbsy_retry(&mut cmd, 3, std::time::Duration::from_millis(1)).await; + let err = result.expect_err("spawn must fail while a writer holds the script"); + assert_eq!(err.raw_os_error(), Some(ETXTBSY), "got: {err:?}"); + } + #[test] fn test_print_auth_status_authenticated() { let status = ClaudeAuthStatus { diff --git a/src/cli/commands/sync/adr_utils.rs b/src/cli/commands/sync/adr_utils.rs index 25b28a82..f70276f5 100644 --- a/src/cli/commands/sync/adr_utils.rs +++ b/src/cli/commands/sync/adr_utils.rs @@ -75,6 +75,9 @@ pub(crate) fn fetch_error_message(e: &ActualError, api_url: &str) -> String { ActualError::ApiResponseError { code, message } => { format!("Actual AI returned an error ({code}): {message}") } + // A quit key or SIGINT during the fetch is the user's decision, not + // an API failure — do not dress it up as one. + ActualError::UserCancelled => "Cancelled".to_string(), _ => format!("API request failed: {e}"), } } @@ -254,3 +257,58 @@ fn log_skip_invalid_project(project_path: &str, adr_id: &str) { let clean = console::strip_ansi_codes(project_path); tracing::warn!("skipping invalid project path '{clean}' for ADR '{adr_id}'"); } + +#[cfg(test)] +mod tests { + use super::*; + + // ── fetch_error_message ── + + #[test] + fn test_fetch_error_message_service_unavailable() { + let msg = fetch_error_message(&ActualError::ServiceUnavailable, "https://api.test"); + assert!(msg.contains("being updated"), "got: {msg}"); + } + + #[test] + fn test_fetch_error_message_connection_refused_mentions_url() { + let e = ActualError::ApiError("error trying to connect: Connection refused".to_string()); + let msg = fetch_error_message(&e, "https://api.test"); + assert!(msg.contains("https://api.test"), "got: {msg}"); + assert!(msg.contains("network"), "got: {msg}"); + } + + #[test] + fn test_fetch_error_message_timeout() { + let e = ActualError::ApiError("operation timed out".to_string()); + let msg = fetch_error_message(&e, "https://api.test"); + assert!(msg.contains("timed out"), "got: {msg}"); + } + + #[test] + fn test_fetch_error_message_api_response_error() { + let e = ActualError::ApiResponseError { + code: "500".to_string(), + message: "boom".to_string(), + }; + let msg = fetch_error_message(&e, "https://api.test"); + assert!(msg.contains("500") && msg.contains("boom"), "got: {msg}"); + } + + #[test] + fn test_fetch_error_message_user_cancelled_is_not_an_api_failure() { + let msg = fetch_error_message(&ActualError::UserCancelled, "https://api.test"); + assert_eq!(msg, "Cancelled"); + assert!( + !msg.contains("API request failed"), + "a user cancel must not be presented as an API failure" + ); + } + + #[test] + fn test_fetch_error_message_generic_fallback() { + let e = ActualError::ApiError("something else".to_string()); + let msg = fetch_error_message(&e, "https://api.test"); + assert!(msg.starts_with("API request failed:"), "got: {msg}"); + } +} diff --git a/src/cli/commands/sync/pipeline.rs b/src/cli/commands/sync/pipeline.rs index 26096137..a77c4709 100644 --- a/src/cli/commands/sync/pipeline.rs +++ b/src/cli/commands/sync/pipeline.rs @@ -10,6 +10,7 @@ use crate::analysis::types::{ use crate::api::client::{build_match_request, ActualApiClient, DEFAULT_API_URL}; use crate::api::retry::{with_retry, RetryConfig}; use crate::cli::args::SyncArgs; +use crate::cli::commands::sync_kb_poller::{setup_input_hub, wait_cancelled, InputHub}; use crate::cli::ui::confirm::format_project_summary_plain; use crate::cli::ui::header::{AuthDisplay, RunnerDisplay}; use crate::cli::ui::progress::SyncPhase; @@ -158,12 +159,62 @@ pub(crate) fn run_sync_with_probe( // banner box (rendered by the TUI directly), not in the log pane. let mut pipeline = TuiRenderer::new_with_version(false, args.no_tui, env!("CARGO_PKG_VERSION")); - // Start a nav-only keyboard poller so navigation (↑/↓, scroll, fullscreen) - // works during all phases, not just tailoring. - let (nav_poller, nav_rx) = super::super::sync_kb_poller::setup_nav_only(pipeline.is_tui()); - pipeline.set_nav_rx_opt(nav_rx); - let mut nav_poller = Some(nav_poller); + // Start the input hub: one background thread owns crossterm events for + // the whole run, routing navigation (↑/↓, scroll, fullscreen), prompt + // input, resize redraws, and quit/cancel keys. A single reader means + // prompts never race a poller thread for keystrokes. + let mut input = setup_input_hub(pipeline.is_tui()); + pipeline.connect_input_hub(input.connect()); + let result = run_sync_inner( + args, + root_dir, + cfg_path, + term, + runner, + auth_display, + runner_display, + runner_probe, + semgrep_check, + &mut pipeline, + &mut input, + ); + + // Keep the transcript readable before the alternate screen closes: on + // failure the log pane holds the diagnostics that were just written + // (subprocess stderr, API errors) — pause so the user can read, scroll, + // and copy them before teardown erases the pane, symmetric with the + // success-path review pause below (a contract the PTY e2e suite pins, + // including under --force). Skip the pause when the user cancelled — + // they asked to leave. Both pauses are no-ops in Plain/Quiet modes: + // `--no-tui` is the non-blocking path for pty-wrapped automation, which + // would otherwise present a real TTY and wait here forever. + match &result { + Ok(()) => pipeline.wait_for_keypress(), + Err(ActualError::UserCancelled) => {} + Err(_) => pipeline.pause_for_failure_review(), + } + result +} + +/// Body of [`run_sync_with_probe`], with the renderer and input hub owned by +/// the caller so the wrapper can hold the TUI open for transcript review on +/// both success and failure exits (previously error paths tore the TUI down +/// instantly, erasing the very diagnostics just written to the log pane). +#[allow(clippy::too_many_arguments)] +fn run_sync_inner( + args: &SyncArgs, + root_dir: &Path, + cfg_path: &Path, + term: &dyn TerminalIO, + runner: &R, + auth_display: Option<&AuthDisplay>, + runner_display: Option<&RunnerDisplay>, + runner_probe: Option Result<(), ActualError>>>, + semgrep_check: Option bool>>, + pipeline: &mut TuiRenderer, + input: &mut InputHub, +) -> Result<(), ActualError> { pipeline.start(SyncPhase::Environment, "Checking environment..."); // Load config early so we can show server URL and cache status in the @@ -324,6 +375,9 @@ pub(crate) fn run_sync_with_probe( // Blank line for visual breathing room. pipeline.println(""); + // A quit key during the (possibly long) semgrep download takes effect here. + cancel_checkpoint(input.is_cancelled(), pipeline)?; + // Runner pre-flight probe (runs during Environment phase, before Analysis). // Failures surface here rather than deep in the Tailor phase. if let Some(probe) = runner_probe { @@ -361,6 +415,10 @@ pub(crate) fn run_sync_with_probe( } }; + // Analysis is synchronous CPU-bound work; a quit key pressed during it + // takes effect at this stage boundary. + cancel_checkpoint(input.is_cancelled(), pipeline)?; + // 4. Filter by --project if specified let analysis = match filter_projects(analysis, &args.projects) { Ok(filtered) => filtered, @@ -387,7 +445,7 @@ pub(crate) fn run_sync_with_probe( pipeline.println(line); } } else { - confirm_or_change_loop(&mut analysis, &mut pipeline, term)?; + confirm_or_change_loop(&mut analysis, pipeline, term)?; } // ── Phase 2: fetch + tailor ── @@ -400,12 +458,13 @@ pub(crate) fn run_sync_with_probe( if args.reset_rejections { clear_rejections(&mut config, &repo_key); save_to(&config, cfg_path)?; - pipeline.suspend(|| { - eprintln!( - "{} Cleared ADR rejection memory for this repository", - theme::success(&theme::SUCCESS) - ); - }); + // eprintln (not suspend): suspend leaves and re-enters the alternate + // screen around output the user never gets to read — the log pane is + // the visible surface in TUI mode, stderr in plain mode. + pipeline.eprintln(&format!( + " {} Cleared ADR rejection memory for this repository", + theme::success(&theme::SUCCESS) + )); } let rejected_ids = get_rejections(&config, &repo_key); @@ -425,25 +484,27 @@ pub(crate) fn run_sync_with_probe( root_dir, &analysis, )); + // Signals analysis is another long synchronous stage; honor a quit key here. + cancel_checkpoint(input.is_cancelled(), pipeline)?; + let request = build_match_request(&analysis, &config, &signals); let client = ActualApiClient::new(&api_url)?; if args.verbose { - pipeline.suspend(|| { - eprintln!("API request to: {api_url}/adrs/match"); - eprintln!( - " projects: {}", - request - .projects - .iter() - .map(|p| p.name.as_str()) - .collect::>() - .join(", ") - ); - }); + pipeline.eprintln(&format!(" API request to: {api_url}/adrs/match")); + pipeline.eprintln(&format!( + " projects: {}", + request + .projects + .iter() + .map(|p| p.name.as_str()) + .collect::>() + .join(", ") + )); } let root_dir_owned = root_dir.to_path_buf(); + let cancel_token = input.cancel_token(); // Resolve the effective output format: CLI flag takes precedence over config, // and config takes precedence over the default. let output_format = args @@ -465,17 +526,16 @@ pub(crate) fn run_sync_with_probe( std::time::Duration::from_secs(60), ]; let retry_config = RetryConfig::default(); - let api_response = fetch_with_503_backoff( - || async { - tokio::select! { - result = with_retry(&retry_config, || client.post_match(&request)) => result, - _ = tokio::signal::ctrl_c() => Err(ActualError::UserCancelled), - } - }, - &DELAYS_503, - &mut pipeline, - ) - .await; + // The cancel future wraps the WHOLE fetch (attempts and backoff + // sleeps alike), so a quit key or SIGINT aborts even mid-backoff. + let api_response = tokio::select! { + result = fetch_with_503_backoff( + || with_retry(&retry_config, || client.post_match(&request)), + &DELAYS_503, + &mut *pipeline, + ) => result, + _ = wait_cancelled(cancel_token) => Err(ActualError::UserCancelled), + }; (api_response, fs_future.await) }); @@ -504,22 +564,20 @@ pub(crate) fn run_sync_with_probe( }; if args.verbose { - pipeline.suspend(|| { - eprintln!( - " matched: {}, by_framework: {:?}", - response.metadata.total_matched, response.metadata.by_framework - ); - }); + pipeline.eprintln(&format!( + " matched: {}, by_framework: {:?}", + response.metadata.total_matched, response.metadata.by_framework + )); } // 2c. Filter by rejections let filtered_adrs = pre_filter_rejected(&response.matched_adrs, &rejected_ids); if !rejected_ids.is_empty() && args.verbose { - pipeline.suspend(|| { - let removed = response.matched_adrs.len() - filtered_adrs.len(); - eprintln!(" filtered out {removed} previously rejected ADRs"); - }); + let removed = response.matched_adrs.len() - filtered_adrs.len(); + pipeline.eprintln(&format!( + " filtered out {removed} previously rejected ADRs" + )); } // 2d. Tailor or skip (--no-tailor), with caching @@ -621,19 +679,11 @@ pub(crate) fn run_sync_with_probe( let (event_tx, mut event_rx) = tokio::sync::mpsc::unbounded_channel::(); runner.set_event_tx(event_tx); - // Stop the nav-only poller before starting the tailoring poller - // (which also handles quit/cancel keys) to avoid two threads reading - // crossterm events simultaneously. - drop(nav_poller.take()); - pipeline.set_nav_rx_opt(None); - // In TUI mode (raw mode active) the terminal consumes keyboard input, // so OS-level SIGINT from Ctrl+C may not reach the process reliably. - // `sync_kb_poller::setup` optionally spawns a background poller thread - // and returns a combined OS-signal + keyboard cancel future, plus a - // nav channel receiver for arrow-key navigation during execution. - let (kb_poller, nav_rx, cancel) = super::super::sync_kb_poller::setup(pipeline.is_tui()); - pipeline.set_nav_rx_opt(nav_rx); + // The input hub's quit keys (q/Q/Esc/Ctrl+C) set the cancel signal; + // `wait_cancelled` combines it with OS SIGINT for non-TUI mode. + let cancel = wait_cancelled(input.cancel_token()); let result = rt.block_on(async { let tailor_fut = tailor_all_projects( @@ -647,15 +697,13 @@ pub(crate) fn run_sync_with_probe( tailor_fut, &mut progress_rx, &mut event_rx, - &mut pipeline, + &mut *pipeline, project_count, cancel, ) .await }); - // The keyboard poller (if started) is signalled to stop via its Drop impl. - drop(kb_poller); match result { Ok(output) => { pipeline.success( @@ -762,6 +810,12 @@ pub(crate) fn run_sync_with_probe( // edits) to avoid unnecessary writes. let output = crate::tailoring::minor_change::filter_minor_changes(output, root_dir); + // A quit key pressed during tailoring wind-down or diff preparation + // takes effect here, before anything is written. (Safe against a key + // racing the upcoming file prompt: the hub clears the cancel latch when + // a prompt opens, so an unanswered latch here is genuine exec intent.) + cancel_checkpoint(input.is_cancelled(), pipeline)?; + // ── Phase 3: confirm + write (fully implemented) ── // pipeline stays alive through confirm+write so output goes into the TUI // log pane. It drops naturally at end of run_sync. @@ -773,7 +827,7 @@ pub(crate) fn run_sync_with_probe( args.full, &output_format, term, - &mut pipeline, + pipeline, )?; // Keep `sync_result` alive when telemetry is compiled out. #[cfg(not(feature = "telemetry"))] @@ -856,13 +910,6 @@ pub(crate) fn run_sync_with_probe( } } - // Stop the nav poller (if still alive) before entering review mode, - // so its background thread doesn't compete for crossterm key events - // with `wait_for_keypress`'s direct key reader. - drop(nav_poller.take()); - pipeline.set_nav_rx_opt(None); - - pipeline.wait_for_keypress(); Ok(()) } @@ -889,16 +936,27 @@ where match result { Ok(v) => break Ok(v), Err(ActualError::ServiceUnavailable) if attempt_503 < delays_503.len() => { - let delay_secs = delays_503[attempt_503].as_secs(); - pipeline.update_message( - SyncPhase::Fetch, - &format!( - "Actual AI API is updating \u{2014} retrying in {delay_secs}s ({}/{})...", - attempt_503 + 1, - delays_503.len() - ), - ); - tokio::time::sleep(delays_503[attempt_503]).await; + // Sleep in 1 s slices so the wait stays responsive: each + // tick refreshes the spinner/elapsed display and drains nav + // commands (including resize redraws) via draw(). Note the + // countdown TEXT is currently discarded by update_message — + // the string documents intent for when messages surface. + const TICK: std::time::Duration = std::time::Duration::from_secs(1); + let mut remaining = delays_503[attempt_503]; + while !remaining.is_zero() { + pipeline.update_message( + SyncPhase::Fetch, + &format!( + "Actual AI API is updating \u{2014} retrying in {}s ({}/{})...", + remaining.as_secs(), + attempt_503 + 1, + delays_503.len() + ), + ); + let step = remaining.min(TICK); + tokio::time::sleep(step).await; + remaining = remaining.saturating_sub(step); + } attempt_503 += 1; } Err(e) => break Err(e), @@ -906,6 +964,21 @@ where } } +/// Abort between synchronous pipeline stages when the user pressed a quit key. +/// +/// Long CPU-bound stages (analysis, signals) cannot be interrupted mid-flight; +/// this makes a quit key pressed during them take effect at the next stage +/// boundary, mirroring the Reject path (finish steps, then `UserCancelled`). +fn cancel_checkpoint(cancelled: bool, pipeline: &mut TuiRenderer) -> Result<(), ActualError> { + if cancelled { + tracing::info!("run cancelled by user at stage boundary"); + pipeline.finish_remaining(); + Err(ActualError::UserCancelled) + } else { + Ok(()) + } +} + /// Return the pipeline skip message for a tailoring cache hit. /// /// Distinguishes between a cached result with no applicable ADRs (deterministic @@ -5592,10 +5665,12 @@ mod tests { } #[test] - fn test_fetch_error_message_user_cancelled_catch_all() { + fn test_fetch_error_message_user_cancelled_is_not_an_api_failure() { + // A quit key / SIGINT during the fetch is the user's decision; + // presenting it as an API failure was misleading. let e = ActualError::UserCancelled; let msg = fetch_error_message(&e, "https://api.example.com"); - assert!(msg.contains("API request failed:"), "unexpected msg: {msg}"); + assert_eq!(msg, "Cancelled", "unexpected msg: {msg}"); } #[test] @@ -7135,6 +7210,54 @@ mod tests { ); } + #[tokio::test(start_paused = true)] + async fn test_fetch_with_503_backoff_countdown_sleeps_in_slices() { + // A multi-second delay is slept in 1 s slices (live countdown); the + // retry must still happen after the full delay elapses. + let mut pipeline = TuiRenderer::new(false, true); + let delays: &[std::time::Duration] = &[std::time::Duration::from_secs(3)]; + let mut call_count = 0usize; + let result = super::fetch_with_503_backoff( + || { + call_count += 1; + let n = call_count; + async move { + if n == 1 { + Err(ActualError::ServiceUnavailable) + } else { + Ok(7u32) + } + } + }, + delays, + &mut pipeline, + ) + .await; + assert!( + result.is_ok(), + "expected Ok after countdown, got {result:?}" + ); + assert_eq!(call_count, 2, "fail once, then succeed after the wait"); + } + + // ── cancel_checkpoint tests ── + + #[test] + fn test_cancel_checkpoint_not_cancelled_is_ok() { + let mut pipeline = TuiRenderer::new(false, true); + assert!(super::cancel_checkpoint(false, &mut pipeline).is_ok()); + } + + #[test] + fn test_cancel_checkpoint_cancelled_returns_user_cancelled() { + let mut pipeline = TuiRenderer::new(false, true); + let result = super::cancel_checkpoint(true, &mut pipeline); + assert!( + matches!(result, Err(ActualError::UserCancelled)), + "expected UserCancelled, got {result:?}" + ); + } + // ── apply_content_budget tests ── #[test] diff --git a/src/cli/commands/sync_kb_poller.rs b/src/cli/commands/sync_kb_poller.rs index a373d978..fe3dc136 100644 --- a/src/cli/commands/sync_kb_poller.rs +++ b/src/cli/commands/sync_kb_poller.rs @@ -1,99 +1,203 @@ -/// Keyboard-cancel poller for TUI mode. +/// Unified keyboard/resize input hub for TUI mode. /// -/// During `Tailoring ADRs`, `rt.block_on()` holds the main thread while -/// crossterm raw mode is active. OS-level SIGINT from Ctrl+C may not fire -/// because the terminal intercepts keystrokes before the signal is delivered. -/// This module spawns a background thread that polls crossterm every 100 ms -/// for `q`, `Q`, `Esc`, or `Ctrl+C` and signals cancellation via a oneshot -/// channel. +/// Exactly one background thread owns `crossterm::event::read()` for the +/// whole sync run. Events are routed according to a shared route flag: +/// +/// - **Execution** ([`renderer::ROUTE_EXEC`]): navigation keys become +/// [`NavCmd`]s, quit keys (`q`/`Q`/`Esc`/`Ctrl+C`) set the cancel signal, +/// and resizes become [`NavCmd::Redraw`]. +/// - **Prompt** ([`renderer::ROUTE_PROMPT`]): every key and resize is +/// forwarded verbatim to the active prompt over the prompt channel. +/// +/// This replaces the previous design where a nav-only poller thread and the +/// prompts' own blocking readers competed for the same event queue, so a key +/// pressed at a prompt could be consumed (and dropped or misrouted as a +/// scroll/copy command) by the poller thread instead. +/// +/// Cancellation is a `tokio::sync::watch` channel: quit keys flip it to +/// `true`, [`wait_cancelled`] awaits it (combined with OS SIGINT), and +/// [`InputHub::is_cancelled`] allows checks between synchronous pipeline +/// stages. Raw mode prevents Ctrl+C from reliably delivering SIGINT while +/// the TUI is active, which is why the quit keys are handled here. /// /// Excluded from coverage measurement because the thread body only runs on a -/// real TTY — unit tests cannot drive crossterm event I/O. -use std::sync::{ - atomic::{AtomicBool, Ordering}, - Arc, -}; - -use tokio::sync::oneshot; +/// real TTY — unit tests cannot drive crossterm event I/O. The routing logic +/// is a pure function ([`route_event`]) and is unit-tested below. +use std::sync::{atomic::Ordering, Arc}; -use crate::cli::ui::tui::renderer::NavCmd; +use crate::cli::ui::tui::renderer::{self, HubConnection, HubShared, InputEvent, NavCmd}; -/// Guard returned by [`setup`]. Signals the poller thread to stop when -/// dropped. When no thread was started (non-TUI mode) this is a no-op. -pub struct KbPoller { - inner: Option, +/// Owner of the background input thread and the cancel signal. +/// +/// Dropping the hub signals the thread to stop and joins it. In non-TUI mode +/// no thread is spawned and every accessor returns an inert value. +pub struct InputHub { + inner: Option, + cancel_rx: tokio::sync::watch::Receiver, + /// Keeps the cancel channel's sender alive in non-TUI mode so + /// `wait_cancelled` treats the channel as "never fires" rather than + /// "sender dropped". + _cancel_tx: Option>, + /// The renderer's half of the hub, handed over once via [`Self::connect`]. + conn: Option, } -struct KbPollerInner { - stop: Arc, +struct HubInner { + shared: Arc, handle: std::thread::JoinHandle<()>, } -impl Drop for KbPoller { +impl Drop for InputHub { fn drop(&mut self) { if let Some(inner) = self.inner.take() { - inner.stop.store(true, Ordering::Relaxed); + inner.shared.stop.store(true, Ordering::Relaxed); let _ = inner.handle.join(); } } } -/// Set up a navigation-only keyboard poller (no quit/cancel handling). -/// -/// When `is_tui` is `true`, spawns a background crossterm-poller thread that -/// forwards navigation commands (arrow keys, scroll, copy, fullscreen) via -/// the returned `Receiver`. Quit keys are ignored. -/// -/// When `is_tui` is `false`, returns a no-op guard and `None`. -/// -/// Drop the returned [`KbPoller`] to stop the thread. -pub fn setup_nav_only(is_tui: bool) -> (KbPoller, Option>) { - if is_tui { - let (inner, nav_rx) = spawn_nav_thread(); - (KbPoller { inner: Some(inner) }, Some(nav_rx)) - } else { - (KbPoller { inner: None }, None) +impl InputHub { + /// Hand the renderer its half of the hub (channels + shared state) in a + /// single step. Returns `None` in non-TUI mode or if already connected. + pub(crate) fn connect(&mut self) -> Option { + self.conn.take() + } + + /// A clonable token that [`wait_cancelled`] can await. Each call returns + /// an independent receiver, so multiple pipeline stages can each build + /// their own cancel future. + pub fn cancel_token(&self) -> tokio::sync::watch::Receiver { + self.cancel_rx.clone() + } + + /// Whether a quit key has been pressed. Checked between synchronous + /// pipeline stages, where no future can be awaited. + pub fn is_cancelled(&self) -> bool { + *self.cancel_rx.borrow() } } -/// Set up the keyboard-cancel poller. -/// -/// When `is_tui` is `true`, spawns a background crossterm-poller thread and -/// returns a cancel future that fires on `q`/`Esc`/`Ctrl+C` **or** OS SIGINT. -/// Also returns a `Receiver` for navigation commands. -/// -/// When `is_tui` is `false`, returns a no-op guard and a cancel future that -/// only fires on OS SIGINT. -/// -/// Drop the returned [`KbPoller`] once tailoring completes; it will signal -/// the poller thread (if any) to stop. -pub fn setup( - is_tui: bool, -) -> ( - KbPoller, - Option>, - impl std::future::Future>, -) { - let (kb_poller, kb_cancel_rx, nav_rx) = if is_tui { - let (inner, rx, nav_rx) = spawn_thread(); - (KbPoller { inner: Some(inner) }, Some(rx), Some(nav_rx)) - } else { - (KbPoller { inner: None }, None, None) - }; +/// Spawn the input hub. In non-TUI mode (`is_tui == false`) no thread is +/// started: prompts read the terminal directly (dialoguer) and cancellation +/// falls back to OS SIGINT only. +pub fn setup_input_hub(is_tui: bool) -> InputHub { + let (cancel_tx, cancel_rx) = tokio::sync::watch::channel(false); + if !is_tui { + return InputHub { + inner: None, + cancel_rx, + _cancel_tx: Some(cancel_tx), + conn: None, + }; + } - let cancel_fut = async move { - tokio::select! { - r = tokio::signal::ctrl_c() => r, - r = async { - match kb_cancel_rx { - Some(rx) => rx.await.map_err(|_| std::io::Error::other("keyboard cancel channel closed")), - None => std::future::pending().await, - } - } => r, + let shared = Arc::new(HubShared::new()); + let (nav_tx, nav_rx) = std::sync::mpsc::channel::(); + let (key_tx, key_rx) = std::sync::mpsc::channel::(); + + let handle = spawn_input_thread(shared.clone(), nav_tx, key_tx, cancel_tx); + + InputHub { + inner: Some(HubInner { + shared: shared.clone(), + handle, + }), + cancel_rx, + _cancel_tx: None, + conn: Some(HubConnection { + nav_rx, + prompt_rx: key_rx, + shared, + }), + } +} + +/// Resolve when the user cancels: a quit key flips the watch channel, or the +/// OS delivers SIGINT (Ctrl+C outside raw mode, `kill -INT`, ...). +/// +/// If the watch sender is gone (hub dropped), the keyboard branch waits +/// forever rather than resolving spuriously. +pub async fn wait_cancelled(mut rx: tokio::sync::watch::Receiver) -> std::io::Result<()> { + let keyboard = async move { + loop { + if *rx.borrow() { + return; + } + if rx.changed().await.is_err() { + std::future::pending::<()>().await; + } } }; + tokio::select! { + r = tokio::signal::ctrl_c() => r, + _ = keyboard => Ok(()), + } +} - (kb_poller, nav_rx, cancel_fut) +/// Where the input thread should deliver one routed event. +#[derive(Debug)] +pub(crate) enum RoutedAction { + /// Deliver as a navigation command (execution route). + Nav(NavCmd), + /// Forward verbatim to the active prompt (prompt route). + Prompt(InputEvent), + /// Set the cancel signal (quit key on the execution route). + Cancel, + /// Discard (irrelevant event, e.g. focus/paste, or unbound key). + Ignore, +} + +/// Pure routing decision for one crossterm event given the current route +/// flag and fullscreen state. Extracted from the thread body so it can be +/// unit-tested. +/// +/// `fullscreen` matters only for Esc on the execution route: the hint line +/// advertises "Esc exit fullscreen", so while fullscreen is active Esc must +/// leave fullscreen, not cancel the whole run. +pub(crate) fn route_event( + route: u8, + fullscreen: bool, + event: &crossterm::event::Event, +) -> RoutedAction { + use crossterm::event::{Event, KeyCode, KeyEventKind, KeyModifiers}; + match event { + Event::Key(key) => { + // Terminals using the kitty keyboard protocol (and Windows) + // report Release events too; acting on them double-fires every + // key. Repeat is kept so held keys keep scrolling. + if key.kind == KeyEventKind::Release { + return RoutedAction::Ignore; + } + if route == renderer::ROUTE_PROMPT { + return RoutedAction::Prompt(InputEvent::Key(*key)); + } + if key.code == KeyCode::Esc && fullscreen { + return RoutedAction::Nav(NavCmd::ExitFullscreen); + } + let quit = matches!( + key.code, + KeyCode::Char('q') | KeyCode::Char('Q') | KeyCode::Esc + ) || matches!( + (key.code, key.modifiers), + (KeyCode::Char('c'), m) if m.contains(KeyModifiers::CONTROL) + ); + if quit { + RoutedAction::Cancel + } else if let Some(cmd) = match_nav_key(key.code) { + RoutedAction::Nav(cmd) + } else { + RoutedAction::Ignore + } + } + Event::Resize(_, _) => { + if route == renderer::ROUTE_PROMPT { + RoutedAction::Prompt(InputEvent::Resize) + } else { + RoutedAction::Nav(NavCmd::Redraw) + } + } + _ => RoutedAction::Ignore, + } } /// Map a key code to a navigation command, if applicable. @@ -112,102 +216,340 @@ fn match_nav_key(code: crossterm::event::KeyCode) -> Option { } } -/// Spawn a nav-only crossterm-poller thread (ignores quit keys). -fn spawn_nav_thread() -> (KbPollerInner, std::sync::mpsc::Receiver) { - let (nav_tx, nav_rx) = std::sync::mpsc::channel::(); - let stop = Arc::new(AtomicBool::new(false)); - let stop_flag = stop.clone(); - - let handle = std::thread::spawn(move || { - use crossterm::event::{poll, read, Event}; - - loop { - if stop_flag.load(Ordering::Relaxed) { - break; - } - if let Ok(true) = poll(std::time::Duration::from_millis(100)) { - if let Ok(Event::Key(key)) = read() { - if let Some(cmd) = match_nav_key(key.code) { - let _ = nav_tx.send(cmd); - } - } - } +/// Spawn the single crossterm-reader thread. +/// +/// The thread keeps running after a cancel (repeated quit presses are +/// harmless) and stops when the hub is dropped or the renderer sets the +/// stop flag on a plain-mode fallback. +/// +/// Fail-safe: if the thread exits for any reason other than a requested +/// stop (a panic, or a future bug that breaks the loop), it sets the cancel +/// signal before dying. Raw mode eats Ctrl+C's SIGINT while the TUI is +/// live, so a silently dead input thread would otherwise leave the user +/// with no cancel path at all — cancelling winds the run down instead. +fn spawn_input_thread( + shared: Arc, + nav_tx: std::sync::mpsc::Sender, + key_tx: std::sync::mpsc::Sender, + cancel_tx: tokio::sync::watch::Sender, +) -> std::thread::JoinHandle<()> { + std::thread::spawn(move || { + let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + input_loop(&shared, &nav_tx, &key_tx, &cancel_tx); + })); + if outcome.is_err() || !shared.stop.load(Ordering::Relaxed) { + tracing::error!("input hub thread exited unexpectedly; requesting cancellation"); + let _ = cancel_tx.send(true); } - }); - - (KbPollerInner { stop, handle }, nav_rx) + }) } -/// Spawn the background crossterm-poller thread (with quit/cancel support). -fn spawn_thread() -> ( - KbPollerInner, - oneshot::Receiver<()>, - std::sync::mpsc::Receiver, +/// The hub thread's event loop. Extracted so [`spawn_input_thread`] can wrap +/// it in the fail-safe above. +fn input_loop( + shared: &HubShared, + nav_tx: &std::sync::mpsc::Sender, + key_tx: &std::sync::mpsc::Sender, + cancel_tx: &tokio::sync::watch::Sender, ) { - let (tx, rx) = oneshot::channel::<()>(); - let (nav_tx, nav_rx) = std::sync::mpsc::channel::(); - let stop = Arc::new(AtomicBool::new(false)); - let stop_flag = stop.clone(); + use crossterm::event::{poll, read}; - let handle = std::thread::spawn(move || { - use crossterm::event::{poll, read, Event, KeyCode, KeyModifiers}; + let mut prev_route = renderer::ROUTE_EXEC; + loop { + if shared.stop.load(Ordering::Relaxed) { + break; + } + // A prompt opening supersedes any quit key pressed while it was + // still on its way to the screen: clear the cancel latch on the + // EXEC→PROMPT transition so a keystroke racing the prompt cannot + // poison the run after the user explicitly answers the prompt. + // (Quit keys pressed AT the prompt are routed to the prompt and + // handled there — they never touch this latch.) + let cur_route = shared.route.load(Ordering::Relaxed); + if cur_route == renderer::ROUTE_PROMPT + && prev_route == renderer::ROUTE_EXEC + && *cancel_tx.borrow() + { + tracing::debug!("prompt opened; clearing pending cancel latch"); + let _ = cancel_tx.send(false); + } + prev_route = cur_route; - loop { - if stop_flag.load(Ordering::Relaxed) { - break; - } - // Short timeout so the thread wakes frequently to check `stop`. - if let Ok(true) = poll(std::time::Duration::from_millis(100)) { - if let Ok(Event::Key(key)) = read() { - let quit = matches!( - key.code, - KeyCode::Char('q') | KeyCode::Char('Q') | KeyCode::Esc - ) || matches!( - (key.code, key.modifiers), - (KeyCode::Char('c'), m) if m.contains(KeyModifiers::CONTROL) - ); - if quit { - let _ = tx.send(()); - break; - } - // Navigation keys: send NavCmd events (non-blocking, ignore if receiver dropped) - if let Some(cmd) = match_nav_key(key.code) { + // Short timeout so the thread wakes frequently to check `stop` + // and the route transition above. + if let Ok(true) = poll(std::time::Duration::from_millis(100)) { + if let Ok(event) = read() { + match route_event( + shared.route.load(Ordering::Relaxed), + shared.fullscreen.load(Ordering::Relaxed), + &event, + ) { + // Send errors mean the receiver is gone; ignore. + RoutedAction::Nav(cmd) => { let _ = nav_tx.send(cmd); } + RoutedAction::Prompt(ev) => { + let _ = key_tx.send(ev); + } + RoutedAction::Cancel => { + tracing::debug!("quit key latched cancel (exec route)"); + let _ = cancel_tx.send(true); + } + RoutedAction::Ignore => {} } } } - }); - - (KbPollerInner { stop, handle }, rx, nav_rx) + } } #[cfg(test)] mod tests { use super::*; - use std::sync::atomic::Ordering; + use crossterm::event::{Event, KeyCode, KeyEvent, KeyModifiers}; + + fn key(code: KeyCode) -> Event { + Event::Key(KeyEvent::new(code, KeyModifiers::NONE)) + } + + fn ctrl(c: char) -> Event { + Event::Key(KeyEvent::new(KeyCode::Char(c), KeyModifiers::CONTROL)) + } + + // ── route_event: execution route ── + + #[test] + fn exec_route_quit_keys_cancel() { + for ev in [ + key(KeyCode::Char('q')), + key(KeyCode::Char('Q')), + key(KeyCode::Esc), + ctrl('c'), + ] { + assert!( + matches!( + route_event(renderer::ROUTE_EXEC, false, &ev), + RoutedAction::Cancel + ), + "expected Cancel for {ev:?}" + ); + } + } + + #[test] + fn exec_route_nav_keys_become_nav_cmds() { + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Up)), + RoutedAction::Nav(NavCmd::StepUp) + )); + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Char('u'))), + RoutedAction::Nav(NavCmd::ScrollUp) + )); + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Char('y'))), + RoutedAction::Nav(NavCmd::CopyOutput) + )); + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Char('f'))), + RoutedAction::Nav(NavCmd::ToggleFullscreen) + )); + } + + #[test] + fn exec_route_unbound_key_ignored() { + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Char('x'))), + RoutedAction::Ignore + )); + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Enter)), + RoutedAction::Ignore + )); + } + + #[test] + fn exec_route_resize_requests_redraw() { + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &Event::Resize(80, 24)), + RoutedAction::Nav(NavCmd::Redraw) + )); + } + + #[test] + fn exec_route_esc_exits_fullscreen_instead_of_cancelling() { + // The hint line advertises "Esc exit fullscreen" — while fullscreen + // is active Esc must NOT abort the run. + assert!(matches!( + route_event(renderer::ROUTE_EXEC, true, &key(KeyCode::Esc)), + RoutedAction::Nav(NavCmd::ExitFullscreen) + )); + // Without fullscreen, Esc is a quit key. + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &key(KeyCode::Esc)), + RoutedAction::Cancel + )); + // Fullscreen does not shield the other quit keys. + assert!(matches!( + route_event(renderer::ROUTE_EXEC, true, &key(KeyCode::Char('q'))), + RoutedAction::Cancel + )); + } + + #[test] + fn release_events_are_ignored_on_both_routes() { + use crossterm::event::KeyEventKind; + let mut release = KeyEvent::new(KeyCode::Char('q'), KeyModifiers::NONE); + release.kind = KeyEventKind::Release; + let ev = Event::Key(release); + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &ev), + RoutedAction::Ignore + )); + assert!(matches!( + route_event(renderer::ROUTE_PROMPT, false, &ev), + RoutedAction::Ignore + )); + } + + #[test] + fn repeat_events_still_act() { + use crossterm::event::KeyEventKind; + let mut repeat = KeyEvent::new(KeyCode::Char('u'), KeyModifiers::NONE); + repeat.kind = KeyEventKind::Repeat; + assert!( + matches!( + route_event(renderer::ROUTE_EXEC, false, &Event::Key(repeat)), + RoutedAction::Nav(NavCmd::ScrollUp) + ), + "held keys must keep scrolling" + ); + } + + // ── route_event: prompt route ── + + #[test] + fn prompt_route_forwards_all_keys_verbatim() { + // Keys that would be quit/nav/copy on the exec route must reach the + // prompt untouched — this is the race the hub exists to fix. + for code in [ + KeyCode::Enter, + KeyCode::Char('y'), + KeyCode::Char('q'), + KeyCode::Esc, + KeyCode::Char('d'), + KeyCode::Up, + ] { + match route_event(renderer::ROUTE_PROMPT, false, &key(code)) { + RoutedAction::Prompt(InputEvent::Key(k)) => assert_eq!(k.code, code), + other => panic!("expected Prompt(Key) for {code:?}, got {other:?}"), + } + } + } + + #[test] + fn prompt_route_ctrl_c_forwarded_to_prompt() { + // Prompts implement their own Ctrl+C handling (UserCancelled). + assert!(matches!( + route_event(renderer::ROUTE_PROMPT, false, &ctrl('c')), + RoutedAction::Prompt(InputEvent::Key(_)) + )); + } + + #[test] + fn prompt_route_resize_forwarded_as_resize() { + assert!(matches!( + route_event(renderer::ROUTE_PROMPT, false, &Event::Resize(80, 24)), + RoutedAction::Prompt(InputEvent::Resize) + )); + } + + #[test] + fn non_key_events_ignored_on_exec_route() { + assert!(matches!( + route_event(renderer::ROUTE_EXEC, false, &Event::FocusGained), + RoutedAction::Ignore + )); + } + + // ── hub in non-TUI mode ── + + #[test] + fn non_tui_hub_is_inert() { + let mut hub = setup_input_hub(false); + assert!(hub.connect().is_none()); + assert!(!hub.is_cancelled()); + drop(hub); + } + + // ── wait_cancelled ── + + #[tokio::test] + async fn wait_cancelled_resolves_when_watch_fires() { + let (tx, rx) = tokio::sync::watch::channel(false); + let handle = tokio::spawn(wait_cancelled(rx)); + tx.send(true).expect("receiver alive"); + let result = handle.await.expect("task completes"); + assert!(result.is_ok()); + } + + #[tokio::test] + async fn wait_cancelled_resolves_immediately_when_already_set() { + let (tx, rx) = tokio::sync::watch::channel(true); + let result = wait_cancelled(rx).await; + assert!(result.is_ok()); + drop(tx); + } + + #[tokio::test] + async fn wait_cancelled_pends_when_sender_dropped() { + // A dropped sender must NOT resolve the future (that would cancel a + // run spuriously); it should stay pending until the timeout. + let (tx, rx) = tokio::sync::watch::channel(false); + drop(tx); + let result = + tokio::time::timeout(std::time::Duration::from_millis(50), wait_cancelled(rx)).await; + assert!(result.is_err(), "expected timeout, got {result:?}"); + } + + #[tokio::test] + async fn cancel_token_clones_are_independent() { + let (tx, rx) = tokio::sync::watch::channel(false); + let hub = InputHub { + inner: None, + cancel_rx: rx, + _cancel_tx: None, + conn: None, + }; + let token_a = hub.cancel_token(); + let token_b = hub.cancel_token(); + tx.send(true).expect("receivers alive"); + assert!(wait_cancelled(token_a).await.is_ok()); + assert!(wait_cancelled(token_b).await.is_ok()); + assert!(hub.is_cancelled()); + } + + // ── drop semantics ── #[test] fn drop_sets_flag_and_joins_thread() { - let stop = Arc::new(AtomicBool::new(false)); - let stop_clone = stop.clone(); + let shared = Arc::new(HubShared::new()); + let shared_clone = shared.clone(); let handle = std::thread::spawn(move || { - while !stop_clone.load(Ordering::Relaxed) { + while !shared_clone.stop.load(Ordering::Relaxed) { std::thread::yield_now(); } }); - let poller = KbPoller { - inner: Some(KbPollerInner { stop, handle }), + let (_tx, cancel_rx) = tokio::sync::watch::channel(false); + let hub = InputHub { + inner: Some(HubInner { shared, handle }), + cancel_rx, + _cancel_tx: None, + conn: None, }; - // Drop must set the flag and join the thread; reaching this line confirms it. - drop(poller); - } - - #[test] - fn drop_is_noop_when_inner_is_none() { - let poller = KbPoller { inner: None }; - drop(poller); + // Drop must set the flag and join the thread; reaching the next line + // confirms it. + drop(hub); } } diff --git a/src/cli/ui/diff.rs b/src/cli/ui/diff.rs index 5d29fd15..cfd56a25 100644 --- a/src/cli/ui/diff.rs +++ b/src/cli/ui/diff.rs @@ -258,11 +258,9 @@ pub fn format_adr_diffs(adr_diffs: &[AdrDiff]) -> Vec { let mut lines = Vec::new(); for adr_diff in adr_diffs { - let short_id = if adr_diff.adr_id.len() > 8 { - &adr_diff.adr_id[..8] - } else { - &adr_diff.adr_id - }; + // chars().take() rather than a byte slice: a byte index can split a + // multi-byte character in a non-ASCII id and panic. + let short_id: String = adr_diff.adr_id.chars().take(8).collect(); match &adr_diff.change { AdrChange::Added => { @@ -1089,6 +1087,40 @@ mod tests { ); } + #[test] + fn test_format_adr_diffs_non_ascii_id_truncates_on_char_boundary() { + // A byte slice at index 8 would split the multi-byte 'β' here and + // panic; truncation must count characters, not bytes. + let diffs = vec![AdrDiff { + adr_id: "αβγδεζηθικλμ".to_string(), + change: AdrChange::Removed, + old_content: None, + new_content: None, + }]; + let lines = format_adr_diffs(&diffs); + let plain = console::strip_ansi_codes(&lines.join("\n")).into_owned(); + assert!( + plain.contains("- [αβγδεζηθ] removed"), + "expected 8-character truncated id in: {plain}" + ); + } + + #[test] + fn test_format_adr_diffs_short_id_kept_whole() { + let diffs = vec![AdrDiff { + adr_id: "abc".to_string(), + change: AdrChange::Removed, + old_content: None, + new_content: None, + }]; + let lines = format_adr_diffs(&diffs); + let plain = console::strip_ansi_codes(&lines.join("\n")).into_owned(); + assert!( + plain.contains("- [abc] removed"), + "expected whole short id in: {plain}" + ); + } + #[test] fn test_format_adr_diffs_updated() { let diffs = vec![AdrDiff { diff --git a/src/cli/ui/tui/renderer.rs b/src/cli/ui/tui/renderer.rs index 53a541f2..78a0bfc8 100644 --- a/src/cli/ui/tui/renderer.rs +++ b/src/cli/ui/tui/renderer.rs @@ -49,6 +49,102 @@ pub enum NavCmd { CopyOutput, /// Toggle full-screen output mode (hides the left sidebar). ToggleFullscreen, + /// Leave full-screen output mode (idempotent — safe under the routing + /// race where fullscreen was already exited). + ExitFullscreen, + /// Redraw the frame without changing state (e.g. after a terminal resize). + Redraw, +} + +/// A terminal input event relevant to the TUI: a key press or a resize. +/// +/// Resize is surfaced (rather than silently discarded) so blocking prompt +/// loops can redraw the frame immediately instead of leaving a stale layout +/// until the next keypress. +#[derive(Debug, Clone, Copy)] +pub(crate) enum InputEvent { + Key(crossterm::event::KeyEvent), + Resize, +} + +/// Route flag values shared with the background input thread: input is +/// dispatched as navigation/cancel commands during execution, or forwarded +/// verbatim to the active prompt. +pub(crate) const ROUTE_EXEC: u8 = 0; +pub(crate) const ROUTE_PROMPT: u8 = 1; + +/// Whether the real terminal is currently in TUI state (raw mode + alternate +/// screen). Read by the panic hook so a crash restores the terminal *before* +/// the panic message prints — otherwise the message lands inside the +/// alternate screen and is erased when [`TuiRenderer`] drops on unwind, +/// leaving the user with a blank terminal and no diagnostic. +static TUI_ACTIVE: std::sync::atomic::AtomicBool = std::sync::atomic::AtomicBool::new(false); + +/// One-time installation guard for the panic hook. +static PANIC_HOOK: std::sync::Once = std::sync::Once::new(); + +/// Install (once) a panic hook that restores the terminal and then delegates +/// to the previously installed hook, so panic output reaches the normal +/// screen buffer. +/// +/// Known trade-off: the hook cannot distinguish fatal panics from panics +/// that are caught (e.g. a worker panic surfaced as a `JoinError`), so a +/// recovered panic also restores the terminal while the TUI is live. Every +/// such path in the sync pipeline aborts the run immediately afterwards, so +/// the brief cooked-mode window is accepted. +fn install_panic_hook() { + PANIC_HOOK.call_once(|| { + let prev = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + restore_terminal_if_active(); + prev(info); + })); + }); +} + +/// Leave TUI state if it is active: disable raw mode, leave the alternate +/// screen, and show the cursor. The flag is cleared first (atomic swap), so +/// duplicate or concurrent calls restore at most once; restoring an +/// already-restored terminal is harmless anyway. +fn restore_terminal_if_active() { + if TUI_ACTIVE.swap(false, std::sync::atomic::Ordering::SeqCst) { + let _ = disable_raw_mode(); + let _ = execute!(io::stderr(), LeaveAlternateScreen, Show); + } +} + +/// State shared between the renderer and the input-hub thread. +/// +/// Grouped in one struct (rather than separately plumbed atomics) so the +/// renderer↔hub contract is handed over as a unit and cannot be half-wired. +pub(crate) struct HubShared { + /// Input routing: [`ROUTE_EXEC`] or [`ROUTE_PROMPT`]. While a prompt is + /// active the hub forwards events verbatim over the prompt channel + /// instead of interpreting them as navigation/cancel. + pub(crate) route: std::sync::atomic::AtomicU8, + /// Mirror of the renderer's fullscreen state; drives Esc routing (exit + /// fullscreen vs cancel the run). + pub(crate) fullscreen: std::sync::atomic::AtomicBool, + /// Stops the hub thread (set by the hub's Drop, or by the renderer's + /// plain-mode fallback so dialoguer prompts never race the hub). + pub(crate) stop: std::sync::atomic::AtomicBool, +} + +impl HubShared { + pub(crate) fn new() -> Self { + Self { + route: std::sync::atomic::AtomicU8::new(ROUTE_EXEC), + fullscreen: std::sync::atomic::AtomicBool::new(false), + stop: std::sync::atomic::AtomicBool::new(false), + } + } +} + +/// Everything the renderer needs from the input hub, connected in one step. +pub(crate) struct HubConnection { + pub(crate) nav_rx: std::sync::mpsc::Receiver, + pub(crate) prompt_rx: std::sync::mpsc::Receiver, + pub(crate) shared: std::sync::Arc, } /// Abstraction over crossterm event reading so the event loop can be tested @@ -56,23 +152,63 @@ pub enum NavCmd { pub(crate) trait EventSource { /// Block until the next key event and return it. fn next_key(&mut self) -> io::Result; + + /// Block until the next input event (key or resize). Key-only sources + /// need not override this. + fn next_event(&mut self) -> io::Result { + self.next_key().map(InputEvent::Key) + } } -/// Production event source that reads from crossterm. +/// Production event source that reads from crossterm directly. +/// +/// Only used when no input hub is active (defensive fallback); with a hub +/// running, prompts read from [`ChannelEventSource`] instead so the hub +/// thread remains the sole crossterm reader. pub(crate) struct CrosstermEventSource; impl EventSource for CrosstermEventSource { fn next_key(&mut self) -> io::Result { + loop { + if let InputEvent::Key(key) = self.next_event()? { + return Ok(key); + } + } + } + + fn next_event(&mut self) -> io::Result { use crossterm::event::{read, Event}; loop { match read()? { - Event::Key(key) => return Ok(key), + Event::Key(key) => return Ok(InputEvent::Key(key)), + Event::Resize(_, _) => return Ok(InputEvent::Resize), _ => continue, } } } } +/// Event source that reads prompt input forwarded by the input hub thread. +struct ChannelEventSource { + rx: std::sync::mpsc::Receiver, +} + +impl EventSource for ChannelEventSource { + fn next_key(&mut self) -> io::Result { + loop { + if let InputEvent::Key(key) = self.next_event()? { + return Ok(key); + } + } + } + + fn next_event(&mut self) -> io::Result { + self.rx + .recv() + .map_err(|_| io::Error::new(io::ErrorKind::UnexpectedEof, "input hub stopped")) + } +} + /// Which button has focus in the inline confirmation UI. #[derive(Debug, Clone, Copy, PartialEq)] pub enum ConfirmFocus { @@ -665,6 +801,13 @@ pub struct TuiRenderer { version: String, /// Receives navigation commands from the background keyboard poller during execution. nav_rx: Option>, + /// Prompt-input channel from the input hub. When present, interactive + /// prompts read from it (flipping the shared route flag) instead of + /// reading crossterm directly. + prompt_rx: Option>, + /// Shared renderer↔hub state: routing flag, fullscreen mirror (kept in + /// sync by [`Self::set_fullscreen`]), and the hub thread's stop flag. + hub: Option>, } impl TuiRenderer { @@ -725,6 +868,8 @@ impl TuiRenderer { fullscreen: false, version: version.to_string(), nav_rx: None, + prompt_rx: None, + hub: None, } } @@ -735,12 +880,29 @@ impl TuiRenderer { /// visible tearing on slow terminals. fn try_setup_tui() -> Option { enable_raw_mode().ok()?; - execute!(io::stderr(), EnterAlternateScreen, Hide).ok()?; - Terminal::new(ratatui::backend::CrosstermBackend::new(BufWriter::new( + if execute!(io::stderr(), EnterAlternateScreen, Hide).is_err() { + // Unwind the partial setup: raw mode is already on but the + // alternate screen was never entered. Without this, falling back + // to Plain mode leaves the terminal raw for the whole process. + let _ = disable_raw_mode(); + return None; + } + match Terminal::new(ratatui::backend::CrosstermBackend::new(BufWriter::new( io::stderr(), - ))) - .ok() - .map(|t| Mode::Tui(Box::new(TuiTerminalImpl(t)) as Box)) + ))) { + Ok(t) => { + install_panic_hook(); + TUI_ACTIVE.store(true, std::sync::atomic::Ordering::SeqCst); + Some(Mode::Tui( + Box::new(TuiTerminalImpl(t)) as Box + )) + } + Err(_) => { + let _ = execute!(io::stderr(), LeaveAlternateScreen, Show); + let _ = disable_raw_mode(); + None + } + } } /// Construct a renderer in Tui mode using the given backend (for tests). @@ -781,6 +943,8 @@ impl TuiRenderer { fullscreen: false, version: "0.0.0-test".to_string(), nav_rx: None, + prompt_rx: None, + hub: None, } } @@ -791,6 +955,84 @@ impl TuiRenderer { self.nav_rx = rx; } + /// Connect the input hub in one step: nav channel, prompt channel, and + /// the shared state. A single handover means the wiring cannot be + /// half-done (e.g. prompt channel set but the fullscreen mirror not, + /// which would silently revert Esc to "cancel the run" in fullscreen). + /// `None` (non-TUI mode) clears everything. + pub(crate) fn connect_input_hub(&mut self, conn: Option) { + match conn { + Some(conn) => { + self.nav_rx = Some(conn.nav_rx); + self.prompt_rx = Some(conn.prompt_rx); + self.hub = Some(conn.shared); + } + None => { + self.nav_rx = None; + self.prompt_rx = None; + self.hub = None; + } + } + } + + /// Single mutation point for fullscreen state: updates the field and the + /// hub-shared mirror together so Esc routing never observes a stale value. + fn set_fullscreen(&mut self, on: bool) { + self.fullscreen = on; + if let Some(hub) = &self.hub { + hub.fullscreen + .store(on, std::sync::atomic::Ordering::Relaxed); + } + } + + /// Begin routing input to a prompt: flip the shared route flag to + /// [`ROUTE_PROMPT`] and drain any stale events left over from a + /// previously aborted prompt. Returns `None` when no hub is connected. + fn take_prompt_source(&mut self) -> Option<(ChannelEventSource, std::sync::Arc)> { + let rx = self.prompt_rx.take()?; + let shared = match &self.hub { + Some(hub) => hub.clone(), + None => { + // Half-connected state (should be unreachable): put the + // channel back and fall back to direct reads. + self.prompt_rx = Some(rx); + return None; + } + }; + // Drain BEFORE flipping the route: everything in the prompt channel + // right now is provably stale (the hub only feeds it while the route + // is PROMPT, which it hasn't been since the last prompt closed). + // Draining after the flip could discard a fresh type-ahead key the + // hub forwards the instant the route changes. + while rx.try_recv().is_ok() {} + shared + .route + .store(ROUTE_PROMPT, std::sync::atomic::Ordering::Relaxed); + // Discard navigation commands queued before the flip: draw() applies + // drained NavCmds, so a stale scroll/copy/fullscreen command would + // otherwise fire mid-prompt (or reset review-mode scroll state that + // was just initialized). One in-flight event may still slip through + // after this drain — that is cosmetic only (scroll/flash), never a + // prompt-consuming or panicking action. + if let Some(nav) = &self.nav_rx { + while nav.try_recv().is_ok() {} + } + Some((ChannelEventSource { rx }, shared)) + } + + /// End prompt routing: flip the route flag back to [`ROUTE_EXEC`] and + /// keep the channel for the next prompt. + fn restore_prompt_source( + &mut self, + source: ChannelEventSource, + shared: std::sync::Arc, + ) { + shared + .route + .store(ROUTE_EXEC, std::sync::atomic::Ordering::Relaxed); + self.prompt_rx = Some(source.rx); + } + /// Drain all pending [`NavCmd`]s from the channel and apply each one. fn apply_nav_cmds(&mut self) { // We can't hold a borrow on self.nav_rx while also calling handle_nav_cmd(self, …), @@ -857,9 +1099,17 @@ impl TuiRenderer { self.copy_and_flash(log_idx); } NavCmd::ToggleFullscreen => { - self.fullscreen = !self.fullscreen; + self.set_fullscreen(!self.fullscreen); + self.scroll_offset = 0; + } + NavCmd::ExitFullscreen => { + self.set_fullscreen(false); self.scroll_offset = 0; } + NavCmd::Redraw => { + // No state change: nav commands are drained inside draw(), so + // the redraw this requested is already in progress. + } } } @@ -1017,6 +1267,35 @@ impl TuiRenderer { } } + /// Push a diagnostic line: log pane in TUI mode, **stderr** in Plain mode. + /// + /// Use for --verbose/diagnostic output. Plain mode reserves stdout for + /// primary output (the documented stream contract, pinned by + /// integration tests), so diagnostics must go to stderr there — while + /// in TUI mode the log pane is the only surface the user can read. + pub fn eprintln(&mut self, line: &str) { + if let Mode::Quiet = &self.mode { + return; + } + self.push_log(self.active_step, line); + self.draw(); + if let Mode::Plain = &self.mode { + eprintln!("{line}"); + } + } + + /// Hold the TUI open after a failed run so the transcript (subprocess + /// stderr, API errors) stays readable, then wait for a key. + /// No-op outside TUI mode — non-interactive runs must never block. + pub fn pause_for_failure_review(&mut self) { + if !self.is_tui() { + return; + } + self.println(""); + self.println(" Run failed \u{2014} press any key to close this view"); + self.wait_for_keypress(); + } + /// Push a line to a specific step's log pane, stripping ANSI escape codes. /// /// All internal log pushes should go through this method (or through @@ -1047,7 +1326,13 @@ impl TuiRenderer { /// Show "Press any key to close" hint and block until a keypress in TUI mode. /// In Plain/Quiet mode: no-op (non-interactive sessions should not block). pub fn wait_for_keypress(&mut self) { - self.wait_for_keypress_impl(&mut CrosstermEventSource); + match self.take_prompt_source() { + Some((mut source, route)) => { + self.wait_for_keypress_impl(&mut source); + self.restore_prompt_source(source, route); + } + None => self.wait_for_keypress_impl(&mut CrosstermEventSource), + } } fn wait_for_keypress_impl(&mut self, source: &mut dyn EventSource) { @@ -1061,7 +1346,16 @@ impl TuiRenderer { self.hint = true; self.scroll_offset = 0; self.draw(); - while let Ok(key) = source.next_key() { + while let Ok(event) = source.next_event() { + let key = match event { + InputEvent::Key(key) => key, + InputEvent::Resize => { + // Reflow the layout immediately instead of waiting + // for the next keypress. + self.draw(); + continue; + } + }; // Clear any flash message from the previous key. self.flash_message = None; // Compute log_height matching render_to's formula exactly. @@ -1100,7 +1394,7 @@ impl TuiRenderer { } (KeyCode::Esc, _) => { if self.fullscreen { - self.fullscreen = false; + self.set_fullscreen(false); self.scroll_offset = 0; self.draw(); } else { @@ -1110,7 +1404,7 @@ impl TuiRenderer { (KeyCode::Char('c'), m) if m.contains(KeyModifiers::CONTROL) => break, // Toggle full-screen output mode (KeyCode::Char('f'), _) => { - self.fullscreen = !self.fullscreen; + self.set_fullscreen(!self.fullscreen); self.scroll_offset = 0; self.draw(); } @@ -1128,7 +1422,10 @@ impl TuiRenderer { } // Scroll output up within selected step (PgUp) (KeyCode::PageUp, _) | (KeyCode::Char('u'), _) => { - self.scroll_offset = (self.scroll_offset + log_height / 2).min(max); + // saturating_add: a stale exec-phase ScrollTop can leave + // the offset at usize::MAX, which a plain add overflows. + self.scroll_offset = + self.scroll_offset.saturating_add(log_height / 2).min(max); self.draw(); } // Scroll output down within selected step (PgDn) @@ -1169,7 +1466,14 @@ impl TuiRenderer { analysis: &RepoAnalysis, term: &dyn TerminalIO, ) -> Result { - self.confirm_project_impl(analysis, term, &mut CrosstermEventSource) + match self.take_prompt_source() { + Some((mut source, route)) => { + let result = self.confirm_project_impl(analysis, term, &mut source); + self.restore_prompt_source(source, route); + result + } + None => self.confirm_project_impl(analysis, term, &mut CrosstermEventSource), + } } fn confirm_project_impl( @@ -1207,9 +1511,15 @@ impl TuiRenderer { ) -> Result { use crossterm::event::{KeyCode, KeyModifiers}; loop { - let key = source.next_key().map_err(|e| { + let key = match source.next_event().map_err(|e| { crate::error::ActualError::InternalError(format!("event read failed: {e}")) - })?; + })? { + InputEvent::Key(key) => key, + InputEvent::Resize => { + self.draw(); + continue; + } + }; match (key.code, key.modifiers) { (KeyCode::Enter, _) => { let focus = self @@ -1278,7 +1588,14 @@ impl TuiRenderer { force: bool, term: &dyn TerminalIO, ) -> Result, crate::error::ActualError> { - self.select_files_in_tui_impl(output, force, term, &mut CrosstermEventSource) + match self.take_prompt_source() { + Some((mut source, route)) => { + let result = self.select_files_in_tui_impl(output, force, term, &mut source); + self.restore_prompt_source(source, route); + result + } + None => self.select_files_in_tui_impl(output, force, term, &mut CrosstermEventSource), + } } fn select_files_in_tui_impl( @@ -1368,9 +1685,15 @@ impl TuiRenderer { ) -> Result>, crate::error::ActualError> { use crossterm::event::{KeyCode, KeyModifiers}; loop { - let key = source.next_key().map_err(|e| { + let key = match source.next_event().map_err(|e| { crate::error::ActualError::InternalError(format!("event read failed: {e}")) - })?; + })? { + InputEvent::Key(key) => key, + InputEvent::Resize => { + self.draw(); + continue; + } + }; let n = self .file_select_state .as_ref() @@ -1441,7 +1764,16 @@ impl TuiRenderer { default: Option, term: &dyn TerminalIO, ) -> Result { - self.select_one_in_tui_impl(prompt, items, default, term, &mut CrosstermEventSource) + match self.take_prompt_source() { + Some((mut source, route)) => { + let result = self.select_one_in_tui_impl(prompt, items, default, term, &mut source); + self.restore_prompt_source(source, route); + result + } + None => { + self.select_one_in_tui_impl(prompt, items, default, term, &mut CrosstermEventSource) + } + } } fn select_one_in_tui_impl( @@ -1482,9 +1814,15 @@ impl TuiRenderer { ) -> Result, crate::error::ActualError> { use crossterm::event::{KeyCode, KeyModifiers}; loop { - let key = source.next_key().map_err(|e| { + let key = match source.next_event().map_err(|e| { crate::error::ActualError::InternalError(format!("event read failed: {e}")) - })?; + })? { + InputEvent::Key(key) => key, + InputEvent::Resize => { + self.draw(); + continue; + } + }; match (key.code, key.modifiers) { (KeyCode::Up, _) => { if let Some(ref mut ss) = self.single_select_state { @@ -1605,9 +1943,20 @@ impl TuiRenderer { tracing::error!("TUI draw failed: {err_msg}; falling back to plain output"); // Clean up terminal state before switching to plain mode so the // Drop impl does not attempt a double-cleanup. + TUI_ACTIVE.store(false, std::sync::atomic::Ordering::SeqCst); let _ = disable_raw_mode(); let _ = execute!(io::stderr(), LeaveAlternateScreen, Show); self.mode = Mode::Plain; + // Shut the input hub down and drop its channels: plain-mode + // prompts read the terminal directly (dialoguer), and a live hub + // thread would compete with them for keystrokes — the exact + // two-reader race the hub exists to prevent. + if let Some(hub) = &self.hub { + hub.stop.store(true, std::sync::atomic::Ordering::Relaxed); + } + self.prompt_rx = None; + self.nav_rx = None; + self.hub = None; } } } @@ -1615,6 +1964,10 @@ impl TuiRenderer { impl Drop for TuiRenderer { fn drop(&mut self) { if let Mode::Tui(_) = &self.mode { + // Restore unconditionally rather than via the flag: test + // renderers (new_with_tui) never set TUI_ACTIVE but still hold + // Mode::Tui, and the escapes are harmless on a non-TTY stderr. + TUI_ACTIVE.store(false, std::sync::atomic::Ordering::SeqCst); // Silently ignore errors — in tests there is no raw-mode TTY. let _ = disable_raw_mode(); let _ = execute!(io::stderr(), LeaveAlternateScreen, Show); @@ -1645,6 +1998,39 @@ impl EventSource for MockEventSource { } } +/// Like [`MockEventSource`] but yields full [`InputEvent`]s, so tests can +/// inject resize events into the prompt loops. +#[cfg(test)] +pub(crate) struct MockInputSource { + pub(crate) events: std::collections::VecDeque, +} + +#[cfg(test)] +impl MockInputSource { + pub(crate) fn new(events: Vec) -> Self { + Self { + events: events.into(), + } + } +} + +#[cfg(test)] +impl EventSource for MockInputSource { + fn next_key(&mut self) -> io::Result { + loop { + if let InputEvent::Key(key) = self.next_event()? { + return Ok(key); + } + } + } + + fn next_event(&mut self) -> io::Result { + self.events + .pop_front() + .ok_or_else(|| io::Error::new(io::ErrorKind::UnexpectedEof, "no more events")) + } +} + #[cfg(test)] mod tests { use super::*; @@ -4163,4 +4549,404 @@ mod tests { ) .unwrap(); } + + // ── input hub integration: resize events in prompt loops ── + + fn tui_renderer_100x30() -> TuiRenderer { + let backend = TestBackend::new(100, 30); + let terminal = Terminal::new(backend).unwrap(); + TuiRenderer::new_with_tui(terminal) + } + + fn enter_key() -> crossterm::event::KeyEvent { + use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE) + } + + #[test] + fn test_confirm_loop_resize_redraws_and_continues() { + let mut r = tui_renderer_100x30(); + r.confirm_state = Some(ConfirmState { + question: "Proceed?".to_string(), + focus: ConfirmFocus::Accept, + }); + let mut source = + MockInputSource::new(vec![InputEvent::Resize, InputEvent::Key(enter_key())]); + let result = r.run_confirm_loop(&mut source); + assert!( + matches!(result, Ok(ConfirmAction::Accept)), + "resize must not consume the prompt; Enter after it accepts" + ); + assert!(source.events.is_empty(), "both events consumed"); + } + + #[test] + fn test_file_select_loop_resize_redraws_and_continues() { + let mut r = tui_renderer_100x30(); + r.file_select_state = Some(FileSelectState { + items: vec!["a.md".to_string(), "b.md".to_string()], + checked: vec![true, true], + cursor: 0, + }); + let mut source = + MockInputSource::new(vec![InputEvent::Resize, InputEvent::Key(enter_key())]); + let result = r.run_file_select_loop(&mut source); + assert!( + matches!(result, Ok(Some(ref v)) if v == &vec![0, 1]), + "expected all items selected after resize + Enter, got {result:?}" + ); + } + + #[test] + fn test_single_select_loop_resize_redraws_and_continues() { + let mut r = tui_renderer_100x30(); + r.single_select_state = Some(SingleSelectState { + prompt: "Pick one".to_string(), + items: vec!["x".to_string(), "y".to_string()], + cursor: 1, + }); + let mut source = + MockInputSource::new(vec![InputEvent::Resize, InputEvent::Key(enter_key())]); + let result = r.run_single_select_loop(&mut source); + assert!( + matches!(result, Ok(Some(1))), + "expected cursor position confirmed after resize + Enter, got {result:?}" + ); + } + + #[test] + fn test_wait_for_keypress_resize_redraws_and_continues() { + use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + let mut r = tui_renderer_100x30(); + let q = KeyEvent::new(KeyCode::Char('q'), KeyModifiers::NONE); + let mut source = MockInputSource::new(vec![InputEvent::Resize, InputEvent::Key(q)]); + r.wait_for_keypress_impl(&mut source); + assert!(source.events.is_empty(), "resize then quit both consumed"); + assert!(!r.hint, "review mode exited"); + } + + // ── input hub integration: prompt channel routing ── + + /// Build a hub connection for tests, returning the senders and shared + /// state alongside it. + fn test_hub_connection() -> ( + std::sync::mpsc::Sender, + std::sync::mpsc::Sender, + std::sync::Arc, + HubConnection, + ) { + let (nav_tx, nav_rx) = std::sync::mpsc::channel(); + let (key_tx, prompt_rx) = std::sync::mpsc::channel(); + let shared = std::sync::Arc::new(HubShared::new()); + let conn = HubConnection { + nav_rx, + prompt_rx, + shared: shared.clone(), + }; + (nav_tx, key_tx, shared, conn) + } + + #[test] + fn test_take_prompt_source_flips_route_and_drains_stale_events() { + use std::sync::atomic::Ordering; + + let mut r = tui_renderer_100x30(); + let (_nav_tx, tx, shared, conn) = test_hub_connection(); + r.connect_input_hub(Some(conn)); + + // A stale event left over from an aborted prompt must be discarded. + tx.send(InputEvent::Resize).unwrap(); + + let (mut source, taken_shared) = r.take_prompt_source().expect("channel connected"); + assert_eq!(shared.route.load(Ordering::Relaxed), ROUTE_PROMPT); + + // Only events sent after the take are visible. + tx.send(InputEvent::Key(enter_key())).unwrap(); + match source.next_event().expect("event available") { + InputEvent::Key(k) => assert_eq!(k.code, crossterm::event::KeyCode::Enter), + other => panic!("stale event leaked into prompt: {other:?}"), + } + + r.restore_prompt_source(source, taken_shared); + assert_eq!(shared.route.load(Ordering::Relaxed), ROUTE_EXEC); + assert!( + r.take_prompt_source().is_some(), + "channel is reusable for the next prompt" + ); + } + + #[test] + fn test_confirm_project_reads_from_prompt_channel() { + use crate::cli::ui::test_utils::MockTerminal; + use std::sync::atomic::Ordering; + + let mut r = tui_renderer_100x30(); + let term = MockTerminal::new(vec![]); + let analysis = RepoAnalysis { + is_monorepo: false, + workspace_type: None, + projects: vec![], + }; + + let (_nav_tx, tx, shared, conn) = test_hub_connection(); + r.connect_input_hub(Some(conn)); + + // Simulate the hub thread: forward Enter once the route flips to + // prompt (sending earlier would be drained as stale). + let shared_probe = shared.clone(); + let sender = std::thread::spawn(move || { + while shared_probe.route.load(Ordering::Relaxed) != ROUTE_PROMPT { + std::thread::yield_now(); + } + tx.send(InputEvent::Key(enter_key())).unwrap(); + }); + + let result = r.confirm_project(&analysis, &term); + sender.join().unwrap(); + + assert!(matches!(result, Ok(ConfirmAction::Accept))); + assert_eq!( + shared.route.load(Ordering::Relaxed), + ROUTE_EXEC, + "route restored after the prompt" + ); + assert!( + r.take_prompt_source().is_some(), + "channel returned to the renderer for the next prompt" + ); + } + + #[test] + fn test_pause_for_failure_review_plain_mode_is_noop() { + let mut r = TuiRenderer::new(false, true); // Plain mode + r.pause_for_failure_review(); // must return immediately, no block + } + + #[test] + fn test_pause_for_failure_review_tui_shows_message_and_waits() { + use std::sync::atomic::Ordering; + let mut r = tui_renderer_100x30(); + let (_nav_tx, tx, shared, conn) = test_hub_connection(); + r.connect_input_hub(Some(conn)); + + let shared_probe = shared.clone(); + let sender = std::thread::spawn(move || { + while shared_probe.route.load(Ordering::Relaxed) != ROUTE_PROMPT { + std::thread::yield_now(); + } + tx.send(InputEvent::Key(enter_key())).unwrap(); + }); + + r.pause_for_failure_review(); + sender.join().unwrap(); + + assert!( + r.all_log_text().contains("Run failed"), + "failure notice must be in the pane" + ); + } + + #[test] + fn test_eprintln_writes_to_log_pane_in_tui_mode() { + let mut r = tui_renderer_100x30(); + r.eprintln("diagnostic line"); + assert!(r.all_log_text().contains("diagnostic line")); + } + + #[test] + fn test_eprintln_quiet_mode_is_noop() { + let mut r = TuiRenderer::new(true, false); // Quiet mode + r.eprintln("should vanish"); + assert!(!r.all_log_text().contains("should vanish")); + } + + #[test] + fn test_prompt_channel_disconnect_surfaces_as_error() { + let mut r = tui_renderer_100x30(); + r.confirm_state = Some(ConfirmState { + question: "Proceed?".to_string(), + focus: ConfirmFocus::Accept, + }); + let (tx, rx) = std::sync::mpsc::channel::(); + drop(tx); + let mut source = ChannelEventSource { rx }; + let result = r.run_confirm_loop(&mut source); + assert!( + matches!(result, Err(crate::error::ActualError::InternalError(_))), + "hub gone must surface as an error, got {result:?}" + ); + } + + #[test] + fn test_confirm_project_without_hub_falls_back() { + // No prompt channel set: Plain-mode path must still work unchanged. + use crate::cli::ui::test_utils::MockTerminal; + let term = MockTerminal::new(vec![]); + let mut r = TuiRenderer::new(false, true); // Plain mode + let analysis = RepoAnalysis { + is_monorepo: false, + workspace_type: None, + projects: vec![], + }; + let _ = r.confirm_project(&analysis, &term); + } + + #[test] + fn test_handle_nav_cmd_redraw_is_stateless() { + let mut r = tui_renderer_100x30(); + let before_scroll = r.scroll_offset; + let before_fullscreen = r.fullscreen; + r.handle_nav_cmd(NavCmd::Redraw); + assert_eq!(r.scroll_offset, before_scroll); + assert_eq!(r.fullscreen, before_fullscreen); + assert!(r.viewing_step.is_none()); + } + + #[test] + fn test_set_fullscreen_syncs_shared_mirror() { + use std::sync::atomic::Ordering; + let mut r = tui_renderer_100x30(); + let (_nav_tx, _key_tx, shared, conn) = test_hub_connection(); + r.connect_input_hub(Some(conn)); + + r.handle_nav_cmd(NavCmd::ToggleFullscreen); + assert!(r.fullscreen); + assert!( + shared.fullscreen.load(Ordering::Relaxed), + "hub mirror must track fullscreen ON" + ); + + r.handle_nav_cmd(NavCmd::ExitFullscreen); + assert!(!r.fullscreen); + assert!( + !shared.fullscreen.load(Ordering::Relaxed), + "hub mirror must track fullscreen OFF" + ); + } + + #[test] + fn test_connect_input_hub_none_clears_everything() { + let mut r = tui_renderer_100x30(); + let (_nav_tx, _key_tx, _shared, conn) = test_hub_connection(); + r.connect_input_hub(Some(conn)); + assert!(r.hub.is_some()); + r.connect_input_hub(None); + assert!(r.hub.is_none()); + assert!(r.prompt_rx.is_none()); + assert!(r.nav_rx.is_none()); + assert!( + r.take_prompt_source().is_none(), + "disconnected renderer falls back to direct reads" + ); + } + + #[test] + fn test_exit_fullscreen_is_idempotent() { + let mut r = tui_renderer_100x30(); + assert!(!r.fullscreen); + // Exiting while already windowed must not toggle fullscreen ON — + // that is the race ExitFullscreen exists to avoid. + r.handle_nav_cmd(NavCmd::ExitFullscreen); + assert!(!r.fullscreen); + r.handle_nav_cmd(NavCmd::ExitFullscreen); + assert!(!r.fullscreen); + } + + // ── crash-safe terminal lifecycle ── + + /// Serializes the tests that mutate the process-global `TUI_ACTIVE`. + /// Poison-tolerant: a failing serialized test must not cascade. + static TUI_ACTIVE_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + fn tui_active_test_guard() -> std::sync::MutexGuard<'static, ()> { + TUI_ACTIVE_TEST_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } + + #[test] + fn test_restore_terminal_if_active_clears_flag_and_is_idempotent() { + use std::sync::atomic::Ordering; + let _guard = tui_active_test_guard(); + TUI_ACTIVE.store(true, Ordering::SeqCst); + restore_terminal_if_active(); + assert!( + !TUI_ACTIVE.load(Ordering::SeqCst), + "restore must clear the active flag" + ); + // Second call must be a no-op (flag already cleared). + restore_terminal_if_active(); + assert!(!TUI_ACTIVE.load(Ordering::SeqCst)); + } + + #[test] + fn test_install_panic_hook_is_idempotent() { + // Once-guarded: repeated installs must not stack hooks or panic. + install_panic_hook(); + install_panic_hook(); + } + + #[test] + fn test_panic_hook_restores_terminal_state_before_reporting() { + use std::sync::atomic::Ordering; + let _guard = tui_active_test_guard(); + install_panic_hook(); + TUI_ACTIVE.store(true, Ordering::SeqCst); + let result = std::panic::catch_unwind(|| panic!("deliberate panic: hook test")); + assert!(result.is_err(), "closure must have panicked"); + assert!( + !TUI_ACTIVE.load(Ordering::SeqCst), + "panic hook must leave TUI state before the message prints" + ); + } + + #[test] + fn test_review_scroll_up_saturates_after_stale_scroll_top() { + use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + let mut r = tui_renderer_100x30(); + // A ScrollTop queued during execution can still be applied by the + // entry draw() after review mode resets the offset — the 'u' branch + // must not overflow on the resulting usize::MAX. + let (tx, rx) = std::sync::mpsc::channel(); + r.set_nav_rx_opt(Some(rx)); + tx.send(NavCmd::ScrollTop).unwrap(); + let u = KeyEvent::new(KeyCode::Char('u'), KeyModifiers::NONE); + let q = KeyEvent::new(KeyCode::Char('q'), KeyModifiers::NONE); + let mut source = MockEventSource::new(vec![u, q]); + // Pre-fix this panicked in debug builds (usize::MAX + log_height/2). + r.wait_for_keypress_impl(&mut source); + } + + #[test] + fn test_take_prompt_source_discards_stale_nav_commands() { + let mut r = tui_renderer_100x30(); + let (nav_tx, _key_tx, _shared, conn) = test_hub_connection(); + r.connect_input_hub(Some(conn)); + + // Queue a nav command from before the prompt, then open the prompt. + nav_tx.send(NavCmd::ScrollTop).unwrap(); + let taken = r.take_prompt_source().expect("hub connected"); + // The stale command must NOT be applied by subsequent draws. + r.draw(); + assert_eq!( + r.scroll_offset, 0, + "stale ScrollTop leaked into the prompt: offset={}", + r.scroll_offset + ); + let (source, taken_shared) = taken; + r.restore_prompt_source(source, taken_shared); + } + + #[test] + fn test_drop_clears_tui_active_flag() { + use std::sync::atomic::Ordering; + let _guard = tui_active_test_guard(); + let r = tui_renderer_100x30(); + TUI_ACTIVE.store(true, Ordering::SeqCst); + drop(r); + assert!( + !TUI_ACTIVE.load(Ordering::SeqCst), + "dropping a TUI renderer must clear the active flag" + ); + } }