Define VMCP core interface and Config contract#5450
Conversation
Establish the contract-only first step of the vMCP New/Serve split: the identity-parameterized VMCP domain interface and the core Config struct of collaborators. This is the domain boundary that New(cfg) -> VMCP and the decorators will consume; no implementation yet. Implements issue #5434: - Add pkg/vmcp/core with the VMCP interface (ListTools/CallTool/List+ Read/List+GetPrompt/Lookup{Tool,Resource,Prompt}/Close), identity an explicit *auth.Identity on every data method, never read from context. - Encode the full behavioral contract in the interface doc comment (anonymous nil identity, decorators subtract-only, read-only args/meta, nil ResourceReadResult.Meta). - Declare Config with typed collaborator fields; no construction logic. - No mcp-go types cross the boundary (anti-pattern #5). The core lives in pkg/vmcp/core rather than root pkg/vmcp because Config references aggregator/router/composer/health/authz, all of which import pkg/vmcp; a root-package Config would create an import cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5450 +/- ##
==========================================
- Coverage 68.85% 68.85% -0.01%
==========================================
Files 634 634
Lines 64437 64437
==========================================
- Hits 44371 44370 -1
- Misses 16789 16790 +1
Partials 3277 3277 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
There was a problem hiding this comment.
Review summary
Clean, well-scoped contract-only PR: a single 156-line new file (pkg/vmcp/core/core.go) declaring the identity-parameterized VMCP interface and a typed Config collaborator struct, with no implementation. All ten acceptance-criteria methods are present, identity is an explicit *auth.Identity on every data method (anti-pattern #1), and zero mcp-go types cross the boundary (anti-pattern #5). The interface is a faithful non-session-scoped generalization of the prior-art Caller interface.
Verified against the codebase:
- The package-location deviation (
pkg/vmcp/coreinstead of the issue's rootpkg/vmcp) is justified —aggregator,router,composer, andhealthall import rootpkg/vmcp, so a root-packageConfigreferencing them would create an import cycle. Confirmed empirically. - All referenced types resolve (
vmcp.Tool/Resource/Prompt, the result wrappers,BackendRegistry,BackendClient, and everyConfigcollaborator type). meta-on-CallTool-only (notGetPrompt) matches both theCallerprior art andBackendClient.GetPrompt(which has nometaparam). The approved deviation from AC#2's wording is sound.- Omitting the
Elicitationfield is correct — no domainElicitationRequestertype exists yet, and the field is purely additive on a struct with zero consumers.
Three minor doc nits (all non-blocking, see inline comments).
| # | Severity | Finding | Raised by |
|---|---|---|---|
| 1 | LOW | Doc comment hard-codes types.go:556-561 line numbers that will rot |
3 reviewers |
| 2 | LOW | godoc link [pkg/vmcp/session/types.ShouldAllowAnonymous] won't render (package not imported) |
1 reviewer |
Doc reconciliation (no code change): Issue #5434's acceptance-criteria checkboxes reference pkg/vmcp/vmcp.go; since the canonical location is now core.VMCP, the issue and downstream PRs #5437/#5439 should be annotated so the core package is recognized as the single front door.
Notes for downstream PRs (contract claims this PR cannot enforce — flag during implementation review):
- #5437 must actually copy
args/metabefore mutating (the read-only guarantee is doc-only here), andList*/Lookup*must share a single admission filter so a denied capability is invisible to both paths. - #5438's admission decorator constructor should take only an inner
VMCP(no backend-reaching collaborators) to keep the "decorators may only subtract reachability" invariant structurally true. - Confirm the
composercollaborator is constructed internally byNewfromWorkflowDefsrather than injected; if injected, add the field now to avoid a laterConfigshape change.
Recommendation: APPROVE — no blocking issues; the inline items are optional polish.
🤖 Generated with Claude Code
Addresses #5450 review comments: - LOW pkg/vmcp/core/core.go (3351574912): replace the unresolvable [pkg/vmcp/session/types.ShouldAllowAnonymous] godoc link (package not imported) with plain prose so it no longer renders as literal brackets. - LOW pkg/vmcp/core/core.go (3351574935): drop the rot-prone types.go:556-561 line citation in favor of the self-maintaining [vmcp.ResourceReadResult].Meta doc link. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
RFC THV-0076 extracts a small, identity-parameterized domain object from today's vMCP transport god-object (
server.New), so that aggregation, routing, and composite workflows live behind a single explicit contract instead of being entangled with HTTP/session concerns. Before any of that machinery can be built, the domain boundary itself has to exist and be agreed on — that is what this PR delivers.This is the contract-only first PR of Phase 1. It declares:
VMCPdomain interface (ListTools/CallTool,ListResources/ReadResource,ListPrompts/GetPrompt, theLookup*resolution methods, andClose), with its full behavioral contract encoded in the doc comment.Configstruct of collaborators as typed field declarations.There is no implementation:
New(cfg) (VMCP, error)and the concretecoreVMCPbodies land in #5437. The deliverable here is the durable contract that #5437 implements and thatServeand the decorators downstream consume.Closes #5434
Type of change
Test plan
task test)task lint-fix)This is a contract-only change, so compile-time conformance is the guarantee per the issue's testing strategy — there is no behavior to exercise until
Newis implemented (#5437). Verification performed:task buildgreen — proves the package compiles with no import cycle.task testgreen (exit 0, no races) — full suite unchanged;server.Newis untouched.task lintfinding is a pre-existing gosec G115 incmd/thv/app/upgrade.go, unrelated to this PR and not in this diff.Changes
pkg/vmcp/core/core.goVMCPinterface with its full contract doc comment, and the typedConfigcollaborator struct.Does this introduce a user-facing change?
No.
Special notes for reviewers
Package location differs from the issue text (user-approved). The issue specified placing the interface and
Configin the rootpkg/vmcppackage (pkg/vmcp/vmcp.go). However,Configreferences the aggregator, router, composer, health, and authz collaborators, and all of those transitively import the rootpkg/vmcppackage — verified empirically withgo list -deps. A root-packageConfigwould therefore create an import cycle. After confirming this, the interface andConfigwere placed in a newpkg/vmcp/coresubpackage instead. This also resolves the same latent cycle that #5437'sNew/coreVMCPwould otherwise hit. The issue text and #5437/#5430 likely need a follow-up note to reflect this package location.Two other user-approved deviations from the issue's literal spec:
Config.Elicitation ElicitationRequesterfield is omitted here because its domain type is owned by P1.3 Domain-typed ElicitationRequester (bounded rewrite) #5436 and is out of scope for this PR. A doc comment marks where it will be added.metais onCallToolonly, notGetPrompt. This matches the authoritative interface signature, theCallerprior art, andvmcp.BackendClient.GetPrompt(which has nometaparameter — there is no client_metato forward on the prompts/get path). AC#2's prose mentioningGetPromptis a wording discrepancy in the issue.Context: Parent story #5430; this PR blocks #5437.