Implement capability discovery and querying for Virtual MCP Server

[JAORMX]

Summary

Implements capability discovery and querying for Virtual MCP Server based on the THV-2106 proposal.

This PR discovers backend MCP servers, queries their capabilities, and builds routing tables with full backend information ready for request routing.

Key Changes

1. Backend Registry (pkg/vmcp/registry.go)

• Shared kernel component for thread-safe backend access
• BackendRegistry interface with Get(), List(), Count()
• immutableRegistry implementation (read-only, Phase 1)
• BackendToTarget() helper for converting backends to routing targets
• Avoids circular dependencies, reusable across aggregator/router/health monitoring

2. Backend Client (pkg/vmcp/client/client.go)

• MCP protocol communication using mark3labs/mcp-go SDK
• Supports streamable-HTTP and SSE transports
• Methods: ListCapabilities(), CallTool(), ReadResource(), GetPrompt()
• 629 lines of type conversion tests validating SDK integration

3. CLI Backend Discoverer (pkg/vmcp/aggregator/cli_discoverer.go)

• Discovers Docker/Podman workloads from ToolHive groups
• Uses existing groups.Manager and workloads.Manager
• Filters for healthy/running backends only

4. Default Aggregator (pkg/vmcp/aggregator/default_aggregator.go)

• Parallel backend queries with errgroup (10 concurrent max)
• Graceful failure handling (continues with remaining backends)
• Five-stage pipeline: query → resolve conflicts → merge
• Routing table now has complete BackendTarget info (URL, transport, auth, health)

Why mark3labs/mcp-go?

• Proven in production (pkg/transport/bridge.go)
• Better client flexibility for per-backend authentication
• Fits our architecture patterns
• Abstracted behind interfaces for future migration

Architectural Decisions

BackendRegistry as Shared Kernel

Placed in pkg/vmcp root (like types.go, errors.go) because:

• Reusable across aggregator, router, and health monitoring
• Avoids circular dependencies
• Single source of truth for backend information

No Connection Pooling Yet

Why: Prevents confused deputy security vulnerability without proper authentication.

Without backend auth:
User A request → Could use pooled connection with User B's credentials ❌

Phase 1 approach:
Create/close client per request → Secure, simple, correct ✅

Connection pooling will be added after backend authentication is implemented with proper user-scoped pools.

Thread Safety

• Phase 1: Immutable structures (safe concurrent reads)
• Go maps are safe for concurrent reads without writes
• Future: Add mutexRegistry when health monitoring requires updates

Test Coverage

• All unit tests pass (go test ./pkg/vmcp/...)
• Type conversion tests (629 lines)
• Parallel-safe tests with t.Parallel()
• Tests verify full BackendTarget population
• Generated mocks for interfaces

Acceptance Criteria

• ✅ Fails initialization if backends unreachable
• ✅ Logs backend outages with warnings
• ✅ Handles transient failures with retries (SDK level)
• ✅ Graceful partial failure handling

Next Steps

• Conflict resolution strategies (prefix, priority, manual)
• Request routing using populated routing table
• Backend authentication (enables connection pooling)

🤖 Generated with Claude Code

[JAORMX]

@claude please review this.

[claude]

Claude finished @JAORMX's task —— View job

---

Code Review Complete ✓

I've reviewed the PR implementing capability discovery and querying for Virtual MCP Server. Overall, this is well-architected, thoughtfully designed code that follows ToolHive patterns effectively. The code quality is high with excellent test coverage and clear separation of concerns.

---

Major Strengths

1. Excellent Architecture & Design Patterns ✓

• Clean Domain-Driven Design with proper bounded contexts
• Interface-based design enables testability and future extensibility
• Platform abstraction (CLI vs K8s) follows ToolHive patterns perfectly
• Five-method aggregation pipeline is clear and composable

2. Robust Concurrency & Error Handling ✓

• Parallel backend queries with errgroup and configurable concurrency limit (10)
• Graceful failure handling - continues with remaining backends on partial failure
• Thread-safe map access with mutex protection
• Proper context propagation throughout

3. Comprehensive Test Coverage ✓

• 8 test scenarios covering all edge cases
• All tests parallel-safe with t.Parallel()
• Smart non-determinism handling using DoAndReturn()
• Interface-based testing with generated mocks

4. Good Code Organization ✓

• Public methods at top, private at bottom (follows CLAUDE.md)
• Clear package documentation
• Proper error wrapping with context
• Separation of domain types from infrastructure

---

Issues & Recommendations

Critical Issues: None ✓

Medium Priority Issues

1. Resource Cleanup in client.go ⚠️

Location: pkg/vmcp/client/client.go

Issue: Multiple MCP client lifecycle management concerns:

a) Lines 101-109, 201-214, 261-269, 299-312: Creating new clients for every operation

// Current pattern repeated 4 times
c, err := h.clientFactory(ctx, target)
if err != nil {
    return nil, fmt.Errorf("...")
}
defer c.Close()

Impact:

• Performance: Connection overhead for each operation (handshake, initialization)
• Resource leaks: If context cancellation occurs during operations, deferred Close() may not execute properly
• Backend load: Unnecessary repeated connections

Recommendation: Implement connection pooling or reuse:

// Option 1: Add connection pool to httpBackendClient
type httpBackendClient struct {
    clientFactory func(...)
    clientPool    sync.Map // map[string]*pooledClient with TTL
}

// Option 2: Let callers manage lifecycle for multiple operations
type ClientSession interface {
    ListCapabilities(...) (...)
    CallTool(...) (...)
    Close() error
}

b) Line 70: Missing cleanup on initialization failure

if err := initializeClient(ctx, c); err != nil {
    _ = c.Close()  // Ignoring error - consider logging
    return nil, fmt.Errorf(...)
}

Recommendation: Log the close error:

if err := c.Close(); err != nil {
    logger.Warnf("Failed to close client after init failure: %v", err)
}

---

2. Incomplete Backend Target Construction ⚠️

Location: pkg/vmcp/aggregator/default_aggregator.go:199-203

Issue: Routing table entries are incomplete:

routingTable.Tools[resolvedTool.ResolvedName] = &vmcp.BackendTarget{
    WorkloadID: resolvedTool.BackendID,  // Only ID, missing URL/transport/auth
}

Impact: Router cannot actually forward requests without BaseURL, TransportType, and AuthStrategy.

Current State: Comment acknowledges this (// For now, we'll create a minimal target...)

Recommendation for Future PR:

• Pass backend map through the pipeline: ResolveConflicts(backends map[string]Backend, ...)
• Populate full BackendTarget from the backend map in MergeCapabilities
• Add validation that routing table entries are complete before returning

Suggested Improvement:

// Store backends during aggregation
type defaultAggregator struct {
    backendClient vmcp.BackendClient
    backendCache  map[string]vmcp.Backend  // Add this
}

// In MergeCapabilities, look up full backend info:
backend := a.backendCache[resolvedTool.BackendID]
routingTable.Tools[resolvedTool.ResolvedName] = &vmcp.BackendTarget{
    WorkloadID:    backend.ID,
    WorkloadName:  backend.Name,
    BaseURL:       backend.BaseURL,
    TransportType: backend.TransportType,
    AuthStrategy:  backend.AuthStrategy,
    AuthMetadata:  backend.AuthMetadata,
    HealthStatus:  backend.HealthStatus,
    Metadata:      backend.Metadata,
}

---

3. Missing Unit Tests for cli_discoverer.go ⚠️

Location: pkg/vmcp/aggregator/cli_discoverer.go

Issue: No unit tests for the CLI backend discoverer (only default_aggregator_test.go exists).

Impact: Critical discovery logic is untested:

• Group existence validation
• Workload filtering by status
• URL availability checks
• Metadata population

Recommendation: Add cli_discoverer_test.go:

func TestCLIBackendDiscoverer_Discover(t *testing.T) {
    t.Run("discovers healthy backends", func(t *testing.T) { ... })
    t.Run("filters out stopped workloads", func(t *testing.T) { ... })
    t.Run("filters out workloads without URLs", func(t *testing.T) { ... })
    t.Run("handles group not found", func(t *testing.T) { ... })
    t.Run("handles empty groups", func(t *testing.T) { ... })
    t.Run("copies labels to metadata", func(t *testing.T) { ... })
}

---

Low Priority / Minor Issues

4. Type Conversion Duplication 💡

Location: pkg/vmcp/client/client.go:140-158

Issue: InputSchema conversion logic is verbose and repeated pattern.

Recommendation: Extract to helper function:

func convertToolInputSchema(schema mcp.ToolInputSchema) map[string]any {
    result := map[string]any{"type": schema.Type}
    if schema.Properties != nil {
        result["properties"] = schema.Properties
    }
    if len(schema.Required) > 0 {
        result["required"] = schema.Required
    }
    if schema.Defs != nil {
        result["$defs"] = schema.Defs
    }
    return result
}

---

5. Logging Improvements 💡

Location: pkg/vmcp/aggregator/cli_discoverer.go:69

Issue: Debug log doesn't show status value:

logger.Debugf("Skipping workload %s with status %s", name, workload.Status)

Recommendation: Include actual status to aid debugging.

---

6. Error Message Consistency 💡

Location: pkg/vmcp/aggregator/aggregator.go:188-198

Issue: Error variable naming isn't perfectly consistent with ToolHive patterns.

Recommendation: Consider prefixing with package name for clarity:

var (
    ErrAggregatorNoBackends         = fmt.Errorf("no backends found in group")
    ErrAggregatorBackendQueryFailed = fmt.Errorf("failed to query backend capabilities")
    // ...
)

---

7. TODO Comments 💡

Locations:

• pkg/vmcp/client/client.go:46: Authentication header injection
• pkg/vmcp/client/client.go:191: Query server capabilities for logging/sampling
• pkg/vmcp/client/client.go:253: Handle other content types (audio, resource)
• pkg/vmcp/client/client.go:342: Handle other content types

Recommendation: Track these as issues so they don't get lost. Consider creating follow-up issues for:

• Outgoing authentication implementation
• Full MCP content type support
• Server capability introspection

---

Architecture & Design Analysis

Positive Patterns ✓

1. DDD Bounded Contexts: Clear separation between aggregator, client, and router concerns
2. Interface Segregation: Small, focused interfaces (BackendDiscoverer, Aggregator, BackendClient)
3. Factory Pattern: Platform-specific discoverers (CLI vs K8s)
4. Pipeline Pattern: Clear aggregation stages (Query → Resolve → Merge)
5. Graceful Degradation: Continues with partial backend failures

SDK Choice Justification ✓

The decision to use mark3labs/mcp-go is well-justified in the PR description:

• Already proven in production (pkg/transport/bridge.go)
• Better client flexibility for per-backend auth
• Direct http.Handler implementation
• Both SDKs support middleware, but mark3labs fits auth patterns better

Confirmed by reviewing pkg/transport/bridge.go - the SDK is already in use and working well.

---

Test Quality Analysis

Coverage: Excellent ✓

• 8 test functions covering 5 aggregator methods
• Success paths, error paths, edge cases (empty, conflicts, partial failures)
• All tests use mocks properly with gomock

Test Patterns: Strong ✓

// Smart handling of non-determinism
mockClient.EXPECT().ListCapabilities(...).
    DoAndReturn(func(_ context.Context, target *vmcp.BackendTarget) (...) {
        if target.WorkloadID == "backend1" {
            return caps1, nil
        }
        return nil, errors.New("connection timeout")
    }).Times(2)

Improvements Needed:

• Add tests for cli_discoverer.go (critical gap)
• Consider integration tests for full pipeline with real backend responses

---

Code Quality Metrics

• Complexity: Appropriate - clear logic flow, no overly complex functions
• Maintainability: High - well-documented, follows patterns, testable
• Performance: Good - parallel queries, bounded concurrency, graceful failure
• Security: Adequate for Phase 1 - authentication is TODO for next phase
• Documentation: Excellent - package docs, godocs, inline comments

---

Alignment with CLAUDE.md Guidelines

✅ Follows ToolHive Patterns

• Uses workloads.Manager and groups.Manager correctly
• Interface-based design like pkg/container/runtime
• Error wrapping consistent with codebase

✅ Code Organization

• Public methods at top, private at bottom
• Packages focused on single responsibilities
• Proper use of pkg/ structure

✅ Testing

• Ginkgo/Gomega not used (using testify instead) - acceptable for this package
• Mocks generated with mockgen ✓
• Tests are parallel-safe ✓

✅ Commit Quality

• Single commit with clear message
• Co-authored-by trailer present
• Descriptive PR body

---

Recommendations Summary

Before Merge:

1. ✅ Add unit tests for cli_discoverer.go (critical)
2. ⚠️ Document the incomplete routing table in MergeCapabilities (add TODO comment referencing tracking issue)

In Next PR (Phase 2):

1. Implement connection pooling/reuse in httpBackendClient
2. Complete BackendTarget population in routing table
3. Implement outgoing authentication (WithHTTPHeaderFunc)
4. Add conflict resolution strategies (prefix, priority, manual)
5. Add server capability introspection for logging/sampling

Nice to Have:

• Extract convertToolInputSchema helper
• Create tracking issues for all TODOs
• Add integration tests

---

Verdict

✅ Approve with minor suggestions

This is high-quality, production-ready code for Phase 1. The architecture is sound, test coverage is strong (except for one gap), and it follows ToolHive patterns well. The identified issues are either:

• Expected TODOs for future phases (auth, conflict resolution)
• Performance optimizations that can wait (connection pooling)
• Missing tests that should be added

The only blocker I'd recommend is adding tests for cli_discoverer.go since discovery is a critical path. Everything else can be addressed in follow-up PRs.

Great work on this foundation! 🎉

---

[codecov]

Codecov Report

❌ Patch coverage is 44.59161% with 251 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.93%. Comparing base (720b794) to head (0af705b).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/vmcp/client/client.go	12.85%	182 Missing and 1 partial ⚠️
pkg/vmcp/registry.go	0.00%	30 Missing ⚠️
pkg/vmcp/aggregator/default_aggregator.go	82.55%	16 Missing and 10 partials ⚠️
pkg/vmcp/aggregator/cli_discoverer.go	86.66%	7 Missing and 1 partial ⚠️
pkg/vmcp/aggregator/k8s_discoverer.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2354      +/-   ##
==========================================
- Coverage   54.26%   53.93%   -0.34%     
==========================================
  Files         242      249       +7     
  Lines       23446    23966     +520     
==========================================
+ Hits        12724    12927     +203     
- Misses       9514     9812     +298     
- Partials     1208     1227      +19     

☔ 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.

[Copilot]

Pull Request Overview

This PR implements the Virtual MCP Server's backend client and capability aggregation infrastructure, enabling ToolHive to discover and aggregate capabilities from multiple MCP backend servers.

• Adds HTTP/SSE transport support for communicating with backend MCP servers
• Implements capability discovery and aggregation logic with conflict detection
• Provides both CLI (Docker/Podman) and Kubernetes discoverer interfaces (K8s pending implementation)

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
pkg/vmcp/types.go	Adds mockgen directive for generating BackendClient and HealthChecker mocks
pkg/vmcp/mocks/mock_backend_client.go	Generated mock implementations for testing backend client interactions
pkg/vmcp/errors.go	Adds ErrUnsupportedTransport error for invalid transport types
pkg/vmcp/client/client.go	Implements HTTP backend client with streamable-HTTP and SSE transport support
pkg/vmcp/client/client_test.go	Unit tests for client error handling and transport validation
pkg/vmcp/client/conversions_test.go	Tests for MCP SDK type conversions to vmcp domain types
pkg/vmcp/aggregator/aggregator.go	Defines aggregator interfaces with mockgen directive
pkg/vmcp/aggregator/default_aggregator.go	Implements parallel capability querying and conflict resolution
pkg/vmcp/aggregator/default_aggregator_test.go	Tests for aggregation pipeline and error handling
pkg/vmcp/aggregator/cli_discoverer.go	Implements CLI-based backend discovery from Docker/Podman workloads
pkg/vmcp/aggregator/cli_discoverer_test.go	Tests for CLI discoverer with various filtering scenarios
pkg/vmcp/aggregator/k8s_discoverer.go	Stub implementation for Kubernetes backend discovery (not yet implemented)
pkg/vmcp/aggregator/discoverer.go	Package documentation for discoverer implementations
pkg/vmcp/aggregator/mocks/mock_interfaces.go	Generated mocks for aggregator interfaces

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[blkt]

First few comments, I don't have time to further review this at the moment, but I'll try to review it further.

[yrobla]

added some comments