mirror of
https://github.com/openai/codex.git
synced 2026-11-13 06:45:07 +00:00
Honor per-directory bundled skill settings in skills/list (#37979)
## Why `skills/list` can load multiple working directories in one request, but bundled skill discovery did not account for different effective configurations across those directories. ## What changed - Resolve `skills.bundled.enabled` from each load input's effective config layer stack inside the host skills service. - Include or exclude system-scoped skills for each working directory based on that resolved setting. - Keep bundled skills enabled by default when the setting is absent or invalid. ## Testing - Add a multi-directory `skills/list` test with bundled skills disabled in one project and enabled in another. - Add config-layer tests for defaults, overrides, and malformed bundled-skill configuration. GitOrigin-RevId: e4e97dc7d0a374ac71d0aa11e55a9e337f661717
This commit is contained in:
1 parent
0ca439900e
commit
7d486ffa94
14 files changed
+135
-91
No files matched your search
@@ -7,6 +7,7 @@ use std::sync::Weak;
|
||||
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::SkillConfigRules;
|
||||
use codex_config::bundled_skills_enabled_from_stack;
|
||||
use codex_config::skill_config_rules_from_stack;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
@@ -19,9 +20,7 @@ use tokio::sync::OnceCell;
|
||||
use tokio::sync::Semaphore;
|
||||
use tracing::info;
|
||||
use tracing::instrument;
|
||||
use tracing::warn;
|
||||
|
||||
use codex_config::SkillsConfig;
|
||||
use codex_skills::LoadedSkills;
|
||||
use codex_skills::SkillLoadFuture;
|
||||
use codex_skills::SkillRootLoadRequest;
|
||||
@@ -43,7 +42,6 @@ pub struct HostSkillsLoadInput {
|
||||
cwd: AbsolutePathBuf,
|
||||
effective_skill_roots: Vec<PluginSkillRoot>,
|
||||
config_layer_stack: ConfigLayerStack,
|
||||
bundled_skills_enabled: bool,
|
||||
plugin_skill_snapshots: Option<SkillRootSnapshots<PluginSkillRoot>>,
|
||||
}
|
||||
|
||||
@@ -52,13 +50,11 @@ impl HostSkillsLoadInput {
|
||||
cwd: AbsolutePathBuf,
|
||||
effective_skill_roots: Vec<PluginSkillRoot>,
|
||||
config_layer_stack: ConfigLayerStack,
|
||||
bundled_skills_enabled: bool,
|
||||
) -> Self {
|
||||
Self {
|
||||
cwd,
|
||||
effective_skill_roots,
|
||||
config_layer_stack,
|
||||
bundled_skills_enabled,
|
||||
plugin_skill_snapshots: None,
|
||||
}
|
||||
}
|
||||
@@ -219,7 +215,8 @@ impl HostSkillsService {
|
||||
input: &HostSkillsLoadInput,
|
||||
fs: Option<Arc<dyn ExecutorFileSystem>>,
|
||||
) -> Vec<HostSkillRoot> {
|
||||
if input.bundled_skills_enabled {
|
||||
let bundled_skills_enabled = bundled_skills_enabled_from_stack(&input.config_layer_stack);
|
||||
if bundled_skills_enabled {
|
||||
self.ensure_system_skills_installed();
|
||||
}
|
||||
let mut roots = resolve_skill_roots(
|
||||
@@ -230,7 +227,7 @@ impl HostSkillsService {
|
||||
self.extra_roots(),
|
||||
)
|
||||
.await;
|
||||
if !input.bundled_skills_enabled {
|
||||
if !bundled_skills_enabled {
|
||||
roots.retain(|root| root.scope != SkillScope::System);
|
||||
}
|
||||
roots
|
||||
@@ -487,28 +484,6 @@ impl Hash for FileSystemIdentity {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn bundled_skills_enabled_from_stack(
|
||||
config_layer_stack: &codex_config::ConfigLayerStack,
|
||||
) -> bool {
|
||||
let effective_config = config_layer_stack.effective_config();
|
||||
let Some(skills_value) = effective_config
|
||||
.as_table()
|
||||
.and_then(|table| table.get("skills"))
|
||||
else {
|
||||
return true;
|
||||
};
|
||||
|
||||
let skills: SkillsConfig = match skills_value.clone().try_into() {
|
||||
Ok(skills) => skills,
|
||||
Err(err) => {
|
||||
warn!("invalid skills config: {err}");
|
||||
return true;
|
||||
}
|
||||
};
|
||||
|
||||
skills.bundled.unwrap_or_default().enabled
|
||||
}
|
||||
|
||||
fn config_skills_cache_key(
|
||||
roots: &[HostSkillRoot],
|
||||
skill_config_rules: &SkillConfigRules,
|
||||
|
||||
@@ -172,7 +172,6 @@ async fn skills_for_config_with_stack(
|
||||
cwd.path().abs(),
|
||||
effective_skill_roots.to_vec(),
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(config_layer_stack),
|
||||
);
|
||||
skills_service
|
||||
.snapshot_for_config(&skills_input, Some(Arc::clone(&LOCAL_FS)))
|
||||
@@ -226,7 +225,6 @@ async fn watchable_skill_root_paths_exclude_plugin_and_system_roots() {
|
||||
cwd.path().abs(),
|
||||
vec![plugin_skill_root.clone()],
|
||||
config_layer_stack,
|
||||
/*bundled_skills_enabled*/ true,
|
||||
);
|
||||
let skills_service = HostSkillsService::new(
|
||||
codex_home.path().abs(),
|
||||
@@ -262,7 +260,6 @@ async fn snapshot_for_config_merges_extension_host_and_legacy_plugin_roots() {
|
||||
cwd.path().abs(),
|
||||
vec![plugin_skill_root],
|
||||
config_layer_stack,
|
||||
/*bundled_skills_enabled*/ false,
|
||||
);
|
||||
let skills_service = HostSkillsService::new(
|
||||
codex_home.path().abs(),
|
||||
@@ -356,12 +353,7 @@ async fn skills_list_snapshots_share_host_roots_only_within_one_request() {
|
||||
);
|
||||
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,
|
||||
)
|
||||
HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack)
|
||||
};
|
||||
|
||||
write_user_skill(&codex_home, "first", "first-skill", "first skill");
|
||||
@@ -434,7 +426,6 @@ async fn skills_for_config_refreshes_cache_when_remote_plugin_id_changes() {
|
||||
cwd.path().abs(),
|
||||
vec![plugin_skill_root.clone()],
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&config_layer_stack),
|
||||
)
|
||||
.with_plugin_skill_snapshots(Some(test_plugin_skill_snapshots()));
|
||||
let plugin_snapshot = skills_service
|
||||
@@ -504,12 +495,8 @@ async fn set_extra_roots_replaces_runtime_roots_and_clears_cache() {
|
||||
/*bundled_skills_enabled*/ true,
|
||||
);
|
||||
|
||||
let skills_input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
Vec::new(),
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&config_layer_stack),
|
||||
);
|
||||
let skills_input =
|
||||
HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone());
|
||||
let empty_snapshot = skills_service
|
||||
.for_request()
|
||||
.snapshot_for_cwd(
|
||||
@@ -699,12 +686,8 @@ async fn skills_for_cwd_loads_repo_and_user_roots_with_local_fs() {
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("valid config layer stack");
|
||||
let skills_input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
Vec::new(),
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&config_layer_stack),
|
||||
);
|
||||
let skills_input =
|
||||
HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone());
|
||||
let skills_service = HostSkillsService::new(
|
||||
codex_home.path().abs(),
|
||||
/*bundled_skills_enabled*/ true,
|
||||
@@ -770,12 +753,8 @@ async fn skills_for_cwd_without_fs_skips_repo_roots() {
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("valid config layer stack");
|
||||
let skills_input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
Vec::new(),
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&config_layer_stack),
|
||||
);
|
||||
let skills_input =
|
||||
HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone());
|
||||
let skills_service = HostSkillsService::new(
|
||||
codex_home.path().abs(),
|
||||
/*bundled_skills_enabled*/ true,
|
||||
@@ -843,12 +822,8 @@ async fn skills_for_cwd_uses_cached_result_until_force_reload() {
|
||||
codex_home.path().abs(),
|
||||
/*bundled_skills_enabled*/ true,
|
||||
);
|
||||
let base_input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
Vec::new(),
|
||||
config_layer_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&config_layer_stack),
|
||||
);
|
||||
let base_input =
|
||||
HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), config_layer_stack.clone());
|
||||
let config_input = base_input
|
||||
.clone()
|
||||
.with_plugin_skill_snapshots(Some(test_plugin_skill_snapshots()));
|
||||
@@ -929,12 +904,7 @@ async fn skills_for_config_ignores_cwd_cache_when_session_flags_reenable_skill()
|
||||
codex_home.path().abs(),
|
||||
/*bundled_skills_enabled*/ true,
|
||||
);
|
||||
let parent_input = HostSkillsLoadInput::new(
|
||||
cwd.path().abs(),
|
||||
Vec::new(),
|
||||
parent_stack.clone(),
|
||||
bundled_skills_enabled_from_stack(&parent_stack),
|
||||
);
|
||||
let parent_input = HostSkillsLoadInput::new(cwd.path().abs(), Vec::new(), parent_stack.clone());
|
||||
|
||||
let parent_snapshot = skills_service
|
||||
.for_request()
|
||||
|
||||
@@ -34,7 +34,6 @@ 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;
|
||||
pub use provider::ExecutorSkillProvider;
|
||||
pub use provider::HostSkillProvider;
|
||||
|
||||
Reference in new issue
Block a user