diff --git a/integration-tests/json-output.test.ts b/integration-tests/json-output.test.ts index 7a65a65133..2a89e7de8a 100644 --- a/integration-tests/json-output.test.ts +++ b/integration-tests/json-output.test.ts @@ -86,4 +86,31 @@ describe('JSON output', () => { 'current auth type is oauth-personal', ); }); + + it('should not exit on tool errors and allow model to self-correct in JSON mode', async () => { + const result = await rig.run( + 'Read the contents of /path/to/nonexistent/file.txt and tell me what it says', + '--output-format', + 'json', + ); + + const parsed = JSON.parse(result); + + // The response should contain an actual response from the model, + // not a fatal error that caused the CLI to exit + expect(parsed).toHaveProperty('response'); + expect(typeof parsed.response).toBe('string'); + + // The model should acknowledge the error in its response + expect(parsed.response.toLowerCase()).toMatch( + /cannot|does not exist|not found|unable to|error|couldn't/, + ); + + // Stats should be present, indicating the session completed normally + expect(parsed).toHaveProperty('stats'); + expect(parsed.stats).toHaveProperty('tools'); + + // Should NOT have an error field at the top level + expect(parsed.error).toBeUndefined(); + }); }); diff --git a/packages/cli/src/utils/errors.test.ts b/packages/cli/src/utils/errors.test.ts index 0df7ddf118..5478287ae2 100644 --- a/packages/cli/src/utils/errors.test.ts +++ b/packages/cli/src/utils/errors.test.ts @@ -291,90 +291,90 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format error as JSON and exit with default code', () => { - expect(() => { - handleToolError(toolName, toolError, mockConfig); - }).toThrow('process.exit called with code: 54'); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - JSON.stringify( - { - error: { - type: 'FatalToolExecutionError', - message: 'Error executing tool test-tool: Tool failed', - code: 54, - }, - }, - null, - 2, - ), - ); - }); - - it('should use custom error code', () => { - expect(() => { - handleToolError(toolName, toolError, mockConfig, 'CUSTOM_TOOL_ERROR'); - }).toThrow('process.exit called with code: 54'); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - JSON.stringify( - { - error: { - type: 'FatalToolExecutionError', - message: 'Error executing tool test-tool: Tool failed', - code: 'CUSTOM_TOOL_ERROR', - }, - }, - null, - 2, - ), - ); - }); - - it('should use numeric error code and exit with that code', () => { - expect(() => { - handleToolError(toolName, toolError, mockConfig, 500); - }).toThrow('process.exit called with code: 500'); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - JSON.stringify( - { - error: { - type: 'FatalToolExecutionError', - message: 'Error executing tool test-tool: Tool failed', - code: 500, - }, - }, - null, - 2, - ), - ); - }); - - it('should prefer resultDisplay over error message', () => { - expect(() => { + describe('non-fatal errors', () => { + it('should log error message to stderr without exiting for recoverable errors', () => { handleToolError( toolName, toolError, mockConfig, - 'DISPLAY_ERROR', + 'invalid_tool_params', + ); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); + // Should not exit for non-fatal errors + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should not exit for file not found errors', () => { + handleToolError(toolName, toolError, mockConfig, 'file_not_found'); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should not exit for permission denied errors', () => { + handleToolError(toolName, toolError, mockConfig, 'permission_denied'); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should not exit for path not in workspace errors', () => { + handleToolError( + toolName, + toolError, + mockConfig, + 'path_not_in_workspace', + ); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + + it('should prefer resultDisplay over error message', () => { + handleToolError( + toolName, + toolError, + mockConfig, + 'invalid_tool_params', 'Display message', ); - }).toThrow('process.exit called with code: 54'); - expect(consoleErrorSpy).toHaveBeenCalledWith( - JSON.stringify( - { - error: { - type: 'FatalToolExecutionError', - message: 'Error executing tool test-tool: Display message', - code: 'DISPLAY_ERROR', + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error executing tool test-tool: Display message', + ); + expect(processExitSpy).not.toHaveBeenCalled(); + }); + }); + + describe('fatal errors', () => { + it('should exit immediately for NO_SPACE_LEFT errors', () => { + expect(() => { + handleToolError(toolName, toolError, mockConfig, 'no_space_left'); + }).toThrow('process.exit called with code: 54'); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + JSON.stringify( + { + error: { + type: 'FatalToolExecutionError', + message: 'Error executing tool test-tool: Tool failed', + code: 'no_space_left', + }, }, - }, - null, - 2, - ), - ); + null, + 2, + ), + ); + }); }); }); }); diff --git a/packages/cli/src/utils/errors.ts b/packages/cli/src/utils/errors.ts index 7ff27fb92a..ba018434eb 100644 --- a/packages/cli/src/utils/errors.ts +++ b/packages/cli/src/utils/errors.ts @@ -10,8 +10,9 @@ import { JsonFormatter, parseAndFormatApiError, FatalTurnLimitedError, - FatalToolExecutionError, FatalCancellationError, + FatalToolExecutionError, + isFatalToolError, } from '@google/gemini-cli-core'; export function getErrorMessage(error: unknown): string { @@ -88,33 +89,42 @@ export function handleError( /** * Handles tool execution errors specifically. - * In JSON mode, outputs formatted JSON error and exits. - * In text mode, outputs error message to stderr only. + * + * Fatal errors (e.g., NO_SPACE_LEFT) cause the CLI to exit immediately, + * as they indicate unrecoverable system state. + * + * Non-fatal errors (e.g., INVALID_TOOL_PARAMS, FILE_NOT_FOUND, PATH_NOT_IN_WORKSPACE) + * are logged to stderr and the error response is sent back to the model, + * allowing it to self-correct. */ export function handleToolError( toolName: string, toolError: Error, config: Config, - errorCode?: string | number, + errorType?: string, resultDisplay?: string, ): void { const errorMessage = `Error executing tool ${toolName}: ${resultDisplay || toolError.message}`; - const toolExecutionError = new FatalToolExecutionError(errorMessage); - if (config.getOutputFormat() === OutputFormat.JSON) { - const formatter = new JsonFormatter(); - const formattedError = formatter.formatError( - toolExecutionError, - errorCode ?? toolExecutionError.exitCode, - ); + const isFatal = isFatalToolError(errorType); - console.error(formattedError); - process.exit( - typeof errorCode === 'number' ? errorCode : toolExecutionError.exitCode, - ); - } else { - console.error(errorMessage); + if (isFatal) { + const toolExecutionError = new FatalToolExecutionError(errorMessage); + if (config.getOutputFormat() === OutputFormat.JSON) { + const formatter = new JsonFormatter(); + const formattedError = formatter.formatError( + toolExecutionError, + errorType ?? toolExecutionError.exitCode, + ); + console.error(formattedError); + } else { + console.error(errorMessage); + } + process.exit(toolExecutionError.exitCode); } + + // Non-fatal: log and continue + console.error(errorMessage); } /** diff --git a/packages/core/src/tools/tool-error.ts b/packages/core/src/tools/tool-error.ts index 3c1c9a8a77..50ce99f2e8 100644 --- a/packages/core/src/tools/tool-error.ts +++ b/packages/core/src/tools/tool-error.ts @@ -6,6 +6,10 @@ /** * A type-safe enum for tool-related errors. + * + * Error types are categorized as: + * - Recoverable: LLM can self-correct (e.g., invalid params, file not found) + * - Fatal: System-level issues that prevent continued execution (e.g., disk full, critical I/O errors) */ export enum ToolErrorType { // General Errors @@ -68,3 +72,29 @@ export enum ToolErrorType { // WebSearch-specific Errors WEB_SEARCH_FAILED = 'web_search_failed', } + +/** + * Determines if a tool error type should be treated as fatal. + * + * Fatal errors are system-level issues that indicate the environment is in a bad state + * and continued execution is unlikely to succeed. These include: + * - Disk space issues (NO_SPACE_LEFT) + * + * Non-fatal errors are issues the LLM can potentially recover from by: + * - Correcting invalid parameters (INVALID_TOOL_PARAMS) + * - Trying different files (FILE_NOT_FOUND) + * - Respecting security boundaries (PATH_NOT_IN_WORKSPACE, PERMISSION_DENIED) + * - Using different tools or approaches + * + * @param errorType - The tool error type to check + * @returns true if the error should cause the CLI to exit, false if it's recoverable + */ +export function isFatalToolError(errorType?: string): boolean { + if (!errorType) { + return false; + } + + const fatalErrors = new Set([ToolErrorType.NO_SPACE_LEFT]); + + return fatalErrors.has(errorType); +}