feat: add validation rules for reserved words, XML tags, and type checks #54
Conversation
Add missing validation rules per Agent Skills specification: - Reserved word check: name cannot contain 'anthropic' or 'claude' - XML tag check: name and description cannot contain XML-like tags - Type check: name and description must be strings (detects YAML type coercion) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds three new validation rules to the skillport validate command to enforce Anthropic's Agent Skills specification requirements: reserved word checking (prevents names containing "anthropic" or "claude"), XML tag detection (prevents XML-like tags in name and description fields), and type validation (ensures name and description are strings in frontmatter).
Key Changes:
- Added reserved word validation to prevent skill names from containing "anthropic" or "claude"
- Added XML tag detection using regex pattern to prevent injection attacks
- Added type checking to detect YAML type coercion issues (e.g.,
name: yesbecomingTrue)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/skillport/modules/skills/internal/validation.py | Implements reserved word detection, XML tag pattern matching, and type validation for name/description fields |
| tests/unit/test_validation_rules.py | Adds unit tests for reserved words, XML tags, and type validation edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Return the first reserved word found in name, or None.""" | ||
| name_lower = name.lower() | ||
| for word in RESERVED_WORDS: | ||
| if word in name_lower: | ||
| return word |
There was a problem hiding this comment.
The reserved word detection uses substring matching, which will produce false positives. For example, a skill named "anthropology-101" would be incorrectly rejected because it contains "anthropic" as a substring. Consider using word boundary matching instead, such as using regex with \b word boundaries or splitting on hyphens and checking each component.
| """Return the first reserved word found in name, or None.""" | |
| name_lower = name.lower() | |
| for word in RESERVED_WORDS: | |
| if word in name_lower: | |
| return word | |
| """Return the first reserved word found in name, or None. | |
| Reserved words are matched against hyphen-separated components of the name | |
| (rather than arbitrary substrings) to avoid false positives such as | |
| "anthropology-101" containing "anthropic" as a substring. | |
| """ | |
| normalized_lower = unicodedata.normalize("NFKC", name).lower() | |
| for part in normalized_lower.split("-"): | |
| if part and part in RESERVED_WORDS: | |
| return part |
| def test_name_reserved_word_case_insensitive(self): | ||
| """Reserved word check should be case-insensitive.""" | ||
| # Note: name validation already fails on uppercase, but reserved check is independent | ||
| issues = validate_skill_record( | ||
| {"name": "my-skill", "description": "desc", "path": "/skills/my-skill"} | ||
| ) | ||
| reserved_issues = [i for i in issues if "reserved" in i.message.lower()] | ||
| assert len(reserved_issues) == 0 |
There was a problem hiding this comment.
This test doesn't actually verify case-insensitive reserved word detection. The name "my-skill" contains no reserved words (neither "anthropic" nor "claude"), so this test would pass even if the case-insensitive logic was broken. To properly test case insensitivity, use a name like "my-Anthropic-skill" or "my-CLAUDE-tool" and verify it triggers the reserved word check.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _contains_xml_tags(description): | ||
| issues.append( | ||
| ValidationIssue( | ||
| severity="fatal", | ||
| message="frontmatter.description: cannot contain XML tags", |
There was a problem hiding this comment.
If a SKILL.md frontmatter uses a non-string description (e.g., YAML list such as description: [item1, item2]), validate_skill_record now appends the new "must be a string" fatal but then immediately calls _contains_xml_tags(description) on lines 217-221, which invokes re.search on a list and raises TypeError: expected string or bytes-like object. The validation command (skillport validate path loader passes the raw meta as description) will crash before returning issues, so the new type-check rule never surfaces and the CLI aborts on this input.
Useful? React with 👍 / 👎.
Prevent TypeError when name/description are non-string types (e.g., YAML list or boolean). The XML tag check and other string operations now only run when the value is actually a string. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@codex |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if name and isinstance(name, str): | ||
| if len(name) > NAME_MAX_LENGTH: | ||
| issues.append( | ||
| ValidationIssue( |
There was a problem hiding this comment.
The new string-type enforcement is only applied in the meta branch, and the main validation block now runs only when isinstance(name, str). Calls without meta (e.g., validate_skill and skillport validate --id, which pass index records with no _meta and often no path) will treat non-string fields like name=True or description=['desc'] as valid because they bypass this guarded block, so the intended "must be a string" rule is never raised in those flows. This lets YAML type-coerced data pass validation whenever meta is unavailable.
Useful? React with 👍 / 👎.
Move type checks from meta-only block to always run on skill values. This ensures non-string name/description are detected even when validating via index (without meta). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Required fields (value checks) - only check if type is valid | ||
| if not name and name_is_str: | ||
| # Empty string | ||
| issues.append( | ||
| ValidationIssue(severity="fatal", message="frontmatter.name: missing", field="name") |
There was a problem hiding this comment.
In validation.py, the required-field checks now run only when name_is_str/desc_is_str is true (if not name and name_is_str etc.), while the new type checks above are only executed for truthy values. As a result, frontmatter like name: null, name: [], or description: false (falsy non-strings) bypass both the type check and the missing check and return no fatal issues, letting invalid skills pass validation—previous logic would have flagged them as missing. This affects any validation run on SKILL.md data containing YAML null/empty collection values.
Useful? React with 👍 / 👎.
Restructure type/required checks using if/elif to ensure falsy non-string values like null, empty list, or False are caught as type errors instead of silently passing validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@codex |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Description & Motivation
Add missing validation rules to
skillport validatecommand per Agent Skills specification. These rules ensure SKILL.md frontmatter follows Anthropic's official requirements.Changes
namefield cannot contain 'anthropic' or 'claude' (prevents confusion with official skills)nameanddescriptionfields cannot contain XML-like tags (<tag>,</tag>) for securitynameanddescriptionmust be strings (detects YAML type coercion likename: yes→True)How to Test
Checklist