diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 3e4ce037..79ee2d0d 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -150,7 +150,7 @@ describe('runNonInteractive', () => { expect(processStdoutSpy).toHaveBeenCalledWith('\n'); }); - it('should handle error during tool execution', async () => { + it('should handle error during tool execution and should send error back to the model', async () => { const toolCallEvent: ServerGeminiStreamEvent = { type: GeminiEventType.ToolCallRequest, value: { @@ -162,20 +162,52 @@ describe('runNonInteractive', () => { }, }; mockCoreExecuteToolCall.mockResolvedValue({ - error: new Error('Tool execution failed badly'), - errorType: ToolErrorType.UNHANDLED_EXCEPTION, + error: new Error('Execution failed'), + errorType: ToolErrorType.EXECUTION_FAILED, + responseParts: { + functionResponse: { + name: 'errorTool', + response: { + output: 'Error: Execution failed', + }, + }, + }, + resultDisplay: 'Execution failed', }); - mockGeminiClient.sendMessageStream.mockReturnValue( - createStreamFromEvents([toolCallEvent]), - ); + const finalResponse: ServerGeminiStreamEvent[] = [ + { + type: GeminiEventType.Content, + value: 'Sorry, let me try again.', + }, + ]; + mockGeminiClient.sendMessageStream + .mockReturnValueOnce(createStreamFromEvents([toolCallEvent])) + .mockReturnValueOnce(createStreamFromEvents(finalResponse)); await runNonInteractive(mockConfig, 'Trigger tool error', 'prompt-id-3'); expect(mockCoreExecuteToolCall).toHaveBeenCalled(); expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Error executing tool errorTool: Tool execution failed badly', + 'Error executing tool errorTool: Execution failed', ); - expect(processExitSpy).toHaveBeenCalledWith(1); + expect(processExitSpy).not.toHaveBeenCalled(); + expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2); + expect(mockGeminiClient.sendMessageStream).toHaveBeenNthCalledWith( + 2, + [ + { + functionResponse: { + name: 'errorTool', + response: { + output: 'Error: Execution failed', + }, + }, + }, + ], + expect.any(AbortSignal), + 'prompt-id-3', + ); + expect(processStdoutSpy).toHaveBeenCalledWith('Sorry, let me try again.'); }); it('should exit with error if sendMessageStream throws initially', async () => { diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index f2efe8fc..b65bf15d 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -12,7 +12,6 @@ import { shutdownTelemetry, isTelemetrySdkInitialized, GeminiEventType, - ToolErrorType, parseAndFormatApiError, } from '@google/gemini-cli-core'; import { Content, Part, FunctionCall } from '@google/genai'; @@ -109,8 +108,6 @@ export async function runNonInteractive( console.error( `Error executing tool ${fc.name}: ${toolResponse.resultDisplay || toolResponse.error.message}`, ); - if (toolResponse.errorType === ToolErrorType.UNHANDLED_EXCEPTION) - process.exit(1); } if (toolResponse.responseParts) { diff --git a/packages/core/src/core/nonInteractiveToolExecutor.test.ts b/packages/core/src/core/nonInteractiveToolExecutor.test.ts index b0ed7107..484d2e45 100644 --- a/packages/core/src/core/nonInteractiveToolExecutor.test.ts +++ b/packages/core/src/core/nonInteractiveToolExecutor.test.ts @@ -11,6 +11,7 @@ import { ToolCallRequestInfo, ToolResult, Config, + ToolErrorType, } from '../index.js'; import { Part } from '@google/genai'; import { MockTool } from '../test-utils/tools.js'; @@ -50,7 +51,7 @@ describe('executeToolCall', () => { returnDisplay: 'Success!', }; vi.mocked(mockToolRegistry.getTool).mockReturnValue(mockTool); - vi.spyOn(mockTool, 'buildAndExecute').mockResolvedValue(toolResult); + vi.spyOn(mockTool, 'validateBuildAndExecute').mockResolvedValue(toolResult); const response = await executeToolCall( mockConfig, @@ -60,7 +61,7 @@ describe('executeToolCall', () => { ); expect(mockToolRegistry.getTool).toHaveBeenCalledWith('testTool'); - expect(mockTool.buildAndExecute).toHaveBeenCalledWith( + expect(mockTool.validateBuildAndExecute).toHaveBeenCalledWith( request.args, abortController.signal, ); @@ -112,17 +113,26 @@ describe('executeToolCall', () => { ]); }); - it('should return an error if tool execution fails', async () => { + it('should return an error if tool validation fails', async () => { const request: ToolCallRequestInfo = { callId: 'call3', name: 'testTool', - args: { param1: 'value1' }, + args: { param1: 'invalid' }, isClientInitiated: false, prompt_id: 'prompt-id-3', }; - const executionError = new Error('Tool execution failed'); + const validationErrorResult: ToolResult = { + llmContent: 'Error: Invalid parameters', + returnDisplay: 'Invalid parameters', + error: { + message: 'Invalid parameters', + type: ToolErrorType.INVALID_TOOL_PARAMS, + }, + }; vi.mocked(mockToolRegistry.getTool).mockReturnValue(mockTool); - vi.spyOn(mockTool, 'buildAndExecute').mockRejectedValue(executionError); + vi.spyOn(mockTool, 'validateBuildAndExecute').mockResolvedValue( + validationErrorResult, + ); const response = await executeToolCall( mockConfig, @@ -130,22 +140,24 @@ describe('executeToolCall', () => { mockToolRegistry, abortController.signal, ); - - expect(response.callId).toBe('call3'); - expect(response.error).toBe(executionError); - expect(response.resultDisplay).toBe('Tool execution failed'); - expect(response.responseParts).toEqual([ - { + expect(response).toStrictEqual({ + callId: 'call3', + error: new Error('Invalid parameters'), + errorType: ToolErrorType.INVALID_TOOL_PARAMS, + responseParts: { functionResponse: { - name: 'testTool', id: 'call3', - response: { error: 'Tool execution failed' }, + name: 'testTool', + response: { + output: 'Error: Invalid parameters', + }, }, }, - ]); + resultDisplay: 'Invalid parameters', + }); }); - it('should handle cancellation during tool execution', async () => { + it('should return an error if tool execution fails', async () => { const request: ToolCallRequestInfo = { callId: 'call4', name: 'testTool', @@ -153,32 +165,56 @@ describe('executeToolCall', () => { isClientInitiated: false, prompt_id: 'prompt-id-4', }; - const cancellationError = new Error('Operation cancelled'); - vi.mocked(mockToolRegistry.getTool).mockReturnValue(mockTool); - - vi.spyOn(mockTool, 'buildAndExecute').mockImplementation( - async (_args, signal) => { - if (signal?.aborted) { - return Promise.reject(cancellationError); - } - return new Promise((_resolve, reject) => { - signal?.addEventListener('abort', () => { - reject(cancellationError); - }); - // Simulate work that might happen if not aborted immediately - const timeoutId = setTimeout( - () => - reject( - new Error('Should have been cancelled if not aborted prior'), - ), - 100, - ); - signal?.addEventListener('abort', () => clearTimeout(timeoutId)); - }); + const executionErrorResult: ToolResult = { + llmContent: 'Error: Execution failed', + returnDisplay: 'Execution failed', + error: { + message: 'Execution failed', + type: ToolErrorType.EXECUTION_FAILED, }, + }; + vi.mocked(mockToolRegistry.getTool).mockReturnValue(mockTool); + vi.spyOn(mockTool, 'validateBuildAndExecute').mockResolvedValue( + executionErrorResult, + ); + + const response = await executeToolCall( + mockConfig, + request, + mockToolRegistry, + abortController.signal, + ); + expect(response).toStrictEqual({ + callId: 'call4', + error: new Error('Execution failed'), + errorType: ToolErrorType.EXECUTION_FAILED, + responseParts: { + functionResponse: { + id: 'call4', + name: 'testTool', + response: { + output: 'Error: Execution failed', + }, + }, + }, + resultDisplay: 'Execution failed', + }); + }); + + it('should return an unhandled exception error if execution throws', async () => { + const request: ToolCallRequestInfo = { + callId: 'call5', + name: 'testTool', + args: { param1: 'value1' }, + isClientInitiated: false, + prompt_id: 'prompt-id-5', + }; + const executionError = new Error('Something went very wrong'); + vi.mocked(mockToolRegistry.getTool).mockReturnValue(mockTool); + vi.spyOn(mockTool, 'validateBuildAndExecute').mockRejectedValue( + executionError, ); - abortController.abort(); // Abort before calling const response = await executeToolCall( mockConfig, request, @@ -186,18 +222,28 @@ describe('executeToolCall', () => { abortController.signal, ); - expect(response.callId).toBe('call4'); - expect(response.error?.message).toBe(cancellationError.message); - expect(response.resultDisplay).toBe('Operation cancelled'); + expect(response.callId).toBe('call5'); + expect(response.error).toBe(executionError); + expect(response.errorType).toBe(ToolErrorType.UNHANDLED_EXCEPTION); + expect(response.resultDisplay).toBe('Something went very wrong'); + expect(response.responseParts).toEqual([ + { + functionResponse: { + name: 'testTool', + id: 'call5', + response: { error: 'Something went very wrong' }, + }, + }, + ]); }); it('should correctly format llmContent with inlineData', async () => { const request: ToolCallRequestInfo = { - callId: 'call5', + callId: 'call6', name: 'testTool', args: {}, isClientInitiated: false, - prompt_id: 'prompt-id-5', + prompt_id: 'prompt-id-6', }; const imageDataPart: Part = { inlineData: { mimeType: 'image/png', data: 'base64data' }, @@ -207,7 +253,7 @@ describe('executeToolCall', () => { returnDisplay: 'Image processed', }; vi.mocked(mockToolRegistry.getTool).mockReturnValue(mockTool); - vi.spyOn(mockTool, 'buildAndExecute').mockResolvedValue(toolResult); + vi.spyOn(mockTool, 'validateBuildAndExecute').mockResolvedValue(toolResult); const response = await executeToolCall( mockConfig, @@ -221,7 +267,7 @@ describe('executeToolCall', () => { { functionResponse: { name: 'testTool', - id: 'call5', + id: 'call6', response: { output: 'Binary content of type image/png was processed.', }, diff --git a/packages/core/src/core/nonInteractiveToolExecutor.ts b/packages/core/src/core/nonInteractiveToolExecutor.ts index ab38d830..b7c4faae 100644 --- a/packages/core/src/core/nonInteractiveToolExecutor.ts +++ b/packages/core/src/core/nonInteractiveToolExecutor.ts @@ -66,7 +66,7 @@ export async function executeToolCall( try { // Directly execute without confirmation or live output handling const effectiveAbortSignal = abortSignal ?? new AbortController().signal; - const toolResult: ToolResult = await tool.buildAndExecute( + const toolResult: ToolResult = await tool.validateBuildAndExecute( toolCallRequest.args, effectiveAbortSignal, // No live output callback for non-interactive mode diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index 2702de02..fc19fc72 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -13,6 +13,7 @@ export enum ToolErrorType { UNKNOWN = 'unknown', UNHANDLED_EXCEPTION = 'unhandled_exception', TOOL_NOT_REGISTERED = 'tool_not_registered', + EXECUTION_FAILED = 'execution_failed', // File System Errors FILE_NOT_FOUND = 'file_not_found', diff --git a/packages/core/src/tools/tools.test.ts b/packages/core/src/tools/tools.test.ts index 9942d3a9..a98d1121 100644 --- a/packages/core/src/tools/tools.test.ts +++ b/packages/core/src/tools/tools.test.ts @@ -4,8 +4,119 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; -import { hasCycleInSchema } from './tools.js'; // Added getStringifiedResultForDisplay +import { describe, it, expect, vi } from 'vitest'; +import { + DeclarativeTool, + hasCycleInSchema, + Kind, + ToolInvocation, + ToolResult, +} from './tools.js'; +import { ToolErrorType } from './tool-error.js'; + +class TestToolInvocation implements ToolInvocation { + constructor( + readonly params: object, + private readonly executeFn: () => Promise, + ) {} + + getDescription(): string { + return 'A test invocation'; + } + + toolLocations() { + return []; + } + + shouldConfirmExecute(): Promise { + return Promise.resolve(false); + } + + execute(): Promise { + return this.executeFn(); + } +} + +class TestTool extends DeclarativeTool { + private readonly buildFn: (params: object) => TestToolInvocation; + + constructor(buildFn: (params: object) => TestToolInvocation) { + super('test-tool', 'Test Tool', 'A tool for testing', Kind.Other, {}); + this.buildFn = buildFn; + } + + build(params: object): ToolInvocation { + return this.buildFn(params); + } +} + +describe('DeclarativeTool', () => { + describe('validateBuildAndExecute', () => { + const abortSignal = new AbortController().signal; + + it('should return INVALID_TOOL_PARAMS error if build fails', async () => { + const buildError = new Error('Invalid build parameters'); + const buildFn = vi.fn().mockImplementation(() => { + throw buildError; + }); + const tool = new TestTool(buildFn); + const params = { foo: 'bar' }; + + const result = await tool.validateBuildAndExecute(params, abortSignal); + + expect(buildFn).toHaveBeenCalledWith(params); + expect(result).toEqual({ + llmContent: `Error: Invalid parameters provided. Reason: ${buildError.message}`, + returnDisplay: buildError.message, + error: { + message: buildError.message, + type: ToolErrorType.INVALID_TOOL_PARAMS, + }, + }); + }); + + it('should return EXECUTION_FAILED error if execute fails', async () => { + const executeError = new Error('Execution failed'); + const executeFn = vi.fn().mockRejectedValue(executeError); + const invocation = new TestToolInvocation({}, executeFn); + const buildFn = vi.fn().mockReturnValue(invocation); + const tool = new TestTool(buildFn); + const params = { foo: 'bar' }; + + const result = await tool.validateBuildAndExecute(params, abortSignal); + + expect(buildFn).toHaveBeenCalledWith(params); + expect(executeFn).toHaveBeenCalled(); + expect(result).toEqual({ + llmContent: `Error: Tool call execution failed. Reason: ${executeError.message}`, + returnDisplay: executeError.message, + error: { + message: executeError.message, + type: ToolErrorType.EXECUTION_FAILED, + }, + }); + }); + + it('should return the result of execute on success', async () => { + const successResult: ToolResult = { + llmContent: 'Success!', + returnDisplay: 'Success!', + summary: 'Tool executed successfully', + }; + const executeFn = vi.fn().mockResolvedValue(successResult); + const invocation = new TestToolInvocation({}, executeFn); + const buildFn = vi.fn().mockReturnValue(invocation); + const tool = new TestTool(buildFn); + const params = { foo: 'bar' }; + + const result = await tool.validateBuildAndExecute(params, abortSignal); + + expect(buildFn).toHaveBeenCalledWith(params); + expect(executeFn).toHaveBeenCalled(); + expect(result).toEqual(successResult); + }); + }); +}); describe('hasCycleInSchema', () => { it('should detect a simple direct cycle', () => { diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 68ea00af..deb54cf6 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -200,6 +200,64 @@ export abstract class DeclarativeTool< const invocation = this.build(params); return invocation.execute(signal, updateOutput); } + + /** + * Similar to `build` but never throws. + * @param params The raw, untrusted parameters from the model. + * @returns A `ToolInvocation` instance. + */ + private silentBuild( + params: TParams, + ): ToolInvocation | Error { + try { + return this.build(params); + } catch (e) { + if (e instanceof Error) { + return e; + } + return new Error(String(e)); + } + } + + /** + * A convenience method that builds and executes the tool in one step. + * Never throws. + * @param params The raw, untrusted parameters from the model. + * @params abortSignal a signal to abort. + * @returns The result of the tool execution. + */ + async validateBuildAndExecute( + params: TParams, + abortSignal: AbortSignal, + ): Promise { + const invocationOrError = this.silentBuild(params); + if (invocationOrError instanceof Error) { + const errorMessage = invocationOrError.message; + return { + llmContent: `Error: Invalid parameters provided. Reason: ${errorMessage}`, + returnDisplay: errorMessage, + error: { + message: errorMessage, + type: ToolErrorType.INVALID_TOOL_PARAMS, + }, + }; + } + + try { + return await invocationOrError.execute(abortSignal); + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : String(error); + return { + llmContent: `Error: Tool call execution failed. Reason: ${errorMessage}`, + returnDisplay: errorMessage, + error: { + message: errorMessage, + type: ToolErrorType.EXECUTION_FAILED, + }, + }; + } + } } /**