fix(memory): rebuild curated memory from all ledgers when MEMORY.md is blank - #3091
fix(memory): rebuild curated memory from all ledgers when MEMORY.md is blank#3091wzq-xzwj wants to merge 1 commit into
Conversation
…s blank A blank curated MEMORY.md with an advanced consolidation watermark means the view was lost or cleared without resetting the watermark: merging only post-watermark ledgers into "(empty)" silently drops every older day from the curated file, and nothing rebuilds it afterwards (the daily ledgers still hold the facts). readMemoryMd also returns "" on transient read errors, so this is not limited to manual deletion. Treat a blank view with an existing watermark as a lost view: warn and assemble the consolidation input like a first run (all ledgers, no "(since ...)" label). Consistent states keep their exact previous behavior. Covered by four new cases in MemoryConsolidatorFilesystemTest.
| seedStoreFile( | ||
| store, | ||
| ns, | ||
| "memory/2025-06-14.md", |
There was a problem hiding this comment.
Non-blocking: could we also cover an archived ledger here (for example, memory/archive/2025-06-14.md)? Memory maintenance normally moves old daily ledgers under memory/archive/ after the retention period, and the recovery guarantee depends on the filesystem glob remaining recursive. A regression test would make the “all available ledgers” contract explicit.
CryoThrust
left a comment
CryoThrust
left a comment
There was a problem hiding this comment.
The rebuild condition is conceptually useful, but could we distinguish an actually blank/missing MEMORY.md from a read failure before resetting the watermark?
MemoryConsolidator currently receives a String, while WorkspaceManager.readWithOverride/readFileQuietly returns "" both when the file is empty/missing and when the filesystem or local read fails (the latter is logged and then collapsed to empty). In that failure case, currentMemory.isBlank() && watermark != EPOCH would reset the watermark and rebuild from all ledgers, potentially overwriting the curated view based on a transient I/O/overlay failure.
Could the read path expose a small status (missing/blank/read-error), or otherwise make the consolidator skip/fail on read errors while retaining the new full-rebuild behavior for a genuinely blank file? The added tests cover blank-file recovery well; a read-error regression test would make this safety boundary explicit.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Real bug class: clearing or losing MEMORY.md while the consolidation watermark survived would silently drop every pre-watermark day from the curated view. Rebuilding from the full ledger history is the right recovery, the javadoc explains the why, and the tests are thorough. COMMENT only because the rebuild is unbounded and repeatable.
Findings
- [Warning]
MemoryConsolidator.java:171— full-history rebuild has no size/count cap on the prompt input. - [Info]
MemoryConsolidator.java:176— persisted watermark unchanged, so a failing/blank model response re-triggers the expensive rebuild every run. - [Info] test — whitespace-only and never-created MEMORY.md cases not asserted.
Suggestions
Bound the rebuild (newest N ledgers or a char budget with a truncation marker) and record an attempt marker so the expensive path runs once per detected loss rather than once per heartbeat.
Automated review by github-manager-bot
| // cleared without resetting the watermark: merging only post-watermark ledgers into | ||
| // "(empty)" would silently drop every older day from the curated file. Rebuild from | ||
| // the full ledger history instead — the same input assembly as a first run. | ||
| if (currentMemory.isBlank() && !watermark.equals(Instant.EPOCH)) { |
There was a problem hiding this comment.
The rebuild input is unbounded. readDailyEntries(rc, Instant.EPOCH) concatenates every memory/*.md ledger with no count or byte cap, so a long-lived workspace whose MEMORY.md was cleared now sends the whole ledger history in one call. The maxMemoryTokens * 4 budget only bounds the MEMORY.md output side, not this user prompt. On a store-backed session with hundreds of daily files this can exceed the context window or dominate cost. Consider taking the newest N ledgers / a max-char budget (logging what was truncated), or consolidating in rolling chunks.
| "MEMORY.md is blank while a consolidation watermark exists ({}) — rebuilding" | ||
| + " from all daily ledgers", | ||
| watermark); | ||
| watermark = Instant.EPOCH; |
There was a problem hiding this comment.
Resetting the local watermark is right for this run, but the persisted watermark is untouched, so a blank MEMORY.md re-triggers the full rebuild on every heartbeat until the model returns non-blank output (the consolidated.isBlank() path below also returns without advancing it). A cheap lastRebuildAttempt marker, or rebuilding at most once per watermark value, would stop the repeat cost.
| Files.exists(localState), | ||
| "state file should be written to local disk when no filesystem is configured"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Good coverage of rebuild / no-fresh-ledgers / incremental / skip. Two gaps worth pinning: a whitespace-only MEMORY.md (same branch via isBlank()), and the never-created MEMORY.md case — WorkspaceManager.readWithOverride returns "" for both missing and empty, so a stray watermark with no file also rebuilds. Presumably intended; asserting it documents the behaviour.
4 participants
Closes #3090
问题
MemoryConsolidator每次合并的输入组装是"当前 MEMORY.md（或(empty)）+ 水位之后严格修改的账本"，用模型输出覆写 MEMORY.md，然后推进水位。它从不校验 MEMORY.md 是否承载着此前合并保留下来的长期记忆。当 MEMORY.md 缺失或为空而水位文件还在时——手动清理、误删等——下一次合并会把"仅剩的水位后账本"合并进空视图并覆写文件：水位之前未再次修改的旧账本不参与重建，整个过程没有针对该状态的告警。
修复
在
consolidate()中：策划视图空白而水位已存在时，记一条警告并把本次运行的水位按EPOCH处理——与首次运行相同的输入组装。(since ...)提示词标签与账本过滤共用同一个变量，自动消失。Javadoc 已补充说明重建行为。各状态下的行为对照：
(empty)——旧账本不参与修复后的语义：当 MEMORY.md 缺失或为空且已有合并水位时，本次合并重新纳入全部可用账本。这会重新生成记忆摘要，不保证还原原文件；人工补充过、或已无对应账本的内容可能无法恢复。
限制
测试
MemoryConsolidatorFilesystemTest9 项全部通过，其中新增 4 项：consolidate_rebuildsFromAllLedgersWhenMemoryMdBlank——水位前、水位后的账本都进入模型输入，无(since ...)标签，MEMORY.md 写入模型输出，水位正常推进consolidate_rebuildsEvenWithoutFreshLedgersWhenMemoryMdBlank——水位后没有新提取也能重建丢失的视图（修复前会跳过，旧账本永远不参与）consolidate_keepsIncrementalMergeWhenMemoryMdPresent——增量合并行为不变consolidate_skipsWhenViewPresentAndNoFreshLedgers——跳过行为不变agentscope-harness模块全量 2335 项：除 2 项既有错误外全部通过（DangerousPathBypassTest的 2 个 symlink 用例需要 Windows 管理员特权，属环境限制，与本改动无关）。开放问题
全量重建会把整个账本历史在一次调用中发给 LLM，token 开销高于增量合并；替代方案是检测到该状态时跳过并告警。选用重建的理由：每日账本是事实源头、MEMORY.md 是派生视图，目前对丢失的视图没有任何恢复机制；代价是重建调用更重，且输出是重新生成的摘要而非原文件还原。如果维护者倾向更保守的行为，可以随时调整。