From 394a7ea019b71341a22269d106e3f2e27ab95774 Mon Sep 17 00:00:00 2001 From: Riddhi Dutta Date: Mon, 17 Nov 2025 22:24:09 +0530 Subject: [PATCH] Refactored 3 files of tools package (#13231) Co-authored-by: riddhi --- packages/core/src/tools/edit.test.ts | 476 +++++++++--------- packages/core/src/tools/mcp-tool.test.ts | 612 ++++++++++------------- packages/core/src/tools/ripGrep.test.ts | 284 +++++------ 3 files changed, 593 insertions(+), 779 deletions(-) diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index d8f650f463..1f301c2129 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -210,84 +210,80 @@ describe('EditTool', () => { ); }); - it('should treat $ literally and not as replacement pattern', () => { - const current = "price is $100 and pattern end is ' '"; - const oldStr = 'price is $100'; - const newStr = 'price is $200'; + it.each([ + { + name: '$ literal', + current: "price is $100 and pattern end is ' '", + oldStr: 'price is $100', + newStr: 'price is $200', + expected: "price is $200 and pattern end is ' '", + }, + { + name: "$' literal", + current: 'foo', + oldStr: 'foo', + newStr: "bar$'baz", + expected: "bar$'baz", + }, + { + name: '$& literal', + current: 'hello world', + oldStr: 'hello', + newStr: '$&-replacement', + expected: '$&-replacement world', + }, + { + name: '$` literal', + current: 'prefix-middle-suffix', + oldStr: 'middle', + newStr: 'new$`content', + expected: 'prefix-new$`content-suffix', + }, + { + name: '$1, $2 capture groups literal', + current: 'test string', + oldStr: 'test', + newStr: '$1$2replacement', + expected: '$1$2replacement string', + }, + { + name: 'normal strings without problematic $', + current: 'normal text replacement', + oldStr: 'text', + newStr: 'string', + expected: 'normal string replacement', + }, + { + name: 'multiple occurrences with $ sequences', + current: 'foo bar foo baz', + oldStr: 'foo', + newStr: "test$'end", + expected: "test$'end bar test$'end baz", + }, + { + name: 'complex regex patterns with $ at end', + current: "| select('match', '^[sv]d[a-z]$')", + oldStr: "'^[sv]d[a-z]$'", + newStr: "'^[sv]d[a-z]$' # updated", + expected: "| select('match', '^[sv]d[a-z]$' # updated)", + }, + { + name: 'empty replacement with problematic $', + current: 'test content', + oldStr: 'nothing', + newStr: "replacement$'text", + expected: 'test content', + }, + { + name: '$$ (escaped dollar)', + current: 'price value', + oldStr: 'value', + newStr: '$$100', + expected: 'price $$100', + }, + ])('should handle $name', ({ current, oldStr, newStr, expected }) => { const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe("price is $200 and pattern end is ' '"); - }); - - it("should treat $' literally and not as a replacement pattern", () => { - const current = 'foo'; - const oldStr = 'foo'; - const newStr = "bar$'baz"; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe("bar$'baz"); - }); - - it('should treat $& literally and not as a replacement pattern', () => { - const current = 'hello world'; - const oldStr = 'hello'; - const newStr = '$&-replacement'; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe('$&-replacement world'); - }); - - it('should treat $` literally and not as a replacement pattern', () => { - const current = 'prefix-middle-suffix'; - const oldStr = 'middle'; - const newStr = 'new$`content'; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe('prefix-new$`content-suffix'); - }); - - it('should treat $1, $2 capture groups literally', () => { - const current = 'test string'; - const oldStr = 'test'; - const newStr = '$1$2replacement'; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe('$1$2replacement string'); - }); - - it('should use replaceAll for normal strings without problematic $ sequences', () => { - const current = 'normal text replacement'; - const oldStr = 'text'; - const newStr = 'string'; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe('normal string replacement'); - }); - - it('should handle multiple occurrences with problematic $ sequences', () => { - const current = 'foo bar foo baz'; - const oldStr = 'foo'; - const newStr = "test$'end"; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe("test$'end bar test$'end baz"); - }); - - it('should handle complex regex patterns with $ at end', () => { - const current = "| select('match', '^[sv]d[a-z]$')"; - const oldStr = "'^[sv]d[a-z]$'"; - const newStr = "'^[sv]d[a-z]$' # updated"; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe("| select('match', '^[sv]d[a-z]$' # updated)"); - }); - - it('should handle empty replacement with problematic $ in newString', () => { - const current = 'test content'; - const oldStr = 'nothing'; - const newStr = "replacement$'text"; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe('test content'); // No replacement because oldStr not found - }); - - it('should handle $$ (escaped dollar) correctly', () => { - const current = 'price value'; - const oldStr = 'value'; - const newStr = '$$100'; - const result = applyReplacement(current, oldStr, newStr, false); - expect(result).toBe('price $$100'); + expect(result).toBe(expected); }); }); @@ -786,46 +782,40 @@ describe('EditTool', () => { }); }); - it('should not include modification message when proposed content is not modified', async () => { - const initialContent = 'This is some old text.'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - modified_by_user: false, - }; + it.each([ + { + name: 'modified_by_user is false', + modifiedByUser: false, + }, + { + name: 'modified_by_user is not provided', + modifiedByUser: undefined, + }, + ])( + 'should not include modification message when $name', + async ({ modifiedByUser }) => { + const initialContent = 'This is some old text.'; + fs.writeFileSync(filePath, initialContent, 'utf8'); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + ...(modifiedByUser !== undefined && { + modified_by_user: modifiedByUser, + }), + }; - (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( - ApprovalMode.AUTO_EDIT, - ); - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).not.toMatch( - /User modified the `new_string` content/, - ); - }); - - it('should not include modification message when modified_by_user is not provided', async () => { - const initialContent = 'This is some old text.'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - }; - - (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( - ApprovalMode.AUTO_EDIT, - ); - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).not.toMatch( - /User modified the `new_string` content/, - ); - }); + expect(result.llmContent).not.toMatch( + /User modified the `new_string` content/, + ); + }, + ); it('should return error if old_string and new_string are identical', async () => { const initialContent = 'This is some identical text.'; @@ -877,149 +867,133 @@ describe('EditTool', () => { filePath = path.join(rootDir, testFile); }); - it('should return FILE_NOT_FOUND error', async () => { - const params: EditToolParams = { - file_path: filePath, - old_string: 'any', - new_string: 'new', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe(ToolErrorType.FILE_NOT_FOUND); - }); + it.each([ + { + name: 'FILE_NOT_FOUND error', + setup: () => {}, + params: { file_path: '', old_string: 'any', new_string: 'new' }, + expectedError: ToolErrorType.FILE_NOT_FOUND, + isAsyncTest: true, + }, + { + name: 'ATTEMPT_TO_CREATE_EXISTING_FILE error', + setup: (fp: string) => fs.writeFileSync(fp, 'existing content', 'utf8'), + params: { file_path: '', old_string: '', new_string: 'new content' }, + expectedError: ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE, + isAsyncTest: true, + }, + { + name: 'NO_OCCURRENCE_FOUND error', + setup: (fp: string) => fs.writeFileSync(fp, 'content', 'utf8'), + params: { file_path: '', old_string: 'not-found', new_string: 'new' }, + expectedError: ToolErrorType.EDIT_NO_OCCURRENCE_FOUND, + isAsyncTest: true, + }, + { + name: 'EXPECTED_OCCURRENCE_MISMATCH error', + setup: (fp: string) => fs.writeFileSync(fp, 'one one two', 'utf8'), + params: { + file_path: '', + old_string: 'one', + new_string: 'new', + expected_replacements: 3, + }, + expectedError: ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, + isAsyncTest: true, + }, + { + name: 'NO_CHANGE error', + setup: (fp: string) => fs.writeFileSync(fp, 'content', 'utf8'), + params: { file_path: '', old_string: 'content', new_string: 'content' }, + expectedError: ToolErrorType.EDIT_NO_CHANGE, + isAsyncTest: true, + }, + { + name: 'relative path (should not throw)', + setup: () => {}, + params: { + file_path: 'relative/path.txt', + old_string: 'a', + new_string: 'b', + }, + expectedError: null, + isAsyncTest: false, + }, + { + name: 'FILE_WRITE_FAILURE on write error', + setup: (fp: string) => { + fs.writeFileSync(fp, 'content', 'utf8'); + fs.chmodSync(fp, '444'); + }, + params: { + file_path: '', + old_string: 'content', + new_string: 'new content', + }, + expectedError: ToolErrorType.FILE_WRITE_FAILURE, + isAsyncTest: true, + }, + ])( + 'should return $name', + async ({ setup, params, expectedError, isAsyncTest }) => { + const testParams = { + ...params, + file_path: params.file_path || filePath, + }; + setup(filePath); - it('should return ATTEMPT_TO_CREATE_EXISTING_FILE error', async () => { - fs.writeFileSync(filePath, 'existing content', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: '', - new_string: 'new content', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe( - ToolErrorType.ATTEMPT_TO_CREATE_EXISTING_FILE, - ); - }); - - it('should return NO_OCCURRENCE_FOUND error', async () => { - fs.writeFileSync(filePath, 'content', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'not-found', - new_string: 'new', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_OCCURRENCE_FOUND); - }); - - it('should return EXPECTED_OCCURRENCE_MISMATCH error', async () => { - fs.writeFileSync(filePath, 'one one two', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'one', - new_string: 'new', - expected_replacements: 3, - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe( - ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, - ); - }); - - it('should return NO_CHANGE error', async () => { - fs.writeFileSync(filePath, 'content', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'content', - new_string: 'content', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_CHANGE); - }); - - it('should not throw error for relative path', async () => { - const params: EditToolParams = { - file_path: 'relative/path.txt', - old_string: 'a', - new_string: 'b', - }; - expect(() => tool.build(params)).not.toThrow(); - }); - - it('should return FILE_WRITE_FAILURE on write error', async () => { - fs.writeFileSync(filePath, 'content', 'utf8'); - // Make file readonly to trigger a write error - fs.chmodSync(filePath, '444'); - - const params: EditToolParams = { - file_path: filePath, - old_string: 'content', - new_string: 'new content', - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); - }); + if (!isAsyncTest) { + expect(() => tool.build(testParams)).not.toThrow(); + } else { + const invocation = tool.build(testParams); + const result = await invocation.execute(new AbortController().signal); + expect(result.error?.type).toBe(expectedError); + } + }, + ); }); describe('getDescription', () => { - it('should return "No file changes to..." if old_string and new_string are the same', () => { - const testFileName = 'test.txt'; - const params: EditToolParams = { - file_path: path.join(rootDir, testFileName), - old_string: 'identical_string', - new_string: 'identical_string', - }; - const invocation = tool.build(params); - // shortenPath will be called internally, resulting in just the file name - expect(invocation.getDescription()).toBe( - `No file changes to ${testFileName}`, - ); - }); - - it('should return a snippet of old and new strings if they are different', () => { - const testFileName = 'test.txt'; - const params: EditToolParams = { - file_path: path.join(rootDir, testFileName), - old_string: 'this is the old string value', - new_string: 'this is the new string value', - }; - const invocation = tool.build(params); - // shortenPath will be called internally, resulting in just the file name - // The snippets are truncated at 30 chars + '...' - expect(invocation.getDescription()).toBe( - `${testFileName}: this is the old string value => this is the new string value`, - ); - }); - - it('should handle very short strings correctly in the description', () => { - const testFileName = 'short.txt'; - const params: EditToolParams = { - file_path: path.join(rootDir, testFileName), - old_string: 'old', - new_string: 'new', - }; - const invocation = tool.build(params); - expect(invocation.getDescription()).toBe(`${testFileName}: old => new`); - }); - - it('should truncate long strings in the description', () => { - const testFileName = 'long.txt'; - const params: EditToolParams = { - file_path: path.join(rootDir, testFileName), - old_string: + it.each([ + { + name: 'identical strings (no change)', + fileName: 'test.txt', + oldStr: 'identical_string', + newStr: 'identical_string', + expected: 'No file changes to test.txt', + }, + { + name: 'different strings (full)', + fileName: 'test.txt', + oldStr: 'this is the old string value', + newStr: 'this is the new string value', + expected: + 'test.txt: this is the old string value => this is the new string value', + }, + { + name: 'very short strings', + fileName: 'short.txt', + oldStr: 'old', + newStr: 'new', + expected: 'short.txt: old => new', + }, + { + name: 'long strings (truncated)', + fileName: 'long.txt', + oldStr: 'this is a very long old string that will definitely be truncated', - new_string: - 'this is a very long new string that will also be truncated', + newStr: 'this is a very long new string that will also be truncated', + expected: + 'long.txt: this is a very long old string... => this is a very long new string...', + }, + ])('should handle $name', ({ fileName, oldStr, newStr, expected }) => { + const params: EditToolParams = { + file_path: path.join(rootDir, fileName), + old_string: oldStr, + new_string: newStr, }; const invocation = tool.build(params); - expect(invocation.getDescription()).toBe( - `${testFileName}: this is a very long old string... => this is a very long new string...`, - ); + expect(invocation.getDescription()).toBe(expected); }); }); diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts index 680fa92998..9c40f69a7e 100644 --- a/packages/core/src/tools/mcp-tool.test.ts +++ b/packages/core/src/tools/mcp-tool.test.ts @@ -25,6 +25,18 @@ const mockCallableToolInstance: Mocked = { // Add other methods if DiscoveredMCPTool starts using them }; +const createSdkResponse = ( + toolName: string, + response: Record, +): Part[] => [ + { + functionResponse: { + name: toolName, + response, + }, + }, +]; + describe('generateValidName', () => { it('should return a valid name for a simple function', () => { expect(generateValidName('myFunction')).toBe('myFunction'); @@ -46,17 +58,16 @@ describe('generateValidName', () => { expect(generateValidName('!@#$%^&*()')).toBe('__________'); }); - it('should handle names that are exactly 63 characters long', () => { - expect(generateValidName('a'.repeat(63)).length).toBe(63); - }); - - it('should handle names that are exactly 64 characters long', () => { - expect(generateValidName('a'.repeat(64)).length).toBe(63); - }); - - it('should handle names that are longer than 64 characters', () => { - expect(generateValidName('a'.repeat(80)).length).toBe(63); - }); + it.each([ + { length: 63, expected: 63, description: 'exactly 63 characters' }, + { length: 64, expected: 63, description: 'exactly 64 characters' }, + { length: 80, expected: 63, description: 'longer than 64 characters' }, + ])( + 'should handle names that are $description long', + ({ length, expected }) => { + expect(generateValidName('a'.repeat(length)).length).toBe(expected); + }, + ); }); describe('DiscoveredMCPTool', () => { @@ -270,20 +281,11 @@ describe('DiscoveredMCPTool', () => { const params = { param: 'test' }; const successMessage = 'This is a success message.'; - // Simulate the response from the GenAI SDK, which wraps the MCP - // response in a functionResponse Part. - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - // The `content` array contains MCP ContentBlocks. - content: [{ type: 'text', text: successMessage }], - }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [{ type: 'text', text: successMessage }], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -302,23 +304,17 @@ describe('DiscoveredMCPTool', () => { it('should handle an AudioBlock response', async () => { const params = { param: 'play' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { - type: 'audio', - data: 'BASE64_AUDIO_DATA', - mimeType: 'audio/mp3', - }, - ], + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { + type: 'audio', + data: 'BASE64_AUDIO_DATA', + mimeType: 'audio/mp3', }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -339,24 +335,18 @@ describe('DiscoveredMCPTool', () => { it('should handle a ResourceLinkBlock response', async () => { const params = { param: 'get' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { - type: 'resource_link', - uri: 'file:///path/to/thing', - name: 'resource-name', - title: 'My Resource', - }, - ], + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { + type: 'resource_link', + uri: 'file:///path/to/thing', + name: 'resource-name', + title: 'My Resource', }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -373,26 +363,20 @@ describe('DiscoveredMCPTool', () => { it('should handle an embedded text ResourceBlock response', async () => { const params = { param: 'get' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { - type: 'resource', - resource: { - uri: 'file:///path/to/text.txt', - text: 'This is the text content.', - mimeType: 'text/plain', - }, - }, - ], + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { + type: 'resource', + resource: { + uri: 'file:///path/to/text.txt', + text: 'This is the text content.', + mimeType: 'text/plain', + }, }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -405,26 +389,20 @@ describe('DiscoveredMCPTool', () => { it('should handle an embedded binary ResourceBlock response', async () => { const params = { param: 'get' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { - type: 'resource', - resource: { - uri: 'file:///path/to/data.bin', - blob: 'BASE64_BINARY_DATA', - mimeType: 'application/octet-stream', - }, - }, - ], + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { + type: 'resource', + resource: { + uri: 'file:///path/to/data.bin', + blob: 'BASE64_BINARY_DATA', + mimeType: 'application/octet-stream', + }, }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -447,25 +425,19 @@ describe('DiscoveredMCPTool', () => { it('should handle a mix of content block types', async () => { const params = { param: 'complex' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { type: 'text', text: 'First part.' }, - { - type: 'image', - data: 'BASE64_IMAGE_DATA', - mimeType: 'image/jpeg', - }, - { type: 'text', text: 'Second part.' }, - ], + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { type: 'text', text: 'First part.' }, + { + type: 'image', + data: 'BASE64_IMAGE_DATA', + mimeType: 'image/jpeg', }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + { type: 'text', text: 'Second part.' }, + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -490,20 +462,14 @@ describe('DiscoveredMCPTool', () => { it('should ignore unknown content block types', async () => { const params = { param: 'test' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { type: 'text', text: 'Valid part.' }, - { type: 'future_block', data: 'some-data' }, - ], - }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { type: 'text', text: 'Valid part.' }, + { type: 'future_block', data: 'some-data' }, + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -516,38 +482,32 @@ describe('DiscoveredMCPTool', () => { it('should handle a complex mix of content block types', async () => { const params = { param: 'super-complex' }; - const sdkResponse: Part[] = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [ - { type: 'text', text: 'Here is a resource.' }, - { - type: 'resource_link', - uri: 'file:///path/to/resource', - name: 'resource-name', - title: 'My Resource', - }, - { - type: 'resource', - resource: { - uri: 'file:///path/to/text.txt', - text: 'Embedded text content.', - mimeType: 'text/plain', - }, - }, - { - type: 'image', - data: 'BASE64_IMAGE_DATA', - mimeType: 'image/jpeg', - }, - ], + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [ + { type: 'text', text: 'Here is a resource.' }, + { + type: 'resource_link', + uri: 'file:///path/to/resource', + name: 'resource-name', + title: 'My Resource', }, - }, - }, - ]; - mockCallTool.mockResolvedValue(sdkResponse); + { + type: 'resource', + resource: { + uri: 'file:///path/to/text.txt', + text: 'Embedded text content.', + mimeType: 'text/plain', + }, + }, + { + type: 'image', + data: 'BASE64_IMAGE_DATA', + mimeType: 'image/jpeg', + }, + ], + }), + ); const invocation = tool.build(params); const toolResult = await invocation.execute(new AbortController().signal); @@ -574,6 +534,9 @@ describe('DiscoveredMCPTool', () => { }); describe('AbortSignal support', () => { + const MOCK_TOOL_DELAY = 1000; + const ABORT_DELAY = 50; + it('should abort immediately if signal is already aborted', async () => { const params = { param: 'test' }; const controller = new AbortController(); @@ -608,7 +571,7 @@ describe('DiscoveredMCPTool', () => { }, }, ]); - }, 1000); + }, MOCK_TOOL_DELAY); }), ); @@ -616,7 +579,7 @@ describe('DiscoveredMCPTool', () => { const promise = invocation.execute(controller.signal); // Abort after a short delay to simulate cancellation during execution - setTimeout(() => controller.abort(), 50); + setTimeout(() => controller.abort(), ABORT_DELAY); await expect(promise).rejects.toThrow('Tool call aborted'); }); @@ -624,18 +587,12 @@ describe('DiscoveredMCPTool', () => { it('should complete successfully if not aborted', async () => { const params = { param: 'test' }; const controller = new AbortController(); - const successResponse = [ - { - functionResponse: { - name: serverToolName, - response: { - content: [{ type: 'text', text: 'Success' }], - }, - }, - }, - ]; - mockCallTool.mockResolvedValue(successResponse); + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { + content: [{ type: 'text', text: 'Success' }], + }), + ); const invocation = tool.build(params); const result = await invocation.execute(controller.signal); @@ -650,16 +607,10 @@ describe('DiscoveredMCPTool', () => { it('should handle tool error even when abort signal is provided', async () => { const params = { param: 'test' }; const controller = new AbortController(); - const errorResponse = [ - { - functionResponse: { - name: serverToolName, - response: { error: { isError: true } }, - }, - }, - ]; - mockCallTool.mockResolvedValue(errorResponse); + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { error: { isError: true } }), + ); const invocation = tool.build(params); const result = await invocation.execute(controller.signal); @@ -684,48 +635,50 @@ describe('DiscoveredMCPTool', () => { ); }); - it('should cleanup event listeners properly on successful completion', async () => { - const params = { param: 'test' }; - const controller = new AbortController(); - const successResponse = [ - { - functionResponse: { - name: serverToolName, - response: { + it.each([ + { + name: 'successful completion', + setup: () => { + mockCallTool.mockResolvedValue( + createSdkResponse(serverToolName, { content: [{ type: 'text', text: 'Success' }], - }, - }, + }), + ); }, - ]; + expectError: false, + }, + { + name: 'error', + setup: () => { + mockCallTool.mockRejectedValue(new Error('Tool execution failed')); + }, + expectError: true, + }, + ])( + 'should cleanup event listeners properly on $name', + async ({ setup, expectError }) => { + const params = { param: 'test' }; + const controller = new AbortController(); - mockCallTool.mockResolvedValue(successResponse); + setup(); - const invocation = tool.build(params); - await invocation.execute(controller.signal); + const invocation = tool.build(params); - controller.abort(); - expect(controller.signal.aborted).toBe(true); - }); + if (expectError) { + try { + await invocation.execute(controller.signal); + } catch (_error) { + // Expected error + } + } else { + await invocation.execute(controller.signal); + } - it('should cleanup event listeners properly on error', async () => { - const params = { param: 'test' }; - const controller = new AbortController(); - const expectedError = new Error('Tool execution failed'); - - mockCallTool.mockRejectedValue(expectedError); - - const invocation = tool.build(params); - - try { - await invocation.execute(controller.signal); - } catch (error) { - expect(error).toBe(expectedError); - } - - // Verify cleanup by aborting after error - controller.abort(); - expect(controller.signal.aborted).toBe(true); - }); + // Verify cleanup by aborting after execution + controller.abort(); + expect(controller.signal.aborted).toBe(true); + }, + ); }); }); @@ -787,106 +740,61 @@ describe('DiscoveredMCPTool', () => { } }); - it('should add server to allowlist on ProceedAlwaysServer', async () => { - const invocation = tool.build({ param: 'mock' }) as any; - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).not.toBe(false); - if ( - confirmation && - typeof confirmation === 'object' && - 'onConfirm' in confirmation && - typeof confirmation.onConfirm === 'function' - ) { - await confirmation.onConfirm( - ToolConfirmationOutcome.ProceedAlwaysServer, + it.each([ + { + outcome: ToolConfirmationOutcome.ProceedAlwaysServer, + description: 'add server to allowlist on ProceedAlwaysServer', + shouldAddServer: true, + shouldAddTool: false, + }, + { + outcome: ToolConfirmationOutcome.ProceedAlwaysTool, + description: 'add tool to allowlist on ProceedAlwaysTool', + shouldAddServer: false, + shouldAddTool: true, + }, + { + outcome: ToolConfirmationOutcome.Cancel, + description: 'handle Cancel confirmation outcome', + shouldAddServer: false, + shouldAddTool: false, + }, + { + outcome: ToolConfirmationOutcome.ProceedOnce, + description: 'handle ProceedOnce confirmation outcome', + shouldAddServer: false, + shouldAddTool: false, + }, + ])( + 'should $description', + async ({ outcome, shouldAddServer, shouldAddTool }) => { + const toolAllowlistKey = `${serverName}.${serverToolName}`; + const invocation = tool.build({ param: 'mock' }) as any; + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, ); - expect(invocation.constructor.allowlist.has(serverName)).toBe(true); - } else { - throw new Error( - 'Confirmation details or onConfirm not in expected format', - ); - } - }); - it('should add tool to allowlist on ProceedAlwaysTool', async () => { - const toolAllowlistKey = `${serverName}.${serverToolName}`; - const invocation = tool.build({ param: 'mock' }) as any; - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).not.toBe(false); - if ( - confirmation && - typeof confirmation === 'object' && - 'onConfirm' in confirmation && - typeof confirmation.onConfirm === 'function' - ) { - await confirmation.onConfirm(ToolConfirmationOutcome.ProceedAlwaysTool); - expect(invocation.constructor.allowlist.has(toolAllowlistKey)).toBe( - true, - ); - } else { - throw new Error( - 'Confirmation details or onConfirm not in expected format', - ); - } - }); - - it('should handle Cancel confirmation outcome', async () => { - const invocation = tool.build({ param: 'mock' }) as any; - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).not.toBe(false); - if ( - confirmation && - typeof confirmation === 'object' && - 'onConfirm' in confirmation && - typeof confirmation.onConfirm === 'function' - ) { - // Cancel should not add anything to allowlist - await confirmation.onConfirm(ToolConfirmationOutcome.Cancel); - expect(invocation.constructor.allowlist.has(serverName)).toBe(false); - expect( - invocation.constructor.allowlist.has( - `${serverName}.${serverToolName}`, - ), - ).toBe(false); - } else { - throw new Error( - 'Confirmation details or onConfirm not in expected format', - ); - } - }); - - it('should handle ProceedOnce confirmation outcome', async () => { - const invocation = tool.build({ param: 'mock' }) as any; - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).not.toBe(false); - if ( - confirmation && - typeof confirmation === 'object' && - 'onConfirm' in confirmation && - typeof confirmation.onConfirm === 'function' - ) { - // ProceedOnce should not add anything to allowlist - await confirmation.onConfirm(ToolConfirmationOutcome.ProceedOnce); - expect(invocation.constructor.allowlist.has(serverName)).toBe(false); - expect( - invocation.constructor.allowlist.has( - `${serverName}.${serverToolName}`, - ), - ).toBe(false); - } else { - throw new Error( - 'Confirmation details or onConfirm not in expected format', - ); - } - }); + expect(confirmation).not.toBe(false); + if ( + confirmation && + typeof confirmation === 'object' && + 'onConfirm' in confirmation && + typeof confirmation.onConfirm === 'function' + ) { + await confirmation.onConfirm(outcome); + expect(invocation.constructor.allowlist.has(serverName)).toBe( + shouldAddServer, + ); + expect(invocation.constructor.allowlist.has(toolAllowlistKey)).toBe( + shouldAddTool, + ); + } else { + throw new Error( + 'Confirmation details or onConfirm not in expected format', + ); + } + }, + ); }); describe('shouldConfirmExecute with folder trust', () => { @@ -894,59 +802,49 @@ describe('DiscoveredMCPTool', () => { isTrustedFolder: () => isTrusted, }); - it('should return false if trust is true and folder is trusted', async () => { - const trustedTool = new DiscoveredMCPTool( + it.each([ + { + trust: true, + isTrusted: true, + shouldConfirm: false, + description: 'return false if trust is true and folder is trusted', + }, + { + trust: true, + isTrusted: false, + shouldConfirm: true, + description: + 'return confirmation details if trust is true but folder is not trusted', + }, + { + trust: false, + isTrusted: true, + shouldConfirm: true, + description: + 'return confirmation details if trust is false, even if folder is trusted', + }, + ])('should $description', async ({ trust, isTrusted, shouldConfirm }) => { + const testTool = new DiscoveredMCPTool( mockCallableToolInstance, serverName, serverToolName, baseDescription, inputSchema, - true, // trust = true + trust, undefined, - mockConfig(true) as any, // isTrustedFolder = true + mockConfig(isTrusted) as any, ); - const invocation = trustedTool.build({ param: 'mock' }); - expect( - await invocation.shouldConfirmExecute(new AbortController().signal), - ).toBe(false); - }); - - it('should return confirmation details if trust is true but folder is not trusted', async () => { - const trustedTool = new DiscoveredMCPTool( - mockCallableToolInstance, - serverName, - serverToolName, - baseDescription, - inputSchema, - true, // trust = true - undefined, - mockConfig(false) as any, // isTrustedFolder = false - ); - const invocation = trustedTool.build({ param: 'mock' }); + const invocation = testTool.build({ param: 'mock' }); const confirmation = await invocation.shouldConfirmExecute( new AbortController().signal, ); - expect(confirmation).not.toBe(false); - expect(confirmation).toHaveProperty('type', 'mcp'); - }); - it('should return confirmation details if trust is false, even if folder is trusted', async () => { - const untrustedTool = new DiscoveredMCPTool( - mockCallableToolInstance, - serverName, - serverToolName, - baseDescription, - inputSchema, - false, // trust = false - undefined, - mockConfig(true) as any, // isTrustedFolder = true - ); - const invocation = untrustedTool.build({ param: 'mock' }); - const confirmation = await invocation.shouldConfirmExecute( - new AbortController().signal, - ); - expect(confirmation).not.toBe(false); - expect(confirmation).toHaveProperty('type', 'mcp'); + if (shouldConfirm) { + expect(confirmation).not.toBe(false); + expect(confirmation).toHaveProperty('type', 'mcp'); + } else { + expect(confirmation).toBe(false); + } }); }); diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index e3c3ec6948..24b2de35bb 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -39,6 +39,10 @@ const downloadRipGrepMock = vi.mocked(downloadRipGrep); const originalGetGlobalBinDir = Storage.getGlobalBinDir.bind(Storage); const storageSpy = vi.spyOn(Storage, 'getGlobalBinDir'); +function getRipgrepBinaryName() { + return process.platform === 'win32' ? 'rg.exe' : 'rg'; +} + describe('canUseRipgrep', () => { let tempRootDir: string; let binDir: string; @@ -58,9 +62,7 @@ describe('canUseRipgrep', () => { }); it('should return true if ripgrep already exists', async () => { - const candidateNames = - process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; - const existingPath = path.join(binDir, candidateNames[0]); + const existingPath = path.join(binDir, getRipgrepBinaryName()); await fs.writeFile(existingPath, ''); const result = await canUseRipgrep(); @@ -69,9 +71,7 @@ describe('canUseRipgrep', () => { }); it('should download ripgrep and return true if it does not exist initially', async () => { - const candidateNames = - process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; - const expectedPath = path.join(binDir, candidateNames[0]); + const expectedPath = path.join(binDir, getRipgrepBinaryName()); downloadRipGrepMock.mockImplementation(async () => { await fs.writeFile(expectedPath, ''); @@ -100,9 +100,7 @@ describe('canUseRipgrep', () => { }); it('should only download once when called concurrently', async () => { - const candidateNames = - process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; - const expectedPath = path.join(binDir, candidateNames[0]); + const expectedPath = path.join(binDir, getRipgrepBinaryName()); downloadRipGrepMock.mockImplementation( () => @@ -146,9 +144,7 @@ describe('ensureRgPath', () => { }); it('should return rg path if ripgrep already exists', async () => { - const candidateNames = - process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; - const existingPath = path.join(binDir, candidateNames[0]); + const existingPath = path.join(binDir, getRipgrepBinaryName()); await fs.writeFile(existingPath, ''); const rgPath = await ensureRgPath(); @@ -157,9 +153,7 @@ describe('ensureRgPath', () => { }); it('should return rg path if ripgrep is downloaded successfully', async () => { - const candidateNames = - process.platform === 'win32' ? ['rg.exe', 'rg'] : ['rg']; - const expectedPath = path.join(binDir, candidateNames[0]); + const expectedPath = path.join(binDir, getRipgrepBinaryName()); downloadRipGrepMock.mockImplementation(async () => { await fs.writeFile(expectedPath, ''); @@ -301,24 +295,33 @@ describe('RipGrepTool', () => { }); describe('validateToolParams', () => { - it('should return null for valid params (pattern only)', () => { - const params: RipGrepToolParams = { pattern: 'hello' }; - expect(grepTool.validateToolParams(params)).toBeNull(); - }); - - it('should return null for valid params (pattern and path)', () => { - const params: RipGrepToolParams = { pattern: 'hello', dir_path: '.' }; - expect(grepTool.validateToolParams(params)).toBeNull(); - }); - - it('should return null for valid params (pattern, path, and include)', () => { - const params: RipGrepToolParams = { - pattern: 'hello', - dir_path: '.', - include: '*.txt', - }; - expect(grepTool.validateToolParams(params)).toBeNull(); - }); + it.each([ + { + name: 'pattern only', + params: { pattern: 'hello' }, + expected: null, + }, + { + name: 'pattern and path', + params: { pattern: 'hello', dir_path: '.' }, + expected: null, + }, + { + name: 'pattern, path, and include', + params: { pattern: 'hello', dir_path: '.', include: '*.txt' }, + expected: null, + }, + { + name: 'invalid regex pattern', + params: { pattern: '[[' }, + expected: null, + }, + ])( + 'should return null for valid params ($name)', + ({ params, expected }) => { + expect(grepTool.validateToolParams(params)).toBe(expected); + }, + ); it('should return error if pattern is missing', () => { const params = { dir_path: '.' } as unknown as RipGrepToolParams; @@ -327,11 +330,6 @@ describe('RipGrepTool', () => { ); }); - it('should return null for what would be an invalid regex pattern', () => { - const params: RipGrepToolParams = { pattern: '[[' }; - expect(grepTool.validateToolParams(params)).toBeNull(); - }); - it('should return error if path does not exist', () => { const params: RipGrepToolParams = { pattern: 'hello', @@ -1018,80 +1016,26 @@ describe('RipGrepTool', () => { expect(() => grepTool.build(params)).toThrow(/Path validation failed/); }); - it('should handle empty directories gracefully', async () => { - const emptyDir = path.join(tempRootDir, 'empty'); - await fs.mkdir(emptyDir); + it.each([ + { + name: 'empty directories', + setup: async () => { + const emptyDir = path.join(tempRootDir, 'empty'); + await fs.mkdir(emptyDir); + return { pattern: 'test', dir_path: 'empty' }; + }, + }, + { + name: 'empty files', + setup: async () => { + await fs.writeFile(path.join(tempRootDir, 'empty.txt'), ''); + return { pattern: 'anything' }; + }, + }, + ])('should handle $name gracefully', async ({ setup }) => { + mockSpawn.mockImplementationOnce(createMockSpawn({ exitCode: 1 })); - // Setup specific mock for this test - searching in empty directory should return no matches - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onClose) { - onClose(1); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); - - const params: RipGrepToolParams = { pattern: 'test', dir_path: 'empty' }; - const invocation = grepTool.build(params); - const result = await invocation.execute(abortSignal); - - expect(result.llmContent).toContain('No matches found'); - expect(result.returnDisplay).toBe('No matches found'); - }); - - it('should handle empty files correctly', async () => { - await fs.writeFile(path.join(tempRootDir, 'empty.txt'), ''); - - // Setup specific mock for this test - searching for anything in empty files should return no matches - mockSpawn.mockImplementationOnce(() => { - const mockProcess = { - stdout: { - on: vi.fn(), - removeListener: vi.fn(), - }, - stderr: { - on: vi.fn(), - removeListener: vi.fn(), - }, - on: vi.fn(), - removeListener: vi.fn(), - kill: vi.fn(), - }; - - setTimeout(() => { - const onClose = mockProcess.on.mock.calls.find( - (call) => call[0] === 'close', - )?.[1]; - - if (onClose) { - onClose(1); - } - }, 0); - - return mockProcess as unknown as ChildProcess; - }); - - const params: RipGrepToolParams = { pattern: 'anything' }; + const params = await setup(); const invocation = grepTool.build(params); const result = await invocation.execute(abortSignal); @@ -1627,38 +1571,42 @@ describe('RipGrepTool', () => { expect(result.llmContent).toContain('L1: HELLO world'); }); - it('should handle fixed_strings parameter', async () => { - mockSpawn.mockImplementationOnce( - createMockSpawn({ - outputData: - JSON.stringify({ - type: 'match', - data: { - path: { text: 'fileA.txt' }, - line_number: 1, - lines: { text: 'hello.world\n' }, - }, - }) + '\n', - exitCode: 0, - }), - ); + it.each([ + { + name: 'fixed_strings parameter', + params: { pattern: 'hello.world', fixed_strings: true }, + mockOutput: { + path: { text: 'fileA.txt' }, + line_number: 1, + lines: { text: 'hello.world\n' }, + }, + expectedArgs: ['--fixed-strings'], + expectedPattern: 'hello.world', + }, + ])( + 'should handle $name', + async ({ params, mockOutput, expectedArgs, expectedPattern }) => { + mockSpawn.mockImplementationOnce( + createMockSpawn({ + outputData: + JSON.stringify({ type: 'match', data: mockOutput }) + '\n', + exitCode: 0, + }), + ); - const params: RipGrepToolParams = { - pattern: 'hello.world', - fixed_strings: true, - }; - const invocation = grepTool.build(params); - const result = await invocation.execute(abortSignal); - expect(mockSpawn).toHaveBeenLastCalledWith( - expect.anything(), - expect.arrayContaining(['--fixed-strings']), - expect.anything(), - ); - expect(result.llmContent).toContain( - 'Found 1 match for pattern "hello.world"', - ); - expect(result.llmContent).toContain('L1: hello.world'); - }); + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(mockSpawn).toHaveBeenLastCalledWith( + expect.anything(), + expect.arrayContaining(expectedArgs), + expect.anything(), + ); + expect(result.llmContent).toContain( + `Found 1 match for pattern "${expectedPattern}"`, + ); + }, + ); it('should handle no_ignore parameter', async () => { mockSpawn.mockImplementationOnce( @@ -1680,14 +1628,12 @@ describe('RipGrepTool', () => { const invocation = grepTool.build(params); const result = await invocation.execute(abortSignal); - // Should have --no-ignore expect(mockSpawn).toHaveBeenLastCalledWith( expect.anything(), expect.arrayContaining(['--no-ignore']), expect.anything(), ); - // Should NOT have default excludes when no_ignore is true expect(mockSpawn).toHaveBeenLastCalledWith( expect.anything(), expect.not.arrayContaining(['--glob', '!node_modules']), @@ -1749,22 +1695,29 @@ describe('RipGrepTool', () => { }); describe('getDescription', () => { - it('should generate correct description with pattern only', () => { - const params: RipGrepToolParams = { pattern: 'testPattern' }; - const invocation = grepTool.build(params); - expect(invocation.getDescription()).toBe("'testPattern' within ./"); - }); - - it('should generate correct description with pattern and include', () => { - const params: RipGrepToolParams = { - pattern: 'testPattern', - include: '*.ts', - }; - const invocation = grepTool.build(params); - expect(invocation.getDescription()).toBe( - "'testPattern' in *.ts within ./", - ); - }); + it.each([ + { + name: 'pattern only', + params: { pattern: 'testPattern' }, + expected: "'testPattern' within ./", + }, + { + name: 'pattern and include', + params: { pattern: 'testPattern', include: '*.ts' }, + expected: "'testPattern' in *.ts within ./", + }, + { + name: 'root path in description', + params: { pattern: 'testPattern', dir_path: '.' }, + expected: "'testPattern' within ./", + }, + ])( + 'should generate correct description with $name', + ({ params, expected }) => { + const invocation = grepTool.build(params); + expect(invocation.getDescription()).toBe(expected); + }, + ); it('should generate correct description with pattern and path', async () => { const dirPath = path.join(tempRootDir, 'src', 'app'); @@ -1774,13 +1727,11 @@ describe('RipGrepTool', () => { dir_path: path.join('src', 'app'), }; const invocation = grepTool.build(params); - // The path will be relative to the tempRootDir, so we check for containment. expect(invocation.getDescription()).toContain("'testPattern' within"); expect(invocation.getDescription()).toContain(path.join('src', 'app')); }); it('should use ./ when no path is specified (defaults to CWD)', () => { - // Create a mock config with multiple directories const multiDirConfig = { getTargetDir: () => tempRootDir, getWorkspaceContext: () => @@ -1808,15 +1759,6 @@ describe('RipGrepTool', () => { ); expect(invocation.getDescription()).toContain(path.join('src', 'app')); }); - - it('should use ./ for root path in description', () => { - const params: RipGrepToolParams = { - pattern: 'testPattern', - dir_path: '.', - }; - const invocation = grepTool.build(params); - expect(invocation.getDescription()).toBe("'testPattern' within ./"); - }); }); }); afterAll(() => {