diff --git a/src/filesystem/__tests__/lib.test.ts b/src/filesystem/__tests__/lib.test.ts index bfe8987bfd..28356af502 100644 --- a/src/filesystem/__tests__/lib.test.ts +++ b/src/filesystem/__tests__/lib.test.ts @@ -399,18 +399,19 @@ describe('Lib Functions', () => { { oldText: 'line2', newText: 'modified line2' } ]; - mockFs.rename.mockResolvedValueOnce(undefined); - + mockFs.copyFile.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + const result = await applyFileEdits('/test/file.txt', edits, false); - + expect(result).toContain('modified line2'); - // Should write to temporary file then rename + // Should write to temporary file then copy expect(mockFs.writeFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), 'line1\nmodified line2\nline3\n', 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( + expect(mockFs.copyFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt' ); @@ -432,17 +433,18 @@ describe('Lib Functions', () => { { oldText: 'line1', newText: 'first line' }, { oldText: 'line3', newText: 'third line' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + + mockFs.copyFile.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + await applyFileEdits('/test/file.txt', edits, false); - + expect(mockFs.writeFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), 'first line\nline2\nthird line\n', 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( + expect(mockFs.copyFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt' ); @@ -450,21 +452,22 @@ describe('Lib Functions', () => { it('handles whitespace-flexible matching', async () => { mockFs.readFile.mockResolvedValue(' line1\n line2\n line3\n'); - + const edits = [ { oldText: 'line2', newText: 'modified line2' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + + mockFs.copyFile.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + await applyFileEdits('/test/file.txt', edits, false); - + expect(mockFs.writeFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), ' line1\n modified line2\n line3\n', 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( + expect(mockFs.copyFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), '/test/file.txt' ); @@ -481,24 +484,25 @@ describe('Lib Functions', () => { it('handles complex multi-line edits with indentation', async () => { mockFs.readFile.mockResolvedValue('function test() {\n console.log("hello");\n return true;\n}'); - + const edits = [ - { - oldText: ' console.log("hello");\n return true;', - newText: ' console.log("world");\n console.log("test");\n return false;' + { + oldText: ' console.log("hello");\n return true;', + newText: ' console.log("world");\n console.log("test");\n return false;' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + + mockFs.copyFile.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + await applyFileEdits('/test/file.js', edits, false); - + expect(mockFs.writeFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), 'function test() {\n console.log("world");\n console.log("test");\n return false;\n}', 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( + expect(mockFs.copyFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), '/test/file.js' ); @@ -506,24 +510,25 @@ describe('Lib Functions', () => { it('handles edits with different indentation patterns', async () => { mockFs.readFile.mockResolvedValue(' if (condition) {\n doSomething();\n }'); - + const edits = [ - { - oldText: 'doSomething();', - newText: 'doSomethingElse();\n doAnotherThing();' + { + oldText: 'doSomething();', + newText: 'doSomethingElse();\n doAnotherThing();' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + + mockFs.copyFile.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + await applyFileEdits('/test/file.js', edits, false); - + expect(mockFs.writeFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), ' if (condition) {\n doSomethingElse();\n doAnotherThing();\n }', 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( + expect(mockFs.copyFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.js\.[a-f0-9]+\.tmp$/), '/test/file.js' ); @@ -531,21 +536,22 @@ describe('Lib Functions', () => { it('handles CRLF line endings in file content', async () => { mockFs.readFile.mockResolvedValue('line1\r\nline2\r\nline3\r\n'); - + const edits = [ { oldText: 'line2', newText: 'modified line2' } ]; - - mockFs.rename.mockResolvedValueOnce(undefined); - + + mockFs.copyFile.mockResolvedValueOnce(undefined); + mockFs.unlink.mockResolvedValueOnce(undefined); + await applyFileEdits('/test/file.txt', edits, false); - + expect(mockFs.writeFile).toHaveBeenCalledWith( expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/), 'line1\nmodified line2\nline3\n', 'utf-8' ); - expect(mockFs.rename).toHaveBeenCalledWith( + expect(mockFs.copyFile).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 240ca0d476..0c699355b8 100644 --- a/src/filesystem/lib.ts +++ b/src/filesystem/lib.ts @@ -142,18 +142,17 @@ 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. + // Security: Write to temp file first, then copy to target. + // This prevents partial writes if an error occurs mid-operation. + // The temp file is always cleaned up in the finally block. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, content, 'utf-8'); - await fs.rename(tempPath, filePath); - } catch (renameError) { + await fs.copyFile(tempPath, filePath); + } finally { try { await fs.unlink(tempPath); } catch {} - throw renameError; } } else { throw error; @@ -240,18 +239,17 @@ 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. + // Security: Write to temp file first, then copy to target. + // This prevents partial writes if an error occurs mid-operation. + // The temp file is always cleaned up in the finally block. const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`; try { await fs.writeFile(tempPath, modifiedContent, 'utf-8'); - await fs.rename(tempPath, filePath); - } catch (error) { + await fs.copyFile(tempPath, filePath); + } finally { try { await fs.unlink(tempPath); } catch {} - throw error; } }