fix(config): persisted-vs-effective split — never persist serve flags, MCPPROXY_* env or the env API key - #1302
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>
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>
…-only overrides 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 <noreply@anthropic.com>
…gistry - 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 <noreply@anthropic.com>
…ng config only 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 <noreply@anthropic.com>
…raw file 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 <noreply@anthropic.com>
… saved
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 <noreply@anthropic.com>
…re only edited fields 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 <noreply@anthropic.com>
…lone 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…es it instead 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
577dda5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5a467df5.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://claude-jovial-sutherland-f77.mcpproxy-docs.pages.dev |
Dumbris
changed the base branch from
claude/confident-panini-c552d5
to
main
September 18, 2026 12:42
# Conflicts: # cmd/mcpproxy/main.go # cmd/mcpproxy/serve_flag_persistence_test.go # internal/runtime/lifecycle.go
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…indows-safe Follow-up to cb35775 (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 <noreply@anthropic.com>
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35349239459 --repo smart-mcp-proxy/mcpproxy-go
|
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.
Stacked on #1299 (base branch
claude/confident-panini-c552d5; retarget tomainonce that merges).Problem
#1299 fixed
serve's own three saves. Two other persist paths still marshalled the live config, which carries every process-only override —serveflags (--listen,--read-only,--tool-response-mode, …), theMCPPROXY_*env overrides the loader applies, and theMCPPROXY_API_KEYthatValidate/EnsureAPIKeyfold intoapi_key:telemetry.persistConfigLocked(first-runanonymous_id, semver builds only)Runtime.SaveConfiguration(every API-driven server add/enable/quarantine decision) andApplyConfigSo
serve --listen :0(orMCPPROXY_LISTEN, or the env API key) landed inmcp_config.jsonthe first time a server was enabled from the Web UI.Design: persisted vs effective
A process-wide override registry in
internal/config/process_overrides.go:OverrideForProcess(cfg, Field, source, value)sets the effective value and records(field, source, process value, file value at load). The loader routes everyMCPPROXY_*override through it (rebuilt atomically per load),Validate/EnsureAPIKeythe env API key, andcmd/mcpproxyevery serve flag (loadConfig,applyServeLoggingFlags,applyServeRuntimeFlags). A flag shadows an env record of the same field.PersistableConfig(effective, path)restores the current file value for every field whose effective value still equals its override.config.SaveConfigapplies it centrally, so runtime, telemetry, the server-edition admin handlers and the load-modify-save CLI subcommands are all covered.SaveConfigWithEdits(cfg, mergeBase, path)is the save that persists an API edit: overridden fields that moved relative to the merge base (the desired config) are the caller's edits and are written as-is — whatever they moved to, including the flag's own value — so the UI's restart-gatedlistenedits, the tray's port-conflictSetListenAddress, toggling a field away from and back to its flag value, and "cancel the pending change" all still persist. Records are never removed, so a concurrent save of the still-live config keeps restoring the file value (no window in whichMCPPROXY_API_KEYis unprotected).ReloadConfigurationre-applies the serve flags onto the pinned running config (the loader only re-applies env), skips a flag the API had superseded, republishes when it differs from the file, feeds the running config to the component side effects (parity withApplyConfig), and keeps the desired config = file's restart-gated fields + hot flags so GET→PUT round trips after a reload neither leak nor flip a flag.Known limitation (documented on
PersistableConfigWithEdits): an edit that sets an overridden field to exactly the override's value while the merge base already holds it is indistinguishable from a round trip — unreachable from the Web UI, which already shows that value.Tests
Failing-first tests for each path:
internal/telemetry/process_overrides_persist_test.go,internal/runtime/process_overrides_persist_test.go(SaveConfiguration, ApplyConfig round trip / edit / toggle-back / cancel / failed save, reload × flags, watcher, component parity),internal/config/process_overrides_test.go(registry, nested copy-on-write, env batch atomicity, stacked env+flag, concurrent stale save, read/write serialisation),cmd/mcpproxy/serve_flag_persistence_test.go(every serve flag registered; API edit of a flagged field persists).go test -race ./internal/runtime/... ./internal/telemetry/... ./internal/config/... ./cmd/mcpproxy✅./scripts/test-api-e2e.sh65/65 ✅ (run as a scratch copy with the blanketpkill -f "mcpproxy.*serve"on line 80 removed,LISTEN_PORT=18471, so the live tray core survived).github/.golangci.yml: 0 issues ✅Cross-model review
10 fix→re-review rounds per the CLAUDE.md cap. Rounds 1–2 via
opencodegithub-copilot/gpt-5.6-sol --variant high; from round 3 every Copilot model via opencode reported "exceeded your monthly quota", so rounds 3–10 ran throughcodex exec -m gpt-5.6-sol(the documented fallback). Round 10 verdict: CLEAN. Each round's finding is recorded in the corresponding commit message; one finding was rejected ("env value changes between loads" — a process's environment is fixed).Follow-ups (not in this PR)
cmd/mcpproxyrecordStartupOutcomeandtelemetry.MaybePrintFirstRunNoticemutatecfg.Telemetrywithout the telemetry service's mutex while its goroutine reads it. Needs a locked mutator ontelemetry.Serviceplumbed throughserver→main.telemetry.persistConfigfor unrelated fields remains, though an overridden field is now always restored from the file.🤖 Generated with Claude Code