Repository navigation
Python: make the memory markdown escape invertible for backslash runs - #9728
Manohar Paturi (ManoharPaturi) wants to merge 2 commits into
Conversation
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
Manohar Paturi (@ManoharPaturi) One material backward-compatibility regression remains in python/packages/core/agent_framework/_harness/_memory.py:145-148. The new decoder assumes every odd backslash run was produced by the new encoding, but the previous writer left runs of two or more backslashes unchanged. Existing unversioned files with three, five, or seven backslashes before # therefore lose characters when loaded and persist the corruption on rewrite. Please make the new encoding distinguishable, retain legacy decoding for unversioned files, and add compatibility tests that read odd runs written by the previous implementation.
A line starting with backslash hash used to gain one more backslash on write that the reader never stripped, so every reload corrupted the memory a little more. Runs of n originals are now written as 2n+1 backslashes, which decodes exactly, and every written record carries a format marker identifying the invertible encoding. Files without the marker were written by the previous implementation, which left runs of two or more backslashes verbatim, so they are decoded with the previous reader's exact behavior: strip one backslash only from a body starting backslash hash, and leave every longer run untouched. Compatibility tests load odd runs written by the previous implementation from unversioned files and assert they survive unchanged. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
b6f347c to
72698c1
Compare
|
Fixed in 72698c1, with your compatibility case reproduced first. The new encoding is now distinguishable: every written record carries a format marker, and on read the marker selects the decoder. Files without the marker were written by the previous implementation, which left runs of two or more backslashes verbatim, so they are decoded with the previous reader's exact behavior: strip one backslash only from a body starting backslash hash, and leave every longer run untouched. Compatibility tests load unversioned files holding three, five, and seven backslashes before a hash and assert they survive unchanged rather than losing characters, plus an unversioned single-backslash line still decoding exactly as before. Versioned files round trip every run length from zero through five exactly. The unversioned odd-run test fails on the previous head and passes now, the memory suite passes at 38 tests, and ruff check and format are clean. |
|
Eduard van Valkenburg (@eavanvalkenburg) the PR body now carries the required Contribution Checklist with the non-breaking item confirmed, matching the template. Everything from your review is addressed in the latest push and the thread replies have the verification details. |
|
Manohar Paturi (@ManoharPaturi) The requested unversioned backslash cases now work, but format detection introduces a separate content-boundary regression in A record with summary Please recognize and consume a standalone format marker only in the metadata header, before content sections, and preserve the same text inside summaries or memories. Add a summary round-trip regression and a legacy-body marker case. |
The format marker is recognized as a standalone line before any content section, so marker shaped summary or memory text stays content and a legacy body line mentioning the marker no longer switches decoding. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
|
Fixed in c4a3e00, with both shapes reproduced on the previous head first. The marker is now recognized and consumed only as a standalone line in the metadata header, before any content section, and the substring pre-scan is gone. A summary holding exactly the marker text round trips as summary content next to its memory instead of being swallowed and backfilled from For the legacy side, a body line that merely mentions the marker no longer selects v2 decoding for the whole file. The regression stores such a mention next to a three backslash memory and asserts both survive verbatim under legacy decoding, matching the same file without the mention. The escape feature itself is untouched: the run length sweep from zero through five still round trips exactly. Both regressions fail on the previous head and pass now. The memory suite passes at 40 tests and ruff check and format are clean. |
|
Manohar Paturi (@ManoharPaturi), thanks for taking the time to investigate and implement this. We want to review and agree on #9727 before accepting an implementation PR, rather than evaluating the issue and implementation together. I’m closing this PR for now. Please wait for maintainer direction on the issue before opening a new implementation PR. |
Motivation & Context
_escape_markdown_lineprepended one backslash to any line starting with#or\#, but_unescape_markdown_lineonly stripped a backslash from a body starting with exactly\#. A line that already began with an escaped hash therefore gained a backslash on every write and read cycle, and the corruption compounded across the rereads that auto loading and consolidation perform. Reproduced on main with a one line round trip:\# literal factcame back as\\# literal fact.Description & Review Guide
nbackslashes before a hash to2n + 1, and the reader reduces an odd run back to the originaln. A plain#line still gets the single backslash it always had, which is then = 0case.Related Issue
Fixes #9727
AI Assistance
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.