Skip to content

Add vMCP Serve skeleton and ServerConfig#5467

Draft
tgrunnagle wants to merge 3 commits into
vmcp-core-p1-5_issue_5438from
vmcp-core-p2-1_issue_5439
Draft

Add vMCP Serve skeleton and ServerConfig#5467
tgrunnagle wants to merge 3 commits into
vmcp-core-p1-5_issue_5438from
vmcp-core-p2-1_issue_5439

Conversation

@tgrunnagle

Copy link
Copy Markdown
Contributor

Summary

This is the transport-side entry point of the New/Serve split that the rest of
the vMCP Phase 2 refactor (epic #5419) builds on. Today server.New is a
god-object that constructs both the identity-parameterized core domain logic and
the HTTP/SDK transport in one call. This PR lands the additive Serve skeleton so
later tasks can relocate the remaining transport concerns onto it without churning
server.New.

  • Add Serve(ctx, core.VMCP, *ServerConfig) (*Server, error) in the new
    pkg/vmcp/server/serve.go. Serve wraps an already-constructed core VMCP and
    returns the existing *Server struct, so all carried-forward accessors
    (Handler/Start/Stop/Address/MCPServer/Ready) keep working unchanged.
  • Add the transport-only ServerConfig struct — the subset of the legacy
    server.Config that configures the HTTP/SDK runtime, plus the cross-cutting
    TelemetryProvider/AuditConfig consumed by both the core and the transport.
  • Serve applies the same transport defaults server.New applies today
    (Host127.0.0.1, EndpointPath/mcp, Name/Version/SessionTTL
    defaults; Port == 0 stays "OS-assigned"), builds the mcp-go server, and wires
    the core's Close into the server's shutdown sequence.
  • Guard the /status handler against a nil backend registry, since the transport
    does not yet own the registry at this phase.

server.New is intentionally not routed through Serve in this PR, so the
change is purely additive and observable behavior is unchanged. This keeps the PR
small and the parity surface flat; Phase 3 (#5444/#5445) reduces server.New to a
wrapper over Serve.

Stacked PR. This branch is stacked on #5459 (admission seam, Closes #5438),
which is itself stacked on the New/VMCP work (#5437). It should target the
parent branch vmcp-core-p1-5_issue_5438 for review, not main. The
pkg/vmcp/core/* changes visible in a diff against main are inherited from
those upstream PRs and are not part of this change. The only files this PR
introduces are pkg/vmcp/server/serve.go, pkg/vmcp/server/serve_test.go, and
the pkg/vmcp/server/status.go nil-registry guard.

Closes #5439

Type of change

  • Refactoring (no behavior change)

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Added pkg/vmcp/server/serve_test.go covering:

  • Serve applies transport defaults identically to server.New and leaves the
    caller's ServerConfig unmutated and Port == 0 OS-assigned.
  • Serve preserves explicitly set config values.
  • The returned *Server exposes a usable MCPServer(), and Handler(ctx)
    registers the unauthenticated routes (/health, /ping, /readyz, /status,
    /api/backends/health); /metrics is absent without a telemetry provider.
  • Stop runs the shutdown funcs and closes the injected core.
  • Validation returns a vmcp.ErrInvalidConfig-wrapped error for nil cfg, nil
    core, and nil SessionFactory.
  • A reflection-based drift guard asserts every Config field is populated by
    buildServeConfig except the documented intentionally-unmapped set
    (AuthzMiddleware, HealthMonitorConfig).

The existing server.New-driven behavioral-parity suite stays green; server.New
is untouched and not routed through Serve. Repo-wide lint is clean apart from a
pre-existing, unrelated gosec G115 finding in cmd/thv/app/upgrade.go.

Changes

File Change
pkg/vmcp/server/serve.go New Serve entry point and transport-only ServerConfig; buildServeConfig maps config and applies defaults
pkg/vmcp/server/serve_test.go Unit tests for defaults, config preservation, route registration, shutdown wiring, validation, and mapping drift
pkg/vmcp/server/status.go Nil-registry guard in buildStatusResponse so the Serve skeleton returns an empty backend view instead of panicking

Does this introduce a user-facing change?

No.

Special notes for reviewers

tgrunnagle and others added 2 commits June 8, 2026 12:00
Introduce the transport-side entry point of the vMCP New/Serve split.
Serve wraps an already-constructed core VMCP and returns the existing
*Server, building the mcp-go server and applying the transport defaults
that server.New applies today. The route mux and HTTP lifecycle are
provided by the carried-forward Handler/Start/Stop, so observable
behavior is unchanged and server.New is not yet routed through Serve.

Implements changes for issue #5439:
- Add ServerConfig (transport-only fields plus the cross-cutting
  TelemetryProvider/AuditConfig) and Serve in pkg/vmcp/server/serve.go
- Validate nil cfg / nil core / nil SessionFactory via ErrInvalidConfig
- Wire the core's idempotent Close into the server shutdown sequence
- Guard the /status handler against a nil backend registry, which the
  transport does not own yet in this phase

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Clarify the Serve doc comment: it builds only the mcp-go server and
  shutdown wiring; the route mux and HTTP lifecycle stay in the
  carried-forward Handler/Start until later phases relocate them
- Document the Config fields buildServeConfig deliberately leaves
  unmapped (AuthzMiddleware, HealthMonitorConfig) and why
- Add a reflection drift guard asserting every other Config field is
  populated by buildServeConfig, so a future field can't be dropped
  silently

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jun 8, 2026
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.25%. Comparing base (d048950) to head (3d5e331).

Additional details and impacted files
@@                      Coverage Diff                      @@
##           vmcp-core-p1-5_issue_5438    #5467      +/-   ##
=============================================================
+ Coverage                      69.23%   69.25%   +0.02%     
=============================================================
  Files                            637      638       +1     
  Lines                          64859    64909      +50     
=============================================================
+ Hits                           44907    44955      +48     
- Misses                         16629    16633       +4     
+ Partials                        3323     3321       -2     

☔ View full report in Codecov by Harness.
📢 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.

@tgrunnagle tgrunnagle left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Multi-agent review — Serve skeleton + ServerConfig

Sound, behavior-preserving additive skeleton. The deferral of the route mux/lifecycle to the carried-forward Handler/Start/Stop (rather than rebuilding it in Serve) is the correct design call and faithful to the epic's "relocate, don't rewrite" principle — not a scope gap. Tests pass; clean boundary (no mcp-go types cross core.VMCP); buildServeConfig copies-before-mutating (better than New).

No HIGH findings — nothing blocks merge. The MEDIUM items are about tightening the parity-drift protection this staged refactor leans on, plus a couple of test/comment-precision gaps.

Severity Finding
MEDIUM Default literals triplicated (New / buildServeConfig / test) + drift guard checks presence, not value/inverse
MEDIUM Dead Config.StatusReporter mapping — set directly on the struct, mapped only to satisfy the drift test
MEDIUM .well-known/ route in the AC but untested; "auth-chain absent" comment overstates reality (Handler still builds the chain)
LOW-MED Serve-built *Server is not startable (nil session/discovery/registry); document the nil-field contract
LOW nil-registry guard may silently mask a real misconfig on the server.New path

Minor (non-blocking, no inline comment): no positive /metrics-registered assertion; Close idempotency and a combined nil-cfg+nil-vmcp ordering case aren't asserted.

Reviewed by 4 specialist agents (correctness, architecture, test, security). Codex cross-review skipped (CLI not installed).

🤖 Generated with Claude Code

Comment thread pkg/vmcp/server/serve.go Outdated
Comment thread pkg/vmcp/server/serve.go Outdated
Comment thread pkg/vmcp/server/serve.go
Comment thread pkg/vmcp/server/serve_test.go
Comment thread pkg/vmcp/server/serve_test.go
Comment thread pkg/vmcp/server/status.go
Addresses #5467 review comments:
- MEDIUM serve.go (3381738359): extract shared default consts
  (defaultServerName/Version/Host/EndpointPath) referenced by New and
  buildServeConfig, and assert against them in tests, killing the
  triplicated literals
- MEDIUM serve.go (3381738361): drop the dead Config.StatusReporter
  mapping (set directly on Server like HealthMonitor) and add it to the
  drift guard's intentionallyUnmapped set
- LOW-MEDIUM serve.go (3381738364): document the returned *Server's
  nil-field contract (must not be Started/served on "/" until later
  phases wire session/discovery/registry)
- MEDIUM serve_test.go (3381738372): probe /.well-known/ for the clean
  JSON 404; soften the overstated "auth-chain absent" comment
- MEDIUM serve_test.go (3381738374): note the drift guard checks
  presence only; cover SessionTTL/StatusReportingInterval value
  correctness in TestServePreservesExplicitConfig
- LOW status.go (3381738382): slog.Debug in the nil-registry branch so
  an unexpectedly-nil registry on the New path is observable

Also from the review body: add a positive /metrics-registered test and
a combined nil-cfg+nil-vmcp validation case.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tgrunnagle

Copy link
Copy Markdown
Contributor Author

Re: the non-blocking minor items in the review body (addressed in 3d5e331):

  • Positive /metrics assertion — added TestServeHandlerRegistersMetricsWhenTelemetryEnabled, which builds a real telemetry.Provider with EnablePrometheusMetricsPath: true and asserts /metrics returns 200.
  • Combined nil-cfg + nil-vmcp ordering — added a nil config and nil vmcp case to TestServeValidation, confirming it fails cleanly (cfg is checked first; no nil-deref).
  • Close idempotency — left as-is: idempotency is a contract of real core.VMCP implementations, not of Serve's wiring. Serve appends exactly one Close to shutdownFuncs, which TestServeStopClosesCore already verifies. Happy to add a count-based assertion if you'd prefer it pinned.

@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant