diff --git a/codex-rs/core-skills/src/lib.rs b/codex-rs/core-skills/src/lib.rs index 8a0bb5fc39..2d977583fe 100644 --- a/codex-rs/core-skills/src/lib.rs +++ b/codex-rs/core-skills/src/lib.rs @@ -17,7 +17,6 @@ pub const MAX_SKILL_PROMPT_BYTES: usize = 8_000; pub(crate) use invocation_utils::build_implicit_skill_path_indexes; pub use invocation_utils::detect_implicit_skill_invocation_for_command; pub use mention_counts::build_skill_name_counts; -pub use model::HostSkillsSnapshot; pub use model::SkillError; pub use model::SkillLoadOutcome; pub use model::SkillMetadata; diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 85117527a5..2a84238358 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -2,6 +2,7 @@ mod discovery; mod environment; mod namespace; +pub use crate::root_loader::load_skill_root_snapshot; pub use environment::EnvironmentSkillLoadOutcome; pub use environment::EnvironmentSkillMetadata; pub use environment::load_environment_skills_from_root; @@ -161,8 +162,12 @@ where .await } +/// One loaded skill root and the filesystem that supplied it. +/// +/// This is exposed temporarily while host discovery moves out of `core-skills`; callers should +/// combine snapshots through [`SkillLoadOutcome::from_root_snapshots`]. #[derive(Clone)] -pub(crate) struct SkillRootSnapshot { +pub struct SkillRootSnapshot { pub(crate) root: AbsolutePathBuf, pub(crate) is_agent_plugin: bool, pub(crate) skills: Vec, @@ -171,6 +176,25 @@ pub(crate) struct SkillRootSnapshot { pub(crate) file_system: Arc, } +impl SkillRootSnapshot { + pub fn new( + root: AbsolutePathBuf, + skills: Vec, + skill_discovery_path_by_path: Arc>, + errors: Vec, + file_system: Arc, + ) -> Self { + Self { + root, + is_agent_plugin: false, + skills, + skill_discovery_path_by_path, + errors, + file_system, + } + } +} + pub(crate) async fn load_skill_root(root: SkillRoot) -> SkillRootSnapshot { let canonical_root = canonicalize_for_skill_identity(root.file_system.as_ref(), &root.path).await; diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 2b5d212f04..e4d982fb8c 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -102,27 +102,18 @@ impl SkillLoadOutcome { self.file_systems_by_skill_path .get(&skill.path_to_skills_md) } -} -/// Immutable snapshot of host-owned skills and the filesystem mapping needed -/// to read each skill through the environment that discovered it. -#[derive(Debug, Clone)] -pub struct HostSkillsSnapshot { - outcome: Arc, -} - -impl HostSkillsSnapshot { - pub fn new(outcome: Arc) -> Self { - Self { outcome } - } - - pub fn outcome(&self) -> &SkillLoadOutcome { - self.outcome.as_ref() + /// Builds the legacy aggregate from independently loaded roots. + /// + /// This is a temporary migration boundary while host root loading moves to the skills + /// extension. It deliberately reuses the existing merge, precedence, and deduplication logic. + pub fn from_root_snapshots(snapshots: Vec) -> Self { + crate::root_loader::merge_skill_root_snapshots(snapshots) } + /// Reads one loaded skill through the filesystem that discovered it. pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result { let fs = self - .outcome .file_system_for_skill(skill) .unwrap_or_else(|| Arc::clone(&LOCAL_FS)); let path = PathUri::from_abs_path(&skill.path_to_skills_md); diff --git a/codex-rs/core-skills/src/root_loader.rs b/codex-rs/core-skills/src/root_loader.rs index bfd6e4d309..5ba4206565 100644 --- a/codex-rs/core-skills/src/root_loader.rs +++ b/codex-rs/core-skills/src/root_loader.rs @@ -71,48 +71,7 @@ where .acquire() .await .unwrap_or_else(|_| unreachable!()); - let cache_key = match ( - root.plugin_identity.clone(), - root.plugin_namespace.clone(), - root.plugin_root.clone(), - ) { - (Some(plugin_identity), Some(plugin_namespace), Some(plugin_root)) => { - Some(PluginSkillRoot { - path: root.path.clone(), - plugin_identity, - plugin_namespace, - plugin_root, - discovery_mode: root.discovery_mode, - }) - } - _ => None, - }; - let cached_snapshot = cache_key.as_ref().and_then(|cache_key| { - let plugin_skill_snapshots = plugin_skill_snapshots?; - plugin_skill_snapshots - .snapshots_by_root - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .get(cache_key) - .cloned() - }); - - let snapshot = match cached_snapshot { - Some(snapshot) => snapshot, - None => { - let snapshot = load_skill_root(root).await; - if let Some(plugin_skill_snapshots) = plugin_skill_snapshots - && let Some(cache_key) = cache_key - { - plugin_skill_snapshots - .snapshots_by_root - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .insert(cache_key, snapshot.clone()); - } - snapshot - } - }; + let snapshot = load_skill_root_snapshot(root, plugin_skill_snapshots).await; (root_index, snapshot) }) // Keep each load's scan queue bounded while avoiding head-of-line blocking. @@ -129,7 +88,59 @@ where merge_skill_root_snapshots(root_snapshots) } -fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOutcome { +/// Loads one root while preserving the existing plugin-loader snapshot reuse. +/// +/// This is a temporary migration boundary for callers that load non-plugin roots elsewhere but +/// still need plugin roots to use the legacy preload cache. +pub async fn load_skill_root_snapshot( + root: SkillRoot, + plugin_skill_snapshots: Option<&PluginSkillSnapshots>, +) -> SkillRootSnapshot { + let cache_key = match ( + root.plugin_identity.clone(), + root.plugin_namespace.clone(), + root.plugin_root.clone(), + ) { + (Some(plugin_identity), Some(plugin_namespace), Some(plugin_root)) => { + Some(PluginSkillRoot { + path: root.path.clone(), + plugin_identity, + plugin_namespace, + plugin_root, + discovery_mode: root.discovery_mode, + }) + } + _ => None, + }; + let cached_snapshot = cache_key.as_ref().and_then(|cache_key| { + let plugin_skill_snapshots = plugin_skill_snapshots?; + plugin_skill_snapshots + .snapshots_by_root + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .get(cache_key) + .cloned() + }); + + match cached_snapshot { + Some(snapshot) => snapshot, + None => { + let snapshot = load_skill_root(root).await; + if let Some(plugin_skill_snapshots) = plugin_skill_snapshots + && let Some(cache_key) = cache_key + { + plugin_skill_snapshots + .snapshots_by_root + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .insert(cache_key, snapshot.clone()); + } + snapshot + } + } +} + +pub(crate) fn merge_skill_root_snapshots(snapshots: Vec) -> SkillLoadOutcome { fn scope_rank(scope: codex_protocol::protocol::SkillScope) -> u8 { use codex_protocol::protocol::SkillScope; diff --git a/codex-rs/core/src/session/turn_context.rs b/codex-rs/core/src/session/turn_context.rs index e3794370b3..b901b4c602 100644 --- a/codex-rs/core/src/session/turn_context.rs +++ b/codex-rs/core/src/session/turn_context.rs @@ -3,7 +3,6 @@ use crate::environment_selection::TurnEnvironmentSnapshot; use crate::shell_snapshot::ShellSnapshotFile; use codex_core_plugins::PluginCommandAttribution; use codex_core_plugins::TrustedPluginRoots; -use codex_core_skills::HostSkillsSnapshot; use codex_file_system::FileSystemSandboxContext; use codex_model_provider::SharedModelProvider; use codex_protocol::SessionId; @@ -15,6 +14,7 @@ use codex_protocol::protocol::MultiAgentVersion; use codex_protocol::protocol::ThreadHistoryMode; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_sandboxing::policy_transforms::effective_permission_profile; +use codex_skills_extension::HostSkillsSnapshot; use codex_utils_path_uri::PathUri; use futures::FutureExt; use futures::future::BoxFuture; diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 4fc1d50768..d90b51140b 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -442,82 +442,6 @@ async fn persisted_remote_plugin_command_attribution_flows_through_turn_context( Ok(()) } -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn capability_sections_render_in_developer_message_in_order() -> Result<()> { - skip_if_no_network!(Ok(())); - let server = start_mock_server().await; - let apps_server = AppsTestServer::mount_with_connector_name(&server, "Google Calendar").await?; - - let resp_mock = mount_sse_once( - &server, - sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), - ) - .await; - - let codex_home = Arc::new(TempDir::new()?); - write_plugin_skill_plugin(codex_home.as_ref()); - write_plugin_app_plugin(codex_home.as_ref()); - let mut builder = test_codex() - .with_home(Arc::clone(&codex_home)) - .with_extensions(skills_extensions()) - .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) - .with_config(move |config| { - config - .features - .enable(Feature::Apps) - .expect("test config should allow feature update"); - config.chatgpt_base_url = apps_server.chatgpt_base_url; - }); - let test_codex = builder.build(&server).await?; - let codex = Arc::clone(&test_codex.codex); - - codex - .submit(Op::UserInput { - items: vec![codex_protocol::user_input::UserInput::Text { - text: "hello".into(), - text_elements: Vec::new(), - }], - final_output_json_schema: None, - responsesapi_client_metadata: None, - additional_context: Default::default(), - thread_settings: Default::default(), - }) - .await?; - - wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; - - let request = resp_mock.single_request(); - let developer_messages = request.message_input_texts("developer"); - let developer_text = developer_messages.join("\n\n"); - let apps_pos = developer_text - .find("## Apps") - .expect("expected apps section in developer message"); - let skills_pos = developer_text - .find("## Skills") - .expect("expected skills section in developer message"); - let plugins_pos = developer_text - .find("## Plugins") - .expect("expected plugins section in developer message"); - assert!( - skills_pos < apps_pos && apps_pos < plugins_pos, - "expected Skills -> Apps -> Plugins order: {developer_messages:?}" - ); - assert!( - !developer_text.contains("`sample`: inspect sample data"), - "did not expect plugin description in developer message: {developer_messages:?}" - ); - assert!( - developer_text.contains("skill entries are prefixed with `plugin_name:`"), - "expected plugin skill naming guidance in developer message: {developer_messages:?}" - ); - assert!( - developer_text.contains("sample:sample-search: inspect sample data"), - "expected namespaced plugin skill summary in developer message: {developer_messages:?}" - ); - - Ok(()) -} - #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn agent_plugin_skills_use_shared_catalog_and_direct_child_discovery() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index afa081ecbc..16f80ad6ff 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -8,10 +8,12 @@ use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; use codex_core::StartThreadOptions; use codex_core::config::Config; +use codex_exec_server::CreateDirectoryOptions; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionWarning; use codex_features::Feature; +use codex_login::CodexAuth; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; @@ -32,12 +34,14 @@ use codex_skills_extension::catalog::SkillReadResult; use codex_skills_extension::catalog::SkillResourceId; use codex_skills_extension::catalog::SkillSearchResult; use codex_skills_extension::catalog::SkillSourceKind; +use codex_skills_extension::install; use codex_skills_extension::install_with_providers; 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_path_uri::PathUri; 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; @@ -47,6 +51,7 @@ use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_mcp_server; use pretty_assertions::assert_eq; @@ -446,6 +451,204 @@ async fn rendered_catalogs_for_turns( Ok((developer_texts, client_warning_messages)) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills() -> Result<()> { + skip_if_wine_exec!( + Ok(()), + "executor-backed repo skills require matching host and executor path conventions" + ); + skip_if_no_network!(Ok(())); + + const HOST_SKILL_BODY: &str = "Use the host skill instructions."; + const REPO_SKILL_BODY: &str = "Use the repository skill instructions."; + const PLUGIN_SKILL_BODY: &str = "Use the legacy plugin skill instructions."; + + let server = responses::start_mock_server().await; + let apps_server = AppsTestServer::mount_with_connector_name(&server, "Google Calendar").await?; + let response = mount_sse_once( + &server, + sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), + ) + .await; + + let codex_home = Arc::new(TempDir::new()?); + let host_skill_dir = codex_home.path().join("skills/host-search"); + std::fs::create_dir_all(&host_skill_dir)?; + let host_skill_path = host_skill_dir.join("SKILL.md"); + std::fs::write( + &host_skill_path, + format!( + "---\nname: host-search\ndescription: inspect host data\n---\n\n{HOST_SKILL_BODY}\n" + ), + )?; + let host_skill_path = dunce::canonicalize(host_skill_path)?; + let plugin_root = codex_home.path().join("plugins/cache/test/sample/local"); + std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?; + std::fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + r#"{"name":"sample","description":"inspect sample data"}"#, + )?; + let plugin_skill_dir = plugin_root.join("skills/sample-search"); + std::fs::create_dir_all(&plugin_skill_dir)?; + let plugin_skill_path = plugin_skill_dir.join("SKILL.md"); + std::fs::write( + &plugin_skill_path, + format!("---\ndescription: inspect sample data\n---\n\n{PLUGIN_SKILL_BODY}\n"), + )?; + let plugin_skill_path = dunce::canonicalize(plugin_skill_path)?; + std::fs::write( + plugin_root.join(".app.json"), + r#"{"apps":{"sample":{"id":"calendar"}}}"#, + )?; + std::fs::write( + codex_home.path().join("config.toml"), + "[features]\nplugins = true\n\n[plugins.\"sample@test\"]\nenabled = true\n", + )?; + + let mut extensions = ExtensionRegistryBuilder::::new(); + install(&mut extensions, |config: &Config| SkillsExtensionConfig { + include_instructions: config.include_skill_instructions, + bundled_skills_enabled: config.bundled_skills_enabled(), + orchestrator_skills_enabled: config.orchestrator_skills_enabled, + shadow_selection_enabled: config.features.enabled(Feature::SkillSearch), + }); + let mut builder = test_codex() + .with_home(Arc::clone(&codex_home)) + .with_extensions(Arc::new(extensions.build())) + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_workspace_setup(|cwd, fs| async move { + let skill_dir = cwd.join(".agents/skills/repo-search"); + fs.create_directory( + &PathUri::from_host_native_path(&skill_dir)?, + CreateDirectoryOptions { recursive: true }, + /*sandbox*/ None, + ) + .await?; + fs.write_file( + &PathUri::from_host_native_path(skill_dir.join("SKILL.md"))?, + format!( + "---\nname: repo-search\ndescription: inspect repo data\n---\n\n{REPO_SKILL_BODY}\n" + ) + .into_bytes(), + /*sandbox*/ None, + ) + .await?; + Ok(()) + }) + .with_config(move |config| { + config + .features + .enable(Feature::Apps) + .expect("test config should allow feature update"); + config.chatgpt_base_url = apps_server.chatgpt_base_url; + }); + let test = builder.build_with_auto_env(&server).await?; + let repo_skill_path = test + .fs() + .canonicalize( + &PathUri::from_abs_path(&test.config.cwd.join(".agents/skills/repo-search/SKILL.md")), + /*sandbox*/ None, + ) + .await? + .to_abs_path()? + .to_path_buf(); + + test.codex + .submit(Op::UserInput { + items: vec![ + UserInput::Text { + text: "use all skills".to_string(), + text_elements: Vec::new(), + }, + UserInput::Skill { + name: "host-search".to_string(), + path: host_skill_path.clone(), + }, + UserInput::Skill { + name: "repo-search".to_string(), + path: repo_skill_path.clone(), + }, + UserInput::Skill { + name: "sample:sample-search".to_string(), + path: plugin_skill_path.clone(), + }, + ], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + + core_test_support::wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let request = response.single_request(); + let developer_messages = request.message_input_texts("developer"); + let developer_text = developer_messages.join("\n\n"); + let apps_pos = developer_text + .find("## Apps") + .expect("expected apps section in developer message"); + let skills_pos = developer_text + .find("## Skills") + .expect("expected skills section in developer message"); + let plugins_pos = developer_text + .find("## Plugins") + .expect("expected plugins section in developer message"); + assert!( + skills_pos < apps_pos && apps_pos < plugins_pos, + "expected Skills -> Apps -> Plugins order: {developer_messages:?}" + ); + assert!( + !developer_text.contains("`sample`: inspect sample data"), + "did not expect plugin description in developer message: {developer_messages:?}" + ); + assert!( + developer_text.contains("skill entries are prefixed with `plugin_name:`"), + "expected plugin skill naming guidance in developer message: {developer_messages:?}" + ); + assert!( + developer_text.contains("sample:sample-search: inspect sample data"), + "expected namespaced plugin skill summary in developer message: {developer_messages:?}" + ); + assert!( + developer_text.contains("repo-search: inspect repo data"), + "expected repo skill summary in developer message: {developer_messages:?}" + ); + assert!( + developer_text.contains("host-search: inspect host data"), + "expected host skill summary in developer message: {developer_messages:?}" + ); + + let user_text = request.message_input_texts("user").join("\n"); + for (name, path, body) in [ + ("host-search", &host_skill_path, HOST_SKILL_BODY), + ("repo-search", &repo_skill_path, REPO_SKILL_BODY), + ( + "sample:sample-search", + &plugin_skill_path, + PLUGIN_SKILL_BODY, + ), + ] { + assert!( + user_text.contains(&format!("\n{name}")), + "expected injected skill `{name}` in user input: {user_text}" + ); + assert!( + user_text.contains(path.to_string_lossy().as_ref()), + "expected path for `{name}` in user input: {user_text}" + ); + assert!( + user_text.contains(body), + "expected body for `{name}` in user input: {user_text}" + ); + } + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn explicit_only_orchestrator_skill_is_hidden_but_can_be_invoked() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card.rs index aab58d32e9..2fb482bd41 100644 --- a/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card.rs +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use codex_core_skills::HostSkillsSnapshot; +use crate::HostSkillsSnapshot; use crate::catalog::SkillCatalog; use crate::catalog::SkillSourceKind; diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index fff087af4a..6ed0b8bc68 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use std::sync::Arc; -use codex_core_skills::HostSkillsSnapshot; +use crate::HostSkillsSnapshot; use codex_core_skills::injection::HostSkillsCatalogInWorldState; use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; diff --git a/codex-rs/ext/skills/src/host_service.rs b/codex-rs/ext/skills/src/host_service.rs index edeb208875..b759e99d33 100644 --- a/codex-rs/ext/skills/src/host_service.rs +++ b/codex-rs/ext/skills/src/host_service.rs @@ -12,6 +12,8 @@ use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_plugins::PluginIdentity; use codex_utils_plugins::PluginSkillRoot; +use codex_utils_plugins::SkillDiscoveryMode; +use futures::StreamExt; use tokio::sync::OnceCell; use tokio::sync::Semaphore; use tracing::info; @@ -19,18 +21,22 @@ use tracing::instrument; use tracing::warn; use codex_config::SkillsConfig; -use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::PluginSkillSnapshots; +use codex_core_skills::SkillError; use codex_core_skills::SkillLoadOutcome; use codex_core_skills::config_rules::SkillConfigRules; use codex_core_skills::config_rules::resolve_disabled_skill_paths; use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_core_skills::loader::SkillRoot; -use codex_core_skills::loader::load_skills_from_roots; +use codex_core_skills::loader::SkillRootSnapshot; +use codex_core_skills::loader::load_skill_root_snapshot; use codex_skills::install_system_skills; +use crate::HostSkillsSnapshot; use crate::host_roots::resolve_skill_roots; +use crate::loader::HostSkillRoot; +use crate::loader::load_host_skill_root; #[derive(Debug, Clone)] pub struct HostSkillsLoadInput { @@ -282,12 +288,55 @@ impl HostSkillsService { roots: Vec, skill_config_rules: &SkillConfigRules, ) -> SkillLoadOutcome { - let outcome = load_skills_from_roots( - roots, - input.plugin_skill_snapshots.as_ref(), - Arc::clone(&self.root_scan_slots), - ) - .await; + let plugin_skill_snapshots = input.plugin_skill_snapshots.as_ref(); + let mut indexed_snapshots = futures::stream::iter(roots.into_iter().enumerate()) + .map(|(root_index, root)| async move { + let _root_scan_slot = self + .root_scan_slots + .acquire() + .await + .unwrap_or_else(|_| unreachable!()); + let use_legacy_loader = root.plugin_identity.is_some() + || root.plugin_namespace.is_some() + || root.plugin_root.is_some() + || root.discovery_mode != SkillDiscoveryMode::Recursive; + let snapshot = if use_legacy_loader { + load_skill_root_snapshot(root, plugin_skill_snapshots).await + } else { + let snapshot = load_host_skill_root(HostSkillRoot { + path: root.path, + scope: root.scope, + file_system: root.file_system, + plugin_root: root.plugin_root, + }) + .await; + SkillRootSnapshot::new( + snapshot.root, + snapshot.skills, + snapshot.skill_discovery_path_by_path, + snapshot + .errors + .into_iter() + .map(|error| SkillError { + path: error.path, + message: error.message, + }) + .collect(), + snapshot.file_system, + ) + }; + (root_index, snapshot) + }) + .buffer_unordered(MAX_CONCURRENT_ROOT_SCANS) + .collect::>() + .await; + indexed_snapshots.sort_unstable_by_key(|(root_index, _)| *root_index); + let outcome = SkillLoadOutcome::from_root_snapshots( + indexed_snapshots + .into_iter() + .map(|(_, snapshot)| snapshot) + .collect(), + ); let outcome = codex_core_skills::filter_skill_load_outcome_for_product( outcome, self.restriction_product, diff --git a/codex-rs/ext/skills/src/host_service_tests.rs b/codex-rs/ext/skills/src/host_service_tests.rs index 047ea00a56..2c5559d7ca 100644 --- a/codex-rs/ext/skills/src/host_service_tests.rs +++ b/codex-rs/ext/skills/src/host_service_tests.rs @@ -183,6 +183,104 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { assert_eq!(outcome2.skills, outcome1.skills); } +#[tokio::test] +async fn snapshot_for_config_merges_extension_host_and_legacy_plugin_roots() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + write_user_skill(&codex_home, "user", "user-skill", "from the host loader"); + let plugin_skill_path = write_plugin_skill( + &codex_home, + "test", + "sample", + "search", + "search", + "from the plugin loader", + ); + let plugin_skill_root = + plugin_skill_root_for_skill_path(&plugin_skill_path, "sample@test", "sample"); + let config_layer_stack = config_stack(&codex_home, "[skills.bundled]\nenabled = false\n"); + let input = HostSkillsLoadInput::new( + cwd.path().abs(), + vec![plugin_skill_root], + config_layer_stack, + /*bundled_skills_enabled*/ false, + ); + let skills_service = HostSkillsService::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ false, + ); + + let snapshot = skills_service + .snapshot_for_config(&input, Some(Arc::clone(&LOCAL_FS))) + .await; + let skills = snapshot + .outcome() + .skills + .iter() + .map(|skill| (skill.name.as_str(), skill.plugin_id.as_deref())) + .collect::>(); + + assert_eq!( + skills, + vec![("sample:search", Some("sample@test")), ("user-skill", None)] + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn snapshot_for_config_preserves_host_precedence_for_symlinked_plugin_root() { + use std::os::unix::fs::symlink; + + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let plugin_skill_path = write_plugin_skill( + &codex_home, + "test", + "sample", + "search", + "search", + "shared skill", + ); + let plugin_skill_root = + plugin_skill_root_for_skill_path(&plugin_skill_path, "sample@test", "sample"); + symlink( + plugin_skill_root.path.as_path(), + codex_home.path().join("skills"), + ) + .expect("symlink user skills root to plugin skills root"); + let config_layer_stack = config_stack(&codex_home, "[skills.bundled]\nenabled = false\n"); + let skills_service = HostSkillsService::new( + codex_home.path().abs(), + /*bundled_skills_enabled*/ false, + ); + + let outcome = skills_for_config_with_stack( + &skills_service, + &cwd, + &config_layer_stack, + &[plugin_skill_root], + ) + .await; + + assert_eq!( + outcome.skills, + vec![codex_skills::SkillMetadata { + name: "sample:search".to_string(), + description: "shared skill".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + path_to_skills_md: dunce::canonicalize(plugin_skill_path) + .expect("canonical plugin skill path") + .abs(), + scope: SkillScope::User, + plugin_id: None, + remote_plugin_id: None, + }] + ); +} + #[tokio::test] async fn skills_for_config_refreshes_cache_when_remote_plugin_id_changes() { let codex_home = tempfile::tempdir().expect("tempdir"); diff --git a/codex-rs/ext/skills/src/host_snapshot.rs b/codex-rs/ext/skills/src/host_snapshot.rs new file mode 100644 index 0000000000..3f44dc3f80 --- /dev/null +++ b/codex-rs/ext/skills/src/host_snapshot.rs @@ -0,0 +1,25 @@ +use std::io; +use std::sync::Arc; + +use codex_core_skills::SkillLoadOutcome; +use codex_skills::SkillMetadata; + +/// Immutable snapshot of host-owned skills and their source filesystems. +#[derive(Debug, Clone)] +pub struct HostSkillsSnapshot { + outcome: Arc, +} + +impl HostSkillsSnapshot { + pub fn new(outcome: Arc) -> Self { + Self { outcome } + } + + pub fn outcome(&self) -> &SkillLoadOutcome { + self.outcome.as_ref() + } + + pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result { + self.outcome.read_skill_text(skill).await + } +} diff --git a/codex-rs/ext/skills/src/lib.rs b/codex-rs/ext/skills/src/lib.rs index 532bb33b40..5416e30d02 100644 --- a/codex-rs/ext/skills/src/lib.rs +++ b/codex-rs/ext/skills/src/lib.rs @@ -6,8 +6,7 @@ mod extension; mod fragments; mod host_roots; mod host_service; -// Host loading is staged before its crate-internal runtime caller in this PR stack. -#[allow(dead_code)] +mod host_snapshot; mod loader; pub mod provider; mod render; @@ -28,6 +27,7 @@ pub use extension::install_with_providers_and_metrics; pub use host_service::HostSkillsLoadInput; pub use host_service::HostSkillsService; pub use host_service::bundled_skills_enabled_from_stack; +pub use host_snapshot::HostSkillsSnapshot; pub use provider::ExecutorSkillProvider; pub use provider::HostSkillProvider; pub use provider::OrchestratorSkillProvider; diff --git a/codex-rs/ext/skills/src/loader/host.rs b/codex-rs/ext/skills/src/loader/host.rs index 9500413ff2..a6c6d0c4f2 100644 --- a/codex-rs/ext/skills/src/loader/host.rs +++ b/codex-rs/ext/skills/src/loader/host.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::sync::Arc; use codex_exec_server::ExecutorFileSystem; @@ -46,6 +47,7 @@ pub struct HostSkillError { pub struct HostSkillRootSnapshot { pub root: AbsolutePathBuf, pub skills: Vec, + pub skill_discovery_path_by_path: Arc>, pub errors: Vec, pub file_system: Arc, } @@ -59,10 +61,12 @@ struct ResolvedDiscoveredSkill { pub async fn load_host_skill_root(root: HostSkillRoot) -> HostSkillRootSnapshot { let canonical_root = canonicalize_for_skill_identity(root.file_system.as_ref(), &root.path).await; - let (skills, errors) = load_skills_under_root(&root, &canonical_root).await; + let (skills, skill_discovery_path_by_path, errors) = + load_skills_under_root(&root, &canonical_root).await; HostSkillRootSnapshot { root: canonical_root, skills, + skill_discovery_path_by_path, errors, file_system: root.file_system, } @@ -71,7 +75,11 @@ pub async fn load_host_skill_root(root: HostSkillRoot) -> HostSkillRootSnapshot async fn load_skills_under_root( skill_root: &HostSkillRoot, root: &AbsolutePathBuf, -) -> (Vec, Vec) { +) -> ( + Vec, + Arc>, + Vec, +) { let file_system = skill_root.file_system.as_ref(); let plugin_root = match skill_root.plugin_root.as_ref() { Some(plugin_root) => Some(canonicalize_for_skill_identity(file_system, plugin_root).await), @@ -99,7 +107,7 @@ async fn load_skills_under_root( error!("{warning}"); } if skills.is_empty() { - return (Vec::new(), Vec::new()); + return (Vec::new(), Arc::default(), Vec::new()); } let root_uri = PathUri::from_abs_path(root); @@ -146,6 +154,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( file_system, &skill.skill, @@ -155,7 +168,7 @@ async fn load_skills_under_root( plugin_root, ) .await; - (skill.path, skill.path_uri, result) + (skill.path, skill.path_uri, discovery_path, result) } }) .buffered(MAX_CONCURRENT_SKILL_LOADS) @@ -163,8 +176,9 @@ async fn load_skills_under_root( let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results); let mut loaded_skills = Vec::new(); + let mut skill_discovery_path_by_path = HashMap::new(); let mut errors = Vec::new(); - 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) @@ -174,14 +188,22 @@ async fn load_skills_under_root( Ok(skill) }); match result { - Ok(skill) => loaded_skills.push(skill), + Ok(skill) => { + skill_discovery_path_by_path + .insert(skill.path_to_skills_md.clone(), discovery_path); + loaded_skills.push(skill); + } Err(message) if skill_root.scope != SkillScope::System => { errors.push(HostSkillError { path, message }); } Err(_) => {} } } - (loaded_skills, errors) + ( + loaded_skills, + Arc::new(skill_discovery_path_by_path), + errors, + ) } async fn parse_skill_file( diff --git a/codex-rs/ext/skills/src/loader/host_tests.rs b/codex-rs/ext/skills/src/loader/host_tests.rs index 8e5fd3be37..b382a6191f 100644 --- a/codex-rs/ext/skills/src/loader/host_tests.rs +++ b/codex-rs/ext/skills/src/loader/host_tests.rs @@ -393,6 +393,12 @@ async fn follows_directory_symlinks_for_user_but_not_system_scope() { assert_eq!(user_snapshot.errors, Vec::new()); assert_eq!(user_snapshot.skills.len(), 1); assert_eq!(user_snapshot.skills[0].path_to_skills_md, target_skill); + assert_eq!( + user_snapshot + .skill_discovery_path_by_path + .get(&target_skill), + Some(&user_snapshot.root.join("alias/SKILL.md")) + ); assert_eq!(system_snapshot.errors, Vec::new()); assert_eq!(system_snapshot.skills, Vec::new()); } diff --git a/codex-rs/ext/skills/src/loader/mod.rs b/codex-rs/ext/skills/src/loader/mod.rs index aa8fb07adb..83f2f8d699 100644 --- a/codex-rs/ext/skills/src/loader/mod.rs +++ b/codex-rs/ext/skills/src/loader/mod.rs @@ -7,6 +7,7 @@ mod namespace; pub(crate) use environment::load_environment_skills_from_discovery; pub(crate) use environment::load_environment_skills_from_root; pub(crate) use host::HostSkillRoot; +pub(crate) use host::load_host_skill_root; pub(super) const SKILLS_FILENAME: &str = "SKILL.md"; pub(super) const SKILLS_METADATA_DIR: &str = "agents"; diff --git a/codex-rs/ext/skills/src/provider.rs b/codex-rs/ext/skills/src/provider.rs index 7a5294560a..cd42bde5e3 100644 --- a/codex-rs/ext/skills/src/provider.rs +++ b/codex-rs/ext/skills/src/provider.rs @@ -6,7 +6,7 @@ mod executor; mod host; mod orchestrator; -use codex_core_skills::HostSkillsSnapshot; +use crate::HostSkillsSnapshot; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::ResolvedSelectedCapabilityRoot; diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index fd14fb70cb..fcc41285b9 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -1,11 +1,11 @@ use super::*; +use crate::HostSkillsSnapshot; 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_exec_server::LOCAL_FS; diff --git a/codex-rs/ext/skills/src/shadow_selection_experiment.rs b/codex-rs/ext/skills/src/shadow_selection_experiment.rs index 1d7d0d592f..b43d377f16 100644 --- a/codex-rs/ext/skills/src/shadow_selection_experiment.rs +++ b/codex-rs/ext/skills/src/shadow_selection_experiment.rs @@ -6,7 +6,7 @@ use std::sync::PoisonError; use std::time::Duration; use std::time::Instant; -use codex_core_skills::HostSkillsSnapshot; +use crate::HostSkillsSnapshot; use codex_otel::MetricsClient; use codex_protocol::user_input::UserInput; 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 e3160b43ed..a53ef44604 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -4,7 +4,6 @@ use std::sync::Arc; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_environment_skills_from_root as load_legacy_environment_skills_from_root; @@ -28,6 +27,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_skills_extension::ExecutorSkillProvider; +use codex_skills_extension::HostSkillsSnapshot; use codex_skills_extension::catalog::SkillAuthority; use codex_skills_extension::catalog::SkillPackageId; use codex_skills_extension::catalog::SkillResourceId; diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index db720ecebb..6dfb938576 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -4,7 +4,6 @@ use std::sync::Mutex; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; -use codex_core_skills::HostSkillsSnapshot; use codex_core_skills::SkillLoadOutcome; use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_core_skills::loader::MAX_CONCURRENT_ROOT_SCANS; @@ -45,6 +44,7 @@ use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::user_input::UserInput; use codex_skills::SkillMetadata; use codex_skills_extension::HostSkillProvider; +use codex_skills_extension::HostSkillsSnapshot; use codex_skills_extension::SkillProviders; use codex_skills_extension::SkillsExtensionConfig; use codex_skills_extension::catalog::SkillAuthority;