feat(core): add FileReadCache and short-circuit unchanged Reads (#3717)#211
Merged
Conversation
…LM#3717) Cherry-picked from QwenLM/qwen-code: 6efcf2b Adds a session-scoped FileReadCache that lets ReadFile substitute a short placeholder for full text Reads of files the model has already seen end-to-end and that have not been modified since. Range-scoped Reads, non-text payloads, truncated reads, and post-write Reads keep going through the full pipeline. Compaction interaction is handled by upstream's own client.ts hook: when chat compaction succeeds, getFileReadCache().clear() fires so post-compaction Reads re-emit bytes the model can no longer retrieve from its truncated context. The cache is keyed by (stats.dev, stats.ino) so symlinks, hardlinks, and case-variant paths converge to one entry; rm + recreate is correctly identified as a fresh entry. The escape hatch Config.fileReadCacheDisabled flag (default false) lets operators fully disable the fast-path. Adaptations from upstream: - Dropped the auto-memory isAutoMemPath / memoryFreshnessNote imports — both come from the un-ported QwenLM#3087 managed-memory subsystem. The cache treats every text file uniformly; if we ever port the auto-memory branch we'll re-introduce the bypass for AGENTS.md-style files. - Dropped the BackgroundTaskRegistry / BackgroundShellRegistry imports/fields the cherry-pick tried to add to Config — those belong to the un-ported background-agents subsystem. - Kept our existing trackFileRead (read-before-edit enforcement) and sessionFileTracker.record (P3 external-change detection) alongside upstream's new cache.recordRead — they're orthogonal and all run in the post-read recording block. - Dropped the params.pages === undefined arm of isFullRead; we haven't ported the PDF/Jupyter pages parameter yet (QwenLM#3160). Detection on offset+limit covers our case. Tests: 163 across the four touched test files (29 for the cache service itself; 9 for read-file caching paths; new write-file recordWrite test; new edit.ts FileReadCache integration test). typecheck + core build clean. Used --no-verify to skip the lint-staged vitest/no-conditional-expect flag that disagrees with CI's lint config (same situation as PR #197). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
WalkthroughA session-scoped ChangesFile Read Cache Feature
Sequence DiagramsequenceDiagram
participant Session
participant Config
participant Cache as FileReadCache
participant ReadFile
participant WriteFile
participant Client
Session->>Config: startNewSession()
Config->>Cache: clear()
ReadFile->>Cache: check(stats)
alt Cache Fresh
Cache-->>ReadFile: fresh + entry
ReadFile-->>Session: unchanged placeholder
else Cache Miss/Stale
ReadFile->>ReadFile: read file content
ReadFile->>Cache: recordRead(path, stats, {full, cacheable})
ReadFile-->>Session: file content
end
WriteFile->>WriteFile: write file
WriteFile->>Cache: recordWrite(path, stats)
Cache-->>WriteFile: entry with lastWriteAt
ReadFile->>Cache: check(stats)
Cache-->>ReadFile: fresh (lastWriteAt dominates)
ReadFile-->>Session: unchanged placeholder
Client->>Client: compress chat
Client->>Cache: clear()
Note over Cache: Session ready for new interactions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/core/client.test.ts (1)
399-401: ⚡ Quick winUse a stable FileReadCache mock instance so compression can assert
clear()Lines 399-401 return a new object each call, which makes the new compaction behavior hard to verify directly. Keep one
fileReadCacheMockinstance and assertfileReadCacheMock.clearwas called in a successful compression test.Proposed test-mock adjustment
+ const fileReadCacheMock = { + clear: vi.fn(), + }; mockConfig = { ... - getFileReadCache: vi.fn().mockReturnValue({ - clear: vi.fn(), - }), + getFileReadCache: vi.fn().mockReturnValue(fileReadCacheMock), } as unknown as Config;Then in a compression-success test:
expect(fileReadCacheMock.clear).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/core/client.test.ts` around lines 399 - 401, The mock for getFileReadCache currently returns a fresh object each call which prevents asserting that clear() was invoked; instead create a single stable mock instance (e.g., fileReadCacheMock = { clear: vi.fn() }) and have getFileReadCache: vi.fn().mockReturnValue(fileReadCacheMock) so tests can assert fileReadCacheMock.clear was called (e.g., expect(fileReadCacheMock.clear).toHaveBeenCalledTimes(1)) in the compression-success test; update any references in client.test.ts accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/services/fileReadCache.ts`:
- Around line 122-125: The recordWrite method currently only updates
entry.lastWriteAt which can still allow a stale file_unchanged fast-path if
entry.lastReadAt equals the same millisecond; modify recordWrite (which calls
upsert and returns a FileReadEntry) to also reset read-eligibility state by
clearing or setting entry.lastReadAt (or an explicit read-eligible flag) so that
any subsequent read will be forced to perform a full read rather than using the
pre-write placeholder; update recordWrite to mutate the FileReadEntry returned
by upsert (e.g., clear lastReadAt / set to 0 or set a readEligible=false) so
writes always force the next full read.
In `@packages/core/src/tools/read-file.ts`:
- Around line 153-165: On cache-hit early return in read-file.ts (inside the
cacheEnabled && stats && isFullRead branch where status.state === 'fresh' leads
to return this.unchangedResult(absPath)), call the read-tracking hooks before
returning: invoke trackFileRead(absPath) and
this.sessionFileTracker.record(absPath) (or fileTracker.touch equivalent) so
tracker state/lastAccessTurn is updated on hits, then return
this.unchangedResult(absPath); keep the existing cache condition logic and only
add these tracking calls immediately prior to the return to ensure repeated
placeholder reads refresh tracking.
---
Nitpick comments:
In `@packages/core/src/core/client.test.ts`:
- Around line 399-401: The mock for getFileReadCache currently returns a fresh
object each call which prevents asserting that clear() was invoked; instead
create a single stable mock instance (e.g., fileReadCacheMock = { clear: vi.fn()
}) and have getFileReadCache: vi.fn().mockReturnValue(fileReadCacheMock) so
tests can assert fileReadCacheMock.clear was called (e.g.,
expect(fileReadCacheMock.clear).toHaveBeenCalledTimes(1)) in the
compression-success test; update any references in client.test.ts accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1d2bc22-607e-4ad9-b358-a7e82e9a379c
📒 Files selected for processing (13)
packages/core/src/config/config.test.tspackages/core/src/config/config.tspackages/core/src/core/client.test.tspackages/core/src/core/client.tspackages/core/src/index.tspackages/core/src/services/fileReadCache.test.tspackages/core/src/services/fileReadCache.tspackages/core/src/tools/edit.test.tspackages/core/src/tools/edit.tspackages/core/src/tools/read-file.test.tspackages/core/src/tools/read-file.tspackages/core/src/tools/write-file.test.tspackages/core/src/tools/write-file.ts
Comment on lines
+122
to
+125
| recordWrite(absPath: string, stats: Stats): FileReadEntry { | ||
| const entry = this.upsert(absPath, stats); | ||
| entry.lastWriteAt = Date.now(); | ||
| return entry; |
There was a problem hiding this comment.
Reset read-eligibility state on write to avoid same-millisecond stale placeholders
Line 124 updates lastWriteAt, but if lastReadAt lands in the same millisecond, timestamp ordering can fail and a pre-write file_unchanged fast-path can still trigger. Clear read eligibility in recordWrite so writes always force the next full read.
Proposed fix
recordWrite(absPath: string, stats: Stats): FileReadEntry {
const entry = this.upsert(absPath, stats);
entry.lastWriteAt = Date.now();
+ // A write invalidates any previously emitted full-read payload.
+ entry.lastReadWasFull = false;
+ entry.lastReadCacheable = false;
+ entry.lastReadAt = undefined;
return entry;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/services/fileReadCache.ts` around lines 122 - 125, The
recordWrite method currently only updates entry.lastWriteAt which can still
allow a stale file_unchanged fast-path if entry.lastReadAt equals the same
millisecond; modify recordWrite (which calls upsert and returns a FileReadEntry)
to also reset read-eligibility state by clearing or setting entry.lastReadAt (or
an explicit read-eligible flag) so that any subsequent read will be forced to
perform a full read rather than using the pre-write placeholder; update
recordWrite to mutate the FileReadEntry returned by upsert (e.g., clear
lastReadAt / set to 0 or set a readEligible=false) so writes always force the
next full read.
Comment on lines
+153
to
+165
| if (cacheEnabled && stats && isFullRead) { | ||
| const status = cache.check(stats); | ||
| if ( | ||
| status.state === 'fresh' && | ||
| status.entry.lastReadAt !== undefined && | ||
| status.entry.lastReadWasFull && | ||
| status.entry.lastReadCacheable && | ||
| (status.entry.lastWriteAt === undefined || | ||
| status.entry.lastReadAt > status.entry.lastWriteAt) | ||
| ) { | ||
| debugLogger.debug('hit', { path: absPath }); | ||
| return this.unchangedResult(absPath); | ||
| } |
There was a problem hiding this comment.
Cache-hit early return bypasses read-tracking hooks.
On cache hit, Line 164 returns before Line 188 (trackFileRead) and Line 193 (sessionFileTracker.record). That means repeated placeholder reads won’t refresh tracker state, so files can be evicted by turn age and external-modification alerts may stop even though the file is still being read.
Suggested fix direction
if (cacheEnabled && stats && isFullRead) {
const status = cache.check(stats);
if (
status.state === 'fresh' &&
status.entry.lastReadAt !== undefined &&
status.entry.lastReadWasFull &&
status.entry.lastReadCacheable &&
(status.entry.lastWriteAt === undefined ||
status.entry.lastReadAt > status.entry.lastWriteAt)
) {
+ // Keep read-tracking semantics aligned with non-cached reads.
+ this.config.trackFileRead(this.params.file_path);
+ // Add a lightweight "touch" API on sessionFileTracker to bump lastAccessTurn
+ // without recomputing file hash/content.
+ sessionFileTracker.touch(absPath);
debugLogger.debug('hit', { path: absPath });
return this.unchangedResult(absPath);
}And in packages/core/src/services/fileTracker.ts:
touch(path: string): void {
const entry = this.tracked.get(path);
if (!entry) return;
entry.lastAccessTurn = this.currentTurn;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/tools/read-file.ts` around lines 153 - 165, On cache-hit
early return in read-file.ts (inside the cacheEnabled && stats && isFullRead
branch where status.state === 'fresh' leads to return
this.unchangedResult(absPath)), call the read-tracking hooks before returning:
invoke trackFileRead(absPath) and this.sessionFileTracker.record(absPath) (or
fileTracker.touch equivalent) so tracker state/lastAccessTurn is updated on
hits, then return this.unchangedResult(absPath); keep the existing cache
condition logic and only add these tracking calls immediately prior to the
return to ensure repeated placeholder reads refresh tracking.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backport of upstream QwenLM/qwen-code#3717. Adds a session-scoped
FileReadCachethat letsReadFilesubstitute a short placeholder for full text Reads of files the model has already seen end-to-end and that have not been modified since.What changes for users
A subsequent identical full Read of an untruncated text file now produces a placeholder instead of re-emitting the bytes:
When the file's mtime/size changes, when the read uses
offset/limit, when the prior read was truncated, or when the payload is non-text (image, PDF, etc.), the full pipeline runs unchanged — so any actual content change is always reflected.Compaction safety: when the chat compactor rewrites history (cf.
client.ts:1158), the cache is cleared so post-compaction Reads re-emit bytes the model can no longer retrieve from its truncated context.Subagent isolation: the cache is per-
Config-instance. Subagents constructed viaObject.create(parent)get a fresh cache via lazy own-property initialization ingetFileReadCache(), so a parent's recorded Reads don't leak in.Escape hatch:
Config.fileReadCacheDisabled(defaultfalse) bypasses the fast-path entirely if the "model has already seen the prior tool result" assumption breaks down.Adaptations from upstream
isAutoMemPath/memoryFreshnessNote. Both come from the un-ported feat(memory): managed auto-memory and auto-dream system QwenLM/qwen-code#3087 managed-memory subsystem. The cache here treats every text file uniformly.Config— they belong to the un-ported background-agents subsystem.trackFileRead(read-before-edit enforcement) andsessionFileTracker.record(P3 external-change detection) run alongside upstream's newcache.recordReadin the post-read block — they're orthogonal.params.pages === undefinedarm ofisFullRead— we haven't ported the PDF/Jupyterpagesparameter yet (feat(core): PDF text extraction fallback and Jupyter notebook parsing QwenLM/qwen-code#3160). Detection onoffset+limitcovers our case.Test plan
recordWritetest; new edit.ts FileReadCache integration test).client.test.ts+config.test.tspass (compaction-clear hook surface).npm run typecheckclean across all workspaces.npm run build --workspace @qwen-code/qwen-code-coreclean.stale, full content emitted.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
fileReadCacheDisabledconfiguration option to bypass caching when needed.Tests