fix(core): record auto-memory index reads in FileReadCache - #7468
Conversation
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
ZijianZhang989
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
gwinthis
left a comment
There was a problem hiding this comment.
Architecture Review — PR #7468
Verdict: ✅ Approve — small, focused, correct fix for a real UX friction bug.
What it does
Registers auto-memory MEMORY.md reads (project-level + user-level) in the session FileReadCache during refreshHierarchicalMemory(), so the model's first write_file to update the index isn't rejected by the prior-read guard.
Code quality
| Aspect | Assessment |
|---|---|
| Scope | Minimal — 1 new private method, 1 call site, 1 regression test |
| Error handling | stat failure → warn log, no crash. Prior-read guard stays in effect as fallback. Correct. |
| Parallelism | Promise.all for both indexes — non-blocking, independent |
| Test isolation | Temp dirs + env restore in finally + clearAutoMemoryRootCache() — clean |
| Guard ordering | indexContent === null check before stat — avoids unnecessary I/O |
Design insight: Implicit Read Registration
This PR surfaces a broader architectural pattern worth documenting:
The system reads files on the model's behalf (system prompt assembly, memory loading, QWEN.md injection) but the prior-read guard only tracks model-initiated reads (via read_file tool). This creates a "phantom read" gap: the model has seen the content in its context, but the guard doesn't know it.
The fix pattern — register implicit reads in the same cache that guards explicit writes — is the right approach. It maintains the invariant: "if the model can see a file's content in its context, the guard should know about it."
Minor observations (non-blocking)
-
Scope boundary is correct: PR explicitly excludes
QWEN.md,AGENTS.md, and team-memory indexes. These are loaded vialoadServerHierarchicalMemorywhich has a different code path. Extending to those would be a separate PR. -
full: trueis correct here: The auto-memory index is read in its entirety (readAutoMemoryIndexreturns the full file content), sofull: trueaccurately reflects what the model sees. -
Race window: Between
readAutoMemoryIndex()(content read) andfsPromises.stat()(cache seeding), the file could change. The PR handles this correctly — ifstatfails, the cache entry is simply not seeded, and the model gets the standard "read first" error on write. No data corruption possible.
E2E verification and review noteI ran a deterministic fake-model CLI E2E against the PR base ( Coverage:
Result:
Screenshots: Review note: I also did a full diff review. The fix is small and matches the issue, but there is one residual prior-read race to consider: |
|
Thanks for the careful E2E check and the race note. I updated the PR so auto-memory index reads now return the stats captured before the content read, and FileReadCache records that same fingerprint instead of performing a later post-read stat(). I also added regression coverage showing a file changed after the captured read stats is treated as stale rather than authorized. |
Maintainer local verification — merge referenceVerified locally on macOS (Darwin 24.6.0) against the current head 1. Real-flow A/B (unmocked)A standalone harness drives the actual
This reproduces #7287 on base (the model must burn an extra 2. Regression + full suites
3. On the race concern from the earlier reviewThe follow-up commit "seed memory read cache from read stats" resolves it correctly. VerdictLGTM — merge-ready. Behavior matches the linked issue, the earlier race note is addressed and independently verified on the current head, and macOS is now covered alongside the author's Windows testing. 中文版本维护者本地验证 — 合并参考在 macOS(Darwin 24.6.0)上针对当前 head 1. 真实流程 A/B(未打桩)一个独立脚本驱动真实的
base 上复现了 #7287(模型必须先额外调用一次 2. 回归测试 + 完整套件
3. 关于上一轮评审提出的竞态问题后续 commit “seed memory read cache from read stats” 正确地解决了它。 结论LGTM,可以合并。 行为符合关联 issue,上一轮的竞态提醒已被解决并在当前 head 上独立验证,macOS 也已随作者的 Windows 测试一并覆盖。 |
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. Clean fix — auto-memory MEMORY.md indexes now register in FileReadCache on load, eliminating the spurious write rejection on first save. The stat+read TOCTOU is correctly handled (test verifies FILE_CHANGED_SINCE_READ when file changes between stat and read). Backward compatible — old readAutoMemoryIndex still exists. New readAutoMemoryIndexWithStats returns both content and fs.Stats. Tests cover cache seeding and race detection.
— qwen3.8-max-preview via Qwen Code /review
|
Post-merge correctness follow-up: I found one lifecycle gap that is separate from the stat-before-read race fixed here.
A later write based on A can therefore pass The inverse also occurs after I think the implicit-read snapshot needs to be tied to the lifecycle of the prompt/cache instance that actually receives it: grant it only when the corresponding system instruction is installed, and preserve or reseed it while that instruction remains active. |





What this PR does
Registers project-level and user-level auto-memory
MEMORY.mdreads in the sessionFileReadCachewhen those indexes are loaded into the system prompt.Why it's needed
The model already sees these index files through the auto-memory prompt, but the prior-read guard did not know that. As a result, the first
write_fileupdate toMEMORY.mdin a fresh session was rejected until the model spent an extraread_fileround-trip.Reviewer Test Plan
How to verify
Run
npm test --workspace=@qwen-code/qwen-code-core -- src/config/config.test.ts. The regression test creates project-level and user-levelMEMORY.mdfiles, refreshes hierarchical memory, and verifies thatcheckPriorRead()allows overwriting both index files without an explicitread_filecall.The test also uses a temporary memory base directory and restores the environment after completion, so it does not touch a real user memory directory.
Evidence (Before & After)
N/A — non-user-visible core behavior and regression test change.
Tested on
Environment (optional)
Local npm workspace on Windows.
config.test.tspassed with 419 tests. Changed files also passed ESLint and Prettier checks.Risk & Scope
statfails after the prompt-loading read, the cache entry is not seeded and the existing prior-read protection remains in effect; the refresh itself continues without failing.QWEN.md,AGENTS.md, team-memory indexes, and broader system-prompt file tracking.Linked Issues
Fixes #7287
中文说明
What this PR does
当项目级和用户级自动记忆
MEMORY.md被加载到 system prompt 时,将这两次读取登记到当前 session 的FileReadCache中。Why it's needed
模型实际上已经通过 auto-memory prompt 看到了这些索引文件,但 prior-read guard 并不知道这次读取。因此,在全新 session 中第一次使用
write_file更新MEMORY.md时,必须额外调用一次read_file,否则写入会被拒绝。Reviewer Test Plan
How to verify
运行
npm test --workspace=@qwen-code/qwen-code-core -- src/config/config.test.ts。回归测试会创建项目级和用户级MEMORY.md,刷新 hierarchical memory,并验证checkPriorRead()可以在没有显式调用read_file的情况下允许覆盖这两个索引文件。测试使用临时 memory base directory,并在结束后恢复环境变量,不会修改真实用户 memory 目录。
Evidence (Before & After)
不适用:这是不可见的 core 行为和回归测试改动。
Tested on
Environment (optional)
Windows 本地 npm workspace。
config.test.ts的 419 个测试全部通过,改动文件也通过了 ESLint 和 Prettier 检查。Risk & Scope
stat失败,cache 不会登记该文件,但原有 prior-read 保护仍然生效;refresh 本身不会失败。QWEN.md、AGENTS.md、team-memory 索引以及更广泛的 system-prompt 文件追踪。Linked Issues
Fixes #7287