diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index e146b79866..c9d04dd9e5 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -61,6 +61,11 @@ impl SkillLoadOutcome { self.skill_root_by_path.get(path) } + /// Returns loaded skill roots in discovery order. + pub fn skill_roots_in_discovery_order(&self) -> impl Iterator { + self.skill_roots.iter() + } + pub(crate) fn file_system_for_skill( &self, skill: &SkillMetadata, diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index 3040e0bf10..fac170bcce 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -35,6 +35,7 @@ use codex_skills_extension::provider::SkillListQuery; use codex_skills_extension::provider::SkillProviderFuture; use codex_skills_extension::provider::SkillReadRequest; use codex_skills_extension::provider::SkillSearchRequest; +use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_string::approx_token_count; use core_test_support::apps_test_server::AppsTestServer; use core_test_support::apps_test_server::apps_enabled_builder; @@ -602,6 +603,111 @@ async fn production_turn_keeps_full_host_only_catalog_when_it_fits() -> Result<( Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn production_turn_preserves_host_alias_root_order_across_turns() -> Result<()> { + let server = responses::start_mock_server().await; + let response = responses::mount_sse_sequence( + &server, + ["resp-1", "resp-2"] + .into_iter() + .map(|response_id| { + sse(vec![ + ev_response_created(response_id), + ev_completed(response_id), + ]) + }) + .collect(), + ) + .await; + let temp_parent = TempDir::new()?; + let long_parent = temp_parent + .path() + .join("codex-home-with-long-shared-prefix-for-production-alias-order-test"); + std::fs::create_dir_all(&long_parent)?; + let codex_home = Arc::new(TempDir::new_in(&long_parent)?); + let first_root_path = long_parent.join("first-discovered-skills-root-with-long-shared-prefix"); + let second_root_path = + long_parent.join("second-discovered-skills-root-with-long-shared-prefix"); + for (root, prefix) in [ + (&first_root_path, "z-first"), + (&second_root_path, "a-second"), + ] { + for index in 0..6 { + let name = format!("{prefix}-{index:02}"); + let skill_dir = root.join(&name); + std::fs::create_dir_all(&skill_dir)?; + std::fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: d\n---\n\n# body\n"), + )?; + } + } + let first_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&first_root_path)?)?; + let second_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&second_root_path)?)?; + let (extensions, _) = + catalog_extensions(SkillCatalog::default(), /*include_host_provider*/ true); + let mut builder = test_codex() + .with_home(Arc::clone(&codex_home)) + .with_extensions(extensions) + .with_model_info_override("gpt-5.5", |model_info| { + model_info.context_window = Some(SHORTENING_CONTEXT_WINDOW); + model_info.max_context_window = None; + }) + .with_config(configure_catalog_test); + let test = builder.build_with_auto_env(&server).await?; + test.thread_manager + .skills_service() + .set_extra_roots(vec![first_root.clone(), second_root.clone()]); + + test.submit_turn("Inspect the available skills.").await?; + test.submit_turn("Inspect the available skills again.") + .await?; + + let expected_alias_lines = vec![ + format!( + "- `r0` = `{}`", + first_root.to_string_lossy().replace('\\', "/") + ), + format!( + "- `r1` = `{}`", + second_root.to_string_lossy().replace('\\', "/") + ), + ]; + let expected_skill_lines = vec![ + "- a-second-00: d (file: r1/a-second-00/SKILL.md)".to_string(), + "- z-first-00: d (file: r0/z-first-00/SKILL.md)".to_string(), + ]; + let actual = response + .requests() + .iter() + .map(|request| { + let developer_text = request.message_input_texts("developer").join("\n"); + let alias_lines = developer_text + .lines() + .filter(|line| line.starts_with("- `r")) + .map(str::to_string) + .collect::>(); + let skill_lines = developer_text + .lines() + .filter(|line| { + line.starts_with("- a-second-00:") || line.starts_with("- z-first-00:") + }) + .map(str::to_string) + .collect::>(); + (alias_lines, skill_lines) + }) + .collect::>(); + + assert_eq!( + actual, + vec![ + (expected_alias_lines.clone(), expected_skill_lines.clone()), + (expected_alias_lines, expected_skill_lines), + ] + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn production_turn_uses_provider_host_catalog_and_core_snapshot_injection() -> Result<()> { let server = responses::start_mock_server().await; diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index 6d09d39cef..3655146834 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -179,6 +179,7 @@ pub struct SkillCatalogEntry { pub main_prompt: SkillResourceId, pub display_path: Option, display_path_root: Option, + display_path_root_order: Option, prompt_scope: Option, pub dependencies: Option, pub enabled: bool, @@ -202,6 +203,7 @@ impl SkillCatalogEntry { main_prompt, display_path: None, display_path_root: None, + display_path_root_order: None, prompt_scope: None, dependencies: None, enabled: true, @@ -225,6 +227,11 @@ impl SkillCatalogEntry { 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); + self + } + pub(crate) fn with_prompt_scope(mut self, prompt_scope: SkillScope) -> Self { self.prompt_scope = Some(prompt_scope); self @@ -259,6 +266,10 @@ impl SkillCatalogEntry { self.display_path_root.as_deref() } + pub(crate) fn display_path_root_order(&self) -> Option { + self.display_path_root_order + } + pub(crate) fn prompt_scope(&self) -> Option { self.prompt_scope } diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index e2c47b6a1a..53ccb885aa 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use codex_core_skills::SkillLoadOutcome; use codex_skills::SkillMetadata; @@ -82,6 +84,11 @@ impl SkillProvider for HostSkillProvider { } fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { + let root_order_by_path = outcome + .skill_roots_in_discovery_order() + .enumerate() + .map(|(index, root)| (root.as_path(), index)) + .collect::>(); let mut catalog = SkillCatalog { entries: Vec::new(), warnings: outcome @@ -101,6 +108,9 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { let mut entry = catalog_entry_from_skill(skill, enabled); 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('\\', "/")); + if let Some(root_order) = root_order_by_path.get(root.as_path()) { + entry = entry.with_display_path_root_order(*root_order); + } } catalog.push_entry(entry); } diff --git a/codex-rs/ext/skills/src/render.rs b/codex-rs/ext/skills/src/render.rs index c8b36fc1e3..c41f3966e6 100644 --- a/codex-rs/ext/skills/src/render.rs +++ b/codex-rs/ext/skills/src/render.rs @@ -892,7 +892,10 @@ fn build_alias_plan( let mut alias_root_by_display_root = HashMap::new(); let mut alias_roots = Vec::new(); let mut seen = HashSet::new(); - for entry in entries { + 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; } diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index 29eb36ff33..16c51fcf00 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -2,10 +2,22 @@ use super::*; use crate::catalog::SkillAuthority; use crate::catalog::SkillPackageId; use crate::catalog::SkillResourceId; +use crate::provider::HostSkillProvider; +use crate::provider::SkillListQuery; +use crate::provider::SkillProvider; +use codex_core_skills::HostSkillsSnapshot; +use codex_core_skills::loader::SkillRoot; +use codex_core_skills::loader::load_skills_from_roots; use codex_core_skills::render_available_skills_body; +use codex_exec_server::LOCAL_FS; use codex_extension_api::ContextualUserFragment; use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; +use std::sync::Arc; +use std::time::SystemTime; +use std::time::UNIX_EPOCH; +use tokio::sync::Semaphore; fn entry(name: &str, description: &str, short_description: Option<&str>) -> SkillCatalogEntry { entry_with_path( @@ -266,6 +278,89 @@ fn path_aliases_retain_every_skill_under_budget_pressure() { assert!(!body.contains("additional skills omitted")); } +#[tokio::test] +async fn host_alias_roots_follow_core_discovery_order() -> Result<(), Box> { + let unique = SystemTime::now().duration_since(UNIX_EPOCH)?.as_nanos(); + let parent = std::env::temp_dir().join(format!( + "codex-skills-extension-alias-order-{}-{unique}", + std::process::id() + )); + let user_root_path = parent.join("user-root"); + let system_root_path = parent.join("system-root"); + for (root, name) in [ + (&user_root_path, "user-skill"), + (&system_root_path, "system-skill"), + ] { + let skill_dir = root.join(name); + std::fs::create_dir_all(&skill_dir)?; + std::fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {name}\n---\n"), + )?; + } + let user_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&user_root_path)?)?; + let system_root = AbsolutePathBuf::try_from(std::fs::canonicalize(&system_root_path)?)?; + let outcome = load_skills_from_roots( + [ + SkillRoot { + path: user_root.clone(), + scope: SkillScope::User, + file_system: Arc::clone(&LOCAL_FS), + plugin_identity: None, + plugin_namespace: None, + plugin_root: None, + discovery_mode: Default::default(), + }, + SkillRoot { + path: system_root.clone(), + scope: SkillScope::System, + file_system: Arc::clone(&LOCAL_FS), + plugin_identity: None, + plugin_namespace: None, + plugin_root: None, + discovery_mode: Default::default(), + }, + ], + /*plugin_skill_snapshots*/ None, + Arc::new(Semaphore::new(2)), + ) + .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 mut entries = catalog.entries.iter().collect::>(); + SkillCatalogRenderPolicy::CoreCompatible.order_entries(&mut entries); + let actual = build_alias_plan(&entries, SkillMetadataBudget::Characters(usize::MAX)) + .expect("alias plan should build") + .skill_root_lines; + std::fs::remove_dir_all(parent)?; + + assert_eq!( + actual, + vec![ + format!( + "- `r0` = `{}`", + user_root.to_string_lossy().replace('\\', "/") + ), + format!( + "- `r1` = `{}`", + system_root.to_string_lossy().replace('\\', "/") + ), + ] + ); + Ok(()) +} + #[test] fn mixed_catalogs_keep_absolute_authority_aware_rendering_under_budget_pressure() { let root = "/Users/test/.codex/plugins/cache/openai-curated/example/hash1234567890/skills-with-a-very-long-shared-prefix";