diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 7af09352..905c5776 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -9,10 +9,14 @@ import { partListUnionToString } from '../core/geminiRequest.js'; import path from 'path'; import fs from 'fs/promises'; import os from 'os'; -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { Config } from '../config/config.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; +import { ToolErrorType } from './tool-error.js'; +import * as glob from 'glob'; + +vi.mock('glob', { spy: true }); describe('GlobTool', () => { let tempRootDir: string; // This will be the rootDirectory for the GlobTool instance @@ -203,6 +207,29 @@ describe('GlobTool', () => { path.resolve(tempRootDir, 'older.sortme'), ); }); + + it('should return a PATH_NOT_IN_WORKSPACE error if path is outside workspace', async () => { + // Bypassing validation to test execute method directly + vi.spyOn(globTool, 'validateToolParams').mockReturnValue(null); + const params: GlobToolParams = { pattern: '*.txt', path: '/etc' }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.PATH_NOT_IN_WORKSPACE); + expect(result.returnDisplay).toBe('Path is not within workspace'); + }); + + it('should return a GLOB_EXECUTION_ERROR on glob failure', async () => { + vi.mocked(glob.glob).mockRejectedValue(new Error('Glob failed')); + const params: GlobToolParams = { pattern: '*.txt' }; + const invocation = globTool.build(params); + const result = await invocation.execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.GLOB_EXECUTION_ERROR); + expect(result.llmContent).toContain( + 'Error during glob search operation: Glob failed', + ); + // Reset glob. + vi.mocked(glob.glob).mockReset(); + }); }); describe('validateToolParams', () => { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 60186758..2bd26fbd 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -16,6 +16,7 @@ import { } from './tools.js'; import { shortenPath, makeRelative } from '../utils/paths.js'; import { Config } from '../config/config.js'; +import { ToolErrorType } from './tool-error.js'; // Subset of 'Path' interface provided by 'glob' that we can implement for testing export interface GlobPath { @@ -115,9 +116,14 @@ class GlobToolInvocation extends BaseToolInvocation< this.params.path, ); if (!workspaceContext.isPathWithinWorkspace(searchDirAbsolute)) { + const rawError = `Error: Path "${this.params.path}" is not within any workspace directory`; return { - llmContent: `Error: Path "${this.params.path}" is not within any workspace directory`, + llmContent: rawError, returnDisplay: `Path is not within workspace`, + error: { + message: rawError, + type: ToolErrorType.PATH_NOT_IN_WORKSPACE, + }, }; } searchDirectories = [searchDirAbsolute]; @@ -234,9 +240,14 @@ class GlobToolInvocation extends BaseToolInvocation< const errorMessage = error instanceof Error ? error.message : String(error); console.error(`GlobLogic execute Error: ${errorMessage}`, error); + const rawError = `Error during glob search operation: ${errorMessage}`; return { - llmContent: `Error during glob search operation: ${errorMessage}`, + llmContent: rawError, returnDisplay: `Error: An unexpected error occurred.`, + error: { + message: rawError, + type: ToolErrorType.GLOB_EXECUTION_ERROR, + }, }; } } diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 4bb59115..a152c88c 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -11,6 +11,10 @@ import fs from 'fs/promises'; import os from 'os'; import { Config } from '../config/config.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; +import { ToolErrorType } from './tool-error.js'; +import * as glob from 'glob'; + +vi.mock('glob', { spy: true }); // Mock the child_process module to control grep/git grep behavior vi.mock('child_process', () => ({ @@ -223,6 +227,15 @@ describe('GrepTool', () => { /params must have required property 'pattern'/, ); }); + + it('should return a GREP_EXECUTION_ERROR on failure', async () => { + vi.mocked(glob.globStream).mockRejectedValue(new Error('Glob failed')); + const params: GrepToolParams = { pattern: 'hello' }; + const invocation = grepTool.build(params); + const result = await invocation.execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.GREP_EXECUTION_ERROR); + vi.mocked(glob.globStream).mockReset(); + }); }); describe('multi-directory workspace', () => { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 86988c44..4a0b8af4 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -21,6 +21,7 @@ import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { isGitRepository } from '../utils/gitUtils.js'; import { Config } from '../config/config.js'; +import { ToolErrorType } from './tool-error.js'; // --- Interfaces --- @@ -198,6 +199,10 @@ class GrepToolInvocation extends BaseToolInvocation< return { llmContent: `Error during grep search operation: ${errorMessage}`, returnDisplay: `Error: ${errorMessage}`, + error: { + message: errorMessage, + type: ToolErrorType.GREP_EXECUTION_ERROR, + }, }; } } diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index c0b553e1..088edb53 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -23,6 +23,7 @@ import { LSTool } from './ls.js'; import { Config } from '../config/config.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; +import { ToolErrorType } from './tool-error.js'; describe('LSTool', () => { let lsTool: LSTool; @@ -288,6 +289,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('Path is not a directory'); expect(result.returnDisplay).toBe('Error: Path is not a directory.'); + expect(result.error?.type).toBe(ToolErrorType.PATH_IS_NOT_A_DIRECTORY); }); it('should handle non-existent paths', async () => { @@ -302,6 +304,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('Error listing directory'); expect(result.returnDisplay).toBe('Error: Failed to list directory.'); + expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); it('should sort directories first, then files alphabetically', async () => { @@ -357,6 +360,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('Error listing directory'); expect(result.llmContent).toContain('permission denied'); expect(result.returnDisplay).toBe('Error: Failed to list directory.'); + expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); it('should throw for invalid params at build time', async () => { diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 3cb09f83..89f267d0 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -15,6 +15,7 @@ import { } from './tools.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { Config, DEFAULT_FILE_FILTERING_OPTIONS } from '../config/config.js'; +import { ToolErrorType } from './tool-error.js'; /** * Parameters for the LS tool @@ -114,11 +115,19 @@ class LSToolInvocation extends BaseToolInvocation { } // Helper for consistent error formatting - private errorResult(llmContent: string, returnDisplay: string): ToolResult { + private errorResult( + llmContent: string, + returnDisplay: string, + type: ToolErrorType, + ): ToolResult { return { llmContent, // Keep returnDisplay simpler in core logic returnDisplay: `Error: ${returnDisplay}`, + error: { + message: llmContent, + type, + }, }; } @@ -135,12 +144,14 @@ class LSToolInvocation extends BaseToolInvocation { return this.errorResult( `Error: Directory not found or inaccessible: ${this.params.path}`, `Directory not found or inaccessible.`, + ToolErrorType.FILE_NOT_FOUND, ); } if (!stats.isDirectory()) { return this.errorResult( `Error: Path is not a directory: ${this.params.path}`, `Path is not a directory.`, + ToolErrorType.PATH_IS_NOT_A_DIRECTORY, ); } @@ -253,7 +264,11 @@ class LSToolInvocation extends BaseToolInvocation { }; } catch (error) { const errorMsg = `Error listing directory: ${error instanceof Error ? error.message : String(error)}`; - return this.errorResult(errorMsg, 'Failed to list directory.'); + return this.errorResult( + errorMsg, + 'Failed to list directory.', + ToolErrorType.LS_EXECUTION_ERROR, + ); } } } diff --git a/packages/core/src/tools/mcp-tool.test.ts b/packages/core/src/tools/mcp-tool.test.ts index d5e4eee8..dc1d5c57 100644 --- a/packages/core/src/tools/mcp-tool.test.ts +++ b/packages/core/src/tools/mcp-tool.test.ts @@ -17,6 +17,7 @@ import { import { DiscoveredMCPTool, generateValidName } from './mcp-tool.js'; // Added getStringifiedResultForDisplay import { ToolResult, ToolConfirmationOutcome } from './tools.js'; // Added ToolConfirmationOutcome import { CallableTool, Part } from '@google/genai'; +import { ToolErrorType } from './tool-error.js'; // Mock @google/genai mcpToTool and CallableTool // We only need to mock the parts of CallableTool that DiscoveredMCPTool uses. @@ -189,7 +190,7 @@ describe('DiscoveredMCPTool', () => { { isErrorValue: true, description: 'true (bool)' }, { isErrorValue: 'true', description: '"true" (str)' }, ])( - 'should consider a ToolResult with isError $description to be a failure', + 'should return a structured error if MCP tool reports an error', async ({ isErrorValue }) => { const tool = new DiscoveredMCPTool( mockCallableToolInstance, @@ -210,16 +211,18 @@ describe('DiscoveredMCPTool', () => { }, ]; mockCallTool.mockResolvedValue(mockMcpToolResponseParts); - const expectedError = new Error( - `MCP tool '${serverToolName}' reported tool error with response: ${JSON.stringify( - mockMcpToolResponseParts, - )}`, - ); + const expectedErrorMessage = `MCP tool '${serverToolName}' reported tool error with response: ${JSON.stringify( + mockMcpToolResponseParts, + )}`; const invocation = tool.build(params); - await expect( - invocation.execute(new AbortController().signal), - ).rejects.toThrow(expectedError); + const result = await invocation.execute(new AbortController().signal); + + expect(result.error?.type).toBe(ToolErrorType.MCP_TOOL_ERROR); + expect(result.llmContent).toBe(expectedErrorMessage); + expect(result.returnDisplay).toContain( + `Error: MCP tool '${serverToolName}' reported an error.`, + ); }, ); diff --git a/packages/core/src/tools/mcp-tool.ts b/packages/core/src/tools/mcp-tool.ts index baa517f6..6ad1da78 100644 --- a/packages/core/src/tools/mcp-tool.ts +++ b/packages/core/src/tools/mcp-tool.ts @@ -16,6 +16,7 @@ import { ToolResult, } from './tools.js'; import { CallableTool, FunctionCall, Part } from '@google/genai'; +import { ToolErrorType } from './tool-error.js'; type ToolParams = Record; @@ -139,9 +140,19 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation< // Ensure the response is not an error if (this.isMCPToolError(rawResponseParts)) { - throw new Error( - `MCP tool '${this.serverToolName}' reported tool error with response: ${JSON.stringify(rawResponseParts)}`, - ); + const errorMessage = `MCP tool '${ + this.serverToolName + }' reported tool error with response: ${JSON.stringify( + rawResponseParts, + )}`; + return { + llmContent: errorMessage, + returnDisplay: `Error: MCP tool '${this.serverToolName}' reported an error.`, + error: { + message: errorMessage, + type: ToolErrorType.MCP_TOOL_ERROR, + }, + }; } const transformedParts = transformMcpContentToParts(rawResponseParts); diff --git a/packages/core/src/tools/memoryTool.test.ts b/packages/core/src/tools/memoryTool.test.ts index dfcdd300..b4a055c8 100644 --- a/packages/core/src/tools/memoryTool.test.ts +++ b/packages/core/src/tools/memoryTool.test.ts @@ -16,6 +16,7 @@ import * as fs from 'fs/promises'; import * as path from 'path'; import * as os from 'os'; import { ToolConfirmationOutcome } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; // Mock dependencies vi.mock(import('fs/promises'), async (importOriginal) => { @@ -287,6 +288,9 @@ describe('MemoryTool', () => { expect(result.returnDisplay).toBe( `Error saving memory: ${underlyingError.message}`, ); + expect(result.error?.type).toBe( + ToolErrorType.MEMORY_TOOL_EXECUTION_ERROR, + ); }); }); diff --git a/packages/core/src/tools/memoryTool.ts b/packages/core/src/tools/memoryTool.ts index 77d84216..70bf6adc 100644 --- a/packages/core/src/tools/memoryTool.ts +++ b/packages/core/src/tools/memoryTool.ts @@ -20,6 +20,7 @@ import * as Diff from 'diff'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { tildeifyPath } from '../utils/paths.js'; import { ModifiableDeclarativeTool, ModifyContext } from './modifiable-tool.js'; +import { ToolErrorType } from './tool-error.js'; const memoryToolSchemaData: FunctionDeclaration = { name: 'save_memory', @@ -273,6 +274,10 @@ class MemoryToolInvocation extends BaseToolInvocation< error: `Failed to save memory. Detail: ${errorMessage}`, }), returnDisplay: `Error saving memory: ${errorMessage}`, + error: { + message: errorMessage, + type: ToolErrorType.MEMORY_TOOL_EXECUTION_ERROR, + }, }; } } diff --git a/packages/core/src/tools/read-many-files.test.ts b/packages/core/src/tools/read-many-files.test.ts index b432998d..0e55f5c0 100644 --- a/packages/core/src/tools/read-many-files.test.ts +++ b/packages/core/src/tools/read-many-files.test.ts @@ -15,6 +15,10 @@ import os from 'os'; import { Config } from '../config/config.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; import { StandardFileSystemService } from '../services/fileSystemService.js'; +import { ToolErrorType } from './tool-error.js'; +import * as glob from 'glob'; + +vi.mock('glob', { spy: true }); vi.mock('mime-types', () => { const lookup = (filename: string) => { @@ -566,6 +570,28 @@ Content of file[1] }); }); + describe('Error handling', () => { + it('should return an INVALID_TOOL_PARAMS error if no paths are provided', async () => { + const params = { paths: [], include: [] }; + expect(() => { + tool.build(params); + }).toThrow('params/paths must NOT have fewer than 1 items'); + }); + + it('should return a READ_MANY_FILES_SEARCH_ERROR on glob failure', async () => { + vi.mocked(glob.glob).mockRejectedValue(new Error('Glob failed')); + const params = { paths: ['*.txt'] }; + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + expect(result.error?.type).toBe( + ToolErrorType.READ_MANY_FILES_SEARCH_ERROR, + ); + expect(result.llmContent).toBe('Error during file search: Glob failed'); + // Reset glob. + vi.mocked(glob.glob).mockReset(); + }); + }); + describe('Batch Processing', () => { const createMultipleFiles = (count: number, contentPrefix = 'Content') => { const files: string[] = []; diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index b3568cad..06bc2212 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -29,6 +29,7 @@ import { recordFileOperationMetric, FileOperation, } from '../telemetry/metrics.js'; +import { ToolErrorType } from './tool-error.js'; /** * Parameters for the ReadManyFilesTool. @@ -232,13 +233,6 @@ ${finalExclusionPatternsForDescription : [...exclude]; const searchPatterns = [...inputPatterns, ...include]; - if (searchPatterns.length === 0) { - return { - llmContent: 'No search paths or include patterns provided.', - returnDisplay: `## Information\n\nNo search paths or include patterns were specified. Nothing to read or concatenate.`, - }; - } - try { const allEntries = new Set(); const workspaceDirs = this.config.getWorkspaceContext().getDirectories(); @@ -352,9 +346,14 @@ ${finalExclusionPatternsForDescription }); } } catch (error) { + const errorMessage = `Error during file search: ${getErrorMessage(error)}`; return { - llmContent: `Error during file search: ${getErrorMessage(error)}`, + llmContent: errorMessage, returnDisplay: `## File Search Error\n\nAn error occurred while searching for files:\n\`\`\`\n${getErrorMessage(error)}\n\`\`\``, + error: { + message: errorMessage, + type: ToolErrorType.READ_MANY_FILES_SEARCH_ERROR, + }, }; } diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index fc19fc72..95bed0c4 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -24,10 +24,43 @@ export enum ToolErrorType { PERMISSION_DENIED = 'permission_denied', NO_SPACE_LEFT = 'no_space_left', TARGET_IS_DIRECTORY = 'target_is_directory', + PATH_NOT_IN_WORKSPACE = 'path_not_in_workspace', + SEARCH_PATH_NOT_FOUND = 'search_path_not_found', + SEARCH_PATH_NOT_A_DIRECTORY = 'search_path_not_a_directory', // Edit-specific Errors EDIT_PREPARATION_FAILURE = 'edit_preparation_failure', EDIT_NO_OCCURRENCE_FOUND = 'edit_no_occurrence_found', EDIT_EXPECTED_OCCURRENCE_MISMATCH = 'edit_expected_occurrence_mismatch', EDIT_NO_CHANGE = 'edit_no_change', + + // Glob-specific Errors + GLOB_EXECUTION_ERROR = 'glob_execution_error', + + // Grep-specific Errors + GREP_EXECUTION_ERROR = 'grep_execution_error', + + // Ls-specific Errors + LS_EXECUTION_ERROR = 'ls_execution_error', + PATH_IS_NOT_A_DIRECTORY = 'path_is_not_a_directory', + + // MCP-specific Errors + MCP_TOOL_ERROR = 'mcp_tool_error', + + // Memory-specific Errors + MEMORY_TOOL_EXECUTION_ERROR = 'memory_tool_execution_error', + + // ReadManyFiles-specific Errors + READ_MANY_FILES_SEARCH_ERROR = 'read_many_files_search_error', + + // DiscoveredTool-specific Errors + DISCOVERED_TOOL_EXECUTION_ERROR = 'discovered_tool_execution_error', + + // WebFetch-specific Errors + WEB_FETCH_NO_URL_IN_PROMPT = 'web_fetch_no_url_in_prompt', + WEB_FETCH_FALLBACK_FAILED = 'web_fetch_fallback_failed', + WEB_FETCH_PROCESSING_ERROR = 'web_fetch_processing_error', + + // WebSearch-specific Errors + WEB_SEARCH_FAILED = 'web_search_failed', } diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index 6db60377..0084ddff 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -24,6 +24,7 @@ import fs from 'node:fs'; import { MockTool } from '../test-utils/tools.js'; import { McpClientManager } from './mcp-client-manager.js'; +import { ToolErrorType } from './tool-error.js'; vi.mock('node:fs'); @@ -311,6 +312,81 @@ describe('ToolRegistry', () => { }); }); + it('should return a DISCOVERED_TOOL_EXECUTION_ERROR on tool failure', async () => { + const discoveryCommand = 'my-discovery-command'; + mockConfigGetToolDiscoveryCommand.mockReturnValue(discoveryCommand); + vi.spyOn(config, 'getToolCallCommand').mockReturnValue('my-call-command'); + + const toolDeclaration: FunctionDeclaration = { + name: 'failing-tool', + description: 'A tool that fails', + parametersJsonSchema: { + type: 'object', + properties: {}, + }, + }; + + const mockSpawn = vi.mocked(spawn); + // --- Discovery Mock --- + const discoveryProcess = { + stdout: { on: vi.fn(), removeListener: vi.fn() }, + stderr: { on: vi.fn(), removeListener: vi.fn() }, + on: vi.fn(), + }; + mockSpawn.mockReturnValueOnce(discoveryProcess as any); + + discoveryProcess.stdout.on.mockImplementation((event, callback) => { + if (event === 'data') { + callback( + Buffer.from( + JSON.stringify([{ functionDeclarations: [toolDeclaration] }]), + ), + ); + } + }); + discoveryProcess.on.mockImplementation((event, callback) => { + if (event === 'close') { + callback(0); + } + }); + + await toolRegistry.discoverAllTools(); + const discoveredTool = toolRegistry.getTool('failing-tool'); + expect(discoveredTool).toBeDefined(); + + // --- Execution Mock --- + const executionProcess = { + stdout: { on: vi.fn(), removeListener: vi.fn() }, + stderr: { on: vi.fn(), removeListener: vi.fn() }, + stdin: { write: vi.fn(), end: vi.fn() }, + on: vi.fn(), + connected: true, + disconnect: vi.fn(), + removeListener: vi.fn(), + }; + mockSpawn.mockReturnValueOnce(executionProcess as any); + + executionProcess.stderr.on.mockImplementation((event, callback) => { + if (event === 'data') { + callback(Buffer.from('Something went wrong')); + } + }); + executionProcess.on.mockImplementation((event, callback) => { + if (event === 'close') { + callback(1); // Non-zero exit code + } + }); + + const invocation = (discoveredTool as DiscoveredTool).build({}); + const result = await invocation.execute(new AbortController().signal); + + expect(result.error?.type).toBe( + ToolErrorType.DISCOVERED_TOOL_EXECUTION_ERROR, + ); + expect(result.llmContent).toContain('Stderr: Something went wrong'); + expect(result.llmContent).toContain('Exit Code: 1'); + }); + it('should discover tools using MCP servers defined in getMcpServers', async () => { const discoverSpy = vi.spyOn( McpClientManager.prototype, diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 7acd778d..60c9977d 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -20,6 +20,7 @@ import { connectAndDiscover } from './mcp-client.js'; import { McpClientManager } from './mcp-client-manager.js'; import { DiscoveredMCPTool } from './mcp-tool.js'; import { parse } from 'shell-quote'; +import { ToolErrorType } from './tool-error.js'; import { safeJsonStringify } from '../utils/safeJsonStringify.js'; type ToolParams = Record; @@ -106,6 +107,10 @@ class DiscoveredToolInvocation extends BaseToolInvocation< return { llmContent, returnDisplay: llmContent, + error: { + message: llmContent, + type: ToolErrorType.DISCOVERED_TOOL_EXECUTION_ERROR, + }, }; } diff --git a/packages/core/src/tools/web-fetch.test.ts b/packages/core/src/tools/web-fetch.test.ts index b589c139..a42b3851 100644 --- a/packages/core/src/tools/web-fetch.test.ts +++ b/packages/core/src/tools/web-fetch.test.ts @@ -4,17 +4,72 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { WebFetchTool } from './web-fetch.js'; import { Config, ApprovalMode } from '../config/config.js'; import { ToolConfirmationOutcome } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; +import * as fetchUtils from '../utils/fetch.js'; + +const mockGenerateContent = vi.fn(); +const mockGetGeminiClient = vi.fn(() => ({ + generateContent: mockGenerateContent, +})); + +vi.mock('../utils/fetch.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fetchWithTimeout: vi.fn(), + isPrivateIp: vi.fn(), + }; +}); describe('WebFetchTool', () => { - const mockConfig = { - getApprovalMode: vi.fn(), - setApprovalMode: vi.fn(), - getProxy: vi.fn(), - } as unknown as Config; + let mockConfig: Config; + + beforeEach(() => { + vi.resetAllMocks(); + mockConfig = { + getApprovalMode: vi.fn(), + setApprovalMode: vi.fn(), + getProxy: vi.fn(), + getGeminiClient: mockGetGeminiClient, + } as unknown as Config; + }); + + describe('execute', () => { + it('should return WEB_FETCH_NO_URL_IN_PROMPT when no URL is in the prompt for fallback', async () => { + vi.spyOn(fetchUtils, 'isPrivateIp').mockReturnValue(true); + const tool = new WebFetchTool(mockConfig); + const params = { prompt: 'no url here' }; + expect(() => tool.build(params)).toThrow( + "The 'prompt' must contain at least one valid URL (starting with http:// or https://).", + ); + }); + + it('should return WEB_FETCH_FALLBACK_FAILED on fallback fetch failure', async () => { + vi.spyOn(fetchUtils, 'isPrivateIp').mockReturnValue(true); + vi.spyOn(fetchUtils, 'fetchWithTimeout').mockRejectedValue( + new Error('fetch failed'), + ); + const tool = new WebFetchTool(mockConfig); + const params = { prompt: 'fetch https://private.ip' }; + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + expect(result.error?.type).toBe(ToolErrorType.WEB_FETCH_FALLBACK_FAILED); + }); + + it('should return WEB_FETCH_PROCESSING_ERROR on general processing failure', async () => { + vi.spyOn(fetchUtils, 'isPrivateIp').mockReturnValue(false); + mockGenerateContent.mockRejectedValue(new Error('API error')); + const tool = new WebFetchTool(mockConfig); + const params = { prompt: 'fetch https://public.ip' }; + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + expect(result.error?.type).toBe(ToolErrorType.WEB_FETCH_PROCESSING_ERROR); + }); + }); describe('shouldConfirmExecute', () => { it('should return confirmation details with the correct prompt and urls', async () => { @@ -58,10 +113,10 @@ describe('WebFetchTool', () => { }); it('should return false if approval mode is AUTO_EDIT', async () => { - const tool = new WebFetchTool({ - ...mockConfig, - getApprovalMode: () => ApprovalMode.AUTO_EDIT, - } as unknown as Config); + vi.spyOn(mockConfig, 'getApprovalMode').mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const tool = new WebFetchTool(mockConfig); const params = { prompt: 'fetch https://example.com' }; const invocation = tool.build(params); const confirmationDetails = await invocation.shouldConfirmExecute( @@ -72,11 +127,7 @@ describe('WebFetchTool', () => { }); it('should call setApprovalMode when onConfirm is called with ProceedAlways', async () => { - const setApprovalMode = vi.fn(); - const tool = new WebFetchTool({ - ...mockConfig, - setApprovalMode, - } as unknown as Config); + const tool = new WebFetchTool(mockConfig); const params = { prompt: 'fetch https://example.com' }; const invocation = tool.build(params); const confirmationDetails = await invocation.shouldConfirmExecute( @@ -93,7 +144,9 @@ describe('WebFetchTool', () => { ); } - expect(setApprovalMode).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT); + expect(mockConfig.setApprovalMode).toHaveBeenCalledWith( + ApprovalMode.AUTO_EDIT, + ); }); }); }); diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index 26e59efc..dfefae3d 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -13,6 +13,7 @@ import { ToolInvocation, ToolResult, } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; import { getErrorMessage } from '../utils/errors.js'; import { ApprovalMode, Config } from '../config/config.js'; import { getResponseText } from '../utils/generateContentResponseUtilities.js'; @@ -73,12 +74,6 @@ class WebFetchToolInvocation extends BaseToolInvocation< private async executeFallback(signal: AbortSignal): Promise { const urls = extractUrls(this.params.prompt); - if (urls.length === 0) { - return { - llmContent: 'Error: No URL found in the prompt for fallback.', - returnDisplay: 'Error: No URL found in the prompt for fallback.', - }; - } // For now, we only support one URL for fallback let url = urls[0]; @@ -130,6 +125,10 @@ ${textContent} return { llmContent: `Error: ${errorMessage}`, returnDisplay: `Error: ${errorMessage}`, + error: { + message: errorMessage, + type: ToolErrorType.WEB_FETCH_FALLBACK_FAILED, + }, }; } } @@ -300,6 +299,10 @@ ${sourceListFormatted.join('\n')}`; return { llmContent: `Error: ${errorMessage}`, returnDisplay: `Error: ${errorMessage}`, + error: { + message: errorMessage, + type: ToolErrorType.WEB_FETCH_PROCESSING_ERROR, + }, }; } } diff --git a/packages/core/src/tools/web-search.test.ts b/packages/core/src/tools/web-search.test.ts index c0620c08..0f7c1d6c 100644 --- a/packages/core/src/tools/web-search.test.ts +++ b/packages/core/src/tools/web-search.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, vi, beforeEach, afterEach, Mock } from 'vitest'; import { WebSearchTool, WebSearchToolParams } from './web-search.js'; import { Config } from '../config/config.js'; import { GeminiClient } from '../core/client.js'; +import { ToolErrorType } from './tool-error.js'; // Mock GeminiClient and Config constructor vi.mock('../core/client.js'); @@ -112,7 +113,7 @@ describe('WebSearchTool', () => { expect(result.returnDisplay).toBe('No information found.'); }); - it('should handle API errors gracefully', async () => { + it('should return a WEB_SEARCH_FAILED error on failure', async () => { const params: WebSearchToolParams = { query: 'error query' }; const testError = new Error('API Failure'); (mockGeminiClient.generateContent as Mock).mockRejectedValue(testError); @@ -120,6 +121,7 @@ describe('WebSearchTool', () => { const invocation = tool.build(params); const result = await invocation.execute(abortSignal); + expect(result.error?.type).toBe(ToolErrorType.WEB_SEARCH_FAILED); expect(result.llmContent).toContain('Error:'); expect(result.llmContent).toContain('API Failure'); expect(result.returnDisplay).toBe('Error performing web search.'); diff --git a/packages/core/src/tools/web-search.ts b/packages/core/src/tools/web-search.ts index 71038d5e..442fac4f 100644 --- a/packages/core/src/tools/web-search.ts +++ b/packages/core/src/tools/web-search.ts @@ -12,6 +12,7 @@ import { ToolInvocation, ToolResult, } from './tools.js'; +import { ToolErrorType } from './tool-error.js'; import { getErrorMessage } from '../utils/errors.js'; import { Config } from '../config/config.js'; @@ -153,6 +154,10 @@ class WebSearchToolInvocation extends BaseToolInvocation< return { llmContent: `Error: ${errorMessage}`, returnDisplay: `Error performing web search.`, + error: { + message: errorMessage, + type: ToolErrorType.WEB_SEARCH_FAILED, + }, }; } }