From fce314eabd8674d81f05c2cec3263366eeaff874 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:45:25 -0700 Subject: [PATCH] feat(skills): advisory SKILL.md convention linter on create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds tools/skill_linter.py — a soft companion to the hard frontmatter validator. It encodes the CONTRIBUTING 'Skill authoring standards (HARDLINE)' conventions that today only a human reviewer catches: - shell-utility references in prose (`grep`/`sed`/`cat`...) that should name the native tool (search_files/patch/read_file) - missing version/author/license/metadata.hermes block - name != directory, invalid name format - description over the 60-char prompt budget, marketing words - dangling references/ links, forbidden scaffolding files - POSIX-only script primitives without a platforms: gate Findings are ADVISORY. skill_manage(create) attaches them as lint_warnings + lint_hint in the success result; nothing is blocked (the hard rejects already run in _validate_frontmatter). A CLI (python -m tools.skill_linter ) exits 1 only on ERROR-severity findings so CI can gate on structural breakage without failing on nits. Calibrated against the bundled skills/ tree: 76 advisory findings, exit 0, no false positives after excluding repo-root scripts/ refs. Inspired by MiniMax Code's skill-creator lint step; adapted to our existing validator + skill_utils rather than a parallel system. --- tests/tools/test_skill_linter.py | 189 +++++++++++++ tools/skill_linter.py | 462 +++++++++++++++++++++++++++++++ tools/skill_manager_tool.py | 29 ++ 3 files changed, 680 insertions(+) create mode 100644 tests/tools/test_skill_linter.py create mode 100644 tools/skill_linter.py diff --git a/tests/tools/test_skill_linter.py b/tests/tools/test_skill_linter.py new file mode 100644 index 0000000000000..b4c03692b2018 --- /dev/null +++ b/tests/tools/test_skill_linter.py @@ -0,0 +1,189 @@ +"""Tests for tools/skill_linter.py — the advisory SKILL.md convention linter.""" + +from pathlib import Path + +import pytest + +from tools.skill_linter import ( + ERROR, + WARNING, + format_findings, + has_errors, + lint_content, + lint_skill, +) + +# A clean, peer-shaped SKILL.md that should produce zero findings. +CLEAN = """--- +name: my-skill +description: Search arXiv papers by keyword, author, or ID. +version: 1.0.0 +author: Hermes Agent +license: MIT +metadata: + hermes: + tags: [arxiv, research] + related_skills: [] +--- + +# My Skill + +## Overview +Does a thing. + +## When to Use +- When the user wants X. + +## Procedure +1. Use `read_file` to load it. +""" + + +def _rules(findings): + return {f.rule for f in findings} + + +def test_clean_skill_has_no_findings(): + assert lint_content(CLEAN) == [] + + +def test_description_too_long_is_warning(): + long_desc = "x" * 80 + content = CLEAN.replace( + "Search arXiv papers by keyword, author, or ID.", long_desc + ) + findings = lint_content(content) + assert "description-length" in _rules(findings) + assert all(f.severity == WARNING for f in findings) + + +def test_marketing_words_flagged(): + content = CLEAN.replace( + "Search arXiv papers by keyword, author, or ID.", + "A powerful comprehensive tool.", + ) + findings = lint_content(content) + assert "description-marketing" in _rules(findings) + + +def test_shell_utility_reference_in_prose_flagged(): + content = CLEAN.replace("Use `read_file` to load it.", "Use `grep` to find it.") + findings = lint_content(content) + assert "shell-utility-reference" in _rules(findings) + + +def test_shell_utility_inside_code_block_not_flagged(): + # A fenced code block legitimately shows grep; prose check must skip it. + content = CLEAN + "\n```bash\ngrep -r foo .\n```\n" + findings = lint_content(content) + assert "shell-utility-reference" not in _rules(findings) + + +def test_missing_metadata_block_warns(): + content = """--- +name: bare-skill +description: Does a thing briefly. +--- + +# Bare Skill + +## When to Use +- now +""" + findings = lint_content(content) + rules = _rules(findings) + assert "missing-metadata" in rules + + +def test_missing_when_to_use_section_warns(): + content = CLEAN.replace("## When to Use\n- When the user wants X.\n", "") + findings = lint_content(content) + assert "missing-section" in _rules(findings) + + +def test_bad_name_format_is_error(): + content = CLEAN.replace("name: my-skill", "name: My_Skill!") + findings = lint_content(content) + assert "name-format" in _rules(findings) + assert has_errors(findings) + + +def test_name_dir_mismatch_is_error(tmp_path): + skill_dir = tmp_path / "actual-dir" + skill_dir.mkdir() + findings = lint_content(CLEAN, skill_dir=skill_dir) # name is my-skill + assert "name-dir-mismatch" in _rules(findings) + assert has_errors(findings) + + +def test_dangling_reference_link_flagged(tmp_path): + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + content = CLEAN + "\nSee references/missing.md for detail.\n" + findings = lint_content(content, skill_dir=skill_dir) + assert "dangling-reference" in _rules(findings) + + +def test_present_reference_link_not_flagged(tmp_path): + skill_dir = tmp_path / "my-skill" + (skill_dir / "references").mkdir(parents=True) + (skill_dir / "references" / "detail.md").write_text("x") + content = CLEAN + "\nSee references/detail.md for detail.\n" + findings = lint_content(content, skill_dir=skill_dir) + assert "dangling-reference" not in _rules(findings) + + +def test_posix_primitive_without_platforms_warns(tmp_path): + skill_dir = tmp_path / "my-skill" + (skill_dir / "scripts").mkdir(parents=True) + (skill_dir / "scripts" / "run.py").write_text("import fcntl\nfcntl.flock(1, 2)\n") + findings = lint_content(CLEAN, skill_dir=skill_dir) + assert "platforms-gating" in _rules(findings) + + +def test_posix_primitive_with_platforms_ok(tmp_path): + skill_dir = tmp_path / "my-skill" + (skill_dir / "scripts").mkdir(parents=True) + (skill_dir / "scripts" / "run.py").write_text("import fcntl\n") + content = CLEAN.replace( + "version: 1.0.0", "version: 1.0.0\nplatforms: [linux, macos]" + ) + findings = lint_content(content, skill_dir=skill_dir) + assert "platforms-gating" not in _rules(findings) + + +def test_forbidden_file_flagged(tmp_path): + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + (skill_dir / "README.md").write_text("# readme") + findings = lint_content(CLEAN, skill_dir=skill_dir) + assert "forbidden-file" in _rules(findings) + + +def test_invalid_platforms_value_warns(): + content = CLEAN.replace( + "version: 1.0.0", "version: 1.0.0\nplatforms: [linux, solaris]" + ) + findings = lint_content(content) + assert "platforms-value" in _rules(findings) + + +def test_lint_skill_reads_from_disk(tmp_path): + skill_dir = tmp_path / "my-skill" + skill_dir.mkdir() + skill_md = skill_dir / "SKILL.md" + skill_md.write_text(CLEAN) + findings = lint_skill(skill_md) + assert findings == [] + + +def test_author_caps_warned(): + content = CLEAN.replace("author: Hermes Agent", "author: hermes agent") + findings = lint_content(content) + assert "author-caps" in _rules(findings) + + +def test_format_findings_renders(): + findings = lint_content(CLEAN.replace("name: my-skill", "name: BAD")) + out = format_findings(findings) + assert "name-format" in out diff --git a/tools/skill_linter.py b/tools/skill_linter.py new file mode 100644 index 0000000000000..f838547bf8540 --- /dev/null +++ b/tools/skill_linter.py @@ -0,0 +1,462 @@ +"""Structural + convention linter for SKILL.md files. + +The hard *validator* in ``tools/skill_manager_tool.py::_validate_frontmatter`` +guards the non-negotiables (fence present, YAML mapping, ``name`` + +``description`` present, description length, non-empty body, size cap) and is a +create/edit BLOCKER. This module is the softer, broader companion: it encodes +the "Skill authoring standards (HARDLINE)" conventions from ``CONTRIBUTING.md`` +that today are only caught by a human reviewer — shell-utility references +instead of native tools, a missing author/license/metadata block, a +``name`` that doesn't match its directory, dangling ``references/`` links, +marketing words in the description, ``platforms:`` gating vs POSIX-only +primitives, and forbidden scaffolding files. + +Design contract (matches the Hermes "no lazy-reading escape hatches / don't +destroy the feature" posture): + +* Findings are **advisory** by default. ``lint_skill`` returns a list of + :class:`LintFinding`; the caller decides whether any severity blocks. The + create-path surfaces them as guidance, never as a hard reject (the hard + rejects already live in the validator). +* Pure functions, no I/O beyond reading the files it is pointed at, so CI, the + ``skill_manage`` create path, and a contributor running it locally all share + one implementation. +* Reuses ``agent.skill_utils`` helpers rather than re-parsing frontmatter, so + BOM handling / platform matching / the 60-char prompt budget stay in one + place. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from pathlib import Path +from typing import Any, Dict, List, Optional + +from agent.skill_utils import ( + SKILL_PROMPT_DESC_LIMIT, + parse_frontmatter, +) + +# ── Rule data ──────────────────────────────────────────────────────────────── + +# Shell utilities the agent already has wrapped as first-class tools. Naming +# them in SKILL.md prose steers the model to a raw shell call instead of the +# native tool. Maps the banned token -> the native tool the prose should name. +# (CONTRIBUTING.md "Skill authoring standards" rule 2.) +_SHELL_UTIL_TO_TOOL: Dict[str, str] = { + "grep": "search_files", + "rg": "search_files", + "cat": "read_file", + "head": "read_file", + "tail": "read_file", + "sed": "patch", + "awk": "patch", + "find": "search_files (target='files')", + "ls": "search_files (target='files')", +} + +# Marketing words the description must not contain (rule 1). +_MARKETING_WORDS = ( + "powerful", + "comprehensive", + "seamless", + "advanced", + "cutting-edge", + "state-of-the-art", + "revolutionary", + "robust", +) + +# POSIX-only primitives that, if a bundled script uses them, require the skill +# to declare ``platforms:`` (rule 3). Detected in scripts/, not in prose. +_POSIX_PRIMITIVES = ( + "fcntl", + "termios", + "os.setsid", + "signal.SIGKILL", + "osascript", + "/proc/", + "apt-get", + "systemctl", +) + +# Scaffolding files a skill should not ship (skill-creator anti-pattern; keeps +# skills dense). These are noise, not skill content. +_FORBIDDEN_FILES = ( + "README.md", + "CHANGELOG.md", + "install.sh", + ".env", + ".env.example", + ".gitignore", +) + +# Recommended modern section order (rule 5). We check presence of the load +# bearing ones, not exact ordering, to avoid being a change-detector. +_EXPECTED_SECTIONS = ("When to Use", "When to use") + +ERROR = "error" +WARNING = "warning" + + +@dataclass +class LintFinding: + """A single lint result. ``severity`` is advisory metadata for the caller.""" + + severity: str # ERROR | WARNING + rule: str + message: str + + def format(self) -> str: + badge = "✗" if self.severity == ERROR else "⚠" + return f"{badge} [{self.rule}] {self.message}" + + +# ── Individual checks ──────────────────────────────────────────────────────── + + +def _check_name_matches_dir( + frontmatter: Dict[str, Any], skill_dir: Optional[Path] +) -> List[LintFinding]: + if skill_dir is None: + return [] + name = str(frontmatter.get("name", "")).strip() + if not name: + return [] + if name != skill_dir.name: + return [ + LintFinding( + ERROR, + "name-dir-mismatch", + f"frontmatter name '{name}' does not match directory " + f"'{skill_dir.name}'; they must be identical.", + ) + ] + return [] + + +def _check_name_format(frontmatter: Dict[str, Any]) -> List[LintFinding]: + name = str(frontmatter.get("name", "")).strip() + if not name: + return [] + if not re.fullmatch(r"[a-z0-9][a-z0-9_-]*", name): + return [ + LintFinding( + ERROR, + "name-format", + f"name '{name}' must be lowercase letters, digits, hyphens, " + f"and underscores only.", + ) + ] + return [] + + +def _check_description(frontmatter: Dict[str, Any]) -> List[LintFinding]: + findings: List[LintFinding] = [] + # Raw description as authored — extract_skill_description() applies the + # 60-char prompt truncation, so it can never exceed the limit; measure the + # raw frontmatter value for the length check. + desc = str(frontmatter.get("description", "")).strip().strip("'\"") + if not desc: + return findings + if len(desc) > SKILL_PROMPT_DESC_LIMIT: + findings.append( + LintFinding( + WARNING, + "description-length", + f"description is {len(desc)} chars; the skill index truncates " + f"past {SKILL_PROMPT_DESC_LIMIT} chars + '...', losing routing " + f"signal. Keep it to one sentence.", + ) + ) + lower = desc.lower() + hits = [w for w in _MARKETING_WORDS if re.search(rf"\b{re.escape(w)}\b", lower)] + if hits: + findings.append( + LintFinding( + WARNING, + "description-marketing", + f"description contains marketing words {hits}; state the " + f"capability, not adjectives.", + ) + ) + return findings + + +def _check_metadata_block(frontmatter: Dict[str, Any]) -> List[LintFinding]: + findings: List[LintFinding] = [] + for key in ("version", "author", "license"): + if key not in frontmatter: + findings.append( + LintFinding( + WARNING, + "missing-metadata", + f"frontmatter is missing '{key}'; every peer skill has it.", + ) + ) + meta = frontmatter.get("metadata") + hermes_meta = meta.get("hermes") if isinstance(meta, dict) else None + if not isinstance(hermes_meta, dict): + findings.append( + LintFinding( + WARNING, + "missing-metadata", + "frontmatter is missing metadata.hermes.{tags, related_skills}.", + ) + ) + else: + if "tags" not in hermes_meta: + findings.append( + LintFinding( + WARNING, "missing-metadata", "metadata.hermes.tags is missing." + ) + ) + author = str(frontmatter.get("author", "")) + if author and author.strip().lower() in ("hermes", "agent", "hermes agent") and ( + author != "Hermes Agent" + ): + findings.append( + LintFinding( + WARNING, + "author-caps", + f"author '{author}' should be 'Hermes Agent' (proper caps) " + f"or a real contributor name.", + ) + ) + return findings + + +def _check_shell_utilities(body: str) -> List[LintFinding]: + """Flag banned shell utilities named in PROSE (not fenced code blocks).""" + findings: List[LintFinding] = [] + prose = _strip_code_blocks(body) + for util, tool in _SHELL_UTIL_TO_TOOL.items(): + # Backtick-wrapped mention in prose, e.g. `grep` — the failure mode + # CONTRIBUTING rule 2 targets. Bare words in sentences are too noisy. + if re.search(rf"`{re.escape(util)}`", prose): + findings.append( + LintFinding( + WARNING, + "shell-utility-reference", + f"prose references `{util}`; name the native tool " + f"`{tool}` instead.", + ) + ) + return findings + + +def _check_sections(body: str) -> List[LintFinding]: + if not any(re.search(rf"^#+\s+{re.escape(s)}", body, re.M) for s in _EXPECTED_SECTIONS): + return [ + LintFinding( + WARNING, + "missing-section", + "no '## When to Use' section found; skills need explicit " + "trigger conditions near the top.", + ) + ] + return [] + + +def _check_reference_links(body: str, skill_dir: Optional[Path]) -> List[LintFinding]: + """Flag references/ links in the body that don't resolve on disk.""" + if skill_dir is None: + return [] + findings: List[LintFinding] = [] + seen: set[str] = set() + # Only references/, templates/, assets/ are reliably skill-owned. `scripts/` + # is excluded: dev skills routinely mention repo-root scripts like + # `scripts/run_tests.sh` that legitimately live outside the skill dir. + for match in re.finditer(r"(references|templates|assets)/[\w./-]+", body): + rel = match.group(0) + if rel in seen: + continue + seen.add(rel) + # Skip obvious placeholders / globs. + if "*" in rel or rel.endswith("/"): + continue + if not (skill_dir / rel).exists(): + findings.append( + LintFinding( + WARNING, + "dangling-reference", + f"body references '{rel}' but that file does not exist " + f"in the skill directory.", + ) + ) + return findings + + +def _check_platforms_gating( + frontmatter: Dict[str, Any], skill_dir: Optional[Path] +) -> List[LintFinding]: + """If bundled scripts use POSIX-only primitives, require platforms:.""" + if skill_dir is None: + return [] + if frontmatter.get("platforms"): + return [] # already gated + scripts_dir = skill_dir / "scripts" + if not scripts_dir.is_dir(): + return [] + offenders: Dict[str, List[str]] = {} + for script in scripts_dir.rglob("*"): + if not script.is_file() or script.suffix not in (".py", ".sh", ".bash"): + continue + try: + text = script.read_text(encoding="utf-8", errors="ignore") + except OSError: + continue + hit = [p for p in _POSIX_PRIMITIVES if p in text] + if hit: + offenders[script.name] = hit + if offenders: + detail = "; ".join(f"{k}: {v}" for k, v in offenders.items()) + return [ + LintFinding( + WARNING, + "platforms-gating", + f"scripts use POSIX-only primitives ({detail}) but no " + f"'platforms:' frontmatter is declared. Fix cross-platform or " + f"gate with platforms: [linux, macos].", + ) + ] + return [] + + +def _check_forbidden_files(skill_dir: Optional[Path]) -> List[LintFinding]: + if skill_dir is None: + return [] + findings: List[LintFinding] = [] + for fname in _FORBIDDEN_FILES: + if (skill_dir / fname).exists(): + findings.append( + LintFinding( + WARNING, + "forbidden-file", + f"skill ships '{fname}'; skills should not include " + f"scaffolding/config files.", + ) + ) + return findings + + +def _check_platform_list_valid(frontmatter: Dict[str, Any]) -> List[LintFinding]: + platforms = frontmatter.get("platforms") + if not platforms: + return [] + valid = {"linux", "macos", "windows", "darwin"} + items = platforms if isinstance(platforms, list) else [platforms] + bad = [p for p in items if str(p).lower() not in valid] + if bad: + return [ + LintFinding( + WARNING, + "platforms-value", + f"platforms contains unrecognized value(s) {bad}; expected a " + f"subset of {sorted(valid)}.", + ) + ] + return [] + + +# ── Helpers ────────────────────────────────────────────────────────────────── + + +def _strip_code_blocks(body: str) -> str: + """Remove fenced code blocks so prose-only checks don't fire on examples.""" + return re.sub(r"```.*?```", "", body, flags=re.S) + + +# ── Public API ─────────────────────────────────────────────────────────────── + + +def lint_content( + content: str, *, skill_dir: Optional[Path] = None +) -> List[LintFinding]: + """Lint raw SKILL.md *content*. + + Pass ``skill_dir`` to enable on-disk checks (name/dir match, dangling + reference links, POSIX-primitive gating, forbidden files). Without it, + only content-only checks run — useful for the create path, where the file + is not yet written. + """ + frontmatter, body = parse_frontmatter(content) + findings: List[LintFinding] = [] + findings += _check_name_format(frontmatter) + findings += _check_name_matches_dir(frontmatter, skill_dir) + findings += _check_description(frontmatter) + findings += _check_metadata_block(frontmatter) + findings += _check_platform_list_valid(frontmatter) + findings += _check_shell_utilities(body) + findings += _check_sections(body) + findings += _check_reference_links(body, skill_dir) + findings += _check_platforms_gating(frontmatter, skill_dir) + findings += _check_forbidden_files(skill_dir) + return findings + + +def lint_skill(skill_md_path: Path) -> List[LintFinding]: + """Lint a SKILL.md file on disk, with all on-disk checks enabled.""" + skill_md_path = Path(skill_md_path) + content = skill_md_path.read_text(encoding="utf-8", errors="ignore") + return lint_content(content, skill_dir=skill_md_path.parent) + + +def format_findings(findings: List[LintFinding]) -> str: + """Render findings as a newline-joined human-readable block.""" + return "\n".join(f.format() for f in findings) + + +def has_errors(findings: List[LintFinding]) -> bool: + return any(f.severity == ERROR for f in findings) + + +def _main(argv: Optional[List[str]] = None) -> int: + """CLI: ``python -m tools.skill_linter ...`` + + Accepts SKILL.md files or skill directories (recursively linted). Prints + findings grouped per skill. Exit code 1 if any ERROR-severity finding is + present (WARNING-only is exit 0), so CI can gate on structural breakage + without failing on advisory convention nits. + """ + import sys + + args = argv if argv is not None else sys.argv[1:] + if not args: + print("usage: python -m tools.skill_linter ...") + return 2 + + targets: List[Path] = [] + for arg in args: + p = Path(arg) + if p.is_dir(): + targets.extend(sorted(p.rglob("SKILL.md"))) + elif p.name == "SKILL.md" and p.is_file(): + targets.append(p) + else: + print(f"skip (not a SKILL.md or dir): {arg}") + + any_error = False + total = 0 + for skill_md in targets: + findings = lint_skill(skill_md) + if not findings: + continue + total += len(findings) + if has_errors(findings): + any_error = True + print(f"\n{skill_md.parent.name} ({skill_md}):") + print(format_findings(findings)) + + if total == 0: + print(f"All {len(targets)} skill(s) clean.") + else: + print(f"\n{total} finding(s) across {len(targets)} skill(s).") + return 1 if any_error else 0 + + +if __name__ == "__main__": + import sys + + sys.exit(_main()) diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 9ef337144d223..0d80722025e99 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -971,9 +971,38 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An "skill_manage(action='write_file', name='{}', file_path='references/example.md', file_content='...')".format(name) ) _add_description_prompt_preview(result, content) + _attach_lint_findings(result, skill_md) return result +def _attach_lint_findings(result: Dict[str, Any], skill_md: Path) -> None: + """Run the advisory SKILL.md linter and attach any findings to *result*. + + The linter enforces the CONTRIBUTING "Skill authoring standards (HARDLINE)" + conventions that the hard validator does not (shell-utility references, + missing metadata, dangling reference links, POSIX gating, forbidden files). + Findings are ADVISORY — surfaced as guidance so the author can fix them, + never a hard block. The hard rejects already ran in _validate_frontmatter. + """ + try: + from tools.skill_linter import lint_skill # local import: optional path + + findings = lint_skill(skill_md) + except Exception: + return + if not findings: + return + result["lint_warnings"] = [ + {"severity": f.severity, "rule": f.rule, "message": f.message} + for f in findings + ] + result["lint_hint"] = ( + "The skill was created. These are advisory authoring-convention " + "findings (not blockers) — fix them with skill_manage(action='patch') " + "to match Hermes skill standards." + ) + + def _edit_skill(name: str, content: str) -> Dict[str, Any]: """Replace the SKILL.md of any existing skill (full rewrite).""" err = _validate_frontmatter(content)