mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-16 17:11:04 -07:00
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.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
})),
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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),
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -142,8 +142,10 @@ class LSToolInvocation extends BaseToolInvocation<LSToolParams, ToolResult> {
|
||||
): 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<LSToolParams, ToolResult> {
|
||||
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<LSToolParams, ToolResult> {
|
||||
// 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<LSToolParams, ToolResult> {
|
||||
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) {
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user