Skip to content

Python: skill name validation improvements#4530

Merged
moonbox3 merged 4 commits intomainfrom
copilot/port-validation-improvements-to-python
Apr 13, 2026
Merged

Python: skill name validation improvements#4530
moonbox3 merged 4 commits intomainfrom
copilot/port-validation-improvements-to-python

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 6, 2026

  • Update VALID_NAME_RE to reject consecutive hyphens
  • Update error message in _validate_skill_metadata to mention consecutive hyphens (split to stay within 120-char line limit)
  • In _read_and_parse_skill_file, enforce frontmatter name matches parent directory name
  • Add tests: consecutive hyphens rejected
  • Add tests: directory name mismatch rejected
  • Fix E501 lint error (line too long)
Original prompt

Create a PR in microsoft/agent-framework that ports the validation improvements from .NET PR #4526 to the Python agent skills implementation.

Context

Required behavior changes (Python)

  1. Skill name validation must reject consecutive hyphens
  • Update VALID_NAME_RE from the current pattern that allows consecutive hyphens to a pattern that disallows them (equivalent to .NET regex: ^[a-z0-9]([a-z0-9]*-[a-z0-9])*[a-z0-9]*$).
  • Update the corresponding validation error message in _validate_skill_metadata to mention that consecutive hyphens are not allowed.
  1. File-based skills: enforce that the name: from SKILL.md frontmatter matches the parent directory name
  • Implement this check in the file-based skill loading path after frontmatter extraction (best location: _read_and_parse_skill_file(skill_dir_path)), because it has access to the skill directory path.
  • On mismatch: log an error similar to .NET (include SKILL.md path, frontmatter name, directory name) and exclude the skill (return None).
  • For code-defined skills (created in Python code), do not apply directory matching.
  1. Tests
  • Add/extend Python unit tests to cover:
    • invalid skill names with consecutive hyphens (e.g. consecutive--hyphens) are rejected.
    • directory name mismatch (folder wrong-dir-name containing SKILL.md with frontmatter name: actual-skill-name) is rejected.
  • Keep tests consistent with existing repo testing patterns.

Notes

  • Ensure behavior remains case-sensitive for directory name match (align with .NET’s ordinal comparison).
  • Keep existing behavior for other invalid names (leading/trailing hyphen, spaces, etc.).
  • Run/adjust formatting/linting if the repo enforces it.

Files likely to change

  • python/packages/core/agent_framework/_skills.py
  • Python test file(s) that cover skills discovery/validation (discover via repo search).

Acceptance criteria

  • Python skill discovery rejects consecutive-hyphen names.
  • Python skill discovery rejects SKILL.md whose name: does not match the containing directory.
  • New/updated unit tests pass.

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…e hyphens and enforce directory name match

Co-authored-by: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com>
Copilot AI changed the title [WIP] Update Python agent skills validation for skill names Port .NET skill name validation improvements to Python: reject consecutive hyphens and enforce directory/name match Mar 6, 2026
@SergeyMenshykh SergeyMenshykh changed the title Port .NET skill name validation improvements to Python: reject consecutive hyphens and enforce directory/name match Python: skill name validation improvements Mar 6, 2026
@SergeyMenshykh
Copy link
Copy Markdown
Member

@copilot fix the failed check:

FAILED: lint in packages/core
Poe => ruff check
E501 Line too long (137 > 120)
--> agent_framework/_skills.py:735:121
|
733 | …
734 | …': Must be {MAX_NAME_LENGTH} characters or fewer, "
735 | …, and must not start or end with a hyphen or contain consecutive
hyphens."
|
^^^^^^^^^^^^^^^^^
736 | …
|

Found 1 error.

…ll_metadata

Co-authored-by: SergeyMenshykh <68852919+SergeyMenshykh@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot fix the failed check:

FAILED: lint in packages/core
...

Fixed in 73bb51b — split the long string literal across two lines to stay within the 120-character limit.

@SergeyMenshykh SergeyMenshykh marked this pull request as ready for review April 13, 2026 12:20
Copilot AI review requested due to automatic review settings April 13, 2026 12:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Ports the skill validation improvements from the .NET implementation into the Python skills loader to tighten name rules and prevent misconfigured file-based skills from being discovered.

Changes:

  • Updated skill name validation to reject consecutive hyphens.
  • Improved _validate_skill_metadata error message to explicitly mention consecutive hyphens.
  • Enforced SKILL.md frontmatter name: to match the containing directory name for file-based skills, with tests covering both new validations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
python/packages/core/agent_framework/_skills.py Tightens VALID_NAME_RE, updates validation error text, and adds directory-name matching enforcement during file-based skill load.
python/packages/core/tests/core/test_skills.py Adds unit tests ensuring consecutive-hyphen names and directory/name mismatches are skipped/rejected.

@moonbox3
Copy link
Copy Markdown
Contributor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/core/agent_framework
   _skills.py4161596%176, 888, 890–892, 946, 985–986, 1059, 1064, 1113, 1118, 1282–1283, 1531
TOTAL27204318488% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
5443 20 💤 0 ❌ 0 🔥 1m 23s ⏱️

@eavanvalkenburg
Copy link
Copy Markdown
Member

@SergeyMenshykh is this still relevant?

@SergeyMenshykh
Copy link
Copy Markdown
Member

@SergeyMenshykh is this still relevant?

Yep, it's still needed, even though the code will be extracted to a separate class in one of the next PRs.

@SergeyMenshykh SergeyMenshykh added this pull request to the merge queue Apr 13, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 13, 2026
@moonbox3 moonbox3 added this pull request to the merge queue Apr 13, 2026
Merged via the queue into main with commit b89adb2 Apr 13, 2026
31 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in Agent Framework Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants