Handle invalid plugin skills manifest field (#25717)

## Summary
- Treat invalid `plugin.json` `skills` shapes as a field-level warning
instead of rejecting the whole manifest
- Keep valid string path behavior unchanged and continue falling back to
the default `skills/` root
- Add regression coverage for array-shaped `skills`

## Tests
- `just fmt`
- `cargo test -p codex-core-plugins`
This commit is contained in:
xli-oai authored and GitHub committed 2026-06-01 17:19:34 -07:00
1 parent 0b3a6f7185
commit 1ad0d7aa4b
2 files changed
+63 -2

No files matched your search

@@ -959,6 +959,43 @@ async fn load_plugins_ignores_manifest_component_paths_without_dot_slash() {
); );
} }
#[tokio::test]
async fn load_plugins_ignores_invalid_manifest_skills_shape() {
let codex_home = TempDir::new().unwrap();
let plugin_root = codex_home
.path()
.join("plugins/cache")
.join("test/sample/local");
write_file(
&plugin_root.join(".codex-plugin/plugin.json"),
r#"{
"name": "sample",
"skills": ["./custom-skills/"]
}"#,
);
write_file(
&plugin_root.join("skills/default-skill/SKILL.md"),
"---\nname: default-skill\ndescription: default skill\n---\n",
);
write_file(
&plugin_root.join("custom-skills/custom-skill/SKILL.md"),
"---\nname: custom-skill\ndescription: custom skill\n---\n",
);
let outcome = load_plugins_from_config(
&plugin_config_toml(/*enabled*/ true, /*plugins_feature_enabled*/ true),
codex_home.path(),
)
.await;
assert_eq!(outcome.plugins()[0].error, None);
assert_eq!(
outcome.plugins()[0].skill_roots,
vec![plugin_root.join("skills").abs()]
);
}
#[tokio::test] #[tokio::test]
async fn load_plugins_preserves_disabled_plugins_without_effective_contributions() { async fn load_plugins_preserves_disabled_plugins_without_effective_contributions() {
let codex_home = TempDir::new().unwrap(); let codex_home = TempDir::new().unwrap();
+26 -2
View File
@@ -23,7 +23,7 @@ struct RawPluginManifest {
// Keep manifest paths as raw strings so we can validate the required `./...` syntax before // Keep manifest paths as raw strings so we can validate the required `./...` syntax before
// resolving them under the plugin root. // resolving them under the plugin root.
#[serde(default)] #[serde(default)]
skills: Option<String>, skills: Option<RawPluginManifestPath>,
#[serde(default)] #[serde(default)]
mcp_servers: Option<String>, mcp_servers: Option<String>,
#[serde(default)] #[serde(default)]
@@ -127,6 +127,13 @@ enum RawPluginManifestDefaultPromptEntry {
Invalid(JsonValue), Invalid(JsonValue),
} }
#[derive(Debug, Deserialize)]
#[serde(untagged)]
enum RawPluginManifestPath {
Path(String),
Invalid(JsonValue),
}
#[derive(Debug, Deserialize)] #[derive(Debug, Deserialize)]
#[serde(untagged)] #[serde(untagged)]
enum RawPluginManifestHooks { enum RawPluginManifestHooks {
@@ -238,7 +245,7 @@ pub fn load_plugin_manifest(plugin_root: &Path) -> Option<PluginManifest> {
description, description,
keywords, keywords,
paths: PluginManifestPaths { paths: PluginManifestPaths {
skills: resolve_manifest_path(plugin_root, "skills", skills.as_deref()), skills: resolve_manifest_path_value(plugin_root, "skills", skills.as_ref()),
mcp_servers: resolve_manifest_path( mcp_servers: resolve_manifest_path(
plugin_root, plugin_root,
"mcpServers", "mcpServers",
@@ -394,6 +401,23 @@ fn json_value_type(value: &JsonValue) -> &'static str {
} }
} }
fn resolve_manifest_path_value(
plugin_root: &Path,
field: &'static str,
path: Option<&RawPluginManifestPath>,
) -> Option<AbsolutePathBuf> {
match path? {
RawPluginManifestPath::Path(path) => resolve_manifest_path(plugin_root, field, Some(path)),
RawPluginManifestPath::Invalid(value) => {
tracing::warn!(
"ignoring {field}: expected a string; found {}",
json_value_type(value)
);
None
}
}
}
fn resolve_manifest_path( fn resolve_manifest_path(
plugin_root: &Path, plugin_root: &Path,
field: &'static str, field: &'static str,