feat(core): refuse Edit/WriteFile when the file changed since last read

[ihubanov]

Fixes #3839.

FileReadCache.check() already returns 'stale' when on-disk (mtime, size) no longer matches what we recorded on the last Read or Write. The write path doesn't consult it. So Edit and WriteFile will happily overwrite a file that's changed under them — no error. Two parallel AgentTool calls write the same path, second one wins, first one's gone. Same when a hook touches it. Same when the user saves it in their editor. Same when an earlier tool call in the turn modified it. The cache notices. The write path doesn't.

This wires the cache into the pre-write path. Stat the target, ask the cache, and if there's a recorded prior interaction whose fingerprint no longer matches, return FILE_STALE_BEFORE_WRITE with a recovery message that names the file. Files the model has never touched fall through — first-time edits and brand new files still work. Honors the existing FileReadCacheDisabled escape hatch.

Implementation:

• FileReadCache.staleWriteReason(absPath, stats) — returns the user-visible recovery string when blocking, null otherwise.
• Two call-site checks in packages/core/src/tools/edit.ts and packages/core/src/tools/write-file.ts — both run before any write.
• New ToolErrorType.FILE_STALE_BEFORE_WRITE.

Tests:

• 5 unit tests on staleWriteReason (unknown / fresh / mtime-stale / size-stale / self-write-then-write).
• 2 integration tests in write-file.test.ts (fence fires after external mutation; first-time writes pass through).
• beforeEach cache-clear in write-file.test.ts. Without this, inode reuse on Linux makes some pre-existing fixture-recreate tests inherit cache entries from a prior test and (correctly) hit the new fence.

vitest run on the three affected files: 126/126 pass. Full vitest run shows 2 pre-existing failures in src/skills/skill-activation.test.ts (test timeouts) that reproduce on unmodified main and are unrelated to this change.

Test plan

• Read a file, modify it via another process / hook, then Edit or WriteFile it via the model — expect FILE_STALE_BEFORE_WRITE with the recovery message.
• Edit a file the model just Read with no external change in between — expect normal success.
• Write a brand new file (no prior Read) — expect normal success.
• Set FileReadCacheDisabled=true and confirm the fence is bypassed.

[wenshao]

Overview

Wires the existing FileReadCache (which already tracks (mtime, size) per inode for Read/Write) into the pre-write path of Edit and WriteFile. When the on-disk fingerprint no longer matches what the model last saw, the tool now refuses with a new FILE_STALE_BEFORE_WRITE error and a model-readable recovery message, rather than silently clobbering. Files the model never touched (brand new, or never Read in this session) still write through. Honors the existing FileReadCacheDisabled escape hatch.

The fix is small, surgical, and conceptually correct: it consumes a signal the cache was already producing.

Strengths

• Correct semantics. The fence only fires on state: 'stale' from check() — i.e., there is a prior interaction and drift. unknown (first-time touch) and fresh both fall through, which preserves all current legitimate flows including new file creation via Edit with empty old_string.
• Self-write loop is handled. recordWrite after a successful write refreshes the fingerprint to post-write stats, so a tool's own write doesn't self-block on the next call. The dedicated unit test pins this behavior.
• Escape hatch respected. Both call sites short-circuit on getFileReadCacheDisabled(), matching read-file.ts:150.
• Error type consistent with the project's ToolErrorType enum — added once in tool-error.ts, used at both call sites.
• Test fixture hygiene. The beforeEach(() => fileReadCache.clear()) in write-file.test.ts is a real fix for module-scoped cache leakage across tests after inode reuse — well-justified by its inline comment.
• Documentation. staleWriteReason's JSDoc spells out why unknown falls through and who is supposed to consume the returned string. Style matches FileReadCache's existing comment density.

Issues / Suggestions

1. Missing integration test in edit.test.ts (minor)

edit.test.ts adds getFileReadCacheDisabled: () => false to the mock — necessary, since execute now calls it — but no integration test mirrors the two new ones in write-file.test.ts for Edit. The fence path in edit.ts is currently exercised only via the underlying staleWriteReason unit test; the edit-tool wrapper (return shape, error type, no write occurring) isn't directly verified.

Suggest adding one Edit-side test: prior recordRead, mutate the file externally, attempt Edit → expect FILE_STALE_BEFORE_WRITE and the file content unchanged. Symmetric to the WriteFile test and cheap to add.

2. Redundant fs.statSync per call (minor performance)

Edit: the new fence stats once; calculateEdit will then read the file; the post-write block stats again.
WriteFile: similarly, fence stats, readTextFile then opens the file, post-write stats again.

Two-to-three syscalls per write isn't a concern for interactive tools, but preStats from the fence could be threaded into calculateEdit if you wanted to tighten it. Not blocking; flagging for completeness.

3. TOCTOU window is narrowed, not closed

The fence stat happens before calculateEdit / readTextFile. Between fence-stat and the actual write there is still a window where a third party can mutate the file. The PR description doesn't claim atomicity, and the cache is a best-effort fingerprint, not a lock — but the message wording ("Re-read the file before writing — its contents are no longer what you saw") implies a strong guarantee that doesn't strictly hold for the moment between fence and write. Acceptable as documented, but worth being honest about.

4. Inode-keyed cache: delete-and-recreate hole (known limitation, not introduced here)

If a file is deleted and recreated externally between Read and Write, the new inode is unknown to the cache and the fence won't block. This is a property of FileReadCache.inodeKey(), already acknowledged in the class-level comment. Not within this PR's scope, but worth noting that the fence is "drift detection on the same inode" rather than "the file the model saw is the file we're about to write."

A realPath-based secondary lookup (already stored on the entry) could close this on POSIX, but it'd be a separate change.

5. Recovery message could name the tool (minor wording)

Re-read the file before writing — its contents are no longer what you saw.

The model interprets prose as instruction. "Re-read using `read_file` before writing" is more actionable and avoids ambiguity in models that might interpret "re-read" as a re-attempt of the same tool. Trivial.

6. try { ... } catch { /* swallow */ } is too broad in edit.ts

```ts
try {
const preStats = fs.statSync(this.params.file_path);
...
} catch {
// stat failure (file missing, race, etc.) — defer to the
// existing edit path's error handling.
}
```

ENOENT (new-file path) is the legitimate fall-through. EACCES / EBUSY / etc. would also be silently swallowed here, deferred to the existing path. The existing path does report them, so behavior is preserved — but a narrowed if (isNodeError(err) && err.code !== 'ENOENT') rethrow would surface unexpected errors at the fence rather than later. Symmetric to how calculateEdit already handles this in the read branch.

7. staleWriteReason test asserts loosely

```ts
expect(reason).toMatch(/re-read/i); // mtime-stale case
expect(reason).toMatch(/has been modified/i); // size-stale case
```

The actual returned message contains both phrases regardless of mtime vs size drift, so these patterns don't distinguish the two cases the way the test names suggest. Either accept as-is (test verifies "non-empty informative message") or differentiate the message content for the two paths. Cosmetic.

8. getConfirmationDetails is not fenced

For Edit, getConfirmationDetails re-runs calculateEdit to render the confirmation diff to the user. If the file changed between the model's Read and the user's confirmation prompt, the diff shown is computed against post-mutation content — the user may see a diff that doesn't look like what the model "should" be doing. Then execute fences. So the user wastes a confirmation step on a doomed call.

A pre-flight staleWriteReason in getConfirmationDetails would catch this earlier. Defensible to leave it for follow-up; calling out for awareness.

Security / Correctness

• No injection / path-traversal surface introduced; the path used is already validated by the tool's existing param schema.
• The fence increases data-safety (refuses to silently destroy work). Failure mode is conservative (refuse with recoverable error).
• FileReadCacheDisabled continues to provide a documented bypass; no new bypass introduced.

Verdict

The fix is right, the scope is right, and the tests are mostly right. Two things worth addressing before merge:

1. Add the mirror integration test in edit.test.ts (#1).
2. Narrow the catch in edit.ts to ENOENT only (#6).

Everything else is opinion-tier — fine to land as-is or address in follow-up.

[wenshao]

Heads-up — this work overlaps with #3774 ("enforce prior read before Edit / WriteFile mutates a file"), which is currently open. Quick comparison so we can avoid duplicate review effort:

Case	This PR (#3840)	#3774
File never Read in this session, then Edit/Write	Falls through (cache returns unknown)	Refuses (EDIT_REQUIRES_PRIOR_READ)
File Read, then drifted (mtime/size) before Write	Refuses (FILE_STALE_BEFORE_WRITE)	Refuses (FILE_CHANGED_SINCE_READ)
Non-text payloads (binary / image / PDF / notebook)	Not addressed	Refuses with redirect to other tooling
Special files (FIFO / socket / device)	Not addressed	Refuses
fs.stat failed (EACCES / EBUSY / NFS)	Caught and swallowed	Distinct PRIOR_READ_VERIFICATION_FAILED
Diff size	+193 / -0	+1591 / -91

The drift case is functionally equivalent between the two PRs (both consult cache.check() === 'stale'); only the error code differs.

The bigger thing worth surfacing is that the unknown policy diverges by design:

• This PR is explicit that unknown should fall through ("first-time edits and brand new files still work").
• feat(core): enforce prior read before Edit / WriteFile mutates a file #3774 is explicit that any pre-existing file must have been Read first; only new-file creation is exempt.

That's a real design disagreement, not just two implementations of the same idea — worth a maintainer call before either lands. If #3774 merges first, this PR's drift fence becomes redundant and its unknown-falls-through behavior would conflict with the prior-read requirement. If this PR merges first, #3774's review will need to revisit the surface.

Not a request to close — your PR is smaller, more targeted, and easier to land on its own merits if the maintainers prefer the conservative scope. Just flagging the overlap so reviewers can pick one direction deliberately.

[ihubanov]

Added the symmetric Edit-side integration tests in cb1bbbe — fence path + first-time-edit fall-through, mirroring the WriteFile pair. 128/128 on the three affected files.

On the redundant fs.statSync: skipping for now. Two stat syscalls in an interactive tool isn't worth threading preStats through calculateEdit's signature. Can revisit if it ever shows up in profiling.

[ihubanov]

Yeah, saw #3774 after I posted. Different scope — theirs blocks any pre-existing file edit without a prior Read, mine only fences proven drift. Smaller patch, narrower claim.

If #3774 lands first drop this one — fence becomes redundant. Otherwise it stacks under the stricter rule.

[tanzhenxin]

Hey @ihubanov — thanks for putting this together, and the diagnosis is exactly right: the cache notices when files change, the write path didn't consult it, and you correctly identified the parallel-AgentTool / hook-touch / external-edit cases as the failure modes.

I'm going to close this, but I want to be clear about why.

This PR was authored on May 4 against a base that predates #3774, which merged on May 6. #3774 lands the same fence in priorReadEnforcement.ts (checkPriorRead) — same cache.check(stats) lookup, same recovery-message shape, but with one important asymmetry: checkPriorRead rejects on the 'unknown' state too, where this PR explicitly lets it fall through ("brand new files still work"). That fall-through is the gap #3774's review round 3 specifically closed — without it, a model can attempt Edits with candidate old_strings on an unread file and use NO_OCCURRENCE_FOUND / EXPECTED_OCCURRENCE_MISMATCH as a content oracle. So the two paths aren't quite equivalent: this one is strictly weaker on the security-critical case.

Two follow-ups have already landed on top of #3774 — #3810 (cache hygiene on history rewrites) and #3932 (accept partial reads) — so the area is settled enough that landing a parallel staleWriteReason / FILE_STALE_BEFORE_WRITE on the side would create two error types and two detection paths for the same drift, with callers keying on whichever happens to fire first. Plus the rebase against current main will conflict at the same insertion points in edit.ts and write-file.ts.

Thanks again for the work here.

[wenshao]

[Critical] The getFileReadCacheDisabled bypass path is completely untested in both edit.test.ts and write-file.test.ts. Production code gates the entire stale fence on !this.config.getFileReadCacheDisabled(), yet both test files hardcode getFileReadCacheDisabled: () => false with no test that sets it to true and verifies the fence is bypassed when a stale cache entry exists. If this check is accidentally removed or refactored incorrectly, no test will catch it — this is the only branch in the new feature with zero coverage.

---

[Suggestion] recordWrite is called unconditionally at edit.ts:457 and write-file.ts:278 even when getFileReadCacheDisabled() returns true. While Read tools respect the disabled flag and skip these cache entries, the entries accumulate uselessly, causing a minor memory leak in long-running sessions with the cache disabled.

---

[Suggestion] The "file exists but never touched" fall-through path is not covered in integration tests. The existing "proceeds normally when the model has never read the file" tests use files that don't exist on disk (exercising the ENOENT/catch path), rather than files that exist but have no cache entry (exercising the staleWriteReason returns null because check() returns unknown path). A regression here would break first-time edits/writes to existing on-disk files with no test to catch it.

— DeepSeek/deepseek-v4-pro via Qwen Code /review

[wenshao]

[Suggestion] const prior = entry.lastReadAt ?? entry.lastWriteAt always prefers lastReadAt even when lastWriteAt is more recent (writes typically happen after reads for the same file). The age shown to the model ((3600s ago)) can be misleading — it may say 300s when the model just wrote the file 5s ago.

Suggested change

	const prior = entry.lastReadAt ?? entry.lastWriteAt;
	const prior = Math.max(entry.lastReadAt ?? 0, entry.lastWriteAt ?? 0);

— DeepSeek/deepseek-v4-pro via Qwen Code /review

[wenshao]

[Suggestion] Edit's stale check unconditionally calls fs.statSync(this.params.file_path) even for new files (old_string=''). WriteFile guards this behind fileExists, but Edit does not — inconsistent, and wastes a syscall + try/catch on every new-file edit.

Suggested change

	if (!this.config.getFileReadCacheDisabled()) {
	const filePreExists = fs.existsSync(this.params.file_path);
	if (filePreExists && !this.config.getFileReadCacheDisabled()) {

— DeepSeek/deepseek-v4-pro via Qwen Code /review

[wenshao]

[Suggestion] WriteFile's stale-check statSync here and the subsequent readTextFile (for encoding detection, ~line 208) are separate operations. If the file is externally modified between these two calls, the fence reports "fresh" but the content read silently picks up the already-modified version, which then gets overwritten. Edit has old_string matching as a second line of defense; WriteFile has no such backstop. Consider moving the stale check to after the content read, using the read operation's stat results.

— DeepSeek/deepseek-v4-pro via Qwen Code /review