fix(filesystem): address review feedback on append/write_or_update tools

- Use fs.appendFile directly instead of read-modify-write (O(1) memory)
- Eliminate TOCTOU race between fs.access and fs.readFile
- Replace unreliable birthtime===mtime heuristic with pre-op existence check
- Have writeOrUpdateFileContent delegate to appendFileContent (no duplication)
- Consolidate three identical Zod schemas into one PathContentArgsSchema

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: agn-7

src/filesystem/__tests__/lib.test.ts

@@ -316,48 +316,39 @@ describe('Lib Functions', () => {
       it('throws error if file does not exist', async () => {
         const error = new Error('ENOENT');
         (error as any).code = 'ENOENT';
-        mockFs.access.mockRejectedValue(error);
+        mockFs.appendFile.mockRejectedValue(error);
 
         await expect(appendFileContent('/test/nonexistent.txt', 'new content'))
           .rejects.toThrow('File does not exist');
       });
 
       it('appends content to existing file', async () => {
-        mockFs.access.mockResolvedValue(undefined);
-        mockFs.readFile.mockResolvedValue('existing content' as any);
-        mockFs.writeFile.mockResolvedValue(undefined);
-        mockFs.rename.mockResolvedValue(undefined);
+        mockFs.appendFile.mockResolvedValue(undefined);
 
         await appendFileContent('/test/file.txt', '\nnew content');
 
-        expect(mockFs.readFile).toHaveBeenCalledWith('/test/file.txt', 'utf-8');
-        expect(mockFs.writeFile).toHaveBeenCalledWith(
-          expect.stringContaining('.tmp'),
-          'existing content\nnew content',
-          'utf-8'
+        expect(mockFs.appendFile).toHaveBeenCalledWith(
+          '/test/file.txt',
+          '\nnew content',
+          { encoding: 'utf-8', flag: 'a' }
         );
-        expect(mockFs.rename).toHaveBeenCalled();
       });
 
-      it('handles write errors and cleans up temp file', async () => {
-        mockFs.access.mockResolvedValue(undefined);
-        mockFs.readFile.mockResolvedValue('existing content' as any);
-        mockFs.writeFile.mockResolvedValue(undefined);
-        mockFs.rename.mockRejectedValue(new Error('Rename failed'));
-        mockFs.unlink.mockResolvedValue(undefined);
+      it('propagates non-ENOENT errors', async () => {
+        const error = new Error('Permission denied');
+        (error as any).code = 'EACCES';
+        mockFs.appendFile.mockRejectedValue(error);
 
         await expect(appendFileContent('/test/file.txt', 'new content'))
-          .rejects.toThrow('Rename failed');
-
-        expect(mockFs.unlink).toHaveBeenCalled();
+          .rejects.toThrow('Permission denied');
       });
     });
 
     describe('writeOrUpdateFileContent', () => {
       it('creates new file if it does not exist', async () => {
         const error = new Error('ENOENT');
         (error as any).code = 'ENOENT';
-        mockFs.access.mockRejectedValue(error);
+        mockFs.appendFile.mockRejectedValue(error);
         mockFs.writeFile.mockResolvedValue(undefined);
 
         await writeOrUpdateFileContent('/test/newfile.txt', 'initial content');
@@ -370,29 +361,25 @@ describe('Lib Functions', () => {
       });
 
       it('appends to existing file', async () => {
-        mockFs.access.mockResolvedValue(undefined);
-        mockFs.readFile.mockResolvedValue('existing content' as any);
-        mockFs.writeFile.mockResolvedValue(undefined);
-        mockFs.rename.mockResolvedValue(undefined);
+        mockFs.appendFile.mockResolvedValue(undefined);
 
         await writeOrUpdateFileContent('/test/file.txt', '\nappended content');
 
-        expect(mockFs.readFile).toHaveBeenCalledWith('/test/file.txt', 'utf-8');
-        expect(mockFs.writeFile).toHaveBeenCalledWith(
-          expect.stringContaining('.tmp'),
-          'existing content\nappended content',
-          'utf-8'
+        expect(mockFs.appendFile).toHaveBeenCalledWith(
+          '/test/file.txt',
+          '\nappended content',
+          { encoding: 'utf-8', flag: 'a' }
         );
-        expect(mockFs.rename).toHaveBeenCalled();
+        expect(mockFs.writeFile).not.toHaveBeenCalled();
       });
 
-      it('handles file exists error during creation by using atomic write', async () => {
+      it('falls back to atomic rename when target file already exists at create time', async () => {
         const notFoundError = new Error('ENOENT');
         (notFoundError as any).code = 'ENOENT';
         const existsError = new Error('EEXIST');
         (existsError as any).code = 'EEXIST';
 
-        mockFs.access.mockRejectedValue(notFoundError);
+        mockFs.appendFile.mockRejectedValue(notFoundError);
         mockFs.writeFile
           .mockRejectedValueOnce(existsError)
           .mockResolvedValueOnce(undefined);
@@ -404,10 +391,10 @@ describe('Lib Functions', () => {
         expect(mockFs.rename).toHaveBeenCalled();
       });
 
-      it('propagates non-ENOENT access errors', async () => {
+      it('propagates non-ENOENT append errors', async () => {
         const error = new Error('Permission denied');
         (error as any).code = 'EACCES';
-        mockFs.access.mockRejectedValue(error);
+        mockFs.appendFile.mockRejectedValue(error);
 
         await expect(writeOrUpdateFileContent('/test/file.txt', 'content'))
           .rejects.toThrow('Permission denied');

src/filesystem/index.ts

@@ -112,20 +112,13 @@ const ReadMultipleFilesArgsSchema = z.object({
     .describe("Array of file paths to read. Each path must be a string pointing to a valid file within allowed directories."),
 });
 
-const WriteFileArgsSchema = z.object({
-  path: z.string(),
-  content: z.string(),
-});
-
-const AppendFileArgsSchema = z.object({
-  path: z.string(),
-  content: z.string(),
-});
-
-const WriteOrUpdateFileArgsSchema = z.object({
+const PathContentArgsSchema = z.object({
   path: z.string(),
   content: z.string(),
 });
+const WriteFileArgsSchema = PathContentArgsSchema;
+const AppendFileArgsSchema = PathContentArgsSchema;
+const WriteOrUpdateFileArgsSchema = PathContentArgsSchema;
 
 const EditOperation = z.object({
   oldText: z.string().describe('Text to search for - must match exactly'),
@@ -420,13 +413,12 @@ server.registerTool(
   },
   async (args: z.infer<typeof WriteOrUpdateFileArgsSchema>) => {
     const validPath = await validatePath(args.path);
+    const existed = await fs.access(validPath).then(() => true).catch(() => false);
     await writeOrUpdateFileContent(validPath, args.content);
 
-    // Determine if file was created or updated for better feedback
-    const stats = await fs.stat(validPath);
-    const message = stats.birthtime.getTime() === stats.mtime.getTime()
-      ? `Successfully created ${args.path} with content`
-      : `Successfully appended content to ${args.path}`;
+    const message = existed
+      ? `Successfully appended content to ${args.path}`
+      : `Successfully created ${args.path} with content`;
 
     return {
       content: [{ type: "text" as const, text: message }],

src/filesystem/lib.ts

@@ -185,66 +185,25 @@ export async function writeFileContent(filePath: string, content: string): Promi
 }
 
 export async function appendFileContent(filePath: string, content: string): Promise<void> {
-  // Check if file exists
   try {
-    await fs.access(filePath);
+    await fs.appendFile(filePath, content, { encoding: 'utf-8', flag: 'a' });
   } catch (error) {
     if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
       throw new Error(`File does not exist: ${filePath}`);
     }
     throw error;
   }
-
-  // Read existing content
-  const existingContent = await readFileContent(filePath);
-
-  // Combine existing and new content
-  const combinedContent = existingContent + content;
-
-  // Use atomic write to update the file
-  const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`;
-  try {
-    await fs.writeFile(tempPath, combinedContent, 'utf-8');
-    await fs.rename(tempPath, filePath);
-  } catch (error) {
-    try {
-      await fs.unlink(tempPath);
-    } catch {}
-    throw error;
-  }
 }
 
 export async function writeOrUpdateFileContent(filePath: string, content: string): Promise<void> {
-  // Check if file exists
-  let fileExists = false;
   try {
-    await fs.access(filePath);
-    fileExists = true;
+    await appendFileContent(filePath, content);
   } catch (error) {
-    if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
-      throw error;
-    }
-  }
-
-  if (fileExists) {
-    // File exists, append the content
-    const existingContent = await readFileContent(filePath);
-    const combinedContent = existingContent + content;
-
-    // Use atomic write to update the file
-    const tempPath = `${filePath}.${randomBytes(16).toString('hex')}.tmp`;
-    try {
-      await fs.writeFile(tempPath, combinedContent, 'utf-8');
-      await fs.rename(tempPath, filePath);
-    } catch (error) {
-      try {
-        await fs.unlink(tempPath);
-      } catch {}
-      throw error;
+    if (error instanceof Error && error.message.startsWith('File does not exist:')) {
+      await writeFileContent(filePath, content);
+      return;
     }
-  } else {
-    // File doesn't exist, create it with the content
-    await writeFileContent(filePath, content);
+    throw error;
   }
 }