Route server.New through core.New and Serve

[tgrunnagle]

Summary

server.New was the vMCP transport god-object: a ~210-line constructor that inlined the
mcp-go server, SDK hooks, two-phase session creation, the workflow composer, telemetry
decoration, and the health monitor (anti-pattern #3). Phases 1–2 (#5430/#5431) extracted a
stateless domain core (core.New) and a transport entry point (Serve) behind a stable
seam, and #5444 added the deriveCoreConfig/deriveServerConfig projections — but
server.New still ran the legacy inline path, so the behavioral-parity suite only ever
exercised the dead-end legacy chain rather than the live core path.

This PR reduces server.New's body to the thin wrapper the RFC (THV-0076) specifies:
Serve(ctx, core.New(deriveCoreConfig(...)), deriveServerConfig(...)). The New/Serve
split becomes the single live path for every caller, so the parity suite now genuinely
gates the core path. The 7-param server.New signature is byte-for-byte unchanged.

• Route New through the core. server.New builds the backend health monitor at the
  composition root (A2), assembles the domain core via core.New(deriveCoreConfig(...)),
  and hands it to Serve(deriveServerConfig(...)). The health monitor is threaded both as
  a health.StatusProvider into the core (capability filtering) and as the lifecycle owner
  into Serve (Start/Stop, P2.5 Move AS runner, status reporter, optimizer, health monitor under Serve #5443).
• Feed the core under the frozen signature. Added Aggregator and Authz fields to
  server.Config — the only channel to supply the core without changing the signature.
  cli/serve.go populates both. A new authfactory.BuildAuthzConfig surfaces the
  *authz.Config the core admission seam consumes, reusing the exact config the existing
  Cedar HTTP middleware builder produces (DRY, allow-all parity preserved for the
  no-policies case).
• Resolve the elicitation construction-order inversion. The SDK elicitation adapter
  wraps the mcp-go server that Serve builds after core.New, but the core requires a
  non-nil ElicitationRequester at construction. A late-bound requester
  (elicitation_latebound.go) is handed to the core and bound to the real adapter after
  Serve returns, before serving begins.
• Make the core the single aggregator. FactoryConfig.AdvertiseFromCore is set so the
  session factory sources tools from the core and the Serve-layer optimizer (Wire the tool optimizer onto the Serve path #5543)
  advertises find_tool/call_tool, rather than the factory decorator aggregating in
  parallel.
• Remove now-dead constructor cruft. Dropped New's //nolint:gocyclo, the dead
  validateWorkflows, and four orphaned Server fields
  (router/backendClient/handlerFactory/capabilityAdapter) the legacy inline body
  held.
• Rework the parity suite to exercise the core path. Server tests now source tools from
  a stub or real Aggregator and route calls through core.CallTool → backend client,
  traversing identity binding — instead of the legacy chain.

Review feedback added two changes the reroute activates:

• Cache backend aggregation per identity. The core re-derives the advertised view on
  every call, so the reroute would otherwise sweep every backend's tools/list on every
  tool call. aggregator.NewCachingAggregator is a transparent, identity-keyed, TTL-bounded
  decorator wrapping the Aggregator below the core (the cache key includes the subject and
  forwarded credentials, so no caller's capability view leaks to another).
• Move backend health-monitor ownership into the core. core.New builds, starts, and
  (via Close) stops the monitor and filters with it, exposing it through a BackendHealth()
  reporter; the server keeps only the /health + /status reporting routes. This gives the
  monitor one owner and removes the server-side buildHealthMonitor, the
  healthMonitor/healthMonitorMu fields, ServerConfig.HealthMonitor, and the typed-nil
  dance. (Reverses the P2.5 Move AS runner, status reporter, optimizer, health monitor under Serve #5443 split that homed the monitor's Start/Stop in Serve.)

Part of #5445.

Rebased onto #5561

This branch is rebased onto #5561, which landed the two Serve-path prerequisites this
reroute would otherwise have had to carry, so they are intentionally not in this diff:

• Composite-tool workflow telemetry (toolhive_vmcp_workflow_*) now lives in the core
  (core_telemetry.go's telemetryComposer), reproducing the metrics the now-bypassed
  session-layer composite decorator emitted.
• Passthrough-header forwarding on the Serve path is resolved as per-request (Restore session-stable passthrough-header forwarding on the Serve path #5560):
  the shared backend client merges the headers captured by headerforward.CaptureMiddleware,
  and TestVMCPServer_PassthroughHeaders asserts the per-request contract. Forward passthrough headers on the Serve path #5561 also
  switched the integration harness (NewVMCPServer) to build the core via core.New + Serve
  directly, so both it and this rerouted server.New exercise the same production path.

Large PR Justification

This PR is one atomic refactor whose line count is dominated by tests, not production code:

• The production change stays modest, and server.go shrinks. 13 non-test files change
  (+555 / −510), and server.go itself is a net reduction (+154 / −312, ≈158 lines
  removed) — the goal is shrinking the server.New god-object to a thin wrapper. The growth
  beyond the bare reroute is the two review-driven changes above (the caching aggregator
  decorator and moving the health monitor into the core), both small per-file deltas.
• The bulk of the diff is tests (+1025 / −636). The behavioral-parity suite had to be
  reworked to exercise the live core.New + Serve path (it previously exercised the dead
  legacy chain), and review feedback added regression tests (Cedar-authz integration,
  late-bound elicitation, nil-aggregator, the construction-guard tests, the caching-decorator
  tests, and the relocated core health-monitor lifecycle tests). CLAUDE.md explicitly allows
  large PRs for test-heavy changes.
• It is indivisible. The reroute flips server.New from the legacy inline path to
  core.New + Serve; a partial application would leave the parity suite red, so the complete
  switch is the smallest reviewable unit.
• It is already split. Per P3.2 Reduce server.New body to the wrapper #5445's recommendation this is PR 1 of 2 (302a); the
  stacked follow-up (302b) removes the remaining now-dead discovery middleware and the factory
  aggregator. Folding 302b in here would make this PR larger, not smaller.

Type of change

• Refactoring (no behavior change)

Test plan

• Unit tests (task test) — full repo suite passes (exit 0)
• Linting (task lint) — clean except a pre-existing, unrelated G115 in cmd/thv/app/upgrade.go (not touched by this PR)
• Integration tests (go test ./test/integration/vmcp/...)
• Race detector + no-cache (go test -race -count=1) across pkg/vmcp/server, pkg/vmcp/core, test/integration/vmcp — no data races

Regression verification

This is the highest-integration-risk PR in the epic: it retires the legacy path and routes
every caller through core.New + Serve, so the acceptance gate is behavioral parity.
The reroute decomposes into four links — wiring (derive*), core domain logic, Serve
transport, and end-to-end — each independently covered. Verification performed for this PR:

• The parity suite now genuinely gates the live path. The pre-existing TestServe* /
  TestIntegration_* server-package suites and the test/integration/vmcp end-to-end suite
  previously exercised the dead-end legacy chain; after the reroute they traverse
  server.New → core.New + Serve for real — tools/list, tools/call, resources, prompts,
  composite workflows, optimizer mode, session lifecycle/termination, token binding, audit
  logging, telemetry, and the real-backend path.
• The moved authorization boundary is covered end-to-end. Authz moved off the HTTP
  AuthzMiddleware onto the core admission seam (Config.Authz). Coverage spans the wiring
  (TestDeriveCoreConfigMapsAllFields asserts the exact *authz.Config reaches the core,
  plus the raw-server-name and nil→allow-all cases), the enforcement (core/admission_test.go
  with a real Cedar authorizer and annotation-gated policies), and the handler mapping
  (TestServeCoreToolHandler genericizes the denial and asserts the authorizer detail never
  leaks; TestServeToolCallTerminatesOnBindingFailure proves the fail-closed binding check).
  New TestIntegration_RealBackend_CedarAuthzGatesToolCall closes the integration gap:
  it boots server.New with a real Cedar policy over HTTP and proves both core gates — the
  list filter (an unpermitted tool is filtered out, unadvertised, and uncallable) and the
  call gate (an advertised tool's arg-gated call is denied with the generic message and no
  detail leak).
• A coverage gap in the new code was found and closed. A function-coverage pass on the
  reroute showed New and buildHealthMonitor at 100% but
  lateBoundElicitationRequester.RequestElicitation (the new elicitation indirection every
  composite elicitation flows through) at 0%. New elicitation_latebound_test.go covers
  its not-bound guard, verbatim delegation, and concurrent bind/read under -race → 100%.
• Blast radius is one in-tree caller (cli/serve.go, whose tests pass). The external
  brood-box embedder is the only out-of-tree caller — see the user-facing-change note below.

Does this introduce a user-facing change?

The Go signature of server.New is unchanged, but its runtime contract changed for
out-of-tree embedders (notably the external brood-box embedder):

• Config.Aggregator is now required — core.New fails without it, because the core is
  the single source of the advertised capability set.
• Config.AuthzMiddleware is now vestigial on the New/Serve path: Serve never
  applies it. Authorization is enforced by the core admission seam from Config.Authz. An
  embedder that previously enforced Cedar authz only via AuthzMiddleware must set
  Config.Authz instead. Setting AuthzMiddleware without Authz is now a construction
  error (server.New returns vmcp.ErrInvalidConfig) rather than a logged warning, so the
  silent allow-all fails fast instead of degrading quietly.
• Config.Authz combined with Config.OptimizerConfig is now rejected at construction
  (vmcp.ErrInvalidConfig): the optimizer meta-tools (find_tool/call_tool) have no Cedar
  representation, so the combination is mutually exclusive per the core admission-seam contract.

In-tree (cli/serve.go) both fields are set and the optimizer is not combined with authz, so
no behavior changes for the CLI or operator. The new construction errors require coordination
with the brood-box maintainers.

Special notes for reviewers

This is the highest-integration-risk PR in the epic: it is the moment the legacy path is
retired and every request flows through the core.New + Serve split for real. The full
behavioral-parity suite is the acceptance gate.

PR 1 of a 2-PR split of #5445. The issue itself recommends the split because the full
change exceeds the 400 LOC / 10 file limit. This PR (302a) reroutes server.New and reworks
the parity suite. The stacked follow-up PR (302b) performs the now-safe dead-code removal —
nothing in 302b is reachable after this PR:

• Physical deletion of the now-dead discovery HTTP middleware and its context seam (the
  authz and annotation-enrichment blocks were already deleted in this PR per review; discovery
  is entangled with the context-coupled router, so it stays for 302b).
• Removing session.WithAggregator and the session factory's aggregator field
  (making the AC2 "factory must not aggregate" contract structural rather than a comment).
• Removing the now-orphaned Aggregator.ProcessPreQueriedCapabilities.
• Dropping cli/serve.go's WithAggregator(agg) factory wiring.

server.Config.AuthzMiddleware is intentionally kept as a vestigial input: the
cli/serve.go assignment stays (set alongside Config.Authz), the HTTP blocks that read it
were removed in this PR, and deriveServerConfig omits it so the Serve path never applies
it. Setting it without Config.Authz is now a construction error (see the user-facing change).

[github-actions]

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

---

This review will be automatically dismissed once you add the justification section.

[codecov]

Codecov Report

❌ Patch coverage is 83.22148% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.92%. Comparing base (cbaa26b) to head (1fec9e4).
⚠️ Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/vmcp/auth/factory/incoming.go	38.46%	6 Missing and 2 partials ⚠️
pkg/vmcp/cli/serve.go	0.00%	5 Missing ⚠️
pkg/vmcp/core/core_vmcp.go	84.37%	3 Missing and 2 partials ⚠️
pkg/vmcp/server/status_reporting.go	0.00%	1 Missing and 3 partials ⚠️
pkg/vmcp/aggregator/caching_aggregator.go	95.00%	1 Missing and 1 partial ⚠️
pkg/vmcp/server/server.go	97.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5556      +/-   ##
==========================================
- Coverage   69.97%   69.92%   -0.05%     
==========================================
  Files         653      660       +7     
  Lines       66489    66805     +316     
==========================================
+ Hits        46524    46713     +189     
- Misses      16586    16724     +138     
+ Partials     3379     3368      -11     

☔ 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 Consensus Review

Agents consulted: architecture-design, security, concurrency-performance, test-coverage, general-code-quality

Consensus Summary

#	Finding	Score	Severity	Action
F1	//nolint:gocyclo on Start() not removed per 302a AC	8/10	MEDIUM	Fix
F2	AuthzMiddleware-without-Authz should be startup error, not WARN	7/10	MEDIUM	Fix
F3	Goroutine leak if core.New succeeds but Serve fails	7/10	MEDIUM	Fix
F5	Optimizer + Config.Authz combination has no guard	7/10	MEDIUM	Fix
F6	No TestNew_NilAggregator_ReturnsError for newly required field	7/10	MEDIUM	Fix

Overall

This PR correctly achieves its primary goal: server.New's body is now a thin composition root over core.New + Serve, and the behavioral-parity suite genuinely gates the live path for the first time. The late-bound elicitation requester is a clean, well-tested solution to the construction-order inversion. The Cedar authz DRY refactor and the new authz_integration_test.go are high-quality additions.

Five MEDIUM findings warrant attention before merge. Two carry security risk: the AuthzMiddleware-without-Authz guard degrades to allow-all with only a log warning (F2), and the Optimizer + Config.Authz combination lacks a construction-time mutual-exclusion check that the admission layer docs require (F5). One is a resource-management correctness issue in an error path: core.New's background state-store goroutine leaks if Serve subsequently fails (F3). One is a missed 302a acceptance-criteria item: the //nolint:gocyclo on (*Server).Start() was listed as a 302a deliverable in the split note but was not removed (F1). One is a missing test for the new required Config.Aggregator field (F6).

The intentionally-deferred 302b items (A1 dead-code cleanup, session.WithAggregator removal, ProcessPreQueriedCapabilities removal) are correctly scoped out. The AdvertiseFromCore: true guard for double-aggregation is the right interim approach, but should be explicitly verified before 302b lands.

F1: //nolint:gocyclo on Start() not removed

The split note for TASK-302a explicitly lists "remove the two //nolint:gocyclo" as a 302a deliverable. The directive on New was correctly removed. The one on (*Server).Start() at server.go:688 was not removed. Run task lint after removing it: if lint passes (the complexity moved to Serve), remove it; if it fires, the AC claim needs updating.

---

Generated with Claude Code

[tgrunnagle]

F1 addressed in f5cfa7b — removed the //nolint:gocyclo on (*Server).Start(). Confirmed task lint passes without it (the complexity moved to Serve).

Large PR justification has been provided. Thank you!

[github-actions]

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

[jerm-dro]

Two blockers (the per-call aggregation below, plus the health-ownership one inline on the monitor wiring), with inline suggestions, a nitpick, and a question. The reroute itself is sound — these are mostly about what flipping to the single live path activates.

blocker: per-call backend re-aggregation on the Serve path

First — apologies we didn't flag this earlier; it's hard to see until the path comes together, since the per-call aggregation only becomes live once server.New routes through the core.

This PR makes core.New + Serve the single live path, changing backend discovery from once-per-session to once-per-call: coreToolHandler (serve_handlers.go:196) → core.CallTool (core_calls.go:40) → aggregatedView → AggregateCapabilities → QueryAllCapabilities, a full tools/list sweep of every backend on every tools/call (and resources/read). The legacy path swept once at initialize and routed calls through the session's cached routing table, so a session with M calls goes from ~1 sweep to ~M+1. The result-only parity suite won't surface this.

Possible solution: a transparent caching decorator wrapping the aggregator.Aggregator, applied at composition below the core — AggregateCapabilities serves a cached view on hit and only sweeps backends on miss/TTL-expiry. Core and Serve stay unchanged; no loopback to the server, and it needn't be session-scoped. Backend enumeration is identity-dependent, so the cache key MUST include identity — at the granularity that actually drives enumeration (subject + forwarded scopes/credential, not just the backend set). A backend-set-only key would serve one user's capability view to another. Keyed that way it is shared across a single user's sessions without leaking across users; a TTL bounds staleness, matching legacy's once-per-session freshness. (This memoizes the per-identity enumeration the call path already does correctly — same behavior, without the M+1 backend cost.)

nitpick: prefer deleting now-dead code in the same PR

Now that this PR routes every caller through Serve, the authz/annotation blocks in (*Server).Handler (~server.go:592) are dead — deriveServerConfig drops AuthzMiddleware, so the guard is always false. I would generally find it clearer to delete code in the same PR where it becomes unused, rather than leaving an always-false guard on main that reads as load-bearing until 302b (you already did this for New's own orphaned fields / validateWorkflows). Pure deletions of provably-dead code are cheap to review, so folding it in carries near-zero risk. Keeping 302a behavioral and 302b mechanical-deletion is a defensible call too — flagging the preference for next time, not asking to re-cut the split.

[tgrunnagle]

Per-call re-aggregation (body blocker) addressed in b8dcc8c — added aggregator.NewCachingAggregator, a transparent decorator wrapping the Aggregator below the core. It memoizes AggregateCapabilities for a TTL (30s), keyed on a SHA-256 of (subject, forwarded headers, backend IDs) so one caller's capability view is never served to another; errors aren't cached; expired entries evict on miss. server.New wraps Config.Aggregator with it. Core and Serve unchanged.

[tgrunnagle]

Dead authz/annotation Handler blocks (nitpick) folded into 1591269 — deleted both AuthzMiddleware != nil blocks in (*Server).Handler and the now-orphaned annotation_enrichment.go. (Backend-enrichment + discovery remain for 302b: the former is a guarded no-op rather than always-false dead code, and discovery is entangled with the context-coupled router.)

[jerm-dro]

Approving. Non-blocking follow-up: the entire s.core == nil legacy path is now unreachable dead code — Serve is the sole *Server constructor and always sets the core — so 302b should delete all of it (discovery middleware, backend_enrichment, GetAdaptedTools, and the session-factory aggregation/composite/telemetry mirrors), not just the annotation_enrichment removed in this PR.