feat(cli): implement compact tool output (#20974)

This commit is contained in:
Jarrod Whelan
2026-03-30 16:43:29 -07:00
committed by GitHub
parent 3e95b8ec59
commit 1df5c98b33
45 changed files with 2670 additions and 386 deletions
+24 -6
View File
@@ -7,6 +7,7 @@
import fsPromises from 'node:fs/promises';
import { debugLogger } from '../utils/debugLogger.js';
import { MAX_LINE_LENGTH_TEXT_FILE } from '../utils/constants.js';
import type { GrepResult } from './tools.js';
/**
* Result object for a single grep match
@@ -148,12 +149,18 @@ export async function formatGrepResults(
},
searchLocationDescription: string,
totalMaxMatches: number,
): Promise<{ llmContent: string; returnDisplay: string }> {
): Promise<{ llmContent: string; returnDisplay: GrepResult }> {
const { pattern, names_only, include_pattern } = params;
if (allMatches.length === 0) {
const noMatchMsg = `No matches found for pattern "${pattern}" ${searchLocationDescription}${include_pattern ? ` (filter: "${include_pattern}")` : ''}.`;
return { llmContent: noMatchMsg, returnDisplay: `No matches found` };
return {
llmContent: noMatchMsg,
returnDisplay: {
summary: 'No matches found',
matches: [],
},
};
}
const matchesByFile = groupMatchesByFile(allMatches);
@@ -181,7 +188,10 @@ export async function formatGrepResults(
llmContent += filePaths.join('\n');
return {
llmContent: llmContent.trim(),
returnDisplay: `Found ${filePaths.length} files${wasTruncated ? ' (limited)' : ''}`,
returnDisplay: {
summary: `Found ${filePaths.length} files${wasTruncated ? ' (limited)' : ''}`,
matches: [],
},
};
}
@@ -213,8 +223,16 @@ export async function formatGrepResults(
return {
llmContent: llmContent.trim(),
returnDisplay: `Found ${matchCount} ${matchTerm}${
wasTruncated ? ' (limited)' : ''
}`,
returnDisplay: {
summary: `Found ${matchCount} ${matchTerm}${wasTruncated ? ' (limited)' : ''}`,
matches: allMatches
.filter((m) => !m.isContext)
.map((m) => ({
filePath: m.filePath,
absolutePath: m.absolutePath,
lineNumber: m.lineNumber,
line: m.line,
})),
},
};
}
+19 -7
View File
@@ -6,7 +6,7 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { GrepTool, type GrepToolParams } from './grep.js';
import type { ToolResult } from './tools.js';
import type { ToolResult, GrepResult } from './tools.js';
import path from 'node:path';
import { isSubpath } from '../utils/paths.js';
import fs from 'node:fs/promises';
@@ -187,7 +187,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 GrepResult)?.summary).toBe(
'Found 3 matches',
);
}, 30000);
it('should include files that start with ".." in JS fallback', async () => {
@@ -228,7 +230,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 GrepResult)?.summary).toBe(
'Found 1 match',
);
}, 30000);
it('should find matches with an include glob', async () => {
@@ -245,7 +249,9 @@ describe('GrepTool', () => {
expect(result.llmContent).toContain(
'L2: function baz() { return "hello"; }',
);
expect(result.returnDisplay).toBe('Found 1 match');
expect((result.returnDisplay as GrepResult)?.summary).toBe(
'Found 1 match',
);
}, 30000);
it('should find matches with an include glob and path', async () => {
@@ -265,7 +271,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 GrepResult)?.summary).toBe(
'Found 1 match',
);
}, 30000);
it('should return "No matches found" when pattern does not exist', async () => {
@@ -275,7 +283,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 GrepResult)?.summary).toBe(
'No matches found',
);
}, 30000);
it('should handle regex special characters correctly', async () => {
@@ -501,7 +511,9 @@ describe('GrepTool', () => {
expect(result.llmContent).toContain('L2: second line with world');
// And sub/fileC.txt should be excluded because limit reached
expect(result.llmContent).not.toContain('File: sub/fileC.txt');
expect(result.returnDisplay).toBe('Found 2 matches (limited)');
expect((result.returnDisplay as GrepResult)?.summary).toBe(
'Found 2 matches (limited)',
);
});
it('should respect max_matches_per_file in JS fallback', async () => {
+2 -2
View File
@@ -30,7 +30,7 @@ import { isGitRepository } from '../utils/gitUtils.js';
import type { Config } from '../config/config.js';
import type { FileExclusions } from '../utils/ignorePatterns.js';
import { ToolErrorType } from './tool-error.js';
import { GREP_TOOL_NAME } from './tool-names.js';
import { GREP_TOOL_NAME, GREP_DISPLAY_NAME } from './tool-names.js';
import { buildPatternArgsPattern } from '../policy/utils.js';
import { debugLogger } from '../utils/debugLogger.js';
import { GREP_DEFINITION } from './definitions/coreTools.js';
@@ -653,7 +653,7 @@ export class GrepTool extends BaseDeclarativeTool<GrepToolParams, ToolResult> {
) {
super(
GrepTool.Name,
'SearchText',
GREP_DISPLAY_NAME,
GREP_DEFINITION.base.description!,
Kind.Search,
GREP_DEFINITION.base.parametersJsonSchema,
+26 -7
View File
@@ -131,7 +131,10 @@ 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).toEqual({
summary: 'Listed 2 item(s).',
files: ['[DIR] subdir', 'file1.txt'],
});
});
it('should list files from secondary workspace directory', async () => {
@@ -146,7 +149,10 @@ 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).toEqual({
summary: 'Listed 1 item(s).',
files: expect.any(Array),
});
});
it('should handle empty directories', async () => {
@@ -171,7 +177,10 @@ 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).toEqual({
summary: 'Listed 1 item(s).',
files: expect.any(Array),
});
});
it('should respect gitignore patterns', async () => {
@@ -185,7 +194,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).toEqual(
expect.objectContaining({ summary: 'Listed 2 item(s). (2 ignored)' }),
);
});
it('should respect geminiignore patterns', async () => {
@@ -200,7 +211,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).toEqual(
expect.objectContaining({ summary: 'Listed 2 item(s). (1 ignored)' }),
);
});
it('should handle non-directory paths', async () => {
@@ -287,7 +300,10 @@ 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).toEqual({
summary: 'Listed 1 item(s).',
files: expect.any(Array),
});
statSpy.mockRestore();
});
@@ -347,7 +363,10 @@ 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).toEqual({
summary: 'Listed 1 item(s).',
files: expect.any(Array),
});
});
});
+8 -4
View File
@@ -20,7 +20,7 @@ import { makeRelative, shortenPath } from '../utils/paths.js';
import type { Config } from '../config/config.js';
import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js';
import { ToolErrorType } from './tool-error.js';
import { LS_TOOL_NAME } from './tool-names.js';
import { LS_TOOL_NAME, LS_DISPLAY_NAME } from './tool-names.js';
import { buildDirPathArgsPattern } from '../policy/utils.js';
import { debugLogger } from '../utils/debugLogger.js';
import { LS_DEFINITION } from './definitions/coreTools.js';
@@ -143,7 +143,6 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
): ToolResult {
return {
llmContent,
// Keep returnDisplay simpler in core logic
returnDisplay: `Error: ${returnDisplay}`,
error: {
message: llmContent,
@@ -284,7 +283,12 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
return {
llmContent: resultMessage,
returnDisplay: displayMessage,
returnDisplay: {
summary: displayMessage,
files: entries.map(
(entry) => `${entry.isDirectory ? '[DIR] ' : ''}${entry.name}`,
),
},
};
} catch (error) {
const errorMsg = `Error listing directory: ${error instanceof Error ? error.message : String(error)}`;
@@ -309,7 +313,7 @@ export class LSTool extends BaseDeclarativeTool<LSToolParams, ToolResult> {
) {
super(
LSTool.Name,
'ReadFolder',
LS_DISPLAY_NAME,
LS_DEFINITION.base.description!,
Kind.Search,
LS_DEFINITION.base.parametersJsonSchema,
+28 -17
View File
@@ -31,6 +31,7 @@ import {
import * as glob from 'glob';
import { createMockMessageBus } from '../test-utils/mock-message-bus.js';
import { GEMINI_IGNORE_FILE_NAME } from '../config/constants.js';
import type { ReadManyFilesResult } from './tools.js';
vi.mock('glob', { spy: true });
@@ -277,7 +278,7 @@ describe('ReadManyFilesTool', () => {
`--- ${expectedPath} ---\n\nContent of file1\n\n`,
`\n--- End of content ---`,
]);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **1 file(s)**',
);
});
@@ -301,7 +302,7 @@ describe('ReadManyFilesTool', () => {
c.includes(`--- ${expectedPath2} ---\n\nContent2\n\n`),
),
).toBe(true);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **2 file(s)**',
);
});
@@ -327,7 +328,7 @@ describe('ReadManyFilesTool', () => {
),
).toBe(true);
expect(content.find((c) => c.includes('sub/data.json'))).toBeUndefined();
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **2 file(s)**',
);
});
@@ -347,7 +348,7 @@ describe('ReadManyFilesTool', () => {
expect(
content.find((c) => c.includes('src/main.test.ts')),
).toBeUndefined();
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **1 file(s)**',
);
});
@@ -359,7 +360,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 ReadManyFilesResult).summary).toContain(
'No files were read and concatenated based on the criteria.',
);
});
@@ -379,7 +380,7 @@ describe('ReadManyFilesTool', () => {
expect(
content.find((c) => c.includes('node_modules/some-lib/index.js')),
).toBeUndefined();
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **1 file(s)**',
);
});
@@ -406,7 +407,7 @@ describe('ReadManyFilesTool', () => {
c.includes(`--- ${expectedPath2} ---\n\napp code\n\n`),
),
).toBe(true);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **2 file(s)**',
);
});
@@ -430,7 +431,7 @@ describe('ReadManyFilesTool', () => {
},
'\n--- End of content ---',
]);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **1 file(s)**',
);
});
@@ -471,8 +472,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 ReadManyFilesResult).summary).toContain(
'**Skipped 1 item(s):**',
);
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'- `document.pdf` (Reason: asset file (image/pdf/audio) was not explicitly requested by name or extension)',
);
});
@@ -516,9 +519,15 @@ describe('ReadManyFilesTool', () => {
const params = { include: ['foo.bar', 'bar.ts', 'foo.quux'] };
const invocation = tool.build(params);
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 ReadManyFilesResult).files).not.toContain(
'foo.bar',
);
expect((result.returnDisplay as ReadManyFilesResult).files).not.toContain(
'foo.quux',
);
expect((result.returnDisplay as ReadManyFilesResult).files).toContain(
'bar.ts',
);
});
it('should read files from multiple workspace directories', async () => {
@@ -594,7 +603,7 @@ describe('ReadManyFilesTool', () => {
c.includes(`--- ${expectedPath2} ---\n\nContent2\n\n`),
),
).toBe(true);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **2 file(s)**',
);
@@ -646,7 +655,7 @@ Content of receive-detail
`,
`\n--- End of content ---`,
]);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **1 file(s)**',
);
});
@@ -665,7 +674,7 @@ Content of file[1]
`,
`\n--- End of content ---`,
]);
expect(result.returnDisplay).toContain(
expect((result.returnDisplay as ReadManyFilesResult).summary).toContain(
'Successfully read and concatenated content from **1 file(s)**',
);
});
@@ -764,7 +773,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 ReadManyFilesResult).summary).toContain(
'Successfully read',
);
// Verify valid files were processed
const expectedPath1 = path.join(tempRootDir, 'valid1.txt');
+18 -4
View File
@@ -13,6 +13,7 @@ import {
type ToolResult,
type PolicyUpdateOptions,
type ToolConfirmationOutcome,
type ReadManyFilesResult,
} from './tools.js';
import { getErrorMessage } from '../utils/errors.js';
import * as fsPromises from 'node:fs/promises';
@@ -36,7 +37,10 @@ import { getProgrammingLanguage } from '../telemetry/telemetry-utils.js';
import { logFileOperation } from '../telemetry/loggers.js';
import { FileOperationEvent } from '../telemetry/types.js';
import { ToolErrorType } from './tool-error.js';
import { READ_MANY_FILES_TOOL_NAME } from './tool-names.js';
import {
READ_MANY_FILES_TOOL_NAME,
READ_MANY_FILES_DISPLAY_NAME,
} from './tool-names.js';
import { READ_MANY_FILES_DEFINITION } from './definitions/coreTools.js';
import { resolveToolDeclaration } from './definitions/resolver.js';
@@ -269,7 +273,7 @@ ${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: `Error: ${getErrorMessage(error)}`,
error: {
message: errorMessage,
type: ToolErrorType.READ_MANY_FILES_SEARCH_ERROR,
@@ -483,9 +487,19 @@ ${finalExclusionPatternsForDescription
'No files matching the criteria were found or all were skipped.',
);
}
const returnDisplay: ReadManyFilesResult = {
summary: displayMessage.trim(),
files: processedFilesRelativePaths,
skipped: skippedFiles,
include: this.params.include,
excludes: effectiveExcludes,
targetDir: this.config.getTargetDir(),
};
return {
llmContent: contentParts,
returnDisplay: displayMessage.trim(),
returnDisplay,
};
}
}
@@ -507,7 +521,7 @@ export class ReadManyFilesTool extends BaseDeclarativeTool<
) {
super(
ReadManyFilesTool.Name,
'ReadManyFiles',
READ_MANY_FILES_DISPLAY_NAME,
READ_MANY_FILES_DEFINITION.base.description!,
Kind.Read,
READ_MANY_FILES_DEFINITION.base.parametersJsonSchema,
+28 -9
View File
@@ -19,6 +19,7 @@ import {
ensureRgPath,
type RipGrepToolParams,
} from './ripGrep.js';
import type { GrepResult } from './tools.js';
import path from 'node:path';
import { isSubpath } from '../utils/paths.js';
import fs from 'node:fs/promises';
@@ -447,7 +448,9 @@ describe('RipGrepTool', () => {
`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 GrepResult).summary).toBe(
'Found 3 matches',
);
});
it('should ignore matches that escape the base path', async () => {
@@ -509,7 +512,9 @@ describe('RipGrepTool', () => {
);
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 GrepResult).summary).toBe(
'Found 1 match',
);
});
it('should find matches with an include glob', async () => {
@@ -542,7 +547,9 @@ describe('RipGrepTool', () => {
expect(result.llmContent).toContain(
'L2: function baz() { return "hello"; }',
);
expect(result.returnDisplay).toBe('Found 1 match');
expect((result.returnDisplay as GrepResult).summary).toBe(
'Found 1 match',
);
});
it('should find matches with an include glob and path', async () => {
@@ -579,7 +586,9 @@ describe('RipGrepTool', () => {
);
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 GrepResult).summary).toBe(
'Found 1 match',
);
});
it('should return "No matches found" when pattern does not exist', async () => {
@@ -596,7 +605,9 @@ describe('RipGrepTool', () => {
expect(result.llmContent).toContain(
'No matches found for pattern "nonexistentpattern" in path ".".',
);
expect(result.returnDisplay).toBe('No matches found');
expect((result.returnDisplay as GrepResult).summary).toBe(
'No matches found',
);
});
it('should throw error for invalid regex pattern during build', async () => {
@@ -689,7 +700,9 @@ describe('RipGrepTool', () => {
});
const result = await invocation.execute(abortSignal);
expect(result.returnDisplay).toContain('(limited)');
expect((result.returnDisplay as GrepResult).summary).toContain(
'(limited)',
);
}, 10000);
it('should filter out files based on FileDiscoveryService even if ripgrep returns them', async () => {
@@ -740,7 +753,9 @@ describe('RipGrepTool', () => {
expect(result.llmContent).toContain('should be kept');
expect(result.llmContent).not.toContain('ignored.txt');
expect(result.llmContent).not.toContain('should be ignored');
expect(result.returnDisplay).toContain('Found 1 match');
expect((result.returnDisplay as GrepResult).summary).toContain(
'Found 1 match',
);
});
it('should handle regex special characters correctly', async () => {
@@ -1064,7 +1079,9 @@ describe('RipGrepTool', () => {
controller.abort();
const result = await invocation.execute(controller.signal);
expect(result.returnDisplay).toContain('No matches found');
expect((result.returnDisplay as GrepResult).summary).toContain(
'No matches found',
);
});
});
@@ -1946,7 +1963,9 @@ describe('RipGrepTool', () => {
expect(result.llmContent).toContain('L1: match 1');
expect(result.llmContent).toContain('L2: match 2');
expect(result.llmContent).not.toContain('L3: match 3');
expect(result.returnDisplay).toBe('Found 2 matches (limited)');
expect((result.returnDisplay as GrepResult).summary).toBe(
'Found 2 matches (limited)',
);
});
it('should return only file paths when names_only is true', async () => {
+7 -1
View File
@@ -921,12 +921,18 @@ export const isListResult = (
): res is ListDirectoryResult | ReadManyFilesResult =>
isStructuredToolResult(res) && 'files' in res && Array.isArray(res.files);
export const isReadManyFilesResult = (
res: unknown,
): res is ReadManyFilesResult => isListResult(res) && 'include' in res;
export type ToolResultDisplay =
| string
| FileDiff
| AnsiOutput
| TodoList
| SubagentProgress;
| SubagentProgress
| GrepResult
| ListDirectoryResult
| ReadManyFilesResult;
export type TodoStatus =
| 'pending'