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

Remove skills list extra roots (#21485)

Browse Source 
## Summary
- Remove `perCwdExtraUserRoots` / `SkillsListExtraRootsForCwd` from the
`skills/list` app-server API.
- Drop Rust app-server and `codex-core-skills` extra-root plumbing so
skill scans are keyed by the normal cwd/user/plugin roots only.
- Regenerate app-server schemas and update docs/tests that only existed
for the removed extra-roots behavior.

## Validation
- `just write-app-server-schema`
- `just fmt`
- `cargo test -p codex-app-server-protocol`
- `cargo test -p codex-core-skills`
- `just fix -p codex-app-server-protocol`
- `just fix -p codex-core-skills`
- `just fix -p codex-app-server`
- `just fix -p codex-tui`

## Notes
- `cargo test -p codex-app-server --test all skills_list` ran the edited
skills-list cases, but the full filtered run ended on existing
`skills_changed_notification_is_emitted_after_skill_change` timeout
after a websocket `401`.
- `cargo test -p codex-tui --lib` compiled the changed TUI callers, then
failed two unrelated status permission tests because local
`/etc/codex/requirements.toml` forbids `DangerFullAccess`.
- Source-truth check found the OpenAI monorepo still has
generated/app-server-kit mirror references to the removed field; those
should be cleaned up when generated app-server types are synced or in a
companion OpenAI cleanup.
This commit is contained in:
xli-oai
2026-05-07 20:56:42 -07:00
committed by GitHub
Unverified
parent 99016ec732
commit 314229fd72
17 changed files with 46 additions and 518 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/schema/json/ClientRequest.json
Generated
-29
View File
@@ -3212,24 +3212,6 @@
],
"type": "object"
},
"SkillsListExtraRootsForCwd": {
"properties": {
"cwd": {
"type": "string"
},
"extraUserRoots": {
"items": {
"type": "string"
},
"type": "array"
}
},
"required": [
"cwd",
"extraUserRoots"
],
"type": "object"
},
"SkillsListParams": {
"properties": {
"cwds": {
@@ -3242,17 +3224,6 @@
"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"
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json
Generated
-29
View File
@@ -14766,24 +14766,6 @@
],
"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": {
@@ -14797,17 +14779,6 @@
"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",
codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json
Generated
-29
View File
@@ -12634,24 +12634,6 @@
],
"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": {
@@ -12665,17 +12647,6 @@
"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",
codex-rs/app-server-protocol/schema/json/v2/SkillsListParams.json
Generated
-31
View File
@@ -1,25 +1,5 @@
{
"$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.",
@@ -31,17 +11,6 @@
"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",
codex-rs/app-server-protocol/schema/typescript/v2/SkillsListExtraRootsForCwd.ts
Generated
-5
View File
@@ -1,5 +0,0 @@
// 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<string>, };
codex-rs/app-server-protocol/schema/typescript/v2/SkillsListParams.ts
Generated
+1 -6
View File
@@ -1,7 +1,6 @@
// 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 = {
/**
@@ -11,8 +10,4 @@ cwds?: Array<string>,
/**
* When true, bypass the skills cache and re-scan skills from disk.
*/
forceReload?: boolean,
/**
* Optional per-cwd extra roots to scan as user-scoped skills.
*/
perCwdExtraUserRoots?: Array<SkillsListExtraRootsForCwd> | null, };
forceReload?: boolean, };
codex-rs/app-server-protocol/schema/typescript/v2/index.ts
Generated
-1
View File
@@ -336,7 +336,6 @@ export type { SkillsChangedNotification } from "./SkillsChangedNotification";
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 { SortDirection } from "./SortDirection";
codex-rs/app-server-protocol/src/protocol/common.rs
-1
View File
@@ -1656,7 +1656,6 @@ mod tests {
params: v2::SkillsListParams {
cwds: Vec::new(),
force_reload: false,
per_cwd_extra_user_roots: None,
},
};
assert_eq!(
codex-rs/app-server-protocol/src/protocol/v2/plugin.rs
+1 -14
View File
@@ -15,7 +15,7 @@ use serde::Serialize;
use std::path::PathBuf;
use ts_rs::TS;
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, JsonSchema, TS)]
#[serde(rename_all = "camelCase")]
#[ts(export_to = "v2/")]
pub struct SkillsListParams {
@@ -26,19 +26,6 @@ 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<Vec<SkillsListExtraRootsForCwd>>,
}
#[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<PathBuf>,
}
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
codex-rs/app-server-protocol/src/protocol/v2/tests.rs
+1 -14
View File
@@ -2508,33 +2508,20 @@ fn skills_list_params_serialization_uses_force_reload() {
serde_json::to_value(SkillsListParams {
cwds: Vec::new(),
force_reload: false,
per_cwd_extra_user_roots: None,
})
.unwrap(),
json!({
"perCwdExtraUserRoots": null,
}),
json!({}),
);
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"],
}
],
}),
);
}
codex-rs/app-server/README.md
+1 -9
View File
@@ -1485,21 +1485,13 @@ $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.
The server also emits `skills/changed` notifications when watched local skill files change. Treat this as an invalidation signal and re-run `skills/list` with your current params when needed.
```json
{ "method": "skills/list", "id": 25, "params": {
"cwds": ["/Users/me/project", "/Users/me/other-project"],
"forceReload": true,
"perCwdExtraUserRoots": [
{
"cwd": "/Users/me/project",
"extraUserRoots": ["/Users/me/shared-skills"]
}
]
"forceReload": true
} }
{ "id": 25, "result": {
"data": [{
codex-rs/app-server/src/request_processors/catalog_processor.rs
+2 -43
View File
@@ -382,44 +382,12 @@ impl CatalogRequestProcessor {
&self,
params: SkillsListParams,
) -> Result<SkillsListResponse, JSONRPCErrorError> {
let SkillsListParams {
cwds,
force_reload,
per_cwd_extra_user_roots,
} = params;
let SkillsListParams { cwds, force_reload } = params;
let cwds = if cwds.is_empty() {
vec![self.config.cwd.to_path_buf()]
} else {
cwds
};
let cwd_set: HashSet<PathBuf> = cwds.iter().cloned().collect();
let mut extra_roots_by_cwd: HashMap<PathBuf, Vec<AbsolutePathBuf>> = 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 {
let root =
AbsolutePathBuf::from_absolute_path_checked(root.as_path()).map_err(|_| {
invalid_request(format!(
"skills/list perCwdExtraUserRoots extraUserRoots paths must be absolute: {}",
root.display()
))
})?;
valid_extra_roots.push(root);
}
extra_roots_by_cwd
.entry(entry.cwd)
.or_default()
.extend(valid_extra_roots);
}
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
let auth = self.auth_manager.auth().await;
@@ -436,7 +404,6 @@ impl CatalogRequestProcessor {
let mut data = futures::stream::iter(cwds.into_iter().enumerate())
.map(|(index, cwd)| {
let config = &config;
let extra_roots_by_cwd = &extra_roots_by_cwd;
let fs = fs.clone();
let plugins_manager = &plugins_manager;
let skills_manager = &skills_manager;
@@ -458,9 +425,6 @@ impl CatalogRequestProcessor {
);
}
};
let extra_roots = extra_roots_by_cwd
.get(&cwd)
.map_or(&[][..], std::vec::Vec::as_slice);
let effective_skill_roots = if workspace_codex_plugins_enabled {
let plugins_input = config.plugins_config_input();
plugins_manager
@@ -479,12 +443,7 @@ impl CatalogRequestProcessor {
config.bundled_skills_enabled(),
);
let outcome = skills_manager
.skills_for_cwd_with_extra_user_roots(
&skills_input,
force_reload,
extra_roots,
fs,
)
.skills_for_cwd(&skills_input, force_reload, fs)
.await;
let errors = errors_to_info(&outcome.errors);
let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths);
codex-rs/app-server/tests/suite/v2/skills_list.rs
+15 -136
View File
@@ -11,7 +11,6 @@ use codex_app_server_protocol::PluginListParams;
use codex_app_server_protocol::PluginListResponse;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::SkillsChangedNotification;
use codex_app_server_protocol::SkillsListExtraRootsForCwd;
use codex_app_server_protocol::SkillsListParams;
use codex_app_server_protocol::SkillsListResponse;
use codex_app_server_protocol::ThreadStartParams;
@@ -133,44 +132,6 @@ fn write_cached_remote_plugin_with_skill(
Ok(skill_path)
}
#[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.as_path(), cwd.path());
assert!(
data[0]
.skills
.iter()
.any(|skill| skill.name == "extra-skill")
);
Ok(())
}
#[tokio::test]
async fn skills_list_loads_remote_installed_plugin_skills_from_cache() -> Result<()> {
let codex_home = TempDir::new()?;
@@ -266,7 +227,6 @@ async fn skills_list_loads_remote_installed_plugin_skills_from_cache() -> Result
.send_skills_list_request(SkillsListParams {
cwds: vec![cwd.path().to_path_buf()],
force_reload: true,
per_cwd_extra_user_roots: None,
})
.await?;
let stale_skills_list_response: JSONRPCResponse = timeout(
@@ -317,7 +277,6 @@ async fn skills_list_loads_remote_installed_plugin_skills_from_cache() -> Result
.send_skills_list_request(SkillsListParams {
cwds: vec![cwd.path().to_path_buf()],
force_reload: false,
per_cwd_extra_user_roots: None,
})
.await?;
let skills_list_response: JSONRPCResponse = timeout(
@@ -392,7 +351,6 @@ async fn skills_list_excludes_plugin_skills_when_workspace_codex_plugins_disable
.send_skills_list_request(SkillsListParams {
cwds: vec![repo_root.path().to_path_buf()],
force_reload: true,
per_cwd_extra_user_roots: None,
})
.await?;
@@ -424,9 +382,13 @@ async fn skills_list_excludes_plugin_skills_when_workspace_codex_plugins_disable
async fn skills_list_skips_cwd_roots_when_environment_disabled() -> Result<()> {
let codex_home = TempDir::new()?;
let cwd = TempDir::new()?;
let extra_root = TempDir::new()?;
write_skill(&codex_home, "home-skill")?;
write_skill(&extra_root, "extra-skill")?;
let repo_skill_dir = cwd.path().join(".codex/skills/repo-skill");
std::fs::create_dir_all(&repo_skill_dir)?;
std::fs::write(
repo_skill_dir.join("SKILL.md"),
"---\nname: repo-skill\ndescription: from repo root\n---\n\n# Body\n",
)?;
let mut mcp = McpProcess::new_with_env(
codex_home.path(),
@@ -439,10 +401,6 @@ async fn skills_list_skips_cwd_roots_when_environment_disabled() -> Result<()> {
.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?;
@@ -465,41 +423,7 @@ async fn skills_list_skips_cwd_roots_when_environment_disabled() -> Result<()> {
data[0]
.skills
.iter()
.all(|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
.all(|skill| skill.name != "repo-skill")
);
Ok(())
}
@@ -517,7 +441,6 @@ async fn skills_list_accepts_relative_cwds() -> Result<()> {
.send_skills_list_request(SkillsListParams {
cwds: vec![relative_cwd.clone()],
force_reload: true,
per_cwd_extra_user_roots: None,
})
.await?;
@@ -549,7 +472,6 @@ async fn skills_list_preserves_requested_cwd_order() -> Result<()> {
second_cwd.path().to_path_buf(),
],
force_reload: true,
per_cwd_extra_user_roots: None,
})
.await?;
@@ -571,61 +493,19 @@ async fn skills_list_preserves_requested_cwd_order() -> Result<()> {
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.as_path(), requested_cwd.path());
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.
// Seed the cwd cache before the cwd-local skill exists.
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(
@@ -642,14 +522,17 @@ async fn skills_list_uses_cached_result_until_force_reload() -> Result<()> {
.all(|skill| skill.name != "late-extra-skill")
);
let skill_dir = cwd.path().join(".codex/skills/late-extra-skill");
std::fs::create_dir_all(&skill_dir)?;
std::fs::write(
skill_dir.join("SKILL.md"),
"---\nname: late-extra-skill\ndescription: late skill\n---\n\n# Body\n",
)?;
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(
@@ -670,10 +553,6 @@ async fn skills_list_uses_cached_result_until_force_reload() -> Result<()> {
.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(
codex-rs/core-skills/src/manager.rs
-33
View File
@@ -131,17 +131,6 @@ impl SkillsManager {
input: &SkillsLoadInput,
force_reload: bool,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
self.skills_for_cwd_with_extra_user_roots(input, force_reload, &[], fs)
.await
}
pub async fn skills_for_cwd_with_extra_user_roots(
&self,
input: &SkillsLoadInput,
force_reload: bool,
extra_user_roots: &[AbsolutePathBuf],
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
let use_cwd_cache = fs.is_some();
if use_cwd_cache
@@ -161,18 +150,6 @@ impl SkillsManager {
if !bundled_skills_enabled_from_stack(&input.config_layer_stack) {
roots.retain(|root| root.scope != SkillScope::System);
}
if let Some(fs) = fs {
roots.extend(
normalize_extra_user_roots(extra_user_roots)
.into_iter()
.map(|path| SkillRoot {
path,
scope: SkillScope::User,
file_system: Arc::clone(&fs),
plugin_id: None,
}),
);
}
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let outcome = self.build_skill_outcome(roots, &skill_config_rules).await;
if use_cwd_cache {
@@ -300,16 +277,6 @@ fn finalize_skill_outcome(
outcome
}
fn normalize_extra_user_roots(extra_user_roots: &[AbsolutePathBuf]) -> Vec<AbsolutePathBuf> {
let mut normalized: Vec<AbsolutePathBuf> = extra_user_roots
.iter()
.map(|root| root.canonicalize().unwrap_or_else(|_| root.clone()))
.collect();
normalized.sort_unstable();
normalized.dedup();
normalized
}
#[cfg(test)]
#[path = "manager_tests.rs"]
mod tests;
codex-rs/core-skills/src/manager_tests.rs
+25 -136
View File
@@ -11,7 +11,6 @@ use codex_exec_server::LOCAL_FS;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_absolute_path::test_support::PathExt;
use codex_utils_absolute_path::test_support::test_path_buf;
use codex_utils_plugins::PluginSkillRoot;
use pretty_assertions::assert_eq;
use std::collections::HashSet;
@@ -263,75 +262,13 @@ async fn skills_for_config_disables_plugin_skills_by_name() {
}
#[tokio::test]
async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() {
async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let extra_root = tempfile::tempdir().expect("tempdir");
let config_layer_stack = config_stack(&codex_home, "");
let skills_manager = SkillsManager::new(
codex_home.path().abs(),
/*bundled_skills_enabled*/ true,
);
let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await;
write_user_skill(&extra_root, "x", "extra-skill", "from extra root");
let extra_root_path = extra_root.path().abs();
let base_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
config_layer_stack.clone(),
bundled_skills_enabled_from_stack(&config_layer_stack),
);
let outcome_with_extra = skills_manager
.skills_for_cwd_with_extra_user_roots(
&base_input,
/*force_reload*/ true,
std::slice::from_ref(&extra_root_path),
Some(Arc::clone(&LOCAL_FS)),
)
.await;
assert!(
outcome_with_extra
.skills
.iter()
.any(|skill| skill.name == "extra-skill")
);
assert!(
outcome_with_extra
.skills
.iter()
.any(|skill| skill.scope == SkillScope::System)
);
// The cwd-only API returns the current cached entry for this cwd, even when that entry
// was produced with extra roots.
let base_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
config_layer_stack.clone(),
bundled_skills_enabled_from_stack(&config_layer_stack),
);
let outcome_without_extra = skills_manager
.skills_for_cwd(
&base_input,
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
)
.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_loads_repo_user_and_extra_roots_with_local_fs() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let extra_root = tempfile::tempdir().expect("tempdir");
let repo_dot_codex = cwd.path().join(".codex");
fs::create_dir_all(&repo_dot_codex).expect("create repo config dir");
write_user_skill(&codex_home, "user", "user-skill", "from local user root");
write_user_skill(&extra_root, "extra", "extra-skill", "from extra root");
let repo_skill_dir = repo_dot_codex.join("skills/repo");
fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir");
fs::write(
@@ -366,10 +303,9 @@ async fn skills_for_cwd_loads_repo_user_and_extra_roots_with_local_fs() {
);
let outcome = skills_manager
.skills_for_cwd_with_extra_user_roots(
.skills_for_cwd(
&skills_input,
/*force_reload*/ true,
&[extra_root.path().abs()],
Some(Arc::clone(&LOCAL_FS)),
)
.await;
@@ -386,19 +322,16 @@ async fn skills_for_cwd_loads_repo_user_and_extra_roots_with_local_fs() {
.collect::<HashSet<_>>();
assert!(loaded_names.contains("user-skill"));
assert!(loaded_names.contains("repo-skill"));
assert!(loaded_names.contains("extra-skill"));
}
#[tokio::test]
async fn skills_for_cwd_without_fs_skips_repo_and_extra_roots() {
async fn skills_for_cwd_without_fs_skips_repo_roots() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let extra_root = tempfile::tempdir().expect("tempdir");
let repo_dot_codex = cwd.path().join(".codex");
fs::create_dir_all(&repo_dot_codex).expect("create repo config dir");
write_user_skill(&codex_home, "user", "user-skill", "from local user root");
write_user_skill(&extra_root, "extra", "extra-skill", "from extra root");
let repo_skill_dir = repo_dot_codex.join("skills/repo");
fs::create_dir_all(&repo_skill_dir).expect("create repo skill dir");
fs::write(
@@ -433,12 +366,7 @@ async fn skills_for_cwd_without_fs_skips_repo_and_extra_roots() {
);
let outcome = skills_manager
.skills_for_cwd_with_extra_user_roots(
&skills_input,
/*force_reload*/ true,
&[extra_root.path().abs()],
/*fs*/ None,
)
.skills_for_cwd(&skills_input, /*force_reload*/ true, /*fs*/ None)
.await;
assert!(
@@ -453,7 +381,6 @@ async fn skills_for_cwd_without_fs_skips_repo_and_extra_roots() {
.collect::<HashSet<_>>();
assert!(loaded_names.contains("user-skill"));
assert!(!loaded_names.contains("repo-skill"));
assert!(!loaded_names.contains("extra-skill"));
}
#[tokio::test]
@@ -499,22 +426,15 @@ async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() {
}
#[tokio::test]
async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() {
async fn skills_for_cwd_uses_cached_result_until_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_layer_stack = config_stack(&codex_home, "");
let skills_manager = SkillsManager::new(
codex_home.path().abs(),
/*bundled_skills_enabled*/ true,
);
let _ = skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await;
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().abs();
let base_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
@@ -522,32 +442,25 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() {
bundled_skills_enabled_from_stack(&config_layer_stack),
);
let outcome_a = skills_manager
.skills_for_cwd_with_extra_user_roots(
&base_input,
/*force_reload*/ true,
std::slice::from_ref(&extra_root_a_path),
Some(Arc::clone(&LOCAL_FS)),
)
.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().abs();
let outcome_b = skills_manager
.skills_for_cwd_with_extra_user_roots(
.skills_for_cwd(
&base_input,
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
)
.await;
assert!(
outcome_a
.skills
.iter()
.all(|skill| skill.name != "late-skill")
);
write_user_skill(&codex_home, "late", "late-skill", "added after cache");
let outcome_b = skills_manager
.skills_for_cwd(
&base_input,
/*force_reload*/ false,
std::slice::from_ref(&extra_root_b_path),
Some(Arc::clone(&LOCAL_FS)),
)
.await;
@@ -555,20 +468,13 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() {
outcome_b
.skills
.iter()
.any(|skill| skill.name == "extra-skill-a")
);
assert!(
outcome_b
.skills
.iter()
.all(|skill| skill.name != "extra-skill-b")
.all(|skill| skill.name != "late-skill")
);
let outcome_reloaded = skills_manager
.skills_for_cwd_with_extra_user_roots(
.skills_for_cwd(
&base_input,
/*force_reload*/ true,
std::slice::from_ref(&extra_root_b_path),
Some(Arc::clone(&LOCAL_FS)),
)
.await;
@@ -576,25 +482,8 @@ async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() {
outcome_reloaded
.skills
.iter()
.any(|skill| skill.name == "extra-skill-b")
.any(|skill| skill.name == "late-skill")
);
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 = test_path_buf("/tmp/a").abs();
let b = test_path_buf("/tmp/b").abs();
let first = normalize_extra_user_roots(&[a.clone(), b.clone(), a.clone()]);
let second = normalize_extra_user_roots(&[b, a]);
assert_eq!(first, second);
}
#[cfg_attr(windows, ignore)]
codex-rs/tui/src/app/background_requests.rs
-1
View File
@@ -648,7 +648,6 @@ pub(super) async fn fetch_skills_list(
params: SkillsListParams {
cwds: vec![cwd],
force_reload: true,
per_cwd_extra_user_roots: None,
},
})
.await
codex-rs/tui/src/app/thread_routing.rs
-1
View File
@@ -621,7 +621,6 @@ impl App {
.skills_list(codex_app_server_protocol::SkillsListParams {
cwds: cwds.clone(),
force_reload: *force_reload,
per_cwd_extra_user_roots: None,
})
.await,
"failed to refresh skills",