diff --git a/codex-rs/tui/src/tui/keyboard_modes.rs b/codex-rs/tui/src/tui/keyboard_modes.rs index 66e8ea7976..83e409836c 100644 --- a/codex-rs/tui/src/tui/keyboard_modes.rs +++ b/codex-rs/tui/src/tui/keyboard_modes.rs @@ -3,6 +3,8 @@ //! The TUI uses crossterm's keyboard enhancement stack while it owns the terminal, but //! process exit gets a stronger reset so the parent shell does not inherit enhanced key //! reporting if a terminal misses the normal stack pop. +//! Windows terminal detection has a deadline, including WSL interop process launch. +//! Inconclusive detection keeps keyboard enhancements disabled on WSL. use std::fmt; use std::io::stdout; @@ -17,26 +19,41 @@ use ratatui::crossterm::execute; const DISABLE_KEYBOARD_ENHANCEMENT_ENV_VAR: &str = "CODEX_TUI_DISABLE_KEYBOARD_ENHANCEMENT"; +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum VscodeDetection { + VsCode, + Other, + Unknown, +} + pub(super) fn keyboard_enhancement_disabled() -> bool { let disable_env = std::env::var(DISABLE_KEYBOARD_ENHANCEMENT_ENV_VAR).ok(); let is_wsl = running_in_wsl(); - let is_vscode_terminal = is_wsl && running_in_vscode_terminal(); - keyboard_enhancement_disabled_for(disable_env.as_deref(), is_wsl, is_vscode_terminal) + let vscode_detection = if is_wsl { + detect_vscode_terminal() + } else { + VscodeDetection::Other + }; + keyboard_enhancement_disabled_for(disable_env.as_deref(), is_wsl, vscode_detection) } fn keyboard_enhancement_disabled_for( disable_env: Option<&str>, is_wsl: bool, - is_vscode_terminal: bool, + vscode_detection: VscodeDetection, ) -> bool { if let Some(disabled) = parse_bool_env(disable_env) { return disabled; } // VS Code running a WSL shell can hide TERM_PROGRAM from the Linux process - // environment, so `running_in_vscode_terminal` also probes the Windows-side - // environment through WSL interop. - is_wsl && is_vscode_terminal + // environment. If the Windows-side probe is inconclusive, avoid enabling the + // keyboard mode that can break dead-key composition in VS Code on WSL. + is_wsl + && matches!( + vscode_detection, + VscodeDetection::VsCode | VscodeDetection::Unknown + ) } fn parse_bool_env(value: Option<&str>) -> Option { @@ -64,69 +81,131 @@ fn running_in_wsl() -> bool { } pub(super) fn running_in_vscode_terminal() -> bool { + detect_vscode_terminal() == VscodeDetection::VsCode +} + +fn detect_vscode_terminal() -> VscodeDetection { if term_program_is_vscode(std::env::var("TERM_PROGRAM").ok().as_deref()) { - return true; + return VscodeDetection::VsCode; } vscode_terminal_detected( std::env::var("TERM_PROGRAM").ok().as_deref(), - windows_term_program().as_deref(), + windows_vscode_detection(), ) } fn vscode_terminal_detected( linux_term_program: Option<&str>, - windows_term_program: Option<&str>, -) -> bool { - term_program_is_vscode(linux_term_program) || term_program_is_vscode(windows_term_program) + windows_detection: VscodeDetection, +) -> VscodeDetection { + if term_program_is_vscode(linux_term_program) { + VscodeDetection::VsCode + } else { + windows_detection + } } fn term_program_is_vscode(value: Option<&str>) -> bool { value.is_some_and(|value| value.eq_ignore_ascii_case("vscode")) } -fn windows_term_program() -> Option { +fn windows_vscode_detection() -> VscodeDetection { #[cfg(target_os = "linux")] { - static WINDOWS_TERM_PROGRAM: std::sync::OnceLock> = + static WINDOWS_VSCODE_DETECTION: std::sync::OnceLock = std::sync::OnceLock::new(); - WINDOWS_TERM_PROGRAM - .get_or_init(read_windows_term_program) - .clone() + *WINDOWS_VSCODE_DETECTION.get_or_init(read_windows_vscode_detection) } #[cfg(not(target_os = "linux"))] { - None + VscodeDetection::Unknown } } #[cfg(target_os = "linux")] -fn read_windows_term_program() -> Option { +fn read_windows_vscode_detection() -> VscodeDetection { if !running_in_wsl() { - return None; + return VscodeDetection::Other; } - let executable = codex_utils_path::system_executable("cmd.exe")?; - let output = std::process::Command::new(executable) - .args(["/d", "/s", "/c", "set TERM_PROGRAM"]) - .stdin(std::process::Stdio::null()) - .stderr(std::process::Stdio::null()) - .output() - .ok()?; - - if !output.status.success() { - return None; - } - - String::from_utf8_lossy(&output.stdout) - .lines() - .find_map(|line| { - line.trim_end_matches('\r') - .strip_prefix("TERM_PROGRAM=") - .map(str::to_string) - }) - .filter(|value| !value.trim().is_empty()) + read_windows_vscode_detection_with_timeout( + || { + let executable = codex_utils_path::system_executable("cmd.exe")?; + std::process::Command::new(executable) + .args(["/d", "/s", "/c", "set TERM_PROGRAM"]) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::null()) + .spawn() + .ok() + }, + std::time::Duration::from_secs(/*secs*/ 1), + ) } +#[cfg(any(target_os = "linux", all(test, unix)))] +fn read_windows_vscode_detection_with_timeout( + spawn: impl FnOnce() -> Option + Send + 'static, + timeout: std::time::Duration, +) -> VscodeDetection { + let deadline = std::time::Instant::now() + timeout; + let (sender, receiver) = std::sync::mpsc::channel(); + // WSL interop can block inside spawn itself. Keep launch and cleanup off the + // startup thread; the OnceLock caches a timeout so this worker is not retried. + let worker = std::thread::Builder::new() + .name("windows-term-program".into()) + .spawn(move || { + let output = (|| { + let mut child = spawn()?; + while std::time::Instant::now() < deadline { + match child.try_wait() { + Ok(Some(_)) => return child.wait_with_output().ok(), + Ok(None) => { + std::thread::sleep(std::time::Duration::from_millis(/*millis*/ 10)) + } + Err(_) => break, + } + } + let _ = child.kill(); + let _ = child.wait(); + None + })(); + let _ = sender.send(output); + }); + if worker.is_err() { + return VscodeDetection::Unknown; + } + + let Ok(Some(output)) = + receiver.recv_timeout(deadline.saturating_duration_since(std::time::Instant::now())) + else { + return VscodeDetection::Unknown; + }; + if !output.status.success() { + // `cmd.exe /c set TERM_PROGRAM` exits with 1 when the variable is absent. + return if output.status.code() == Some(1) { + VscodeDetection::Other + } else { + VscodeDetection::Unknown + }; + } + + let output = String::from_utf8_lossy(&output.stdout); + let term_program = output + .lines() + .find_map(|line| line.trim_end_matches('\r').strip_prefix("TERM_PROGRAM=")) + .filter(|value| !value.trim().is_empty()); + if term_program_is_vscode(term_program) { + VscodeDetection::VsCode + } else { + VscodeDetection::Other + } +} + +#[cfg(all(test, unix))] +#[path = "windows_term_program_tests.rs"] +mod windows_term_program_tests; + pub(super) fn enable_keyboard_enhancement() { if keyboard_enhancement_disabled() { return; @@ -318,6 +397,7 @@ mod tests { use super::DisableModifyOtherKeys; use super::EnableModifyOtherKeys; use super::ResetKeyboardEnhancementFlags; + use super::VscodeDetection; use super::keyboard_enhancement_disabled_for; use super::keyboard_enhancement_flags; use super::parse_bool_env; @@ -432,53 +512,60 @@ mod tests { } #[test] - fn keyboard_enhancement_auto_disables_for_vscode_in_wsl() { - assert!(keyboard_enhancement_disabled_for( - /*disable_env*/ None, /*is_wsl*/ true, /*is_vscode_terminal*/ true - )); - } - - #[test] - fn keyboard_enhancement_auto_disable_requires_wsl_and_vscode() { - assert!(!keyboard_enhancement_disabled_for( - /*disable_env*/ None, /*is_wsl*/ true, /*is_vscode_terminal*/ false - )); - assert!(!keyboard_enhancement_disabled_for( - /*disable_env*/ None, /*is_wsl*/ false, /*is_vscode_terminal*/ true - )); + fn keyboard_enhancement_auto_disables_for_vscode_or_unknown_in_wsl() { + for detection in [ + VscodeDetection::VsCode, + VscodeDetection::Other, + VscodeDetection::Unknown, + ] { + assert_eq!( + keyboard_enhancement_disabled_for( + /*disable_env*/ None, /*is_wsl*/ true, detection + ), + detection != VscodeDetection::Other, + ); + assert!(!keyboard_enhancement_disabled_for( + /*disable_env*/ None, /*is_wsl*/ false, detection + )); + } } #[test] fn keyboard_enhancement_env_flag_overrides_auto_detection() { - assert!(!keyboard_enhancement_disabled_for( - Some("0"), - /*is_wsl*/ true, - /*is_vscode_terminal*/ true - )); - assert!(keyboard_enhancement_disabled_for( - Some("1"), - /*is_wsl*/ false, - /*is_vscode_terminal*/ false - )); + for detection in [ + VscodeDetection::VsCode, + VscodeDetection::Other, + VscodeDetection::Unknown, + ] { + assert!(!keyboard_enhancement_disabled_for( + Some("0"), + /*is_wsl*/ true, + detection + )); + assert!(keyboard_enhancement_disabled_for( + Some("1"), + /*is_wsl*/ false, + detection + )); + } } #[test] fn vscode_terminal_detection_uses_linux_and_windows_term_program() { - assert!(vscode_terminal_detected( - Some("vscode"), - /*windows_term_program*/ None - )); - assert!(vscode_terminal_detected( - /*linux_term_program*/ None, - Some("vscode") - )); - assert!(!vscode_terminal_detected( - /*linux_term_program*/ None, - Some("WindowsTerminal") - )); - assert!(!vscode_terminal_detected( - /*linux_term_program*/ None, /*windows_term_program*/ None - )); + for detection in [ + VscodeDetection::VsCode, + VscodeDetection::Other, + VscodeDetection::Unknown, + ] { + assert_eq!( + vscode_terminal_detected(Some("vscode"), detection), + VscodeDetection::VsCode, + ); + assert_eq!( + vscode_terminal_detected(/*linux_term_program*/ None, detection), + detection, + ); + } } #[test] diff --git a/codex-rs/tui/src/tui/windows_term_program_tests.rs b/codex-rs/tui/src/tui/windows_term_program_tests.rs new file mode 100644 index 0000000000..01679a3acc --- /dev/null +++ b/codex-rs/tui/src/tui/windows_term_program_tests.rs @@ -0,0 +1,128 @@ +//! Regression coverage for bounded Windows terminal detection and child cleanup. + +use super::VscodeDetection; +use super::keyboard_enhancement_disabled_for; +use super::read_windows_vscode_detection_with_timeout; +use pretty_assertions::assert_eq; +use std::process::Command; +use std::process::Stdio; +use std::sync::mpsc; +use std::time::Duration; +use std::time::Instant; + +#[test] +fn reads_term_program_only_from_successful_probe() { + for (script, expected) in [ + ("printf 'TERM_PROGRAM=vscode\r\n'", VscodeDetection::VsCode), + ( + "printf 'TERM_PROGRAM=vscode\r\n'; exit 1", + VscodeDetection::Other, + ), + ( + "printf 'TERM_PROGRAM=WindowsTerminal'", + VscodeDetection::Other, + ), + ("exit 1", VscodeDetection::Other), + ("exit 2", VscodeDetection::Unknown), + ] { + assert_eq!( + read_windows_vscode_detection_with_timeout( + move || { + Command::new("/bin/sh") + .args(["-c", script]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .ok() + }, + Duration::from_secs(/*secs*/ 5), + ), + expected, + ); + } +} + +#[test] +fn failed_launch_is_unknown() { + assert_eq!( + read_windows_vscode_detection_with_timeout(|| None, Duration::from_secs(/*secs*/ 5)), + VscodeDetection::Unknown, + ); +} + +#[test] +fn times_out_and_reaps_running_probe() { + let (sender, receiver) = mpsc::channel(); + let started = Instant::now(); + let result = read_windows_vscode_detection_with_timeout( + move || { + let child = Command::new("sleep").arg("30").spawn().unwrap(); + sender.send(child.id()).unwrap(); + Some(child) + }, + Duration::from_millis(/*millis*/ 100), + ); + assert_eq!(result, VscodeDetection::Unknown); + assert!(keyboard_enhancement_disabled_for( + /*disable_env*/ None, /*is_wsl*/ true, result + )); + assert!(started.elapsed() < Duration::from_secs(/*secs*/ 2)); + assert_child_reaped( + receiver + .recv_timeout(Duration::from_secs(/*secs*/ 5)) + .unwrap(), + ); +} + +#[test] +fn times_out_during_launch_and_reaps_child_when_launch_finishes() { + let (release, blocked) = mpsc::channel(); + let (sender, receiver) = mpsc::channel(); + let started = Instant::now(); + let result = read_windows_vscode_detection_with_timeout( + move || { + // Model a WSL launch that cannot return a Child until interop recovers. + blocked + .recv_timeout(Duration::from_secs(/*secs*/ 5)) + .unwrap(); + let child = Command::new("sleep").arg("30").spawn().unwrap(); + sender.send(child.id()).unwrap(); + Some(child) + }, + Duration::from_millis(/*millis*/ 100), + ); + release.send(()).unwrap(); + assert_eq!(result, VscodeDetection::Unknown); + assert!(keyboard_enhancement_disabled_for( + /*disable_env*/ None, /*is_wsl*/ true, result + )); + assert!(started.elapsed() < Duration::from_secs(/*secs*/ 2)); + assert_child_reaped( + receiver + .recv_timeout(Duration::from_secs(/*secs*/ 5)) + .unwrap(), + ); +} + +fn assert_child_reaped(pid: u32) { + let started = Instant::now(); + loop { + // SAFETY: signal 0 only checks whether this process still exists. + if unsafe { + libc::kill(pid as libc::pid_t, /*sig*/ 0) + } == -1 + { + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::ESRCH) + ); + return; + } + assert!( + started.elapsed() < Duration::from_secs(/*secs*/ 5), + "timed-out terminal probe was not reaped" + ); + std::thread::sleep(Duration::from_millis(/*millis*/ 10)); + } +}