Move SDK hooks and two-phase session creation under Serve

[tgrunnagle]

Summary

This is step P2.2 of the vMCP New/Serve split (Phase 2). The mcp-go SDK hooks and the
two-phase session-creation wiring are SDK-lifecycle concerns that belong in the transport
layer of the New/Serve split, not in the core VMCP domain object — but they still lived in
server.New. This PR relocates them under Serve so they execute identically when Serve is
exercised directly, keeping the mcp-go two-phase dance (anti-pattern #5) isolated behind the
transport boundary and out of the core interface.

• Why: Finish moving the SDK session layer behind Serve so the core VMCP object stays
  free of mcp-go session concepts (hooks, two-phase creation, cross-pod Redis re-hydration).
• What: Move the three SDK hooks (OnRegisterSession, OnBeforeListTools,
  OnBeforeCallTool) and the session-creation wiring (transport session manager, session data
  storage, vMCP session manager) from server.New into Serve in pkg/vmcp/server/serve.go.
• Scope: Purely additive. server.New keeps its own copy until Phase 3, so its signature
  and observable behavior are unchanged; the relocated callbacks and collaborators keep
  their existing shapes.

Closes #5440

Type of change

• Refactoring (no behavior change)

Test plan

• Unit tests (task test) — pkg/vmcp/server passes with -race
• Linting — golangci-lint run ./pkg/vmcp/server/... reports 0 issues
• Manual testing (describe below)

New Serve-level tests drive the relocated hooks and two-phase session creation through the
SDK lifecycle directly (mounting the Streamable HTTP server on the relocated mcpServer +
vmcpSessionMgr, bypassing the not-yet-relocated discovery middleware that #5441/#5442 own):

• TestServeRegistersSessionHooks — an MCP initialize fires OnRegisterSession, which runs
  two-phase creation (MakeSessionWithID) and injects the per-session tools so a subsequent
  tools/list advertises them; the OnBeforeListTools hook runs and no-ops on the pod-local
  path.
• TestServeLazyInjectsToolsForRehydratedSession — covers the cross-pod re-injection branch:
  with a session registered in the vMCP session manager, a fresh SDK ClientSession whose
  per-session tool store is empty (as on a second pod, where OnRegisterSession never fired)
  gets its tools re-injected by lazyInjectSessionTools; a populated store is left untouched.
• TestServeReturnsErrorWhenSessionManagerConstructionFails — asserts Serve surfaces a
  sessionmanager.New failure that occurs after the session data storage is built (the path
  the closeStorageOnErr guard protects). It confirms the guarded path is reached; it does not
  directly observe Close() (the storage is built internally).
• TestBuildSessionDataStorage / TestBuildSessionDataStorageRedis — provider selection:
  nil/empty/memory (case-insensitive) yields in-process storage; redis takes the Redis path
  reading THV_SESSION_REDIS_PASSWORD; unknown providers are rejected.

The ~1.2k-line HTTP session-management integration suite stays on server.New (unchanged) as
the behavioral-parity gate.

Note on repo-wide checks: task lint-fix has a pre-existing, unrelated gosec G115 failure
in cmd/thv/app/upgrade.go, and task test has a pre-existing, unrelated
Docker-dependent failure in pkg/mcp/server (TestBuildServerConfig "no available runtime
found"). Neither is caused by this change; the affected packages are untouched here.

Changes

File	Change
pkg/vmcp/server/serve.go	Relocate the three SDK hooks and two-phase session-creation wiring (transport session manager, buildSessionDataStorage with the closeStorageOnErr guard, vMCP session manager) into Serve; rework ServerConfig (see below).
pkg/vmcp/server/serve_session_test.go	New: Serve-level coverage of the relocated hooks + two-phase creation driven through the SDK lifecycle, plus buildSessionDataStorage provider selection.
pkg/vmcp/server/serve_test.go	Update existing Serve tests for the new ServerConfig shape (SessionManagerConfig + BackendRegistry); add nil-BackendRegistry validation case; register t.Cleanup teardown.

Implementation details

• ServerConfig reshaped: added a pre-built *sessionmanager.FactoryConfig slot
  (SessionManagerConfig) and a shared BackendRegistry. The now-redundant standalone
  SessionFactory / OptimizerFactory / OptimizerConfig slots were folded into the
  FactoryConfig. Both new collaborators are validated and fail loudly with
  vmcp.ErrInvalidConfig when nil — BackendRegistry in particular, because the
  OnRegisterSession hook enumerates it inside a goroutine where the error would otherwise be
  swallowed.
• Mechanical relocation: the hook callbacks and the *Server receiver methods
  (handleSessionRegistration, lazyInjectSessionTools) are unchanged — they already operate
  against s.vmcpSessionMgr and the SDK server.ClientSession. The hooks object is built up
  front (so it can be passed to NewMCPServer) but its callbacks are registered after srv is
  assembled, since they close over srv.
• Resource-leak guard preserved: buildSessionDataStorage keeps the closeStorageOnErr
  defer so the storage's background cleanup goroutine is released on any error path out of
  Serve (pairs acquisition with release).
• Boundary held: no mcp-go types cross the VMCP interface; the "fixed at initialize"
  capability set, identity binding, and the cross-pod Redis re-hydration path stay in
  Serve/session.

Does this introduce a user-facing change?

No. This relocates internal transport wiring within pkg/vmcp/server; server.New's signature
and observable behavior are unchanged.

Special notes for reviewers

• Stacked PR: this branch is stacked on its dependency P2.1 Serve skeleton + ServerConfig #5439. The base branch is
  vmcp-core-p2-1_issue_5439 (not main) so the diff contains only this issue's two commits.
• The returned *Server has a live session manager and backend registry but a still-nil
  discovery manager / router / backend client at this phase. It must not be Start()ed or
  served on the / MCP route until P2.3 Move middleware chain under Serve; remove authz + annotation mw #5441/P2.4 Replace discovery-into-context with direct VMCP calls #5442 wire those fields; the unauthenticated routes
  remain safe to serve. The new tests mount the Streamable HTTP server directly to stay within
  this task's scope.
• Follow-up tasks relocate the remaining transport concerns: the authenticated middleware chain
  (P2.3 Move middleware chain under Serve; remove authz + annotation mw #5441), the direct VMCP request path (P2.4 Replace discovery-into-context with direct VMCP calls #5442), and the AS runner / status reporter /
  optimizer / health monitor lifecycle (P2.5 Move AS runner, status reporter, optimizer, health monitor under Serve #5443). Phase 3 (P3.1 deriveCoreConfig/deriveServerConfig config split #5444) reduces server.New to a
  Serve(ctx, New(...), ...) wrapper.

[codecov]

Codecov Report

❌ Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.29%. Comparing base (c311dc0) to head (5c5103d).

Files with missing lines	Patch %	Lines
pkg/vmcp/server/serve.go	87.17%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5471      +/-   ##
==========================================
+ Coverage   69.27%   69.29%   +0.02%     
==========================================
  Files         638      638              
  Lines       64904    64931      +27     
==========================================
+ Hits        44960    44992      +32     
+ Misses      16622    16612      -10     
- Partials     3322     3327       +5     

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

Multi-agent review — Move SDK hooks + two-phase session creation under Serve

Reviewed by 4 specialist agents (Go correctness, vMCP architecture, test coverage, general quality). Codex cross-review skipped (CLI not installed).

This is a faithful, behavior-preserving relocation. The block moved into Serve is a near-byte-for-byte copy of server.New (server.go:338–522), differing only by a safe ctx→hookCtx rename. The SDK boundary holds — zero mcp-go imports leak onto core.VMCP. ServerConfig reshape reduces config surface by reusing the existing FactoryConfig. All acceptance criteria in #5440 are met and scope is well under the 400 LOC / 10 file budget. On a normal PR this would be an APPROVE; left as a COMMENT because the PR is a draft (and GitHub disallows self-approval).

Findings

#	Severity	Finding	Location
M1	MEDIUM	validateWorkflows obligation moved to composition root with no enforcement seam	serve.go:117
M2	MEDIUM	Cross-pod re-injection path untested; PR description's "covered by integration suite" claim is inaccurate	serve_session_test.go:103
L1	LOW	sessionManager cleanup goroutine leaks on the Serve error path (asymmetric with the new storage guard)	serve.go:206
L2	LOW	toolSessionState.lastSession written under mutex but never read (dead state)	serve_session_test.go:45
L3	LOW	TestServeClosesStorageOnSessionManagerError name overstates — does not observe Close()	serve_session_test.go:169
L4	LOW	Redis test assertion not bound to the Redis branch	serve_session_test.go:245

0 HIGH — no blocking issues. The two MEDIUM items are worth resolving (or consciously deferring) before this leaves draft: M2 is the only one really worth acting on in this PR — either add the missing test or correct the description. M1 and L1 are reasonable to track for Phase 3 (#5444) when the two wiring copies collapse. L2–L4 are minor test-quality nits. Details are in the inline comments.

Dropped below threshold (no action): a wording nit on the discovery.Middleware nil-deref comment (the named mechanism was independently verified accurate, just incomplete); a doc clarification that the telemetry provider is intentionally set in two places; and an optional .Times(1) hardening of the MakeSessionWithID mock.