mirror of
https://github.com/openai/codex.git
synced 2026-06-01 19:02:59 +00:00
feat: add SkillPolicy to skill metadata and support allow_implicit_invocation (#11244)
Tested by setting the policy in agents/openai.yaml to true, false, and leaving it unset (default). ``` policy: allow_implicit_invocation: false ``` <img width="847" height="289" alt="Screenshot 2026-02-09 at 3 42 41 PM" src="https://github.com/user-attachments/assets/d3476264-3355-47cf-894a-4ffba53e3481" />
This commit is contained in:
@@ -294,8 +294,10 @@ impl Codex {
|
||||
config.features.disable(Feature::Collab);
|
||||
}
|
||||
|
||||
let enabled_skills = loaded_skills.enabled_skills();
|
||||
let user_instructions = get_user_instructions(&config, Some(&enabled_skills)).await;
|
||||
let allowed_skills_for_implicit_invocation =
|
||||
loaded_skills.allowed_skills_for_implicit_invocation();
|
||||
let user_instructions =
|
||||
get_user_instructions(&config, Some(&allowed_skills_for_implicit_invocation)).await;
|
||||
|
||||
let exec_policy = ExecPolicyManager::load(&config.config_layer_stack)
|
||||
.await
|
||||
|
||||
@@ -431,6 +431,7 @@ mod tests {
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: Some(SkillDependencies { tools }),
|
||||
policy: None,
|
||||
path: PathBuf::from("skill"),
|
||||
scope: SkillScope::User,
|
||||
}
|
||||
|
||||
@@ -20,6 +20,9 @@ dependencies:
|
||||
description: "GitHub MCP server"
|
||||
transport: "streamable_http"
|
||||
url: "https://api.githubcopilot.com/mcp/"
|
||||
|
||||
policy:
|
||||
allow_implicit_invocation: true
|
||||
```
|
||||
|
||||
## Field descriptions and constraints
|
||||
@@ -41,3 +44,6 @@ Top-level constraints:
|
||||
- `dependencies.tools[].description`: Human-readable explanation of the dependency.
|
||||
- `dependencies.tools[].transport`: Connection type when `type` is `mcp`.
|
||||
- `dependencies.tools[].url`: MCP server URL when `type` is `mcp`.
|
||||
- `policy.allow_implicit_invocation`: When false, the skill is not injected into
|
||||
the model context by default, but can still be invoked explicitly via `$skill`.
|
||||
Defaults to true.
|
||||
|
||||
@@ -473,6 +473,7 @@ mod tests {
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: PathBuf::from(path),
|
||||
scope: codex_protocol::protocol::SkillScope::User,
|
||||
}
|
||||
|
||||
@@ -9,6 +9,7 @@ use crate::skills::model::SkillError;
|
||||
use crate::skills::model::SkillInterface;
|
||||
use crate::skills::model::SkillLoadOutcome;
|
||||
use crate::skills::model::SkillMetadata;
|
||||
use crate::skills::model::SkillPolicy;
|
||||
use crate::skills::model::SkillToolDependency;
|
||||
use crate::skills::system::system_cache_root_dir;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
@@ -47,6 +48,8 @@ struct SkillMetadataFile {
|
||||
interface: Option<Interface>,
|
||||
#[serde(default)]
|
||||
dependencies: Option<Dependencies>,
|
||||
#[serde(default)]
|
||||
policy: Option<Policy>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default, Deserialize)]
|
||||
@@ -65,6 +68,12 @@ struct Dependencies {
|
||||
tools: Vec<DependencyTool>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
struct Policy {
|
||||
#[serde(default)]
|
||||
allow_implicit_invocation: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Default, Deserialize)]
|
||||
struct DependencyTool {
|
||||
#[serde(rename = "type")]
|
||||
@@ -481,7 +490,7 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result<SkillMetadata, Ski
|
||||
.as_deref()
|
||||
.map(sanitize_single_line)
|
||||
.filter(|value| !value.is_empty());
|
||||
let (interface, dependencies) = load_skill_metadata(path);
|
||||
let (interface, dependencies, policy) = load_skill_metadata(path);
|
||||
|
||||
validate_len(&name, MAX_NAME_LEN, "name")?;
|
||||
validate_len(&description, MAX_DESCRIPTION_LEN, "description")?;
|
||||
@@ -501,21 +510,28 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result<SkillMetadata, Ski
|
||||
short_description,
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
path: resolved_path,
|
||||
scope,
|
||||
})
|
||||
}
|
||||
|
||||
fn load_skill_metadata(skill_path: &Path) -> (Option<SkillInterface>, Option<SkillDependencies>) {
|
||||
fn load_skill_metadata(
|
||||
skill_path: &Path,
|
||||
) -> (
|
||||
Option<SkillInterface>,
|
||||
Option<SkillDependencies>,
|
||||
Option<SkillPolicy>,
|
||||
) {
|
||||
// Fail open: optional metadata should not block loading SKILL.md.
|
||||
let Some(skill_dir) = skill_path.parent() else {
|
||||
return (None, None);
|
||||
return (None, None, None);
|
||||
};
|
||||
let metadata_path = skill_dir
|
||||
.join(SKILLS_METADATA_DIR)
|
||||
.join(SKILLS_METADATA_FILENAME);
|
||||
if !metadata_path.exists() {
|
||||
return (None, None);
|
||||
return (None, None, None);
|
||||
}
|
||||
|
||||
let contents = match fs::read_to_string(&metadata_path) {
|
||||
@@ -526,7 +542,7 @@ fn load_skill_metadata(skill_path: &Path) -> (Option<SkillInterface>, Option<Ski
|
||||
path = metadata_path.display(),
|
||||
label = SKILLS_METADATA_FILENAME
|
||||
);
|
||||
return (None, None);
|
||||
return (None, None, None);
|
||||
}
|
||||
};
|
||||
|
||||
@@ -538,13 +554,20 @@ fn load_skill_metadata(skill_path: &Path) -> (Option<SkillInterface>, Option<Ski
|
||||
path = metadata_path.display(),
|
||||
label = SKILLS_METADATA_FILENAME
|
||||
);
|
||||
return (None, None);
|
||||
return (None, None, None);
|
||||
}
|
||||
};
|
||||
|
||||
let SkillMetadataFile {
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
} = parsed;
|
||||
|
||||
(
|
||||
resolve_interface(parsed.interface, skill_dir),
|
||||
resolve_dependencies(parsed.dependencies),
|
||||
resolve_interface(interface, skill_dir),
|
||||
resolve_dependencies(dependencies),
|
||||
resolve_policy(policy),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -593,6 +616,12 @@ fn resolve_dependencies(dependencies: Option<Dependencies>) -> Option<SkillDepen
|
||||
}
|
||||
}
|
||||
|
||||
fn resolve_policy(policy: Option<Policy>) -> Option<SkillPolicy> {
|
||||
policy.map(|policy| SkillPolicy {
|
||||
allow_implicit_invocation: policy.allow_implicit_invocation,
|
||||
})
|
||||
}
|
||||
|
||||
fn resolve_dependency_tool(tool: DependencyTool) -> Option<SkillToolDependency> {
|
||||
let r#type = resolve_required_str(
|
||||
tool.kind,
|
||||
@@ -991,6 +1020,7 @@ mod tests {
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1137,6 +1167,7 @@ mod tests {
|
||||
},
|
||||
],
|
||||
}),
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1191,12 +1222,79 @@ interface:
|
||||
default_prompt: Some("default prompt".to_string()),
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(skill_path.as_path()),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn loads_skill_policy_from_yaml() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let skill_path = write_skill(&codex_home, "demo", "policy-skill", "from json");
|
||||
let skill_dir = skill_path.parent().expect("skill dir");
|
||||
|
||||
write_skill_metadata_at(
|
||||
skill_dir,
|
||||
r#"
|
||||
policy:
|
||||
allow_implicit_invocation: false
|
||||
"#,
|
||||
);
|
||||
|
||||
let cfg = make_config(&codex_home).await;
|
||||
let outcome = load_skills(&cfg);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
assert_eq!(outcome.skills.len(), 1);
|
||||
assert_eq!(
|
||||
outcome.skills[0].policy,
|
||||
Some(SkillPolicy {
|
||||
allow_implicit_invocation: Some(false),
|
||||
})
|
||||
);
|
||||
assert!(outcome.allowed_skills_for_implicit_invocation().is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn empty_skill_policy_defaults_to_allow_implicit_invocation() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let skill_path = write_skill(&codex_home, "demo", "policy-empty", "from json");
|
||||
let skill_dir = skill_path.parent().expect("skill dir");
|
||||
|
||||
write_skill_metadata_at(
|
||||
skill_dir,
|
||||
r#"
|
||||
policy: {}
|
||||
"#,
|
||||
);
|
||||
|
||||
let cfg = make_config(&codex_home).await;
|
||||
let outcome = load_skills(&cfg);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
outcome.errors
|
||||
);
|
||||
assert_eq!(outcome.skills.len(), 1);
|
||||
assert_eq!(
|
||||
outcome.skills[0].policy,
|
||||
Some(SkillPolicy {
|
||||
allow_implicit_invocation: None,
|
||||
})
|
||||
);
|
||||
assert_eq!(
|
||||
outcome.allowed_skills_for_implicit_invocation(),
|
||||
outcome.skills
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn accepts_icon_paths_under_assets_dir() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
@@ -1240,6 +1338,7 @@ interface:
|
||||
default_prompt: None,
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1279,6 +1378,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1331,6 +1431,7 @@ interface:
|
||||
default_prompt: None,
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1371,6 +1472,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1414,6 +1516,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&shared_skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1473,6 +1576,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1508,6 +1612,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&shared_skill_path),
|
||||
scope: SkillScope::Admin,
|
||||
}]
|
||||
@@ -1547,6 +1652,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&linked_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
@@ -1565,9 +1671,10 @@ interface:
|
||||
fs::create_dir_all(&system_root).unwrap();
|
||||
symlink_dir(shared.path(), &system_root.join("shared"));
|
||||
|
||||
let cfg = make_config(&codex_home).await;
|
||||
let outcome = load_skills(&cfg);
|
||||
|
||||
let outcome = load_skills_from_roots([SkillRoot {
|
||||
path: system_root,
|
||||
scope: SkillScope::System,
|
||||
}]);
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
"unexpected errors: {:?}",
|
||||
@@ -1593,8 +1700,11 @@ interface:
|
||||
"should not load",
|
||||
);
|
||||
|
||||
let cfg = make_config(&codex_home).await;
|
||||
let outcome = load_skills(&cfg);
|
||||
let skills_root = codex_home.path().join("skills");
|
||||
let outcome = load_skills_from_roots([SkillRoot {
|
||||
path: skills_root,
|
||||
scope: SkillScope::User,
|
||||
}]);
|
||||
|
||||
assert!(
|
||||
outcome.errors.is_empty(),
|
||||
@@ -1609,6 +1719,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&within_depth_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1635,6 +1746,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1665,6 +1777,7 @@ interface:
|
||||
short_description: Some("short summary".to_string()),
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
}]
|
||||
@@ -1776,6 +1889,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
@@ -1810,6 +1924,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
@@ -1862,6 +1977,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&nested_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
@@ -1871,6 +1987,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&root_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
@@ -1909,6 +2026,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
@@ -1945,6 +2063,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
@@ -1985,6 +2104,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&repo_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
@@ -1994,6 +2114,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&user_skill_path),
|
||||
scope: SkillScope::User,
|
||||
},
|
||||
@@ -2057,6 +2178,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: first_path,
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
@@ -2066,6 +2188,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: second_path,
|
||||
scope: SkillScope::Repo,
|
||||
},
|
||||
@@ -2136,6 +2259,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
}]
|
||||
@@ -2193,6 +2317,7 @@ interface:
|
||||
short_description: None,
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path: normalized(&skill_path),
|
||||
scope: SkillScope::System,
|
||||
}]
|
||||
|
||||
@@ -17,4 +17,5 @@ pub use manager::SkillsManager;
|
||||
pub use model::SkillError;
|
||||
pub use model::SkillLoadOutcome;
|
||||
pub use model::SkillMetadata;
|
||||
pub use model::SkillPolicy;
|
||||
pub use render::render_skills_section;
|
||||
|
||||
@@ -10,10 +10,25 @@ pub struct SkillMetadata {
|
||||
pub short_description: Option<String>,
|
||||
pub interface: Option<SkillInterface>,
|
||||
pub dependencies: Option<SkillDependencies>,
|
||||
pub policy: Option<SkillPolicy>,
|
||||
pub path: PathBuf,
|
||||
pub scope: SkillScope,
|
||||
}
|
||||
|
||||
impl SkillMetadata {
|
||||
fn allow_implicit_invocation(&self) -> bool {
|
||||
self.policy
|
||||
.as_ref()
|
||||
.and_then(|policy| policy.allow_implicit_invocation)
|
||||
.unwrap_or(true)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
|
||||
pub struct SkillPolicy {
|
||||
pub allow_implicit_invocation: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct SkillInterface {
|
||||
pub display_name: Option<String>,
|
||||
@@ -57,10 +72,14 @@ impl SkillLoadOutcome {
|
||||
!self.disabled_paths.contains(&skill.path)
|
||||
}
|
||||
|
||||
pub fn enabled_skills(&self) -> Vec<SkillMetadata> {
|
||||
pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool {
|
||||
self.is_skill_enabled(skill) && skill.allow_implicit_invocation()
|
||||
}
|
||||
|
||||
pub fn allowed_skills_for_implicit_invocation(&self) -> Vec<SkillMetadata> {
|
||||
self.skills
|
||||
.iter()
|
||||
.filter(|skill| self.is_skill_enabled(skill))
|
||||
.filter(|skill| self.is_skill_allowed_for_implicit_invocation(skill))
|
||||
.cloned()
|
||||
.collect()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user