mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat: support disabling bundled system skills (#13792)
Support disable bundled system skills with a config: [skills.bundled] enabled = false
This commit is contained in:
committed by
GitHub
Unverified
parent
710682598d
commit
0c33af7746
@@ -189,10 +189,9 @@ impl MessageProcessor {
|
||||
outgoing: outgoing.clone(),
|
||||
}));
|
||||
let thread_manager = Arc::new(ThreadManager::new(
|
||||
config.codex_home.clone(),
|
||||
config.as_ref(),
|
||||
auth_manager.clone(),
|
||||
session_source,
|
||||
config.model_catalog.clone(),
|
||||
CollaborationModesConfig {
|
||||
default_mode_request_user_input: config
|
||||
.features
|
||||
|
||||
@@ -284,6 +284,16 @@
|
||||
}
|
||||
]
|
||||
},
|
||||
"BundledSkillsConfig": {
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"enabled": {
|
||||
"default": true,
|
||||
"type": "boolean"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"ConfigProfile": {
|
||||
"additionalProperties": false,
|
||||
"description": "Collection of common configuration options that a user can define as a unit in `config.toml`.",
|
||||
@@ -1463,6 +1473,9 @@
|
||||
"SkillsConfig": {
|
||||
"additionalProperties": false,
|
||||
"properties": {
|
||||
"bundled": {
|
||||
"$ref": "#/definitions/BundledSkillsConfig"
|
||||
},
|
||||
"config": {
|
||||
"items": {
|
||||
"$ref": "#/definitions/SkillConfig"
|
||||
|
||||
@@ -788,7 +788,7 @@ enabled = false
|
||||
.expect("custom role should apply");
|
||||
|
||||
let plugins_manager = Arc::new(PluginsManager::new(home.path().to_path_buf()));
|
||||
let skills_manager = SkillsManager::new(home.path().to_path_buf(), plugins_manager);
|
||||
let skills_manager = SkillsManager::new(home.path().to_path_buf(), plugins_manager, true);
|
||||
let outcome = skills_manager.skills_for_config(&config);
|
||||
let skill = outcome
|
||||
.skills
|
||||
|
||||
@@ -2058,6 +2058,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() {
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
config.codex_home.clone(),
|
||||
Arc::clone(&plugins_manager),
|
||||
true,
|
||||
));
|
||||
let result = Session::new(
|
||||
session_configuration,
|
||||
@@ -2160,6 +2161,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
config.codex_home.clone(),
|
||||
Arc::clone(&plugins_manager),
|
||||
true,
|
||||
));
|
||||
let network_approval = Arc::new(NetworkApprovalService::default());
|
||||
|
||||
@@ -2715,6 +2717,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
config.codex_home.clone(),
|
||||
Arc::clone(&plugins_manager),
|
||||
true,
|
||||
));
|
||||
let network_approval = Arc::new(NetworkApprovalService::default());
|
||||
|
||||
|
||||
@@ -282,6 +282,7 @@ async fn guardian_subagent_does_not_inherit_parent_exec_policy_rules() {
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
config.codex_home.clone(),
|
||||
Arc::clone(&plugins_manager),
|
||||
true,
|
||||
));
|
||||
let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager)));
|
||||
let file_watcher = Arc::new(FileWatcher::noop());
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use crate::config::edit::ConfigEdit;
|
||||
use crate::config::edit::ConfigEditsBuilder;
|
||||
use crate::config::edit::apply_blocking;
|
||||
use crate::config::types::BundledSkillsConfig;
|
||||
use crate::config::types::FeedbackConfigToml;
|
||||
use crate::config::types::HistoryPersistence;
|
||||
use crate::config::types::McpServerTransportConfig;
|
||||
@@ -155,6 +156,25 @@ consolidation_model = "gpt-5"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_bundled_skills_config() {
|
||||
let cfg: ConfigToml = toml::from_str(
|
||||
r#"
|
||||
[skills.bundled]
|
||||
enabled = false
|
||||
"#,
|
||||
)
|
||||
.expect("TOML deserialization should succeed");
|
||||
|
||||
assert_eq!(
|
||||
cfg.skills,
|
||||
Some(SkillsConfig {
|
||||
bundled: Some(BundledSkillsConfig { enabled: false }),
|
||||
config: Vec::new(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn config_toml_deserializes_model_availability_nux() {
|
||||
let toml = r#"
|
||||
|
||||
@@ -2681,6 +2681,10 @@ impl Config {
|
||||
.network
|
||||
.is_some()
|
||||
}
|
||||
|
||||
pub fn bundled_skills_enabled(&self) -> bool {
|
||||
crate::skills::manager::bundled_skills_enabled_from_stack(&self.config_layer_stack)
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn uses_deprecated_instructions_file(config_layer_stack: &ConfigLayerStack) -> bool {
|
||||
|
||||
@@ -785,10 +785,26 @@ pub struct PluginConfig {
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct SkillsConfig {
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub bundled: Option<BundledSkillsConfig>,
|
||||
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub config: Vec<SkillConfig>,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct BundledSkillsConfig {
|
||||
#[serde(default = "default_enabled")]
|
||||
pub enabled: bool,
|
||||
}
|
||||
|
||||
impl Default for BundledSkillsConfig {
|
||||
fn default() -> Self {
|
||||
Self { enabled: true }
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Default, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct SandboxWorkspaceWrite {
|
||||
|
||||
@@ -25,6 +25,7 @@ use crate::skills::loader::SkillRoot;
|
||||
use crate::skills::loader::load_skills_from_roots;
|
||||
use crate::skills::loader::skill_roots;
|
||||
use crate::skills::system::install_system_skills;
|
||||
use crate::skills::system::uninstall_system_skills;
|
||||
|
||||
pub struct SkillsManager {
|
||||
codex_home: PathBuf,
|
||||
@@ -33,16 +34,24 @@ pub struct SkillsManager {
|
||||
}
|
||||
|
||||
impl SkillsManager {
|
||||
pub fn new(codex_home: PathBuf, plugins_manager: Arc<PluginsManager>) -> Self {
|
||||
if let Err(err) = install_system_skills(&codex_home) {
|
||||
tracing::error!("failed to install system skills: {err}");
|
||||
}
|
||||
|
||||
Self {
|
||||
pub fn new(
|
||||
codex_home: PathBuf,
|
||||
plugins_manager: Arc<PluginsManager>,
|
||||
bundled_skills_enabled: bool,
|
||||
) -> Self {
|
||||
let manager = Self {
|
||||
codex_home,
|
||||
plugins_manager,
|
||||
cache_by_cwd: RwLock::new(HashMap::new()),
|
||||
};
|
||||
if !bundled_skills_enabled {
|
||||
// The loader caches bundled skills under `skills/.system`. Clearing that directory is
|
||||
// best-effort cleanup; root selection still enforces the config even if removal fails.
|
||||
uninstall_system_skills(&manager.codex_home);
|
||||
} else if let Err(err) = install_system_skills(&manager.codex_home) {
|
||||
tracing::error!("failed to install system skills: {err}");
|
||||
}
|
||||
manager
|
||||
}
|
||||
|
||||
/// Load skills for an already-constructed [`Config`], avoiding any additional config-layer
|
||||
@@ -66,11 +75,15 @@ impl SkillsManager {
|
||||
|
||||
pub(crate) fn skill_roots_for_config(&self, config: &Config) -> Vec<SkillRoot> {
|
||||
let loaded_plugins = self.plugins_manager.plugins_for_config(config);
|
||||
skill_roots(
|
||||
let mut roots = skill_roots(
|
||||
&config.config_layer_stack,
|
||||
&config.cwd,
|
||||
loaded_plugins.effective_skill_roots(),
|
||||
)
|
||||
);
|
||||
if !config.bundled_skills_enabled() {
|
||||
roots.retain(|root| root.scope != SkillScope::System);
|
||||
}
|
||||
roots
|
||||
}
|
||||
|
||||
pub async fn skills_for_cwd(&self, cwd: &Path, force_reload: bool) -> SkillLoadOutcome {
|
||||
@@ -136,6 +149,9 @@ impl SkillsManager {
|
||||
cwd,
|
||||
loaded_plugins.effective_skill_roots(),
|
||||
);
|
||||
if !bundled_skills_enabled_from_stack(&config_layer_stack) {
|
||||
roots.retain(|root| root.scope != SkillScope::System);
|
||||
}
|
||||
roots.extend(
|
||||
normalized_extra_user_roots
|
||||
.iter()
|
||||
@@ -145,13 +161,7 @@ impl SkillsManager {
|
||||
scope: SkillScope::User,
|
||||
}),
|
||||
);
|
||||
let mut outcome = load_skills_from_roots(roots);
|
||||
if !extra_user_roots.is_empty() {
|
||||
// When extra user roots are provided, skip system skills before caching the result.
|
||||
outcome
|
||||
.skills
|
||||
.retain(|skill| skill.scope != SkillScope::System);
|
||||
}
|
||||
let outcome = load_skills_from_roots(roots);
|
||||
let outcome = finalize_skill_outcome(outcome, &config_layer_stack);
|
||||
let mut cache = match self.cache_by_cwd.write() {
|
||||
Ok(cache) => cache,
|
||||
@@ -179,6 +189,28 @@ impl SkillsManager {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn bundled_skills_enabled_from_stack(
|
||||
config_layer_stack: &crate::config_loader::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 disabled_paths_from_stack(
|
||||
config_layer_stack: &crate::config_loader::ConfigLayerStack,
|
||||
) -> HashSet<PathBuf> {
|
||||
@@ -267,6 +299,24 @@ mod tests {
|
||||
fs::write(skill_dir.join("SKILL.md"), content).unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn new_with_disabled_bundled_skills_removes_stale_cached_system_skills() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let stale_system_skill_dir = codex_home.path().join("skills/.system/stale-skill");
|
||||
fs::create_dir_all(&stale_system_skill_dir).expect("create stale system skill dir");
|
||||
fs::write(stale_system_skill_dir.join("SKILL.md"), "# stale\n")
|
||||
.expect("write stale system skill");
|
||||
|
||||
let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf()));
|
||||
let _skills_manager =
|
||||
SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, false);
|
||||
|
||||
assert!(
|
||||
!codex_home.path().join("skills/.system").exists(),
|
||||
"expected disabling system skills to remove stale cached bundled skills"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_for_config_seeds_cache_by_cwd() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
@@ -283,7 +333,8 @@ mod tests {
|
||||
.expect("defaults for test should always succeed");
|
||||
|
||||
let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf()));
|
||||
let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager);
|
||||
let skills_manager =
|
||||
SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true);
|
||||
|
||||
write_user_skill(&codex_home, "a", "skill-a", "from a");
|
||||
let outcome1 = skills_manager.skills_for_config(&cfg);
|
||||
@@ -317,7 +368,8 @@ mod tests {
|
||||
.expect("defaults for test should always succeed");
|
||||
|
||||
let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf()));
|
||||
let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager);
|
||||
let skills_manager =
|
||||
SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true);
|
||||
let _ = skills_manager.skills_for_config(&config);
|
||||
|
||||
write_user_skill(&extra_root, "x", "extra-skill", "from extra root");
|
||||
@@ -335,6 +387,12 @@ mod tests {
|
||||
.iter()
|
||||
.any(|skill| skill.name == "extra-skill")
|
||||
);
|
||||
assert!(
|
||||
outcome_with_extra
|
||||
.skills
|
||||
.iter()
|
||||
.any(|skill| skill.scope == SkillScope::System)
|
||||
);
|
||||
|
||||
// The cwd-only API returns the current cached entry for this cwd, even when that entry
|
||||
// was produced with extra roots.
|
||||
@@ -343,6 +401,65 @@ mod tests {
|
||||
assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_for_config_excludes_bundled_skills_when_disabled_in_config() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let cwd = tempfile::tempdir().expect("tempdir");
|
||||
let bundled_skill_dir = codex_home.path().join("skills/.system/bundled-skill");
|
||||
fs::create_dir_all(&bundled_skill_dir).expect("create bundled skill dir");
|
||||
fs::write(
|
||||
bundled_skill_dir.join("SKILL.md"),
|
||||
"---\nname: bundled-skill\ndescription: from bundled root\n---\n\n# Body\n",
|
||||
)
|
||||
.expect("write bundled skill");
|
||||
|
||||
fs::write(
|
||||
codex_home.path().join(crate::config::CONFIG_TOML_FILE),
|
||||
"[skills.bundled]\nenabled = false\n",
|
||||
)
|
||||
.expect("write config");
|
||||
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.harness_overrides(ConfigOverrides {
|
||||
cwd: Some(cwd.path().to_path_buf()),
|
||||
..Default::default()
|
||||
})
|
||||
.build()
|
||||
.await
|
||||
.expect("load config");
|
||||
|
||||
let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf()));
|
||||
let skills_manager = SkillsManager::new(
|
||||
codex_home.path().to_path_buf(),
|
||||
plugins_manager,
|
||||
config.bundled_skills_enabled(),
|
||||
);
|
||||
|
||||
// Recreate the cached bundled skill after startup cleanup so this assertion exercises
|
||||
// root selection rather than relying on directory removal succeeding.
|
||||
fs::create_dir_all(&bundled_skill_dir).expect("recreate bundled skill dir");
|
||||
fs::write(
|
||||
bundled_skill_dir.join("SKILL.md"),
|
||||
"---\nname: bundled-skill\ndescription: from bundled root\n---\n\n# Body\n",
|
||||
)
|
||||
.expect("rewrite bundled skill");
|
||||
|
||||
let outcome = skills_manager.skills_for_config(&config);
|
||||
assert!(
|
||||
outcome
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.name != "bundled-skill")
|
||||
);
|
||||
assert!(
|
||||
outcome
|
||||
.skills
|
||||
.iter()
|
||||
.all(|skill| skill.scope != SkillScope::System)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_for_cwd_with_extra_roots_only_refreshes_on_force_reload() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
@@ -361,7 +478,8 @@ mod tests {
|
||||
.expect("defaults for test should always succeed");
|
||||
|
||||
let plugins_manager = Arc::new(PluginsManager::new(codex_home.path().to_path_buf()));
|
||||
let skills_manager = SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager);
|
||||
let skills_manager =
|
||||
SkillsManager::new(codex_home.path().to_path_buf(), plugins_manager, true);
|
||||
let _ = skills_manager.skills_for_config(&config);
|
||||
|
||||
write_user_skill(&extra_root_a, "x", "extra-skill-a", "from extra root a");
|
||||
|
||||
@@ -1,2 +1,9 @@
|
||||
pub(crate) use codex_skills::install_system_skills;
|
||||
pub(crate) use codex_skills::system_cache_root_dir;
|
||||
|
||||
use std::path::Path;
|
||||
|
||||
pub(crate) fn uninstall_system_skills(codex_home: &Path) {
|
||||
let system_skills_dir = system_cache_root_dir(codex_home);
|
||||
let _ = std::fs::remove_dir_all(&system_skills_dir);
|
||||
}
|
||||
|
||||
@@ -25,7 +25,6 @@ use crate::skills::SkillsManager;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::config_types::CollaborationModeMask;
|
||||
use codex_protocol::openai_models::ModelPreset;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::protocol::InitialHistory;
|
||||
use codex_protocol::protocol::McpServerRefreshConfig;
|
||||
use codex_protocol::protocol::Op;
|
||||
@@ -145,18 +144,19 @@ pub(crate) struct ThreadManagerState {
|
||||
|
||||
impl ThreadManager {
|
||||
pub fn new(
|
||||
codex_home: PathBuf,
|
||||
config: &Config,
|
||||
auth_manager: Arc<AuthManager>,
|
||||
session_source: SessionSource,
|
||||
model_catalog: Option<ModelsResponse>,
|
||||
collaboration_modes_config: CollaborationModesConfig,
|
||||
) -> Self {
|
||||
let codex_home = config.codex_home.clone();
|
||||
let (thread_created_tx, _) = broadcast::channel(THREAD_CREATED_CHANNEL_CAPACITY);
|
||||
let plugins_manager = Arc::new(PluginsManager::new(codex_home.clone()));
|
||||
let mcp_manager = Arc::new(McpManager::new(Arc::clone(&plugins_manager)));
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
codex_home.clone(),
|
||||
Arc::clone(&plugins_manager),
|
||||
config.bundled_skills_enabled(),
|
||||
));
|
||||
let file_watcher = build_file_watcher(codex_home.clone(), Arc::clone(&skills_manager));
|
||||
Self {
|
||||
@@ -166,7 +166,7 @@ impl ThreadManager {
|
||||
models_manager: Arc::new(ModelsManager::new(
|
||||
codex_home,
|
||||
auth_manager.clone(),
|
||||
model_catalog,
|
||||
config.model_catalog.clone(),
|
||||
collaboration_modes_config,
|
||||
)),
|
||||
skills_manager,
|
||||
@@ -216,6 +216,7 @@ impl ThreadManager {
|
||||
let skills_manager = Arc::new(SkillsManager::new(
|
||||
codex_home.clone(),
|
||||
Arc::clone(&plugins_manager),
|
||||
true,
|
||||
));
|
||||
let file_watcher = build_file_watcher(codex_home.clone(), Arc::clone(&skills_manager));
|
||||
Self {
|
||||
|
||||
@@ -179,12 +179,11 @@ impl TestCodexBuilder {
|
||||
resume_from: Option<PathBuf>,
|
||||
) -> anyhow::Result<TestCodex> {
|
||||
let auth = self.auth.clone();
|
||||
let thread_manager = if let Some(model_catalog) = config.model_catalog.clone() {
|
||||
let thread_manager = if config.model_catalog.is_some() {
|
||||
ThreadManager::new(
|
||||
config.codex_home.clone(),
|
||||
&config,
|
||||
codex_core::test_support::auth_manager_from_auth(auth.clone()),
|
||||
SessionSource::Exec,
|
||||
Some(model_catalog),
|
||||
CollaborationModesConfig::default(),
|
||||
)
|
||||
} else {
|
||||
|
||||
@@ -815,10 +815,9 @@ async fn prefers_apikey_when_config_prefers_apikey_even_with_chatgpt_tokens() {
|
||||
Err(e) => panic!("Failed to load CodexAuth: {e}"),
|
||||
};
|
||||
let thread_manager = ThreadManager::new(
|
||||
codex_home.path().to_path_buf(),
|
||||
&config,
|
||||
auth_manager,
|
||||
SessionSource::Exec,
|
||||
config.model_catalog.clone(),
|
||||
CollaborationModesConfig {
|
||||
default_mode_request_user_input: config
|
||||
.features
|
||||
|
||||
@@ -59,10 +59,9 @@ impl MessageProcessor {
|
||||
config.cli_auth_credentials_store_mode,
|
||||
);
|
||||
let thread_manager = Arc::new(ThreadManager::new(
|
||||
config.codex_home.clone(),
|
||||
config.as_ref(),
|
||||
auth_manager,
|
||||
SessionSource::Mcp,
|
||||
config.model_catalog.clone(),
|
||||
CollaborationModesConfig {
|
||||
default_mode_request_user_input: config
|
||||
.features
|
||||
|
||||
@@ -1695,10 +1695,9 @@ impl App {
|
||||
let harness_overrides =
|
||||
normalize_harness_overrides_for_cwd(harness_overrides, &config.cwd)?;
|
||||
let thread_manager = Arc::new(ThreadManager::new(
|
||||
config.codex_home.clone(),
|
||||
&config,
|
||||
auth_manager.clone(),
|
||||
SessionSource::Cli,
|
||||
config.model_catalog.clone(),
|
||||
CollaborationModesConfig {
|
||||
default_mode_request_user_input: config
|
||||
.features
|
||||
|
||||
Reference in New Issue
Block a user