fix(serve): stop persisting CLI flag and env overrides into the config file - #1299
Merged
Merged
Conversation
`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 <noreply@anthropic.com>
…PI 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <data_dir>/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 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
384e84b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://692fbe47.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-confident-panini-c552.mcpproxy-docs.pages.dev |
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 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35324363616 --repo smart-mcp-proxy/mcpproxy-go
|
Dumbris
added a commit
that referenced
this pull request
Sep 18, 2026
…gnature CI on PR #1300 failed to build cmd/mcpproxy on every platform: "assignment mismatch: 2 variables but loadConfig returns 3 values". Unrelated to this PR's instance-ID changes -- main itself is currently broken this way (#1299 changed loadConfig to also return a *serveConfigSaver, and the listen_flag_test.go added by #1301 wasn't updated for it). Discard the unused saver return to match the real signature. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mcpproxy servepersisted its config at three sites (auto-generatedapi_key, first-run telemetry notice,recordStartupOutcome) by marshalling the in-memory config — which already carried every CLI flag override. Soserve --listen :0(or--listen "", now mapped to:0) wrote"listen": ":0"intomcp_config.json, and the next unflaggedserveor tray-launched core silently booted in stdio mode. Verified empirically 2026-09-17 with a fresh{"mcpServers":[]}config.What changed
serveConfigSaver(cmd/mcpproxy/main.go):loadConfignow returns(cfg, saver, err).saver.savereads the config file as its base (config.ReadFile, snapshot of the same at startup as fallback) and overlays only whatserveowns: the API key it generated itself (into an emptyapi_keyonly) andcfg.Telemetry. All three save sites use it;recordStartupOutcometakes the save func as a parameter.config.ReadFile(internal/config/loader.go):DefaultConfig()+loadConfigFile, nothing else.LoadFromFileis not a read — it appliesMCPPROXY_*env overrides, copiesMCPPROXY_API_KEYintoapi_keyviaValidate, createsdata_dir, and replaces the process-global registry list — none of which a save may do.config.LoadWithPath:Load()discarded the file it discovered (./mcp_config.jsonor the home file) andrunServerre-derived<data_dir>/mcp_config.json, a possibly different, never-loaded file. The saver andNewServerWithConfigPathnow use the file that was actually loaded.Load()keeps its signature.Because the base is the file rather than the live config, this also stops
--read-only,--log-level,--disable-management,--data-dir, andMCPPROXY_*env values from leaking through these three sites, and a late fatal-serve save no longer resurrects a startup-era server list over changes the runtime persisted since.The
--listen ""→:0mapping is untouched.Not fixed here (pre-existing, same class)
Two other persist paths still marshal the live config with overrides: the telemetry service's anonymous-id persist (
internal/telemetry/telemetry.gopersistConfigLocked, semver builds only) andRuntime.SaveConfiguration(every API-driven config change). A config-level strip would break the UI's legitimate restart-gatedlistenedits (internal/runtime/restart_gated.go), so that needs a persisted-vs-effective split — follow-up. Also pre-existing:recordStartupOutcome/MaybePrintFirstRunNoticemutatecfg.Telemetrywithout the telemetry service's mutex.Tests
cmd/mcpproxy/serve_flag_persistence_test.go drives
loadConfigwith a cobra command bound to the package globals, then the save sites, and decodes the file as raw JSON:loadConfigflags (incl.--listen :0) +runServer's--read-only/--log-levelmutations do not reach the file; generatedapi_keyand telemetry doMCPPROXY_API_KEYandMCPPROXY_LISTENare never writtenteamsblock survives (loader normalization)--config, saves land in the discovered./mcp_config.json, not<data_dir>/mcp_config.jsonEvery test was watched failing against the old behaviour before its fix.
Verification
go test ./cmd/mcpproxy ./internal/config✅.github/.golangci.yml: 0 issues ✅./scripts/test-api-e2e.sh: 65/65 (one earlier run had 2 unidentified failures, then 5 consecutive clean runs — timing flake)opencode run --model github-copilot/gpt-5.6-sol: 5 rounds; rounds 1–4 each surfaced a genuine issue (late-save clobber, env API key,LoadFromFileside effects, legacyteamserasure, discovery-path mismatch), all fixed with a test; round 5 VERDICT: CLEAN.🤖 Generated with Claude Code