diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index 628807fe2..98a26950a 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -179,4 +179,83 @@ describe('AcpFileSystemService', () => { expect(client.readTextFile).not.toHaveBeenCalled(); }); }); + + describe('readTextFileWithInfo', () => { + it('reads through ACP and strips UTF-8 BOM', async () => { + const client = { + readTextFile: vi.fn().mockResolvedValue({ content: '\ufeffhello' }), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-info-1', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const result = await svc.readTextFileWithInfo('/some/file.txt'); + expect(result).toEqual({ + content: 'hello', + encoding: 'utf-8', + bom: true, + }); + expect(client.readTextFile).toHaveBeenCalledWith({ + path: '/some/file.txt', + sessionId: 'session-info-1', + }); + }); + + it('converts RESOURCE_NOT_FOUND error to ENOENT', async () => { + const resourceNotFoundError = { + code: RESOURCE_NOT_FOUND_CODE, + message: 'File not found', + }; + const client = { + readTextFile: vi.fn().mockRejectedValue(resourceNotFoundError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-info-2', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + await expect( + svc.readTextFileWithInfo('/some/missing.txt'), + ).rejects.toMatchObject({ + code: 'ENOENT', + errno: -2, + path: '/some/missing.txt', + }); + }); + + it('uses fallback when readTextFile capability is disabled', async () => { + const client = { + readTextFile: vi.fn(), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + ( + fallback.readTextFileWithInfo as ReturnType + ).mockResolvedValue({ content: 'fallback', encoding: 'gbk', bom: false }); + + const svc = new AcpFileSystemService( + client, + 'session-info-3', + { readTextFile: false, writeTextFile: true }, + fallback, + ); + + const result = await svc.readTextFileWithInfo('/some/file.txt'); + expect(result).toEqual({ + content: 'fallback', + encoding: 'gbk', + bom: false, + }); + expect(fallback.readTextFileWithInfo).toHaveBeenCalledWith( + '/some/file.txt', + ); + expect(client.readTextFile).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/acp-integration/service/filesystem.ts b/packages/cli/src/acp-integration/service/filesystem.ts index 25ad296fb..459b936a8 100644 --- a/packages/cli/src/acp-integration/service/filesystem.ts +++ b/packages/cli/src/acp-integration/service/filesystem.ts @@ -16,6 +16,26 @@ import type { const RESOURCE_NOT_FOUND_CODE = -32002; +function getErrorCode(error: unknown): unknown { + if (error instanceof RequestError) { + return error.code; + } + + if (typeof error === 'object' && error !== null && 'code' in error) { + return (error as { code?: unknown }).code; + } + + return undefined; +} + +function createEnoentError(filePath: string): NodeJS.ErrnoException { + const err = new Error(`File not found: ${filePath}`) as NodeJS.ErrnoException; + err.code = 'ENOENT'; + err.errno = -2; + err.path = filePath; + return err; +} + export class AcpFileSystemService implements FileSystemService { constructor( private readonly connection: AgentSideConnection, @@ -36,21 +56,10 @@ export class AcpFileSystemService implements FileSystemService { sessionId: this.sessionId, }); } catch (error) { - const errorCode = - error instanceof RequestError - ? error.code - : typeof error === 'object' && error !== null && 'code' in error - ? (error as { code?: unknown }).code - : undefined; + const errorCode = getErrorCode(error); if (errorCode === RESOURCE_NOT_FOUND_CODE) { - const err = new Error( - `File not found: ${filePath}`, - ) as NodeJS.ErrnoException; - err.code = 'ENOENT'; - err.errno = -2; - err.path = filePath; - throw err; + throw createEnoentError(filePath); } throw error; @@ -60,9 +69,33 @@ export class AcpFileSystemService implements FileSystemService { } async readTextFileWithInfo(filePath: string): Promise { - // ACP protocol does not expose encoding metadata; delegate to the local - // fallback which performs a single-pass read with encoding detection. - return this.fallback.readTextFileWithInfo(filePath); + if (!this.capabilities.readTextFile) { + return this.fallback.readTextFileWithInfo(filePath); + } + + let response: { content: string }; + try { + response = await this.connection.readTextFile({ + path: filePath, + sessionId: this.sessionId, + }); + } catch (error) { + const errorCode = getErrorCode(error); + if (errorCode === RESOURCE_NOT_FOUND_CODE) { + throw createEnoentError(filePath); + } + throw error; + } + + const hasUtf8Bom = + response.content.length > 0 && response.content.codePointAt(0) === 0xfeff; + + return { + content: hasUtf8Bom ? response.content.slice(1) : response.content, + // ACP protocol currently returns text only and does not expose source encoding. + encoding: 'utf-8', + bom: hasUtf8Bom, + }; } async writeTextFile( diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index c76c1e2c2..18ef00c32 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -548,6 +548,35 @@ describe('WriteFileTool', () => { ); }); + it('should treat metadata ENOENT as new file when readTextFile returned empty content', async () => { + const filePath = path.join(rootDir, 'execute_acp_like_missing_file.txt'); + const proposedContent = 'content from acp-like flow'; + const writeSpy = vi.spyOn(fsService, 'writeTextFile'); + + // Simulate ACP behavior where missing files can be returned as empty content. + vi.spyOn(fsService, 'readTextFile').mockResolvedValueOnce(''); + vi.spyOn(fsService, 'readTextFileWithInfo').mockImplementationOnce(() => { + const error = new Error('File not found') as NodeJS.ErrnoException; + error.code = 'ENOENT'; + return Promise.reject(error); + }); + + const params = { file_path: filePath, content: proposedContent }; + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error).toBeUndefined(); + expect(result.llmContent).toMatch( + /Successfully created and wrote to new file/, + ); + expect(writeSpy).toHaveBeenCalledWith(filePath, proposedContent, { + bom: false, + encoding: undefined, + }); + expect(fs.existsSync(filePath)).toBe(true); + expect(fs.readFileSync(filePath, 'utf8')).toBe(proposedContent); + }); + it('should create directory if it does not exist', async () => { const dirPath = path.join(rootDir, 'new_dir_for_write'); const filePath = path.join(dirPath, 'file_in_new_dir.txt'); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index b578aab00..80867f037 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -135,8 +135,8 @@ class WriteFileToolInvocation extends BaseToolInvocation< override async shouldConfirmExecute( _abortSignal: AbortSignal, ): Promise { -const mode = this.config.getApprovalMode(); - if (mode === ApprovalMode.AUTO_EDIT || mode === ApprovalMode.YOLO) { + const mode = this.config.getApprovalMode(); + if (mode === ApprovalMode.AUTO_EDIT || mode === ApprovalMode.YOLO) { return false; } @@ -230,10 +230,7 @@ const mode = this.config.getApprovalMode(); } = correctedContentResult; // fileExists is true if the file existed (and was readable or unreadable but caught by readError). // fileExists is false if the file did not exist (ENOENT). - const isNewFile = - !fileExists || - (correctedContentResult.error !== undefined && - !correctedContentResult.fileExists); + let isNewFile = !fileExists; try { const dirName = path.dirname(file_path); @@ -246,15 +243,28 @@ const mode = this.config.getApprovalMode(); let useBOM = false; let detectedEncoding: string | undefined; if (!isNewFile) { - // Use readTextFileWithInfo for a single I/O pass that returns encoding - // and BOM metadata together, avoiding separate detectFileBOM / detectFileEncoding calls. - const fileInfo = await this.config - .getFileSystemService() - .readTextFileWithInfo(file_path); - useBOM = fileInfo.bom; - detectedEncoding = fileInfo.encoding; - } else { + try { + // Use readTextFileWithInfo for a single I/O pass that returns encoding + // and BOM metadata together, avoiding separate detectFileBOM / detectFileEncoding calls. + const fileInfo = await this.config + .getFileSystemService() + .readTextFileWithInfo(file_path); + useBOM = fileInfo.bom; + detectedEncoding = fileInfo.encoding; + } catch (error) { + if (!isNodeError(error) || error.code !== 'ENOENT') { + throw error; + } + // ACP backends may report missing files as empty content in readTextFile(), + // then surface ENOENT only when metadata is requested. + // Treat this as a new-file write path instead of failing. + isNewFile = true; + } + } + + if (isNewFile) { useBOM = this.config.getDefaultFileEncoding() === FileEncoding.UTF8_BOM; + detectedEncoding = undefined; } await this.config diff --git a/packages/vscode-ide-companion/src/utils/errorMessage.ts b/packages/vscode-ide-companion/src/utils/errorMessage.ts index 37c0c1de3..8cd7301b0 100644 --- a/packages/vscode-ide-companion/src/utils/errorMessage.ts +++ b/packages/vscode-ide-companion/src/utils/errorMessage.ts @@ -8,6 +8,18 @@ export function getErrorMessage( error: unknown, fallback = 'Unknown error', ): string { + const combineMessageAndDetails = ( + message: string | null, + detailsMessage: string | null, + ): string | null => { + if (message && detailsMessage) { + return message === detailsMessage + ? message + : `${message}: ${detailsMessage}`; + } + return message ?? detailsMessage; + }; + const extractDetailsMessage = (value: unknown): string | null => { if (typeof value === 'string' && value) { return value; @@ -47,23 +59,32 @@ export function getErrorMessage( } if (typeof error === 'object' && error !== null) { const record = error as Record; - if (typeof record['message'] === 'string' && record['message']) { - return record['message']; - } + const topLevelMessage = + typeof record['message'] === 'string' && record['message'] + ? record['message'] + : null; const topLevelDetailsMessage = extractDetailsMessage(record['data']); - if (topLevelDetailsMessage) { - return topLevelDetailsMessage; + const combinedTopLevelMessage = combineMessageAndDetails( + topLevelMessage, + topLevelDetailsMessage, + ); + if (combinedTopLevelMessage) { + return combinedTopLevelMessage; } const nested = record['error']; if (typeof nested === 'object' && nested !== null) { const nestedRecord = nested as Record; - const nestedMessage = nestedRecord['message']; - if (typeof nestedMessage === 'string' && nestedMessage) { - return nestedMessage; - } + const nestedMessage = + typeof nestedRecord['message'] === 'string' && nestedRecord['message'] + ? nestedRecord['message'] + : null; const nestedDetailsMessage = extractDetailsMessage(nestedRecord['data']); - if (nestedDetailsMessage) { - return nestedDetailsMessage; + const combinedNestedMessage = combineMessageAndDetails( + nestedMessage, + nestedDetailsMessage, + ); + if (combinedNestedMessage) { + return combinedNestedMessage; } } try {