fix(config): persisted-vs-effective split — never persist serve flags, MCPPROXY_* env or the env API key

cmd/mcpproxy/main.go

@@ -432,68 +432,14 @@ func runServer(cmd *cobra.Command, _ []string) error {
 	// Get flag values from command (handles both global and local flags)
 	cmdLogLevel, _ := cmd.Flags().GetString("log-level")
 	cmdLogToFile, _ := cmd.Flags().GetBool("log-to-file")
-	cmdLogDir, _ := cmd.Flags().GetString("log-dir")
-	cmdDebugSearch, _ := cmd.Flags().GetBool("debug-search")
-	cmdToolResponseLimit, _ := cmd.Flags().GetInt("tool-response-limit")
-	cmdRequireMCPAuth, _ := cmd.Flags().GetBool("require-mcp-auth")
-	cmdReadOnlyMode, _ := cmd.Flags().GetBool("read-only")
-	cmdDisableManagement, _ := cmd.Flags().GetBool("disable-management")
-	cmdAllowServerAdd, _ := cmd.Flags().GetBool("allow-server-add")
-	cmdAllowServerRemove, _ := cmd.Flags().GetBool("allow-server-remove")
-	cmdEnablePrompts, _ := cmd.Flags().GetBool("enable-prompts")
-	cmdAggregateUpstreamPrompts, _ := cmd.Flags().GetBool("aggregate-upstream-prompts")
 
 	// Load configuration first to get logging settings
 	cfg, saver, err := loadConfig(cmd)
 	if err != nil {
 		return fmt.Errorf("failed to load configuration: %w", err)
 	}
 
-	// Override logging settings from command line
-	if cfg.Logging == nil {
-		// Use command-specific default level (INFO for server command)
-		defaultLevel := cmdLogLevel
-		if defaultLevel == "" {
-			defaultLevel = defaultLogLevel // Server command defaults to INFO
-		}
-
-		cfg.Logging = &config.LogConfig{
-			Level:         defaultLevel,
-			EnableFile:    !cmd.Flags().Changed("log-to-file") || cmdLogToFile, // Default true for serve, unless explicitly disabled
-			EnableConsole: true,
-			Filename:      "main.log",
-			MaxSize:       10,
-			MaxBackups:    5,
-			MaxAge:        30,
-			Compress:      true,
-			JSONFormat:    false,
-		}
-	} else {
-		// Override specific fields from command line
-		if cmdLogLevel != "" {
-			cfg.Logging.Level = cmdLogLevel
-		} else if cfg.Logging.Level == "" {
-			cfg.Logging.Level = defaultLogLevel // Server command defaults to INFO
-		}
-
-		// For serve mode: Enable file logging by default, only disable if explicitly set to false
-		if cmd.Flags().Changed("log-to-file") {
-			cfg.Logging.EnableFile = cmdLogToFile
-		} else {
-			cfg.Logging.EnableFile = true // Default to true for serve mode
-		}
-
-		if cfg.Logging.Filename == "" || cfg.Logging.Filename == "mcpproxy.log" {
-			cfg.Logging.Filename = "main.log"
-		}
-	}
-
-	// Resolve the log directory. An explicit --log-dir wins; otherwise a
-	// non-default data dir co-locates logs under <data-dir>/logs so that
-	// tests/e2e/harness `serve` runs do not pollute the shared OS-standard
-	// prod log (root cause of the phantom "core restarts every 10s" in
-	// MCP-2250). The default data dir keeps the OS-standard location.
-	cfg.Logging.LogDir = resolveServeLogDir(cmdLogDir, cfg.Logging.LogDir, cfg.DataDir, defaultDataDirPath())
+	applyServeLoggingFlags(cmd, cfg)
 
 	// Setup logger with new logging system
 	logger, err := logs.SetupLogger(cfg.Logging)
@@ -545,38 +491,11 @@ func runServer(cmd *cobra.Command, _ []string) error {
 	// Issue #566: registries (e.g. Pulse) require a versioned User-Agent.
 	registries.SetVersion(version)
 
-	// Override other settings from command line
-	cfg.DebugSearch = cmdDebugSearch
-
-	if cmdToolResponseLimit != 0 {
-		cfg.ToolResponseLimit = cmdToolResponseLimit
-	}
-
-	// Apply security settings from command line ONLY if explicitly set
-	if cmd.Flags().Changed("require-mcp-auth") {
-		cfg.RequireMCPAuth = cmdRequireMCPAuth
-	}
-	if cmd.Flags().Changed("read-only") {
-		cfg.ReadOnlyMode = cmdReadOnlyMode
-	}
-	if cmd.Flags().Changed("disable-management") {
-		cfg.DisableManagement = cmdDisableManagement
-	}
-	if cmd.Flags().Changed("allow-server-add") {
-		cfg.AllowServerAdd = cmdAllowServerAdd
-	}
-	if cmd.Flags().Changed("allow-server-remove") {
-		cfg.AllowServerRemove = cmdAllowServerRemove
-	}
-	if cmd.Flags().Changed("enable-prompts") {
-		cfg.EnablePrompts = cmdEnablePrompts
-	}
-	if cmd.Flags().Changed("aggregate-upstream-prompts") {
-		cfg.AggregateUpstreamPrompts = cmdAggregateUpstreamPrompts
-	}
+	applyServeRuntimeFlags(cmd, cfg)
 
 	logger.Info("Configuration loaded",
 		zap.String("data_dir", cfg.DataDir),
+		zap.Strings("process_overrides", config.ProcessOverrideFields()),
 		zap.Int("servers_count", len(cfg.Servers)),
 		zap.Bool("require_mcp_auth", cfg.RequireMCPAuth),
 		zap.Bool("read_only_mode", cfg.ReadOnlyMode),
@@ -822,9 +741,11 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) {
 	// never persist a one-off CLI choice.
 	saver := newServeConfigSaver(cfg, loadedPath)
 
-	// Override with command line flags ONLY if they were explicitly set
+	// Override with command line flags ONLY if they were explicitly set. Each
+	// one is a process-only override (config.OverrideForProcess): the effective
+	// config carries it, no save path persists it — see process_overrides.go.
 	if dataDir != "" {
-		cfg.DataDir = dataDir
+		config.OverrideForProcess(cfg, config.FieldDataDir, config.OverrideSourceFlag, dataDir)
 	}
 	if cmd.Flags().Changed("listen") {
 		listenFlag, _ := cmd.Flags().GetString("listen")
@@ -836,18 +757,18 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) {
 		if listenFlag == "" {
 			listenFlag = ":0"
 		}
-		cfg.Listen = listenFlag
+		config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, listenFlag)
 	}
 	if cmd.Flags().Changed("tray-endpoint") {
 		trayEndpointFlag, _ := cmd.Flags().GetString("tray-endpoint")
-		cfg.TrayEndpoint = trayEndpointFlag
+		config.OverrideForProcess(cfg, config.FieldTrayEndpoint, config.OverrideSourceFlag, trayEndpointFlag)
 	}
 	if cmd.Flags().Changed("enable-socket") {
 		enableSocketFlag, _ := cmd.Flags().GetBool("enable-socket")
-		cfg.EnableSocket = enableSocketFlag
+		config.OverrideForProcess(cfg, config.FieldEnableSocket, config.OverrideSourceFlag, enableSocketFlag)
 	}
 	if toolResponseLimit != 0 {
-		cfg.ToolResponseLimit = toolResponseLimit
+		config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, toolResponseLimit)
 	}
 	applyToolResponseModeFlag(cfg, cmd.Flags().Changed("tool-response-mode"), toolResponseMode)
 	applyDirectToolResponseModeFlag(cfg, cmd.Flags().Changed("direct-tool-response-mode"), directToolResponseMode)
@@ -867,7 +788,7 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) {
 // invalid values with a tool_response_mode error.
 func applyToolResponseModeFlag(cfg *config.Config, changed bool, mode string) {
 	if changed {
-		cfg.ToolResponseMode = mode
+		config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, mode)
 	}
 }
 
@@ -879,7 +800,89 @@ func applyToolResponseModeFlag(cfg *config.Config, changed bool, mode string) {
 // rejects invalid values with a direct_tool_response_mode error.
 func applyDirectToolResponseModeFlag(cfg *config.Config, changed bool, mode string) {
 	if changed {
-		cfg.DirectToolResponseMode = mode
+		config.OverrideForProcess(cfg, config.FieldDirectToolResponseMode, config.OverrideSourceFlag, mode)
+	}
+}
+
+// applyServeLoggingFlags fills serve's logging defaults and layers the
+// --log-level / --log-to-file / --log-dir flags on top. The flags are
+// process-only overrides (config.OverrideForProcess) so no save path writes
+// them into the file; the serve defaults (INFO, file logging on, main.log)
+// are plain in-memory fills, as they always were.
+func applyServeLoggingFlags(cmd *cobra.Command, cfg *config.Config) {
+	cmdLogLevel, _ := cmd.Flags().GetString("log-level")
+	cmdLogToFile, _ := cmd.Flags().GetBool("log-to-file")
+	cmdLogDir, _ := cmd.Flags().GetString("log-dir")
+
+	if cfg.Logging == nil {
+		cfg.Logging = &config.LogConfig{
+			EnableConsole: true,
+			Filename:      "main.log",
+			MaxSize:       10,
+			MaxBackups:    5,
+			MaxAge:        30,
+			Compress:      true,
+			JSONFormat:    false,
+		}
+	}
+
+	if cmdLogLevel != "" {
+		config.OverrideForProcess(cfg, config.FieldLogLevel, config.OverrideSourceFlag, cmdLogLevel)
+	} else if cfg.Logging.Level == "" {
+		cfg.Logging.Level = defaultLogLevel // Server command defaults to INFO
+	}
+
+	// For serve mode: Enable file logging by default, only disable if explicitly set to false
+	if cmd.Flags().Changed("log-to-file") {
+		config.OverrideForProcess(cfg, config.FieldLogEnableFile, config.OverrideSourceFlag, cmdLogToFile)
+	} else {
+		cfg.Logging.EnableFile = true // Default to true for serve mode
+	}
+
+	if cfg.Logging.Filename == "" || cfg.Logging.Filename == "mcpproxy.log" {
+		cfg.Logging.Filename = "main.log"
+	}
+
+	// Resolve the log directory. An explicit --log-dir wins; otherwise a
+	// non-default data dir co-locates logs under <data-dir>/logs so that
+	// tests/e2e/harness `serve` runs do not pollute the shared OS-standard
+	// prod log (root cause of the phantom "core restarts every 10s" in
+	// MCP-2250). The default data dir keeps the OS-standard location.
+	logDir := resolveServeLogDir(cmdLogDir, cfg.Logging.LogDir, cfg.DataDir, defaultDataDirPath())
+	if cmdLogDir != "" {
+		config.OverrideForProcess(cfg, config.FieldLogDir, config.OverrideSourceFlag, logDir)
+	} else {
+		cfg.Logging.LogDir = logDir
+	}
+}
+
+// applyServeRuntimeFlags layers the remaining serve flags onto the loaded
+// config, each as a process-only override (config.OverrideForProcess).
+func applyServeRuntimeFlags(cmd *cobra.Command, cfg *config.Config) {
+	flags := cmd.Flags()
+
+	// --debug-search has always applied unconditionally (its default is
+	// false), so it is recorded unconditionally too.
+	cmdDebugSearch, _ := flags.GetBool("debug-search")
+	config.OverrideForProcess(cfg, config.FieldDebugSearch, config.OverrideSourceFlag, cmdDebugSearch)
+
+	// Apply security settings from command line ONLY if explicitly set
+	for _, f := range []struct {
+		flag  string
+		field config.Field[bool]
+	}{
+		{"require-mcp-auth", config.FieldRequireMCPAuth},
+		{"read-only", config.FieldReadOnlyMode},
+		{"disable-management", config.FieldDisableManagement},
+		{"allow-server-add", config.FieldAllowServerAdd},
+		{"allow-server-remove", config.FieldAllowServerRemove},
+		{"enable-prompts", config.FieldEnablePrompts},
+		{"aggregate-upstream-prompts", config.FieldAggregateUpstreamPrompts},
+	} {
+		if flags.Changed(f.flag) {
+			v, _ := flags.GetBool(f.flag)
+			config.OverrideForProcess(cfg, f.field, config.OverrideSourceFlag, v)
+		}
 	}
 }
 

cmd/mcpproxy/serve_flag_persistence_test.go

@@ -356,3 +356,139 @@ func TestServeSaverUsesTheDiscoveredConfigPath(t *testing.T) {
 	untouched := readConfigFileJSON(t, unrelated)
 	assert.Nil(t, untouched["telemetry"], "save landed in the unrelated <data_dir> config")
 }
+
+// newServeRuntimeFlagTestCmd adds the flags runServer (not loadConfig) applies
+// onto the loaded config.
+func newServeRuntimeFlagTestCmd() *cobra.Command {
+	cmd := newServeFlagTestCmd()
+	cmd.Flags().String("log-level", "", "")
+	cmd.Flags().Bool("log-to-file", true, "")
+	cmd.Flags().String("log-dir", "", "")
+	cmd.Flags().Bool("debug-search", false, "")
+	cmd.Flags().Bool("require-mcp-auth", false, "")
+	cmd.Flags().Bool("read-only", false, "")
+	cmd.Flags().Bool("disable-management", false, "")
+	cmd.Flags().Bool("allow-server-add", true, "")
+	cmd.Flags().Bool("allow-server-remove", true, "")
+	cmd.Flags().Bool("enable-prompts", true, "")
+	cmd.Flags().Bool("aggregate-upstream-prompts", false, "")
+	return cmd
+}
+
+// The serve saver covers serve's OWN three saves. Every other persist path —
+// the runtime's SaveConfiguration on a server enable, telemetry's first-run
+// anonymous_id write — goes through config.SaveConfig with the live config.
+// Those must not persist the flag overrides either, which is what registering
+// every flag as a process-only override (config.OverrideForProcess) buys: the
+// central save seam writes the file's value back.
+func TestServeFlagsAreRegisteredAsProcessOverrides(t *testing.T) {
+	saveServeGlobals(t)
+	t.Cleanup(config.ResetProcessOverrides)
+	config.ResetProcessOverrides()
+	path := writeServeFlagTestConfig(t)
+	configFile, dataDir = path, filepath.Dir(path)
+
+	cmd := newServeRuntimeFlagTestCmd()
+	require.NoError(t, cmd.ParseFlags([]string{
+		"--listen", ":0",
+		"--tray-endpoint", "unix:///tmp/x.sock",
+		"--enable-socket=false",
+		"--tool-response-limit", "500",
+		"--tool-response-mode", "compact",
+		"--direct-tool-response-mode", "deferred",
+		"--log-level", "debug",
+		"--log-to-file=false",
+		"--log-dir", t.TempDir(),
+		"--debug-search",
+		"--require-mcp-auth",
+		"--read-only",
+		"--disable-management",
+		"--allow-server-add=false",
+		"--allow-server-remove=false",
+		"--enable-prompts=false",
+		"--aggregate-upstream-prompts",
+	}))
+
+	cfg, _, err := loadConfig(cmd)
+	require.NoError(t, err)
+	applyServeLoggingFlags(cmd, cfg)
+	applyServeRuntimeFlags(cmd, cfg)
+
+	// The effective config carries every flag…
+	assert.Equal(t, ":0", cfg.Listen)
+	assert.Equal(t, "debug", cfg.Logging.Level)
+	assert.False(t, cfg.Logging.EnableFile)
+	assert.True(t, cfg.ReadOnlyMode)
+	assert.True(t, cfg.DebugSearch)
+	assert.False(t, cfg.AllowServerAdd)
+
+	// …and a plain runtime-style save writes none of them.
+	cfg.ToolsLimit = 42 // a genuine in-memory change that must persist
+	require.NoError(t, config.SaveConfig(cfg, path))
+
+	file := readConfigFileJSON(t, path)
+	assert.Equal(t, "127.0.0.1:8080", file["listen"])
+	assert.Nil(t, file["tray_endpoint"])
+	assert.Equal(t, true, file["enable_socket"])
+	assert.Equal(t, float64(20000), file["tool_response_limit"])
+	assert.Equal(t, "full", file["tool_response_mode"])
+	assert.Equal(t, "full", file["direct_tool_response_mode"])
+	logging, _ := file["logging"].(map[string]any)
+	assert.Equal(t, "info", logging["level"])
+	assert.Equal(t, true, logging["enable_file"])
+	assert.NotEqual(t, cfg.Logging.LogDir, logging["log_dir"], "--log-dir leaked")
+	assert.NotEqual(t, true, file["debug_search"])
+	assert.NotEqual(t, true, file["require_mcp_auth"])
+	assert.NotEqual(t, true, file["read_only_mode"])
+	assert.NotEqual(t, true, file["disable_management"])
+	assert.NotEqual(t, false, file["allow_server_add"])
+	assert.NotEqual(t, false, file["allow_server_remove"])
+	assert.NotEqual(t, false, file["enable_prompts"])
+	assert.NotEqual(t, true, file["aggregate_upstream_prompts"])
+	assert.Equal(t, float64(42), file["tools_limit"])
+}
+
+// A field the API edits after the flag was applied is a real change and is
+// persisted, flag or no flag.
+func TestServeFlagOverrideEditedViaAPIIsPersisted(t *testing.T) {
+	saveServeGlobals(t)
+	t.Cleanup(config.ResetProcessOverrides)
+	config.ResetProcessOverrides()
+	path := writeServeFlagTestConfig(t)
+	configFile, dataDir = path, filepath.Dir(path)
+
+	cmd := newServeRuntimeFlagTestCmd()
+	require.NoError(t, cmd.ParseFlags([]string{"--listen", ":0", "--read-only"}))
+	cfg, _, err := loadConfig(cmd)
+	require.NoError(t, err)
+	applyServeRuntimeFlags(cmd, cfg)
+
+	cfg.Listen = "127.0.0.1:9090" // the Settings page
+	cfg.ReadOnlyMode = false      // toggled back off
+	require.NoError(t, config.SaveConfig(cfg, path))
+
+	file := readConfigFileJSON(t, path)
+	assert.Equal(t, "127.0.0.1:9090", file["listen"])
+	assert.Equal(t, false, file["read_only_mode"])
+}
+
+// Both loadConfig and runServer apply --tool-response-limit; the second
+// registration must not replace the recorded file value (the fallback when
+// the file cannot be read at save time) with the flag's own value.
+func TestServeFlagsRegisteredTwiceKeepTheFileFallback(t *testing.T) {
+	saveServeGlobals(t)
+	t.Cleanup(config.ResetProcessOverrides)
+	config.ResetProcessOverrides()
+	path := writeServeFlagTestConfig(t)
+	configFile, dataDir = path, filepath.Dir(path)
+
+	cmd := newServeRuntimeFlagTestCmd()
+	require.NoError(t, cmd.ParseFlags([]string{"--tool-response-limit", "500"}))
+	cfg, _, err := loadConfig(cmd)
+	require.NoError(t, err)
+	applyServeRuntimeFlags(cmd, cfg)
+	require.Equal(t, 500, cfg.ToolResponseLimit)
+
+	persisted := config.PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json"))
+	assert.Equal(t, 20000, persisted.ToolResponseLimit)
+}