mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add runtime extra skill roots API (#24977)
## Summary - Add v2 `skills/extraRoots/set` to replace app-server process-local standalone skill roots. The setting is not persisted, accepts missing roots, and `extraRoots: []` clears the runtime set. - Wire runtime roots into core skill discovery for `skills/list` and turn loads, clear skill caches on set, and register the roots with the skills watcher so later filesystem changes emit `skills/changed`. - Update app-server docs, generated JSON/TypeScript schemas, and coverage for serialization, missing roots, empty clears, and restart behavior. ## Testing - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-core-skills` - `cargo test -p codex-app-server skills_extra_roots_set_updates_process_runtime_roots` - `just fix -p codex-app-server-protocol` - `just fix -p codex-core-skills` - `just fix -p codex-app-server`
This commit is contained in:
committed by
GitHub
Unverified
parent
42c80385cd
commit
f0a839ea0c
@@ -2988,6 +2988,20 @@
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsExtraRootsSetParams": {
|
||||
"properties": {
|
||||
"extraRoots": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"type": "array"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"extraRoots"
|
||||
],
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsListParams": {
|
||||
"properties": {
|
||||
"cwds": {
|
||||
@@ -4783,6 +4797,30 @@
|
||||
"title": "Skills/listRequest",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"id": {
|
||||
"$ref": "#/definitions/RequestId"
|
||||
},
|
||||
"method": {
|
||||
"enum": [
|
||||
"skills/extraRoots/set"
|
||||
],
|
||||
"title": "Skills/extraRoots/setRequestMethod",
|
||||
"type": "string"
|
||||
},
|
||||
"params": {
|
||||
"$ref": "#/definitions/SkillsExtraRootsSetParams"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"id",
|
||||
"method",
|
||||
"params"
|
||||
],
|
||||
"title": "Skills/extraRoots/setRequest",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"id": {
|
||||
|
||||
+45
@@ -709,6 +709,30 @@
|
||||
"title": "Skills/listRequest",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"id": {
|
||||
"$ref": "#/definitions/v2/RequestId"
|
||||
},
|
||||
"method": {
|
||||
"enum": [
|
||||
"skills/extraRoots/set"
|
||||
],
|
||||
"title": "Skills/extraRoots/setRequestMethod",
|
||||
"type": "string"
|
||||
},
|
||||
"params": {
|
||||
"$ref": "#/definitions/v2/SkillsExtraRootsSetParams"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"id",
|
||||
"method",
|
||||
"params"
|
||||
],
|
||||
"title": "Skills/extraRoots/setRequest",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"id": {
|
||||
@@ -15121,6 +15145,27 @@
|
||||
"title": "SkillsConfigWriteResponse",
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsExtraRootsSetParams": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"extraRoots": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/v2/AbsolutePathBuf"
|
||||
},
|
||||
"type": "array"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"extraRoots"
|
||||
],
|
||||
"title": "SkillsExtraRootsSetParams",
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsExtraRootsSetResponse": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"title": "SkillsExtraRootsSetResponse",
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsListEntry": {
|
||||
"properties": {
|
||||
"cwd": {
|
||||
|
||||
+45
@@ -1457,6 +1457,30 @@
|
||||
"title": "Skills/listRequest",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"id": {
|
||||
"$ref": "#/definitions/RequestId"
|
||||
},
|
||||
"method": {
|
||||
"enum": [
|
||||
"skills/extraRoots/set"
|
||||
],
|
||||
"title": "Skills/extraRoots/setRequestMethod",
|
||||
"type": "string"
|
||||
},
|
||||
"params": {
|
||||
"$ref": "#/definitions/SkillsExtraRootsSetParams"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"id",
|
||||
"method",
|
||||
"params"
|
||||
],
|
||||
"title": "Skills/extraRoots/setRequest",
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"properties": {
|
||||
"id": {
|
||||
@@ -12945,6 +12969,27 @@
|
||||
"title": "SkillsConfigWriteResponse",
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsExtraRootsSetParams": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"extraRoots": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"type": "array"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"extraRoots"
|
||||
],
|
||||
"title": "SkillsExtraRootsSetParams",
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsExtraRootsSetResponse": {
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"title": "SkillsExtraRootsSetResponse",
|
||||
"type": "object"
|
||||
},
|
||||
"SkillsListEntry": {
|
||||
"properties": {
|
||||
"cwd": {
|
||||
|
||||
@@ -0,0 +1,22 @@
|
||||
{
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"definitions": {
|
||||
"AbsolutePathBuf": {
|
||||
"description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.",
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"properties": {
|
||||
"extraRoots": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"type": "array"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
"extraRoots"
|
||||
],
|
||||
"title": "SkillsExtraRootsSetParams",
|
||||
"type": "object"
|
||||
}
|
||||
@@ -0,0 +1,5 @@
|
||||
{
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"title": "SkillsExtraRootsSetResponse",
|
||||
"type": "object"
|
||||
}
|
||||
File diff suppressed because one or more lines are too long
+6
@@ -0,0 +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 { AbsolutePathBuf } from "../AbsolutePathBuf";
|
||||
|
||||
export type SkillsExtraRootsSetParams = { extraRoots: Array<AbsolutePathBuf>, };
|
||||
+5
@@ -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 SkillsExtraRootsSetResponse = Record<string, never>;
|
||||
@@ -344,6 +344,8 @@ export type { SkillToolDependency } from "./SkillToolDependency";
|
||||
export type { SkillsChangedNotification } from "./SkillsChangedNotification";
|
||||
export type { SkillsConfigWriteParams } from "./SkillsConfigWriteParams";
|
||||
export type { SkillsConfigWriteResponse } from "./SkillsConfigWriteResponse";
|
||||
export type { SkillsExtraRootsSetParams } from "./SkillsExtraRootsSetParams";
|
||||
export type { SkillsExtraRootsSetResponse } from "./SkillsExtraRootsSetResponse";
|
||||
export type { SkillsListEntry } from "./SkillsListEntry";
|
||||
export type { SkillsListParams } from "./SkillsListParams";
|
||||
export type { SkillsListResponse } from "./SkillsListResponse";
|
||||
|
||||
@@ -610,6 +610,11 @@ client_request_definitions! {
|
||||
serialization: global_shared_read("config"),
|
||||
response: v2::SkillsListResponse,
|
||||
},
|
||||
SkillsExtraRootsSet => "skills/extraRoots/set" {
|
||||
params: v2::SkillsExtraRootsSetParams,
|
||||
serialization: global("config"),
|
||||
response: v2::SkillsExtraRootsSetResponse,
|
||||
},
|
||||
HooksList => "hooks/list" {
|
||||
params: v2::HooksListParams,
|
||||
serialization: global("config"),
|
||||
@@ -1721,6 +1726,17 @@ mod tests {
|
||||
Some(ClientRequestSerializationScope::GlobalSharedRead("config"))
|
||||
);
|
||||
|
||||
let skills_extra_roots_set = ClientRequest::SkillsExtraRootsSet {
|
||||
request_id: request_id(),
|
||||
params: v2::SkillsExtraRootsSetParams {
|
||||
extra_roots: vec![absolute_path("/tmp/skills")],
|
||||
},
|
||||
};
|
||||
assert_eq!(
|
||||
skills_extra_roots_set.serialization_scope(),
|
||||
Some(ClientRequestSerializationScope::Global("config"))
|
||||
);
|
||||
|
||||
let plugin_list = ClientRequest::PluginList {
|
||||
request_id: request_id(),
|
||||
params: v2::PluginListParams {
|
||||
|
||||
@@ -35,6 +35,18 @@ pub struct SkillsListResponse {
|
||||
pub data: Vec<SkillsListEntry>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct SkillsExtraRootsSetParams {
|
||||
pub extra_roots: Vec<AbsolutePathBuf>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct SkillsExtraRootsSetResponse {}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
|
||||
@@ -2626,6 +2626,27 @@ fn skills_list_params_serialization_uses_force_reload() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skills_extra_roots_set_params_serialization_uses_extra_roots() {
|
||||
assert_eq!(
|
||||
serde_json::to_value(SkillsExtraRootsSetParams {
|
||||
extra_roots: vec![absolute_path("tmp/skills")],
|
||||
})
|
||||
.unwrap(),
|
||||
json!({
|
||||
"extraRoots": [absolute_path_string("tmp/skills")],
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skills_extra_roots_set_params_rejects_relative_roots() {
|
||||
let result = serde_json::from_value::<SkillsExtraRootsSetParams>(json!({
|
||||
"extraRoots": ["relative/path"],
|
||||
}));
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn plugin_source_serializes_local_git_and_remote_variants() {
|
||||
let local_path = if cfg!(windows) {
|
||||
|
||||
@@ -196,6 +196,7 @@ Example with notification opt-out:
|
||||
- `environment/add` — experimental; add or replace a named remote environment by `environmentId` and `execServerUrl` for later selection by `thread/start` or `turn/start`; returns `{}` and does not change the default environment.
|
||||
- `collaborationMode/list` — list available collaboration mode presets (experimental, no pagination). Built-in presets do not select a model; the Plan preset selects medium reasoning effort. This response omits built-in developer instructions; clients should either pass `settings.developer_instructions: null` when setting a mode to use Codex's built-in instructions, or provide their own instructions explicitly.
|
||||
- `skills/list` — list skills for one or more `cwd` values (optional `forceReload`).
|
||||
- `skills/extraRoots/set` — replace the app-server process runtime extra standalone skill roots. The roots are not persisted; missing directories are accepted and simply load no skills.
|
||||
- `hooks/list` — list discovered hooks for one or more `cwd` values.
|
||||
- `marketplace/add` — add a remote plugin marketplace from an HTTP(S) Git URL, SSH Git URL, or GitHub `owner/repo` shorthand, then persist it into the user marketplace config. Returns the installed root path plus whether the marketplace was already present.
|
||||
- `marketplace/remove` — remove a configured marketplace by name from the user marketplace config, and delete its installed marketplace root when one exists.
|
||||
@@ -1506,6 +1507,7 @@ $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`).
|
||||
`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.
|
||||
Use `skills/extraRoots/set` to replace additional standalone skill roots for the current app-server process. These roots use the same layout as other standalone skill roots: each root contains skill directories, and each skill directory contains `SKILL.md`. Missing roots are accepted and load no skills until they exist. This setting is lost when app-server exits.
|
||||
|
||||
```json
|
||||
{ "method": "skills/list", "id": 25, "params": {
|
||||
@@ -1542,12 +1544,23 @@ The server also emits `skills/changed` notifications when watched local skill fi
|
||||
}
|
||||
```
|
||||
|
||||
```json
|
||||
{
|
||||
"method": "skills/extraRoots/set",
|
||||
"id": 26,
|
||||
"params": {
|
||||
"extraRoots": ["/Users/me/generated-skills"]
|
||||
}
|
||||
}
|
||||
{ "id": 26, "result": {} }
|
||||
```
|
||||
|
||||
To enable or disable a skill by absolute path:
|
||||
|
||||
```json
|
||||
{
|
||||
"method": "skills/config/write",
|
||||
"id": 26,
|
||||
"id": 27,
|
||||
"params": {
|
||||
"path": "/Users/alice/.codex/skills/skill-creator/SKILL.md",
|
||||
"name": null,
|
||||
@@ -1561,7 +1574,7 @@ To enable or disable a skill by name:
|
||||
```json
|
||||
{
|
||||
"method": "skills/config/write",
|
||||
"id": 27,
|
||||
"id": 28,
|
||||
"params": {
|
||||
"path": null,
|
||||
"name": "github:yeet",
|
||||
|
||||
@@ -354,6 +354,8 @@ impl MessageProcessor {
|
||||
app_list_shutdown_token,
|
||||
);
|
||||
let catalog_processor = CatalogRequestProcessor::new(
|
||||
outgoing.clone(),
|
||||
Arc::clone(&skills_watcher),
|
||||
auth_manager.clone(),
|
||||
Arc::clone(&thread_manager),
|
||||
Arc::clone(&config),
|
||||
@@ -1108,6 +1110,9 @@ impl MessageProcessor {
|
||||
ClientRequest::SkillsList { params, .. } => {
|
||||
self.catalog_processor.skills_list(params).await
|
||||
}
|
||||
ClientRequest::SkillsExtraRootsSet { params, .. } => {
|
||||
self.catalog_processor.skills_extra_roots_set(params).await
|
||||
}
|
||||
ClientRequest::HooksList { params, .. } => {
|
||||
self.catalog_processor.hooks_list(params).await
|
||||
}
|
||||
|
||||
@@ -156,6 +156,8 @@ use codex_app_server_protocol::ServerRequestResolvedNotification;
|
||||
use codex_app_server_protocol::SkillSummary;
|
||||
use codex_app_server_protocol::SkillsConfigWriteParams;
|
||||
use codex_app_server_protocol::SkillsConfigWriteResponse;
|
||||
use codex_app_server_protocol::SkillsExtraRootsSetParams;
|
||||
use codex_app_server_protocol::SkillsExtraRootsSetResponse;
|
||||
use codex_app_server_protocol::SkillsListParams;
|
||||
use codex_app_server_protocol::SkillsListResponse;
|
||||
use codex_app_server_protocol::SortDirection;
|
||||
|
||||
@@ -4,6 +4,8 @@ use futures::StreamExt;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct CatalogRequestProcessor {
|
||||
pub(super) outgoing: Arc<OutgoingMessageSender>,
|
||||
pub(super) skills_watcher: Arc<SkillsWatcher>,
|
||||
pub(super) auth_manager: Arc<AuthManager>,
|
||||
pub(super) thread_manager: Arc<ThreadManager>,
|
||||
pub(super) config: Arc<Config>,
|
||||
@@ -96,6 +98,8 @@ fn errors_to_info(
|
||||
|
||||
impl CatalogRequestProcessor {
|
||||
pub(crate) fn new(
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
skills_watcher: Arc<SkillsWatcher>,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
thread_manager: Arc<ThreadManager>,
|
||||
config: Arc<Config>,
|
||||
@@ -103,6 +107,8 @@ impl CatalogRequestProcessor {
|
||||
workspace_settings_cache: Arc<workspace_settings::WorkspaceSettingsCache>,
|
||||
) -> Self {
|
||||
Self {
|
||||
outgoing,
|
||||
skills_watcher,
|
||||
auth_manager,
|
||||
thread_manager,
|
||||
config,
|
||||
@@ -138,6 +144,15 @@ impl CatalogRequestProcessor {
|
||||
.map(|response| Some(response.into()))
|
||||
}
|
||||
|
||||
pub(crate) async fn skills_extra_roots_set(
|
||||
&self,
|
||||
params: SkillsExtraRootsSetParams,
|
||||
) -> Result<Option<ClientResponsePayload>, JSONRPCErrorError> {
|
||||
self.skills_extra_roots_set_response(params)
|
||||
.await
|
||||
.map(|response| Some(response.into()))
|
||||
}
|
||||
|
||||
pub(crate) async fn model_list(
|
||||
&self,
|
||||
params: ModelListParams,
|
||||
@@ -567,6 +582,24 @@ impl CatalogRequestProcessor {
|
||||
Ok(SkillsListResponse { data })
|
||||
}
|
||||
|
||||
async fn skills_extra_roots_set_response(
|
||||
&self,
|
||||
params: SkillsExtraRootsSetParams,
|
||||
) -> Result<SkillsExtraRootsSetResponse, JSONRPCErrorError> {
|
||||
let SkillsExtraRootsSetParams { extra_roots } = params;
|
||||
self.skills_watcher
|
||||
.register_runtime_extra_roots(&extra_roots);
|
||||
self.thread_manager
|
||||
.skills_manager()
|
||||
.set_extra_roots(extra_roots);
|
||||
self.outgoing
|
||||
.send_server_notification(ServerNotification::SkillsChanged(
|
||||
codex_app_server_protocol::SkillsChangedNotification {},
|
||||
))
|
||||
.await;
|
||||
Ok(SkillsExtraRootsSetResponse {})
|
||||
}
|
||||
|
||||
/// Handle `hooks/list` by resolving hooks for each requested cwd.
|
||||
async fn hooks_list_response(
|
||||
&self,
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use std::time::Duration;
|
||||
|
||||
use crate::outgoing_message::OutgoingMessageSender;
|
||||
@@ -15,6 +16,7 @@ use codex_file_watcher::ThrottledWatchReceiver;
|
||||
use codex_file_watcher::WatchPath;
|
||||
use codex_file_watcher::WatchRegistration;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
use tokio_util::sync::DropGuard;
|
||||
use tracing::warn;
|
||||
@@ -26,6 +28,7 @@ const WATCHER_THROTTLE_INTERVAL: Duration = Duration::from_millis(50);
|
||||
|
||||
pub(crate) struct SkillsWatcher {
|
||||
subscriber: FileWatcherSubscriber,
|
||||
runtime_extra_roots_registration: Mutex<WatchRegistration>,
|
||||
shutdown_token: CancellationToken,
|
||||
_shutdown_drop_guard: DropGuard,
|
||||
}
|
||||
@@ -48,6 +51,7 @@ impl SkillsWatcher {
|
||||
Self::spawn_event_loop(rx, skills_manager, outgoing, shutdown_token.child_token());
|
||||
Arc::new(Self {
|
||||
subscriber,
|
||||
runtime_extra_roots_registration: Mutex::new(WatchRegistration::default()),
|
||||
shutdown_token,
|
||||
_shutdown_drop_guard: shutdown_drop_guard,
|
||||
})
|
||||
@@ -57,6 +61,22 @@ impl SkillsWatcher {
|
||||
self.shutdown_token.cancel();
|
||||
}
|
||||
|
||||
pub(crate) fn register_runtime_extra_roots(&self, extra_roots: &[AbsolutePathBuf]) {
|
||||
let roots = extra_roots
|
||||
.iter()
|
||||
.map(|root| WatchPath {
|
||||
path: root.clone().into_path_buf(),
|
||||
recursive: true,
|
||||
})
|
||||
.collect();
|
||||
let registration = self.subscriber.register_paths(roots);
|
||||
let mut guard = self
|
||||
.runtime_extra_roots_registration
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
*guard = registration;
|
||||
}
|
||||
|
||||
pub(crate) async fn register_thread_config(
|
||||
&self,
|
||||
config: &Config,
|
||||
|
||||
@@ -71,6 +71,7 @@ use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ReviewStartParams;
|
||||
use codex_app_server_protocol::SendAddCreditsNudgeEmailParams;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
use codex_app_server_protocol::SkillsExtraRootsSetParams;
|
||||
use codex_app_server_protocol::SkillsListParams;
|
||||
use codex_app_server_protocol::ThreadArchiveParams;
|
||||
use codex_app_server_protocol::ThreadCompactStartParams;
|
||||
@@ -675,6 +676,15 @@ impl McpProcess {
|
||||
self.send_request("skills/list", params).await
|
||||
}
|
||||
|
||||
/// Send a `skills/extraRoots/set` JSON-RPC request.
|
||||
pub async fn send_skills_extra_roots_set_request(
|
||||
&mut self,
|
||||
params: SkillsExtraRootsSetParams,
|
||||
) -> anyhow::Result<i64> {
|
||||
let params = Some(serde_json::to_value(params)?);
|
||||
self.send_request("skills/extraRoots/set", params).await
|
||||
}
|
||||
|
||||
/// Send a `hooks/list` JSON-RPC request.
|
||||
pub async fn send_hooks_list_request(
|
||||
&mut self,
|
||||
|
||||
@@ -13,11 +13,14 @@ 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::SkillsExtraRootsSetParams;
|
||||
use codex_app_server_protocol::SkillsExtraRootsSetResponse;
|
||||
use codex_app_server_protocol::SkillsListParams;
|
||||
use codex_app_server_protocol::SkillsListResponse;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_config::types::AuthCredentialsStoreMode;
|
||||
use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::timeout;
|
||||
@@ -40,6 +43,23 @@ fn write_skill(root: &TempDir, name: &str) -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn expect_skills_changed_notification(
|
||||
mcp: &mut McpProcess,
|
||||
timeout_duration: Duration,
|
||||
) -> Result<()> {
|
||||
let notification = timeout(
|
||||
timeout_duration,
|
||||
mcp.read_stream_until_notification_message("skills/changed"),
|
||||
)
|
||||
.await??;
|
||||
let params = notification
|
||||
.params
|
||||
.context("skills/changed params must be present")?;
|
||||
let notification: SkillsChangedNotification = serde_json::from_value(params)?;
|
||||
assert_eq!(notification, SkillsChangedNotification {});
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn write_plugins_enabled_config_with_base_url(
|
||||
codex_home: &std::path::Path,
|
||||
base_url: &str,
|
||||
@@ -573,6 +593,150 @@ async fn skills_list_uses_cached_result_until_force_reload() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_extra_roots_set_updates_process_runtime_roots() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let cwd = TempDir::new()?;
|
||||
let extra_root = TempDir::new()?;
|
||||
let extra_skills_root = extra_root.path().join("skills");
|
||||
let skill_dir = extra_skills_root.join("runtime-skill");
|
||||
std::fs::create_dir_all(&skill_dir)?;
|
||||
std::fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
"---\nname: runtime-skill\ndescription: runtime skill\n---\n\n# Body\n",
|
||||
)?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let set_request_id = mcp
|
||||
.send_skills_extra_roots_set_request(SkillsExtraRootsSetParams {
|
||||
extra_roots: vec![AbsolutePathBuf::from_absolute_path(&extra_skills_root)?],
|
||||
})
|
||||
.await?;
|
||||
let set_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(set_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let _: SkillsExtraRootsSetResponse = to_response(set_response)?;
|
||||
expect_skills_changed_notification(&mut mcp, DEFAULT_TIMEOUT).await?;
|
||||
|
||||
let skills_request_id = mcp
|
||||
.send_skills_list_request(SkillsListParams {
|
||||
cwds: vec![cwd.path().to_path_buf()],
|
||||
force_reload: false,
|
||||
})
|
||||
.await?;
|
||||
let skills_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(skills_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let SkillsListResponse { data } = to_response(skills_response)?;
|
||||
assert_eq!(data.len(), 1);
|
||||
assert_eq!(data[0].errors, Vec::new());
|
||||
assert!(
|
||||
data[0]
|
||||
.skills
|
||||
.iter()
|
||||
.any(|skill| skill.name == "runtime-skill")
|
||||
);
|
||||
|
||||
let missing_root = extra_root.path().join("missing-skills");
|
||||
let reset_request_id = mcp
|
||||
.send_skills_extra_roots_set_request(SkillsExtraRootsSetParams {
|
||||
extra_roots: vec![AbsolutePathBuf::from_absolute_path(&missing_root)?],
|
||||
})
|
||||
.await?;
|
||||
let reset_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(reset_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let _: SkillsExtraRootsSetResponse = to_response(reset_response)?;
|
||||
expect_skills_changed_notification(&mut mcp, DEFAULT_TIMEOUT).await?;
|
||||
|
||||
let skills_request_id = mcp
|
||||
.send_skills_list_request(SkillsListParams {
|
||||
cwds: vec![cwd.path().to_path_buf()],
|
||||
force_reload: false,
|
||||
})
|
||||
.await?;
|
||||
let skills_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(skills_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let SkillsListResponse { data } = to_response(skills_response)?;
|
||||
assert_eq!(data.len(), 1);
|
||||
assert_eq!(data[0].errors, Vec::new());
|
||||
assert!(
|
||||
data[0]
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
|
||||
let clear_request_id = mcp
|
||||
.send_skills_extra_roots_set_request(SkillsExtraRootsSetParams {
|
||||
extra_roots: Vec::new(),
|
||||
})
|
||||
.await?;
|
||||
let clear_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(clear_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let _: SkillsExtraRootsSetResponse = to_response(clear_response)?;
|
||||
expect_skills_changed_notification(&mut mcp, DEFAULT_TIMEOUT).await?;
|
||||
let skills_request_id = mcp
|
||||
.send_skills_list_request(SkillsListParams {
|
||||
cwds: vec![cwd.path().to_path_buf()],
|
||||
force_reload: false,
|
||||
})
|
||||
.await?;
|
||||
let skills_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(skills_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let SkillsListResponse { data } = to_response(skills_response)?;
|
||||
assert_eq!(data.len(), 1);
|
||||
assert_eq!(data[0].errors, Vec::new());
|
||||
assert!(
|
||||
data[0]
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
|
||||
drop(mcp);
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??;
|
||||
let skills_request_id = mcp
|
||||
.send_skills_list_request(SkillsListParams {
|
||||
cwds: vec![cwd.path().to_path_buf()],
|
||||
force_reload: false,
|
||||
})
|
||||
.await?;
|
||||
let skills_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(skills_request_id)),
|
||||
)
|
||||
.await??;
|
||||
let SkillsListResponse { data } = to_response(skills_response)?;
|
||||
assert_eq!(data.len(), 1);
|
||||
assert_eq!(data[0].errors, Vec::new());
|
||||
assert!(
|
||||
data[0]
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_changed_notification_is_emitted_after_skill_change() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
@@ -235,6 +235,7 @@ pub(crate) async fn skill_roots(
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
cwd: &AbsolutePathBuf,
|
||||
plugin_skill_roots: Vec<PluginSkillRoot>,
|
||||
extra_skill_roots: Vec<AbsolutePathBuf>,
|
||||
) -> Vec<SkillRoot> {
|
||||
let home_dir =
|
||||
home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok());
|
||||
@@ -244,6 +245,7 @@ pub(crate) async fn skill_roots(
|
||||
cwd,
|
||||
home_dir.as_ref(),
|
||||
plugin_skill_roots,
|
||||
extra_skill_roots,
|
||||
)
|
||||
.await
|
||||
}
|
||||
@@ -254,6 +256,7 @@ async fn skill_roots_with_home_dir(
|
||||
cwd: &AbsolutePathBuf,
|
||||
home_dir: Option<&AbsolutePathBuf>,
|
||||
plugin_skill_roots: Vec<PluginSkillRoot>,
|
||||
extra_skill_roots: Vec<AbsolutePathBuf>,
|
||||
) -> Vec<SkillRoot> {
|
||||
let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir, fs.clone());
|
||||
roots.extend(plugin_skill_roots.into_iter().map(|root| SkillRoot {
|
||||
@@ -263,6 +266,13 @@ async fn skill_roots_with_home_dir(
|
||||
plugin_id: Some(root.plugin_id),
|
||||
plugin_root: Some(root.plugin_root),
|
||||
}));
|
||||
roots.extend(extra_skill_roots.into_iter().map(|path| SkillRoot {
|
||||
path,
|
||||
scope: SkillScope::User,
|
||||
file_system: Arc::clone(&LOCAL_FS),
|
||||
plugin_id: None,
|
||||
plugin_root: None,
|
||||
}));
|
||||
roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await);
|
||||
dedupe_skill_roots_by_path(&mut roots);
|
||||
roots
|
||||
@@ -1051,7 +1061,15 @@ pub(crate) async fn skill_roots_from_layer_stack(
|
||||
cwd: &AbsolutePathBuf,
|
||||
home_dir: Option<&AbsolutePathBuf>,
|
||||
) -> Vec<SkillRoot> {
|
||||
skill_roots_with_home_dir(Some(fs), config_layer_stack, cwd, home_dir, Vec::new()).await
|
||||
skill_roots_with_home_dir(
|
||||
Some(fs),
|
||||
config_layer_stack,
|
||||
cwd,
|
||||
home_dir,
|
||||
Vec::new(),
|
||||
Vec::new(),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -1900,6 +1900,7 @@ async fn skill_roots_include_admin_with_lowest_priority() {
|
||||
&cfg.config_layer_stack,
|
||||
&cfg.cwd,
|
||||
Vec::new(),
|
||||
Vec::new(),
|
||||
)
|
||||
.await
|
||||
.into_iter()
|
||||
|
||||
@@ -51,6 +51,7 @@ impl SkillsLoadInput {
|
||||
pub struct SkillsManager {
|
||||
codex_home: AbsolutePathBuf,
|
||||
restriction_product: Option<Product>,
|
||||
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
|
||||
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, SkillLoadOutcome>>,
|
||||
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, SkillLoadOutcome>>,
|
||||
}
|
||||
@@ -68,6 +69,7 @@ impl SkillsManager {
|
||||
let manager = Self {
|
||||
codex_home,
|
||||
restriction_product,
|
||||
extra_roots: RwLock::new(Vec::new()),
|
||||
cache_by_cwd: RwLock::new(HashMap::new()),
|
||||
cache_by_config: RwLock::new(HashMap::new()),
|
||||
};
|
||||
@@ -81,6 +83,17 @@ impl SkillsManager {
|
||||
manager
|
||||
}
|
||||
|
||||
pub fn set_extra_roots(&self, extra_roots: Vec<AbsolutePathBuf>) {
|
||||
{
|
||||
let mut roots = self
|
||||
.extra_roots
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
*roots = extra_roots;
|
||||
}
|
||||
self.clear_cache();
|
||||
}
|
||||
|
||||
/// Load skills for an already-constructed [`Config`], avoiding any additional config-layer
|
||||
/// loading.
|
||||
///
|
||||
@@ -118,6 +131,7 @@ impl SkillsManager {
|
||||
&input.config_layer_stack,
|
||||
&input.cwd,
|
||||
input.effective_skill_roots.clone(),
|
||||
self.extra_roots(),
|
||||
)
|
||||
.await;
|
||||
if !input.bundled_skills_enabled {
|
||||
@@ -145,6 +159,7 @@ impl SkillsManager {
|
||||
&input.config_layer_stack,
|
||||
&input.cwd,
|
||||
input.effective_skill_roots.clone(),
|
||||
self.extra_roots(),
|
||||
)
|
||||
.await;
|
||||
if !bundled_skills_enabled_from_stack(&input.config_layer_stack) {
|
||||
@@ -214,6 +229,13 @@ impl SkillsManager {
|
||||
Err(err) => err.into_inner().get(cache_key).cloned(),
|
||||
}
|
||||
}
|
||||
|
||||
fn extra_roots(&self) -> Vec<AbsolutePathBuf> {
|
||||
match self.extra_roots.read() {
|
||||
Ok(roots) => roots.clone(),
|
||||
Err(err) => err.into_inner().clone(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
|
||||
|
||||
@@ -221,6 +221,128 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() {
|
||||
assert_eq!(outcome2.skills, outcome1.skills);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
|
||||
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_input = SkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
Vec::new(),
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&config_layer_stack),
|
||||
);
|
||||
let empty_outcome = skills_manager
|
||||
.skills_for_cwd(
|
||||
&skills_input,
|
||||
/*force_reload*/ false,
|
||||
Some(Arc::clone(&LOCAL_FS)),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
empty_outcome
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
|
||||
let extra_skills_root = extra_root.path().join("skills");
|
||||
let skill_dir = extra_skills_root.join("runtime-skill");
|
||||
fs::create_dir_all(&skill_dir).expect("create skill dir");
|
||||
fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
"---\nname: runtime-skill\ndescription: runtime skill\n---\n\n# Body\n",
|
||||
)
|
||||
.expect("write skill");
|
||||
skills_manager.set_extra_roots(vec![extra_skills_root.abs()]);
|
||||
|
||||
let runtime_outcome = skills_manager
|
||||
.skills_for_cwd(
|
||||
&skills_input,
|
||||
/*force_reload*/ false,
|
||||
Some(Arc::clone(&LOCAL_FS)),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
runtime_outcome
|
||||
.skills
|
||||
.iter()
|
||||
.any(|skill| skill.name == "runtime-skill")
|
||||
);
|
||||
|
||||
skills_manager.set_extra_roots(vec![extra_root.path().join("missing-skills").abs()]);
|
||||
let replaced_outcome = skills_manager
|
||||
.skills_for_cwd(
|
||||
&skills_input,
|
||||
/*force_reload*/ false,
|
||||
Some(Arc::clone(&LOCAL_FS)),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(replaced_outcome.errors, Vec::new());
|
||||
assert!(
|
||||
replaced_outcome
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn set_extra_roots_applies_to_config_loads_and_empty_clears() {
|
||||
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 empty_outcome =
|
||||
skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await;
|
||||
assert!(
|
||||
empty_outcome
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
|
||||
let extra_skills_root = extra_root.path().join("skills");
|
||||
let skill_dir = extra_skills_root.join("runtime-skill");
|
||||
fs::create_dir_all(&skill_dir).expect("create skill dir");
|
||||
fs::write(
|
||||
skill_dir.join("SKILL.md"),
|
||||
"---\nname: runtime-skill\ndescription: runtime skill\n---\n\n# Body\n",
|
||||
)
|
||||
.expect("write skill");
|
||||
skills_manager.set_extra_roots(vec![extra_skills_root.abs()]);
|
||||
|
||||
let runtime_outcome =
|
||||
skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await;
|
||||
assert!(
|
||||
runtime_outcome
|
||||
.skills
|
||||
.iter()
|
||||
.any(|skill| skill.name == "runtime-skill")
|
||||
);
|
||||
|
||||
skills_manager.set_extra_roots(Vec::new());
|
||||
let cleared_outcome =
|
||||
skills_for_config_with_stack(&skills_manager, &cwd, &config_layer_stack, &[]).await;
|
||||
assert!(
|
||||
cleared_outcome
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "runtime-skill")
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_for_config_disables_plugin_skills_by_name() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
Reference in New Issue
Block a user