mirror of
https://github.com/openai/codex.git
synced 2026-10-19 17:28:46 +00:00
Encapsulate watchable skill root selection (#37833)
## Why The app server should not need access to host skill root internals to decide which paths require filesystem watching. ## What changed - Add `HostSkillsService::watchable_skill_root_paths` to return non-plugin, non-system skill roots for the watcher. - Make `HostSkillRoot` and root resolution private to the skills extension. - Update the app-server watcher to consume the filtered paths directly. ## Testing Add a host service test confirming that user skill roots remain watchable while plugin and generated system roots are excluded. GitOrigin-RevId: b3e3b576e4ffe96230af71d9bf7c6c315b1705d3
This commit is contained in:
@@ -15,7 +15,6 @@ use codex_file_watcher::Receiver;
|
||||
use codex_file_watcher::ThrottledWatchReceiver;
|
||||
use codex_file_watcher::WatchPath;
|
||||
use codex_file_watcher::WatchRegistration;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||
use codex_skills::system_cache_root_dir;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -121,14 +120,11 @@ impl SkillsWatcher {
|
||||
);
|
||||
let roots = thread_manager
|
||||
.skills_service()
|
||||
.skill_roots_for_config(&skills_input, Some(environment.get_filesystem()))
|
||||
.watchable_skill_root_paths(&skills_input, environment.get_filesystem())
|
||||
.await
|
||||
.into_iter()
|
||||
// Plugin roots have explicit lifecycle invalidation; generated system skills are
|
||||
// installed before this watcher starts.
|
||||
.filter(|root| root.plugin_identity().is_none() && root.scope != SkillScope::System)
|
||||
.map(|root| WatchPath {
|
||||
path: root.path.into_path_buf(),
|
||||
.map(|path| WatchPath {
|
||||
path: path.into_path_buf(),
|
||||
recursive: true,
|
||||
})
|
||||
.collect();
|
||||
|
||||
@@ -197,7 +197,24 @@ impl HostSkillsService {
|
||||
.await
|
||||
}
|
||||
|
||||
pub async fn skill_roots_for_config(
|
||||
/// Returns filesystem roots whose changes should invalidate discovered host skills.
|
||||
///
|
||||
/// Plugin roots have their own lifecycle invalidation, and bundled roots are installed before
|
||||
/// filesystem watching begins.
|
||||
pub async fn watchable_skill_root_paths(
|
||||
&self,
|
||||
input: &HostSkillsLoadInput,
|
||||
fs: Arc<dyn ExecutorFileSystem>,
|
||||
) -> Vec<AbsolutePathBuf> {
|
||||
self.skill_roots_for_config(input, Some(fs))
|
||||
.await
|
||||
.into_iter()
|
||||
.filter(|root| root.plugin_identity().is_none() && root.scope != SkillScope::System)
|
||||
.map(|root| root.path)
|
||||
.collect()
|
||||
}
|
||||
|
||||
async fn skill_roots_for_config(
|
||||
&self,
|
||||
input: &HostSkillsLoadInput,
|
||||
fs: Option<Arc<dyn ExecutorFileSystem>>,
|
||||
|
||||
@@ -207,6 +207,40 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() {
|
||||
assert_eq!(outcome2.skills, outcome1.skills);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn watchable_skill_root_paths_exclude_plugin_and_system_roots() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let cwd = tempfile::tempdir().expect("tempdir");
|
||||
let skill_path = write_plugin_skill(
|
||||
&codex_home,
|
||||
"test",
|
||||
"sample",
|
||||
"search",
|
||||
"search",
|
||||
"plugin skill",
|
||||
);
|
||||
let plugin_skill_root = plugin_skill_root_for_skill_path(&skill_path, "sample@test", "sample");
|
||||
let config_layer_stack = config_stack(&codex_home, "");
|
||||
let input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
vec![plugin_skill_root.clone()],
|
||||
config_layer_stack,
|
||||
/*bundled_skills_enabled*/ true,
|
||||
);
|
||||
let skills_service = HostSkillsService::new(
|
||||
codex_home.path().abs(),
|
||||
/*bundled_skills_enabled*/ true,
|
||||
);
|
||||
|
||||
let watchable_paths = skills_service
|
||||
.watchable_skill_root_paths(&input, Arc::clone(&LOCAL_FS))
|
||||
.await;
|
||||
|
||||
assert!(watchable_paths.contains(&codex_home.path().join("skills").abs()));
|
||||
assert!(!watchable_paths.contains(&plugin_skill_root.path));
|
||||
assert!(!watchable_paths.contains(&codex_home.path().join("skills/.system").abs()));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn snapshot_for_config_merges_extension_host_and_legacy_plugin_roots() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
@@ -36,7 +36,6 @@ pub use host_service::HostSkillsRequest;
|
||||
pub use host_service::HostSkillsService;
|
||||
pub use host_service::bundled_skills_enabled_from_stack;
|
||||
pub use host_snapshot::HostSkillsSnapshot;
|
||||
pub use loader::HostSkillRoot;
|
||||
pub use provider::ExecutorSkillProvider;
|
||||
pub use provider::HostSkillProvider;
|
||||
pub use provider::OrchestratorSkillProvider;
|
||||
|
||||
@@ -33,10 +33,10 @@ use super::metadata::validate_len;
|
||||
use super::namespace::SkillNamespaceResolver;
|
||||
|
||||
/// A resolved host skill root ready for filesystem discovery.
|
||||
pub struct HostSkillRoot {
|
||||
pub path: AbsolutePathBuf,
|
||||
pub scope: SkillScope,
|
||||
pub file_system: Arc<dyn ExecutorFileSystem>,
|
||||
pub(crate) struct HostSkillRoot {
|
||||
pub(crate) path: AbsolutePathBuf,
|
||||
pub(crate) scope: SkillScope,
|
||||
pub(crate) file_system: Arc<dyn ExecutorFileSystem>,
|
||||
plugin: Option<PluginSkillRootContext>,
|
||||
}
|
||||
|
||||
@@ -76,7 +76,7 @@ impl HostSkillRoot {
|
||||
}
|
||||
|
||||
/// Returns the owning plugin identity when this root belongs to a plugin.
|
||||
pub fn plugin_identity(&self) -> Option<&PluginIdentity> {
|
||||
pub(crate) fn plugin_identity(&self) -> Option<&PluginIdentity> {
|
||||
self.plugin.as_ref().map(|plugin| &plugin.identity)
|
||||
}
|
||||
|
||||
|
||||
@@ -9,7 +9,7 @@ mod namespace;
|
||||
|
||||
pub(crate) use environment::load_environment_skills_from_discovery;
|
||||
pub(crate) use environment::load_environment_skills_from_root;
|
||||
pub use host::HostSkillRoot;
|
||||
pub(crate) use host::HostSkillRoot;
|
||||
pub(crate) use host::HostSkillRootSnapshot;
|
||||
pub(crate) use host::load_host_skill_root;
|
||||
pub(crate) use host_merge::load_and_merge_host_skill_roots;
|
||||
|
||||
Reference in New Issue
Block a user