From c1926c83d987fe2e3e58ab85d8356f4381c321a4 Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Tue, 17 Feb 2026 21:56:00 -0800 Subject: [PATCH] fix: skills module audit cleanup (#173) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: skills module audit cleanup — deduplicate loading, async gating, pre-compute scoring fields Address 7 issues from the skills module audit (#157–#163): - Extract shared `load_and_validate_skill` helper, eliminating ~90 lines of duplication between `load_skill_md` and `load_skill_md_standalone` - Wrap blocking gating subprocess calls (`which`/`where`) in `tokio::task::spawn_blocking` to avoid blocking the async runtime - Remove dead `SkillParseError::FileTooLarge` and `SkillSource::Registry` - Replace `HashMap` with `HashSet` in discovery - Fix misleading doc comment and unnecessary `ref` clone pattern - Use `CARGO_PKG_VERSION` for catalog HTTP user-agent instead of hardcoded "0.1" - Pre-compute lowercased keywords/tags at load time to avoid per-message allocation in the scoring hot path - Add tests for flat SKILL.md layout, mixed layouts, and lowercased field population Closes #157, closes #158, closes #159, closes #160, closes #161, closes #162, closes #163 Co-Authored-By: Claude Opus 4.6 * fix: address PR #173 review feedback - Distinguish cancel vs panic in spawn_blocking JoinError and include error details in the gating failure message (Copilot review) - Restore lowercased_keywords/lowercased_tags to `pub` for consistency with other LoadedSkill fields (Copilot review) Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/skills/attenuation.rs | 2 + src/skills/catalog.rs | 4 +- src/skills/gating.rs | 42 +++++-- src/skills/mod.rs | 10 +- src/skills/parser.rs | 3 - src/skills/registry.rs | 239 +++++++++++++++++++------------------- src/skills/selector.rs | 23 ++-- 7 files changed, 176 insertions(+), 147 deletions(-) diff --git a/src/skills/attenuation.rs b/src/skills/attenuation.rs index 36a7f63d..87b743ba 100644 --- a/src/skills/attenuation.rs +++ b/src/skills/attenuation.rs @@ -141,6 +141,8 @@ mod tests { source: SkillSource::User(PathBuf::from("/tmp")), content_hash: "sha256:000".to_string(), compiled_patterns: vec![], + lowercased_keywords: vec![], + lowercased_tags: vec![], } } diff --git a/src/skills/catalog.rs b/src/skills/catalog.rs index 3a6eeb77..76c8b971 100644 --- a/src/skills/catalog.rs +++ b/src/skills/catalog.rs @@ -72,7 +72,7 @@ impl SkillCatalog { let client = reqwest::Client::builder() .timeout(REQUEST_TIMEOUT) - .user_agent("ironclaw/0.1") + .user_agent(concat!("ironclaw/", env!("CARGO_PKG_VERSION"))) .build() .unwrap_or_default(); @@ -88,7 +88,7 @@ impl SkillCatalog { pub fn with_url(url: &str) -> Self { let client = reqwest::Client::builder() .timeout(REQUEST_TIMEOUT) - .user_agent("ironclaw/0.1") + .user_agent(concat!("ironclaw/", env!("CARGO_PKG_VERSION"))) .build() .unwrap_or_default(); diff --git a/src/skills/gating.rs b/src/skills/gating.rs index 0c13b323..7051f71e 100644 --- a/src/skills/gating.rs +++ b/src/skills/gating.rs @@ -14,14 +14,40 @@ pub struct GatingResult { pub failures: Vec, } -/// Check whether gating requirements are satisfied. +/// Async wrapper around [`check_requirements_sync`] that offloads blocking +/// subprocess calls (`which`/`where`) to a blocking thread pool via +/// `tokio::task::spawn_blocking`. +pub async fn check_requirements(requirements: &GatingRequirements) -> GatingResult { + let requirements = requirements.clone(); + tokio::task::spawn_blocking(move || check_requirements_sync(&requirements)) + .await + .unwrap_or_else(|e| { + let message = if e.is_panic() { + format!("gating check panicked: {}", e) + } else if e.is_cancelled() { + format!("gating check task was cancelled: {}", e) + } else { + format!("gating check failed to join: {}", e) + }; + tracing::error!("{}", message); + GatingResult { + passed: false, + failures: vec![message], + } + }) +} + +/// Check whether gating requirements are satisfied (synchronous). /// /// - `bins`: checks that each binary is findable via `which` (PATH lookup). /// - `env`: checks that each environment variable is set. /// - `config`: checks that each config file path exists. /// /// Skills that fail gating should be logged and skipped, not loaded. -pub fn check_requirements(requirements: &GatingRequirements) -> GatingResult { +/// +/// This is the synchronous implementation; prefer the async [`check_requirements`] +/// wrapper when calling from async contexts to avoid blocking the tokio runtime. +pub fn check_requirements_sync(requirements: &GatingRequirements) -> GatingResult { let mut failures = Vec::new(); for bin in &requirements.bins { @@ -77,7 +103,7 @@ mod tests { #[test] fn test_empty_requirements_pass() { let req = GatingRequirements::default(); - let result = check_requirements(&req); + let result = check_requirements_sync(&req); assert!(result.passed); assert!(result.failures.is_empty()); } @@ -88,7 +114,7 @@ mod tests { bins: vec!["__ironclaw_nonexistent_binary_xyz__".to_string()], ..Default::default() }; - let result = check_requirements(&req); + let result = check_requirements_sync(&req); assert!(!result.passed); assert_eq!(result.failures.len(), 1); assert!(result.failures[0].contains("binary not found")); @@ -100,7 +126,7 @@ mod tests { env: vec!["__IRONCLAW_TEST_NONEXISTENT_VAR__".to_string()], ..Default::default() }; - let result = check_requirements(&req); + let result = check_requirements_sync(&req); assert!(!result.passed); assert!(result.failures[0].contains("env var not set")); } @@ -112,7 +138,7 @@ mod tests { env: vec!["PATH".to_string()], ..Default::default() }; - let result = check_requirements(&req); + let result = check_requirements_sync(&req); assert!(result.passed); } @@ -122,7 +148,7 @@ mod tests { config: vec!["/nonexistent/path/ironclaw_test.conf".to_string()], ..Default::default() }; - let result = check_requirements(&req); + let result = check_requirements_sync(&req); assert!(!result.passed); assert!(result.failures[0].contains("config not found")); } @@ -134,7 +160,7 @@ mod tests { env: vec!["__NO_SUCH_VAR__".to_string()], config: vec!["/no/such/file".to_string()], }; - let result = check_requirements(&req); + let result = check_requirements_sync(&req); assert!(!result.passed); assert_eq!(result.failures.len(), 3); } diff --git a/src/skills/mod.rs b/src/skills/mod.rs index 7091166a..78407812 100644 --- a/src/skills/mod.rs +++ b/src/skills/mod.rs @@ -89,8 +89,6 @@ pub enum SkillSource { User(PathBuf), /// Bundled with the application. Bundled(PathBuf), - /// Downloaded from a registry. - Registry { name: String }, } /// Activation criteria parsed from SKILL.md frontmatter `activation` section. @@ -198,6 +196,12 @@ pub struct LoadedSkill { pub content_hash: String, /// Pre-compiled regex patterns from activation criteria (compiled at load time). pub compiled_patterns: Vec, + /// Pre-computed lowercased keywords for scoring (avoids per-message allocation). + /// Derived from `manifest.activation.keywords` at load time — do not mutate independently. + pub lowercased_keywords: Vec, + /// Pre-computed lowercased tags for scoring (avoids per-message allocation). + /// Derived from `manifest.activation.tags` at load time — do not mutate independently. + pub lowercased_tags: Vec, } impl LoadedSkill { @@ -440,6 +444,8 @@ metadata: source: SkillSource::User(PathBuf::from("/tmp/test")), content_hash: "sha256:000".to_string(), compiled_patterns: vec![], + lowercased_keywords: vec![], + lowercased_tags: vec![], }; assert_eq!(skill.name(), "test"); assert_eq!(skill.version(), "1.0.0"); diff --git a/src/skills/parser.rs b/src/skills/parser.rs index 1d9f4fba..532dcdde 100644 --- a/src/skills/parser.rs +++ b/src/skills/parser.rs @@ -19,9 +19,6 @@ pub enum SkillParseError { #[error("Invalid skill name '{name}': must match [a-zA-Z0-9][a-zA-Z0-9._-]{{0,63}}")] InvalidName { name: String }, - - #[error("SKILL.md too large: {size} bytes (max {max} bytes)")] - FileTooLarge { size: u64, max: u64 }, } /// Result of parsing a SKILL.md file. diff --git a/src/skills/registry.rs b/src/skills/registry.rs index 765917f2..517e2694 100644 --- a/src/skills/registry.rs +++ b/src/skills/registry.rs @@ -8,12 +8,12 @@ //! layouts are supported. Earlier locations win on name collision (workspace //! overrides user). Uses async I/O throughout to avoid blocking the tokio runtime. -use std::collections::HashMap; +use std::collections::HashSet; use std::path::{Path, PathBuf}; use sha2::{Digest, Sha256}; -use crate::skills::gating::check_requirements; +use crate::skills::gating; use crate::skills::parser::{SkillParseError, parse_skill_md}; use crate::skills::{ GatingRequirements, LoadedSkill, MAX_PROMPT_FILE_SIZE, SkillSource, SkillTrust, @@ -66,7 +66,7 @@ pub enum SkillRegistryError { /// Registry of available skills. pub struct SkillRegistry { - /// Loaded skills keyed by name. + /// All loaded skills. skills: Vec, /// User skills directory (~/.ironclaw/skills/). user_dir: PathBuf, @@ -97,18 +97,18 @@ impl SkillRegistry { /// 2. User skills directory -- Trusted pub async fn discover_all(&mut self) -> Vec { let mut loaded_names: Vec = Vec::new(); - let mut seen: HashMap = HashMap::new(); + let mut seen: HashSet = HashSet::new(); // 1. Workspace skills (highest priority) - if let Some(ref ws_dir) = self.workspace_dir.clone() { + if let Some(ws_dir) = self.workspace_dir.clone() { let ws_skills = self - .discover_from_dir(ws_dir, SkillTrust::Trusted, SkillSource::Workspace) + .discover_from_dir(&ws_dir, SkillTrust::Trusted, SkillSource::Workspace) .await; for (name, skill) in ws_skills { - if seen.contains_key(&name) { + if seen.contains(&name) { continue; } - seen.insert(name.clone(), ()); + seen.insert(name.clone()); loaded_names.push(name); self.skills.push(skill); } @@ -120,11 +120,11 @@ impl SkillRegistry { .discover_from_dir(&user_dir, SkillTrust::Trusted, SkillSource::User) .await; for (name, skill) in user_skills { - if seen.contains_key(&name) { + if seen.contains(&name) { tracing::debug!("Skipping user skill '{}' (overridden by workspace)", name); continue; } - seen.insert(name.clone(), ()); + seen.insert(name.clone()); loaded_names.push(name); self.skills.push(skill); } @@ -241,103 +241,7 @@ impl SkillRegistry { trust: SkillTrust, source: SkillSource, ) -> Result<(String, LoadedSkill), SkillRegistryError> { - // Check for symlink at the file level - let file_meta = - tokio::fs::symlink_metadata(path) - .await - .map_err(|e| SkillRegistryError::ReadError { - path: path.display().to_string(), - reason: e.to_string(), - })?; - - if file_meta.is_symlink() { - return Err(SkillRegistryError::SymlinkDetected { - path: path.display().to_string(), - }); - } - - // Read and check size - let raw_bytes = tokio::fs::read(path) - .await - .map_err(|e| SkillRegistryError::ReadError { - path: path.display().to_string(), - reason: e.to_string(), - })?; - - if raw_bytes.len() as u64 > MAX_PROMPT_FILE_SIZE { - return Err(SkillRegistryError::FileTooLarge { - name: path.display().to_string(), - size: raw_bytes.len() as u64, - max: MAX_PROMPT_FILE_SIZE, - }); - } - - let raw_content = - String::from_utf8(raw_bytes).map_err(|e| SkillRegistryError::ReadError { - path: path.display().to_string(), - reason: format!("Invalid UTF-8: {}", e), - })?; - - // Normalize line endings before parsing to handle CRLF - let normalized_content = normalize_line_endings(&raw_content); - - // Parse SKILL.md - let parsed = parse_skill_md(&normalized_content).map_err(|e: SkillParseError| match e { - SkillParseError::InvalidName { ref name } => SkillRegistryError::ParseError { - name: name.clone(), - reason: e.to_string(), - }, - _ => SkillRegistryError::ParseError { - name: path.display().to_string(), - reason: e.to_string(), - }, - })?; - - let manifest = parsed.manifest; - let prompt_content = parsed.prompt_content; - - // Check gating requirements - if let Some(ref meta) = manifest.metadata - && let Some(ref openclaw) = meta.openclaw - { - let gating = check_requirements(&openclaw.requires); - if !gating.passed { - return Err(SkillRegistryError::GatingFailed { - name: manifest.name.clone(), - reason: gating.failures.join("; "), - }); - } - } - - // Check token budget (reject if prompt is > 2x declared budget) - // ~4 bytes per token for English prose = ~0.25 tokens per byte - let approx_tokens = (prompt_content.len() as f64 * 0.25) as usize; - let declared = manifest.activation.max_context_tokens; - if declared > 0 && approx_tokens > declared * 2 { - return Err(SkillRegistryError::TokenBudgetExceeded { - name: manifest.name.clone(), - approx_tokens, - declared, - }); - } - - // Compute content hash - let content_hash = compute_hash(&prompt_content); - - // Compile regex patterns - let compiled_patterns = LoadedSkill::compile_patterns(&manifest.activation.patterns); - - let name = manifest.name.clone(); - let skill = LoadedSkill { - manifest, - prompt_content, - trust, - source, - content_hash, - compiled_patterns, - }; - - Ok((name, skill)) + load_and_validate_skill(path, trust, source).await } /// Get all loaded skills. @@ -387,8 +291,7 @@ impl SkillRegistry { // Load by re-reading from disk (validates round-trip) let source = SkillSource::User(skill_dir); - // Use a temporary registry-less load (load_skill_md_standalone) - load_skill_md_standalone(&skill_path, SkillTrust::Installed, source).await + load_and_validate_skill(&skill_path, SkillTrust::Installed, source).await } /// Commit a prepared skill into the in-memory registry. @@ -464,10 +367,6 @@ impl SkillRegistry { name: name.to_string(), reason: "bundled skills cannot be removed".to_string(), }), - SkillSource::Registry { .. } => Err(SkillRegistryError::CannotRemove { - name: name.to_string(), - reason: "registry skills should be uninstalled, not removed".to_string(), - }), } } @@ -527,11 +426,12 @@ impl SkillRegistry { } } -/// Load a single SKILL.md file without requiring a SkillRegistry instance. +/// Load and validate a single SKILL.md file from disk. /// -/// This is used by `prepare_install_to_disk` to avoid borrowing the registry -/// across async boundaries. -async fn load_skill_md_standalone( +/// Shared implementation used by both `SkillRegistry::load_skill_md` (discovery) +/// and `SkillRegistry::prepare_install_to_disk` (installation). This avoids +/// duplicating the read/parse/validate/hash pipeline. +async fn load_and_validate_skill( path: &Path, trust: SkillTrust, source: SkillSource, @@ -551,6 +451,7 @@ async fn load_skill_md_standalone( }); } + // Read and check size let raw_bytes = tokio::fs::read(path) .await .map_err(|e| SkillRegistryError::ReadError { @@ -571,8 +472,10 @@ async fn load_skill_md_standalone( reason: format!("Invalid UTF-8: {}", e), })?; + // Normalize line endings before parsing to handle CRLF let normalized_content = normalize_line_endings(&raw_content); + // Parse SKILL.md let parsed = parse_skill_md(&normalized_content).map_err(|e: SkillParseError| match e { SkillParseError::InvalidName { ref name } => SkillRegistryError::ParseError { name: name.clone(), @@ -587,18 +490,21 @@ async fn load_skill_md_standalone( let manifest = parsed.manifest; let prompt_content = parsed.prompt_content; + // Check gating requirements if let Some(ref meta) = manifest.metadata && let Some(ref openclaw) = meta.openclaw { - let gating = check_requirements(&openclaw.requires); - if !gating.passed { + let result = gating::check_requirements(&openclaw.requires).await; + if !result.passed { return Err(SkillRegistryError::GatingFailed { name: manifest.name.clone(), - reason: gating.failures.join("; "), + reason: result.failures.join("; "), }); } } + // Check token budget (reject if prompt is > 2x declared budget) + // ~4 bytes per token for English prose = ~0.25 tokens per byte let approx_tokens = (prompt_content.len() as f64 * 0.25) as usize; let declared = manifest.activation.max_context_tokens; if declared > 0 && approx_tokens > declared * 2 { @@ -609,9 +515,26 @@ async fn load_skill_md_standalone( }); } + // Compute content hash let content_hash = compute_hash(&prompt_content); + + // Compile regex patterns let compiled_patterns = LoadedSkill::compile_patterns(&manifest.activation.patterns); + // Pre-compute lowercased keywords and tags for efficient scoring + let lowercased_keywords = manifest + .activation + .keywords + .iter() + .map(|k| k.to_lowercase()) + .collect(); + let lowercased_tags = manifest + .activation + .tags + .iter() + .map(|t| t.to_lowercase()) + .collect(); + let name = manifest.name.clone(); let skill = LoadedSkill { manifest, @@ -620,6 +543,8 @@ async fn load_skill_md_standalone( source, content_hash, compiled_patterns, + lowercased_keywords, + lowercased_tags, }; Ok((name, skill)) @@ -635,8 +560,10 @@ pub fn compute_hash(content: &str) -> String { /// Helper to check gating for a `GatingRequirements`. Useful for callers that /// don't have the full skill loaded yet. -pub fn check_gating(requirements: &GatingRequirements) -> crate::skills::gating::GatingResult { - check_requirements(requirements) +pub async fn check_gating( + requirements: &GatingRequirements, +) -> crate::skills::gating::GatingResult { + gating::check_requirements(requirements).await } #[cfg(test)] @@ -939,6 +866,74 @@ mod tests { assert_eq!(registry.count(), 1); } + #[tokio::test] + async fn test_load_flat_layout() { + let dir = tempfile::tempdir().unwrap(); + + // Place a SKILL.md directly in the skills directory (flat layout) + fs::write( + dir.path().join("SKILL.md"), + "---\nname: flat-skill\ndescription: A flat layout skill\nactivation:\n keywords: [\"flat\"]\n---\n\nYou are a flat layout test skill.\n", + ).unwrap(); + + let mut registry = SkillRegistry::new(dir.path().to_path_buf()); + let loaded = registry.discover_all().await; + + assert_eq!(loaded, vec!["flat-skill"]); + assert_eq!(registry.count(), 1); + + let skill = ®istry.skills()[0]; + assert_eq!(skill.trust, SkillTrust::Trusted); + assert!(skill.prompt_content.contains("flat layout test skill")); + } + + #[tokio::test] + async fn test_mixed_flat_and_subdirectory_layout() { + let dir = tempfile::tempdir().unwrap(); + + // Flat layout: SKILL.md directly in the skills directory + fs::write( + dir.path().join("SKILL.md"), + "---\nname: flat-skill\n---\n\nFlat prompt.\n", + ) + .unwrap(); + + // Subdirectory layout: /SKILL.md + let sub_dir = dir.path().join("sub-skill"); + fs::create_dir(&sub_dir).unwrap(); + fs::write( + sub_dir.join("SKILL.md"), + "---\nname: sub-skill\n---\n\nSub prompt.\n", + ) + .unwrap(); + + let mut registry = SkillRegistry::new(dir.path().to_path_buf()); + let loaded = registry.discover_all().await; + + assert_eq!(registry.count(), 2); + assert!(loaded.contains(&"flat-skill".to_string())); + assert!(loaded.contains(&"sub-skill".to_string())); + } + + #[tokio::test] + async fn test_lowercased_fields_populated() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("case-skill"); + fs::create_dir(&skill_dir).unwrap(); + + fs::write( + skill_dir.join("SKILL.md"), + "---\nname: case-skill\nactivation:\n keywords: [\"Write\", \"EDIT\"]\n tags: [\"Email\", \"PROSE\"]\n---\n\nTest prompt.\n", + ).unwrap(); + + let mut registry = SkillRegistry::new(dir.path().to_path_buf()); + registry.discover_all().await; + + let skill = registry.find_by_name("case-skill").unwrap(); + assert_eq!(skill.lowercased_keywords, vec!["write", "edit"]); + assert_eq!(skill.lowercased_tags, vec!["email", "prose"]); + } + #[test] fn test_compute_hash_deterministic() { let h1 = compute_hash("hello world"); diff --git a/src/skills/selector.rs b/src/skills/selector.rs index 2e725b11..060d0caf 100644 --- a/src/skills/selector.rs +++ b/src/skills/selector.rs @@ -100,19 +100,17 @@ pub fn prefilter_skills<'a>( /// Score a skill against a user message. fn score_skill(skill: &LoadedSkill, message_lower: &str, message_original: &str) -> u32 { let mut score: u32 = 0; - let criteria = &skill.manifest.activation; // Keyword scoring with cap to prevent gaming via keyword stuffing let mut keyword_score: u32 = 0; - for keyword in &criteria.keywords { - let kw_lower = keyword.to_lowercase(); + for kw_lower in &skill.lowercased_keywords { // Exact word match (surrounded by word boundaries) if message_lower .split_whitespace() - .any(|word| word.trim_matches(|c: char| !c.is_alphanumeric()) == kw_lower) + .any(|word| word.trim_matches(|c: char| !c.is_alphanumeric()) == kw_lower.as_str()) { keyword_score += 10; - } else if message_lower.contains(&kw_lower) { + } else if message_lower.contains(kw_lower.as_str()) { // Substring match keyword_score += 5; } @@ -121,9 +119,8 @@ fn score_skill(skill: &LoadedSkill, message_lower: &str, message_original: &str) // Tag scoring from activation.tags let mut tag_score: u32 = 0; - for tag in &criteria.tags { - let tag_lower = tag.to_lowercase(); - if message_lower.contains(&tag_lower) { + for tag_lower in &skill.lowercased_tags { + if message_lower.contains(tag_lower.as_str()) { tag_score += 3; } } @@ -150,15 +147,19 @@ mod tests { fn make_skill(name: &str, keywords: &[&str], tags: &[&str], patterns: &[&str]) -> LoadedSkill { let pattern_strings: Vec = patterns.iter().map(|s| s.to_string()).collect(); let compiled = LoadedSkill::compile_patterns(&pattern_strings); + let kw_vec: Vec = keywords.iter().map(|s| s.to_string()).collect(); + let tag_vec: Vec = tags.iter().map(|s| s.to_string()).collect(); + let lowercased_keywords = kw_vec.iter().map(|k| k.to_lowercase()).collect(); + let lowercased_tags = tag_vec.iter().map(|t| t.to_lowercase()).collect(); LoadedSkill { manifest: SkillManifest { name: name.to_string(), version: "1.0.0".to_string(), description: format!("{} skill", name), activation: ActivationCriteria { - keywords: keywords.iter().map(|s| s.to_string()).collect(), + keywords: kw_vec, patterns: pattern_strings, - tags: tags.iter().map(|s| s.to_string()).collect(), + tags: tag_vec, max_context_tokens: 1000, }, metadata: None, @@ -168,6 +169,8 @@ mod tests { source: SkillSource::User(PathBuf::from("/tmp/test")), content_hash: "sha256:000".to_string(), compiled_patterns: compiled, + lowercased_keywords, + lowercased_tags, } }