refactor(skill): apply ce-code-review fixes — bump to 3.2.2, fallback tests, comparison resolver
12 fixes from the multi-agent code review on PR #400: Version 3.2.1 -> 3.2.2 across all manifests (SKILL.md frontmatter + body header, pyproject.toml, .claude-plugin/{plugin,marketplace}.json, sync.sh cache path). The PR ships observable behavior changes (STEP 0 logic flip, resolver order change, badge fallback) that should not silently appear under the same version number — the new fallback reads SKILL.md version directly so the badge would otherwise be misleading. render.py::_skill_version: - `import re` moved to module top - _VERSION_RE extracted as a module-level compiled pattern that accepts double-quoted, single-quoted, and unquoted YAML version scalars - `break` -> `continue` on corrupt manifest, so a corrupt inner manifest no longer shadows a valid outer one - Wrap SKILL.md read_text() in try/except for UnicodeDecodeError to keep badge emission from crashing on mis-encoded SKILL.md - Docstring clarifies precedence; inline comment marks the fallback boundary between the manifest walk and the SKILL.md walk tests/test_skill_version.py (new): 7 unit tests for the fallback paths (manifest absent, manifest corrupt, corrupt-inner + valid-outer, both absent, SKILL.md without version, single-quoted, unquoted). tests/test_plugin_contract.py: tombstone test asserting .codex-plugin/ stays removed (was the only CI guard against accidental reintroduction). SKILL.md: - STEP 0 bash echoes CLAUDE_CACHE_LATEST so the model can see the resolved value when deciding whether to hop - "Both shapes ship in the wild" comment now names the two cache layouts (nested {cache}/{version}/skills/last30days/ vs flat {cache}/{version}/) - Comparison-mode bash invocation gets its own inline SKILL_ROOT resolver (latent gap: the contract tells the model to skip Step 1 on comparison queries, so SKILL_ROOT was previously unset there) CHANGELOG.md: [Unreleased] entries for the resolver rewrite and the breaking removal of Codex native-plugin support. All 9 reviewer personas surfaced findings; 3 cross-reviewer corroboration clusters were promoted (import re, "both shapes" comment, missing fallback tests). Maintainability follow-up flagged: regex now duplicated across render.py and 2 test files; could consolidate via shared lib/skill_meta.py helper in a future PR.
This commit is contained in:
@@ -0,0 +1,113 @@
|
||||
"""Unit tests for render._skill_version() fallback paths.
|
||||
|
||||
The function reads version from .claude-plugin/plugin.json first, then falls back
|
||||
to SKILL.md frontmatter. These tests use monkeypatch to swap the render module's
|
||||
__file__ attribute, which controls where the walk starts.
|
||||
"""
|
||||
|
||||
import sys
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "skills" / "last30days" / "scripts"))
|
||||
|
||||
from lib import render
|
||||
|
||||
|
||||
class SkillVersionFallbackTests(unittest.TestCase):
|
||||
def setUp(self):
|
||||
# tmp_path equivalent for unittest
|
||||
import tempfile
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.tmp_path = Path(self._tmp.name)
|
||||
|
||||
def tearDown(self):
|
||||
self._tmp.cleanup()
|
||||
|
||||
def _make_render_at(self, parent: Path) -> Path:
|
||||
"""Place a dummy render.py inside parent and return its path."""
|
||||
parent.mkdir(parents=True, exist_ok=True)
|
||||
fake_render = parent / "render.py"
|
||||
fake_render.write_text("")
|
||||
return fake_render
|
||||
|
||||
def _write_manifest(self, parent: Path, version: str | None) -> None:
|
||||
"""Write .claude-plugin/plugin.json under parent. version=None writes corrupt JSON."""
|
||||
d = parent / ".claude-plugin"
|
||||
d.mkdir(parents=True, exist_ok=True)
|
||||
if version is None:
|
||||
(d / "plugin.json").write_text("{not valid json")
|
||||
else:
|
||||
(d / "plugin.json").write_text(f'{{"version": "{version}"}}')
|
||||
|
||||
def _write_skill_md(self, parent: Path, frontmatter_version_line: str | None) -> None:
|
||||
"""Write SKILL.md with frontmatter. None writes a SKILL.md with no version line."""
|
||||
if frontmatter_version_line is None:
|
||||
body = "---\nname: test\n---\n# body\n"
|
||||
else:
|
||||
body = f"---\nname: test\n{frontmatter_version_line}\n---\n# body\n"
|
||||
(parent / "SKILL.md").write_text(body)
|
||||
|
||||
def test_manifest_absent_falls_back_to_skill_md_frontmatter(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, 'version: "9.9.9"')
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("9.9.9", render._skill_version())
|
||||
|
||||
def test_manifest_corrupt_falls_back_to_skill_md_frontmatter(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_manifest(skill_dir, version=None) # corrupt
|
||||
self._write_skill_md(skill_dir, 'version: "8.8.8"')
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("8.8.8", render._skill_version())
|
||||
|
||||
def test_corrupt_inner_manifest_does_not_shadow_valid_outer_manifest(self):
|
||||
outer = self.tmp_path / "outer"
|
||||
inner = outer / "skill_root"
|
||||
fake_render = self._make_render_at(inner)
|
||||
self._write_manifest(inner, version=None) # corrupt at inner
|
||||
self._write_manifest(outer, version="7.7.7") # valid at outer
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("7.7.7", render._skill_version())
|
||||
|
||||
def test_neither_source_present_returns_question_mark(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
# No manifest, no SKILL.md anywhere under tmp_path
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("?", render._skill_version())
|
||||
|
||||
def test_skill_md_without_version_returns_question_mark(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, frontmatter_version_line=None)
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("?", render._skill_version())
|
||||
|
||||
def test_unquoted_yaml_version_is_accepted(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, "version: 6.6.6")
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("6.6.6", render._skill_version())
|
||||
|
||||
def test_single_quoted_yaml_version_is_accepted(self):
|
||||
skill_dir = self.tmp_path / "skill_root"
|
||||
fake_render = self._make_render_at(skill_dir)
|
||||
self._write_skill_md(skill_dir, "version: '5.5.5'")
|
||||
|
||||
with patch.object(render, "__file__", str(fake_render)):
|
||||
self.assertEqual("5.5.5", render._skill_version())
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user