From b11a88ded9e326377c4bd7dedd8b77a82fea32cb Mon Sep 17 00:00:00 2001 From: Achille Negrier Date: Fri, 16 Oct 2026 13:47:08 +0000 Subject: [PATCH 1/6] Name MCP service client entries by leaf (exa) instead of dashed full name Register UC MCP Services in coding-agent configs under the service leaf name (`exa` for `main.default.exa`). A leaf that is shared by two services being configured, or already used by a different server (other ug entries, managed servers), falls back to the dashed full name (`main-default-exa`). URLs are unchanged. Existing dashed entries are recognised by URL and renamed to the leaf in place on the next run (old entry removed from the agent, no duplicate), merging clients when the legacy and new entry coexist. Co-authored-by: Isaac --- src/ucode/mcp.py | 185 +++++++++++++++++++++++++++++++++++++++++----- tests/test_mcp.py | 158 ++++++++++++++++++++++++++++++++------- 2 files changed, 295 insertions(+), 48 deletions(-) diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 079871830..6acdeda8b 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -7,7 +7,8 @@ import shutil import subprocess import threading -from collections.abc import Callable +from collections import Counter +from collections.abc import Callable, Iterable from concurrent.futures import ThreadPoolExecutor, as_completed from typing import Any from urllib.parse import urlparse @@ -575,6 +576,105 @@ def _servers_by_name(mcp_servers: list[dict]) -> dict[str, dict]: return servers +def _legacy_mcp_service_entry_name(full_name: str) -> str: + """Dashed entry name (`main-default-exa`): agent CLIs reject dots in registered names.""" + return full_name.replace(".", "-") + + +def mcp_service_entry_names( + full_names: Iterable[str], + workspace: str, + *, + existing_servers: Iterable[dict] = (), + managed_names: Iterable[str] = (), +) -> dict[str, str]: + """Map each MCP service full name (`..`) to its agent entry name. + + The entry name is the leaf (`exa` for `main.default.exa`). A service falls back to the dashed + full name (`main-default-exa`) when its leaf is not unique: two services in ``full_names`` share + it, or an entry in ``existing_servers`` (other ug entries, managed servers) has that name but + points at a different URL. An existing entry with the same URL is the same service, not a + clash. A service whose dashed name is in ``managed_names`` keeps it: managed config written by + an older ug still holds it under that name, and the "already managed" skip matches on names. + URLs are unaffected.""" + unique = sorted(set(full_names)) + leaf_counts = Counter(full_name.rsplit(".", 1)[-1] for full_name in unique) + url_by_name = { + name: server.get("url") for server in existing_servers if (name := _server_name(server)) + } + legacy_managed = set(managed_names) + names: dict[str, str] = {} + for full_name in unique: + leaf = full_name.rsplit(".", 1)[-1] + clash = ( + _legacy_mcp_service_entry_name(full_name) in legacy_managed + or leaf_counts[leaf] > 1 + or ( + leaf in url_by_name + and url_by_name[leaf] != build_mcp_service_url(workspace, full_name) + ) + ) + names[full_name] = _legacy_mcp_service_entry_name(full_name) if clash else leaf + return names + + +def _mcp_service_full_name(server: dict, workspace: str) -> str | None: + """Full name of the MCP service a ug-named entry registers, else None. + + Only entries named with the leaf or the legacy dashed full name qualify, so a hand-named entry + or a non-service server is never touched.""" + name, url = _server_name(server), server.get("url") + prefix = build_mcp_service_url(workspace, "") + if not name or not isinstance(url, str) or not url.startswith(prefix): + return None + full_name = url.removeprefix(prefix) + if full_name.count(".") != 2 or not all(full_name.split(".")): + return None + if name not in (full_name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(full_name)): + return None + return full_name + + +def _normalize_mcp_service_entry_names( + servers: list[dict], workspace: str, existing_servers: Iterable[dict] = () +) -> list[dict]: + """Rename ug-named MCP service entries to the current naming rule, in place and in order. + + Upgrades legacy dashed entries (`main-default-exa`) to the leaf (`exa`) and downgrades leaf + entries that now collide. Entries that end up with the same name (the legacy and the new entry + of one service, e.g. after a `union`) are merged, their clients combined, so nothing is + duplicated or orphaned. Other entries are returned untouched; ``existing_servers`` (managed + servers) only constrain the names chosen.""" + full_names = [_mcp_service_full_name(server, workspace) for server in servers] + service_fulls = {full for full in full_names if full} + if not service_fulls: + return servers + others = [s for s, full in zip(servers, full_names, strict=True) if not full] + managed = list(existing_servers) + names = mcp_service_entry_names( + service_fulls, + workspace, + existing_servers=[*others, *managed], + managed_names=[n for server in managed if (n := _server_name(server))], + ) + result: list[dict] = [] + index_by_name: dict[str, int] = {} + for server, full in zip(servers, full_names, strict=True): + if not full: + result.append(server) + continue + new_name = names[full] + if new_name in index_by_name: + kept = result[index_by_name[new_name]] + clients = list(kept.get("clients") or []) + merged = clients + [c for c in server.get("clients") or [] if c not in clients] + result[index_by_name[new_name]] = {**kept, "clients": merged} + continue + index_by_name[new_name] = len(result) + result.append(server if server.get("name") == new_name else {**server, "name": new_name}) + return result + + def _mcp_entry_url_host(entry: dict) -> str | None: """Return the host of an MCP entry's URL, or ``None`` if missing/malformed.""" url = entry.get("url") @@ -661,11 +761,13 @@ def _mcp_service_choice( shows rows you can act on, so two kinds are hidden: a managed service (never ug's to add or remove) and, under `mcp add`, an already-configured one (nothing to add). Replace mode still lists ug's own as a removable toggle; an unregistered service is an add-choice.""" - registered_as = name.replace(".", "-") display_title = f"MCP: {name}" - if registered_as in (managed_names or set()): + # An entry may carry the leaf name or, from older ug versions / collisions, the dashed full name. + candidates = (name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name)) + if any(c in (managed_names or set()) for c in candidates): return None - if registered_as in known_names: + registered_as = next((c for c in candidates if c in known_names), None) + if registered_as is not None: return None if additive else _server_choice(registered_as, True, display_title) return _add_choice(f"{MCP_SERVICE_SELECTION_PREFIX}{name}", display_title) @@ -712,7 +814,7 @@ def push(choice: questionary.Choice | None) -> None: # (see resolver). The shared helper is also used by the background walk that # streams more services in, so up-front and streamed rows match exactly. push(_mcp_service_choice(name, known_names, additive, managed)) - displayed_names.add(name.replace(".", "-")) + displayed_names.update((name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name))) for name in available_external_names: display_title = f"Connection: {name}" @@ -922,12 +1024,9 @@ def _resolve_managed_mcp_servers( # slowing every `ug configure`. The `unity_catalog_location` selector above is the one path that # still discovers, since a whole-schema pointer has no explicit set to register. working: list[dict] = [] - seen: set[str] = set() + entry_names = mcp_service_entry_names(names, workspace) for full_name in sorted(set(names)): - entry_name = full_name.replace(".", "-") - if entry_name in seen: - continue - seen.add(entry_name) + entry_name = entry_names[full_name] working.append( { "name": entry_name, @@ -1232,9 +1331,12 @@ def _resolve_mcp_selection( full_name = selection.removeprefix(MCP_SERVICE_SELECTION_PREFIX) if not full_name: raise RuntimeError("missing MCP service name") - # Agent CLIs (claude/codex/gemini) reject dots in registered names. - # URL keeps the UC `..` form; entry name uses dashes. - return full_name.replace(".", "-"), build_mcp_service_url(workspace, full_name) + # Agent CLIs (claude/codex/gemini) reject dots in registered names. URL keeps the UC + # `..` form. The entry name here is the collision-safe dashed form; callers + # shorten it to the leaf via `_normalize_mcp_service_entry_names` once the whole set is known. + return _legacy_mcp_service_entry_name(full_name), build_mcp_service_url( + workspace, full_name + ) if selection.startswith(VECTOR_SEARCH_SELECTION_PREFIX): return _resolve_catalog_schema_selection( @@ -1660,6 +1762,7 @@ def _resolve_location_mcp_servers( location: str, original_servers: list[dict], services: set[str] | None = None, + managed_servers: list[dict] | None = None, ) -> list[dict]: """Build the desired MCP server list for ``--location .``. @@ -1716,10 +1819,25 @@ def _resolve_location_mcp_servers( ] original_by_name = _servers_by_name(original_servers) + # The location is a strict replacement, so previously-registered MCP services don't count as + # name holders; every other entry (and every managed server) does. + service_prefix = build_mcp_service_url(workspace, "") + holders = [ + s for s in original_servers if not str(s.get("url") or "").startswith(service_prefix) + ] + entry_names = mcp_service_entry_names( + names, + workspace, + existing_servers=[*holders, *(managed_servers or [])], + managed_names=[n for server in managed_servers or [] if (n := _server_name(server))], + ) working_servers: list[dict] = [] for full_name in names: - entry_name = full_name.replace(".", "-") - original = original_by_name.get(entry_name) + entry_name = entry_names[full_name] + # An entry from an older ug carries the dashed name; adopt it so it is renamed, not duplicated. + original = original_by_name.get(entry_name) or original_by_name.get( + _legacy_mcp_service_entry_name(full_name) + ) original_clients = list((original or {}).get("clients") or []) merged_clients = original_clients + [c for c in clients if c not in original_clients] candidate = { @@ -1906,7 +2024,8 @@ def _configure_v2_mcp_selectors( picker_servers = [s for s in original_mcp_servers if s.get("kind") != SKILLS_MCP_KIND] original_by_name = _servers_by_name(picker_servers) - managed_names = managed_mcp_server_names(state, set(clients)) + managed_servers = managed_mcp_servers(state, set(clients)) + managed_names = {name for server in managed_servers if (name := _server_name(server))} working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() skipped_managed: set[str] = set() @@ -1929,6 +2048,14 @@ def _configure_v2_mcp_selectors( if append: working_mcp_servers = _union_missing(original_mcp_servers, working_mcp_servers) + working_mcp_servers = _normalize_mcp_service_entry_names( + working_mcp_servers, workspace, managed_servers + ) + working_names = { + n + for s in working_mcp_servers + if s.get("kind") != SKILLS_MCP_KIND and (n := _server_name(s)) + } changed = apply_mcp_server_changes( original_mcp_servers, @@ -2000,17 +2127,26 @@ def configure_mcp_command( state, "Add MCP Servers" if append else "MCP Servers", agents=agents ) - managed_names = managed_mcp_server_names(state, set(clients)) + managed_servers = managed_mcp_servers(state, set(clients)) + managed_names = {name for server in managed_servers if (name := _server_name(server))} original_mcp_servers_for_location: list[dict] = list(state.get("mcp_servers") or []) if location is not None: working_mcp_servers = _resolve_location_mcp_servers( - workspace, profile, clients, location, original_mcp_servers_for_location, services + workspace, + profile, + clients, + location, + original_mcp_servers_for_location, + services, + managed_servers, ) working_mcp_servers = _drop_managed_servers(working_mcp_servers, managed_names) if append: - working_mcp_servers = _union_missing( - original_mcp_servers_for_location, working_mcp_servers + working_mcp_servers = _normalize_mcp_service_entry_names( + _union_missing(original_mcp_servers_for_location, working_mcp_servers), + workspace, + managed_servers, ) changed = apply_mcp_server_changes( original_mcp_servers_for_location, @@ -2110,6 +2246,15 @@ def configure_mcp_command( if append: working_mcp_servers = _union_missing(original_mcp_servers, working_mcp_servers) + # Shorten dashed names to the leaf and upgrade legacy dashed entries now the whole set is known. + working_mcp_servers = _normalize_mcp_service_entry_names( + working_mcp_servers, workspace, managed_servers + ) + working_names = { + n + for s in working_mcp_servers + if s.get("kind") != SKILLS_MCP_KIND and (n := _server_name(s)) + } changed = apply_mcp_server_changes( original_mcp_servers, diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 68f1fb39d..51b82a5b3 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1661,17 +1661,17 @@ def fake_list(workspace, token, parent): assert seen == {"parent": "system.ai"} assert picker_called == [] - assert [c[1] for c in configured] == ["system-ai-github", "system-ai-slack"] + assert [c[1] for c in configured] == ["github", "slack"] assert configured[0][2] == f"{WS}/ai-gateway/mcp-services/system.ai.github" assert saved_states[-1]["mcp_servers"] == [ { - "name": "system-ai-github", + "name": "github", "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", "auth": "proxy", "clients": ["claude"], }, { - "name": "system-ai-slack", + "name": "slack", "url": f"{WS}/ai-gateway/mcp-services/system.ai.slack", "auth": "proxy", "clients": ["claude"], @@ -1712,10 +1712,10 @@ def test_replaces_servers_outside_location(self, monkeypatch): assert mcp.configure_mcp_command(location="system.ai") == 0 assert removed == [("claude", "databricks-sql")] - assert [c[1] for c in configured] == ["system-ai-github"] + assert [c[1] for c in configured] == ["github"] assert saved_states[-1]["mcp_servers"] == [ { - "name": "system-ai-github", + "name": "github", "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", "auth": "proxy", "clients": ["claude"], @@ -1791,10 +1791,10 @@ def test_existing_entry_gets_reconfigured_for_newly_added_clients(self, monkeypa assert mcp.configure_mcp_command(location="system.ai") == 0 - assert [c[0] for c in configured] == ["claude", "codex"] + assert sorted(c[0] for c in configured) == ["claude", "codex"] assert saved_states[-1]["mcp_servers"] == [ { - "name": "system-ai-github", + "name": "github", "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", "auth": "proxy", "clients": ["claude", "codex"], @@ -1842,10 +1842,10 @@ def test_keeps_servers_outside_location(self, monkeypatch): # Nothing is removed; the new service is added and the outside one kept. assert removed == [] - assert [c[1] for c in configured] == ["system-ai-github"] + assert [c[1] for c in configured] == ["github"] assert saved_states[-1]["mcp_servers"] == [ { - "name": "system-ai-github", + "name": "github", "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", "auth": "proxy", "clients": ["claude"], @@ -1883,9 +1883,10 @@ def test_services_subset_keeps_others_in_location(self, monkeypatch): assert mcp.add_mcp_command(location="system.ai", services={"github"}) == 0 - assert removed == [] + # The legacy dashed `system-ai-slack` is renamed to `slack` in place (old entry removed). + assert removed == [("claude", "system-ai-slack")] names = [s["name"] for s in saved_states[-1]["mcp_servers"]] - assert names == ["system-ai-github", "system-ai-slack"] + assert names == ["github", "slack"] def test_empty_services_is_a_noop(self, monkeypatch): """`mcp add --names ""` has nothing to add, so it's a no-op that never @@ -1918,7 +1919,7 @@ def test_agents_scopes_registration_to_named_agent(self, monkeypatch): assert mcp.add_mcp_command(location="system.ai", agents={"claude"}) == 0 - assert configured == [("claude", "system-ai-github")] + assert configured == [("claude", "github")] assert saved_states[-1]["mcp_servers"][0]["clients"] == ["claude"] def test_agents_not_configured_raises(self, monkeypatch): @@ -2099,10 +2100,10 @@ def test_configures_only_the_requested_subset(self, monkeypatch): ) # slack is dropped; only the two requested services are configured. - assert sorted(c[1] for c in configured) == ["system-ai-github", "system-ai-gmail"] + assert sorted(c[1] for c in configured) == ["github", "gmail"] assert sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) == [ - "system-ai-github", - "system-ai-gmail", + "github", + "gmail", ] def test_matches_bare_short_names(self, monkeypatch): @@ -2122,7 +2123,7 @@ def test_matches_bare_short_names(self, monkeypatch): assert mcp.configure_mcp_command(location="system.ai", services={"github"}) == 0 - assert [c[1] for c in configured] == ["system-ai-github"] + assert [c[1] for c in configured] == ["github"] def test_unknown_requested_service_warns_and_skips(self, monkeypatch): configured: list[tuple[str, str, str, dict]] = [] @@ -2149,7 +2150,7 @@ def test_unknown_requested_service_warns_and_skips(self, monkeypatch): ) # The known service is still configured; the unknown one is reported, not fatal. - assert [c[1] for c in configured] == ["system-ai-github"] + assert [c[1] for c in configured] == ["github"] assert any("system.ai.ghost" in w for w in warnings) def test_empty_services_removes_everything(self, monkeypatch): @@ -2233,11 +2234,11 @@ def test_adds_and_removes_to_match_new_selection(self, monkeypatch): ) # slack removed, gmail added, github untouched (entry unchanged). - assert removed == [("claude", "system-ai-slack")] - assert [c[1] for c in configured] == ["system-ai-gmail"] + assert sorted(removed) == [("claude", "system-ai-github"), ("claude", "system-ai-slack")] + assert sorted(c[1] for c in configured) == ["github", "gmail"] assert sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) == [ - "system-ai-github", - "system-ai-gmail", + "github", + "gmail", ] def test_full_names_without_location_derive_the_schema(self, monkeypatch): @@ -2261,7 +2262,7 @@ def fake_list(workspace, token, parent): assert mcp.configure_mcp_command(services={"system.ai.github", "system.ai.slack"}) == 0 assert seen == {"parent": "system.ai"} - assert sorted(c[1] for c in configured) == ["system-ai-github", "system-ai-slack"] + assert sorted(c[1] for c in configured) == ["github", "slack"] def test_short_name_without_location_raises(self): try: @@ -3230,6 +3231,107 @@ def test_drops_foreign_workspace_skills_entry(self, monkeypatch): assert state["mcp_servers"] == [] +class TestMcpServiceEntryNames: + """Entry names are the service leaf (`exa`), falling back to the dashed full name on a clash.""" + + def test_unique_leaf_name_is_used(self): + assert mcp.mcp_service_entry_names(["main.default.exa"], WS) == {"main.default.exa": "exa"} + + def test_colliding_leaf_names_fall_back_to_dashed_full_names(self): + names = mcp.mcp_service_entry_names( + ["main.default.exa", "ml.tools.exa", "main.default.x"], WS + ) + assert names == { + "main.default.exa": "main-default-exa", + "ml.tools.exa": "ml-tools-exa", + "main.default.x": "x", + } + + def test_leaf_held_by_another_server_falls_back(self): + other = {"name": "exa", "url": f"{WS}/api/2.0/mcp/external/exa"} + names = mcp.mcp_service_entry_names(["main.default.exa"], WS, existing_servers=[other]) + assert names == {"main.default.exa": "main-default-exa"} + + def test_existing_entry_for_the_same_service_is_not_a_clash(self): + same = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "main.default.exa")} + names = mcp.mcp_service_entry_names(["main.default.exa"], WS, existing_servers=[same]) + assert names == {"main.default.exa": "exa"} + + def test_normalize_upgrades_legacy_entry_in_place_without_duplicating(self): + legacy = { + "name": "main-default-exa", + "url": mcp.build_mcp_service_url(WS, "main.default.exa"), + "auth": "proxy", + "clients": ["claude"], + } + fresh = {**legacy, "name": "exa", "clients": ["codex"]} + other = {"name": "databricks-sql", "url": f"{WS}/api/2.0/mcp/sql", "clients": ["claude"]} + assert mcp._normalize_mcp_service_entry_names([other, legacy], WS) == [ + other, + {**legacy, "name": "exa"}, + ] + merged = mcp._normalize_mcp_service_entry_names([legacy, fresh], WS) + assert merged == [{**legacy, "name": "exa", "clients": ["claude", "codex"]}] + + def test_location_rerun_renames_legacy_entry_to_leaf(self, monkeypatch): + """Upgrade: an existing `main-default-exa` entry becomes `exa`; the old one is removed + from the agent and nothing is duplicated or orphaned.""" + legacy = { + "name": "main-default-exa", + "url": mcp.build_mcp_service_url(WS, "main.default.exa"), + "auth": "proxy", + "clients": ["claude"], + } + saved_states: list[dict] = [] + configured: list[tuple[str, str]] = [] + removed: list[tuple[str, str]] = [] + _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [legacy]}) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) + ) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.append((client, name)) or [], + ) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name: removed.append((client, name)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + assert mcp.configure_mcp_command(location="main.default") == 0 + + assert configured == [("claude", "exa")] + assert removed == [("claude", "main-default-exa")] + assert saved_states[-1]["mcp_servers"] == [{**legacy, "name": "exa"}] + + def test_location_with_colliding_leaves_uses_dashed_names(self, monkeypatch): + saved_states: list[dict] = [] + _stub_location_base(monkeypatch, {**CLAUDE_STATE}) + monkeypatch.setattr( + mcp, + "list_mcp_services", + lambda ws, token, parent: (["main.default.exa", "main.default.other"], None), + ) + monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + # An already-registered `ml.tools.exa` keeps its (dashed) name when `add` unions it in. + other = { + "name": "ml-tools-exa", + "url": mcp.build_mcp_service_url(WS, "ml.tools.exa"), + "auth": "proxy", + "clients": ["claude"], + } + monkeypatch.setattr(mcp, "load_state", lambda: {**CLAUDE_STATE, "mcp_servers": [other]}) + + assert mcp.add_mcp_command(location="main.default", services={"main.default.exa"}) == 0 + + names = sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) + assert names == ["main-default-exa", "ml-tools-exa"] + + class TestResolveManagedMcpServers: def test_location_registers_the_whole_schema(self, monkeypatch): captured: dict = {} @@ -3264,8 +3366,8 @@ def test_names_build_entries_directly_without_discovery(self, monkeypatch): ["claude", "codex"], ) by_name = {s["name"]: s for s in out} - assert set(by_name) == {"system-ai-github", "system-ai-jira", "main-tools-foo"} - github = by_name["system-ai-github"] + assert set(by_name) == {"github", "jira", "foo"} + github = by_name["github"] assert github["url"] == mcp.build_mcp_service_url(WS, "system.ai.github") assert github["auth"] == "proxy" assert github["clients"] == ["claude", "codex"] @@ -3279,7 +3381,7 @@ def test_duplicate_names_collapse_to_one_entry(self, monkeypatch): out = mcp._resolve_managed_mcp_servers( {"names": ["system.ai.github", "system.ai.github"]}, WS, None, ["claude"] ) - assert [s["name"] for s in out] == ["system-ai-github"] + assert [s["name"] for s in out] == ["github"] def test_malformed_names_are_skipped_with_a_warning(self, monkeypatch): # A bare (`github`) or over-qualified (`a.b.c.d`) name isn't a full FQN and can't be @@ -3298,7 +3400,7 @@ def test_malformed_names_are_skipped_with_a_warning(self, monkeypatch): out = mcp._resolve_managed_mcp_servers( {"names": ["github", "a.b.c.d", "system.ai.slack"]}, WS, None, ["claude"] ) - assert {s["name"] for s in out} == {"system-ai-slack"} + assert {s["name"] for s in out} == {"slack"} assert warned and "github" in warned[0] and "a.b.c.d" in warned[0] def test_all_malformed_names_returns_empty_without_discovery(self, monkeypatch): @@ -3946,8 +4048,8 @@ def test_location_add_skips_an_already_managed_service(self, monkeypatch, capsys assert mcp.add_mcp_command(location="system.ai") == 0 # Only the unmanaged service is registered, and the skip is reported rather than silent. - assert configured == ["system-ai-slack"] - assert [s["name"] for s in saved_states[-1]["mcp_servers"]] == ["system-ai-slack"] + assert configured == ["slack"] + assert [s["name"] for s in saved_states[-1]["mcp_servers"]] == ["slack"] out = capsys.readouterr().out assert "system-ai-github" in out assert "managed configuration" in out From 992d17dbaf21b5595dce7e0e04772a0bd963aba9 Mon Sep 17 00:00:00 2001 From: Achille Negrier Date: Fri, 16 Oct 2026 17:58:23 +0000 Subject: [PATCH 2/6] Mock the claude CLI in the reconfigure-for-new-clients test Co-authored-by: Isaac --- tests/test_mcp.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 51b82a5b3..e511313d5 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -1787,6 +1787,11 @@ def test_existing_entry_gets_reconfigured_for_newly_added_clients(self, monkeypa "configure_client_mcp_server", lambda client, name, url, *a, **kw: configured.append((client, name, url)) or [], ) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name, *a, **kw: [], + ) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) assert mcp.configure_mcp_command(location="system.ai") == 0 From 179169fe5ec7bb15924440fc056f2271298a057a Mon Sep 17 00:00:00 2001 From: Achille Negrier Date: Sat, 17 Oct 2026 12:27:03 +0000 Subject: [PATCH 3/6] Address review: avoid short-name clashes with existing and managed entries Co-authored-by: Isaac --- src/ucode/agents/claude.py | 24 +++ src/ucode/agents/codex.py | 24 +++ src/ucode/mcp.py | 178 +++++++++++++++----- tests/conftest.py | 6 + tests/test_mcp.py | 333 +++++++++++++++++++++++++++++++++++-- 5 files changed, 510 insertions(+), 55 deletions(-) diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 183228b2a..f9bb4394d 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -1037,6 +1037,30 @@ def read_managed_mcp_urls() -> dict[str, str]: } +def read_user_mcp_urls() -> dict[str, str]: + """``{name: url}`` for every server in Claude's user-scope ``mcpServers`` (empty if unreadable). + + The URL is the entry's own ``url`` (native HTTP) or the ``--url`` argument of a ``ug mcp-proxy`` + command; ``""`` when the entry carries neither (a developer's own stdio server). Read-only, so ug + can tell a developer's hand-added server from its own before picking an entry name.""" + config = _read_claude_config_for_rewrite(claude_mcp_config_path()) + servers = (config or {}).get("mcpServers") + if not isinstance(servers, dict): + return {} + urls: dict[str, str] = {} + for name, entry in servers.items(): + if not isinstance(entry, dict): + continue + args = entry.get("args") + if isinstance(entry.get("url"), str): + urls[name] = entry["url"] + elif isinstance(args, list) and "--url" in args and args.index("--url") + 1 < len(args): + urls[name] = str(args[args.index("--url") + 1]) + else: + urls[name] = "" + return urls + + def _register_web_search_mcp( workspace: str, search_model: str, diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 0402a8732..210a55bc3 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -989,6 +989,30 @@ def read_managed_mcp_urls() -> dict[str, str]: return urls +def read_user_mcp_urls() -> dict[str, str]: + """``{name: url}`` for every server in Codex's user-scope ``[mcp_servers]`` (empty if unreadable). + + The URL is the entry's own ``url`` (native HTTP) or the ``--url`` argument of a ``ug mcp-proxy`` + command; ``""`` when the entry carries neither (a developer's own server). Read-only, so ug can + tell a developer's hand-added server from its own before picking an entry name.""" + doc = _read_user_config_for_rewrite(user_mcp_config_path()) + servers = doc.get(MANAGED_MCP_CONFIG_KEY) if doc is not None else None + if not isinstance(servers, dict): + return {} + urls: dict[str, str] = {} + for name, entry in servers.items(): + if not isinstance(entry, dict): + continue + args = entry.get("args") + if isinstance(entry.get("url"), str): + urls[name] = str(entry["url"]) + elif isinstance(args, list) and "--url" in args and args.index("--url") + 1 < len(args): + urls[name] = str(args[args.index("--url") + 1]) + else: + urls[name] = "" + return urls + + def default_model(state: dict) -> str | None: """Return a managed Codex model, or leave selection to Codex.""" if isinstance(state.get("codex_default_model"), str): diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 6acdeda8b..59de7e1b9 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -591,10 +591,12 @@ def mcp_service_entry_names( """Map each MCP service full name (`..`) to its agent entry name. The entry name is the leaf (`exa` for `main.default.exa`). A service falls back to the dashed - full name (`main-default-exa`) when its leaf is not unique: two services in ``full_names`` share - it, or an entry in ``existing_servers`` (other ug entries, managed servers) has that name but - points at a different URL. An existing entry with the same URL is the same service, not a - clash. A service whose dashed name is in ``managed_names`` keeps it: managed config written by + full name (`main-default-exa`) when the leaf is taken: an entry in ``existing_servers`` (ug's + own entries, managed servers, a developer's native client config) has that name but points at a + different URL, or two services in ``full_names`` share the leaf and neither already owns it. An + existing entry with the same URL is the same service, not a clash, and a service that already + holds its leaf keeps it, so a name once assigned stays put when a colliding service shows up + later. A service whose dashed name is in ``managed_names`` keeps it: managed config written by an older ug still holds it under that name, and the "already managed" skip matches on names. URLs are unaffected.""" unique = sorted(set(full_names)) @@ -606,18 +608,36 @@ def mcp_service_entry_names( names: dict[str, str] = {} for full_name in unique: leaf = full_name.rsplit(".", 1)[-1] + owns_leaf = url_by_name.get(leaf) == build_mcp_service_url(workspace, full_name) clash = ( _legacy_mcp_service_entry_name(full_name) in legacy_managed - or leaf_counts[leaf] > 1 - or ( - leaf in url_by_name - and url_by_name[leaf] != build_mcp_service_url(workspace, full_name) - ) + or (leaf in url_by_name and not owns_leaf) + or (leaf_counts[leaf] > 1 and not owns_leaf) ) names[full_name] = _legacy_mcp_service_entry_name(full_name) if clash else leaf return names +def _native_mcp_servers(clients: Iterable[str], ug_servers: Iterable[dict]) -> list[dict]: + """Servers already in the user-scope config of ``clients`` that ug did not register. + + Only Claude and Codex are read (the others have no reader yet). Entries named like one of + ``ug_servers`` are skipped: ug's own, which it may rename or remove. What's left, a developer's + hand-added `github`, must not be overwritten by a ug entry of the same name. Entries without a + readable URL carry none, so they never match a service URL.""" + ug_names = {name for server in ug_servers if (name := _server_name(server))} + readers = {"claude": claude.read_user_mcp_urls, "codex": codex.read_user_mcp_urls} + native: list[dict] = [] + for client in clients: + reader = readers.get(client) + if reader is None: + continue + for name, url in reader().items(): + if name not in ug_names: + native.append({"name": name, "url": url or None}) + return native + + def _mcp_service_full_name(server: dict, workspace: str) -> str | None: """Full name of the MCP service a ug-named entry registers, else None. @@ -636,25 +656,28 @@ def _mcp_service_full_name(server: dict, workspace: str) -> str | None: def _normalize_mcp_service_entry_names( - servers: list[dict], workspace: str, existing_servers: Iterable[dict] = () + servers: list[dict], + workspace: str, + managed_servers: Iterable[dict] = (), + foreign_servers: Iterable[dict] = (), ) -> list[dict]: """Rename ug-named MCP service entries to the current naming rule, in place and in order. Upgrades legacy dashed entries (`main-default-exa`) to the leaf (`exa`) and downgrades leaf entries that now collide. Entries that end up with the same name (the legacy and the new entry of one service, e.g. after a `union`) are merged, their clients combined, so nothing is - duplicated or orphaned. Other entries are returned untouched; ``existing_servers`` (managed - servers) only constrain the names chosen.""" + duplicated or orphaned. Other entries are returned untouched; ``managed_servers`` and + ``foreign_servers`` (a developer's native client entries, see :func:`_native_mcp_servers`) only + constrain the names chosen. A leaf name an entry already holds stays with it.""" full_names = [_mcp_service_full_name(server, workspace) for server in servers] service_fulls = {full for full in full_names if full} if not service_fulls: return servers - others = [s for s, full in zip(servers, full_names, strict=True) if not full] - managed = list(existing_servers) + managed = list(managed_servers) names = mcp_service_entry_names( service_fulls, workspace, - existing_servers=[*others, *managed], + existing_servers=[*servers, *managed, *foreign_servers], managed_names=[n for server in managed if (n := _server_name(server))], ) result: list[dict] = [] @@ -753,6 +776,7 @@ def _mcp_service_choice( known_names: set[str], additive: bool, managed_names: set[str] | None = None, + entry_urls: dict[str, str | None] | None = None, ) -> questionary.Choice | None: """Picker choice for one MCP-service full name (`..`), or None to hide it. @@ -760,13 +784,24 @@ def _mcp_service_choice( more services in, so a streamed row is built identically to an up-front one. The picker only shows rows you can act on, so two kinds are hidden: a managed service (never ug's to add or remove) and, under `mcp add`, an already-configured one (nothing to add). Replace mode still - lists ug's own as a removable toggle; an unregistered service is an add-choice.""" + lists ug's own as a removable toggle; an unregistered service is an add-choice. + + An entry may carry the leaf name or, from older ug versions / collisions, the dashed full name. + ``entry_urls`` (name -> URL of the known and managed entries) tells this service's entry from an + unrelated one that merely shares the leaf (`other.schema.exa`, a hand-named `exa`); a name with + no recorded URL is trusted.""" display_title = f"MCP: {name}" - # An entry may carry the leaf name or, from older ug versions / collisions, the dashed full name. candidates = (name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name)) - if any(c in (managed_names or set()) for c in candidates): + + def holds(candidate: str, names: set[str]) -> bool: + if candidate not in names: + return False + url = (entry_urls or {}).get(candidate) + return url is None or url.endswith(f"/{name}") + + if any(holds(c, managed_names or set()) for c in candidates): return None - registered_as = next((c for c in candidates if c in known_names), None) + registered_as = next((c for c in candidates if holds(c, known_names)), None) if registered_as is not None: return None if additive else _server_choice(registered_as, True, display_title) return _add_choice(f"{MCP_SERVICE_SELECTION_PREFIX}{name}", display_title) @@ -782,9 +817,11 @@ def build_mcp_picker_choices( available_uc_functions_servers: list[dict] | None = None, additive: bool = False, managed_names: set[str] | None = None, + managed_urls: dict[str, str | None] | None = None, ) -> list[questionary.Choice | questionary.Separator]: original_by_name = _servers_by_name(original_servers) managed = managed_names or set() + entry_urls = {n: s.get("url") for n, s in original_by_name.items()} | (managed_urls or {}) # Managed servers aren't in ug's own `mcp_servers` state, so add them here to make every # source's already-configured branch pick them up. known_names = set(original_by_name) | managed @@ -813,7 +850,7 @@ def push(choice: questionary.Choice | None) -> None: # Picker shows the dotted UC name; state/agents store the dashed form # (see resolver). The shared helper is also used by the background walk that # streams more services in, so up-front and streamed rows match exactly. - push(_mcp_service_choice(name, known_names, additive, managed)) + push(_mcp_service_choice(name, known_names, additive, managed, entry_urls)) displayed_names.update((name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name))) for name in available_external_names: @@ -912,6 +949,7 @@ def prompt_for_mcp_server_choices( managed_names: set[str] | None = None, background_loader: Callable[[Callable[[list[questionary.Choice]], None], threading.Event], None] | None = None, + managed_urls: dict[str, str | None] | None = None, ) -> list[str] | None | _Back: """Show the MCP server picker. Returns the list of selected values, `None` if cancelled (Ctrl-C), or `_BACK` if `allow_back` and the user pressed Left @@ -941,6 +979,7 @@ def prompt_for_mcp_server_choices( available_uc_functions_servers, additive=additive, managed_names=managed_names, + managed_urls=managed_urls, ), style=picker_style(), instruction=instruction, @@ -983,7 +1022,11 @@ def _is_app_mcp_server(server: dict) -> bool: def _resolve_managed_mcp_servers( - selector: dict, workspace: str, profile: str | None, clients: list[str] + selector: dict, + workspace: str, + profile: str | None, + clients: list[str], + existing_servers: Iterable[dict] = (), ) -> list[dict]: """Resolve a managed ``mcp_servers`` selector into MCP server entries for ``clients``. @@ -999,10 +1042,16 @@ def _resolve_managed_mcp_servers( Either way the managed set is tracked on its own and never entangles the developer's own servers or the skills connection. A ``names`` entry that isn't a full three-part FQN is skipped with a warning, so an admin's typo in one entry never blocks the developer from the valid entries. + + ``existing_servers`` are entries the developer already has (their ug ``mcp_servers`` and native + client config): a managed entry never takes a name one of them holds for a different server, or + the user-scope fallback would overwrite it. """ location = selector.get("unity_catalog_location") if isinstance(location, str) and location: - return _resolve_location_mcp_servers(workspace, profile, clients, location, []) + return _resolve_location_mcp_servers( + workspace, profile, clients, location, [], foreign_servers=existing_servers + ) names = [n for n in (selector.get("names") or []) if isinstance(n, str) and n] if not names: return [] @@ -1024,7 +1073,7 @@ def _resolve_managed_mcp_servers( # slowing every `ug configure`. The `unity_catalog_location` selector above is the one path that # still discovers, since a whole-schema pointer has no explicit set to register. working: list[dict] = [] - entry_names = mcp_service_entry_names(names, workspace) + entry_names = mcp_service_entry_names(names, workspace, existing_servers=existing_servers) for full_name in sorted(set(names)): entry_name = entry_names[full_name] working.append( @@ -1070,18 +1119,27 @@ def managed_mcp_server_names(state: dict, agents: set[str] | None = None) -> set return {name for server in managed_mcp_servers(state, agents) if (name := _server_name(server))} -def _drop_managed_servers(servers: list[dict], managed_names: set[str]) -> list[dict]: +def _managed_urls(managed_servers: Iterable[dict]) -> set[str]: + return {url for server in managed_servers if isinstance(url := server.get("url"), str)} + + +def _drop_managed_servers( + servers: list[dict], managed_names: set[str], managed_urls: Iterable[str] = () +) -> list[dict]: """Drop the servers managed config already provides, and name them so the skip isn't silent. The non-interactive counterpart to the picker's `already configured (managed)` row. Adding one - anyway would write a second, user-scope registration that shadows the managed one.""" - if not managed_names: + anyway would write a second, user-scope registration that shadows the managed one. A server is + managed when its name is, or when its URL is one the managed config provides (a leaf-named + `exa` and a legacy dashed `main-default-exa` are the same managed service).""" + urls = set(managed_urls) + if not managed_names and not urls: return servers kept: list[dict] = [] skipped: set[str] = set() for server in servers: name = _server_name(server) - if name and name in managed_names: + if name and (name in managed_names or server.get("url") in urls): skipped.add(name) else: kept.append(server) @@ -1184,7 +1242,14 @@ def reconcile_managed_mcp_servers(managed: dict, agents: set[str]) -> list[dict] workspace, profile, clients = setup_mcp_clients( state, "Managed MCP Servers", agents=scope, action_note="Registering for" ) - working = _resolve_managed_mcp_servers(selector, workspace, profile, clients) + own = [*(state.get("mcp_servers") or []), *previous] + working = _resolve_managed_mcp_servers( + selector, + workspace, + profile, + clients, + [*own, *_native_mcp_servers(clients, own)], + ) else: # No MCP client configured: nothing to register, but still clear what a prior configure left. workspace = state.get("workspace") @@ -1419,6 +1484,7 @@ def _mcp_services_background_loader( known_names: set[str], additive: bool, managed_names: set[str] | None = None, + entry_urls: dict[str, str | None] | None = None, ) -> Callable[[Callable[[list[questionary.Choice]], None], threading.Event], None]: """Return a picker `background_loader` that runs the workspace-wide MCP-services walk and streams each schema's newly-found services into the open picker as choices, so the walk @@ -1434,7 +1500,11 @@ def on_services(new_names: list[str]) -> None: new_choices = [ choice for name in new_names - if (choice := _mcp_service_choice(name, known_names, additive, managed_names)) + if ( + choice := _mcp_service_choice( + name, known_names, additive, managed_names, entry_urls + ) + ) is not None ] if new_choices: @@ -1763,6 +1833,8 @@ def _resolve_location_mcp_servers( original_servers: list[dict], services: set[str] | None = None, managed_servers: list[dict] | None = None, + foreign_servers: Iterable[dict] = (), + keep_existing: bool = False, ) -> list[dict]: """Build the desired MCP server list for ``--location .``. @@ -1819,16 +1891,23 @@ def _resolve_location_mcp_servers( ] original_by_name = _servers_by_name(original_servers) - # The location is a strict replacement, so previously-registered MCP services don't count as - # name holders; every other entry (and every managed server) does. + # The location is a strict replacement, so previously-registered MCP services outside it don't + # count as name holders (unless ``keep_existing``: `mcp add` keeps them, so their names stay + # taken); every other entry, the ones for these services (they keep their name), every managed + # server and the developer's native entries (``foreign_servers``) do. service_prefix = build_mcp_service_url(workspace, "") + location_urls = {build_mcp_service_url(workspace, n) for n in names} holders = [ - s for s in original_servers if not str(s.get("url") or "").startswith(service_prefix) + s + for s in original_servers + if keep_existing + or not str(s.get("url") or "").startswith(service_prefix) + or s.get("url") in location_urls ] entry_names = mcp_service_entry_names( names, workspace, - existing_servers=[*holders, *(managed_servers or [])], + existing_servers=[*holders, *(managed_servers or []), *foreign_servers], managed_names=[n for server in managed_servers or [] if (n := _server_name(server))], ) working_servers: list[dict] = [] @@ -2026,6 +2105,7 @@ def _configure_v2_mcp_selectors( managed_servers = managed_mcp_servers(state, set(clients)) managed_names = {name for server in managed_servers if (name := _server_name(server))} + managed_urls = _managed_urls(managed_servers) working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() skipped_managed: set[str] = set() @@ -2033,7 +2113,8 @@ def _configure_v2_mcp_selectors( entry_name, url = _resolve_mcp_selection(selection, workspace, available_app_servers) if entry_name in working_names: continue - if entry_name in managed_names: + # Managed config may carry this server under its leaf (`exa`), not the dashed name. + if entry_name in managed_names or url in managed_urls: skipped_managed.add(entry_name) continue working_mcp_servers.append( @@ -2049,8 +2130,13 @@ def _configure_v2_mcp_selectors( if append: working_mcp_servers = _union_missing(original_mcp_servers, working_mcp_servers) working_mcp_servers = _normalize_mcp_service_entry_names( - working_mcp_servers, workspace, managed_servers + working_mcp_servers, + workspace, + managed_servers, + _native_mcp_servers(clients, original_mcp_servers), ) + # After normalizing: a legacy dashed entry the union brought back may now carry a managed name. + working_mcp_servers = _drop_managed_servers(working_mcp_servers, managed_names, managed_urls) working_names = { n for s in working_mcp_servers @@ -2129,6 +2215,7 @@ def configure_mcp_command( managed_servers = managed_mcp_servers(state, set(clients)) managed_names = {name for server in managed_servers if (name := _server_name(server))} + managed_urls = _managed_urls(managed_servers) original_mcp_servers_for_location: list[dict] = list(state.get("mcp_servers") or []) if location is not None: @@ -2140,14 +2227,20 @@ def configure_mcp_command( original_mcp_servers_for_location, services, managed_servers, + _native_mcp_servers(clients, original_mcp_servers_for_location), + keep_existing=append, ) - working_mcp_servers = _drop_managed_servers(working_mcp_servers, managed_names) if append: working_mcp_servers = _normalize_mcp_service_entry_names( _union_missing(original_mcp_servers_for_location, working_mcp_servers), workspace, managed_servers, + _native_mcp_servers(clients, original_mcp_servers_for_location), ) + # After normalizing: a legacy dashed entry the union brought back may now carry a managed name. + working_mcp_servers = _drop_managed_servers( + working_mcp_servers, managed_names, managed_urls + ) changed = apply_mcp_server_changes( original_mcp_servers_for_location, working_mcp_servers, @@ -2184,6 +2277,9 @@ def configure_mcp_command( set(original_by_name) | managed_names, additive=append, managed_names=managed_names, + entry_urls={ + n: s.get("url") for s in [*picker_servers, *managed_servers] if (n := _server_name(s)) + }, ) selections = prompt_for_mcp_server_choices( discovered["external"], @@ -2196,6 +2292,7 @@ def configure_mcp_command( additive=append, managed_names=managed_names, background_loader=services_loader, + managed_urls={n: s.get("url") for s in managed_servers if (n := _server_name(s))}, ) if selections is None or isinstance(selections, _Back): # No back-navigation without the source step; `_Back` can't occur, but keep the guard @@ -2248,7 +2345,14 @@ def configure_mcp_command( working_mcp_servers = _union_missing(original_mcp_servers, working_mcp_servers) # Shorten dashed names to the leaf and upgrade legacy dashed entries now the whole set is known. working_mcp_servers = _normalize_mcp_service_entry_names( - working_mcp_servers, workspace, managed_servers + working_mcp_servers, + workspace, + managed_servers, + _native_mcp_servers(clients, original_mcp_servers), + ) + # After normalizing: a legacy dashed entry may now carry a managed name. + working_mcp_servers = _drop_managed_servers( + working_mcp_servers, managed_names, _managed_urls(managed_servers) ) working_names = { n diff --git a/tests/conftest.py b/tests/conftest.py index 3b8ff90f2..c318b1f8a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,6 +35,7 @@ def _isolate_ucode_state(tmp_path, monkeypatch): import ucode.managed_files as managed_files_mod import ucode.os_compatibility.subprocess_cross_os as subprocess_cross_os_mod import ucode.state as state_mod + from ucode.agents import claude as claude_mod from ucode.agents import codex as codex_mod state_dir = tmp_path / ".ucode" @@ -57,6 +58,11 @@ def _isolate_ucode_state(tmp_path, monkeypatch): codex_mod, "CODEX_MODEL_CATALOG_PATH", state_dir / "codex-model-catalog.json" ) monkeypatch.setattr(codex_mod, "CODEX_CONFIG_PATH", tmp_path / ".codex" / "ucode.config.toml") + # `mcp` reads the developer's native Claude/Codex MCP config to avoid name clashes; never the real one. + monkeypatch.setattr(claude_mod, "CLAUDE_MCP_CONFIG_PATH", tmp_path / ".claude.json") + monkeypatch.setattr(codex_mod, "LEGACY_CODEX_CONFIG_PATH", tmp_path / ".codex" / "config.toml") + monkeypatch.delenv("CLAUDE_CONFIG_DIR", raising=False) + monkeypatch.delenv("CODEX_HOME", raising=False) def reject_privileged_write(path, _desired_text): pytest.fail( diff --git a/tests/test_mcp.py b/tests/test_mcp.py index e511313d5..4d3c55330 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import threading from unittest.mock import MagicMock @@ -2238,7 +2239,8 @@ def test_adds_and_removes_to_match_new_selection(self, monkeypatch): == 0 ) - # slack removed, gmail added, github untouched (entry unchanged). + # slack removed, gmail added; github is renamed in place (legacy dashed entry removed, leaf + # entry added). assert sorted(removed) == [("claude", "system-ai-github"), ("claude", "system-ai-slack")] assert sorted(c[1] for c in configured) == ["github", "gmail"] assert sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) == [ @@ -2246,6 +2248,50 @@ def test_adds_and_removes_to_match_new_selection(self, monkeypatch): "gmail", ] + def test_failed_rename_is_not_saved_and_the_next_run_recovers(self, monkeypatch): + legacy = { + "name": "system-ai-github", + "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", + "auth": "proxy", + "clients": ["claude"], + } + removed: list[tuple[str, str]] = [] + configured: list[str] = [] + saved_states: list[dict] = [] + fail = [True] + _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [legacy]}) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda workspace, token, parent: (["system.ai.github"], None) + ) + + def configure(client, name, url, *a, **kw): + if fail[0]: + raise RuntimeError("claude mcp add failed") + configured.append(name) + return [] + + monkeypatch.setattr(mcp, "configure_client_mcp_server", configure) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name, *a, **kw: removed.append((client, name)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) + + # Adding `github` fails after the legacy entry was removed: the error surfaces and ug state + # is not rewritten, so it still lists the legacy entry. + with pytest.raises(RuntimeError, match="claude mcp add failed"): + mcp.configure_mcp_command(location="system.ai") + assert removed == [("claude", "system-ai-github")] + assert saved_states == [] + + # The next run starts from the same state, removes the (already gone) legacy entry again and + # registers `github`, so the rename completes with nothing duplicated. + fail[0] = False + assert mcp.configure_mcp_command(location="system.ai") == 0 + assert configured == ["github"] + assert [s["name"] for s in saved_states[-1]["mcp_servers"]] == ["github"] + def test_full_names_without_location_derive_the_schema(self, monkeypatch): configured: list[tuple[str, str, str, dict]] = [] seen: dict[str, str] = {} @@ -3302,7 +3348,7 @@ def test_location_rerun_renames_legacy_entry_to_leaf(self, monkeypatch): monkeypatch.setattr( mcp, "remove_client_mcp_server", - lambda client, name: removed.append((client, name)) or [], + lambda client, name, *a, **kw: removed.append((client, name)) or [], ) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) @@ -3312,36 +3358,221 @@ def test_location_rerun_renames_legacy_entry_to_leaf(self, monkeypatch): assert removed == [("claude", "main-default-exa")] assert saved_states[-1]["mcp_servers"] == [{**legacy, "name": "exa"}] - def test_location_with_colliding_leaves_uses_dashed_names(self, monkeypatch): + def test_assigned_leaf_name_is_sticky_when_a_colliding_service_arrives(self): + held = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "ml.tools.exa")} + names = mcp.mcp_service_entry_names( + ["main.default.exa", "ml.tools.exa"], WS, existing_servers=[held] + ) + # `ml.tools.exa` keeps `exa`; the newcomer gets the dashed name. Neither is renamed. + assert names == {"ml.tools.exa": "exa", "main.default.exa": "main-default-exa"} + + def test_add_keeps_the_existing_leaf_and_dashes_the_newcomer(self, monkeypatch): + existing = { + "name": "exa", + "url": mcp.build_mcp_service_url(WS, "ml.tools.exa"), + "auth": "proxy", + "clients": ["claude"], + } saved_states: list[dict] = [] - _stub_location_base(monkeypatch, {**CLAUDE_STATE}) + configured: list[tuple[str, str]] = [] + removed: list[tuple[str, str]] = [] + _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [existing]}) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) + ) monkeypatch.setattr( mcp, - "list_mcp_services", - lambda ws, token, parent: (["main.default.exa", "main.default.other"], None), + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.append((client, name)) or [], + ) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name, *a, **kw: removed.append((client, name)) or [], ) - monkeypatch.setattr(mcp, "configure_client_mcp_server", lambda *a, **kw: []) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - # An already-registered `ml.tools.exa` keeps its (dashed) name when `add` unions it in. - other = { - "name": "ml-tools-exa", - "url": mcp.build_mcp_service_url(WS, "ml.tools.exa"), + + assert mcp.add_mcp_command(location="main.default", services={"main.default.exa"}) == 0 + + assert removed == [] + assert configured == [("claude", "main-default-exa")] + assert sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) == [ + "exa", + "main-default-exa", + ] + + def test_unrelated_entry_with_the_leaf_name_is_not_this_service(self): + # In ug state or the developer's config, an `exa` for another server is not `main.default.exa`. + other = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "other.schema.exa")} + hand_named = {"name": "exa"} + for held in (other, hand_named): + names = mcp.mcp_service_entry_names(["main.default.exa"], WS, existing_servers=[held]) + assert names == {"main.default.exa": "main-default-exa"} + + def test_normalize_keeps_a_leaf_held_by_a_native_entry_for_another_server(self): + legacy = { + "name": "main-default-exa", + "url": mcp.build_mcp_service_url(WS, "main.default.exa"), + "clients": ["claude"], + } + native = {"name": "exa", "url": "https://other.example.com/mcp"} + out = mcp._normalize_mcp_service_entry_names([legacy], WS, foreign_servers=[native]) + assert out == [legacy] + + def test_location_leaves_a_developers_native_entry_alone(self, monkeypatch): + """A hand-added `github` in the developer's Claude config is never overwritten.""" + configured: list[str] = [] + _stub_location_base(monkeypatch, {**CLAUDE_STATE}) + monkeypatch.setattr( + mcp.claude, "read_user_mcp_urls", lambda: {"github": "https://example.com/my-github"} + ) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["system.ai.github"], None) + ) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.append(name) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: None) + + assert mcp.configure_mcp_command(location="system.ai") == 0 + + assert configured == ["system-ai-github"] + + def test_location_takes_the_leaf_when_the_native_entry_is_the_same_service(self, monkeypatch): + configured: list[str] = [] + _stub_location_base(monkeypatch, {**CLAUDE_STATE}) + same = mcp.build_mcp_service_url(WS, "system.ai.github") + monkeypatch.setattr(mcp.claude, "read_user_mcp_urls", lambda: {"github": same}) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["system.ai.github"], None) + ) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.append(name) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: None) + + assert mcp.configure_mcp_command(location="system.ai") == 0 + + assert configured == ["github"] + + def test_native_servers_skip_ug_owned_names_and_read_both_clients(self, monkeypatch): + monkeypatch.setattr( + mcp.claude, "read_user_mcp_urls", lambda: {"mine": "u1", "ug-owned": "u2", "bare": ""} + ) + monkeypatch.setattr(mcp.codex, "read_user_mcp_urls", lambda: {"mine": "u3"}) + native = mcp._native_mcp_servers(["claude", "codex", "gemini"], [{"name": "ug-owned"}]) + assert native == [ + {"name": "mine", "url": "u1"}, + {"name": "bare", "url": None}, + {"name": "mine", "url": "u3"}, + ] + + def test_picker_ignores_an_unrelated_entry_with_the_leaf_name(self): + other = {"exa": mcp.build_mcp_service_url(WS, "other.schema.exa")} + mine = {"exa": mcp.build_mcp_service_url(WS, "main.default.exa")} + # Under `mcp add` an unrelated `exa` must not hide the service; its own entry still does. + shown = mcp._mcp_service_choice("main.default.exa", {"exa"}, True, None, other) + assert shown is not None and shown.value.startswith(mcp.MCP_ADD_PREFIX) + assert mcp._mcp_service_choice("main.default.exa", {"exa"}, True, None, mine) is None + # In replace mode the unrelated `exa` is no toggle for this service either. + replace = mcp._mcp_service_choice("main.default.exa", {"exa"}, False, None, other) + assert replace is not None and replace.value.startswith(mcp.MCP_ADD_PREFIX) + # A managed `exa` for another server doesn't make this service "managed". + assert mcp._mcp_service_choice("main.default.exa", set(), True, {"exa"}, other) is not None + assert mcp._mcp_service_choice("main.default.exa", set(), True, {"exa"}, mine) is None + + def test_drop_managed_matches_a_dashed_entry_by_url(self): + url = mcp.build_mcp_service_url(WS, "main.default.exa") + servers = [{"name": "main-default-exa", "url": url}, {"name": "keep", "url": "k"}] + kept = mcp._drop_managed_servers(servers, {"exa"}, {url}) + assert [s["name"] for s in kept] == ["keep"] + + def test_legacy_entry_renamed_onto_a_managed_name_is_dropped_not_duplicated(self, monkeypatch): + """`mcp add` unions the legacy `system-ai-github` back; once renamed to the managed + `github` it must be dropped (and removed from the agent), not registered twice.""" + legacy = { + "name": "system-ai-github", + "url": mcp.build_mcp_service_url(WS, "system.ai.github"), "auth": "proxy", "clients": ["claude"], } - monkeypatch.setattr(mcp, "load_state", lambda: {**CLAUDE_STATE, "mcp_servers": [other]}) + saved_states: list[dict] = [] + configured: list[str] = [] + removed: list[tuple[str, str]] = [] + _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [legacy]}) + monkeypatch.setattr(mcp.claude, "read_managed_mcp_urls", lambda: {"github": legacy["url"]}) + monkeypatch.setattr(mcp.codex, "read_managed_mcp_urls", lambda: {}) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["system.ai.slack"], None) + ) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.append(name) or [], + ) + monkeypatch.setattr( + mcp, + "remove_client_mcp_server", + lambda client, name, *a, **kw: removed.append((client, name)) or [], + ) + monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - assert mcp.add_mcp_command(location="main.default", services={"main.default.exa"}) == 0 + assert mcp.add_mcp_command(location="system.ai") == 0 - names = sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) - assert names == ["main-default-exa", "ml-tools-exa"] + assert configured == ["slack"] + assert removed == [("claude", "system-ai-github")] + assert [s["name"] for s in saved_states[-1]["mcp_servers"]] == ["slack"] + + +class TestNativeMcpConfigReaders: + def test_claude_reads_http_and_proxy_urls(self, tmp_path): + mcp.claude.claude_mcp_config_path().write_text( + json.dumps( + { + "mcpServers": { + "http": {"type": "http", "url": "https://a.example.com/mcp"}, + "proxy": {"type": "stdio", "command": "ug", "args": ["--url", "u2"]}, + "own": {"type": "stdio", "command": "node", "args": ["x.js"]}, + } + } + ) + ) + assert mcp.claude.read_user_mcp_urls() == { + "http": "https://a.example.com/mcp", + "proxy": "u2", + "own": "", + } + + def test_codex_reads_http_and_proxy_urls(self): + path = mcp.codex.user_mcp_config_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + '[mcp_servers.http]\nurl = "https://a.example.com/mcp"\n' + '[mcp_servers.proxy]\ncommand = "ug"\nargs = ["--url", "u2"]\n' + '[mcp_servers.own]\ncommand = "node"\n' + ) + assert mcp.codex.read_user_mcp_urls() == { + "http": "https://a.example.com/mcp", + "proxy": "u2", + "own": "", + } + + def test_missing_or_unparseable_configs_read_as_empty(self): + assert mcp.claude.read_user_mcp_urls() == {} + assert mcp.codex.read_user_mcp_urls() == {} + mcp.claude.claude_mcp_config_path().write_text("{not json") + assert mcp.claude.read_user_mcp_urls() == {} class TestResolveManagedMcpServers: def test_location_registers_the_whole_schema(self, monkeypatch): captured: dict = {} - def fake_loc(ws, profile, clients, location, original, services=None): + def fake_loc(ws, profile, clients, location, original, services=None, *a, **kw): captured.update(location=location, services=services, original=original) return [{"name": "system-ai-a", "url": "u", "auth": "proxy", "clients": clients}] @@ -3388,6 +3619,39 @@ def test_duplicate_names_collapse_to_one_entry(self, monkeypatch): ) assert [s["name"] for s in out] == ["github"] + def test_names_avoid_entries_the_developer_already_has(self, monkeypatch): + # A developer's own `github` (e.g. an external connection) must not be overwritten by the + # managed fallback; the same service already registered as `github` is not a clash. + monkeypatch.setattr( + mcp, + "_resolve_location_mcp_servers", + lambda *a, **k: pytest.fail("must not discover for explicit FQN names"), + ) + mine = {"name": "github", "url": f"{WS}/api/2.0/mcp/external/github"} + out = mcp._resolve_managed_mcp_servers( + {"names": ["system.ai.github", "system.ai.jira"]}, WS, None, ["claude"], [mine] + ) + assert [s["name"] for s in out] == ["system-ai-github", "jira"] + same = {"name": "github", "url": mcp.build_mcp_service_url(WS, "system.ai.github")} + out = mcp._resolve_managed_mcp_servers( + {"names": ["system.ai.github"]}, WS, None, ["claude"], [same] + ) + assert [s["name"] for s in out] == ["github"] + + def test_location_selector_passes_the_developers_entries_for_clash_checks(self, monkeypatch): + seen: dict = {} + + def fake_loc(ws, profile, clients, location, original, services=None, *a, **kw): + seen.update(kw) + return [] + + monkeypatch.setattr(mcp, "_resolve_location_mcp_servers", fake_loc) + mine = {"name": "exa", "url": "https://example.com/exa"} + mcp._resolve_managed_mcp_servers( + {"unity_catalog_location": "system.ai"}, WS, None, ["claude"], [mine] + ) + assert list(seen["foreign_servers"]) == [mine] + def test_malformed_names_are_skipped_with_a_warning(self, monkeypatch): # A bare (`github`) or over-qualified (`a.b.c.d`) name isn't a full FQN and can't be # located under a single `.`. Skip it with a warning rather than failing, @@ -3439,7 +3703,9 @@ def fake_setup(s, section, **kw): monkeypatch.setattr( mcp, "_resolve_managed_mcp_servers", - lambda selector, ws, profile, clients: [dict(s, clients=clients) for s in resolved], + lambda selector, ws, profile, clients, *a, **kw: [ + dict(s, clients=clients) for s in resolved + ], ) monkeypatch.setattr( mcp, @@ -3492,7 +3758,7 @@ def test_location_config_registers(self, monkeypatch): monkeypatch.setattr( mcp, "_resolve_managed_mcp_servers", - lambda selector, ws, profile, clients: ( + lambda selector, ws, profile, clients, *a, **kw: ( seen_selector.update(selector) or [dict(s, clients=clients) for s in [{"name": "system-ai-a", "url": "u"}]] ), @@ -3503,6 +3769,37 @@ def test_location_config_registers(self, monkeypatch): assert seen_selector == {"unity_catalog_location": "system.ai"} assert [s["name"] for s in out] == ["system-ai-a"] + def test_reconcile_names_managed_servers_around_the_developers_entries(self, monkeypatch): + own = {"name": "exa", "url": "https://example.com/exa", "clients": ["claude"]} + state = {"workspace": WS, "profile": None, "managed_mcp_servers": [], "mcp_servers": [own]} + applied: dict = {} + self._wire( + monkeypatch, + state=state, + configured=["claude"], + resolved=[], + applied=applied, + saved={}, + setup_calls=[], + ) + monkeypatch.setattr( + mcp.claude, "read_user_mcp_urls", lambda: {"exa": "x", "hand": "https://h.example.com"} + ) + seen: dict = {} + + def fake_resolve(selector, ws, profile, clients, existing=()): + seen["existing"] = list(existing) + return [] + + monkeypatch.setattr(mcp, "_resolve_managed_mcp_servers", fake_resolve) + + mcp.reconcile_managed_mcp_servers( + {"mcp_servers": {"names": ["main.tools.exa"]}}, {"claude"} + ) + + # The ug entry, plus the native entry ug doesn't own (`exa` is ug's, so it is skipped). + assert seen["existing"] == [own, {"name": "hand", "url": "https://h.example.com"}] + def test_reconcile_removes_servers_the_admin_dropped(self, monkeypatch): # A config that no longer lists any MCP server clears the previously-registered managed set. previous = [{"name": "old", "url": "u", "auth": "proxy", "clients": ["claude"]}] From 3f209168a0ee4a3fdc3db8d554568c6bbb7ef995 Mon Sep 17 00:00:00 2001 From: Achille Negrier Date: Sat, 17 Oct 2026 13:30:56 +0000 Subject: [PATCH 4/6] Address review round 2: URL-aware holders, no duplicate managed entries Co-authored-by: Isaac --- src/ucode/agents/claude.py | 9 +---- src/ucode/agents/codex.py | 17 +++----- src/ucode/config_io.py | 15 ++++++++ src/ucode/mcp.py | 79 +++++++++++++++++++++++++++----------- tests/test_mcp.py | 57 +++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 41 deletions(-) diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index f9bb4394d..20a46a53a 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -21,6 +21,7 @@ ToolSpec, backup_existing_file, deep_merge_dict, + mcp_entry_url, prune_key_paths, read_json_safe, write_json_file, @@ -1051,13 +1052,7 @@ def read_user_mcp_urls() -> dict[str, str]: for name, entry in servers.items(): if not isinstance(entry, dict): continue - args = entry.get("args") - if isinstance(entry.get("url"), str): - urls[name] = entry["url"] - elif isinstance(args, list) and "--url" in args and args.index("--url") + 1 < len(args): - urls[name] = str(args[args.index("--url") + 1]) - else: - urls[name] = "" + urls[name] = mcp_entry_url(entry) or "" return urls diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 210a55bc3..66cf54c36 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -30,6 +30,7 @@ backup_existing_file, deep_merge_dict, is_dry_run, + mcp_entry_url, prune_key_paths, read_json_safe, read_toml_safe, @@ -981,11 +982,9 @@ def read_managed_mcp_urls() -> dict[str, str]: return {} urls: dict[str, str] = {} for name, entry in servers.items(): - args = entry.get("args") if isinstance(entry, dict) else None - if isinstance(args, list) and "--url" in args: - index = args.index("--url") - if index + 1 < len(args): - urls[name] = str(args[index + 1]) + url = mcp_entry_url(entry) + if url is not None: + urls[name] = url return urls @@ -1003,13 +1002,7 @@ def read_user_mcp_urls() -> dict[str, str]: for name, entry in servers.items(): if not isinstance(entry, dict): continue - args = entry.get("args") - if isinstance(entry.get("url"), str): - urls[name] = str(entry["url"]) - elif isinstance(args, list) and "--url" in args and args.index("--url") + 1 < len(args): - urls[name] = str(args[args.index("--url") + 1]) - else: - urls[name] = "" + urls[name] = mcp_entry_url(entry) or "" return urls diff --git a/src/ucode/config_io.py b/src/ucode/config_io.py index e431cf177..70c177b6e 100644 --- a/src/ucode/config_io.py +++ b/src/ucode/config_io.py @@ -266,3 +266,18 @@ def parse_dotenv(path: Path) -> dict[str, str]: def write_dotenv(path: Path, env: dict[str, str]) -> None: content = "".join(f'{key}="{val}"\n' for key, val in env.items()) write_text_file(path, content) + + +def mcp_entry_url(entry: object) -> str | None: + """URL an MCP config entry points at, or None when it carries none. + + The entry's own ``url`` (native HTTP) wins, else the value after ``--url`` in the ``args`` of a + ``ug mcp-proxy`` command. One parser for every reader so they cannot drift apart.""" + if not isinstance(entry, dict): + return None + if isinstance(entry.get("url"), str): + return str(entry["url"]) + args = entry.get("args") + if isinstance(args, list) and "--url" in args and args.index("--url") + 1 < len(args): + return str(args[args.index("--url") + 1]) + return None diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 59de7e1b9..8684a6d80 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -587,6 +587,7 @@ def mcp_service_entry_names( *, existing_servers: Iterable[dict] = (), managed_names: Iterable[str] = (), + reuse_same_url: bool = False, ) -> dict[str, str]: """Map each MCP service full name (`..`) to its agent entry name. @@ -598,23 +599,38 @@ def mcp_service_entry_names( holds its leaf keeps it, so a name once assigned stays put when a colliding service shows up later. A service whose dashed name is in ``managed_names`` keeps it: managed config written by an older ug still holds it under that name, and the "already managed" skip matches on names. - URLs are unaffected.""" + URLs are unaffected. + + Several holders can share a name (a Claude `github` and a Codex `github`): the leaf is free only + if every holder of it has this service's URL. With ``reuse_same_url`` a service whose leaf is + not already its own reuses the name of an existing entry with its own URL (a legacy + `system-ai-github`), so a managed entry takes that entry's place instead of running next to it.""" unique = sorted(set(full_names)) leaf_counts = Counter(full_name.rsplit(".", 1)[-1] for full_name in unique) - url_by_name = { - name: server.get("url") for server in existing_servers if (name := _server_name(server)) - } + urls_by_name: dict[str, set[str | None]] = {} + name_by_url: dict[str, str] = {} + for server in existing_servers: + if not (existing_name := _server_name(server)): + continue + urls_by_name.setdefault(existing_name, set()).add(server.get("url")) + if isinstance(server.get("url"), str): + name_by_url.setdefault(server["url"], existing_name) legacy_managed = set(managed_names) names: dict[str, str] = {} for full_name in unique: leaf = full_name.rsplit(".", 1)[-1] - owns_leaf = url_by_name.get(leaf) == build_mcp_service_url(workspace, full_name) + url = build_mcp_service_url(workspace, full_name) + holder_urls = urls_by_name.get(leaf, set()) + owns_leaf = holder_urls == {url} clash = ( _legacy_mcp_service_entry_name(full_name) in legacy_managed - or (leaf in url_by_name and not owns_leaf) + or (bool(holder_urls) and not owns_leaf) or (leaf_counts[leaf] > 1 and not owns_leaf) ) - names[full_name] = _legacy_mcp_service_entry_name(full_name) if clash else leaf + if reuse_same_url and not owns_leaf and url in name_by_url and leaf_counts[leaf] == 1: + names[full_name] = name_by_url[url] + else: + names[full_name] = _legacy_mcp_service_entry_name(full_name) if clash else leaf return names @@ -771,6 +787,22 @@ def _add_choice(selection: str, title: str) -> questionary.Choice: return questionary.Choice(title=title, value=f"{MCP_ADD_PREFIX}{selection}") +def _mcp_service_holder( + name: str, names: set[str], entry_urls: dict[str, str | None] | None = None +) -> str | None: + """Entry name in ``names`` that registers MCP service ``name``, else None. + + A leaf or dashed entry counts only if its recorded URL is this service's (a name with no + recorded URL is trusted); an unrelated `exa` that merely shares the leaf does not.""" + for candidate in (name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name)): + if candidate not in names: + continue + url = (entry_urls or {}).get(candidate) + if url is None or url.endswith(f"/{name}"): + return candidate + return None + + def _mcp_service_choice( name: str, known_names: set[str], @@ -791,17 +823,9 @@ def _mcp_service_choice( unrelated one that merely shares the leaf (`other.schema.exa`, a hand-named `exa`); a name with no recorded URL is trusted.""" display_title = f"MCP: {name}" - candidates = (name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name)) - - def holds(candidate: str, names: set[str]) -> bool: - if candidate not in names: - return False - url = (entry_urls or {}).get(candidate) - return url is None or url.endswith(f"/{name}") - - if any(holds(c, managed_names or set()) for c in candidates): + if _mcp_service_holder(name, managed_names or set(), entry_urls) is not None: return None - registered_as = next((c for c in candidates if holds(c, known_names)), None) + registered_as = _mcp_service_holder(name, known_names, entry_urls) if registered_as is not None: return None if additive else _server_choice(registered_as, True, display_title) return _add_choice(f"{MCP_SERVICE_SELECTION_PREFIX}{name}", display_title) @@ -851,7 +875,13 @@ def push(choice: questionary.Choice | None) -> None: # (see resolver). The shared helper is also used by the background walk that # streams more services in, so up-front and streamed rows match exactly. push(_mcp_service_choice(name, known_names, additive, managed, entry_urls)) - displayed_names.update((name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name))) + # Only the entry this service actually holds is represented by its row; an unrelated entry + # sharing the leaf stays undisplayed so the fallback toggle below still lists it. + held = _mcp_service_holder(name, known_names, entry_urls) or _mcp_service_holder( + name, managed, entry_urls + ) + if held is not None: + displayed_names.add(held) for name in available_external_names: display_title = f"Connection: {name}" @@ -1073,7 +1103,9 @@ def _resolve_managed_mcp_servers( # slowing every `ug configure`. The `unity_catalog_location` selector above is the one path that # still discovers, since a whole-schema pointer has no explicit set to register. working: list[dict] = [] - entry_names = mcp_service_entry_names(names, workspace, existing_servers=existing_servers) + entry_names = mcp_service_entry_names( + names, workspace, existing_servers=existing_servers, reuse_same_url=True + ) for full_name in sorted(set(names)): entry_name = entry_names[full_name] working.append( @@ -1914,9 +1946,12 @@ def _resolve_location_mcp_servers( for full_name in names: entry_name = entry_names[full_name] # An entry from an older ug carries the dashed name; adopt it so it is renamed, not duplicated. - original = original_by_name.get(entry_name) or original_by_name.get( - _legacy_mcp_service_entry_name(full_name) - ) + service_url = build_mcp_service_url(workspace, full_name) + legacy = original_by_name.get(_legacy_mcp_service_entry_name(full_name)) + original = original_by_name.get(entry_name) + if original is not None and original.get("url") != service_url: + original = None + original = original or legacy original_clients = list((original or {}).get("clients") or []) merged_clients = original_clients + [c for c in clients if c not in original_clients] candidate = { diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 4d3c55330..1b1c98ab1 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -3485,6 +3485,63 @@ def test_picker_ignores_an_unrelated_entry_with_the_leaf_name(self): assert mcp._mcp_service_choice("main.default.exa", set(), True, {"exa"}, other) is not None assert mcp._mcp_service_choice("main.default.exa", set(), True, {"exa"}, mine) is None + def test_any_holder_with_a_different_url_makes_the_leaf_a_clash(self): + # Claude's hand-added `github` and Codex's `github` for this service share a name. + mine = {"name": "github", "url": mcp.build_mcp_service_url(WS, "system.ai.github")} + hand = {"name": "github", "url": "https://example.com/my-github"} + for order in ([mine, hand], [hand, mine]): + names = mcp.mcp_service_entry_names(["system.ai.github"], WS, existing_servers=order) + assert names == {"system.ai.github": "system-ai-github"} + same = mcp.mcp_service_entry_names(["system.ai.github"], WS, existing_servers=[mine, mine]) + assert same == {"system.ai.github": "github"} + + def test_picker_keeps_an_unrelated_leaf_entry_as_its_own_toggle(self): + other = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "other.schema.exa")} + choices = mcp.build_mcp_picker_choices( + [], [], [], [other], available_mcp_service_names=["main.default.exa"] + ) + values = [c.value for c in choices] + assert f"{mcp.MCP_ADD_PREFIX}{mcp.MCP_SERVICE_SELECTION_PREFIX}main.default.exa" in values + # The user's unrelated `exa` is still listed (so replace mode does not drop it). + assert "exa" in values + + def test_picker_does_not_repeat_a_service_that_holds_its_leaf(self): + mine = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "main.default.exa")} + choices = mcp.build_mcp_picker_choices( + [], [], [], [mine], available_mcp_service_names=["main.default.exa"] + ) + assert [c.value for c in choices] == ["exa"] + + def test_managed_entry_reuses_the_name_of_a_same_url_user_entry(self, monkeypatch): + monkeypatch.setattr( + mcp, + "_resolve_location_mcp_servers", + lambda *a, **k: pytest.fail("must not discover for explicit FQN names"), + ) + legacy = { + "name": "system-ai-github", + "url": mcp.build_mcp_service_url(WS, "system.ai.github"), + } + out = mcp._resolve_managed_mcp_servers( + {"names": ["system.ai.github"]}, WS, None, ["claude"], [legacy] + ) + assert [s["name"] for s in out] == ["system-ai-github"] + + def test_reconfigure_does_not_inherit_clients_of_an_unrelated_entry(self, monkeypatch): + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) + ) + monkeypatch.setattr(mcp, "get_databricks_token", lambda *a, **k: "t") + unrelated = { + "name": "exa", + "url": mcp.build_mcp_service_url(WS, "ml.tools.exa"), + "auth": "proxy", + "clients": ["gemini"], + } + out = mcp._resolve_location_mcp_servers(WS, None, ["claude"], "main.default", [unrelated]) + mine = [s for s in out if s["url"].endswith("main.default.exa")] + assert [(s["name"], s["clients"]) for s in mine] == [("exa", ["claude"])] + def test_drop_managed_matches_a_dashed_entry_by_url(self): url = mcp.build_mcp_service_url(WS, "main.default.exa") servers = [{"name": "main-default-exa", "url": url}, {"name": "keep", "url": "k"}] From 4b3a9adcb07dcfeb818a897b8464ab39665a2942 Mon Sep 17 00:00:00 2001 From: Achille Negrier Date: Sat, 17 Oct 2026 14:14:23 +0000 Subject: [PATCH 5/6] Simplify: short name only when completely free, never rename existing entries Co-authored-by: Isaac --- src/ucode/agents/claude.py | 22 +- src/ucode/agents/codex.py | 38 +-- src/ucode/config_io.py | 15 - src/ucode/mcp.py | 390 +++++++---------------- tests/test_mcp.py | 615 +++++++++---------------------------- 5 files changed, 279 insertions(+), 801 deletions(-) diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 9a9edb76f..c81e1fa5d 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -21,7 +21,6 @@ ToolSpec, backup_existing_file, deep_merge_dict, - mcp_entry_url, prune_key_paths, read_json_safe, write_json_file, @@ -1075,22 +1074,15 @@ def read_managed_mcp_urls() -> dict[str, str]: } -def read_user_mcp_urls() -> dict[str, str]: - """``{name: url}`` for every server in Claude's user-scope ``mcpServers`` (empty if unreadable). +def read_user_mcp_names() -> set[str] | None: + """Names in Claude's user-scope ``mcpServers``; ``None`` when the config can't be parsed. - The URL is the entry's own ``url`` (native HTTP) or the ``--url`` argument of a ``ug mcp-proxy`` - command; ``""`` when the entry carries neither (a developer's own stdio server). Read-only, so ug - can tell a developer's hand-added server from its own before picking an entry name.""" + Read-only, so ug can tell a name is free before picking an entry name.""" config = _read_claude_config_for_rewrite(claude_mcp_config_path()) - servers = (config or {}).get("mcpServers") - if not isinstance(servers, dict): - return {} - urls: dict[str, str] = {} - for name, entry in servers.items(): - if not isinstance(entry, dict): - continue - urls[name] = mcp_entry_url(entry) or "" - return urls + if config is None: + return None + servers = config.get("mcpServers") + return set(servers) if isinstance(servers, dict) else set() def _register_web_search_mcp( diff --git a/src/ucode/agents/codex.py b/src/ucode/agents/codex.py index 13b6b5ec5..8531a6897 100644 --- a/src/ucode/agents/codex.py +++ b/src/ucode/agents/codex.py @@ -30,7 +30,6 @@ backup_existing_file, deep_merge_dict, is_dry_run, - mcp_entry_url, prune_key_paths, read_json_safe, read_toml_safe, @@ -867,6 +866,17 @@ def _read_user_config_for_rewrite(path: Path) -> tomlkit.TOMLDocument | None: return None +def read_user_mcp_names() -> set[str] | None: + """Names in Codex's user-scope ``[mcp_servers]``; ``None`` when the config can't be parsed. + + Read-only, so ug can tell a name is free before picking an entry name.""" + doc = _read_user_config_for_rewrite(user_mcp_config_path()) + if doc is None: + return None + servers = doc.get(MANAGED_MCP_CONFIG_KEY) + return set(servers) if isinstance(servers, dict) else set() + + def write_user_mcp_servers(add: dict[str, dict], remove: set[str]) -> set[str]: """Apply ``add``/``remove`` to Codex's user-scope ``[mcp_servers]`` (``~/.codex/config.toml``, or under ``$CODEX_HOME``) in a single read-modify-write, instead of one ``codex mcp`` subprocess @@ -982,27 +992,11 @@ def read_managed_mcp_urls() -> dict[str, str]: return {} urls: dict[str, str] = {} for name, entry in servers.items(): - url = mcp_entry_url(entry) - if url is not None: - urls[name] = url - return urls - - -def read_user_mcp_urls() -> dict[str, str]: - """``{name: url}`` for every server in Codex's user-scope ``[mcp_servers]`` (empty if unreadable). - - The URL is the entry's own ``url`` (native HTTP) or the ``--url`` argument of a ``ug mcp-proxy`` - command; ``""`` when the entry carries neither (a developer's own server). Read-only, so ug can - tell a developer's hand-added server from its own before picking an entry name.""" - doc = _read_user_config_for_rewrite(user_mcp_config_path()) - servers = doc.get(MANAGED_MCP_CONFIG_KEY) if doc is not None else None - if not isinstance(servers, dict): - return {} - urls: dict[str, str] = {} - for name, entry in servers.items(): - if not isinstance(entry, dict): - continue - urls[name] = mcp_entry_url(entry) or "" + args = entry.get("args") if isinstance(entry, dict) else None + if isinstance(args, list) and "--url" in args: + index = args.index("--url") + if index + 1 < len(args): + urls[name] = str(args[index + 1]) return urls diff --git a/src/ucode/config_io.py b/src/ucode/config_io.py index 70c177b6e..e431cf177 100644 --- a/src/ucode/config_io.py +++ b/src/ucode/config_io.py @@ -266,18 +266,3 @@ def parse_dotenv(path: Path) -> dict[str, str]: def write_dotenv(path: Path, env: dict[str, str]) -> None: content = "".join(f'{key}="{val}"\n' for key, val in env.items()) write_text_file(path, content) - - -def mcp_entry_url(entry: object) -> str | None: - """URL an MCP config entry points at, or None when it carries none. - - The entry's own ``url`` (native HTTP) wins, else the value after ``--url`` in the ``args`` of a - ``ug mcp-proxy`` command. One parser for every reader so they cannot drift apart.""" - if not isinstance(entry, dict): - return None - if isinstance(entry.get("url"), str): - return str(entry["url"]) - args = entry.get("args") - if isinstance(args, list) and "--url" in args and args.index("--url") + 1 < len(args): - return str(args[args.index("--url") + 1]) - return None diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 06fbb6532..6c7e07efd 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -576,144 +576,6 @@ def _servers_by_name(mcp_servers: list[dict]) -> dict[str, dict]: return servers -def _legacy_mcp_service_entry_name(full_name: str) -> str: - """Dashed entry name (`main-default-exa`): agent CLIs reject dots in registered names.""" - return full_name.replace(".", "-") - - -def mcp_service_entry_names( - full_names: Iterable[str], - workspace: str, - *, - existing_servers: Iterable[dict] = (), - managed_names: Iterable[str] = (), - reuse_same_url: bool = False, -) -> dict[str, str]: - """Map each MCP service full name (`..`) to its agent entry name. - - The entry name is the leaf (`exa` for `main.default.exa`). A service falls back to the dashed - full name (`main-default-exa`) when the leaf is taken: an entry in ``existing_servers`` (ug's - own entries, managed servers, a developer's native client config) has that name but points at a - different URL, or two services in ``full_names`` share the leaf and neither already owns it. An - existing entry with the same URL is the same service, not a clash, and a service that already - holds its leaf keeps it, so a name once assigned stays put when a colliding service shows up - later. A service whose dashed name is in ``managed_names`` keeps it: managed config written by - an older ug still holds it under that name, and the "already managed" skip matches on names. - URLs are unaffected. - - Several holders can share a name (a Claude `github` and a Codex `github`): the leaf is free only - if every holder of it has this service's URL. With ``reuse_same_url`` a service whose leaf is - not already its own reuses the name of an existing entry with its own URL (a legacy - `system-ai-github`), so a managed entry takes that entry's place instead of running next to it.""" - unique = sorted(set(full_names)) - leaf_counts = Counter(full_name.rsplit(".", 1)[-1] for full_name in unique) - urls_by_name: dict[str, set[str | None]] = {} - name_by_url: dict[str, str] = {} - for server in existing_servers: - if not (existing_name := _server_name(server)): - continue - urls_by_name.setdefault(existing_name, set()).add(server.get("url")) - if isinstance(server.get("url"), str): - name_by_url.setdefault(server["url"], existing_name) - legacy_managed = set(managed_names) - names: dict[str, str] = {} - for full_name in unique: - leaf = full_name.rsplit(".", 1)[-1] - url = build_mcp_service_url(workspace, full_name) - holder_urls = urls_by_name.get(leaf, set()) - owns_leaf = holder_urls == {url} - clash = ( - _legacy_mcp_service_entry_name(full_name) in legacy_managed - or (bool(holder_urls) and not owns_leaf) - or (leaf_counts[leaf] > 1 and not owns_leaf) - ) - if reuse_same_url and not owns_leaf and url in name_by_url and leaf_counts[leaf] == 1: - names[full_name] = name_by_url[url] - else: - names[full_name] = _legacy_mcp_service_entry_name(full_name) if clash else leaf - return names - - -def _native_mcp_servers(clients: Iterable[str], ug_servers: Iterable[dict]) -> list[dict]: - """Servers already in the user-scope config of ``clients`` that ug did not register. - - Only Claude and Codex are read (the others have no reader yet). Entries named like one of - ``ug_servers`` are skipped: ug's own, which it may rename or remove. What's left, a developer's - hand-added `github`, must not be overwritten by a ug entry of the same name. Entries without a - readable URL carry none, so they never match a service URL.""" - ug_names = {name for server in ug_servers if (name := _server_name(server))} - readers = {"claude": claude.read_user_mcp_urls, "codex": codex.read_user_mcp_urls} - native: list[dict] = [] - for client in clients: - reader = readers.get(client) - if reader is None: - continue - for name, url in reader().items(): - if name not in ug_names: - native.append({"name": name, "url": url or None}) - return native - - -def _mcp_service_full_name(server: dict, workspace: str) -> str | None: - """Full name of the MCP service a ug-named entry registers, else None. - - Only entries named with the leaf or the legacy dashed full name qualify, so a hand-named entry - or a non-service server is never touched.""" - name, url = _server_name(server), server.get("url") - prefix = build_mcp_service_url(workspace, "") - if not name or not isinstance(url, str) or not url.startswith(prefix): - return None - full_name = url.removeprefix(prefix) - if full_name.count(".") != 2 or not all(full_name.split(".")): - return None - if name not in (full_name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(full_name)): - return None - return full_name - - -def _normalize_mcp_service_entry_names( - servers: list[dict], - workspace: str, - managed_servers: Iterable[dict] = (), - foreign_servers: Iterable[dict] = (), -) -> list[dict]: - """Rename ug-named MCP service entries to the current naming rule, in place and in order. - - Upgrades legacy dashed entries (`main-default-exa`) to the leaf (`exa`) and downgrades leaf - entries that now collide. Entries that end up with the same name (the legacy and the new entry - of one service, e.g. after a `union`) are merged, their clients combined, so nothing is - duplicated or orphaned. Other entries are returned untouched; ``managed_servers`` and - ``foreign_servers`` (a developer's native client entries, see :func:`_native_mcp_servers`) only - constrain the names chosen. A leaf name an entry already holds stays with it.""" - full_names = [_mcp_service_full_name(server, workspace) for server in servers] - service_fulls = {full for full in full_names if full} - if not service_fulls: - return servers - managed = list(managed_servers) - names = mcp_service_entry_names( - service_fulls, - workspace, - existing_servers=[*servers, *managed, *foreign_servers], - managed_names=[n for server in managed if (n := _server_name(server))], - ) - result: list[dict] = [] - index_by_name: dict[str, int] = {} - for server, full in zip(servers, full_names, strict=True): - if not full: - result.append(server) - continue - new_name = names[full] - if new_name in index_by_name: - kept = result[index_by_name[new_name]] - clients = list(kept.get("clients") or []) - merged = clients + [c for c in server.get("clients") or [] if c not in clients] - result[index_by_name[new_name]] = {**kept, "clients": merged} - continue - index_by_name[new_name] = len(result) - result.append(server if server.get("name") == new_name else {**server, "name": new_name}) - return result - - def _mcp_entry_url_host(entry: dict) -> str | None: """Return the host of an MCP entry's URL, or ``None`` if missing/malformed.""" url = entry.get("url") @@ -783,22 +645,77 @@ def _server_choice(name: str, checked: bool, title: str | None = None) -> questi ) +def _native_mcp_names() -> set[str] | None: + """Names in the developer's own Claude and Codex MCP config; ``None`` if either can't be parsed.""" + names: set[str] = set() + for reader in (claude.read_user_mcp_names, codex.read_user_mcp_names): + found = reader() + if found is None: + return None + names |= found + return names + + +def mcp_service_entry_names( + full_names: Iterable[str], workspace: str, existing_servers: Iterable[dict] = () +) -> dict[str, str]: + """Map each MCP service full name (`..`) to its agent entry name. + + A service already registered in ``existing_servers`` (ug state, managed) keeps that entry's + name, so nothing is ever renamed. A new service gets its leaf (`exa` for `main.default.exa`) + only when the name is completely free: no existing entry, no entry in the developer's native + Claude/Codex config (any URL), no other service in ``full_names`` with the same leaf, and the + native config was readable. Otherwise it gets the dashed full name (`main-default-exa`; agent + CLIs reject dots in names).""" + unique = sorted(set(full_names)) + leaf_counts = Counter(full_name.rsplit(".", 1)[-1] for full_name in unique) + name_by_url: dict[str, str] = {} + taken: set[str] = set() + for server in existing_servers: + if name := _server_name(server): + taken.add(name) + if isinstance(server.get("url"), str): + name_by_url.setdefault(server["url"], name) + native = ( + _native_mcp_names() + if any(build_mcp_service_url(workspace, n) not in name_by_url for n in unique) + else set() + ) + names: dict[str, str] = {} + for full_name in unique: + leaf = full_name.rsplit(".", 1)[-1] + if (url := build_mcp_service_url(workspace, full_name)) in name_by_url: + names[full_name] = name_by_url[url] + elif native is not None and leaf_counts[leaf] == 1 and leaf not in taken | native: + names[full_name] = leaf + else: + names[full_name] = full_name.replace(".", "-") + return names + + +def _mcp_service_selection_names( + selections: Iterable[str], workspace: str, existing_servers: Iterable[dict] +) -> dict[str, str]: + """Entry names for the MCP-service selections in ``selections``, decided as one set.""" + prefix = MCP_SERVICE_SELECTION_PREFIX + full_names = [s.removeprefix(prefix) for s in selections if s.startswith(prefix)] + return mcp_service_entry_names([n for n in full_names if n], workspace, existing_servers) + + def _add_choice(selection: str, title: str) -> questionary.Choice: return questionary.Choice(title=title, value=f"{MCP_ADD_PREFIX}{selection}") def _mcp_service_holder( - name: str, names: set[str], entry_urls: dict[str, str | None] | None = None + name: str, names: set[str], entry_urls: dict[str, str | None] ) -> str | None: - """Entry name in ``names`` that registers MCP service ``name``, else None. + """Entry name in ``names`` that registers MCP service ``name`` (leaf or dashed), else None. - A leaf or dashed entry counts only if its recorded URL is this service's (a name with no - recorded URL is trusted); an unrelated `exa` that merely shares the leaf does not.""" - for candidate in (name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name)): - if candidate not in names: - continue - url = (entry_urls or {}).get(candidate) - if url is None or url.endswith(f"/{name}"): + An entry counts only if its recorded URL is this service's, so an unrelated `exa` that merely + shares the leaf does not (an entry with no recorded URL is trusted).""" + for candidate in (name.replace(".", "-"), name.rsplit(".", 1)[-1]): + url = entry_urls.get(candidate) + if candidate in names and (url is None or url.endswith(f"/{name}")): return candidate return None @@ -816,17 +733,12 @@ def _mcp_service_choice( more services in, so a streamed row is built identically to an up-front one. The picker only shows rows you can act on, so two kinds are hidden: a managed service (never ug's to add or remove) and, under `mcp add`, an already-configured one (nothing to add). Replace mode still - lists ug's own as a removable toggle; an unregistered service is an add-choice. - - An entry may carry the leaf name or, from older ug versions / collisions, the dashed full name. - ``entry_urls`` (name -> URL of the known and managed entries) tells this service's entry from an - unrelated one that merely shares the leaf (`other.schema.exa`, a hand-named `exa`); a name with - no recorded URL is trusted.""" + lists ug's own as a removable toggle; an unregistered service is an add-choice.""" + urls = entry_urls or {} display_title = f"MCP: {name}" - if _mcp_service_holder(name, managed_names or set(), entry_urls) is not None: + if _mcp_service_holder(name, managed_names or set(), urls) is not None: return None - registered_as = _mcp_service_holder(name, known_names, entry_urls) - if registered_as is not None: + if (registered_as := _mcp_service_holder(name, known_names, urls)) is not None: return None if additive else _server_choice(registered_as, True, display_title) return _add_choice(f"{MCP_SERVICE_SELECTION_PREFIX}{name}", display_title) @@ -841,11 +753,10 @@ def build_mcp_picker_choices( available_uc_functions_servers: list[dict] | None = None, additive: bool = False, managed_names: set[str] | None = None, - managed_urls: dict[str, str | None] | None = None, + entry_urls: dict[str, str | None] | None = None, ) -> list[questionary.Choice | questionary.Separator]: original_by_name = _servers_by_name(original_servers) managed = managed_names or set() - entry_urls = {n: s.get("url") for n, s in original_by_name.items()} | (managed_urls or {}) # Managed servers aren't in ug's own `mcp_servers` state, so add them here to make every # source's already-configured branch pick them up. known_names = set(original_by_name) | managed @@ -875,11 +786,8 @@ def push(choice: questionary.Choice | None) -> None: # (see resolver). The shared helper is also used by the background walk that # streams more services in, so up-front and streamed rows match exactly. push(_mcp_service_choice(name, known_names, additive, managed, entry_urls)) - # Only the entry this service actually holds is represented by its row; an unrelated entry - # sharing the leaf stays undisplayed so the fallback toggle below still lists it. - held = _mcp_service_holder(name, known_names, entry_urls) or _mcp_service_holder( - name, managed, entry_urls - ) + # Only the entry this service holds is represented by its row. + held = _mcp_service_holder(name, known_names, entry_urls or {}) if held is not None: displayed_names.add(held) @@ -979,7 +887,7 @@ def prompt_for_mcp_server_choices( managed_names: set[str] | None = None, background_loader: Callable[[Callable[[list[questionary.Choice]], None], threading.Event], None] | None = None, - managed_urls: dict[str, str | None] | None = None, + entry_urls: dict[str, str | None] | None = None, ) -> list[str] | None | _Back: """Show the MCP server picker. Returns the list of selected values, `None` if cancelled (Ctrl-C), or `_BACK` if `allow_back` and the user pressed Left @@ -1009,7 +917,7 @@ def prompt_for_mcp_server_choices( available_uc_functions_servers, additive=additive, managed_names=managed_names, - managed_urls=managed_urls, + entry_urls=entry_urls, ), style=picker_style(), instruction=instruction, @@ -1072,15 +980,11 @@ def _resolve_managed_mcp_servers( Either way the managed set is tracked on its own and never entangles the developer's own servers or the skills connection. A ``names`` entry that isn't a full three-part FQN is skipped with a warning, so an admin's typo in one entry never blocks the developer from the valid entries. - - ``existing_servers`` are entries the developer already has (their ug ``mcp_servers`` and native - client config): a managed entry never takes a name one of them holds for a different server, or - the user-scope fallback would overwrite it. """ location = selector.get("unity_catalog_location") if isinstance(location, str) and location: return _resolve_location_mcp_servers( - workspace, profile, clients, location, [], foreign_servers=existing_servers + workspace, profile, clients, location, [], existing_servers=existing_servers ) names = [n for n in (selector.get("names") or []) if isinstance(n, str) and n] if not names: @@ -1103,11 +1007,13 @@ def _resolve_managed_mcp_servers( # slowing every `ug configure`. The `unity_catalog_location` selector above is the one path that # still discovers, since a whole-schema pointer has no explicit set to register. working: list[dict] = [] - entry_names = mcp_service_entry_names( - names, workspace, existing_servers=existing_servers, reuse_same_url=True - ) + seen: set[str] = set() + entry_names = mcp_service_entry_names(names, workspace, existing_servers) for full_name in sorted(set(names)): entry_name = entry_names[full_name] + if entry_name in seen: + continue + seen.add(entry_name) working.append( { "name": entry_name, @@ -1151,27 +1057,18 @@ def managed_mcp_server_names(state: dict, agents: set[str] | None = None) -> set return {name for server in managed_mcp_servers(state, agents) if (name := _server_name(server))} -def _managed_urls(managed_servers: Iterable[dict]) -> set[str]: - return {url for server in managed_servers if isinstance(url := server.get("url"), str)} - - -def _drop_managed_servers( - servers: list[dict], managed_names: set[str], managed_urls: Iterable[str] = () -) -> list[dict]: +def _drop_managed_servers(servers: list[dict], managed_names: set[str]) -> list[dict]: """Drop the servers managed config already provides, and name them so the skip isn't silent. The non-interactive counterpart to the picker's `already configured (managed)` row. Adding one - anyway would write a second, user-scope registration that shadows the managed one. A server is - managed when its name is, or when its URL is one the managed config provides (a leaf-named - `exa` and a legacy dashed `main-default-exa` are the same managed service).""" - urls = set(managed_urls) - if not managed_names and not urls: + anyway would write a second, user-scope registration that shadows the managed one.""" + if not managed_names: return servers kept: list[dict] = [] skipped: set[str] = set() for server in servers: name = _server_name(server) - if name and (name in managed_names or server.get("url") in urls): + if name and name in managed_names: skipped.add(name) else: kept.append(server) @@ -1274,13 +1171,8 @@ def reconcile_managed_mcp_servers(managed: dict, agents: set[str]) -> list[dict] workspace, profile, clients = setup_mcp_clients( state, "Managed MCP Servers", agents=scope, action_note="Registering for" ) - own = [*(state.get("mcp_servers") or []), *previous] working = _resolve_managed_mcp_servers( - selector, - workspace, - profile, - clients, - [*own, *_native_mcp_servers(clients, own)], + selector, workspace, profile, clients, [*(state.get("mcp_servers") or []), *previous] ) else: # No MCP client configured: nothing to register, but still clear what a prior configure left. @@ -1392,6 +1284,7 @@ def _resolve_mcp_selection( available_genie_servers: list[dict] | None = None, available_vector_search_servers: list[dict] | None = None, available_uc_functions_servers: list[dict] | None = None, + service_names: dict[str, str] | None = None, ) -> tuple[str, str]: if selection.startswith(APP_MCP_SELECTION_PREFIX): app_name = selection.removeprefix(APP_MCP_SELECTION_PREFIX) @@ -1428,12 +1321,11 @@ def _resolve_mcp_selection( full_name = selection.removeprefix(MCP_SERVICE_SELECTION_PREFIX) if not full_name: raise RuntimeError("missing MCP service name") - # Agent CLIs (claude/codex/gemini) reject dots in registered names. URL keeps the UC - # `..` form. The entry name here is the collision-safe dashed form; callers - # shorten it to the leaf via `_normalize_mcp_service_entry_names` once the whole set is known. - return _legacy_mcp_service_entry_name(full_name), build_mcp_service_url( - workspace, full_name - ) + # Agent CLIs (claude/codex/gemini) reject dots in registered names. + # URL keeps the UC `..` form; the entry name is the dashed full name unless + # `service_names` (see `mcp_service_entry_names`) picked another for this run. + entry_name = (service_names or {}).get(full_name) or full_name.replace(".", "-") + return entry_name, build_mcp_service_url(workspace, full_name) if selection.startswith(VECTOR_SEARCH_SELECTION_PREFIX): return _resolve_catalog_schema_selection( @@ -1864,9 +1756,7 @@ def _resolve_location_mcp_servers( location: str, original_servers: list[dict], services: set[str] | None = None, - managed_servers: list[dict] | None = None, - foreign_servers: Iterable[dict] = (), - keep_existing: bool = False, + existing_servers: Iterable[dict] = (), ) -> list[dict]: """Build the desired MCP server list for ``--location .``. @@ -1923,35 +1813,11 @@ def _resolve_location_mcp_servers( ] original_by_name = _servers_by_name(original_servers) - # The location is a strict replacement, so previously-registered MCP services outside it don't - # count as name holders (unless ``keep_existing``: `mcp add` keeps them, so their names stay - # taken); every other entry, the ones for these services (they keep their name), every managed - # server and the developer's native entries (``foreign_servers``) do. - service_prefix = build_mcp_service_url(workspace, "") - location_urls = {build_mcp_service_url(workspace, n) for n in names} - holders = [ - s - for s in original_servers - if keep_existing - or not str(s.get("url") or "").startswith(service_prefix) - or s.get("url") in location_urls - ] - entry_names = mcp_service_entry_names( - names, - workspace, - existing_servers=[*holders, *(managed_servers or []), *foreign_servers], - managed_names=[n for server in managed_servers or [] if (n := _server_name(server))], - ) + entry_names = mcp_service_entry_names(names, workspace, [*original_servers, *existing_servers]) working_servers: list[dict] = [] for full_name in names: entry_name = entry_names[full_name] - # An entry from an older ug carries the dashed name; adopt it so it is renamed, not duplicated. - service_url = build_mcp_service_url(workspace, full_name) - legacy = original_by_name.get(_legacy_mcp_service_entry_name(full_name)) original = original_by_name.get(entry_name) - if original is not None and original.get("url") != service_url: - original = None - original = original or legacy original_clients = list((original or {}).get("clients") or []) merged_clients = original_clients + [c for c in clients if c not in original_clients] candidate = { @@ -2140,16 +2006,19 @@ def _configure_v2_mcp_selectors( managed_servers = managed_mcp_servers(state, set(clients)) managed_names = {name for server in managed_servers if (name := _server_name(server))} - managed_urls = _managed_urls(managed_servers) + service_names = _mcp_service_selection_names( + selectors, workspace, [*original_mcp_servers, *managed_servers] + ) working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() skipped_managed: set[str] = set() for selection in selectors: - entry_name, url = _resolve_mcp_selection(selection, workspace, available_app_servers) + entry_name, url = _resolve_mcp_selection( + selection, workspace, available_app_servers, service_names=service_names + ) if entry_name in working_names: continue - # Managed config may carry this server under its leaf (`exa`), not the dashed name. - if entry_name in managed_names or url in managed_urls: + if entry_name in managed_names: skipped_managed.add(entry_name) continue working_mcp_servers.append( @@ -2164,19 +2033,6 @@ def _configure_v2_mcp_selectors( if append: working_mcp_servers = _union_missing(original_mcp_servers, working_mcp_servers) - working_mcp_servers = _normalize_mcp_service_entry_names( - working_mcp_servers, - workspace, - managed_servers, - _native_mcp_servers(clients, original_mcp_servers), - ) - # After normalizing: a legacy dashed entry the union brought back may now carry a managed name. - working_mcp_servers = _drop_managed_servers(working_mcp_servers, managed_names, managed_urls) - working_names = { - n - for s in working_mcp_servers - if s.get("kind") != SKILLS_MCP_KIND and (n := _server_name(s)) - } changed = apply_mcp_server_changes( original_mcp_servers, @@ -2250,7 +2106,6 @@ def configure_mcp_command( managed_servers = managed_mcp_servers(state, set(clients)) managed_names = {name for server in managed_servers if (name := _server_name(server))} - managed_urls = _managed_urls(managed_servers) original_mcp_servers_for_location: list[dict] = list(state.get("mcp_servers") or []) if location is not None: @@ -2261,21 +2116,13 @@ def configure_mcp_command( location, original_mcp_servers_for_location, services, - managed_servers, - _native_mcp_servers(clients, original_mcp_servers_for_location), - keep_existing=append, + existing_servers=managed_servers, ) + working_mcp_servers = _drop_managed_servers(working_mcp_servers, managed_names) if append: - working_mcp_servers = _normalize_mcp_service_entry_names( - _union_missing(original_mcp_servers_for_location, working_mcp_servers), - workspace, - managed_servers, - _native_mcp_servers(clients, original_mcp_servers_for_location), + working_mcp_servers = _union_missing( + original_mcp_servers_for_location, working_mcp_servers ) - # After normalizing: a legacy dashed entry the union brought back may now carry a managed name. - working_mcp_servers = _drop_managed_servers( - working_mcp_servers, managed_names, managed_urls - ) changed = apply_mcp_server_changes( original_mcp_servers_for_location, working_mcp_servers, @@ -2301,6 +2148,9 @@ def configure_mcp_command( if "apps" in excluded_sources: picker_servers = [s for s in picker_servers if not _is_app_mcp_server(s)] original_by_name = _servers_by_name(picker_servers) + entry_urls = { + n: s.get("url") for s in [*picker_servers, *managed_servers] if (n := _server_name(s)) + } # Single source (MCP services), so there's no "choose sources" step — discover the fast # `system.ai` list, show the picker immediately, and let the workspace-wide walk stream in @@ -2312,9 +2162,7 @@ def configure_mcp_command( set(original_by_name) | managed_names, additive=append, managed_names=managed_names, - entry_urls={ - n: s.get("url") for s in [*picker_servers, *managed_servers] if (n := _server_name(s)) - }, + entry_urls=entry_urls, ) selections = prompt_for_mcp_server_choices( discovered["external"], @@ -2327,7 +2175,7 @@ def configure_mcp_command( additive=append, managed_names=managed_names, background_loader=services_loader, - managed_urls={n: s.get("url") for s in managed_servers if (n := _server_name(s))}, + entry_urls=entry_urls, ) if selections is None or isinstance(selections, _Back): # No back-navigation without the source step; `_Back` can't occur, but keep the guard @@ -2339,6 +2187,11 @@ def configure_mcp_command( available_vector_search_servers = discovered["vector_search"] available_uc_functions_servers = discovered["uc_functions"] + service_names = _mcp_service_selection_names( + [s.removeprefix(MCP_ADD_PREFIX) for s in selections if s.startswith(MCP_ADD_PREFIX)], + workspace, + [*original_mcp_servers, *managed_servers], + ) working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() add_selections: list[str] = [] @@ -2360,6 +2213,7 @@ def configure_mcp_command( available_genie_mcp_servers, available_vector_search_servers, available_uc_functions_servers, + service_names=service_names, ) except RuntimeError as exc: print_warning(f"Skipped MCP selection `{selection}`: {exc}.") @@ -2378,22 +2232,6 @@ def configure_mcp_command( if append: working_mcp_servers = _union_missing(original_mcp_servers, working_mcp_servers) - # Shorten dashed names to the leaf and upgrade legacy dashed entries now the whole set is known. - working_mcp_servers = _normalize_mcp_service_entry_names( - working_mcp_servers, - workspace, - managed_servers, - _native_mcp_servers(clients, original_mcp_servers), - ) - # After normalizing: a legacy dashed entry may now carry a managed name. - working_mcp_servers = _drop_managed_servers( - working_mcp_servers, managed_names, _managed_urls(managed_servers) - ) - working_names = { - n - for s in working_mcp_servers - if s.get("kind") != SKILLS_MCP_KIND and (n := _server_name(s)) - } changed = apply_mcp_server_changes( original_mcp_servers, diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 02f2f9b49..5f218e004 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -9,7 +9,7 @@ import pytest from ucode import mcp -from ucode.agents import claude +from ucode.agents import claude, codex WS = "https://example.databricks.com" CLAUDE_STATE = {"workspace": WS, "available_tools": ["claude"]} @@ -1788,19 +1788,14 @@ def test_existing_entry_gets_reconfigured_for_newly_added_clients(self, monkeypa "configure_client_mcp_server", lambda client, name, url, *a, **kw: configured.append((client, name, url)) or [], ) - monkeypatch.setattr( - mcp, - "remove_client_mcp_server", - lambda client, name, *a, **kw: [], - ) monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) assert mcp.configure_mcp_command(location="system.ai") == 0 - assert sorted(c[0] for c in configured) == ["claude", "codex"] + assert [c[0] for c in configured] == ["claude", "codex"] assert saved_states[-1]["mcp_servers"] == [ { - "name": "github", + "name": "system-ai-github", "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", "auth": "proxy", "clients": ["claude", "codex"], @@ -1889,10 +1884,9 @@ def test_services_subset_keeps_others_in_location(self, monkeypatch): assert mcp.add_mcp_command(location="system.ai", services={"github"}) == 0 - # The legacy dashed `system-ai-slack` is renamed to `slack` in place (old entry removed). - assert removed == [("claude", "system-ai-slack")] + assert removed == [] names = [s["name"] for s in saved_states[-1]["mcp_servers"]] - assert names == ["github", "slack"] + assert names == ["github", "system-ai-slack"] def test_empty_services_is_a_noop(self, monkeypatch): """`mcp add --names ""` has nothing to add, so it's a no-op that never @@ -2239,59 +2233,14 @@ def test_adds_and_removes_to_match_new_selection(self, monkeypatch): == 0 ) - # slack removed, gmail added; github is renamed in place (legacy dashed entry removed, leaf - # entry added). - assert sorted(removed) == [("claude", "system-ai-github"), ("claude", "system-ai-slack")] - assert sorted(c[1] for c in configured) == ["github", "gmail"] + # slack removed, gmail added, github untouched (entry unchanged). + assert removed == [("claude", "system-ai-slack")] + assert [c[1] for c in configured] == ["gmail"] assert sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) == [ - "github", "gmail", + "system-ai-github", ] - def test_failed_rename_is_not_saved_and_the_next_run_recovers(self, monkeypatch): - legacy = { - "name": "system-ai-github", - "url": f"{WS}/ai-gateway/mcp-services/system.ai.github", - "auth": "proxy", - "clients": ["claude"], - } - removed: list[tuple[str, str]] = [] - configured: list[str] = [] - saved_states: list[dict] = [] - fail = [True] - _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [legacy]}) - monkeypatch.setattr( - mcp, "list_mcp_services", lambda workspace, token, parent: (["system.ai.github"], None) - ) - - def configure(client, name, url, *a, **kw): - if fail[0]: - raise RuntimeError("claude mcp add failed") - configured.append(name) - return [] - - monkeypatch.setattr(mcp, "configure_client_mcp_server", configure) - monkeypatch.setattr( - mcp, - "remove_client_mcp_server", - lambda client, name, *a, **kw: removed.append((client, name)) or [], - ) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - # Adding `github` fails after the legacy entry was removed: the error surfaces and ug state - # is not rewritten, so it still lists the legacy entry. - with pytest.raises(RuntimeError, match="claude mcp add failed"): - mcp.configure_mcp_command(location="system.ai") - assert removed == [("claude", "system-ai-github")] - assert saved_states == [] - - # The next run starts from the same state, removes the (already gone) legacy entry again and - # registers `github`, so the rename completes with nothing duplicated. - fail[0] = False - assert mcp.configure_mcp_command(location="system.ai") == 0 - assert configured == ["github"] - assert [s["name"] for s in saved_states[-1]["mcp_servers"]] == ["github"] - def test_full_names_without_location_derive_the_schema(self, monkeypatch): configured: list[tuple[str, str, str, dict]] = [] seen: dict[str, str] = {} @@ -3282,354 +3231,11 @@ def test_drops_foreign_workspace_skills_entry(self, monkeypatch): assert state["mcp_servers"] == [] -class TestMcpServiceEntryNames: - """Entry names are the service leaf (`exa`), falling back to the dashed full name on a clash.""" - - def test_unique_leaf_name_is_used(self): - assert mcp.mcp_service_entry_names(["main.default.exa"], WS) == {"main.default.exa": "exa"} - - def test_colliding_leaf_names_fall_back_to_dashed_full_names(self): - names = mcp.mcp_service_entry_names( - ["main.default.exa", "ml.tools.exa", "main.default.x"], WS - ) - assert names == { - "main.default.exa": "main-default-exa", - "ml.tools.exa": "ml-tools-exa", - "main.default.x": "x", - } - - def test_leaf_held_by_another_server_falls_back(self): - other = {"name": "exa", "url": f"{WS}/api/2.0/mcp/external/exa"} - names = mcp.mcp_service_entry_names(["main.default.exa"], WS, existing_servers=[other]) - assert names == {"main.default.exa": "main-default-exa"} - - def test_existing_entry_for_the_same_service_is_not_a_clash(self): - same = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "main.default.exa")} - names = mcp.mcp_service_entry_names(["main.default.exa"], WS, existing_servers=[same]) - assert names == {"main.default.exa": "exa"} - - def test_normalize_upgrades_legacy_entry_in_place_without_duplicating(self): - legacy = { - "name": "main-default-exa", - "url": mcp.build_mcp_service_url(WS, "main.default.exa"), - "auth": "proxy", - "clients": ["claude"], - } - fresh = {**legacy, "name": "exa", "clients": ["codex"]} - other = {"name": "databricks-sql", "url": f"{WS}/api/2.0/mcp/sql", "clients": ["claude"]} - assert mcp._normalize_mcp_service_entry_names([other, legacy], WS) == [ - other, - {**legacy, "name": "exa"}, - ] - merged = mcp._normalize_mcp_service_entry_names([legacy, fresh], WS) - assert merged == [{**legacy, "name": "exa", "clients": ["claude", "codex"]}] - - def test_location_rerun_renames_legacy_entry_to_leaf(self, monkeypatch): - """Upgrade: an existing `main-default-exa` entry becomes `exa`; the old one is removed - from the agent and nothing is duplicated or orphaned.""" - legacy = { - "name": "main-default-exa", - "url": mcp.build_mcp_service_url(WS, "main.default.exa"), - "auth": "proxy", - "clients": ["claude"], - } - saved_states: list[dict] = [] - configured: list[tuple[str, str]] = [] - removed: list[tuple[str, str]] = [] - _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [legacy]}) - monkeypatch.setattr( - mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) - ) - monkeypatch.setattr( - mcp, - "configure_client_mcp_server", - lambda client, name, url, *a, **kw: configured.append((client, name)) or [], - ) - monkeypatch.setattr( - mcp, - "remove_client_mcp_server", - lambda client, name, *a, **kw: removed.append((client, name)) or [], - ) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.configure_mcp_command(location="main.default") == 0 - - assert configured == [("claude", "exa")] - assert removed == [("claude", "main-default-exa")] - assert saved_states[-1]["mcp_servers"] == [{**legacy, "name": "exa"}] - - def test_assigned_leaf_name_is_sticky_when_a_colliding_service_arrives(self): - held = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "ml.tools.exa")} - names = mcp.mcp_service_entry_names( - ["main.default.exa", "ml.tools.exa"], WS, existing_servers=[held] - ) - # `ml.tools.exa` keeps `exa`; the newcomer gets the dashed name. Neither is renamed. - assert names == {"ml.tools.exa": "exa", "main.default.exa": "main-default-exa"} - - def test_add_keeps_the_existing_leaf_and_dashes_the_newcomer(self, monkeypatch): - existing = { - "name": "exa", - "url": mcp.build_mcp_service_url(WS, "ml.tools.exa"), - "auth": "proxy", - "clients": ["claude"], - } - saved_states: list[dict] = [] - configured: list[tuple[str, str]] = [] - removed: list[tuple[str, str]] = [] - _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [existing]}) - monkeypatch.setattr( - mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) - ) - monkeypatch.setattr( - mcp, - "configure_client_mcp_server", - lambda client, name, url, *a, **kw: configured.append((client, name)) or [], - ) - monkeypatch.setattr( - mcp, - "remove_client_mcp_server", - lambda client, name, *a, **kw: removed.append((client, name)) or [], - ) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.add_mcp_command(location="main.default", services={"main.default.exa"}) == 0 - - assert removed == [] - assert configured == [("claude", "main-default-exa")] - assert sorted(s["name"] for s in saved_states[-1]["mcp_servers"]) == [ - "exa", - "main-default-exa", - ] - - def test_unrelated_entry_with_the_leaf_name_is_not_this_service(self): - # In ug state or the developer's config, an `exa` for another server is not `main.default.exa`. - other = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "other.schema.exa")} - hand_named = {"name": "exa"} - for held in (other, hand_named): - names = mcp.mcp_service_entry_names(["main.default.exa"], WS, existing_servers=[held]) - assert names == {"main.default.exa": "main-default-exa"} - - def test_normalize_keeps_a_leaf_held_by_a_native_entry_for_another_server(self): - legacy = { - "name": "main-default-exa", - "url": mcp.build_mcp_service_url(WS, "main.default.exa"), - "clients": ["claude"], - } - native = {"name": "exa", "url": "https://other.example.com/mcp"} - out = mcp._normalize_mcp_service_entry_names([legacy], WS, foreign_servers=[native]) - assert out == [legacy] - - def test_location_leaves_a_developers_native_entry_alone(self, monkeypatch): - """A hand-added `github` in the developer's Claude config is never overwritten.""" - configured: list[str] = [] - _stub_location_base(monkeypatch, {**CLAUDE_STATE}) - monkeypatch.setattr( - mcp.claude, "read_user_mcp_urls", lambda: {"github": "https://example.com/my-github"} - ) - monkeypatch.setattr( - mcp, "list_mcp_services", lambda ws, token, parent: (["system.ai.github"], None) - ) - monkeypatch.setattr( - mcp, - "configure_client_mcp_server", - lambda client, name, url, *a, **kw: configured.append(name) or [], - ) - monkeypatch.setattr(mcp, "save_state", lambda state: None) - - assert mcp.configure_mcp_command(location="system.ai") == 0 - - assert configured == ["system-ai-github"] - - def test_location_takes_the_leaf_when_the_native_entry_is_the_same_service(self, monkeypatch): - configured: list[str] = [] - _stub_location_base(monkeypatch, {**CLAUDE_STATE}) - same = mcp.build_mcp_service_url(WS, "system.ai.github") - monkeypatch.setattr(mcp.claude, "read_user_mcp_urls", lambda: {"github": same}) - monkeypatch.setattr( - mcp, "list_mcp_services", lambda ws, token, parent: (["system.ai.github"], None) - ) - monkeypatch.setattr( - mcp, - "configure_client_mcp_server", - lambda client, name, url, *a, **kw: configured.append(name) or [], - ) - monkeypatch.setattr(mcp, "save_state", lambda state: None) - - assert mcp.configure_mcp_command(location="system.ai") == 0 - - assert configured == ["github"] - - def test_native_servers_skip_ug_owned_names_and_read_both_clients(self, monkeypatch): - monkeypatch.setattr( - mcp.claude, "read_user_mcp_urls", lambda: {"mine": "u1", "ug-owned": "u2", "bare": ""} - ) - monkeypatch.setattr(mcp.codex, "read_user_mcp_urls", lambda: {"mine": "u3"}) - native = mcp._native_mcp_servers(["claude", "codex", "gemini"], [{"name": "ug-owned"}]) - assert native == [ - {"name": "mine", "url": "u1"}, - {"name": "bare", "url": None}, - {"name": "mine", "url": "u3"}, - ] - - def test_picker_ignores_an_unrelated_entry_with_the_leaf_name(self): - other = {"exa": mcp.build_mcp_service_url(WS, "other.schema.exa")} - mine = {"exa": mcp.build_mcp_service_url(WS, "main.default.exa")} - # Under `mcp add` an unrelated `exa` must not hide the service; its own entry still does. - shown = mcp._mcp_service_choice("main.default.exa", {"exa"}, True, None, other) - assert shown is not None and shown.value.startswith(mcp.MCP_ADD_PREFIX) - assert mcp._mcp_service_choice("main.default.exa", {"exa"}, True, None, mine) is None - # In replace mode the unrelated `exa` is no toggle for this service either. - replace = mcp._mcp_service_choice("main.default.exa", {"exa"}, False, None, other) - assert replace is not None and replace.value.startswith(mcp.MCP_ADD_PREFIX) - # A managed `exa` for another server doesn't make this service "managed". - assert mcp._mcp_service_choice("main.default.exa", set(), True, {"exa"}, other) is not None - assert mcp._mcp_service_choice("main.default.exa", set(), True, {"exa"}, mine) is None - - def test_any_holder_with_a_different_url_makes_the_leaf_a_clash(self): - # Claude's hand-added `github` and Codex's `github` for this service share a name. - mine = {"name": "github", "url": mcp.build_mcp_service_url(WS, "system.ai.github")} - hand = {"name": "github", "url": "https://example.com/my-github"} - for order in ([mine, hand], [hand, mine]): - names = mcp.mcp_service_entry_names(["system.ai.github"], WS, existing_servers=order) - assert names == {"system.ai.github": "system-ai-github"} - same = mcp.mcp_service_entry_names(["system.ai.github"], WS, existing_servers=[mine, mine]) - assert same == {"system.ai.github": "github"} - - def test_picker_keeps_an_unrelated_leaf_entry_as_its_own_toggle(self): - other = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "other.schema.exa")} - choices = mcp.build_mcp_picker_choices( - [], [], [], [other], available_mcp_service_names=["main.default.exa"] - ) - values = [c.value for c in choices] - assert f"{mcp.MCP_ADD_PREFIX}{mcp.MCP_SERVICE_SELECTION_PREFIX}main.default.exa" in values - # The user's unrelated `exa` is still listed (so replace mode does not drop it). - assert "exa" in values - - def test_picker_does_not_repeat_a_service_that_holds_its_leaf(self): - mine = {"name": "exa", "url": mcp.build_mcp_service_url(WS, "main.default.exa")} - choices = mcp.build_mcp_picker_choices( - [], [], [], [mine], available_mcp_service_names=["main.default.exa"] - ) - assert [c.value for c in choices] == ["exa"] - - def test_managed_entry_reuses_the_name_of_a_same_url_user_entry(self, monkeypatch): - monkeypatch.setattr( - mcp, - "_resolve_location_mcp_servers", - lambda *a, **k: pytest.fail("must not discover for explicit FQN names"), - ) - legacy = { - "name": "system-ai-github", - "url": mcp.build_mcp_service_url(WS, "system.ai.github"), - } - out = mcp._resolve_managed_mcp_servers( - {"names": ["system.ai.github"]}, WS, None, ["claude"], [legacy] - ) - assert [s["name"] for s in out] == ["system-ai-github"] - - def test_reconfigure_does_not_inherit_clients_of_an_unrelated_entry(self, monkeypatch): - monkeypatch.setattr( - mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) - ) - monkeypatch.setattr(mcp, "get_databricks_token", lambda *a, **k: "t") - unrelated = { - "name": "exa", - "url": mcp.build_mcp_service_url(WS, "ml.tools.exa"), - "auth": "proxy", - "clients": ["gemini"], - } - out = mcp._resolve_location_mcp_servers(WS, None, ["claude"], "main.default", [unrelated]) - mine = [s for s in out if s["url"].endswith("main.default.exa")] - assert [(s["name"], s["clients"]) for s in mine] == [("exa", ["claude"])] - - def test_drop_managed_matches_a_dashed_entry_by_url(self): - url = mcp.build_mcp_service_url(WS, "main.default.exa") - servers = [{"name": "main-default-exa", "url": url}, {"name": "keep", "url": "k"}] - kept = mcp._drop_managed_servers(servers, {"exa"}, {url}) - assert [s["name"] for s in kept] == ["keep"] - - def test_legacy_entry_renamed_onto_a_managed_name_is_dropped_not_duplicated(self, monkeypatch): - """`mcp add` unions the legacy `system-ai-github` back; once renamed to the managed - `github` it must be dropped (and removed from the agent), not registered twice.""" - legacy = { - "name": "system-ai-github", - "url": mcp.build_mcp_service_url(WS, "system.ai.github"), - "auth": "proxy", - "clients": ["claude"], - } - saved_states: list[dict] = [] - configured: list[str] = [] - removed: list[tuple[str, str]] = [] - _stub_location_base(monkeypatch, {**CLAUDE_STATE, "mcp_servers": [legacy]}) - monkeypatch.setattr(mcp.claude, "read_managed_mcp_urls", lambda: {"github": legacy["url"]}) - monkeypatch.setattr(mcp.codex, "read_managed_mcp_urls", lambda: {}) - monkeypatch.setattr( - mcp, "list_mcp_services", lambda ws, token, parent: (["system.ai.slack"], None) - ) - monkeypatch.setattr( - mcp, - "configure_client_mcp_server", - lambda client, name, url, *a, **kw: configured.append(name) or [], - ) - monkeypatch.setattr( - mcp, - "remove_client_mcp_server", - lambda client, name, *a, **kw: removed.append((client, name)) or [], - ) - monkeypatch.setattr(mcp, "save_state", lambda state: saved_states.append(state.copy())) - - assert mcp.add_mcp_command(location="system.ai") == 0 - - assert configured == ["slack"] - assert removed == [("claude", "system-ai-github")] - assert [s["name"] for s in saved_states[-1]["mcp_servers"]] == ["slack"] - - -class TestNativeMcpConfigReaders: - def test_claude_reads_http_and_proxy_urls(self, tmp_path): - mcp.claude.claude_mcp_config_path().write_text( - json.dumps( - { - "mcpServers": { - "http": {"type": "http", "url": "https://a.example.com/mcp"}, - "proxy": {"type": "stdio", "command": "ug", "args": ["--url", "u2"]}, - "own": {"type": "stdio", "command": "node", "args": ["x.js"]}, - } - } - ) - ) - assert mcp.claude.read_user_mcp_urls() == { - "http": "https://a.example.com/mcp", - "proxy": "u2", - "own": "", - } - - def test_codex_reads_http_and_proxy_urls(self): - path = mcp.codex.user_mcp_config_path() - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text( - '[mcp_servers.http]\nurl = "https://a.example.com/mcp"\n' - '[mcp_servers.proxy]\ncommand = "ug"\nargs = ["--url", "u2"]\n' - '[mcp_servers.own]\ncommand = "node"\n' - ) - assert mcp.codex.read_user_mcp_urls() == { - "http": "https://a.example.com/mcp", - "proxy": "u2", - "own": "", - } - - def test_missing_or_unparseable_configs_read_as_empty(self): - assert mcp.claude.read_user_mcp_urls() == {} - assert mcp.codex.read_user_mcp_urls() == {} - mcp.claude.claude_mcp_config_path().write_text("{not json") - assert mcp.claude.read_user_mcp_urls() == {} - - class TestResolveManagedMcpServers: def test_location_registers_the_whole_schema(self, monkeypatch): captured: dict = {} - def fake_loc(ws, profile, clients, location, original, services=None, *a, **kw): + def fake_loc(ws, profile, clients, location, original, services=None, existing_servers=()): captured.update(location=location, services=services, original=original) return [{"name": "system-ai-a", "url": "u", "auth": "proxy", "clients": clients}] @@ -3676,39 +3282,6 @@ def test_duplicate_names_collapse_to_one_entry(self, monkeypatch): ) assert [s["name"] for s in out] == ["github"] - def test_names_avoid_entries_the_developer_already_has(self, monkeypatch): - # A developer's own `github` (e.g. an external connection) must not be overwritten by the - # managed fallback; the same service already registered as `github` is not a clash. - monkeypatch.setattr( - mcp, - "_resolve_location_mcp_servers", - lambda *a, **k: pytest.fail("must not discover for explicit FQN names"), - ) - mine = {"name": "github", "url": f"{WS}/api/2.0/mcp/external/github"} - out = mcp._resolve_managed_mcp_servers( - {"names": ["system.ai.github", "system.ai.jira"]}, WS, None, ["claude"], [mine] - ) - assert [s["name"] for s in out] == ["system-ai-github", "jira"] - same = {"name": "github", "url": mcp.build_mcp_service_url(WS, "system.ai.github")} - out = mcp._resolve_managed_mcp_servers( - {"names": ["system.ai.github"]}, WS, None, ["claude"], [same] - ) - assert [s["name"] for s in out] == ["github"] - - def test_location_selector_passes_the_developers_entries_for_clash_checks(self, monkeypatch): - seen: dict = {} - - def fake_loc(ws, profile, clients, location, original, services=None, *a, **kw): - seen.update(kw) - return [] - - monkeypatch.setattr(mcp, "_resolve_location_mcp_servers", fake_loc) - mine = {"name": "exa", "url": "https://example.com/exa"} - mcp._resolve_managed_mcp_servers( - {"unity_catalog_location": "system.ai"}, WS, None, ["claude"], [mine] - ) - assert list(seen["foreign_servers"]) == [mine] - def test_malformed_names_are_skipped_with_a_warning(self, monkeypatch): # A bare (`github`) or over-qualified (`a.b.c.d`) name isn't a full FQN and can't be # located under a single `.`. Skip it with a warning rather than failing, @@ -3760,9 +3333,7 @@ def fake_setup(s, section, **kw): monkeypatch.setattr( mcp, "_resolve_managed_mcp_servers", - lambda selector, ws, profile, clients, *a, **kw: [ - dict(s, clients=clients) for s in resolved - ], + lambda selector, ws, profile, clients, *_: [dict(s, clients=clients) for s in resolved], ) monkeypatch.setattr( mcp, @@ -3815,7 +3386,7 @@ def test_location_config_registers(self, monkeypatch): monkeypatch.setattr( mcp, "_resolve_managed_mcp_servers", - lambda selector, ws, profile, clients, *a, **kw: ( + lambda selector, ws, profile, clients, *_: ( seen_selector.update(selector) or [dict(s, clients=clients) for s in [{"name": "system-ai-a", "url": "u"}]] ), @@ -3826,37 +3397,6 @@ def test_location_config_registers(self, monkeypatch): assert seen_selector == {"unity_catalog_location": "system.ai"} assert [s["name"] for s in out] == ["system-ai-a"] - def test_reconcile_names_managed_servers_around_the_developers_entries(self, monkeypatch): - own = {"name": "exa", "url": "https://example.com/exa", "clients": ["claude"]} - state = {"workspace": WS, "profile": None, "managed_mcp_servers": [], "mcp_servers": [own]} - applied: dict = {} - self._wire( - monkeypatch, - state=state, - configured=["claude"], - resolved=[], - applied=applied, - saved={}, - setup_calls=[], - ) - monkeypatch.setattr( - mcp.claude, "read_user_mcp_urls", lambda: {"exa": "x", "hand": "https://h.example.com"} - ) - seen: dict = {} - - def fake_resolve(selector, ws, profile, clients, existing=()): - seen["existing"] = list(existing) - return [] - - monkeypatch.setattr(mcp, "_resolve_managed_mcp_servers", fake_resolve) - - mcp.reconcile_managed_mcp_servers( - {"mcp_servers": {"names": ["main.tools.exa"]}}, {"claude"} - ) - - # The ug entry, plus the native entry ug doesn't own (`exa` is ug's, so it is skipped). - assert seen["existing"] == [own, {"name": "hand", "url": "https://h.example.com"}] - def test_reconcile_removes_servers_the_admin_dropped(self, monkeypatch): # A config that no longer lists any MCP server clears the previously-registered managed set. previous = [{"name": "old", "url": "u", "auth": "proxy", "clients": ["claude"]}] @@ -4391,7 +3931,10 @@ def test_location_add_skips_an_already_managed_service(self, monkeypatch, capsys configured: list[str] = [] saved_states: list[dict] = [] _stub_location_base(monkeypatch, {**CLAUDE_STATE}) - self._stub_managed_files(monkeypatch, claude_urls={"system-ai-github": "managed-url"}) + self._stub_managed_files( + monkeypatch, + claude_urls={"system-ai-github": f"{WS}/ai-gateway/mcp-services/system.ai.github"}, + ) monkeypatch.setattr( mcp, "list_mcp_services", @@ -4823,3 +4366,129 @@ def test_setup_mcp_clients_user_messages_name_ug_not_ucode(monkeypatch, capsys): mcp.setup_mcp_clients(state, "Add MCP Servers", require_auth=False, agents={"codex"}) with pytest.raises(RuntimeError, match="Run `ug configure` first"): mcp.setup_mcp_clients({}, "Add MCP Servers") + + +class TestMcpServiceEntryNames: + """A service gets its short name only when it is completely free; nothing is ever renamed.""" + + @staticmethod + def _url(full_name): + return mcp.build_mcp_service_url(WS, full_name) + + def _run(self, monkeypatch, services, state_extra=None, location="main.default"): + """Run `ug mcp add --location` and return ({name: url} configured, saved servers).""" + state = {**CLAUDE_STATE, **(state_extra or {})} + saved: list[dict] = [] + configured: dict[str, str] = {} + _stub_location_base(monkeypatch, state) + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (sorted(services), None) + ) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.update({name: url}) or [], + ) + monkeypatch.setattr(mcp, "remove_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda s: saved.append(s.copy())) + assert mcp.add_mcp_command(location=location, services=set(services)) == 0 + return configured, (saved[-1]["mcp_servers"] if saved else state.get("mcp_servers")) + + def test_free_leaf_gets_the_short_name(self, monkeypatch): + configured, _ = self._run(monkeypatch, {"main.default.exa", "main.default.github"}) + assert sorted(configured) == ["exa", "github"] + + def test_leaf_taken_in_ug_state_gets_the_dashed_name(self, monkeypatch): + held = {"name": "exa", "url": self._url("ml.tools.exa"), "auth": "proxy"} + held["clients"] = ["claude"] + configured, servers = self._run(monkeypatch, {"main.default.exa"}, {"mcp_servers": [held]}) + assert list(configured) == ["main-default-exa"] + assert held in servers + + def test_leaf_taken_by_a_managed_entry_gets_the_dashed_name(self, monkeypatch): + managed = {"name": "exa", "url": f"{WS}/api/2.0/mcp/external/exa", "clients": ["claude"]} + configured, _ = self._run( + monkeypatch, {"main.default.exa"}, {"managed_mcp_servers": [managed]} + ) + assert list(configured) == ["main-default-exa"] + + def test_same_service_already_managed_is_skipped_not_duplicated(self, monkeypatch): + managed = {"name": "exa", "url": self._url("main.default.exa"), "clients": ["claude"]} + configured, _ = self._run( + monkeypatch, {"main.default.exa"}, {"managed_mcp_servers": [managed]} + ) + assert configured == {} + + def test_leaf_in_native_claude_config_gets_the_dashed_name(self, monkeypatch): + # Any URL counts, even the same service registered by hand. + path = claude.claude_mcp_config_path() + path.write_text(json.dumps({"mcpServers": {"exa": {"url": self._url("main.default.exa")}}})) + configured, _ = self._run(monkeypatch, {"main.default.exa"}) + assert list(configured) == ["main-default-exa"] + + def test_leaf_in_native_codex_config_blocks_a_claude_only_run(self, monkeypatch): + codex.user_mcp_config_path().parent.mkdir(parents=True, exist_ok=True) + codex.user_mcp_config_path().write_text('[mcp_servers.exa]\nurl = "https://x.test/mcp"\n') + configured, _ = self._run(monkeypatch, {"main.default.exa", "main.default.github"}) + assert sorted(configured) == ["github", "main-default-exa"] + + def test_unreadable_native_config_makes_every_name_dashed(self, monkeypatch): + claude.claude_mcp_config_path().write_text("{not json") + configured, _ = self._run(monkeypatch, {"main.default.exa"}) + assert list(configured) == ["main-default-exa"] + + def test_services_sharing_a_leaf_in_one_run_both_get_dashed_names(self, monkeypatch): + names = mcp.mcp_service_entry_names( + ["main.default.exa", "ml.tools.exa", "main.default.x"], WS + ) + assert names == { + "main.default.exa": "main-default-exa", + "ml.tools.exa": "ml-tools-exa", + "main.default.x": "x", + } + + def test_rerun_never_renames_existing_entries(self, monkeypatch): + dashed = { + "name": "main-default-exa", + "url": self._url("main.default.exa"), + "auth": "proxy", + "clients": ["claude"], + } + short = {**dashed, "name": "github", "url": self._url("main.default.github")} + configured, servers = self._run( + monkeypatch, + {"main.default.exa", "main.default.github"}, + {"mcp_servers": [dashed, short]}, + ) + assert sorted(s["name"] for s in servers) == ["github", "main-default-exa"] + assert set(configured) <= {"github", "main-default-exa"} + + def test_managed_location_services_use_the_same_rule(self, monkeypatch): + monkeypatch.setattr(mcp, "get_databricks_token", lambda workspace, profile=None: "token") + monkeypatch.setattr( + mcp, "list_mcp_services", lambda ws, token, parent: (["main.default.exa"], None) + ) + working = mcp._resolve_managed_mcp_servers( + {"unity_catalog_location": "main.default"}, WS, None, ["claude"] + ) + assert [s["name"] for s in working] == ["exa"] + held = {"name": "exa", "url": f"{WS}/api/2.0/mcp/external/exa"} + working = mcp._resolve_managed_mcp_servers( + {"unity_catalog_location": "main.default"}, WS, None, ["claude"], [held] + ) + assert [s["name"] for s in working] == ["main-default-exa"] + + def test_picker_finds_a_service_registered_under_its_short_name(self): + urls = {"exa": self._url("main.default.exa")} + choice = mcp._mcp_service_choice( + "main.default.exa", {"exa"}, additive=False, entry_urls=urls + ) + assert choice.value == "exa" and choice.checked + assert mcp._mcp_service_choice("main.default.exa", {"exa"}, True, entry_urls=urls) is None + + def test_picker_ignores_an_unrelated_entry_with_the_leaf_name(self): + urls = {"exa": self._url("other.schema.exa")} + choice = mcp._mcp_service_choice( + "main.default.exa", {"exa"}, additive=False, entry_urls=urls + ) + assert choice.value.startswith(mcp.MCP_ADD_PREFIX) From 5804e060c3a44fe3ef44ef3d6967fed15833d997 Mon Sep 17 00:00:00 2001 From: Achille Negrier Date: Sat, 17 Oct 2026 14:58:47 +0000 Subject: [PATCH 6/6] Keep existing managed names; avoid same-run name collisions Co-authored-by: Isaac --- src/ucode/mcp.py | 31 +++++++++++++++++++++++++-- tests/test_mcp.py | 53 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/src/ucode/mcp.py b/src/ucode/mcp.py index 6c7e07efd..ac00a1fde 100644 --- a/src/ucode/mcp.py +++ b/src/ucode/mcp.py @@ -1172,7 +1172,12 @@ def reconcile_managed_mcp_servers(managed: dict, agents: set[str]) -> list[dict] state, "Managed MCP Servers", agents=scope, action_note="Registering for" ) working = _resolve_managed_mcp_servers( - selector, workspace, profile, clients, [*(state.get("mcp_servers") or []), *previous] + selector, + workspace, + profile, + clients, + # Managed entries first (file + fallback), so an entry already there keeps its name. + [*managed_mcp_servers(state), *(state.get("mcp_servers") or [])], ) else: # No MCP client configured: nothing to register, but still clear what a prior configure left. @@ -2187,10 +2192,32 @@ def configure_mcp_command( available_vector_search_servers = discovered["vector_search"] available_uc_functions_servers = discovered["uc_functions"] + # Names other selections in this run will hold (kept entries, and new non-service adds), so a + # service's leaf never collides with one of them. + run_names: list[dict] = [] + for selection in selections: + if not selection.startswith(MCP_ADD_PREFIX): + run_names.append({"name": selection}) + elif not selection.removeprefix(MCP_ADD_PREFIX).startswith(MCP_SERVICE_SELECTION_PREFIX): + try: + run_names.append( + { + "name": _resolve_mcp_selection( + selection.removeprefix(MCP_ADD_PREFIX), + workspace, + discovered["apps"], + discovered["genie"], + discovered["vector_search"], + discovered["uc_functions"], + )[0] + } + ) + except RuntimeError: + pass # reported when the selection is resolved below service_names = _mcp_service_selection_names( [s.removeprefix(MCP_ADD_PREFIX) for s in selections if s.startswith(MCP_ADD_PREFIX)], workspace, - [*original_mcp_servers, *managed_servers], + [*original_mcp_servers, *managed_servers, *run_names], ) working_mcp_servers: list[dict] = list(skills_servers) working_names: set[str] = set() diff --git a/tests/test_mcp.py b/tests/test_mcp.py index 5f218e004..b5ae9a6d3 100644 --- a/tests/test_mcp.py +++ b/tests/test_mcp.py @@ -3397,6 +3397,32 @@ def test_location_config_registers(self, monkeypatch): assert seen_selector == {"unity_catalog_location": "system.ai"} assert [s["name"] for s in out] == ["system-ai-a"] + def test_existing_managed_file_entry_keeps_its_name_on_upgrade(self, monkeypatch): + url = mcp.build_mcp_service_url(WS, "system.ai.github") + state = {"workspace": WS, "profile": None, "managed_mcp_servers": []} + applied: dict = {} + real_resolve = mcp._resolve_managed_mcp_servers + self._wire( + monkeypatch, + state=state, + configured=["claude"], + resolved=[], + applied=applied, + saved={}, + setup_calls=[], + ) + monkeypatch.setattr(mcp, "_resolve_managed_mcp_servers", real_resolve) + monkeypatch.setattr(mcp, "_native_mcp_names", lambda: set()) + monkeypatch.setattr(mcp.claude, "read_managed_mcp_urls", lambda: {"system-ai-github": url}) + monkeypatch.setattr(mcp.codex, "read_managed_mcp_urls", lambda: {}) + monkeypatch.setattr(mcp.claude, "managed_mcp_uses_managed_file", lambda *a, **k: False) + monkeypatch.setattr(mcp.claude, "reconcile_managed_mcp", lambda *a, **k: True) + monkeypatch.setattr(mcp.codex, "reconcile_managed_mcp", lambda *a, **k: True) + out = mcp.reconcile_managed_mcp_servers( + {"mcp_servers": {"names": ["system.ai.github"]}}, {"claude"} + ) + assert [s["name"] for s in out] == ["system-ai-github"] + def test_reconcile_removes_servers_the_admin_dropped(self, monkeypatch): # A config that no longer lists any MCP server clears the previously-registered managed set. previous = [{"name": "old", "url": "u", "auth": "proxy", "clients": ["claude"]}] @@ -4478,6 +4504,33 @@ def test_managed_location_services_use_the_same_rule(self, monkeypatch): ) assert [s["name"] for s in working] == ["main-default-exa"] + def test_leaf_held_by_another_selection_in_the_run_gets_the_dashed_name(self, monkeypatch): + # An external connection named `github` and `system.ai.github` picked together: both would + # resolve to `github`, so the service must take the dashed name and neither is dropped. + _stub_location_base(monkeypatch, {**CLAUDE_STATE}) + configured: dict[str, str] = {} + saved: list[dict] = [] + monkeypatch.setattr(mcp, "ensure_databricks_auth", lambda workspace, profile=None: None) + monkeypatch.setattr(mcp, "discover_mcp_service_names", lambda workspace, profile=None: []) + monkeypatch.setattr( + mcp, + "prompt_for_mcp_server_choices", + lambda *a, **kw: [ + f"{mcp.MCP_ADD_PREFIX}{mcp.EXTERNAL_MCP_SELECTION_PREFIX}github", + f"{mcp.MCP_ADD_PREFIX}{mcp.MCP_SERVICE_SELECTION_PREFIX}system.ai.github", + ], + ) + monkeypatch.setattr( + mcp, + "configure_client_mcp_server", + lambda client, name, url, *a, **kw: configured.update({name: url}) or [], + ) + monkeypatch.setattr(mcp, "remove_client_mcp_server", lambda *a, **kw: []) + monkeypatch.setattr(mcp, "save_state", lambda s: saved.append(s.copy())) + assert mcp.configure_mcp_command() == 0 + assert sorted(configured) == ["github", "system-ai-github"] + assert sorted(s["name"] for s in saved[-1]["mcp_servers"]) == ["github", "system-ai-github"] + def test_picker_finds_a_service_registered_under_its_short_name(self): urls = {"exa": self._url("main.default.exa")} choice = mcp._mcp_service_choice(