Keep multi-workspace skill listings consistent (#37812)

## Why

A single `skills/list` request should use one view of shared skill roots across
all requested workspaces while still applying each workspace's skill rules.

## What changed

- Add a request-scoped host skills view that reuses non-plugin root snapshots
  across `cwd` entries without persisting them across requests.
- Resolve user-scoped plugin skill roots once per request, then apply workspace
  skill configuration separately for each `cwd`.
- Make `forceReload` refresh plugin roots before building the request-scoped
  view so subsequent entries see the refreshed skills.

## Testing

Expanded `skills/list` and host skills service tests to cover multiple ordered
workspaces, per-workspace skill rules, request-local snapshots, cached results,
and forced plugin-root refreshes.

GitOrigin-RevId: eedd3a4c3e213b7f30df6cacd3adf23ba2967437
This commit is contained in:
Charlie Marsh authored and copyberry committed 2026-08-10 11:27:22 +00:00
1 parent 34ecac1f2b
commit 1c042dd4d8
9 files changed
+429 -140

No files matched your search

+71 -24
View File
@@ -33,8 +33,10 @@ use crate::HostSkillsSnapshot;
use crate::SkillLoadOutcome;
use crate::host_roots::resolve_skill_roots;
use crate::loader::HostSkillRoot;
use crate::loader::HostSkillRootSnapshot;
use crate::loader::MAX_CONCURRENT_ROOT_SCANS;
use crate::loader::load_and_merge_host_skill_roots;
use crate::loader::load_and_merge_host_skill_roots_with_request_snapshots;
#[derive(Debug, Clone)]
pub struct HostSkillsLoadInput {
@@ -84,6 +86,33 @@ pub struct HostSkillsService {
root_scan_slots: Arc<Semaphore>,
}
pub(crate) type RequestSkillRootSnapshots =
RwLock<HashMap<ConfigSkillRootCacheKey, Arc<OnceCell<HostSkillRootSnapshot>>>>;
/// Shares host-root scans between workspaces for the lifetime of one request.
pub struct HostSkillsRequest<'a> {
service: &'a HostSkillsService,
root_snapshots: RequestSkillRootSnapshots,
}
impl HostSkillsRequest<'_> {
pub async fn snapshot_for_cwd(
&self,
input: &HostSkillsLoadInput,
force_reload: bool,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> HostSkillsSnapshot {
self.service
.snapshot_for_cwd_with_root_snapshots(
input,
force_reload,
fs,
Some(&self.root_snapshots),
)
.await
}
}
impl HostSkillsService {
pub fn new(codex_home: AbsolutePathBuf, bundled_skills_enabled: bool) -> Self {
Self::new_with_restriction_product(codex_home, bundled_skills_enabled, Some(Product::Codex))
@@ -110,6 +139,14 @@ impl HostSkillsService {
service
}
/// Creates a request-local view without changing persistent plugin snapshots.
pub fn for_request(&self) -> HostSkillsRequest<'_> {
HostSkillsRequest {
service: self,
root_snapshots: RwLock::new(HashMap::new()),
}
}
pub fn set_extra_roots(&self, extra_roots: Vec<AbsolutePathBuf>) {
{
let mut roots = self
@@ -155,6 +192,7 @@ impl HostSkillsService {
&skill_config_rules,
cache_key,
/*force_reload*/ false,
/*request_root_snapshots*/ None,
)
.await
}
@@ -181,11 +219,12 @@ impl HostSkillsService {
roots
}
pub async fn snapshot_for_cwd(
async fn snapshot_for_cwd_with_root_snapshots(
&self,
input: &HostSkillsLoadInput,
force_reload: bool,
fs: Option<Arc<dyn ExecutorFileSystem>>,
request_root_snapshots: Option<&RequestSkillRootSnapshots>,
) -> HostSkillsSnapshot {
let bundled_skills_enabled = bundled_skills_enabled_from_stack(&input.config_layer_stack);
if bundled_skills_enabled {
@@ -224,11 +263,12 @@ impl HostSkillsService {
&skill_config_rules,
cache_key,
force_reload,
request_root_snapshots,
)
.await
} else {
HostSkillsSnapshot::new(Arc::new(
self.build_skill_outcome(input, roots, &skill_config_rules)
self.build_skill_outcome(input, roots, &skill_config_rules, request_root_snapshots)
.await,
))
};
@@ -249,6 +289,7 @@ impl HostSkillsService {
skill_config_rules: &SkillConfigRules,
cache_key: ConfigSkillsCacheKey,
force_reload: bool,
request_root_snapshots: Option<&RequestSkillRootSnapshots>,
) -> HostSkillsSnapshot {
let snapshot_cell = {
let mut cache = self
@@ -271,8 +312,13 @@ impl HostSkillsService {
snapshot_cell
.get_or_init(|| async {
HostSkillsSnapshot::new(Arc::new(
self.build_skill_outcome(input, roots, skill_config_rules)
.await,
self.build_skill_outcome(
input,
roots,
skill_config_rules,
request_root_snapshots,
)
.await,
))
})
.await
@@ -285,12 +331,14 @@ impl HostSkillsService {
input: &HostSkillsLoadInput,
roots: Vec<HostSkillRoot>,
skill_config_rules: &SkillConfigRules,
request_root_snapshots: Option<&RequestSkillRootSnapshots>,
) -> SkillLoadOutcome {
let outcome = load_and_merge_host_skill_roots(
let outcome = load_and_merge_host_skill_roots_with_request_snapshots(
roots,
&self.root_scan_slots,
self.restriction_product,
input.plugin_skill_snapshots.as_ref(),
request_root_snapshots,
)
.await;
let disabled_paths = skill_config_rules.resolve_disabled_paths(
@@ -397,7 +445,7 @@ struct ConfigSkillsCacheKey {
}
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct ConfigSkillRootCacheKey {
pub(crate) struct ConfigSkillRootCacheKey {
path: AbsolutePathBuf,
scope_rank: u8,
plugin_identity: Option<PluginIdentity>,
@@ -450,24 +498,7 @@ fn config_skills_cache_key(
plugin_skill_snapshots: Option<&SkillRootSnapshots<PluginSkillRoot>>,
) -> ConfigSkillsCacheKey {
ConfigSkillsCacheKey {
roots: roots
.iter()
.map(|root| {
let scope_rank = match root.scope {
SkillScope::Repo => 0,
SkillScope::User => 1,
SkillScope::System => 2,
SkillScope::Admin => 3,
};
ConfigSkillRootCacheKey {
path: root.path.clone(),
scope_rank,
plugin_identity: root.plugin_identity().cloned(),
plugin_namespace: root.plugin_namespace().map(str::to_string),
file_system: FileSystemIdentity(Arc::downgrade(&root.file_system)),
}
})
.collect(),
roots: roots.iter().map(config_skill_root_cache_key).collect(),
skill_config_rules: skill_config_rules.clone(),
plugin_skill_snapshots: plugin_skill_snapshots
.filter(|_| roots.iter().any(|root| root.plugin_identity().is_some()))
@@ -475,6 +506,22 @@ fn config_skills_cache_key(
}
}
pub(crate) fn config_skill_root_cache_key(root: &HostSkillRoot) -> ConfigSkillRootCacheKey {
let scope_rank = match root.scope {
SkillScope::Repo => 0,
SkillScope::User => 1,
SkillScope::System => 2,
SkillScope::Admin => 3,
};
ConfigSkillRootCacheKey {
path: root.path.clone(),
scope_rank,
plugin_identity: root.plugin_identity().cloned(),
plugin_namespace: root.plugin_namespace().map(str::to_string),
file_system: FileSystemIdentity(Arc::downgrade(&root.file_system)),
}
}
#[cfg(test)]
#[path = "host_service_tests.rs"]
mod tests;
+81 -1
View File
@@ -305,6 +305,76 @@ async fn snapshot_for_config_preserves_host_precedence_for_symlinked_plugin_root
);
}
#[tokio::test]
async fn skills_list_snapshots_share_host_roots_only_within_one_request() {
let codex_home = tempfile::tempdir().expect("tempdir");
let first_cwd = tempfile::tempdir().expect("tempdir");
let second_cwd = tempfile::tempdir().expect("tempdir");
let config_layer_stack = config_stack(&codex_home, "");
let disabled_config_layer_stack = config_stack(
&codex_home,
&name_toggle_config("first-skill", /*enabled*/ false),
);
let skills_service = HostSkillsService::new(
codex_home.path().abs(),
/*bundled_skills_enabled*/ true,
);
let request = skills_service.for_request();
let input = |cwd: &TempDir, config_layer_stack| {
HostSkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
config_layer_stack,
/*bundled_skills_enabled*/ true,
)
};
write_user_skill(&codex_home, "first", "first-skill", "first skill");
let first = request
.snapshot_for_cwd(
&input(&first_cwd, config_layer_stack.clone()),
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
)
.await;
write_user_skill(&codex_home, "second", "second-skill", "second skill");
let second = request
.snapshot_for_cwd(
&input(&second_cwd, disabled_config_layer_stack),
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
)
.await;
assert_eq!(second.outcome().skills, first.outcome().skills);
let first_skill_path = first
.outcome()
.skills
.iter()
.find(|skill| skill.name == "first-skill")
.expect("first skill should be discovered")
.path_to_skills_md
.clone();
assert!(!first.outcome().disabled_paths.contains(&first_skill_path));
assert!(second.outcome().disabled_paths.contains(&first_skill_path));
let third = skills_service
.for_request()
.snapshot_for_cwd(
&input(&first_cwd, config_layer_stack),
/*force_reload*/ true,
Some(Arc::clone(&LOCAL_FS)),
)
.await;
assert!(
third
.outcome()
.skills
.iter()
.any(|skill| skill.name == "second-skill")
);
}
#[tokio::test]
async fn skills_for_config_refreshes_cache_when_remote_plugin_id_changes() {
let codex_home = tempfile::tempdir().expect("tempdir");
@@ -344,6 +414,7 @@ async fn skills_for_config_refreshes_cache_when_remote_plugin_id_changes() {
.clone()
.with_plugin_skill_snapshots(/*plugin_skill_snapshots*/ None);
let listing_snapshot = skills_service
.for_request()
.snapshot_for_cwd(
&listing_input,
/*force_reload*/ false,
@@ -405,6 +476,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
bundled_skills_enabled_from_stack(&config_layer_stack),
);
let empty_snapshot = skills_service
.for_request()
.snapshot_for_cwd(
&skills_input,
/*force_reload*/ false,
@@ -430,6 +502,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
skills_service.set_extra_roots(vec![extra_skills_root.abs()]);
let runtime_snapshot = skills_service
.for_request()
.snapshot_for_cwd(
&skills_input,
/*force_reload*/ false,
@@ -446,6 +519,7 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
skills_service.set_extra_roots(vec![extra_root.path().join("missing-skills").abs()]);
let replaced_snapshot = skills_service
.for_request()
.snapshot_for_cwd(
&skills_input,
/*force_reload*/ false,
@@ -603,6 +677,7 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() {
);
let snapshot = skills_service
.for_request()
.snapshot_for_cwd(
&skills_input,
/*force_reload*/ true,
@@ -673,6 +748,7 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() {
);
let snapshot = skills_service
.for_request()
.snapshot_for_cwd(&skills_input, /*force_reload*/ true, /*fs*/ None)
.await;
let outcome = snapshot.outcome();
@@ -742,9 +818,10 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() {
let config_input = base_input
.clone()
.with_plugin_skill_snapshots(Some(test_plugin_skill_snapshots()));
let request = skills_service.for_request();
let (config_snapshot, snapshot_a) = tokio::join!(
skills_service.snapshot_for_config(&config_input, Some(Arc::clone(&LOCAL_FS))),
skills_service.snapshot_for_cwd(
request.snapshot_for_cwd(
&base_input,
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
@@ -765,6 +842,7 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() {
write_user_skill(&codex_home, "late", "late-skill", "added after cache");
let snapshot_b = skills_service
.for_request()
.snapshot_for_cwd(
&base_input,
/*force_reload*/ false,
@@ -780,6 +858,7 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() {
);
let snapshot_reloaded = skills_service
.for_request()
.snapshot_for_cwd(
&base_input,
/*force_reload*/ true,
@@ -825,6 +904,7 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill()
);
let parent_snapshot = skills_service
.for_request()
.snapshot_for_cwd(
&parent_input,
/*force_reload*/ true,
+1
View File
@@ -32,6 +32,7 @@ pub use host_outcome::SkillLoadOutcome;
pub use host_prompt::HostSkillPrompts;
pub use host_prompt::InjectedHostSkillPrompts;
pub use host_service::HostSkillsLoadInput;
pub use host_service::HostSkillsRequest;
pub use host_service::HostSkillsService;
pub use host_service::bundled_skills_enabled_from_stack;
pub use host_snapshot::HostSkillsSnapshot;
+75 -34
View File
@@ -11,6 +11,7 @@ use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_path_uri::PathUri;
use codex_utils_plugins::PluginSkillRoot;
use codex_utils_plugins::migrated_command_skills_root;
use futures::FutureExt;
use futures::StreamExt;
use tokio::sync::OnceCell;
use tokio::sync::Semaphore;
@@ -20,12 +21,31 @@ use super::MAX_CONCURRENT_ROOT_SCANS;
use super::host::HostSkillRootSnapshot;
use super::load_host_skill_root;
use crate::SkillLoadOutcome;
use crate::host_service::RequestSkillRootSnapshots;
use crate::host_service::config_skill_root_cache_key;
pub(crate) async fn load_and_merge_host_skill_roots(
roots: Vec<HostSkillRoot>,
root_scan_slots: &Semaphore,
restriction_product: Option<Product>,
plugin_skill_snapshots: Option<&SkillRootSnapshots<PluginSkillRoot>>,
) -> SkillLoadOutcome {
load_and_merge_host_skill_roots_with_request_snapshots(
roots,
root_scan_slots,
restriction_product,
plugin_skill_snapshots,
/*request_root_snapshots*/ None,
)
.await
}
pub(crate) async fn load_and_merge_host_skill_roots_with_request_snapshots(
roots: Vec<HostSkillRoot>,
root_scan_slots: &Semaphore,
restriction_product: Option<Product>,
plugin_skill_snapshots: Option<&SkillRootSnapshots<PluginSkillRoot>>,
request_root_snapshots: Option<&RequestSkillRootSnapshots>,
) -> SkillLoadOutcome {
let migrated_command_roots_by_plugin = roots
.iter()
@@ -42,42 +62,63 @@ pub(crate) async fn load_and_merge_host_skill_roots(
.cloned();
let plugin_skill_root = root.plugin_skill_root();
let file_system = Arc::clone(&root.file_system);
let _root_scan_slot = root_scan_slots
.acquire()
.await
.unwrap_or_else(|_| unreachable!());
let cached_snapshot = plugin_skill_root
.as_ref()
.and_then(|plugin_skill_root| plugin_skill_snapshots?.get(plugin_skill_root));
let snapshot = match cached_snapshot {
Some(snapshot) => HostSkillRootSnapshot {
root: snapshot.root,
skills: snapshot.skills,
skill_discovery_path_by_path: snapshot.skill_discovery_path_by_path,
errors: snapshot.errors,
file_system: Arc::clone(&file_system),
is_agent_plugin: snapshot.is_agent_plugin,
},
None => {
let snapshot = load_host_skill_root(root).await;
if let Some(plugin_skill_snapshots) = plugin_skill_snapshots
&& let Some(plugin_skill_root) = plugin_skill_root
{
plugin_skill_snapshots.insert(
plugin_skill_root,
LoadedSkillRoot {
root: snapshot.root.clone(),
skills: snapshot.skills.clone(),
skill_discovery_path_by_path: Arc::clone(
&snapshot.skill_discovery_path_by_path,
),
errors: snapshot.errors.clone(),
is_agent_plugin: snapshot.is_agent_plugin,
},
);
let request_snapshot = if plugin_skill_root.is_none() {
request_root_snapshots.map(|snapshots| {
Arc::clone(
snapshots
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner)
.entry(config_skill_root_cache_key(&root))
.or_insert_with(|| Arc::new(OnceCell::new())),
)
})
} else {
None
};
let load = async {
let _root_scan_slot = root_scan_slots
.acquire()
.await
.unwrap_or_else(|_| unreachable!());
let cached_snapshot = plugin_skill_root
.as_ref()
.and_then(|plugin_skill_root| plugin_skill_snapshots?.get(plugin_skill_root));
match cached_snapshot {
Some(snapshot) => HostSkillRootSnapshot {
root: snapshot.root,
skills: snapshot.skills,
skill_discovery_path_by_path: snapshot.skill_discovery_path_by_path,
errors: snapshot.errors,
file_system: Arc::clone(&file_system),
is_agent_plugin: snapshot.is_agent_plugin,
},
None => {
let snapshot = load_host_skill_root(root).await;
if let Some(plugin_skill_snapshots) = plugin_skill_snapshots
&& let Some(plugin_skill_root) = plugin_skill_root
{
plugin_skill_snapshots.insert(
plugin_skill_root,
LoadedSkillRoot {
root: snapshot.root.clone(),
skills: snapshot.skills.clone(),
skill_discovery_path_by_path: Arc::clone(
&snapshot.skill_discovery_path_by_path,
),
errors: snapshot.errors.clone(),
is_agent_plugin: snapshot.is_agent_plugin,
},
);
}
snapshot
}
snapshot
}
}
.boxed();
let snapshot = if let Some(snapshot) = request_snapshot {
snapshot.get_or_init(|| load).await.clone()
} else {
load.await
};
let migrated_command_root = match (plugin_root.as_ref(), migrated_command_root) {
(Some(plugin_root), Some(migrated_command_root)) => Some(
+2
View File
@@ -10,8 +10,10 @@ 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::HostSkillRootSnapshot;
pub(crate) use host::load_host_skill_root;
pub(crate) use host_merge::load_and_merge_host_skill_roots;
pub(crate) use host_merge::load_and_merge_host_skill_roots_with_request_snapshots;
pub(crate) const MAX_CONCURRENT_ROOT_SCANS: usize = 8;
pub(super) const SKILLS_FILENAME: &str = "SKILL.md";