openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-09-06 15:29:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

Avoid skill filesystem scans on cache hits

Browse Source
This commit is contained in:
Xin Lin
2026-06-18 19:10:56 -07:00
parent 38c96866f0
commit 2cae3a1cac
2 changed files with 279 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

124
codex-rs/core-skills/src/service.rs
View File

@@ -1,9 +1,13 @@
use std::collections::HashMap;
use std::collections::HashSet;
use std::hash::Hash;
use std::hash::Hasher;
use std::sync::Arc;
use std::sync::RwLock;
use codex_app_server_protocol::ConfigLayerSource;
use codex_config::ConfigLayerStack;
use codex_config::ConfigLayerStackOrdering;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
@@ -128,13 +132,16 @@ impl SkillsService {
input: &SkillsLoadInput,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> HostSkillsSnapshot {
let roots = self.skill_roots_for_config(input, fs).await;
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let cache_key = config_skills_cache_key(&roots, &skill_config_rules);
let extra_roots = self.extra_roots();
let cache_key = config_skills_cache_key(input, &extra_roots, fs.as_ref());
if let Some(snapshot) = self.cached_snapshot_for_config(&cache_key) {
return snapshot;
}
let roots = self
.skill_roots_for_config_with_extra_roots(input, fs, extra_roots)
.await;
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let snapshot = HostSkillsSnapshot::new(Arc::new(
self.build_skill_outcome(input, roots, &skill_config_rules)
.await,
@@ -151,13 +158,23 @@ impl SkillsService {
&self,
input: &SkillsLoadInput,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> Vec<SkillRoot> {
self.skill_roots_for_config_with_extra_roots(input, fs, self.extra_roots())
.await
}
async fn skill_roots_for_config_with_extra_roots(
&self,
input: &SkillsLoadInput,
fs: Option<Arc<dyn ExecutorFileSystem>>,
extra_roots: Vec<AbsolutePathBuf>,
) -> Vec<SkillRoot> {
let mut roots = skill_roots(
fs,
&input.config_layer_stack,
&input.cwd,
input.effective_skill_roots.clone(),
self.extra_roots(),
extra_roots,
)
.await;
if !input.bundled_skills_enabled {
@@ -268,10 +285,42 @@ impl SkillsService {
}
}
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
/// Skill-relevant inputs that can be compared before root discovery touches the filesystem.
#[derive(Clone, PartialEq, Eq, Hash)]
struct ConfigSkillsCacheKey {
roots: Vec<(AbsolutePathBuf, u8, Option<String>, Option<String>)>,
skill_config_rules: SkillConfigRules,
cwd: AbsolutePathBuf,
config_layers: Vec<ConfigLayerSkillsCacheKey>,
effective_skill_roots: Vec<PluginSkillRoot>,
bundled_skills_enabled: bool,
extra_roots: Vec<AbsolutePathBuf>,
file_system: Option<ExecutorFileSystemCacheKey>,
}
#[derive(Clone, PartialEq, Eq, Hash)]
struct ConfigLayerSkillsCacheKey {
source: std::mem::Discriminant<ConfigLayerSource>,
config_folder: Option<AbsolutePathBuf>,
disabled: bool,
skills_config: Option<String>,
project_root_markers: Option<String>,
}
// Snapshots retain filesystem-bound skill paths, so cache entries must distinguish instances.
#[derive(Clone)]
struct ExecutorFileSystemCacheKey(Arc<dyn ExecutorFileSystem>);
impl PartialEq for ExecutorFileSystemCacheKey {
fn eq(&self, other: &Self) -> bool {
Arc::ptr_eq(&self.0, &other.0)
}
}
impl Eq for ExecutorFileSystemCacheKey {}
impl Hash for ExecutorFileSystemCacheKey {
fn hash<H: Hasher>(&self, state: &mut H) {
std::ptr::hash(Arc::as_ptr(&self.0), state);
}
}
pub fn bundled_skills_enabled_from_stack(
@@ -297,28 +346,55 @@ pub fn bundled_skills_enabled_from_stack(
}
fn config_skills_cache_key(
roots: &[SkillRoot],
skill_config_rules: &SkillConfigRules,
input: &SkillsLoadInput,
extra_roots: &[AbsolutePathBuf],
fs: Option<&Arc<dyn ExecutorFileSystem>>,
) -> 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,
cwd: input.cwd.clone(),
config_layers: input
.config_layer_stack
.get_layers(
ConfigLayerStackOrdering::LowestPrecedenceFirst,
/*include_disabled*/ true,
)
.into_iter()
.filter_map(|layer| {
let config_folder = layer.config_folder();
let skills_config = if matches!(
layer.name,
ConfigLayerSource::User { .. } | ConfigLayerSource::SessionFlags
) {
layer.config.get("skills").map(ToString::to_string)
} else {
None
};
(
root.path.clone(),
scope_rank,
root.plugin_id.clone(),
root.plugin_namespace.clone(),
)
let project_root_markers = if !layer.is_disabled()
&& !matches!(layer.name, ConfigLayerSource::Project { .. })
{
layer
.config
.get("project_root_markers")
.map(ToString::to_string)
} else {
None
};
(config_folder.is_some()
|| skills_config.is_some()
|| project_root_markers.is_some())
.then(|| ConfigLayerSkillsCacheKey {
source: std::mem::discriminant(&layer.name),
config_folder,
disabled: layer.is_disabled(),
skills_config,
project_root_markers,
})
})
.collect(),
skill_config_rules: skill_config_rules.clone(),
effective_skill_roots: input.effective_skill_roots.clone(),
bundled_skills_enabled: input.bundled_skills_enabled,
extra_roots: extra_roots.to_vec(),
file_system: fs.cloned().map(ExecutorFileSystemCacheKey),
}
}

179
codex-rs/core-skills/src/service_tests.rs
View File

@@ -7,10 +7,20 @@ use codex_config::CONFIG_TOML_FILE;
use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerStack;
use codex_config::ConfigRequirementsToml;
use codex_exec_server::CopyOptions;
use codex_exec_server::CreateDirectoryOptions;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::ExecutorFileSystemFuture;
use codex_exec_server::FileMetadata;
use codex_exec_server::FileSystemReadStream;
use codex_exec_server::FileSystemSandboxContext;
use codex_exec_server::LOCAL_FS;
use codex_exec_server::ReadDirectoryEntry;
use codex_exec_server::RemoveOptions;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
use codex_utils_absolute_path::test_support::PathExt;
use codex_utils_path_uri::PathUri;
use codex_utils_plugins::PluginSkillRoot;
use pretty_assertions::assert_eq;
use std::collections::HashSet;
@@ -18,8 +28,121 @@ use std::fs;
use std::path::Path;
use std::path::PathBuf;
use std::sync::Arc;
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;
use tempfile::TempDir;
struct CountingFileSystem {
delegate: Arc<dyn ExecutorFileSystem>,
operation_count: AtomicUsize,
}
impl CountingFileSystem {
fn new(delegate: Arc<dyn ExecutorFileSystem>) -> Self {
Self {
delegate,
operation_count: AtomicUsize::new(0),
}
}
fn operation_count(&self) -> usize {
self.operation_count.load(Ordering::Relaxed)
}
fn record_operation(&self) {
self.operation_count.fetch_add(1, Ordering::Relaxed);
}
}
impl ExecutorFileSystem for CountingFileSystem {
fn canonicalize<'a>(
&'a self,
path: &'a PathUri,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, PathUri> {
self.record_operation();
self.delegate.canonicalize(path, sandbox)
}
fn read_file<'a>(
&'a self,
path: &'a PathUri,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, Vec<u8>> {
self.record_operation();
self.delegate.read_file(path, sandbox)
}
fn read_file_stream<'a>(
&'a self,
path: &'a PathUri,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> {
self.record_operation();
self.delegate.read_file_stream(path, sandbox)
}
fn write_file<'a>(
&'a self,
path: &'a PathUri,
contents: Vec<u8>,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, ()> {
self.record_operation();
self.delegate.write_file(path, contents, sandbox)
}
fn create_directory<'a>(
&'a self,
path: &'a PathUri,
options: CreateDirectoryOptions,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, ()> {
self.record_operation();
self.delegate.create_directory(path, options, sandbox)
}
fn get_metadata<'a>(
&'a self,
path: &'a PathUri,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, FileMetadata> {
self.record_operation();
self.delegate.get_metadata(path, sandbox)
}
fn read_directory<'a>(
&'a self,
path: &'a PathUri,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, Vec<ReadDirectoryEntry>> {
self.record_operation();
self.delegate.read_directory(path, sandbox)
}
fn remove<'a>(
&'a self,
path: &'a PathUri,
options: RemoveOptions,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, ()> {
self.record_operation();
self.delegate.remove(path, options, sandbox)
}
fn copy<'a>(
&'a self,
source_path: &'a PathUri,
destination_path: &'a PathUri,
options: CopyOptions,
sandbox: Option<&'a FileSystemSandboxContext>,
) -> ExecutorFileSystemFuture<'a, ()> {
self.record_operation();
self.delegate
.copy(source_path, destination_path, options, sandbox)
}
}
fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &str) {
let skill_dir = codex_home.path().join("skills").join(dir);
fs::create_dir_all(&skill_dir).unwrap();
@@ -228,6 +351,62 @@ async fn skills_for_config_reuses_cache_for_same_effective_config() {
assert_eq!(outcome2.skills, outcome1.skills);
}
#[tokio::test]
async fn skills_for_config_cache_hit_avoids_environment_filesystem_operations() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
let config_layer_stack = config_stack(&codex_home, "");
let skills_service = SkillsService::new(
codex_home.path().abs(),
/*bundled_skills_enabled*/ true,
);
let skills_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
config_layer_stack,
/*bundled_skills_enabled*/ true,
);
let counting_file_system = Arc::new(CountingFileSystem::new(Arc::clone(&LOCAL_FS)));
let file_system: Arc<dyn ExecutorFileSystem> = counting_file_system.clone();
skills_service
.snapshot_for_config(&skills_input, Some(Arc::clone(&file_system)))
.await;
let operations_after_cache_miss = counting_file_system.operation_count();
assert!(operations_after_cache_miss > 0);
skills_service
.snapshot_for_config(&skills_input, Some(Arc::clone(&file_system)))
.await;
assert_eq!(
counting_file_system.operation_count(),
operations_after_cache_miss
);
let unrelated_session_flag_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
config_stack_with_session_flags(&codex_home, "", "model = 'gpt-5'"),
/*bundled_skills_enabled*/ true,
);
skills_service
.snapshot_for_config(
&unrelated_session_flag_input,
Some(Arc::clone(&file_system)),
)
.await;
assert_eq!(
counting_file_system.operation_count(),
operations_after_cache_miss
);
skills_service.clear_cache();
skills_service
.snapshot_for_config(&skills_input, Some(file_system))
.await;
assert!(counting_file_system.operation_count() > operations_after_cache_miss);
}
#[tokio::test]
async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
let codex_home = tempfile::tempdir().expect("tempdir");