From c48470689aa6f830b6da611b807665352ab549e1 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:08:40 +0300 Subject: [PATCH 01/18] fix(serve): stop persisting CLI flag overrides into the config file `serve` saves the config at three sites (auto-generated api_key, first-run telemetry notice, recordStartupOutcome) and wrote the in-memory cfg, which already carried every CLI flag override. `serve --listen :0` therefore wrote `"listen": ":0"` into mcp_config.json and the next unflagged start (or the tray-launched core) silently booted in stdio mode. loadConfig now snapshots the file-loaded config before any flag override and returns a serveConfigSaver; its save() writes that snapshot plus only the runtime-generated fields (api_key, telemetry). All three save sites use it, and recordStartupOutcome takes the save func as a parameter. The snapshot precedes runServer's own overrides too, so --read-only, --log-level, --disable-management etc. no longer leak either. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 61 +++++-- cmd/mcpproxy/serve_flag_persistence_test.go | 189 ++++++++++++++++++++ cmd/mcpproxy/startup_outcome.go | 11 +- cmd/mcpproxy/startup_outcome_test.go | 4 +- 4 files changed, 246 insertions(+), 19 deletions(-) create mode 100644 cmd/mcpproxy/serve_flag_persistence_test.go diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 794e7c50e..219cf1feb 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -444,7 +444,7 @@ func runServer(cmd *cobra.Command, _ []string) error { cmdAggregateUpstreamPrompts, _ := cmd.Flags().GetBool("aggregate-upstream-prompts") // Load configuration first to get logging settings - cfg, err := loadConfig(cmd) + cfg, saver, err := loadConfig(cmd) if err != nil { return fmt.Errorf("failed to load configuration: %w", err) } @@ -608,7 +608,7 @@ func runServer(cmd *cobra.Command, _ []string) error { configPathToSave = config.GetConfigPath(cfg.DataDir) } - if err := config.SaveConfig(cfg, configPathToSave); err != nil { + if err := saver.save(cfg, configPathToSave); err != nil { logger.Warn("Failed to save auto-generated API key to config file", zap.Error(err), zap.String("config_path", configPathToSave)) @@ -637,7 +637,7 @@ func runServer(cmd *cobra.Command, _ []string) error { srv, err := server.NewServerWithConfigPath(cfg, actualConfigPath, logger) if err != nil { // Spec 042: classify the failure into a startup outcome enum. - recordStartupOutcome(cfg, actualConfigPath, classifyStartupError(err)) + recordStartupOutcome(cfg, actualConfigPath, classifyStartupError(err), saver.save) return fmt.Errorf("failed to create server: %w", err) } @@ -645,7 +645,7 @@ func runServer(cmd *cobra.Command, _ []string) error { // the user has not already seen it). Persist the flag so we never nag // twice. if telemetry.MaybePrintFirstRunNotice(cfg, os.Stderr) { - _ = config.SaveConfig(cfg, actualConfigPath) + _ = saver.save(cfg, actualConfigPath) } // Setup signal handling for graceful shutdown @@ -692,11 +692,11 @@ func runServer(cmd *cobra.Command, _ []string) error { logger.Info("Starting mcpproxy server") if err := srv.StartServer(ctx); err != nil { // Spec 042: classify the failure into a startup outcome enum. - recordStartupOutcome(cfg, actualConfigPath, classifyStartupError(err)) + recordStartupOutcome(cfg, actualConfigPath, classifyStartupError(err), saver.save) return fmt.Errorf("failed to start server: %w", err) } // Spec 042: clean start. - recordStartupOutcome(cfg, actualConfigPath, "success") + recordStartupOutcome(cfg, actualConfigPath, "success", saver.save) // Wait for context cancellation (signal) or a fatal serve failure select { @@ -717,7 +717,7 @@ func runServer(cmd *cobra.Command, _ []string) error { // conflicts) and can react via its state machine. logger.Error("Server failed, shutting down", zap.Error(err)) // Spec 042: overwrite the optimistic "success" outcome recorded above. - recordStartupOutcome(cfg, actualConfigPath, classifyStartupError(err)) + recordStartupOutcome(cfg, actualConfigPath, classifyStartupError(err), saver.save) srv.SetShutdownInfo("error", "") if shutdownErr := srv.Shutdown(); shutdownErr != nil { logger.Error("Error shutting down server", zap.Error(shutdownErr)) @@ -726,7 +726,40 @@ func runServer(cmd *cobra.Command, _ []string) error { } } -func loadConfig(cmd *cobra.Command) (*config.Config, error) { +// serveConfigSaver persists the config during `serve` without leaking the CLI +// flag overrides that loadConfig and runServer layer onto the in-memory config +// (--listen, --tray-endpoint, --enable-socket, --tool-response-*, --log-level, +// --read-only, ...). A flag applies to that one process only; persisting it +// would make the next unflagged start — or the tray-launched core — inherit a +// one-off choice (`--listen :0` used to write `"listen": ":0"` into the file, +// after which the core silently booted in stdio mode). +type serveConfigSaver struct { + // fileCfg is the config exactly as loaded from the file, before any flag + // override. Logging is the only pointer runServer mutates in place, so it + // is copied; the other pointer fields are shared and never overridden. + fileCfg config.Config +} + +func newServeConfigSaver(cfg *config.Config) *serveConfigSaver { + s := &serveConfigSaver{fileCfg: *cfg} + if cfg.Logging != nil { + logging := *cfg.Logging + s.fileCfg.Logging = &logging + } + return s +} + +// save writes the file-loaded values plus the only fields `serve` legitimately +// generates at runtime: the auto-generated API key and the telemetry state +// (last_startup_outcome, first-run notice flag). +func (s *serveConfigSaver) save(cfg *config.Config, path string) error { + persisted := s.fileCfg + persisted.APIKey = cfg.APIKey + persisted.Telemetry = cfg.Telemetry + return config.SaveConfig(&persisted, path) +} + +func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { var cfg *config.Config var err error @@ -739,7 +772,7 @@ func loadConfig(cmd *cobra.Command) (*config.Config, error) { // default here; the explicit one used to exit with "no such file or // directory" instead, which made that whole flow unusable. if _, ensureErr := config.EnsureConfigFile(configFile, dataDir); ensureErr != nil { - return nil, ensureErr + return nil, nil, ensureErr } cfg, err = config.LoadFromFile(configFile) } else { @@ -747,9 +780,13 @@ func loadConfig(cmd *cobra.Command) (*config.Config, error) { } if err != nil { - return nil, fmt.Errorf("failed to load configuration: %w", err) + return nil, nil, fmt.Errorf("failed to load configuration: %w", err) } + // Snapshot the file-loaded values before any flag override so the saves + // in runServer never persist a one-off CLI choice. + saver := newServeConfigSaver(cfg) + // Override with command line flags ONLY if they were explicitly set if dataDir != "" { cfg.DataDir = dataDir @@ -774,10 +811,10 @@ func loadConfig(cmd *cobra.Command) (*config.Config, error) { // Validate the configuration if err := cfg.Validate(); err != nil { - return nil, fmt.Errorf("invalid configuration: %w", err) + return nil, nil, fmt.Errorf("invalid configuration: %w", err) } - return cfg, nil + return cfg, saver, nil } // applyToolResponseModeFlag applies the --tool-response-mode serve flag onto diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go new file mode 100644 index 000000000..de60364f3 --- /dev/null +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -0,0 +1,189 @@ +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// newServeFlagTestCmd builds a cobra command carrying the `serve` flags that +// loadConfig reads, bound to the same package globals as the real command. +func newServeFlagTestCmd() *cobra.Command { + cmd := &cobra.Command{Use: "serve"} + cmd.Flags().StringVarP(&listen, "listen", "l", "", "") + cmd.Flags().StringVar(&trayEndpoint, "tray-endpoint", "", "") + cmd.Flags().BoolVar(&enableSocket, "enable-socket", true, "") + cmd.Flags().IntVar(&toolResponseLimit, "tool-response-limit", 0, "") + cmd.Flags().StringVar(&toolResponseMode, "tool-response-mode", "", "") + cmd.Flags().StringVar(&directToolResponseMode, "direct-tool-response-mode", "", "") + return cmd +} + +// writeServeFlagTestConfig writes a config file whose values differ from every +// flag the test passes, so a leaked override is visible in the file. +func writeServeFlagTestConfig(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + path := filepath.Join(tmp, "mcp_config.json") + raw := `{ + "listen": "127.0.0.1:8080", + "data_dir": ` + jsonString(tmp) + `, + "enable_socket": true, + "tool_response_limit": 20000, + "tool_response_mode": "full", + "direct_tool_response_mode": "full", + "logging": {"level": "info", "enable_file": true, "enable_console": true, "filename": "main.log"}, + "mcpServers": [] +}` + require.NoError(t, os.WriteFile(path, []byte(raw), 0o600)) + return path +} + +func jsonString(s string) string { + b, _ := json.Marshal(s) + return string(b) +} + +func readConfigFileJSON(t *testing.T, path string) map[string]any { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + return m +} + +// saveServeGlobals snapshots the package globals the serve flags are bound to +// and restores them when the test ends. Tests here mutate globals, so they +// must not use t.Parallel. +func saveServeGlobals(t *testing.T) { + t.Helper() + oldConfigFile, oldDataDir := configFile, dataDir + oldListen, oldTray, oldSocket := listen, trayEndpoint, enableSocket + oldLimit, oldMode, oldDirect := toolResponseLimit, toolResponseMode, directToolResponseMode + t.Cleanup(func() { + configFile, dataDir = oldConfigFile, oldDataDir + listen, trayEndpoint, enableSocket = oldListen, oldTray, oldSocket + toolResponseLimit, toolResponseMode, directToolResponseMode = oldLimit, oldMode, oldDirect + }) +} + +// A `serve` CLI flag applies to that one process only. The saves runServer +// performs (auto-generated API key, first-run telemetry notice, startup +// outcome) must write the file-loaded values back, not the flag overrides — +// otherwise `serve --listen :0` writes `"listen": ":0"` into the file and the +// next unflagged start (or the tray-launched core) boots in stdio mode. +func TestServeFlagOverridesAreNotPersisted(t *testing.T) { + cases := []struct { + name string + args []string + key string + want any + inMem func(t *testing.T, cfg *config.Config) + }{ + { + name: "listen :0", + args: []string{"--listen", ":0"}, + key: "listen", want: "127.0.0.1:8080", + inMem: func(t *testing.T, cfg *config.Config) { assert.Equal(t, ":0", cfg.Listen) }, + }, + { + name: "listen explicit address", + args: []string{"--listen", "127.0.0.1:9999"}, + key: "listen", want: "127.0.0.1:8080", + inMem: func(t *testing.T, cfg *config.Config) { assert.Equal(t, "127.0.0.1:9999", cfg.Listen) }, + }, + { + name: "tray-endpoint", + args: []string{"--tray-endpoint", "unix:///tmp/x.sock"}, + key: "tray_endpoint", want: nil, + inMem: func(t *testing.T, cfg *config.Config) { assert.Equal(t, "unix:///tmp/x.sock", cfg.TrayEndpoint) }, + }, + { + name: "enable-socket=false", + args: []string{"--enable-socket=false"}, + key: "enable_socket", want: true, + inMem: func(t *testing.T, cfg *config.Config) { assert.False(t, cfg.EnableSocket) }, + }, + { + name: "tool-response-limit", + args: []string{"--tool-response-limit", "500"}, + key: "tool_response_limit", want: float64(20000), + inMem: func(t *testing.T, cfg *config.Config) { assert.Equal(t, 500, cfg.ToolResponseLimit) }, + }, + { + name: "tool-response-mode", + args: []string{"--tool-response-mode", "compact"}, + key: "tool_response_mode", want: "full", + inMem: func(t *testing.T, cfg *config.Config) { assert.Equal(t, "compact", cfg.ToolResponseMode) }, + }, + { + name: "direct-tool-response-mode", + args: []string{"--direct-tool-response-mode", "deferred"}, + key: "direct_tool_response_mode", want: "full", + inMem: func(t *testing.T, cfg *config.Config) { assert.Equal(t, "deferred", cfg.DirectToolResponseMode) }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cmd := newServeFlagTestCmd() + require.NoError(t, cmd.ParseFlags(tc.args)) + + cfg, saver, err := loadConfig(cmd) + require.NoError(t, err) + tc.inMem(t, cfg) + + // The API-key save site: the generated key must land in the file + // while the flag override must not. + cfg.APIKey = "mcp_test_generated_key" + require.NoError(t, saver.save(cfg, path)) + + file := readConfigFileJSON(t, path) + assert.Equal(t, "mcp_test_generated_key", file["api_key"], "runtime-generated api_key must persist") + assert.Equal(t, tc.want, file[tc.key], "flag override leaked into %s", tc.key) + }) + } +} + +// The startup-outcome and first-run-notice saves reuse the same path: they +// persist only the telemetry fields they own, on top of the file-loaded values. +func TestServeSaverPersistsTelemetryWithoutFlagOverrides(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cmd := newServeFlagTestCmd() + require.NoError(t, cmd.ParseFlags([]string{"--listen", ":0"})) + cfg, saver, err := loadConfig(cmd) + require.NoError(t, err) + require.Equal(t, ":0", cfg.Listen) + + // Simulate the in-place mutations runServer makes before its saves. + cfg.Logging.Level = "debug" + cfg.ReadOnlyMode = true + + recordStartupOutcome(cfg, path, "success", saver.save) + cfg.Telemetry.NoticeShown = true + require.NoError(t, saver.save(cfg, path)) + + file := readConfigFileJSON(t, path) + assert.Equal(t, "127.0.0.1:8080", file["listen"]) + assert.Equal(t, false, file["read_only_mode"], "runServer flag override leaked") + logging, _ := file["logging"].(map[string]any) + assert.Equal(t, "info", logging["level"], "log-level override leaked") + telemetry, _ := file["telemetry"].(map[string]any) + assert.Equal(t, "success", telemetry["last_startup_outcome"]) + assert.Equal(t, true, telemetry["notice_shown"]) +} diff --git a/cmd/mcpproxy/startup_outcome.go b/cmd/mcpproxy/startup_outcome.go index e932e0e2d..4860ec760 100644 --- a/cmd/mcpproxy/startup_outcome.go +++ b/cmd/mcpproxy/startup_outcome.go @@ -26,10 +26,11 @@ func classifyStartupError(err error) string { } } -// recordStartupOutcome persists the last startup outcome to the config file. -// Spec 042 User Story 5. The next heartbeat reads this value into the payload -// as last_startup_outcome. -func recordStartupOutcome(cfg *config.Config, configPath, outcome string) { +// recordStartupOutcome persists the last startup outcome to the config file +// via save (serveConfigSaver.save in runServer, so CLI flag overrides are not +// written along with it). Spec 042 User Story 5. The next heartbeat reads this +// value into the payload as last_startup_outcome. +func recordStartupOutcome(cfg *config.Config, configPath, outcome string, save func(*config.Config, string) error) { if cfg == nil { return } @@ -43,7 +44,7 @@ func recordStartupOutcome(cfg *config.Config, configPath, outcome string) { if configPath == "" { return } - if err := config.SaveConfig(cfg, configPath); err != nil { + if err := save(cfg, configPath); err != nil { // Best-effort; telemetry must never block startup. zap.L().Debug("Failed to persist last_startup_outcome", zap.String("outcome", outcome), diff --git a/cmd/mcpproxy/startup_outcome_test.go b/cmd/mcpproxy/startup_outcome_test.go index 01571cf44..9f0da990a 100644 --- a/cmd/mcpproxy/startup_outcome_test.go +++ b/cmd/mcpproxy/startup_outcome_test.go @@ -31,7 +31,7 @@ func TestRecordStartupOutcomeMapping(t *testing.T) { func TestRecordStartupOutcomePersists(t *testing.T) { cfg := &config.Config{} - recordStartupOutcome(cfg, "", "success") + recordStartupOutcome(cfg, "", "success", config.SaveConfig) if cfg.Telemetry == nil { t.Fatal("Telemetry should be initialized") @@ -41,7 +41,7 @@ func TestRecordStartupOutcomePersists(t *testing.T) { } // Idempotent: second call with same outcome leaves the field unchanged. - recordStartupOutcome(cfg, "", "success") + recordStartupOutcome(cfg, "", "success", config.SaveConfig) if cfg.Telemetry.LastStartupOutcome != "success" { t.Errorf("LastStartupOutcome changed: %q", cfg.Telemetry.LastStartupOutcome) } From 0e85539f85c10a5e6a9fd14ff2cc714ea071e042 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:18:04 +0300 Subject: [PATCH 02/18] fix(serve): base serve saves on the current file, never persist env API key Cross-review round 1 (gpt-5.6-sol): the fatal-serve-error save can fire hours after startup and would have written the startup-era snapshot over servers the runtime persisted since; and Validate() copies MCPPROXY_API_KEY into cfg.APIKey, so every save wrote that secret to disk. serveConfigSaver.save now re-reads the file as its base (snapshot only if the file is unreadable), takes api_key from the raw file, and overlays only the key serve generated itself plus cfg.Telemetry. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 58 ++++++++++++++++++--- cmd/mcpproxy/serve_flag_persistence_test.go | 57 ++++++++++++++++++++ 2 files changed, 107 insertions(+), 8 deletions(-) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 219cf1feb..561a58b0f 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -25,6 +25,7 @@ package main import ( "context" + "encoding/json" "errors" "fmt" "os" @@ -601,6 +602,7 @@ func runServer(cmd *cobra.Command, _ []string) error { logger.Warn(frameMsg) // Save the auto-generated key to config file for persistence + saver.setGeneratedAPIKey(apiKey) var configPathToSave string if configFile != "" { configPathToSave = configFile @@ -733,11 +735,22 @@ func runServer(cmd *cobra.Command, _ []string) error { // would make the next unflagged start — or the tray-launched core — inherit a // one-off choice (`--listen :0` used to write `"listen": ":0"` into the file, // after which the core silently booted in stdio mode). +// +// The three `serve` saves (auto-generated API key, first-run telemetry notice, +// startup outcome) own exactly two things: the generated key and the telemetry +// state. Everything else is written back from the current config FILE, so a +// save that fires late (the fatal-serve-error path can run hours after +// startup) never resurrects a startup-era server list over changes the runtime +// persisted in the meantime. type serveConfigSaver struct { - // fileCfg is the config exactly as loaded from the file, before any flag - // override. Logging is the only pointer runServer mutates in place, so it - // is copied; the other pointer fields are shared and never overridden. + // fileCfg is the config as loaded at startup, before any flag override. + // It is the base only when the file can no longer be read; Logging is + // copied because runServer mutates it in place. fileCfg config.Config + // generatedAPIKey is set only when serve generated the key itself. A key + // from MCPPROXY_API_KEY is an override like any flag and must not be + // copied into the file. + generatedAPIKey string } func newServeConfigSaver(cfg *config.Config) *serveConfigSaver { @@ -749,16 +762,45 @@ func newServeConfigSaver(cfg *config.Config) *serveConfigSaver { return s } -// save writes the file-loaded values plus the only fields `serve` legitimately -// generates at runtime: the auto-generated API key and the telemetry state -// (last_startup_outcome, first-run notice flag). +// setGeneratedAPIKey marks key as generated by this process so save persists it. +func (s *serveConfigSaver) setGeneratedAPIKey(key string) { + s.generatedAPIKey = key +} + +// save writes the current file contents plus the fields `serve` owns: the +// API key it generated (if any) and cfg.Telemetry (last_startup_outcome, +// first-run notice flag). func (s *serveConfigSaver) save(cfg *config.Config, path string) error { - persisted := s.fileCfg - persisted.APIKey = cfg.APIKey + var persisted config.Config + if onDisk, err := config.LoadFromFile(path); err == nil { + persisted = *onDisk + } else { + persisted = s.fileCfg + } + // Loading runs Validate, which copies MCPPROXY_API_KEY into APIKey; take + // the key from the raw file so an env secret is never written to disk. + persisted.APIKey = rawFileAPIKey(path) + if s.generatedAPIKey != "" { + persisted.APIKey = s.generatedAPIKey + } persisted.Telemetry = cfg.Telemetry return config.SaveConfig(&persisted, path) } +// rawFileAPIKey returns the api_key literally present in the config file +// ("" when absent or unreadable), bypassing the env override Validate applies. +func rawFileAPIKey(path string) string { + data, err := os.ReadFile(path) + if err != nil { + return "" + } + var raw struct { + APIKey string `json:"api_key"` + } + _ = json.Unmarshal(data, &raw) + return raw.APIKey +} + func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { var cfg *config.Config var err error diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index de60364f3..62863b3ce 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -148,6 +148,7 @@ func TestServeFlagOverridesAreNotPersisted(t *testing.T) { // The API-key save site: the generated key must land in the file // while the flag override must not. cfg.APIKey = "mcp_test_generated_key" + saver.setGeneratedAPIKey(cfg.APIKey) require.NoError(t, saver.save(cfg, path)) file := readConfigFileJSON(t, path) @@ -187,3 +188,59 @@ func TestServeSaverPersistsTelemetryWithoutFlagOverrides(t *testing.T) { assert.Equal(t, "success", telemetry["last_startup_outcome"]) assert.Equal(t, true, telemetry["notice_shown"]) } + +// An API key that came from MCPPROXY_API_KEY is an override like any flag: the +// saves must not copy that secret into the file. Only a key `serve` generated +// itself is persisted. +func TestServeSaverDoesNotPersistEnvAPIKey(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + t.Setenv("MCPPROXY_API_KEY", "mcp_env_secret") + + cfg, saver, err := loadConfig(newServeFlagTestCmd()) + require.NoError(t, err) + apiKey, wasGenerated, _ := cfg.EnsureAPIKey() + require.Equal(t, "mcp_env_secret", apiKey) + require.False(t, wasGenerated) + + recordStartupOutcome(cfg, path, "success", saver.save) + + file := readConfigFileJSON(t, path) + assert.Nil(t, file["api_key"], "env API key leaked into the config file") + telemetry, _ := file["telemetry"].(map[string]any) + assert.Equal(t, "success", telemetry["last_startup_outcome"]) +} + +// The fatal-serve-error save can fire hours after startup, by which time the +// runtime has persisted its own changes (servers added via the API, quarantine +// decisions). That save must layer the telemetry fields onto the CURRENT file, +// not resurrect the startup-time snapshot. +func TestServeSaverKeepsChangesTheRuntimePersistedLater(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cmd := newServeFlagTestCmd() + require.NoError(t, cmd.ParseFlags([]string{"--listen", ":0"})) + cfg, saver, err := loadConfig(cmd) + require.NoError(t, err) + + // Simulate a runtime save that landed after startup: a server was added + // and a top-level setting changed. + onDisk, err := config.LoadFromFile(path) + require.NoError(t, err) + onDisk.Servers = append(onDisk.Servers, &config.ServerConfig{Name: "added-later", URL: "http://127.0.0.1:1/mcp", Protocol: "http", Enabled: true}) + onDisk.ToolsLimit = 7 + require.NoError(t, config.SaveConfig(onDisk, path)) + + recordStartupOutcome(cfg, path, "other_error", saver.save) + + file := readConfigFileJSON(t, path) + servers, _ := file["mcpServers"].([]any) + require.Len(t, servers, 1, "server added after startup was clobbered") + assert.Equal(t, float64(7), file["tools_limit"], "setting changed after startup was clobbered") + assert.Equal(t, "127.0.0.1:8080", file["listen"]) + telemetry, _ := file["telemetry"].(map[string]any) + assert.Equal(t, "other_error", telemetry["last_startup_outcome"]) +} From 788e8b31bb994f287a8046d13674d06a19bf7b4f Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:27:10 +0300 Subject: [PATCH 03/18] fix(serve): read the merge base without loader side effects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-review round 2 (gpt-5.6-sol): config.LoadFromFile is not a read — it applies MCPPROXY_* env overrides, copies MCPPROXY_API_KEY into api_key via Validate, creates data_dir and replaces the process-global registry list; the fallback path dropped a configured key when the file vanished; and a startup-generated key overrode a key rotated later via the API. The base is now DefaultConfig + json.Unmarshal of the file (snapshot of the same at startup as fallback), and the generated key only fills an empty api_key. This also stops env overrides leaking on the serve path. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 114 +++++++++++--------- cmd/mcpproxy/serve_flag_persistence_test.go | 62 +++++++++++ 2 files changed, 123 insertions(+), 53 deletions(-) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 561a58b0f..0a2afa92b 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -603,12 +603,7 @@ func runServer(cmd *cobra.Command, _ []string) error { // Save the auto-generated key to config file for persistence saver.setGeneratedAPIKey(apiKey) - var configPathToSave string - if configFile != "" { - configPathToSave = configFile - } else { - configPathToSave = config.GetConfigPath(cfg.DataDir) - } + configPathToSave := serveConfigPath(cfg) if err := saver.save(cfg, configPathToSave); err != nil { logger.Warn("Failed to save auto-generated API key to config file", @@ -629,13 +624,7 @@ func runServer(cmd *cobra.Command, _ []string) error { } // Create server with the actual config path used - var actualConfigPath string - if configFile != "" { - actualConfigPath = configFile - } else { - // When using default config, still track the actual path used - actualConfigPath = config.GetConfigPath(cfg.DataDir) - } + actualConfigPath := serveConfigPath(cfg) srv, err := server.NewServerWithConfigPath(cfg, actualConfigPath, logger) if err != nil { // Spec 042: classify the failure into a startup outcome enum. @@ -728,33 +717,40 @@ func runServer(cmd *cobra.Command, _ []string) error { } } -// serveConfigSaver persists the config during `serve` without leaking the CLI -// flag overrides that loadConfig and runServer layer onto the in-memory config -// (--listen, --tray-endpoint, --enable-socket, --tool-response-*, --log-level, -// --read-only, ...). A flag applies to that one process only; persisting it -// would make the next unflagged start — or the tray-launched core — inherit a -// one-off choice (`--listen :0` used to write `"listen": ":0"` into the file, -// after which the core silently booted in stdio mode). +// serveConfigSaver persists the config during `serve` without leaking the +// process-only overrides that loadConfig and runServer layer onto the in-memory +// config: CLI flags (--listen, --tray-endpoint, --enable-socket, +// --tool-response-*, --log-level, --read-only, ...) and MCPPROXY_* env values. +// An override applies to that one process only; persisting it would make the +// next unflagged start — or the tray-launched core — inherit a one-off choice +// (`--listen :0` used to write `"listen": ":0"` into the file, after which the +// core silently booted in stdio mode). // // The three `serve` saves (auto-generated API key, first-run telemetry notice, // startup outcome) own exactly two things: the generated key and the telemetry -// state. Everything else is written back from the current config FILE, so a -// save that fires late (the fatal-serve-error path can run hours after -// startup) never resurrects a startup-era server list over changes the runtime -// persisted in the meantime. +// state. Everything else is written back from the config FILE as it is at save +// time, so a save that fires late (the fatal-serve-error path can run hours +// after startup) never resurrects a startup-era server list over changes the +// runtime persisted in the meantime. type serveConfigSaver struct { - // fileCfg is the config as loaded at startup, before any flag override. - // It is the base only when the file can no longer be read; Logging is - // copied because runServer mutates it in place. + // fileCfg is the file as read at startup (see readConfigFile), the base + // only when the file can no longer be read at save time. fileCfg config.Config - // generatedAPIKey is set only when serve generated the key itself. A key - // from MCPPROXY_API_KEY is an override like any flag and must not be - // copied into the file. + // generatedAPIKey is set only when serve generated the key itself. It + // fills an empty api_key; a key rotated through the API since is kept. generatedAPIKey string } -func newServeConfigSaver(cfg *config.Config) *serveConfigSaver { - s := &serveConfigSaver{fileCfg: *cfg} +// newServeConfigSaver snapshots the file at path; when it cannot be read +// (e.g. --config=/dev/null) the loaded cfg — taken before any flag override, +// Logging copied because runServer mutates it in place — stands in. +func newServeConfigSaver(cfg *config.Config, path string) *serveConfigSaver { + s := &serveConfigSaver{} + if fileCfg, err := readConfigFile(path); err == nil { + s.fileCfg = *fileCfg + return s + } + s.fileCfg = *cfg if cfg.Logging != nil { logging := *cfg.Logging s.fileCfg.Logging = &logging @@ -768,37 +764,49 @@ func (s *serveConfigSaver) setGeneratedAPIKey(key string) { } // save writes the current file contents plus the fields `serve` owns: the -// API key it generated (if any) and cfg.Telemetry (last_startup_outcome, -// first-run notice flag). +// API key it generated (only into an empty api_key) and cfg.Telemetry +// (last_startup_outcome, first-run notice flag). func (s *serveConfigSaver) save(cfg *config.Config, path string) error { - var persisted config.Config - if onDisk, err := config.LoadFromFile(path); err == nil { + persisted := s.fileCfg + if onDisk, err := readConfigFile(path); err == nil { persisted = *onDisk - } else { - persisted = s.fileCfg } - // Loading runs Validate, which copies MCPPROXY_API_KEY into APIKey; take - // the key from the raw file so an env secret is never written to disk. - persisted.APIKey = rawFileAPIKey(path) - if s.generatedAPIKey != "" { + if persisted.APIKey == "" { persisted.APIKey = s.generatedAPIKey } persisted.Telemetry = cfg.Telemetry return config.SaveConfig(&persisted, path) } -// rawFileAPIKey returns the api_key literally present in the config file -// ("" when absent or unreadable), bypassing the env override Validate applies. -func rawFileAPIKey(path string) string { +// readConfigFile decodes the config file over the defaults and nothing more. +// config.LoadFromFile is NOT a read: it applies MCPPROXY_* env overrides, +// copies MCPPROXY_API_KEY into api_key via Validate, creates data_dir and +// replaces the process-global registry list — none of which a save may do. +func readConfigFile(path string) (*config.Config, error) { data, err := os.ReadFile(path) if err != nil { - return "" + return nil, err } - var raw struct { - APIKey string `json:"api_key"` + cfg := config.DefaultConfig() + if err := json.Unmarshal(data, cfg); err != nil { + return nil, err + } + // Same stamp the loader applies, so a save never writes a zero time. + for _, server := range cfg.Servers { + if server.Created.IsZero() { + server.Created = time.Now() + } + } + return cfg, nil +} + +// serveConfigPath is the file runServer saves to: --config when given, else +// the default path under the data dir. +func serveConfigPath(cfg *config.Config) string { + if configFile != "" { + return configFile } - _ = json.Unmarshal(data, &raw) - return raw.APIKey + return config.GetConfigPath(cfg.DataDir) } func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { @@ -825,9 +833,9 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { return nil, nil, fmt.Errorf("failed to load configuration: %w", err) } - // Snapshot the file-loaded values before any flag override so the saves - // in runServer never persist a one-off CLI choice. - saver := newServeConfigSaver(cfg) + // Snapshot the file before any flag override so the saves in runServer + // never persist a one-off CLI choice. + saver := newServeConfigSaver(cfg, serveConfigPath(cfg)) // Override with command line flags ONLY if they were explicitly set if dataDir != "" { diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index 62863b3ce..14247c59e 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -244,3 +244,65 @@ func TestServeSaverKeepsChangesTheRuntimePersistedLater(t *testing.T) { telemetry, _ := file["telemetry"].(map[string]any) assert.Equal(t, "other_error", telemetry["last_startup_outcome"]) } + +// If the file vanished after startup, the fallback must recreate it with the +// key the file had — not drop it, and not substitute an env key. +func TestServeSaverFallbackKeepsFileAPIKey(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + raw, err := os.ReadFile(path) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, []byte(`{"api_key":"mcp_file_key",`+string(raw[1:])), 0o600)) + t.Setenv("MCPPROXY_API_KEY", "mcp_env_secret") + + cfg, saver, err := loadConfig(newServeFlagTestCmd()) + require.NoError(t, err) + require.NoError(t, os.Remove(path)) + + recordStartupOutcome(cfg, path, "success", saver.save) + + file := readConfigFileJSON(t, path) + assert.Equal(t, "mcp_file_key", file["api_key"]) + assert.Equal(t, "127.0.0.1:8080", file["listen"]) +} + +// A key serve generated at startup fills an EMPTY api_key only. If the key was +// rotated through the API and persisted later, a late save keeps the new one. +func TestServeSaverGeneratedKeyDoesNotOverrideRotatedKey(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + + cfg, saver, err := loadConfig(newServeFlagTestCmd()) + require.NoError(t, err) + saver.setGeneratedAPIKey("mcp_generated_at_startup") + require.NoError(t, saver.save(cfg, path)) + require.Equal(t, "mcp_generated_at_startup", readConfigFileJSON(t, path)["api_key"]) + + // Simulate a runtime-persisted key rotation. + onDisk := readConfigFileJSON(t, path) + onDisk["api_key"] = "mcp_rotated" + rotated, err := json.Marshal(onDisk) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, rotated, 0o600)) + + recordStartupOutcome(cfg, path, "other_error", saver.save) + assert.Equal(t, "mcp_rotated", readConfigFileJSON(t, path)["api_key"]) +} + +// The merge base is the file as written, not a full load: MCPPROXY_* env +// overrides (applied by config.LoadFromFile) must not be written back either. +func TestServeSaverBaseIgnoresEnvOverrides(t *testing.T) { + saveServeGlobals(t) + path := writeServeFlagTestConfig(t) + configFile, dataDir = path, filepath.Dir(path) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:1") + + cfg, saver, err := loadConfig(newServeFlagTestCmd()) + require.NoError(t, err) + require.Equal(t, "127.0.0.1:1", cfg.Listen, "env override applies to the process") + + recordStartupOutcome(cfg, path, "success", saver.save) + assert.Equal(t, "127.0.0.1:8080", readConfigFileJSON(t, path)["listen"], "env override leaked into the file") +} From e43194805a09bb458cd4a9d749be08d4b1a24704 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:30:49 +0300 Subject: [PATCH 04/18] fix(serve): merge base goes through the loader's read step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-review round 3 (gpt-5.6-sol): a bare json.Unmarshal skipped the read-time normalizations loadConfigFile applies (legacy "teams" → "server_edition", created stamps), so a serve-time save erased a legacy teams block. Export config.ReadFile — DefaultConfig + loadConfigFile, nothing else — and use it as the saver's base so it stays in lockstep with the loader. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 19 ++----------------- cmd/mcpproxy/serve_flag_persistence_test.go | 20 ++++++++++++++++++++ internal/config/loader.go | 13 +++++++++++++ 3 files changed, 35 insertions(+), 17 deletions(-) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 0a2afa92b..8bed0e2f4 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -25,7 +25,6 @@ package main import ( "context" - "encoding/json" "errors" "fmt" "os" @@ -778,26 +777,12 @@ func (s *serveConfigSaver) save(cfg *config.Config, path string) error { return config.SaveConfig(&persisted, path) } -// readConfigFile decodes the config file over the defaults and nothing more. +// readConfigFile is the side-effect-free read the saver merges into. // config.LoadFromFile is NOT a read: it applies MCPPROXY_* env overrides, // copies MCPPROXY_API_KEY into api_key via Validate, creates data_dir and // replaces the process-global registry list — none of which a save may do. func readConfigFile(path string) (*config.Config, error) { - data, err := os.ReadFile(path) - if err != nil { - return nil, err - } - cfg := config.DefaultConfig() - if err := json.Unmarshal(data, cfg); err != nil { - return nil, err - } - // Same stamp the loader applies, so a save never writes a zero time. - for _, server := range cfg.Servers { - if server.Created.IsZero() { - server.Created = time.Now() - } - } - return cfg, nil + return config.ReadFile(path) } // serveConfigPath is the file runServer saves to: --config when given, else diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index 14247c59e..632190619 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -306,3 +306,23 @@ func TestServeSaverBaseIgnoresEnvOverrides(t *testing.T) { recordStartupOutcome(cfg, path, "success", saver.save) assert.Equal(t, "127.0.0.1:8080", readConfigFileJSON(t, path)["listen"], "env override leaked into the file") } + +// The merge base must apply the loader's read-time normalizations (legacy +// "teams" → "server_edition", MCP-1086) or a serve-time save erases them. +func TestServeSaverKeepsLegacyTeamsBlock(t *testing.T) { + saveServeGlobals(t) + tmp := t.TempDir() + path := filepath.Join(tmp, "mcp_config.json") + raw := `{"listen":"127.0.0.1:8080","data_dir":` + jsonString(tmp) + `,"teams":{},"mcpServers":[]}` + require.NoError(t, os.WriteFile(path, []byte(raw), 0o600)) + configFile, dataDir = path, tmp + + cfg, saver, err := loadConfig(newServeFlagTestCmd()) + require.NoError(t, err) + require.NotNil(t, cfg.ServerEdition, "loader normalizes teams → server_edition") + + recordStartupOutcome(cfg, path, "success", saver.save) + + file := readConfigFileJSON(t, path) + assert.NotNil(t, file["server_edition"], "legacy teams block was erased by the save") +} diff --git a/internal/config/loader.go b/internal/config/loader.go index 8e8e62a28..2ac98f088 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -24,6 +24,19 @@ const ( falseValue = "false" ) +// ReadFile decodes the config file over the defaults and applies only the +// read-time normalizations (legacy key migration, created stamps). Unlike +// LoadFromFile it applies no env overrides, runs no validation, creates no +// directories and touches no process-global state, so it is safe as the base +// of a read-modify-write save while the server is running. +func ReadFile(configPath string) (*Config, error) { + cfg := DefaultConfig() + if err := loadConfigFile(configPath, cfg); err != nil { + return nil, err + } + return cfg, nil +} + // LoadFromFile loads configuration from a specific file func LoadFromFile(configPath string) (*Config, error) { cfg := DefaultConfig() From 6324e2da6499b50d05aff21fa5d8f153d4269906 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 06:35:29 +0300 Subject: [PATCH 05/18] fix(serve): save to the config file that was actually loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-review round 4 (gpt-5.6-sol): without --config, config.Load() discovers ./mcp_config.json or the home file but discarded the path, and runServer re-derived /mcp_config.json — a possibly different, never-loaded file that the merge would read as its base. Add config.LoadWithPath (Load keeps its signature) reporting the file it read or created, absolute; the saver carries that path and runServer uses it for every save and for the runtime's config path. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 25 ++++++------- cmd/mcpproxy/serve_flag_persistence_test.go | 30 ++++++++++++++++ internal/config/loader.go | 40 ++++++++++++++------- 3 files changed, 68 insertions(+), 27 deletions(-) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 8bed0e2f4..0eadf95e9 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -602,7 +602,7 @@ func runServer(cmd *cobra.Command, _ []string) error { // Save the auto-generated key to config file for persistence saver.setGeneratedAPIKey(apiKey) - configPathToSave := serveConfigPath(cfg) + configPathToSave := saver.path if err := saver.save(cfg, configPathToSave); err != nil { logger.Warn("Failed to save auto-generated API key to config file", @@ -622,8 +622,8 @@ func runServer(cmd *cobra.Command, _ []string) error { zap.String("api_key_prefix", maskedKey)) } - // Create server with the actual config path used - actualConfigPath := serveConfigPath(cfg) + // Create server with the config path that was actually loaded + actualConfigPath := saver.path srv, err := server.NewServerWithConfigPath(cfg, actualConfigPath, logger) if err != nil { // Spec 042: classify the failure into a startup outcome enum. @@ -732,6 +732,9 @@ func runServer(cmd *cobra.Command, _ []string) error { // after startup) never resurrects a startup-era server list over changes the // runtime persisted in the meantime. type serveConfigSaver struct { + // path is the config file loadConfig actually read (or created): the + // destination of every serve save and the runtime's config path. + path string // fileCfg is the file as read at startup (see readConfigFile), the base // only when the file can no longer be read at save time. fileCfg config.Config @@ -744,7 +747,7 @@ type serveConfigSaver struct { // (e.g. --config=/dev/null) the loaded cfg — taken before any flag override, // Logging copied because runServer mutates it in place — stands in. func newServeConfigSaver(cfg *config.Config, path string) *serveConfigSaver { - s := &serveConfigSaver{} + s := &serveConfigSaver{path: path} if fileCfg, err := readConfigFile(path); err == nil { s.fileCfg = *fileCfg return s @@ -785,18 +788,10 @@ func readConfigFile(path string) (*config.Config, error) { return config.ReadFile(path) } -// serveConfigPath is the file runServer saves to: --config when given, else -// the default path under the data dir. -func serveConfigPath(cfg *config.Config) string { - if configFile != "" { - return configFile - } - return config.GetConfigPath(cfg.DataDir) -} - func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { var cfg *config.Config var err error + loadedPath := configFile // Load configuration - use LoadFromFile if config file specified, otherwise use Load if configFile != "" { @@ -811,7 +806,7 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { } cfg, err = config.LoadFromFile(configFile) } else { - cfg, err = config.Load() + cfg, loadedPath, err = config.LoadWithPath() } if err != nil { @@ -820,7 +815,7 @@ func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { // Snapshot the file before any flag override so the saves in runServer // never persist a one-off CLI choice. - saver := newServeConfigSaver(cfg, serveConfigPath(cfg)) + saver := newServeConfigSaver(cfg, loadedPath) // Override with command line flags ONLY if they were explicitly set if dataDir != "" { diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index 632190619..d5793fd2f 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -326,3 +326,33 @@ func TestServeSaverKeepsLegacyTeamsBlock(t *testing.T) { file := readConfigFileJSON(t, path) assert.NotNil(t, file["server_edition"], "legacy teams block was erased by the save") } + +// Without --config, config.Load() discovers ./mcp_config.json (or the home +// file). The saves must go to THAT file, not to /mcp_config.json, +// which may be an unrelated config the merge would otherwise read as its base. +func TestServeSaverUsesTheDiscoveredConfigPath(t *testing.T) { + saveServeGlobals(t) + cwd := t.TempDir() + t.Chdir(cwd) + other := filepath.Join(t.TempDir(), "data") + require.NoError(t, os.MkdirAll(other, 0o700)) + loaded := filepath.Join(cwd, "mcp_config.json") + unrelated := filepath.Join(other, "mcp_config.json") + require.NoError(t, os.WriteFile(loaded, []byte(`{"listen":"127.0.0.1:8080","data_dir":`+jsonString(other)+`,"tools_limit":11,"mcpServers":[]}`), 0o600)) + require.NoError(t, os.WriteFile(unrelated, []byte(`{"listen":"127.0.0.1:7","tools_limit":99,"mcpServers":[]}`), 0o600)) + configFile, dataDir = "", "" + + cfg, saver, err := loadConfig(newServeFlagTestCmd()) + require.NoError(t, err) + require.Equal(t, 11, cfg.ToolsLimit, "config.Load() picked ./mcp_config.json") + assert.Equal(t, loaded, saver.path) + + recordStartupOutcome(cfg, saver.path, "success", saver.save) + + got := readConfigFileJSON(t, loaded) + assert.Equal(t, float64(11), got["tools_limit"]) + telemetry, _ := got["telemetry"].(map[string]any) + assert.Equal(t, "success", telemetry["last_startup_outcome"]) + untouched := readConfigFileJSON(t, unrelated) + assert.Nil(t, untouched["telemetry"], "save landed in the unrelated config") +} diff --git a/internal/config/loader.go b/internal/config/loader.go index 2ac98f088..cdb471c22 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -101,6 +101,14 @@ func warnNormalizedTrustModes(cfg *Config) { // Load loads configuration from file, environment, and defaults func Load() (*Config, error) { + cfg, _, err := LoadWithPath() + return cfg, err +} + +// LoadWithPath is Load that also reports which config file it read (or +// created), so callers that persist later write to the file they loaded +// rather than to a path re-derived from data_dir. +func LoadWithPath() (*Config, string, error) { cfg := DefaultConfig() // Set up viper @@ -111,15 +119,22 @@ func Load() (*Config, error) { configFileAutoLoaded := false if configPath != "" { if err := loadConfigFile(configPath, cfg); err != nil { - return nil, fmt.Errorf("failed to load config file %s: %w", configPath, err) + return nil, "", fmt.Errorf("failed to load config file %s: %w", configPath, err) } } else { // Try to find config file in common locations - configFound, _, err := findAndLoadConfigFile(cfg) + configFound, foundPath, err := findAndLoadConfigFile(cfg) if err != nil && configFound { - return nil, err // Only return error if config was found but couldn't be loaded + return nil, "", err // Only return error if config was found but couldn't be loaded } configFileAutoLoaded = configFound + // Discovery returns "mcp_config.json" for the cwd hit; report it + // absolute so later saves do not depend on the working directory. + if abs, absErr := filepath.Abs(foundPath); configFound && absErr == nil { + configPath = abs + } else { + configPath = foundPath + } // If no config file was found, create a default one if !configFound { @@ -127,21 +142,22 @@ func Load() (*Config, error) { if cfg.DataDir == "" { homeDir, err := os.UserHomeDir() if err != nil { - return nil, fmt.Errorf("failed to get user home directory: %w", err) + return nil, "", fmt.Errorf("failed to get user home directory: %w", err) } cfg.DataDir = filepath.Join(homeDir, DefaultDataDir) } // Create data directory if it doesn't exist if err := os.MkdirAll(cfg.DataDir, 0700); err != nil { - return nil, fmt.Errorf("failed to create data directory %s: %w", cfg.DataDir, err) + return nil, "", fmt.Errorf("failed to create data directory %s: %w", cfg.DataDir, err) } // Create default config file defaultConfigPath := filepath.Join(cfg.DataDir, ConfigFileName) if err := createDefaultConfigFile(defaultConfigPath, cfg); err != nil { - return nil, fmt.Errorf("failed to create default config file: %w", err) + return nil, "", fmt.Errorf("failed to create default config file: %w", err) } + configPath = defaultConfigPath fmt.Fprintf(os.Stderr, "INFO: Created default configuration file at %s\n", defaultConfigPath) } @@ -152,7 +168,7 @@ func Load() (*Config, error) { if !configFileAutoLoaded { // Override with viper (CLI flags and env vars) if err := viper.Unmarshal(cfg); err != nil { - return nil, fmt.Errorf("failed to unmarshal config: %w", err) + return nil, "", fmt.Errorf("failed to unmarshal config: %w", err) } } @@ -160,7 +176,7 @@ func Load() (*Config, error) { if cfg.DataDir == "" { homeDir, err := os.UserHomeDir() if err != nil { - return nil, fmt.Errorf("failed to get user home directory: %w", err) + return nil, "", fmt.Errorf("failed to get user home directory: %w", err) } cfg.DataDir = filepath.Join(homeDir, DefaultDataDir) } @@ -173,7 +189,7 @@ func Load() (*Config, error) { // these are invalid path characters on Windows and the directory can't be created anyway. if !strings.Contains(cfg.DataDir, "${") { if err := os.MkdirAll(cfg.DataDir, 0700); err != nil { - return nil, fmt.Errorf("failed to create data directory %s: %w", cfg.DataDir, err) + return nil, "", fmt.Errorf("failed to create data directory %s: %w", cfg.DataDir, err) } } @@ -181,7 +197,7 @@ func Load() (*Config, error) { upstreamList := viper.GetStringSlice("upstream") for _, upstream := range upstreamList { if err := parseUpstreamServer(upstream, cfg); err != nil { - return nil, fmt.Errorf("failed to parse upstream server %s: %w", upstream, err) + return nil, "", fmt.Errorf("failed to parse upstream server %s: %w", upstream, err) } } @@ -193,13 +209,13 @@ func Load() (*Config, error) { // Validate configuration if err := cfg.Validate(); err != nil { - return nil, fmt.Errorf("invalid configuration: %w", err) + return nil, "", fmt.Errorf("invalid configuration: %w", err) } // Initialize registries from config initializeRegistries(cfg) - return cfg, nil + return cfg, configPath, nil } // setupViper configures viper with environment variable handling From 384e84baafca34677929f394f5ed1924aa8dba92 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:26:00 +0300 Subject: [PATCH 06/18] fix(config): rename ReadFile to DecodeConfigFile for the oauth door scan TestNoDoorPublishesARawServerLeaf keys config-returning functions by bare name and requires unanimity; a config.ReadFile that returns *Config tainted every os.ReadFile call in the scanned trees (flagged internal/tray/managers.go:loadIcon). All five red CI jobs were this one test. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 2 +- internal/config/loader.go | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 0eadf95e9..f1b753317 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -785,7 +785,7 @@ func (s *serveConfigSaver) save(cfg *config.Config, path string) error { // copies MCPPROXY_API_KEY into api_key via Validate, creates data_dir and // replaces the process-global registry list — none of which a save may do. func readConfigFile(path string) (*config.Config, error) { - return config.ReadFile(path) + return config.DecodeConfigFile(path) } func loadConfig(cmd *cobra.Command) (*config.Config, *serveConfigSaver, error) { diff --git a/internal/config/loader.go b/internal/config/loader.go index cdb471c22..20fef023b 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -24,12 +24,14 @@ const ( falseValue = "false" ) -// ReadFile decodes the config file over the defaults and applies only the -// read-time normalizations (legacy key migration, created stamps). Unlike +// DecodeConfigFile decodes the config file over the defaults and applies only +// the read-time normalizations (legacy key migration, created stamps). Unlike // LoadFromFile it applies no env overrides, runs no validation, creates no // directories and touches no process-global state, so it is safe as the base -// of a read-modify-write save while the server is running. -func ReadFile(configPath string) (*Config, error) { +// of a read-modify-write save while the server is running. (Not "ReadFile": +// the oauth door scan keys config-returning functions by bare name and would +// taint every os.ReadFile call in the tree.) +func DecodeConfigFile(configPath string) (*Config, error) { cfg := DefaultConfig() if err := loadConfigFile(configPath, cfg); err != nil { return nil, err From 988fdafbbc6b7faa09360ba2a39d09061626e77d Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 09:58:00 +0300 Subject: [PATCH 07/18] fix(config): split persisted config from effective config for process-only overrides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every persist path saved the LIVE config — the file plus the `serve` CLI flags, the MCPPROXY_* env overrides and the MCPPROXY_API_KEY that Validate folds into api_key. PR #1299 fixed serve's own three saves; the runtime's SaveConfiguration (every API-driven server change) and telemetry's first-run anonymous_id write still marshalled the whole effective config, so `serve --listen :0` (or MCPPROXY_LISTEN, or the env API key) landed in mcp_config.json the first time a server was enabled from the Web UI. The fix is a process-wide override registry in internal/config: - OverrideForProcess(cfg, Field, source, value) sets the effective value and records (field, process value, file value at load). The loader routes every MCPPROXY_* override through it, Validate the env API key, and cmd/mcpproxy every serve flag (loadConfig + the new applyServeLoggingFlags / applyServeRuntimeFlags). - PersistableConfig(effective, path) restores the file's current value for every field whose effective value STILL equals its override. A field edited since (the Settings page changing listen, the tray picking an alternate port, an API toggle of read-only) no longer matches and is persisted as the edit it is — which keeps the restart-gated listen flow working, where a blanket "restore the file value" would have thrown the edit away. - SaveConfig applies it centrally, so runtime, telemetry, the server edition's admin handlers and the load-modify-save CLI subcommands are all covered. The runtime additionally computes the persistable form for its config-watcher self-write markers and the watcher's memory-vs-disk comparison, otherwise its own saves would read as external edits and reload the file over the hot overrides. Env-sourced entries are rebuilt on every load (a reload reflects the variables set now); flag entries survive reloads. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 191 ++++++----- cmd/mcpproxy/serve_flag_persistence_test.go | 115 +++++++ internal/config/config.go | 5 +- internal/config/loader.go | 54 +-- internal/config/process_overrides.go | 313 ++++++++++++++++++ internal/config/process_overrides_test.go | 190 +++++++++++ internal/runtime/config_watcher.go | 18 +- internal/runtime/config_watcher_test.go | 2 +- internal/runtime/lifecycle.go | 6 +- .../runtime/process_overrides_persist_test.go | 143 ++++++++ internal/runtime/runtime.go | 4 +- .../process_overrides_persist_test.go | 56 ++++ 12 files changed, 966 insertions(+), 131 deletions(-) create mode 100644 internal/config/process_overrides.go create mode 100644 internal/config/process_overrides_test.go create mode 100644 internal/runtime/process_overrides_persist_test.go create mode 100644 internal/telemetry/process_overrides_persist_test.go diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index f1b753317..fd43f49f2 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -432,16 +432,6 @@ 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) @@ -449,51 +439,7 @@ func runServer(cmd *cobra.Command, _ []string) error { 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 /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) @@ -540,38 +486,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), @@ -817,24 +736,26 @@ 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") - 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) @@ -854,7 +775,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) } } @@ -866,7 +787,93 @@ 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 /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) + + if cmdToolResponseLimit, _ := flags.GetInt("tool-response-limit"); cmdToolResponseLimit != 0 { + config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, cmdToolResponseLimit) + } + + // 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) + } } } diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index d5793fd2f..126d28708 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -356,3 +356,118 @@ func TestServeSaverUsesTheDiscoveredConfigPath(t *testing.T) { untouched := readConfigFileJSON(t, unrelated) assert.Nil(t, untouched["telemetry"], "save landed in the unrelated 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"]) +} diff --git a/internal/config/config.go b/internal/config/config.go index 0fc2569e4..e44643932 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2710,7 +2710,10 @@ func (c *Config) Validate() error { // Check environment variable for API key // Use LookupEnv to distinguish between "not set" and "set to empty string" if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists { - c.APIKey = envAPIKey // Allow empty string to explicitly disable authentication + // Allow empty string to explicitly disable authentication. A + // process-only override: no save path may write it into api_key + // (see process_overrides.go). + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envAPIKey) } } diff --git a/internal/config/loader.go b/internal/config/loader.go index 20fef023b..1a716089f 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -421,6 +421,10 @@ func atomicWriteFile(path string, data []byte, perm os.FileMode) error { // SaveConfig saves configuration to file func SaveConfig(cfg *Config, path string) error { + // Never persist a process-only override (serve flag, MCPPROXY_* env, env + // API key): write the file's value back for every field still carrying + // one. See process_overrides.go. + cfg = PersistableConfig(cfg, path) data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return fmt.Errorf("failed to marshal config: %w", err) @@ -656,8 +660,13 @@ func expandDataDir(cfg *Config) { cfg.DataDir = resolved } -// applyTLSEnvOverrides applies environment variable overrides for TLS configuration +// applyTLSEnvOverrides applies the MCPPROXY_* environment overrides. Each one +// goes through OverrideForProcess so no save path persists it (see +// process_overrides.go); the env-sourced set is rebuilt from scratch on every +// load so a reload reflects the variables set now. func applyTLSEnvOverrides(cfg *Config) { + clearProcessOverrides(OverrideSourceEnv) + // Ensure TLS config is initialized if cfg.TLS == nil { cfg.TLS = &TLSConfig{ @@ -670,27 +679,27 @@ func applyTLSEnvOverrides(cfg *Config) { // Override listen address from environment if value := os.Getenv("MCPPROXY_LISTEN"); value != "" { - cfg.Listen = value + OverrideForProcess(cfg, FieldListen, OverrideSourceEnv, value) } // Override TLS enabled from environment if value := os.Getenv("MCPPROXY_TLS_ENABLED"); value != "" { - cfg.TLS.Enabled = (value == trueValue || value == "1") + OverrideForProcess(cfg, FieldTLSEnabled, OverrideSourceEnv, value == trueValue || value == "1") } // Override TLS client cert requirement from environment if value := os.Getenv("MCPPROXY_TLS_REQUIRE_CLIENT_CERT"); value != "" { - cfg.TLS.RequireClientCert = (value == trueValue || value == "1") + OverrideForProcess(cfg, FieldTLSRequireClientCert, OverrideSourceEnv, value == trueValue || value == "1") } // Override TLS certificates directory from environment if value := os.Getenv("MCPPROXY_CERTS_DIR"); value != "" { - cfg.TLS.CertsDir = value + OverrideForProcess(cfg, FieldTLSCertsDir, OverrideSourceEnv, value) } // Override data directory from environment (for backward compatibility) if value := os.Getenv("MCPPROXY_DATA"); value != "" { - cfg.DataDir = value + OverrideForProcess(cfg, FieldDataDir, OverrideSourceEnv, value) } // Override trusted hosts for reverse-proxy deployments (GH #898). @@ -702,7 +711,7 @@ func applyTLSEnvOverrides(cfg *Config) { hosts = append(hosts, h) } } - cfg.TrustedHosts = hosts + OverrideForProcess(cfg, FieldTrustedHosts, OverrideSourceEnv, hosts) } // Override the offline TPA signature-bundle path from environment @@ -710,10 +719,7 @@ func applyTLSEnvOverrides(cfg *Config) { // the env value wins over the file value, and materializes the security // block so a config with no `security` key can still point at a corpus. if value := os.Getenv(EnvTPABundlePath); value != "" { - if cfg.Security == nil { - cfg.Security = &SecurityConfig{} - } - cfg.Security.TPABundlePath = value + OverrideForProcess(cfg, FieldTPABundlePath, OverrideSourceEnv, value) } // Override the automatic informational baseline-scan kill switch from @@ -729,23 +735,17 @@ func applyTLSEnvOverrides(cfg *Config) { switch os.Getenv(EnvAutoBaselineScan) { case trueValue, "1": enabled := true - if cfg.Security == nil { - cfg.Security = &SecurityConfig{} - } - cfg.Security.AutoBaselineScan = &enabled + OverrideForProcess(cfg, FieldAutoBaselineScan, OverrideSourceEnv, &enabled) case falseValue, "0": enabled := false - if cfg.Security == nil { - cfg.Security = &SecurityConfig{} - } - cfg.Security.AutoBaselineScan = &enabled + OverrideForProcess(cfg, FieldAutoBaselineScan, OverrideSourceEnv, &enabled) } // Override retrieve_tools serialization mode from environment (Spec 085). // Explicit MCPPROXY_* alias per the established loader convention; the // value is validated by cfg.Validate() right after these overrides apply. if value := os.Getenv("MCPPROXY_TOOL_RESPONSE_MODE"); value != "" { - cfg.ToolResponseMode = value + OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceEnv, value) } // Override DIRECT-surface serialization mode from environment (Spec 102). @@ -753,7 +753,7 @@ func applyTLSEnvOverrides(cfg *Config) { // separate config axis: that one governs retrieve_tools, this one governs // the direct enumeration surface. Setting one must never move the other. if value := os.Getenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE"); value != "" { - cfg.DirectToolResponseMode = value + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceEnv, value) } // Override the GLOBAL aggregate concurrency limiter from environment @@ -764,14 +764,14 @@ func applyTLSEnvOverrides(cfg *Config) { // so a typo cannot silently reshape the proxy's admission behavior. if value := os.Getenv("MCPPROXY_MAX_CONCURRENT_REQUESTS"); value != "" { if n, err := strconv.Atoi(value); err == nil && n >= 0 { - cfg.MaxConcurrentRequests = &n + OverrideForProcess(cfg, FieldMaxConcurrentRequests, OverrideSourceEnv, &n) } else { fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_MAX_CONCURRENT_REQUESTS=%q (want a non-negative integer)\n", value) } } if value := os.Getenv("MCPPROXY_QUEUE_SIZE"); value != "" { if n, err := strconv.Atoi(value); err == nil && n >= 0 { - cfg.QueueSize = &n + OverrideForProcess(cfg, FieldQueueSize, OverrideSourceEnv, &n) } else { fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_QUEUE_SIZE=%q (want a non-negative integer)\n", value) } @@ -779,7 +779,7 @@ func applyTLSEnvOverrides(cfg *Config) { if value := os.Getenv("MCPPROXY_QUEUE_TIMEOUT"); value != "" { if d, err := time.ParseDuration(value); err == nil && d >= 0 { qt := Duration(d) - cfg.QueueTimeout = &qt + OverrideForProcess(cfg, FieldQueueTimeout, OverrideSourceEnv, &qt) } else { fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_QUEUE_TIMEOUT=%q (want a duration such as \"30s\")\n", value) } @@ -795,7 +795,7 @@ func applyTLSEnvOverrides(cfg *Config) { if value := os.Getenv("MCPPROXY_HTTP_READ_TIMEOUT"); value != "" { if d, err := time.ParseDuration(value); err == nil && d >= 0 { rt := Duration(d) - cfg.HTTPReadTimeout = &rt + OverrideForProcess(cfg, FieldHTTPReadTimeout, OverrideSourceEnv, &rt) } else { fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_READ_TIMEOUT=%q (want a duration such as \"120s\", or \"0s\" to disable)\n", value) } @@ -803,7 +803,7 @@ func applyTLSEnvOverrides(cfg *Config) { if value := os.Getenv("MCPPROXY_HTTP_WRITE_TIMEOUT"); value != "" { if d, err := time.ParseDuration(value); err == nil && d >= 0 { wt := Duration(d) - cfg.HTTPWriteTimeout = &wt + OverrideForProcess(cfg, FieldHTTPWriteTimeout, OverrideSourceEnv, &wt) } else { fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_WRITE_TIMEOUT=%q (want a duration such as \"300s\", or \"0s\" to disable)\n", value) } @@ -811,7 +811,7 @@ func applyTLSEnvOverrides(cfg *Config) { if value := os.Getenv("MCPPROXY_HTTP_IDLE_TIMEOUT"); value != "" { if d, err := time.ParseDuration(value); err == nil && d >= 0 { it := Duration(d) - cfg.HTTPIdleTimeout = &it + OverrideForProcess(cfg, FieldHTTPIdleTimeout, OverrideSourceEnv, &it) } else { fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_IDLE_TIMEOUT=%q (want a duration such as \"180s\"; \"0s\" falls back to the read timeout)\n", value) } diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go new file mode 100644 index 000000000..55196f65a --- /dev/null +++ b/internal/config/process_overrides.go @@ -0,0 +1,313 @@ +package config + +import ( + "reflect" + "sort" + "sync" +) + +// Process-only overrides: the persisted-vs-effective split. +// +// The config a process runs with is the file PLUS a layer of one-off choices — +// `serve` CLI flags (--listen, --read-only, --tool-response-mode, ...), the +// MCPPROXY_* environment variables the loader applies, and the MCPPROXY_API_KEY +// that Validate copies into api_key. That effective config is the only one the +// daemon holds: the runtime, the telemetry service and every API handler read +// and — crucially — SAVE it. Before this existed, any persist path (first-run +// anonymous_id generation, a server enable from the Web UI, the startup-outcome +// stamp) wrote the overrides into mcp_config.json, and the next unflagged start +// — or the tray-launched core — inherited a choice that was meant for one run +// (`--listen :0` used to leave `"listen": ":0"` behind and the core silently +// booted in stdio mode). +// +// The registry below records, per overridden field, the value this process was +// given and the value the file held when it was applied. PersistableConfig then +// answers "what should go to disk": for every recorded field whose EFFECTIVE +// value still equals the override, the file's value is written back; a field +// that has since been edited (the Settings page changing listen, the tray +// picking an alternate port) no longer matches the override and is persisted +// as the edit it is. That is what keeps the API's legitimate edits of the very +// same fields working — a blanket "always restore the file value" would have +// thrown those edits away. +// +// SaveConfig applies the split centrally so every persist path — the runtime, +// telemetry, the server edition's admin handlers, the CLI subcommands that +// load-modify-save — is covered without each having to remember. Callers that +// need to know the exact bytes going to disk (the runtime's config-watcher +// self-write markers) call PersistableConfig themselves first; the mapping is +// idempotent, so SaveConfig re-applying it is harmless. + +// OverrideSource says where a process-only override came from. +type OverrideSource string + +const ( + // OverrideSourceFlag is a `serve` command-line flag. + OverrideSourceFlag OverrideSource = "flag" + // OverrideSourceEnv is a MCPPROXY_* environment variable, including the + // MCPPROXY_API_KEY that Validate folds into api_key. + OverrideSourceEnv OverrideSource = "env" +) + +// Field names a configuration value a process-only override can shadow. Set +// copies nested structs before writing (copy-on-write) so a PersistableConfig +// result never writes through a pointer it shares with the effective config. +type Field[T any] struct { + Name string + Get func(*Config) T + Set func(*Config, T) +} + +// The overridable fields. Keep in lockstep with the overrides applied in +// cmd/mcpproxy (serve flags), applyTLSEnvOverrides (env) and Validate (env API +// key): an override applied without going through OverrideForProcess is +// persisted by every save path, which is the bug this file exists to close. +var ( + FieldListen = scalar("listen", func(c *Config) *string { return &c.Listen }) + FieldDataDir = scalar("data_dir", func(c *Config) *string { return &c.DataDir }) + FieldAPIKey = scalar("api_key", func(c *Config) *string { return &c.APIKey }) + FieldTrayEndpoint = scalar("tray_endpoint", func(c *Config) *string { return &c.TrayEndpoint }) + FieldEnableSocket = scalar("enable_socket", func(c *Config) *bool { return &c.EnableSocket }) + FieldToolResponseLimit = scalar("tool_response_limit", func(c *Config) *int { return &c.ToolResponseLimit }) + FieldToolResponseMode = scalar("tool_response_mode", func(c *Config) *string { return &c.ToolResponseMode }) + FieldDirectToolResponseMode = scalar("direct_tool_response_mode", func(c *Config) *string { return &c.DirectToolResponseMode }) + FieldDebugSearch = scalar("debug_search", func(c *Config) *bool { return &c.DebugSearch }) + FieldRequireMCPAuth = scalar("require_mcp_auth", func(c *Config) *bool { return &c.RequireMCPAuth }) + FieldReadOnlyMode = scalar("read_only_mode", func(c *Config) *bool { return &c.ReadOnlyMode }) + FieldDisableManagement = scalar("disable_management", func(c *Config) *bool { return &c.DisableManagement }) + FieldAllowServerAdd = scalar("allow_server_add", func(c *Config) *bool { return &c.AllowServerAdd }) + FieldAllowServerRemove = scalar("allow_server_remove", func(c *Config) *bool { return &c.AllowServerRemove }) + FieldEnablePrompts = scalar("enable_prompts", func(c *Config) *bool { return &c.EnablePrompts }) + FieldAggregateUpstreamPrompts = scalar("aggregate_upstream_prompts", func(c *Config) *bool { return &c.AggregateUpstreamPrompts }) + FieldTrustedHosts = scalar("trusted_hosts", func(c *Config) *[]string { return &c.TrustedHosts }) + FieldMaxConcurrentRequests = scalar("max_concurrent_requests", func(c *Config) **int { return &c.MaxConcurrentRequests }) + FieldQueueSize = scalar("queue_size", func(c *Config) **int { return &c.QueueSize }) + FieldQueueTimeout = scalar("queue_timeout", func(c *Config) **Duration { return &c.QueueTimeout }) + FieldHTTPReadTimeout = scalar("http_read_timeout", func(c *Config) **Duration { return &c.HTTPReadTimeout }) + FieldHTTPWriteTimeout = scalar("http_write_timeout", func(c *Config) **Duration { return &c.HTTPWriteTimeout }) + FieldHTTPIdleTimeout = scalar("http_idle_timeout", func(c *Config) **Duration { return &c.HTTPIdleTimeout }) + + FieldLogLevel = Field[string]{ + Name: "logging.level", + Get: func(c *Config) string { return loggingOf(c).Level }, + Set: func(c *Config, v string) { l := cowLogging(c); l.Level = v }, + } + FieldLogEnableFile = Field[bool]{ + Name: "logging.enable_file", + Get: func(c *Config) bool { return loggingOf(c).EnableFile }, + Set: func(c *Config, v bool) { l := cowLogging(c); l.EnableFile = v }, + } + FieldLogDir = Field[string]{ + Name: "logging.log_dir", + Get: func(c *Config) string { return loggingOf(c).LogDir }, + Set: func(c *Config, v string) { l := cowLogging(c); l.LogDir = v }, + } + FieldTLSEnabled = Field[bool]{ + Name: "tls.enabled", + Get: func(c *Config) bool { return tlsOf(c).Enabled }, + Set: func(c *Config, v bool) { t := cowTLS(c); t.Enabled = v }, + } + FieldTLSRequireClientCert = Field[bool]{ + Name: "tls.require_client_cert", + Get: func(c *Config) bool { return tlsOf(c).RequireClientCert }, + Set: func(c *Config, v bool) { t := cowTLS(c); t.RequireClientCert = v }, + } + FieldTLSCertsDir = Field[string]{ + Name: "tls.certs_dir", + Get: func(c *Config) string { return tlsOf(c).CertsDir }, + Set: func(c *Config, v string) { t := cowTLS(c); t.CertsDir = v }, + } + FieldTPABundlePath = Field[string]{ + Name: "security.tpa_bundle_path", + Get: func(c *Config) string { return securityOf(c).TPABundlePath }, + Set: func(c *Config, v string) { s := cowSecurity(c); s.TPABundlePath = v }, + } + FieldAutoBaselineScan = Field[*bool]{ + Name: "security.auto_baseline_scan", + Get: func(c *Config) *bool { return securityOf(c).AutoBaselineScan }, + Set: func(c *Config, v *bool) { + if c.Security == nil && v == nil { + return // nothing to unset; do not materialize an empty block + } + s := cowSecurity(c) + s.AutoBaselineScan = v + }, + } +) + +// scalar builds a Field for a top-level value addressed by pointer. +func scalar[T any](name string, ptr func(*Config) *T) Field[T] { + return Field[T]{ + Name: name, + Get: func(c *Config) T { return *ptr(c) }, + Set: func(c *Config, v T) { *ptr(c) = v }, + } +} + +// loggingOf/tlsOf/securityOf read a nested block, treating a missing one as +// its zero value; cowLogging/cowTLS/cowSecurity replace the block with a copy +// before a write so the pointer shared with the effective config is untouched. +func loggingOf(c *Config) LogConfig { + if c.Logging == nil { + return LogConfig{} + } + return *c.Logging +} + +func cowLogging(c *Config) *LogConfig { + l := loggingOf(c) + c.Logging = &l + return c.Logging +} + +func tlsOf(c *Config) TLSConfig { + if c.TLS == nil { + return TLSConfig{} + } + return *c.TLS +} + +func cowTLS(c *Config) *TLSConfig { + t := tlsOf(c) + c.TLS = &t + return c.TLS +} + +func securityOf(c *Config) SecurityConfig { + if c.Security == nil { + return SecurityConfig{} + } + return *c.Security +} + +func cowSecurity(c *Config) *SecurityConfig { + s := securityOf(c) + c.Security = &s + return c.Security +} + +// processOverride is one recorded override. +type processOverride interface { + name() string + source() OverrideSource + // restore writes the persisted value of the field into out when out still + // carries the process value. base is the file as it stands now (nil when + // unreadable, in which case the value the file held at override time is + // used). + restore(out, base *Config) +} + +type typedOverride[T any] struct { + field Field[T] + src OverrideSource + process T // the value this process runs with + loaded T // the file's value when the override was applied +} + +func (o typedOverride[T]) name() string { return o.field.Name } +func (o typedOverride[T]) source() OverrideSource { return o.src } + +func (o typedOverride[T]) restore(out, base *Config) { + if !reflect.DeepEqual(o.field.Get(out), o.process) { + return // edited since the override was applied: a real change, persist it + } + fileValue := o.loaded + if base != nil { + fileValue = o.field.Get(base) + } + o.field.Set(out, fileValue) +} + +var ( + processOverridesMu sync.RWMutex + // processOverrides is keyed by field name; a later override of the same + // field (a reload re-applying env, a flag applied after env) replaces the + // earlier one — one process value per field. + processOverrides = map[string]processOverride{} +) + +// OverrideForProcess sets field f on cfg to value for THIS PROCESS ONLY and +// records it, so PersistableConfig (and therefore SaveConfig) writes the file's +// value back as long as the effective value still equals the override. +func OverrideForProcess[T any](cfg *Config, f Field[T], source OverrideSource, value T) { + if cfg == nil { + return + } + loaded := f.Get(cfg) + f.Set(cfg, value) + + processOverridesMu.Lock() + processOverrides[f.Name] = typedOverride[T]{field: f, src: source, process: value, loaded: loaded} + processOverridesMu.Unlock() +} + +// clearProcessOverrides drops every override recorded from source. The loader +// calls it for OverrideSourceEnv before re-applying the environment on each +// load, so a reload reflects the variables set NOW; flag overrides, applied +// once at startup, survive reloads. +func clearProcessOverrides(source OverrideSource) { + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for name, o := range processOverrides { + if o.source() == source { + delete(processOverrides, name) + } + } +} + +// ResetProcessOverrides forgets every recorded override. For tests. +func ResetProcessOverrides() { + processOverridesMu.Lock() + processOverrides = map[string]processOverride{} + processOverridesMu.Unlock() +} + +// ProcessOverrideFields lists the names of the fields currently overridden for +// this process, sorted, for diagnostics and logging. +func ProcessOverrideFields() []string { + processOverridesMu.RLock() + defer processOverridesMu.RUnlock() + names := make([]string, 0, len(processOverrides)) + for name := range processOverrides { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// PersistableConfig returns the config that should go to disk at path for the +// given effective config: a shallow copy in which every field that still +// carries its process-only override is replaced by the value the file at path +// holds now (or held when the override was applied, if the file cannot be read). +// Fields that were edited since keep the edit. With no overrides recorded the +// effective config is returned as is. +// +// The copy shares Servers, Registries and every nested block it does not touch +// with effective; the ones it restores are copied first. Callers must not +// mutate the shared structures. +func PersistableConfig(effective *Config, path string) *Config { + if effective == nil { + return nil + } + processOverridesMu.RLock() + overrides := make([]processOverride, 0, len(processOverrides)) + for _, o := range processOverrides { + overrides = append(overrides, o) + } + processOverridesMu.RUnlock() + if len(overrides) == 0 { + return effective + } + + var base *Config + if path != "" { + if onDisk, err := ReadFile(path); err == nil { + base = onDisk + } + } + + out := *effective + for _, o := range overrides { + o.restore(&out, base) + } + return &out +} diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go new file mode 100644 index 000000000..382127b15 --- /dev/null +++ b/internal/config/process_overrides_test.go @@ -0,0 +1,190 @@ +package config + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A process-only override (CLI flag, MCPPROXY_* env, env API key) shadows the +// file value for this one process. The effective config carries the override; +// the persisted config must not — unless something edited the field afterwards +// (an API PUT of a new listen address), which is a real change to persist. + +func writeOverrideTestFile(t *testing.T, raw string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "mcp_config.json") + require.NoError(t, os.WriteFile(path, []byte(raw), 0o600)) + return path +} + +func readJSON(t *testing.T, path string) map[string]any { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + return m +} + +func TestOverrideForProcess_SetsEffectiveValueAndRecordsIt(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Listen = "127.0.0.1:8080" + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + assert.Equal(t, ":0", cfg.Listen, "the override is the effective value") + assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) +} + +func TestPersistableConfig_RestoresFileValueWhileOverrideStillApplies(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + persisted := PersistableConfig(cfg, path) + assert.Equal(t, "127.0.0.1:8080", persisted.Listen) + assert.Equal(t, ":0", cfg.Listen, "the effective config is left untouched") +} + +func TestPersistableConfig_KeepsAnEditOfTheOverriddenField(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + // An API edit: the effective value no longer matches the override. + edited := *cfg + edited.Listen = "127.0.0.1:9090" + persisted := PersistableConfig(&edited, path) + assert.Equal(t, "127.0.0.1:9090", persisted.Listen, "an edit of an overridden field is persisted") +} + +func TestPersistableConfig_PrefersTheCurrentFileOverTheLoadTimeValue(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + // Something else (an API edit, an external editor) moved the file on. + require.NoError(t, os.WriteFile(path, []byte(`{"listen": "127.0.0.1:7070", "mcpServers": []}`), 0o600)) + + persisted := PersistableConfig(cfg, path) + assert.Equal(t, "127.0.0.1:7070", persisted.Listen, "a round-tripped override must not resurrect the load-time file value") +} + +func TestPersistableConfig_FallsBackToLoadTimeValueWhenFileUnreadable(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Listen = "127.0.0.1:8080" + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, "127.0.0.1:8080", persisted.Listen) +} + +func TestPersistableConfig_NestedFieldsAreCopiedNotMutated(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Logging = &LogConfig{Level: "info"} + cfg.TLS.Enabled = false + OverrideForProcess(cfg, FieldLogLevel, OverrideSourceFlag, "debug") + OverrideForProcess(cfg, FieldTLSEnabled, OverrideSourceEnv, true) + require.Equal(t, "debug", cfg.Logging.Level) + require.True(t, cfg.TLS.Enabled) + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, "info", persisted.Logging.Level) + assert.False(t, persisted.TLS.Enabled) + assert.Equal(t, "debug", cfg.Logging.Level, "restoring must not write through the shared Logging pointer") + assert.True(t, cfg.TLS.Enabled, "restoring must not write through the shared TLS pointer") + assert.NotSame(t, cfg.Logging, persisted.Logging) + assert.NotSame(t, cfg.TLS, persisted.TLS) +} + +func TestPersistableConfig_NoOverridesReturnsInput(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + cfg := DefaultConfig() + assert.Same(t, cfg, PersistableConfig(cfg, "/nonexistent")) +} + +func TestSaveConfig_DoesNotPersistProcessOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "read_only_mode": false, "mcpServers": []}`) + + cfg, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + OverrideForProcess(cfg, FieldAPIKey, OverrideSourceEnv, "env-secret") + cfg.ToolsLimit = 42 // an ordinary in-memory edit, persisted as usual + + require.NoError(t, SaveConfig(cfg, path)) + + m := readJSON(t, path) + assert.Equal(t, "127.0.0.1:8080", m["listen"]) + assert.NotEqual(t, true, m["read_only_mode"]) + assert.NotEqual(t, "env-secret", m["api_key"]) + assert.Equal(t, float64(42), m["tools_limit"]) +} + +func TestLoadFromFile_RecordsEnvOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "0.0.0.0:9999") + t.Setenv("MCPPROXY_TOOL_RESPONSE_MODE", "compact") + t.Setenv("MCPPROXY_API_KEY", "from-env") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "0.0.0.0:9999", cfg.Listen) + require.Equal(t, "compact", cfg.ToolResponseMode) + require.Equal(t, "from-env", cfg.APIKey) + + require.NoError(t, SaveConfig(cfg, path)) + m := readJSON(t, path) + assert.Equal(t, "127.0.0.1:8080", m["listen"]) + assert.Equal(t, "full", m["tool_response_mode"]) + assert.NotEqual(t, "from-env", m["api_key"]) +} + +func TestLoadFromFile_ReplacesEnvOverridesButKeepsFlagOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + t.Setenv("MCPPROXY_TOOL_RESPONSE_MODE", "compact") + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + assert.ElementsMatch(t, []string{"listen", "tool_response_mode"}, ProcessOverrideFields()) + + // A reload with the variable gone drops the env entry and keeps the flag. + os.Unsetenv("MCPPROXY_TOOL_RESPONSE_MODE") + _, err = LoadFromFile(path) + require.NoError(t, err) + assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) +} diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index f588d39e5..9e090e460 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -131,8 +131,12 @@ type selfWriteEntry struct { // does; a marshal failure just skips recording (the write itself would have // failed the same way). Recording an already-present payload refreshes its // timestamp; when full, the oldest entry is evicted. -func (r *Runtime) noteConfigSelfWrite(cfg *config.Config) { - data, err := json.MarshalIndent(cfg, "", " ") +// +// path is the file the save targets: the payload is the PERSISTABLE form of +// cfg (config.PersistableConfig), which is what SaveConfig actually writes when +// a serve flag or MCPPROXY_* env override is in force. +func (r *Runtime) noteConfigSelfWrite(cfg *config.Config, path string) { + data, err := json.MarshalIndent(config.PersistableConfig(cfg, path), "", " ") if err != nil { return } @@ -158,8 +162,8 @@ func (r *Runtime) noteConfigSelfWrite(cfg *config.Config) { // those bytes never reached disk, so a later byte-identical write of them is // a genuine external edit the watcher must reload. Only the failed payload is // removed — markers pre-armed by other (successful) saves stay live. -func (r *Runtime) forgetConfigSelfWrite(cfg *config.Config) { - data, err := json.MarshalIndent(cfg, "", " ") +func (r *Runtime) forgetConfigSelfWrite(cfg *config.Config, path string) { + data, err := json.MarshalIndent(config.PersistableConfig(cfg, path), "", " ") if err != nil { return } @@ -235,8 +239,12 @@ func (r *Runtime) reloadFromDiskIfChanged(absPath string) { // event came from our own save. If a future save path diverges, this // degrades to one redundant (idempotent) reload — never a loop, since // ReloadConfiguration never writes the file. + // Compared in its PERSISTABLE form: with a serve flag or MCPPROXY_* env + // override in force the file legitimately differs from memory in exactly + // those fields (config.PersistableConfig), and reading that difference as + // an external edit would reload the file over the override on every save. trimmedDisk := bytes.TrimSpace(diskBytes) - if current, merr := json.MarshalIndent(r.ConfigSnapshot().Config, "", " "); merr == nil { + if current, merr := json.MarshalIndent(config.PersistableConfig(r.ConfigSnapshot().Config, absPath), "", " "); merr == nil { if bytes.Equal(bytes.TrimSpace(current), trimmedDisk) { // The file now matches memory. If that content matches none of // the recorded self-writes, the file has moved past our saves diff --git a/internal/runtime/config_watcher_test.go b/internal/runtime/config_watcher_test.go index 7bfbe8730..4a8909d5d 100644 --- a/internal/runtime/config_watcher_test.go +++ b/internal/runtime/config_watcher_test.go @@ -464,7 +464,7 @@ func TestConfigWatcher_BackToBackSelfWritesBothSuppressed(t *testing.T) { // this window). With a single slot this evicts A's record. cfgB := editedConfig(initialCfg, 72222) cfgB.Listen = "127.0.0.1:2" - rt.noteConfigSelfWrite(cfgB) + rt.noteConfigSelfWrite(cfgB, cfgPath) // A's debounce fires while disk still holds A. It must stay suppressed. time.Sleep(1200 * time.Millisecond) diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 8581397d4..f6314c668 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1476,9 +1476,9 @@ func (r *Runtime) SaveConfiguration() error { // the configSvc snapshot (the running config). Marked as our own // write first, or the watcher reads the pending value back as an // external edit and hot-applies what we deliberately deferred. - r.noteConfigSelfWrite(diskCopy) + r.noteConfigSelfWrite(diskCopy, snapshot.Path) if err := config.SaveConfig(diskCopy, snapshot.Path); err != nil { - r.forgetConfigSelfWrite(diskCopy) + r.forgetConfigSelfWrite(diskCopy, snapshot.Path) r.logger.Error("Failed to save config to file (pending-aware path)", zap.Error(err)) return err } @@ -1492,7 +1492,7 @@ func (r *Runtime) SaveConfiguration() error { // Fallback to legacy save (no configSvc store to keep in sync) if diskCopy != nil { configCopy = diskCopy - r.noteConfigSelfWrite(diskCopy) + r.noteConfigSelfWrite(diskCopy, snapshot.Path) } if err := config.SaveConfig(configCopy, snapshot.Path); err != nil { r.logger.Error("Failed to save config to file (legacy path)", zap.Error(err)) diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go new file mode 100644 index 000000000..17a8563f5 --- /dev/null +++ b/internal/runtime/process_overrides_persist_test.go @@ -0,0 +1,143 @@ +package runtime + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// The runtime's persist paths (SaveConfiguration on every API-driven server +// change, ApplyConfig on PUT /api/v1/config) write the live config, which +// carries the `serve` CLI flag and MCPPROXY_* env overrides. Those are +// process-only and must not reach the file — while a genuine API edit of the +// same field (the Settings page changing listen) still must. +func newOverriddenRuntime(t *testing.T) (*Runtime, string) { + t.Helper() + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "mcp_config.json") + initial := config.DefaultConfig() + initial.Listen = "127.0.0.1:8080" + initial.DataDir = tmp + initial.ToolResponseMode = "full" + require.NoError(t, config.SaveConfig(initial, cfgPath)) + + cfg, err := config.ReadFile(cfgPath) + require.NoError(t, err) + config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, ":0") + config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, "compact") + config.OverrideForProcess(cfg, config.FieldReadOnlyMode, config.OverrideSourceFlag, true) + config.OverrideForProcess(cfg, config.FieldAPIKey, config.OverrideSourceEnv, "env-secret") + + rt, err := New(cfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + return rt, cfgPath +} + +func readConfigJSON(t *testing.T, path string) map[string]any { + t.Helper() + data, err := os.ReadFile(path) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + return m +} + +func assertNoOverridesOnDisk(t *testing.T, m map[string]any) { + t.Helper() + assert.Equal(t, "127.0.0.1:8080", m["listen"], "--listen must not be persisted") + assert.Equal(t, "full", m["tool_response_mode"], "--tool-response-mode must not be persisted") + assert.NotEqual(t, true, m["read_only_mode"], "--read-only must not be persisted") + assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not be persisted") +} + +func TestSaveConfiguration_DoesNotPersistProcessOverrides(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + require.NoError(t, rt.SaveConfiguration()) + + assertNoOverridesOnDisk(t, readConfigJSON(t, cfgPath)) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, ":0", live.Listen, "the effective config keeps the override") + assert.Equal(t, "compact", live.ToolResponseMode) + assert.True(t, live.ReadOnlyMode) + assert.Equal(t, "env-secret", live.APIKey) +} + +// GET /config returns the desired config — which at startup IS the effective +// one, overrides included — and a PUT round-trips it. A field that comes back +// unchanged from that round trip is not an edit. +func TestApplyConfig_RoundTrippedOverrideIsNotPersisted(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, ":0", desired.Listen, "GET /config serves the effective value") + desired.ToolsLimit = 42 // the actual edit + + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + m := readConfigJSON(t, cfgPath) + assertNoOverridesOnDisk(t, m) + assert.Equal(t, float64(42), m["tools_limit"], "the real edit is persisted") +} + +// The Settings page changing listen while `--listen` is in force is a real +// edit: it must reach the file (restart-gated, so it stays pending in memory). +func TestApplyConfig_EditOfAnOverriddenFieldIsPersisted(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.Listen = "127.0.0.1:9090" + desired.ToolResponseMode = "full" // hot: turning the flag's choice back off + + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + m := readConfigJSON(t, cfgPath) + assert.Equal(t, "127.0.0.1:9090", m["listen"]) + assert.Equal(t, "full", m["tool_response_mode"]) + assert.NotEqual(t, true, m["read_only_mode"], "the untouched override is still not persisted") + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, ":0", live.Listen, "listen is restart-gated: memory keeps the bound value") + assert.Equal(t, "full", live.ToolResponseMode, "the hot edit is live") + + // A later unrelated save must not revert the persisted edit. + require.NoError(t, rt.SaveConfiguration()) + m = readConfigJSON(t, cfgPath) + assert.Equal(t, "127.0.0.1:9090", m["listen"]) +} + +// The config watcher compares the file with memory to recognise the daemon's +// own saves. With overrides in force the file legitimately differs from memory +// in exactly those fields; that difference must not read as an external edit, +// or every API save would trigger a reload that drops the hot overrides. +func TestConfigWatcher_OwnSaveWithOverridesIsNotAnExternalEdit(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + require.NoError(t, rt.SaveConfiguration()) + rt.clearSelfWrites() // the marker path is tested elsewhere; force the snapshot comparison + + rt.reloadFromDiskIfChanged(cfgPath) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, "compact", live.ToolResponseMode, "the daemon's own save must not reload the file over the flag") + assert.True(t, live.ReadOnlyMode) +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 3fe9d7e87..4320f5cff 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1737,7 +1737,7 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // (config_watcher.go). If the save fails, its entry is removed again on // the error path below — nothing reached disk, so a later byte-identical // EXTERNAL write of this config is a genuine edit the watcher must reload. - r.noteConfigSelfWrite(newCfg) + r.noteConfigSelfWrite(newCfg, savePath) saveErr := config.SaveConfig(newCfg, savePath) if saveErr != nil { @@ -1746,7 +1746,7 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // would suppress a genuine external write of byte-identical JSON. // Only this payload is forgotten — markers from other still-pending // successful saves stay live. - r.forgetConfigSelfWrite(newCfg) + r.forgetConfigSelfWrite(newCfg, savePath) r.logger.Error("Failed to save configuration to disk", zap.String("path", savePath), zap.Error(saveErr)) diff --git a/internal/telemetry/process_overrides_persist_test.go b/internal/telemetry/process_overrides_persist_test.go new file mode 100644 index 000000000..261dd006a --- /dev/null +++ b/internal/telemetry/process_overrides_persist_test.go @@ -0,0 +1,56 @@ +package telemetry + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// The first-run anonymous_id generation persists the WHOLE config the daemon +// handed to telemetry.New — the effective one, carrying every `serve` flag and +// MCPPROXY_* env override. Those apply to this process only and must never +// reach the file, or the next unflagged start inherits them. +func TestEnsureAnonymousID_DoesNotPersistProcessOverrides(t *testing.T) { + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "mcp_config.json") + raw := `{"listen": "127.0.0.1:8080", "data_dir": ` + jsonQuote(tmp) + `, "read_only_mode": false, "mcpServers": []}` + require.NoError(t, os.WriteFile(cfgPath, []byte(raw), 0o600)) + + cfg, err := config.ReadFile(cfgPath) + require.NoError(t, err) + config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, ":0") + config.OverrideForProcess(cfg, config.FieldReadOnlyMode, config.OverrideSourceFlag, true) + config.OverrideForProcess(cfg, config.FieldAPIKey, config.OverrideSourceEnv, "env-secret") + + svc := New(cfg, cfgPath, "v1.0.0", "personal", zap.NewNop()) + svc.ensureAnonymousID() + require.NotEmpty(t, cfg.GetAnonymousID()) + + data, err := os.ReadFile(cfgPath) + require.NoError(t, err) + var m map[string]any + require.NoError(t, json.Unmarshal(data, &m)) + + tel, _ := m["telemetry"].(map[string]any) + require.NotNil(t, tel, "the generated id must be persisted") + assert.Equal(t, cfg.GetAnonymousID(), tel["anonymous_id"]) + + assert.Equal(t, "127.0.0.1:8080", m["listen"], "--listen must not be persisted") + assert.NotEqual(t, true, m["read_only_mode"], "--read-only must not be persisted") + assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not be persisted") +} + +func jsonQuote(s string) string { + b, _ := json.Marshal(s) + return string(b) +} From 4f93eafef0dc35e627e9da80f79dcdbc60ca860f Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 10:25:39 +0300 Subject: [PATCH 08/18] fix(config): close the review-round-1 gaps in the process-override registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - EnsureAPIKey let MCPPROXY_API_KEY replace a key the FILE holds without recording it, so the next runtime/telemetry save wrote the env secret over the file key. Recorded as an env override (only when it actually differs, so Validate's record keeps the real file value). - The registry now keys entries by (field, source): a field overridden by env AND a flag keeps both records, and a reload — which rebuilds the env set — no longer drops the flag record and turns its value into "an edit". A flag layered over an env override inherits the env record's file value as its fallback. - The loader rebuilds the env entries as ONE registry update (envOverrideBatch.commit) instead of clear-then-add under separate locks, so a save on another goroutine can never observe an empty registry mid-reload and persist the overrides. - ReapplyFlagOverrides re-layers the serve flags onto a freshly reloaded config (configsvc.ReloadFromFile and the legacy fallback); the loader only re-applies env, so a hand edit of an unrelated key used to switch --read-only / --tool-response-mode off. - Documented the inherent limitation: an explicit edit that sets an overridden field to exactly the override's value is indistinguishable from a round trip (unreachable from the Web UI, which already shows that value). Co-Authored-By: Claude Opus 5 --- internal/config/config.go | 8 +- internal/config/loader.go | 146 ++++++++--------- internal/config/process_overrides.go | 135 +++++++++++++--- internal/config/process_overrides_test.go | 148 ++++++++++++++++++ internal/runtime/configsvc/service.go | 4 + internal/runtime/lifecycle.go | 1 + .../runtime/process_overrides_persist_test.go | 27 ++++ 7 files changed, 373 insertions(+), 96 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index e44643932..3f624b0b4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -2209,7 +2209,13 @@ func (c *Config) EnsureAPIKey() (apiKey string, wasGenerated bool, source APIKey // Check environment variable for API key first - this overrides config file // Use LookupEnv to distinguish between "not set" and "set to empty string" if envAPIKey, exists := os.LookupEnv("MCPPROXY_API_KEY"); exists && envAPIKey != "" { - c.APIKey = envAPIKey + // A process-only override of whatever the file holds: no save path + // may replace the file's key with it (see process_overrides.go). + // Already equal means Validate recorded it (or the file literally + // holds the env key); re-recording would lose the file value. + if c.APIKey != envAPIKey { + OverrideForProcess(c, FieldAPIKey, OverrideSourceEnv, envAPIKey) + } return c.APIKey, false, APIKeySourceEnvironment } diff --git a/internal/config/loader.go b/internal/config/loader.go index 1a716089f..475ab281d 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -665,7 +665,8 @@ func expandDataDir(cfg *Config) { // process_overrides.go); the env-sourced set is rebuilt from scratch on every // load so a reload reflects the variables set now. func applyTLSEnvOverrides(cfg *Config) { - clearProcessOverrides(OverrideSourceEnv) + b := &envOverrideBatch{} + defer b.commit() // Ensure TLS config is initialized if cfg.TLS == nil { @@ -678,49 +679,42 @@ func applyTLSEnvOverrides(cfg *Config) { } // Override listen address from environment - if value := os.Getenv("MCPPROXY_LISTEN"); value != "" { - OverrideForProcess(cfg, FieldListen, OverrideSourceEnv, value) - } + value := os.Getenv("MCPPROXY_LISTEN") + envOverride(b, cfg, FieldListen, value != "", value) // Override TLS enabled from environment - if value := os.Getenv("MCPPROXY_TLS_ENABLED"); value != "" { - OverrideForProcess(cfg, FieldTLSEnabled, OverrideSourceEnv, value == trueValue || value == "1") - } + value = os.Getenv("MCPPROXY_TLS_ENABLED") + envOverride(b, cfg, FieldTLSEnabled, value != "", value == trueValue || value == "1") // Override TLS client cert requirement from environment - if value := os.Getenv("MCPPROXY_TLS_REQUIRE_CLIENT_CERT"); value != "" { - OverrideForProcess(cfg, FieldTLSRequireClientCert, OverrideSourceEnv, value == trueValue || value == "1") - } + value = os.Getenv("MCPPROXY_TLS_REQUIRE_CLIENT_CERT") + envOverride(b, cfg, FieldTLSRequireClientCert, value != "", value == trueValue || value == "1") // Override TLS certificates directory from environment - if value := os.Getenv("MCPPROXY_CERTS_DIR"); value != "" { - OverrideForProcess(cfg, FieldTLSCertsDir, OverrideSourceEnv, value) - } + value = os.Getenv("MCPPROXY_CERTS_DIR") + envOverride(b, cfg, FieldTLSCertsDir, value != "", value) // Override data directory from environment (for backward compatibility) - if value := os.Getenv("MCPPROXY_DATA"); value != "" { - OverrideForProcess(cfg, FieldDataDir, OverrideSourceEnv, value) - } + value = os.Getenv("MCPPROXY_DATA") + envOverride(b, cfg, FieldDataDir, value != "", value) // Override trusted hosts for reverse-proxy deployments (GH #898). // Comma-separated list of Host header values accepted on loopback listeners. - if value := os.Getenv("MCPPROXY_TRUSTED_HOSTS"); value != "" { - var hosts []string - for _, h := range strings.Split(value, ",") { - if h = strings.TrimSpace(h); h != "" { - hosts = append(hosts, h) - } + var hosts []string + value = os.Getenv("MCPPROXY_TRUSTED_HOSTS") + for _, h := range strings.Split(value, ",") { + if h = strings.TrimSpace(h); h != "" { + hosts = append(hosts, h) } - OverrideForProcess(cfg, FieldTrustedHosts, OverrideSourceEnv, hosts) } + envOverride(b, cfg, FieldTrustedHosts, value != "", hosts) // Override the offline TPA signature-bundle path from environment // (spec 086 FR-019). Explicit MCPPROXY_* alias per the loader convention; // the env value wins over the file value, and materializes the security // block so a config with no `security` key can still point at a corpus. - if value := os.Getenv(EnvTPABundlePath); value != "" { - OverrideForProcess(cfg, FieldTPABundlePath, OverrideSourceEnv, value) - } + value = os.Getenv(EnvTPABundlePath) + envOverride(b, cfg, FieldTPABundlePath, value != "", value) // Override the automatic informational baseline-scan kill switch from // environment. Materializes the security block so an install with no @@ -732,29 +726,29 @@ func applyTLSEnvOverrides(cfg *Config) { // bare `value != ""` check would have materialized `false` here and silently // turned automatic scanning off for a config that had explicitly enabled it, // because the accessor then reads the overwritten field rather than the env. + var autoScan *bool switch os.Getenv(EnvAutoBaselineScan) { case trueValue, "1": enabled := true - OverrideForProcess(cfg, FieldAutoBaselineScan, OverrideSourceEnv, &enabled) + autoScan = &enabled case falseValue, "0": enabled := false - OverrideForProcess(cfg, FieldAutoBaselineScan, OverrideSourceEnv, &enabled) + autoScan = &enabled } + envOverride(b, cfg, FieldAutoBaselineScan, autoScan != nil, autoScan) // Override retrieve_tools serialization mode from environment (Spec 085). // Explicit MCPPROXY_* alias per the established loader convention; the // value is validated by cfg.Validate() right after these overrides apply. - if value := os.Getenv("MCPPROXY_TOOL_RESPONSE_MODE"); value != "" { - OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceEnv, value) - } + value = os.Getenv("MCPPROXY_TOOL_RESPONSE_MODE") + envOverride(b, cfg, FieldToolResponseMode, value != "", value) // Override DIRECT-surface serialization mode from environment (Spec 102). // A separate variable from MCPPROXY_TOOL_RESPONSE_MODE above, matching the // separate config axis: that one governs retrieve_tools, this one governs // the direct enumeration surface. Setting one must never move the other. - if value := os.Getenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE"); value != "" { - OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceEnv, value) - } + value = os.Getenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE") + envOverride(b, cfg, FieldDirectToolResponseMode, value != "", value) // Override the GLOBAL aggregate concurrency limiter from environment // (spec 093 FR-022, GH #955). Only this scope has an env scheme: the @@ -762,28 +756,12 @@ func applyTLSEnvOverrides(cfg *Config) { // An explicit 0 is meaningful (it disables the limiter), so the value is // materialized as a pointer; malformed values are warned about and ignored // so a typo cannot silently reshape the proxy's admission behavior. - if value := os.Getenv("MCPPROXY_MAX_CONCURRENT_REQUESTS"); value != "" { - if n, err := strconv.Atoi(value); err == nil && n >= 0 { - OverrideForProcess(cfg, FieldMaxConcurrentRequests, OverrideSourceEnv, &n) - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_MAX_CONCURRENT_REQUESTS=%q (want a non-negative integer)\n", value) - } - } - if value := os.Getenv("MCPPROXY_QUEUE_SIZE"); value != "" { - if n, err := strconv.Atoi(value); err == nil && n >= 0 { - OverrideForProcess(cfg, FieldQueueSize, OverrideSourceEnv, &n) - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_QUEUE_SIZE=%q (want a non-negative integer)\n", value) - } - } - if value := os.Getenv("MCPPROXY_QUEUE_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - qt := Duration(d) - OverrideForProcess(cfg, FieldQueueTimeout, OverrideSourceEnv, &qt) - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_QUEUE_TIMEOUT=%q (want a duration such as \"30s\")\n", value) - } - } + maxReqSet, maxReq := envNonNegativeInt("MCPPROXY_MAX_CONCURRENT_REQUESTS", "want a non-negative integer") + envOverride(b, cfg, FieldMaxConcurrentRequests, maxReqSet, maxReq) + queueSizeSet, queueSize := envNonNegativeInt("MCPPROXY_QUEUE_SIZE", "want a non-negative integer") + envOverride(b, cfg, FieldQueueSize, queueSizeSet, queueSize) + queueTimeoutSet, queueTimeout := envNonNegativeDuration("MCPPROXY_QUEUE_TIMEOUT", "want a duration such as \"30s\"") + envOverride(b, cfg, FieldQueueTimeout, queueTimeoutSet, queueTimeout) // Override the HTTP server's request deadlines from environment (GH #965) // — the escape hatch for operators who cannot edit the config file. An @@ -792,28 +770,42 @@ func applyTLSEnvOverrides(cfg *Config) { // ResolveHTTPIdleTimeout), so the value is materialized as a pointer; // malformed values are warned about and ignored so a typo cannot silently // reintroduce a response-truncating deadline. - if value := os.Getenv("MCPPROXY_HTTP_READ_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - rt := Duration(d) - OverrideForProcess(cfg, FieldHTTPReadTimeout, OverrideSourceEnv, &rt) - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_READ_TIMEOUT=%q (want a duration such as \"120s\", or \"0s\" to disable)\n", value) - } + readTimeoutSet, readTimeout := envNonNegativeDuration("MCPPROXY_HTTP_READ_TIMEOUT", "want a duration such as \"120s\", or \"0s\" to disable") + envOverride(b, cfg, FieldHTTPReadTimeout, readTimeoutSet, readTimeout) + writeTimeoutSet, writeTimeout := envNonNegativeDuration("MCPPROXY_HTTP_WRITE_TIMEOUT", "want a duration such as \"300s\", or \"0s\" to disable") + envOverride(b, cfg, FieldHTTPWriteTimeout, writeTimeoutSet, writeTimeout) + idleTimeoutSet, idleTimeout := envNonNegativeDuration("MCPPROXY_HTTP_IDLE_TIMEOUT", "want a duration such as \"180s\"; \"0s\" falls back to the read timeout") + envOverride(b, cfg, FieldHTTPIdleTimeout, idleTimeoutSet, idleTimeout) +} + +// envNonNegativeInt reads a non-negative integer env override; malformed +// values are warned about and ignored so a typo cannot silently reshape the +// proxy's behavior. +func envNonNegativeInt(name, want string) (bool, *int) { + value := os.Getenv(name) + if value == "" { + return false, nil } - if value := os.Getenv("MCPPROXY_HTTP_WRITE_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - wt := Duration(d) - OverrideForProcess(cfg, FieldHTTPWriteTimeout, OverrideSourceEnv, &wt) - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_WRITE_TIMEOUT=%q (want a duration such as \"300s\", or \"0s\" to disable)\n", value) - } + n, err := strconv.Atoi(value) + if err != nil || n < 0 { + fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid %s=%q (%s)\n", name, value, want) + return false, nil } - if value := os.Getenv("MCPPROXY_HTTP_IDLE_TIMEOUT"); value != "" { - if d, err := time.ParseDuration(value); err == nil && d >= 0 { - it := Duration(d) - OverrideForProcess(cfg, FieldHTTPIdleTimeout, OverrideSourceEnv, &it) - } else { - fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid MCPPROXY_HTTP_IDLE_TIMEOUT=%q (want a duration such as \"180s\"; \"0s\" falls back to the read timeout)\n", value) - } + return true, &n +} + +// envNonNegativeDuration reads a non-negative duration env override; malformed +// values are warned about and ignored. +func envNonNegativeDuration(name, want string) (bool, *Duration) { + value := os.Getenv(name) + if value == "" { + return false, nil + } + d, err := time.ParseDuration(value) + if err != nil || d < 0 { + fmt.Fprintf(os.Stderr, "WARN: Ignoring invalid %s=%q (%s)\n", name, value, want) + return false, nil } + dur := Duration(d) + return true, &dur } diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index 55196f65a..a8d791bca 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -194,6 +194,12 @@ type processOverride interface { // unreadable, in which case the value the file held at override time is // used). restore(out, base *Config) + // loadedValue is the file value recorded when the override was applied. + loadedValue() any + // reapply layers the process value back onto a freshly loaded cfg and + // returns the entry with its recorded file value refreshed (loaded is + // what cfg held before, or fileValue when the caller knows better). + reapply(cfg *Config, fileValue any, useFileValue bool) processOverride } type typedOverride[T any] struct { @@ -205,6 +211,7 @@ type typedOverride[T any] struct { func (o typedOverride[T]) name() string { return o.field.Name } func (o typedOverride[T]) source() OverrideSource { return o.src } +func (o typedOverride[T]) loadedValue() any { return o.loaded } func (o typedOverride[T]) restore(out, base *Config) { if !reflect.DeepEqual(o.field.Get(out), o.process) { @@ -217,12 +224,28 @@ func (o typedOverride[T]) restore(out, base *Config) { o.field.Set(out, fileValue) } +func (o typedOverride[T]) reapply(cfg *Config, fileValue any, useFileValue bool) processOverride { + o.loaded = o.field.Get(cfg) + if v, ok := fileValue.(T); ok && useFileValue { + o.loaded = v + } + o.field.Set(cfg, o.process) + return o +} + +// overrideKey identifies one registry entry: the same field may be overridden +// by env AND by a flag (the flag, applied later, wins in memory), and both +// records have to survive — a reload rebuilds the env set, and dropping the +// flag record with it would turn the flag's value into "an edit" on the next +// save. +type overrideKey struct { + field string + source OverrideSource +} + var ( processOverridesMu sync.RWMutex - // processOverrides is keyed by field name; a later override of the same - // field (a reload re-applying env, a flag applied after env) replaces the - // earlier one — one process value per field. - processOverrides = map[string]processOverride{} + processOverrides = map[overrideKey]processOverride{} ) // OverrideForProcess sets field f on cfg to value for THIS PROCESS ONLY and @@ -232,43 +255,114 @@ func OverrideForProcess[T any](cfg *Config, f Field[T], source OverrideSource, v if cfg == nil { return } - loaded := f.Get(cfg) + entry := newOverride(cfg, f, source, value) f.Set(cfg, value) processOverridesMu.Lock() - processOverrides[f.Name] = typedOverride[T]{field: f, src: source, process: value, loaded: loaded} + processOverrides[overrideKey{f.Name, source}] = entry processOverridesMu.Unlock() } -// clearProcessOverrides drops every override recorded from source. The loader -// calls it for OverrideSourceEnv before re-applying the environment on each -// load, so a reload reflects the variables set NOW; flag overrides, applied -// once at startup, survive reloads. -func clearProcessOverrides(source OverrideSource) { +// newOverride builds the record for applying value to f WITHOUT setting it. +// The recorded file value is what cfg holds now — unless this is a flag +// layered over an env override of the same field, whose record already +// knows the real file value. +func newOverride[T any](cfg *Config, f Field[T], source OverrideSource, value T) typedOverride[T] { + loaded := f.Get(cfg) + if source == OverrideSourceFlag { + processOverridesMu.RLock() + env, ok := processOverrides[overrideKey{f.Name, OverrideSourceEnv}] + processOverridesMu.RUnlock() + if ok { + if v, isT := env.loadedValue().(T); isT { + loaded = v + } + } + } + return typedOverride[T]{field: f, src: source, process: value, loaded: loaded} +} + +// envOverrideBatch collects the env overrides of one load and commits them in +// ONE registry update. The loader rebuilds the env set on every load (so a +// reload reflects the variables set now); clearing and re-adding under +// separate locks would leave a window in which a save on another goroutine — +// telemetry, the runtime — sees no env entries at all and persists them. +type envOverrideBatch struct { + managed []string // every field the loader consults, set or not + entries []processOverride // the ones that are set +} + +// consider applies an env override for f when present and, either way, marks +// f as env-managed so a stale entry for it is dropped at commit. +func envOverride[T any](b *envOverrideBatch, cfg *Config, f Field[T], present bool, value T) { + b.managed = append(b.managed, f.Name) + if !present { + return + } + b.entries = append(b.entries, newOverride(cfg, f, OverrideSourceEnv, value)) + f.Set(cfg, value) +} + +// commit atomically replaces the env entries of every managed field with the +// batch. Entries of other sources (flags) and env entries the loader does not +// manage (api_key, recorded by Validate/EnsureAPIKey) are untouched. +func (b *envOverrideBatch) commit() { processOverridesMu.Lock() defer processOverridesMu.Unlock() - for name, o := range processOverrides { - if o.source() == source { - delete(processOverrides, name) + for _, name := range b.managed { + delete(processOverrides, overrideKey{name, OverrideSourceEnv}) + } + for _, e := range b.entries { + processOverrides[overrideKey{e.name(), OverrideSourceEnv}] = e + } +} + +// ReapplyFlagOverrides layers every flag-sourced override back onto cfg — a +// config freshly loaded from the file, on which the loader has already +// re-applied the env overrides but knows nothing about the serve flags — and +// refreshes each record's file value. Without it a hot reload after an +// external edit silently switched `--read-only` (or any other flag) off. +func ReapplyFlagOverrides(cfg *Config) { + if cfg == nil { + return + } + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for key, o := range processOverrides { + if key.source != OverrideSourceFlag { + continue } + // Over an env override of the same field the config already carries + // the env value; the env record knows what the file said. + env, hasEnv := processOverrides[overrideKey{key.field, OverrideSourceEnv}] + var fileValue any + if hasEnv { + fileValue = env.loadedValue() + } + processOverrides[key] = o.reapply(cfg, fileValue, hasEnv) } } // ResetProcessOverrides forgets every recorded override. For tests. func ResetProcessOverrides() { processOverridesMu.Lock() - processOverrides = map[string]processOverride{} + processOverrides = map[overrideKey]processOverride{} processOverridesMu.Unlock() } // ProcessOverrideFields lists the names of the fields currently overridden for -// this process, sorted, for diagnostics and logging. +// this process, sorted and de-duplicated, for diagnostics and logging. func ProcessOverrideFields() []string { processOverridesMu.RLock() defer processOverridesMu.RUnlock() + seen := make(map[string]struct{}, len(processOverrides)) names := make([]string, 0, len(processOverrides)) - for name := range processOverrides { - names = append(names, name) + for key := range processOverrides { + if _, dup := seen[key.field]; dup { + continue + } + seen[key.field] = struct{}{} + names = append(names, key.field) } sort.Strings(names) return names @@ -284,6 +378,11 @@ func ProcessOverrideFields() []string { // The copy shares Servers, Registries and every nested block it does not touch // with effective; the ones it restores are copied first. Callers must not // mutate the shared structures. +// +// Known limitation: "edited" is inferred from the value. An explicit API edit +// that sets an overridden field to exactly the override's value is +// indistinguishable from a round trip and is not persisted — unreachable from +// the Web UI, which already shows the override as the current value. func PersistableConfig(effective *Config, path string) *Config { if effective == nil { return nil diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index 382127b15..8ebb3c65b 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -188,3 +188,151 @@ func TestLoadFromFile_ReplacesEnvOverridesButKeepsFlagOverrides(t *testing.T) { require.NoError(t, err) assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) } + +// Round-1 review findings. + +// EnsureAPIKey lets MCPPROXY_API_KEY win over a key the FILE holds; that is +// an override like Validate's and must not replace the file key on disk. +func TestEnsureAPIKey_EnvKeyOverFileKeyIsNotPersisted(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "api_key": "file-key", "mcpServers": []}`) + t.Setenv("MCPPROXY_API_KEY", "env-key") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "file-key", cfg.APIKey, "Validate only fills an EMPTY api_key from env") + + key, generated, source := cfg.EnsureAPIKey() + require.Equal(t, "env-key", key) + require.False(t, generated) + require.Equal(t, APIKeySourceEnvironment, source) + + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "file-key", readJSON(t, path)["api_key"]) +} + +// A key EnsureAPIKey generated is the one thing that must persist. +func TestEnsureAPIKey_GeneratedKeyIsPersisted(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + os.Unsetenv("MCPPROXY_API_KEY") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + key, generated, _ := cfg.EnsureAPIKey() + require.True(t, generated) + + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, key, readJSON(t, path)["api_key"]) +} + +// The same field overridden by env AND a flag keeps both records: the flag +// wins in memory, neither leaks, and a reload (which rebuilds the env set) +// must not turn the flag's value into "an edit" by dropping its record. +func TestOverrides_EnvAndFlagOnTheSameFieldBothSurviveReload(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:9000") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9999") + require.Equal(t, "127.0.0.1:9999", cfg.Listen) + + // The reload: the loader rebuilds the env entries on a fresh config. + reloaded, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "127.0.0.1:9000", reloaded.Listen, "the loader applies env") + ReapplyFlagOverrides(reloaded) + assert.Equal(t, "127.0.0.1:9999", reloaded.Listen, "flag overrides are re-applied on reload") + + require.NoError(t, SaveConfig(reloaded, path)) + assert.Equal(t, "127.0.0.1:8080", readJSON(t, path)["listen"]) + + // The original (pre-reload) effective config saves the same way. + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "127.0.0.1:8080", readJSON(t, path)["listen"]) +} + +// The flag record's load-time fallback is the FILE value, not the env value +// the flag happened to be layered over. +func TestOverrides_FlagOverEnvFallsBackToFileValue(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:9000") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9999") + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, "127.0.0.1:8080", persisted.Listen) +} + +// ReapplyFlagOverrides re-layers every flag-sourced override onto a freshly +// loaded config and refreshes the recorded file value. +func TestReapplyFlagOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"read_only_mode": false, "logging": {"level": "info"}, "mcpServers": []}`) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + OverrideForProcess(cfg, FieldLogLevel, OverrideSourceFlag, "debug") + + // The file changed and was reloaded. + require.NoError(t, os.WriteFile(path, []byte(`{"read_only_mode": false, "logging": {"level": "warn"}, "mcpServers": []}`), 0o600)) + reloaded, err := LoadFromFile(path) + require.NoError(t, err) + require.Equal(t, "warn", reloaded.Logging.Level) + ReapplyFlagOverrides(reloaded) + assert.True(t, reloaded.ReadOnlyMode) + assert.Equal(t, "debug", reloaded.Logging.Level) + + persisted := PersistableConfig(reloaded, filepath.Join(t.TempDir(), "missing.json")) + assert.False(t, persisted.ReadOnlyMode) + assert.Equal(t, "warn", persisted.Logging.Level, "the fallback tracks the RELOADED file value") +} + +// Rebuilding the env set on a reload must be atomic with respect to saves on +// other goroutines: a save that lands mid-rebuild must never see an empty (or +// half-built) registry and persist the overrides. +func TestOverrides_EnvRebuildIsAtomicWithSaves(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_LISTEN", "0.0.0.0:9999") + t.Setenv("MCPPROXY_TOOL_RESPONSE_MODE", "compact") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-stop: + return + default: + } + _, _ = LoadFromFile(path) // rebuilds the env entries + } + }() + for i := 0; i < 200; i++ { + persisted := PersistableConfig(cfg, path) + if persisted.Listen != "127.0.0.1:8080" || persisted.ToolResponseMode != "full" { + close(stop) + <-done + t.Fatalf("iteration %d: env override leaked mid-rebuild: listen=%q mode=%q", i, persisted.Listen, persisted.ToolResponseMode) + } + } + close(stop) + <-done +} diff --git a/internal/runtime/configsvc/service.go b/internal/runtime/configsvc/service.go index 0aa97daaf..3b93dfc02 100644 --- a/internal/runtime/configsvc/service.go +++ b/internal/runtime/configsvc/service.go @@ -216,6 +216,10 @@ func (s *Service) ReloadFromFile() (*Snapshot, error) { if err != nil { return nil, fmt.Errorf("failed to load config from %s: %w", current.Path, err) } + // The loader re-applies the MCPPROXY_* env overrides but knows nothing + // about the serve flags; the effective config is the file PLUS both, so a + // hand edit of an unrelated key must not switch `--read-only` off. + config.ReapplyFlagOverrides(newConfig) // Update atomically if err := s.Update(newConfig, UpdateTypeReload, "file_reload"); err != nil { diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index f6314c668..289170d8a 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1583,6 +1583,7 @@ func (r *Runtime) ReloadConfiguration() error { if loadErr != nil { return fmt.Errorf("failed to reload config: %w", loadErr) } + config.ReapplyFlagOverrides(newConfig) // Already holding configCommitMu; use the locked helper so we don't // re-acquire the non-reentrant mutex (would deadlock). r.updateConfigLocked(newConfig, cfgPath) diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index 17a8563f5..fdc38bc65 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -141,3 +141,30 @@ func TestConfigWatcher_OwnSaveWithOverridesIsNotAnExternalEdit(t *testing.T) { assert.Equal(t, "compact", live.ToolResponseMode, "the daemon's own save must not reload the file over the flag") assert.True(t, live.ReadOnlyMode) } + +// A genuine external edit reloads the file; the loader re-applies env +// overrides, and the runtime must re-apply the serve flags the same way, or a +// hand edit of an unrelated key silently switches --read-only off. +func TestReloadConfiguration_KeepsFlagOverridesEffective(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 // the external edit + require.NoError(t, config.SaveConfig(edited, cfgPath)) + + require.NoError(t, rt.ReloadConfiguration()) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, 77, live.ToolsLimit, "the external edit is adopted") + assert.True(t, live.ReadOnlyMode, "--read-only survives a reload") + assert.Equal(t, "compact", live.ToolResponseMode, "--tool-response-mode survives a reload") + assert.Equal(t, ":0", live.Listen) + + // …and the next save still does not persist them. + require.NoError(t, rt.SaveConfiguration()) + m := readConfigJSON(t, cfgPath) + assertNoOverridesOnDisk(t, m) + assert.Equal(t, float64(77), m["tools_limit"]) +} From 3cdd47e7d1910139fc099b3cb122f15f54ce2d4c Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 10:49:56 +0300 Subject: [PATCH 09/18] fix(config): retire superseded overrides; re-apply flags on the running config only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2: - An override the running config no longer carries was superseded by an API edit (a hot apply, UpdateListenAddress). It is now retired (RetireSupersededOverrides, called from ApplyConfig with what the process adopted and from SaveConfiguration with the live config), so a reload no longer resurrects the flag over the edit, and — the sharper case — an edit BACK to the override's value persists as the edit it is instead of being swapped for the file value. - ReapplyFlagOverrides(cfg, live) runs on the runtime's pinned live copy, not inside configsvc.ReloadFromFile: the desired config stays the file, so a pending file edit of a restart-gated flag field (listen) is still reported as pending and not clobbered by a later save. The pinned config is republished whenever it differs from the raw file. - A repeated registration of the same override (loadConfig and runServer both applied --tool-response-limit) inherits the previous record's file value; the duplicate registration is also removed. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy/main.go | 4 - cmd/mcpproxy/serve_flag_persistence_test.go | 21 ++++++ internal/config/process_overrides.go | 62 +++++++++++++--- internal/config/process_overrides_test.go | 73 ++++++++++++++++++- internal/runtime/configsvc/service.go | 4 - internal/runtime/lifecycle.go | 37 ++++++++-- .../runtime/process_overrides_persist_test.go | 68 +++++++++++++++++ internal/runtime/runtime.go | 5 ++ 8 files changed, 250 insertions(+), 24 deletions(-) diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index fd43f49f2..1a18b0d7b 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -853,10 +853,6 @@ func applyServeRuntimeFlags(cmd *cobra.Command, cfg *config.Config) { cmdDebugSearch, _ := flags.GetBool("debug-search") config.OverrideForProcess(cfg, config.FieldDebugSearch, config.OverrideSourceFlag, cmdDebugSearch) - if cmdToolResponseLimit, _ := flags.GetInt("tool-response-limit"); cmdToolResponseLimit != 0 { - config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, cmdToolResponseLimit) - } - // Apply security settings from command line ONLY if explicitly set for _, f := range []struct { flag string diff --git a/cmd/mcpproxy/serve_flag_persistence_test.go b/cmd/mcpproxy/serve_flag_persistence_test.go index 126d28708..c67a11277 100644 --- a/cmd/mcpproxy/serve_flag_persistence_test.go +++ b/cmd/mcpproxy/serve_flag_persistence_test.go @@ -471,3 +471,24 @@ func TestServeFlagOverrideEditedViaAPIIsPersisted(t *testing.T) { 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) +} diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index a8d791bca..0372d060a 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -196,6 +196,8 @@ type processOverride interface { restore(out, base *Config) // loadedValue is the file value recorded when the override was applied. loadedValue() any + // supersededBy reports whether live no longer carries the process value. + supersededBy(live *Config) bool // reapply layers the process value back onto a freshly loaded cfg and // returns the entry with its recorded file value refreshed (loaded is // what cfg held before, or fileValue when the caller knows better). @@ -224,6 +226,11 @@ func (o typedOverride[T]) restore(out, base *Config) { o.field.Set(out, fileValue) } +// supersededBy reports whether live no longer carries the process value. +func (o typedOverride[T]) supersededBy(live *Config) bool { + return !reflect.DeepEqual(o.field.Get(live), o.process) +} + func (o typedOverride[T]) reapply(cfg *Config, fileValue any, useFileValue bool) processOverride { o.loaded = o.field.Get(cfg) if v, ok := fileValue.(T); ok && useFileValue { @@ -267,16 +274,25 @@ func OverrideForProcess[T any](cfg *Config, f Field[T], source OverrideSource, v // The recorded file value is what cfg holds now — unless this is a flag // layered over an env override of the same field, whose record already // knows the real file value. +// +// A repeated registration of the same override (loadConfig and runServer both +// apply --tool-response-limit; Validate runs twice) finds the previous +// override already in place, so the file value is inherited from the +// existing record rather than read off cfg. func newOverride[T any](cfg *Config, f Field[T], source OverrideSource, value T) typedOverride[T] { loaded := f.Get(cfg) - if source == OverrideSourceFlag { - processOverridesMu.RLock() - env, ok := processOverrides[overrideKey{f.Name, OverrideSourceEnv}] - processOverridesMu.RUnlock() - if ok { - if v, isT := env.loadedValue().(T); isT { - loaded = v - } + processOverridesMu.RLock() + prev, hasPrev := processOverrides[overrideKey{f.Name, source}] + env, hasEnv := processOverrides[overrideKey{f.Name, OverrideSourceEnv}] + processOverridesMu.RUnlock() + if hasPrev { + if p, ok := prev.(typedOverride[T]); ok && reflect.DeepEqual(loaded, p.process) { + loaded = p.loaded + } + } + if source == OverrideSourceFlag && hasEnv { + if v, ok := env.loadedValue().(T); ok { + loaded = v } } return typedOverride[T]{field: f, src: source, process: value, loaded: loaded} @@ -322,7 +338,11 @@ func (b *envOverrideBatch) commit() { // re-applied the env overrides but knows nothing about the serve flags — and // refreshes each record's file value. Without it a hot reload after an // external edit silently switched `--read-only` (or any other flag) off. -func ReapplyFlagOverrides(cfg *Config) { +// +// live is the config this process was running before the reload. A flag the +// live config no longer carries was superseded by an API edit (which is on +// disk by now); it is retired rather than resurrected over that edit. +func ReapplyFlagOverrides(cfg, live *Config) { if cfg == nil { return } @@ -332,6 +352,10 @@ func ReapplyFlagOverrides(cfg *Config) { if key.source != OverrideSourceFlag { continue } + if live != nil && o.supersededBy(live) { + delete(processOverrides, key) + continue + } // Over an env override of the same field the config already carries // the env value; the env record knows what the file said. env, hasEnv := processOverrides[overrideKey{key.field, OverrideSourceEnv}] @@ -343,6 +367,26 @@ func ReapplyFlagOverrides(cfg *Config) { } } +// RetireSupersededOverrides forgets every override the running config no +// longer carries. An API edit that moved an overridden field to another +// value has superseded the override for this process: from then on the field +// is ordinary, so an edit BACK to the override's value persists as the edit +// it is instead of being swapped for the file value. Call it with the config +// this process actually runs (never with a file-derived one, whose values +// differ from every override by construction). +func RetireSupersededOverrides(live *Config) { + if live == nil { + return + } + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for key, o := range processOverrides { + if o.supersededBy(live) { + delete(processOverrides, key) + } + } +} + // ResetProcessOverrides forgets every recorded override. For tests. func ResetProcessOverrides() { processOverridesMu.Lock() diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index 8ebb3c65b..6fc63a02f 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -246,7 +246,7 @@ func TestOverrides_EnvAndFlagOnTheSameFieldBothSurviveReload(t *testing.T) { reloaded, err := LoadFromFile(path) require.NoError(t, err) require.Equal(t, "127.0.0.1:9000", reloaded.Listen, "the loader applies env") - ReapplyFlagOverrides(reloaded) + ReapplyFlagOverrides(reloaded, nil) assert.Equal(t, "127.0.0.1:9999", reloaded.Listen, "flag overrides are re-applied on reload") require.NoError(t, SaveConfig(reloaded, path)) @@ -290,7 +290,7 @@ func TestReapplyFlagOverrides(t *testing.T) { reloaded, err := LoadFromFile(path) require.NoError(t, err) require.Equal(t, "warn", reloaded.Logging.Level) - ReapplyFlagOverrides(reloaded) + ReapplyFlagOverrides(reloaded, nil) assert.True(t, reloaded.ReadOnlyMode) assert.Equal(t, "debug", reloaded.Logging.Level) @@ -336,3 +336,72 @@ func TestOverrides_EnvRebuildIsAtomicWithSaves(t *testing.T) { close(stop) <-done } + +// Round-2 review findings. + +// A flag the API superseded in this process (a hot edit to a different +// value) must not come back on a reload: the edit is on disk, the flag is +// retired. +func TestReapplyFlagOverrides_SkipsAFlagTheLiveConfigSuperseded(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"read_only_mode": false, "tool_response_mode": "full", "mcpServers": []}`) + + live, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(live, FieldReadOnlyMode, OverrideSourceFlag, false) // explicit --read-only=false + OverrideForProcess(live, FieldToolResponseMode, OverrideSourceFlag, "compact") + + live.ReadOnlyMode = true // the API edit, applied hot and persisted + require.NoError(t, os.WriteFile(path, []byte(`{"read_only_mode": true, "tool_response_mode": "full", "tools_limit": 5, "mcpServers": []}`), 0o600)) + + reloaded, err := LoadFromFile(path) + require.NoError(t, err) + ReapplyFlagOverrides(reloaded, live) + assert.True(t, reloaded.ReadOnlyMode, "the superseded flag must not be resurrected") + assert.Equal(t, "compact", reloaded.ToolResponseMode, "the untouched flag is re-applied") + assert.Equal(t, []string{"tool_response_mode"}, ProcessOverrideFields(), "the superseded flag is retired") +} + +// Once the running config carries a different value than the override, the +// override is retired: a later edit back to the override's value is then an +// ordinary edit and persists (it used to restore the file value instead). +func TestRetireSupersededOverrides(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"tool_response_mode": "full", "listen": "127.0.0.1:8080", "mcpServers": []}`) + + cfg, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceFlag, "compact") + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") + + // The API sets the hot field to something else; listen stays pinned. + cfg.ToolResponseMode = "full" + require.NoError(t, SaveConfig(cfg, path)) + RetireSupersededOverrides(cfg) + assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) + + // …and back to the flag's value: a real edit now. + cfg.ToolResponseMode = "compact" + require.NoError(t, SaveConfig(cfg, path)) + m := readJSON(t, path) + assert.Equal(t, "compact", m["tool_response_mode"]) + assert.Equal(t, "127.0.0.1:8080", m["listen"], "the pinned flag is still not persisted") +} + +// Registering the same override twice (loadConfig and runServer both apply +// --tool-response-limit; Validate runs twice) must keep the FILE value as the +// fallback, not the flag value the second registration finds in place. +func TestOverrideForProcess_RepeatedRegistrationKeepsTheFileFallback(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.ToolResponseLimit = 20000 + OverrideForProcess(cfg, FieldToolResponseLimit, OverrideSourceFlag, 500) + OverrideForProcess(cfg, FieldToolResponseLimit, OverrideSourceFlag, 500) + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.Equal(t, 20000, persisted.ToolResponseLimit) +} diff --git a/internal/runtime/configsvc/service.go b/internal/runtime/configsvc/service.go index 3b93dfc02..0aa97daaf 100644 --- a/internal/runtime/configsvc/service.go +++ b/internal/runtime/configsvc/service.go @@ -216,10 +216,6 @@ func (s *Service) ReloadFromFile() (*Snapshot, error) { if err != nil { return nil, fmt.Errorf("failed to load config from %s: %w", current.Path, err) } - // The loader re-applies the MCPPROXY_* env overrides but knows nothing - // about the serve flags; the effective config is the file PLUS both, so a - // hand edit of an unrelated key must not switch `--read-only` off. - config.ReapplyFlagOverrides(newConfig) // Update atomically if err := s.Update(newConfig, UpdateTypeReload, "file_reload"); err != nil { diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 289170d8a..f00968d14 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1,7 +1,9 @@ package runtime import ( + "bytes" "context" + "encoding/json" "fmt" "strings" "time" @@ -1412,6 +1414,11 @@ func (r *Runtime) SaveConfiguration() error { return fmt.Errorf("failed to clone configuration") } + // A serve flag / env override the running config no longer carries was + // superseded (UpdateListenAddress, an earlier hot apply); retire it so + // the field is persisted like any other from now on. + config.RetireSupersededOverrides(configCopy) + // Update servers with latest from storage configCopy.Servers = latestServers @@ -1583,7 +1590,10 @@ func (r *Runtime) ReloadConfiguration() error { if loadErr != nil { return fmt.Errorf("failed to reload config: %w", loadErr) } - config.ReapplyFlagOverrides(newConfig) + r.mu.RLock() + live := r.cfg + r.mu.RUnlock() + config.ReapplyFlagOverrides(newConfig, live) // Already holding configCommitMu; use the locked helper so we don't // re-acquire the non-reentrant mutex (would deadlock). r.updateConfigLocked(newConfig, cfgPath) @@ -1610,15 +1620,24 @@ func (r *Runtime) ReloadConfiguration() error { // a surface nobody was being served, and pinRestartGated on the apply // path would pin to a value that was never live. r.mu.RLock() - pinned := pinRestartGated(r.cfg, newSnapshot.Config) + live := r.cfg + pinned := pinRestartGated(live, newSnapshot.Config) r.mu.RUnlock() + // The loader re-applied the MCPPROXY_* env overrides but knows nothing + // about the serve flags; the RUNNING config is the file plus both, so + // a hand edit of an unrelated key must not switch `--read-only` off. + // Applied to the pinned copy only (nested blocks copy-on-write): the + // desired config below stays the file, so a pending file edit of a + // restart-gated flag field (listen) is still reported as pending. + config.ReapplyFlagOverrides(pinned, live) + // Republish so the configsvc snapshot and r.cfg cannot disagree: // ReloadFromFile has already published the RAW file, which live // subscribers would read as the running configuration. Skipped when - // nothing is pending — the common case, where pinned is equivalent to - // what ReloadFromFile just published. - if DetectConfigChanges(newSnapshot.Config, pinned).RequiresRestart { + // nothing is pending and no flag differs — the common case, where + // pinned is equivalent to what ReloadFromFile just published. + if DetectConfigChanges(newSnapshot.Config, pinned).RequiresRestart || !configsEquivalent(newSnapshot.Config, pinned) { if uerr := r.configSvc.Update(pinned, configsvc.UpdateTypeModify, "reload_pin_restart_gated"); uerr != nil { r.logger.Error("Failed to republish the pinned configuration after reload", zap.Error(uerr)) } @@ -2252,3 +2271,11 @@ func (r *Runtime) supervisorEventForwarder() { } } } + +// configsEquivalent reports whether two configs marshal to the same JSON — +// the same comparison the config watcher uses to recognise its own saves. +func configsEquivalent(a, b *config.Config) bool { + ja, errA := json.Marshal(a) + jb, errB := json.Marshal(b) + return errA == nil && errB == nil && bytes.Equal(ja, jb) +} diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index fdc38bc65..2b6e72dfe 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -168,3 +168,71 @@ func TestReloadConfiguration_KeepsFlagOverridesEffective(t *testing.T) { assertNoOverridesOnDisk(t, m) assert.Equal(t, float64(77), m["tools_limit"]) } + +// A hot API edit of an overridden field supersedes the flag for this process: +// a later external edit of an unrelated key must not resurrect it on reload. +func TestReloadConfiguration_DoesNotResurrectAFlagTheAPISuperseded(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.ToolResponseMode = "full" // the API turns the flag's choice off + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + require.Equal(t, "full", edited.ToolResponseMode) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, "full", live.ToolResponseMode, "the API edit survives the reload") + assert.True(t, live.ReadOnlyMode, "the untouched flag survives the reload") +} + +// Toggling an overridden hot field away from the flag and back again via the +// API: the second edit is a real edit and must persist, not be swapped for +// the file's value. +func TestApplyConfig_TogglingAnOverriddenFieldBackPersists(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.ToolResponseMode = "full" + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + assert.Equal(t, "full", readConfigJSON(t, cfgPath)["tool_response_mode"]) + + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + desired.ToolResponseMode = "compact" + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + assert.Equal(t, "compact", readConfigJSON(t, cfgPath)["tool_response_mode"]) +} + +// A reload must keep the DESIRED config equal to the file: a restart-gated +// flag (listen) is re-applied to the running config only, so a pending file +// edit of that field is still reported as pending and not clobbered. +func TestReloadConfiguration_RestartGatedFlagDoesNotHideAPendingFileEdit(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + edited.Listen = "127.0.0.1:9090" + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.Equal(t, ":0", live.Listen, "the bound listener keeps the flag value") + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + assert.Equal(t, "127.0.0.1:9090", desired.Listen, "the desired config is the file") + + require.NoError(t, rt.SaveConfiguration()) + assert.Equal(t, "127.0.0.1:9090", readConfigJSON(t, cfgPath)["listen"], "a later save keeps the pending edit") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 4320f5cff..839fb6c17 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1790,6 +1790,11 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // smuggling the still-pending value into memory. hotCfg := pinRestartGated(r.cfg, newCfg) + // An overridden hot field this apply moved away from its serve flag / env + // value is superseded for this process: retire the override so an edit + // BACK to that value later persists as the edit it is. + config.RetireSupersededOverrides(hotCfg) + // What this process can actually adopt, always computed against the running // config — never against the desired one `result` was diffed from, which can // hold a value that was never live. Restart-gated fields are equal by From 5604c273aa225553cc49aecfb5c5917cb374ab73 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 10:58:30 +0300 Subject: [PATCH 10/18] fix(runtime): reload side effects follow the running config, not the raw file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 3 (Sol, partial — quota): after republishing the pinned + flag-reapplied config, ReloadConfiguration kept feeding the RAW file snapshot to the upstream manager, applyComponentConfigLocked (truncator, logging), telemetry and the update checker — so a hot reload rebuilt the truncator from the file's tool_response_limit while r.cfg said the flag's. Parity with ApplyConfig, which applies hotCfg: every side effect now takes the running config; the restart-required warning still diffs the file. Co-Authored-By: Claude Opus 5 --- internal/runtime/lifecycle.go | 28 +++++++++---- .../runtime/process_overrides_persist_test.go | 39 +++++++++++++++++++ internal/truncate/truncator.go | 5 +++ 3 files changed, 64 insertions(+), 8 deletions(-) diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index f00968d14..3a303452a 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1604,6 +1604,15 @@ func (r *Runtime) ReloadConfiguration() error { return fmt.Errorf("failed to reload config: %w", err) } + // fileCfg is the file as reloaded — the DESIRED config. running is what + // this process adopts from it: restart-gated fields pinned to the live + // values and the serve flags re-applied. They coincide unless something is + // pending or a flag is in force; every per-component side effect below + // follows running (parity with ApplyConfig, which applies hotCfg), while + // the restart-required warning diffs the file. + fileCfg := newSnapshot.Config + running := newSnapshot.Config + // Sync the legacy r.cfg/r.cfgPath fields too: Runtime.GetConfig() still // backs GET/PATCH /api/v1/config and other httpapi handlers. Without this, // a disk reload only lands in the configsvc snapshot — the API keeps @@ -1637,11 +1646,14 @@ func (r *Runtime) ReloadConfiguration() error { // subscribers would read as the running configuration. Skipped when // nothing is pending and no flag differs — the common case, where // pinned is equivalent to what ReloadFromFile just published. - if DetectConfigChanges(newSnapshot.Config, pinned).RequiresRestart || !configsEquivalent(newSnapshot.Config, pinned) { + if DetectConfigChanges(fileCfg, pinned).RequiresRestart || !configsEquivalent(fileCfg, pinned) { if uerr := r.configSvc.Update(pinned, configsvc.UpdateTypeModify, "reload_pin_restart_gated"); uerr != nil { r.logger.Error("Failed to republish the pinned configuration after reload", zap.Error(uerr)) + } else { + newSnapshot = r.configSvc.Current() } } + running = pinned r.mu.Lock() r.cfg = pinned @@ -1649,7 +1661,7 @@ func (r *Runtime) ReloadConfiguration() error { // including over an API change that was still waiting for a restart: // whoever edited the file wins, and nothing may keep merging onto a // base the file no longer agrees with. - r.desiredCfg = newSnapshot.Config + r.desiredCfg = fileCfg if newSnapshot.Path != "" { r.cfgPath = newSnapshot.Path } @@ -1663,8 +1675,8 @@ func (r *Runtime) ReloadConfiguration() error { // ConfigApplyResult; the disk path had no channel at all, so at least make // it loud in the log. Log-only on purpose: auto-restarting on a file save // would be far more surprising than a stale deadline. - if oldSnapshot != nil && oldSnapshot.Config != nil && newSnapshot != nil && newSnapshot.Config != nil { - if result := DetectConfigChanges(oldSnapshot.Config, newSnapshot.Config); result.RequiresRestart { + if oldSnapshot != nil && oldSnapshot.Config != nil && fileCfg != nil { + if result := DetectConfigChanges(oldSnapshot.Config, fileCfg); result.RequiresRestart { r.logger.Warn("Config file change includes restart-required fields; the running server keeps the old values until restart", zap.Strings("changed_fields", result.ChangedFields), zap.String("reason", result.RestartReason)) @@ -1677,7 +1689,7 @@ func (r *Runtime) ReloadConfiguration() error { // health_check_interval from this, so external edits must reach it too — // not only API applies. if r.upstreamManager != nil { - r.upstreamManager.SetGlobalConfig(newSnapshot.Config) + r.upstreamManager.SetGlobalConfig(running) } // Parity with ApplyConfig's live per-component side effects (PR #857 @@ -1686,7 +1698,7 @@ func (r *Runtime) ReloadConfiguration() error { // external edit lands in the snapshot/API while the running components // keep their stale values. r.mu.Lock() - r.applyComponentConfigLocked(oldSnapshot.Config, newSnapshot.Config) + r.applyComponentConfigLocked(oldSnapshot.Config, running) r.mu.Unlock() if err := r.LoadConfiguredServers(nil); err != nil { @@ -1700,12 +1712,12 @@ func (r *Runtime) ReloadConfiguration() error { // fsnotify config file watcher (config_watcher.go), which funnels external // file edits into this method. nil-safe + fire-and-forget. if r.telemetryService != nil { - r.telemetryService.NotifyConfigChanged(newSnapshot.Config) + r.telemetryService.NotifyConfigChanged(running) } // Spec 079 FR-012: re-gate the update checker on the disk-reload path too // (ApplyConfig covers the API path). SetConfig no-ops when unchanged. - r.applyUpdateCheckConfig(newSnapshot.Config) + r.applyUpdateCheckConfig(running) go r.postConfigReload() diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index 2b6e72dfe..cdd723a23 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -236,3 +236,42 @@ func TestReloadConfiguration_RestartGatedFlagDoesNotHideAPendingFileEdit(t *test require.NoError(t, rt.SaveConfiguration()) assert.Equal(t, "127.0.0.1:9090", readConfigJSON(t, cfgPath)["listen"], "a later save keeps the pending edit") } + +// The reload's per-component side effects (upstream manager global config, +// truncator, logging, telemetry) must follow the RUNNING config — the file +// plus the flags — exactly as ApplyConfig applies hotCfg, not the raw file. +func TestReloadConfiguration_ComponentsFollowTheRunningConfig(t *testing.T) { + t.Cleanup(config.ResetProcessOverrides) + config.ResetProcessOverrides() + + tmp := t.TempDir() + cfgPath := filepath.Join(tmp, "mcp_config.json") + initial := config.DefaultConfig() + initial.DataDir = tmp + initial.ToolResponseLimit = 20000 + initial.ToolResponseMode = "full" + require.NoError(t, config.SaveConfig(initial, cfgPath)) + + cfg, err := config.ReadFile(cfgPath) + require.NoError(t, err) + config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, 500) + config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, "compact") + + rt, err := New(cfg, cfgPath, zap.NewNop()) + require.NoError(t, err) + t.Cleanup(func() { _ = rt.Close() }) + require.Equal(t, 500, rt.Truncator().Limit()) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + assert.Equal(t, 500, rt.Truncator().Limit(), "the truncator must not be rebuilt from the file's limit") + if um := rt.upstreamManager; um != nil { + assert.Equal(t, "compact", um.GlobalConfig().ToolResponseMode, "the upstream manager must see the running config") + } + snap := rt.ConfigSnapshot() + assert.Equal(t, "compact", snap.Config.ToolResponseMode, "the published snapshot is the running config") +} diff --git a/internal/truncate/truncator.go b/internal/truncate/truncator.go index de93d61d6..b7487c2b8 100644 --- a/internal/truncate/truncator.go +++ b/internal/truncate/truncator.go @@ -28,6 +28,11 @@ func NewTruncator(limit int) *Truncator { return &Truncator{limit: limit} } +// Limit returns the character limit this truncator applies. +func (t *Truncator) Limit() int { + return t.limit +} + // Truncate analyzes and truncates a tool response if it exceeds the limit. // The record array the resulting cache handle pages is inferred from the // payload; callers that already know which array their truncation banner From 8c06f06b72304dc97c6cbf34925385ba496c1339 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:06:37 +0300 Subject: [PATCH 11/18] fix(config): one effective override per field; retire on what the API saved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4 (codex gpt-5.6-sol; the opencode Copilot models are out of monthly quota): - A flag shadows an env override of the same field, so only that winner may decide whether the field was edited. The shadowed env record used to intercept an API edit that happened to equal the env value and restore the file value instead. PersistableConfig and RetireSupersededOverrides now resolve one effective override per field; a superseded field forgets its whole stack (the loader re-records env on the next reload). - ApplyConfig retires against what the API SAVED (newCfg), not the pinned hotCfg: editing listen under --listen ends that override for this process even though the listener stays bound, so an edit back to the flag's value ("cancel the pending change") is persisted as asked — disk, the desired config and the API result agree again. Co-Authored-By: Claude Opus 5 --- internal/config/process_overrides.go | 48 ++++++++++++++++--- internal/config/process_overrides_test.go | 39 +++++++++++++++ .../runtime/process_overrides_persist_test.go | 29 +++++++++++ internal/runtime/runtime.go | 8 ++-- 4 files changed, 114 insertions(+), 10 deletions(-) diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index 0372d060a..45e5d27d7 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -353,7 +353,11 @@ func ReapplyFlagOverrides(cfg, live *Config) { continue } if live != nil && o.supersededBy(live) { + // Superseded by an API edit: the field is API-managed now, so + // the env record beneath the flag goes too (the loader has + // just re-recorded it; see RetireSupersededOverrides). delete(processOverrides, key) + delete(processOverrides, overrideKey{key.field, OverrideSourceEnv}) continue } // Over an env override of the same field the config already carries @@ -367,6 +371,27 @@ func ReapplyFlagOverrides(cfg, live *Config) { } } +// effectiveOverridesLocked returns the one override that is in force per +// field: a flag shadows an env override of the same field (the flag is +// applied after the env value, so it is what the process actually runs with). +// Only the winner may decide whether the field was edited — the shadowed env +// record would otherwise intercept an API edit that happens to equal the env +// value. Caller must hold processOverridesMu (read or write). +func effectiveOverridesLocked() []processOverride { + winners := make(map[string]processOverride, len(processOverrides)) + for key, o := range processOverrides { + if prev, ok := winners[key.field]; ok && prev.source() == OverrideSourceFlag { + continue + } + winners[key.field] = o + } + out := make([]processOverride, 0, len(winners)) + for _, o := range winners { + out = append(out, o) + } + return out +} + // RetireSupersededOverrides forgets every override the running config no // longer carries. An API edit that moved an overridden field to another // value has superseded the override for this process: from then on the field @@ -374,15 +399,27 @@ func ReapplyFlagOverrides(cfg, live *Config) { // it is instead of being swapped for the file value. Call it with the config // this process actually runs (never with a file-derived one, whose values // differ from every override by construction). +// +// live is the config the process adopted, or — for a restart-gated field the +// API edited but the process cannot adopt — the config the API saved: an +// operator editing listen under --listen has ended that override for this +// process even though the listener stays bound, and a later edit back to the +// flag's value must persist as asked. +// +// A superseded field forgets its whole stack (flag and env): the field is +// API-managed from now on. func RetireSupersededOverrides(live *Config) { if live == nil { return } processOverridesMu.Lock() defer processOverridesMu.Unlock() - for key, o := range processOverrides { - if o.supersededBy(live) { - delete(processOverrides, key) + for _, o := range effectiveOverridesLocked() { + if !o.supersededBy(live) { + continue + } + for _, source := range []OverrideSource{OverrideSourceFlag, OverrideSourceEnv} { + delete(processOverrides, overrideKey{o.name(), source}) } } } @@ -432,10 +469,7 @@ func PersistableConfig(effective *Config, path string) *Config { return nil } processOverridesMu.RLock() - overrides := make([]processOverride, 0, len(processOverrides)) - for _, o := range processOverrides { - overrides = append(overrides, o) - } + overrides := effectiveOverridesLocked() processOverridesMu.RUnlock() if len(overrides) == 0 { return effective diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index 6fc63a02f..ceac66f1e 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -405,3 +405,42 @@ func TestOverrideForProcess_RepeatedRegistrationKeepsTheFileFallback(t *testing. persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) assert.Equal(t, 20000, persisted.ToolResponseLimit) } + +// Round-4 review findings. + +// With env AND a flag on the same field only the flag is effective; the env +// record must not intercept an API edit that happens to equal the env value. +func TestPersistableConfig_StackedEnvAndFlag_EditToTheEnvValuePersists(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"direct_tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE", "deferred") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") + + cfg.DirectToolResponseMode = "deferred" // the API edit: visibly different from "compact" + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) +} + +// Retiring a superseded field forgets the whole stack, not only the winner. +func TestRetireSupersededOverrides_RetiresTheWholeStack(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"direct_tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE", "deferred") + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") + + cfg.DirectToolResponseMode = "deferred" + RetireSupersededOverrides(cfg) + assert.NotContains(t, ProcessOverrideFields(), "direct_tool_response_mode") + + cfg.DirectToolResponseMode = "compact" // back to the (retired) flag value: an ordinary edit + require.NoError(t, SaveConfig(cfg, path)) + assert.Equal(t, "compact", readJSON(t, path)["direct_tool_response_mode"]) +} diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index cdd723a23..e9ba90c92 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -275,3 +275,32 @@ func TestReloadConfiguration_ComponentsFollowTheRunningConfig(t *testing.T) { snap := rt.ConfigSnapshot() assert.Equal(t, "compact", snap.Config.ToolResponseMode, "the published snapshot is the running config") } + +// An API edit of a restart-gated overridden field (listen under --listen) +// ends the override for this process even though the listener stays bound: +// a second edit back to the flag's value is then persisted as asked, so disk, +// the desired config and the API result agree. +func TestApplyConfig_EditingListenUnderAFlagEndsTheOverride(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.Listen = "127.0.0.1:9090" + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + require.Equal(t, "127.0.0.1:9090", readConfigJSON(t, cfgPath)["listen"]) + + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, "127.0.0.1:9090", desired.Listen) + desired.Listen = ":0" // cancel: back to what this process is bound to + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + assert.Equal(t, ":0", readConfigJSON(t, cfgPath)["listen"], "the explicit edit is persisted as asked") + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + assert.Equal(t, ":0", desired.Listen) + assert.NotContains(t, config.ProcessOverrideFields(), "listen") + assert.Contains(t, config.ProcessOverrideFields(), "read_only_mode", "untouched overrides stay") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 839fb6c17..883098cca 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1790,10 +1790,12 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // smuggling the still-pending value into memory. hotCfg := pinRestartGated(r.cfg, newCfg) - // An overridden hot field this apply moved away from its serve flag / env + // An overridden field this apply moved away from its serve flag / env // value is superseded for this process: retire the override so an edit - // BACK to that value later persists as the edit it is. - config.RetireSupersededOverrides(hotCfg) + // BACK to that value later persists as the edit it is. Judged on what was + // SAVED (newCfg), not on hotCfg: an edit of listen under --listen ends + // that override even though the listener stays bound to the flag's value. + config.RetireSupersededOverrides(newCfg) // What this process can actually adopt, always computed against the running // config — never against the desired one `result` was diffed from, which can From fa5565e95401b2a7a2a2cafda48e1f7f1a190601 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:14:25 +0300 Subject: [PATCH 12/18] fix(runtime): desired config keeps the hot flags after a reload; retire only edited fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 5 (codex gpt-5.6-sol): after a disk reload the desired config was the raw file, so GET /config showed read_only_mode=false under --read-only and any unrelated PUT round-tripped that value back — retiring the flag and hot-applying the file's value. - ReloadConfiguration's desired config is now pinRestartGated(fileCfg, pinned): the file's restart-gated fields (a pending listen edit stays pending) with the hot flags riding along, exactly like the startup desired config. - ApplyConfig retires an override only when the apply MOVED the field relative to its merge base (RetireEditedOverrides(baseCfg, newCfg)); a round trip of a value the base already held is not an edit. Co-Authored-By: Claude Opus 5 --- internal/config/process_overrides.go | 30 ++++++++++ internal/config/process_overrides_test.go | 25 +++++++++ internal/runtime/lifecycle.go | 9 ++- .../runtime/process_overrides_persist_test.go | 55 +++++++++++++++++++ internal/runtime/runtime.go | 8 ++- 5 files changed, 122 insertions(+), 5 deletions(-) diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index 45e5d27d7..6c0410037 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -198,6 +198,8 @@ type processOverride interface { loadedValue() any // supersededBy reports whether live no longer carries the process value. supersededBy(live *Config) bool + // movedBetween reports whether the field differs between base and next. + movedBetween(base, next *Config) bool // reapply layers the process value back onto a freshly loaded cfg and // returns the entry with its recorded file value refreshed (loaded is // what cfg held before, or fileValue when the caller knows better). @@ -231,6 +233,11 @@ func (o typedOverride[T]) supersededBy(live *Config) bool { return !reflect.DeepEqual(o.field.Get(live), o.process) } +// movedBetween reports whether the field differs between base and next. +func (o typedOverride[T]) movedBetween(base, next *Config) bool { + return !reflect.DeepEqual(o.field.Get(base), o.field.Get(next)) +} + func (o typedOverride[T]) reapply(cfg *Config, fileValue any, useFileValue bool) processOverride { o.loaded = o.field.Get(cfg) if v, ok := fileValue.(T); ok && useFileValue { @@ -424,6 +431,29 @@ func RetireSupersededOverrides(live *Config) { } } +// RetireEditedOverrides forgets every override whose field the caller +// actually edited: next differs from the override AND from base, the config +// the edit was merged onto (the desired config for PUT/PATCH /api/v1/config). +// A field that merely round-tripped a value base already held — the file's +// listen after a disk reload, say — is not an edit, whatever it equals; a +// blanket "differs from the override" test would retire the override on the +// first unrelated save after a reload. +func RetireEditedOverrides(base, next *Config) { + if base == nil || next == nil { + return + } + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for _, o := range effectiveOverridesLocked() { + if !o.supersededBy(next) || !o.movedBetween(base, next) { + continue + } + for _, source := range []OverrideSource{OverrideSourceFlag, OverrideSourceEnv} { + delete(processOverrides, overrideKey{o.name(), source}) + } + } +} + // ResetProcessOverrides forgets every recorded override. For tests. func ResetProcessOverrides() { processOverridesMu.Lock() diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index ceac66f1e..3b4512d11 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -444,3 +444,28 @@ func TestRetireSupersededOverrides_RetiresTheWholeStack(t *testing.T) { require.NoError(t, SaveConfig(cfg, path)) assert.Equal(t, "compact", readJSON(t, path)["direct_tool_response_mode"]) } + +// Round-5 review finding. + +// An apply retires only the overrides its caller actually edited: a field that +// merely round-tripped a value the merge base already held is not an edit. +func TestRetireEditedOverrides_IgnoresRoundTrips(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") + + base := *cfg + base.ReadOnlyMode = false // a merge base that lost the flag (a disk reload) + next := base + next.ToolsLimit = 42 // the only thing the caller changed + RetireEditedOverrides(&base, &next) + assert.ElementsMatch(t, []string{"direct_tool_response_mode", "read_only_mode"}, ProcessOverrideFields(), + "a round-tripped value is not an edit") + + next.DirectToolResponseMode = "full" // a real edit off the flag + RetireEditedOverrides(&base, &next) + assert.Equal(t, []string{"read_only_mode"}, ProcessOverrideFields()) +} diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 3a303452a..6c9c54c2b 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1660,8 +1660,13 @@ func (r *Runtime) ReloadConfiguration() error { // The file IS the desired configuration, so a disk reload resets it — // including over an API change that was still waiting for a restart: // whoever edited the file wins, and nothing may keep merging onto a - // base the file no longer agrees with. - r.desiredCfg = fileCfg + // base the file no longer agrees with. The hot serve flags ride along + // exactly as they do in the startup desired config (the effective + // one): every PUT/PATCH round-trips this document, and a base that + // had lost --read-only would hand the file's value back as an "edit". + // Restart-gated fields stay the file's, so a pending edit of listen + // is still reported as pending. + r.desiredCfg = pinRestartGated(fileCfg, pinned) if newSnapshot.Path != "" { r.cfgPath = newSnapshot.Path } diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index e9ba90c92..b1ee1037b 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -304,3 +304,58 @@ func TestApplyConfig_EditingListenUnderAFlagEndsTheOverride(t *testing.T) { assert.NotContains(t, config.ProcessOverrideFields(), "listen") assert.Contains(t, config.ProcessOverrideFields(), "read_only_mode", "untouched overrides stay") } + +// After a disk reload the desired config must still carry the hot flags, as +// it does at startup, so a GET→PUT round trip of an unrelated edit neither +// retires --read-only nor hot-applies the file's value over it. +func TestApplyConfig_UnrelatedEditAfterReloadKeepsHotFlags(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + assert.True(t, desired.ReadOnlyMode, "GET /config shows the effective hot flag after a reload") + assert.Equal(t, "compact", desired.ToolResponseMode) + desired.ToolsLimit = 99 // the unrelated edit + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + live, err := rt.GetConfig() + require.NoError(t, err) + assert.True(t, live.ReadOnlyMode, "--read-only must survive an unrelated API edit") + assert.Equal(t, "compact", live.ToolResponseMode) + assert.Equal(t, 99, live.ToolsLimit) + assert.Contains(t, config.ProcessOverrideFields(), "read_only_mode") + + m := readConfigJSON(t, cfgPath) + assertNoOverridesOnDisk(t, m) + assert.Equal(t, float64(99), m["tools_limit"]) +} + +// A round trip of the file's listen value after a reload is not an edit of +// listen: the --listen override survives and is still not persisted. +func TestApplyConfig_RoundTripAfterReloadKeepsTheListenOverride(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, "127.0.0.1:8080", desired.Listen, "restart-gated: the desired config is the file") + desired.ToolsLimit = 99 + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + assert.Contains(t, config.ProcessOverrideFields(), "listen") + require.NoError(t, rt.SaveConfiguration()) + assert.Equal(t, "127.0.0.1:8080", readConfigJSON(t, cfgPath)["listen"]) +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 883098cca..dfa03e0b6 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1793,9 +1793,11 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // An overridden field this apply moved away from its serve flag / env // value is superseded for this process: retire the override so an edit // BACK to that value later persists as the edit it is. Judged on what was - // SAVED (newCfg), not on hotCfg: an edit of listen under --listen ends - // that override even though the listener stays bound to the flag's value. - config.RetireSupersededOverrides(newCfg) + // SAVED (newCfg) against the merge base, not on hotCfg: an edit of listen + // under --listen ends that override even though the listener stays bound + // to the flag's value, while a round trip of a value the base already + // held (the file's listen after a disk reload) is not an edit at all. + config.RetireEditedOverrides(baseCfg, newCfg) // What this process can actually adopt, always computed against the running // config — never against the desired one `result` was diffed from, which can From effbe7c91a504da88375595e33695ca8f350f07a Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:20:29 +0300 Subject: [PATCH 13/18] fix(runtime): retire an edited override before the save, on "moved" alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 6 (codex gpt-5.6-sol): after a reload the desired listen is the file's, so an API edit setting it to the flag's own address is a distinguishable edit (base != next == override) — but retirement ran after the save and required the value to differ from the override, so PersistableConfig swapped the edit for the file value and the override stayed. RetireEditedOverrides now retires every field the apply moved relative to its merge base, whatever it moved to, and ApplyConfig calls it before config.SaveConfig. Co-Authored-By: Claude Opus 5 --- internal/config/process_overrides.go | 13 +++++++--- internal/config/process_overrides_test.go | 18 +++++++++++++ .../runtime/process_overrides_persist_test.go | 26 +++++++++++++++++++ internal/runtime/runtime.go | 21 ++++++++------- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index 6c0410037..4cdc7f3e1 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -432,12 +432,19 @@ func RetireSupersededOverrides(live *Config) { } // RetireEditedOverrides forgets every override whose field the caller -// actually edited: next differs from the override AND from base, the config -// the edit was merged onto (the desired config for PUT/PATCH /api/v1/config). +// actually edited: next differs from base, the config the edit was merged +// onto (the desired config for PUT/PATCH /api/v1/config). What it was moved +// TO does not matter — moving listen from the file's address to the flag's +// own address is the operator making that address permanent, and is an edit +// (base != next == override is distinguishable, unlike a plain round trip). // A field that merely round-tripped a value base already held — the file's // listen after a disk reload, say — is not an edit, whatever it equals; a // blanket "differs from the override" test would retire the override on the // first unrelated save after a reload. +// +// Call it BEFORE the save that persists next: once the field is API-managed +// the save writes the edit; called after, PersistableConfig would already +// have swapped an edit equal to the override for the file value. func RetireEditedOverrides(base, next *Config) { if base == nil || next == nil { return @@ -445,7 +452,7 @@ func RetireEditedOverrides(base, next *Config) { processOverridesMu.Lock() defer processOverridesMu.Unlock() for _, o := range effectiveOverridesLocked() { - if !o.supersededBy(next) || !o.movedBetween(base, next) { + if !o.movedBetween(base, next) { continue } for _, source := range []OverrideSource{OverrideSourceFlag, OverrideSourceEnv} { diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index 3b4512d11..b85fb276a 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -469,3 +469,21 @@ func TestRetireEditedOverrides_IgnoresRoundTrips(t *testing.T) { RetireEditedOverrides(&base, &next) assert.Equal(t, []string{"read_only_mode"}, ProcessOverrideFields()) } + +// Round-6 review finding: moving a field from a base value to the override's +// own value is an edit too (base != next == override) and retires it. +func TestRetireEditedOverrides_MovingToTheOverrideValueIsAnEdit(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + cfg.Listen = "127.0.0.1:8080" + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9000") + + base := *cfg + base.Listen = "127.0.0.1:8080" // the desired config after a disk reload + next := base + next.Listen = "127.0.0.1:9000" // the operator makes the flag's address permanent + RetireEditedOverrides(&base, &next) + assert.NotContains(t, ProcessOverrideFields(), "listen") +} diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index b1ee1037b..998c45c44 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -359,3 +359,29 @@ func TestApplyConfig_RoundTripAfterReloadKeepsTheListenOverride(t *testing.T) { require.NoError(t, rt.SaveConfiguration()) assert.Equal(t, "127.0.0.1:8080", readConfigJSON(t, cfgPath)["listen"]) } + +// After a reload the desired listen is the file's; an API edit that sets it +// to the flag's own address is a distinguishable, legitimate edit and must +// reach the file. +func TestApplyConfig_EditingListenToTheFlagValueAfterReloadPersists(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + + edited, err := config.ReadFile(cfgPath) + require.NoError(t, err) + edited.ToolsLimit = 77 + require.NoError(t, config.SaveConfig(edited, cfgPath)) + require.NoError(t, rt.ReloadConfiguration()) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + require.Equal(t, "127.0.0.1:8080", desired.Listen) + desired.Listen = ":0" // make the flag's address permanent + _, err = rt.ApplyConfig(desired, cfgPath) + require.NoError(t, err) + + assert.Equal(t, ":0", readConfigJSON(t, cfgPath)["listen"], "the edit must reach the file") + desired, err = rt.GetDesiredConfig() + require.NoError(t, err) + assert.Equal(t, ":0", desired.Listen) + assert.NotContains(t, config.ProcessOverrideFields(), "listen") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index dfa03e0b6..a7ae5aae1 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1737,6 +1737,18 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // (config_watcher.go). If the save fails, its entry is removed again on // the error path below — nothing reached disk, so a later byte-identical // EXTERNAL write of this config is a genuine edit the watcher must reload. + // An overridden field this apply MOVED (relative to the merge base, so a + // round trip of a value the base already held is not an edit) is + // API-managed for this process from here on: retire the serve flag / env + // override BEFORE the save so the edit — whatever value it moved to, + // including the override's own — is what reaches disk, and an edit back + // later persists as the edit it is. Judged on what is SAVED, not on the + // pinned hot config: an edit of listen under --listen ends that override + // even though the listener stays bound to the flag's value. A failed save + // below leaves the field retired; the apply reports the failure and the + // desired config is unchanged, so nothing has been persisted wrongly. + config.RetireEditedOverrides(baseCfg, newCfg) + r.noteConfigSelfWrite(newCfg, savePath) saveErr := config.SaveConfig(newCfg, savePath) @@ -1790,15 +1802,6 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // smuggling the still-pending value into memory. hotCfg := pinRestartGated(r.cfg, newCfg) - // An overridden field this apply moved away from its serve flag / env - // value is superseded for this process: retire the override so an edit - // BACK to that value later persists as the edit it is. Judged on what was - // SAVED (newCfg) against the merge base, not on hotCfg: an edit of listen - // under --listen ends that override even though the listener stays bound - // to the flag's value, while a round trip of a value the base already - // held (the file's listen after a disk reload) is not an edit at all. - config.RetireEditedOverrides(baseCfg, newCfg) - // What this process can actually adopt, always computed against the running // config — never against the desired one `result` was diffed from, which can // hold a value that was never live. Restart-gated fields are equal by From f348e31a1bd7b659bd7d6b22bf7b8a17915d4877 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:27:11 +0300 Subject: [PATCH 14/18] fix(runtime): put a retired override back when the save fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 7 (codex gpt-5.6-sol): retirement precedes the save, so a transient write failure left the field retired while the live config still carried the override — the next unrelated save would have written it, for MCPPROXY_API_KEY the secret, into the file. RetireEditedOverrides returns what it removed; ApplyConfig restores it on the save error path. Co-Authored-By: Claude Opus 5 --- internal/config/process_overrides.go | 45 +++++++++++++++++-- internal/config/process_overrides_test.go | 23 ++++++++++ .../runtime/process_overrides_persist_test.go | 24 ++++++++++ internal/runtime/runtime.go | 11 +++-- 4 files changed, 95 insertions(+), 8 deletions(-) diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index 4cdc7f3e1..d91e436d7 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -444,10 +444,15 @@ func RetireSupersededOverrides(live *Config) { // // Call it BEFORE the save that persists next: once the field is API-managed // the save writes the edit; called after, PersistableConfig would already -// have swapped an edit equal to the override for the file value. -func RetireEditedOverrides(base, next *Config) { +// have swapped an edit equal to the override for the file value. If that +// save then FAILS, call Restore on the result: nothing reached disk, the +// live and desired configs still carry the override, and leaving it retired +// would let the next unrelated save write it — for MCPPROXY_API_KEY, leak the +// secret — into the file. +func RetireEditedOverrides(base, next *Config) RetiredOverrides { + var retired RetiredOverrides if base == nil || next == nil { - return + return retired } processOverridesMu.Lock() defer processOverridesMu.Unlock() @@ -456,7 +461,39 @@ func RetireEditedOverrides(base, next *Config) { continue } for _, source := range []OverrideSource{OverrideSourceFlag, OverrideSourceEnv} { - delete(processOverrides, overrideKey{o.name(), source}) + key := overrideKey{o.name(), source} + if entry, ok := processOverrides[key]; ok { + retired.entries = append(retired.entries, retiredEntry{key, entry}) + delete(processOverrides, key) + } + } + } + return retired +} + +// RetiredOverrides is what RetireEditedOverrides removed, so a failed save +// can put it back. +type RetiredOverrides struct { + entries []retiredEntry +} + +type retiredEntry struct { + key overrideKey + entry processOverride +} + +// Restore re-registers the retired overrides. An entry re-recorded for the +// same field and source in the meantime (a reload rebuilding the env set) +// is newer and is kept. +func (r RetiredOverrides) Restore() { + if len(r.entries) == 0 { + return + } + processOverridesMu.Lock() + defer processOverridesMu.Unlock() + for _, e := range r.entries { + if _, exists := processOverrides[e.key]; !exists { + processOverrides[e.key] = e.entry } } } diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index b85fb276a..1ccb84d1a 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -487,3 +487,26 @@ func TestRetireEditedOverrides_MovingToTheOverrideValueIsAnEdit(t *testing.T) { RetireEditedOverrides(&base, &next) assert.NotContains(t, ProcessOverrideFields(), "listen") } + +// Round-7 review finding: retirement precedes the save, so a failed save must +// be able to put the overrides back — or a later unrelated save leaks them. +func TestRetireEditedOverrides_RestoreAfterFailedSave(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + + cfg := DefaultConfig() + OverrideForProcess(cfg, FieldAPIKey, OverrideSourceEnv, "env-secret") + OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + + base := *cfg + next := base + next.APIKey = "rotated" + retired := RetireEditedOverrides(&base, &next) + assert.Equal(t, []string{"read_only_mode"}, ProcessOverrideFields()) + + retired.Restore() // the save failed + assert.ElementsMatch(t, []string{"api_key", "read_only_mode"}, ProcessOverrideFields()) + + persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) + assert.NotEqual(t, "env-secret", persisted.APIKey) +} diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index 998c45c44..38a3c4e16 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -385,3 +385,27 @@ func TestApplyConfig_EditingListenToTheFlagValueAfterReloadPersists(t *testing.T assert.Equal(t, ":0", desired.Listen) assert.NotContains(t, config.ProcessOverrideFields(), "listen") } + +// A failed save must not leave an override retired: the env API key would +// otherwise leak into the file on the next unrelated save once disk recovers. +func TestApplyConfig_FailedSaveRestoresTheRetiredOverride(t *testing.T) { + rt, cfgPath := newOverriddenRuntime(t) + dir := filepath.Dir(cfgPath) + + desired, err := rt.GetDesiredConfig() + require.NoError(t, err) + desired.APIKey = "rotated-key" + + require.NoError(t, os.Chmod(dir, 0o500)) // the atomic write cannot create its temp file + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + _, err = rt.ApplyConfig(desired, cfgPath) + require.Error(t, err, "the save must fail") + require.NoError(t, os.Chmod(dir, 0o700)) + + assert.Contains(t, config.ProcessOverrideFields(), "api_key", "the override is back after the failed save") + + require.NoError(t, rt.SaveConfiguration()) // disk recovered; an unrelated save + m := readConfigJSON(t, cfgPath) + assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not leak") + assert.NotEqual(t, "rotated-key", m["api_key"], "the failed edit must not appear either") +} diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index a7ae5aae1..7fb9a0b74 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1737,6 +1737,7 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // (config_watcher.go). If the save fails, its entry is removed again on // the error path below — nothing reached disk, so a later byte-identical // EXTERNAL write of this config is a genuine edit the watcher must reload. + // // An overridden field this apply MOVED (relative to the merge base, so a // round trip of a value the base already held is not an edit) is // API-managed for this process from here on: retire the serve flag / env @@ -1744,10 +1745,11 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // including the override's own — is what reaches disk, and an edit back // later persists as the edit it is. Judged on what is SAVED, not on the // pinned hot config: an edit of listen under --listen ends that override - // even though the listener stays bound to the flag's value. A failed save - // below leaves the field retired; the apply reports the failure and the - // desired config is unchanged, so nothing has been persisted wrongly. - config.RetireEditedOverrides(baseCfg, newCfg) + // even though the listener stays bound to the flag's value. Restored if + // the save below fails: nothing reached disk and the live config still + // carries the override, which the next unrelated save would otherwise + // persist. + retired := config.RetireEditedOverrides(baseCfg, newCfg) r.noteConfigSelfWrite(newCfg, savePath) @@ -1759,6 +1761,7 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // Only this payload is forgotten — markers from other still-pending // successful saves stay live. r.forgetConfigSelfWrite(newCfg, savePath) + retired.Restore() r.logger.Error("Failed to save configuration to disk", zap.String("path", savePath), zap.Error(saveErr)) From 68c111c65db0095cfb9dbe0fdf83fd80e73fe779 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:36:42 +0300 Subject: [PATCH 15/18] =?UTF-8?q?fix(config):=20never=20remove=20an=20over?= =?UTF-8?q?ride=20record=20=E2=80=94=20the=20editing=20save=20ignores=20it?= =?UTF-8?q?=20instead?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 8 (codex gpt-5.6-sol): removing a record on an API edit opened a window in which a concurrent save of the still-live config (telemetry) had no protection and could write MCPPROXY_API_KEY to disk. Retirement is gone. The save that persists an API edit is SaveConfigWithEdits(cfg, mergeBase, path): the overridden fields that MOVED between the merge base (the desired config) and the saved config are the caller's edits and are written as they are, whatever they moved to; every other save keeps restoring the CURRENT file value — which after the edit's save is the edit itself. No mutable state, no window. ApplyConfig uses it and marks its self-write with the same bytes; ReapplyFlagOverrides skips (keeps) a flag the live config superseded. All earlier scenarios (toggle back, cancel a pending listen edit, edit to the flag's own value after a reload, failed save) are re-expressed as tests against the new seam, plus a concurrent stale-save test for the api_key leak. Co-Authored-By: Claude Opus 5 --- internal/config/loader.go | 15 +- internal/config/process_overrides.go | 146 ++++----------- internal/config/process_overrides_test.go | 173 ++++++++++-------- internal/runtime/config_watcher.go | 17 +- internal/runtime/lifecycle.go | 5 - .../runtime/process_overrides_persist_test.go | 11 +- internal/runtime/runtime.go | 28 +-- 7 files changed, 177 insertions(+), 218 deletions(-) diff --git a/internal/config/loader.go b/internal/config/loader.go index 475ab281d..e7dc4b8ad 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -424,7 +424,20 @@ func SaveConfig(cfg *Config, path string) error { // Never persist a process-only override (serve flag, MCPPROXY_* env, env // API key): write the file's value back for every field still carrying // one. See process_overrides.go. - cfg = PersistableConfig(cfg, path) + return writeConfigFile(PersistableConfig(cfg, path), path) +} + +// SaveConfigWithEdits is SaveConfig for the save that persists an API edit: +// the overridden fields that moved between mergeBase (the config the edit was +// merged onto) and cfg are the caller's edits and are written as they are; +// every other overridden field is written back from the file as in +// SaveConfig. See PersistableConfigWithEdits. +func SaveConfigWithEdits(cfg, mergeBase *Config, path string) error { + return writeConfigFile(PersistableConfigWithEdits(cfg, mergeBase, path), path) +} + +// writeConfigFile marshals cfg exactly as given and writes it atomically. +func writeConfigFile(cfg *Config, path string) error { data, err := json.MarshalIndent(cfg, "", " ") if err != nil { return fmt.Errorf("failed to marshal config: %w", err) diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index d91e436d7..936835c87 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -348,7 +348,7 @@ func (b *envOverrideBatch) commit() { // // live is the config this process was running before the reload. A flag the // live config no longer carries was superseded by an API edit (which is on -// disk by now); it is retired rather than resurrected over that edit. +// disk by now) and is not resurrected over that edit. func ReapplyFlagOverrides(cfg, live *Config) { if cfg == nil { return @@ -360,11 +360,10 @@ func ReapplyFlagOverrides(cfg, live *Config) { continue } if live != nil && o.supersededBy(live) { - // Superseded by an API edit: the field is API-managed now, so - // the env record beneath the flag goes too (the loader has - // just re-recorded it; see RetireSupersededOverrides). - delete(processOverrides, key) - delete(processOverrides, overrideKey{key.field, OverrideSourceEnv}) + // Superseded by an API edit that is on disk by now: the file + // value stands. The record is KEPT — a stale config still + // carrying the flag value (telemetry's, an in-flight save's) + // must go on restoring the file value rather than persist it. continue } // Over an env override of the same field the config already carries @@ -399,105 +398,6 @@ func effectiveOverridesLocked() []processOverride { return out } -// RetireSupersededOverrides forgets every override the running config no -// longer carries. An API edit that moved an overridden field to another -// value has superseded the override for this process: from then on the field -// is ordinary, so an edit BACK to the override's value persists as the edit -// it is instead of being swapped for the file value. Call it with the config -// this process actually runs (never with a file-derived one, whose values -// differ from every override by construction). -// -// live is the config the process adopted, or — for a restart-gated field the -// API edited but the process cannot adopt — the config the API saved: an -// operator editing listen under --listen has ended that override for this -// process even though the listener stays bound, and a later edit back to the -// flag's value must persist as asked. -// -// A superseded field forgets its whole stack (flag and env): the field is -// API-managed from now on. -func RetireSupersededOverrides(live *Config) { - if live == nil { - return - } - processOverridesMu.Lock() - defer processOverridesMu.Unlock() - for _, o := range effectiveOverridesLocked() { - if !o.supersededBy(live) { - continue - } - for _, source := range []OverrideSource{OverrideSourceFlag, OverrideSourceEnv} { - delete(processOverrides, overrideKey{o.name(), source}) - } - } -} - -// RetireEditedOverrides forgets every override whose field the caller -// actually edited: next differs from base, the config the edit was merged -// onto (the desired config for PUT/PATCH /api/v1/config). What it was moved -// TO does not matter — moving listen from the file's address to the flag's -// own address is the operator making that address permanent, and is an edit -// (base != next == override is distinguishable, unlike a plain round trip). -// A field that merely round-tripped a value base already held — the file's -// listen after a disk reload, say — is not an edit, whatever it equals; a -// blanket "differs from the override" test would retire the override on the -// first unrelated save after a reload. -// -// Call it BEFORE the save that persists next: once the field is API-managed -// the save writes the edit; called after, PersistableConfig would already -// have swapped an edit equal to the override for the file value. If that -// save then FAILS, call Restore on the result: nothing reached disk, the -// live and desired configs still carry the override, and leaving it retired -// would let the next unrelated save write it — for MCPPROXY_API_KEY, leak the -// secret — into the file. -func RetireEditedOverrides(base, next *Config) RetiredOverrides { - var retired RetiredOverrides - if base == nil || next == nil { - return retired - } - processOverridesMu.Lock() - defer processOverridesMu.Unlock() - for _, o := range effectiveOverridesLocked() { - if !o.movedBetween(base, next) { - continue - } - for _, source := range []OverrideSource{OverrideSourceFlag, OverrideSourceEnv} { - key := overrideKey{o.name(), source} - if entry, ok := processOverrides[key]; ok { - retired.entries = append(retired.entries, retiredEntry{key, entry}) - delete(processOverrides, key) - } - } - } - return retired -} - -// RetiredOverrides is what RetireEditedOverrides removed, so a failed save -// can put it back. -type RetiredOverrides struct { - entries []retiredEntry -} - -type retiredEntry struct { - key overrideKey - entry processOverride -} - -// Restore re-registers the retired overrides. An entry re-recorded for the -// same field and source in the meantime (a reload rebuilding the env set) -// is newer and is kept. -func (r RetiredOverrides) Restore() { - if len(r.entries) == 0 { - return - } - processOverridesMu.Lock() - defer processOverridesMu.Unlock() - for _, e := range r.entries { - if _, exists := processOverrides[e.key]; !exists { - processOverrides[e.key] = e.entry - } - } -} - // ResetProcessOverrides forgets every recorded override. For tests. func ResetProcessOverrides() { processOverridesMu.Lock() @@ -534,11 +434,36 @@ func ProcessOverrideFields() []string { // with effective; the ones it restores are copied first. Callers must not // mutate the shared structures. // -// Known limitation: "edited" is inferred from the value. An explicit API edit -// that sets an overridden field to exactly the override's value is -// indistinguishable from a round trip and is not persisted — unreachable from -// the Web UI, which already shows the override as the current value. +// A save that carries an API edit uses PersistableConfigWithEdits instead, so +// the edited fields are persisted whatever they equal. func PersistableConfig(effective *Config, path string) *Config { + return PersistableConfigWithEdits(effective, nil, path) +} + +// PersistableConfigWithEdits is PersistableConfig for the save that persists +// an API edit: mergeBase is the config the edit was merged onto (the desired +// config for PUT/PATCH /api/v1/config), and every overridden field that +// MOVED between mergeBase and effective is the caller's edit — persisted as +// is, whatever it moved to. Moving listen from the file's address to the +// flag's own address is the operator making that address permanent +// (base != next == override is distinguishable, unlike a plain round trip); +// moving it back to the file's value is an edit too. A field that merely +// round-tripped a value mergeBase already held — the file's listen after a +// disk reload, say — keeps restoring the file value, whatever it equals. +// +// The override records are never removed by an edit. The one save that +// carries the edit ignores them; every other save — a concurrent telemetry +// write of the still-live config, the next server enable — keeps restoring +// the CURRENT file value, which after the edit's save is the edit itself. +// That is what makes an edit of api_key under MCPPROXY_API_KEY safe: there +// is no window in which a stale config carrying the env secret is +// unprotected. +// +// Known limitation: an edit that sets an overridden field to exactly the +// override's value while mergeBase already holds that value is +// indistinguishable from a round trip and is not persisted — unreachable +// from the Web UI, which already shows the override as the current value. +func PersistableConfigWithEdits(effective, mergeBase *Config, path string) *Config { if effective == nil { return nil } @@ -558,6 +483,9 @@ func PersistableConfig(effective *Config, path string) *Config { out := *effective for _, o := range overrides { + if mergeBase != nil && o.movedBetween(mergeBase, effective) { + continue // the caller's edit + } o.restore(&out, base) } return &out diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index 1ccb84d1a..c2cf2f4d9 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -360,34 +360,8 @@ func TestReapplyFlagOverrides_SkipsAFlagTheLiveConfigSuperseded(t *testing.T) { ReapplyFlagOverrides(reloaded, live) assert.True(t, reloaded.ReadOnlyMode, "the superseded flag must not be resurrected") assert.Equal(t, "compact", reloaded.ToolResponseMode, "the untouched flag is re-applied") - assert.Equal(t, []string{"tool_response_mode"}, ProcessOverrideFields(), "the superseded flag is retired") -} - -// Once the running config carries a different value than the override, the -// override is retired: a later edit back to the override's value is then an -// ordinary edit and persists (it used to restore the file value instead). -func TestRetireSupersededOverrides(t *testing.T) { - t.Cleanup(ResetProcessOverrides) - ResetProcessOverrides() - path := writeOverrideTestFile(t, `{"tool_response_mode": "full", "listen": "127.0.0.1:8080", "mcpServers": []}`) - - cfg, err := ReadFile(path) - require.NoError(t, err) - OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceFlag, "compact") - OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") - - // The API sets the hot field to something else; listen stays pinned. - cfg.ToolResponseMode = "full" - require.NoError(t, SaveConfig(cfg, path)) - RetireSupersededOverrides(cfg) - assert.Equal(t, []string{"listen"}, ProcessOverrideFields()) - - // …and back to the flag's value: a real edit now. - cfg.ToolResponseMode = "compact" - require.NoError(t, SaveConfig(cfg, path)) - m := readJSON(t, path) - assert.Equal(t, "compact", m["tool_response_mode"]) - assert.Equal(t, "127.0.0.1:8080", m["listen"], "the pinned flag is still not persisted") + assert.ElementsMatch(t, []string{"read_only_mode", "tool_response_mode"}, ProcessOverrideFields(), + "the record stays: a stale config still carrying the flag value must keep restoring the file") } // Registering the same override twice (loadConfig and runServer both apply @@ -425,88 +399,137 @@ func TestPersistableConfig_StackedEnvAndFlag_EditToTheEnvValuePersists(t *testin assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) } -// Retiring a superseded field forgets the whole stack, not only the winner. -func TestRetireSupersededOverrides_RetiresTheWholeStack(t *testing.T) { +// Round-5 review finding. + +// Edit-aware saves (review rounds 2-8). An override is never "retired": the +// save that persists an API edit ignores the overrides of the fields the +// caller MOVED relative to its merge base, and every other save keeps +// restoring the file value. No mutable state means no window in which a +// concurrent save of the still-live config is unprotected. + +func TestSaveConfigWithEdits_PersistsAnEditBackToTheOverrideValue(t *testing.T) { t.Cleanup(ResetProcessOverrides) ResetProcessOverrides() - path := writeOverrideTestFile(t, `{"direct_tool_response_mode": "full", "mcpServers": []}`) - t.Setenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE", "deferred") + path := writeOverrideTestFile(t, `{"tool_response_mode": "full", "listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg, err := LoadFromFile(path) + cfg, err := ReadFile(path) require.NoError(t, err) - OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") + OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceFlag, "compact") + OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") - cfg.DirectToolResponseMode = "deferred" - RetireSupersededOverrides(cfg) - assert.NotContains(t, ProcessOverrideFields(), "direct_tool_response_mode") + base := *cfg + next := base + next.ToolResponseMode = "full" // away from the flag + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + assert.Equal(t, "full", readJSON(t, path)["tool_response_mode"]) - cfg.DirectToolResponseMode = "compact" // back to the (retired) flag value: an ordinary edit - require.NoError(t, SaveConfig(cfg, path)) - assert.Equal(t, "compact", readJSON(t, path)["direct_tool_response_mode"]) + base = next + next.ToolResponseMode = "compact" // and back to it: still an edit + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + m := readJSON(t, path) + assert.Equal(t, "compact", m["tool_response_mode"]) + assert.Equal(t, "127.0.0.1:8080", m["listen"], "the untouched override is still not persisted") } -// Round-5 review finding. - -// An apply retires only the overrides its caller actually edited: a field that -// merely round-tripped a value the merge base already held is not an edit. -func TestRetireEditedOverrides_IgnoresRoundTrips(t *testing.T) { +func TestSaveConfigWithEdits_RoundTripIsNotAnEdit(t *testing.T) { t.Cleanup(ResetProcessOverrides) ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"read_only_mode": false, "mcpServers": []}`) - cfg := DefaultConfig() + cfg, err := ReadFile(path) + require.NoError(t, err) OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) - OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") base := *cfg - base.ReadOnlyMode = false // a merge base that lost the flag (a disk reload) next := base next.ToolsLimit = 42 // the only thing the caller changed - RetireEditedOverrides(&base, &next) - assert.ElementsMatch(t, []string{"direct_tool_response_mode", "read_only_mode"}, ProcessOverrideFields(), - "a round-tripped value is not an edit") - - next.DirectToolResponseMode = "full" // a real edit off the flag - RetireEditedOverrides(&base, &next) - assert.Equal(t, []string{"read_only_mode"}, ProcessOverrideFields()) + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + m := readJSON(t, path) + assert.NotEqual(t, true, m["read_only_mode"]) + assert.Equal(t, float64(42), m["tools_limit"]) } -// Round-6 review finding: moving a field from a base value to the override's -// own value is an edit too (base != next == override) and retires it. -func TestRetireEditedOverrides_MovingToTheOverrideValueIsAnEdit(t *testing.T) { +// Moving a field from a base value to the override's own value is an edit +// (base != next == override is distinguishable, unlike a round trip). +func TestSaveConfigWithEdits_MovingToTheOverrideValueIsAnEdit(t *testing.T) { t.Cleanup(ResetProcessOverrides) ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg := DefaultConfig() - cfg.Listen = "127.0.0.1:8080" + cfg, err := ReadFile(path) + require.NoError(t, err) OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9000") base := *cfg base.Listen = "127.0.0.1:8080" // the desired config after a disk reload next := base next.Listen = "127.0.0.1:9000" // the operator makes the flag's address permanent - RetireEditedOverrides(&base, &next) - assert.NotContains(t, ProcessOverrideFields(), "listen") + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + assert.Equal(t, "127.0.0.1:9000", readJSON(t, path)["listen"]) } -// Round-7 review finding: retirement precedes the save, so a failed save must -// be able to put the overrides back — or a later unrelated save leaks them. -func TestRetireEditedOverrides_RestoreAfterFailedSave(t *testing.T) { +// With env AND a flag on the same field only the flag is effective; the env +// record must not intercept a plain save of an edit that equals the env value +// once that edit is on disk. +func TestPersistableConfig_StackedEnvAndFlag_RestoresFromTheFile(t *testing.T) { t.Cleanup(ResetProcessOverrides) ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"direct_tool_response_mode": "full", "mcpServers": []}`) + t.Setenv("MCPPROXY_DIRECT_TOOL_RESPONSE_MODE", "deferred") - cfg := DefaultConfig() - OverrideForProcess(cfg, FieldAPIKey, OverrideSourceEnv, "env-secret") - OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) + cfg, err := LoadFromFile(path) + require.NoError(t, err) + OverrideForProcess(cfg, FieldDirectToolResponseMode, OverrideSourceFlag, "compact") base := *cfg next := base - next.APIKey = "rotated" - retired := RetireEditedOverrides(&base, &next) - assert.Equal(t, []string{"read_only_mode"}, ProcessOverrideFields()) + next.DirectToolResponseMode = "deferred" // visibly different from "compact" + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) - retired.Restore() // the save failed - assert.ElementsMatch(t, []string{"api_key", "read_only_mode"}, ProcessOverrideFields()) + // A later plain save of the same effective config keeps what is on disk. + require.NoError(t, SaveConfig(&next, path)) + assert.Equal(t, "deferred", readJSON(t, path)["direct_tool_response_mode"]) +} - persisted := PersistableConfig(cfg, filepath.Join(t.TempDir(), "missing.json")) - assert.NotEqual(t, "env-secret", persisted.APIKey) +// While an API save persists a rotated api_key, a concurrent plain save of +// the still-live config (telemetry, another runtime path) must never write +// the env secret: the override record is never removed, only ignored by the +// one save that edits the field. +func TestSaveConfigWithEdits_ConcurrentStaleSaveNeverLeaksTheEnvKey(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + live, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(live, FieldAPIKey, OverrideSourceEnv, "env-secret") + + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + for { + select { + case <-stop: + return + default: + } + _ = SaveConfig(live, path) // the stale live config, env-secret and all + } + }() + for i := 0; i < 100; i++ { + base := *live + next := base + next.APIKey = "rotated-key" + require.NoError(t, SaveConfigWithEdits(&next, &base, path)) + if got := readJSON(t, path)["api_key"]; got == "env-secret" { + close(stop) + <-done + t.Fatalf("iteration %d: env API key leaked into the file", i) + } + } + close(stop) + <-done + assert.NotEqual(t, "env-secret", readJSON(t, path)["api_key"]) } diff --git a/internal/runtime/config_watcher.go b/internal/runtime/config_watcher.go index 9e090e460..952524432 100644 --- a/internal/runtime/config_watcher.go +++ b/internal/runtime/config_watcher.go @@ -136,7 +136,14 @@ type selfWriteEntry struct { // cfg (config.PersistableConfig), which is what SaveConfig actually writes when // a serve flag or MCPPROXY_* env override is in force. func (r *Runtime) noteConfigSelfWrite(cfg *config.Config, path string) { - data, err := json.MarshalIndent(config.PersistableConfig(cfg, path), "", " ") + r.noteConfigSelfWriteWithEdits(cfg, nil, path) +} + +// noteConfigSelfWriteWithEdits is noteConfigSelfWrite for a save that carries +// an API edit (config.SaveConfigWithEdits): mergeBase is the config the edit +// was merged onto, so the marker matches the bytes that save writes. +func (r *Runtime) noteConfigSelfWriteWithEdits(cfg, mergeBase *config.Config, path string) { + data, err := json.MarshalIndent(config.PersistableConfigWithEdits(cfg, mergeBase, path), "", " ") if err != nil { return } @@ -163,7 +170,13 @@ func (r *Runtime) noteConfigSelfWrite(cfg *config.Config, path string) { // a genuine external edit the watcher must reload. Only the failed payload is // removed — markers pre-armed by other (successful) saves stay live. func (r *Runtime) forgetConfigSelfWrite(cfg *config.Config, path string) { - data, err := json.MarshalIndent(config.PersistableConfig(cfg, path), "", " ") + r.forgetConfigSelfWriteWithEdits(cfg, nil, path) +} + +// forgetConfigSelfWriteWithEdits is forgetConfigSelfWrite's counterpart to +// noteConfigSelfWriteWithEdits. +func (r *Runtime) forgetConfigSelfWriteWithEdits(cfg, mergeBase *config.Config, path string) { + data, err := json.MarshalIndent(config.PersistableConfigWithEdits(cfg, mergeBase, path), "", " ") if err != nil { return } diff --git a/internal/runtime/lifecycle.go b/internal/runtime/lifecycle.go index 6c9c54c2b..7656d0602 100644 --- a/internal/runtime/lifecycle.go +++ b/internal/runtime/lifecycle.go @@ -1414,11 +1414,6 @@ func (r *Runtime) SaveConfiguration() error { return fmt.Errorf("failed to clone configuration") } - // A serve flag / env override the running config no longer carries was - // superseded (UpdateListenAddress, an earlier hot apply); retire it so - // the field is persisted like any other from now on. - config.RetireSupersededOverrides(configCopy) - // Update servers with latest from storage configCopy.Servers = latestServers diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index 38a3c4e16..6fdcee1bf 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -301,8 +301,8 @@ func TestApplyConfig_EditingListenUnderAFlagEndsTheOverride(t *testing.T) { desired, err = rt.GetDesiredConfig() require.NoError(t, err) assert.Equal(t, ":0", desired.Listen) - assert.NotContains(t, config.ProcessOverrideFields(), "listen") - assert.Contains(t, config.ProcessOverrideFields(), "read_only_mode", "untouched overrides stay") + require.NoError(t, rt.SaveConfiguration()) + assert.Equal(t, ":0", readConfigJSON(t, cfgPath)["listen"], "a later unrelated save keeps it") } // After a disk reload the desired config must still carry the hot flags, as @@ -330,7 +330,6 @@ func TestApplyConfig_UnrelatedEditAfterReloadKeepsHotFlags(t *testing.T) { assert.True(t, live.ReadOnlyMode, "--read-only must survive an unrelated API edit") assert.Equal(t, "compact", live.ToolResponseMode) assert.Equal(t, 99, live.ToolsLimit) - assert.Contains(t, config.ProcessOverrideFields(), "read_only_mode") m := readConfigJSON(t, cfgPath) assertNoOverridesOnDisk(t, m) @@ -355,7 +354,6 @@ func TestApplyConfig_RoundTripAfterReloadKeepsTheListenOverride(t *testing.T) { _, err = rt.ApplyConfig(desired, cfgPath) require.NoError(t, err) - assert.Contains(t, config.ProcessOverrideFields(), "listen") require.NoError(t, rt.SaveConfiguration()) assert.Equal(t, "127.0.0.1:8080", readConfigJSON(t, cfgPath)["listen"]) } @@ -383,12 +381,11 @@ func TestApplyConfig_EditingListenToTheFlagValueAfterReloadPersists(t *testing.T desired, err = rt.GetDesiredConfig() require.NoError(t, err) assert.Equal(t, ":0", desired.Listen) - assert.NotContains(t, config.ProcessOverrideFields(), "listen") } // A failed save must not leave an override retired: the env API key would // otherwise leak into the file on the next unrelated save once disk recovers. -func TestApplyConfig_FailedSaveRestoresTheRetiredOverride(t *testing.T) { +func TestApplyConfig_FailedSaveKeepsTheOverrideProtected(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) dir := filepath.Dir(cfgPath) @@ -402,8 +399,6 @@ func TestApplyConfig_FailedSaveRestoresTheRetiredOverride(t *testing.T) { require.Error(t, err, "the save must fail") require.NoError(t, os.Chmod(dir, 0o700)) - assert.Contains(t, config.ProcessOverrideFields(), "api_key", "the override is back after the failed save") - require.NoError(t, rt.SaveConfiguration()) // disk recovered; an unrelated save m := readConfigJSON(t, cfgPath) assert.NotEqual(t, "env-secret", m["api_key"], "MCPPROXY_API_KEY must not leak") diff --git a/internal/runtime/runtime.go b/internal/runtime/runtime.go index 7fb9a0b74..ff2389341 100644 --- a/internal/runtime/runtime.go +++ b/internal/runtime/runtime.go @@ -1738,30 +1738,22 @@ func (r *Runtime) applyConfigLocked(newCfg *config.Config, cfgPath string) (*Con // the error path below — nothing reached disk, so a later byte-identical // EXTERNAL write of this config is a genuine edit the watcher must reload. // - // An overridden field this apply MOVED (relative to the merge base, so a - // round trip of a value the base already held is not an edit) is - // API-managed for this process from here on: retire the serve flag / env - // override BEFORE the save so the edit — whatever value it moved to, - // including the override's own — is what reaches disk, and an edit back - // later persists as the edit it is. Judged on what is SAVED, not on the - // pinned hot config: an edit of listen under --listen ends that override - // even though the listener stays bound to the flag's value. Restored if - // the save below fails: nothing reached disk and the live config still - // carries the override, which the next unrelated save would otherwise - // persist. - retired := config.RetireEditedOverrides(baseCfg, newCfg) - - r.noteConfigSelfWrite(newCfg, savePath) - - saveErr := config.SaveConfig(newCfg, savePath) + // The overridden fields this apply MOVED relative to its merge base are + // the caller's edits and are persisted as they are — whatever they moved + // to, including a serve flag's own value; a round trip of a value the + // base already held keeps restoring the file value. The override records + // stay: a concurrent save of the still-live config must keep restoring + // the file value (config.PersistableConfigWithEdits). + r.noteConfigSelfWriteWithEdits(newCfg, baseCfg, savePath) + + saveErr := config.SaveConfigWithEdits(newCfg, baseCfg, savePath) if saveErr != nil { // Drop the pre-armed self-write entry: the save never landed, so no // future fs event for these bytes can be our own echo. Keeping it // would suppress a genuine external write of byte-identical JSON. // Only this payload is forgotten — markers from other still-pending // successful saves stay live. - r.forgetConfigSelfWrite(newCfg, savePath) - retired.Restore() + r.forgetConfigSelfWriteWithEdits(newCfg, baseCfg, savePath) r.logger.Error("Failed to save configuration to disk", zap.String("path", savePath), zap.Error(saveErr)) From 5ea8eb39a4134de3a0da2fa71ce512d83ff3063d Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:42:55 +0300 Subject: [PATCH 16/18] fix(config): serialise every in-process save's read-base-then-write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 9 (codex gpt-5.6-sol): a save reads the file as its base and writes a whole replacement, so an API edit landing between another save's read and write was reverted (the residual window telemetry.persistConfig documents, widened by the base read). SaveConfigWithEdits now holds one package mutex across the read and the write, so in-process savers — the runtime, telemetry, serve's own saves — always read the file the previous save wrote. A test hook between the two steps pins the schedule. Co-Authored-By: Claude Opus 5 --- internal/config/loader.go | 34 ++++++++++++++++--- internal/config/process_overrides_test.go | 41 +++++++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/internal/config/loader.go b/internal/config/loader.go index e7dc4b8ad..05c52b239 100644 --- a/internal/config/loader.go +++ b/internal/config/loader.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strconv" "strings" + "sync" "time" "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" @@ -421,10 +422,7 @@ func atomicWriteFile(path string, data []byte, perm os.FileMode) error { // SaveConfig saves configuration to file func SaveConfig(cfg *Config, path string) error { - // Never persist a process-only override (serve flag, MCPPROXY_* env, env - // API key): write the file's value back for every field still carrying - // one. See process_overrides.go. - return writeConfigFile(PersistableConfig(cfg, path), path) + return SaveConfigWithEdits(cfg, nil, path) } // SaveConfigWithEdits is SaveConfig for the save that persists an API edit: @@ -432,10 +430,36 @@ func SaveConfig(cfg *Config, path string) error { // merged onto) and cfg are the caller's edits and are written as they are; // every other overridden field is written back from the file as in // SaveConfig. See PersistableConfigWithEdits. +// +// Never persists a process-only override (serve flag, MCPPROXY_* env, env +// API key): the file's value is written back for every field still carrying +// one (see process_overrides.go). That makes every save a read-modify-write +// of the file, so in-process savers — the runtime, telemetry, serve's own +// saves — are serialised: a save always reads the file the previous save +// wrote, and an API edit can never be reverted by a concurrent save that had +// read the file before it landed. (A stale config saved whole still +// overwrites unrelated fields with what it holds — the residual window +// telemetry.persistConfig documents — but a field it merely carries from an +// override is restored from the file, never from that stale copy.) func SaveConfigWithEdits(cfg, mergeBase *Config, path string) error { - return writeConfigFile(PersistableConfigWithEdits(cfg, mergeBase, path), path) + saveConfigMu.Lock() + defer saveConfigMu.Unlock() + persisted := PersistableConfigWithEdits(cfg, mergeBase, path) + if saveConfigTestHook != nil { + saveConfigTestHook() + } + return writeConfigFile(persisted, path) } +// saveConfigMu serialises the read-base-then-write of every in-process save. +// Leaf-level: nothing under it takes another lock except the override +// registry's RWMutex (a leaf itself). +var saveConfigMu sync.Mutex + +// saveConfigTestHook, when set, runs between a save's base read and its write +// (under saveConfigMu). Tests only. +var saveConfigTestHook func() + // writeConfigFile marshals cfg exactly as given and writes it atomically. func writeConfigFile(cfg *Config, path string) error { data, err := json.MarshalIndent(cfg, "", " ") diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index c2cf2f4d9..a61ff709a 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -533,3 +534,43 @@ func TestSaveConfigWithEdits_ConcurrentStaleSaveNeverLeaksTheEnvKey(t *testing.T <-done assert.NotEqual(t, "env-secret", readJSON(t, path)["api_key"]) } + +// Round-9 review finding: a plain save reads the file as its base and writes +// a whole replacement. Without serialisation an API edit that lands between +// that read and that write is reverted. In-process writers (the runtime, +// telemetry, serve's own saves) are serialised through one mutex, so the +// later save always reads the earlier save's file. +func TestSaveConfig_ReadBaseAndWriteAreSerialisedAgainstOtherSaves(t *testing.T) { + t.Cleanup(ResetProcessOverrides) + ResetProcessOverrides() + t.Cleanup(func() { saveConfigTestHook = nil }) + path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) + + live, err := ReadFile(path) + require.NoError(t, err) + OverrideForProcess(live, FieldAPIKey, OverrideSourceEnv, "env-secret") + + var once bool + editDone := make(chan error, 1) + saveConfigTestHook = func() { + if once { + return + } + once = true + // The stale save has read its base ("" for api_key). Now an API edit + // rotates the key concurrently; it must not be able to land between + // this read and the stale save's write. + go func() { + base := *live + next := base + next.APIKey = "rotated-key" + editDone <- SaveConfigWithEdits(&next, &base, path) + }() + time.Sleep(100 * time.Millisecond) + } + + require.NoError(t, SaveConfig(live, path)) // the stale, telemetry-style save + require.NoError(t, <-editDone) + + assert.Equal(t, "rotated-key", readJSON(t, path)["api_key"], "the API edit must not be reverted by the stale save") +} From 0ddac48ac403eb5cf41e42c8024e41039ec85895 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 11:49:28 +0300 Subject: [PATCH 17/18] =?UTF-8?q?fix(config):=20follow=20the=20ReadFile?= =?UTF-8?q?=E2=86=92DecodeConfigFile=20rename=20from=20#1299?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5 --- internal/config/process_overrides.go | 2 +- internal/config/process_overrides_test.go | 18 +++++++++--------- .../runtime/process_overrides_persist_test.go | 18 +++++++++--------- .../process_overrides_persist_test.go | 2 +- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/internal/config/process_overrides.go b/internal/config/process_overrides.go index 936835c87..9c65bbec1 100644 --- a/internal/config/process_overrides.go +++ b/internal/config/process_overrides.go @@ -476,7 +476,7 @@ func PersistableConfigWithEdits(effective, mergeBase *Config, path string) *Conf var base *Config if path != "" { - if onDisk, err := ReadFile(path); err == nil { + if onDisk, err := DecodeConfigFile(path); err == nil { base = onDisk } } diff --git a/internal/config/process_overrides_test.go b/internal/config/process_overrides_test.go index a61ff709a..620a4f316 100644 --- a/internal/config/process_overrides_test.go +++ b/internal/config/process_overrides_test.go @@ -49,7 +49,7 @@ func TestPersistableConfig_RestoresFileValueWhileOverrideStillApplies(t *testing ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") @@ -63,7 +63,7 @@ func TestPersistableConfig_KeepsAnEditOfTheOverriddenField(t *testing.T) { ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") @@ -79,7 +79,7 @@ func TestPersistableConfig_PrefersTheCurrentFileOverTheLoadTimeValue(t *testing. ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") @@ -135,7 +135,7 @@ func TestSaveConfig_DoesNotPersistProcessOverrides(t *testing.T) { ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "read_only_mode": false, "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) @@ -413,7 +413,7 @@ func TestSaveConfigWithEdits_PersistsAnEditBackToTheOverrideValue(t *testing.T) ResetProcessOverrides() path := writeOverrideTestFile(t, `{"tool_response_mode": "full", "listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldToolResponseMode, OverrideSourceFlag, "compact") OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, ":0") @@ -437,7 +437,7 @@ func TestSaveConfigWithEdits_RoundTripIsNotAnEdit(t *testing.T) { ResetProcessOverrides() path := writeOverrideTestFile(t, `{"read_only_mode": false, "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldReadOnlyMode, OverrideSourceFlag, true) @@ -457,7 +457,7 @@ func TestSaveConfigWithEdits_MovingToTheOverrideValueIsAnEdit(t *testing.T) { ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - cfg, err := ReadFile(path) + cfg, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(cfg, FieldListen, OverrideSourceFlag, "127.0.0.1:9000") @@ -502,7 +502,7 @@ func TestSaveConfigWithEdits_ConcurrentStaleSaveNeverLeaksTheEnvKey(t *testing.T ResetProcessOverrides() path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - live, err := ReadFile(path) + live, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(live, FieldAPIKey, OverrideSourceEnv, "env-secret") @@ -546,7 +546,7 @@ func TestSaveConfig_ReadBaseAndWriteAreSerialisedAgainstOtherSaves(t *testing.T) t.Cleanup(func() { saveConfigTestHook = nil }) path := writeOverrideTestFile(t, `{"listen": "127.0.0.1:8080", "mcpServers": []}`) - live, err := ReadFile(path) + live, err := DecodeConfigFile(path) require.NoError(t, err) OverrideForProcess(live, FieldAPIKey, OverrideSourceEnv, "env-secret") diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index 6fdcee1bf..2213bf548 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -31,7 +31,7 @@ func newOverriddenRuntime(t *testing.T) (*Runtime, string) { initial.ToolResponseMode = "full" require.NoError(t, config.SaveConfig(initial, cfgPath)) - cfg, err := config.ReadFile(cfgPath) + cfg, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, ":0") config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, "compact") @@ -148,7 +148,7 @@ func TestConfigWatcher_OwnSaveWithOverridesIsNotAnExternalEdit(t *testing.T) { func TestReloadConfiguration_KeepsFlagOverridesEffective(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) edited.ToolsLimit = 77 // the external edit require.NoError(t, config.SaveConfig(edited, cfgPath)) @@ -180,7 +180,7 @@ func TestReloadConfiguration_DoesNotResurrectAFlagTheAPISuperseded(t *testing.T) _, err = rt.ApplyConfig(desired, cfgPath) require.NoError(t, err) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) require.Equal(t, "full", edited.ToolResponseMode) edited.ToolsLimit = 77 @@ -220,7 +220,7 @@ func TestApplyConfig_TogglingAnOverriddenFieldBackPersists(t *testing.T) { func TestReloadConfiguration_RestartGatedFlagDoesNotHideAPendingFileEdit(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) edited.Listen = "127.0.0.1:9090" require.NoError(t, config.SaveConfig(edited, cfgPath)) @@ -252,7 +252,7 @@ func TestReloadConfiguration_ComponentsFollowTheRunningConfig(t *testing.T) { initial.ToolResponseMode = "full" require.NoError(t, config.SaveConfig(initial, cfgPath)) - cfg, err := config.ReadFile(cfgPath) + cfg, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) config.OverrideForProcess(cfg, config.FieldToolResponseLimit, config.OverrideSourceFlag, 500) config.OverrideForProcess(cfg, config.FieldToolResponseMode, config.OverrideSourceFlag, "compact") @@ -262,7 +262,7 @@ func TestReloadConfiguration_ComponentsFollowTheRunningConfig(t *testing.T) { t.Cleanup(func() { _ = rt.Close() }) require.Equal(t, 500, rt.Truncator().Limit()) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) edited.ToolsLimit = 77 require.NoError(t, config.SaveConfig(edited, cfgPath)) @@ -311,7 +311,7 @@ func TestApplyConfig_EditingListenUnderAFlagEndsTheOverride(t *testing.T) { func TestApplyConfig_UnrelatedEditAfterReloadKeepsHotFlags(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) edited.ToolsLimit = 77 require.NoError(t, config.SaveConfig(edited, cfgPath)) @@ -341,7 +341,7 @@ func TestApplyConfig_UnrelatedEditAfterReloadKeepsHotFlags(t *testing.T) { func TestApplyConfig_RoundTripAfterReloadKeepsTheListenOverride(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) edited.ToolsLimit = 77 require.NoError(t, config.SaveConfig(edited, cfgPath)) @@ -364,7 +364,7 @@ func TestApplyConfig_RoundTripAfterReloadKeepsTheListenOverride(t *testing.T) { func TestApplyConfig_EditingListenToTheFlagValueAfterReloadPersists(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) - edited, err := config.ReadFile(cfgPath) + edited, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) edited.ToolsLimit = 77 require.NoError(t, config.SaveConfig(edited, cfgPath)) diff --git a/internal/telemetry/process_overrides_persist_test.go b/internal/telemetry/process_overrides_persist_test.go index 261dd006a..f19572cae 100644 --- a/internal/telemetry/process_overrides_persist_test.go +++ b/internal/telemetry/process_overrides_persist_test.go @@ -26,7 +26,7 @@ func TestEnsureAnonymousID_DoesNotPersistProcessOverrides(t *testing.T) { raw := `{"listen": "127.0.0.1:8080", "data_dir": ` + jsonQuote(tmp) + `, "read_only_mode": false, "mcpServers": []}` require.NoError(t, os.WriteFile(cfgPath, []byte(raw), 0o600)) - cfg, err := config.ReadFile(cfgPath) + cfg, err := config.DecodeConfigFile(cfgPath) require.NoError(t, err) config.OverrideForProcess(cfg, config.FieldListen, config.OverrideSourceFlag, ":0") config.OverrideForProcess(cfg, config.FieldReadOnlyMode, config.OverrideSourceFlag, true) From 577dda5ca3edb6f87481990d66a89899cb207f81 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Fri, 18 Sep 2026 16:16:01 +0300 Subject: [PATCH 18/18] fix(test): make TestApplyConfig_FailedSaveKeepsTheOverrideProtected Windows-safe Follow-up to cb35775e9 (the main merge-conflict resolution for this PR). The test simulated a failed atomic config save by os.Chmod'ing the config directory to 0o500. That works on POSIX but Windows does not enforce Unix-style directory permission bits via os.Chmod the same way, so the GitHub Actions windows-latest runner could still write into the "read-only" directory and the save silently succeeded, failing the test with "An error is expected but got nil". Replace it with a mechanism that fails identically on every OS: point the save at a path whose parent is a plain file instead of a directory. writeConfigFile's os.MkdirAll(dir, 0700) pre-flight does a pure-Go `Stat(dir); if err == nil && !IsDir() { return ENOTDIR }` check before any OS-specific mkdir syscall, so this induces the same real write failure on Linux, macOS and Windows. The broken path is scoped to this one ApplyConfig call only (the runtime's own cfgPath stays a real writable directory), so the later SaveConfiguration() still exercises "disk recovered" for real. Co-Authored-By: Claude Sonnet 5 --- .../runtime/process_overrides_persist_test.go | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/internal/runtime/process_overrides_persist_test.go b/internal/runtime/process_overrides_persist_test.go index 2213bf548..af81dc337 100644 --- a/internal/runtime/process_overrides_persist_test.go +++ b/internal/runtime/process_overrides_persist_test.go @@ -387,17 +387,31 @@ func TestApplyConfig_EditingListenToTheFlagValueAfterReloadPersists(t *testing.T // otherwise leak into the file on the next unrelated save once disk recovers. func TestApplyConfig_FailedSaveKeepsTheOverrideProtected(t *testing.T) { rt, cfgPath := newOverriddenRuntime(t) - dir := filepath.Dir(cfgPath) + tmp := filepath.Dir(cfgPath) desired, err := rt.GetDesiredConfig() require.NoError(t, err) desired.APIKey = "rotated-key" - require.NoError(t, os.Chmod(dir, 0o500)) // the atomic write cannot create its temp file - t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) - _, err = rt.ApplyConfig(desired, cfgPath) + // Induce a real, cross-platform write failure by pointing this ONE save + // at a path whose parent is a plain file instead of a directory, rather + // than chmod'ing an existing directory read-only: Windows does not enforce + // Unix-style directory permission bits via os.Chmod the way POSIX does, so + // the GitHub Actions windows-latest runner can still write into a + // "read-only" directory and the save silently succeeds. writeConfigFile's + // os.MkdirAll(dir, 0700) pre-flight does `Stat(dir); if err == nil && + // !IsDir() { return ENOTDIR }` — pure Go logic that runs identically on + // every OS, so a file sitting where the save's directory should be fails + // the same way on Linux, macOS and Windows. This targets only this one + // ApplyConfig call: cfgPath (the runtime's own saved-to path, still a real + // writable directory) is untouched, so the later SaveConfiguration below + // exercises "disk recovered" for real. + blockedDir := filepath.Join(tmp, "blocked-save-dir") + require.NoError(t, os.WriteFile(blockedDir, []byte("not a directory"), 0o644)) + brokenPath := filepath.Join(blockedDir, "mcp_config.json") + + _, err = rt.ApplyConfig(desired, brokenPath) require.Error(t, err, "the save must fail") - require.NoError(t, os.Chmod(dir, 0o700)) require.NoError(t, rt.SaveConfiguration()) // disk recovered; an unrelated save m := readConfigJSON(t, cfgPath)