From 9a33e5c0a08e67b85ab80125d62bbe359c06cdd3 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Mon, 23 Mar 2026 12:57:40 -0700 Subject: [PATCH] feat: support disable skills by name. (#15378) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Support disabling skills by name, primarily for plugin skills. We can’t use the path, since plugin skill paths may change across versions. --- .../schema/json/ClientRequest.json | 20 +- .../codex_app_server_protocol.schemas.json | 24 ++- .../codex_app_server_protocol.v2.schemas.json | 24 ++- .../schema/json/v2/PluginReadResponse.json | 4 + .../json/v2/SkillsConfigWriteParams.json | 26 ++- .../schema/typescript/v2/SkillSummary.ts | 2 +- .../typescript/v2/SkillsConfigWriteParams.ts | 11 +- .../app-server-protocol/src/protocol/v2.rs | 8 +- codex-rs/app-server/README.md | 23 +- .../app-server/src/codex_message_processor.rs | 35 ++- .../app-server/tests/suite/v2/plugin_read.rs | 5 + codex-rs/core/config.schema.json | 14 +- codex-rs/core/src/config/edit.rs | 71 +++++-- codex-rs/core/src/config/edit_tests.rs | 21 ++ codex-rs/core/src/config/types.rs | 7 +- codex-rs/core/src/config/types_tests.rs | 41 ++++ codex-rs/core/src/plugins/manager.rs | 151 ++++++++++--- codex-rs/core/src/plugins/manager_tests.rs | 162 +++++++++++++- codex-rs/core/src/plugins/store.rs | 11 +- codex-rs/core/src/plugins/store_tests.rs | 44 ++++ codex-rs/core/src/skills/config_rules.rs | 135 ++++++++++++ codex-rs/core/src/skills/manager.rs | 83 ++------ codex-rs/core/src/skills/manager_tests.rs | 199 +++++++++++++++++- codex-rs/core/src/skills/mod.rs | 1 + 24 files changed, 983 insertions(+), 139 deletions(-) create mode 100644 codex-rs/core/src/skills/config_rules.rs diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index ae8e6fed3..903b26b80 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -2343,13 +2343,27 @@ "enabled": { "type": "boolean" }, + "name": { + "description": "Name-based selector.", + "type": [ + "string", + "null" + ] + }, "path": { - "type": "string" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Path-based selector." } }, "required": [ - "enabled", - "path" + "enabled" ], "type": "object" }, diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 6f7124751..6b731a9b5 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -11352,6 +11352,9 @@ "description": { "type": "string" }, + "enabled": { + "type": "boolean" + }, "interface": { "anyOf": [ { @@ -11377,6 +11380,7 @@ }, "required": [ "description", + "enabled", "name", "path" ], @@ -11433,13 +11437,27 @@ "enabled": { "type": "boolean" }, + "name": { + "description": "Name-based selector.", + "type": [ + "string", + "null" + ] + }, "path": { - "type": "string" + "anyOf": [ + { + "$ref": "#/definitions/v2/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Path-based selector." } }, "required": [ - "enabled", - "path" + "enabled" ], "title": "SkillsConfigWriteParams", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index a9c3b9663..0d69834f1 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -9112,6 +9112,9 @@ "description": { "type": "string" }, + "enabled": { + "type": "boolean" + }, "interface": { "anyOf": [ { @@ -9137,6 +9140,7 @@ }, "required": [ "description", + "enabled", "name", "path" ], @@ -9193,13 +9197,27 @@ "enabled": { "type": "boolean" }, + "name": { + "description": "Name-based selector.", + "type": [ + "string", + "null" + ] + }, "path": { - "type": "string" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Path-based selector." } }, "required": [ - "enabled", - "path" + "enabled" ], "title": "SkillsConfigWriteParams", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json b/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json index 5fecf5037..1917935a2 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json +++ b/codex-rs/app-server-protocol/schema/json/v2/PluginReadResponse.json @@ -318,6 +318,9 @@ "description": { "type": "string" }, + "enabled": { + "type": "boolean" + }, "interface": { "anyOf": [ { @@ -343,6 +346,7 @@ }, "required": [ "description", + "enabled", "name", "path" ], diff --git a/codex-rs/app-server-protocol/schema/json/v2/SkillsConfigWriteParams.json b/codex-rs/app-server-protocol/schema/json/v2/SkillsConfigWriteParams.json index 3fa74811d..696226a50 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/SkillsConfigWriteParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/SkillsConfigWriteParams.json @@ -1,16 +1,36 @@ { "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "AbsolutePathBuf": { + "description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.", + "type": "string" + } + }, "properties": { "enabled": { "type": "boolean" }, + "name": { + "description": "Name-based selector.", + "type": [ + "string", + "null" + ] + }, "path": { - "type": "string" + "anyOf": [ + { + "$ref": "#/definitions/AbsolutePathBuf" + }, + { + "type": "null" + } + ], + "description": "Path-based selector." } }, "required": [ - "enabled", - "path" + "enabled" ], "title": "SkillsConfigWriteParams", "type": "object" diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/SkillSummary.ts b/codex-rs/app-server-protocol/schema/typescript/v2/SkillSummary.ts index 818e0b05d..ea3739353 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/SkillSummary.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/SkillSummary.ts @@ -3,4 +3,4 @@ // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. import type { SkillInterface } from "./SkillInterface"; -export type SkillSummary = { name: string, description: string, shortDescription: string | null, interface: SkillInterface | null, path: string, }; +export type SkillSummary = { name: string, description: string, shortDescription: string | null, interface: SkillInterface | null, path: string, enabled: boolean, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsConfigWriteParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsConfigWriteParams.ts index 5a4bcf9bc..273d593cf 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsConfigWriteParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsConfigWriteParams.ts @@ -1,5 +1,14 @@ // GENERATED CODE! DO NOT MODIFY BY HAND! // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually. +import type { AbsolutePathBuf } from "../AbsolutePathBuf"; -export type SkillsConfigWriteParams = { path: string, enabled: boolean, }; +export type SkillsConfigWriteParams = { +/** + * Path-based selector. + */ +path?: AbsolutePathBuf | null, +/** + * Name-based selector. + */ +name?: string | null, enabled: boolean, }; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index a1d5e5562..e270d8ad6 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -3340,6 +3340,7 @@ pub struct SkillSummary { pub short_description: Option, pub interface: Option, pub path: PathBuf, + pub enabled: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3378,7 +3379,12 @@ pub enum PluginSource { #[serde(rename_all = "camelCase")] #[ts(export_to = "v2/")] pub struct SkillsConfigWriteParams { - pub path: PathBuf, + /// Path-based selector. + #[ts(optional = nullable)] + pub path: Option, + /// Name-based selector. + #[ts(optional = nullable)] + pub name: Option, pub enabled: bool, } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 4d139b7f7..ace7f2bd4 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -164,10 +164,10 @@ Example with notification opt-out: - `collaborationMode/list` — list available collaboration mode presets (experimental, no pagination). This response omits built-in developer instructions; clients should either pass `settings.developer_instructions: null` when setting a mode to use Codex's built-in instructions, or provide their own instructions explicitly. - `skills/list` — list skills for one or more `cwd` values (optional `forceReload`). - `plugin/list` — list discovered plugin marketplaces and plugin state, including effective marketplace install/auth policy metadata and best-effort `featuredPluginIds` for the official curated marketplace. `interface.category` uses the marketplace category when present; otherwise it falls back to the plugin manifest category. Pass `forceRemoteSync: true` to refresh curated plugin state before listing (**under development; do not call from production clients yet**). -- `plugin/read` — read one plugin by `marketplacePath` plus `pluginName`, returning marketplace info, a list-style `summary`, manifest descriptions/interface metadata, and bundled skills/apps/MCP server names. Plugin app summaries also include `needsAuth` when the server can determine connector accessibility (**under development; do not call from production clients yet**). +- `plugin/read` — read one plugin by `marketplacePath` plus `pluginName`, returning marketplace info, a list-style `summary`, manifest descriptions/interface metadata, and bundled skills/apps/MCP server names. Returned plugin skills include their current `enabled` state after local config filtering. Plugin app summaries also include `needsAuth` when the server can determine connector accessibility (**under development; do not call from production clients yet**). - `skills/changed` — notification emitted when watched local skill files change. - `app/list` — list available apps. -- `skills/config/write` — write user-level skill config by path. +- `skills/config/write` — write user-level skill config by name or absolute path. - `plugin/install` — install a plugin from a discovered marketplace entry, rejecting marketplace entries marked unavailable for install, install MCPs if any, and return the effective plugin auth policy plus any apps that still need auth (**under development; do not call from production clients yet**). - `plugin/uninstall` — uninstall a plugin by id by removing its cached files and clearing its user-level config entry (**under development; do not call from production clients yet**). - `mcpServer/oauth/login` — start an OAuth login for a configured MCP server; returns an `authorization_url` and later emits `mcpServer/oauthLogin/completed` once the browser flow finishes. @@ -1145,14 +1145,29 @@ The server also emits `skills/changed` notifications when watched local skill fi } ``` -To enable or disable a skill by path: +To enable or disable a skill by absolute path: ```json { "method": "skills/config/write", "id": 26, "params": { - "path": "/Users/me/.codex/skills/skill-creator/SKILL.md", + "path": "/Users/alice/.codex/skills/skill-creator/SKILL.md", + "name": null, + "enabled": false + } +} +``` + +To enable or disable a skill by name: + +```json +{ + "method": "skills/config/write", + "id": 27, + "params": { + "path": null, + "name": "github:yeet", "enabled": false } } diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 8917f392d..947a49ff2 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -5672,7 +5672,7 @@ impl CodexMessageProcessor { interface: outcome.plugin.interface.map(plugin_interface_to_info), }, description: outcome.plugin.description, - skills: plugin_skills_to_info(&visible_skills), + skills: plugin_skills_to_info(&visible_skills, &outcome.plugin.disabled_skill_paths), apps: app_summaries, mcp_servers: outcome.plugin.mcp_server_names, }; @@ -5687,8 +5687,30 @@ impl CodexMessageProcessor { request_id: ConnectionRequestId, params: SkillsConfigWriteParams, ) { - let SkillsConfigWriteParams { path, enabled } = params; - let edits = vec![ConfigEdit::SetSkillConfig { path, enabled }]; + let SkillsConfigWriteParams { + path, + name, + enabled, + } = params; + let edit = match (path, name) { + (Some(path), None) => ConfigEdit::SetSkillConfig { + path: path.into_path_buf(), + enabled, + }, + (None, Some(name)) if !name.trim().is_empty() => { + ConfigEdit::SetSkillConfigByName { name, enabled } + } + _ => { + let error = JSONRPCErrorError { + code: INVALID_PARAMS_ERROR_CODE, + message: "skills/config/write requires exactly one of path or name".to_string(), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + return; + } + }; + let edits = vec![edit]; let result = ConfigEditsBuilder::new(&self.config.codex_home) .with_edits(edits) .apply() @@ -5696,6 +5718,7 @@ impl CodexMessageProcessor { match result { Ok(()) => { + self.thread_manager.plugins_manager().clear_cache(); self.thread_manager.skills_manager().clear_cache(); self.outgoing .send_response( @@ -7669,7 +7692,10 @@ fn skills_to_info( .collect() } -fn plugin_skills_to_info(skills: &[codex_core::skills::SkillMetadata]) -> Vec { +fn plugin_skills_to_info( + skills: &[codex_core::skills::SkillMetadata], + disabled_skill_paths: &std::collections::HashSet, +) -> Vec { skills .iter() .map(|skill| SkillSummary { @@ -7687,6 +7713,7 @@ fn plugin_skills_to_info(skills: &[codex_core::skills::SkillMetadata]) -> Vec), - /// Set or clear a skill config entry under `[[skills.config]]`. + /// Set or clear a skill config entry under `[[skills.config]]` by path. SetSkillConfig { path: PathBuf, enabled: bool }, + /// Set or clear a skill config entry under `[[skills.config]]` by name. + SetSkillConfigByName { name: String, enabled: bool }, /// Set trust_level under `[projects.""]`, /// migrating inline tables to explicit tables. SetProjectTrustLevel { path: PathBuf, level: TrustLevel }, @@ -60,6 +62,12 @@ pub enum ConfigEdit { ClearPath { segments: Vec }, } +#[derive(Clone, Debug, PartialEq, Eq)] +enum SkillConfigSelector { + Name(String), + Path(PathBuf), +} + /// Produces a config edit that sets `[tui].theme = ""`. pub fn syntax_theme_edit(name: &str) -> ConfigEdit { ConfigEdit::SetPath { @@ -387,7 +395,10 @@ impl ConfigDocument { )), ConfigEdit::ReplaceMcpServers(servers) => Ok(self.replace_mcp_servers(servers)), ConfigEdit::SetSkillConfig { path, enabled } => { - Ok(self.set_skill_config(path.as_path(), *enabled)) + Ok(self.set_skill_config(SkillConfigSelector::Path(path.clone()), *enabled)) + } + ConfigEdit::SetSkillConfigByName { name, enabled } => { + Ok(self.set_skill_config(SkillConfigSelector::Name(name.clone()), *enabled)) } ConfigEdit::SetPath { segments, value } => Ok(self.insert(segments, value.clone())), ConfigEdit::ClearPath { segments } => Ok(self.clear_owned(segments)), @@ -478,8 +489,16 @@ impl ConfigDocument { true } - fn set_skill_config(&mut self, path: &Path, enabled: bool) -> bool { - let normalized_path = normalize_skill_config_path(path); + fn set_skill_config(&mut self, selector: SkillConfigSelector, enabled: bool) -> bool { + let selector = match selector { + SkillConfigSelector::Name(name) => SkillConfigSelector::Name(name.trim().to_string()), + SkillConfigSelector::Path(path) => { + SkillConfigSelector::Path(PathBuf::from(normalize_skill_config_path(&path))) + } + }; + if matches!(&selector, SkillConfigSelector::Name(name) if name.is_empty()) { + return false; + } let mut remove_skills_table = false; let mut mutated = false; @@ -538,12 +557,8 @@ impl ConfigDocument { }; let existing_index = overrides.iter().enumerate().find_map(|(idx, table)| { - table - .get("path") - .and_then(|item| item.as_str()) - .map(Path::new) - .map(normalize_skill_config_path) - .filter(|value| *value == normalized_path) + skill_config_selector_from_table(table) + .filter(|value| value == &selector) .map(|_| idx) }); @@ -561,7 +576,7 @@ impl ConfigDocument { } else if let Some(index) = existing_index { for (idx, table) in overrides.iter_mut().enumerate() { if idx == index { - table["path"] = value(normalized_path); + write_skill_config_selector(table, &selector); table["enabled"] = value(false); mutated = true; break; @@ -570,7 +585,7 @@ impl ConfigDocument { } else { let mut entry = TomlTable::new(); entry.set_implicit(false); - entry["path"] = value(normalized_path); + write_skill_config_selector(&mut entry, &selector); entry["enabled"] = value(false); overrides.push(entry); mutated = true; @@ -699,6 +714,38 @@ fn normalize_skill_config_path(path: &Path) -> String { .to_string() } +fn skill_config_selector_from_table(table: &TomlTable) -> Option { + let path = table + .get("path") + .and_then(|item| item.as_str()) + .map(Path::new) + .map(|path| SkillConfigSelector::Path(PathBuf::from(normalize_skill_config_path(path)))); + let name = table + .get("name") + .and_then(|item| item.as_str()) + .map(str::trim) + .filter(|name| !name.is_empty()) + .map(|name| SkillConfigSelector::Name(name.to_string())); + + match (path, name) { + (Some(selector), None) | (None, Some(selector)) => Some(selector), + _ => None, + } +} + +fn write_skill_config_selector(table: &mut TomlTable, selector: &SkillConfigSelector) { + match selector { + SkillConfigSelector::Name(name) => { + table.remove("path"); + table["name"] = value(name.clone()); + } + SkillConfigSelector::Path(path) => { + table.remove("name"); + table["path"] = value(path.to_string_lossy().to_string()); + } + } +} + /// Persist edits using a blocking strategy. pub fn apply_blocking( codex_home: &Path, diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index 5a31d84dd..632716f00 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -110,6 +110,27 @@ enabled = false assert_eq!(contents, ""); } +#[test] +fn set_skill_config_writes_name_selector_entry() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + + ConfigEditsBuilder::new(codex_home) + .with_edits([ConfigEdit::SetSkillConfigByName { + name: "github:yeet".to_string(), + enabled: false, + }]) + .apply_blocking() + .expect("persist"); + + let contents = std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); + let expected = r#"[[skills.config]] +name = "github:yeet" +enabled = false +"#; + assert_eq!(contents, expected); +} + #[test] fn blocking_set_model_preserves_inline_table_contents() { let tmp = tempdir().expect("tmpdir"); diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 3b20779cd..7a94700ce 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -804,7 +804,12 @@ impl Notice { #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct SkillConfig { - pub path: AbsolutePathBuf, + /// Path-based selector. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub path: Option, + /// Name-based selector. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub name: Option, pub enabled: bool, } diff --git a/codex-rs/core/src/config/types_tests.rs b/codex-rs/core/src/config/types_tests.rs index adb65e167..dde0cd18f 100644 --- a/codex-rs/core/src/config/types_tests.rs +++ b/codex-rs/core/src/config/types_tests.rs @@ -243,6 +243,47 @@ fn deserialize_server_config_with_tool_filters() { assert_eq!(cfg.disabled_tools, Some(vec!["blocked".to_string()])); } +#[test] +fn deserialize_skill_config_with_name_selector() { + let cfg: SkillConfig = toml::from_str( + r#" + name = "github:yeet" + enabled = false + "#, + ) + .expect("should deserialize skill config with name selector"); + + assert_eq!(cfg.name.as_deref(), Some("github:yeet")); + assert_eq!(cfg.path, None); + assert!(!cfg.enabled); +} + +#[test] +fn deserialize_skill_config_with_path_selector() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let cfg: SkillConfig = toml::from_str(&format!( + r#" + path = {path:?} + enabled = false + "#, + path = skill_path.display().to_string(), + )) + .expect("should deserialize skill config with path selector"); + + assert_eq!( + cfg, + SkillConfig { + path: Some( + AbsolutePathBuf::from_absolute_path(&skill_path) + .expect("skill path should be absolute"), + ), + name: None, + enabled: false, + } + ); +} + #[test] fn deserialize_rejects_command_and_url() { toml::from_str::( diff --git a/codex-rs/core/src/plugins/manager.rs b/codex-rs/core/src/plugins/manager.rs index f02758d27..571535b5f 100644 --- a/codex-rs/core/src/plugins/manager.rs +++ b/codex-rs/core/src/plugins/manager.rs @@ -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, + pub disabled_skill_paths: HashSet, pub apps: Vec, pub mcp_server_names: Vec, } @@ -183,6 +186,8 @@ pub struct LoadedPlugin { pub root: AbsolutePathBuf, pub enabled: bool, pub skill_roots: Vec, + pub disabled_skill_paths: HashSet, + pub has_enabled_skills: bool, pub mcp_servers: HashMap, pub apps: Vec, pub error: Option, @@ -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 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, ) -> 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::::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, + 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, + disabled_skill_paths: HashSet, + 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, + 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::>(); + 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 { let mut paths = default_skill_roots(plugin_root); if let Some(path) = &manifest_paths.skills { diff --git a/codex-rs/core/src/plugins/manager_tests.rs b/codex-rs/core/src/plugins/manager_tests.rs index cd8541a85..630808dc0 100644 --- a/codex-rs/core/src/plugins/manager_tests.rs +++ b/codex-rs/core/src/plugins/manager_tests.rs @@ -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()); } diff --git a/codex-rs/core/src/plugins/store.rs b/codex-rs/core/src/plugins/store.rs index 262ca1028..97b0ccefe 100644 --- a/codex-rs/core/src/plugins/store.rs +++ b/codex-rs/core/src/plugins/store.rs @@ -110,10 +110,15 @@ impl PluginStore { .filter(|version| validate_plugin_segment(version, "plugin version").is_ok()) .collect::>(); 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() } } diff --git a/codex-rs/core/src/plugins/store_tests.rs b/codex-rs/core/src/plugins/store_tests.rs index b1da11a8a..61c346025 100644 --- a/codex-rs/core/src/plugins/store_tests.rs +++ b/codex-rs/core/src/plugins/store_tests.rs @@ -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(); diff --git a/codex-rs/core/src/skills/config_rules.rs b/codex-rs/core/src/skills/config_rules.rs new file mode 100644 index 000000000..fa1be878b --- /dev/null +++ b/codex-rs/core/src/skills/config_rules.rs @@ -0,0 +1,135 @@ +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; + +use codex_app_server_protocol::ConfigLayerSource; +use tracing::warn; + +use crate::config::types::SkillConfig; +use crate::config::types::SkillsConfig; +use crate::config_loader::ConfigLayerStack; +use crate::config_loader::ConfigLayerStackOrdering; +use crate::skills::SkillMetadata; + +#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] +pub(crate) enum SkillConfigRuleSelector { + Name(String), + Path(PathBuf), +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub(crate) struct SkillConfigRule { + pub selector: SkillConfigRuleSelector, + pub enabled: bool, +} + +#[derive(Debug, Clone, Default, PartialEq, Eq, Hash)] +pub(crate) struct SkillConfigRules { + pub entries: Vec, +} + +pub(crate) fn skill_config_rules_from_stack( + config_layer_stack: &ConfigLayerStack, +) -> SkillConfigRules { + let mut entries = Vec::new(); + for layer in config_layer_stack.get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ true, + ) { + if !matches!( + layer.name, + ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags + ) { + continue; + } + + let Some(skills_value) = layer.config.get("skills") else { + continue; + }; + let skills: SkillsConfig = match skills_value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + warn!("invalid skills config: {err}"); + continue; + } + }; + + for entry in skills.config { + let Some(selector) = skill_config_rule_selector(&entry) else { + continue; + }; + // Preserve layer order so a later name selector can override an earlier path selector + // for the same loaded skill. + entries.retain(|entry: &SkillConfigRule| entry.selector != selector); + entries.push(SkillConfigRule { + selector, + enabled: entry.enabled, + }); + } + } + + SkillConfigRules { entries } +} + +pub(crate) fn resolve_disabled_skill_paths( + skills: &[SkillMetadata], + rules: &SkillConfigRules, +) -> HashSet { + let mut disabled_paths = HashSet::new(); + + for entry in &rules.entries { + match &entry.selector { + SkillConfigRuleSelector::Path(path) => { + if entry.enabled { + disabled_paths.remove(path); + } else { + disabled_paths.insert(path.clone()); + } + } + SkillConfigRuleSelector::Name(name) => { + for path in skills + .iter() + .filter(|skill| skill.name == *name) + .map(|skill| skill.path_to_skills_md.clone()) + { + if entry.enabled { + disabled_paths.remove(&path); + } else { + disabled_paths.insert(path); + } + } + } + } + } + + disabled_paths +} + +fn skill_config_rule_selector(entry: &SkillConfig) -> Option { + match (entry.path.as_ref(), entry.name.as_deref()) { + (Some(path), None) => Some(SkillConfigRuleSelector::Path(normalize_rule_path( + path.as_path(), + ))), + (None, Some(name)) => { + let name = name.trim(); + if name.is_empty() { + warn!("ignoring empty skills.config name override"); + None + } else { + Some(SkillConfigRuleSelector::Name(name.to_string())) + } + } + (Some(_), Some(_)) => { + warn!("ignoring skills.config entry with both path and name selectors"); + None + } + (None, None) => { + warn!("ignoring skills.config entry without a path or name selector"); + None + } + } +} + +fn normalize_rule_path(path: &Path) -> PathBuf { + dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) +} diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index 982780f82..446580b6c 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -5,7 +5,6 @@ use std::path::PathBuf; use std::sync::Arc; use std::sync::RwLock; -use codex_app_server_protocol::ConfigLayerSource; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -16,12 +15,14 @@ use tracing::warn; use crate::config::Config; use crate::config::types::SkillsConfig; use crate::config_loader::CloudRequirementsLoader; -use crate::config_loader::ConfigLayerStackOrdering; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::plugins::PluginsManager; use crate::skills::SkillLoadOutcome; use crate::skills::build_implicit_skill_path_indexes; +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 crate::skills::loader::skill_roots; @@ -81,15 +82,13 @@ impl SkillsManager { /// to share a directory. pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome { let roots = self.skill_roots_for_config(config); - let cache_key = config_skills_cache_key(&roots, &config.config_layer_stack); + let skill_config_rules = skill_config_rules_from_stack(&config.config_layer_stack); + let cache_key = config_skills_cache_key(&roots, &skill_config_rules); if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { return outcome; } - let outcome = crate::skills::filter_skill_load_outcome_for_product( - finalize_skill_outcome(load_skills_from_roots(roots), &config.config_layer_stack), - self.restriction_product, - ); + let outcome = self.build_skill_outcome(roots, &skill_config_rules); let mut cache = self .cache_by_config .write() @@ -192,7 +191,8 @@ impl SkillsManager { scope: SkillScope::User, }), ); - let outcome = self.build_skill_outcome(roots, &config_layer_stack); + let skill_config_rules = skill_config_rules_from_stack(&config_layer_stack); + let outcome = self.build_skill_outcome(roots, &skill_config_rules); let mut cache = self .cache_by_cwd .write() @@ -204,12 +204,14 @@ impl SkillsManager { fn build_skill_outcome( &self, roots: Vec, - config_layer_stack: &crate::config_loader::ConfigLayerStack, + skill_config_rules: &SkillConfigRules, ) -> SkillLoadOutcome { - crate::skills::filter_skill_load_outcome_for_product( - finalize_skill_outcome(load_skills_from_roots(roots), config_layer_stack), + let outcome = crate::skills::filter_skill_load_outcome_for_product( + load_skills_from_roots(roots), self.restriction_product, - ) + ); + let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules); + finalize_skill_outcome(outcome, disabled_paths) } pub fn clear_cache(&self) { @@ -256,7 +258,7 @@ impl SkillsManager { #[derive(Debug, Clone, PartialEq, Eq, Hash)] struct ConfigSkillsCacheKey { roots: Vec<(PathBuf, u8)>, - disabled_paths: Vec, + skill_config_rules: SkillConfigRules, } pub(crate) fn bundled_skills_enabled_from_stack( @@ -281,53 +283,10 @@ pub(crate) fn bundled_skills_enabled_from_stack( skills.bundled.unwrap_or_default().enabled } -fn disabled_paths_from_stack( - config_layer_stack: &crate::config_loader::ConfigLayerStack, -) -> HashSet { - let mut configs = HashMap::new(); - for layer in config_layer_stack.get_layers( - ConfigLayerStackOrdering::LowestPrecedenceFirst, - /*include_disabled*/ true, - ) { - if !matches!( - layer.name, - ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags - ) { - continue; - } - - let Some(skills_value) = layer.config.get("skills") else { - continue; - }; - let skills: SkillsConfig = match skills_value.clone().try_into() { - Ok(skills) => skills, - Err(err) => { - warn!("invalid skills config: {err}"); - continue; - } - }; - - for entry in skills.config { - let path = normalize_override_path(entry.path.as_path()); - configs.insert(path, entry.enabled); - } - } - - configs - .into_iter() - .filter_map(|(path, enabled)| (!enabled).then_some(path)) - .collect() -} - fn config_skills_cache_key( roots: &[SkillRoot], - config_layer_stack: &crate::config_loader::ConfigLayerStack, + skill_config_rules: &SkillConfigRules, ) -> ConfigSkillsCacheKey { - let mut disabled_paths: Vec = disabled_paths_from_stack(config_layer_stack) - .into_iter() - .collect(); - disabled_paths.sort_unstable(); - ConfigSkillsCacheKey { roots: roots .iter() @@ -341,15 +300,15 @@ fn config_skills_cache_key( (root.path.clone(), scope_rank) }) .collect(), - disabled_paths, + skill_config_rules: skill_config_rules.clone(), } } fn finalize_skill_outcome( mut outcome: SkillLoadOutcome, - config_layer_stack: &crate::config_loader::ConfigLayerStack, + disabled_paths: HashSet, ) -> SkillLoadOutcome { - outcome.disabled_paths = disabled_paths_from_stack(config_layer_stack); + outcome.disabled_paths = disabled_paths; let (by_scripts_dir, by_doc_path) = build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()); outcome.implicit_skills_by_scripts_dir = Arc::new(by_scripts_dir); @@ -357,10 +316,6 @@ fn finalize_skill_outcome( outcome } -fn normalize_override_path(path: &Path) -> PathBuf { - dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) -} - fn normalize_extra_user_roots(extra_user_roots: &[PathBuf]) -> Vec { let mut normalized: Vec = extra_user_roots .iter() diff --git a/codex-rs/core/src/skills/manager_tests.rs b/codex-rs/core/src/skills/manager_tests.rs index cb3c48ed7..d9e5859f4 100644 --- a/codex-rs/core/src/skills/manager_tests.rs +++ b/codex-rs/core/src/skills/manager_tests.rs @@ -5,6 +5,10 @@ use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirementsToml; use crate::plugins::PluginsManager; +use crate::skills::SkillMetadata; +use crate::skills::config_rules::resolve_disabled_skill_paths; +use crate::skills::config_rules::skill_config_rules_from_stack; +use codex_app_server_protocol::ConfigLayerSource; use pretty_assertions::assert_eq; use std::fs; use std::path::PathBuf; @@ -17,6 +21,49 @@ fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &s fs::write(skill_dir.join("SKILL.md"), content).unwrap(); } +fn write_plugin_skill( + codex_home: &TempDir, + marketplace: &str, + plugin_name: &str, + dir: &str, + name: &str, + description: &str, +) -> PathBuf { + let plugin_root = codex_home + .path() + .join("plugins/cache") + .join(marketplace) + .join(plugin_name) + .join("local"); + let skill_dir = plugin_root.join("skills").join(dir); + fs::create_dir_all(plugin_root.join(".codex-plugin")).unwrap(); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + plugin_root.join(".codex-plugin/plugin.json"), + format!(r#"{{"name":"{plugin_name}"}}"#), + ) + .unwrap(); + let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n"); + let skill_path = skill_dir.join("SKILL.md"); + fs::write(&skill_path, content).unwrap(); + skill_path +} + +fn test_skill(name: &str, path: PathBuf) -> SkillMetadata { + SkillMetadata { + name: name.to_string(), + description: "test".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permission_profile: None, + managed_network_override: None, + path_to_skills_md: path, + scope: SkillScope::User, + } +} + #[test] fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -68,6 +115,68 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() { assert_eq!(outcome2.skills, outcome1.skills); } +#[tokio::test] +async fn skills_for_config_disables_plugin_skills_by_name() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let skill_path = write_plugin_skill( + &codex_home, + "test", + "sample", + "sample-search", + "sample-search", + "search sample data", + ); + fs::write( + codex_home.path().join(crate::config::CONFIG_TOML_FILE), + r#"[features] +plugins = true + +[[skills.config]] +name = "sample:sample-search" +enabled = false + +[plugins."sample@test"] +enabled = true +"#, + ) + .expect("write config"); + + let config = ConfigBuilder::default() + .codex_home(codex_home.path().to_path_buf()) + .harness_overrides(ConfigOverrides { + cwd: Some(cwd.path().to_path_buf()), + ..Default::default() + }) + .build() + .await + .expect("load config"); + + let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf())); + let skills_manager = SkillsManager::new( + codex_home.path().to_path_buf(), + plugins_manager, + config.bundled_skills_enabled(), + ); + + let outcome = skills_manager.skills_for_config(&config); + let skill = outcome + .skills + .iter() + .find(|skill| skill.name == "sample:sample-search") + .expect("plugin skill should load"); + let skill_path = dunce::canonicalize(skill_path).expect("skill path should canonicalize"); + + assert_eq!(skill.path_to_skills_md, skill_path); + assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md)); + assert!( + !outcome + .allowed_skills_for_implicit_invocation() + .iter() + .any(|allowed_skill| allowed_skill.path_to_skills_md == skill.path_to_skills_md) + ); +} + #[tokio::test] async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { let codex_home = tempfile::tempdir().expect("tempdir"); @@ -282,9 +391,10 @@ fn normalize_extra_user_roots_is_stable_for_equivalent_inputs() { #[cfg_attr(windows, ignore)] #[test] -fn disabled_paths_from_stack_allows_session_flags_to_override_user_layer() { +fn disabled_paths_for_skills_allows_session_flags_to_override_user_layer() { let tempdir = tempfile::tempdir().expect("tempdir"); let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("demo-skill", skill_path.clone()); let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) .expect("user config path should be absolute"); let user_layer = ConfigLayerEntry::new( @@ -316,14 +426,19 @@ enabled = true ) .expect("valid config layer stack"); - assert_eq!(disabled_paths_from_stack(&stack), HashSet::new()); + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::new() + ); } #[cfg_attr(windows, ignore)] #[test] -fn disabled_paths_from_stack_allows_session_flags_to_disable_user_enabled_skill() { +fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill() { let tempdir = tempfile::tempdir().expect("tempdir"); let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("demo-skill", skill_path.clone()); let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) .expect("user config path should be absolute"); let user_layer = ConfigLayerEntry::new( @@ -355,12 +470,88 @@ enabled = false ) .expect("valid config layer stack"); + let skill_config_rules = skill_config_rules_from_stack(&stack); assert_eq!( - disabled_paths_from_stack(&stack), + resolve_disabled_skill_paths(&[skill], &skill_config_rules), HashSet::from([skill_path]) ); } +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_disables_matching_name_selectors() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("github:yeet", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str( + r#"[[skills.config]] +name = "github:yeet" +enabled = false +"#, + ) + .expect("user layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::from([skill_path]) + ); +} + +#[cfg_attr(windows, ignore)] +#[test] +fn disabled_paths_for_skills_allows_name_selector_to_override_path_selector() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_path = tempdir.path().join("skills").join("demo").join("SKILL.md"); + let skill = test_skill("github:yeet", skill_path.clone()); + let user_file = AbsolutePathBuf::try_from(tempdir.path().join("config.toml")) + .expect("user config path should be absolute"); + let user_layer = ConfigLayerEntry::new( + ConfigLayerSource::User { file: user_file }, + toml::from_str(&format!( + r#"[[skills.config]] +path = "{}" +enabled = false +"#, + skill_path.display() + )) + .expect("user layer toml"), + ); + let session_layer = ConfigLayerEntry::new( + ConfigLayerSource::SessionFlags, + toml::from_str( + r#"[[skills.config]] +name = "github:yeet" +enabled = true +"#, + ) + .expect("session layer toml"), + ); + let stack = ConfigLayerStack::new( + vec![user_layer, session_layer], + Default::default(), + ConfigRequirementsToml::default(), + ) + .expect("valid config layer stack"); + + let skill_config_rules = skill_config_rules_from_stack(&stack); + assert_eq!( + resolve_disabled_skill_paths(&[skill], &skill_config_rules), + HashSet::new() + ); +} + #[cfg_attr(windows, ignore)] #[tokio::test] async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill() { diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index 4138ecbb8..ad76cd9da 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -1,3 +1,4 @@ +pub(crate) mod config_rules; mod env_var_dependencies; pub mod injection; pub(crate) mod invocation_utils;