fix(config): skip merging fallback servers when managed servers exist - #10
fix(config): skip merging fallback servers when managed servers exist#10lydakis wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 241f6d36f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(cfg.Servers) > 0 { | ||
| return nil |
There was a problem hiding this comment.
When a config defines both at least one managed servers.* entry and an explicit fallback_sources list, this early return prevents MergeFallbackServersForCWD from reading those user-specified sources at all. That regresses partial-adoption setups such as “keep github in mcpx config, but import everything else from /tmp/legacy.json”: the old collision logic already ensured managed entries won per-name, so skipping the entire merge drops unrelated discovered servers unnecessarily.
Useful? React with 👍 / 👎.
…shadowing (#36) * fix: stop CWD config discovery, listing probes, and fallback command shadowing Rewrite the still-valid security/correctness fixes from stale PRs #12, #11, and #10 onto current main. Bare mcpx stays config-only, project-local .mcp.json is no longer auto-imported, and fallback merge is skipped when managed servers already exist. * fix: align fallback watch with managed config and keep named virtual servers Skip merge and source watching only when config.toml has MCPXConfig origins, not whenever cfg.Servers is non-empty. Filter reserved CLI names from fallback discovery, and resolve explicit --help / shim install targets via list_tools so Codex virtual servers still work.
|
Superseded by #36 (landed on main). Closing this stale March branch. |
1 participant
Motivation
.mcp.json) from injecting server names that can shadow internal CLI commands and cause unintended tool execution when the user already has managed servers configured.Description
MergeFallbackServersForCWDto skip loading/merging fallback sources whencfg.Serversis non-empty (internal/config/fallback.go).TestMergeFallbackServersKeepsManagedOnlyWhenConfiguredininternal/config/fallback_test.go).FallbackSourcesoverrides so existing onboarding and configured-source semantics remain unchanged.Testing
go test ./internal/config -run MergeFallback -count=1, which passed.go vet ./...andgo build ./..., both of which passed.go test ./...which fails in this environment due to pre-existinginternal/configCodex-related tests (TestLoadCodexConfigFileAddsCodexAppsServerFromAuthFileandTestLoadCodexConfigFileCodexAppsUsesConnectorsTokenEnv) that are unrelated to the fallback merge change.Codex Task