Repository navigation
fix(mem_os): strip only anchored markdown fences in clean_json_response (#2420) - #2590
syzsunshine219 wants to merge 2 commits into
Conversation
…se (MemTensor#2420) clean_json_response previously used a global `response.replace("```json", "").replace("```", "")`, which removed every triple-backtick sequence in the response — including ones that lived inside JSON string values. Any LLM response whose payload contained a code snippet (a fenced Python block, an unclosed partial fence, etc.) came out silently corrupted before json.loads, so suggestions and chat metadata carrying markdown snippets were returned to callers with mangled content. Both api/handlers/chat_handler.py and api/handlers/suggestion_handler.py pipe responses through clean_json_response, so the bug surface was the entire suggestion and chat pipeline. The fix replaces the global string replace with two anchored regex substitutions that only strip the outer fences: - opening fence (^\`\`\`[^\\n`]*\\n?) — covers the optional info string (`json` or otherwise) and trailing newline - closing fence (\\n?\`\`\`\\s*$) — at the very end only Interior \`\`\` sequences inside a JSON string value are preserved verbatim. As a side effect the function now correctly handles responses that have only an opening or only a closing fence (the original implementation would leave those stranded mid-string). - Add module-level _JSON_FENCE_OPEN_RE / _JSON_FENCE_CLOSE_RE - Rewrite clean_json_response body to use anchored re.sub - Four new tests in tests/mem_os/test_format_utils.py pin the contract: the issue reproducer (interior fenced code preserved), unclosed-fence-in-value, opening-only response, and closing-only response Fixes MemTensor#2420.
… pattern Move the fence regex constants below the memos imports so ruff's E402 (module-import-not-at-top) passes again; the constants had been inserted between the stdlib and memos import blocks, failing every build-matrix job at the Ruff checks step. Restrict the opening-fence info string from the greedy not-newline-not- backtick class to a blank-padded short identifier. The greedy class matched JSON payload characters up to the next backtick, so a single-line fenced payload — triple-backtick + JSON + triple-backtick with no newline after the fence, or with an info string followed by a space — was swallowed whole and clean_json_response returned an empty string, breaking json.loads downstream. The tightened class only accepts an actual info string and keeps the newline optional so truncated fences are still stripped. Add regression tests for both single-line fenced forms.
🤖 Open Code ReviewTarget: PR #2590 ✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). Generated by cloud-assistant via Open Code Review. |
✅ Automated Test Results: PASSEDRequired tests passed (14/14 executed, 0 skipped). Duration: 17s [advisory, non-gating] AI-generated tests on branch test/auto-gen-e45aa70eef2d46db-20261110001310: 54/54 passed — these do NOT affect the PR verdict; review the branch manually. Branch: Tested source: |
Restores the change proposed in #2489 for review against
mainafter its former base branch,dev-v2.0.36, was deleted. The source commits and authors are preserved.Source head:
41269f335d7c08a13cc1242eff662ff688203434This PR is open for review. Review and CI against the current base are pending; validation claims in the original description below refer to the earlier snapshot. The original discussion and evidence remain in #2489.
Original change description
Summary
clean_json_responseused a globalresponse.replace("```json", "").replace("```", ""), which removed every triple-backtick sequence — including ones inside JSON string values. Any LLM response whose payload contained a code snippet came out silently corrupted beforejson.loads.Root cause
src/memos/mem_os/utils/format_utils.py:1416(pre-fix):Reproducer from the issue:
Both
api/handlers/chat_handler.pyandapi/handlers/suggestion_handler.pypipe responses through this function, so the bug surface was the entire chat-metadata and suggestion pipeline.Fix
Replace the global string replace with two anchored regex substitutions that only strip the outer fences:
re.compile(r"^```[^\n`]*\n?")— covers the optional info string (jsonor otherwise) and trailing newlinere.compile(r"\n?```\s*$")— at the very end onlyInterior
```sequences inside a JSON string value are preserved verbatim. As a side effect the function now correctly handles responses that have only an opening or only a closing fence (the original implementation would leave those stranded mid-string).Tests
Added four cases to
tests/mem_os/test_format_utils.py:test_clean_json_response_preserves_internal_fenced_code— the issue reproducer (red against unpatchedmain)test_clean_json_response_preserves_unclosed_fence_in_value— a value containing a single unclosed fencetest_clean_json_response_handles_only_opening_fence— best-effort handling when the closing fence is missingtest_clean_json_response_handles_only_closing_fence— best-effort handling when the opening fence is missingExisting tests (single backticks in JSON content, plain JSON, complex JSON, etc.) all continue to pass —
test_clean_json_response_preserves_internal_backtickswas a good seed for the new tests.Verification
Ruff:
ruff format --checkandruff checkboth clean on touched files (the 2 pre-existing findings were auto-fixed byruff check --fixduring this PR — both wereE501 line-too-longinclean_json_responseitself, not introduced by this change).Out of scope
Closes #2420