scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-09-11 22:51:26 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: replace unbounded registry with LRU cache for MCP tool names

Browse source 
- Remove mcpToolNameRegistry (unbounded Map) and clearMcpToolNameRegistry
- Add LRU cache with size limit of 100 entries for encoded name computation
- Add findToolByEncodedMcpName() to compare encoded names at lookup time
- Add hasHashSuffix() helper for checking if a name needs lookup
- Pass original encoded MCP name through the tool call stack
- Update UseMcpToolTool.validateToolExists() to use encoded comparison
- Update tests to cover new functions and LRU cache behavior

This addresses the reviewer feedback to avoid memory leaks from the
unbounded registry by comparing encoded tool names on both sides and
using an LRU cache for performance.
This commit is contained in:
Roo Code 2026-01-16 08:53:12 +00:00
parent 865e9a361e
commit 7be724fdd1
5 changed files with 248 additions and 111 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
src/core/assistant-message/presentAssistantMessage.ts
View file

@ -276,6 +276,7 @@ export async function presentAssistantMessage(cline: Task) {
// Execute the MCP tool using the same handler as use_mcp_tool
// Create a synthetic ToolUse block that the useMcpToolTool can handle
// Include the original encoded MCP name for lookup via encoded name comparison
const syntheticToolUse: ToolUse<"use_mcp_tool"> = {
type: "tool_use",
id: mcpBlock.id,
@ -290,6 +291,7 @@ export async function presentAssistantMessage(cline: Task) {
server_name: resolvedServerName,
tool_name: mcpBlock.toolName,
arguments: mcpBlock.arguments,
_encodedMcpName: mcpBlock.name, // Original encoded name for lookup via comparison
},
}

45
src/core/tools/UseMcpToolTool.ts
View file

@ -4,6 +4,7 @@ import { Task } from "../task/Task"
import { formatResponse } from "../prompts/responses"
import { t } from "../../i18n"
import type { ToolUse } from "../../shared/tools"
import { findToolByEncodedMcpName, hasHashSuffix } from "../../utils/mcp-name"
import { BaseTool, ToolCallbacks } from "./BaseTool"
@ -35,7 +36,11 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
}
}
async execute(params: UseMcpToolParams, task: Task, callbacks: ToolCallbacks): Promise<void> {
async execute(
params: UseMcpToolParams & { _encodedMcpName?: string },
task: Task,
callbacks: ToolCallbacks,
): Promise<void> {
const { askApproval, handleError, pushToolResult, toolProtocol } = callbacks
try {
@ -48,11 +53,22 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
const { serverName, toolName, parsedArguments } = validation
// Validate that the tool exists on the server
const toolValidation = await this.validateToolExists(task, serverName, toolName, pushToolResult)
// Pass the original encoded MCP name for lookup via comparison when direct lookup fails
const encodedMcpName = params._encodedMcpName
const toolValidation = await this.validateToolExists(
task,
serverName,
toolName,
pushToolResult,
encodedMcpName,
)
if (!toolValidation.isValid) {
return
}
// Use the resolved tool name (may differ from parsed name for shortened names)
const resolvedToolName = toolValidation.resolvedToolName || toolName
// Reset mistake count on successful validation
task.consecutiveMistakeCount = 0
@ -60,7 +76,7 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
const completeMessage = JSON.stringify({
type: "use_mcp_tool",
serverName,
toolName,
toolName: resolvedToolName,
arguments: params.arguments ? JSON.stringify(params.arguments) : undefined,
} satisfies ClineAskUseMcpServer)
@ -75,7 +91,7 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
await this.executeToolAndProcessResult(
task,
serverName,
toolName,
resolvedToolName,
parsedArguments,
executionId,
pushToolResult,
@ -156,7 +172,8 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
serverName: string,
toolName: string,
pushToolResult: (content: string) => void,
): Promise<{ isValid: boolean; availableTools?: string[] }> {
encodedMcpName?: string,
): Promise<{ isValid: boolean; availableTools?: string[]; resolvedToolName?: string }> {
try {
// Get the MCP hub to access server information
const provider = task.providerRef.deref()
@ -205,8 +222,20 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
return { isValid: false, availableTools: [] }
}
// Check if the requested tool exists
const tool = server.tools.find((tool) => tool.name === toolName)
// Check if the requested tool exists by direct name match
let tool = server.tools.find((tool) => tool.name === toolName)
let resolvedToolName = toolName
// If direct lookup fails and we have an encoded MCP name with hash suffix,
// try to find the tool by comparing encoded names
if (!tool && encodedMcpName && hasHashSuffix(encodedMcpName)) {
const availableToolNames = server.tools.map((t) => t.name)
const matchedToolName = findToolByEncodedMcpName(serverName, encodedMcpName, availableToolNames)
if (matchedToolName) {
tool = server.tools.find((t) => t.name === matchedToolName)
resolvedToolName = matchedToolName
}
}
if (!tool) {
// Tool not found - provide list of available tools
@ -252,7 +281,7 @@ export class UseMcpToolTool extends BaseTool<"use_mcp_tool"> {
}
// Tool exists and is enabled
return { isValid: true, availableTools: server.tools.map((tool) => tool.name) }
return { isValid: true, availableTools: server.tools.map((tool) => tool.name), resolvedToolName }
} catch (error) {
// If there's an error during validation, log it but don't block the tool execution
// The actual tool call might still fail with a proper error

7
src/shared/tools.ts
View file

@ -108,7 +108,12 @@ export type NativeToolArgs = {
search_files: { path: string; regex: string; file_pattern?: string | null }
switch_mode: { mode_slug: string; reason: string }
update_todo_list: { todos: string }
use_mcp_tool: { server_name: string; tool_name: string; arguments?: Record<string, unknown> }
use_mcp_tool: {
server_name: string
tool_name: string
arguments?: Record<string, unknown>
_encodedMcpName?: string
}
write_to_file: { path: string; content: string }
// Add more tools as they are migrated to native protocol
}

188
src/utils/__tests__/mcp-name.spec.ts
View file

@ -10,15 +10,16 @@ import {
HYPHEN_ENCODING,
MAX_TOOL_NAME_LENGTH,
HASH_SUFFIX_LENGTH,
mcpToolNameRegistry,
clearMcpToolNameRegistry,
clearEncodedNameCache,
computeHashSuffix,
findToolByEncodedMcpName,
hasHashSuffix,
} from "../mcp-name"
describe("mcp-name utilities", () => {
// Clear the registry before each test to ensure isolation
// Clear the cache before each test to ensure isolation
beforeEach(() => {
clearMcpToolNameRegistry()
clearEncodedNameCache()
})
describe("constants", () => {
@ -175,27 +176,27 @@ describe("mcp-name utilities", () => {
expect(result).toMatch(/_[a-f0-9]{8}$/)
})
it("should use hash suffix for long names and register them", () => {
it("should use hash suffix for long names and cache them", () => {
const longServer = "a".repeat(80)
const longTool = "b".repeat(80)
const result = buildMcpToolName(longServer, longTool)
// The shortened name should be registered
expect(mcpToolNameRegistry.has(result)).toBe(true)
const registered = mcpToolNameRegistry.get(result)
expect(registered).toEqual({
serverName: longServer,
toolName: longTool,
})
// The shortened name should be deterministic
expect(result.length).toBe(128)
expect(result).toMatch(/_[a-f0-9]{8}$/)
// Building again should return the same result (from cache)
const result2 = buildMcpToolName(longServer, longTool)
expect(result2).toBe(result)
})
it("should produce deterministic hash suffixes", () => {
const longServer = "a".repeat(80)
const longTool = "b".repeat(80)
// Build the same name twice
clearMcpToolNameRegistry()
// Build the same name twice with cache cleared between
clearEncodedNameCache()
const result1 = buildMcpToolName(longServer, longTool)
clearMcpToolNameRegistry()
clearEncodedNameCache()
const result2 = buildMcpToolName(longServer, longTool)
// Should produce identical results
expect(result1).toBe(result2)
@ -427,46 +428,6 @@ describe("mcp-name utilities", () => {
})
describe("hash suffix roundtrip - fixes issue #10766", () => {
it("should preserve original names through roundtrip with long hyphenated tool names", () => {
// This is the exact scenario from issue #10766
// Tool name with many hyphens that exceeds 128 chars when encoded
const serverName = "abcdefghij-kl-mnop-qrs-tuv-with-extra-long-suffix-to-exceed-limit"
const toolName = "wxyz-abcd-efghijk-lmno-plus-additional-long-suffix-here"
// Build the tool name
const builtName = buildMcpToolName(serverName, toolName)
// Should be truncated to 128 chars with hash suffix
expect(builtName.length).toBe(128)
expect(builtName).toMatch(/_[a-f0-9]{8}$/)
// The critical fix: parsing should return the ORIGINAL names
const parsed = parseMcpToolName(builtName)
expect(parsed).toEqual({
serverName: serverName, // Original with hyphens!
toolName: toolName, // Original with hyphens!
})
})
it("should not corrupt hyphen encoding when truncation is needed", () => {
// Long server and tool names that would cause truncation mid-encoding
const serverName = "very-long-server-name-with-many-hyphens-and-extra-content-to-exceed"
const toolName = "another-long-tool-name-with-hyphens-and-extra-content-to-exceed"
// Build the tool name
const builtName = buildMcpToolName(serverName, toolName)
// Should be truncated to 128 chars
expect(builtName.length).toBe(128)
// Parse should return original names (via registry lookup)
const parsed = parseMcpToolName(builtName)
expect(parsed).toEqual({
serverName: serverName,
toolName: toolName,
})
})
it("should work correctly when names do not need truncation", () => {
// Short names that don't need truncation
const serverName = "server"
@ -486,38 +447,113 @@ describe("mcp-name utilities", () => {
})
})
it("should handle the registry lookup for shortened names", () => {
const serverName = "a".repeat(80)
const toolName = "b".repeat(80) + "-hyphen"
it("should find tool by encoded name comparison for shortened names", () => {
// This is the new approach: instead of registry, compare encoded names
const serverName = "abcdefghij-kl-mnop-qrs-tuv-with-extra-long-suffix-to-exceed-limit"
const toolName = "wxyz-abcd-efghijk-lmno-plus-additional-long-suffix-here"
// Build registers the shortened name
const builtName = buildMcpToolName(serverName, toolName)
// Build the encoded tool name
const encodedName = buildMcpToolName(serverName, toolName)
// Verify it's in the registry
expect(mcpToolNameRegistry.has(builtName)).toBe(true)
// Should be truncated to 128 chars with hash suffix
expect(encodedName.length).toBe(128)
expect(encodedName).toMatch(/_[a-f0-9]{8}$/)
// Parse uses the registry to get original names
const parsed = parseMcpToolName(builtName)
expect(parsed).toEqual({
serverName: serverName,
toolName: toolName,
})
// The new approach: use findToolByEncodedMcpName to find the matching tool
const availableTools = [toolName, "other-tool", "another-tool"]
const foundTool = findToolByEncodedMcpName(serverName, encodedName, availableTools)
expect(foundTool).toBe(toolName)
})
it("should not find tool when none matches the encoded name", () => {
const serverName = "server"
const encodedName = "mcp--server--nonexistent_tool_a1b2c3d4"
const availableTools = ["tool1", "tool2", "tool3"]
const foundTool = findToolByEncodedMcpName(serverName, encodedName, availableTools)
expect(foundTool).toBeNull()
})
})
describe("clearMcpToolNameRegistry", () => {
it("should clear all registered tool names", () => {
// Register some tool names via buildMcpToolName
describe("findToolByEncodedMcpName", () => {
it("should find tool by exact encoded name match", () => {
const serverName = "myserver"
const toolName = "get-data"
const encodedName = buildMcpToolName(serverName, toolName)
const availableTools = ["other-tool", "get-data", "another-tool"]
const foundTool = findToolByEncodedMcpName(serverName, encodedName, availableTools)
expect(foundTool).toBe("get-data")
})
it("should find tool for shortened names with hash suffix", () => {
const serverName = "a".repeat(80)
const toolName = "b".repeat(80) + "-hyphen"
const encodedName = buildMcpToolName(serverName, toolName)
// The encoded name should have a hash suffix
expect(hasHashSuffix(encodedName)).toBe(true)
const availableTools = ["other-tool", toolName, "another-tool"]
const foundTool = findToolByEncodedMcpName(serverName, encodedName, availableTools)
expect(foundTool).toBe(toolName)
})
it("should return null when no tool matches", () => {
const serverName = "server"
const encodedName = buildMcpToolName(serverName, "nonexistent")
const availableTools = ["tool1", "tool2"]
const foundTool = findToolByEncodedMcpName(serverName, encodedName, availableTools)
expect(foundTool).toBeNull()
})
it("should handle empty available tools list", () => {
const serverName = "server"
const encodedName = buildMcpToolName(serverName, "tool")
const foundTool = findToolByEncodedMcpName(serverName, encodedName, [])
expect(foundTool).toBeNull()
})
})
describe("hasHashSuffix", () => {
it("should return true for names with hash suffix", () => {
expect(hasHashSuffix("mcp--server--tool_a1b2c3d4")).toBe(true)
expect(hasHashSuffix("mcp--server--tool_12345678")).toBe(true)
expect(hasHashSuffix("mcp--server--tool_abcdef00")).toBe(true)
})
it("should return false for names without hash suffix", () => {
expect(hasHashSuffix("mcp--server--tool")).toBe(false)
expect(hasHashSuffix("mcp--server--tool_name")).toBe(false)
expect(hasHashSuffix("mcp--server--tool___name")).toBe(false)
})
it("should return false for names with partial hash patterns", () => {
// Not enough hex chars
expect(hasHashSuffix("mcp--server--tool_abc")).toBe(false)
// No underscore before hash
expect(hasHashSuffix("mcp--server--toola1b2c3d4")).toBe(false)
})
})
describe("clearEncodedNameCache", () => {
it("should clear the encoded name cache", () => {
// Build some names to populate cache
const longServer = "a".repeat(80)
const longTool = "b".repeat(80)
buildMcpToolName(longServer, longTool)
buildMcpToolName("server", "tool")
expect(mcpToolNameRegistry.size).toBeGreaterThan(0)
// Clear the cache
clearEncodedNameCache()
// Clear the registry
clearMcpToolNameRegistry()
expect(mcpToolNameRegistry.size).toBe(0)
// Verify cache is cleared by checking that rebuilding takes the same path
// (we can't directly access the cache, but the function should work)
const result = buildMcpToolName(longServer, longTool)
expect(result.length).toBe(128)
expect(result).toMatch(/_[a-f0-9]{8}$/)
})
})
})

117
src/utils/mcp-name.ts
View file

@ -46,21 +46,48 @@ export const MAX_TOOL_NAME_LENGTH = 128
export const HASH_SUFFIX_LENGTH = 8
/**
* Registry mapping shortened tool names (with hash suffix) to their original
* server and tool names. This is used to look up the original names when
* the model returns a shortened tool name.
*
* Key: shortened MCP tool name (e.g., "mcp--server--tool_a1b2c3d4")
* Value: { serverName, toolName } with original (decoded) names
* LRU cache for encoded tool names.
* Maps "serverName:toolName" to the encoded MCP tool name.
* This avoids recomputing hash suffixes for frequently used tools.
*/
export const mcpToolNameRegistry = new Map<string, { serverName: string; toolName: string }>()
const ENCODED_NAME_CACHE_SIZE = 100
const encodedNameCache = new Map<string, string>()
/**
* Clear the MCP tool name registry.
* Should be called when MCP servers are refreshed or disconnected.
* Get an encoded name from the LRU cache, updating recency.
*/
export function clearMcpToolNameRegistry(): void {
mcpToolNameRegistry.clear()
function getCachedEncodedName(key: string): string | undefined {
const value = encodedNameCache.get(key)
if (value !== undefined) {
// Move to end (most recently used) by deleting and re-adding
encodedNameCache.delete(key)
encodedNameCache.set(key, value)
}
return value
}
/**
* Set an encoded name in the LRU cache, evicting oldest if needed.
*/
function setCachedEncodedName(key: string, value: string): void {
if (encodedNameCache.has(key)) {
encodedNameCache.delete(key)
} else if (encodedNameCache.size >= ENCODED_NAME_CACHE_SIZE) {
// Evict the oldest entry (first in iteration order)
const oldestKey = encodedNameCache.keys().next().value
if (oldestKey) {
encodedNameCache.delete(oldestKey)
}
}
encodedNameCache.set(key, value)
}
/**
* Clear the encoded name cache.
* Exported for testing purposes.
*/
export function clearEncodedNameCache(): void {
encodedNameCache.clear()
}
/**
@ -166,22 +193,30 @@ export function sanitizeMcpName(name: string): string {
* The format is: mcp--{sanitized_server_name}--{sanitized_tool_name}
*
* The total length is capped at 128 characters per MCP spec.
* When truncation is needed, a hash suffix is appended to preserve uniqueness
* and the mapping is stored in the registry for later lookup.
* When truncation is needed, a hash suffix is appended to preserve uniqueness.
* The result is cached for efficient repeated lookups.
*
* @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 {
// Check cache first
const cacheKey = `${serverName}:${toolName}`
const cached = getCachedEncodedName(cacheKey)
if (cached !== undefined) {
return cached
}
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}`
// If within limit, return as-is
// If within limit, cache and return
if (fullName.length <= MAX_TOOL_NAME_LENGTH) {
setCachedEncodedName(cacheKey, fullName)
return fullName
}
@ -195,9 +230,8 @@ export function buildMcpToolName(serverName: string, toolName: string): string {
const truncatedBase = fullName.slice(0, maxTruncatedLength)
const shortenedName = `${truncatedBase}${suffixWithSeparator}`
// Register the mapping from shortened name to original names
// Store the original (decoded) names so parseMcpToolName can return them directly
mcpToolNameRegistry.set(shortenedName, { serverName, toolName })
// Cache the result
setCachedEncodedName(cacheKey, shortenedName)
return shortenedName
}
@ -218,8 +252,9 @@ export function decodeMcpName(sanitizedName: string): string {
* This handles sanitized names by splitting on the "--" separator
* and decoding triple underscores back to hyphens.
*
* For shortened names (those with hash suffixes), this first checks
* the registry for the original names.
* Note: For shortened names (those with hash suffixes), this function
* returns the parsed names which may be truncated. Use findToolByEncodedMcpName()
* to find the correct original tool name by comparing encoded names.
*
* @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
@ -230,13 +265,6 @@ export function parseMcpToolName(mcpToolName: string): { serverName: string; too
return null
}
// First, check if this is a shortened name in the registry
// This handles names that were truncated with hash suffixes
const registeredName = mcpToolNameRegistry.get(mcpToolName)
if (registeredName) {
return registeredName
}
// Remove the "mcp--" prefix
const remainder = mcpToolName.slice(prefix.length)
@ -259,3 +287,40 @@ export function parseMcpToolName(mcpToolName: string): { serverName: string; too
toolName: decodeMcpName(toolName),
}
}
/**
* Find a tool by comparing encoded MCP names.
* This is used when the model returns a shortened name with a hash suffix.
* Instead of using a registry, we compare by encoding each available tool name
* and checking if it matches the encoded name from the model.
*
* @param serverName - The original server name (decoded)
* @param encodedMcpName - The full encoded MCP tool name returned by the model
* @param availableToolNames - List of available tool names on the server
* @returns The matching original tool name, or null if not found
*/
export function findToolByEncodedMcpName(
serverName: string,
encodedMcpName: string,
availableToolNames: string[],
): string | null {
for (const toolName of availableToolNames) {
const encoded = buildMcpToolName(serverName, toolName)
if (encoded === encodedMcpName) {
return toolName
}
}
return null
}
/**
* Check if an MCP tool name appears to be a shortened name with a hash suffix.
* Shortened names have the pattern: ...._XXXXXXXX where X is a hex character.
*
* @param mcpToolName - The MCP tool name to check
* @returns true if the name appears to have a hash suffix
*/
export function hasHashSuffix(mcpToolName: string): boolean {
// Check for pattern: ends with underscore + 8 hex characters
return /_.{8}$/.test(mcpToolName) && /[a-f0-9]{8}$/.test(mcpToolName)
}