diff options
Diffstat (limited to 'packages/core/src/tools/write-file.test.ts')
| -rw-r--r-- | packages/core/src/tools/write-file.test.ts | 192 |
1 files changed, 74 insertions, 118 deletions
diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 06561602..2d877115 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -13,7 +13,11 @@ import { vi, type Mocked, } from 'vitest'; -import { WriteFileTool, WriteFileToolParams } from './write-file.js'; +import { + getCorrectedFileContent, + WriteFileTool, + WriteFileToolParams, +} from './write-file.js'; import { ToolErrorType } from './tool-error.js'; import { FileDiff, @@ -174,74 +178,67 @@ describe('WriteFileTool', () => { vi.clearAllMocks(); }); - describe('validateToolParams', () => { - it('should return null for valid absolute path within root', () => { + describe('build', () => { + it('should return an invocation for a valid absolute path within root', () => { const params = { file_path: path.join(rootDir, 'test.txt'), content: 'hello', }; - expect(tool.validateToolParams(params)).toBeNull(); + const invocation = tool.build(params); + expect(invocation).toBeDefined(); + expect(invocation.params).toEqual(params); }); - it('should return error for relative path', () => { + it('should throw an error for a relative path', () => { const params = { file_path: 'test.txt', content: 'hello' }; - expect(tool.validateToolParams(params)).toMatch( - /File path must be absolute/, - ); + expect(() => tool.build(params)).toThrow(/File path must be absolute/); }); - it('should return error for path outside root', () => { + it('should throw an error for a path outside root', () => { const outsidePath = path.resolve(tempDir, 'outside-root.txt'); const params = { file_path: outsidePath, content: 'hello', }; - const error = tool.validateToolParams(params); - expect(error).toContain( - 'File path must be within one of the workspace directories', + expect(() => tool.build(params)).toThrow( + /File path must be within one of the workspace directories/, ); }); - it('should return error if path is a directory', () => { + it('should throw an error if path is a directory', () => { const dirAsFilePath = path.join(rootDir, 'a_directory'); fs.mkdirSync(dirAsFilePath); const params = { file_path: dirAsFilePath, content: 'hello', }; - expect(tool.validateToolParams(params)).toMatch( + expect(() => tool.build(params)).toThrow( `Path is a directory, not a file: ${dirAsFilePath}`, ); }); - it('should return error if the content is null', () => { + it('should throw an error if the content is null', () => { const dirAsFilePath = path.join(rootDir, 'a_directory'); fs.mkdirSync(dirAsFilePath); const params = { file_path: dirAsFilePath, content: null, } as unknown as WriteFileToolParams; // Intentionally non-conforming - expect(tool.validateToolParams(params)).toMatch( - `params/content must be string`, - ); + expect(() => tool.build(params)).toThrow('params/content must be string'); }); - }); - describe('getDescription', () => { - it('should return error if the file_path is empty', () => { + it('should throw error if the file_path is empty', () => { const dirAsFilePath = path.join(rootDir, 'a_directory'); fs.mkdirSync(dirAsFilePath); const params = { file_path: '', content: '', }; - expect(tool.getDescription(params)).toMatch( - `Model did not provide valid parameters for write file tool, missing or empty "file_path"`, - ); + expect(() => tool.build(params)).toThrow(`Missing or empty "file_path"`); }); }); - describe('_getCorrectedFileContent', () => { + describe('getCorrectedFileContent', () => { it('should call ensureCorrectFileContent for a new file', async () => { const filePath = path.join(rootDir, 'new_corrected_file.txt'); const proposedContent = 'Proposed new content.'; @@ -250,8 +247,8 @@ describe('WriteFileTool', () => { // Ensure the mock is set for this specific test case if needed, or rely on beforeEach mockEnsureCorrectFileContent.mockResolvedValue(correctedContent); - // @ts-expect-error _getCorrectedFileContent is private - const result = await tool._getCorrectedFileContent( + const result = await getCorrectedFileContent( + mockConfig, filePath, proposedContent, abortSignal, @@ -287,8 +284,8 @@ describe('WriteFileTool', () => { occurrences: 1, } as CorrectedEditResult); - // @ts-expect-error _getCorrectedFileContent is private - const result = await tool._getCorrectedFileContent( + const result = await getCorrectedFileContent( + mockConfig, filePath, proposedContent, abortSignal, @@ -324,8 +321,8 @@ describe('WriteFileTool', () => { throw readError; }); - // @ts-expect-error _getCorrectedFileContent is private - const result = await tool._getCorrectedFileContent( + const result = await getCorrectedFileContent( + mockConfig, filePath, proposedContent, abortSignal, @@ -349,18 +346,6 @@ describe('WriteFileTool', () => { describe('shouldConfirmExecute', () => { const abortSignal = new AbortController().signal; - it('should return false if params are invalid (relative path)', async () => { - const params = { file_path: 'relative.txt', content: 'test' }; - const confirmation = await tool.shouldConfirmExecute(params, abortSignal); - expect(confirmation).toBe(false); - }); - - it('should return false if params are invalid (outside root)', async () => { - const outsidePath = path.resolve(tempDir, 'outside-root.txt'); - const params = { file_path: outsidePath, content: 'test' }; - const confirmation = await tool.shouldConfirmExecute(params, abortSignal); - expect(confirmation).toBe(false); - }); it('should return false if _getCorrectedFileContent returns an error', async () => { const filePath = path.join(rootDir, 'confirm_error_file.txt'); @@ -373,7 +358,8 @@ describe('WriteFileTool', () => { throw readError; }); - const confirmation = await tool.shouldConfirmExecute(params, abortSignal); + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute(abortSignal); expect(confirmation).toBe(false); vi.spyOn(fs, 'readFileSync').mockImplementation(originalReadFileSync); @@ -387,8 +373,8 @@ describe('WriteFileTool', () => { mockEnsureCorrectFileContent.mockResolvedValue(correctedContent); // Ensure this mock is active const params = { file_path: filePath, content: proposedContent }; - const confirmation = (await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = (await invocation.shouldConfirmExecute( abortSignal, )) as ToolEditConfirmationDetails; @@ -430,8 +416,8 @@ describe('WriteFileTool', () => { }); const params = { file_path: filePath, content: proposedContent }; - const confirmation = (await tool.shouldConfirmExecute( - params, + const invocation = tool.build(params); + const confirmation = (await invocation.shouldConfirmExecute( abortSignal, )) as ToolEditConfirmationDetails; @@ -461,31 +447,6 @@ describe('WriteFileTool', () => { describe('execute', () => { const abortSignal = new AbortController().signal; - it('should return error if params are invalid (relative path)', async () => { - const params = { file_path: 'relative.txt', content: 'test' }; - const result = await tool.execute(params, abortSignal); - expect(result.llmContent).toContain( - 'Could not write file due to invalid parameters:', - ); - expect(result.returnDisplay).toMatch(/File path must be absolute/); - expect(result.error).toEqual({ - message: 'File path must be absolute: relative.txt', - type: ToolErrorType.INVALID_TOOL_PARAMS, - }); - }); - - it('should return error if params are invalid (path outside root)', async () => { - const outsidePath = path.resolve(tempDir, 'outside-root.txt'); - const params = { file_path: outsidePath, content: 'test' }; - const result = await tool.execute(params, abortSignal); - expect(result.llmContent).toContain( - 'Could not write file due to invalid parameters:', - ); - expect(result.returnDisplay).toContain( - 'File path must be within one of the workspace directories', - ); - expect(result.error?.type).toBe(ToolErrorType.INVALID_TOOL_PARAMS); - }); it('should return error if _getCorrectedFileContent returns an error during execute', async () => { const filePath = path.join(rootDir, 'execute_error_file.txt'); @@ -498,7 +459,8 @@ describe('WriteFileTool', () => { throw readError; }); - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Error checking existing file:'); expect(result.returnDisplay).toMatch( /Error checking existing file: Simulated read error for execute/, @@ -520,11 +482,9 @@ describe('WriteFileTool', () => { mockEnsureCorrectFileContent.mockResolvedValue(correctedContent); const params = { file_path: filePath, content: proposedContent }; + const invocation = tool.build(params); - const confirmDetails = await tool.shouldConfirmExecute( - params, - abortSignal, - ); + const confirmDetails = await invocation.shouldConfirmExecute(abortSignal); if ( typeof confirmDetails === 'object' && 'onConfirm' in confirmDetails && @@ -533,7 +493,7 @@ describe('WriteFileTool', () => { await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); } - const result = await tool.execute(params, abortSignal); + const result = await invocation.execute(abortSignal); expect(mockEnsureCorrectFileContent).toHaveBeenCalledWith( proposedContent, @@ -578,11 +538,9 @@ describe('WriteFileTool', () => { }); const params = { file_path: filePath, content: proposedContent }; + const invocation = tool.build(params); - const confirmDetails = await tool.shouldConfirmExecute( - params, - abortSignal, - ); + const confirmDetails = await invocation.shouldConfirmExecute(abortSignal); if ( typeof confirmDetails === 'object' && 'onConfirm' in confirmDetails && @@ -591,7 +549,7 @@ describe('WriteFileTool', () => { await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); } - const result = await tool.execute(params, abortSignal); + const result = await invocation.execute(abortSignal); expect(mockEnsureCorrectEdit).toHaveBeenCalledWith( filePath, @@ -623,11 +581,9 @@ describe('WriteFileTool', () => { mockEnsureCorrectFileContent.mockResolvedValue(content); // Ensure this mock is active const params = { file_path: filePath, content }; + const invocation = tool.build(params); // Simulate confirmation if your logic requires it before execute, or remove if not needed for this path - const confirmDetails = await tool.shouldConfirmExecute( - params, - abortSignal, - ); + const confirmDetails = await invocation.shouldConfirmExecute(abortSignal); if ( typeof confirmDetails === 'object' && 'onConfirm' in confirmDetails && @@ -636,7 +592,7 @@ describe('WriteFileTool', () => { await confirmDetails.onConfirm(ToolConfirmationOutcome.ProceedOnce); } - await tool.execute(params, abortSignal); + await invocation.execute(abortSignal); expect(fs.existsSync(dirPath)).toBe(true); expect(fs.statSync(dirPath).isDirectory()).toBe(true); @@ -654,7 +610,8 @@ describe('WriteFileTool', () => { content, modified_by_user: true, }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).toMatch(/User modified the `content`/); }); @@ -669,7 +626,8 @@ describe('WriteFileTool', () => { content, modified_by_user: false, }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).not.toMatch(/User modified the `content`/); }); @@ -683,7 +641,8 @@ describe('WriteFileTool', () => { file_path: filePath, content, }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.llmContent).not.toMatch(/User modified the `content`/); }); @@ -695,7 +654,7 @@ describe('WriteFileTool', () => { file_path: path.join(rootDir, 'file.txt'), content: 'test content', }; - expect(tool.validateToolParams(params)).toBeNull(); + expect(() => tool.build(params)).not.toThrow(); }); it('should reject paths outside workspace root', () => { @@ -703,24 +662,9 @@ describe('WriteFileTool', () => { file_path: '/etc/passwd', content: 'malicious', }; - const error = tool.validateToolParams(params); - expect(error).toContain( - 'File path must be within one of the workspace directories', - ); - expect(error).toContain(rootDir); - }); - - it('should provide clear error message with workspace directories', () => { - const outsidePath = path.join(tempDir, 'outside-root.txt'); - const params = { - file_path: outsidePath, - content: 'test', - }; - const error = tool.validateToolParams(params); - expect(error).toContain( - 'File path must be within one of the workspace directories', + expect(() => tool.build(params)).toThrow( + /File path must be within one of the workspace directories/, ); - expect(error).toContain(rootDir); }); }); @@ -740,13 +684,16 @@ describe('WriteFileTool', () => { }); const params = { file_path: filePath, content }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.PERMISSION_DENIED); expect(result.llmContent).toContain( `Permission denied writing to file: ${filePath} (EACCES)`, ); - expect(result.returnDisplay).toContain('Permission denied'); + expect(result.returnDisplay).toContain( + `Permission denied writing to file: ${filePath} (EACCES)`, + ); vi.spyOn(fs, 'writeFileSync').mockImplementation(originalWriteFileSync); }); @@ -766,13 +713,16 @@ describe('WriteFileTool', () => { }); const params = { file_path: filePath, content }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.NO_SPACE_LEFT); expect(result.llmContent).toContain( `No space left on device: ${filePath} (ENOSPC)`, ); - expect(result.returnDisplay).toContain('No space left'); + expect(result.returnDisplay).toContain( + `No space left on device: ${filePath} (ENOSPC)`, + ); vi.spyOn(fs, 'writeFileSync').mockImplementation(originalWriteFileSync); }); @@ -799,13 +749,16 @@ describe('WriteFileTool', () => { }); const params = { file_path: dirPath, content }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.TARGET_IS_DIRECTORY); expect(result.llmContent).toContain( `Target is a directory, not a file: ${dirPath} (EISDIR)`, ); - expect(result.returnDisplay).toContain('Target is a directory'); + expect(result.returnDisplay).toContain( + `Target is a directory, not a file: ${dirPath} (EISDIR)`, + ); vi.spyOn(fs, 'existsSync').mockImplementation(originalExistsSync); vi.spyOn(fs, 'writeFileSync').mockImplementation(originalWriteFileSync); @@ -824,13 +777,16 @@ describe('WriteFileTool', () => { }); const params = { file_path: filePath, content }; - const result = await tool.execute(params, abortSignal); + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); expect(result.llmContent).toContain( 'Error writing to file: Generic write error', ); - expect(result.returnDisplay).toContain('Generic write error'); + expect(result.returnDisplay).toContain( + 'Error writing to file: Generic write error', + ); }); }); }); |