Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4934 +/- ##
==========================================
- Coverage 69.76% 69.74% -0.02%
==========================================
Files 560 560
Lines 56475 56477 +2
==========================================
- Hits 39397 39391 -6
- Misses 14055 14063 +8
Partials 3023 3023 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
amirejaz
approved these changes
Apr 20, 2026
Embedders that wrap the vMCP composer in their own pipeline cannot
today target elicitation at the mark3labs MCPServer instance that
actually serves /mcp: the field is unexported, there is no accessor,
and the SDK adapter constructor is also unexported. A parallel
MCPServer built by the embedder does not work because ClientSession
correlation is keyed to the server that received initialize.
Add two additive, read-only seams on pkg/vmcp/server:
- (*Server).MCPServer() returns the authoritative serving instance.
- NewSDKElicitationAdapter exports the existing adapter constructor
so embedders can obtain a composer.SDKElicitationRequester bound
to the serving MCPServer.
Doc comments spell out the in-process trust boundary, lifecycle
ownership, the "prefer per-session over global tool registration"
footgun, and MCP 2025-06-18 elicitation caller obligations (ctx
propagation with a bounded deadline, elicitation capability on the
client, distinct accept/decline/cancel handling, flat-schema shape,
no secrets or internal addressing in prompts).
Closes #4930
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
23b3bb5 to
51850ff
Compare
Collaborator
Author
|
Had to rebase |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
*server.MCPServerthat actually handles/mcptraffic. The field is unexported, there is no accessor, and the SDK adapter constructor is also unexported. A parallelMCPServerbuilt by the embedder does not work becauseClientSessioncorrelation is keyed to the server that receivedinitialize.pkg/vmcp/server:(*Server).MCPServer() *server.MCPServerreturns the authoritative serving instance.NewSDKElicitationAdapterexports the existing adapter constructor so embedders can obtain acomposer.SDKElicitationRequesterbound to the servingMCPServer.Start/Stoponly), the "prefer per-session over global tool registration" footgun, and the MCP 2025-06-18 elicitation caller contract: ctx propagation with a bounded deadline, client capability advertisement, distinct accept/decline/cancel handling, flat-schema shape, and no secrets or internal addressing in prompt payloads.Closes #4930
Type of change
Test plan
task test)task lint-fix)Manual verification in the worktree:
task lint-fix— 0 issues.go test -count=1 ./pkg/vmcp/server/...— all green, including the newTestServer_MCPServer_ReturnsSameInstancewhitebox test that asserts the accessor returns the exact pointer stored at construction.go build ./...— clean.newSDKElicitationAdapterreferences — none remain.Does this introduce a user-facing change?
No end-user behavior change. Public API of
pkg/vmcp/servergains two exported symbols ((*Server).MCPServer()andNewSDKElicitationAdapter); the addition is purely additive and no existing call site behavior changes.Implementation plan
Approved implementation plan
Approach
Implement options (a) + (b) from the issue — the two minimal, orthogonal, additive seams.
Rejected alternatives:
WithElicitationHandleroverride): the stated use case is embedders running their own composer pipeline, not replacing toolhive's internal one. (c) would add surface without serving the described need.NewElicitationRequester(*Server) composer.SDKElicitationRequesterthat hides the mark3labs type was considered (mitigates anti-pattern Implement secret injection #5 in.claude/rules/vmcp-anti-patterns.md). Rejected because the issue explicitly asks for*MCPServerdisclosure and the embedder use case extends beyond elicitation (hooks, notifications, sampling). The doc comment onMCPServer()acknowledges the SDK-type exposure as a deliberate, narrow tradeoff.Changes
NewSDKElicitationAdapterinpkg/vmcp/server/sdk_elicitation_adapter.go(wasnewSDKElicitationAdapter). Struct stays unexported; only the constructor is public. Doc expanded to direct embedders to(*Server).MCPServer(). One internal caller updated (server.go:344) and one test reference updated.(*Server).MCPServer() *server.MCPServerinpkg/vmcp/server/server.go, placed next toSessionManager(), with a doc comment covering:Start/Stopown it; callers MUST NOT close or reconfigure.Things NOT changing
composer.SDKElicitationRequester/composer.NewDefaultElicitationHandler— already public.Configstruct — no new field.Anti-pattern check (
.claude/rules/vmcp-anti-patterns.md)Server; one accessor method.Pre-code reviews
Reviewed by
toolhive-expert,security-advisor,mcp-protocol-expert, andPlanagents in parallel before any code was written. Findings (em-dash removal in docs, protocol-caller notes, test name and redundancy) folded into the final doc comments and test.Post-code reviews
Reviewed by
code-reviewer,toolhive-expert,mcp-protocol-expert,security-advisor,unit-test-writer, and thevmcp-reviewskill in parallel on the diff. All findings addressed; no blocking issues remain.🤖 Generated with Claude Code