fix(filesystem): preserve file permissions during write and edit operations (#4115)

* fix(filesystem): preserve file permissions during write and edit operations

The atomic write pattern (write temp file + rename) replaces the original
inode, causing the new file to have default 0644 permissions regardless of
what the original file had. This breaks executable scripts and other files
with non-default permissions.

Fix: capture stat.mode before writing and restore it with chmod after rename.

Fixes both writeFileContent() and applyFileEdits().

* fix(filesystem): mask chmod mode and keep chmod failure from failing the write

Pass origStats.mode & 0o777 to chmod instead of the full st_mode. Move
the chmod out of the try whose catch unlinks the temp file and rethrows,
since rename has already succeeded by then. Add a test that an EPERM
from chmod does not reject the write.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Wo28CHPXyM3DHoLWvKnCK

---------

Co-authored-by: Dustin Rudiger <drudiger@apple.com>
Co-authored-by: olaservo <olahungerford@gmail.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

src/filesystem/__tests__/lib.test.ts

@@ -310,6 +310,33 @@ describe('Lib Functions', () => {
         
         expect(mockFs.writeFile).toHaveBeenCalledWith('/test/file.txt', 'new content', { encoding: "utf-8", flag: 'wx' });
       });
+
+      it('preserves file permissions when overwriting existing file', async () => {
+        // First writeFile call with 'wx' flag fails because file exists
+        mockFs.writeFile.mockRejectedValueOnce(Object.assign(new Error('EEXIST'), { code: 'EEXIST' }));
+        // stat returns executable permissions
+        mockFs.stat.mockResolvedValueOnce({ mode: 0o100755 });
+        // Second writeFile (to temp) succeeds
+        mockFs.writeFile.mockResolvedValueOnce(undefined);
+        mockFs.rename.mockResolvedValueOnce(undefined);
+        mockFs.chmod.mockResolvedValueOnce(undefined);
+
+        await writeFileContent('/test/script.sh', 'new content');
+
+        expect(mockFs.stat).toHaveBeenCalledWith('/test/script.sh');
+        expect(mockFs.chmod).toHaveBeenCalledWith('/test/script.sh', 0o755);
+      });
+
+      it('does not fail the write when chmod fails', async () => {
+        mockFs.writeFile.mockRejectedValueOnce(Object.assign(new Error('EEXIST'), { code: 'EEXIST' }));
+        mockFs.stat.mockResolvedValueOnce({ mode: 0o100755 });
+        mockFs.writeFile.mockResolvedValueOnce(undefined);
+        mockFs.rename.mockResolvedValueOnce(undefined);
+        mockFs.chmod.mockRejectedValueOnce(Object.assign(new Error('EPERM'), { code: 'EPERM' }));
+
+        await expect(writeFileContent('/test/script.sh', 'new content')).resolves.toBeUndefined();
+        expect(mockFs.unlink).not.toHaveBeenCalled();
+      });
     });
 
   });
@@ -418,6 +445,8 @@ describe('Lib Functions', () => {
       beforeEach(() => {
         mockFs.readFile.mockResolvedValue('line1\nline2\nline3\n');
         mockFs.writeFile.mockResolvedValue(undefined);
+        mockFs.stat.mockResolvedValue({ mode: 0o100644 });
+        mockFs.chmod.mockResolvedValue(undefined);
       });
 
       it('applies simple text replacement', async () => {
@@ -512,6 +541,30 @@ describe('Lib Functions', () => {
         );
       });
 
+      it('preserves file permissions after applying edits', async () => {
+        mockFs.stat.mockResolvedValue({ mode: 0o100755 });
+        const edits = [
+          { oldText: 'line2', newText: 'modified line2' }
+        ];
+        
+        mockFs.rename.mockResolvedValueOnce(undefined);
+        
+        await applyFileEdits('/test/script.sh', edits, false);
+        
+        expect(mockFs.stat).toHaveBeenCalledWith('/test/script.sh');
+        expect(mockFs.chmod).toHaveBeenCalledWith('/test/script.sh', 0o755);
+      });
+
+      it('does not restore permissions in dry run mode', async () => {
+        const edits = [
+          { oldText: 'line2', newText: 'modified line2' }
+        ];
+        
+        await applyFileEdits('/test/file.txt', edits, true);
+        
+        expect(mockFs.chmod).not.toHaveBeenCalled();
+      });
+
       it('throws error for non-matching edits', async () => {
         const edits = [
           { oldText: 'nonexistent line', newText: 'replacement' }

src/filesystem/lib.ts

@@ -168,6 +168,7 @@ export async function writeFileContent(filePath: string, content: string): Promi
       // 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.
+      const origStats = await fs.stat(filePath);
       const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`;
       try {
         await fs.writeFile(tempPath, content, 'utf-8');
@@ -178,6 +179,13 @@ export async function writeFileContent(filePath: string, content: string): Promi
         } catch {}
         throw renameError;
       }
+      // Restore original permission bits since the atomic rename replaces the
+      // inode and the temp file has default (0644) permissions. Mask off the
+      // file-type bits; POSIX leaves them unspecified for chmod. A chmod
+      // failure must not fail the write, which has already succeeded.
+      try {
+        await fs.chmod(filePath, origStats.mode & 0o777);
+      } catch {}
     } else {
       throw error;
     }
@@ -266,6 +274,7 @@ export async function applyFileEdits(
     // 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.
+    const origStats = await fs.stat(filePath);
     const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`;
     try {
       await fs.writeFile(tempPath, modifiedContent, 'utf-8');
@@ -276,6 +285,13 @@ export async function applyFileEdits(
       } catch {}
       throw error;
     }
+    // Restore original permission bits since the atomic rename replaces the
+    // inode and the temp file has default (0644) permissions. Mask off the
+    // file-type bits; POSIX leaves them unspecified for chmod. A chmod
+    // failure must not fail the write, which has already succeeded.
+    try {
+      await fs.chmod(filePath, origStats.mode & 0o777);
+    } catch {}
   }
 
   return formattedDiff;