Implement tool conflict resolution for Virtual MCP Server

[JAORMX]

Summary

Implements tool conflict resolution strategies for the Virtual MCP Server aggregation system. This enables vMCP to merge capabilities from multiple backend MCP servers while resolving duplicate tool names.

Three conflict resolution strategies:

• Prefix Strategy: Automatically prefixes all tools with workload identifier (e.g., {workload}_, {workload}., custom formats). Deterministic and requires no configuration beyond format.

• Priority Strategy: Uses explicit priority ordering where first backend wins conflicts. Lower-priority conflicting tools are dropped with warnings. Non-conflicting tools from all backends are included.

• Manual Strategy: Requires explicit overrides for ALL conflicts. Fails at startup with detailed error messages if any conflicts lack overrides. Safest option for production deployments.

Code reuse from existing middleware:

Extracted shared filtering/override logic from pkg/mcp/tool_filter.go into applyFilteringAndOverrides() function. Both the HTTP middleware (battle-tested) and the new aggregator use this exact same code, ensuring consistency and reducing duplication.

Key changes:

• pkg/vmcp/aggregator/conflict_resolver.go - Three strategy implementations
• pkg/vmcp/aggregator/tool_adapter.go - Minimal adapter to reuse middleware logic
• pkg/vmcp/aggregator/default_aggregator.go - Integrated conflict resolver
• pkg/mcp/tool_filter.go - Extracted shared logic, added ApplyToolFiltering() API
• Comprehensive table-driven unit tests for all strategies

Per-backend tool filtering and overrides:

Applied during capability discovery BEFORE conflict resolution using the existing mcp.WithToolsFilter() and mcp.WithToolsOverride() patterns. This reduces the tool set entering conflict resolution.

Test Plan

• All unit tests pass (go test ./pkg/vmcp/aggregator ./pkg/mcp)
• Linting clean (task lint-fix)
• Prefix strategy with different formats ({workload}_, {workload}.)
• Priority strategy with 2-way and 3-way conflicts
• Manual strategy validation catches unresolved conflicts
• Per-backend filtering and overrides preserve InputSchema and BackendID
• Existing middleware behavior unchanged (all tests pass)
• Edge cases: empty priority order, description-only overrides, no conflicts
• Integration test with real backend MCP servers (follow-up)

Related

Part of Virtual MCP Server implementation (THV-2106). Builds on capability discovery work from PR #2354.

🤖 Generated with Claude Code

[JAORMX]

@claude please review this in context of docs/proposals/THV-2106-virtual-mcp-server.md . look for code quality, security and best practices.

[claude]

Claude finished @JAORMX's task —— View job

---

Code Review Complete

I've reviewed this PR against the THV-2106 proposal and ToolHive best practices. Overall, this is excellent work with well-structured code, comprehensive tests, and smart reuse of existing middleware logic. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Code Reuse Pattern

The extraction of applyFilteringAndOverrides() from the HTTP middleware into a shared function is exactly right:

• pkg/mcp/tool_filter.go:539-566 - Clean separation of core logic
• pkg/vmcp/aggregator/tool_adapter.go - Minimal adapter layer
• Both HTTP middleware and aggregator use identical logic
• Reduces duplication and ensures consistency

2. Well-Designed Conflict Resolution Strategies

All three strategies are implemented correctly per the proposal:

• Prefix: Clean string substitution with configurable formats
• Priority: Proper ordering with O(1) lookups via priority map
• Manual: Fail-fast validation with detailed error messages

3. Comprehensive Test Coverage

• Table-driven tests for all three strategies
• Edge cases covered (empty priority, backends not in list, etc.)
• Tests verify both filtering and overrides work correctly
• Schema and BackendID preservation verified

4. Good Logging and Observability

• Appropriate log levels (Debug for operations, Warn for conflicts, Info for summaries)
• Conflict counts tracked and reported
• Backend query failures logged but don't fail entire operation

---

🔒 Security Review

✅ No Security Issues Found

• No credential handling (delegated to token exchange)
• No SQL injection risks (no database queries)
• No command injection (no shell execution)
• Input validation via MCP spec compliance
• Conflict resolution is deterministic and safe

Minor Recommendation: DoS Prevention

The parallel backend queries use errgroup.SetLimit(10) to prevent overwhelming backends. Consider making this configurable:

g.SetLimit(10) // pkg/vmcp/aggregator/default_aggregator.go:96

Suggestion: Add maxConcurrentQueries to operational config (not blocking, future enhancement).

---

📋 Code Quality Issues

1. Inconsistent Error Wrapping (Low Priority)

Location: pkg/vmcp/aggregator/default_aggregator.go:64

return nil, fmt.Errorf("%w: %s: %v", ErrBackendQueryFailed, backend.ID, err)

Issue: Mixing sentinel error with formatted context makes errors harder to match with errors.Is().

Recommendation: Choose one pattern consistently:

// Option A: Pure sentinel error
return nil, fmt.Errorf("backend %s: %w", backend.ID, ErrBackendQueryFailed)

// Option B: Wrap with context (current approach is fine)
return nil, fmt.Errorf("%w: %s: %v", ErrBackendQueryFailed, backend.ID, err)

2. Potential Memory Allocation Optimization (Very Low Priority)

Location: pkg/vmcp/aggregator/tool_adapter.go:59

simpleTools := make([]mcp.SimpleTool, len(tools))
originalToolsByName := make(map[string]vmcp.Tool, len(tools))

Observation: originalToolsByName might be larger than needed if tools are filtered.

Suggestion: Only allocate the map after filtering if many tools are typically filtered out. Current approach is fine for small tool lists.

3. Missing Edge Case Test (Medium Priority)

Location: Test coverage for processBackendTools()

Missing: Test case where override references a tool that doesn't exist in the backend's tool list. Current code would fail silently.

Recommendation: Add test case:

{
    name: "override references non-existent tool",
    backendID: "github",
    tools: []vmcp.Tool{
        {Name: "real_tool", Description: "Real"},
    },
    workloadConfig: &config.WorkloadToolConfig{
        Workload: "github",
        Overrides: map[string]*config.ToolOverride{
            "fake_tool": {Name: "renamed_fake"},
        },
    },
    wantCount: 1,
    wantNames: []string{"real_tool"},
},

---

🎯 Alignment with Best Practices

✅ Follows ToolHive Patterns

• Public methods at top, private at bottom (pkg/vmcp/aggregator/conflict_resolver.go)
• Clean interface segregation (ConflictResolver interface)
• Proper use of contexts throughout
• Factory pattern for resolver creation

✅ Go Best Practices

• Proper error wrapping with fmt.Errorf and %w
• No naked returns
• Proper use of sync.Mutex for concurrent access
• Clean separation of concerns
• Good use of table-driven tests

⚠️ Minor Deviation: Gocyclo Complexity

Location: pkg/mcp/tool_filter.go:405

//nolint:gocyclo
func processEventStream(...)

Note: This is acceptable since it's inheriting complexity from existing middleware. No action needed.

---

📚 Alignment with Proposal

Reviewed against docs/proposals/THV-2106-virtual-mcp-server.md:

✅ Fully Aligned

• Section "Tool Conflict Resolution" (lines 756-833): All three strategies implemented exactly as specified
• Prefix formats: {workload}, {workload}_, {workload}. all supported
• Priority behavior: First backend wins, lower priority tools dropped with warnings
• Manual validation: Fails at startup with detailed error listing unresolved conflicts
• Per-backend filtering/overrides: Applied before conflict resolution as specified

✅ Additional Implementation Detail

The proposal doesn't explicitly specify the order of operations, but the implementation correctly:

1. Applies per-backend filtering/overrides FIRST (processBackendTools)
2. Then applies conflict resolution (ResolveToolConflicts)

This is the right order and aligns with the proposal's intent.

---

🔍 Detailed Code Review Notes

conflict_resolver.go

• Line 53-58: Good collision detection even with prefixing (rare but handled)
• Line 105-108: Excellent validation of priority order (no empty strings)
• Line 111: O(1) priority lookup map - smart optimization
• Line 267-269: Clean validation separation in manual resolver
• Line 374-384: Very clear error messages for unresolved conflicts

tool_adapter.go

• Line 42-56: Smart reverse map for preserving InputSchema after overrides
• Line 79-84: Excellent handling of name lookups (checks reverse map)
• Line 93: Correctly uses backendID parameter as source of truth (not tool.BackendID)

tool_filter.go

• Line 82-103: New ApplyToolFiltering() function is clean and reusable
• Line 539-566: Extracted logic is identical to original (verified)
• Line 568-577: Helper function for reverse name lookup

default_aggregator.go

• Line 68: Per-backend filtering applied at right time (before conflict resolution)
• Line 96: Concurrency limit prevents DoS (good)
• Line 108-110: Graceful failure handling (logs but continues)
• Line 159-178: Sensible fallback when no resolver configured

---

🎯 Test Quality Assessment

Excellent Coverage

• Prefix strategy: Multiple formats, conflicts, no conflicts
• Priority strategy: 2-way, 3-way conflicts, empty priority order
• Manual strategy: All conflicts resolved, unresolved, description-only overrides
• Tool adapter: Filtering, overrides, schema preservation

Test Quality Metrics

• ✅ Table-driven (idiomatic Go)
• ✅ Parallel execution (t.Parallel())
• ✅ Clear test names
• ✅ Good coverage of happy/sad paths
• ✅ Edge cases included

---

📋 Recommendations Summary

Must Fix (None)

No blocking issues found.

Should Fix

1. Add test case for override referencing non-existent tool (Medium priority)

Nice to Have

1. Consider making maxConcurrentQueries configurable (Low priority, future)
2. Standardize error wrapping pattern (Low priority)
3. Consider optimizing map allocation in processBackendTools (Very low priority)

---

✅ Final Verdict

Approve with minor suggestions

This is high-quality code that:

• ✅ Implements the proposal specifications correctly
• ✅ Follows ToolHive and Go best practices
• ✅ Has no security vulnerabilities
• ✅ Includes comprehensive test coverage
• ✅ Reuses existing battle-tested middleware logic
• ✅ Has excellent error handling and logging

The suggested improvements are all optional and don't block merging. Great work! 🎉

---

Todo List

• Read THV-2106 proposal document to understand requirements
• Review conflict_resolver.go for implementation correctness
• Review tool_adapter.go for proper middleware integration
• Review tool_filter.go refactoring
• Review default_aggregator.go integration
• Review test coverage and quality
• Check for security issues
• Verify alignment with Go and ToolHive best practices
• Provide comprehensive feedback

---

[codecov]

Codecov Report

❌ Patch coverage is 87.81870% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.22%. Comparing base (216a280) to head (1bd6867).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/mcp/tool_filter.go	78.84%	10 Missing and 1 partial ⚠️
pkg/vmcp/aggregator/tool_adapter.go	80.95%	5 Missing and 3 partials ⚠️
pkg/vmcp/aggregator/default_aggregator.go	86.27%	5 Missing and 2 partials ⚠️
pkg/vmcp/aggregator/manual_resolver.go	90.14%	4 Missing and 3 partials ⚠️
pkg/vmcp/aggregator/prefix_resolver.go	78.57%	4 Missing and 2 partials ⚠️
pkg/vmcp/aggregator/conflict_resolver.go	93.93%	2 Missing ⚠️
pkg/vmcp/aggregator/priority_resolver.go	97.36%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2365      +/-   ##
==========================================
+ Coverage   53.79%   54.22%   +0.42%     
==========================================
  Files         254      259       +5     
  Lines       24509    24824     +315     
==========================================
+ Hits        13185    13461     +276     
- Misses      10039    10068      +29     
- Partials     1285     1295      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[yrobla]

added some comments

[JAORMX]

@claude be cool and address the comments

[claude]

Claude finished @JAORMX's task —— View job

---

Addressing Review Comments ✅

All review comments from @yrobla have been addressed!

• Read and understand all review comments
• Address comment 1: O(n²) complexity in tool_filter.go:515
• Address comment 2: Conflict resolution logic in conflict_resolver.go:63 (skipped - comment is clear enough)
• Address comment 3: Potential nil panic in tool_adapter.go:86
• Address comment 4: Dropping all backends in conflict_resolver.go:164
• Run tests (task test)
• Commit and push changes

Changes Made

1. Fixed potential nil panic (pkg/vmcp/aggregator/tool_adapter.go:86):

   • Added existence check before accessing originalToolsByName[originalName]
   • If tool not found, logs warning and skips instead of panicking
   • Changed from fixed-size slice to append pattern to handle skipped tools

2. Added complexity clarification (pkg/mcp/tool_filter.go:511):

   • Added comment explaining O(n²) loop is acceptable for small tool lists in non-hot path
   • Typical tool lists are < 100 tools per backend
   • Inner loop breaks early on match

3. Improved warning message (pkg/vmcp/aggregator/conflict_resolver.go:165):

   • Enhanced logging when priority strategy drops all conflicting tools from backends not in priority list
   • Now shows which backends were involved: Tool foo exists in multiple backends ["backend1", "backend2"] but none are in priority order, dropping all instances
   • Added clarifying comment that this behavior is intentional

4. Comment 2 - No change needed:

   • The counter at conflict_resolver.go:63 already has a clear comment
   • It tracks when prefix strategy changed a tool name (e.g., list_repos → github_list_repos)

Test Results

All tests passing:

ok  	github.com/stacklok/toolhive/pkg/vmcp/aggregator	0.066s
ok  	github.com/stacklok/toolhive/pkg/mcp	0.053s

Commit

Pushed commit f58142b to branch vmcp-conflict-resolution:

• View commit
• View diff

---