diff --git a/crates/tui/src/child_env.rs b/crates/tui/src/child_env.rs index 70e4a2bc..dac02912 100644 --- a/crates/tui/src/child_env.rs +++ b/crates/tui/src/child_env.rs @@ -35,6 +35,8 @@ where value.as_ref().to_os_string(), ); } + #[cfg(windows)] + fill_windows_common_program_files(&mut env); env } @@ -184,6 +186,9 @@ fn is_allowed_parent_env_key(key: &OsStr) -> bool { | "PROGRAMFILES" | "PROGRAMFILES(X86)" | "PROGRAMW6432" + | "COMMONPROGRAMFILES" + | "COMMONPROGRAMFILES(X86)" + | "COMMONPROGRAMW6432" | "PROCESSOR_ARCHITECTURE" | "NUGET_PACKAGES" | "NUGET_HTTP_CACHE_PATH" @@ -266,6 +271,26 @@ fn upsert_env(env: &mut Vec<(OsString, OsString)>, key: OsString, value: OsStrin env.push((key, value)); } +#[cfg(any(windows, test))] +fn fill_windows_common_program_files(env: &mut Vec<(OsString, OsString)>) { + for (key, default) in [ + ("CommonProgramFiles", r"C:\Program Files\Common Files"), + ( + "CommonProgramFiles(x86)", + r"C:\Program Files (x86)\Common Files", + ), + ("CommonProgramW6432", r"C:\Program Files\Common Files"), + ] { + let existing = env + .iter() + .find(|(existing, _)| normalize_key(existing) == normalize_key(OsStr::new(key))) + .map(|(_, value)| value.to_string_lossy().trim().is_empty()); + if existing.unwrap_or(true) { + upsert_env(env, OsString::from(key), OsString::from(default)); + } + } +} + fn normalize_key(key: &OsStr) -> String { key.to_string_lossy().to_ascii_uppercase() } @@ -387,6 +412,9 @@ mod tests { "PROGRAMFILES", "PROGRAMFILES(X86)", "PROGRAMW6432", + "COMMONPROGRAMFILES", + "COMMONPROGRAMFILES(X86)", + "COMMONPROGRAMW6432", "PROCESSOR_ARCHITECTURE", "NUGET_PACKAGES", "DOTNET_ROOT", @@ -408,6 +436,41 @@ mod tests { ); } + #[test] + fn windows_common_program_files_defaults_replace_empty_values() { + let mut env = vec![ + (OsString::from("CommonProgramFiles"), OsString::new()), + ( + OsString::from("CommonProgramFiles(x86)"), + OsString::from(" "), + ), + ( + OsString::from("CommonProgramW6432"), + OsString::from(r"D:\Common Files"), + ), + ]; + + fill_windows_common_program_files(&mut env); + + let get = |name: &str| { + env.iter() + .find(|(key, _)| normalize_key(key) == normalize_key(OsStr::new(name))) + .map(|(_, value)| value.to_string_lossy().into_owned()) + }; + assert_eq!( + get("CommonProgramFiles").as_deref(), + Some(r"C:\Program Files\Common Files") + ); + assert_eq!( + get("CommonProgramFiles(x86)").as_deref(), + Some(r"C:\Program Files (x86)\Common Files") + ); + assert_eq!( + get("CommonProgramW6432").as_deref(), + Some(r"D:\Common Files") + ); + } + #[test] fn mcp_env_allowlist_includes_python_bootstrap_keys() { for key in [ diff --git a/crates/tui/src/commands/groups/config/config.rs b/crates/tui/src/commands/groups/config/config.rs index 972e1a09..979b9738 100644 --- a/crates/tui/src/commands/groups/config/config.rs +++ b/crates/tui/src/commands/groups/config/config.rs @@ -658,6 +658,7 @@ pub fn set_config_value(app: &mut App, key: &str, value: &str, persist: bool) -> if let Err(e) = settings.set(&key, value) { return CommandResult::error(format!("{e}")); } + settings.apply_env_overrides(); let mut action = None; match key.as_str() { @@ -835,6 +836,8 @@ pub fn set_config_value(app: &mut App, key: &str, value: &str, persist: bool) -> }, ), "composer_vim_mode" | "vim_mode" | "vim" => settings.composer_vim_mode.clone(), + "low_motion" | "motion" => settings.low_motion.to_string(), + "fancy_animations" | "fancy" | "animations" => settings.fancy_animations.to_string(), _ => value.to_string(), }; @@ -1394,6 +1397,44 @@ mod tests { ); } + #[test] + fn config_fancy_animations_obeys_ghostty_override() { + let temp_root = env::temp_dir().join(format!( + "codewhale-tui-ghostty-fancy-config-test-{}", + std::process::id() + )); + fs::create_dir_all(&temp_root).unwrap(); + let _guard = EnvGuard::new(&temp_root); + let prev_term_program = env::var_os("TERM_PROGRAM"); + // Safety: test-only environment mutation guarded by EnvGuard's lock. + unsafe { + env::set_var("TERM_PROGRAM", "Ghostty"); + } + + let mut app = create_test_app(); + assert!(!app.fancy_animations); + + let result = set_config_value(&mut app, "fancy_animations", "true", false); + + assert!(!result.is_error); + assert!( + !app.fancy_animations, + "Ghostty compatibility override must keep the water strip disabled" + ); + assert_eq!( + result.message.as_deref(), + Some("fancy_animations = false (session only, add --save to persist)") + ); + + // Safety: cleanup under EnvGuard's lock. + unsafe { + match prev_term_program { + Some(v) => env::set_var("TERM_PROGRAM", v), + None => env::remove_var("TERM_PROGRAM"), + } + } + } + #[test] fn config_model_accepts_future_deepseek_model_id() { let mut app = create_test_app(); diff --git a/crates/tui/src/settings.rs b/crates/tui/src/settings.rs index 7eeb5313..d00521e5 100644 --- a/crates/tui/src/settings.rs +++ b/crates/tui/src/settings.rs @@ -456,18 +456,23 @@ impl Settings { self.low_motion = true; self.fancy_animations = false; } - // VS Code (TERM_PROGRAM=vscode, #1356), Ghostty (TERM_PROGRAM=ghostty, - // #1445), and a few VTE terminals (#1470) produce visible flicker at - // 120 FPS. Drop to the 30 FPS low-motion cap for them automatically. + // VS Code (TERM_PROGRAM=vscode, #1356), Ghostty (#1445), and a few + // VTE terminals (#1470) produce visible flicker at 120 FPS. Drop to + // the 30 FPS low-motion cap for them automatically. Ghostty may report + // either TERM_PROGRAM=Ghostty/ghostty or TERM=xterm-ghostty. // Like NO_ANIMATIONS above, this unconditionally overrides any // disk-loaded value — consistent precedence: env signals always win. + let term_program = std::env::var("TERM_PROGRAM") + .unwrap_or_default() + .to_ascii_lowercase(); + let term = std::env::var("TERM") + .unwrap_or_default() + .to_ascii_lowercase(); + let term_forces_low_motion = + matches!(term_program.as_str(), "vscode" | "ghostty") || term.contains("ghostty"); let vte_env_forces_low_motion = std::env::var_os("TILIX_ID").is_some_and(|v| !v.is_empty()) || std::env::var_os("TERMINATOR_UUID").is_some_and(|v| !v.is_empty()); - if matches!( - std::env::var("TERM_PROGRAM").as_deref(), - Ok("vscode") | Ok("ghostty") - ) || vte_env_forces_low_motion - { + if term_forces_low_motion || vte_env_forces_low_motion { self.low_motion = true; self.fancy_animations = false; } @@ -1673,6 +1678,7 @@ mod tests { let prev_tmux = std::env::var_os("TMUX"); let prev_sty = std::env::var_os("STY"); let prev_term_program = std::env::var_os("TERM_PROGRAM"); + let prev_term = std::env::var_os("TERM"); let prev_ssh_client = std::env::var_os("SSH_CLIENT"); let prev_ssh_tty = std::env::var_os("SSH_TTY"); let prev_tilix_id = std::env::var_os("TILIX_ID"); @@ -1690,6 +1696,7 @@ mod tests { std::env::remove_var("TMUX"); std::env::remove_var("STY"); std::env::remove_var("TERM_PROGRAM"); + std::env::remove_var("TERM"); std::env::remove_var("SSH_CLIENT"); std::env::remove_var("SSH_TTY"); std::env::remove_var("TILIX_ID"); @@ -1736,6 +1743,10 @@ mod tests { Some(v) => std::env::set_var("TERM_PROGRAM", v), None => std::env::remove_var("TERM_PROGRAM"), } + match prev_term { + Some(v) => std::env::set_var("TERM", v), + None => std::env::remove_var("TERM"), + } match prev_ssh_client { Some(v) => std::env::set_var("SSH_CLIENT", v), None => std::env::remove_var("SSH_CLIENT"), @@ -1799,18 +1810,18 @@ mod tests { let prev = std::env::var_os("TERM_PROGRAM"); // SAFETY: serialised by the guard. unsafe { - std::env::set_var("TERM_PROGRAM", "ghostty"); + std::env::set_var("TERM_PROGRAM", "Ghostty"); } let mut settings = Settings::default(); assert!(!settings.low_motion, "default is animated"); settings.apply_env_overrides(); assert!( settings.low_motion, - "TERM_PROGRAM=ghostty must enable low_motion to prevent flickering (#1445)" + "TERM_PROGRAM=Ghostty must enable low_motion to prevent flickering (#1445)" ); assert!( !settings.fancy_animations, - "TERM_PROGRAM=ghostty must disable fancy_animations" + "TERM_PROGRAM=Ghostty must disable fancy_animations" ); // SAFETY: cleanup under the guard. unsafe { @@ -1821,10 +1832,44 @@ mod tests { } } + #[test] + fn ghostty_term_fallback_forces_low_motion_on() { + let _g = term_program_test_guard(); + let prev_program = std::env::var_os("TERM_PROGRAM"); + let prev_term = std::env::var_os("TERM"); + // SAFETY: serialised by the guard. + unsafe { + std::env::remove_var("TERM_PROGRAM"); + std::env::set_var("TERM", "xterm-ghostty"); + } + let mut settings = Settings::default(); + settings.apply_env_overrides(); + assert!( + settings.low_motion, + "TERM=xterm-ghostty must enable low_motion when TERM_PROGRAM is absent" + ); + assert!( + !settings.fancy_animations, + "TERM=xterm-ghostty must disable fancy_animations" + ); + // SAFETY: cleanup under the guard. + unsafe { + match prev_program { + Some(v) => std::env::set_var("TERM_PROGRAM", v), + None => std::env::remove_var("TERM_PROGRAM"), + } + match prev_term { + Some(v) => std::env::set_var("TERM", v), + None => std::env::remove_var("TERM"), + } + } + } + #[test] fn non_vscode_term_program_does_not_force_low_motion() { let _g = term_program_test_guard(); let prev = std::env::var_os("TERM_PROGRAM"); + let prev_term = std::env::var_os("TERM"); let prev_ssh_client = std::env::var_os("SSH_CLIENT"); let prev_ssh_tty = std::env::var_os("SSH_TTY"); let prev_tilix_id = std::env::var_os("TILIX_ID"); @@ -1838,6 +1883,7 @@ mod tests { unsafe { std::env::remove_var("SSH_CLIENT"); std::env::remove_var("SSH_TTY"); + std::env::remove_var("TERM"); std::env::remove_var("TILIX_ID"); std::env::remove_var("TERMINATOR_UUID"); std::env::remove_var("TMUX"); @@ -1861,6 +1907,10 @@ mod tests { Some(v) => std::env::set_var("TERM_PROGRAM", v), None => std::env::remove_var("TERM_PROGRAM"), } + match prev_term { + Some(v) => std::env::set_var("TERM", v), + None => std::env::remove_var("TERM"), + } if let Some(v) = prev_ssh_client { std::env::set_var("SSH_CLIENT", v); } diff --git a/crates/tui/src/tools/file.rs b/crates/tui/src/tools/file.rs index 1b0b4dfb..62ff3df1 100644 --- a/crates/tui/src/tools/file.rs +++ b/crates/tui/src/tools/file.rs @@ -10,6 +10,7 @@ use super::spec::{ }; use async_trait::async_trait; use serde_json::{Value, json}; +use std::fmt::Display; use std::fs; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; @@ -356,7 +357,7 @@ fn read_pdf_via_pdf_extract( // pdf-extract returns pages in document order; `start`/`end` are // 1-indexed inclusive (validated above), so we convert to a // 0-indexed half-open slice with bounds clamping. - let pages = pdf_extract::extract_text_by_pages(path).map_err(|e| { + let pages = guard_pdf_extract(|| pdf_extract::extract_text_by_pages(path)).map_err(|e| { ToolError::execution_failed(format!( "pdf-extract failed on {}: {e} (set `prefer_external_pdftotext = true` in settings.toml to retry via pdftotext)", path.display() @@ -379,7 +380,7 @@ fn read_pdf_via_pdf_extract( // extract_text uses an internal codepath that can hang on certain PDF // cross-reference tables or font encodings (#2641). The per-page path // avoids that hang and produces identical output when joined. - pdf_extract::extract_text_by_pages(path) + guard_pdf_extract(|| pdf_extract::extract_text_by_pages(path)) .map(|pages| pages.join("\n")) .map_err(|e| { ToolError::execution_failed(format!( @@ -391,6 +392,31 @@ fn read_pdf_via_pdf_extract( Ok(ToolResult::success(clean_pdf_text(&text))) } +fn guard_pdf_extract(extract: F) -> Result +where + E: Display, + F: FnOnce() -> Result, +{ + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(extract)) { + Ok(Ok(value)) => Ok(value), + Ok(Err(err)) => Err(err.to_string()), + Err(payload) => Err(format!( + "extractor panicked: {}", + panic_payload_message(payload.as_ref()) + )), + } +} + +fn panic_payload_message(payload: &(dyn std::any::Any + Send)) -> String { + if let Some(message) = payload.downcast_ref::<&str>() { + (*message).to_string() + } else if let Some(message) = payload.downcast_ref::() { + message.clone() + } else { + "unknown panic".to_string() + } +} + fn read_pdf_via_pdftotext( path: &Path, page_range: Option<(u32, u32)>, @@ -1380,6 +1406,17 @@ mod tests { assert!(single.content.contains("Recursive Language Models")); } + #[test] + fn pdf_extract_panic_is_returned_as_tool_error_text() { + let err = guard_pdf_extract(|| -> Result { + panic!("assertion failed: name == \"Identity-H\""); + }) + .expect_err("panic should become an error"); + + assert!(err.contains("extractor panicked")); + assert!(err.contains("Identity-H")); + } + #[tokio::test] async fn read_file_pdf_path_uses_pdf_extract_by_default() { if !sample_pdf_present() { diff --git a/crates/tui/src/tools/web_run.rs b/crates/tui/src/tools/web_run.rs index 73c17612..77ff58e7 100644 --- a/crates/tui/src/tools/web_run.rs +++ b/crates/tui/src/tools/web_run.rs @@ -14,6 +14,7 @@ use regex::Regex; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; use std::collections::{HashMap, VecDeque}; +use std::fmt::Display; use std::hash::{Hash, Hasher}; use std::sync::{Arc, OnceLock}; use std::time::{Duration, Instant}; @@ -1204,10 +1205,35 @@ fn parse_pdf_page( } fn pdf_extract_text(bytes: &[u8]) -> Result { - pdf_extract::extract_text_from_mem(bytes) + guard_pdf_extract(|| pdf_extract::extract_text_from_mem(bytes)) .map_err(|e| ToolError::execution_failed(format!("PDF extract failed: {e}"))) } +fn guard_pdf_extract(extract: F) -> Result +where + E: Display, + F: FnOnce() -> Result, +{ + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(extract)) { + Ok(Ok(value)) => Ok(value), + Ok(Err(err)) => Err(err.to_string()), + Err(payload) => Err(format!( + "extractor panicked: {}", + panic_payload_message(payload.as_ref()) + )), + } +} + +fn panic_payload_message(payload: &(dyn std::any::Any + Send)) -> String { + if let Some(message) = payload.downcast_ref::<&str>() { + (*message).to_string() + } else if let Some(message) = payload.downcast_ref::() { + message.clone() + } else { + "unknown panic".to_string() + } +} + fn split_pdf_pages(text: &str) -> Vec> { let raw_pages: Vec<&str> = text.split('\x0C').collect(); raw_pages @@ -1794,6 +1820,17 @@ mod tests { assert_eq!(percent_decode("foo+bar%20baz"), "foo+bar baz"); } + #[test] + fn pdf_extract_panic_is_returned_as_tool_error_text() { + let err = guard_pdf_extract(|| -> Result { + panic!("assertion failed: name == \"Identity-H\""); + }) + .expect_err("panic should become an error"); + + assert!(err.contains("extractor panicked")); + assert!(err.contains("Identity-H")); + } + #[test] fn scoped_ref_prefix_is_session_specific() { let _lock = lock_web_run_test_state();