diff --git a/packages/types/npm/package.metadata.json b/packages/types/npm/package.metadata.json index 2e662585366..3a95bc19be4 100644 --- a/packages/types/npm/package.metadata.json +++ b/packages/types/npm/package.metadata.json @@ -1,6 +1,6 @@ { "name": "@roo-code/types", - "version": "1.90.0", + "version": "1.91.0", "description": "TypeScript type definitions for Roo Code.", "publishConfig": { "access": "public", diff --git a/packages/types/src/__tests__/cloud.test.ts b/packages/types/src/__tests__/cloud.test.ts index 31a942c2d41..c1fdd02a66e 100644 --- a/packages/types/src/__tests__/cloud.test.ts +++ b/packages/types/src/__tests__/cloud.test.ts @@ -1,10 +1,13 @@ // npx vitest run src/__tests__/cloud.test.ts import { + organizationCloudSettingsSchema, organizationFeaturesSchema, organizationSettingsSchema, + type OrganizationCloudSettings, type OrganizationFeatures, type OrganizationSettings, + type WorkspaceTaskVisibility, } from "../cloud.js" describe("organizationFeaturesSchema", () => { @@ -171,3 +174,84 @@ describe("organizationSettingsSchema with features", () => { expect(result.data).toEqual(input) }) }) + +describe("organizationCloudSettingsSchema with workspaceTaskVisibility", () => { + it("should validate without workspaceTaskVisibility property", () => { + const input = { + recordTaskMessages: true, + enableTaskSharing: true, + } + const result = organizationCloudSettingsSchema.safeParse(input) + expect(result.success).toBe(true) + expect(result.data?.workspaceTaskVisibility).toBeUndefined() + }) + + it("should validate with workspaceTaskVisibility as 'all'", () => { + const input = { + recordTaskMessages: true, + workspaceTaskVisibility: "all" as WorkspaceTaskVisibility, + } + const result = organizationCloudSettingsSchema.safeParse(input) + expect(result.success).toBe(true) + expect(result.data?.workspaceTaskVisibility).toBe("all") + }) + + it("should validate with workspaceTaskVisibility as 'list-only'", () => { + const input = { + workspaceTaskVisibility: "list-only" as WorkspaceTaskVisibility, + } + const result = organizationCloudSettingsSchema.safeParse(input) + expect(result.success).toBe(true) + expect(result.data?.workspaceTaskVisibility).toBe("list-only") + }) + + it("should validate with workspaceTaskVisibility as 'full-lockdown'", () => { + const input = { + workspaceTaskVisibility: "full-lockdown" as WorkspaceTaskVisibility, + } + const result = organizationCloudSettingsSchema.safeParse(input) + expect(result.success).toBe(true) + expect(result.data?.workspaceTaskVisibility).toBe("full-lockdown") + }) + + it("should reject invalid workspaceTaskVisibility value", () => { + const input = { + workspaceTaskVisibility: "invalid-value", + } + const result = organizationCloudSettingsSchema.safeParse(input) + expect(result.success).toBe(false) + }) + + it("should have correct TypeScript type", () => { + // Type-only test to ensure TypeScript compilation + const settings: OrganizationCloudSettings = { + recordTaskMessages: true, + workspaceTaskVisibility: "all", + } + expect(settings.workspaceTaskVisibility).toBe("all") + + const settingsWithoutVisibility: OrganizationCloudSettings = { + recordTaskMessages: false, + } + expect(settingsWithoutVisibility.workspaceTaskVisibility).toBeUndefined() + }) + + it("should validate in organizationSettingsSchema with workspaceTaskVisibility", () => { + const input = { + version: 1, + cloudSettings: { + recordTaskMessages: true, + enableTaskSharing: true, + workspaceTaskVisibility: "list-only" as WorkspaceTaskVisibility, + }, + defaultSettings: {}, + allowList: { + allowAll: true, + providers: {}, + }, + } + const result = organizationSettingsSchema.safeParse(input) + expect(result.success).toBe(true) + expect(result.data?.cloudSettings?.workspaceTaskVisibility).toBe("list-only") + }) +}) diff --git a/packages/types/src/cloud.ts b/packages/types/src/cloud.ts index a90d342c35c..774bac52ef2 100644 --- a/packages/types/src/cloud.ts +++ b/packages/types/src/cloud.ts @@ -120,6 +120,14 @@ export const organizationDefaultSettingsSchema = globalSettingsSchema export type OrganizationDefaultSettings = z.infer +/** + * WorkspaceTaskVisibility + */ + +const workspaceTaskVisibilitySchema = z.enum(["all", "list-only", "full-lockdown"]) + +export type WorkspaceTaskVisibility = z.infer + /** * OrganizationCloudSettings */ @@ -129,6 +137,7 @@ export const organizationCloudSettingsSchema = z.object({ enableTaskSharing: z.boolean().optional(), taskShareExpirationDays: z.number().int().positive().optional(), allowMembersViewAllTasks: z.boolean().optional(), + workspaceTaskVisibility: workspaceTaskVisibilitySchema.optional(), }) export type OrganizationCloudSettings = z.infer diff --git a/src/api/providers/openrouter.ts b/src/api/providers/openrouter.ts index 063f23c210d..37550722e55 100644 --- a/src/api/providers/openrouter.ts +++ b/src/api/providers/openrouter.ts @@ -44,6 +44,13 @@ type OpenRouterChatCompletionParams = OpenAI.Chat.ChatCompletionCreateParams & { reasoning?: OpenRouterReasoningParams } +// OpenRouter error structure that may include metadata.raw with actual upstream error +interface OpenRouterErrorResponse { + message?: string + code?: number + metadata?: { raw?: string } +} + // See `OpenAI.Chat.Completions.ChatCompletionChunk["usage"]` // `CompletionsAPI.CompletionUsage` // See also: https://openrouter.ai/docs/use-cases/usage-accounting @@ -111,6 +118,29 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH return this.currentReasoningDetails.length > 0 ? this.currentReasoningDetails : undefined } + /** + * Handle OpenRouter streaming error response and report to telemetry. + * OpenRouter may include metadata.raw with the actual upstream provider error. + */ + private handleStreamingError(error: OpenRouterErrorResponse, modelId: string, operation: string): never { + const rawErrorMessage = error?.metadata?.raw || error?.message + + const apiError = Object.assign( + new ApiProviderError( + rawErrorMessage ?? "Unknown error", + this.providerName, + modelId, + operation, + error?.code, + ), + { status: error?.code, error: { message: error?.message, metadata: error?.metadata } }, + ) + + TelemetryService.instance.captureException(apiError) + + throw new Error(`OpenRouter API Error ${error?.code}: ${rawErrorMessage}`) + } + override async *createMessage( systemPrompt: string, messages: Anthropic.Messages.MessageParam[], @@ -228,15 +258,9 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH try { stream = await this.client.chat.completions.create(completionParams, requestOptions) } catch (error) { - TelemetryService.instance.captureException( - new ApiProviderError( - error instanceof Error ? error.message : String(error), - this.providerName, - modelId, - "createMessage", - ), - ) - + const errorMessage = error instanceof Error ? error.message : String(error) + const apiError = new ApiProviderError(errorMessage, this.providerName, modelId, "createMessage") + TelemetryService.instance.captureException(apiError) throw handleOpenAIError(error, this.providerName) } @@ -259,23 +283,7 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH for await (const chunk of stream) { // OpenRouter returns an error object instead of the OpenAI SDK throwing an error. if ("error" in chunk) { - const error = chunk.error as { message?: string; code?: number } - console.error(`OpenRouter API Error: ${error?.code} - ${error?.message}`) - - TelemetryService.instance.captureException( - Object.assign( - new ApiProviderError( - error?.message ?? "Unknown error", - this.providerName, - modelId, - "createMessage", - error?.code, - ), - { status: error?.code }, - ), - ) - - throw new Error(`OpenRouter API Error ${error?.code}: ${error?.message}`) + this.handleStreamingError(chunk.error as OpenRouterErrorResponse, modelId, "createMessage") } const delta = chunk.choices[0]?.delta @@ -473,36 +481,14 @@ export class OpenRouterHandler extends BaseProvider implements SingleCompletionH signal: metadata?.signal, }) } catch (error) { - TelemetryService.instance.captureException( - new ApiProviderError( - error instanceof Error ? error.message : String(error), - this.providerName, - modelId, - "completePrompt", - ), - ) - + const errorMessage = error instanceof Error ? error.message : String(error) + const apiError = new ApiProviderError(errorMessage, this.providerName, modelId, "completePrompt") + TelemetryService.instance.captureException(apiError) throw handleOpenAIError(error, this.providerName) } if ("error" in response) { - const error = response.error as { message?: string; code?: number } - console.error(`OpenRouter API Error: ${error?.code} - ${error?.message}`) - - TelemetryService.instance.captureException( - Object.assign( - new ApiProviderError( - error?.message ?? "Unknown error", - this.providerName, - modelId, - "completePrompt", - error?.code, - ), - { status: error?.code }, - ), - ) - - throw new Error(`OpenRouter API Error ${error?.code}: ${error?.message}`) + this.handleStreamingError(response.error as OpenRouterErrorResponse, modelId, "completePrompt") } const completion = response as OpenAI.Chat.ChatCompletion diff --git a/src/core/assistant-message/NativeToolCallParser.ts b/src/core/assistant-message/NativeToolCallParser.ts index 9de8d7a8573..745dc7c495f 100644 --- a/src/core/assistant-message/NativeToolCallParser.ts +++ b/src/core/assistant-message/NativeToolCallParser.ts @@ -7,12 +7,14 @@ import { toolParamNames, type NativeToolArgs, } from "../../shared/tools" +import { resolveToolAlias } from "../prompts/tools/filter-tools-for-mode" import { parseJSON } from "partial-json" import type { ApiStreamToolCallStartChunk, ApiStreamToolCallDeltaChunk, ApiStreamToolCallEndChunk, } from "../../api/transform/stream" +import { MCP_TOOL_PREFIX, MCP_TOOL_SEPARATOR, parseMcpToolName } from "../../utils/mcp-name" /** * Helper type to extract properly typed native arguments for a given tool. @@ -238,7 +240,8 @@ export class NativeToolCallParser { toolCall.argumentsAccumulator += chunk // For dynamic MCP tools, we don't return partial updates - wait for final - if (toolCall.name.startsWith("mcp_")) { + const mcpPrefix = MCP_TOOL_PREFIX + MCP_TOOL_SEPARATOR + if (toolCall.name.startsWith(mcpPrefix)) { return null } @@ -247,12 +250,18 @@ export class NativeToolCallParser { try { const partialArgs = parseJSON(toolCall.argumentsAccumulator) + // Resolve tool alias to canonical name + const resolvedName = resolveToolAlias(toolCall.name) as ToolName + // Preserve original name if it differs from resolved (i.e., it was an alias) + const originalName = toolCall.name !== resolvedName ? toolCall.name : undefined + // Create partial ToolUse with extracted values return this.createPartialToolUse( toolCall.id, - toolCall.name as ToolName, + resolvedName, partialArgs || {}, true, // partial + originalName, ) } catch { // Even partial-json-parser can fail on severely malformed JSON @@ -328,12 +337,14 @@ export class NativeToolCallParser { /** * Create a partial ToolUse from currently parsed arguments. * Used during streaming to show progress. + * @param originalName - The original tool name as called by the model (if different from canonical name) */ private static createPartialToolUse( id: string, name: ToolName, partialArgs: Record, partial: boolean, + originalName?: string, ): ToolUse | null { // Build legacy params for display // NOTE: For streaming partial updates, we MUST populate params even for complex types @@ -506,18 +517,33 @@ export class NativeToolCallParser { } break - // Add other tools as needed + case "search_and_replace": + if (partialArgs.path !== undefined || partialArgs.operations !== undefined) { + nativeArgs = { + path: partialArgs.path, + operations: partialArgs.operations, + } + } + break + default: break } - return { + const result: ToolUse = { type: "tool_use" as const, name, params, partial, nativeArgs, } + + // Preserve original name for API history when an alias was used + if (originalName) { + result.originalName = originalName + } + + return result } /** @@ -531,14 +557,18 @@ export class NativeToolCallParser { name: TName arguments: string }): ToolUse | McpToolUse | null { - // Check if this is a dynamic MCP tool (mcp_serverName_toolName) - if (typeof toolCall.name === "string" && toolCall.name.startsWith("mcp_")) { + // Check if this is a dynamic MCP tool (mcp--serverName--toolName) + const mcpPrefix = MCP_TOOL_PREFIX + MCP_TOOL_SEPARATOR + if (typeof toolCall.name === "string" && toolCall.name.startsWith(mcpPrefix)) { return this.parseDynamicMcpTool(toolCall) } - // Validate tool name - if (!toolNames.includes(toolCall.name as ToolName)) { - console.error(`Invalid tool name: ${toolCall.name}`) + // Resolve tool alias to canonical name (e.g., "edit_file" -> "apply_diff", "temp_edit_file" -> "search_and_replace") + const resolvedName = resolveToolAlias(toolCall.name as string) as TName + + // Validate tool name (after alias resolution) + if (!toolNames.includes(resolvedName as ToolName)) { + console.error(`Invalid tool name: ${toolCall.name} (resolved: ${resolvedName})`) console.error(`Valid tool names:`, toolNames) return null } @@ -555,13 +585,13 @@ export class NativeToolCallParser { // Skip complex parameters that have been migrated to nativeArgs. // For read_file, the 'files' parameter is a FileEntry[] array that can't be // meaningfully stringified. The properly typed data is in nativeArgs instead. - if (toolCall.name === "read_file" && key === "files") { + if (resolvedName === "read_file" && key === "files") { continue } // Validate parameter name if (!toolParamNames.includes(key as ToolParamName)) { - console.warn(`Unknown parameter '${key}' for tool '${toolCall.name}'`) + console.warn(`Unknown parameter '${key}' for tool '${resolvedName}'`) console.warn(`Valid param names:`, toolParamNames) continue } @@ -581,7 +611,7 @@ export class NativeToolCallParser { // will fall back to legacy parameter parsing if supported. let nativeArgs: NativeArgsFor | undefined = undefined - switch (toolCall.name) { + switch (resolvedName) { case "read_file": if (args.files && Array.isArray(args.files)) { nativeArgs = { files: this.convertFileEntries(args.files) } as NativeArgsFor @@ -762,12 +792,17 @@ export class NativeToolCallParser { const result: ToolUse = { type: "tool_use" as const, - name: toolCall.name, + name: resolvedName, params, partial: false, // Native tool calls are always complete when yielded nativeArgs, } + // Preserve original name for API history when an alias was used + if (toolCall.name !== resolvedName) { + result.originalName = toolCall.name + } + return result } catch (error) { console.error( @@ -780,7 +815,7 @@ export class NativeToolCallParser { } /** - * Parse dynamic MCP tools (named mcp_serverName_toolName). + * Parse dynamic MCP tools (named mcp--serverName--toolName). * These are generated dynamically by getMcpServerTools() and are returned * as McpToolUse objects that preserve the original tool name. * @@ -794,26 +829,19 @@ export class NativeToolCallParser { const args = JSON.parse(toolCall.arguments || "{}") // Extract server_name and tool_name from the tool name itself - // Format: mcp_serverName_toolName - const nameParts = toolCall.name.split("_") - if (nameParts.length < 3 || nameParts[0] !== "mcp") { + // Format: mcp--serverName--toolName (using -- separator) + const parsed = parseMcpToolName(toolCall.name) + if (!parsed) { console.error(`Invalid dynamic MCP tool name format: ${toolCall.name}`) return null } - // Server name is the second part, tool name is everything after - const serverName = nameParts[1] - const toolName = nameParts.slice(2).join("_") - - if (!serverName || !toolName) { - console.error(`Could not extract server_name or tool_name from: ${toolCall.name}`) - return null - } + const { serverName, toolName } = parsed const result: McpToolUse = { type: "mcp_tool_use" as const, id: toolCall.id, - // Keep the original tool name (e.g., "mcp_serverName_toolName") for API history + // Keep the original tool name (e.g., "mcp--serverName--toolName") for API history name: toolCall.name, serverName, toolName, diff --git a/src/core/assistant-message/presentAssistantMessage.ts b/src/core/assistant-message/presentAssistantMessage.ts index 76e693572b3..fe57ef763ba 100644 --- a/src/core/assistant-message/presentAssistantMessage.ts +++ b/src/core/assistant-message/presentAssistantMessage.ts @@ -246,6 +246,18 @@ export async function presentAssistantMessage(cline: Task) { TelemetryService.instance.captureToolUsage(cline.taskId, "use_mcp_tool", toolProtocol) } + // Resolve sanitized server name back to original server name + // The serverName from parsing is sanitized (e.g., "my_server" from "my server") + // We need the original name to find the actual MCP connection + const mcpHub = cline.providerRef.deref()?.getMcpHub() + let resolvedServerName = mcpBlock.serverName + if (mcpHub) { + const originalName = mcpHub.findServerNameBySanitizedName(mcpBlock.serverName) + if (originalName) { + resolvedServerName = originalName + } + } + // Execute the MCP tool using the same handler as use_mcp_tool // Create a synthetic ToolUse block that the useMcpToolTool can handle const syntheticToolUse: ToolUse<"use_mcp_tool"> = { @@ -253,13 +265,13 @@ export async function presentAssistantMessage(cline: Task) { id: mcpBlock.id, name: "use_mcp_tool", params: { - server_name: mcpBlock.serverName, + server_name: resolvedServerName, tool_name: mcpBlock.toolName, arguments: JSON.stringify(mcpBlock.arguments), }, partial: mcpBlock.partial, nativeArgs: { - server_name: mcpBlock.serverName, + server_name: resolvedServerName, tool_name: mcpBlock.toolName, arguments: mcpBlock.arguments, }, @@ -714,7 +726,11 @@ export async function presentAssistantMessage(cline: Task) { // potentially causing the stream to appear frozen. if (!block.partial) { const modelInfo = cline.api.getModel() - const includedTools = modelInfo?.info?.includedTools + // Resolve aliases in includedTools before validation + // e.g., "edit_file" should resolve to "apply_diff" + const rawIncludedTools = modelInfo?.info?.includedTools + const { resolveToolAlias } = await import("../prompts/tools/filter-tools-for-mode") + const includedTools = rawIncludedTools?.map((tool) => resolveToolAlias(tool)) try { validateToolUse( diff --git a/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts b/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts index d2ccaa84fb0..d189b999150 100644 --- a/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts +++ b/src/core/prompts/tools/__tests__/filter-tools-for-mode.spec.ts @@ -487,7 +487,7 @@ describe("filterMcpToolsForMode", () => { it("should return original tools when modelInfo is undefined", () => { const tools = new Set(["read_file", "write_to_file", "apply_diff"]) const result = applyModelToolCustomization(tools, codeMode, undefined) - expect(result).toEqual(tools) + expect(result.allowedTools).toEqual(tools) }) it("should exclude tools specified in excludedTools", () => { @@ -498,9 +498,9 @@ describe("filterMcpToolsForMode", () => { excludedTools: ["apply_diff"], } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) - expect(result.has("apply_diff")).toBe(false) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) + expect(result.allowedTools.has("apply_diff")).toBe(false) }) it("should exclude multiple tools", () => { @@ -511,10 +511,10 @@ describe("filterMcpToolsForMode", () => { excludedTools: ["apply_diff", "write_to_file"], } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("execute_command")).toBe(true) - expect(result.has("write_to_file")).toBe(false) - expect(result.has("apply_diff")).toBe(false) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("execute_command")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(false) + expect(result.allowedTools.has("apply_diff")).toBe(false) }) it("should include tools only if they belong to allowed groups", () => { @@ -525,9 +525,9 @@ describe("filterMcpToolsForMode", () => { includedTools: ["write_to_file", "apply_diff"], // Both in edit group } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) - expect(result.has("apply_diff")).toBe(true) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) + expect(result.allowedTools.has("apply_diff")).toBe(true) }) it("should NOT include tools from groups not allowed by mode", () => { @@ -539,9 +539,9 @@ describe("filterMcpToolsForMode", () => { } // Architect mode doesn't have edit group const result = applyModelToolCustomization(tools, architectMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(false) // Not in allowed groups - expect(result.has("apply_diff")).toBe(false) // Not in allowed groups + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(false) // Not in allowed groups + expect(result.allowedTools.has("apply_diff")).toBe(false) // Not in allowed groups }) it("should apply both exclude and include operations", () => { @@ -553,10 +553,10 @@ describe("filterMcpToolsForMode", () => { includedTools: ["search_and_replace"], // Another edit tool (customTool) } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) - expect(result.has("apply_diff")).toBe(false) // Excluded - expect(result.has("search_and_replace")).toBe(true) // Included + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) + expect(result.allowedTools.has("apply_diff")).toBe(false) // Excluded + expect(result.allowedTools.has("search_and_replace")).toBe(true) // Included }) it("should handle empty excludedTools and includedTools arrays", () => { @@ -568,7 +568,7 @@ describe("filterMcpToolsForMode", () => { includedTools: [], } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result).toEqual(tools) + expect(result.allowedTools).toEqual(tools) }) it("should ignore excluded tools that are not in the original set", () => { @@ -579,9 +579,9 @@ describe("filterMcpToolsForMode", () => { excludedTools: ["apply_diff", "nonexistent_tool"], } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) - expect(result.size).toBe(2) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) + expect(result.allowedTools.size).toBe(2) }) it("should NOT include customTools by default", () => { @@ -594,8 +594,8 @@ describe("filterMcpToolsForMode", () => { } const result = applyModelToolCustomization(tools, codeMode, modelInfo) // customTools should not be in the result unless explicitly included - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) }) it("should NOT include tools that are not in any TOOL_GROUPS", () => { @@ -606,8 +606,8 @@ describe("filterMcpToolsForMode", () => { includedTools: ["my_custom_tool"], // Not in any tool group } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("my_custom_tool")).toBe(false) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("my_custom_tool")).toBe(false) }) it("should NOT include undefined tools even with allowed groups", () => { @@ -619,8 +619,8 @@ describe("filterMcpToolsForMode", () => { } // Even though architect mode has read group, undefined tools are not added const result = applyModelToolCustomization(tools, architectMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("custom_edit_tool")).toBe(false) + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("custom_edit_tool")).toBe(false) }) describe("with customTools defined in TOOL_GROUPS", () => { @@ -647,9 +647,9 @@ describe("filterMcpToolsForMode", () => { includedTools: ["special_edit_tool"], // customTool from edit group } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) - expect(result.has("special_edit_tool")).toBe(true) // customTool should be included + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) + expect(result.allowedTools.has("special_edit_tool")).toBe(true) // customTool should be included }) it("should NOT include customTools when not specified in includedTools", () => { @@ -660,9 +660,9 @@ describe("filterMcpToolsForMode", () => { // No includedTools specified } const result = applyModelToolCustomization(tools, codeMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("write_to_file")).toBe(true) - expect(result.has("special_edit_tool")).toBe(false) // customTool should NOT be included by default + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("write_to_file")).toBe(true) + expect(result.allowedTools.has("special_edit_tool")).toBe(false) // customTool should NOT be included by default }) it("should NOT include customTools from groups not allowed by mode", () => { @@ -674,8 +674,8 @@ describe("filterMcpToolsForMode", () => { } // Architect mode doesn't have edit group const result = applyModelToolCustomization(tools, architectMode, modelInfo) - expect(result.has("read_file")).toBe(true) - expect(result.has("special_edit_tool")).toBe(false) // customTool should NOT be included + expect(result.allowedTools.has("read_file")).toBe(true) + expect(result.allowedTools.has("special_edit_tool")).toBe(false) // customTool should NOT be included }) }) }) @@ -822,5 +822,31 @@ describe("filterMcpToolsForMode", () => { expect(toolNames).toContain("search_and_replace") // Included expect(toolNames).not.toContain("apply_diff") // Excluded }) + + it("should rename tools to alias names when model includes aliases", () => { + const codeMode: ModeConfig = { + slug: "code", + name: "Code", + roleDefinition: "Test", + groups: ["read", "edit", "browser", "command", "mcp"] as const, + } + + const modelInfo: ModelInfo = { + contextWindow: 100000, + supportsPromptCache: false, + includedTools: ["edit_file", "write_file"], + } + + const filtered = filterNativeToolsForMode(mockNativeTools, "code", [codeMode], {}, undefined, { + modelInfo, + }) + + const toolNames = filtered.map((t) => ("function" in t ? t.function.name : "")) + + expect(toolNames).toContain("edit_file") + expect(toolNames).toContain("write_file") + expect(toolNames).not.toContain("apply_diff") + expect(toolNames).not.toContain("write_to_file") + }) }) }) diff --git a/src/core/prompts/tools/filter-tools-for-mode.ts b/src/core/prompts/tools/filter-tools-for-mode.ts index eb87c9bbeca..3c1b2e3676d 100644 --- a/src/core/prompts/tools/filter-tools-for-mode.ts +++ b/src/core/prompts/tools/filter-tools-for-mode.ts @@ -1,11 +1,131 @@ import type OpenAI from "openai" import type { ModeConfig, ToolName, ToolGroup, ModelInfo } from "@roo-code/types" import { getModeBySlug, getToolsForMode, isToolAllowedForMode } from "../../../shared/modes" -import { TOOL_GROUPS, ALWAYS_AVAILABLE_TOOLS } from "../../../shared/tools" +import { TOOL_GROUPS, ALWAYS_AVAILABLE_TOOLS, TOOL_ALIASES } from "../../../shared/tools" import { defaultModeSlug } from "../../../shared/modes" import type { CodeIndexManager } from "../../../services/code-index/manager" import type { McpHub } from "../../../services/mcp/McpHub" +/** + * Reverse lookup map - maps alias name to canonical tool name. + * Built once at module load from the central TOOL_ALIASES constant. + */ +const ALIAS_TO_CANONICAL: Map = new Map( + Object.entries(TOOL_ALIASES).map(([alias, canonical]) => [alias, canonical]), +) + +/** + * Canonical to aliases map - maps canonical tool name to array of alias names. + * Built once at module load from the central TOOL_ALIASES constant. + */ +const CANONICAL_TO_ALIASES: Map = new Map() + +// Build the reverse mapping (canonical -> aliases) +for (const [alias, canonical] of Object.entries(TOOL_ALIASES)) { + const existing = CANONICAL_TO_ALIASES.get(canonical) ?? [] + existing.push(alias) + CANONICAL_TO_ALIASES.set(canonical, existing) +} + +/** + * Pre-computed alias groups map - maps any tool name (canonical or alias) to its full group. + * Built once at module load for O(1) lookup. + */ +const ALIAS_GROUPS: Map = new Map() + +// Build alias groups for all tools +for (const [canonical, aliases] of CANONICAL_TO_ALIASES.entries()) { + const group = Object.freeze([canonical, ...aliases]) + // Map canonical to group + ALIAS_GROUPS.set(canonical, group) + // Map each alias to the same group + for (const alias of aliases) { + ALIAS_GROUPS.set(alias, group) + } +} + +/** + * Cache for renamed tool definitions. + * Maps "canonicalName:aliasName" to the pre-built tool definition. + * This avoids creating new objects via spread operators on every assistant message. + */ +const RENAMED_TOOL_CACHE: Map = new Map() + +/** + * Gets or creates a renamed tool definition with the alias name. + * Uses RENAMED_TOOL_CACHE to avoid repeated object allocation. + * + * @param tool - The original tool definition + * @param aliasName - The alias name to use + * @returns Cached or newly created renamed tool definition + */ +function getOrCreateRenamedTool( + tool: OpenAI.Chat.ChatCompletionTool, + aliasName: string, +): OpenAI.Chat.ChatCompletionTool { + if (!("function" in tool) || !tool.function) { + return tool + } + + const cacheKey = `${tool.function.name}:${aliasName}` + let renamedTool = RENAMED_TOOL_CACHE.get(cacheKey) + + if (!renamedTool) { + renamedTool = { + ...tool, + function: { + ...tool.function, + name: aliasName, + }, + } + RENAMED_TOOL_CACHE.set(cacheKey, renamedTool) + } + + return renamedTool +} + +/** + * Resolves a tool name to its canonical name. + * If the tool name is an alias, returns the canonical tool name. + * If it's already a canonical name or unknown, returns as-is. + * + * @param toolName - The tool name to resolve (may be an alias) + * @returns The canonical tool name + */ +export function resolveToolAlias(toolName: string): string { + const canonical = ALIAS_TO_CANONICAL.get(toolName) + return canonical ?? toolName +} + +/** + * Applies tool alias resolution to a set of allowed tools. + * Resolves any aliases to their canonical tool names. + * + * @param allowedTools - Set of tools that may contain aliases + * @returns Set with aliases resolved to canonical names + */ +export function applyToolAliases(allowedTools: Set): Set { + const result = new Set() + + for (const tool of allowedTools) { + // Resolve alias to canonical name + result.add(resolveToolAlias(tool)) + } + + return result +} + +/** + * Gets all tools in an alias group (including the canonical tool). + * Uses pre-computed ALIAS_GROUPS map for O(1) lookup. + * + * @param toolName - Any tool name in the alias group + * @returns Array of all tool names in the alias group, or just the tool if not aliased + */ +export function getToolAliasGroup(toolName: string): readonly string[] { + return ALIAS_GROUPS.get(toolName) ?? [toolName] +} + /** * Apply model-specific tool customization to a set of allowed tools. * @@ -18,21 +138,33 @@ import type { McpHub } from "../../../services/mcp/McpHub" * @param modelInfo - Model configuration with tool customization * @returns Modified set of tools after applying model customization */ +/** + * Result of applying model tool customization. + * Contains the set of allowed tools and any alias renames to apply. + */ +interface ModelToolCustomizationResult { + allowedTools: Set + /** Maps canonical tool name to alias name for tools that should be renamed */ + aliasRenames: Map +} + export function applyModelToolCustomization( allowedTools: Set, modeConfig: ModeConfig, modelInfo?: ModelInfo, -): Set { +): ModelToolCustomizationResult { if (!modelInfo) { - return allowedTools + return { allowedTools, aliasRenames: new Map() } } const result = new Set(allowedTools) + const aliasRenames = new Map() // Apply excluded tools (remove from allowed set) if (modelInfo.excludedTools && modelInfo.excludedTools.length > 0) { modelInfo.excludedTools.forEach((tool) => { - result.delete(tool) + const resolvedTool = resolveToolAlias(tool) + result.delete(resolvedTool) }) } @@ -59,16 +191,21 @@ export function applyModelToolCustomization( ) // Add included tools only if they belong to an allowed group - // This includes both regular tools and customTools + // If the tool was specified as an alias, track the rename modelInfo.includedTools.forEach((tool) => { - const toolGroup = toolToGroup.get(tool) + const resolvedTool = resolveToolAlias(tool) + const toolGroup = toolToGroup.get(resolvedTool) if (toolGroup && allowedGroups.has(toolGroup)) { - result.add(tool) + result.add(resolvedTool) + // If the tool was specified as an alias, rename it in the API + if (tool !== resolvedTool) { + aliasRenames.set(resolvedTool, tool) + } } }) } - return result + return { allowedTools: result, aliasRenames } } /** @@ -123,7 +260,12 @@ export function filterNativeToolsForMode( // Apply model-specific tool customization const modelInfo = settings?.modelInfo as ModelInfo | undefined - allowedToolNames = applyModelToolCustomization(allowedToolNames, modeConfig, modelInfo) + const { allowedTools: customizedTools, aliasRenames } = applyModelToolCustomization( + allowedToolNames, + modeConfig, + modelInfo, + ) + allowedToolNames = customizedTools // Conditionally exclude codebase_search if feature is disabled or not configured if ( @@ -163,14 +305,27 @@ export function filterNativeToolsForMode( allowedToolNames.delete("access_mcp_resource") } - // Filter native tools based on allowed tool names - return nativeTools.filter((tool) => { + // Filter native tools based on allowed tool names and apply alias renames + const filteredTools: OpenAI.Chat.ChatCompletionTool[] = [] + + for (const tool of nativeTools) { // Handle both ChatCompletionTool and ChatCompletionCustomTool if ("function" in tool && tool.function) { - return allowedToolNames.has(tool.function.name) + const toolName = tool.function.name + if (allowedToolNames.has(toolName)) { + // Check if this tool should be renamed to an alias + const aliasName = aliasRenames.get(toolName) + if (aliasName) { + // Use cached renamed tool definition to avoid per-message object allocation + filteredTools.push(getOrCreateRenamedTool(tool, aliasName)) + } else { + filteredTools.push(tool) + } + } } - return false - }) + } + + return filteredTools } /** @@ -232,7 +387,16 @@ export function isToolAllowedInMode( } // Check if the tool is allowed by the mode's groups - return isToolAllowedForMode(toolName, modeSlug, customModes ?? [], undefined, undefined, experiments ?? {}) + // Resolve to canonical name and check that single value + const canonicalTool = resolveToolAlias(toolName) + return isToolAllowedForMode( + canonicalTool as ToolName, + modeSlug, + customModes ?? [], + undefined, + undefined, + experiments ?? {}, + ) } /** diff --git a/src/core/prompts/tools/native-tools/mcp_server.ts b/src/core/prompts/tools/native-tools/mcp_server.ts index 36efdf83fb4..f40da7cf500 100644 --- a/src/core/prompts/tools/native-tools/mcp_server.ts +++ b/src/core/prompts/tools/native-tools/mcp_server.ts @@ -1,5 +1,6 @@ import type OpenAI from "openai" import { McpHub } from "../../../../services/mcp/McpHub" +import { buildMcpToolName } from "../../../../utils/mcp-name" /** * Dynamically generates native tool definitions for all enabled tools across connected MCP servers. @@ -43,11 +44,14 @@ export function getMcpServerTools(mcpHub?: McpHub): OpenAI.Chat.ChatCompletionTo parameters.required = toolInputRequired } - // Use mcp_ prefix to identify dynamic MCP tools + // Build sanitized tool name for API compliance + // The name is sanitized to conform to API requirements (e.g., Gemini's function name restrictions) + const toolName = buildMcpToolName(server.name, tool.name) + const toolDefinition: OpenAI.Chat.ChatCompletionTool = { type: "function", function: { - name: `mcp_${server.name}_${tool.name}`, + name: toolName, description: tool.description, parameters: parameters, }, diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 3a71ccbb438..87fbcf68253 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -3290,10 +3290,16 @@ export class Task extends EventEmitter implements TaskLike { // nativeArgs is already in the correct API format for all tools const input = toolUse.nativeArgs || toolUse.params + // Use originalName (alias) if present for API history consistency. + // When tool aliases are used (e.g., "edit_file" -> "search_and_replace"), + // we want the alias name in the conversation history to match what the model + // was told the tool was named, preventing confusion in multi-turn conversations. + const toolNameForHistory = toolUse.originalName ?? toolUse.name + assistantContent.push({ type: "tool_use" as const, id: toolCallId, - name: toolUse.name, + name: toolNameForHistory, input, }) } diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index a5cef0751c9..a6fa45e49fc 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -34,6 +34,7 @@ import { arePathsEqual, getWorkspacePath } from "../../utils/path" import { injectVariables } from "../../utils/config" import { NotificationService } from "./costrict/NotificationService" import { safeWriteJson } from "../../utils/safeWriteJson" +import { sanitizeMcpName } from "../../utils/mcp-name" // Discriminated union for connection states export type ConnectedMcpConnection = { @@ -156,6 +157,7 @@ export class McpHub { private configChangeDebounceTimers: Map = new Map() private isProgrammaticUpdate: boolean = false private flagResetTimer?: NodeJS.Timeout + private sanitizedNameRegistry: Map = new Map() constructor(provider: ClineProvider) { this.providerRef = new WeakRef(provider) @@ -300,7 +302,7 @@ export class McpHub { const timer = setTimeout(async () => { this.configChangeDebounceTimers.delete(key) await this.handleConfigFileChange(filePath, source) - }, 600) // 600ms debounce + }, 500) // 500ms debounce this.configChangeDebounceTimers.set(key, timer) } @@ -629,6 +631,10 @@ export class McpHub { // Remove existing connection if it exists with the same source await this.deleteConnection(name, source) + // Register the sanitized name for O(1) lookup + const sanitizedName = sanitizeMcpName(name) + this.sanitizedNameRegistry.set(sanitizedName, name) + // Check if MCP is globally enabled const mcpEnabled = await this.isMcpEnabled() if (!mcpEnabled) { @@ -925,15 +931,33 @@ export class McpHub { return } - if (connection.client.getServerCapabilities()?.tools) { + const { tools, resources } = connection.client.getServerCapabilities() ?? {} + + if (tools) { connection.server.tools = await this.fetchToolsList(serverName, source) } - if (connection.client.getServerCapabilities()?.resources) { + if (resources) { connection.server.resources = await this.fetchResourcesList(serverName, source) connection.server.resourceTemplates = await this.fetchResourceTemplatesList(serverName, source) } } + /** + * Find a connection by sanitized server name. + * This is used when parsing MCP tool responses where the server name has been + * sanitized (e.g., hyphens replaced with underscores) for API compliance. + * @param sanitizedServerName The sanitized server name from the API tool call + * @returns The original server name if found, or null if no match + */ + public findServerNameBySanitizedName(sanitizedServerName: string): string | null { + const exactMatch = this.connections.find((conn) => conn.server.name === sanitizedServerName) + if (exactMatch) { + return exactMatch.server.name + } + + return this.sanitizedNameRegistry.get(sanitizedServerName) ?? null + } + private async fetchToolsList(serverName: string, source?: "global" | "project"): Promise { try { // Use the helper method to find the connection @@ -1068,6 +1092,13 @@ export class McpHub { if (source && conn.server.source !== source) return true return false }) + + // Remove from sanitized name registry if no more connections with this name exist + const remainingConnections = this.connections.filter((conn) => conn.server.name === name) + if (remainingConnections.length === 0) { + const sanitizedName = sanitizeMcpName(name) + this.sanitizedNameRegistry.delete(sanitizedName) + } } async updateServerConnections( diff --git a/src/shared/tools.ts b/src/shared/tools.ts index e7b95d2a6c0..1146633b15b 100644 --- a/src/shared/tools.ts +++ b/src/shared/tools.ts @@ -122,6 +122,12 @@ export interface ToolUse { type: "tool_use" id?: string // Optional ID to track tool calls name: TName + /** + * The original tool name as called by the model (e.g. an alias like "edit_file"), + * if it differs from the canonical tool name used for execution. + * Used to preserve tool names in API conversation history. + */ + originalName?: string // params is a partial record, allowing only some or none of the possible parameters to be used params: Partial> partial: boolean @@ -309,6 +315,22 @@ export const ALWAYS_AVAILABLE_TOOLS: ToolName[] = [ "run_slash_command", ] as const +/** + * Central registry of tool aliases. + * Maps alias name -> canonical tool name. + * + * This allows models to use alternative names for tools (e.g., "edit_file" instead of "apply_diff"). + * When a model calls a tool by its alias, the system resolves it to the canonical name for execution, + * but preserves the alias in API conversation history for consistency. + * + * To add a new alias, simply add an entry here. No other files need to be modified. + */ +export const TOOL_ALIASES: Record = { + edit_file: "apply_diff", + write_file: "write_to_file", + temp_edit_file: "search_and_replace", +} as const + export type DiffResult = | { success: true; content: string; failParts?: DiffResult[] } | ({ diff --git a/src/utils/__tests__/mcp-name.spec.ts b/src/utils/__tests__/mcp-name.spec.ts new file mode 100644 index 00000000000..76c069ee8d0 --- /dev/null +++ b/src/utils/__tests__/mcp-name.spec.ts @@ -0,0 +1,178 @@ +import { sanitizeMcpName, buildMcpToolName, parseMcpToolName, MCP_TOOL_SEPARATOR, MCP_TOOL_PREFIX } from "../mcp-name" + +describe("mcp-name utilities", () => { + describe("constants", () => { + it("should have correct separator and prefix", () => { + expect(MCP_TOOL_SEPARATOR).toBe("--") + expect(MCP_TOOL_PREFIX).toBe("mcp") + }) + }) + + describe("sanitizeMcpName", () => { + it("should return underscore placeholder for empty input", () => { + expect(sanitizeMcpName("")).toBe("_") + }) + + it("should replace spaces with underscores", () => { + expect(sanitizeMcpName("my server")).toBe("my_server") + expect(sanitizeMcpName("server name here")).toBe("server_name_here") + }) + + it("should remove invalid characters", () => { + expect(sanitizeMcpName("server@name!")).toBe("servername") + expect(sanitizeMcpName("test#$%^&*()")).toBe("test") + }) + + it("should keep valid characters (alphanumeric, underscore, dot, colon, dash)", () => { + expect(sanitizeMcpName("server_name")).toBe("server_name") + expect(sanitizeMcpName("server.name")).toBe("server.name") + expect(sanitizeMcpName("server:name")).toBe("server:name") + expect(sanitizeMcpName("server-name")).toBe("server-name") + expect(sanitizeMcpName("Server123")).toBe("Server123") + }) + + it("should prepend underscore if name starts with non-letter/underscore", () => { + expect(sanitizeMcpName("123server")).toBe("_123server") + expect(sanitizeMcpName("-server")).toBe("_-server") + expect(sanitizeMcpName(".server")).toBe("_.server") + }) + + it("should not modify names that start with letter or underscore", () => { + expect(sanitizeMcpName("server")).toBe("server") + expect(sanitizeMcpName("_server")).toBe("_server") + expect(sanitizeMcpName("Server")).toBe("Server") + }) + + it("should replace double-hyphen sequences with single hyphen to avoid separator conflicts", () => { + expect(sanitizeMcpName("server--name")).toBe("server-name") + expect(sanitizeMcpName("test---server")).toBe("test-server") + expect(sanitizeMcpName("my----tool")).toBe("my-tool") + }) + + it("should handle complex names with multiple issues", () => { + expect(sanitizeMcpName("My Server @ Home!")).toBe("My_Server__Home") + expect(sanitizeMcpName("123-test server")).toBe("_123-test_server") + }) + + it("should return placeholder for names that become empty after sanitization", () => { + expect(sanitizeMcpName("@#$%")).toBe("_unnamed") + // Spaces become underscores, which is a valid character, so it returns "_" + expect(sanitizeMcpName(" ")).toBe("_") + }) + }) + + describe("buildMcpToolName", () => { + it("should build tool name with mcp-- prefix and -- separators", () => { + expect(buildMcpToolName("server", "tool")).toBe("mcp--server--tool") + }) + + it("should sanitize both server and tool names", () => { + expect(buildMcpToolName("my server", "my tool")).toBe("mcp--my_server--my_tool") + }) + + it("should handle names with special characters", () => { + expect(buildMcpToolName("server@name", "tool!name")).toBe("mcp--servername--toolname") + }) + + it("should truncate long names to 64 characters", () => { + const longServer = "a".repeat(50) + const longTool = "b".repeat(50) + const result = buildMcpToolName(longServer, longTool) + expect(result.length).toBeLessThanOrEqual(64) + expect(result.startsWith("mcp--")).toBe(true) + }) + + it("should handle names starting with numbers", () => { + expect(buildMcpToolName("123server", "456tool")).toBe("mcp--_123server--_456tool") + }) + + it("should preserve underscores in server and tool names", () => { + expect(buildMcpToolName("my_server", "my_tool")).toBe("mcp--my_server--my_tool") + }) + }) + + describe("parseMcpToolName", () => { + it("should parse valid mcp tool names", () => { + expect(parseMcpToolName("mcp--server--tool")).toEqual({ + serverName: "server", + toolName: "tool", + }) + }) + + it("should return null for non-mcp tool names", () => { + expect(parseMcpToolName("server--tool")).toBeNull() + expect(parseMcpToolName("tool")).toBeNull() + }) + + it("should return null for old underscore format", () => { + expect(parseMcpToolName("mcp_server_tool")).toBeNull() + }) + + it("should handle tool names with underscores", () => { + expect(parseMcpToolName("mcp--server--tool_name")).toEqual({ + serverName: "server", + toolName: "tool_name", + }) + }) + + it("should correctly handle server names with underscores (fixed from old behavior)", () => { + // With the new -- separator, server names with underscores work correctly + expect(parseMcpToolName("mcp--my_server--tool")).toEqual({ + serverName: "my_server", + toolName: "tool", + }) + }) + + it("should handle both server and tool names with underscores", () => { + expect(parseMcpToolName("mcp--my_server--get_forecast")).toEqual({ + serverName: "my_server", + toolName: "get_forecast", + }) + }) + + it("should return null for malformed names", () => { + expect(parseMcpToolName("mcp--")).toBeNull() + expect(parseMcpToolName("mcp--server")).toBeNull() + }) + }) + + describe("roundtrip behavior", () => { + it("should be able to parse names that were built", () => { + const toolName = buildMcpToolName("server", "tool") + const parsed = parseMcpToolName(toolName) + expect(parsed).toEqual({ + serverName: "server", + toolName: "tool", + }) + }) + + it("should preserve sanitized names through roundtrip with underscores", () => { + // Names with underscores now work correctly through roundtrip + const toolName = buildMcpToolName("my_server", "my_tool") + const parsed = parseMcpToolName(toolName) + expect(parsed).toEqual({ + serverName: "my_server", + toolName: "my_tool", + }) + }) + + it("should handle spaces that get converted to underscores", () => { + // "my server" becomes "my_server" after sanitization + const toolName = buildMcpToolName("my server", "get tool") + const parsed = parseMcpToolName(toolName) + expect(parsed).toEqual({ + serverName: "my_server", + toolName: "get_tool", + }) + }) + + it("should handle complex server and tool names", () => { + const toolName = buildMcpToolName("Weather API", "get_current_forecast") + const parsed = parseMcpToolName(toolName) + expect(parsed).toEqual({ + serverName: "Weather_API", + toolName: "get_current_forecast", + }) + }) + }) +}) diff --git a/src/utils/mcp-name.ts b/src/utils/mcp-name.ts new file mode 100644 index 00000000000..af50f392cfe --- /dev/null +++ b/src/utils/mcp-name.ts @@ -0,0 +1,120 @@ +/** + * Utilities for sanitizing MCP server and tool names to conform to + * API function name requirements (e.g., Gemini's restrictions). + * + * Gemini function name requirements: + * - Must start with a letter or an underscore + * - Must be alphanumeric (a-z, A-Z, 0-9), underscores (_), dots (.), colons (:), or dashes (-) + * - Maximum length of 64 characters + */ + +/** + * Separator used between MCP prefix, server name, and tool name. + * We use "--" (double hyphen) because: + * 1. It's allowed by Gemini (dashes are permitted in function names) + * 2. It won't conflict with underscores in sanitized server/tool names + * 3. It's unique enough to be a reliable delimiter for parsing + */ +export const MCP_TOOL_SEPARATOR = "--" + +/** + * Prefix for all MCP tool function names. + */ +export const MCP_TOOL_PREFIX = "mcp" + +/** + * Sanitize a name to be safe for use in API function names. + * This removes special characters and ensures the name starts correctly. + * + * Note: This does NOT remove dashes from names, but the separator "--" is + * distinct enough (double hyphen) that single hyphens in names won't conflict. + * + * @param name - The original name (e.g., MCP server name or tool name) + * @returns A sanitized name that conforms to API requirements + */ +export function sanitizeMcpName(name: string): string { + if (!name) { + return "_" + } + + // Replace spaces with underscores first + let sanitized = name.replace(/\s+/g, "_") + + // Remove any characters that are not alphanumeric, underscores, dots, colons, or dashes + sanitized = sanitized.replace(/[^a-zA-Z0-9_.\-:]/g, "") + + // Replace any double-hyphen sequences with single hyphen to avoid separator conflicts + sanitized = sanitized.replace(/--+/g, "-") + + // Ensure the name starts with a letter or underscore + if (sanitized.length > 0 && !/^[a-zA-Z_]/.test(sanitized)) { + sanitized = "_" + sanitized + } + + // If empty after sanitization, use a placeholder + if (!sanitized) { + sanitized = "_unnamed" + } + + return sanitized +} + +/** + * Build a full MCP tool function name from server and tool names. + * The format is: mcp--{sanitized_server_name}--{sanitized_tool_name} + * + * The total length is capped at 64 characters to conform to API limits. + * + * @param serverName - The MCP server name + * @param toolName - The tool name + * @returns A sanitized function name in the format mcp--serverName--toolName + */ +export function buildMcpToolName(serverName: string, toolName: string): string { + const sanitizedServer = sanitizeMcpName(serverName) + const sanitizedTool = sanitizeMcpName(toolName) + + // Build the full name: mcp--{server}--{tool} + const fullName = `${MCP_TOOL_PREFIX}${MCP_TOOL_SEPARATOR}${sanitizedServer}${MCP_TOOL_SEPARATOR}${sanitizedTool}` + + // Truncate if necessary (max 64 chars for Gemini) + if (fullName.length > 64) { + return fullName.slice(0, 64) + } + + return fullName +} + +/** + * Parse an MCP tool function name back into server and tool names. + * This handles sanitized names by splitting on the "--" separator. + * + * Note: This returns the sanitized names, not the original names. + * The original names cannot be recovered from the sanitized version. + * + * @param mcpToolName - The full MCP tool name (e.g., "mcp--weather--get_forecast") + * @returns An object with serverName and toolName, or null if parsing fails + */ +export function parseMcpToolName(mcpToolName: string): { serverName: string; toolName: string } | null { + const prefix = MCP_TOOL_PREFIX + MCP_TOOL_SEPARATOR + if (!mcpToolName.startsWith(prefix)) { + return null + } + + // Remove the "mcp--" prefix + const remainder = mcpToolName.slice(prefix.length) + + // Split on the separator to get server and tool names + const separatorIndex = remainder.indexOf(MCP_TOOL_SEPARATOR) + if (separatorIndex === -1) { + return null + } + + const serverName = remainder.slice(0, separatorIndex) + const toolName = remainder.slice(separatorIndex + MCP_TOOL_SEPARATOR.length) + + if (!serverName || !toolName) { + return null + } + + return { serverName, toolName } +} diff --git a/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx b/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx index 7f5d08c9918..c70392e630c 100644 --- a/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx +++ b/webview-ui/src/components/chat/checkpoints/CheckpointSaved.tsx @@ -1,4 +1,4 @@ -import { useMemo, useRef, /*useState*/ useEffect } from "react" +import { useMemo, useRef, useState, useEffect } from "react" import { useTranslation } from "react-i18next" import { cn } from "@/lib/utils" @@ -16,8 +16,9 @@ type CheckpointSavedProps = { export const CheckpointSaved = ({ checkpoint, currentHash, ...props }: CheckpointSavedProps) => { const { t } = useTranslation() const isCurrent = currentHash === props.commitHash - // const [isPopoverOpen, setIsPopoverOpen] = useState(false) - // const [isClosing, setIsClosing] = useState(false) + const [isPopoverOpen, setIsPopoverOpen] = useState(false) + const [isClosing, setIsClosing] = useState(false) + const [isHovering, setIsHovering] = useState(false) const closeTimer = useRef(null) useEffect(() => { @@ -30,22 +31,33 @@ export const CheckpointSaved = ({ checkpoint, currentHash, ...props }: Checkpoin }, []) const handlePopoverOpenChange = (open: boolean) => { - // setIsPopoverOpen(open) + setIsPopoverOpen(open) if (open) { - // setIsClosing(false) + setIsClosing(false) if (closeTimer.current) { window.clearTimeout(closeTimer.current) closeTimer.current = null } } else { - // setIsClosing(true) + setIsClosing(true) closeTimer.current = window.setTimeout(() => { - // setIsClosing(false) + setIsClosing(false) closeTimer.current = null }, 200) // keep menu visible briefly to avoid popover jump } } + const handleMouseEnter = () => { + setIsHovering(true) + } + + const handleMouseLeave = () => { + setIsHovering(false) + } + + // Menu is visible when hovering, popover is open, or briefly after popover closes + const menuVisible = isHovering || isPopoverOpen || isClosing + const metadata = useMemo(() => { if (!checkpoint) { return undefined @@ -65,7 +77,10 @@ export const CheckpointSaved = ({ checkpoint, currentHash, ...props }: Checkpoin } return ( -
+
{t("chat:checkpoint.regular")} @@ -78,8 +93,8 @@ export const CheckpointSaved = ({ checkpoint, currentHash, ...props }: Checkpoin "linear-gradient(90deg, rgba(0, 188, 255, .65), rgba(0, 188, 255, .65) 80%, rgba(0, 188, 255, 0) 99%)", }}> - {/* Keep menu visible while popover is open or briefly after close to prevent jump */} -
+ {/* Keep menu visible while hovering, popover is open, or briefly after close to prevent jump */} +
{ } }) -import { render, waitFor, screen } from "@/utils/test-utils" +import { render, waitFor, screen, fireEvent } from "@/utils/test-utils" import React from "react" import userEvent from "@testing-library/user-event" import { CheckpointSaved } from "../CheckpointSaved" @@ -54,9 +54,9 @@ describe("CheckpointSaved popover visibility", () => { const getMenu = () => getByTestId("checkpoint-menu-container") as HTMLElement - // Initially hidden (relies on group-hover) + // Initially hidden (not hovering) expect(getMenu()).toBeTruthy() - // expect(getMenu().className).toContain("hidden") + expect(getMenu().className).toContain("hidden") // Open via captured handler await waitForOpenHandler() @@ -64,7 +64,7 @@ describe("CheckpointSaved popover visibility", () => { await waitFor(() => { expect(getMenu().className).toContain("block") - // expect(getMenu().className).not.toContain("hidden") + expect(getMenu().className).not.toContain("hidden") }) // Close via captured handler — menu remains visible briefly, then hides @@ -74,13 +74,18 @@ describe("CheckpointSaved popover visibility", () => { expect(getMenu().className).toContain("block") }) - // await waitFor(() => { - // expect(getMenu().className).toContain("hidden") - // }) + await waitFor(() => { + expect(getMenu().className).toContain("hidden") + }) }) it("resets confirm state when popover closes", async () => { - const { getByTestId } = render() + const { getByTestId, container } = render() + const getParentDiv = () => + container.querySelector("[class*='flex items-center justify-between']") as HTMLElement + + // Hover to make menu visible + fireEvent.mouseEnter(getParentDiv()) // Open the popover await waitForOpenHandler() @@ -106,10 +111,12 @@ describe("CheckpointSaved popover visibility", () => { }) it("closes popover after preview and after confirm restore", async () => { - const { getByTestId } = render() + const { getByTestId, container } = render() const popoverRoot = () => getByTestId("restore-popover") const menuContainer = () => getByTestId("checkpoint-menu-container") + const getParentDiv = () => + container.querySelector("[class*='flex items-center justify-between']") as HTMLElement // Open await waitForOpenHandler() @@ -125,11 +132,16 @@ describe("CheckpointSaved popover visibility", () => { expect(popoverRoot().getAttribute("data-open")).toBe("false") expect(menuContainer().className).toContain("block") }) - // await waitFor(() => { - // expect(menuContainer().className).toContain("hidden") - // }) - // Reopen + // Simulate mouse leaving the component to trigger hide + fireEvent.mouseLeave(getParentDiv()) + + await waitFor(() => { + expect(menuContainer().className).toContain("hidden") + }) + + // Hover to make menu visible again, then reopen + fireEvent.mouseEnter(getParentDiv()) lastOnOpenChange?.(true) await waitFor(() => { expect(popoverRoot().getAttribute("data-open")).toBe("true") @@ -141,8 +153,36 @@ describe("CheckpointSaved popover visibility", () => { await waitFor(() => { expect(popoverRoot().getAttribute("data-open")).toBe("false") }) - // await waitFor(() => { - // expect(menuContainer().className).toContain("hidden") - // }) + + // Simulate mouse leaving the component to trigger hide + fireEvent.mouseLeave(getParentDiv()) + + await waitFor(() => { + expect(menuContainer().className).toContain("hidden") + }) + }) + + it("shows menu on hover and hides when mouse leaves", async () => { + const { getByTestId, container } = render() + + const getMenu = () => getByTestId("checkpoint-menu-container") as HTMLElement + const getParentDiv = () => + container.querySelector("[class*='flex items-center justify-between']") as HTMLElement + + // Initially hidden (not hovering) + expect(getMenu().className).toContain("hidden") + + // Hover over the component + fireEvent.mouseEnter(getParentDiv()) + await waitFor(() => { + expect(getMenu().className).toContain("block") + expect(getMenu().className).not.toContain("hidden") + }) + + // Mouse leaves the component + fireEvent.mouseLeave(getParentDiv()) + await waitFor(() => { + expect(getMenu().className).toContain("hidden") + }) }) }) diff --git a/webview-ui/src/components/mcp/McpView.tsx b/webview-ui/src/components/mcp/McpView.tsx index b8deb9a7b4a..1ce9679816b 100644 --- a/webview-ui/src/components/mcp/McpView.tsx +++ b/webview-ui/src/components/mcp/McpView.tsx @@ -1,4 +1,4 @@ -import React, { useState } from "react" +import React, { useState, useEffect } from "react" import { Trans } from "react-i18next" import { VSCodeCheckbox, @@ -206,6 +206,14 @@ const ServerRow = ({ server, alwaysAllowMcp }: { server: McpServer; alwaysAllowM return configTimeout ?? 60 // Default 1 minute (60 seconds) }) + // Update timeoutValue when server.config changes + useEffect(() => { + const configTimeout = JSON.parse(server.config)?.timeout + if (configTimeout !== undefined) { + setTimeoutValue(configTimeout) + } + }, [server.config]) + // Computed property to check if server is expandable const isExpandable = server.status === "connected" && !server.disabled diff --git a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts index 40800d93423..8e169b1c5da 100644 --- a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts +++ b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts @@ -5,7 +5,7 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query" import { renderHook } from "@testing-library/react" import type { Mock } from "vitest" -import { ProviderSettings, ModelInfo, BEDROCK_1M_CONTEXT_MODEL_IDS } from "@roo-code/types" +import { ProviderSettings, ModelInfo, BEDROCK_1M_CONTEXT_MODEL_IDS, litellmDefaultModelInfo } from "@roo-code/types" import { useSelectedModel } from "../useSelectedModel" import { useRouterModels } from "../useRouterModels" @@ -539,4 +539,119 @@ describe("useSelectedModel", () => { expect(result.current.info?.contextWindow).toBe(200_000) }) }) + + describe("litellm provider", () => { + beforeEach(() => { + mockUseOpenRouterModelProviders.mockReturnValue({ + data: {}, + isLoading: false, + isError: false, + } as any) + }) + + it("should use litellmDefaultModelInfo as fallback when routerModels.litellm is empty", () => { + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: {}, + requesty: {}, + unbound: {}, + litellm: {}, + "io-intelligence": {}, + }, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "litellm", + litellmModelId: "some-model", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.provider).toBe("litellm") + // Should fall back to default model ID since "some-model" doesn't exist in empty litellm models + expect(result.current.id).toBe("claude-3-7-sonnet-20250219") + // Should use litellmDefaultModelInfo as fallback + expect(result.current.info).toEqual(litellmDefaultModelInfo) + expect(result.current.info?.supportsNativeTools).toBe(true) + }) + + it("should use litellmDefaultModelInfo when selected model not found in routerModels", () => { + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: {}, + requesty: {}, + unbound: {}, + litellm: { + "existing-model": { + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + supportsNativeTools: true, + }, + }, + "io-intelligence": {}, + }, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "litellm", + litellmModelId: "non-existing-model", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.provider).toBe("litellm") + // Falls back to default model ID + expect(result.current.id).toBe("claude-3-7-sonnet-20250219") + // Should use litellmDefaultModelInfo as fallback since default model also not in router models + expect(result.current.info).toEqual(litellmDefaultModelInfo) + expect(result.current.info?.supportsNativeTools).toBe(true) + }) + + it("should use model info from routerModels when model exists", () => { + const customModelInfo: ModelInfo = { + maxTokens: 16384, + contextWindow: 128000, + supportsImages: true, + supportsPromptCache: true, + supportsNativeTools: true, + description: "Custom LiteLLM model", + } + + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: {}, + requesty: {}, + unbound: {}, + litellm: { + "custom-model": customModelInfo, + }, + "io-intelligence": {}, + }, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "litellm", + litellmModelId: "custom-model", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.provider).toBe("litellm") + expect(result.current.id).toBe("custom-model") + // Should use the model info from routerModels, not the fallback + expect(result.current.info).toEqual(customModelInfo) + expect(result.current.info?.supportsNativeTools).toBe(true) + }) + }) }) diff --git a/webview-ui/src/components/ui/hooks/useSelectedModel.ts b/webview-ui/src/components/ui/hooks/useSelectedModel.ts index c12a4530f56..b245600a0aa 100644 --- a/webview-ui/src/components/ui/hooks/useSelectedModel.ts +++ b/webview-ui/src/components/ui/hooks/useSelectedModel.ts @@ -31,6 +31,7 @@ import { ioIntelligenceModels, basetenModels, qwenCodeModels, + litellmDefaultModelInfo, BEDROCK_1M_CONTEXT_MODEL_IDS, isDynamicProvider, getProviderDefaultModelId, @@ -196,7 +197,7 @@ function getSelectedModel({ } case "litellm": { const id = getValidatedModelId(apiConfiguration.litellmModelId, routerModels.litellm, defaultModelId) - const info = routerModels.litellm?.[id] + const info = routerModels.litellm?.[id] ?? litellmDefaultModelInfo return { id, info } } case "xai": {