mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Load skill interface metadata in the host loader (#36903)
## What changed - Move skill interface validation and asset-path resolution into `codex-skills` so both skill loaders share the same behavior. - Populate host-loaded skills with validated display metadata, prompts, brand colors, and local icons from `agents/openai.yaml`. - Allow plugin skills to reference icons under the plugin's shared `assets` directory while rejecting absolute paths and paths that escape permitted asset roots. ## Testing - Add resolver tests for valid, invalid, local, and plugin-shared interface assets. - Add host loader coverage for interface fields and asset-path restrictions. GitOrigin-RevId: 3f316191752eeebd15d11ac0bee82201574320ec
This commit is contained in:
1 parent
f93109615f
commit
49b0aebd6f
10 files changed
+634
-456
No files matched your search
@@ -32,6 +32,7 @@ pub struct HostSkillRoot {
|
||||
pub path: AbsolutePathBuf,
|
||||
pub scope: SkillScope,
|
||||
pub file_system: Arc<dyn ExecutorFileSystem>,
|
||||
pub plugin_root: Option<AbsolutePathBuf>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
@@ -72,6 +73,10 @@ async fn load_skills_under_root(
|
||||
root: &AbsolutePathBuf,
|
||||
) -> (Vec<SkillMetadata>, 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),
|
||||
None => None,
|
||||
};
|
||||
let directory_symlinks = match skill_root.scope {
|
||||
SkillScope::User | SkillScope::Repo | SkillScope::Admin => DirectorySymlinkPolicy::Follow,
|
||||
SkillScope::System => DirectorySymlinkPolicy::Ignore,
|
||||
@@ -138,16 +143,20 @@ async fn load_skills_under_root(
|
||||
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)
|
||||
.map(|skill| {
|
||||
let plugin_root = plugin_root.as_ref();
|
||||
async move {
|
||||
let result = parse_skill_file(
|
||||
file_system,
|
||||
&skill.skill,
|
||||
&skill.path,
|
||||
&skill.path_uri,
|
||||
skill_root.scope,
|
||||
plugin_root,
|
||||
)
|
||||
.await;
|
||||
(skill.path, skill.path_uri, result)
|
||||
}
|
||||
})
|
||||
.buffered(MAX_CONCURRENT_SKILL_LOADS)
|
||||
.collect::<Vec<_>>();
|
||||
@@ -181,6 +190,7 @@ async fn parse_skill_file(
|
||||
path: &AbsolutePathBuf,
|
||||
path_uri: &PathUri,
|
||||
scope: SkillScope,
|
||||
plugin_root: Option<&AbsolutePathBuf>,
|
||||
) -> Result<SkillMetadata, String> {
|
||||
let metadata_path = path_uri
|
||||
.parent()
|
||||
@@ -194,7 +204,7 @@ async fn parse_skill_file(
|
||||
.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),
|
||||
load_host_skill_metadata(file_system, path, &metadata, plugin_root),
|
||||
);
|
||||
let contents = contents.map_err(|error| format!("failed to read file: {error}"))?;
|
||||
let ParsedSkillFrontmatter {
|
||||
@@ -204,6 +214,7 @@ async fn parse_skill_file(
|
||||
} = parse_skill_frontmatter_metadata(&contents, || default_skill_name(path))
|
||||
.map_err(|error| error.to_string())?;
|
||||
let LoadedSkillMetadata {
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
} = loaded_metadata;
|
||||
@@ -212,7 +223,7 @@ async fn parse_skill_file(
|
||||
name,
|
||||
description,
|
||||
short_description,
|
||||
interface: None,
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
path_to_skills_md: path.clone(),
|
||||
|
||||
@@ -5,6 +5,7 @@ use codex_exec_server::LOCAL_FS;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_skills::SkillDependencies;
|
||||
use codex_skills::SkillInterface;
|
||||
use codex_skills::SkillMetadata;
|
||||
use codex_skills::SkillPolicy;
|
||||
use codex_skills::SkillToolDependency;
|
||||
@@ -39,6 +40,53 @@ fn root_for(temp_dir: &TempDir, scope: SkillScope) -> HostSkillRoot {
|
||||
path: AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute root"),
|
||||
scope,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_root: None,
|
||||
}
|
||||
}
|
||||
|
||||
struct PluginSkillFixture {
|
||||
root: TempDir,
|
||||
plugin_root: AbsolutePathBuf,
|
||||
skill_path: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
impl PluginSkillFixture {
|
||||
fn new() -> Self {
|
||||
let root = TempDir::new().expect("temp dir");
|
||||
let plugin_root = AbsolutePathBuf::from_absolute_path(
|
||||
fs::canonicalize(root.path()).expect("canonical plugin root"),
|
||||
)
|
||||
.expect("absolute plugin root");
|
||||
let skill_path = write_skill(
|
||||
&root,
|
||||
"skills/send-message",
|
||||
"name: send-message\ndescription: Send messages",
|
||||
);
|
||||
Self {
|
||||
root,
|
||||
plugin_root,
|
||||
skill_path,
|
||||
}
|
||||
}
|
||||
|
||||
fn write_asset(&self, relative_path: &str) {
|
||||
let asset_path = self.root.path().join(relative_path);
|
||||
fs::create_dir_all(asset_path.parent().expect("asset parent"))
|
||||
.expect("create asset directory");
|
||||
fs::write(asset_path, "<svg/>").expect("write asset");
|
||||
}
|
||||
|
||||
fn write_metadata(&self, contents: &str) {
|
||||
write_metadata(&self.root, "skills/send-message", contents);
|
||||
}
|
||||
|
||||
fn host_root(&self) -> HostSkillRoot {
|
||||
HostSkillRoot {
|
||||
path: self.plugin_root.join("skills"),
|
||||
scope: SkillScope::User,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_root: Some(self.plugin_root.clone()),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -122,6 +170,163 @@ async fn invalid_optional_metadata_fails_open() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn loads_host_interface_metadata_and_local_asset_paths() {
|
||||
let root = TempDir::new().expect("temp dir");
|
||||
let skill_path = write_skill(&root, "demo", "name: demo\ndescription: Demo skill");
|
||||
fs::create_dir_all(root.path().join("demo/assets")).expect("create assets");
|
||||
write_metadata(
|
||||
&root,
|
||||
"demo",
|
||||
r##"interface:
|
||||
display_name: Demo
|
||||
short_description: Interface summary
|
||||
icon_small: assets/icon.svg
|
||||
icon_large: assets/icon-large.png
|
||||
brand_color: "#123ABC"
|
||||
default_prompt: Run the demo
|
||||
"##,
|
||||
);
|
||||
|
||||
let snapshot = load_host_skill_root(root_for(&root, SkillScope::User)).await;
|
||||
let skill_dir = skill_path.parent().expect("skill parent");
|
||||
|
||||
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: Some(SkillInterface {
|
||||
display_name: Some("Demo".to_string()),
|
||||
short_description: Some("Interface summary".to_string()),
|
||||
icon_small: Some(skill_dir.join("assets/icon.svg")),
|
||||
icon_large: Some(skill_dir.join("assets/icon-large.png")),
|
||||
brand_color: Some("#123ABC".to_string()),
|
||||
default_prompt: Some("Run the demo".to_string()),
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: skill_path,
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
remote_plugin_id: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn loads_plugin_skill_interface_icons_from_local_and_shared_assets() {
|
||||
let fixture = PluginSkillFixture::new();
|
||||
fixture.write_asset("skills/send-message/assets/icon.svg");
|
||||
fixture.write_asset("assets/logo.svg");
|
||||
fixture.write_metadata(
|
||||
r#"interface:
|
||||
icon_small: "assets/icon.svg"
|
||||
icon_large: "../../assets/logo.svg"
|
||||
"#,
|
||||
);
|
||||
|
||||
let snapshot = load_host_skill_root(fixture.host_root()).await;
|
||||
|
||||
assert_eq!(snapshot.errors, Vec::new());
|
||||
assert_eq!(
|
||||
snapshot.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "send-message".to_string(),
|
||||
description: "Send messages".to_string(),
|
||||
short_description: None,
|
||||
interface: Some(SkillInterface {
|
||||
display_name: None,
|
||||
short_description: None,
|
||||
icon_small: Some(
|
||||
fixture
|
||||
.plugin_root
|
||||
.join("skills/send-message/assets/icon.svg"),
|
||||
),
|
||||
icon_large: Some(fixture.plugin_root.join("assets/logo.svg")),
|
||||
brand_color: None,
|
||||
default_prompt: None,
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: fixture.skill_path,
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
remote_plugin_id: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rejects_plugin_skill_interface_icons_outside_shared_assets() {
|
||||
let fixture = PluginSkillFixture::new();
|
||||
fixture.write_asset("other/icon.svg");
|
||||
fixture.write_metadata(
|
||||
r#"interface:
|
||||
display_name: Send Message
|
||||
icon_small: "../../other/icon.svg"
|
||||
"#,
|
||||
);
|
||||
|
||||
let snapshot = load_host_skill_root(fixture.host_root()).await;
|
||||
|
||||
assert_eq!(snapshot.errors, Vec::new());
|
||||
assert_eq!(
|
||||
snapshot.skills,
|
||||
vec![SkillMetadata {
|
||||
name: "send-message".to_string(),
|
||||
description: "Send messages".to_string(),
|
||||
short_description: None,
|
||||
interface: Some(SkillInterface {
|
||||
display_name: Some("Send Message".to_string()),
|
||||
short_description: None,
|
||||
icon_small: None,
|
||||
icon_large: None,
|
||||
brand_color: None,
|
||||
default_prompt: None,
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: fixture.skill_path,
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
remote_plugin_id: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rejects_interface_fields_that_escape_or_fail_validation() {
|
||||
let root = TempDir::new().expect("temp dir");
|
||||
let skill_path = write_skill(&root, "demo", "name: demo\ndescription: Demo skill");
|
||||
write_metadata(
|
||||
&root,
|
||||
"demo",
|
||||
"interface:\n icon_small: ../outside.svg\n brand_color: blue\n",
|
||||
);
|
||||
|
||||
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: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: skill_path,
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
remote_plugin_id: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skips_hidden_host_skills() {
|
||||
let root = TempDir::new().expect("temp dir");
|
||||
|
||||
@@ -1,12 +1,16 @@
|
||||
use std::io;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_skills::SkillDependencies;
|
||||
use codex_skills::SkillInterface;
|
||||
use codex_skills::SkillInterfaceAssetPolicy;
|
||||
use codex_skills::SkillInterfaceFile;
|
||||
use codex_skills::SkillParseError;
|
||||
use codex_skills::SkillPolicy;
|
||||
use codex_skills::SkillToolDependency;
|
||||
use codex_skills::resolve_skill_interface;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use serde::Deserialize;
|
||||
|
||||
use super::MAX_DEPENDENCY_COMMAND_LEN;
|
||||
@@ -20,34 +24,17 @@ use super::discovery::SkillMetadataDiscovery;
|
||||
|
||||
#[derive(Debug, Default, Deserialize)]
|
||||
pub(super) struct SkillMetadataFile {
|
||||
// Keep parsing the host-only interface fields so malformed interface metadata
|
||||
// invalidates the file exactly as it does in the legacy loader.
|
||||
#[serde(default, rename = "interface")]
|
||||
_interface: Option<Interface>,
|
||||
#[serde(default)]
|
||||
interface: Option<SkillInterfaceFile>,
|
||||
#[serde(default)]
|
||||
pub(super) dependencies: Option<Dependencies>,
|
||||
#[serde(default)]
|
||||
pub(super) policy: Option<Policy>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default, Deserialize)]
|
||||
struct Interface {
|
||||
#[serde(rename = "display_name")]
|
||||
_display_name: Option<String>,
|
||||
#[serde(rename = "short_description")]
|
||||
_short_description: Option<String>,
|
||||
#[serde(rename = "icon_small")]
|
||||
_icon_small: Option<PathBuf>,
|
||||
#[serde(rename = "icon_large")]
|
||||
_icon_large: Option<PathBuf>,
|
||||
#[serde(rename = "brand_color")]
|
||||
_brand_color: Option<String>,
|
||||
#[serde(rename = "default_prompt")]
|
||||
_default_prompt: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
pub(super) struct LoadedSkillMetadata {
|
||||
pub(super) interface: Option<SkillInterface>,
|
||||
pub(super) dependencies: Option<SkillDependencies>,
|
||||
pub(super) policy: Option<SkillPolicy>,
|
||||
}
|
||||
@@ -79,9 +66,14 @@ struct DependencyTool {
|
||||
|
||||
pub(super) async fn load_host_skill_metadata(
|
||||
file_system: &dyn ExecutorFileSystem,
|
||||
skill_path: &AbsolutePathBuf,
|
||||
metadata: &SkillMetadataDiscovery,
|
||||
plugin_root: Option<&AbsolutePathBuf>,
|
||||
) -> LoadedSkillMetadata {
|
||||
// Fail open: optional metadata should not block loading SKILL.md.
|
||||
let Some(skill_dir) = skill_path.parent() else {
|
||||
return LoadedSkillMetadata::default();
|
||||
};
|
||||
let metadata_path = match metadata {
|
||||
SkillMetadataDiscovery::Present(path) => path,
|
||||
SkillMetadataDiscovery::Absent => return LoadedSkillMetadata::default(),
|
||||
@@ -130,11 +122,16 @@ pub(super) async fn load_host_skill_metadata(
|
||||
};
|
||||
|
||||
let SkillMetadataFile {
|
||||
_interface: _,
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
} = parsed;
|
||||
let asset_policy = match plugin_root {
|
||||
Some(plugin_root) => SkillInterfaceAssetPolicy::PluginShared { plugin_root },
|
||||
None => SkillInterfaceAssetPolicy::LocalOnly,
|
||||
};
|
||||
LoadedSkillMetadata {
|
||||
interface: resolve_skill_interface(interface, &skill_dir, asset_policy),
|
||||
dependencies: resolve_dependencies(dependencies),
|
||||
policy: resolve_policy(policy),
|
||||
}
|
||||
@@ -221,7 +218,11 @@ pub(super) fn validate_len(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn resolve_str(value: Option<String>, max_len: usize, field: &'static str) -> Option<String> {
|
||||
pub(super) fn resolve_str(
|
||||
value: Option<String>,
|
||||
max_len: usize,
|
||||
field: &'static str,
|
||||
) -> Option<String> {
|
||||
let value = value?;
|
||||
let value = sanitize_single_line(&value);
|
||||
if value.is_empty() {
|
||||
|
||||
Reference in new issue
Block a user