Repository navigation
Name MCP service client entries by their short name - #1160
nachill-databricks wants to merge 8 commits into
Conversation
…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 <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
|
/ug-review |
|
🔍 OpenCodeReview found 9 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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) |
There was a problem hiding this comment.
Matching on the leaf alone means other.schema.exa, or any unrelated server named exa, counts as already registered once main.default.exa is exa. Under mcp add it gets hidden; in replace mode it becomes a duplicate exa toggle. Can we check the URL as well?
| 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] |
There was a problem hiding this comment.
what happens if the dev already has their own exa (hand-named, or other.schema.exa)? managed fallback now registers main.default.exa as exa too, overwriting it, and a later managed drop removes it. can we pass state["mcp_servers"] as existing_servers?
| working_mcp_servers = _normalize_mcp_service_entry_names( | ||
| _union_missing(original_mcp_servers_for_location, working_mcp_servers), | ||
| workspace, | ||
| managed_servers, | ||
| ) |
There was a problem hiding this comment.
this runs after _drop_managed_servers, so a legacy main-default-exa brought back by the union gets renamed to exa, the same name as the managed entry (same URL means no clash). can we drop managed after normalizing?
| # 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")] |
There was a problem hiding this comment.
the comment above still says "github untouched (entry unchanged)", but github is now removed and re-added. can we update it? also, can we cover the partial-failure case where configuring github fails after system-ai-github was removed?
| def test_leaf_held_by_another_server_falls_back(self): | ||
| other = {"name": "exa", "url": f"{WS}/api/2.0/mcp/external/exa"} |
There was a problem hiding this comment.
what happens if the user hand-added github in ~/.claude.json or codex config? clash check reads only ug state, and configure_client_mcp_server removes and re-adds by name, so their entry gets overwritten. can we check native configs too and add a test?
| monkeypatch.setattr( | ||
| mcp, | ||
| "remove_client_mcp_server", | ||
| lambda client, name: removed.append((client, name)) or [], |
There was a problem hiding this comment.
can we use *a, **kw like the other stubs? if the call signature grows, this stub will raise TypeError.
| 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"] |
There was a problem hiding this comment.
this pins down name churn: adding ml.tools.exa later renames an existing exa back to main-default-exa, which breaks names users/admins already reference. can a name stay put once it's assigned?
| 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"} |
There was a problem hiding this comment.
_resolve_managed_mcp_servers names by leaf without checking the developer's ug mcp_servers. what happens if they already have a github (e.g. an external connection)? on the user-scope fallback, managed github overwrites theirs. can we add a clash check and a test for it?
…tries Co-authored-by: Isaac <no-reply@databricks.com>
|
/ug-review |
| 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] = "" |
There was a problem hiding this comment.
can we pull this url/--url extraction into one shared helper? it's copied verbatim in claude.py, and read_managed_mcp_urls above already parses --url its own way, so the copies can drift apart.
Suggestion:
| 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) # shared helper: entry url, else value after --url, else "" |
| url_by_name = { | ||
| name: server.get("url") for server in existing_servers if (name := _server_name(server)) | ||
| } |
There was a problem hiding this comment.
what happens if two holders share a name? _native_mcp_servers returns github for both claude (hand URL) and codex (this service URL). The last one wins, so owns_leaf is True and ug overwrites the hand-added Claude github. can we treat it as a clash if any holder's URL differs?
| push(_mcp_service_choice(name, known_names, additive, managed)) | ||
| displayed_names.add(name.replace(".", "-")) | ||
| push(_mcp_service_choice(name, known_names, additive, managed, entry_urls)) | ||
| displayed_names.update((name.rsplit(".", 1)[-1], _legacy_mcp_service_entry_name(name))) |
There was a problem hiding this comment.
what happens if ug state has an unrelated exa (e.g. other.schema.exa, or a hand-named one) and main.default.exa is listed? _mcp_service_choice shows an add row because the URLs don't match, but exa still lands in displayed_names. Its fallback toggle at L934 is then skipped, and in replace mode the unshown entry counts as unselected, so the user's server gets removed. can we only mark the candidate this service actually holds, e.g. return registered_as from the helper?
| # 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, existing_servers=existing_servers) |
There was a problem hiding this comment.
what happens to a user whose mcp_servers still has the legacy system-ai-github (same URL as the managed entry)? Before, the managed entry shared that name and took its place. Now reconcile registers the managed github next to it, so the service runs twice until they rerun ug configure mcp. can reconcile reuse the name of a same-URL user entry, or drop that entry?
| original = original_by_name.get(entry_name) or original_by_name.get( | ||
| _legacy_mcp_service_entry_name(full_name) | ||
| ) |
There was a problem hiding this comment.
can we check the URL before adopting original? In replace mode, holders skip service entries outside the location. So state exa → ml.tools.exa lets main.default.exa take exa, and this inherits the unrelated entry's clients, registering it for agents the user never picked.
Suggestion:
| original = original_by_name.get(entry_name) or original_by_name.get( | |
| _legacy_mcp_service_entry_name(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") != build_mcp_service_url(workspace, full_name): | |
| original = None | |
| original = original or legacy |
Polly AI ReviewBlocking issues
Security vulnerabilitiesThe first two blockers weaken configuration ownership by overwriting unrelated native entries. No separate injection, authentication bypass, credential exposure, or unsafe-deserialization issue was established. Non-blocking notes
ApproachCentral naming allocation is appropriate, but it needs one batch plan that preserves client-specific ownership, reserves both leaf and fallback names, and merges entries only when their service URLs match. SummaryDiff size (computed): 5 files; +771 / -64 lines (835 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe change introduces short service names, collision fallback, legacy migration, managed-entry handling, and Claude/Codex native-config readers. The migration and ownership requirements justify more than a simple naming substitution. Most implementation work supports those requirements. A single allocation plan would reduce repeated ownership checks and prevent the conflicting decisions identified above. The shared-fixture changes support isolation of the new filesystem reads. No documentation, dependency, or generated-file changes are included. TestsTest-by-test assessmentThe frozen snapshot checksum was verified, and both trusted review-policy files were read in full. Focused offline replays of reconstructed changed functions supported the five blockers; external effects were stubbed. These were not complete CLI executions. No pytest cases or lint gates were independently executed, so “253 passed” remains author-reported. Execution limitation: dependency probes inadvertently used “Component” below means in-process configuration orchestration with registration effects stubbed.
No test functions were removed. Existing coverage remains useful, but it does not catch the demonstrated blockers. The smallest suitable regression additions are:
These checks belong at unit/component layers; another E2E naming matrix is unnecessary. For manual confirmation in disposable configuration, register one legacy service in Claude and Codex, rerun configuration scoped only to Claude, and inspect both agents’ MCP lists. Codex must retain a working registration. Also confirm that configuring a service cannot replace an unrelated Cursor entry with the same leaf. ScopeAll changed files support short naming, migration, ownership checks, or their test isolation. No clearly unrelated changes were found. The blockers concern correctness within the stated outcome, not scope expansion. Automated review by Polly · workflow run |
Co-authored-by: Isaac <no-reply@databricks.com>
…b.com/nachill-databricks/ucode into achille/short-mcp-service-entry-names
|
/ug-review |
| config = _read_claude_config_for_rewrite(claude_mcp_config_path()) | ||
| servers = (config or {}).get("mcpServers") |
There was a problem hiding this comment.
what happens if ~/.claude.json doesn't parse? we return {} so a dev's github doesn't count as a clash, and the CLI fallback in write_user_mcp_servers can overwrite it. can we treat unreadable as unknown and fall back to the legacy 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 |
There was a problem hiding this comment.
same as claude: an unparseable config.toml gives {}, so ug can take a dev's server name and the codex mcp add fallback overwrites it. can we tell the caller the file was unreadable?
| ``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))} |
There was a problem hiding this comment.
what happens if ug's exa is registered only for claude and codex has a hand-added exa? the name-only skip hides the codex one, so the next run with codex overwrites it. can we skip only per client (server["clients"])?
Suggestion:
| ug_names = {name for server in ug_servers if (name := _server_name(server))} | |
| ug_names_by_client = { | |
| client: {n for s in ug_servers if (n := _server_name(s)) and client in (s.get("clients") or [])} | |
| for client in clients | |
| } |
| # 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 | ||
| ) |
There was a problem hiding this comment.
under mcp add, the drop now runs after the union, so an original entry whose name or URL matches a managed one gets unregistered, and removed is forced to [] so the summary doesn't show it. that breaks "add never removes". can we drop only renamed or new entries? same thing happens at the other two call sites.
| 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 == [] |
There was a problem hiding this comment.
this locks in remove-before-add: if the add fails during upgrade, the user has neither system-ai-github nor github until a manual rerun. can we add first (or roll back) and assert the legacy entry survives?
| assert state["mcp_servers"] == [] | ||
|
|
||
|
|
||
| class TestMcpServiceEntryNames: |
There was a problem hiding this comment.
are all these tests necessary? many of them call private helpers (_normalize_*, _mcp_service_choice, _drop_managed_servers) and overlap on clash cases. can we cut them down to a few end-to-end configure/add cases?
| 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): |
There was a problem hiding this comment.
this has ug take over a hand-added native github (same URL) and save it to ug state. a later mcp removal then deletes the dev's entry. can we test that, or leave it unowned?
| ) | ||
| assert [s["name"] for s in out] == ["github"] | ||
|
|
||
| def test_location_selector_passes_the_developers_entries_for_clash_checks(self, monkeypatch): |
There was a problem hiding this comment.
what happens to managed entries an older ug registered as system-ai-* under a location selector? they now resolve to the leaf. can we add a reconcile test that the dashed managed entry is removed and not duplicated?
Polly AI ReviewBlocking issues
Security vulnerabilitiesNo additional demonstrated security vulnerabilities found. The ownership failures above cause configuration loss; no credential redirection or disclosure was established. This assessment is static, not live security validation. Non-blocking notes
ApproachCentral naming and shared URL extraction fit the existing module boundaries. A safer implementation would build one URL-keyed migration plan, reserve final names, and track ownership per client before applying changes. Reuse The repeated name-to-URL construction at SummaryDiff size (computed): 6 files; +870 / -68 lines (938 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe change introduces short service-entry names, native Claude/Codex config readers, collision handling, legacy migration, and URL-aware managed/picker reconciliation. It also adjusts test isolation and adds naming, migration, reader, and reconciliation coverage. No documentation, dependency, or generated-file changes are included. The breadth largely follows from the stateful migration requirement, rather than unrelated work. Concrete reductions are sharing the repeated URL-map construction and consolidating the overlapping single-holder collision scenarios. The blocking migration and ownership defects need resolution before approval. TestsTest-by-test assessmentBoth trusted policy files and the frozen diff were read in full; the supplied checksum matched. Head-line references come from the frozen diff, with surrounding context checked against the trusted base checkout. No tests, lint, typecheck, live workspace calls, or PR-authored code were executed. The reported 253 passing tests and lint result are author-provided evidence.
The inventory accounts for added and modified tests, the changed fixture, and the changed reconciliation helper. No test functions were deleted. Existing coverage establishes ordinary naming, normalization, picker classification, and failed-rename recovery. It does not suffice for the four blocking transitions. The smallest useful regressions are:
These do not require new live E2Es. Final-name uniqueness and persisted coexistence remain conditional boundaries described in the notes. The changed coverage also does not demonstrate malformed Codex TOML or the native-HTTP branch of the existing managed Codex reader. After fixes, run ScopeAll six changed files support the stated naming, collision-avoidance, or migration outcome. Native readers, URL-aware managed exclusion, and test isolation are necessary supporting changes, not independent features. No clearly unrelated changes were found. The unresolved questions concern supported naming inputs and the extent of the coexistence guarantee, not unrelated scope. Automated review by Polly · workflow run |
… entries Co-authored-by: Isaac <no-reply@databricks.com>
|
/ug-review |
| ) | ||
| working = _resolve_managed_mcp_servers(selector, workspace, profile, clients) | ||
| working = _resolve_managed_mcp_servers( | ||
| selector, workspace, profile, clients, [*(state.get("mcp_servers") or []), *previous] |
There was a problem hiding this comment.
previous holds only the fallback entries (working_fallback). Entries already in the managed file (e.g. system-ai-github) aren't in it, so on upgrade they get renamed to github. Also, because personal entries come first, a personal entry's name wins over the managed one for the same URL. Can we pass managed_mcp_servers(state) and put it first?
| service_names = _mcp_service_selection_names( | ||
| [s.removeprefix(MCP_ADD_PREFIX) for s in selections if s.startswith(MCP_ADD_PREFIX)], |
There was a problem hiding this comment.
what happens if a connection github (registered under the bare name github) is picked with system.ai.github in the same run? Both resolve to github, and the working_names dedup silently drops the second one. Can the leaf check also cover the other selections' entry names?
Co-authored-by: Isaac <no-reply@databricks.com>
Polly AI ReviewBlocking issues
Security vulnerabilitiesNo introduced security vulnerabilities found. Non-blocking notes
ApproachThe shared allocator and reused native-config readers fit existing boundaries. Use unfiltered SummaryDiff size (computed): 5 files; +326 / -46 lines (372 changed text lines); 0 binary files.
Shares use additions + deletions, including generated text and lockfiles. Binary files have no line count. Tests include fixtures/helpers under test paths and colocated test/spec files; categories are path-based, not coverage measurements. ChangesThe change assigns short names to new UC MCP registrations, adds collision checks against existing configurations, and updates picker recognition. It also adds native-config readers, test isolation, and regression scenarios. No documentation, dependency, or generated-file changes are included. The implementation is proportional to the goal. The concrete reduction opportunity is duplicated test setup and scenarios, assessed below. Existing OS-managed name preservation and cross-client managed reservations need correction before merge. TestsTest-by-test assessmentTests were not executed during this review. The reported test results are PR-author evidence, not independently verified results. The table accounts for all changed test functions and support. No test functions were deleted.
Required regression coverage for the blocking issues:
Existing append, replacement, dashed-picker, and discovery-failure tests provide useful surrounding coverage. Changing the managed-skip fixture from a placeholder URL to the actual service URL does not remove a valid service-identity guarantee. Additional live E2E tests are unnecessary for these allocation rules. After fixes, run ScopeAll changes support the stated short-name outcome, including native-config isolation and updated expectations. No unrelated changes were found. The blocking issues concern incomplete preservation and reservation behavior within that scope. Automated review by Polly · workflow run |
2 participants
Problem
UC MCP services are registered in coding-agent configs under the dashed full name (
main-default-exa), which is long and noisy in agent UIs.Change
A new UC MCP service gets its short leaf name (
main.default.exabecomesexa,system.ai.githubbecomesgithub) only when that name is completely free:Otherwise the entry gets the dashed full name, exactly as on
main.Existing entries are never renamed or removed. There is no in-place migration: existing dashed entries keep working as they are, and a service already registered (matched by URL, in ug state or managed) keeps its current name on every rerun. Only new registrations get the short name. URLs are unchanged.
The picker recognises a service registered under either name (short or dashed), so it does not offer it again.
Before / after
Real runs in a sandbox HOME against a live workspace; workspace hostname and catalog name replaced with and . These were taken with the earlier version of this PR; for a fresh registration of a free name the result is the same.
Claude Code
/mcp, before (main):After (this branch): the entry is named
figma:ug mcp add+claude mcp list, before:After:
Testing
uv run pytest tests/test_mcp.py -q: 259 passed. NewTestMcpServiceEntryNamesdrivesug mcp add --locationend to end: free leaf gets the short name; leaf taken in ug state, by a managed entry, in the native Claude config, or in the native Codex config gets the dashed name; an unparseable native config gives dashed names; two services sharing a leaf both get dashed names; reruns leave existing short and dashed entries untouched; an already-managed service is skipped, not duplicated.uvx --from rust-just just lint: ruff check, ruff format --check and ty check all pass.tests/test_agents_init.py::TestInstallToolBinary::test_required_update_*andtests/test_e2e_user_agent.py, which also fail onmain.Not yet verified
ug mcp addafter this simplification (the screenshots above predate it).This pull request and its description were written by Isaac.