diff --git a/CHANGELOG.md b/CHANGELOG.md index af669fc9..b1b09709 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,10 @@ Feishu/Lark/mobile companion work remain out of scope for this release. scroll margins/origin mode before key repaints after resume, resize, and turn completion, preventing alt-screen content from drifting downward and leaving blank rows at the top. +- **Interactive subprocesses wait for terminal release** (#1085) - shell/editor + handoff now waits until the UI has actually left alt-screen/raw mode before + launching the child process, preventing the TUI from repainting into host + scrollback after interactive tool use. - **Light theme reasoning blocks stay light** (#1070, #936 partial) - thinking/reasoning background tints now map to the light reasoning surface instead of keeping the dark-mode tint after `/theme light`. diff --git a/crates/tui/src/core/engine/tool_execution.rs b/crates/tui/src/core/engine/tool_execution.rs index e1a41343..47d0e2b6 100644 --- a/crates/tui/src/core/engine/tool_execution.rs +++ b/crates/tui/src/core/engine/tool_execution.rs @@ -4,7 +4,7 @@ //! parallel-tool fanout out of `engine.rs`; the turn loop still owns planning, //! approval, and how tool results are written back into session state. -use std::{fs::OpenOptions, io::Write}; +use std::{fs::OpenOptions, io::Write, sync::Arc, time::Duration}; use super::*; @@ -21,9 +21,11 @@ use super::*; /// after. That worked on the happy path, but if the tool's future was dropped /// — Ctrl+C cancellation, sub-agent abort, parent task cancelled while the /// tool was awaiting — the second `await` never reached and `ResumeEvents` -/// was never sent. The terminal stayed paused: parent shell scrollbar took -/// over, mouse wheel scrolled the host terminal instead of the transcript, -/// and the TUI rendered as if into a regular cooked-mode buffer. +/// was never sent. It also let interactive children start before the UI had +/// actually left alt-screen/raw mode. Both failures strand the TUI in a +/// regular shell scrollback: the parent shell scrollbar takes over, mouse +/// wheel scrolls the host terminal instead of the transcript, and the TUI +/// renders at the bottom of cooked-mode output. /// /// `Drop` runs synchronously and can't await, so we first use `try_send` on a /// **clone of the event channel** to push `ResumeEvents` non-blockingly. If the @@ -42,9 +44,35 @@ impl InteractiveTerminalGuard { return Self { tx: None }; } // Best-effort: if the receiver is gone the TUI has already shut down - // and there's nothing to restore. Either way we still arm the guard - // so `Drop` symmetrically tries the resume. - let _ = tx.send(Event::PauseEvents).await; + // and there's nothing to restore. If the event is delivered, wait for + // the UI to actually release the terminal before starting the child. + let ack = Arc::new(tokio::sync::Notify::new()); + match tx + .send(Event::PauseEvents { + ack: Some(ack.clone()), + }) + .await + { + Ok(()) => { + if tokio::time::timeout(Duration::from_millis(750), ack.notified()) + .await + .is_err() + { + tracing::warn!( + target: "engine.tool_execution", + "InteractiveTerminalGuard: timed out waiting for terminal pause ack; \ + continuing with interactive tool" + ); + } + } + Err(err) => { + tracing::debug!( + target: "engine.tool_execution", + ?err, + "InteractiveTerminalGuard: event channel closed before PauseEvents" + ); + } + } Self { tx: Some(tx) } } } @@ -306,6 +334,37 @@ mod tests { assert!(matches!(resumed, Event::ResumeEvents)); } + #[tokio::test] + async fn terminal_guard_waits_for_pause_ack_before_returning() { + let (tx, mut rx) = mpsc::channel(4); + let task = tokio::spawn(InteractiveTerminalGuard::engage(tx, true)); + + let event = tokio::time::timeout(Duration::from_secs(1), rx.recv()) + .await + .expect("pause event") + .expect("event channel still open"); + let ack = match event { + Event::PauseEvents { ack: Some(ack) } => ack, + other => panic!("expected PauseEvents with ack, got {other:?}"), + }; + + tokio::task::yield_now().await; + assert!(!task.is_finished(), "guard returned before pause ack"); + + ack.notify_one(); + let guard = tokio::time::timeout(Duration::from_secs(1), task) + .await + .expect("guard returned after ack") + .expect("guard task joined"); + + drop(guard); + let resumed = tokio::time::timeout(Duration::from_secs(1), rx.recv()) + .await + .expect("resume event") + .expect("event channel still open"); + assert!(matches!(resumed, Event::ResumeEvents)); + } + #[test] fn emit_tool_audit_writes_jsonl_line_when_env_var_set() { let _g = audit_test_guard(); diff --git a/crates/tui/src/core/events.rs b/crates/tui/src/core/events.rs index 5f680fad..74d6cd44 100644 --- a/crates/tui/src/core/events.rs +++ b/crates/tui/src/core/events.rs @@ -3,7 +3,7 @@ //! These events flow from the engine to the TUI via a channel, //! enabling non-blocking, real-time updates. -use std::path::PathBuf; +use std::{path::PathBuf, sync::Arc}; use serde_json::Value; @@ -211,8 +211,12 @@ pub enum Event { /// Status message for UI display Status { message: String }, - /// Pause terminal input events (for interactive subprocesses) - PauseEvents, + /// Pause terminal input events (for interactive subprocesses). + PauseEvents { + /// Optional one-shot notification fired after the UI has actually + /// released the terminal to the child process. + ack: Option>, + }, /// Resume terminal input events after subprocess completion ResumeEvents, diff --git a/crates/tui/src/tui/ui.rs b/crates/tui/src/tui/ui.rs index af3024a0..74b6ba09 100644 --- a/crates/tui/src/tui/ui.rs +++ b/crates/tui/src/tui/ui.rs @@ -1118,7 +1118,7 @@ async fn run_event_loop( "Capacity memory persist failed ({action}): {error}" )); } - EngineEvent::PauseEvents => { + EngineEvent::PauseEvents { ack } => { if !event_broker.is_paused() { pause_terminal( terminal, @@ -1129,6 +1129,9 @@ async fn run_event_loop( event_broker.pause_events(); terminal_paused_at = Some(Instant::now()); } + if let Some(ack) = ack { + ack.notify_one(); + } } EngineEvent::ResumeEvents => { if event_broker.is_paused() {