From 2d525d9fd03f87c563c861a5ccbb454cd1e2880a Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Sun, 1 Feb 2026 11:23:02 +0800 Subject: [PATCH] fix: Address review comments for BOM encoding support - Edit tool now respects defaultFileEncoding for new files - Edit tool preserves BOM character for existing files without re-adding - AcpFileSystemService detects BOM through ACP client with fallback - Use line: null, limit: 1 for efficient BOM detection - Add unit tests for AcpFileSystemService.detectFileBOM - Add unit tests for EditTool BOM handling Co-authored-by: Qwen-Coder --- .../service/filesystem.test.ts | 87 +++++++++++++++++++ .../src/acp-integration/service/filesystem.ts | 17 +++- packages/core/src/tools/edit.test.ts | 75 ++++++++++++++++ packages/core/src/tools/edit.ts | 20 ++++- 4 files changed, 195 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index a2ae0939e..6eb3dfa1b 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -17,6 +17,93 @@ const createFallback = (): FileSystemService => ({ }); describe('AcpFileSystemService', () => { + describe('detectFileBOM', () => { + it('detects BOM through ACP client when content starts with U+FEFF', async () => { + const client = { + readTextFile: vi + .fn() + .mockResolvedValue({ content: '\ufeff// BOM file' }), + } as unknown as import('../acp.js').Client; + + const svc = new AcpFileSystemService( + client, + 'session-1', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const result = await svc.detectFileBOM('/test/file.txt'); + expect(result).toBe(true); + expect(client.readTextFile).toHaveBeenCalledWith({ + path: '/test/file.txt', + sessionId: 'session-1', + line: null, + limit: 1, + }); + }); + + it('detects no BOM through ACP client when content does not start with U+FEFF', async () => { + const client = { + readTextFile: vi.fn().mockResolvedValue({ content: '// No BOM file' }), + } as unknown as import('../acp.js').Client; + + const svc = new AcpFileSystemService( + client, + 'session-2', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const result = await svc.detectFileBOM('/test/file.txt'); + expect(result).toBe(false); + }); + + it('falls back to local filesystem when ACP client fails', async () => { + const client = { + readTextFile: vi.fn().mockRejectedValue(new Error('Network error')), + } as unknown as import('../acp.js').Client; + + const fallback = createFallback(); + (fallback.detectFileBOM as ReturnType).mockResolvedValue( + true, + ); + + const svc = new AcpFileSystemService( + client, + 'session-3', + { readTextFile: true, writeTextFile: true }, + fallback, + ); + + const result = await svc.detectFileBOM('/test/file.txt'); + expect(result).toBe(true); + expect(fallback.detectFileBOM).toHaveBeenCalledWith('/test/file.txt'); + }); + + it('falls back to local filesystem when readTextFile capability is disabled', async () => { + const client = { + readTextFile: vi.fn(), + } as unknown as import('../acp.js').Client; + + const fallback = createFallback(); + (fallback.detectFileBOM as ReturnType).mockResolvedValue( + false, + ); + + const svc = new AcpFileSystemService( + client, + 'session-4', + { readTextFile: false, writeTextFile: true }, + fallback, + ); + + const result = await svc.detectFileBOM('/test/file.txt'); + expect(result).toBe(false); + expect(fallback.detectFileBOM).toHaveBeenCalledWith('/test/file.txt'); + expect(client.readTextFile).not.toHaveBeenCalled(); + }); + }); + describe('readTextFile ENOENT handling', () => { it('converts RESOURCE_NOT_FOUND error to ENOENT', async () => { const resourceNotFoundError = { diff --git a/packages/cli/src/acp-integration/service/filesystem.ts b/packages/cli/src/acp-integration/service/filesystem.ts index fc86ee5bb..17a0cdbcf 100644 --- a/packages/cli/src/acp-integration/service/filesystem.ts +++ b/packages/cli/src/acp-integration/service/filesystem.ts @@ -74,7 +74,22 @@ export class AcpFileSystemService implements FileSystemService { } async detectFileBOM(filePath: string): Promise { - // Always use fallback for BOM detection + // Try to detect BOM through ACP client first by reading first line + if (this.capabilities.readTextFile) { + try { + const response = await this.client.readTextFile({ + path: filePath, + sessionId: this.sessionId, + line: null, + limit: 1, + }); + // Check if content starts with BOM character (U+FEFF) + return response.content.charCodeAt(0) === 0xfeff; + } catch { + // Fall through to fallback if ACP read fails + } + } + // Fall back to local filesystem detection return this.fallback.detectFileBOM(filePath); } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 9e41b938e..8b55e28a9 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -87,6 +87,7 @@ describe('EditTool', () => { getGeminiMdFileCount: () => 0, setGeminiMdFileCount: vi.fn(), getToolRegistry: () => ({}) as any, // Minimal mock for ToolRegistry + getDefaultFileEncoding: vi.fn().mockReturnValue('utf-8'), } as unknown as Config; // Reset mocks before each test @@ -473,6 +474,80 @@ describe('EditTool', () => { }); }); + it('should create new file with BOM when defaultFileEncoding is utf-8-bom', async () => { + // Change config to use utf-8-bom + (mockConfig.getDefaultFileEncoding as Mock).mockReturnValue('utf-8-bom'); + + const newFileName = 'bom_new_file.txt'; + const newFilePath = path.join(rootDir, newFileName); + const fileContent = 'Content for BOM file.'; + const params: EditToolParams = { + file_path: newFilePath, + old_string: '', + new_string: fileContent, + }; + + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + const invocation = tool.build(params); + await invocation.execute(new AbortController().signal); + + // Verify file has BOM + const fileBuffer = fs.readFileSync(newFilePath); + expect(fileBuffer[0]).toBe(0xef); + expect(fileBuffer[1]).toBe(0xbb); + expect(fileBuffer[2]).toBe(0xbf); + expect(fileBuffer.toString('utf8')).toContain(fileContent); + }); + + it('should create new file without BOM when defaultFileEncoding is utf-8', async () => { + // Config defaults to utf-8 + const newFileName = 'no_bom_new_file.txt'; + const newFilePath = path.join(rootDir, newFileName); + const fileContent = 'Content without BOM.'; + const params: EditToolParams = { + file_path: newFilePath, + old_string: '', + new_string: fileContent, + }; + + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + const invocation = tool.build(params); + await invocation.execute(new AbortController().signal); + + // Verify file does not have BOM + const fileBuffer = fs.readFileSync(newFilePath); + expect(fileBuffer[0]).not.toBe(0xef); + expect(fileBuffer.toString('utf8')).toBe(fileContent); + }); + + it('should preserve BOM character in content when editing existing file', async () => { + const bomFilePath = path.join(rootDir, 'existing_bom.txt'); + // Create file with BOM (BOM is \ufeff character in string) + const originalContent = '\ufeff// Original line\nconst x = 1;'; + fs.writeFileSync(bomFilePath, originalContent, 'utf8'); + + const params: EditToolParams = { + file_path: bomFilePath, + old_string: 'const x = 1;', + new_string: 'const x = 2;', + }; + + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + const invocation = tool.build(params); + await invocation.execute(new AbortController().signal); + + // Verify file still has BOM and new content + const resultContent = fs.readFileSync(bomFilePath, 'utf8'); + expect(resultContent.charCodeAt(0)).toBe(0xfeff); // BOM preserved + expect(resultContent).toContain('const x = 2;'); + }); + it('should return error if old_string is not found in file', async () => { fs.writeFileSync(filePath, 'Some content.', 'utf8'); const params: EditToolParams = { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index ec2572904..e7d8aea7f 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -20,6 +20,7 @@ import { makeRelative, shortenPath } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; +import { FileEncoding } from '../services/fileSystemService.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; import { ReadFileTool } from './read-file.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; @@ -367,9 +368,22 @@ class EditToolInvocation implements ToolInvocation { try { this.ensureParentDirectoriesExist(this.params.file_path); - await this.config - .getFileSystemService() - .writeTextFile(this.params.file_path, editData.newContent); + + // For new files, apply default file encoding setting + // For existing files, keep original content as-is (including any BOM character) + if (editData.isNewFile) { + const useBOM = + this.config.getDefaultFileEncoding() === FileEncoding.UTF8_BOM; + await this.config + .getFileSystemService() + .writeTextFile(this.params.file_path, editData.newContent, { + bom: useBOM, + }); + } else { + await this.config + .getFileSystemService() + .writeTextFile(this.params.file_path, editData.newContent); + } const fileName = path.basename(this.params.file_path); const originallyProposedContent =