Skip to content

Commit 7463182

Browse files
JAORMXclaudegithub-actions[bot]
authored
Implement tool conflict resolution for Virtual MCP Server (#2365)
* Implement tool conflict resolution for Virtual MCP Server Add three conflict resolution strategies (prefix, priority, manual) to handle duplicate tool names across multiple backend MCP servers in vMCP aggregation. This implements the aggregation conflict resolution portion of the Virtual MCP Server proposal (THV-2106), enabling vMCP to merge capabilities from multiple backends while resolving naming conflicts. Key features: - Prefix strategy: automatically prefixes tools with workload identifier (supports {workload}_, {workload}., custom formats) - Priority strategy: explicit ordering with first-wins semantics (drops lower-priority conflicting tools with warnings) - Manual strategy: requires explicit overrides with startup validation (fails if any conflicts lack overrides, safest for production) - Reuses existing mcp.WithToolsFilter/Override middleware logic - Per-backend tool filtering and overrides applied before conflict resolution - Tracks conflict metadata (count resolved, strategy used) Implementation: - Extracted shared filtering/override logic from pkg/mcp/tool_filter.go - Created applyFilteringAndOverrides() as single source of truth - Both HTTP middleware and aggregator use the same battle-tested code - Updated defaultAggregator to integrate conflict resolver - Comprehensive table-driven unit tests for all strategies This follows DDD principles with clear bounded contexts and maintains backward compatibility with existing middleware behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * 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 <[email protected]> * Add clarifying comments for conflict resolution logic Address review feedback by adding detailed comments explaining: 1. What "resolved conflict" means in the context of conflict resolution: - Prefix strategy: Renaming tools proactively prevents collisions - Manual strategy: Explicit overrides resolve existing conflicts - Priority strategy: Drops duplicates rather than renaming 2. Manual resolver validation: Documents that collision detection after override prevents new conflicts from being introduced by bad config These clarifications help reviewers understand that renaming IS conflict resolution, not just a side effect. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * Remove ConflictsResolved counter from aggregation metadata Remove the ConflictsResolved counter as it was not defined in the proposal and caused confusion about what constitutes a "resolved conflict" vs a preventive renaming. The ConflictStrategy field remains to indicate which resolution approach was used (prefix, priority, manual), which is sufficient for observability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * Refactor conflict resolvers into separate files Split conflict_resolver.go into focused files for better code organization: - prefix_resolver.go: PrefixConflictResolver implementation - priority_resolver.go: PriorityConflictResolver implementation - manual_resolver.go: ManualConflictResolver implementation - conflict_resolver.go: Factory function and shared helpers Each strategy now has its own file, making the code easier to navigate and maintain. Shared helpers (groupToolsByName, toolWithBackend) remain in conflict_resolver.go for reuse. No functional changes - pure refactoring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * Use prefix strategy as fallback in priority resolver Change priority resolver to use prefix strategy for backends not in the priority list instead of dropping their conflicting tools. This prevents data loss while maintaining explicit control for prioritized backends. Behavior: - Backends in priority list: priority strategy (first wins) - Backends NOT in priority list with conflicts: prefix strategy fallback - Example: priority_order=["github"], but slack+teams both have "send_message" Result: "slack_send_message" and "teams_send_message" (both included) This addresses review feedback about dropping tools unnecessarily and provides a more practical default behavior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: Claude <[email protected]> Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Juan Antonio Osorio <[email protected]>
1 parent 7644d08 commit 7463182

11 files changed

+1400
-71
lines changed

pkg/mcp/tool_filter.go

Lines changed: 106 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,36 @@ func (c *toolMiddlewareConfig) getToolListOverride(toolName string) (*toolOverri
7272
// middleware.
7373
type ToolMiddlewareOption func(*toolMiddlewareConfig) error
7474

75+
// SimpleTool represents a minimal tool with name and description.
76+
// This is used by ApplyToolFiltering to work with tools in a generic way.
77+
type SimpleTool struct {
78+
Name string
79+
Description string
80+
}
81+
82+
// ApplyToolFiltering applies filtering and overriding to a list of tools.
83+
// This is the core logic used by both the HTTP middleware and other components
84+
// that need to apply the same filtering/overriding behavior.
85+
//
86+
// Returns the filtered and overridden tools.
87+
func ApplyToolFiltering(opts []ToolMiddlewareOption, tools []SimpleTool) ([]SimpleTool, error) {
88+
config := &toolMiddlewareConfig{
89+
filterTools: make(map[string]struct{}),
90+
actualToUserOverride: make(map[string]toolOverrideEntry),
91+
userToActualOverride: make(map[string]toolOverrideEntry),
92+
}
93+
94+
// Apply options to build config
95+
for _, opt := range opts {
96+
if err := opt(config); err != nil {
97+
return nil, err
98+
}
99+
}
100+
101+
// Use the shared core logic
102+
return applyFilteringAndOverrides(config, tools), nil
103+
}
104+
75105
// WithToolsFilter is a function that can be used to configure the tool
76106
// middleware to use a filter list of tools.
77107
func WithToolsFilter(toolsFilter ...string) ToolMiddlewareOption {
@@ -448,7 +478,10 @@ func processToolsListResponse(
448478
toolsListResponse toolsListResponse,
449479
w io.Writer,
450480
) error {
451-
filteredTools := []map[string]any{}
481+
// Convert to SimpleTool format for shared processing
482+
simpleTools := make([]SimpleTool, 0, len(*toolsListResponse.Result.Tools))
483+
toolMaps := make([]map[string]any, 0, len(*toolsListResponse.Result.Tools))
484+
452485
for _, tool := range *toolsListResponse.Result.Tools {
453486
// NOTE: the spec does not allow for name to be missing.
454487
toolName, ok := tool["name"].(string)
@@ -461,31 +494,90 @@ func processToolsListResponse(
461494
return errToolNameNotFound
462495
}
463496

497+
// Get description if present (optional in MCP spec)
498+
description, _ := tool["description"].(string)
499+
500+
simpleTools = append(simpleTools, SimpleTool{
501+
Name: toolName,
502+
Description: description,
503+
})
504+
toolMaps = append(toolMaps, tool)
505+
}
506+
507+
// Apply the shared filtering/override logic
508+
processedTools := applyFilteringAndOverrides(config, simpleTools)
509+
510+
// 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
515+
filteredTools := make([]map[string]any, 0, len(processedTools))
516+
for _, processed := range processedTools {
517+
// Find the original tool map by matching names
518+
for i, simple := range simpleTools {
519+
if simple.Name == processed.Name || simple.Name == findOriginalName(config, processed.Name) {
520+
// Clone the original map and update name/description
521+
toolCopy := make(map[string]any, len(toolMaps[i]))
522+
for k, v := range toolMaps[i] {
523+
toolCopy[k] = v
524+
}
525+
toolCopy["name"] = processed.Name
526+
if processed.Description != "" {
527+
toolCopy["description"] = processed.Description
528+
}
529+
filteredTools = append(filteredTools, toolCopy)
530+
break
531+
}
532+
}
533+
}
534+
535+
toolsListResponse.Result.Tools = &filteredTools
536+
if err := json.NewEncoder(w).Encode(toolsListResponse); err != nil {
537+
return fmt.Errorf("%w: %v", errBug, err)
538+
}
539+
540+
return nil
541+
}
542+
543+
// applyFilteringAndOverrides is the core logic for filtering and overriding tools.
544+
// This implements the exact same logic as before but is now extracted for reuse.
545+
func applyFilteringAndOverrides(config *toolMiddlewareConfig, tools []SimpleTool) []SimpleTool {
546+
result := make([]SimpleTool, 0, len(tools))
547+
for _, tool := range tools {
548+
description := tool.Description
549+
464550
// If the tool is overridden, we need to use the override name and description.
465-
if entry, ok := config.getToolListOverride(toolName); ok {
551+
if entry, ok := config.getToolListOverride(tool.Name); ok {
466552
if entry.OverrideName != "" {
467-
tool["name"] = entry.OverrideName
553+
tool.Name = entry.OverrideName
468554
}
469555
if entry.OverrideDescription != "" {
470-
tool["description"] = entry.OverrideDescription
556+
description = entry.OverrideDescription
471557
}
472-
toolName = entry.OverrideName
473558
}
474559

475560
// If the tool is in the filter, we add it to the filtered tools list.
476-
// Note that lookup is done using the user-known name, which might be
477-
// different from the actual tool name.
478-
if config.isToolInFilter(toolName) {
479-
filteredTools = append(filteredTools, tool)
561+
// Note that lookup is done using the user-known name (tool.Name after override).
562+
if config.isToolInFilter(tool.Name) {
563+
result = append(result, SimpleTool{
564+
Name: tool.Name,
565+
Description: description,
566+
})
480567
}
481568
}
569+
return result
570+
}
482571

483-
toolsListResponse.Result.Tools = &filteredTools
484-
if err := json.NewEncoder(w).Encode(toolsListResponse); err != nil {
485-
return fmt.Errorf("%w: %v", errBug, err)
572+
// findOriginalName attempts to find the original tool name before override.
573+
func findOriginalName(config *toolMiddlewareConfig, overriddenName string) string {
574+
// Iterate through overrides to find reverse mapping
575+
for actualName, entry := range config.actualToUserOverride {
576+
if entry.OverrideName == overriddenName {
577+
return actualName
578+
}
486579
}
487-
488-
return nil
580+
return overriddenName
489581
}
490582

491583
// toolCallFix mimics a sum type in Go. The actual types represent the

pkg/vmcp/aggregator/aggregator.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -160,9 +160,6 @@ type AggregationMetadata struct {
160160
// PromptCount is the total number of prompts.
161161
PromptCount int
162162

163-
// ConflictsResolved is the number of conflicts that were resolved.
164-
ConflictsResolved int
165-
166163
// ConflictStrategy is the strategy used for conflict resolution.
167164
ConflictStrategy vmcp.ConflictResolutionStrategy
168165
}
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Package aggregator provides capability aggregation for Virtual MCP Server.
2+
//
3+
// This file contains the factory function for creating conflict resolvers
4+
// and shared helper functions used by multiple resolver implementations.
5+
package aggregator
6+
7+
import (
8+
"fmt"
9+
10+
"github.com/stacklok/toolhive/pkg/logger"
11+
"github.com/stacklok/toolhive/pkg/vmcp"
12+
"github.com/stacklok/toolhive/pkg/vmcp/config"
13+
)
14+
15+
// NewConflictResolver creates the appropriate conflict resolver based on configuration.
16+
func NewConflictResolver(aggregationConfig *config.AggregationConfig) (ConflictResolver, error) {
17+
if aggregationConfig == nil {
18+
// Default to prefix strategy with default format
19+
logger.Infof("No aggregation config provided, using default prefix strategy")
20+
return NewPrefixConflictResolver("{workload}_"), nil
21+
}
22+
23+
switch aggregationConfig.ConflictResolution {
24+
case vmcp.ConflictStrategyPrefix:
25+
prefixFormat := "{workload}_" // Default
26+
if aggregationConfig.ConflictResolutionConfig != nil &&
27+
aggregationConfig.ConflictResolutionConfig.PrefixFormat != "" {
28+
prefixFormat = aggregationConfig.ConflictResolutionConfig.PrefixFormat
29+
}
30+
logger.Infof("Using prefix conflict resolution strategy (format: %s)", prefixFormat)
31+
return NewPrefixConflictResolver(prefixFormat), nil
32+
33+
case vmcp.ConflictStrategyPriority:
34+
if aggregationConfig.ConflictResolutionConfig == nil ||
35+
len(aggregationConfig.ConflictResolutionConfig.PriorityOrder) == 0 {
36+
return nil, fmt.Errorf("priority strategy requires priority_order in conflict_resolution_config")
37+
}
38+
logger.Infof("Using priority conflict resolution strategy (order: %v)",
39+
aggregationConfig.ConflictResolutionConfig.PriorityOrder)
40+
return NewPriorityConflictResolver(aggregationConfig.ConflictResolutionConfig.PriorityOrder)
41+
42+
case vmcp.ConflictStrategyManual:
43+
logger.Infof("Using manual conflict resolution strategy")
44+
return NewManualConflictResolver(aggregationConfig.Tools)
45+
46+
default:
47+
return nil, fmt.Errorf("%w: %s", ErrInvalidConflictStrategy, aggregationConfig.ConflictResolution)
48+
}
49+
}
50+
51+
// toolWithBackend is a helper struct to track which backend a tool comes from.
52+
// This is shared by multiple conflict resolution strategies.
53+
type toolWithBackend struct {
54+
Tool vmcp.Tool
55+
BackendID string
56+
}
57+
58+
// groupToolsByName groups tools by their names to detect conflicts.
59+
// This is shared by multiple conflict resolution strategies.
60+
func groupToolsByName(toolsByBackend map[string][]vmcp.Tool) map[string][]toolWithBackend {
61+
toolsByName := make(map[string][]toolWithBackend)
62+
for backendID, tools := range toolsByBackend {
63+
for _, tool := range tools {
64+
toolsByName[tool.Name] = append(toolsByName[tool.Name], toolWithBackend{
65+
Tool: tool,
66+
BackendID: backendID,
67+
})
68+
}
69+
}
70+
return toolsByName
71+
}

0 commit comments

Comments
 (0)