Skip to content

Conversation

JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Oct 7, 2025

Summary

This PR introduces a proposal for the Virtual MCP Server, a new ToolHive feature that aggregates multiple MCP servers from a group into a single unified interface.

What does this PR do?

Adds a technical proposal document that outlines the design and implementation approach for the Virtual MCP Server (thv virtual command).

Key Design Decisions

  • Group-based architecture: Leverages existing ToolHive groups as the fundamental abstraction for managing backend MCP servers
  • Reuses existing constructs: Uses ToolsFilter for including specific tools and ToolOverride for renaming/redescribing tools
  • Per-backend authentication: Supports different authentication strategies per backend (pass-through, token exchange, service accounts, header injection)
  • Phased implementation: MVP focuses on basic aggregation, with composite tools and advanced features in later phases

Benefits

  • Simplifies client integration (single connection instead of multiple)
  • Centralizes authentication complexity handling
  • Enables cross-service workflows via composite tools
  • Maintains full backward compatibility with existing MCP clients and servers

Testing

This is a proposal document - implementation and testing will follow in subsequent PRs.

Related Issues

This proposal addresses the need for consolidating multiple MCP servers as discussed in the Virtual MCP design document.

@JAORMX JAORMX force-pushed the proposal/virtual-mcp-server branch 2 times, most recently from a2a5279 to 9312f00 Compare October 7, 2025 13:04
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.34%. Comparing base (f6a5060) to head (ac1d31f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2106      +/-   ##
==========================================
- Coverage   53.35%   53.34%   -0.01%     
==========================================
  Files         231      231              
  Lines       29536    29536              
==========================================
- Hits        15759    15757       -2     
- Misses      12638    12640       +2     
  Partials     1139     1139              

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

@JAORMX JAORMX requested a review from Copilot October 7, 2025 13:25
@JAORMX
Copy link
Collaborator Author

JAORMX commented Oct 7, 2025

@claude please review this

Copy link
Contributor

claude bot commented Oct 7, 2025

Claude finished @JAORMX's task —— View job


Code Review: Virtual MCP Server Proposal

I've reviewed the Virtual MCP Server proposal. This is a well-thought-out design that appropriately leverages existing ToolHive constructs. Here's my detailed feedback:


Strengths

1. Excellent reuse of existing patterns

  • Leverages groups.Manager and workloads.Manager (docs/proposals/virtual-mcp-server.md:121-124)
  • Reuses ToolsFilter and ToolOverride middleware (docs/proposals/virtual-mcp-server.md:136-138, 218-219)
  • These are battle-tested components already in production

2. Clear phased implementation

  • MVP focuses on core aggregation (docs/proposals/virtual-mcp-server.md:186-196)
  • Composite tools deferred to Phase 2 is pragmatic
  • Allows incremental delivery and validation

3. Good alignment with MCP specification

  • Maintains protocol compatibility (docs/proposals/virtual-mcp-server.md:13)
  • Proper handling of tools, resources, and prompts (docs/proposals/virtual-mcp-server.md:128-132)

Issues & Recommendations

1. Authentication design needs clarification ⚠️

Issue: The proposal mentions token exchange (docs/proposals/virtual-mcp-server.md:66-68, 144) but there's already a separate token exchange proposal. The relationship between these two features isn't clear.

Questions:

Recommendation: Add a section explicitly stating:

# References token exchange from docs/proposals/token-exchange-middleware.md
backend_auth:
  github:
    type: "token_exchange"
    # Uses existing token exchange middleware implementation

2. Group reference validation ⚠️

Issue: The proposal doesn't specify what happens if the referenced group doesn't exist or is empty (docs/proposals/virtual-mcp-server.md:38).

Recommendation: Add validation behavior:

  • Return clear error if group doesn't exist
  • Handle case where group has no running workloads
  • Specify whether virtual MCP should start automatically when group workloads start

3. Tool name conflict resolution needs more detail ⚠️

Issue: The conflict_resolution field mentions "prefix | priority | manual" (docs/proposals/virtual-mcp-server.md:42) but doesn't explain:

  • What prefix format is used (workload name? custom prefix?)
  • How priority order is determined
  • What "manual" means (presumably the override section?)

Recommendation: Expand with examples:

aggregation:
  conflict_resolution: "prefix"  # Adds workload name as prefix: github_create_pr
  # OR
  conflict_resolution: "priority"  # First workload wins, others ignored
  priority_order: ["github", "jira", "slack"]

4. Missing error handling specifications ⚠️

Open questions from docs/proposals/virtual-mcp-server.md:240-244:

  1. ✓ Streaming responses - acknowledged as open question
  2. ✓ Backend capabilities caching - acknowledged as open question
  3. ✓ Rate limits - acknowledged as open question
  4. Missing: What happens when a backend workload fails/stops?
  5. Missing: Partial failure handling (some backends succeed, others fail)
  6. Missing: Circuit breaker patterns for unhealthy backends

Recommendation: Add section on failure modes and degradation strategies.

5. CLI command naming 💭

Minor consideration: The command thv virtual (docs/proposals/virtual-mcp-server.md:169) could be more descriptive. Consider:

  • thv virtual-server - clearer that it's running a server
  • thv aggregate - describes the function
  • thv virtual is fine if brevity is preferred

6. Composite tools implementation complexity ⚠️

Concern: The composite tools feature (docs/proposals/virtual-mcp-server.md:91-104) is Phase 2 but may be architecturally complex:

  • Requires DAG execution for depends_on
  • Needs variable substitution ({{.params.pr_number}})
  • Error handling across multiple tool calls
  • Transaction semantics (what if step 2 fails after step 1 succeeds?)

Recommendation: Consider whether composite tools belong in a separate proposal, as they're almost a workflow engine feature rather than an aggregation feature.

7. Security considerations 🔒

Missing: Security implications of aggregation:

  • Can workloads in the same group belong to different security contexts?
  • Should there be authorization checks per backend in addition to virtual MCP auth?
  • How are secrets scoped across aggregated backends?

Recommendation: Add explicit security boundary documentation.

8. Performance metrics and observability 📊

Success criteria (docs/proposals/virtual-mcp-server.md:246-252) includes "< 10ms routing overhead" but:

  • No mention of how this will be measured
  • No observability/telemetry strategy for debugging multi-backend issues
  • No mention of distributed tracing across backends

Recommendation: Add observability requirements to implementation notes.


Minor Issues

9. Typo/Grammar

Line 238: "LLM-generated backend" - this alternative seems like a different feature entirely and may confuse readers. Consider removing or clarifying.

10. Implementation notes location

The "Reusing Existing Components" section (docs/proposals/virtual-mcp-server.md:214-223) is excellent but buried at the end. Consider moving it closer to "High-Level Design" since it explains how the pieces fit together.


Questions for Discussion

  1. Workload lifecycle: Should the virtual MCP automatically start/stop underlying workloads, or only work with already-running workloads?

  2. Dynamic membership: If workloads are added/removed from the group while virtual MCP is running, should it automatically update? (Mentioned as Phase 3, line 199)

  3. Client registration: Should the virtual MCP be registered with MCP clients separately from its backend workloads?

  4. Backwards compatibility: Can existing workloads be added to groups without modification, or do they need reconfiguration?


Overall Assessment

Verdict: ✅ Approve with minor revisions

This is a solid proposal that appropriately reuses existing ToolHive infrastructure. The phased approach is sensible, and the core aggregation feature is well-scoped. The main gaps are around error handling, security boundaries, and clarifying the relationship with the token exchange proposal.

Priority fixes before implementation:

  1. Clarify authentication strategy and token exchange integration (Issue fix(typo): corrects readme #1)
  2. Specify failure handling and degradation behavior (Issue Implement secret store #4)
  3. Expand conflict resolution details (Issue Bump golangci/golangci-lint-action from 2f856675483cb8b9378ee77ee0beb67955aca9d7 to 4696ba8babb6127d732c3c6dde519db15edab9ea #3)

Nice-to-haves:


Job Run | Branch: proposal/virtual-mcp-server

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a technical proposal for a Virtual MCP Server feature that consolidates multiple MCP servers from a ToolHive group into a single unified interface. The proposal aims to simplify client integration and centralize authentication complexity while maintaining backward compatibility.

  • Adds comprehensive design documentation for the Virtual MCP Server (thv virtual command)
  • Defines group-based architecture leveraging existing ToolHive constructs
  • Outlines phased implementation approach from basic aggregation to enterprise features

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ChrisJBurns
Copy link
Collaborator

Should there be authorization checks per backend in addition to virtual MCP auth?

@JAORMX This part of Claudes review is what I was getting at in the vMCP GDoc. I'll explain more here just so it's visible to others.

If the benefit of vMCP itself is that it can aggregate 10 backing MCP servers, that's great, but let's think of the security implications / risks of this. In this case, a single vMCP server will have access to different 10 MCP servers. In the event that vMCP is ever exploited, would it be able to make calls to the backing MCP servers directly? Or will they themselves be protected by authn/z? If so, then this isn't as much of a concern from me.

@JAORMX
Copy link
Collaborator Author

JAORMX commented Oct 7, 2025

If the benefit of vMCP itself is that it can aggregate 10 backing MCP servers, that's great, but let's think of the security implications / risks of this. In this case, a single vMCP server will have access to different 10 MCP servers. In the event that vMCP is ever exploited, would it be able to make calls to the backing MCP servers directly? Or will they themselves be protected by authn/z? If so, then this isn't as much of a concern from me.

I see. So... we should clarify the architecture and deployment recommendations then. backed MCP servers should still not be deployed as they are without appropriate protections. That is, they should still enforce authentication and perhaps even authorization. If the vMCP is ever exploited, the backed MCP servers will still authenticate and authorize the request. I'm more keen on a zero-trust policy in general.

@ChrisJBurns
Copy link
Collaborator

That is, they should still enforce authentication and perhaps even authorization. If the vMCP is ever exploited, the backed MCP servers will still authenticate and authorize the request. I'm more keen on a zero-trust policy in general.

@JAORMX Yep, that would be my understanding (and hope) too! I had a feeling you thought the same way but I just wanted to triple check 😄

@JAORMX
Copy link
Collaborator Author

JAORMX commented Oct 7, 2025

@ChrisJBurns picture me with a tinfoil hat on at any given moment 😄

@jhrozek
Copy link
Contributor

jhrozek commented Oct 7, 2025

Should there be authorization checks per backend in addition to virtual MCP auth?

@JAORMX This part of Claudes review is what I was getting at in the vMCP GDoc. I'll explain more here just so it's visible to others.

If the benefit of vMCP itself is that it can aggregate 10 backing MCP servers, that's great, but let's think of the security implications / risks of this. In this case, a single vMCP server will have access to different 10 MCP servers. In the event that vMCP is ever exploited, would it be able to make calls to the backing MCP servers directly? Or will they themselves be protected by authn/z? If so, then this isn't as much of a concern from me.

Hmm, but the vMCP server would be an OAuth app so the user would connect with a token that says aud=vmcp right? And then the vMCP server would have to authenticate to each of the back end servers which means sending a token with aud=server1, aud=server2 if we are to reuse the usual authnz for the back end MCP servers. We could do that provided that the token exchange is set up to allow that. However, we'd have to send the external API token along with the request -or- let the back end token servers exchange the tokens they receive for external API tokens.

That would work if we can use token exchange all the way through, but not if we need to drive a consent and do the full flow (we would want to drive the consent chain to the consent on the vMCP server correct)? So in that case we'd have to send the back end server through a back channel?

@JAORMX JAORMX force-pushed the proposal/virtual-mcp-server branch 2 times, most recently from 13e4f9d to 6fc029f Compare October 14, 2025 12:15
JAORMX added a commit that referenced this pull request Oct 15, 2025
This proposal introduces MCPGroup support to the Kubernetes operator,
enabling the Virtual MCP Server feature and logical grouping of MCPServer
resources.

Key design decisions:
- Explicit group field on MCPServer spec (matches CLI behavior)
- Simple MCPGroup CRD with description field
- Status tracks server list and count
- Namespace-scoped groups only
- Enables field selector queries for group membership

Related: #2106
@JAORMX JAORMX force-pushed the proposal/virtual-mcp-server branch 2 times, most recently from a983db5 to f1f918c Compare October 20, 2025 09:29
@JAORMX JAORMX requested review from ChrisJBurns and jhrozek October 20, 2025 09:29
@JAORMX
Copy link
Collaborator Author

JAORMX commented Oct 20, 2025

@claude could you do another review?

Copy link
Contributor

claude bot commented Oct 20, 2025

Claude finished @JAORMX's task —— View job


Code Review: Virtual MCP Server Proposal

I've completed a thorough review of the Virtual MCP Server proposal. The document has been significantly enhanced since the last review, with excellent additions around elicitation support and composite tools. Here's my detailed feedback:


Summary of Changes Since Last Review

The most recent commit (f1f918c) added:

  • Elicitation support for composite tools (lines 246-260)
  • State persistence strategy for workflows (lines 334-336)
  • Incident investigation example demonstrating tool reuse (lines 140-183)
  • Enhanced composite tool example with user confirmations (lines 91-138)

These additions substantially improve the proposal's completeness and practicality.


Strengths

1. Excellent architectural reuse

  • Leverages existing groups.Manager and workloads.Manager (lines 201-204)
  • Reuses ToolsFilter and ToolOverride middleware (lines 214-218)
  • Builds on proven ToolHive patterns rather than inventing new ones

2. Clear phased implementation

  • MVP focuses on core aggregation without overcommitting (lines 283-288)
  • Advanced features properly deferred to Phase 2 (lines 290-294)
  • Allows for incremental delivery and validation

3. Elicitation integration is well-designed

  • Follows MCP specification for elicitation (line 247)
  • Proper handling of accept/decline/cancel actions (lines 253-260)
  • Good balance between flexibility and simplicity

4. Practical examples

  • Incident investigation example (lines 140-183) effectively demonstrates tool reuse
  • Deployment workflow (lines 91-138) shows realistic use case with confirmations

Critical Issues

1. Token Exchange Integration Needs Clarification ⚠️

Issue: The proposal mentions token_exchange authentication (lines 66-68, 224) but doesn't reference the existing token exchange proposal (THV-2063-token-exchange-middleware.md).

Current gap:

  • THV-2063 defines token exchange for single backend scenarios
  • This proposal needs token exchange for multiple backends simultaneously
  • No explanation of how to configure different audiences/client IDs per backend

Questions:

  1. Will each backend need its own OAuth client registration?
  2. How does the config map token exchange credentials per backend?
  3. Can backends share the same IDP but different audiences?
  4. What happens when backends use different IDPs entirely?

Recommendation: Add a section explicitly linking to THV-2063:

backend_auth:
  github:
    type: "token_exchange"
    # Reuses token exchange implementation from THV-2063-token-exchange-middleware.md
    token_exchange:
      token_url: "https://github.com/oauth/token"
      audience: "github-api"
      client_id: "github-client"
      client_secret_ref:
        name: "github-oauth"
        key: "client_secret"
      scopes: "repo read:org"

Also address:

  • Reference THV-2063 in Implementation Notes section (around line 323)
  • Add note about per-backend token caching strategy
  • Clarify if Virtual MCP needs its own client registration separate from backends

2. Security Model Insufficiently Addressed 🔒

Issue: The security discussion from PR comments (ChrisJBurns, jhrozek) about zero-trust and per-backend authz is not reflected in the proposal.

Missing security considerations:

  1. Zero-trust architecture: Proposal should state that backend MCP servers MUST still enforce their own authn/authz even when accessed via Virtual MCP
  2. Security boundary documentation: Where are trust boundaries? Can Virtual MCP be exploited to bypass backend security?
  3. Credential scope isolation: How are secrets scoped to prevent cross-contamination?
  4. Authorization policies: Should Virtual MCP enforce Cedar policies in addition to backend policies?

From PR discussion (ChrisJBurns at 2025-10-07T13:46:40Z):

In the event that vMCP is ever exploited, would it be able to make calls to the backing MCP servers directly?

From PR discussion (JAORMX at 2025-10-07T13:49:36Z):

backed MCP servers should still not be deployed as they are without appropriate protections. [...] I'm more keen on a zero-trust policy in general.

Recommendation: Add explicit Security section:

## Security Considerations

### Zero-Trust Architecture

Virtual MCP operates on a zero-trust security model:

1. **Backend servers remain protected**: All backend MCP servers MUST continue to enforce their own authentication and authorization, independent of Virtual MCP's auth layer
2. **Defense in depth**: Virtual MCP compromise does not grant automatic access to backends
3. **Credential isolation**: Each backend receives only credentials scoped for that specific service
4. **Audit trail preservation**: Backend servers maintain their own audit logs in addition to Virtual MCP's aggregated logging

### Security Boundaries

┌─────────────────────────────────────────┐
│ Client (authenticated to Virtual MCP) │
└──────────────┬──────────────────────────┘
│ Auth boundary #1
┌──────────────▼──────────────────────────┐
│ Virtual MCP Server │
│ - Validates client token │
│ - Exchanges/transforms per backend │
└──┬───────────┬────────────┬──────────────┘
│ │ │ Auth boundary #2
┌──▼───┐ ┌───▼────┐ ┌────▼─────┐
│GitHub│ │ Jira │ │ Slack │
│(auth)│ │ (auth) │ │ (auth) │
└──────┘ └────────┘ └──────────┘


### Deployment Recommendations

- Backend MCP servers SHOULD NOT be directly accessible to clients (network isolation)
- Virtual MCP SHOULD run with minimal privileges (least privilege principle)
- Secrets for backend authentication MUST be stored in secure secret backends (1Password, encrypted storage)
- Virtual MCP SHOULD enforce additional authorization policies using Cedar before routing requests

3. Conflict Resolution Underspecified ⚠️

Issue: Line 42 mentions conflict_resolution: "prefix" | priority | manual but doesn't explain the behavior.

Questions:

  1. prefix: What prefix format? Workload name? Custom prefix?
  2. priority: How is priority order determined? First-wins? Configuration-based?
  3. manual: Is this referring to the overrides section?

Recommendation: Expand with concrete examples:

aggregation:
  # Option 1: Automatic prefixing with workload name
  conflict_resolution: "prefix"
  # Results in: github_create_pr, jira_create_pr

  # Option 2: Priority-based (first backend wins)
  conflict_resolution: "priority"
  priority_order: ["github", "jira", "slack"]
  # If both github and jira expose "create_issue", use github's version

  # Option 3: Manual resolution via overrides
  conflict_resolution: "manual"  # Requires explicit overrides for conflicts
  # Throws error if tools conflict without override

Major Issues

4. Error Handling and Failure Modes Inadequate ⚠️

Current state: Open questions section (lines 343-347) acknowledges some gaps but doesn't address critical failure scenarios.

Missing specifications:

  1. Backend workload failure: What if a backend stops/crashes during Virtual MCP operation?
  2. Partial failures: Some backends succeed, others timeout - how to handle?
  3. Circuit breaker patterns: Should Virtual MCP stop routing to unhealthy backends?
  4. Composite tool rollback: If step 3 fails, what happens to steps 1-2?
  5. Elicitation timeout: User doesn't respond - how long to wait?
    • ✅ Partially addressed: Line 336 mentions 30-minute workflow timeout
    • ❌ Missing: What happens to backend state if elicitation times out mid-workflow?

Recommendation: Add Failure Handling section:

### Failure Handling

#### Backend Unavailability
- **Detection**: Health checks every 30s against backend `/health` endpoint
- **Behavior**: Mark backend as unavailable, remove its tools from capability list
- **Recovery**: Automatic re-inclusion when health check succeeds
- **Client notification**: Return error with unavailable backend list

#### Partial Failures
- **Tool calls**: Return error immediately if target backend fails
- **Capability aggregation**: Succeed with partial capability list if some backends fail
- **Composite tools**: Abort workflow on first step failure, return error with completed steps

#### Circuit Breaker
- **Threshold**: 5 consecutive failures or 50% error rate over 1 minute
- **Open state**: Stop routing to backend for 60 seconds
- **Half-open**: Allow 1 request through to test recovery

5. Composite Tool Complexity May Need Separate Proposal 🤔

Concern: Composite tools (lines 91-138, 245-260) introduce significant complexity:

  • DAG execution with depends_on
  • Template variable substitution ({{.params.x}}, {{.steps.y.output}})
  • Conditional execution based on expressions
  • Error handling across multiple tool calls
  • State persistence for long-running workflows (lines 334-336)
  • Elicitation protocol integration

This is essentially a workflow engine, which is a different problem domain than request aggregation/routing.

Risk: Combining both in one implementation may:

  • Delay MVP delivery
  • Complicate testing
  • Mix concerns (routing vs orchestration)

Recommendation: Consider one of:

  1. Separate proposal: Move composite tools to THV-XXXX-workflow-engine.md
  2. Simplified Phase 2: Defer complex features (DAG execution, conditionals, state persistence) to Phase 3
  3. Keep as-is: If composite tools are core to Virtual MCP value prop (which they are, based on benefits section line 306)

Given the emphasis in benefits section, keeping composite tools makes sense, but:

  • Add note acknowledging this is a significant undertaking
  • Consider MVP composite tools without elicitation/conditionals/persistence
  • Add separate success criteria for composite tool features

6. State Persistence Design Insufficient ⚠️

Issue: Line 334-336 mentions "in-memory with clean storage interface" but doesn't address:

  1. Memory limits: What if 1000 workflows are running? Memory exhaustion?
  2. Server restart: All workflows lost?
  3. Distributed deployment: Can't share state across multiple Virtual MCP instances
  4. Debugging: How to inspect workflow state for troubleshooting?

Recommendation: Either:

  • Option A: Commit to persistent storage from Phase 2 (Redis, PostgreSQL, etcd)
  • Option B: Add clear limitations to documentation:
    ### Phase 2 State Persistence Limitations
    
    - **Single instance only**: No horizontal scaling
    - **Ephemeral workflows**: Server restart loses all in-flight workflows
    - **Memory bounded**: Maximum 1000 concurrent workflows (configurable)
    - **No debugging tools**: State inspection requires restart with debug logging
    
    Phase 3 will migrate to persistent storage for production deployments.

Minor Issues

7. Group Reference Validation ℹ️

Issue: Line 38 references group: "engineering-team" but doesn't specify validation behavior.

Add:

#### Group Validation
- Error if referenced group doesn't exist: `group 'engineering-team' not found`
- Error if group is empty: `group 'engineering-team' has no workloads`
- Option to auto-start workloads: `--auto-start-workloads` flag

8. CLI Command Naming 💭

Minor: thv virtual (line 266) could be clearer. Consider:

  • thv virtual-server - explicit that it's running a server
  • thv aggregate - describes the function
  • Current name is fine if brevity preferred

9. Observability Strategy Missing 📊

Issue: Success criteria (lines 349-355) mentions "< 10ms routing overhead" but no mention of how to measure this.

Add to Implementation Notes:

### Observability

- **Metrics**: OpenTelemetry metrics for routing latency, backend health, tool call distribution
- **Tracing**: Distributed traces spanning Virtual MCP → Backend calls
- **Logging**: Structured logs with correlation IDs for multi-backend requests
- **Dashboards**: Pre-built Grafana dashboard for Virtual MCP monitoring

10. Streaming Responses Open Question

Note: Line 345 asks "How to handle streaming responses across multiple backends?"

This is actually a critical decision for Phase 1, not Phase 3, because:

  • MCP clients may expect streaming responses
  • If unsupported in MVP, needs clear documentation
  • May affect transport selection (SSE vs HTTP)

Recommendation: Decide for Phase 1:

  • Option A: Only support non-streaming backends in MVP
  • Option B: Support streaming but only single-backend streaming (not merged streams)
  • Option C: Full streaming support with multiplexed channels (complex)

Add to MVP scope: "Phase 1 supports streaming from individual backends but does not merge streaming responses across multiple backends."


Additional Comments

11. Token Exchange vs Other Auth Types 🔐

The proposal lists 5 auth types (lines 223-227) but only token_exchange is well-defined via THV-2063:

  • pass_through: Clear enough
  • service_account: Where are credentials stored? How refreshed?
  • header_injection: Similar to service_account but different header?
  • mapped_claims: Requires JWT manipulation - complex and risky

Recommendation: For Phase 1, support only:

  • pass_through
  • token_exchange (via THV-2063)

Defer service_account, header_injection, mapped_claims to Phase 2.

12. Elicitation Example Could Be Clearer 💡

The elicitation example (lines 102-122) is good but could emphasize the user interaction flow:

**User experience**:
1. User calls `deploy_and_notify` with pr_number=123
2. Virtual MCP merges PR (step 1)
3. Virtual MCP prompts user: "PR 123 merged. Deploy to [Staging/Production]? [Notify team: yes/no]"
4. User selects "Production" and "yes"
5. Virtual MCP deploys (step 3) and notifies (step 4)

13. Relationship to MCPRemoteProxy CRD 🤔

I notice a recent commit mentions MCPRemoteProxy CRD (from git log). Is there overlap with Virtual MCP? Should the proposal mention this?

If MCPRemoteProxy is for Kubernetes operator and thv virtual is for CLI, clarify the relationship:

### Kubernetes Deployment

Virtual MCP can be deployed in Kubernetes via the `MCPRemoteProxy` CRD (see THV-XXXX for details). The CLI `thv virtual` command is intended for local development and single-machine deployments.

Overall Assessment

Verdict: ✅ Approve with significant revisions

This is a strong proposal with a well-thought-out design that appropriately leverages existing ToolHive infrastructure. The recent additions around elicitation and state persistence show good evolution based on feedback.

However, there are critical gaps that must be addressed before implementation:

Blocking Issues (must fix before Phase 1 implementation):

  1. Security model (Issue Do we want the container monitor? #2) - Add explicit zero-trust security section
  2. Token exchange integration (Issue fix(typo): corrects readme #1) - Link to THV-2063 and show per-backend config
  3. Conflict resolution details (Issue Bump golangci/golangci-lint-action from 2f856675483cb8b9378ee77ee0beb67955aca9d7 to 4696ba8babb6127d732c3c6dde519db15edab9ea #3) - Define prefix/priority/manual behaviors
  4. Streaming support decision (Issue Implement secrets management #10) - Clarify MVP streaming support

Important Issues (should address before Phase 2):

  1. ⚠️ Error handling specification (Issue Implement secret store #4) - Add failure modes and degradation
  2. ⚠️ State persistence limitations (Issue Figure out ergonomics for exposing directories #6) - Document in-memory constraints
  3. ⚠️ Composite tool scope (Issue Implement secret injection #5) - Consider complexity vs MVP timeline

Nice-to-haves (can defer to implementation):

  1. ℹ️ Group validation (Issue Create proxy subcommand #7)
  2. 📊 Observability strategy (Issue Use an actual logger #9)
  3. 🔐 Simplified auth types for Phase 1 (Issue feat: Add directory mounting capabilities to vt run and registry run #11)

Recommendations for Next Steps

  1. Address blocking issues: Add security section, token exchange details, conflict resolution examples
  2. Decide on composite tools: Keep in proposal but simplify Phase 1 scope? Or separate proposal?
  3. Update implementation phases: Based on composite tool decision, revise MVP scope
  4. Link related proposals: Reference THV-2063 (token exchange), mention MCPRemoteProxy if relevant
  5. Consider creating follow-up proposals:
    • THV-XXXX-virtual-mcp-observability.md (if observability is complex)
    • THV-XXXX-virtual-mcp-security-model.md (if security warrants detailed design)

Additional Context from PR Discussion

From the PR comments, I see important security discussion between @ChrisJBurns, @JAORMX, and @jhrozek about:

The proposal should explicitly address the OAuth consent question from @jhrozek:

we'd have to send the external API token along with the request -or- let the back end token servers exchange the tokens they receive for external API tokens

This is a critical architectural decision about where token exchange happens (Virtual MCP vs backends).


Job Run | Branch: proposal/virtual-mcp-server

This proposal introduces a Virtual MCP Server that aggregates multiple
MCP servers from a ToolHive group into a single unified interface.

Key features:
- Leverages existing ToolHive groups for backend management
- Uses existing ToolsFilter and ToolOverride constructs
- Supports per-backend authentication strategies
- Enables composite tools for cross-service workflows
- Maintains full MCP protocol compatibility

The Virtual MCP Server will simplify client integration by providing a
single connection point while handling the complexity of multiple backend
authentication requirements and tool namespace management.

Signed-off-by: Juan Antonio Osorio <[email protected]>
@JAORMX JAORMX force-pushed the proposal/virtual-mcp-server branch from f1f918c to ac1d31f Compare October 20, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants