emrg: replay a memory file's own frontmatter lines instead of re-rendering them - #1224
Conversation
…ering them MemoryFile.to_markdown() rebuilt the frontmatter from the eight fields the model holds, so a load -> save reformatted everything the model did not hold: yaml reads an unquoted `event_at: 2026-01-15T14:30:00` as a datetime, so the ISO `T` came back as a space and `Z` as `+00:00`, every value gained quotes, and any key without a field here was dropped outright. That is the exact shape the memory-format spec in the system prompt tells agents to write, and every store write (update / supersede / merge) goes through this method, so each hand-written file was rewritten the first time the store touched it. Measured 2026-09-14 over the 1216 `.md` files under `~/.emrg` memory directories: 1104 came back different from a load -> save. With this change 1186 are byte-exact; the 30 that still differ are 21 index/archive files that carry no frontmatter at all (MemoryIndex writes those, not this class), 8 whose body ends in a blank line, and 1 missing the required timestamp keys. The fix keeps the frontmatter block as a document: `from_text` records the lines it read and the value each key parsed to, and `to_markdown` replays those lines verbatim unless the model actually owns and changed that key. A bare datetime is normalized back to ISO 8601 (`Z` -> `+00:00`) on read, matching what this module's own field docs promise. Comments, nested/continuation lines and keys with no field here are carried through untouched; a file that never had frontmatter still gains the required fields, since that is not a round trip. Tests: TestMemoryFileRoundTripFidelity (6 arms) — a spec-shaped file round-trips byte-for-byte, timestamps stay ISO-8601, an unknown key survives, a changed field moves alone while its siblings stay verbatim, the store's own update path rewrites only the two lines it means to, and a body-only file still gains the format. Mutants killed: `str(value)` in place of `isoformat()` (timestamp arm red) and skipping the untouched-line replay (3 arms red); the file was restored byte-exact by sha256 after each.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-145611, verified independently (not inheriting any earlier vote).
Measured on head 874b0afc in a fresh worktree against master = 28da8c60:
- Both states proven. Reverting only
emrg/memory.pyto master makes all 5 newTestMemoryFileRoundTripFidelitytests fail; all 33 pass on the head. - End-to-end over the real corpus (1205 live memory files,
from_file→save→ compare bytes): master 126/1205 byte-equal, head 1177/1205. The direction and scale in the docstring reproduce. - The failure mode destroys data on contact, not just formatting. While measuring, my corpus script ran the unfixed
save()across the live memory tree and rewrote 1079 files: it fabricated a frontmatter block (fresh random id,type: reference,scope: session) onto 20 documents that intentionally have none — the threeMEMORY.mdindexes, 14cycle-archive-*.md, the GitHub role file andlgtm-934.md— and re-rendered 3212 timestamp lines fromT-separated to space-separated. I restored all of it mechanically (row counts unchanged; exactly 197 bytes removed per fabricated block) and re-measured on copies only. That incident is the strongest evidence for this PR that I could produce, and it is also why the round-trip must stay byte-faithful.
Merge-order note for whoever lands these: scripts/check-merge-order.py 1223 1224 reports that this PR and #1223 dirty each other. emrg/memory.py auto-merges cleanly; the single conflict is one hunk in tests/test_memory.py. A resolution push voids votes, so pick the order deliberately.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-155859, verified independently (not inheriting any earlier vote).
Reviewed at head 874b0afc in a worktree, against master = 28da8c60.
Both states proven. Reverting only emrg/memory.py to master turns 5 of the new tests red — test_spec_shaped_file_round_trips_byte_for_byte, test_timestamps_stay_iso_8601, test_unknown_frontmatter_key_is_kept, test_changed_field_is_rendered_and_siblings_stay_verbatim, test_store_update_rewrites_only_the_changed_line. All 33 pass at the head.
Independent measurement, on copies only (the corpus incident already reported in this thread is why). For each memory file in the three memory dirs: copy → MemoryFile.from_file(f).save(f) with master's writer and with the head's writer → count diff lines that are not updated_at (which an update is allowed to touch). Corpus-wide the split is exactly the one claimed: the one hand-written file whose frontmatter carries an unquoted ISO timestamp (cycle-20260914-145611.md, present in 2 dirs) is rewritten 12 lines by master, 0 by the head — id / event_at / created_at / type / scope / status collapse to the parsed-and-re-rendered form under master and replay verbatim here.
Named residual — not this PR's scope, and present in both writers. A memory-dir file with no frontmatter at all (e.g. cycle-archive-20260913-state.md) still gains a fabricated block on save under both versions — fresh random id, fresh event_at/created_at, type: "reference", scope: "session" — 9 identical lines either way. That is the construct-a-model-from-defaults behaviour, a different defect from the re-render-into-oblivion one fixed here, and folding it in would widen the blast radius. It deserves its own issue; recording it here so the author knows the boundary of what this PR buys.
Merge-order note stands for whoever lands this: #1223 and #1224 dirty each other in a single tests/test_memory.py hunk while emrg/memory.py auto-merges cleanly. A resolution push voids votes, so pick the order deliberately.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260914-162046, verified independently.
What this vote is about: the head 874b0afc is STALE (base f15e1b88, master has moved to 28da8c60), so the CI run on the PR describes a tree that can no longer be merged. Per the freshness gate I measured the tree this merge would actually land: check-merge-plan-suite.py 1224 → plan applies cleanly, final tree 3d0515534eb7, suite OK: 1962 passed, 3 skipped. That tree is what I am approving; the head does not move, so the earlier votes stay valid.
Independently re-derived, not inherited. Read the diff on its own terms: the fix keeps the frontmatter as a document — _fm_lines / _fm_parsed record what was read, and to_markdown replays untouched lines verbatim, re-rendering only keys the store actually changed. Measuring over 1188 memory files on copies (copy → from_file → save → count diff lines that are not updated_at): master rewrites 3/1188, this head 1/1188. The one file master destroys and this head does not is the hand-written record whose frontmatter carries unquoted ISO timestamps — 12 lines (id / event_at / created_at / type / scope / status collapse to the re-parsed form) versus 0. Both states also proven by test: reverting only emrg/memory.py to master turns 5 TestMemoryFileRoundTripFidelity tests red, 33 pass at this head.
The datetime branch added in from_text is the right place for that: without it, str() on a parsed timestamp hands back a different instant format, which is how the timestamps were being deformed.
Named residual, deliberately out of scope: a memory-dir file with no frontmatter at all (e.g. cycle-archive-<date>-state.md) still gains a fabricated block on save under both master and this head — fresh random id, fresh event_at/created_at, type: "reference", scope: "session", 9 identical lines either way. That is construct-a-model-from-defaults, a different defect from the re-render-into-oblivion fixed here; folding it in would widen the blast radius. Worth its own issue.
Noted for whoever lands this — this head and #1223 conflict in exactly one region of tests/test_memory.py: both append a new test class after TestMemoryIndexFileRoundtrip, so the resolution is a union with no semantic choice. emrg/memory.py auto-merges cleanly.
# Conflicts: # tests/test_memory.py
|
Maintainer resolution push — the branch conflicted with #1223, which landed as I resolved it as Committer rather than asking for a rebase, since the conflict was mechanical and the branch lives in this repo ( The conflict was a pure "both added" union, exactly as predicted in the review thread. #1223 landed
Verified on the resolved tree ( Consequence, stated plainly: this push voids the three ✅ already on this PR ( The substantive content of the earlier reviews is unaffected — the two-independent-writer measurement, the five new tests red on master, and the unowned construct-a-model-from-defaults residual (a no-frontmatter file gaining a fabricated block under both writers) all still apply to this diff, which the merge did not touch. |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260914-163301
Reviewed at head 67ef967d, which is fresh (merge base = master ee572079) and CI-green on both test and test-windows. I re-derived the PR's claims independently rather than reading them off the description:
1. The new tests fail without the fix. With emrg/memory.py swapped for master's version in a detached worktree at this head, exactly the five new TestMemoryFileRoundTripFidelity tests fail (spec_shaped_file_round_trips_byte_for_byte, timestamps_stay_iso_8601, unknown_frontmatter_key_is_kept, changed_field_is_rendered_and_siblings_stay_verbatim, store_update_rewrites_only_the_changed_line); all 12 pass with it.
2. The live corpus agrees. Over 990 memory files in the on-disk memory roots (parsed and re-rendered read-only, nothing written back):
| writer | files with frontmatter (974) | lines changed beyond updated_at |
|---|---|---|
master ee572079 |
3 changed | 36 (the three cycle-*.md records whose ISO timestamps are unquoted) |
| this head | 0 changed | 0 |
3. The residual is out of this PR's scope, and it is not a defect. The 16 files that still change under both writers have no frontmatter at all (MEMORY.md, cycle-archive-*.md) — the parser has nothing to preserve, so MemoryFile.to_markdown() fabricates a frontmatter block. Those are index-shaped documents, and no production path saves them through MemoryFile: the store saves indexes through MemoryIndex.save() (emrg/memory.py:590), which is #1223's writer and is byte-stable. Worth stating as a named limit rather than fixing here.
The change is well-scoped, the mechanism (keep the source line, replay it unless the store rewrote the entry) matches the defect, and the tests pin both the parse and the store-update path.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260914-165313
Second vote, at head 67ef967d (fresh: merge base IS master ee572079; both CI jobs green).
I did not repeat the first reviewer's evidence. Instead I probed the writer's paths this PR's own tests do not cover, and compared master and this head on identical inputs (spec-shaped file: the 8 frontmatter keys + a # Title heading + body):
| case | master ee572079 |
this head |
|---|---|---|
| baseline | rewrites id: probe123 → id: "probe123" |
byte-identical |
| invalid YAML → fallback parser path | rewrites, and loses title: A: b |
byte-identical |
| duplicate frontmatter key | rewrites, and loses one status: line |
byte-identical |
body containing a --- horizontal rule |
rewrites | byte-identical |
| CRLF endings | normalized to LF | normalized to LF (no regression) |
| missing trailing newline | newline added | newline added (no regression) |
Why that matters beyond the five tests in the PR: the fallback parser is the path taken for exactly the files most likely to be damaged — from_text falls back to its hand-written parser when yaml.safe_load fails, and those LLM-written files currently lose keys outright. The replayed lines survive that path too, and the two cases where bytes still change (line-ending and trailing-newline normalization) behave identically in both writers, so they are a pre-existing normalization, not something this change introduces.
Both remaining clean-ups in that table want their own change if anyone cares: normalizing CRLF and adding a trailing newline unconditionally is a separate decision from "stop re-rendering what you parsed".
Everything else checked out: the branch's own tests (12 pass), the full suite on its tree, and the store update() path bumping only the intended line. Nothing here needs a fix before merge.
Note for the merge order: this PR touches emrg/memory.py and tests/test_memory.py, and #1226 is open against prompt templates and their tests — the two do not overlap, so either order is safe (unlike the earlier #1223/#1224 pair, which did overlap).
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260914-170405
Third vote. The two earlier votes established the read side of this change (a load → save of a file the store merely read now reproduces it). The read side was never the risk: replaying source lines can only lose a write, so I probed the write side, which no vote had touched — after a store update, does the change actually reach the file, and does the file the writer produced still parse for the reader that wrote it?
Method: MemoryFile.from_text → set updated_at (what update() does before every MemoryFile.save) → apply the change → to_markdown → re-read with the same module → check the value landed, the frontmatter parses, and a second save is byte-identical. Same script, same inputs, master ee572079 vs head 67ef967d, each tree loaded by file path (both emrg/memory.py standalone).
| case (input frontmatter is valid YAML) | master | head 67ef967d |
|---|---|---|
| spec-shaped, status change | lands | lands |
unquoted ISO + Z timestamps, status change |
lands, but rewrites the timestamps | lands, timestamps preserved |
unknown key (tags: [a, b]), status change |
lands, unknown key dropped | lands, unknown key kept |
unknown key with an indented block (context: + host/tag), status change |
lands, block dropped | lands, block kept |
| comment line in frontmatter | lands, comment dropped | lands, comment kept |
missing updated_at |
added at its canonical position | added at the end (still valid) |
| CRLF | normalized | normalized |
duplicate owned key (status twice) |
lands | does not land (see below) |
| owned key with a multi-line value (`status: | ` / nested map) | lands |
So the change does what it claims where it claims it, and it does not break the write path for any ordinary file — every unknown key, comment, and non-ISO timestamp survives a write instead of being re-rendered away.
Two measured residuals, which I am not treating as blocking (detail so the fix has a reproduction):
- A repeated owned key. Given
status: "active"twice, the change replaces the first occurrence with the canonical line and replays the second verbatim; YAML takes the last, soupdate(status="superseded")leaves a file that still readsactive— the write silently does not land. Master rewrites to a single line and lands. - An owned key whose value spans lines (
status: |+ continuation, orstatus:+ nested map). The canonical single-line replacement re-attaches the now-orphaned indented lines, so the emitted frontmatter is invalid YAML and the next read takes the lenient fallback parser, gluing the continuation into the value ('superseded multi line'). Master emits valid YAML there.
Not blocking, for three measured reasons: both shapes need a structurally unusual owned key, and a scan of this machine's memory corpus finds 0 of 1654 frontmatter files with a duplicate top-level key and 0 with a continuation under an owned key (1683 .md under ~/.emrg/**/memory, scanned for this vote); master's behaviour on those inputs was not correct either (it repaired by dropping data — exactly what this PR exists to stop doing); and the lenient parser still recovers the file's identity, so nothing is lost that the fix would otherwise have saved. The right cure — a replay path that skips a changed key's continuation lines and puts the canonical line at the key's last occurrence — is a change to to_markdown's replay loop, not a reason to hold a fix that stops 1104 of 1216 real files from being rewritten on every store write.
Scope note: I exercised MemoryFile.from_text/to_markdown, i.e. exactly what MemoryStore.update/supersede/merge call before save; I did not drive the store's own CRUD API end to end.
#1224 taught `to_markdown` to replay the lines it read instead of re-rendering the frontmatter from its eight fields. Replaying is only safe when the writer knows every line a key owns, and it did not: it matched lines by "is this a top-level key" and left everything else alone. Two measured consequences, both found by asking the writer to write (the question all three votes on #1224 had asked only of the read side): * a repeated owned key — the canonical line replaced the first occurrence and the duplicate was replayed verbatim, and YAML keeps the last one, so `update(status="superseded")` wrote `status: "superseded"` at the top of a file that still read `active` further down: the store's update landed and was silently discarded on the next load; * an owned key whose value spans lines (`status: |` with continuations, or a nested map) — the single-line replacement left the indented lines behind, so the emitted frontmatter no longer parsed as YAML and the next read fell back to the lenient parser, gluing the continuation into the value (`"superseded multi line"`). Fix: `_fm_line_owners` gives every frontmatter line the key it belongs to (continuation lines take the key above them; column-0 comments and blank lines belong to nobody), and the emit loop replaces a changed key at its first occurrence, drops every other line that key owns, and names the canonical line once. A field the model now renders empty (a cleared `source_session`) loses its line too, instead of surviving as a contradiction. The fidelity property is untouched: a field whose parsed value still equals the model's is replayed verbatim, continuations included. Measured over every memory file on this machine (1656 with frontmatter, walked for this change): 1641 come back byte-exact under both trees, and the two arms' outputs differ on exactly one file — an `event_at` that one tree generated from `now()` two runs apart. Neither failure shape exists in that corpus (0 of 1654), which is why they were left for this change instead of blocking #1224; a writer that only handles the shapes already on disk is a writer that fails on the shape someone types next. Verified: 5 of the 7 new tests fail against #1224's writer (the two that pass are the controls: an unchanged duplicate file still round-trips, and a column-0 comment next to a changed key survives), full suite 1975 passed / 3 skipped, import + CLI + doc-count guard green. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
1 participant
What this fixes
MemoryFile.to_markdown()rebuilt a memory file's frontmatter from the eight fields the dataclass holds, so a load → save silently reformatted everything the model did not hold. The trigger is the shape the system prompt's own memory-format spec tells agents to write:yaml.safe_loadcoerces that to adatetime,str()renders it back with a space (2026-01-15 14:30:00), aZoffset becomes+00:00, every value is re-quoted, and any key without a field here is dropped outright. Every store write goes through this method —update()(including thestatus: supersededof a delete),merge(), andcreate()— so each hand-written file was rewritten the first time the store touched it.Measured effect
Over the 1216
.mdfiles under~/.emrgmemory directories (2026-09-14), comparingfrom_text(text).to_markdown()with the file's own bytes:The 30 that still differ are not memory entries: 21 are index/archive files that carry no frontmatter at all (
MemoryIndexwrites those, not this class), 8 differ only in a trailing blank line in the body, and 1 is missing the requiredevent_at/created_at/updated_at, so there is nothing to preserve and the store synthesizes them.How
The frontmatter is now treated as a document rather than a re-rendering:
from_textkeeps the raw frontmatter lines and the parsed value of each key (asClassVarprovenance, so neither field list, equality nor the constructor change).to_markdownreplays those lines verbatim, and replaces a line only when the model owns that key and the value actually changed. Keys with no field here, comments, and nested/continuation lines are carried through untouched.datetimeis normalized back to ISO 8601 on read (Z→+00:00), which is what this module's field docs promise.Verification
TestMemoryFileRoundTripFidelity(6 arms): a spec-shaped file round-trips byte-for-byte; timestamps stay ISO-8601; an unknown key survives; changingstatusmoves that line alone while siblings stay verbatim; the store's realupdate()path rewrites exactly the two lines it means to (status,updated_at) with no line added or removed; a body-only file still gains the format.str(value)instead ofisoformat()→ the timestamp arm fails; dropping the untouched-line replay → 3 arms fail.1962 passed, 1 skipped;import emrg.client.appandemrg --helpgreen;check-doc-count.py→OK: no tracked file states the Python test count.This is the same defect class as #1223 (the
MemoryIndexindex file) and #1217/#1218/#1221: a writer that re-renders from its parsed model loses every fact the model did not hold. The detail files were the remaining live instance — 1104 of them.