feat(core): let grep results satisfy prior-read checks#5043
Conversation
| try { | ||
| const stats = await fs.stat(filePath); | ||
| if (!stats.isFile()) { | ||
| return; |
There was a problem hiding this comment.
[Critical] cacheable: true unconditionally overrides a prior cacheable: false for non-text files (e.g., .ipynb) via the sticky-on-true logic in FileReadCache.recordRead.
Exploit chain:
read_file(notebook.ipynb)→ setscacheable: false(notebooks return structured payload,originalLineCountisundefined)- grep matches the same file →
recordRead({full: false, cacheable: true})→ same fingerprint → sticky-on-true upgradeslastReadCacheabletotrue checkPriorReadnow approves Edit/WriteFile against a notebook the model never saw as editable text, risking silent JSON corruption
Ripgrep does NOT skip .ipynb (it's a text file). The .ipynb scenario is the most concrete, but any file type that read_file returns as non-cacheable is affected.
| return; | |
| const ext = path.extname(filePath).toLowerCase(); | |
| const cacheable = ext !== '.ipynb'; | |
| cache.recordRead(filePath, stats, { full: false, cacheable }); |
— qwen3.7-max via Qwen Code /review
| const fileAStats = await fs.stat(path.join(tempRootDir, 'fileA.txt')); | ||
| const fileCStats = await fs.stat( | ||
| path.join(tempRootDir, 'sub', 'fileC.txt'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] These assertions only verify state === 'fresh' but do not check lastReadWasFull === false or lastReadCacheable === true. The {full: false, cacheable: true} contract — which is the entire semantic point of this PR — is unverified. If someone accidentally flipped full to true, these tests would still pass but the Read tool would incorrectly return file_unchanged placeholders.
| ); | |
| const fileACheck = fileReadCache.check(fileAStats); | |
| expect(fileACheck.state).toBe('fresh'); | |
| if (fileACheck.state === 'fresh') { | |
| expect(fileACheck.entry.lastReadWasFull).toBe(false); | |
| expect(fileACheck.entry.lastReadCacheable).toBe(true); | |
| } | |
| const fileCCheck = fileReadCache.check(fileCStats); | |
| expect(fileCCheck.state).toBe('fresh'); | |
| if (fileCCheck.state === 'fresh') { | |
| expect(fileCCheck.entry.lastReadWasFull).toBe(false); | |
| expect(fileCCheck.entry.lastReadCacheable).toBe(true); | |
| } |
— qwen3.7-max via Qwen Code /review
|
|
||
| import fs from 'node:fs/promises'; | ||
| import type { Config } from '../config/config.js'; | ||
|
|
There was a problem hiding this comment.
[Suggestion] No dedicated grepReadTracking.test.ts for this new module. The function has 5 code paths (cache disabled, no cache, happy path, stat failure, non-file result) but only the happy path is exercised indirectly through grep/ripGrep tests. The early-return guards (getFileReadCacheDisabled, getFileReadCache?.() returning undefined) and the error catch-all have zero test coverage.
— qwen3.7-max via Qwen Code /review
| } | ||
| cache.recordRead(filePath, stats, { full: false, cacheable: true }); | ||
| } catch { | ||
| // Grep should not fail because a matched file disappeared before |
There was a problem hiding this comment.
[Suggestion] The catch block swallows ALL errors, not just ENOENT. The comment justifies only the file-disappeared case, but EACCES, EIO, EBUSY, and NFS errors are also silently discarded with zero logging. If all grep result files fail stat on a network filesystem, the entire optimization becomes a silent no-op with no diagnostic path.
Consider narrowing the catch to ENOENT and adding a debug log for other failures:
| // Grep should not fail because a matched file disappeared before | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| // Log non-ENOENT failures for debugging | |
| console.debug(`grepReadTracking: stat failed for ${filePath}: ${(err as Error).message}`); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| await Promise.all( |
There was a problem hiding this comment.
[Suggestion] Promise.all fires N concurrent fs.stat calls with no concurrency limit. For a grep returning hundreds of unique file paths, this creates a burst of simultaneous syscalls on libuv's thread pool. More importantly, the await at both call sites (grep.ts:282, ripGrep.ts:370) blocks the grep tool's return on the stat burst completing.
Consider either batching:
const BATCH = 50;
for (let i = 0; i < unique.length; i += BATCH) {
await Promise.all(unique.slice(i, i + BATCH).map(async (fp) => { ... }));
}Or fire-and-forget (since cache stamping is a side-effect that doesn't affect the grep return value):
void recordGrepResultFileReads(this.config, resultFilePaths);— qwen3.7-max via Qwen Code /review
he-yufeng
force-pushed
the
fix/grep-satisfies-prior-read
branch
from
40eb598 to
192f78d
Compare
|
Thanks for the review. I updated the patch to address the cache-safety concerns:
Validated locally:
|
Runtime verification — real TUI, tmux-driven (maintainer merge reference)Verdict: PASS — built PR head Claim (my read of the diff)
Method
Steps
Findings
Build: 中文版（验证报告）运行时验证 — tmux 驱动真实 TUI（合并参考）结论：PASS — 本地构建 PR head 对 diff 的理解
方法
步骤
观察
构建:两个 worktree 上 |
3 participants
What this PR does
This PR lets the structured grep tools stamp the session FileReadCache for files whose matching lines are actually returned to the model. Both the JS fallback GrepTool and the ripgrep-backed implementation now record visible result file paths as partial, cacheable text reads, so a follow-up Edit or WriteFile can use the existing prior-read enforcement path instead of requiring a separate read_file call.
Why it's needed
Issue #4939 points out that grep-to-edit is a common workflow: the model has already seen the matching line, but Edit currently rejects the file because only read_file populates FileReadCache. Reusing recordRead keeps the existing mtime/size drift check and non-text rejection behavior intact, while avoiding an unnecessary extra tool call for files that grep already exposed.
Reviewer Test Plan
How to verify
Run the focused tool tests and confirm grep/ripgrep results still return the same resultFilePaths while the matching files are marked fresh in FileReadCache. The change intentionally records grep output as full: false, cacheable: true, so it satisfies prior-read enforcement without enabling the read_file full-read fast path.
Commands I ran locally:
npm test --workspace=@qwen-code/qwen-code-core -- src/tools/grep.test.ts src/tools/ripGrep.test.ts npm run lint --workspace=@qwen-code/qwen-code-core npm run typecheck --workspace=@qwen-code/qwen-code-core npx prettier --check packages/core/src/tools/grepReadTracking.ts packages/core/src/tools/grep.ts packages/core/src/tools/ripGrep.ts packages/core/src/tools/grep.test.ts packages/core/src/tools/ripGrep.test.ts git diff --checkEvidence (Before & After)
N/A. This is core tool state tracking rather than a UI change.
Tested on
Environment (optional)
Windows 11, Node/npm from the local Qwen Code workspace.
npm cicompleted first; it reported existing npm audit/deprecation warnings but no install failure.Risk & Scope
Linked Issues
Part of #4939.
中文说明
What this PR does
这个 PR 让结构化 grep 工具在把匹配行返回给模型时，同步把这些文件记录到当前会话的 FileReadCache。JS fallback 的 GrepTool 和 ripgrep 版本都会把可见结果文件记录为 partial、cacheable 的文本 read，因此后续 Edit / WriteFile 可以复用现有 prior-read enforcement，不再强制再调用一次 read_file。
Why it's needed
#4939 提到 grep 到 edit 是非常常见的路径：模型已经看到了匹配行，但当前只有 read_file 会写入 FileReadCache，所以 Edit 仍会拒绝。这里复用 recordRead，保留现有的 mtime/size 漂移检查和非文本文件保护，同时减少一次没有必要的工具调用。
Reviewer Test Plan
How to verify
运行 focused tool tests，确认 grep/ripgrep 仍返回相同的 resultFilePaths，并且匹配文件在 FileReadCache 里变成 fresh。这个改动刻意用 full: false, cacheable: true 记录 grep 输出，所以它只满足 prior-read enforcement，不会误触发 read_file 的 full-read fast path。
本地已运行：
npm test --workspace=@qwen-code/qwen-code-core -- src/tools/grep.test.ts src/tools/ripGrep.test.ts npm run lint --workspace=@qwen-code/qwen-code-core npm run typecheck --workspace=@qwen-code/qwen-code-core npx prettier --check packages/core/src/tools/grepReadTracking.ts packages/core/src/tools/grep.ts packages/core/src/tools/ripGrep.ts packages/core/src/tools/grep.test.ts packages/core/src/tools/ripGrep.test.ts git diff --checkEvidence (Before & After)
N/A。这里改的是 core tool 状态记录，不是 UI 行为。
Tested on
Environment (optional)
Windows 11，本地 Qwen Code workspace 的 Node/npm。先运行了
npm ci，安装成功；只出现已有 npm audit / deprecated package 警告。Risk & Scope
Linked Issues
#4939 的一部分。