mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-22 16:25:58 +00:00
fix(skills): strip UTF-8 BOM before parsing SKILL.md frontmatter
A UTF-8 BOM saved into a SKILL.md (e.g. Notepad or PowerShell `>`) is kept by
read_text(encoding="utf-8"), so the string handed to parse_frontmatter starts
with the BOM and the startswith("---") fence check fails. The whole frontmatter
is then silently dropped: the skill loads with no name/description, `platforms`
gating falls open (a macOS-only skill becomes visible everywhere), and
required_environment_variables / metadata.hermes.config setup never fires.
Strip a single leading BOM at the top of parse_frontmatter, the shared
chokepoint for every local skill-loading path (_parse_skill_file,
discover_all_skill_config_vars, DESCRIPTION.md parsing, _inject_skill_config,
and the tools/skills_tool._parse_frontmatter re-export), so the whole class is
covered, not just the reported site. Only the leading marker is removed; a BOM
mid-content is left as data. Mirrors the existing file-tools BOM handling
(#35278) and CONTRIBUTING.md "File encoding".
Adds tests: BOM'd frontmatter parses identically to plain, the body is
BOM-free, platforms gating and config-var extraction survive a BOM, and an
end-to-end BOM-write / plain-read round trip (mirroring _parse_skill_file).
This commit is contained in:
parent
f28248c662
commit
a4ecb3da9a
3 changed files with 111 additions and 0 deletions
|
|
@ -126,10 +126,22 @@ def parse_frontmatter(content: str) -> Tuple[Dict[str, Any], str]:
|
|||
Uses yaml with CSafeLoader for full YAML support (nested metadata, lists)
|
||||
with a fallback to simple key:value splitting for robustness.
|
||||
|
||||
A single leading UTF-8 BOM (U+FEFF) is stripped before parsing. Windows
|
||||
GUI editors (Notepad, PowerShell ``>``) prepend one when saving a SKILL.md
|
||||
as UTF-8, and ``read_text(encoding="utf-8")`` preserves it (only
|
||||
``utf-8-sig`` strips it). Left in place, the BOM defeats the ``---`` fence
|
||||
check below and the whole frontmatter is silently discarded — name,
|
||||
description, ``platforms`` gating, env-var setup, and conditional
|
||||
activation all vanish. See CONTRIBUTING.md "File encoding".
|
||||
|
||||
Returns:
|
||||
(frontmatter_dict, remaining_body)
|
||||
"""
|
||||
frontmatter: Dict[str, Any] = {}
|
||||
|
||||
# Strip only a leading BOM; a BOM mid-content is data, not a marker.
|
||||
if content.startswith("\ufeff"):
|
||||
content = content[1:]
|
||||
body = content
|
||||
|
||||
if not content.startswith("---"):
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@
|
|||
from unittest.mock import patch
|
||||
|
||||
from agent.skill_utils import (
|
||||
extract_skill_config_vars,
|
||||
extract_skill_conditions,
|
||||
get_disabled_skill_names,
|
||||
get_external_skills_dirs,
|
||||
|
|
@ -10,6 +11,7 @@ from agent.skill_utils import (
|
|||
is_external_skill_path,
|
||||
is_skill_support_path,
|
||||
iter_skill_index_files,
|
||||
parse_frontmatter,
|
||||
resolve_skill_config_values,
|
||||
skill_matches_platform,
|
||||
skill_matches_platform_list,
|
||||
|
|
@ -386,3 +388,89 @@ class TestNormalizeSkillLookupName:
|
|||
monkeypatch.setattr("agent.skill_utils.get_skills_dir", lambda: tmp_path / "skills")
|
||||
outside = str(tmp_path / "outside" / "skill")
|
||||
assert normalize_skill_lookup_name(outside) == outside
|
||||
|
||||
|
||||
# ── parse_frontmatter: UTF-8 BOM tolerance ─────────────────────────────────
|
||||
|
||||
|
||||
class TestParseFrontmatterBOM:
|
||||
"""A UTF-8 BOM (U+FEFF) on a Windows-saved SKILL.md must not defeat
|
||||
frontmatter parsing.
|
||||
|
||||
Notepad and PowerShell ``>`` prepend a BOM when saving UTF-8;
|
||||
``read_text(encoding="utf-8")`` (what ``_parse_skill_file`` uses) keeps
|
||||
it, so the bytes handed to ``parse_frontmatter`` start with a BOM ahead of
|
||||
the ``---`` fence. Before the fix the ``startswith("---")`` check returned
|
||||
False and the whole frontmatter was silently dropped — the skill loaded
|
||||
nameless, platform gating fell open, and env-var/config setup never fired.
|
||||
"""
|
||||
|
||||
SKILL = (
|
||||
"---\n"
|
||||
"name: my-skill\n"
|
||||
"description: Does a thing.\n"
|
||||
"platforms: [macos]\n"
|
||||
"metadata:\n"
|
||||
" hermes:\n"
|
||||
" config:\n"
|
||||
" - key: my.key\n"
|
||||
" description: A configured value\n"
|
||||
"---\n\n"
|
||||
"# My Skill\n\nBody text.\n"
|
||||
)
|
||||
|
||||
def test_bom_frontmatter_matches_plain(self):
|
||||
plain_fm, plain_body = parse_frontmatter(self.SKILL)
|
||||
bom_fm, bom_body = parse_frontmatter("\ufeff" + self.SKILL)
|
||||
assert bom_fm == plain_fm
|
||||
assert bom_body == plain_body
|
||||
assert bom_fm["name"] == "my-skill"
|
||||
assert bom_fm["description"] == "Does a thing."
|
||||
|
||||
def test_bom_body_has_no_leading_marker(self):
|
||||
_, body = parse_frontmatter("\ufeff" + self.SKILL)
|
||||
assert not body.startswith("\ufeff")
|
||||
assert body.lstrip().startswith("# My Skill")
|
||||
|
||||
def test_bom_without_frontmatter_strips_marker(self):
|
||||
# A BOM'd file with no frontmatter still gets the invisible marker
|
||||
# removed from the body so it never reaches the system prompt.
|
||||
fm, body = parse_frontmatter("\ufeff# Heading\nText.\n")
|
||||
assert fm == {}
|
||||
assert body == "# Heading\nText.\n"
|
||||
|
||||
def test_interior_bom_is_preserved(self):
|
||||
# Only the leading marker is stripped; a U+FEFF in the body is data.
|
||||
fm, body = parse_frontmatter("\ufeff---\nname: x\n---\nbo\ufeffdy\n")
|
||||
assert fm["name"] == "x"
|
||||
assert "\ufeff" in body
|
||||
|
||||
def test_bom_platform_gating_regression(self):
|
||||
# The concrete harm: a macOS-only skill must stay hidden on non-macOS
|
||||
# whether or not the file carries a BOM. Empty frontmatter (the bug)
|
||||
# reads as "no platform restriction" and leaks the skill everywhere.
|
||||
with patch("agent.skill_utils.sys.platform", "win32"), patch(
|
||||
"agent.skill_utils.is_termux", return_value=False
|
||||
):
|
||||
plain_fm, _ = parse_frontmatter(self.SKILL)
|
||||
bom_fm, _ = parse_frontmatter("\ufeff" + self.SKILL)
|
||||
assert skill_matches_platform(plain_fm) is False
|
||||
assert skill_matches_platform(bom_fm) is False
|
||||
|
||||
def test_bom_config_vars_preserved(self):
|
||||
# metadata.hermes.config drives secure setup-on-load; it must survive
|
||||
# a BOM so Windows users still get prompted for the value.
|
||||
bom_fm, _ = parse_frontmatter("\ufeff" + self.SKILL)
|
||||
assert [v["key"] for v in extract_skill_config_vars(bom_fm)] == ["my.key"]
|
||||
|
||||
def test_real_file_read_path(self, tmp_path):
|
||||
# End-to-end: write the file the way a Windows editor does (utf-8-sig
|
||||
# emits a BOM), read it the way _parse_skill_file does (plain utf-8),
|
||||
# and confirm the frontmatter survives the round trip.
|
||||
f = tmp_path / "SKILL.md"
|
||||
f.write_text(self.SKILL, encoding="utf-8-sig")
|
||||
raw = f.read_text(encoding="utf-8")
|
||||
assert raw.startswith("\ufeff") # BOM really is present on disk
|
||||
fm, _ = parse_frontmatter(raw)
|
||||
assert fm["name"] == "my-skill"
|
||||
assert fm["platforms"] == ["macos"]
|
||||
|
|
|
|||
|
|
@ -94,6 +94,17 @@ class TestParseFrontmatter:
|
|||
# Should still parse what it can via fallback
|
||||
assert "name" in fm
|
||||
|
||||
def test_utf8_bom_frontmatter(self):
|
||||
"""A leading UTF-8 BOM (Windows Notepad / PowerShell ``>`` save) must
|
||||
not drop the frontmatter. Confirms the fix reaches the tools/ surface
|
||||
via the _parse_frontmatter re-export."""
|
||||
bom = chr(0xFEFF)
|
||||
content = bom + "---\nname: test\ndescription: A test.\n---\n\n# Body\n"
|
||||
fm, body = _parse_frontmatter(content)
|
||||
assert fm["name"] == "test"
|
||||
assert fm["description"] == "A test."
|
||||
assert not body.startswith(bom)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _parse_tags
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue