From e3f44ca3b30f85c139f717d271fa6f0e5aa64560 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Sat, 18 Apr 2026 19:04:53 -0700 Subject: [PATCH] Fix plugin cache panic when cwd is unavailable (#18499) ## Summary Fixes #16637. (I hit this bug after 11h of work on a long-running task.) Plugin cache initialization could panic when an already-absolute cache path was normalized through `AbsolutePathBuf::from_absolute_path`, because that path still consulted `current_dir()`. This changes absolute-path normalization so already-absolute paths do not depend on cwd, and makes plugin cache root construction available as a fallible path through `PluginStore::try_new()`. Plugin cache subpaths now use `AbsolutePathBuf::join()` instead of re-absolutizing derived absolute paths. --- codex-rs/core-plugins/src/loader.rs | 12 ++++-- codex-rs/core-plugins/src/store.rs | 30 ++++++------- codex-rs/core-plugins/src/store_tests.rs | 12 ++++++ .../utils/absolute-path/src/absolutize.rs | 4 ++ codex-rs/utils/absolute-path/src/lib.rs | 42 +++++++++++++++++++ 5 files changed, 80 insertions(+), 20 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index 32b4b5d034..f3e5f8d4f1 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -129,7 +129,7 @@ pub fn refresh_curated_plugin_cache( plugin_version: &str, configured_curated_plugin_ids: &[PluginId], ) -> Result { - let store = PluginStore::new(codex_home.to_path_buf()); + let store = PluginStore::try_new(codex_home.to_path_buf()).map_err(|err| err.to_string())?; let curated_marketplace_path = AbsolutePathBuf::try_from( codex_home .join(".tmp/plugins") @@ -234,7 +234,7 @@ fn refresh_non_curated_plugin_cache_with_mode( .map(PluginId::as_key) .collect::>(); - let store = PluginStore::new(codex_home.to_path_buf()); + let store = PluginStore::try_new(codex_home.to_path_buf()).map_err(|err| err.to_string())?; let marketplace_outcome = list_marketplaces(additional_roots) .map_err(|err| format!("failed to discover marketplaces for cache refresh: {err}"))?; let mut plugin_sources = HashMap::::new(); @@ -742,7 +742,13 @@ pub async fn installed_plugin_telemetry_metadata( codex_home: &Path, plugin_id: &PluginId, ) -> PluginTelemetryMetadata { - let store = PluginStore::new(codex_home.to_path_buf()); + let store = match PluginStore::try_new(codex_home.to_path_buf()) { + Ok(store) => store, + Err(err) => { + warn!("failed to resolve plugin cache root: {err}"); + return PluginTelemetryMetadata::from_plugin_id(plugin_id); + } + }; let Some(plugin_root) = store.active_plugin_root(plugin_id) else { return PluginTelemetryMetadata::from_plugin_id(plugin_id); }; diff --git a/codex-rs/core-plugins/src/store.rs b/codex-rs/core-plugins/src/store.rs index 4ada6a3d53..757aec8bc5 100644 --- a/codex-rs/core-plugins/src/store.rs +++ b/codex-rs/core-plugins/src/store.rs @@ -28,10 +28,15 @@ pub struct PluginStore { impl PluginStore { pub fn new(codex_home: PathBuf) -> Self { - Self { - root: AbsolutePathBuf::try_from(codex_home.join(PLUGINS_CACHE_DIR)) - .unwrap_or_else(|err| panic!("plugin cache root should be absolute: {err}")), - } + Self::try_new(codex_home) + .unwrap_or_else(|err| panic!("plugin cache root should be absolute: {err}")) + } + + pub fn try_new(codex_home: PathBuf) -> Result { + let root = AbsolutePathBuf::from_absolute_path_checked(codex_home.join(PLUGINS_CACHE_DIR)) + .map_err(|err| PluginStoreError::io("failed to resolve plugin cache root", err))?; + + Ok(Self { root }) } pub fn root(&self) -> &AbsolutePathBuf { @@ -39,22 +44,13 @@ impl PluginStore { } pub fn plugin_base_root(&self, plugin_id: &PluginId) -> AbsolutePathBuf { - AbsolutePathBuf::try_from( - self.root - .as_path() - .join(&plugin_id.marketplace_name) - .join(&plugin_id.plugin_name), - ) - .unwrap_or_else(|err| panic!("plugin cache path should resolve to an absolute path: {err}")) + self.root + .join(&plugin_id.marketplace_name) + .join(&plugin_id.plugin_name) } pub fn plugin_root(&self, plugin_id: &PluginId, plugin_version: &str) -> AbsolutePathBuf { - AbsolutePathBuf::try_from( - self.plugin_base_root(plugin_id) - .as_path() - .join(plugin_version), - ) - .unwrap_or_else(|err| panic!("plugin cache path should resolve to an absolute path: {err}")) + self.plugin_base_root(plugin_id).join(plugin_version) } pub fn active_plugin_version(&self, plugin_id: &PluginId) -> Option { diff --git a/codex-rs/core-plugins/src/store_tests.rs b/codex-rs/core-plugins/src/store_tests.rs index c1d5dd1ab6..45feff61bd 100644 --- a/codex-rs/core-plugins/src/store_tests.rs +++ b/codex-rs/core-plugins/src/store_tests.rs @@ -33,6 +33,18 @@ fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) { ); } +#[test] +fn try_new_rejects_relative_codex_home() { + let err = PluginStore::try_new(PathBuf::from("relative")) + .expect_err("relative codex home should fail"); + let err = err.to_string().replace('\\', "/"); + + assert_eq!( + err, + "failed to resolve plugin cache root: path is not absolute: relative/plugins/cache" + ); +} + #[test] fn install_copies_plugin_into_default_marketplace() { let tmp = tempdir().unwrap(); diff --git a/codex-rs/utils/absolute-path/src/absolutize.rs b/codex-rs/utils/absolute-path/src/absolutize.rs index 7965c81f7b..4c1842ada7 100644 --- a/codex-rs/utils/absolute-path/src/absolutize.rs +++ b/codex-rs/utils/absolute-path/src/absolutize.rs @@ -12,6 +12,10 @@ use std::path::Path; use std::path::PathBuf; pub(super) fn absolutize(path: &Path) -> std::io::Result { + if path.is_absolute() { + return Ok(normalize_path(path)); + } + Ok(absolutize_from(path, &std::env::current_dir()?)) } diff --git a/codex-rs/utils/absolute-path/src/lib.rs b/codex-rs/utils/absolute-path/src/lib.rs index 200dfd3705..86161d23f0 100644 --- a/codex-rs/utils/absolute-path/src/lib.rs +++ b/codex-rs/utils/absolute-path/src/lib.rs @@ -328,6 +328,8 @@ mod tests { use crate::test_support::test_path_buf; use pretty_assertions::assert_eq; use std::fs; + #[cfg(unix)] + use std::process::Command; use tempfile::tempdir; #[test] @@ -341,6 +343,46 @@ mod tests { assert_eq!(abs_path_buf.as_path(), absolute_path.as_path()); } + #[cfg(unix)] + #[test] + fn from_absolute_path_does_not_read_current_dir_when_path_is_absolute() { + let status = Command::new(std::env::current_exe().expect("current test binary")) + .arg("from_absolute_path_with_removed_current_dir_child") + .arg("--ignored") + .env("CODEX_ABSOLUTE_PATH_REMOVED_CWD_CHILD", "1") + .status() + .expect("run child test"); + + assert!(status.success()); + } + + #[cfg(unix)] + #[test] + #[ignore] + fn from_absolute_path_with_removed_current_dir_child() { + if std::env::var_os("CODEX_ABSOLUTE_PATH_REMOVED_CWD_CHILD").is_none() { + return; + } + + let original_cwd = std::env::current_dir().expect("original cwd"); + let temp_dir = tempdir().expect("temp dir"); + let removed_cwd = temp_dir.path().to_path_buf(); + std::env::set_current_dir(&removed_cwd).expect("enter temp dir"); + std::fs::remove_dir(&removed_cwd).expect("remove current dir"); + std::env::current_dir().expect_err("current dir should be unavailable"); + + let path = AbsolutePathBuf::from_absolute_path(test_path_buf( + "/tmp/codex/../codex-home/plugins/cache", + )) + .expect("absolute path should not require current dir"); + + std::env::set_current_dir(original_cwd).expect("restore cwd"); + assert_eq!( + path.as_path(), + test_path_buf("/tmp/codex-home/plugins/cache") + ); + } + #[test] fn from_absolute_path_checked_rejects_relative_path() { let err = AbsolutePathBuf::from_absolute_path_checked("relative/path")