From 79d4b17d5ff6855ff6b9d6460b8732d7d265a0c0 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 1 Apr 2026 16:13:52 -0700 Subject: [PATCH 1/3] refactor: remove redundant config default fallbacks in server and launcher Add Config.EnsureGatewayDefaults() that guarantees cfg.Gateway is non-nil with all defaults applied. Call it from NewUnified() and launcher.New() to replace the per-field nil-guard + fallback blocks that duplicated defaults already established by the config loading layer. This removes ~20 lines of redundant defensive code across unified.go (3 blocks for PayloadDir, PayloadPathPrefix, PayloadSizeThreshold) and launcher.go (1 block for StartupTimeout). Closes #2983 Closes #2984 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/config/config_core.go | 12 ++++++++++++ internal/launcher/launcher.go | 15 +++++++-------- internal/launcher/launcher_test.go | 6 ++++-- internal/server/unified.go | 26 ++++++++++---------------- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/internal/config/config_core.go b/internal/config/config_core.go index 2356bff5..7dead27d 100644 --- a/internal/config/config_core.go +++ b/internal/config/config_core.go @@ -198,6 +198,18 @@ func applyGatewayDefaults(cfg *GatewayConfig) { } } +// EnsureGatewayDefaults guarantees that cfg.Gateway is non-nil and that all +// gateway-level fields have sensible defaults applied. LoadFromFile and +// LoadFromStdin already call this, but consumers that receive a Config +// constructed manually (e.g. in tests) can call it to avoid nil-pointer panics. +func (cfg *Config) EnsureGatewayDefaults() { + if cfg.Gateway == nil { + cfg.Gateway = &GatewayConfig{} + } + applyGatewayDefaults(cfg.Gateway) + applyDefaults(cfg) +} + // LoadFromFile loads configuration from a TOML file. // // This function uses the BurntSushi/toml v1.6.0+ parser with TOML 1.1 support, diff --git a/internal/launcher/launcher.go b/internal/launcher/launcher.go index 6336459a..3285fc10 100644 --- a/internal/launcher/launcher.go +++ b/internal/launcher/launcher.go @@ -55,14 +55,13 @@ func New(ctx context.Context, cfg *config.Config) *Launcher { log.Println("[LAUNCHER] Detected running inside a container") } - // Get startup timeout from config, default to config.DefaultStartupTimeout seconds - startupTimeout := time.Duration(config.DefaultStartupTimeout) * time.Second - if cfg.Gateway != nil && cfg.Gateway.StartupTimeout > 0 { - startupTimeout = time.Duration(cfg.Gateway.StartupTimeout) * time.Second - logLauncher.Printf("Using configured startup timeout: %v", startupTimeout) - } else { - logLauncher.Printf("Using default startup timeout: %v", startupTimeout) - } + // Guarantee cfg.Gateway is non-nil with defaults applied. + // LoadFromFile/LoadFromStdin already ensure this, but tests may + // construct configs manually without going through the load path. + cfg.EnsureGatewayDefaults() + + startupTimeout := time.Duration(cfg.Gateway.StartupTimeout) * time.Second + logLauncher.Printf("Using startup timeout: %v", startupTimeout) // Initialize OIDC provider from environment if available var oidcProvider *oidc.Provider diff --git a/internal/launcher/launcher_test.go b/internal/launcher/launcher_test.go index 7d0717fe..c3016e28 100644 --- a/internal/launcher/launcher_test.go +++ b/internal/launcher/launcher_test.go @@ -527,6 +527,7 @@ func TestGetOrLaunchForSession_SessionReuse(t *testing.T) { ctx := context.Background() cfg := &config.Config{ Servers: map[string]*config.ServerConfig{}, + Gateway: &config.GatewayConfig{StartupTimeout: config.DefaultStartupTimeout}, } l := New(ctx, cfg) defer l.Close() @@ -606,7 +607,8 @@ func TestLauncher_StartupTimeout(t *testing.T) { } func TestLauncher_TimeoutWithNilGateway(t *testing.T) { - // Test that launcher uses default timeout when Gateway config is nil + // Test that launcher uses default timeout when Gateway config is nil. + // EnsureGatewayDefaults is called by launcher.New, so nil Gateway is safe. ctx := context.Background() cfg := &config.Config{ Servers: map[string]*config.ServerConfig{ @@ -615,7 +617,7 @@ func TestLauncher_TimeoutWithNilGateway(t *testing.T) { URL: "http://example.com", }, }, - Gateway: nil, // No gateway config + Gateway: nil, // No gateway config — EnsureGatewayDefaults fills defaults } l := New(ctx, cfg) diff --git a/internal/server/unified.go b/internal/server/unified.go index a515197b..4c6a4639 100644 --- a/internal/server/unified.go +++ b/internal/server/unified.go @@ -110,25 +110,19 @@ type UnifiedServer struct { // NewUnified creates a new unified MCP server func NewUnified(ctx context.Context, cfg *config.Config) (*UnifiedServer, error) { logUnified.Printf("Creating new unified server: sequentialLaunch=%v, servers=%d", cfg.SequentialLaunch, len(cfg.Servers)) - l := launcher.New(ctx, cfg) - // Get payload directory from config, with fallback to default - payloadDir := config.DefaultPayloadDir - if cfg.Gateway != nil && cfg.Gateway.PayloadDir != "" { - payloadDir = cfg.Gateway.PayloadDir - } + // Guarantee cfg.Gateway is non-nil with defaults applied. + // LoadFromFile/LoadFromStdin already ensure this, but tests may + // construct configs manually without going through the load path. + cfg.EnsureGatewayDefaults() - // Get payload path prefix from config (empty by default) - payloadPathPrefix := "" - if cfg.Gateway != nil && cfg.Gateway.PayloadPathPrefix != "" { - payloadPathPrefix = cfg.Gateway.PayloadPathPrefix - } + l := launcher.New(ctx, cfg) - // Get payload size threshold from config, with fallback to default - payloadSizeThreshold := config.DefaultPayloadSizeThreshold - if cfg.Gateway != nil && cfg.Gateway.PayloadSizeThreshold > 0 { - payloadSizeThreshold = cfg.Gateway.PayloadSizeThreshold - } + // Config loading guarantees cfg.Gateway is non-nil and all fields + // have defaults applied via applyGatewayDefaults/applyDefaults. + payloadDir := cfg.Gateway.PayloadDir + payloadPathPrefix := cfg.Gateway.PayloadPathPrefix + payloadSizeThreshold := cfg.Gateway.PayloadSizeThreshold logUnified.Printf("Payload configuration: dir=%s, pathPrefix=%s, sizeThreshold=%d bytes (%.2f KB)", payloadDir, payloadPathPrefix, payloadSizeThreshold, float64(payloadSizeThreshold)/1024) From 55906735abb1c6a4ca8f670a6234a6e0e4baa9f7 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 1 Apr 2026 16:20:59 -0700 Subject: [PATCH 2/3] Update internal/config/config_core.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/config/config_core.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/config/config_core.go b/internal/config/config_core.go index 7dead27d..1aa72d58 100644 --- a/internal/config/config_core.go +++ b/internal/config/config_core.go @@ -199,9 +199,10 @@ func applyGatewayDefaults(cfg *GatewayConfig) { } // EnsureGatewayDefaults guarantees that cfg.Gateway is non-nil and that all -// gateway-level fields have sensible defaults applied. LoadFromFile and -// LoadFromStdin already call this, but consumers that receive a Config -// constructed manually (e.g. in tests) can call it to avoid nil-pointer panics. +// gateway-level fields have sensible defaults applied. This matches the +// invariants enforced by the standard loaders (LoadFromFile, LoadFromStdin), +// and can be used by callers that construct Config values manually (e.g. in +// tests) to avoid nil-pointer panics and ensure consistent defaults. func (cfg *Config) EnsureGatewayDefaults() { if cfg.Gateway == nil { cfg.Gateway = &GatewayConfig{} From 046ef0abb98a6c0414c747977182c5a9786d94f9 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Wed, 1 Apr 2026 16:21:10 -0700 Subject: [PATCH 3/3] Update internal/server/unified.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/server/unified.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/internal/server/unified.go b/internal/server/unified.go index 4c6a4639..1df1d201 100644 --- a/internal/server/unified.go +++ b/internal/server/unified.go @@ -111,11 +111,6 @@ type UnifiedServer struct { func NewUnified(ctx context.Context, cfg *config.Config) (*UnifiedServer, error) { logUnified.Printf("Creating new unified server: sequentialLaunch=%v, servers=%d", cfg.SequentialLaunch, len(cfg.Servers)) - // Guarantee cfg.Gateway is non-nil with defaults applied. - // LoadFromFile/LoadFromStdin already ensure this, but tests may - // construct configs manually without going through the load path. - cfg.EnsureGatewayDefaults() - l := launcher.New(ctx, cfg) // Config loading guarantees cfg.Gateway is non-nil and all fields