github/codex
4
0 Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-13 22:47:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

Move executor skill bundle loading into the skills extension (#36877)

Browse Source 
## What changed

- Move parsing of pre-discovered executor skill bundles from `core-skills` to
  the skills extension.
- Expose shared `SKILL.md` frontmatter parsing from `codex-skills` so direct
  and pre-discovered loading use the same validation and repair behavior.

## Testing

- Add parity coverage for direct and pre-discovered executor skill catalogs,
  including plugin namespaces, metadata, product policy, and warnings.
- Add a snapshot for the resulting pre-discovered executor catalog.

GitOrigin-RevId: c5f888226fa5600bd8b90f5682400da39a5db5ff
This commit is contained in:
felixxia-oai authored and copyberry committed 2026-08-04 10:41:01 +00:00
1 parent 2a16af8234
commit 4c25d6cc5c
17 files changed
+1138 -488
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

codex-rs/core-skills/src/loader.rs
+18 -202
View File
@@ -4,9 +4,6 @@ mod namespace;
pub use environment::EnvironmentSkillLoadOutcome;
pub use environment::EnvironmentSkillMetadata;
pub use environment::EnvironmentSkillSnapshot;
pub use environment::EnvironmentSkillSnapshotOutcome;
pub use environment::load_environment_skills_from_discovery;
pub use environment::load_environment_skills_from_root;
use crate::model::SkillDependencies;
@@ -26,6 +23,9 @@ use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
use codex_skills::ParsedSkillFrontmatter;
use codex_skills::SkillParseError;
use codex_skills::parse_skill_frontmatter_metadata;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::AbsolutePathBufGuard;
use codex_utils_path_uri::PathUri;
@@ -60,22 +60,6 @@ use tracing::error;
// TODO(anp): Tune this eight-scan limit after revisiting byte-based backpressure.
pub const MAX_CONCURRENT_ROOT_SCANS: usize = 8;
#[derive(Debug, Deserialize)]
struct SkillFrontmatter {
#[serde(default)]
name: Option<String>,
#[serde(default)]
description: Option<String>,
#[serde(default)]
metadata: SkillFrontmatterMetadata,
}
#[derive(Debug, Default, Deserialize)]
struct SkillFrontmatterMetadata {
#[serde(default, rename = "short-description")]
short_description: Option<String>,
}
#[derive(Debug, Default, Deserialize)]
struct SkillMetadataFile {
#[serde(default)]
@@ -128,13 +112,6 @@ struct DependencyTool {
url: Option<String>,
}
#[derive(Debug, Clone, PartialEq, Eq)]
struct ParsedSkillFrontmatter {
name: String,
description: String,
short_description: Option<String>,
}
const SKILLS_FILENAME: &str = "SKILL.md";
const AGENTS_DIR_NAME: &str = ".agents";
const SKILLS_METADATA_DIR: &str = "agents";
@@ -165,31 +142,27 @@ struct ResolvedDiscoveredSkill {
}
#[derive(Debug)]
enum SkillParseError {
enum SkillLoadError {
Read(std::io::Error),
MissingFrontmatter,
InvalidYaml(serde_yaml::Error),
MissingField(&'static str),
InvalidField { field: &'static str, reason: String },
Parse(SkillParseError),
}
impl fmt::Display for SkillParseError {
impl fmt::Display for SkillLoadError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
SkillParseError::Read(e) => write!(f, "failed to read file: {e}"),
SkillParseError::MissingFrontmatter => {
write!(f, "missing YAML frontmatter delimited by ---")
}
SkillParseError::InvalidYaml(e) => write!(f, "invalid YAML: {e}"),
SkillParseError::MissingField(field) => write!(f, "missing field `{field}`"),
SkillParseError::InvalidField { field, reason } => {
write!(f, "invalid {field}: {reason}")
}
Self::Read(error) => write!(f, "failed to read file: {error}"),
Self::Parse(error) => write!(f, "{error}"),
}
}
}
impl Error for SkillParseError {}
impl Error for SkillLoadError {}
impl From<SkillParseError> for SkillLoadError {
fn from(error: SkillParseError) -> Self {
Self::Parse(error)
}
}
pub struct SkillRoot {
pub path: AbsolutePathBuf,
@@ -693,7 +666,7 @@ async fn parse_skill_file(
scope: SkillScope,
plugin_identity: Option<&PluginIdentity>,
plugin_root: Option<&AbsolutePathBuf>,
) -> Result<SkillMetadata, SkillParseError> {
) -> Result<SkillMetadata, SkillLoadError> {
let metadata_path = path_uri
.parent()
.and_then(|parent| parent.join(SKILLS_METADATA_DIR).ok())
@@ -708,12 +681,12 @@ async fn parse_skill_file(
fs.read_file_text(path_uri, /*sandbox*/ None),
load_skill_metadata(fs, path, &metadata, plugin_root),
);
let contents = contents.map_err(SkillParseError::Read)?;
let contents = contents.map_err(SkillLoadError::Read)?;
let ParsedSkillFrontmatter {
name: base_name,
description,
short_description,
} = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(path))?;
} = parse_skill_frontmatter_metadata(&contents, || default_skill_name(path))?;
let LoadedSkillMetadata {
interface,
dependencies,
@@ -734,56 +707,6 @@ async fn parse_skill_file(
})
}
fn parse_skill_frontmatter_metadata_inner(
contents: &str,
default_name: impl FnOnce() -> String,
) -> Result<ParsedSkillFrontmatter, SkillParseError> {
let frontmatter = extract_frontmatter(contents).ok_or(SkillParseError::MissingFrontmatter)?;
let parsed: SkillFrontmatter = match serde_yaml::from_str(&frontmatter) {
Ok(parsed) => Ok(parsed),
Err(original_error) => match repair_frontmatter_scalar_fields(&frontmatter) {
// Some third-party skills use prose like `description: Build for AWS: ECS`
// or `argument-hint: <duration: e.g. 7d>`. Keep the repair line-oriented
// so unrelated invalid YAML still surfaces.
Some(repaired_frontmatter) => {
serde_yaml::from_str(&repaired_frontmatter).map_err(|_| original_error)
}
None => Err(original_error),
},
}
.map_err(SkillParseError::InvalidYaml)?;
let name = parsed
.name
.as_deref()
.map(sanitize_single_line)
.filter(|value| !value.is_empty())
.unwrap_or_else(default_name);
let description = parsed
.description
.as_deref()
.map(sanitize_single_line)
.unwrap_or_default();
let short_description = parsed
.metadata
.short_description
.as_deref()
.map(sanitize_single_line)
.filter(|value| !value.is_empty());
validate_len(&name, MAX_NAME_LEN, "name")?;
if description.is_empty() {
return Err(SkillParseError::MissingField("description"));
}
Ok(ParsedSkillFrontmatter {
name,
description,
short_description,
})
}
fn default_skill_name(path: &AbsolutePathBuf) -> String {
path.parent()
.and_then(|parent| {
@@ -1067,91 +990,6 @@ fn sanitize_single_line(raw: &str) -> String {
raw.split_whitespace().collect::<Vec<_>>().join(" ")
}
fn repair_frontmatter_scalar_fields(frontmatter: &str) -> Option<String> {
let mut changed = false;
let mut block_scalar_indent: Option<usize> = None;
let mut repaired_lines: Vec<String> = Vec::new();
for line in frontmatter.lines() {
let indent = line
.chars()
.take_while(|character| *character == ' ')
.count();
if let Some(block_indent) = block_scalar_indent {
if line.trim().is_empty() || indent > block_indent {
repaired_lines.push(line.to_string());
continue;
}
block_scalar_indent = None;
}
let Some((key, value)) = line.split_once(':') else {
repaired_lines.push(line.to_string());
continue;
};
if key.trim().is_empty() || !value.chars().next().is_none_or(char::is_whitespace) {
repaired_lines.push(line.to_string());
continue;
}
let trimmed_start = value.trim_start();
let leading_whitespace = &value[..value.len() - trimmed_start.len()];
let mut scalar = trimmed_start;
let mut comment = "";
for (index, character) in trimmed_start.char_indices() {
if character == '#'
&& (index == 0
|| trimmed_start[..index]
.chars()
.next_back()
.is_some_and(char::is_whitespace))
{
let comment_start = trimmed_start[..index].trim_end().len();
scalar = &trimmed_start[..comment_start];
comment = &trimmed_start[comment_start..];
break;
}
}
let scalar = scalar.trim_end();
let Some(first_char) = scalar.chars().next() else {
repaired_lines.push(line.to_string());
continue;
};
if matches!(first_char, '|' | '>') {
block_scalar_indent = Some(indent);
repaired_lines.push(line.to_string());
continue;
}
if matches!(first_char, '\'' | '"') {
repaired_lines.push(line.to_string());
continue;
}
let mut has_colon_separator = false;
let mut chars = scalar.chars().peekable();
while let Some(character) = chars.next() {
if character == ':'
&& matches!(chars.peek(), Some(next_character) if next_character.is_whitespace())
{
has_colon_separator = true;
break;
}
}
let invalid_flow_like_scalar = matches!(first_char, '[' | '{' | '@' | '`')
&& serde_yaml::from_str::<serde_yaml::Value>(scalar).is_err();
if !has_colon_separator && !invalid_flow_like_scalar {
repaired_lines.push(line.to_string());
continue;
}
let quoted_scalar = format!("'{}'", scalar.replace('\'', "''"));
repaired_lines.push(format!(
"{key}:{leading_whitespace}{quoted_scalar}{comment}"
));
changed = true;
}
changed.then(|| repaired_lines.join("\n"))
}
fn validate_len(
value: &str,
max_len: usize,
@@ -1211,28 +1049,6 @@ fn resolve_color_str(value: Option<String>, field: &'static str) -> Option<Strin
}
}
fn extract_frontmatter(contents: &str) -> Option<String> {
let mut lines = contents.lines();
if !matches!(lines.next(), Some(line) if line.trim() == "---") {
return None;
}
let mut frontmatter_lines: Vec<&str> = Vec::new();
let mut found_closing = false;
for line in lines.by_ref() {
if line.trim() == "---" {
found_closing = true;
break;
}
frontmatter_lines.push(line);
}
if frontmatter_lines.is_empty() || !found_closing {
return None;
}
Some(frontmatter_lines.join("\n"))
}
#[cfg(test)]
pub(crate) async fn skill_roots_from_layer_stack(
fs: Arc<dyn ExecutorFileSystem>,
codex-rs/core-skills/src/loader/environment.rs
+3 -142
View File
@@ -1,10 +1,10 @@
use std::collections::HashMap;
use std::io;
use codex_exec_server::CapabilityRootDiscovery;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::protocol::Product;
pub use codex_skills::EnvironmentSkillMetadata;
use codex_skills::ParsedSkillFrontmatter;
use codex_skills::parse_skill_frontmatter_metadata;
use codex_utils_path_uri::PathUri;
use futures::StreamExt;
@@ -12,7 +12,6 @@ use crate::model::SkillDependencies;
use crate::model::SkillPolicy;
use super::MAX_QUALIFIED_NAME_LEN;
use super::ParsedSkillFrontmatter;
use super::SkillMetadataFile;
use super::discovery::DirectorySymlinkPolicy;
use super::discovery::DiscoveredSkill;
@@ -22,7 +21,6 @@ use super::discovery::SkillDiscoveryOptions;
use super::discovery::SkillMetadataDiscovery;
use super::discovery::discover_skills;
use super::namespace::SkillNamespaceResolver;
use super::parse_skill_frontmatter_metadata_inner;
use super::resolve_dependencies;
use super::resolve_policy;
use super::sanitize_single_line;
@@ -37,19 +35,6 @@ struct ParsedEnvironmentSkill {
policy: Option<SkillPolicy>,
}
/// Parsed executor skill plus the instructions already materialized by capability discovery.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct EnvironmentSkillSnapshot {
pub metadata: EnvironmentSkillMetadata,
pub instructions: String,
}
#[derive(Debug, Default)]
pub struct EnvironmentSkillSnapshotOutcome {
pub skills: Vec<EnvironmentSkillSnapshot>,
pub warnings: Vec<String>,
}
impl ParsedEnvironmentSkill {
async fn load(
file_system: &dyn ExecutorFileSystem,
@@ -72,7 +57,7 @@ impl ParsedEnvironmentSkill {
name: base_name,
description,
short_description,
} = parse_skill_frontmatter_metadata_inner(&contents, || default_skill_name(&skill.path))
} = parse_skill_frontmatter_metadata(&contents, || default_skill_name(&skill.path))
.map_err(|err| err.to_string())?;
let (dependencies, policy) = match &skill.metadata {
SkillMetadataDiscovery::Present(_) | SkillMetadataDiscovery::Absent => {
@@ -197,130 +182,6 @@ pub async fn load_environment_skills_from_root(
outcome
}
/// Parses an executor-produced manifest bundle without issuing additional filesystem requests.
pub fn load_environment_skills_from_discovery(
discovery: &CapabilityRootDiscovery,
restriction_product: Option<Product>,
) -> EnvironmentSkillSnapshotOutcome {
let mut outcome = EnvironmentSkillSnapshotOutcome {
warnings: discovery.warnings.clone(),
..Default::default()
};
if let Some(error) = &discovery.error {
outcome.warnings.push(error.clone());
return outcome;
}
let mut plugin_namespaces = HashMap::new();
for (plugin_root, name) in discovery.namespace_manifests.iter().filter_map(|manifest| {
#[derive(serde::Deserialize)]
struct ManifestName {
#[serde(default)]
name: String,
}
let plugin_root = manifest.path.parent()?.parent()?;
let parsed = serde_json::from_str::<ManifestName>(&manifest.contents).ok()?;
let name = if parsed.name.trim().is_empty() {
plugin_root.basename()?
} else {
parsed.name
};
Some((plugin_root, name))
}) {
// Exec-server orders manifests by the same precedence as local discovery. Preserve the
// first manifest if an older or alternate server returns duplicates for one plugin root.
plugin_namespaces.entry(plugin_root).or_insert(name);
}
for skill in &discovery.skills {
let plugin_namespace =
nearest_plugin_namespace(&skill.instructions.path, &plugin_namespaces);
let ParsedSkillFrontmatter {
name: base_name,
description,
short_description,
} = match parse_skill_frontmatter_metadata_inner(&skill.instructions.contents, || {
default_skill_name(&skill.instructions.path)
}) {
Ok(frontmatter) => frontmatter,
Err(error) => {
outcome.warnings.push(format!(
"Failed to load environment skill at {}: {error}",
skill.instructions.path
));
continue;
}
};
let name = plugin_namespace
.map(|namespace| format!("{namespace}:{base_name}"))
.unwrap_or(base_name);
if let Err(error) = validate_len(&name, MAX_QUALIFIED_NAME_LEN, "qualified name") {
outcome.warnings.push(format!(
"Failed to load environment skill at {}: {error}",
skill.instructions.path
));
continue;
}
let (dependencies, policy) = skill
.metadata
.as_ref()
.and_then(|metadata| {
serde_yaml::from_str::<SkillMetadataFile>(&metadata.contents)
.map_err(|error| {
tracing::warn!(
path = %metadata.path,
"ignoring invalid discovered skill metadata: {error}"
);
})
.ok()
})
.map(|metadata| {
(
resolve_dependencies(metadata.dependencies),
resolve_policy(metadata.policy),
)
})
.unwrap_or((None, None));
let metadata = EnvironmentSkillMetadata {
path_to_skills_md: skill.instructions.path.clone(),
name,
description,
short_description,
dependencies,
policy,
};
if metadata.matches_product_restriction(restriction_product) {
outcome.skills.push(EnvironmentSkillSnapshot {
metadata,
instructions: skill.instructions.contents.clone(),
});
}
}
outcome.skills.sort_by(|left, right| {
left.metadata.name.cmp(&right.metadata.name).then_with(|| {
left.metadata
.path_to_skills_md
.to_string()
.cmp(&right.metadata.path_to_skills_md.to_string())
})
});
outcome
}
fn nearest_plugin_namespace<'a>(
skill_path: &PathUri,
plugin_namespaces: &'a HashMap<PathUri, String>,
) -> Option<&'a str> {
let mut ancestor = skill_path.parent();
while let Some(path) = ancestor {
if let Some(namespace) = plugin_namespaces.get(&path) {
return Some(namespace);
}
ancestor = path.parent();
}
None
}
async fn read_skill_contents(
file_system: &dyn ExecutorFileSystem,
skill_path: &PathUri,