From ce22ea9712aef301faa1c017f83bf06a43bb9580 Mon Sep 17 00:00:00 2001 From: rphilizaire-openai Date: Fri, 7 Aug 2026 18:58:52 +0000 Subject: [PATCH] Generalize skill locator aliases across providers (#37488) ## What changed - Compact executor and orchestrator skill locators under metadata pressure, using provider-specific `e` and `o` aliases alongside host `r` aliases. - Derive alias roots from executor discovery paths and orchestrator namespaces while preserving host plugin root handling. - Centralize alias ordering, deduplication, and longest-prefix matching across skill providers. ## Testing - Cover alias planning, cross-platform executor roots, and host plugin discovery. GitOrigin-RevId: e7d15bd3891c08ae20170e96c4ad45c76d353707 --- codex-rs/ext/skills/src/aliases.rs | 55 +++++ codex-rs/ext/skills/src/aliases_tests.rs | 73 ++++++ codex-rs/ext/skills/src/catalog.rs | 26 +-- codex-rs/ext/skills/src/host_aliases.rs | 68 ++++++ codex-rs/ext/skills/src/lib.rs | 2 + codex-rs/ext/skills/src/provider/executor.rs | 14 +- codex-rs/ext/skills/src/provider/host.rs | 4 +- .../ext/skills/src/provider/host_tests.rs | 4 +- .../ext/skills/src/provider/orchestrator.rs | 4 +- codex-rs/ext/skills/src/render.rs | 215 +++++------------- codex-rs/ext/skills/src/render_tests.rs | 119 ++++++---- .../tests/executor_file_system_authority.rs | 92 ++++++++ 12 files changed, 458 insertions(+), 218 deletions(-) create mode 100644 codex-rs/ext/skills/src/aliases.rs create mode 100644 codex-rs/ext/skills/src/aliases_tests.rs create mode 100644 codex-rs/ext/skills/src/host_aliases.rs diff --git a/codex-rs/ext/skills/src/aliases.rs b/codex-rs/ext/skills/src/aliases.rs new file mode 100644 index 0000000000..954877efcd --- /dev/null +++ b/codex-rs/ext/skills/src/aliases.rs @@ -0,0 +1,55 @@ +use std::collections::HashSet; + +struct AliasRoot { + name: String, + value: String, +} + +pub(crate) struct AliasPlan { + roots: Vec, +} + +impl AliasPlan { + pub(crate) fn build(prefix: &str, candidates: &[&str]) -> Option { + let mut roots = Vec::new(); + let mut seen = HashSet::new(); + + for &root in candidates { + if !seen.insert(root) { + continue; + } + + let index = roots.len(); + roots.push(AliasRoot { + name: format!("{prefix}{index}"), + value: root.to_string(), + }); + } + + (!roots.is_empty()).then_some(Self { roots }) + } + + pub(crate) fn shorten(&self, locator: &str) -> Option { + self.roots + .iter() + .filter_map(|root| { + let suffix = locator + .strip_prefix(root.value.trim_end_matches('/'))? + .strip_prefix('/')?; + Some((root, suffix)) + }) + .max_by_key(|(root, _)| root.value.len()) + .map(|(root, suffix)| format!("{}/{suffix}", root.name)) + } + + pub(crate) fn root_lines(&self) -> Vec { + self.roots + .iter() + .map(|root| format!("- `{}` = `{}`", root.name, root.value)) + .collect() + } +} + +#[cfg(test)] +#[path = "aliases_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/aliases_tests.rs b/codex-rs/ext/skills/src/aliases_tests.rs new file mode 100644 index 0000000000..52bf41b6b9 --- /dev/null +++ b/codex-rs/ext/skills/src/aliases_tests.rs @@ -0,0 +1,73 @@ +use pretty_assertions::assert_eq; + +use super::AliasPlan; + +#[test] +fn assigns_aliases_in_first_seen_order() { + let roots = [ + "/skills/beta", + "/skills/alpha", + "/skills/beta", + "/skills/alpha", + ]; + let plan = AliasPlan::build("r", &roots).expect("alias plan should build"); + + assert_eq!( + vec!["- `r0` = `/skills/beta`", "- `r1` = `/skills/alpha`"], + plan.root_lines() + ); +} + +#[test] +fn reuses_aliases_for_duplicate_roots() { + let root = "/skills/shared"; + let plan = AliasPlan::build("r", &[root, root, root]).expect("alias plan should build"); + + assert_eq!(vec!["- `r0` = `/skills/shared`"], plan.root_lines()); + assert_eq!( + Some("r0/alpha/SKILL.md".to_string()), + plan.shorten("/skills/shared/alpha/SKILL.md") + ); + assert_eq!( + Some("r0/beta/SKILL.md".to_string()), + plan.shorten("/skills/shared/beta/SKILL.md") + ); +} + +#[test] +fn shortens_filesystem_and_skill_resource_locators() { + for (prefix, root, locator, expected) in [ + ( + "r", + "/skills/shared", + "/skills/shared/alpha/SKILL.md", + "r0/alpha/SKILL.md", + ), + ( + "e", + "skill://executor/workspace/skills", + "skill://executor/workspace/skills/alpha/SKILL.md", + "e0/alpha/SKILL.md", + ), + ("o", "skill://plugin", "skill://plugin/alpha", "o0/alpha"), + ] { + let plan = AliasPlan::build(prefix, &[root, root]).expect("alias plan should build"); + + assert_eq!(Some(expected.to_string()), plan.shorten(locator)); + } +} + +#[test] +fn rejects_unknown_roots_and_locators_outside_the_root() { + let root = "/skills/shared"; + let plan = AliasPlan::build("r", &[root, root]).expect("alias plan should build"); + + assert_eq!(None, plan.shorten("/skills/other/alpha")); + assert_eq!(None, plan.shorten("/skills/shared-other/alpha")); + assert_eq!(None, plan.shorten(root)); +} + +#[test] +fn returns_none_when_no_roots_are_provided() { + assert!(AliasPlan::build("r", &[]).is_none()); +} diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index 3655146834..e72c8c73ac 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -178,8 +178,8 @@ pub struct SkillCatalogEntry { pub short_description: Option, pub main_prompt: SkillResourceId, pub display_path: Option, - display_path_root: Option, - display_path_root_order: Option, + alias_root: Option, + alias_root_order: Option, prompt_scope: Option, pub dependencies: Option, pub enabled: bool, @@ -202,8 +202,8 @@ impl SkillCatalogEntry { short_description: None, main_prompt, display_path: None, - display_path_root: None, - display_path_root_order: None, + alias_root: None, + alias_root_order: None, prompt_scope: None, dependencies: None, enabled: true, @@ -221,14 +221,14 @@ impl SkillCatalogEntry { self } - /// Sets the shared filesystem prefix that may be compacted in model-visible paths. - pub fn with_display_path_root(mut self, display_path_root: impl Into) -> Self { - self.display_path_root = Some(display_path_root.into()); + /// Sets the shared locator prefix that may be compacted in model-visible skill catalogs. + pub fn with_alias_root(mut self, alias_root: impl Into) -> Self { + self.alias_root = Some(alias_root.into()); self } - pub(crate) fn with_display_path_root_order(mut self, display_path_root_order: usize) -> Self { - self.display_path_root_order = Some(display_path_root_order); + pub(crate) fn with_alias_root_order(mut self, alias_root_order: usize) -> Self { + self.alias_root_order = Some(alias_root_order); self } @@ -262,12 +262,12 @@ impl SkillCatalogEntry { .unwrap_or_else(|| self.main_prompt.as_str()) } - pub(crate) fn display_path_root(&self) -> Option<&str> { - self.display_path_root.as_deref() + pub(crate) fn alias_root(&self) -> Option<&str> { + self.alias_root.as_deref() } - pub(crate) fn display_path_root_order(&self) -> Option { - self.display_path_root_order + pub(crate) fn alias_root_order(&self) -> Option { + self.alias_root_order } pub(crate) fn prompt_scope(&self) -> Option { diff --git a/codex-rs/ext/skills/src/host_aliases.rs b/codex-rs/ext/skills/src/host_aliases.rs new file mode 100644 index 0000000000..163dfde1d2 --- /dev/null +++ b/codex-rs/ext/skills/src/host_aliases.rs @@ -0,0 +1,68 @@ +use std::collections::HashMap; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; + +use crate::catalog::SkillCatalogEntry; + +pub(crate) fn shared_host_alias_roots(entries: &[&SkillCatalogEntry]) -> Vec { + let mut plugin_version_skill_counts = HashMap::new(); + for root in entries.iter().filter_map(|entry| entry.alias_root()) { + let Some(plugin_version) = plugin_version_base(Path::new(root)) else { + continue; + }; + let count = plugin_version_skill_counts + .entry(plugin_version) + .or_insert(0usize); + *count = count.saturating_add(1); + } + + entries + .iter() + .filter_map(|entry| entry.alias_root()) + .map(|root| { + let root = Path::new(root); + let shared_root = match plugin_version_base(root) { + Some(plugin_version) + if plugin_version_skill_counts + .get(&plugin_version) + .copied() + .unwrap_or_default() + <= 1 => + { + plugin_marketplace_base(root).unwrap_or_else(|| root.to_path_buf()) + } + Some(_) | None => root.to_path_buf(), + }; + + shared_root.to_string_lossy().replace('\\', "/") + }) + .collect() +} + +fn plugin_marketplace_base(path: &Path) -> Option { + let mut candidate = path; + while let Some(parent) = candidate.parent() { + if parent.file_name()?.to_str()? == "cache" + && parent.parent()?.file_name()?.to_str()? == "plugins" + { + return Some(candidate.to_path_buf()); + } + candidate = parent; + } + None +} + +fn plugin_version_base(path: &Path) -> Option { + let marketplace_base = plugin_marketplace_base(path)?; + let mut relative_components = path.strip_prefix(&marketplace_base).ok()?.components(); + let plugin = match relative_components.next()? { + Component::Normal(plugin) => plugin, + _ => return None, + }; + let version = match relative_components.next()? { + Component::Normal(version) => version, + _ => return None, + }; + Some(marketplace_base.join(plugin).join(version)) +} diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index eba9e6e8c2..4833056abc 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -1,9 +1,11 @@ +mod aliases; pub mod catalog; mod catalog_prompt; mod config; mod dynamic_skill_selector; mod extension; mod fragments; +mod host_aliases; mod host_roots; mod host_service; mod host_snapshot; diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index b095708d86..0f3f75fe60 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -89,6 +89,7 @@ impl SkillProvider for ExecutorSkillProvider { &skill, authority.clone(), selected_root_id, + path, environment_id, /*instructions*/ None, )); @@ -172,8 +173,10 @@ impl ExecutorSkillProvider { let mut catalog = SkillCatalog::default(); for root in snapshot.roots() { let selected_root_id = &root.selected_root.id; - let CapabilityRootLocation::Environment { environment_id, .. } = - &root.selected_root.location; + let CapabilityRootLocation::Environment { + environment_id, + path, + } = &root.selected_root.location; let discovery = match &root.result { Ok(discovery) => discovery.as_ref(), Err(error) => { @@ -193,6 +196,7 @@ impl ExecutorSkillProvider { &skill.metadata, authority.clone(), selected_root_id, + path, environment_id, Some(skill.instructions), )); @@ -206,10 +210,15 @@ fn catalog_entry_from_skill( skill: &EnvironmentSkillMetadata, authority: SkillAuthority, selected_root_id: &str, + selected_root_path: &PathUri, environment_id: &str, instructions: Option, ) -> SkillCatalogEntry { let handle_prefix = format!("skill://{selected_root_id}/"); + let alias_root = format!( + "{handle_prefix}{}", + normalized_environment_path(selected_root_path).trim_start_matches('/') + ); let normalized_main_path = normalized_environment_path(&skill.path_to_skills_md); let normalized_package_path = skill.path_to_skills_md.parent().map_or_else( || normalized_main_path.clone(), @@ -245,6 +254,7 @@ fn catalog_entry_from_skill( ) .with_short_description(skill.short_description.clone()) .with_display_path(main_resource) + .with_alias_root(alias_root) .with_dependencies(skill.dependencies.clone()); if skill.allows_implicit_invocation() { diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 6bdbbf5421..b3c4865f3d 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -112,9 +112,9 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { entry = entry.with_display_path(discovery_path.to_string_lossy().replace('\\', "/")); } if let Some(root) = outcome.skill_root_for_path(&skill.path_to_skills_md) { - entry = entry.with_display_path_root(root.to_string_lossy().replace('\\', "/")); + entry = entry.with_alias_root(root.to_string_lossy().replace('\\', "/")); if let Some(root_order) = root_order_by_path.get(root.as_path()) { - entry = entry.with_display_path_root_order(*root_order); + entry = entry.with_alias_root_order(*root_order); } } catalog.push_entry(entry); diff --git a/codex-rs/ext/skills/src/provider/host_tests.rs b/codex-rs/ext/skills/src/provider/host_tests.rs index 6269585f60..d1966e3a28 100644 --- a/codex-rs/ext/skills/src/provider/host_tests.rs +++ b/codex-rs/ext/skills/src/provider/host_tests.rs @@ -48,7 +48,7 @@ async fn host_catalog_entries_carry_their_render_metadata() -> Result<(), Box Option { let uri = validated_skill_uri(resource.uri.as_str(), MAX_SKILL_PACKAGE_URI_CHARS)?; + let namespace = uri.strip_prefix("skill://")?.split_once('/')?.0; let meta = resource.meta.as_ref()?.as_object()?; let allow_implicit_invocation = meta .get("allow_implicit_invocation") @@ -271,7 +272,8 @@ fn catalog_entry_from_resource(resource: &Resource) -> Option description, SkillResourceId::new(main_prompt), ) - .with_display_path(uri); + .with_display_path(uri) + .with_alias_root(format!("skill://{namespace}")); Some(if allow_implicit_invocation { entry diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 7a2127a081..70b05cede5 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,20 +1,17 @@ use std::borrow::Cow; -use std::collections::HashMap; -use std::collections::HashSet; -use std::path::Component; -use std::path::Path; -use std::path::PathBuf; use codex_core_skills::MAX_SKILL_PROMPT_BYTES; use codex_protocol::protocol::SkillScope; use codex_utils_string::approx_token_count; use codex_utils_string::take_bytes_at_char_boundary; +use crate::aliases::AliasPlan; use crate::catalog::SkillCatalog; use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillSourceKind; use crate::catalog_prompt::render_available_skills_body; use crate::fragments::AvailableSkillsInstructions; +use crate::host_aliases::shared_host_alias_roots; const DEFAULT_SKILL_METADATA_CHAR_BUDGET: usize = 8_000; const SKILL_METADATA_CONTEXT_WINDOW_PERCENT: usize = 2; @@ -693,12 +690,14 @@ fn build_aliased_combined_catalog( host_entries: &[&SkillCatalogEntry], budget: SkillMetadataBudget, ) -> Option { - let plan = build_alias_plan(host_entries, budget)?; - if plan.table_cost >= budget.limit() { + let plan = build_alias_plan(host_entries)?; + let skill_root_lines = plan.root_lines(); + let table_cost = aliased_metadata_overhead_cost(budget, &skill_root_lines); + if table_cost >= budget.limit() { return None; } - let adjusted_limit = budget.limit().saturating_sub(plan.table_cost); + let adjusted_limit = budget.limit().saturating_sub(table_cost); let adjusted_budget = match budget { SkillMetadataBudget::Tokens(_) => SkillMetadataBudget::Tokens(adjusted_limit), SkillMetadataBudget::Characters(_) => SkillMetadataBudget::Characters(adjusted_limit), @@ -717,7 +716,7 @@ fn build_aliased_combined_catalog( SkillLine::with_locator( entry, SkillCatalogRenderPolicy::CoreCompatible, - render_skill_path_with_aliases(entry, &plan), + render_skill_locator_with_aliases(entry, &plan), ) }) .collect(); @@ -726,7 +725,7 @@ fn build_aliased_combined_catalog( orchestrator_lines, host_lines, adjusted_budget, - plan.skill_root_lines, + skill_root_lines, )) } @@ -904,47 +903,12 @@ fn available_skills_fragment( .into_fragment(include_skills_usage_instructions) } -struct AliasPlan { - skill_root_lines: Vec, - alias_root_by_display_root: HashMap, - root_aliases: HashMap, - table_cost: usize, -} - fn build_aliased_catalog( entries: &[&SkillCatalogEntry], policy: SkillCatalogRenderPolicy, budget: SkillMetadataBudget, ) -> Option { - let plan = build_alias_plan(entries, budget)?; - if plan.table_cost >= budget.limit() { - return None; - } - - let adjusted_limit = budget.limit().saturating_sub(plan.table_cost); - let adjusted_budget = match budget { - SkillMetadataBudget::Tokens(_) => SkillMetadataBudget::Tokens(adjusted_limit), - SkillMetadataBudget::Characters(_) => SkillMetadataBudget::Characters(adjusted_limit), - }; - let skill_lines = entries - .iter() - .map(|entry| { - SkillLine::with_locator(entry, policy, render_skill_path_with_aliases(entry, &plan)) - }) - .collect(); - Some(render_catalog( - skill_lines, - adjusted_budget, - plan.skill_root_lines, - policy, - )) -} - -fn build_alias_plan( - entries: &[&SkillCatalogEntry], - budget: SkillMetadataBudget, -) -> Option { - // The shared alias prompt only describes host filesystem skills. + // Alias instructions currently describe only host filesystem skills. if entries .iter() .any(|entry| entry.authority.kind != SkillSourceKind::Host) @@ -952,132 +916,71 @@ fn build_alias_plan( return None; } - let plugin_version_skill_counts = plugin_version_skill_counts_for_entries(entries); - let mut alias_root_by_display_root = HashMap::new(); - let mut alias_roots = Vec::new(); - let mut seen = HashSet::new(); - let mut alias_ordered_entries = entries.to_vec(); - alias_ordered_entries - .sort_by_key(|entry| entry.display_path_root_order().unwrap_or(usize::MAX)); - for entry in alias_ordered_entries { - if entry.authority.kind != SkillSourceKind::Host { - continue; - } - let Some(display_root) = entry.display_path_root() else { - continue; - }; - let alias_root = - alias_root_for_display_root(Path::new(display_root), &plugin_version_skill_counts) - .to_string_lossy() - .replace('\\', "/"); - alias_root_by_display_root.insert(display_root.to_string(), alias_root.clone()); - if seen.insert(alias_root.clone()) { - alias_roots.push(alias_root); - } - } - if alias_roots.is_empty() { + let plan = build_alias_plan(entries)?; + let skill_root_lines = plan.root_lines(); + let table_cost = aliased_metadata_overhead_cost(budget, &skill_root_lines); + if table_cost >= budget.limit() { return None; } - let root_aliases = alias_roots + let adjusted_limit = budget.limit().saturating_sub(table_cost); + let adjusted_budget = match budget { + SkillMetadataBudget::Tokens(_) => SkillMetadataBudget::Tokens(adjusted_limit), + SkillMetadataBudget::Characters(_) => SkillMetadataBudget::Characters(adjusted_limit), + }; + let skill_lines = entries .iter() - .enumerate() - .map(|(index, root)| (root.clone(), format!("r{index}"))) + .map(|entry| { + SkillLine::with_locator( + entry, + policy, + render_skill_locator_with_aliases(entry, &plan), + ) + }) .collect(); - let skill_root_lines = alias_roots - .iter() - .enumerate() - .map(|(index, root)| format!("- `r{index}` = `{root}`")) - .collect::>(); - let table_cost = aliased_metadata_overhead_cost(budget, &skill_root_lines); - Some(AliasPlan { + Some(render_catalog( + skill_lines, + adjusted_budget, skill_root_lines, - alias_root_by_display_root, - root_aliases, - table_cost, - }) + policy, + )) } -fn plugin_version_skill_counts_for_entries( - entries: &[&SkillCatalogEntry], -) -> HashMap { - let mut counts = HashMap::new(); - for root in entries.iter().filter_map(|entry| { - (entry.authority.kind == SkillSourceKind::Host) - .then(|| entry.display_path_root()) - .flatten() - }) { - if let Some(plugin_version_base) = plugin_version_base(Path::new(root)) { - let count = counts.entry(plugin_version_base).or_insert(0usize); - *count = count.saturating_add(1); - } +fn build_alias_plan(entries: &[&SkillCatalogEntry]) -> Option { + let source = &entries.first()?.authority.kind; + if entries.iter().any(|entry| &entry.authority.kind != source) { + return None; } - counts -} - -fn alias_root_for_display_root( - root: &Path, - plugin_version_skill_counts: &HashMap, -) -> PathBuf { - let Some(plugin_version_base) = plugin_version_base(root) else { - return root.to_path_buf(); + let prefix = match source { + SkillSourceKind::Host => "r", + SkillSourceKind::Executor => "e", + SkillSourceKind::Orchestrator => "o", + SkillSourceKind::Custom(_) => return None, }; - let skill_count = plugin_version_skill_counts - .get(&plugin_version_base) - .copied() - .unwrap_or_default(); - if skill_count > 1 { - root.to_path_buf() - } else { - plugin_marketplace_base(root).unwrap_or_else(|| root.to_path_buf()) - } -} -fn plugin_marketplace_base(path: &Path) -> Option { - let mut candidate = path; - while let Some(parent) = candidate.parent() { - if parent.file_name()?.to_str()? == "cache" - && parent.parent()?.file_name()?.to_str()? == "plugins" - { - return Some(candidate.to_path_buf()); - } - candidate = parent; - } - None -} - -fn plugin_version_base(path: &Path) -> Option { - let marketplace_base = plugin_marketplace_base(path)?; - let mut relative_components = path.strip_prefix(&marketplace_base).ok()?.components(); - let plugin = match relative_components.next()? { - Component::Normal(plugin) => plugin, - _ => return None, + let mut alias_ordered_entries = entries.to_vec(); + alias_ordered_entries.sort_by_key(|entry| entry.alias_root_order().unwrap_or(usize::MAX)); + let roots = match source { + SkillSourceKind::Host => shared_host_alias_roots(&alias_ordered_entries), + SkillSourceKind::Executor | SkillSourceKind::Orchestrator => alias_ordered_entries + .iter() + .filter_map(|entry| entry.alias_root()) + .map(str::to_string) + .collect(), + SkillSourceKind::Custom(_) => return None, }; - let version = match relative_components.next()? { - Component::Normal(version) => version, - _ => return None, - }; - Some(marketplace_base.join(plugin).join(version)) + let roots = roots.iter().map(String::as_str).collect::>(); + + AliasPlan::build(prefix, &roots) } -fn render_skill_path_with_aliases(entry: &SkillCatalogEntry, plan: &AliasPlan) -> String { - if entry.authority.kind != SkillSourceKind::Host { +fn render_skill_locator_with_aliases(entry: &SkillCatalogEntry, plan: &AliasPlan) -> String { + if entry.alias_root().is_none() { return entry.rendered_path().to_string(); } - let Some(display_root) = entry.display_path_root() else { - return entry.rendered_path().to_string(); - }; - let Some(alias_root) = plan.alias_root_by_display_root.get(display_root) else { - return entry.rendered_path().to_string(); - }; - let Some(alias) = plan.root_aliases.get(alias_root) else { - return entry.rendered_path().to_string(); - }; - let Ok(relative_path) = Path::new(entry.rendered_path()).strip_prefix(alias_root) else { - return entry.rendered_path().to_string(); - }; - let relative_path = relative_path.to_string_lossy().replace('\\', "/"); - format!("{alias}/{relative_path}") + let locator = entry.rendered_path().replace('\\', "/"); + plan.shorten(&locator) + .unwrap_or_else(|| entry.rendered_path().to_string()) } fn aliased_metadata_overhead_cost( diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 9088c1d6e7..2f0b16327c 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -200,10 +200,10 @@ fn path_aliases_are_not_used_without_budget_pressure() { entries: vec![ entry("alpha", "Alpha skill.", /*short_description*/ None) .with_display_path(format!("{root}/alpha/SKILL.md")) - .with_display_path_root(root), + .with_alias_root(root), entry("beta", "Beta skill.", /*short_description*/ None) .with_display_path(format!("{root}/beta/SKILL.md")) - .with_display_path_root(root), + .with_alias_root(root), ], warnings: Vec::new(), }; @@ -232,7 +232,7 @@ fn path_aliases_retain_every_skill_under_budget_pressure() { let name = format!("shared-root-skill-{index}"); entry(&name, "Description.", /*short_description*/ None) .with_display_path(format!("{root}/skill-{index}/SKILL.md")) - .with_display_path_root(root) + .with_alias_root(root) }) .collect::>(); let catalog = SkillCatalog { @@ -240,17 +240,17 @@ fn path_aliases_retain_every_skill_under_budget_pressure() { warnings: Vec::new(), }; let visible_entries = catalog.entries.iter().collect::>(); - let plan = build_alias_plan( - &visible_entries, + let plan = build_alias_plan(&visible_entries).expect("alias plan should build"); + let table_cost = aliased_metadata_overhead_cost( SkillMetadataBudget::Characters(usize::MAX), - ) - .expect("alias plan should build"); - let alias_minimum = visible_entries.iter().fold(plan.table_cost, |cost, entry| { + &plan.root_lines(), + ); + let alias_minimum = visible_entries.iter().fold(table_cost, |cost, entry| { cost.saturating_add( SkillLine::with_locator( entry, SkillCatalogRenderPolicy::ExtensionCompatible, - render_skill_path_with_aliases(entry, &plan), + render_skill_locator_with_aliases(entry, &plan), ) .minimum_cost(SkillMetadataBudget::Characters(usize::MAX)), ) @@ -330,9 +330,9 @@ async fn host_alias_roots_follow_core_discovery_order() -> Result<(), Box>(); SkillCatalogRenderPolicy::CoreCompatible.order_entries(&mut entries); - let actual = build_alias_plan(&entries, SkillMetadataBudget::Characters(usize::MAX)) + let actual = build_alias_plan(&entries) .expect("alias plan should build") - .skill_root_lines; + .root_lines(); std::fs::remove_dir_all(parent)?; assert_eq!( @@ -359,7 +359,7 @@ fn mixed_catalogs_keep_absolute_authority_aware_rendering_under_budget_pressure( let name = format!("host-skill-{index}"); entry(&name, "Description.", /*short_description*/ None) .with_display_path(format!("{root}/skill-{index}/SKILL.md")) - .with_display_path_root(root) + .with_alias_root(root) }) .collect::>(); entries.push( @@ -384,13 +384,7 @@ fn mixed_catalogs_keep_absolute_authority_aware_rendering_under_budget_pressure( ) }); - assert!( - build_alias_plan( - &visible_entries, - SkillMetadataBudget::Characters(usize::MAX), - ) - .is_none() - ); + assert!(build_alias_plan(&visible_entries).is_none()); let fragment = available_skills_fragment( &catalog, @@ -488,7 +482,7 @@ fn mixed_catalog_prefers_executor_inclusion_over_total_aliased_inclusion() { .map(|name| { entry(name, "", /*short_description*/ None) .with_display_path(format!("{root}/{name}/SKILL.md")) - .with_display_path_root(root.as_str()) + .with_alias_root(root.as_str()) }) .collect(), warnings: Vec::new(), @@ -563,38 +557,79 @@ fn mixed_catalog_prefers_executor_inclusion_over_total_aliased_inclusion() { assert_eq!(host.skill_root_lines, Vec::::new()); } -#[test] -fn singleton_plugin_versions_share_the_marketplace_alias_root() { - let github_root = "/Users/test/.codex/plugins/cache/openai-curated/github/hash123/skills"; - let slack_root = "/Users/test/.codex/plugins/cache/openai-curated/slack/hash456/skills"; - let entries = [ - entry("github", "GitHub skill.", /*short_description*/ None) - .with_display_path(format!("{github_root}/github/SKILL.md")) - .with_display_path_root(github_root), - entry("slack", "Slack skill.", /*short_description*/ None) - .with_display_path(format!("{slack_root}/slack/SKILL.md")) - .with_display_path_root(slack_root), - ]; - let visible_entries = entries.iter().collect::>(); - - let plan = build_alias_plan( - &visible_entries, - SkillMetadataBudget::Characters(usize::MAX), +#[tokio::test] +async fn singleton_plugin_versions_share_the_marketplace_alias_root() +-> Result<(), Box> { + let unique = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos(); + let parent = std::env::temp_dir().join(format!( + "codex-skills-extension-marketplace-alias-{}-{unique}", + std::process::id() + )); + let marketplace_path = parent.join("plugins/cache/openai-curated"); + let mut roots = Vec::new(); + for (plugin, version) in [("github", "hash123"), ("slack", "hash456")] { + let root = marketplace_path.join(plugin).join(version).join("skills"); + let skill_dir = root.join(plugin); + std::fs::create_dir_all(&skill_dir)?; + std::fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {plugin}\ndescription: {plugin} skill.\n---\n"), + )?; + roots.push(HostSkillRoot::host( + AbsolutePathBuf::try_from(std::fs::canonicalize(root)?)?, + SkillScope::User, + Arc::clone(&LOCAL_FS), + )); + } + let marketplace_root = dunce::canonicalize(&marketplace_path)? + .to_string_lossy() + .replace('\\', "/"); + let outcome = load_and_merge_host_skill_roots( + roots, + &Semaphore::new(/*permits*/ 2), + /*restriction_product*/ None, + /*plugin_skill_snapshots*/ None, ) - .expect("alias plan should build"); + .await; + let catalog = HostSkillProvider::new() + .list(SkillListQuery { + turn_id: "turn-1".to_string(), + executor_roots: Vec::new(), + resolved_executor_roots: Vec::new(), + host_snapshot: Some(Arc::new(HostSkillsSnapshot::new(Arc::new(outcome)))), + include_host_skills: true, + include_bundled_skills: false, + include_orchestrator_skills: false, + mcp_resources: None, + executor_capability_discovery: None, + }) + .await?; + let entries = catalog.entries.iter().collect::>(); + let plan = build_alias_plan(&entries).expect("alias plan should build"); + let github = entries + .iter() + .find(|entry| entry.name == "github") + .ok_or("GitHub skill should be discovered")?; + let slack = entries + .iter() + .find(|entry| entry.name == "slack") + .ok_or("Slack skill should be discovered")?; assert_eq!( - plan.skill_root_lines, - vec!["- `r0` = `/Users/test/.codex/plugins/cache/openai-curated`".to_string()] + plan.root_lines(), + vec![format!("- `r0` = `{marketplace_root}`")] ); assert_eq!( - render_skill_path_with_aliases(&entries[0], &plan), + render_skill_locator_with_aliases(github, &plan), "r0/github/hash123/skills/github/SKILL.md" ); assert_eq!( - render_skill_path_with_aliases(&entries[1], &plan), + render_skill_locator_with_aliases(slack, &plan), "r0/slack/hash456/skills/slack/SKILL.md" ); + + std::fs::remove_dir_all(parent)?; + Ok(()) } #[test] diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index fe381a3b36..ed4a92ebcc 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -8,10 +8,14 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; +use codex_exec_server::CapabilityRootDiscovery; +use codex_exec_server::CapabilityTextFile; use codex_exec_server::CopyOptions; use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::DiscoveredSkillFiles; use codex_exec_server::EnvironmentManager; use codex_exec_server::ExecutorCapabilityDiscoveryCache; +use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::ExecutorFileSystemFuture; use codex_exec_server::FileMetadata; @@ -29,6 +33,8 @@ use codex_skills_extension::ExecutorSkillProvider; use codex_skills_extension::HostSkillsLoadInput; use codex_skills_extension::HostSkillsService; use codex_skills_extension::catalog::SkillAuthority; +use codex_skills_extension::catalog::SkillCatalog; +use codex_skills_extension::catalog::SkillCatalogEntry; use codex_skills_extension::catalog::SkillPackageId; use codex_skills_extension::catalog::SkillResourceId; use codex_skills_extension::catalog::SkillSourceKind; @@ -367,6 +373,92 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { std::fs::remove_dir_all(test_root).expect("remove skill directory"); } +#[tokio::test] +async fn executor_discovery_preserves_posix_and_windows_locator_alias_roots() { + let provider = ExecutorSkillProvider::new_with_restriction_product( + Arc::new(EnvironmentManager::default_for_tests()), + /*restriction_product*/ None, + ); + + for (root_uri, alias_root) in [ + ( + "file:///workspace/skills", + "skill://cross-platform-root/workspace/skills", + ), + ( + "file:///C:/workspace/skills", + "skill://cross-platform-root/C:/workspace/skills", + ), + ] { + let root = PathUri::parse(root_uri).expect("executor root URI"); + let skill_path = root.join("skill/SKILL.md").expect("executor skill URI"); + let selected_root = SelectedCapabilityRoot { + id: "cross-platform-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "remote".to_string(), + path: root.clone(), + }, + }; + let discovery = ExecutorCapabilityDiscoverySnapshot::new( + std::slice::from_ref(&selected_root), + vec![Ok(Arc::new(CapabilityRootDiscovery { + id: selected_root.id.clone(), + path: root, + plugin: None, + skills: vec![DiscoveredSkillFiles { + instructions: CapabilityTextFile { + path: skill_path.clone(), + contents: SKILL_CONTENTS.to_string(), + }, + metadata: None, + }], + namespace_manifests: Vec::new(), + warnings: Vec::new(), + error: None, + }))], + HashMap::new(), + ); + let catalog = provider + .list(SkillListQuery { + turn_id: "turn-1".to_string(), + executor_roots: vec![selected_root], + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: true, + include_orchestrator_skills: false, + mcp_resources: None, + executor_capability_discovery: Some(discovery), + }) + .await + .expect("list executor skills"); + let resource = format!("{alias_root}/skill/SKILL.md"); + + assert_eq!( + catalog, + SkillCatalog { + entries: vec![ + SkillCatalogEntry::new( + SkillPackageId(format!("{alias_root}/skill")), + SkillAuthority::new(SkillSourceKind::Executor, "cross-platform-root"), + "synthetic", + "Synthetic executor skill.", + SkillResourceId::environment_with_contents( + resource.clone(), + "remote", + skill_path, + SKILL_CONTENTS.to_string(), + ), + ) + .with_display_path(resource) + .with_alias_root(alias_root) + ], + warnings: Vec::new(), + } + ); + } +} + #[tokio::test] async fn executor_discovery_routes_produce_equivalent_catalog_metadata() { let id = NEXT_TEST_ROOT_ID.fetch_add(1, Ordering::Relaxed);