Skip to content

Commit 20faf82

Browse files
Vasanthdev2004Vasanthdev2004
committed
fix(mcp): check name uniqueness on the whole config, before the startup split
Startup separates unconfigured built-in defaults from the servers the user asked for, and the two halves are normalized by separate calls. A collision that straddles the split is invisible to both: a user-configured " exa" is critical while the built-in "exa" is optional, and they are one runtime server with two panel rows sharing a failure, a tool count, and each other's redaction context. The check moves out of NormalizeConfig into ValidateUniqueNames, which NormalizeConfig still calls, and startup runs it on the merged configuration before splitting.
1 parent fc640c9 commit 20faf82

3 files changed

Lines changed: 114 additions & 24 deletions

File tree

‎internal/cli/app.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,15 @@ func runInteractiveTUIWithSetup(stderr io.Writer, deps appDeps, permissionMode a
839839
mcpTokenStore = nil
840840
err = nil
841841
}
842+
// Checked on the WHOLE configuration, before the split. The two halves
843+
// normalize separately, so a collision that straddles them is invisible to
844+
// either call: a user-configured " exa" is critical while the built-in
845+
// default "exa" is optional, and they are one runtime server. Ambiguous
846+
// identity is worth refusing to start over, because every downstream join is
847+
// keyed on that name and one of the two entries is unreachable regardless.
848+
if err := mcp.ValidateUniqueNames(mcpConfig); err != nil {
849+
return writeAppError(stderr, err.Error(), 1)
850+
}
842851
criticalMCPConfig, optionalMCPConfig := splitMCPStartupConfig(mcpConfig)
843852
mcpRuntime := mcpToolRuntime(noopMCPRuntime{})
844853
if len(criticalMCPConfig.Servers) > 0 {

‎internal/cli/mcp_server_identity_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package cli
22

33
import (
4+
"sort"
45
"strings"
56
"testing"
67

78
"github.com/Gitlawb/zero/internal/config"
9+
"github.com/Gitlawb/zero/internal/mcp"
810
)
911

1012
// ONE CONFIG KEY PER RUNTIME IDENTITY, ENFORCED WHERE THE KEY IS WRITTEN.
@@ -68,3 +70,54 @@ func TestUpsertAllowsADistinctName(t *testing.T) {
6870
t.Errorf("servers = %#v, want both", cfg.file.MCP.Servers)
6971
}
7072
}
73+
74+
// THE UNIQUENESS CHECK HAS TO RUN ON THE WHOLE CONFIGURATION, BEFORE THE SPLIT.
75+
//
76+
// Startup separates unconfigured built-in defaults from the servers the user
77+
// asked for, so the two halves are normalized by separate calls. A collision
78+
// that straddles the split is invisible to both of them: a user-configured
79+
// " exa" is critical, the built-in "exa" is optional, and they are one runtime
80+
// server with two panel rows sharing a failure and a tool count.
81+
func TestACollisionAcrossTheStartupSplitIsStillRefused(t *testing.T) {
82+
defaults := config.DefaultMCPServers()
83+
names := make([]string, 0, len(defaults))
84+
for name := range defaults {
85+
names = append(names, name)
86+
}
87+
sort.Strings(names)
88+
if len(names) == 0 {
89+
t.Skip("no built-in MCP defaults to collide with")
90+
}
91+
name := names[0]
92+
93+
// The user's own entry under a padded key: same runtime identity, different
94+
// configuration, so it is not the untouched default.
95+
mine := defaults[name]
96+
mine.Headers = map[string]string{"X-Api-Key": "opaque-workspace-9f3c2b7ae1d8"}
97+
98+
cfg := config.MCPConfig{Servers: map[string]config.MCPServerConfig{
99+
name: defaults[name],
100+
" " + name: mine,
101+
}}
102+
103+
critical, optional := splitMCPStartupConfig(cfg)
104+
if len(critical.Servers) != 1 || len(optional.Servers) != 1 {
105+
t.Fatalf("the two entries did not straddle the split: critical=%v optional=%v", critical.Servers, optional.Servers)
106+
}
107+
// Each half on its own sees one server and no collision, which is why the
108+
// per-half check cannot be the guard.
109+
if err := mcp.ValidateUniqueNames(critical); err != nil {
110+
t.Fatalf("the critical half alone reported a collision: %v", err)
111+
}
112+
if err := mcp.ValidateUniqueNames(optional); err != nil {
113+
t.Fatalf("the optional half alone reported a collision: %v", err)
114+
}
115+
116+
err := mcp.ValidateUniqueNames(cfg)
117+
if err == nil {
118+
t.Fatal("a collision straddling the startup split was accepted")
119+
}
120+
if !strings.Contains(err.Error(), name) || !strings.Contains(err.Error(), "rename") {
121+
t.Errorf("the refusal is not actionable: %v", err)
122+
}
123+
}

‎internal/mcp/config.go‎

Lines changed: 52 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -48,25 +48,11 @@ func NormalizeConfig(cfg config.MCPConfig) ([]Server, error) {
4848
}
4949
sort.Strings(names)
5050

51+
if err := ValidateUniqueNames(cfg); err != nil {
52+
return nil, err
53+
}
54+
5155
servers := make([]Server, 0, len(names))
52-
// THE NORMALIZED NAME IS AN IDENTITY, so it has to be unique.
53-
//
54-
// Trimming means "docs" and " docs " are two config keys and one runtime
55-
// server, and everything downstream keys on the runtime name: tool
56-
// accounting, the skipped-server observations the panel renders, and
57-
// invalidation. Two rows then share one failure, both report the same state,
58-
// and Go map iteration decides which configuration survives.
59-
//
60-
// It is also a confidentiality problem rather than only a wrong status. Each
61-
// row redacts that shared error with ITS OWN configuration, so if the server
62-
// that actually failed echoed a credential, the other row does not have that
63-
// value among its candidates and prints it.
64-
//
65-
// Rejecting is the honest answer: one of the two entries is unreachable
66-
// whatever we do, and an error naming both spellings is something an
67-
// operator can act on. A single padded name still works, because trimming is
68-
// not the problem; two names trimming to one is.
69-
claimed := make(map[string]string, len(names))
7056
for _, name := range names {
7157
raw := cfg.Servers[name]
7258
if raw.Disabled {
@@ -76,17 +62,59 @@ func NormalizeConfig(cfg config.MCPConfig) ([]Server, error) {
7662
if err != nil {
7763
return nil, err
7864
}
79-
if previous, taken := claimed[server.Name]; taken {
80-
return nil, fmt.Errorf(
81-
"mcp: server names %q and %q both resolve to %q; rename one so each server has its own identity",
82-
previous, name, server.Name)
83-
}
84-
claimed[server.Name] = name
8565
servers = append(servers, server)
8666
}
8767
return servers, nil
8868
}
8969

70+
// ValidateUniqueNames reports two enabled configuration keys that resolve to
71+
// one runtime identity.
72+
//
73+
// THE NORMALIZED NAME IS AN IDENTITY, so it has to be unique. Trimming means
74+
// "docs" and " docs " are two configuration keys and one runtime server, and
75+
// everything downstream keys on the runtime name: tool accounting, the
76+
// skipped-server observations the panel renders, and invalidation. Two rows then
77+
// share one failure, both report the same state, and Go map iteration decides
78+
// which configuration survives.
79+
//
80+
// It is also a confidentiality problem rather than only a wrong status. Each row
81+
// redacts that shared error with ITS OWN configuration, so if the server that
82+
// actually failed echoed a credential, the other row does not have that value
83+
// among its candidates and prints it.
84+
//
85+
// Rejecting is the honest answer: one of the two entries is unreachable whatever
86+
// we do, and an error naming both spellings is something an operator can act on.
87+
// A single padded name still works, because trimming is not the problem; two
88+
// names trimming to one is. A disabled entry claims nothing, matching the rest
89+
// of startup, which skips it before it reaches any of these paths.
90+
//
91+
// Exported because startup splits the configuration before registering it, so
92+
// the two halves normalize separately and a collision that straddles them is
93+
// invisible to either call. A user-configured " exa" alongside the built-in
94+
// default "exa" is exactly that shape.
95+
func ValidateUniqueNames(cfg config.MCPConfig) error {
96+
names := make([]string, 0, len(cfg.Servers))
97+
for name := range cfg.Servers {
98+
names = append(names, name)
99+
}
100+
sort.Strings(names)
101+
102+
claimed := make(map[string]string, len(names))
103+
for _, name := range names {
104+
if cfg.Servers[name].Disabled {
105+
continue
106+
}
107+
canonical := strings.TrimSpace(name)
108+
if previous, taken := claimed[canonical]; taken {
109+
return fmt.Errorf(
110+
"mcp: server names %q and %q both resolve to %q; rename one so each server has its own identity",
111+
previous, name, canonical)
112+
}
113+
claimed[canonical] = name
114+
}
115+
return nil
116+
}
117+
90118
func normalizeServer(name string, raw config.MCPServerConfig) (Server, error) {
91119
name = strings.TrimSpace(name)
92120
if err := ValidateServerName(name); err != nil {

0 commit comments

Comments
 (0)