diff --git a/codex-rs/tui/src/app/input.rs b/codex-rs/tui/src/app/input.rs index ce74740ce5..0df9cc7836 100644 --- a/codex-rs/tui/src/app/input.rs +++ b/codex-rs/tui/src/app/input.rs @@ -261,7 +261,7 @@ impl App { let config = self.chat_widget.config_ref(); let file_system_policy = config.permissions.file_system_sandbox_policy(); let editor_result = tui - .with_restored(|| async { + .with_restored(tui::TerminalHandoff::KeepScreen, || async { external_editor::run_editor( &seed, &editor_cmd, diff --git a/codex-rs/tui/src/custom_terminal.rs b/codex-rs/tui/src/custom_terminal.rs index 6f7d5cf5a0..60081516bd 100644 --- a/codex-rs/tui/src/custom_terminal.rs +++ b/codex-rs/tui/src/custom_terminal.rs @@ -272,7 +272,7 @@ where } /// Gets the previous buffer as a reference. - fn previous_buffer(&self) -> &Buffer { + pub(crate) fn previous_buffer(&self) -> &Buffer { &self.buffers[1 - self.current] } diff --git a/codex-rs/tui/src/daemon_recovery.rs b/codex-rs/tui/src/daemon_recovery.rs index 0a9e8faf09..2c9959201e 100644 --- a/codex-rs/tui/src/daemon_recovery.rs +++ b/codex-rs/tui/src/daemon_recovery.rs @@ -94,7 +94,7 @@ pub(super) async fn check( issue.reason ))), (Some(1), Some(features)) if managed_daemon => { - tui.with_restored(|| async { + tui.with_restored(crate::tui::TerminalHandoff::Restore, || async { crossterm::terminal::disable_raw_mode()?; codex_app_server_daemon::restart_with_features(features) .await diff --git a/codex-rs/tui/src/startup_orchestration.rs b/codex-rs/tui/src/startup_orchestration.rs index cbba29ac3f..a19900f75a 100644 --- a/codex-rs/tui/src/startup_orchestration.rs +++ b/codex-rs/tui/src/startup_orchestration.rs @@ -388,7 +388,7 @@ pub(super) async fn run_main_inner( startup_draft.flush_pending_events().await?; startup_draft .tui_mut() - .with_restored(|| { + .with_restored(crate::tui::TerminalHandoff::Restore, || { oss_selection::select_oss_provider(lmstudio_status, ollama_status) }) .await? @@ -526,7 +526,7 @@ pub(super) async fn run_main_inner( startup_draft.flush_pending_events().await?; let output = startup_draft .tui_mut() - .with_restored(|| async { + .with_restored(crate::tui::TerminalHandoff::Restore, || async { // Package installation may print progress; keep ordinary Ctrl+C handling. crossterm::terminal::disable_raw_mode()?; let result = codex_app_server_daemon::start_with_features(&daemon_features).await; @@ -602,7 +602,7 @@ pub(super) async fn run_main_inner( startup_draft.flush_pending_events().await?; startup_draft .tui_mut() - .with_restored(|| async { + .with_restored(crate::tui::TerminalHandoff::Restore, || async { #[allow(clippy::print_stderr)] { eprintln!("Could not create otel exporter: {e}"); @@ -815,7 +815,7 @@ pub(super) async fn run_main_inner( startup_draft.flush_pending_events().await?; startup_draft .tui_mut() - .with_restored(|| async { + .with_restored(crate::tui::TerminalHandoff::Restore, || async { // Provider setup may print progress or block in an external downloader. // Restore ordinary signal handling so Ctrl+C can interrupt that process. crossterm::terminal::disable_raw_mode()?; diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index dc14abf35d..dbe264f4b8 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -315,13 +315,30 @@ enum KeyboardRestore { ResetAfterExit, } +#[derive(Clone, Copy)] +pub(crate) enum TerminalHandoff { + Restore, + KeepScreen, +} + fn restore_common( raw_mode_restore: RawModeRestore, keyboard_restore: KeyboardRestore, + handoff: TerminalHandoff, ) -> Result<()> { let mut first_error = ensure_virtual_terminal_processing().err(); - if let Err(err) = ALTERNATE_SCREEN.restore(&mut stdout(), keyboard_restore) { + let screen_result = match handoff { + TerminalHandoff::KeepScreen if ALTERNATE_SCREEN.is_active() => { + let result = ALTERNATE_SCREEN.release_input(&mut stdout()); + let _ = execute!(stdout(), keyboard_modes::DisableModifyOtherKeys); + result + } + TerminalHandoff::Restore | TerminalHandoff::KeepScreen => { + ALTERNATE_SCREEN.restore(&mut stdout(), keyboard_restore) + } + }; + if let Err(err) = screen_result { first_error.get_or_insert(err); } @@ -355,7 +372,11 @@ fn restore_common( /// Inverse of `set_modes`. #[cfg(unix)] pub fn restore() -> Result<()> { - restore_common(RawModeRestore::Disable, KeyboardRestore::PopStack) + restore_common( + RawModeRestore::Disable, + KeyboardRestore::PopStack, + TerminalHandoff::Restore, + ) } /// Force crossterm's cached raw-mode state back in sync with the terminal after `fg`. @@ -375,8 +396,12 @@ pub(super) fn reapply_raw_mode_after_resume() -> Result<()> { /// Uses a stronger keyboard reset than `restore` so the parent shell recovers even if a /// terminal missed the stack pop that normally pairs with [`set_modes`]. pub fn restore_after_exit() -> Result<()> { - let mut first_error = - restore_common(RawModeRestore::Disable, KeyboardRestore::ResetAfterExit).err(); + let mut first_error = restore_common( + RawModeRestore::Disable, + KeyboardRestore::ResetAfterExit, + TerminalHandoff::Restore, + ) + .err(); if let Err(err) = terminal_stderr::finish() { first_error.get_or_insert(err); } @@ -388,8 +413,8 @@ pub fn restore_after_exit() -> Result<()> { } /// Restore the terminal to its original state, but keep raw mode enabled. -pub fn restore_keep_raw() -> Result<()> { - restore_common(RawModeRestore::Keep, KeyboardRestore::PopStack) +fn restore_keep_raw(handoff: TerminalHandoff) -> Result<()> { + restore_common(RawModeRestore::Keep, KeyboardRestore::PopStack, handoff) } /// Flush the underlying stdin buffer to clear any input that may be buffered at the terminal level. @@ -887,7 +912,7 @@ impl Tui { /// This pauses crossterm's stdin polling by dropping the underlying event stream, restores /// terminal modes and stderr while keeping raw mode enabled, then re-applies Codex TUI modes /// and stderr suppression before resuming events. - pub async fn with_restored(&mut self, f: F) -> R + pub(crate) async fn with_restored(&mut self, handoff: TerminalHandoff, f: F) -> R where F: FnOnce() -> Fut, Fut: Future, @@ -895,13 +920,47 @@ impl Tui { // Pause crossterm events to avoid stdin conflicts with external program `f`. self.pause_events(); - // Leave alt screen if active to avoid conflicts with external program `f`. + // Restore both screens' input modes before giving the terminal to an editor. Repaint the + // last frame on the alternate screen so separate-window editors leave Codex visible. let was_alt_screen = self.is_alt_screen_active(); - if was_alt_screen { - let _ = self.leave_alt_screen_for_handoff(); - } - - if let Err(err) = restore_keep_raw() { + let visible_frame = (was_alt_screen && matches!(handoff, TerminalHandoff::KeepScreen)) + .then(|| self.terminal.previous_buffer().clone()); + let restore_result = if let Some(visible_frame) = visible_frame { + stdout() + .sync_update(|_| { + let leave_result = self.leave_alt_screen_for_handoff(); + let main_result = restore_keep_raw(TerminalHandoff::Restore); + let enter_result = self.enter_alt_screen(); + if !self.is_alt_screen_active() { + return leave_result.and(main_result).and(enter_result); + } + let draw_result = if enter_result.is_ok() { + self.terminal.draw(|frame| { + let area = frame.area().intersection(visible_frame.area); + for y in area.top()..area.bottom() { + for x in area.left()..area.right() { + frame.buffer_mut()[(x, y)] = visible_frame[(x, y)].clone(); + } + } + }) + } else { + Ok(()) + }; + let input_result = restore_keep_raw(TerminalHandoff::KeepScreen); + leave_result + .and(main_result) + .and(enter_result) + .and(draw_result) + .and(input_result) + }) + .and_then(std::convert::identity) + } else { + if was_alt_screen { + let _ = self.leave_alt_screen_for_handoff(); + } + restore_keep_raw(TerminalHandoff::Restore) + }; + if let Err(err) = restore_result { tracing::warn!("failed to restore terminal modes before external program: {err}"); } if let Err(err) = terminal_stderr::pause() { @@ -910,6 +969,12 @@ impl Tui { let output = f().await; + if was_alt_screen && matches!(handoff, TerminalHandoff::KeepScreen) { + // The editor may already have left the alternate screen. Return to the main screen + // before reapplying its keyboard mode and entering the alternate screen again. + let _ = self.leave_alt_screen_for_handoff(); + } + if let Err(err) = terminal_stderr::resume() { tracing::warn!("failed to suppress terminal stderr after external program: {err}"); } diff --git a/codex-rs/tui/src/tui/alternate_screen.rs b/codex-rs/tui/src/tui/alternate_screen.rs index 8cc8ca46e2..c9c6e76c50 100644 --- a/codex-rs/tui/src/tui/alternate_screen.rs +++ b/codex-rs/tui/src/tui/alternate_screen.rs @@ -1,9 +1,9 @@ //! Pair alternate-screen transitions with that screen's independent keyboard-mode stack. //! -//! Push only on entry and pop before leaving; the main screen keeps its own TUI mode until -//! terminal handoff. Transcript surfaces retain pointer reporting across overlays and disable it -//! before yielding to the shell. Promoting an overlay must not push another keyboard frame. -//! Refresh tmux's input policy on entry and apply it to every mouse-capture request. +//! Push only on entry and pop before leaving or yielding input to an editor. The main screen +//! keeps its own TUI mode until the handoff returns to it. Transcript surfaces retain pointer +//! reporting across overlays and disable it before yielding. Promoting an overlay must not push +//! another keyboard frame. Refresh tmux's input policy on entry for mouse-capture requests. use std::io::Result; use std::io::Write; @@ -32,6 +32,7 @@ pub(super) static ALTERNATE_SCREEN: AlternateScreen = AlternateScreen { mouse_active: AtomicBool::new(/*v*/ false), mouse_capture_disabled: AtomicBool::new(/*v*/ false), input_configured: AtomicBool::new(/*v*/ false), + keyboard_active: AtomicBool::new(/*v*/ false), }; #[derive(Default)] @@ -42,6 +43,8 @@ pub(super) struct AlternateScreen { mouse_capture_disabled: AtomicBool, // A cleanup/setup error must not make the next identical request look already applied. input_configured: AtomicBool, + // An editor can take over the screen after Codex has popped its keyboard mode. + keyboard_active: AtomicBool, } /// Report pointer motion so the owned transcript can update its return-to-bottom hover state. @@ -74,6 +77,10 @@ impl AlternateScreen { self.active.store(/*val*/ true, Ordering::Relaxed); writer.flush()?; let mouse_capture = keyboard_modes::enable_keyboard_enhancement(writer); + self.keyboard_active.store( + !cfg!(windows) && !keyboard_modes::keyboard_enhancement_disabled(), + Ordering::Relaxed, + ); self.mouse_capture_disabled.store( mouse_capture == MouseCapture::DisabledByTmux, Ordering::Relaxed, @@ -127,29 +134,36 @@ impl AlternateScreen { Ok(()) } - pub(super) fn leave(&self, writer: &mut impl Write) -> Result<()> { + /// Release input modes without changing the screen, so an editor can take it over. + pub(super) fn release_input(&self, writer: &mut impl Write) -> Result<()> { self.input_configured .store(/*val*/ false, Ordering::Relaxed); let mouse_result = self.disable_mouse(writer); // Crossterm never pushes a keyboard stack on native Windows: its input-record API // already reports enhanced keys, and its push/pop commands return Unsupported. - let keyboard_result = if cfg!(windows) || keyboard_modes::keyboard_enhancement_disabled() { - Ok(()) - } else { + let keyboard_result = if self.keyboard_active.load(Ordering::Relaxed) { // modifyOtherKeys is not stacked per screen; keep the main screen's fallback enabled // until terminal handoff restores its keyboard modes too. - execute!(writer, PopKeyboardEnhancementFlags) + let result = execute!(writer, PopKeyboardEnhancementFlags); + if result.is_ok() { + self.keyboard_active.store(/*val*/ false, Ordering::Relaxed); + } + result + } else { + Ok(()) }; let scroll_result = execute!(writer, DisableAlternateScroll); + mouse_result.and(keyboard_result).and(scroll_result) + } + + pub(super) fn leave(&self, writer: &mut impl Write) -> Result<()> { + let input_result = self.release_input(writer); // A failed earlier cleanup write must not prevent the actual screen transition. let screen_result = execute!(writer, LeaveAlternateScreen); if screen_result.is_ok() { self.active.store(/*val*/ false, Ordering::Relaxed); } - mouse_result - .and(keyboard_result) - .and(scroll_result) - .and(screen_result) + input_result.and(screen_result) } pub(super) fn restore( diff --git a/codex-rs/tui/src/tui/alternate_screen_tests.rs b/codex-rs/tui/src/tui/alternate_screen_tests.rs index 2be750edd2..799c8eb83e 100644 --- a/codex-rs/tui/src/tui/alternate_screen_tests.rs +++ b/codex-rs/tui/src/tui/alternate_screen_tests.rs @@ -92,6 +92,45 @@ impl KeyboardScreens { } } +#[test] +fn editor_handoff_balances_keyboard_stacks_if_editor_leaves_the_screen() { + if keyboard_modes::keyboard_enhancement_disabled() { + return; + } + for editor_leaves in [false, true] { + let screen = AlternateScreen::default(); + let mut output = Vec::new(); + let mut keyboard = KeyboardScreens::default(); + execute!( + output, + PushKeyboardEnhancementFlags(KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES) + ) + .unwrap(); + keyboard_modes::enable_keyboard_enhancement(&mut output); + screen.enter(&mut output, /*capture_mouse*/ true).unwrap(); + screen.leave(&mut output).unwrap(); + screen + .restore(&mut output, KeyboardRestore::PopStack) + .unwrap(); + screen.enter(&mut output, /*capture_mouse*/ true).unwrap(); + screen.release_input(&mut output).unwrap(); + keyboard.process(&std::mem::take(&mut output)); + assert_eq!(keyboard.state(), (true, &[1][..], &[][..])); + + if editor_leaves { + execute!(output, LeaveAlternateScreen).unwrap(); + } + screen.leave(&mut output).unwrap(); + keyboard_modes::enable_keyboard_enhancement(&mut output); + screen.enter(&mut output, /*capture_mouse*/ true).unwrap(); + screen + .restore(&mut output, KeyboardRestore::PopStack) + .unwrap(); + keyboard.process(&output); + assert_eq!(keyboard.state(), (false, &[1][..], &[][..])); + } +} + // Capture-specific tests must not inherit the tmux session running the test binary. fn enter_with_mouse_enabled( screen: &AlternateScreen, diff --git a/codex-rs/tui/src/tui/keyboard_modes.rs b/codex-rs/tui/src/tui/keyboard_modes.rs index 335be9fa4d..4dddb97f78 100644 --- a/codex-rs/tui/src/tui/keyboard_modes.rs +++ b/codex-rs/tui/src/tui/keyboard_modes.rs @@ -337,7 +337,7 @@ impl Command for EnableModifyOtherKeys { } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -struct DisableModifyOtherKeys; +pub(super) struct DisableModifyOtherKeys; impl Command for DisableModifyOtherKeys { fn write_ansi(&self, f: &mut impl fmt::Write) -> fmt::Result { diff --git a/codex-rs/tui/tests/suite/external_editor_tests.rs b/codex-rs/tui/tests/suite/external_editor_tests.rs new file mode 100644 index 0000000000..8b58e629da --- /dev/null +++ b/codex-rs/tui/tests/suite/external_editor_tests.rs @@ -0,0 +1,105 @@ +//! Exercise editor handoff through a terminal, including editors that switch screens. + +use super::PtyCodex; +use super::write_test_config; +use anyhow::Result; +use anyhow::ensure; +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::time::Duration; +use std::time::Instant; + +#[test] +fn external_editors_keep_the_screen_and_return_the_draft() -> Result<()> { + let repo_root = codex_utils_cargo_bin::repo_root()?; + for (editor_output, fullscreen) in [ + ("", true), + ("printf '\\033[?1049h\\033[2J\\033[Hterminal editor'", true), + ("printf '\\033[2J\\033[Hterminal editor'", true), + ("printf '\\033[?1049l\\033[2J\\033[Hterminal editor'", true), + ("", false), + ] { + let home = tempfile::tempdir()?; + write_test_config(home.path(), &repo_root)?; + let editor = home.path().join("editor.sh"); + fs::write( + &editor, + format!( + "#!/bin/sh\n{editor_output}\n: > \"$0.ready\"\nIFS= read -r ignored\n\ + {exit_screen}\nprintf 'edited draft' > \"$1\"\n", + exit_screen = if editor_output.contains("1049h") { + "printf '\\033[?1049l'" + } else { + "" + }, + ), + )?; + fs::set_permissions(&editor, fs::Permissions::from_mode(0o755))?; + let args = if fullscreen { + vec!["-c", "sandbox_mode=\"read-only\""] + } else { + vec![ + "-c", + "sandbox_mode=\"read-only\"", + "-c", + "tui.fullscreen_transcript=false", + ] + }; + let codex = codex_utils_cargo_bin::cargo_bin("codex-tui") + .or_else(|_| codex_utils_cargo_bin::cargo_bin("codex"))?; + let mut terminal = PtyCodex::start_binary(&codex, &repo_root, home, &args, Some(&editor))?; + terminal.wait_for_startup()?; + terminal.wait_for_screen("GPT-5.6-Terra")?; + terminal.write_input(b"initial draft")?; + terminal.wait_for_screen("initial draft")?; + terminal.write_input(b"\x07")?; + + let deadline = Instant::now() + Duration::from_secs(/*secs*/ 10); + while !editor.with_extension("sh.ready").exists() && Instant::now() < deadline { + terminal.read_output(Duration::from_millis(/*millis*/ 50))?; + terminal.ensure_running()?; + } + ensure!( + editor.with_extension("sh.ready").exists(), + "editor did not start" + ); + + let mut visible = None; + if editor_output.is_empty() { + terminal.wait_for_screen("Save and close external editor to continue.")?; + ensure!(terminal.screen_contains("initial draft")); + if fullscreen { + ensure!(terminal.screen_contains("OpenAI Codex")); + ensure!(terminal.screen_contains("GPT-5.6-Terra")); + } + ensure!( + terminal.parser.screen().mouse_protocol_mode() == vt100::MouseProtocolMode::None + ); + if fullscreen { + visible = Some( + terminal + .screen_contents() + .lines() + .filter(|line| { + line.contains("initial draft") + || line.contains("Save and close external editor to continue.") + }) + .collect::>() + .join("\n"), + ); + } + } else { + terminal.wait_for_screen("terminal editor")?; + } + + terminal.write_input(b"\n")?; + terminal.wait_for_screen("edited draft")?; + ensure!(terminal.parser.screen().alternate_screen() == fullscreen); + terminal.write_input(b"!")?; + terminal.wait_for_screen("edited draft!")?; + if let Some(visible) = visible { + insta::assert_snapshot!("external_editor_visible_handoff", visible); + } + } + Ok(()) +} diff --git a/codex-rs/tui/tests/suite/focus_palette.rs b/codex-rs/tui/tests/suite/focus_palette.rs index f280efda51..fa47056984 100644 --- a/codex-rs/tui/tests/suite/focus_palette.rs +++ b/codex-rs/tui/tests/suite/focus_palette.rs @@ -22,6 +22,8 @@ const STARTUP_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); const FOCUS_INPUT_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 5); const FOCUS_PROBE_INPUT: &str = "focus-palette-24527"; +#[path = "external_editor_tests.rs"] +mod external_editor; #[path = "tui_mode_picker_tests.rs"] mod tui_mode_picker; @@ -291,7 +293,9 @@ impl PtyCodex { ) -> Result { let codex = codex_utils_cargo_bin::cargo_bin("codex-tui") .or_else(|_| codex_utils_cargo_bin::cargo_bin("codex"))?; - Self::start_binary(&codex, repo_root, codex_home, extra_args) + Self::start_binary( + &codex, repo_root, codex_home, extra_args, /*editor*/ None, + ) } /// Include the CLI dispatch futures when testing production stack headroom. @@ -302,7 +306,9 @@ impl PtyCodex { ) -> Result { let codex = codex_utils_cargo_bin::cargo_bin("codex") .context("build codex-cli and set CARGO_BIN_EXE_codex to its executable")?; - Self::start_binary(&codex, repo_root, codex_home, extra_args) + Self::start_binary( + &codex, repo_root, codex_home, extra_args, /*editor*/ None, + ) } fn start_binary( @@ -310,6 +316,7 @@ impl PtyCodex { repo_root: &Path, codex_home: TempDir, extra_args: &[&str], + editor: Option<&Path>, ) -> Result { let mut master_fd = -1; let mut slave_fd = -1; @@ -342,7 +349,11 @@ impl PtyCodex { let stdin = slave.try_clone().context("clone pseudo-terminal stdin")?; let stdout = slave.try_clone().context("clone pseudo-terminal stdout")?; - let child = Command::new(codex) + let mut command = Command::new(codex); + if let Some(editor) = editor { + command.env("VISUAL", editor); + } + let child = command .args(extra_args) .arg("-C") .arg(repo_root) diff --git a/codex-rs/tui/tests/suite/snapshots/all__suite__focus_palette__external_editor__external_editor_visible_handoff.snap b/codex-rs/tui/tests/suite/snapshots/all__suite__focus_palette__external_editor__external_editor_visible_handoff.snap new file mode 100644 index 0000000000..18945d1721 --- /dev/null +++ b/codex-rs/tui/tests/suite/snapshots/all__suite__focus_palette__external_editor__external_editor_visible_handoff.snap @@ -0,0 +1,7 @@ +--- +source: tui/tests/suite/external_editor_tests.rs +assertion_line: 89 +expression: visible +--- +› initial draft + Save and close external editor to continue.