Merge pull request #412 from tmchow/refactor/skill-meta-version-helper

refactor: consolidate SKILL.md version regex into lib/skill_meta.py

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 "?"
 
 

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)

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):

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()

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()