Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions packages/core/src/services/fileReadCache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,56 @@ describe('FileReadCache', () => {
});
});

describe('staleWriteReason', () => {
it('returns null for an unknown file (caller should not block)', () => {
const cache = new FileReadCache();
expect(cache.staleWriteReason('/x/foo.ts', makeStats())).toBeNull();
});

it('returns null when the file is fresh', () => {
const cache = new FileReadCache();
const stats = makeStats();
cache.recordRead('/x/foo.ts', stats, { full: true, cacheable: true });
expect(cache.staleWriteReason('/x/foo.ts', stats)).toBeNull();
});

it('returns a recovery message when mtime drifted after a Read', () => {
const cache = new FileReadCache();
cache.recordRead('/x/foo.ts', makeStats({ mtimeMs: 1000 }), {
full: true,
cacheable: true,
});
const reason = cache.staleWriteReason(
'/x/foo.ts',
makeStats({ mtimeMs: 2000 }),
);
expect(reason).not.toBeNull();
expect(reason).toContain('/x/foo.ts');
expect(reason).toMatch(/re-read/i);
});

it('returns a recovery message when size drifted after a Write', () => {
const cache = new FileReadCache();
cache.recordWrite('/x/foo.ts', makeStats({ size: 100 }));
const reason = cache.staleWriteReason(
'/x/foo.ts',
makeStats({ size: 250 }),
);
expect(reason).not.toBeNull();
expect(reason).toMatch(/has been modified/i);
});

it("does not block a follow-up write after the tool's own write", () => {
// Edit / WriteFile call recordWrite() with the post-write stats;
// the cache is then in sync with disk. Without this, a tool that
// performs two writes back-to-back would self-block.
const cache = new FileReadCache();
const stats = makeStats({ mtimeMs: 5000, size: 50 });
cache.recordWrite('/x/foo.ts', stats);
expect(cache.staleWriteReason('/x/foo.ts', stats)).toBeNull();
});
});

describe('recordRead', () => {
beforeEach(() => {
vi.useFakeTimers();
Expand Down
32 changes: 32 additions & 0 deletions packages/core/src/services/fileReadCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,38 @@ export class FileReadCache {
this.byInode.delete(FileReadCache.inodeKey(stats));
}

/**
* Used by Edit / WriteFile to refuse a write whose backing file has
* mutated out from under the model since its last tracked Read or
* write. Returns a non-null reason string when the caller should
* abort; null otherwise.
*
* The semantics intentionally match {@link check}, with one extra
* filter: an `unknown` entry never blocks. A file the model has not
* yet touched in this session is fair game to write — that covers
* brand new files and files the user named directly without ever
* Reading them. We only fence the case where the model *did* see a
* version of the file, then the file changed externally (another
* agent, a hook, the user's own editor), and the model is now about
* to overwrite based on stale knowledge.
*
* The returned message is what the model will see verbatim, so it is
* worded as a recovery instruction, not a stack trace.
*/
staleWriteReason(absPath: string, stats: Stats): string | null {
const status = this.check(stats);
if (status.state !== 'stale') return null;
const entry = status.entry;
const prior = entry.lastReadAt ?? entry.lastWriteAt;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

const ageSeconds =
prior === undefined ? undefined : Math.round((Date.now() - prior) / 1000);
const ageNote = ageSeconds === undefined ? '' : ` (${ageSeconds}s ago)`;
return (
`${absPath} has been modified since this session last touched it${ageNote}. ` +
`Re-read the file before writing — its contents are no longer what you saw.`
);
}

/** Drop every entry. Used by tests and on Config shutdown. */
clear(): void {
this.byInode.clear();
Expand Down
53 changes: 53 additions & 0 deletions packages/core/src/tools/edit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ describe('EditTool', () => {
getToolRegistry: () => ({}) as any, // Minimal mock for ToolRegistry
getDefaultFileEncoding: vi.fn().mockReturnValue('utf-8'),
getFileReadCache: () => fileReadCache,
getFileReadCacheDisabled: () => false,
} as unknown as Config;

// Reset mocks before each test
Expand Down Expand Up @@ -957,6 +958,58 @@ describe('EditTool', () => {
);
}
});

it('refuses to edit a file that changed since the last tracked Read', async () => {
// Symmetric with WriteFile's stale-fence test. The model recorded a
// Read of the file, then something else (another tool call running
// in parallel, a hook, or the user's editor) mutated it. Edit must
// not silently land its replacement on the new content.
const filePath = path.join(rootDir, 'edit-stale.txt');
fs.writeFileSync(filePath, 'original content');
const initialStats = fs.statSync(filePath);
fileReadCache.recordRead(filePath, initialStats, {
full: true,
cacheable: true,
});

// Mutate the file behind the cache's back, ensuring the mtime moves
// forward by at least one millisecond so the cache fingerprint
// differs from the new on-disk state.
const futureMs = initialStats.mtimeMs + 1000;
fs.writeFileSync(filePath, 'changed by someone else');
fs.utimesSync(filePath, futureMs / 1000, futureMs / 1000);

const invocation = tool.build({
file_path: filePath,
old_string: 'original',
new_string: 'attempted',
}) as ToolInvocation<EditToolParams, ToolResult>;
const abortSignal = new AbortController().signal;
const result = await invocation.execute(abortSignal);

expect(result.error?.type).toBe(ToolErrorType.FILE_STALE_BEFORE_WRITE);
// The on-disk content must be the external write, untouched by Edit.
expect(fs.readFileSync(filePath, 'utf8')).toBe('changed by someone else');
});

it('proceeds normally when the model has never read the file', async () => {
// First-time edits (brand new files via empty old_string, or files
// the user named without ever Reading) must continue to work —
// staleness is only a fence when the cache has a recorded prior
// interaction.
const filePath = path.join(rootDir, 'edit-fresh.txt');

const invocation = tool.build({
file_path: filePath,
old_string: '',
new_string: 'hello',
}) as ToolInvocation<EditToolParams, ToolResult>;
const abortSignal = new AbortController().signal;
const result = await invocation.execute(abortSignal);

expect(result.error).toBeUndefined();
expect(fs.readFileSync(filePath, 'utf8')).toBe('hello');
});
});

describe.skipIf(process.platform === 'win32')(
Expand Down
27 changes: 27 additions & 0 deletions packages/core/src/tools/edit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,33 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> {
* @returns Result of the edit operation
*/
async execute(signal: AbortSignal): Promise<ToolResult> {
// Refuse to edit a file that has changed since this session last
// saw it. The cache only blocks when there *is* a prior interaction
// (Read or Write) and the on-disk fingerprint no longer matches —
// first-time edits, brand-new files, and files the model has just
// had Read into context all fall through.
if (!this.config.getFileReadCacheDisabled()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

try {
const preStats = fs.statSync(this.params.file_path);
const reason = this.config
.getFileReadCache()
.staleWriteReason(this.params.file_path, preStats);
if (reason !== null) {
return {
llmContent: reason,
returnDisplay: reason,
error: {
message: reason,
type: ToolErrorType.FILE_STALE_BEFORE_WRITE,
},
};
}
} catch {
// stat failure (file missing, race, etc.) — defer to the
// existing edit path's error handling.
}
}

let editData: CalculatedEdit;
try {
editData = await this.calculateEdit(this.params);
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/tool-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export enum ToolErrorType {
READ_CONTENT_FAILURE = 'read_content_failure',
ATTEMPT_TO_CREATE_EXISTING_FILE = 'attempt_to_create_existing_file',
FILE_TOO_LARGE = 'file_too_large',
FILE_STALE_BEFORE_WRITE = 'file_stale_before_write',
PERMISSION_DENIED = 'permission_denied',
NO_SPACE_LEFT = 'no_space_left',
TARGET_IS_DIRECTORY = 'target_is_directory',
Expand Down
55 changes: 55 additions & 0 deletions packages/core/src/tools/write-file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const mockConfigInternal = {
}) as unknown as ToolRegistry,
getDefaultFileEncoding: () => 'utf-8',
getFileReadCache: () => fileReadCache,
getFileReadCacheDisabled: () => false,
};
const mockConfig = mockConfigInternal as unknown as Config;

Expand All @@ -85,6 +86,12 @@ describe('WriteFileTool', () => {

beforeEach(() => {
vi.clearAllMocks();
// The FileReadCache is module-scoped (line ~41) and would otherwise
// accumulate entries from prior tests, including stale (mtime, size)
// pairs at inodes the OS may reuse for newly-created fixture files.
// Clearing per-test mirrors what every real session start does
// (Config initializes a fresh cache).
fileReadCache.clear();
// Create a unique temporary directory for files created outside the root
tempDir = fs.mkdtempSync(
path.join(os.tmpdir(), 'write-file-test-external-'),
Expand Down Expand Up @@ -798,5 +805,53 @@ describe('WriteFileTool', () => {
fs.unlinkSync(filePath);
}
});

it('refuses to overwrite a file that changed since the last tracked Read', async () => {
// Reproduces the cross-agent / external-edit scenario: the model
// recorded a Read of foo.txt, then something else (another tool
// call running in parallel, a hook, or the user's own editor)
// mutated the file. WriteFile must not silently clobber.
fileReadCache.clear();
const filePath = path.join(rootDir, 'stale.txt');
fs.writeFileSync(filePath, 'original');
const initialStats = fs.statSync(filePath);
fileReadCache.recordRead(filePath, initialStats, {
full: true,
cacheable: true,
});

// Mutate the file behind the cache's back, ensuring the mtime moves
// forward by at least one millisecond so the cache fingerprint
// differs from the new on-disk state.
const futureMs = initialStats.mtimeMs + 1000;
fs.writeFileSync(filePath, 'changed by someone else');
fs.utimesSync(filePath, futureMs / 1000, futureMs / 1000);

const invocation = tool.build({
file_path: filePath,
content: 'attempted overwrite',
});
const result = await invocation.execute(abortSignal);

expect(result.error?.type).toBe(ToolErrorType.FILE_STALE_BEFORE_WRITE);
expect(fs.readFileSync(filePath, 'utf8')).toBe('changed by someone else');
});

it('proceeds normally when the model has never read the file', async () => {
// First-time writes (brand new files, files the user named without
// ever Reading) must continue to work — staleness is only a fence
// when the cache has a recorded prior interaction.
fileReadCache.clear();
const filePath = path.join(rootDir, 'new.txt');

const invocation = tool.build({
file_path: filePath,
content: 'hello',
});
const result = await invocation.execute(abortSignal);

expect(result.error).toBeUndefined();
expect(fs.readFileSync(filePath, 'utf8')).toBe('hello');
});
});
});
27 changes: 27 additions & 0 deletions packages/core/src/tools/write-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,33 @@ class WriteFileToolInvocation extends BaseToolInvocation<
let detectedEncoding: string | undefined;
let detectedLineEnding: LineEnding | undefined;
const dirName = path.dirname(file_path);

// Refuse to clobber a file that has changed since this session last
// saw it. The cache only blocks when there *is* a prior interaction
// (Read or Write) and the on-disk fingerprint no longer matches —
// brand-new files and files the model never read fall through.
if (fileExists && !this.config.getFileReadCacheDisabled()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

try {
const preStats = fs.statSync(file_path);
const reason = this.config
.getFileReadCache()
.staleWriteReason(file_path, preStats);
if (reason !== null) {
return {
llmContent: reason,
returnDisplay: reason,
error: {
message: reason,
type: ToolErrorType.FILE_STALE_BEFORE_WRITE,
},
};
}
} catch {
// stat raced with deletion or transient FS error — let the
// existing read/write path produce the canonical error.
}
}

if (fileExists) {
try {
const fileInfo = await this.config
Expand Down
Loading