Enrich V2 capability discovery with resolved plugin and skill metadata (#53763)

## What changed

- Return qualified skill names such as `team:deploy` and parsed `dependencies` and `policy` instead of a separate namespace and raw metadata file. Share `agents/openai.yaml` parsing and validation between skill loaders and executor discovery.
- Include each standalone skill's search-root ID and path in `discoveryRoot`, while accepting responses from older executors that omit it.
- Add plugin `displayName`, falling back to the manifest name or installation directory.
- Materialize inline MCP declarations in `mcpConfig` with the manifest as their source path, and report incomplete discovery when declared MCP or Apps configuration files have no returned contents.

## Testing

Add coverage for JSON round trips, omitted `discoveryRoot`, optional and invalid skill metadata, qualified names, display-name fallbacks, and inline MCP configuration. Verify standalone search roots survive cached discovery and sandboxed RPC responses.

GitOrigin-RevId: 4124972444ad7c2fb8c6418acbf9a589d797fad2
This commit is contained in:
Anthony Tafoya
2026-10-13 20:40:38 +00:00
committed by copyberry
parent 21c1616119
commit 20b4ff2f35
20 changed files with 621 additions and 233 deletions
+1 -1
View File
@@ -3303,6 +3303,7 @@ dependencies = [
"codex-network-proxy",
"codex-protocol",
"codex-shell-command",
"codex-skills",
"codex-utils-path-uri",
"pretty_assertions",
"serde",
@@ -4639,7 +4640,6 @@ dependencies = [
"schemars 0.8.22",
"serde",
"serde_json",
"serde_yaml",
"tempfile",
"tokio",
"toml 0.9.11+spec-1.1.0",
@@ -147,11 +147,13 @@ impl Executor {
.iter().map(|root| json!({"id":root["id"],"path":root["path"]})).collect::<Vec<_>>()
})),
Some("capabilities/discoverV2") => Some(json!({
"plugins":[{"id":"inert-sentinel@market","remotePluginId":null,
"plugins":[{"id":"inert-sentinel@market","displayName":"inert-sentinel","remotePluginId":null,
"version":"1","root":"file:///inert-sentinel",
"files":{"manifest":{"path":"file:///inert-sentinel/.codex-plugin/plugin.json",
"contents":"{\"name\":\"inert-sentinel\",\"mcpServers\":{\"inert-sentinel\":{\"command\":\"never-run\"}}}"},
"mcpConfig":null,"appsConfig":null},"skills":[]}],
"mcpConfig":{"path":"file:///inert-sentinel/.codex-plugin/plugin.json",
"contents":"{\"inert-sentinel\":{\"command\":\"never-run\"}}"},
"appsConfig":null},"skills":[]}],
"skills":[{"name":"inert-sentinel","description":"must not enter the prompt",
"shortDescription":null,"path":"file:///inert-sentinel/SKILL.md","metadata":null}],
"warnings":[]
+1
View File
@@ -19,6 +19,7 @@ codex-install-context = { workspace = true }
codex-network-proxy = { workspace = true }
codex-protocol = { workspace = true }
codex-shell-command = { workspace = true }
codex-skills = { workspace = true }
codex-utils-path-uri = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true, features = ["arbitrary_precision", "raw_value"] }
@@ -1,13 +1,14 @@
//! Executor capability inventory; callers apply identity and enablement.
use codex_file_system::FileSystemSandboxContext;
use codex_skills::SkillDependencies;
use codex_skills::SkillPolicy;
use codex_utils_path_uri::PathUri;
use serde::Deserialize;
use serde::Serialize;
use std::fmt;
use std::sync::Arc;
use crate::CapabilityTextFile;
use crate::DiscoveredPluginFiles;
pub const CAPABILITIES_DISCOVER_V2_METHOD: &str = "capabilities/discoverV2";
@@ -44,6 +45,8 @@ pub struct DiscoverV2CapabilitiesResponse {
pub struct ExecutorPlugin {
/// Environment-local ID: `<name>@<marketplace>`.
pub id: String,
/// Manifest display name, falling back to its name or the installation directory.
pub display_name: String,
pub remote_plugin_id: Option<String>,
pub version: String,
pub root: PathUri,
@@ -51,20 +54,30 @@ pub struct ExecutorPlugin {
pub skills: Vec<ExecutorSkill>,
}
/// Executor-owned search root for a standalone skill.
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct SkillDiscoveryRoot {
pub id: String,
pub path: PathUri,
}
/// Skill metadata, excluding instructions and resources.
#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct ExecutorSkill {
/// Final skill name, qualified by the nearest enclosing plugin namespace when present.
pub name: String,
/// Nearest enclosing legacy plugin's name, following V1 precedence.
#[serde(default)]
pub namespace: Option<String>,
pub description: String,
#[serde(default)]
pub short_description: Option<String>,
/// Path to SKILL.md, not its containing directory.
pub path: PathUri,
pub metadata: Option<CapabilityTextFile>,
/// Absent for plugin skills and responses from older executors.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub discovery_root: Option<SkillDiscoveryRoot>,
pub dependencies: Option<SkillDependencies>,
pub policy: Option<SkillPolicy>,
}
impl fmt::Debug for ExecutorPlugin {
@@ -95,12 +108,7 @@ impl fmt::Debug for ExecutorSkill {
formatter
.debug_struct("ExecutorSkill")
.field("name", &self.name)
.field("namespace", &self.namespace)
.field("path", &self.path)
.field(
"metadata_path",
&self.metadata.as_ref().map(|file| &file.path),
)
.finish()
}
}
@@ -1,6 +1,7 @@
//! V2 discovery requests and responses round-trip through JSON.
use super::*;
use crate::CapabilityTextFile;
use pretty_assertions::assert_eq;
#[test]
@@ -19,16 +20,37 @@ fn discovery_contract_round_trips_json() -> Result<(), Box<dyn std::error::Error
contents: "{}".to_string(),
};
let skill = ExecutorSkill {
name: "example".to_string(),
namespace: Some("nested".to_string()),
name: "nested:example".to_string(),
description: "Example skill".to_string(),
short_description: None,
path: PathUri::parse("file:///plugin/skills/example/SKILL.md")?,
metadata: Some(text_file.clone()),
discovery_root: None,
dependencies: Some(codex_skills::SkillDependencies {
tools: vec![codex_skills::SkillToolDependency {
r#type: "mcp".into(),
value: "deploy".into(),
description: None,
transport: Some("stdio".into()),
command: Some("deploy-mcp".into()),
url: None,
oauth_callback_port: Some(3118),
}],
}),
policy: Some(codex_skills::SkillPolicy {
allow_implicit_invocation: Some(false),
products: vec![],
}),
};
let mut standalone = skill.clone();
standalone.path = PathUri::parse("file:///home/skills/nested/example/SKILL.md")?;
standalone.discovery_root = Some(SkillDiscoveryRoot {
id: "codex-home-skills".to_string(),
path: PathUri::parse("file:///home/skills")?,
});
let response = DiscoverV2CapabilitiesResponse {
plugins: vec![ExecutorPlugin {
id: "example@company".to_string(),
display_name: "Example".to_string(),
remote_plugin_id: None,
version: "local".to_string(),
root: PathUri::parse("file:///plugin")?,
@@ -37,15 +59,34 @@ fn discovery_contract_round_trips_json() -> Result<(), Box<dyn std::error::Error
mcp_config: Some(text_file.clone()),
apps_config: Some(text_file),
},
skills: vec![skill.clone()],
skills: vec![skill],
}],
skills: vec![skill],
skills: vec![standalone],
warnings: Vec::new(),
};
let wire = serde_json::to_value(&response)?;
let mut wire = serde_json::to_value(&response)?;
assert_eq!(
wire["skills"][0]["discoveryRoot"],
serde_json::json!({
"id": "codex-home-skills",
"path": "file:///home/skills",
})
);
assert_eq!(wire["plugins"][0]["skills"][0].get("discoveryRoot"), None);
assert_eq!(
serde_json::from_value::<DiscoverV2CapabilitiesResponse>(wire.clone())?,
response
);
// Older executors omit the root metadata.
wire["skills"][0]
.as_object_mut()
.expect("skill object")
.remove("discoveryRoot");
let mut legacy_response = response;
legacy_response.skills[0].discovery_root = None;
assert_eq!(
serde_json::from_value::<DiscoverV2CapabilitiesResponse>(wire)?,
response
legacy_response
);
Ok(())
}
@@ -966,7 +966,8 @@ pub struct CapabilityRootsDiscoverResponse {
pub roots: Vec<CapabilityRootDiscovery>,
}
/// Recognized UTF-8 capability file materialized by the exec-server.
/// UTF-8 capability contents materialized by the exec-server.
/// `path` identifies the source file; V2 inline MCP contents are extracted from its manifest.
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct CapabilityTextFile {
@@ -979,7 +980,9 @@ pub struct CapabilityTextFile {
#[serde(rename_all = "camelCase")]
pub struct DiscoveredPluginFiles {
pub manifest: CapabilityTextFile,
/// File-backed MCP declarations, including the conventional `.mcp.json` fallback.
/// MCP declarations from a selected file, including the conventional `.mcp.json` fallback.
/// V2 also materializes inline declarations, using the manifest as the source path.
/// V1 leaves inline declarations only in `manifest`.
#[serde(default)]
pub mcp_config: Option<CapabilityTextFile>,
/// File-backed connector declarations.
@@ -6,16 +6,19 @@ use std::sync::Arc;
use codex_file_system::ExecutorFileSystem;
use codex_skills::parse_skill_frontmatter_metadata;
use codex_skills::parse_skill_metadata;
use futures::StreamExt;
use serde::Deserialize;
use serde::Serialize;
use crate::CapabilityRootDiscoverRequest;
use crate::CapabilityRootDiscovery;
use crate::CapabilityTextFile;
use crate::DiscoverV2CapabilitiesResponse;
use crate::ExecutorPlugin;
use crate::ExecutorSkill;
use crate::LocalFileSystem;
use crate::SkillDiscoveryRoot;
use crate::capability_discovery::MAX_CONCURRENT_ROOTS;
use crate::capability_discovery::discover_root;
use crate::discover_v2::capability_locations::CapabilityLocation;
@@ -39,6 +42,23 @@ struct CapabilityDiscovery {
warnings: Vec<String>,
}
// Only the metadata V2 returns or needs to materialize discovered configuration.
#[derive(Deserialize)]
#[serde(rename_all = "camelCase")]
struct PluginManifestMetadata {
#[serde(default)]
name: String,
interface: Option<PluginManifestInterface>,
mcp_servers: Option<serde_json::Value>,
apps: Option<String>,
}
#[derive(Deserialize)]
#[serde(rename_all = "camelCase")]
struct PluginManifestInterface {
display_name: Option<String>,
}
impl Default for CapabilityDiscovery {
fn default() -> Self {
Self {
@@ -237,18 +257,87 @@ fn parse_root(
}
ancestor = root.parent();
}
let metadata = skill.metadata.as_ref().and_then(|metadata| {
parse_skill_metadata(&metadata.contents).map_err(|error| {
tracing::warn!(path = %metadata.path, %error, "ignoring invalid discovered skill metadata");
}).ok()
}).unwrap_or_default();
skills.push(ExecutorSkill {
name: parsed.name,
namespace,
name: namespace
.map(|namespace| format!("{namespace}:{}", parsed.name))
.unwrap_or(parsed.name),
description: parsed.description,
short_description: parsed.short_description,
path,
metadata: skill.metadata,
discovery_root: location
.standalone_root_id
.as_ref()
.map(|id| SkillDiscoveryRoot {
id: id.clone(),
path: location.root.clone(),
}),
dependencies: metadata.dependencies,
policy: metadata.policy,
});
}
let content = if let (Some(plugin), Some(files)) = (location.plugin, discovery.plugin) {
let content = if let (Some(plugin), Some(mut files)) = (location.plugin, discovery.plugin) {
let manifest =
match serde_json::from_str::<PluginManifestMetadata>(&files.manifest.contents) {
Ok(manifest) => manifest,
Err(_) => {
warnings.push(format!(
"invalid plugin manifest at {}",
files.manifest.path
));
return CapabilityDiscovery {
warnings,
..Default::default()
};
}
};
let name = if manifest.name.trim().is_empty() {
location.root.basename().unwrap_or(manifest.name)
} else {
manifest.name
};
let display_name = manifest
.interface
.and_then(|interface| interface.display_name)
.map(|display_name| display_name.trim().to_string())
.filter(|display_name| !display_name.is_empty())
.unwrap_or(name);
// V2 materializes inline declarations; the path identifies their source manifest.
if files.mcp_config.is_none() {
if let Some(contents) = manifest
.mcp_servers
.as_ref()
.filter(|value| value.is_object())
{
files.mcp_config = Some(CapabilityTextFile {
path: files.manifest.path.clone(),
contents: contents.to_string(),
});
} else if manifest
.mcp_servers
.as_ref()
.is_some_and(serde_json::Value::is_string)
{
warnings.push(format!(
"plugin `{}` at {} declares an MCP configuration file, but discovery returned no contents",
plugin.id, location.root
));
}
}
if files.apps_config.is_none() && manifest.apps.is_some() {
warnings.push(format!(
"plugin `{}` at {} declares an Apps configuration file, but discovery returned no contents",
plugin.id, location.root
));
}
DiscoveredSource::Plugin(Box::new(ExecutorPlugin {
id: plugin.id,
display_name,
remote_plugin_id: plugin.remote_plugin_id,
version: plugin.version,
root: location.root,
@@ -39,6 +39,7 @@ const AGENTS_SKILLS_SUFFIX: &str = ".agents/skills";
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub(crate) struct CapabilityLocation {
pub root: PathUri,
pub standalone_root_id: Option<String>,
pub plugin: Option<InstalledPlugin>,
}
@@ -69,19 +70,22 @@ pub(crate) async fn prewarm_locations(
.map(PathUri::to_abs_path)
.transpose()?;
// Fixed standalone roots extend Flora's V1 root; project skill discovery is deferred.
let mut skill_roots = vec![codex_home.join("skills")];
let mut skill_roots = vec![("codex-home-skills", codex_home.join("skills"))];
if let Some(home) = user_home {
skill_roots.push(home.join(AGENTS_SKILLS_SUFFIX));
skill_roots.push(("user-agent-skills", home.join(AGENTS_SKILLS_SUFFIX)));
}
if let Some(system_root) = &request.system_skills_root {
skill_roots.push(system_root.to_abs_path()?);
skill_roots.push(("system-skills", system_root.to_abs_path()?));
}
CandidateCapabilityLocations::resolve(
fs,
Some(PathUri::from_abs_path(&codex_home.join(PLUGINS_CACHE_DIR))),
skill_roots
.into_iter()
.map(|root| PathUri::from_abs_path(&root))
.map(|(id, root)| crate::SkillDiscoveryRoot {
id: id.to_string(),
path: PathUri::from_abs_path(&root),
})
.collect(),
)
.await
@@ -171,6 +175,7 @@ async fn discover_cached_plugins(
};
locations.locations.push(CapabilityLocation {
root: PathUri::from_abs_path(&package_root.join(&version)),
standalone_root_id: None,
plugin: Some(InstalledPlugin {
id,
version,
@@ -5,6 +5,7 @@ use super::CapabilityLocations;
use super::MAX_LOCATIONS;
use super::discover_cached_plugins;
use crate::LocalFileSystem;
use crate::SkillDiscoveryRoot;
use codex_file_system::ExecutorFileSystem;
use codex_file_watcher::WatchPath;
use codex_utils_path_uri::PathUri;
@@ -22,7 +23,7 @@ pub(super) struct CandidateCapabilityLocations {
#[derive(Clone, Debug, PartialEq, Eq)]
enum CandidateCapabilityLocation {
Cache(PathUri),
Standalone(PathUri),
Standalone(SkillDiscoveryRoot),
}
impl CandidateCapabilityLocations {
@@ -30,7 +31,7 @@ impl CandidateCapabilityLocations {
pub(super) async fn resolve(
fs: &LocalFileSystem,
cache_root: Option<PathUri>,
skill_roots: Vec<PathUri>,
skill_roots: Vec<SkillDiscoveryRoot>,
) -> io::Result<CapabilityLocations> {
let mut candidates = cache_root
.into_iter()
@@ -38,7 +39,7 @@ impl CandidateCapabilityLocations {
.collect::<Vec<_>>();
let mut standalone = skill_roots;
let mut seen = std::collections::HashSet::new();
standalone.retain(|root| seen.insert(root.clone()));
standalone.retain(|root| seen.insert(root.path.clone()));
candidates.extend(
standalone
.into_iter()
@@ -49,8 +50,8 @@ impl CandidateCapabilityLocations {
let mut retained_candidates = Vec::new();
for candidate in candidates {
let root = match &candidate {
CandidateCapabilityLocation::Cache(root)
| CandidateCapabilityLocation::Standalone(root) => root,
CandidateCapabilityLocation::Cache(root) => root,
CandidateCapabilityLocation::Standalone(root) => &root.path,
};
let exists = match fs
.get_metadata(root, Default::default(), /*sandbox*/ None)
@@ -90,7 +91,8 @@ impl CandidateCapabilityLocations {
// Standalone roots need no manifest or version selection.
CandidateCapabilityLocation::Standalone(root) => Ok(CapabilityLocations {
locations: vec![CapabilityLocation {
root: root.clone(),
root: root.path.clone(),
standalone_root_id: Some(root.id.clone()),
plugin: None,
}],
..Default::default()
@@ -9,6 +9,7 @@ use crate::CapabilityRootDiscoverRequest;
use crate::DiscoverV2CapabilitiesRequest;
use crate::ExecutorSkill;
use crate::LocalFileSystem;
use crate::SkillDiscoveryRoot;
use crate::capability_discovery::discover_root;
use crate::discover_v2::capability_locations::tests::MANIFEST;
use crate::discover_v2::capability_locations::tests::request;
@@ -36,6 +37,24 @@ async fn discovery_preserves_inventory_and_v1_metadata() -> anyhow::Result<()> {
MANIFEST,
)?;
}
let inline_root = "codex/plugins/cache/test/unconfigured/local";
write(
temp.path(),
"codex/plugins/cache/test/disabled/local/.codex-plugin/plugin.json",
r#"{"name":" ","interface":{"displayName":" "},"mcpServers":"./missing-mcp.json","apps":"./missing-apps.json"}"#,
)?;
let inline_manifest = r#"{"name":"inline","interface":{"displayName":" Inline Plugin "},"mcpServers":{"inline":{"command":"node"}}}"#;
write(
temp.path(),
&format!("{inline_root}/.codex-plugin/plugin.json"),
inline_manifest,
)?;
// Inline declarations take precedence over a conventional MCP file.
write(
temp.path(),
&format!("{inline_root}/.mcp.json"),
r#"{"wrong":{"command":"wrong"}}"#,
)?;
let root = "codex/plugins/cache/test/demo/local";
write(
temp.path(),
@@ -86,6 +105,17 @@ async fn discovery_preserves_inventory_and_v1_metadata() -> anyhow::Result<()> {
.await;
// V1 keeps undeclared/migrated skills and excludes the skill beyond its depth limit.
assert_eq!(v1.skills.len(), 4);
let inline_v1 = discover_root(
&file_system,
CapabilityRootDiscoverRequest {
id: "unconfigured@test".to_string(),
path: PathUri::from_host_native_path(temp.path().join(inline_root))?,
sandbox: None,
},
)
.await;
let inline_v1_files = inline_v1.plugin.expect("inline plugin");
assert_eq!(inline_v1_files.mcp_config, None);
let manager = CapabilityManager::new(file_system);
manager.seed_prewarmed_locations(request.clone()).await?;
let response = manager
@@ -102,6 +132,14 @@ async fn discovery_preserves_inventory_and_v1_metadata() -> anyhow::Result<()> {
.collect::<Vec<_>>(),
vec!["demo@test", "disabled@test", "unconfigured@test"],
);
assert_eq!(
response
.plugins
.iter()
.map(|plugin| plugin.display_name.as_str())
.collect::<Vec<_>>(),
vec!["demo", "local", "Inline Plugin"],
);
let plugin = response
.plugins
.first()
@@ -122,38 +160,60 @@ async fn discovery_preserves_inventory_and_v1_metadata() -> anyhow::Result<()> {
v1.skills
.into_iter()
.map(|skill| {
let namespace = if skill
let name = if skill
.instructions
.path
.starts_with(&plugin.root.join("nested")?)
{
"team"
"team:deploy"
} else {
"demo"
"demo:deploy"
};
Ok(expected_skill(skill.instructions.path, Some(namespace)))
Ok(expected_skill(skill.instructions.path, name))
})
.collect::<anyhow::Result<Vec<_>>>()?
);
assert_eq!(
response.skills,
vec![
ExecutorSkill {
name: "deploy skill".to_string(),
..expected_skill(
request.codex_home.join("skills/deploy skill/SKILL.md")?,
/*namespace*/ None,
)
},
expected_skill(
request.codex_home.join("skills/deploy skill/SKILL.md")?,
"deploy skill",
),
expected_skill(
request
.codex_home
.join("skills/nested/skills/deploy/SKILL.md")?,
Some("team")
"team:deploy"
),
],
]
.into_iter()
.map(|mut skill| {
skill.discovery_root = Some(SkillDiscoveryRoot {
id: "codex-home-skills".to_string(),
path: request.codex_home.join("skills").expect("skills root"),
});
skill
})
.collect::<Vec<_>>(),
);
let inline_plugin = response
.plugins
.iter()
.find(|plugin| plugin.id == "unconfigured@test")
.expect("inline plugin");
assert_eq!(inline_plugin.files.manifest, inline_v1_files.manifest);
assert_eq!(
inline_plugin.files.mcp_config,
Some(crate::CapabilityTextFile {
path: inline_v1_files.manifest.path,
contents: serde_json::json!({"inline": {"command": "node"}}).to_string(),
})
);
assert_eq!(
response.warnings,
vec!["some capability metadata could not be loaded; discovery may be incomplete"],
);
assert_eq!(response.warnings, v1.warnings);
let json = serde_json::to_string(&response)?;
assert!(
response.plugins.len() + plugin.skills.len() + response.skills.len() <= MAX_CAPABILITIES
@@ -164,14 +224,15 @@ async fn discovery_preserves_inventory_and_v1_metadata() -> anyhow::Result<()> {
Ok(())
}
fn expected_skill(path: PathUri, namespace: Option<&str>) -> ExecutorSkill {
fn expected_skill(path: PathUri, name: &str) -> ExecutorSkill {
ExecutorSkill {
name: "deploy".to_string(),
namespace: namespace.map(str::to_string),
name: name.to_string(),
description: "Deploy the service.".to_string(),
short_description: None,
path,
metadata: None,
discovery_root: None,
dependencies: None,
policy: None,
}
}
@@ -214,3 +275,133 @@ async fn discovery_reports_location_failures_without_paths() -> anyhow::Result<(
}
Ok(())
}
#[tokio::test]
async fn discovery_resolves_optional_skill_metadata() -> anyhow::Result<()> {
use codex_protocol::protocol::Product;
use codex_skills::SkillDependencies;
use codex_skills::SkillPolicy;
use codex_skills::SkillToolDependency;
let dependencies = Some(SkillDependencies {
tools: vec![SkillToolDependency {
r#type: "mcp".into(),
value: "deploy".into(),
description: None,
transport: Some("stdio".into()),
command: Some("deploy-mcp".into()),
url: None,
oauth_callback_port: Some(3118),
}],
});
let policy = Some(SkillPolicy {
allow_implicit_invocation: Some(false),
products: vec![Product::Codex, Product::Chatgpt],
});
for (yaml, dependencies, policy) in [
(None, None, None),
(Some("interface: [invalid]"), None, None),
(Some("dependencies:\n tools: [{type: mcp}]"), None, None),
(Some("dependencies: {tools: []}"), None, None),
(
Some(
r#"dependencies:
tools:
- type: " mcp "
value: " deploy "
transport: stdio
command: deploy-mcp
oauth: {callback_port: 3118}
- type: mcp
value: " "
policy:
allow_implicit_invocation: false
products: [codex, CHATGPT]
"#,
),
dependencies,
policy,
),
] {
let temp = tempfile::tempdir()?;
let request = request(temp.path())?;
let plugin_root = "codex/plugins/cache/test/demo/local";
write(
temp.path(),
&format!("{plugin_root}/.codex-plugin/plugin.json"),
MANIFEST,
)?;
for root in [
"codex/skills/deploy".to_string(),
format!("{plugin_root}/skills/deploy"),
] {
write(temp.path(), &format!("{root}/SKILL.md"), SKILL)?;
if let Some(yaml) = yaml {
write(temp.path(), &format!("{root}/agents/openai.yaml"), yaml)?;
}
}
let manager = CapabilityManager::new(LocalFileSystem::unsandboxed());
manager.seed_prewarmed_locations(request.clone()).await?;
let response = manager
.get_or_refresh_discovery(DiscoverV2CapabilitiesRequest {
cwd: PathUri::from_host_native_path(temp.path().join("project"))?,
sandbox: None,
})
.await?;
let mut expected =
expected_skill(request.codex_home.join("skills/deploy/SKILL.md")?, "deploy");
expected.dependencies = dependencies;
expected.policy = policy;
expected.discovery_root = Some(SkillDiscoveryRoot {
id: "codex-home-skills".to_string(),
path: request.codex_home.join("skills")?,
});
assert_eq!(response.skills, vec![expected.clone()]);
expected.path = request
.codex_home
.join("plugins/cache/test/demo/local/skills/deploy/SKILL.md")?;
expected.name = "demo:deploy".into();
expected.discovery_root = None;
assert_eq!(response.plugins[0].skills, vec![expected]);
assert_eq!(response.warnings, Vec::<String>::new());
}
Ok(())
}
#[tokio::test]
async fn standalone_skills_retain_their_search_roots() -> anyhow::Result<()> {
let temp = tempfile::tempdir()?;
let request = request(temp.path())?;
let roots = [
("codex-home-skills", "codex/skills"),
("user-agent-skills", "home/.agents/skills"),
("system-skills", "etc/codex/skills"),
];
let mut expected = Vec::new();
for (id, root) in roots {
for relative in ["deploy/SKILL.md", "nested/deploy/SKILL.md"] {
let path = format!("{root}/{relative}");
write(temp.path(), &path, SKILL)?;
let mut skill = expected_skill(
PathUri::from_host_native_path(temp.path().join(path))?,
"deploy",
);
skill.discovery_root = Some(SkillDiscoveryRoot {
id: id.to_string(),
path: PathUri::from_host_native_path(temp.path().join(root))?,
});
expected.push(skill);
}
}
let manager = CapabilityManager::new(LocalFileSystem::unsandboxed());
manager.seed_prewarmed_locations(request.clone()).await?;
let query = DiscoverV2CapabilitiesRequest {
cwd: PathUri::from_host_native_path(temp.path().join("project"))?,
sandbox: None,
};
let response = manager.get_or_refresh_discovery(query.clone()).await?;
assert_eq!(response.skills, expected);
// Cached discovery must retain the same root metadata.
assert_eq!(manager.get_or_refresh_discovery(query).await?, response);
Ok(())
}
@@ -67,15 +67,21 @@ async fn prewarming_resolves_global_locations_without_cwd_or_config() -> anyhow:
actual.sort_unstable_by_key(|location| location.root.to_string());
let mut expected = vec![CapabilityLocation {
root: request.codex_home.join("plugins/cache/team/demo/1.10.0")?,
standalone_root_id: None,
plugin: Some(InstalledPlugin {
id: "demo@team".to_string(),
version: "1.10.0".to_string(),
remote_plugin_id: Some("plugins~demo".to_string()),
}),
}];
for root in ["codex/skills", "home/.agents/skills", "etc/codex/skills"] {
for (id, root) in [
("codex-home-skills", "codex/skills"),
("user-agent-skills", "home/.agents/skills"),
("system-skills", "etc/codex/skills"),
] {
expected.push(CapabilityLocation {
root: PathUri::from_host_native_path(temp.path().join(root))?,
standalone_root_id: Some(id.to_string()),
plugin: None,
});
}
+1
View File
@@ -228,6 +228,7 @@ pub use protocol::ShellInfo;
pub use protocol::ShellSnapshotRequest;
pub use protocol::SignalParams;
pub use protocol::SignalResponse;
pub use protocol::SkillDiscoveryRoot;
pub use protocol::TerminateParams;
pub use protocol::TerminateResponse;
pub use protocol::WriteParams;
+33 -2
View File
@@ -6,10 +6,12 @@ use codex_exec_server::CAPABILITIES_DISCOVER_V2_METHOD;
use codex_exec_server::DiscoverV2CapabilitiesRequest;
use codex_exec_server::DiscoverV2CapabilitiesResponse;
use codex_exec_server::InitializeResponse;
use codex_exec_server::SkillDiscoveryRoot;
use codex_exec_server_protocol::JSONRPCMessage;
use codex_utils_path_uri::PathUri;
use common::exec_server::ExecServerHarness;
use common::exec_server::exec_server_with_env;
use pretty_assertions::assert_eq;
#[cfg(any(target_os = "linux", target_os = "macos", windows))]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -71,6 +73,19 @@ async fn v2_rpc_refreshes_metadata_and_enforces_sandbox() -> anyhow::Result<()>
.iter()
.any(|skill| skill.name == "demo" && skill.description == "original")
);
let expected_root = Some(SkillDiscoveryRoot {
id: "codex-home-skills".to_string(),
path: PathUri::from_host_native_path(codex_home_path.join("skills"))?,
});
assert_eq!(
first
.skills
.iter()
.find(|skill| skill.name == "demo")
.context("demo skill")?
.discovery_root,
expected_root,
);
assert!(!serde_json::to_string(&first)?.contains("PRIVATE INSTRUCTION BODY"));
assert!(
@@ -131,7 +146,15 @@ async fn v2_rpc_refreshes_metadata_and_enforces_sandbox() -> anyhow::Result<()>
{
let response = discover(&mut server, &restricted).await?;
assert!(response.plugins.is_empty());
assert!(response.skills.iter().any(|skill| skill.name == "demo"));
assert_eq!(
response
.skills
.iter()
.find(|skill| skill.name == "demo")
.context("sandboxed demo skill")?
.discovery_root,
expected_root,
);
assert!(!serde_json::to_string(&response)?.contains("mail@market"));
}
// Windows restricted tokens reject split read access; a supported read-only policy succeeds.
@@ -163,7 +186,15 @@ async fn v2_rpc_refreshes_metadata_and_enforces_sandbox() -> anyhow::Result<()>
.iter()
.any(|plugin| plugin.id == "mail@market")
);
assert!(response.skills.iter().any(|skill| skill.name == "demo"));
assert_eq!(
response
.skills
.iter()
.find(|skill| skill.name == "demo")
.context("sandboxed demo skill")?
.discovery_root,
expected_root,
);
}
}
-1
View File
@@ -32,7 +32,6 @@ futures = { workspace = true }
schemars = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }
serde_yaml = { workspace = true }
tokio = { workspace = true, features = ["rt", "sync", "time"] }
toml = { workspace = true }
tracing = { workspace = true }
+5 -15
View File
@@ -24,12 +24,10 @@ use super::discovery::MAX_CONCURRENT_SKILL_LOADS;
use super::discovery::SkillDiscoveryOptions;
use super::discovery::SkillMetadataDiscovery;
use super::discovery::discover_skills;
use super::metadata::SkillMetadataFile;
use super::metadata::resolve_dependencies;
use super::metadata::resolve_policy;
use super::metadata::sanitize_single_line;
use super::metadata::validate_len;
use super::namespace::SkillNamespaceResolver;
use codex_skills::parse_skill_metadata;
struct ParsedEnvironmentSkill {
path_to_skills_md: PathUri,
@@ -269,7 +267,7 @@ pub fn load_environment_skills_from_discovery(
.metadata
.as_ref()
.and_then(|metadata| {
serde_yaml::from_str::<SkillMetadataFile>(&metadata.contents)
parse_skill_metadata(&metadata.contents)
.map_err(|error| {
tracing::warn!(
path = %metadata.path,
@@ -278,12 +276,7 @@ pub fn load_environment_skills_from_discovery(
})
.ok()
})
.map(|metadata| {
(
resolve_dependencies(metadata.dependencies),
resolve_policy(metadata.policy),
)
})
.map(|metadata| (metadata.dependencies, metadata.policy))
.unwrap_or((None, None));
let metadata = EnvironmentSkillMetadata {
path_to_skills_md: skill.instructions.path.clone(),
@@ -367,7 +360,7 @@ async fn read_skill_metadata(
return (None, None);
}
};
let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) {
let parsed = match parse_skill_metadata(&contents) {
Ok(parsed) => parsed,
Err(error) => {
tracing::warn!("ignoring {metadata_path}: invalid metadata: {error}");
@@ -375,10 +368,7 @@ async fn read_skill_metadata(
}
};
(
resolve_dependencies(parsed.dependencies),
resolve_policy(parsed.policy),
)
(parsed.dependencies, parsed.policy)
}
fn default_skill_name(path: &PathUri) -> String {
+6 -150
View File
@@ -3,37 +3,19 @@ use std::io;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::GetMetadataOptions;
use codex_exec_server::ReadFileOptions;
use codex_protocol::protocol::Product;
use codex_skills::ParsedSkillMetadata;
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::parse_skill_metadata;
use codex_skills::resolve_skill_interface;
use codex_utils_absolute_path::AbsolutePathBuf;
use serde::Deserialize;
use super::MAX_DEPENDENCY_COMMAND_LEN;
use super::MAX_DEPENDENCY_DESCRIPTION_LEN;
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 {
#[serde(default)]
interface: Option<SkillInterfaceFile>,
#[serde(default)]
pub(super) dependencies: Option<Dependencies>,
#[serde(default)]
pub(super) policy: Option<Policy>,
}
#[derive(Default)]
pub(super) struct LoadedSkillMetadata {
pub(super) interface: Option<SkillInterface>,
@@ -41,39 +23,6 @@ pub(super) struct LoadedSkillMetadata {
pub(super) policy: Option<SkillPolicy>,
}
#[derive(Debug, Default, Deserialize)]
pub(super) struct Dependencies {
#[serde(default)]
tools: Vec<DependencyTool>,
}
#[derive(Debug, Deserialize)]
pub(super) struct Policy {
#[serde(default)]
allow_implicit_invocation: Option<bool>,
#[serde(default)]
products: Vec<Product>,
}
#[derive(Debug, Default, Deserialize)]
struct DependencyTool {
#[serde(rename = "type")]
kind: Option<String>,
value: Option<String>,
description: Option<String>,
transport: Option<String>,
command: Option<String>,
url: Option<String>,
oauth: Option<DependencyOAuth>,
}
#[derive(Debug, Default, Deserialize)]
#[serde(rename_all = "camelCase")]
struct DependencyOAuth {
#[serde(alias = "callback_port")]
callback_port: Option<u16>,
}
pub(super) async fn load_host_skill_metadata(
file_system: &dyn ExecutorFileSystem,
skill_path: &AbsolutePathBuf,
@@ -127,7 +76,7 @@ pub(super) async fn load_host_skill_metadata(
}
};
let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) {
let parsed = match parse_skill_metadata(&contents) {
Ok(parsed) => parsed,
Err(error) => {
tracing::warn!(
@@ -138,7 +87,7 @@ pub(super) async fn load_host_skill_metadata(
}
};
let SkillMetadataFile {
let ParsedSkillMetadata {
interface,
dependencies,
policy,
@@ -149,73 +98,10 @@ pub(super) async fn load_host_skill_metadata(
};
LoadedSkillMetadata {
interface: resolve_skill_interface(interface, &skill_dir, asset_policy),
dependencies: resolve_dependencies(dependencies),
policy: resolve_policy(policy),
dependencies,
policy,
}
}
pub(super) fn resolve_dependencies(
dependencies: Option<Dependencies>,
) -> Option<SkillDependencies> {
let dependencies = dependencies?;
let tools = dependencies
.tools
.into_iter()
.filter_map(resolve_dependency_tool)
.collect::<Vec<_>>();
if tools.is_empty() {
None
} else {
Some(SkillDependencies { tools })
}
}
pub(super) fn resolve_policy(policy: Option<Policy>) -> Option<SkillPolicy> {
policy.map(|policy| SkillPolicy {
allow_implicit_invocation: policy.allow_implicit_invocation,
products: policy.products,
})
}
fn resolve_dependency_tool(tool: DependencyTool) -> Option<SkillToolDependency> {
let r#type = resolve_required_str(
tool.kind,
MAX_DEPENDENCY_TYPE_LEN,
"dependencies.tools.type",
)?;
let value = resolve_required_str(
tool.value,
MAX_DEPENDENCY_VALUE_LEN,
"dependencies.tools.value",
)?;
let description = resolve_str(
tool.description,
MAX_DEPENDENCY_DESCRIPTION_LEN,
"dependencies.tools.description",
);
let transport = resolve_str(
tool.transport,
MAX_DEPENDENCY_TRANSPORT_LEN,
"dependencies.tools.transport",
);
let command = resolve_str(
tool.command,
MAX_DEPENDENCY_COMMAND_LEN,
"dependencies.tools.command",
);
let url = resolve_str(tool.url, MAX_DEPENDENCY_URL_LEN, "dependencies.tools.url");
let oauth_callback_port = tool.oauth.and_then(|oauth| oauth.callback_port);
Some(SkillToolDependency {
r#type,
value,
description,
transport,
command,
url,
oauth_callback_port,
})
}
pub(super) fn sanitize_single_line(raw: &str) -> String {
raw.split_whitespace().collect::<Vec<_>>().join(" ")
}
@@ -236,33 +122,3 @@ pub(super) fn validate_len(
}
Ok(())
}
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() {
tracing::warn!("ignoring {field}: value is empty");
return None;
}
if value.chars().count() > max_len {
tracing::warn!("ignoring {field}: exceeds maximum length of {max_len} characters");
return None;
}
Some(value)
}
fn resolve_required_str(
value: Option<String>,
max_len: usize,
field: &'static str,
) -> Option<String> {
let Some(value) = value else {
tracing::warn!("ignoring {field}: value is missing");
return None;
};
resolve_str(Some(value), max_len, field)
}
-7
View File
@@ -21,12 +21,5 @@ pub(super) const SKILLS_METADATA_DIR: &str = "agents";
pub(super) const SKILLS_METADATA_FILENAME: &str = "openai.yaml";
pub(super) const MAX_NAME_LEN: usize = 64;
pub(super) const MAX_QUALIFIED_NAME_LEN: usize = MAX_NAME_LEN * 2 + 1;
pub(super) const MAX_DESCRIPTION_LEN: usize = 1024;
pub(super) const MAX_DEPENDENCY_TYPE_LEN: usize = MAX_NAME_LEN;
pub(super) const MAX_DEPENDENCY_TRANSPORT_LEN: usize = MAX_NAME_LEN;
pub(super) const MAX_DEPENDENCY_VALUE_LEN: usize = MAX_DESCRIPTION_LEN;
pub(super) const MAX_DEPENDENCY_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN;
pub(super) const MAX_DEPENDENCY_COMMAND_LEN: usize = MAX_DESCRIPTION_LEN;
pub(super) const MAX_DEPENDENCY_URL_LEN: usize = MAX_DESCRIPTION_LEN;
pub(super) const MAX_SCAN_DEPTH: usize = 6;
pub(super) const MAX_SKILLS_DIRS_PER_ROOT: usize = 2000;
+3
View File
@@ -2,6 +2,7 @@ mod interface;
mod invocation;
mod loading;
mod mentions;
mod metadata;
mod model;
mod name_counts;
mod parser;
@@ -30,6 +31,8 @@ pub use mentions::extract_tool_mentions_with_sigil;
pub use mentions::normalize_skill_path;
pub use mentions::plugin_config_name_from_path;
pub use mentions::tool_kind_for_path;
pub use metadata::ParsedSkillMetadata;
pub use metadata::parse_skill_metadata;
pub use model::EnvironmentSkillMetadata;
pub use model::SkillDependencies;
pub use model::SkillInterface;
+164
View File
@@ -0,0 +1,164 @@
//! Parses optional skill YAML metadata with shared host and executor validation.
use codex_protocol::protocol::Product;
use serde::Deserialize;
use crate::SkillDependencies;
use crate::SkillInterfaceFile;
use crate::SkillPolicy;
use crate::SkillToolDependency;
const MAX_DEPENDENCY_TYPE_LEN: usize = 64;
const MAX_DEPENDENCY_TRANSPORT_LEN: usize = 64;
const MAX_DEPENDENCY_VALUE_LEN: usize = 1024;
const MAX_DEPENDENCY_DESCRIPTION_LEN: usize = 1024;
const MAX_DEPENDENCY_COMMAND_LEN: usize = 1024;
const MAX_DEPENDENCY_URL_LEN: usize = 1024;
/// Parsed optional metadata; callers resolve interface assets for their filesystem.
#[derive(Debug, Default)]
pub struct ParsedSkillMetadata {
pub interface: Option<SkillInterfaceFile>,
pub dependencies: Option<SkillDependencies>,
pub policy: Option<SkillPolicy>,
}
/// Parses `agents/openai.yaml`, dropping invalid individual dependency declarations.
pub fn parse_skill_metadata(contents: &str) -> Result<ParsedSkillMetadata, serde_yaml::Error> {
let parsed: SkillMetadataFile = serde_yaml::from_str(contents)?;
Ok(ParsedSkillMetadata {
interface: parsed.interface,
dependencies: resolve_dependencies(parsed.dependencies),
policy: resolve_policy(parsed.policy),
})
}
#[derive(Debug, Default, Deserialize)]
struct SkillMetadataFile {
#[serde(default)]
interface: Option<SkillInterfaceFile>,
#[serde(default)]
dependencies: Option<Dependencies>,
#[serde(default)]
policy: Option<Policy>,
}
#[derive(Debug, Default, Deserialize)]
struct Dependencies {
#[serde(default)]
tools: Vec<DependencyTool>,
}
#[derive(Debug, Deserialize)]
struct Policy {
#[serde(default)]
allow_implicit_invocation: Option<bool>,
#[serde(default)]
products: Vec<Product>,
}
#[derive(Debug, Default, Deserialize)]
struct DependencyTool {
#[serde(rename = "type")]
kind: Option<String>,
value: Option<String>,
description: Option<String>,
transport: Option<String>,
command: Option<String>,
url: Option<String>,
oauth: Option<DependencyOAuth>,
}
#[derive(Debug, Default, Deserialize)]
#[serde(rename_all = "camelCase")]
struct DependencyOAuth {
#[serde(alias = "callback_port")]
callback_port: Option<u16>,
}
fn resolve_dependencies(dependencies: Option<Dependencies>) -> Option<SkillDependencies> {
let dependencies = dependencies?;
let tools = dependencies
.tools
.into_iter()
.filter_map(resolve_dependency_tool)
.collect::<Vec<_>>();
if tools.is_empty() {
None
} else {
Some(SkillDependencies { tools })
}
}
fn resolve_policy(policy: Option<Policy>) -> Option<SkillPolicy> {
policy.map(|policy| SkillPolicy {
allow_implicit_invocation: policy.allow_implicit_invocation,
products: policy.products,
})
}
fn resolve_dependency_tool(tool: DependencyTool) -> Option<SkillToolDependency> {
let r#type = resolve_required_str(
tool.kind,
MAX_DEPENDENCY_TYPE_LEN,
"dependencies.tools.type",
)?;
let value = resolve_required_str(
tool.value,
MAX_DEPENDENCY_VALUE_LEN,
"dependencies.tools.value",
)?;
let description = resolve_str(
tool.description,
MAX_DEPENDENCY_DESCRIPTION_LEN,
"dependencies.tools.description",
);
let transport = resolve_str(
tool.transport,
MAX_DEPENDENCY_TRANSPORT_LEN,
"dependencies.tools.transport",
);
let command = resolve_str(
tool.command,
MAX_DEPENDENCY_COMMAND_LEN,
"dependencies.tools.command",
);
let url = resolve_str(tool.url, MAX_DEPENDENCY_URL_LEN, "dependencies.tools.url");
let oauth_callback_port = tool.oauth.and_then(|oauth| oauth.callback_port);
Some(SkillToolDependency {
r#type,
value,
description,
transport,
command,
url,
oauth_callback_port,
})
}
fn resolve_str(value: Option<String>, max_len: usize, field: &'static str) -> Option<String> {
let value = value?;
let value = value.split_whitespace().collect::<Vec<_>>().join(" ");
if value.is_empty() {
tracing::warn!("ignoring {field}: value is empty");
return None;
}
if value.chars().count() > max_len {
tracing::warn!("ignoring {field}: exceeds maximum length of {max_len} characters");
return None;
}
Some(value)
}
fn resolve_required_str(
value: Option<String>,
max_len: usize,
field: &'static str,
) -> Option<String> {
let Some(value) = value else {
tracing::warn!("ignoring {field}: value is missing");
return None;
};
resolve_str(Some(value), max_len, field)
}
+6 -3
View File
@@ -59,7 +59,8 @@ impl EnvironmentSkillMetadata {
}
}
#[derive(Debug, Clone, PartialEq, Eq, Default)]
#[derive(Debug, Clone, PartialEq, Eq, Default, serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct SkillPolicy {
pub allow_implicit_invocation: Option<bool>,
// TODO: Enforce product gating in Codex skill selection/injection instead of only parsing and
@@ -77,12 +78,14 @@ pub struct SkillInterface {
pub default_prompt: Option<String>,
}
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct SkillDependencies {
pub tools: Vec<SkillToolDependency>,
}
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct SkillToolDependency {
pub r#type: String,
pub value: String,