Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/types/src/global-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export const globalSettingsSchema = z.object({

mcpEnabled: z.boolean().optional(),
enableMcpServerCreation: z.boolean().optional(),
mcpServers: z.record(z.string(), z.any()).optional(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mcpServers property uses z.any() which bypasses type safety. Could we define a proper Zod schema for MCP server configurations? This would provide better validation and type safety. Something like:

Suggested change
mcpServers: z.record(z.string(), z.any()).optional(),
mcpServers: z.record(z.string(), z.union([
z.object({
type: z.literal('stdio'),
command: z.string(),
args: z.array(z.string()).optional(),
env: z.record(z.string()).optional(),
disabled: z.boolean().optional(),
alwaysAllow: z.array(z.string()).optional()
}),
z.object({
type: z.literal('sse'),
url: z.string().url(),
headers: z.record(z.string()).optional(),
disabled: z.boolean().optional()
})
])).optional(),


remoteControlEnabled: z.boolean().optional(),

Expand Down
206 changes: 206 additions & 0 deletions src/extension/__tests__/api.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"
import * as vscode from "vscode"
import { API } from "../api"
import { ClineProvider } from "../../core/webview/ClineProvider"
import { Package } from "../../shared/package"

// Mock vscode module
vi.mock("vscode", () => ({
commands: {
executeCommand: vi.fn(),
},
workspace: {
getConfiguration: vi.fn(() => ({
update: vi.fn(),
})),
},
window: {
showErrorMessage: vi.fn(),
showInformationMessage: vi.fn(),
showWarningMessage: vi.fn(),
createTextEditorDecorationType: vi.fn().mockReturnValue({
dispose: vi.fn(),
}),
},
ConfigurationTarget: {
Global: 1,
},
}))

describe("API", () => {
let api: API
let mockOutputChannel: any
let mockProvider: any
let mockMcpHub: any

beforeEach(() => {
// Create mock output channel
mockOutputChannel = {
appendLine: vi.fn(),
}

// Create mock MCP hub
mockMcpHub = {
initializeRuntimeMcpServers: vi.fn(),
}

// Create mock provider
mockProvider = {
context: {
extension: {
packageJSON: {
version: "1.0.0",
},
},
},
setValues: vi.fn(),
removeClineFromStack: vi.fn(),
postStateToWebview: vi.fn(),
postMessageToWebview: vi.fn(),
createTask: vi.fn().mockResolvedValue({ taskId: "test-task-id" }),
getMcpHub: vi.fn().mockReturnValue(mockMcpHub),
on: vi.fn(),
off: vi.fn(),
}

// Create API instance
api = new API(mockOutputChannel, mockProvider as any)
})

afterEach(() => {
vi.clearAllMocks()
})

describe("startNewTask", () => {
it("should initialize runtime MCP servers when mcpServers is provided in configuration", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test coverage for the happy path! Consider adding tests for error scenarios:

  • Invalid server configurations
  • Initialization failures
  • What happens when initializeRuntimeMcpServers throws an error

This would ensure the error handling is robust.

const configuration = {
apiProvider: "openai" as const,
mcpServers: {
"test-server": {
type: "stdio",
command: "node",
args: ["test.js"],
},
},
}

const taskId = await api.startNewTask({
configuration,
text: "Test task",
})

// Verify MCP hub was retrieved
expect(mockProvider.getMcpHub).toHaveBeenCalled()

// Verify runtime MCP servers were initialized
expect(mockMcpHub.initializeRuntimeMcpServers).toHaveBeenCalledWith(configuration.mcpServers)

// Verify other methods were called
expect(mockProvider.setValues).toHaveBeenCalledWith(configuration)
expect(mockProvider.createTask).toHaveBeenCalled()
expect(taskId).toBe("test-task-id")
})

it("should not initialize MCP servers when mcpServers is not provided", async () => {
const configuration = {
apiProvider: "openai" as const,
}

await api.startNewTask({
configuration,
text: "Test task",
})

// Verify MCP hub was not retrieved
expect(mockProvider.getMcpHub).not.toHaveBeenCalled()

// Verify runtime MCP servers were not initialized
expect(mockMcpHub.initializeRuntimeMcpServers).not.toHaveBeenCalled()

// Verify other methods were still called
expect(mockProvider.setValues).toHaveBeenCalledWith(configuration)
expect(mockProvider.createTask).toHaveBeenCalled()
})

it("should handle when MCP hub is not available", async () => {
// Make getMcpHub return undefined
mockProvider.getMcpHub.mockReturnValue(undefined)

const configuration = {
apiProvider: "openai" as const,
mcpServers: {
"test-server": {
type: "stdio",
command: "node",
args: ["test.js"],
},
},
}

// Should not throw an error
const taskId = await api.startNewTask({
configuration,
text: "Test task",
})

// Verify MCP hub was retrieved
expect(mockProvider.getMcpHub).toHaveBeenCalled()

// Verify runtime MCP servers were not initialized (since hub is undefined)
expect(mockMcpHub.initializeRuntimeMcpServers).not.toHaveBeenCalled()

// Verify other methods were still called
expect(mockProvider.setValues).toHaveBeenCalledWith(configuration)
expect(mockProvider.createTask).toHaveBeenCalled()
expect(taskId).toBe("test-task-id")
})

it("should handle complex MCP server configurations", async () => {
const configuration = {
apiProvider: "openai" as const,
mcpServers: {
"stdio-server": {
type: "stdio",
command: "node",
args: ["server.js"],
env: { NODE_ENV: "production" },
disabled: false,
alwaysAllow: ["tool1", "tool2"],
},
"sse-server": {
type: "sse",
url: "http://localhost:8080/sse",
headers: { Authorization: "Bearer token" },
disabled: true,
},
},
}

await api.startNewTask({
configuration,
text: "Test task",
})

// Verify runtime MCP servers were initialized with the full configuration
expect(mockMcpHub.initializeRuntimeMcpServers).toHaveBeenCalledWith(configuration.mcpServers)
})

it("should handle empty mcpServers object", async () => {
const configuration = {
apiProvider: "openai" as const,
mcpServers: {},
}

await api.startNewTask({
configuration,
text: "Test task",
})

// Verify MCP hub was retrieved
expect(mockProvider.getMcpHub).toHaveBeenCalled()

// Verify runtime MCP servers were initialized with empty object
expect(mockMcpHub.initializeRuntimeMcpServers).toHaveBeenCalledWith({})
})
})
})
9 changes: 9 additions & 0 deletions src/extension/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI {
}

if (configuration) {
// Handle MCP servers if provided in configuration
if (configuration.mcpServers && typeof configuration.mcpServers === "object") {
const mcpHub = provider.getMcpHub()
if (mcpHub) {
// Initialize runtime MCP servers
await mcpHub.initializeRuntimeMcpServers(configuration.mcpServers)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When initializeRuntimeMcpServers fails, should we log this error or notify the user? Currently, if the mcpHub exists but initialization fails, the error is silently swallowed in McpHub. Consider adding error handling here:

Suggested change
await mcpHub.initializeRuntimeMcpServers(configuration.mcpServers)
if (configuration.mcpServers && typeof configuration.mcpServers === "object") {
const mcpHub = provider.getMcpHub()
if (mcpHub) {
try {
// Initialize runtime MCP servers
await mcpHub.initializeRuntimeMcpServers(configuration.mcpServers)
} catch (error) {
console.error('Failed to initialize runtime MCP servers:', error)
// Optionally notify user or continue without MCP servers
}
}
}

}
}

await provider.setValues(configuration)

if (configuration.allowedCommands) {
Expand Down
Loading
Loading