From 73dc6b999689e188f54860d9541045e1b3c592d9 Mon Sep 17 00:00:00 2001 From: Trevin Chow Date: Sat, 16 May 2026 18:17:33 -0700 Subject: [PATCH] refactor: consolidate SKILL.md version regex into lib/skill_meta.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same `^version:\s*"([^"]+)"\s*$` regex (or a slight variant) was duplicated across three files: render.py inline, test_plugin_contract.py local helper, test_version_consistency.py local helper. A future change to the SKILL.md frontmatter version format would have needed to update three places without any compile-time pressure to keep them in sync. New skills/last30days/scripts/lib/skill_meta.py provides: - `_VERSION_RE` private compiled pattern (accepts double-quoted, single-quoted, or unquoted YAML version scalars per the widening landed in 997708a) - `read_skill_version(skill_md_path: Path) -> str | None` helper that catches OSError + UnicodeDecodeError and returns None on miss Callers updated: - render.py::_skill_version now calls skill_meta.read_skill_version inside the SKILL.md fallback loop, returning `read_skill_version(...) or "?"`. Semantically equivalent to the old break-after-first-SKILL.md logic. - test_plugin_contract.py and test_version_consistency.py import the helper instead of defining the regex inline. Both files use the established sys.path.insert pattern. Added tests/test_skill_meta.py with 6 direct unit tests covering the helper's full contract: missing file, undecodable bytes, no-version-line, and all three quoting styles (double, single, unquoted). Previously the helper was only exercised transitively through render._skill_version(). Added test_skill_md_uses_double_quoted_version to test_version_consistency.py — the old per-test regex incidentally asserted "this repo's SKILL.md uses double-quotes" by being strict; the shared helper accepts all three styles, so the assertion is now explicit instead of implicit. Code-reviewed by ce-code-review (8 reviewers); safe_auto fixes applied inline (rename to _VERSION_RE, group or-chain instead of generator, docstring tightened, dropped unnecessary `from __future__ import annotations`, tightened signature to Path-only). Conftest.py refactor for the sys.path.insert duplication across ~20 test files filed as issue #411 — out of scope for this PR (touches many files, separate concern). Test results: 23 passed in the affected test set (16 prior + 6 new test_skill_meta tests + 1 new double-quote assertion). Full suite shows same 13 pre-existing failures as main; zero new failures. --- skills/last30days/scripts/lib/render.py | 25 +++------ skills/last30days/scripts/lib/skill_meta.py | 33 +++++++++++ tests/test_plugin_contract.py | 12 ++-- tests/test_skill_meta.py | 61 +++++++++++++++++++++ tests/test_version_consistency.py | 24 ++++++-- 5 files changed, 128 insertions(+), 27 deletions(-) create mode 100644 skills/last30days/scripts/lib/skill_meta.py create mode 100644 tests/test_skill_meta.py diff --git a/skills/last30days/scripts/lib/render.py b/skills/last30days/scripts/lib/render.py index f8f1541..4d80718 100644 --- a/skills/last30days/scripts/lib/render.py +++ b/skills/last30days/scripts/lib/render.py @@ -4,18 +4,11 @@ from __future__ import annotations import json import pathlib -import re from collections import Counter from datetime import date from urllib.parse import urlparse -from . import dates, schema - - -_VERSION_RE = re.compile( - r'''^version:\s*(?:"([^"]+)"|'([^']+)'|(\S+))\s*$''', - re.MULTILINE, -) +from . import dates, schema, skill_meta def _skill_version() -> str: @@ -25,11 +18,12 @@ def _skill_version() -> str: Hermes, etc.) do not always carry `.claude-plugin/plugin.json` — that file ships with plugin-cache installs but not with per-harness skill installs. SKILL.md frontmatter is the fallback that keeps the badge from emitting v? on those installs. Returns "?" only - if both sources are missing. + if no usable version string is found from either source (missing files, corrupt JSON, + or SKILL.md without a version line). A corrupt manifest at one ancestor does not shadow a valid manifest at a deeper one - (continue, not break). YAML frontmatter accepts double-quoted, single-quoted, or - unquoted version scalars. + (continue, not break). SKILL.md parsing accepts double-quoted, single-quoted, or + unquoted YAML version scalars (delegated to skill_meta.read_skill_version). """ here = pathlib.Path(__file__).resolve() for parent in here.parents: @@ -43,16 +37,11 @@ def _skill_version() -> str: return version # No usable manifest found at any ancestor — fall back to SKILL.md frontmatter. + # First SKILL.md found in the walk is THIS skill's; never traverse past it. for parent in here.parents: skill_md = parent / "SKILL.md" if skill_md.is_file(): - try: - match = _VERSION_RE.search(skill_md.read_text()) - except (OSError, UnicodeDecodeError): - break - if match: - return next(g for g in match.groups() if g is not None) - break + return skill_meta.read_skill_version(skill_md) or "?" return "?" diff --git a/skills/last30days/scripts/lib/skill_meta.py b/skills/last30days/scripts/lib/skill_meta.py new file mode 100644 index 0000000..4e23fff --- /dev/null +++ b/skills/last30days/scripts/lib/skill_meta.py @@ -0,0 +1,33 @@ +"""SKILL.md metadata helpers — single source of truth for parsing skill frontmatter. + +Centralizes the version regex that previously lived in render.py and was +duplicated in tests/test_plugin_contract.py and tests/test_version_consistency.py. +""" + +import re +from pathlib import Path + +# Matches `version: "x.y.z"`, `version: 'x.y.z'`, or `version: x.y.z` in YAML +# frontmatter. Multiline so the pattern can be applied to a full SKILL.md text. +# Three alternation groups — exactly one captures per successful match. +_VERSION_RE = re.compile( + r'''^version:\s*(?:"([^"]+)"|'([^']+)'|(\S+))\s*$''', + re.MULTILINE, +) + + +def read_skill_version(skill_md_path: Path) -> str | None: + """Return the version string from a SKILL.md's frontmatter, or None. + + Returns None if the file can't be read (missing, permission, decode error) + or if no `version:` line is found. Accepts double-quoted, single-quoted, + or unquoted YAML version scalars. + """ + try: + text = skill_md_path.read_text() + except (OSError, UnicodeDecodeError): + return None + match = _VERSION_RE.search(text) + if not match: + return None + return match.group(1) or match.group(2) or match.group(3) diff --git a/tests/test_plugin_contract.py b/tests/test_plugin_contract.py index 6e8480e..ba1f4a9 100644 --- a/tests/test_plugin_contract.py +++ b/tests/test_plugin_contract.py @@ -1,5 +1,5 @@ import json -import re +import sys import tomllib import unittest from pathlib import Path @@ -8,17 +8,19 @@ from pathlib import Path ROOT = Path(__file__).resolve().parents[1] SKILL_ROOT = ROOT / "skills" / "last30days" +sys.path.insert(0, str(SKILL_ROOT / "scripts")) +from lib.skill_meta import read_skill_version # noqa: E402 + def _json(path: Path) -> dict: return json.loads(path.read_text(encoding="utf-8")) def _skill_version() -> str: - text = (SKILL_ROOT / "SKILL.md").read_text(encoding="utf-8") - match = re.search(r'^version:\s*"([^"]+)"\s*$', text, re.MULTILINE) - if not match: + version = read_skill_version(SKILL_ROOT / "SKILL.md") + if not version: raise AssertionError("SKILL.md version frontmatter not found") - return match.group(1) + return version class TestPluginContract(unittest.TestCase): diff --git a/tests/test_skill_meta.py b/tests/test_skill_meta.py new file mode 100644 index 0000000..4ab6b12 --- /dev/null +++ b/tests/test_skill_meta.py @@ -0,0 +1,61 @@ +"""Direct unit tests for skill_meta.read_skill_version. + +Covers the helper's own contract independent of render._skill_version which +exercises it transitively. Without these, regressions in error handling or +regex coverage inside the helper could pass CI because render.py's fallback +to "?" swallows the signal. +""" + +import sys +import tempfile +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT / "skills" / "last30days" / "scripts")) +from lib.skill_meta import read_skill_version # noqa: E402 + + +class ReadSkillVersionTests(unittest.TestCase): + def setUp(self) -> None: + self._tmp = tempfile.TemporaryDirectory() + self.tmp_path = Path(self._tmp.name) + + def tearDown(self) -> None: + self._tmp.cleanup() + + def _write_skill_md(self, body: str) -> Path: + path = self.tmp_path / "SKILL.md" + path.write_text(body) + return path + + def test_double_quoted_version(self) -> None: + path = self._write_skill_md('---\nname: x\nversion: "9.9.9"\n---\n') + self.assertEqual("9.9.9", read_skill_version(path)) + + def test_single_quoted_version(self) -> None: + path = self._write_skill_md("---\nname: x\nversion: '8.8.8'\n---\n") + self.assertEqual("8.8.8", read_skill_version(path)) + + def test_unquoted_version(self) -> None: + path = self._write_skill_md("---\nname: x\nversion: 7.7.7\n---\n") + self.assertEqual("7.7.7", read_skill_version(path)) + + def test_missing_file_returns_none(self) -> None: + self.assertIsNone(read_skill_version(self.tmp_path / "does-not-exist.md")) + + def test_no_version_line_returns_none(self) -> None: + path = self._write_skill_md("---\nname: x\n---\n# body without version\n") + self.assertIsNone(read_skill_version(path)) + + def test_undecodable_bytes_returns_none(self) -> None: + # Bytes 128-255 don't form valid UTF-8 sequences; read_text() raises + # UnicodeDecodeError which the helper must catch. + path = self.tmp_path / "SKILL.md" + path.write_bytes(bytes(range(128, 256))) + self.assertIsNone(read_skill_version(path)) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_version_consistency.py b/tests/test_version_consistency.py index e7fef5d..2966565 100644 --- a/tests/test_version_consistency.py +++ b/tests/test_version_consistency.py @@ -1,4 +1,5 @@ import re +import sys import unittest from pathlib import Path @@ -6,16 +7,31 @@ from pathlib import Path ROOT = Path(__file__).resolve().parents[1] SKILL_ROOT = ROOT / "skills" / "last30days" +sys.path.insert(0, str(SKILL_ROOT / "scripts")) +from lib.skill_meta import read_skill_version # noqa: E402 + def _skill_version() -> str: - text = (SKILL_ROOT / "SKILL.md").read_text(encoding="utf-8") - match = re.search(r'^version:\s*"([^"]+)"\s*$', text, re.MULTILINE) - if not match: + version = read_skill_version(SKILL_ROOT / "SKILL.md") + if not version: raise AssertionError("SKILL.md version frontmatter not found") - return match.group(1) + return version class TestVersionConsistency(unittest.TestCase): + def test_skill_md_uses_double_quoted_version(self) -> None: + # The shared VERSION_RE in skill_meta.py accepts double-quoted, + # single-quoted, and unquoted YAML version scalars. This repo's + # SKILL.md must use the double-quoted form so the badge string stays + # deterministic and contributors don't accidentally introduce a + # quoting style that's harder for downstream tooling to parse. + text = (SKILL_ROOT / "SKILL.md").read_text(encoding="utf-8") + self.assertRegex( + text, + re.compile(r'^version:\s*"[^"]+"\s*$', re.MULTILINE), + msg="SKILL.md frontmatter version must use double-quoted form", + ) + def test_root_skill_header_matches_frontmatter_version(self) -> None: text = (SKILL_ROOT / "SKILL.md").read_text(encoding="utf-8") version = _skill_version()