mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
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.
This commit is contained in:
Huang-404-Q
GitHub
parent
1794c40888
commit
dff39dec77
@ -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!")
|
||||
|
||||
@ -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!"
|
||||
|
||||
|
||||
@ -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() != "---":
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user