Added /mcp command support and cleaned up broken tests (#817)

This commit is contained in:
Bryan Morgan 2025-06-07 15:06:18 -04:00 committed by GitHub
parent 6ea4479064
commit 28ff62e7b1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 449 additions and 33 deletions

View File

@ -56,7 +56,11 @@ import {
type SlashCommandActionReturn,
} from './slashCommandProcessor.js';
import { MessageType } from '../types.js';
import { type Config } from '@gemini-code/core';
import {
type Config,
MCPServerStatus,
getMCPServerStatus,
} from '@gemini-code/core';
import * as ShowMemoryCommandModule from './useShowMemoryCommand.js';
import { GIT_COMMIT_INFO } from '../../generated/git-commit.js';
@ -378,14 +382,54 @@ Add any other context about the problem here.
expect(commandResult).toBe(true);
});
it('should display available tools when tools are found', async () => {
const mockTools = [{ name: 'tool1' }, { name: 'tool2' }];
it('should display only Gemini CLI tools (filtering out MCP tools)', async () => {
// Create mock tools - some with serverName property (MCP tools) and some without (Gemini CLI tools)
const mockTools = [
{ name: 'tool1' },
{ name: 'tool2' },
{ name: 'mcp_tool1', serverName: 'mcp-server1' },
{ name: 'mcp_tool2', serverName: 'mcp-server1' },
];
mockConfig = {
...mockConfig,
getToolRegistry: vi.fn().mockResolvedValue({
getAllTools: vi.fn().mockReturnValue(mockTools),
}),
} as unknown as Config;
const { handleSlashCommand } = getProcessor();
let commandResult: SlashCommandActionReturn | boolean = false;
await act(async () => {
commandResult = handleSlashCommand('/tools');
});
// Should only show tool1 and tool2, not the MCP tools
expect(mockAddItem).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
type: MessageType.INFO,
text: 'Available Gemini CLI tools:\n\ntool1\ntool2',
}),
expect.any(Number),
);
expect(commandResult).toBe(true);
});
it('should display a message when no Gemini CLI tools are available', async () => {
// Only MCP tools available
const mockTools = [
{ name: 'mcp_tool1', serverName: 'mcp-server1' },
{ name: 'mcp_tool2', serverName: 'mcp-server1' },
];
mockConfig = {
...mockConfig,
getToolRegistry: vi.fn().mockResolvedValue({
getAllTools: vi.fn().mockReturnValue(mockTools),
}),
} as unknown as Config;
const { handleSlashCommand } = getProcessor();
let commandResult: SlashCommandActionReturn | boolean = false;
await act(async () => {
@ -396,36 +440,208 @@ Add any other context about the problem here.
2,
expect.objectContaining({
type: MessageType.INFO,
text: 'Available tools:\n\ntool1\ntool2',
text: 'Available Gemini CLI tools:\n\n',
}),
expect.any(Number),
);
expect(commandResult).toBe(true);
});
});
describe('/mcp command', () => {
beforeEach(() => {
// Mock the core module with getMCPServerStatus
vi.mock('@gemini-code/core', async (importOriginal) => {
const actual = await importOriginal();
return {
...actual,
MCPServerStatus: {
CONNECTED: 'connected',
CONNECTING: 'connecting',
DISCONNECTED: 'disconnected',
},
getMCPServerStatus: vi.fn(),
};
});
});
it('should show an error if tool registry is not available', async () => {
mockConfig = {
...mockConfig,
getToolRegistry: vi.fn().mockResolvedValue(undefined),
} as unknown as Config;
const { handleSlashCommand } = getProcessor();
let commandResult: SlashCommandActionReturn | boolean = false;
await act(async () => {
commandResult = handleSlashCommand('/mcp');
});
expect(mockAddItem).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
type: MessageType.ERROR,
text: 'Could not retrieve tool registry.',
}),
expect.any(Number),
);
expect(commandResult).toBe(true);
});
it('should display a message when no tools are available', async () => {
const mockTools: unknown[] = [];
it('should display a message when no MCP servers are configured', async () => {
mockConfig = {
...mockConfig,
getToolRegistry: vi.fn().mockResolvedValue({
getAllTools: vi.fn().mockReturnValue(mockTools),
getToolsByServer: vi.fn().mockReturnValue([]),
}),
getMcpServers: vi.fn().mockReturnValue({}),
} as unknown as Config;
const { handleSlashCommand } = getProcessor();
let commandResult: SlashCommandActionReturn | boolean = false;
await act(async () => {
commandResult = handleSlashCommand('/tools');
commandResult = handleSlashCommand('/mcp');
});
expect(mockAddItem).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
type: MessageType.INFO,
text: 'Available tools:\n\n',
text: 'No MCP servers configured.',
}),
expect.any(Number),
);
expect(commandResult).toBe(true);
});
it('should display configured MCP servers with status indicators and their tools', async () => {
// Mock MCP servers configuration
const mockMcpServers = {
server1: { command: 'cmd1' },
server2: { command: 'cmd2' },
server3: { command: 'cmd3' },
};
// Setup getMCPServerStatus mock implementation
vi.mocked(getMCPServerStatus).mockImplementation((serverName) => {
if (serverName === 'server1') return MCPServerStatus.CONNECTED;
if (serverName === 'server2') return MCPServerStatus.CONNECTING;
return MCPServerStatus.DISCONNECTED; // Default for server3 and others
});
// Mock tools from each server
const mockServer1Tools = [
{ name: 'server1_tool1' },
{ name: 'server1_tool2' },
];
const mockServer2Tools = [{ name: 'server2_tool1' }];
const mockServer3Tools = [{ name: 'server3_tool1' }];
const mockGetToolsByServer = vi.fn().mockImplementation((serverName) => {
if (serverName === 'server1') return mockServer1Tools;
if (serverName === 'server2') return mockServer2Tools;
if (serverName === 'server3') return mockServer3Tools;
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');
});
expect(mockAddItem).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Configured MCP servers and tools:'),
}),
expect.any(Number),
);
// Check that the message contains details about both servers and their tools
const message = mockAddItem.mock.calls[1][0].text;
// Server 1 - Connected (green dot)
expect(message).toContain('🟢 server1 (2 tools):');
expect(message).toContain('server1_tool1');
expect(message).toContain('server1_tool2');
// Server 2 - Connecting (yellow dot)
expect(message).toContain('🟡 server2 (1 tools):');
expect(message).toContain('server2_tool1');
// Server 3 - No status, should default to Disconnected (red dot)
expect(message).toContain('🔴 server3 (1 tools):');
expect(message).toContain('server3_tool1');
expect(commandResult).toBe(true);
});
it('should indicate when a server has no tools', async () => {
// Mock MCP servers configuration
const mockMcpServers = {
server1: { command: 'cmd1' },
server2: { command: 'cmd2' },
};
// Setup getMCPServerStatus mock implementation
vi.mocked(getMCPServerStatus).mockImplementation((serverName) => {
if (serverName === 'server1') return MCPServerStatus.CONNECTED;
if (serverName === 'server2') return MCPServerStatus.DISCONNECTED;
return MCPServerStatus.DISCONNECTED;
});
// Mock tools from each server - server2 has no tools
const mockServer1Tools = [{ name: 'server1_tool1' }];
const mockServer2Tools = [];
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');
});
expect(mockAddItem).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
type: MessageType.INFO,
text: expect.stringContaining('Configured MCP servers and tools:'),
}),
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('🟢 server1 (1 tools):');
expect(message).toContain('server1_tool1');
expect(message).toContain('🔴 server2 (0 tools):');
expect(message).toContain('No tools available');
expect(commandResult).toBe(true);
});
});
});

View File

@ -9,7 +9,7 @@ import { type PartListUnion } from '@google/genai';
import open from 'open';
import process from 'node:process';
import { UseHistoryManagerReturn } from './useHistoryManager.js';
import { Config } from '@gemini-code/core';
import { Config, MCPServerStatus, getMCPServerStatus } from '@gemini-code/core';
import { Message, MessageType, HistoryItemWithoutId } from '../types.js';
import { createShowMemoryAction } from './useShowMemoryCommand.js';
import { GIT_COMMIT_INFO } from '../../generated/git-commit.js';
@ -137,10 +137,75 @@ export const useSlashCommandProcessor = (
openThemeDialog();
},
},
{
name: 'mcp',
description: 'list configured MCP servers and tools',
action: async (_mainCommand, _subCommand, _args) => {
const toolRegistry = await config?.getToolRegistry();
if (!toolRegistry) {
addMessage({
type: MessageType.ERROR,
content: 'Could not retrieve tool registry.',
timestamp: new Date(),
});
return;
}
const mcpServers = config?.getMcpServers() || {};
const serverNames = Object.keys(mcpServers);
if (serverNames.length === 0) {
addMessage({
type: MessageType.INFO,
content: 'No MCP servers configured.',
timestamp: new Date(),
});
return;
}
let message = 'Configured MCP servers and tools:\n\n';
for (const serverName of serverNames) {
const serverTools = toolRegistry.getToolsByServer(serverName);
const status = getMCPServerStatus(serverName);
// Add status indicator
let statusDot = '';
switch (status) {
case MCPServerStatus.CONNECTED:
statusDot = '🟢'; // Green dot for connected
break;
case MCPServerStatus.CONNECTING:
statusDot = '🟡'; // Yellow dot for connecting
break;
case MCPServerStatus.DISCONNECTED:
default:
statusDot = '🔴'; // Red dot for disconnected
break;
}
message += `${statusDot} ${serverName} (${serverTools.length} tools):\n`;
if (serverTools.length > 0) {
serverTools.forEach((tool) => {
message += ` - ${tool.name}\n`;
});
} else {
message += ' No tools available\n';
}
message += '\n';
}
addMessage({
type: MessageType.INFO,
content: message,
timestamp: new Date(),
});
},
},
{
name: 'memory',
description:
'Manage memory. Usage: /memory <show|refresh|add> [text for add]',
'manage memory. Usage: /memory <show|refresh|add> [text for add]',
action: (mainCommand, subCommand, args) => {
switch (subCommand) {
case 'show':
@ -163,7 +228,7 @@ export const useSlashCommandProcessor = (
},
{
name: 'tools',
description: 'list available tools',
description: 'list available Gemini CLI tools',
action: async (_mainCommand, _subCommand, _args) => {
const toolRegistry = await config?.getToolRegistry();
const tools = toolRegistry?.getAllTools();
@ -175,10 +240,14 @@ export const useSlashCommandProcessor = (
});
return;
}
const toolList = tools.map((tool) => tool.name);
// Filter out MCP tools by checking if they have a serverName property
const geminiTools = tools.filter((tool) => !('serverName' in tool));
const geminiToolList = geminiTools.map((tool) => tool.name);
addMessage({
type: MessageType.INFO,
content: `Available tools:\n\n${toolList.join('\n')}`,
content: `Available Gemini CLI tools:\n\n${geminiToolList.join('\n')}`,
timestamp: new Date(),
});
},
@ -191,7 +260,7 @@ export const useSlashCommandProcessor = (
},
{
name: 'about',
description: 'Show version info',
description: 'show version info',
action: (_mainCommand, _subCommand, _args) => {
const osVersion = process.platform;
let sandboxEnv = 'no sandbox';
@ -214,7 +283,7 @@ export const useSlashCommandProcessor = (
},
{
name: 'bug',
description: 'Submit a bug report.',
description: 'submit a bug report',
action: (_mainCommand, _subCommand, args) => {
let bugDescription = _subCommand || '';
if (args) {

View File

@ -44,6 +44,8 @@ export * from './tools/memoryTool.js';
export * from './tools/shell.js';
export * from './tools/web-search.js';
export * from './tools/read-many-files.js';
export * from './tools/mcp-client.js';
export * from './tools/mcp-tool.js';
// Export telemetry functions
export * from './telemetry/index.js';

View File

@ -91,7 +91,7 @@ Expectation for required parameters:
2. \`old_string\` MUST be the exact literal text to replace (including all whitespace, indentation, newlines, and surrounding code etc.).
3. \`new_string\` MUST be the exact literal text to replace \`old_string\` with (also including all whitespace, indentation, newlines, and surrounding code etc.). Ensure the resulting code is correct and idiomatic.
4. NEVER escape \`old_string\` or \`new_string\`, that would break the exact literal text requirement.
**Important:** If ANY of the above are not satisfied, the tool will fail. CRITICAL for \`old_string\`: Must uniquely identify the single instance to change. Include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string matches multiple locations, or does not match exactly, the tool will fail.,
**Important:** If ANY of the above are not satisfied, the tool will fail. CRITICAL for \`old_string\`: Must uniquely identify the single instance to change. Include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string matches multiple locations, or does not match exactly, the tool will fail.
**Multiple replacements:** Set \`expected_replacements\` to the number of occurrences you want to replace. The tool will replace ALL occurrences that match \`old_string\` exactly. Ensure the number of replacements matches your expectation.`,
{
properties: {

View File

@ -185,7 +185,9 @@ describe('GlobTool', () => {
});
it('should return error if pattern is missing (schema validation)', () => {
const params = { path: '.' } as unknown as GlobToolParams;
// Need to correctly define this as an object without pattern
const params = { path: '.' };
// @ts-expect-error - We're intentionally creating invalid params for testing
expect(globTool.validateToolParams(params)).toContain(
'Parameters failed schema validation',
);
@ -209,7 +211,8 @@ describe('GlobTool', () => {
const params = {
pattern: '*.ts',
path: 123,
} as unknown as GlobToolParams;
};
// @ts-expect-error - We're intentionally creating invalid params for testing
expect(globTool.validateToolParams(params)).toContain(
'Parameters failed schema validation',
);
@ -219,7 +222,8 @@ describe('GlobTool', () => {
const params = {
pattern: '*.ts',
case_sensitive: 'true',
} as unknown as GlobToolParams;
};
// @ts-expect-error - We're intentionally creating invalid params for testing
expect(globTool.validateToolParams(params)).toContain(
'Parameters failed schema validation',
);

View File

@ -13,6 +13,83 @@ import { DiscoveredMCPTool } from './mcp-tool.js';
import { CallableTool, FunctionDeclaration, mcpToTool } from '@google/genai';
import { ToolRegistry } from './tool-registry.js';
/**
* Enum representing the connection status of an MCP server
*/
export enum MCPServerStatus {
/** Server is disconnected or experiencing errors */
DISCONNECTED = 'disconnected',
/** Server is in the process of connecting */
CONNECTING = 'connecting',
/** Server is connected and ready to use */
CONNECTED = 'connected',
}
/**
* Map to track the status of each MCP server within the core package
*/
const mcpServerStatusesInternal: Map<string, MCPServerStatus> = new Map();
/**
* Event listeners for MCP server status changes
*/
type StatusChangeListener = (
serverName: string,
status: MCPServerStatus,
) => void;
const statusChangeListeners: StatusChangeListener[] = [];
/**
* Add a listener for MCP server status changes
*/
export function addMCPStatusChangeListener(
listener: StatusChangeListener,
): void {
statusChangeListeners.push(listener);
}
/**
* Remove a listener for MCP server status changes
*/
export function removeMCPStatusChangeListener(
listener: StatusChangeListener,
): void {
const index = statusChangeListeners.indexOf(listener);
if (index !== -1) {
statusChangeListeners.splice(index, 1);
}
}
/**
* Update the status of an MCP server
*/
function updateMCPServerStatus(
serverName: string,
status: MCPServerStatus,
): void {
mcpServerStatusesInternal.set(serverName, status);
// Notify all listeners
for (const listener of statusChangeListeners) {
listener(serverName, status);
}
}
/**
* Get the current status of an MCP server
*/
export function getMCPServerStatus(serverName: string): MCPServerStatus {
return (
mcpServerStatusesInternal.get(serverName) || MCPServerStatus.DISCONNECTED
);
}
/**
* Get all MCP server statuses
*/
export function getAllMCPServerStatuses(): Map<string, MCPServerStatus> {
return new Map(mcpServerStatusesInternal);
}
export async function discoverMcpTools(
mcpServers: Record<string, MCPServerConfig>,
mcpServerCommand: string | undefined,
@ -43,6 +120,9 @@ async function connectAndDiscover(
mcpServerConfig: MCPServerConfig,
toolRegistry: ToolRegistry,
): Promise<void> {
// Initialize the server status as connecting
updateMCPServerStatus(mcpServerName, MCPServerStatus.CONNECTING);
let transport;
if (mcpServerConfig.url) {
transport = new SSEClientTransport(new URL(mcpServerConfig.url));
@ -61,6 +141,8 @@ async function connectAndDiscover(
console.error(
`MCP server '${mcpServerName}' has invalid configuration: missing both url (for SSE) and command (for stdio). Skipping.`,
);
// Update status to disconnected
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);
return;
}
@ -72,16 +154,22 @@ async function connectAndDiscover(
try {
await mcpClient.connect(transport);
// Connection successful
updateMCPServerStatus(mcpServerName, MCPServerStatus.CONNECTED);
} catch (error) {
console.error(
`failed to start or connect to MCP server '${mcpServerName}' ` +
`${JSON.stringify(mcpServerConfig)}; \n${error}`,
);
// Update status to disconnected
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);
return;
}
mcpClient.onerror = (error) => {
console.error(`MCP ERROR (${mcpServerName}):`, error.toString());
// Update status to disconnected on error
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);
};
if (transport instanceof StdioClientTransport && transport.stderr) {
@ -110,6 +198,8 @@ async function connectAndDiscover(
} else if (transport instanceof SSEClientTransport) {
await transport.close();
}
// Update status to disconnected
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);
return;
}
@ -168,6 +258,8 @@ async function connectAndDiscover(
) {
await transport.close();
}
// Update status to disconnected
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);
}
// If no tools were registered from this MCP server, the following 'if' block
@ -184,6 +276,8 @@ async function connectAndDiscover(
transport instanceof SSEClientTransport
) {
await transport.close();
// Update status to disconnected
updateMCPServerStatus(mcpServerName, MCPServerStatus.DISCONNECTED);
}
}
}

View File

@ -45,6 +45,10 @@ describe('loadServerHierarchicalMemory', () => {
beforeEach(() => {
vi.resetAllMocks();
// Set environment variables to indicate test environment
process.env.NODE_ENV = 'test';
process.env.VITEST = 'true';
setGeminiMdFilename(DEFAULT_CONTEXT_FILENAME); // Use defined const
mockOs.homedir.mockReturnValue(USER_HOME);

View File

@ -56,19 +56,31 @@ async function findProjectRoot(startDir: string): Promise<string | null> {
return currentDir;
}
} catch (error: unknown) {
// Don't log ENOENT errors as they're expected when .git doesn't exist
// Also don't log errors in test environments, which often have mocked fs
const isENOENT =
typeof error === 'object' &&
error !== null &&
'code' in error &&
(error as { code: string }).code === 'ENOENT';
// Only log unexpected errors in non-test environments
// process.env.NODE_ENV === 'test' or VITEST are common test indicators
const isTestEnv = process.env.NODE_ENV === 'test' || process.env.VITEST;
if (!isENOENT && !isTestEnv) {
if (typeof error === 'object' && error !== null && 'code' in error) {
const fsError = error as { code: string; message: string };
if (fsError.code !== 'ENOENT') {
logger.warn(
`Error checking for .git directory at ${gitPath}: ${fsError.message}`,
);
}
} else {
logger.warn(
`Non-standard error checking for .git directory at ${gitPath}: ${String(error)}`,
);
}
}
}
const parentDir = path.dirname(currentDir);
if (parentDir === currentDir) {
return null;
@ -136,8 +148,12 @@ async function collectDownwardGeminiFiles(
}
}
} catch (error) {
// Only log warnings in non-test environments
const isTestEnv = process.env.NODE_ENV === 'test' || process.env.VITEST;
if (!isTestEnv) {
const message = error instanceof Error ? error.message : String(error);
logger.warn(`Error scanning directory ${directory}: ${message}`);
}
if (debugMode) logger.debug(`Failed to scan directory: ${directory}`);
}
return collectedPaths;
@ -283,10 +299,13 @@ async function readGeminiMdFiles(
`Successfully read: ${filePath} (Length: ${content.length})`,
);
} catch (error: unknown) {
const isTestEnv = process.env.NODE_ENV === 'test' || process.env.VITEST;
if (!isTestEnv) {
const message = error instanceof Error ? error.message : String(error);
logger.warn(
`Warning: Could not read ${getCurrentGeminiMdFilename()} file at ${filePath}. Error: ${message}`,
);
}
results.push({ filePath, content: null }); // Still include it with null content
if (debugMode) logger.debug(`Failed to read: ${filePath}`);
}

View File

@ -35,8 +35,16 @@ shift $((OPTIND - 1))
# note it can be string or boolean, and if missing `npx json` will return empty string
USER_SETTINGS_FILE="$HOME/.gemini/settings.json"
if [ -z "${GEMINI_SANDBOX:-}" ] && [ -f "$USER_SETTINGS_FILE" ]; then
USER_SANDBOX_SETTING=$(sed -e 's/\/\/.*//' -e 's/\/\*.*\*\///g' -e '/^[[:space:]]*\/\//d' "$USER_SETTINGS_FILE" | npx json 'sandbox')
if [ -n "$USER_SANDBOX_SETTING" ]; then
# Check if jq is available (more reliable than npx json)
if command -v jq &>/dev/null; then
USER_SANDBOX_SETTING=$(jq -r '.sandbox // empty' "$USER_SETTINGS_FILE" 2>/dev/null || echo "")
else
# Fallback to npx json with error handling
USER_SANDBOX_SETTING=$(sed -e 's/\/\/.*//' -e 's/\/\*.*\*\///g' -e '/^[[:space:]]*\/\//d' "$USER_SETTINGS_FILE" | npx json 'sandbox' 2>/dev/null || echo "")
fi
# Avoid setting GEMINI_SANDBOX to complex objects
if [ -n "$USER_SANDBOX_SETTING" ] && [[ ! "$USER_SANDBOX_SETTING" =~ ^\{.*\}$ ]]; then
GEMINI_SANDBOX=$USER_SANDBOX_SETTING
fi
fi