diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 6c85b77c80..8f41fb223d 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -174,24 +174,23 @@ fn emit_skill_injected_metric( /// /// Structured `UserInput::Skill` selections are resolved first by path against /// enabled skills. Text inputs are then scanned to extract `$skill-name` tokens, and we -/// iterate `skills` in their existing order to preserve prior ordering semantics. -/// Explicit links are resolved by path and plain names are only used when the match -/// is unambiguous. +/// iterate loaded skills in their existing order to preserve prior ordering semantics. +/// Explicit paths match either a skill's canonical identity or its logical discovery +/// path, and plain names are only used when the match is unambiguous. /// /// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where: /// `S` = number of skills, `T` = total text length, `N_s` = number of structured skill inputs, /// `N_t` = number of text inputs, `M` = max mentions parsed from a single text input. pub fn collect_explicit_skill_mentions( inputs: &[UserInput], - skills: &[SkillMetadata], - disabled_paths: &HashSet, + loaded_skills: &SkillLoadOutcome, connector_slug_counts: &HashMap, ) -> Vec { - let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0; + let skill_name_counts = + build_skill_name_counts(&loaded_skills.skills, &loaded_skills.disabled_paths).0; let selection_context = SkillSelectionContext { - skills, - disabled_paths, + loaded_skills, skill_name_counts: &skill_name_counts, connector_slug_counts, }; @@ -207,19 +206,25 @@ pub fn collect_explicit_skill_mentions( continue; }; - if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) { + let Some(skill) = selection_context.loaded_skills.skills.iter().find(|skill| { + skill.path_to_skills_md == path + || selection_context + .loaded_skills + .skill_discovery_path_for_path(&skill.path_to_skills_md) + .is_some_and(|discovery_path| discovery_path == &path) + }) else { + continue; + }; + + if !selection_context.loaded_skills.is_skill_enabled(skill) + || seen_paths.contains(&skill.path_to_skills_md) + { continue; } - if let Some(skill) = selection_context - .skills - .iter() - .find(|skill| skill.path_to_skills_md == path) - { - seen_paths.insert(skill.path_to_skills_md.clone()); - seen_names.insert(skill.name.clone()); - selected.push(skill.clone()); - } + seen_paths.insert(skill.path_to_skills_md.clone()); + seen_names.insert(skill.name.clone()); + selected.push(skill.clone()); } } @@ -241,8 +246,7 @@ pub fn collect_explicit_skill_mentions( } struct SkillSelectionContext<'a> { - skills: &'a [SkillMetadata], - disabled_paths: &'a HashSet, + loaded_skills: &'a SkillLoadOutcome, skill_name_counts: &'a HashMap, connector_slug_counts: &'a HashMap, } @@ -400,7 +404,7 @@ fn select_skills_from_mentions( return; } - let mention_skill_paths: HashSet<&str> = mentions + let mention_skill_paths: HashSet = mentions .paths() .filter(|path| { !matches!( @@ -408,30 +412,34 @@ fn select_skills_from_mentions( ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin ) }) - .map(normalize_skill_path) + .map(normalize_host_skill_path) .collect(); - for skill in selection_context.skills { - if selection_context - .disabled_paths - .contains(&skill.path_to_skills_md) + for skill in &selection_context.loaded_skills.skills { + if !selection_context.loaded_skills.is_skill_enabled(skill) || seen_paths.contains(&skill.path_to_skills_md) { continue; } - let path_str = skill.path_to_skills_md.to_string_lossy(); - if mention_skill_paths.contains(path_str.as_ref()) { + let canonical_path = normalize_host_skill_path(&skill.path_to_skills_md.to_string_lossy()); + let matches_discovery_path = selection_context + .loaded_skills + .skill_discovery_path_for_path(&skill.path_to_skills_md) + .is_some_and(|discovery_path| { + mention_skill_paths.contains(&normalize_host_skill_path( + &discovery_path.to_string_lossy(), + )) + }); + if mention_skill_paths.contains(&canonical_path) || matches_discovery_path { seen_paths.insert(skill.path_to_skills_md.clone()); seen_names.insert(skill.name.clone()); selected.push(skill.clone()); } } - for skill in selection_context.skills { - if selection_context - .disabled_paths - .contains(&skill.path_to_skills_md) + for skill in &selection_context.loaded_skills.skills { + if !selection_context.loaded_skills.is_skill_enabled(skill) || seen_paths.contains(&skill.path_to_skills_md) { continue; diff --git a/codex-rs/core-skills/src/injection_tests.rs b/codex-rs/core-skills/src/injection_tests.rs index 01cbb65af9..0716869ca0 100644 --- a/codex-rs/core-skills/src/injection_tests.rs +++ b/codex-rs/core-skills/src/injection_tests.rs @@ -5,6 +5,7 @@ use codex_utils_absolute_path::test_support::test_path_buf; use pretty_assertions::assert_eq; use std::collections::HashMap; use std::collections::HashSet; +use std::sync::Arc; fn make_skill(name: &str, path: &str) -> SkillMetadata { SkillMetadata { @@ -51,7 +52,26 @@ fn collect_mentions( disabled_paths: &HashSet, connector_slug_counts: &HashMap, ) -> Vec { - collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts) + let loaded_skills = SkillLoadOutcome { + skills: skills.to_vec(), + disabled_paths: disabled_paths.clone(), + ..Default::default() + }; + collect_explicit_skill_mentions(inputs, &loaded_skills, connector_slug_counts) +} + +fn skill_outcome_with_discovery_path( + skill: SkillMetadata, + discovery_path: &str, +) -> SkillLoadOutcome { + SkillLoadOutcome { + skill_discovery_path_by_path: Arc::new(HashMap::from([( + skill.path_to_skills_md.clone(), + test_path_buf(discovery_path).abs(), + )])), + skills: vec![skill], + ..Default::default() + } } #[test] @@ -180,6 +200,61 @@ fn collect_explicit_skill_mentions_prioritizes_structured_inputs() { assert_eq!(selected, vec![beta, alpha]); } +#[test] +fn collect_explicit_skill_mentions_accepts_structured_discovery_path() { + let skill = make_skill("linked-skill", "/tmp/shared/linked-skill/SKILL.md"); + let loaded_skills = skill_outcome_with_discovery_path( + skill.clone(), + "/tmp/project/.agents/skills/linked-skill/SKILL.md", + ); + let inputs = vec![UserInput::Skill { + name: "linked-skill".to_string(), + path: test_path_buf("/tmp/project/.agents/skills/linked-skill/SKILL.md"), + }]; + + let selected = collect_explicit_skill_mentions(&inputs, &loaded_skills, &HashMap::new()); + + assert_eq!(selected, vec![skill]); +} + +#[test] +fn collect_explicit_skill_mentions_accepts_linked_discovery_path() { + let skill = make_skill("linked-skill", "/tmp/shared/linked-skill/SKILL.md"); + let loaded_skills = skill_outcome_with_discovery_path( + skill.clone(), + "/tmp/project/.agents/skills/linked-skill/SKILL.md", + ); + let inputs = vec![UserInput::Text { + text: linked_skill_mention( + "linked-skill", + "/tmp/project/.agents/skills/linked-skill/SKILL.md", + ), + text_elements: Vec::new(), + }]; + + let selected = collect_explicit_skill_mentions(&inputs, &loaded_skills, &HashMap::new()); + + assert_eq!(selected, vec![skill]); +} + +#[test] +fn collect_explicit_skill_mentions_rejects_disabled_discovery_path() { + let skill = make_skill("linked-skill", "/tmp/shared/linked-skill/SKILL.md"); + let mut loaded_skills = skill_outcome_with_discovery_path( + skill.clone(), + "/tmp/project/.agents/skills/linked-skill/SKILL.md", + ); + loaded_skills.disabled_paths.insert(skill.path_to_skills_md); + let inputs = vec![UserInput::Skill { + name: "linked-skill".to_string(), + path: test_path_buf("/tmp/project/.agents/skills/linked-skill/SKILL.md"), + }]; + + let selected = collect_explicit_skill_mentions(&inputs, &loaded_skills, &HashMap::new()); + + assert_eq!(selected, Vec::new()); +} + #[test] fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fallback() { let alpha = make_skill("alpha-skill", "/tmp/alpha"); diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 36e4fe5690..85117527a5 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -39,6 +39,7 @@ use futures::FutureExt; use futures::StreamExt; use namespace::SkillNamespaceResolver; use serde::Deserialize; +use std::collections::HashMap; use std::error::Error; use std::fmt; use std::io; @@ -165,6 +166,7 @@ pub(crate) struct SkillRootSnapshot { pub(crate) root: AbsolutePathBuf, pub(crate) is_agent_plugin: bool, pub(crate) skills: Vec, + pub(crate) skill_discovery_path_by_path: Arc>, pub(crate) errors: Vec, pub(crate) file_system: Arc, } @@ -178,6 +180,7 @@ pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { root: canonical_root, is_agent_plugin: root.discovery_mode == SkillDiscoveryMode::DirectChildren, skills: outcome.skills, + skill_discovery_path_by_path: outcome.skill_discovery_path_by_path, errors: outcome.errors, file_system: root.file_system, } @@ -324,6 +327,11 @@ async fn load_skills_under_root( .map(|skill| { let plugin_root = plugin_root.as_ref(); async move { + let discovery_path = skill + .skill + .path + .to_abs_path() + .unwrap_or_else(|_| skill.path.clone()); let result = parse_skill_file( fs, &skill.skill, @@ -335,14 +343,14 @@ async fn load_skills_under_root( ) .await .map_err(|err| err.to_string()); - (skill.path, skill.path_uri, result) + (skill.path, skill.path_uri, discovery_path, result) } }) .buffered(MAX_CONCURRENT_SKILL_LOADS) .collect::>() .boxed(); let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results); - for (path, path_uri, result) in skill_results { + for (path, path_uri, discovery_path, result) in skill_results { let result = result.and_then(|mut skill| { skill.name = namespace_resolver .for_skill(&root_uri, &path_uri) @@ -352,7 +360,11 @@ async fn load_skills_under_root( Ok(skill) }); match result { - Ok(skill) => outcome.skills.push(skill), + Ok(skill) => { + Arc::make_mut(&mut outcome.skill_discovery_path_by_path) + .insert(skill.path_to_skills_md.clone(), discovery_path); + outcome.skills.push(skill); + } Err(err) if skill_root.scope != SkillScope::System => { outcome.errors.push(SkillError { path, message: err }) } diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 93ebe41e8a..9f09cbfdf6 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -623,6 +623,21 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() { remote_plugin_id: None, }] ); + let canonical_skill_path = normalized(&shared_skill_path); + let discovery_path = outcome + .skill_root_for_path(&canonical_skill_path) + .expect("symlinked skill should retain its discovery root") + .join("shared/demo/SKILL.md"); + assert_eq!( + outcome.skill_discovery_path_for_path(&canonical_skill_path), + Some(&discovery_path) + ); + let filtered_outcome = + crate::filter_skill_load_outcome_for_product(outcome, Some(Product::Codex)); + assert_eq!( + filtered_outcome.skill_discovery_path_for_path(&canonical_skill_path), + Some(&discovery_path) + ); } // Directory symlinks on Windows can require Developer Mode or administrator privileges. @@ -795,6 +810,15 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() { remote_plugin_id: None, }] ); + let canonical_skill_path = normalized(&linked_skill_path); + let discovery_path = outcome + .skill_root_for_path(&canonical_skill_path) + .expect("repo skill should retain its discovery root") + .join("shared/demo/SKILL.md"); + assert_eq!( + outcome.skill_discovery_path_for_path(&canonical_skill_path), + Some(&discovery_path) + ); } #[tokio::test] @@ -1880,6 +1904,41 @@ async fn deduplicates_by_path_preferring_first_root() { ); } +#[cfg(unix)] +#[tokio::test] +async fn deduplicated_symlinked_skill_preserves_first_discovery_path() { + let source_root = tempfile::tempdir().expect("source tempdir"); + let first_root = tempfile::tempdir().expect("first tempdir"); + let second_root = tempfile::tempdir().expect("second tempdir"); + let skill_path = write_skill_at(source_root.path(), "demo", "demo", "shared skill"); + symlink_dir( + &source_root.path().join("demo"), + &first_root.path().join("first-link"), + ); + symlink_dir( + &source_root.path().join("demo"), + &second_root.path().join("second-link"), + ); + + let outcome = load_skills_for_test([ + local_skill_root(first_root.path(), SkillScope::Repo), + local_skill_root(second_root.path(), SkillScope::User), + ]) + .await; + let canonical_skill_path = normalized(&skill_path); + let expected_root = normalized(first_root.path()); + let expected_discovery_path = expected_root.join("first-link/SKILL.md"); + + assert_eq!(outcome.skills.len(), 1); + assert_eq!( + ( + outcome.skill_root_for_path(&canonical_skill_path), + outcome.skill_discovery_path_for_path(&canonical_skill_path), + ), + (Some(&expected_root), Some(&expected_discovery_path)) + ); +} + #[tokio::test] async fn keeps_duplicate_names_from_repo_and_user() { let codex_home = tempfile::tempdir().expect("tempdir"); diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 789827f7b0..2b5d212f04 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -28,6 +28,7 @@ pub struct SkillLoadOutcome { pub disabled_paths: HashSet, pub(crate) skill_roots: Vec, pub(crate) skill_root_by_path: Arc>, + pub(crate) skill_discovery_path_by_path: Arc>, pub(crate) agent_plugin_skill_paths: HashSet, pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, pub(crate) implicit_skills_by_scripts_dir: Arc>, @@ -81,6 +82,14 @@ impl SkillLoadOutcome { self.skill_root_by_path.get(path) } + /// Returns the logical path used to discover a canonical skill path. + pub fn skill_discovery_path_for_path( + &self, + path: &AbsolutePathBuf, + ) -> Option<&AbsolutePathBuf> { + self.skill_discovery_path_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() @@ -179,6 +188,14 @@ pub fn filter_skill_load_outcome_for_product( .map(|(path, root)| (path.clone(), root.clone())) .collect(), ); + outcome.skill_discovery_path_by_path = Arc::new( + outcome + .skill_discovery_path_by_path + .iter() + .filter(|(path, _)| retained_paths.contains(*path)) + .map(|(path, discovery_path)| (path.clone(), discovery_path.clone())) + .collect(), + ); outcome .agent_plugin_skill_paths .retain(|path| retained_paths.contains(path)); diff --git a/codex-rs/core-skills/src/root_loader.rs b/codex-rs/core-skills/src/root_loader.rs index ca7d059f43..bfd6e4d309 100644 --- a/codex-rs/core-skills/src/root_loader.rs +++ b/codex-rs/core-skills/src/root_loader.rs @@ -145,6 +145,7 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut let mut outcome = SkillLoadOutcome::default(); let mut skill_roots = Vec::new(); let mut skill_root_by_path = HashMap::new(); + let mut skill_discovery_path_by_path = HashMap::new(); let mut file_systems_by_skill_path = HashMap::new(); for snapshot in snapshots { @@ -152,6 +153,7 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut root, is_agent_plugin, skills, + skill_discovery_path_by_path: discovery_paths, errors, file_system, } = snapshot; @@ -162,6 +164,9 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut let path = skill.path_to_skills_md.clone(); if !skill_root_by_path.contains_key(&path) { skill_root_by_path.insert(path.clone(), root.clone()); + if let Some(discovery_path) = discovery_paths.get(&path) { + skill_discovery_path_by_path.insert(path.clone(), discovery_path.clone()); + } file_systems_by_skill_path.insert(path.clone(), Arc::clone(&file_system)); if is_agent_plugin { outcome.agent_plugin_skill_paths.insert(path); @@ -182,6 +187,7 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut .map(|skill| skill.path_to_skills_md.clone()) .collect::>(); skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path)); + skill_discovery_path_by_path.retain(|path, _| retained_skill_paths.contains(path)); let used_roots = skill_root_by_path.values().cloned().collect::>(); skill_roots.retain(|root| used_roots.contains(root)); file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); @@ -190,6 +196,7 @@ fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOut .retain(|path| retained_skill_paths.contains(path)); outcome.skill_roots = skill_roots; outcome.skill_root_by_path = Arc::new(skill_root_by_path); + outcome.skill_discovery_path_by_path = Arc::new(skill_discovery_path_by_path); outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); outcome.skills.sort_by(|a, b| { diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 9d1cec66a4..16ef4e7d36 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -691,12 +691,8 @@ async fn required_mcp_servers_for_input( }; let skills_snapshot = turn_context.skills_snapshot(); let skills_outcome = skills_snapshot.outcome(); - let mentioned_skills = collect_explicit_skill_mentions( - user_input, - &skills_outcome.skills, - &skills_outcome.disabled_paths, - &connector_slug_counts, - ); + let mentioned_skills = + collect_explicit_skill_mentions(user_input, skills_outcome, &connector_slug_counts); for skill in mentioned_skills { if let Some(dependencies) = skill.dependencies { required_servers.extend( @@ -771,12 +767,8 @@ async fn build_skills_and_plugins( .await?; let skill_name_counts_lower = build_skill_name_counts(&skills_outcome.skills, &skills_outcome.disabled_paths).1; - let mentioned_skills = collect_explicit_skill_mentions( - user_input, - &skills_outcome.skills, - &skills_outcome.disabled_paths, - &connector_slug_counts, - ); + let mentioned_skills = + collect_explicit_skill_mentions(user_input, skills_outcome, &connector_slug_counts); maybe_prompt_and_install_mcp_dependencies( sess, turn_context, diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index 84407b5b86..35b19036b7 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -3,14 +3,19 @@ use anyhow::Result; use codex_core::TurnInput; +use codex_core::config::Config; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; +use codex_extension_api::ExtensionRegistryBuilder; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Op; use codex_protocol::user_input::UserInput; +use codex_skills_extension::SkillsExtensionConfig; +use codex_skills_extension::install; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use core_test_support::create_directory_symlink; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_response_created; @@ -18,6 +23,7 @@ use core_test_support::responses::mount_sse_once; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_remote; use core_test_support::skip_if_target_windows; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; @@ -135,6 +141,106 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn user_turn_selects_symlinked_skill_by_advertised_discovery_path() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_remote!( + Ok(()), + "remote filesystems do not expose directory symlink creation" + ); + + let server = start_mock_server().await; + let skill_body = "instructions from the canonical linked skill"; + let mut extensions = ExtensionRegistryBuilder::::new(); + install(&mut extensions, |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + bundled_skills_enabled: false, + orchestrator_skills_enabled: false, + shadow_selection_enabled: false, + }); + let mut builder = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_workspace_setup(move |cwd, _fs| async move { + let source_skill_dir = cwd.join("shared-skills/linked-demo"); + let discovery_root = cwd.join(".agents/skills"); + std::fs::create_dir_all(source_skill_dir.as_path())?; + std::fs::create_dir_all(discovery_root.as_path())?; + std::fs::write( + source_skill_dir.join("SKILL.md"), + format!( + "---\nname: linked-demo\ndescription: Linked demo skill\n---\n\n{skill_body}\n" + ), + )?; + create_directory_symlink( + source_skill_dir.as_path(), + discovery_root.join("linked-demo").as_path(), + ); + Ok(()) + }); + let test = builder.build_with_auto_env(&server).await?; + let discovery_root = test.config.cwd.join(".agents/skills").canonicalize()?; + let discovery_path = discovery_root.join("linked-demo/SKILL.md"); + let canonical_path = discovery_path.canonicalize()?; + let discovery_path_display = discovery_path.display(); + let canonical_path_display = canonical_path.display(); + let mock = mount_sse_once( + &server, + sse(vec![ + ev_response_created("linked-skill-response"), + ev_assistant_message("linked-skill-message", "done"), + ev_completed("linked-skill-response"), + ]), + ) + .await; + + test.codex + .try_start_turn_if_idle(vec![TurnInput::UserInput { + content: vec![ + UserInput::Text { + text: format!("please use [$linked-demo]({discovery_path_display})"), + text_elements: Vec::new(), + }, + UserInput::Skill { + name: "linked-demo".to_string(), + path: discovery_path.to_path_buf(), + }, + ], + client_id: Some("linked-skill-user-message".to_string()), + }]) + .await + .map_err(|error| { + anyhow::anyhow!("linked skill input was rejected: {:?}", error.reason()) + })?; + + core_test_support::wait_for_event(test.codex.as_ref(), |event| { + matches!(event, codex_protocol::protocol::EventMsg::TurnComplete(_)) + }) + .await; + + let request = mock.single_request(); + let developer_texts = request.message_input_texts("developer"); + let advertised_path = format!("(file: {discovery_path_display})"); + assert!( + developer_texts + .iter() + .any(|text| text.contains(&advertised_path)), + "expected symlink discovery path in the skill catalog, got {developer_texts:?}" + ); + + let user_texts = request.message_input_texts("user"); + let canonical_identity = format!("{canonical_path_display}"); + assert!( + user_texts.iter().any(|text| { + text.contains("\nlinked-demo") + && text.contains(&canonical_identity) + && text.contains(skill_body) + }), + "expected canonical skill instructions selected by discovery path, got {user_texts:?}" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn idle_user_turn_includes_skill_instructions_in_the_first_request() -> Result<()> { skip_if_target_windows!(Ok(()), "requires native cross-OS skill paths"); diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 7e391e090b..6bdbbf5421 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -106,6 +106,11 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { for (skill, enabled) in outcome.skills_with_enabled() { let mut entry = catalog_entry_from_skill(skill, enabled); + if let Some(discovery_path) = + outcome.skill_discovery_path_for_path(&skill.path_to_skills_md) + { + 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('\\', "/")); if let Some(root_order) = root_order_by_path.get(root.as_path()) { diff --git a/codex-rs/ext/skills/src/provider/host_tests.rs b/codex-rs/ext/skills/src/provider/host_tests.rs index 302cf68de7..0d9b0b0cc2 100644 --- a/codex-rs/ext/skills/src/provider/host_tests.rs +++ b/codex-rs/ext/skills/src/provider/host_tests.rs @@ -63,3 +63,53 @@ async fn host_catalog_entries_carry_their_render_metadata() -> Result<(), Box Result<(), Box> { + let root = tempfile::tempdir()?; + let source = tempfile::tempdir()?; + let source_skill_dir = source.path().join("linked-skill"); + std::fs::create_dir_all(&source_skill_dir)?; + std::fs::write( + source_skill_dir.join("SKILL.md"), + "---\nname: linked-skill\ndescription: Linked skill.\n---\n# Linked skill\n", + )?; + std::os::unix::fs::symlink(&source_skill_dir, root.path().join("linked-skill"))?; + + let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root.path())?)?; + let outcome = load_skills_from_roots( + [SkillRoot { + path: root.clone(), + scope: SkillScope::User, + 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(1)), + ) + .await; + let catalog = catalog_from_outcome(&outcome); + let canonical_path = std::fs::canonicalize(source_skill_dir.join("SKILL.md"))?; + let discovery_path = root.join("linked-skill/SKILL.md"); + + assert_eq!(catalog.entries.len(), 1); + assert_eq!( + ( + catalog.entries[0].main_prompt.as_str(), + catalog.entries[0].display_path.as_deref(), + catalog.entries[0].display_path_root(), + ), + ( + canonical_path.to_string_lossy().as_ref(), + Some(discovery_path.to_string_lossy().as_ref()), + Some(root.to_string_lossy().as_ref()), + ) + ); + + Ok(()) +} diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index 2253983176..cc36f5aa81 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -1615,6 +1615,16 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul ), )?; } + #[cfg(unix)] + { + let source_skill_dir = test_root.join("shared-skills/linked-skill"); + std::fs::create_dir_all(&source_skill_dir)?; + std::fs::write( + source_skill_dir.join("SKILL.md"), + "---\nname: linked-skill\ndescription: Linked skill.\n---\n# Linked skill\n", + )?; + std::os::unix::fs::symlink(&source_skill_dir, root.join("linked-skill"))?; + } let root = AbsolutePathBuf::try_from(std::fs::canonicalize(root)?)?; let rendered_root = root.to_string_lossy().replace('\\', "/"); let outcome = load_skills_from_roots( @@ -1632,7 +1642,7 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul ) .await; assert_eq!(outcome.errors, Vec::new()); - assert_eq!(outcome.skills.len(), 12); + assert_eq!(outcome.skills.len(), 12 + usize::from(cfg!(unix))); let mut builder = ExtensionRegistryBuilder::new(); install(&mut builder, skills_extension_config); @@ -1687,6 +1697,8 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul ); assert!(catalog.contains("(file: r0/skill-00/SKILL.md)")); assert!(catalog.contains("(file: r0/skill-11/SKILL.md)")); + #[cfg(unix)] + assert!(catalog.contains("(file: r0/linked-skill/SKILL.md)")); assert!(!catalog.contains("additional skills omitted")); std::fs::remove_dir_all(test_root)?;