fix(skills): keep repo command metadata compatible

code-rs/core/src/skills/assets/samples/skill-creator/scripts/quick_validate.py

@@ -8,6 +8,24 @@
 
 import yaml
 
+MAX_STRUCTURED_ITEMS_PER_SKILL = 64
+MAX_STRUCTURED_ID_LEN = 96
+MAX_STRUCTURED_PURPOSE_LEN = 256
+MAX_STRUCTURED_VALUE_LEN = 512
+MAX_COMMAND_ARGV_TOKENS = 32
+MAX_COMMAND_TOKEN_LEN = 160
+ALLOWED_TOP_LEVEL_KEYS = {
+    "name",
+    "description",
+    "metadata",
+    "policy",
+    "resources",
+    "commands",
+    "workflow_defaults",
+}
+ALLOWED_RESOURCE_KINDS = {"script", "reference", "template", "asset"}
+ALLOWED_COMMAND_SOURCES = {"skill", "repo", "external"}
+
 
 def validate_skill(skill_path):
     """Basic validation of a skill"""
@@ -37,6 +55,10 @@ def validate_skill(skill_path):
     if "description" not in frontmatter:
         return False, "Missing required field: description"
 
+    unexpected_keys = set(frontmatter) - ALLOWED_TOP_LEVEL_KEYS
+    if unexpected_keys:
+        return False, f"Unexpected frontmatter key(s): {', '.join(sorted(unexpected_keys))}"
+
     name = frontmatter.get("name", "")
     if not isinstance(name, str) or not name.strip():
         return False, "Name must be a non-empty string"
@@ -61,9 +83,189 @@ def validate_skill(skill_path):
             f"Description is too long ({len(description)} characters). Maximum is 1024 characters.",
         )
 
+    structured_error = validate_structured_metadata(frontmatter, skill_path)
+    if structured_error:
+        return False, structured_error
+
     return True, "Skill validation passed"
 
 
+def validate_structured_metadata(frontmatter, skill_path):
+    resource_paths, resource_error = validate_resources(frontmatter.get("resources"), skill_path)
+    if resource_error:
+        return resource_error
+
+    command_error = validate_commands(frontmatter.get("commands"), resource_paths, skill_path)
+    if command_error:
+        return command_error
+
+    return validate_workflow_defaults(frontmatter.get("workflow_defaults"))
+
+
+def validate_resources(resources, skill_path):
+    if resources is None:
+        return set(), None
+    if not isinstance(resources, list):
+        return set(), "resources must be a list"
+    if len(resources) > MAX_STRUCTURED_ITEMS_PER_SKILL:
+        return set(), f"resources must contain at most {MAX_STRUCTURED_ITEMS_PER_SKILL} entries"
+
+    paths = set()
+    for index, resource in enumerate(resources):
+        field = f"resources[{index}]"
+        if not isinstance(resource, dict):
+            return paths, f"{field} must be a YAML dictionary"
+        unexpected = set(resource) - {"path", "kind", "description"}
+        if unexpected:
+            return paths, f"Unexpected key(s) in {field}: {', '.join(sorted(unexpected))}"
+
+        raw_path = resource.get("path")
+        path_error = validate_relative_path(raw_path, f"{field}.path")
+        if path_error:
+            return paths, path_error
+        assert isinstance(raw_path, str)
+        raw_path = raw_path.strip()
+        if raw_path in paths:
+            return paths, f"{field}.path duplicates {raw_path}"
+        if not (skill_path / raw_path).exists():
+            return paths, f"{field}.path points to missing {raw_path}"
+        paths.add(raw_path)
+
+        if resource.get("kind") not in ALLOWED_RESOURCE_KINDS:
+            allowed = ", ".join(sorted(ALLOWED_RESOURCE_KINDS))
+            return paths, f"{field}.kind must be one of: {allowed}"
+        desc_error = validate_optional_text(
+            resource.get("description"),
+            MAX_STRUCTURED_PURPOSE_LEN,
+            f"{field}.description",
+        )
+        if desc_error:
+            return paths, desc_error
+    return paths, None
+
+
+def validate_commands(commands, resource_paths, skill_path):
+    if commands is None:
+        return None
+    if not isinstance(commands, list):
+        return "commands must be a list"
+    if len(commands) > MAX_STRUCTURED_ITEMS_PER_SKILL:
+        return f"commands must contain at most {MAX_STRUCTURED_ITEMS_PER_SKILL} entries"
+
+    names = set()
+    for index, command in enumerate(commands):
+        field = f"commands[{index}]"
+        if not isinstance(command, dict):
+            return f"{field} must be a YAML dictionary"
+        unexpected = set(command) - {"name", "source", "resource_path", "example_argv", "purpose"}
+        if unexpected:
+            return f"Unexpected key(s) in {field}: {', '.join(sorted(unexpected))}"
+
+        name = command.get("name")
+        text_error = validate_required_text(name, MAX_STRUCTURED_ID_LEN, f"{field}.name")
+        if text_error:
+            return text_error
+        assert isinstance(name, str)
+        name = name.strip()
+        if not all(c.islower() or c.isdigit() or c == "-" for c in name):
+            return f"{field}.name must be lowercase with only letters, digits, and hyphens"
+        if name in names:
+            return f"{field}.name duplicates {name}"
+        names.add(name)
+
+        source = command.get("source", "skill")
+        if source not in ALLOWED_COMMAND_SOURCES:
+            allowed = ", ".join(sorted(ALLOWED_COMMAND_SOURCES))
+            return f"{field}.source must be one of: {allowed}"
+
+        resource_path = command.get("resource_path")
+        if source == "skill":
+            path_error = validate_relative_path(resource_path, f"{field}.resource_path")
+            if path_error:
+                return path_error
+            assert isinstance(resource_path, str)
+            resource_path = resource_path.strip()
+            if resource_path not in resource_paths:
+                return f"{field}.resource_path must be listed in resources"
+            if not (skill_path / resource_path).is_file():
+                return f"{field}.resource_path points to missing file {resource_path}"
+        elif resource_path is not None:
+            return f"{field}.resource_path is only allowed for source: skill"
+
+        argv_error = validate_example_argv(command.get("example_argv"), f"{field}.example_argv")
+        if argv_error:
+            return argv_error
+        purpose_error = validate_required_text(
+            command.get("purpose"),
+            MAX_STRUCTURED_PURPOSE_LEN,
+            f"{field}.purpose",
+        )
+        if purpose_error:
+            return purpose_error
+    return None
+
+
+def validate_workflow_defaults(defaults):
+    if defaults is None:
+        return None
+    if not isinstance(defaults, list):
+        return "workflow_defaults must be a list"
+    if len(defaults) > MAX_STRUCTURED_ITEMS_PER_SKILL:
+        return f"workflow_defaults must contain at most {MAX_STRUCTURED_ITEMS_PER_SKILL} entries"
+    for index, default in enumerate(defaults):
+        field = f"workflow_defaults[{index}]"
+        if not isinstance(default, dict):
+            return f"{field} must be a YAML dictionary"
+        unexpected = set(default) - {"name", "value", "description"}
+        if unexpected:
+            return f"Unexpected key(s) in {field}: {', '.join(sorted(unexpected))}"
+        name_error = validate_required_text(default.get("name"), MAX_STRUCTURED_ID_LEN, f"{field}.name")
+        if name_error:
+            return name_error
+        value_error = validate_required_text(default.get("value"), MAX_STRUCTURED_VALUE_LEN, f"{field}.value")
+        if value_error:
+            return value_error
+        desc_error = validate_optional_text(default.get("description"), MAX_STRUCTURED_PURPOSE_LEN, f"{field}.description")
+        if desc_error:
+            return desc_error
+    return None
+
+
+def validate_required_text(value, max_length, field):
+    if not isinstance(value, str) or not value.strip():
+        return f"{field} must be a non-empty string"
+    if len(value.strip()) > max_length:
+        return f"{field} is too long ({len(value.strip())} characters). Maximum is {max_length} characters."
+    return None
+
+
+def validate_optional_text(value, max_length, field):
+    if value is None:
+        return None
+    return validate_required_text(value, max_length, field)
+
+
+def validate_example_argv(argv, field):
+    if not isinstance(argv, list) or not argv:
+        return f"{field} must be a non-empty list"
+    if len(argv) > MAX_COMMAND_ARGV_TOKENS:
+        return f"{field} must contain at most {MAX_COMMAND_ARGV_TOKENS} tokens"
+    for index, token in enumerate(argv):
+        token_error = validate_required_text(token, MAX_COMMAND_TOKEN_LEN, f"{field}[{index}]")
+        if token_error:
+            return token_error
+    return None
+
+
+def validate_relative_path(value, field):
+    if not isinstance(value, str) or not value.strip():
+        return f"{field} must be a non-empty string"
+    path = Path(value.strip())
+    if path.is_absolute() or ".." in path.parts:
+        return f"{field} must be relative and must not contain '..'"
+    return None
+
+
 def main():
     if len(sys.argv) < 2:
         print("Usage: python quick_validate.py <skill_directory>")

code-rs/core/src/skills/loader.rs

@@ -497,7 +497,7 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result<SkillMetadata, Ski
     let resolved_path = normalize_path(path).unwrap_or_else(|_| path.to_path_buf());
     let skill_dir = resolved_path.parent().unwrap_or_else(|| Path::new("."));
     let resources = parse_skill_resources(parsed.resources, skill_dir)?;
-    let commands = parse_skill_commands(parsed.commands, skill_dir, scope, &resources)?;
+    let commands = parse_skill_commands(parsed.commands, skill_dir, &resources)?;
     let workflow_defaults = parse_workflow_defaults(parsed.workflow_defaults)?;
     let policy = parsed
         .policy
@@ -564,7 +564,6 @@ fn parse_skill_resources(
 fn parse_skill_commands(
     commands: Vec<SkillFrontmatterCommand>,
     skill_dir: &Path,
-    scope: SkillScope,
     declared_resources: &[SkillResource],
 ) -> Result<Vec<SkillCommand>, SkillParseError> {
     if commands.len() > MAX_STRUCTURED_ITEMS_PER_SKILL {
@@ -588,7 +587,6 @@ fn parse_skill_commands(
         let resource_path = parse_command_resource_path(
             &name,
             source,
-            scope,
             command.resource_path,
             skill_dir,
             declared_resources,
@@ -614,7 +612,6 @@ fn parse_skill_commands(
 fn parse_command_resource_path(
     name: &str,
     source: SkillCommandSource,
-    scope: SkillScope,
     resource_path: Option<PathBuf>,
     skill_dir: &Path,
     declared_resources: &[SkillResource],
@@ -652,12 +649,6 @@ fn parse_command_resource_path(
             Ok(Some(resolved_resource_path))
         }
         SkillCommandSource::Repo => {
-            if scope != SkillScope::Repo {
-                return Err(SkillParseError::InvalidField {
-                    field: "commands.source",
-                    reason: format!("command '{name}' uses source repo outside a repo skill"),
-                });
-            }
             if resource_path.is_some() {
                 return Err(SkillParseError::InvalidField {
                     field: "commands.resource_path",
@@ -1254,7 +1245,7 @@ workflow_defaults:
     }
 
     #[test]
-    fn repo_commands_require_repo_scope() {
+    fn repo_commands_can_be_declared_by_user_skills() {
         let skills_root = tempfile::tempdir().expect("tempdir");
         let skill_dir = skills_root.path().join("bad");
         fs::create_dir_all(&skill_dir).expect("create skill dir");
@@ -1280,15 +1271,15 @@ commands:
             scope: SkillScope::User,
         }]);
 
-        assert!(outcome.skills.is_empty());
-        assert_eq!(outcome.errors.len(), 1);
         assert!(
-            outcome.errors[0]
-                .message
-                .contains("uses source repo outside a repo skill"),
-            "unexpected error: {}",
-            outcome.errors[0].message
+            outcome.errors.is_empty(),
+            "unexpected errors: {:?}",
+            outcome.errors
         );
+        let skill = outcome.skills.first().expect("skill");
+        assert_eq!(skill.commands.len(), 1);
+        assert_eq!(skill.commands[0].source, SkillCommandSource::Repo);
+        assert_eq!(skill.commands[0].resource_path, None);
     }
 
     #[test]