Add host skill root loading (#36884)

## What changed

- Add a loader that discovers skills from canonical host roots and preserves their `SkillScope`.
- Skip hidden directories, follow directory symlinks for user, repo, and admin scopes, and ignore them for system skills.
- Load dependencies and policy from optional `agents/openai.yaml` metadata without rejecting a skill when that metadata is missing or invalid.
- Resolve plugin and symlink namespaces while retaining host paths and reporting per-skill errors outside system scope.

## Testing

Add coverage for frontmatter and metadata loading, invalid optional metadata, hidden directories, plugin namespaces, and scope-specific symlink discovery.

GitOrigin-RevId: ac436d93b5943b7eaae32143e43367921dd1c350
This commit is contained in:
felixxia-oai
2026-08-04 11:19:27 +00:00
committed by copyberry
parent fd1e4d7a6d
commit 1669c2403f
9 changed files with 578 additions and 6 deletions
+1 -1
View File
@@ -21,6 +21,7 @@ codex-otel = { workspace = true }
codex-protocol = { workspace = true }
codex-skills = { workspace = true }
codex-tools = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-utils-path-uri = { workspace = true }
codex-utils-plugins = { workspace = true }
codex-utils-string = { workspace = true }
@@ -35,7 +36,6 @@ url = { workspace = true }
[dev-dependencies]
codex-models-manager = { workspace = true }
codex-utils-absolute-path = { workspace = true }
codex-utils-cargo-bin = { workspace = true }
insta = { workspace = true }
opentelemetry_sdk = { workspace = true }
+2
View File
@@ -4,6 +4,8 @@ mod config;
mod dynamic_skill_selector;
mod extension;
mod fragments;
// Host loading is staged before its crate-internal runtime caller in this PR stack.
#[allow(dead_code)]
mod loader;
pub mod provider;
mod render;
+42 -2
View File
@@ -16,6 +16,21 @@ use super::SKILLS_METADATA_FILENAME;
const MAX_SKILLS_ENTRIES_PER_ROOT: usize = 20_000;
pub(super) const MAX_CONCURRENT_SKILL_LOADS: usize = 64;
pub(super) enum DirectorySymlinkPolicy {
Follow,
Ignore,
}
pub(super) enum HiddenDirectoryPolicy {
Include,
Skip,
}
pub(super) struct SkillDiscoveryOptions {
pub directory_symlinks: DirectorySymlinkPolicy,
pub hidden_directories: HiddenDirectoryPolicy,
}
pub(super) struct SkillDiscovery {
pub skills: Vec<DiscoveredSkill>,
pub plugin_roots: HashSet<PathUri>,
@@ -37,6 +52,7 @@ pub(super) enum SkillMetadataDiscovery {
pub(super) async fn discover_skills(
file_system: &dyn ExecutorFileSystem,
root: &PathUri,
options: SkillDiscoveryOptions,
) -> SkillDiscovery {
let empty_discovery = || SkillDiscovery {
skills: Vec::new(),
@@ -51,8 +67,14 @@ pub(super) async fn discover_skills(
max_depth: MAX_SCAN_DEPTH,
max_directories: MAX_SKILLS_DIRS_PER_ROOT,
max_entries: MAX_SKILLS_ENTRIES_PER_ROOT,
follow_directory_symlinks: true,
prune_hidden_directories: false,
follow_directory_symlinks: matches!(
options.directory_symlinks,
DirectorySymlinkPolicy::Follow
),
prune_hidden_directories: matches!(
options.hidden_directories,
HiddenDirectoryPolicy::Skip
),
},
/*sandbox*/ None,
)
@@ -86,11 +108,15 @@ pub(super) async fn discover_skills(
));
}
let skip_hidden = matches!(options.hidden_directories, HiddenDirectoryPolicy::Skip);
let mut skill_files = Vec::new();
let mut file_paths = HashSet::new();
let mut metadata_directory_parents = HashSet::new();
let mut plugin_roots = HashSet::new();
for entry in walk.entries {
if skip_hidden && has_hidden_ancestor_below_root(&entry.path, root) {
continue;
}
match entry.kind {
WalkEntryKind::Directory => {
if entry
@@ -138,6 +164,20 @@ pub(super) async fn discover_skills(
}
}
fn has_hidden_ancestor_below_root(path: &PathUri, root: &PathUri) -> bool {
let mut ancestor = path.parent();
while let Some(current) = ancestor {
if &current == root {
return false;
}
if current.basename().is_some_and(|name| name.starts_with('.')) {
return true;
}
ancestor = current.parent();
}
false
}
fn discover_skill_metadata(
skill_path: &PathUri,
file_paths: &HashSet<PathUri>,
+12 -1
View File
@@ -13,8 +13,11 @@ use codex_utils_path_uri::PathUri;
use futures::StreamExt;
use super::MAX_QUALIFIED_NAME_LEN;
use super::discovery::DirectorySymlinkPolicy;
use super::discovery::DiscoveredSkill;
use super::discovery::HiddenDirectoryPolicy;
use super::discovery::MAX_CONCURRENT_SKILL_LOADS;
use super::discovery::SkillDiscoveryOptions;
use super::discovery::SkillMetadataDiscovery;
use super::discovery::discover_skills;
use super::metadata::SkillMetadataFile;
@@ -111,7 +114,15 @@ pub async fn load_environment_skills_from_root(
let mut outcome = EnvironmentSkillLoadOutcome::default();
// Preserve environment discovery behavior by following directory aliases and including
// hidden directories exposed by the executor.
let discovery = discover_skills(file_system, root).await;
let discovery = discover_skills(
file_system,
root,
SkillDiscoveryOptions {
directory_symlinks: DirectorySymlinkPolicy::Follow,
hidden_directories: HiddenDirectoryPolicy::Include,
},
)
.await;
tracing::Span::current().record("skill_count", discovery.skills.len());
outcome.warnings.extend(discovery.warnings);
if discovery.skills.is_empty() {
+251
View File
@@ -0,0 +1,251 @@
use std::sync::Arc;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::protocol::SkillScope;
use codex_skills::ParsedSkillFrontmatter;
use codex_skills::SkillMetadata;
use codex_skills::parse_skill_frontmatter_metadata;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use futures::StreamExt;
use tracing::error;
use super::MAX_QUALIFIED_NAME_LEN;
use super::SKILLS_METADATA_DIR;
use super::SKILLS_METADATA_FILENAME;
use super::discovery::DirectorySymlinkPolicy;
use super::discovery::DiscoveredSkill;
use super::discovery::HiddenDirectoryPolicy;
use super::discovery::MAX_CONCURRENT_SKILL_LOADS;
use super::discovery::SkillDiscovery;
use super::discovery::SkillDiscoveryOptions;
use super::discovery::SkillMetadataDiscovery;
use super::discovery::discover_skills;
use super::metadata::LoadedSkillMetadata;
use super::metadata::load_host_skill_metadata;
use super::metadata::sanitize_single_line;
use super::metadata::validate_len;
use super::namespace::SkillNamespaceResolver;
/// A resolved host skill root ready for filesystem discovery.
pub struct HostSkillRoot {
pub path: AbsolutePathBuf,
pub scope: SkillScope,
pub file_system: Arc<dyn ExecutorFileSystem>,
}
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct HostSkillError {
pub path: AbsolutePathBuf,
pub message: String,
}
/// Skills and errors loaded from one canonical host root.
#[derive(Clone)]
pub struct HostSkillRootSnapshot {
pub root: AbsolutePathBuf,
pub skills: Vec<SkillMetadata>,
pub errors: Vec<HostSkillError>,
pub file_system: Arc<dyn ExecutorFileSystem>,
}
struct ResolvedDiscoveredSkill {
skill: DiscoveredSkill,
path: AbsolutePathBuf,
path_uri: PathUri,
}
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;
HostSkillRootSnapshot {
root: canonical_root,
skills,
errors,
file_system: root.file_system,
}
}
async fn load_skills_under_root(
skill_root: &HostSkillRoot,
root: &AbsolutePathBuf,
) -> (Vec<SkillMetadata>, Vec<HostSkillError>) {
let file_system = skill_root.file_system.as_ref();
let directory_symlinks = match skill_root.scope {
SkillScope::User | SkillScope::Repo | SkillScope::Admin => DirectorySymlinkPolicy::Follow,
SkillScope::System => DirectorySymlinkPolicy::Ignore,
};
let SkillDiscovery {
skills,
plugin_roots,
mut namespace_roots,
warnings,
} = discover_skills(
file_system,
&PathUri::from_abs_path(root),
SkillDiscoveryOptions {
directory_symlinks,
hidden_directories: HiddenDirectoryPolicy::Skip,
},
)
.await;
for warning in warnings {
error!("{warning}");
}
if skills.is_empty() {
return (Vec::new(), Vec::new());
}
let root_uri = PathUri::from_abs_path(root);
let resolved_skills = futures::stream::iter(skills)
.map(|skill| async move {
let path_uri = file_system
.canonicalize(&skill.path, /*sandbox*/ None)
.await
.unwrap_or_else(|_| skill.path.clone());
let path = match path_uri.to_abs_path() {
Ok(path) => path,
Err(error) => {
error!("failed to convert discovered skill path {path_uri}: {error}");
return None;
}
};
Some(ResolvedDiscoveredSkill {
skill,
path,
path_uri,
})
})
.buffered(MAX_CONCURRENT_SKILL_LOADS)
.filter_map(futures::future::ready)
.collect::<Vec<_>>()
.await;
namespace_roots.extend(resolved_skills.iter().filter_map(|skill| {
(skill.path_uri != skill.skill.path)
.then(|| skill.path_uri.parent())
.flatten()
}));
let skill_paths = resolved_skills
.iter()
.map(|skill| skill.path_uri.clone())
.collect::<Vec<_>>();
let namespace_resolver = SkillNamespaceResolver::discover(
file_system,
&root_uri,
&skill_paths,
plugin_roots,
namespace_roots,
);
let skill_results = futures::stream::iter(resolved_skills)
.map(|skill| async move {
let result = parse_skill_file(
file_system,
&skill.skill,
&skill.path,
&skill.path_uri,
skill_root.scope,
)
.await;
(skill.path, skill.path_uri, result)
})
.buffered(MAX_CONCURRENT_SKILL_LOADS)
.collect::<Vec<_>>();
let (namespace_resolver, skill_results) = tokio::join!(namespace_resolver, skill_results);
let mut loaded_skills = Vec::new();
let mut errors = Vec::new();
for (path, path_uri, result) in skill_results {
let result = result.and_then(|mut skill| {
skill.name = namespace_resolver
.for_skill(&root_uri, &path_uri)
.qualify(&skill.name);
validate_len(&skill.name, MAX_QUALIFIED_NAME_LEN, "qualified name")
.map_err(|error| error.to_string())?;
Ok(skill)
});
match result {
Ok(skill) => loaded_skills.push(skill),
Err(message) if skill_root.scope != SkillScope::System => {
errors.push(HostSkillError { path, message });
}
Err(_) => {}
}
}
(loaded_skills, errors)
}
async fn parse_skill_file(
file_system: &dyn ExecutorFileSystem,
skill: &DiscoveredSkill,
path: &AbsolutePathBuf,
path_uri: &PathUri,
scope: SkillScope,
) -> Result<SkillMetadata, String> {
let metadata_path = path_uri
.parent()
.and_then(|parent| parent.join(SKILLS_METADATA_DIR).ok())
.and_then(|directory| directory.join(SKILLS_METADATA_FILENAME).ok());
let metadata = match &skill.metadata {
SkillMetadataDiscovery::Present(_) => metadata_path.map(SkillMetadataDiscovery::Present),
SkillMetadataDiscovery::Probe(_) => metadata_path.map(SkillMetadataDiscovery::Probe),
SkillMetadataDiscovery::Absent => None,
}
.unwrap_or(SkillMetadataDiscovery::Absent);
let (contents, loaded_metadata) = tokio::join!(
file_system.read_file_text(path_uri, /*sandbox*/ None),
load_host_skill_metadata(file_system, &metadata),
);
let contents = contents.map_err(|error| format!("failed to read file: {error}"))?;
let ParsedSkillFrontmatter {
name,
description,
short_description,
} = parse_skill_frontmatter_metadata(&contents, || default_skill_name(path))
.map_err(|error| error.to_string())?;
let LoadedSkillMetadata {
dependencies,
policy,
} = loaded_metadata;
Ok(SkillMetadata {
name,
description,
short_description,
interface: None,
dependencies,
policy,
path_to_skills_md: path.clone(),
scope,
plugin_id: None,
remote_plugin_id: None,
})
}
fn default_skill_name(path: &AbsolutePathBuf) -> String {
path.parent()
.and_then(|parent| {
parent
.file_name()
.and_then(|name| name.to_str())
.map(sanitize_single_line)
})
.filter(|value| !value.is_empty())
.unwrap_or_else(|| "skill".to_string())
}
async fn canonicalize_for_skill_identity(
file_system: &dyn ExecutorFileSystem,
path: &AbsolutePathBuf,
) -> AbsolutePathBuf {
let path_uri = PathUri::from_abs_path(path);
file_system
.canonicalize(&path_uri, /*sandbox*/ None)
.await
.and_then(|path| path.to_abs_path())
.unwrap_or_else(|_| path.clone())
}
#[cfg(test)]
#[path = "host_tests.rs"]
mod tests;
@@ -0,0 +1,193 @@
use std::fs;
use std::sync::Arc;
use codex_exec_server::LOCAL_FS;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
use codex_skills::SkillDependencies;
use codex_skills::SkillMetadata;
use codex_skills::SkillPolicy;
use codex_skills::SkillToolDependency;
use codex_utils_absolute_path::AbsolutePathBuf;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
use super::HostSkillRoot;
use super::load_host_skill_root;
fn write_skill(root: &TempDir, directory: &str, frontmatter: &str) -> AbsolutePathBuf {
let skill_dir = root.path().join(directory);
fs::create_dir_all(&skill_dir).expect("create skill directory");
let skill_path = skill_dir.join("SKILL.md");
fs::write(
&skill_path,
format!("---\n{frontmatter}\n---\n\n# Instructions\n"),
)
.expect("write skill");
AbsolutePathBuf::from_absolute_path(fs::canonicalize(skill_path).expect("canonical skill path"))
.expect("absolute skill path")
}
fn write_metadata(root: &TempDir, directory: &str, contents: &str) {
let metadata_dir = root.path().join(directory).join("agents");
fs::create_dir_all(&metadata_dir).expect("create metadata directory");
fs::write(metadata_dir.join("openai.yaml"), contents).expect("write metadata");
}
fn root_for(temp_dir: &TempDir, scope: SkillScope) -> HostSkillRoot {
HostSkillRoot {
path: AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute root"),
scope,
file_system: Arc::clone(&LOCAL_FS),
}
}
#[tokio::test]
async fn loads_host_frontmatter_dependencies_and_policy() {
let root = TempDir::new().expect("temp dir");
let skill_path = write_skill(
&root,
"demo",
"name: demo\ndescription: Demo skill\nmetadata:\n short-description: Short demo",
);
write_metadata(
&root,
"demo",
r##"dependencies:
tools:
- type: mcp
value: demo-tool
description: Demo tool
policy:
allow_implicit_invocation: false
products: [codex]
"##,
);
let snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await;
assert_eq!(snapshot.errors, Vec::new());
assert_eq!(
snapshot.skills,
vec![SkillMetadata {
name: "demo".to_string(),
description: "Demo skill".to_string(),
short_description: Some("Short demo".to_string()),
interface: None,
dependencies: Some(SkillDependencies {
tools: vec![SkillToolDependency {
r#type: "mcp".to_string(),
value: "demo-tool".to_string(),
description: Some("Demo tool".to_string()),
transport: None,
command: None,
url: None,
}],
}),
policy: Some(SkillPolicy {
allow_implicit_invocation: Some(false),
products: vec![Product::Codex],
}),
path_to_skills_md: skill_path,
scope: SkillScope::User,
plugin_id: None,
remote_plugin_id: None,
}]
);
}
#[tokio::test]
async fn invalid_optional_metadata_fails_open() {
let root = TempDir::new().expect("temp dir");
let skill_path = write_skill(&root, "demo", "name: demo\ndescription: Demo skill");
write_metadata(&root, "demo", "interface: [not-an-interface]");
let snapshot = load_host_skill_root(root_for(&root, SkillScope::Repo)).await;
assert_eq!(snapshot.errors, Vec::new());
assert_eq!(
snapshot.skills,
vec![SkillMetadata {
name: "demo".to_string(),
description: "Demo skill".to_string(),
short_description: None,
interface: None,
dependencies: None,
policy: None,
path_to_skills_md: skill_path,
scope: SkillScope::Repo,
plugin_id: None,
remote_plugin_id: None,
}]
);
}
#[tokio::test]
async fn skips_hidden_host_skills() {
let root = TempDir::new().expect("temp dir");
let visible_path = write_skill(
&root,
"visible",
"name: visible\ndescription: Visible skill",
);
write_skill(&root, ".hidden", "name: hidden\ndescription: Hidden skill");
let snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await;
assert_eq!(snapshot.errors, Vec::new());
assert_eq!(snapshot.skills.len(), 1);
assert_eq!(snapshot.skills[0].path_to_skills_md, visible_path);
}
#[tokio::test]
async fn discovers_nested_plugin_namespace_without_plugin_identity() {
let root = TempDir::new().expect("temp dir");
let plugin_manifest = root.path().join("nested/.codex-plugin/plugin.json");
fs::create_dir_all(plugin_manifest.parent().expect("plugin manifest parent"))
.expect("create plugin manifest directory");
fs::write(&plugin_manifest, r#"{"name":"plugin-name"}"#).expect("write plugin manifest");
let skill_path = write_skill(
&root,
"nested/skills/demo",
"name: demo\ndescription: Demo skill",
);
let snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await;
assert_eq!(snapshot.errors, Vec::new());
assert_eq!(
snapshot.skills,
vec![SkillMetadata {
name: "plugin-name:demo".to_string(),
description: "Demo skill".to_string(),
short_description: None,
interface: None,
dependencies: None,
policy: None,
path_to_skills_md: skill_path,
scope: SkillScope::User,
plugin_id: None,
remote_plugin_id: None,
}]
);
}
#[cfg(unix)]
#[tokio::test]
async fn follows_directory_symlinks_for_user_but_not_system_scope() {
use std::os::unix::fs::symlink;
let root = TempDir::new().expect("temp dir");
let target = TempDir::new().expect("target temp dir");
let target_skill = write_skill(&target, "demo", "name: demo\ndescription: Symlinked skill");
symlink(target.path().join("demo"), root.path().join("alias")).expect("create symlink");
let user_snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await;
let system_snapshot = load_host_skill_root(root_for(&root, SkillScope::System)).await;
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!(system_snapshot.errors, Vec::new());
assert_eq!(system_snapshot.skills, Vec::new());
}
@@ -1,5 +1,7 @@
use std::io;
use std::path::PathBuf;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::protocol::Product;
use codex_skills::SkillDependencies;
use codex_skills::SkillParseError;
@@ -13,6 +15,8 @@ use super::MAX_DEPENDENCY_TRANSPORT_LEN;
use super::MAX_DEPENDENCY_TYPE_LEN;
use super::MAX_DEPENDENCY_URL_LEN;
use super::MAX_DEPENDENCY_VALUE_LEN;
use super::SKILLS_METADATA_FILENAME;
use super::discovery::SkillMetadataDiscovery;
#[derive(Debug, Default, Deserialize)]
pub(super) struct SkillMetadataFile {
@@ -42,6 +46,12 @@ struct Interface {
_default_prompt: Option<String>,
}
#[derive(Default)]
pub(super) struct LoadedSkillMetadata {
pub(super) dependencies: Option<SkillDependencies>,
pub(super) policy: Option<SkillPolicy>,
}
#[derive(Debug, Default, Deserialize)]
pub(super) struct Dependencies {
#[serde(default)]
@@ -67,6 +77,68 @@ struct DependencyTool {
url: Option<String>,
}
pub(super) async fn load_host_skill_metadata(
file_system: &dyn ExecutorFileSystem,
metadata: &SkillMetadataDiscovery,
) -> LoadedSkillMetadata {
// Fail open: optional metadata should not block loading SKILL.md.
let metadata_path = match metadata {
SkillMetadataDiscovery::Present(path) => path,
SkillMetadataDiscovery::Absent => return LoadedSkillMetadata::default(),
SkillMetadataDiscovery::Probe(path) => {
match file_system.get_metadata(path, /*sandbox*/ None).await {
Ok(metadata) if metadata.is_file => {}
Ok(_) => return LoadedSkillMetadata::default(),
Err(error) if error.kind() == io::ErrorKind::NotFound => {
return LoadedSkillMetadata::default();
}
Err(error) => {
tracing::warn!(
"ignoring {path}: failed to stat {label}: {error}",
label = SKILLS_METADATA_FILENAME
);
return LoadedSkillMetadata::default();
}
}
path
}
};
let contents = match file_system
.read_file_text(metadata_path, /*sandbox*/ None)
.await
{
Ok(contents) => contents,
Err(error) => {
tracing::warn!(
"ignoring {metadata_path}: failed to read {label}: {error}",
label = SKILLS_METADATA_FILENAME
);
return LoadedSkillMetadata::default();
}
};
let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) {
Ok(parsed) => parsed,
Err(error) => {
tracing::warn!(
"ignoring {metadata_path}: invalid {label}: {error}",
label = SKILLS_METADATA_FILENAME
);
return LoadedSkillMetadata::default();
}
};
let SkillMetadataFile {
_interface: _,
dependencies,
policy,
} = parsed;
LoadedSkillMetadata {
dependencies: resolve_dependencies(dependencies),
policy: resolve_policy(policy),
}
}
pub(super) fn resolve_dependencies(
dependencies: Option<Dependencies>,
) -> Option<SkillDependencies> {
+1
View File
@@ -1,5 +1,6 @@
mod discovery;
mod environment;
mod host;
mod metadata;
mod namespace;
+4 -2
View File
@@ -18,8 +18,10 @@ use super::discovery::MAX_CONCURRENT_SKILL_LOADS;
/// ancestor manifest probes for sibling skills. This resolver resolves relevant
/// roots once per scan, then selects the nearest matching root for each skill.
///
/// The deepest matching canonical symlink root or nested plugin root wins,
/// followed by the namespace inherited from the scanned skills root.
/// Namespace precedence is:
///
/// 1. the deepest matching canonical symlink root or nested plugin root;
/// 2. the namespace inherited from the scanned skills root.
pub(crate) struct SkillNamespaceResolver {
inherited_namespace: ResolvedSkillNamespace,
nested_namespaces: Vec<(PathUri, ResolvedSkillNamespace)>,