gitops/optimclaw
1
0
Fork 0
mirror of https://github.com/outbackdingo/optimclaw.git synced 2026-08-26 15:40:18 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: skills module audit cleanup (#173)

Browse Source 
* 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<String, ()>` with `HashSet<String>` 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 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Illia Polosukhin
2026-02-18 05:56:00 +00:00
committed by GitHub
co-authored by Claude Opus 4.6
parent a1b0e34b3b
commit c1926c83d9
7 changed files with 176 additions and 147 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/skills/attenuation.rs
+2
View File
@@ -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![],
}
}
src/skills/catalog.rs
+2 -2
View File
@@ -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();
src/skills/gating.rs
+34 -8
View File
@@ -14,14 +14,40 @@ pub struct GatingResult {
pub failures: Vec<String>,
}
/// 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);
}
src/skills/mod.rs
+8 -2
View File
@@ -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<Regex>,
/// 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<String>,
/// 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<String>,
}
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");
src/skills/parser.rs
-3
View File
@@ -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.
src/skills/registry.rs
+117 -122
View File
@@ -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<LoadedSkill>,
/// 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<String> {
let mut loaded_names: Vec<String> = Vec::new();
let mut seen: HashMap<String, ()> = HashMap::new();
let mut seen: HashSet<String> = 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 = &registry.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: <name>/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");
src/skills/selector.rs
+13 -10
View File
@@ -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<String> = patterns.iter().map(|s| s.to_string()).collect();
let compiled = LoadedSkill::compile_patterns(&pattern_strings);
let kw_vec: Vec<String> = keywords.iter().map(|s| s.to_string()).collect();
let tag_vec: Vec<String> = 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,
}
}