mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
Python: skill name validation improvements (#4530)
* Initial plan * Port .NET validation improvements to Python skills: reject consecutive hyphens and enforce directory name match Co-authored-by: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com> * Fix E501 lint error: split long error message string in _validate_skill_metadata Co-authored-by: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
913397492f
commit
b89adb280b
@@ -486,8 +486,8 @@ YAML_KV_RE = re.compile(
|
||||
)
|
||||
|
||||
# Validates skill names: lowercase letters, numbers, hyphens only;
|
||||
# must not start or end with a hyphen.
|
||||
VALID_NAME_RE = re.compile(r"^[a-z0-9]([a-z0-9\-]*[a-z0-9])?$")
|
||||
# must not start or end with a hyphen, and must not contain consecutive hyphens.
|
||||
VALID_NAME_RE = re.compile(r"^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$")
|
||||
|
||||
# Default system prompt template for advertising available skills to the model.
|
||||
# Use {skills} as the placeholder for the generated skills XML list.
|
||||
@@ -1156,7 +1156,8 @@ def _validate_skill_metadata(
|
||||
if len(name) > MAX_NAME_LENGTH or not VALID_NAME_RE.match(name):
|
||||
return (
|
||||
f"Skill from '{source}' has an invalid name '{name}': Must be {MAX_NAME_LENGTH} characters or fewer, "
|
||||
"using only lowercase letters, numbers, and hyphens, and must not start or end with a hyphen."
|
||||
"using only lowercase letters, numbers, and hyphens, and must not start or end with a hyphen "
|
||||
"or contain consecutive hyphens."
|
||||
)
|
||||
|
||||
if not description or not description.strip():
|
||||
@@ -1241,6 +1242,17 @@ def _read_and_parse_skill_file(
|
||||
return None
|
||||
|
||||
name, description = result
|
||||
|
||||
dir_name = Path(skill_dir_path).name
|
||||
if name != dir_name:
|
||||
logger.error(
|
||||
"SKILL.md at '%s' has frontmatter name '%s' that does not match the directory name '%s'; skipping.",
|
||||
skill_file,
|
||||
name,
|
||||
dir_name,
|
||||
)
|
||||
return None
|
||||
|
||||
return name, description, content
|
||||
|
||||
|
||||
|
||||
@@ -296,6 +296,15 @@ class TestDiscoverAndLoadSkills:
|
||||
skills = _discover_file_skills([str(tmp_path)])
|
||||
assert len(skills) == 0
|
||||
|
||||
def test_skips_skill_with_name_directory_mismatch(self, tmp_path: Path) -> None:
|
||||
skill_dir = tmp_path / "wrong-dir-name"
|
||||
skill_dir.mkdir()
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: actual-skill-name\ndescription: A skill.\n---\nBody.", encoding="utf-8"
|
||||
)
|
||||
skills = _discover_file_skills([str(tmp_path)])
|
||||
assert len(skills) == 0
|
||||
|
||||
def test_deduplicates_skill_names(self, tmp_path: Path) -> None:
|
||||
dir1 = tmp_path / "dir1"
|
||||
dir2 = tmp_path / "dir2"
|
||||
@@ -904,6 +913,11 @@ class TestSkill:
|
||||
provider = SkillsProvider(skills=[invalid_skill])
|
||||
assert len(provider._skills) == 0
|
||||
|
||||
def test_name_with_consecutive_hyphens_skipped(self) -> None:
|
||||
invalid_skill = Skill(name="consecutive--hyphens", description="A skill.", content="Body")
|
||||
provider = SkillsProvider(skills=[invalid_skill])
|
||||
assert len(provider._skills) == 0
|
||||
|
||||
def test_name_too_long_skipped(self) -> None:
|
||||
invalid_skill = Skill(name="a" * 65, description="A skill.", content="Body")
|
||||
provider = SkillsProvider(skills=[invalid_skill])
|
||||
@@ -1421,6 +1435,11 @@ class TestValidateSkillMetadata:
|
||||
assert result is not None
|
||||
assert "invalid name" in result
|
||||
|
||||
def test_name_with_consecutive_hyphens(self) -> None:
|
||||
result = _validate_skill_metadata("consecutive--hyphens", "desc", "source")
|
||||
assert result is not None
|
||||
assert "invalid name" in result
|
||||
|
||||
def test_single_char_name(self) -> None:
|
||||
assert _validate_skill_metadata("a", "desc", "source") is None
|
||||
|
||||
@@ -1526,6 +1545,15 @@ class TestReadAndParseSkillFile:
|
||||
result = _read_and_parse_skill_file(str(skill_dir))
|
||||
assert result is None
|
||||
|
||||
def test_name_directory_mismatch_returns_none(self, tmp_path: Path) -> None:
|
||||
skill_dir = tmp_path / "wrong-dir-name"
|
||||
skill_dir.mkdir()
|
||||
(skill_dir / "SKILL.md").write_text(
|
||||
"---\nname: actual-skill-name\ndescription: A skill.\n---\nBody.", encoding="utf-8"
|
||||
)
|
||||
result = _read_and_parse_skill_file(str(skill_dir))
|
||||
assert result is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests: _create_resource_element
|
||||
|
||||
Reference in New Issue
Block a user