diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index c26061c974..88e81ded57 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -398,7 +398,12 @@ func Serve(ctx context.Context, cfg ServeConfig) error { }() } - serverCfg := &vmcpserver.Config{ + // Resolve transport defaults once here at the composition root: the + // vMCP config edge is the single place flags/CRD/YAML become a fully-resolved + // Config, so server.New, Serve, and the derive* helpers downstream are pure + // pass-through. WithDefaults fills any unset Host/EndpointPath/SessionTTL/Name/ + // Version (EndpointPath in particular is never set by the CLI). + serverCfg := vmcpserver.WithDefaults(&vmcpserver.Config{ Name: vmcpCfg.Name, Version: versions.Version, GroupRef: vmcpCfg.Group, @@ -419,7 +424,7 @@ func Serve(ctx context.Context, cfg ServeConfig) error { OptimizerConfig: optCfg, SessionFactory: sessionFactory, SessionStorage: vmcpCfg.SessionStorage, - } + }) // Assign Watcher only when backendWatcher is non-nil. A typed nil // *k8s.BackendWatcher assigned to the Watcher interface produces a diff --git a/pkg/vmcp/server/derive.go b/pkg/vmcp/server/derive.go new file mode 100644 index 0000000000..6b304f2b45 --- /dev/null +++ b/pkg/vmcp/server/derive.go @@ -0,0 +1,148 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "cmp" + + "github.com/stacklok/toolhive/pkg/authz" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/aggregator" + "github.com/stacklok/toolhive/pkg/vmcp/composer" + "github.com/stacklok/toolhive/pkg/vmcp/core" + "github.com/stacklok/toolhive/pkg/vmcp/health" + "github.com/stacklok/toolhive/pkg/vmcp/router" + "github.com/stacklok/toolhive/pkg/vmcp/server/sessionmanager" +) + +// WithDefaults returns a copy of cfg with the transport defaults applied to any field +// left unset: Name, Version, Host, EndpointPath, and SessionTTL. It is the single place +// these defaults are defined. The composition root (cli) — and any test that builds a +// Config by hand — resolves its Config through WithDefaults before handing it to New, so +// New, Serve, buildServeConfig, and the derive* helpers below can all treat their input as +// already-resolved pass-through (default once at the edge, not per-constructor). +// +// Port 0 is left untouched — it means "let the OS assign a random port" (the CLI flag +// supplies the production default of 4483). +// +// cfg is treated as read-only: a shallow copy is returned and the caller's value is not +// mutated (go-style: copy before mutating caller input). +func WithDefaults(cfg *Config) *Config { + resolved := *cfg + resolved.Name = cmp.Or(resolved.Name, defaultServerName) + resolved.Version = cmp.Or(resolved.Version, defaultServerVersion) + resolved.Host = cmp.Or(resolved.Host, defaultHost) + resolved.EndpointPath = cmp.Or(resolved.EndpointPath, defaultEndpointPath) + resolved.SessionTTL = cmp.Or(resolved.SessionTTL, defaultSessionTTL) + return &resolved +} + +// deriveServerConfig projects the transport-only configuration out of the legacy, +// in-memory server.Config onto the ServerConfig that Serve consumes. It is the +// transport half of the Phase 3 config decomposition (#5444); the composition root +// (#5445) will call it when server.New is reduced to a wrapper over +// Serve(ctx, core.New(deriveCoreConfig(cfg, …)), deriveServerConfig(cfg, …)). +// +// cfg is treated as read-only (go-style: copy before mutating caller input); a fresh +// ServerConfig is returned. This is a pure projection: transport defaults are resolved +// once at the composition root via WithDefaults, so cfg already holds resolved +// values and deriveServerConfig applies no defaulting of its own. Port 0 still means +// "OS-assigned". +// +// Three collaborators the transport needs but server.Config does not carry directly are +// passed in by the composition root rather than reached out of cfg: +// - healthMon: the *health.Monitor built at the composition root (A2). Serve owns only +// its Start/Stop; the same instance's StatusProvider is injected into the core via +// deriveCoreConfig. Nil disables health monitoring. +// - backendRegistry: the shared backend registry (also passed to deriveCoreConfig); +// the Serve session layer enumerates it when opening a session's connections. +// - sessionManagerConfig: the pre-assembled *sessionmanager.FactoryConfig. The legacy +// SessionFactory/OptimizerFactory/OptimizerConfig fields fold into it (#5440), and its +// ComposerFactory closes over the core-owned router/backend client, so only the +// composition root can build it. +// +// AuthzMiddleware is intentionally NOT projected: ServerConfig has no such field — +// authorization moved to the core admission seam (#5438). The AuthzMiddleware field on +// server.Config is kept as a vestigial input the Serve path ignores (cli/serve.go still +// sets it and must stay unchanged); only the dead HTTP authz/annotation blocks that read +// it are removed later (#5445). +func deriveServerConfig( + cfg *Config, + healthMon *health.Monitor, + backendRegistry vmcp.BackendRegistry, + sessionManagerConfig *sessionmanager.FactoryConfig, +) *ServerConfig { + return &ServerConfig{ + Name: cfg.Name, + Version: cfg.Version, + GroupRef: cfg.GroupRef, + Host: cfg.Host, + Port: cfg.Port, // 0 means "OS-assigned". + EndpointPath: cfg.EndpointPath, + SessionTTL: cfg.SessionTTL, + AuthMiddleware: cfg.AuthMiddleware, + RateLimitMiddleware: cfg.RateLimitMiddleware, + AuthInfoHandler: cfg.AuthInfoHandler, + AuthServer: cfg.AuthServer, + HealthMonitor: healthMon, + StatusReportingInterval: cfg.StatusReportingInterval, + StatusReporter: cfg.StatusReporter, + Watcher: cfg.Watcher, + BackendRegistry: backendRegistry, + SessionStorage: cfg.SessionStorage, + SessionManagerConfig: sessionManagerConfig, + // Cross-cutting (also on core.Config) — R3, not a clean partition: + TelemetryProvider: cfg.TelemetryProvider, + AuditConfig: cfg.AuditConfig, + } +} + +// deriveCoreConfig assembles the core.Config that core.New consumes from the legacy +// server.Config plus the collaborators the composition root builds. It is the core half +// of the Phase 3 config decomposition (#5444); #5445 will call it inside the reduced +// server.New as core.New(deriveCoreConfig(cfg, …)). +// +// cfg is treated as read-only; only its cross-cutting fields are read: +// - ServerName = cfg.Name — the Cedar resource entity name, matching the serverName the +// legacy HTTP authz path threads through (cli/serve.go passes vmcpCfg.Name). The raw +// name is used (not the transport default) so authorization keys on the real +// VirtualMCPServer name rather than the synthetic "toolhive-vmcp" fallback. +// - TelemetryProvider / AuditConfig — the cross-cutting fields shared with the transport +// (R3, not a clean partition); deriveServerConfig copies the same two. +// +// Everything else is a collaborator passed through rather than reached out of cfg: the +// aggregator, router, backend client, backend registry, and workflow definitions that +// arrive as separate server.New parameters today; the authz config that feeds the +// admission seam (server.Config carries only the pre-built AuthzMiddleware, never the +// authz.Config, so it cannot come from cfg); the elicitation requester; and the health +// StatusProvider — the read-only view of the same *health.Monitor deriveServerConfig +// places on ServerConfig.HealthMonitor (A2). A nil healthProvider means no health +// filtering; a nil authzCfg means allow-all (matching today's AuthzMiddleware != nil +// guard). +func deriveCoreConfig( + cfg *Config, + agg aggregator.Aggregator, + rt router.Router, + backendClient vmcp.BackendClient, + backendRegistry vmcp.BackendRegistry, + workflowDefs map[string]*composer.WorkflowDefinition, + authzCfg *authz.Config, + elicitation vmcp.ElicitationRequester, + healthProvider health.StatusProvider, +) *core.Config { + return &core.Config{ + Aggregator: agg, + Router: rt, + BackendRegistry: backendRegistry, + BackendClient: backendClient, + WorkflowDefs: workflowDefs, + Authz: authzCfg, + ServerName: cfg.Name, + // Cross-cutting (also on ServerConfig) — R3, not a clean partition: + TelemetryProvider: cfg.TelemetryProvider, + AuditConfig: cfg.AuditConfig, + HealthStatusProvider: healthProvider, + Elicitation: elicitation, + } +} diff --git a/pkg/vmcp/server/derive_test.go b/pkg/vmcp/server/derive_test.go new file mode 100644 index 0000000000..378724a472 --- /dev/null +++ b/pkg/vmcp/server/derive_test.go @@ -0,0 +1,278 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "net/http" + "reflect" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive/pkg/audit" + asrunner "github.com/stacklok/toolhive/pkg/authserver/runner" + "github.com/stacklok/toolhive/pkg/authz" + "github.com/stacklok/toolhive/pkg/telemetry" + "github.com/stacklok/toolhive/pkg/vmcp" + aggmocks "github.com/stacklok/toolhive/pkg/vmcp/aggregator/mocks" + "github.com/stacklok/toolhive/pkg/vmcp/composer" + vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/pkg/vmcp/health" + vmcpmocks "github.com/stacklok/toolhive/pkg/vmcp/mocks" + routermocks "github.com/stacklok/toolhive/pkg/vmcp/router/mocks" +) + +// stubStatusProvider is a non-nil health.StatusProvider for derivation tests; its +// behavior is not exercised — only its identity is asserted (it is passed through +// onto core.Config.HealthStatusProvider). +type stubStatusProvider struct{} + +func (*stubStatusProvider) QueryBackendStatus(string) (vmcp.BackendHealthStatus, bool) { + return vmcp.BackendHealthy, true +} + +// populatedLegacyConfig returns a server.Config with every field deriveServerConfig +// reads set to a distinctive non-zero value, so a dropped or wrong-source mapping +// surfaces in the assertions below. AuthzMiddleware is set too, to prove derivation +// retains it on the legacy Config (vestigial) while never projecting it. +func populatedLegacyConfig() *Config { + passthrough := func(h http.Handler) http.Handler { return h } + return &Config{ + Name: "vmcp-name", + Version: "9.9.9", + GroupRef: "grp", + Host: "0.0.0.0", + Port: 7777, + EndpointPath: "/custom", + SessionTTL: 17 * time.Minute, + AuthMiddleware: passthrough, + AuthzMiddleware: passthrough, + AuthInfoHandler: http.NewServeMux(), + RateLimitMiddleware: passthrough, + AuthServer: &asrunner.EmbeddedAuthServer{}, + TelemetryProvider: &telemetry.Provider{}, + AuditConfig: &audit.Config{}, + StatusReportingInterval: 11 * time.Second, + Watcher: stubWatcher{}, + StatusReporter: stubServeReporter{}, + SessionStorage: &vmcpconfig.SessionStorageConfig{}, + } +} + +func TestDeriveServerConfigProjectsTransportFields(t *testing.T) { + t.Parallel() + + cfg := populatedLegacyConfig() + healthMon := &health.Monitor{} + registry := vmcp.NewImmutableRegistry([]vmcp.Backend{}) + smCfg := testMinimalSessionManagerConfig() + + got := deriveServerConfig(cfg, healthMon, registry, smCfg) + + // Scalars projected verbatim (cfg's values are all non-default, so cmp.Or returns them). + assert.Equal(t, "vmcp-name", got.Name) + assert.Equal(t, "9.9.9", got.Version) + assert.Equal(t, "grp", got.GroupRef) + assert.Equal(t, "0.0.0.0", got.Host) + assert.Equal(t, 7777, got.Port) + assert.Equal(t, "/custom", got.EndpointPath) + assert.Equal(t, 17*time.Minute, got.SessionTTL) + assert.Equal(t, 11*time.Second, got.StatusReportingInterval) + + // Func/handler/pointer fields projected by reference. + assert.NotNil(t, got.AuthMiddleware) + assert.NotNil(t, got.RateLimitMiddleware) + assert.Same(t, cfg.AuthInfoHandler, got.AuthInfoHandler) + assert.Same(t, cfg.AuthServer, got.AuthServer) + assert.Same(t, cfg.SessionStorage, got.SessionStorage) + assert.Equal(t, cfg.Watcher, got.Watcher) + assert.Equal(t, cfg.StatusReporter, got.StatusReporter) + + // Cross-cutting fields shared with the core (R3). + assert.Same(t, cfg.TelemetryProvider, got.TelemetryProvider) + assert.Same(t, cfg.AuditConfig, got.AuditConfig) + + // Collaborators passed in (not on server.Config) — threaded through, not from cfg. + assert.Same(t, healthMon, got.HealthMonitor) + assert.Same(t, registry, got.BackendRegistry) + assert.Same(t, smCfg, got.SessionManagerConfig) +} + +// deriveServerConfig no longer applies transport defaults — it is a pure projection now +// that defaulting is resolved once at the composition root. The WithDefaults +// resolver carries that behavior and is covered by TestWithDefaults (server_test.go). + +func TestDeriveServerConfigPropagatesNilCrossCutting(t *testing.T) { + t.Parallel() + + // A deliberately-disabled subsystem (nil provider/config/monitor) must stay nil — + // derivation must not substitute a default or panic. + got := deriveServerConfig(&Config{}, nil, nil, nil) + + assert.Nil(t, got.TelemetryProvider) + assert.Nil(t, got.AuditConfig) + assert.Nil(t, got.HealthMonitor) + assert.Nil(t, got.BackendRegistry) + assert.Nil(t, got.SessionManagerConfig) +} + +// TestDeriveServerConfigMapsAllFields guards deriveServerConfig against silent drift: +// with every readable source field non-zero and every collaborator param non-nil, every +// ServerConfig field must be populated. deriveServerConfig is a pure pass-through (no +// defaulting), so this presence check covers every field uniformly — a dropped mapping +// surfaces as a zero field. The skip set (none today) is the escape hatch for any future +// semantically-zero-valid field — a bool/int that legitimately defaults to its zero value +// — since a presence check cannot distinguish "unset" from "validly zero". +func TestDeriveServerConfigMapsAllFields(t *testing.T) { + t.Parallel() + + got := deriveServerConfig( + populatedLegacyConfig(), + &health.Monitor{}, + vmcp.NewImmutableRegistry([]vmcp.Backend{}), + testMinimalSessionManagerConfig(), + ) + + assertAllFieldsPopulated(t, *got, "ServerConfig", nil) +} + +func TestDeriveCoreConfigAssemblesCollaborators(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + cfg := &Config{ + Name: "core-name", + TelemetryProvider: &telemetry.Provider{}, + AuditConfig: &audit.Config{}, + } + agg := aggmocks.NewMockAggregator(ctrl) + rt := routermocks.NewMockRouter(ctrl) + backendClient := vmcpmocks.NewMockBackendClient(ctrl) + registry := vmcpmocks.NewMockBackendRegistry(ctrl) + workflowDefs := map[string]*composer.WorkflowDefinition{"wf": {}} + authzCfg := &authz.Config{} + elicitation := vmcpmocks.NewMockElicitationRequester(ctrl) + healthProvider := &stubStatusProvider{} + + got := deriveCoreConfig(cfg, agg, rt, backendClient, registry, workflowDefs, authzCfg, elicitation, healthProvider) + + // Collaborators passed through by identity. + assert.Same(t, agg, got.Aggregator) + assert.Same(t, rt, got.Router) + assert.Same(t, backendClient, got.BackendClient) + assert.Same(t, registry, got.BackendRegistry) + assert.Same(t, authzCfg, got.Authz) + assert.Same(t, elicitation, got.Elicitation) + assert.Same(t, healthProvider, got.HealthStatusProvider) + assert.Equal(t, workflowDefs, got.WorkflowDefs) + + // ServerName uses the raw cfg.Name for authz parity (no transport default applied). + assert.Equal(t, "core-name", got.ServerName) + + // Cross-cutting fields shared with the transport (R3). + assert.Same(t, cfg.TelemetryProvider, got.TelemetryProvider) + assert.Same(t, cfg.AuditConfig, got.AuditConfig) +} + +func TestDeriveCoreConfigUsesRawServerNameAndNilDefaults(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + // Empty Name must NOT be defaulted (authz keys on the real VirtualMCPServer name), + // and nil admission/health inputs must propagate as nil (allow-all / no filtering). + got := deriveCoreConfig( + &Config{}, + aggmocks.NewMockAggregator(ctrl), + routermocks.NewMockRouter(ctrl), + vmcpmocks.NewMockBackendClient(ctrl), + vmcpmocks.NewMockBackendRegistry(ctrl), + nil, // workflowDefs + nil, // authzCfg + nil, // elicitation + nil, // healthProvider + ) + + assert.Empty(t, got.ServerName) + assert.Nil(t, got.Authz) + assert.Nil(t, got.Elicitation) + assert.Nil(t, got.HealthStatusProvider) + assert.Nil(t, got.WorkflowDefs) +} + +// TestDeriveCoreConfigMapsAllFields guards deriveCoreConfig against silent drift: with +// every cross-cutting source field non-zero and every collaborator non-nil, every +// core.Config field must be populated. Like TestDeriveServerConfigMapsAllFields this is a +// pure presence check; the skip set is the escape hatch for any future semantically-zero- +// valid field (a bool/int that legitimately defaults to its zero value). +func TestDeriveCoreConfigMapsAllFields(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + cfg := &Config{ + Name: "core-name", + TelemetryProvider: &telemetry.Provider{}, + AuditConfig: &audit.Config{}, + } + + got := deriveCoreConfig( + cfg, + aggmocks.NewMockAggregator(ctrl), + routermocks.NewMockRouter(ctrl), + vmcpmocks.NewMockBackendClient(ctrl), + vmcpmocks.NewMockBackendRegistry(ctrl), + map[string]*composer.WorkflowDefinition{"wf": {}}, + &authz.Config{}, + vmcpmocks.NewMockElicitationRequester(ctrl), + &stubStatusProvider{}, + ) + + assertAllFieldsPopulated(t, *got, "core.Config", nil) +} + +// TestDeriveConfigsTreatInputAsReadOnly verifies neither helper mutates cfg. server.New +// applies its defaults by writing back onto cfg; the derivation helpers must not, so a +// caller's Config is unchanged after derivation (go-style: copy before mutating input). +func TestDeriveConfigsTreatInputAsReadOnly(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + authzFn := func(h http.Handler) http.Handler { return h } + cfg := &Config{AuthzMiddleware: authzFn} // empty scalars: defaulting would mutate them. + + _ = deriveServerConfig(cfg, nil, nil, nil) + _ = deriveCoreConfig( + cfg, + aggmocks.NewMockAggregator(ctrl), + routermocks.NewMockRouter(ctrl), + vmcpmocks.NewMockBackendClient(ctrl), + vmcpmocks.NewMockBackendRegistry(ctrl), + nil, nil, nil, nil, + ) + + // Transport defaults were NOT written back onto the caller's Config. + assert.Empty(t, cfg.Name) + assert.Empty(t, cfg.Version) + assert.Empty(t, cfg.Host) + assert.Empty(t, cfg.EndpointPath) + assert.Zero(t, cfg.SessionTTL) + // The vestigial AuthzMiddleware field is retained on the legacy Config (never cleared). + assert.NotNil(t, cfg.AuthzMiddleware) +} + +// assertAllFieldsPopulated asserts every field of the struct value v is non-zero, +// skipping any name in skip. typeName labels failures. +func assertAllFieldsPopulated(t *testing.T, v any, typeName string, skip map[string]struct{}) { + t.Helper() + rv := reflect.ValueOf(v) + rt := rv.Type() + for i := range rt.NumField() { + name := rt.Field(i).Name + if _, skipped := skip[name]; skipped { + continue + } + assert.Falsef(t, rv.Field(i).IsZero(), "%s.%s was not populated by derivation", typeName, name) + } +} diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index 6187cc88a9..b66cf80d4a 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -4,7 +4,6 @@ package server import ( - "cmp" "context" "fmt" "net/http" @@ -296,11 +295,11 @@ func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) return srv, nil } -// buildServeConfig maps the transport-only ServerConfig onto the existing *Config -// the carried-forward (*Server) methods read from, applying the same transport -// defaults server.New applies today. Defaults are applied here rather than by -// mutating the caller's ServerConfig (go-style: copy before mutating caller input). -// Port 0 is left untouched to mean "OS-assigned". +// buildServeConfig maps the transport-only ServerConfig onto the existing *Config the +// carried-forward (*Server) methods read from. It is a pure pass-through: transport +// defaults are resolved once at the composition root via WithDefaults, so the +// incoming ServerConfig is already resolved and buildServeConfig applies no defaulting of +// its own. Port 0 still means "OS-assigned". // // Several Config fields are deliberately NOT mapped at this phase (see // TestBuildServeConfigMapsSharedFields, which guards this list against drift): @@ -320,13 +319,13 @@ func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) // wiring), not via Config→New, so these Config fields are unused on the Serve path. func buildServeConfig(cfg *ServerConfig) *Config { return &Config{ - Name: cmp.Or(cfg.Name, defaultServerName), - Version: cmp.Or(cfg.Version, defaultServerVersion), + Name: cfg.Name, + Version: cfg.Version, GroupRef: cfg.GroupRef, - Host: cmp.Or(cfg.Host, defaultHost), + Host: cfg.Host, Port: cfg.Port, - EndpointPath: cmp.Or(cfg.EndpointPath, defaultEndpointPath), - SessionTTL: cmp.Or(cfg.SessionTTL, defaultSessionTTL), + EndpointPath: cfg.EndpointPath, + SessionTTL: cfg.SessionTTL, AuthMiddleware: cfg.AuthMiddleware, RateLimitMiddleware: cfg.RateLimitMiddleware, AuthInfoHandler: cfg.AuthInfoHandler, diff --git a/pkg/vmcp/server/serve_session_test.go b/pkg/vmcp/server/serve_session_test.go index 34ed1331eb..965d7a04cb 100644 --- a/pkg/vmcp/server/serve_session_test.go +++ b/pkg/vmcp/server/serve_session_test.go @@ -204,6 +204,7 @@ func TestServeRegistersSessionHooks(t *testing.T) { fc := &fakeCore{tools: []vmcp.Tool{testTool}} srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), }) @@ -302,6 +303,7 @@ func TestServeLazyInjectsToolsForRehydratedSession(t *testing.T) { fc := &fakeCore{tools: []vmcp.Tool{testTool}} srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), }) @@ -365,6 +367,7 @@ func TestServeReturnsErrorWhenSessionManagerConstructionFails(t *testing.T) { t.Parallel() srv, err := Serve(context.Background(), &stubVMCP{}, &ServerConfig{ + SessionTTL: time.Minute, SessionManagerConfig: &sessionmanager.FactoryConfig{Base: testMinimalFactory(), CacheCapacity: -1}, BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), }) @@ -460,6 +463,7 @@ func TestServeHandlerSkipsDiscoveryAndRoutesCallThroughCore(t *testing.T) { fc := &fakeCore{tools: []vmcp.Tool{testTool}} srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), }) @@ -523,6 +527,7 @@ func TestServeEnforcesSessionBinding(t *testing.T) { fc := &fakeCore{tools: []vmcp.Tool{testTool}} srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), }) @@ -576,6 +581,7 @@ func registerServeSession(t *testing.T, fc *fakeCore) (*Server, string, string) factory, _ := newToolSessionFactory(t, ctrl, fc.tools) srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), }) diff --git a/pkg/vmcp/server/serve_test.go b/pkg/vmcp/server/serve_test.go index 6e9d552044..966186c3e5 100644 --- a/pkg/vmcp/server/serve_test.go +++ b/pkg/vmcp/server/serve_test.go @@ -87,44 +87,29 @@ func testMinimalSessionManagerConfig() *sessionmanager.FactoryConfig { return &sessionmanager.FactoryConfig{Base: testMinimalFactory()} } -// testMinimalServeConfig returns a minimal valid ServerConfig for Serve tests that do -// not exercise session creation: a non-nil SessionManagerConfig and an empty -// BackendRegistry, the two required collaborators Serve validates. +// testMinimalServeConfig returns a minimal valid, fully-resolved ServerConfig for Serve +// tests that do not exercise session creation: the two required collaborators Serve +// validates (a non-nil SessionManagerConfig and an empty BackendRegistry) plus the +// transport scalars resolved to their defaults. Serve is a pure pass-through now — +// transport defaulting moved to the composition root via WithDefaults — so the +// scalars must be set here; in particular SessionTTL must be non-zero or the session data +// storage construction fails ("ttl must be a positive duration"). func testMinimalServeConfig() *ServerConfig { return &ServerConfig{ + Name: defaultServerName, + Version: defaultServerVersion, + Host: defaultHost, + EndpointPath: defaultEndpointPath, + SessionTTL: defaultSessionTTL, SessionManagerConfig: testMinimalSessionManagerConfig(), BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), } } -func TestServeAppliesTransportDefaults(t *testing.T) { - t.Parallel() - - // Empty transport fields exercise every default; Port is left zero. - cfg := testMinimalServeConfig() - - srv, err := Serve(context.Background(), &stubVMCP{}, cfg) - require.NoError(t, err) - t.Cleanup(func() { _ = srv.Stop(context.Background()) }) - require.NotNil(t, srv) - require.NotNil(t, srv.MCPServer()) - - // Defaults mirror server.New and are applied to the server's own config, - // leaving the caller's ServerConfig untouched. Asserting against the shared - // consts (not literals) keeps this test from being a third copy that can drift. - assert.Equal(t, defaultServerName, srv.config.Name) - assert.Equal(t, defaultServerVersion, srv.config.Version) - assert.Equal(t, defaultHost, srv.config.Host) - assert.Equal(t, defaultEndpointPath, srv.config.EndpointPath) - assert.Equal(t, defaultSessionTTL, srv.config.SessionTTL) - assert.Equal(t, 0, srv.config.Port) // Port 0 => OS-assigned - - assert.Empty(t, cfg.Host, "caller config must not be mutated") - - // Address reflects the defaulted host and unassigned port (no listener yet). - assert.Equal(t, defaultHost+":0", srv.Address()) -} - +// Transport defaulting is no longer Serve's job (it is resolved once at the +// composition root via WithDefaults), so there is no "Serve applies defaults" test — the +// WithDefaults resolver is covered by TestWithDefaults, and Serve's faithful pass-through +// of an already-resolved config is covered by TestServePreservesExplicitConfig below. func TestServePreservesExplicitConfig(t *testing.T) { t.Parallel() @@ -235,6 +220,7 @@ func TestServeHandlerRegistersMetricsWhenTelemetryEnabled(t *testing.T) { t.Cleanup(func() { _ = provider.Shutdown(ctx) }) srv, err := Serve(ctx, &stubVMCP{}, &ServerConfig{ + SessionTTL: defaultSessionTTL, SessionManagerConfig: testMinimalSessionManagerConfig(), BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), TelemetryProvider: provider, @@ -321,9 +307,9 @@ func TestServeValidation(t *testing.T) { // reason, mirroring the buildServeConfig doc comment. // // This is a PRESENCE check only — with every source field non-zero it cannot catch a -// wrong-source mapping or default-value drift. Value correctness is carried by -// TestServePreservesExplicitConfig (pass-through scalars) and -// TestServeAppliesTransportDefaults (the cmp.Or defaults). +// wrong-source mapping. buildServeConfig is a pure pass-through (defaulting moved to the +// composition root via WithDefaults), so value correctness is carried by +// TestServePreservesExplicitConfig. func TestBuildServeConfigMapsSharedFields(t *testing.T) { t.Parallel() diff --git a/pkg/vmcp/server/server.go b/pkg/vmcp/server/server.go index 2355bd103c..146a9799dd 100644 --- a/pkg/vmcp/server/server.go +++ b/pkg/vmcp/server/server.go @@ -331,24 +331,15 @@ func New( backendRegistry vmcp.BackendRegistry, workflowDefs map[string]*composer.WorkflowDefinition, ) (*Server, error) { - // Apply defaults - if cfg.Host == "" { - cfg.Host = defaultHost - } - // Note: Port 0 means "let OS assign random port" - intentionally no default applied here. - // CLI provides default via flag (4483), so Port is only 0 in tests for dynamic port assignment. - if cfg.EndpointPath == "" { - cfg.EndpointPath = defaultEndpointPath - } - if cfg.Name == "" { - cfg.Name = defaultServerName - } - if cfg.Version == "" { - cfg.Version = defaultServerVersion - } - if cfg.SessionTTL == 0 { - cfg.SessionTTL = defaultSessionTTL - } + // Resolve transport defaults on a COPY. The composition root (cli) already resolves + // them at the edge via WithDefaults (a single defaulting list); New repeats + // it defensively so legacy direct callers and tests that build a Config by hand keep + // working — but without mutating the caller's value (go-style: copy before mutating + // caller input). That non-mutation is what lets #5445 hand the raw, un-defaulted + // cfg.Name to the core for Cedar authz parity. New's own defaulting goes away when + // #5445 reduces it to a Serve(core.New(...)) wrapper; until then WithDefaults is the + // single place the default list lives, shared with the edge. + cfg = WithDefaults(cfg) // Create hooks for SDK integration hooks := &server.Hooks{} diff --git a/pkg/vmcp/server/server_test.go b/pkg/vmcp/server/server_test.go index 402f857e8e..672217a712 100644 --- a/pkg/vmcp/server/server_test.go +++ b/pkg/vmcp/server/server_test.go @@ -215,6 +215,78 @@ func TestNew(t *testing.T) { } } +// TestWithDefaults covers the single transport-defaulting resolver. It is the one place +// the default list lives; the composition root and the constructors route +// their Config through it, so New/Serve/derive* downstream are pure pass-through. +func TestWithDefaults(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in *server.Config + want server.Config // only transport scalars are compared + }{ + { + name: "fills every transport default on an empty config", + in: &server.Config{}, + want: server.Config{ + Name: "toolhive-vmcp", Version: "0.1.0", Host: "127.0.0.1", + EndpointPath: "/mcp", SessionTTL: 30 * time.Minute, Port: 0, + }, + }, + { + name: "preserves explicitly set values", + in: &server.Config{ + Name: "custom", Version: "1.2.3", Host: "0.0.0.0", + EndpointPath: "/rpc", SessionTTL: 7 * time.Minute, Port: 8080, + }, + want: server.Config{ + Name: "custom", Version: "1.2.3", Host: "0.0.0.0", + EndpointPath: "/rpc", SessionTTL: 7 * time.Minute, Port: 8080, + }, + }, + { + name: "fills only the unset fields", + in: &server.Config{Host: "192.168.1.1", Port: 9000}, + want: server.Config{ + Name: "toolhive-vmcp", Version: "0.1.0", Host: "192.168.1.1", + EndpointPath: "/mcp", SessionTTL: 30 * time.Minute, Port: 9000, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := server.WithDefaults(tt.in) + assert.Equal(t, tt.want.Name, got.Name) + assert.Equal(t, tt.want.Version, got.Version) + assert.Equal(t, tt.want.Host, got.Host) + assert.Equal(t, tt.want.EndpointPath, got.EndpointPath) + assert.Equal(t, tt.want.SessionTTL, got.SessionTTL) + assert.Equal(t, tt.want.Port, got.Port) // Port is never defaulted (0 => OS-assigned) + }) + } +} + +// TestWithDefaultsDoesNotMutateInput verifies WithDefaults treats its argument as +// read-only: an all-unset Config passes through with no fields written back, so callers +// (and the constructors that call it on a copy) never see their value mutated. This +// non-mutation is what preserves the raw cfg.Name for downstream Cedar authz parity. +func TestWithDefaultsDoesNotMutateInput(t *testing.T) { + t.Parallel() + + in := &server.Config{} // all unset: defaulting would be visible as mutation + _ = server.WithDefaults(in) + + assert.Empty(t, in.Name) + assert.Empty(t, in.Version) + assert.Empty(t, in.Host) + assert.Empty(t, in.EndpointPath) + assert.Zero(t, in.SessionTTL) +} + func TestServer_Address(t *testing.T) { t.Parallel()