mirror of
https://github.com/openai/codex.git
synced 2026-10-20 00:18:49 +00:00
Fix marketplace path matching for Windows junctions (#52696)
## Why Equivalent local source paths can fail to match an already-added marketplace, and redirected managed roots can lose their managed classification on Windows. ## What changed - Compare local marketplace sources after filesystem normalization. - Reuse the resolved request path when locating an existing local marketplace so retargeting a configured junction does not switch the returned root. - Accept both the configured managed root and its canonical target on Windows while continuing to reject unowned aliases to that target. ## Testing Add Windows regression tests for canonical and legacy path spellings, retargeted local-source junctions, and redirected managed roots with unowned aliases. GitOrigin-RevId: f1ae46e8e7384075ed1baa6b33b2821f795e17f8
This commit is contained in:
@@ -5,6 +5,7 @@ use crate::marketplace::validate_marketplace_root;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use codex_config::MarketplaceConfigUpdate;
|
||||
use codex_config::record_user_marketplace;
|
||||
use codex_utils_path::paths_match_after_normalization;
|
||||
use std::fs;
|
||||
use std::io::ErrorKind;
|
||||
use std::path::Path;
|
||||
@@ -77,10 +78,10 @@ pub(super) fn installed_marketplace_root_for_source(
|
||||
if !install_metadata.matches_config(marketplace) {
|
||||
continue;
|
||||
}
|
||||
let Some(root) =
|
||||
resolve_configured_marketplace_root(marketplace_name, marketplace, install_root)
|
||||
else {
|
||||
continue;
|
||||
let root = match &install_metadata.source {
|
||||
// Keep using the resolved request path if the configured alias is retargeted.
|
||||
InstalledMarketplaceSource::Local { path } => PathBuf::from(path),
|
||||
InstalledMarketplaceSource::Git { .. } => install_root.join(marketplace_name),
|
||||
};
|
||||
if validate_marketplace_root(&root).is_ok() {
|
||||
return Ok(Some(root));
|
||||
@@ -178,8 +179,15 @@ impl MarketplaceInstallMetadata {
|
||||
fn matches_config(&self, marketplace: &toml::Value) -> bool {
|
||||
marketplace.get("source_type").and_then(toml::Value::as_str)
|
||||
== Some(self.config_source_type())
|
||||
&& marketplace.get("source").and_then(toml::Value::as_str)
|
||||
== Some(self.config_source().as_str())
|
||||
&& marketplace
|
||||
.get("source")
|
||||
.and_then(toml::Value::as_str)
|
||||
.is_some_and(|source| match &self.source {
|
||||
InstalledMarketplaceSource::Local { path } => {
|
||||
paths_match_after_normalization(source, path)
|
||||
}
|
||||
InstalledMarketplaceSource::Git { url, .. } => source == url,
|
||||
})
|
||||
&& marketplace.get("ref").and_then(toml::Value::as_str) == self.ref_name()
|
||||
&& config_sparse_paths(marketplace) == self.sparse_paths()
|
||||
}
|
||||
@@ -260,4 +268,81 @@ mod tests {
|
||||
|
||||
assert_eq!(root, Some(source_root));
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn local_marketplace_matches_canonical_and_legacy_windows_sources() {
|
||||
let source = TempDir::new().unwrap();
|
||||
let other = TempDir::new().unwrap();
|
||||
let metadata = MarketplaceInstallMetadata::from_source(
|
||||
&MarketplaceSource::Local {
|
||||
path: source.path().canonicalize().unwrap(),
|
||||
},
|
||||
&[],
|
||||
);
|
||||
let canonical = source.path().canonicalize().unwrap();
|
||||
let legacy =
|
||||
codex_utils_absolute_path::normalize_windows_device_path(&canonical.to_string_lossy())
|
||||
.unwrap();
|
||||
for spelling in [
|
||||
canonical.to_string_lossy().into_owned(),
|
||||
legacy.replace('\\', "/"),
|
||||
] {
|
||||
let config: toml::Value =
|
||||
toml::from_str(&format!("source_type = \"local\"\nsource = {spelling:?}\n"))
|
||||
.unwrap();
|
||||
assert!(metadata.matches_config(&config));
|
||||
}
|
||||
let different = other.path().to_string_lossy();
|
||||
let config: toml::Value = toml::from_str(&format!(
|
||||
"source_type = \"local\"\nsource = {different:?}\n"
|
||||
))
|
||||
.unwrap();
|
||||
assert!(!metadata.matches_config(&config));
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn installed_local_root_does_not_follow_retargeted_configured_junction() {
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
let source_root = codex_home.path().join("source");
|
||||
let other_root = codex_home.path().join("other");
|
||||
for (root, name) in [(&source_root, "source"), (&other_root, "other")] {
|
||||
fs::create_dir_all(root.join(".agents/plugins")).unwrap();
|
||||
fs::write(
|
||||
root.join(".agents/plugins/marketplace.json"),
|
||||
format!(r#"{{"name":"{name}","plugins":[]}}"#),
|
||||
)
|
||||
.unwrap();
|
||||
}
|
||||
let alias = codex_home.path().join("configured-source");
|
||||
crate::test_support::create_directory_junction(&source_root, &alias);
|
||||
let configured_metadata = MarketplaceInstallMetadata::from_source(
|
||||
&MarketplaceSource::Local {
|
||||
path: alias.clone(),
|
||||
},
|
||||
&[],
|
||||
);
|
||||
record_added_marketplace_entry(codex_home.path(), "source", &configured_metadata).unwrap();
|
||||
let canonical_source = source_root.canonicalize().unwrap();
|
||||
let request_metadata = MarketplaceInstallMetadata::from_source(
|
||||
&MarketplaceSource::Local {
|
||||
path: canonical_source.clone(),
|
||||
},
|
||||
&[],
|
||||
);
|
||||
let root = installed_marketplace_root_for_source(
|
||||
codex_home.path(),
|
||||
&codex_home.path().join("marketplaces"),
|
||||
&request_metadata,
|
||||
)
|
||||
.unwrap()
|
||||
.unwrap();
|
||||
|
||||
// Model an updater switching the alias before the caller validates the returned root.
|
||||
fs::remove_dir(&alias).unwrap();
|
||||
crate::test_support::create_directory_junction(&other_root, &alias);
|
||||
assert_eq!(root, canonical_source);
|
||||
assert_eq!(validate_marketplace_root(&root).unwrap(), "source");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -534,13 +534,19 @@ fn is_expected_managed_path(path: &Path, expected: &Path) -> bool {
|
||||
#[cfg(windows)]
|
||||
{
|
||||
let path = path.to_string_lossy();
|
||||
let expected = expected.to_string_lossy();
|
||||
let path = codex_utils_absolute_path::normalize_windows_device_path(&path)
|
||||
.unwrap_or_else(|| path.into_owned());
|
||||
let expected = codex_utils_absolute_path::normalize_windows_device_path(&expected)
|
||||
.unwrap_or_else(|| expected.into_owned());
|
||||
path.replace('/', "\\")
|
||||
.eq_ignore_ascii_case(&expected.replace('/', "\\"))
|
||||
.unwrap_or_else(|| path.into_owned())
|
||||
.replace('/', "\\");
|
||||
// Resolve only the owned root; arbitrary aliases must not borrow managed provenance.
|
||||
let canonical_expected = expected.canonicalize().ok();
|
||||
std::iter::once(expected)
|
||||
.chain(canonical_expected.as_deref())
|
||||
.any(|expected| {
|
||||
let expected = expected.to_string_lossy();
|
||||
let expected = codex_utils_absolute_path::normalize_windows_device_path(&expected)
|
||||
.unwrap_or_else(|| expected.into_owned());
|
||||
path.eq_ignore_ascii_case(&expected.replace('/', "\\"))
|
||||
})
|
||||
}
|
||||
#[cfg(not(any(target_os = "macos", windows)))]
|
||||
{
|
||||
|
||||
@@ -31,6 +31,25 @@ fn primary_runtime_cache_uses_user_profile_on_windows() {
|
||||
));
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[test]
|
||||
fn managed_windows_root_accepts_redirected_target_but_not_unowned_alias() {
|
||||
let temp = TempDir::new().unwrap();
|
||||
let target = temp.path().join("redirected-cache");
|
||||
let owned = temp.path().join(".cache");
|
||||
let alias = temp.path().join("unowned-alias");
|
||||
let other = temp.path().join("different-source");
|
||||
fs::create_dir_all(&target).unwrap();
|
||||
fs::create_dir_all(&other).unwrap();
|
||||
crate::test_support::create_directory_junction(&target, &owned);
|
||||
crate::test_support::create_directory_junction(&target, &alias);
|
||||
let canonical = owned.canonicalize().unwrap();
|
||||
assert!(is_expected_managed_path(&canonical, &owned));
|
||||
assert!(is_expected_managed_path(&owned, &owned));
|
||||
assert!(!is_expected_managed_path(&alias, &owned));
|
||||
assert!(!is_expected_managed_path(&other, &owned));
|
||||
}
|
||||
|
||||
fn config_layer_stack_with_user_config(
|
||||
requirements_toml: &str,
|
||||
user_config: Option<(&str, AbsolutePathBuf)>,
|
||||
|
||||
@@ -52,6 +52,18 @@ use toml::Value;
|
||||
pub(crate) const TEST_CURATED_PLUGIN_SHA: &str = "0123456789abcdef0123456789abcdef01234567";
|
||||
pub(crate) const TEST_CURATED_PLUGIN_CACHE_VERSION: &str = "01234567";
|
||||
|
||||
#[cfg(windows)]
|
||||
/// Model redirected directories without requiring Windows symlink privileges.
|
||||
pub(crate) fn create_directory_junction(target: &Path, alias: &Path) {
|
||||
let output = codex_utils_process::background_command("cmd")
|
||||
.args(["/D", "/C", "mklink", "/J"])
|
||||
.arg(alias)
|
||||
.arg(target)
|
||||
.output()
|
||||
.unwrap();
|
||||
assert!(output.status.success(), "mklink /J failed: {output:?}");
|
||||
}
|
||||
|
||||
pub(crate) fn test_plugins_manager(codex_home: PathBuf) -> PluginsManager {
|
||||
PluginsManager::new(
|
||||
codex_home,
|
||||
|
||||
Reference in New Issue
Block a user