jcarr
/
gemini-cli
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

addressed b/423798481 (#887)

This commit is contained in:
Bryan Morgan 2025-06-10 08:47:46 -04:00 committed by GitHub
parent 749c010a0d
commit 1e3abf96b5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 222 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

129
packages/cli/src/ui/hooks/slashCommandProcessor.test.ts
View File

@ -60,6 +60,8 @@ import {
type Config,
MCPServerStatus,
getMCPServerStatus,
MCPDiscoveryState,
getMCPDiscoveryState,
} from '@gemini-cli/core';
import { useSessionStats } from '../contexts/SessionContext.js';
@ -525,7 +527,7 @@ Add any other context about the problem here.
describe('/mcp command', () => {
beforeEach(() => {
// Mock the core module with getMCPServerStatus
// Mock the core module with getMCPServerStatus and getMCPDiscoveryState
vi.mock('@gemini-cli/core', async (importOriginal) => {
const actual = await importOriginal();
return {
@ -535,7 +537,13 @@ Add any other context about the problem here.
CONNECTING: 'connecting',
DISCONNECTED: 'disconnected',
},
MCPDiscoveryState: {
NOT_STARTED: 'not_started',
IN_PROGRESS: 'in_progress',
COMPLETED: 'completed',
},
getMCPServerStatus: vi.fn(),
getMCPDiscoveryState: vi.fn(),
};
});
});
@ -596,13 +604,18 @@ Add any other context about the problem here.
server3: { command: 'cmd3' },
};
// Setup getMCPServerStatus mock implementation
// Setup getMCPServerStatus mock implementation - use all CONNECTED to avoid startup message in this test
vi.mocked(getMCPServerStatus).mockImplementation((serverName) => {
if (serverName === 'server1') return MCPServerStatus.CONNECTED;
if (serverName === 'server2') return MCPServerStatus.CONNECTING;
if (serverName === 'server2') return MCPServerStatus.CONNECTED;
return MCPServerStatus.DISCONNECTED; // Default for server3 and others
});
// Setup getMCPDiscoveryState mock to return completed so no startup message is shown
vi.mocked(getMCPDiscoveryState).mockReturnValue(
MCPDiscoveryState.COMPLETED,
);
// Mock tools from each server
const mockServer1Tools = [
{ name: 'server1_tool1' },
@ -638,24 +651,30 @@ Add any other context about the problem here.
2,
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Configured MCP servers and tools:'),
text: expect.stringContaining('Configured MCP servers:'),
}),
expect.any(Number),
);
// Check that the message contains details about both servers and their tools
// Check that the message contains details about servers and their tools
const message = mockAddItem.mock.calls[1][0].text;
// Server 1 - Connected (green dot)
expect(message).toContain('🟢 \u001b[1mserver1\u001b[0m (2 tools)');
// Server 1 - Connected
expect(message).toContain(
'🟢 \u001b[1mserver1\u001b[0m - Ready (2 tools)',
);
expect(message).toContain('\u001b[36mserver1_tool1\u001b[0m');
expect(message).toContain('\u001b[36mserver1_tool2\u001b[0m');
// Server 2 - Connecting (yellow dot)
expect(message).toContain('🟡 \u001b[1mserver2\u001b[0m (1 tools)');
// Server 2 - Connected
expect(message).toContain(
'🟢 \u001b[1mserver2\u001b[0m - Ready (1 tools)',
);
expect(message).toContain('\u001b[36mserver2_tool1\u001b[0m');
// Server 3 - No status, should default to Disconnected (red dot)
expect(message).toContain('🔴 \u001b[1mserver3\u001b[0m (1 tools)');
// Server 3 - Disconnected
expect(message).toContain(
'🔴 \u001b[1mserver3\u001b[0m - Disconnected (1 tools cached)',
);
expect(message).toContain('\u001b[36mserver3_tool1\u001b[0m');
expect(commandResult).toBe(true);
@ -676,6 +695,11 @@ Add any other context about the problem here.
return MCPServerStatus.DISCONNECTED;
});
// Setup getMCPDiscoveryState mock to return completed
vi.mocked(getMCPDiscoveryState).mockReturnValue(
MCPDiscoveryState.COMPLETED,
);
// Mock tools from server with descriptions
const mockServerTools = [
{ name: 'tool1', description: 'This is tool 1 description' },
@ -700,7 +724,7 @@ Add any other context about the problem here.
2,
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Configured MCP servers and tools:'),
text: expect.stringContaining('Configured MCP servers:'),
}),
expect.any(Number),
);
@ -708,7 +732,7 @@ Add any other context about the problem here.
const message = mockAddItem.mock.calls[1][0].text;
// Check that server description is included (with ANSI color codes)
expect(message).toContain('\u001b[1mserver1\u001b[0m (2 tools)');
expect(message).toContain('\u001b[1mserver1\u001b[0m - Ready (2 tools)');
expect(message).toContain(
'\u001b[32mThis is a server description\u001b[0m',
);
@ -740,6 +764,11 @@ Add any other context about the problem here.
return MCPServerStatus.DISCONNECTED;
});
// Setup getMCPDiscoveryState mock to return completed
vi.mocked(getMCPDiscoveryState).mockReturnValue(
MCPDiscoveryState.COMPLETED,
);
// Mock tools from each server - server2 has no tools
const mockServer1Tools = [{ name: 'server1_tool1' }];
@ -769,19 +798,87 @@ Add any other context about the problem here.
2,
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Configured MCP servers and tools:'),
text: expect.stringContaining('Configured MCP servers:'),
}),
expect.any(Number),
);
// Check that the message contains details about both servers and their tools
const message = mockAddItem.mock.calls[1][0].text;
expect(message).toContain('🟢 \u001b[1mserver1\u001b[0m (1 tools)');
expect(message).toContain(
'🟢 \u001b[1mserver1\u001b[0m - Ready (1 tools)',
);
expect(message).toContain('\u001b[36mserver1_tool1\u001b[0m');
expect(message).toContain('🔴 \u001b[1mserver2\u001b[0m (0 tools)');
expect(message).toContain(
'🔴 \u001b[1mserver2\u001b[0m - Disconnected (0 tools cached)',
);
expect(message).toContain('No tools available');
expect(commandResult).toBe(true);
});
it('should show startup indicator when servers are connecting', async () => {
// Mock MCP servers configuration
const mockMcpServers = {
server1: { command: 'cmd1' },
server2: { command: 'cmd2' },
};
// Setup getMCPServerStatus mock implementation with one server connecting
vi.mocked(getMCPServerStatus).mockImplementation((serverName) => {
if (serverName === 'server1') return MCPServerStatus.CONNECTED;
if (serverName === 'server2') return MCPServerStatus.CONNECTING;
return MCPServerStatus.DISCONNECTED;
});
// Setup getMCPDiscoveryState mock to return in progress
vi.mocked(getMCPDiscoveryState).mockReturnValue(
MCPDiscoveryState.IN_PROGRESS,
);
// Mock tools from each server
const mockServer1Tools = [{ name: 'server1_tool1' }];
const mockServer2Tools = [{ name: 'server2_tool1' }];
const mockGetToolsByServer = vi.fn().mockImplementation((serverName) => {
if (serverName === 'server1') return mockServer1Tools;
if (serverName === 'server2') return mockServer2Tools;
return [];
});
mockConfig = {
...mockConfig,
getToolRegistry: vi.fn().mockResolvedValue({
getToolsByServer: mockGetToolsByServer,
}),
getMcpServers: vi.fn().mockReturnValue(mockMcpServers),
} as unknown as Config;
const { handleSlashCommand } = getProcessor();
let commandResult: SlashCommandActionReturn | boolean = false;
await act(async () => {
commandResult = handleSlashCommand('/mcp');
});
const message = mockAddItem.mock.calls[1][0].text;
// Check that startup indicator is shown
expect(message).toContain(
'⏳ MCP servers are starting up (1 initializing)...',
);
expect(message).toContain(
'Note: First startup may take longer. Tool availability will update automatically.',
);
// Check server statuses
expect(message).toContain(
'🟢 \u001b[1mserver1\u001b[0m - Ready (1 tools)',
);
expect(message).toContain(
'🔄 \u001b[1mserver2\u001b[0m - Starting... (first startup may take longer) (tools will appear when ready)',
);
expect(commandResult).toBe(true);
});
});
});

54
packages/cli/src/ui/hooks/slashCommandProcessor.ts
View File

@ -9,7 +9,13 @@ import { type PartListUnion } from '@google/genai';
import open from 'open';
import process from 'node:process';
import { UseHistoryManagerReturn } from './useHistoryManager.js';
import { Config, MCPServerStatus, getMCPServerStatus } from '@gemini-cli/core';
import {
Config,
MCPServerStatus,
getMCPServerStatus,
getMCPDiscoveryState,
MCPDiscoveryState,
} from '@gemini-cli/core';
import { Message, MessageType, HistoryItemWithoutId } from '../types.js';
import { useSessionStats } from '../contexts/SessionContext.js';
import { createShowMemoryAction } from './useShowMemoryCommand.js';
@ -226,32 +232,62 @@ export const useSlashCommandProcessor = (
return;
}
let message = 'Configured MCP servers and tools:\n\n';
// Check if any servers are still connecting
const connectingServers = serverNames.filter(
(name) => getMCPServerStatus(name) === MCPServerStatus.CONNECTING,
);
const discoveryState = getMCPDiscoveryState();
let message = '';
// Add overall discovery status message if needed
if (
discoveryState === MCPDiscoveryState.IN_PROGRESS ||
connectingServers.length > 0
) {
message += `\u001b[33m⏳ MCP servers are starting up (${connectingServers.length} initializing)...\u001b[0m\n`;
message += `\u001b[90mNote: First startup may take longer. Tool availability will update automatically.\u001b[0m\n\n`;
}
message += 'Configured MCP servers:\n\n';
for (const serverName of serverNames) {
const serverTools = toolRegistry.getToolsByServer(serverName);
const status = getMCPServerStatus(serverName);
// Add status indicator
let statusDot = '';
// Add status indicator with descriptive text
let statusIndicator = '';
let statusText = '';
switch (status) {
case MCPServerStatus.CONNECTED:
statusDot = '🟢'; // Green dot for connected
statusIndicator = '🟢';
statusText = 'Ready';
break;
case MCPServerStatus.CONNECTING:
statusDot = '🟡'; // Yellow dot for connecting
statusIndicator = '🔄';
statusText = 'Starting... (first startup may take longer)';
break;
case MCPServerStatus.DISCONNECTED:
default:
statusDot = '🔴'; // Red dot for disconnected
statusIndicator = '🔴';
statusText = 'Disconnected';
break;
}
// Get server description if available
const server = mcpServers[serverName];
// Format server header with bold formatting
message += `${statusDot} \u001b[1m${serverName}\u001b[0m (${serverTools.length} tools)`;
// Format server header with bold formatting and status
message += `${statusIndicator} \u001b[1m${serverName}\u001b[0m - ${statusText}`;
// Add tool count with conditional messaging
if (status === MCPServerStatus.CONNECTED) {
message += ` (${serverTools.length} tools)`;
} else if (status === MCPServerStatus.CONNECTING) {
message += ` (tools will appear when ready)`;
} else {
message += ` (${serverTools.length} tools cached)`;
}
// Add server description with proper handling of multi-line descriptions
if (useShowDescriptions && server?.description) {

82
packages/core/src/tools/mcp-client.ts
View File

@ -27,11 +27,28 @@ export enum MCPServerStatus {
CONNECTED = 'connected',
}
/**
* Enum representing the overall MCP discovery state
*/
export enum MCPDiscoveryState {
/** Discovery has not started yet */
NOT_STARTED = 'not_started',
/** Discovery is currently in progress */
IN_PROGRESS = 'in_progress',
/** Discovery has completed (with or without errors) */
COMPLETED = 'completed',
}
/**
* Map to track the status of each MCP server within the core package
*/
const mcpServerStatusesInternal: Map<string, MCPServerStatus> = new Map();
/**
* Track the overall MCP discovery state
*/
let mcpDiscoveryState: MCPDiscoveryState = MCPDiscoveryState.NOT_STARTED;
/**
* Event listeners for MCP server status changes
*/
@ -92,29 +109,48 @@ export function getAllMCPServerStatuses(): Map<string, MCPServerStatus> {
return new Map(mcpServerStatusesInternal);
}
/**
* Get the current MCP discovery state
*/
export function getMCPDiscoveryState(): MCPDiscoveryState {
return mcpDiscoveryState;
}
export async function discoverMcpTools(
mcpServers: Record<string, MCPServerConfig>,
mcpServerCommand: string | undefined,
toolRegistry: ToolRegistry,
): Promise<void> {
if (mcpServerCommand) {
const cmd = mcpServerCommand;
const args = parse(cmd, process.env) as string[];
if (args.some((arg) => typeof arg !== 'string')) {
throw new Error('failed to parse mcpServerCommand: ' + cmd);
}
// use generic server name 'mcp'
mcpServers['mcp'] = {
command: args[0],
args: args.slice(1),
};
}
// Set discovery state to in progress
mcpDiscoveryState = MCPDiscoveryState.IN_PROGRESS;
const discoveryPromises = Object.entries(mcpServers).map(
([mcpServerName, mcpServerConfig]) =>
connectAndDiscover(mcpServerName, mcpServerConfig, toolRegistry),
);
await Promise.all(discoveryPromises);
try {
if (mcpServerCommand) {
const cmd = mcpServerCommand;
const args = parse(cmd, process.env) as string[];
if (args.some((arg) => typeof arg !== 'string')) {
throw new Error('failed to parse mcpServerCommand: ' + cmd);
}
// use generic server name 'mcp'
mcpServers['mcp'] = {
command: args[0],
args: args.slice(1),
};
}
const discoveryPromises = Object.entries(mcpServers).map(
([mcpServerName, mcpServerConfig]) =>
connectAndDiscover(mcpServerName, mcpServerConfig, toolRegistry),
);
await Promise.all(discoveryPromises);
// Mark discovery as completed
mcpDiscoveryState = MCPDiscoveryState.COMPLETED;
} catch (error) {
// Still mark as completed even with errors
mcpDiscoveryState = MCPDiscoveryState.COMPLETED;
throw error;
}
}
async function connectAndDiscover(
@ -172,9 +208,19 @@ async function connectAndDiscover(
// Connection successful
updateMCPServerStatus(mcpServerName, MCPServerStatus.CONNECTED);
} catch (error) {
// Create a safe config object that excludes sensitive information
const safeConfig = {
command: mcpServerConfig.command,
url: mcpServerConfig.url,
cwd: mcpServerConfig.cwd,
timeout: mcpServerConfig.timeout,
trust: mcpServerConfig.trust,
// Exclude args and env which may contain sensitive data
};
console.error(
`failed to start or connect to MCP server '${mcpServerName}' ` +
`${JSON.stringify(mcpServerConfig)}; \n${error}`,
`${JSON.stringify(safeConfig)}; \n${error}`,
);
// Update status to disconnected
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);