diff --git a/fastmcp_slim/fastmcp/server/providers/skills/_common.py b/fastmcp_slim/fastmcp/server/providers/skills/_common.py index c752d4791..8f1d280bb 100644 --- a/fastmcp_slim/fastmcp/server/providers/skills/_common.py +++ b/fastmcp_slim/fastmcp/server/providers/skills/_common.py @@ -66,9 +66,16 @@ def _parse_frontmatter_line_based(frontmatter_text: str) -> dict[str, Any]: def parse_frontmatter(content: str) -> tuple[dict[str, Any], str]: """Parse YAML frontmatter from markdown content. - Prefers full YAML parsing so multiline block scalars (`|` / `>`) work. - If YAML parsing fails (invalid YAML, recursion limit, etc.), falls back to - a line-based key:value parser so plain frontmatter is not discarded. + Uses `yaml.BaseLoader` so multiline block scalars (`|` / `>`) work while + every scalar value is returned as a plain string. `SkillInfo.frontmatter` + is a public mapping; letting a full loader (e.g. `safe_load`) apply + implicit scalar typing would silently turn `version: 1.10` into the float + `1.1`, `enabled: yes` into `True`, or a date-like value into + `datetime.date` for every skill author, which is a backwards-incompatible + surprise rather than an internal detail. `BaseLoader` has no such + constructors, so values round-trip as written. If YAML parsing fails + (invalid YAML, recursion limit, etc.), falls back to a line-based + key:value parser so plain frontmatter is not discarded. Args: content: Markdown content potentially starting with --- @@ -93,7 +100,7 @@ def parse_frontmatter(content: str) -> tuple[dict[str, Any], str]: remaining = content[3 + end_match.end() :] try: - parsed = yaml.safe_load(frontmatter_text) + parsed = yaml.load(frontmatter_text, Loader=yaml.BaseLoader) except (yaml.YAMLError, RecursionError): # Prefer partial recovery over discarding every key (issue #4416 review). return _parse_frontmatter_line_based(frontmatter_text), remaining @@ -101,17 +108,6 @@ def parse_frontmatter(content: str) -> tuple[dict[str, Any], str]: if not isinstance(parsed, dict): return {}, remaining - # YAML may type `description: true` / dates as non-str, but SkillInfo.description - # is a str field that callers truthiness-check. - if "description" in parsed and parsed["description"] is not None: - if not isinstance(parsed["description"], str): - # YAML 1.1 coerces bare words like `yes` to non-str; recover the - # original text via the line-based parser instead. - raw = _parse_frontmatter_line_based(frontmatter_text).get("description") - parsed["description"] = ( - raw if isinstance(raw, str) else str(parsed["description"]) - ) - return parsed, remaining diff --git a/tests/server/providers/test_skills_provider.py b/tests/server/providers/test_skills_provider.py index 6310e6609..c1583c9ee 100644 --- a/tests/server/providers/test_skills_provider.py +++ b/tests/server/providers/test_skills_provider.py @@ -130,27 +130,37 @@ Body assert frontmatter == {} assert body == "Body\n" - def test_frontmatter_numeric_scalars_are_typed(self): - """yaml.safe_load applies YAML typing to unquoted scalars. + def test_frontmatter_scalars_are_preserved_as_strings(self): + """`SkillInfo.frontmatter` is public; it must not silently retype values. - `version` is never read downstream (only `description` is, in - skill_provider.py), so this typed behavior is safe today, but it IS - a change from the old parser, which always kept every value as a - string. This pins the actual coerced types so a regression is - caught if that assumption ever changes. + `yaml.BaseLoader` parses block scalars (unlike the old line parser) + but applies no implicit scalar resolution, so every value comes back + exactly as written instead of being coerced to float/bool/date the + way a full loader (e.g. `safe_load`) would. + + The `description: null` case looks surprising but is intentional: + under `BaseLoader`, YAML's null token resolves to the *string* + `"null"`, not Python `None`. This matches the pre-existing line-based + parser (which did `value.strip()` on the raw text and also produced + `"null"`), so this restores original behavior rather than inventing + a new one. Do not "fix" this back to `None`. """ content = """--- -version: 1.0 -enabled: true +version: 1.10 +enabled: yes +released: 2024-01-01 +description: null --- Body """ frontmatter, body = parse_frontmatter(content) - assert frontmatter["version"] == 1.0 - assert isinstance(frontmatter["version"], float) - assert frontmatter["enabled"] is True - assert isinstance(frontmatter["enabled"], bool) + assert frontmatter["version"] == "1.10" + assert frontmatter["enabled"] == "yes" + assert frontmatter["released"] == "2024-01-01" + assert frontmatter["description"] == "null" + for value in frontmatter.values(): + assert isinstance(value, str) def test_frontmatter_description_bool_recovers_original_text(self): content = """---