fix(skills): parse SKILL.md frontmatter when --- is not the first byte

docs/scaffolding.md

@@ -198,6 +198,25 @@ the `jvagent/orchestrator` action via:
   owns them.
 - `skills_source`: `library`, `app`, or `both`
 
+### `SKILL.md` frontmatter parsing
+
+The frontmatter block is what declares a skill's tools (`allowed-tools`), its
+hard action requirements (`requires-actions`), and everything else the
+orchestrator gates on. A block that fails to parse is not an error the model
+sees — the skill loads owning nothing *and* the raw frontmatter text lands in
+the rendered procedure. Two tolerances exist so that stays unlikely:
+
+- **Leading characters are ignored.** A blank first line, an indent, or a UTF-8
+  BOM (invisible; Notepad and some Office exports add one) no longer pushes the
+  opening `---` out of position.
+- **Underscores are accepted for every known key.** `allowed_tools` is read as
+  `allowed-tools` and logged at INFO. Hyphens remain canonical, and win if a
+  file somehow carries both.
+
+A key that is *nearly* a known one — `allowed-tool`, say — is still ignored,
+but now logs a WARNING naming what it was probably meant to be. Unrecognized
+keys that resemble nothing are passed through untouched.
+
 ### Skill commands
 
 ```bash

jvagent/scaffold/skill_resolve.py

@@ -6,6 +6,7 @@
 import importlib
 import logging
 import os
+from difflib import get_close_matches
 from pathlib import Path
 from typing import Any, Dict, List, Optional, Tuple, Union
 
@@ -23,13 +24,98 @@
 SELECTOR_ALL = "-all"
 
 
+_KNOWN_FRONTMATTER_KEYS = frozenset(
+    {
+        "allowed-channels",
+        "allowed-tools",
+        "always-active",
+        "coactivate-with",
+        "denied-channels",
+        "deny-access-directive",
+        "dependencies",
+        "description",
+        "disabled-tools",
+        "dispatch",
+        "exports",
+        "extends",
+        "imports",
+        "interview",
+        "license",
+        "lock-companions",
+        "name",
+        "parameters",
+        "requires-actions",
+        "requires-jvagent",
+        "requires-tasks",
+        "spec",
+        "tags",
+        "task-lock",
+        "verbatim-final",
+        "version",
+    }
+)
+
+
+def _normalize_frontmatter_keys(
+    meta: Dict[str, Any], skill_path: Path
+) -> Dict[str, Any]:
+    """Accept underscore spellings, and name a key that is nearly right.
+
+    A handful of keys already tolerated both spellings (``task_lock``,
+    ``allowed_channels``, …) while the rest did not, so ``allowed_tools``
+    parsed to nothing and the skill loaded owning no tools — silently, and
+    indistinguishably from a skill that declares none. Underscores are now
+    accepted for every known key, and a near-miss is logged rather than
+    dropped: a declaration that does not take effect should say so.
+    """
+    if not meta:
+        return meta
+    out: Dict[str, Any] = {}
+    for key, value in meta.items():
+        name = str(key)
+        if name in _KNOWN_FRONTMATTER_KEYS:
+            out[name] = value
+            continue
+        hyphenated = name.replace("_", "-").lower()
+        if hyphenated in _KNOWN_FRONTMATTER_KEYS:
+            # Silent acceptance would hide a real inconsistency; say what was
+            # assumed so the file can be corrected.
+            logger.info(
+                "Skill %s: frontmatter key %r read as %r",
+                skill_path,
+                name,
+                hyphenated,
+            )
+            out.setdefault(hyphenated, value)
+            continue
+        close = get_close_matches(hyphenated, sorted(_KNOWN_FRONTMATTER_KEYS), 1, 0.8)
+        if close:
+            logger.warning(
+                "Skill %s: unknown frontmatter key %r is ignored — did you mean "
+                "%r? Nothing it declares takes effect.",
+                skill_path,
+                name,
+                close[0],
+            )
+        out[name] = value
+    return out
+
+
 def _parse_frontmatter(raw: str, skill_path: Path) -> Tuple[Dict[str, Any], str]:
-    """Parse optional YAML frontmatter and return (meta, content)."""
-    content = raw.strip()
-    if not raw.startswith("---"):
+    """Parse optional YAML frontmatter and return (meta, content).
+
+    The delimiter check runs on the *stripped* text, and a UTF-8 BOM is
+    removed first. Testing ``raw`` directly meant a leading blank line, a
+    stray indent, or an editor-inserted BOM made the whole frontmatter block
+    parse as body: ``allowed-tools`` was then never read (so the skill owned
+    no tools) *and* the frontmatter text reached the rendered procedure. Both
+    halves are silent. ``sop_extend`` already strips before this check.
+    """
+    content = raw.lstrip("").strip()
+    if not content.startswith("---"):
         return {}, content
 
-    parts = raw.split("---", 2)
+    parts = content.split("---", 2)
     if len(parts) < 3:
         raise ValueError(f"Invalid frontmatter format in {skill_path}")
 
@@ -39,7 +125,7 @@ def _parse_frontmatter(raw: str, skill_path: Path) -> Tuple[Dict[str, Any], str]
     if not isinstance(parsed, dict):
         raise ValueError(f"Frontmatter must be a YAML mapping in {skill_path}")
 
-    return parsed, parts[2].strip()
+    return _normalize_frontmatter_keys(parsed, skill_path), parts[2].strip()
 
 
 def _normalize_allowed_tools(raw_value: Any, skill_path: Path) -> List[str]:

tests/scaffold/test_skill_resolve.py

@@ -4,6 +4,8 @@
 
 from pathlib import Path
 
+import pytest
+
 from jvagent.scaffold.skill_resolve import (
     apply_skill_selector,
     resolve_agent_skills,
@@ -238,3 +240,193 @@ def test_parse_skill_bundle_requires_actions_string_form(tmp_path: Path) -> None
     data = parse_skill_bundle(skill_dir, source="builtin")
     assert data is not None
     assert data["requires_actions"] == ["GoogleCalendarAction"]
+
+
+class TestFrontmatterLeadingCharacters:
+    """A SKILL.md whose ``---`` is not the very first byte.
+
+    ``_parse_frontmatter`` tested ``raw.startswith("---")`` on the UNSTRIPPED
+    text while using the stripped text for everything else. A leading blank
+    line, indentation, or an editor-inserted UTF-8 BOM therefore made the whole
+    frontmatter block read as body, which fails twice over:
+
+    - ``allowed-tools`` is never parsed, so the skill declares no tools and
+      cannot own anything ``skill_only_tools`` gates
+    - the frontmatter text lands in the rendered procedure, so ``allowed-tools:``
+      shows up verbatim in the prompt
+
+    ``sop_extend.py`` already strips before the check; this is the outlier.
+    """
+
+    BODY = (
+        "---\n"
+        "name: doc_helper\n"
+        "description: helps with docs\n"
+        "allowed-tools:\n"
+        "  - pageindex__search\n"
+        "  - pageindex__list\n"
+        "---\n"
+        "\n"
+        "# Procedure\n"
+        "Do the thing.\n"
+    )
+
+    @pytest.mark.parametrize(
+        "label,prefix",
+        [
+            ("leading newline", "\n"),
+            ("leading blank lines", "\n\n"),
+            ("leading spaces", "  "),
+            ("utf-8 bom", ""),
+            ("bom then newline", "\n"),
+        ],
+    )
+    def test_frontmatter_is_parsed_despite_leading_characters(
+        self, label: str, prefix: str
+    ) -> None:
+        from jvagent.scaffold.skill_resolve import _parse_frontmatter
+
+        meta, content = _parse_frontmatter(prefix + self.BODY, Path("SKILL.md"))
+        assert meta.get("allowed-tools") == [
+            "pageindex__search",
+            "pageindex__list",
+        ], f"{label}: allowed-tools must still be parsed"
+        assert (
+            "allowed-tools" not in content
+        ), f"{label}: frontmatter must not leak into the rendered body"
+        assert content.startswith("# Procedure"), f"{label}: body should start clean"
+
+    def test_clean_file_still_parses(self) -> None:
+        from jvagent.scaffold.skill_resolve import _parse_frontmatter
+
+        meta, content = _parse_frontmatter(self.BODY, Path("SKILL.md"))
+        assert meta["name"] == "doc_helper"
+        assert content.startswith("# Procedure")
+
+    def test_body_horizontal_rule_is_not_a_delimiter(self) -> None:
+        """``---`` as a markdown rule in the body must survive intact."""
+        from jvagent.scaffold.skill_resolve import _parse_frontmatter
+
+        raw = self.BODY + "\nSection\n\n---\n\nMore\n"
+        _meta, content = _parse_frontmatter(raw, Path("SKILL.md"))
+        assert "---" in content
+        assert content.rstrip().endswith("More")
+
+    def test_file_without_frontmatter_is_untouched(self) -> None:
+        from jvagent.scaffold.skill_resolve import _parse_frontmatter
+
+        meta, content = _parse_frontmatter("# Just a doc\n\nbody\n", Path("SKILL.md"))
+        assert meta == {}
+        assert content == "# Just a doc\n\nbody"
+
+
+class TestFrontmatterKeySpelling:
+    """Underscore spellings of hyphenated frontmatter keys.
+
+    ``task_lock``, ``allowed_channels``, ``requires_tasks``,
+    ``lock_companions`` and ``deny_access_directive`` were each given an
+    explicit underscore fallback at their read site, but ``allowed_tools`` and
+    the rest were not. The inconsistency is invisible: a skill written with
+    ``allowed_tools:`` parses fine, loads fine, and owns no tools — the same end
+    state as the BOM bug, reached a different way. Underscores are now accepted
+    for every known key.
+    """
+
+    @staticmethod
+    def _write(tmp_path: Path, frontmatter: str) -> Path:
+        skill_dir = tmp_path / "spelling_skill"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text(
+            f"---\n{frontmatter}---\n\n# Procedure\nDo the thing.\n",
+            encoding="utf-8",
+        )
+        return skill_dir
+
+    def test_allowed_tools_underscore_is_accepted(self, tmp_path: Path) -> None:
+        from jvagent.scaffold.skill_resolve import parse_skill_bundle
+
+        skill_dir = self._write(
+            tmp_path,
+            "name: doc_helper\n"
+            "description: helps with docs\n"
+            "allowed_tools:\n"
+            "  - pageindex__search\n"
+            "  - pageindex__list\n",
+        )
+        data = parse_skill_bundle(skill_dir, source="builtin")
+        assert data is not None
+        assert data["allowed_tools"] == ["pageindex__search", "pageindex__list"]
+
+    @pytest.mark.parametrize(
+        "written,canonical",
+        [
+            ("disabled_tools", "disabled_tools"),
+            ("requires_actions", "requires_actions"),
+            ("coactivate_with", "coactivate_with"),
+        ],
+    )
+    def test_other_list_keys_accept_underscores(
+        self, tmp_path: Path, written: str, canonical: str
+    ) -> None:
+        from jvagent.scaffold.skill_resolve import parse_skill_bundle
+
+        skill_dir = self._write(
+            tmp_path,
+            f"name: doc_helper\ndescription: d\n{written}:\n  - alpha\n",
+        )
+        data = parse_skill_bundle(skill_dir, source="builtin")
+        assert data is not None
+        assert data[canonical] == ["alpha"]
+
+    @pytest.mark.parametrize(
+        "block",
+        [
+            "allowed_tools:\n  - underscore_tool\nallowed-tools:\n  - hyphen_tool\n",
+            "allowed-tools:\n  - hyphen_tool\nallowed_tools:\n  - underscore_tool\n",
+        ],
+        ids=["underscore-first", "hyphen-first"],
+    )
+    def test_hyphenated_spelling_wins_when_both_are_present(
+        self, tmp_path: Path, block: str
+    ) -> None:
+        """A file carrying both spellings must not depend on YAML key order."""
+        from jvagent.scaffold.skill_resolve import parse_skill_bundle
+
+        skill_dir = self._write(tmp_path, f"name: doc_helper\ndescription: d\n{block}")
+        data = parse_skill_bundle(skill_dir, source="builtin")
+        assert data is not None
+        assert data["allowed_tools"] == ["hyphen_tool"]
+
+    def test_near_miss_key_is_reported(self, caplog: pytest.LogCaptureFixture) -> None:
+        """A typo'd key is still ignored — but it no longer goes unmentioned."""
+        import logging
+
+        from jvagent.scaffold.skill_resolve import _parse_frontmatter
+
+        with caplog.at_level(logging.WARNING, logger="jvagent.scaffold.skill_resolve"):
+            meta, _content = _parse_frontmatter(
+                "---\nname: s\nallowed-tool:\n  - a\n---\n\nbody\n",
+                Path("SKILL.md"),
+            )
+        assert "allowed-tool" in meta, "unknown keys are preserved, not dropped"
+        assert "allowed-tools" not in meta, "a typo must not silently take effect"
+        assert any(
+            "allowed-tool" in record.message and "allowed-tools" in record.message
+            for record in caplog.records
+        ), "the near-miss should name the key it was probably meant to be"
+
+    def test_unrelated_unknown_key_is_left_alone(
+        self, caplog: pytest.LogCaptureFixture
+    ) -> None:
+        """Custom keys are legal; only near-misses are worth a warning."""
+        import logging
+
+        from jvagent.scaffold.skill_resolve import _parse_frontmatter
+
+        with caplog.at_level(logging.WARNING, logger="jvagent.scaffold.skill_resolve"):
+            meta, _content = _parse_frontmatter(
+                "---\nname: s\nteam_owner: platform\n---\n\nbody\n",
+                Path("SKILL.md"),
+            )
+        assert meta["team_owner"] == "platform"
+        assert not caplog.records