diff --git a/src/commands/cmd_maintenance.rs b/src/commands/cmd_maintenance.rs index 41ec06e1..6e264354 100644 --- a/src/commands/cmd_maintenance.rs +++ b/src/commands/cmd_maintenance.rs @@ -104,22 +104,21 @@ fn platform_base() -> String { } // The on-device model store, matching harness/local_models.rs: {base}/Models -// or {base}/RunAnywhere/Models. Every base it can derive is checked -- the -// global --home override, the env override, the kit's answer, and the -// platform default -- and the first store that actually exists wins, so a -// missing bootstrap cannot hide it. +// or {base}/RunAnywhere/Models. An explicit --home takes precedence over +// RUNANYWHERE_HOME, and either override confines the search to that base, +// even when its model store is absent. With neither override, check the +// kit's answer and the platform default so a missing bootstrap cannot hide it. fn models_directory(home_override: &str) -> String { let mut bases: Vec = Vec::new(); - if let Some(env) = getenv("RUNANYWHERE_HOME") { - bases.push(env); - } let home = cli_paths::resolve_home(home_override); if !home.is_empty() { bases.push(home); } - let fallback = platform_base(); - if !fallback.is_empty() { - bases.push(fallback); + if home_override.is_empty() && getenv("RUNANYWHERE_HOME").is_none() { + let fallback = platform_base(); + if !fallback.is_empty() { + bases.push(fallback); + } } for base in &bases { @@ -552,6 +551,41 @@ mod tests { use super::*; use crate::util::env_lock::lock as env_lock; + struct RestoreEnv(Vec<(&'static str, Option)>); + + impl Drop for RestoreEnv { + fn drop(&mut self) { + for (name, value) in &self.0 { + // SAFETY: callers hold env_lock until with_storage_env returns. + unsafe { + match value { + Some(value) => std::env::set_var(name, value), + None => std::env::remove_var(name), + } + } + } + } + } + + // Callers hold env_lock for the whole test, including tempfile creation. + fn with_storage_env(vars: &[(&'static str, Option<&Path>)], body: impl FnOnce() -> T) -> T { + let _restore = RestoreEnv( + vars.iter() + .map(|(name, _)| (*name, std::env::var_os(name))) + .collect(), + ); + for (name, value) in vars { + // SAFETY: env_lock is held until after the previous values are restored. + unsafe { + match value { + Some(value) => std::env::set_var(name, value), + None => std::env::remove_var(name), + } + } + } + body() + } + // Dir_size (uninstall's pre-delete size preview) must follow a // symlink to a regular file, matching C++'s is_regular_file(ec)/ // file_size(ec) (which call status(), following symlinks). @@ -626,27 +660,104 @@ mod tests { // always resolve against HOME/RUNANYWHERE_HOME, so it could remove a // different model store than the one `wally models` was just pointed at. #[test] - #[cfg(unix)] fn models_directory_prefers_the_home_override_over_home() { let _lock = env_lock(); let home_a = tempfile::tempdir().expect("tempdir A"); let home_b = tempfile::tempdir().expect("tempdir B"); std::fs::create_dir_all(home_b.path().join("Models")).expect("mkdir B/Models"); - // SAFETY: env_lock() is held for this whole test body. - unsafe { - std::env::set_var("HOME", home_a.path()); - std::env::remove_var("RUNANYWHERE_HOME"); + + with_storage_env( + &[("HOME", Some(home_a.path())), ("RUNANYWHERE_HOME", None)], + || { + assert_eq!( + PathBuf::from(models_directory(&home_b.path().to_string_lossy())), + home_b.path().join("Models") + ); + }, + ); + } + + #[test] + fn models_directory_prefers_home_override_over_runanywhere_home() { + let _lock = env_lock(); + for layout in ["Models", "RunAnywhere/Models"] { + let explicit = tempfile::tempdir().expect("explicit home"); + let from_env = tempfile::tempdir().expect("environment home"); + let expected = explicit.path().join(layout); + std::fs::create_dir_all(&expected).expect("explicit model store"); + std::fs::create_dir_all(from_env.path().join("Models")) + .expect("environment model store"); + + with_storage_env(&[("RUNANYWHERE_HOME", Some(from_env.path()))], || { + assert_eq!( + PathBuf::from(models_directory(&explicit.path().to_string_lossy())), + expected + ); + }); } + } - let result = models_directory(&home_b.path().to_string_lossy()); + #[test] + fn models_directory_does_not_fall_back_when_home_override_has_no_models() { + let _lock = env_lock(); + let explicit = tempfile::tempdir().expect("explicit home"); + let from_env = tempfile::tempdir().expect("environment home"); + let default_home = tempfile::tempdir().expect("default home"); + std::fs::create_dir_all(from_env.path().join("Models")).expect("environment model store"); + + for env_home in [Some(from_env.path()), None] { + with_storage_env( + &[ + ("HOME", Some(default_home.path())), + ("XDG_DATA_HOME", Some(default_home.path())), + ("RUNANYWHERE_HOME", env_home), + ], + || { + std::fs::create_dir_all(Path::new(&platform_base()).join("Models")) + .expect("default model store"); + assert_eq!( + PathBuf::from(models_directory(&explicit.path().to_string_lossy())), + explicit.path().join("Models") + ); + }, + ); + } + } - // SAFETY: still holding env_lock(). - unsafe { std::env::remove_var("HOME") }; + #[test] + fn models_directory_uses_runanywhere_home_without_a_home_override() { + let _lock = env_lock(); + for layout in ["Models", "RunAnywhere/Models"] { + let from_env = tempfile::tempdir().expect("environment home"); + let expected = from_env.path().join(layout); + std::fs::create_dir_all(&expected).expect("environment model store"); + + with_storage_env(&[("RUNANYWHERE_HOME", Some(from_env.path()))], || { + assert_eq!(PathBuf::from(models_directory("")), expected); + }); + } + } - assert!( - result.starts_with(home_b.path().to_str().expect("utf8 path")), - "models_directory({home_b:?}) must resolve against the override, not HOME \ - ({home_a:?}): got {result}" + #[test] + fn models_directory_does_not_fall_back_when_runanywhere_home_has_no_models() { + let _lock = env_lock(); + let from_env = tempfile::tempdir().expect("environment home"); + let default_home = tempfile::tempdir().expect("default home"); + + with_storage_env( + &[ + ("HOME", Some(default_home.path())), + ("XDG_DATA_HOME", Some(default_home.path())), + ("RUNANYWHERE_HOME", Some(from_env.path())), + ], + || { + std::fs::create_dir_all(Path::new(&platform_base()).join("Models")) + .expect("default model store"); + assert_eq!( + PathBuf::from(models_directory("")), + from_env.path().join("Models") + ); + }, ); }