P3.2 Reduce server.New body to the wrapper

[tgrunnagle]

Description

Reduce server.New's body to the thin wrapper
Serve(ctx, New(deriveCoreConfig(cfg, …)), deriveServerConfig(cfg, healthMon)), keeping
the existing 7-param signature byte-for-byte unchanged. This is the moment the
god-object server.New (anti-pattern #3) is dismantled and the single live path
becomes the New/Serve split — so it is also where the A1-deferred cleanup lands:
build the health monitor at the composition root (A2), then delete the now-dead
authz/annotation HTTP blocks and retire the discovery middleware/seam + its s.core == nil guard that Phase 2 left in place behind guards, **and delete session.WithAggregator

• the factory aggregator field** so the AC2 "factory must not aggregate" contract (P2.4 Replace discovery-into-context with direct VMCP calls #5442)
  becomes structural rather than a remembered comment. Both //nolint:gocyclo directives
  are removed (the complexity is gone, not suppressed). This is the highest-integration-risk
  PR in the epic: until now the behavioral-parity suite exercised the legacy path; here
  server.New is rerouted through Serve for real, so the full parity suite is the gate.

Context

This is the final code change of Phase 3 (RFC THV-0076). #5444 introduced
deriveCoreConfig/deriveServerConfig, which split the in-memory server.Config
into the core Config and transport ServerConfig (and threaded the built health
monitor). This task wires those derivers into server.New so its body delegates entirely
to New + Serve. Per architecture.md "server.New compatibility wrapper" (14-17, 43-47,
368, 480-482, 501), the 7-param signature and observable MCP behavior stay stable
throughout — the server.New(...) call site (cli/serve.go:421) and the external
brood-box embedder require no changes. (The one cli/serve.go edit this task makes is
dropping the factory's WithAggregator(agg) wiring — see "Factory aggregator removal
(AC2 made structural)" below.)

Why the A1 cleanup lands here. Phase 2 deliberately did not delete the
authz/annotation middleware (#5441) or the discovery middleware (#5442) from the
shared (*Server).Handler, because server.New still used the legacy chain and deleting
shared middleware would have opened an authz/aggregation regression window. Instead, Phase
2 made those layers inert on the Serve path (authz/annotation via a nil
AuthzMiddleware; discovery via a s.core == nil guard). Once this task routes
server.New through Serve, the legacy path is gone — every request flows through a
Serve-built *Server (core present, AuthzMiddleware nil) — so those layers are now
dead on all paths and can be safely deleted, completing the "shrink the middleware
chain" goal (anti-patterns #1/#4).

The AuthzMiddleware field stays. cli/serve.go (serve.go:~374) still assigns
Config.AuthzMiddleware, and the "cli/serve.go unchanged" constraint forbids editing that
assignment. So the field is kept as a vestigial input; only the HTTP blocks that
read it are deleted, and deriveServerConfig already omits it so the Serve path never
applies it. The core admission seam enforces authz from the Authz config instead.

Factory aggregator removal (AC2 made structural). #5442 established that on the Serve
path the core is the single aggregator and the session factory MUST NOT aggregate, but
left this as an unenforced contract (documented on ServerConfig.SessionManagerConfig)
because no production composition root wired Serve yet. This task is where it goes live.
cli/serve.go:352 builds the session factory with WithAggregator(agg) and threads it
in via cfg.SessionFactory → deriveServerConfig → Serve; once server.New routes through
Serve, the factory and the core would both aggregate — double-aggregation and drift,
exactly the AC2 violation #5442 guarded against. So this task deletes session.WithAggregator
and the factory's aggregator field outright (per PR #5491 review feedback), turning the
contract from a comment people must remember into a deletion that makes the violation
structurally impossible. The agg aggregator itself is kept — it still feeds the core via
deriveCoreConfig; only its wiring into the factory is removed.

Health monitor (A2). Because the wrapper is Serve(ctx, New(deriveCoreConfig(cfg)), deriveServerConfig(cfg)), New is evaluated before Serve. The wrapper therefore builds
the *health.Monitor at the composition root (mirroring the old server.go:436-450
construction) and threads it both ways: as a health.StatusProvider into deriveCoreConfig
(so the core can filterHealthyBackends) and as the built *health.Monitor into
deriveServerConfig (so Serve owns its Start/Stop lifecycle, #5443).

Because the body is fully reimplemented behind a stable surface, the full behavioral-parity
suite is the acceptance gate (research.md / architecture.md "Behavioral parity /
integration", 468-473).

Parent Story: #[#5432-issue]
Dependencies: #[#5444-issue] (#5444 — deriveCoreConfig/deriveServerConfig config split)
Blocks: #5446

Split note: this PR likely exceeds the 400 LOC / 10 file limit because it both
reroutes server.New and removes a swath of now-dead middleware. Recommended split:

• TASK-302a — build the monitor at the root + reduce server.New's body to
  Serve(ctx, New(deriveCoreConfig(...)), deriveServerConfig(..., healthMon)) + remove the
  two //nolint:gocyclo. After this, the legacy path is gone (everything routes through
  Serve); the dead authz/annotation/discovery blocks remain but are unreachable.
• TASK-302b — delete the now-dead authz block (606-609), annotation-enrichment block
  (611-617) + AnnotationEnrichmentMiddleware (keep convertAnnotations, reused by the
  core seam), the discovery middleware + handleSubsequentRequest + WithDiscoveredCapabilities
  context seam, and the s.core == nil discovery guard. **Also delete session.WithAggregator
  • the factory aggregator field** (plus buildRoutingTableWithAggregator, the
    f.aggregator != nil branch, and the now-orphaned Aggregator.ProcessPreQueriedCapabilities),
    and drop the WithAggregator(agg) wiring at cli/serve.go:352. Mostly dead-code removal;
    compiles and passes parity on its own.

Both keep the AuthzMiddleware field; the only cli/serve.go edit is dropping the
WithAggregator(agg) factory wiring (302b). Use the /split-pr skill.

Acceptance Criteria

• server.New body is Serve(ctx, New(deriveCoreConfig(cfg, …)), deriveServerConfig(cfg, healthMon)); the transport/core wiring no longer lives inline in server.New.
• The 7-param New(ctx, cfg, rt, backendClient, discoveryMgr, backendRegistry, workflowDefs) (*Server, error) signature is byte-for-byte unchanged.
• Health monitor (A2): the wrapper builds the *health.Monitor at the composition root (nil when HealthMonitorConfig is nil), injects its StatusProvider into New via deriveCoreConfig, and passes the same *health.Monitor into Serve via deriveServerConfig. The core filters; Serve lifecycles (P2.5 Move AS runner, status reporter, optimizer, health monitor under Serve #5443).
• A1 cleanup (now safe — legacy path gone): the now-dead authz block (server.go:606-609), the AnnotationEnrichmentMiddleware block (611-617) and middleware symbol, the discovery middleware + handleSubsequentRequest routing injection + the WithDiscoveredCapabilities/DiscoveredCapabilitiesFromContext context seam, and the s.core == nil discovery guard are deleted. convertAnnotations (annotation_enrichment.go:92) is retained (reused by the core admission seam).
• Factory never aggregates (AC2 made structural): session.WithAggregator (factory.go:147) and the factory aggregator field (factory.go:118) are deleted, along with buildRoutingTableWithAggregator (factory.go:264) and the if f.aggregator != nil branch (factory.go:419-422) — makeBaseSession always uses the raw buildRoutingTable. The composition root's aggregator→factory wiring (createSessionFactory(..., agg) / WithAggregator(agg), cli/serve.go:352, :656-664) is dropped (the agg aggregator still feeds the core via deriveCoreConfig). Aggregator.ProcessPreQueriedCapabilities (aggregator.go:78, impl default_aggregator.go:501) — whose only caller was the factory — is removed. Without this, routing server.New through Serve would double-aggregate (factory + core), the AC2 violation P2.4 Replace discovery-into-context with direct VMCP calls #5442 deferred.
• The AuthzMiddleware field on server.Config is kept (vestigial); cli/serve.go still sets it (that assignment is unchanged). Only the HTTP blocks that read it are removed.
• Both //nolint:gocyclo directives are removed — New (server.go:300) and Start (server.go:682) — and lint is clean without them (task lint-fix).
• cli/serve.go's server.New(...) call (line 421) and AuthzMiddleware assignment (~374) are unchanged; the only cli/serve.go edit is dropping the factory's WithAggregator(agg) wiring (line 352 / createSessionFactory), required by the factory-aggregator removal. The external brood-box embedder requires no changes.
• Acceptance gate: the full behavioral-parity suite (tools/list, tools/call, resources, prompts, composite workflows, session lifecycle, cross-pod Redis paths) passes equivalently before/after — this is the first PR where server.New actually runs through Serve, so it carries the epic's concentrated integration risk.
• Existing thv vmcp serve E2E suite (test/e2e/vmcp_cli_*test.go, chainsaw operator scenarios) passes unchanged.
• PR (or each split PR) is ≤ 400 LOC and ≤ 10 files changed (excluding tests/docs/generated)
• server.New signature and observable behavior unchanged
• All tests pass (task test); lint clean (task lint-fix)
• Code reviewed and approved

Technical Approach

Recommended Implementation

Replace the inline body of server.New with a delegation to the two constructors
relocated in Phases 1–2, building the health monitor at the composition root first:

1. healthMon, err := <build monitor> — mirror server.go:436-450 (nil HealthMonitorConfig
   ⇒ nil monitor / disabled; otherwise health.NewMonitor(backendClient, backendRegistry.List(ctx), *cfg.HealthMonitorConfig)). This runs at the composition root because New (next) needs the
   StatusProvider at construction (A2).
2. core, err := New(deriveCoreConfig(cfg, rt, backendClient, discoveryMgr, backendRegistry, workflowDefs, healthMon))
   — assembles the core VMCP (aggregator/router/backend registry/composer/admission seam/
   elicitation) with healthMon injected as the StatusProvider (architecture.md "New (core)
   wiring relocated", 278-284).
3. return Serve(ctx, core, deriveServerConfig(cfg, healthMon)) — assembles the transport
   *Server (mcp-go server/hooks, streamable HTTP, session manager, middleware chain, AS
   runner, status reporter) and owns healthMon's lifecycle (architecture.md "Serve (transport)
   wiring relocated", 285-290; P2.5 Move AS runner, status reporter, optimizer, health monitor under Serve #5443).

Defaults currently applied at the top of server.New (Host/EndpointPath/Name/Version/
SessionTTL, server.go:310-327) must continue to be honored — fold them into the deriver path
or keep them in the wrapper so the derived configs carry the same resolved values.
Cross-cutting fields (TelemetryProvider, AuditConfig, the health monitor) reach both
New and Serve (R3; architecture.md 483-487).

A1 cleanup (safe now that the legacy path is gone). With server.New routed through
Serve, every *Server has s.core != nil and AuthzMiddleware == nil, so the guarded
layers are unreachable. Delete them:

• the authz block (server.go:606-609) and the AnnotationEnrichmentMiddleware block
  (server.go:611-617); remove the AnnotationEnrichmentMiddleware symbol but keep
  convertAnnotations (annotation_enrichment.go:92, reused by the core admission seam);
• the discovery middleware (the application site at server.go:633-637 and the
  s.core == nil guard P2.4 Replace discovery-into-context with direct VMCP calls #5442 added), handleSubsequentRequest
  (discovery/middleware.go:273-315), and the WithDiscoveredCapabilities /
  DiscoveredCapabilitiesFromContext seam (discovery/context.go:26,35);
• update the chain's leading comment (server.go:574-579) to the post-cleanup order.

Do not remove the AuthzMiddleware field from server.Config (cli/serve.go sets it;
the field is now vestigial, the new path ignores it). The Start //nolint:gocyclo is
removed because the orchestration complexity it suppressed moved into the decomposed Serve.

Factory aggregator removal (AC2 made structural). Because server.New now routes through
Serve, the cli-built session factory reaches the Serve path; an aggregating factory would
double-aggregate against the core. Delete session.WithAggregator (factory.go:147) and the
aggregator field (factory.go:118); delete buildRoutingTableWithAggregator (factory.go:264)
and the if f.aggregator != nil branch (factory.go:419-422) so makeBaseSession always uses
the raw buildRoutingTable. Drop the WithAggregator(agg) wiring at the composition root
(cli/serve.go:352 / createSessionFactory, :656-664) — keep agg, which still feeds the
core via deriveCoreConfig. Aggregator.ProcessPreQueriedCapabilities (aggregator.go:78,
default_aggregator.go:501) loses its only caller and is removed.

Patterns & Frameworks

• mcp-go SDK boundary stays inside Serve/adapter — no mcp-go types cross the VMCP boundary (core principle fix(typo): corrects readme #1; anti-pattern Implement secret injection #5). server.New must not reintroduce any mcp-go wiring inline.
• Thin orchestrator over pre-built subsystems — server.New becomes the textbook fix for the God-Object anti-pattern (Bump golangci/golangci-lint-action from 2f856675483cb8b9378ee77ee0beb67955aca9d7 to 4696ba8babb6127d732c3c6dde519db15edab9ea #3): a thin shim composing New + Serve, not a 380-line constructor.
• Delete only what is dead — the authz/annotation/discovery removals are valid only because routing through Serve eliminates the legacy path; keep convertAnnotations and the AuthzMiddleware field (still referenced by cli/serve.go).
• Stable interface, additive change — pkg/vmcp/server is Stable (vmcp-library.md table); the signature does not move (core principle Start simple hardcoded registry #8). Do not widen or alter it.
• Coding conventions: .claude/rules/go-style.md (immutable assignment, copy-before-mutate, no //nolint unless a confirmed false positive — here we are removing suppressions), .claude/rules/vmcp-anti-patterns.md (fix(typo): corrects readme #1 context coupling, Bump golangci/golangci-lint-action from 2f856675483cb8b9378ee77ee0beb67955aca9d7 to 4696ba8babb6127d732c3c6dde519db15edab9ea #3 God Object, Implement secret store #4 middleware overuse, Implement secret injection #5 SDK coupling, Figure out ergonomics for exposing directories #6 config-everywhere). Run the vmcp-review skill on the PR.

Code Pointers

• pkg/vmcp/server/server.go:301 — server.New(...). Signature unchanged; body becomes the wrapper. The default-application block (310-327) and all inline transport/core wiring below it are replaced by the build-monitor + Serve(ctx, New(deriveCoreConfig(...)), deriveServerConfig(..., healthMon)) delegation.
• pkg/vmcp/server/server.go:436-450 — health monitor construction; relocated into the wrapper (composition root), its StatusProvider injected into New (A2).
• pkg/vmcp/server/server.go:300 — //nolint:gocyclo on New. Remove.
• pkg/vmcp/server/server.go:682 — //nolint:gocyclo on (*Server).Start (server.go:683). Remove.
• pkg/vmcp/server/server.go:606-609, :611-617 — the now-dead authz + annotation-enrichment blocks. Delete (legacy path gone). Keep convertAnnotations (annotation_enrichment.go:92).
• pkg/vmcp/server/server.go:633-637 + the s.core == nil guard from P2.4 Replace discovery-into-context with direct VMCP calls #5442 — the discovery-middleware application. Delete.
• pkg/vmcp/discovery/middleware.go:273-315 (handleSubsequentRequest), discovery/context.go:26,35 (WithDiscoveredCapabilities/DiscoveredCapabilitiesFromContext) — the context seam. Delete/retire.
• pkg/vmcp/server/server.go:121-125 — AuthzMiddleware field on server.Config. Keep (vestigial; cli/serve.go sets it). Do not remove.
• pkg/vmcp/session/factory.go:118,147,264,419-422 — the aggregator field, WithAggregator option, buildRoutingTableWithAggregator, and the if f.aggregator != nil branch. Delete — the factory never aggregates once the core is the single aggregator (AC2 made structural).
• pkg/vmcp/aggregator/aggregator.go:78, pkg/vmcp/aggregator/default_aggregator.go:501 — Aggregator.ProcessPreQueriedCapabilities. Its only caller was the factory; remove once the factory aggregator is gone.
• pkg/vmcp/cli/serve.go:352, :656-664 — createSessionFactory(outgoingRegistry, agg) + the WithAggregator(agg) append. Remove the aggregator→factory wiring (keep agg; it still feeds the core via deriveCoreConfig). This is the one cli/serve.go edit the task makes.
• pkg/vmcp/cli/serve.go:382-421 — composition root; calls vmcpserver.New(...) at line 421 and assigns AuthzMiddleware at ~374. These require no changes (only the factory WithAggregator wiring at line 352 is removed, above).
• deriveCoreConfig / deriveServerConfig — introduced by P3.1 deriveCoreConfig/deriveServerConfig config split #5444 (dependency); this task consumes them (deriveServerConfig now takes the built healthMon).
• architecture.md "Key Files to Modify" (43-47) and "server.New compatibility wrapper" (14-17, 368, 480-482, 501).
• Parity suite homes (mirror existing cases; do not rewrite): pkg/vmcp/server/server_test.go, pkg/vmcp/server/integration_test.go, and the reusable harness test/integration/vmcp/helpers/vmcp_server.go.

Component Interfaces

// Signature byte-for-byte unchanged; only the body changes.
// The two //nolint:gocyclo directives (New @300, Start @682) are removed.
func New(
    ctx context.Context,
    cfg *Config,
    rt router.Router,
    backendClient vmcp.BackendClient,
    discoveryMgr discovery.Manager,
    backendRegistry vmcp.BackendRegistry,
    workflowDefs map[string]*composer.WorkflowDefinition,
) (*Server, error) {
    // defaults honored as today (via the derivers/wrapper).
    healthMon, err := buildHealthMonitor(ctx, cfg, backendClient, backendRegistry) // nil => disabled (A2)
    if err != nil {
        return nil, err
    }
    core, err := New(deriveCoreConfig(cfg, rt, backendClient, discoveryMgr, backendRegistry, workflowDefs, healthMon))
    if err != nil {
        return nil, err
    }
    return Serve(ctx, core, deriveServerConfig(cfg, healthMon))
}

Testing Strategy

Unit Tests

• server.New still applies Host/EndpointPath/Name/Version/SessionTTL defaults (server.go:310-327) — same resolved values reach the derived Config/ServerConfig.
• The wrapper builds the health monitor once and injects the same instance as the core's StatusProvider and Serve's lifecycle *Monitor (A2); nil HealthMonitorConfig ⇒ disabled on both sides.
• server.New propagates the error (and returns nil *Server) when monitor build, the core New, or Serve fails.
• Lint passes with both //nolint:gocyclo removed (gocyclo no longer triggers on New or Start).
• After cleanup, the middleware chain no longer contains authz/annotation/discovery layers; convertAnnotations and the AuthzMiddleware field still compile (the field is set by cli/serve.go, read by nothing).
• The session factory no longer exposes WithAggregator (compile-time) and makeBaseSession always uses the raw buildRoutingTable; a Serve-path session aggregates exactly once (in the core) and the advertised set matches the core's ListTools output — no factory re-aggregation, no drift.

Integration / Behavioral Parity Tests

• Drive the stable server.New wrapper and assert MCP responses equivalent before/after for: tools/list, tools/call, resources, prompts, and composite workflows (homes: server_test.go, integration_test.go, helpers/vmcp_server.go).
• Authz parity is critical here — this is the first PR where server.New enforces authz via the core admission seam instead of the HTTP middleware; assert allow/deny outcomes for tools/list filtering and tools/call denial match pre-refactor behavior (cross-check the R1 suite from P1.5 Core admission seam (bounded rewrite) #5438).
• Session lifecycle parity: "fixed at initialize" capability set, bound-session / identity-binding, two-phase creation unchanged.
• Cross-pod Redis parity: session rehydration / lazyInjectSessionTools path produces equivalent behavior.
• E2E: thv vmcp serve suite (test/e2e/vmcp_cli_*test.go) and chainsaw operator scenarios pass unchanged.

Edge Cases

• nil-identity / anonymous request semantics unchanged through the wrapper.
• No-authz-configured path remains allow-all (no admission regression) — now via the core seam, not the HTTP middleware.
• Typed-nil Watcher handling (cli/serve.go:404-409) is unaffected by the wrapper.

Out of Scope

• Any change to server.New's signature, parameter order, or types (stays byte-for-byte).
• Any change to cli/serve.go beyond dropping the factory's WithAggregator(agg) wiring (line 352), or any change to the external brood-box embedder — including removing the now-vestigial AuthzMiddleware assignment (the field is kept).
• Any change to vmcpconfig.Config / CRD / YAML / wire / storage format (in-memory only).
• Introducing deriveCoreConfig/deriveServerConfig themselves — that is P3.1 deriveCoreConfig/deriveServerConfig config split #5444.
• Documentation/doc.go updates and the decorator example — those are P4.1 Update vMCP architecture docs + doc.go #5446 / P4.2 Runnable decorator example embedder #5447.
• Removing the server.New wrapper or deprecating it — it is retained as a stable shim.

References

• RFC THV-0076: vMCP Core Interface — Splitting Domain Logic from Transport
• Parent story: #[Phase 3: Reduce server.New to wrapper + config split #5432-issue]
• Epic: vMCP interface refactor #5419