fix(filesystem): harden file replacement with EPERM fallback

src/filesystem/__tests__/lib.test.ts

@@ -308,6 +308,126 @@ describe('Lib Functions', () => {
 
         expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' });
       });
+
+      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;
+        epermError.code = 'EPERM';
+
+        mockFs.writeFile
+          .mockRejectedValueOnce(eexistError)   // First write fails (file exists)
+          .mockResolvedValueOnce(undefined)      // Temp file write succeeds
+          .mockResolvedValueOnce(undefined);     // Direct overwrite succeeds
+        mockFs.rename.mockRejectedValueOnce(epermError);  // Rename fails (locked)
+        mockFs.readFile.mockResolvedValueOnce(Buffer.from('new content'));
+        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.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).not.toHaveBeenCalled();
+        expect(mockFs.unlink).toHaveBeenCalledWith(
+          expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
+        );
+      });
+
+      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)
+          .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)
+
+        // Should NOT throw — the target file was written successfully
+        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',
+          { 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 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)
+          .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$/)
+        );
+      });
+
+      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$/)
+        );
+      });
     });
 
   });
@@ -553,6 +673,98 @@ describe('Lib Functions', () => {
         );
       });
 
+      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
+          .mockResolvedValueOnce('line1\nline2\nline3\n')
+          .mockResolvedValueOnce(Buffer.from('line1\nmodified line2\nline3\n'));
+        mockFs.writeFile
+          .mockResolvedValueOnce(undefined)
+          .mockResolvedValueOnce(undefined);
+        mockFs.rename.mockRejectedValueOnce(epermError);
+        mockFs.unlink.mockResolvedValueOnce(undefined);
+
+        const edits = [{ oldText: 'line2', newText: 'modified line2' }];
+        const result = await applyFileEdits('/test/file.txt', edits, false);
+
+        // 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.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).not.toHaveBeenCalled();
+        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('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
+          .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
+
+        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.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',
+          { force: true }
+        );
+      });
+
+      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');
 

src/filesystem/lib.ts

@@ -140,6 +140,57 @@ export async function validatePath(requestedPath: string): Promise<string> {
 }
 
 
+/**
+ * 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). 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:**
+ * - 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.
+ */
+async function cleanupTempFile(tempPath: string): Promise<void> {
+  try {
+    await fs.unlink(tempPath);
+  } catch {}
+}
+
+async function replaceFileFromTemp(tempPath: string, targetPath: string): Promise<void> {
+  try {
+    await fs.rename(tempPath, targetPath);
+  } catch (renameError) {
+    if ((renameError as NodeJS.ErrnoException).code === 'EPERM') {
+      try {
+        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 {
+      // For non-EPERM errors, clean up the temp file and re-throw
+      await cleanupTempFile(tempPath);
+      throw renameError;
+    }
+  }
+}
+
 // File Operations
 export async function getFileStats(filePath: string): Promise<FileInfo> {
   const stats = await fs.stat(filePath);
@@ -165,18 +216,15 @@ 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');
-        await fs.rename(tempPath, filePath);
-      } catch (renameError) {
-        try {
-          await fs.unlink(tempPath);
-        } catch {}
-        throw renameError;
+        await replaceFileFromTemp(tempPath, filePath);
+      } catch (tempWriteError) {
+        await cleanupTempFile(tempPath);
+        throw tempWriteError;
       }
     } else {
       throw error;
@@ -263,18 +311,15 @@ 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');
-      await fs.rename(tempPath, filePath);
-    } catch (error) {
-      try {
-        await fs.unlink(tempPath);
-      } catch {}
-      throw error;
+      await replaceFileFromTemp(tempPath, filePath);
+    } catch (writeError) {
+      await cleanupTempFile(tempPath);
+      throw writeError;
     }
   }