refactor: consolidate SKILL.md version regex into lib/skill_meta.py
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.
This commit is contained in:
@@ -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 "?"
|
||||
|
||||
|
||||
|
||||
@@ -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)
|
||||
@@ -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):
|
||||
|
||||
@@ -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()
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user