Skip to content

feat: restrict which skill sources Codex can load #21457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
viyatb-oai wants to merge 1 commit into
base: codex/viyatb/managed-artifact-exposure
Choose a base branch
from
+200 −7
Open

feat: restrict which skill sources Codex can load #21457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 55 additions & 6 deletions codex-rs/core-skills/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::sync::Arc;
use std::sync::RwLock;

use codex_config::ConfigLayerStack;
use codex_config::SkillSourceRequirement;
use codex_exec_server::ExecutorFileSystem;
use codex_protocol::protocol::Product;
use codex_protocol::protocol::SkillScope;
Expand Down Expand Up @@ -51,7 +52,7 @@ impl SkillsLoadInput {
pub struct SkillsManager {
codex_home: AbsolutePathBuf,
restriction_product: Option<Product>,
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, SkillLoadOutcome>>,
cache_by_cwd: RwLock<HashMap<CwdSkillsCacheKey, SkillLoadOutcome>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, SkillLoadOutcome>>,
}

Expand Down Expand Up @@ -123,6 +124,7 @@ impl SkillsManager {
if !input.bundled_skills_enabled {
roots.retain(|root| root.scope != SkillScope::System);
}
retain_allowed_skill_roots(&mut roots, &input.config_layer_stack);
roots
}

Expand All @@ -144,9 +146,10 @@ impl SkillsManager {
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
let use_cwd_cache = fs.is_some();
let cache_key = cwd_skills_cache_key(&input.cwd, &input.config_layer_stack);
if use_cwd_cache
&& !force_reload
&& let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd)
&& let Some(outcome) = self.cached_outcome_for_cwd(&cache_key)
{
return outcome;
}
Expand All @@ -173,14 +176,15 @@ impl SkillsManager {
}),
);
}
retain_allowed_skill_roots(&mut roots, &input.config_layer_stack);
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let outcome = self.build_skill_outcome(roots, &skill_config_rules).await;
if use_cwd_cache {
let mut cache = self
.cache_by_cwd
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.insert(input.cwd.clone(), outcome.clone());
cache.insert(cache_key, outcome.clone());
}
outcome
}
Expand Down Expand Up @@ -221,10 +225,10 @@ impl SkillsManager {
info!("skills cache cleared ({cleared} entries)");
}

fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option<SkillLoadOutcome> {
fn cached_outcome_for_cwd(&self, cache_key: &CwdSkillsCacheKey) -> Option<SkillLoadOutcome> {
match self.cache_by_cwd.read() {
Ok(cache) => cache.get(cwd).cloned(),
Err(err) => err.into_inner().get(cwd).cloned(),
Ok(cache) => cache.get(cache_key).cloned(),
Err(err) => err.into_inner().get(cache_key).cloned(),
}
}

Expand All @@ -239,12 +243,43 @@ impl SkillsManager {
}
}

fn retain_allowed_skill_roots(roots: &mut Vec<SkillRoot>, config_layer_stack: &ConfigLayerStack) {
let Some(requirements) = config_layer_stack.requirements().skills.as_ref() else {
return;
};

roots.retain(|root| {
requirements
.value
.allows_source(skill_source_for_root(root))
});
}

fn skill_source_for_root(root: &SkillRoot) -> SkillSourceRequirement {
if root.plugin_id.is_some() {
return SkillSourceRequirement::Plugin;
}

match root.scope {
SkillScope::Repo => SkillSourceRequirement::Repo,
SkillScope::User => SkillSourceRequirement::User,
SkillScope::System => SkillSourceRequirement::System,
SkillScope::Admin => SkillSourceRequirement::Admin,
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct ConfigSkillsCacheKey {
roots: Vec<(AbsolutePathBuf, u8, Option<String>)>,
skill_config_rules: SkillConfigRules,
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
struct CwdSkillsCacheKey {
cwd: AbsolutePathBuf,
allowed_sources: Option<Vec<SkillSourceRequirement>>,
}

pub fn bundled_skills_enabled_from_stack(
config_layer_stack: &codex_config::ConfigLayerStack,
) -> bool {
Expand Down Expand Up @@ -288,6 +323,20 @@ fn config_skills_cache_key(
}
}

fn cwd_skills_cache_key(
cwd: &AbsolutePathBuf,
config_layer_stack: &ConfigLayerStack,
) -> CwdSkillsCacheKey {
CwdSkillsCacheKey {
cwd: cwd.clone(),
allowed_sources: config_layer_stack
.requirements()
.skills
.as_ref()
.and_then(|requirements| requirements.value.allowed_sources.clone()),
}
}

fn finalize_skill_outcome(
mut outcome: SkillLoadOutcome,
disabled_paths: HashSet<AbsolutePathBuf>,
Expand Down
146 changes: 145 additions & 1 deletion codex-rs/core-skills/src/manager_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ use codex_app_server_protocol::ConfigLayerSource;
use codex_config::CONFIG_TOML_FILE;
use codex_config::ConfigLayerEntry;
use codex_config::ConfigLayerStack;
use codex_config::ConfigRequirements;
use codex_config::ConfigRequirementsToml;
use codex_config::RequirementSource;
use codex_config::SkillSourceRequirement;
use codex_config::SkillsRequirementsToml;
use codex_config::Sourced;
use codex_exec_server::LOCAL_FS;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_absolute_path::test_support::PathBufExt;
Expand All @@ -27,6 +32,13 @@ fn write_user_skill(codex_home: &TempDir, dir: &str, name: &str, description: &s
fs::write(skill_dir.join("SKILL.md"), content).unwrap();
}

fn write_repo_skill(cwd: &TempDir, dir: &str, name: &str, description: &str) {
let skill_dir = cwd.path().join(".agents/skills").join(dir);
fs::create_dir_all(&skill_dir).unwrap();
let content = format!("---\nname: {name}\ndescription: {description}\n---\n\n# Body\n");
fs::write(skill_dir.join("SKILL.md"), content).unwrap();
}

fn write_plugin_skill(
codex_home: &TempDir,
marketplace: &str,
Expand Down Expand Up @@ -94,9 +106,17 @@ fn user_config_layer(codex_home: &TempDir, config_toml: &str) -> ConfigLayerEntr
}

fn config_stack(codex_home: &TempDir, user_config_toml: &str) -> ConfigLayerStack {
config_stack_with_requirements(codex_home, user_config_toml, ConfigRequirements::default())
}

fn config_stack_with_requirements(
codex_home: &TempDir,
user_config_toml: &str,
requirements: ConfigRequirements,
) -> ConfigLayerStack {
ConfigLayerStack::new(
vec![user_config_layer(codex_home, user_config_toml)],
Default::default(),
requirements,
ConfigRequirementsToml::default(),
)
.expect("valid config layer stack")
Expand Down Expand Up @@ -262,6 +282,65 @@ async fn skills_for_config_disables_plugin_skills_by_name() {
);
}

#[tokio::test]
async fn skills_for_config_filters_disallowed_managed_sources() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
write_user_skill(&codex_home, "user", "user-skill", "from user");
write_repo_skill(&cwd, "repo", "repo-skill", "from repo");
let plugin_skill_path = write_plugin_skill(
&codex_home,
"test",
"sample",
"plugin",
"plugin-skill",
"from plugin",
);
let config_layer_stack = config_stack_with_requirements(
&codex_home,
"",
ConfigRequirements {
skills: Some(Sourced::new(
SkillsRequirementsToml {
allowed_sources: Some(vec![
SkillSourceRequirement::System,
SkillSourceRequirement::Admin,
SkillSourceRequirement::Plugin,
]),
},
RequirementSource::Unknown,
)),
..Default::default()
},
);
let skills_manager = SkillsManager::new(
codex_home.path().abs(),
/*bundled_skills_enabled*/ true,
);

let outcome = skills_for_config_with_stack(
&skills_manager,
&cwd,
&config_layer_stack,
&[plugin_skill_path
.parent()
.and_then(std::path::Path::parent)
.expect("plugin skills root")
.to_path_buf()
.abs()],
)
.await;
let skill_names = outcome
.skills
.iter()
.map(|skill| skill.name.as_str())
.collect::<HashSet<_>>();

assert!(skill_names.contains("sample:plugin-skill"));
assert!(!skill_names.contains("user-skill"));
assert!(!skill_names.contains("repo-skill"));
}

#[tokio::test]
async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() {
let codex_home = tempfile::tempdir().expect("tempdir");
Expand Down Expand Up @@ -322,6 +401,71 @@ async fn skills_for_cwd_reuses_cached_entry_even_when_entry_has_extra_roots() {
assert_eq!(outcome_without_extra.errors, outcome_with_extra.errors);
}

#[tokio::test]
async fn skills_for_cwd_separates_cache_entries_by_managed_allowed_sources() {
let codex_home = tempfile::tempdir().expect("tempdir");
let cwd = tempfile::tempdir().expect("tempdir");
write_user_skill(&codex_home, "user", "user-skill", "from user");
let unrestricted_stack = config_stack(&codex_home, "");
let restricted_stack = config_stack_with_requirements(
&codex_home,
"",
ConfigRequirements {
skills: Some(Sourced::new(
SkillsRequirementsToml {
allowed_sources: Some(vec![SkillSourceRequirement::System]),
},
RequirementSource::Unknown,
)),
..Default::default()
},
);
let skills_manager = SkillsManager::new(
codex_home.path().abs(),
/*bundled_skills_enabled*/ true,
);

let unrestricted_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
unrestricted_stack.clone(),
bundled_skills_enabled_from_stack(&unrestricted_stack),
);
let unrestricted = skills_manager
.skills_for_cwd(
&unrestricted_input,
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
)
.await;
assert!(
unrestricted
.skills
.iter()
.any(|skill| skill.name == "user-skill")
);

let restricted_input = SkillsLoadInput::new(
cwd.path().abs(),
Vec::new(),
restricted_stack.clone(),
bundled_skills_enabled_from_stack(&restricted_stack),
);
let restricted = skills_manager
.skills_for_cwd(
&restricted_input,
/*force_reload*/ false,
Some(Arc::clone(&LOCAL_FS)),
)
.await;
assert!(
!restricted
.skills
.iter()
.any(|skill| skill.name == "user-skill")
);
}

#[tokio::test]
async fn skills_for_cwd_loads_repo_user_and_extra_roots_with_local_fs() {
let codex_home = tempfile::tempdir().expect("tempdir");
Expand Down
Loading