Fix CI release job no-common-ancestor failure and expand test coverage

packages/skilllint/tests/test_frontmatter_module.py

@@ -0,0 +1,224 @@
+"""Tests for skilllint.frontmatter module (mmap-based processor).
+
+Tests:
+- loads_frontmatter: no-closing-delimiter path, YAML parse error path
+- load_frontmatter / dump_frontmatter / dumps_frontmatter / update_field
+- process_markdown_file: empty file, no-frontmatter, malformed, and with
+  lint_and_fix mocked to drive both the "no fix needed" and "fix needed" paths
+- lint_and_fix raises NotImplementedError (scaffold)
+
+How: Uses tmp_path for real files; unittest.mock.patch to control lint_and_fix.
+Why: frontmatter.py is separate from frontmatter_utils.py and had 0% coverage
+     for the mmap-based process_markdown_file and several loads_frontmatter
+     branches.
+"""
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+from unittest.mock import patch
+
+import pytest
+
+from skilllint.frontmatter import (
+    Post,
+    dump_frontmatter,
+    dumps_frontmatter,
+    lint_and_fix,
+    load_frontmatter,
+    loads_frontmatter,
+    process_markdown_file,
+    update_field,
+)
+
+if TYPE_CHECKING:
+    from pathlib import Path
+
+
+class TestLoadsFrontmatter:
+    """Tests for loads_frontmatter string parser."""
+
+    def test_no_opening_delimiter_returns_empty_metadata(self) -> None:
+        """Text not starting with '---' yields empty metadata."""
+        text = "# Just a heading\n\nSome body text.\n"
+        post = loads_frontmatter(text)
+        assert post.metadata == {}
+        assert "Just a heading" in post.content
+
+    def test_unclosed_frontmatter_returns_empty_metadata(self) -> None:
+        """Text starting with '---' but without a closing delimiter yields empty metadata."""
+        text = "---\nname: test\ndescription: no closing\n"
+        post = loads_frontmatter(text)
+        assert post.metadata == {}
+        # The unclosed text is returned as content
+        assert post.content == text
+
+    def test_valid_frontmatter_parsed(self) -> None:
+        """Standard frontmatter is parsed correctly."""
+        text = "---\nname: my-skill\ndescription: A test skill\n---\n\nBody.\n"
+        post = loads_frontmatter(text)
+        assert post.metadata["name"] == "my-skill"
+        assert post.metadata["description"] == "A test skill"
+        assert post.content == "Body."
+
+    def test_non_dict_yaml_returns_empty_metadata(self) -> None:
+        """YAML that parses as a non-dict (list) yields empty metadata."""
+        # A YAML list is valid YAML but not a dict — metadata stays empty.
+        text = "---\n- item1\n- item2\n---\n\nBody.\n"
+        post = loads_frontmatter(text)
+        assert post.metadata == {}
+
+    def test_empty_frontmatter_block_returns_empty_metadata(self) -> None:
+        """Frontmatter delimiters with no content yield empty metadata."""
+        text = "---\n---\n\nBody text.\n"
+        post = loads_frontmatter(text)
+        assert post.metadata == {}
+
+    def test_body_content_extracted(self) -> None:
+        """Body is everything after the closing '---' line."""
+        text = "---\nname: test\n---\n\nFirst line.\nSecond line.\n"
+        post = loads_frontmatter(text)
+        assert "First line." in post.content
+        assert "Second line." in post.content
+
+
+class TestLoadFrontmatter:
+    """Tests for load_frontmatter file-based API."""
+
+    def test_loads_from_file(self, tmp_path: Path) -> None:
+        """Reads and parses a real file."""
+        f = tmp_path / "test.md"
+        f.write_text("---\nname: skill\n---\n\nContent.\n", encoding="utf-8")
+        post = load_frontmatter(f)
+        assert post.metadata["name"] == "skill"
+
+    def test_string_path_accepted(self, tmp_path: Path) -> None:
+        """Accepts a string path as well as a Path object."""
+        f = tmp_path / "test.md"
+        f.write_text("---\nname: skill\n---\n\nContent.\n", encoding="utf-8")
+        post = load_frontmatter(str(f))
+        assert post.metadata["name"] == "skill"
+
+
+class TestDumpFrontmatter:
+    """Tests for dump_frontmatter and dumps_frontmatter."""
+
+    def test_dump_produces_delimiters(self) -> None:
+        """Serialised output starts with '---' and contains another '---'."""
+        post = Post(metadata={"name": "skill"}, content="Body.\n")
+        result = dump_frontmatter(post)
+        assert result.startswith("---\n")
+        assert "\n---\n" in result
+
+    def test_dump_includes_body(self) -> None:
+        """Body content appears after the closing delimiter."""
+        post = Post(metadata={"name": "skill"}, content="My body.")
+        result = dump_frontmatter(post)
+        assert "My body." in result
+
+    def test_dumps_writes_file(self, tmp_path: Path) -> None:
+        """dumps_frontmatter writes a valid file that can be round-tripped."""
+        post = Post(metadata={"name": "write-test"}, content="Body text.")
+        target = tmp_path / "out.md"
+        dumps_frontmatter(post, target)
+        reloaded = load_frontmatter(target)
+        assert reloaded.metadata["name"] == "write-test"
+
+
+class TestUpdateField:
+    """Tests for update_field convenience function."""
+
+    def test_updates_existing_field(self, tmp_path: Path) -> None:
+        """An existing key is overwritten."""
+        f = tmp_path / "skill.md"
+        f.write_text("---\nname: old\n---\n\nBody.\n", encoding="utf-8")
+        update_field(f, "name", "new")
+        post = load_frontmatter(f)
+        assert post.metadata["name"] == "new"
+
+    def test_adds_new_field(self, tmp_path: Path) -> None:
+        """A new key is inserted."""
+        f = tmp_path / "skill.md"
+        f.write_text("---\nname: skill\n---\n\nBody.\n", encoding="utf-8")
+        update_field(f, "model", "sonnet")
+        post = load_frontmatter(f)
+        assert post.metadata["model"] == "sonnet"
+        assert post.metadata["name"] == "skill"
+
+
+class TestProcessMarkdownFile:
+    """Tests for process_markdown_file (mmap-based in-place processor)."""
+
+    def test_empty_file_is_skipped_silently(self, tmp_path: Path) -> None:
+        """Empty file does not raise (mmap raises ValueError on zero-length)."""
+        f = tmp_path / "empty.md"
+        f.write_text("", encoding="utf-8")
+        # Should return without error
+        process_markdown_file(str(f))
+        assert f.read_text(encoding="utf-8") == ""
+
+    def test_file_without_frontmatter_is_untouched(self, tmp_path: Path) -> None:
+        """File with no opening '---' delimiter is not modified."""
+        original = "# No frontmatter\n\nJust a body.\n"
+        f = tmp_path / "plain.md"
+        f.write_text(original, encoding="utf-8")
+        process_markdown_file(str(f))
+        assert f.read_text(encoding="utf-8") == original
+
+    def test_malformed_no_closing_delimiter_is_untouched(self, tmp_path: Path) -> None:
+        """File with opening '---' but no closing delimiter is not modified."""
+        original = "---\nname: skill\ndescription: no closing\n"
+        f = tmp_path / "malformed.md"
+        f.write_text(original, encoding="utf-8")
+        process_markdown_file(str(f))
+        assert f.read_text(encoding="utf-8") == original
+
+    def test_lint_and_fix_not_needed_no_write(self, tmp_path: Path) -> None:
+        """When lint_and_fix returns (False, ...), the file is not rewritten."""
+        original = "---\nname: skill\n---\n\nBody.\n"
+        f = tmp_path / "ok.md"
+        f.write_text(original, encoding="utf-8")
+
+        with patch("skilllint.frontmatter.lint_and_fix", return_value=(False, b"name: skill\n")):
+            process_markdown_file(str(f))
+
+        # File must be unchanged
+        assert f.read_text(encoding="utf-8") == original
+
+    def test_lint_and_fix_needed_rewrites_file(self, tmp_path: Path) -> None:
+        """When lint_and_fix returns (True, new_bytes), the file is rewritten."""
+        original = "---\nname: skill\n---\n\nBody.\n"
+        f = tmp_path / "fixme.md"
+        f.write_text(original, encoding="utf-8")
+
+        fixed_yaml = b"name: fixed-skill\n"
+        with patch("skilllint.frontmatter.lint_and_fix", return_value=(True, fixed_yaml)):
+            process_markdown_file(str(f))
+
+        result = f.read_text(encoding="utf-8")
+        assert "fixed-skill" in result
+        # Body should still be present
+        assert "Body." in result
+
+    def test_lint_and_fix_adds_trailing_newline_if_missing(self, tmp_path: Path) -> None:
+        """lint_and_fix result without trailing newline gets one appended."""
+        original = "---\nname: skill\n---\n\nBody.\n"
+        f = tmp_path / "nonl.md"
+        f.write_text(original, encoding="utf-8")
+
+        # Bytes without trailing newline
+        fixed_yaml = b"name: newname"
+        with patch("skilllint.frontmatter.lint_and_fix", return_value=(True, fixed_yaml)):
+            process_markdown_file(str(f))
+
+        result = f.read_text(encoding="utf-8")
+        assert "newname" in result
+
+
+class TestLintAndFix:
+    """Tests for the lint_and_fix scaffold."""
+
+    def test_raises_not_implemented(self) -> None:
+        """lint_and_fix raises NotImplementedError (scaffold, not yet integrated)."""
+        with pytest.raises(NotImplementedError, match="lint_and_fix is not yet implemented"):
+            lint_and_fix(b"name: skill\n")

packages/skilllint/tests/test_limits.py

@@ -0,0 +1,159 @@
+"""Tests for skilllint.limits module.
+
+Tests:
+- Constant values match specification sources
+- Provider and RuleLimit enums have expected members
+- Legacy alias constants equal the canonical values
+- Token thresholds are logically ordered
+
+How: Direct attribute and enum-member inspection.
+Why: limits.py is the single source of truth for all validation thresholds;
+     regressions here silently break rule enforcement without these tests.
+"""
+
+from __future__ import annotations
+
+from skilllint.limits import (
+    BODY_TOKEN_ERROR,
+    BODY_TOKEN_WARNING,
+    COMPATIBILITY_MAX_LENGTH,
+    DESCRIPTION_MAX_LENGTH,
+    DESCRIPTION_MIN_LENGTH,
+    LICENSE_MAX_LENGTH,
+    MAX_SKILL_NAME_LENGTH,
+    METADATA_TOKEN_BUDGET,
+    NAME_MAX_LENGTH,
+    NAME_MIN_LENGTH,
+    NAME_PATTERN,
+    RECOMMENDED_DESCRIPTION_LENGTH,
+    TOKEN_ERROR_THRESHOLD,
+    TOKEN_WARNING_THRESHOLD,
+    Provider,
+    RuleLimit,
+)
+
+
+class TestProviderEnum:
+    """Tests for the Provider enumeration."""
+
+    def test_agentskills_io_member(self) -> None:
+        """Provider.AGENTSKILLS_IO has value 'agentskills.io'."""
+        assert Provider.AGENTSKILLS_IO.value == "agentskills.io"
+
+    def test_claude_code_member(self) -> None:
+        """Provider.CLAUDE_CODE has value 'claude-code'."""
+        assert Provider.CLAUDE_CODE.value == "claude-code"
+
+    def test_cursor_member(self) -> None:
+        """Provider.CURSOR has value 'cursor'."""
+        assert Provider.CURSOR.value == "cursor"
+
+    def test_codex_member(self) -> None:
+        """Provider.CODEX has value 'codex'."""
+        assert Provider.CODEX.value == "codex"
+
+    def test_skilllint_member(self) -> None:
+        """Provider.SKILL_LINT has value 'skilllint'."""
+        assert Provider.SKILL_LINT.value == "skilllint"
+
+    def test_all_providers_accounted_for(self) -> None:
+        """Provider enum contains exactly the expected set of members."""
+        expected = {"AGENTSKILLS_IO", "CLAUDE_CODE", "CURSOR", "CODEX", "SKILL_LINT"}
+        assert {m.name for m in Provider} == expected
+
+
+class TestRuleLimitEnum:
+    """Tests for the RuleLimit enumeration."""
+
+    def test_fm_name_max_length_member(self) -> None:
+        """RuleLimit.FM_NAME_MAX_LENGTH is accessible."""
+        assert RuleLimit.FM_NAME_MAX_LENGTH.value == "fm_name_max_length"
+
+    def test_fm_description_max_length_member(self) -> None:
+        """RuleLimit.FM_DESCRIPTION_MAX_LENGTH is accessible."""
+        assert RuleLimit.FM_DESCRIPTION_MAX_LENGTH.value == "fm_desc_max_length"
+
+    def test_body_warning_member(self) -> None:
+        """RuleLimit.BODY_WARNING is accessible."""
+        assert RuleLimit.BODY_WARNING.value == "body_warning"
+
+    def test_body_error_member(self) -> None:
+        """RuleLimit.BODY_ERROR is accessible."""
+        assert RuleLimit.BODY_ERROR.value == "body_error"
+
+    def test_metadata_budget_member(self) -> None:
+        """RuleLimit.METADATA_BUDGET is accessible."""
+        assert RuleLimit.METADATA_BUDGET.value == "metadata_budget"
+
+    def test_sk_description_min_length_member(self) -> None:
+        """RuleLimit.SK_DESCRIPTION_MIN_LENGTH is accessible."""
+        assert RuleLimit.SK_DESCRIPTION_MIN_LENGTH.value == "sk_desc_min_length"
+
+
+class TestFrontmatterFieldLimits:
+    """Tests for frontmatter field limit constants."""
+
+    def test_name_max_length(self) -> None:
+        """NAME_MAX_LENGTH is 64 per spec."""
+        assert NAME_MAX_LENGTH == 64
+
+    def test_description_max_length(self) -> None:
+        """DESCRIPTION_MAX_LENGTH is 1024 per spec."""
+        assert DESCRIPTION_MAX_LENGTH == 1024
+
+    def test_license_max_length(self) -> None:
+        """LICENSE_MAX_LENGTH is 500 per spec."""
+        assert LICENSE_MAX_LENGTH == 500
+
+    def test_compatibility_max_length(self) -> None:
+        """COMPATIBILITY_MAX_LENGTH is 500 per spec."""
+        assert COMPATIBILITY_MAX_LENGTH == 500
+
+    def test_name_min_length(self) -> None:
+        """NAME_MIN_LENGTH is 1 per spec."""
+        assert NAME_MIN_LENGTH == 1
+
+    def test_name_pattern(self) -> None:
+        """NAME_PATTERN is the expected regex string."""
+        assert NAME_PATTERN == r"^[a-z0-9]+(-[a-z0-9]+)*$"
+
+    def test_description_min_length(self) -> None:
+        """DESCRIPTION_MIN_LENGTH is 20 (best practice)."""
+        assert DESCRIPTION_MIN_LENGTH == 20
+
+
+class TestTokenThresholds:
+    """Tests for token threshold constants."""
+
+    def test_body_token_warning(self) -> None:
+        """BODY_TOKEN_WARNING is 4400."""
+        assert BODY_TOKEN_WARNING == 4400
+
+    def test_body_token_error(self) -> None:
+        """BODY_TOKEN_ERROR is 8800."""
+        assert BODY_TOKEN_ERROR == 8800
+
+    def test_warning_below_error(self) -> None:
+        """Warning threshold is strictly less than error threshold."""
+        assert BODY_TOKEN_WARNING < BODY_TOKEN_ERROR
+
+    def test_metadata_token_budget(self) -> None:
+        """METADATA_TOKEN_BUDGET is 100."""
+        assert METADATA_TOKEN_BUDGET == 100
+
+    def test_backward_compat_aliases(self) -> None:
+        """TOKEN_WARNING_THRESHOLD and TOKEN_ERROR_THRESHOLD match canonical values."""
+        assert TOKEN_WARNING_THRESHOLD == BODY_TOKEN_WARNING
+        assert TOKEN_ERROR_THRESHOLD == BODY_TOKEN_ERROR
+
+
+class TestLegacyAliases:
+    """Tests for deprecated / legacy alias constants."""
+
+    def test_max_skill_name_length_is_40(self) -> None:
+        """Legacy MAX_SKILL_NAME_LENGTH is 40 (differs from spec 64)."""
+        assert MAX_SKILL_NAME_LENGTH == 40
+
+    def test_recommended_description_length_matches_max(self) -> None:
+        """RECOMMENDED_DESCRIPTION_LENGTH equals DESCRIPTION_MAX_LENGTH."""
+        assert RECOMMENDED_DESCRIPTION_LENGTH == DESCRIPTION_MAX_LENGTH