From b0a45d38c3189b8fca8b3b6f97968590759607c3 Mon Sep 17 00:00:00 2001 From: vyagh Date: Fri, 26 Dec 2025 10:21:58 +0530 Subject: [PATCH] fix: detect race condition when file is modified during user confirmation (#24) - Add EDIT_FILE_MODIFIED_DURING_WAIT error type - Capture file mtime when read during shouldConfirmExecute() - Validate mtime before write in execute() - Add test case for race condition detection Closes #24 --- packages/core/src/tools/edit.test.ts | 27 ++++++++++++ packages/core/src/tools/edit.ts | 60 +++++++++++++++++++++++---- packages/core/src/tools/tool-error.ts | 1 + 3 files changed, 79 insertions(+), 9 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 430b163..e4b0a62 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -589,6 +589,33 @@ describe('EditTool', () => { const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); }); + + it('should return FILE_MODIFIED_DURING_WAIT error if file changed between confirmation and execute', async () => { + fs.writeFileSync(filePath, 'original content', 'utf8'); + + const params: EditToolParams = { + file_path: filePath, + old_string: 'original', + new_string: 'modified', + }; + const invocation = tool.build(params); + + // Simulate confirmation flow - this captures mtime + await invocation.shouldConfirmExecute(new AbortController().signal); + + // Simulate external modification after confirmation + // Use utimesSync to guarantee a different mtime (works on all filesystems) + fs.writeFileSync(filePath, 'externally modified content', 'utf8'); + const futureTime = new Date(Date.now() + 2000); + fs.utimesSync(filePath, futureTime, futureTime); + + // Execute should detect the race condition + const result = await invocation.execute(new AbortController().signal); + expect(result.error?.type).toBe( + ToolErrorType.EDIT_FILE_MODIFIED_DURING_WAIT, + ); + expect(result.llmContent).toContain('modified externally'); + }); }); describe('getDescription', () => { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index e9d489f..234c45b 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -101,10 +101,16 @@ interface CalculatedEdit { } class EditToolInvocation implements ToolInvocation { + /** + * Stores the file's mtime at the time it was read for confirmation. + * Used to detect if the file was modified externally before we write. + */ + private readMtimeMs?: number; + constructor( private readonly config: Config, public params: EditToolParams, - ) {} + ) { } toolLocations(): ToolLocation[] { return [{ path: this.params.file_path }]; @@ -195,11 +201,11 @@ class EditToolInvocation implements ToolInvocation { const newContent = !error ? applyReplacement( - currentContent, - finalOldString, - finalNewString, - isNewFile, - ) + currentContent, + finalOldString, + finalNewString, + isNewFile, + ) : (currentContent ?? ''); if (!error && fileExists && currentContent === newContent) { @@ -247,6 +253,16 @@ class EditToolInvocation implements ToolInvocation { return false; } + // Capture file mtime BEFORE calculateEdit for race condition detection + // This must happen before reading to avoid TOCTOU gap + let capturedMtimeMs: number | undefined; + try { + const stats = await fs.promises.stat(this.params.file_path); + capturedMtimeMs = stats.mtimeMs; + } catch { + // File doesn't exist yet - will be caught in calculateEdit + } + let editData: CalculatedEdit; try { editData = await this.calculateEdit(this.params); @@ -261,6 +277,11 @@ class EditToolInvocation implements ToolInvocation { return false; } + // Only store mtime if file existed (not a new file creation) + if (!editData.isNewFile && capturedMtimeMs !== undefined) { + this.readMtimeMs = capturedMtimeMs; + } + const fileName = path.basename(this.params.file_path); const fileDiff = Diff.createPatch( fileName, @@ -273,7 +294,7 @@ class EditToolInvocation implements ToolInvocation { const ideClient = this.config.getIdeClient(); const ideConfirmation = this.config.getIdeMode() && - ideClient?.getConnectionStatus().status === IDEConnectionStatus.Connected + ideClient?.getConnectionStatus().status === IDEConnectionStatus.Connected ? ideClient.openDiff(this.params.file_path, editData.newContent) : undefined; @@ -297,6 +318,8 @@ class EditToolInvocation implements ToolInvocation { // for info on a possible race condition where the file is modified on disk while being edited. this.params.old_string = editData.currentContent ?? ''; this.params.new_string = result.content; + // Clear mtime check since IDE modified params - the file state has intentionally changed + this.readMtimeMs = undefined; } } }, @@ -333,6 +356,26 @@ class EditToolInvocation implements ToolInvocation { * @returns Result of the edit operation */ async execute(_signal: AbortSignal): Promise { + // Check for race condition FIRST: file modified since we read it for confirmation + if (this.readMtimeMs !== undefined) { + try { + const currentStats = await fs.promises.stat(this.params.file_path); + if (currentStats.mtimeMs !== this.readMtimeMs) { + const errorMsg = `File was modified externally since it was read. Use ${ReadFileTool.Name} to get current content and retry the edit.`; + return { + llmContent: errorMsg, + returnDisplay: `Error: ${errorMsg}`, + error: { + message: errorMsg, + type: ToolErrorType.EDIT_FILE_MODIFIED_DURING_WAIT, + }, + }; + } + } catch { + // If we can't stat the file now, let calculateEdit handle the error + } + } + let editData: CalculatedEdit; try { editData = await this.calculateEdit(this.params); @@ -462,8 +505,7 @@ class EditToolInvocation implements ToolInvocation { */ export class EditTool extends BaseDeclarativeTool - implements ModifiableDeclarativeTool -{ + implements ModifiableDeclarativeTool { static readonly Name = ToolNames.EDIT; constructor(private readonly config: Config) { super( diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index f65800b..dad569f 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -33,6 +33,7 @@ export enum ToolErrorType { EDIT_NO_OCCURRENCE_FOUND = 'edit_no_occurrence_found', EDIT_EXPECTED_OCCURRENCE_MISMATCH = 'edit_expected_occurrence_mismatch', EDIT_NO_CHANGE = 'edit_no_change', + EDIT_FILE_MODIFIED_DURING_WAIT = 'edit_file_modified_during_wait', // Glob-specific Errors GLOB_EXECUTION_ERROR = 'glob_execution_error',