Merge ff2138d837 into 20e7fc0164

services/memory/skill_format.py

@@ -100,6 +100,18 @@ def _parse_scalar(raw: str) -> Any:
     if raw.lower() in ("null", "none", "~"):
         return None
     if (raw[0] == raw[-1]) and raw[0] in ("'", '"'):
+        if raw[0] == '"':
+            # _emit_scalar writes double-quoted scalars with json.dumps, so
+            # decode the escapes instead of only stripping the quotes. Without
+            # this, `\"` / `\\` / `\uXXXX` stayed verbatim in the value and the
+            # next save escaped their backslashes again, doubling them on every
+            # load/save cycle (issue #5210).
+            try:
+                return json.loads(raw)
+            except ValueError:
+                # Hand-written file using escapes JSON rejects (e.g. a bare
+                # Windows path). Keep the previous literal reading.
+                pass
         return raw[1:-1]
     # Try number
     try:
@@ -171,6 +183,26 @@ def parse_frontmatter(text: str) -> tuple[Dict[str, Any], str]:
     return fm, body
 
 
+# Characters that force a quoted scalar. The punctuation would otherwise change
+# how the value reads back; the second row is every character str.splitlines()
+# treats as a line break, and parse_frontmatter() reads one scalar per line, so
+# emitting one of those bare would split the value across lines.
+_FM_MUST_QUOTE = (
+    ":", "#", "[", "]", "{", "}", ",", "&", "*", "!", "|", ">", "'", '"', "%", "@",
+    "\n", "\r", "\v", "\f", "\x1c", "\x1d", "\x1e", "\x85", "\u2028", "\u2029",
+)
+
+# json.dumps escapes every C0 control character, but with ensure_ascii=False it
+# passes NEL / LINE SEPARATOR / PARAGRAPH SEPARATOR through literally, and
+# str.splitlines() still breaks on all three. Re-escape exactly those, which
+# json.loads decodes again on the way in, so the pair stays symmetric.
+_FM_POST_DUMPS_ESCAPES = (
+    ("\x85", "\\u0085"),
+    ("\u2028", "\\u2028"),
+    ("\u2029", "\\u2029"),
+)
+
+
 def _emit_scalar(v: Any) -> str:
     if v is None:
         return "null"
@@ -181,8 +213,15 @@ def _emit_scalar(v: Any) -> str:
     if isinstance(v, list):
         return "[" + ", ".join(_emit_scalar(x) for x in v) + "]"
     s = str(v)
-    if any(c in s for c in (":", "#", "\n", "[", "]", "{", "}", ",", "&", "*", "!", "|", ">", "'", '"', "%", "@")):
-        return json.dumps(s)
+    if any(c in s for c in _FM_MUST_QUOTE):
+        # ensure_ascii=False keeps non-ASCII text as itself. SKILL.md is UTF-8 at
+        # both ends (skills.py reads it, atomic_write_text writes it), so the
+        # \uXXXX form bought nothing and leaked into the parsed value (#5210).
+        out = json.dumps(s, ensure_ascii=False)
+        for ch, esc in _FM_POST_DUMPS_ESCAPES:
+            if ch in out:
+                out = out.replace(ch, esc)
+        return out
     return s
 
 

tests/test_skill_frontmatter_escape_roundtrip.py

@@ -0,0 +1,214 @@
+"""Regression for issue #5210 — SKILL.md frontmatter scalars must round-trip.
+
+``_emit_scalar`` quotes a scalar with ``json.dumps`` whenever it contains
+punctuation that would change how the line reads back. ``_parse_scalar`` used
+to undo that with a bare ``raw[1:-1]``: it stripped the quotes but never
+decoded the escapes. So ``"Pr\\u00fcfung"`` was read back as the literal text
+``Pr\\u00fcfung``, and the next save escaped *that* backslash again.
+
+The damage compounds — each save doubles the backslash run — so a German or
+Japanese skill description degrades into backslash noise after a handful of
+edits, and the same happens to a plain-ASCII description that merely contains
+a quote character. The escapes are also shown verbatim in the skills list and
+the ``/skills`` catalog.
+
+The fix is symmetric: emit with ``ensure_ascii=False`` (SKILL.md is UTF-8 at
+both ends) and parse double-quoted scalars with ``json.loads``.
+"""
+
+import json
+
+import pytest
+
+from services.memory.skill_format import (
+    Skill,
+    _emit_scalar,
+    _parse_scalar,
+    parse_frontmatter,
+)
+from services.memory.skills import SkillsManager
+
+# Umlauts plus a comma — the comma is what forces the quoted form, which is the
+# only path that was corrupted. Reported verbatim in issue #5210.
+GERMAN = "Einstiegs- und Pr\u00fcfungslinie f\u00fcr AGB, Datenschutz"
+JAPANESE = "\u30b9\u30ad\u30eb: \u30c6\u30b9\u30c8\u7528\u306e\u8aac\u660e"
+QUOTED_ASCII = 'Use the "grep" tool, then summarise'
+
+
+def _cycle(skill: Skill, times: int = 1) -> Skill:
+    """Save to markdown and read it straight back, `times` times over."""
+    for _ in range(times):
+        skill = Skill.from_markdown(skill.to_markdown())
+    return skill
+
+
+# ---------------------------------------------------------------------------
+# The reported corruption
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    "description",
+    [GERMAN, JAPANESE, QUOTED_ASCII],
+    ids=["german", "japanese", "quoted-ascii"],
+)
+def test_description_survives_repeated_saves(description):
+    """Five load/save cycles must leave the text byte-identical.
+
+    One cycle is enough to corrupt it; five is where the doubling became
+    obvious in the field.
+    """
+    result = _cycle(Skill(name="demo", description=description), times=5)
+    assert result.description == description
+
+
+def test_corruption_does_not_compound_across_saves():
+    """Pin the *growth*, not just the mismatch.
+
+    The original defect was not a one-off mangling — the escaped form was
+    re-escaped on every save, so the value grew without bound. A regression
+    that reintroduced single-level mangling would still be caught by the test
+    above; this one catches the runaway specifically.
+    """
+    skill = Skill(name="demo", description=GERMAN)
+    lengths = []
+    for _ in range(5):
+        skill = _cycle(skill)
+        lengths.append(len(skill.description))
+    assert len(set(lengths)) == 1, f"description length drifted across saves: {lengths}"
+
+
+# ---------------------------------------------------------------------------
+# What actually lands on disk
+# ---------------------------------------------------------------------------
+
+
+def test_non_ascii_is_written_as_utf8_not_ascii_escapes():
+    """SKILL.md is opened as UTF-8 at both ends, so \\uXXXX buys nothing and
+    only makes the file unreadable to a human editing it."""
+    markdown = Skill(name="demo", description=GERMAN).to_markdown()
+    line = next(l for l in markdown.splitlines() if l.startswith("description:"))
+    assert "Pr\u00fcfungslinie" in line
+    assert "\\u00fc" not in line
+
+
+def test_quoted_scalar_is_valid_json():
+    """The emitted form is what the parser now feeds to json.loads, so the two
+    halves cannot drift apart without this failing."""
+    emitted = _emit_scalar(QUOTED_ASCII)
+    assert json.loads(emitted) == QUOTED_ASCII
+
+
+# ---------------------------------------------------------------------------
+# Existing files
+# ---------------------------------------------------------------------------
+
+
+def test_legacy_ascii_escaped_file_is_read_correctly():
+    """Files already written by the old emitter hold real JSON escapes, so the
+    new parser recovers the intended text instead of the escape source."""
+    markdown = '---\nname: demo\ndescription: "Pr\\u00fcfung, x"\n---\n\n'
+    assert Skill.from_markdown(markdown).description == "Pr\u00fcfung, x"
+
+
+def test_already_corrupted_file_heals_one_level_per_load():
+    """A file that took one round of damage carries a doubled backslash. That
+    is still valid JSON, so reading it yields the single-backslash form and the
+    value stops degrading."""
+    markdown = '---\nname: demo\ndescription: "Pr\\\\u00fcfung, x"\n---\n\n'
+    once = Skill.from_markdown(markdown)
+    assert once.description == "Pr\\u00fcfung, x"
+    # And it is now stable rather than growing on every subsequent save.
+    assert _cycle(once, times=3).description == "Pr\\u00fcfung, x"
+
+
+def test_non_json_escape_falls_back_to_literal_reading():
+    """A hand-written frontmatter value can hold escapes JSON rejects (a bare
+    Windows path is the common one). Those must keep their previous literal
+    reading rather than raising."""
+    assert _parse_scalar('"C:\\Users\\demo"') == "C:\\Users\\demo"
+
+
+# ---------------------------------------------------------------------------
+# Unchanged behaviour
+# ---------------------------------------------------------------------------
+
+
+def test_plain_scalars_are_still_emitted_bare():
+    """Only values needing quotes get them — the common case must not suddenly
+    start quoting, which would churn every SKILL.md on disk."""
+    assert _emit_scalar("open-pr-from-branch") == "open-pr-from-branch"
+    assert _emit_scalar("1.0.0") == "1.0.0"
+    assert _emit_scalar(True) == "true"
+    assert _emit_scalar(None) == "null"
+    assert _emit_scalar(0.8) == "0.8"
+
+
+def test_single_quoted_scalar_keeps_literal_reading():
+    """Only double-quoted scalars are JSON. Single-quoted ones are read the way
+    they always were."""
+    assert _parse_scalar("'plain, text'") == "plain, text"
+
+
+def test_lists_round_trip_with_non_ascii_entries():
+    skill = Skill(name="demo", tags=["b\u00fcro", "recht, steuern"])
+    assert _cycle(skill, times=3).tags == ["b\u00fcro", "recht, steuern"]
+
+
+# ---------------------------------------------------------------------------
+# Line-break characters
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    "sep",
+    ["\u2028", "\u2029", "\x85", "\r", "\v", "\f", "\x1c"],
+    ids=["ls", "ps", "nel", "cr", "vt", "ff", "fs"],
+)
+def test_line_break_characters_do_not_split_the_frontmatter(sep):
+    """parse_frontmatter() reads one scalar per line via str.splitlines(),
+    which breaks on far more than \\n. Any of these landing unescaped in the
+    file would silently truncate the value and shift the remainder into a
+    bogus key.
+
+    json.dumps covers the C0 ones, but with ensure_ascii=False it passes NEL,
+    LINE SEPARATOR and PARAGRAPH SEPARATOR through as themselves — so those
+    three are re-escaped explicitly.
+    """
+    description = f"before{sep}after, x"
+    markdown = Skill(name="demo", description=description).to_markdown()
+
+    frontmatter_text = markdown.split("---")[1]
+    assert len(frontmatter_text.strip().splitlines()) == len(
+        [l for l in frontmatter_text.strip().split("\n") if l.strip()]
+    ), "a scalar leaked a line break into the frontmatter"
+
+    fm, _body = parse_frontmatter(markdown)
+    assert fm["description"] == description
+
+
+# ---------------------------------------------------------------------------
+# End to end, through real files
+# ---------------------------------------------------------------------------
+
+
+def test_description_survives_real_save_load_cycles_on_disk(tmp_path):
+    """The unit tests above go straight through to_markdown/from_markdown.
+    This drives the same path the app does — SkillsManager writing UTF-8 files
+    with atomic_write_text and reading them back — because the encoding used at
+    either end is part of the fix.
+    """
+    manager = SkillsManager(str(tmp_path))
+    manager.add_skill(name="agb-pruefung", description=GERMAN, category="general")
+
+    for _ in range(4):
+        assert manager.update_skill("agb-pruefung", {"status": "published"})
+
+    stored = [s for s in manager.load_all() if s["name"] == "agb-pruefung"]
+    assert len(stored) == 1
+    assert stored[0]["description"] == GERMAN
+
+    on_disk = (tmp_path / "skills" / "general" / "agb-pruefung" / "SKILL.md").read_text(
+        encoding="utf-8"
+    )
+    assert GERMAN in on_disk