From a4b129eb3e1a6929c09d6e2e1af0638122c56f0d Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Fri, 7 Aug 2026 12:41:51 +0000 Subject: [PATCH] Add shared skill root loading interfaces (#37439) ## What changed - Add `SkillRootLoader` and its request/result types to `codex-skills` so skill roots can be loaded through a shared, object-safe interface. - Add owner-managed snapshot cache handles that support reuse and use cache allocation identity for equality and hashing. - Move `SkillError` into `codex-skills` and re-export the new loading API. ## Testing - Verify snapshot sharing and identity semantics. - Verify loaders can be used as shared trait objects. GitOrigin-RevId: a26a8101008a6c862824a7346cdbe6d05f4bcc01 --- codex-rs/core-skills/src/model.rs | 7 +- codex-rs/skills/src/lib.rs | 9 ++ codex-rs/skills/src/loading.rs | 119 +++++++++++++++++++++++++++ codex-rs/skills/src/loading_tests.rs | 79 ++++++++++++++++++ 4 files changed, 208 insertions(+), 6 deletions(-) create mode 100644 codex-rs/skills/src/loading.rs create mode 100644 codex-rs/skills/src/loading_tests.rs diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index 58cda12e3d..814bd136a6 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -8,6 +8,7 @@ use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; pub use codex_skills::SkillDependencies; +pub use codex_skills::SkillError; pub use codex_skills::SkillInterface; pub use codex_skills::SkillMetadata; pub use codex_skills::SkillPolicy; @@ -15,12 +16,6 @@ pub use codex_skills::SkillToolDependency; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct SkillError { - pub path: AbsolutePathBuf, - pub message: String, -} - #[derive(Debug, Clone, Default)] pub struct SkillLoadOutcome { pub skills: Vec, diff --git a/codex-rs/skills/src/lib.rs b/codex-rs/skills/src/lib.rs index d09dd3dbe4..a33a552902 100644 --- a/codex-rs/skills/src/lib.rs +++ b/codex-rs/skills/src/lib.rs @@ -1,5 +1,6 @@ mod interface; mod invocation; +mod loading; mod mentions; mod model; mod name_counts; @@ -12,6 +13,14 @@ pub use interface::SkillInterfaceFile; pub use interface::resolve_skill_interface; pub use invocation::ImplicitSkillLookup; pub use invocation::detect_implicit_skill_invocation_for_command; +pub use loading::LoadedSkillRoot; +pub use loading::LoadedSkills; +pub use loading::SkillError; +pub use loading::SkillLoadFuture; +pub use loading::SkillRootLoadRequest; +pub use loading::SkillRootLoader; +pub use loading::SkillRootSnapshotCache; +pub use loading::SkillRootSnapshots; pub use mentions::ToolMentionKind; pub use mentions::ToolMentions; pub use mentions::app_id_from_path; diff --git a/codex-rs/skills/src/loading.rs b/codex-rs/skills/src/loading.rs new file mode 100644 index 0000000000..59458b2417 --- /dev/null +++ b/codex-rs/skills/src/loading.rs @@ -0,0 +1,119 @@ +use std::collections::HashMap; +use std::fmt; +use std::future::Future; +use std::hash::Hash; +use std::hash::Hasher; +use std::pin::Pin; +use std::sync::Arc; + +use codex_protocol::protocol::Product; +use codex_utils_absolute_path::AbsolutePathBuf; + +use crate::SkillMetadata; + +/// A skill document that could not be read, parsed, or validated. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkillError { + pub path: AbsolutePathBuf, + pub message: String, +} + +/// Filesystem-independent metadata discovered beneath one canonical skill root. +#[derive(Debug, Clone, PartialEq)] +pub struct LoadedSkillRoot { + pub root: AbsolutePathBuf, + pub skills: Vec, + pub skill_discovery_path_by_path: Arc>, + pub errors: Vec, + pub is_agent_plugin: bool, +} + +/// Skills and errors produced by loading an ordered set of roots. +#[derive(Debug, Clone, Default, PartialEq)] +pub struct LoadedSkills { + pub skills: Vec, + pub errors: Vec, +} + +/// Caches parsed roots without assigning ownership of the cache to a loader. +/// +/// Implementations own storage and determine its lifetime; loaders may reuse an existing snapshot +/// or publish a newly loaded one without learning about the caller's broader state. +pub trait SkillRootSnapshotCache: Send + Sync { + fn get(&self, root: &Root) -> Option; + + fn insert(&self, root: Root, snapshot: LoadedSkillRoot); +} + +/// Shared access to one owner-managed collection of parsed skill roots. +/// +/// Equality and hashing use the cache allocation's identity so callers can include the handle in +/// their own cache keys without inspecting or taking ownership of its contents. +pub struct SkillRootSnapshots { + cache: Arc>, +} + +impl SkillRootSnapshots { + pub fn new(cache: Arc>) -> Self { + Self { cache } + } + + pub fn get(&self, root: &Root) -> Option { + self.cache.get(root) + } + + pub fn insert(&self, root: Root, snapshot: LoadedSkillRoot) { + self.cache.insert(root, snapshot); + } +} + +impl Clone for SkillRootSnapshots { + fn clone(&self) -> Self { + Self { + cache: Arc::clone(&self.cache), + } + } +} + +impl PartialEq for SkillRootSnapshots { + fn eq(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.cache, &other.cache) + } +} + +impl Eq for SkillRootSnapshots {} + +impl Hash for SkillRootSnapshots { + fn hash(&self, state: &mut H) { + (Arc::as_ptr(&self.cache) as *const ()).hash(state); + } +} + +impl fmt::Debug for SkillRootSnapshots { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("SkillRootSnapshots").finish_non_exhaustive() + } +} + +/// Inputs for loading roots while optionally reusing snapshots owned by the caller. +#[derive(Debug, Clone)] +pub struct SkillRootLoadRequest { + pub roots: Vec, + pub restriction_product: Option, + pub snapshots: Option>, +} + +/// Object-safe future returned by a shared skill-root loader. +pub type SkillLoadFuture<'a, T> = Pin + Send + 'a>>; + +/// Loads ordered skill roots without owning their source inventory or capability state. +/// +/// Implementations are responsible for filesystem access, metadata parsing, root precedence, +/// product filtering, and reuse of any snapshots supplied in the request. +pub trait SkillRootLoader: Send + Sync { + fn load_roots(&self, request: SkillRootLoadRequest) -> SkillLoadFuture<'_, LoadedSkills>; +} + +#[cfg(test)] +#[path = "loading_tests.rs"] +mod tests; diff --git a/codex-rs/skills/src/loading_tests.rs b/codex-rs/skills/src/loading_tests.rs new file mode 100644 index 0000000000..4abad3be02 --- /dev/null +++ b/codex-rs/skills/src/loading_tests.rs @@ -0,0 +1,79 @@ +use std::collections::HashMap; +use std::collections::HashSet; +use std::sync::Arc; +use std::sync::Mutex; + +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; + +use super::LoadedSkillRoot; +use super::LoadedSkills; +use super::SkillLoadFuture; +use super::SkillRootLoadRequest; +use super::SkillRootLoader; +use super::SkillRootSnapshotCache; +use super::SkillRootSnapshots; + +#[derive(Default)] +struct TestSnapshotCache { + snapshots: Mutex>, +} + +impl SkillRootSnapshotCache for TestSnapshotCache { + fn get(&self, root: &String) -> Option { + self.snapshots.lock().unwrap().get(root).cloned() + } + + fn insert(&self, root: String, snapshot: LoadedSkillRoot) { + self.snapshots.lock().unwrap().insert(root, snapshot); + } +} + +struct TestSkillRootLoader; + +impl SkillRootLoader for TestSkillRootLoader { + fn load_roots( + &self, + _request: SkillRootLoadRequest, + ) -> SkillLoadFuture<'_, LoadedSkills> { + Box::pin(async { LoadedSkills::default() }) + } +} + +#[test] +fn snapshot_handles_share_owner_managed_values_and_identity() { + let snapshots = SkillRootSnapshots::new(Arc::new(TestSnapshotCache::default())); + let cloned_snapshots = snapshots.clone(); + let separate_snapshots = SkillRootSnapshots::new(Arc::new(TestSnapshotCache::default())); + let root = "plugin-root".to_string(); + let snapshot = LoadedSkillRoot { + root: AbsolutePathBuf::try_from(std::env::temp_dir()).unwrap(), + skills: Vec::new(), + skill_discovery_path_by_path: Arc::new(HashMap::new()), + errors: Vec::new(), + is_agent_plugin: false, + }; + + snapshots.insert(root.clone(), snapshot.clone()); + + assert_eq!(cloned_snapshots.get(&root), Some(snapshot)); + assert_eq!(separate_snapshots.get(&root), None); + assert_eq!(snapshots, cloned_snapshots); + assert_ne!(snapshots, separate_snapshots); + assert_eq!( + HashSet::from([snapshots, cloned_snapshots, separate_snapshots]).len(), + 2 + ); +} + +#[test] +fn skill_root_loader_supports_shared_trait_objects() { + let loader: Arc> = Arc::new(TestSkillRootLoader); + let future = loader.load_roots(SkillRootLoadRequest { + roots: vec!["plugin-root".to_string()], + restriction_product: None, + snapshots: None, + }); + + drop(future); +}