Merge pull request #352 from andrewyng/rpMcpGlobalWins

mcp: global config wins on name clash with a trusted workspace

coworker/mcp/config.py

@@ -94,17 +94,18 @@ def load_mcp_servers(
 ) -> list[MCPServerDef]:
     """Merge global + (when trusted) workspace `mcpServers` into parsed server defs.
 
-    Workspace entries win on name clash, but only after ``workspace_trusted`` — the
-    same consent boundary used for repository ``allowed_commands``. Untrusted
-    workspaces contribute nothing, so a cloned repo cannot shadow a global server
-    or spawn stdio processes via ``.coworker/mcp.json``.
+    Only trusted workspaces contribute — the same consent boundary as repository
+    ``allowed_commands`` — and **global wins on name clash**, so even a trusted repo
+    cannot silently redefine a global server by reusing its name. ``${VAR}`` refs in
+    a workspace def are resolved from the user's env, which is acceptable only because
+    the workspace is trusted; untrusted workspaces are never read.
     """
     secrets = secrets or SecretStore()
     merged: dict[str, dict[str, Any]] = {}
     for path in _config_paths(workspace, workspace_trusted=workspace_trusted):
         for name, raw in (_read(path).get("mcpServers") or {}).items():
             if isinstance(raw, dict):
-                merged[name] = raw
+                merged.setdefault(name, raw)  # global first → global wins on clash
     return [_parse(name, raw, secrets) for name, raw in merged.items()]
 
 

tests/test_mcp.py

@@ -55,10 +55,8 @@ def test_load_merges_global_and_workspace(tmp_path, monkeypatch):
         ws / ".coworker" / "mcp.json",
         {
             "mcpServers": {
-                "fs": {
-                    "command": "echo",
-                    "args": ["workspace-wins"],
-                },  # overrides global
+                "fs": {"command": "echo", "args": ["workspace-loses"]},  # clashes: global wins
+                "ws_only": {"command": "echo", "args": ["ws"], "enabled": True},
             }
         },
     )
@@ -67,7 +65,9 @@ def test_load_merges_global_and_workspace(tmp_path, monkeypatch):
         s.name: s
         for s in load_mcp_servers(ws, secrets=SecretStore(), workspace_trusted=True)
     }
-    assert servers["fs"].args == ["workspace-wins"]
+    # Global wins on name clash; a non-clashing trusted workspace server still loads.
+    assert servers["fs"].args == ["global"]
+    assert servers["ws_only"].args == ["ws"]
     assert servers["fs"].transport == "stdio"
     assert servers["docs"].transport == "http" and servers["docs"].enabled is False
     assert servers["docs"].requires_approval is True  # default
@@ -108,11 +108,13 @@ def test_untrusted_workspace_mcp_ignored(tmp_path, monkeypatch):
         assert set(servers) == {"fs"}
         assert servers["fs"].args == ["global"]
 
+    # Trusted: the evil stdio server loads, but the clashing `fs` name still resolves
+    # to the global def — a trusted repo cannot silently redefine a global server.
     trusted = {
         s.name: s
         for s in load_mcp_servers(ws, secrets=SecretStore(), workspace_trusted=True)
     }
-    assert trusted["fs"].args == ["pwned"]
+    assert trusted["fs"].args == ["global"]
     assert "evil" in trusted