mirror of
https://github.com/openai/codex.git
synced 2026-10-25 18:28:38 +00:00
Load host skill roots through the skills extension (#37162)
## What changed - Load ordinary recursive host skill roots with the skills extension's host loader. - Keep plugin-specific roots on the existing loader so plugin snapshot caching and namespacing continue to work. - Merge both kinds of root snapshots through the existing precedence and deduplication logic, including discovery paths and source filesystem mappings. - Move `HostSkillsSnapshot` into the skills extension as the owner of host skill loading. ## Testing - Cover merged host and plugin catalogs, precedence for symlinked roots, discovery paths, and end-to-end rendering and invocation of host, repository, and plugin skills. GitOrigin-RevId: c18080afb917c33c57bc17e36b40dd0f3c72d1d7
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
use std::collections::HashMap;
|
||||
|
||||
use codex_core_skills::HostSkillsSnapshot;
|
||||
use crate::HostSkillsSnapshot;
|
||||
|
||||
use crate::catalog::SkillCatalog;
|
||||
use crate::catalog::SkillSourceKind;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<SkillRoot>,
|
||||
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::<Vec<_>>()
|
||||
.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,
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
|
||||
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");
|
||||
|
||||
@@ -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<SkillLoadOutcome>,
|
||||
}
|
||||
|
||||
impl HostSkillsSnapshot {
|
||||
pub fn new(outcome: Arc<SkillLoadOutcome>) -> Self {
|
||||
Self { outcome }
|
||||
}
|
||||
|
||||
pub fn outcome(&self) -> &SkillLoadOutcome {
|
||||
self.outcome.as_ref()
|
||||
}
|
||||
|
||||
pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result<String> {
|
||||
self.outcome.read_skill_text(skill).await
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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<SkillMetadata>,
|
||||
pub skill_discovery_path_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub errors: Vec<HostSkillError>,
|
||||
pub file_system: Arc<dyn ExecutorFileSystem>,
|
||||
}
|
||||
@@ -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<SkillMetadata>, Vec<HostSkillError>) {
|
||||
) -> (
|
||||
Vec<SkillMetadata>,
|
||||
Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
Vec<HostSkillError>,
|
||||
) {
|
||||
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(
|
||||
|
||||
@@ -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());
|
||||
}
|
||||
|
||||
@@ -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";
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user