From 34f33e4ada8f417203d87f3b5858c97add8dad6b Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 10 Oct 2026 19:09:03 +0000 Subject: [PATCH] Keep startup modals protected when terminal input draining times out (#52968) ## Why An unfinished terminal control sequence can prevent buffered input from draining before a startup modal appears. Propagating that timeout aborts modal handling instead of keeping the screen available for an explicit user decision. ## What changed Treat input-drain timeouts as a reason to keep startup modal input quarantined. Discard key, paste, and mouse events while retrying the drain, and continue processing server events. Preserve this protection when another request resolves and when the agent overview is restored after reconnecting. ## Testing Add a Unix PTY regression test covering unfinished control sequences, queued approval and overview shortcuts, unrelated request resolution, and overview reconnection. Verify that buffered input cannot answer an approval or create a task, while fresh shortcuts work after the drain completes. GitOrigin-RevId: 476bedbd45f13a15d0019e4ce2e875406d4a53ca --- codex-rs/tui/src/app.rs | 17 +- codex-rs/tui/src/app/app_server_events.rs | 3 +- codex-rs/tui/src/app/reconnect.rs | 4 + codex-rs/tui/src/app/startup.rs | 17 +- codex-rs/tui/src/app/tests/startup.rs | 191 ++++++++++++++++++++++ codex-rs/tui/src/app/thread_routing.rs | 3 +- 6 files changed, 227 insertions(+), 8 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 9d7d85cd0d..c1eb2d323a 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -655,7 +655,8 @@ pub(crate) struct App { pending_managed_worktree_attach: Option>, /// Keeps protected screens quarantined until initialized chat receives genuine user input. startup_protected_input_boundary: bool, - /// Keeps that boundary armed while a startup approval waits for the typing-idle timer. + /// Keeps that boundary armed while an approval waits for the typing-idle timer or a visible + /// modal's terminal input has not settled. startup_pending_protected_request: bool, /// Invalidates in-flight full rate-limit reads when a newer rolling hard stop arrives. account_email_request_id: Option, @@ -855,6 +856,17 @@ impl App { self.invalidate_right_click_paste(&event); self.finish_clipboard(tui, &event); let event = self.finish_right_click_paste(tui, event); + if self.startup_protected_input_boundary + && self.startup_pending_protected_request + && self.chat_widget.has_active_modal() + && matches!( + &event, + TuiEvent::Key(_) | TuiEvent::Paste(_) | TuiEvent::Mouse(_) + ) + { + self.discard_startup_modal_input(tui)?; + return Ok(AppRunControl::Continue); + } let idle_draw = matches!(event, TuiEvent::Draw); if self.handle_rendered_selection_event(tui, &event)? { return Ok(AppRunControl::Continue); @@ -1121,8 +1133,7 @@ impl App { && self.chat_widget.has_active_modal() && self.startup_protected_input_boundary { - tui.discard_pending_input_before_interactive_screen()?; - self.startup_pending_protected_request = false; + self.discard_startup_modal_input(tui)?; } if self.chat_widget.ambient_pet_image_enabled() { let ambient_pet_area = Rect::new( diff --git a/codex-rs/tui/src/app/app_server_events.rs b/codex-rs/tui/src/app/app_server_events.rs index 560f5d564b..4ceb678804 100644 --- a/codex-rs/tui/src/app/app_server_events.rs +++ b/codex-rs/tui/src/app/app_server_events.rs @@ -282,7 +282,8 @@ impl App { self.chat_widget.dismiss_app_server_request(&request); if self.startup_pending_protected_request { self.startup_pending_protected_request = - self.chat_widget.has_pending_protected_request(); + self.chat_widget.has_active_modal() + || self.chat_widget.has_pending_protected_request(); } } } diff --git a/codex-rs/tui/src/app/reconnect.rs b/codex-rs/tui/src/app/reconnect.rs index 00a9853c56..39336fade6 100644 --- a/codex-rs/tui/src/app/reconnect.rs +++ b/codex-rs/tui/src/app/reconnect.rs @@ -481,6 +481,10 @@ impl App { let view = self.agents_overview_view(threads, selected); self.chat_widget.show_bottom_pane_view(Box::new(view)); self.refresh_agents_overview_threads(app_server); + // The Overview survived request cleanup, but terminal typeahead may have survived too. + if self.startup_protected_input_boundary { + self.render_startup_frame(tui, app_event_rx)?; + } } #[cfg(any(target_os = "windows", test))] if interrupted_windows_setup { diff --git a/codex-rs/tui/src/app/startup.rs b/codex-rs/tui/src/app/startup.rs index bfc9c7d210..a684a56c0c 100644 --- a/codex-rs/tui/src/app/startup.rs +++ b/codex-rs/tui/src/app/startup.rs @@ -174,8 +174,21 @@ impl App { self.chat_widget.pre_draw_tick(); self.render_chat_widget_frame(tui, tui.terminal.last_known_screen_size)?; if self.chat_widget.has_active_modal() && self.startup_protected_input_boundary { - tui.discard_pending_input_before_interactive_screen()?; - self.startup_pending_protected_request = false; + self.discard_startup_modal_input(tui)?; + } + Ok(()) + } + + /// Keep a visible modal closed to input if the terminal has not settled yet. The next input + /// event retries the drain instead of acting on the modal; server events can continue meanwhile. + pub(super) fn discard_startup_modal_input(&mut self, tui: &mut tui::Tui) -> Result<()> { + match tui.discard_pending_input_before_interactive_screen() { + Ok(()) => self.startup_pending_protected_request = false, + Err(err) if err.kind() == std::io::ErrorKind::TimedOut => { + self.startup_pending_protected_request = true; + tracing::warn!(%err, "keeping startup modal input quarantined"); + } + Err(err) => return Err(err.into()), } Ok(()) } diff --git a/codex-rs/tui/src/app/tests/startup.rs b/codex-rs/tui/src/app/tests/startup.rs index c4c0fee68e..da40cb7809 100644 --- a/codex-rs/tui/src/app/tests/startup.rs +++ b/codex-rs/tui/src/app/tests/startup.rs @@ -777,6 +777,197 @@ async fn later_thread_approval_preserves_input_after_startup_boundary_ends() -> Ok(()) } +#[cfg(unix)] +#[tokio::test] +async fn unattended_modals_survive_unfinished_terminal_input() -> Result<()> { + use std::os::fd::FromRawFd; + use std::process::Command; + use std::process::Stdio; + + // Isolate crossterm's global parser and give its input poll a real terminal, as in + // tui_startup_tests. No key is delivered to the app before the approval. + if std::env::var_os("CODEX_UNATTENDED_MODALS_PTY").is_none() { + let mut master = -1; + let mut slave = -1; + // Safety: openpty initializes both file descriptors on success. + assert_eq!( + unsafe { + libc::openpty( + &mut master, + &mut slave, + std::ptr::null_mut(), + std::ptr::null_mut(), + std::ptr::null_mut(), + ) + }, + 0, + ); + // Safety: openpty transferred ownership of these descriptors. + let _master = unsafe { std::fs::File::from_raw_fd(master) }; + let slave = unsafe { std::fs::File::from_raw_fd(slave) }; + let output = Command::new(std::env::current_exe()?) + .args([ + "--exact", + "app::tests::startup::unattended_modals_survive_unfinished_terminal_input", + "--nocapture", + ]) + .env("CODEX_UNATTENDED_MODALS_PTY", "1") + .stdin(Stdio::from(slave)) + .output()?; + assert!( + output.status.success(), + "PTY child failed: {}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + return Ok(()); + } + + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + app.startup_protected_input_boundary = true; + let thread_id = ThreadId::new(); + app.enqueue_primary_thread_session( + test_thread_session(thread_id, test_path_buf("/tmp/project")), + Vec::new(), + ) + .await?; + while app_event_rx.try_recv().is_ok() {} + let mut tui = crate::tui::test_support::make_test_tui()?; + let mut app_server = + crate::start_embedded_app_server_for_picker(app.chat_widget.config_ref()).await?; + crossterm::event::buffer_input(b"\x1b]10;")?; + let request = exec_approval_request(thread_id, "turn-1", "call-1", /*approval_id*/ None); + let _ = app + .pending_app_server_requests + .note_server_request(&request); + app.enqueue_primary_thread_request(request).await?; + let event = app.active_thread_rx.as_mut().unwrap().try_recv()?; + app.handle_active_thread_event(&mut tui, &mut app_server, event) + .await?; + assert!(app.chat_widget.has_active_modal()); + + // A separate tracked request may resolve while the visible approval's input is quarantined. + let mut other = exec_approval_request( + ThreadId::new(), + "turn-2", + "call-2", + /*approval_id*/ None, + ); + let ServerRequest::CommandExecutionRequestApproval { request_id, params } = &mut other else { + unreachable!("exec approval helper returns an exec approval"); + }; + *request_id = AppServerRequestId::Integer(2); + let resolved = codex_app_server_protocol::ServerRequestResolvedNotification { + thread_id: params.thread_id.clone(), + request_id: request_id.clone(), + }; + let _ = app.pending_app_server_requests.note_server_request(&other); + app.handle_app_server_event( + &app_server, + codex_app_server_client::AppServerEvent::ServerNotification(Box::new( + ServerNotification::ServerRequestResolved(resolved), + )), + ) + .await; + + // Action keys inside the unfinished sequence and immediately after its terminator are + // still typeahead. Even the first decoded key must retry the drain, not accept the prompt. + crossterm::event::buffer_input(b"1y\r\x1b\\y\r")?; + let crossterm::event::Event::Key(key) = crossterm::event::read()? else { + panic!("expected queued approval shortcut"); + }; + app.handle_tui_event(&mut tui, &mut app_server, TuiEvent::Key(key)) + .await?; + assert!(app.chat_widget.has_active_modal()); + while let Ok(event) = app_event_rx.try_recv() { + assert!( + !matches!(event, AppEvent::SubmitThreadOp { .. }), + "typeahead answered unattended approval: {event:?}" + ); + } + + // After the drain has settled a fresh explicit decision works normally. + app.handle_tui_event( + &mut tui, + &mut app_server, + TuiEvent::Key(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)), + ) + .await?; + assert!(!app.chat_widget.has_active_modal()); + + // Overview can open before the first real input, and survives a server reconnect. + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + app.startup_protected_input_boundary = true; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + app.app_server_target = AppServerTarget::Remote { + endpoint: crate::resolve_remote_addr(&format!("ws://{}", listener.local_addr()?))?, + }; + let server = tokio::spawn(async move { + let (stream, _) = listener.accept().await?; + super::disconnect::serve_reconnect_requests( + tokio_tungstenite::accept_async(stream).await?, + |request| { + std::future::ready(Some(match request.method.as_str() { + "thread/list" | "thread/loaded/list" => { + serde_json::json!({"result": {"data": [], "nextCursor": null}}) + } + method => panic!("unexpected overview reconnect request: {method}"), + })) + }, + ) + .await + }); + let view = app.agents_overview_view(Vec::new(), /*selected_thread_id*/ None); + app.chat_widget.show_bottom_pane_view(Box::new(view)); + crossterm::event::buffer_input(b"\x1b]10;")?; + app.render_startup_frame(&mut tui, &app_event_rx)?; + assert!(app.begin_reconnect()); + let connected = super::super::reconnect::reconnect( + app.app_server_target.clone(), + app.config.clone(), + app.local_settings.clone(), + /*thread_id*/ None, + /*remote_cwd*/ None, + app_server.thread_tool_transport(), + super::super::reconnect::ReconnectPresentation::Overview, + ) + .await?; + app.finish_reconnect( + &mut tui, + &mut app_server, + &mut app_event_rx, + connected, + CODEX_CLI_VERSION, + ) + .await?; + + crossterm::event::buffer_input(b"1n\r\x1b\\n\r")?; + let crossterm::event::Event::Key(key) = crossterm::event::read()? else { + panic!("expected queued overview shortcut"); + }; + app.handle_tui_event(&mut tui, &mut app_server, TuiEvent::Key(key)) + .await?; + assert!( + !std::iter::from_fn(|| app_event_rx.try_recv().ok()) + .any(|event| matches!(event, AppEvent::NewAgentsOverviewSession { .. })), + "typeahead created a task in restored Overview" + ); + app.handle_tui_event( + &mut tui, + &mut app_server, + TuiEvent::Key(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)), + ) + .await?; + assert!( + std::iter::from_fn(|| app_event_rx.try_recv().ok()) + .any(|event| matches!(event, AppEvent::NewAgentsOverviewSession { .. })), + "fresh Overview shortcut should create a task" + ); + app_server.shutdown().await?; + server.await??; + Ok(()) +} + #[tokio::test] async fn auto_declined_mcp_elicitations_do_not_leave_startup_quarantine_armed() { for replay in [false, true] { diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index 9197df86bf..eedd0929a9 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -2241,8 +2241,7 @@ impl App { { self.chat_widget.pre_draw_tick(); self.render_chat_widget_frame(tui, tui.terminal.last_known_screen_size)?; - tui.discard_pending_input_before_interactive_screen()?; - self.startup_pending_protected_request = false; + self.discard_startup_modal_input(tui)?; } if self.backtrack_render_pending { tui.frame_requester().schedule_frame();