From 85be576d72be108ebfaec70d2210fce515300849 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Sat, 30 May 2026 18:09:38 -0400 Subject: [PATCH] Define overlapping skill command policy handling --- code-rs/core/src/skills/command_policy.rs | 325 +++++++++++++++++++-- code-rs/core/tests/skill_command_policy.rs | 13 + docs/skills.md | 27 ++ 3 files changed, 339 insertions(+), 26 deletions(-) diff --git a/code-rs/core/src/skills/command_policy.rs b/code-rs/core/src/skills/command_policy.rs index 72851ff33e2..214aa849269 100644 --- a/code-rs/core/src/skills/command_policy.rs +++ b/code-rs/core/src/skills/command_policy.rs @@ -5,6 +5,7 @@ use crate::skills::model::SkillCommandPolicyPreferred; use crate::skills::model::SkillCommandPolicyPreferredKind; use crate::skills::model::SkillMetadata; use regex_lite::Regex; +use std::collections::BTreeSet; use std::path::PathBuf; #[derive(Debug, Clone)] @@ -14,6 +15,7 @@ pub(crate) struct SkillCommandPolicyRuntime { #[derive(Debug, Clone)] pub(crate) struct CompiledSkillCommandPolicy { + order: usize, pub(crate) skill_name: String, pub(crate) skill_path: PathBuf, pub(crate) id: String, @@ -35,7 +37,15 @@ enum MatchSpecificity { pub(crate) struct SkillCommandPolicyMatch<'a> { pub(crate) policy: &'a CompiledSkillCommandPolicy, matched_command: String, + related: Vec>, +} + +#[derive(Debug, Clone)] +struct SingleSkillCommandPolicyMatch<'a> { + policy: &'a CompiledSkillCommandPolicy, + matched_command: String, specificity: MatchSpecificity, + matcher_width: usize, } impl SkillCommandPolicyRuntime { @@ -46,7 +56,7 @@ impl SkillCommandPolicyRuntime { continue; }; for command_policy in &policy.command_policies { - if let Some(compiled) = compile_policy(skill, command_policy) { + if let Some(compiled) = compile_policy(skill, command_policy, policies.len()) { policies.push(compiled); } } @@ -63,7 +73,7 @@ impl SkillCommandPolicyRuntime { return None; } let normalized = NormalizedCommand::from_command(command); - let mut best: Option> = None; + let mut matches = Vec::new(); for policy in &self.policies { if has_confirm_prefix && policy.action == SkillCommandPolicyAction::RequireConfirm { continue; @@ -71,22 +81,32 @@ impl SkillCommandPolicyRuntime { let Some(candidate) = policy_matches(policy, &normalized) else { continue; }; - if best - .as_ref() - .is_none_or(|current| candidate.is_more_specific_than(current)) - { - best = Some(candidate); - } + matches.push(candidate); } - best + matches.sort_by(compare_policy_matches); + let primary = matches.first()?.clone(); + Some(SkillCommandPolicyMatch { + policy: primary.policy, + matched_command: primary.matched_command, + related: matches, + }) } } -impl<'a> SkillCommandPolicyMatch<'a> { - fn is_more_specific_than(&self, other: &Self) -> bool { - specificity_rank(self.specificity) < specificity_rank(other.specificity) - } +fn compare_policy_matches( + left: &SingleSkillCommandPolicyMatch<'_>, + right: &SingleSkillCommandPolicyMatch<'_>, +) -> std::cmp::Ordering { + specificity_rank(left.specificity) + .cmp(&specificity_rank(right.specificity)) + .then_with(|| right.matcher_width.cmp(&left.matcher_width)) + .then_with(|| left.policy.order.cmp(&right.policy.order)) + .then_with(|| left.policy.skill_name.cmp(&right.policy.skill_name)) + .then_with(|| left.policy.skill_path.cmp(&right.policy.skill_path)) + .then_with(|| left.policy.id.cmp(&right.policy.id)) +} +impl SkillCommandPolicyMatch<'_> { pub(crate) fn guidance(&self, original_label: &str, original_value: &str) -> String { let policy = self.policy; let message = policy.message.as_deref().unwrap_or(match policy.action { @@ -107,19 +127,28 @@ impl<'a> SkillCommandPolicyMatch<'a> { policy.id, policy.skill_path.display() ), + "Selected by matcher specificity, matcher width, then stable skill load order.".to_string(), String::new(), message.to_string(), String::new(), ]; - if !policy.preferred.is_empty() { + let preferred = self.unique_preferred_actions(); + if !preferred.is_empty() { lines.push("Preferred actions:".to_string()); - for preferred in &policy.preferred { + for preferred in preferred { lines.push(format_preferred(preferred)); } lines.push(String::new()); } + let related = self.related_policy_lines(); + if !related.is_empty() { + lines.push("Also matched policies:".to_string()); + lines.extend(related); + lines.push(String::new()); + } + if policy.action == SkillCommandPolicyAction::RequireConfirm { lines.push("Resend with `confirm:` only if the user explicitly asked for this raw command.".to_string()); lines.push(String::new()); @@ -129,11 +158,41 @@ impl<'a> SkillCommandPolicyMatch<'a> { lines.push(format!("{original_label}: {original_value}")); lines.join("\n") } + + fn unique_preferred_actions(&self) -> Vec<&SkillCommandPolicyPreferred> { + let mut seen = BTreeSet::new(); + let mut preferred = Vec::new(); + for policy_match in &self.related { + for action in &policy_match.policy.preferred { + if seen.insert(preferred_key(action)) { + preferred.push(action); + } + } + } + preferred + } + + fn related_policy_lines(&self) -> Vec { + self.related + .iter() + .skip(1) + .map(|policy_match| { + format!( + "- skill `{}` ({}) at {}; matched_command: {}", + policy_match.policy.skill_name, + policy_match.policy.id, + policy_match.policy.skill_path.display(), + policy_match.matched_command + ) + }) + .collect() + } } fn compile_policy( skill: &SkillMetadata, policy: &SkillCommandPolicy, + order: usize, ) -> Option { let shell_regex = policy .matcher @@ -141,6 +200,7 @@ fn compile_policy( .as_ref() .and_then(|pattern| Regex::new(pattern).ok()); Some(CompiledSkillCommandPolicy { + order, skill_name: skill.name.clone(), skill_path: skill.path.clone(), id: policy.id.clone(), @@ -155,14 +215,15 @@ fn compile_policy( fn policy_matches<'a>( policy: &'a CompiledSkillCommandPolicy, normalized: &NormalizedCommand, -) -> Option> { +) -> Option> { if let Some(argv_exact) = policy.matcher.argv_exact.as_ref() { for argv in normalized.argv_candidates() { if argv == argv_exact.as_slice() { - return Some(SkillCommandPolicyMatch { + return Some(SingleSkillCommandPolicyMatch { policy, matched_command: argv.join(" "), specificity: MatchSpecificity::Exact, + matcher_width: argv_exact.len(), }); } } @@ -171,10 +232,11 @@ fn policy_matches<'a>( if let Some(argv_prefix) = policy.matcher.argv_prefix.as_ref() { for argv in normalized.argv_candidates() { if argv.starts_with(argv_prefix) { - return Some(SkillCommandPolicyMatch { + return Some(SingleSkillCommandPolicyMatch { policy, matched_command: argv.join(" "), specificity: MatchSpecificity::Prefix, + matcher_width: argv_prefix.len(), }); } } @@ -183,10 +245,11 @@ fn policy_matches<'a>( if let Some(regex) = policy.shell_regex.as_ref() { for text in normalized.text_candidates() { if regex.is_match(&text) { - return Some(SkillCommandPolicyMatch { + return Some(SingleSkillCommandPolicyMatch { policy, matched_command: text.clone(), specificity: MatchSpecificity::Regex, + matcher_width: 0, }); } } @@ -261,6 +324,22 @@ fn extract_shell_script(command: &[String]) -> Option<(usize, &str)> { None } +fn preferred_key(preferred: &SkillCommandPolicyPreferred) -> String { + let kind = match preferred.kind { + SkillCommandPolicyPreferredKind::Script => "script", + SkillCommandPolicyPreferredKind::Skill => "skill", + SkillCommandPolicyPreferredKind::Command => "command", + }; + let path = preferred + .path + .as_ref() + .map(|path| path.to_string_lossy().into_owned()) + .unwrap_or_default(); + let name = preferred.name.as_deref().unwrap_or_default(); + let example = preferred.example_argv.join("\u{1f}"); + format!("{kind}\u{1f}{path}\u{1f}{name}\u{1f}{example}") +} + fn format_preferred(preferred: &SkillCommandPolicyPreferred) -> String { let kind = match preferred.kind { SkillCommandPolicyPreferredKind::Script => "script", @@ -327,18 +406,30 @@ mod tests { use crate::skills::model::SkillScope; fn runtime(policy: SkillCommandPolicy) -> SkillCommandPolicyRuntime { - SkillCommandPolicyRuntime::from_skills(&[SkillMetadata { - name: "github".to_string(), - description: "GitHub workflows".to_string(), + runtime_from_skills(vec![skill("github", vec![policy], false)]) + } + + fn runtime_from_skills(skills: Vec) -> SkillCommandPolicyRuntime { + SkillCommandPolicyRuntime::from_skills(&skills) + } + + fn skill( + name: &str, + command_policies: Vec, + allow_implicit_invocation: bool, + ) -> SkillMetadata { + SkillMetadata { + name: name.to_string(), + description: format!("{name} workflows"), short_description: None, - path: PathBuf::from("/tmp/github/SKILL.md"), + path: PathBuf::from(format!("/tmp/{name}/SKILL.md")), scope: SkillScope::User, content: String::new(), policy: Some(SkillPolicy { - allow_implicit_invocation: Some(false), - command_policies: vec![policy], + allow_implicit_invocation: Some(allow_implicit_invocation), + command_policies, }), - }]) + } } fn policy(matcher: SkillCommandMatcher) -> SkillCommandPolicy { @@ -357,6 +448,22 @@ mod tests { } } + fn policy_with_id(id: &str, matcher: SkillCommandMatcher) -> SkillCommandPolicy { + let mut policy = policy(matcher); + policy.id = id.to_string(); + policy + } + + fn script_preferred(path: &str, purpose: &str) -> SkillCommandPolicyPreferred { + SkillCommandPolicyPreferred { + kind: SkillCommandPolicyPreferredKind::Script, + path: Some(PathBuf::from(path)), + name: None, + example_argv: vec![path.to_string()], + purpose: Some(purpose.to_string()), + } + } + #[test] fn argv_prefix_matches_direct_command() { let runtime = runtime(policy(SkillCommandMatcher { @@ -435,6 +542,172 @@ mod tests { ); } + #[test] + fn overlap_prefers_exact_over_prefix_and_includes_related_guidance() { + let prefix_policy = policy_with_id( + "prefix-policy", + SkillCommandMatcher { + argv_prefix: Some(vec!["gh".to_string(), "pr".to_string()]), + ..Default::default() + }, + ); + let exact_policy = policy_with_id( + "exact-policy", + SkillCommandMatcher { + argv_exact: Some(vec!["gh".to_string(), "pr".to_string(), "merge".to_string()]), + ..Default::default() + }, + ); + let runtime = runtime_from_skills(vec![ + skill("github", vec![prefix_policy], true), + skill("merge-helper", vec![exact_policy], false), + ]); + + let guidance = runtime + .check(&["gh".to_string(), "pr".to_string(), "merge".to_string()], false) + .expect("match") + .guidance("original_argv", "[\"gh\", \"pr\", \"merge\"]"); + + assert!(guidance.contains("Command policy matched skill `merge-helper` (exact-policy)")); + assert!(guidance.contains("Also matched policies:")); + assert!(guidance.contains("skill `github` (prefix-policy)")); + } + + #[test] + fn overlap_prefers_prefix_over_regex() { + let regex_policy = policy_with_id( + "regex-policy", + SkillCommandMatcher { + shell_regex: Some("^gh\\s+pr\\s+merge\\b".to_string()), + ..Default::default() + }, + ); + let prefix_policy = policy_with_id( + "prefix-policy", + SkillCommandMatcher { + argv_prefix: Some(vec!["gh".to_string(), "pr".to_string(), "merge".to_string()]), + ..Default::default() + }, + ); + let runtime = runtime_from_skills(vec![skill( + "github", + vec![regex_policy, prefix_policy], + true, + )]); + + let matched = runtime + .check( + &[ + "bash".to_string(), + "-lc".to_string(), + "gh pr merge 244".to_string(), + ], + false, + ) + .expect("match"); + + assert_eq!(matched.policy.id, "prefix-policy"); + } + + #[test] + fn equal_specificity_uses_stable_loader_order() { + let first_policy = policy_with_id( + "first-policy", + SkillCommandMatcher { + argv_prefix: Some(vec!["gh".to_string(), "issue".to_string()]), + ..Default::default() + }, + ); + let second_policy = policy_with_id( + "second-policy", + SkillCommandMatcher { + argv_prefix: Some(vec!["gh".to_string(), "issue".to_string()]), + ..Default::default() + }, + ); + let runtime = runtime_from_skills(vec![ + skill("first", vec![first_policy], true), + skill("second", vec![second_policy], true), + ]); + + let guidance = runtime + .check(&["gh".to_string(), "issue".to_string(), "list".to_string()], false) + .expect("match") + .guidance("original_argv", "[\"gh\", \"issue\", \"list\"]"); + + assert!(guidance.contains("Command policy matched skill `first` (first-policy)")); + assert!(guidance.contains("skill `second` (second-policy)")); + } + + #[test] + fn overlap_deduplicates_preferred_actions() { + let preferred = script_preferred("/tmp/github/scripts/gh-pr.py", "merge safely"); + let mut first_policy = policy_with_id( + "first-policy", + SkillCommandMatcher { + argv_prefix: Some(vec!["gh".to_string(), "pr".to_string(), "merge".to_string()]), + ..Default::default() + }, + ); + first_policy.preferred = vec![preferred.clone()]; + let mut second_policy = policy_with_id( + "second-policy", + SkillCommandMatcher { + shell_regex: Some("^gh\\s+pr\\s+merge\\b".to_string()), + ..Default::default() + }, + ); + second_policy.preferred = vec![preferred]; + let runtime = runtime_from_skills(vec![ + skill("github", vec![first_policy], true), + skill("github-plan", vec![second_policy], true), + ]); + + let guidance = runtime + .check( + &[ + "bash".to_string(), + "-lc".to_string(), + "gh pr merge 244".to_string(), + ], + false, + ) + .expect("match") + .guidance("original_script", "gh pr merge 244"); + + assert_eq!(guidance.matches("- script;").count(), 1); + assert!(guidance.contains("/tmp/github/scripts/gh-pr.py")); + assert!(guidance.contains("skill `github-plan` (second-policy)")); + } + + #[test] + fn manual_only_policy_participates_in_overlap_guidance() { + let broad_policy = policy_with_id( + "broad-policy", + SkillCommandMatcher { + argv_prefix: Some(vec!["gh".to_string()]), + ..Default::default() + }, + ); + let manual_policy = policy_with_id( + "manual-policy", + SkillCommandMatcher { + argv_exact: Some(vec!["gh".to_string(), "issue".to_string(), "close".to_string()]), + ..Default::default() + }, + ); + let runtime = runtime_from_skills(vec![ + skill("github", vec![broad_policy], true), + skill("manual-github", vec![manual_policy], false), + ]); + + let matched = runtime + .check(&["gh".to_string(), "issue".to_string(), "close".to_string()], false) + .expect("match"); + + assert_eq!(matched.policy.skill_name, "manual-github"); + } + #[test] fn guidance_includes_preferred_action() { let runtime = runtime(policy(SkillCommandMatcher { diff --git a/code-rs/core/tests/skill_command_policy.rs b/code-rs/core/tests/skill_command_policy.rs index 381985a41b3..dda5313d34c 100644 --- a/code-rs/core/tests/skill_command_policy.rs +++ b/code-rs/core/tests/skill_command_policy.rs @@ -113,6 +113,16 @@ policy: path: scripts/gh-pr.py example_argv: ["scripts/gh-pr.py", "merge", ""] purpose: Merge through the helper. + - id: prefer-pr-helper-for-pr-writes + match: + shell_regex: "^gh\\s+pr\\s+(merge|create|edit|comment)\\b" + action: require_preferred + message: Raw gh pr writes should use the helper family. + preferred: + - kind: script + path: scripts/gh-pr.py + example_argv: ["scripts/gh-pr.py", "merge", ""] + purpose: Merge through the helper. --- Use the GitHub helper. "#, @@ -184,7 +194,10 @@ Use the GitHub helper. .expect("follow-up request should contain policy tool output"); assert!(output.contains("Command policy matched skill `github`")); assert!(output.contains("Raw gh pr merge bypasses the helper flow.")); + assert!(output.contains("Also matched policies:")); + assert!(output.contains("prefer-pr-helper-for-pr-writes")); assert!(output.contains("scripts/gh-pr.py")); + assert_eq!(output.matches("- script;").count(), 1); } fn find_function_call_output_text(value: &Value) -> Option<&str> { diff --git a/docs/skills.md b/docs/skills.md index e22dc3b6595..e89e76cff9b 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -55,9 +55,36 @@ default. - `metadata.short-description` (non-empty when present, <=160 chars) - `policy.allow_implicit_invocation` (`true` by default; set `false` for manual-only skills) + - `policy.command_policies` for skill-owned command guardrails. Policies can + match `argv_exact`, `argv_prefix`, or `shell_regex` command shapes and can + require preferred actions, require explicit confirmation, or reject a raw + command. - The body can contain any Markdown; it is not injected into context until the skill is opened. +## Command policies + +Use `policy.command_policies` when a skill owns a fragile or preferred command +workflow, such as a helper script that preserves Markdown formatting, routes +through the correct token, normalizes output, or performs cleanup. + +When multiple loaded skills match the same command, Every Code selects one +primary policy deterministically by matcher specificity (`argv_exact`, then +`argv_prefix`, then `shell_regex`), matcher width, and stable skill load order. +The guard guidance also lists lower-priority matching policies and de-duplicates +identical preferred actions, so overlapping skills can still provide useful +context without repeating the same helper suggestion. + +Manual-only skills are not described in the implicit skill prompt, but their +command policies still participate in runtime command guards. Treat +manual-only as an invocation rule, not a policy-disable switch. + +Prefer one canonical owner for each raw command path. If two sibling skills care +about the same command, give the policy to the skill that owns the workflow and +have the other skill reference that owner in prose. Keep matchers as narrow as +the workflow allows; broad prefixes are appropriate only when the skill truly +owns the whole command family. + ## Loading and rendering - Loaded once at startup.