fix(cli): prevent exit on non-fatal tool errors (#10671)

This commit is contained in:
Jerop Kipruto
2025-10-09 17:20:20 -04:00
committed by GitHub
parent a8379d1f4b
commit 5f96eba54a
4 changed files with 160 additions and 93 deletions

View File

@@ -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();
});
});

View File

@@ -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,
),
);
});
});
});
});

View File

@@ -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);
}
/**

View File

@@ -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<string>([ToolErrorType.NO_SPACE_LEFT]);
return fatalErrors.has(errorType);
}