From 3ce035151ae9f185ea1facb04ad601c1aa6b0eb3 Mon Sep 17 00:00:00 2001 From: Jarrod Whelan Date: Wed, 11 Feb 2026 02:26:08 -0800 Subject: [PATCH] feat(core): introduce structured tool results for grep, ls, and read-many-files --- packages/core/src/tools/grep.test.ts | 22 ++++++--- packages/core/src/tools/grep.ts | 11 ++++- packages/core/src/tools/ls.test.ts | 45 ++++++++++++++----- packages/core/src/tools/ls.ts | 14 ++++-- .../core/src/tools/read-many-files.test.ts | 37 ++++++++------- packages/core/src/tools/read-many-files.ts | 9 ++-- packages/core/src/tools/tools.ts | 17 +++---- 7 files changed, 104 insertions(+), 51 deletions(-) diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 3f1f023faf..fb3e30d7fd 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -7,7 +7,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import type { GrepToolParams } from './grep.js'; import { GrepTool } from './grep.js'; -import type { ToolResult } from './tools.js'; +import type { ToolResult, StructuredToolResult } from './tools.js'; import path from 'node:path'; import { isSubpath } from '../utils/paths.js'; import fs from 'node:fs/promises'; @@ -181,7 +181,9 @@ describe('GrepTool', () => { `File: ${path.join('sub', 'fileC.txt')}`, ); expect(result.llmContent).toContain('L1: another world in sub dir'); - expect(result.returnDisplay).toBe('Found 3 matches'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Found 3 matches', + ); }, 30000); it('should include files that start with ".." in JS fallback', async () => { @@ -222,7 +224,9 @@ describe('GrepTool', () => { ); expect(result.llmContent).toContain('File: fileC.txt'); // Path relative to 'sub' expect(result.llmContent).toContain('L1: another world in sub dir'); - expect(result.returnDisplay).toBe('Found 1 match'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Found 1 match', + ); }, 30000); it('should find matches with an include glob', async () => { @@ -236,7 +240,9 @@ describe('GrepTool', () => { expect(result.llmContent).toContain( 'L2: function baz() { return "hello"; }', ); - expect(result.returnDisplay).toBe('Found 1 match'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Found 1 match', + ); }, 30000); it('should find matches with an include glob and path', async () => { @@ -256,7 +262,9 @@ describe('GrepTool', () => { ); expect(result.llmContent).toContain('File: another.js'); expect(result.llmContent).toContain('L1: const greeting = "hello";'); - expect(result.returnDisplay).toBe('Found 1 match'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Found 1 match', + ); }, 30000); it('should return "No matches found" when pattern does not exist', async () => { @@ -266,7 +274,9 @@ describe('GrepTool', () => { expect(result.llmContent).toContain( 'No matches found for pattern "nonexistentpattern" in the workspace directory.', ); - expect(result.returnDisplay).toBe('No matches found'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'No matches found', + ); }, 30000); it('should handle regex special characters correctly', async () => { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 41ef2533de..dd7a973bf5 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -244,7 +244,12 @@ class GrepToolInvocation extends BaseToolInvocation< if (allMatches.length === 0) { const noMatchMsg = `No matches found for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}.`; - return { llmContent: noMatchMsg, returnDisplay: `No matches found` }; + return { + llmContent: noMatchMsg, + returnDisplay: { + summary: `No matches found`, + }, + }; } const wasTruncated = allMatches.length >= totalMaxMatches; @@ -300,7 +305,9 @@ class GrepToolInvocation extends BaseToolInvocation< const errorMessage = getErrorMessage(error); return { llmContent: `Error during grep search operation: ${errorMessage}`, - returnDisplay: `Error: ${errorMessage}`, + returnDisplay: { + summary: `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 4bc57b8d32..68c3185749 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -16,6 +16,7 @@ import { ToolErrorType } from './tool-error.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; import { GEMINI_IGNORE_FILE_NAME } from '../config/constants.js'; +import type { StructuredToolResult } from './tools.js'; describe('LSTool', () => { let lsTool: LSTool; @@ -123,7 +124,9 @@ describe('LSTool', () => { expect(result.llmContent).toContain('[DIR] subdir'); expect(result.llmContent).toContain('file1.txt'); - expect(result.returnDisplay).toBe('Listed 2 item(s).'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 2 item(s).', + ); }); it('should list files from secondary workspace directory', async () => { @@ -138,7 +141,9 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('secondary-file.txt'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 1 item(s).', + ); }); it('should handle empty directories', async () => { @@ -148,7 +153,9 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toBe(`Directory ${emptyDir} is empty.`); - expect(result.returnDisplay).toBe('Directory is empty.'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Directory is empty.', + ); }); it('should respect ignore patterns', async () => { @@ -163,7 +170,9 @@ describe('LSTool', () => { expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('file2.log'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 1 item(s).', + ); }); it('should respect gitignore patterns', async () => { @@ -177,7 +186,9 @@ describe('LSTool', () => { expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('file2.log'); // .git is always ignored by default. - expect(result.returnDisplay).toBe('Listed 2 item(s). (2 ignored)'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 2 item(s). (2 ignored)', + ); }); it('should respect geminiignore patterns', async () => { @@ -192,7 +203,9 @@ describe('LSTool', () => { expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('file2.log'); - expect(result.returnDisplay).toBe('Listed 2 item(s). (1 ignored)'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 2 item(s). (1 ignored)', + ); }); it('should handle non-directory paths', async () => { @@ -203,7 +216,9 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Path is not a directory'); - expect(result.returnDisplay).toBe('Error: Path is not a directory.'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Error: Path is not a directory.', + ); expect(result.error?.type).toBe(ToolErrorType.PATH_IS_NOT_A_DIRECTORY); }); @@ -213,7 +228,9 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('Error listing directory'); - expect(result.returnDisplay).toBe('Error: Failed to list directory.'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Error: Failed to list directory.', + ); expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); @@ -253,7 +270,9 @@ 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.returnDisplay as StructuredToolResult).summary).toBe( + 'Error: Failed to list directory.', + ); expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); @@ -279,7 +298,9 @@ describe('LSTool', () => { // Should still list the other files expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('problematic.txt'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 1 item(s).', + ); statSpy.mockRestore(); }); @@ -339,7 +360,9 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('secondary-file.txt'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect((result.returnDisplay as StructuredToolResult).summary).toBe( + 'Listed 1 item(s).', + ); }); }); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index a4645fb8a3..f7de20c240 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -124,8 +124,10 @@ class LSToolInvocation extends BaseToolInvocation { ): ToolResult { return { llmContent, - // Keep returnDisplay simpler in core logic - returnDisplay: `Error: ${returnDisplay}`, + // Return an object with summary for dense output support + returnDisplay: { + summary: `Error: ${returnDisplay}`, + }, error: { message: llmContent, type, @@ -150,7 +152,9 @@ class LSToolInvocation extends BaseToolInvocation { if (validationError) { return { llmContent: validationError, - returnDisplay: 'Path not in workspace.', + returnDisplay: { + summary: 'Path not in workspace.', + }, error: { message: validationError, type: ToolErrorType.PATH_NOT_IN_WORKSPACE, @@ -182,7 +186,9 @@ class LSToolInvocation extends BaseToolInvocation { // Changed error message to be more neutral for LLM return { llmContent: `Directory ${resolvedDirPath} is empty.`, - returnDisplay: `Directory is empty.`, + returnDisplay: { + summary: `Directory is empty.`, + }, }; } diff --git a/packages/core/src/tools/read-many-files.test.ts b/packages/core/src/tools/read-many-files.test.ts index f340424a35..1bd8c36d9c 100644 --- a/packages/core/src/tools/read-many-files.test.ts +++ b/packages/core/src/tools/read-many-files.test.ts @@ -23,6 +23,7 @@ import { } from '../utils/ignorePatterns.js'; import * as glob from 'glob'; import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +import type { StructuredToolResult } from './tools.js'; import { GEMINI_IGNORE_FILE_NAME } from '../config/constants.js'; vi.mock('glob', { spy: true }); @@ -260,7 +261,7 @@ describe('ReadManyFilesTool', () => { `--- ${expectedPath} ---\n\nContent of file1\n\n`, `\n--- End of content ---`, ]); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **1 file(s)**', ); }); @@ -284,7 +285,7 @@ describe('ReadManyFilesTool', () => { c.includes(`--- ${expectedPath2} ---\n\nContent2\n\n`), ), ).toBe(true); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **2 file(s)**', ); }); @@ -310,7 +311,7 @@ describe('ReadManyFilesTool', () => { ), ).toBe(true); expect(content.find((c) => c.includes('sub/data.json'))).toBeUndefined(); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **2 file(s)**', ); }); @@ -330,7 +331,7 @@ describe('ReadManyFilesTool', () => { expect( content.find((c) => c.includes('src/main.test.ts')), ).toBeUndefined(); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **1 file(s)**', ); }); @@ -342,7 +343,7 @@ describe('ReadManyFilesTool', () => { expect(result.llmContent).toEqual([ 'No files matching the criteria were found or all were skipped.', ]); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'No files were read and concatenated based on the criteria.', ); }); @@ -362,7 +363,7 @@ describe('ReadManyFilesTool', () => { expect( content.find((c) => c.includes('node_modules/some-lib/index.js')), ).toBeUndefined(); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **1 file(s)**', ); }); @@ -389,7 +390,7 @@ describe('ReadManyFilesTool', () => { c.includes(`--- ${expectedPath2} ---\n\napp code\n\n`), ), ).toBe(true); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **2 file(s)**', ); }); @@ -413,7 +414,7 @@ describe('ReadManyFilesTool', () => { }, '\n--- End of content ---', ]); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **1 file(s)**', ); }); @@ -454,8 +455,10 @@ describe('ReadManyFilesTool', () => { c.includes(`--- ${expectedPath} ---\n\ntext notes\n\n`), ), ).toBe(true); - expect(result.returnDisplay).toContain('**Skipped 1 item(s):**'); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( + '**Skipped 1 item(s):**', + ); + expect((result.returnDisplay as StructuredToolResult).summary).toContain( '- `document.pdf` (Reason: asset file (image/pdf/audio) was not explicitly requested by name or extension)', ); }); @@ -501,7 +504,9 @@ describe('ReadManyFilesTool', () => { const result = await invocation.execute(new AbortController().signal); expect(result.returnDisplay).not.toContain('foo.bar'); expect(result.returnDisplay).not.toContain('foo.quux'); - expect(result.returnDisplay).toContain('bar.ts'); + expect((result.returnDisplay as StructuredToolResult).summary).toContain( + 'bar.ts', + ); }); it('should read files from multiple workspace directories', async () => { @@ -577,7 +582,7 @@ describe('ReadManyFilesTool', () => { c.includes(`--- ${expectedPath2} ---\n\nContent2\n\n`), ), ).toBe(true); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **2 file(s)**', ); @@ -629,7 +634,7 @@ Content of receive-detail `, `\n--- End of content ---`, ]); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **1 file(s)**', ); }); @@ -648,7 +653,7 @@ Content of file[1] `, `\n--- End of content ---`, ]); - expect(result.returnDisplay).toContain( + expect((result.returnDisplay as StructuredToolResult).summary).toContain( 'Successfully read and concatenated content from **1 file(s)**', ); }); @@ -747,7 +752,9 @@ Content of file[1] // Should successfully process valid files despite one failure expect(content.length).toBeGreaterThanOrEqual(3); - expect(result.returnDisplay).toContain('Successfully read'); + expect((result.returnDisplay as StructuredToolResult).summary).toContain( + 'Successfully read', + ); // Verify valid files were processed const expectedPath1 = path.join(tempRootDir, 'valid1.txt'); diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index 9f35927439..29d6fded6a 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -250,7 +250,9 @@ ${finalExclusionPatternsForDescription const errorMessage = `Error during file search: ${getErrorMessage(error)}`; return { llmContent: errorMessage, - returnDisplay: `## File Search Error\n\nAn error occurred while searching for files:\n\`\`\`\n${getErrorMessage(error)}\n\`\`\``, + returnDisplay: { + summary: `Error: ${getErrorMessage(error)}`, + }, error: { message: errorMessage, type: ToolErrorType.READ_MANY_FILES_SEARCH_ERROR, @@ -447,10 +449,7 @@ ${finalExclusionPatternsForDescription } const returnDisplay: ReadManyFilesResult = { - summary: - processedFilesRelativePaths.length > 0 - ? `Read ${processedFilesRelativePaths.length} file(s)` - : 'No files read', + summary: displayMessage.trim(), files: processedFilesRelativePaths, skipped: skippedFiles, include: this.params.include, diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 07656285f0..3b07aba00a 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -664,9 +664,12 @@ export interface TodoList { todos: Todo[]; } -export interface GrepResult { +export interface StructuredToolResult { summary: string; - matches: Array<{ +} + +export interface GrepResult extends StructuredToolResult { + matches?: Array<{ filePath: string; lineNumber: number; line: string; @@ -674,15 +677,13 @@ export interface GrepResult { payload?: string; } -export interface ListDirectoryResult { - summary: string; - files: string[]; +export interface ListDirectoryResult extends StructuredToolResult { + files?: string[]; payload?: string; } -export interface ReadManyFilesResult { - summary: string; - files: string[]; +export interface ReadManyFilesResult extends StructuredToolResult { + files?: string[]; skipped?: Array<{ path: string; reason: string }>; include?: string[]; excludes?: string[];