From f027149f2de4784b7f8e378891a5ecb623b265b0 Mon Sep 17 00:00:00 2001 From: Thiago Lima Date: Sat, 5 Sep 2026 17:56:58 -0300 Subject: [PATCH] Keep stable shell paths in detection and recover stale shell preferences Shell detection canonicalized /opt/homebrew/bin/ to the versioned Homebrew Cellar path, and that path was persisted as the new-session shell preference. A formula upgrade removes the old Cellar directory, so the exact path match against re-detected shells failed and the preference silently fell back to the system default shell. Detection now keeps the discovered (symlink-stable) path for storage and launch, using the canonical path only as the dedupe key. Preferences that no longer match a detected shell recover instead of resetting to the default: an existing path is matched by canonical target (covering preferences persisted by older builds), and a vanished path resolves to the detected known shell of the same type, preferring the install closest to the stale path. Session restore applies the same recovery to stale snapshot paths before falling back to a custom shell. Fixes warpdotdev/warp#15836 --- app/src/terminal/available_shells.rs | 126 ++++++++-- app/src/terminal/available_shells_tests.rs | 272 ++++++++++++++++++++- 2 files changed, 376 insertions(+), 22 deletions(-) diff --git a/app/src/terminal/available_shells.rs b/app/src/terminal/available_shells.rs index 9590bcb52b6..121804b15bc 100644 --- a/app/src/terminal/available_shells.rs +++ b/app/src/terminal/available_shells.rs @@ -1,6 +1,6 @@ use std::borrow::Cow; #[cfg(feature = "local_tty")] -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; #[cfg(feature = "local_tty")] use std::path::Path; use std::path::PathBuf; @@ -574,6 +574,14 @@ impl AvailableShells { shell_type, } = config { + // Snapshots may carry a path removed by a package-manager upgrade; + // recover to the detected shell of the same type instead of a dead path. + if !file_exists_and_is_executable(executable_path) + && let Some(shell) = + self.closest_known_shell(*shell_type, Some(executable_path)) + { + return Some(shell); + } Some(AvailableShell::new_custom_shell( executable_path.file_name()?.to_str()?.to_string(), executable_path.clone(), @@ -599,10 +607,81 @@ impl AvailableShells { .iter() .find(|shell| shell.matches_preference(&preference)) .cloned() + .or_else(|| self.recover_unmatched_executable_preference(&preference)) .unwrap_or_default(), } } + /// Recovers a preference whose persisted executable path does not exactly match any + /// detected shell — e.g. a Homebrew Cellar path persisted by an older build, or one + /// removed by a formula upgrade — rather than silently resetting to the system default. + fn recover_unmatched_executable_preference( + &self, + preference: &NewSessionShell, + ) -> Option { + let NewSessionShell::Executable(path) = preference else { + return None; + }; + let persisted_path = Path::new(path); + if file_exists_and_is_executable(persisted_path) { + // Older builds persisted canonicalized paths while detection now stores stable + // discovered paths; an existing path is honored only if it aliases a detected + // shell, otherwise it's an out-of-catalog choice left to launch-time fallback. + let target = + dunce::canonicalize(persisted_path).unwrap_or_else(|_| persisted_path.to_path_buf()); + return self + .shells + .iter() + .find(|shell| { + matches!(shell.state.as_ref(), Config::KnownLocal(LocalConfig { executable_path, .. }) + if dunce::canonicalize(executable_path).unwrap_or_else(|_| executable_path.clone()) == target) + }) + .cloned(); + } + let shell_type = persisted_path + .file_name() + .and_then(|file_name| file_name.to_str()) + .and_then(ShellType::from_name)?; + self.closest_known_shell(shell_type, Some(persisted_path)) + } + + /// Finds the known shell of the given type sharing the longest path prefix with + /// `reference_path` (a stale Homebrew Cellar path should pick the shell under + /// `/opt/homebrew/bin`), ties broken by catalog order. + fn closest_known_shell( + &self, + shell_type: ShellType, + reference_path: Option<&Path>, + ) -> Option { + let mut best: Option<(&AvailableShell, usize)> = None; + for shell in self.shells.iter().filter_map(|shell| match shell.state.as_ref() { + Config::KnownLocal(LocalConfig { + executable_path, + shell_type: detected_type, + .. + }) if *detected_type == shell_type => Some((shell, executable_path)), + Config::Custom(_) + | Config::SystemDefault + | Config::Wsl { .. } + | Config::MSYS2(_) + | Config::DockerSandbox { .. } => None, + }) { + let shared = reference_path + .map(|reference| { + reference + .components() + .zip(shell.1.components()) + .take_while(|(a, b)| a == b) + .count() + }) + .unwrap_or(0); + if !matches!(best, Some((_, best_shared)) if best_shared >= shared) { + best = Some((shell.0, shared)); + } + } + best.map(|(shell, _)| shell.clone()) + } + /// Sets the user-preferred shell for new sessions. Saves the value back to user settings. pub fn set_user_preferred_shell( &self, @@ -672,17 +751,19 @@ impl AvailableShells { } let mut fallback_shells = fallback_shell_map.remove(command_name).unwrap_or_default(); - for path in Self::resolve_all_executables(command_name, paths_to_search.iter()) { - fallback_shells.remove(&path); + for (canonicalized, discovered) in + Self::resolve_all_executables(command_name, paths_to_search.iter()) + { + fallback_shells.remove(&canonicalized); known_shells.push(AvailableShell::new_local_executable( command_name.to_string(), - path, + discovered, shell_type, )); } // We append shells found in /etc/shells but not the path after the shells found on the path. - for path in fallback_shells.iter() { + for path in fallback_shells.values() { if file_exists_and_is_executable(path) { known_shells.push(AvailableShell::new_local_executable( command_name.to_string(), @@ -807,30 +888,37 @@ impl AvailableShells { paths } - /// Resolves all full paths to executables of the given command name in PATH. + /// Resolves all full paths to executables of the given command name in PATH, + /// returning `(canonical, discovered)` pairs per unique executable. /// /// `paths_to_search` should contain the locations in PATH along with any /// manually added paths that we want to search. + /// + /// The canonical path is only the dedupe key; the discovered path is what callers + /// store and launch, since stable symlinks like Homebrew's `/opt/homebrew/bin/` + /// survive formula upgrades that remove the versioned Cellar directory. fn resolve_all_executables<'a>( command: &str, paths_to_search: impl Iterator, - ) -> Vec { + ) -> Vec<(PathBuf, PathBuf)> { use itertools::Itertools as _; paths_to_search .filter_map(|single_path| { - let joined = single_path.join(command); - let canonicalized = dunce::canonicalize(&joined).unwrap_or(joined); - file_exists_and_is_executable(&canonicalized).then_some(canonicalized) + let discovered = single_path.join(command); + let canonicalized = + dunce::canonicalize(&discovered).unwrap_or_else(|_| discovered.clone()); + file_exists_and_is_executable(&canonicalized) + .then_some((canonicalized, discovered)) }) - .unique() + .unique_by(|(canonicalized, _)| canonicalized.clone()) .collect() } fn load_fallback_shells( path: &Path, shell_types: &[(ShellType, &str)], - ) -> anyhow::Result>> { + ) -> anyhow::Result>> { use std::fs::File; use std::io::{BufRead, BufReader}; @@ -839,7 +927,7 @@ impl AvailableShells { let file = File::open(path)?; for (_, exe) in shell_types.iter() { - shells.insert(exe.to_string(), HashSet::new()); + shells.insert(exe.to_string(), HashMap::new()); } let reader = BufReader::new(file); @@ -850,15 +938,17 @@ impl AvailableShells { // - is it not empty? // - does the "file_name" map to a shell that we support? // - // If all of those are true, then we add it to the set of paths associated with that shell + // If all of those are true, then we add it to the paths associated with that + // shell, keyed by canonical path so aliases dedupe (last spelling wins). The + // stored value keeps the raw entry (see resolve_all_executables for why). if !line.trim_start().starts_with('#') && !line.trim().is_empty() { - let Ok(path) = dunce::canonicalize(line) else { + let Ok(canonical) = dunce::canonicalize(&line) else { continue; }; - if let Some(file_name) = path.file_name().and_then(|name| name.to_str()) - && let Some(set) = shells.get_mut(file_name) + if let Some(file_name) = canonical.file_name().and_then(|name| name.to_str()) + && let Some(paths) = shells.get_mut(file_name) { - set.insert(path); + paths.insert(canonical, PathBuf::from(&line)); } } } diff --git a/app/src/terminal/available_shells_tests.rs b/app/src/terminal/available_shells_tests.rs index bf92bfd904c..fc960679d57 100644 --- a/app/src/terminal/available_shells_tests.rs +++ b/app/src/terminal/available_shells_tests.rs @@ -110,15 +110,15 @@ fn test_dedupe_symlinks_when_discovering_paths() { let fallback_shells = AvailableShells::load_known_shells(&paths_to_search, Some(etc_shells.as_path())); - // We should expect there to be only one shell, with the path and id for that shell being - // the canonical path to the executable. + // We should expect there to be only one shell: canonical paths dedupe the symlink + // aliases, but the stored path is the discovered one from the first search location. assert_eq!( fallback_shells, vec![AvailableShell { - id: Some(format!("local:{}", usr_bin_bash.display())), + id: Some(format!("local:{}", bin_bash.display())), state: Arc::new(Config::KnownLocal(LocalConfig { command: "bash".to_string(), - executable_path: usr_bin_bash, + executable_path: bin_bash, shell_type: ShellType::Bash, })) }] @@ -127,6 +127,270 @@ fn test_dedupe_symlinks_when_discovering_paths() { ); } +#[test] +fn test_keeps_stable_symlink_path_for_homebrew_shells() { + FeatureFlag::ShellSelector.set_enabled(true); + VirtualFS::test( + "test_keeps_stable_symlink_path_for_homebrew_shells", + |dirs, mut sandbox| { + let bin_fish = dirs.tests().join("bin").join("fish"); + let cellar_fish = dirs + .tests() + .join("Cellar") + .join("fish") + .join("1.0") + .join("bin") + .join("fish"); + + sandbox.mkdir("Cellar/fish/1.0/bin"); + sandbox.mkdir("bin"); + sandbox.with_files(vec![Stub::MockExecutable("Cellar/fish/1.0/bin/fish")]); + sandbox.ln("Cellar/fish/1.0/bin/fish", "bin/fish"); + + let shells = AvailableShells::load_known_shells(&[dirs.tests().join("bin")], None); + + // The stored path must be the stable symlink (bin/fish), which survives a Homebrew + // upgrade; the versioned Cellar path is removed when a new version is installed. + assert_eq!( + shells, + vec![AvailableShell { + id: Some(format!("local:{}", bin_fish.display())), + state: Arc::new(Config::KnownLocal(LocalConfig { + command: "fish".to_string(), + executable_path: bin_fish, + shell_type: ShellType::Fish, + })) + }], + "expected the stable bin path, but shells contained {shells:?} (cellar path: {})", + cellar_fish.display() + ); + }, + ); +} + +#[test] +fn test_recovers_executable_preference_via_canonical_alias() { + // A preference persisted by an older build carries the canonical Cellar path, while + // detection now reports the stable bin symlink. Right after the Warp update both + // still exist, and recovery must follow the canonical alias. + VirtualFS::test( + "test_recovers_executable_preference_via_canonical_alias", + |dirs, mut sandbox| { + let bin_fish = dirs.tests().join("bin").join("fish"); + let cellar_fish = dirs + .tests() + .join("Cellar") + .join("fish") + .join("1.0") + .join("bin") + .join("fish"); + + sandbox.mkdir("Cellar/fish/1.0/bin"); + sandbox.mkdir("bin"); + sandbox.with_files(vec![Stub::MockExecutable("Cellar/fish/1.0/bin/fish")]); + sandbox.ln("Cellar/fish/1.0/bin/fish", "bin/fish"); + + let shells = make_available_shells(vec![AvailableShell::new_local_executable( + "fish".to_string(), + bin_fish.clone(), + ShellType::Fish, + )]); + + let recovered = shells + .recover_unmatched_executable_preference(&NewSessionShell::Executable( + cellar_fish.display().to_string(), + )) + .expect("should recover to the detected fish"); + + if let Config::KnownLocal(config) = recovered.state.as_ref() { + assert_eq!(config.executable_path, bin_fish); + } else { + panic!("expected a KnownLocal shell, got {recovered:?}"); + } + }, + ); +} + +#[test] +fn test_recovers_stale_executable_preference_by_shell_type() { + // After the persisted Cellar path is removed by a formula upgrade, recovery + // falls back to the detected shell of the same type. + VirtualFS::test( + "test_recovers_stale_executable_preference_by_shell_type", + |dirs, mut sandbox| { + let bin_fish = dirs.tests().join("bin").join("fish"); + let removed_cellar_fish = dirs + .tests() + .join("Cellar") + .join("fish") + .join("1.0") + .join("bin") + .join("fish"); + + sandbox.mkdir("bin"); + sandbox.with_files(vec![Stub::MockExecutable("bin/fish")]); + + let shells = make_available_shells(vec![AvailableShell::new_local_executable( + "fish".to_string(), + bin_fish.clone(), + ShellType::Fish, + )]); + + let recovered = shells + .recover_unmatched_executable_preference(&NewSessionShell::Executable( + removed_cellar_fish.display().to_string(), + )) + .expect("should recover to the detected fish"); + + if let Config::KnownLocal(config) = recovered.state.as_ref() { + assert_eq!(config.executable_path, bin_fish); + } else { + panic!("expected a KnownLocal shell, got {recovered:?}"); + } + }, + ); +} + +#[test] +fn test_recovery_prefers_the_closest_install_prefix() { + // With several installs of the same shell on the machine, a stale preference + // recovers to the detected shell closest to where the stale one lived. + VirtualFS::test( + "test_recovery_prefers_the_closest_install_prefix", + |dirs, _sandbox| { + let macports_fish = dirs.tests().join("opt/local/bin").join("fish"); + let homebrew_fish = dirs.tests().join("opt/homebrew/bin").join("fish"); + let removed_cellar_fish = dirs + .tests() + .join("opt/homebrew/Cellar") + .join("fish") + .join("1.0") + .join("bin") + .join("fish"); + + let shells = make_available_shells(vec![ + AvailableShell::new_local_executable( + "fish".to_string(), + macports_fish, + ShellType::Fish, + ), + AvailableShell::new_local_executable( + "fish".to_string(), + homebrew_fish.clone(), + ShellType::Fish, + ), + ]); + + let recovered = shells + .recover_unmatched_executable_preference(&NewSessionShell::Executable( + removed_cellar_fish.display().to_string(), + )) + .expect("should recover to a detected fish"); + + if let Config::KnownLocal(config) = recovered.state.as_ref() { + assert_eq!(config.executable_path, homebrew_fish); + } else { + panic!("expected a KnownLocal shell, got {recovered:?}"); + } + }, + ); +} + +#[test] +fn test_does_not_recover_existing_unmatched_or_non_shell_preference() { + VirtualFS::test( + "test_does_not_recover_existing_unmatched_or_non_shell_preference", + |dirs, mut sandbox| { + sandbox.mkdir("usr/bin"); + sandbox.with_files(vec![Stub::MockExecutable("usr/bin/zsh")]); + + // The detected zsh lives at a different path with no file behind it, + // so its canonical form cannot alias the existing preference path. + let shells = make_available_shells(vec![AvailableShell::new_local_executable( + "zsh".to_string(), + dirs.tests().join("usr/local/bin").join("zsh"), + ShellType::Zsh, + )]); + + // An existing executable that matches no detected binary is a deliberate + // out-of-catalog choice and is left to the launch-time fallback. + assert!( + shells + .recover_unmatched_executable_preference(&NewSessionShell::Executable( + dirs.tests().join("usr/bin").join("zsh").display().to_string(), + )) + .is_none() + ); + + // A stale path whose file name is not a supported shell does not recover either. + assert!( + shells + .recover_unmatched_executable_preference(&NewSessionShell::Executable( + dirs.tests().join("removed/bin").join("nu").display().to_string(), + )) + .is_none() + ); + }, + ); +} + +#[test] +fn test_get_from_shell_launch_data_recovers_stale_snapshot_path() { + VirtualFS::test( + "test_get_from_shell_launch_data_recovers_stale_snapshot_path", + |dirs, mut sandbox| { + let bin_fish = dirs.tests().join("bin").join("fish"); + + sandbox.mkdir("bin"); + sandbox.with_files(vec![Stub::MockExecutable("bin/fish")]); + + let shells = make_available_shells(vec![AvailableShell::new_local_executable( + "fish".to_string(), + bin_fish.clone(), + ShellType::Fish, + )]); + + // A snapshot carrying a Cellar path removed by a formula upgrade restores + // to the detected shell instead of a dead custom path. + let stale = ShellLaunchData::Executable { + executable_path: dirs + .tests() + .join("Cellar") + .join("fish") + .join("1.0") + .join("bin") + .join("fish"), + shell_type: ShellType::Fish, + }; + let recovered = shells + .get_from_shell_launch_data(&stale) + .expect("should recover from the stale snapshot path"); + if let Config::KnownLocal(config) = recovered.state.as_ref() { + assert_eq!(config.executable_path, bin_fish); + } else { + panic!("expected a KnownLocal shell, got {recovered:?}"); + } + + // An existing out-of-catalog path still restores as a custom shell. + sandbox.mkdir("usr/bin"); + sandbox.with_files(vec![Stub::MockExecutable("usr/bin/zsh")]); + let custom_path = dirs.tests().join("usr/bin").join("zsh"); + let existing = ShellLaunchData::Executable { + executable_path: custom_path.clone(), + shell_type: ShellType::Zsh, + }; + assert_eq!( + shells.get_from_shell_launch_data(&existing), + Some(AvailableShell::new_custom_shell( + "zsh".to_string(), + custom_path, + ShellType::Zsh, + )) + ); + }, + ); +} + #[test] fn test_find_by_command_name_matches_known_shell() { let zsh_path = PathBuf::from("/bin/zsh");