From 7ea7edda8ae2f35d5048fe196a208664d7c1c1b4 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Sat, 8 Aug 2026 09:28:07 -0400 Subject: [PATCH 1/2] fix(skills): parse SKILL.md frontmatter when --- is not the first byte _parse_frontmatter tested the delimiter on the unstripped text while using the stripped text for everything else: content = raw.strip() if not raw.startswith("---"): return {}, content So a SKILL.md beginning with a blank line, an indent, or an editor-inserted UTF-8 BOM parsed as having no frontmatter at all, and the whole block was returned as body. Both consequences are silent, and they compound: - allowed-tools is never read, so SkillDoc.requires_tools is empty and the skill owns nothing. Any tool named in skill_only_tools then has no owning skill and fails closed -- the gate looks broken when the declaration simply never loaded. - the frontmatter text lands in SkillDoc.body, which is rendered into the prompt's skills section, so "allowed-tools:" appears verbatim to the model. That is the reported symptom exactly: allowed-tools showing up in the prompt while skill gating misbehaves. Reproduced with a real four-tool declaration -- leading newline, leading spaces and BOM each yield zero parsed tools and a leaked block; clean and CRLF files are unaffected. Strip the BOM, then test and split the stripped text. sop_extend.py already strips before its check; this was the outlier. Tests written first: five parametrised leading-character cases, red before the fix, plus controls for a clean file, a --- horizontal rule inside the body, and a file with no frontmatter, so the fix cannot over-reach. --- jvagent/scaffold/skill_resolve.py | 16 ++++-- tests/scaffold/test_skill_resolve.py | 80 ++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/jvagent/scaffold/skill_resolve.py b/jvagent/scaffold/skill_resolve.py index c3eb8b9a..a9f5439f 100644 --- a/jvagent/scaffold/skill_resolve.py +++ b/jvagent/scaffold/skill_resolve.py @@ -24,12 +24,20 @@ 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}") diff --git a/tests/scaffold/test_skill_resolve.py b/tests/scaffold/test_skill_resolve.py index 09d0d3c4..4fc711ea 100644 --- a/tests/scaffold/test_skill_resolve.py +++ b/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,81 @@ 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" From 0abbf2ebbe476cc88a1481f23d65145ec1230608 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Sat, 8 Aug 2026 09:49:57 -0400 Subject: [PATCH 2/2] fix(skills): accept underscore frontmatter keys, warn on near-misses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BOM/leading-whitespace fix in 7ea7edda closed one way for a SKILL.md frontmatter block to be dropped without a word. This closes the other. Six keys had a hand-written underscore fallback at their read site (`task_lock`, `lock_companions`, `requires_tasks`, `allowed_channels`, `denied_channels`, `deny_access_directive` — skill_resolve.py:311-372) and the remaining twenty did not. `allowed_tools:` therefore parsed to nothing: the skill loaded, exposed no tools, and owned nothing `skill_only_tools` gates — indistinguishable from a skill that declares none, with no warning anywhere. Same end state as the BOM bug, reached a different way. `_normalize_frontmatter_keys()` now runs on the parsed mapping before `_parse_frontmatter` returns: - underscores accepted for every key in `_KNOWN_FRONTMATTER_KEYS` (26 keys, enumerated from the `frontmatter.get(...)` read sites), logged at INFO so the assumption is visible and the file can be corrected - hyphenated spelling wins when a file carries both, independent of YAML key order - an unknown key within difflib ratio 0.8 of a known one (`allowed-tool`) is still ignored, but logs a WARNING naming the key it was probably meant to be. Unrecognized keys resembling nothing are passed through untouched, so custom frontmatter stays legal and silent. Tests mutation-checked three ways: unwiring the normalizer, replacing `setdefault` with plain assignment, and disabling the near-miss lookup each turn the relevant cases red while the controls stay green. docs/scaffolding.md documents both tolerances under skill discovery. --- docs/scaffolding.md | 19 +++++ jvagent/scaffold/skill_resolve.py | 80 ++++++++++++++++++- tests/scaffold/test_skill_resolve.py | 112 +++++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 1 deletion(-) diff --git a/docs/scaffolding.md b/docs/scaffolding.md index 59cd70ba..aec67c6f 100644 --- a/docs/scaffolding.md +++ b/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 diff --git a/jvagent/scaffold/skill_resolve.py b/jvagent/scaffold/skill_resolve.py index a9f5439f..d7317fe4 100644 --- a/jvagent/scaffold/skill_resolve.py +++ b/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,6 +24,83 @@ 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). @@ -47,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]: diff --git a/tests/scaffold/test_skill_resolve.py b/tests/scaffold/test_skill_resolve.py index 4fc711ea..cb010b4d 100644 --- a/tests/scaffold/test_skill_resolve.py +++ b/tests/scaffold/test_skill_resolve.py @@ -318,3 +318,115 @@ def test_file_without_frontmatter_is_untouched(self) -> None: 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