stacklok / stacklok/toolhive

Phase 3: Reduce server.New to wrapper + config split

Open
#5,432 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

refactor vmcp
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Description

Land the structural payoff of the vMCP domain/transport split: split the in-memory
server.Config into a core Config + a transport ServerConfig (via
deriveCoreConfig/deriveServerConfig), then reduce server.New's body to the
thin wrapper Serve(ctx, New(deriveCoreConfig(cfg)), deriveServerConfig(cfg))
removing both //nolint:gocyclo as the god-object dismantles. This story maps to
RFC Phase 3 and is the only phase that touches server.New's body; the
7-param signature and observable behavior stay byte-for-byte stable while the
implementation behind them collapses onto the New/Serve seam built in Phases 1–2.

Context

See RFC THV-0076: vMCP Core Interface
for full design details. Part of the vMCP interface refactor (epic #5419).

By the time this story runs, Phase 1 has produced the VMCP interface +
New(cfg) -> VMCP core (with the admission and elicitation seams) and Phase 2 has
produced Serve(ctx, VMCP, *ServerConfig) -> *Server with all transport concerns
(mcp-go server, SDK hooks, two-phase session creation, the full middleware chain,
AS runner, status reporter, optimizer, health monitor) already re-homed under it —
but Serve is not yet called by server.New. This story closes that loop. Per
architecture.md ("PR-Sized Decomposition Guidance → Phase 3" and "Key Files to
Modify"), the work is the config decomposition plus the wrapper reduction at
pkg/vmcp/server/server.go:301.

The config split is not a clean partition (R3): per architecture.md
"Constraints", the cross-cutting fields TelemetryProvider, AuditConfig, and the
health view are consumed on both sides (the core decorates the backend client
with telemetry and runs the workflow auditor; the transport adds telemetry/audit
middleware and lifecycles the health monitor). The health monitor is built at the
composition root
(A2): because New runs before Serve, the wrapper builds it once,
injects its StatusProvider into New, and hands the built *health.Monitor to Serve.
ServerConfig carries no AuthzMiddleware (authz lives in the core admission seam);
the AuthzMiddleware field on server.Config is kept vestigialcli/serve.go
sets it and stays unchanged — and only the dead HTTP authz/annotation blocks (plus the
discovery middleware/seam) are deleted in #5445 once the legacy path is gone. This is an
in-memory-only change:
vmcpconfig.Config, the CRD/YAML model, and the wire/storage format are unchanged
inputs to New.

This phase is small but high-stakes: it is gated by the full behavioral-parity
suite
(driving the stable server.New wrapper) plus the unchanged
thv vmcp serve E2E suite — the only safety net proving the reimplemented body is
observably identical.

RFC Phase(s): Phase 3
Dependencies: #5431

Scope

In scope

  • Split the in-memory-only server.Config (server.go:92-185) into the core
    Config (collaborators + workflowDefs + Authz for the admission seam +
    cross-cutting TelemetryProvider/AuditConfig) and a transport-only
    ServerConfig, exposed via deriveCoreConfig(cfg, …) and deriveServerConfig(cfg)
    (#5444).
  • Pass the cross-cutting fields (TelemetryProvider, AuditConfig) to both New
    and Serve (R3 — not a clean partition); build the health monitor at the composition
    root and thread its StatusProvider into New + the built *Monitor into Serve
    (A2). ServerConfig omits AuthzMiddleware; the field on server.Config is kept
    vestigial
    (cli/serve.go unchanged) (#5444).
  • Reduce server.New's body to
    Serve(ctx, New(deriveCoreConfig(cfg, …)), deriveServerConfig(cfg, healthMon)), keeping
    the 7-param signature byte-for-byte unchanged, and remove both //nolint:gocyclo
    (New @300, Start @682). With the legacy path now gone, complete the A1-deferred
    cleanup
    : delete the now-dead authz/annotation HTTP blocks and retire the discovery
    middleware/seam + its s.core == nil guard (keeping convertAnnotations and the
    vestigial AuthzMiddleware field). Highest-integration-risk PR; likely splits (#5445).

Out of scope

  • Defining or moving any transport concern under Serve, or the VMCP/New core
    itself — all of that lands in #5430 (Phase 1) and #5431 (Phase 2);
    this story only derives configs and wires the existing New/Serve together.
  • Any change to server.New's signature — it is stable throughout the epic; only
    the body is reimplemented.
  • Any change to pkg/vmcp/cli/serve.go (the composition root) — it must require
    no changes and keep calling vmcpserver.New(...) as today.
  • Any change to the CRD / YAML model or the vmcpconfig.Config loaders / wire /
    storage format — the split is in-memory only.
  • Docs (docs/arch/vmcp-library.md, pkg/vmcp/doc.go) and the runnable decorator
    example — #5433 (Phase 4).

Child Tasks

PR-sized tasks under this story (each ≤ 400 LOC, ≤ 10 files changed excluding
tests/docs/generated, one logical change):

  • #5444: P3.1 deriveCoreConfig/deriveServerConfig config split
  • #5445: P3.2 Reduce server.New body to the wrapper

Acceptance Criteria

  • All child tasks complete and merged
  • server.New's 7-param signature and observable behavior are unchanged
    (byte-for-byte signature; clients/embedders see no behavioral difference)
  • deriveCoreConfig/deriveServerConfig derive the core Config and the
    transport ServerConfig from the existing in-memory server.Config, with the
    cross-cutting TelemetryProvider/AuditConfig passed to both sides, the health
    monitor built at the composition root and threaded both ways (A2), and ServerConfig
    omitting AuthzMiddleware (the server.Config field is kept vestigial; cli/serve.go
    unchanged) (R3)
  • server.New's body is reduced to
    Serve(ctx, New(deriveCoreConfig(cfg, …)), deriveServerConfig(cfg, healthMon)), both
    //nolint:gocyclo (New @300, Start @682) are removed with lint clean, and the
    A1-deferred cleanup is completed (dead authz/annotation blocks + discovery middleware/seam
    deleted; convertAnnotations and the vestigial AuthzMiddleware field retained)
  • pkg/vmcp/cli/serve.go is unchanged; no in-repo caller (nor the external
    brood-box embedder) requires any change
  • No serialized / wire / CRD / YAML / storage format change — the split is
    in-memory only and vmcpconfig.Config is an unchanged input to New
  • 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, and the existing thv vmcp serve
    E2E suite passes unchanged

References

  • RFC THV-0076 (link above)
  • Epic: #5419

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the Phase 3 guidance in architecture.md and the RFC, then inspect pkg/vmcp/server/server.go around Config and server.New, along with dependencies #5431, #5444, and #5445. Run the full behavioral-parity suite and the existing thv vmcp serve E2E suite before changing anything. Done means the stable New signature and behavior remain unchanged, cli/serve.go is untouched, and both suites pass after the config split and wrapper integration.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.