diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 5ced1803f..2cbc80c51 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2636,6 +2636,7 @@ dependencies = [ "libc", "pretty_assertions", "reqwest", + "semver", "serde", "serde_json", "tar", diff --git a/codex-rs/core-plugins/Cargo.toml b/codex-rs/core-plugins/Cargo.toml index 352d6e571..83df070dd 100644 --- a/codex-rs/core-plugins/Cargo.toml +++ b/codex-rs/core-plugins/Cargo.toml @@ -32,6 +32,7 @@ chrono = { workspace = true } dirs = { workspace = true } flate2 = { workspace = true } reqwest = { workspace = true } +semver = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } tar = { workspace = true } diff --git a/codex-rs/core-plugins/src/store.rs b/codex-rs/core-plugins/src/store.rs index fe662a142..19c3b0116 100644 --- a/codex-rs/core-plugins/src/store.rs +++ b/codex-rs/core-plugins/src/store.rs @@ -4,8 +4,10 @@ use codex_plugin::PluginId; use codex_plugin::validate_plugin_segment; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::find_plugin_manifest_path; +use semver::Version; use serde::Deserialize; use serde_json::Value as JsonValue; +use std::cmp::Ordering; use std::fs; use std::io; use std::path::Path; @@ -75,7 +77,7 @@ impl PluginStore { }) .filter(|version| validate_plugin_version_segment(version).is_ok()) .collect::>(); - discovered_versions.sort_unstable(); + discovered_versions.sort_unstable_by(|left, right| compare_plugin_versions(left, right)); if discovered_versions.is_empty() { None } else if discovered_versions @@ -286,6 +288,15 @@ fn replace_plugin_root_atomically( let staged_version_root = staged_root.join(plugin_version); copy_dir_recursive(source, &staged_version_root)?; + let target_version_root = target_root.join(plugin_version); + if target_root.exists() && !target_version_root.exists() { + fs::rename(&staged_version_root, &target_version_root).map_err(|err| { + PluginStoreError::io("failed to activate updated plugin cache version", err) + })?; + remove_old_plugin_versions(target_root, plugin_version)?; + return Ok(()); + } + if target_root.exists() { let backup_dir = tempfile::Builder::new() .prefix("plugin-backup-") @@ -322,6 +333,52 @@ fn replace_plugin_root_atomically( Ok(()) } +fn remove_old_plugin_versions( + target_root: &Path, + plugin_version: &str, +) -> Result<(), PluginStoreError> { + let Ok(entries) = fs::read_dir(target_root) else { + return Ok(()); + }; + + for entry in entries.filter_map(Result::ok) { + let Ok(file_type) = entry.file_type() else { + continue; + }; + if !file_type.is_dir() { + continue; + } + let Ok(version) = entry.file_name().into_string() else { + continue; + }; + if version == plugin_version || validate_plugin_version_segment(&version).is_err() { + continue; + } + + if fs::remove_dir_all(entry.path()).is_err() + && old_plugin_version_would_stay_active(&version, plugin_version) + { + return Err(PluginStoreError::Invalid(format!( + "failed to activate updated plugin cache version `{plugin_version}` while `{version}` remains active" + ))); + } + } + + Ok(()) +} + +fn old_plugin_version_would_stay_active(old_version: &str, new_version: &str) -> bool { + old_version == DEFAULT_PLUGIN_VERSION + || compare_plugin_versions(old_version, new_version).is_gt() +} + +fn compare_plugin_versions(left: &str, right: &str) -> Ordering { + match (Version::parse(left), Version::parse(right)) { + (Ok(left), Ok(right)) => left.cmp(&right), + _ => left.cmp(right), + } +} + fn copy_dir_recursive(source: &Path, target: &Path) -> Result<(), PluginStoreError> { fs::create_dir_all(target) .map_err(|err| PluginStoreError::io("failed to create plugin target directory", err))?; diff --git a/codex-rs/core-plugins/src/store_tests.rs b/codex-rs/core-plugins/src/store_tests.rs index 0ba6b0d2c..200055fe6 100644 --- a/codex-rs/core-plugins/src/store_tests.rs +++ b/codex-rs/core-plugins/src/store_tests.rs @@ -247,7 +247,7 @@ fn active_plugin_version_prefers_default_local_version_when_multiple_versions_ex } #[test] -fn active_plugin_version_returns_last_sorted_version_when_default_is_missing() { +fn active_plugin_version_returns_latest_version_when_default_is_missing() { let tmp = tempdir().unwrap(); write_plugin( &tmp.path().join("plugins/cache/debug"), @@ -268,6 +268,76 @@ fn active_plugin_version_returns_last_sorted_version_when_default_is_missing() { ); } +#[test] +fn active_plugin_version_compares_semver_versions_semantically() { + let tmp = tempdir().unwrap(); + write_plugin( + &tmp.path().join("plugins/cache/debug"), + "sample-plugin/9.0.0", + "sample-plugin", + ); + write_plugin( + &tmp.path().join("plugins/cache/debug"), + "sample-plugin/10.0.0", + "sample-plugin", + ); + let store = PluginStore::new(tmp.path().to_path_buf()); + let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(); + + assert_eq!( + store.active_plugin_version(&plugin_id), + Some("10.0.0".to_string()) + ); +} + +#[test] +fn install_with_new_version_keeps_existing_plugin_root_and_prunes_old_versions() { + let tmp = tempdir().unwrap(); + let store = PluginStore::new(tmp.path().to_path_buf()); + let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap(); + + write_plugin_with_version(tmp.path(), "v1", "sample-plugin", Some("1.0.0")); + store + .install( + AbsolutePathBuf::try_from(tmp.path().join("v1")).unwrap(), + plugin_id.clone(), + ) + .unwrap(); + + write_plugin_with_version(tmp.path(), "v2", "sample-plugin", Some("2.0.0")); + store + .install( + AbsolutePathBuf::try_from(tmp.path().join("v2")).unwrap(), + plugin_id.clone(), + ) + .unwrap(); + + assert_eq!( + store.active_plugin_version(&plugin_id), + Some("2.0.0".to_string()) + ); + assert!( + tmp.path() + .join("plugins/cache/debug/sample-plugin/2.0.0") + .is_dir() + ); + assert!( + !tmp.path() + .join("plugins/cache/debug/sample-plugin/1.0.0") + .exists() + ); +} + +#[test] +fn old_plugin_version_would_stay_active_for_local_or_later_versions() { + assert!(old_plugin_version_would_stay_active( + DEFAULT_PLUGIN_VERSION, + "1.0.0" + )); + assert!(old_plugin_version_would_stay_active("10.0.0", "9.0.0")); + assert!(!old_plugin_version_would_stay_active("1.0.0", "2.0.0")); +} + #[test] fn plugin_root_rejects_path_separators_in_key_segments() { let err = PluginId::parse("../../etc@debug").unwrap_err();