From a6c26dd91a1ce03685ab9181c33074e640bdff76 Mon Sep 17 00:00:00 2001 From: Jarrod Whelan <150866123+jwhelangoog@users.noreply.github.com> Date: Mon, 9 Mar 2026 16:56:25 -0700 Subject: [PATCH] Tool Content Fixes/Changes * fix(core): use full file content for accurate diff stats in edit tool Ensures the edit tool uses fully calculated new file content (instead of just the replacement snippet) when generating diff stats. This prevents the bug where 'mass deletion' were inaccurately reports in the UI. The fix calculates the 'AI-recommended' full file state when user modifications are present. This allows getDiffStat to receive three full-file strings, maintaining accurate telemetry for AI vs. user contributions while fixing the visual bug. * feat(ui): structured compact payloads for list and grep tool output leverages structured object return types (ReadManyFilesResult, ListDirectoryResult, GrepResult) to enable rich, expandable compact output in the CLI. Updates the underlying core tools (read_many_files, ls, grep) to return these objects and wires up the UI in DenseToolMessage to parse and render them as expandable lists. --- packages/core/src/tools/edit.ts | 24 +++++++++- packages/core/src/tools/grep-utils.ts | 27 ++++++++--- packages/core/src/tools/grep.test.ts | 26 ++++++++--- packages/core/src/tools/ls.test.ts | 53 +++++++++++++++++----- packages/core/src/tools/ls.ts | 18 ++++++-- packages/core/src/tools/read-many-files.ts | 17 ++++++- packages/core/src/tools/ripGrep.test.ts | 37 +++++++++++---- 7 files changed, 161 insertions(+), 41 deletions(-) diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 06f9657745..6b9efdaafa 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -895,11 +895,33 @@ class EditToolInvocation DEFAULT_DIFF_OPTIONS, ); + // Determine the full content as originally proposed by the AI to ensure accurate diff stats. + let fullAiProposedContent = editData.newContent; + if ( + this.params.modified_by_user && + this.params.ai_proposed_content !== undefined + ) { + try { + const aiReplacement = await calculateReplacement(this.config, { + params: { + ...this.params, + new_string: this.params.ai_proposed_content, + }, + currentContent: editData.currentContent ?? '', + abortSignal: signal, + }); + fullAiProposedContent = aiReplacement.newContent; + } catch (_e) { + // Fallback to newContent if speculative calculation fails + fullAiProposedContent = editData.newContent; + } + } + const diffStat = getDiffStat( fileName, editData.currentContent ?? '', + fullAiProposedContent, editData.newContent, - this.params.new_string, ); displayResult = { fileDiff, diff --git a/packages/core/src/tools/grep-utils.ts b/packages/core/src/tools/grep-utils.ts index 2191588301..4d9ab6fb27 100644 --- a/packages/core/src/tools/grep-utils.ts +++ b/packages/core/src/tools/grep-utils.ts @@ -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,17 @@ 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`, + }, + }; } const matchesByFile = groupMatchesByFile(allMatches); @@ -181,7 +187,9 @@ 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)' : ''}`, + }, }; } @@ -213,8 +221,15 @@ 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, + lineNumber: m.lineNumber, + line: m.line, + })), + }, }; } diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 1f0a8ee98f..ea0d03065c 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -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'; @@ -180,7 +180,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 () => { @@ -221,7 +223,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 () => { @@ -238,7 +242,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 () => { @@ -258,7 +264,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 () => { @@ -268,7 +276,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 () => { @@ -480,7 +490,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 () => { diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index 63d7693123..12e050d26e 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -123,7 +123,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 () => { @@ -138,7 +141,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 () => { @@ -148,7 +154,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).toEqual( + expect.objectContaining({ summary: 'Directory is empty.' }), + ); }); it('should respect ignore patterns', async () => { @@ -163,7 +171,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 () => { @@ -177,7 +188,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 () => { @@ -192,7 +205,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 () => { @@ -203,7 +218,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).toEqual( + expect.objectContaining({ summary: 'Error: Path is not a directory.' }), + ); expect(result.error?.type).toBe(ToolErrorType.PATH_IS_NOT_A_DIRECTORY); }); @@ -213,7 +230,11 @@ 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).toEqual( + expect.objectContaining({ + summary: 'Error: Failed to list directory.', + }), + ); expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); @@ -253,7 +274,11 @@ 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).toEqual( + expect.objectContaining({ + summary: 'Error: Failed to list directory.', + }), + ); expect(result.error?.type).toBe(ToolErrorType.LS_EXECUTION_ERROR); }); @@ -279,7 +304,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(); }); @@ -339,7 +367,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), + }); }); }); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 972c857cbd..3753ab9317 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -142,8 +142,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, @@ -168,7 +170,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, @@ -200,7 +204,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.`, + }, }; } @@ -279,7 +285,9 @@ class LSToolInvocation extends BaseToolInvocation { llmContent: resultMessage, returnDisplay: { summary: displayMessage, - files: entries.map((e) => (e.isDirectory ? `${e.name}/` : e.name)), + files: entries.map( + (entry) => `${entry.isDirectory ? '[DIR] ' : ''}${entry.name}`, + ), }, }; } catch (error) { diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index e37caff315..85c9fecca4 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -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'; @@ -267,7 +268,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, @@ -462,9 +465,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, }; } } diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index a1b155fb7a..4481bf3e54 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -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 () => {