From d9f94103cdf37030ee2c15d10fe9388674a5302b Mon Sep 17 00:00:00 2001 From: Gaurav Sehgal Date: Sat, 13 Dec 2025 06:31:12 +0530 Subject: [PATCH] Add clarity to error messages (#14879) --- packages/cli/src/nonInteractiveCli.test.ts | 2 - packages/cli/src/nonInteractiveCli.ts | 6 +- .../src/ui/hooks/atCommandProcessor.test.ts | 88 ++++++++++++------- .../cli/src/ui/hooks/atCommandProcessor.ts | 43 +++++---- packages/cli/src/ui/hooks/useGeminiStream.ts | 3 +- 5 files changed, 88 insertions(+), 54 deletions(-) diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index f40dc68cbd..388f3e6454 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -198,7 +198,6 @@ describe('runNonInteractive', () => { ); vi.mocked(handleAtCommand).mockImplementation(async ({ query }) => ({ processedQuery: [{ text: query }], - shouldProceed: true, })); }); @@ -573,7 +572,6 @@ describe('runNonInteractive', () => { // 3. Setup the mock to return the processed parts mockHandleAtCommand.mockResolvedValue({ processedQuery: processedParts, - shouldProceed: true, }); // Mock a simple stream response from the Gemini client diff --git a/packages/cli/src/nonInteractiveCli.ts b/packages/cli/src/nonInteractiveCli.ts index 53f2b9a0eb..08ea421ab3 100644 --- a/packages/cli/src/nonInteractiveCli.ts +++ b/packages/cli/src/nonInteractiveCli.ts @@ -230,7 +230,7 @@ export async function runNonInteractive({ } if (!query) { - const { processedQuery, shouldProceed } = await handleAtCommand({ + const { processedQuery, error } = await handleAtCommand({ query: input, config, addItem: (_item, _timestamp) => 0, @@ -239,11 +239,11 @@ export async function runNonInteractive({ signal: abortController.signal, }); - if (!shouldProceed || !processedQuery) { + if (error || !processedQuery) { // An error occurred during @include processing (e.g., file not found). // The error message is already logged by handleAtCommand. throw new FatalInputError( - 'Exiting due to an error processing the @ command.', + error || 'Exiting due to an error processing the @ command.', ); } query = processedQuery as Part[]; diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index 68eaa423de..ae093ee56c 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -17,6 +17,7 @@ import { COMMON_IGNORE_PATTERNS, // DEFAULT_FILE_EXCLUDES, } from '@google/gemini-cli-core'; +import * as core from '@google/gemini-cli-core'; import * as os from 'node:os'; import { ToolCallStatus } from '../types.js'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; @@ -120,7 +121,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: query }], - shouldProceed: true, }); }); @@ -138,7 +138,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: queryWithSpaces }], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( 'Lone @ detected, will be treated as text in the modified query.', @@ -171,7 +170,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockAddItem).toHaveBeenCalledWith( expect.objectContaining({ @@ -211,7 +209,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${dirPath} resolved to directory, using glob: ${resolvedGlob}`, @@ -246,7 +243,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -276,7 +272,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockAddItem).toHaveBeenCalledWith( expect.objectContaining({ @@ -321,7 +316,6 @@ describe('handleAtCommand', () => { { text: content2 }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -362,7 +356,6 @@ describe('handleAtCommand', () => { { text: content2 }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -401,7 +394,6 @@ describe('handleAtCommand', () => { { text: content1 }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${invalidFile} not found directly, attempting glob search.`, @@ -428,7 +420,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: 'Check @nonexistent.txt and @ also' }], - shouldProceed: true, }); }); @@ -462,7 +453,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: query }], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${gitIgnoredFile} is git-ignored and will be skipped.`, @@ -501,7 +491,6 @@ describe('handleAtCommand', () => { { text: 'console.log("Hello world");' }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -534,7 +523,6 @@ describe('handleAtCommand', () => { { text: '# Project README' }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${gitIgnoredFile} is git-ignored and will be skipped.`, @@ -562,7 +550,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: query }], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${gitFile} is git-ignored and will be skipped.`, @@ -595,7 +582,8 @@ describe('handleAtCommand', () => { `Glob tool not found. Path ${invalidFile} will be skipped.`, ); expect(result.processedQuery).toEqual([{ text: query }]); - expect(result.shouldProceed).toBe(true); + expect(result.processedQuery).not.toBeNull(); + expect(result.error).toBeUndefined(); }); }); @@ -622,7 +610,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: query }], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${geminiIgnoredFile} is gemini-ignored and will be skipped.`, @@ -660,7 +647,6 @@ describe('handleAtCommand', () => { { text: 'console.log("Hello world");' }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -696,7 +682,6 @@ describe('handleAtCommand', () => { { text: '// Main application entry' }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( `Path ${geminiIgnoredFile} is gemini-ignored and will be skipped.`, @@ -824,7 +809,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }, ); @@ -863,7 +847,6 @@ describe('handleAtCommand', () => { { text: content2 }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -893,7 +876,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -924,7 +906,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -955,7 +936,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -986,11 +966,10 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); - it('should not terminate at period within file name', async () => { + it('should correctly handle file paths with multiple periods', async () => { const fileContent = 'Version info'; const filePath = await createTestFile( path.join(testRootDir, 'version.1.2.3.txt'), @@ -1017,7 +996,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -1046,7 +1024,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -1075,7 +1052,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); @@ -1104,7 +1080,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); }); }); @@ -1136,7 +1111,6 @@ describe('handleAtCommand', () => { { text: fileContent }, { text: '\n--- End of content ---' }, ], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( @@ -1165,7 +1139,8 @@ describe('handleAtCommand', () => { signal: abortController.signal, }); - expect(result.shouldProceed).toBe(true); + expect(result.processedQuery).not.toBeNull(); + expect(result.error).toBeUndefined(); expect(result.processedQuery).toEqual( expect.arrayContaining([ { text: `Check @${path.join(subDirPath, '**')} please.` }, @@ -1207,7 +1182,6 @@ describe('handleAtCommand', () => { expect(result).toEqual({ processedQuery: [{ text: `Check @${outsidePath} please.` }], - shouldProceed: true, }); expect(mockOnDebugMessage).toHaveBeenCalledWith( @@ -1326,7 +1300,8 @@ describe('handleAtCommand', () => { signal: abortController.signal, }); - expect(result.shouldProceed).toBe(false); + expect(result.processedQuery).toBeNull(); + expect(result.error).toBeDefined(); expect(mockAddItem).toHaveBeenCalledWith( expect.objectContaining({ type: 'tool_group', @@ -1342,4 +1317,51 @@ describe('handleAtCommand', () => { ); }); }); + + it('should return error if the read_many_files tool is cancelled by user', async () => { + const fileContent = 'Some content'; + const filePath = await createTestFile( + path.join(testRootDir, 'file.txt'), + fileContent, + ); + const query = `@${filePath}`; + + // Simulate user cancellation + const mockToolInstance = { + buildAndExecute: vi + .fn() + .mockRejectedValue(new Error('User cancelled operation')), + displayName: 'Read Many Files', + build: vi.fn(() => ({ + execute: mockToolInstance.buildAndExecute, + getDescription: vi.fn(() => 'Mocked tool description'), + })), + }; + const viSpy = vi.spyOn(core, 'ReadManyFilesTool'); + viSpy.mockImplementation( + () => mockToolInstance as unknown as core.ReadManyFilesTool, + ); + + const result = await handleAtCommand({ + query, + config: mockConfig, + addItem: mockAddItem, + onDebugMessage: mockOnDebugMessage, + messageId: 134, + signal: abortController.signal, + }); + + expect(result).toEqual({ + processedQuery: null, + error: `Exiting due to an error processing the @ command: Error reading files (file.txt): User cancelled operation`, + }); + + expect(mockAddItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'tool_group', + tools: [expect.objectContaining({ status: ToolCallStatus.Error })], + }), + 134, + ); + }); }); diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index 6f620f3f4d..05b2f1542e 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -35,7 +35,7 @@ interface HandleAtCommandParams { interface HandleAtCommandResult { processedQuery: PartListUnion | null; - shouldProceed: boolean; + error?: string; } interface AtCommandPart { @@ -144,7 +144,7 @@ export async function handleAtCommand({ ); if (atPathCommandParts.length === 0) { - return { processedQuery: [{ text: query }], shouldProceed: true }; + return { processedQuery: [{ text: query }] }; } // Get centralized file discovery service @@ -172,7 +172,10 @@ export async function handleAtCommand({ { type: 'error', text: 'Error: read_many_files tool not found.' }, userMessageTimestamp, ); - return { processedQuery: null, shouldProceed: false }; + return { + processedQuery: null, + error: 'Error: read_many_files tool not found.', + }; } for (const atPathPart of atPathCommandParts) { @@ -189,16 +192,17 @@ export async function handleAtCommand({ if (!pathName) { // This case should ideally not be hit if parseAllAtCommands ensures content after @ // but as a safeguard: + const errMsg = `Error: Invalid @ command '${originalAtPath}'. No path specified.`; addItem( { type: 'error', - text: `Error: Invalid @ command '${originalAtPath}'. No path specified.`, + text: errMsg, }, userMessageTimestamp, ); // Decide if this is a fatal error for the whole command or just skip this @ part // For now, let's be strict and fail the command if one @path is malformed. - return { processedQuery: null, shouldProceed: false }; + return { processedQuery: null, error: errMsg }; } // Check if this is an MCP resource reference (serverName:uri format) @@ -417,16 +421,13 @@ export async function handleAtCommand({ onDebugMessage('No valid file paths found in @ commands to read.'); if (initialQueryText === '@' && query.trim() === '@') { // If the only thing was a lone @, pass original query (which might have spaces) - return { processedQuery: [{ text: query }], shouldProceed: true }; + return { processedQuery: [{ text: query }] }; } else if (!initialQueryText && query) { // If all @-commands were invalid and no surrounding text, pass original query - return { processedQuery: [{ text: query }], shouldProceed: true }; + return { processedQuery: [{ text: query }] }; } // Otherwise, proceed with the (potentially modified) query text that doesn't involve file reading - return { - processedQuery: [{ text: initialQueryText || query }], - shouldProceed: true, - }; + return { processedQuery: [{ text: initialQueryText || query }] }; } const processedQueryParts: PartListUnion = [{ text: initialQueryText }]; @@ -494,7 +495,16 @@ export async function handleAtCommand({ >, userMessageTimestamp, ); - return { processedQuery: null, shouldProceed: false }; + // Find the first error to report + const firstError = resourceReadDisplays.find( + (d) => d.status === ToolCallStatus.Error, + )!; + const errorMessages = resourceReadDisplays + .filter((d) => d.status === ToolCallStatus.Error) + .map((d) => d.resultDisplay); + console.error(errorMessages); + const errorMsg = `Exiting due to an error processing the @ command: ${firstError.resultDisplay}`; + return { processedQuery: null, error: errorMsg }; } if (pathSpecsToRead.length === 0) { @@ -507,7 +517,7 @@ export async function handleAtCommand({ userMessageTimestamp, ); } - return { processedQuery: processedQueryParts, shouldProceed: true }; + return { processedQuery: processedQueryParts }; } const toolArgs = { @@ -593,7 +603,7 @@ export async function handleAtCommand({ userMessageTimestamp, ); } - return { processedQuery: processedQueryParts, shouldProceed: true }; + return { processedQuery: processedQueryParts }; } catch (error: unknown) { readManyFilesDisplay = { callId: `client-read-${userMessageTimestamp}`, @@ -612,7 +622,10 @@ export async function handleAtCommand({ } as Omit, userMessageTimestamp, ); - return { processedQuery: null, shouldProceed: false }; + return { + processedQuery: null, + error: `Exiting due to an error processing the @ command: ${readManyFilesDisplay.resultDisplay}`, + }; } } diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 47b97936f6..86c4b6c114 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -483,7 +483,8 @@ export const useGeminiStream = ( userMessageTimestamp, ); - if (!atCommandResult.shouldProceed) { + if (atCommandResult.error) { + onDebugMessage(atCommandResult.error); return { queryToSend: null, shouldProceed: false }; } localQueryToSendToGemini = atCommandResult.processedQuery;