mirror of
https://github.com/openai/codex.git
synced 2026-05-02 18:37:01 +00:00
## Why Skill metadata accepted a `permissions` block and stored the result on `SkillMetadata`, but that data was never consumed by runtime behavior. Leaving the dead parsing path in place makes it look like skills can widen or otherwise influence execution permissions when, in practice, declared skill permissions are ignored. This change removes that misleading surface area so the skill metadata model matches what the system actually uses. ## What changed - removed `permission_profile` and `managed_network_override` from `core-skills::SkillMetadata` - stopped parsing `permissions` from skill metadata in `core-skills/src/loader.rs` - deleted the loader tests that only exercised the removed permissions parsing path - cleaned up dependent `SkillMetadata` constructors in tests and TUI code that were only carrying `None` for those fields ## Testing - `cargo test -p codex-core-skills` - `cargo test -p codex-tui submission_prefers_selected_duplicate_skill_path` - `just argument-comment-lint`
347 lines
11 KiB
Rust
347 lines
11 KiB
Rust
use super::*;
|
|
use pretty_assertions::assert_eq;
|
|
use std::collections::HashMap;
|
|
use std::collections::HashSet;
|
|
|
|
fn make_skill(name: &str, path: &str) -> SkillMetadata {
|
|
SkillMetadata {
|
|
name: name.to_string(),
|
|
description: format!("{name} skill"),
|
|
short_description: None,
|
|
interface: None,
|
|
dependencies: None,
|
|
policy: None,
|
|
path_to_skills_md: PathBuf::from(path),
|
|
scope: codex_protocol::protocol::SkillScope::User,
|
|
}
|
|
}
|
|
|
|
fn set<'a>(items: &'a [&'a str]) -> HashSet<&'a str> {
|
|
items.iter().copied().collect()
|
|
}
|
|
|
|
fn assert_mentions(text: &str, expected_names: &[&str], expected_paths: &[&str]) {
|
|
let mentions = extract_tool_mentions(text);
|
|
assert_eq!(mentions.names, set(expected_names));
|
|
assert_eq!(mentions.paths, set(expected_paths));
|
|
}
|
|
|
|
fn collect_mentions(
|
|
inputs: &[UserInput],
|
|
skills: &[SkillMetadata],
|
|
disabled_paths: &HashSet<PathBuf>,
|
|
connector_slug_counts: &HashMap<String, usize>,
|
|
) -> Vec<SkillMetadata> {
|
|
collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts)
|
|
}
|
|
|
|
#[test]
|
|
fn text_mentions_skill_requires_exact_boundary() {
|
|
assert_eq!(
|
|
true,
|
|
text_mentions_skill("use $notion-research-doc please", "notion-research-doc")
|
|
);
|
|
assert_eq!(
|
|
true,
|
|
text_mentions_skill("($notion-research-doc)", "notion-research-doc")
|
|
);
|
|
assert_eq!(
|
|
true,
|
|
text_mentions_skill("$notion-research-doc.", "notion-research-doc")
|
|
);
|
|
assert_eq!(
|
|
false,
|
|
text_mentions_skill("$notion-research-docs", "notion-research-doc")
|
|
);
|
|
assert_eq!(
|
|
false,
|
|
text_mentions_skill("$notion-research-doc_extra", "notion-research-doc")
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn text_mentions_skill_handles_end_boundary_and_near_misses() {
|
|
assert_eq!(true, text_mentions_skill("$alpha-skill", "alpha-skill"));
|
|
assert_eq!(false, text_mentions_skill("$alpha-skillx", "alpha-skill"));
|
|
assert_eq!(
|
|
true,
|
|
text_mentions_skill("$alpha-skillx and later $alpha-skill ", "alpha-skill")
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn text_mentions_skill_handles_many_dollars_without_looping() {
|
|
let prefix = "$".repeat(256);
|
|
let text = format!("{prefix} not-a-mention");
|
|
assert_eq!(false, text_mentions_skill(&text, "alpha-skill"));
|
|
}
|
|
|
|
#[test]
|
|
fn extract_tool_mentions_handles_plain_and_linked_mentions() {
|
|
assert_mentions(
|
|
"use $alpha and [$beta](/tmp/beta)",
|
|
&["alpha", "beta"],
|
|
&["/tmp/beta"],
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn extract_tool_mentions_skips_common_env_vars() {
|
|
assert_mentions("use $PATH and $alpha", &["alpha"], &[]);
|
|
assert_mentions("use [$HOME](/tmp/skill)", &[], &[]);
|
|
assert_mentions("use $XDG_CONFIG_HOME and $beta", &["beta"], &[]);
|
|
}
|
|
|
|
#[test]
|
|
fn extract_tool_mentions_requires_link_syntax() {
|
|
assert_mentions("[beta](/tmp/beta)", &[], &[]);
|
|
assert_mentions("[$beta] /tmp/beta", &["beta"], &[]);
|
|
assert_mentions("[$beta]()", &["beta"], &[]);
|
|
}
|
|
|
|
#[test]
|
|
fn extract_tool_mentions_trims_linked_paths_and_allows_spacing() {
|
|
assert_mentions("use [$beta] ( /tmp/beta )", &["beta"], &["/tmp/beta"]);
|
|
}
|
|
|
|
#[test]
|
|
fn extract_tool_mentions_stops_at_non_name_chars() {
|
|
assert_mentions(
|
|
"use $alpha.skill and $beta_extra",
|
|
&["alpha", "beta_extra"],
|
|
&[],
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn extract_tool_mentions_keeps_plugin_skill_namespaces() {
|
|
assert_mentions(
|
|
"use $slack:search and $alpha",
|
|
&["alpha", "slack:search"],
|
|
&[],
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_text_respects_skill_order() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let beta = make_skill("beta-skill", "/tmp/beta");
|
|
let skills = vec![beta.clone(), alpha.clone()];
|
|
let inputs = vec![UserInput::Text {
|
|
text: "first $alpha-skill then $beta-skill".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
// Text scanning should not change the previous selection ordering semantics.
|
|
assert_eq!(selected, vec![beta, alpha]);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_prioritizes_structured_inputs() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let beta = make_skill("beta-skill", "/tmp/beta");
|
|
let skills = vec![alpha.clone(), beta.clone()];
|
|
let inputs = vec![
|
|
UserInput::Text {
|
|
text: "please run $alpha-skill".to_string(),
|
|
text_elements: Vec::new(),
|
|
},
|
|
UserInput::Skill {
|
|
name: "beta-skill".to_string(),
|
|
path: PathBuf::from("/tmp/beta"),
|
|
},
|
|
];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, vec![beta, alpha]);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fallback() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let skills = vec![alpha];
|
|
let inputs = vec![
|
|
UserInput::Text {
|
|
text: "please run $alpha-skill".to_string(),
|
|
text_elements: Vec::new(),
|
|
},
|
|
UserInput::Skill {
|
|
name: "alpha-skill".to_string(),
|
|
path: PathBuf::from("/tmp/missing"),
|
|
},
|
|
];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fallback() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let skills = vec![alpha];
|
|
let inputs = vec![
|
|
UserInput::Text {
|
|
text: "please run $alpha-skill".to_string(),
|
|
text_elements: Vec::new(),
|
|
},
|
|
UserInput::Skill {
|
|
name: "alpha-skill".to_string(),
|
|
path: PathBuf::from("/tmp/alpha"),
|
|
},
|
|
];
|
|
let disabled = HashSet::from([PathBuf::from("/tmp/alpha")]);
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_dedupes_by_path() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let skills = vec![alpha.clone()];
|
|
let inputs = vec and [$alpha-skill](/tmp/alpha)".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, vec![alpha]);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_ambiguous_name() {
|
|
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
|
let beta = make_skill("demo-skill", "/tmp/beta");
|
|
let skills = vec![alpha, beta];
|
|
let inputs = vec![UserInput::Text {
|
|
text: "use $demo-skill and again $demo-skill".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_prefers_linked_path_over_name() {
|
|
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
|
let beta = make_skill("demo-skill", "/tmp/beta");
|
|
let skills = vec![alpha, beta.clone()];
|
|
let inputs = vec".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, vec![beta]);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_plain_name_when_connector_matches() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let skills = vec![alpha];
|
|
let inputs = vec![UserInput::Text {
|
|
text: "use $alpha-skill".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::from([("alpha-skill".to_string(), 1)]);
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_allows_explicit_path_with_connector_conflict() {
|
|
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
|
let skills = vec![alpha.clone()];
|
|
let inputs = vec".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::from([("alpha-skill".to_string(), 1)]);
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, vec![alpha]);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_when_linked_path_disabled() {
|
|
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
|
let beta = make_skill("demo-skill", "/tmp/beta");
|
|
let skills = vec![alpha, beta];
|
|
let inputs = vec".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let disabled = HashSet::from([PathBuf::from("/tmp/alpha")]);
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_prefers_resource_path() {
|
|
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
|
let beta = make_skill("demo-skill", "/tmp/beta");
|
|
let skills = vec![alpha, beta.clone()];
|
|
let inputs = vec".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, vec![beta]);
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_missing_path_with_no_fallback() {
|
|
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
|
let beta = make_skill("demo-skill", "/tmp/beta");
|
|
let skills = vec![alpha, beta];
|
|
let inputs = vec".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|
|
|
|
#[test]
|
|
fn collect_explicit_skill_mentions_skips_missing_path_without_fallback() {
|
|
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
|
let skills = vec![alpha];
|
|
let inputs = vec".to_string(),
|
|
text_elements: Vec::new(),
|
|
}];
|
|
let connector_counts = HashMap::new();
|
|
|
|
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
|
|
|
assert_eq!(selected, Vec::new());
|
|
}
|