From dff39dec7741af1880f43b92a3cea72de5a44d40 Mon Sep 17 00:00:00 2001 From: Huang-404-Q <129407632+Huang-404-Q@users.noreply.github.com> Date: Thu, 22 Oct 2026 09:24:04 +0800 Subject: [PATCH] fix(skills): align skill-creator validation with the runtime schema (#7180) * fix(skills): align skill-creator validation with the runtime schema quick_validate.py rejected skills the runtime accepts and accepted skills the runtime drops: - Read SKILL.md with utf-8-sig so a BOM-prefixed file (Windows editors) is not reported as missing frontmatter. - Allow the DeerFlow frontmatter fields (argument-hint, required-secrets, secrets-autonomous, version, author) that the runtime schema accepts. - Reject empty name/description, which the loader drops. - Validate required-secrets/secrets-autonomous shapes like the runtime validator, so a malformed declaration fails before packaging. Tests cover each case in backend/tests/test_skill_creator_quick_validate.py. * fix(skills): close the remaining skill-creator parity gaps - Read SKILL.md with utf-8-sig in utils.parse_skill_md too, so a BOM file that quick_validate now accepts does not fail in run_eval/run_loop/ improve_description. - Stringify YAML keys before the unexpected-key check, so mixed non-string keys report cleanly instead of raising TypeError in sorted(). - Accept trailing whitespace on the frontmatter delimiters, matching the runtime parser. Each case has a regression test that fails on the previous commit. * fix(skills): validate allowed-tools and stop over-rejecting compatibility The reviewer found two remaining parity gaps: - allowed-tools was not validated, while the runtime parser rejects non-string list items, empty tool names, unmatched closing parentheses and unclosed quotes. Mirror those shape rules. - compatibility was rejected for non-string or >500-char values, but the runtime validator never inspects it. Drop the check so this script does not reject skills the runtime accepts. Each case has a regression test that fails on the previous commit. * fix(skills): reject unclosed parenthesized allowed-tools patterns The runtime parser raises when the scan ends with depth > 0; the quick_validate loop only checked for an unmatched closing parenthesis. Add the depth check and move the unclosed-pattern case to the rejected tests. --- .../test_skill_creator_quick_validate.py | 152 +++++++++++++++++- .../skill-creator/scripts/quick_validate.py | 123 ++++++++++---- skills/public/skill-creator/scripts/utils.py | 4 +- 3 files changed, 247 insertions(+), 32 deletions(-) diff --git a/backend/tests/test_skill_creator_quick_validate.py b/backend/tests/test_skill_creator_quick_validate.py index 112a8ebc09..d1dfb00cf2 100644 --- a/backend/tests/test_skill_creator_quick_validate.py +++ b/backend/tests/test_skill_creator_quick_validate.py @@ -5,6 +5,8 @@ import importlib.util from pathlib import Path from types import SimpleNamespace +import pytest + REPO_ROOT = Path(__file__).resolve().parents[2] SKILL_CREATOR_ROOT = REPO_ROOT / "skills" / "public" / "skill-creator" SCRIPTS_DIR = SKILL_CREATOR_ROOT / "scripts" @@ -125,13 +127,18 @@ def test_skill_creator_text_io_declares_utf8() -> None: if not isinstance(node, ast.Call): continue + read_only = False if isinstance(node.func, ast.Attribute) and node.func.attr in {"read_text", "write_text"}: operation = node.func.attr + read_only = operation == "read_text" elif isinstance(node.func, ast.Name) and node.func.id == "open": mode = node.args[1] if len(node.args) > 1 else next((keyword.value for keyword in node.keywords if keyword.arg == "mode"), None) if isinstance(mode, ast.Constant) and isinstance(mode.value, str) and "b" in mode.value: continue operation = "open" + # ``utf-8-sig`` is the documented read encoding for SKILL.md + # (it strips a leading BOM); writes must stay plain UTF-8. + read_only = not (isinstance(mode, ast.Constant) and isinstance(mode.value, str) and any(flag in mode.value for flag in "wax+")) elif isinstance(node.func, ast.Attribute) and isinstance(node.func.value, ast.Name) and node.func.value.id == "subprocess": keywords = {keyword.arg: keyword.value for keyword in node.keywords if keyword.arg is not None} text_mode = any(isinstance(keywords.get(name), ast.Constant) and keywords[name].value is True for name in ("text", "universal_newlines")) @@ -142,8 +149,149 @@ def test_skill_creator_text_io_declares_utf8() -> None: continue encoding = next((keyword.value for keyword in node.keywords if keyword.arg == "encoding"), None) - if not isinstance(encoding, ast.Constant) or encoding.value != "utf-8": + allowed = {"utf-8", "utf-8-sig"} if read_only else {"utf-8"} + if not isinstance(encoding, ast.Constant) or encoding.value not in allowed: relative_path = script_path.relative_to(SKILL_CREATOR_ROOT) missing_encoding.append(f"{relative_path}:{node.lineno} {operation}") - assert not missing_encoding, f"text I/O must declare encoding='utf-8': {missing_encoding}" + assert not missing_encoding, f"text I/O must declare encoding='utf-8' (or 'utf-8-sig' for reads): {missing_encoding}" + + +def _write_skill(tmp_path: Path, content: str) -> Path: + skill_dir = tmp_path / "demo-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text(content, encoding="utf-8", newline="") + return skill_dir + + +def test_validate_skill_accepts_bom_prefixed_frontmatter(tmp_path: Path) -> None: + validator = _load_validator() + skill_dir = _write_skill( + tmp_path, + "---\nname: demo-skill\ndescription: Handles BOM-prefixed files.\n---\n\n# Demo\n", + ) + + assert validator.validate_skill(skill_dir) == (True, "Skill is valid!") + + +@pytest.mark.parametrize( + ("field", "message"), + [ + ("name", "Name cannot be empty"), + ("description", "Description cannot be empty"), + ], +) +def test_validate_skill_rejects_empty_required_fields(tmp_path: Path, field: str, message: str) -> None: + validator = _load_validator() + values = {"name": "demo-skill", "description": "Handles empty fields."} + values[field] = '""' + content = "---\n" + "\n".join(f"{key}: {value}" for key, value in values.items()) + "\n---\n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (False, message) + + +@pytest.mark.parametrize( + "extra", + [ + 'argument-hint: "[file]"', + "required-secrets:\n - API_KEY", + "secrets-autonomous: false", + "version: 1.0.0", + "author: someone", + ], +) +def test_validate_skill_accepts_runtime_frontmatter_fields(tmp_path: Path, extra: str) -> None: + validator = _load_validator() + content = f"---\nname: demo-skill\ndescription: Handles runtime fields.\n{extra}\n---\n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (True, "Skill is valid!") + + +@pytest.mark.parametrize( + ("extra", "message"), + [ + ("required-secrets: API_KEY", "required-secrets must be a list"), + ( + 'required-secrets:\n - name: API_KEY\n optional: "no"', + "required-secrets entry 'API_KEY' optional must be a boolean", + ), + ('secrets-autonomous: "yes"', "secrets-autonomous must be a boolean"), + ], +) +def test_validate_skill_rejects_malformed_runtime_fields(tmp_path: Path, extra: str, message: str) -> None: + validator = _load_validator() + content = f"---\nname: demo-skill\ndescription: Handles malformed fields.\n{extra}\n---\n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (False, message) + + +def test_validate_skill_accepts_trailing_whitespace_on_delimiters(tmp_path: Path) -> None: + validator = _load_validator() + content = "--- \nname: demo-skill\ndescription: Handles padded delimiters.\n--- \n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (True, "Skill is valid!") + + +def test_validate_skill_reports_mixed_non_string_keys_without_crashing(tmp_path: Path) -> None: + validator = _load_validator() + content = "---\nname: demo-skill\ndescription: Handles odd keys.\n42: bad\ncustom-field: bad\n---\n\n# Demo\n" + + valid, message = validator.validate_skill(_write_skill(tmp_path, content)) + + assert valid is False + assert "Unexpected key(s)" in message + assert "42" in message + assert "custom-field" in message + + +def test_shared_skill_parser_accepts_bom_prefixed_frontmatter(tmp_path: Path) -> None: + utils = _load_script("utils") + skill_dir = tmp_path / "demo-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: demo-skill\ndescription: Handles BOM-prefixed files.\n---\n\n# Demo\n", + encoding="utf-8", + newline="", + ) + + name, description, _ = utils.parse_skill_md(skill_dir) + + assert name == "demo-skill" + assert description == "Handles BOM-prefixed files." + + +@pytest.mark.parametrize( + ("allowed_tools", "message"), + [ + ("[Read, 42]", "allowed-tools must contain only strings"), + ('["Read", ""]', "allowed-tools cannot contain empty tool names"), + ('"Bash(grep))"', "allowed-tools contains an unmatched closing parenthesis"), + ('"Bash(\'grep)"', "allowed-tools contains an unclosed quote"), + ('"Bash(grep"', "allowed-tools contains an unclosed parenthesized pattern"), + ("42", "allowed-tools must be a space-separated string or list of strings"), + ], +) +def test_validate_skill_rejects_malformed_allowed_tools(tmp_path: Path, allowed_tools: str, message: str) -> None: + validator = _load_validator() + content = f"---\nname: demo-skill\ndescription: Handles tools.\nallowed-tools: {allowed_tools}\n---\n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (False, message) + + +@pytest.mark.parametrize( + "allowed_tools", + ['"Read Bash(grep)"', '["Read", "Bash(grep)"]'], +) +def test_validate_skill_accepts_well_formed_allowed_tools(tmp_path: Path, allowed_tools: str) -> None: + validator = _load_validator() + content = f"---\nname: demo-skill\ndescription: Handles tools.\nallowed-tools: {allowed_tools}\n---\n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (True, "Skill is valid!") + + +@pytest.mark.parametrize("compatibility", ["123", '"' + "x" * 501 + '"']) +def test_validate_skill_accepts_compatibility_the_runtime_accepts(tmp_path: Path, compatibility: str) -> None: + validator = _load_validator() + content = f"---\nname: demo-skill\ndescription: Handles compatibility.\ncompatibility: {compatibility}\n---\n\n# Demo\n" + + assert validator.validate_skill(_write_skill(tmp_path, content)) == (True, "Skill is valid!") diff --git a/skills/public/skill-creator/scripts/quick_validate.py b/skills/public/skill-creator/scripts/quick_validate.py index 218ce74744..0c21df69c5 100755 --- a/skills/public/skill-creator/scripts/quick_validate.py +++ b/skills/public/skill-creator/scripts/quick_validate.py @@ -17,16 +17,18 @@ def validate_skill(skill_path): if not skill_md.exists(): return False, "SKILL.md not found" - # Read and validate frontmatter + # Read and validate frontmatter. ``utf-8-sig`` decodes plain UTF-8 unchanged + # and strips the BOM that Windows editors prepend, matching the runtime parser. try: - content = skill_md.read_text(encoding="utf-8") + content = skill_md.read_text(encoding="utf-8-sig") except UnicodeDecodeError: return False, "SKILL.md is not valid UTF-8" if not content.startswith('---'): return False, "No YAML frontmatter found" - # Extract frontmatter - match = re.match(r'^---\n(.*?)\n---', content, re.DOTALL) + # Extract frontmatter. Allow trailing whitespace on the delimiters, like + # the runtime parser in backend/packages/harness/deerflow/skills/frontmatter.py. + match = re.match(r'^---[ \t]*\n(.*?)\n---[ \t]*(?:\n|$)', content, re.DOTALL) if not match: return False, "Invalid frontmatter format" @@ -37,11 +39,20 @@ def validate_skill(skill_path): frontmatter = yaml.safe_load(frontmatter_text) if not isinstance(frontmatter, dict): return False, "Frontmatter must be a YAML dictionary" + # YAML permits non-string keys; downstream validation expects strings. + frontmatter = {str(key): value for key, value in frontmatter.items()} except yaml.YAMLError as e: return False, f"Invalid YAML in frontmatter: {e}" - # Define allowed properties - ALLOWED_PROPERTIES = {'name', 'description', 'license', 'allowed-tools', 'metadata', 'compatibility'} + # Define allowed properties. Keep this in sync with the runtime schema in + # backend/packages/harness/deerflow/skills/frontmatter.py and the field + # rules in backend/packages/harness/deerflow/skills/validation.py: a skill + # that the runtime accepts must not be rejected here before packaging. + ALLOWED_PROPERTIES = { + 'name', 'description', 'license', 'allowed-tools', 'argument-hint', + 'required-secrets', 'secrets-autonomous', 'metadata', 'compatibility', + 'version', 'author', + } # Check for unexpected properties (excluding nested keys under metadata) unexpected_keys = set(frontmatter.keys()) - ALLOWED_PROPERTIES @@ -62,36 +73,90 @@ def validate_skill(skill_path): if not isinstance(name, str): return False, f"Name must be a string, got {type(name).__name__}" name = name.strip() - if name: - # Check naming convention (kebab-case: lowercase with hyphens) - if not re.match(r'^[a-z0-9-]+$', name): - return False, f"Name '{name}' should be kebab-case (lowercase letters, digits, and hyphens only)" - if name.startswith('-') or name.endswith('-') or '--' in name: - return False, f"Name '{name}' cannot start/end with hyphen or contain consecutive hyphens" - # Check name length (max 64 characters per spec) - if len(name) > 64: - return False, f"Name is too long ({len(name)} characters). Maximum is 64 characters." + if not name: + return False, "Name cannot be empty" + # Check naming convention (kebab-case: lowercase with hyphens) + if not re.match(r'^[a-z0-9-]+$', name): + return False, f"Name '{name}' should be kebab-case (lowercase letters, digits, and hyphens only)" + if name.startswith('-') or name.endswith('-') or '--' in name: + return False, f"Name '{name}' cannot start/end with hyphen or contain consecutive hyphens" + # Check name length (max 64 characters per spec) + if len(name) > 64: + return False, f"Name is too long ({len(name)} characters). Maximum is 64 characters." # Extract and validate description description = frontmatter.get('description', '') if not isinstance(description, str): return False, f"Description must be a string, got {type(description).__name__}" description = description.strip() - if description: - # Check for angle brackets - if '<' in description or '>' in description: - return False, "Description cannot contain angle brackets (< or >)" - # Check description length (max 1024 characters per spec) - if len(description) > 1024: - return False, f"Description is too long ({len(description)} characters). Maximum is 1024 characters." + if not description: + return False, "Description cannot be empty" + # Check for angle brackets + if '<' in description or '>' in description: + return False, "Description cannot contain angle brackets (< or >)" + # Check description length (max 1024 characters per spec) + if len(description) > 1024: + return False, f"Description is too long ({len(description)} characters). Maximum is 1024 characters." - # Validate compatibility field if present (optional) - compatibility = frontmatter.get('compatibility', '') - if compatibility: - if not isinstance(compatibility, str): - return False, f"Compatibility must be a string, got {type(compatibility).__name__}" - if len(compatibility) > 500: - return False, f"Compatibility is too long ({len(compatibility)} characters). Maximum is 500 characters." + # The runtime validator does not inspect `compatibility` (export.py only + # coerces it), so this script must not reject values the runtime accepts. + + # Validate allowed-tools with the same shape rules as the runtime parser + # (backend/packages/harness/deerflow/skills/parser.py). + allowed_tools = frontmatter.get('allowed-tools') + if allowed_tools is not None: + if isinstance(allowed_tools, str): + depth = 0 + quote = None + escaped = False + for char in allowed_tools: + if escaped: + escaped = False + continue + if char == '\\': + escaped = True + continue + if quote is not None: + if char == quote: + quote = None + continue + if char in {'"', "'"}: + quote = char + continue + if char == '(': + depth += 1 + elif char == ')': + if depth == 0: + return False, "allowed-tools contains an unmatched closing parenthesis" + depth -= 1 + if quote is not None: + return False, "allowed-tools contains an unclosed quote" + if depth > 0: + return False, "allowed-tools contains an unclosed parenthesized pattern" + elif isinstance(allowed_tools, list): + for item in allowed_tools: + if not isinstance(item, str): + return False, "allowed-tools must contain only strings" + if not item.strip(): + return False, "allowed-tools cannot contain empty tool names" + else: + return False, "allowed-tools must be a space-separated string or list of strings" + + # Validate the DeerFlow-specific declarations with the same shapes the + # runtime validator enforces, so a malformed skill fails before packaging. + required_secrets = frontmatter.get('required-secrets') + if required_secrets is not None and not isinstance(required_secrets, list): + return False, "required-secrets must be a list" + if required_secrets is not None: + for item in required_secrets: + if isinstance(item, dict) and not isinstance(item.get('optional', False), bool): + if item.get('name') is None: + return False, "required-secrets entry without a name has an optional field that must be a boolean" + return False, f"required-secrets entry {item.get('name')!r} optional must be a boolean" + + secrets_autonomous = frontmatter.get('secrets-autonomous') + if secrets_autonomous is not None and not isinstance(secrets_autonomous, bool): + return False, "secrets-autonomous must be a boolean" return True, "Skill is valid!" diff --git a/skills/public/skill-creator/scripts/utils.py b/skills/public/skill-creator/scripts/utils.py index 3676913bbe..c1a09c12b4 100644 --- a/skills/public/skill-creator/scripts/utils.py +++ b/skills/public/skill-creator/scripts/utils.py @@ -6,7 +6,9 @@ from pathlib import Path def parse_skill_md(skill_path: Path) -> tuple[str, str, str]: """Parse a SKILL.md file, returning (name, description, full_content).""" - content = (skill_path / "SKILL.md").read_text(encoding="utf-8") + # utf-8-sig strips the BOM that Windows editors prepend, matching the + # runtime parser and quick_validate.py. + content = (skill_path / "SKILL.md").read_text(encoding="utf-8-sig") lines = content.split("\n") if lines[0].strip() != "---":