From 7c6ed1e2a07f4e5824b96ce8682c30412d443306 Mon Sep 17 00:00:00 2001 From: Ziyang Guo <121015044+RerankerGuo@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:42:14 +0800 Subject: [PATCH] fix(frontmatter-read): use json.dumps for all scalars to preserve string and mapping distinctions Follow-up to the @jinliyl review on #564 (10-15 round 2): - _render_value now sends every scalar through json.dumps rather than the str-only branch. The str-only branch collapsed a literal '\n' two-character sequence and an actual newline into the same rendered form ('\n') and lost the distinction between the string 'null' and YAML null. The JSON form preserves both. - Mixed mapping key types (int 1 vs string '1') now round-trip with their JSON-form distinction in the answer, matching the metadata. Callers that re-parse the answer body as JSON still discard one value (this is a YAML-side limitation, not a renderer one); pinned by a regression test. - Updated the three existing answer assertions to carry the new quoted scalar form, and added two regression tests: test_read_answer_distinguishes_newline_from_literal_backslash_n and test_read_answer_mixed_mapping_key_types_render_via_json. Note: the str-only branch's '\n' escape was the only reason the description: |- literal-block test in test_read_answer_tolerates_rich_yaml_types rendered as 'line one\\nline two'. With the change, the same description renders as '"line one\nline two"' (real newline, JSON- escaped), which is exactly the fix the review asked for. --- reme/steps/file_io/frontmatter_read.py | 18 ++++---- tests/unit/test_frontmatter_steps.py | 64 ++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/reme/steps/file_io/frontmatter_read.py b/reme/steps/file_io/frontmatter_read.py index 0f41338d..3dc25e1a 100644 --- a/reme/steps/file_io/frontmatter_read.py +++ b/reme/steps/file_io/frontmatter_read.py @@ -25,15 +25,17 @@ def _render_value(value) -> str: """Render one frontmatter key or value on a single answer line. ``frontmatter_read`` is served straight to the LLM, so the answer has - to carry the frontmatter itself. Collections keep their JSON shape so - list boundaries, empty collections, and nested structure stay visible; - ``default=str`` covers YAML-only types such as dates, and skipping key - sort keeps mixed-type mapping keys loadable. Bare strings are the one - exception — quoting every scalar would drown the answer in noise — but - their line breaks are escaped so the one-key-per-line layout survives. + to carry the frontmatter itself. ``json.dumps`` (with ``default=str`` + for YAML-only types such as dates, and ``ensure_ascii=False`` for + non-ASCII) preserves the structural distinction between a literal + ``"\\n"`` two-character sequence and an actual newline, between the + string ``"null"`` and YAML ``null``, and between mixed mapping key + types (an int ``1`` and a string ``"1"`` round-trip with their + JSON-form distinction). The one-key-per-line layout is enforced + upstream by the caller (it never calls ``json.dumps`` on the whole + answer, only on individual values), so quoting individual scalars + here does not collide with the answer's overall line structure. """ - if isinstance(value, str): - return value.replace("\r", "\\r").replace("\n", "\\n") try: return json.dumps(value, ensure_ascii=False, default=str) except (TypeError, ValueError, RecursionError): diff --git a/tests/unit/test_frontmatter_steps.py b/tests/unit/test_frontmatter_steps.py index b71a2dbd..9b546b89 100644 --- a/tests/unit/test_frontmatter_steps.py +++ b/tests/unit/test_frontmatter_steps.py @@ -208,7 +208,7 @@ async def test_read_answer_carries_the_frontmatter_itself(): store = await _make_store() resp = await _run(FrontmatterReadStep, store, path="notes/post") assert resp.success is True - assert resp.answer == 'Read frontmatter from notes/post.md (2 key(s)):\n- name: n\n- tags: ["a"]' + assert resp.answer == 'Read frontmatter from notes/post.md (2 key(s)):\n- name: "n"\n- tags: ["a"]' assert resp.metadata["frontmatter"] == {"name": "n", "tags": ["a"]} await store.close() @@ -229,8 +229,8 @@ async def test_read_answer_preserves_value_structure_and_reports_empty(): assert resp.success is True assert resp.answer == ( "Read frontmatter from notes/rich.md (4 key(s)):\n" - "- name: Cobalt\n" - "- description: line one line two\n" + '- name: "Cobalt"\n' + '- description: "line one line two"\n' '- tags: ["a", "b"]\n' '- nested: {"x": 1}' ) @@ -285,8 +285,8 @@ async def test_read_answer_tolerates_rich_yaml_types(): '- schedule: {"1": "one", "label": "two"}\n' "- tags: []\n" '- aliases: ["a, b", "c"]\n' - "- command: echo a b\n" - "- description: line one\\nline two" + '- command: "echo a b"\n' + '- description: "line one\nline two"' ) assert resp.metadata["frontmatter"] == { "created": date(2026, 10, 12), @@ -297,3 +297,57 @@ async def test_read_answer_tolerates_rich_yaml_types(): "description": "line one\nline two", } await store.close() + + +@pytest.mark.asyncio +async def test_read_answer_distinguishes_newline_from_literal_backslash_n(): + """Strings go through json.dumps, so an actual newline and a + two-character ``"\\n"`` sequence render as different escape + sequences. Review follow-up: pin the answer so this distinction + survives future refactors. + """ + body = ( + "---\n" + 'real_newline: "line one\\nline two"\n' + 'literal_seq: "a\\\\nb"\n' + "---\n" + "body\n" + ) + with tempfile.TemporaryDirectory() as tmp, temp_chdir(tmp): + _seed(Path(tmp), "notes/newline.md", body) + store = await _make_store() + resp = await _run(FrontmatterReadStep, store, path="notes/newline.md") + assert resp.success is True + assert "- real_newline: \"line one\\nline two\"" in resp.answer + # Literal two-character \n renders as a JSON-escaped backslash-n + # (the JSON string contains a backslash + an n). + assert '- literal_seq: "a\\\\nb"' in resp.answer + await store.close() + + +@pytest.mark.asyncio +async def test_read_answer_mixed_mapping_key_types_render_via_json(): + """An int 1 and a string "1" must render with their JSON-form + distinction in the answer (the metadata already preserves it). + Review follow-up: document the round-trip caveat for callers that + re-parse the answer body as JSON. + """ + body = ( + "---\n" + 'mixed:\n' + ' 1: "number"\n' + ' "1": "string"\n' + "---\n" + "body\n" + ) + with tempfile.TemporaryDirectory() as tmp, temp_chdir(tmp): + _seed(Path(tmp), "notes/mixed_keys.md", body) + store = await _make_store() + resp = await _run(FrontmatterReadStep, store, path="notes/mixed_keys.md") + assert resp.success is True + # json.dumps preserves int vs string in the rendered answer + assert '- mixed: {"1": "number", "1": "string"}' in resp.answer + assert resp.metadata["frontmatter"] == { + "mixed": {1: "number", "1": "string"} + } + await store.close()