feat(core): Migrate web-search, write-file, and discovered-tool. (#6188)

Co-authored-by: Jacob Richman <jacob314@gmail.com>

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(() => tool.build(params)).toThrow(
+        /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(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',
+      );
     });
   });
 });