Skip to content

Commit f58142b

Browse files
Address review comments on tool conflict resolution
- Add nil check in tool_adapter.go to prevent panic when tool not found in originalToolsByName map - Add comment explaining O(n²) complexity is acceptable for small tool lists - Improve warning message when priority strategy drops tools from backends not in priority list - Show which backends were involved when dropping conflicting tools Co-authored-by: Juan Antonio Osorio <JAORMX@users.noreply.github.com>
1 parent 32725c6 commit f58142b

File tree

3 files changed

+22
-7
lines changed

3 files changed

+22
-7
lines changed

pkg/mcp/tool_filter.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,10 @@ func processToolsListResponse(
508508
processedTools := applyFilteringAndOverrides(config, simpleTools)
509509

510510
// Build the filtered response by matching processed tools with their original maps
511+
// Note: This is O(n²) complexity, but acceptable because:
512+
// - Tool lists are typically small (< 100 tools per backend)
513+
// - Only runs once during tool list retrieval (not in hot path)
514+
// - Inner loop breaks early on match
511515
filteredTools := make([]map[string]any, 0, len(processedTools))
512516
for _, processed := range processedTools {
513517
// Find the original tool map by matching names

pkg/vmcp/aggregator/conflict_resolver.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,13 @@ func (r *PriorityConflictResolver) ResolveToolConflicts(
163163
// Conflict detected - choose the highest priority backend
164164
winner := r.selectWinner(candidates)
165165
if winner == nil {
166-
// All candidates are from backends not in priority list
167-
logger.Warnf("Tool %s exists in multiple backends but none are in priority order, skipping", toolName)
166+
// All candidates are from backends not in priority list - drop all of them
167+
// This is intentional: priority strategy requires explicit priority ordering
168+
backendIDs := make([]string, len(candidates))
169+
for i, c := range candidates {
170+
backendIDs[i] = c.BackendID
171+
}
172+
logger.Warnf("Tool %s exists in multiple backends %v but none are in priority order, dropping all instances", toolName, backendIDs)
168173
droppedTools += len(candidates)
169174
continue
170175
}

pkg/vmcp/aggregator/tool_adapter.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,24 +74,30 @@ func processBackendTools(
7474
}
7575

7676
// Convert back to vmcp.Tool, preserving InputSchema and BackendID
77-
result := make([]vmcp.Tool, len(processed))
78-
for i, simpleTool := range processed {
77+
result := make([]vmcp.Tool, 0, len(processed))
78+
for _, simpleTool := range processed {
7979
// Find the original tool name (before any override)
8080
originalName := simpleTool.Name
8181
if revName, wasOverridden := reverseOverrideMap[simpleTool.Name]; wasOverridden {
8282
originalName = revName
8383
}
8484

8585
// Look up the original tool to preserve InputSchema and BackendID
86-
originalTool := originalToolsByName[originalName]
86+
originalTool, exists := originalToolsByName[originalName]
87+
if !exists {
88+
// This should not happen unless there's a bug in the filtering logic,
89+
// but skip the tool rather than panicking
90+
logger.Warnf("Tool %s not found in original tools map for backend %s, skipping", originalName, backendID)
91+
continue
92+
}
8793

8894
// Construct the result tool with processed name/description but original schema
89-
result[i] = vmcp.Tool{
95+
result = append(result, vmcp.Tool{
9096
Name: simpleTool.Name, // Use the processed (potentially overridden) name
9197
Description: simpleTool.Description, // Use the processed (potentially overridden) description
9298
InputSchema: originalTool.InputSchema,
9399
BackendID: backendID, // Use the backendID parameter (source of truth)
94-
}
100+
})
95101
}
96102

97103
return result

0 commit comments

Comments
 (0)