diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index dd54eb25d..bce2e48c9 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -133,6 +133,10 @@ client_request_definitions! { params: v2::SkillsListParams, response: v2::SkillsListResponse, }, + SkillsConfigWrite => "skills/config/write" { + params: v2::SkillsConfigWriteParams, + response: v2::SkillsConfigWriteResponse, + }, TurnStart => "turn/start" { params: v2::TurnStartParams, response: v2::TurnStartResponse, diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index b44e1575a..2855b6f57 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1274,6 +1274,7 @@ pub struct SkillMetadata { pub interface: Option, pub path: PathBuf, pub scope: SkillScope, + pub enabled: bool, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -1311,6 +1312,21 @@ pub struct SkillsListEntry { pub errors: Vec, } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct SkillsConfigWriteParams { + pub path: PathBuf, + pub enabled: bool, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct SkillsConfigWriteResponse { + pub effective_enabled: bool, +} + impl From for SkillMetadata { fn from(value: CoreSkillMetadata) -> Self { Self { @@ -1320,6 +1336,7 @@ impl From for SkillMetadata { interface: value.interface.map(SkillInterface::from), path: value.path, scope: value.scope.into(), + enabled: true, } } } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 75e1853d7..891632796 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -87,6 +87,7 @@ Example (from OpenAI's official VSCode extension): - `command/exec` — run a single command under the server sandbox without starting a thread/turn (handy for utilities and validation). - `model/list` — list available models (with reasoning effort options). - `skills/list` — list skills for one or more `cwd` values (optional `forceReload`). +- `skills/config/write` — write user-level skill config by path. - `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. - `config/mcpServer/reload` — reload MCP server config from disk and queue a refresh for loaded threads (applied on each thread's next active turn); returns `{}`. Use this after editing `config.toml` without restarting the server. - `mcpServerStatus/list` — enumerate configured MCP servers with their tools, resources, resource templates, and auth status; supports cursor+limit pagination. @@ -483,17 +484,30 @@ Example: $skill-creator Add a new skill for triaging flaky CI and include step-by-step usage. ``` -Use `skills/list` to fetch the available skills (optionally scoped by `cwd` and/or with `forceReload`). +Use `skills/list` to fetch the available skills (optionally scoped by `cwds`, with `forceReload`). ```json { "method": "skills/list", "id": 25, "params": { - "cwd": "/Users/me/project", + "cwds": ["/Users/me/project"], "forceReload": false } } { "id": 25, "result": { - "skills": [ - { "name": "skill-creator", "description": "Create or update a Codex skill" } - ] + "data": [{ + "cwd": "/Users/me/project", + "skills": [ + { "name": "skill-creator", "description": "Create or update a Codex skill", "enabled": true } + ], + "errors": [] + }] +} } +``` + +To enable or disable a skill by path: + +```json +{ "method": "skills/config/write", "id": 26, "params": { + "path": "/Users/me/.codex/skills/skill-creator/SKILL.md", + "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 153028512..fb6699a34 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -85,6 +85,8 @@ use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::SessionConfiguredNotification; use codex_app_server_protocol::SetDefaultModelParams; use codex_app_server_protocol::SetDefaultModelResponse; +use codex_app_server_protocol::SkillsConfigWriteParams; +use codex_app_server_protocol::SkillsConfigWriteResponse; use codex_app_server_protocol::SkillsListParams; use codex_app_server_protocol::SkillsListResponse; use codex_app_server_protocol::Thread; @@ -131,6 +133,7 @@ use codex_core::auth::login_with_api_key; use codex_core::config::Config; use codex_core::config::ConfigOverrides; use codex_core::config::ConfigService; +use codex_core::config::edit::ConfigEdit; use codex_core::config::edit::ConfigEditsBuilder; use codex_core::config::types::McpServerTransportConfig; use codex_core::default_client::get_codex_user_agent; @@ -400,6 +403,9 @@ impl CodexMessageProcessor { ClientRequest::SkillsList { request_id, params } => { self.skills_list(request_id, params).await; } + ClientRequest::SkillsConfigWrite { request_id, params } => { + self.skills_config_write(request_id, params).await; + } ClientRequest::TurnStart { request_id, params } => { self.turn_start(request_id, params).await; } @@ -3246,7 +3252,7 @@ impl CodexMessageProcessor { for cwd in cwds { let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; let errors = errors_to_info(&outcome.errors); - let skills = skills_to_info(&outcome.skills); + let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths); data.push(codex_app_server_protocol::SkillsListEntry { cwd, skills, @@ -3258,6 +3264,37 @@ impl CodexMessageProcessor { .await; } + async fn skills_config_write(&self, request_id: RequestId, params: SkillsConfigWriteParams) { + let SkillsConfigWriteParams { path, enabled } = params; + let edits = vec![ConfigEdit::SetSkillConfig { path, enabled }]; + let result = ConfigEditsBuilder::new(&self.config.codex_home) + .with_edits(edits) + .apply() + .await; + + match result { + Ok(()) => { + self.thread_manager.skills_manager().clear_cache(); + self.outgoing + .send_response( + request_id, + SkillsConfigWriteResponse { + effective_enabled: enabled, + }, + ) + .await; + } + Err(err) => { + let error = JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to update skill settings: {err}"), + data: None, + }; + self.outgoing.send_error(request_id, error).await; + } + } + } + async fn interrupt_conversation( &mut self, request_id: RequestId, @@ -3918,25 +3955,30 @@ impl CodexMessageProcessor { fn skills_to_info( skills: &[codex_core::skills::SkillMetadata], + disabled_paths: &std::collections::HashSet, ) -> Vec { skills .iter() - .map(|skill| codex_app_server_protocol::SkillMetadata { - name: skill.name.clone(), - description: skill.description.clone(), - short_description: skill.short_description.clone(), - interface: skill.interface.clone().map(|interface| { - codex_app_server_protocol::SkillInterface { - display_name: interface.display_name, - short_description: interface.short_description, - icon_small: interface.icon_small, - icon_large: interface.icon_large, - brand_color: interface.brand_color, - default_prompt: interface.default_prompt, - } - }), - path: skill.path.clone(), - scope: skill.scope.into(), + .map(|skill| { + let enabled = !disabled_paths.contains(&skill.path); + codex_app_server_protocol::SkillMetadata { + name: skill.name.clone(), + description: skill.description.clone(), + short_description: skill.short_description.clone(), + interface: skill.interface.clone().map(|interface| { + codex_app_server_protocol::SkillInterface { + display_name: interface.display_name, + short_description: interface.short_description, + icon_small: interface.icon_small, + icon_large: interface.icon_large, + brand_color: interface.brand_color, + default_prompt: interface.default_prompt, + } + }), + path: skill.path.clone(), + scope: skill.scope.into(), + enabled, + } }) .collect() } diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 37a0cce70..e954d88c1 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -75,6 +75,9 @@ "apply_patch_freeform": { "type": "boolean" }, + "child_agents_md": { + "type": "boolean" + }, "collab": { "type": "boolean" }, @@ -99,9 +102,6 @@ "experimental_windows_sandbox": { "type": "boolean" }, - "child_agents_md": { - "type": "boolean" - }, "include_apply_patch_tool": { "type": "boolean" }, @@ -373,6 +373,14 @@ "description": "When set to `true`, `AgentReasoningRawContentEvent` events will be shown in the UI/output. Defaults to `false`.", "type": "boolean" }, + "skills": { + "description": "User-level skill config entries keyed by SKILL.md path.", + "allOf": [ + { + "$ref": "#/definitions/SkillsConfig" + } + ] + }, "tool_output_token_limit": { "description": "Token budget applied when storing tool/function outputs in the context manager.", "type": "integer", @@ -543,6 +551,9 @@ "apply_patch_freeform": { "type": "boolean" }, + "child_agents_md": { + "type": "boolean" + }, "collab": { "type": "boolean" }, @@ -567,9 +578,6 @@ "experimental_windows_sandbox": { "type": "boolean" }, - "child_agents_md": { - "type": "boolean" - }, "include_apply_patch_tool": { "type": "boolean" }, @@ -1288,6 +1296,34 @@ }, "additionalProperties": false }, + "SkillConfig": { + "type": "object", + "required": [ + "enabled", + "path" + ], + "properties": { + "enabled": { + "type": "boolean" + }, + "path": { + "$ref": "#/definitions/AbsolutePathBuf" + } + }, + "additionalProperties": false + }, + "SkillsConfig": { + "type": "object", + "properties": { + "config": { + "type": "array", + "items": { + "$ref": "#/definitions/SkillConfig" + } + } + }, + "additionalProperties": false + }, "ToolsToml": { "type": "object", "properties": { diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index b554450d1..b59856f26 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -251,7 +251,8 @@ impl Codex { ); } - let user_instructions = get_user_instructions(&config, Some(&loaded_skills.skills)).await; + let enabled_skills = loaded_skills.enabled_skills(); + let user_instructions = get_user_instructions(&config, Some(&enabled_skills)).await; let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack) .await @@ -2264,7 +2265,8 @@ mod handlers { for cwd in cwds { let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; let errors = super::errors_to_info(&outcome.errors); - let skills_metadata = super::skills_to_info(&outcome.skills); + let enabled_skills = outcome.enabled_skills(); + let skills_metadata = super::skills_to_info(&enabled_skills); skills.push(SkillsListEntry { cwd, skills: skills_metadata, diff --git a/codex-rs/core/src/config/edit.rs b/codex-rs/core/src/config/edit.rs index fb6829bb9..f97903b31 100644 --- a/codex-rs/core/src/config/edit.rs +++ b/codex-rs/core/src/config/edit.rs @@ -9,6 +9,7 @@ use std::path::Path; use std::path::PathBuf; use tempfile::NamedTempFile; use tokio::task; +use toml_edit::ArrayOfTables; use toml_edit::DocumentMut; use toml_edit::Item as TomlItem; use toml_edit::Table as TomlTable; @@ -36,6 +37,8 @@ pub enum ConfigEdit { RecordModelMigrationSeen { from: String, to: String }, /// Replace the entire `[mcp_servers]` table. ReplaceMcpServers(BTreeMap), + /// Set or clear a skill config entry under `[[skills.config]]`. + SetSkillConfig { path: PathBuf, enabled: bool }, /// Set trust_level under `[projects.""]`, /// migrating inline tables to explicit tables. SetProjectTrustLevel { path: PathBuf, level: TrustLevel }, @@ -298,6 +301,9 @@ impl ConfigDocument { value(*acknowledged), )), ConfigEdit::ReplaceMcpServers(servers) => Ok(self.replace_mcp_servers(servers)), + ConfigEdit::SetSkillConfig { path, enabled } => { + Ok(self.set_skill_config(path.as_path(), *enabled)) + } ConfigEdit::SetPath { segments, value } => Ok(self.insert(segments, value.clone())), ConfigEdit::ClearPath { segments } => Ok(self.clear_owned(segments)), ConfigEdit::SetProjectTrustLevel { path, level } => { @@ -387,6 +393,113 @@ impl ConfigDocument { true } + fn set_skill_config(&mut self, path: &Path, enabled: bool) -> bool { + let normalized_path = normalize_skill_config_path(path); + let mut remove_skills_table = false; + let mut mutated = false; + + { + let root = self.doc.as_table_mut(); + let skills_item = match root.get_mut("skills") { + Some(item) => item, + None => { + if enabled { + return false; + } + root.insert( + "skills", + TomlItem::Table(document_helpers::new_implicit_table()), + ); + let Some(item) = root.get_mut("skills") else { + return false; + }; + item + } + }; + + if document_helpers::ensure_table_for_write(skills_item).is_none() { + if enabled { + return false; + } + *skills_item = TomlItem::Table(document_helpers::new_implicit_table()); + } + let Some(skills_table) = skills_item.as_table_mut() else { + return false; + }; + + let config_item = match skills_table.get_mut("config") { + Some(item) => item, + None => { + if enabled { + return false; + } + skills_table.insert("config", TomlItem::ArrayOfTables(ArrayOfTables::new())); + let Some(item) = skills_table.get_mut("config") else { + return false; + }; + item + } + }; + + if !matches!(config_item, TomlItem::ArrayOfTables(_)) { + if enabled { + return false; + } + *config_item = TomlItem::ArrayOfTables(ArrayOfTables::new()); + } + + let TomlItem::ArrayOfTables(overrides) = config_item else { + return false; + }; + + 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) + .map(|_| idx) + }); + + if enabled { + if let Some(index) = existing_index { + overrides.remove(index); + mutated = true; + if overrides.is_empty() { + skills_table.remove("config"); + if skills_table.is_empty() { + remove_skills_table = true; + } + } + } + } else if let Some(index) = existing_index { + for (idx, table) in overrides.iter_mut().enumerate() { + if idx == index { + table["path"] = value(normalized_path); + table["enabled"] = value(false); + mutated = true; + break; + } + } + } else { + let mut entry = TomlTable::new(); + entry.set_implicit(false); + entry["path"] = value(normalized_path); + entry["enabled"] = value(false); + overrides.push(entry); + mutated = true; + } + } + + if remove_skills_table { + let root = self.doc.as_table_mut(); + root.remove("skills"); + } + + mutated + } + fn scoped_segments(&self, scope: Scope, segments: &[&str]) -> Vec { let resolved: Vec = segments .iter() @@ -494,6 +607,13 @@ impl ConfigDocument { } } +fn normalize_skill_config_path(path: &Path) -> String { + dunce::canonicalize(path) + .unwrap_or_else(|_| path.to_path_buf()) + .to_string_lossy() + .to_string() +} + /// Persist edits using a blocking strategy. pub fn apply_blocking( codex_home: &Path, @@ -737,6 +857,54 @@ model_reasoning_effort = "high" assert_eq!(contents, "enabled = true\n"); } + #[test] + fn set_skill_config_writes_disabled_entry() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + + ConfigEditsBuilder::new(codex_home) + .with_edits([ConfigEdit::SetSkillConfig { + path: PathBuf::from("/tmp/skills/demo/SKILL.md"), + 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]] +path = "/tmp/skills/demo/SKILL.md" +enabled = false +"#; + assert_eq!(contents, expected); + } + + #[test] + fn set_skill_config_removes_entry_when_enabled() { + let tmp = tempdir().expect("tmpdir"); + let codex_home = tmp.path(); + std::fs::write( + codex_home.join(CONFIG_TOML_FILE), + r#"[[skills.config]] +path = "/tmp/skills/demo/SKILL.md" +enabled = false +"#, + ) + .expect("seed config"); + + ConfigEditsBuilder::new(codex_home) + .with_edits([ConfigEdit::SetSkillConfig { + path: PathBuf::from("/tmp/skills/demo/SKILL.md"), + enabled: true, + }]) + .apply_blocking() + .expect("persist"); + + let contents = + std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config"); + assert_eq!(contents, ""); + } + #[test] fn blocking_set_model_preserves_inline_table_contents() { let tmp = tempdir().expect("tmpdir"); diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6228bc3ac..956df12e9 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -13,6 +13,7 @@ use crate::config::types::SandboxWorkspaceWrite; use crate::config::types::ScrollInputMode; use crate::config::types::ShellEnvironmentPolicy; use crate::config::types::ShellEnvironmentPolicyToml; +use crate::config::types::SkillsConfig; use crate::config::types::Tui; use crate::config::types::UriBasedFileOpener; use crate::config_loader::ConfigLayerStack; @@ -911,6 +912,9 @@ pub struct ConfigToml { /// Nested tools section for feature toggles pub tools: Option, + /// User-level skill config entries keyed by SKILL.md path. + pub skills: Option, + /// Centralized feature flags (new). Prefer this over individual toggles. #[serde(default)] // Injects known feature keys into the schema and forbids unknown keys. diff --git a/codex-rs/core/src/config/types.rs b/codex-rs/core/src/config/types.rs index 9d75302cb..46c06d9a2 100644 --- a/codex-rs/core/src/config/types.rs +++ b/codex-rs/core/src/config/types.rs @@ -600,6 +600,20 @@ impl Notice { pub(crate) const TABLE_KEY: &'static str = "notice"; } +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct SkillConfig { + pub path: AbsolutePathBuf, + pub enabled: bool, +} + +#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)] +#[schemars(deny_unknown_fields)] +pub struct SkillsConfig { + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub config: Vec, +} + #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)] #[schemars(deny_unknown_fields)] pub struct SandboxWorkspaceWrite { diff --git a/codex-rs/core/src/skills/injection.rs b/codex-rs/core/src/skills/injection.rs index a143fce1f..dfd7e2af5 100644 --- a/codex-rs/core/src/skills/injection.rs +++ b/codex-rs/core/src/skills/injection.rs @@ -1,4 +1,5 @@ use std::collections::HashSet; +use std::path::PathBuf; use crate::skills::SkillLoadOutcome; use crate::skills::SkillMetadata; @@ -25,7 +26,8 @@ pub(crate) async fn build_skill_injections( return SkillInjections::default(); }; - let mentioned_skills = collect_explicit_skill_mentions(inputs, &outcome.skills); + let mentioned_skills = + collect_explicit_skill_mentions(inputs, &outcome.skills, &outcome.disabled_paths); if mentioned_skills.is_empty() { return SkillInjections::default(); } @@ -46,9 +48,9 @@ pub(crate) async fn build_skill_injections( } Err(err) => { let message = format!( - "Failed to load skill {} at {}: {err:#}", - skill.name, - skill.path.display() + "Failed to load skill {name} at {path}: {err:#}", + name = skill.name, + path = skill.path.display() ); result.warnings.push(message); } @@ -61,6 +63,7 @@ pub(crate) async fn build_skill_injections( fn collect_explicit_skill_mentions( inputs: &[UserInput], skills: &[SkillMetadata], + disabled_paths: &HashSet, ) -> Vec { let mut selected: Vec = Vec::new(); let mut seen: HashSet = HashSet::new(); @@ -69,6 +72,7 @@ fn collect_explicit_skill_mentions( if let UserInput::Skill { name, path } = input && seen.insert(name.clone()) && let Some(skill) = skills.iter().find(|s| s.name == *name && s.path == *path) + && !disabled_paths.contains(&skill.path) { selected.push(skill.clone()); } diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index da51fea41..f6dd3dae9 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -105,13 +105,13 @@ where discover_skills_under_root(&root.path, root.scope, &mut outcome); } - let mut seen: HashSet = HashSet::new(); + let mut seen: HashSet = HashSet::new(); outcome .skills - .retain(|skill| seen.insert(skill.name.clone())); + .retain(|skill| seen.insert(skill.path.clone())); fn scope_rank(scope: SkillScope) -> u8 { - // Higher-priority scopes first (matches dedupe priority order). + // Higher-priority scopes first (matches root scan order for dedupe). match scope { SkillScope::Repo => 0, SkillScope::User => 1, @@ -445,10 +445,23 @@ fn resolve_asset_path( return None; } - let mut components = path.components().peekable(); - while matches!(components.peek(), Some(Component::CurDir)) { - components.next(); + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::Normal(component) => normalized.push(component), + Component::ParentDir => { + tracing::warn!("ignoring {field}: icon path must not contain '..'"); + return None; + } + _ => { + tracing::warn!("ignoring {field}: icon path must be under assets/"); + return None; + } + } } + + let mut components = normalized.components(); match components.next() { Some(Component::Normal(component)) if component == "assets" => {} _ => { @@ -456,12 +469,8 @@ fn resolve_asset_path( return None; } } - if components.any(|component| matches!(component, Component::ParentDir)) { - tracing::warn!("ignoring {field}: icon path must not contain '..'"); - return None; - } - Some(skill_dir.join(path)) + Some(skill_dir.join(normalized)) } fn sanitize_single_line(raw: &str) -> String { @@ -707,8 +716,8 @@ default_prompt = " default prompt " interface: Some(SkillInterface { display_name: Some("UI Skill".to_string()), short_description: Some("short desc".to_string()), - icon_small: Some(normalized_skill_dir.join("./assets/small-400px.png")), - icon_large: Some(normalized_skill_dir.join("./assets/large-logo.svg")), + icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), + icon_large: Some(normalized_skill_dir.join("assets/large-logo.svg")), brand_color: Some("#3B82F6".to_string()), default_prompt: Some("default prompt".to_string()), }), @@ -753,7 +762,7 @@ icon_large = "./assets/logo.svg" display_name: Some("UI Skill".to_string()), short_description: None, icon_small: Some(normalized_skill_dir.join("assets/icon.png")), - icon_large: Some(normalized_skill_dir.join("./assets/logo.svg")), + icon_large: Some(normalized_skill_dir.join("assets/logo.svg")), brand_color: None, default_prompt: None, }), @@ -835,7 +844,7 @@ default_prompt = "{too_long}" interface: Some(SkillInterface { display_name: Some("UI Skill".to_string()), short_description: None, - icon_small: Some(normalized_skill_dir.join("./assets/small-400px.png")), + icon_small: Some(normalized_skill_dir.join("assets/small-400px.png")), icon_large: None, brand_color: None, default_prompt: None, @@ -1376,12 +1385,47 @@ icon_large = "./assets/../logo.svg" } #[tokio::test] - async fn deduplicates_by_name_preferring_repo_over_user() { + async fn deduplicates_by_path_preferring_first_root() { + let root = tempfile::tempdir().expect("tempdir"); + + let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); + + let outcome = load_skills_from_roots([ + SkillRoot { + path: root.path().to_path_buf(), + scope: SkillScope::Repo, + }, + SkillRoot { + path: root.path().to_path_buf(), + scope: SkillScope::User, + }, + ]); + + assert!( + outcome.errors.is_empty(), + "unexpected errors: {:?}", + outcome.errors + ); + assert_eq!( + outcome.skills, + vec![SkillMetadata { + name: "dupe-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + interface: None, + path: normalized(&skill_path), + scope: SkillScope::Repo, + }] + ); + } + + #[tokio::test] + async fn keeps_duplicate_names_from_repo_and_user() { let codex_home = tempfile::tempdir().expect("tempdir"); let repo_dir = tempfile::tempdir().expect("tempdir"); mark_as_git_repo(repo_dir.path()); - let _user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); + let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); let repo_skill_path = write_skill_at( &repo_dir .path() @@ -1402,42 +1446,94 @@ icon_large = "./assets/../logo.svg" ); assert_eq!( outcome.skills, - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from repo".to_string(), - short_description: None, - interface: None, - path: normalized(&repo_skill_path), - scope: SkillScope::Repo, - }] + vec![ + SkillMetadata { + name: "dupe-skill".to_string(), + description: "from repo".to_string(), + short_description: None, + interface: None, + path: normalized(&repo_skill_path), + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "dupe-skill".to_string(), + description: "from user".to_string(), + short_description: None, + interface: None, + path: normalized(&user_skill_path), + scope: SkillScope::User, + }, + ] ); } #[tokio::test] - async fn loads_system_skills_when_present() { + async fn keeps_duplicate_names_from_nested_codex_dirs() { let codex_home = tempfile::tempdir().expect("tempdir"); + let repo_dir = tempfile::tempdir().expect("tempdir"); + mark_as_git_repo(repo_dir.path()); - let _system_skill_path = - write_system_skill(&codex_home, "system", "dupe-skill", "from system"); - let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); + let nested_dir = repo_dir.path().join("nested/inner"); + fs::create_dir_all(&nested_dir).unwrap(); - let cfg = make_config(&codex_home).await; + let root_skill_path = write_skill_at( + &repo_dir + .path() + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "root", + "dupe-skill", + "from root", + ); + let nested_skill_path = write_skill_at( + &repo_dir + .path() + .join("nested") + .join(REPO_ROOT_CONFIG_DIR_NAME) + .join(SKILLS_DIR_NAME), + "nested", + "dupe-skill", + "from nested", + ); + + let cfg = make_config_for_cwd(&codex_home, nested_dir).await; let outcome = load_skills(&cfg); + assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", outcome.errors ); + let root_path = + canonicalize_path(&root_skill_path).unwrap_or_else(|_| root_skill_path.clone()); + let nested_path = + canonicalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone()); + let (first_path, second_path, first_description, second_description) = + if root_path <= nested_path { + (root_path, nested_path, "from root", "from nested") + } else { + (nested_path, root_path, "from nested", "from root") + }; assert_eq!( outcome.skills, - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from user".to_string(), - short_description: None, - interface: None, - path: normalized(&user_skill_path), - scope: SkillScope::User, - }] + vec![ + SkillMetadata { + name: "dupe-skill".to_string(), + description: first_description.to_string(), + short_description: None, + interface: None, + path: first_path, + scope: SkillScope::Repo, + }, + SkillMetadata { + name: "dupe-skill".to_string(), + description: second_description.to_string(), + short_description: None, + interface: None, + path: second_path, + scope: SkillScope::Repo, + }, + ] ); } @@ -1448,7 +1544,7 @@ icon_large = "./assets/../logo.svg" let repo_dir = outer_dir.path().join("repo"); fs::create_dir_all(&repo_dir).unwrap(); - write_skill_at( + let _skill_path = write_skill_at( &outer_dir .path() .join(REPO_ROOT_CONFIG_DIR_NAME) @@ -1457,7 +1553,6 @@ icon_large = "./assets/../logo.svg" "outer-skill", "from outer", ); - mark_as_git_repo(&repo_dir); let cfg = make_config_for_cwd(&codex_home, repo_dir).await; @@ -1581,164 +1676,4 @@ icon_large = "./assets/../logo.svg" } assert_eq!(scopes, expected); } - - #[tokio::test] - async fn deduplicates_by_name_preferring_system_over_admin() { - let system_dir = tempfile::tempdir().expect("tempdir"); - let admin_dir = tempfile::tempdir().expect("tempdir"); - - let system_skill_path = - write_skill_at(system_dir.path(), "system", "dupe-skill", "from system"); - let _admin_skill_path = - write_skill_at(admin_dir.path(), "admin", "dupe-skill", "from admin"); - - let outcome = load_skills_from_roots([ - SkillRoot { - path: system_dir.path().to_path_buf(), - scope: SkillScope::System, - }, - SkillRoot { - path: admin_dir.path().to_path_buf(), - scope: SkillScope::Admin, - }, - ]); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from system".to_string(), - short_description: None, - interface: None, - path: normalized(&system_skill_path), - scope: SkillScope::System, - }] - ); - } - - #[tokio::test] - async fn deduplicates_by_name_preferring_user_over_system() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let work_dir = tempfile::tempdir().expect("tempdir"); - - let user_skill_path = write_skill(&codex_home, "user", "dupe-skill", "from user"); - let _system_skill_path = - write_system_skill(&codex_home, "system", "dupe-skill", "from system"); - - let cfg = make_config_for_cwd(&codex_home, work_dir.path().to_path_buf()).await; - - let outcome = load_skills(&cfg); - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from user".to_string(), - short_description: None, - interface: None, - path: normalized(&user_skill_path), - scope: SkillScope::User, - }] - ); - } - - #[tokio::test] - async fn deduplicates_by_name_preferring_repo_over_system() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - - let repo_skill_path = write_skill_at( - &repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "repo", - "dupe-skill", - "from repo", - ); - let _system_skill_path = - write_system_skill(&codex_home, "system", "dupe-skill", "from system"); - - let cfg = make_config_for_cwd(&codex_home, repo_dir.path().to_path_buf()).await; - - let outcome = load_skills(&cfg); - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - assert_eq!( - outcome.skills, - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from repo".to_string(), - short_description: None, - interface: None, - path: normalized(&repo_skill_path), - scope: SkillScope::Repo, - }] - ); - } - - #[tokio::test] - async fn deduplicates_by_name_preferring_nearest_project_codex_dir() { - let codex_home = tempfile::tempdir().expect("tempdir"); - let repo_dir = tempfile::tempdir().expect("tempdir"); - mark_as_git_repo(repo_dir.path()); - - let nested_dir = repo_dir.path().join("nested/inner"); - fs::create_dir_all(&nested_dir).unwrap(); - - let _root_skill_path = write_skill_at( - &repo_dir - .path() - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "root", - "dupe-skill", - "from root", - ); - let nested_skill_path = write_skill_at( - &repo_dir - .path() - .join("nested") - .join(REPO_ROOT_CONFIG_DIR_NAME) - .join(SKILLS_DIR_NAME), - "nested", - "dupe-skill", - "from nested", - ); - - let cfg = make_config_for_cwd(&codex_home, nested_dir).await; - let outcome = load_skills(&cfg); - - assert!( - outcome.errors.is_empty(), - "unexpected errors: {:?}", - outcome.errors - ); - let expected_path = - canonicalize_path(&nested_skill_path).unwrap_or_else(|_| nested_skill_path.clone()); - assert_eq!( - vec![SkillMetadata { - name: "dupe-skill".to_string(), - description: "from nested".to_string(), - short_description: None, - interface: None, - path: expected_path, - scope: SkillScope::Repo, - }], - outcome.skills - ); - } } diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index bff928a56..8aa698688 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -1,12 +1,15 @@ use std::collections::HashMap; +use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; use std::sync::RwLock; use codex_utils_absolute_path::AbsolutePathBuf; use toml::Value as TomlValue; +use tracing::warn; use crate::config::Config; +use crate::config::types::SkillsConfig; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::skills::SkillLoadOutcome; @@ -44,7 +47,8 @@ impl SkillsManager { } let roots = skill_roots_from_layer_stack(&config.config_layer_stack); - let outcome = load_skills_from_roots(roots); + let mut outcome = load_skills_from_roots(roots); + outcome.disabled_paths = disabled_paths_from_stack(&config.config_layer_stack); match self.cache_by_cwd.write() { Ok(mut cache) => { cache.insert(cwd.to_path_buf(), outcome.clone()); @@ -100,7 +104,8 @@ impl SkillsManager { }; let roots = skill_roots_from_layer_stack(&config_layer_stack); - let outcome = load_skills_from_roots(roots); + let mut outcome = load_skills_from_roots(roots); + outcome.disabled_paths = disabled_paths_from_stack(&config_layer_stack); match self.cache_by_cwd.write() { Ok(mut cache) => { cache.insert(cwd.to_path_buf(), outcome.clone()); @@ -111,6 +116,51 @@ impl SkillsManager { } outcome } + + pub fn clear_cache(&self) { + match self.cache_by_cwd.write() { + Ok(mut cache) => cache.clear(), + Err(err) => err.into_inner().clear(), + } + } +} + +fn disabled_paths_from_stack( + config_layer_stack: &crate::config_loader::ConfigLayerStack, +) -> HashSet { + let mut disabled = HashSet::new(); + let mut configs = HashMap::new(); + // Skills config is user-layer only for now; higher-precedence layers are ignored. + let Some(user_layer) = config_layer_stack.get_user_layer() else { + return disabled; + }; + let Some(skills_value) = user_layer.config.get("skills") else { + return disabled; + }; + let skills: SkillsConfig = match skills_value.clone().try_into() { + Ok(skills) => skills, + Err(err) => { + warn!("invalid skills config: {err}"); + return disabled; + } + }; + + for entry in skills.config { + let path = normalize_override_path(entry.path.as_path()); + configs.insert(path, entry.enabled); + } + + for (path, enabled) in configs { + if !enabled { + disabled.insert(path); + } + } + + disabled +} + +fn normalize_override_path(path: &Path) -> PathBuf { + dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) } #[cfg(test)] diff --git a/codex-rs/core/src/skills/model.rs b/codex-rs/core/src/skills/model.rs index fba00d271..fe3357f9d 100644 --- a/codex-rs/core/src/skills/model.rs +++ b/codex-rs/core/src/skills/model.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::path::PathBuf; use codex_protocol::protocol::SkillScope; @@ -32,4 +33,25 @@ pub struct SkillError { pub struct SkillLoadOutcome { pub skills: Vec, pub errors: Vec, + pub disabled_paths: HashSet, +} + +impl SkillLoadOutcome { + pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool { + !self.disabled_paths.contains(&skill.path) + } + + pub fn enabled_skills(&self) -> Vec { + self.skills + .iter() + .filter(|skill| self.is_skill_enabled(skill)) + .cloned() + .collect() + } + + pub fn skills_with_enabled(&self) -> impl Iterator { + self.skills + .iter() + .map(|skill| (skill, self.is_skill_enabled(skill))) + } } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index e1054d0a3..3593e1f16 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -622,7 +622,7 @@ impl ChatWidget { self.submit_op(Op::ListCustomPrompts); self.submit_op(Op::ListSkills { cwds: Vec::new(), - force_reload: false, + force_reload: true, }); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message); diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index c0944fa08..5b3343d5b 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -544,7 +544,7 @@ impl ChatWidget { self.submit_op(Op::ListCustomPrompts); self.submit_op(Op::ListSkills { cwds: Vec::new(), - force_reload: false, + force_reload: true, }); if let Some(user_message) = self.initial_user_message.take() { self.submit_user_message(user_message);