feat(tools): enhance skill validator with tag quality, workflow, prereq, and safety checks - #88
feat(tools): enhance skill validator with tag quality, workflow, prereq, and safety checks#88KennyUMN wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Enhances tools/validate-skill.py from a frontmatter-only validator into a broader “skill quality” validator, adding checks for tag quality, workflow completeness, prerequisite usage consistency, and high-risk safety/authorization language, plus new CLI flags to select checks and enforce strictness.
Changes:
- Added new quality checks (
tags,body,prereqs,safety) and CLI flags (--check,--strict) totools/validate-skill.py. - Modified the frontmatter parser to avoid nested-YAML key collisions.
- Updated
tools/README.mdto document the new checks and flags.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/validate-skill.py | Adds new validation checks + CLI options; updates frontmatter parsing logic. |
| tools/README.md | Documents the new validator capabilities and CLI flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Only process top-level (indent=0) lines as frontmatter fields. | ||
| # Indented lines belong to nested YAML structures (e.g. mitre_f3:) | ||
| # which we don't parse — skip them entirely to avoid corrupting | ||
| # top-level fields like name: with nested key values. | ||
| if line.startswith(" ") or line.startswith("\t"): | ||
| continue | ||
|
|
||
| # Handle list items (indent=0, starts with "- ") | ||
| if stripped.startswith("- ") and current_key: | ||
| list_values.append(stripped[2:].strip().strip('"').strip("'")) | ||
| data[current_key] = list(list_values) # copy so future mutations don't leak | ||
| data[current_key] = list(list_values) | ||
| continue |
| _LIB_ALIASES = { | ||
| "boto3": ["boto3"], | ||
| "sslyze": ["sslyze"], | ||
| "msal": ["msal"], | ||
| "requests": ["requests", "request"], | ||
| "python-evtx": ["python_evtx", "python-evtx", "Evtx"], |
| # Get the workflow body (everything after Prerequisites section) | ||
| # to avoid matching the prerequisite list itself | ||
| workflow_body = [] | ||
| past_prereq = False | ||
| for line in body.split("\n"): | ||
| if re.match(r"^##\s+prereq", line, re.IGNORECASE): | ||
| past_prereq = True | ||
| continue | ||
| if past_prereq and re.match(r"^##\s+", line): | ||
| past_prereq = False | ||
| if not past_prereq: | ||
| workflow_body.append(line) | ||
|
|
||
| workflow_text = "\n".join(workflow_body) |
mukul975
left a comment
There was a problem hiding this comment.
Thanks for extending the validator with the tag-quality, workflow, prereq, and safety checks (#86) — these are useful additions. Two things before merge:
- Rebase on main. PR #87 (the nested-name misparse fix) has now been merged into
tools/validate-skill.py— the same file this PR rewrites. Please rebase onto current main so your additive checks layer on top of the merged fix rather than reverting it. Right now they will conflict. - After rebase, a few review points:
- Tag-quality stopword check: confirm the false-positive surface (legit tags that happen to contain a flagged word) is acceptable.
- Python 3.9 compatibility: no walrus in comprehensions, no
match/case, noX | Nonehints — the repo targets 3.9. - Make sure the new checks don't duplicate what the merged #87 validator already does.
Note: there are competing validator PRs (#91 proposes a separate skill_quality_validator.py). The repo should converge on one validator — extending the existing validate-skill.py here is the preferred path, so this PR is the one to carry forward once rebased.
…eq, and safety checks Extends tools/validate-skill.py with four new quality checks requested in mukul975#86: 1. Tag quality — flags generic stop-words and filename-split tags that provide no agent routing value (e.g. 'analyzing', 'with', 'block') 2. Workflow completeness — verifies presence of required sections (workflow/instructions/steps) and recommends When to Use, Prerequisites, and Output sections; flags potential stub workflows (code prerequisites listed but no code blocks present) 3. Prerequisite consistency — checks that libraries/tools listed in Prerequisites actually appear in the workflow body 4. Safety gates — flags high-risk skills (red team, pentest, malware, credential access, phishing, C2, exploit) that lack authorization, scope, or legal-notice language Also fixes a frontmatter parser bug where nested YAML structures (e.g. mitre_f3:) could corrupt top-level fields like name: by overwriting them with nested key values. New CLI flags: --check tags,body,prereqs,safety (select specific checks) --strict (treat warnings as errors) Refs mukul975#86
c9a79ac to
610f38b
Compare
KennyUMN
left a comment
There was a problem hiding this comment.
Rebased onto current main — all three conflict points addressed:
-
Rebase done. The branch now sits cleanly on top of current main (including the nested-name misparse fix from #87).
-
Conflict resolution details:
REQUIRED_FIELDSnow includesversion,author, andlicenseadded by #87.- The duplicate indent-guard that #87 added to
parse_frontmatterwas dropped from our version since the equivalent check already exists earlier in the same loop body — no functional change, single clear guard. - Our new
validate_skill()andmain()functions incorporate the.bakdirectory skip from #87'smain().
-
Tag-quality false positives: The stopword list flags words like
forensics,analysis,detectionwhen they appear as standalone tags with no domain specificity. We're comfortable with this surface — a tag likeforensicsgenuinely provides no routing value compared todigital-forensicsormemory-forensics. The list is conservative enough that no real routing tags are flagged. -
Python 3.9 compatibility: Confirmed — no walrus operators, no
match/case, noX | Nonetype union hints. The file uses stdlib only. -
No duplication with #87: The merged validator does only frontmatter field presence + subdomain checks. Our additions (tag quality, workflow completeness, prereq consistency, safety gates) are entirely new check functions that run after the base frontmatter checks.
What
Extends
tools/validate-skill.pywith four new quality checks requested in #86:Tag quality — flags generic stop-words and filename-split tags that provide no agent routing value (e.g.
analyzing,with,block,logs). These are words split from the skill filename rather than meaningful cybersecurity discovery terms — the exact bugs identified in the NLPM audit: 7 mechanical bugs found (tags, stub workflows) #49 NLPM audit.Workflow completeness — verifies presence of required sections (workflow/instructions/steps) and recommends When to Use, Prerequisites, and Output sections. Also includes stub detection: flags skills that list code-based prerequisites (e.g.
pip install boto3) but have no code blocks in the workflow body.Prerequisite consistency — extracts library/tool names from the Prerequisites section and checks whether each listed library actually appears in the workflow body. Warns when a prerequisite is declared but never used.
Safety gates — identifies high-risk skills (red team, pentest, malware, credential access, phishing simulation, C2, exploit) by subdomain, tag, and name keywords. Flags those that lack authorization, scope, or legal-notice language.
Why
Issue #86 explicitly requests these checks. Issue #49 (NLPM audit) found 7 mechanical bugs across the skill catalog — word-split tags, stub workflows with prerequisites listed but no code — that a validator with these checks would have caught before merge. The existing validator only checked frontmatter field presence; it could not detect these quality issues.
Changes
tools/validate-skill.py— extended with new check functions, new CLI flags (--check,--strict), and a frontmatter parser fix for nested YAML structurestools/README.md— updated documentation for new checks and CLI flagsFrontmatter parser fix
The parser had a bug where nested YAML structures (e.g.
mitre_f3:with nestedname:keys) would corrupt top-level fields — a nestedname: 'Account Manipulation: Account Linking'would overwrite the skill's actualname:field. Fixed by only processing top-level (indent=0) lines as frontmatter fields.Testing
Validated against 16 sample skills covering high-quality skills, stub skills from issue #49, high-risk skills with authorization language, and skills with nested YAML frontmatter:
Results match expectations:
New CLI flags
Refs #86