From a33ee46e3b506efcc3947088bd4433abbe93d030 Mon Sep 17 00:00:00 2001 From: xl-openai Date: Mon, 9 Feb 2026 13:30:38 -0800 Subject: [PATCH] feat: extend skills/list to support additional roots. (#10835) Add an optional perCwdExtraUserRoots --- .../schema/json/ClientRequest.json | 29 +++ .../codex_app_server_protocol.schemas.json | 29 +++ .../schema/json/v2/SkillsListParams.json | 31 +++ .../v2/SkillsListExtraRootsForCwd.ts | 5 + .../schema/typescript/v2/SkillsListParams.ts | 7 +- .../schema/typescript/v2/index.ts | 1 + .../app-server-protocol/src/protocol/v2.rs | 31 ++- codex-rs/app-server/README.md | 13 +- .../app-server/src/codex_message_processor.rs | 45 +++- .../app-server/tests/common/mcp_process.rs | 10 + codex-rs/app-server/tests/suite/v2/mod.rs | 1 + .../app-server/tests/suite/v2/skills_list.rs | 216 ++++++++++++++++++ codex-rs/core/src/skills/manager.rs | 197 +++++++++++++++- 13 files changed, 598 insertions(+), 17 deletions(-) create mode 100644 codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts create mode 100644 codex-rs/app-server/tests/suite/v2/skills_list.rs diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index 1f19f8f55..c981dfda6 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -2210,6 +2210,24 @@ ], "type": "object" }, + "SkillsListExtraRootsForCwd": { + "properties": { + "cwd": { + "type": "string" + }, + "extraUserRoots": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "cwd", + "extraUserRoots" + ], + "type": "object" + }, "SkillsListParams": { "properties": { "cwds": { @@ -2222,6 +2240,17 @@ "forceReload": { "description": "When true, bypass the skills cache and re-scan skills from disk.", "type": "boolean" + }, + "perCwdExtraUserRoots": { + "default": null, + "description": "Optional per-cwd extra roots to scan as user-scoped skills.", + "items": { + "$ref": "#/definitions/SkillsListExtraRootsForCwd" + }, + "type": [ + "array", + "null" + ] } }, "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 6f36a9a89..6f57754dd 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 @@ -14079,6 +14079,24 @@ ], "type": "object" }, + "SkillsListExtraRootsForCwd": { + "properties": { + "cwd": { + "type": "string" + }, + "extraUserRoots": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "cwd", + "extraUserRoots" + ], + "type": "object" + }, "SkillsListParams": { "$schema": "http://json-schema.org/draft-07/schema#", "properties": { @@ -14092,6 +14110,17 @@ "forceReload": { "description": "When true, bypass the skills cache and re-scan skills from disk.", "type": "boolean" + }, + "perCwdExtraUserRoots": { + "default": null, + "description": "Optional per-cwd extra roots to scan as user-scoped skills.", + "items": { + "$ref": "#/definitions/v2/SkillsListExtraRootsForCwd" + }, + "type": [ + "array", + "null" + ] } }, "title": "SkillsListParams", diff --git a/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json b/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json index a9a8a9ef8..77d12e917 100644 --- a/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json +++ b/codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json @@ -1,5 +1,25 @@ { "$schema": "http://json-schema.org/draft-07/schema#", + "definitions": { + "SkillsListExtraRootsForCwd": { + "properties": { + "cwd": { + "type": "string" + }, + "extraUserRoots": { + "items": { + "type": "string" + }, + "type": "array" + } + }, + "required": [ + "cwd", + "extraUserRoots" + ], + "type": "object" + } + }, "properties": { "cwds": { "description": "When empty, defaults to the current session working directory.", @@ -11,6 +31,17 @@ "forceReload": { "description": "When true, bypass the skills cache and re-scan skills from disk.", "type": "boolean" + }, + "perCwdExtraUserRoots": { + "default": null, + "description": "Optional per-cwd extra roots to scan as user-scoped skills.", + "items": { + "$ref": "#/definitions/SkillsListExtraRootsForCwd" + }, + "type": [ + "array", + "null" + ] } }, "title": "SkillsListParams", diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts new file mode 100644 index 000000000..c18cd4ba1 --- /dev/null +++ b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts @@ -0,0 +1,5 @@ +// 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. + +export type SkillsListExtraRootsForCwd = { cwd: string, extraUserRoots: Array, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts index d44e6551f..f58444381 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts @@ -1,6 +1,7 @@ // 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 { SkillsListExtraRootsForCwd } from "./SkillsListExtraRootsForCwd"; export type SkillsListParams = { /** @@ -10,4 +11,8 @@ cwds?: Array, /** * When true, bypass the skills cache and re-scan skills from disk. */ -forceReload?: boolean, }; +forceReload?: boolean, +/** + * Optional per-cwd extra roots to scan as user-scoped skills. + */ +perCwdExtraUserRoots?: Array | null, }; diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts index 05e8322e4..f54d94e7f 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/index.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/index.ts @@ -124,6 +124,7 @@ export type { SkillToolDependency } from "./SkillToolDependency"; export type { SkillsConfigWriteParams } from "./SkillsConfigWriteParams"; export type { SkillsConfigWriteResponse } from "./SkillsConfigWriteResponse"; export type { SkillsListEntry } from "./SkillsListEntry"; +export type { SkillsListExtraRootsForCwd } from "./SkillsListExtraRootsForCwd"; export type { SkillsListParams } from "./SkillsListParams"; export type { SkillsListResponse } from "./SkillsListResponse"; export type { SkillsRemoteReadParams } from "./SkillsRemoteReadParams"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2.rs b/codex-rs/app-server-protocol/src/protocol/v2.rs index cdd0b4bcf..a75bdaab3 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2.rs @@ -1704,6 +1704,19 @@ pub struct SkillsListParams { /// When true, bypass the skills cache and re-scan skills from disk. #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub force_reload: bool, + + /// Optional per-cwd extra roots to scan as user-scoped skills. + #[serde(default)] + #[ts(optional = nullable)] + pub per_cwd_extra_user_roots: Option>, +} + +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +#[ts(export_to = "v2/")] +pub struct SkillsListExtraRootsForCwd { + pub cwd: PathBuf, + pub extra_user_roots: Vec, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] @@ -3352,20 +3365,36 @@ mod tests { serde_json::to_value(SkillsListParams { cwds: Vec::new(), force_reload: false, + per_cwd_extra_user_roots: None, }) .unwrap(), - json!({}), + json!({ + "perCwdExtraUserRoots": null, + }), ); assert_eq!( serde_json::to_value(SkillsListParams { cwds: vec![PathBuf::from("/repo")], force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: PathBuf::from("/repo"), + extra_user_roots: vec![ + PathBuf::from("/shared/skills"), + PathBuf::from("/tmp/x") + ], + }]), }) .unwrap(), json!({ "cwds": ["/repo"], "forceReload": true, + "perCwdExtraUserRoots": [ + { + "cwd": "/repo", + "extraUserRoots": ["/shared/skills", "/tmp/x"], + } + ], }), ); } diff --git a/codex-rs/app-server/README.md b/codex-rs/app-server/README.md index 8946880f9..04a254a60 100644 --- a/codex-rs/app-server/README.md +++ b/codex-rs/app-server/README.md @@ -663,11 +663,20 @@ $skill-creator Add a new skill for triaging flaky CI and include step-by-step us ``` Use `skills/list` to fetch the available skills (optionally scoped by `cwds`, with `forceReload`). +You can also add `perCwdExtraUserRoots` to scan additional absolute paths as `user` scope for specific `cwd` entries. +Entries whose `cwd` is not present in `cwds` are ignored. +`skills/list` might reuse a cached skills result per `cwd`; setting `forceReload` to `true` refreshes the result from disk. ```json { "method": "skills/list", "id": 25, "params": { - "cwds": ["/Users/me/project"], - "forceReload": false + "cwds": ["/Users/me/project", "/Users/me/other-project"], + "forceReload": true, + "perCwdExtraUserRoots": [ + { + "cwd": "/Users/me/project", + "extraUserRoots": ["/Users/me/shared-skills"] + } + ] } } { "id": 25, "result": { "data": [{ diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index e1bfe3a33..0c24ed30c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -4574,17 +4574,58 @@ impl CodexMessageProcessor { } async fn skills_list(&self, request_id: ConnectionRequestId, params: SkillsListParams) { - let SkillsListParams { cwds, force_reload } = params; + let SkillsListParams { + cwds, + force_reload, + per_cwd_extra_user_roots, + } = params; let cwds = if cwds.is_empty() { vec![self.config.cwd.clone()] } else { cwds }; + let cwd_set: HashSet = cwds.iter().cloned().collect(); + + let mut extra_roots_by_cwd: HashMap> = HashMap::new(); + for entry in per_cwd_extra_user_roots.unwrap_or_default() { + if !cwd_set.contains(&entry.cwd) { + warn!( + cwd = %entry.cwd.display(), + "ignoring per-cwd extra roots for cwd not present in skills/list cwds" + ); + continue; + } + + let mut valid_extra_roots = Vec::new(); + for root in entry.extra_user_roots { + if !root.is_absolute() { + self.send_invalid_request_error( + request_id, + format!( + "skills/list perCwdExtraUserRoots extraUserRoots paths must be absolute: {}", + root.display() + ), + ) + .await; + return; + } + valid_extra_roots.push(root); + } + extra_roots_by_cwd + .entry(entry.cwd) + .or_default() + .extend(valid_extra_roots); + } let skills_manager = self.thread_manager.skills_manager(); let mut data = Vec::new(); for cwd in cwds { - let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; + let extra_roots = extra_roots_by_cwd + .get(&cwd) + .map_or(&[][..], std::vec::Vec::as_slice); + let outcome = skills_manager + .skills_for_cwd_with_extra_user_roots(&cwd, force_reload, extra_roots) + .await; let errors = errors_to_info(&outcome.errors); let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths); data.push(codex_app_server_protocol::SkillsListEntry { diff --git a/codex-rs/app-server/tests/common/mcp_process.rs b/codex-rs/app-server/tests/common/mcp_process.rs index 57c29fcf9..5e2d7fa42 100644 --- a/codex-rs/app-server/tests/common/mcp_process.rs +++ b/codex-rs/app-server/tests/common/mcp_process.rs @@ -50,6 +50,7 @@ use codex_app_server_protocol::SendUserMessageParams; use codex_app_server_protocol::SendUserTurnParams; use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::SetDefaultModelParams; +use codex_app_server_protocol::SkillsListParams; use codex_app_server_protocol::ThreadArchiveParams; use codex_app_server_protocol::ThreadCompactStartParams; use codex_app_server_protocol::ThreadForkParams; @@ -490,6 +491,15 @@ impl McpProcess { self.send_request("app/list", params).await } + /// Send a `skills/list` JSON-RPC request. + pub async fn send_skills_list_request( + &mut self, + params: SkillsListParams, + ) -> anyhow::Result { + let params = Some(serde_json::to_value(params)?); + self.send_request("skills/list", params).await + } + /// Send a `collaborationMode/list` JSON-RPC request. pub async fn send_list_collaboration_modes_request( &mut self, diff --git a/codex-rs/app-server/tests/suite/v2/mod.rs b/codex-rs/app-server/tests/suite/v2/mod.rs index c25a6d056..48622acdd 100644 --- a/codex-rs/app-server/tests/suite/v2/mod.rs +++ b/codex-rs/app-server/tests/suite/v2/mod.rs @@ -15,6 +15,7 @@ mod plan_item; mod rate_limits; mod request_user_input; mod review; +mod skills_list; mod thread_archive; mod thread_fork; mod thread_list; diff --git a/codex-rs/app-server/tests/suite/v2/skills_list.rs b/codex-rs/app-server/tests/suite/v2/skills_list.rs new file mode 100644 index 000000000..e099bdb64 --- /dev/null +++ b/codex-rs/app-server/tests/suite/v2/skills_list.rs @@ -0,0 +1,216 @@ +use std::time::Duration; + +use anyhow::Result; +use app_test_support::McpProcess; +use app_test_support::to_response; +use codex_app_server_protocol::JSONRPCResponse; +use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::SkillsListExtraRootsForCwd; +use codex_app_server_protocol::SkillsListParams; +use codex_app_server_protocol::SkillsListResponse; +use pretty_assertions::assert_eq; +use tempfile::TempDir; +use tokio::time::timeout; + +const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10); + +fn write_skill(root: &TempDir, name: &str) -> Result<()> { + let skill_dir = root.path().join("skills").join(name); + std::fs::create_dir_all(&skill_dir)?; + let content = format!("---\nname: {name}\ndescription: {name} description\n---\n\n# Body\n"); + std::fs::write(skill_dir.join("SKILL.md"), content)?; + Ok(()) +} + +#[tokio::test] +async fn skills_list_includes_skills_from_per_cwd_extra_user_roots() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&extra_root, "extra-skill")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let SkillsListResponse { data } = to_response(response)?; + assert_eq!(data.len(), 1); + assert_eq!(data[0].cwd, cwd.path().to_path_buf()); + assert!( + data[0] + .skills + .iter() + .any(|skill| skill.name == "extra-skill") + ); + Ok(()) +} + +#[tokio::test] +async fn skills_list_rejects_relative_extra_user_roots() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![std::path::PathBuf::from("relative/skills")], + }]), + }) + .await?; + + let err = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_error_message(RequestId::Integer(request_id)), + ) + .await??; + assert!( + err.error + .message + .contains("perCwdExtraUserRoots extraUserRoots paths must be absolute"), + "unexpected error: {}", + err.error.message + ); + Ok(()) +} + +#[tokio::test] +async fn skills_list_ignores_per_cwd_extra_roots_for_unknown_cwd() -> Result<()> { + let codex_home = TempDir::new()?; + let requested_cwd = TempDir::new()?; + let unknown_cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&extra_root, "ignored-extra-skill")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![requested_cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: unknown_cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let SkillsListResponse { data } = to_response(response)?; + assert_eq!(data.len(), 1); + assert_eq!(data[0].cwd, requested_cwd.path().to_path_buf()); + assert!( + data[0] + .skills + .iter() + .all(|skill| skill.name != "ignored-extra-skill") + ); + Ok(()) +} + +#[tokio::test] +async fn skills_list_uses_cached_result_until_force_reload() -> Result<()> { + let codex_home = TempDir::new()?; + let cwd = TempDir::new()?; + let extra_root = TempDir::new()?; + write_skill(&extra_root, "late-extra-skill")?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + // Seed the cwd cache first without extra roots. + let first_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: false, + per_cwd_extra_user_roots: None, + }) + .await?; + let first_response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(first_request_id)), + ) + .await??; + let SkillsListResponse { data: first_data } = to_response(first_response)?; + assert_eq!(first_data.len(), 1); + assert!( + first_data[0] + .skills + .iter() + .all(|skill| skill.name != "late-extra-skill") + ); + + let second_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: false, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + let second_response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_request_id)), + ) + .await??; + let SkillsListResponse { data: second_data } = to_response(second_response)?; + assert_eq!(second_data.len(), 1); + assert!( + second_data[0] + .skills + .iter() + .all(|skill| skill.name != "late-extra-skill") + ); + + let third_request_id = mcp + .send_skills_list_request(SkillsListParams { + cwds: vec![cwd.path().to_path_buf()], + force_reload: true, + per_cwd_extra_user_roots: Some(vec![SkillsListExtraRootsForCwd { + cwd: cwd.path().to_path_buf(), + extra_user_roots: vec![extra_root.path().to_path_buf()], + }]), + }) + .await?; + let third_response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(third_request_id)), + ) + .await??; + let SkillsListResponse { data: third_data } = to_response(third_response)?; + assert_eq!(third_data.len(), 1); + assert!( + third_data[0] + .skills + .iter() + .any(|skill| skill.name == "late-extra-skill") + ); + Ok(()) +} diff --git a/codex-rs/core/src/skills/manager.rs b/codex-rs/core/src/skills/manager.rs index 61dcd98f5..cc6193f76 100644 --- a/codex-rs/core/src/skills/manager.rs +++ b/codex-rs/core/src/skills/manager.rs @@ -4,6 +4,7 @@ use std::path::Path; use std::path::PathBuf; use std::sync::RwLock; +use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use toml::Value as TomlValue; use tracing::info; @@ -15,6 +16,7 @@ use crate::config_loader::CloudRequirementsLoader; use crate::config_loader::LoaderOverrides; use crate::config_loader::load_config_layers_state; use crate::skills::SkillLoadOutcome; +use crate::skills::loader::SkillRoot; use crate::skills::loader::load_skills_from_roots; use crate::skills::loader::skill_roots_from_layer_stack_with_agents; use crate::skills::system::install_system_skills; @@ -40,11 +42,7 @@ impl SkillsManager { /// loading. This also seeds the per-cwd cache for subsequent lookups. pub fn skills_for_config(&self, config: &Config) -> SkillLoadOutcome { let cwd = &config.cwd; - let cached = match self.cache_by_cwd.read() { - Ok(cache) => cache.get(cwd).cloned(), - Err(err) => err.into_inner().get(cwd).cloned(), - }; - if let Some(outcome) = cached { + if let Some(outcome) = self.cached_outcome_for_cwd(cwd) { return outcome; } @@ -61,14 +59,25 @@ impl SkillsManager { } pub async fn skills_for_cwd(&self, cwd: &Path, force_reload: bool) -> SkillLoadOutcome { - let cached = match self.cache_by_cwd.read() { - Ok(cache) => cache.get(cwd).cloned(), - Err(err) => err.into_inner().get(cwd).cloned(), - }; - if !force_reload && let Some(outcome) = cached { + if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { return outcome; } + self.skills_for_cwd_with_extra_user_roots(cwd, force_reload, &[]) + .await + } + + pub async fn skills_for_cwd_with_extra_user_roots( + &self, + cwd: &Path, + force_reload: bool, + extra_user_roots: &[PathBuf], + ) -> SkillLoadOutcome { + if !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(cwd) { + return outcome; + } + let normalized_extra_user_roots = normalize_extra_user_roots(extra_user_roots); + let cwd_abs = match AbsolutePathBuf::try_from(cwd) { Ok(cwd_abs) => cwd_abs, Err(err) => { @@ -104,7 +113,16 @@ impl SkillsManager { } }; - let roots = skill_roots_from_layer_stack_with_agents(&config_layer_stack, cwd); + let mut roots = skill_roots_from_layer_stack_with_agents(&config_layer_stack, cwd); + roots.extend( + normalized_extra_user_roots + .iter() + .cloned() + .map(|path| SkillRoot { + path, + scope: SkillScope::User, + }), + ); let mut outcome = load_skills_from_roots(roots); outcome.disabled_paths = disabled_paths_from_stack(&config_layer_stack); let mut cache = match self.cache_by_cwd.write() { @@ -124,6 +142,13 @@ impl SkillsManager { cache.clear(); info!("skills cache cleared ({cleared} entries)"); } + + fn cached_outcome_for_cwd(&self, cwd: &Path) -> Option { + match self.cache_by_cwd.read() { + Ok(cache) => cache.get(cwd).cloned(), + Err(err) => err.into_inner().get(cwd).cloned(), + } + } } fn disabled_paths_from_stack( @@ -164,6 +189,16 @@ 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() + .map(|path| dunce::canonicalize(path).unwrap_or_else(|_| path.clone())) + .collect(); + normalized.sort_unstable(); + normalized.dedup(); + normalized +} + #[cfg(test)] mod tests { use super::*; @@ -171,6 +206,7 @@ mod tests { use crate::config::ConfigOverrides; use pretty_assertions::assert_eq; use std::fs; + use std::path::PathBuf; use tempfile::TempDir; fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) { @@ -211,4 +247,143 @@ mod tests { assert_eq!(outcome2.errors, outcome1.errors); assert_eq!(outcome2.skills, outcome1.skills); } + + #[tokio::test] + async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root = tempfile::tempdir().expect("tempdir"); + + 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("defaults for test should always succeed"); + + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf()); + let _ = skills_manager.skills_for_config(&config); + + write_user_skill(&extra_root, "x", "extra-skill", "from extra root"); + let extra_root_path = extra_root.path().to_path_buf(); + let outcome_with_extra = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + true, + std::slice::from_ref(&extra_root_path), + ) + .await; + assert!( + outcome_with_extra + .skills + .iter() + .any(|skill| skill.name == "extra-skill") + ); + + // The cwd-only API returns the current cached entry for this cwd, even when that entry + // was produced with extra roots. + let outcome_without_extra = skills_manager.skills_for_cwd(cwd.path(), false).await; + assert_eq!(outcome_without_extra.skills, outcome_with_extra.skills); + assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors); + } + + #[tokio::test] + async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() { + let codex_home = tempfile::tempdir().expect("tempdir"); + let cwd = tempfile::tempdir().expect("tempdir"); + let extra_root_a = tempfile::tempdir().expect("tempdir"); + let extra_root_b = tempfile::tempdir().expect("tempdir"); + + 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("defaults for test should always succeed"); + + let skills_manager = SkillsManager::new(codex_home.path().to_path_buf()); + let _ = skills_manager.skills_for_config(&config); + + write_user_skill(&extra_root_a, "x", "extra-skill-a", "from extra root a"); + write_user_skill(&extra_root_b, "x", "extra-skill-b", "from extra root b"); + + let extra_root_a_path = extra_root_a.path().to_path_buf(); + let outcome_a = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + true, + std::slice::from_ref(&extra_root_a_path), + ) + .await; + assert!( + outcome_a + .skills + .iter() + .any(|skill| skill.name == "extra-skill-a") + ); + assert!( + outcome_a + .skills + .iter() + .all(|skill| skill.name != "extra-skill-b") + ); + + let extra_root_b_path = extra_root_b.path().to_path_buf(); + let outcome_b = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + false, + std::slice::from_ref(&extra_root_b_path), + ) + .await; + assert!( + outcome_b + .skills + .iter() + .any(|skill| skill.name == "extra-skill-a") + ); + assert!( + outcome_b + .skills + .iter() + .all(|skill| skill.name != "extra-skill-b") + ); + + let outcome_reloaded = skills_manager + .skills_for_cwd_with_extra_user_roots( + cwd.path(), + true, + std::slice::from_ref(&extra_root_b_path), + ) + .await; + assert!( + outcome_reloaded + .skills + .iter() + .any(|skill| skill.name == "extra-skill-b") + ); + assert!( + outcome_reloaded + .skills + .iter() + .all(|skill| skill.name != "extra-skill-a") + ); + } + + #[test] + fn normalize_extra_user_roots_is_stable_for_equivalent_inputs() { + let a = PathBuf::from("/tmp/a"); + let b = PathBuf::from("/tmp/b"); + + let first = normalize_extra_user_roots(&[a.clone(), b.clone(), a.clone()]); + let second = normalize_extra_user_roots(&[b, a]); + + assert_eq!(first, second); + } }