feat(core): introduce structured tool results for grep, ls, and read-many-files

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