fix(core): target microcompaction cache disarms - #5407
Conversation
| entry.realPath === absPath || | ||
| resolvePath(entry.realPath) === target | ||
| ) { | ||
| this.byInode.delete(key); |
There was a problem hiding this comment.
[Suggestion] invalidateByPath deletes the entire cache entry (mtimeMs, sizeBytes, lastReadAt, lastReadCacheable, readResidentInHistory) while markReadEvictedFromHistory only sets entry.readResidentInHistory = false, deliberately preserving the fingerprint. This creates two issues:
- If a file was replaced then re-read, both the stale entry (old inode) and a fresh entry (new inode) share the same
realPath.invalidateByPathdeletes both — the fresh entry's prior-read enforcement data is lost. priorReadEnforcement.tstreatsstate: 'unknown'andstate: 'stale'differently; full deletion causes'unknown'where'stale'would be more appropriate.
Consider setting readResidentInHistory = false instead of deleting, to match markReadEvictedFromHistory semantics:
invalidateByPath(absPath: string): boolean {
const target = resolvePath(absPath);
let disarmed = false;
for (const entry of this.byInode.values()) {
if (resolvePath(entry.realPath) === target) {
entry.readResidentInHistory = false;
disarmed = true;
}
}
return disarmed;
}— qwen3.7-max via Qwen Code /review
| @@ -294,9 +295,10 @@ export class FileReadCache { | |||
| * Returns `true` if a matching entry was found and disarmed; `false` | |||
| * if there is no entry for `stats` (never tracked, or `stats` | |||
| * resolved to a different inode than recorded — file replaced / | |||
There was a problem hiding this comment.
[Suggestion] The realPath field's JSDoc (lines 45–49) still states "Diagnostic only — it is *not* used for lookup", but invalidateByPath now uses it as a lookup key (entry.realPath === absPath || resolvePath(entry.realPath) === target). The field's contract has silently changed from "informational" to "load-bearing". Future refactors that treat realPath as diagnostic-only (e.g., dropping it or changing how it's populated) will break invalidateByPath with no compile-time signal.
Update the realPath doc to reflect the new usage:
/**
* Last absolute path we observed pointing at this inode. Used by
* {@link invalidateByPath} as a best-effort lookup fallback when
* Stats are unavailable. Multiple paths can resolve to the same
* inode (symlinks, case variants), so this match is not authoritative.
*/
realPath: string;— qwen3.7-max via Qwen Code /review
| } | ||
| const evictedPath = meta.evictedReadPaths[i]; | ||
| if (evictedPath) { | ||
| fileReadCache.invalidateByPath(evictedPath); |
There was a problem hiding this comment.
[Suggestion] invalidateByPath(evictedPath) return value is silently discarded. When it returns false (no cache entry found for this path), there is no logging or fallback. The old code called clear() which was at least visible in debug logs. If a stale armed entry survives because path matching failed (e.g., symlink variant, CWD change between read and eviction), the failure is completely invisible.
Consider logging when invalidation misses:
if (evictedPath) {
const invalidated = fileReadCache.invalidateByPath(evictedPath);
if (!invalidated) {
debugLogger.debug(
`[FILE_READ_CACHE] invalidateByPath miss for ${evictedPath} after ${logTag}`,
);
}
usedPathFallback = true;
}— qwen3.7-max via Qwen Code /review
| debugLogger.debug( | ||
| `[FILE_READ_CACHE] disarmed fast-path for ` + | ||
| `[FILE_READ_CACHE] disarmed fast-path by path for ` + | ||
| `${meta.evictedReadPaths.length} file(s) after ${logTag}`, |
There was a problem hiding this comment.
[Suggestion] Both branches log nearly identical messages — the only difference is "by path" in one. Both report meta.evictedReadPaths.length regardless of how many actually used the path fallback vs. succeeded via inode stats. An engineer debugging cache behavior from logs cannot easily distinguish the two paths or tell how many disarms went through each sub-path.
Consider differentiating more clearly or merging into a single log:
debugLogger.debug(
`[FILE_READ_CACHE] disarmed fast-path for ` +
`${meta.evictedReadPaths.length} file(s) after ${logTag}` +
(usedPathFallback ? ' (path fallback used)' : ''),
);— qwen3.7-max via Qwen Code /review
| ); | ||
| if (!sizePlan) { | ||
| return { history }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] keptPathRefs is initialized to [] and only populated in each branch (tool for force/idle, sizePlan.toolRefs for size). The kept-path filter in the clearing loop suppresses eviction for all FILE_PATH_TOOLS (read_file, edit, write_file) when a kept same-path result exists. This is safe for read_file, but for EDIT/WRITE_FILE evictions, a kept READ result for the same path would suppress the write's cache disarm — the fast-path could retain post-read readResidentInHistory = true even though the file was modified.
Currently unreachable because keepRecent always keeps the most recent (post-write) result, but the invariant is fragile — any future change to the clearing strategy (priority-based, size-weighted) could expose it. Consider restricting the kept-path suppression to READ_FILE evictions only:
if (
part.functionResponse.name !== ToolNames.READ_FILE ||
!keptFilePaths.has(p)
) {
evictedReadPaths.add(p);
}— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @tt-a1i — thanks for the fix! The code changes look focused, but the PR body doesn't follow our PR template.
The template requires these sections:
- What this PR does — prose description of the change
- Why it's needed — motivation / problem being solved
- Reviewer Test Plan — how a reviewer can verify this (with Before/After evidence)
- Risk & Scope — main risk, what's out of scope, breaking changes
- Linked Issues — closing keyword or reference
Right now the body has ## Summary, ## Tests, and ## AI Assistance Disclosure which don't map to the template. The Reviewer Test Plan section is especially important — maintainers use it to verify the fix without having to reverse-engineer the reproduction. Without it, review gets delayed.
Could you update the PR description to match the template? Once that's done I'll continue the review.
中文说明
@tt-a1i 感谢修复！代码改动很集中，但 PR 正文没有使用我们的 PR 模板。
模板要求以下章节：
- What this PR does — 用文字描述改动内容
- Why it's needed — 动机 / 要解决的问题
- Reviewer Test Plan — 审查者如何验证（含 Before/After 证据）
- Risk & Scope — 主要风险、不在范围内的内容、破坏性变更
- Linked Issues — 关闭关键词或引用
当前正文用的是 ## Summary、## Tests、## AI Assistance Disclosure，跟模板对不上。Reviewer Test Plan 尤其重要——维护者靠它来验证修复，缺了会导致 review 延迟。
请更新 PR 描述以匹配模板，更新后我会继续审查。
— Qwen Code · qwen3.7-max
|
updated the PR description to match the template. code is unchanged. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-reviewed at HEAD a7d33b3a — no new issues. I re-verified the core correctness of the targeted cache disarm: an unrecoverable blanked read (unresolvedEvictedReads > 0) still falls back to a blanket clear() (client.ts:2714 — the safety net is intact), path-resolution failures are narrowed to invalidateByPath instead of wiping everything, and buildKeptFilePaths only protects a path it can prove is single-resident (conservative — errs toward disarming, so no stale-cache risk). core typecheck clean; changed-area tests pass (305); CI green (58 checks).
The 5 suggestions from my earlier review remain open (all non-blocking, already inline):
fileReadCache.ts:333—invalidateByPathdeletes the whole entry rather than just disarming (inconsistent withmarkReadEvictedFromHistory).fileReadCache.ts:297—realPathJSDoc still says "not used for lookup", butinvalidateByPathnow matches on it.client.ts:2738—invalidateByPath's return value is discarded (a no-op miss is indistinguishable from a real disarm).client.ts:2745— the two debug branches log near-identical messages.microcompact.ts:523—keptPathRefs/keptPathHistoryinit + per-branch population could be tightened.
No blockers from the code review. (The bot's CHANGES_REQUESTED is about the PR-template sections, not the code.)
— claude-opus-4-8 via Claude Code /qreview
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ — all required sections present, bilingual, linked issue. On direction: this is squarely in scope — a real bug (#4259) where microcompaction incorrectly disarms paths that still have quotable read results in context. Core engine caching behavior, exactly the kind of fix that matters for long-session reliability. No Claude Code CHANGELOG reference for this specific area, but microcompaction and context management are core mission. On approach: the scope feels right — three focused source changes ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必要章节齐全，双语，关联了 issue。 方向：完全在范围内 — #4259 是一个真实的 bug，microcompaction 会错误地把仍然有可引用 read result 的路径解除武装。这是核心引擎缓存行为，对长会话可靠性很重要。Claude Code CHANGELOG 中没有直接相关的参考，但 microcompaction 和上下文管理是核心任务。 方案：范围合理 — 三处聚焦的源码改动（cache 上的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): to fix #4259, I'd (1) filter out paths from The PR matches this proposal closely. The implementation is clean and the three source changes are well-separated:
No critical blockers found. Two minor observations:
These were also raised by @wenshao's earlier inline suggestions and remain unaddressed at commit TestsAll 305 unit tests pass across the three affected test files:
Typecheck ( Real-Scenario Testing
Installed build (v0.18.3) — smoke testDev build (this PR) — blockedThis change is internal (caching behavior during microcompaction) — no user-visible CLI output difference. The behavioral improvement is fully validated by the unit test suite: before this PR, 中文说明代码审查独立方案（在阅读 diff 之前）：要修复 #4259，我会 (1) 在 context 中仍有同路径 kept result 时，从 PR 与这个方案高度吻合。 实现干净，三处源码改动分离得当：
无关键阻塞问题。 两个小观察：
这些也是 @wenshao 之前 inline suggestions 提出的，在 commit 测试305 个单元测试全部通过：
Typecheck、Lint 均通过 ✅ 真实场景测试
已用安装版 qwen v0.18.3 做了基础 smoke test，文件读取功能正常。本 PR 的改动是内部缓存行为，无 CLI 可见输出差异，行为改进由单元测试充分验证。 — Qwen Code · qwen3.7-max |
Final ReflectionStepping back: this PR solves a genuine problem cleanly. The motivation is clear (microcompaction shouldn't disarm paths that are still quotable), the implementation is minimal and well-separated, and the test coverage is thorough — 305 tests including edge cases for reused ids, pending content, and ghost files. Going back to my independent proposal: the PR matches it closely, and in fact the The two minor items (stale The Verdict: approve. The code is correct, focused, well-tested, and solves the stated problem. The two minor suggestions from the prior review are worth addressing but don't block the merge. 中文说明最终评审回顾全局：这个 PR 干净地解决了一个真实问题。动机清晰（microcompaction 不应解除仍然可引用的路径的武装），实现最小且分离得当，测试覆盖充分 — 305 个测试涵盖了复用 id、pending content、ghost file 等边界情况。 回到我的独立方案：PR 与之高度吻合，而且 两个小问题（过时的
结论：approve。 代码正确、聚焦、测试充分，解决了所述问题。前次 review 中的两个小建议值得处理，但不阻塞合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ Code is correct, focused, and well-tested. Minor polish items (stale JSDoc, log dedup) from prior review are non-blocking.
Maintainer verification — local merge build + before/after (both halves)Verified the 3-way merge result ( 1. What the fix does (#4259)Microcompaction "disarms" FileReadCache entries for the file-read results it blanks. Two over-aggressive behaviors are fixed:
2. Build / tests / typecheck / lint (merge result)
3. Before/after A — microcompaction keeps same-path reads quotableRunning the PR's new tests against the old So the two new protections genuinely change behavior, while the ambiguity safety and all existing #4239 tests are preserved. With the PR source → all pass. 4. Before/after B — client targets the path instead of wiping the cacheRunning the PR's tests against the old All 4 fail on old code because it calls 5. Consistency / safety audit
Note on merge state
VerdictCorrect, well-scoped, and thoroughly tested two-part fix: it stops microcompaction from evicting paths that are still quotable, and makes the cache fallback surgical (one ghost file no longer wipes the whole cache) — all while preserving the #4239-safe behavior for ambiguous ids and the blanket wipe for truly-unresolvable reads. Merge is clean, typecheck/lint clean, 305/305 local, CI green on all three platforms. ✅ Safe to merge. Verified by maintainer @wenshao: 3-way merge build ( 中文版（点击展开）维护者验证 —— 本地合并构建 + 前后对比（两个半部分）在 macOS（Darwin arm64，Node v22.22.2）上验证了三方合并结果（ 1. 修复做了什么（#4259）microcompaction 会为它清空（blank）的文件读取结果"解除"（disarm）FileReadCache 条目。本 PR 修了两个过激行为：
2. 构建 / 测试 / 类型检查 / lint（合并结果）
3. 前后对比 A —— microcompaction 让同路径读取保持可引用用 PR 的新测试去打旧版 即：两个新保护确实改变了行为，而歧义安全性和全部既有 #4239 测试都被保留。换上 PR 的源后 → 全部通过。 4. 前后对比 B —— client 只针对该 path，而非清空整个缓存用 PR 的测试去打旧版 这 4 个在旧代码上都失败，因为旧代码在 PR 期望 5. 一致性 / 安全审计
关于合并状态
结论正确、范围克制、测试充分的两段式修复：它阻止 microcompaction 驱逐仍可引用的 path，并让缓存回退变成外科手术式（一个 ghost 文件不再清空整个缓存）——同时保留了对歧义 id 的 #4239 安全行为，以及对真正无法解析读取的全量清空。合并干净，类型检查/lint 干净，本地 305/305，CI 三平台全绿。✅ 可以合并。 维护者 @wenshao 验证：三方合并构建（ |
3 participants
What this PR does
This avoids reporting evicted read paths when a kept same-path tool result remains quotable. It adds a path-level FileReadCache fallback for stat failures or inode mismatches, while keeping the blanket clear fallback for idless or unlinkable blanked reads.
Why it's needed
Issue #4259 showed that microcompaction could disarm quoting for a file path even when the conversation still retained a same-path read result that should remain quotable. That makes later file references look unavailable even though useful read context is still present. The fix lets the cache fall back by path when identity checks are unavailable, without trusting unrelated blanked reads.
Reviewer Test Plan
How to verify
Check that microcompaction keeps a path quotable when a retained same-path read result exists, and still clears broadly for idless or unlinkable blanked reads. The tests cover the FileReadCache fallback and the microcompaction/client behavior that consumes it.
Evidence (Before & After)
Before: a blanked read could cause microcompaction to report a path as evicted even when a kept same-path read remained in context. After: same-path retained reads keep that path quotable, and the broad clear fallback remains for ambiguous blanked reads.
Tested on
Environment (optional)
Node 22 via
npx -p node@22.Commands run locally:
npx -p node@22 node node_modules/vitest/vitest.mjs run --coverage.enabled=false packages/core/src/services/microcompaction/microcompact.test.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/core/client.test.tsnpx -p node@22 node node_modules/typescript/bin/tsc --noEmit --project packages/core/tsconfig.jsonnpx -p node@22 node node_modules/eslint/bin/eslint.js packages/core/src/services/microcompaction/microcompact.ts packages/core/src/services/microcompaction/microcompact.test.ts packages/core/src/services/fileReadCache.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/core/client.ts packages/core/src/core/client.test.tsnpx -p node@22 node node_modules/prettier/bin/prettier.cjs --check packages/core/src/services/microcompaction/microcompact.ts packages/core/src/services/microcompaction/microcompact.test.ts packages/core/src/services/fileReadCache.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/core/client.ts packages/core/src/core/client.test.tsgit diff --checkRisk & Scope
Linked Issues
Fixes #4259
中文说明
What this PR does
这个 PR 避免在仍然保留同路径可引用 tool result 时，把对应 read path 报告成已驱逐。它为 stat 失败或 inode 不匹配增加了 path-level FileReadCache fallback，同时保留 idless 或无法关联路径的 blanked reads 的 blanket clear fallback。
Why it's needed
#4259 暴露出 microcompaction 可能会让一个文件路径失去引用能力，即使对话里还保留着同路径、仍应可引用的 read result。这样后续文件引用会看起来不可用，但实际上还有有用的 read context。这个修复在 identity check 不可用时按 path fallback，同时不会信任无关的 blanked reads。
Reviewer Test Plan
How to verify
确认当保留了同路径 read result 时，microcompaction 会让该 path 继续可引用；同时对于 idless 或无法关联路径的 blanked reads，仍然会走 broad clear。测试覆盖了 FileReadCache fallback，以及消费它的 microcompaction/client 行为。
Evidence (Before & After)
Before：blanked read 可能让 microcompaction 把某个 path 报告成已驱逐，即使 context 里还保留着同路径 read。After：同路径 retained reads 会让该 path 保持可引用，ambiguous blanked reads 仍然保留 broad clear fallback。
Tested on
Environment (optional)
通过
npx -p node@22使用 Node 22。本地运行过：
npx -p node@22 node node_modules/vitest/vitest.mjs run --coverage.enabled=false packages/core/src/services/microcompaction/microcompact.test.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/core/client.test.tsnpx -p node@22 node node_modules/typescript/bin/tsc --noEmit --project packages/core/tsconfig.jsonnpx -p node@22 node node_modules/eslint/bin/eslint.js packages/core/src/services/microcompaction/microcompact.ts packages/core/src/services/microcompaction/microcompact.test.ts packages/core/src/services/fileReadCache.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/core/client.ts packages/core/src/core/client.test.tsnpx -p node@22 node node_modules/prettier/bin/prettier.cjs --check packages/core/src/services/microcompaction/microcompact.ts packages/core/src/services/microcompaction/microcompact.test.ts packages/core/src/services/fileReadCache.ts packages/core/src/services/fileReadCache.test.ts packages/core/src/core/client.ts packages/core/src/core/client.test.tsgit diff --checkRisk & Scope
Linked Issues
Fixes #4259
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.