agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

feat: support disable skills by name. (#15378)

Browse Source 
Support disabling skills by name, primarily for plugin skills. We can’t
use the path, since plugin skill paths may change across versions.
This commit is contained in:
xl-openai
2026-03-23 12:57:40 -07:00
committed by GitHub
parent 332edba78e
commit 9a33e5c0a0
24 changed files with 983 additions and 139 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/plugins/manager.rs
+124 -27
View File
@@ -19,7 +19,6 @@ use super::remote::fetch_remote_featured_plugin_ids;
use super::remote::fetch_remote_plugin_status;
use super::remote::uninstall_remote_plugin;
use super::startup_sync::start_startup_remote_plugin_sync_once;
use super::store::DEFAULT_PLUGIN_VERSION;
use super::store::PluginId;
use super::store::PluginIdError;
use super::store::PluginInstallResult as StorePluginInstallResult;
@@ -38,6 +37,9 @@ use crate::config::types::McpServerConfig;
use crate::config::types::PluginConfig;
use crate::config_loader::ConfigLayerStack;
use crate::skills::SkillMetadata;
use crate::skills::config_rules::SkillConfigRules;
use crate::skills::config_rules::resolve_disabled_skill_paths;
use crate::skills::config_rules::skill_config_rules_from_stack;
use crate::skills::loader::SkillRoot;
use crate::skills::loader::load_skills_from_roots;
use codex_app_server_protocol::ConfigValueWriteParams;
@@ -152,6 +154,7 @@ pub struct PluginDetail {
pub installed: bool,
pub enabled: bool,
pub skills: Vec<SkillMetadata>,
pub disabled_skill_paths: HashSet<PathBuf>,
pub apps: Vec<AppConnectorId>,
pub mcp_server_names: Vec<String>,
}
@@ -183,6 +186,8 @@ pub struct LoadedPlugin {
pub root: AbsolutePathBuf,
pub enabled: bool,
pub skill_roots: Vec<PathBuf>,
pub disabled_skill_paths: HashSet<PathBuf>,
pub has_enabled_skills: bool,
pub mcp_servers: HashMap<String, McpServerConfig>,
pub apps: Vec<AppConnectorId>,
pub error: Option<String>,
@@ -235,7 +240,7 @@ impl PluginCapabilitySummary {
.clone()
.unwrap_or_else(|| plugin.config_name.clone()),
description: prompt_safe_plugin_description(plugin.manifest_description.as_deref()),
has_skills: !plugin.skill_roots.is_empty(),
has_skills: plugin.has_enabled_skills,
mcp_server_names,
app_connector_ids: plugin.apps.clone(),
};
@@ -258,11 +263,16 @@ impl PluginCapabilitySummary {
impl From<PluginDetail> for PluginCapabilitySummary {
fn from(value: PluginDetail) -> Self {
let has_skills = value.skills.iter().any(|skill| {
!value
.disabled_skill_paths
.contains(&skill.path_to_skills_md)
});
Self {
config_name: value.id,
display_name: value.name,
description: prompt_safe_plugin_description(value.description.as_deref()),
has_skills: !value.skills.is_empty(),
has_skills,
mcp_server_names: value.mcp_server_names,
app_connector_ids: value.apps,
}
@@ -531,7 +541,11 @@ impl PluginsManager {
return outcome;
}
let outcome = load_plugins_from_layer_stack(&config.config_layer_stack, &self.store);
let outcome = load_plugins_from_layer_stack(
&config.config_layer_stack,
&self.store,
self.restriction_product,
);
log_plugin_load_errors(&outcome);
let mut cache = match self.cached_enabled_outcome.write() {
Ok(cache) => cache,
@@ -1070,6 +1084,11 @@ impl PluginsManager {
let source_path = match &plugin.source {
MarketplacePluginSource::Local { path } => path.clone(),
};
if !source_path.as_path().is_dir() {
return Err(MarketplaceError::InvalidPlugin(
"path does not exist or is not a directory".to_string(),
));
}
let manifest = load_plugin_manifest(source_path.as_path()).ok_or_else(|| {
MarketplaceError::InvalidPlugin(
"missing or invalid .codex-plugin/plugin.json".to_string(),
@@ -1077,15 +1096,13 @@ impl PluginsManager {
})?;
let description = manifest.description.clone();
let manifest_paths = &manifest.paths;
let skill_roots = plugin_skill_roots(source_path.as_path(), manifest_paths);
let skills = load_skills_from_roots(skill_roots.into_iter().map(|path| SkillRoot {
path,
scope: SkillScope::User,
}))
.skills
.into_iter()
.filter(|skill| skill.matches_product_restriction_for_product(self.restriction_product))
.collect();
let skill_config_rules = skill_config_rules_from_stack(&config.config_layer_stack);
let resolved_skills = load_plugin_skills(
source_path.as_path(),
manifest_paths,
self.restriction_product,
&skill_config_rules,
);
let apps = load_plugin_apps(source_path.as_path());
let mcp_config_paths = plugin_mcp_config_paths(source_path.as_path(), manifest_paths);
let mut mcp_server_names = Vec::new();
@@ -1111,7 +1128,8 @@ impl PluginsManager {
interface: plugin.interface,
installed: installed_plugins.contains(&plugin_key),
enabled: enabled_plugins.contains(&plugin_key),
skills,
skills: resolved_skills.skills,
disabled_skill_paths: resolved_skills.disabled_skill_paths,
apps,
mcp_server_names,
},
@@ -1347,7 +1365,9 @@ struct PluginAppConfig {
pub(crate) fn load_plugins_from_layer_stack(
config_layer_stack: &ConfigLayerStack,
store: &PluginStore,
restriction_product: Option<Product>,
) -> PluginLoadOutcome {
let skill_config_rules = skill_config_rules_from_stack(config_layer_stack);
let mut configured_plugins: Vec<_> = configured_plugins_from_stack(config_layer_stack)
.into_iter()
.collect();
@@ -1356,7 +1376,13 @@ pub(crate) fn load_plugins_from_layer_stack(
let mut plugins = Vec::with_capacity(configured_plugins.len());
let mut seen_mcp_server_names = HashMap::<String, String>::new();
for (configured_name, plugin) in configured_plugins {
let loaded_plugin = load_plugin(configured_name.clone(), &plugin, store);
let loaded_plugin = load_plugin(
configured_name.clone(),
&plugin,
store,
restriction_product,
&skill_config_rules,
);
for name in loaded_plugin.mcp_servers.keys() {
if let Some(previous_plugin) =
seen_mcp_server_names.insert(name.clone(), configured_name.clone())
@@ -1463,16 +1489,24 @@ fn configured_plugins_from_stack(
}
}
fn load_plugin(config_name: String, plugin: &PluginConfig, store: &PluginStore) -> LoadedPlugin {
let plugin_root = PluginId::parse(&config_name).map(|plugin_id| {
store
.active_plugin_root(&plugin_id)
.unwrap_or_else(|| store.plugin_root(&plugin_id, DEFAULT_PLUGIN_VERSION))
});
let root = match &plugin_root {
Ok(plugin_root) => plugin_root.clone(),
Err(_) => store.root().clone(),
};
fn load_plugin(
config_name: String,
plugin: &PluginConfig,
store: &PluginStore,
restriction_product: Option<Product>,
skill_config_rules: &SkillConfigRules,
) -> LoadedPlugin {
let plugin_id = PluginId::parse(&config_name);
let active_plugin_root = plugin_id
.as_ref()
.ok()
.and_then(|plugin_id| store.active_plugin_root(plugin_id));
let root = active_plugin_root
.clone()
.unwrap_or_else(|| match &plugin_id {
Ok(plugin_id) => store.plugin_base_root(plugin_id),
Err(_) => store.root().clone(),
});
let mut loaded_plugin = LoadedPlugin {
config_name,
manifest_name: None,
@@ -1480,6 +1514,8 @@ fn load_plugin(config_name: String, plugin: &PluginConfig, store: &PluginStore)
root,
enabled: plugin.enabled,
skill_roots: Vec::new(),
disabled_skill_paths: HashSet::new(),
has_enabled_skills: false,
mcp_servers: HashMap::new(),
apps: Vec::new(),
error: None,
@@ -1489,8 +1525,14 @@ fn load_plugin(config_name: String, plugin: &PluginConfig, store: &PluginStore)
return loaded_plugin;
}
let plugin_root = match plugin_root {
Ok(plugin_root) => plugin_root,
let plugin_root = match plugin_id {
Ok(_) => match active_plugin_root {
Some(plugin_root) => plugin_root,
None => {
loaded_plugin.error = Some("plugin is not installed".to_string());
return loaded_plugin;
}
},
Err(err) => {
loaded_plugin.error = Some(err.to_string());
return loaded_plugin;
@@ -1511,6 +1553,15 @@ fn load_plugin(config_name: String, plugin: &PluginConfig, store: &PluginStore)
loaded_plugin.manifest_name = Some(manifest.name.clone());
loaded_plugin.manifest_description = manifest.description.clone();
loaded_plugin.skill_roots = plugin_skill_roots(plugin_root.as_path(), manifest_paths);
let resolved_skills = load_plugin_skills(
plugin_root.as_path(),
manifest_paths,
restriction_product,
skill_config_rules,
);
let has_enabled_skills = resolved_skills.has_enabled_skills();
loaded_plugin.disabled_skill_paths = resolved_skills.disabled_skill_paths;
loaded_plugin.has_enabled_skills = has_enabled_skills;
let mut mcp_servers = HashMap::new();
for mcp_config_path in plugin_mcp_config_paths(plugin_root.as_path(), manifest_paths) {
let plugin_mcp = load_mcp_servers_from_file(plugin_root.as_path(), &mcp_config_path);
@@ -1530,6 +1581,52 @@ fn load_plugin(config_name: String, plugin: &PluginConfig, store: &PluginStore)
loaded_plugin
}
struct ResolvedPluginSkills {
skills: Vec<SkillMetadata>,
disabled_skill_paths: HashSet<PathBuf>,
had_errors: bool,
}
impl ResolvedPluginSkills {
fn has_enabled_skills(&self) -> bool {
// Keep the plugin visible in capability summaries if skill loading was partial.
self.had_errors
|| self
.skills
.iter()
.any(|skill| !self.disabled_skill_paths.contains(&skill.path_to_skills_md))
}
}
fn load_plugin_skills(
plugin_root: &Path,
manifest_paths: &PluginManifestPaths,
restriction_product: Option<Product>,
skill_config_rules: &SkillConfigRules,
) -> ResolvedPluginSkills {
let outcome = load_skills_from_roots(
plugin_skill_roots(plugin_root, manifest_paths)
.into_iter()
.map(|path| SkillRoot {
path,
scope: SkillScope::User,
}),
);
let had_errors = !outcome.errors.is_empty();
let skills = outcome
.skills
.into_iter()
.filter(|skill| skill.matches_product_restriction_for_product(restriction_product))
.collect::<Vec<_>>();
let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules);
ResolvedPluginSkills {
skills,
disabled_skill_paths,
had_errors,
}
}
fn plugin_skill_roots(plugin_root: &Path, manifest_paths: &PluginManifestPaths) -> Vec<PathBuf> {
let mut paths = default_skill_roots(plugin_root);
if let Some(path) = &manifest_paths.skills {
codex-rs/core/src/plugins/manager_tests.rs
+160 -2
View File
@@ -13,6 +13,7 @@ use crate::plugins::test_support::write_curated_plugin_sha_with as write_curated
use crate::plugins::test_support::write_file;
use crate::plugins::test_support::write_openai_curated_marketplace;
use codex_app_server_protocol::ConfigLayerSource;
use codex_protocol::protocol::Product;
use pretty_assertions::assert_eq;
use std::fs;
use tempfile::TempDir;
@@ -139,6 +140,8 @@ fn load_plugins_loads_default_skills_and_mcp_servers() {
root: AbsolutePathBuf::try_from(plugin_root.clone()).unwrap(),
enabled: true,
skill_roots: vec![plugin_root.join("skills")],
disabled_skill_paths: HashSet::new(),
has_enabled_skills: true,
mcp_servers: HashMap::from([(
"sample".to_string(),
McpServerConfig {
@@ -185,6 +188,89 @@ fn load_plugins_loads_default_skills_and_mcp_servers() {
);
}
#[test]
fn load_plugins_resolves_disabled_skill_names_against_loaded_plugin_skills() {
let codex_home = TempDir::new().unwrap();
let plugin_root = codex_home
.path()
.join("plugins/cache")
.join("test/sample/local");
let skill_path = plugin_root.join("skills/sample-search/SKILL.md");
write_file(
&plugin_root.join(".codex-plugin/plugin.json"),
r#"{"name":"sample"}"#,
);
write_file(
&skill_path,
"---\nname: sample-search\ndescription: search sample data\n---\n",
);
let config_toml = r#"[features]
plugins = true
[[skills.config]]
name = "sample:sample-search"
enabled = false
[plugins."sample@test"]
enabled = true
"#;
let outcome = load_plugins_from_config(config_toml, codex_home.path());
let skill_path = dunce::canonicalize(skill_path).expect("skill path should canonicalize");
assert_eq!(
outcome.plugins[0].disabled_skill_paths,
HashSet::from([skill_path])
);
assert!(!outcome.plugins[0].has_enabled_skills);
assert!(outcome.capability_summaries().is_empty());
}
#[test]
fn load_plugins_ignores_unknown_disabled_skill_names() {
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"}"#,
);
write_file(
&plugin_root.join("skills/sample-search/SKILL.md"),
"---\nname: sample-search\ndescription: search sample data\n---\n",
);
let config_toml = r#"[features]
plugins = true
[[skills.config]]
name = "sample:missing-skill"
enabled = false
[plugins."sample@test"]
enabled = true
"#;
let outcome = load_plugins_from_config(config_toml, codex_home.path());
assert!(outcome.plugins[0].disabled_skill_paths.is_empty());
assert!(outcome.plugins[0].has_enabled_skills);
assert_eq!(
outcome.capability_summaries(),
&[PluginCapabilitySummary {
config_name: "sample@test".to_string(),
display_name: "sample".to_string(),
description: None,
has_skills: true,
mcp_server_names: Vec::new(),
app_connector_ids: Vec::new(),
}]
);
}
#[test]
fn plugin_telemetry_metadata_uses_default_mcp_config_path() {
let codex_home = TempDir::new().unwrap();
@@ -540,6 +626,8 @@ fn load_plugins_preserves_disabled_plugins_without_effective_contributions() {
root: AbsolutePathBuf::try_from(plugin_root).unwrap(),
enabled: false,
skill_roots: Vec::new(),
disabled_skill_paths: HashSet::new(),
has_enabled_skills: false,
mcp_servers: HashMap::new(),
apps: Vec::new(),
error: None,
@@ -651,6 +739,8 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() {
root: AbsolutePathBuf::try_from(codex_home.path().join(dir_name)).unwrap(),
enabled: true,
skill_roots: Vec::new(),
disabled_skill_paths: HashSet::new(),
has_enabled_skills: false,
mcp_servers: HashMap::new(),
apps: Vec::new(),
error: None,
@@ -664,6 +754,7 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() {
let outcome = PluginLoadOutcome::from_plugins(vec![
LoadedPlugin {
skill_roots: vec![codex_home.path().join("skills-plugin/skills")],
has_enabled_skills: true,
..plugin("skills@test", "skills-plugin", "skills-plugin")
},
LoadedPlugin {
@@ -1166,6 +1257,70 @@ enabled = true
assert!(matches!(err, MarketplaceError::PluginsDisabled));
}
#[tokio::test]
async fn read_plugin_for_config_uses_user_layer_skill_settings_only() {
let tmp = tempfile::tempdir().unwrap();
let repo_root = tmp.path().join("repo");
let plugin_root = repo_root.join("enabled-plugin");
fs::create_dir_all(repo_root.join(".git")).unwrap();
fs::create_dir_all(repo_root.join(".agents/plugins")).unwrap();
write_file(
&repo_root.join(".agents/plugins/marketplace.json"),
r#"{
"name": "debug",
"plugins": [
{
"name": "enabled-plugin",
"source": {
"source": "local",
"path": "./enabled-plugin"
}
}
]
}"#,
);
write_file(
&plugin_root.join(".codex-plugin/plugin.json"),
r#"{"name":"enabled-plugin"}"#,
);
write_file(
&plugin_root.join("skills/sample-search/SKILL.md"),
"---\nname: sample-search\ndescription: search sample data\n---\n",
);
write_file(
&tmp.path().join(CONFIG_TOML_FILE),
r#"[features]
plugins = true
[plugins."enabled-plugin@debug"]
enabled = true
"#,
);
write_file(
&repo_root.join(".codex/config.toml"),
r#"[[skills.config]]
name = "enabled-plugin:sample-search"
enabled = false
"#,
);
let config = load_config(tmp.path(), &repo_root).await;
let outcome = PluginsManager::new(tmp.path().to_path_buf())
.read_plugin_for_config(
&config,
&PluginReadRequest {
plugin_name: "enabled-plugin".to_string(),
marketplace_path: AbsolutePathBuf::try_from(
repo_root.join(".agents/plugins/marketplace.json"),
)
.unwrap(),
},
)
.unwrap();
assert!(outcome.plugin.disabled_skill_paths.is_empty());
}
#[tokio::test]
async fn sync_plugins_from_remote_returns_default_when_feature_disabled() {
let tmp = tempfile::tempdir().unwrap();
@@ -2082,8 +2237,11 @@ fn load_plugins_ignores_project_config_files() {
)
.expect("config layer stack should build");
let outcome =
load_plugins_from_layer_stack(&stack, &PluginStore::new(codex_home.path().to_path_buf()));
let outcome = load_plugins_from_layer_stack(
&stack,
&PluginStore::new(codex_home.path().to_path_buf()),
Some(Product::Codex),
);
assert_eq!(outcome, PluginLoadOutcome::default());
}
codex-rs/core/src/plugins/store.rs
+8 -3
View File
@@ -110,10 +110,15 @@ impl PluginStore {
.filter(|version| validate_plugin_segment(version, "plugin version").is_ok())
.collect::<Vec<_>>();
discovered_versions.sort_unstable();
if discovered_versions.len() == 1 {
discovered_versions.pop()
} else {
if discovered_versions.is_empty() {
None
} else if discovered_versions
.iter()
.any(|version| version == DEFAULT_PLUGIN_VERSION)
{
Some(DEFAULT_PLUGIN_VERSION.to_string())
} else {
discovered_versions.pop()
}
}
codex-rs/core/src/plugins/store_tests.rs
+44
View File
@@ -130,6 +130,50 @@ fn active_plugin_version_reads_version_directory_name() {
);
}
#[test]
fn active_plugin_version_prefers_default_local_version_when_multiple_versions_exist() {
let tmp = tempdir().unwrap();
write_plugin(
&tmp.path().join("plugins/cache/debug"),
"sample-plugin/0123456789abcdef",
"sample-plugin",
);
write_plugin(
&tmp.path().join("plugins/cache/debug"),
"sample-plugin/local",
"sample-plugin",
);
let store = PluginStore::new(tmp.path().to_path_buf());
let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap();
assert_eq!(
store.active_plugin_version(&plugin_id),
Some("local".to_string())
);
}
#[test]
fn active_plugin_version_returns_last_sorted_version_when_default_is_missing() {
let tmp = tempdir().unwrap();
write_plugin(
&tmp.path().join("plugins/cache/debug"),
"sample-plugin/0123456789abcdef",
"sample-plugin",
);
write_plugin(
&tmp.path().join("plugins/cache/debug"),
"sample-plugin/fedcba9876543210",
"sample-plugin",
);
let store = PluginStore::new(tmp.path().to_path_buf());
let plugin_id = PluginId::new("sample-plugin".to_string(), "debug".to_string()).unwrap();
assert_eq!(
store.active_plugin_version(&plugin_id),
Some("fedcba9876543210".to_string())
);
}
#[test]
fn plugin_root_rejects_path_separators_in_key_segments() {
let err = PluginId::parse("../../etc@debug").unwrap_err();