mirror of
https://github.com/openai/codex.git
synced 2026-08-23 13:09:46 +00:00
Tighten the skills extension API surface (#37838)
## What changed - Restrict host loading, snapshot, and outcome implementation details to the skills extension crate. - Require `HostSkillsLoadInput` construction through its public constructor instead of exposing its fields. - Remove redundant implicit-invocation helpers from `SkillLoadOutcome`; use the `ImplicitSkillLookup` interface for enabled-skill lookup instead. GitOrigin-RevId: e79c77fc3e69ecc0231b0c87e5f9f24a5487f9e4
This commit is contained in:
@@ -27,7 +27,7 @@ pub struct SkillLoadOutcome {
|
||||
|
||||
impl SkillLoadOutcome {
|
||||
/// Builds an already-composed outcome while retaining the filesystem that supplied each skill.
|
||||
pub fn from_parts(
|
||||
pub(crate) fn from_parts(
|
||||
skills: Vec<SkillMetadata>,
|
||||
errors: Vec<SkillError>,
|
||||
skill_roots: Vec<AbsolutePathBuf>,
|
||||
@@ -52,25 +52,13 @@ impl SkillLoadOutcome {
|
||||
!self.disabled_paths.contains(&skill.path_to_skills_md)
|
||||
}
|
||||
|
||||
pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool {
|
||||
self.is_skill_enabled(skill) && skill.allows_implicit_invocation()
|
||||
}
|
||||
|
||||
pub fn allowed_skills_for_implicit_invocation(&self) -> Vec<SkillMetadata> {
|
||||
self.skills
|
||||
.iter()
|
||||
.filter(|skill| self.is_skill_allowed_for_implicit_invocation(skill))
|
||||
.cloned()
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub fn skills_with_enabled(&self) -> impl Iterator<Item = (&SkillMetadata, bool)> {
|
||||
pub(crate) fn skills_with_enabled(&self) -> impl Iterator<Item = (&SkillMetadata, bool)> {
|
||||
self.skills
|
||||
.iter()
|
||||
.map(|skill| (skill, self.is_skill_enabled(skill)))
|
||||
}
|
||||
|
||||
pub fn with_disabled_paths(mut self, disabled_paths: HashSet<AbsolutePathBuf>) -> Self {
|
||||
pub(crate) fn with_disabled_paths(mut self, disabled_paths: HashSet<AbsolutePathBuf>) -> Self {
|
||||
self.disabled_paths = disabled_paths;
|
||||
let mut by_scripts_dir = HashMap::new();
|
||||
let mut by_doc_path = HashMap::new();
|
||||
@@ -92,18 +80,18 @@ impl SkillLoadOutcome {
|
||||
self
|
||||
}
|
||||
|
||||
pub fn is_agent_plugin_skill(&self, skill: &SkillMetadata) -> bool {
|
||||
pub(crate) fn is_agent_plugin_skill(&self, skill: &SkillMetadata) -> bool {
|
||||
self.agent_plugin_skill_paths
|
||||
.contains(&skill.path_to_skills_md)
|
||||
}
|
||||
|
||||
/// Returns the discovery root that supplied a loaded skill path.
|
||||
pub fn skill_root_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> {
|
||||
pub(crate) fn skill_root_for_path(&self, path: &AbsolutePathBuf) -> Option<&AbsolutePathBuf> {
|
||||
self.skill_root_by_path.get(path)
|
||||
}
|
||||
|
||||
/// Returns the logical path used to discover a canonical skill path.
|
||||
pub fn skill_discovery_path_for_path(
|
||||
pub(crate) fn skill_discovery_path_for_path(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
) -> Option<&AbsolutePathBuf> {
|
||||
@@ -111,7 +99,7 @@ impl SkillLoadOutcome {
|
||||
}
|
||||
|
||||
/// Returns loaded skill roots in discovery order.
|
||||
pub fn skill_roots_in_discovery_order(&self) -> impl Iterator<Item = &AbsolutePathBuf> {
|
||||
pub(crate) fn skill_roots_in_discovery_order(&self) -> impl Iterator<Item = &AbsolutePathBuf> {
|
||||
self.skill_roots.iter()
|
||||
}
|
||||
|
||||
@@ -124,7 +112,7 @@ impl SkillLoadOutcome {
|
||||
}
|
||||
|
||||
/// Reads one loaded skill through the filesystem that discovered it.
|
||||
pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result<String> {
|
||||
pub(crate) async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result<String> {
|
||||
let fs = self
|
||||
.file_system_for_skill(skill)
|
||||
.unwrap_or_else(|| Arc::clone(&LOCAL_FS));
|
||||
|
||||
@@ -40,10 +40,10 @@ use crate::loader::load_and_merge_host_skill_roots_with_request_snapshots;
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct HostSkillsLoadInput {
|
||||
pub cwd: AbsolutePathBuf,
|
||||
pub effective_skill_roots: Vec<PluginSkillRoot>,
|
||||
pub config_layer_stack: ConfigLayerStack,
|
||||
pub bundled_skills_enabled: bool,
|
||||
cwd: AbsolutePathBuf,
|
||||
effective_skill_roots: Vec<PluginSkillRoot>,
|
||||
config_layer_stack: ConfigLayerStack,
|
||||
bundled_skills_enabled: bool,
|
||||
plugin_skill_snapshots: Option<SkillRootSnapshots<PluginSkillRoot>>,
|
||||
}
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@ use codex_config::ConfigLayerSource;
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigRequirementsToml;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_skills::ImplicitSkillLookup;
|
||||
use codex_skills::LoadedSkillRoot;
|
||||
use codex_skills::SkillRootSnapshotCache;
|
||||
use codex_skills::SkillRootSnapshots;
|
||||
@@ -662,10 +663,9 @@ async fn skills_for_config_disables_plugin_skills_by_name() {
|
||||
assert_eq!(skill.path_to_skills_md, skill_path);
|
||||
assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md));
|
||||
assert!(
|
||||
!outcome
|
||||
.allowed_skills_for_implicit_invocation()
|
||||
.iter()
|
||||
.any(|allowed_skill| allowed_skill.path_to_skills_md == skill.path_to_skills_md)
|
||||
outcome
|
||||
.implicit_skill_for_doc_path(&skill.path_to_skills_md)
|
||||
.is_none()
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -109,13 +109,13 @@ impl HostSkillRoot {
|
||||
|
||||
/// Skills and errors loaded from one canonical host root.
|
||||
#[derive(Clone)]
|
||||
pub struct HostSkillRootSnapshot {
|
||||
pub root: AbsolutePathBuf,
|
||||
pub skills: Vec<SkillMetadata>,
|
||||
pub skill_discovery_path_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub errors: Vec<SkillError>,
|
||||
pub file_system: Arc<dyn ExecutorFileSystem>,
|
||||
pub is_agent_plugin: bool,
|
||||
pub(crate) struct HostSkillRootSnapshot {
|
||||
pub(crate) root: AbsolutePathBuf,
|
||||
pub(crate) skills: Vec<SkillMetadata>,
|
||||
pub(crate) skill_discovery_path_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub(crate) errors: Vec<SkillError>,
|
||||
pub(crate) file_system: Arc<dyn ExecutorFileSystem>,
|
||||
pub(crate) is_agent_plugin: bool,
|
||||
}
|
||||
|
||||
struct ResolvedDiscoveredSkill {
|
||||
@@ -124,7 +124,7 @@ struct ResolvedDiscoveredSkill {
|
||||
path_uri: PathUri,
|
||||
}
|
||||
|
||||
pub async fn load_host_skill_root(root: HostSkillRoot) -> HostSkillRootSnapshot {
|
||||
pub(crate) async fn load_host_skill_root(root: HostSkillRoot) -> HostSkillRootSnapshot {
|
||||
let is_agent_plugin = root.discovery_mode() == SkillDiscoveryMode::DirectChildren;
|
||||
let canonical_root =
|
||||
canonicalize_for_skill_identity(root.file_system.as_ref(), &root.path).await;
|
||||
|
||||
Reference in New Issue
Block a user