From 2ffe8cd579c27fb3ece5c4725d7fff5477a15cc9 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Wed, 22 Jul 2026 12:34:52 +0000 Subject: [PATCH] Match core skill ordering in extension catalogs (#34746) ## Why Core-compatible catalog rendering needs to preserve the core renderer's skill priority, including when metadata budgets omit lower-priority entries. ## What changed - Carry each host skill's `SkillScope` into its catalog entry. - Under `CoreCompatible`, order visible skills by system, admin, repo, and user scope, then by name and prompt resource ID. - Preserve insertion order under `ExtensionCompatible`. ## Testing Add coverage for propagating host skill scope and for both rendering policies. GitOrigin-RevId: 9594a177edb2166abf1b5719c51a46fcee000600 --- codex-rs/ext/skills/src/catalog.rs | 12 +++ codex-rs/ext/skills/src/provider/host.rs | 5 + .../ext/skills/src/provider/host_tests.rs | 54 ++++++++++ codex-rs/ext/skills/src/render.rs | 28 ++++- codex-rs/ext/skills/src/render_tests.rs | 102 +++++++++++++++++- 5 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 codex-rs/ext/skills/src/provider/host_tests.rs diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index e9a217bfbff8..23bf112767b7 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -1,3 +1,4 @@ +use codex_protocol::protocol::SkillScope; use codex_skills::SkillDependencies; use codex_utils_path_uri::PathUri; use std::sync::Arc; @@ -139,6 +140,7 @@ pub struct SkillCatalogEntry { pub short_description: Option, pub main_prompt: SkillResourceId, pub display_path: Option, + prompt_scope: Option, pub dependencies: Option, pub enabled: bool, pub prompt_visible: bool, @@ -160,6 +162,7 @@ impl SkillCatalogEntry { short_description: None, main_prompt, display_path: None, + prompt_scope: None, dependencies: None, enabled: true, prompt_visible: true, @@ -176,6 +179,11 @@ impl SkillCatalogEntry { self } + pub(crate) fn with_prompt_scope(mut self, prompt_scope: SkillScope) -> Self { + self.prompt_scope = Some(prompt_scope); + self + } + pub fn with_dependencies(mut self, dependencies: Option) -> Self { self.dependencies = dependencies; self @@ -196,6 +204,10 @@ impl SkillCatalogEntry { .as_deref() .unwrap_or_else(|| self.main_prompt.as_str()) } + + pub(crate) fn prompt_scope(&self) -> Option { + self.prompt_scope + } } /// Merged catalog for one turn. diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 493ce51eb0f9..7b8f8b429127 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -116,6 +116,7 @@ fn catalog_entry_from_skill(skill: &SkillMetadata, enabled: bool) -> SkillCatalo ) .with_short_description(skill.short_description.clone()) .with_display_path(display_path) + .with_prompt_scope(skill.scope) .with_dependencies(skill.dependencies.clone()); if !enabled { @@ -127,3 +128,7 @@ fn catalog_entry_from_skill(skill: &SkillMetadata, enabled: bool) -> SkillCatalo entry } + +#[cfg(test)] +#[path = "host_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/provider/host_tests.rs b/codex-rs/ext/skills/src/provider/host_tests.rs new file mode 100644 index 000000000000..072c638785e5 --- /dev/null +++ b/codex-rs/ext/skills/src/provider/host_tests.rs @@ -0,0 +1,54 @@ +use std::sync::Arc; +use std::time::SystemTime; +use std::time::UNIX_EPOCH; + +use codex_core_skills::loader::SkillRoot; +use codex_core_skills::loader::load_skills_from_roots; +use codex_exec_server::LOCAL_FS; +use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; +use tokio::sync::Semaphore; + +use super::catalog_from_outcome; + +#[tokio::test] +async fn host_catalog_entries_carry_their_prompt_scope() -> Result<(), Box> { + let unique = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos(); + let root = std::env::temp_dir().join(format!( + "codex-skills-extension-host-provider-{}-{unique}", + std::process::id() + )); + let skill_path = root.join("demo").join("SKILL.md"); + std::fs::create_dir_all( + skill_path + .parent() + .ok_or("skill path should have a parent")?, + )?; + std::fs::write( + &skill_path, + "---\nname: demo\ndescription: Demo skill.\n---\n# Demo\n", + )?; + let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root)?)?; + let outcome = load_skills_from_roots( + [SkillRoot { + path: root.clone(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_id: None, + plugin_namespace: None, + plugin_root: None, + }], + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(1)), + ) + .await; + + let catalog = catalog_from_outcome(&outcome); + + assert_eq!(catalog.entries.len(), 1); + assert_eq!(catalog.entries[0].prompt_scope(), Some(SkillScope::User)); + + std::fs::remove_dir_all(root.as_path())?; + Ok(()) +} diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index 440385d80059..f7152673be46 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -1,5 +1,6 @@ use std::borrow::Cow; +use codex_protocol::protocol::SkillScope; use codex_utils_string::approx_token_count; use codex_utils_string::take_bytes_at_char_boundary; @@ -41,6 +42,27 @@ impl SkillCatalogRenderPolicy { .unwrap_or(entry.description.as_str()), } } + + fn order_entries(self, entries: &mut [&SkillCatalogEntry]) { + match self { + Self::CoreCompatible => { + let scope_rank = |entry: &SkillCatalogEntry| match entry.prompt_scope() { + Some(SkillScope::System) => 0, + Some(SkillScope::Admin) => 1, + Some(SkillScope::Repo) => 2, + Some(SkillScope::User) => 3, + None => 4, + }; + entries.sort_by(|a, b| { + scope_rank(a) + .cmp(&scope_rank(b)) + .then_with(|| a.name.cmp(&b.name)) + .then_with(|| a.main_prompt.as_str().cmp(b.main_prompt.as_str())) + }); + } + Self::ExtensionCompatible => {} + } + } } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -287,10 +309,14 @@ pub(crate) fn available_skills_fragment( policy: SkillCatalogRenderPolicy, budget: SkillMetadataBudget, ) -> Option { - let skill_lines = catalog + let mut entries = catalog .entries .iter() .filter(|entry| entry.enabled && entry.prompt_visible) + .collect::>(); + policy.order_entries(&mut entries); + let skill_lines = entries + .iter() .map(|entry| SkillLine::new(entry, policy)) .collect(); let (mut skill_lines, mut omitted) = render_skill_lines(skill_lines, budget); diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index ee94e19950b3..d9d615ad31dd 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -4,19 +4,115 @@ use crate::catalog::SkillPackageId; use crate::catalog::SkillResourceId; use codex_core_skills::render_available_skills_body; use codex_extension_api::ContextualUserFragment; +use codex_protocol::protocol::SkillScope; use pretty_assertions::assert_eq; fn entry(name: &str, description: &str, short_description: Option<&str>) -> SkillCatalogEntry { + entry_with_path( + name, + description, + short_description, + &format!("/skills/{name}/SKILL.md"), + ) +} + +fn entry_with_path( + name: &str, + description: &str, + short_description: Option<&str>, + path: &str, +) -> SkillCatalogEntry { SkillCatalogEntry::new( - SkillPackageId(name.to_string()), + SkillPackageId(path.to_string()), SkillAuthority::new(SkillSourceKind::Host, "host"), name, description, - SkillResourceId::new(format!("/skills/{name}/SKILL.md")), + SkillResourceId::new(path), ) .with_short_description(short_description.map(str::to_string)) } +#[test] +fn ordering_follows_render_policy() { + let catalog = SkillCatalog { + entries: [ + ("repo-zeta", SkillScope::Repo, "/skills/repo-zeta/SKILL.md"), + ( + "user-alpha", + SkillScope::User, + "/skills/user-alpha/SKILL.md", + ), + ( + "system-zeta", + SkillScope::System, + "/skills/system-zeta/SKILL.md", + ), + ( + "admin-alpha", + SkillScope::Admin, + "/skills/admin-alpha/SKILL.md", + ), + ( + "repo-alpha", + SkillScope::Repo, + "/skills/repo-alpha-z/SKILL.md", + ), + ( + "repo-alpha", + SkillScope::Repo, + "/skills/repo-alpha-a/SKILL.md", + ), + ] + .into_iter() + .map(|(name, scope, path)| { + entry_with_path(name, "Description.", /*short_description*/ None, path) + .with_prompt_scope(scope) + }) + .collect(), + warnings: Vec::new(), + }; + + let render = |policy| { + available_skills_fragment( + &catalog, + /*include_skills_usage_instructions*/ false, + policy, + SkillMetadataBudget::Characters(usize::MAX), + ) + .expect("catalog should render") + .body() + }; + + assert_eq!( + render(SkillCatalogRenderPolicy::CoreCompatible), + render_available_skills_body( + &[], + &[ + "- system-zeta: Description. (file: /skills/system-zeta/SKILL.md)".to_string(), + "- admin-alpha: Description. (file: /skills/admin-alpha/SKILL.md)".to_string(), + "- repo-alpha: Description. (file: /skills/repo-alpha-a/SKILL.md)".to_string(), + "- repo-alpha: Description. (file: /skills/repo-alpha-z/SKILL.md)".to_string(), + "- repo-zeta: Description. (file: /skills/repo-zeta/SKILL.md)".to_string(), + "- user-alpha: Description. (file: /skills/user-alpha/SKILL.md)".to_string(), + ], + ) + ); + assert_eq!( + render(SkillCatalogRenderPolicy::ExtensionCompatible), + render_available_skills_body( + &[], + &[ + "- repo-zeta: Description. (file: /skills/repo-zeta/SKILL.md)".to_string(), + "- user-alpha: Description. (file: /skills/user-alpha/SKILL.md)".to_string(), + "- system-zeta: Description. (file: /skills/system-zeta/SKILL.md)".to_string(), + "- admin-alpha: Description. (file: /skills/admin-alpha/SKILL.md)".to_string(), + "- repo-alpha: Description. (file: /skills/repo-alpha-z/SKILL.md)".to_string(), + "- repo-alpha: Description. (file: /skills/repo-alpha-a/SKILL.md)".to_string(), + ], + ) + ); +} + #[test] fn description_selection_follows_render_policy() { let catalog = SkillCatalog { @@ -51,8 +147,8 @@ fn description_selection_follows_render_policy() { render_available_skills_body( &[], &[ - "- shortened: full description (file: /skills/shortened/SKILL.md)".to_string(), "- fallback: fallback description (file: /skills/fallback/SKILL.md)".to_string(), + "- shortened: full description (file: /skills/shortened/SKILL.md)".to_string(), ], ) );