mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-24 13:01:29 -07:00
refactor(cli,core): foundational layout, identity management, and type safety
This commit establishes the structural foundation and required infrastructure to support the upcoming compact tool output changes. It includes identity management improvements, layout fixes, and type-safety enhancements that stand independently. 1. Identity & History Management: - useHistoryManager: Ensure strictly increasing IDs for history items, even with identical timestamps. - acpClient: Introduced callIdCounter to prevent callId collisions during rapid execution. - MainContent: Implemented negative ID logic for pending items to ensure stable React keys and prevent collisions. - historyUtils: New file containing extracted history logic (isToolExecuting, getAllToolCalls) hoisted from AppContainer. 2. UI Infrastructure & Layout: - test-utils/render: Modernized renderWithProviders and removed legacy props. - AskUserDialog: Fixed layout, scroll visibility, and alignment issues. - toolLayoutUtils: Increased TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT for better spacing. - ShellToolMessage/ToolGroupMessage: Updated line-count expectations and snapshots to align with layout changes. 3. IDE & Diffing Improvements: - ToolActionsContext: Refactored IdeClient initialization to fix a race condition and potential memory leak. - edit/diffOptions: Improved accuracy of diff stat derivation, ensuring "full content" stats are available for the model. - coreToolScheduler: Automatically derive diff stats from patches if missing. - state-manager: Ensure diffStat preservation for rejected tool calls. 4. Type Safety & Constants: - types/tools: Added foundational types like DiffStat, FileDiff, and StructuredToolResult. - Type Guards: Added guards for isFileDiff, isTodoList, isAnsiOutput, and hasSummary. - CodeColorizer: Added function overloads to gracefully handle null language detection. - tool-names: Introduced DISPLAY_NAME constants for consistent tool labeling. This commit passes all workspace tests and builds successfully. Feature-specific logic for compact output is excluded.
This commit is contained in:
@@ -8,6 +8,7 @@ import { type FunctionCall } from '@google/genai';
|
||||
import type {
|
||||
ToolConfirmationOutcome,
|
||||
ToolConfirmationPayload,
|
||||
DiffStat,
|
||||
} from '../tools/tools.js';
|
||||
import type { ToolCall } from '../scheduler/types.js';
|
||||
|
||||
@@ -88,6 +89,7 @@ export type SerializableConfirmationDetails =
|
||||
originalContent: string | null;
|
||||
newContent: string;
|
||||
isModifying?: boolean;
|
||||
diffStat?: DiffStat;
|
||||
}
|
||||
| {
|
||||
type: 'exec';
|
||||
|
||||
@@ -23,6 +23,7 @@ import {
|
||||
getToolSuggestion,
|
||||
isToolCallResponseInfo,
|
||||
} from '../utils/tool-utils.js';
|
||||
import { getDiffStatFromPatch } from '../tools/diffOptions.js';
|
||||
import type { ToolConfirmationRequest } from '../confirmation-bus/types.js';
|
||||
import { MessageBusType } from '../confirmation-bus/types.js';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
@@ -312,6 +313,12 @@ export class CoreToolScheduler {
|
||||
waitingCall.confirmationDetails.originalContent,
|
||||
newContent: waitingCall.confirmationDetails.newContent,
|
||||
filePath: waitingCall.confirmationDetails.filePath,
|
||||
// Derive stats from the patch if they aren't already present
|
||||
diffStat:
|
||||
waitingCall.confirmationDetails.diffStat ??
|
||||
getDiffStatFromPatch(
|
||||
waitingCall.confirmationDetails.fileDiff,
|
||||
),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -22,6 +22,7 @@ import {
|
||||
ToolConfirmationOutcome,
|
||||
type AnyDeclarativeTool,
|
||||
type AnyToolInvocation,
|
||||
type FileDiff,
|
||||
} from '../tools/tools.js';
|
||||
import { MessageBusType } from '../confirmation-bus/types.js';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
@@ -359,7 +360,7 @@ describe('SchedulerStateManager', () => {
|
||||
expect(active.confirmationDetails).toEqual(details);
|
||||
});
|
||||
|
||||
it('should preserve diff when cancelling an edit tool call', () => {
|
||||
it('should preserve diff and derive stats when cancelling an edit tool call', () => {
|
||||
const call = createValidatingCall();
|
||||
stateManager.enqueue([call]);
|
||||
stateManager.dequeue();
|
||||
@@ -369,9 +370,9 @@ describe('SchedulerStateManager', () => {
|
||||
title: 'Edit',
|
||||
fileName: 'test.txt',
|
||||
filePath: '/path/to/test.txt',
|
||||
fileDiff: 'diff',
|
||||
originalContent: 'old',
|
||||
newContent: 'new',
|
||||
fileDiff: '@@ -1,1 +1,1 @@\n-old line\n+new line',
|
||||
originalContent: 'old line',
|
||||
newContent: 'new line',
|
||||
onConfirm: vi.fn(),
|
||||
};
|
||||
|
||||
@@ -389,13 +390,14 @@ describe('SchedulerStateManager', () => {
|
||||
|
||||
const completed = stateManager.completedBatch[0] as CancelledToolCall;
|
||||
expect(completed.status).toBe(CoreToolCallStatus.Cancelled);
|
||||
expect(completed.response.resultDisplay).toEqual({
|
||||
fileDiff: 'diff',
|
||||
fileName: 'test.txt',
|
||||
filePath: '/path/to/test.txt',
|
||||
originalContent: 'old',
|
||||
newContent: 'new',
|
||||
});
|
||||
const result = completed.response.resultDisplay as FileDiff;
|
||||
expect(result.fileDiff).toBe(details.fileDiff);
|
||||
expect(result.diffStat).toEqual(
|
||||
expect.objectContaining({
|
||||
model_added_lines: 1,
|
||||
model_removed_lines: 1,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('should ignore status updates for non-existent callIds', () => {
|
||||
|
||||
@@ -32,6 +32,7 @@ import {
|
||||
type SerializableConfirmationDetails,
|
||||
} from '../confirmation-bus/types.js';
|
||||
import { isToolCallResponseInfo } from '../utils/tool-utils.js';
|
||||
import { getDiffStatFromPatch } from '../tools/diffOptions.js';
|
||||
|
||||
/**
|
||||
* Handler for terminal tool calls.
|
||||
@@ -473,6 +474,8 @@ export class SchedulerStateManager {
|
||||
filePath: details.filePath,
|
||||
originalContent: details.originalContent,
|
||||
newContent: details.newContent,
|
||||
// Derive stats from the patch if they aren't already present
|
||||
diffStat: details.diffStat ?? getDiffStatFromPatch(details.fileDiff),
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -76,3 +76,39 @@ export function getDiffStat(
|
||||
user_removed_chars: userStats.removedChars,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Extracts line and character stats from a unified diff patch string.
|
||||
* This is useful for reconstructing stats for rejected or errored operations
|
||||
* where the full strings may no longer be easily accessible.
|
||||
*/
|
||||
export function getDiffStatFromPatch(patch: string): DiffStat {
|
||||
let addedLines = 0;
|
||||
let removedLines = 0;
|
||||
let addedChars = 0;
|
||||
let removedChars = 0;
|
||||
|
||||
const lines = patch.split('\n');
|
||||
for (const line of lines) {
|
||||
// Only count lines that are additions or removals,
|
||||
// excluding the diff headers (--- and +++) and metadata (\)
|
||||
if (line.startsWith('+') && !line.startsWith('+++')) {
|
||||
addedLines++;
|
||||
addedChars += line.length - 1;
|
||||
} else if (line.startsWith('-') && !line.startsWith('---')) {
|
||||
removedLines++;
|
||||
removedChars += line.length - 1;
|
||||
}
|
||||
}
|
||||
|
||||
return {
|
||||
model_added_lines: addedLines,
|
||||
model_removed_lines: removedLines,
|
||||
model_added_chars: addedChars,
|
||||
model_removed_chars: removedChars,
|
||||
user_added_lines: 0,
|
||||
user_removed_lines: 0,
|
||||
user_added_chars: 0,
|
||||
user_removed_chars: 0,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -896,11 +896,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,
|
||||
|
||||
@@ -150,8 +150,6 @@ export {
|
||||
SKILL_PARAM_NAME,
|
||||
};
|
||||
|
||||
export const LS_TOOL_NAME_LEGACY = 'list_directory'; // Just to be safe if anything used the old exported name directly
|
||||
|
||||
export const EDIT_TOOL_NAMES = new Set([EDIT_TOOL_NAME, WRITE_FILE_TOOL_NAME]);
|
||||
|
||||
/**
|
||||
@@ -183,6 +181,11 @@ export const EDIT_DISPLAY_NAME = 'Edit';
|
||||
export const ASK_USER_DISPLAY_NAME = 'Ask User';
|
||||
export const READ_FILE_DISPLAY_NAME = 'ReadFile';
|
||||
export const GLOB_DISPLAY_NAME = 'FindFiles';
|
||||
export const LS_DISPLAY_NAME = 'ReadFolder';
|
||||
export const GREP_DISPLAY_NAME = 'SearchText';
|
||||
export const WEB_SEARCH_DISPLAY_NAME = 'GoogleSearch';
|
||||
export const WEB_FETCH_DISPLAY_NAME = 'WebFetch';
|
||||
export const READ_MANY_FILES_DISPLAY_NAME = 'ReadManyFiles';
|
||||
|
||||
/**
|
||||
* Mapping of legacy tool names to their current names.
|
||||
|
||||
@@ -816,6 +816,21 @@ export interface TodoList {
|
||||
|
||||
export type ToolLiveOutput = string | AnsiOutput | SubagentProgress;
|
||||
|
||||
export interface StructuredToolResult {
|
||||
summary: string;
|
||||
}
|
||||
|
||||
export function isStructuredToolResult(
|
||||
obj: unknown,
|
||||
): obj is StructuredToolResult {
|
||||
return (
|
||||
typeof obj === 'object' &&
|
||||
obj !== null &&
|
||||
'summary' in obj &&
|
||||
typeof obj.summary === 'string'
|
||||
);
|
||||
}
|
||||
|
||||
export type ToolResultDisplay =
|
||||
| string
|
||||
| FileDiff
|
||||
@@ -869,6 +884,7 @@ export interface ToolEditConfirmationDetails {
|
||||
originalContent: string | null;
|
||||
newContent: string;
|
||||
isModifying?: boolean;
|
||||
diffStat?: DiffStat;
|
||||
ideConfirmation?: Promise<DiffUpdateResult>;
|
||||
}
|
||||
|
||||
|
||||
@@ -30,7 +30,7 @@ import {
|
||||
NetworkRetryAttemptEvent,
|
||||
} from '../telemetry/index.js';
|
||||
import { LlmRole } from '../telemetry/llmRole.js';
|
||||
import { WEB_FETCH_TOOL_NAME } from './tool-names.js';
|
||||
import { WEB_FETCH_TOOL_NAME, WEB_FETCH_DISPLAY_NAME } from './tool-names.js';
|
||||
import { debugLogger } from '../utils/debugLogger.js';
|
||||
import { coreEvents } from '../utils/events.js';
|
||||
import { retryWithBackoff, getRetryErrorType } from '../utils/retry.js';
|
||||
@@ -891,7 +891,7 @@ export class WebFetchTool extends BaseDeclarativeTool<
|
||||
) {
|
||||
super(
|
||||
WebFetchTool.Name,
|
||||
'WebFetch',
|
||||
WEB_FETCH_DISPLAY_NAME,
|
||||
WEB_FETCH_DEFINITION.base.description!,
|
||||
Kind.Fetch,
|
||||
WEB_FETCH_DEFINITION.base.parametersJsonSchema,
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
*/
|
||||
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import { WEB_SEARCH_TOOL_NAME } from './tool-names.js';
|
||||
import { WEB_SEARCH_TOOL_NAME, WEB_SEARCH_DISPLAY_NAME } from './tool-names.js';
|
||||
import type { GroundingMetadata } from '@google/genai';
|
||||
import {
|
||||
BaseDeclarativeTool,
|
||||
@@ -212,7 +212,7 @@ export class WebSearchTool extends BaseDeclarativeTool<
|
||||
) {
|
||||
super(
|
||||
WebSearchTool.Name,
|
||||
'GoogleSearch',
|
||||
WEB_SEARCH_DISPLAY_NAME,
|
||||
WEB_SEARCH_DEFINITION.base.description!,
|
||||
Kind.Search,
|
||||
WEB_SEARCH_DEFINITION.base.parametersJsonSchema,
|
||||
|
||||
Reference in New Issue
Block a user