From 926d4b6ee29475c600b934e699057836eb9f3d7a Mon Sep 17 00:00:00 2001 From: Arif Celebi Date: Mon, 16 Mar 2026 21:31:50 +0000 Subject: [PATCH 1/4] fix(filesystem): harden file replacement with EPERM fallback Extract replaceFileFromTemp helper that falls back to fs.cp when fs.rename fails with EPERM (Windows locked files). Temp file cleanup is best-effort so a successful write is never masked by a cleanup failure (e.g. antivirus holding the temp file). Changes: - Extract replaceFileFromTemp from inline rename logic in writeFileContent and applyFileEdits - Add EPERM fallback: rename -> cp + best-effort unlink - Document FILE_SHARE_DELETE limitation and non-atomic fallback in JSDoc - Add tests for EPERM fallback through both writeFileContent and applyFileEdits - Add tests for successful writes despite temp cleanup failure Fixes #3430 --- src/filesystem/__tests__/lib.test.ts | 110 +++++++++++++++++++++++++++ src/filesystem/lib.ts | 63 ++++++++++----- 2 files changed, 155 insertions(+), 18 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index f7e585af22..fc4a28eb17 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -308,6 +308,61 @@ describe('Lib Functions', () => { expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' }); }); + + it('falls back to fs.cp when fs.rename fails with EPERM', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) // First write fails (file exists) + .mockResolvedValueOnce(undefined); // Temp file write succeeds + mockFs.rename.mockRejectedValueOnce(epermError); // Rename fails (locked) + mockFs.cp.mockResolvedValueOnce(undefined); // cp succeeds + mockFs.unlink.mockResolvedValueOnce(undefined); // Temp cleanup succeeds + + await writeFileContent('/test/file.txt', 'new content'); + + expect(mockFs.rename).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt' + ); + expect(mockFs.cp).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt', + { force: true } + ); + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); + + it('succeeds when fs.cp works but temp file unlink fails', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyError.code = 'EBUSY'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) + .mockResolvedValueOnce(undefined); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.cp.mockResolvedValueOnce(undefined); + mockFs.unlink.mockRejectedValueOnce(ebusyError); // Temp cleanup fails (e.g. antivirus) + + // Should NOT throw — the target file was written successfully + await expect(writeFileContent('/test/file.txt', 'new content')) + .resolves.toBeUndefined(); + + expect(mockFs.cp).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt', + { force: true } + ); + }); }); }); @@ -553,6 +608,61 @@ describe('Lib Functions', () => { ); }); + it('falls back to fs.cp when fs.rename fails with EPERM during file edit', async () => { + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + + mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); + mockFs.writeFile.mockResolvedValue(undefined); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.cp.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + + const edits = [{ oldText: 'line2', newText: 'modified line2' }]; + const result = await applyFileEdits('/test/file.txt', edits, false); + + // Should have tried rename first, then fallen back to cp + unlink + expect(mockFs.rename).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt' + ); + expect(mockFs.cp).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt', + { force: true } + ); + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + // Edit should still produce a valid diff + expect(result).toContain('modified line2'); + }); + + it('succeeds when fs.cp works but temp file unlink fails during file edit', async () => { + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyError.code = 'EBUSY'; + + mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); + mockFs.writeFile.mockResolvedValue(undefined); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.cp.mockResolvedValueOnce(undefined); + mockFs.unlink.mockRejectedValueOnce(ebusyError); // Temp cleanup fails + + const edits = [{ oldText: 'line2', newText: 'modified line2' }]; + + // Should NOT throw — the target file was written successfully + const result = await applyFileEdits('/test/file.txt', edits, false); + + expect(result).toContain('modified line2'); + expect(mockFs.cp).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + '/test/file.txt', + { force: true } + ); + }); + it('handles CRLF line endings in file content', async () => { mockFs.readFile.mockResolvedValue('line1\r\nline2\r\nline3\r\n'); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 17e4654cd5..1ec1d028c8 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -140,6 +140,47 @@ export async function validatePath(requestedPath: string): Promise { } +/** + * Replace a target file with the contents of a temporary file. + * + * Uses `fs.rename` for an atomic swap when possible. On Windows, rename can + * fail with `EPERM` when the target is held open by another process (e.g. + * VS Code) *provided* that process opened the file with `FILE_SHARE_DELETE`. + * In that case the function falls back to `fs.cp` + best-effort `fs.unlink`. + * + * **Limitations:** + * - The `fs.cp` fallback is *not* atomic — there is a brief window between + * the internal `unlink(dest)` and `copyFile(src, dest)` performed by + * `fs.cp({ force: true })`. + * - The fallback only succeeds when the locking process uses + * `FILE_SHARE_DELETE`. Editors that lock without this flag will still + * produce an `EPERM` error. + * + * @param tempPath Path to the temporary file that contains the new content. + * @param targetPath Path to the destination file to be replaced. + */ +async function replaceFileFromTemp(tempPath: string, targetPath: string): Promise { + try { + await fs.rename(tempPath, targetPath); + } catch (renameError) { + if ((renameError as NodeJS.ErrnoException).code === 'EPERM') { + // Fallback: copy then best-effort cleanup + await fs.cp(tempPath, targetPath, { force: true }); + try { + await fs.unlink(tempPath); + } catch { + // Best-effort cleanup; target was already written successfully + } + } else { + // For non-EPERM errors, clean up the temp file and re-throw + try { + await fs.unlink(tempPath); + } catch {} + throw renameError; + } + } +} + // File Operations export async function getFileStats(filePath: string): Promise { const stats = await fs.stat(filePath); @@ -169,15 +210,8 @@ export async function writeFileContent(filePath: string, content: string): Promi // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; - try { - await fs.writeFile(tempPath, content, 'utf-8'); - await fs.rename(tempPath, filePath); - } catch (renameError) { - try { - await fs.unlink(tempPath); - } catch {} - throw renameError; - } + await fs.writeFile(tempPath, content, 'utf-8'); + await replaceFileFromTemp(tempPath, filePath); } else { throw error; } @@ -267,15 +301,8 @@ export async function applyFileEdits( // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; - try { - await fs.writeFile(tempPath, modifiedContent, 'utf-8'); - await fs.rename(tempPath, filePath); - } catch (error) { - try { - await fs.unlink(tempPath); - } catch {} - throw error; - } + await fs.writeFile(tempPath, modifiedContent, 'utf-8'); + await replaceFileFromTemp(tempPath, filePath); } return formattedDiff; From c22fad2c4b8e2b7fb079ec3905e569dd8fd5efd3 Mon Sep 17 00:00:00 2001 From: Arif Celebi Date: Tue, 17 Mar 2026 06:14:56 +0000 Subject: [PATCH 2/4] fix(fetch): refresh uv lockfile --- src/fetch/uv.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fetch/uv.lock b/src/fetch/uv.lock index c2159b229f..0690b49f76 100644 --- a/src/fetch/uv.lock +++ b/src/fetch/uv.lock @@ -547,7 +547,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "httpx", specifier = "<0.28" }, + { name = "httpx", specifier = ">=0.27" }, { name = "markdownify", specifier = ">=0.13.1" }, { name = "mcp", specifier = ">=1.1.3" }, { name = "protego", specifier = ">=0.3.1" }, From b014d43b21e80b956b5d3aeb973035b71d05996c Mon Sep 17 00:00:00 2001 From: Arif Celebi Date: Fri, 24 Apr 2026 00:07:50 +0100 Subject: [PATCH 3/4] fix(filesystem): clean up temp files on fallback failures --- src/filesystem/__tests__/lib.test.ts | 60 ++++++++++++++++++++++++++++ src/filesystem/lib.ts | 37 ++++++++++++----- 2 files changed, 86 insertions(+), 11 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index fc4a28eb17..f857a534b5 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -363,6 +363,48 @@ describe('Lib Functions', () => { { force: true } ); }); + + it('cleans up temp file and re-throws when fs.cp fails after EPERM', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const epermError = new Error('EPERM') as NodeJS.ErrnoException; + epermError.code = 'EPERM'; + const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; + enospcError.code = 'ENOSPC'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) + .mockResolvedValueOnce(undefined); + mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.cp.mockRejectedValueOnce(enospcError); + mockFs.unlink.mockResolvedValue(undefined); + + await expect(writeFileContent('/test/file.txt', 'new content')) + .rejects.toThrow('ENOSPC'); + + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); + + it('cleans up temp file and re-throws when temp write fails', async () => { + const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; + eexistError.code = 'EEXIST'; + const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; + enospcError.code = 'ENOSPC'; + + mockFs.writeFile + .mockRejectedValueOnce(eexistError) + .mockRejectedValueOnce(enospcError); + mockFs.unlink.mockResolvedValue(undefined); + + await expect(writeFileContent('/test/file.txt', 'new content')) + .rejects.toThrow('ENOSPC'); + + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); }); }); @@ -663,6 +705,24 @@ describe('Lib Functions', () => { ); }); + it('cleans up temp file and re-throws when temp write fails during file edit', async () => { + const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; + enospcError.code = 'ENOSPC'; + + mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); + mockFs.writeFile.mockRejectedValueOnce(enospcError); + mockFs.unlink.mockResolvedValue(undefined); + + const edits = [{ oldText: 'line2', newText: 'modified line2' }]; + + await expect(applyFileEdits('/test/file.txt', edits, false)) + .rejects.toThrow('ENOSPC'); + + expect(mockFs.unlink).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + }); + it('handles CRLF line endings in file content', async () => { mockFs.readFile.mockResolvedValue('line1\r\nline2\r\nline3\r\n'); diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 1ec1d028c8..9dec9e0435 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -159,23 +159,28 @@ export async function validatePath(requestedPath: string): Promise { * @param tempPath Path to the temporary file that contains the new content. * @param targetPath Path to the destination file to be replaced. */ +async function cleanupTempFile(tempPath: string): Promise { + try { + await fs.unlink(tempPath); + } catch {} +} + async function replaceFileFromTemp(tempPath: string, targetPath: string): Promise { try { await fs.rename(tempPath, targetPath); } catch (renameError) { if ((renameError as NodeJS.ErrnoException).code === 'EPERM') { // Fallback: copy then best-effort cleanup - await fs.cp(tempPath, targetPath, { force: true }); try { - await fs.unlink(tempPath); - } catch { - // Best-effort cleanup; target was already written successfully + await fs.cp(tempPath, targetPath, { force: true }); + } catch (copyError) { + await cleanupTempFile(tempPath); + throw copyError; } + await cleanupTempFile(tempPath); } else { // For non-EPERM errors, clean up the temp file and re-throw - try { - await fs.unlink(tempPath); - } catch {} + await cleanupTempFile(tempPath); throw renameError; } } @@ -210,8 +215,13 @@ export async function writeFileContent(filePath: string, content: string): Promi // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; - await fs.writeFile(tempPath, content, 'utf-8'); - await replaceFileFromTemp(tempPath, filePath); + try { + await fs.writeFile(tempPath, content, 'utf-8'); + await replaceFileFromTemp(tempPath, filePath); + } catch (tempWriteError) { + await cleanupTempFile(tempPath); + throw tempWriteError; + } } else { throw error; } @@ -301,8 +311,13 @@ export async function applyFileEdits( // could be created between validation and write. Rename operations // replace the target file atomically and don't follow symlinks. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; - await fs.writeFile(tempPath, modifiedContent, 'utf-8'); - await replaceFileFromTemp(tempPath, filePath); + try { + await fs.writeFile(tempPath, modifiedContent, 'utf-8'); + await replaceFileFromTemp(tempPath, filePath); + } catch (writeError) { + await cleanupTempFile(tempPath); + throw writeError; + } } return formattedDiff; From f78ab42049a1305fc8440c0959c62dcb46e64dcd Mon Sep 17 00:00:00 2001 From: hxaxd Date: Thu, 16 Jul 2026 22:02:36 +0800 Subject: [PATCH 4/4] fix(filesystem): prefer direct overwrite before copy fallback --- src/filesystem/__tests__/lib.test.ts | 86 +++++++++++++++++++++------- src/filesystem/lib.ts | 41 +++++++------ 2 files changed, 86 insertions(+), 41 deletions(-) diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index f857a534b5..3b9cfcd203 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -309,7 +309,7 @@ describe('Lib Functions', () => { expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' }); }); - it('falls back to fs.cp when fs.rename fails with EPERM', async () => { + it('uses direct overwrite before the symlink-unsafe fs.cp fallback', async () => { const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; eexistError.code = 'EEXIST'; const epermError = new Error('EPERM') as NodeJS.ErrnoException; @@ -317,9 +317,10 @@ describe('Lib Functions', () => { mockFs.writeFile .mockRejectedValueOnce(eexistError) // First write fails (file exists) - .mockResolvedValueOnce(undefined); // Temp file write succeeds + .mockResolvedValueOnce(undefined) // Temp file write succeeds + .mockResolvedValueOnce(undefined); // Direct overwrite succeeds mockFs.rename.mockRejectedValueOnce(epermError); // Rename fails (locked) - mockFs.cp.mockResolvedValueOnce(undefined); // cp succeeds + mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content')); mockFs.unlink.mockResolvedValueOnce(undefined); // Temp cleanup succeeds await writeFileContent('/test/file.txt', 'new content'); @@ -328,28 +329,35 @@ describe('Lib Functions', () => { expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt' ); - expect(mockFs.cp).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + expect(mockFs.readFile).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( '/test/file.txt', - { force: true } + Buffer.from('new content') ); + expect(mockFs.cp).not.toHaveBeenCalled(); expect(mockFs.unlink).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), ); }); - it('succeeds when fs.cp works but temp file unlink fails', async () => { + it('falls back to fs.cp when direct overwrite fails', async () => { const eexistError = new Error('EEXIST') as NodeJS.ErrnoException; eexistError.code = 'EEXIST'; const epermError = new Error('EPERM') as NodeJS.ErrnoException; epermError.code = 'EPERM'; + const ebusyWriteError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyWriteError.code = 'EBUSY'; const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; ebusyError.code = 'EBUSY'; mockFs.writeFile .mockRejectedValueOnce(eexistError) - .mockResolvedValueOnce(undefined); + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(ebusyWriteError); mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content')); mockFs.cp.mockResolvedValueOnce(undefined); mockFs.unlink.mockRejectedValueOnce(ebusyError); // Temp cleanup fails (e.g. antivirus) @@ -357,6 +365,13 @@ describe('Lib Functions', () => { await expect(writeFileContent('/test/file.txt', 'new content')) .resolves.toBeUndefined(); + expect(mockFs.readFile).toHaveBeenCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('new content') + ); expect(mockFs.cp).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt', @@ -369,19 +384,27 @@ describe('Lib Functions', () => { eexistError.code = 'EEXIST'; const epermError = new Error('EPERM') as NodeJS.ErrnoException; epermError.code = 'EPERM'; + const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyError.code = 'EBUSY'; const enospcError = new Error('ENOSPC') as NodeJS.ErrnoException; enospcError.code = 'ENOSPC'; mockFs.writeFile .mockRejectedValueOnce(eexistError) - .mockResolvedValueOnce(undefined); + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(ebusyError); mockFs.rename.mockRejectedValueOnce(epermError); + mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content')); mockFs.cp.mockRejectedValueOnce(enospcError); mockFs.unlink.mockResolvedValue(undefined); await expect(writeFileContent('/test/file.txt', 'new content')) .rejects.toThrow('ENOSPC'); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('new content') + ); expect(mockFs.unlink).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) ); @@ -650,44 +673,56 @@ describe('Lib Functions', () => { ); }); - it('falls back to fs.cp when fs.rename fails with EPERM during file edit', async () => { + it('uses direct overwrite before the symlink-unsafe fs.cp fallback during file edit', async () => { const epermError = new Error('EPERM') as NodeJS.ErrnoException; epermError.code = 'EPERM'; - mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); - mockFs.writeFile.mockResolvedValue(undefined); + mockFs.readFile + .mockResolvedValueOnce('line1\nline2\nline3\n') + .mockResolvedValueOnce(Buffer.from('line1\nmodified line2\nline3\n')); + mockFs.writeFile + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(undefined); mockFs.rename.mockRejectedValueOnce(epermError); - mockFs.cp.mockResolvedValueOnce(undefined); mockFs.unlink.mockResolvedValueOnce(undefined); const edits = [{ oldText: 'line2', newText: 'modified line2' }]; const result = await applyFileEdits('/test/file.txt', edits, false); - // Should have tried rename first, then fallen back to cp + unlink + // Should try rename, then overwrite in place without invoking fs.cp. expect(mockFs.rename).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt' ); - expect(mockFs.cp).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), + expect(mockFs.readFile).toHaveBeenLastCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( '/test/file.txt', - { force: true } + Buffer.from('line1\nmodified line2\nline3\n') ); + expect(mockFs.cp).not.toHaveBeenCalled(); expect(mockFs.unlink).toHaveBeenCalledWith( - expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), ); // Edit should still produce a valid diff expect(result).toContain('modified line2'); }); - it('succeeds when fs.cp works but temp file unlink fails during file edit', async () => { + it('falls back to fs.cp when direct overwrite fails during file edit', async () => { const epermError = new Error('EPERM') as NodeJS.ErrnoException; epermError.code = 'EPERM'; + const ebusyWriteError = new Error('EBUSY') as NodeJS.ErrnoException; + ebusyWriteError.code = 'EBUSY'; const ebusyError = new Error('EBUSY') as NodeJS.ErrnoException; ebusyError.code = 'EBUSY'; - mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n'); - mockFs.writeFile.mockResolvedValue(undefined); + mockFs.readFile + .mockResolvedValueOnce('line1\nline2\nline3\n') + .mockResolvedValueOnce(Buffer.from('line1\nmodified line2\nline3\n')); + mockFs.writeFile + .mockResolvedValueOnce(undefined) + .mockRejectedValueOnce(ebusyWriteError); mockFs.rename.mockRejectedValueOnce(epermError); mockFs.cp.mockResolvedValueOnce(undefined); mockFs.unlink.mockRejectedValueOnce(ebusyError); // Temp cleanup fails @@ -698,6 +733,13 @@ describe('Lib Functions', () => { const result = await applyFileEdits('/test/file.txt', edits, false); expect(result).toContain('modified line2'); + expect(mockFs.readFile).toHaveBeenLastCalledWith( + expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/) + ); + expect(mockFs.writeFile).toHaveBeenLastCalledWith( + '/test/file.txt', + Buffer.from('line1\nmodified line2\nline3\n') + ); expect(mockFs.cp).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt', diff --git a/src/filesystem/lib.ts b/src/filesystem/lib.ts index 9dec9e0435..97aadac734 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -145,16 +145,17 @@ export async function validatePath(requestedPath: string): Promise { * * Uses `fs.rename` for an atomic swap when possible. On Windows, rename can * fail with `EPERM` when the target is held open by another process (e.g. - * VS Code) *provided* that process opened the file with `FILE_SHARE_DELETE`. - * In that case the function falls back to `fs.cp` + best-effort `fs.unlink`. + * VS Code). In that case the function first overwrites the target in place, + * avoiding the destination unlink performed by `fs.cp({ force: true })`. + * If direct overwrite is also blocked, it falls back to `fs.cp`. * * **Limitations:** - * - The `fs.cp` fallback is *not* atomic — there is a brief window between - * the internal `unlink(dest)` and `copyFile(src, dest)` performed by - * `fs.cp({ force: true })`. - * - The fallback only succeeds when the locking process uses - * `FILE_SHARE_DELETE`. Editors that lock without this flag will still - * produce an `EPERM` error. + * - Direct overwrite requires the locking process to share write access. + * - The `fs.cp` fallback requires delete sharing and is *not* atomic: there + * is a brief symlink race window between its internal `unlink(dest)` and + * `copyFile(src, dest)`. + * - Editors that grant neither write nor delete sharing still produce an + * error. * * @param tempPath Path to the temporary file that contains the new content. * @param targetPath Path to the destination file to be replaced. @@ -170,12 +171,16 @@ async function replaceFileFromTemp(tempPath: string, targetPath: string): Promis await fs.rename(tempPath, targetPath); } catch (renameError) { if ((renameError as NodeJS.ErrnoException).code === 'EPERM') { - // Fallback: copy then best-effort cleanup try { - await fs.cp(tempPath, targetPath, { force: true }); - } catch (copyError) { - await cleanupTempFile(tempPath); - throw copyError; + const content = await fs.readFile(tempPath); + await fs.writeFile(targetPath, content); + } catch { + try { + await fs.cp(tempPath, targetPath, { force: true }); + } catch (copyError) { + await cleanupTempFile(tempPath); + throw copyError; + } } await cleanupTempFile(tempPath); } else { @@ -211,9 +216,8 @@ export async function writeFileContent(filePath: string, content: string): Promi await fs.writeFile(filePath, content, { encoding: "utf-8", flag: 'wx' }); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'EEXIST') { - // Security: Use atomic rename to prevent race conditions where symlinks - // could be created between validation and write. Rename operations - // replace the target file atomically and don't follow symlinks. + // Prefer atomic rename; replaceFileFromTemp documents the Windows + // locked-file fallbacks and their security tradeoffs. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, content, 'utf-8'); @@ -307,9 +311,8 @@ export async function applyFileEdits( const formattedDiff = `${'`'.repeat(numBackticks)}diff\n${diff}${'`'.repeat(numBackticks)}\n\n`; if (!dryRun) { - // Security: Use atomic rename to prevent race conditions where symlinks - // could be created between validation and write. Rename operations - // replace the target file atomically and don't follow symlinks. + // Prefer atomic rename; replaceFileFromTemp documents the Windows + // locked-file fallbacks and their security tradeoffs. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, modifiedContent, 'utf-8');