mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-15 00:21:09 -07:00
fix(cli): strengthen identity management and resolve key collisions / key duplication issues
- Improve history ID generation logic to be strictly increasing, preventing "Transition Race" collisions between pending and finalized items.
- Strengthen ID tracking to correctly handle session resumes by ensuring new IDs always start after the highest existing entry.
- Assign unique negative IDs and negatively indexed keys (`pending-${i}`) to pending history items in `MainContent`.
- Add a global `callIdCounter` to `acpClient` to safeguard against sub-millisecond tool call collisions.
- Harden `DenseToolMessage` rendering with stable string keys for conditional UI components.
- Update unit tests to verify strictly increasing ID generation and align with negatively indexed pending IDs.
This commit is contained in:
@@ -96,6 +96,8 @@ export async function runAcpClient(
|
||||
await connection.closed.finally(runExitCleanup);
|
||||
}
|
||||
|
||||
let callIdCounter = 0;
|
||||
|
||||
export class GeminiAgent {
|
||||
private sessions: Map<string, Session> = new Map();
|
||||
private clientCapabilities: acp.ClientCapabilities | undefined;
|
||||
@@ -834,7 +836,7 @@ export class Session {
|
||||
promptId: string,
|
||||
fc: FunctionCall,
|
||||
): Promise<Part[]> {
|
||||
const callId = fc.id ?? `${fc.name}-${Date.now()}`;
|
||||
const callId = fc.id ?? `${fc.name}-${Date.now()}-${++callIdCounter}`;
|
||||
const args = fc.args ?? {};
|
||||
|
||||
const startTime = Date.now();
|
||||
@@ -1295,7 +1297,7 @@ export class Session {
|
||||
include: pathSpecsToRead,
|
||||
};
|
||||
|
||||
const callId = `${readManyFilesTool.name}-${Date.now()}`;
|
||||
const callId = `${readManyFilesTool.name}-${Date.now()}-${++callIdCounter}`;
|
||||
|
||||
try {
|
||||
const invocation = readManyFilesTool.build(toolArgs);
|
||||
|
||||
@@ -96,7 +96,7 @@ describe('getToolGroupBorderAppearance', () => {
|
||||
});
|
||||
|
||||
it('inspects only the last pending tool_group item if current has no tools', () => {
|
||||
const item = { type: 'tool_group' as const, tools: [], id: 1 };
|
||||
const item = { type: 'tool_group' as const, tools: [], id: -1 };
|
||||
const pendingItems = [
|
||||
{
|
||||
type: 'tool_group' as const,
|
||||
@@ -157,7 +157,7 @@ describe('getToolGroupBorderAppearance', () => {
|
||||
confirmationDetails: undefined,
|
||||
} as IndividualToolCallDisplay,
|
||||
],
|
||||
id: 1,
|
||||
id: -1,
|
||||
};
|
||||
const result = getToolGroupBorderAppearance(
|
||||
item,
|
||||
@@ -186,7 +186,7 @@ describe('getToolGroupBorderAppearance', () => {
|
||||
confirmationDetails: undefined,
|
||||
} as IndividualToolCallDisplay,
|
||||
],
|
||||
id: 1,
|
||||
id: -1,
|
||||
};
|
||||
const result = getToolGroupBorderAppearance(
|
||||
item,
|
||||
@@ -275,7 +275,7 @@ describe('getToolGroupBorderAppearance', () => {
|
||||
confirmationDetails: undefined,
|
||||
} as IndividualToolCallDisplay,
|
||||
],
|
||||
id: 1,
|
||||
id: -1,
|
||||
};
|
||||
const result = getToolGroupBorderAppearance(
|
||||
item,
|
||||
@@ -291,7 +291,7 @@ describe('getToolGroupBorderAppearance', () => {
|
||||
});
|
||||
|
||||
it('handles empty tools with active shell turn (isCurrentlyInShellTurn)', () => {
|
||||
const item = { type: 'tool_group' as const, tools: [], id: 1 };
|
||||
const item = { type: 'tool_group' as const, tools: [], id: -1 };
|
||||
|
||||
// active shell turn
|
||||
const result = getToolGroupBorderAppearance(
|
||||
@@ -690,7 +690,7 @@ describe('MainContent', () => {
|
||||
pendingHistoryItems: [
|
||||
{
|
||||
type: 'tool_group',
|
||||
id: 1,
|
||||
id: -1,
|
||||
tools: [
|
||||
{
|
||||
callId: 'call_1',
|
||||
|
||||
@@ -126,7 +126,7 @@ export const MainContent = () => {
|
||||
|
||||
const pendingItems = useMemo(
|
||||
() => (
|
||||
<Box flexDirection="column">
|
||||
<Box flexDirection="column" key="pending-items-group">
|
||||
{pendingHistoryItems.map((item, i) => {
|
||||
const prevType =
|
||||
i === 0
|
||||
@@ -139,12 +139,12 @@ export const MainContent = () => {
|
||||
|
||||
return (
|
||||
<HistoryItemDisplay
|
||||
key={i}
|
||||
key={`pending-${i}`}
|
||||
availableTerminalHeight={
|
||||
uiState.constrainHeight ? staticAreaMaxItemHeight : undefined
|
||||
}
|
||||
terminalWidth={mainAreaWidth}
|
||||
item={{ ...item, id: 0 }}
|
||||
item={{ ...item, id: -(i + 1) }}
|
||||
isPending={true}
|
||||
isExpandable={true}
|
||||
isFirstThinking={isFirstThinking}
|
||||
@@ -153,7 +153,10 @@ export const MainContent = () => {
|
||||
);
|
||||
})}
|
||||
{showConfirmationQueue && confirmingTool && (
|
||||
<ToolConfirmationQueue confirmingTool={confirmingTool} />
|
||||
<ToolConfirmationQueue
|
||||
key="confirmation-queue"
|
||||
confirmingTool={confirmingTool}
|
||||
/>
|
||||
)}
|
||||
</Box>
|
||||
),
|
||||
|
||||
@@ -519,12 +519,12 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
|
||||
{description}
|
||||
</Box>
|
||||
{summary && (
|
||||
<Box marginLeft={1} flexGrow={0}>
|
||||
<Box key="tool-summary" marginLeft={1} flexGrow={0}>
|
||||
{summary}
|
||||
</Box>
|
||||
)}
|
||||
{isAlternateBuffer && diff && (
|
||||
<Box ref={toggleRef} marginLeft={1} flexGrow={1}>
|
||||
<Box key="tool-toggle" ref={toggleRef} marginLeft={1} flexGrow={1}>
|
||||
<Text color={theme.text.link} dimColor>
|
||||
[{isExpanded ? 'Hide Diff' : 'Show Diff'}]
|
||||
</Text>
|
||||
|
||||
@@ -39,6 +39,56 @@ describe('useHistoryManager', () => {
|
||||
expect(result.current.history[0].id).toBeGreaterThanOrEqual(timestamp);
|
||||
});
|
||||
|
||||
it('should generate strictly increasing IDs even if baseTimestamp goes backwards', () => {
|
||||
const { result } = renderHook(() => useHistory());
|
||||
const timestamp = 1000000;
|
||||
const itemData: Omit<HistoryItem, 'id'> = { type: 'info', text: 'First' };
|
||||
|
||||
let id1!: number;
|
||||
let id2!: number;
|
||||
|
||||
act(() => {
|
||||
id1 = result.current.addItem(itemData, timestamp);
|
||||
// Try to add with a smaller timestamp
|
||||
id2 = result.current.addItem(itemData, timestamp - 500);
|
||||
});
|
||||
|
||||
expect(id1).toBe(timestamp);
|
||||
expect(id2).toBe(id1 + 1);
|
||||
expect(result.current.history[1].id).toBe(id2);
|
||||
});
|
||||
|
||||
it('should ensure new IDs start after existing IDs when resuming a session', () => {
|
||||
const initialItems: HistoryItem[] = [
|
||||
{ id: 5000, type: 'info', text: 'Existing' },
|
||||
];
|
||||
const { result } = renderHook(() => useHistory({ initialItems }));
|
||||
|
||||
let newId!: number;
|
||||
act(() => {
|
||||
// Try to add with a timestamp smaller than the highest existing ID
|
||||
newId = result.current.addItem({ type: 'info', text: 'New' }, 2000);
|
||||
});
|
||||
|
||||
expect(newId).toBe(5001);
|
||||
expect(result.current.history[1].id).toBe(5001);
|
||||
});
|
||||
|
||||
it('should update lastIdRef when loading new history', () => {
|
||||
const { result } = renderHook(() => useHistory());
|
||||
|
||||
act(() => {
|
||||
result.current.loadHistory([{ id: 8000, type: 'info', text: 'Loaded' }]);
|
||||
});
|
||||
|
||||
let newId!: number;
|
||||
act(() => {
|
||||
newId = result.current.addItem({ type: 'info', text: 'New' }, 1000);
|
||||
});
|
||||
|
||||
expect(newId).toBe(8001);
|
||||
});
|
||||
|
||||
it('should generate unique IDs for items added with the same base timestamp', () => {
|
||||
const { result } = renderHook(() => useHistory());
|
||||
const timestamp = Date.now();
|
||||
@@ -215,8 +265,8 @@ describe('useHistoryManager', () => {
|
||||
const after = Date.now();
|
||||
|
||||
expect(result.current.history).toHaveLength(1);
|
||||
// ID should be >= before + 1 (since counter starts at 0 and increments to 1)
|
||||
expect(result.current.history[0].id).toBeGreaterThanOrEqual(before + 1);
|
||||
// ID should be >= before (since baseTimestamp defaults to Date.now())
|
||||
expect(result.current.history[0].id).toBeGreaterThanOrEqual(before);
|
||||
expect(result.current.history[0].id).toBeLessThanOrEqual(after + 1);
|
||||
});
|
||||
|
||||
|
||||
@@ -42,16 +42,22 @@ export function useHistory({
|
||||
initialItems?: HistoryItem[];
|
||||
} = {}): UseHistoryManagerReturn {
|
||||
const [history, setHistory] = useState<HistoryItem[]>(initialItems);
|
||||
const messageIdCounterRef = useRef(0);
|
||||
const lastIdRef = useRef(
|
||||
initialItems.reduce((max, item) => Math.max(max, item.id), 0),
|
||||
);
|
||||
|
||||
// Generates a unique message ID based on a timestamp and a counter.
|
||||
// Generates a unique message ID based on a timestamp, ensuring it is always
|
||||
// greater than any previously assigned ID.
|
||||
const getNextMessageId = useCallback((baseTimestamp: number): number => {
|
||||
messageIdCounterRef.current += 1;
|
||||
return baseTimestamp + messageIdCounterRef.current;
|
||||
const nextId = Math.max(baseTimestamp, lastIdRef.current + 1);
|
||||
lastIdRef.current = nextId;
|
||||
return nextId;
|
||||
}, []);
|
||||
|
||||
const loadHistory = useCallback((newHistory: HistoryItem[]) => {
|
||||
setHistory(newHistory);
|
||||
const maxId = newHistory.reduce((max, item) => Math.max(max, item.id), 0);
|
||||
lastIdRef.current = Math.max(lastIdRef.current, maxId);
|
||||
}, []);
|
||||
|
||||
// Adds a new item to the history state with a unique ID.
|
||||
@@ -153,7 +159,7 @@ export function useHistory({
|
||||
// Clears the entire history state and resets the ID counter.
|
||||
const clearItems = useCallback(() => {
|
||||
setHistory([]);
|
||||
messageIdCounterRef.current = 0;
|
||||
lastIdRef.current = 0;
|
||||
}, []);
|
||||
|
||||
return useMemo(
|
||||
|
||||
Reference in New Issue
Block a user